[PATCH v4] hid-logitech-hidpp: read battery voltage from newer devices

Subsystems: hid core layer, hid logitech drivers, hid++ logitech drivers, the rest

STALE2574d

2 messages, 2 authors, 2019-08-23 · open the first message on its own page

[PATCH v4] hid-logitech-hidpp: read battery voltage from newer devices

From: Pedro Vanzella <hidden>
Date: 2019-08-23 15:50:59

Newer Logitech mice report their battery voltage through feature 0x1001
instead of the battery levels through feature 0x1000.

When the device is brought up and we try to query the battery, figure
out if it supports the old or the new feature. If it supports the new
feature, record the feature index and read the battery voltage and
its status.

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

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

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 <redacted>
---
 drivers/hid/hid-logitech-hidpp.c | 140 ++++++++++++++++++++++++++++++-
 1 file changed, 138 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 0179f7ed77e5..5c9c3133d2ae 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -87,6 +87,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
@@ -135,12 +136,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;
 };
 
@@ -1219,6 +1222,122 @@ 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 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,
@@ -1228,6 +1347,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,
@@ -1265,6 +1385,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;
@@ -3083,6 +3206,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) {
@@ -3204,12 +3330,16 @@ 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
+		else {
 			ret = hidpp20_query_battery_info(hidpp);
+			if (ret)
+				ret = hidpp20_query_battery_voltage_info(hidpp);
+		}
 
 		if (ret)
 			return ret;
@@ -3234,7 +3364,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++] =
@@ -3244,6 +3374,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;
@@ -3408,6 +3542,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.23.0

Re: [PATCH v4] hid-logitech-hidpp: read battery voltage from newer devices

From: Benjamin Tissoires <hidden>
Date: 2019-08-23 16:15:29

Hi Pedro,

On Fri, Aug 23, 2019 at 5:51 PM Pedro Vanzella [off-list ref] wrote:
Newer Logitech mice report their battery voltage through feature 0x1001
instead of the battery levels through feature 0x1000.

When the device is brought up and we try to query the battery, figure
out if it supports the old or the new feature. If it supports the new
feature, record the feature index and read the battery voltage and
its status.

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

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

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 <redacted>
I still have comments, please see inline:
quoted hunk
---
 drivers/hid/hid-logitech-hidpp.c | 140 ++++++++++++++++++++++++++++++-
 1 file changed, 138 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 0179f7ed77e5..5c9c3133d2ae 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -87,6 +87,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
@@ -135,12 +136,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;
 };
@@ -1219,6 +1222,122 @@ 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];
If I am not wrong, you can use get_unaligned_be16 here instead
+
+       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 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)
this should probably be defined in 0x1001 as
EVENT_BATTERY_VOLTAGE_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) {
There is no point in using an extra `changed` variable here, just
unwind the boolean operation above here.
quoted hunk
+               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,
@@ -1228,6 +1347,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,
@@ -1265,6 +1385,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;
@@ -3083,6 +3206,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) {
@@ -3204,12 +3330,16 @@ 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
+               else {
                        ret = hidpp20_query_battery_info(hidpp);
+                       if (ret)
+                               ret = hidpp20_query_battery_voltage_info(hidpp);
+               }
we should probably do the other way around: first test
hidpp20_query_battery_voltage_info and then go for the generic one, so
that the error returned will be the one from the most common feature,
and you will keep existing behaviour.
quoted hunk
                if (ret)
                        return ret;
@@ -3234,7 +3364,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++] =
@@ -3244,6 +3374,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;
@@ -3408,6 +3542,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);
I'd put the test above `hidpp20_query_battery_info(hidpp);` and would
add an else statement to fetch the battery info
        }
        if (hidpp->battery.ps)
                power_supply_changed(hidpp->battery.ps);
--
2.23.0
Cheers,
Benjamin
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help