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

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