| Message ID | 20191113182938.279299-1-hdegoede@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] i2c: acpi: Force bus speed to 400KHz if a Silead touchscreen is present | expand |
Hi On 11/13/19 8:29 PM, 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> > --- > Changes in v2: > -Print a warning when we are forcing the bus to another speed then the > lowest speed of all devices the DSTD lists on the bus > --- > drivers/i2c/i2c-core-acpi.c | 28 +++++++++++++++++++++++++++- > 1 file changed, 27 insertions(+), 1 deletion(-) > I think this is the only sane way to go forward, i.e. kernel figures it out not the user needing to have a custom DSDT. Of course there is a small risk some device on the same bus cease working but benefit of this patch is worth of it. Otherwise these touchscreen devices keep not working and getting possible regression increases also knowledge. Reviewed-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
On Wed, Nov 13, 2019 at 07:29:38PM +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> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
On Wed, Nov 13, 2019 at 07:29:38PM +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> Stable material, I'd say? > + dev_warn(dev, FW_BUG "DSDT wrongly sets I2C bus speed to %d, forcing it to %d\n", > + lookup.min_speed, lookup.force_speed); I have not a strong opinion here. However, does the DSDT really wrongly set a bus speed when it is the touchscreen controller which cannot handle lower speeds and other devies are specified to run at 100kHz?
Hi, On 14-11-2019 21:48, Wolfram Sang wrote: > On Wed, Nov 13, 2019 at 07:29:38PM +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> > > Stable material, I'd say? Yes adding this to stable would be good, thanks. > >> + dev_warn(dev, FW_BUG "DSDT wrongly sets I2C bus speed to %d, forcing it to %d\n", >> + lookup.min_speed, lookup.force_speed); > > I have not a strong opinion here. However, does the DSDT really wrongly > set a bus speed when it is the touchscreen controller which cannot > handle lower speeds and other devies are specified to run at 100kHz? Well it is configuring the bus at a speed where not all devices can work, where as there is another speed where all devices do work. With that said I'm open to a different wording for the warning. Feel free to modify this before you add it to your tree. I do wonder what Windows does here, changing the bus speed when going from one device to another makes no sense since the non addressed device still needs to be able to handle the address part of the transaction, so the whole ACPI model where a speed is assigned to an i2c_client rather then to the bus is weird here, anyways... Regards, Hans
> Well it is configuring the bus at a speed where not all devices > can work, where as there is another speed where all devices do work. > With that said I'm open to a different wording for the warning. Feel > free to modify this before you add it to your tree. Changed the message to "DSDT uses known not-working I2C bus speed %d, forcing it to %d\n" and applied to for-current, thanks!
Hi, On 15-11-2019 21:56, Wolfram Sang wrote: > >> Well it is configuring the bus at a speed where not all devices >> can work, where as there is another speed where all devices do work. >> With that said I'm open to a different wording for the warning. Feel >> free to modify this before you add it to your tree. > > Changed the message to "DSDT uses known not-working I2C bus speed %d, > forcing it to %d\n" and applied to for-current, thanks! The new message sounds good to me, thanks. Regards, Hans
diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c index 9cb2aa1e20ef..af9f3394a4a6 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,16 @@ 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) { + if (lookup.force_speed != lookup.min_speed) + dev_warn(dev, FW_BUG "DSDT wrongly sets I2C bus speed to %d, forcing it to %d\n", + lookup.min_speed, 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);
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> --- Changes in v2: -Print a warning when we are forcing the bus to another speed then the lowest speed of all devices the DSTD lists on the bus --- drivers/i2c/i2c-core-acpi.c | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-)