HID: elecom: rewrite report fixup for EX-G and future mice
PATCH 1/1 HID: elecom: rewrite report fixup for EX-G and future mice
This patch rewrites the mouse report fixup used for the DEFT and HUGE ELECOM trackballs in order to make it generic enough to fix other ELECOM mice with similar issues. This patch also uses this new report fixup function to fix the ELECOM EX-G trackball which has 6 physical buttons and a similar issue to the other two mice.
ELECOM's track record has so far shown that they like to re-use the same report descriptor for multiple different mice regardless of the number of buttons the mouse has. This means that the missing buttons on multiple mice can be fixed in one function without introducing phantom buttons which would in turn cause the number of mouse buttons to be misreported to userspace.
This patch drops the very verbose report descriptor "diff" comment for a more abridged yet hopefully just as informative generic version.
---
I've let this patch sit on the old thread [1] for two weeks now and apart from the removal of the diff nobody has said anything about it so I'm posting it as an actual patch now.
It includes a few small fixes that I missed when posting the old one and this patch is based on the for-4.16/hid-quirks-cleanup/_base branch for convenience. (Since the hid_have_special_driver array has moved to hid-quirks.c now.)
Once again ccing all the relevant parties. I already explained my reason for removing the long diff (unfortunately Diego's email about this got dropped from the list as it was a HTML email) and why my replacement is still as good.
This was tested with my EX-G but since I don't have a HUGE or DEFT I have to rely on the fact that as far as I could tell the two report descriptors are identical and as far as I can tell this change shouldn't cause the fixup to work any differently for the other two mice which were fixed. However, if one of the original authors of this fixup could test with their mice I would very much appreciate the re-assurance.
P.S. Is that #define MOUSE_BUTTONS_MAX appropriate?
[1]: https://marc.info/[email protected]
--- drivers/hid/Kconfig | 1 + drivers/hid/hid-elecom.c | 76 +++++++++++++++++++++++++----------------------- drivers/hid/hid-ids.h | 1 + drivers/hid/hid-quirks.c | 1 + 4 files changed, 43 insertions(+), 36 deletions(-)
drivers/hid/Kconfig +1 -0
@@ -280,6 +280,7 @@ config HID_ELECOM ---help--- Support for ELECOM devices: - BM084 Bluetooth Mouse + - EX-G Trackball (Wired) - DEFT Trackball (Wired and wireless) - HUGE Trackball (Wired and wireless)
drivers/hid/hid-elecom.c +40 -36
106 lines not on this page · raw message
drivers/hid/hid-ids.h +1 -0
@@ -370,6 +370,7 @@#define USB_VENDOR_ID_ELECOM 0x056e#define USB_DEVICE_ID_ELECOM_BM084 0x0061 +#define USB_DEVICE_ID_ELECOM_EX_G_WIRED 0x00fb#define USB_DEVICE_ID_ELECOM_DEFT_WIRED 0x00fe#define USB_DEVICE_ID_ELECOM_DEFT_WIRELESS 0x00ff#define USB_DEVICE_ID_ELECOM_HUGE_WIRED 0x010c
drivers/hid/hid-quirks.c +1 -0
@@ -335,6 +335,7 @@ static const struct hid_device_id hid_have_special_driver[] = {#endif #if IS_ENABLED(CONFIG_HID_ELECOM) { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_BM084) }, + { HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_EX_G_WIRED) }, { HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_DEFT_WIRED) }, { HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_DEFT_WIRELESS) }, { HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_HUGE_WIRED) },
Signature · 1 line
2.15.1
drivers/hid/hid-elecom.c line 43
Quoted 3 lines · Tomasz Kramkowski
drivers/hid/hid-elecom.c +3
-0+static void mouse_button_fixup (struct hid_device *hdev, +__u8 *rdesc,unsigned int *rsize, +int nbuttons)
I've just remembered what has been bugging me yesterday when I was reviewing this patch. I had come to the realisation (and then subsequently forgotten) that this function should probably return __u8 * and also get assigned to rdesc on the other end. It seems to me that it makes most sense to allow for the possibility (although slim) of this function eventually being expanded to actually replace the report descriptor (technically the full report descriptor contains a bunch of useless crap like INPUT reports for media keys and the FEATURE report which as far as I can tell is totally useless or may or may not be some tactic by ELECOM to future-proof their firmware).
The other option would be to make rsize not a pointer because it doesn't need to be. But that kind of makes the flow of the two functions somewhat inconsistent. I'm not sure if I'm alone in that feeling.
Anyway, I should have written this down when I first caught it, sorry for the noise. I'll let you guys review this patch and give any other feedback you might have and I'll try to get a v2 as soon as possible afterwards.
Signature · 1 line
Tomasz Kramkowski | GPG: 40B037BA0A5B8680 | Web: https://the-tk.com/
Jiri Kosina
maintainerQuoted 12 lines · Tomasz Kramkowski
drivers/hid/hid-elecom.c line 43
Quoted 3 lines · Tomasz Kramkowski
drivers/hid/hid-elecom.c +3
-0+static void mouse_button_fixup (struct hid_device *hdev, +__u8 *rdesc,unsigned int *rsize, +int nbuttons)I've just remembered what has been bugging me yesterday when I was reviewing this patch. I had come to the realisation (and then subsequently forgotten) that this function should probably return
__u8* and also get assigned to rdesc on the other end. It seems to me that it makes most sense to allow for the possibility (although slim) of this function eventually being expanded to actually replace the report descriptor
Sure, but you can extend the API of mouse_button_fixup() once such need arises; no need to pass data pointers around without having actual use for them.
Signature · 2 lines
Jiri Kosina SUSE Labs
Quoted 15 lines · Jiri Kosina
Quoted 12 lines · Tomasz Kramkowski
drivers/hid/hid-elecom.c line 43
Quoted 3 lines · Tomasz Kramkowski
drivers/hid/hid-elecom.c +3
-0+static void mouse_button_fixup (struct hid_device *hdev, +__u8 *rdesc,unsigned int *rsize, +int nbuttons)I've just remembered what has been bugging me yesterday when I was reviewing this patch. I had come to the realisation (and then subsequently forgotten) that this function should probably return
__u8* and also get assigned to rdesc on the other end. It seems to me that it makes most sense to allow for the possibility (although slim) of this function eventually being expanded to actually replace the report descriptor
Quoted 3 lines
Sure, but you can extend the API of
mouse_button_fixup()once such need arises; no need to pass data pointers around without having actual use for them.
Alright, that's fine. Anything else to change before I send a v2? Also, would you like v2 in-reply-to the root of this thread or as its own thread?
Signature · 1 line
Tomasz Kramkowski | GPG: 40B037BA0A5B8680 | Web: https://the-tk.com/
Jiri Kosina
maintainerQuoted 1 line · Tomasz Kramkowski
Alright, that's fine. Anything else to change before I send a v2?
Not from my side, I think we're good to go.
Quoted 2 lines
Also, would you like v2 in-reply-to the root of this thread or as its own thread?
Feel free to send it as a followup here. Thanks,
Signature · 2 lines
Jiri Kosina SUSE Labs
Tomasz please add the wireless version in your next patch, a web search shows it's called M-XT3DRBK and the USB ID is 0x00fc.
Thanks,
Alex
Quoted 15 lines · Jiri Kosina
Quoted 1 line · Tomasz Kramkowski
Alright, that's fine. Anything else to change before I send a v2?
Not from my side, I think we're good to go.
Quoted 2 lines
Also, would you like v2 in-reply-to the root of this thread or as its own thread?
Feel free to send it as a followup here. Thanks,
Signature · 2 lines
Jiri Kosina SUSE Labs
Quoted 2 lines · Alex Manoussakis
Tomasz please add the wireless version in your next patch, a web search shows it's called M-XT3DRBK and the USB ID is
0x00fc.
M-XT3DRBK is the full model number for the wireless EX-G but since the HUGE and DEFT both use their short names in the hid-ids macros then I'll call this one EX_G_WIRELESS so that it matches the naming style of everything else.
However, I'm a bit apprehensive about sending in a patch for a device I can't test. Even if it's most likely going to work. Do you know of anyone who can test this wireless EX-G variant?
I guess Jiri can chip in if he thinks this is not necessary. Technically this patch should sit in linux-next for a while (but reportedly few people actually run on that iirc) and the worst that could happen is that the checks in the report fixup function don't match the expected report and don't apply the fix, or (unlikely) some padding bits get exposed as buttons with no function.
Either way, I've made this change locally and will be submitting v2 when I have a bit more time later this week.
Quoted 2 lines
Thanks,
Alex
Signature · 1 line
Tomasz Kramkowski | GPG: 40B037BA0A5B8680 | Web: https://the-tk.com/
Quoted 5 lines · Tomasz Kramkowski
Quoted 2 lines · Alex Manoussakis
Tomasz please add the wireless version in your next patch, a web search shows it's called M-XT3DRBK and the USB ID is
0x00fc.I'll call this one
EX_G_WIRELESSso that it matches the naming style
Sounds fine.
Quoted 3 lines
However, I'm a bit apprehensive about sending in a patch for a device I can't test. Even if it's most likely going to work. Do you know of anyone who can test this wireless EX-G variant?
No, but I can imagine their disappointment when their device won't work when it could have :-) Most users aren't developers and won't submit kernel patches for their device. They'll just assume linux has subpar device support, so it's up to us to avoid this :-P
It would be a pity to miss the opportunity to add a device variant that will almost certainly work (and harmless in the extremely remote chance it doesn't).
Even if a Linux user with the wireless device is a developer and decides to fix it and sends a patch to add the wireless later it's a lot more work for everyone involved (and delay in getting it in mainline) compared to just making your work complete now!
I didn't test the wireless HUGE either, but added it anyway. I did see reports of people successfully using DKMS modules floating around the Internet with their wireless HUGE/DEFT devices. I even tried one of them meant for the *wireless* DEFT, and changed the usb ID and saw it worked for my *wired* HUGE. This shows how Elecom tries to make their different devices work in the same manner as much as possible (which makes sense as they won't want to complicate their own software either) making the wireless addition a no-brainer IMO.
Also the EX_G_WIRELESS matches Elecom's convention of incrementing the wired device ID by 1 for the wireless, so it all looks consistent and predictable.
BTW I did try your patch with my wired HUGE and it works fine, dmesg showed your revised message after booting, and I verified all 3 Fn buttons work fine.
[ 7.444707] elecom 0003:056E:010C.0005: Fixing up ELECOM mouse button count
Thanks!
Alex
PATCH v2 1/1 HID: elecom: rewrite report fixup for EX-G and future mice
In reply to Tomasz KramkowskiThis patch rewrites the mouse report fixup used for the DEFT and HUGE elecom trackballs in order to make it generic enough to fix other elecom mice with similar issues. This patch also uses this new report fixup function to fix the Elecom EX-G trackball which has 6 physical buttons and a similar issue to the other two mice.
Elecom's track record has so far shown that they like to re-use the same report descriptor for multiple different mice regardless of the number of buttons the mouse has. This means that the missing buttons on multiple mice can be fixed in one function without introducing phantom buttons which would in turn cause the number of mouse buttons to be misreported to userspace.
This patch drops the very verbose report descriptor "diff" comment for a more abridged yet hopefully just as informative generic version.
---
v2 changes:
* pass rsize directly to mouse_button_fixup
* add support for wireless EX-G variant
v1: https://marc.info/[email protected]
--- drivers/hid/Kconfig | 1 + drivers/hid/hid-elecom.c | 78 ++++++++++++++++++++++++++---------------------- drivers/hid/hid-ids.h | 2 ++ drivers/hid/hid-quirks.c | 2 ++ 4 files changed, 47 insertions(+), 36 deletions(-)
drivers/hid/Kconfig +1 -0
@@ -280,6 +280,7 @@ config HID_ELECOM ---help--- Support for ELECOM devices: - BM084 Bluetooth Mouse + - EX-G Trackball (Wired and wireless) - DEFT Trackball (Wired and wireless) - HUGE Trackball (Wired and wireless)
drivers/hid/hid-elecom.c +42 -36
108 lines not on this page · raw message
drivers/hid/hid-ids.h +2 -0
@@ -370,6 +370,8 @@#define USB_VENDOR_ID_ELECOM 0x056e#define USB_DEVICE_ID_ELECOM_BM084 0x0061 +#define USB_DEVICE_ID_ELECOM_EX_G_WIRED 0x00fb +#define USB_DEVICE_ID_ELECOM_EX_G_WIRELESS 0x00fc#define USB_DEVICE_ID_ELECOM_DEFT_WIRED 0x00fe#define USB_DEVICE_ID_ELECOM_DEFT_WIRELESS 0x00ff#define USB_DEVICE_ID_ELECOM_HUGE_WIRED 0x010c
drivers/hid/hid-quirks.c +2 -0
@@ -335,6 +335,8 @@ static const struct hid_device_id hid_have_special_driver[] = {#endif #if IS_ENABLED(CONFIG_HID_ELECOM) { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_BM084) }, + { HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_EX_G_WIRED) }, + { HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_EX_G_WIRELESS) }, { HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_DEFT_WIRED) }, { HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_DEFT_WIRELESS) }, { HID_USB_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_HUGE_WIRED) },
Signature · 1 line
2.15.1
Quoted 16 lines · Tomasz Kramkowski
This patch rewrites the mouse report fixup used for the DEFT and HUGE elecom trackballs in order to make it generic enough to fix other elecom mice with similar issues. This patch also uses this new report fixup function to fix the Elecom EX-G trackball which has 6 physical buttons and a similar issue to the other two mice.
Elecom's track record has so far shown that they like to re-use the same report descriptor for multiple different mice regardless of the number of buttons the mouse has. This means that the missing buttons on multiple mice can be fixed in one function without introducing phantom buttons which would in turn cause the number of mouse buttons to be misreported to userspace.
This patch drops the very verbose report descriptor "diff" comment for a more abridged yet hopefully just as informative generic version.
Quoted 6 lines
Signed-off-by Tomasz Kramkowski <[email protected]>---
v2 changes:
* pass rsize directly tomouse_button_fixup
* add support for wireless EX-G variant
v1: https://marc.info/[email protected]
Applied, thanks.
Signature · 2 lines
Jiri Kosina SUSE Labs