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
