[PATCH] platform/x86: touchscreen_dmi: Add swap-x-y quirk for Goodix touchscreen on Estar Beauty HD tablet

[PATCH] platform/x86: touchscreen_dmi: Add swap-x-y quirk for Goodix touchscreen on Estar Beauty HD tablet

[Hans de Goede]

The Estar Beauty HD (MID 7316R) tablet uses a Goodix touchscreen,
with the X and Y coordinates swapped compared to the LCD panel.

Add a touchscreen_dmi entry for this adding a "touchscreen-swapped-x-y"
device-property to the i2c-client instantiated for this device before
the driver binds.

This is the first entry of a Goodix touchscreen to touchscreen_dmi.c,
so far DMI quirks for Goodix touchscreen's have been added directly
to drivers/input/touchscreen/goodix.c. Currently there are 3
DMI tables in goodix.c:
1. rotated_screen[] for devices where the touchscreen is rotated
   180 degrees vs the LCD panel
2. inverted_x_screen[] for devices where the X axis is inverted
3. nine_bytes_report[] for devices which use a non standard touch
   report size

Arguably only 3. really needs to be inside the driver and the other
2 cases are better handled through the generic touchscreen DMI quirk
mechanism from touchscreen_dmi.c, which allows adding device-props to
any i2c-client. Esp. now that goodix.c is using the generic
touchscreen_properties code.

Alternative to the approach from this patch we could add a 4th
dmi_system_id table for devices with swapped-x-y axis to goodix.c,
but that seems undesirable.

Cc: Bastien Nocera <hadess@hadess.net>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/touchscreen_dmi.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/platform/x86/touchscreen_dmi.c b/drivers/platform/x86/touchscreen_dmi.c
index 5783139d0a11..c4de932302d6 100644
--- a/drivers/platform/x86/touchscreen_dmi.c
+++ b/drivers/platform/x86/touchscreen_dmi.c
@@ -263,6 +263,16 @@ static const struct ts_dmi_data digma_citi_e200_data = {
 	.properties	= digma_citi_e200_props,
 };
 
+static const struct property_entry estar_beauty_hd_props[] = {
+	PROPERTY_ENTRY_BOOL("touchscreen-swapped-x-y"),
+	{ }
+};
+
+static const struct ts_dmi_data estar_beauty_hd_data = {
+	.acpi_name	= "GDIX1001:00",
+	.properties	= estar_beauty_hd_props,
+};
+
 static const struct property_entry gp_electronic_t701_props[] = {
 	PROPERTY_ENTRY_U32("touchscreen-size-x", 960),
 	PROPERTY_ENTRY_U32("touchscreen-size-y", 640),
@@ -942,6 +952,14 @@ const struct dmi_system_id touchscreen_dmi_table[] = {
 			DMI_MATCH(DMI_BOARD_NAME, "Cherry Trail CR"),
 		},
 	},
+	{
+		/* Estar Beauty HD (MID 7316R) */
+		.driver_data = (void *)&estar_beauty_hd_data,
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Estar"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "eSTAR BEAUTY HD Intel Quad core"),
+		},
+	},
 	{
 		/* GP-electronic T701 */
 		.driver_data = (void *)&gp_electronic_t701_data,
-- 
2.28.0

Re: [PATCH] platform/x86: touchscreen_dmi: Add swap-x-y quirk for Goodix touchscreen on Estar Beauty HD tablet

[Hans de Goede]

Hi,

On 12/24/20 3:02 PM, Christopher Piggott wrote:
> Hans, I have the almost the exact same problem with a Xenarc 705T that has an eGalax controller on it.  In this case, it's not just a swap of X and Y, it's actually rotated 90 degrees.  I have been searching for a more generic solution to this problem.  I'm wondering if there's a more generic way to do this, up in maybe hid-input or somewhere, and I'm also wondering if it's at all feasible to do it so that you don't specify swapxy, rotation, and size transformations.  In my mind this calls for an affine transform, where if somebody really needs to manipulate this they specify a matrix.  I can see wanting to have parameters like you added to make it easier for people, but I would just make those parameters formulate an affine transformation matrix that could also be specified explicitly as x1, x2, y1, y1.
> 
> At one point in time, the eGalax driver handled this with a swapxy.  Somebody somewhere along the line thought that the eGalax driver could be discarded in lieu of standard handling in hid, but without the ability to transform that's not the case.  In my particular case, I'm actually doing this for android, where these drivers aren't even loaded as modules (so it's kind of a pain).
> 
> What do you think?

Any transformation which you need because of the touchscreen and LCD panel orientation
not matching can be achieved through the swap-x-y and invert-x and invert-y device properties
defined in:

Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml

