[PATCH v10 00/12] Add Wacom I2C support to rM2

[PATCH v10 00/12] Add Wacom I2C support to rM2

[Alistair Francis]

Add support to the reMarkable2 (rM2) for the Wacom I2C device.

This is based on the reMarkable Linux fork and with this series I am
able to use the Wacom digitiser on the rM2.

v10:
 - Add a new patch to determine the generation
 - Use generation to determine if tilt is supported
 - Address comments from v9
 - Remove flip-pressure
v9:
 - Add two new patches
 - Fixup the device tree interrupt line
v7:
 - Fx the compatible name and documentation

Alistair Francis (12):
  dt-bindings: Add Wacom to vendor bindings
  dt-bindings: touchscreen: Initial commit of wacom,i2c
  Input: wacom_i2c - Add device tree support to wacom_i2c
  Input: wacom_i2c - Add touchscren properties
  Input: wacom_i2c - Read the descriptor values
  Input: wacom_i2c - Add support for distance and tilt x/y
  Input: wacom_i2c - Clean up the query device fields
  Input: wacom_i2c - Add support for vdd regulator
  Input: wacom_i2c - Use macros for the bit masks
  Input: wacom_i2c - Allow flipping the values from the DT
  ARM: imx_v6_v7_defconfig: Enable Wacom I2C
  ARM: dts: imx7d: remarkable2: add wacom digitizer device

 .../input/touchscreen/wacom,generic.yaml      |  68 +++++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 arch/arm/boot/dts/imx7d-remarkable2.dts       |  61 ++++
 arch/arm/configs/imx_v6_v7_defconfig          |   1 +
 drivers/input/touchscreen/wacom_i2c.c         | 262 +++++++++++++++---
 5 files changed, 358 insertions(+), 36 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/wacom,generic.yaml

-- 
2.31.1

[PATCH v10 01/12] dt-bindings: Add Wacom to vendor bindings

[Alistair Francis]

Signed-off-by: Alistair Francis <alistair@alistair23.me>
Acked-by: Rob Herring <robh@kernel.org>
Cc: devicetree@vger.kernel.org
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 4dfaae537daf..31745c45dd92 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -1252,6 +1252,8 @@ patternProperties:
     description: Vision Optical Technology Co., Ltd.
   "^vxt,.*":
     description: VXT Ltd
+  "^wacom,.*":
+    description: Wacom Co., Ltd
   "^wand,.*":
     description: Wandbord (Technexion)
   "^waveshare,.*":
-- 
2.31.1

[PATCH] Input: wacom_i2c - Allow flipping the values from the DT

[Alistair Francis]

Allow the device tree properties to flip the tilx, position or distance
values.

This is required for the stylus to work correctly on the reMarkable 2.

Signed-off-by: Alistair Francis <alistair@alistair23.me>
---
 .../input/touchscreen/wacom,generic.yaml      | 20 +++++++++++
 drivers/input/touchscreen/wacom_i2c.c         | 36 +++++++++++++++++++
 2 files changed, 56 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/wacom,generic.yaml b/Documentation/devicetree/bindings/input/touchscreen/wacom,generic.yaml
index a8a7f362b0ce..d451c9a235b6 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/wacom,generic.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/wacom,generic.yaml
@@ -25,6 +25,26 @@ properties:
   vdd-supply:
     description: Power Supply
 
+  flip-tilt-x:
+    type: boolean
+    description: Flip the tilt X values read from device
+
+  flip-tilt-y:
+    type: boolean
+    description: Flip the tilt Y values read from device
+
+  flip-pos-x:
+    type: boolean
+    description: Flip the X position values read from device
+
+  flip-pos-y:
+    type: boolean
+    description: Flip the Y position values read from device
+
+  flip-distance:
+    type: boolean
+    description: Flip the distance values read from device
+
 required:
   - compatible
   - reg
diff --git a/drivers/input/touchscreen/wacom_i2c.c b/drivers/input/touchscreen/wacom_i2c.c
index f78a43212901..903353106ca8 100644
--- a/drivers/input/touchscreen/wacom_i2c.c
+++ b/drivers/input/touchscreen/wacom_i2c.c
@@ -105,6 +105,12 @@ struct wacom_i2c {
 	u8 data[WACOM_QUERY_SIZE];
 	bool prox;
 	int tool;
+
+	bool flip_tilt_x;
+	bool flip_tilt_y;
+	bool flip_pos_x;
+	bool flip_pos_y;
+	bool flip_distance;
 };
 
 static int wacom_query_device(struct i2c_client *client,
@@ -198,6 +204,19 @@ static int wacom_query_device(struct i2c_client *client,
 	return 0;
 }
 
+#ifdef CONFIG_OF
+static void wacom_of_read(struct wacom_i2c *wac_i2c)
+{
+	struct i2c_client *client = wac_i2c->client;
+
+	wac_i2c->flip_tilt_x = of_property_read_bool(client->dev.of_node, "flip-tilt-x");
+	wac_i2c->flip_tilt_y = of_property_read_bool(client->dev.of_node, "flip-tilt-y");
+	wac_i2c->flip_pos_x = of_property_read_bool(client->dev.of_node, "flip-pos-x");
+	wac_i2c->flip_pos_y = of_property_read_bool(client->dev.of_node, "flip-pos-y");
+	wac_i2c->flip_distance = of_property_read_bool(client->dev.of_node, "flip-distance");
+}
+#endif
+
 static irqreturn_t wacom_i2c_irq(int irq, void *dev_id)
 {
 	struct wacom_i2c *wac_i2c = dev_id;
@@ -234,6 +253,13 @@ static irqreturn_t wacom_i2c_irq(int irq, void *dev_id)
 
 	wac_i2c->prox = data[3] & 0x20;
 
+	// Flip the values based on properties from the device tree
+	distance = wac_i2c->flip_distance ? -distance : distance;
+	x = wac_i2c->flip_pos_x ? (features->x_max - x) : x;
+	y = wac_i2c->flip_pos_y ? (features->y_max - y) : y;
+	tilt_x = wac_i2c->flip_tilt_x ? -tilt_x : tilt_x;
+	tilt_y = wac_i2c->flip_tilt_y ? -tilt_y : tilt_y;
+
 	touchscreen_report_pos(input, &wac_i2c->props, features->x_max,
 			       features->y_max, true);
 	input_report_key(input, BTN_TOUCH, tsw || ers);
@@ -361,6 +387,16 @@ static int wacom_i2c_probe(struct i2c_client *client,
 		return error;
 	}
 
+#ifdef CONFIG_OF
+	wacom_of_read(wac_i2c);
+#else
+	wac_i2c->flip_tilt_x = false;
+	wac_i2c->flip_tilt_y = false;
+	wac_i2c->flip_pos_x = false;
+	wac_i2c->flip_pos_y = false;
+	wac_i2c->flip_distance = false;
+#endif
+
 	return 0;
 }
 
-- 
2.31.1

[PATCH v10 02/12] dt-bindings: touchscreen: Initial commit of wacom,i2c

[Alistair Francis]

Signed-off-by: Alistair Francis <alistair@alistair23.me>
Reviewed-by: Rob Herring <robh@kernel.org>
Cc: devicetree@vger.kernel.org
---
 .../input/touchscreen/wacom,generic.yaml      | 48 +++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/wacom,generic.yaml

diff --git a/Documentation/devicetree/bindings/input/touchscreen/wacom,generic.yaml b/Documentation/devicetree/bindings/input/touchscreen/wacom,generic.yaml
new file mode 100644
index 000000000000..a8a7f362b0ce
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/wacom,generic.yaml
@@ -0,0 +1,48 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/touchscreen/wacom,generic.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Wacom I2C Controller
+
+maintainers:
+  - Alistair Francis <alistair@alistair23.me>
+
+allOf:
+  - $ref: touchscreen.yaml#
+
+properties:
+  compatible:
+    const: wacom,i2c-30
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  vdd-supply:
+    description: Power Supply
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include "dt-bindings/interrupt-controller/irq.h"
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        digitiser@9 {
+                compatible = "wacom,i2c-30";
+                reg = <0x9>;
+                interrupt-parent = <&gpio1>;
+                interrupts = <9 IRQ_TYPE_LEVEL_LOW>;
+                vdd-supply = <&reg_touch>;
+        };
+    };
-- 
2.31.1

[PATCH v10 03/12] Input: wacom_i2c - Add device tree support to wacom_i2c

[Alistair Francis]

Allow the wacom-i2c device to be exposed via device tree.

Signed-off-by: Alistair Francis <alistair@alistair23.me>
---
 drivers/input/touchscreen/wacom_i2c.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/input/touchscreen/wacom_i2c.c b/drivers/input/touchscreen/wacom_i2c.c
index 22826c387da5..6053595f2b30 100644
--- a/drivers/input/touchscreen/wacom_i2c.c
+++ b/drivers/input/touchscreen/wacom_i2c.c
@@ -12,6 +12,7 @@
 #include <linux/slab.h>
 #include <linux/irq.h>
 #include <linux/interrupt.h>
+#include <linux/of.h>
 #include <asm/unaligned.h>
 
 #define WACOM_CMD_QUERY0	0x04
@@ -241,10 +242,17 @@ static const struct i2c_device_id wacom_i2c_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, wacom_i2c_id);
 
