[PATCH v2 0/6] HID: huion/uclogic merge and few additions

[PATCH v2 0/6] HID: huion/uclogic merge and few additions

[Benjamin Tissoires]

Hi,

this is the v2 of the merge of Huion and UcLogic.
I removed the previous 3/4 as mentioned during the review and added
3 more.
Note that 6/6 could have been squashed in 2/6, but I kept it separate
to keep the first 2 reviewed.

Cheers,
Benjamin

Benjamin Tissoires (6):
  HID: uclogic: Set quirks from inside the driver
  HID: uclogic: merge hid-huion driver in hid-uclogic
  HID: uclogic: name the input nodes based on their tool
  HID: uclogic: apply quirk NO_EMPTY_INPUT
  HID: uclogic: discard the extra Pen input node on Huion tablets
  HID: uclogic: actually invert the in-range bit for huion tablets only

 drivers/hid/Kconfig             |   8 +-
 drivers/hid/Makefile            |   1 -
 drivers/hid/hid-huion.c         | 290 -----------------------------------
 drivers/hid/hid-uclogic.c       | 324 +++++++++++++++++++++++++++++++++++++++-
 drivers/hid/usbhid/hid-quirks.c |   4 -
 5 files changed, 324 insertions(+), 303 deletions(-)
 delete mode 100644 drivers/hid/hid-huion.c

-- 
2.1.0

[PATCH v2 1/6] HID: uclogic: Set quirks from inside the driver

[Benjamin Tissoires]

Based on a patch from: Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com>

Most of the tablets handled by hid-uclogic already use MULTI_INPUT.
For the ones which are not quirked in usbhid/hidquirks, they have a
custom report descriptor which contains only one report per HID
interface. For those tablets HID_QUIRK_MULTI_INPUT is transparent.

According to https://github.com/DIGImend/tablets, the only problematic
tablet currently handled by hid-uclogic is the TWHA60 v3. This tablet
presents different report descriptors from the ones currently quirked.
This is not a problem per se, given that this tablet is not supported
currently in this version (it needs the same command as a Huion to
start forwarding events).

Reviewed-by: Nikolai Kondrashov <spbnick@gmail.com>
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---

no changes in v2

 drivers/hid/hid-uclogic.c       | 27 +++++++++++++++++++++++++++
 drivers/hid/usbhid/hid-quirks.c |  4 ----
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-uclogic.c b/drivers/hid/hid-uclogic.c
index fb8b516..22dccce 100644
--- a/drivers/hid/hid-uclogic.c
+++ b/drivers/hid/hid-uclogic.c
@@ -626,6 +626,32 @@ static __u8 *uclogic_report_fixup(struct hid_device *hdev, __u8 *rdesc,
 	return rdesc;
 }
 
+static int uclogic_probe(struct hid_device *hdev,
+		const struct hid_device_id *id)
+{
+	int rc;
+
+	/*
+	 * libinput requires the pad interface to be on a different node
+	 * than the pen, so use QUIRK_MULTI_INPUT for all tablets.
+	 */
+	hdev->quirks |= HID_QUIRK_MULTI_INPUT;
+
+	rc = hid_parse(hdev);
+	if (rc) {
+		hid_err(hdev, "parse failed\n");
+		return rc;
+	}
+
+	rc = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
+	if (rc) {
+		hid_err(hdev, "hw start failed\n");
+		return rc;
+	}
+
+	return 0;
+}
+
 static const struct hid_device_id uclogic_devices[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_UCLOGIC,
 				USB_DEVICE_ID_UCLOGIC_TABLET_PF1209) },
@@ -648,6 +674,7 @@ MODULE_DEVICE_TABLE(hid, uclogic_devices);
 static struct hid_driver uclogic_driver = {
 	.name = "uclogic",
 	.id_table = uclogic_devices,
+	.probe = uclogic_probe,
 	.report_fixup = uclogic_report_fixup,
 };
 module_hid_driver(uclogic_driver);
