Re: [PATCH v6 10/12] i2c: of-prober: Add GPIO support

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

On Wed, Sep 04, 2024 at 05:00:12PM +0800, Chen-Yu Tsai wrote:
> This adds GPIO 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.
> regulator support was added in the previous 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.

...

> +static int i2c_of_probe_get_gpiod(struct device_node *node, struct property *prop,
> +				  struct i2c_of_probe_data *data)
> +{
> +	struct fwnode_handle *fwnode = of_fwnode_handle(node);
> +	struct gpio_descs *gpiods;
> +	struct gpio_desc *gpiod;
> +	char propname[32]; /* 32 is max size of property name */
> +	char *con_id = NULL;
> +	size_t new_size;
> +	int len, ret;
> +
> +	len = gpio_get_property_name_length(prop->name);
> +	if (len < 0)
> +		return 0;
> +
> +	ret = strscpy(propname, prop->name);
> +	if (ret < 0) {
> +		pr_err("%pOF: length of GPIO name \"%s\" exceeds current limit\n",
> +		       node, prop->name);
> +		return -EINVAL;

Any good reason to shadow the error code from strscpy() here?

> +	}
> +
> +	if (len > 0) {
> +		/* "len < ARRAY_SIZE(propname)" guaranteed by strscpy() above */

is guaranteed

> +		propname[len] = '\0';

Please, check if it's guaranteed by strscpy() (IIRC it is, hence redundant line).

> +		con_id = propname;
> +	}
> +
> +	/*
> +	 * GPIO descriptors are not reference counted. GPIOD_FLAGS_BIT_NONEXCLUSIVE
> +	 * can't differentiate between GPIOs shared between devices to be probed and
> +	 * other devices (which is incorrect). If the initial request fails with
> +	 * -EBUSY, retry with GPIOD_FLAGS_BIT_NONEXCLUSIVE and see if it matches
> +	 * any existing ones.
> +	 */
> +	gpiod = fwnode_gpiod_get_index(fwnode, con_id, 0, GPIOD_ASIS, "i2c-of-prober");
> +	if (IS_ERR(gpiod)) {
> +		if (PTR_ERR(gpiod) != -EBUSY || !data->gpiods)
> +			return PTR_ERR(gpiod);
> +
> +		gpiod = fwnode_gpiod_get_index(fwnode, con_id, 0,
> +					       GPIOD_ASIS | GPIOD_FLAGS_BIT_NONEXCLUSIVE,
> +					       "i2c-of-prober");
> +		for (unsigned int i = 0; i < data->gpiods->ndescs; i++)
> +			if (gpiod == data->gpiods->desc[i])
> +				return 1;
> +
> +		return -EBUSY;
> +	}
> +
> +	new_size = struct_size(gpiods, desc, data->gpiods ? data->gpiods->ndescs + 1 : 1);
> +	gpiods = krealloc(data->gpiods, new_size, GFP_KERNEL);
> +	if (!gpiods) {
> +		gpiod_put(gpiod);
> +		return -ENOMEM;
> +	}
> +
> +	data->gpiods = gpiods;
> +	data->gpiods->desc[data->gpiods->ndescs++] = gpiod;
> +
> +	return 1;
> +}

...

> +static int i2c_of_probe_set_gpios(struct device *dev, struct i2c_of_probe_data *data)
> +{
> +	int ret;
> +	int gpio_i;

Why signed? And can it be simply named 'i'?

> +
> +	if (!data->gpiods)
> +		return 0;
> +
> +	for (gpio_i = 0; gpio_i < data->gpiods->ndescs; gpio_i++) {
> +		/*
> +		 * "reset" GPIOs normally have opposite polarity compared to
> +		 * "enable" GPIOs. Instead of parsing the flags again, simply
> +		 * set the raw value to high.
> +		 */
> +		dev_dbg(dev, "Setting GPIO %d\n", gpio_i);
> +		ret = gpiod_direction_output_raw(data->gpiods->desc[gpio_i], 1);
> +		if (ret)
> +			goto disable_gpios;
> +	}
> +
> +	msleep(data->opts->post_reset_deassert_delay_ms);
> +
> +	return 0;
> +
> +disable_gpios:

> +	for (gpio_i--; gpio_i >= 0; gpio_i--)

	while (i--)

> +		gpiod_set_raw_value_cansleep(data->gpiods->desc[gpio_i], 0);
> +
> +	return ret;
> +}


-- 
With Best Regards,
Andy Shevchenko