* [PATCH] HID: logitech-hidpp: BatteryVoltage: only read chargeStatus if extPower is active
@ 2020-01-11 19:24 Filipe Laíns
2020-01-20 13:43 ` Pedro Vanzella
0 siblings, 1 reply; 4+ messages in thread
From: Filipe Laíns @ 2020-01-11 19:24 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Henrik Rydberg, linux-input,
linux-kernel, Pedro Vanzella
Cc: Filipe Laíns
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.
Example:
With an unplugged mouse
$ cat /sys/class/power_supply/hidpp_battery_0/status
Charging
This patch makes fixes that, it also renames charge_sts to flags as
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;
}
--
2.24.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] HID: logitech-hidpp: BatteryVoltage: only read chargeStatus if extPower is active
2020-01-11 19:24 [PATCH] HID: logitech-hidpp: BatteryVoltage: only read chargeStatus if extPower is active Filipe Laíns
@ 2020-01-20 13:43 ` Pedro Vanzella
2020-01-27 9:12 ` Benjamin Tissoires
0 siblings, 1 reply; 4+ messages in thread
From: Pedro Vanzella @ 2020-01-20 13:43 UTC (permalink / raw)
To: Filipe Laíns, Jiri Kosina, Benjamin Tissoires,
Henrik Rydberg, linux-input, linux-kernel
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.
>
> This patch makes fixes that, it also renames charge_sts to flags as
> 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>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] HID: logitech-hidpp: BatteryVoltage: only read chargeStatus if extPower is active
2020-01-20 13:43 ` Pedro Vanzella
@ 2020-01-27 9:12 ` Benjamin Tissoires
2020-01-28 15:25 ` Benjamin Tissoires
0 siblings, 1 reply; 4+ messages in thread
From: Benjamin Tissoires @ 2020-01-27 9:12 UTC (permalink / raw)
To: Pedro Vanzella
Cc: Filipe Laíns, Jiri Kosina, Henrik Rydberg,
open list:HID CORE LAYER, lkml
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
> > 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>
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] HID: logitech-hidpp: BatteryVoltage: only read chargeStatus if extPower is active
2020-01-27 9:12 ` Benjamin Tissoires
@ 2020-01-28 15:25 ` Benjamin Tissoires
0 siblings, 0 replies; 4+ messages in thread
From: Benjamin Tissoires @ 2020-01-28 15:25 UTC (permalink / raw)
To: Pedro Vanzella
Cc: Filipe Laíns, Jiri Kosina, Henrik Rydberg,
open list:HID CORE LAYER, lkml
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>
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-01-28 15:25 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-11 19:24 [PATCH] HID: logitech-hidpp: BatteryVoltage: only read chargeStatus if extPower is active Filipe Laíns
2020-01-20 13:43 ` Pedro Vanzella
2020-01-27 9:12 ` Benjamin Tissoires
2020-01-28 15:25 ` Benjamin Tissoires
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).