[PATCH v2 0/4] Read battery voltage from G403 and G900 mice

[PATCH v2 0/4] Read battery voltage from G403 and G900 mice

[Pedro Vanzella]

The gaming line of Logitech devices doesn't use the old hidpp20 feature
for battery level reporting. Instead, they report the current voltage
of the battery, in millivolts.

This patch set handles this case by adding a quirk to the devices we know
to have this new feature, in both wired and wireless mode.

This version of the patch set is better split, as well as adding the
quirk to make sure we don't needlessly probe every device connected.

Pedro Vanzella (4):
  HID: hid-logitech-hidpp: add quirk to handle battery voltage
  HID: hid-logitech-hidpp: add function to query battery voltage
  HID: hid-logitech-hidpp: report battery voltage to the power supply
  HID: hid-logitech-hidpp: subscribe to battery voltage events

 drivers/hid/hid-logitech-hidpp.c | 150 ++++++++++++++++++++++++++++++-
 1 file changed, 147 insertions(+), 3 deletions(-)

-- 
2.21.0

[PATCH v2 1/4] HID: hid-logitech-hidpp: add quirk to handle battery voltage

[Pedro Vanzella]

By adding this quirk we're able to handle battery voltage for devices
in both wired and wireless modes.

Signed-off-by: Pedro Vanzella <pedro@pedrovanzella.com>
---
 drivers/hid/hid-logitech-hidpp.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 72fc9c0566db..8b38c14725b8 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -63,7 +63,8 @@ MODULE_PARM_DESC(disable_tap_to_click,
 #define HIDPP_QUIRK_CLASS_G920			BIT(3)
 #define HIDPP_QUIRK_CLASS_K750			BIT(4)
 
-/* bits 2..20 are reserved for classes */
+/* bits 2..1f are reserved for classes */
+#define HIDPP_QUIRK_BATTERY_VOLTAGE_X1001	BIT(20)
 /* #define HIDPP_QUIRK_CONNECT_EVENTS		BIT(21) disabled */
 #define HIDPP_QUIRK_WTP_PHYSICAL_BUTTONS	BIT(22)
 #define HIDPP_QUIRK_NO_HIDINPUT			BIT(23)
@@ -3733,6 +3734,13 @@ static const struct hid_device_id hidpp_devices[] = {
 	  LDJ_DEVICE(0xb305),
 	  .driver_data = HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS },
 
+	{ /* Logitech G403 Gaming Mouse over Lightspeed */
+	  LDJ_DEVICE(0x405d),
+	  .driver_data = HIDPP_QUIRK_BATTERY_VOLTAGE_X1001 },
+	{ /* Logitech G900 Gaming Mouse over Lightspeed */
+	  LDJ_DEVICE(0x4053),
+	  .driver_data = HIDPP_QUIRK_BATTERY_VOLTAGE_X1001 },
+
 	{ LDJ_DEVICE(HID_ANY_ID) },
 
 	{ /* Keyboard LX501 (Y-RR53) */
@@ -3752,7 +3760,8 @@ static const struct hid_device_id hidpp_devices[] = {
 	{ /* Logitech G700 Gaming Mouse over USB */
 	  HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC06B) },
 	{ /* Logitech G900 Gaming Mouse over USB */
-	  HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC081) },
+	  HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC081),
+	  .driver_data = HIDPP_QUIRK_BATTERY_VOLTAGE_X1001 },
 	{ /* Logitech G920 Wheel over USB */
 	  HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G920_WHEEL),
 		.driver_data = HIDPP_QUIRK_CLASS_G920 | HIDPP_QUIRK_FORCE_OUTPUT_REPORTS},
-- 
2.21.0

[PATCH v2 2/4] HID: hid-logitech-hidpp: add function to query battery voltage

[Pedro Vanzella]

When the device is brought up, if it's one of the devices we know
supports battery voltage checking, figure out the feature index
and get the battery voltage and status.

If everything went correctly, record the fact that we're capable
of querying battery voltage.

Signed-off-by: Pedro Vanzella <pedro@pedrovanzella.com>
---
 drivers/hid/hid-logitech-hidpp.c | 94 ++++++++++++++++++++++++++++++++
 1 file changed, 94 insertions(+)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 8b38c14725b8..31e99363ab65 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -92,6 +92,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
 #define HIDPP_CAPABILITY_HIDPP20_BATTERY	BIT(1)
 #define HIDPP_CAPABILITY_BATTERY_MILEAGE	BIT(2)
 #define HIDPP_CAPABILITY_BATTERY_LEVEL_STATUS	BIT(3)
