All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] HID: elecom: rewrite report fixup for EX-G and future mice
@ 2017-12-04 20:55 Tomasz Kramkowski
  2017-12-05 20:17 ` Tomasz Kramkowski
  2017-12-19 20:44 ` [PATCH v2] " Tomasz Kramkowski
  0 siblings, 2 replies; 10+ messages in thread
From: Tomasz Kramkowski @ 2017-12-04 20:55 UTC (permalink / raw)
  To: Jiri Kosina
  Cc: Benjamin Tissoires, Yuxuan Shui, Diego Elio Pettenò,
	Alex Manoussakis, linux-input, linux-kernel, 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.

Signed-off-by: Tomasz Kramkowski <tk@the-tk.com>
---
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/?i=20171119002353.GA15931@gaia.local
---
 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(-)

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 9058dbc4dd6e..7b1a51c0f326 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -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)
 
diff --git a/drivers/hid/hid-elecom.c b/drivers/hid/hid-elecom.c
index 54aeea57d209..b19361eaae3a 100644
--- a/drivers/hid/hid-elecom.c
+++ b/drivers/hid/hid-elecom.c
@@ -1,9 +1,15 @@
 /*
- *  HID driver for ELECOM devices.
+ *  HID driver for ELECOM devices:
+ *  - BM084 Bluetooth Mouse
+ *  - EX-G Trackball (Wired)
+ *  - DEFT Trackball (Wired and wireless)
+ *  - HUGE Trackball (Wired and wireless)
+ *
  *  Copyright (c) 2010 Richard Nauber <Richard.Nauber@gmail.com>
  *  Copyright (c) 2016 Yuxuan Shui <yshuiv7@gmail.com>
  *  Copyright (c) 2017 Diego Elio Pettenò <flameeyes@flameeyes.eu>
  *  Copyright (c) 2017 Alex Manoussakis <amanou@gnu.org>
+ *  Copyright (c) 2017 Tomasz Kramkowski <tk@the-tk.com>
  */
 
 /*
@@ -19,6 +25,34 @@
 
 #include "hid-ids.h"
 
+/*
+ * Certain ELECOM mice misreport their button count meaning that they only work
+ * correctly with the ELECOM mouse assistant software which is unavailable for
+ * Linux. A four extra INPUT reports and a FEATURE report are described by the
+ * report descriptor but it does not appear that these enable software to
+ * control what the extra buttons map to. The only simple and straightforward
+ * solution seems to involve fixing up the report descriptor.
+ *
+ * Report descriptor format:
+ * Positions 13, 15, 21 and 31 store the button bit count, button usage minimum,
+ * button usage maximum and padding bit count respectively.
+ */
+#define MOUSE_BUTTONS_MAX 8
+static void mouse_button_fixup(struct hid_device *hdev,
+			       __u8 *rdesc, unsigned int *rsize,
+			       int nbuttons)
+{
+	if (*rsize < 32 || rdesc[12] != 0x95 ||
+	    rdesc[14] != 0x75 || rdesc[15] != 0x01 ||
+	    rdesc[20] != 0x29 || rdesc[30] != 0x75)
+		return;
+	hid_info(hdev, "Fixing up ELECOM mouse button count\n");
+	nbuttons = clamp(nbuttons, 0, MOUSE_BUTTONS_MAX);
+	rdesc[13] = nbuttons;
+	rdesc[21] = nbuttons;
+	rdesc[31] = MOUSE_BUTTONS_MAX - nbuttons;
+}
+
 static __u8 *elecom_report_fixup(struct hid_device *hdev, __u8 *rdesc,
 		unsigned int *rsize)
 {
@@ -31,45 +65,14 @@ static __u8 *elecom_report_fixup(struct hid_device *hdev, __u8 *rdesc,
 			rdesc[47] = 0x00;
 		}
 		break;
+	case USB_DEVICE_ID_ELECOM_EX_G_WIRED:
+		mouse_button_fixup(hdev, rdesc, rsize, 6);
+		break;
 	case USB_DEVICE_ID_ELECOM_DEFT_WIRED:
 	case USB_DEVICE_ID_ELECOM_DEFT_WIRELESS:
 	case USB_DEVICE_ID_ELECOM_HUGE_WIRED:
 	case USB_DEVICE_ID_ELECOM_HUGE_WIRELESS:
-		/* The DEFT/HUGE trackball has eight buttons, but its descriptor
-		 * only reports five, disabling the three Fn buttons on the top
-		 * of the mouse.
-		 *
-		 * Apply the following diff to the descriptor:
-		 *
-		 * Collection (Physical),              Collection (Physical),
-		 *     Report ID (1),                      Report ID (1),
-		 *     Report Count (5),           ->      Report Count (8),
-		 *     Report Size (1),                    Report Size (1),
-		 *     Usage Page (Button),                Usage Page (Button),
-		 *     Usage Minimum (01h),                Usage Minimum (01h),
-		 *     Usage Maximum (05h),        ->      Usage Maximum (08h),
-		 *     Logical Minimum (0),                Logical Minimum (0),
-		 *     Logical Maximum (1),                Logical Maximum (1),
-		 *     Input (Variable),                   Input (Variable),
-		 *     Report Count (1),           ->      Report Count (0),
-		 *     Report Size (3),                    Report Size (3),
-		 *     Input (Constant),                   Input (Constant),
-		 *     Report Size (16),                   Report Size (16),
-		 *     Report Count (2),                   Report Count (2),
-		 *     Usage Page (Desktop),               Usage Page (Desktop),
-		 *     Usage (X),                          Usage (X),
-		 *     Usage (Y),                          Usage (Y),
-		 *     Logical Minimum (-32768),           Logical Minimum (-32768),
-		 *     Logical Maximum (32767),            Logical Maximum (32767),
-		 *     Input (Variable, Relative),         Input (Variable, Relative),
-		 * End Collection,                     End Collection,
-		 */
-		if (*rsize == 213 && rdesc[13] == 5 && rdesc[21] == 5) {
-			hid_info(hdev, "Fixing up Elecom DEFT/HUGE Fn buttons\n");
-			rdesc[13] = 8; /* Button/Variable Report Count */
-			rdesc[21] = 8; /* Button/Variable Usage Maximum */
-			rdesc[29] = 0; /* Button/Constant Report Count */
-		}
+		mouse_button_fixup(hdev, rdesc, rsize, 8);
 		break;
 	}
 	return rdesc;
@@ -77,6 +80,7 @@ static __u8 *elecom_report_fixup(struct hid_device *hdev, __u8 *rdesc,
 
 static const struct hid_device_id elecom_devices[] = {
 	{ 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) },
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 5da3d6256d25..d757c78e7e15 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -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
diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
index 015e0c10248b..48054d2d6795 100644
--- a/drivers/hid/hid-quirks.c
+++ b/drivers/hid/hid-quirks.c
@@ -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) },
-- 
2.15.1

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] HID: elecom: rewrite report fixup for EX-G and future mice
  2017-12-04 20:55 [PATCH] HID: elecom: rewrite report fixup for EX-G and future mice Tomasz Kramkowski
@ 2017-12-05 20:17 ` Tomasz Kramkowski
  2017-12-07 10:04   ` Jiri Kosina
  2017-12-19 20:44 ` [PATCH v2] " Tomasz Kramkowski
  1 sibling, 1 reply; 10+ messages in thread
From: Tomasz Kramkowski @ 2017-12-05 20:17 UTC (permalink / raw)
  To: Jiri Kosina
  Cc: Benjamin Tissoires, Yuxuan Shui, Diego Elio Pettenò,
	Alex Manoussakis, linux-input, linux-kernel

On Mon, Dec 04, 2017 at 08:55:50PM +0000, Tomasz Kramkowski wrote:
> +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.

-- 
Tomasz Kramkowski | GPG: 40B037BA0A5B8680 | Web: https://the-tk.com/

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] HID: elecom: rewrite report fixup for EX-G and future mice
  2017-12-05 20:17 ` Tomasz Kramkowski
@ 2017-12-07 10:04   ` Jiri Kosina
  2017-12-09 17:23     ` Tomasz Kramkowski
  0 siblings, 1 reply; 10+ messages in thread
From: Jiri Kosina @ 2017-12-07 10:04 UTC (permalink / raw)
  To: Tomasz Kramkowski
  Cc: Benjamin Tissoires, Yuxuan Shui, Diego Elio Pettenò,
	Alex Manoussakis, linux-input, linux-kernel

On Tue, 5 Dec 2017, Tomasz Kramkowski wrote:

> On Mon, Dec 04, 2017 at 08:55:50PM +0000, Tomasz Kramkowski wrote:
> > +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.

-- 
Jiri Kosina
SUSE Labs

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] HID: elecom: rewrite report fixup for EX-G and future mice
  2017-12-07 10:04   ` Jiri Kosina
@ 2017-12-09 17:23     ` Tomasz Kramkowski
  2017-12-11 10:28       ` Jiri Kosina
  0 siblings, 1 reply; 10+ messages in thread
From: Tomasz Kramkowski @ 2017-12-09 17:23 UTC (permalink / raw)
  To: Jiri Kosina
  Cc: Benjamin Tissoires, Yuxuan Shui, Diego Elio Pettenò,
	Alex Manoussakis, linux-input, linux-kernel

On Thu, Dec 07, 2017 at 11:04:37AM +0100, Jiri Kosina wrote:
> On Tue, 5 Dec 2017, Tomasz Kramkowski wrote:
> 
> > On Mon, Dec 04, 2017 at 08:55:50PM +0000, Tomasz Kramkowski wrote:
> > > +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.

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?

-- 
Tomasz Kramkowski | GPG: 40B037BA0A5B8680 | Web: https://the-tk.com/

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] HID: elecom: rewrite report fixup for EX-G and future mice
  2017-12-09 17:23     ` Tomasz Kramkowski
@ 2017-12-11 10:28       ` Jiri Kosina
  2017-12-11 17:31         ` Alex Manoussakis
  0 siblings, 1 reply; 10+ messages in thread
From: Jiri Kosina @ 2017-12-11 10:28 UTC (permalink / raw)
  To: Tomasz Kramkowski
  Cc: Benjamin Tissoires, Yuxuan Shui, Diego Elio Pettenò,
	Alex Manoussakis, linux-input, linux-kernel

On Sat, 9 Dec 2017, Tomasz Kramkowski wrote:

> Alright, that's fine. Anything else to change before I send a v2? 

Not from my side, I think we're good to go.

> 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,

-- 
Jiri Kosina
SUSE Labs

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] HID: elecom: rewrite report fixup for EX-G and future mice
  2017-12-11 10:28       ` Jiri Kosina
@ 2017-12-11 17:31         ` Alex Manoussakis
  2017-12-13 21:47           ` Tomasz Kramkowski
  0 siblings, 1 reply; 10+ messages in thread
From: Alex Manoussakis @ 2017-12-11 17:31 UTC (permalink / raw)
  To: Jiri Kosina, Tomasz Kramkowski
  Cc: Benjamin Tissoires, Yuxuan Shui, Diego Elio Pettenò,
	linux-input, linux-kernel

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

On Mon, Dec 11, 2017 at 11:28:37AM +0100, Jiri Kosina wrote:
> On Sat, 9 Dec 2017, Tomasz Kramkowski wrote:
> 
> > Alright, that's fine. Anything else to change before I send a v2? 
> 
> Not from my side, I think we're good to go.
> 
> > 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,
> 
> -- 
> Jiri Kosina
> SUSE Labs
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] HID: elecom: rewrite report fixup for EX-G and future mice
  2017-12-11 17:31         ` Alex Manoussakis
@ 2017-12-13 21:47           ` Tomasz Kramkowski
  2017-12-14  1:02             ` Alex Manoussakis
  0 siblings, 1 reply; 10+ messages in thread
From: Tomasz Kramkowski @ 2017-12-13 21:47 UTC (permalink / raw)
  To: Alex Manoussakis
  Cc: Jiri Kosina, Benjamin Tissoires, Yuxuan Shui,
	Diego Elio Pettenò,
	linux-input, linux-kernel

On Mon, Dec 11, 2017 at 12:31:16PM -0500, Alex Manoussakis wrote:
> 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.

> Thanks,
> Alex

-- 
Tomasz Kramkowski | GPG: 40B037BA0A5B8680 | Web: https://the-tk.com/

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] HID: elecom: rewrite report fixup for EX-G and future mice
  2017-12-13 21:47           ` Tomasz Kramkowski
@ 2017-12-14  1:02             ` Alex Manoussakis
  0 siblings, 0 replies; 10+ messages in thread
From: Alex Manoussakis @ 2017-12-14  1:02 UTC (permalink / raw)
  To: Tomasz Kramkowski
  Cc: Jiri Kosina, Benjamin Tissoires, Yuxuan Shui,
	Diego Elio Pettenò,
	linux-input, linux-kernel

On Wed, Dec 13, 2017 at 09:47:46PM +0000, Tomasz Kramkowski wrote:
> On Mon, Dec 11, 2017 at 12:31:16PM -0500, Alex Manoussakis wrote:
> > 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_WIRELESS so that it matches the naming style

Sounds fine.

