Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1560647 > unrolled thread

[PATCH 00/17] Report power supply from hid-logitech-dj and others

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/17] Report power supply from hid-logitech-dj and others Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-17 15:40 +0100
    [PATCH 12/17] HID: logitech-hidpp: allow non HID++ devices to be handled by this module Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-17 15:40 +0100
    [PATCH 13/17] HID: logitech-hidpp: make .probe usbhid capable Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-17 15:40 +0100
      Re: [PATCH 13/17] HID: logitech-hidpp: make .probe usbhid capable Benjamin Tissoires <benjamin.tissoires@gmail.com> - 2017-01-18 10:30 +0100
        Re: [PATCH 13/17] HID: logitech-hidpp: make .probe usbhid capable Jiri Kosina <jikos@kernel.org> - 2017-01-19 12:00 +0100
          Re: [PATCH 13/17] HID: logitech-hidpp: make .probe usbhid capable Benjamin Tissoires <benjamin.tissoires@gmail.com> - 2017-01-19 12:20 +0100
    [PATCH 14/17] HID: logitech-hidpp: do not query the name through HID++ for 1.0 devices Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-17 15:40 +0100
    [PATCH 06/17] HID: logitech-hidpp: create the battery for all types of HID++ devices Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-17 15:40 +0100
    [PATCH 11/17] HID: logitech-hidpp: add a sysfs file to tell we support power_supply Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-17 15:40 +0100
    [PATCH 03/17] HID: logitech-hidpp: make sure we only register one battery per device Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-17 15:40 +0100
    [PATCH 02/17] HID: logitech-hidpp: Add scope to battery Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-17 15:40 +0100
      Re: [PATCH 02/17] HID: logitech-hidpp: Add scope to battery Bastien Nocera <hadess@hadess.net> - 2017-01-18 12:40 +0100
      Re: [PATCH 02/17] HID: logitech-hidpp: Add scope to battery Jiri Kosina <jikos@kernel.org> - 2017-01-20 14:50 +0100
        Re: [PATCH 02/17] HID: logitech-hidpp: Add scope to battery Jiri Kosina <jikos@kernel.org> - 2017-01-20 15:30 +0100
        Re: [PATCH 02/17] HID: logitech-hidpp: Add scope to battery Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-20 15:30 +0100
    [PATCH 15/17] HID: logitech-hidpp: rework probe path for unifying devices Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-17 15:40 +0100
    [PATCH 16/17] HID: logitech-hidpp: report battery for the G700 over wireless Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-17 15:40 +0100
    [PATCH 10/17] HID: logitech-hidpp: notify battery on connect Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-17 15:40 +0100
    [PATCH 04/17] HID: logitech-hidpp: battery: remove overloads and provide ONLINE Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-17 16:00 +0100
    [PATCH 07/17] HID: logitech-hidpp: return an error if the feature is not present Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-17 16:00 +0100
    [PATCH 01/17] HID: logitech-dj: allow devices to request full pairing information Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-17 16:00 +0100
    [PATCH 05/17] HID: logitech-hidpp: forward device info in power_supply Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-17 16:00 +0100
    Re: [PATCH 00/17] Report power supply from hid-logitech-dj and  others Bastien Nocera <hadess@hadess.net> - 2017-01-23 15:40 +0100
      Re: [PATCH 00/17] Report power supply from hid-logitech-dj and others Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-23 16:30 +0100
      Re: [PATCH 00/17] Report power supply from hid-logitech-dj and  others Bastien Nocera <hadess@hadess.net> - 2017-01-24 18:20 +0100

Page 2 of 2 — ← Prev page 1 [2]

#1560682 — [PATCH 01/17] HID: logitech-dj: allow devices to request full pairing information

Register 0xB5 should be handled specially no matter what function is
used. This allows to retrieve the serial and the Quad ID from
hid-logitech-hidpp directly.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-logitech-dj.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index 5bc6d80..71ce4ca 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -692,8 +692,12 @@ static void logi_dj_ll_close(struct hid_device *hid)
 	dbg_hid("%s:%s\n", __func__, hid->phys);
 }
 
-static u8 unifying_name_query[]  = {0x10, 0xff, 0x83, 0xb5, 0x40, 0x00, 0x00};
-static u8 unifying_name_answer[] = {0x11, 0xff, 0x83, 0xb5};
+/*
+ * Register 0xB5 is "pairing information". It is solely intended for the
+ * receiver, so do not overwrite the device index.
+ */
+static u8 unifying_pairing_query[]  = {0x10, 0xff, 0x83, 0xb5};
+static u8 unifying_pairing_answer[] = {0x11, 0xff, 0x83, 0xb5};
 
 static int logi_dj_ll_raw_request(struct hid_device *hid,
 				  unsigned char reportnum, __u8 *buf,
@@ -712,8 +716,8 @@ static int logi_dj_ll_raw_request(struct hid_device *hid,
 
 		/* special case where we should not overwrite
 		 * the device_index */
-		if (count == 7 && !memcmp(buf, unifying_name_query,
-					  sizeof(unifying_name_query)))
+		if (count == 7 && !memcmp(buf, unifying_pairing_query,
+					  sizeof(unifying_pairing_query)))
 			buf[4] |= djdev->device_index - 1;
 		else
 			buf[1] = djdev->device_index;
@@ -911,9 +915,8 @@ static int logi_dj_hidpp_event(struct hid_device *hdev,
 		/* special case were the device wants to know its unifying
 		 * name */
 		if (size == HIDPP_REPORT_LONG_LENGTH &&
-		    !memcmp(data, unifying_name_answer,
-			    sizeof(unifying_name_answer)) &&
-		    ((data[4] & 0xF0) == 0x40))
+		    !memcmp(data, unifying_pairing_answer,
+			    sizeof(unifying_pairing_answer)))
 			device_index = (data[4] & 0x0F) + 1;
 		else
 			return false;
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1560683 — [PATCH 05/17] HID: logitech-hidpp: forward device info in power_supply

Better forwarding the device name, manufacturer and serial to upower.
Note that serial is still empty, it will be filled in a later patch
in this series.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-logitech-hidpp.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 3c57886..77bfb65 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -758,6 +758,9 @@ static enum power_supply_property hidpp_battery_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_CAPACITY,
 	POWER_SUPPLY_PROP_SCOPE,
+	POWER_SUPPLY_PROP_MODEL_NAME,
+	POWER_SUPPLY_PROP_MANUFACTURER,
+	POWER_SUPPLY_PROP_SERIAL_NUMBER,
 };
 
 static int hidpp_battery_get_property(struct power_supply *psy,
@@ -783,6 +786,15 @@ static int hidpp_battery_get_property(struct power_supply *psy,
 					hidpp->battery.status ==
 					      POWER_SUPPLY_STATUS_FULL;
 			break;
+		case POWER_SUPPLY_PROP_MODEL_NAME:
+			val->strval = hidpp->hid_dev->name;
+			break;
+		case POWER_SUPPLY_PROP_MANUFACTURER:
+			val->strval = "Logitech";
+			break;
+		case POWER_SUPPLY_PROP_SERIAL_NUMBER:
+			val->strval = hidpp->hid_dev->uniq;
+			break;
 		default:
 			ret = -EINVAL;
 			break;
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1565005 — Re: [PATCH 00/17] Report power supply from hid-logitech-dj and others

On Tue, 2017-01-17 at 15:35 +0100, Benjamin Tissoires wrote:
> Hey guys,
> 
> I tried to revive the in-kernel battery support for HID++ devices.
> I was thinking of doing just a few patches, but in the end I had to
> do
> cleanups and some more tweaks...
> 
> So, the final result is that now hid-logitech-hidpp should allow to
> handle any HID++ device, no matter which connection it uses.
> I was able to test it on some unifying devices, some USB and
> Bluetooth,
> but I'd like to get the confirmation from Simon that I did not break
> the G920.
> 
> Other than that, I implemented most features asked by Bastien during
> the
> last round:
> - have a sysfs file to indicate we are capable of power_supply
> - use ONLINE capability (not sure if I mess something up or if Gnome
> handles
>   it correctly)
> - report product, serial and manufacturer
> - report K750 battery info (not Lux, sorry)
> - report HID++ 1.0 battery info
> 
> 
<snip>

I've tested your patches with the kernel build that you kindly
provided. The output of "upower -d", here[1], shows both a K750
keyboard (the one with the solar charging) and a T650 touchpad (which
was plugged in to a separate power supply when testing).

Here's a jumble of notes:
- UPower expects the serial number to be available when the device is
created. This wasn't the case for the keyboard here, and we end up with
no serial number, even though the serial_number sysfs file is now
populated
- the K750's battery state doesn't seem to match that found by the
UPower code, eg. it's stuck in "Unknown" when upower could detect that
it is charging (it's sunny here). That might also be why the icon is
stuck at "battery-missing-symbolic".
- the model names of the batteries seem to have manufacturer
information prepended, eg. vendor: Logitech model_name: Logitech K750
I'd have expected to only have "K750" there.
- the touchpad is detected as a random "battery", but that's likely due
to the slightly dodgy code in UPower (look for "try to detect using the
device type" and cringe)
- the serial number is in a different format than in UPower:
  kernel: 4101-6f-63-fd-39
  UPower: 6F63FD39

I'll look at updating the UPower code, thanks.

UPower's "power_supply_ class code:
https://cgit.freedesktop.org/upower/tree/src/linux/up-device-supply.c
and its HID++ support:
https://cgit.freedesktop.org/upower/tree/src/linux/up-device-unifying.c

[1]: https://paste.fedoraproject.org/535093/51785481

[toc] | [prev] | [next] | [standalone]

#1565061

On Jan 23 2017 or thereabouts, Bastien Nocera wrote:
> On Tue, 2017-01-17 at 15:35 +0100, Benjamin Tissoires wrote:
> > Hey guys,
> > 
> > I tried to revive the in-kernel battery support for HID++ devices.
> > I was thinking of doing just a few patches, but in the end I had to
> > do
> > cleanups and some more tweaks...
> > 
> > So, the final result is that now hid-logitech-hidpp should allow to
> > handle any HID++ device, no matter which connection it uses.
> > I was able to test it on some unifying devices, some USB and
> > Bluetooth,
> > but I'd like to get the confirmation from Simon that I did not break
> > the G920.
> > 
> > Other than that, I implemented most features asked by Bastien during
> > the
> > last round:
> > - have a sysfs file to indicate we are capable of power_supply
> > - use ONLINE capability (not sure if I mess something up or if Gnome
> > handles
> >   it correctly)
> > - report product, serial and manufacturer
> > - report K750 battery info (not Lux, sorry)
> > - report HID++ 1.0 battery info
> > 
> > 
> <snip>
> 
> I've tested your patches with the kernel build that you kindly
> provided. The output of "upower -d", here[1], shows both a K750
> keyboard (the one with the solar charging) and a T650 touchpad (which
> was plugged in to a separate power supply when testing).
> 
> Here's a jumble of notes:
> - UPower expects the serial number to be available when the device is
> created. This wasn't the case for the keyboard here, and we end up with
> no serial number, even though the serial_number sysfs file is now
> populated

I think I can fix that, but I think upower might need to get some tweaks
too. In the kernel, the way the sysfs files are created is decided by
power_supply core. We just set a list of properties and power_supply
creates the sysfs. I would say it creates the files with the order of
the property list we provide, thus the fixable state.

But this means that the order we declare the properties is rather
important because you are probably notified by one specific property,
which should be the last one in the list. I'd need to check into the
udev enumeration process, but we should probably enforce upower to not
handle the power_supply before it gets fully initialized (either in
power_supply core, or in udev, or in upower).

> - the K750's battery state doesn't seem to match that found by the
> UPower code, eg. it's stuck in "Unknown" when upower could detect that
> it is charging (it's sunny here). That might also be why the icon is
> stuck at "battery-missing-symbolic".

I'll look into that. But I tested it with 2 K750 here and both were
reporting charging... I wonder if this has to do with the enumeration
process too.

> - the model names of the batteries seem to have manufacturer
> information prepended, eg. vendor: Logitech model_name: Logitech K750
> I'd have expected to only have "K750" there.

That's fixable.

> - the touchpad is detected as a random "battery", but that's likely due
> to the slightly dodgy code in UPower (look for "try to detect using the
> device type" and cringe)

Yep, I'll cringe, for sure :)

> - the serial number is in a different format than in UPower:
>   kernel: 4101-6f-63-fd-39
>   UPower: 6F63FD39

Yes, the serial is the same format as the one reported on the Windows
application. The first 4 chars are the unifying PID, and the rest is the
same than yours. I like having the Quad ID (unifying PID): that way, you
are ensured to have a unique identifier across all Unifying devices.

Is it really an issue to change the serial?

> 
> I'll look at updating the UPower code, thanks.

Thank you for testing and reporting!

Cheers,
Benjamin

> 
> UPower's "power_supply_ class code:
> https://cgit.freedesktop.org/upower/tree/src/linux/up-device-supply.c
> and its HID++ support:
> https://cgit.freedesktop.org/upower/tree/src/linux/up-device-unifying.c
> 
> [1]: https://paste.fedoraproject.org/535093/51785481

[toc] | [prev] | [next] | [standalone]

#1566016 — Re: [PATCH 00/17] Report power supply from hid-logitech-dj and others

On Mon, 2017-01-23 at 15:35 +0100, Bastien Nocera wrote:
> On Tue, 2017-01-17 at 15:35 +0100, Benjamin Tissoires wrote:
> > Hey guys,
> > 
> > I tried to revive the in-kernel battery support for HID++ devices.
> > I was thinking of doing just a few patches, but in the end I had to
> > do
> > cleanups and some more tweaks...
> > 
> > So, the final result is that now hid-logitech-hidpp should allow to
> > handle any HID++ device, no matter which connection it uses.
> > I was able to test it on some unifying devices, some USB and
> > Bluetooth,
> > but I'd like to get the confirmation from Simon that I did not
> > break
> > the G920.
> > 
> > Other than that, I implemented most features asked by Bastien
> > during
> > the
> > last round:
> > - have a sysfs file to indicate we are capable of power_supply
> > - use ONLINE capability (not sure if I mess something up or if
> > Gnome
> > handles
> >   it correctly)
> > - report product, serial and manufacturer
> > - report K750 battery info (not Lux, sorry)
> > - report HID++ 1.0 battery info
> > 
> > 
> 
> <snip>
> 
> I've tested your patches with the kernel build that you kindly
> provided. The output of "upower -d", here[1], shows both a K750
> keyboard (the one with the solar charging) and a T650 touchpad (which
> was plugged in to a separate power supply when testing).
> 
> Here's a jumble of notes:
> - UPower expects the serial number to be available when the device is
> created. This wasn't the case for the keyboard here, and we end up
> with
> no serial number, even though the serial_number sysfs file is now
> populated

I think I've fixed this, the code in UPower didn't even try to read the
serial_number attribute, for "devices" (as opposed to the batteries
that power the computer).

> - the K750's battery state doesn't seem to match that found by the
> UPower code, eg. it's stuck in "Unknown" when upower could detect
> that
> it is charging (it's sunny here). That might also be why the icon is
> stuck at "battery-missing-symbolic".
> - the model names of the batteries seem to have manufacturer
> information prepended, eg. vendor: Logitech model_name: Logitech K750
> I'd have expected to only have "K750" there.
> - the touchpad is detected as a random "battery", but that's likely
> due
> to the slightly dodgy code in UPower (look for "try to detect using
> the
> device type" and cringe)

This is fixed in UPower master, and it's much nicer.

> - the serial number is in a different format than in UPower:
>   kernel: 4101-6f-63-fd-39
>   UPower: 6F63FD39
> 
> I'll look at updating the UPower code, thanks.
> 
> UPower's "power_supply_ class code:
> https://cgit.freedesktop.org/upower/tree/src/linux/up-device-supply.c
> and its HID++ support:
> https://cgit.freedesktop.org/upower/tree/src/linux/up-device-unifying
> .c
> 
> [1]: https://paste.fedoraproject.org/535093/51785481
> --
> To unsubscribe from this list: send the line "unsubscribe linux-
> input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web