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
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 <[email protected]>
---
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
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
> On Jun 5, 2019, at 6:24 PM, Filipe Laíns <[email protected]> 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
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 <[email protected]>
Thank you,
Filipe Laíns