> 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2] HID: elecom: rewrite report fixup for EX-G and future mice
  2017-12-04 20:55 [PATCH] HID: elecom: rewrite report fixup for EX-G and future mice Tomasz Kramkowski
  2017-12-05 20:17 ` Tomasz Kramkowski
@ 2017-12-19 20:44 ` Tomasz Kramkowski
  2018-01-23 14:40   ` Jiri Kosina
  1 sibling, 1 reply; 10+ messages in thread
From: Tomasz Kramkowski @ 2017-12-19 20:44 UTC (permalink / raw)
  To: Jiri Kosina
  Cc: Benjamin Tissoires, Yuxuan Shui, Diego Elio Pettenò,
	Alex Manoussakis, linux-input, linux-kernel, 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.

Signed-off-by: Tomasz Kramkowski <tk@the-tk.com>
---
v2 changes:
* pass rsize directly to mouse_button_fixup
* add support for wireless EX-G variant
v1: https://marc.info/?i=20171204205550.2621-1-tk@the-tk.com
---
 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(-)

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 9058dbc4dd6e..19c499f5623d 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -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)
 
diff --git a/drivers/hid/hid-elecom.c b/drivers/hid/hid-elecom.c
index 54aeea57d209..1a1ecc491c02 100644
--- a/drivers/hid/hid-elecom.c
+++ b/drivers/hid/hid-elecom.c
@@ -1,9 +1,15 @@
 /*
- *  HID driver for ELECOM devices.
+ *  HID driver for ELECOM devices:
+ *  - BM084 Bluetooth Mouse
+ *  - EX-G Trackball (Wired and wireless)
+ *  - DEFT Trackball (Wired and wireless)
+ *  - HUGE Trackball (Wired and wireless)
+ *
  *  Copyright (c) 2010 Richard Nauber <Richard.Nauber@gmail.com>
  *  Copyright (c) 2016 Yuxuan Shui <yshuiv7@gmail.com>
  *  Copyright (c) 2017 Diego Elio Pettenò <flameeyes@flameeyes.eu>
  *  Copyright (c) 2017 Alex Manoussakis <amanou@gnu.org>
+ *  Copyright (c) 2017 Tomasz Kramkowski <tk@the-tk.com>
  */
 
 /*
@@ -19,6 +25,34 @@
 
 #include "hid-ids.h"
 
+/*
+ * Certain ELECOM mice misreport their button count meaning that they only work
+ * correctly with the ELECOM mouse assistant software which is unavailable for
+ * Linux. A four extra INPUT reports and a FEATURE report are described by the
+ * report descriptor but it does not appear that these enable software to
+ * control what the extra buttons map to. The only simple and straightforward
+ * solution seems to involve fixing up the report descriptor.
+ *
+ * Report descriptor format:
+ * Positions 13, 15, 21 and 31 store the button bit count, button usage minimum,
+ * button usage maximum and padding bit count respectively.
+ */
+#define MOUSE_BUTTONS_MAX 8
+static void mouse_button_fixup(struct hid_device *hdev,
+			       __u8 *rdesc, unsigned int rsize,
+			       int nbuttons)
+{
+	if (rsize < 32 || rdesc[12] != 0x95 ||
+	    rdesc[14] != 0x75 || rdesc[15] != 0x01 ||
+	    rdesc[20] != 0x29 || rdesc[30] != 0x75)
+		return;
+	hid_info(hdev, "Fixing up Elecom mouse button count\n");
+	nbuttons = clamp(nbuttons, 0, MOUSE_BUTTONS_MAX);
+	rdesc[13] = nbuttons;
+	rdesc[21] = nbuttons;
+	rdesc[31] = MOUSE_BUTTONS_MAX - nbuttons;
+}
+
 static __u8 *elecom_report_fixup(struct hid_device *hdev, __u8 *rdesc,
 		unsigned int *rsize)
 {
@@ -31,45 +65,15 @@ static __u8 *elecom_report_fixup(struct hid_device *hdev, __u8 *rdesc,
 			rdesc[47] = 0x00;
 		}
 		break;
+	case USB_DEVICE_ID_ELECOM_EX_G_WIRED:
+	case USB_DEVICE_ID_ELECOM_EX_G_WIRELESS:
+		mouse_button_fixup(hdev, rdesc, *rsize, 6);
+		break;
 	case USB_DEVICE_ID_ELECOM_DEFT_WIRED:
 	case USB_DEVICE_ID_ELECOM_DEFT_WIRELESS:
 	case USB_DEVICE_ID_ELECOM_HUGE_WIRED:
 	case USB_DEVICE_ID_ELECOM_HUGE_WIRELESS:
-		/* The DEFT/HUGE trackball has eight buttons, but its descriptor
-		 * only reports five, disabling the three Fn buttons on the top
-		 * of the mouse.
-		 *
-		 * Apply the following diff to the descriptor:
-		 *
-		 * Collection (Physical),              Collection (Physical),
-		 *     Report ID (1),                      Report ID (1),
-		 *     Report Count (5),           ->      Report Count (8),
-		 *     Report Size (1),                    Report Size (1),
-		 *     Usage Page (Button),                Usage Page (Button),
-		 *     Usage Minimum (01h),                Usage Minimum (01h),
-		 *     Usage Maximum (05h),        ->      Usage Maximum (08h),
-		 *     Logical Minimum (0),                Logical Minimum (0),
-		 *     Logical Maximum (1),                Logical Maximum (1),
-		 *     Input (Variable),                   Input (Variable),
-		 *     Report Count (1),           ->      Report Count (0),
-		 *     Report Size (3),                    Report Size (3),
-		 *     Input (Constant),                   Input (Constant),
-		 *     Report Size (16),                   Report Size (16),
-		 *     Report Count (2),                   Report Count (2),
-		 *     Usage Page (Desktop),               Usage Page (Desktop),
-		 *     Usage (X),                          Usage (X),
-		 *     Usage (Y),                          Usage (Y),
-		 *     Logical Minimum (-32768),           Logical Minimum (-32768),
-		 *     Logical Maximum (32767),            Logical Maximum (32767),
-		 *     Input (Variable, Relative),         Input (Variable, Relative),
-		 * End Collection,                     End Collection,
-		 */
-		if (*rsize == 213 && rdesc[13] == 5 && rdesc[21] == 5) {
-			hid_info(hdev, "Fixing up Elecom DEFT/HUGE Fn buttons\n");
-			rdesc[13] = 8; /* Button/Variable Report Count */
-			rdesc[21] = 8; /* Button/Variable Usage Maximum */
-			rdesc[29] = 0; /* Button/Constant Report Count */
-		}
+		mouse_button_fixup(hdev, rdesc, *rsize, 8);
 		break;
 	}
 	return rdesc;
@@ -77,6 +81,8 @@ static __u8 *elecom_report_fixup(struct hid_device *hdev, __u8 *rdesc,
 
 static const struct hid_device_id elecom_devices[] = {
 	{ 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) },
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 5da3d6256d25..d5daf2e638f7 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -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
diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
index 015e0c10248b..6f5f9c2d30f9 100644
--- a/drivers/hid/hid-quirks.c
+++ b/drivers/hid/hid-quirks.c
@@ -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) },
-- 
2.15.1

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] HID: elecom: rewrite report fixup for EX-G and future mice
  2017-12-19 20:44 ` [PATCH v2] " Tomasz Kramkowski
@ 2018-01-23 14:40   ` Jiri Kosina
  0 siblings, 0 replies; 10+ messages in thread
From: Jiri Kosina @ 2018-01-23 14:40 UTC (permalink / raw)
  To: Tomasz Kramkowski
  Cc: Benjamin Tissoires, Yuxuan Shui, Diego Elio Pettenò,
	Alex Manoussakis, linux-input, linux-kernel

On Tue, 19 Dec 2017, Tomasz Kramkowski wrote:

> 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.
> 
> Signed-off-by: Tomasz Kramkowski <tk@the-tk.com>
> ---
> v2 changes:
> * pass rsize directly to mouse_button_fixup
> * add support for wireless EX-G variant
> v1: https://marc.info/?i=20171204205550.2621-1-tk@the-tk.com

Applied, thanks.

-- 
Jiri Kosina
SUSE Labs

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2018-01-23 14:40 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-04 20:55 [PATCH] HID: elecom: rewrite report fixup for EX-G and future mice Tomasz Kramkowski
2017-12-05 20:17 ` Tomasz Kramkowski
2017-12-07 10:04   ` Jiri Kosina
2017-12-09 17:23     ` Tomasz Kramkowski
2017-12-11 10:28       ` Jiri Kosina
2017-12-11 17:31         ` Alex Manoussakis
2017-12-13 21:47           ` Tomasz Kramkowski
2017-12-14  1:02             ` Alex Manoussakis
2017-12-19 20:44 ` [PATCH v2] " Tomasz Kramkowski
2018-01-23 14:40   ` Jiri Kosina

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.