Re: [PATCH RFC 1/6] ARM: pxa: Convert Spitz OHCI to GPIO descriptors

Re: [PATCH RFC 1/6] ARM: pxa: Convert Spitz OHCI to GPIO descriptors

[Andy Shevchenko]

On Sun, Sep 24, 2023 at 06:42:54PM +0200, Duje Mihanović wrote:
> Sharp's Spitz board still uses the legacy GPIO interface for controlling
> a GPIO pin related to the USB host controller.
> 
> Convert this function to use the new GPIO descriptor interface.

...

> +	pxa_ohci->usb_host = gpiod_get(&pdev->dev, "usb-host", GPIOD_OUT_LOW);
> +	if (IS_ERR(pxa_ohci->usb_host)) {
> +		dev_warn(&pdev->dev, "failed to get USB host GPIO with %d\n",
> +				(int) pxa_ohci->usb_host);

Casting is no go in 99.9% cases in printf(), so use proper specifier.
Hint: Nice looking message can be obtained by using %pe.

> +		pxa_ohci->usb_host = NULL;

Instead, call for _optional() API.

> +	}

...

> +	if (pxa_ohci->usb_host)
> +		gpiod_put(pxa_ohci->usb_host);

Linus, Bart, do we have misdesigned _optinal() GPIO APIs?

In GPIOLIB=n, the above requires that redundant check. Shouldn't we replace
gpiod_put() stub to be simply no-op?

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC 4/6] ARM: pxa: Convert reset driver to GPIO descriptors

[Andy Shevchenko]

On Sun, Sep 24, 2023 at 06:42:57PM +0200, Duje Mihanović wrote:
> The PXA reset driver still uses the legacy GPIO interface for
> configuring and asserting the reset pin.
> 
> Convert it to use the GPIO descriptor interface.

...

> +	reset_gpio = gpiod_get(NULL, "reset generator", GPIOD_ASIS);
> +	if (IS_ERR(reset_gpio)) {
> +		printk(KERN_ERR "Can't request reset_gpio: %ld\n",
> +				PTR_ERR(reset_gpio));

pr_err()

> +		return PTR_ERR(reset_gpio);
>  	}

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC 5/6] ARM: pxa: Convert Spitz hsync to GPIO descriptors

[Andy Shevchenko]

On Sun, Sep 24, 2023 at 06:42:58PM +0200, Duje Mihanović wrote:
> Sharp's Spitz still uses the legacy GPIO interface in its wait_for_hsync

We refer to the functions as wait_for_hsync().

> function.
> 
> Convert it to use the GPIO descriptor interface.

...

> +	hsync = gpiod_get(NULL, "hsync", GPIOD_IN);
> +	if (IS_ERR(hsync))
> +		pr_err("Failed to get hsync GPIO: %ld\n", PTR_ERR(hsync));

So, I didn't get, is this GPIO crucial for functioning or optional?
If the former, we need to stop here or bail out (if possible), do we?
Or should use _optional() API?

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC 0/6] ARM: pxa: GPIO descriptor conversions

[Andy Shevchenko]

On Sun, Sep 24, 2023 at 06:42:53PM +0200, Duje Mihanović wrote:
> Hello,
> 
> Small series to convert some of the board files in the mach-pxa directory
> to use the new GPIO descriptor interface.
> 
> Most notably, the am200epd, am300epd and Spitz matrix keypad among
> others are not converted in this series.

Log waited clean up, thank you for doing it!
My comments are placed per individual patches.

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC 1/6] ARM: pxa: Convert Spitz OHCI to GPIO descriptors

[Linus Walleij]

On Mon, Sep 25, 2023 at 9:30 AM Andy Shevchenko <andy@kernel.org> wrote:

> > +     if (pxa_ohci->usb_host)
> > +             gpiod_put(pxa_ohci->usb_host);
>
> Linus, Bart, do we have misdesigned _optinal() GPIO APIs?
>
> In GPIOLIB=n, the above requires that redundant check. Shouldn't we replace
> gpiod_put() stub to be simply no-op?

You mean the WARN_ON(desc) in gpiod_put() in the static inline
stub version?

I thought about it for a bit, drafted a patch removing them, and then
realized the following:

If someone is making the gpiolib optional for a driver, i.e. neither
DEPENDS ON GPIOLIB nor SELECT GPIOLIB, they are a quite
narrow segment. I would say in 9 cases out of 10 or more this is
just a driver that should depend on or select GPIOLIB.

I think such drivers should actually do the NULL checks and not be
too convenient, the reason is readability: someone reading that
driver will be thinking gpios are not optional if they can call
gpiod_set_value(), gpiod_put() etc without any sign that the
desc is optional.

If the driver uses [devm_]gpiod_get_optional() the library is not
using the stubs and does the right thing, and it is clear that
the GPIO is *runtime* optional.

But *compile time* optional, *combined* with runtime optional -
I'm not so happy if we try to avoid warnings around that. I think
it leads to confusing configs and code that looks like gpiolib is
around despite it wasn't selected.

If the code isn't depending on or selecting GPIOLIB and still
use _optional() calls, it better be ready to do some extra checks,
because this is a weird combo, it can't be common.

Could be a documentation update making this clear though.

What do you other people think?

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC 1/6] ARM: pxa: Convert Spitz OHCI to GPIO descriptors

[Andy Shevchenko]

On Wed, Sep 27, 2023 at 04:01:58PM +0200, Linus Walleij wrote:
> On Mon, Sep 25, 2023 at 9:30 AM Andy Shevchenko <andy@kernel.org> wrote:

...

