[PATCH] rules: support Logitech Unifying in Linux 3.19

[Peter Wu]

On Thursday 22 January 2015 13:52:46 Benjamin Tissoires wrote:
> On Jan 21 2015 or thereabouts, Peter Wu wrote:
> > On Wednesday 21 January 2015 10:16:04 Benjamin Tissoires wrote:
> > > Hi Peter,
> > > 
> > > On Jan 21 2015 or thereabouts, Peter Wu wrote:
> > > > Linux 3.19 changed the way in which devices get registered by
> > > > introducing a hid-logitech-hidpp module. The driver name therefore needs
> > > > to be adjusted.
> > > > 
> > > > Signed-off-by: Peter Wu <peter at lekensteyn.nl>
> > > > ---
> > > > Hi,
> > > > 
> > > > This patch makes UPower recognize Logitech Unifying receivers again.
> > > > 
> > > > Benjamin, I have CC'ed you to let you know that there is possibly
> > > > userspace breakage due to the hid-logitech-hidpp rework.
> > > 
> > > Thanks for letting me know. I forgot about the upower handling, and the
> > > rule indeed needs a patch.
> > > 
> > > > 
> > > > Kind regards,
> > > > Peter
> > > > ---
> > > >  rules/95-upower-csr.rules | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/rules/95-upower-csr.rules b/rules/95-upower-csr.rules
> > > > index b476660..39539ca 100644
> > > > --- a/rules/95-upower-csr.rules
> > > > +++ b/rules/95-upower-csr.rules
> > > > @@ -25,5 +25,8 @@ SUBSYSTEM!="hid", GOTO="up_unifying_end"
> > > >  ATTRS{idVendor}=="046d", ENV{UPOWER_VENDOR}="Logitech, Inc."
> > > >  ATTRS{idVendor}=="046d", ATTRS{idProduct}=="c52b", DRIVER=="logitech-djdevice", ENV{UPOWER_BATTERY_TYPE}="unifying"
> > > >  ATTRS{idVendor}=="046d", ATTRS{idProduct}=="c532", DRIVER=="logitech-djdevice", ENV{UPOWER_BATTERY_TYPE}="unifying"
> > > > +# These devices bind to the hid-logitech-hidpp module since Linux 3.19
> > > > +ATTRS{idVendor}=="046d", ATTRS{idProduct}=="c52b", DRIVER=="logitech-hidpp-device", ENV{UPOWER_BATTERY_TYPE}="unifying"
> > > > +ATTRS{idVendor}=="046d", ATTRS{idProduct}=="c532", DRIVER=="logitech-hidpp-device", ENV{UPOWER_BATTERY_TYPE}="unifying"
> > > 
> > > This seems wrong.
> > > The Pids c52b and c532 are now solely used by the receiver. So they are
> > > using logitech-djreceiver, and so we should keep the match on the
> > > driver for the current string.
> > 
> > The 'S' in ATTRS matches parent devices so it will correctly match child
> > devices using the logitech-hidpp-device driver.
> > 
> > > OTOH, the devices changed of driver, but they are also now not using the
> > > PIDs from the receiver (well, nothing prevent Logitech from reusing c52b
> > > and c53b as a wireless PID).
> > > 
> > > So I think we should remove the PID match here too. The name of the
> > > driver and the VID match is sufficient to guarantee that the unifying
> > > protocol can be used.
> > 
> > I see what you mean, the receivers bind themselves to the two PIDs, but
> > the devices themselves are loaded based on the Wireless PIDs which are
> > read from the receiver. See v2 below (tested with a 0xc52b receiver).
> > > Cheers.
> > > Benjamin
> > > 
> > > PS: not sure I am clear enough in my explanations... It looks like it's
> > > difficult for me to align 2 thought properly this morning :)
> > > 
> > > >  ATTRS{idVendor}=="046d", ATTRS{idProduct}=="c52f", ENV{UPOWER_BATTERY_TYPE}="lg-wireless"
> > > >  LABEL="up_unifying_end"
> > > 
> > ---
> > From 00d5062b744abb7bd73d6a98f08ed5ab359e79ff Mon Sep 17 00:00:00 2001
> > From: Peter Wu <peter at lekensteyn.nl>
> > Date: Wed, 21 Jan 2015 15:48:47 +0100
> > Subject: [PATCH v2] rules: support Logitech Unifying in Linux 3.19
> > 
> > Linux 3.19 changed the way in which devices get registered by
> > introducing a hid-logitech-hidpp module. The driver name therefore needs
> > to be adjusted.
> > 
> > As the driver only binds to Logitech HID++ devices which are below a
> > receiver, it should be safe to drop the precise idProduct. These IDs
> > would require all Unifying devices to be connected through a receiver
> > which matches the two product IDs.
> > 
> > Signed-off-by: Peter Wu <peter at lekensteyn.nl>
> > ---
> >  rules/95-upower-csr.rules | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/rules/95-upower-csr.rules b/rules/95-upower-csr.rules
> > index b476660..22f3ee0 100644
> > --- a/rules/95-upower-csr.rules
> > +++ b/rules/95-upower-csr.rules
> > @@ -25,5 +25,7 @@ SUBSYSTEM!="hid", GOTO="up_unifying_end"
> >  ATTRS{idVendor}=="046d", ENV{UPOWER_VENDOR}="Logitech, Inc."
> >  ATTRS{idVendor}=="046d", ATTRS{idProduct}=="c52b", DRIVER=="logitech-djdevice", ENV{UPOWER_BATTERY_TYPE}="unifying"
> >  ATTRS{idVendor}=="046d", ATTRS{idProduct}=="c532", DRIVER=="logitech-djdevice", ENV{UPOWER_BATTERY_TYPE}="unifying"
> > +# These devices bind to the hid-logitech-hidpp module since Linux 3.19
> > +ATTRS{idVendor}=="046d", DRIVER=="logitech-hidpp-device", ENV{UPOWER_BATTERY_TYPE}="unifying"
> 
> By having the rule actually written made me wondered what would happen
> with a Bluetooth T651 touchpad. And actually, the battery_type gets
> assigned, but upower ignores it as this is not a dj device (it expects
> usb and a receiver as a parent, whereas bluetooth hidpp devices are on
> their own).
> 
> So this looks fine to me. When (if) the actual support of bluetooth
> hidpp devices gets added to upower, the rule will already be in place,
> so we should be good.

IIRC, the new hid-logitech-hidpp module allows you to write to the
hidraw devices to bypass the receiver. Bluetooth probably does not have
a receiver that understands the HID++ protocol, since the wireless
transport is handled by Bluetooth.

Looking at the implementation (up-device-unifying.c, hidpp-device.c) it
looks relatively simple to remove the assumption of a receiver.
Currently the receiver is used to read the device type
(mouse/keyboard/...), short model name ("M525", "K800") and a 32-bit
serial number.

At least the model name can now be read from sysfs. Would it make sense
to extend the hid-logitech-hidpp driver to add a HID_UNIQ field
matching the serial number?

> FWIW, Reviewed-by: Benjamin Tissoires <benjamin.tissoires at redhat.com>

Thanks!

Kind regards,
Peter

> Cheers,
> Benjamin
>
> >  ATTRS{idVendor}=="046d", ATTRS{idProduct}=="c52f", ENV{UPOWER_BATTERY_TYPE}="lg-wireless"
> >  LABEL="up_unifying_end"
>