[PATCH 0/2] patches for Innomedia INNEX GENESIS/ATARI adapter

[PATCH 0/2] patches for Innomedia INNEX GENESIS/ATARI adapter

[Tomasz Kramkowski]

This is a couple of patches which fix issues with the (1292:4745)
Innomedia INNEX GENESIS/ATARI controller USB adapter. I've personally
tested these patches against the master of the hid tree.

Patch 2 depends on patch 1.

I wasn't sure if adding a new config symbol meant I needed to add it to
some default config so I left that alone.

Hopefully I didn't screw anything up.

Thanks,

-- 
Tomasz Kramkowski

Tomasz Kramkowski (2):
  HID: add Innomedia INNEX GENESIS/ATARI adapter support
  HID: usbhid: add quirk for Innomedia INNEX GENESIS/ATARI adapter

 drivers/hid/Kconfig             |  6 ++++
 drivers/hid/Makefile            |  1 +
 drivers/hid/hid-core.c          |  1 +
 drivers/hid/hid-ids.h           |  3 ++
 drivers/hid/hid-innomedia.c     | 63 +++++++++++++++++++++++++++++++++++++++++
 drivers/hid/usbhid/hid-quirks.c |  1 +
 6 files changed, 75 insertions(+)
 create mode 100644 drivers/hid/hid-innomedia.c

-- 
2.10.2

[PATCH 1/2] HID: add innomedia INNEX GENESIS/ATARI adapter support

[Tomasz Kramkowski]

Add a new module named hid-innomedia which implements support for the
(1292:4745) innomedia INNEX GENESIS/ATARI Controller USB adapter.

This device mis-reports the X and Y axis on the DPad when up or left are
pressed. The value reported is outside the range reported by the report
descriptor. The device reports -2 for left or up, 1 for right or down, 0
for nothing pressed and -1 for up and down or left and right pressed
simultaneously. This means that when left or up are pressed, the value
gets dropped by hid-input. Additionally, when both up and down or left
and right are pressed the device incorrectly reports up or left
respectively.

Signed-off-by: Tomasz Kramkowski <tk@the-tk.com>
---
 drivers/hid/Kconfig         |  6 +++++
 drivers/hid/Makefile        |  1 +
 drivers/hid/hid-core.c      |  1 +
 drivers/hid/hid-ids.h       |  3 +++
 drivers/hid/hid-innomedia.c | 63 +++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 74 insertions(+)
 create mode 100644 drivers/hid/hid-innomedia.c

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 4070b73..2b1829b 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -369,6 +369,12 @@ config HID_ICADE
 	To compile this driver as a module, choose M here: the
 	module will be called hid-icade.
 
+config HID_INNOMEDIA
+	tristate "Innomedia GENESIS/ATARI Controller USB adapter"
+	depends on HID
+	---help---
+	Support for the Innomedia GENESIS/ATARI Controller USB adapter.
+
 config HID_TWINHAN
 	tristate "Twinhan IR remote control"
 	depends on HID
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 4d111f2..10a802b 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -49,6 +49,7 @@ obj-$(CONFIG_HID_HOLTEK)	+= hid-holtek-mouse.o
 obj-$(CONFIG_HID_HOLTEK)	+= hid-holtekff.o
 obj-$(CONFIG_HID_HYPERV_MOUSE)	+= hid-hyperv.o
 obj-$(CONFIG_HID_ICADE)		+= hid-icade.o
+obj-$(CONFIG_HID_INNOMEDIA)	+= hid-innomedia.o
 obj-$(CONFIG_HID_KENSINGTON)	+= hid-kensington.o
 obj-$(CONFIG_HID_KEYTOUCH)	+= hid-keytouch.o
 obj-$(CONFIG_HID_KYE)		+= hid-kye.o
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index cff060b..e410e75 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1912,6 +1912,7 @@ static const struct hid_device_id hid_have_special_driver[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_HOLTEK_ALT, USB_DEVICE_ID_HOLTEK_ALT_MOUSE_A081) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_HOLTEK_ALT, USB_DEVICE_ID_HOLTEK_ALT_MOUSE_A0C2) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_HUION, USB_DEVICE_ID_HUION_TABLET) },
+	{ HID_USB_DEVICE(USB_VENDOR_ID_INNOMEDIA, USB_DEVICE_ID_INNEX_GENESIS_ATARI) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_JESS2, USB_DEVICE_ID_JESS2_COLOR_RUMBLE_PAD) },
 	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_ION, USB_DEVICE_ID_ICADE) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_KENSINGTON, USB_DEVICE_ID_KS_SLIMBLADE) },
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index ec277b9..b0e6f7c 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -537,6 +537,9 @@
 #define USB_VENDOR_ID_IRTOUCHSYSTEMS	0x6615
 #define USB_DEVICE_ID_IRTOUCH_INFRARED_USB	0x0070
 
