* Re: [PATCH] HID: input: do not report stylus battery state as "full"
2021-06-29 18:25 [PATCH] HID: input: do not report stylus battery state as "full" Dmitry Torokhov
@ 2021-06-29 22:07 ` Kenneth Albanowski
2021-06-30 7:09 ` Benjamin Tissoires
2021-07-15 18:55 ` Jiri Kosina
2 siblings, 0 replies; 5+ messages in thread
From: Kenneth Albanowski @ 2021-06-29 22:07 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Jiri Kosina, Benjamin Tissoires, Hans de Goede, linux-input,
linux-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] HID: input: do not report stylus battery state as "full"
2021-06-29 18:25 [PATCH] HID: input: do not report stylus battery state as "full" Dmitry Torokhov
2021-06-29 22:07 ` Kenneth Albanowski
@ 2021-06-30 7:09 ` Benjamin Tissoires
2021-07-02 1:02 ` Dmitry Torokhov
2021-07-15 18:55 ` Jiri Kosina
2 siblings, 1 reply; 5+ messages in thread
From: Benjamin Tissoires @ 2021-06-30 7:09 UTC (permalink / raw)
To: Dmitry Torokhov, Bastien Nocera
Cc: Jiri Kosina, Hans de Goede, Kenneth Albanowski,
open list:HID CORE LAYER, lkml
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] HID: input: do not report stylus battery state as "full"
2021-06-30 7:09 ` Benjamin Tissoires
@ 2021-07-02 1:02 ` Dmitry Torokhov
0 siblings, 0 replies; 5+ messages in thread
From: Dmitry Torokhov @ 2021-07-02 1:02 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Bastien Nocera, Jiri Kosina, Hans de Goede, Kenneth Albanowski,
open list:HID CORE LAYER, lkml
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] HID: input: do not report stylus battery state as "full"
2021-06-29 18:25 [PATCH] HID: input: do not report stylus battery state as "full" Dmitry Torokhov
2021-06-29 22:07 ` Kenneth Albanowski
2021-06-30 7:09 ` Benjamin Tissoires
@ 2021-07-15 18:55 ` Jiri Kosina
2 siblings, 0 replies; 5+ messages in thread
From: Jiri Kosina @ 2021-07-15 18:55 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Benjamin Tissoires, Hans de Goede, Kenneth Albanowski,
linux-input, linux-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread