[PATCH] i2c: acpi: Force bus speed to 400KHz if a Silead touchscreen is present

[PATCH] i2c: acpi: Force bus speed to 400KHz if a Silead touchscreen is present

[Hans de Goede]

Many cheap devices use Silead touchscreen controllers. Testing has shown
repeatedly that these touchscreen controllers work fine at 400KHz, but for
unknown reasons do not work properly at 100KHz. This has been seen on
both ARM and x86 devices using totally different i2c controllers.

On some devices the ACPI tables list another device at the same I2C-bus
as only being capable of 100KHz, testing has shown that these other
devices work fine at 400KHz (as can be expected of any recent I2C hw).

This commit makes i2c_acpi_find_bus_speed() always return 400KHz if a
Silead touchscreen controller is present, fixing the touchscreen not
working on devices which ACPI tables' wrongly list another device on the
same bus as only being capable of 100KHz.

Specifically this fixes the touchscreen on the Jumper EZpad 6 m4 not
working.

Reported-and-tested-by: youling 257 <youling257@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/i2c/i2c-core-acpi.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index 9cb2aa1e20ef..420c356eba06 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -39,6 +39,7 @@ struct i2c_acpi_lookup {
 	int index;
 	u32 speed;
 	u32 min_speed;
+	u32 force_speed;
 };
 
 /**
@@ -285,6 +286,19 @@ i2c_acpi_match_device(const struct acpi_device_id *matches,
 	return acpi_match_device(matches, &client->dev);
 }
 
+static const struct acpi_device_id i2c_acpi_force_400khz_device_ids[] = {
+	/*
+	 * These Silead touchscreen controllers only work at 400KHz, for
+	 * some reason they do not work at 100KHz. On some devices the ACPI
+	 * tables list another device at their bus as only being capable
+	 * of 100KHz, testing has shown that these other devices work fine
+	 * at 400KHz (as can be expected of any recent i2c hw) so we force
+	 * the speed of the bus to 400 KHz if a Silead device is present.
+	 */
+	{ "MSSL1680", 0 },
+	{}
+};
+
 static acpi_status i2c_acpi_lookup_speed(acpi_handle handle, u32 level,
 					   void *data, void **return_value)
 {
@@ -303,6 +317,9 @@ static acpi_status i2c_acpi_lookup_speed(acpi_handle handle, u32 level,
 	if (lookup->speed <= lookup->min_speed)
 		lookup->min_speed = lookup->speed;
 
+	if (acpi_match_device_ids(adev, i2c_acpi_force_400khz_device_ids) == 0)
+		lookup->force_speed = 400000;
+
 	return AE_OK;
 }
 
@@ -340,7 +357,12 @@ u32 i2c_acpi_find_bus_speed(struct device *dev)
 		return 0;
 	}
 
-	return lookup.min_speed != UINT_MAX ? lookup.min_speed : 0;
+	if (lookup.force_speed)
+		return lookup.force_speed;
+	else if (lookup.min_speed != UINT_MAX)
+		return lookup.min_speed;
+	else
+		return 0;
 }
 EXPORT_SYMBOL_GPL(i2c_acpi_find_bus_speed);
 
-- 
2.23.0

Re: [PATCH] i2c: acpi: Force bus speed to 400KHz if a Silead touchscreen is present

[Mika Westerberg]

+Jarkko

On Mon, Nov 11, 2019 at 11:48:27AM +0100, Hans de Goede wrote:
> Many cheap devices use Silead touchscreen controllers. Testing has shown
> repeatedly that these touchscreen controllers work fine at 400KHz, but for
> unknown reasons do not work properly at 100KHz. This has been seen on
> both ARM and x86 devices using totally different i2c controllers.
> 
> On some devices the ACPI tables list another device at the same I2C-bus
> as only being capable of 100KHz, testing has shown that these other
> devices work fine at 400KHz (as can be expected of any recent I2C hw).
> 
> This commit makes i2c_acpi_find_bus_speed() always return 400KHz if a
> Silead touchscreen controller is present, fixing the touchscreen not
> working on devices which ACPI tables' wrongly list another device on the
> same bus as only being capable of 100KHz.
> 
> Specifically this fixes the touchscreen on the Jumper EZpad 6 m4 not
> working.
> 
> Reported-and-tested-by: youling 257 <youling257@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/i2c/i2c-core-acpi.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
> index 9cb2aa1e20ef..420c356eba06 100644
> --- a/drivers/i2c/i2c-core-acpi.c
> +++ b/drivers/i2c/i2c-core-acpi.c
> @@ -39,6 +39,7 @@ struct i2c_acpi_lookup {
>  	int index;
>  	u32 speed;
>  	u32 min_speed;
> +	u32 force_speed;
>  };
>  
>  /**
> @@ -285,6 +286,19 @@ i2c_acpi_match_device(const struct acpi_device_id *matches,
>  	return acpi_match_device(matches, &client->dev);
>  }
>  
> +static const struct acpi_device_id i2c_acpi_force_400khz_device_ids[] = {
> +	/*
> +	 * These Silead touchscreen controllers only work at 400KHz, for
> +	 * some reason they do not work at 100KHz. On some devices the ACPI
> +	 * tables list another device at their bus as only being capable
> +	 * of 100KHz, testing has shown that these other devices work fine
> +	 * at 400KHz (as can be expected of any recent i2c hw) so we force
> +	 * the speed of the bus to 400 KHz if a Silead device is present.
> +	 */
> +	{ "MSSL1680", 0 },
> +	{}
> +};
> +
>  static acpi_status i2c_acpi_lookup_speed(acpi_handle handle, u32 level,
>  					   void *data, void **return_value)
>  {
> @@ -303,6 +317,9 @@ static acpi_status i2c_acpi_lookup_speed(acpi_handle handle, u32 level,
>  	if (lookup->speed <= lookup->min_speed)
>  		lookup->min_speed = lookup->speed;
>  
> +	if (acpi_match_device_ids(adev, i2c_acpi_force_400khz_device_ids) == 0)
> +		lookup->force_speed = 400000;

I wonder if it makes sense to log a warning about this? So when
debugging it can be clearly seen from the logs that the device is
quirked.

> +
>  	return AE_OK;
>  }
>  
> @@ -340,7 +357,12 @@ u32 i2c_acpi_find_bus_speed(struct device *dev)
>  		return 0;
>  	}
>  
> -	return lookup.min_speed != UINT_MAX ? lookup.min_speed : 0;
> +	if (lookup.force_speed)
> +		return lookup.force_speed;
> +	else if (lookup.min_speed != UINT_MAX)
> +		return lookup.min_speed;
> +	else
> +		return 0;
>  }
>  EXPORT_SYMBOL_GPL(i2c_acpi_find_bus_speed);
>  
> -- 
> 2.23.0

