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

[Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>]

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@xxxxxxx> 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@xxxxxxx>
> > Cc: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > Cc: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> > Cc: Pavel Machek <pavel@xxxxxxx>
> > Cc: Linus Walleij <linus.walleij@xxxxxxxxxx>
> > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxx>
> 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/  |