commit f34fd6ee1be8 breaks current dwapb gpio DT users

commit f34fd6ee1be8 breaks current dwapb gpio DT users

[Jisheng Zhang]

Hi Emil, Linus,

commit f34fd6ee1be8 ("gpio: dwapb: Use generic request, free and
set_config") breaks all current dwapb gpio DT users, for example, getting
cd-gpios will always fail as -EPROBE_DEFER. Before the commit,
dwapb_gpio_set_config() returns -ENOTSUPP for !PIN_CONFIG_INPUT_DEBOUNCE
then gpio_set_config_with_argument_optional() will happily ignore
-ENOTSUPP. After the commit, dwapb_gpio_set_config() will return
-EPROBE_DEFER unless the gpio-ranges are set in DT.

The key problem here is: almost all dwapb gpio DT users don't set
the gpio-ranges DT property, so I guess current dwapb gpio DT users
have this problem and the commit also breaks old DT compatbility.
So could we getback to previous behavior?

Thanks in advance

Re: commit f34fd6ee1be8 breaks current dwapb gpio DT users

[Emil Renner Berthing]

Jisheng Zhang wrote:
> Hi Emil, Linus,
>
> commit f34fd6ee1be8 ("gpio: dwapb: Use generic request, free and
> set_config") breaks all current dwapb gpio DT users, for example, getting
> cd-gpios will always fail as -EPROBE_DEFER. Before the commit,
> dwapb_gpio_set_config() returns -ENOTSUPP for !PIN_CONFIG_INPUT_DEBOUNCE
> then gpio_set_config_with_argument_optional() will happily ignore
> -ENOTSUPP. After the commit, dwapb_gpio_set_config() will return
> -EPROBE_DEFER unless the gpio-ranges are set in DT.
>
> The key problem here is: almost all dwapb gpio DT users don't set
> the gpio-ranges DT property, so I guess current dwapb gpio DT users
> have this problem and the commit also breaks old DT compatbility.
> So could we getback to previous behavior?

Hi Jisheng

It seems like the gpiochip_generic_request() and gpiochip_generic_free()
functions have guards to handle when gpio-ranges are not set that
gpiochip_generic_config() lacks. Could you try the patch below?

Otherwise I'm also fine with just reverting the patch if this is not the right
solution.

/Emil

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 8b3a0f45b574..e434e8cc1229 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2042,6 +2042,11 @@ EXPORT_SYMBOL_GPL(gpiochip_generic_free);
 int gpiochip_generic_config(struct gpio_chip *gc, unsigned int offset,
                            unsigned long config)
 {
+#ifdef CONFIG_PINCTRL
+       if (list_empty(&gc->gpiodev->pin_ranges))
+               return -ENOTSUPP;
+#endif
+
        return pinctrl_gpio_set_config(gc, offset, config);
 }
 EXPORT_SYMBOL_GPL(gpiochip_generic_config);

Re: commit f34fd6ee1be8 breaks current dwapb gpio DT users

[Jisheng Zhang]

On Sat, Feb 17, 2024 at 09:44:27AM -0800, Emil Renner Berthing wrote:
> Jisheng Zhang wrote:
> > Hi Emil, Linus,
> >
> > commit f34fd6ee1be8 ("gpio: dwapb: Use generic request, free and
> > set_config") breaks all current dwapb gpio DT users, for example, getting
> > cd-gpios will always fail as -EPROBE_DEFER. Before the commit,
> > dwapb_gpio_set_config() returns -ENOTSUPP for !PIN_CONFIG_INPUT_DEBOUNCE
> > then gpio_set_config_with_argument_optional() will happily ignore
> > -ENOTSUPP. After the commit, dwapb_gpio_set_config() will return
> > -EPROBE_DEFER unless the gpio-ranges are set in DT.
> >
> > The key problem here is: almost all dwapb gpio DT users don't set
> > the gpio-ranges DT property, so I guess current dwapb gpio DT users
> > have this problem and the commit also breaks old DT compatbility.
> > So could we getback to previous behavior?
> 
> Hi Jisheng
> 
> It seems like the gpiochip_generic_request() and gpiochip_generic_free()
> functions have guards to handle when gpio-ranges are not set that
> gpiochip_generic_config() lacks. Could you try the patch below?
> 
> Otherwise I'm also fine with just reverting the patch if this is not the right
> solution.
> 
> /Emil
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 8b3a0f45b574..e434e8cc1229 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2042,6 +2042,11 @@ EXPORT_SYMBOL_GPL(gpiochip_generic_free);
>  int gpiochip_generic_config(struct gpio_chip *gc, unsigned int offset,
>                             unsigned long config)
>  {
> +#ifdef CONFIG_PINCTRL
> +       if (list_empty(&gc->gpiodev->pin_ranges))
> +               return -ENOTSUPP;
> +#endif
> +
>         return pinctrl_gpio_set_config(gc, offset, config);
>  }
>  EXPORT_SYMBOL_GPL(gpiochip_generic_config);

Hi Emil,

The above patch can fix the gpio issue. So,
Reported-by: Jisheng Zhang <jszhang@kernel.org>
Tested-by: Jisheng Zhang <jszhang@kernel.org>

Thanks

Re: commit f34fd6ee1be8 breaks current dwapb gpio DT users

[Linus Walleij]

On Sat, Feb 17, 2024 at 6:44 PM Emil Renner Berthing
<emil.renner.berthing@canonical.com> wrote:

> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2042,6 +2042,11 @@ EXPORT_SYMBOL_GPL(gpiochip_generic_free);
>  int gpiochip_generic_config(struct gpio_chip *gc, unsigned int offset,
>                             unsigned long config)
>  {
> +#ifdef CONFIG_PINCTRL

Please do this:

if (IS_ENABLED(CONFIG_PINCTRL) && list_empty(&gc->gpiodev->pin_ranges))
...

The ifdef is so ugly.

> +       if (list_empty(&gc->gpiodev->pin_ranges))
> +               return -ENOTSUPP;
> +#endif

That looks like a reasonable fix, I try to wrap my head around if it
would affect
any users but can't figure it out, we have to test.

Can you please send it as a proper patch? With the above fixed:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: commit f34fd6ee1be8 breaks current dwapb gpio DT users

[Emil Renner Berthing]

Linus Walleij wrote:
> On Sat, Feb 17, 2024 at 6:44 PM Emil Renner Berthing
> <emil.renner.berthing@canonical.com> wrote:
>
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -2042,6 +2042,11 @@ EXPORT_SYMBOL_GPL(gpiochip_generic_free);
> >  int gpiochip_generic_config(struct gpio_chip *gc, unsigned int offset,
> >                             unsigned long config)
> >  {
> > +#ifdef CONFIG_PINCTRL
>
> Please do this:
>
> if (IS_ENABLED(CONFIG_PINCTRL) && list_empty(&gc->gpiodev->pin_ranges))
> ...
>
> The ifdef is so ugly.

I agree, but I'm not sure it will work in this case since the pin_ranges
member is only there when CONFIG_PINCTRL=y. That would also explain why
gpiochip_generic_request() and gpiochip_generic_free() use ifdefs.

>
> > +       if (list_empty(&gc->gpiodev->pin_ranges))
> > +               return -ENOTSUPP;
> > +#endif
>
> That looks like a reasonable fix, I try to wrap my head around if it
> would affect
> any users but can't figure it out, we have to test.
>
> Can you please send it as a proper patch? With the above fixed:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Will do, thanks!

/Emil