| Message ID | 20201224135158.10976-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 46c54cf2706122c37497896d56d67b0c0aca2ede |
| Headers | show |
| Series | platform/x86: touchscreen_dmi: Add swap-x-y quirk for Goodix touchscreen on Estar Beauty HD tablet | expand |
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 >
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, >
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 ;)
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
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.
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,
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(+)