[PATCH] HID: input: do not report stylus battery state as "full"

[PATCH] HID: input: do not report stylus battery state as "full"

[Dmitry Torokhov]

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

Re: [PATCH] HID: input: do not report stylus battery state as "full"

[Kenneth Albanowski]

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

Re: [PATCH] HID: input: do not report stylus battery state as "full"

[Benjamin Tissoires]

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
>

Re: [PATCH] HID: input: do not report stylus battery state as "full"

[Dmitry Torokhov]

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.

-- 
Dmitry

Re: [PATCH] HID: input: do not report stylus battery state as "full"

[Jiri Kosina]

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,

-- 
Jiri Kosina
SUSE Labs