+static const struct of_device_id wacom_i2c_of_match_table[] = {
+	{ .compatible = "wacom,i2c-30" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, wacom_i2c_of_match_table);
+
 static struct i2c_driver wacom_i2c_driver = {
 	.driver	= {
 		.name	= "wacom_i2c",
 		.pm	= &wacom_i2c_pm,
+		.of_match_table = wacom_i2c_of_match_table,
 	},
 
 	.probe		= wacom_i2c_probe,
-- 
2.31.1

[PATCH v10 04/12] Input: wacom_i2c - Add touchscren properties

[Alistair Francis]

Connect touchscreen properties to the wacom_i2c.

Signed-off-by: Alistair Francis <alistair@alistair23.me>
---
 drivers/input/touchscreen/wacom_i2c.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/input/touchscreen/wacom_i2c.c b/drivers/input/touchscreen/wacom_i2c.c
index 6053595f2b30..28255c77d426 100644
--- a/drivers/input/touchscreen/wacom_i2c.c
+++ b/drivers/input/touchscreen/wacom_i2c.c
@@ -11,6 +11,7 @@
 #include <linux/i2c.h>
 #include <linux/slab.h>
 #include <linux/irq.h>
+#include <linux/input/touchscreen.h>
 #include <linux/interrupt.h>
 #include <linux/of.h>
 #include <asm/unaligned.h>
@@ -33,6 +34,8 @@ struct wacom_features {
 struct wacom_i2c {
 	struct i2c_client *client;
 	struct input_dev *input;
+	struct touchscreen_properties props;
+	struct wacom_features features;
 	u8 data[WACOM_QUERY_SIZE];
 	bool prox;
 	int tool;
@@ -90,6 +93,7 @@ static irqreturn_t wacom_i2c_irq(int irq, void *dev_id)
 {
 	struct wacom_i2c *wac_i2c = dev_id;
 	struct input_dev *input = wac_i2c->input;
+	struct wacom_features *features = &wac_i2c->features;
 	u8 *data = wac_i2c->data;
 	unsigned int x, y, pressure;
 	unsigned char tsw, f1, f2, ers;
@@ -114,6 +118,8 @@ static irqreturn_t wacom_i2c_irq(int irq, void *dev_id)
 
 	wac_i2c->prox = data[3] & 0x20;
 
+	touchscreen_report_pos(input, &wac_i2c->props, features->x_max,
+			       features->y_max, true);
 	input_report_key(input, BTN_TOUCH, tsw || ers);
 	input_report_key(input, wac_i2c->tool, wac_i2c->prox);
 	input_report_key(input, BTN_STYLUS, f1);
@@ -151,7 +157,7 @@ static int wacom_i2c_probe(struct i2c_client *client,
 	struct device *dev = &client->dev;
 	struct wacom_i2c *wac_i2c;
 	struct input_dev *input;
-	struct wacom_features features = { 0 };
+	struct wacom_features *features;
 	int error;
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
@@ -159,14 +165,15 @@ static int wacom_i2c_probe(struct i2c_client *client,
 		return -EIO;
 	}
 
-	error = wacom_query_device(client, &features);
-	if (error)
-		return error;
-
 	wac_i2c = devm_kzalloc(dev, sizeof(*wac_i2c), GFP_KERNEL);
 	if (!wac_i2c)
 		return -ENOMEM;
 
+	features = &wac_i2c->features;
+	error = wacom_query_device(client, features);
+	if (error)
+		return error;
+
 	wac_i2c->client = client;
 
 	input = devm_input_allocate_device(dev);
@@ -178,7 +185,7 @@ static int wacom_i2c_probe(struct i2c_client *client,
 	input->name = "Wacom I2C Digitizer";
 	input->id.bustype = BUS_I2C;
 	input->id.vendor = 0x56a;
-	input->id.version = features.fw_version;
+	input->id.version = features->fw_version;
 	input->open = wacom_i2c_open;
 	input->close = wacom_i2c_close;
 
@@ -190,10 +197,11 @@ static int wacom_i2c_probe(struct i2c_client *client,
 	__set_bit(BTN_STYLUS2, input->keybit);
 	__set_bit(BTN_TOUCH, input->keybit);
 
-	input_set_abs_params(input, ABS_X, 0, features.x_max, 0, 0);
-	input_set_abs_params(input, ABS_Y, 0, features.y_max, 0, 0);
+	touchscreen_parse_properties(input, true, &wac_i2c->props);
+	input_set_abs_params(input, ABS_X, 0, features->x_max, 0, 0);
+	input_set_abs_params(input, ABS_Y, 0, features->y_max, 0, 0);
 	input_set_abs_params(input, ABS_PRESSURE,
-			     0, features.pressure_max, 0, 0);
+			     0, features->pressure_max, 0, 0);
 
 	input_set_drvdata(input, wac_i2c);
 
-- 
2.31.1

[PATCH v10 05/12] Input: wacom_i2c - Read the descriptor values

[Alistair Francis]

When we query the device let's also read the descriptor from the device.
This tells us useful information, including the version, which we use to
determine a generation.

This is based on the driver from Wacom.

Signed-off-by: Alistair Francis <alistair@alistair23.me>
---
 drivers/input/touchscreen/wacom_i2c.c | 64 +++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/drivers/input/touchscreen/wacom_i2c.c b/drivers/input/touchscreen/wacom_i2c.c
index 28255c77d426..72ba4a36459b 100644
--- a/drivers/input/touchscreen/wacom_i2c.c
+++ b/drivers/input/touchscreen/wacom_i2c.c
@@ -16,6 +16,7 @@
 #include <linux/of.h>
 #include <asm/unaligned.h>
 
+#define WACOM_DESC_REG	0x01
 #define WACOM_CMD_QUERY0	0x04
 #define WACOM_CMD_QUERY1	0x00
 #define WACOM_CMD_QUERY2	0x33
@@ -24,11 +25,46 @@
 #define WACOM_CMD_THROW1	0x00
 #define WACOM_QUERY_SIZE	19
 
+#define WACOM_MAX_DATA_SIZE_BG9     10
+#define WACOM_MAX_DATA_SIZE_G12     15
+#define WACOM_MAX_DATA_SIZE_AG14    17
+#define WACOM_MAX_DATA_SIZE         22
+
+/* Generation selction */
+/* Before and at G9 generation */
+#define WACOM_BG9	0
+/* G12 generation the IC supports "height"*/
+#define WACOM_G12	1
+/* After and at G14 generation the IC supports "height" and
+ * it is defined as "Z" axis
+ */
+#define WACOM_AG14	2
+
+struct wacom_desc {
+	u16 descLen;
+	u16 version;
+	u16 reportLen;
+	u16 reportReg;
+	u16 inputReg;
+	u16 maxInputLen;
+	u16 outputReg;
+	u16 maxOutputLen;
+	u16 commReg;
+	u16 dataReg;
+	u16 vendorID;
+	u16 productID;
+	u16 fwVersion;
+	u16 misc_high;
+	u16 misc_low;
+};
+
 struct wacom_features {
+	struct wacom_desc desc;
 	int x_max;
 	int y_max;
 	int pressure_max;
 	char fw_version;
+	unsigned char generation;
 };
 
 struct wacom_i2c {
@@ -45,6 +81,7 @@ static int wacom_query_device(struct i2c_client *client,
 			      struct wacom_features *features)
 {
 	int ret;
+	u8 cmd_wac_desc[] = {WACOM_DESC_REG, 0x00};
 	u8 cmd1[] = { WACOM_CMD_QUERY0, WACOM_CMD_QUERY1,
 			WACOM_CMD_QUERY2, WACOM_CMD_QUERY3 };
 	u8 cmd2[] = { WACOM_CMD_THROW0, WACOM_CMD_THROW1 };
@@ -70,6 +107,33 @@ static int wacom_query_device(struct i2c_client *client,
 		},
 	};
 
+	/* Read the description register */
+	ret = i2c_master_send(client, cmd_wac_desc, sizeof(cmd_wac_desc));
+	if (ret < 0)
+		return ret;
+	ret = i2c_master_recv(client, (char *)&features->desc, sizeof(features->desc));
+	if (ret < 0)
+		return ret;
+
+	switch (features->desc.maxInputLen) {
+	case WACOM_MAX_DATA_SIZE_BG9:
+		features->generation = WACOM_BG9;
+		break;
+
+	case WACOM_MAX_DATA_SIZE_G12:
+		features->generation = WACOM_G12;
+		break;
+
+	case WACOM_MAX_DATA_SIZE_AG14:
+		features->generation = WACOM_AG14;
+		break;
+
+	default:
+		/* Cover all generations possible */
+		features->generation = WACOM_AG14;
+		break;
+	}
+
 	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
 	if (ret < 0)
 		return ret;
-- 
2.31.1

Re: [PATCH v10 05/12] Input: wacom_i2c - Read the descriptor values

[Dmitry Torokhov]

On Sun, Aug 29, 2021 at 07:19:18PM +1000, Alistair Francis wrote:
> When we query the device let's also read the descriptor from the device.
> This tells us useful information, including the version, which we use to
> determine a generation.
> 
> This is based on the driver from Wacom.
> 
> Signed-off-by: Alistair Francis <alistair@alistair23.me>
> ---
>  drivers/input/touchscreen/wacom_i2c.c | 64 +++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/wacom_i2c.c b/drivers/input/touchscreen/wacom_i2c.c
> index 28255c77d426..72ba4a36459b 100644
> --- a/drivers/input/touchscreen/wacom_i2c.c
> +++ b/drivers/input/touchscreen/wacom_i2c.c
> @@ -16,6 +16,7 @@
>  #include <linux/of.h>
>  #include <asm/unaligned.h>
>  
> +#define WACOM_DESC_REG	0x01
>  #define WACOM_CMD_QUERY0	0x04
>  #define WACOM_CMD_QUERY1	0x00
>  #define WACOM_CMD_QUERY2	0x33
> @@ -24,11 +25,46 @@
>  #define WACOM_CMD_THROW1	0x00
>  #define WACOM_QUERY_SIZE	19
>  
> +#define WACOM_MAX_DATA_SIZE_BG9     10
> +#define WACOM_MAX_DATA_SIZE_G12     15
> +#define WACOM_MAX_DATA_SIZE_AG14    17
> +#define WACOM_MAX_DATA_SIZE         22
> +
> +/* Generation selction */
> +/* Before and at G9 generation */
> +#define WACOM_BG9	0
> +/* G12 generation the IC supports "height"*/
> +#define WACOM_G12	1
> +/* After and at G14 generation the IC supports "height" and
> + * it is defined as "Z" axis
> + */
> +#define WACOM_AG14	2
> +
> +struct wacom_desc {
> +	u16 descLen;
> +	u16 version;
> +	u16 reportLen;
> +	u16 reportReg;
> +	u16 inputReg;
> +	u16 maxInputLen;
> +	u16 outputReg;
> +	u16 maxOutputLen;
> +	u16 commReg;
> +	u16 dataReg;
> +	u16 vendorID;
> +	u16 productID;
> +	u16 fwVersion;
> +	u16 misc_high;
> +	u16 misc_low;
> +};

This looks like I2C HID descriptor. Is the device actually I2C HID
compatible? If so we should use that and abandon this driver.

Ping, Tatsunosuke, please advise.

Thanks.

-- 
Dmitry

Re: [PATCH v10 05/12] Input: wacom_i2c - Read the descriptor values

[Dmitry Torokhov]

Hi Tatsunosuke,

On Thu, Sep 02, 2021 at 07:33:49AM +0000, Tobita, Tatsunosuke wrote:
> Hi Dmitry,
> 
> Yes, our firmware supports HID over I2C.  However, some of our
> customers often do not want to use HID to handle our hardware; even
> they don't install the generic HID driver neither.  In such case, we
> need to distinguish what generation of our device customer's has. And
> to do so, we check I2C HID descriptor even though the driver is not
> working with HID driver components, but this one.  That is why I2C HID
> descriptor is used there. It is called, but the situation with this
> driver is not supposed to work as a HID device.

I would like to understand better why the customers do not want to use
HID. There needs to be a _very_ strong reason to essentially duplicate
HID layer in a vendor driver and I inclined to say that such customers
would need to patch their kernels themselves.

Thanks.

-- 
Dmitry

Re: [PATCH v10 05/12] Input: wacom_i2c - Read the descriptor values

[Dmitry Torokhov]

Hi Ping,

On Tue, Sep 07, 2021 at 10:25:43PM -0700, Ping Cheng wrote:
> Hi Dmitry,
> 
> On Mon, Sep 6, 2021, 11:05 PM Dmitry Torokhov <dmitry.torokhov@gmail.com>
> wrote:
> 
> > Hi Tatsunosuke,
> >
> > On Thu, Sep 02, 2021 at 07:33:49AM +0000, Tobita, Tatsunosuke wrote:
> > > Hi Dmitry,
> > >
> > > Yes, our firmware supports HID over I2C.  However, some of our
> > > customers often do not want to use HID to handle our hardware; even
> > > they don't install the generic HID driver neither.  In such case, we
> > > need to distinguish what generation of our device customer's has. And
> > > to do so, we check I2C HID descriptor even though the driver is not
> > > working with HID driver components, but this one.  That is why I2C HID
> > > descriptor is used there. It is called, but the situation with this
> > > driver is not supposed to work as a HID device.
> >
> > I would like to understand better why the customers do not want to use
> > HID.
> 
> 
> Those customers normally run embedded Linux. Their hardwares have very
> specific use cases. They don't need to support any other HID devices except
> the Wacom i2c device.
> 
> >
> There needs to be a _very_ strong reason to essentially duplicate
> > HID layer in a vendor driver and I inclined to say that such customers
> 
> would need to patch their kernels themselves.
> 
> 
> They most likely don't want to duplicate HID layer. They just don't need
> most of the HID layer code.

They just need touchscreen support. Plus stylus support. And maybe
battery support. And maybe something else down the road... And they need
to introduce DT and ACPI descriptors to be able to mould the behavior to
platform needs. Which is pretty much the purpose of HID layer.

> wacom_i2c simplifies their deployment and
> testing process. Most of those customers are very small companies...

And now please continue this train of thoughts and consider every touch
vendor. Wacom is not unique. We have Elan, Cypress, Weida, Goodix, etc.
etc. Vendor drivers were acceptable before we had I2C standard, but now
it is much better for everyone to share the efforts and use HID instead
of replicating it for every vendor.

Thanks.

-- 
Dmitry

RE: [PATCH v10 05/12] Input: wacom_i2c - Read the descriptor values

[Tobita, Tatsunosuke]

Hi Dmitry and Ping,

I understand that we should stick with HID as much as possible.
However, there're certainly situations in which some do not want even whole HID, but only an individual functionality for a certain device.
In that case, they may not even include the bit of the HID, but exclude HID. What about then; what they should do without HID?
This would be also the questions if such situations happened to other vendors than Wacom.

Also, what I need to add is that the early generations of our I2C devices do not support HID which is why "wacom_i2c" was added in 2011.


Tats

-----Original Message-----
From: Dmitry Torokhov <dmitry.torokhov@gmail.com> 
Sent: Wednesday, September 8, 2021 2:56 PM
To: Ping Cheng <pinglinux@gmail.com>
Cc: Tobita, Tatsunosuke <tatsunosuke.tobita@wacom.com>; Alistair Francis <alistair@alistair23.me>; Cheng, Ping <Ping.Cheng@wacom.com>; linux-input <linux-input@vger.kernel.org>; linux-imx@nxp.com; kernel@pengutronix.de; Tatsunosuke Tobita <junkpainting@gmail.com>; linux-kernel@vger.kernel.org; alistair23@gmail.com; robh+dt@kernel.org; devicetree@vger.kernel.org
Subject: Re: [PATCH v10 05/12] Input: wacom_i2c - Read the descriptor values

[EXTERNAL]

Hi Ping,

On Tue, Sep 07, 2021 at 10:25:43PM -0700, Ping Cheng wrote:
> Hi Dmitry,
>
> On Mon, Sep 6, 2021, 11:05 PM Dmitry Torokhov 
> <dmitry.torokhov@gmail.com>
> wrote:
>
> > Hi Tatsunosuke,
> >
> > On Thu, Sep 02, 2021 at 07:33:49AM +0000, Tobita, Tatsunosuke wrote:
> > > Hi Dmitry,
> > >
> > > Yes, our firmware supports HID over I2C.  However, some of our 
> > > customers often do not want to use HID to handle our hardware; 
> > > even they don't install the generic HID driver neither.  In such 
> > > case, we need to distinguish what generation of our device 
> > > customer's has. And to do so, we check I2C HID descriptor even 
> > > though the driver is not working with HID driver components, but 
> > > this one.  That is why I2C HID descriptor is used there. It is 
> > > called, but the situation with this driver is not supposed to work as a HID device.
> >
> > I would like to understand better why the customers do not want to 
> > use HID.
>
>
> Those customers normally run embedded Linux. Their hardwares have very 
> specific use cases. They don't need to support any other HID devices 
> except the Wacom i2c device.
>
> >
> There needs to be a _very_ strong reason to essentially duplicate
> > HID layer in a vendor driver and I inclined to say that such 
> > customers
>
> would need to patch their kernels themselves.
>
>
> They most likely don't want to duplicate HID layer. They just don't 
> need most of the HID layer code.

They just need touchscreen support. Plus stylus support. And maybe battery support. And maybe something else down the road... And they need to introduce DT and ACPI descriptors to be able to mould the behavior to platform needs. Which is pretty much the purpose of HID layer.

> wacom_i2c simplifies their deployment and testing process. Most of 
> those customers are very small companies...

And now please continue this train of thoughts and consider every touch vendor. Wacom is not unique. We have Elan, Cypress, Weida, Goodix, etc.
etc. Vendor drivers were acceptable before we had I2C standard, but now it is much better for everyone to share the efforts and use HID instead of replicating it for every vendor.

Thanks.

--
Dmitry

RE: [PATCH v10 05/12] Input: wacom_i2c - Read the descriptor values

[Tobita, Tatsunosuke]

Hi Dmitry,

I now understand what you mean. The understandable example is USB. The most of the recent drivers for USB devices has bee released as HiD driver.
That said. It's glad if you can have comments too about my questions.
Especially, when someone doesn't want the whole HID driver, but a single I2C I/F'd input device driver.
And also, I want to make correction that ***not*** all of our devices support HID. Some old devices do not support HID; that's why I added the driver in 2011.

Thanks,

Tats

-----Original Message-----
From: Tobita, Tatsunosuke <tatsunosuke.tobita@wacom.com> 
Sent: Friday, September 10, 2021 1:10 PM
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>; Ping Cheng <pinglinux@gmail.com>
Cc: Alistair Francis <alistair@alistair23.me>; Cheng, Ping <Ping.Cheng@wacom.com>; linux-input <linux-input@vger.kernel.org>; linux-imx@nxp.com; kernel@pengutronix.de; Tatsunosuke Tobita <junkpainting@gmail.com>; linux-kernel@vger.kernel.org; alistair23@gmail.com; robh+dt@kernel.org; devicetree@vger.kernel.org
Subject: RE: [PATCH v10 05/12] Input: wacom_i2c - Read the descriptor values

[EXTERNAL]

Hi Dmitry and Ping,

I understand that we should stick with HID as much as possible.
However, there're certainly situations in which some do not want even whole HID, but only an individual functionality for a certain device.
In that case, they may not even include the bit of the HID, but exclude HID. What about then; what they should do without HID?
This would be also the questions if such situations happened to other vendors than Wacom.

Also, what I need to add is that the early generations of our I2C devices do not support HID which is why "wacom_i2c" was added in 2011.


Tats

-----Original Message-----
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Sent: Wednesday, September 8, 2021 2:56 PM
To: Ping Cheng <pinglinux@gmail.com>
Cc: Tobita, Tatsunosuke <tatsunosuke.tobita@wacom.com>; Alistair Francis <alistair@alistair23.me>; Cheng, Ping <Ping.Cheng@wacom.com>; linux-input <linux-input@vger.kernel.org>; linux-imx@nxp.com; kernel@pengutronix.de; Tatsunosuke Tobita <junkpainting@gmail.com>; linux-kernel@vger.kernel.org; alistair23@gmail.com; robh+dt@kernel.org; devicetree@vger.kernel.org
Subject: Re: [PATCH v10 05/12] Input: wacom_i2c - Read the descriptor values

[EXTERNAL]

Hi Ping,

On Tue, Sep 07, 2021 at 10:25:43PM -0700, Ping Cheng wrote:
> Hi Dmitry,
>
> On Mon, Sep 6, 2021, 11:05 PM Dmitry Torokhov 
> <dmitry.torokhov@gmail.com>
> wrote:
>
> > Hi Tatsunosuke,
> >
> > On Thu, Sep 02, 2021 at 07:33:49AM +0000, Tobita, Tatsunosuke wrote:
> > > Hi Dmitry,
> > >
> > > Yes, our firmware supports HID over I2C.  However, some of our 
> > > customers often do not want to use HID to handle our hardware; 
> > > even they don't install the generic HID driver neither.  In such 
> > > case, we need to distinguish what generation of our device 
> > > customer's has. And to do so, we check I2C HID descriptor even 
> > > though the driver is not working with HID driver components, but 
> > > this one.  That is why I2C HID descriptor is used there. It is 
> > > called, but the situation with this driver is not supposed to work as a HID device.
> >
> > I would like to understand better why the customers do not want to 
> > use HID.
>
>
> Those customers normally run embedded Linux. Their hardwares have very 
> specific use cases. They don't need to support any other HID devices 
> except the Wacom i2c device.
>
> >
> There needs to be a _very_ strong reason to essentially duplicate
> > HID layer in a vendor driver and I inclined to say that such 
> > customers
>
> would need to patch their kernels themselves.
>
>
> They most likely don't want to duplicate HID layer. They just don't 
> need most of the HID layer code.

They just need touchscreen support. Plus stylus support. And maybe battery support. And maybe something else down the road... And they need to introduce DT and ACPI descriptors to be able to mould the behavior to platform needs. Which is pretty much the purpose of HID layer.

> wacom_i2c simplifies their deployment and testing process. Most of 
> those customers are very small companies...

And now please continue this train of thoughts and consider every touch vendor. Wacom is not unique. We have Elan, Cypress, Weida, Goodix, etc.
etc. Vendor drivers were acceptable before we had I2C standard, but now it is much better for everyone to share the efforts and use HID instead of replicating it for every vendor.

Thanks.

--
Dmitry

Re: [PATCH v10 05/12] Input: wacom_i2c - Read the descriptor values

[Dmitry Torokhov]

Hi Tatsunosuke,

On Wed, Oct 06, 2021 at 07:08:38AM +0000, Tobita, Tatsunosuke wrote:
> Hi Dmitry,
> 
> I now understand what you mean. The understandable example is USB. The
> most of the recent drivers for USB devices has bee released as HiD
> driver.  That said. It's glad if you can have comments too about my
> questions.  Especially, when someone doesn't want the whole HID
> driver, but a single I2C I/F'd input device driver.

So far I have not heard a good reason for "not wanting" to use a
standard, well tested solution that everyone else is using, and instead
having a custom driver that essentially reimplements everything that HID
layer already does. Is the additional memory requirements of HID layer
too onerous? Can we address that instead?

If we continue this train of thought, why are they concerned with HID,
but happy with using I2C layer? Why don't they require a driver that
bangs directly onto I2C master ports bypassing all the layersi and
communicating with the peripheral directly?

> And also, I want to make correction that ***not*** all of our devices
> support HID. Some old devices do not support HID; that's why I added
> the driver in 2011.

And that is a good reason to keep existing version of wacom_i2c in the
kernel, but we should not try to extend it to handle HID-compatible
devices.

Thanks,
Dmitry

> 
> Thanks,
> 
> Tats
> 
> -----Original Message-----
> From: Tobita, Tatsunosuke <tatsunosuke.tobita@wacom.com> 
> Sent: Friday, September 10, 2021 1:10 PM
> To: Dmitry Torokhov <dmitry.torokhov@gmail.com>; Ping Cheng <pinglinux@gmail.com>
> Cc: Alistair Francis <alistair@alistair23.me>; Cheng, Ping <Ping.Cheng@wacom.com>; linux-input <linux-input@vger.kernel.org>; linux-imx@nxp.com; kernel@pengutronix.de; Tatsunosuke Tobita <junkpainting@gmail.com>; linux-kernel@vger.kernel.org; alistair23@gmail.com; robh+dt@kernel.org; devicetree@vger.kernel.org
> Subject: RE: [PATCH v10 05/12] Input: wacom_i2c - Read the descriptor values
> 
> [EXTERNAL]
> 
> Hi Dmitry and Ping,
> 
> I understand that we should stick with HID as much as possible.
> However, there're certainly situations in which some do not want even whole HID, but only an individual functionality for a certain device.
> In that case, they may not even include the bit of the HID, but exclude HID. What about then; what they should do without HID?
> This would be also the questions if such situations happened to other vendors than Wacom.
> 
> Also, what I need to add is that the early generations of our I2C devices do not support HID which is why "wacom_i2c" was added in 2011.
> 
> 
> Tats
> 
> -----Original Message-----
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Sent: Wednesday, September 8, 2021 2:56 PM
> To: Ping Cheng <pinglinux@gmail.com>
> Cc: Tobita, Tatsunosuke <tatsunosuke.tobita@wacom.com>; Alistair Francis <alistair@alistair23.me>; Cheng, Ping <Ping.Cheng@wacom.com>; linux-input <linux-input@vger.kernel.org>; linux-imx@nxp.com; kernel@pengutronix.de; Tatsunosuke Tobita <junkpainting@gmail.com>; linux-kernel@vger.kernel.org; alistair23@gmail.com; robh+dt@kernel.org; devicetree@vger.kernel.org
> Subject: Re: [PATCH v10 05/12] Input: wacom_i2c - Read the descriptor values
> 
> [EXTERNAL]
> 
> Hi Ping,
> 
> On Tue, Sep 07, 2021 at 10:25:43PM -0700, Ping Cheng wrote:
> > Hi Dmitry,
> >
> > On Mon, Sep 6, 2021, 11:05 PM Dmitry Torokhov 
> > <dmitry.torokhov@gmail.com>
> > wrote:
> >
> > > Hi Tatsunosuke,
> > >
> > > On Thu, Sep 02, 2021 at 07:33:49AM +0000, Tobita, Tatsunosuke wrote:
> > > > Hi Dmitry,
> > > >
> > > > Yes, our firmware supports HID over I2C.  However, some of our 
> > > > customers often do not want to use HID to handle our hardware; 
> > > > even they don't install the generic HID driver neither.  In such 
> > > > case, we need to distinguish what generation of our device 
> > > > customer's has. And to do so, we check I2C HID descriptor even 
> > > > though the driver is not working with HID driver components, but 
> > > > this one.  That is why I2C HID descriptor is used there. It is 
> > > > called, but the situation with this driver is not supposed to work as a HID device.
> > >
> > > I would like to understand better why the customers do not want to 
> > > use HID.
> >
> >
> > Those customers normally run embedded Linux. Their hardwares have very 
> > specific use cases. They don't need to support any other HID devices 
> > except the Wacom i2c device.
> >
> > >
> > There needs to be a _very_ strong reason to essentially duplicate
> > > HID layer in a vendor driver and I inclined to say that such 
> > > customers
> >
> > would need to patch their kernels themselves.
> >
> >
> > They most likely don't want to duplicate HID layer. They just don't 
> > need most of the HID layer code.
> 
> They just need touchscreen support. Plus stylus support. And maybe battery support. And maybe something else down the road... And they need to introduce DT and ACPI descriptors to be able to mould the behavior to platform needs. Which is pretty much the purpose of HID layer.
> 
> > wacom_i2c simplifies their deployment and testing process. Most of 
> > those customers are very small companies...
> 
> And now please continue this train of thoughts and consider every touch vendor. Wacom is not unique. We have Elan, Cypress, Weida, Goodix, etc.
> etc. Vendor drivers were acceptable before we had I2C standard, but now it is much better for everyone to share the efforts and use HID instead of replicating it for every vendor.
> 
> Thanks.
> 
> --
> Dmitry

RE: [PATCH v10 05/12] Input: wacom_i2c - Read the descriptor values

[Tobita, Tatsunosuke]

Dmitry,

Thanks you. I appreciate that your answered my questions.
I think now I agree we just stay with the current wacom_i2c.

This is just to tell you what kind of situation we deal with wacom_i2c with our customers.
We work with OEMs,/ODMs and they often want to add/customize their own features onto Wacom pen functionality with their Linux based OS, such as Chrome OS, Android, Linux desktop distribution, etc.
In those cases, some of them want to use wacom_i2c.
The reason is if they used wacom_i2c, they would be able to concentrate on their own desired programs.
Since wacom_i2c has simple structure for initialization - their desired special preparation- and for handling input events to add their specific purpose,
it is easier for OEMs/ODMs to handle such. On the other hand, they may have to look into whole hid codes throughout parsing hid collections when they
want to the same as I just described prior to this sentence; and it takes time more than they do with wacom_i2c. Also, finding problems becomes more difficult for them if they have.

We usually recommend the customers use i2c-hid, but not all of them follow it and use wacom_i2c with that reasons above.
We, this time, tried to update it because some of the customers try to use the current wacom_i2c as it is - compatible older generations of the products- and ask us the correct one.
Yes we do then, but we thought maybe updating wacom_i2c may help them use it easier.

That's it. I just want to tell you why we started suddenly update the driver.
Thanks,

Tats

-----Original Message-----
From: Dmitry Torokhov <dmitry.torokhov@gmail.com> 
Sent: Thursday, October 7, 2021 3:03 AM
To: Tobita, Tatsunosuke <tatsunosuke.tobita@wacom.com>
Cc: Ping Cheng <pinglinux@gmail.com>; Alistair Francis <alistair@alistair23.me>; Cheng, Ping <Ping.Cheng@wacom.com>; linux-input <linux-input@vger.kernel.org>; linux-imx@nxp.com; kernel@pengutronix.de; Tatsunosuke Tobita <junkpainting@gmail.com>; linux-kernel@vger.kernel.org; alistair23@gmail.com; robh+dt@kernel.org; devicetree@vger.kernel.org
Subject: Re: [PATCH v10 05/12] Input: wacom_i2c - Read the descriptor values

[EXTERNAL]

Hi Tatsunosuke,

On Wed, Oct 06, 2021 at 07:08:38AM +0000, Tobita, Tatsunosuke wrote:
> Hi Dmitry,
>
> I now understand what you mean. The understandable example is USB. The 
> most of the recent drivers for USB devices has bee released as HiD 
> driver.  That said. It's glad if you can have comments too about my 
> questions.  Especially, when someone doesn't want the whole HID 
> driver, but a single I2C I/F'd input device driver.

So far I have not heard a good reason for "not wanting" to use a standard, well tested solution that everyone else is using, and instead having a custom driver that essentially reimplements everything that HID layer already does. Is the additional memory requirements of HID layer too onerous? Can we address that instead?

If we continue this train of thought, why are they concerned with HID, but happy with using I2C layer? Why don't they require a driver that bangs directly onto I2C master ports bypassing all the layersi and communicating with the peripheral directly?

> And also, I want to make correction that ***not*** all of our devices 
> support HID. Some old devices do not support HID; that's why I added 
> the driver in 2011.

And that is a good reason to keep existing version of wacom_i2c in the kernel, but we should not try to extend it to handle HID-compatible devices.

Thanks,
Dmitry

>
> Thanks,
>
> Tats
>
> -----Original Message-----
> From: Tobita, Tatsunosuke <tatsunosuke.tobita@wacom.com>
> Sent: Friday, September 10, 2021 1:10 PM
> To: Dmitry Torokhov <dmitry.torokhov@gmail.com>; Ping Cheng 
> <pinglinux@gmail.com>
> Cc: Alistair Francis <alistair@alistair23.me>; Cheng, Ping 
> <Ping.Cheng@wacom.com>; linux-input <linux-input@vger.kernel.org>; 
> linux-imx@nxp.com; kernel@pengutronix.de; Tatsunosuke Tobita 
> <junkpainting@gmail.com>; linux-kernel@vger.kernel.org; 
> alistair23@gmail.com; robh+dt@kernel.org; devicetree@vger.kernel.org
> Subject: RE: [PATCH v10 05/12] Input: wacom_i2c - Read the descriptor 
> values
>
> [EXTERNAL]
>
> Hi Dmitry and Ping,
>
> I understand that we should stick with HID as much as possible.
> However, there're certainly situations in which some do not want even whole HID, but only an individual functionality for a certain device.
> In that case, they may not even include the bit of the HID, but exclude HID. What about then; what they should do without HID?
> This would be also the questions if such situations happened to other vendors than Wacom.
>
> Also, what I need to add is that the early generations of our I2C devices do not support HID which is why "wacom_i2c" was added in 2011.
>
>
> Tats
>
> -----Original Message-----
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Sent: Wednesday, September 8, 2021 2:56 PM
> To: Ping Cheng <pinglinux@gmail.com>
> Cc: Tobita, Tatsunosuke <tatsunosuke.tobita@wacom.com>; Alistair 
> Francis <alistair@alistair23.me>; Cheng, Ping <Ping.Cheng@wacom.com>; 
> linux-input <linux-input@vger.kernel.org>; linux-imx@nxp.com; 
> kernel@pengutronix.de; Tatsunosuke Tobita <junkpainting@gmail.com>; 
> linux-kernel@vger.kernel.org; alistair23@gmail.com; 
> robh+dt@kernel.org; devicetree@vger.kernel.org
> Subject: Re: [PATCH v10 05/12] Input: wacom_i2c - Read the descriptor 
> values
>
> [EXTERNAL]
>
> Hi Ping,
>
> On Tue, Sep 07, 2021 at 10:25:43PM -0700, Ping Cheng wrote:
> > Hi Dmitry,
> >
> > On Mon, Sep 6, 2021, 11:05 PM Dmitry Torokhov 
> > <dmitry.torokhov@gmail.com>
> > wrote:
> >
> > > Hi Tatsunosuke,
> > >
> > > On Thu, Sep 02, 2021 at 07:33:49AM +0000, Tobita, Tatsunosuke wrote:
> > > > Hi Dmitry,
> > > >
> > > > Yes, our firmware supports HID over I2C.  However, some of our 
> > > > customers often do not want to use HID to handle our hardware; 
> > > > even they don't install the generic HID driver neither.  In such 
> > > > case, we need to distinguish what generation of our device 
> > > > customer's has. And to do so, we check I2C HID descriptor even 
> > > > though the driver is not working with HID driver components, but 
> > > > this one.  That is why I2C HID descriptor is used there. It is 
> > > > called, but the situation with this driver is not supposed to work as a HID device.
> > >
> > > I would like to understand better why the customers do not want to 
> > > use HID.
> >
> >
> > Those customers normally run embedded Linux. Their hardwares have 
> > very specific use cases. They don't need to support any other HID 
> > devices except the Wacom i2c device.
> >
> > >
> > There needs to be a _very_ strong reason to essentially duplicate
> > > HID layer in a vendor driver and I inclined to say that such 
> > > customers
> >
> > would need to patch their kernels themselves.
> >
> >
> > They most likely don't want to duplicate HID layer. They just don't 
> > need most of the HID layer code.
>
> They just need touchscreen support. Plus stylus support. And maybe battery support. And maybe something else down the road... And they need to introduce DT and ACPI descriptors to be able to mould the behavior to platform needs. Which is pretty much the purpose of HID layer.
>
> > wacom_i2c simplifies their deployment and testing process. Most of 
> > those customers are very small companies...
>
> And now please continue this train of thoughts and consider every touch vendor. Wacom is not unique. We have Elan, Cypress, Weida, Goodix, etc.
> etc. Vendor drivers were acceptable before we had I2C standard, but now it is much better for everyone to share the efforts and use HID instead of replicating it for every vendor.
>
> Thanks.
>
> --
> Dmitry

Re: [PATCH v10 05/12] Input: wacom_i2c - Read the descriptor values

[Ping Cheng]

Hi Dmitry,

On Tue, Sep 7, 2021 at 10:55 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> > > >
> > > > Yes, our firmware supports HID over I2C.  However, some of our
> > > > customers often do not want to use HID to handle our hardware; even
> > > > they don't install the generic HID driver neither.  In such case, we
> > > > need to distinguish what generation of our device customer's has. And
> > > > to do so, we check I2C HID descriptor even though the driver is not
> > > > working with HID driver components, but this one.  That is why I2C HID
> > > > descriptor is used there. It is called, but the situation with this
> > > > driver is not supposed to work as a HID device.
> > >
> > > I would like to understand better why the customers do not want to use
> > > HID.
> >
> >
> > Those customers normally run embedded Linux. Their hardwares have very
> > specific use cases. They don't need to support any other HID devices except
> > the Wacom i2c device.
> >
> > >
> > There needs to be a _very_ strong reason to essentially duplicate
> > > HID layer in a vendor driver and I inclined to say that such customers
> >
> > would need to patch their kernels themselves.
> >
> > They most likely don't want to duplicate HID layer. They just don't need
> > most of the HID layer code.
>
> They just need touchscreen support. Plus stylus support. And maybe
> battery support. And maybe something else down the road... And they need
> to introduce DT and ACPI descriptors to be able to mould the behavior to
> platform needs. Which is pretty much the purpose of HID layer.

I see your point.

> > wacom_i2c simplifies their deployment and
> > testing process. Most of those customers are very small companies...
>
> And now please continue this train of thoughts and consider every touch
> vendor. Wacom is not unique. We have Elan, Cypress, Weida, Goodix, etc.
> etc. Vendor drivers were acceptable before we had I2C standard, but now
> it is much better for everyone to share the efforts and use HID instead
> of replicating it for every vendor.

And I agree with you that we should share our efforts on the main tasks.

However, with the same token of sharing efforts, I see the benefit of
merging this set of patches upstream. From the version number we can
tell the patchset has gone through at least 10 rounds of review and
update. Alistair has put a lot of effort to get this far (Thank you
Alistair for your time and effort!). A few community developers have
also reviewed the patches. This set of patches thoroughly touched all
parts of the components that related to an input i2c driver, which is
much better than the original version. This patchset would be a great
starting point for vendors to create their out of tree drivers, when
necessary. It would also offer vendors a clear picture of what
components they need to change/update to make their i2c input device
work under kernel input subsystem.

So, merging the patchset will benefit more people and preserve the
effort that went into the patchset so far. If you like, you can add a
comment in the patch mentioning that future effort should be directed
to the i2c-hid subsystem, etc...

I think the ultimate goal is to encourage more people to contribute to
open source projects and to make Linux a great platform that everyone
can work on easily.

Thank you for your consideration and support,
Ping

Re: [PATCH v10 05/12] Input: wacom_i2c - Read the descriptor values

[Dmitry Torokhov]

Hi Ping,

On Fri, Sep 17, 2021 at 01:43:18PM -0700, Ping Cheng wrote:
> Hi Dmitry,
> 
> On Tue, Sep 7, 2021 at 10:55 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > > > >
> > > > > Yes, our firmware supports HID over I2C.  However, some of our
> > > > > customers often do not want to use HID to handle our hardware; even
> > > > > they don't install the generic HID driver neither.  In such case, we
> > > > > need to distinguish what generation of our device customer's has. And
> > > > > to do so, we check I2C HID descriptor even though the driver is not
> > > > > working with HID driver components, but this one.  That is why I2C HID
> > > > > descriptor is used there. It is called, but the situation with this
> > > > > driver is not supposed to work as a HID device.
> > > >
> > > > I would like to understand better why the customers do not want to use
> > > > HID.
> > >
> > >
> > > Those customers normally run embedded Linux. Their hardwares have very
> > > specific use cases. They don't need to support any other HID devices except
> > > the Wacom i2c device.
> > >
> > > >
> > > There needs to be a _very_ strong reason to essentially duplicate
> > > > HID layer in a vendor driver and I inclined to say that such customers
> > >
> > > would need to patch their kernels themselves.
> > >
> > > They most likely don't want to duplicate HID layer. They just don't need
> > > most of the HID layer code.
> >
> > They just need touchscreen support. Plus stylus support. And maybe
> > battery support. And maybe something else down the road... And they need
> > to introduce DT and ACPI descriptors to be able to mould the behavior to
> > platform needs. Which is pretty much the purpose of HID layer.
> 
> I see your point.
> 
> > > wacom_i2c simplifies their deployment and
> > > testing process. Most of those customers are very small companies...
> >
> > And now please continue this train of thoughts and consider every touch
> > vendor. Wacom is not unique. We have Elan, Cypress, Weida, Goodix, etc.
> > etc. Vendor drivers were acceptable before we had I2C standard, but now
> > it is much better for everyone to share the efforts and use HID instead
> > of replicating it for every vendor.
> 
> And I agree with you that we should share our efforts on the main tasks.
> 
> However, with the same token of sharing efforts, I see the benefit of
> merging this set of patches upstream. From the version number we can
> tell the patchset has gone through at least 10 rounds of review and
> update. Alistair has put a lot of effort to get this far (Thank you
> Alistair for your time and effort!).

I am sorry, but the fact that a developer spent a lot of time writing
code can not be used as a justification for merging said code.

> A few community developers have
> also reviewed the patches. This set of patches thoroughly touched all
> parts of the components that related to an input i2c driver, which is
> much better than the original version. This patchset would be a great
> starting point for vendors to create their out of tree drivers, when
> necessary. It would also offer vendors a clear picture of what
> components they need to change/update to make their i2c input device
> work under kernel input subsystem.

And they can do that by looking at the mailing list archive and they can
also follow this discussion and see why what they are doing is quite
wrong. I mean, why would they stop at dropping HID only? Why not bypass
I2C system as well and poke at the I2C controller directly. Skip PCI as
well?

> 
> So, merging the patchset will benefit more people and preserve the
> effort that went into the patchset so far. If you like, you can add a
> comment in the patch mentioning that future effort should be directed
> to the i2c-hid subsystem, etc...

No, we should not merge the patch set that we agreed is wrong in its
approach. Maybe if I (and other community reviewers) realized that the
device was HID compliant we could stop this effort earlier, but
unfortunately it did not happen, so effort was wasted, but this happens
sometimes.

Thanks.

-- 
Dmitry

Re: [PATCH v10 05/12] Input: wacom_i2c - Read the descriptor values

[Alistair Francis]

On Tue, Sep 21, 2021 at 2:35 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Hi Ping,
>
> On Fri, Sep 17, 2021 at 01:43:18PM -0700, Ping Cheng wrote:
> > Hi Dmitry,
> >
> > On Tue, Sep 7, 2021 at 10:55 PM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > >
> > > > > >
> > > > > > Yes, our firmware supports HID over I2C.  However, some of our
> > > > > > customers often do not want to use HID to handle our hardware; even
> > > > > > they don't install the generic HID driver neither.  In such case, we
> > > > > > need to distinguish what generation of our device customer's has. And
> > > > > > to do so, we check I2C HID descriptor even though the driver is not
> > > > > > working with HID driver components, but this one.  That is why I2C HID
> > > > > > descriptor is used there. It is called, but the situation with this
> > > > > > driver is not supposed to work as a HID device.
> > > > >
> > > > > I would like to understand better why the customers do not want to use
> > > > > HID.
> > > >
> > > >
> > > > Those customers normally run embedded Linux. Their hardwares have very
> > > > specific use cases. They don't need to support any other HID devices except
> > > > the Wacom i2c device.
> > > >
> > > > >
> > > > There needs to be a _very_ strong reason to essentially duplicate
> > > > > HID layer in a vendor driver and I inclined to say that such customers
> > > >
> > > > would need to patch their kernels themselves.
> > > >
> > > > They most likely don't want to duplicate HID layer. They just don't need
> > > > most of the HID layer code.
> > >
> > > They just need touchscreen support. Plus stylus support. And maybe
> > > battery support. And maybe something else down the road... And they need
> > > to introduce DT and ACPI descriptors to be able to mould the behavior to
> > > platform needs. Which is pretty much the purpose of HID layer.
> >
> > I see your point.
> >
> > > > wacom_i2c simplifies their deployment and
> > > > testing process. Most of those customers are very small companies...
> > >
> > > And now please continue this train of thoughts and consider every touch
> > > vendor. Wacom is not unique. We have Elan, Cypress, Weida, Goodix, etc.
> > > etc. Vendor drivers were acceptable before we had I2C standard, but now
> > > it is much better for everyone to share the efforts and use HID instead
> > > of replicating it for every vendor.
> >
> > And I agree with you that we should share our efforts on the main tasks.
> >
> > However, with the same token of sharing efforts, I see the benefit of
> > merging this set of patches upstream. From the version number we can
> > tell the patchset has gone through at least 10 rounds of review and
> > update. Alistair has put a lot of effort to get this far (Thank you
> > Alistair for your time and effort!).
>
> I am sorry, but the fact that a developer spent a lot of time writing
> code can not be used as a justification for merging said code.
>
> > A few community developers have
> > also reviewed the patches. This set of patches thoroughly touched all
> > parts of the components that related to an input i2c driver, which is
> > much better than the original version. This patchset would be a great
> > starting point for vendors to create their out of tree drivers, when
> > necessary. It would also offer vendors a clear picture of what
> > components they need to change/update to make their i2c input device
> > work under kernel input subsystem.
>
> And they can do that by looking at the mailing list archive and they can
> also follow this discussion and see why what they are doing is quite
> wrong. I mean, why would they stop at dropping HID only? Why not bypass
> I2C system as well and poke at the I2C controller directly. Skip PCI as
> well?
>
> >
> > So, merging the patchset will benefit more people and preserve the
> > effort that went into the patchset so far. If you like, you can add a
> > comment in the patch mentioning that future effort should be directed
> > to the i2c-hid subsystem, etc...
>
> No, we should not merge the patch set that we agreed is wrong in its
> approach. Maybe if I (and other community reviewers) realized that the
> device was HID compliant we could stop this effort earlier, but
> unfortunately it did not happen, so effort was wasted, but this happens
> sometimes.

Should I prepare a patch to remove the wacom I2C driver entirely then?

Alistair

>
> Thanks.
>
> --
> Dmitry

[PATCH v10 06/12] Input: wacom_i2c - Add support for distance and tilt x/y

[Alistair Francis]

Add support for the distance and tilt x/y.

This is based on the out of tree rM2 driver.

Signed-off-by: Alistair Francis <alistair@alistair23.me>
---
 drivers/input/touchscreen/wacom_i2c.c | 30 +++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/wacom_i2c.c b/drivers/input/touchscreen/wacom_i2c.c
index 72ba4a36459b..d6036406a9f3 100644
--- a/drivers/input/touchscreen/wacom_i2c.c
+++ b/drivers/input/touchscreen/wacom_i2c.c
@@ -23,7 +23,7 @@
 #define WACOM_CMD_QUERY3	0x02
 #define WACOM_CMD_THROW0	0x05
 #define WACOM_CMD_THROW1	0x00
-#define WACOM_QUERY_SIZE	19
+#define WACOM_QUERY_SIZE	22
 
 #define WACOM_MAX_DATA_SIZE_BG9     10
 #define WACOM_MAX_DATA_SIZE_G12     15
@@ -63,6 +63,9 @@ struct wacom_features {
 	int x_max;
 	int y_max;
 	int pressure_max;
+	int distance_max;
+	int tilt_x_max;
+	int tilt_y_max;
 	char fw_version;
 	unsigned char generation;
 };
@@ -144,6 +147,15 @@ static int wacom_query_device(struct i2c_client *client,
 	features->y_max = get_unaligned_le16(&data[5]);
 	features->pressure_max = get_unaligned_le16(&data[11]);
 	features->fw_version = get_unaligned_le16(&data[13]);
+	if (features->generation) {
+		features->distance_max = data[16];
+		features->tilt_x_max = get_unaligned_le16(&data[17]);
+		features->tilt_y_max = get_unaligned_le16(&data[19]);
+	} else {
+		features->distance_max = -1;
+		features->tilt_x_max = -1;
+		features->tilt_y_max = -1;
+	}
 
 	dev_dbg(&client->dev,
 		"x_max:%d, y_max:%d, pressure:%d, fw:%d\n",
@@ -161,6 +173,7 @@ static irqreturn_t wacom_i2c_irq(int irq, void *dev_id)
 	u8 *data = wac_i2c->data;
 	unsigned int x, y, pressure;
 	unsigned char tsw, f1, f2, ers;
+	short tilt_x, tilt_y, distance;
 	int error;
 
 	error = i2c_master_recv(wac_i2c->client,
@@ -176,6 +189,12 @@ static irqreturn_t wacom_i2c_irq(int irq, void *dev_id)
 	y = le16_to_cpup((__le16 *)&data[6]);
 	pressure = le16_to_cpup((__le16 *)&data[8]);
 
+	/* Signed */
+	tilt_x = get_unaligned_le16(&data[11]);
+	tilt_y = get_unaligned_le16(&data[13]);
+
+	distance = get_unaligned_le16(&data[15]);
+
 	if (!wac_i2c->prox)
 		wac_i2c->tool = (data[3] & 0x0c) ?
 			BTN_TOOL_RUBBER : BTN_TOOL_PEN;
@@ -191,6 +210,9 @@ static irqreturn_t wacom_i2c_irq(int irq, void *dev_id)
 	input_report_abs(input, ABS_X, x);
 	input_report_abs(input, ABS_Y, y);
 	input_report_abs(input, ABS_PRESSURE, pressure);
+	input_report_abs(input, ABS_DISTANCE, distance);
+	input_report_abs(input, ABS_TILT_X, tilt_x);
+	input_report_abs(input, ABS_TILT_Y, tilt_y);
 	input_sync(input);
 
 out:
@@ -266,7 +288,11 @@ static int wacom_i2c_probe(struct i2c_client *client,
 	input_set_abs_params(input, ABS_Y, 0, features->y_max, 0, 0);
 	input_set_abs_params(input, ABS_PRESSURE,
 			     0, features->pressure_max, 0, 0);
-
+	input_set_abs_params(input, ABS_DISTANCE, 0, features->distance_max, 0, 0);
+	input_set_abs_params(input, ABS_TILT_X, -features->tilt_x_max,
+			     features->tilt_x_max, 0, 0);
+	input_set_abs_params(input, ABS_TILT_Y, -features->tilt_y_max,
+			     features->tilt_y_max, 0, 0);
 	input_set_drvdata(input, wac_i2c);
 
 	error = devm_request_threaded_irq(dev, client->irq, NULL, wacom_i2c_irq,
-- 
2.31.1

[PATCH v10 07/12] Input: wacom_i2c - Clean up the query device fields

[Alistair Francis]

Improve the query device fields to be more verbose.

Signed-off-by: Alistair Francis <alistair@alistair23.me>
---
 drivers/input/touchscreen/wacom_i2c.c | 63 ++++++++++++++++++---------
 1 file changed, 43 insertions(+), 20 deletions(-)

diff --git a/drivers/input/touchscreen/wacom_i2c.c b/drivers/input/touchscreen/wacom_i2c.c
index d6036406a9f3..a35d8fda7e7b 100644
--- a/drivers/input/touchscreen/wacom_i2c.c
+++ b/drivers/input/touchscreen/wacom_i2c.c
@@ -13,16 +13,33 @@
 #include <linux/irq.h>
 #include <linux/input/touchscreen.h>
 #include <linux/interrupt.h>
+#include <linux/reset.h>
 #include <linux/of.h>
 #include <asm/unaligned.h>
 
+// Registers
 #define WACOM_DESC_REG	0x01
-#define WACOM_CMD_QUERY0	0x04
-#define WACOM_CMD_QUERY1	0x00
-#define WACOM_CMD_QUERY2	0x33
-#define WACOM_CMD_QUERY3	0x02
-#define WACOM_CMD_THROW0	0x05
-#define WACOM_CMD_THROW1	0x00
+#define WACOM_COMMAND_LSB   0x04
+#define WACOM_COMMAND_MSB   0x00
+
+#define WACOM_DATA_LSB      0x05
+#define WACOM_DATA_MSB      0x00
+
+// Report types
+#define REPORT_FEATURE      0x30
+
+// Requests / operations
+#define OPCODE_GET_REPORT   0x02
+
+// Power settings
+#define POWER_ON            0x00
+#define POWER_SLEEP         0x01
+
+// Input report ids
+#define WACOM_PEN_DATA_REPORT           2
+#define WACOM_SHINONOME_REPORT          26
+
+#define WACOM_QUERY_REPORT	3
 #define WACOM_QUERY_SIZE	22
 
 #define WACOM_MAX_DATA_SIZE_BG9     10
@@ -85,27 +102,30 @@ static int wacom_query_device(struct i2c_client *client,
 {
 	int ret;
 	u8 cmd_wac_desc[] = {WACOM_DESC_REG, 0x00};
-	u8 cmd1[] = { WACOM_CMD_QUERY0, WACOM_CMD_QUERY1,
-			WACOM_CMD_QUERY2, WACOM_CMD_QUERY3 };
-	u8 cmd2[] = { WACOM_CMD_THROW0, WACOM_CMD_THROW1 };
 	u8 data[WACOM_QUERY_SIZE];
+
+	u8 get_query_data_cmd[] = {
+		WACOM_COMMAND_LSB,
+		WACOM_COMMAND_MSB,
+		REPORT_FEATURE | WACOM_QUERY_REPORT,
+		OPCODE_GET_REPORT,
+		WACOM_DATA_LSB,
+		WACOM_DATA_MSB,
+	};
+
 	struct i2c_msg msgs[] = {
+		// Request reading of feature ReportID: 3 (Pen Query Data)
 		{
 			.addr = client->addr,
 			.flags = 0,
-			.len = sizeof(cmd1),
-			.buf = cmd1,
-		},
-		{
-			.addr = client->addr,
-			.flags = 0,
-			.len = sizeof(cmd2),
-			.buf = cmd2,
+			.len = sizeof(get_query_data_cmd),
+			.buf = get_query_data_cmd,
 		},
+		// Read 21 bytes
 		{
 			.addr = client->addr,
 			.flags = I2C_M_RD,
-			.len = sizeof(data),
+			.len = WACOM_QUERY_SIZE - 1,
 			.buf = data,
 		},
 	};
@@ -158,9 +178,12 @@ static int wacom_query_device(struct i2c_client *client,
 	}
 
 	dev_dbg(&client->dev,
-		"x_max:%d, y_max:%d, pressure:%d, fw:%d\n",
+		"x_max:%d, y_max:%d, pressure:%d, fw:%d, "
+		"distance: %d, tilt_x_max: %d, tilt_y_max: %d\n",
 		features->x_max, features->y_max,
-		features->pressure_max, features->fw_version);
+		features->pressure_max, features->fw_version,
+		features->distance_max,
+		features->tilt_x_max, features->tilt_y_max);
 
 	return 0;
 }
-- 
2.31.1

Re: [PATCH v10 07/12] Input: wacom_i2c - Clean up the query device fields

[Kari Argillander]

Just minor style issues checked.

On Sun, Aug 29, 2021 at 07:19:20PM +1000, Alistair Francis wrote:
> Improve the query device fields to be more verbose.
> 
> Signed-off-by: Alistair Francis <alistair@alistair23.me>
> ---
>  drivers/input/touchscreen/wacom_i2c.c | 63 ++++++++++++++++++---------
>  1 file changed, 43 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/wacom_i2c.c b/drivers/input/touchscreen/wacom_i2c.c
> index d6036406a9f3..a35d8fda7e7b 100644
> --- a/drivers/input/touchscreen/wacom_i2c.c
> +++ b/drivers/input/touchscreen/wacom_i2c.c
> @@ -13,16 +13,33 @@
>  #include <linux/irq.h>
>  #include <linux/input/touchscreen.h>
>  #include <linux/interrupt.h>
> +#include <linux/reset.h>
>  #include <linux/of.h>
>  #include <asm/unaligned.h>
>  
> +// Registers

Use /* */ comments.

>  #define WACOM_DESC_REG	0x01
> -#define WACOM_CMD_QUERY0	0x04
> -#define WACOM_CMD_QUERY1	0x00
> -#define WACOM_CMD_QUERY2	0x33
> -#define WACOM_CMD_QUERY3	0x02
> -#define WACOM_CMD_THROW0	0x05
> -#define WACOM_CMD_THROW1	0x00
> +#define WACOM_COMMAND_LSB   0x04
> +#define WACOM_COMMAND_MSB   0x00
> +
> +#define WACOM_DATA_LSB      0x05
> +#define WACOM_DATA_MSB      0x00

You use spaces here. Should be tabs.

> +
> +// Report types
> +#define REPORT_FEATURE      0x30
> +
> +// Requests / operations
> +#define OPCODE_GET_REPORT   0x02
> +
> +// Power settings
> +#define POWER_ON            0x00
> +#define POWER_SLEEP         0x01
> +
> +// Input report ids
> +#define WACOM_PEN_DATA_REPORT           2
> +#define WACOM_SHINONOME_REPORT          26
> +
> +#define WACOM_QUERY_REPORT	3
>  #define WACOM_QUERY_SIZE	22
>  
>  #define WACOM_MAX_DATA_SIZE_BG9     10
> @@ -85,27 +102,30 @@ static int wacom_query_device(struct i2c_client *client,
>  {
>  	int ret;
>  	u8 cmd_wac_desc[] = {WACOM_DESC_REG, 0x00};
> -	u8 cmd1[] = { WACOM_CMD_QUERY0, WACOM_CMD_QUERY1,
> -			WACOM_CMD_QUERY2, WACOM_CMD_QUERY3 };
> -	u8 cmd2[] = { WACOM_CMD_THROW0, WACOM_CMD_THROW1 };
>  	u8 data[WACOM_QUERY_SIZE];
> +
> +	u8 get_query_data_cmd[] = {
> +		WACOM_COMMAND_LSB,
> +		WACOM_COMMAND_MSB,
> +		REPORT_FEATURE | WACOM_QUERY_REPORT,
> +		OPCODE_GET_REPORT,
> +		WACOM_DATA_LSB,
> +		WACOM_DATA_MSB,
> +	};
> +
>  	struct i2c_msg msgs[] = {
> +		// Request reading of feature ReportID: 3 (Pen Query Data)

//

>  		{
>  			.addr = client->addr,
>  			.flags = 0,
> -			.len = sizeof(cmd1),
> -			.buf = cmd1,
> -		},
> -		{
> -			.addr = client->addr,
> -			.flags = 0,
> -			.len = sizeof(cmd2),
> -			.buf = cmd2,
> +			.len = sizeof(get_query_data_cmd),
> +			.buf = get_query_data_cmd,
>  		},
> +		// Read 21 bytes

//

>  		{
>  			.addr = client->addr,
>  			.flags = I2C_M_RD,
> -			.len = sizeof(data),
> +			.len = WACOM_QUERY_SIZE - 1,
>  			.buf = data,
>  		},
>  	};
> @@ -158,9 +178,12 @@ static int wacom_query_device(struct i2c_client *client,
>  	}
>  
>  	dev_dbg(&client->dev,
> -		"x_max:%d, y_max:%d, pressure:%d, fw:%d\n",
> +		"x_max:%d, y_max:%d, pressure:%d, fw:%d, "
> +		"distance: %d, tilt_x_max: %d, tilt_y_max: %d\n",
>  		features->x_max, features->y_max,
> -		features->pressure_max, features->fw_version);
> +		features->pressure_max, features->fw_version,
> +		features->distance_max,
> +		features->tilt_x_max, features->tilt_y_max);
>  
>  	return 0;
>  }
> -- 
> 2.31.1
> 

[PATCH v10 08/12] Input: wacom_i2c - Add support for vdd regulator

[Alistair Francis]

Add support for a VDD regulator. This allows the kernel to probe the
Wacom-I2C device on the rM2.

Signed-off-by: Alistair Francis <alistair@alistair23.me>
---
 drivers/input/touchscreen/wacom_i2c.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/wacom_i2c.c b/drivers/input/touchscreen/wacom_i2c.c
index a35d8fda7e7b..13341e76368b 100644
--- a/drivers/input/touchscreen/wacom_i2c.c
+++ b/drivers/input/touchscreen/wacom_i2c.c
@@ -13,6 +13,7 @@
 #include <linux/irq.h>
 #include <linux/input/touchscreen.h>
 #include <linux/interrupt.h>
+#include <linux/regulator/consumer.h>
 #include <linux/reset.h>
 #include <linux/of.h>
 #include <asm/unaligned.h>
@@ -92,6 +93,7 @@ struct wacom_i2c {
 	struct input_dev *input;
 	struct touchscreen_properties props;
 	struct wacom_features features;
+	struct regulator *vdd;
 	u8 data[WACOM_QUERY_SIZE];
 	bool prox;
 	int tool;
@@ -278,6 +280,16 @@ static int wacom_i2c_probe(struct i2c_client *client,
 	if (!wac_i2c)
 		return -ENOMEM;
 
+	wac_i2c->vdd = regulator_get(&client->dev, "vdd");
+	if (IS_ERR(wac_i2c->vdd))
+		return PTR_ERR(wac_i2c->vdd);
+
+	error = regulator_enable(wac_i2c->vdd);
+	if (error) {
+		regulator_put(wac_i2c->vdd);
+		return error;
+	}
+
 	features = &wac_i2c->features;
 	error = wacom_query_device(client, features);
 	if (error)
@@ -286,8 +298,11 @@ static int wacom_i2c_probe(struct i2c_client *client,
 	wac_i2c->client = client;
 
 	input = devm_input_allocate_device(dev);
-	if (!input)
+	if (!input) {
+		regulator_disable(wac_i2c->vdd);
+		regulator_put(wac_i2c->vdd);
 		return -ENOMEM;
+	}
 
 	wac_i2c->input = input;
 
@@ -322,6 +337,8 @@ static int wacom_i2c_probe(struct i2c_client *client,
 					  IRQF_ONESHOT, "wacom_i2c", wac_i2c);
 	if (error) {
 		dev_err(dev, "Failed to request IRQ: %d\n", error);
+		regulator_disable(wac_i2c->vdd);
+		regulator_put(wac_i2c->vdd);
 		return error;
 	}
 
@@ -331,6 +348,8 @@ static int wacom_i2c_probe(struct i2c_client *client,
 	error = input_register_device(wac_i2c->input);
 	if (error) {
 		dev_err(dev, "Failed to register input device: %d\n", error);
+		regulator_disable(wac_i2c->vdd);
+		regulator_put(wac_i2c->vdd);
 		return error;
 	}
 
-- 
2.31.1

[PATCH v10 09/12] Input: wacom_i2c - Use macros for the bit masks

[Alistair Francis]

To make the code easier to read use macros for the bit masks.

Signed-off-by: Alistair Francis <alistair@alistair23.me>
---
 drivers/input/touchscreen/wacom_i2c.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/input/touchscreen/wacom_i2c.c b/drivers/input/touchscreen/wacom_i2c.c
index 13341e76368b..f78a43212901 100644
--- a/drivers/input/touchscreen/wacom_i2c.c
+++ b/drivers/input/touchscreen/wacom_i2c.c
@@ -18,6 +18,14 @@
 #include <linux/of.h>
 #include <asm/unaligned.h>
 
+// Bitmasks (for data[3])
+#define WACOM_TIP_SWITCH_bm         (1 << 0)
+#define WACOM_BARREL_SWITCH_bm      (1 << 1)
+#define WACOM_ERASER_bm             (1 << 2)
+#define WACOM_INVERT_bm             (1 << 3)
+#define WACOM_BARREL_SWITCH_2_bm    (1 << 4)
+#define WACOM_IN_RANGE_bm           (1 << 5)
+
 // Registers
 #define WACOM_DESC_REG	0x01
 #define WACOM_COMMAND_LSB   0x04
@@ -206,10 +214,10 @@ static irqreturn_t wacom_i2c_irq(int irq, void *dev_id)
 	if (error < 0)
 		goto out;
 
-	tsw = data[3] & 0x01;
-	ers = data[3] & 0x04;
-	f1 = data[3] & 0x02;
-	f2 = data[3] & 0x10;
+	tsw = data[3] & WACOM_TIP_SWITCH_bm;
+	ers = data[3] & WACOM_ERASER_bm;
+	f1 = data[3] & WACOM_BARREL_SWITCH_bm;
+	f2 = data[3] & WACOM_BARREL_SWITCH_2_bm;
 	x = le16_to_cpup((__le16 *)&data[4]);
 	y = le16_to_cpup((__le16 *)&data[6]);
 	pressure = le16_to_cpup((__le16 *)&data[8]);
-- 
2.31.1

[PATCH v10 10/12] Input: wacom_i2c - Allow flipping the values from the DT

[Alistair Francis]

Allow the device tree properties to flip the tilx, position or distance
values.

This is required for the stylus to work correctly on the reMarkable 2.

Signed-off-by: Alistair Francis <alistair@alistair23.me>
---
 .../input/touchscreen/wacom,generic.yaml      | 20 +++++++++++
 drivers/input/touchscreen/wacom_i2c.c         | 34 +++++++++++++++++++
 2 files changed, 54 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/wacom,generic.yaml b/Documentation/devicetree/bindings/input/touchscreen/wacom,generic.yaml
index a8a7f362b0ce..d451c9a235b6 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/wacom,generic.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/wacom,generic.yaml
@@ -25,6 +25,26 @@ properties:
   vdd-supply:
     description: Power Supply
 
+  flip-tilt-x:
+    type: boolean
+    description: Flip the tilt X values read from device
+
+  flip-tilt-y:
+    type: boolean
+    description: Flip the tilt Y values read from device
+
+  flip-pos-x:
+    type: boolean
+    description: Flip the X position values read from device
+
+  flip-pos-y:
+    type: boolean
+    description: Flip the Y position values read from device
+
+  flip-distance:
+    type: boolean
+    description: Flip the distance values read from device
+
 required:
   - compatible
   - reg
diff --git a/drivers/input/touchscreen/wacom_i2c.c b/drivers/input/touchscreen/wacom_i2c.c
index f78a43212901..dd8fa351c6e4 100644
--- a/drivers/input/touchscreen/wacom_i2c.c
+++ b/drivers/input/touchscreen/wacom_i2c.c
@@ -105,6 +105,12 @@ struct wacom_i2c {
 	u8 data[WACOM_QUERY_SIZE];
 	bool prox;
 	int tool;
+
+	bool flip_tilt_x;
+	bool flip_tilt_y;
+	bool flip_pos_x;
+	bool flip_pos_y;
+	bool flip_distance;
 };
 
 static int wacom_query_device(struct i2c_client *client,
@@ -198,6 +204,25 @@ static int wacom_query_device(struct i2c_client *client,
 	return 0;
 }
 
+static void wacom_of_read(struct wacom_i2c *wac_i2c)
+{
+	if (!IS_ENABLED(CONFIG_OF)) {
+		struct i2c_client *client = wac_i2c->client;
+
+		wac_i2c->flip_tilt_x = of_property_read_bool(client->dev.of_node, "flip-tilt-x");
+		wac_i2c->flip_tilt_y = of_property_read_bool(client->dev.of_node, "flip-tilt-y");
+		wac_i2c->flip_pos_x = of_property_read_bool(client->dev.of_node, "flip-pos-x");
+		wac_i2c->flip_pos_y = of_property_read_bool(client->dev.of_node, "flip-pos-y");
+		wac_i2c->flip_distance = of_property_read_bool(client->dev.of_node, "flip-distance");
+	} else {
+		wac_i2c->flip_tilt_x = false;
+		wac_i2c->flip_tilt_y = false;
+		wac_i2c->flip_pos_x = false;
+		wac_i2c->flip_pos_y = false;
+		wac_i2c->flip_distance = false;
+	}
+}
+
 static irqreturn_t wacom_i2c_irq(int irq, void *dev_id)
 {
 	struct wacom_i2c *wac_i2c = dev_id;
@@ -234,6 +259,13 @@ static irqreturn_t wacom_i2c_irq(int irq, void *dev_id)
 
 	wac_i2c->prox = data[3] & 0x20;
 
+	// Flip the values based on properties from the device tree
+	distance = wac_i2c->flip_distance ? -distance : distance;
+	x = wac_i2c->flip_pos_x ? (features->x_max - x) : x;
+	y = wac_i2c->flip_pos_y ? (features->y_max - y) : y;
+	tilt_x = wac_i2c->flip_tilt_x ? -tilt_x : tilt_x;
+	tilt_y = wac_i2c->flip_tilt_y ? -tilt_y : tilt_y;
+
 	touchscreen_report_pos(input, &wac_i2c->props, features->x_max,
 			       features->y_max, true);
 	input_report_key(input, BTN_TOUCH, tsw || ers);
@@ -361,6 +393,8 @@ static int wacom_i2c_probe(struct i2c_client *client,
 		return error;
 	}
 
+	wacom_of_read(wac_i2c);
+
 	return 0;
 }
 
-- 
2.31.1

Re: [PATCH v10 10/12] Input: wacom_i2c - Allow flipping the values from the DT

[Rob Herring]

On Sun, 29 Aug 2021 19:19:23 +1000, Alistair Francis wrote:
> Allow the device tree properties to flip the tilx, position or distance
> values.
> 
> This is required for the stylus to work correctly on the reMarkable 2.
> 
> Signed-off-by: Alistair Francis <alistair@alistair23.me>
> ---
>  .../input/touchscreen/wacom,generic.yaml      | 20 +++++++++++
>  drivers/input/touchscreen/wacom_i2c.c         | 34 +++++++++++++++++++
>  2 files changed, 54 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH v10 11/12] ARM: imx_v6_v7_defconfig: Enable Wacom I2C

[Alistair Francis]

Enable the Wacom I2C in the imx defconfig as it is used by the
reMarkable2 tablet.

Signed-off-by: Alistair Francis <alistair@alistair23.me>
---
 arch/arm/configs/imx_v6_v7_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/configs/imx_v6_v7_defconfig b/arch/arm/configs/imx_v6_v7_defconfig
index 079fcd8d1d11..477dac1edc75 100644
--- a/arch/arm/configs/imx_v6_v7_defconfig
+++ b/arch/arm/configs/imx_v6_v7_defconfig
@@ -176,6 +176,7 @@ CONFIG_TOUCHSCREEN_DA9052=y
 CONFIG_TOUCHSCREEN_EGALAX=y
 CONFIG_TOUCHSCREEN_GOODIX=y
 CONFIG_TOUCHSCREEN_ILI210X=y
+CONFIG_TOUCHSCREEN_WACOM_I2C=y
 CONFIG_TOUCHSCREEN_MAX11801=y
 CONFIG_TOUCHSCREEN_IMX6UL_TSC=y
 CONFIG_TOUCHSCREEN_EDT_FT5X06=y
-- 
2.31.1

[PATCH v10 12/12] ARM: dts: imx7d: remarkable2: add wacom digitizer device

[Alistair Francis]

Enable the wacom_i2c touchscreen for the reMarkable2.

Signed-off-by: Alistair Francis <alistair@alistair23.me>
---
 arch/arm/boot/dts/imx7d-remarkable2.dts | 61 +++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/arch/arm/boot/dts/imx7d-remarkable2.dts b/arch/arm/boot/dts/imx7d-remarkable2.dts
index 89cbf13097a4..052f9da32398 100644
--- a/arch/arm/boot/dts/imx7d-remarkable2.dts
+++ b/arch/arm/boot/dts/imx7d-remarkable2.dts
@@ -34,6 +34,19 @@ reg_brcm: regulator-brcm {
 		startup-delay-us = <150>;
 	};
 
+	reg_digitizer: regulator-digitizer {
+		compatible = "regulator-fixed";
+		regulator-name = "VDD_3V3_DIGITIZER";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+		pinctrl-names = "default", "sleep";
+		pinctrl-0 = <&pinctrl_digitizer_reg>;
+		pinctrl-1 = <&pinctrl_digitizer_reg>;
+		gpio = <&gpio1 6 GPIO_ACTIVE_HIGH>;
+		enable-active-high;
+		startup-delay-us = <100000>; /* 100 ms */
+	};
+
 	wifi_pwrseq: wifi_pwrseq {
 		compatible = "mmc-pwrseq-simple";
 		pinctrl-names = "default";
@@ -51,6 +64,28 @@ &clks {
 	assigned-clock-rates = <0>, <32768>;
 };
 
+&i2c1 {
+	clock-frequency = <400000>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_i2c1>;
+	status = "okay";
+
+	wacom_digitizer: digitizer@9 {
+		compatible = "wacom,i2c-30";
+		reg = <0x09>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_wacom>;
+		interrupt-parent = <&gpio1>;
+		interrupts = <1 IRQ_TYPE_LEVEL_LOW>;
+		flip-tilt-x;
+		flip-tilt-y;
+		flip-pos-x;
+		flip-pos-y;
+		flip-distance;
+		vdd-supply = <&reg_digitizer>;
+	};
+};
+
 &snvs_pwrkey {
 	status = "okay";
 };
@@ -117,6 +152,25 @@ &wdog1 {
 	fsl,ext-reset-output;
 };
 
+&iomuxc_lpsr {
+	pinctrl_digitizer_reg: digitizerreggrp {
+		fsl,pins = <
+			/* DIGITIZER_PWR_EN */
+			MX7D_PAD_LPSR_GPIO1_IO06__GPIO1_IO6	0x14
+		>;
+	};
+
+	pinctrl_wacom: wacomgrp {
+		fsl,pins = <
+			/*MX7D_PAD_LPSR_GPIO1_IO05__GPIO1_IO5	0x00000014 /* FWE */
+			MX7D_PAD_LPSR_GPIO1_IO04__GPIO1_IO4	0x00000074 /* PDCTB */
+			MX7D_PAD_LPSR_GPIO1_IO01__GPIO1_IO1	0x00000034 /* WACOM INT */
+			/*MX7D_PAD_LPSR_GPIO1_IO06__GPIO1_IO6	0x00000014 /* WACOM PWR ENABLE */
+			/*MX7D_PAD_LPSR_GPIO1_IO00__GPIO1_IO0	0x00000074 /* WACOM RESET */
+		>;
+	};
+};
+
 &iomuxc {
 	pinctrl_brcm_reg: brcmreggrp {
 		fsl,pins = <
@@ -125,6 +179,13 @@ MX7D_PAD_SAI1_TX_BCLK__GPIO6_IO13	0x14
 		>;
 	};
 
+	pinctrl_i2c1: i2c1grp {
+		fsl,pins = <
+			MX7D_PAD_I2C1_SDA__I2C1_SDA		0x4000007f
+			MX7D_PAD_I2C1_SCL__I2C1_SCL		0x4000007f
+		>;
+	};
+
 	pinctrl_uart1: uart1grp {
 		fsl,pins = <
 			MX7D_PAD_UART1_TX_DATA__UART1_DCE_TX	0x79
-- 
2.31.1