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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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