[RFCv3 0/7] USI stylus support series

[RFCv3 0/7] USI stylus support series

[Tero Kristo]

Hi,

Another update here based on received feedback. Main change compared to v2:

- Dropped patch #5 : HID: core: map USI pen style reports directly
  This is not needed anymore, as the same result can be reached by
  modifying the flags of the relevant field in rdesc. This is done now
  as part of patch #7 in the BPF code.

I also dropped one of the fixes from my test branch [1] as pointed out
by Benjamin, it is not needed if the BPF program is executed with the
modalias link.

Updated test branch can be found from [2].

-Tero

[1]
https://github.com/t-kristo/linux/commit/81b27fd46780ce67c2706d586c0f4a437e87dbf6
(HID: bpf: fix file mapping)
[2] https://github.com/t-kristo/linux/commits/usi-5.16-rfc-v3-bpf

[RFCv3 1/7] 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>
---
 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 2c72ce4147b1..39ebedb2323b 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))
@@ -872,10 +873,7 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
 
 		case 0x5b: /* TransducerSerialNumber */
 		case 0x6e: /* TransducerSerialNumber2 */
-			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 e95800bab56a..96eaca0d5322 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -766,6 +766,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

[RFCv3 2/7] 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>
---
 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 39ebedb2323b..73c2edda742e 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1737,6 +1737,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

[RFCv3 3/7] 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.

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

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 73c2edda742e..b428ee9b4d9b 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;
@@ -876,6 +880,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/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/hid.h b/include/uapi/linux/hid.h
index 861bfbbfc565..60ef9b615a1a 100644
--- a/include/uapi/linux/hid.h
+++ b/include/uapi/linux/hid.h
@@ -255,6 +255,7 @@
 #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
@@ -267,6 +268,15 @@
 #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_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
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

[RFCv3 4/7] 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>
---
 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 b428ee9b4d9b..f6332d269d49 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1477,7 +1477,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;
@@ -1492,7 +1493,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 96eaca0d5322..3f1fd4fcf1a9 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -636,7 +636,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

