| Message ID | 20220407115406.115112-1-hadess@hadess.net |
|---|---|
| State | Accepted |
| Commit | 6ff1cae2e30a79265bcce85f617663c480936ab0 |
| Headers | show |
| Series | HID: wacom: Correct power_supply type | expand |
On Thu, 7 Apr 2022, Bastien Nocera wrote: > POWER_SUPPLY_TYPE_USB seems to only ever be used by USB ports that are > used to charge the machine itself (so a "system" scope), like the > single USB port on a phone, rather than devices. > > The wacom_sys driver is the only driver that sets its device battery as > being a USB type, which doesn't seem correct based on its usage, so > switch it to be a battery type like all the other USB-connected devices. > > Signed-off-by: Bastien Nocera <hadess@hadess.net> > --- > drivers/hid/wacom_sys.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c > index 066c567dbaa2..620fe74f5676 100644 > --- a/drivers/hid/wacom_sys.c > +++ b/drivers/hid/wacom_sys.c > @@ -1777,7 +1777,7 @@ static int __wacom_initialize_battery(struct wacom *wacom, > bat_desc->get_property = wacom_battery_get_property; > sprintf(battery->bat_name, "wacom_battery_%ld", n); > bat_desc->name = battery->bat_name; > - bat_desc->type = POWER_SUPPLY_TYPE_USB; > + bat_desc->type = POWER_SUPPLY_TYPE_BATTERY; > bat_desc->use_for_apm = 0; > > ps_bat = devm_power_supply_register(dev, bat_desc, &psy_cfg); Thanks Bastien, makes sense. CCing Jason and Ping (the Wacom driver maintainers) to get their Ack.
It seems that the USB type was chosen to fit into a upower heuristic that still exists (see [1], [2]). Looking at the upower code I suspect that swapping to the Battery type will at least cause "UP_DEVICE_KIND_TABLET" to no longer be used for our dongle-based wireless devices (Bluetooth-based might still be fine though). We haven't sold dongle-based devices in a while, but they're definitely still out there. If the batteries in those devices are seen as system batteries that could cause a problem -- e.g. triggering hibernation when the tablet battery gets low. I think it would be wise to test this first to see if there's any obvious real-world fallout from the change... [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=96983296281507f049425b84f0d244c40d506eba [2]: https://cgit.freedesktop.org/upower/tree/src/linux/up-device-supply.c Jason --- Now instead of four in the eights place / you’ve got three, ‘Cause you added one / (That is to say, eight) to the two, / But you can’t take seven from three, / So you look at the sixty-fours.... On Thu, Apr 7, 2022 at 1:52 PM Bastien Nocera <hadess@hadess.net> wrote: > > POWER_SUPPLY_TYPE_USB seems to only ever be used by USB ports that are > used to charge the machine itself (so a "system" scope), like the > single USB port on a phone, rather than devices. > > The wacom_sys driver is the only driver that sets its device battery as > being a USB type, which doesn't seem correct based on its usage, so > switch it to be a battery type like all the other USB-connected devices. > > Signed-off-by: Bastien Nocera <hadess@hadess.net> > --- > drivers/hid/wacom_sys.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c > index 066c567dbaa2..620fe74f5676 100644 > --- a/drivers/hid/wacom_sys.c > +++ b/drivers/hid/wacom_sys.c > @@ -1777,7 +1777,7 @@ static int __wacom_initialize_battery(struct wacom *wacom, > bat_desc->get_property = wacom_battery_get_property; > sprintf(battery->bat_name, "wacom_battery_%ld", n); > bat_desc->name = battery->bat_name; > - bat_desc->type = POWER_SUPPLY_TYPE_USB; > + bat_desc->type = POWER_SUPPLY_TYPE_BATTERY; > bat_desc->use_for_apm = 0; > > ps_bat = devm_power_supply_register(dev, bat_desc, &psy_cfg); > -- > 2.35.1 >
Following up on my previous comment. I've been able to test this patch with both flavors of wireless interface. Both Bluetooth (Intuos Pro) and dongle-based (Intuos5) appear to have mostly-correct behavior while charging and discharging, even when the battery level gradually drops to zero. The misbehaviors I see appear to be limited to upower mis-categorizing the devices as an e.g. keyboard or generic battery rather than as a tablet. This leads to some slightly confusing UI issues (e.g. GNOME and KDE referring to the device incorrectly), but nothing too annoying. If upower is taught to recognize tablets under more circumstances those issues should disappear. Ping tells me you may have an Intuos4 Wireless, Bastien? Any additional testing you can do with that device would be appreciated, though even without it I'm personally comfortable enough to provide an ack: Acked-by: Jason Gerecke <jason.gerecke@wacom.com> Jason --- Now instead of four in the eights place / you’ve got three, ‘Cause you added one / (That is to say, eight) to the two, / But you can’t take seven from three, / So you look at the sixty-fours.... On Tue, Apr 12, 2022 at 1:53 AM Jiri Kosina <jikos@kernel.org> wrote: > > On Thu, 7 Apr 2022, Bastien Nocera wrote: > > > POWER_SUPPLY_TYPE_USB seems to only ever be used by USB ports that are > > used to charge the machine itself (so a "system" scope), like the > > single USB port on a phone, rather than devices. > > > > The wacom_sys driver is the only driver that sets its device battery as > > being a USB type, which doesn't seem correct based on its usage, so > > switch it to be a battery type like all the other USB-connected devices. > > > > Signed-off-by: Bastien Nocera <hadess@hadess.net> > > --- > > drivers/hid/wacom_sys.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c > > index 066c567dbaa2..620fe74f5676 100644 > > --- a/drivers/hid/wacom_sys.c > > +++ b/drivers/hid/wacom_sys.c > > @@ -1777,7 +1777,7 @@ static int __wacom_initialize_battery(struct wacom *wacom, > > bat_desc->get_property = wacom_battery_get_property; > > sprintf(battery->bat_name, "wacom_battery_%ld", n); > > bat_desc->name = battery->bat_name; > > - bat_desc->type = POWER_SUPPLY_TYPE_USB; > > + bat_desc->type = POWER_SUPPLY_TYPE_BATTERY; > > bat_desc->use_for_apm = 0; > > > > ps_bat = devm_power_supply_register(dev, bat_desc, &psy_cfg); > > Thanks Bastien, makes sense. CCing Jason and Ping (the Wacom driver > maintainers) to get their Ack. > > -- > Jiri Kosina > SUSE Labs >
On Mon, 2022-04-11 at 17:08 +0200, Jiri Kosina wrote: > On Thu, 7 Apr 2022, Bastien Nocera wrote: > > > POWER_SUPPLY_TYPE_USB seems to only ever be used by USB ports that > > are > > used to charge the machine itself (so a "system" scope), like the > > single USB port on a phone, rather than devices. > > > > The wacom_sys driver is the only driver that sets its device > > battery as > > being a USB type, which doesn't seem correct based on its usage, so > > switch it to be a battery type like all the other USB-connected > > devices. > > > > Signed-off-by: Bastien Nocera <hadess@hadess.net> > > --- > > drivers/hid/wacom_sys.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c > > index 066c567dbaa2..620fe74f5676 100644 > > --- a/drivers/hid/wacom_sys.c > > +++ b/drivers/hid/wacom_sys.c > > @@ -1777,7 +1777,7 @@ static int __wacom_initialize_battery(struct > > wacom *wacom, > > bat_desc->get_property = wacom_battery_get_property; > > sprintf(battery->bat_name, "wacom_battery_%ld", n); > > bat_desc->name = battery->bat_name; > > - bat_desc->type = POWER_SUPPLY_TYPE_USB; > > + bat_desc->type = POWER_SUPPLY_TYPE_BATTERY; > > bat_desc->use_for_apm = 0; > > > > ps_bat = devm_power_supply_register(dev, bat_desc, > > &psy_cfg); > > Thanks Bastien, makes sense. CCing Jason and Ping (the Wacom driver > maintainers) to get their Ack. Hey, I know both Jason and Ping, but their name didn't show up when running get_maintainers.pl and neither of their names are in the MAINTAINERS file. This probably needs fixing. Cheers
On Mon, 2022-04-11 at 11:51 -0700, Jason Gerecke wrote: > It seems that the USB type was chosen to fit into a upower heuristic > that still exists (see [1], [2]). This heuristic was introduced in a commit that reads: " Hardcode wacom battery devices as not power-supply devices We'll switch to a kernel property when the power_supply interface is fixed. " Might be time :) > Looking at the upower code I suspect > that swapping to the Battery type will at least cause > "UP_DEVICE_KIND_TABLET" to no longer be used for our dongle-based > wireless devices (Bluetooth-based might still be fine though). We > haven't sold dongle-based devices in a while, but they're definitely > still out there. If the batteries in those devices are seen as system > batteries that could cause a problem -- e.g. triggering hibernation > when the tablet battery gets low. Whatever the type of "power_supply", the device will never be detected as supplying the system, as the "scope" is correct. > I think it would be wise to test this first to see if there's any > obvious real-world fallout from the change... Worse case scenario, the tablet is detected as something other than a tablet. If that happens, please file a bug against upower and attach the output of "udevadm info --export-db" and CC: me on the issue, and I'll fix the detection. Cheers > > [1]: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=96983296281507f049425b84f0d244c40d506eba > [2]: > https://cgit.freedesktop.org/upower/tree/src/linux/up-device-supply.c > > Jason > --- > Now instead of four in the eights place / > you’ve got three, ‘Cause you added one / > (That is to say, eight) to the two, / > But you can’t take seven from three, / > So you look at the sixty-fours.... > > > > On Thu, Apr 7, 2022 at 1:52 PM Bastien Nocera <hadess@hadess.net> > wrote: > > > > POWER_SUPPLY_TYPE_USB seems to only ever be used by USB ports that > > are > > used to charge the machine itself (so a "system" scope), like the > > single USB port on a phone, rather than devices. > > > > The wacom_sys driver is the only driver that sets its device > > battery as > > being a USB type, which doesn't seem correct based on its usage, so > > switch it to be a battery type like all the other USB-connected > > devices. > > > > Signed-off-by: Bastien Nocera <hadess@hadess.net> > > --- > > drivers/hid/wacom_sys.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c > > index 066c567dbaa2..620fe74f5676 100644 > > --- a/drivers/hid/wacom_sys.c > > +++ b/drivers/hid/wacom_sys.c > > @@ -1777,7 +1777,7 @@ static int __wacom_initialize_battery(struct > > wacom *wacom, > > bat_desc->get_property = wacom_battery_get_property; > > sprintf(battery->bat_name, "wacom_battery_%ld", n); > > bat_desc->name = battery->bat_name; > > - bat_desc->type = POWER_SUPPLY_TYPE_USB; > > + bat_desc->type = POWER_SUPPLY_TYPE_BATTERY; > > bat_desc->use_for_apm = 0; > > > > ps_bat = devm_power_supply_register(dev, bat_desc, > > &psy_cfg); > > -- > > 2.35.1 > >
On Wed, 2022-04-13 at 07:59 -0700, Jason Gerecke wrote: > Following up on my previous comment. I've been able to test this > patch > with both flavors of wireless interface. Both Bluetooth (Intuos Pro) > and dongle-based (Intuos5) appear to have mostly-correct behavior > while charging and discharging, even when the battery level gradually > drops to zero. The misbehaviors I see appear to be limited to upower > mis-categorizing the devices as an e.g. keyboard or generic battery > rather than as a tablet. This leads to some slightly confusing UI > issues (e.g. GNOME and KDE referring to the device incorrectly), but > nothing too annoying. If upower is taught to recognize tablets under > more circumstances those issues should disappear. > > Ping tells me you may have an Intuos4 Wireless, Bastien? Any > additional testing you can do with that device would be appreciated, > though even without it I'm personally comfortable enough to provide > an > ack: > > Acked-by: Jason Gerecke <jason.gerecke@wacom.com> Only devices I have is the original Wacom Graphire Bluetooth, and a wired Intuos4 (PTK-640) which I think changed names not long after. This might be enough: https://gitlab.freedesktop.org/upower/upower/-/merge_requests/127 Please file an issue with the info discussed in the previous mail if it isn't. Cheers > > Jason > --- > Now instead of four in the eights place / > you’ve got three, ‘Cause you added one / > (That is to say, eight) to the two, / > But you can’t take seven from three, / > So you look at the sixty-fours.... > > On Tue, Apr 12, 2022 at 1:53 AM Jiri Kosina <jikos@kernel.org> wrote: > > > > On Thu, 7 Apr 2022, Bastien Nocera wrote: > > > > > POWER_SUPPLY_TYPE_USB seems to only ever be used by USB ports > > > that are > > > used to charge the machine itself (so a "system" scope), like the > > > single USB port on a phone, rather than devices. > > > > > > The wacom_sys driver is the only driver that sets its device > > > battery as > > > being a USB type, which doesn't seem correct based on its usage, > > > so > > > switch it to be a battery type like all the other USB-connected > > > devices. > > > > > > Signed-off-by: Bastien Nocera <hadess@hadess.net> > > > --- > > > drivers/hid/wacom_sys.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c > > > index 066c567dbaa2..620fe74f5676 100644 > > > --- a/drivers/hid/wacom_sys.c > > > +++ b/drivers/hid/wacom_sys.c > > > @@ -1777,7 +1777,7 @@ static int > > > __wacom_initialize_battery(struct wacom *wacom, > > > bat_desc->get_property = wacom_battery_get_property; > > > sprintf(battery->bat_name, "wacom_battery_%ld", n); > > > bat_desc->name = battery->bat_name; > > > - bat_desc->type = POWER_SUPPLY_TYPE_USB; > > > + bat_desc->type = POWER_SUPPLY_TYPE_BATTERY; > > > bat_desc->use_for_apm = 0; > > > > > > ps_bat = devm_power_supply_register(dev, bat_desc, > > > &psy_cfg); > > > > Thanks Bastien, makes sense. CCing Jason and Ping (the Wacom driver > > maintainers) to get their Ack. > > > > -- > > Jiri Kosina > > SUSE Labs > >
On Wed, 13 Apr 2022, Jason Gerecke wrote: > Following up on my previous comment. I've been able to test this patch > with both flavors of wireless interface. Both Bluetooth (Intuos Pro) > and dongle-based (Intuos5) appear to have mostly-correct behavior > while charging and discharging, even when the battery level gradually > drops to zero. The misbehaviors I see appear to be limited to upower > mis-categorizing the devices as an e.g. keyboard or generic battery > rather than as a tablet. This leads to some slightly confusing UI > issues (e.g. GNOME and KDE referring to the device incorrectly), but > nothing too annoying. If upower is taught to recognize tablets under > more circumstances those issues should disappear. > > Ping tells me you may have an Intuos4 Wireless, Bastien? Any > additional testing you can do with that device would be appreciated, > though even without it I'm personally comfortable enough to provide an > ack: > > Acked-by: Jason Gerecke <jason.gerecke@wacom.com> Thanks Jason, now applied.
On Wed, 20 Apr 2022, Bastien Nocera wrote: > > > POWER_SUPPLY_TYPE_USB seems to only ever be used by USB ports that > > > are > > > used to charge the machine itself (so a "system" scope), like the > > > single USB port on a phone, rather than devices. > > > > > > The wacom_sys driver is the only driver that sets its device > > > battery as > > > being a USB type, which doesn't seem correct based on its usage, so > > > switch it to be a battery type like all the other USB-connected > > > devices. > > > > > > Signed-off-by: Bastien Nocera <hadess@hadess.net> > > > --- > > > drivers/hid/wacom_sys.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c > > > index 066c567dbaa2..620fe74f5676 100644 > > > --- a/drivers/hid/wacom_sys.c > > > +++ b/drivers/hid/wacom_sys.c > > > @@ -1777,7 +1777,7 @@ static int __wacom_initialize_battery(struct > > > wacom *wacom, > > > bat_desc->get_property = wacom_battery_get_property; > > > sprintf(battery->bat_name, "wacom_battery_%ld", n); > > > bat_desc->name = battery->bat_name; > > > - bat_desc->type = POWER_SUPPLY_TYPE_USB; > > > + bat_desc->type = POWER_SUPPLY_TYPE_BATTERY; > > > bat_desc->use_for_apm = 0; > > > > > > ps_bat = devm_power_supply_register(dev, bat_desc, > > > &psy_cfg); > > > > Thanks Bastien, makes sense. CCing Jason and Ping (the Wacom driver > > maintainers) to get their Ack. > > Hey, > > I know both Jason and Ping, but their name didn't show up when running > get_maintainers.pl and neither of their names are in the MAINTAINERS > file. > > This probably needs fixing. Agreed. Wacom driver is by itself substantial enough in order to have MAINTAINERS entry. Jason, Ping, could you please send me a patch adding yourself as maintainers? Thanks,
On Wed, Apr 20, 2022 at 1:52 AM Bastien Nocera <hadess@hadess.net> wrote: > > On Wed, 2022-04-13 at 07:59 -0700, Jason Gerecke wrote: > > Following up on my previous comment. I've been able to test this > > patch > > with both flavors of wireless interface. Both Bluetooth (Intuos Pro) > > and dongle-based (Intuos5) appear to have mostly-correct behavior > > while charging and discharging, even when the battery level gradually > > drops to zero. The misbehaviors I see appear to be limited to upower > > mis-categorizing the devices as an e.g. keyboard or generic battery > > rather than as a tablet. This leads to some slightly confusing UI > > issues (e.g. GNOME and KDE referring to the device incorrectly), but > > nothing too annoying. If upower is taught to recognize tablets under > > more circumstances those issues should disappear. > > > > Ping tells me you may have an Intuos4 Wireless, Bastien? Any > > additional testing you can do with that device would be appreciated, > > though even without it I'm personally comfortable enough to provide > > an > > ack: > > > > Acked-by: Jason Gerecke <jason.gerecke@wacom.com> > > Only devices I have is the original Wacom Graphire Bluetooth, and a > wired Intuos4 (PTK-640) which I think changed names not long after. > > This might be enough: > https://gitlab.freedesktop.org/upower/upower/-/merge_requests/127 > > Please file an issue with the info discussed in the previous mail if it > isn't. > Thanks Bastien, it seems to do the trick for Bluetooth devices but needs a little more work for the dongle-based Intuos5. I've filed a new issue with some suggestions as requested: https://gitlab.freedesktop.org/upower/upower/-/issues/182 Jason --- Now instead of four in the eights place / you’ve got three, ‘Cause you added one / (That is to say, eight) to the two, / But you can’t take seven from three, / So you look at the sixty-fours.... > Cheers > > > > > Jason > > --- > > Now instead of four in the eights place / > > you’ve got three, ‘Cause you added one / > > (That is to say, eight) to the two, / > > But you can’t take seven from three, / > > So you look at the sixty-fours.... > > > > On Tue, Apr 12, 2022 at 1:53 AM Jiri Kosina <jikos@kernel.org> wrote: > > > > > > On Thu, 7 Apr 2022, Bastien Nocera wrote: > > > > > > > POWER_SUPPLY_TYPE_USB seems to only ever be used by USB ports > > > > that are > > > > used to charge the machine itself (so a "system" scope), like the > > > > single USB port on a phone, rather than devices. > > > > > > > > The wacom_sys driver is the only driver that sets its device > > > > battery as > > > > being a USB type, which doesn't seem correct based on its usage, > > > > so > > > > switch it to be a battery type like all the other USB-connected > > > > devices. > > > > > > > > Signed-off-by: Bastien Nocera <hadess@hadess.net> > > > > --- > > > > drivers/hid/wacom_sys.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c > > > > index 066c567dbaa2..620fe74f5676 100644 > > > > --- a/drivers/hid/wacom_sys.c > > > > +++ b/drivers/hid/wacom_sys.c > > > > @@ -1777,7 +1777,7 @@ static int > > > > __wacom_initialize_battery(struct wacom *wacom, > > > > bat_desc->get_property = wacom_battery_get_property; > > > > sprintf(battery->bat_name, "wacom_battery_%ld", n); > > > > bat_desc->name = battery->bat_name; > > > > - bat_desc->type = POWER_SUPPLY_TYPE_USB; > > > > + bat_desc->type = POWER_SUPPLY_TYPE_BATTERY; > > > > bat_desc->use_for_apm = 0; > > > > > > > > ps_bat = devm_power_supply_register(dev, bat_desc, > > > > &psy_cfg); > > > > > > Thanks Bastien, makes sense. CCing Jason and Ping (the Wacom driver > > > maintainers) to get their Ack. > > > > > > -- > > > Jiri Kosina > > > SUSE Labs > > > >
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c index 066c567dbaa2..620fe74f5676 100644 --- a/drivers/hid/wacom_sys.c +++ b/drivers/hid/wacom_sys.c @@ -1777,7 +1777,7 @@ static int __wacom_initialize_battery(struct wacom *wacom, bat_desc->get_property = wacom_battery_get_property; sprintf(battery->bat_name, "wacom_battery_%ld", n); bat_desc->name = battery->bat_name; - bat_desc->type = POWER_SUPPLY_TYPE_USB; + bat_desc->type = POWER_SUPPLY_TYPE_BATTERY; bat_desc->use_for_apm = 0; ps_bat = devm_power_supply_register(dev, bat_desc, &psy_cfg);
POWER_SUPPLY_TYPE_USB seems to only ever be used by USB ports that are used to charge the machine itself (so a "system" scope), like the single USB port on a phone, rather than devices. The wacom_sys driver is the only driver that sets its device battery as being a USB type, which doesn't seem correct based on its usage, so switch it to be a battery type like all the other USB-connected devices. Signed-off-by: Bastien Nocera <hadess@hadess.net> --- drivers/hid/wacom_sys.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)