diff --git a/drivers/hid/usbhid/hid-quirks.c b/drivers/hid/usbhid/hid-quirks.c
index 989c59a..75ca2de 100644
--- a/drivers/hid/usbhid/hid-quirks.c
+++ b/drivers/hid/usbhid/hid-quirks.c
@@ -106,12 +106,8 @@ static const struct hid_blacklist {
 	{ USB_VENDOR_ID_SYMBOL, USB_DEVICE_ID_SYMBOL_SCANNER_2, HID_QUIRK_NOGET },
 	{ USB_VENDOR_ID_TPV, USB_DEVICE_ID_TPV_OPTICAL_TOUCHSCREEN, HID_QUIRK_NOGET },
 	{ USB_VENDOR_ID_TURBOX, USB_DEVICE_ID_TURBOX_KEYBOARD, HID_QUIRK_NOGET },
-	{ USB_VENDOR_ID_UCLOGIC, USB_DEVICE_ID_UCLOGIC_TABLET_PF1209, HID_QUIRK_MULTI_INPUT },
-	{ USB_VENDOR_ID_UCLOGIC, USB_DEVICE_ID_UCLOGIC_TABLET_WP4030U, HID_QUIRK_MULTI_INPUT },
 	{ USB_VENDOR_ID_UCLOGIC, USB_DEVICE_ID_UCLOGIC_TABLET_KNA5, HID_QUIRK_MULTI_INPUT },
 	{ USB_VENDOR_ID_UCLOGIC, USB_DEVICE_ID_UCLOGIC_TABLET_TWA60, HID_QUIRK_MULTI_INPUT },
-	{ USB_VENDOR_ID_UCLOGIC, USB_DEVICE_ID_UCLOGIC_TABLET_WP5540U, HID_QUIRK_MULTI_INPUT },
-	{ USB_VENDOR_ID_UCLOGIC, USB_DEVICE_ID_UCLOGIC_TABLET_WP8060U, HID_QUIRK_MULTI_INPUT },
 	{ USB_VENDOR_ID_WALTOP, USB_DEVICE_ID_WALTOP_MEDIA_TABLET_10_6_INCH, HID_QUIRK_MULTI_INPUT },
 	{ USB_VENDOR_ID_WALTOP, USB_DEVICE_ID_WALTOP_MEDIA_TABLET_14_1_INCH, HID_QUIRK_MULTI_INPUT },
 	{ USB_VENDOR_ID_WALTOP, USB_DEVICE_ID_WALTOP_SIRIUS_BATTERY_FREE_TABLET, HID_QUIRK_MULTI_INPUT },
-- 
2.1.0

[PATCH v2 2/6] HID: uclogic: merge hid-huion driver in hid-uclogic

[Benjamin Tissoires]

From: Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com>

Merge the hid-huion driver into hid-uclogic as all the devices supported
by hid-huion are in fact UC-Logic devices.

Signed-off-by: Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com>
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---

no changes in v2

 drivers/hid/Kconfig       |   8 +-
 drivers/hid/Makefile      |   1 -
 drivers/hid/hid-huion.c   | 290 ----------------------------------------------
 drivers/hid/hid-uclogic.c | 229 +++++++++++++++++++++++++++++++++++-
 4 files changed, 229 insertions(+), 299 deletions(-)
 delete mode 100644 drivers/hid/hid-huion.c

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 152b006..8a55fd7 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -286,12 +286,6 @@ config HID_GT683R
 	Currently the following devices are know to be supported:
 	  - MSI GT683R
 
-config HID_HUION
-	tristate "Huion tablets"
-	depends on USB_HID
-	---help---
-	Support for Huion 580 tablet.
-
 config HID_KEYTOUCH
 	tristate "Keytouch HID devices"
 	depends on HID
@@ -314,7 +308,7 @@ config HID_UCLOGIC
 	tristate "UC-Logic"
 	depends on HID
 	---help---
-	Support for UC-Logic tablets.
+	Support for UC-Logic and Huion tablets.
 
 config HID_WALTOP
 	tristate "Waltop"
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 6f19958..9c399fe 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -41,7 +41,6 @@ obj-$(CONFIG_HID_GYRATION)	+= hid-gyration.o
 obj-$(CONFIG_HID_HOLTEK)	+= hid-holtek-kbd.o
 obj-$(CONFIG_HID_HOLTEK)	+= hid-holtek-mouse.o
 obj-$(CONFIG_HID_HOLTEK)	+= hid-holtekff.o
-obj-$(CONFIG_HID_HUION)		+= hid-huion.o
 obj-$(CONFIG_HID_HYPERV_MOUSE)	+= hid-hyperv.o
 obj-$(CONFIG_HID_ICADE)		+= hid-icade.o
 obj-$(CONFIG_HID_KENSINGTON)	+= hid-kensington.o
diff --git a/drivers/hid/hid-huion.c b/drivers/hid/hid-huion.c
deleted file mode 100644
index 61b68ca..0000000
--- a/drivers/hid/hid-huion.c
+++ /dev/null
@@ -1,290 +0,0 @@
-/*
- *  HID driver for Huion devices not fully compliant with HID standard
- *
- *  Copyright (c) 2013 Martin Rusko
- *  Copyright (c) 2014 Nikolai Kondrashov
- */
-
-/*
- * 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 <linux/usb.h>
-#include <asm/unaligned.h>
-#include "usbhid/usbhid.h"
-
-#include "hid-ids.h"
-
-/* Report descriptor template placeholder head */
-#define HUION_PH_HEAD	0xFE, 0xED, 0x1D
-
-/* Report descriptor template placeholder IDs */
-enum huion_ph_id {
-	HUION_PH_ID_X_LM,
-	HUION_PH_ID_X_PM,
-	HUION_PH_ID_Y_LM,
-	HUION_PH_ID_Y_PM,
-	HUION_PH_ID_PRESSURE_LM,
-	HUION_PH_ID_NUM
-};
-
-/* Report descriptor template placeholder */
-#define HUION_PH(_ID) HUION_PH_HEAD, HUION_PH_ID_##_ID
-
-/* Fixed report descriptor template */
-static const __u8 huion_tablet_rdesc_template[] = {
-	0x05, 0x0D,             /*  Usage Page (Digitizer),                 */
-	0x09, 0x02,             /*  Usage (Pen),                            */
-	0xA1, 0x01,             /*  Collection (Application),               */
-	0x85, 0x07,             /*      Report ID (7),                      */
-	0x09, 0x20,             /*      Usage (Stylus),                     */
-	0xA0,                   /*      Collection (Physical),              */
-	0x14,                   /*          Logical Minimum (0),            */
-	0x25, 0x01,             /*          Logical Maximum (1),            */
-	0x75, 0x01,             /*          Report Size (1),                */
-	0x09, 0x42,             /*          Usage (Tip Switch),             */
-	0x09, 0x44,             /*          Usage (Barrel Switch),          */
-	0x09, 0x46,             /*          Usage (Tablet Pick),            */
-	0x95, 0x03,             /*          Report Count (3),               */
-	0x81, 0x02,             /*          Input (Variable),               */
-	0x95, 0x03,             /*          Report Count (3),               */
-	0x81, 0x03,             /*          Input (Constant, Variable),     */
-	0x09, 0x32,             /*          Usage (In Range),               */
-	0x95, 0x01,             /*          Report Count (1),               */
-	0x81, 0x02,             /*          Input (Variable),               */
-	0x95, 0x01,             /*          Report Count (1),               */
-	0x81, 0x03,             /*          Input (Constant, Variable),     */
-	0x75, 0x10,             /*          Report Size (16),               */
-	0x95, 0x01,             /*          Report Count (1),               */
-	0xA4,                   /*          Push,                           */
-	0x05, 0x01,             /*          Usage Page (Desktop),           */
-	0x65, 0x13,             /*          Unit (Inch),                    */
-	0x55, 0xFD,             /*          Unit Exponent (-3),             */
-	0x34,                   /*          Physical Minimum (0),           */
-	0x09, 0x30,             /*          Usage (X),                      */
-	0x27, HUION_PH(X_LM),   /*          Logical Maximum (PLACEHOLDER),  */
-	0x47, HUION_PH(X_PM),   /*          Physical Maximum (PLACEHOLDER), */
-	0x81, 0x02,             /*          Input (Variable),               */
-	0x09, 0x31,             /*          Usage (Y),                      */
-	0x27, HUION_PH(Y_LM),   /*          Logical Maximum (PLACEHOLDER),  */
-	0x47, HUION_PH(Y_PM),   /*          Physical Maximum (PLACEHOLDER), */
-	0x81, 0x02,             /*          Input (Variable),               */
-	0xB4,                   /*          Pop,                            */
-	0x09, 0x30,             /*          Usage (Tip Pressure),           */
-	0x27,
-	HUION_PH(PRESSURE_LM),  /*          Logical Maximum (PLACEHOLDER),  */
-	0x81, 0x02,             /*          Input (Variable),               */
-	0xC0,                   /*      End Collection,                     */
-	0xC0                    /*  End Collection                          */
-};
-
-/* Parameter indices */
-enum huion_prm {
-	HUION_PRM_X_LM		= 1,
-	HUION_PRM_Y_LM		= 2,
-	HUION_PRM_PRESSURE_LM	= 4,
-	HUION_PRM_RESOLUTION	= 5,
-	HUION_PRM_NUM
-};
-
-/* Driver data */
-struct huion_drvdata {
-	__u8 *rdesc;
-	unsigned int rsize;
-};
-
-static __u8 *huion_report_fixup(struct hid_device *hdev, __u8 *rdesc,
-		unsigned int *rsize)
-{
-	struct huion_drvdata *drvdata = hid_get_drvdata(hdev);
-	switch (hdev->product) {
-	case USB_DEVICE_ID_HUION_TABLET:
-		if (drvdata->rdesc != NULL) {
-			rdesc = drvdata->rdesc;
-			*rsize = drvdata->rsize;
-		}
-		break;
-	}
-	return rdesc;
-}
-
-/**
- * Enable fully-functional tablet mode and determine device parameters.
- *
- * @hdev:	HID device
- */
-static int huion_tablet_enable(struct hid_device *hdev)
-{
-	int rc;
-	struct usb_device *usb_dev = hid_to_usb_dev(hdev);
-	struct huion_drvdata *drvdata = hid_get_drvdata(hdev);
-	__le16 *buf = NULL;
-	size_t len;
-	s32 params[HUION_PH_ID_NUM];
-	s32 resolution;
-	__u8 *p;
-	s32 v;
-
-	/*
-	 * Read string descriptor containing tablet parameters. The specific
-	 * string descriptor and data were discovered by sniffing the Windows
-	 * driver traffic.
-	 * NOTE: This enables fully-functional tablet mode.
-	 */
-	len = HUION_PRM_NUM * sizeof(*buf);
-	buf = kmalloc(len, GFP_KERNEL);
-	if (buf == NULL) {
-		hid_err(hdev, "failed to allocate parameter buffer\n");
-		rc = -ENOMEM;
-		goto cleanup;
-	}
-	rc = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0),
-				USB_REQ_GET_DESCRIPTOR, USB_DIR_IN,
-				(USB_DT_STRING << 8) + 0x64,
-				0x0409, buf, len,
-				USB_CTRL_GET_TIMEOUT);
-	if (rc == -EPIPE) {
-		hid_err(hdev, "device parameters not found\n");
-		rc = -ENODEV;
-		goto cleanup;
-	} else if (rc < 0) {
-		hid_err(hdev, "failed to get device parameters: %d\n", rc);
-		rc = -ENODEV;
-		goto cleanup;
-	} else if (rc != len) {
-		hid_err(hdev, "invalid device parameters\n");
-		rc = -ENODEV;
-		goto cleanup;
-	}
-
-	/* Extract device parameters */
-	params[HUION_PH_ID_X_LM] = le16_to_cpu(buf[HUION_PRM_X_LM]);
-	params[HUION_PH_ID_Y_LM] = le16_to_cpu(buf[HUION_PRM_Y_LM]);
-	params[HUION_PH_ID_PRESSURE_LM] =
-		le16_to_cpu(buf[HUION_PRM_PRESSURE_LM]);
-	resolution = le16_to_cpu(buf[HUION_PRM_RESOLUTION]);
-	if (resolution == 0) {
-		params[HUION_PH_ID_X_PM] = 0;
-		params[HUION_PH_ID_Y_PM] = 0;
-	} else {
-		params[HUION_PH_ID_X_PM] = params[HUION_PH_ID_X_LM] *
-						1000 / resolution;
-		params[HUION_PH_ID_Y_PM] = params[HUION_PH_ID_Y_LM] *
-						1000 / resolution;
-	}
-
-	/* Allocate fixed report descriptor */
-	drvdata->rdesc = devm_kmalloc(&hdev->dev,
-				sizeof(huion_tablet_rdesc_template),
-				GFP_KERNEL);
-	if (drvdata->rdesc == NULL) {
-		hid_err(hdev, "failed to allocate fixed rdesc\n");
-		rc = -ENOMEM;
-		goto cleanup;
-	}
-	drvdata->rsize = sizeof(huion_tablet_rdesc_template);
-
-	/* Format fixed report descriptor */
-	memcpy(drvdata->rdesc, huion_tablet_rdesc_template,
-		drvdata->rsize);
-	for (p = drvdata->rdesc;
-	     p <= drvdata->rdesc + drvdata->rsize - 4;) {
-		if (p[0] == 0xFE && p[1] == 0xED && p[2] == 0x1D &&
-		    p[3] < sizeof(params)) {
-			v = params[p[3]];
-			put_unaligned(cpu_to_le32(v), (s32 *)p);
-			p += 4;
-		} else {
-			p++;
-		}
-	}
-
-	rc = 0;
-
-cleanup:
-	kfree(buf);
-	return rc;
-}
-
-static int huion_probe(struct hid_device *hdev, const struct hid_device_id *id)
-{
-	int rc;
-	struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
-	struct huion_drvdata *drvdata;
-
-	/* Allocate and assign driver data */
-	drvdata = devm_kzalloc(&hdev->dev, sizeof(*drvdata), GFP_KERNEL);
-	if (drvdata == NULL) {
-		hid_err(hdev, "failed to allocate driver data\n");
-		return -ENOMEM;
-	}
-	hid_set_drvdata(hdev, drvdata);
-
-	switch (id->product) {
-	case USB_DEVICE_ID_HUION_TABLET:
-		/* If this is the pen interface */
-		if (intf->cur_altsetting->desc.bInterfaceNumber == 0) {
-			rc = huion_tablet_enable(hdev);
-			if (rc) {
-				hid_err(hdev, "tablet enabling failed\n");
-				return rc;
-			}
-		}
-		break;
-	}
-
-	rc = hid_parse(hdev);
-	if (rc) {
-		hid_err(hdev, "parse failed\n");
-		return rc;
-	}
-
-	rc = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
-	if (rc) {
-		hid_err(hdev, "hw start failed\n");
-		return rc;
-	}
-
-	return 0;
-}
-
-static int huion_raw_event(struct hid_device *hdev, struct hid_report *report,
-			u8 *data, int size)
-{
-	struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
-
-	/* If this is a pen input report */
-	if (intf->cur_altsetting->desc.bInterfaceNumber == 0 &&
-	    report->type == HID_INPUT_REPORT &&
-	    report->id == 0x07 && size >= 2)
-		/* Invert the in-range bit */
-		data[1] ^= 0x40;
-
-	return 0;
-}
-
-static const struct hid_device_id huion_devices[] = {
-	{ HID_USB_DEVICE(USB_VENDOR_ID_HUION, USB_DEVICE_ID_HUION_TABLET) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_UCLOGIC, USB_DEVICE_ID_HUION_TABLET) },
-	{ }
-};
-MODULE_DEVICE_TABLE(hid, huion_devices);
-
-static struct hid_driver huion_driver = {
-	.name = "huion",
-	.id_table = huion_devices,
-	.probe = huion_probe,
-	.report_fixup = huion_report_fixup,
-	.raw_event = huion_raw_event,
-};
-module_hid_driver(huion_driver);
-
-MODULE_AUTHOR("Martin Rusko");
-MODULE_DESCRIPTION("Huion HID driver");
-MODULE_LICENSE("GPL");
diff --git a/drivers/hid/hid-uclogic.c b/drivers/hid/hid-uclogic.c
index 22dccce..397f1df 100644
--- a/drivers/hid/hid-uclogic.c
+++ b/drivers/hid/hid-uclogic.c
@@ -1,7 +1,8 @@
 /*
  *  HID driver for UC-Logic devices not fully compliant with HID standard
  *
- *  Copyright (c) 2010 Nikolai Kondrashov
+ *  Copyright (c) 2010-2014 Nikolai Kondrashov
+ *  Copyright (c) 2013 Martin Rusko
  */
 
 /*
@@ -15,6 +16,8 @@
 #include <linux/hid.h>
 #include <linux/module.h>
 #include <linux/usb.h>
+#include <asm/unaligned.h>
+#include "usbhid/usbhid.h"
 
 #include "hid-ids.h"
 
@@ -546,11 +549,90 @@ static __u8 twha60_rdesc_fixed1[] = {
 	0xC0        /*  End Collection              */
 };
 
+/* Report descriptor template placeholder head */
+#define UCLOGIC_PH_HEAD	0xFE, 0xED, 0x1D
+
+/* Report descriptor template placeholder IDs */
+enum uclogic_ph_id {
+	UCLOGIC_PH_ID_X_LM,
+	UCLOGIC_PH_ID_X_PM,
+	UCLOGIC_PH_ID_Y_LM,
+	UCLOGIC_PH_ID_Y_PM,
+	UCLOGIC_PH_ID_PRESSURE_LM,
+	UCLOGIC_PH_ID_NUM
+};
+
+/* Report descriptor template placeholder */
+#define UCLOGIC_PH(_ID) UCLOGIC_PH_HEAD, UCLOGIC_PH_ID_##_ID
+
+/* Fixed report descriptor template */
+static const __u8 uclogic_tablet_rdesc_template[] = {
+	0x05, 0x0D,             /*  Usage Page (Digitizer),                 */
+	0x09, 0x02,             /*  Usage (Pen),                            */
+	0xA1, 0x01,             /*  Collection (Application),               */
+	0x85, 0x07,             /*      Report ID (7),                      */
+	0x09, 0x20,             /*      Usage (Stylus),                     */
+	0xA0,                   /*      Collection (Physical),              */
+	0x14,                   /*          Logical Minimum (0),            */
+	0x25, 0x01,             /*          Logical Maximum (1),            */
+	0x75, 0x01,             /*          Report Size (1),                */
+	0x09, 0x42,             /*          Usage (Tip Switch),             */
+	0x09, 0x44,             /*          Usage (Barrel Switch),          */
+	0x09, 0x46,             /*          Usage (Tablet Pick),            */
+	0x95, 0x03,             /*          Report Count (3),               */
+	0x81, 0x02,             /*          Input (Variable),               */
+	0x95, 0x03,             /*          Report Count (3),               */
+	0x81, 0x03,             /*          Input (Constant, Variable),     */
+	0x09, 0x32,             /*          Usage (In Range),               */
+	0x95, 0x01,             /*          Report Count (1),               */
+	0x81, 0x02,             /*          Input (Variable),               */
+	0x95, 0x01,             /*          Report Count (1),               */
+	0x81, 0x03,             /*          Input (Constant, Variable),     */
+	0x75, 0x10,             /*          Report Size (16),               */
+	0x95, 0x01,             /*          Report Count (1),               */
+	0xA4,                   /*          Push,                           */
+	0x05, 0x01,             /*          Usage Page (Desktop),           */
+	0x65, 0x13,             /*          Unit (Inch),                    */
+	0x55, 0xFD,             /*          Unit Exponent (-3),             */
+	0x34,                   /*          Physical Minimum (0),           */
+	0x09, 0x30,             /*          Usage (X),                      */
+	0x27, UCLOGIC_PH(X_LM), /*          Logical Maximum (PLACEHOLDER),  */
+	0x47, UCLOGIC_PH(X_PM), /*          Physical Maximum (PLACEHOLDER), */
+	0x81, 0x02,             /*          Input (Variable),               */
+	0x09, 0x31,             /*          Usage (Y),                      */
+	0x27, UCLOGIC_PH(Y_LM), /*          Logical Maximum (PLACEHOLDER),  */
+	0x47, UCLOGIC_PH(Y_PM), /*          Physical Maximum (PLACEHOLDER), */
+	0x81, 0x02,             /*          Input (Variable),               */
+	0xB4,                   /*          Pop,                            */
+	0x09, 0x30,             /*          Usage (Tip Pressure),           */
+	0x27,
+	UCLOGIC_PH(PRESSURE_LM),/*          Logical Maximum (PLACEHOLDER),  */
+	0x81, 0x02,             /*          Input (Variable),               */
+	0xC0,                   /*      End Collection,                     */
+	0xC0                    /*  End Collection                          */
+};
+
+/* Parameter indices */
+enum uclogic_prm {
+	UCLOGIC_PRM_X_LM	= 1,
+	UCLOGIC_PRM_Y_LM	= 2,
+	UCLOGIC_PRM_PRESSURE_LM	= 4,
+	UCLOGIC_PRM_RESOLUTION	= 5,
+	UCLOGIC_PRM_NUM
+};
+
+/* Driver data */
+struct uclogic_drvdata {
+	__u8 *rdesc;
+	unsigned int rsize;
+};
+
 static __u8 *uclogic_report_fixup(struct hid_device *hdev, __u8 *rdesc,
 					unsigned int *rsize)
 {
 	struct usb_interface *iface = to_usb_interface(hdev->dev.parent);
 	__u8 iface_num = iface->cur_altsetting->desc.bInterfaceNumber;
+	struct uclogic_drvdata *drvdata = hid_get_drvdata(hdev);
 
 	switch (hdev->product) {
 	case USB_DEVICE_ID_UCLOGIC_TABLET_PF1209:
@@ -621,15 +703,120 @@ static __u8 *uclogic_report_fixup(struct hid_device *hdev, __u8 *rdesc,
 			break;
 		}
 		break;
+	default:
+		if (drvdata->rdesc != NULL) {
+			rdesc = drvdata->rdesc;
+			*rsize = drvdata->rsize;
+		}
 	}
 
 	return rdesc;
 }
 
+/**
+ * Enable fully-functional tablet mode and determine device parameters.
+ *
+ * @hdev:	HID device
+ */
+static int uclogic_tablet_enable(struct hid_device *hdev)
+{
+	int rc;
+	struct usb_device *usb_dev = hid_to_usb_dev(hdev);
+	struct uclogic_drvdata *drvdata = hid_get_drvdata(hdev);
+	__le16 *buf = NULL;
+	size_t len;
+	s32 params[UCLOGIC_PH_ID_NUM];
+	s32 resolution;
+	__u8 *p;
+	s32 v;
+
+	/*
+	 * Read string descriptor containing tablet parameters. The specific
+	 * string descriptor and data were discovered by sniffing the Windows
+	 * driver traffic.
+	 * NOTE: This enables fully-functional tablet mode.
+	 */
+	len = UCLOGIC_PRM_NUM * sizeof(*buf);
+	buf = kmalloc(len, GFP_KERNEL);
+	if (buf == NULL) {
+		hid_err(hdev, "failed to allocate parameter buffer\n");
+		rc = -ENOMEM;
+		goto cleanup;
+	}
+	rc = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0),
+				USB_REQ_GET_DESCRIPTOR, USB_DIR_IN,
+				(USB_DT_STRING << 8) + 0x64,
+				0x0409, buf, len,
+				USB_CTRL_GET_TIMEOUT);
+	if (rc == -EPIPE) {
+		hid_err(hdev, "device parameters not found\n");
+		rc = -ENODEV;
+		goto cleanup;
+	} else if (rc < 0) {
+		hid_err(hdev, "failed to get device parameters: %d\n", rc);
+		rc = -ENODEV;
+		goto cleanup;
+	} else if (rc != len) {
+		hid_err(hdev, "invalid device parameters\n");
+		rc = -ENODEV;
+		goto cleanup;
+	}
+
+	/* Extract device parameters */
+	params[UCLOGIC_PH_ID_X_LM] = le16_to_cpu(buf[UCLOGIC_PRM_X_LM]);
+	params[UCLOGIC_PH_ID_Y_LM] = le16_to_cpu(buf[UCLOGIC_PRM_Y_LM]);
+	params[UCLOGIC_PH_ID_PRESSURE_LM] =
+		le16_to_cpu(buf[UCLOGIC_PRM_PRESSURE_LM]);
+	resolution = le16_to_cpu(buf[UCLOGIC_PRM_RESOLUTION]);
+	if (resolution == 0) {
+		params[UCLOGIC_PH_ID_X_PM] = 0;
+		params[UCLOGIC_PH_ID_Y_PM] = 0;
+	} else {
+		params[UCLOGIC_PH_ID_X_PM] = params[UCLOGIC_PH_ID_X_LM] *
+						1000 / resolution;
+		params[UCLOGIC_PH_ID_Y_PM] = params[UCLOGIC_PH_ID_Y_LM] *
+						1000 / resolution;
+	}
+
+	/* Allocate fixed report descriptor */
+	drvdata->rdesc = devm_kzalloc(&hdev->dev,
+				sizeof(uclogic_tablet_rdesc_template),
+				GFP_KERNEL);
+	if (drvdata->rdesc == NULL) {
+		hid_err(hdev, "failed to allocate fixed rdesc\n");
+		rc = -ENOMEM;
+		goto cleanup;
+	}
+	drvdata->rsize = sizeof(uclogic_tablet_rdesc_template);
+
+	/* Format fixed report descriptor */
+	memcpy(drvdata->rdesc, uclogic_tablet_rdesc_template,
+		drvdata->rsize);
+	for (p = drvdata->rdesc;
+	     p <= drvdata->rdesc + drvdata->rsize - 4;) {
+		if (p[0] == 0xFE && p[1] == 0xED && p[2] == 0x1D &&
+		    p[3] < sizeof(params)) {
+			v = params[p[3]];
+			put_unaligned(cpu_to_le32(v), (s32 *)p);
+			p += 4;
+		} else {
+			p++;
+		}
+	}
+
+	rc = 0;
+
+cleanup:
+	kfree(buf);
+	return rc;
+}
+
 static int uclogic_probe(struct hid_device *hdev,
 		const struct hid_device_id *id)
 {
 	int rc;
+	struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
+	struct uclogic_drvdata *drvdata;
 
 	/*
 	 * libinput requires the pad interface to be on a different node
@@ -637,6 +824,26 @@ static int uclogic_probe(struct hid_device *hdev,
 	 */
 	hdev->quirks |= HID_QUIRK_MULTI_INPUT;
 
+	/* Allocate and assign driver data */
+	drvdata = devm_kzalloc(&hdev->dev, sizeof(*drvdata), GFP_KERNEL);
+	if (drvdata == NULL)
+		return -ENOMEM;
+
+	hid_set_drvdata(hdev, drvdata);
+
+	switch (id->product) {
+	case USB_DEVICE_ID_HUION_TABLET:
+		/* If this is the pen interface */
+		if (intf->cur_altsetting->desc.bInterfaceNumber == 0) {
+			rc = uclogic_tablet_enable(hdev);
+			if (rc) {
+				hid_err(hdev, "tablet enabling failed\n");
+				return rc;
+			}
+		}
+		break;
+	}
+
 	rc = hid_parse(hdev);
 	if (rc) {
 		hid_err(hdev, "parse failed\n");
@@ -652,6 +859,21 @@ static int uclogic_probe(struct hid_device *hdev,
 	return 0;
 }
 
+static int uclogic_raw_event(struct hid_device *hdev, struct hid_report *report,
+			u8 *data, int size)
+{
+	struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
+
+	/* If this is a pen input report */
+	if (intf->cur_altsetting->desc.bInterfaceNumber == 0 &&
+	    report->type == HID_INPUT_REPORT &&
+	    report->id == 0x07 && size >= 2)
+		/* Invert the in-range bit */
+		data[1] ^= 0x40;
+
+	return 0;
+}
+
 static const struct hid_device_id uclogic_devices[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_UCLOGIC,
 				USB_DEVICE_ID_UCLOGIC_TABLET_PF1209) },
@@ -667,6 +889,8 @@ static const struct hid_device_id uclogic_devices[] = {
 				USB_DEVICE_ID_UCLOGIC_WIRELESS_TABLET_TWHL850) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_UCLOGIC,
 				USB_DEVICE_ID_UCLOGIC_TABLET_TWHA60) },
+	{ HID_USB_DEVICE(USB_VENDOR_ID_HUION, USB_DEVICE_ID_HUION_TABLET) },
+	{ HID_USB_DEVICE(USB_VENDOR_ID_UCLOGIC, USB_DEVICE_ID_HUION_TABLET) },
 	{ }
 };
 MODULE_DEVICE_TABLE(hid, uclogic_devices);
@@ -676,7 +900,10 @@ static struct hid_driver uclogic_driver = {
 	.id_table = uclogic_devices,
 	.probe = uclogic_probe,
 	.report_fixup = uclogic_report_fixup,
+	.raw_event = uclogic_raw_event,
 };
 module_hid_driver(uclogic_driver);
 
+MODULE_AUTHOR("Martin Rusko");
+MODULE_AUTHOR("Nikolai Kondrashov");
 MODULE_LICENSE("GPL");
-- 
2.1.0

[PATCH v2 3/6] HID: uclogic: name the input nodes based on their tool

[Benjamin Tissoires]

We append "Pen", "Pad", "Mouse", "Keyboard", "Consumer Control" or
"System Control" suffix to the appropriate input node to match what
the Wacom driver does and be more convenient for the user to know
which one is which.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---

changes in v2:
- completed the list with "Consumer Control" and "System Control" to
  force having a suffix to each input node on the Huion H610 Pro

 drivers/hid/hid-uclogic.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/drivers/hid/hid-uclogic.c b/drivers/hid/hid-uclogic.c
