[PATCH] pinctrl: single: parse gpio-range as a raw array (O(N^2) -> O(N))

[Simon Glass]

Hi Jordi,

On 2026-06-14T10:32:32, Jordi Trepat Mur <yordy1902 at gmail.com> wrote:
> pinctrl: single: parse gpio-range as a raw array (O(N^2) -> O(N))
>
> single_add_gpio_func() calls ofnode_parse_phandle_with_args() once per
> gpio-range entry. With a flat device tree, every call re-iterates the
> property from index 0 and, because cellname is set, performs an
> fdt_node_offset_by_phandle() (a full-FDT scan) at every step. The total
> cost is therefore quadratic in the number of entries and proportional
> to the size of the device tree.
>
> On a TI J722S board this makes the pinctrl probe take 723 ms
> pre-relocation (caches off) for only 7 entries; with this change it
> takes 22 ms (-97%). Any pinctrl-single user with a populated
> pinctrl-single,gpio-range property pays this cost.
>
> Read the property once with ofnode_get_property() and iterate over the
> raw cells in memory instead. The phandle target node is never used by
> this function (only the three argument cells are stored), so resolve
> only the first entry with ofnode_parse_phandle_with_args() to validate
> the cell layout. Empty entries (phandle == 0) are skipped, matching the
> previous behaviour.
> [...]
>
> drivers/pinctrl/pinctrl-single.c | 62 +++++++++++++++++++++++++++++++---------
>  1 file changed, 48 insertions(+), 14 deletions(-)

> diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
> @@ -511,27 +511,61 @@ static int single_add_gpio_func(struct udevice *dev)
> +             if (phandle_i == 0) {
> +                     /* empty entry - match original: skip without creating range */
> +                     continue;
> +             }

Not quite. ofnode_parse_phandle_with_args() returns an error for a
zero phandle, and the old loop treats any error as end-of-list and
breaks. So phandle == 0 used to terminate the scan; now it is silently
skipped. Either restore the break, or call out the behaviour change in
the commit message and update the comment.

> diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
> @@ -511,27 +511,61 @@ static int single_add_gpio_func(struct udevice *dev)
> +     if (gpiospec.args_count != 3) {
> +             dev_err(dev, "expected 3 gpio-range args/entry, got %d\n",
> +                     gpiospec.args_count);
> +             return -EINVAL;
> +     }
> +     cells_per_entry = 1 + gpiospec.args_count;      /* phandle + args */
> +     if (total_cells % cells_per_entry) {
> +             dev_err(dev, "gpio-range size %d cells not multiple of %d\n",
> +                     total_cells, cells_per_entry);
> +             return -EINVAL;
> +     }

These two turn a silent no-op (return 0) into a hard probe failure.
args_count is already pinned by #pinctrl-single,gpio-range-cells, and
the size-mismatch case did not exist as an error path before. Please
either keep the lenient behaviour (warn and return 0) or justify the
stricter validation in the commit message.

> diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
> @@ -511,27 +511,61 @@ static int single_add_gpio_func(struct udevice *dev)
> +     const __be32 *list;
> +     int size, total_cells, cells_per_entry, entries, ret, i;

Did you consider ofnode_read_u32_array() (or reading into a small u32
buffer) instead of open-coding the __be32 walk with be32_to_cpup() ?

> diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
> @@ -511,27 +511,61 @@ static int single_add_gpio_func(struct udevice *dev)
> +     int ret_total = 0;
...
> -                     ret = -ENOMEM;
> +                     ret_total = -ENOMEM;
>                       break;

The ret/ret_total split is unnecessary - by the time you enter the
loop the earlier ret has been consumed. Just reuse ret.

> diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
> @@ -511,27 +511,61 @@ static int single_add_gpio_func(struct udevice *dev)
> +             range->offset   = be32_to_cpup(entry + 1);
> +             range->npins    = be32_to_cpup(entry + 2);
> +             range->gpiofunc = be32_to_cpup(entry + 3);

Please drop the extra spacing - single space after the field name, as
in the rest of the file.

+1 to Anshui's suggestion of avoiding alloc. You can create a
variable-length local var, for example.

Is every entry in the 'propname' property always for the same device?
I might be a bit confused, but I am not sure where that is required in
the schema.

Regards,
Simon