Hi Benjamin, On 4/11/23 11:02, Benjamin Tissoires wrote: > 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]. Yes this could be made to work with an ACPI override. But the goal is to make things work OOTB for end users when they install Linux and ACPI overrides are very far from something which works OOTB. > 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? Actually if an ACPI override is used then the setting of the GPIO can be done in _PS0 and _PS3 (power on / off) methods and those can simply include a sleep after setting the GPIO. > 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. I'm merely not documenting it because there are no devicetree users yet. Between the 2 currently supported of devices with a reset GPIO + the I2C-HID capable touchscreen + wacom digitizer on the x86 android Yoga Book 1 I'm trying to get to work that is 4 I2C-HID devices which all follow the pattern of: 1. They have a reset GPIO, 2. they need some delay after reset is deasserted. It seems silly to keep adding more and more device-ids + match-data with just the delays in there when it seems that many many I2C-HID capable controllers/chips follow this pattern. Also note that there already is a very similar "post-power-on-delay-ms" property. I really don't see what makes specifying a delay after enabling regulators through a property ok, but specifying the delay after reset-deassert not ok. Allowing one but not the other is not very consistent. The reason why I'm not documenting the property now is because of lack of current devicetree users. It can be documented once the first DT users show up and getting it accepted should really not be an issue given that "post-power-on-delay-ms" already exists. Note if just the existence of the property is the main stumbling block I can go the match_data route for the wacom digitizer on the Yoga Book 1 too and add an extra i2c_device_id with match-data setting the delay. This could then either be its own specialized driver, or we could still go with the current patch-set (minus the property) and add an i2c_device_id with match-data to i2c-hid-of.c . The only question then is how to name the i2c_device_id for the wacom digitizer. It has a vid:pid of 056A:0169 So maybe "wacom0169" ? >> 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. Actually AFAIK the chromeos folks have an automated test lab where all supported models get tested and they regularly test the latest mainline kernels. So even without me asking for it any regressions here should have been caught in this case since support for both special-case i2c-hid-of drivers was added for chromeos. And the code is almost identical, the only difference is using the bulk-regulator API vs enabling the regulators 1 by 1, which should not make any difference. > 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. This is not trying to parametrize everything, this is trying to parametrize something which turns out to be necessary over 4 different chips/controller models. And I'm pretty sure that if I start looking into ACPI tables I will find many more controllers which use a reset GPIO + a delay after de-assert like this. IOW something which is clearly a very common pattern. You have been advocating to make HID code more generic allowing device-quirks in BPF format to avoid adding drivers for every tiny descriptor fixup. Do you really want to go the route of a tiny driver for every i2c-hid chip variant used with devicetree, rather then having a single extra property ? Note that if patches 1-3 had been in place when Douglas started adding support for the "elan,ekth6915" and "goodix,gt7375p" devices that the devicetree on the chromeos devices using those would then likely have simply used the "hid-descr-addr", "post-power-on-delay-ms" and "post-reset-deassert-delay-ms" properties and no separate drivers would have been necessary at all. (We need patches 4-6 now only to keep compatibility with existing devicetree files which don't set these) So I really see patches 4-6 as a way to reduce future work reviewing specialized drivers for you and Jiri. Yes there may still be some special cases in the future which need a specialized driver which we have now, but I believe that covering the common reset-GPIO pattern will drastically reduce the need for those drivers and thus will lower the maintainer burden. Regards, Hans
