[PATCH v2] serial: mctrl_gpio: Support all GPIO suffixes (gpios vs gpio)

[PATCH v2] serial: mctrl_gpio: Support all GPIO suffixes (gpios vs gpio)

[Stefan Roese]

This patch fixes a backward compatibility issue, when boards use the
old style GPIO suffix "-gpio" instead of the new "-gpios". This
potential problem has been introduced by commit d99482673f95 ("serial:
mctrl_gpio: Check if GPIO property exisits before requesting it").

This patch now fixes this issue by using gpiod_count() which iterates
over all supported GPIO suffixes (thanks to Linus for suggesting this).

With this change, the local string is not needed any more. This way
we can remove the allocation in the loop.

Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Pavel Machek <pavel@denx.de>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
v2
- Use gpiod_count() to check if the GPIO exists (Linus)
- Remove the now unnecessary malloc in the loop (kasprintf)

 drivers/tty/serial/serial_mctrl_gpio.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c
index 2b400189be91..ce73b142c66b 100644
--- a/drivers/tty/serial/serial_mctrl_gpio.c
+++ b/drivers/tty/serial/serial_mctrl_gpio.c
@@ -117,18 +117,11 @@ struct mctrl_gpios *mctrl_gpio_init_noauto(struct device *dev, unsigned int idx)
 
 	for (i = 0; i < UART_GPIO_MAX; i++) {
 		enum gpiod_flags flags;
-		char *gpio_str;
-		bool present;
+		int count;
 
 		/* Check if GPIO property exists and continue if not */
-		gpio_str = kasprintf(GFP_KERNEL, "%s-gpios",
-				     mctrl_gpios_desc[i].name);
-		if (!gpio_str)
-			continue;
-
-		present = device_property_present(dev, gpio_str);
-		kfree(gpio_str);
-		if (!present)
+		count = gpiod_count(dev, mctrl_gpios_desc[i].name);
+		if (count <= 0)
 			continue;
 
 		if (mctrl_gpios_desc[i].dir_out)
-- 
2.22.1

Re: [PATCH v2] serial: mctrl_gpio: Support all GPIO suffixes (gpios vs gpio)

[Linus Walleij]

On Thu, Aug 15, 2019 at 10:53 AM Stefan Roese <sr@denx.de> wrote:

> This patch fixes a backward compatibility issue, when boards use the
> old style GPIO suffix "-gpio" instead of the new "-gpios". This
> potential problem has been introduced by commit d99482673f95 ("serial:
> mctrl_gpio: Check if GPIO property exisits before requesting it").
>
> This patch now fixes this issue by using gpiod_count() which iterates
> over all supported GPIO suffixes (thanks to Linus for suggesting this).
>
> With this change, the local string is not needed any more. This way
> we can remove the allocation in the loop.
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Pavel Machek <pavel@denx.de>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> v2
> - Use gpiod_count() to check if the GPIO exists (Linus)
> - Remove the now unnecessary malloc in the loop (kasprintf)

Pretty neat!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v2] serial: mctrl_gpio: Support all GPIO suffixes (gpios vs gpio)

[Geert Uytterhoeven]

CC Mika, who reported the initial issue

On Thu, Aug 15, 2019 at 10:53 AM Stefan Roese <sr@denx.de> wrote:
>
> This patch fixes a backward compatibility issue, when boards use the
> old style GPIO suffix "-gpio" instead of the new "-gpios". This
> potential problem has been introduced by commit d99482673f95 ("serial:
> mctrl_gpio: Check if GPIO property exisits before requesting it").
>
> This patch now fixes this issue by using gpiod_count() which iterates
> over all supported GPIO suffixes (thanks to Linus for suggesting this).
>
> With this change, the local string is not needed any more. This way
> we can remove the allocation in the loop.
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Pavel Machek <pavel@denx.de>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> v2
> - Use gpiod_count() to check if the GPIO exists (Linus)
> - Remove the now unnecessary malloc in the loop (kasprintf)
>
>  drivers/tty/serial/serial_mctrl_gpio.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c
> index 2b400189be91..ce73b142c66b 100644
> --- a/drivers/tty/serial/serial_mctrl_gpio.c
> +++ b/drivers/tty/serial/serial_mctrl_gpio.c
> @@ -117,18 +117,11 @@ struct mctrl_gpios *mctrl_gpio_init_noauto(struct device *dev, unsigned int idx)
>
>         for (i = 0; i < UART_GPIO_MAX; i++) {
>                 enum gpiod_flags flags;
> -               char *gpio_str;
> -               bool present;
> +               int count;
>
>                 /* Check if GPIO property exists and continue if not */
> -               gpio_str = kasprintf(GFP_KERNEL, "%s-gpios",
> -                                    mctrl_gpios_desc[i].name);
> -               if (!gpio_str)
> -                       continue;
> -
> -               present = device_property_present(dev, gpio_str);
> -               kfree(gpio_str);
> -               if (!present)
> +               count = gpiod_count(dev, mctrl_gpios_desc[i].name);
> +               if (count <= 0)
>                         continue;
>
>                 if (mctrl_gpios_desc[i].dir_out)

Seems like both device_property_present() and gpiod_count()
eventually call into acpi_data_get_property().

However, given
commit 6fe9da42f1d98fdb4be1598e230aca97e66cf35d
Author: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Date:   Tue May 23 20:03:20 2017 +0300

    gpio: acpi: Synchronize acpi_find_gpio() and acpi_gpio_count()

    If we pass connection ID to the both functions and at the same time
    acpi_can_fallback_to_crs() returns false we will get different results,
    i.e. the number of GPIO resources returned by acpi_gpio_count() might be
    not correct.

    Fix this by calling acpi_can_fallback_to_crs() in acpi_gpio_count()
    before trying to fallback.

acpi_find_gpio() and acpi_gpio_count() are supposed to use the exact
same logic, so this patch is not gonna work as intended?!?

Note that I still find it strange that acpi_find_gpio() falls back to
unnamed gpios if con_id != NULL, causing the problem in the first place.
This is gonna bite us again later...

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2] serial: mctrl_gpio: Support all GPIO suffixes (gpios vs gpio)

[Uwe Kleine-König]

On Fri, Aug 16, 2019 at 09:16:39AM +0200, Geert Uytterhoeven wrote:
> CC Mika, who reported the initial issue
> 
> On Thu, Aug 15, 2019 at 10:53 AM Stefan Roese <sr@denx.de> wrote:
> >
> > This patch fixes a backward compatibility issue, when boards use the
> > old style GPIO suffix "-gpio" instead of the new "-gpios". This
> > potential problem has been introduced by commit d99482673f95 ("serial:
> > mctrl_gpio: Check if GPIO property exisits before requesting it").
> >
> > This patch now fixes this issue by using gpiod_count() which iterates
> > over all supported GPIO suffixes (thanks to Linus for suggesting this).
> >
> > With this change, the local string is not needed any more. This way
> > we can remove the allocation in the loop.
> >
> > Signed-off-by: Stefan Roese <sr@denx.de>
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> > Cc: Pavel Machek <pavel@denx.de>
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> > v2
> > - Use gpiod_count() to check if the GPIO exists (Linus)
> > - Remove the now unnecessary malloc in the loop (kasprintf)
> >
> >  drivers/tty/serial/serial_mctrl_gpio.c | 13 +++----------
> >  1 file changed, 3 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c
> > index 2b400189be91..ce73b142c66b 100644
> > --- a/drivers/tty/serial/serial_mctrl_gpio.c
> > +++ b/drivers/tty/serial/serial_mctrl_gpio.c
> > @@ -117,18 +117,11 @@ struct mctrl_gpios *mctrl_gpio_init_noauto(struct device *dev, unsigned int idx)
> >
> >         for (i = 0; i < UART_GPIO_MAX; i++) {
> >                 enum gpiod_flags flags;
> > -               char *gpio_str;
> > -               bool present;
> > +               int count;
> >
> >                 /* Check if GPIO property exists and continue if not */
> > -               gpio_str = kasprintf(GFP_KERNEL, "%s-gpios",
> > -                                    mctrl_gpios_desc[i].name);
> > -               if (!gpio_str)
> > -                       continue;
> > -
> > -               present = device_property_present(dev, gpio_str);
> > -               kfree(gpio_str);
> > -               if (!present)
> > +               count = gpiod_count(dev, mctrl_gpios_desc[i].name);
> > +               if (count <= 0)
> >                         continue;
> >
> >                 if (mctrl_gpios_desc[i].dir_out)
> 
> Seems like both device_property_present() and gpiod_count()
> eventually call into acpi_data_get_property().
> 
> However, given
> commit 6fe9da42f1d98fdb4be1598e230aca97e66cf35d
> Author: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Date:   Tue May 23 20:03:20 2017 +0300
> 
>     gpio: acpi: Synchronize acpi_find_gpio() and acpi_gpio_count()
> 
>     If we pass connection ID to the both functions and at the same time
>     acpi_can_fallback_to_crs() returns false we will get different results,
>     i.e. the number of GPIO resources returned by acpi_gpio_count() might be
>     not correct.
> 
>     Fix this by calling acpi_can_fallback_to_crs() in acpi_gpio_count()
>     before trying to fallback.
> 
> acpi_find_gpio() and acpi_gpio_count() are supposed to use the exact
> same logic, so this patch is not gonna work as intended?!?
> 
> Note that I still find it strange that acpi_find_gpio() falls back to
> unnamed gpios if con_id != NULL, causing the problem in the first place.
> This is gonna bite us again later...

Ah, is this the reason we need an additional check before calling
gpiod_get_index_optional?

I would prefer to fix the acpi code then and drop the additional checks
in the mctrl-gpio code.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v2] serial: mctrl_gpio: Support all GPIO suffixes (gpios vs gpio)

[Uwe Kleine-König]

Hello Stefan,

On Thu, Aug 15, 2019 at 10:53:41AM +0200, Stefan Roese wrote:
> This patch fixes a backward compatibility issue, when boards use the
> old style GPIO suffix "-gpio" instead of the new "-gpios". This
> potential problem has been introduced by commit d99482673f95 ("serial:
> mctrl_gpio: Check if GPIO property exisits before requesting it").
> 
> This patch now fixes this issue by using gpiod_count() which iterates
> over all supported GPIO suffixes (thanks to Linus for suggesting this).
> 
> With this change, the local string is not needed any more. This way
> we can remove the allocation in the loop.
> 
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Pavel Machek <pavel@denx.de>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> v2
> - Use gpiod_count() to check if the GPIO exists (Linus)
> - Remove the now unnecessary malloc in the loop (kasprintf)
> 
>  drivers/tty/serial/serial_mctrl_gpio.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c
> index 2b400189be91..ce73b142c66b 100644
> --- a/drivers/tty/serial/serial_mctrl_gpio.c
> +++ b/drivers/tty/serial/serial_mctrl_gpio.c
> @@ -117,18 +117,11 @@ struct mctrl_gpios *mctrl_gpio_init_noauto(struct device *dev, unsigned int idx)
>  
>  	for (i = 0; i < UART_GPIO_MAX; i++) {
>  		enum gpiod_flags flags;
> -		char *gpio_str;
> -		bool present;
> +		int count;
>  
>  		/* Check if GPIO property exists and continue if not */
> -		gpio_str = kasprintf(GFP_KERNEL, "%s-gpios",
> -				     mctrl_gpios_desc[i].name);
> -		if (!gpio_str)
> -			continue;
> -
> -		present = device_property_present(dev, gpio_str);
> -		kfree(gpio_str);
> -		if (!present)
> +		count = gpiod_count(dev, mctrl_gpios_desc[i].name);
> +		if (count <= 0)
>  			continue;

Can you explain why this check is necessary (both the new and the old
variant)? I would expect that it doesn't add any value over the
gpiod_get_index_optional that follows later.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |