[RFC 0/8] HID: add support for USI style pens

[RFC 0/8] HID: add support for USI style pens

[Tero Kristo]

Hi,

This series is an RFC for USI (Universal Stylus Interface) style pen
support. This is based on documentation from USB org describing the HID
usage tables for digitizers (page 0x0D) and experimentation with actual
USI capable controllers.

This series introduces the USI support with a new HID driver, which
applies the controller specific quirks. The most problematic part of the
USI support is handling of the pen parameters (color, line width, line
style), which are not immediately available from the controller from pen
down event, but must be cached and queried separately from the controller.
In addition to that, when a get-feature report is sent to the
controller, there is a delay before the proper value is reported out; it
is not part of the feature report coming back immediately.
Most of the code in the driver is to handle this (otherwise we could
just use hid-generic.)

This also boils down to the reason why this series is an RFC, I would like
to receive some feedback which option to pick for programming of the new
values for the programmable pen parameters; whether to parse the input
events so userspace can directly write the new values to the input event
file handle, or whether to use IOCTL. Patches #7 / #8 are sort of optional
choices towards this, but are there to show that both approaches can be
done. Direct write to evdev causes some confusion on the driver level
though, thus patch #7 is there to avoid some of that introducing new
input events for writing the parameters. IOCTL might be the cleanest
approach and I am slightly leaning towards that myself (see patch #8,
this would need to be squashed and cleaned up a bit though but would
effectively get rid of some code from patch #6 and completely rid patch #7.)

The driver has been tested with chromebooks that contain either Goodix
or Elan manufactured USI capable touchscreen controllers in them.

Any feedback appreciated!

-Tero

[RFC 1/8] HID: debug: Add USI usages

[Tero Kristo]

From: Mika Westerberg <mika.westerberg@linux.intel.com>

Add USI defined usages to the HID debug code.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
[tero.kristo: squashed in EV_MSC definitions]
Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
---
 drivers/hid/hid-debug.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index fa57d05badf7..9d617f231591 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -141,8 +141,10 @@ static const struct hid_usage_entry hid_usage_table[] = {
     {0, 0x33, "Touch"},
     {0, 0x34, "UnTouch"},
     {0, 0x35, "Tap"},
+    {0, 0x38, "Transducer Index"},
     {0, 0x39, "TabletFunctionKey"},
     {0, 0x3a, "ProgramChangeKey"},
+    {0, 0x3B, "Battery Strength"},
     {0, 0x3c, "Invert"},
     {0, 0x42, "TipSwitch"},
     {0, 0x43, "SecondaryTipSwitch"},
@@ -160,6 +162,39 @@ static const struct hid_usage_entry hid_usage_table[] = {
     {0, 0x59, "ButtonType"},
     {0, 0x5A, "SecondaryBarrelSwitch"},
     {0, 0x5B, "TransducerSerialNumber"},
+    {0, 0x5C, "Preferred Color"},
+    {0, 0x5D, "Preferred Color is Locked"},
+    {0, 0x5E, "Preferred Line Width"},
+    {0, 0x5F, "Preferred Line Width is Locked"},
+    {0, 0x70, "Preferred Line Style"},
+      {0, 0x71, "Preferred Line Style is Locked"},
+      {0, 0x72, "Ink"},
+      {0, 0x73, "Pencil"},
+      {0, 0x74, "Highlighter"},
+      {0, 0x75, "Chisel Marker"},
+      {0, 0x76, "Brush"},
+      {0, 0x77, "No Preference"},
+    {0, 0x80, "Digitizer Diagnostic"},
+    {0, 0x81, "Digitizer Error"},
+      {0, 0x82, "Err Normal Status"},
+      {0, 0x83, "Err Transducers Exceeded"},
+      {0, 0x84, "Err Full Trans Features Unavailable"},
+      {0, 0x85, "Err Charge Low"},
+    {0, 0x90, "Transducer Software Info"},
+      {0, 0x91, "Transducer Vendor Id"},
+      {0, 0x92, "Transducer Product Id"},
+    {0, 0x93, "Device Supported Protocols"},
+    {0, 0x94, "Transducer Supported Protocols"},
+      {0, 0x95, "No Protocol"},
+      {0, 0x96, "Wacom AES Protocol"},
+      {0, 0x97, "USI Protocol"},
+      {0, 0x98, "Microsoft Pen Protocol"},
+    {0, 0xA0, "Supported Report Rates"},
+      {0, 0xA1, "Report Rate"},
+      {0, 0xA2, "Transducer Connected"},
+      {0, 0xA3, "Switch Disabled"},
+      {0, 0xA4, "Switch Unimplemented"},
+      {0, 0xA5, "Transducer Switches"},
   { 15, 0, "PhysicalInterfaceDevice" },
     {0, 0x00, "Undefined"},
     {0, 0x01, "Physical_Interface_Device"},
@@ -990,7 +1025,10 @@ static const char *absolutes[ABS_CNT] = {
 
 static const char *misc[MSC_MAX + 1] = {
 	[MSC_SERIAL] = "Serial",	[MSC_PULSELED] = "Pulseled",
-	[MSC_GESTURE] = "Gesture",	[MSC_RAW] = "RawData"
+	[MSC_GESTURE] = "Gesture",	[MSC_RAW] = "RawData",
+	[MSC_PEN_ID] = "PenID",		[MSC_PEN_COLOR] "PenColor",
+	[MSC_PEN_LINE_WIDTH] = "PenLineWidth",
+	[MSC_PEN_LINE_STYLE] = "PenLineStyle",
 };
 
 static const char *leds[LED_MAX + 1] = {
-- 
2.25.1

[RFC 2/8] HID: Add map_msc() to avoid boilerplate code

[Tero Kristo]

From: Mika Westerberg <mika.westerberg@linux.intel.com>

Since we are going to have more MSC events too, add map_msc() that can
be used to fill in necessary fields and avoid boilerplate code.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
---
 drivers/hid/hid-input.c | 6 ++----
 include/linux/hid.h     | 4 ++++
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 4b5ebeacd283..e1f7cef52afa 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -52,6 +52,7 @@ static const struct {
 #define map_rel(c)	hid_map_usage(hidinput, usage, &bit, &max, EV_REL, (c))
 #define map_key(c)	hid_map_usage(hidinput, usage, &bit, &max, EV_KEY, (c))
 #define map_led(c)	hid_map_usage(hidinput, usage, &bit, &max, EV_LED, (c))
+#define map_msc(c)	hid_map_usage(hidinput, usage, &bit, &max, EV_MSC, (c))
 
 #define map_abs_clear(c)	hid_map_usage_clear(hidinput, usage, &bit, \
 		&max, EV_ABS, (c))
@@ -871,10 +872,7 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
 			break;
 
 		case 0x5b: /* TransducerSerialNumber */
-			usage->type = EV_MSC;
-			usage->code = MSC_SERIAL;
-			bit = input->mscbit;
-			max = MSC_MAX;
+			map_msc(MSC_SERIAL);
 			break;
 
 		default:  goto unknown;
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 9e067f937dbc..010c8bcbee90 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -1000,6 +1000,10 @@ static inline void hid_map_usage(struct hid_input *hidinput,
 		bmap = input->ledbit;
 		limit = LED_MAX;
 		break;
+	case EV_MSC:
+		bmap = input->mscbit;
+		limit = MSC_MAX;
+		break;
 	}
 
 	if (unlikely(c > limit || !bmap)) {
-- 
2.25.1

[RFC 3/8] HID: hid-input: Add suffix also for HID_DG_PEN

[Tero Kristo]

From: Mika Westerberg <mika.westerberg@linux.intel.com>

This and HID_DG_STYLUS are pretty much the same thing so add suffix for
HID_DG_PEN too. This makes the input device name look better.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
---
 drivers/hid/hid-input.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index e1f7cef52afa..05449fa59dc4 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1736,6 +1736,7 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid,
 		case HID_GD_MOUSE:
 			suffix = "Mouse";
 			break;
+		case HID_DG_PEN:
 		case HID_DG_STYLUS:
 			suffix = "Pen";
 			break;
-- 
2.25.1

[RFC 4/8] HID: input: Make hidinput_find_field() static

[Tero Kristo]

From: Mika Westerberg <mika.westerberg@linux.intel.com>

This function is not called outside of hid-input.c so we can make it
static.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
---
 drivers/hid/hid-input.c | 4 ++--
 include/linux/hid.h     | 1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 05449fa59dc4..0506612800db 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1458,7 +1458,8 @@ void hidinput_report_event(struct hid_device *hid, struct hid_report *report)
 }
 EXPORT_SYMBOL_GPL(hidinput_report_event);
 
-int hidinput_find_field(struct hid_device *hid, unsigned int type, unsigned int code, struct hid_field **field)
+static int hidinput_find_field(struct hid_device *hid, unsigned int type,
+			       unsigned int code, struct hid_field **field)
 {
 	struct hid_report *report;
 	int i, j;
@@ -1473,7 +1474,6 @@ int hidinput_find_field(struct hid_device *hid, unsigned int type, unsigned int
 	}
 	return -1;
 }
-EXPORT_SYMBOL_GPL(hidinput_find_field);
 
 struct hid_field *hidinput_get_led_field(struct hid_device *hid)
 {
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 010c8bcbee90..555b5de654b1 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -889,7 +889,6 @@ extern void hidinput_disconnect(struct hid_device *);
 
 int hid_set_field(struct hid_field *, unsigned, __s32);
 int hid_input_report(struct hid_device *, int type, u8 *, u32, int);
-int hidinput_find_field(struct hid_device *hid, unsigned int type, unsigned int code, struct hid_field **field);
 struct hid_field *hidinput_get_led_field(struct hid_device *hid);
 unsigned int hidinput_count_leds(struct hid_device *hid);
 __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code);
-- 
2.25.1

[RFC 5/8] HID: core: Add support for USI style events

[Tero Kristo]

Add support for Universal Stylus Interface (USI) style events to the HID
core and input layers. A new driver will be added in a separate patch
which is going to use this glue logic.

Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
---
 drivers/hid/hid-core.c                 |  3 +++
 drivers/hid/hid-input.c                | 18 ++++++++++++++++++
 include/linux/hid.h                    |  5 +++++
 include/linux/mod_devicetable.h        |  2 +-
 include/uapi/linux/input-event-codes.h | 22 ++++++++++++++--------
 5 files changed, 41 insertions(+), 9 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index dbed2524fd47..194c4bfed1be 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -780,6 +780,9 @@ static void hid_scan_input_usage(struct hid_parser *parser, u32 usage)
 
 	if (usage == HID_DG_CONTACTID)
 		hid->group = HID_GROUP_MULTITOUCH;
+
+	if (usage == HID_DG_TRANSDUCER_INDEX)
+		hid->group = HID_GROUP_USI;
 }
 
 static void hid_scan_feature_usage(struct hid_parser *parser, u32 usage)
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 0506612800db..426700cd8574 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -829,6 +829,10 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
 			}
 			break;
 
+		case 0x38: /* Transducer Index */
+			map_msc(MSC_PEN_ID);
+			break;
+
 		case 0x3b: /* Battery Strength */
 			hidinput_setup_battery(device, HID_INPUT_REPORT, field, false);
 			usage->type = EV_PWR;
@@ -875,6 +879,20 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
 			map_msc(MSC_SERIAL);
 			break;
 
+		case 0x5c: map_msc(MSC_PEN_COLOR);		break;
+		case 0x5e: map_msc(MSC_PEN_LINE_WIDTH);		break;
+
+		case 0x70:
+		case 0x71:
+		case 0x72:
+		case 0x73:
+		case 0x74:
+		case 0x75:
+		case 0x76:
+		case 0x77:
+			map_msc(MSC_PEN_LINE_STYLE);
+			break;
+
 		default:  goto unknown;
 		}
 		break;
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 555b5de654b1..000fd3cf06ce 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -241,6 +241,7 @@ struct hid_item {
 #define HID_DG_TOUCH		0x000d0033
 #define HID_DG_UNTOUCH		0x000d0034
 #define HID_DG_TAP		0x000d0035
+#define HID_DG_TRANSDUCER_INDEX	0x000d0038
 #define HID_DG_TABLETFUNCTIONKEY	0x000d0039
 #define HID_DG_PROGRAMCHANGEKEY	0x000d003a
 #define HID_DG_BATTERYSTRENGTH	0x000d003b
@@ -253,6 +254,9 @@ struct hid_item {
 #define HID_DG_BARRELSWITCH	0x000d0044
 #define HID_DG_ERASER		0x000d0045
 #define HID_DG_TABLETPICK	0x000d0046
+#define HID_DG_PEN_COLOR	0x000d005c
+#define HID_DG_PEN_LINE_WIDTH	0x000d005e
+#define HID_DG_PEN_LINE_STYLE	0x000d0070
 
 #define HID_CP_CONSUMERCONTROL	0x000c0001
 #define HID_CP_NUMERICKEYPAD	0x000c0002
@@ -369,6 +373,7 @@ struct hid_item {
 #define HID_GROUP_MULTITOUCH			0x0002
 #define HID_GROUP_SENSOR_HUB			0x0003
 #define HID_GROUP_MULTITOUCH_WIN_8		0x0004
+#define HID_GROUP_USI				0x0005
 
 /*
  * Vendor specific HID device groups
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index ae2e75d15b21..4ff40be7676b 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -322,7 +322,7 @@ struct pcmcia_device_id {
 #define INPUT_DEVICE_ID_KEY_MAX		0x2ff
 #define INPUT_DEVICE_ID_REL_MAX		0x0f
 #define INPUT_DEVICE_ID_ABS_MAX		0x3f
-#define INPUT_DEVICE_ID_MSC_MAX		0x07
+#define INPUT_DEVICE_ID_MSC_MAX		0x09
 #define INPUT_DEVICE_ID_LED_MAX		0x0f
 #define INPUT_DEVICE_ID_SND_MAX		0x07
 #define INPUT_DEVICE_ID_FF_MAX		0x7f
diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
index 225ec87d4f22..98295f71941a 100644
--- a/include/uapi/linux/input-event-codes.h
+++ b/include/uapi/linux/input-event-codes.h
@@ -901,14 +901,20 @@
  * Misc events
  */
 
-#define MSC_SERIAL		0x00
-#define MSC_PULSELED		0x01
-#define MSC_GESTURE		0x02
-#define MSC_RAW			0x03
-#define MSC_SCAN		0x04
-#define MSC_TIMESTAMP		0x05
-#define MSC_MAX			0x07
-#define MSC_CNT			(MSC_MAX+1)
+#define MSC_SERIAL			0x00
+#define MSC_PULSELED			0x01
+#define MSC_GESTURE			0x02
+#define MSC_RAW				0x03
+#define MSC_SCAN			0x04
+#define MSC_TIMESTAMP			0x05
+/* USI Pen events */
+#define MSC_PEN_ID			0x06
+#define MSC_PEN_COLOR			0x07
+#define MSC_PEN_LINE_WIDTH		0x08
+#define MSC_PEN_LINE_STYLE		0x09
+/* TODO: Add USI diagnostic & battery events too */
+#define MSC_MAX				0x09
+#define MSC_CNT				(MSC_MAX + 1)
 
 /*
  * LEDs
-- 
2.25.1

[RFC 6/8] HID: usi: Add driver for Universal Stylus Interface (USI)

[Tero Kristo]

Universal Stylus Interface (USI) is a new HID hardware spec for
supporting Pen input devices. These devices have a few additional
features compared to the existing HID pen support, and they are mapped
via the EV_MSC interface. A new HID driver is added for supporting the
USI pens, to isolate the quirks these devices have.

USI HID events are documented in the public USB-HID usage table:
    https://usb.org/document-library/hid-usage-tables-122

See chapter 16, Digitizers Page (0x0D)

Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
---
 drivers/hid/Kconfig   |   5 +
 drivers/hid/Makefile  |   1 +
 drivers/hid/hid-usi.c | 774 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/hid.h   |   7 +
 4 files changed, 787 insertions(+)
 create mode 100644 drivers/hid/hid-usi.c

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 3c33bf572d6d..c235ecb8f037 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -1222,6 +1222,11 @@ config HID_MCP2221
 	To compile this driver as a module, choose M here: the module
 	will be called hid-mcp2221.ko.
 
+config HID_USI
+	tristate "USI (Universal Stylus Interface) support"
+	help
+	Provides USI support over I2C HID interface.
+
 endmenu
 
 endif # HID
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index e29efcb1c040..86bafd2b147e 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -124,6 +124,7 @@ hid-uclogic-objs		:= hid-uclogic-core.o \
 				   hid-uclogic-params.o
 obj-$(CONFIG_HID_UCLOGIC)	+= hid-uclogic.o
 obj-$(CONFIG_HID_UDRAW_PS3)	+= hid-udraw-ps3.o
+obj-$(CONFIG_HID_USI)		+= hid-usi.o
 obj-$(CONFIG_HID_LED)		+= hid-led.o
 obj-$(CONFIG_HID_XINMO)		+= hid-xinmo.o
 obj-$(CONFIG_HID_ZEROPLUS)	+= hid-zpff.o
diff --git a/drivers/hid/hid-usi.c b/drivers/hid/hid-usi.c
new file mode 100644
index 000000000000..4674e6993f6e
--- /dev/null
+++ b/drivers/hid/hid-usi.c
@@ -0,0 +1,774 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ *  HID driver for USI (Universal Stylus Interface)
+ *
+ *  Copyright (C) 2021, Intel Corporation
+ *  Authors: Tero Kristo <tero.kristo@linux.intel.com>
+ *	     Mika Westerberg <mika.westerberg@linux.intel.com>
+ *	     Rajmohan Mani <rajmohan.mani@intel.com>
+ */
+
+#include <linux/module.h>
+#include <linux/sysfs.h>
+#include <linux/device.h>
+#include <linux/hid.h>
+#include <linux/input.h>
+#include <linux/leds.h>
+#include <linux/workqueue.h>
+
+#include "hid-ids.h"
+
+#define USI_HAS_PENS		0
+#define USI_PENS_CONFIGURED	1
+
+enum {
+	USI_PEN_FLAGS = 0,
+	USI_PEN_ID,
+	USI_PEN_COLOR,
+	USI_PEN_LINE_WIDTH,
+	USI_PEN_LINE_STYLE,
+	USI_NUM_ATTRS
+};
+
+enum {
+	USI_QUIRK_STYLE_MAX_VAL = 0,
+	USI_QUIRK_QUERY_DATA,
+};
+
+#define USI_MAX_PENS	10
+#define MSC_PEN_FIRST	MSC_PEN_ID
+#define MSC_PEN_LAST	MSC_PEN_LINE_STYLE
+
+struct usi_pen {
+	int index;
+	int values[MSC_PEN_LAST - MSC_PEN_FIRST + 1];
+};
+
+struct usi_drvdata {
+	struct hid_report *features[USI_NUM_ATTRS];
+	struct hid_field *inputs[USI_NUM_ATTRS];
+	unsigned long quirks;
+	unsigned long timeout;
+	u8 in_range_bit;
+	u8 tip_switch_bit;
+	int min_pen;
+	int max_pen;
+	unsigned long flags;
+	struct usi_pen *pens;
+	unsigned long sync_point;
+	size_t npens;
+	struct input_dev *idev;
+	struct hid_device *hdev;
+	struct usi_pen *current_pen;
+	unsigned long query_pending;
+	unsigned long update_pending;
+	unsigned long query_running;
+	bool need_flush;
+	struct delayed_work work;
+	u8 saved_data[USI_NUM_ATTRS];
+	spinlock_t lock; /* private data lock */
+};
+
+static bool _has_quirk(struct usi_drvdata *usi, int quirk)
+{
+	if (test_bit(quirk, &usi->quirks))
+		return true;
+
+	return false;
+}
+
+static int __map_usage(struct hid_usage *usage, struct hid_field *field,
+		       u8 *bit)
+{
+	if (usage->hid == HID_DG_TRANSDUCER_INDEX)
+		return USI_PEN_ID;
+
+	if (usage->hid == HID_DG_INRANGE ||
+	    usage->hid == HID_DG_TIPSWITCH) {
+		*bit = usage->usage_index;
+		return USI_PEN_FLAGS;
+	}
+
+	if (usage->hid == HID_DG_PEN_COLOR)
+		return USI_PEN_COLOR;
+
+	if (usage->hid == HID_DG_PEN_LINE_WIDTH)
+		return USI_PEN_LINE_WIDTH;
+
+	if (field->logical == HID_DG_PEN_LINE_STYLE &&
+	    usage->hid >= HID_DG_PEN_LINE_STYLE_IS_LOCKED &&
+	    usage->hid <= HID_DG_PEN_LINE_STYLE_NO_PREFERENCE)
+		return USI_PEN_LINE_STYLE;
+
+	return -EINVAL;
+}
+
+static inline int __usi_to_msc_id(int id)
+{
+	if (id < USI_PEN_ID || id > USI_PEN_LINE_STYLE)
+		return -EINVAL;
+
+	return id - USI_PEN_ID + MSC_PEN_ID;
+}
+
+static inline int __msc_to_usi_id(unsigned int code)
+{
+	if (code < MSC_PEN_ID || code > MSC_PEN_LINE_STYLE)
+		return -EINVAL;
+
+	return code - MSC_PEN_ID + USI_PEN_ID;
+}
+
+static inline int __usi_pen_get_value(const struct usi_pen *pen,
+				      unsigned int code)
+{
+	if (!pen)
+		return -ENODEV;
+
+	return pen->values[code - MSC_PEN_FIRST];
+}
+
+static inline void __usi_pen_set_value(struct usi_pen *pen,
+				       unsigned int code, int value)
+{
+	if (!pen)
+		return;
+
+	pen->values[code - MSC_PEN_FIRST] = value;
+}
+
+static struct usi_pen *usi_find_pen(struct usi_drvdata *usi, int index)
+{
+	int i;
+
+	for (i = 0; i < usi->npens; i++) {
+		if (usi->pens[i].index == index)
+			return &usi->pens[i];
+	}
+
+	return NULL;
+}
+
+static bool _in_range(struct usi_drvdata *usi, unsigned long flags)
+{
+	return !!(test_bit(usi->in_range_bit, &flags));
+}
+
+static bool _is_touching(struct usi_drvdata *usi, unsigned long flags)
+{
+	return !!(test_bit(usi->tip_switch_bit, &flags));
+}
+
+static struct usi_pen *_usi_select_pen(struct usi_drvdata *usi, int index,
+				       bool in_range)
+{
+	struct usi_pen *pen, *found;
+	int i;
+
+	/* If not in range, forget current pen, and report to input-layer */
+	if (!in_range) {
+		usi->current_pen = NULL;
+		usi->update_pending = 0;
+		usi->query_pending = 0;
+		usi->query_running = 0;
+		input_event(usi->idev, EV_MSC, MSC_PEN_ID, 0);
+		input_sync(usi->idev);
+		return NULL;
+	}
+
+	pen = usi->current_pen;
+	if (pen && pen->index == index)
+		return pen;
+
+	found = usi_find_pen(usi, index);
+	if (!found) {
+		/* Pick next available pen */
+		for (i = 0; i < usi->npens; i++) {
+			pen = &usi->pens[i];
+
+			if (pen->index == -1) {
+				pen->index = index;
+
+				__usi_pen_set_value(pen, MSC_PEN_ID, index);
+				__usi_pen_set_value(pen, MSC_PEN_LINE_STYLE, 1);
+
+				hid_dbg(usi->hdev, "pen %u allocated\n",
+					index);
+
+				found = pen;
+
+				break;
+			}
+		}
+	}
+
+	if (!found)
+		return ERR_PTR(-ENOMEM);
+
+	usi->sync_point = jiffies;
+	usi->current_pen = found;
+
+	input_event(usi->idev, EV_MSC, MSC_PEN_ID, found->index);
+
+	if (_has_quirk(usi, USI_QUIRK_QUERY_DATA)) {
+		for (i = USI_PEN_COLOR; i <= USI_PEN_LINE_STYLE; i++)
+			set_bit(i, &usi->query_pending);
+		cancel_delayed_work_sync(&usi->work);
+		schedule_delayed_work(&usi->work, 0);
+	} else {
+		// HW reads the pen automatically
+		for (i = USI_PEN_COLOR; i <= USI_PEN_LINE_STYLE; i++)
+			set_bit(i, &usi->query_running);
+	}
+
+	return found;
+}
+
+/**
+ * __usi_input_event - Parse input event passed from input layer
+ * @usi: USI handle
+ * @code: input event code
+ * @value: input event value
+ *
+ * Parses input event passed from input layer. This is used to detect
+ * any writes to the USI driver from userspace and to program hardware
+ * to the new value. No return value.
+ */
+static void __usi_input_event(struct usi_drvdata *usi, unsigned int code,
+			      int value)
+{
+	struct usi_pen *pen = usi->current_pen;
+	int cached;
+	unsigned long flags;
+
+	hid_dbg(usi->hdev, "input-event: pen=%d, code=%x, value=%d, cached=%d, update-pending=%lx\n",
+		pen ? pen->index : -1, code, value,
+		pen ? __usi_pen_get_value(pen, code) : -1,
+		usi->update_pending);
+
+	if (code < MSC_PEN_COLOR || code > MSC_PEN_LINE_STYLE)
+		return;
+
+	if (!pen)
+		return;
+
+	if (test_bit(__msc_to_usi_id(code), &usi->update_pending))
+		return;
+
+	/*
+	 * Check if we get a value that is different than what is in cache,
+	 * in this case userspace has written a new value.
+	 */
+	cached = __usi_pen_get_value(pen, code);
+	if (cached == value)
+		return;
+
+	/*
+	 * New value received, kick off the work for actually re-programming HW
+	 */
+	spin_lock_irqsave(&usi->lock, flags);
+	set_bit(__msc_to_usi_id(code), &usi->update_pending);
+	spin_unlock_irqrestore(&usi->lock, flags);
+
+	__usi_pen_set_value(pen, code, value);
+
+	if (!delayed_work_pending(&usi->work)) {
+		long delay = usi->timeout - (jiffies - usi->sync_point);
+
+		if (delay < 0)
+			delay = 0;
+
+		schedule_delayed_work(&usi->work, delay);
+	}
+}
+
+static int _usi_input_event(struct input_dev *input, unsigned int type,
+			    unsigned int code, int value)
+{
+	struct hid_device *dev = input_get_drvdata(input);
+	struct usi_drvdata *usi = hid_get_drvdata(dev);
+
+	if (value < 0)
+		return 0;
+
+	if (type == EV_MSC) {
+		switch (code) {
+		case MSC_PEN_COLOR:
+		case MSC_PEN_LINE_WIDTH:
+		case MSC_PEN_LINE_STYLE:
+			__usi_input_event(usi, code, value);
+			break;
+		}
+	}
+
+	return 0;
+}
+
+static int _usi_input_open(struct input_dev *input)
+{
+	struct hid_device *hdev = input_get_drvdata(input);
+	struct usi_drvdata *usi = hid_get_drvdata(hdev);
+
+	/*
+	 * When a new input handle is opened, we must flush the current
+	 * pen attributes, otherwise client will not know current values.
+	 */
+	usi->need_flush = true;
+
+	return hid_hw_open(hdev);
+}
+
+/**
+ * usi_input_mapping - Parse input mappings passed from HID core
+ * @hdev: HID device
+ * @hi: HID input
+ * @field: HID field
+ * @usage: HID usage
+ * @bit: HID usage bitmap (output)
+ * @max: max usage (output)
+ *
+ * Parse input mappings and apply USI specific tweaks.
+ * Always returns 0 for success.
+ */
+static int usi_input_mapping(struct hid_device *hdev,
+			     struct hid_input *hi, struct hid_field *field,
+			     struct hid_usage *usage, unsigned long **bit,
+			     int *max)
+{
+	struct usi_drvdata *usi = hid_get_drvdata(hdev);
+	int usi_id;
+	u8 bit_idx;
+
+	hid_dbg(hdev, "input-field[%d] usage=%x[%d], phys=%x, log=%x, app=%x\n",
+		field->index, usage->hid, usage->usage_index, field->physical,
+		field->logical, field->application);
+
+	if ((usage->hid & HID_USAGE_PAGE) != HID_UP_DIGITIZER ||
+	    field->application != HID_DG_PEN ||
+	    field->physical != HID_DG_STYLUS)
+		return 0;
+
+	/*
+	 * Pen line style report appears to contain strange encoding
+	 * which confuses HID core. Fix this by forcing it to be
+	 * a variable.
+	 */
+	if (field->logical == HID_DG_PEN_LINE_STYLE)
+		field->flags |= HID_MAIN_ITEM_VARIABLE;
+
+	/*
+	 * Save the field for any USI inputs, needed for parsing
+	 * raw events.
+	 */
+	usi_id = __map_usage(usage, field, &bit_idx);
+	if (usi_id < 0)
+		return 0;
+
+	hid_dbg(usi->hdev, "usi-id %d mapped to offset %d\n",
+		usi_id, field->report_offset);
+
+	usi->inputs[usi_id] = field;
+
+	if (usi_id == USI_PEN_FLAGS) {
+		if (usage->hid == HID_DG_INRANGE)
+			usi->in_range_bit = bit_idx;
+		if (usage->hid == HID_DG_TIPSWITCH)
+			usi->tip_switch_bit = bit_idx;
+	}
+
+	return 0;
+}
+
+/**
+ * usi_raw_event - Parse raw USI events
+ * @hdev: HID device
+ * @report: report being parsed
+ * @data: raw data being parsed
+ * @size: size of the data
+ *
+ * Parses raw events passed directly from HID low level drivers. Used to
+ * select the current pen, and also updates the cached pen variables
+ * to the data if these differ from the ones coming from HW. This is done
+ * because HW reports incorrect values when coming to contact with screen.
+ * Returns 0 in success, negative error value with failure.
+ */
+static int usi_raw_event(struct hid_device *hdev,
+			 struct hid_report *report, u8 *data, int size)
+{
+	struct usi_drvdata *usi = hid_get_drvdata(hdev);
+	struct usi_pen *pen;
+	int i, index;
+	unsigned long flags;
+	u8 *ptr;
+	bool check_work = false;
+	struct hid_field *config;
+	bool touching;
+
+	if (report->application != HID_DG_PEN)
+		return 0;
+
+	hid_dbg(usi->hdev, "%s: qp:%lx, qr:%lx, up:%lx, data=%*ph\n",
+		__func__, usi->query_pending, usi->query_running,
+		usi->update_pending, size, data);
+
+	/* Get pen index */
+	index = data[usi->inputs[USI_PEN_ID]->report_offset / 8 + 1];
+	flags = data[usi->inputs[USI_PEN_FLAGS]->report_offset / 8 + 1];
+
+	pen = _usi_select_pen(usi, index, _in_range(usi, flags));
+	if (!pen)
+		return -ENOENT;
+
+	touching = _is_touching(usi, flags);
+
+	for (i = USI_PEN_COLOR; i < USI_NUM_ATTRS; i++) {
+		int cached = __usi_pen_get_value(pen, __usi_to_msc_id(i));
+		bool changed = false;
+
+		config = usi->inputs[i];
+		ptr = &data[config->report_offset / 8 + 1];
+
+		if (usi->saved_data[i] != *ptr)
+			changed = true;
+
+		hid_dbg(usi->hdev, "%s: i=%d, saved=%x, val=%x, cached=%x, changed=%d\n",
+			__func__, i,
+			usi->saved_data[i],
+			*ptr, cached, changed);
+
+		usi->saved_data[i] = *ptr;
+
+		/*
+		 * Limit logical values to min/max. Pen style has strange
+		 * mapping that goes outside ranges.
+		 */
+		if (*ptr < config->logical_minimum)
+			*ptr = config->logical_minimum;
+
+		if (*ptr > config->logical_maximum)
+			*ptr = config->logical_maximum;
+
+		if (!touching && !_has_quirk(usi, USI_QUIRK_QUERY_DATA)) {
+			usi->sync_point = jiffies;
+			spin_lock_irqsave(&usi->lock, flags);
+			set_bit(i, &usi->query_running);
+			spin_unlock_irqrestore(&usi->lock, flags);
+		}
+
+		if (test_bit(i, &usi->query_running)) {
+			if (changed ||
+			    time_after(jiffies, usi->sync_point +
+				       usi->timeout)) {
+				if (!test_bit(i, &usi->update_pending)) {
+					__usi_pen_set_value(pen,
+							    __usi_to_msc_id(i),
+							    *ptr);
+					cached = *ptr;
+					spin_lock_irqsave(&usi->lock, flags);
+					clear_bit(i, &usi->query_running);
+					spin_unlock_irqrestore(&usi->lock,
+							       flags);
+				}
+
+				check_work = true;
+			}
+		}
+
+		/* Ignore any unexpected data changes */
+		*ptr = cached;
+
+		if (usi->need_flush) {
+			input_event(usi->idev, EV_MSC, __usi_to_msc_id(i), -1);
+			input_event(usi->idev, EV_MSC, __usi_to_msc_id(i),
+				    cached);
+		}
+	}
+
+	usi->need_flush = false;
+
+	if (check_work) {
+		if (usi->update_pending || usi->query_pending) {
+			cancel_delayed_work_sync(&usi->work);
+			schedule_delayed_work(&usi->work, 0);
+		}
+	}
+
+	return 0;
+}
+
+static void _apply_quirks(struct usi_drvdata *usi, struct hid_device *hdev)
+{
+	if (hdev->vendor == I2C_VENDOR_ID_GOODIX &&
+	    hdev->product == 0xe00) {
+		set_bit(USI_QUIRK_STYLE_MAX_VAL, &usi->quirks);
+		set_bit(USI_QUIRK_QUERY_DATA, &usi->quirks);
+		usi->timeout = msecs_to_jiffies(75);
+	} else {
+		usi->timeout = msecs_to_jiffies(100);
+	}
+}
+
+static int usi_probe(struct hid_device *hdev, const struct hid_device_id *id)
+{
+	int ret;
+	struct usi_drvdata *usi;
+
+	hdev->quirks |= HID_QUIRK_INPUT_PER_APP;
+
+	usi = devm_kzalloc(&hdev->dev, sizeof(*usi), GFP_KERNEL);
+	if (!usi)
+		return -ENOMEM;
+
+	usi->hdev = hdev;
+
+	hid_set_drvdata(hdev, usi);
+
+	ret = hid_parse(hdev);
+	if (ret)
+		return ret;
+
+	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
+	if (ret)
+		goto err;
+
+	_apply_quirks(usi, hdev);
+
+	return 0;
+
+err:
+	hid_hw_stop(hdev);
+	return ret;
+}
+
+static void usi_remove(struct hid_device *hdev)
+{
+	hid_hw_stop(hdev);
+}
+
+/**
+ * usi_getset_feature - Read/write USI feature from LL drivers
+ * @usi: USI handle
+ * @code: field to access
+ * @value: value to write (ignored for reads)
+ * @write: true if write access
+ *
+ * Sends a HID HW request to low level drivers to read/write a USI
+ * feature value. Returns 0 on success, negative error value in failure.
+ */
+static int usi_getset_feature(struct usi_drvdata *usi,
+			      unsigned int code, int value, bool write)
+{
+	struct hid_device *hdev = usi->hdev;
+	struct hid_report *report;
+	struct usi_pen *pen = usi->current_pen;
+	size_t len;
+	u8 *buf;
+	int ret;
+
+	hid_dbg(usi->hdev, "%s: pen=%d, code=%x, value=%d, op=%s\n",
+		__func__, pen ? pen->index : 0, code, value,
+		write ? "wr" : "rd");
+
+	if (!pen)
+		return -ENODEV;
+
+	if (code >= USI_NUM_ATTRS)
+		return -ENODEV;
+
+	report = usi->features[code];
+	if (!report)
+		return -EOPNOTSUPP;
+
+	buf = hid_alloc_report_buf(report, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	len = hid_report_len(report);
+	memset(buf, 0, len);
+
+	if (_has_quirk(usi, USI_QUIRK_STYLE_MAX_VAL))
+		if (code == USI_PEN_LINE_STYLE)
+			if (value >= report->field[1]->logical_maximum)
+				value = 255;
+
+	buf[0] = report->id;
+	buf[1] = pen->index;
+	if (write)
+		buf[2] = value;
+
+	ret = hid_hw_raw_request(hdev, buf[0], buf, len, HID_FEATURE_REPORT,
+				 write ? HID_REQ_SET_REPORT :
+				 HID_REQ_GET_REPORT);
+
+	kfree(buf);
+
+	return ret < 0 ? ret : 0;
+}
+
+/**
+ * usi_feature_mapping - map any HID reported features for USI
+ * @hdev: HID device
+ * @field: HID field
+ * @usage: HID usage info
+ *
+ * Does any USI specific tweaks to the HID core reported features,
+ * and stores required fields for later use. No return value.
+ */
+static void usi_feature_mapping(struct hid_device *hdev,
+				struct hid_field *field,
+				struct hid_usage *usage)
+{
+	struct usi_drvdata *usi = hid_get_drvdata(hdev);
+	int usi_id;
+	u8 bit;
+
+	/* Re-map vendor specific usage fields to digitizer */
+	if ((usage->hid & HID_USAGE_PAGE) == HID_UP_MSVENDOR) {
+		usage->hid &= HID_USAGE;
+		usage->hid |= HID_UP_DIGITIZER;
+		field->logical &= HID_USAGE;
+		field->logical |= HID_UP_DIGITIZER;
+	}
+
+	if (usage->hid == HID_DG_TRANSDUCER_INDEX) {
+		if (!test_bit(USI_HAS_PENS, &usi->flags)) {
+			/*
+			 * Transducer index reports us the amount of pens
+			 * supported by HW, store this for later use.
+			 */
+			set_bit(USI_HAS_PENS, &usi->flags);
+			usi->min_pen = field->logical_minimum;
+			usi->max_pen = field->logical_maximum;
+		}
+	} else {
+		usi_id = __map_usage(usage, field, &bit);
+		/*
+		 * For any USI specific usage, store the report for later
+		 * use; needed for read/write access to the feature over
+		 * HID LL drivers
+		 */
+		if (usi_id >= 0)
+			usi->features[usi_id] = field->report;
+	}
+}
+
+/**
+ * usi_work - work function for the USI driver
+ * @work: handle to the work
+ *
+ * Parses any pending USI low level operations and executes one of them.
+ * If there are more pending, the work is re-scheduled to execute again
+ * later. USI driver must execute these from a work, as some of the
+ * control flows entering USI driver are run in interrupt context.
+ * No return value.
+ */
+static void usi_work(struct work_struct *work)
+{
+	struct usi_drvdata *usi =
+		container_of(work, struct usi_drvdata, work.work);
+	int code;
+	bool running = false;
+	unsigned long flags;
+
+	hid_dbg(usi->hdev, "work: update=%lx, query=%lx\n",
+		usi->update_pending, usi->query_pending);
+
+	if (!usi->current_pen)
+		return;
+
+	/* Update first pending value */
+	code = find_first_bit(&usi->update_pending, USI_NUM_ATTRS);
+	if (code < USI_NUM_ATTRS) {
+		usi_getset_feature(usi, code,
+				   __usi_pen_get_value(usi->current_pen,
+						       __usi_to_msc_id(code)),
+				   true);
+		spin_lock_irqsave(&usi->lock, flags);
+		clear_bit(code, &usi->update_pending);
+		clear_bit(code, &usi->query_running);
+		spin_unlock_irqrestore(&usi->lock, flags);
+		running = true;
+	}
+
+	/*
+	 * Query first pending value. Only execute if we didn't have any
+	 * updates pending.
+	 */
+	code = find_first_bit(&usi->query_pending, USI_NUM_ATTRS);
+	if (!running && code < USI_NUM_ATTRS) {
+		usi_getset_feature(usi, code, 0, false);
+		spin_lock_irqsave(&usi->lock, flags);
+		clear_bit(code, &usi->query_pending);
+		set_bit(code, &usi->query_running);
+		spin_unlock_irqrestore(&usi->lock, flags);
+		running = true;
+	}
+
+	if (running) {
+		usi->sync_point = jiffies;
+		schedule_delayed_work(&usi->work, usi->timeout);
+	}
+}
+
+static int usi_allocate_pens(struct usi_drvdata *usi,
+			     struct hid_input *hidinput)
+{
+	size_t max_pens = usi->max_pen - usi->min_pen + 1;
+	int i;
+
+	INIT_DELAYED_WORK(&usi->work, usi_work);
+	spin_lock_init(&usi->lock);
+	usi->idev = hidinput->input;
+	usi->npens = min_t(size_t, max_pens, USI_MAX_PENS);
+	usi->pens = devm_kcalloc(&usi->hdev->dev, usi->npens,
+				 sizeof(*usi->pens), GFP_KERNEL);
+	if (!usi->pens)
+		return -ENOMEM;
+
+	for (i = 0; i < usi->npens; i++) {
+		__usi_pen_set_value(&usi->pens[i], MSC_PEN_ID, -1);
+		usi->pens[i].index = -1;
+	}
+
+	usi->idev->event = _usi_input_event;
+	usi->idev->open = _usi_input_open;
+	hid_dbg(usi->hdev, "allocated %zd pens\n", usi->npens);
+
+	return 0;
+}
+
+static int usi_input_configured(struct hid_device *hdev,
+				struct hid_input *hidinput)
+{
+	struct usi_drvdata *usi = hid_get_drvdata(hdev);
+
+	if (test_bit(USI_HAS_PENS, &usi->flags) &&
+	    !test_bit(USI_PENS_CONFIGURED, &usi->flags) &&
+	    hidinput->application == HID_DG_PEN) {
+		set_bit(USI_PENS_CONFIGURED, &usi->flags);
+		return usi_allocate_pens(usi, hidinput);
+	}
+
+	return 0;
+}
+
+static const struct hid_device_id usi_devices[] = {
+	{ HID_DEVICE(HID_BUS_ANY, HID_GROUP_USI, HID_ANY_ID, HID_ANY_ID) },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(hid, usi_devices);
+
+static struct hid_driver usi_driver = {
+	.name = "usi",
+	.id_table = usi_devices,
+	.input_configured = usi_input_configured,
+	.input_mapping = usi_input_mapping,
+	.probe = usi_probe,
+	.remove = usi_remove,
+	.raw_event = usi_raw_event,
+	.feature_mapping = usi_feature_mapping,
+};
+module_hid_driver(usi_driver);
+
+MODULE_LICENSE("GPL");
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 000fd3cf06ce..05822873b39b 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -257,6 +257,13 @@ struct hid_item {
 #define HID_DG_PEN_COLOR	0x000d005c
 #define HID_DG_PEN_LINE_WIDTH	0x000d005e
 #define HID_DG_PEN_LINE_STYLE	0x000d0070
+#define HID_DG_PEN_LINE_STYLE_IS_LOCKED		0x000d0071
+#define HID_DG_PEN_LINE_STYLE_INK		0x000d0072
+#define HID_DG_PEN_LINE_STYLE_PENCIL		0x000d0073
+#define HID_DG_PEN_LINE_STYLE_HIGHLIGHTER	0x000d0074
+#define HID_DG_PEN_LINE_STYLE_CHISEL_MARKER	0x000d0075
+#define HID_DG_PEN_LINE_STYLE_BRUSH		0x000d0076
+#define HID_DG_PEN_LINE_STYLE_NO_PREFERENCE	0x000d0077
 
 #define HID_CP_CONSUMERCONTROL	0x000c0001
 #define HID_CP_NUMERICKEYPAD	0x000c0002
-- 
2.25.1

[RFC 7/8] HID: USI: add new input event codes for the pen-set value events

[Tero Kristo]

Previously, pen values were set using the same input event codes as were
used for reporting the new values from the core. This lead into some
confusion with both the driver layer and the userspace. To avoid these
issues, allocate new event codes purely for setting the pen
color/width/style.

This patch is a proposal only, and can be omitted if not acceptable, or
if the ioctl solution that is introduced in patch #8 is preferred.

Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
---
 drivers/hid/hid-usi.c                  | 59 ++++++++++++++++----------
 include/linux/mod_devicetable.h        |  2 +-
 include/uapi/linux/input-event-codes.h |  5 ++-
 3 files changed, 42 insertions(+), 24 deletions(-)

diff --git a/drivers/hid/hid-usi.c b/drivers/hid/hid-usi.c
index 4674e6993f6e..a465c140d25a 100644
--- a/drivers/hid/hid-usi.c
+++ b/drivers/hid/hid-usi.c
@@ -37,7 +37,7 @@ enum {
 
 #define USI_MAX_PENS	10
 #define MSC_PEN_FIRST	MSC_PEN_ID
-#define MSC_PEN_LAST	MSC_PEN_LINE_STYLE
+#define MSC_PEN_LAST	MSC_PEN_SET_LINE_STYLE
 
 struct usi_pen {
 	int index;
@@ -63,6 +63,7 @@ struct usi_drvdata {
 	unsigned long query_pending;
 	unsigned long update_pending;
 	unsigned long query_running;
+	unsigned long update_running;
 	bool need_flush;
 	struct delayed_work work;
 	u8 saved_data[USI_NUM_ATTRS];
@@ -113,6 +114,9 @@ static inline int __usi_to_msc_id(int id)
 
 static inline int __msc_to_usi_id(unsigned int code)
 {
+	if (code >= MSC_PEN_SET_COLOR && code <= MSC_PEN_SET_LINE_STYLE)
+		return code - MSC_PEN_SET_COLOR + USI_PEN_COLOR;
+
 	if (code < MSC_PEN_ID || code > MSC_PEN_LINE_STYLE)
 		return -EINVAL;
 
@@ -169,6 +173,7 @@ static struct usi_pen *_usi_select_pen(struct usi_drvdata *usi, int index,
 	if (!in_range) {
 		usi->current_pen = NULL;
 		usi->update_pending = 0;
+		usi->update_running = 0;
 		usi->query_pending = 0;
 		usi->query_running = 0;
 		input_event(usi->idev, EV_MSC, MSC_PEN_ID, 0);
@@ -238,7 +243,6 @@ static void __usi_input_event(struct usi_drvdata *usi, unsigned int code,
 			      int value)
 {
 	struct usi_pen *pen = usi->current_pen;
-	int cached;
 	unsigned long flags;
 
 	hid_dbg(usi->hdev, "input-event: pen=%d, code=%x, value=%d, cached=%d, update-pending=%lx\n",
@@ -246,7 +250,7 @@ static void __usi_input_event(struct usi_drvdata *usi, unsigned int code,
 		pen ? __usi_pen_get_value(pen, code) : -1,
 		usi->update_pending);
 
-	if (code < MSC_PEN_COLOR || code > MSC_PEN_LINE_STYLE)
+	if (code < MSC_PEN_SET_COLOR || code > MSC_PEN_SET_LINE_STYLE)
 		return;
 
 	if (!pen)
@@ -255,14 +259,6 @@ static void __usi_input_event(struct usi_drvdata *usi, unsigned int code,
 	if (test_bit(__msc_to_usi_id(code), &usi->update_pending))
 		return;
 
-	/*
-	 * Check if we get a value that is different than what is in cache,
-	 * in this case userspace has written a new value.
-	 */
-	cached = __usi_pen_get_value(pen, code);
-	if (cached == value)
-		return;
-
 	/*
 	 * New value received, kick off the work for actually re-programming HW
 	 */
@@ -291,14 +287,9 @@ static int _usi_input_event(struct input_dev *input, unsigned int type,
 	if (value < 0)
 		return 0;
 
-	if (type == EV_MSC) {
-		switch (code) {
-		case MSC_PEN_COLOR:
-		case MSC_PEN_LINE_WIDTH:
-		case MSC_PEN_LINE_STYLE:
-			__usi_input_event(usi, code, value);
-			break;
-		}
+	if (type == EV_MSC && code >= MSC_PEN_SET_COLOR &&
+	    code <= MSC_PEN_SET_LINE_STYLE) {
+		__usi_input_event(usi, code, value);
 	}
 
 	return 0;
@@ -407,9 +398,9 @@ static int usi_raw_event(struct hid_device *hdev,
 	if (report->application != HID_DG_PEN)
 		return 0;
 
-	hid_dbg(usi->hdev, "%s: qp:%lx, qr:%lx, up:%lx, data=%*ph\n",
+	hid_dbg(usi->hdev, "%s: qp:%lx, qr:%lx, up:%lx, ur:%lx, data=%*ph\n",
 		__func__, usi->query_pending, usi->query_running,
-		usi->update_pending, size, data);
+		usi->update_pending, usi->update_running, size, data);
 
 	/* Get pen index */
 	index = data[usi->inputs[USI_PEN_ID]->report_offset / 8 + 1];
@@ -455,6 +446,23 @@ static int usi_raw_event(struct hid_device *hdev,
 			spin_unlock_irqrestore(&usi->lock, flags);
 		}
 
+		if (test_bit(i, &usi->update_running)) {
+			int new = __usi_pen_get_value(pen, __usi_to_msc_id(i) +
+						      MSC_PEN_SET_COLOR -
+						      MSC_PEN_COLOR);
+			if ((changed && *ptr == new) ||
+			    time_after(jiffies, usi->sync_point +
+				       usi->timeout)) {
+				__usi_pen_set_value(pen,
+						    __usi_to_msc_id(i), new);
+				cached = new;
+				spin_lock_irqsave(&usi->lock, flags);
+				clear_bit(i, &usi->update_running);
+				spin_unlock_irqrestore(&usi->lock, flags);
+				check_work = true;
+			}
+		}
+
 		if (test_bit(i, &usi->query_running)) {
 			if (changed ||
 			    time_after(jiffies, usi->sync_point +
@@ -681,11 +689,14 @@ static void usi_work(struct work_struct *work)
 	if (code < USI_NUM_ATTRS) {
 		usi_getset_feature(usi, code,
 				   __usi_pen_get_value(usi->current_pen,
-						       __usi_to_msc_id(code)),
+						       __usi_to_msc_id(code) +
+						       MSC_PEN_SET_COLOR -
+						       MSC_PEN_COLOR),
 				   true);
 		spin_lock_irqsave(&usi->lock, flags);
 		clear_bit(code, &usi->update_pending);
 		clear_bit(code, &usi->query_running);
+		set_bit(code, &usi->update_running);
 		spin_unlock_irqrestore(&usi->lock, flags);
 		running = true;
 	}
@@ -734,6 +745,10 @@ static int usi_allocate_pens(struct usi_drvdata *usi,
 	usi->idev->open = _usi_input_open;
 	hid_dbg(usi->hdev, "allocated %zd pens\n", usi->npens);
 
+	input_set_capability(usi->idev, EV_MSC, MSC_PEN_SET_COLOR);
+	input_set_capability(usi->idev, EV_MSC, MSC_PEN_SET_LINE_WIDTH);
+	input_set_capability(usi->idev, EV_MSC, MSC_PEN_SET_LINE_STYLE);
+
 	return 0;
 }
 
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 4ff40be7676b..3c719047da81 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -322,7 +322,7 @@ struct pcmcia_device_id {
 #define INPUT_DEVICE_ID_KEY_MAX		0x2ff
 #define INPUT_DEVICE_ID_REL_MAX		0x0f
 #define INPUT_DEVICE_ID_ABS_MAX		0x3f
-#define INPUT_DEVICE_ID_MSC_MAX		0x09
+#define INPUT_DEVICE_ID_MSC_MAX		0x0c
 #define INPUT_DEVICE_ID_LED_MAX		0x0f
 #define INPUT_DEVICE_ID_SND_MAX		0x07
 #define INPUT_DEVICE_ID_FF_MAX		0x7f
diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
index 98295f71941a..66ac07529eac 100644
--- a/include/uapi/linux/input-event-codes.h
+++ b/include/uapi/linux/input-event-codes.h
@@ -912,8 +912,11 @@
 #define MSC_PEN_COLOR			0x07
 #define MSC_PEN_LINE_WIDTH		0x08
 #define MSC_PEN_LINE_STYLE		0x09
+#define MSC_PEN_SET_COLOR		0x0a
+#define MSC_PEN_SET_LINE_WIDTH		0x0b
+#define MSC_PEN_SET_LINE_STYLE		0x0c
 /* TODO: Add USI diagnostic & battery events too */
-#define MSC_MAX				0x09
+#define MSC_MAX				0x0c
 #define MSC_CNT				(MSC_MAX + 1)
 
 /*
-- 
2.25.1

[RFC 8/8] HID: USI: add support for ioctls

[Tero Kristo]

Add a new device node /dev/usi, which can be used to apply ioctls for
USI to modify pen parameters.

Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
---
 .../userspace-api/ioctl/ioctl-number.rst      |   1 +
 drivers/hid/hid-usi.c                         | 134 ++++++++++++++++--
 include/linux/hid-usi.h                       |  22 +++
 3 files changed, 149 insertions(+), 8 deletions(-)
 create mode 100644 include/linux/hid-usi.h

diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
index 6655d929a351..bfaba2748592 100644
--- a/Documentation/userspace-api/ioctl/ioctl-number.rst
+++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
@@ -154,6 +154,7 @@ Code  Seq#    Include File                                           Comments
 'H'   C0-DF  net/bluetooth/cmtp/cmtp.h                               conflict!
 'H'   C0-DF  net/bluetooth/bnep/bnep.h                               conflict!
 'H'   F1     linux/hid-roccat.h                                      <mailto:erazor_de@users.sourceforge.net>
+'H'   F2-F3  linux/hid-usi.h
 'H'   F8-FA  sound/firewire.h
 'I'   all    linux/isdn.h                                            conflict!
 'I'   00-0F  drivers/isdn/divert/isdn_divert.h                       conflict!
diff --git a/drivers/hid/hid-usi.c b/drivers/hid/hid-usi.c
index a465c140d25a..f7e739d9f554 100644
--- a/drivers/hid/hid-usi.c
+++ b/drivers/hid/hid-usi.c
@@ -10,14 +10,18 @@
 
 #include <linux/module.h>
 #include <linux/sysfs.h>
+#include <linux/cdev.h>
 #include <linux/device.h>
 #include <linux/hid.h>
+#include <linux/hid-usi.h>
 #include <linux/input.h>
 #include <linux/leds.h>
 #include <linux/workqueue.h>
 
 #include "hid-ids.h"
 
+#define USI_MAX_DEVICES		1
+
 #define USI_HAS_PENS		0
 #define USI_PENS_CONFIGURED	1
 
@@ -67,6 +71,12 @@ struct usi_drvdata {
 	bool need_flush;
 	struct delayed_work work;
 	u8 saved_data[USI_NUM_ATTRS];
+	bool user_pending;
+	struct completion user_work_done;
+	struct cdev cdev;
+	struct device *dev;
+	dev_t dev_id;
+	struct class *class;
 	spinlock_t lock; /* private data lock */
 };
 
@@ -237,9 +247,9 @@ static struct usi_pen *_usi_select_pen(struct usi_drvdata *usi, int index,
  *
  * Parses input event passed from input layer. This is used to detect
  * any writes to the USI driver from userspace and to program hardware
- * to the new value. No return value.
+ * to the new value. Returns 0 on success, or negative error value.
  */
-static void __usi_input_event(struct usi_drvdata *usi, unsigned int code,
+static int __usi_input_event(struct usi_drvdata *usi, unsigned int code,
 			      int value)
 {
 	struct usi_pen *pen = usi->current_pen;
@@ -251,13 +261,13 @@ static void __usi_input_event(struct usi_drvdata *usi, unsigned int code,
 		usi->update_pending);
 
 	if (code < MSC_PEN_SET_COLOR || code > MSC_PEN_SET_LINE_STYLE)
-		return;
+		return -EINVAL;
 
 	if (!pen)
-		return;
+		return -ENODEV;
 
 	if (test_bit(__msc_to_usi_id(code), &usi->update_pending))
-		return;
+		return -EBUSY;
 
 	/*
 	 * New value received, kick off the work for actually re-programming HW
@@ -276,6 +286,8 @@ static void __usi_input_event(struct usi_drvdata *usi, unsigned int code,
 
 		schedule_delayed_work(&usi->work, delay);
 	}
+
+	return 0;
 }
 
 static int _usi_input_event(struct input_dev *input, unsigned int type,
@@ -459,6 +471,11 @@ static int usi_raw_event(struct hid_device *hdev,
 				spin_lock_irqsave(&usi->lock, flags);
 				clear_bit(i, &usi->update_running);
 				spin_unlock_irqrestore(&usi->lock, flags);
+				if (usi->user_pending) {
+					complete(&usi->user_work_done);
+					usi->user_pending = false;
+				}
+
 				check_work = true;
 			}
 		}
@@ -516,6 +533,74 @@ static void _apply_quirks(struct usi_drvdata *usi, struct hid_device *hdev)
 	}
 }
 
+static long usi_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	void __user *p = (void __user *)arg;
+	struct usi_pen_info info;
+	struct usi_pen *pen;
+	struct usi_drvdata *usi = file->private_data;
+	int ret;
+
+	if (cmd != USIIOCSET && cmd != USIIOCGET)
+		return -EINVAL;
+
+	if (copy_from_user(&info, p, sizeof(info)))
+		return -EFAULT;
+
+	pen = usi_find_pen(usi, info.index);
+	if (!pen)
+		return -ENODEV;
+
+	switch (cmd) {
+	case USIIOCSET:
+		if (info.code < MSC_PEN_SET_COLOR ||
+		    info.code > MSC_PEN_SET_LINE_STYLE)
+			return -EINVAL;
+
+		init_completion(&usi->user_work_done);
+
+		ret = __usi_input_event(usi, info.code, info.value);
+		if (ret)
+			return ret;
+
+		usi->user_pending = true;
+		ret = wait_for_completion_timeout(&usi->user_work_done,
+						  usi->timeout * 2);
+		if (!ret) {
+			usi->user_pending = false;
+			return -ETIMEDOUT;
+		}
+		return 0;
+
+	case USIIOCGET:
+		ret = __usi_pen_get_value(pen, info.code);
+		if (ret < 0)
+			return ret;
+
+		info.value = ret;
+
+		if (copy_to_user(p, &info, sizeof(info)))
+			return -EFAULT;
+
+		return sizeof(info);
+	}
+
+	return -EINVAL;
+}
+
+static int usi_open(struct inode *inode, struct file *file)
+{
+	struct usi_drvdata *usi = container_of(inode->i_cdev,
+					       struct usi_drvdata, cdev);
+	file->private_data = usi;
+	return 0;
+}
+
+static const struct file_operations usi_ops = {
+	.unlocked_ioctl = usi_ioctl,
+	.open = usi_open,
+};
+
 static int usi_probe(struct hid_device *hdev, const struct hid_device_id *id)
 {
 	int ret;
@@ -529,28 +614,61 @@ static int usi_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 	usi->hdev = hdev;
 
+	ret = alloc_chrdev_region(&usi->dev_id, 0, USI_MAX_DEVICES, "usi");
+	if (ret < 0)
+		return ret;
+
+	cdev_init(&usi->cdev, &usi_ops);
+	ret = cdev_add(&usi->cdev, usi->dev_id, USI_MAX_DEVICES);
+	if (ret < 0)
+		goto err;
+
+	usi->class = class_create(THIS_MODULE, "usi");
+	if (IS_ERR(usi->class)) {
+		ret = PTR_ERR(usi->class);
+		goto err;
+	}
+
+	usi->dev = device_create(usi->class, &hdev->dev, usi->dev_id, NULL,
+				 "usi");
+	if (IS_ERR(usi->dev)) {
+		ret = PTR_ERR(usi->dev);
+		goto err_class;
+	}
+
 	hid_set_drvdata(hdev, usi);
 
 	ret = hid_parse(hdev);
 	if (ret)
-		return ret;
+		goto err_dev;
 
 	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
 	if (ret)
-		goto err;
+		goto err_dev;
 
 	_apply_quirks(usi, hdev);
 
 	return 0;
 
+err_dev:
+	device_destroy(usi->class, usi->dev_id);
+
+err_class:
+	class_destroy(usi->class);
+
 err:
-	hid_hw_stop(hdev);
+	unregister_chrdev_region(usi->dev_id, USI_MAX_DEVICES);
 	return ret;
 }
 
 static void usi_remove(struct hid_device *hdev)
 {
+	struct usi_drvdata *usi = hid_get_drvdata(hdev);
+
 	hid_hw_stop(hdev);
+	device_destroy(usi->class, usi->dev_id);
+	class_destroy(usi->class);
+	unregister_chrdev_region(usi->dev_id, USI_MAX_DEVICES);
 }
 
 /**
diff --git a/include/linux/hid-usi.h b/include/linux/hid-usi.h
new file mode 100644
index 000000000000..1840285c7bc1
--- /dev/null
+++ b/include/linux/hid-usi.h
@@ -0,0 +1,22 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2021, Intel Corporation
+ * Authors: Tero Kristo <tero.kristo@linux.intel.com>
+ */
+
+#ifndef __HID_USI_H
+#define __HID_USI_H
+
+#include <linux/hid.h>
+#include <linux/types.h>
+
+struct usi_pen_info {
+	__s32 index;
+	__u32 code;
+	__s32 value;
+};
+
+#define USIIOCGET _IOR('H', 0xf2, struct usi_pen_info)
+#define USIIOCSET _IOW('H', 0xf3, struct usi_pen_info)
+
+#endif
-- 
2.25.1

Re: [RFC 6/8] HID: usi: Add driver for Universal Stylus Interface (USI)

[Randy Dunlap]

On 11/4/21 6:36 AM, Tero Kristo wrote:
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 3c33bf572d6d..c235ecb8f037 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -1222,6 +1222,11 @@ config HID_MCP2221
>   	To compile this driver as a module, choose M here: the module
>   	will be called hid-mcp2221.ko.
>   
> +config HID_USI
> +	tristate "USI (Universal Stylus Interface) support"
> +	help
> +	Provides USI support over I2C HID interface.

Indent help text "Provides ..." with 2 additional spaces, please,
per coding-style.rst.

-- 
~Randy

Re: [RFC 6/8] HID: usi: Add driver for Universal Stylus Interface (USI)

[Tero Kristo]

On 04/11/2021 17:03, Randy Dunlap wrote:
> On 11/4/21 6:36 AM, Tero Kristo wrote:
>> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
>> index 3c33bf572d6d..c235ecb8f037 100644
>> --- a/drivers/hid/Kconfig
>> +++ b/drivers/hid/Kconfig
>> @@ -1222,6 +1222,11 @@ config HID_MCP2221
>>       To compile this driver as a module, choose M here: the module
>>       will be called hid-mcp2221.ko.
>>   +config HID_USI
>> +    tristate "USI (Universal Stylus Interface) support"
>> +    help
>> +    Provides USI support over I2C HID interface.
>
> Indent help text "Provides ..." with 2 additional spaces, please,
> per coding-style.rst.
>
Sorry yeah, this slipped through. Seems most of the drivers/hid/Kconfig 
is with wrong indentation, copied layout from there. I also need to add 
a bit more beef to this help text while updating it, and remove the 
mention of i2c only.

-Tero

Re: [RFC 0/8] HID: add support for USI style pens

[Benjamin Tissoires]

Hi Tero,

[just a quick note, I am supposed to be on holiday this week]

On Thu, Nov 4, 2021 at 2:38 PM Tero Kristo <tero.kristo@linux.intel.com> wrote:
>
> Hi,
>
> This series is an RFC for USI (Universal Stylus Interface) style pen
> support. This is based on documentation from USB org describing the HID
> usage tables for digitizers (page 0x0D) and experimentation with actual
> USI capable controllers.
>
> This series introduces the USI support with a new HID driver, which
> applies the controller specific quirks. The most problematic part of the
> USI support is handling of the pen parameters (color, line width, line
> style), which are not immediately available from the controller from pen
> down event, but must be cached and queried separately from the controller.
> In addition to that, when a get-feature report is sent to the
> controller, there is a delay before the proper value is reported out; it
> is not part of the feature report coming back immediately.
> Most of the code in the driver is to handle this (otherwise we could
> just use hid-generic.)
>
> This also boils down to the reason why this series is an RFC, I would like
> to receive some feedback which option to pick for programming of the new
> values for the programmable pen parameters; whether to parse the input
> events so userspace can directly write the new values to the input event
> file handle, or whether to use IOCTL. Patches #7 / #8 are sort of optional
> choices towards this, but are there to show that both approaches can be
> done. Direct write to evdev causes some confusion on the driver level
> though, thus patch #7 is there to avoid some of that introducing new
> input events for writing the parameters. IOCTL might be the cleanest
> approach and I am slightly leaning towards that myself (see patch #8,
> this would need to be squashed and cleaned up a bit though but would
> effectively get rid of some code from patch #6 and completely rid patch #7.)

This series unfortunately raised quite a few red flags for me, and I
am glad this is just an RFC.
Let me enumerate them first and discuss a little bit more about those:
 1. USI is supposed to be generic, so why is there a new driver for it
instead of being handled by hid-input.c?
 2. new MSC_EVENTS are created without Dmitry or Peter being CC-ed
 3. new ioctls???
 4. direct write to evdev to write parameters
 5. patch 1/8 doesn't compile without 5/8
 6. no tests :)

1. new driver
After quickly reading the RFC, I think the main issue there is that we
are now having a transducer index which is incompatible with the way
input and evedev works nowadays. Yay, we have a new hid-multitouch for
pen :(

Wacom has been dealing with that situation for years by tweaking the
protocol and by just emitting a different serial number (roughly). I
think the safest approach would be to keep the existing protocol
running so that our user space can handle it properly.

I'd need to read the rest of the code more carefully, but if we could
have a basic generic handling (without the fancy features like
changing the pen style/color) I'd be happier.

2. MSC_* events
there is an issue with those: they are not cached like the ABS_* ones.
Meaning that each report will wake up userspace for something which
basically doesn't change.
I know ABS_* is saturated, but I'd like to have reviews from others on
what could be done here instead of just using MSC_* as a new ABS_*

3. ioctls
this is problematic to me. Any new kernel ABI is problematic to me,
and I'd much rather not add any new ones.

My new set of mind is because of the recent work I have been
conducting regarding eBPF.
Basically I managed to have eBPF programs handling the device
configuration and event processing in a local branch.
I should be able to push a WIP next week, but basically this should
allow me to not have to deal with new kernel APIs besides the generic
eBPF one.
We can imagine a generic hid-input.c processing for those tablets, and
have a new userspace component that loads an eBPF program with its own
userspace API which is capable of the fancy features.

For instance, my current playground is setting the haptic feedback of
the Surface Dial depending on the resolution I set on it.

Furthermore, ioctls on a new cdev means that the classic userspace
libraries will not have access to it without some heavy tuning in the
systemd space (libinput only has read/write access to
/dev/input/event*).

4. direct write to evdev

We enabled that once for LEDs, and it's a pain to maintain. Maybe we
can make a use case for it but given that you don't seem very
enthusiastic about it too, I wonder if this is not a dead end.

5. patch ordering doesn't compile
I guess this is just a rebase hiccup. Not an issue for an RFC

6. tests
For these kinds of new classes of devices, I'd like to have tests in
the https://gitlab.freedesktop.org/libevdev/hid-tools repository.
There is already an initial MR for tablet support (!115 in this
project), and we should extend it with more tests.

I'd happily help with those tests if you could share the report
descriptors and some device dumps made with the hid-recorder tool from
that repository.

>
> The driver has been tested with chromebooks that contain either Goodix
> or Elan manufactured USI capable touchscreen controllers in them.
>
> Any feedback appreciated!

I'll try to have a deeper look next week (though it seems a few bits
stacked up during my week off, sigh).

Cheers,
Benjamin

>
> -Tero
>
>

Re: [RFC 6/8] HID: usi: Add driver for Universal Stylus Interface (USI)

[Randy Dunlap]

On 11/4/21 11:33 PM, Tero Kristo wrote:
> 
> On 04/11/2021 17:03, Randy Dunlap wrote:
>> On 11/4/21 6:36 AM, Tero Kristo wrote:
>>> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
>>> index 3c33bf572d6d..c235ecb8f037 100644
>>> --- a/drivers/hid/Kconfig
>>> +++ b/drivers/hid/Kconfig
>>> @@ -1222,6 +1222,11 @@ config HID_MCP2221
>>>       To compile this driver as a module, choose M here: the module
>>>       will be called hid-mcp2221.ko.
>>>   +config HID_USI
>>> +    tristate "USI (Universal Stylus Interface) support"
>>> +    help
>>> +    Provides USI support over I2C HID interface.
>>
>> Indent help text "Provides ..." with 2 additional spaces, please,
>> per coding-style.rst.
>>
> Sorry yeah, this slipped through. Seems most of the drivers/hid/Kconfig is with wrong indentation, copied layout from there. I also need to add a bit more beef to this help text while updating it, and remove the mention of i2c only.

Thanks for adding more beef to it.

And yes, I have noticed that drivers/hid/Kconfig is a bit messy. :\

-- 
~Randy

Re: [RFC 0/8] HID: add support for USI style pens

[Tero Kristo]

Hi Benjamin,

Thanks for your feedback! Couple of quick replies below.

On 05/11/2021 20:22, Benjamin Tissoires wrote:
> Hi Tero,
>
> [just a quick note, I am supposed to be on holiday this week]
>
> On Thu, Nov 4, 2021 at 2:38 PM Tero Kristo <tero.kristo@linux.intel.com> wrote:
>> Hi,
>>
>> This series is an RFC for USI (Universal Stylus Interface) style pen
>> support. This is based on documentation from USB org describing the HID
>> usage tables for digitizers (page 0x0D) and experimentation with actual
>> USI capable controllers.
>>
>> This series introduces the USI support with a new HID driver, which
>> applies the controller specific quirks. The most problematic part of the
>> USI support is handling of the pen parameters (color, line width, line
>> style), which are not immediately available from the controller from pen
>> down event, but must be cached and queried separately from the controller.
>> In addition to that, when a get-feature report is sent to the
>> controller, there is a delay before the proper value is reported out; it
>> is not part of the feature report coming back immediately.
>> Most of the code in the driver is to handle this (otherwise we could
>> just use hid-generic.)
>>
>> This also boils down to the reason why this series is an RFC, I would like
>> to receive some feedback which option to pick for programming of the new
>> values for the programmable pen parameters; whether to parse the input
>> events so userspace can directly write the new values to the input event
>> file handle, or whether to use IOCTL. Patches #7 / #8 are sort of optional
>> choices towards this, but are there to show that both approaches can be
>> done. Direct write to evdev causes some confusion on the driver level
>> though, thus patch #7 is there to avoid some of that introducing new
>> input events for writing the parameters. IOCTL might be the cleanest
>> approach and I am slightly leaning towards that myself (see patch #8,
>> this would need to be squashed and cleaned up a bit though but would
>> effectively get rid of some code from patch #6 and completely rid patch #7.)
> This series unfortunately raised quite a few red flags for me, and I
> am glad this is just an RFC.
> Let me enumerate them first and discuss a little bit more about those:
>   1. USI is supposed to be generic, so why is there a new driver for it
> instead of being handled by hid-input.c?
>   2. new MSC_EVENTS are created without Dmitry or Peter being CC-ed
>   3. new ioctls???
>   4. direct write to evdev to write parameters
>   5. patch 1/8 doesn't compile without 5/8
>   6. no tests :)
>
> 1. new driver
> After quickly reading the RFC, I think the main issue there is that we
> are now having a transducer index which is incompatible with the way
> input and evedev works nowadays. Yay, we have a new hid-multitouch for
> pen :(
>
> Wacom has been dealing with that situation for years by tweaking the
> protocol and by just emitting a different serial number (roughly). I
> think the safest approach would be to keep the existing protocol
> running so that our user space can handle it properly.
>
> I'd need to read the rest of the code more carefully, but if we could
> have a basic generic handling (without the fancy features like
> changing the pen style/color) I'd be happier.

The USI pen support is partially compatible with existing input 
framework, e.g. co-ordinates + touch events work out of box with 
no-modification to the kernel whatsoever, just by using hid-generic 
driver. What is missing completely is the new features; 
color/width/style. It would be possible to move all these to the 
hid-input driver obviously, I don't think there is anything to prevent 
that. And, I could split up the series so that in the initial patches we 
would only support reporting current color/width/style parameters, and 
add the programmability as a subsequent patch if that would be better. 
It would also be possible to move parts of the code to the input 
subsystem from HID (some initial patches from our side were done this 
way, but I don't think this is necessary.)

>
> 2. MSC_* events
> there is an issue with those: they are not cached like the ABS_* ones.
> Meaning that each report will wake up userspace for something which
> basically doesn't change.
> I know ABS_* is saturated, but I'd like to have reviews from others on
> what could be done here instead of just using MSC_* as a new ABS_*
In my tests, it seems like MSC_* from the USI pens with these patches 
only get reported to userspace if something changes. Otherwise they do 
not get through at all. I have a small quirk in the driver to address 
this for a case where a new userspace handle is opened while pen is 
already in use; it does not get the params reported at all unless I 
flush the current entries out (by reporting a bogus value followed 
immediately by the real value.) Anyways, ABS events would be fine for 
the parameters also I believe if this is desirable.
>
> 3. ioctls
> this is problematic to me. Any new kernel ABI is problematic to me,
> and I'd much rather not add any new ones.
I agree I am torn between the ioctl / direct evdev write. Both have 
their bad sides to them, thus I provided sort of support for both. :)
>
> My new set of mind is because of the recent work I have been
> conducting regarding eBPF.
> Basically I managed to have eBPF programs handling the device
> configuration and event processing in a local branch.
> I should be able to push a WIP next week, but basically this should
> allow me to not have to deal with new kernel APIs besides the generic
> eBPF one.
> We can imagine a generic hid-input.c processing for those tablets, and
> have a new userspace component that loads an eBPF program with its own
> userspace API which is capable of the fancy features.
>
> For instance, my current playground is setting the haptic feedback of
> the Surface Dial depending on the resolution I set on it.
>
> Furthermore, ioctls on a new cdev means that the classic userspace
> libraries will not have access to it without some heavy tuning in the
> systemd space (libinput only has read/write access to
> /dev/input/event*).

So, you are saying it would be possible to use this to support the USI 
pen parameters also somehow? I can take a look at this once available.

>
> 4. direct write to evdev
>
> We enabled that once for LEDs, and it's a pain to maintain. Maybe we
> can make a use case for it but given that you don't seem very
> enthusiastic about it too, I wonder if this is not a dead end.
Well, we need to be able to program the pen parameters somehow from 
userspace, I am open to any suggestions how this could/should be done.
>
> 5. patch ordering doesn't compile
> I guess this is just a rebase hiccup. Not an issue for an RFC
Yeah sorry about that. Will fix all those for next rev.
>
> 6. tests
> For these kinds of new classes of devices, I'd like to have tests in
> the https://gitlab.freedesktop.org/libevdev/hid-tools repository.
> There is already an initial MR for tablet support (!115 in this
> project), and we should extend it with more tests.
Ok I can take a look at these, thanks for the pointer. I am quite new to 
the input side of things and have been using whatever I have been able 
to craft myself, or found via googling.
>
> I'd happily help with those tests if you could share the report
> descriptors and some device dumps made with the hid-recorder tool from
> that repository.

Yeah, I can share these in your preferred format once I figure out the 
test tools. I have been using some custom tools to parse things myself 
so far (mostly kernel traces for both HID + I2C subsystem where my USI 
controller is connected.)

-Tero

>
>> The driver has been tested with chromebooks that contain either Goodix
>> or Elan manufactured USI capable touchscreen controllers in them.
>>
>> Any feedback appreciated!
> I'll try to have a deeper look next week (though it seems a few bits
> stacked up during my week off, sigh).
>
> Cheers,
> Benjamin
>
>> -Tero
>>
>>

Re: [RFC 0/8] HID: add support for USI style pens

[Benjamin Tissoires]

Hi Tero,

On Mon, Nov 8, 2021 at 8:44 AM Tero Kristo <tero.kristo@linux.intel.com> wrote:
>
> Hi Benjamin,
>
> Thanks for your feedback! Couple of quick replies below.
>
> On 05/11/2021 20:22, Benjamin Tissoires wrote:
> > Hi Tero,
> >
> > [just a quick note, I am supposed to be on holiday this week]
> >
> > On Thu, Nov 4, 2021 at 2:38 PM Tero Kristo <tero.kristo@linux.intel.com> wrote:
> >> Hi,
> >>
> >> This series is an RFC for USI (Universal Stylus Interface) style pen
> >> support. This is based on documentation from USB org describing the HID
> >> usage tables for digitizers (page 0x0D) and experimentation with actual
> >> USI capable controllers.
> >>
> >> This series introduces the USI support with a new HID driver, which
> >> applies the controller specific quirks. The most problematic part of the
> >> USI support is handling of the pen parameters (color, line width, line
> >> style), which are not immediately available from the controller from pen
> >> down event, but must be cached and queried separately from the controller.
> >> In addition to that, when a get-feature report is sent to the
> >> controller, there is a delay before the proper value is reported out; it
> >> is not part of the feature report coming back immediately.
> >> Most of the code in the driver is to handle this (otherwise we could
> >> just use hid-generic.)
> >>
> >> This also boils down to the reason why this series is an RFC, I would like
> >> to receive some feedback which option to pick for programming of the new
> >> values for the programmable pen parameters; whether to parse the input
> >> events so userspace can directly write the new values to the input event
> >> file handle, or whether to use IOCTL. Patches #7 / #8 are sort of optional
> >> choices towards this, but are there to show that both approaches can be
> >> done. Direct write to evdev causes some confusion on the driver level
> >> though, thus patch #7 is there to avoid some of that introducing new
> >> input events for writing the parameters. IOCTL might be the cleanest
> >> approach and I am slightly leaning towards that myself (see patch #8,
> >> this would need to be squashed and cleaned up a bit though but would
> >> effectively get rid of some code from patch #6 and completely rid patch #7.)
> > This series unfortunately raised quite a few red flags for me, and I
> > am glad this is just an RFC.
> > Let me enumerate them first and discuss a little bit more about those:
> >   1. USI is supposed to be generic, so why is there a new driver for it
> > instead of being handled by hid-input.c?
> >   2. new MSC_EVENTS are created without Dmitry or Peter being CC-ed
> >   3. new ioctls???
> >   4. direct write to evdev to write parameters
> >   5. patch 1/8 doesn't compile without 5/8
> >   6. no tests :)
> >
> > 1. new driver
> > After quickly reading the RFC, I think the main issue there is that we
> > are now having a transducer index which is incompatible with the way
> > input and evedev works nowadays. Yay, we have a new hid-multitouch for
> > pen :(
> >
> > Wacom has been dealing with that situation for years by tweaking the
> > protocol and by just emitting a different serial number (roughly). I
> > think the safest approach would be to keep the existing protocol
> > running so that our user space can handle it properly.
> >
> > I'd need to read the rest of the code more carefully, but if we could
> > have a basic generic handling (without the fancy features like
> > changing the pen style/color) I'd be happier.
>
> The USI pen support is partially compatible with existing input
> framework, e.g. co-ordinates + touch events work out of box with
> no-modification to the kernel whatsoever, just by using hid-generic
> driver. What is missing completely is the new features;
> color/width/style. It would be possible to move all these to the
> hid-input driver obviously, I don't think there is anything to prevent
> that. And, I could split up the series so that in the initial patches we
> would only support reporting current color/width/style parameters, and
> add the programmability as a subsequent patch if that would be better.
> It would also be possible to move parts of the code to the input
> subsystem from HID (some initial patches from our side were done this
> way, but I don't think this is necessary.)

I think I'd like to see the common behavior in hid-input.c, yes. This
is mostly because other drivers will be able to use that for free
(hid-multitouch for devices that are both touch and pen capable for
instance).

We can deal with the programmability from userspace later.

As for merging part of the code from HID to input, I'd like to see
what you are talking about.

>
> >
> > 2. MSC_* events
> > there is an issue with those: they are not cached like the ABS_* ones.
> > Meaning that each report will wake up userspace for something which
> > basically doesn't change.
> > I know ABS_* is saturated, but I'd like to have reviews from others on
> > what could be done here instead of just using MSC_* as a new ABS_*
> In my tests, it seems like MSC_* from the USI pens with these patches
> only get reported to userspace if something changes. Otherwise they do
> not get through at all. I have a small quirk in the driver to address
> this for a case where a new userspace handle is opened while pen is
> already in use; it does not get the params reported at all unless I
> flush the current entries out (by reporting a bogus value followed
> immediately by the real value.) Anyways, ABS events would be fine for
> the parameters also I believe if this is desirable.

That is by design. When a userspace program opens a device node, it
has to query its current state by running a few ioctls.
We have libevdev for that that simplifies everything for the userspace.
libevdev does a lot more than just that, and every userspace program
should use it to not have to deal with SYN_DROPPED and other
subtleties in the protocol.

> >
> > 3. ioctls
> > this is problematic to me. Any new kernel ABI is problematic to me,
> > and I'd much rather not add any new ones.
> I agree I am torn between the ioctl / direct evdev write. Both have
> their bad sides to them, thus I provided sort of support for both. :)
> >
> > My new set of mind is because of the recent work I have been
> > conducting regarding eBPF.
> > Basically I managed to have eBPF programs handling the device
> > configuration and event processing in a local branch.
> > I should be able to push a WIP next week, but basically this should
> > allow me to not have to deal with new kernel APIs besides the generic
> > eBPF one.
> > We can imagine a generic hid-input.c processing for those tablets, and
> > have a new userspace component that loads an eBPF program with its own
> > userspace API which is capable of the fancy features.
> >
> > For instance, my current playground is setting the haptic feedback of
> > the Surface Dial depending on the resolution I set on it.
> >
> > Furthermore, ioctls on a new cdev means that the classic userspace
> > libraries will not have access to it without some heavy tuning in the
> > systemd space (libinput only has read/write access to
> > /dev/input/event*).
>
> So, you are saying it would be possible to use this to support the USI
> pen parameters also somehow? I can take a look at this once available.

Yeah, basically once you load the ebpf program in the kernel, you have
a shared memory with userspace that you can use to create your own
API.
You could even ditch entirely the input subsystem and do the
processing in the eBPF program and pull the data from that shared
memory.

The way I see is the following:
- hid-tools (or maybe an other repo, not sure there) is responsible
for holding the list of bpf programs that we need to maintain (it's
the new upstream for those kind of quirks)
- we have one or more userspace program to load those eBPF program as
an intrinsic in udev
- those userspace program could be a one shot (attach a bpf program,
pin it and return)
- but userspace program could also present any RPC mechanism (dbus,
unix pipe, etc)

The advantage here is that we can control the API from the userspace:
if we do not use it anymore, we can just ditch the eBPF program (or
not load it). We can also rely on classic lib versioning or dbus
versioning.

I just pushed a branch "hid-bpf-2" at
https://gitlab.freedesktop.org/bentiss/hid/-/commits/hid-bpf-2 with
the first examples. The interesting commits are between the tip and
the `build` branch.

>
> >
> > 4. direct write to evdev
> >
> > We enabled that once for LEDs, and it's a pain to maintain. Maybe we
> > can make a use case for it but given that you don't seem very
> > enthusiastic about it too, I wonder if this is not a dead end.
> Well, we need to be able to program the pen parameters somehow from
> userspace, I am open to any suggestions how this could/should be done.
> >
> > 5. patch ordering doesn't compile
> > I guess this is just a rebase hiccup. Not an issue for an RFC
> Yeah sorry about that. Will fix all those for next rev.
> >
> > 6. tests
> > For these kinds of new classes of devices, I'd like to have tests in
> > the https://gitlab.freedesktop.org/libevdev/hid-tools repository.
> > There is already an initial MR for tablet support (!115 in this
> > project), and we should extend it with more tests.
> Ok I can take a look at these, thanks for the pointer. I am quite new to
> the input side of things and have been using whatever I have been able
> to craft myself, or found via googling.
> >
> > I'd happily help with those tests if you could share the report
> > descriptors and some device dumps made with the hid-recorder tool from
> > that repository.
>
> Yeah, I can share these in your preferred format once I figure out the
> test tools. I have been using some custom tools to parse things myself
> so far (mostly kernel traces for both HID + I2C subsystem where my USI
> controller is connected.)

Maybe just add a new test in
https://gitlab.freedesktop.org/libevdev/hid-tools/-/blob/master/tests/test_tablet.py
like https://gitlab.freedesktop.org/libevdev/hid-tools/-/commit/7fa34c2c86e1380eb9c233422567b24a0fd6541f

To extract the report descriptor, use `sudo hid-recorder` at the root
of this repo, and copy the line starting with `R:` or use your
favorite tool :).

Cheers,
Benjamin

>
> -Tero
>
> >
> >> The driver has been tested with chromebooks that contain either Goodix
> >> or Elan manufactured USI capable touchscreen controllers in them.
> >>
> >> Any feedback appreciated!
> > I'll try to have a deeper look next week (though it seems a few bits
> > stacked up during my week off, sigh).
> >
> > Cheers,
> > Benjamin
> >
> >> -Tero
> >>
> >>
>

Re: [RFC 0/8] HID: add support for USI style pens

[Tero Kristo]

[-- Attachment #1: Type: text/plain, Size: 12584 bytes --]

Hi Benjamin,

On 09/11/2021 19:02, Benjamin Tissoires wrote:
> Hi Tero,
>
> On Mon, Nov 8, 2021 at 8:44 AM Tero Kristo <tero.kristo@linux.intel.com> wrote:
>> Hi Benjamin,
>>
>> Thanks for your feedback! Couple of quick replies below.
>>
>> On 05/11/2021 20:22, Benjamin Tissoires wrote:
>>> Hi Tero,
>>>
>>> [just a quick note, I am supposed to be on holiday this week]
>>>
>>> On Thu, Nov 4, 2021 at 2:38 PM Tero Kristo <tero.kristo@linux.intel.com> wrote:
>>>> Hi,
>>>>
>>>> This series is an RFC for USI (Universal Stylus Interface) style pen
>>>> support. This is based on documentation from USB org describing the HID
>>>> usage tables for digitizers (page 0x0D) and experimentation with actual
>>>> USI capable controllers.
>>>>
>>>> This series introduces the USI support with a new HID driver, which
>>>> applies the controller specific quirks. The most problematic part of the
>>>> USI support is handling of the pen parameters (color, line width, line
>>>> style), which are not immediately available from the controller from pen
>>>> down event, but must be cached and queried separately from the controller.
>>>> In addition to that, when a get-feature report is sent to the
>>>> controller, there is a delay before the proper value is reported out; it
>>>> is not part of the feature report coming back immediately.
>>>> Most of the code in the driver is to handle this (otherwise we could
>>>> just use hid-generic.)
>>>>
>>>> This also boils down to the reason why this series is an RFC, I would like
>>>> to receive some feedback which option to pick for programming of the new
>>>> values for the programmable pen parameters; whether to parse the input
>>>> events so userspace can directly write the new values to the input event
>>>> file handle, or whether to use IOCTL. Patches #7 / #8 are sort of optional
>>>> choices towards this, but are there to show that both approaches can be
>>>> done. Direct write to evdev causes some confusion on the driver level
>>>> though, thus patch #7 is there to avoid some of that introducing new
>>>> input events for writing the parameters. IOCTL might be the cleanest
>>>> approach and I am slightly leaning towards that myself (see patch #8,
>>>> this would need to be squashed and cleaned up a bit though but would
>>>> effectively get rid of some code from patch #6 and completely rid patch #7.)
>>> This series unfortunately raised quite a few red flags for me, and I
>>> am glad this is just an RFC.
>>> Let me enumerate them first and discuss a little bit more about those:
>>>    1. USI is supposed to be generic, so why is there a new driver for it
>>> instead of being handled by hid-input.c?
>>>    2. new MSC_EVENTS are created without Dmitry or Peter being CC-ed
>>>    3. new ioctls???
>>>    4. direct write to evdev to write parameters
>>>    5. patch 1/8 doesn't compile without 5/8
>>>    6. no tests :)
>>>
>>> 1. new driver
>>> After quickly reading the RFC, I think the main issue there is that we
>>> are now having a transducer index which is incompatible with the way
>>> input and evedev works nowadays. Yay, we have a new hid-multitouch for
>>> pen :(
>>>
>>> Wacom has been dealing with that situation for years by tweaking the
>>> protocol and by just emitting a different serial number (roughly). I
>>> think the safest approach would be to keep the existing protocol
>>> running so that our user space can handle it properly.
>>>
>>> I'd need to read the rest of the code more carefully, but if we could
>>> have a basic generic handling (without the fancy features like
>>> changing the pen style/color) I'd be happier.
>> The USI pen support is partially compatible with existing input
>> framework, e.g. co-ordinates + touch events work out of box with
>> no-modification to the kernel whatsoever, just by using hid-generic
>> driver. What is missing completely is the new features;
>> color/width/style. It would be possible to move all these to the
>> hid-input driver obviously, I don't think there is anything to prevent
>> that. And, I could split up the series so that in the initial patches we
>> would only support reporting current color/width/style parameters, and
>> add the programmability as a subsequent patch if that would be better.
>> It would also be possible to move parts of the code to the input
>> subsystem from HID (some initial patches from our side were done this
>> way, but I don't think this is necessary.)
> I think I'd like to see the common behavior in hid-input.c, yes. This
> is mostly because other drivers will be able to use that for free
> (hid-multitouch for devices that are both touch and pen capable for
> instance).
I have looked at this a bit now, and mostly the initial patches in this 
series are good for the purpose, just need to ditch #6+ and replace 
those with something for programming the pen parameters via BPF. I will 
clean up whatever I have atm and post that as RFCv2.
> We can deal with the programmability from userspace later.
>
> As for merging part of the code from HID to input, I'd like to see
> what you are talking about.

The split was mostly for programming the pen parameters. It had 
callbacks to hid-core though, and the API wasn't too clean between 
hid<->input subsystem in this case. Anyways, if BPF is preferred way to 
handle the programming of pen parameters, this split between hid<->input 
is not needed.

>
>>> 2. MSC_* events
>>> there is an issue with those: they are not cached like the ABS_* ones.
>>> Meaning that each report will wake up userspace for something which
>>> basically doesn't change.
>>> I know ABS_* is saturated, but I'd like to have reviews from others on
>>> what could be done here instead of just using MSC_* as a new ABS_*
>> In my tests, it seems like MSC_* from the USI pens with these patches
>> only get reported to userspace if something changes. Otherwise they do
>> not get through at all. I have a small quirk in the driver to address
>> this for a case where a new userspace handle is opened while pen is
>> already in use; it does not get the params reported at all unless I
>> flush the current entries out (by reporting a bogus value followed
>> immediately by the real value.) Anyways, ABS events would be fine for
>> the parameters also I believe if this is desirable.
> That is by design. When a userspace program opens a device node, it
> has to query its current state by running a few ioctls.
> We have libevdev for that that simplifies everything for the userspace.
> libevdev does a lot more than just that, and every userspace program
> should use it to not have to deal with SYN_DROPPED and other
> subtleties in the protocol.

Ok, that seems fine. I need to check how to compile / patch libevdev 
myself to accommodate the changes needed. What is your recommendation 
for the pen events? Use ABS_x at the end of range (0x40+), or stay with 
MSC_x? Either way, I need to allocate some event numbers for these.

>
>>> 3. ioctls
>>> this is problematic to me. Any new kernel ABI is problematic to me,
>>> and I'd much rather not add any new ones.
>> I agree I am torn between the ioctl / direct evdev write. Both have
>> their bad sides to them, thus I provided sort of support for both. :)
>>> My new set of mind is because of the recent work I have been
>>> conducting regarding eBPF.
>>> Basically I managed to have eBPF programs handling the device
>>> configuration and event processing in a local branch.
>>> I should be able to push a WIP next week, but basically this should
>>> allow me to not have to deal with new kernel APIs besides the generic
>>> eBPF one.
>>> We can imagine a generic hid-input.c processing for those tablets, and
>>> have a new userspace component that loads an eBPF program with its own
>>> userspace API which is capable of the fancy features.
>>>
>>> For instance, my current playground is setting the haptic feedback of
>>> the Surface Dial depending on the resolution I set on it.
>>>
>>> Furthermore, ioctls on a new cdev means that the classic userspace
>>> libraries will not have access to it without some heavy tuning in the
>>> systemd space (libinput only has read/write access to
>>> /dev/input/event*).
>> So, you are saying it would be possible to use this to support the USI
>> pen parameters also somehow? I can take a look at this once available.
> Yeah, basically once you load the ebpf program in the kernel, you have
> a shared memory with userspace that you can use to create your own
> API.
> You could even ditch entirely the input subsystem and do the
> processing in the eBPF program and pull the data from that shared
> memory.
>
> The way I see is the following:
> - hid-tools (or maybe an other repo, not sure there) is responsible
> for holding the list of bpf programs that we need to maintain (it's
> the new upstream for those kind of quirks)
> - we have one or more userspace program to load those eBPF program as
> an intrinsic in udev
> - those userspace program could be a one shot (attach a bpf program,
> pin it and return)
> - but userspace program could also present any RPC mechanism (dbus,
> unix pipe, etc)
>
> The advantage here is that we can control the API from the userspace:
> if we do not use it anymore, we can just ditch the eBPF program (or
> not load it). We can also rely on classic lib versioning or dbus
> versioning.
>
> I just pushed a branch "hid-bpf-2" at
> https://gitlab.freedesktop.org/bentiss/hid/-/commits/hid-bpf-2 with
> the first examples. The interesting commits are between the tip and
> the `build` branch.

I did experiment with this a bit, and looked at the code. So, it seems 
like there is possibility for raw_event re-mapping which would be needed 
to fix some quirks for the current USI controllers, so it seems I can do 
whatever needed with the support available there. However, I did run 
into some problems while trying out the hid-bpf samples myself; they 
seem to fail either during the object load or bpf program attach phases. 
Are the new samples tested, and with what clang version? There seem to 
be plenty of pitfalls with the clang version used. And yes, at least 
some of the other programs under samples/bpf/ do work for me so I do 
believe I am compiling these properly.

>
>>> 4. direct write to evdev
>>>
>>> We enabled that once for LEDs, and it's a pain to maintain. Maybe we
>>> can make a use case for it but given that you don't seem very
>>> enthusiastic about it too, I wonder if this is not a dead end.
>> Well, we need to be able to program the pen parameters somehow from
>> userspace, I am open to any suggestions how this could/should be done.
>>> 5. patch ordering doesn't compile
>>> I guess this is just a rebase hiccup. Not an issue for an RFC
>> Yeah sorry about that. Will fix all those for next rev.
>>> 6. tests
>>> For these kinds of new classes of devices, I'd like to have tests in
>>> the https://gitlab.freedesktop.org/libevdev/hid-tools repository.
>>> There is already an initial MR for tablet support (!115 in this
>>> project), and we should extend it with more tests.
>> Ok I can take a look at these, thanks for the pointer. I am quite new to
>> the input side of things and have been using whatever I have been able
>> to craft myself, or found via googling.
>>> I'd happily help with those tests if you could share the report
>>> descriptors and some device dumps made with the hid-recorder tool from
>>> that repository.
>> Yeah, I can share these in your preferred format once I figure out the
>> test tools. I have been using some custom tools to parse things myself
>> so far (mostly kernel traces for both HID + I2C subsystem where my USI
>> controller is connected.)
> Maybe just add a new test in
> https://gitlab.freedesktop.org/libevdev/hid-tools/-/blob/master/tests/test_tablet.py
> like https://gitlab.freedesktop.org/libevdev/hid-tools/-/commit/7fa34c2c86e1380eb9c233422567b24a0fd6541f
>
> To extract the report descriptor, use `sudo hid-recorder` at the root
> of this repo, and copy the line starting with `R:` or use your
> favorite tool :).

Attached the rdesc for now, I can record things once we figure out which 
event codes to use (got the hid-tools working with my buildroot env.)

-Tero


>
> Cheers,
> Benjamin
>
>> -Tero
>>
>>>> The driver has been tested with chromebooks that contain either Goodix
>>>> or Elan manufactured USI capable touchscreen controllers in them.
>>>>
>>>> Any feedback appreciated!
>>> I'll try to have a deeper look next week (though it seems a few bits
>>> stacked up during my week off, sigh).
>>>
>>> Cheers,
>>> Benjamin
>>>
>>>> -Tero
>>>>
>>>>

[-- Attachment #2: rdesc-acer.txt --]
[-- Type: text/plain, Size: 3532 bytes --]

R: 1174 05 0d 09 04 a1 01 85 01 09 22 a1 02 09 42 15 00 25 01 75 01 95 01 81 02 75 01 81 03 75 06 09 51 25 3f 81 02 26 ff 00 75 08 55 0f 65 11 35 00 45 ff 09 48 81 02 09 49 81 02 09 30 81 02 95 01 05 01 a4 26 cf 0f 75 10 55 0f 65 11 09 30 35 00 46 26 01 95 01 81 02 26 77 0a 46 a6 00 09 31 81 02 b4 c0 05 0d 09 22 a1 02 05 0d 09 42 15 00 25 01 75 01 95 01 81 02 75 01 81 03 75 06 09 51 25 3f 81 02 26 ff 00 75 08 55 0f 65 11 35 00 45 ff 09 48 81 02 09 49 81 02 09 30 81 02 95 01 05 01 a4 26 cf 0f 75 10 55 0f 65 11 09 30 35 00 46 26 01 95 01 81 02 26 77 0a 46 a6 00 09 31 81 02 b4 c0 05 0d 09 22 a1 02 05 0d 09 42 15 00 25 01 75 01 95 01 81 02 75 01 81 03 75 06 09 51 25 3f 81 02 26 ff 00 75 08 55 0f 65 11 35 00 45 ff 09 48 81 02 09 49 81 02 09 30 81 02 95 01 05 01 a4 26 cf 0f 75 10 55 0f 65 11 09 30 35 00 46 26 01 95 01 81 02 26 77 0a 46 a6 00 09 31 81 02 b4 c0 05 0d 09 22 a1 02 05 0d 09 42 15 00 25 01 75 01 95 01 81 02 75 01 81 03 75 06 09 51 25 3f 81 02 26 ff 00 75 08 55 0f 65 11 35 00 45 ff 09 48 81 02 09 49 81 02 09 30 81 02 95 01 05 01 a4 26 cf 0f 75 10 55 0f 65 11 09 30 35 00 46 26 01 95 01 81 02 26 77 0a 46 a6 00 09 31 81 02 b4 c0 05 0d 09 22 a1 02 05 0d 09 42 15 00 25 01 75 01 95 01 81 02 75 01 81 03 75 06 09 51 25 3f 81 02 26 ff 00 75 08 55 0f 65 11 35 00 45 ff 09 48 81 02 09 49 81 02 09 30 81 02 95 01 05 01 a4 26 cf 0f 75 10 55 0f 65 11 09 30 35 00 46 26 01 95 01 81 02 26 77 0a 46 a6 00 09 31 81 02 b4 c0 05 0d 09 54 25 7f 96 01 00 75 08 81 02 85 0a 09 55 25 0a b1 02 85 44 06 00 ff 09 c5 16 00 00 26 ff 00 75 08 96 00 01 b1 02 c0 06 ff 01 09 01 a1 01 85 02 16 00 00 26 ff 00 75 08 95 40 09 00 81 02 c0 06 00 ff 09 01 a1 01 85 03 75 08 95 20 09 01 91 02 c0 06 00 ff 09 01 a1 01 85 06 09 03 75 08 95 12 91 02 09 04 75 08 95 03 b1 02 c0 06 01 ff 09 01 a1 01 85 04 15 00 26 ff 00 75 08 95 13 09 00 81 02 c0 05 0d 09 02 a1 01 85 07 35 00 09 20 a1 00 09 32 09 42 09 44 09 3c 09 45 15 00 25 01 75 01 95 05 81 02 95 03 81 03 05 01 09 30 75 10 95 01 a4 55 0f 65 11 46 26 01 26 1c 48 81 42 09 31 46 a6 00 26 bc 2f 81 42 b4 05 0d 09 30 26 00 10 81 02 75 08 95 01 09 3b 25 64 81 42 09 38 15 00 25 02 81 02 09 5c 26 ff 00 81 02 09 5e 81 02 09 70 a1 02 15 01 25 06 09 72 09 73 09 74 09 75 09 76 09 77 81 20 09 5b 25 ff 75 40 81 02 c0 06 00 ff 75 08 95 02 09 01 81 02 c0 05 0d 85 60 09 81 a1 02 09 38 75 08 95 01 15 00 25 02 81 02 09 81 15 01 25 04 09 82 09 83 09 84 09 85 81 20 c0 85 61 09 5c a1 02 15 00 26 ff 00 75 08 95 01 09 38 b1 02 09 5c 26 ff 00 b1 02 09 5d 75 01 95 01 25 01 b1 02 95 07 b1 03 c0 85 62 09 5e a1 02 09 38 15 00 25 02 75 08 95 01 b1 02 09 5e 26 ff 00 b1 02 09 5f 75 01 25 01 b1 02 75 07 b1 03 c0 85 63 09 70 a1 02 75 08 95 01 15 00 25 02 09 38 b1 02 09 70 a1 02 25 06 09 72 09 73 09 74 09 75 09 76 09 77 b1 20 c0 09 71 75 01 25 01 b1 02 75 07 b1 03 c0 85 64 09 80 15 00 25 ff 75 40 95 01 b1 02 85 65 09 44 a1 02 09 38 75 08 95 01 25 02 b1 02 15 01 25 03 09 44 a1 02 09 a4 09 44 09 5a 09 45 09 a3 b1 20 c0 09 5a a1 02 09 a4 09 44 09 5a 09 45 09 a3 b1 20 c0 09 45 a1 02 09 a4 09 44 09 5a 09 45 09 a3 b1 20 c0 c0 85 66 75 08 95 01 05 0d 09 90 a1 02 09 38 25 02 b1 02 09 91 75 10 26 ff 0f b1 02 09 92 75 40 25 ff b1 02 05 06 09 2a 75 08 26 ff 00 a1 02 09 2d b1 02 09 2e b1 02 c0 c0 85 67 05 06 09 2b a1 02 05 0d 25 02 09 38 b1 02 05 06 09 2b a1 02 09 2d 26 ff 00 b1 02 09 2e b1 02 c0 c0 85 68 06 00 ff 09 01 a1 02 05 0d 09 38 75 08 95 01 25 02 b1 02 06 00 ff 09 01 75 10 27 ff ff 00 00 b1 02 c0 85 69 05 0d 09 38 75 08 95 01 15 00 25 02 b1 02 c0 06 00 ff 09 81 a1 01 85 17 75 08 95 1f 09 05 81 02 c0