Re: Wrongly bound Elantech touchpad on Lenovo Yoga Slim 7

[Jiri Slaby <jirislaby@kernel.org>]

Use hansg@kernel.org, not @redhat.com

On 08. 09. 26, 10:27, Jiri Slaby wrote:
> Hi,
> 
> I am resuming an ooold thread...
> 
> On 05. 02. 22, 12:12, Hans de Goede wrote:
>> Hi,
>>
>> On 2/4/22 18:39, Benjamin Tissoires wrote:
>>> Hi,
>>>
>>> [adding Dmitry, the maintainer of the input tree and Hans, a 
>>> colleague of mine]
>>>
>>> On Fri, Feb 4, 2022 at 5:57 PM Takashi Iwai <tiwai@suse.de> wrote:
>>>>
>>>> Hi,
>>>>
>>>> we've got a bug report on openSUSE Bugzilla about the broken touchpad
>>>> on Lenovo Yoga Slim 7:
>>>>    https://bugzilla.opensuse.org/show_bug.cgi?id=1193064
>>>>
>>>> The touchpad is an Elantech one, connected over i2c, and there are two
>>>> drivers supporting it.  Unfortunately, the default one the system
>>>> binds, elan-i2c input driver, doesn't seem working properly, while
>>>> i2c-hid driver works.
>>>
>>> Hans, we do have a similar bug on RHEL at
>>> https://bugzilla.redhat.com/show_bug.cgi?id=2029078 (sorry, private
>>> bug).
>>>
>>> IIRC you worked on the discrimination between i2c-hid and elan_i2c (I
>>> might be completely wrong though).
>>
>> Yes I did work on that, but then the other way around making sure
>> that the i2c-hid driver would not bind to some devices which need
>> the elan_i2c touch*pad* driver.
>>
>> And indeed as Dmitry points out:
>>
>>> I believe we need to do what Hans did for Elan Touch*screen* driver and
>>> avoid binding to the device if it has i2c-hid-specific _DMS in ACPI.
>>> I.e. we need to replicate elants_acpi_is_hid_device().
>>>
>>> Even better would be to factor it out, maybe not into a shared module
>>> but simply shared header with static inline function that we could share
>>> between elan drivers and maybe others as well.
>>
>> I did fix a similar problem for the touchscreen driver last year or so.
>>
>> I agree with Dmitry that we should try to avoid DMI matching here;
>> and I also agree that having some header with a static inline
>> acpi_is_hid_device() device helper would be good.
>>
>> I'm a bit worried about the acpi_is_hid_device() approach though,
>> there is a lot of copy and pasting going on when vendors create
>> ACPI tables and sometimes a "PNP0C50" CID is present combined
>> with a valid i2c-hid _DSM method even though the device is not
>> an i2c-hid device, also see the i2c_hid_acpi_blacklist[] in
>> drivers/hid/i2c-hid/i2c-hid-acpi.c .
>>
>> It seems to me that the problem is that the Lenovo Yoga Slim 7
>> is using what seems to be a very generic "ELAN0000" ACPI hardware
>> id instead of one of the many more specific ones.
>>
>> So we could limit the acpi_is_hid_device() check to just the
>> "ELAN0000" ACPI hardware id I guess?
>>
>> So I see the following 2 options:
>>
>> 1. Add an unconditional acpi_is_hid_device() check to elan_probe()
>>     and watch out for any bug-reports that this is causing breakage
>>     elsehwere
>> 2. Add an acpi_is_hid_device() check to elan_probe() for ACPI enumerated
>>     touchpads with a hardware-id of ELAN0000 only; and still
>>     watch out for any bug-reports that this is causing breakage
>>     elsehwere just to be sure
> 
> I assume noone ever tried to implement this, right? Or was some 
> alternative approach merged in the meantime?
> 
> openSUSE still drags the Takashi's downstream patch with a DMI check. It 
> would be nice to sort this out upstream and drop that private one ;).
> 
> Thanks.
> 
>>>> I'm not sure what's the best fix for this, but below a quick
>>>> workaround using a deny list with DMI matching.
>>>> If this is OK, I can resubmit the patch for merging.
>>>>
>>>> Any comments appreciated.
>>>>
>>>>
>>>> thanks,
>>>>
>>>> Takashi
>>>>
>>>> -- 8< --
>>>> From: Takashi Iwai <tiwai@suse.de>
>>>> Subject: [PATCH] Input: elan_i2c: Add deny list for Lenovo Yoga Slim 7
>>>>
>>>> The touchpad on Lenovo Yoga Slim 7 doesn't work well with elan-i2c but
>>>> rather better with i2c-hid.  Add a deny list for avoiding to bind with
>>>> elan-i2c.
>>>>
>>>> BugLink: https://bugzilla.opensuse.org/show_bug.cgi?id=1193064
>>>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
>>>> ---
>>>>   drivers/input/mouse/elan_i2c_core.c | 19 +++++++++++++++++++
>>>>   1 file changed, 19 insertions(+)
>>>>
>>>> diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/ 
>>>> mouse/elan_i2c_core.c
>>>> index 47af62c12267..fd08481f7aea 100644
>>>> --- a/drivers/input/mouse/elan_i2c_core.c
>>>> +++ b/drivers/input/mouse/elan_i2c_core.c
>>>> @@ -18,6 +18,7 @@
>>>>   #include <linux/acpi.h>
>>>>   #include <linux/delay.h>
>>>>   #include <linux/device.h>
>>>> +#include <linux/dmi.h>
>>>>   #include <linux/firmware.h>
>>>>   #include <linux/i2c.h>
>>>>   #include <linux/init.h>
>>>> @@ -1222,6 +1223,20 @@ static void elan_disable_regulator(void *_data)
>>>>          regulator_disable(data->vcc);
>>>>   }
>>>>
>>>> +static const struct dmi_system_id elan_i2c_denylist[] __initconst = {
>>>> +#if IS_ENABLED(CONFIG_I2C_HID_ACPI)
>>>> +       {
>>>> +               /* Lenovo Yoga Slim 7 is better supported by i2c-hid */
>>>> +               .matches = {
>>>> +                       DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
>>>> +                       DMI_MATCH(DMI_PRODUCT_NAME, "82A3"),
>>>> +                       DMI_MATCH(DMI_PRODUCT_VERSION, "Yoga Slim 7 
>>>> 14ITL05"),
>>>> +               },
>>>> +       },
>>>> +#endif
>>>> +       { }
>>>> +};
>>>> +
>>>>   static int elan_probe(struct i2c_client *client,
>>>>                        const struct i2c_device_id *dev_id)
>>>>   {
>>>> @@ -1233,6 +1248,10 @@ static int elan_probe(struct i2c_client *client,
>>>>
>>>>          if (IS_ENABLED(CONFIG_MOUSE_ELAN_I2C_I2C) &&
>>>>              i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
>>>> +               if (dmi_check_system(elan_i2c_denylist)) {
>>>> +                       dev_info(dev, "Hits deny list, skipping\n");
>>>> +                       return -ENODEV;
>>>> +               }
>>>>                  transport_ops = &elan_i2c_ops;
>>>>          } else if (IS_ENABLED(CONFIG_MOUSE_ELAN_I2C_SMBUS) &&
>>>>                     i2c_check_functionality(client->adapter,
>>>> -- 
>>>> 2.31.1
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>
>>
> 

-- 
js
suse labs