index 397f1df..90e2612 100644
--- a/drivers/hid/hid-uclogic.c
+++ b/drivers/hid/hid-uclogic.c
@@ -713,6 +713,51 @@ static __u8 *uclogic_report_fixup(struct hid_device *hdev, __u8 *rdesc,
 	return rdesc;
 }
 
+static void uclogic_input_configured(struct hid_device *hdev,
+		struct hid_input *hi)
+{
+	char *name;
+	const char *suffix = NULL;
+	struct hid_field *field;
+	size_t len;
+
+	/* no report associated (HID_QUIRK_MULTI_INPUT not set) */
+	if (!hi->report)
+		return;
+
+	field = hi->report->field[0];
+
+	switch (field->application) {
+	case HID_GD_KEYBOARD:
+		suffix = "Keyboard";
+		break;
+	case HID_GD_MOUSE:
+		suffix = "Mouse";
+		break;
+	case HID_GD_KEYPAD:
+		suffix = "Pad";
+		break;
+	case HID_DG_PEN:
+		suffix = "Pen";
+		break;
+	case HID_CP_CONSUMER_CONTROL:
+		suffix = "Consumer Control";
+		break;
+	case HID_GD_SYSTEM_CONTROL:
+		suffix = "System Control";
+		break;
+	}
+
+	if (suffix) {
+		len = strlen(hdev->name) + 2 + strlen(suffix);
+		name = devm_kzalloc(&hi->input->dev, len, GFP_KERNEL);
+		if (name) {
+			snprintf(name, len, "%s %s", hdev->name, suffix);
+			hi->input->name = name;
+		}
+	}
+}
+
 /**
  * Enable fully-functional tablet mode and determine device parameters.
  *
@@ -901,6 +946,7 @@ static struct hid_driver uclogic_driver = {
 	.probe = uclogic_probe,
 	.report_fixup = uclogic_report_fixup,
 	.raw_event = uclogic_raw_event,
+	.input_configured = uclogic_input_configured,
 };
 module_hid_driver(uclogic_driver);
 
-- 
2.1.0

Re: [PATCH v2 3/6] HID: uclogic: name the input nodes based on their tool

[Nikolai Kondrashov]

On 02/26/2015 08:57 PM, Benjamin Tissoires wrote:
> We append "Pen", "Pad", "Mouse", "Keyboard", "Consumer Control" or
> "System Control" suffix to the appropriate input node to match what
> the Wacom driver does and be more convenient for the user to know
> which one is which.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

I read the kernel code and tested this a little. This looks and works fine.

Reviewed-by: Nikolai Kondrashov <spbnick@gmail.com>

[PATCH v2 4/6] HID: uclogic: apply quirk NO_EMPTY_INPUT

[Benjamin Tissoires]

NO_EMPTY_INPUT is useful when MULTI_INPUT is set. It prevents to create
empty input nodes that user space does not know what to do with.

It does not seem to be required at the moment, this is just a preventive
patch. This check is only made during the plug of the device, so it does
not hurt to have it.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---

New in v2

 drivers/hid/hid-uclogic.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/hid/hid-uclogic.c b/drivers/hid/hid-uclogic.c
index 90e2612..ada8a94 100644
--- a/drivers/hid/hid-uclogic.c
+++ b/drivers/hid/hid-uclogic.c
@@ -868,6 +868,7 @@ static int uclogic_probe(struct hid_device *hdev,
 	 * than the pen, so use QUIRK_MULTI_INPUT for all tablets.
 	 */
 	hdev->quirks |= HID_QUIRK_MULTI_INPUT;
