Re: [PATCH v6 09/12] i2c: of-prober: Add regulator support

[Chen-Yu Tsai]

On Wed, Sep 11, 2024 at 8:30 AM Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
>
> Hi,
>
> On Thu, Sep 5, 2024 at 8:45 PM Chen-Yu Tsai <wenst@xxxxxxxxxxxx> wrote:
> >
> > > > IIUC we could have the "options" data structure have much more board
> > > > specific information:
> > > >
> > > >   - name of node to fetch resources (regulator supplies and GPIOs) from
> > > >   - names of the resources for the node given from the previous item
> > > >   - delay time after each resource is toggled
> > > >   - polarity in the case of GPIOs
> > > >   - prober callback to do power sequencing
> > > >
> > > > The "resource collection" step would use the first two items to retrieve
> > > > the regulator supplies and GPIOS instead of the bulk APIs used right now.
> > > >
> > > > The power sequencing callback would use the resources combined with the
> > > > given delays to enable the supplies and toggle the GPIOs.
> > > >
> > > > For now I would probably only implement a generic one regulator supply
> > > > plus one GPIO helper. That is the common case for touchscreens and
> > > > trackpads connected over a ribbon cable.
> > > >
> > > > Does that sound like what you have in mind?
> > >
> > > I guess I'd have to see how the code looks to know for sure, but if I
> > > understand it sounds a little awkward. Specifically, the "options"
> > > sound like they might become complicated enough that you're inventing
> > > your own little programming language (with delays, abilities to drive
> > > pins low and high, abilities to turn on/off clocks, and abilities to
> > > turn off/on regulators) and then probers need to code up their
> > > programs in this language. You also need to handle undoing things
> > > properly if there is a failure in the middle. Like your "program"
> > > would look like this (obviously you'd have to play with enums more,
> > > but you get the idea):
> > >
> > > {
> > >    { OPCODE_TURN_REGULATOR_ON, "vdd" },
> > >    { OPCODE_DELAY, 10 },
> > >    { OPCODE_GPIO_ASSERT, "reset" },
> > >    { OPCODE_DELAY, 5 },
> > >    { OPCODE_GPIO_DEASSERT "reset" },
> > >    { OPCODE_DELAY, 100 },
> > >    { OPCODE_TURN_REGULATOR_ON, "vddIO" },
> > > }
> > >
> > > Why not just expect the board probers to write C code to turn things
> > > on before looking for i2c devices, then provide helpers to the C code?
> > >
> > > So there wouldn't be some generic "resource collection" API, but you'd
> > > provide a helper to make it easy to grab regulators from one of the
> > > nodes by name. If you think bulk enabling regulators is common then
> > > you could make a helper that grabs all of the regulators from a node
> > > in a way that is consistent with the bulk APIs, but I wouldn't expect
> > > every driver to use that since devices I've seen expect regulators to
> > > be enabled in a very specific order even if they don't need a delay
> > > between them.
> > >
> > > I wouldn't expect a "collect all GPIOs" API because it seems really
> > > weird to me that we'd ever want to jam multiple GPIOs in a state
> > > without knowing exactly which GPIO was what and asserting them in the
> > > right sequence.
> >
> > So I'm slightly confused, as it sounds like at this point the i2c prober
> > would be litter more than just a framework, and the heavy lifting is to
> > be all done by callbacks provided by the board-specific driver?
> >
> > So the framework becomes something like:
> >
> > 1. find i2c bus node
> > 2. call provided callback with i2c bus node to gather resources;
> >    let callback handle specifics
> > 3. call provided callback to enable resources
> > 4. for each i2c component, call provided callback to probe
>
> I don't think I'd do it as callbacks but just have the HW prober call
> the functions directly. AKA, instead of doing:
>
>   i2c_of_probe_component(dev, "touchscreen", ts_opts, ts_callbacks);
>
> Do:
>
>   grab_touchscreen_resources(...);
>   power_on_touchscreens(...);
>   i2c_of_probe_component(...);
>   power_off_touchscreen(...);
>   release_touchscreen_resources(...);
>
> Obviously I'm spitballing here, though. Without writing the code it's
> hard for me to know that my proposal would be better, but my gut tells
> me that trying to write something overly generic with lots of options
> / callbacks would be more confusing.

The helpers would be exported along with the framework, so there's nothing
preventing others from using the helpers directly. The framework provides
boilerplate so that scenarios fitting it won't have to duplicate the code.
Obviously I'm basing it on my scenario here, but I think it works out for
the simpler cases.

I'll send out what I have reworked, and we can continue the discussion
either on the mailing list or in person at ELCE and Plumbers.

> >   If the probe succeeded:
> >
> >     5. call provided callback for early release of resources (GPIOs)
> >     6. set "status" to "okay"
> >     7. call provided callback for late release of resources (regulators)
> >
> >   Otherwise at the end of the loop
> >
> > 8. release resources
> >
> > The current code can be reworked into helpers for steps 2, 3, 5, 7 for
> > the single regulator single GPIO case.
> >
> > > > This next item would be a later enhancement (which isn't implemented in
> > > > this series anyway):
> > > >
> > > >   - optional prober callback that does actual probing
> > > >
> > > > In our case it would only be used for cases where an HID-over-I2C
> > > > component shares the same address as a non-HID one, and some extra
> > > > work is needed to determine which type it is. I still need to think
> > > > about the structure of this.
> > >
> > > IMO _that_ would be a great option to the i2c prober. It feels like
> > > you could have an optional register read that needs to match to have
> > > the i2c prober succeed. Most people would leave it blank (just the i2c
> > > device existing is enough) but probably a single register read would
> > > be enough to confirm you got the right device. Most i2c devices have
> > > some sort of "version" / "vendor" / "id" type register somewhere.
> >
> > At least for the stuff that we have (touchscreens and trackpads) such
> > registers typically don't exist, unless it's an HID-over-I2C device,
> > in which case there's the standard HID descriptor at some address.
> > But, yeah, reading the HID descriptor was the use case I had in mind.
> >
> > At least for one Chromebooks it's a bit more tricky because that one
> > HID-over-I2C component shares the same address as a non-HID one. We
> > currently have different SKU IDs and thus different device trees for
> > them, but we could make the prober work with this. It just has be able
> > to tell if the component it's currently probing needs the special
> > prober and is it responding correctly. This bit I need to think about.
>
> I guess Mark Brown also thought that there wouldn't be some magic
> register, but my gut still tells me that most i2c devices have some
> way to confirm that they are what you expect even if it's not an
> official "vendor" or "version" register. Some type of predictable
> register at a predictable location that you could use, at least if you
> knew all of the options that someone might stuff.
>
> For instance, in elan trackpads you can see elan_i2c_get_product_id().
> That just reads a location (ETP_I2C_UNIQUEID_CMD = 0x0101) that could
> theoretically be used to figure out (maybe in conjunction with other
> registers) that it's an elan trackpad instead of an i2c-hid one. You'd
> have to (of course) confirm that an i2c-hid device wouldn't somehow
> return back data from this read that made it look like an elan
> trackpad, but it feels like there ought to be some way to figure it
> out with a few i2c register reads.
>
> ...that being said, I guess my original assertion that you might be
> able to figure out with a simple register read was naive and you'd
> actually need a function (maybe as a callback) to figure this out.

Yeah, that's my plan for a follow-up series, likely after the SKU ID
based prober work is done. The actual probe function would likely only
target the known cases of I2C address conflicts we have on ChromeOS
devices.