Re: [PATCH 0/6] HID: i2c-hid-of: Allow using i2c-hid-of on non OF platforms + remove specialized drivers

[Benjamin Tissoires <benjamin.tissoires@redhat.com>]

Hi Hans,

On Apr 09 2023, Hans de Goede wrote:
> Hi All,
> 
> This series consist of 2 parts:
> 
> 1. Patches 1-3. Allow using i2c-hid-of on non OF platforms to allow I2C-HID
>    devices which are not enumerated by ACPI to work on ACPI platforms
>    (by manual i2c_client instantiation using i2c_client_id matching).

Patches 1 and 2 are looking good, but I wonder if you can not achieve the
same result by relying on an ACPI SSDT override. I got something similar
working on this thread[0].

I understand the "post-reset-deassert-delay-ms" might be something hard
to express with an SSDT, but we should already have all the bits in
place, no?

Also, the problem of "post-reset-deassert-delay-ms" is that you are not
documenting it, because the OF folks do not want this in device tree,
and I tend to agree with them. So this basically creates a brand new
undocumented property, which is less than ideal.

> 
> 2. Patches 4-6. Remove the special i2c-hid-of-elan and i2c-hid-of-goodix
>    driver, folding the functionality into the generic i2c-hid-of driver.
>    Since 1. requires adding reset-gpio support to i2c-hid-of there was
>    very little difference left between the generic i2c-hid-of code and
>    the specialized drivers. So I decided to merge them into the generic
>    driver instead of having duplicate code.

I understand the spirit, but I am not a big fan of this. The reason is
just detailed your following statements: getting tests on those is hard.

So there is code duplication, yes, but OTOH this guarantees that we do
not break those devices while working on something else.

I can always be convinced otherwise, but I still think the approach of
the devicetree-bindings maintainers works better: if you need a new
property that isn't available in the core of i2c-hid-of, and which is
device specific (even if it's just a msleep for a line to be ready),
make this a separate driver. Trying to parametrize everything with
properties will just end up in a situation where one "meaningless"
property will break another device, and it's going to be a pain to
trace, because those drivers are not tested every single kernel release.

> 
> Note patches 4-6 have not been actually tested with an "elan,ekth6915"
> touchscreen nor with a "goodix,gt7375p" touchscreen.
> 
> Douglas, can you perhaps test this patch-set with an "elan,ekth6915"
> touchscreen and with a "goodix,gt7375p" touchscreen ?
> 
> Regards,
> 
> Hans
> 

Cheers,
Benjamin


[0] https://lore.kernel.org/linux-input/20230308155527.jnrsowubvnk22ica@mail.corp.redhat.com/

> 
> Hans de Goede (6):
>   HID: i2c-hid-of: Consistenly use dev local variable in probe()
>   HID: i2c-hid-of: Allow using i2c-hid-of on non OF platforms
>   HID: i2c-hid-of: Add reset GPIO support to i2c-hid-of
>   HID: i2c-hid-of: Add chip_data struct
>   HID: i2c-hid-of: Consolidate Elan support into generic i2c-hid-of
>     driver
>   HID: i2c-hid-of: Consolidate Goodix support into generic i2c-hid-of
>     driver
> 
>  drivers/hid/i2c-hid/Kconfig             |  36 +------
>  drivers/hid/i2c-hid/Makefile            |   2 -
>  drivers/hid/i2c-hid/i2c-hid-of-elan.c   | 129 ------------------------
>  drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 125 -----------------------
>  drivers/hid/i2c-hid/i2c-hid-of.c        | 124 +++++++++++++++++++----
>  5 files changed, 106 insertions(+), 310 deletions(-)
>  delete mode 100644 drivers/hid/i2c-hid/i2c-hid-of-elan.c
>  delete mode 100644 drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> 
> -- 
> 2.39.1
>