[RFCv3 5/7] 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 7a92e2a04a09..cec0470e5485 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,7 +162,40 @@ 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, 0x6e, "TransducerSerialNumber2"},
+    {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

[RFCv3 6/7] samples: hid: add new hid-usi sample

[Tero Kristo]

HID-usi sample adds support for writing USI pen variables. When the
sample is run, it creates a unix fifo under /tmp/usi, which can be
written with "<param> set <val>", where param is in [color,width,style],
and val is an integer value for the parameter.

Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
---
 samples/bpf/Makefile       |   3 +
 samples/bpf/hid_usi.h      |  21 ++++
 samples/bpf/hid_usi_kern.c | 230 ++++++++++++++++++++++++++++++++++
 samples/bpf/hid_usi_user.c | 247 +++++++++++++++++++++++++++++++++++++
 4 files changed, 501 insertions(+)
 create mode 100644 samples/bpf/hid_usi.h
 create mode 100644 samples/bpf/hid_usi_kern.c
 create mode 100644 samples/bpf/hid_usi_user.c

diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index 11f6f0cfab9e..6f45f6048979 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -62,6 +62,7 @@ tprogs-y += xdp_monitor
 
 tprogs-y += hid_mouse
 tprogs-y += hid_surface_dial
+tprogs-y += hid_usi
 
 # Libbpf dependencies
 LIBBPF_SRC = $(TOOLS_PATH)/lib/bpf
@@ -129,6 +130,7 @@ xdp_redirect-objs := xdp_redirect_user.o $(XDP_SAMPLE)
 xdp_monitor-objs := xdp_monitor_user.o $(XDP_SAMPLE)
 hid_mouse-objs := hid_mouse_user.o
 hid_surface_dial-objs := hid_surface_dial_user.o
+hid_usi-objs := hid_usi_user.o
 
 # Tell kbuild to always build the programs
 always-y := $(tprogs-y)
@@ -188,6 +190,7 @@ always-y += hbm_edt_kern.o
 always-y += xdpsock_kern.o
 always-y += hid_mouse_kern.o
 always-y += hid_surface_dial_kern.o
+always-y += hid_usi_kern.o
 
 ifeq ($(ARCH), arm)
 # Strip all except -D__LINUX_ARM_ARCH__ option needed to handle linux
diff --git a/samples/bpf/hid_usi.h b/samples/bpf/hid_usi.h
new file mode 100644
index 000000000000..90803375d9c1
--- /dev/null
+++ b/samples/bpf/hid_usi.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0
+ *
+ * Copyright(c) 2021 Intel Corporation.
+ */
+
+#ifndef HID_USI_H_
+#define HID_USI_H_
+
+#include <linux/bits.h>
+
+enum {
+	USI_PEN_ID = 0,
+	USI_PEN_IN_RANGE,
+	USI_PEN_TOUCHING,
+	USI_PEN_COLOR,
+	USI_PEN_LINE_WIDTH,
+	USI_PEN_LINE_STYLE,
+	USI_NUM_PARAMS
+};
+
+#endif /* HID_USI_H */
diff --git a/samples/bpf/hid_usi_kern.c b/samples/bpf/hid_usi_kern.c
new file mode 100644
index 000000000000..1ba32eef99c3
--- /dev/null
+++ b/samples/bpf/hid_usi_kern.c
@@ -0,0 +1,230 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright(c) 2021 Intel Corporation. */
+#include <linux/version.h>
+#include <uapi/linux/bpf.h>
+#include <uapi/linux/hid.h>
+#include <uapi/linux/bpf_hid.h>
+#include <bpf/bpf_helpers.h>
+
+#include "hid_usi.h"
+
+static const char param_names[USI_NUM_PARAMS][6] = {
+	"id",
+	"flags",
+	"color",
+	"width",
+	"style",
+};
+
+struct {
+	__uint(type, BPF_MAP_TYPE_RINGBUF);
+	__uint(max_entries, 4096);
+} ringbuf SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__type(key, int);
+	__type(value, int);
+	__uint(max_entries, USI_NUM_PARAMS);
+} cache SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__type(key, int);
+	__type(value, int);
+	__uint(max_entries, USI_NUM_PARAMS);
+} wr_cache SEC(".maps");
+
+struct rep_data {
+	int offset;
+	int size;
+	int idx;
+};
+
+static struct rep_data inputs[USI_NUM_PARAMS];
+static struct rep_data features[USI_NUM_PARAMS];
+
+SEC("hid/raw_event")
+int hid_raw_event(struct hid_bpf_ctx *ctx)
+{
+	u32 i;
+	u32 tmp;
+	int val;
+	int *wrc, *c;
+	u8 *buf;
+	u32 flags = 0;
+	int offset;
+	int size;
+	int in_range;
+	int touching;
+	bool cache_valid;
+
+	if (ctx->event.data[0] != inputs[USI_PEN_IN_RANGE].idx)
+		return 0;
+
+	in_range = bpf_hid_get_data(ctx, inputs[USI_PEN_IN_RANGE].offset,
+				    inputs[USI_PEN_IN_RANGE].size);
+	touching = bpf_hid_get_data(ctx, inputs[USI_PEN_TOUCHING].offset,
+				    inputs[USI_PEN_TOUCHING].size);
+
+	bpf_printk("flags: in_range=%d, touching=%d", in_range, touching);
+
+	if (!touching && cache_valid) {
+		for (i = USI_PEN_COLOR; i < USI_NUM_PARAMS; i++) {
+			val = 0;
+
+			/*
+			 * Make a local copy of i so that we can get a
+			 * pointer reference to it. i itself must remain
+			 * inside a register so that BPF verifier can
+			 * calculate its value properly.
+			 */
+			tmp = i;
+			bpf_map_update_elem(&wr_cache, &tmp, &val, 0);
+			bpf_map_update_elem(&cache, &tmp, &val, 0);
+		}
+		cache_valid = false;
+	}
+
+	if (!touching)
+		return 0;
+
+	for (i = USI_PEN_COLOR; i < USI_NUM_PARAMS; i++) {
+		offset = inputs[i].offset;
+		size = inputs[i].size;
+		val = bpf_hid_get_data(ctx, offset, size);
+
+		/*
+		 * Again, make a local copy of i which we can refer via a
+		 * pointer to satisfy BPF verifier.
+		 */
+		tmp = i;
+
+		wrc = bpf_map_lookup_elem(&wr_cache, &tmp);
+		c = bpf_map_lookup_elem(&cache, &tmp);
+		if (!wrc || !c)
+			continue;
+
+		bpf_printk("raw[%d]: %s = %02x", i, param_names[i], val);
+		bpf_printk(" (c=%02x, wr=%02x)", *c, *wrc);
+
+		if (*wrc != *c) {
+			val = *wrc;
+
+			buf = bpf_ringbuf_reserve(&ringbuf, 16, 0);
+			if (!buf)
+				continue;
+
+			buf[0] = features[i].idx;
+			buf[1] = 1;
+			buf[2] = val;
+
+			bpf_hid_raw_request(ctx, buf, 3,
+					    HID_FEATURE_REPORT,
+					    HID_REQ_SET_REPORT);
+
+			bpf_ringbuf_discard(buf, 0);
+		}
+
+		if (val != *c && !cache_valid) {
+			bpf_map_update_elem(&cache, &tmp, &val, 0);
+			bpf_map_update_elem(&wr_cache, &tmp, &val, 0);
+			cache_valid = true;
+		} else {
+			bpf_hid_set_data(ctx, offset, size, *c);
+		}
+	}
+
+	return 0;
+}
+
+SEC("hid/event")
+int set_haptic(struct hid_bpf_ctx *ctx)
+{
+	return 0;
+}
+
+static void process_tag(struct hid_bpf_ctx *ctx, struct rep_data *data,
+			struct hid_bpf_parser *parser, u64 *idx)
+{
+	u32 i;
+	int id;
+	u32 offset;
+
+	for (i = 0; i < parser->local.usage_index && i < 16; i++) {
+		offset = parser->report.current_offset +
+			i * parser->global.report_size;
+
+		switch (parser->local.usage[i]) {
+		case HID_DG_PEN_COLOR:
+			id = USI_PEN_COLOR;
+			break;
+		case HID_DG_PEN_LINE_WIDTH:
+			id = USI_PEN_LINE_WIDTH;
+			break;
+		case HID_DG_PEN_LINE_STYLE_INK:
+			id = USI_PEN_LINE_STYLE;
+			/*
+			 * Force flags for line style. This makes it act
+			 * as a simple variable from HID core point of view.
+			 */
+			bpf_hid_set_data(ctx, (*idx + 1) << 3, 8, 0x2);
+			break;
+		case HID_DG_INRANGE:
+			if (parser->local.usage_index == 1)
+				continue;
+
+			id = USI_PEN_IN_RANGE;
+			break;
+		case HID_DG_TIPSWITCH:
+			if (parser->local.usage_index == 1)
+				continue;
+
+			id = USI_PEN_TOUCHING;
+			break;
+		default:
+			continue;
+		}
+
+		data[id].offset = offset + 8;
+		data[id].size = parser->global.report_size;
+		data[id].idx = parser->report.id;
+
+		bpf_printk("parsed id=%d, offset=%d, idx=%d",
+			   id, data[id].offset, data[id].idx);
+	}
+}
+
+static u64 process_hid_rdesc_item(struct hid_bpf_ctx *ctx,
+				  struct hid_bpf_parser *parser, u64 *idx,
+				  void *data)
+{
+	struct hid_bpf_item *item = &parser->item;
+
+	switch (item->type) {
+	case HID_ITEM_TYPE_MAIN:
+		if (item->tag == HID_MAIN_ITEM_TAG_INPUT)
+			process_tag(ctx, inputs, parser, idx);
+		if (item->tag == HID_MAIN_ITEM_TAG_FEATURE)
+			process_tag(ctx, features, parser, idx);
+	}
+
+	return 0;
+}
+
+SEC("hid/rdesc_fixup")
+int hid_rdesc_fixup(struct hid_bpf_ctx *ctx)
+{
+	int ret;
+
+	if (ctx->type != HID_BPF_RDESC_FIXUP)
+		return 0;
+
+	ret = bpf_hid_foreach_rdesc_item(ctx, process_hid_rdesc_item, (void *)0, 0);
+	bpf_printk("ret: %d", ret);
+
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
+u32 _version SEC("version") = LINUX_VERSION_CODE;
diff --git a/samples/bpf/hid_usi_user.c b/samples/bpf/hid_usi_user.c
new file mode 100644
index 000000000000..b05a3f768835
--- /dev/null
+++ b/samples/bpf/hid_usi_user.c
@@ -0,0 +1,247 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2021, Intel Corporation
+ */
+#include <linux/bpf.h>
+#include <linux/if_link.h>
+#include <assert.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <libgen.h>
+#include <sys/resource.h>
+#include <getopt.h>
+
+#include "bpf_util.h"
+#include <bpf/bpf.h>
+#include <bpf/libbpf.h>
+
+#include <sys/stat.h>
+
+#include "hid_usi.h"
+
+static char *sysfs_path;
+static char *fifoname = "/tmp/usi";
+static int sysfs_fd;
+static int fifo_fd;
+static int prog_count;
+static int cache, wr_cache;
+
+static const struct option long_options[] = {
+	{ "help", no_argument, NULL, 'h' },
+	{ "fifo", required_argument, NULL, 'f' },
+};
+
+struct prog {
+	int fd;
+	enum bpf_attach_type type;
+};
+
+static struct prog progs[10];
+
+static void int_exit(int sig)
+{
+	int ret;
+
+	for (prog_count--; prog_count >= 0 ; prog_count--) {
+		ret = bpf_prog_detach2(progs[prog_count].fd, sysfs_fd, progs[prog_count].type);
+		if (ret)
+			fprintf(stderr, "bpf_prog_detach2: returned %m\n");
+	}
+
+	close(sysfs_fd);
+	close(fifo_fd);
+	remove(fifoname);
+	exit(0);
+}
+
+static void usage(const char *prog)
+{
+	fprintf(stderr,
+		"usage: %s [-f <fifoname>] /dev/HIDRAW\n\n",
+		prog);
+}
+
+static int param_to_idx(const char *param)
+{
+	if (!strcmp(param, "color"))
+		return USI_PEN_COLOR;
+	if (!strcmp(param, "width"))
+		return USI_PEN_LINE_WIDTH;
+	if (!strcmp(param, "style"))
+		return USI_PEN_LINE_STYLE;
+
+	return -EINVAL;
+}
+
+static int write_value(const char *param, int value)
+{
+	int err;
+	int idx = param_to_idx(param);
+
+	printf("%s: param=%s (%d), value=%d\n", __func__, param, idx, value);
+	err = bpf_map_update_elem(wr_cache, &idx, &value, BPF_ANY);
+	if (err)
+		printf("Update failed for %d, err=%d\n", idx, err);
+
+	return 0;
+}
+
+static int read_value(const char *param)
+{
+	int value;
+	int idx = param_to_idx(param);
+
+	printf("%s: param=%s (%d)\n", __func__, param, idx);
+
+	if (bpf_map_lookup_elem(cache, &idx, &value))
+		printf("Value missing for %d\n", idx);
+	else
+		printf("Value for %d = %d\n", idx, value);
+
+	return 0;
+}
+
+static int attach_progs(int argc, char **argv)
+{
+	char filename[256];
+	struct bpf_prog_info info = {};
+	__u32 info_len = sizeof(info);
+	struct bpf_object *obj;
+	struct bpf_program *prog;
+	int err = 0;
+	char buf[BUFSIZ];
+	char param[16];
+	char op[8];
+	int value;
+	int m, n;
+
+	snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
+	obj = bpf_object__open_file(filename, NULL);
+	err = libbpf_get_error(obj);
+	if (err) {
+		fprintf(stderr, "ERROR: opening BPF object file failed\n");
+		obj = NULL;
+		err = 1;
+		goto cleanup;
+	}
+
+	/* load BPF program */
+	err = bpf_object__load(obj);
+	if (err) {
+		fprintf(stderr, "ERROR: loading BPF object file failed\n");
+		goto cleanup;
+	}
+
+	sysfs_fd = open(sysfs_path, O_RDONLY);
+
+	bpf_object__for_each_program(prog, obj) {
+		progs[prog_count].fd = bpf_program__fd(prog);
+		progs[prog_count].type = bpf_program__get_expected_attach_type(prog);
+
+		err = bpf_prog_attach(progs[prog_count].fd,
+				      sysfs_fd,
+				      progs[prog_count].type,
+				      0);
+		if (err) {
+			fprintf(stderr, "bpf_prog_attach: err=%m\n");
+			progs[prog_count].fd = 0;
+			goto cleanup;
+		}
+		printf("attached BPF program with FD=%d, type=%d\n",
+		       progs[prog_count].fd, progs[prog_count].type);
+		prog_count++;
+	}
+
+	signal(SIGINT, int_exit);
+	signal(SIGTERM, int_exit);
+
+	err = bpf_obj_get_info_by_fd(progs[0].fd, &info, &info_len);
+	if (err) {
+		printf("can't get prog info - %s\n", strerror(errno));
+		goto cleanup;
+	}
+
+	cache = bpf_object__find_map_fd_by_name(obj, "cache");
+	if (cache < 0) {
+		printf("can't get 'cache' shared mem from object - %m\n");
+		goto cleanup;
+	}
+
+	wr_cache = bpf_object__find_map_fd_by_name(obj, "wr_cache");
+	if (wr_cache < 0) {
+		printf("can't get 'wr_cache' shared mem from object - %m\n");
+		goto cleanup;
+	}
+
+	mkfifo(fifoname, 0666);
+
+	fifo_fd = open(fifoname, O_RDWR);
+	if (fifo_fd < 0) {
+		perror("Fifo open error.\n");
+		err = fifo_fd;
+		goto cleanup;
+	}
+
+	while (1) {
+		n = read(fifo_fd, buf, BUFSIZ);
+		if (n < 0)
+			break;
+		buf[n] = 0;
+
+		printf("%s: received '%s'\n", __func__, buf);
+
+		m = sscanf(buf, "%16s %8s %d", param, op, &value);
+		if (m == 2 && strcmp(op, "get") == 0)
+			read_value(param);
+		else if (m == 3 && strcmp(op, "set") == 0)
+			write_value(param, value);
+	}
+
+	return 0;
+
+ cleanup:
+	for (prog_count--; prog_count >= 0; prog_count--) {
+		if (bpf_prog_detach2(progs[prog_count].fd, sysfs_fd, progs[prog_count].type))
+			fprintf(stderr, "bpf_prog_detach2: returned %m\n");
+	}
+
+	bpf_object__close(obj);
+	return err;
+}
+
+int main(int argc, char **argv)
+{
+	int opt;
+
+	while ((opt = getopt_long(argc, argv, "f:", long_options,
+				  NULL)) != -1) {
+		switch (opt) {
+		case 'f':
+			fifoname = optarg;
+			break;
+		default:
+			usage(basename(argv[0]));
+			return 1;
+		}
+	}
+
+	if (optind == argc) {
+		usage(basename(argv[0]));
+		return 1;
+	}
+
+	sysfs_path = argv[optind];
+	if (!sysfs_path) {
+		perror("hidraw");
+		return 1;
+	}
+
+	printf("fifoname: %s\n", fifoname);
+	printf("sysfs_path: %s\n", sysfs_path);
+
+	return attach_progs(argc, argv);
+}
-- 
2.25.1

[RFCv3 7/7] samples: hid: convert USI sample to use unix socket for comms

[Tero Kristo]

Convert USI hid sample userspace interface from using unix fifo to unix
socket. This allows sending results back via the communication channel
also.

Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
---
 samples/bpf/hid_usi_user.c | 73 ++++++++++++++++++++++++++------------
 1 file changed, 50 insertions(+), 23 deletions(-)

diff --git a/samples/bpf/hid_usi_user.c b/samples/bpf/hid_usi_user.c
index b05a3f768835..5bf5ebc21a30 100644
--- a/samples/bpf/hid_usi_user.c
+++ b/samples/bpf/hid_usi_user.c
@@ -14,6 +14,8 @@
 #include <libgen.h>
 #include <sys/resource.h>
 #include <getopt.h>
+#include <sys/socket.h>
+#include <sys/un.h>
 
 #include "bpf_util.h"
 #include <bpf/bpf.h>
@@ -24,15 +26,15 @@
 #include "hid_usi.h"
 
 static char *sysfs_path;
-static char *fifoname = "/tmp/usi";
+static char *sockname = "/tmp/usi";
 static int sysfs_fd;
-static int fifo_fd;
+static int sock_fd;
 static int prog_count;
 static int cache, wr_cache;
 
 static const struct option long_options[] = {
 	{ "help", no_argument, NULL, 'h' },
-	{ "fifo", required_argument, NULL, 'f' },
+	{ "sock", required_argument, NULL, 's' },
 };
 
 struct prog {
@@ -53,15 +55,15 @@ static void int_exit(int sig)
 	}
 
 	close(sysfs_fd);
-	close(fifo_fd);
-	remove(fifoname);
+	close(sock_fd);
+	remove(sockname);
 	exit(0);
 }
 
 static void usage(const char *prog)
 {
 	fprintf(stderr,
-		"usage: %s [-f <fifoname>] /dev/HIDRAW\n\n",
+		"usage: %s [-s <sockname>] /dev/HIDRAW\n\n",
 		prog);
 }
 
