Date:   Fri, 22 Sep 2023 10:40:07 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Rob Herring <robh+dt@...nel.org>
Cc:     Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>, devicetree@...r.kernel.org,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        Chen-Yu Tsai <wenst@...omium.org>, linux-input@...r.kernel.org,
        Jiri Kosina <jikos@...nel.org>,
        Hsin-Yi Wang <hsinyi@...omium.org>, linux-gpio@...r.kernel.org,
        linus.walleij@...aro.org,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Johan Hovold <johan+linaro@...nel.org>,
        andriy.shevchenko@...ux.intel.com, broonie@...nel.org,
        frowand.list@...il.com, gregkh@...uxfoundation.org,
        hdegoede@...hat.com, james.clark@....com, james@...iv.tech,
        keescook@...omium.org, linux-kernel@...r.kernel.org,
        rafael@...nel.org, tglx@...utronix.de
Subject: Re: [RFC PATCH] of: device: Support 2nd sources of probeable but
 undiscoverable devices

Hi,

On Fri, Sep 22, 2023 at 7:14 AM Rob Herring <robh+dt@...nel.org> wrote:
>
> > Let's attempt to do something better. Specifically, we'll allow
> > tagging nodes in the device tree as mutually exclusive from one
> > another. This says that only one of the components in this group is
> > present on any given board. To make it concrete, in my proposal this
> > looks like:
> >
> >   / {
> >     tp_ex_group: trackpad-exclusion-group {
> >     };
>
> Interesting way to just get a unique identifier. But it could be any
> phandle not used by another group. So just point all the devices in a
> group to one of the devices in the group.

Fair enough.


> >   &i2c_bus {
> >     tp1: trackpad@10 {
> >       ...
> >       mutual-exclusion-group = <&tp_ex_group>;
> >     };
> >     tp2: trackpad@20 {
> >       ...
> >       mutual-exclusion-group = <&tp_ex_group>;
> >     };
> >     tp3: trackpad@30 {
> >       ...
> >       mutual-exclusion-group = <&tp_ex_group>;
> >     };
> >   };
> >
> > In Linux, we can make things work by simply only probing one of the
> > devices in the group at a time. We can make a mutex per group and
> > enforce locking that mutex around probe. If the first device that gets
> > the mutex fails to probe then it won't try again. If it succeeds then
> > it will acquire the shared resources and future devices (which we know
> > can't be present) will fail to get the shared resources. Future
> > patches could quiet down errors about failing to acquire shared
> > resources or failing to probe if a device is in a
> > mutual-exclusion-group.
>
> This seems like overkill to me. Do we really need groups and a mutex
> for each group? Worst case is what? 2-3 groups of 2-3 devices?
> Instead, what about extending "status" with another value
> ("fail-needs-probe"? (fail-xxx is a documented value)). Currently, the
> kernel would just ignore nodes with that status. Then we can process
> those nodes separately 1-by-1.

My worry here is that this has the potential to impact boot speed in a
non-trivial way. While trackpads and touchscreens _are_ probable,
their probe routines are often quite slow. This is even mentioned in
Dmitry's initial patches adding async probe to the kernel. See commit
765230b5f084 ("driver-core: add asynchronous probing support for
drivers") where he specifically brings up input devices as examples.

It wouldn't be absurd to have a system that has multiple sources for
both the trackpad and the touchscreen. If we have to probe each of
these one at a time then it could be slow. It would be quicker to be
able to probe the trackpads (one at a time) at the same time we're
probing the touchscreens (one at a time). Using the "fail-needs-probe"
doesn't provide information needed to know which devices conflict with
each other. IMO this is still better than nothing, but it worries me
to pick the less-expressive solution for the dts which means that the
information simply isn't there and the OS can't be made better later.

Thinking about this more, I guess even my proposed solution isn't
ideal for probe speed. Let's imagine that we had:

  &i2c_bus {
    tp1: trackpad@10 {
      compatible = "hid-over-i2c";
      reg = <0x10>;
      post-power-on-delay-ms = <200>;
      ...
      mutual-exclusion-group = <&tp1>;
    };
    tp2: trackpad@20 {
      compatible = "hid-over-i2c";
      reg = <0x20>;
      post-power-on-delay-ms = <200>;
      ...
      mutual-exclusion-group = <&tp1>;
    };
  };

With my solution, we'd power the first device up, wait 200 ms, then
check to see if anything acks an i2c xfer at address 0x10. If it
didn't, we'd power down. Then we'd power up the second device
(presumably the same power rail), wait 200 ms, and check to see if
anything acks an i2c xfer at 0x20. It would have been better to just
power up once, wait 200 ms, then check for a device at either 0x10 or
0x20.

I guess with more complex touchscreens this could be more important. I
don't know if we need to try to solve it at this point, but I guess I
could imagine a case where we truly need to take into account all
possible devices (maybe taking the maximum of delays?) to ensure we
don't violate power sequencing requirements for any of them while
probing.

That would lead me to suggest this:

  &i2c_bus {
    trackpad-prober {
      compatible = "mt8173-elm-hana-trackpad-prober";

      tp1: trackpad@10 {
        compatible = "hid-over-i2c";
        reg = <0x10>;
        ...
        post-power-on-delay-ms = <200>;
      };
      tp2: trackpad@20 {
        compatible = "hid-over-i2c";
        reg = <0x20>;
        ...
        post-power-on-delay-ms = <200>;
      };
    };
  };

...but I suspect that would be insta-NAKed because it's creating a
completely virtual device ("mt8173-elm-hana-trackpad-prober") in the
device tree. I don't know if there's something that's functionally
similar that would be OK?

-Doug

Powered by blists - more mailing lists