Linux Input Archive on lore.kernel.org
 help / color / Atom feed
* [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; 2+ 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	[flat|nested] 2+ 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
  0 siblings, 0 replies; 2+ 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] 2+ messages in thread

end of thread, back to index

Thread overview: 2+ 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

Linux Input Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-input/0 linux-input/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-input linux-input/ https://lore.kernel.org/linux-input \
		linux-input@vger.kernel.org
	public-inbox-index linux-input

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-input


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git