+#define USB_VENDOR_ID_INNOMEDIA			0x1292
+#define USB_DEVICE_ID_INNEX_GENESIS_ATARI	0x4745
+
 #define USB_VENDOR_ID_ITE               0x048d
 #define USB_DEVICE_ID_ITE_LENOVO_YOGA   0x8386
 #define USB_DEVICE_ID_ITE_LENOVO_YOGA2  0x8350
diff --git a/drivers/hid/hid-innomedia.c b/drivers/hid/hid-innomedia.c
new file mode 100644
index 0000000..325cd7e
--- /dev/null
+++ b/drivers/hid/hid-innomedia.c
@@ -0,0 +1,63 @@
+/*
+ *  HID driver for quirky Innomedia devices
+ *
+ *  Copyright (c) 2016 Tomasz Kramkowski <tk@the-tk.com>
+ */
+
+/*
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ */
+
+#include <linux/device.h>
+#include <linux/hid.h>
+#include <linux/module.h>
+
+#include "hid-ids.h"
+
+static inline u8 fixaxis(u8 bits, int shift)
+{
+	u8 mask = 0x3 << shift;
+	u8 axis = (bits & mask) >> shift;
+
+	/*
+	 * These controllers report -2 (2) for left/up direction and -1 (3) for
+	 * both up-down or left-right pressed.
+	 */
+	if (axis == 3)
+		axis = 0;
+	else if (axis == 2)
+		axis = 3;
+
+	return (bits & ~mask) | (axis << shift);
+}
+
+static int im_raw_event(struct hid_device *hdev, struct hid_report *report,
+			u8 *data, int size)
+{
+	if (size == 3 && (data[0] == 1 || data[0] == 2)) {
+		data[1] = fixaxis(data[1], 0);
+		data[1] = fixaxis(data[1], 2);
+	}
+
+	return 0;
+}
+
+static const struct hid_device_id im_devices[] = {
+	{ HID_USB_DEVICE(USB_VENDOR_ID_INNOMEDIA, USB_DEVICE_ID_INNEX_GENESIS_ATARI) },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(hid, im_devices);
+
+static struct hid_driver im_driver = {
+	.name = "innomedia",
+	.id_table = im_devices,
+	.raw_event = im_raw_event,
+};
+
+module_hid_driver(im_driver);
+
+MODULE_LICENSE("GPL");
-- 
2.10.2

[PATCH 2/2] HID: usbhid: add quirk for innomedia INNEX GENESIS/ATARI adapter

[Tomasz Kramkowski]

The (1292:4745) Innomedia INNEX GENESIS/ATARI adapter needs
HID_QUIRK_MULTI_INPUT to split the device up into two controllers
instead of inputs from both being merged into one.

This patch depends on patch 1.

Signed-off-by: Tomasz Kramkowski <tk@the-tk.com>
---
 drivers/hid/usbhid/hid-quirks.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/hid/usbhid/hid-quirks.c b/drivers/hid/usbhid/hid-quirks.c
index b3e01c8..b5e013b 100644
--- a/drivers/hid/usbhid/hid-quirks.c
+++ b/drivers/hid/usbhid/hid-quirks.c
@@ -165,6 +165,7 @@ static const struct hid_blacklist {
 	{ USB_VENDOR_ID_MULTIPLE_1781, USB_DEVICE_ID_RAPHNET_4NES4SNES_OLD, HID_QUIRK_MULTI_INPUT },
 	{ USB_VENDOR_ID_DRACAL_RAPHNET, USB_DEVICE_ID_RAPHNET_2NES2SNES, HID_QUIRK_MULTI_INPUT },
 	{ USB_VENDOR_ID_DRACAL_RAPHNET, USB_DEVICE_ID_RAPHNET_4NES4SNES, HID_QUIRK_MULTI_INPUT },
+	{ USB_VENDOR_ID_INNOMEDIA, USB_DEVICE_ID_INNEX_GENESIS_ATARI, HID_QUIRK_MULTI_INPUT },
 
 	{ 0, 0 }
 };
-- 
2.10.2

Re: [PATCH 0/2] patches for Innomedia INNEX GENESIS/ATARI adapter

[Jiri Kosina]

On Fri, 16 Dec 2016, Tomasz Kramkowski wrote:

> This is a couple of patches which fix issues with the (1292:4745)
> Innomedia INNEX GENESIS/ATARI controller USB adapter. I've personally
> tested these patches against the master of the hid tree.
> 
> Patch 2 depends on patch 1.
> 
> I wasn't sure if adding a new config symbol meant I needed to add it to
> some default config so I left that alone.
> 
> Hopefully I didn't screw anything up.

All looks good, except I'd probably squash both the patches into one 'add 
adapter support', as I don't really see a value with separating this into 
two patches. If you agree, I can do that myself and apply.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 0/2] patches for Innomedia INNEX GENESIS/ATARI adapter

