Re: [PATCH v11 1/4] HID: wacom_sys: Add support for flipping the data values

Hi Ping,

On Mon, Oct 18, 2021 at 10:41:55AM -0700, Ping Cheng wrote:

Hi Alistair,

On Sat, Oct 9, 2021, 4:44 AM Alistair Francis [off-list ref]
wrote:

quoted

Add support to the Wacom HID device for flipping the values based on
device tree settings. This allows us to support devices where the panel
is installed in a different orientation, such as the reMarkable2.

This device was designed for hid-generic driver, if it's not driven by
wacom_i2c.c or an out of tree driver.

wacom.ko doesn't support vid 0x2d1f devices.

I am really confused about this distinction. Could you please elaborate
why wacom driver only supports 0x056a (and, curiously, some Lenovo)
devices.

Thanks.

Nacked-by: Ping Cheng [off-list ref]

Sorry about that,
Ping

Signed-off-by: Alistair Francis <redacted>

quoted

---
 .../bindings/input/hid-over-i2c.txt           | 20 ++++++
 drivers/hid/wacom_sys.c                       | 25 ++++++++
 drivers/hid/wacom_wac.c                       | 61 +++++++++++++++++++
 drivers/hid/wacom_wac.h                       | 13 ++++
 4 files changed, 119 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/hid-over-i2c.txt

b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
index c76bafaf98d2..16ebd7c46049 100644

--- a/Documentation/devicetree/bindings/input/hid-over-i2c.txt
+++ b/Documentation/devicetree/bindings/input/hid-over-i2c.txt

@@ -33,6 +33,26 @@ device-specific compatible properties, which should be

used in addition to the
 - post-power-on-delay-ms: time required by the device after enabling its
regulators
   or powering it on, before it is ready for communication.

+  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

Do these really need to be controlled separately from the main
touchcsreen orientation?

quoted

+
+  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

We already have touchscreen-inverted-x/y defined in
Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml,
why are they not sufficient?

quoted

+
+  flip-distance:
+    type: boolean
+    description: Flip the distance values read from device

I am still confused of the notion of flipped distance.

quoted

+
 Example:

        i2c-hid-dev@2c {

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 93f49b766376..47d9590b10bd 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c

@@ -10,6 +10,7 @@

 #include "wacom_wac.h"
 #include "wacom.h"
+#include <linux/of.h>
 #include <linux/input/mt.h>

 #define WAC_MSG_RETRIES                5

@@ -2730,6 +2731,28 @@ static void wacom_mode_change_work(struct

work_struct *work)
        return;
 }

+static void wacom_of_read(struct hid_device *hdev, struct wacom_wac
*wacom_wac)
+{
+       if (IS_ENABLED(CONFIG_OF)) {
+               wacom_wac->flip_tilt_x =
of_property_read_bool(hdev->dev.parent->of_node,
+                                                       "flip-tilt-x");
+               wacom_wac->flip_tilt_y =
of_property_read_bool(hdev->dev.parent->of_node,
+                                                       "flip-tilt-y");
+               wacom_wac->flip_pos_x =
of_property_read_bool(hdev->dev.parent->of_node,
+                                                       "flip-pos-x");
+               wacom_wac->flip_pos_y =
of_property_read_bool(hdev->dev.parent->of_node,
+                                                       "flip-pos-y");
+               wacom_wac->flip_distance =
of_property_read_bool(hdev->dev.parent->of_node,
+                                                       "flip-distance");
+       } else {
+               wacom_wac->flip_tilt_x = false;
+               wacom_wac->flip_tilt_y = false;
+               wacom_wac->flip_pos_x = false;
+               wacom_wac->flip_pos_y = false;
+               wacom_wac->flip_distance = false;
+       }
+}
+
 static int wacom_probe(struct hid_device *hdev,
                const struct hid_device_id *id)
 {

@@ -2797,6 +2820,8 @@ static int wacom_probe(struct hid_device *hdev,
                                 error);
        }

+       wacom_of_read(hdev, wacom_wac);
+
        wacom_wac->probe_complete = true;
        return 0;
 }

diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 33a6908995b1..c01f683e23fa 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c

@@ -3261,6 +3261,63 @@ static int wacom_status_irq(struct wacom_wac

*wacom_wac, size_t len)
        return 0;
 }

