Hi Hans, On Wed, Apr 10, 2019 at 4:55 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > > Hi All, > > Here is v2 of Benjamin's and mine series to enumerate the devices behind > various non DJ receivers instead of relying on their HID proxy mode. Thanks for the re-spin. I spent a good amount of time today reviewing the series and testing it, and I am happy with it. There are a couple of nitpicks I'd like to see addressed, but otherwise, I think this is ready for 5.2. Checkpatch complains about 2 "ERROR: do not use assignment in if condition" in patches 19/37 and 27/37. Though I can see the benefit of having it that way, I think we should stick to checkpatch here as it is too tempting to say "hey, there's a typo here, let's replace `=` by `==`" Checkpatch also complains about: - trailing statements should be on next line: but I do not really mind those, the blocks it complains are clearer that way - lines over 80 characters -> these are fine in that context - Block comments use a trailing */ on a separate line -> please fix those (patches 7/37 and 8/37) as the rest of the code has the correct block comments style > > There are 3 major changes in v2: > > 1) Add a new HID group for 27 MHz devices, this has 2 advantages: > 1a) It allows the logitech-hidpp code to identity 27 Mhz devices and to > automatically enable some HID++ quirls for these, such as e.g. HID++ > mouse-wheel handling (fixing some hwheel issues) > 1b) Since the logitech-hidpp code can now identify these the logitech-dj > code for 27 MHz devices no longer needs separate 27 MHz descriptors for the > consumer and HID++ reports. I am not entirely convinced we need a new group. But OTOH, being able to quickly and reliably filter those legacy device is something that is useful. > > 2) Add support Logitech Bluetooth receivers in HID proxy mode, tested with a > MX5000 keyboard + receiver. This allows e.g. battery monitoring of the kbd. > I played today with a diNovo mini which has a slightly different Logitech BT receiver. This part works fine except for the integrated touchapd which provides reports with an ID 5. I locally hacked something to have the mouse working, but it's a hack. The only problem I have with integrating the diNovo mini is that hid-lg.c does an input_mapping for one key on the keyboard... And I think we will need it too in hid-logitech-hidpp :/ I also added the G700 locally, and a small modification to support unnumbered mouse reports of size 8 is required, but I can send that as a followup patch. Cheers, Benjamin > 3) Rework HID++ mouse-wheel and extra-buttons support making it more > generic and cleaner and add HID++ consumer-keys report. Using the HID++ > consumer keys reports fixes several keys not working on some kbds. > > > Per patch info, unlisted patches are unchanged: > > [PATCH 01/37] HID: quirks: do not blacklist Logitech devices > -New patch split of from a cleanup patch from Benjamin which was touching > many different drivers at once. This removes a dep on that patch. > > [PATCH 12/37] HID: logitech-dj: support sharing struct dj_receiver_dev between USB-interfaces > -Fixed calling hid_err on a NULL hdev > > [PATCH 16/37] HID: logitech-dj: add support for 27 MHz receivers > -Remove chunk to pick a better name for hidpp devices, see new patch below > -Add support for mouse on index 2, numeric-keypad on index 4 > -Use a new group for 27MHz devices > > [PATCH 20/37] HID: logitech-dj: pick a better name for non-unifying receivers > -New patch replacing the dropped chunk from patch 16 as well as replacing > "HID: logitech-hidpp: create a name based on the type if non available" > from v1 > > [PATCH 21/37] HID: logitech-dj: remove false-positive error ... > -New misc. fix patch > > [PATCH 22/37] HID: logitech-dj: make appending of the HID++ ... > [PATCH 23/37] HID: logitech-dj: add support for Logitech Bluetooth > -2 new patches adding support for Logitech BT receivers in HID proxy mode > > [PATCH 26/37] HID: logitech-hidpp: ignore very-short or empty > [PATCH 27/37] HID: logitech-hidpp: do not make failure to get the > [PATCH 28/37] HID: logitech-hidpp: remove double assignment from > [PATCH 29/37] HID: logitech-hidpp: remove unused > -4 new patches (misc. fixes / cleanups) > > [PATCH 31/37] HID: logitech-hidpp: handle devices attached to > -Add support for the new 27 MHz lg devices hid group > > [PATCH 32/37] HID: logitech-hidpp: do not hardcode very long > -This fixes logitech-hidpp rejecting the mx5000 keyboard when paired over BT > > [PATCH 34/37] HID: logitech-hidpp: make hidpp10_set_register_bit a > [PATCH 35/37] HID: logitech-hidpp: add support for HID++ 1.0 wheel > [PATCH 36/37] HID: logitech-hidpp: add support for HID++ 1.0 extra > [PATCH 37/37] HID: logitech-hidpp: add support for HID++ 1.0 > -Reworked HID++ mouse-wheel and extra-buttons support making it more > generic and add HID++ consumer-keys report support > > > > For reference here is the cover letter of v1 of the series: > > Here is a series Benjamin and I have been working on, before this series we > treat Logitech wireless mice / keyboards connected through a non-unifying > receiver as generic HID devices, using the generic HID emulation of the > receiver. This causes several problems: > > -We cannot properly support some special keys / buttons on some keyboards / > mice since we cannot properly identify / talk to the device behind the recv. > -We cannot monitor the battery in these devices > > This series addresses this by actually enumerating (and talking to) the > devices behind the receiver, rather then solely relying on the receiver's > generic HID emulation. > > Note this series applies on top of the: > "[PATCH] HID: force setting drvdata to NULL when removing the driver" > patch which Benjamin send out 2 days ago. > > Regards, > > Hans >
