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 > >