+static int wacom_of_irq(struct wacom_wac *wacom_wac, size_t len)
+{
+       const struct wacom_features *features = &wacom_wac->features;
+       unsigned char *data = wacom_wac->data;
+       struct input_dev *input = wacom_wac->pen_input;
+       unsigned int x, y, pressure;
+       unsigned char tsw, f1, f2, ers;
+       short tilt_x, tilt_y, distance;
+
+       if (!IS_ENABLED(CONFIG_OF))
+               return 0;
+
+       tsw = data[1] & WACOM_TIP_SWITCH_bm;
+       ers = data[1] & WACOM_ERASER_bm;
+       f1 = data[1] & WACOM_BARREL_SWITCH_bm;
+       f2 = data[1] & WACOM_BARREL_SWITCH_2_bm;
+       x = le16_to_cpup((__le16 *)&data[2]);
+       y = le16_to_cpup((__le16 *)&data[4]);
+       pressure = le16_to_cpup((__le16 *)&data[6]);
+
+       /* Signed */
+       tilt_x = get_unaligned_le16(&data[9]);
+       tilt_y = get_unaligned_le16(&data[11]);
+
+       distance = get_unaligned_le16(&data[13]);

You are still parsing raw data. The point of HID is to provide common
framework for scaling raw values.

Thanks.

-- 
Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Hi Dmitry,

I am really confused about this distinction. Could you please elaborate why wacom driver only supports 0x056a (and, curiously, some Lenovo) devices.

We want 0x2d1F to work with ***ONLY*** with the generic HID driver because the every recent firmware is designed for that; no help is needed from other sides.
In contrast, the most of the devices is required help from other sides. Therefore, the devices with VID 0x2D1F devices are supposed to run alone and separated out from our brand shipped products. 
I don't know how much we can go further to tell you about this -because of our business, but 0x2D1F was obtained such purpose. Your understanding is much appreciated.

Thanks,

Tats

-----Original Message-----
From: Dmitry Torokhov <dmitry.torokhov@gmail.com> 
Sent: Tuesday, October 19, 2021 10:51 AM
To: Ping Cheng <redacted>
Cc: Alistair Francis <redacted>; shawnguo@kernel.org; s.hauer@pengutronix.de; linux-imx@nxp.com; Jiri Kosina <jikos@kernel.org>; Benjamin Tissoires <redacted>; linux-input <redacted>; devicetree@vger.kernel.org; LKML <redacted>; linux-arm-kernel@lists.infradead.org; alistair23@gmail.com
Subject: Re: [PATCH v11 1/4] HID: wacom_sys: Add support for flipping the data values

[EXTERNAL]

Hi Ping,

On Mon, Oct 18, 2021 at 10:41:55AM -0700, Ping Cheng wrote:

Hi Alistair,

On Sat, Oct 9, 2021, 4:44 AM Alistair Francis [off-list ref]
wrote:

quoted

Add support to the Wacom HID device for flipping the values based on 
device tree settings. This allows us to support devices where the 
panel is installed in a different orientation, such as the reMarkable2.

This device was designed for hid-generic driver, if it's not driven by 
wacom_i2c.c or an out of tree driver.

wacom.ko doesn't support vid 0x2d1f devices.

I am really confused about this distinction. Could you please elaborate why wacom driver only supports 0x056a (and, curiously, some Lenovo) devices.

Thanks.

Nacked-by: Ping Cheng [off-list ref]

Sorry about that,
Ping

Signed-off-by: Alistair Francis <redacted>

quoted

---
 .../bindings/input/hid-over-i2c.txt           | 20 ++++++
 drivers/hid/wacom_sys.c                       | 25 ++++++++
 drivers/hid/wacom_wac.c                       | 61 +++++++++++++++++++
 drivers/hid/wacom_wac.h                       | 13 ++++
 4 files changed, 119 insertions(+)

diff --git 

a/Documentation/devicetree/bindings/input/hid-over-i2c.txt
b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
index c76bafaf98d2..16ebd7c46049 100644

--- a/Documentation/devicetree/bindings/input/hid-over-i2c.txt
+++ b/Documentation/devicetree/bindings/input/hid-over-i2c.txt

@@ -33,6 +33,26 @@ device-specific compatible properties, which 

should be used in addition to the
 - post-power-on-delay-ms: time required by the device after 
enabling its regulators
   or powering it on, before it is ready for communication.

+  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

Do these really need to be controlled separately from the main touchcsreen orientation?

quoted

+
+  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

We already have touchscreen-inverted-x/y defined in Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml,
why are they not sufficient?

quoted

+
+  flip-distance:
+    type: boolean
+    description: Flip the distance values read from device

I am still confused of the notion of flipped distance.

quoted

+
 Example:

        i2c-hid-dev@2c {

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c index 

93f49b766376..47d9590b10bd 100644

--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c

@@ -10,6 +10,7 @@

 #include "wacom_wac.h"
 #include "wacom.h"
+#include <linux/of.h>
 #include <linux/input/mt.h>

 #define WAC_MSG_RETRIES                5

@@ -2730,6 +2731,28 @@ static void wacom_mode_change_work(struct 

work_struct *work)
        return;
 }

+static void wacom_of_read(struct hid_device *hdev, struct wacom_wac
*wacom_wac)
+{
+       if (IS_ENABLED(CONFIG_OF)) {
+               wacom_wac->flip_tilt_x =
of_property_read_bool(hdev->dev.parent->of_node,
+                                                       "flip-tilt-x");
+               wacom_wac->flip_tilt_y =
of_property_read_bool(hdev->dev.parent->of_node,
+                                                       "flip-tilt-y");
+               wacom_wac->flip_pos_x =
of_property_read_bool(hdev->dev.parent->of_node,
+                                                       "flip-pos-x");
+               wacom_wac->flip_pos_y =
of_property_read_bool(hdev->dev.parent->of_node,
+                                                       "flip-pos-y");
+               wacom_wac->flip_distance =
of_property_read_bool(hdev->dev.parent->of_node,
+                                                       "flip-distance");
+       } else {
+               wacom_wac->flip_tilt_x = false;
+               wacom_wac->flip_tilt_y = false;
+               wacom_wac->flip_pos_x = false;
+               wacom_wac->flip_pos_y = false;
+               wacom_wac->flip_distance = false;
+       }
+}
+
 static int wacom_probe(struct hid_device *hdev,
                const struct hid_device_id *id)  { @@ -2797,6 
+2820,8 @@ static int wacom_probe(struct hid_device *hdev,
                                 error);
        }

+       wacom_of_read(hdev, wacom_wac);
+
        wacom_wac->probe_complete = true;
        return 0;
 }

diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c index 

33a6908995b1..c01f683e23fa 100644

--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c

@@ -3261,6 +3261,63 @@ static int wacom_status_irq(struct wacom_wac 

*wacom_wac, size_t len)
        return 0;
 }

+static int wacom_of_irq(struct wacom_wac *wacom_wac, size_t len) {
+       const struct wacom_features *features = &wacom_wac->features;
+       unsigned char *data = wacom_wac->data;
+       struct input_dev *input = wacom_wac->pen_input;
+       unsigned int x, y, pressure;
+       unsigned char tsw, f1, f2, ers;
+       short tilt_x, tilt_y, distance;
+
+       if (!IS_ENABLED(CONFIG_OF))
+               return 0;
+
+       tsw = data[1] & WACOM_TIP_SWITCH_bm;
+       ers = data[1] & WACOM_ERASER_bm;
+       f1 = data[1] & WACOM_BARREL_SWITCH_bm;
+       f2 = data[1] & WACOM_BARREL_SWITCH_2_bm;
+       x = le16_to_cpup((__le16 *)&data[2]);
+       y = le16_to_cpup((__le16 *)&data[4]);
+       pressure = le16_to_cpup((__le16 *)&data[6]);
+
+       /* Signed */
+       tilt_x = get_unaligned_le16(&data[9]);
+       tilt_y = get_unaligned_le16(&data[11]);
+
+       distance = get_unaligned_le16(&data[13]);

You are still parsing raw data. The point of HID is to provide common framework for scaling raw values.

Thanks.

--
Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Tue, Oct 19, 2021 at 11:51 AM Dmitry Torokhov
[off-list ref] wrote:

Hi Ping,

On Mon, Oct 18, 2021 at 10:41:55AM -0700, Ping Cheng wrote:

quoted

Hi Alistair,

On Sat, Oct 9, 2021, 4:44 AM Alistair Francis [off-list ref]
wrote:

quoted

Add support to the Wacom HID device for flipping the values based on
device tree settings. This allows us to support devices where the panel
is installed in a different orientation, such as the reMarkable2.

This device was designed for hid-generic driver, if it's not driven by
wacom_i2c.c or an out of tree driver.

wacom.ko doesn't support vid 0x2d1f devices.

I am really confused about this distinction. Could you please elaborate
why wacom driver only supports 0x056a (and, curiously, some Lenovo)
devices.

Thanks.

quoted

Nacked-by: Ping Cheng [off-list ref]

Sorry about that,
Ping

Signed-off-by: Alistair Francis <redacted>

quoted

---
 .../bindings/input/hid-over-i2c.txt           | 20 ++++++
 drivers/hid/wacom_sys.c                       | 25 ++++++++
 drivers/hid/wacom_wac.c                       | 61 +++++++++++++++++++
 drivers/hid/wacom_wac.h                       | 13 ++++
 4 files changed, 119 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/hid-over-i2c.txt

b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
index c76bafaf98d2..16ebd7c46049 100644

--- a/Documentation/devicetree/bindings/input/hid-over-i2c.txt
+++ b/Documentation/devicetree/bindings/input/hid-over-i2c.txt

@@ -33,6 +33,26 @@ device-specific compatible properties, which should be

used in addition to the
 - post-power-on-delay-ms: time required by the device after enabling its
regulators
   or powering it on, before it is ready for communication.

+  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

Do these really need to be controlled separately from the main
touchcsreen orientation?

I don't think so actually.

quoted

quoted

+
+  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

We already have touchscreen-inverted-x/y defined in
Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml,
why are they not sufficient?

The touchscreen-* properties aren't applied to HID devices though, at
least not that I can tell.

Alistair

quoted

quoted

+
+  flip-distance:
+    type: boolean
+    description: Flip the distance values read from device

I am still confused of the notion of flipped distance.

quoted

quoted

+
 Example:

        i2c-hid-dev@2c {

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 93f49b766376..47d9590b10bd 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c

@@ -10,6 +10,7 @@

 #include "wacom_wac.h"
 #include "wacom.h"
+#include <linux/of.h>
 #include <linux/input/mt.h>

 #define WAC_MSG_RETRIES                5

@@ -2730,6 +2731,28 @@ static void wacom_mode_change_work(struct

work_struct *work)
        return;
 }

+static void wacom_of_read(struct hid_device *hdev, struct wacom_wac
*wacom_wac)
+{
+       if (IS_ENABLED(CONFIG_OF)) {
+               wacom_wac->flip_tilt_x =
of_property_read_bool(hdev->dev.parent->of_node,
+                                                       "flip-tilt-x");
+               wacom_wac->flip_tilt_y =
of_property_read_bool(hdev->dev.parent->of_node,
+                                                       "flip-tilt-y");
+               wacom_wac->flip_pos_x =
of_property_read_bool(hdev->dev.parent->of_node,
+                                                       "flip-pos-x");
+               wacom_wac->flip_pos_y =
of_property_read_bool(hdev->dev.parent->of_node,
+                                                       "flip-pos-y");
+               wacom_wac->flip_distance =
of_property_read_bool(hdev->dev.parent->of_node,
+                                                       "flip-distance");
+       } else {
+               wacom_wac->flip_tilt_x = false;
+               wacom_wac->flip_tilt_y = false;
+               wacom_wac->flip_pos_x = false;
+               wacom_wac->flip_pos_y = false;
+               wacom_wac->flip_distance = false;
+       }
+}
+
 static int wacom_probe(struct hid_device *hdev,
                const struct hid_device_id *id)
 {

@@ -2797,6 +2820,8 @@ static int wacom_probe(struct hid_device *hdev,
                                 error);
        }

+       wacom_of_read(hdev, wacom_wac);
+
        wacom_wac->probe_complete = true;
        return 0;
 }

diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 33a6908995b1..c01f683e23fa 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c

@@ -3261,6 +3261,63 @@ static int wacom_status_irq(struct wacom_wac

*wacom_wac, size_t len)
        return 0;
 }

+static int wacom_of_irq(struct wacom_wac *wacom_wac, size_t len)
+{
+       const struct wacom_features *features = &wacom_wac->features;
+       unsigned char *data = wacom_wac->data;
+       struct input_dev *input = wacom_wac->pen_input;
+       unsigned int x, y, pressure;
+       unsigned char tsw, f1, f2, ers;
+       short tilt_x, tilt_y, distance;
+
+       if (!IS_ENABLED(CONFIG_OF))
+               return 0;
+
+       tsw = data[1] & WACOM_TIP_SWITCH_bm;
+       ers = data[1] & WACOM_ERASER_bm;
+       f1 = data[1] & WACOM_BARREL_SWITCH_bm;
+       f2 = data[1] & WACOM_BARREL_SWITCH_2_bm;
+       x = le16_to_cpup((__le16 *)&data[2]);
+       y = le16_to_cpup((__le16 *)&data[4]);
+       pressure = le16_to_cpup((__le16 *)&data[6]);
+
+       /* Signed */
+       tilt_x = get_unaligned_le16(&data[9]);
+       tilt_y = get_unaligned_le16(&data[11]);
+
+       distance = get_unaligned_le16(&data[13]);

You are still parsing raw data. The point of HID is to provide common
framework for scaling raw values.

Thanks.

--
Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Wed, Oct 20, 2021 at 09:33:13AM +1000, Alistair Francis wrote:

On Tue, Oct 19, 2021 at 11:51 AM Dmitry Torokhov
[off-list ref] wrote:

quoted

We already have touchscreen-inverted-x/y defined in
Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml,
why are they not sufficient?

The touchscreen-* properties aren't applied to HID devices though, at
least not that I can tell.

No, they are not currently, but that does not mean we need to establish
a new set of properties (property names) for HID case.

Thanks.

-- 
Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Wed, Oct 20, 2021 at 11:05 AM Dmitry Torokhov
[off-list ref] wrote:

On Wed, Oct 20, 2021 at 09:33:13AM +1000, Alistair Francis wrote:

quoted

On Tue, Oct 19, 2021 at 11:51 AM Dmitry Torokhov
[off-list ref] wrote:

quoted

We already have touchscreen-inverted-x/y defined in
Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml,
why are they not sufficient?

The touchscreen-* properties aren't applied to HID devices though, at
least not that I can tell.

No, they are not currently, but that does not mean we need to establish
a new set of properties (property names) for HID case.

I can update the names to use the existing touchscreen ones.

Do you have a hint of where this should be implemented though?

Right now (without "HID: wacom: Add support for the AG14 Wacom
device") the wacom touchscreen is just registered as a generic HID
device. I don't see any good place in hid-core, hid-input or
hid-generic to invert the input values for this.

Even with my "HID: wacom: Add support for the AG14 Wacom device" patch
I don't see a good way of doing this without the raw data parsing.

Alistair

Thanks.

--
Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Wed, Oct 20, 2021 at 11:44:50AM +1000, Alistair Francis wrote:

On Wed, Oct 20, 2021 at 11:05 AM Dmitry Torokhov
[off-list ref] wrote:

quoted

On Wed, Oct 20, 2021 at 09:33:13AM +1000, Alistair Francis wrote:

quoted

On Tue, Oct 19, 2021 at 11:51 AM Dmitry Torokhov
[off-list ref] wrote:

quoted

We already have touchscreen-inverted-x/y defined in
Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml,
why are they not sufficient?

The touchscreen-* properties aren't applied to HID devices though, at
least not that I can tell.

No, they are not currently, but that does not mean we need to establish
a new set of properties (property names) for HID case.

I can update the names to use the existing touchscreen ones.

Do you have a hint of where this should be implemented though?

Right now (without "HID: wacom: Add support for the AG14 Wacom
device") the wacom touchscreen is just registered as a generic HID
device. I don't see any good place in hid-core, hid-input or
hid-generic to invert the input values for this.

I think the transformation should happen in
hid-multitouch.c::mt_process_slot() using helpers from
include/linux/input/touchscreen.h

I think the more challenging question is to how pass/attach struct
touchscreen_properties * to the hid device (i expect the properties will
be attached to i2c-hid device, but maybe we could create a sub-node of
it and attach properties there.

Thanks.

-- 
Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Wed, Oct 20, 2021 at 4:14 AM Dmitry Torokhov
[off-list ref] wrote:

On Wed, Oct 20, 2021 at 11:44:50AM +1000, Alistair Francis wrote:

quoted

On Wed, Oct 20, 2021 at 11:05 AM Dmitry Torokhov
[off-list ref] wrote:

quoted

On Wed, Oct 20, 2021 at 09:33:13AM +1000, Alistair Francis wrote:

quoted

On Tue, Oct 19, 2021 at 11:51 AM Dmitry Torokhov
[off-list ref] wrote:

quoted

We already have touchscreen-inverted-x/y defined in
Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml,
why are they not sufficient?

The touchscreen-* properties aren't applied to HID devices though, at
least not that I can tell.

No, they are not currently, but that does not mean we need to establish
a new set of properties (property names) for HID case.

I can update the names to use the existing touchscreen ones.

Do you have a hint of where this should be implemented though?

Right now (without "HID: wacom: Add support for the AG14 Wacom
device") the wacom touchscreen is just registered as a generic HID
device. I don't see any good place in hid-core, hid-input or
hid-generic to invert the input values for this.

I think the transformation should happen in
hid-multitouch.c::mt_process_slot() using helpers from
include/linux/input/touchscreen.h

I think the more challenging question is to how pass/attach struct
touchscreen_properties * to the hid device (i expect the properties will
be attached to i2c-hid device, but maybe we could create a sub-node of
it and attach properties there.

Sorry but I don't like that very much. This would mean that we have an
out of band information that needs to be carried over to
HID-generic/multitouch and having tests for it is going to be harder.
I would rather have userspace deal with the rotation if we do not have
the information from the device itself.

Foreword: I have been given a hammer, so I see nails everywhere.

The past 3 weeks I have been working on implementing some eBPF hooks
in the HID subsystem. This would IMO be the best solution here: a udev
hwdb rule sees that there is the not-wacom PID/VID (and maybe the
platform or parses the OF properties if they are available in the
sysfs) and adds a couple of functions in the HID stack to rotate the
screen. The advantage is that we do not need to add a new kernel API
anymore, the disadvantage is that we need userspace to "fix" the
kernel behaviour (so at boot, this might be an issue).

I am not at the point where I can share the code as there is a lot of
rewriting and my last attempt is resulting in a page fault, but I'd be
happy to share it more once that hiccup is solved.

Cheers,
Benjamin


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Wed, Oct 20, 2021 at 5:40 PM Benjamin Tissoires
[off-list ref] wrote:

On Wed, Oct 20, 2021 at 4:14 AM Dmitry Torokhov
[off-list ref] wrote:

quoted

On Wed, Oct 20, 2021 at 11:44:50AM +1000, Alistair Francis wrote:

quoted

On Wed, Oct 20, 2021 at 11:05 AM Dmitry Torokhov
[off-list ref] wrote:

quoted

On Wed, Oct 20, 2021 at 09:33:13AM +1000, Alistair Francis wrote:

quoted

On Tue, Oct 19, 2021 at 11:51 AM Dmitry Torokhov
[off-list ref] wrote:

quoted

We already have touchscreen-inverted-x/y defined in
Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml,
why are they not sufficient?

The touchscreen-* properties aren't applied to HID devices though, at
least not that I can tell.

No, they are not currently, but that does not mean we need to establish
a new set of properties (property names) for HID case.

I can update the names to use the existing touchscreen ones.

Do you have a hint of where this should be implemented though?

Right now (without "HID: wacom: Add support for the AG14 Wacom
device") the wacom touchscreen is just registered as a generic HID
device. I don't see any good place in hid-core, hid-input or
hid-generic to invert the input values for this.

I think the transformation should happen in
hid-multitouch.c::mt_process_slot() using helpers from
include/linux/input/touchscreen.h

I think the more challenging question is to how pass/attach struct
touchscreen_properties * to the hid device (i expect the properties will
be attached to i2c-hid device, but maybe we could create a sub-node of
it and attach properties there.

Sorry but I don't like that very much. This would mean that we have an
out of band information that needs to be carried over to
HID-generic/multitouch and having tests for it is going to be harder.
I would rather have userspace deal with the rotation if we do not have
the information from the device itself.

My 2c below

Foreword: I have been given a hammer, so I see nails everywhere.

The past 3 weeks I have been working on implementing some eBPF hooks
in the HID subsystem. This would IMO be the best solution here: a udev
hwdb rule sees that there is the not-wacom PID/VID (and maybe the
platform or parses the OF properties if they are available in the

I'm not sure we have a specific VID/PID to work with here. The VID is
generic AFAIK, not sure about the PID though. Maybe someone from Wacom
could confirm either way.

sysfs) and adds a couple of functions in the HID stack to rotate the
screen. The advantage is that we do not need to add a new kernel API

I would say that touchscreen-inverted-x/y isn't a new API, it's
commonly used. To me it makes sense that it's supported for all
touchscreens.

anymore, the disadvantage is that we need userspace to "fix" the
kernel behaviour (so at boot, this might be an issue).

That's a pain for me. I'm still stuck with the vendors userspace as I
need their propiritory eInk driver code. It also seems like a hassle
for different distros to handle this (compared to just in the DT).

Alistair

I am not at the point where I can share the code as there is a lot of
rewriting and my last attempt is resulting in a page fault, but I'd be
happy to share it more once that hiccup is solved.

Cheers,
Benjamin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Wed, Oct 20, 2021 at 1:34 PM Alistair Francis [off-list ref] wrote:

On Wed, Oct 20, 2021 at 5:40 PM Benjamin Tissoires
[off-list ref] wrote:

quoted

On Wed, Oct 20, 2021 at 4:14 AM Dmitry Torokhov
[off-list ref] wrote:

quoted

On Wed, Oct 20, 2021 at 11:44:50AM +1000, Alistair Francis wrote:

quoted

On Wed, Oct 20, 2021 at 11:05 AM Dmitry Torokhov
[off-list ref] wrote:

quoted

On Wed, Oct 20, 2021 at 09:33:13AM +1000, Alistair Francis wrote:

quoted

On Tue, Oct 19, 2021 at 11:51 AM Dmitry Torokhov
[off-list ref] wrote:

quoted

We already have touchscreen-inverted-x/y defined in
Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml,
why are they not sufficient?

The touchscreen-* properties aren't applied to HID devices though, at
least not that I can tell.

No, they are not currently, but that does not mean we need to establish
a new set of properties (property names) for HID case.

I can update the names to use the existing touchscreen ones.

Do you have a hint of where this should be implemented though?

Right now (without "HID: wacom: Add support for the AG14 Wacom
device") the wacom touchscreen is just registered as a generic HID
device. I don't see any good place in hid-core, hid-input or
hid-generic to invert the input values for this.

I think the transformation should happen in
hid-multitouch.c::mt_process_slot() using helpers from
include/linux/input/touchscreen.h

I think the more challenging question is to how pass/attach struct
touchscreen_properties * to the hid device (i expect the properties will
be attached to i2c-hid device, but maybe we could create a sub-node of
it and attach properties there.

Sorry but I don't like that very much. This would mean that we have an
out of band information that needs to be carried over to
HID-generic/multitouch and having tests for it is going to be harder.
I would rather have userspace deal with the rotation if we do not have
the information from the device itself.

My 2c below

quoted

Foreword: I have been given a hammer, so I see nails everywhere.

The past 3 weeks I have been working on implementing some eBPF hooks
in the HID subsystem. This would IMO be the best solution here: a udev
hwdb rule sees that there is the not-wacom PID/VID (and maybe the
platform or parses the OF properties if they are available in the

I'm not sure we have a specific VID/PID to work with here. The VID is
generic AFAIK, not sure about the PID though. Maybe someone from Wacom
could confirm either way.

It actually doesn't really matter. What matters is that there is a way
to know that this device needs to be rotated, being through DT
properties that would be exported through sysfs, or a hwdb entry that
matches on the product, the platform or something else.

quoted

sysfs) and adds a couple of functions in the HID stack to rotate the
screen. The advantage is that we do not need to add a new kernel API

I would say that touchscreen-inverted-x/y isn't a new API, it's
commonly used. To me it makes sense that it's supported for all
touchscreens.

Well, it's new in the HID world, and this is opening the pandora box:
the patch adds only the equivalent of touchscreen-inverted-x/y, but
not touchscreen-swapped-x-y. So you can not actually rotate a screen
by 90 degrees.

Inverting a value on an axis is easy. Swapping 2 axes is way harder in
the HID world, because you have to interpret the report descriptor
differently.

Also, the patch adds 3 new properties: flip-tilt-x/y and flip-distance.
The tilt and distance would be easy, but suddenly we need to also add
pressure, and all of the other HID definitions. This is going to be
endless. It took me a while to understand Rob's point regarding
generic properties, but we are exactly entering this territory: this
is an endless chase and will never end.

I would much rather have a device specific quirk that would be
triggered by the DT than adding generic properties like that.

Also, hid-multitouch is the most tested driver through the hid-tools
test suite: https://gitlab.freedesktop.org/libevdev/hid-tools
I am not sure how I can add tests for those properties in a generic
way (the creation of the "virtual DT" is going to be problematic).

On the contrary, a device specific quirk can easily be tested without
having to mess too much with the hid subsystem.

quoted

anymore, the disadvantage is that we need userspace to "fix" the
kernel behaviour (so at boot, this might be an issue).

That's a pain for me. I'm still stuck with the vendors userspace as I
need their propiritory eInk driver code. It also seems like a hassle
for different distros to handle this (compared to just in the DT).

I understand the pain. But I am not talking about a 1 kernel cycle
release timeframe. More like 6-12 months to bring in all the pieces
together. Distributions have no issues with udev most of the time
(even those that stuck to the old pre-systemd fork), and it would not
be different than having a udev intrinsic that tags the pen with
ID_INPUT_TABLET so libinput and others can deal with it.

Cheers,
Benjamin

Alistair

quoted

I am not at the point where I can share the code as there is a lot of
rewriting and my last attempt is resulting in a page fault, but I'd be
happy to share it more once that hiccup is solved.

Cheers,
Benjamin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Wed, Oct 20, 2021 at 10:04 PM Benjamin Tissoires
[off-list ref] wrote:

On Wed, Oct 20, 2021 at 1:34 PM Alistair Francis [off-list ref] wrote:

quoted

On Wed, Oct 20, 2021 at 5:40 PM Benjamin Tissoires
[off-list ref] wrote:

quoted

On Wed, Oct 20, 2021 at 4:14 AM Dmitry Torokhov
[off-list ref] wrote:

quoted

On Wed, Oct 20, 2021 at 11:44:50AM +1000, Alistair Francis wrote:

quoted

On Wed, Oct 20, 2021 at 11:05 AM Dmitry Torokhov
[off-list ref] wrote:

quoted

On Wed, Oct 20, 2021 at 09:33:13AM +1000, Alistair Francis wrote:

quoted

On Tue, Oct 19, 2021 at 11:51 AM Dmitry Torokhov
[off-list ref] wrote:

quoted

We already have touchscreen-inverted-x/y defined in
Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml,
why are they not sufficient?

The touchscreen-* properties aren't applied to HID devices though, at
least not that I can tell.

No, they are not currently, but that does not mean we need to establish
a new set of properties (property names) for HID case.

I can update the names to use the existing touchscreen ones.

Do you have a hint of where this should be implemented though?

Right now (without "HID: wacom: Add support for the AG14 Wacom
device") the wacom touchscreen is just registered as a generic HID
device. I don't see any good place in hid-core, hid-input or
hid-generic to invert the input values for this.

I think the transformation should happen in
hid-multitouch.c::mt_process_slot() using helpers from
include/linux/input/touchscreen.h

I think the more challenging question is to how pass/attach struct
touchscreen_properties * to the hid device (i expect the properties will
be attached to i2c-hid device, but maybe we could create a sub-node of
it and attach properties there.

Sorry but I don't like that very much. This would mean that we have an
out of band information that needs to be carried over to
HID-generic/multitouch and having tests for it is going to be harder.
I would rather have userspace deal with the rotation if we do not have
the information from the device itself.

My 2c below

quoted

Foreword: I have been given a hammer, so I see nails everywhere.

The past 3 weeks I have been working on implementing some eBPF hooks
in the HID subsystem. This would IMO be the best solution here: a udev
hwdb rule sees that there is the not-wacom PID/VID (and maybe the
platform or parses the OF properties if they are available in the

I'm not sure we have a specific VID/PID to work with here. The VID is
generic AFAIK, not sure about the PID though. Maybe someone from Wacom
could confirm either way.

It actually doesn't really matter. What matters is that there is a way
to know that this device needs to be rotated, being through DT
properties that would be exported through sysfs, or a hwdb entry that
matches on the product, the platform or something else.

quoted

quoted

sysfs) and adds a couple of functions in the HID stack to rotate the
screen. The advantage is that we do not need to add a new kernel API

I would say that touchscreen-inverted-x/y isn't a new API, it's
commonly used. To me it makes sense that it's supported for all
touchscreens.

Well, it's new in the HID world, and this is opening the pandora box:
the patch adds only the equivalent of touchscreen-inverted-x/y, but
not touchscreen-swapped-x-y. So you can not actually rotate a screen
by 90 degrees.

Inverting a value on an axis is easy. Swapping 2 axes is way harder in
the HID world, because you have to interpret the report descriptor
differently.

Also, the patch adds 3 new properties: flip-tilt-x/y and flip-distance.

This patch does yes, but I'm happy to just drop this to the invert
touchscreen properties.

The tilt and distance would be easy, but suddenly we need to also add
pressure, and all of the other HID definitions. This is going to be
endless. It took me a while to understand Rob's point regarding
generic properties, but we are exactly entering this territory: this
is an endless chase and will never end.

I would much rather have a device specific quirk that would be
triggered by the DT than adding generic properties like that.

That works for me!

A HID_QUIRK_XY_INVERT would work for me and seems useful for others in
the future as well.

I managed to figure out how to do this, I'll send a patch soon.

Also, hid-multitouch is the most tested driver through the hid-tools
test suite: https://gitlab.freedesktop.org/libevdev/hid-tools
I am not sure how I can add tests for those properties in a generic
way (the creation of the "virtual DT" is going to be problematic).

On the contrary, a device specific quirk can easily be tested without
having to mess too much with the hid subsystem.

Great!

Alistair

quoted

quoted

anymore, the disadvantage is that we need userspace to "fix" the
kernel behaviour (so at boot, this might be an issue).

That's a pain for me. I'm still stuck with the vendors userspace as I
need their propiritory eInk driver code. It also seems like a hassle
for different distros to handle this (compared to just in the DT).

I understand the pain. But I am not talking about a 1 kernel cycle
release timeframe. More like 6-12 months to bring in all the pieces
together. Distributions have no issues with udev most of the time
(even those that stuck to the old pre-systemd fork), and it would not
be different than having a udev intrinsic that tags the pen with
ID_INPUT_TABLET so libinput and others can deal with it.

Cheers,
Benjamin

quoted

Alistair

quoted

I am not at the point where I can share the code as there is a lot of
rewriting and my last attempt is resulting in a page fault, but I'd be
happy to share it more once that hiccup is solved.

Cheers,
Benjamin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Wed, Oct 20, 2021 at 12:14 PM Dmitry Torokhov
[off-list ref] wrote:

On Wed, Oct 20, 2021 at 11:44:50AM +1000, Alistair Francis wrote:

quoted

On Wed, Oct 20, 2021 at 11:05 AM Dmitry Torokhov
[off-list ref] wrote:

quoted

On Wed, Oct 20, 2021 at 09:33:13AM +1000, Alistair Francis wrote:

quoted

On Tue, Oct 19, 2021 at 11:51 AM Dmitry Torokhov
[off-list ref] wrote:

quoted

We already have touchscreen-inverted-x/y defined in
Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml,
why are they not sufficient?

The touchscreen-* properties aren't applied to HID devices though, at
least not that I can tell.

No, they are not currently, but that does not mean we need to establish
a new set of properties (property names) for HID case.

I can update the names to use the existing touchscreen ones.

Do you have a hint of where this should be implemented though?

Right now (without "HID: wacom: Add support for the AG14 Wacom
device") the wacom touchscreen is just registered as a generic HID
device. I don't see any good place in hid-core, hid-input or
hid-generic to invert the input values for this.

I think the transformation should happen in
hid-multitouch.c::mt_process_slot() using helpers from
include/linux/input/touchscreen.h

Thanks for the help!

I have managed to get the device to be a hid-multitouch (instead of
hid-generic).

I also think I have figured out a way to get the properties to
hid-multitouch from the i2c-hid device. It requires a change to
touchscreen.c, but it's not a big change.

The main problem now is that hid-multitouch.c::mt_process_slot() isn't
actually called. The code just calls input_sync() from
hid-multitouch.c::mt_report(). It doesn't get to mt_process_slot() due
to rdata->is_mt_collection not being true. Setting
rdata->is_mt_collection to true causes userspace not to see the wacom
input any more.

Alistair

I think the more challenging question is to how pass/attach struct
touchscreen_properties * to the hid device (i expect the properties will
be attached to i2c-hid device, but maybe we could create a sub-node of
it and attach properties there.

Thanks.

--
Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Wed, Oct 20, 2021 at 1:28 PM Alistair Francis [off-list ref] wrote:

On Wed, Oct 20, 2021 at 12:14 PM Dmitry Torokhov
[off-list ref] wrote:

quoted

On Wed, Oct 20, 2021 at 11:44:50AM +1000, Alistair Francis wrote:

quoted

On Wed, Oct 20, 2021 at 11:05 AM Dmitry Torokhov
[off-list ref] wrote:

quoted

On Wed, Oct 20, 2021 at 09:33:13AM +1000, Alistair Francis wrote:

quoted

On Tue, Oct 19, 2021 at 11:51 AM Dmitry Torokhov
[off-list ref] wrote:

quoted

We already have touchscreen-inverted-x/y defined in
Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml,
why are they not sufficient?

The touchscreen-* properties aren't applied to HID devices though, at
least not that I can tell.

No, they are not currently, but that does not mean we need to establish
a new set of properties (property names) for HID case.

I can update the names to use the existing touchscreen ones.

Do you have a hint of where this should be implemented though?

Right now (without "HID: wacom: Add support for the AG14 Wacom
device") the wacom touchscreen is just registered as a generic HID
device. I don't see any good place in hid-core, hid-input or
hid-generic to invert the input values for this.

I think the transformation should happen in
hid-multitouch.c::mt_process_slot() using helpers from
include/linux/input/touchscreen.h

Thanks for the help!

I have managed to get the device to be a hid-multitouch (instead of
hid-generic).

I also think I have figured out a way to get the properties to
hid-multitouch from the i2c-hid device. It requires a change to
touchscreen.c, but it's not a big change.

The main problem now is that hid-multitouch.c::mt_process_slot() isn't
actually called. The code just calls input_sync() from
hid-multitouch.c::mt_report(). It doesn't get to mt_process_slot() due
to rdata->is_mt_collection not being true. Setting
rdata->is_mt_collection to true causes userspace not to see the wacom
input any more.

hid-multitouch now only handles the mutltitouch part. Everything else
is handled in hid-input.c
So if the device is just presenting a stylus to the user space, you
better not use hid-multitouch at all, but hid-generic.

Cheers,
Benjamin

Alistair

quoted

I think the more challenging question is to how pass/attach struct
touchscreen_properties * to the hid device (i expect the properties will
be attached to i2c-hid device, but maybe we could create a sub-node of
it and attach properties there.

Thanks.

--
Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel