[PATCH v5 07/10] i2c: of-prober: Add regulator support

[Chen-Yu Tsai]

On Thu, Aug 22, 2024 at 10:09 PM Andy Shevchenko
<andriy.shevchenko at linux.intel.com> wrote:
>
> On Thu, Aug 22, 2024 at 05:20:00PM +0800, Chen-Yu Tsai wrote:
> > This adds regulator 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.
> > GPIOs will be handled in the next 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.
>
> ...
>
> >  /*
>
> >   * address responds.
> >   *
> >   * TODO:
> > - * - Support handling common regulators and GPIOs.
> > + * - Support handling common GPIOs.
>
> You can split this to two lines in the first place and have less churn in this
> patch and the other one.

Ack.

> >   * - Support I2C muxes
> >   */
>
> ..
>
> > +/* Returns number of regulator supplies found for node, or error. */
> > +static int i2c_of_probe_get_regulator(struct device *dev, struct device_node *node,
> > +                                   struct i2c_of_probe_data *data)
> > +{
> > +     struct regulator_bulk_data *tmp, *new_regulators;
> > +     int ret;
> > +
> > +     ret = of_regulator_bulk_get_all(dev, node, &tmp);
> > +     if (ret <= 0)
> > +             return ret;
>
> I would split this and explain 0 case.

Ack.

> > +     if (!data->regulators) {
> > +             data->regulators = tmp;
> > +             data->regulators_num = ret;
> > +             return ret;
> > +     };
> > +
> > +     new_regulators = krealloc(data->regulators,
> > +                               sizeof(*tmp) * (data->regulators_num + ret),
>
> krealloc_array()

Ack. Somehow I didn't find this function while I was rewriting the code.

> > +                               GFP_KERNEL);
> > +     if (!new_regulators) {
> > +             regulator_bulk_free(ret, tmp);
> > +             return -ENOMEM;
> > +     }
> > +
> > +     data->regulators = new_regulators;
>
> > +     for (unsigned int i = 0; i < ret; i++)
> > +             memcpy(&data->regulators[data->regulators_num++], &tmp[i], sizeof(*tmp));
>
> Seems like copying array to array, no? If so, can't be done in a single memcpy() call?

Ack.

> > +     return ret;
> > +}
>
> ...
>
> > +static int i2c_of_probe_get_res(struct device *dev, struct device_node *node,
> > +                             struct i2c_of_probe_data *data)
> > +{
> > +     struct property *prop;
> > +     int ret;
> > +
> > +     ret = i2c_of_probe_get_regulator(dev, node, data);
> > +     if (ret < 0) {
> > +             dev_err_probe(dev, ret, "Failed to get regulator supplies from %pOF\n", node);
> > +             goto err_cleanup;
> > +     }
> > +
> > +     return 0;
> > +
> > +err_cleanup:
> > +     i2c_of_probe_free_res(data);
> > +     return ret;
> > +}
>
> Hmm... why not
>
> static int i2c_of_probe_get_res(struct device *dev, struct device_node *node,
>                                 struct i2c_of_probe_data *data)
> {
>         struct property *prop;
>         int ret;
>
>         ret = i2c_of_probe_get_regulator(dev, node, data);
>         if (ret < 0) {
>                 i2c_of_probe_free_res(data);
>                 return dev_err_probe(dev, ret, "Failed to get regulator supplies from %pOF\n", node);
>         }
>
>         return 0;
> }
>
> ...

That would be more churn in the next patch, which introduces another
error condition requiring the same cleanup.

> > +static int i2c_of_probe_enable_res(struct device *dev, struct i2c_of_probe_data *data)
> > +{
> > +     int ret = 0;
>
> Redundant assignment.

Ack.

> > +     dev_dbg(dev, "Enabling regulator supplies\n");
> > +
> > +     ret = regulator_bulk_enable(data->regulators_num, data->regulators);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* largest post-power-on pre-reset-deassert delay seen among drivers */
> > +     msleep(500);
>
> How would we monitor if any [new] driver wants to use bigger timeout?

The assumption is that the person doing the integration should test for
this. This prober doesn't get called everywhere. It needs a driver to
call it, and that driver is written by someone for some specific platform.
Maybe I should explicitly spell that out in the function description?
Or even make it a parameter?

Also, having an arbitrarily large number here doesn't help platforms that
want to minimize boot time. On that front I'm also thinking about whether
it is possible to do a handover to the actual driver so that the latter
doesn't have to go through the whole power sequence again.

> > +     return 0;
> > +}
>
> ...
>
> >       struct i2c_adapter *i2c;
> > +     struct i2c_of_probe_data probe_data = {0};
>
> Reversed xmas tree order?

OK...

> '0' is not needed.

Ack.

> ...
>
> > +     /* Grab resources */
> > +     for_each_child_of_node_scoped(i2c_node, node) {
> > +             u32 addr;
> > +
> > +             if (!of_node_name_prefix(node, type))
> > +                     continue;
>
> Is it third or fourth copy of this code? At some point you probably want
>
> #define for_each_child_of_node_with_prefix_scoped()
>         for_each_if(...)
>
> (or equivalent)

Ack.


Thank you for the review.

ChenYu

> > +             if (of_property_read_u32(node, "reg", &addr))
> > +                     continue;
> > +
> > +             dev_dbg(dev, "Requesting resources for %pOF\n", node);
> > +             ret = i2c_of_probe_get_res(dev, node, &probe_data);
> > +             if (ret)
> > +                     return ret;
> > +     }
>
> --
> With Best Regards,
> Andy Shevchenko
>
>