Re: [PATCH v5 08/10] i2c: of-prober: Add GPIO support

On Thu, Aug 22, 2024 at 05:20:01PM +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.

...

+	struct fwnode_handle *fwnode = of_fwnode_handle(node);
+	struct gpio_descs *gpiods;
+	struct gpio_desc *gpiod;
+	char con[32]; /* 32 is max size of property name */

Use 'propname' to be aligned with GPIO library usages.

+	char *con_id = NULL;
+	size_t new_size;
+	int len;

...

+	if (len >= sizeof(con) - 1) {

This can be transformed to check the returned value from strscpy().

+		pr_err("%pOF: length of GPIO name \"%s\" exceeds current limit\n",
+		       node, prop->name);
+		return -EINVAL;
+	}
+
+	if (len > 0) {
+		strscpy(con, prop->name, len + 1);

The correct (robust) call is with destination size. Which means here that you
may use 2-argument strscpy().

+		con_id = con;
+	}

...

+	if (!data->gpiods)
+		return 0;

If it comes a new code (something else besides GPIOs and regulators) this will be a (small) impediment. Better to have a helper for each case and do

	ret = ..._gpiods();
	if (ret)
		...

Same for regulators and anything else in the future, if any.

+		/*
+		 * reset GPIOs normally have opposite polarity compared to

"reset"

+		 * enable GPIOs. Instead of parsing the flags again, simply

"enable"

+		 * set the raw value to high.

This is quite a fragile assumption. Yes, it would work in 98% cases, but will
break if it's not true somewhere else.

+		 */

...

+	/* largest post-reset-deassert delay seen in tree for Elan I2C HID */
+	msleep(300);

Same Q, how do you monitor _all_ the drivers?

...

+disable_gpios:
+	for (gpio_i--; gpio_i >= 0; gpio_i--)
+		gpiod_set_raw_value_cansleep(data->gpiods->desc[gpio_i], 0);

Can't you call the _array() variant here?

...

-	dev_dbg(dev, "Resources: # of regulator supplies = %d\n", probe_data.regulators_num);
+	dev_dbg(dev, "Resources: # of GPIOs = %d, # of regulator supplies = %d\n",
+		probe_data.gpiods ? probe_data.gpiods->ndescs : 0,
+		probe_data.regulators_num);

I would issue one message per class of the devices (GPIOs, regulators, ...)

-- 
With Best Regards,
Andy Shevchenko