On Thu, Aug 22, 2024 at 10:09 PM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> 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
>
>
