mbox series

[RFT,v7,0/6] ARM: pxa: GPIO descriptor conversions

Message ID 20231009-pxa-gpio-v7-0-c8f5f403e856@skole.hr
Headers show
Series ARM: pxa: GPIO descriptor conversions | expand

Message

Duje Mihanović Oct. 9, 2023, 6:33 p.m. UTC 
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.

Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
---
Changes in v7:
- Address maintainer comments:
  - Drop gpiod_put in OHCI
  - Make "struct gpio_descs *leds" in Spitz LEDs global
- Link to v6: https://lore.kernel.org/r/20231006-pxa-gpio-v6-0-981b4910d599@skole.hr

Changes in v6:
- Address maintainer comments:
  - Use devm_gpiod_get_optional() in OHCI
  - Use gpiod_get_array() in Spitz LEDs
- Update trailers
- Link to v5: https://lore.kernel.org/r/20231004-pxa-gpio-v5-0-d99ae6fceea8@skole.hr

Changes in v5:
- Address maintainer comments:
  - Rename "reset generator" GPIO to "reset"
  - Rename ads7846_wait_for_sync() to ads7846_wait_for_sync_gpio()
  - Properly bail out when requesting USB host GPIO fails
  - Use dev_err_probe() when requesting touchscreen sync GPIO fails
  - Use static gpio_desc for gumstix bluetooth reset
- Pulse gumstix bluetooth reset line correctly (assert, then deassert)
- Fix style issue in ads7846_wait_for_sync_gpio()
- Update trailers
- Link to v4: https://lore.kernel.org/r/20231001-pxa-gpio-v4-0-0f3b975e6ed5@skole.hr

Changes in v4:
- Address maintainer comments:
  - Move wait_for_sync() from spitz.c to driver
  - Register LED platform device before getting its gpiod-s
- Add Linus' Reviewed-by
- Link to v3: https://lore.kernel.org/r/20230929-pxa-gpio-v3-0-af8d5e5d1f34@skole.hr

Changes in v3:
- Address maintainer comments:
  - Use GPIO_LOOKUP_IDX for LEDs
  - Drop unnecessary NULL assignments
  - Don't give up on *all* SPI devices if hsync cannot be set up
- Add Linus' Acked-by
- Link to v2: https://lore.kernel.org/r/20230926-pxa-gpio-v2-0-984464d165dd@skole.hr

Changes in v2:
- Address maintainer comments:
  - Change mentions of function to function()
  - Drop cast in OHCI driver dev_warn() call
  - Use %pe in OHCI and reset drivers
  - Use GPIO _optional() API in OHCI driver
  - Drop unnecessary not-null check in OHCI driver
  - Use pr_err() instead of printk() in reset driver
- Rebase on v6.6-rc3
- Link to v1: https://lore.kernel.org/r/20230924-pxa-gpio-v1-0-2805b87d8894@skole.hr

---
Duje Mihanović (6):
      ARM: pxa: Convert Spitz OHCI to GPIO descriptors
      ARM: pxa: Convert Spitz LEDs to GPIO descriptors
      ARM: pxa: Convert Spitz CF power control to GPIO descriptors
      ARM: pxa: Convert reset driver to GPIO descriptors
      ARM: pxa: Convert gumstix Bluetooth to GPIO descriptors
      input: ads7846: Move wait_for_sync() logic to driver

 arch/arm/mach-pxa/gumstix.c         | 22 ++++++------
 arch/arm/mach-pxa/reset.c           | 39 +++++++-------------
 arch/arm/mach-pxa/reset.h           |  3 +-
 arch/arm/mach-pxa/spitz.c           | 71 +++++++++++++++++++++++++------------
 drivers/input/touchscreen/ads7846.c | 22 ++++++++----
 drivers/usb/host/ohci-pxa27x.c      |  5 +++
 include/linux/spi/ads7846.h         |  1 -
 7 files changed, 94 insertions(+), 69 deletions(-)
---
base-commit: 8a749fd1a8720d4619c91c8b6e7528c0a355c0aa
change-id: 20230807-pxa-gpio-3ce25d574814

Best regards,

Comments

