Hi Benjamin,
On 4/19/19 5:52 PM, Benjamin Tissoires wrote:
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
`==`"
Hmm, ok, refactoring this to not have the assignments in the if
is going to be not pretty, but as you wish.
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
Ok, will fix.
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.
Right, I use this in several hid-logitech-hidpp.c patches.
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 :/
Hmm, hid-lg.c also does mapping for a bunch of 27MHz keyboards,
see lg_wireless_mapping. I've prepared patch to add similar mappings
in hwdb. But if you re worried about this causing regressions then
we should probably add a similar set of mappings to hid-logitech-hidpp.c
Ideally we would drop the mapping from hid-lg.c at the same time, but
even though I've 7 27MHz receivers, spreading 4 different models, they
_all_ use the same vid:pid. I've been unable to find any receivers using
the other 2 PIDs marked as LG_WIRELESS in hid-lg.c . I strongly suspect
that the other 2 PIDs will work fine with the new 27MHz support too,
but first we need to find users who can confirm this. Once this is
confirmed lg_wireless_mapping can be dropped, so if we decide to
add this to hid-logitech-hidpp.c then in essence we are just
moving code, not adding it.
I can understand if you're worried about regressions for the 27Mhz kbds,
but IMHO just going with hwdb mappings is fine. The keys which need mapping
are mostly Fn +F1 .. Fn + F12 and the mappings of those are different
per keyboard model. So the current in kernel mappings are wrong as
often as they are right. OTOH not having any mappings means the keys
do not do anything, while with the mappings from lg_wireless_mapping
they at least do something...
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.
Sounds good.
Regards,
Hans
