Message-ID: <CAGXv+5G7h08Pvd24_6LoUB_8w_Cd0RntRSjNdn_FjrRH1ZF5oQ@mail.gmail.com>
Date: Fri, 23 Aug 2024 18:32:16 +0800
From: Chen-Yu Tsai <wenst@...omium.org>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Rob Herring <robh@...nel.org>, Saravana Kannan <saravanak@...gle.com>, 
	Matthias Brugger <matthias.bgg@...il.com>, 
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>, Wolfram Sang <wsa@...nel.org>, 
	Benson Leung <bleung@...omium.org>, Tzung-Bi Shih <tzungbi@...nel.org>, 
	Mark Brown <broonie@...nel.org>, Liam Girdwood <lgirdwood@...il.com>, 
	chrome-platform@...ts.linux.dev, devicetree@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, Douglas Anderson <dianders@...omium.org>, 
	Johan Hovold <johan@...nel.org>, Jiri Kosina <jikos@...nel.org>, linux-i2c@...r.kernel.org
Subject: Re: [PATCH v5 08/10] i2c: of-prober: Add GPIO support

On Thu, Aug 22, 2024 at 10:20 PM Andy Shevchenko
<andriy.shevchenko@...ux.intel.com> wrote:
>
> On Thu, Aug 22, 2024 at 05:20:01PM +0800, Chen-Yu Tsai wrote:
> > This adds GPIO management to the I2C OF component prober.
> > Components that the prober intends to probe likely require their
> > regulator supplies be enabled, and GPIOs be toggled to enable them or
> > bring them out of reset before they will respond to probe attempts.
> > regulator support was added in the previous patch.
> >
> > Without specific knowledge of each component's resource names or
> > power sequencing requirements, the prober can only enable the
> > regulator supplies all at once, and toggle the GPIOs all at once.
> > Luckily, reset pins tend to be active low, while enable pins tend to
> > be active high, so setting the raw status of all GPIO pins to high
> > should work. The wait time before and after resources are enabled
> > are collected from existing drivers and device trees.
> >
> > The prober collects resources from all possible components and enables
> > them together, instead of enabling resources and probing each component
> > one by one. The latter approach does not provide any boot time benefits
> > over simply enabling each component and letting each driver probe
> > sequentially.
> >
> > The prober will also deduplicate the resources, since on a component
> > swap out or co-layout design, the resources are always the same.
> > While duplicate regulator supplies won't cause much issue, shared
> > GPIOs don't work reliably, especially with other drivers. For the
> > same reason, the prober will release the GPIOs before the successfully
> > probed component is actually enabled.
>
> ...
>
> > +     struct fwnode_handle *fwnode = of_fwnode_handle(node);
> > +     struct gpio_descs *gpiods;
> > +     struct gpio_desc *gpiod;
> > +     char con[32]; /* 32 is max size of property name */
>
> Use 'propname' to be aligned with GPIO library usages.

Ack.

> > +     char *con_id = NULL;
> > +     size_t new_size;
> > +     int len;
>
> ...
>
> > +     if (len >= sizeof(con) - 1) {
>
> This can be transformed to check the returned value from strscpy().

Ack.

> > +             pr_err("%pOF: length of GPIO name \"%s\" exceeds current limit\n",
> > +                    node, prop->name);
> > +             return -EINVAL;
> > +     }
> > +
> > +     if (len > 0) {
> > +             strscpy(con, prop->name, len + 1);
>
> The correct (robust) call is with destination size. Which means here that you
> may use 2-argument strscpy().

Ack.

> > +             con_id = con;
> > +     }
>
> ...
>
> > +     if (!data->gpiods)
> > +             return 0;
>
> If it comes a new code (something else besides GPIOs and regulators) this will be a (small) impediment. Better to have a helper for each case and do
>
>         ret = ..._gpiods();
>         if (ret)
>                 ...
>
> Same for regulators and anything else in the future, if any.

I'm not sure I follow. Do you mean wrap each individual type in a wrapper
and call those here, like the following?

    i2c_of_probe_enable_res(...)
    {
        ret = i2c_of_probe_enable_regulators(...)
        if (ret)
              return ret;

        ret = i2c_of_probe_enable_gpios(...)
        if (ret)
              goto error_disable_regulators;

        ...
    }

> > +             /*
> > +              * reset GPIOs normally have opposite polarity compared to
>
> "reset"
>
> > +              * enable GPIOs. Instead of parsing the flags again, simply
>
> "enable"
>
> > +              * set the raw value to high.
>
> This is quite a fragile assumption. Yes, it would work in 98% cases, but will
> break if it's not true somewhere else.

Well, this seems to be the de facto standard. Or it would have to remember
what each GPIO descriptor's name is, and try to classify those into either
"enable" or "reset", and set their respective logical values to 1 or 0.
And then you run into a peripheral with a broken binding that has its
"reset" GPIO inverted, i.e. it's driver behavior needs to follow the
"enable" GPIO style. The class of devices this prober targets are
consumer electronics (laptops, tablets, phones) that at least have gone
through some component selection where the options won't have conflicting
requirements.

And if the polarities of the possible components don't line up, then this
probe structure can't really do anything. One would need something that
power sequences each component separately and probes it. I would really
like to avoid that if possible, as it makes the boot time (to peripheral
available) dependent on which component you have and how far down the
list it is. We have Chromebooks that have 4 touchscreen components
introduced over the years. In that case something more like Doug's
original proposal would work better: something that forces mutual
exclusivity among a class of devices.

> > +              */
>
> ...
>
> > +     /* largest post-reset-deassert delay seen in tree for Elan I2C HID */
> > +     msleep(300);
>
> Same Q, how do you monitor _all_ the drivers?

Discussion in the previous patch.

> ...
>
> > +disable_gpios:
> > +     for (gpio_i--; gpio_i >= 0; gpio_i--)
> > +             gpiod_set_raw_value_cansleep(data->gpiods->desc[gpio_i], 0);
>
> Can't you call the _array() variant here?

I thought that without |struct gpio_array| the _array() variant wouldn't
help, i.e. it would still be a loop internally. Looks like I was wrong.

> ...
>
> > -     dev_dbg(dev, "Resources: # of regulator supplies = %d\n", probe_data.regulators_num);
> > +     dev_dbg(dev, "Resources: # of GPIOs = %d, # of regulator supplies = %d\n",
> > +             probe_data.gpiods ? probe_data.gpiods->ndescs : 0,
> > +             probe_data.regulators_num);
>
> I would issue one message per class of the devices (GPIOs, regulators, ...)

Ack.


Thank you for the review.
ChenYu

Powered by blists - more mailing lists