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
