drivers/gpio/gpiolib.c | 84 +++++++++++++++++++++++++++----------------------- 1 file changed, 46 insertions(+), 38 deletions(-)
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@linux-m68k.org>
Closes: https://lore.kernel.org/all/CAMuHMdX6RuZXAozrF5m625ZepJTVVr4pcyKczSk12MedWvoejw@mail.gmail.com/
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
drivers/gpio/gpiolib.c | 84 +++++++++++++++++++++++++++-----------------------
1 file changed, 46 insertions(+), 38 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 7ee45ccdf1265e94b68fe8fa0d10dbc6adccd93a..fd2821463ddc9fe047fecb3d89b018b5e23b925c 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -938,12 +938,12 @@ 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;
int ret, argc;
- u32 gpios[3]; /* We support up to three-cell bindings. */
- u32 cells;
+ u32 cells = 0;
lflags = GPIO_LOOKUP_FLAGS_DEFAULT;
dflags = GPIOD_ASIS;
@@ -952,43 +952,21 @@ 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)
+
+ ret = fwnode_property_read_u32(gc_node, "#gpio-cells", &cells);
+ if ((ret && is_of_node(fwnode)) || (!ret && (argc % cells)))
return -EINVAL;
+ num_hogs = ret ? 1 : (argc / cells);
+
+ u32 *gpios __free(kfree) = kcalloc(argc, sizeof(u32), GFP_KERNEL);
+ if (!gpios)
+ return -ENOMEM;
+
ret = fwnode_property_read_u32_array(fwnode, "gpios", gpios, argc);
if (ret < 0)
return ret;
- 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;
-
- for (int i = 0; i < argc; i++)
- gpiospec.args[i] = gpios[i];
-
- 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;
- }
-
if (fwnode_property_present(fwnode, "input"))
dflags |= GPIOD_IN;
else if (fwnode_property_present(fwnode, "output-low"))
@@ -1000,11 +978,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++)
+ 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;
+ }
- return gpiod_hog(desc, name, lflags, dflags);
+ desc = gpiochip_get_desc(gc, gpios[i * cells]);
+ if (IS_ERR(desc))
+ return PTR_ERR(desc);
+
+ ret = gpiod_hog(desc, name, lflags, dflags);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
}
static int gpiochip_hog_lines(struct gpio_chip *gc)
---
base-commit: 04310bd443d4a09a486c0bcd7a767e32400eca77
change-id: 20260325-gpio-hogs-multiple-1718d38c1f5b
Best regards,
--
Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>Hi Bartosz,
On Wed, 25 Mar 2026 at 11:18, Bartosz Golaszewski
<bartosz.golaszewski@oss.qualcomm.com> 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@linux-m68k.org>
> Closes: https://lore.kernel.org/all/CAMuHMdX6RuZXAozrF5m625ZepJTVVr4pcyKczSk12MedWvoejw@mail.gmail.com/
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
Thanks, that fixes the issue I saw on Marzen
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -938,12 +938,12 @@ 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;
> int ret, argc;
> - u32 gpios[3]; /* We support up to three-cell bindings. */
> - u32 cells;
> + u32 cells = 0;
Shouldn't the default be 2?
>
> lflags = GPIO_LOOKUP_FLAGS_DEFAULT;
> dflags = GPIOD_ASIS;
> @@ -952,43 +952,21 @@ 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)
> +
> + ret = fwnode_property_read_u32(gc_node, "#gpio-cells", &cells);
Should this call be protected by is_of_node(), like in the old code?
> + if ((ret && is_of_node(fwnode)) || (!ret && (argc % cells)))
> return -EINVAL;
>
> + num_hogs = ret ? 1 : (argc / cells);
While you avoid modulo and division by zero due to the check for ret,
using a default would let you always validate argc against cells,
and remove the need for special casing num_hogs.
> +
> + u32 *gpios __free(kfree) = kcalloc(argc, sizeof(u32), GFP_KERNEL);
kzalloc_objs()?
> + if (!gpios)
> + return -ENOMEM;
> +
> ret = fwnode_property_read_u32_array(fwnode, "gpios", gpios, argc);
> if (ret < 0)
> return ret;
>
> - 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;
> -
> - for (int i = 0; i < argc; i++)
> - gpiospec.args[i] = gpios[i];
> -
> - 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;
> - }
> -
> if (fwnode_property_present(fwnode, "input"))
> dflags |= GPIOD_IN;
> else if (fwnode_property_present(fwnode, "output-low"))
> @@ -1000,11 +978,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++)
> + 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])
This depends on cells being non-zero, and thus having a proper default,
too.
> + lflags |= GPIO_ACTIVE_LOW;
> + }
>
> - return gpiod_hog(desc, name, lflags, dflags);
> + desc = gpiochip_get_desc(gc, gpios[i * cells]);
> + if (IS_ERR(desc))
> + return PTR_ERR(desc);
> +
> + 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@linux-m68k.org
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
On Wed, Mar 25, 2026 at 7:42 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Bartosz,
>
> On Wed, 25 Mar 2026 at 11:18, Bartosz Golaszewski
> <bartosz.golaszewski@oss.qualcomm.com> 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@linux-m68k.org>
> > Closes: https://lore.kernel.org/all/CAMuHMdX6RuZXAozrF5m625ZepJTVVr4pcyKczSk12MedWvoejw@mail.gmail.com/
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
>
> Thanks, that fixes the issue I saw on Marzen
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -938,12 +938,12 @@ 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;
> > int ret, argc;
> > - u32 gpios[3]; /* We support up to three-cell bindings. */
> > - u32 cells;
> > + u32 cells = 0;
>
> Shouldn't the default be 2?
>
For DT I suppose there's no real "default" but it does make sense to
set it to 2 for the generic fwnode path.
> >
> > lflags = GPIO_LOOKUP_FLAGS_DEFAULT;
> > dflags = GPIOD_ASIS;
> > @@ -952,43 +952,21 @@ 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)
> > +
> > + ret = fwnode_property_read_u32(gc_node, "#gpio-cells", &cells);
>
> Should this call be protected by is_of_node(), like in the old code?
>
Not necessarily. We may end up using it with software nodes for
instance. I'd keep it global.
> > + if ((ret && is_of_node(fwnode)) || (!ret && (argc % cells)))
> > return -EINVAL;
> >
> > + num_hogs = ret ? 1 : (argc / cells);
>
> While you avoid modulo and division by zero due to the check for ret,
> using a default would let you always validate argc against cells,
> and remove the need for special casing num_hogs.
>
> > +
> > + u32 *gpios __free(kfree) = kcalloc(argc, sizeof(u32), GFP_KERNEL);
>
> kzalloc_objs()?
>
True.
Bart
On Wed, Mar 25, 2026 at 11:18:32AM +0100, Bartosz Golaszewski 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.
...
> + ret = fwnode_property_read_u32(gc_node, "#gpio-cells", &cells);
> + if ((ret && is_of_node(fwnode)) || (!ret && (argc % cells)))
> return -EINVAL;
Hmm... Shouldn't we return the proper error code in the first case?
if (ret && is_of_node(fwnode))
return ret;
if (!ret && (argc % cells))
return -EINVAL;
...
> + u32 *gpios __free(kfree) = kcalloc(argc, sizeof(u32), GFP_KERNEL);
sizeof(*gpios)
> + if (!gpios)
> + return -ENOMEM;
--
With Best Regards,
Andy Shevchenko
© 2016 - 2026 Red Hat, Inc.