Re: [PATCH] i2c: acpi: Force bus speed to 400KHz if a Silead touchscreen is present

[Hans de Goede]

Hi,

Thank you for the quick reply.

On 11-11-2019 12:06, Mika Westerberg wrote:
> +Jarkko
> 
> On Mon, Nov 11, 2019 at 11:48:27AM +0100, Hans de Goede wrote:
>> Many cheap devices use Silead touchscreen controllers. Testing has shown
>> repeatedly that these touchscreen controllers work fine at 400KHz, but for
>> unknown reasons do not work properly at 100KHz. This has been seen on
>> both ARM and x86 devices using totally different i2c controllers.
>>
>> On some devices the ACPI tables list another device at the same I2C-bus
>> as only being capable of 100KHz, testing has shown that these other
>> devices work fine at 400KHz (as can be expected of any recent I2C hw).
>>
>> This commit makes i2c_acpi_find_bus_speed() always return 400KHz if a
>> Silead touchscreen controller is present, fixing the touchscreen not
>> working on devices which ACPI tables' wrongly list another device on the
>> same bus as only being capable of 100KHz.
>>
>> Specifically this fixes the touchscreen on the Jumper EZpad 6 m4 not
>> working.
>>
>> Reported-and-tested-by: youling 257 <youling257@gmail.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/i2c/i2c-core-acpi.c | 24 +++++++++++++++++++++++-
>>   1 file changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
>> index 9cb2aa1e20ef..420c356eba06 100644
>> --- a/drivers/i2c/i2c-core-acpi.c
>> +++ b/drivers/i2c/i2c-core-acpi.c
>> @@ -39,6 +39,7 @@ struct i2c_acpi_lookup {
>>   	int index;
>>   	u32 speed;
>>   	u32 min_speed;
>> +	u32 force_speed;
>>   };
>>   
>>   /**
>> @@ -285,6 +286,19 @@ i2c_acpi_match_device(const struct acpi_device_id *matches,
>>   	return acpi_match_device(matches, &client->dev);
>>   }
>>   
>> +static const struct acpi_device_id i2c_acpi_force_400khz_device_ids[] = {
>> +	/*
>> +	 * These Silead touchscreen controllers only work at 400KHz, for
>> +	 * some reason they do not work at 100KHz. On some devices the ACPI
>> +	 * tables list another device at their bus as only being capable
>> +	 * of 100KHz, testing has shown that these other devices work fine
>> +	 * at 400KHz (as can be expected of any recent i2c hw) so we force
>> +	 * the speed of the bus to 400 KHz if a Silead device is present.
>> +	 */
>> +	{ "MSSL1680", 0 },
>> +	{}
>> +};
>> +
>>   static acpi_status i2c_acpi_lookup_speed(acpi_handle handle, u32 level,
>>   					   void *data, void **return_value)
>>   {
>> @@ -303,6 +317,9 @@ static acpi_status i2c_acpi_lookup_speed(acpi_handle handle, u32 level,
>>   	if (lookup->speed <= lookup->min_speed)
>>   		lookup->min_speed = lookup->speed;
>>   
>> +	if (acpi_match_device_ids(adev, i2c_acpi_force_400khz_device_ids) == 0)
>> +		lookup->force_speed = 400000;
> 
> I wonder if it makes sense to log a warning about this? So when
> debugging it can be clearly seen from the logs that the device is
> quirked.

Yes and no, yes a warning is a good idea, but not here, since there are a
ton of devices with a Silead touchscreen and only few have this problem.

> 
>> +
>>   	return AE_OK;
>>   }
>>   
>> @@ -340,7 +357,12 @@ u32 i2c_acpi_find_bus_speed(struct device *dev)
>>   		return 0;
>>   	}
>>   
>> -	return lookup.min_speed != UINT_MAX ? lookup.min_speed : 0;
>> +	if (lookup.force_speed) {

So the warning should go here then, like this:

	if (lookup.force_speed > lookup.min_speed)
		pr_warn(FW_BUG "Some decent text Hans has to come-up with");


I'll wait a bit for other remarks before posting a v2 with this added.

Regards,

Hans


>> +		return lookup.force_speed;
>> +	} else if (lookup.min_speed != UINT_MAX)
>> +		return lookup.min_speed;
>> +	else
>> +		return 0;
>>   }
>>   EXPORT_SYMBOL_GPL(i2c_acpi_find_bus_speed);
>>   
>> -- 
>> 2.23.0
>