[PATCH 1/2] platform/x86: touchscreen_dmi: Allow partial (prefix) matches for ACPI names

[PATCH 1/2] platform/x86: touchscreen_dmi: Allow partial (prefix) matches for ACPI names

[Hans de Goede]

On some devices the ACPI name of the touchscreen is e.g. either
MSSL1680:00 or MSSL1680:01 depending on the BIOS version.

This happens for example on the "Chuwi Hi8 Air" tablet where the initial
commit's ts_data uses "MSSL1680:00" but the tablets from the github issue
and linux-hardware.org probe linked below both use "MSSL1680:01".

Replace the strcmp() match on ts_data->acpi_name with a strstarts()
check to allow using a partial match on just the ACPI HID of "MSSL1680"
and change the ts_data->acpi_name for the "Chuwi Hi8 Air" accordingly
to fix the touchscreen not working on models where it is "MSSL1680:01".

Note this drops the length check for I2C_NAME_SIZE. This never was
necessary since the ACPI names used are never more then 11 chars and
I2C_NAME_SIZE is 20 so the replaced strncmp() would always stop long
before reaching I2C_NAME_SIZE.

Link: https://linux-hardware.org/?computer=AC4301C0542A
Closes: https://github.com/onitake/gsl-firmware/issues/91
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/touchscreen_dmi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/touchscreen_dmi.c b/drivers/platform/x86/touchscreen_dmi.c
index 7aee5e9ff2b8..969477c83e56 100644
--- a/drivers/platform/x86/touchscreen_dmi.c
+++ b/drivers/platform/x86/touchscreen_dmi.c
@@ -81,7 +81,7 @@ static const struct property_entry chuwi_hi8_air_props[] = {
 };
 
 static const struct ts_dmi_data chuwi_hi8_air_data = {
-	.acpi_name	= "MSSL1680:00",
+	.acpi_name	= "MSSL1680",
 	.properties	= chuwi_hi8_air_props,
 };
 
