Re: [PATCH v4 4/6] i2c: of-prober: Add GPIO and regulator support

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

On Thu, Aug 08, 2024 at 05:59:27PM +0800, Chen-Yu Tsai wrote:
> This adds GPIO and regulator management to the I2C OF component prober.

Can this be two patches?

> 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.
> 
> 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.

...

> +/*
> + * While 8 seems like a small number, especially when probing many component
> + * options, in practice all the options will have the same resources. The
> + * code getting the resources below does deduplication to avoid conflicts.
> + */
> +#define RESOURCE_MAX 8

Badly (broadly) named constant. Is it not the same that defines arguments in
the OF phandle lookup? Can you use that instead?

...

> +#define REGULATOR_SUFFIX "-supply"

Name is bad, also move '-' to the code, it's not part of the suffix, it's a
separator AFAICT.

...

> +	p = strstr(prop->name, REGULATOR_SUFFIX);

strstr()?! Are you sure it will have no side effects on some interesting names?

> +	if (!p)
> +		return 0;

> +	if (strcmp(p, REGULATOR_SUFFIX))
> +		return 0;

Ah, you do it this way...

What about




> +
> +	strscpy(con, prop->name, p - prop->name + 1);
> +	regulator = regulator_of_get_optional(node, con);
> +	/* DT lookup should never return -ENODEV */
> +	if (IS_ERR(regulator))
> +		return PTR_ERR(regulator);

...

> +	for (int i = 0; i < data->regulators_num; i++)

Why signed?

> +		if (regulator_is_equal(regulator, data->regulators[i])) {
> +			regulator_put(regulator);
> +			regulator = NULL;
> +			break;
> +		}

...

> +#define GPIO_SUFFIX "-gpio"

Bad define name, and why not "gpios"?

...

> +	p = strstr(prop->name, GPIO_SUFFIX);
> +	if (p) {
> +		strscpy(con, prop->name, p - prop->name + 1);
> +		con_id = con;
> +	} else if (strcmp(prop->name, "gpio") && strcmp(prop->name, "gpios")) {
> +		return 0;

We have an array of these suffixes, please use it. If required make it exported
to the others.

> +	}

...

> +	ret = of_parse_phandle_with_args_map(node, prop->name, "gpio", 0, &phargs);
> +	if (ret)
> +		return ret;

> +	gpiod = fwnode_gpiod_get_index(fwnode, con_id, 0, GPIOD_ASIS, "i2c-of-prober");
> +	if (IS_ERR(gpiod)) {
> +		of_node_put(phargs.np);
> +		return PTR_ERR(gpiod);
> +	}

Try not to mix fwnode and OF specifics. You may rely on fwnode for GPIO completely.

> +	if (data->gpiods_num == ARRAY_SIZE(data->gpiods)) {
> +		of_node_put(phargs.np);
> +		gpiod_put(gpiod);
> +		return -ENOMEM;
> +	}

...

> +	for (int i = data->gpiods_num - 1; i >= 0; i--)
> +		gpiod_put(data->gpiods[i]);

This sounds like reinvention of gpiod_*_array() call.

...

> +	for (int i = data->regulators_num; i >= 0; i--)
> +		regulator_put(data->regulators[i]);

Bulk regulators?

...

> +	for_each_child_of_node_scoped(i2c_node, node) {

Eventually _scoped(), but...

> +		u32 addr;
> +
> +		if (!of_node_name_prefix(node, type))
> +			continue;
> +		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, &data);
> +		if (ret) {

> +			of_node_put(i2c_node);

...huh?!

> +			return ret;
> +		}
> +	}

-- 
With Best Regards,
Andy Shevchenko