@@ -84,15 +86,17 @@ static int write_value(const char *param, int value)
 
 	printf("%s: param=%s (%d), value=%d\n", __func__, param, idx, value);
 	err = bpf_map_update_elem(wr_cache, &idx, &value, BPF_ANY);
-	if (err)
+	if (err) {
 		printf("Update failed for %d, err=%d\n", idx, err);
+		return err;
+	}
 
 	return 0;
 }
 
 static int read_value(const char *param)
 {
-	int value;
+	int value = -ENOENT;
 	int idx = param_to_idx(param);
 
 	printf("%s: param=%s (%d)\n", __func__, param, idx);
@@ -102,7 +106,7 @@ static int read_value(const char *param)
 	else
 		printf("Value for %d = %d\n", idx, value);
 
-	return 0;
+	return value;
 }
 
 static int attach_progs(int argc, char **argv)
@@ -118,6 +122,9 @@ static int attach_progs(int argc, char **argv)
 	char op[8];
 	int value;
 	int m, n;
+	struct sockaddr_un addr;
+	struct sockaddr_un from;
+	socklen_t from_len = sizeof(from);
 
 	snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
 	obj = bpf_object__open_file(filename, NULL);
@@ -177,28 +184,48 @@ static int attach_progs(int argc, char **argv)
 		goto cleanup;
 	}
 