+#define HIDPP_CAPABILITY_BATTERY_VOLTAGE	BIT(4)
 
 /*
  * There are two hidpp protocols in use, the first version hidpp10 is known
@@ -140,12 +141,14 @@ struct hidpp_report {
 struct hidpp_battery {
 	u8 feature_index;
 	u8 solar_feature_index;
+	u8 voltage_feature_index;
 	struct power_supply_desc desc;
 	struct power_supply *ps;
 	char name[64];
 	int status;
 	int capacity;
 	int level;
+	int voltage; /* in millivolts */
 	bool online;
 };
 
@@ -1224,6 +1227,92 @@ static int hidpp20_battery_event(struct hidpp_device *hidpp,
 	return 0;
 }
 
+/* -------------------------------------------------------------------------- */
+/* 0x1001: Battery voltage                                                    */
+/* -------------------------------------------------------------------------- */
+
+#define HIDPP_PAGE_BATTERY_VOLTAGE 0x1001
+
+#define CMD_BATTERY_VOLTAGE_GET_BATTERY_VOLTAGE 0x00
+
+static int hidpp20_battery_map_status_voltage(u8 data[3], int *voltage)
+{
+	int status;
+
+	switch (data[2]) {
+	case 0x00: /* discharging */
+		status = POWER_SUPPLY_STATUS_DISCHARGING;
+		break;
+	case 0x10: /* wireless charging */
+	case 0x80: /* charging */
+		status = POWER_SUPPLY_STATUS_CHARGING;
+		break;
+	case 0x81: /* fully charged */
+		status = POWER_SUPPLY_STATUS_FULL;
+		break;
+	default:
+		status = POWER_SUPPLY_STATUS_NOT_CHARGING;
+	}
+
+	*voltage = (data[0] << 8) + data[1];
+
+	return status;
+}
+
+static int hidpp20_battery_get_battery_voltage(struct hidpp_device *hidpp,
+					       u8 feature_index,
+					       int *status, int *voltage)
+{
+	struct hidpp_report response;
+	int ret;
+	u8 *params = (u8 *)response.fap.params;
+
+	ret = hidpp_send_fap_command_sync(hidpp, feature_index,
+					  CMD_BATTERY_VOLTAGE_GET_BATTERY_VOLTAGE,
+					  NULL, 0, &response);
+
+	if (ret > 0) {
+		hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
+			__func__, ret);
+		return -EPROTO;
+	}
+	if (ret)
+		return ret;
+
+	hidpp->capabilities |= HIDPP_CAPABILITY_BATTERY_VOLTAGE;
+
+	*status = hidpp20_battery_map_status_voltage(params, voltage);
+
+	return 0;
+}
+
+static int hidpp20_query_battery_voltage_info(struct hidpp_device *hidpp)
+{
+	u8 feature_type;
+	int ret;
+	int status, voltage;
+
+	if (hidpp->battery.voltage_feature_index == 0xff) {
+		ret = hidpp_root_get_feature(hidpp, HIDPP_PAGE_BATTERY_VOLTAGE,
+					     &hidpp->battery.voltage_feature_index,
+					     &feature_type);
+		if (ret)
+			return ret;
+	}
+
+	ret = hidpp20_battery_get_battery_voltage(hidpp,
+						  hidpp->battery.voltage_feature_index,
+						  &status, &voltage);
+
+	if (ret)
+		return ret;
+
+	hidpp->battery.status = status;
+	hidpp->battery.voltage = voltage;
+	hidpp->battery.online = status != POWER_SUPPLY_STATUS_NOT_CHARGING;
+
+	return 0;
+}
 static enum power_supply_property hidpp_battery_props[] = {
 	POWER_SUPPLY_PROP_ONLINE,
 	POWER_SUPPLY_PROP_STATUS,
@@ -3209,10 +3298,13 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp)
 
 	hidpp->battery.feature_index = 0xff;
 	hidpp->battery.solar_feature_index = 0xff;
+	hidpp->battery.voltage_feature_index = 0xff;
 
 	if (hidpp->protocol_major >= 2) {
 		if (hidpp->quirks & HIDPP_QUIRK_CLASS_K750)
 			ret = hidpp_solar_request_battery_event(hidpp);
+		else if (hidpp->quirks & HIDPP_QUIRK_BATTERY_VOLTAGE_X1001)
+			ret = hidpp20_query_battery_voltage_info(hidpp);
 		else
 			ret = hidpp20_query_battery_info(hidpp);
 
@@ -3413,6 +3505,8 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
 			hidpp10_query_battery_status(hidpp);
 	} else if (hidpp->capabilities & HIDPP_CAPABILITY_HIDPP20_BATTERY) {
 		hidpp20_query_battery_info(hidpp);
+		if (hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_VOLTAGE)
+			hidpp20_query_battery_voltage_info(hidpp);
 	}
 	if (hidpp->battery.ps)
 		power_supply_changed(hidpp->battery.ps);
-- 
2.21.0

