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.
> * - 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.
> + 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()
> + 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?
> + 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;
}
...
> +static int i2c_of_probe_enable_res(struct device *dev, struct i2c_of_probe_data *data)
> +{
> + int ret = 0;
Redundant assignment.
> + 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?
> + return 0;
> +}
...
> struct i2c_adapter *i2c;
> + struct i2c_of_probe_data probe_data = {0};
Reversed xmas tree order?
'0' is not needed.
...
> + /* 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)
> + 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