@@ -1821,7 +1821,7 @@ static void ts_dmi_add_props(struct i2c_client *client)
 	int error;
 
 	if (has_acpi_companion(dev) &&
-	    !strncmp(ts_data->acpi_name, client->name, I2C_NAME_SIZE)) {
+	    strstarts(client->name, ts_data->acpi_name)) {
 		error = device_create_managed_software_node(dev, ts_data->properties, NULL);
 		if (error)
 			dev_err(dev, "failed to add properties: %d\n", error);
-- 
2.43.0

[PATCH 2/2] platform/x86: touchscreen_dmi: Consolidate Goodix upside-down touchscreen data

[Hans de Goede]

Now that prefix matches for ACPI names are supported, the ts_dmi_data
structs for "GDIX1001:00" and "GDIX1001:01" can be consolidated into
a single match matching on "GDIX1001".

For consistency also change gdix1002_00_upside_down_data to match.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/touchscreen_dmi.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/drivers/platform/x86/touchscreen_dmi.c b/drivers/platform/x86/touchscreen_dmi.c
index 969477c83e56..975cf24ae359 100644
--- a/drivers/platform/x86/touchscreen_dmi.c
+++ b/drivers/platform/x86/touchscreen_dmi.c
@@ -415,18 +415,13 @@ static const struct property_entry gdix1001_upside_down_props[] = {
 	{ }
 };
 
-static const struct ts_dmi_data gdix1001_00_upside_down_data = {
-	.acpi_name	= "GDIX1001:00",
+static const struct ts_dmi_data gdix1001_upside_down_data = {
+	.acpi_name	= "GDIX1001",
 	.properties	= gdix1001_upside_down_props,
 };
 
-static const struct ts_dmi_data gdix1001_01_upside_down_data = {
-	.acpi_name	= "GDIX1001:01",
-	.properties	= gdix1001_upside_down_props,
-};
-
-static const struct ts_dmi_data gdix1002_00_upside_down_data = {
-	.acpi_name	= "GDIX1002:00",
+static const struct ts_dmi_data gdix1002_upside_down_data = {
+	.acpi_name	= "GDIX1002",
 	.properties	= gdix1001_upside_down_props,
 };
 
@@ -1412,7 +1407,7 @@ const struct dmi_system_id touchscreen_dmi_table[] = {
 	},
 	{
 		/* Juno Tablet */
-		.driver_data = (void *)&gdix1002_00_upside_down_data,
+		.driver_data = (void *)&gdix1002_upside_down_data,
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Default string"),
 			/* Both product- and board-name being "Default string" is somewhat rare */
@@ -1658,7 +1653,7 @@ const struct dmi_system_id touchscreen_dmi_table[] = {
 	},
 	{
 		/* Teclast X89 (Android version / BIOS) */
-		.driver_data = (void *)&gdix1001_00_upside_down_data,
+		.driver_data = (void *)&gdix1001_upside_down_data,
 		.matches = {
 			DMI_MATCH(DMI_BOARD_VENDOR, "WISKY"),
 			DMI_MATCH(DMI_BOARD_NAME, "3G062i"),
@@ -1666,7 +1661,7 @@ const struct dmi_system_id touchscreen_dmi_table[] = {
 	},
 	{
 		/* Teclast X89 (Windows version / BIOS) */
-		.driver_data = (void *)&gdix1001_01_upside_down_data,
+		.driver_data = (void *)&gdix1001_upside_down_data,
 		.matches = {
 			/* tPAD is too generic, also match on bios date */
 			DMI_MATCH(DMI_BOARD_VENDOR, "TECLAST"),
@@ -1684,7 +1679,7 @@ const struct dmi_system_id touchscreen_dmi_table[] = {
 	},
 	{
 		/* Teclast X98 Pro */
-		.driver_data = (void *)&gdix1001_00_upside_down_data,
+		.driver_data = (void *)&gdix1001_upside_down_data,
 		.matches = {
 			/*
 			 * Only match BIOS date, because the manufacturers
@@ -1788,7 +1783,7 @@ const struct dmi_system_id touchscreen_dmi_table[] = {
 	},
 	{
 		/* "WinBook TW100" */
-		.driver_data = (void *)&gdix1001_00_upside_down_data,
+		.driver_data = (void *)&gdix1001_upside_down_data,
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "WinBook"),
 			DMI_MATCH(DMI_PRODUCT_NAME, "TW100")
@@ -1796,7 +1791,7 @@ const struct dmi_system_id touchscreen_dmi_table[] = {
 	},
 	{
 		/* WinBook TW700 */
-		.driver_data = (void *)&gdix1001_00_upside_down_data,
+		.driver_data = (void *)&gdix1001_upside_down_data,
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "WinBook"),
 			DMI_MATCH(DMI_PRODUCT_NAME, "TW700")
-- 
2.43.0

Re: [PATCH 2/2] platform/x86: touchscreen_dmi: Consolidate Goodix upside-down touchscreen data

[Kuppuswamy Sathyanarayanan]

On 2/12/24 4:06 AM, Hans de Goede wrote:
> Now that prefix matches for ACPI names are supported, the ts_dmi_data
> structs for "GDIX1001:00" and "GDIX1001:01" can be consolidated into
> a single match matching on "GDIX1001".
>
> For consistency also change gdix1002_00_upside_down_data to match.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---

Looks good to me.

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

>  drivers/platform/x86/touchscreen_dmi.c | 25 ++++++++++---------------
>  1 file changed, 10 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/platform/x86/touchscreen_dmi.c b/drivers/platform/x86/touchscreen_dmi.c
> index 969477c83e56..975cf24ae359 100644
> --- a/drivers/platform/x86/touchscreen_dmi.c
> +++ b/drivers/platform/x86/touchscreen_dmi.c
> @@ -415,18 +415,13 @@ static const struct property_entry gdix1001_upside_down_props[] = {
>  	{ }
>  };
>  
> -static const struct ts_dmi_data gdix1001_00_upside_down_data = {
> -	.acpi_name	= "GDIX1001:00",
> +static const struct ts_dmi_data gdix1001_upside_down_data = {
> +	.acpi_name	= "GDIX1001",
>  	.properties	= gdix1001_upside_down_props,
>  };
>  
> -static const struct ts_dmi_data gdix1001_01_upside_down_data = {
> -	.acpi_name	= "GDIX1001:01",
> -	.properties	= gdix1001_upside_down_props,
> -};
> -
> -static const struct ts_dmi_data gdix1002_00_upside_down_data = {
> -	.acpi_name	= "GDIX1002:00",
> +static const struct ts_dmi_data gdix1002_upside_down_data = {
> +	.acpi_name	= "GDIX1002",
>  	.properties	= gdix1001_upside_down_props,
>  };
>  
> @@ -1412,7 +1407,7 @@ const struct dmi_system_id touchscreen_dmi_table[] = {
>  	},
>  	{
>  		/* Juno Tablet */
> -		.driver_data = (void *)&gdix1002_00_upside_down_data,
> +		.driver_data = (void *)&gdix1002_upside_down_data,
>  		.matches = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "Default string"),
>  			/* Both product- and board-name being "Default string" is somewhat rare */
> @@ -1658,7 +1653,7 @@ const struct dmi_system_id touchscreen_dmi_table[] = {
>  	},
>  	{
>  		/* Teclast X89 (Android version / BIOS) */
> -		.driver_data = (void *)&gdix1001_00_upside_down_data,
> +		.driver_data = (void *)&gdix1001_upside_down_data,
>  		.matches = {
>  			DMI_MATCH(DMI_BOARD_VENDOR, "WISKY"),
>  			DMI_MATCH(DMI_BOARD_NAME, "3G062i"),
> @@ -1666,7 +1661,7 @@ const struct dmi_system_id touchscreen_dmi_table[] = {
>  	},
>  	{
>  		/* Teclast X89 (Windows version / BIOS) */
> -		.driver_data = (void *)&gdix1001_01_upside_down_data,
> +		.driver_data = (void *)&gdix1001_upside_down_data,
>  		.matches = {
>  			/* tPAD is too generic, also match on bios date */
>  			DMI_MATCH(DMI_BOARD_VENDOR, "TECLAST"),
> @@ -1684,7 +1679,7 @@ const struct dmi_system_id touchscreen_dmi_table[] = {
>  	},
>  	{
>  		/* Teclast X98 Pro */
> -		.driver_data = (void *)&gdix1001_00_upside_down_data,
> +		.driver_data = (void *)&gdix1001_upside_down_data,
>  		.matches = {
>  			/*
>  			 * Only match BIOS date, because the manufacturers
> @@ -1788,7 +1783,7 @@ const struct dmi_system_id touchscreen_dmi_table[] = {
>  	},
>  	{
>  		/* "WinBook TW100" */
> -		.driver_data = (void *)&gdix1001_00_upside_down_data,
> +		.driver_data = (void *)&gdix1001_upside_down_data,
>  		.matches = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "WinBook"),
>  			DMI_MATCH(DMI_PRODUCT_NAME, "TW100")
> @@ -1796,7 +1791,7 @@ const struct dmi_system_id touchscreen_dmi_table[] = {
>  	},
>  	{
>  		/* WinBook TW700 */
> -		.driver_data = (void *)&gdix1001_00_upside_down_data,
> +		.driver_data = (void *)&gdix1001_upside_down_data,
>  		.matches = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "WinBook"),
>  			DMI_MATCH(DMI_PRODUCT_NAME, "TW700")

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH 1/2] platform/x86: touchscreen_dmi: Allow partial (prefix) matches for ACPI names

[Hans de Goede]

Hi,

On 2/12/24 13:06, Hans de Goede wrote:
> On some devices the ACPI name of the touchscreen is e.g. either
> MSSL1680:00 or MSSL1680:01 depending on the BIOS version.
> 
> This happens for example on the "Chuwi Hi8 Air" tablet where the initial
> commit's ts_data uses "MSSL1680:00" but the tablets from the github issue
> and linux-hardware.org probe linked below both use "MSSL1680:01".
> 
> Replace the strcmp() match on ts_data->acpi_name with a strstarts()
> check to allow using a partial match on just the ACPI HID of "MSSL1680"
> and change the ts_data->acpi_name for the "Chuwi Hi8 Air" accordingly
> to fix the touchscreen not working on models where it is "MSSL1680:01".
> 
> Note this drops the length check for I2C_NAME_SIZE. This never was
> necessary since the ACPI names used are never more then 11 chars and
> I2C_NAME_SIZE is 20 so the replaced strncmp() would always stop long
> before reaching I2C_NAME_SIZE.
> 
> Link: https://linux-hardware.org/?computer=AC4301C0542A
> Closes: https://github.com/onitake/gsl-firmware/issues/91
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Note I plan to merge this into fixes sometime this week or next week.

I'll also merge the follow-up patch into fixes since it is trivial
and that avoids the need to backmerge fixes into for-next.

Regards,

Hans




> ---
>  drivers/platform/x86/touchscreen_dmi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/touchscreen_dmi.c b/drivers/platform/x86/touchscreen_dmi.c
> index 7aee5e9ff2b8..969477c83e56 100644
> --- a/drivers/platform/x86/touchscreen_dmi.c
> +++ b/drivers/platform/x86/touchscreen_dmi.c
> @@ -81,7 +81,7 @@ static const struct property_entry chuwi_hi8_air_props[] = {
>  };
>  
>  static const struct ts_dmi_data chuwi_hi8_air_data = {
> -	.acpi_name	= "MSSL1680:00",
> +	.acpi_name	= "MSSL1680",
>  	.properties	= chuwi_hi8_air_props,
>  };
>  
> @@ -1821,7 +1821,7 @@ static void ts_dmi_add_props(struct i2c_client *client)
>  	int error;
>  
>  	if (has_acpi_companion(dev) &&
> -	    !strncmp(ts_data->acpi_name, client->name, I2C_NAME_SIZE)) {
> +	    strstarts(client->name, ts_data->acpi_name)) {
>  		error = device_create_managed_software_node(dev, ts_data->properties, NULL);
>  		if (error)
>  			dev_err(dev, "failed to add properties: %d\n", error);

Re: [PATCH 1/2] platform/x86: touchscreen_dmi: Allow partial (prefix) matches for ACPI names

[Andy Shevchenko]

On Mon, Feb 12, 2024 at 2:06 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> On some devices the ACPI name of the touchscreen is e.g. either
> MSSL1680:00 or MSSL1680:01 depending on the BIOS version.
>
> This happens for example on the "Chuwi Hi8 Air" tablet where the initial
> commit's ts_data uses "MSSL1680:00" but the tablets from the github issue
> and linux-hardware.org probe linked below both use "MSSL1680:01".
>
> Replace the strcmp() match on ts_data->acpi_name with a strstarts()
> check to allow using a partial match on just the ACPI HID of "MSSL1680"
> and change the ts_data->acpi_name for the "Chuwi Hi8 Air" accordingly
> to fix the touchscreen not working on models where it is "MSSL1680:01".

This is a good example to show why we shouldn't rely on device
instances and be stuck with HID. Ideally I would prefer to see the
acpi_dev_get_first_match_dev() / for_each_acpi_dev_match().

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] platform/x86: touchscreen_dmi: Allow partial (prefix) matches for ACPI names

[Kuppuswamy Sathyanarayanan]

On 2/12/24 4:06 AM, Hans de Goede wrote:
> On some devices the ACPI name of the touchscreen is e.g. either
> MSSL1680:00 or MSSL1680:01 depending on the BIOS version.
>
> This happens for example on the "Chuwi Hi8 Air" tablet where the initial
> commit's ts_data uses "MSSL1680:00" but the tablets from the github issue
> and linux-hardware.org probe linked below both use "MSSL1680:01".
>
> Replace the strcmp() match on ts_data->acpi_name with a strstarts()
> check to allow using a partial match on just the ACPI HID of "MSSL1680"
> and change the ts_data->acpi_name for the "Chuwi Hi8 Air" accordingly
> to fix the touchscreen not working on models where it is "MSSL1680:01".
>
> Note this drops the length check for I2C_NAME_SIZE. This never was
> necessary since the ACPI names used are never more then 11 chars and
> I2C_NAME_SIZE is 20 so the replaced strncmp() would always stop long
> before reaching I2C_NAME_SIZE.
>
> Link: https://linux-hardware.org/?computer=AC4301C0542A
> Closes: https://github.com/onitake/gsl-firmware/issues/91
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---

Since this is a bug fix, don't you want to add stable tag?

Otherwise, it looks good.

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

>  drivers/platform/x86/touchscreen_dmi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/touchscreen_dmi.c b/drivers/platform/x86/touchscreen_dmi.c
> index 7aee5e9ff2b8..969477c83e56 100644
> --- a/drivers/platform/x86/touchscreen_dmi.c
> +++ b/drivers/platform/x86/touchscreen_dmi.c
> @@ -81,7 +81,7 @@ static const struct property_entry chuwi_hi8_air_props[] = {
>  };
>  
>  static const struct ts_dmi_data chuwi_hi8_air_data = {
> -	.acpi_name	= "MSSL1680:00",
> +	.acpi_name	= "MSSL1680",
>  	.properties	= chuwi_hi8_air_props,
>  };
>  
> @@ -1821,7 +1821,7 @@ static void ts_dmi_add_props(struct i2c_client *client)
>  	int error;
>  
>  	if (has_acpi_companion(dev) &&
> -	    !strncmp(ts_data->acpi_name, client->name, I2C_NAME_SIZE)) {
> +	    strstarts(client->name, ts_data->acpi_name)) {
>  		error = device_create_managed_software_node(dev, ts_data->properties, NULL);
>  		if (error)
>  			dev_err(dev, "failed to add properties: %d\n", error);

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH 1/2] platform/x86: touchscreen_dmi: Allow partial (prefix) matches for ACPI names

[Hans de Goede]

Hi,

On 2/13/24 02:01, Kuppuswamy Sathyanarayanan wrote:
> 
> On 2/12/24 4:06 AM, Hans de Goede wrote:
>> On some devices the ACPI name of the touchscreen is e.g. either
>> MSSL1680:00 or MSSL1680:01 depending on the BIOS version.
>>
>> This happens for example on the "Chuwi Hi8 Air" tablet where the initial
>> commit's ts_data uses "MSSL1680:00" but the tablets from the github issue
>> and linux-hardware.org probe linked below both use "MSSL1680:01".
>>
>> Replace the strcmp() match on ts_data->acpi_name with a strstarts()
>> check to allow using a partial match on just the ACPI HID of "MSSL1680"
>> and change the ts_data->acpi_name for the "Chuwi Hi8 Air" accordingly
>> to fix the touchscreen not working on models where it is "MSSL1680:01".
>>
>> Note this drops the length check for I2C_NAME_SIZE. This never was
>> necessary since the ACPI names used are never more then 11 chars and
>> I2C_NAME_SIZE is 20 so the replaced strncmp() would always stop long
>> before reaching I2C_NAME_SIZE.
>>
>> Link: https://linux-hardware.org/?computer=AC4301C0542A
>> Closes: https://github.com/onitake/gsl-firmware/issues/91
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
> 
> Since this is a bug fix, don't you want to add stable tag?

Good point, done.

I've added these to my review-hans (soon to be fixes) branch now.

Regards,

Hans

> 
> Otherwise, it looks good.
> 
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> 
>>  drivers/platform/x86/touchscreen_dmi.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/platform/x86/touchscreen_dmi.c b/drivers/platform/x86/touchscreen_dmi.c
>> index 7aee5e9ff2b8..969477c83e56 100644
>> --- a/drivers/platform/x86/touchscreen_dmi.c
>> +++ b/drivers/platform/x86/touchscreen_dmi.c
>> @@ -81,7 +81,7 @@ static const struct property_entry chuwi_hi8_air_props[] = {
>>  };
>>  
>>  static const struct ts_dmi_data chuwi_hi8_air_data = {
>> -	.acpi_name	= "MSSL1680:00",
>> +	.acpi_name	= "MSSL1680",
>>  	.properties	= chuwi_hi8_air_props,
>>  };
>>  
>> @@ -1821,7 +1821,7 @@ static void ts_dmi_add_props(struct i2c_client *client)
>>  	int error;
>>  
>>  	if (has_acpi_companion(dev) &&
>> -	    !strncmp(ts_data->acpi_name, client->name, I2C_NAME_SIZE)) {
>> +	    strstarts(client->name, ts_data->acpi_name)) {
>>  		error = device_create_managed_software_node(dev, ts_data->properties, NULL);
>>  		if (error)
>>  			dev_err(dev, "failed to add properties: %d\n", error);
>