[PATCH v2 3/4] HID: hid-logitech-hidpp: report battery voltage to the power supply

[Pedro Vanzella]

If we know the device supports reading its voltage, report that.

Note that the protocol only gives us the current voltage in millivolts.

Signed-off-by: Pedro Vanzella <pedro@pedrovanzella.com>
---
 drivers/hid/hid-logitech-hidpp.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 31e99363ab65..d6c59b11b9d2 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -1322,6 +1322,7 @@ static enum power_supply_property hidpp_battery_props[] = {
 	POWER_SUPPLY_PROP_SERIAL_NUMBER,
 	0, /* placeholder for POWER_SUPPLY_PROP_CAPACITY, */
 	0, /* placeholder for POWER_SUPPLY_PROP_CAPACITY_LEVEL, */
+	0, /* placeholder for POWER_SUPPLY_PROP_VOLTAGE_NOW, */
 };
 
 static int hidpp_battery_get_property(struct power_supply *psy,
@@ -1359,6 +1360,9 @@ static int hidpp_battery_get_property(struct power_supply *psy,
 		case POWER_SUPPLY_PROP_SERIAL_NUMBER:
 			val->strval = hidpp->hid_dev->uniq;
 			break;
+		case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+			val->intval = hidpp->battery.voltage;
+			break;
 		default:
 			ret = -EINVAL;
 			break;
@@ -3331,7 +3335,7 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp)
 	if (!battery_props)
 		return -ENOMEM;
 
-	num_battery_props = ARRAY_SIZE(hidpp_battery_props) - 2;
+	num_battery_props = ARRAY_SIZE(hidpp_battery_props) - 3;
 
 	if (hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_MILEAGE)
 		battery_props[num_battery_props++] =
@@ -3341,6 +3345,10 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp)
 		battery_props[num_battery_props++] =
 				POWER_SUPPLY_PROP_CAPACITY_LEVEL;
 
+	if (hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_VOLTAGE)
+		battery_props[num_battery_props++] =
+			POWER_SUPPLY_PROP_VOLTAGE_NOW;
+
 	battery = &hidpp->battery;
 
 	n = atomic_inc_return(&battery_no) - 1;
-- 
2.21.0

[PATCH v2 4/4] HID: hid-logitech-hidpp: subscribe to battery voltage events

[Pedro Vanzella]

Like we do for other ways of interacting with the battery for other
devices, refresh the battery status and notify the power supply
subsystem of the changes in voltage and status.

Signed-off-by: Pedro Vanzella <pedro@pedrovanzella.com>
---
 drivers/hid/hid-logitech-hidpp.c | 33 ++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index d6c59b11b9d2..a37bd0834335 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -1313,6 +1313,36 @@ static int hidpp20_query_battery_voltage_info(struct hidpp_device *hidpp)
 
 	return 0;
 }