[Tomasz Kramkowski]

Whoops, forgot to reply all...

On Mon, Dec 19, 2016 at 11:45:07AM +0100, Jiri Kosina wrote:
> All looks good, except I'd probably squash both the patches into one 'add 
> adapter support', as I don't really see a value with separating this into 
> two patches. If you agree, I can do that myself and apply.
> 
> Thanks,
> 
> -- 
> Jiri Kosina
> SUSE Labs
> 

It's perfectly fine if you squash them, I was erring on the side of
caution, I would rather send two patches and have them squash than to
have to resend after splitting.

Thanks,

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

Re: [PATCH 0/2] patches for Innomedia INNEX GENESIS/ATARI adapter

[Jiri Kosina]

On Mon, 19 Dec 2016, Jiri Kosina wrote:

> > This is a couple of patches which fix issues with the (1292:4745)
> > Innomedia INNEX GENESIS/ATARI controller USB adapter. I've personally
> > tested these patches against the master of the hid tree.
> > 
> > Patch 2 depends on patch 1.
> > 
> > I wasn't sure if adding a new config symbol meant I needed to add it to
> > some default config so I left that alone.
> > 
> > Hopefully I didn't screw anything up.
> 
> All looks good, except I'd probably squash both the patches into one 'add 
> adapter support', as I don't really see a value with separating this into 
> two patches. If you agree, I can do that myself and apply.

We've discussed with Tomasz off-list, and it looks like that there likely 
is a better and more general way to fix it; this seems to have actually 
been caused by 6da7066906e977d4 ("HID: ignore absolute values which don't 
fit between logical min and max"), as it turns out that this check should 
be applied only in case Null state is set (refer to HID specification 
1.11, section 6.2.2.5, p.31), bko#68621.

Tomasz comitted to look into this after coming back from christmas/new 
year vacation. I'm putting the innomedia branch on hold for now.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 0/2] patches for Innomedia INNEX GENESIS/ATARI adapter

[Tomasz Kramkowski]

On Thu, Dec 22, 2016 at 10:00:10PM +0100, Jiri Kosina wrote:
> We've discussed with Tomasz off-list, and it looks like that there likely 
> is a better and more general way to fix it; this seems to have actually 
> been caused by 6da7066906e977d4 ("HID: ignore absolute values which don't 
> fit between logical min and max"), as it turns out that this check should 
> be applied only in case Null state is set (refer to HID specification 
> 1.11, section 6.2.2.5, p.31), bko#68621.
> 
> Tomasz comitted to look into this after coming back from christmas/new 
> year vacation. I'm putting the innomedia branch on hold for now.
> 
> -- 
> Jiri Kosina
> SUSE Labs

I've tested the patch from the bugzilla and it does indeed prevent the
values being discarded. evtest reports the minimum and maximums
correctly as -1 and 1 but the events give values -2 and 1, I'm not sure
if this will be an issue as jstest correctly sees left/up presses as
-32767 and right/down presses as 32767. The device still needs the
HID_QUIRK_MULTI_INPUT quirk to correctly show up as two event/js
devices.

I will still need to do a few more tests to make sure there are no
issues in other applications and to make sure that other devices still
operate correctly too.

Once that is done I will go ahead and send two patches, one with the
authorship preserved for the bugzilla patch and one patch to add the
quirks.

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