Re: [PATCH v2] gpiolib: fix hogs with multiple lines

[Geert Uytterhoeven]

Hi Bartosz,

On Thu, 26 Mar 2026 at 15:18, Bartosz Golaszewski
<bartosz.golaszewski@xxxxxxxxxxxxxxxx> wrote:
> After moving GPIO hog handling into GPIOLIB core, we accidentally stopped
> supporting devicetree hog definitions with multiple lines like so:
>
>         hog {
>                 gpio-hog;
>                 gpios = <3 0>, <4 GPIO_ACTIVE_LOW>;
>                 output-high;
>                 line-name = "foo";
>         };
>
> Restore this functionality to fix reported regressions.
>
> Fixes: d1d564ec4992 ("gpio: move hogs into GPIO core")
> Reported-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Closes: https://lore.kernel.org/all/CAMuHMdX6RuZXAozrF5m625ZepJTVVr4pcyKczSk12MedWvoejw@xxxxxxxxxxxxxx/
> Tested-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxxxxxxxx>
> ---
> Changes in v2:
> - Use a default value of 2 for the number of GPIO cells
> - Use kzalloc_objs() instead of kcalloc()
> - Propagate the error number from fwnode_property_read_u32()
> - Remove special cases forced by the lack of default cells value
> - Link to v1: https://patch.msgid.link/20260325-gpio-hogs-multiple-v1-1-7e3a7347f0d6@xxxxxxxxxxxxxxxx

Thanks for the update!

> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -938,12 +938,18 @@ int gpiochip_add_hog(struct gpio_chip *gc, struct fwnode_handle *fwnode)
>         struct fwnode_handle *gc_node = dev_fwnode(&gc->gpiodev->dev);
>         struct fwnode_reference_args gpiospec;
>         enum gpiod_flags dflags;
> +       const char *name = NULL;
>         struct gpio_desc *desc;
>         unsigned long lflags;
> -       const char *name;
> +       size_t num_hogs;

unsigned int should be sufficient to store the quotient of two positive
32-bit numbers.

>         int ret, argc;
> -       u32 gpios[3]; /* We support up to three-cell bindings. */
> -       u32 cells;
> +       /*
> +        * For devicetree-based systems, this needs to be defined in bindings
> +        * and there's no real default value. For other firmware descriptions
> +        * it makes the most sense to use 2 cells for the GPIO offset and
> +        * request flags.
> +        */
> +       u32 cells = 2;
>
>         lflags = GPIO_LOOKUP_FLAGS_DEFAULT;
>         dflags = GPIOD_ASIS;
> @@ -952,42 +958,22 @@ int gpiochip_add_hog(struct gpio_chip *gc, struct fwnode_handle *fwnode)
>         argc = fwnode_property_count_u32(fwnode, "gpios");
>         if (argc < 0)
>                 return argc;
> -       if (argc > 3)
> -               return -EINVAL;
>
> -       ret = fwnode_property_read_u32_array(fwnode, "gpios", gpios, argc);
> -       if (ret < 0)
> +       ret = fwnode_property_read_u32(gc_node, "#gpio-cells", &cells);
> +       if (ret && is_of_node(fwnode))
>                 return ret;
> +       if (!ret && (argc % cells))

No need to check for !ret, the %-test should be valid for non-DT, too.

I guess we can't have #gpio-cells = <0> (single-GPIO provider not
supporting flags), and it would be very difficult to hog it.

> +               return -EINVAL;
>
> -       if (is_of_node(fwnode)) {
> -               /*
> -                * OF-nodes need some additional special handling for
> -                * translating of devicetree flags.
> -                */
> -               ret = fwnode_property_read_u32(gc_node, "#gpio-cells", &cells);
> -               if (ret)
> -                       return ret;
> -               if (!ret && argc != cells)
> -                       return -EINVAL;
> -
> -               memset(&gpiospec, 0, sizeof(gpiospec));
> -               gpiospec.fwnode = fwnode;
> -               gpiospec.nargs = argc;
> +       num_hogs = argc / cells;
>
> -               for (int i = 0; i < argc; i++)
> -                       gpiospec.args[i] = gpios[i];
> +       u32 *gpios __free(kfree) = kzalloc_objs(*gpios, argc);
> +       if (!gpios)
> +               return -ENOMEM;
>
> -               ret = of_gpiochip_get_lflags(gc, &gpiospec, &lflags);
> -               if (ret)
> -                       return ret;
> -       } else {
> -               /*
> -                * GPIO_ACTIVE_LOW is currently the only lookup flag
> -                * supported for non-OF firmware nodes.
> -                */
> -               if (gpios[1])
> -                       lflags |= GPIO_ACTIVE_LOW;
> -       }
> +       ret = fwnode_property_read_u32_array(fwnode, "gpios", gpios, argc);
> +       if (ret < 0)
> +               return ret;
>
>         if (fwnode_property_present(fwnode, "input"))
>                 dflags |= GPIOD_IN;
> @@ -1000,11 +986,41 @@ int gpiochip_add_hog(struct gpio_chip *gc, struct fwnode_handle *fwnode)
>
>         fwnode_property_read_string(fwnode, "line-name", &name);
>
> -       desc = gpiochip_get_desc(gc, gpios[0]);
> -       if (IS_ERR(desc))
> -               return PTR_ERR(desc);
> +       for (unsigned int i = 0; i < num_hogs; i++) {
> +               if (is_of_node(fwnode)) {
> +                       /*
> +                        * OF-nodes need some additional special handling for
> +                        * translating of devicetree flags.
> +                        */
> +                       memset(&gpiospec, 0, sizeof(gpiospec));
> +                       gpiospec.fwnode = fwnode;
> +                       gpiospec.nargs = cells;
> +
> +                       for (int j = 0; j < cells; j++)

unsigned int

> +                               gpiospec.args[j] = gpios[i * cells + j];
> +
> +                       ret = of_gpiochip_get_lflags(gc, &gpiospec, &lflags);
> +                       if (ret)
> +                               return ret;
> +               } else {
> +                       /*
> +                        * GPIO_ACTIVE_LOW is currently the only lookup flag
> +                        * supported for non-OF firmware nodes.
> +                        */
> +                       if (gpios[i * cells + 1])
> +                               lflags |= GPIO_ACTIVE_LOW;
> +               }
> +
> +               desc = gpiochip_get_desc(gc, gpios[i * cells]);
> +               if (IS_ERR(desc))
> +                       return PTR_ERR(desc);
>
> -       return gpiod_hog(desc, name, lflags, dflags);
> +               ret = gpiod_hog(desc, name, lflags, dflags);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       return 0;
>  }
>
>  static int gpiochip_hog_lines(struct gpio_chip *gc)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds