Re: [PATCH] HID: logitech-hidpp: BatteryVoltage: only read chargeStatus if extPower is active

[Benjamin Tissoires <benjamin.tissoires@redhat.com>]

On Mon, Jan 27, 2020 at 10:12 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> Hi,
>
> On Mon, Jan 20, 2020 at 2:43 PM Pedro Vanzella <pedro@pedrovanzella.com> wrote:
> >
> > On 1/11/20 4:24 PM, Filipe Laíns wrote:
> > > In the HID++ 2.0 function getBatteryInfo() from the BatteryVoltage
> > > (0x1001) feature, chargeStatus is only valid if extPower is active.
> > >
> > > Previously we were ignoring extPower, which resulted in wrong values.
> >
> > Nice catch. Sorry for missing that the first time around.
> >
> > >
> > > Example:
> > >      With an unplugged mouse
> > >
> > >      $ cat /sys/class/power_supply/hidpp_battery_0/status
> > >      Charging
> >
> > Tested and it works as expected now.
>
> Thanks for the patch and the tests.
>
> Unfortunately, the merge window is already opened, and I'd rather not
> sneak this one right now. This patch doesn't seem very critical so I
> rather not annoy the other maintainers.
> I'll make sure it gets in the 5.6 final by pushing it into a rc when
> things are calmer for everybody.
>
> So the plan would be:
> - wait for the 'normal' 5.6 HID pull request to be sent
> - apply this one in for-5.6/upstream-fixes
> - sent this branch for either 5.6-rc1 or 5.6-rc2
>
> Cheers,
> Benjamin
>
> >
> > >
> > > This patch makes fixes that, it also renames charge_sts to flags as

Fixed the typo: 's/makes //' and pushed to for-5.6/upstream-fixes

Cheers,
Benjamin

> > > charge_sts can be confused with chargeStatus from the spec.
> > >
> > > Spec:
> > > +--------+-------------------------------------------------------------------------+
> > > |  byte  |                                    2                                    |
> > > +--------+--------------+------------+------------+----------+----------+----------+
> > > |   bit  |     0..2     |      3     |      4     |     5    |     6    |     7    |
> > > +--------+--------------+------------+------------+----------+----------+----------+
> > > | buffer | chargeStatus | fastCharge | slowCharge | critical | (unused) | extPower |
> > > +--------+--------------+------------+------------+----------+----------+----------+
> > > Table 1 - battery voltage (0x1001), getBatteryInfo() (ASE 0), 3rd byte
> > >
> > > +-------+--------------------------------------+
> > > | value |                meaning               |
> > > +-------+--------------------------------------+
> > > |   0   | Charging                             |
> > > +-------+--------------------------------------+
> > > |   1   | End of charge (100% charged)         |
> > > +-------+--------------------------------------+
> > > |   2   | Charge stopped (any "normal" reason) |
> > > +-------+--------------------------------------+
> > > |   7   | Hardware error                       |
> > > +-------+--------------------------------------+
> > > Table 2 - chargeStatus value
> > >
> > > Signed-off-by: Filipe Laíns <lains@archlinux.org>
> > > ---
> > >   drivers/hid/hid-logitech-hidpp.c | 43 ++++++++++++++++----------------
> > >   1 file changed, 21 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > > index bb063e7d48df..39a5ee0aaab0 100644
> > > --- a/drivers/hid/hid-logitech-hidpp.c
> > > +++ b/drivers/hid/hid-logitech-hidpp.c
> > > @@ -1256,36 +1256,35 @@ static int hidpp20_battery_map_status_voltage(u8 data[3], int *voltage,
> > >   {
> > >       int status;
> > >
> > > -     long charge_sts = (long)data[2];
> > > +     long flags = (long) data[2];
> > >
> > > -     *level = POWER_SUPPLY_CAPACITY_LEVEL_UNKNOWN;
> > > -     switch (data[2] & 0xe0) {
> > > -     case 0x00:
> > > -             status = POWER_SUPPLY_STATUS_CHARGING;
> > > -             break;
> > > -     case 0x20:
> > > -             status = POWER_SUPPLY_STATUS_FULL;
> > > -             *level = POWER_SUPPLY_CAPACITY_LEVEL_FULL;
> > > -             break;
> > > -     case 0x40:
> > > +     if (flags & 0x80)
> > > +             switch (flags & 0x07) {
> > > +             case 0:
> > > +                     status = POWER_SUPPLY_STATUS_CHARGING;
> > > +                     break;
> > > +             case 1:
> > > +                     status = POWER_SUPPLY_STATUS_FULL;
> > > +                     *level = POWER_SUPPLY_CAPACITY_LEVEL_FULL;
> > > +                     break;
> > > +             case 2:
> > > +                     status = POWER_SUPPLY_STATUS_NOT_CHARGING;
> > > +                     break;
> > > +             default:
> > > +                     status = POWER_SUPPLY_STATUS_UNKNOWN;
> > > +                     break;
> > > +             }
> > > +     else
> > >               status = POWER_SUPPLY_STATUS_DISCHARGING;
> > > -             break;
> > > -     case 0xe0:
> > > -             status = POWER_SUPPLY_STATUS_NOT_CHARGING;
> > > -             break;
> > > -     default:
> > > -             status = POWER_SUPPLY_STATUS_UNKNOWN;
> > > -     }
> > >
> > >       *charge_type = POWER_SUPPLY_CHARGE_TYPE_STANDARD;
> > > -     if (test_bit(3, &charge_sts)) {
> > > +     if (test_bit(3, &flags)) {
> > >               *charge_type = POWER_SUPPLY_CHARGE_TYPE_FAST;
> > >       }
> > > -     if (test_bit(4, &charge_sts)) {
> > > +     if (test_bit(4, &flags)) {
> > >               *charge_type = POWER_SUPPLY_CHARGE_TYPE_TRICKLE;
> > >       }
> > > -
> > > -     if (test_bit(5, &charge_sts)) {
> > > +     if (test_bit(5, &flags)) {
> > >               *level = POWER_SUPPLY_CAPACITY_LEVEL_CRITICAL;
> > >       }
> > >
> > >
> >
> > Tested-by: Pedro Vanzella <pedro@pedrovanzella.com>
> > Reviewed-by: Pedro Vanzella <pedro@pedrovanzella.com>
> >