Bartosz Golaszewski Oct. 10, 2023, 11:09 a.m. UTC | #1 
On Mon, Oct 9, 2023 at 8:34 PM Duje Mihanović <duje.mihanovic@skole.hr> 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.
>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
> ---
>  arch/arm/mach-pxa/spitz.c      | 13 ++++++-------
>  drivers/usb/host/ohci-pxa27x.c |  5 +++++
>  2 files changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm/mach-pxa/spitz.c b/arch/arm/mach-pxa/spitz.c
> index cc691b199429..535e2b2e997b 100644
> --- a/arch/arm/mach-pxa/spitz.c
> +++ b/arch/arm/mach-pxa/spitz.c
> @@ -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 = {
> diff --git a/drivers/usb/host/ohci-pxa27x.c b/drivers/usb/host/ohci-pxa27x.c
> index 357d9aee38a3..a809ba0bb25e 100644
> --- a/drivers/usb/host/ohci-pxa27x.c
> +++ b/drivers/usb/host/ohci-pxa27x.c
> @@ -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,10 @@ 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 = devm_gpiod_get_optional(&pdev->dev, "usb-host", GPIOD_OUT_LOW);
> +       if (IS_ERR(pxa_ohci->usb_host))
> +               return dev_err_probe(&pdev->dev, PTR_ERR(pxa_ohci->usb_host),
> +                               "failed to get USB host GPIO\n");
>
>         for (i = 0; i < 3; ++i) {
>                 char name[6];
>
> --
> 2.42.0
>
>

Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Bartosz Golaszewski Oct. 10, 2023, 5:39 p.m. UTC | #2 
On Tue, Oct 10, 2023 at 6:33 PM Duje Mihanović <duje.mihanovic@skole.hr> wrote:
>
> On Tuesday, October 10, 2023 1:12:05 PM CEST Bartosz Golaszewski wrote:
> > Gah! I should have noticed this earlier but this is a perfect
> > candidate for using hogs. Can you use gpiod_add_hogs() from
> > linux/gpio/machine.h instead? That would save you having the lookup
> > and the static leds descriptor array.
>
> From what I can tell, the hogs keep a certain pin at a certain state as long
> as the machine is powered on. Is this really what we want to do with LEDs or
> am I missing something?
>

It doesn't seem like anyone is using these GPIOs once they're
requested? Wouldn't the above definitios be analogous to:

GPIO_HOG("pxa-gpio", SPITZ_GPIO_LED_ORANGE, NULL, GPIO_ACTIVE_HIGH, GPIOD_ASIS)
GPIO_HOG("pxa-gpio", SPITZ_GPIO_LED_GREEN, NULL, GPIO_ACTIVE_HIGH, GPIOD_ASIS)

?

Bart
Linus Walleij Oct. 10, 2023, 8:04 p.m. UTC | #3 
On Tue, Oct 10, 2023 at 7:39 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> It doesn't seem like anyone is using these GPIOs once they're
> requested? Wouldn't the above definitios be analogous to:
>
> GPIO_HOG("pxa-gpio", SPITZ_GPIO_LED_ORANGE, NULL, GPIO_ACTIVE_HIGH, GPIOD_ASIS)
> GPIO_HOG("pxa-gpio", SPITZ_GPIO_LED_GREEN, NULL, GPIO_ACTIVE_HIGH, GPIOD_ASIS)

They are used:
+       spitz_gpio_leds[0].gpiod = leds->desc[0];
+       spitz_gpio_leds[1].gpiod = leds->desc[1];

The descriptors are passed to the leds-gpio driver.

But wait: no.

This whole thing:

+static struct gpio_descs *leds;
+
(...)
+       leds = gpiod_get_array_optional(&spitz_led_device.dev,
+                       NULL, GPIOD_ASIS);
+       spitz_gpio_leds[0].gpiod = leds->desc[0];
+       spitz_gpio_leds[1].gpiod = leds->desc[1];

Just delete all that.

The leds-gpio driver will request and use the lines.

It was just so unorthodox that I missed it. Adding the descriptor
table is enough.

Yours,
Linus Walleij
Bartosz Golaszewski Oct. 11, 2023, 7:59 a.m. UTC | #4 
On Tue, Oct 10, 2023 at 10:04 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Tue, Oct 10, 2023 at 7:39 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> > It doesn't seem like anyone is using these GPIOs once they're
> > requested? Wouldn't the above definitios be analogous to:
> >
> > GPIO_HOG("pxa-gpio", SPITZ_GPIO_LED_ORANGE, NULL, GPIO_ACTIVE_HIGH, GPIOD_ASIS)
> > GPIO_HOG("pxa-gpio", SPITZ_GPIO_LED_GREEN, NULL, GPIO_ACTIVE_HIGH, GPIOD_ASIS)
>
> They are used:
> +       spitz_gpio_leds[0].gpiod = leds->desc[0];
> +       spitz_gpio_leds[1].gpiod = leds->desc[1];
>
> The descriptors are passed to the leds-gpio driver.
>
> But wait: no.
>
> This whole thing:
>
> +static struct gpio_descs *leds;
> +
> (...)
> +       leds = gpiod_get_array_optional(&spitz_led_device.dev,
> +                       NULL, GPIOD_ASIS);
> +       spitz_gpio_leds[0].gpiod = leds->desc[0];
> +       spitz_gpio_leds[1].gpiod = leds->desc[1];
>
> Just delete all that.
>
> The leds-gpio driver will request and use the lines.
>
> It was just so unorthodox that I missed it. Adding the descriptor
> table is enough.

Ah, good catch. Your suggestion is of course the correct one.

Bart

>
> Yours,
> Linus Walleij