| Message ID | 20230409144243.25360-1-hdegoede@redhat.com |
|---|---|
| Headers | show |
| Series | HID: i2c-hid-of: Allow using i2c-hid-of on non OF platforms + remove specialized drivers | expand |
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 >
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
On Apr 11 2023, Hans de Goede wrote: > 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. Fair enough. > > > 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. Though this is all conditional if we can make ACPI SSDT override something that can be shipped while installing the device... > > > 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. AFAIU, the non devicetree properties should also be documented through DT bindings, no? So not documenting feels very much like "I want to slip this one in without having to deal with DT maintainers" (and before you take it personaly, I know this is definitively not the intent). So I'd rather much having a public API documented, even if there are no users. > > 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. The problem, AFAICT, is that I2C-HID is only described through ACPI, and a lot of the HW specific is let to ACPI. So i2c-hid-of, is basically OEM bringing components together on a DT platform, and hoping for the best. This works well in the way that we don't need to add a new driver for it, but they can not easily describe what they need (or even fail like your tablet which is supposed to be working thorugh ACPI). So if we had one big competitor, like Google for Chromebooks, who just said: "this is what you need for OF i2c-hid devices, and you can not rely on anything else", life would be much simpler. > > 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. Agree :/ > > 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. Honestly this is not a good way of doing thing IMO. This is basically what I did with i2c-hid, and it pissed the DT maintainers. (I did it because at the time the maintainers were not reactive, and/or the ML were not correctly set IIRC). > > 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 . I'd much rather have a i2c-hid-of.c internal API, yes. Whether it's a function call, a callback or a match-data (or a driver-data), this is something we are in control and we can change. A blind undocumented property is going to be a pain if we get to change it. (unless of course you can get Rob's ack on the preperty itself). > > 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" ? Seems reasonable to me :) [few minutes later] Well, maybe we don't want the PID to be used here, because we will end up having to quirk every single device. But OTOH I do not see a different solution... > > > >> 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. Well, it's nice to know regressions, but it's nicer to know them before we introduce them in linux-next :) My point is if you don't manage to get tests on those devices, and we can "guarantee" that the changes in i2c-hid-of.c will be a noop for them, why bother merging them together? If the files have dedicated maintainers, we should probably rely on them instead :) > > > 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. Hehe, fair enough :) But my problem here is more who is responsible for this code, and merging them together means more responsibility to the i2c-hid-of.c maintainer. Having separate self-contained drivers for handling device subtleties (when they are not generic) allows to not break one device when fixing one other. > > 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 ? I'd like that property to be validated by Rob first. You raised the inconsistency above, and I'd rather have an ack from him first. Having a "this is how every i2c-hid device works" kind of argument might make it enough for him to change his opinion (because I think that was the argument for the post power delay). > > 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. I think that's how the whole separate driver started. And the argument of having separate drivers still stands, for devices that are not doing the same things like others. The compatible allows to store a set of device specific data that will not change whatever board the device is placed in. So though technically easier for the maintainers, having a dedicated property is putting the burden on the user. While OTOH, if this property is internal API, we can have a table with it, that says that Goodix needs X ms, when Elan Y and Wacom Z ms. But then, you're going to say that this requires a kernel bump, when a device property is just on the board side, so much convenient from an OEM point of view :( > > (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. And I thank you for that :) > > 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. Just a small thinking here: if we keep the current compatible drivers here, we have an example we can point people at if they need fancier things. So maybe keeping them (or one at least) is a good thing, no? Cheers, Benjamin > > Regards, > > Hans > >
Hi Benjamin, On 4/11/23 14:50, Benjamin Tissoires wrote: > On Apr 11 2023, Hans de Goede wrote: >> 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. > > Fair enough. > >> >>> 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. > > Though this is all conditional if we can make ACPI SSDT override > something that can be shipped while installing the device... > >> >>> 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. > > AFAIU, the non devicetree properties should also be documented through > DT bindings, no? So not documenting feels very much like "I want to slip > this one in without having to deal with DT maintainers" (and before you > take it personaly, I know this is definitively not the intent). So I'd > rather much having a public API documented, even if there are no users. Right, so as a hobby I have a tendency to work on these somewhat niche/weird x86 devices, like x86 tablets which use Android as factory OS :) As such I have encountered the need for device-properties to pass info from drivers/platform/x86 code to more generic drivers a number of times before. Each time this happens, if I try to add them to bindings I get asked for some example devicetree code, I then respond that these are *device*-properties not of-properties and that there are no current devicetree users after which the DT maintainers tell me to then NOT document them in the DT bindings, at least not until actually DT users show up. I fully expect any attempt do add this to the DT bindings to go the same way. And now I have you telling me you really want to see this documented at the same time as it getting implemented. Which I fully understand, but does leads to a bit of a catch 22. Anyways lets just go with the alternative of treating this similar as the existing specialized drivers, see below. <snip> >> 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 . > > I'd much rather have a i2c-hid-of.c internal API, yes. Whether it's a > function call, a callback or a match-data (or a driver-data), this is > something we are in control and we can change. Ok. So I see 2 options here: 1. Take the approach from patches 1-4 here, but drop the property and use match data on a new "wacom0169" i2c_device_id instead. This would also pave the way to merging patches 5 + 6 once tested by google to reduce some code duplication. Although you write below you would prefer to keep these around as example code for other specialized drivers... 2. Add a new specialized i2c-hid-of-wacom driver for this. Question: Since this will be using i2c_device_id binding not DT/OF binding the -of- in the name is technically incorrect, but it would be consistent with the other specialized drivers and could be seen as preparation (avoiding a rename/confusion) for when any DT enumerated devices which need special handling show up (note only relevant if you prefer this approach). Either way is fine with me really. So you get to chose. If you let me know which route you prefer, I'll go and prepare either a v2 of this series, or a whole new patch for the new specialized driver. Regards, Hans
On Apr 11 2023, Hans de Goede wrote: > Hi Benjamin, > > On 4/11/23 14:50, Benjamin Tissoires wrote: > > On Apr 11 2023, Hans de Goede wrote: > >> 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. > > > > Fair enough. > > > >> > >>> 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. > > > > Though this is all conditional if we can make ACPI SSDT override > > something that can be shipped while installing the device... > > > >> > >>> 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. > > > > AFAIU, the non devicetree properties should also be documented through > > DT bindings, no? So not documenting feels very much like "I want to slip > > this one in without having to deal with DT maintainers" (and before you > > take it personaly, I know this is definitively not the intent). So I'd > > rather much having a public API documented, even if there are no users. > > Right, so as a hobby I have a tendency to work on these somewhat niche/weird > x86 devices, like x86 tablets which use Android as factory OS :) > > As such I have encountered the need for device-properties to pass info > from drivers/platform/x86 code to more generic drivers a number of > times before. > > Each time this happens, if I try to add them to bindings I get > asked for some example devicetree code, I then respond that these > are *device*-properties not of-properties and that there are no > current devicetree users after which the DT maintainers tell me > to then NOT document them in the DT bindings, at least not until > actually DT users show up. I fully expect any attempt do add > this to the DT bindings to go the same way. > > And now I have you telling me you really want to see this > documented at the same time as it getting implemented. Which > I fully understand, but does leads to a bit of a catch 22. Ouch. Sorry for that. Then I guess if the DT maintainers have a tendency to accept those hidden properties, this is the simplest solution from a i2c-hid/HID maintainer point of view, no? It's going to be a pain for the platform driver because you still have to hardcode those properties somewhere I guess. > > Anyways lets just go with the alternative of treating this > similar as the existing specialized drivers, see below. > > <snip> > > >> 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 . > > > > I'd much rather have a i2c-hid-of.c internal API, yes. Whether it's a > > function call, a callback or a match-data (or a driver-data), this is > > something we are in control and we can change. > > Ok. > > So I see 2 options here: > > 1. Take the approach from patches 1-4 here, but drop the property and > use match data on a new "wacom0169" i2c_device_id instead. > This would also pave the way to merging patches 5 + 6 once tested > by google to reduce some code duplication. Although you write below > you would prefer to keep these around as example code for other > specialized drivers... > > 2. Add a new specialized i2c-hid-of-wacom driver for this. > Question: Since this will be using i2c_device_id binding not > DT/OF binding the -of- in the name is technically incorrect, > but it would be consistent with the other specialized drivers > and could be seen as preparation (avoiding a rename/confusion) > for when any DT enumerated devices which need special handling > show up (note only relevant if you prefer this approach). Well, option 2 is probably too much work for little gain. So I would go with option 1, but with the following questions: - a device property is public, so it can be seen as public API, right? So should we document it some way (not through DT) so we "guarantee" some behavior for it? If the above is correct, then that means that the device property can be used, which makes little changes to your series. But then, why aren't you using that property directly for those 2 other drivers? Can't we have elan and goodix i2c-hid-of variants, be just a stub around adding the gpio names and the specific reset? (A plain "this is completely dumb" answer is fine, just trying to get my head around it). So, given the above, and your experience with the DT maintainers, I would go with patches 1-3 + a documentation of the new property, likely in the header or in kernel docs. Patches 4-6 either dropped, reworked, or left as they are, and we would merge them only if the maintainers of those files tested the changes. And if you prefer storing the post-reset delay in the hid tree, that's fine too, but I guess you would prefer having less friction by keeping it in the platform tree. > > Either way is fine with me really. So you get to chose. If you > let me know which route you prefer, I'll go and prepare either > a v2 of this series, or a whole new patch for the new specialized > driver. Sorry for being a PITA, but having those driver separated allowed to move forward without having to have a spaghetti plate in i2c-hid, which was the case before the split (because *everything* was entangled: ACPI, DT, OF, properties). So that's why I'm trying to understand and minimize the changes. Also, before you sending v2 and involving too much, we could try to wait a few days for Doug to answer, and hear if he has an opinion. But if you rather send v2 right away, that's your choice obviously :) Cheers, Benjamin > > Regards, > > Hans > >
Hi Benjamin, On 4/11/23 18:56, Benjamin Tissoires wrote: > On Apr 11 2023, Hans de Goede wrote: >> Hi Benjamin, >> >> On 4/11/23 14:50, Benjamin Tissoires wrote: >>> On Apr 11 2023, Hans de Goede wrote: >>>> Hi Benjamin, >>>> >>>> On 4/11/23 11:02, Benjamin Tissoires wrote: <snip> >>>>> 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. >>> >>> AFAIU, the non devicetree properties should also be documented through >>> DT bindings, no? So not documenting feels very much like "I want to slip >>> this one in without having to deal with DT maintainers" (and before you >>> take it personaly, I know this is definitively not the intent). So I'd >>> rather much having a public API documented, even if there are no users. >> >> Right, so as a hobby I have a tendency to work on these somewhat niche/weird >> x86 devices, like x86 tablets which use Android as factory OS :) >> >> As such I have encountered the need for device-properties to pass info >> from drivers/platform/x86 code to more generic drivers a number of >> times before. >> >> Each time this happens, if I try to add them to bindings I get >> asked for some example devicetree code, I then respond that these >> are *device*-properties not of-properties and that there are no >> current devicetree users after which the DT maintainers tell me >> to then NOT document them in the DT bindings, at least not until >> actually DT users show up. I fully expect any attempt do add >> this to the DT bindings to go the same way. >> >> And now I have you telling me you really want to see this >> documented at the same time as it getting implemented. Which >> I fully understand, but does leads to a bit of a catch 22. > > Ouch. Sorry for that. No problem. > Then I guess if the DT maintainers have a tendency > to accept those hidden properties, this is the simplest solution from a > i2c-hid/HID maintainer point of view, no? Yes, I believe so, which is why I went this route in the first place. > It's going to be a pain for the > platform driver because you still have to hardcode those properties > somewhere I guess. Since the entire description is missing in ACPI for the digitizer (*) the x86-android-tablets.ko module which contains glue code to support these x86 android tablets already contains the i2c-busnumber, i2c-address, GPIO lookups, IRQ and other necessary device-properties like "hid-descr-addr", so adding one more device-property is very little trouble. *) and also for other devices both on this and other x86 android tablets <snip> >> So I see 2 options here: >> >> 1. Take the approach from patches 1-4 here, but drop the property and >> use match data on a new "wacom0169" i2c_device_id instead. >> This would also pave the way to merging patches 5 + 6 once tested >> by google to reduce some code duplication. Although you write below >> you would prefer to keep these around as example code for other >> specialized drivers... >> >> 2. Add a new specialized i2c-hid-of-wacom driver for this. >> Question: Since this will be using i2c_device_id binding not >> DT/OF binding the -of- in the name is technically incorrect, >> but it would be consistent with the other specialized drivers >> and could be seen as preparation (avoiding a rename/confusion) >> for when any DT enumerated devices which need special handling >> show up (note only relevant if you prefer this approach). > > Well, option 2 is probably too much work for little gain. So I would go > with option 1, but with the following questions: > > - a device property is public, so it can be seen as public API, right? > So should we document it some way (not through DT) so we "guarantee" > some behavior for it? I believe the whole idea from the DT maintainers behind not documenting it as DT binding when not actually used in dts files is to keep it as in kernel *private* API, in this case between the x86-android-tablets.ko code instantiating the i2c_client and the i2c-hid code consuming it. Take the hideep touchscreen on this same tablet for example. After this patch series we could also use it in i2c-hid mode (I did test that as an extra test for patches 1-3) but the stock Android uses it in its native hideep protocol mode which gives some more info (ABS_MT_PRESSURE and ABS_MT_TOUCH_MAJOR). So currently in -next the touchscreen is driven in its native mode. This requires sending a command to (re)set it to native mode since it comes up in i2c-hid mode by default. This command is only send if a device-property is set (to avoid causing issues on other hideep models) and the code consuming that property looks like this: /* * Reset touch report format to the native HiDeep 20 protocol if requested. * This is necessary to make touchscreens which come up in I2C-HID mode * work with this driver. * * Note this is a kernel internal device-property set by x86 platform code, * this MUST not be used in devicetree files without first adding it to * the DT bindings. */ if (device_property_read_bool(&ts->client->dev, "hideep,force-native-protocol")) regmap_write(ts->reg, HIDEEP_WORK_MODE, 0x00); So maybe copy that and just add a: /* * Note this is a kernel internal device-property set by x86 platform code, * this MUST not be used in devicetree files without first adding it to * the DT bindings. */ Comment to the code reading the "post-reset-deassert-delay-ms" property (patch 3/6) for v2 of this patch-set and leave it at that ? (and in hindsight I should have added that comment for v1 already) > If the above is correct, then that means that the device property can > be used, which makes little changes to your series. Sounds good to me. > But then, why aren't you using that property directly for those 2 other > drivers? Can't we have elan and goodix i2c-hid-of variants, be just a > stub around adding the gpio names and the specific reset? (A plain "this > is completely dumb" answer is fine, just trying to get my head around it). Only 1 driver can bind to an i2c_client, and if those stub drivers bind to it, then they must deal with it, or they would need to create some fake i2c_client and pass everything through, but that would be rather ugly. > So, given the above, and your experience with the DT maintainers, I > would go with patches 1-3 + a documentation of the new property, likely > in the header or in kernel docs. I'm fine with going with just patches 1-3. With patch 3 updated to add the "this is a kernel internal only property" comment. > Patches 4-6 either dropped, reworked, or left as they are, and we would > merge them only if the maintainers of those files tested the changes. Patches 4-6 were meant to make adding support for more i2c-hid-of devices in the future easier, nothing more nothing less. So I'm fine with dropping them. I agree that at a minimum they should get tested before merging them. Regards, Hans
On Apr 11 2023, Hans de Goede wrote: > Hi Benjamin, > > On 4/11/23 18:56, Benjamin Tissoires wrote: > > On Apr 11 2023, Hans de Goede wrote: > >> Hi Benjamin, > >> > >> On 4/11/23 14:50, Benjamin Tissoires wrote: > >>> On Apr 11 2023, Hans de Goede wrote: > >>>> Hi Benjamin, > >>>> > >>>> On 4/11/23 11:02, Benjamin Tissoires wrote: > > <snip> > > >>>>> 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. > >>> > >>> AFAIU, the non devicetree properties should also be documented through > >>> DT bindings, no? So not documenting feels very much like "I want to slip > >>> this one in without having to deal with DT maintainers" (and before you > >>> take it personaly, I know this is definitively not the intent). So I'd > >>> rather much having a public API documented, even if there are no users. > >> > >> Right, so as a hobby I have a tendency to work on these somewhat niche/weird > >> x86 devices, like x86 tablets which use Android as factory OS :) > >> > >> As such I have encountered the need for device-properties to pass info > >> from drivers/platform/x86 code to more generic drivers a number of > >> times before. > >> > >> Each time this happens, if I try to add them to bindings I get > >> asked for some example devicetree code, I then respond that these > >> are *device*-properties not of-properties and that there are no > >> current devicetree users after which the DT maintainers tell me > >> to then NOT document them in the DT bindings, at least not until > >> actually DT users show up. I fully expect any attempt do add > >> this to the DT bindings to go the same way. > >> > >> And now I have you telling me you really want to see this > >> documented at the same time as it getting implemented. Which > >> I fully understand, but does leads to a bit of a catch 22. > > > > Ouch. Sorry for that. > > No problem. > > > Then I guess if the DT maintainers have a tendency > > to accept those hidden properties, this is the simplest solution from a > > i2c-hid/HID maintainer point of view, no? > > Yes, I believe so, which is why I went this route in the first place. > > > It's going to be a pain for the > > platform driver because you still have to hardcode those properties > > somewhere I guess. > > Since the entire description is missing in ACPI for the digitizer (*) > the x86-android-tablets.ko module which contains glue code to support > these x86 android tablets already contains the i2c-busnumber, > i2c-address, GPIO lookups, IRQ and other necessary device-properties > like "hid-descr-addr", so adding one more device-property is very little > trouble. > > *) and also for other devices both on this and other x86 android tablets > > <snip> > > >> So I see 2 options here: > >> > >> 1. Take the approach from patches 1-4 here, but drop the property and > >> use match data on a new "wacom0169" i2c_device_id instead. > >> This would also pave the way to merging patches 5 + 6 once tested > >> by google to reduce some code duplication. Although you write below > >> you would prefer to keep these around as example code for other > >> specialized drivers... > >> > >> 2. Add a new specialized i2c-hid-of-wacom driver for this. > >> Question: Since this will be using i2c_device_id binding not > >> DT/OF binding the -of- in the name is technically incorrect, > >> but it would be consistent with the other specialized drivers > >> and could be seen as preparation (avoiding a rename/confusion) > >> for when any DT enumerated devices which need special handling > >> show up (note only relevant if you prefer this approach). > > > > Well, option 2 is probably too much work for little gain. So I would go > > with option 1, but with the following questions: > > > > - a device property is public, so it can be seen as public API, right? > > So should we document it some way (not through DT) so we "guarantee" > > some behavior for it? > > I believe the whole idea from the DT maintainers behind not documenting > it as DT binding when not actually used in dts files is to keep it as > in kernel *private* API, in this case between the x86-android-tablets.ko > code instantiating the i2c_client and the i2c-hid code consuming it. > > Take the hideep touchscreen on this same tablet for example. After > this patch series we could also use it in i2c-hid mode (I did test > that as an extra test for patches 1-3) but the stock Android uses > it in its native hideep protocol mode which gives some more info > (ABS_MT_PRESSURE and ABS_MT_TOUCH_MAJOR). So currently in -next > the touchscreen is driven in its native mode. This requires sending > a command to (re)set it to native mode since it comes up in i2c-hid > mode by default. > > This command is only send if a device-property is set (to avoid > causing issues on other hideep models) and the code consuming > that property looks like this: > > /* > * Reset touch report format to the native HiDeep 20 protocol if requested. > * This is necessary to make touchscreens which come up in I2C-HID mode > * work with this driver. > * > * Note this is a kernel internal device-property set by x86 platform code, > * this MUST not be used in devicetree files without first adding it to > * the DT bindings. > */ > if (device_property_read_bool(&ts->client->dev, "hideep,force-native-protocol")) > regmap_write(ts->reg, HIDEEP_WORK_MODE, 0x00); > > So maybe copy that and just add a: > > /* > * Note this is a kernel internal device-property set by x86 platform code, > * this MUST not be used in devicetree files without first adding it to > * the DT bindings. > */ > > Comment to the code reading the "post-reset-deassert-delay-ms" > property (patch 3/6) for v2 of this patch-set and leave it > at that ? This seems like a better compromised :) > > (and in hindsight I should have added that comment for v1 already) > > > If the above is correct, then that means that the device property can > > be used, which makes little changes to your series. > > Sounds good to me. > > > But then, why aren't you using that property directly for those 2 other > > drivers? Can't we have elan and goodix i2c-hid-of variants, be just a > > stub around adding the gpio names and the specific reset? (A plain "this > > is completely dumb" answer is fine, just trying to get my head around it). > > Only 1 driver can bind to an i2c_client, and if those stub drivers > bind to it, then they must deal with it, or they would need to > create some fake i2c_client and pass everything through, but that > would be rather ugly. I was thinking that the i2c-hid-elan driver would override the property, but that is assuming a driver can change a property once the device is created, which I am now unsure. > > > So, given the above, and your experience with the DT maintainers, I > > would go with patches 1-3 + a documentation of the new property, likely > > in the header or in kernel docs. > > I'm fine with going with just patches 1-3. With patch 3 updated to > add the "this is a kernel internal only property" comment. Sounds like a good plan :) > > > Patches 4-6 either dropped, reworked, or left as they are, and we would > > merge them only if the maintainers of those files tested the changes. > > Patches 4-6 were meant to make adding support for more > i2c-hid-of devices in the future easier, nothing more nothing > less. So I'm fine with dropping them. > > I agree that at a minimum they should get tested before > merging them. We can keep them in a separate series, and wiat until we get some tests on them before merging them, yes. Cheers, Benjamin
Hi, On Tue, Apr 11, 2023 at 9:57 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > > Either way is fine with me really. So you get to chose. If you > > let me know which route you prefer, I'll go and prepare either > > a v2 of this series, or a whole new patch for the new specialized > > driver. > > Sorry for being a PITA, but having those driver separated allowed to > move forward without having to have a spaghetti plate in i2c-hid, which > was the case before the split (because *everything* was entangled: ACPI, > DT, OF, properties). So that's why I'm trying to understand and > minimize the changes. > > Also, before you sending v2 and involving too much, we could try to wait a > few days for Doug to answer, and hear if he has an opinion. But if you > rather send v2 right away, that's your choice obviously :) I can test things if need be, but I want to make sure we're on the right approach before going too deep into testing... I guess a few notes here: In general, I think DT maintainers are pretty leery of anything in the device tree that tries to be "generic" and then a whole pile of "kitchen sink" properties added to actually describe the device. Even if it starts with just a few properties, the worry is that it will end up being more and more over time. They would much rather specify which exact device is present on the board and imply all the properties based on knowing the device. Then the only things that are in the device tree as properties are things that are board-specific. For instance, if there was a hardware strapping that let you choose two different hid descriptor addresses then that would be something to put in the device tree. The "post-power-on-delay-ms" was something that the DT maintainers weren't too happy with. They would have much rather inferred this from the specific compatible. You can actually see that the bindings say that "Just "hid-over-i2c" alone is allowed, but not recommended." Now, that being said, it's not always a hard-and-fast rule. For instance, after years of needing to list every eDP panel directly in device tree (often lying about it when multiple sources were listed), we finally did manage to get the generic "panel-edp" bindings accepted that has "just a few" properties needed to power up a device. ...then the rest of the things we need are inferred once we start talking to the device and get it to self-identify. Bringing it back to i2c-hid-of: even though today the "goodix" and "elan" drivers are largely the same, it wasn't always the case. For a little while we had a whole pile of special logic in the "goodix" driver to deal with the fact that if the touchscreen is powered up (because it's shared or always-on) but the reset line is held asserted that it draws a bunch of extra power. I had to end up taking that logic out because it was too hard to reconcile with the second voltage rail that I needed to add for a different board. See commit 557e05fa9fdd ("HID: i2c-hid: goodix: Stop tying the reset line to the regulator") and eb16f59e8e58 ("HID: i2c-hid: goodix: Add mainboard-vddio-supply"). The need for this special logic is, as far as I know, Goodix specific. I'm not aware of other touchscreens holding themselves in a high power state if they are powered while their reset line is held low. I don't think upstream would have liked a DT properly like "avoid-holding-reset-low-while-powered;" Ironically, there is actually more work to be done here. It turns out that a different Chromebook that I wasn't aware of (and that wasn't upstream yet) actually was relying on behavior to not assert reset and we still need to figure out how to reconcile all of this. :( I guess in general the idea of combining the drivers vs. coming up with generic bindings is actually two separate things. We could have separate bindings and still have one driver. At the time I made i2c-of-goodix I was specifically requested to make separate drivers for it. If maintainers want to re-combine them now, I won't object. ...but at least at the time it was a conscious decision and a specific request to make them separate. Looking at i2c-of-goodix and i2c-of-elan, we could probably combine _those_ two drivers at this point, unless we actually end up needing to go back again and do something goodix-specific for the reset line again. -Doug
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). 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. 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 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