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

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

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@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;

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/  |