| Message ID | YNtlrrKZVQY4byVa@google.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | f4abaa9eebde334045ed6ac4e564d050f1df3013 |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: input: do not report stylus battery state as "full" | expand |
I've tested it on a 5.4 derivative, and that works as expected. Looks good to me. - Kenneth Albanowski - Kenneth On Tue, Jun 29, 2021 at 11:25 AM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > The power supply states of discharging, charging, full, etc, represent > state of charging, not the capacity level of the battery (for which > we have a separate property). Current HID usage tables to not allow > for expressing charging state of the batteries found in generic > styli, so we should simply assume that the battery is discharging > even if current capacity is at 100% when battery strength reporting > is done via HID interface. In fact, we were doing just that before > commit 581c4484769e. > > This change helps UIs to not mis-represent fully charged batteries in > styli as being charging/topping-off. > > Fixes: 581c4484769e ("HID: input: map digitizer battery usage") > Reported-by: Kenneth Albanowski <kenalba@google.com> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > drivers/hid/hid-input.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c > index e982d8173c9c..e85a1a34ff39 100644 > --- a/drivers/hid/hid-input.c > +++ b/drivers/hid/hid-input.c > @@ -417,8 +417,6 @@ static int hidinput_get_battery_property(struct power_supply *psy, > > if (dev->battery_status == HID_BATTERY_UNKNOWN) > val->intval = POWER_SUPPLY_STATUS_UNKNOWN; > - else if (dev->battery_capacity == 100) > - val->intval = POWER_SUPPLY_STATUS_FULL; > else > val->intval = POWER_SUPPLY_STATUS_DISCHARGING; > break; > -- > 2.32.0.93.g670b81a890-goog > > -- > Dmitry
Hi Dmitry, On Tue, Jun 29, 2021 at 8:26 PM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > The power supply states of discharging, charging, full, etc, represent > state of charging, not the capacity level of the battery (for which > we have a separate property). Current HID usage tables to not allow > for expressing charging state of the batteries found in generic > styli, so we should simply assume that the battery is discharging > even if current capacity is at 100% when battery strength reporting > is done via HID interface. In fact, we were doing just that before > commit 581c4484769e. This commit is 4 year old already, so I'd like to have the opinion of Bastien on the matter for the upower side (or at least notify him). > > This change helps UIs to not mis-represent fully charged batteries in > styli as being charging/topping-off. > > Fixes: 581c4484769e ("HID: input: map digitizer battery usage") > Reported-by: Kenneth Albanowski <kenalba@google.com> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > drivers/hid/hid-input.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c > index e982d8173c9c..e85a1a34ff39 100644 > --- a/drivers/hid/hid-input.c > +++ b/drivers/hid/hid-input.c > @@ -417,8 +417,6 @@ static int hidinput_get_battery_property(struct power_supply *psy, > > if (dev->battery_status == HID_BATTERY_UNKNOWN) > val->intval = POWER_SUPPLY_STATUS_UNKNOWN; What's the point of keeping the HID_BATTERY_UNKNOWN code path? AFAICT, before 581c4484769e, we were just returning POWER_SUPPLY_STATUS_DISCHARGING. If we don't need to check for the capacity, I think we could also drop the hunk just before where we do the query of the capacity. Cheers, Benjamin > - else if (dev->battery_capacity == 100) > - val->intval = POWER_SUPPLY_STATUS_FULL; > else > val->intval = POWER_SUPPLY_STATUS_DISCHARGING; > break; > -- > 2.32.0.93.g670b81a890-goog > > -- > Dmitry >
Hi Benjamin, On Wed, Jun 30, 2021 at 09:09:27AM +0200, Benjamin Tissoires wrote: > Hi Dmitry, > > On Tue, Jun 29, 2021 at 8:26 PM Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > > > The power supply states of discharging, charging, full, etc, represent > > state of charging, not the capacity level of the battery (for which > > we have a separate property). Current HID usage tables to not allow > > for expressing charging state of the batteries found in generic > > styli, so we should simply assume that the battery is discharging > > even if current capacity is at 100% when battery strength reporting > > is done via HID interface. In fact, we were doing just that before > > commit 581c4484769e. > > This commit is 4 year old already, so I'd like to have the opinion of > Bastien on the matter for the upower side (or at least notify him). > > > > > This change helps UIs to not mis-represent fully charged batteries in > > styli as being charging/topping-off. > > > > Fixes: 581c4484769e ("HID: input: map digitizer battery usage") > > Reported-by: Kenneth Albanowski <kenalba@google.com> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > --- > > drivers/hid/hid-input.c | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c > > index e982d8173c9c..e85a1a34ff39 100644 > > --- a/drivers/hid/hid-input.c > > +++ b/drivers/hid/hid-input.c > > @@ -417,8 +417,6 @@ static int hidinput_get_battery_property(struct power_supply *psy, > > > > if (dev->battery_status == HID_BATTERY_UNKNOWN) > > val->intval = POWER_SUPPLY_STATUS_UNKNOWN; > > What's the point of keeping the HID_BATTERY_UNKNOWN code path? AFAICT, > before 581c4484769e, we were just returning > POWER_SUPPLY_STATUS_DISCHARGING. If we don't need to check for the > capacity, I think we could also drop the hunk just before where we do > the query of the capacity. I believe it is beneficial to keep this check: prior to 581c4484769e we were only handling batteries reported via generic device control - HID_DC_BATTERYSTRENGTH - essentially UPS batteries that normally can be queried at will. Stylus batteries are typically only reported when stylus is in contact with the digitzer, so until user actually engages stylus we do not have idea about its level/capacity. For this reason I think we should keep reporting POWER_SUPPLY_STATUS_UNKNOWN. Thanks.
On Tue, 29 Jun 2021, Dmitry Torokhov wrote: > The power supply states of discharging, charging, full, etc, represent > state of charging, not the capacity level of the battery (for which > we have a separate property). Current HID usage tables to not allow > for expressing charging state of the batteries found in generic > styli, so we should simply assume that the battery is discharging > even if current capacity is at 100% when battery strength reporting > is done via HID interface. In fact, we were doing just that before > commit 581c4484769e. > > This change helps UIs to not mis-represent fully charged batteries in > styli as being charging/topping-off. > > Fixes: 581c4484769e ("HID: input: map digitizer battery usage") > Reported-by: Kenneth Albanowski <kenalba@google.com> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> As there was no pushback from Bastien, I am queuing this for 5.15. Thanks,
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c index e982d8173c9c..e85a1a34ff39 100644 --- a/drivers/hid/hid-input.c +++ b/drivers/hid/hid-input.c @@ -417,8 +417,6 @@ static int hidinput_get_battery_property(struct power_supply *psy, if (dev->battery_status == HID_BATTERY_UNKNOWN) val->intval = POWER_SUPPLY_STATUS_UNKNOWN; - else if (dev->battery_capacity == 100) - val->intval = POWER_SUPPLY_STATUS_FULL; else val->intval = POWER_SUPPLY_STATUS_DISCHARGING; break;
The power supply states of discharging, charging, full, etc, represent state of charging, not the capacity level of the battery (for which we have a separate property). Current HID usage tables to not allow for expressing charging state of the batteries found in generic styli, so we should simply assume that the battery is discharging even if current capacity is at 100% when battery strength reporting is done via HID interface. In fact, we were doing just that before commit 581c4484769e. This change helps UIs to not mis-represent fully charged batteries in styli as being charging/topping-off. Fixes: 581c4484769e ("HID: input: map digitizer battery usage") Reported-by: Kenneth Albanowski <kenalba@google.com> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/hid/hid-input.c | 2 -- 1 file changed, 2 deletions(-)