LinuxLists.cc - [PATCH] HID: multitouch: Add quirks for flipped axes

2022-12-06 17:56:50

Subject: [PATCH] HID: multitouch: Add quirks for flipped axes

2022-12-06 20:32:15

Subject: Re: [PATCH] HID: multitouch: Add quirks for flipped axes

Hi Allen,

On Tue, Dec 6, 2022 at 9:39 AM Allen Ballway <[email protected]> wrote:
>
> Certain touchscreen devices, such as the ELAN9034, are oriented
> incorrectly and report touches on opposite points on the X and Y axes.
> For example, a 100x200 screen touched at (10,20) would report (90, 180)
> and vice versa.
>
> This is fixed by adding device quirks to transform the touch points
> into the correct spaces, from X -> MAX(X) - X, and Y -> MAX(Y) - Y.
>
> Signed-off-by: Allen Ballway <[email protected]>
> ---
>
> drivers/hid/hid-multitouch.c | 46 ++++++++++++++++++++++++++++++++----
> 1 file changed, 42 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 91a4d3fc30e08..5e14cc4b00f53 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -71,6 +71,8 @@ MODULE_LICENSE("GPL");
> #define MT_QUIRK_SEPARATE_APP_REPORT BIT(19)
> #define MT_QUIRK_FORCE_MULTI_INPUT BIT(20)
> #define MT_QUIRK_DISABLE_WAKEUP BIT(21)
> +#define MT_QUIRK_FLIP_X BIT(22)
> +#define MT_QUIRK_FLIP_Y BIT(23)
>
> #define MT_INPUTMODE_TOUCHSCREEN 0x02
> #define MT_INPUTMODE_TOUCHPAD 0x03
> @@ -212,6 +214,7 @@ static void mt_post_parse(struct mt_device *td, struct mt_application *app);
> #define MT_CLS_GOOGLE 0x0111
> #define MT_CLS_RAZER_BLADE_STEALTH 0x0112
> #define MT_CLS_SMART_TECH 0x0113
> +#define MT_CLS_ELAN_FLIPPED 0x0114
>
> #define MT_DEFAULT_MAXCONTACT 10
> #define MT_MAX_MAXCONTACT 250
> @@ -396,6 +399,17 @@ static const struct mt_class mt_classes[] = {
> MT_QUIRK_CONTACT_CNT_ACCURATE |
> MT_QUIRK_SEPARATE_APP_REPORT,
> },input_abs_get_max(input, ABS_MT_POSITION_X) - *slot->x
> + { .name = MT_CLS_ELAN_FLIPPED,
> + .quirks = MT_QUIRK_ALWAYS_VALID |
> + MT_QUIRK_IGNORE_DUPLICATES |
> + MT_QUIRK_HOVERING |
> + MT_QUIRK_CONTACT_CNT_ACCURATE |
> + MT_QUIRK_STICKY_FINGERS |
> + MT_QUIRK_WIN8_PTP_BUTTONS |
> + MT_QUIRK_FLIP_X |
> + MT_QUIRK_FLIP_Y,
> + .export_all_inputs = true },
> +
> { }
> };
>
> @@ -1115,10 +1129,30 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
> minor = minor >> 1;
> }
>
> - input_event(input, EV_ABS, ABS_MT_POSITION_X, *slot->x);
> - input_event(input, EV_ABS, ABS_MT_POSITION_Y, *slot->y);
> - input_event(input, EV_ABS, ABS_MT_TOOL_X, *slot->cx);
> - input_event(input, EV_ABS, ABS_MT_TOOL_Y, *slot->cy);
> + if (quirks & MT_QUIRK_FLIP_X) {
> + /* Inputs with a flipped X axis need to report MAX - X */
> + int x = input_abs_get_max(input, ABS_MT_POSITION_X) - *slot->x;

Maybe

x = quirks & MT_QUIRK_FLIP_X ?
input_abs_get_max(input, ABS_MT_POSITION_X) - *slot->x :
*slot->x;

and then report it all together?

> + int cx = input_abs_get_max(input, ABS_MT_TOOL_X) - *slot->cx;

Would like to double-check that this conversion is actually needed.

> +
> + input_event(input, EV_ABS, ABS_MT_POSITION_X, x);
> + input_event(input, EV_ABS, ABS_MT_TOOL_X, cx);
> + } else {
> + input_event(input, EV_ABS, ABS_MT_POSITION_X, *slot->x);
> + input_event(input, EV_ABS, ABS_MT_TOOL_X, *slot->cx);
> + }
> +
> + if (quirks & MT_QUIRK_FLIP_Y) {
> + /* Inputs with a flipped Y axis need to report MAX - Y */
> + int y = input_abs_get_max(input, ABS_MT_POSITION_Y) - *slot->y;
> + int cy = input_abs_get_max(input, ABS_MT_TOOL_Y) - *slot->cy;
> +
> + input_event(input, EV_ABS, ABS_MT_POSITION_Y, y);
> + input_event(input, EV_ABS, ABS_MT_TOOL_Y, cy);
> + } else {
> + input_event(input, EV_ABS, ABS_MT_POSITION_Y, *slot->y);
> + input_event(input, EV_ABS, ABS_MT_TOOL_Y, *slot->cy);
> + }
> +
> input_event(input, EV_ABS, ABS_MT_DISTANCE, !*slot->tip_state);
> input_event(input, EV_ABS, ABS_MT_ORIENTATION, orientation);

I think we might need to do something about this too as orientation will change.

> input_event(input, EV_ABS, ABS_MT_PRESSURE, *slot->p);
> @@ -1963,6 +1997,10 @@ static const struct hid_device_id mt_devices[] = {
> USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_C002) },
>
> /* Elan devices */
> + { .driver_data = MT_CLS_ELAN_FLIPPED,
> + HID_DEVICE(BUS_I2C, HID_GROUP_MULTITOUCH_WIN_8,
> + USB_VENDOR_ID_ELAN, 0x2dcd) },
> +

I believe this needs to be plumbed through i2c-hid and involve DMI check as I am
concerned that product 0x2dcd might have been used in other devices where it was
mounted in a different way.

> { .driver_data = MT_CLS_WIN_8_FORCE_MULTI_INPUT,
> HID_DEVICE(BUS_I2C, HID_GROUP_MULTITOUCH_WIN_8,
> USB_VENDOR_ID_ELAN, 0x313a) },
> --
> 2.39.0.rc0.267.gcb52ba06e7-goog
>

Thanks,
Dmitry

2022-12-08 17:22:11

by Allen Ballway

[permalink] [raw]

Subject: [PATCH v2] HID: multitouch: Add quirks for flipped axes

2022-12-12 19:00:49

by Dmitry Torokhov

[permalink] [raw]

Subject: Re: [PATCH v2] HID: multitouch: Add quirks for flipped axes

On Thu, Dec 08, 2022 at 04:58:36PM +0000, Allen Ballway wrote:
> Certain touchscreen devices, such as the ELAN9034, are oriented
> incorrectly and report touches on opposite points on the X and Y axes.
> For example, a 100x200 screen touched at (10,20) would report (90, 180)
> and vice versa.
>
> This is fixed by adding device quirks to transform the touch points
> into the correct spaces, from X -> MAX(X) - X, and Y -> MAX(Y) - Y.
>
> Signed-off-by: Allen Ballway <[email protected]>
> ---
> V1 -> V2: Address review comments, change to use DMI match. Confirmed
> MT_TOOL_X/Y require transformation and update orientation based on
> flipped axes.

I think you might need to push the DMI match into i2c-hid driver, and
bubble the quirks up here, otherwise if you connect a 2nd USB
touchscreen to such a system will also result in the quirk being applied
to it as far as I understand. In i2c-hid you can make a fine-grained
match on vendor/product and system to limit the quirk to hopefully the
right peripheral.

Thanks.

--
Dmitry

2022-12-13 01:12:48

by Allen Ballway

[permalink] [raw]

Subject: [PATCH v3] HID: multitouch: Add quirks for flipped axes

Certain touchscreen devices, such as the ELAN9034, are oriented
incorrectly and report touches on opposite points on the X and Y axes.
For example, a 100x200 screen touched at (10,20) would report (90, 180)
and vice versa.

This changed fixes the issue by adding device quirks to transform
the touch points into the correct spaces, from X -> MAX(X) - X,
and Y -> MAX(Y) - Y. These quirks are added in hid-quirks checking
both DMI information and device vendor and product IDs. The quirk
is handled in hid-multitouch to do the actual transformation.

Signed-off-by: Allen Ballway <[email protected]>
---
V2 -> V3: Use existing HID_QUIRK_*_INVERT and match the quirk in
hid-quirk, passing down to hid-multitouch through the hid device.

V1 -> V2: Address review comments, change to use DMI match. Confirmed
MT_TOOL_X/Y require transformation and update orientation based on
flipped axes.

drivers/hid/hid-multitouch.c | 43 ++++++++++++++++++++++++++++++++----
drivers/hid/hid-quirks.c | 33 +++++++++++++++++++++++++++
2 files changed, 72 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 91a4d3fc30e08..1f4c2aa511359 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -71,6 +71,8 @@ MODULE_LICENSE("GPL");
#define MT_QUIRK_SEPARATE_APP_REPORT BIT(19)
#define MT_QUIRK_FORCE_MULTI_INPUT BIT(20)
#define MT_QUIRK_DISABLE_WAKEUP BIT(21)
+#define MT_QUIRK_X_INVERT BIT(22)
+#define MT_QUIRK_Y_INVERT BIT(23)

#define MT_INPUTMODE_TOUCHSCREEN 0x02
#define MT_INPUTMODE_TOUCHPAD 0x03
@@ -1086,6 +1088,10 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
int orientation = wide;
int max_azimuth;
int azimuth;
+ int x;
+ int y;
+ int cx;
+ int cy;

if (slot->a != DEFAULT_ZERO) {
/*
@@ -1104,6 +1110,16 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
if (azimuth > max_azimuth * 2)
azimuth -= max_azimuth * 4;
orientation = -azimuth;
+
+ /* Orientation is inverted if the X or Y axes are
+ * flipped, but normalized if both are inverted.
+ */
+ if (quirks & (MT_QUIRK_X_INVERT | MT_QUIRK_Y_INVERT) &&
+ !((quirks & MT_QUIRK_X_INVERT)
+ && (quirks & MT_QUIRK_Y_INVERT))) {
+ orientation = -orientation;
+ }
+
}

if (quirks & MT_QUIRK_TOUCH_SIZE_SCALING) {
@@ -1115,10 +1131,23 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
minor = minor >> 1;
}

- input_event(input, EV_ABS, ABS_MT_POSITION_X, *slot->x);
- input_event(input, EV_ABS, ABS_MT_POSITION_Y, *slot->y);
- input_event(input, EV_ABS, ABS_MT_TOOL_X, *slot->cx);
- input_event(input, EV_ABS, ABS_MT_TOOL_Y, *slot->cy);
+ x = quirks & MT_QUIRK_X_INVERT ?
+ input_abs_get_max(input, ABS_MT_POSITION_X) - *slot->x :
+ *slot->x;
+ y = quirks & MT_QUIRK_Y_INVERT ?
+ input_abs_get_max(input, ABS_MT_POSITION_Y) - *slot->y :
+ *slot->y;
+ cx = quirks & MT_QUIRK_X_INVERT ?
+ input_abs_get_max(input, ABS_MT_POSITION_X) - *slot->cx :
+ *slot->cx;
+ cy = quirks & MT_QUIRK_Y_INVERT ?
+ input_abs_get_max(input, ABS_MT_POSITION_Y) - *slot->cy :
+ *slot->cy;
+
+ input_event(input, EV_ABS, ABS_MT_POSITION_X, x);
+ input_event(input, EV_ABS, ABS_MT_POSITION_Y, y);
+ input_event(input, EV_ABS, ABS_MT_TOOL_X, cx);
+ input_event(input, EV_ABS, ABS_MT_TOOL_Y, cy);
input_event(input, EV_ABS, ABS_MT_DISTANCE, !*slot->tip_state);
input_event(input, EV_ABS, ABS_MT_ORIENTATION, orientation);
input_event(input, EV_ABS, ABS_MT_PRESSURE, *slot->p);
@@ -1735,6 +1764,12 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
if (id->vendor == HID_ANY_ID && id->product == HID_ANY_ID)
td->serial_maybe = true;

+ if (hdev->quirks & HID_QUIRK_X_INVERT)
+ td->mtclass.quirks |= MT_QUIRK_X_INVERT;
+
+ if (hdev->quirks & HID_QUIRK_Y_INVERT)
+ td->mtclass.quirks |= MT_QUIRK_Y_INVERT;
+
/* This allows the driver to correctly support devices
* that emit events over several HID messages.
*/
diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
index 0e9702c7f7d6c..47c6cd62f019a 100644
--- a/drivers/hid/hid-quirks.c
+++ b/drivers/hid/hid-quirks.c
@@ -16,6 +16,7 @@
#include <linux/export.h>
#include <linux/slab.h>
#include <linux/mutex.h>
+#include <linux/dmi.h>
#include <linux/input/elan-i2c-ids.h>

