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
