| Message ID | 20211009114313.17967-1-alistair@alistair23.me (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v11,1/4] HID: wacom_sys: Add support for flipping the data values | expand |
On Sat, 09 Oct 2021 21:43:10 +1000, Alistair Francis wrote: > 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. > > Signed-off-by: Alistair Francis <alistair@alistair23.me> > --- > .../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(+) > Reviewed-by: Rob Herring <robh@kernel.org>
On Tue, Oct 19, 2021 at 3:42 AM Ping Cheng <pinglinux@gmail.com> wrote: > > Hi Alistair, > > On Sat, Oct 9, 2021, 4:44 AM Alistair Francis <alistair@alistair23.me> wrote: >> >> 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. > > Nacked-by: Ping Cheng <Ping.Cheng@wacom.com> Any ideas how to support the hardware then? I can't use the wacom_i2c driver as the panel supports I2C HID. But I can't use the I2C HID driver as I need the values flipped to support the installed orientation. Alistair > > Sorry about that, > Ping > >> Signed-off-by: Alistair Francis <alistair@alistair23.me> >> --- >> .../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 >> + >> + 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 >> + >> 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]); >> + >> + /* keep touch state for pen events */ >> + if (!wacom_wac->shared->touch_down) >> + wacom_wac->tool[0] = (data[1] & 0x0c) ? >> + BTN_TOOL_RUBBER : BTN_TOOL_PEN; >> + >> + wacom_wac->shared->touch_down = data[1] & 0x20; >> + >> + // Flip the values based on properties from the device tree >> + >> + // Default to a negative value for distance as HID compliant Wacom >> + // devices generally specify the hovering distance as negative. >> + distance = wacom_wac->flip_distance ? distance : -distance; >> + x = wacom_wac->flip_pos_x ? (features->x_max - x) : x; >> + y = wacom_wac->flip_pos_y ? (features->y_max - y) : y; >> + tilt_x = wacom_wac->flip_tilt_x ? -tilt_x : tilt_x; >> + tilt_y = wacom_wac->flip_tilt_y ? -tilt_y : tilt_y; >> + >> + input_report_key(input, BTN_TOUCH, tsw || ers); >> + input_report_key(input, wacom_wac->tool[0], wacom_wac->shared->touch_down); >> + input_report_key(input, BTN_STYLUS, f1); >> + input_report_key(input, BTN_STYLUS2, f2); >> + 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); >> + >> + return 1; >> +} >> + >> void wacom_wac_irq(struct wacom_wac *wacom_wac, size_t len) >> { >> bool sync; >> @@ -3379,6 +3436,10 @@ void wacom_wac_irq(struct wacom_wac *wacom_wac, size_t len) >> sync = wacom_remote_irq(wacom_wac, len); >> break; >> >> + case HID_GENERIC: >> + sync = wacom_of_irq(wacom_wac, len); >> + break; >> + >> default: >> sync = false; >> break; >> diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h >> index 8b2d4e5b2303..4dd5a56bf347 100644 >> --- a/drivers/hid/wacom_wac.h >> +++ b/drivers/hid/wacom_wac.h >> @@ -157,6 +157,14 @@ >> #define WACOM_HID_WT_Y (WACOM_HID_UP_WACOMTOUCH | 0x131) >> #define WACOM_HID_WT_REPORT_VALID (WACOM_HID_UP_WACOMTOUCH | 0x1d0) >> >> +// 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) >> + >> #define WACOM_BATTERY_USAGE(f) (((f)->hid == HID_DG_BATTERYSTRENGTH) || \ >> ((f)->hid == WACOM_HID_WD_BATTERY_CHARGING) || \ >> ((f)->hid == WACOM_HID_WD_BATTERY_LEVEL)) >> @@ -357,6 +365,11 @@ struct wacom_wac { >> bool has_mode_change; >> bool is_direct_mode; >> bool is_invalid_bt_frame; >> + bool flip_tilt_x; >> + bool flip_tilt_y; >> + bool flip_pos_x; >> + bool flip_pos_y; >> + bool flip_distance; >> }; >> >> #endif >> -- >> 2.31.1 >>
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 <alistair@alistair23.me> > wrote: > > > 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 <Ping.Cheng@wacom.com> > > Sorry about that, > Ping > > Signed-off-by: Alistair Francis <alistair@alistair23.me> > > --- > > .../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? > > + > > + 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? > > + > > + flip-distance: > > + type: boolean > > + description: Flip the distance values read from device I am still confused of the notion of flipped distance. > > + > > 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.
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 <pinglinux@gmail.com> Cc: Alistair Francis <alistair@alistair23.me>; shawnguo@kernel.org; s.hauer@pengutronix.de; linux-imx@nxp.com; Jiri Kosina <jikos@kernel.org>; Benjamin Tissoires <benjamin.tissoires@redhat.com>; linux-input <linux-input@vger.kernel.org>; devicetree@vger.kernel.org; LKML <linux-kernel@vger.kernel.org>; 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 <alistair@alistair23.me> > wrote: > > > 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 <Ping.Cheng@wacom.com> > > Sorry about that, > Ping > > Signed-off-by: Alistair Francis <alistair@alistair23.me> > > --- > > .../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? > > + > > + 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? > > + > > + flip-distance: > > + type: boolean > > + description: Flip the distance values read from device I am still confused of the notion of flipped distance. > > + > > 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
On Tue, Oct 19, 2021 at 11:51 AM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > 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 <alistair@alistair23.me> > > wrote: > > > > > 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 <Ping.Cheng@wacom.com> > > > > Sorry about that, > > Ping > > > > Signed-off-by: Alistair Francis <alistair@alistair23.me> > > > --- > > > .../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. > > > > + > > > + 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 > > > > + > > > + flip-distance: > > > + type: boolean > > > + description: Flip the distance values read from device > > I am still confused of the notion of flipped distance. > > > > + > > > 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
On Wed, Oct 20, 2021 at 09:33:13AM +1000, Alistair Francis wrote: > On Tue, Oct 19, 2021 at 11:51 AM Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > > > 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.
On Wed, Oct 20, 2021 at 11:05 AM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > On Wed, Oct 20, 2021 at 09:33:13AM +1000, Alistair Francis wrote: > > On Tue, Oct 19, 2021 at 11:51 AM Dmitry Torokhov > > <dmitry.torokhov@gmail.com> wrote: > > > > > > 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
On Wed, Oct 20, 2021 at 11:44:50AM +1000, Alistair Francis wrote: > On Wed, Oct 20, 2021 at 11:05 AM Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > > > On Wed, Oct 20, 2021 at 09:33:13AM +1000, Alistair Francis wrote: > > > On Tue, Oct 19, 2021 at 11:51 AM Dmitry Torokhov > > > <dmitry.torokhov@gmail.com> wrote: > > > > > > > > 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.
On Wed, Oct 20, 2021 at 4:14 AM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > On Wed, Oct 20, 2021 at 11:44:50AM +1000, Alistair Francis wrote: > > On Wed, Oct 20, 2021 at 11:05 AM Dmitry Torokhov > > <dmitry.torokhov@gmail.com> wrote: > > > > > > On Wed, Oct 20, 2021 at 09:33:13AM +1000, Alistair Francis wrote: > > > > On Tue, Oct 19, 2021 at 11:51 AM Dmitry Torokhov > > > > <dmitry.torokhov@gmail.com> wrote: > > > > > > > > > > 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
On Wed, Oct 20, 2021 at 12:14 PM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > On Wed, Oct 20, 2021 at 11:44:50AM +1000, Alistair Francis wrote: > > On Wed, Oct 20, 2021 at 11:05 AM Dmitry Torokhov > > <dmitry.torokhov@gmail.com> wrote: > > > > > > On Wed, Oct 20, 2021 at 09:33:13AM +1000, Alistair Francis wrote: > > > > On Tue, Oct 19, 2021 at 11:51 AM Dmitry Torokhov > > > > <dmitry.torokhov@gmail.com> wrote: > > > > > > > > > > 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
On Wed, Oct 20, 2021 at 5:40 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > On Wed, Oct 20, 2021 at 4:14 AM Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > > > On Wed, Oct 20, 2021 at 11:44:50AM +1000, Alistair Francis wrote: > > > On Wed, Oct 20, 2021 at 11:05 AM Dmitry Torokhov > > > <dmitry.torokhov@gmail.com> wrote: > > > > > > > > On Wed, Oct 20, 2021 at 09:33:13AM +1000, Alistair Francis wrote: > > > > > On Tue, Oct 19, 2021 at 11:51 AM Dmitry Torokhov > > > > > <dmitry.torokhov@gmail.com> wrote: > > > > > > > > > > > > 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 >
On Wed, Oct 20, 2021 at 1:28 PM Alistair Francis <alistair23@gmail.com> wrote: > > On Wed, Oct 20, 2021 at 12:14 PM Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > > > On Wed, Oct 20, 2021 at 11:44:50AM +1000, Alistair Francis wrote: > > > On Wed, Oct 20, 2021 at 11:05 AM Dmitry Torokhov > > > <dmitry.torokhov@gmail.com> wrote: > > > > > > > > On Wed, Oct 20, 2021 at 09:33:13AM +1000, Alistair Francis wrote: > > > > > On Tue, Oct 19, 2021 at 11:51 AM Dmitry Torokhov > > > > > <dmitry.torokhov@gmail.com> wrote: > > > > > > > > > > > > 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 > > > > > 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 >
On Wed, Oct 20, 2021 at 1:34 PM Alistair Francis <alistair23@gmail.com> wrote: > > On Wed, Oct 20, 2021 at 5:40 PM Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: > > > > On Wed, Oct 20, 2021 at 4:14 AM Dmitry Torokhov > > <dmitry.torokhov@gmail.com> wrote: > > > > > > On Wed, Oct 20, 2021 at 11:44:50AM +1000, Alistair Francis wrote: > > > > On Wed, Oct 20, 2021 at 11:05 AM Dmitry Torokhov > > > > <dmitry.torokhov@gmail.com> wrote: > > > > > > > > > > On Wed, Oct 20, 2021 at 09:33:13AM +1000, Alistair Francis wrote: > > > > > > On Tue, Oct 19, 2021 at 11:51 AM Dmitry Torokhov > > > > > > <dmitry.torokhov@gmail.com> wrote: > > > > > > > > > > > > > > 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. 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. > > > 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. > > > 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 > > > > > 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 > > >
On Wed, Oct 20, 2021 at 10:04 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > On Wed, Oct 20, 2021 at 1:34 PM Alistair Francis <alistair23@gmail.com> wrote: > > > > On Wed, Oct 20, 2021 at 5:40 PM Benjamin Tissoires > > <benjamin.tissoires@redhat.com> wrote: > > > > > > On Wed, Oct 20, 2021 at 4:14 AM Dmitry Torokhov > > > <dmitry.torokhov@gmail.com> wrote: > > > > > > > > On Wed, Oct 20, 2021 at 11:44:50AM +1000, Alistair Francis wrote: > > > > > On Wed, Oct 20, 2021 at 11:05 AM Dmitry Torokhov > > > > > <dmitry.torokhov@gmail.com> wrote: > > > > > > > > > > > > On Wed, Oct 20, 2021 at 09:33:13AM +1000, Alistair Francis wrote: > > > > > > > On Tue, Oct 19, 2021 at 11:51 AM Dmitry Torokhov > > > > > > > <dmitry.torokhov@gmail.com> wrote: > > > > > > > > > > > > > > > > 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. > > 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. > > > > > > 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 > > > > > > 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 > > > > > > > > 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 > > > > > >
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 + + 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 + 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]); + + /* keep touch state for pen events */ + if (!wacom_wac->shared->touch_down) + wacom_wac->tool[0] = (data[1] & 0x0c) ? + BTN_TOOL_RUBBER : BTN_TOOL_PEN; + + wacom_wac->shared->touch_down = data[1] & 0x20; + + // Flip the values based on properties from the device tree + + // Default to a negative value for distance as HID compliant Wacom + // devices generally specify the hovering distance as negative. + distance = wacom_wac->flip_distance ? distance : -distance; + x = wacom_wac->flip_pos_x ? (features->x_max - x) : x; + y = wacom_wac->flip_pos_y ? (features->y_max - y) : y; + tilt_x = wacom_wac->flip_tilt_x ? -tilt_x : tilt_x; + tilt_y = wacom_wac->flip_tilt_y ? -tilt_y : tilt_y; + + input_report_key(input, BTN_TOUCH, tsw || ers); + input_report_key(input, wacom_wac->tool[0], wacom_wac->shared->touch_down); + input_report_key(input, BTN_STYLUS, f1); + input_report_key(input, BTN_STYLUS2, f2); + 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); + + return 1; +} + void wacom_wac_irq(struct wacom_wac *wacom_wac, size_t len) { bool sync; @@ -3379,6 +3436,10 @@ void wacom_wac_irq(struct wacom_wac *wacom_wac, size_t len) sync = wacom_remote_irq(wacom_wac, len); break; + case HID_GENERIC: + sync = wacom_of_irq(wacom_wac, len); + break; + default: sync = false; break; diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h index 8b2d4e5b2303..4dd5a56bf347 100644 --- a/drivers/hid/wacom_wac.h +++ b/drivers/hid/wacom_wac.h @@ -157,6 +157,14 @@ #define WACOM_HID_WT_Y (WACOM_HID_UP_WACOMTOUCH | 0x131) #define WACOM_HID_WT_REPORT_VALID (WACOM_HID_UP_WACOMTOUCH | 0x1d0) +// 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) + #define WACOM_BATTERY_USAGE(f) (((f)->hid == HID_DG_BATTERYSTRENGTH) || \ ((f)->hid == WACOM_HID_WD_BATTERY_CHARGING) || \ ((f)->hid == WACOM_HID_WD_BATTERY_LEVEL)) @@ -357,6 +365,11 @@ struct wacom_wac { bool has_mode_change; bool is_direct_mode; bool is_invalid_bt_frame; + bool flip_tilt_x; + bool flip_tilt_y; + bool flip_pos_x; + bool flip_pos_y; + bool flip_distance; }; #endif
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. Signed-off-by: Alistair Francis <alistair@alistair23.me> --- .../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(+)