| Message ID | 20190815085341.28088-1-sr@denx.de |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] serial: mctrl_gpio: Support all GPIO suffixes (gpios vs gpio) | expand |
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
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
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
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
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)
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(-)