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