Note that swap-x-y will also swap the width/height and min/max properties for the
touchscreen. So you specify the min and max values as reported by the controller and
since the touchscreen is mounted 90 degrees rotated those will then be swapped
before being reported to userspace.

This is all handled by the generic touchscreen-properties helpers from:
drivers/input/touchscreen/of_touchscreen.c
(touchscreen_parse_properties, touchscreen_report_pos and others)

You can find plenty examples of drivers using those.

The proper solution for this would be to convert the driver which you are
using to use these helpers, see e.g. commit fafef982c735 ("Input: goodix - 
use generic touchscreen_properties"), in combination with setting the
properties needed for your device in the devicetree of your device.

And yes if you need this with a standard HID device then the 
drivers/hid/hid-multitouch.c properly needs to get support for the
helpers from drivers/input/touchscreen/of_touchscreen.c . Which is not
entirely trivial, but should be not that hard to do.

Regards,

Hans





> 
> 
> 
> On Thu, Dec 24, 2020 at 8:55 AM Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>> wrote:
> 
>     The Estar Beauty HD (MID 7316R) tablet uses a Goodix touchscreen,
>     with the X and Y coordinates swapped compared to the LCD panel.
> 
>     Add a touchscreen_dmi entry for this adding a "touchscreen-swapped-x-y"
>     device-property to the i2c-client instantiated for this device before
>     the driver binds.
> 
>     This is the first entry of a Goodix touchscreen to touchscreen_dmi.c,
>     so far DMI quirks for Goodix touchscreen's have been added directly
>     to drivers/input/touchscreen/goodix.c. Currently there are 3
>     DMI tables in goodix.c:
>     1. rotated_screen[] for devices where the touchscreen is rotated
>        180 degrees vs the LCD panel
>     2. inverted_x_screen[] for devices where the X axis is inverted
>     3. nine_bytes_report[] for devices which use a non standard touch
>        report size
> 
>     Arguably only 3. really needs to be inside the driver and the other
>     2 cases are better handled through the generic touchscreen DMI quirk
>     mechanism from touchscreen_dmi.c, which allows adding device-props to
>     any i2c-client. Esp. now that goodix.c is using the generic
>     touchscreen_properties code.
> 
>     Alternative to the approach from this patch we could add a 4th
>     dmi_system_id table for devices with swapped-x-y axis to goodix.c,
>     but that seems undesirable.
> 
>     Cc: Bastien Nocera <hadess@hadess.net <mailto:hadess@hadess.net>>
>     Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com <mailto:dmitry.torokhov@gmail.com>>
>     Signed-off-by: Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>>
>     ---
>      drivers/platform/x86/touchscreen_dmi.c | 18 ++++++++++++++++++
>      1 file changed, 18 insertions(+)
> 
>     diff --git a/drivers/platform/x86/touchscreen_dmi.c b/drivers/platform/x86/touchscreen_dmi.c
>     index 5783139d0a11..c4de932302d6 100644
>     --- a/drivers/platform/x86/touchscreen_dmi.c
>     +++ b/drivers/platform/x86/touchscreen_dmi.c
>     @@ -263,6 +263,16 @@ static const struct ts_dmi_data digma_citi_e200_data = {
>             .properties     = digma_citi_e200_props,
>      };
> 
>     +static const struct property_entry estar_beauty_hd_props[] = {
>     +       PROPERTY_ENTRY_BOOL("touchscreen-swapped-x-y"),
>     +       { }
>     +};
>     +
>     +static const struct ts_dmi_data estar_beauty_hd_data = {
>     +       .acpi_name      = "GDIX1001:00",
>     +       .properties     = estar_beauty_hd_props,
>     +};
>     +
>      static const struct property_entry gp_electronic_t701_props[] = {
>             PROPERTY_ENTRY_U32("touchscreen-size-x", 960),
>             PROPERTY_ENTRY_U32("touchscreen-size-y", 640),
>     @@ -942,6 +952,14 @@ const struct dmi_system_id touchscreen_dmi_table[] = {
>                             DMI_MATCH(DMI_BOARD_NAME, "Cherry Trail CR"),
>                     },
>             },
>     +       {
>     +               /* Estar Beauty HD (MID 7316R) */
>     +               .driver_data = (void *)&estar_beauty_hd_data,
>     +               .matches = {
>     +                       DMI_MATCH(DMI_SYS_VENDOR, "Estar"),
>     +                       DMI_MATCH(DMI_PRODUCT_NAME, "eSTAR BEAUTY HD Intel Quad core"),
>     +               },
>     +       },
>             {
>                     /* GP-electronic T701 */
>                     .driver_data = (void *)&gp_electronic_t701_data,
>     -- 
>     2.28.0
> 

Re: [PATCH] platform/x86: touchscreen_dmi: Add swap-x-y quirk for Goodix touchscreen on Estar Beauty HD tablet

[Hans de Goede]

Hi,

On 12/24/20 2:51 PM, Hans de Goede wrote:
> The Estar Beauty HD (MID 7316R) tablet uses a Goodix touchscreen,
> with the X and Y coordinates swapped compared to the LCD panel.
> 
> Add a touchscreen_dmi entry for this adding a "touchscreen-swapped-x-y"
> device-property to the i2c-client instantiated for this device before
> the driver binds.
> 
> This is the first entry of a Goodix touchscreen to touchscreen_dmi.c,
> so far DMI quirks for Goodix touchscreen's have been added directly
> to drivers/input/touchscreen/goodix.c. Currently there are 3
> DMI tables in goodix.c:
> 1. rotated_screen[] for devices where the touchscreen is rotated
>    180 degrees vs the LCD panel
> 2. inverted_x_screen[] for devices where the X axis is inverted
> 3. nine_bytes_report[] for devices which use a non standard touch
>    report size
> 
> Arguably only 3. really needs to be inside the driver and the other
> 2 cases are better handled through the generic touchscreen DMI quirk
> mechanism from touchscreen_dmi.c, which allows adding device-props to
> any i2c-client. Esp. now that goodix.c is using the generic
> touchscreen_properties code.
> 
> Alternative to the approach from this patch we could add a 4th
> dmi_system_id table for devices with swapped-x-y axis to goodix.c,
> but that seems undesirable.
> 
> Cc: Bastien Nocera <hadess@hadess.net>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans

> ---
>  drivers/platform/x86/touchscreen_dmi.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/platform/x86/touchscreen_dmi.c b/drivers/platform/x86/touchscreen_dmi.c
> index 5783139d0a11..c4de932302d6 100644
> --- a/drivers/platform/x86/touchscreen_dmi.c
> +++ b/drivers/platform/x86/touchscreen_dmi.c
> @@ -263,6 +263,16 @@ static const struct ts_dmi_data digma_citi_e200_data = {
>  	.properties	= digma_citi_e200_props,
>  };
>  
> +static const struct property_entry estar_beauty_hd_props[] = {
> +	PROPERTY_ENTRY_BOOL("touchscreen-swapped-x-y"),
> +	{ }
> +};
> +
> +static const struct ts_dmi_data estar_beauty_hd_data = {
> +	.acpi_name	= "GDIX1001:00",
> +	.properties	= estar_beauty_hd_props,
> +};
> +
>  static const struct property_entry gp_electronic_t701_props[] = {
>  	PROPERTY_ENTRY_U32("touchscreen-size-x", 960),
>  	PROPERTY_ENTRY_U32("touchscreen-size-y", 640),
> @@ -942,6 +952,14 @@ const struct dmi_system_id touchscreen_dmi_table[] = {
>  			DMI_MATCH(DMI_BOARD_NAME, "Cherry Trail CR"),
>  		},
>  	},
> +	{
> +		/* Estar Beauty HD (MID 7316R) */
> +		.driver_data = (void *)&estar_beauty_hd_data,
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Estar"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "eSTAR BEAUTY HD Intel Quad core"),
> +		},
> +	},
>  	{
>  		/* GP-electronic T701 */
>  		.driver_data = (void *)&gp_electronic_t701_data,
> 

Re: [PATCH] platform/x86: touchscreen_dmi: Add swap-x-y quirk for Goodix touchscreen on Estar Beauty HD tablet

[Bastien Nocera]

On Mon, 2021-01-04 at 13:00 +0100, Hans de Goede wrote:
> <
> <snip>
> Thank you for your patch, I've applied this patch to my review-hans 
> branch:
>  
> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
> 
> Note it will show up in my review-hans branch once I've pushed my
> local branch there, which might take a while.

Do you plan on sending a patch to migrate the other quirks in goodix.c
itself to touchscreen_dmi.c?

(also, feels a bit weird that your robot replies to yourself like it
was a different person ;)

Re: [PATCH] platform/x86: touchscreen_dmi: Add swap-x-y quirk for Goodix touchscreen on Estar Beauty HD tablet

[Hans de Goede]

Hi,

On 1/4/21 1:04 PM, Bastien Nocera wrote:
> On Mon, 2021-01-04 at 13:00 +0100, Hans de Goede wrote:
>> <
>> <snip>
>> Thank you for your patch, I've applied this patch to my review-hans 
>> branch:
>>  
>> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
>>
>> Note it will show up in my review-hans branch once I've pushed my
>> local branch there, which might take a while.
> 
> Do you plan on sending a patch to migrate the other quirks in goodix.c
> itself to touchscreen_dmi.c?

Ideally yes, but the code in touchscreen_dmi.c matches on the ACPI device-name
the device to know which i2c-device to attach the properties to and that
is based on the ACPI HID.

This is likely "GDIX1001" in most cases, but according to the git log the
"Teclast X98 Pro"
"Cube I15-TC"
"Teclast X89"

Quirks have been added after the "GDIX1002" HID was added, so in theory
they could be using that.

And also some DSTD-s have multiple GDIX1001 nodes with only 1 being
enabled (returning non 0 from _STA) in which case the device-name could
be e.g. "GDIX1001:01" instead of "GDIX1001:00".

So I've just checked if I can find dmesg output or an acpidump for all devices
with a quirk to ensure that I get the device-name right. I can confirm that the
ACPI device-name for the touchscreen is "GDIX1001:00" on the:

  "Teclast X98 Pro"
  "WinBook TW100"
  "WinBook TW700"

I could be either "GDIX1001:00" or "GDIX1001:01" on the "Teclast X89", but
I have that one myself, so I can check that myself.

That just leaves the "Cube I15-TC", for which I cannot find an acpidump
or dmesg output. I will mail the author of the patch adding the quirk for
that one with a patch moving the quirk asking him to test the patch
(and fix it if necessary).

###

Dmitry, once I have a patch ready to move the goodix rotated_screen
and inverted_x_screen DMI quirk tables to platform/drivers/x86/touchscreen_dmi.c
where all other (x86) touchscreen quirks have been gathered so far, the question
becomes how to merge that patch ?

I see 2 options:

1. Have 2 separate patches, one adding the quirks to
platform/drivers/x86/touchscreen_dmi.c and a second patch removing the
DMI tables (and the code handling them) from goodix.c. And then merge
them through the pdx86 resp. the input tree.

2. Have 1 big patch doing both.

The downside of 1. is that there might be a point in git history where
the coordinates of the touchscreens regress. Depending on which pull-req
lands first (if the pdx86 pull-req for 5.12-rc1 gets merged first there
is no issue). But only when git-bisecting so I think that 1. is best to
avoid any merge issues. At least platform/drivers/x86/touchscreen_dmi.c
sees a lot of activity every cycle. So another option would be to 
do 1 big patch and then merge that through the pdx86 tree (I can provide
an immutable branch for that).

Dmitry if you can let me know which way you would prefer to move forward
with this then I can prepare the 1 or 2 patches (once I hear back from the
"Cube I15-TC" quirk patch author).

Regards,

Hans

Re: [PATCH] platform/x86: touchscreen_dmi: Add swap-x-y quirk for Goodix touchscreen on Estar Beauty HD tablet

[Dmitry Torokhov]

Hi Hans,

On Mon, Jan 04, 2021 at 02:24:27PM +0100, Hans de Goede wrote:
> 
> Dmitry, once I have a patch ready to move the goodix rotated_screen
> and inverted_x_screen DMI quirk tables to platform/drivers/x86/touchscreen_dmi.c
> where all other (x86) touchscreen quirks have been gathered so far, the question
> becomes how to merge that patch ?
> 
> I see 2 options:
> 
> 1. Have 2 separate patches, one adding the quirks to
> platform/drivers/x86/touchscreen_dmi.c and a second patch removing the
> DMI tables (and the code handling them) from goodix.c. And then merge
> them through the pdx86 resp. the input tree.
> 
> 2. Have 1 big patch doing both.
> 
> The downside of 1. is that there might be a point in git history where
> the coordinates of the touchscreens regress. Depending on which pull-req
> lands first (if the pdx86 pull-req for 5.12-rc1 gets merged first there
> is no issue). But only when git-bisecting so I think that 1. is best to
> avoid any merge issues. At least platform/drivers/x86/touchscreen_dmi.c
> sees a lot of activity every cycle. So another option would be to 
> do 1 big patch and then merge that through the pdx86 tree (I can provide
> an immutable branch for that).
> 
> Dmitry if you can let me know which way you would prefer to move forward
> with this then I can prepare the 1 or 2 patches (once I hear back from the
> "Cube I15-TC" quirk patch author).

I am fine with a single patch going through either platform or input
tree. We could even have an immutable branch off 5.10 and merge it into
both trees to ensure there are no clashes.

Thanks.

-- 
Dmitry