Hi, On Tue, Apr 11, 2023 at 9:57 AM Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx> 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