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@xxxxxxxxxxxxxxxxxxxx/ > > 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 >