-	mkfifo(fifoname, 0666);
+	sock_fd = socket(PF_UNIX, SOCK_DGRAM, 0);
+	if (sock_fd < 0) {
+		perror("socket open error.\n");
+		err = sock_fd;
+		goto cleanup;
+	}
+
+	addr.sun_family = AF_UNIX;
+	strcpy(addr.sun_path, sockname);
+	unlink(sockname);
 
-	fifo_fd = open(fifoname, O_RDWR);
-	if (fifo_fd < 0) {
-		perror("Fifo open error.\n");
-		err = fifo_fd;
+	if (bind(sock_fd, (struct sockaddr *)&addr, sizeof(addr)) < 0) {
+		perror("bind");
 		goto cleanup;
 	}
 
 	while (1) {
-		n = read(fifo_fd, buf, BUFSIZ);
+		n = recvfrom(sock_fd, buf, BUFSIZ, 0, (struct sockaddr *)&from,
+			     &from_len);
 		if (n < 0)
 			break;
 		buf[n] = 0;
 
 		printf("%s: received '%s'\n", __func__, buf);
 
+		printf("%s: from_len=%d, from=%s\n", __func__, from_len,
+		       from.sun_path);
+
 		m = sscanf(buf, "%16s %8s %d", param, op, &value);
-		if (m == 2 && strcmp(op, "get") == 0)
-			read_value(param);
-		else if (m == 3 && strcmp(op, "set") == 0)
-			write_value(param, value);
+		if (m == 2 && strcmp(op, "get") == 0) {
+			value = read_value(param);
+			sprintf(buf, "%s: %d\n", param, value);
+			printf("%s: sending '%s'\n", __func__, buf);
+			sendto(sock_fd, buf, strlen(buf) + 1, 0,
+			       (struct sockaddr *)&from, from_len);
+		} else if (m == 3 && strcmp(op, "set") == 0) {
+			err = write_value(param, value);
+			sprintf(buf, "%s: %d, err=%d\n", param, value, err);
+			printf("%s: sending '%s'\n", __func__, buf);
+			sendto(sock_fd, buf, strlen(buf) + 1, 0,
+			       (struct sockaddr *)&from, from_len);
+		}
 	}
 
 	return 0;
@@ -217,11 +244,11 @@ int main(int argc, char **argv)
 {
 	int opt;
 
-	while ((opt = getopt_long(argc, argv, "f:", long_options,
+	while ((opt = getopt_long(argc, argv, "s:", long_options,
 				  NULL)) != -1) {
 		switch (opt) {
-		case 'f':
-			fifoname = optarg;
+		case 's':
+			sockname = optarg;
 			break;
 		default:
 			usage(basename(argv[0]));
@@ -240,7 +267,7 @@ int main(int argc, char **argv)
 		return 1;
 	}
 
-	printf("fifoname: %s\n", fifoname);
+	printf("sockname: %s\n", sockname);
 	printf("sysfs_path: %s\n", sysfs_path);
 
 	return attach_progs(argc, argv);
-- 
2.25.1

Re: [RFCv3 1/7] HID: Add map_msc() to avoid boilerplate code

[Benjamin Tissoires]

On Wed, Dec 1, 2021 at 5:43 PM Tero Kristo <tero.kristo@linux.intel.com> wrote:
>
> 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>

This one looks good to me and I should be able to take it in v5.17.

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

Cheers,
Benjamin

> ---
>  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 2c72ce4147b1..39ebedb2323b 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))
> @@ -872,10 +873,7 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
>
>                 case 0x5b: /* TransducerSerialNumber */
>                 case 0x6e: /* TransducerSerialNumber2 */
> -                       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 e95800bab56a..96eaca0d5322 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -766,6 +766,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
>

Re: [RFCv3 2/7] HID: hid-input: Add suffix also for HID_DG_PEN

[Benjamin Tissoires]

On Wed, Dec 1, 2021 at 5:43 PM Tero Kristo <tero.kristo@linux.intel.com> wrote:
>
> 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>
> ---
>  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 39ebedb2323b..73c2edda742e 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1737,6 +1737,7 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid,
>                 case HID_GD_MOUSE:
>                         suffix = "Mouse";
>                         break;
> +               case HID_DG_PEN:

hid-multitouch overrides this in mt_input_configured() and sets
"Stylus" instead.
A common code would be best, but now I realizes that this would mean:
HID_DG_PEN -> "Stylus"
HID_DG_STYLUS -> "Pen"

This is part of the configuration API, because we can have
configuration snippets based on the device name :)

So ideally, we need:
this patch to add DG_PEN support in hid-input.c and remove the special
case from hid-multitouch, with a comment explaining the Pen/Stylus
mistake :)

Cheers,
Benjamin


>                 case HID_DG_STYLUS:
>                         suffix = "Pen";
>                         break;
> --
> 2.25.1
>

Re: [RFCv3 3/7] HID: core: Add support for USI style events

[Benjamin Tissoires]

On Wed, Dec 1, 2021 at 5:43 PM Tero Kristo <tero.kristo@linux.intel.com> wrote:
>
> Add support for Universal Stylus Interface (USI) style events to the HID
> core and input layers.
>
> Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
> ---
>  drivers/hid/hid-input.c                | 18 ++++++++++++++++++
>  include/linux/mod_devicetable.h        |  2 +-
>  include/uapi/linux/hid.h               | 10 ++++++++++
>  include/uapi/linux/input-event-codes.h | 22 ++++++++++++++--------
>  4 files changed, 43 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 73c2edda742e..b428ee9b4d9b 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);

This is the new slot version for pens, I am not sure we really want to
blindly introduce that without testing.

Do you have a panel that supports multiple values (at once) for this
usage (2 pens???). If not, I would simply skip that usage until we get
an actual hardware that makes use of it.

> +                       break;
> +
>                 case 0x3b: /* Battery Strength */
>                         hidinput_setup_battery(device, HID_INPUT_REPORT, field, false);
>                         usage->type = EV_PWR;
> @@ -876,6 +880,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;

We already have ABS_TOOL_WIDTH which seems to relate to that very closely.

> +
> +               case 0x70:
> +               case 0x71:
> +               case 0x72:
> +               case 0x73:
> +               case 0x74:
> +               case 0x75:
> +               case 0x76:
> +               case 0x77:
> +                       map_msc(MSC_PEN_LINE_STYLE);

Nope, this is wrong. It took me a long time to understand the report
descriptor (see my last reply to your v2).
Basically, we need one input event for each value between 0x72 and
0x77. HID core should translate the array of 1 element into a set of
{depressed usage, pressed usage} and from the user-space, we will get
"MSC_PEN_STYLE_CHISEL_MARKER 1" for instance.

And *maybe* we could even use KEY events there, so we could remap them
(but that's a very distant maybe).

> +                       break;
> +
>                 default:  goto unknown;
>                 }
>                 break;
> 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/hid.h b/include/uapi/linux/hid.h
> index 861bfbbfc565..60ef9b615a1a 100644
> --- a/include/uapi/linux/hid.h
> +++ b/include/uapi/linux/hid.h
> @@ -255,6 +255,7 @@
>  #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
> @@ -267,6 +268,15 @@
>  #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_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

Could you integrate that patch before my patch "HID: export the
various HID defines from the spec in the uapi"? And also have it as a
separate patch from the mapping?
This way I can schedule that part earlier before we settle on the
actual user space usages.

>
>  #define HID_CP_CONSUMERCONTROL                 0x000c0001
>  #define HID_CP_NUMERICKEYPAD                   0x000c0002
> 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

PEN_LINE_WIDTH should be dropped.

> +#define MSC_PEN_LINE_STYLE             0x09

Again, PEN_LINE_STYLE is HID only and we should only map the values,
not the title of the array.

Cheers,
Benjamin

> +/* TODO: Add USI diagnostic & battery events too */
> +#define MSC_MAX                                0x09
> +#define MSC_CNT                                (MSC_MAX + 1)
>
>  /*
>   * LEDs
> --
> 2.25.1
>

Re: [RFCv3 4/7] HID: input: Make hidinput_find_field() static

[Benjamin Tissoires]

On Wed, Dec 1, 2021 at 5:43 PM Tero Kristo <tero.kristo@linux.intel.com> wrote:
>
> 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>

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

Cheers,
Benjamin

> ---
>  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 b428ee9b4d9b..f6332d269d49 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1477,7 +1477,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;
> @@ -1492,7 +1493,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 96eaca0d5322..3f1fd4fcf1a9 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -636,7 +636,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
>

Re: [RFCv3 0/7] USI stylus support series

[Benjamin Tissoires]

Hi Tero,

On Wed, Dec 1, 2021 at 5:43 PM Tero Kristo <tero.kristo@linux.intel.com> wrote:
>
> Hi,
>
> Another update here based on received feedback. Main change compared to v2:

If that's OK with you, I'd like to cherry-pick some patches from the
series: 1/7, 2/7 (the version from v4 because I requested changes),
4/7 and a lighter 5/7 where we don't have the MSC events we are still
discussing.

So Ideally, can you post a v4 based on top of hid.git/for-next
(without my hid-bpf changes) with those 4 patches?

Patch 3 is still up for discussion, and patches 6 and 7 are obviously RFC.

Actually, Patch 3 could be split into a part where you add the HID
usages and a part with the mapping of them. The HID usages part can be
integrated now too, and we'll have the USI mapping that would require
Dmitry's ack in a separate patch.

But if you prefer having everything in one series, that's fine by me too.

Cheers,
Benjamin

>
> - Dropped patch #5 : HID: core: map USI pen style reports directly
>   This is not needed anymore, as the same result can be reached by
>   modifying the flags of the relevant field in rdesc. This is done now
>   as part of patch #7 in the BPF code.
>
> I also dropped one of the fixes from my test branch [1] as pointed out
> by Benjamin, it is not needed if the BPF program is executed with the
> modalias link.
>
> Updated test branch can be found from [2].
>
> -Tero
>
> [1]
> https://github.com/t-kristo/linux/commit/81b27fd46780ce67c2706d586c0f4a437e87dbf6
> (HID: bpf: fix file mapping)
> [2] https://github.com/t-kristo/linux/commits/usi-5.16-rfc-v3-bpf
>
>

Re: [RFCv3 3/7] HID: core: Add support for USI style events

[Benjamin Tissoires]

On 12/8/21 16:18, Benjamin Tissoires wrote:
> On Wed, Dec 1, 2021 at 5:43 PM Tero Kristo <tero.kristo@linux.intel.com> wrote:
>>
>> Add support for Universal Stylus Interface (USI) style events to the HID
>> core and input layers.
>>
>> Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
>> ---
>>   drivers/hid/hid-input.c                | 18 ++++++++++++++++++
>>   include/linux/mod_devicetable.h        |  2 +-
>>   include/uapi/linux/hid.h               | 10 ++++++++++
>>   include/uapi/linux/input-event-codes.h | 22 ++++++++++++++--------
>>   4 files changed, 43 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
>> index 73c2edda742e..b428ee9b4d9b 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);
> 
> This is the new slot version for pens, I am not sure we really want to
> blindly introduce that without testing.
> 
> Do you have a panel that supports multiple values (at once) for this
> usage (2 pens???). If not, I would simply skip that usage until we get
> an actual hardware that makes use of it.
> 
>> +                       break;
>> +
>>                  case 0x3b: /* Battery Strength */
>>                          hidinput_setup_battery(device, HID_INPUT_REPORT, field, false);
>>                          usage->type = EV_PWR;
>> @@ -876,6 +880,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;
> 
> We already have ABS_TOOL_WIDTH which seems to relate to that very closely.
> 
>> +
>> +               case 0x70:
>> +               case 0x71:
>> +               case 0x72:
>> +               case 0x73:
>> +               case 0x74:
>> +               case 0x75:
>> +               case 0x76:
>> +               case 0x77:
>> +                       map_msc(MSC_PEN_LINE_STYLE);
> 
> Nope, this is wrong. It took me a long time to understand the report
> descriptor (see my last reply to your v2).
> Basically, we need one input event for each value between 0x72 and
> 0x77. HID core should translate the array of 1 element into a set of
> {depressed usage, pressed usage} and from the user-space, we will get
> "MSC_PEN_STYLE_CHISEL_MARKER 1" for instance.

This seems to be working (on top of this RFC):
---
diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index cec0470e5485..18035c63c358 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -1027,8 +1027,12 @@ static const char *misc[MSC_MAX + 1] = {
  	[MSC_SERIAL] = "Serial",	[MSC_PULSELED] = "Pulseled",
  	[MSC_GESTURE] = "Gesture",	[MSC_RAW] = "RawData",
  	[MSC_PEN_ID] = "PenID",		[MSC_PEN_COLOR] "PenColor",
-	[MSC_PEN_LINE_WIDTH] = "PenLineWidth",
-	[MSC_PEN_LINE_STYLE] = "PenLineStyle",
+	[MSC_PEN_LINE_STYLE_INK] = "PenLineStyleInk",
+	[MSC_PEN_LINE_STYLE_PENCIL] = "PenLineStylePencil",
+	[MSC_PEN_LINE_STYLE_HIGHLIGHTER] = "PenLineStyleHighlighter",
+	[MSC_PEN_LINE_STYLE_CHISEL_MARKER] = "PenLineStyleChiselMarker",
+	[MSC_PEN_LINE_STYLE_BRUSH] = "PenLineStyleBrush",
+	[MSC_PEN_LINE_STYLE_NO_PREFERENCE] = "PenLineStyleNoPreference",
  };
  
  static const char *leds[LED_MAX + 1] = {
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 079e67c5168f..837585f4e673 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -832,8 +832,7 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
  			break;
  
  		case 0x38: /* Transducer Index */
-			map_msc(MSC_PEN_ID);
-			break;
+			goto ignore;
  
  		case 0x3b: /* Battery Strength */
  			hidinput_setup_battery(device, HID_INPUT_REPORT, field, false);
@@ -882,18 +881,36 @@ 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);
+		case 0x5c: /* Digitizer Preferred Color */
+			map_msc(MSC_PEN_COLOR);
+			break;
+
+		case 0x5e: /* Digitizer Preferred Line Width */
+			map_abs_clear(ABS_TOOL_WIDTH);
+			break;
+
+
+		case 0x70: /* Preferred Line Style -> not an input usage*/
+		case 0x71: /* Preferred Line Style is Locked */
+			goto ignore;
+
+		case 0x72: /* Ink */
+			map_msc(MSC_PEN_LINE_STYLE_INK);
+			break;
+		case 0x73: /* Pencil */
+			map_msc(MSC_PEN_LINE_STYLE_PENCIL);
+			break;
+		case 0x74: /* Highlighter */
+			map_msc(MSC_PEN_LINE_STYLE_HIGHLIGHTER);
+			break;
+		case 0x75: /* Chisel Marker */
+			map_msc(MSC_PEN_LINE_STYLE_CHISEL_MARKER);
+			break;
+		case 0x76: /* Brush */
+			map_msc(MSC_PEN_LINE_STYLE_BRUSH);
+			break;
+		case 0x77: /* No Preference */
+			map_msc(MSC_PEN_LINE_STYLE_NO_PREFERENCE);
  			break;
  
  		default:  goto unknown;
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index a76d37082ab5..bf2b76a6e7e8 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		0x0d
  #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..ff705245b7ae 100644
--- a/include/uapi/linux/input-event-codes.h
+++ b/include/uapi/linux/input-event-codes.h
@@ -908,12 +908,16 @@
  #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
+#define MSC_PEN_ID				0x06
+#define MSC_PEN_COLOR				0x07
+#define MSC_PEN_LINE_STYLE_INK			0x08
+#define MSC_PEN_LINE_STYLE_PENCIL		0x09
+#define MSC_PEN_LINE_STYLE_HIGHLIGHTER		0x0a
+#define MSC_PEN_LINE_STYLE_CHISEL_MARKER	0x0b
+#define MSC_PEN_LINE_STYLE_BRUSH		0x0c
+#define MSC_PEN_LINE_STYLE_NO_PREFERENCE	0x0d
  /* TODO: Add USI diagnostic & battery events too */
-#define MSC_MAX				0x09
+#define MSC_MAX				0x0d
  #define MSC_CNT				(MSC_MAX + 1)
  
  /*
---

Cheers,
Benjamin

> 
> And *maybe* we could even use KEY events there, so we could remap them
> (but that's a very distant maybe).
> 
>> +                       break;
>> +
>>                  default:  goto unknown;
>>                  }
>>                  break;
>> 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/hid.h b/include/uapi/linux/hid.h
>> index 861bfbbfc565..60ef9b615a1a 100644
>> --- a/include/uapi/linux/hid.h
>> +++ b/include/uapi/linux/hid.h
>> @@ -255,6 +255,7 @@
>>   #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
>> @@ -267,6 +268,15 @@
>>   #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_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
> 
> Could you integrate that patch before my patch "HID: export the
> various HID defines from the spec in the uapi"? And also have it as a
> separate patch from the mapping?
> This way I can schedule that part earlier before we settle on the
> actual user space usages.
> 
>>
>>   #define HID_CP_CONSUMERCONTROL                 0x000c0001
>>   #define HID_CP_NUMERICKEYPAD                   0x000c0002
>> 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
> 
> PEN_LINE_WIDTH should be dropped.
> 
>> +#define MSC_PEN_LINE_STYLE             0x09
> 
> Again, PEN_LINE_STYLE is HID only and we should only map the values,
> not the title of the array.
> 
> Cheers,
> Benjamin
> 
>> +/* TODO: Add USI diagnostic & battery events too */
>> +#define MSC_MAX                                0x09
>> +#define MSC_CNT                                (MSC_MAX + 1)
>>
>>   /*
>>    * LEDs
>> --
>> 2.25.1
>>

Re: [RFCv3 0/7] USI stylus support series

[Tero Kristo]

Hi Benjamin,

On 08/12/2021 17:30, Benjamin Tissoires wrote:
> Hi Tero,
>
> On Wed, Dec 1, 2021 at 5:43 PM Tero Kristo <tero.kristo@linux.intel.com> wrote:
>> Hi,
>>
>> Another update here based on received feedback. Main change compared to v2:
> If that's OK with you, I'd like to cherry-pick some patches from the
> series: 1/7, 2/7 (the version from v4 because I requested changes),
> 4/7 and a lighter 5/7 where we don't have the MSC events we are still
> discussing.
>
> So Ideally, can you post a v4 based on top of hid.git/for-next
> (without my hid-bpf changes) with those 4 patches?

Yes, I can post these.

>
> Patch 3 is still up for discussion, and patches 6 and 7 are obviously RFC.
>
> Actually, Patch 3 could be split into a part where you add the HID
> usages and a part with the mapping of them. The HID usages part can be
> integrated now too, and we'll have the USI mapping that would require
> Dmitry's ack in a separate patch.
Ok, I'll take a look at this part also.
>
> But if you prefer having everything in one series, that's fine by me too.

No, I am fine with splitting things up and start getting things integrated.

-Tero

>
> Cheers,
> Benjamin
>
>> - Dropped patch #5 : HID: core: map USI pen style reports directly
>>    This is not needed anymore, as the same result can be reached by
>>    modifying the flags of the relevant field in rdesc. This is done now
>>    as part of patch #7 in the BPF code.
>>
>> I also dropped one of the fixes from my test branch [1] as pointed out
>> by Benjamin, it is not needed if the BPF program is executed with the
>> modalias link.
>>
>> Updated test branch can be found from [2].
>>
>> -Tero
>>
>> [1]
>> https://github.com/t-kristo/linux/commit/81b27fd46780ce67c2706d586c0f4a437e87dbf6
>> (HID: bpf: fix file mapping)
>> [2] https://github.com/t-kristo/linux/commits/usi-5.16-rfc-v3-bpf
>>
>>

Re: [RFCv3 3/7] HID: core: Add support for USI style events

[Tero Kristo]

Hi Benjamin,

On 08/12/2021 17:18, Benjamin Tissoires wrote:
> On Wed, Dec 1, 2021 at 5:43 PM Tero Kristo <tero.kristo@linux.intel.com> wrote:
>> Add support for Universal Stylus Interface (USI) style events to the HID
>> core and input layers.
>>
>> Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
>> ---
>>   drivers/hid/hid-input.c                | 18 ++++++++++++++++++
>>   include/linux/mod_devicetable.h        |  2 +-
>>   include/uapi/linux/hid.h               | 10 ++++++++++
>>   include/uapi/linux/input-event-codes.h | 22 ++++++++++++++--------
>>   4 files changed, 43 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
>> index 73c2edda742e..b428ee9b4d9b 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);
> This is the new slot version for pens, I am not sure we really want to
> blindly introduce that without testing.
>
> Do you have a panel that supports multiple values (at once) for this
> usage (2 pens???). If not, I would simply skip that usage until we get
> an actual hardware that makes use of it.
Afaik, current controllers (panels) only support single pen at a time. 
You can use multiple pens with them and they effectively re-use the same 
pen-id, but you can't use them at the same time.
>
>> +                       break;
>> +
>>                  case 0x3b: /* Battery Strength */
>>                          hidinput_setup_battery(device, HID_INPUT_REPORT, field, false);
>>                          usage->type = EV_PWR;
>> @@ -876,6 +880,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;
> We already have ABS_TOOL_WIDTH which seems to relate to that very closely.
Ok, I can switch to use this one.
>
>> +
>> +               case 0x70:
>> +               case 0x71:
>> +               case 0x72:
>> +               case 0x73:
>> +               case 0x74:
>> +               case 0x75:
>> +               case 0x76:
>> +               case 0x77:
>> +                       map_msc(MSC_PEN_LINE_STYLE);
> Nope, this is wrong. It took me a long time to understand the report
> descriptor (see my last reply to your v2).
> Basically, we need one input event for each value between 0x72 and
> 0x77. HID core should translate the array of 1 element into a set of
> {depressed usage, pressed usage} and from the user-space, we will get
> "MSC_PEN_STYLE_CHISEL_MARKER 1" for instance.
>
> And *maybe* we could even use KEY events there, so we could remap them
> (but that's a very distant maybe).
>
>> +                       break;
>> +
>>                  default:  goto unknown;
>>                  }
>>                  break;
>> 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/hid.h b/include/uapi/linux/hid.h
>> index 861bfbbfc565..60ef9b615a1a 100644
>> --- a/include/uapi/linux/hid.h
>> +++ b/include/uapi/linux/hid.h
>> @@ -255,6 +255,7 @@
>>   #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
>> @@ -267,6 +268,15 @@
>>   #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_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
> Could you integrate that patch before my patch "HID: export the
> various HID defines from the spec in the uapi"? And also have it as a
> separate patch from the mapping?
> This way I can schedule that part earlier before we settle on the
> actual user space usages.
>
>>   #define HID_CP_CONSUMERCONTROL                 0x000c0001
>>   #define HID_CP_NUMERICKEYPAD                   0x000c0002
>> 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
> PEN_LINE_WIDTH should be dropped.
>
>> +#define MSC_PEN_LINE_STYLE             0x09
> Again, PEN_LINE_STYLE is HID only and we should only map the values,
> not the title of the array.

Ok will update this, thanks.

-Tero

>
> Cheers,
> Benjamin
>
>> +/* TODO: Add USI diagnostic & battery events too */
>> +#define MSC_MAX                                0x09
>> +#define MSC_CNT                                (MSC_MAX + 1)
>>
>>   /*
>>    * LEDs
>> --
>> 2.25.1
>>