Re: [PATCH v5] hid-logitech-hidpp: read battery voltage from newer devices
On 9/9/19 8:00 AM, Filipe Laíns wrote:
On Sat, 2019-08-31 at 13:56 -0400, Pedro Vanzella wrote:
+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 = get_unaligned_be16(data);
+
+ return status;
+}
Here's the missing specification:
++-+
| 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
I know this is already on the 5th revision but could you please change
hidpp20_battery_map_status_voltage() to properly handle the 3rd byte?
Also, if you could make sure those values are properly exported via
sysfs it would be great! We will need them for proper upower support.
Sure thing. I'll have a new patch in a few days.
Thanks for figuring that stuff out, by the way.
Sorry for not saying this in my first review. Since then I have done
some testing and I discovered that if we want to get accurate upower
support we will need the values exposed by the 3rd byte to be
exported properly.
I am not sure about the endianness of chargeStatus but you can find
that easily.
Thank you!
Filipe Laíns
[PATCH v5] hid-logitech-hidpp: read battery voltage from newer devices
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.
Since there are no known devices which implement both the old and the
new battery feature, we make sure to try the newer feature first and
only fall back to the old one if that fails.
Signed-off-by: Pedro Vanzella
---
drivers/hid/hid-logitech-hidpp.c | 141 ++-
1 file changed, 137 insertions(+), 4 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 0179f7ed77e5..2f215e5be001 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,118 @@ 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
+
+#define EVENT_BATTERY_VOLTAGE_STATUS_BROADCAST 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 = get_unaligned_be16(data);
+
+ 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-&
[PATCH v4] hid-logitech-hidpp: read battery voltage from newer devices
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
---
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,
+
Re: [Resubmit] Read battery voltage from Logitech Gaming mice
On 8/23/19 10:32 AM, Benjamin Tissoires wrote: On Fri, Aug 23, 2019 at 4:22 PM Pedro Vanzella wrote: Hi Benjamin, On 8/23/19 4:25 AM, Benjamin Tissoires wrote: Hi Pedro, On Thu, Aug 22, 2019 at 10:19 PM Pedro Vanzella wrote: Resumitting this after having rebased it against the latest changes. thanks for resubmitting. Sorry I wasn't able to provide feedback on the last revision No worries, I know how these things are. 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. So the quirk is in the end a bad idea after all. I had some chats with Filipe that made me realize this. I actually resubmitted by Filipe's request, since the patches weren't applying cleanly anymore. The idea was to apply these patches and in the future refactor the code to use the feature discovery routines. Re-reading our previous exchanges also made me understood why I wasn't happy with the initial submission: for every request the code was checking both features 0x1000 and 0x1001 when we can remember this once and for all during hidpp_initialize_battery(). Yeah I wasn't too happy about this either, but since there was nothing prohibiting some device to have both features enabled, I thought this wasn't too horrible. I honestly don't think we will find one device that has both features enabled. It doesn't make sense as the "new" feature allows for fine tuning in the software, which would be moot if you also enable the percentage. So I think we should remove the useless quirk in the end (bad idea from me, I concede), and instead during hidpp_initialize_battery() set the correct HIDPP_CAPABILITY_*. Not entirely sure if we should try to call 0x1000, or 0x1001 or if we should rely on the 0x0001 feature to know which feature is available, but this should be implementation detail. I like the idea of calling 0x0001 once on device boot much better. I think we could easily just fetch the location for the features we know about and want to deal with once only. This also makes sure we support every single device that supports this feature, so that is a huge plus. In fact, I think we should _not_ call 0x0001 on battery init, but only call battery init _after_ we called 0x0001 and discovered either 0x1000 or 0x1001 (or the solar battery feature, or any other one that might crop up in the feature). ack for that 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. It is for sure easy to review, but doesn't make much sense in the end. I think we should squash all the patches together as you are just adding one feature in the driver, and it is a little bit disturbing to first add the quirk that has no use, then set up the structs when they are not used, and so on, so forth. You're right. My first instinct was to send a single patch. As much as I tested this, I always feel like breaking the patch up post-facto will break a git bisect in the future and everyone will hate me :P as long as the patches are compiling and are not breaking, git bisect will not be a problem. However, we might end up with the last one, which is not very explicit in what it does as it just enables the features implemented previously. So we (you, me and Filipe) should probably come up with an action plan here. The way I see it there are two issues here: one is adding this feature, and the other is refactoring to use feature discovery for all features. There are advantages and disadvantages to doing one or another first and we might want to discuss that. By merging this first (probably after I resubmit it as a single squashed patch) we get to test it a bit better and have a usable feature sooner. Plenty of people have been requesting this and there is plenty of stuff that can be built on top of it, but only once this is actually merged I think. On the other hand, by first refactoring the rest of the code to use 0x0001 we avoid some rework on this patch. It should be minor, as most functions here do all the heavy lifting after the initial feature discovery, and are thus mostly independent from how that is done. I'm happy either way, so just let me know what you guys decide. I think we should merge your v3 squashed series with a slight autodetection during battery init, like the method you used in the v1. This would remove the quirk, but keep the straightforward commands when addressing battery data. Alright, I rebased against for-5.4/logitech to make sure, squashed everything and restored the detection code from v1, removing the quirk. Tested and it works on both wired and wireless on my G900. Relying on 0x
Re: [Resubmit] Read battery voltage from Logitech Gaming mice
Hi Benjamin, On 8/23/19 4:25 AM, Benjamin Tissoires wrote: Hi Pedro, On Thu, Aug 22, 2019 at 10:19 PM Pedro Vanzella wrote: Resumitting this after having rebased it against the latest changes. thanks for resubmitting. Sorry I wasn't able to provide feedback on the last revision No worries, I know how these things are. 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. So the quirk is in the end a bad idea after all. I had some chats with Filipe that made me realize this. I actually resubmitted by Filipe's request, since the patches weren't applying cleanly anymore. The idea was to apply these patches and in the future refactor the code to use the feature discovery routines. Re-reading our previous exchanges also made me understood why I wasn't happy with the initial submission: for every request the code was checking both features 0x1000 and 0x1001 when we can remember this once and for all during hidpp_initialize_battery(). Yeah I wasn't too happy about this either, but since there was nothing prohibiting some device to have both features enabled, I thought this wasn't too horrible. So I think we should remove the useless quirk in the end (bad idea from me, I concede), and instead during hidpp_initialize_battery() set the correct HIDPP_CAPABILITY_*. Not entirely sure if we should try to call 0x1000, or 0x1001 or if we should rely on the 0x0001 feature to know which feature is available, but this should be implementation detail. I like the idea of calling 0x0001 once on device boot much better. I think we could easily just fetch the location for the features we know about and want to deal with once only. This also makes sure we support every single device that supports this feature, so that is a huge plus. In fact, I think we should _not_ call 0x0001 on battery init, but only call battery init _after_ we called 0x0001 and discovered either 0x1000 or 0x1001 (or the solar battery feature, or any other one that might crop up in the feature). 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. It is for sure easy to review, but doesn't make much sense in the end. I think we should squash all the patches together as you are just adding one feature in the driver, and it is a little bit disturbing to first add the quirk that has no use, then set up the structs when they are not used, and so on, so forth. You're right. My first instinct was to send a single patch. As much as I tested this, I always feel like breaking the patch up post-facto will break a git bisect in the future and everyone will hate me :P So we (you, me and Filipe) should probably come up with an action plan here. The way I see it there are two issues here: one is adding this feature, and the other is refactoring to use feature discovery for all features. There are advantages and disadvantages to doing one or another first and we might want to discuss that. By merging this first (probably after I resubmit it as a single squashed patch) we get to test it a bit better and have a usable feature sooner. Plenty of people have been requesting this and there is plenty of stuff that can be built on top of it, but only once this is actually merged I think. On the other hand, by first refactoring the rest of the code to use 0x0001 we avoid some rework on this patch. It should be minor, as most functions here do all the heavy lifting after the initial feature discovery, and are thus mostly independent from how that is done. I'm happy either way, so just let me know what you guys decide. If you guys (or anyone else reading this on the public list, really) has any input - naming things being notoriosly hard, I'm actually happy with nitpicking - I'd appreciate it. On that note, come to think of it, I'm not 100% sure reporting the voltage in milivolts is the standard way. I looked through the docs, but found no solid guideline. It was either that or a float, so I think I made the right call here, but still. - Pedro Cheers, Benjamin
[PATCH v3 3/4] hid-logitech-hidpp: report battery voltage to the power supply
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
---
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 e1ddc9cdcc8b..06bee97d33b4 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -1319,6 +1319,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,
@@ -1356,6 +1357,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;
@@ -3328,7 +3332,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++] =
@@ -3338,6 +3342,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.23.0
[PATCH v3 4/4] hid-logitech-hidpp: subscribe to battery voltage events
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
---
drivers/hid/hid-logitech-hidpp.c | 32
1 file changed, 32 insertions(+)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 06bee97d33b4..9f09ed6abbad 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -1310,6 +1310,35 @@ 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,
@@ -3178,6 +3207,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.23.0
[PATCH v3 1/4] hid-logitech-hidpp: add quirk to handle battery voltage
This quirk allows us to pick which devices support the 0x1001 hidpp
feature to read the battery voltage.
Signed-off-by: Pedro Vanzella
---
drivers/hid/hid-logitech-hidpp.c | 16 +---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 0179f7ed77e5..402ddba93adc 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -59,7 +59,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_HIDINPUTBIT(23)
@@ -3732,6 +3733,13 @@ static const struct hid_device_id hidpp_devices[] = {
LDJ_DEVICE(0xb30b),
.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) */
@@ -3750,13 +3758,15 @@ static const struct hid_device_id hidpp_devices[] = {
{ L27MHZ_DEVICE(HID_ANY_ID) },
{ /* Logitech G403 Wireless Gaming Mouse over USB */
- HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC082) },
+ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC082),
+ .driver_data = HIDPP_QUIRK_BATTERY_VOLTAGE_X1001 },
{ /* Logitech G703 Gaming Mouse over USB */
HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC087) },
{ /* Logitech G703 Hero Gaming Mouse over USB */
HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC090) },
{ /* 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 G903 Gaming Mouse over USB */
HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC086) },
{ /* Logitech G903 Hero Gaming Mouse over USB */
--
2.23.0
[Resubmit] Read battery voltage from Logitech Gaming mice
Resumitting this after having rebased it against the latest changes. 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.
[PATCH v3 2/4] hid-logitech-hidpp: add function to query battery voltage
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
---
drivers/hid/hid-logitech-hidpp.c | 95
1 file changed, 95 insertions(+)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 402ddba93adc..e1ddc9cdcc8b 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -88,6 +88,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
@@ -136,12 +137,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;
};
@@ -1220,6 +1223,93 @@ 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,
@@ -3205,10 +3295,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_b
Re: [PATCH v2 0/4] Read battery voltage from G403 and G900 mice
> On Jun 5, 2019, at 6:24 PM, Filipe Laíns 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
[PATCH v2 2/4] HID: hid-logitech-hidpp: add function to query battery voltage
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
---
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_b
[PATCH v2 3/4] HID: hid-logitech-hidpp: report battery voltage to the power supply
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
---
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
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
---
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
[PATCH v2 1/4] HID: hid-logitech-hidpp: add quirk to handle battery voltage
By adding this quirk we're able to handle battery voltage for devices
in both wired and wireless modes.
Signed-off-by: Pedro Vanzella
---
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_HIDINPUTBIT(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 0/4] Read battery voltage from G403 and G900 mice
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
Re: [PATCH 2/2] HID: hid-logitech-hidpp: subscribe to battery voltage change events
Sorry for littering the list, but please ignore this patch set. I'll
have one that uses a quirk to detect the right devices in a little
while.
On 06/04, Pedro Vanzella wrote:
> Same as with the other ways of reporting battery status,
> fetch the battery voltage on raw hidpp events.
>
> Signed-off-by: Pedro Vanzella
> ---
> drivers/hid/hid-logitech-hidpp.c | 32
> 1 file changed, 32 insertions(+)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c
> b/drivers/hid/hid-logitech-hidpp.c
> index e68ea44b0d24..1eee206a0aed 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -1313,6 +1313,35 @@ 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 +3210,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
>
--
Pedro Vanzella
pedrovanzella.com
#include
Don't Panic
signature.asc
Description: PGP signature
[PATCH 2/2] HID: hid-logitech-hidpp: subscribe to battery voltage change events
Same as with the other ways of reporting battery status,
fetch the battery voltage on raw hidpp events.
Signed-off-by: Pedro Vanzella
---
drivers/hid/hid-logitech-hidpp.c | 32
1 file changed, 32 insertions(+)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index e68ea44b0d24..1eee206a0aed 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -1313,6 +1313,35 @@ 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 +3210,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
[PATCH 1/2] HID: hid-logitech-hidpp: Report battery voltage
Newer devices have a feature with id 0x1001 that reports the battery
voltage instead of the old feature with id 0x1000 that reports a
percentage.
This patch fetches and decodes this data for all devices that
support it.
Signed-off-by: Pedro Vanzella
---
drivers/hid/hid-logitech-hidpp.c | 108 ++-
1 file changed, 106 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 72fc9c0566db..e68ea44b0d24 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -91,6 +91,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
@@ -139,12 +140,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;
};
@@ -1223,6 +1226,93 @@ 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,
@@ -1232,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,
@@ -1269,6 +1360,9 @@ static int hidpp_batte
Re: [PATCH] HID: hid-logitech-hidpp: detect wireless lightspeed devices
On 05/28, Benjamin Tissoires wrote:
> On Tue, May 28, 2019 at 6:30 PM Pedro Vanzella
> wrote:
> >
> > Send a low device index when the device is connected via the lightspeed
> > receiver so that the receiver will pass the message along to the device
> > instead of responding. If we don't do that, we end up thinking it's a
> > hidpp10 device and miss out on all new features available to newer devices.
> >
> > This will enable correct detection of the following models:
> > G603, GPro, G305, G613, G900 and G903, and possibly others.
>
> Thanks for the patch.
Thanks for reviewing it :)
> However, there is already support for this receiver in Linus' tree:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/hid/hid-logitech-dj.c?id=f5fb57a74e88bd1788f57bf77d587c91d4dc9d57
>
> With kernel 5.2-rc1, the connected device should already be handled by
> hid-logitech-hidpp :)
Why are the wireless receivers handled by hid-logitech-dj and the wired
mice handled by hid-logitech-hidpp? They are, in the end, all hidpp
devices, and having them all handled by the -hidpp driver with a quirk
class would allow us to check for support for the battery voltage
feature, as it seems to be an either-or scenario here.
- Pedro
>
> Cheers,
> Benjamin
>
> >
> > Signed-off-by: Pedro Vanzella
> > ---
> > 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 72fc9c0566db..621fce141d9f 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -62,6 +62,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
> > #define HIDPP_QUIRK_CLASS_K400 BIT(2)
> > #define HIDPP_QUIRK_CLASS_G920 BIT(3)
> > #define HIDPP_QUIRK_CLASS_K750 BIT(4)
> > +#define HIDPP_QUIRK_CLASS_LIGHTSPEED BIT(5)
> >
> > /* bits 2..20 are reserved for classes */
> > /* #define HIDPP_QUIRK_CONNECT_EVENTS BIT(21) disabled */
> > @@ -236,7 +237,11 @@ static int __hidpp_send_report(struct hid_device *hdev,
> > * set the device_index as the receiver, it will be overwritten by
> > * hid_hw_request if needed
> > */
> > - hidpp_report->device_index = 0xff;
> > + if (hidpp->quirks & HIDPP_QUIRK_CLASS_LIGHTSPEED) {
> > + hidpp_report->device_index = 0x01;
> > + } else {
> > + hidpp_report->device_index = 0xff;
> > + }
> >
> > if (hidpp->quirks & HIDPP_QUIRK_FORCE_OUTPUT_REPORTS) {
> > ret = hid_hw_output_report(hdev, (u8 *)hidpp_report,
> > fields_count);
> > @@ -3753,6 +3758,9 @@ static const struct hid_device_id hidpp_devices[] = {
> > HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC06B) },
> > { /* Logitech G900 Gaming Mouse over USB */
> > HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC081) },
> > + { /* Logitech Gaming Mice over Lightspeed Receiver */
> > + HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC539),
> > + .driver_data = HIDPP_QUIRK_CLASS_LIGHTSPEED },
> > { /* 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
> >
--
Pedro Vanzella
pedrovanzella.com
#include
Don't Panic
signature.asc
Description: PGP signature
[PATCH] HID: hid-logitech-hidpp: detect wireless lightspeed devices
Send a low device index when the device is connected via the lightspeed
receiver so that the receiver will pass the message along to the device
instead of responding. If we don't do that, we end up thinking it's a
hidpp10 device and miss out on all new features available to newer devices.
This will enable correct detection of the following models:
G603, GPro, G305, G613, G900 and G903, and possibly others.
Signed-off-by: Pedro Vanzella
---
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 72fc9c0566db..621fce141d9f 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -62,6 +62,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
#define HIDPP_QUIRK_CLASS_K400 BIT(2)
#define HIDPP_QUIRK_CLASS_G920 BIT(3)
#define HIDPP_QUIRK_CLASS_K750 BIT(4)
+#define HIDPP_QUIRK_CLASS_LIGHTSPEED BIT(5)
/* bits 2..20 are reserved for classes */
/* #define HIDPP_QUIRK_CONNECT_EVENTS BIT(21) disabled */
@@ -236,7 +237,11 @@ static int __hidpp_send_report(struct hid_device *hdev,
* set the device_index as the receiver, it will be overwritten by
* hid_hw_request if needed
*/
- hidpp_report->device_index = 0xff;
+ if (hidpp->quirks & HIDPP_QUIRK_CLASS_LIGHTSPEED) {
+ hidpp_report->device_index = 0x01;
+ } else {
+ hidpp_report->device_index = 0xff;
+ }
if (hidpp->quirks & HIDPP_QUIRK_FORCE_OUTPUT_REPORTS) {
ret = hid_hw_output_report(hdev, (u8 *)hidpp_report,
fields_count);
@@ -3753,6 +3758,9 @@ static const struct hid_device_id hidpp_devices[] = {
HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC06B) },
{ /* Logitech G900 Gaming Mouse over USB */
HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC081) },
+ { /* Logitech Gaming Mice over Lightspeed Receiver */
+ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC539),
+ .driver_data = HIDPP_QUIRK_CLASS_LIGHTSPEED },
{ /* 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