#include "hid-ids.h"
@@ -957,6 +958,29 @@ static const struct hid_device_id hid_mouse_ignore_list[] = {
{ }
};

+static const struct hid_device_id elan_flipped_quirks[] = {
+ { HID_DEVICE(BUS_I2C, HID_GROUP_MULTITOUCH_WIN_8, USB_VENDOR_ID_ELAN, 0x2dcd),
+ HID_QUIRK_X_INVERT | HID_QUIRK_Y_INVERT },
+ { }
+};
+
+/*
+ * This list contains devices which have specific issues based on the system
+ * they're on and not just the device itself. The driver_data will have a
+ * specific hid device to match against.
+ */
+static const struct dmi_system_id dmi_override_table[] = {
+ {
+ .ident = "DynaBook K50/FR",
+ .matches = {
+ DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Dynabook Inc."),
+ DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "dynabook K50/FR"),
+ },
+ .driver_data = (void *)elan_flipped_quirks,
+ },
+ { } /* Terminate list */
+};
+
bool hid_ignore(struct hid_device *hdev)
{
int i;
@@ -1238,6 +1262,7 @@ static unsigned long hid_gets_squirk(const struct hid_device *hdev)
{
const struct hid_device_id *bl_entry;
unsigned long quirks = 0;
+ const struct dmi_system_id *system_id;

if (hid_match_id(hdev, hid_ignore_list))
quirks |= HID_QUIRK_IGNORE;
@@ -1249,6 +1274,14 @@ static unsigned long hid_gets_squirk(const struct hid_device *hdev)
if (bl_entry != NULL)
quirks |= bl_entry->driver_data;

+ system_id = dmi_first_match(dmi_override_table);
+ if (system_id != NULL) {
+ bl_entry = hid_match_id(hdev, system_id->driver_data);
+ if (bl_entry != NULL)
+ quirks |= bl_entry->driver_data;
+ }
+
+
if (quirks)
dbg_hid("Found squirk 0x%lx for HID device 0x%04x:0x%04x\n",
quirks, hdev->vendor, hdev->product);
--
2.39.0.rc1.256.g54fd8350bd-goog

2022-12-13 10:01:55

by Benjamin Tissoires

[permalink] [raw]

Subject: Re: [PATCH v3] HID: multitouch: Add quirks for flipped axes

On Tue, Dec 13, 2022 at 2:01 AM Allen Ballway <[email protected]> wrote:
>
> Certain touchscreen devices, such as the ELAN9034, are oriented
> incorrectly and report touches on opposite points on the X and Y axes.
> For example, a 100x200 screen touched at (10,20) would report (90, 180)
> and vice versa.
>
> This changed fixes the issue by adding device quirks to transform
> the touch points into the correct spaces, from X -> MAX(X) - X,
> and Y -> MAX(Y) - Y. These quirks are added in hid-quirks checking
> both DMI information and device vendor and product IDs. The quirk
> is handled in hid-multitouch to do the actual transformation.
>
> Signed-off-by: Allen Ballway <[email protected]>
> ---
> V2 -> V3: Use existing HID_QUIRK_*_INVERT and match the quirk in
> hid-quirk, passing down to hid-multitouch through the hid device.
>
> V1 -> V2: Address review comments, change to use DMI match. Confirmed
> MT_TOOL_X/Y require transformation and update orientation based on
> flipped axes.
>
>
> drivers/hid/hid-multitouch.c | 43 ++++++++++++++++++++++++++++++++----
> drivers/hid/hid-quirks.c | 33 +++++++++++++++++++++++++++
> 2 files changed, 72 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 91a4d3fc30e08..1f4c2aa511359 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -71,6 +71,8 @@ MODULE_LICENSE("GPL");
> #define MT_QUIRK_SEPARATE_APP_REPORT BIT(19)
> #define MT_QUIRK_FORCE_MULTI_INPUT BIT(20)
> #define MT_QUIRK_DISABLE_WAKEUP BIT(21)
> +#define MT_QUIRK_X_INVERT BIT(22)
> +#define MT_QUIRK_Y_INVERT BIT(23)

Why duplicate the already available quirks in struct hid_device?

>
> #define MT_INPUTMODE_TOUCHSCREEN 0x02
> #define MT_INPUTMODE_TOUCHPAD 0x03
> @@ -1086,6 +1088,10 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
> int orientation = wide;
> int max_azimuth;
> int azimuth;
> + int x;
> + int y;
> + int cx;
> + int cy;
>
> if (slot->a != DEFAULT_ZERO) {
> /*
> @@ -1104,6 +1110,16 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
> if (azimuth > max_azimuth * 2)
> azimuth -= max_azimuth * 4;
> orientation = -azimuth;
> +
> + /* Orientation is inverted if the X or Y axes are
> + * flipped, but normalized if both are inverted.
> + */
> + if (quirks & (MT_QUIRK_X_INVERT | MT_QUIRK_Y_INVERT) &&
> + !((quirks & MT_QUIRK_X_INVERT)
> + && (quirks & MT_QUIRK_Y_INVERT))) {

This is done in every finger slot in every report. We should probably
cache that information somewhere.

> + orientation = -orientation;
> + }
> +
> }
>
> if (quirks & MT_QUIRK_TOUCH_SIZE_SCALING) {
> @@ -1115,10 +1131,23 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
> minor = minor >> 1;
> }
>
> - input_event(input, EV_ABS, ABS_MT_POSITION_X, *slot->x);
> - input_event(input, EV_ABS, ABS_MT_POSITION_Y, *slot->y);
> - input_event(input, EV_ABS, ABS_MT_TOOL_X, *slot->cx);
> - input_event(input, EV_ABS, ABS_MT_TOOL_Y, *slot->cy);
> + x = quirks & MT_QUIRK_X_INVERT ?
> + input_abs_get_max(input, ABS_MT_POSITION_X) - *slot->x :
> + *slot->x;
> + y = quirks & MT_QUIRK_Y_INVERT ?
> + input_abs_get_max(input, ABS_MT_POSITION_Y) - *slot->y :
> + *slot->y;
> + cx = quirks & MT_QUIRK_X_INVERT ?
> + input_abs_get_max(input, ABS_MT_POSITION_X) - *slot->cx :
> + *slot->cx;
> + cy = quirks & MT_QUIRK_Y_INVERT ?
> + input_abs_get_max(input, ABS_MT_POSITION_Y) - *slot->cy :
> + *slot->cy;

I can't help but think that there must be a better way of doing that.
If I didn't postpone HID-BPF to 6.3 I would have asked you to do this
as a BPF program.

Still, the whole point of the slot pointers was to remove the data
copy everywhere, and you are adding it back. Not to mention that the
same tests and accesses to variables are called multiple times.

> +
> + input_event(input, EV_ABS, ABS_MT_POSITION_X, x);
> + input_event(input, EV_ABS, ABS_MT_POSITION_Y, y);
> + input_event(input, EV_ABS, ABS_MT_TOOL_X, cx);
> + input_event(input, EV_ABS, ABS_MT_TOOL_Y, cy);
> input_event(input, EV_ABS, ABS_MT_DISTANCE, !*slot->tip_state);
> input_event(input, EV_ABS, ABS_MT_ORIENTATION, orientation);
> input_event(input, EV_ABS, ABS_MT_PRESSURE, *slot->p);
> @@ -1735,6 +1764,12 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
> if (id->vendor == HID_ANY_ID && id->product == HID_ANY_ID)
> td->serial_maybe = true;
>
> + if (hdev->quirks & HID_QUIRK_X_INVERT)
> + td->mtclass.quirks |= MT_QUIRK_X_INVERT;
> +
> + if (hdev->quirks & HID_QUIRK_Y_INVERT)
> + td->mtclass.quirks |= MT_QUIRK_Y_INVERT;

As mentioned above, I don't see the point in doing that duplication of quirks.

> +
> /* This allows the driver to correctly support devices
> * that emit events over several HID messages.
> */
> diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c

AFAICT, Dmitry told you twice to use i2c-hid, not hid-quirks.c.

We already have i2c-hid-dmi-quirks.c that you could extend. The
rationale is that this way, the dmi check will be done only for
internal peripherals, leaving aside the USB ones.

Cheers,
Benjamin

> index 0e9702c7f7d6c..47c6cd62f019a 100644
> --- a/drivers/hid/hid-quirks.c
> +++ b/drivers/hid/hid-quirks.c
> @@ -16,6 +16,7 @@
> #include <linux/export.h>
> #include <linux/slab.h>
> #include <linux/mutex.h>
> +#include <linux/dmi.h>
> #include <linux/input/elan-i2c-ids.h>
>
> #include "hid-ids.h"
> @@ -957,6 +958,29 @@ static const struct hid_device_id hid_mouse_ignore_list[] = {
> { }
> };
>
> +static const struct hid_device_id elan_flipped_quirks[] = {
> + { HID_DEVICE(BUS_I2C, HID_GROUP_MULTITOUCH_WIN_8, USB_VENDOR_ID_ELAN, 0x2dcd),
> + HID_QUIRK_X_INVERT | HID_QUIRK_Y_INVERT },
> + { }
> +};
> +
> +/*
> + * This list contains devices which have specific issues based on the system
> + * they're on and not just the device itself. The driver_data will have a
> + * specific hid device to match against.
> + */
> +static const struct dmi_system_id dmi_override_table[] = {
> + {
> + .ident = "DynaBook K50/FR",
> + .matches = {
> + DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Dynabook Inc."),
> + DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "dynabook K50/FR"),
> + },
> + .driver_data = (void *)elan_flipped_quirks,
> + },
> + { } /* Terminate list */
> +};
> +
> bool hid_ignore(struct hid_device *hdev)
> {
> int i;
> @@ -1238,6 +1262,7 @@ static unsigned long hid_gets_squirk(const struct hid_device *hdev)
> {
> const struct hid_device_id *bl_entry;
> unsigned long quirks = 0;
> + const struct dmi_system_id *system_id;
>
> if (hid_match_id(hdev, hid_ignore_list))
> quirks |= HID_QUIRK_IGNORE;
> @@ -1249,6 +1274,14 @@ static unsigned long hid_gets_squirk(const struct hid_device *hdev)
> if (bl_entry != NULL)
> quirks |= bl_entry->driver_data;
>
> + system_id = dmi_first_match(dmi_override_table);
> + if (system_id != NULL) {
> + bl_entry = hid_match_id(hdev, system_id->driver_data);
> + if (bl_entry != NULL)
> + quirks |= bl_entry->driver_data;
> + }
> +
> +
> if (quirks)
> dbg_hid("Found squirk 0x%lx for HID device 0x%04x:0x%04x\n",
> quirks, hdev->vendor, hdev->product);
> --
> 2.39.0.rc1.256.g54fd8350bd-goog
>

2022-12-13 16:56:00

by Allen Ballway

[permalink] [raw]

Subject: Re: [PATCH v3] HID: multitouch: Add quirks for flipped axes

On Tue, Dec 13, 2022 at 1:19 AM Benjamin Tissoires
<[email protected]> wrote:
>
> On Tue, Dec 13, 2022 at 2:01 AM Allen Ballway <[email protected]> wrote:
> >
> > Certain touchscreen devices, such as the ELAN9034, are oriented
> > incorrectly and report touches on opposite points on the X and Y axes.
> > For example, a 100x200 screen touched at (10,20) would report (90, 180)
> > and vice versa.
> >
> > This changed fixes the issue by adding device quirks to transform
> > the touch points into the correct spaces, from X -> MAX(X) - X,
> > and Y -> MAX(Y) - Y. These quirks are added in hid-quirks checking
> > both DMI information and device vendor and product IDs. The quirk
> > is handled in hid-multitouch to do the actual transformation.
> >
> > Signed-off-by: Allen Ballway <[email protected]>
> > ---
> > V2 -> V3: Use existing HID_QUIRK_*_INVERT and match the quirk in
> > hid-quirk, passing down to hid-multitouch through the hid device.
> >
> > V1 -> V2: Address review comments, change to use DMI match. Confirmed
> > MT_TOOL_X/Y require transformation and update orientation based on
> > flipped axes.
> >
> >
> > drivers/hid/hid-multitouch.c | 43 ++++++++++++++++++++++++++++++++----
> > drivers/hid/hid-quirks.c | 33 +++++++++++++++++++++++++++
> > 2 files changed, 72 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> > index 91a4d3fc30e08..1f4c2aa511359 100644
> > --- a/drivers/hid/hid-multitouch.c
> > +++ b/drivers/hid/hid-multitouch.c
> > @@ -71,6 +71,8 @@ MODULE_LICENSE("GPL");
> > #define MT_QUIRK_SEPARATE_APP_REPORT BIT(19)
> > #define MT_QUIRK_FORCE_MULTI_INPUT BIT(20)
> > #define MT_QUIRK_DISABLE_WAKEUP BIT(21)
> > +#define MT_QUIRK_X_INVERT BIT(22)
> > +#define MT_QUIRK_Y_INVERT BIT(23)
>
> Why duplicate the already available quirks in struct hid_device?

This no longer seems necessary, and will be removed.

>
> >
> > #define MT_INPUTMODE_TOUCHSCREEN 0x02
> > #define MT_INPUTMODE_TOUCHPAD 0x03
> > @@ -1086,6 +1088,10 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
> > int orientation = wide;
> > int max_azimuth;
> > int azimuth;
> > + int x;
> > + int y;
> > + int cx;
> > + int cy;
> >
> > if (slot->a != DEFAULT_ZERO) {
> > /*
> > @@ -1104,6 +1110,16 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
> > if (azimuth > max_azimuth * 2)
> > azimuth -= max_azimuth * 4;
> > orientation = -azimuth;
> > +
> > + /* Orientation is inverted if the X or Y axes are
> > + * flipped, but normalized if both are inverted.
> > + */
> > + if (quirks & (MT_QUIRK_X_INVERT | MT_QUIRK_Y_INVERT) &&
> > + !((quirks & MT_QUIRK_X_INVERT)
> > + && (quirks & MT_QUIRK_Y_INVERT))) {
>
> This is done in every finger slot in every report. We should probably
> cache that information somewhere.

I can do this once in mt_probe and keep it saved as its own quirk.

>
> > + orientation = -orientation;
> > + }
> > +
> > }
> >
> > if (quirks & MT_QUIRK_TOUCH_SIZE_SCALING) {
> > @@ -1115,10 +1131,23 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
> > minor = minor >> 1;
> > }
> >
> > - input_event(input, EV_ABS, ABS_MT_POSITION_X, *slot->x);
> > - input_event(input, EV_ABS, ABS_MT_POSITION_Y, *slot->y);
> > - input_event(input, EV_ABS, ABS_MT_TOOL_X, *slot->cx);
> > - input_event(input, EV_ABS, ABS_MT_TOOL_Y, *slot->cy);
> > + x = quirks & MT_QUIRK_X_INVERT ?
> > + input_abs_get_max(input, ABS_MT_POSITION_X) - *slot->x :
> > + *slot->x;
> > + y = quirks & MT_QUIRK_Y_INVERT ?
> > + input_abs_get_max(input, ABS_MT_POSITION_Y) - *slot->y :
> > + *slot->y;
> > + cx = quirks & MT_QUIRK_X_INVERT ?
> > + input_abs_get_max(input, ABS_MT_POSITION_X) - *slot->cx :
> > + *slot->cx;
> > + cy = quirks & MT_QUIRK_Y_INVERT ?
> > + input_abs_get_max(input, ABS_MT_POSITION_Y) - *slot->cy :
> > + *slot->cy;
>
> I can't help but think that there must be a better way of doing that.
> If I didn't postpone HID-BPF to 6.3 I would have asked you to do this
> as a BPF program.
>
> Still, the whole point of the slot pointers was to remove the data
> copy everywhere, and you are adding it back. Not to mention that the
> same tests and accesses to variables are called multiple times.

I can move the transformation to mt_touch_input_mapping which has easier access
to the quirks coming from hid_device rather than using duplicated quirks for
mt_application/class.

>
> > +
> > + input_event(input, EV_ABS, ABS_MT_POSITION_X, x);
> > + input_event(input, EV_ABS, ABS_MT_POSITION_Y, y);
> > + input_event(input, EV_ABS, ABS_MT_TOOL_X, cx);
> > + input_event(input, EV_ABS, ABS_MT_TOOL_Y, cy);
> > input_event(input, EV_ABS, ABS_MT_DISTANCE, !*slot->tip_state);
> > input_event(input, EV_ABS, ABS_MT_ORIENTATION, orientation);
> > input_event(input, EV_ABS, ABS_MT_PRESSURE, *slot->p);
> > @@ -1735,6 +1764,12 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > if (id->vendor == HID_ANY_ID && id->product == HID_ANY_ID)
> > td->serial_maybe = true;
> >
> > + if (hdev->quirks & HID_QUIRK_X_INVERT)
> > + td->mtclass.quirks |= MT_QUIRK_X_INVERT;
> > +
> > + if (hdev->quirks & HID_QUIRK_Y_INVERT)
> > + td->mtclass.quirks |= MT_QUIRK_Y_INVERT;
>
> As mentioned above, I don't see the point in doing that duplication of quirks.

Will remove

>
> > +
> > /* This allows the driver to correctly support devices
> > * that emit events over several HID messages.
> > */
> > diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
>
> AFAICT, Dmitry told you twice to use i2c-hid, not hid-quirks.c.
>
> We already have i2c-hid-dmi-quirks.c that you could extend. The
> rationale is that this way, the dmi check will be done only for
> internal peripherals, leaving aside the USB ones.

I had added the quirks to hid_device to i2c-hid-dmi-quirks before
trying this but they were getting stomped by the quirks coming from
hid-quirks. I will move these back to i2c-hid-dmi-quirks and change
hid-device to add them to whatever quirks come from hid-quirks.

>
> Cheers,
> Benjamin
>
> > index 0e9702c7f7d6c..47c6cd62f019a 100644
> > --- a/drivers/hid/hid-quirks.c
> > +++ b/drivers/hid/hid-quirks.c
> > @@ -16,6 +16,7 @@
> > #include <linux/export.h>
> > #include <linux/slab.h>
> > #include <linux/mutex.h>
> > +#include <linux/dmi.h>
> > #include <linux/input/elan-i2c-ids.h>
> >
> > #include "hid-ids.h"
> > @@ -957,6 +958,29 @@ static const struct hid_device_id hid_mouse_ignore_list[] = {
> > { }
> > };
> >
> > +static const struct hid_device_id elan_flipped_quirks[] = {
> > + { HID_DEVICE(BUS_I2C, HID_GROUP_MULTITOUCH_WIN_8, USB_VENDOR_ID_ELAN, 0x2dcd),
> > + HID_QUIRK_X_INVERT | HID_QUIRK_Y_INVERT },
> > + { }
> > +};
> > +
> > +/*
> > + * This list contains devices which have specific issues based on the system
> > + * they're on and not just the device itself. The driver_data will have a
> > + * specific hid device to match against.
> > + */
> > +static const struct dmi_system_id dmi_override_table[] = {
> > + {
> > + .ident = "DynaBook K50/FR",
> > + .matches = {
> > + DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Dynabook Inc."),
> > + DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "dynabook K50/FR"),
> > + },
> > + .driver_data = (void *)elan_flipped_quirks,
> > + },
> > + { } /* Terminate list */
> > +};
> > +
> > bool hid_ignore(struct hid_device *hdev)
> > {
> > int i;
> > @@ -1238,6 +1262,7 @@ static unsigned long hid_gets_squirk(const struct hid_device *hdev)
> > {
> > const struct hid_device_id *bl_entry;
> > unsigned long quirks = 0;
> > + const struct dmi_system_id *system_id;
> >
> > if (hid_match_id(hdev, hid_ignore_list))
> > quirks |= HID_QUIRK_IGNORE;
> > @@ -1249,6 +1274,14 @@ static unsigned long hid_gets_squirk(const struct hid_device *hdev)
> > if (bl_entry != NULL)
> > quirks |= bl_entry->driver_data;
> >
> > + system_id = dmi_first_match(dmi_override_table);
> > + if (system_id != NULL) {
> > + bl_entry = hid_match_id(hdev, system_id->driver_data);
> > + if (bl_entry != NULL)
> > + quirks |= bl_entry->driver_data;
> > + }
> > +
> > +
> > if (quirks)
> > dbg_hid("Found squirk 0x%lx for HID device 0x%04x:0x%04x\n",
> > quirks, hdev->vendor, hdev->product);
> > --
> > 2.39.0.rc1.256.g54fd8350bd-goog
> >
>

Thanks for the comments,
Allen

2022-12-13 22:05:29

by Allen Ballway

[permalink] [raw]

Subject: Re: [PATCH v3] HID: multitouch: Add quirks for flipped axes

On Tue, Dec 13, 2022 at 8:45 AM Allen Ballway <[email protected]> wrote:
>
> On Tue, Dec 13, 2022 at 1:19 AM Benjamin Tissoires
> <[email protected]> wrote:
> >
> > On Tue, Dec 13, 2022 at 2:01 AM Allen Ballway <[email protected]> wrote:
> > >
> > > Certain touchscreen devices, such as the ELAN9034, are oriented
> > > incorrectly and report touches on opposite points on the X and Y axes.
> > > For example, a 100x200 screen touched at (10,20) would report (90, 180)
> > > and vice versa.
> > >
> > > This changed fixes the issue by adding device quirks to transform
> > > the touch points into the correct spaces, from X -> MAX(X) - X,
> > > and Y -> MAX(Y) - Y. These quirks are added in hid-quirks checking
> > > both DMI information and device vendor and product IDs. The quirk
> > > is handled in hid-multitouch to do the actual transformation.
> > >
> > > Signed-off-by: Allen Ballway <[email protected]>
> > > ---
> > > V2 -> V3: Use existing HID_QUIRK_*_INVERT and match the quirk in
> > > hid-quirk, passing down to hid-multitouch through the hid device.
> > >
> > > V1 -> V2: Address review comments, change to use DMI match. Confirmed
> > > MT_TOOL_X/Y require transformation and update orientation based on
> > > flipped axes.
> > >
> > >
> > > drivers/hid/hid-multitouch.c | 43 ++++++++++++++++++++++++++++++++----
> > > drivers/hid/hid-quirks.c | 33 +++++++++++++++++++++++++++
> > > 2 files changed, 72 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> > > index 91a4d3fc30e08..1f4c2aa511359 100644
> > > --- a/drivers/hid/hid-multitouch.c
> > > +++ b/drivers/hid/hid-multitouch.c
> > > @@ -71,6 +71,8 @@ MODULE_LICENSE("GPL");
> > > #define MT_QUIRK_SEPARATE_APP_REPORT BIT(19)
> > > #define MT_QUIRK_FORCE_MULTI_INPUT BIT(20)
> > > #define MT_QUIRK_DISABLE_WAKEUP BIT(21)
> > > +#define MT_QUIRK_X_INVERT BIT(22)
> > > +#define MT_QUIRK_Y_INVERT BIT(23)
> >
> > Why duplicate the already available quirks in struct hid_device?
>
> This no longer seems necessary, and will be removed.
>
> >
> > >
> > > #define MT_INPUTMODE_TOUCHSCREEN 0x02
> > > #define MT_INPUTMODE_TOUCHPAD 0x03
> > > @@ -1086,6 +1088,10 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
> > > int orientation = wide;
> > > int max_azimuth;
> > > int azimuth;
> > > + int x;
> > > + int y;
> > > + int cx;
> > > + int cy;
> > >
> > > if (slot->a != DEFAULT_ZERO) {
> > > /*
> > > @@ -1104,6 +1110,16 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
> > > if (azimuth > max_azimuth * 2)
> > > azimuth -= max_azimuth * 4;
> > > orientation = -azimuth;
> > > +
> > > + /* Orientation is inverted if the X or Y axes are
> > > + * flipped, but normalized if both are inverted.
> > > + */
> > > + if (quirks & (MT_QUIRK_X_INVERT | MT_QUIRK_Y_INVERT) &&
> > > + !((quirks & MT_QUIRK_X_INVERT)
> > > + && (quirks & MT_QUIRK_Y_INVERT))) {
> >
> > This is done in every finger slot in every report. We should probably
> > cache that information somewhere.
>
> I can do this once in mt_probe and keep it saved as its own quirk.
>
> >
> > > + orientation = -orientation;
> > > + }
> > > +
> > > }
> > >
> > > if (quirks & MT_QUIRK_TOUCH_SIZE_SCALING) {
> > > @@ -1115,10 +1131,23 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
> > > minor = minor >> 1;
> > > }
> > >
> > > - input_event(input, EV_ABS, ABS_MT_POSITION_X, *slot->x);
> > > - input_event(input, EV_ABS, ABS_MT_POSITION_Y, *slot->y);
> > > - input_event(input, EV_ABS, ABS_MT_TOOL_X, *slot->cx);
> > > - input_event(input, EV_ABS, ABS_MT_TOOL_Y, *slot->cy);
> > > + x = quirks & MT_QUIRK_X_INVERT ?
> > > + input_abs_get_max(input, ABS_MT_POSITION_X) - *slot->x :
> > > + *slot->x;
> > > + y = quirks & MT_QUIRK_Y_INVERT ?
> > > + input_abs_get_max(input, ABS_MT_POSITION_Y) - *slot->y :
> > > + *slot->y;
> > > + cx = quirks & MT_QUIRK_X_INVERT ?
> > > + input_abs_get_max(input, ABS_MT_POSITION_X) - *slot->cx :
> > > + *slot->cx;
> > > + cy = quirks & MT_QUIRK_Y_INVERT ?
> > > + input_abs_get_max(input, ABS_MT_POSITION_Y) - *slot->cy :
> > > + *slot->cy;
> >
> > I can't help but think that there must be a better way of doing that.
> > If I didn't postpone HID-BPF to 6.3 I would have asked you to do this
> > as a BPF program.
> >
> > Still, the whole point of the slot pointers was to remove the data
> > copy everywhere, and you are adding it back. Not to mention that the
> > same tests and accesses to variables are called multiple times.
>
> I can move the transformation to mt_touch_input_mapping which has easier access
> to the quirks coming from hid_device rather than using duplicated quirks for
> mt_application/class.

It looks like I won't be able to make the change in
mt_touch_input_mapping because
the slot values I need to transform are pointers into the hid_report
coming through the device.
The other use of HID_QUIRK_*_INVERT is in hid-input.c and is
transforming the value
before calling input_event. I see some changes to the field values happening in
mt_need_to_apply_feature and doing a hid_hw_raw_request, but those
seem to happen
once with a fixed value and seems more expensive than just doing the
transform every time,
if that would even work for this.

>
> >
> > > +
> > > + input_event(input, EV_ABS, ABS_MT_POSITION_X, x);
> > > + input_event(input, EV_ABS, ABS_MT_POSITION_Y, y);
> > > + input_event(input, EV_ABS, ABS_MT_TOOL_X, cx);
> > > + input_event(input, EV_ABS, ABS_MT_TOOL_Y, cy);
> > > input_event(input, EV_ABS, ABS_MT_DISTANCE, !*slot->tip_state);
> > > input_event(input, EV_ABS, ABS_MT_ORIENTATION, orientation);
> > > input_event(input, EV_ABS, ABS_MT_PRESSURE, *slot->p);
> > > @@ -1735,6 +1764,12 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > > if (id->vendor == HID_ANY_ID && id->product == HID_ANY_ID)
> > > td->serial_maybe = true;
> > >
> > > + if (hdev->quirks & HID_QUIRK_X_INVERT)
> > > + td->mtclass.quirks |= MT_QUIRK_X_INVERT;
> > > +
> > > + if (hdev->quirks & HID_QUIRK_Y_INVERT)
> > > + td->mtclass.quirks |= MT_QUIRK_Y_INVERT;
> >
> > As mentioned above, I don't see the point in doing that duplication of quirks.
>
> Will remove
>
> >
> > > +
> > > /* This allows the driver to correctly support devices
> > > * that emit events over several HID messages.
> > > */
> > > diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
> >
> > AFAICT, Dmitry told you twice to use i2c-hid, not hid-quirks.c.
> >
> > We already have i2c-hid-dmi-quirks.c that you could extend. The
> > rationale is that this way, the dmi check will be done only for
> > internal peripherals, leaving aside the USB ones.
>
> I had added the quirks to hid_device to i2c-hid-dmi-quirks before
> trying this but they were getting stomped by the quirks coming from
> hid-quirks. I will move these back to i2c-hid-dmi-quirks and change
> hid-device to add them to whatever quirks come from hid-quirks.

I can get the quirks to come from i2c-hid-dmi-quirks.c, but the hid_device
quirks reset on probe so I will need to get them again in hid_lookup_quirk.
I can at least put it behind an if (bus == I2C) to keep the DMI checks to
a minimum but I don't see a way to get the quirk from i2c-hid-dmi-quirks
without going through hid-quirks.

>
> >
> > Cheers,
> > Benjamin
> >
> > > index 0e9702c7f7d6c..47c6cd62f019a 100644
> > > --- a/drivers/hid/hid-quirks.c
> > > +++ b/drivers/hid/hid-quirks.c
> > > @@ -16,6 +16,7 @@
> > > #include <linux/export.h>
> > > #include <linux/slab.h>
> > > #include <linux/mutex.h>
> > > +#include <linux/dmi.h>
> > > #include <linux/input/elan-i2c-ids.h>
> > >
> > > #include "hid-ids.h"
> > > @@ -957,6 +958,29 @@ static const struct hid_device_id hid_mouse_ignore_list[] = {
> > > { }
> > > };
> > >
> > > +static const struct hid_device_id elan_flipped_quirks[] = {
> > > + { HID_DEVICE(BUS_I2C, HID_GROUP_MULTITOUCH_WIN_8, USB_VENDOR_ID_ELAN, 0x2dcd),
> > > + HID_QUIRK_X_INVERT | HID_QUIRK_Y_INVERT },
> > > + { }
> > > +};
> > > +
> > > +/*
> > > + * This list contains devices which have specific issues based on the system
> > > + * they're on and not just the device itself. The driver_data will have a
> > > + * specific hid device to match against.
> > > + */
> > > +static const struct dmi_system_id dmi_override_table[] = {
> > > + {
> > > + .ident = "DynaBook K50/FR",
> > > + .matches = {
> > > + DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Dynabook Inc."),
> > > + DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "dynabook K50/FR"),
> > > + },
> > > + .driver_data = (void *)elan_flipped_quirks,
> > > + },
> > > + { } /* Terminate list */
> > > +};
> > > +
> > > bool hid_ignore(struct hid_device *hdev)
> > > {
> > > int i;
> > > @@ -1238,6 +1262,7 @@ static unsigned long hid_gets_squirk(const struct hid_device *hdev)
> > > {
> > > const struct hid_device_id *bl_entry;
> > > unsigned long quirks = 0;
> > > + const struct dmi_system_id *system_id;
> > >
> > > if (hid_match_id(hdev, hid_ignore_list))
> > > quirks |= HID_QUIRK_IGNORE;are
> > > @@ -1249,6 +1274,14 @@ static unsigned long hid_gets_squirk(const struct hid_device *hdev)
> > > if (bl_entry != NULL)
> > > quirks |= bl_entry->driver_data;
> > >
> > > + system_id = dmi_first_match(dmi_override_table);
> > > + if (system_id != NULL) {
> > > + bl_entry = hid_match_id(hdev, system_id->driver_data);
> > > + if (bl_entry != NULL)
> > > + quirks |= bl_entry->driver_data;
> > > + }
> > > +
> > > +
> > > if (quirks)
> > > dbg_hid("Found squirk 0x%lx for HID device 0x%04x:0x%04x\n",
> > > quirks, hdev->vendor, hdev->product);
> > > --
> > > 2.39.0.rc1.256.g54fd8350bd-goog
> > >
> >
>
> Thanks for the comments,
> Allen

Thanks,
Allen

2022-12-14 18:30:40

by Allen Ballway

[permalink] [raw]

Subject: [PATCH v4] HID: multitouch: Add quirks for flipped axes

Certain touchscreen devices, such as the ELAN9034, are oriented
incorrectly and report touches on opposite points on the X and Y axes.
For example, a 100x200 screen touched at (10,20) would report (90, 180)
and vice versa.

This is fixed by adding device quirks to transform the touch points
into the correct spaces, from X -> MAX(X) - X, and Y -> MAX(Y) - Y.

Signed-off-by: Allen Ballway <[email protected]>
---
V3 -> V4: Move quirk logic to i2c_hid_get_dmi_quirks and remove
duplicate quirks from hid-multitouch.

V2 -> V3: Use existing HID_QUIRK_*_INVERT and match the quirk in
hid-quirk, passing down to hid-multitouch through the hid device.

V1 -> V2: Address review comments, change to use DMI match. Confirmed
MT_TOOL_X/Y require transformation and update orientation based on
flipped axes.

drivers/hid/hid-multitouch.c | 39 ++++++++++++++++++---
drivers/hid/hid-quirks.c | 5 +++
drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c | 43 ++++++++++++++++++++++++
drivers/hid/i2c-hid/i2c-hid.h | 3 ++
4 files changed, 86 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 91a4d3fc30e08..622fe6928104c 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -71,6 +71,7 @@ MODULE_LICENSE("GPL");
#define MT_QUIRK_SEPARATE_APP_REPORT BIT(19)
#define MT_QUIRK_FORCE_MULTI_INPUT BIT(20)
#define MT_QUIRK_DISABLE_WAKEUP BIT(21)
+#define MT_QUIRK_ORIENTATION_INVERT BIT(22)

#define MT_INPUTMODE_TOUCHSCREEN 0x02
#define MT_INPUTMODE_TOUCHPAD 0x03
@@ -1009,6 +1010,7 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
struct mt_usages *slot)
{
struct input_mt *mt = input->mt;
+ struct hid_device *hdev = td->hdev;
__s32 quirks = app->quirks;
bool valid = true;
bool confidence_state = true;
@@ -1086,6 +1088,10 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
int orientation = wide;
int max_azimuth;
int azimuth;
+ int x;
+ int y;
+ int cx;
+ int cy;

if (slot->a != DEFAULT_ZERO) {
/*
@@ -1104,6 +1110,9 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
if (azimuth > max_azimuth * 2)
azimuth -= max_azimuth * 4;
orientation = -azimuth;
+ if (quirks & MT_QUIRK_ORIENTATION_INVERT)
+ orientation = -orientation;
+
}

if (quirks & MT_QUIRK_TOUCH_SIZE_SCALING) {
@@ -1115,10 +1124,23 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
minor = minor >> 1;
}

- input_event(input, EV_ABS, ABS_MT_POSITION_X, *slot->x);
- input_event(input, EV_ABS, ABS_MT_POSITION_Y, *slot->y);
- input_event(input, EV_ABS, ABS_MT_TOOL_X, *slot->cx);
- input_event(input, EV_ABS, ABS_MT_TOOL_Y, *slot->cy);
+ x = hdev->quirks & HID_QUIRK_X_INVERT ?
+ input_abs_get_max(input, ABS_MT_POSITION_X) - *slot->x :
+ *slot->x;
+ y = hdev->quirks & HID_QUIRK_Y_INVERT ?
+ input_abs_get_max(input, ABS_MT_POSITION_Y) - *slot->y :
+ *slot->y;
+ cx = hdev->quirks & HID_QUIRK_X_INVERT ?
+ input_abs_get_max(input, ABS_MT_POSITION_X) - *slot->cx :
+ *slot->cx;
+ cy = hdev->quirks & HID_QUIRK_Y_INVERT ?
+ input_abs_get_max(input, ABS_MT_POSITION_Y) - *slot->cy :
+ *slot->cy;
+
+ input_event(input, EV_ABS, ABS_MT_POSITION_X, x);
+ input_event(input, EV_ABS, ABS_MT_POSITION_Y, y);
+ input_event(input, EV_ABS, ABS_MT_TOOL_X, cx);
+ input_event(input, EV_ABS, ABS_MT_TOOL_Y, cy);
input_event(input, EV_ABS, ABS_MT_DISTANCE, !*slot->tip_state);
input_event(input, EV_ABS, ABS_MT_ORIENTATION, orientation);
input_event(input, EV_ABS, ABS_MT_PRESSURE, *slot->p);
@@ -1735,6 +1757,15 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
if (id->vendor == HID_ANY_ID && id->product == HID_ANY_ID)
td->serial_maybe = true;

+
+ /* Orientation is inverted if the X or Y axes are
+ * flipped, but normalized if both are inverted.
+ */
+ if (hdev->quirks & (HID_QUIRK_X_INVERT | HID_QUIRK_Y_INVERT) &&
+ !((hdev->quirks & HID_QUIRK_X_INVERT)
+ && (hdev->quirks & HID_QUIRK_Y_INVERT)))
+ td->mtclass.quirks = MT_QUIRK_ORIENTATION_INVERT;
+
/* This allows the driver to correctly support devices
* that emit events over several HID messages.
*/
diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
index 0e9702c7f7d6c..c4d58d9a05fe9 100644
--- a/drivers/hid/hid-quirks.c
+++ b/drivers/hid/hid-quirks.c
@@ -19,6 +19,7 @@
#include <linux/input/elan-i2c-ids.h>

#include "hid-ids.h"
+#include "i2c-hid/i2c-hid.h"

/*
* Alphabetically sorted by vendor then product.
@@ -1298,6 +1299,10 @@ unsigned long hid_lookup_quirk(const struct hid_device *hdev)
quirks = hid_gets_squirk(hdev);
mutex_unlock(&dquirks_lock);

+ /* Get quirks specific to I2C devices */
+ if (hdev->bus == BUS_I2C)
+ quirks |= i2c_hid_get_dmi_quirks(hdev->vendor, hdev->product);
+
return quirks;
}
EXPORT_SYMBOL_GPL(hid_lookup_quirk);
diff --git a/drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c b/drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c
index 8e0f67455c098..554a7dc285365 100644
--- a/drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c
+++ b/drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c
@@ -10,8 +10,10 @@
#include <linux/types.h>
#include <linux/dmi.h>
#include <linux/mod_devicetable.h>
+#include <linux/hid.h>

#include "i2c-hid.h"
+#include "../hid-ids.h"

struct i2c_hid_desc_override {
@@ -416,6 +418,28 @@ static const struct dmi_system_id i2c_hid_dmi_desc_override_table[] = {
{ } /* Terminate list */
};

+static const struct hid_device_id i2c_hid_elan_flipped_quirks = {
+ HID_DEVICE(BUS_I2C, HID_GROUP_MULTITOUCH_WIN_8, USB_VENDOR_ID_ELAN, 0x2dcd),
+ HID_QUIRK_X_INVERT | HID_QUIRK_Y_INVERT
+};
+
+/*
+ * This list contains devices which have specific issues based on the system
+ * they're on and not just the device itself. The driver_data will have a
+ * specific hid device to match against.
+ */
+static const struct dmi_system_id i2c_hid_dmi_quirk_table[] = {
+ {
+ .ident = "DynaBook K50/FR",
+ .matches = {
+ DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Dynabook Inc."),
+ DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "dynabook K50/FR"),
+ },
+ .driver_data = (void *)&i2c_hid_elan_flipped_quirks,
+ },
+ { } /* Terminate list */
+};
+

struct i2c_hid_desc *i2c_hid_get_dmi_i2c_hid_desc_override(uint8_t *i2c_name)
{
@@ -450,3 +474,22 @@ char *i2c_hid_get_dmi_hid_report_desc_override(uint8_t *i2c_name,
*size = override->hid_report_desc_size;
return override->hid_report_desc;
}
+
+u32 i2c_hid_get_dmi_quirks(const u16 vendor, const u16 product)
+{
+ u32 quirks = 0;
+ const struct dmi_system_id *system_id =
+ dmi_first_match(i2c_hid_dmi_quirk_table);
+
+ if (system_id) {
+ const struct hid_device_id *device_id =
+ (struct hid_device_id *)(system_id->driver_data);
+
+ if (device_id && device_id->vendor == vendor &&
+ device_id->product == product)
+ quirks = device_id->driver_data;
+ }
+
+ return quirks;
+}
+EXPORT_SYMBOL_GPL(i2c_hid_get_dmi_quirks);
diff --git a/drivers/hid/i2c-hid/i2c-hid.h b/drivers/hid/i2c-hid/i2c-hid.h
index 96c75510ad3f1..2c7b66d5caa0f 100644
--- a/drivers/hid/i2c-hid/i2c-hid.h
+++ b/drivers/hid/i2c-hid/i2c-hid.h
@@ -9,6 +9,7 @@
struct i2c_hid_desc *i2c_hid_get_dmi_i2c_hid_desc_override(uint8_t *i2c_name);
char *i2c_hid_get_dmi_hid_report_desc_override(uint8_t *i2c_name,
unsigned int *size);
+u32 i2c_hid_get_dmi_quirks(const u16 vendor, const u16 product);
#else
static inline struct i2c_hid_desc
*i2c_hid_get_dmi_i2c_hid_desc_override(uint8_t *i2c_name)
@@ -16,6 +17,8 @@ static inline struct i2c_hid_desc
static inline char *i2c_hid_get_dmi_hid_report_desc_override(uint8_t *i2c_name,
unsigned int *size)
{ return NULL; }
+static inline u32 i2c_hid_get_dmi_quirks(const u16 vendor, const u16 product)
+{ return 0; }
#endif

/**
--
2.39.0.rc1.256.g54fd8350bd-goog

2022-12-14 22:38:17

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v4] HID: multitouch: Add quirks for flipped axes

Hi Allen,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on hid/for-next]
[also build test ERROR on linus/master v6.1 next-20221214]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Allen-Ballway/HID-multitouch-Add-quirks-for-flipped-axes/20221215-021448
base: https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git for-next
patch link: https://lore.kernel.org/r/20221214181334.323360-1-ballway%40chromium.org
patch subject: [PATCH v4] HID: multitouch: Add quirks for flipped axes
config: x86_64-randconfig-a006
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/6f10db9d7ffacd402e75fb3089f3347d3a24236e
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Allen-Ballway/HID-multitouch-Add-quirks-for-flipped-axes/20221215-021448
git checkout 6f10db9d7ffacd402e75fb3089f3347d3a24236e
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

ld: drivers/hid/hid-quirks.o: in function `hid_lookup_quirk':
>> drivers/hid/hid-quirks.c:1304: undefined reference to `i2c_hid_get_dmi_quirks'
pahole: .tmp_vmlinux.btf: No such file or directory
.btf.vmlinux.bin.o: file not recognized: file format not recognized

vim +1304 drivers/hid/hid-quirks.c

1258
1259 /**
1260 * hid_lookup_quirk - return any quirks associated with a HID device
1261 * @hdev: the HID device to look for
1262 *
1263 * Description:
1264 * Given a HID device, return any quirks associated with that device.
1265 *
1266 * Return: an unsigned long quirks value.
1267 */
1268 unsigned long hid_lookup_quirk(const struct hid_device *hdev)
1269 {
1270 unsigned long quirks = 0;
1271 const struct hid_device_id *quirk_entry = NULL;
1272
1273 /* NCR devices must not be queried for reports */
1274 if (hdev->bus == BUS_USB &&
1275 hdev->vendor == USB_VENDOR_ID_NCR &&
1276 hdev->product >= USB_DEVICE_ID_NCR_FIRST &&
1277 hdev->product <= USB_DEVICE_ID_NCR_LAST)
1278 return HID_QUIRK_NO_INIT_REPORTS;
1279
1280 /* These devices must be ignored if version (bcdDevice) is too old */
1281 if (hdev->bus == BUS_USB && hdev->vendor == USB_VENDOR_ID_JABRA) {
1282 switch (hdev->product) {
1283 case USB_DEVICE_ID_JABRA_SPEAK_410:
1284 if (hdev->version < 0x0111)
1285 return HID_QUIRK_IGNORE;
1286 break;
1287 case USB_DEVICE_ID_JABRA_SPEAK_510:
1288 if (hdev->version < 0x0214)
1289 return HID_QUIRK_IGNORE;
1290 break;
1291 }
1292 }
1293
1294 mutex_lock(&dquirks_lock);
1295 quirk_entry = hid_exists_dquirk(hdev);
1296 if (quirk_entry)
1297 quirks = quirk_entry->driver_data;
1298 else
1299 quirks = hid_gets_squirk(hdev);
1300 mutex_unlock(&dquirks_lock);
1301
1302 /* Get quirks specific to I2C devices */
1303 if (hdev->bus == BUS_I2C)
> 1304 quirks |= i2c_hid_get_dmi_quirks(hdev->vendor, hdev->product);

--
0-DAY CI Kernel Test Service
https://01.org/lkp

Attachments:

(No filename) (3.55 kB)
config (113.40 kB)
Download all attachments

2022-12-14 23:11:40

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v4] HID: multitouch: Add quirks for flipped axes

Attachments:

(No filename) (2.12 kB)
config (154.00 kB)
Download all attachments

2022-12-14 23:51:21

by Allen Ballway

[permalink] [raw]

Subject: [PATCH v5] HID: multitouch: Add quirks for flipped axes

Certain touchscreen devices, such as the ELAN9034, are oriented
incorrectly and report touches on opposite points on the X and Y axes.
For example, a 100x200 screen touched at (10,20) would report (90, 180)
and vice versa.

This is fixed by adding device quirks to transform the touch points
into the correct spaces, from X -> MAX(X) - X, and Y -> MAX(Y) - Y.

Signed-off-by: Allen Ballway <[email protected]>
---
V4 -> V5: Add IS_ENABLED check for CONFIG_DMI to prevent linker error.

V3 -> V4: Move quirk logic to i2c_hid_get_dmi_quirks and remove
duplicate quirks from hid-multitouch.

V2 -> V3: Use existing HID_QUIRK_*_INVERT and match the quirk in
hid-quirk, passing down to hid-multitouch through the hid device.

V1 -> V2: Address review comments, change to use DMI match. Confirmed
MT_TOOL_X/Y require transformation and update orientation based on
flipped axes.

drivers/hid/hid-multitouch.c | 39 ++++++++++++++++++---
drivers/hid/hid-quirks.c | 5 +++
drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c | 43 ++++++++++++++++++++++++
drivers/hid/i2c-hid/i2c-hid.h | 3 ++
4 files changed, 86 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 91a4d3fc30e08..622fe6928104c 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -71,6 +71,7 @@ MODULE_LICENSE("GPL");
#define MT_QUIRK_SEPARATE_APP_REPORT BIT(19)
#define MT_QUIRK_FORCE_MULTI_INPUT BIT(20)
#define MT_QUIRK_DISABLE_WAKEUP BIT(21)
+#define MT_QUIRK_ORIENTATION_INVERT BIT(22)

#define MT_INPUTMODE_TOUCHSCREEN 0x02
#define MT_INPUTMODE_TOUCHPAD 0x03
@@ -1009,6 +1010,7 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
struct mt_usages *slot)
{
struct input_mt *mt = input->mt;
+ struct hid_device *hdev = td->hdev;
__s32 quirks = app->quirks;
bool valid = true;
bool confidence_state = true;
@@ -1086,6 +1088,10 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
int orientation = wide;
int max_azimuth;
int azimuth;
+ int x;
+ int y;
+ int cx;
+ int cy;

if (slot->a != DEFAULT_ZERO) {
/*
@@ -1104,6 +1110,9 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
if (azimuth > max_azimuth * 2)
azimuth -= max_azimuth * 4;
orientation = -azimuth;
+ if (quirks & MT_QUIRK_ORIENTATION_INVERT)
+ orientation = -orientation;
+
}

if (quirks & MT_QUIRK_TOUCH_SIZE_SCALING) {
@@ -1115,10 +1124,23 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
minor = minor >> 1;
}

- input_event(input, EV_ABS, ABS_MT_POSITION_X, *slot->x);
- input_event(input, EV_ABS, ABS_MT_POSITION_Y, *slot->y);
- input_event(input, EV_ABS, ABS_MT_TOOL_X, *slot->cx);
- input_event(input, EV_ABS, ABS_MT_TOOL_Y, *slot->cy);
+ x = hdev->quirks & HID_QUIRK_X_INVERT ?
+ input_abs_get_max(input, ABS_MT_POSITION_X) - *slot->x :
+ *slot->x;
+ y = hdev->quirks & HID_QUIRK_Y_INVERT ?
+ input_abs_get_max(input, ABS_MT_POSITION_Y) - *slot->y :
+ *slot->y;
+ cx = hdev->quirks & HID_QUIRK_X_INVERT ?
+ input_abs_get_max(input, ABS_MT_POSITION_X) - *slot->cx :
+ *slot->cx;
+ cy = hdev->quirks & HID_QUIRK_Y_INVERT ?
+ input_abs_get_max(input, ABS_MT_POSITION_Y) - *slot->cy :
+ *slot->cy;
+
+ input_event(input, EV_ABS, ABS_MT_POSITION_X, x);
+ input_event(input, EV_ABS, ABS_MT_POSITION_Y, y);
+ input_event(input, EV_ABS, ABS_MT_TOOL_X, cx);
+ input_event(input, EV_ABS, ABS_MT_TOOL_Y, cy);
input_event(input, EV_ABS, ABS_MT_DISTANCE, !*slot->tip_state);
input_event(input, EV_ABS, ABS_MT_ORIENTATION, orientation);
input_event(input, EV_ABS, ABS_MT_PRESSURE, *slot->p);
@@ -1735,6 +1757,15 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
if (id->vendor == HID_ANY_ID && id->product == HID_ANY_ID)
td->serial_maybe = true;

+
+ /* Orientation is inverted if the X or Y axes are
+ * flipped, but normalized if both are inverted.
+ */
+ if (hdev->quirks & (HID_QUIRK_X_INVERT | HID_QUIRK_Y_INVERT) &&
+ !((hdev->quirks & HID_QUIRK_X_INVERT)
+ && (hdev->quirks & HID_QUIRK_Y_INVERT)))
+ td->mtclass.quirks = MT_QUIRK_ORIENTATION_INVERT;
+
/* This allows the driver to correctly support devices
* that emit events over several HID messages.
*/
diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
index 0e9702c7f7d6c..60ab33377c2a0 100644
--- a/drivers/hid/hid-quirks.c
+++ b/drivers/hid/hid-quirks.c
@@ -19,6 +19,7 @@
#include <linux/input/elan-i2c-ids.h>

#include "hid-ids.h"
+#include "i2c-hid/i2c-hid.h"

/*
* Alphabetically sorted by vendor then product.
@@ -1298,6 +1299,10 @@ unsigned long hid_lookup_quirk(const struct hid_device *hdev)
quirks = hid_gets_squirk(hdev);
mutex_unlock(&dquirks_lock);

+ /* Get quirks specific to I2C devices */
+ if (IS_ENABLED(CONFIG_DMI) && hdev->bus == BUS_I2C)
+ quirks |= i2c_hid_get_dmi_quirks(hdev->vendor, hdev->product);
+
return quirks;
}
EXPORT_SYMBOL_GPL(hid_lookup_quirk);
diff --git a/drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c b/drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c
index 8e0f67455c098..554a7dc285365 100644
--- a/drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c
+++ b/drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c
@@ -10,8 +10,10 @@
#include <linux/types.h>
#include <linux/dmi.h>
#include <linux/mod_devicetable.h>
+#include <linux/hid.h>

#include "i2c-hid.h"
+#include "../hid-ids.h"

struct i2c_hid_desc_override {
@@ -416,6 +418,28 @@ static const struct dmi_system_id i2c_hid_dmi_desc_override_table[] = {
{ } /* Terminate list */
};

+static const struct hid_device_id i2c_hid_elan_flipped_quirks = {
+ HID_DEVICE(BUS_I2C, HID_GROUP_MULTITOUCH_WIN_8, USB_VENDOR_ID_ELAN, 0x2dcd),
+ HID_QUIRK_X_INVERT | HID_QUIRK_Y_INVERT
+};
+
+/*
+ * This list contains devices which have specific issues based on the system
+ * they're on and not just the device itself. The driver_data will have a
+ * specific hid device to match against.
+ */
+static const struct dmi_system_id i2c_hid_dmi_quirk_table[] = {
+ {
+ .ident = "DynaBook K50/FR",
+ .matches = {
+ DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Dynabook Inc."),
+ DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "dynabook K50/FR"),
+ },
+ .driver_data = (void *)&i2c_hid_elan_flipped_quirks,
+ },
+ { } /* Terminate list */
+};
+

struct i2c_hid_desc *i2c_hid_get_dmi_i2c_hid_desc_override(uint8_t *i2c_name)
{
@@ -450,3 +474,22 @@ char *i2c_hid_get_dmi_hid_report_desc_override(uint8_t *i2c_name,
*size = override->hid_report_desc_size;
return override->hid_report_desc;
}
+
+u32 i2c_hid_get_dmi_quirks(const u16 vendor, const u16 product)
+{
+ u32 quirks = 0;
+ const struct dmi_system_id *system_id =
+ dmi_first_match(i2c_hid_dmi_quirk_table);
+
+ if (system_id) {
+ const struct hid_device_id *device_id =
+ (struct hid_device_id *)(system_id->driver_data);
+
+ if (device_id && device_id->vendor == vendor &&
+ device_id->product == product)
+ quirks = device_id->driver_data;
+ }
+
+ return quirks;
+}
+EXPORT_SYMBOL_GPL(i2c_hid_get_dmi_quirks);
diff --git a/drivers/hid/i2c-hid/i2c-hid.h b/drivers/hid/i2c-hid/i2c-hid.h
index 96c75510ad3f1..2c7b66d5caa0f 100644
--- a/drivers/hid/i2c-hid/i2c-hid.h
+++ b/drivers/hid/i2c-hid/i2c-hid.h
@@ -9,6 +9,7 @@
struct i2c_hid_desc *i2c_hid_get_dmi_i2c_hid_desc_override(uint8_t *i2c_name);
char *i2c_hid_get_dmi_hid_report_desc_override(uint8_t *i2c_name,
unsigned int *size);
+u32 i2c_hid_get_dmi_quirks(const u16 vendor, const u16 product);
#else
static inline struct i2c_hid_desc
*i2c_hid_get_dmi_i2c_hid_desc_override(uint8_t *i2c_name)
@@ -16,6 +17,8 @@ static inline struct i2c_hid_desc
static inline char *i2c_hid_get_dmi_hid_report_desc_override(uint8_t *i2c_name,
unsigned int *size)
{ return NULL; }
+static inline u32 i2c_hid_get_dmi_quirks(const u16 vendor, const u16 product)
+{ return 0; }
#endif

/**
--
2.39.0.rc1.256.g54fd8350bd-goog

2022-12-15 00:43:09

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v4] HID: multitouch: Add quirks for flipped axes

Attachments:

(No filename) (1.90 kB)
config (143.54 kB)
Download all attachments

2022-12-15 08:21:48

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v5] HID: multitouch: Add quirks for flipped axes

Hi Allen,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on hid/for-next]
[also build test ERROR on linus/master v6.1 next-20221215]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Allen-Ballway/HID-multitouch-Add-quirks-for-flipped-axes/20221215-074621
base: https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git for-next
patch link: https://lore.kernel.org/r/20221214234531.508881-1-ballway%40chromium.org
patch subject: [PATCH v5] HID: multitouch: Add quirks for flipped axes
config: x86_64-randconfig-a013
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/3a75289ec8e3545651146f24516ddd43b27d36af
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Allen-Ballway/HID-multitouch-Add-quirks-for-flipped-axes/20221215-074621
git checkout 3a75289ec8e3545651146f24516ddd43b27d36af
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

ld: drivers/hid/hid-quirks.o: in function `hid_lookup_quirk':
>> drivers/hid/hid-quirks.c:1304: undefined reference to `i2c_hid_get_dmi_quirks'
pahole: .tmp_vmlinux.btf: No such file or directory
.btf.vmlinux.bin.o: file not recognized: file format not recognized

vim +1304 drivers/hid/hid-quirks.c

1258
1259 /**
1260 * hid_lookup_quirk - return any quirks associated with a HID device
1261 * @hdev: the HID device to look for
1262 *
1263 * Description:
1264 * Given a HID device, return any quirks associated with that device.
1265 *
1266 * Return: an unsigned long quirks value.
1267 */
1268 unsigned long hid_lookup_quirk(const struct hid_device *hdev)
1269 {
1270 unsigned long quirks = 0;
1271 const struct hid_device_id *quirk_entry = NULL;
1272
1273 /* NCR devices must not be queried for reports */
1274 if (hdev->bus == BUS_USB &&
1275 hdev->vendor == USB_VENDOR_ID_NCR &&
1276 hdev->product >= USB_DEVICE_ID_NCR_FIRST &&
1277 hdev->product <= USB_DEVICE_ID_NCR_LAST)
1278 return HID_QUIRK_NO_INIT_REPORTS;
1279
1280 /* These devices must be ignored if version (bcdDevice) is too old */
1281 if (hdev->bus == BUS_USB && hdev->vendor == USB_VENDOR_ID_JABRA) {
1282 switch (hdev->product) {
1283 case USB_DEVICE_ID_JABRA_SPEAK_410:
1284 if (hdev->version < 0x0111)
1285 return HID_QUIRK_IGNORE;
1286 break;
1287 case USB_DEVICE_ID_JABRA_SPEAK_510:
1288 if (hdev->version < 0x0214)
1289 return HID_QUIRK_IGNORE;
1290 break;
1291 }
1292 }
1293
1294 mutex_lock(&dquirks_lock);
1295 quirk_entry = hid_exists_dquirk(hdev);
1296 if (quirk_entry)
1297 quirks = quirk_entry->driver_data;
1298 else
1299 quirks = hid_gets_squirk(hdev);
1300 mutex_unlock(&dquirks_lock);
1301
1302 /* Get quirks specific to I2C devices */
1303 if (IS_ENABLED(CONFIG_DMI) && hdev->bus == BUS_I2C)
> 1304 quirks |= i2c_hid_get_dmi_quirks(hdev->vendor, hdev->product);

--
0-DAY CI Kernel Test Service
https://01.org/lkp

Attachments:

(No filename) (3.58 kB)
config (120.57 kB)
Download all attachments

2022-12-15 12:39:28

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v5] HID: multitouch: Add quirks for flipped axes

Attachments:

(No filename) (1.67 kB)
config (139.78 kB)
Download all attachments

2022-12-15 18:12:54

by Allen Ballway

[permalink] [raw]

Subject: [PATCH v6] HID: multitouch: Add quirks for flipped axes

Certain touchscreen devices, such as the ELAN9034, are oriented
incorrectly and report touches on opposite points on the X and Y axes.
For example, a 100x200 screen touched at (10,20) would report (90, 180)
and vice versa.

This is fixed by adding device quirks to transform the touch points
into the correct spaces, from X -> MAX(X) - X, and Y -> MAX(Y) - Y.

Signed-off-by: Allen Ballway <[email protected]>
---
V5 -> V6: Add another IS_ENABLED check for CONFIG_I2C_DMI_CORE because
the module may not be there at all.

V4 -> V5: Add IS_ENABLED check for CONFIG_DMI to prevent linker error.

V3 -> V4: Move quirk logic to i2c_hid_get_dmi_quirks and remove
duplicate quirks from hid-multitouch.

V2 -> V3: Use existing HID_QUIRK_*_INVERT and match the quirk in
hid-quirk, passing down to hid-multitouch through the hid device.

V1 -> V2: Address review comments, change to use DMI match. Confirmed
MT_TOOL_X/Y require transformation and update orientation based on
flipped axes.

drivers/hid/hid-multitouch.c | 39 ++++++++++++++++++---
drivers/hid/hid-quirks.c | 6 ++++
drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c | 43 ++++++++++++++++++++++++
drivers/hid/i2c-hid/i2c-hid.h | 3 ++
4 files changed, 87 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 91a4d3fc30e08..622fe6928104c 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -71,6 +71,7 @@ MODULE_LICENSE("GPL");
#define MT_QUIRK_SEPARATE_APP_REPORT BIT(19)
#define MT_QUIRK_FORCE_MULTI_INPUT BIT(20)
#define MT_QUIRK_DISABLE_WAKEUP BIT(21)
+#define MT_QUIRK_ORIENTATION_INVERT BIT(22)

#define MT_INPUTMODE_TOUCHSCREEN 0x02
#define MT_INPUTMODE_TOUCHPAD 0x03
@@ -1009,6 +1010,7 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
struct mt_usages *slot)
{
struct input_mt *mt = input->mt;
+ struct hid_device *hdev = td->hdev;
__s32 quirks = app->quirks;
bool valid = true;
bool confidence_state = true;
@@ -1086,6 +1088,10 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
int orientation = wide;
int max_azimuth;
int azimuth;
+ int x;
+ int y;
+ int cx;
+ int cy;

if (slot->a != DEFAULT_ZERO) {
/*
@@ -1104,6 +1110,9 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
if (azimuth > max_azimuth * 2)
azimuth -= max_azimuth * 4;
orientation = -azimuth;
+ if (quirks & MT_QUIRK_ORIENTATION_INVERT)
+ orientation = -orientation;
+
}

if (quirks & MT_QUIRK_TOUCH_SIZE_SCALING) {
@@ -1115,10 +1124,23 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
minor = minor >> 1;
}

- input_event(input, EV_ABS, ABS_MT_POSITION_X, *slot->x);
- input_event(input, EV_ABS, ABS_MT_POSITION_Y, *slot->y);
- input_event(input, EV_ABS, ABS_MT_TOOL_X, *slot->cx);
- input_event(input, EV_ABS, ABS_MT_TOOL_Y, *slot->cy);
+ x = hdev->quirks & HID_QUIRK_X_INVERT ?
+ input_abs_get_max(input, ABS_MT_POSITION_X) - *slot->x :
+ *slot->x;
+ y = hdev->quirks & HID_QUIRK_Y_INVERT ?
+ input_abs_get_max(input, ABS_MT_POSITION_Y) - *slot->y :
+ *slot->y;
+ cx = hdev->quirks & HID_QUIRK_X_INVERT ?
+ input_abs_get_max(input, ABS_MT_POSITION_X) - *slot->cx :
+ *slot->cx;
+ cy = hdev->quirks & HID_QUIRK_Y_INVERT ?
+ input_abs_get_max(input, ABS_MT_POSITION_Y) - *slot->cy :
+ *slot->cy;
+
+ input_event(input, EV_ABS, ABS_MT_POSITION_X, x);
+ input_event(input, EV_ABS, ABS_MT_POSITION_Y, y);
+ input_event(input, EV_ABS, ABS_MT_TOOL_X, cx);
+ input_event(input, EV_ABS, ABS_MT_TOOL_Y, cy);
input_event(input, EV_ABS, ABS_MT_DISTANCE, !*slot->tip_state);
input_event(input, EV_ABS, ABS_MT_ORIENTATION, orientation);
input_event(input, EV_ABS, ABS_MT_PRESSURE, *slot->p);
@@ -1735,6 +1757,15 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
if (id->vendor == HID_ANY_ID && id->product == HID_ANY_ID)
td->serial_maybe = true;

+
+ /* Orientation is inverted if the X or Y axes are
+ * flipped, but normalized if both are inverted.
+ */
+ if (hdev->quirks & (HID_QUIRK_X_INVERT | HID_QUIRK_Y_INVERT) &&
+ !((hdev->quirks & HID_QUIRK_X_INVERT)
+ && (hdev->quirks & HID_QUIRK_Y_INVERT)))
+ td->mtclass.quirks = MT_QUIRK_ORIENTATION_INVERT;
+
/* This allows the driver to correctly support devices
* that emit events over several HID messages.
*/
diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
index 0e9702c7f7d6c..78452faf3c9b4 100644
--- a/drivers/hid/hid-quirks.c
+++ b/drivers/hid/hid-quirks.c
@@ -19,6 +19,7 @@
#include <linux/input/elan-i2c-ids.h>

#include "hid-ids.h"
+#include "i2c-hid/i2c-hid.h"

/*
* Alphabetically sorted by vendor then product.
@@ -1298,6 +1299,11 @@ unsigned long hid_lookup_quirk(const struct hid_device *hdev)
quirks = hid_gets_squirk(hdev);
mutex_unlock(&dquirks_lock);

+ /* Get quirks specific to I2C devices */
+ if (IS_ENABLED(CONFIG_I2C_DMI_CORE) && IS_ENABLED(CONFIG_DMI) &&
+ hdev->bus == BUS_I2C)
+ quirks |= i2c_hid_get_dmi_quirks(hdev->vendor, hdev->product);
+
return quirks;
}
EXPORT_SYMBOL_GPL(hid_lookup_quirk);
diff --git a/drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c b/drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c
index 8e0f67455c098..554a7dc285365 100644
--- a/drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c
+++ b/drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c
@@ -10,8 +10,10 @@
#include <linux/types.h>
#include <linux/dmi.h>
#include <linux/mod_devicetable.h>
+#include <linux/hid.h>

#include "i2c-hid.h"
+#include "../hid-ids.h"

struct i2c_hid_desc_override {
@@ -416,6 +418,28 @@ static const struct dmi_system_id i2c_hid_dmi_desc_override_table[] = {
{ } /* Terminate list */
};

+static const struct hid_device_id i2c_hid_elan_flipped_quirks = {
+ HID_DEVICE(BUS_I2C, HID_GROUP_MULTITOUCH_WIN_8, USB_VENDOR_ID_ELAN, 0x2dcd),
+ HID_QUIRK_X_INVERT | HID_QUIRK_Y_INVERT
+};
+
+/*
+ * This list contains devices which have specific issues based on the system
+ * they're on and not just the device itself. The driver_data will have a
+ * specific hid device to match against.
+ */
+static const struct dmi_system_id i2c_hid_dmi_quirk_table[] = {
+ {
+ .ident = "DynaBook K50/FR",
+ .matches = {
+ DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Dynabook Inc."),
+ DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "dynabook K50/FR"),
+ },
+ .driver_data = (void *)&i2c_hid_elan_flipped_quirks,
+ },
+ { } /* Terminate list */
+};
+

struct i2c_hid_desc *i2c_hid_get_dmi_i2c_hid_desc_override(uint8_t *i2c_name)
{
@@ -450,3 +474,22 @@ char *i2c_hid_get_dmi_hid_report_desc_override(uint8_t *i2c_name,
*size = override->hid_report_desc_size;
return override->hid_report_desc;
}
+
+u32 i2c_hid_get_dmi_quirks(const u16 vendor, const u16 product)
+{
+ u32 quirks = 0;
+ const struct dmi_system_id *system_id =
+ dmi_first_match(i2c_hid_dmi_quirk_table);
+
+ if (system_id) {
+ const struct hid_device_id *device_id =
+ (struct hid_device_id *)(system_id->driver_data);
+
+ if (device_id && device_id->vendor == vendor &&
+ device_id->product == product)
+ quirks = device_id->driver_data;
+ }
+
+ return quirks;
+}
+EXPORT_SYMBOL_GPL(i2c_hid_get_dmi_quirks);
diff --git a/drivers/hid/i2c-hid/i2c-hid.h b/drivers/hid/i2c-hid/i2c-hid.h
index 96c75510ad3f1..2c7b66d5caa0f 100644
--- a/drivers/hid/i2c-hid/i2c-hid.h
+++ b/drivers/hid/i2c-hid/i2c-hid.h
@@ -9,6 +9,7 @@
struct i2c_hid_desc *i2c_hid_get_dmi_i2c_hid_desc_override(uint8_t *i2c_name);
char *i2c_hid_get_dmi_hid_report_desc_override(uint8_t *i2c_name,
unsigned int *size);
+u32 i2c_hid_get_dmi_quirks(const u16 vendor, const u16 product);
#else
static inline struct i2c_hid_desc
*i2c_hid_get_dmi_i2c_hid_desc_override(uint8_t *i2c_name)
@@ -16,6 +17,8 @@ static inline struct i2c_hid_desc
static inline char *i2c_hid_get_dmi_hid_report_desc_override(uint8_t *i2c_name,
unsigned int *size)
{ return NULL; }
+static inline u32 i2c_hid_get_dmi_quirks(const u16 vendor, const u16 product)
+{ return 0; }
#endif

/**
--
2.39.0.rc1.256.g54fd8350bd-goog

2023-01-10 20:29:46

by Allen Ballway

[permalink] [raw]

Subject: [PATCH v6 RESEND] HID: multitouch: Add quirks for flipped axes

Certain touchscreen devices, such as the ELAN9034, are oriented
incorrectly and report touches on opposite points on the X and Y axes.
For example, a 100x200 screen touched at (10,20) would report (90, 180)
and vice versa.

This is fixed by adding device quirks to transform the touch points
into the correct spaces, from X -> MAX(X) - X, and Y -> MAX(Y) - Y.

Signed-off-by: Allen Ballway <[email protected]>
---
V5 -> V6: Add another IS_ENABLED check for CONFIG_I2C_DMI_CORE because
the module may not be there at all.

V4 -> V5: Add IS_ENABLED check for CONFIG_DMI to prevent linker error.

V3 -> V4: Move quirk logic to i2c_hid_get_dmi_quirks and remove
duplicate quirks from hid-multitouch.

V2 -> V3: Use existing HID_QUIRK_*_INVERT and match the quirk in
hid-quirk, passing down to hid-multitouch through the hid device.

V1 -> V2: Address review comments, change to use DMI match. Confirmed
MT_TOOL_X/Y require transformation and update orientation based on
flipped axes.

drivers/hid/hid-multitouch.c | 39 ++++++++++++++++++---
drivers/hid/hid-quirks.c | 6 ++++
drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c | 43 ++++++++++++++++++++++++
drivers/hid/i2c-hid/i2c-hid.h | 3 ++
4 files changed, 87 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 91a4d3fc30e08..622fe6928104c 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -71,6 +71,7 @@ MODULE_LICENSE("GPL");
#define MT_QUIRK_SEPARATE_APP_REPORT BIT(19)
#define MT_QUIRK_FORCE_MULTI_INPUT BIT(20)
#define MT_QUIRK_DISABLE_WAKEUP BIT(21)
+#define MT_QUIRK_ORIENTATION_INVERT BIT(22)

#define MT_INPUTMODE_TOUCHSCREEN 0x02
#define MT_INPUTMODE_TOUCHPAD 0x03
@@ -1009,6 +1010,7 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
struct mt_usages *slot)
{
struct input_mt *mt = input->mt;
+ struct hid_device *hdev = td->hdev;
__s32 quirks = app->quirks;
bool valid = true;
bool confidence_state = true;
@@ -1086,6 +1088,10 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
int orientation = wide;
int max_azimuth;
int azimuth;
+ int x;
+ int y;
+ int cx;
+ int cy;

if (slot->a != DEFAULT_ZERO) {
/*
@@ -1104,6 +1110,9 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
if (azimuth > max_azimuth * 2)
azimuth -= max_azimuth * 4;
orientation = -azimuth;
+ if (quirks & MT_QUIRK_ORIENTATION_INVERT)
+ orientation = -orientation;
+
}

if (quirks & MT_QUIRK_TOUCH_SIZE_SCALING) {
@@ -1115,10 +1124,23 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
minor = minor >> 1;
}

- input_event(input, EV_ABS, ABS_MT_POSITION_X, *slot->x);
- input_event(input, EV_ABS, ABS_MT_POSITION_Y, *slot->y);
- input_event(input, EV_ABS, ABS_MT_TOOL_X, *slot->cx);
- input_event(input, EV_ABS, ABS_MT_TOOL_Y, *slot->cy);
+ x = hdev->quirks & HID_QUIRK_X_INVERT ?
+ input_abs_get_max(input, ABS_MT_POSITION_X) - *slot->x :
+ *slot->x;
+ y = hdev->quirks & HID_QUIRK_Y_INVERT ?
+ input_abs_get_max(input, ABS_MT_POSITION_Y) - *slot->y :
+ *slot->y;
+ cx = hdev->quirks & HID_QUIRK_X_INVERT ?
+ input_abs_get_max(input, ABS_MT_POSITION_X) - *slot->cx :
+ *slot->cx;
+ cy = hdev->quirks & HID_QUIRK_Y_INVERT ?
+ input_abs_get_max(input, ABS_MT_POSITION_Y) - *slot->cy :
+ *slot->cy;
+
+ input_event(input, EV_ABS, ABS_MT_POSITION_X, x);
+ input_event(input, EV_ABS, ABS_MT_POSITION_Y, y);
+ input_event(input, EV_ABS, ABS_MT_TOOL_X, cx);
+ input_event(input, EV_ABS, ABS_MT_TOOL_Y, cy);
input_event(input, EV_ABS, ABS_MT_DISTANCE, !*slot->tip_state);
input_event(input, EV_ABS, ABS_MT_ORIENTATION, orientation);
input_event(input, EV_ABS, ABS_MT_PRESSURE, *slot->p);
@@ -1735,6 +1757,15 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
if (id->vendor == HID_ANY_ID && id->product == HID_ANY_ID)
td->serial_maybe = true;

+
+ /* Orientation is inverted if the X or Y axes are
+ * flipped, but normalized if both are inverted.
+ */
+ if (hdev->quirks & (HID_QUIRK_X_INVERT | HID_QUIRK_Y_INVERT) &&
+ !((hdev->quirks & HID_QUIRK_X_INVERT)
+ && (hdev->quirks & HID_QUIRK_Y_INVERT)))
+ td->mtclass.quirks = MT_QUIRK_ORIENTATION_INVERT;
+
/* This allows the driver to correctly support devices
* that emit events over several HID messages.
*/
diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
index 0e9702c7f7d6c..78452faf3c9b4 100644
--- a/drivers/hid/hid-quirks.c
+++ b/drivers/hid/hid-quirks.c
@@ -19,6 +19,7 @@
#include <linux/input/elan-i2c-ids.h>

#include "hid-ids.h"
+#include "i2c-hid/i2c-hid.h"

/*
* Alphabetically sorted by vendor then product.
@@ -1298,6 +1299,11 @@ unsigned long hid_lookup_quirk(const struct hid_device *hdev)
quirks = hid_gets_squirk(hdev);
mutex_unlock(&dquirks_lock);

+ /* Get quirks specific to I2C devices */
+ if (IS_ENABLED(CONFIG_I2C_DMI_CORE) && IS_ENABLED(CONFIG_DMI) &&
+ hdev->bus == BUS_I2C)
+ quirks |= i2c_hid_get_dmi_quirks(hdev->vendor, hdev->product);
+
return quirks;
}
EXPORT_SYMBOL_GPL(hid_lookup_quirk);
diff --git a/drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c b/drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c
index 8e0f67455c098..554a7dc285365 100644
--- a/drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c
+++ b/drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c
@@ -10,8 +10,10 @@
#include <linux/types.h>
#include <linux/dmi.h>
#include <linux/mod_devicetable.h>
+#include <linux/hid.h>

#include "i2c-hid.h"
+#include "../hid-ids.h"

struct i2c_hid_desc_override {
@@ -416,6 +418,28 @@ static const struct dmi_system_id i2c_hid_dmi_desc_override_table[] = {
{ } /* Terminate list */
};

+static const struct hid_device_id i2c_hid_elan_flipped_quirks = {
+ HID_DEVICE(BUS_I2C, HID_GROUP_MULTITOUCH_WIN_8, USB_VENDOR_ID_ELAN, 0x2dcd),
+ HID_QUIRK_X_INVERT | HID_QUIRK_Y_INVERT
+};
+
+/*
+ * This list contains devices which have specific issues based on the system
+ * they're on and not just the device itself. The driver_data will have a
+ * specific hid device to match against.
+ */
+static const struct dmi_system_id i2c_hid_dmi_quirk_table[] = {
+ {
+ .ident = "DynaBook K50/FR",
+ .matches = {
+ DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Dynabook Inc."),
+ DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "dynabook K50/FR"),
+ },
+ .driver_data = (void *)&i2c_hid_elan_flipped_quirks,
+ },
+ { } /* Terminate list */
+};
+

struct i2c_hid_desc *i2c_hid_get_dmi_i2c_hid_desc_override(uint8_t *i2c_name)
{
@@ -450,3 +474,22 @@ char *i2c_hid_get_dmi_hid_report_desc_override(uint8_t *i2c_name,
*size = override->hid_report_desc_size;
return override->hid_report_desc;
}
+
+u32 i2c_hid_get_dmi_quirks(const u16 vendor, const u16 product)
+{
+ u32 quirks = 0;
+ const struct dmi_system_id *system_id =
+ dmi_first_match(i2c_hid_dmi_quirk_table);
+
+ if (system_id) {
+ const struct hid_device_id *device_id =
+ (struct hid_device_id *)(system_id->driver_data);
+
+ if (device_id && device_id->vendor == vendor &&
+ device_id->product == product)
+ quirks = device_id->driver_data;
+ }
+
+ return quirks;
+}
+EXPORT_SYMBOL_GPL(i2c_hid_get_dmi_quirks);
diff --git a/drivers/hid/i2c-hid/i2c-hid.h b/drivers/hid/i2c-hid/i2c-hid.h
index 96c75510ad3f1..2c7b66d5caa0f 100644
--- a/drivers/hid/i2c-hid/i2c-hid.h
+++ b/drivers/hid/i2c-hid/i2c-hid.h
@@ -9,6 +9,7 @@
struct i2c_hid_desc *i2c_hid_get_dmi_i2c_hid_desc_override(uint8_t *i2c_name);
char *i2c_hid_get_dmi_hid_report_desc_override(uint8_t *i2c_name,
unsigned int *size);
+u32 i2c_hid_get_dmi_quirks(const u16 vendor, const u16 product);
#else
static inline struct i2c_hid_desc
*i2c_hid_get_dmi_i2c_hid_desc_override(uint8_t *i2c_name)
@@ -16,6 +17,8 @@ static inline struct i2c_hid_desc
static inline char *i2c_hid_get_dmi_hid_report_desc_override(uint8_t *i2c_name,
unsigned int *size)
{ return NULL; }
+static inline u32 i2c_hid_get_dmi_quirks(const u16 vendor, const u16 product)
+{ return 0; }
#endif

/**
--
2.39.0.rc1.256.g54fd8350bd-goog

2023-01-18 10:22:15

by Jiri Kosina

[permalink] [raw]

Subject: Re: [PATCH v6 RESEND] HID: multitouch: Add quirks for flipped axes

On Tue, 10 Jan 2023, Allen Ballway wrote:

> Certain touchscreen devices, such as the ELAN9034, are oriented
> incorrectly and report touches on opposite points on the X and Y axes.
> For example, a 100x200 screen touched at (10,20) would report (90, 180)
> and vice versa.
>
> This is fixed by adding device quirks to transform the touch points
> into the correct spaces, from X -> MAX(X) - X, and Y -> MAX(Y) - Y.
>
> Signed-off-by: Allen Ballway <[email protected]>
> ---
> V5 -> V6: Add another IS_ENABLED check for CONFIG_I2C_DMI_CORE because
> the module may not be there at all.
>
> V4 -> V5: Add IS_ENABLED check for CONFIG_DMI to prevent linker error.
>
> V3 -> V4: Move quirk logic to i2c_hid_get_dmi_quirks and remove
> duplicate quirks from hid-multitouch.
>
> V2 -> V3: Use existing HID_QUIRK_*_INVERT and match the quirk in
> hid-quirk, passing down to hid-multitouch through the hid device.
>
> V1 -> V2: Address review comments, change to use DMI match. Confirmed
> MT_TOOL_X/Y require transformation and update orientation based on
> flipped axes.

Appplied, thank you.

--
Jiri Kosina
SUSE Labs

2023-01-31 19:47:14

by Allen Ballway

[permalink] [raw]

Subject: [PATCH v7] HID: multitouch: Add quirks for flipped axes

Certain touchscreen devices, such as the ELAN9034, are oriented
incorrectly and report touches on opposite points on the X and Y axes.
For example, a 100x200 screen touched at (10,20) would report (90, 180)
and vice versa.

This is fixed by adding device quirks to transform the touch points
into the correct spaces, from X -> MAX(X) - X, and Y -> MAX(Y) - Y.

Signed-off-by: Allen Ballway <[email protected]>
---
V6 -> V7: Fix typo in config name.

V5 -> V6: Add another IS_ENABLED check for CONFIG_I2C_DMI_CORE because
the module may not be there at all.

V4 -> V5: Add IS_ENABLED check for CONFIG_DMI to prevent linker error.

V3 -> V4: Move quirk logic to i2c_hid_get_dmi_quirks and remove
duplicate quirks from hid-multitouch.

V2 -> V3: Use existing HID_QUIRK_*_INVERT and match the quirk in
hid-quirk, passing down to hid-multitouch through the hid device.

V1 -> V2: Address review comments, change to use DMI match. Confirmed
MT_TOOL_X/Y require transformation and update orientation based on
flipped axes.

drivers/hid/hid-multitouch.c | 39 ++++++++++++++++++---
drivers/hid/hid-quirks.c | 6 ++++
drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c | 43 ++++++++++++++++++++++++
drivers/hid/i2c-hid/i2c-hid.h | 3 ++
4 files changed, 87 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 91a4d3fc30e08..622fe6928104c 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -71,6 +71,7 @@ MODULE_LICENSE("GPL");
#define MT_QUIRK_SEPARATE_APP_REPORT BIT(19)
#define MT_QUIRK_FORCE_MULTI_INPUT BIT(20)
#define MT_QUIRK_DISABLE_WAKEUP BIT(21)
+#define MT_QUIRK_ORIENTATION_INVERT BIT(22)

#define MT_INPUTMODE_TOUCHSCREEN 0x02
#define MT_INPUTMODE_TOUCHPAD 0x03
@@ -1009,6 +1010,7 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
struct mt_usages *slot)
{
struct input_mt *mt = input->mt;
+ struct hid_device *hdev = td->hdev;
__s32 quirks = app->quirks;
bool valid = true;
bool confidence_state = true;
@@ -1086,6 +1088,10 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
int orientation = wide;
int max_azimuth;
int azimuth;
+ int x;
+ int y;
+ int cx;
+ int cy;

if (slot->a != DEFAULT_ZERO) {
/*
@@ -1104,6 +1110,9 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
if (azimuth > max_azimuth * 2)
azimuth -= max_azimuth * 4;
orientation = -azimuth;
+ if (quirks & MT_QUIRK_ORIENTATION_INVERT)
+ orientation = -orientation;
+
}

if (quirks & MT_QUIRK_TOUCH_SIZE_SCALING) {
@@ -1115,10 +1124,23 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
minor = minor >> 1;
}

- input_event(input, EV_ABS, ABS_MT_POSITION_X, *slot->x);
- input_event(input, EV_ABS, ABS_MT_POSITION_Y, *slot->y);
- input_event(input, EV_ABS, ABS_MT_TOOL_X, *slot->cx);
- input_event(input, EV_ABS, ABS_MT_TOOL_Y, *slot->cy);
+ x = hdev->quirks & HID_QUIRK_X_INVERT ?
+ input_abs_get_max(input, ABS_MT_POSITION_X) - *slot->x :
+ *slot->x;
+ y = hdev->quirks & HID_QUIRK_Y_INVERT ?
+ input_abs_get_max(input, ABS_MT_POSITION_Y) - *slot->y :
+ *slot->y;
+ cx = hdev->quirks & HID_QUIRK_X_INVERT ?
+ input_abs_get_max(input, ABS_MT_POSITION_X) - *slot->cx :
+ *slot->cx;
+ cy = hdev->quirks & HID_QUIRK_Y_INVERT ?
+ input_abs_get_max(input, ABS_MT_POSITION_Y) - *slot->cy :
+ *slot->cy;
+
+ input_event(input, EV_ABS, ABS_MT_POSITION_X, x);
+ input_event(input, EV_ABS, ABS_MT_POSITION_Y, y);
+ input_event(input, EV_ABS, ABS_MT_TOOL_X, cx);
+ input_event(input, EV_ABS, ABS_MT_TOOL_Y, cy);
input_event(input, EV_ABS, ABS_MT_DISTANCE, !*slot->tip_state);
input_event(input, EV_ABS, ABS_MT_ORIENTATION, orientation);
input_event(input, EV_ABS, ABS_MT_PRESSURE, *slot->p);
@@ -1735,6 +1757,15 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
if (id->vendor == HID_ANY_ID && id->product == HID_ANY_ID)
td->serial_maybe = true;

+
+ /* Orientation is inverted if the X or Y axes are
+ * flipped, but normalized if both are inverted.
+ */
+ if (hdev->quirks & (HID_QUIRK_X_INVERT | HID_QUIRK_Y_INVERT) &&
+ !((hdev->quirks & HID_QUIRK_X_INVERT)
+ && (hdev->quirks & HID_QUIRK_Y_INVERT)))
+ td->mtclass.quirks = MT_QUIRK_ORIENTATION_INVERT;
+
/* This allows the driver to correctly support devices
* that emit events over several HID messages.
*/
diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
index 0e9702c7f7d6c..78452faf3c9b4 100644
--- a/drivers/hid/hid-quirks.c
+++ b/drivers/hid/hid-quirks.c
@@ -19,6 +19,7 @@
#include <linux/input/elan-i2c-ids.h>

#include "hid-ids.h"
+#include "i2c-hid/i2c-hid.h"

/*
* Alphabetically sorted by vendor then product.
@@ -1298,6 +1299,11 @@ unsigned long hid_lookup_quirk(const struct hid_device *hdev)
quirks = hid_gets_squirk(hdev);
mutex_unlock(&dquirks_lock);

+ /* Get quirks specific to I2C devices */
+ if (IS_ENABLED(CONFIG_I2C_HID_CORE) && IS_ENABLED(CONFIG_DMI) &&
+ hdev->bus == BUS_I2C)
+ quirks |= i2c_hid_get_dmi_quirks(hdev->vendor, hdev->product);
+
return quirks;
}
EXPORT_SYMBOL_GPL(hid_lookup_quirk);
diff --git a/drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c b/drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c
index 8e0f67455c098..554a7dc285365 100644
--- a/drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c
+++ b/drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c
@@ -10,8 +10,10 @@
#include <linux/types.h>
#include <linux/dmi.h>
#include <linux/mod_devicetable.h>
+#include <linux/hid.h>

#include "i2c-hid.h"
+#include "../hid-ids.h"

struct i2c_hid_desc_override {
@@ -416,6 +418,28 @@ static const struct dmi_system_id i2c_hid_dmi_desc_override_table[] = {
{ } /* Terminate list */
};

+static const struct hid_device_id i2c_hid_elan_flipped_quirks = {
+ HID_DEVICE(BUS_I2C, HID_GROUP_MULTITOUCH_WIN_8, USB_VENDOR_ID_ELAN, 0x2dcd),
+ HID_QUIRK_X_INVERT | HID_QUIRK_Y_INVERT
+};
+
+/*
+ * This list contains devices which have specific issues based on the system
+ * they're on and not just the device itself. The driver_data will have a
+ * specific hid device to match against.
+ */
+static const struct dmi_system_id i2c_hid_dmi_quirk_table[] = {
+ {
+ .ident = "DynaBook K50/FR",
+ .matches = {
+ DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Dynabook Inc."),
+ DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "dynabook K50/FR"),
+ },
+ .driver_data = (void *)&i2c_hid_elan_flipped_quirks,
+ },
+ { } /* Terminate list */
+};
+

struct i2c_hid_desc *i2c_hid_get_dmi_i2c_hid_desc_override(uint8_t *i2c_name)
{
@@ -450,3 +474,22 @@ char *i2c_hid_get_dmi_hid_report_desc_override(uint8_t *i2c_name,
*size = override->hid_report_desc_size;
return override->hid_report_desc;
}
+
+u32 i2c_hid_get_dmi_quirks(const u16 vendor, const u16 product)
+{
+ u32 quirks = 0;
+ const struct dmi_system_id *system_id =
+ dmi_first_match(i2c_hid_dmi_quirk_table);
+
+ if (system_id) {
+ const struct hid_device_id *device_id =
+ (struct hid_device_id *)(system_id->driver_data);
+
+ if (device_id && device_id->vendor == vendor &&
+ device_id->product == product)
+ quirks = device_id->driver_data;
+ }
+
+ return quirks;
+}
+EXPORT_SYMBOL_GPL(i2c_hid_get_dmi_quirks);
diff --git a/drivers/hid/i2c-hid/i2c-hid.h b/drivers/hid/i2c-hid/i2c-hid.h
index 96c75510ad3f1..2c7b66d5caa0f 100644
--- a/drivers/hid/i2c-hid/i2c-hid.h
+++ b/drivers/hid/i2c-hid/i2c-hid.h
@@ -9,6 +9,7 @@
struct i2c_hid_desc *i2c_hid_get_dmi_i2c_hid_desc_override(uint8_t *i2c_name);
char *i2c_hid_get_dmi_hid_report_desc_override(uint8_t *i2c_name,
unsigned int *size);
+u32 i2c_hid_get_dmi_quirks(const u16 vendor, const u16 product);
#else
static inline struct i2c_hid_desc
*i2c_hid_get_dmi_i2c_hid_desc_override(uint8_t *i2c_name)
@@ -16,6 +17,8 @@ static inline struct i2c_hid_desc
static inline char *i2c_hid_get_dmi_hid_report_desc_override(uint8_t *i2c_name,
unsigned int *size)
{ return NULL; }
+static inline u32 i2c_hid_get_dmi_quirks(const u16 vendor, const u16 product)
+{ return 0; }
#endif

/**
--
2.39.0.rc1.256.g54fd8350bd-goog