| Message ID | 20250610-gpiochip-set-rv-gpio-v1-1-3a9a3c1472ff@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: convert another round of GPIO drivers to using new line value setters | expand |
On Tue, Jun 10, 2025 at 2:33 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > struct gpio_chip now has callbacks for setting line values that return > an integer, allowing to indicate failures. Convert the driver to using > them. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
Hi, On 2025-06-10 14:33:11 +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > struct gpio_chip now has callbacks for setting line values that return > an integer, allowing to indicate failures. Convert the driver to using > them. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > --- > drivers/gpio/gpio-mmio.c | 53 ++++++++++++++++++++++++++++++------------------ > 1 file changed, 33 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c > index 4841e4ebe7a67d0f954e9a6f995ec5758f124edd..9169eccadb238efe944d494054b1e009f16eee7f 100644 > --- a/drivers/gpio/gpio-mmio.c > +++ b/drivers/gpio/gpio-mmio.c > @@ -211,11 +211,12 @@ static int bgpio_get_multiple_be(struct gpio_chip *gc, unsigned long *mask, > return 0; > } > > -static void bgpio_set_none(struct gpio_chip *gc, unsigned int gpio, int val) > +static int bgpio_set_none(struct gpio_chip *gc, unsigned int gpio, int val) > { > + return 0; > } > > -static void bgpio_set(struct gpio_chip *gc, unsigned int gpio, int val) > +static int bgpio_set(struct gpio_chip *gc, unsigned int gpio, int val) > { > unsigned long mask = bgpio_line2mask(gc, gpio); > unsigned long flags; > @@ -230,10 +231,12 @@ static void bgpio_set(struct gpio_chip *gc, unsigned int gpio, int val) > gc->write_reg(gc->reg_dat, gc->bgpio_data); > > raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags); > + > + return 0; > } > > -static void bgpio_set_with_clear(struct gpio_chip *gc, unsigned int gpio, > - int val) > +static int bgpio_set_with_clear(struct gpio_chip *gc, unsigned int gpio, > + int val) > { > unsigned long mask = bgpio_line2mask(gc, gpio); > > @@ -241,9 +244,11 @@ static void bgpio_set_with_clear(struct gpio_chip *gc, unsigned int gpio, > gc->write_reg(gc->reg_set, mask); > else > gc->write_reg(gc->reg_clr, mask); > + > + return 0; > } > > -static void bgpio_set_set(struct gpio_chip *gc, unsigned int gpio, int val) > +static int bgpio_set_set(struct gpio_chip *gc, unsigned int gpio, int val) > { > unsigned long mask = bgpio_line2mask(gc, gpio); > unsigned long flags; > @@ -258,6 +263,8 @@ static void bgpio_set_set(struct gpio_chip *gc, unsigned int gpio, int val) > gc->write_reg(gc->reg_set, gc->bgpio_data); > > raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags); > + > + return 0; > } > > static void bgpio_multiple_get_masks(struct gpio_chip *gc, > @@ -298,21 +305,25 @@ static void bgpio_set_multiple_single_reg(struct gpio_chip *gc, > raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags); > } > > -static void bgpio_set_multiple(struct gpio_chip *gc, unsigned long *mask, > +static int bgpio_set_multiple(struct gpio_chip *gc, unsigned long *mask, > unsigned long *bits) > { > bgpio_set_multiple_single_reg(gc, mask, bits, gc->reg_dat); > + > + return 0; > } > > -static void bgpio_set_multiple_set(struct gpio_chip *gc, unsigned long *mask, > - unsigned long *bits) > +static int bgpio_set_multiple_set(struct gpio_chip *gc, unsigned long *mask, > + unsigned long *bits) > { > bgpio_set_multiple_single_reg(gc, mask, bits, gc->reg_set); > + > + return 0; > } > > -static void bgpio_set_multiple_with_clear(struct gpio_chip *gc, > - unsigned long *mask, > - unsigned long *bits) > +static int bgpio_set_multiple_with_clear(struct gpio_chip *gc, > + unsigned long *mask, > + unsigned long *bits) > { > unsigned long set_mask, clear_mask; > > @@ -322,6 +333,8 @@ static void bgpio_set_multiple_with_clear(struct gpio_chip *gc, > gc->write_reg(gc->reg_set, set_mask); > if (clear_mask) > gc->write_reg(gc->reg_clr, clear_mask); > + > + return 0; > } > > static int bgpio_dir_return(struct gpio_chip *gc, unsigned int gpio, bool dir_out) > @@ -510,18 +523,18 @@ static int bgpio_setup_io(struct gpio_chip *gc, > if (set && clr) { > gc->reg_set = set; > gc->reg_clr = clr; > - gc->set = bgpio_set_with_clear; > - gc->set_multiple = bgpio_set_multiple_with_clear; > + gc->set_rv = bgpio_set_with_clear; > + gc->set_multiple_rv = bgpio_set_multiple_with_clear; > } else if (set && !clr) { > gc->reg_set = set; > - gc->set = bgpio_set_set; > - gc->set_multiple = bgpio_set_multiple_set; > + gc->set_rv = bgpio_set_set; > + gc->set_multiple_rv = bgpio_set_multiple_set; > } else if (flags & BGPIOF_NO_OUTPUT) { > - gc->set = bgpio_set_none; > - gc->set_multiple = NULL; > + gc->set_rv = bgpio_set_none; > + gc->set_multiple_rv = NULL; > } else { > - gc->set = bgpio_set; > - gc->set_multiple = bgpio_set_multiple; > + gc->set_rv = bgpio_set; > + gc->set_multiple_rv = bgpio_set_multiple; > } > > if (!(flags & BGPIOF_UNREADABLE_REG_SET) && > @@ -654,7 +667,7 @@ int bgpio_init(struct gpio_chip *gc, struct device *dev, > } > > gc->bgpio_data = gc->read_reg(gc->reg_dat); > - if (gc->set == bgpio_set_set && > + if (gc->set_rv == bgpio_set_set && > !(flags & BGPIOF_UNREADABLE_REG_SET)) > gc->bgpio_data = gc->read_reg(gc->reg_set); > > > -- > 2.48.1 > Isn't this missing to convert gc->set() to gc-set_rv() in several places? Without the attached diff I get a null pointer reference on e.g. the spacemit k1 driver. Regards, Klara Modin -- diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c index 9169eccadb23..57622f45d33e 100644 --- a/drivers/gpio/gpio-mmio.c +++ b/drivers/gpio/gpio-mmio.c @@ -362,7 +362,7 @@ static int bgpio_dir_out_err(struct gpio_chip *gc, unsigned int gpio, static int bgpio_simple_dir_out(struct gpio_chip *gc, unsigned int gpio, int val) { - gc->set(gc, gpio, val); + gc->set_rv(gc, gpio, val); return bgpio_dir_return(gc, gpio, true); } @@ -427,14 +427,14 @@ static int bgpio_dir_out_dir_first(struct gpio_chip *gc, unsigned int gpio, int val) { bgpio_dir_out(gc, gpio, val); - gc->set(gc, gpio, val); + gc->set_rv(gc, gpio, val); return bgpio_dir_return(gc, gpio, true); } static int bgpio_dir_out_val_first(struct gpio_chip *gc, unsigned int gpio, int val) { - gc->set(gc, gpio, val); + gc->set_rv(gc, gpio, val); bgpio_dir_out(gc, gpio, val); return bgpio_dir_return(gc, gpio, true); }
On Wed, Jun 18, 2025 at 6:21 PM Mark Brown <broonie@kernel.org> wrote: > > On Tue, Jun 10, 2025 at 02:33:11PM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > struct gpio_chip now has callbacks for setting line values that return > > an integer, allowing to indicate failures. Convert the driver to using > > them. > > I'm seeing boot failures on a UDOOq (an i.MX6 based board) in -next > today which bisect to this patch (in -next as b908d35d0003cc7). We get > a NULL pointer dereference during boot while probing the poweroff driver > for the system: > > [ 0.443319] Unable to handle kernel NULL pointer dereference at virtual address 00000000 when execute > [ 0.443330] [00000000] *pgd=00000000 > [ 0.443347] Internal error: Oops: 80000005 [#2] SMP ARM > > ... > > [ 2.522761] bgpio_dir_out_val_first from gpiod_direction_output_raw_commit+0x194/0x390 > [ 2.533330] gpiod_direction_output_raw_commit from gpiod_find_and_request+0x134/0x434 > [ 2.541276] gpiod_find_and_request from gpiod_get_index+0x58/0x70 > [ 2.547482] gpiod_get_index from devm_gpiod_get_index+0x10/0x78 > [ 2.553516] devm_gpiod_get_index from gpio_poweroff_probe+0xe8/0x174 > [ 2.559990] gpio_poweroff_probe from platform_probe+0x5c/0xb4 > Thanks, a patch[1] is already up for review. Please give it a try and leave your Tested-by: if you can. Bartosz [1] https://lore.kernel.org/all/20250618-gpio-mmio-fix-setter-v1-2-2578ffb77019@linaro.org/
diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c index 4841e4ebe7a67d0f954e9a6f995ec5758f124edd..9169eccadb238efe944d494054b1e009f16eee7f 100644 --- a/drivers/gpio/gpio-mmio.c +++ b/drivers/gpio/gpio-mmio.c @@ -211,11 +211,12 @@ static int bgpio_get_multiple_be(struct gpio_chip *gc, unsigned long *mask, return 0; } -static void bgpio_set_none(struct gpio_chip *gc, unsigned int gpio, int val) +static int bgpio_set_none(struct gpio_chip *gc, unsigned int gpio, int val) { + return 0; } -static void bgpio_set(struct gpio_chip *gc, unsigned int gpio, int val) +static int bgpio_set(struct gpio_chip *gc, unsigned int gpio, int val) { unsigned long mask = bgpio_line2mask(gc, gpio); unsigned long flags; @@ -230,10 +231,12 @@ static void bgpio_set(struct gpio_chip *gc, unsigned int gpio, int val) gc->write_reg(gc->reg_dat, gc->bgpio_data); raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags); + + return 0; } -static void bgpio_set_with_clear(struct gpio_chip *gc, unsigned int gpio, - int val) +static int bgpio_set_with_clear(struct gpio_chip *gc, unsigned int gpio, + int val) { unsigned long mask = bgpio_line2mask(gc, gpio); @@ -241,9 +244,11 @@ static void bgpio_set_with_clear(struct gpio_chip *gc, unsigned int gpio, gc->write_reg(gc->reg_set, mask); else gc->write_reg(gc->reg_clr, mask); + + return 0; } -static void bgpio_set_set(struct gpio_chip *gc, unsigned int gpio, int val) +static int bgpio_set_set(struct gpio_chip *gc, unsigned int gpio, int val) { unsigned long mask = bgpio_line2mask(gc, gpio); unsigned long flags; @@ -258,6 +263,8 @@ static void bgpio_set_set(struct gpio_chip *gc, unsigned int gpio, int val) gc->write_reg(gc->reg_set, gc->bgpio_data); raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags); + + return 0; } static void bgpio_multiple_get_masks(struct gpio_chip *gc, @@ -298,21 +305,25 @@ static void bgpio_set_multiple_single_reg(struct gpio_chip *gc, raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags); } -static void bgpio_set_multiple(struct gpio_chip *gc, unsigned long *mask, +static int bgpio_set_multiple(struct gpio_chip *gc, unsigned long *mask, unsigned long *bits) { bgpio_set_multiple_single_reg(gc, mask, bits, gc->reg_dat); + + return 0; } -static void bgpio_set_multiple_set(struct gpio_chip *gc, unsigned long *mask, - unsigned long *bits) +static int bgpio_set_multiple_set(struct gpio_chip *gc, unsigned long *mask, + unsigned long *bits) { bgpio_set_multiple_single_reg(gc, mask, bits, gc->reg_set); + + return 0; } -static void bgpio_set_multiple_with_clear(struct gpio_chip *gc, - unsigned long *mask, - unsigned long *bits) +static int bgpio_set_multiple_with_clear(struct gpio_chip *gc, + unsigned long *mask, + unsigned long *bits) { unsigned long set_mask, clear_mask; @@ -322,6 +333,8 @@ static void bgpio_set_multiple_with_clear(struct gpio_chip *gc, gc->write_reg(gc->reg_set, set_mask); if (clear_mask) gc->write_reg(gc->reg_clr, clear_mask); + + return 0; } static int bgpio_dir_return(struct gpio_chip *gc, unsigned int gpio, bool dir_out) @@ -510,18 +523,18 @@ static int bgpio_setup_io(struct gpio_chip *gc, if (set && clr) { gc->reg_set = set; gc->reg_clr = clr; - gc->set = bgpio_set_with_clear; - gc->set_multiple = bgpio_set_multiple_with_clear; + gc->set_rv = bgpio_set_with_clear; + gc->set_multiple_rv = bgpio_set_multiple_with_clear; } else if (set && !clr) { gc->reg_set = set; - gc->set = bgpio_set_set; - gc->set_multiple = bgpio_set_multiple_set; + gc->set_rv = bgpio_set_set; + gc->set_multiple_rv = bgpio_set_multiple_set; } else if (flags & BGPIOF_NO_OUTPUT) { - gc->set = bgpio_set_none; - gc->set_multiple = NULL; + gc->set_rv = bgpio_set_none; + gc->set_multiple_rv = NULL; } else { - gc->set = bgpio_set; - gc->set_multiple = bgpio_set_multiple; + gc->set_rv = bgpio_set; + gc->set_multiple_rv = bgpio_set_multiple; } if (!(flags & BGPIOF_UNREADABLE_REG_SET) && @@ -654,7 +667,7 @@ int bgpio_init(struct gpio_chip *gc, struct device *dev, } gc->bgpio_data = gc->read_reg(gc->reg_dat); - if (gc->set == bgpio_set_set && + if (gc->set_rv == bgpio_set_set && !(flags & BGPIOF_UNREADABLE_REG_SET)) gc->bgpio_data = gc->read_reg(gc->reg_set);