+
+static int hidpp20_battery_voltage_event(struct hidpp_device *hidpp,
+					 u8 *data, int size)
+{
+	struct hidpp_report *report = (struct hidpp_report *)data;
+	int status, voltage;
+	bool changed;
+
+	if (report->fap.feature_index != hidpp->battery.voltage_feature_index ||
+	    report->fap.funcindex_clientid !=
+		    EVENT_BATTERY_LEVEL_STATUS_BROADCAST)
+		return 0;
+
+	status = hidpp20_battery_map_status_voltage(report->fap.params,
+						    &voltage);
+
+	hidpp->battery.online = status != POWER_SUPPLY_STATUS_NOT_CHARGING;
+
+	changed = voltage != hidpp->battery.voltage ||
+		  status != hidpp->battery.status;
+
+	if (changed) {
+		hidpp->battery.voltage = voltage;
+		hidpp->battery.status = status;
+		if (hidpp->battery.ps)
+			power_supply_changed(hidpp->battery.ps);
+	}
+	return 0;
+}
+
 static enum power_supply_property hidpp_battery_props[] = {
 	POWER_SUPPLY_PROP_ONLINE,
 	POWER_SUPPLY_PROP_STATUS,
@@ -3181,6 +3211,9 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
 		ret = hidpp_solar_battery_event(hidpp, data, size);
 		if (ret != 0)
 			return ret;
+		ret = hidpp20_battery_voltage_event(hidpp, data, size);
+		if (ret != 0)
+			return ret;
 	}
 
 	if (hidpp->capabilities & HIDPP_CAPABILITY_HIDPP10_BATTERY) {
-- 
2.21.0

Re: [PATCH v2 0/4] Read battery voltage from G403 and G900 mice

[Filipe Laíns]

[-- Attachment #1: Type: text/plain, Size: 1348 bytes --]

On Wed, 2019-06-05 at 15:45 -0400, Pedro Vanzella wrote:
> The gaming line of Logitech devices doesn't use the old hidpp20
> feature
> for battery level reporting. Instead, they report the current voltage
> of the battery, in millivolts.
> 
> This patch set handles this case by adding a quirk to the devices we
> know
> to have this new feature, in both wired and wireless mode.
> 
> This version of the patch set is better split, as well as adding the
> quirk to make sure we don't needlessly probe every device connected.
> 
> Pedro Vanzella (4):
>   HID: hid-logitech-hidpp: add quirk to handle battery voltage
>   HID: hid-logitech-hidpp: add function to query battery voltage
>   HID: hid-logitech-hidpp: report battery voltage to the power supply
>   HID: hid-logitech-hidpp: subscribe to battery voltage events
> 
>  drivers/hid/hid-logitech-hidpp.c | 150
> ++++++++++++++++++++++++++++++-
>  1 file changed, 147 insertions(+), 3 deletions(-)
> 

Hello,

Why using quirks? 0x1001 is a feature, it should be discoverable in
IFeatureSet (0x0001). I don't understand the need to hardcode the
supported devices, HID++ exists specifically to prevent that.

Wasn't this what you started in your previous patch? Why move away from
it?

Thank you,
Filipe Laíns
3DCE 51D6 0930 EBA4 7858 BA41 46F6 33CB B0EB 4BF2

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 0/4] Read battery voltage from G403 and G900 mice

[Pedro Vanzella]

> On Jun 5, 2019, at 6:24 PM, Filipe Laíns <lains@archlinux.org> wrote:
> 
>> On Wed, 2019-06-05 at 15:45 -0400, Pedro Vanzella wrote:
>> The gaming line of Logitech devices doesn't use the old hidpp20
>> feature
>> for battery level reporting. Instead, they report the current voltage
>> of the battery, in millivolts.
>> 
>> This patch set handles this case by adding a quirk to the devices we
>> know
>> to have this new feature, in both wired and wireless mode.
>> 
>> This version of the patch set is better split, as well as adding the
>> quirk to make sure we don't needlessly probe every device connected.
>> 
>> Pedro Vanzella (4):
>>  HID: hid-logitech-hidpp: add quirk to handle battery voltage
>>  HID: hid-logitech-hidpp: add function to query battery voltage
>>  HID: hid-logitech-hidpp: report battery voltage to the power supply
>>  HID: hid-logitech-hidpp: subscribe to battery voltage events
>> 
>> drivers/hid/hid-logitech-hidpp.c | 150
>> ++++++++++++++++++++++++++++++-
>> 1 file changed, 147 insertions(+), 3 deletions(-)
>> 
> 
> Hello,
> 
> Why using quirks? 0x1001 is a feature, it should be discoverable in
> IFeatureSet (0x0001). I don't understand the need to hardcode the
> supported devices, HID++ exists specifically to prevent that.
> 
> Wasn't this what you started in your previous patch? Why move away from
> it?

I was asked to change to conform to the way the other features were handled. I’ll let the maintainers decide, but I agree with you that the other way was better.

In fact, since the kernel only needs to support about half a dozen features, we could refactor the probe function to, well, probe the device for those features and set the capability flags. It looks to me like that would be cleaner and easier to extend (and would make it easier to support future devices).

> Thank you,
> Filipe Laíns
> 3DCE 51D6 0930 EBA4 7858 BA41 46F6 33CB B0EB 4BF2

Re: [PATCH v2 0/4] Read battery voltage from G403 and G900 mice

[Filipe Laíns]

[-- Attachment #1: Type: text/plain, Size: 1238 bytes --]

On Wed, 2019-06-05 at 15:45 -0400, Pedro Vanzella wrote:
> The gaming line of Logitech devices doesn't use the old hidpp20
> feature
> for battery level reporting. Instead, they report the current voltage
> of the battery, in millivolts.
> 
> This patch set handles this case by adding a quirk to the devices we
> know
> to have this new feature, in both wired and wireless mode.
> 
> This version of the patch set is better split, as well as adding the
> quirk to make sure we don't needlessly probe every device connected.
> 
> Pedro Vanzella (4):
>   HID: hid-logitech-hidpp: add quirk to handle battery voltage
>   HID: hid-logitech-hidpp: add function to query battery voltage
>   HID: hid-logitech-hidpp: report battery voltage to the power supply
>   HID: hid-logitech-hidpp: subscribe to battery voltage events
> 
>  drivers/hid/hid-logitech-hidpp.c | 150
> ++++++++++++++++++++++++++++++-
>  1 file changed, 147 insertions(+), 3 deletions(-)
> 

This is OK. However since we now have a feature discovery routine I
think the code should use that instead of quirks. I will send a patch
making this change.

You can have my
Reviewed-by: Filipe Laíns <lains@archlinux.org>

Thank you,
Filipe Laíns

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]