+	hdev->quirks |= HID_QUIRK_NO_EMPTY_INPUT;
 
 	/* Allocate and assign driver data */
 	drvdata = devm_kzalloc(&hdev->dev, sizeof(*drvdata), GFP_KERNEL);
-- 
2.1.0

Re: [PATCH v2 4/6] HID: uclogic: apply quirk NO_EMPTY_INPUT

[Nikolai Kondrashov]

On 02/26/2015 08:57 PM, Benjamin Tissoires wrote:
> NO_EMPTY_INPUT is useful when MULTI_INPUT is set. It prevents to create
> empty input nodes that user space does not know what to do with.
>
> It does not seem to be required at the moment, this is just a preventive
> patch. This check is only made during the plug of the device, so it does
> not hurt to have it.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>
> New in v2
>
>   drivers/hid/hid-uclogic.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/hid/hid-uclogic.c b/drivers/hid/hid-uclogic.c
> index 90e2612..ada8a94 100644
> --- a/drivers/hid/hid-uclogic.c
> +++ b/drivers/hid/hid-uclogic.c
> @@ -868,6 +868,7 @@ static int uclogic_probe(struct hid_device *hdev,
>   	 * than the pen, so use QUIRK_MULTI_INPUT for all tablets.
>   	 */
>   	hdev->quirks |= HID_QUIRK_MULTI_INPUT;
> +	hdev->quirks |= HID_QUIRK_NO_EMPTY_INPUT;
>
>   	/* Allocate and assign driver data */
>   	drvdata = devm_kzalloc(&hdev->dev, sizeof(*drvdata), GFP_KERNEL);

This seems safe and fine as a preventive.

Reviewed-by: Nikolai Kondrashov <spbnick@gmail.com>

Nick

[PATCH v2 5/6] HID: uclogic: discard the extra Pen input node on Huion tablets

[Benjamin Tissoires]

Some Huion tablets present 2 HID Pen interfaces. Only one is used, so
we can drop the unused one.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---

New in v2

 drivers/hid/hid-uclogic.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/hid/hid-uclogic.c b/drivers/hid/hid-uclogic.c
index ada8a94..f44e72b 100644
--- a/drivers/hid/hid-uclogic.c
+++ b/drivers/hid/hid-uclogic.c
@@ -713,6 +713,25 @@ static __u8 *uclogic_report_fixup(struct hid_device *hdev, __u8 *rdesc,
 	return rdesc;
 }
 
+static int uclogic_input_mapping(struct hid_device *hdev, struct hid_input *hi,
+		struct hid_field *field, struct hid_usage *usage,
+		unsigned long **bit, int *max)
+{
+	struct usb_interface *intf;
+
+	if (hdev->product == USB_DEVICE_ID_HUION_TABLET) {
+		intf = to_usb_interface(hdev->dev.parent);
+
+		/* discard the unused pen interface */
+		if ((intf->cur_altsetting->desc.bInterfaceNumber != 0) &&
+		    (field->application == HID_DG_PEN))
+			return -1;
+	}
+
+	/* let hid-core decide what to do */
+	return 0;
+}
+
 static void uclogic_input_configured(struct hid_device *hdev,
 		struct hid_input *hi)
 {
@@ -947,6 +966,7 @@ static struct hid_driver uclogic_driver = {
 	.probe = uclogic_probe,
 	.report_fixup = uclogic_report_fixup,
 	.raw_event = uclogic_raw_event,
+	.input_mapping = uclogic_input_mapping,
 	.input_configured = uclogic_input_configured,
 };
 module_hid_driver(uclogic_driver);
-- 
2.1.0

Re: [PATCH v2 5/6] HID: uclogic: discard the extra Pen input node on Huion tablets

[Nikolai Kondrashov]

On 02/26/2015 08:57 PM, Benjamin Tissoires wrote:
> Some Huion tablets present 2 HID Pen interfaces. Only one is used, so
> we can drop the unused one.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

This is what might have been plaguing GIMP users. Thanks, Benjamin!

Reviewed-by: Nikolai Kondrashov <spbnick@gmail.com>

Nick

[PATCH v2 6/6] HID: uclogic: actually invert the in-range bit for huion tablets only

[Benjamin Tissoires]

This hack is only needed for Huion tablets. It does not seem to have
any effect on the other tablets handled by this device right now, but
it's better to check for the product id sooner than discovering that
we have messed up one tablet later.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---

New in v2

 drivers/hid/hid-uclogic.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-uclogic.c b/drivers/hid/hid-uclogic.c
index f44e72b..5f5c16e 100644
--- a/drivers/hid/hid-uclogic.c
+++ b/drivers/hid/hid-uclogic.c
@@ -930,7 +930,8 @@ static int uclogic_raw_event(struct hid_device *hdev, struct hid_report *report,
 	struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
 
 	/* If this is a pen input report */
-	if (intf->cur_altsetting->desc.bInterfaceNumber == 0 &&
+	if (hdev->product == USB_DEVICE_ID_HUION_TABLET &&
+	    intf->cur_altsetting->desc.bInterfaceNumber == 0 &&
 	    report->type == HID_INPUT_REPORT &&
 	    report->id == 0x07 && size >= 2)
 		/* Invert the in-range bit */
-- 
2.1.0

Re: [PATCH v2 6/6] HID: uclogic: actually invert the in-range bit for huion tablets only

[Nikolai Kondrashov]

On 02/26/2015 08:57 PM, Benjamin Tissoires wrote:
> This hack is only needed for Huion tablets. It does not seem to have
> any effect on the other tablets handled by this device right now, but
> it's better to check for the product id sooner than discovering that
> we have messed up one tablet later.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>
> New in v2
>
>   drivers/hid/hid-uclogic.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-uclogic.c b/drivers/hid/hid-uclogic.c
> index f44e72b..5f5c16e 100644
> --- a/drivers/hid/hid-uclogic.c
> +++ b/drivers/hid/hid-uclogic.c
> @@ -930,7 +930,8 @@ static int uclogic_raw_event(struct hid_device *hdev, struct hid_report *report,
>   	struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
>
>   	/* If this is a pen input report */
> -	if (intf->cur_altsetting->desc.bInterfaceNumber == 0 &&
> +	if (hdev->product == USB_DEVICE_ID_HUION_TABLET &&
> +	    intf->cur_altsetting->desc.bInterfaceNumber == 0 &&
>   	    report->type == HID_INPUT_REPORT &&
>   	    report->id == 0x07 && size >= 2)
>   		/* Invert the in-range bit */

Right, I completely forgot about this. That's why it's important to put these
things out for testing.

However, this should be inverted not just for Huion tablets, but rather for
all "enabled" tablets. The UC-Logic TWHA60 v3 inverts that bit as well when
enabled, IIRC. We can filter enabled tablets by drvdata->rdesc != NULL, or add
an explicit flag.

Nick

Re: [PATCH v2 6/6] HID: uclogic: actually invert the in-range bit for huion tablets only

[Benjamin Tissoires]

On Feb 28 2015 or thereabouts, Nikolai Kondrashov wrote:
> On 02/26/2015 08:57 PM, Benjamin Tissoires wrote:
> >This hack is only needed for Huion tablets. It does not seem to have
> >any effect on the other tablets handled by this device right now, but
> >it's better to check for the product id sooner than discovering that
> >we have messed up one tablet later.
> >
> >Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> >---
> >
> >New in v2
> >
> >  drivers/hid/hid-uclogic.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/hid/hid-uclogic.c b/drivers/hid/hid-uclogic.c
> >index f44e72b..5f5c16e 100644
> >--- a/drivers/hid/hid-uclogic.c
> >+++ b/drivers/hid/hid-uclogic.c
> >@@ -930,7 +930,8 @@ static int uclogic_raw_event(struct hid_device *hdev, struct hid_report *report,
> >  	struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
> >
> >  	/* If this is a pen input report */
> >-	if (intf->cur_altsetting->desc.bInterfaceNumber == 0 &&
> >+	if (hdev->product == USB_DEVICE_ID_HUION_TABLET &&
> >+	    intf->cur_altsetting->desc.bInterfaceNumber == 0 &&
> >  	    report->type == HID_INPUT_REPORT &&
> >  	    report->id == 0x07 && size >= 2)
> >  		/* Invert the in-range bit */
> 
> Right, I completely forgot about this. That's why it's important to put these
> things out for testing.

yes, and no. A thorough review can help detect these while it's a pain
to ask each and every users to test every possible device :)

> 
> However, this should be inverted not just for Huion tablets, but rather for
> all "enabled" tablets. The UC-Logic TWHA60 v3 inverts that bit as well when
> enabled, IIRC. We can filter enabled tablets by drvdata->rdesc != NULL, or add
> an explicit flag.

I like the flag. It will remove the dependency to usb in raw_event()
which is always a plus.

Speaking of which. I started looking at these dependencies in the hid
subtree (like every year, in spring, these dependencies itches me).
I think we can find a preventive solution which would allow to reinject
through uhid any device even if the driver wants to talk to the usb
layer. This will require some more effort on my side but will allow us
to have at least some regressions tests in hid-uclogic too.

Cheers,
Benjamin

Re: [PATCH v2 6/6] HID: uclogic: actually invert the in-range bit for huion tablets only

[Nikolai Kondrashov]

On 03/03/2015 12:25 AM, Benjamin Tissoires wrote:
> On Feb 28 2015 or thereabouts, Nikolai Kondrashov wrote:
>> On 02/26/2015 08:57 PM, Benjamin Tissoires wrote:
>>> This hack is only needed for Huion tablets. It does not seem to have
>>> any effect on the other tablets handled by this device right now, but
>>> it's better to check for the product id sooner than discovering that
>>> we have messed up one tablet later.
>>>
>>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>>> ---
>>>
>>> New in v2
>>>
>>>   drivers/hid/hid-uclogic.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/hid/hid-uclogic.c b/drivers/hid/hid-uclogic.c
>>> index f44e72b..5f5c16e 100644
>>> --- a/drivers/hid/hid-uclogic.c
>>> +++ b/drivers/hid/hid-uclogic.c
>>> @@ -930,7 +930,8 @@ static int uclogic_raw_event(struct hid_device *hdev, struct hid_report *report,
>>>   	struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
>>>
>>>   	/* If this is a pen input report */
>>> -	if (intf->cur_altsetting->desc.bInterfaceNumber == 0 &&
>>> +	if (hdev->product == USB_DEVICE_ID_HUION_TABLET &&
>>> +	    intf->cur_altsetting->desc.bInterfaceNumber == 0 &&
>>>   	    report->type == HID_INPUT_REPORT &&
>>>   	    report->id == 0x07 && size >= 2)
>>>   		/* Invert the in-range bit */
>>
>> Right, I completely forgot about this. That's why it's important to put these
>> things out for testing.
>
> yes, and no. A thorough review can help detect these while it's a pain
> to ask each and every users to test every possible device :)

Perhaps :)

>> However, this should be inverted not just for Huion tablets, but rather for
>> all "enabled" tablets. The UC-Logic TWHA60 v3 inverts that bit as well when
>> enabled, IIRC. We can filter enabled tablets by drvdata->rdesc != NULL, or add
>> an explicit flag.
>
> I like the flag. It will remove the dependency to usb in raw_event()
> which is always a plus.

Great!

> Speaking of which. I started looking at these dependencies in the hid
> subtree (like every year, in spring, these dependencies itches me).
> I think we can find a preventive solution which would allow to reinject
> through uhid any device even if the driver wants to talk to the usb
> layer. This will require some more effort on my side but will allow us
> to have at least some regressions tests in hid-uclogic too.

This sounds good. I was thinking about record/replay on HID level over uhid,
similar to evdev level, but as usual didn't have time to even think it through
(it's not my day job).

Nick