[RFC,1/6] ARM: pxa: Convert Spitz OHCI to GPIO descriptors
Commit Message
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.
Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
---
arch/arm/mach-pxa/spitz.c | 13 ++++++-------
drivers/usb/host/ohci-pxa27x.c | 10 ++++++++++
2 files changed, 16 insertions(+), 7 deletions(-)
Comments
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?
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
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.
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.
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
@@ -649,23 +649,22 @@ static inline void spitz_mmc_init(void) {}
* USB Host
******************************************************************************/
#if defined(CONFIG_USB_OHCI_HCD) || defined(CONFIG_USB_OHCI_HCD_MODULE)
+GPIO_LOOKUP_SINGLE(spitz_usb_host_gpio_table, "pxa27x-ohci", "gpio-pxa",
+ SPITZ_GPIO_USB_HOST, "usb-host", GPIO_ACTIVE_LOW);
+
static int spitz_ohci_init(struct device *dev)
{
- int err;
-
- err = gpio_request(SPITZ_GPIO_USB_HOST, "USB_HOST");
- if (err)
- return err;
+ gpiod_add_lookup_table(&spitz_usb_host_gpio_table);
/* Only Port 2 is connected, setup USB Port 2 Output Control Register */
UP2OCR = UP2OCR_HXS | UP2OCR_HXOE | UP2OCR_DPPDE | UP2OCR_DMPDE;
- return gpio_direction_output(SPITZ_GPIO_USB_HOST, 1);
+ return 0;
}
static void spitz_ohci_exit(struct device *dev)
{
- gpio_free(SPITZ_GPIO_USB_HOST);
+ gpiod_remove_lookup_table(&spitz_usb_host_gpio_table);
}
static struct pxaohci_platform_data spitz_ohci_platform_data = {
@@ -121,6 +121,7 @@ struct pxa27x_ohci {
void __iomem *mmio_base;
struct regulator *vbus[3];
bool vbus_enabled[3];
+ struct gpio_desc *usb_host;
};
#define to_pxa27x_ohci(hcd) (struct pxa27x_ohci *)(hcd_to_ohci(hcd)->priv)
@@ -447,6 +448,12 @@ static int ohci_hcd_pxa27x_probe(struct platform_device *pdev)
pxa_ohci = to_pxa27x_ohci(hcd);
pxa_ohci->clk = usb_clk;
pxa_ohci->mmio_base = (void __iomem *)hcd->regs;
+ 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);
+ pxa_ohci->usb_host = NULL;
+ }
for (i = 0; i < 3; ++i) {
char name[6];
@@ -512,6 +519,9 @@ static void ohci_hcd_pxa27x_remove(struct platform_device *pdev)
for (i = 0; i < 3; ++i)
pxa27x_ohci_set_vbus_power(pxa_ohci, i, false);
+ if (pxa_ohci->usb_host)
+ gpiod_put(pxa_ohci->usb_host);
+
usb_put_hcd(hcd);
}