> > > +     if (pxa_ohci->usb_host)
> > > +             gpiod_put(pxa_ohci->usb_host);
> >
> > Linus, Bart, do we have misdesigned _optinal() GPIO APIs?
> >
> > In GPIOLIB=n, the above requires that redundant check. Shouldn't we replace
> > gpiod_put() stub to be simply no-op?
> 
> You mean the WARN_ON(desc) in gpiod_put() in the static inline
> stub version?
> 
> I thought about it for a bit, drafted a patch removing them, and then
> realized the following:
> 
> If someone is making the gpiolib optional for a driver, i.e. neither
> DEPENDS ON GPIOLIB nor SELECT GPIOLIB, they are a quite
> narrow segment. I would say in 9 cases out of 10 or more this is
> just a driver that should depend on or select GPIOLIB.
> 
> I think such drivers should actually do the NULL checks and not be
> too convenient, the reason is readability: someone reading that
> driver will be thinking gpios are not optional if they can call
> gpiod_set_value(), gpiod_put() etc without any sign that the
> desc is optional.
> 
> If the driver uses [devm_]gpiod_get_optional() the library is not
> using the stubs and does the right thing, and it is clear that
> the GPIO is *runtime* optional.
> 
> But *compile time* optional, *combined* with runtime optional -
> I'm not so happy if we try to avoid warnings around that. I think
> it leads to confusing configs and code that looks like gpiolib is
> around despite it wasn't selected.
> 
> If the code isn't depending on or selecting GPIOLIB and still
> use _optional() calls, it better be ready to do some extra checks,
> because this is a weird combo, it can't be common.
> 
> Could be a documentation update making this clear though.
> 
> What do you other people think?

The problem here indeed if the code is not selecting or being dependent on
GPIOLIB and uses _optional() calls.

I agree that this is quite a niche that should be addressed on the driver side.

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC 1/6] ARM: pxa: Convert Spitz OHCI to GPIO descriptors

[Andy Shevchenko]

On Sun, Oct 01, 2023 at 11:18:41AM +0300, Andy Shevchenko wrote:
> On Wed, Sep 27, 2023 at 04:01:58PM +0200, Linus Walleij wrote:
> > On Mon, Sep 25, 2023 at 9:30 AM Andy Shevchenko <andy@kernel.org> wrote:

...

> > > > +     if (pxa_ohci->usb_host)
> > > > +             gpiod_put(pxa_ohci->usb_host);
> > >
> > > Linus, Bart, do we have misdesigned _optinal() GPIO APIs?
> > >
> > > In GPIOLIB=n, the above requires that redundant check. Shouldn't we replace
> > > gpiod_put() stub to be simply no-op?
> > 
> > You mean the WARN_ON(desc) in gpiod_put() in the static inline
> > stub version?
> > 
> > I thought about it for a bit, drafted a patch removing them, and then
> > realized the following:
> > 
> > If someone is making the gpiolib optional for a driver, i.e. neither
> > DEPENDS ON GPIOLIB nor SELECT GPIOLIB, they are a quite
> > narrow segment. I would say in 9 cases out of 10 or more this is
> > just a driver that should depend on or select GPIOLIB.
> > 
> > I think such drivers should actually do the NULL checks and not be
> > too convenient, the reason is readability: someone reading that
> > driver will be thinking gpios are not optional if they can call
> > gpiod_set_value(), gpiod_put() etc without any sign that the
> > desc is optional.
> > 
> > If the driver uses [devm_]gpiod_get_optional() the library is not
> > using the stubs and does the right thing, and it is clear that
> > the GPIO is *runtime* optional.
> > 
> > But *compile time* optional, *combined* with runtime optional -
> > I'm not so happy if we try to avoid warnings around that. I think
> > it leads to confusing configs and code that looks like gpiolib is
> > around despite it wasn't selected.
> > 
> > If the code isn't depending on or selecting GPIOLIB and still
> > use _optional() calls, it better be ready to do some extra checks,
> > because this is a weird combo, it can't be common.
> > 
> > Could be a documentation update making this clear though.
> > 
> > What do you other people think?
> 
> The problem here indeed if the code is not selecting or being dependent on
> GPIOLIB and uses _optional() calls.
> 
> I agree that this is quite a niche that should be addressed on the driver side.

One more thing, though. I think those warnings are incomplete or actually
reversed, and we outta use WARN_ON(IS_ERR(desc)), no?

This way it will fix my concerns and your concerns will be satisfied, right?
So, if gpiod_get() returns an error pointer and then we are trying to
free it with GPIOLIB=n, _then_ we will got a warning and it's obvious that
driver has to be prepared for that, otherwise if we have it NULL and
call for gpiod_get_optional(), even with GPIOLIB=n, it's fine to free, we
don't care.

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC 1/6] ARM: pxa: Convert Spitz OHCI to GPIO descriptors

[Linus Walleij]

On Sun, Oct 1, 2023 at 11:22 AM Andy Shevchenko <andy@kernel.org> wrote:

 One more thing, though. I think those warnings are incomplete or actually
> reversed, and we outta use WARN_ON(IS_ERR(desc)), no?
>
> This way it will fix my concerns and your concerns will be satisfied, right?
> So, if gpiod_get() returns an error pointer and then we are trying to
> free it with GPIOLIB=n, _then_ we will got a warning and it's obvious that
> driver has to be prepared for that, otherwise if we have it NULL and
> call for gpiod_get_optional(), even with GPIOLIB=n, it's fine to free, we
> don't care.

Since we return return ERR_PTR(-ENOSYS) when compiled out
this sounds right to me!

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel