Linux-GPIO Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/2] gpiolib: Add for_each_gpio_suffix() helper
@ 2019-08-08 13:25 Stefan Roese
  2019-08-08 13:25 ` [PATCH 2/2] serial: mctrl_gpio: Support all GPIO suffixes (gpios vs gpio) Stefan Roese
                   ` (2 more replies)
  0 siblings, 3 replies; 20+ messages in thread
From: Stefan Roese @ 2019-08-08 13:25 UTC (permalink / raw)
  To: linux-serial, linux-gpio
  Cc: Andy Shevchenko, Geert Uytterhoeven, Pavel Machek, Linus Walleij,
	Greg Kroah-Hartman

Add a helper macro to enable the interation over all supported GPIO
suffixes (currently "gpios" & "gpio"). This will be used by the serial
mctrl code to check, if a GPIO property exists before requesting it.

Signed-off-by: Stefan Roese <sr@denx.de>
Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
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>
---
 drivers/gpio/gpiolib.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 7c52c2442173..a3add73f99d6 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -92,6 +92,12 @@ struct acpi_gpio_info {
 /* gpio suffixes used for ACPI and device tree lookup */
 static __maybe_unused const char * const gpio_suffixes[] = { "gpios", "gpio" };
 
+#define for_each_gpio_suffix(idx, suffix)				\
+	for (idx = 0;							\
+	     idx < ARRAY_SIZE(gpio_suffixes) &&				\
+		     (suffix = gpio_suffixes[idx]) != NULL;		\
+	     idx++)
+
 #ifdef CONFIG_OF_GPIO
 struct gpio_desc *of_find_gpio(struct device *dev,
 			       const char *con_id,
-- 
2.22.0


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 2/2] serial: mctrl_gpio: Support all GPIO suffixes (gpios vs gpio)
  2019-08-08 13:25 [PATCH 1/2] gpiolib: Add for_each_gpio_suffix() helper Stefan Roese
@ 2019-08-08 13:25 ` Stefan Roese
  2019-08-08 13:48   ` Andy Shevchenko
  2019-08-12 11:17   ` Geert Uytterhoeven
  2019-08-08 13:44 ` [PATCH 1/2] gpiolib: Add for_each_gpio_suffix() helper Andy Shevchenko
  2019-08-10  8:27 ` Linus Walleij
  2 siblings, 2 replies; 20+ messages in thread
From: Stefan Roese @ 2019-08-08 13:25 UTC (permalink / raw)
  To: linux-serial, linux-gpio
  Cc: Andy Shevchenko, Geert Uytterhoeven, Pavel Machek, Linus Walleij,
	Greg Kroah-Hartman

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 iterating over all supported GPIO
suffixes by using the newly introduced for_each_gpio_suffix() helper.

Also, the string buffer is now allocated on the stack to avoid the
problem of allocation in a loop and its potential failure.

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>
---
 drivers/tty/serial/serial_mctrl_gpio.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c
index 2b400189be91..d444fdaa280a 100644
--- a/drivers/tty/serial/serial_mctrl_gpio.c
+++ b/drivers/tty/serial/serial_mctrl_gpio.c
@@ -15,6 +15,7 @@
 #include <linux/property.h>
 
 #include "serial_mctrl_gpio.h"
+#include "../../gpio/gpiolib.h"
 
 struct mctrl_gpios {
 	struct uart_port *port;
@@ -117,17 +118,24 @@ 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;
+		const char *suffix;
+		char gpio_str[32];	/* 32 is max size of property name */
 		bool present;
+		int k;
+
+		/*
+		 * Check if GPIO property exists and continue if not. Iterate
+		 * over all supported GPIO suffixes (foo-gpios vs. foo-gpio).
+		 */
+		for_each_gpio_suffix(k, suffix) {
+			snprintf(gpio_str, sizeof(gpio_str), "%s-%s",
+				 mctrl_gpios_desc[i].name, suffix);
+
+			present = device_property_present(dev, gpio_str);
+			if (present)
+				break;
+		}
 
-		/* 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)
 			continue;
 
-- 
2.22.0


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/2] gpiolib: Add for_each_gpio_suffix() helper
  2019-08-08 13:25 [PATCH 1/2] gpiolib: Add for_each_gpio_suffix() helper Stefan Roese
  2019-08-08 13:25 ` [PATCH 2/2] serial: mctrl_gpio: Support all GPIO suffixes (gpios vs gpio) Stefan Roese
@ 2019-08-08 13:44 ` Andy Shevchenko
  2019-08-08 14:13   ` Stefan Roese
  2019-08-10  8:27 ` Linus Walleij
  2 siblings, 1 reply; 20+ messages in thread
From: Andy Shevchenko @ 2019-08-08 13:44 UTC (permalink / raw)
  To: Stefan Roese
  Cc: linux-serial, linux-gpio, Geert Uytterhoeven, Pavel Machek,
	Linus Walleij, Greg Kroah-Hartman

On Thu, Aug 08, 2019 at 03:25:42PM +0200, Stefan Roese wrote:
> Add a helper macro to enable the interation over all supported GPIO
> suffixes (currently "gpios" & "gpio"). This will be used by the serial
> mctrl code to check, if a GPIO property exists before requesting it.

Thanks! My comments below.

> +#define for_each_gpio_suffix(idx, suffix)				\
> +	for (idx = 0;							\
> +	     idx < ARRAY_SIZE(gpio_suffixes) &&				\
> +		     (suffix = gpio_suffixes[idx]) != NULL;		\
> +	     idx++)

I think we can use comma here, like

	for (idx = 0; idx < ARRAY_SIZE(...), suffix = ...; idx++;)

however, this needs to be checked, b/c I think the last operation makes a
return code, and probably we have to assign suffix first.

(and perhaps switch to one letter for idx makes it fit one line)

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] serial: mctrl_gpio: Support all GPIO suffixes (gpios vs gpio)
  2019-08-08 13:25 ` [PATCH 2/2] serial: mctrl_gpio: Support all GPIO suffixes (gpios vs gpio) Stefan Roese
@ 2019-08-08 13:48   ` Andy Shevchenko
  2019-08-08 13:59     ` Stefan Roese
  2019-08-12 11:17   ` Geert Uytterhoeven
  1 sibling, 1 reply; 20+ messages in thread
From: Andy Shevchenko @ 2019-08-08 13:48 UTC (permalink / raw)
  To: Stefan Roese
  Cc: linux-serial, linux-gpio, Geert Uytterhoeven, Pavel Machek,
	Linus Walleij, Greg Kroah-Hartman

On Thu, Aug 08, 2019 at 03:25:43PM +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 iterating over all supported GPIO
> suffixes by using the newly introduced for_each_gpio_suffix() helper.
> 
> Also, the string buffer is now allocated on the stack to avoid the
> problem of allocation in a loop and its potential failure.

>  	for (i = 0; i < UART_GPIO_MAX; i++) {
>  		enum gpiod_flags flags;
> -		char *gpio_str;
> +		const char *suffix;
> +		char gpio_str[32];	/* 32 is max size of property name */

Hmm... don't we have some define for the maximum length of property?

Or maybe we can still continue using kasprintf() approach?

>  		bool present;
> +		int k;
> +
> +		/*
> +		 * Check if GPIO property exists and continue if not. Iterate
> +		 * over all supported GPIO suffixes (foo-gpios vs. foo-gpio).
> +		 */
> +		for_each_gpio_suffix(k, suffix) {
> +			snprintf(gpio_str, sizeof(gpio_str), "%s-%s",
> +				 mctrl_gpios_desc[i].name, suffix);
> +
> +			present = device_property_present(dev, gpio_str);
> +			if (present)
> +				break;
> +		}
>  
> -		/* 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);

Because there is no more explicit assignment of present outside of the loop,
the compiler may warn about uninitialized variable in use...

> -		kfree(gpio_str);

>  		if (!present)

...here.

>  			continue;

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] serial: mctrl_gpio: Support all GPIO suffixes (gpios vs gpio)
  2019-08-08 13:48   ` Andy Shevchenko
@ 2019-08-08 13:59     ` Stefan Roese
  2019-08-12 10:53       ` Andy Shevchenko
  0 siblings, 1 reply; 20+ messages in thread
From: Stefan Roese @ 2019-08-08 13:59 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: linux-serial, linux-gpio, Geert Uytterhoeven, Pavel Machek,
	Linus Walleij, Greg Kroah-Hartman

Hi Andy,

On 08.08.19 15:48, Andy Shevchenko wrote:
> On Thu, Aug 08, 2019 at 03:25:43PM +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 iterating over all supported GPIO
>> suffixes by using the newly introduced for_each_gpio_suffix() helper.
>>
>> Also, the string buffer is now allocated on the stack to avoid the
>> problem of allocation in a loop and its potential failure.
> 
>>   	for (i = 0; i < UART_GPIO_MAX; i++) {
>>   		enum gpiod_flags flags;
>> -		char *gpio_str;
>> +		const char *suffix;
>> +		char gpio_str[32];	/* 32 is max size of property name */
> 
> Hmm... don't we have some define for the maximum length of property?

I've come up with this assumption from this code (identical comment):

https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib-of.c#L293

(and other places in drivers/gpio/*)
  
> Or maybe we can still continue using kasprintf() approach?

Frankly, I was feeling a bit uncomfortable with this memory allocation
in a loop. And Pavel also commented on this:

https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2066286.html

So I would really prefer to move this buffer to the stack instead.
  
>>   		bool present;
>> +		int k;
>> +
>> +		/*
>> +		 * Check if GPIO property exists and continue if not. Iterate
>> +		 * over all supported GPIO suffixes (foo-gpios vs. foo-gpio).
>> +		 */
>> +		for_each_gpio_suffix(k, suffix) {
>> +			snprintf(gpio_str, sizeof(gpio_str), "%s-%s",
>> +				 mctrl_gpios_desc[i].name, suffix);
>> +
>> +			present = device_property_present(dev, gpio_str);
>> +			if (present)
>> +				break;
>> +		}
>>   
>> -		/* 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);
> 
> Because there is no more explicit assignment of present outside of the loop,
> the compiler may warn about uninitialized variable in use...
> 
>> -		kfree(gpio_str);
> 
>>   		if (!present)
> 
> ...here.

My compiler does not warn (MIPS GCC 8.1) but I see your point. I'll add
an initialization in the next version.

Thanks,
Stefan

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/2] gpiolib: Add for_each_gpio_suffix() helper
  2019-08-08 13:44 ` [PATCH 1/2] gpiolib: Add for_each_gpio_suffix() helper Andy Shevchenko
@ 2019-08-08 14:13   ` Stefan Roese
  0 siblings, 0 replies; 20+ messages in thread
From: Stefan Roese @ 2019-08-08 14:13 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: linux-serial, linux-gpio, Geert Uytterhoeven, Pavel Machek,
	Linus Walleij, Greg Kroah-Hartman

Hi Andy,

On 08.08.19 15:44, Andy Shevchenko wrote:
> On Thu, Aug 08, 2019 at 03:25:42PM +0200, Stefan Roese wrote:
>> Add a helper macro to enable the interation over all supported GPIO
>> suffixes (currently "gpios" & "gpio"). This will be used by the serial
>> mctrl code to check, if a GPIO property exists before requesting it.
> 
> Thanks! My comments below.
> 
>> +#define for_each_gpio_suffix(idx, suffix)				\
>> +	for (idx = 0;							\
>> +	     idx < ARRAY_SIZE(gpio_suffixes) &&				\
>> +		     (suffix = gpio_suffixes[idx]) != NULL;		\
>> +	     idx++)
> 
> I think we can use comma here, like
> 
> 	for (idx = 0; idx < ARRAY_SIZE(...), suffix = ...; idx++;)
> 
> however, this needs to be checked, b/c I think the last operation makes a
> return code, and probably we have to assign suffix first.

Yes, this seems to be the case (assign suffix first). But in this
case, the assignment is done *before* checking if the index is still
valid (in the array). In this case "suffix = gpio_suffices[2]". Or am
I missing something?
  
> (and perhaps switch to one letter for idx makes it fit one line)

Yes.

Thanks,
Stefan

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/2] gpiolib: Add for_each_gpio_suffix() helper
  2019-08-08 13:25 [PATCH 1/2] gpiolib: Add for_each_gpio_suffix() helper Stefan Roese
  2019-08-08 13:25 ` [PATCH 2/2] serial: mctrl_gpio: Support all GPIO suffixes (gpios vs gpio) Stefan Roese
  2019-08-08 13:44 ` [PATCH 1/2] gpiolib: Add for_each_gpio_suffix() helper Andy Shevchenko
@ 2019-08-10  8:27 ` Linus Walleij
  2019-08-10  8:48   ` Greg Kroah-Hartman
  2019-08-12 11:18   ` Geert Uytterhoeven
  2 siblings, 2 replies; 20+ messages in thread
From: Linus Walleij @ 2019-08-10  8:27 UTC (permalink / raw)
  To: Stefan Roese
  Cc: linux-serial, open list:GPIO SUBSYSTEM, Andy Shevchenko,
	Geert Uytterhoeven, Pavel Machek, Greg Kroah-Hartman

On Thu, Aug 8, 2019 at 3:25 PM Stefan Roese <sr@denx.de> wrote:

> Add a helper macro to enable the interation over all supported GPIO
> suffixes (currently "gpios" & "gpio"). This will be used by the serial
> mctrl code to check, if a GPIO property exists before requesting it.
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 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>

I really like this patch, it makes things so much more readable.

Do you want me to apply both patches to the GPIO tree when
we agreed on the final version? I need some ACK from some
serial maintainer.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/2] gpiolib: Add for_each_gpio_suffix() helper
  2019-08-10  8:27 ` Linus Walleij
@ 2019-08-10  8:48   ` Greg Kroah-Hartman
  2019-08-12 11:18   ` Geert Uytterhoeven
  1 sibling, 0 replies; 20+ messages in thread
From: Greg Kroah-Hartman @ 2019-08-10  8:48 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Stefan Roese, linux-serial, open list:GPIO SUBSYSTEM,
	Andy Shevchenko, Geert Uytterhoeven, Pavel Machek

On Sat, Aug 10, 2019 at 10:27:27AM +0200, Linus Walleij wrote:
> On Thu, Aug 8, 2019 at 3:25 PM Stefan Roese <sr@denx.de> wrote:
> 
> > Add a helper macro to enable the interation over all supported GPIO
> > suffixes (currently "gpios" & "gpio"). This will be used by the serial
> > mctrl code to check, if a GPIO property exists before requesting it.
> >
> > Signed-off-by: Stefan Roese <sr@denx.de>
> > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > 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>
> 
> I really like this patch, it makes things so much more readable.
> 
> Do you want me to apply both patches to the GPIO tree when
> we agreed on the final version? I need some ACK from some
> serial maintainer.

When you all can agree on the final version, I'll be glad to give my ack
for you to take these :)

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] serial: mctrl_gpio: Support all GPIO suffixes (gpios vs gpio)
  2019-08-08 13:59     ` Stefan Roese
@ 2019-08-12 10:53       ` Andy Shevchenko
  2019-08-13 11:42         ` Pavel Machek
  0 siblings, 1 reply; 20+ messages in thread
From: Andy Shevchenko @ 2019-08-12 10:53 UTC (permalink / raw)
  To: Stefan Roese
  Cc: linux-serial, linux-gpio, Geert Uytterhoeven, Pavel Machek,
	Linus Walleij, Greg Kroah-Hartman

On Thu, Aug 08, 2019 at 03:59:36PM +0200, Stefan Roese wrote:
> On 08.08.19 15:48, Andy Shevchenko wrote:
> > On Thu, Aug 08, 2019 at 03:25:43PM +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 iterating over all supported GPIO
> > > suffixes by using the newly introduced for_each_gpio_suffix() helper.
> > > 
> > > Also, the string buffer is now allocated on the stack to avoid the
> > > problem of allocation in a loop and its potential failure.
> > 
> > >   	for (i = 0; i < UART_GPIO_MAX; i++) {
> > >   		enum gpiod_flags flags;
> > > -		char *gpio_str;
> > > +		const char *suffix;
> > > +		char gpio_str[32];	/* 32 is max size of property name */
> > 
> > Hmm... don't we have some define for the maximum length of property?
> 
> I've come up with this assumption from this code (identical comment):
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib-of.c#L293
> 
> (and other places in drivers/gpio/*)

I tried hard to find an evidence of this in Linux kernel, I assume that comes
from DT compiler or something, but fail. Linux kernel OF properties handling is
written in the assumption of arbitrary length of the property name.

It might be that my hard was not hard at all and I missed something.

> > Or maybe we can still continue using kasprintf() approach?
> 
> Frankly, I was feeling a bit uncomfortable with this memory allocation
> in a loop. And Pavel also commented on this:
> 
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2066286.html

If memory allocator fails, it's a big issue, and what will happen next probably
much less important.

> So I would really prefer to move this buffer to the stack instead.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] serial: mctrl_gpio: Support all GPIO suffixes (gpios vs gpio)
  2019-08-08 13:25 ` [PATCH 2/2] serial: mctrl_gpio: Support all GPIO suffixes (gpios vs gpio) Stefan Roese
  2019-08-08 13:48   ` Andy Shevchenko
@ 2019-08-12 11:17   ` Geert Uytterhoeven
  2019-08-12 11:53     ` Stefan Roese
  1 sibling, 1 reply; 20+ messages in thread
From: Geert Uytterhoeven @ 2019-08-12 11:17 UTC (permalink / raw)
  To: Stefan Roese
  Cc: open list:SERIAL DRIVERS, open list:GPIO SUBSYSTEM,
	Andy Shevchenko, Pavel Machek, Linus Walleij, Greg Kroah-Hartman

Hi Stefan,

On Thu, Aug 8, 2019 at 3:25 PM 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 iterating over all supported GPIO
> suffixes by using the newly introduced for_each_gpio_suffix() helper.
>
> Also, the string buffer is now allocated on the stack to avoid the
> problem of allocation in a loop and its potential failure.
>
> Signed-off-by: Stefan Roese <sr@denx.de>

Do we really need to spread this *-gpio" legacy support all over the kernel?

Seeing the only in-kernel users of legacy "rts-gpio" are

    arch/arm/boot/dts/am335x-nano.dts:      rts-gpio = <&gpio0 13
GPIO_ACTIVE_HIGH>;
    arch/arm/boot/dts/am335x-nano.dts:      rts-gpio = <&gpio2 15
GPIO_ACTIVE_HIGH>;
    arch/arm/boot/dts/am335x-pdu001.dts:    rts-gpio = <&gpio1 9
GPIO_ACTIVE_HIGH>;

and this is handled by omap-serial.c, predating mctrl_gpio, I'd like to
reconsider.

Documentation/devicetree/bindings/serial/serial.txt always described
the "*-gpios"
variants, so there should be no users of the legacy "*-gpio" variants.

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/2] gpiolib: Add for_each_gpio_suffix() helper
  2019-08-10  8:27 ` Linus Walleij
  2019-08-10  8:48   ` Greg Kroah-Hartman
@ 2019-08-12 11:18   ` Geert Uytterhoeven
  2019-08-14  8:48     ` Linus Walleij
  1 sibling, 1 reply; 20+ messages in thread
From: Geert Uytterhoeven @ 2019-08-12 11:18 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Stefan Roese, open list:SERIAL DRIVERS, open list:GPIO SUBSYSTEM,
	Andy Shevchenko, Pavel Machek, Greg Kroah-Hartman

Hi Linus,

On Sat, Aug 10, 2019 at 10:27 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Aug 8, 2019 at 3:25 PM Stefan Roese <sr@denx.de> wrote:
> > Add a helper macro to enable the interation over all supported GPIO
> > suffixes (currently "gpios" & "gpio"). This will be used by the serial
> > mctrl code to check, if a GPIO property exists before requesting it.
> >
> > Signed-off-by: Stefan Roese <sr@denx.de>
> > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > 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>
>
> I really like this patch, it makes things so much more readable.

Do we really need to spread this *-gpio" legacy support all over the kernel?

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] serial: mctrl_gpio: Support all GPIO suffixes (gpios vs gpio)
  2019-08-12 11:17   ` Geert Uytterhoeven
@ 2019-08-12 11:53     ` Stefan Roese
  2019-08-12 12:11       ` Geert Uytterhoeven
  0 siblings, 1 reply; 20+ messages in thread
From: Stefan Roese @ 2019-08-12 11:53 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: open list:SERIAL DRIVERS, open list:GPIO SUBSYSTEM,
	Andy Shevchenko, Pavel Machek, Linus Walleij, Greg Kroah-Hartman

Hi Geert,

On 12.08.19 13:17, Geert Uytterhoeven wrote:
> Hi Stefan,
> 
> On Thu, Aug 8, 2019 at 3:25 PM 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 iterating over all supported GPIO
>> suffixes by using the newly introduced for_each_gpio_suffix() helper.
>>
>> Also, the string buffer is now allocated on the stack to avoid the
>> problem of allocation in a loop and its potential failure.
>>
>> Signed-off-by: Stefan Roese <sr@denx.de>
> 
> Do we really need to spread this *-gpio" legacy support all over the kernel?
> 
> Seeing the only in-kernel users of legacy "rts-gpio" are
> 
>      arch/arm/boot/dts/am335x-nano.dts:      rts-gpio = <&gpio0 13
> GPIO_ACTIVE_HIGH>;
>      arch/arm/boot/dts/am335x-nano.dts:      rts-gpio = <&gpio2 15
> GPIO_ACTIVE_HIGH>;
>      arch/arm/boot/dts/am335x-pdu001.dts:    rts-gpio = <&gpio1 9
> GPIO_ACTIVE_HIGH>;
> 
> and this is handled by omap-serial.c, predating mctrl_gpio, I'd like to
> reconsider.
> 
> Documentation/devicetree/bindings/serial/serial.txt always described
> the "*-gpios"
> variants, so there should be no users of the legacy "*-gpio" variants.

Hmmm, you were the one to comment about supporting (not breaking) the
"-gpio" variant:

https://lkml.org/lkml/2019/6/24/248

That was my main motivation to work on this patch.

If we all agree, that only the documented "-gpios" variant needs to
be supported, then we can drop this patch.

Thanks,
Stefan

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] serial: mctrl_gpio: Support all GPIO suffixes (gpios vs gpio)
  2019-08-12 11:53     ` Stefan Roese
@ 2019-08-12 12:11       ` Geert Uytterhoeven
  2019-08-12 12:31         ` Andy Shevchenko
  0 siblings, 1 reply; 20+ messages in thread
From: Geert Uytterhoeven @ 2019-08-12 12:11 UTC (permalink / raw)
  To: Stefan Roese
  Cc: open list:SERIAL DRIVERS, open list:GPIO SUBSYSTEM,
	Andy Shevchenko, Pavel Machek, Linus Walleij, Greg Kroah-Hartman,
	Mika Westerberg

Hi Stefan,

On Mon, Aug 12, 2019 at 1:54 PM Stefan Roese <sr@denx.de> wrote:
> On 12.08.19 13:17, Geert Uytterhoeven wrote:
> > On Thu, Aug 8, 2019 at 3:25 PM 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 iterating over all supported GPIO
> >> suffixes by using the newly introduced for_each_gpio_suffix() helper.
> >>
> >> Also, the string buffer is now allocated on the stack to avoid the
> >> problem of allocation in a loop and its potential failure.
> >>
> >> Signed-off-by: Stefan Roese <sr@denx.de>
> >
> > Do we really need to spread this *-gpio" legacy support all over the kernel?
> >
> > Seeing the only in-kernel users of legacy "rts-gpio" are
> >
> >      arch/arm/boot/dts/am335x-nano.dts:      rts-gpio = <&gpio0 13
> > GPIO_ACTIVE_HIGH>;
> >      arch/arm/boot/dts/am335x-nano.dts:      rts-gpio = <&gpio2 15
> > GPIO_ACTIVE_HIGH>;
> >      arch/arm/boot/dts/am335x-pdu001.dts:    rts-gpio = <&gpio1 9
> > GPIO_ACTIVE_HIGH>;
> >
> > and this is handled by omap-serial.c, predating mctrl_gpio, I'd like to
> > reconsider.
> >
> > Documentation/devicetree/bindings/serial/serial.txt always described
> > the "*-gpios"
> > variants, so there should be no users of the legacy "*-gpio" variants.
>
> Hmmm, you were the one to comment about supporting (not breaking) the
> "-gpio" variant:
>
> https://lkml.org/lkml/2019/6/24/248
>
> That was my main motivation to work on this patch.

I know. That's why I wrote "reconsider".  Before, I assumed there were actual
users of the legacy properties, which turned out to be wrong.

IMHO no driver should be aware there can be multiple possible suffixes.

Sorry for the trouble.

> If we all agree, that only the documented "-gpios" variant needs to
> be supported, then we can drop this patch.

BTW, I'm still wondering if d99482673f950817 ("serial: mctrl_gpio: Check
if GPIO property exisits before requesting it") is the right fix.
This is a place where we rely explicitly on named GPIOs being present, so
IMHO the ACPI code should not return a random GPIO instead.

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] serial: mctrl_gpio: Support all GPIO suffixes (gpios vs gpio)
  2019-08-12 12:11       ` Geert Uytterhoeven
@ 2019-08-12 12:31         ` Andy Shevchenko
  0 siblings, 0 replies; 20+ messages in thread
From: Andy Shevchenko @ 2019-08-12 12:31 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Stefan Roese, open list:SERIAL DRIVERS, open list:GPIO SUBSYSTEM,
	Pavel Machek, Linus Walleij, Greg Kroah-Hartman, Mika Westerberg

On Mon, Aug 12, 2019 at 02:11:42PM +0200, Geert Uytterhoeven wrote:
> On Mon, Aug 12, 2019 at 1:54 PM Stefan Roese <sr@denx.de> wrote:
> > On 12.08.19 13:17, Geert Uytterhoeven wrote:

> > If we all agree, that only the documented "-gpios" variant needs to
> > be supported, then we can drop this patch.
> 
> BTW, I'm still wondering if d99482673f950817 ("serial: mctrl_gpio: Check
> if GPIO property exisits before requesting it") is the right fix.
> This is a place where we rely explicitly on named GPIOs being present, so
> IMHO the ACPI code should not return a random GPIO instead.

ACPI is not taking it to consideration for now. That is, there is no support of
mctrl gpio for ACPI.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] serial: mctrl_gpio: Support all GPIO suffixes (gpios vs gpio)
  2019-08-12 10:53       ` Andy Shevchenko
@ 2019-08-13 11:42         ` Pavel Machek
  2019-08-13 12:15           ` Andy Shevchenko
  0 siblings, 1 reply; 20+ messages in thread
From: Pavel Machek @ 2019-08-13 11:42 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Stefan Roese, linux-serial, linux-gpio, Geert Uytterhoeven,
	Pavel Machek, Linus Walleij, Greg Kroah-Hartman

[-- Attachment #1: Type: text/plain, Size: 2546 bytes --]

On Mon 2019-08-12 13:53:07, Andy Shevchenko wrote:
> On Thu, Aug 08, 2019 at 03:59:36PM +0200, Stefan Roese wrote:
> > On 08.08.19 15:48, Andy Shevchenko wrote:
> > > On Thu, Aug 08, 2019 at 03:25:43PM +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 iterating over all supported GPIO
> > > > suffixes by using the newly introduced for_each_gpio_suffix() helper.
> > > > 
> > > > Also, the string buffer is now allocated on the stack to avoid the
> > > > problem of allocation in a loop and its potential failure.
> > > 
> > > >   	for (i = 0; i < UART_GPIO_MAX; i++) {
> > > >   		enum gpiod_flags flags;
> > > > -		char *gpio_str;
> > > > +		const char *suffix;
> > > > +		char gpio_str[32];	/* 32 is max size of property name */
> > > 
> > > Hmm... don't we have some define for the maximum length of property?
> > 
> > I've come up with this assumption from this code (identical comment):
> > 
> > https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib-of.c#L293
> > 
> > (and other places in drivers/gpio/*)
> 
> I tried hard to find an evidence of this in Linux kernel, I assume that comes
> from DT compiler or something, but fail. Linux kernel OF properties handling is
> written in the assumption of arbitrary length of the property name.
> 
> It might be that my hard was not hard at all and I missed something.
> 
> > > Or maybe we can still continue using kasprintf() approach?
> > 
> > Frankly, I was feeling a bit uncomfortable with this memory allocation
> > in a loop. And Pavel also commented on this:
> > 
> > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2066286.html
> 
> If memory allocator fails, it's a big issue, and what will happen next probably
> much less important.

Not... really. With "too big" allocations, it will fail.

Anyway, my point is that allocating in a loop for this is slow and
ugly. If we don't have a maximum property length, we should probably
invent some. I mean, we can agree that 64KB property name is not okay,
right?


								Pavel
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] serial: mctrl_gpio: Support all GPIO suffixes (gpios vs gpio)
  2019-08-13 11:42         ` Pavel Machek
@ 2019-08-13 12:15           ` Andy Shevchenko
  0 siblings, 0 replies; 20+ messages in thread
From: Andy Shevchenko @ 2019-08-13 12:15 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Stefan Roese, linux-serial, linux-gpio, Geert Uytterhoeven,
	Linus Walleij, Greg Kroah-Hartman

On Tue, Aug 13, 2019 at 01:42:58PM +0200, Pavel Machek wrote:
> On Mon 2019-08-12 13:53:07, Andy Shevchenko wrote:
> > On Thu, Aug 08, 2019 at 03:59:36PM +0200, Stefan Roese wrote:
> > > On 08.08.19 15:48, Andy Shevchenko wrote:
> > > > On Thu, Aug 08, 2019 at 03:25:43PM +0200, Stefan Roese wrote:

> > I tried hard to find an evidence of this in Linux kernel, I assume that comes
> > from DT compiler or something, but fail. Linux kernel OF properties handling is
> > written in the assumption of arbitrary length of the property name.
> > 
> > It might be that my hard was not hard at all and I missed something.
> > 
> > > > Or maybe we can still continue using kasprintf() approach?
> > > 
> > > Frankly, I was feeling a bit uncomfortable with this memory allocation
> > > in a loop. And Pavel also commented on this:
> > > 
> > > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2066286.html
> > 
> > If memory allocator fails, it's a big issue, and what will happen next probably
> > much less important.
> 
> Not... really. With "too big" allocations, it will fail.

Yeah, or with relatively small if memory is too much fragmented.
But we are talking about really small allocations here in most cases, right?

> Anyway, my point is that allocating in a loop for this is slow and
> ugly. If we don't have a maximum property length, we should probably
> invent some. I mean, we can agree that 64KB property name is not okay,
> right?

My point that is should be declared on the level of property API.
Restricting it on the level of one, 'client' to that API, framework is not
a solution.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/2] gpiolib: Add for_each_gpio_suffix() helper
  2019-08-12 11:18   ` Geert Uytterhoeven
@ 2019-08-14  8:48     ` Linus Walleij
  2019-08-14 13:17       ` Stefan Roese
  0 siblings, 1 reply; 20+ messages in thread
From: Linus Walleij @ 2019-08-14  8:48 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Stefan Roese, open list:SERIAL DRIVERS, open list:GPIO SUBSYSTEM,
	Andy Shevchenko, Pavel Machek, Greg Kroah-Hartman

On Mon, Aug 12, 2019 at 1:18 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Sat, Aug 10, 2019 at 10:27 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Thu, Aug 8, 2019 at 3:25 PM Stefan Roese <sr@denx.de> wrote:
> > > Add a helper macro to enable the interation over all supported GPIO
> > > suffixes (currently "gpios" & "gpio"). This will be used by the serial
> > > mctrl code to check, if a GPIO property exists before requesting it.
> > >
> > > Signed-off-by: Stefan Roese <sr@denx.de>
> > > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > 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>
> >
> > I really like this patch, it makes things so much more readable.
>
> Do we really need to spread this *-gpio" legacy support all over the kernel?

Not really :/

Isn't it possible to use something like gpiod_count(dev, "foo") to
check for any GPIOs instead?

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/2] gpiolib: Add for_each_gpio_suffix() helper
  2019-08-14  8:48     ` Linus Walleij
@ 2019-08-14 13:17       ` Stefan Roese
  2019-08-14 13:32         ` Geert Uytterhoeven
  2019-08-14 13:38         ` Andy Shevchenko
  0 siblings, 2 replies; 20+ messages in thread
From: Stefan Roese @ 2019-08-14 13:17 UTC (permalink / raw)
  To: Linus Walleij, Geert Uytterhoeven
  Cc: open list:SERIAL DRIVERS, open list:GPIO SUBSYSTEM,
	Andy Shevchenko, Pavel Machek, Greg Kroah-Hartman

On 14.08.19 10:48, Linus Walleij wrote:
> On Mon, Aug 12, 2019 at 1:18 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Sat, Aug 10, 2019 at 10:27 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>>> On Thu, Aug 8, 2019 at 3:25 PM Stefan Roese <sr@denx.de> wrote:
>>>> Add a helper macro to enable the interation over all supported GPIO
>>>> suffixes (currently "gpios" & "gpio"). This will be used by the serial
>>>> mctrl code to check, if a GPIO property exists before requesting it.
>>>>
>>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>>> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>>> 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>
>>>
>>> I really like this patch, it makes things so much more readable.
>>
>> Do we really need to spread this *-gpio" legacy support all over the kernel?
> 
> Not really :/
> 
> Isn't it possible to use something like gpiod_count(dev, "foo") to
> check for any GPIOs instead?

Good idea. I can rework my patch to use gpiod_count() to check if the
GPIO exists before requesting it. This way, we're not spreading the
legacy "-gpio" support any more.

But I'm unsure, if I should change the string malloc (kasprintf) to the
fixed length string on the stack as I've done in this patch version.

Thanks,
Stefan

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/2] gpiolib: Add for_each_gpio_suffix() helper
  2019-08-14 13:17       ` Stefan Roese
@ 2019-08-14 13:32         ` Geert Uytterhoeven
  2019-08-14 13:38         ` Andy Shevchenko
  1 sibling, 0 replies; 20+ messages in thread
From: Geert Uytterhoeven @ 2019-08-14 13:32 UTC (permalink / raw)
  To: Stefan Roese
  Cc: Linus Walleij, open list:SERIAL DRIVERS,
	open list:GPIO SUBSYSTEM, Andy Shevchenko, Pavel Machek,
	Greg Kroah-Hartman

Hi Stefan,

On Wed, Aug 14, 2019 at 3:17 PM Stefan Roese <sr@denx.de> wrote:
> On 14.08.19 10:48, Linus Walleij wrote:
> > On Mon, Aug 12, 2019 at 1:18 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >> On Sat, Aug 10, 2019 at 10:27 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> >>> On Thu, Aug 8, 2019 at 3:25 PM Stefan Roese <sr@denx.de> wrote:
> >>>> Add a helper macro to enable the interation over all supported GPIO
> >>>> suffixes (currently "gpios" & "gpio"). This will be used by the serial
> >>>> mctrl code to check, if a GPIO property exists before requesting it.
> >>>>
> >>>> Signed-off-by: Stefan Roese <sr@denx.de>
> >>>> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >>>> 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>
> >>>
> >>> I really like this patch, it makes things so much more readable.
> >>
> >> Do we really need to spread this *-gpio" legacy support all over the kernel?
> >
> > Not really :/
> >
> > Isn't it possible to use something like gpiod_count(dev, "foo") to
> > check for any GPIOs instead?
>
> Good idea. I can rework my patch to use gpiod_count() to check if the
> GPIO exists before requesting it. This way, we're not spreading the
> legacy "-gpio" support any more.
>
> But I'm unsure, if I should change the string malloc (kasprintf) to the
> fixed length string on the stack as I've done in this patch version.

Would that work for the mctrl-gpio case?
The issue is that there exist GPIOs (found by index), but they don't match
the passed con_id.

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/2] gpiolib: Add for_each_gpio_suffix() helper
  2019-08-14 13:17       ` Stefan Roese
  2019-08-14 13:32         ` Geert Uytterhoeven
@ 2019-08-14 13:38         ` Andy Shevchenko
  1 sibling, 0 replies; 20+ messages in thread
From: Andy Shevchenko @ 2019-08-14 13:38 UTC (permalink / raw)
  To: Stefan Roese
  Cc: Linus Walleij, Geert Uytterhoeven, open list:SERIAL DRIVERS,
	open list:GPIO SUBSYSTEM, Pavel Machek, Greg Kroah-Hartman

On Wed, Aug 14, 2019 at 03:17:44PM +0200, Stefan Roese wrote:
> On 14.08.19 10:48, Linus Walleij wrote:
> > On Mon, Aug 12, 2019 at 1:18 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Sat, Aug 10, 2019 at 10:27 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> > > > On Thu, Aug 8, 2019 at 3:25 PM Stefan Roese <sr@denx.de> wrote:
> > > > > Add a helper macro to enable the interation over all supported GPIO
> > > > > suffixes (currently "gpios" & "gpio"). This will be used by the serial
> > > > > mctrl code to check, if a GPIO property exists before requesting it.
> > > > > 
> > > > > Signed-off-by: Stefan Roese <sr@denx.de>
> > > > > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > > 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>
> > > > 
> > > > I really like this patch, it makes things so much more readable.
> > > 
> > > Do we really need to spread this *-gpio" legacy support all over the kernel?
> > 
> > Not really :/
> > 
> > Isn't it possible to use something like gpiod_count(dev, "foo") to
> > check for any GPIOs instead?
> 
> Good idea. I can rework my patch to use gpiod_count() to check if the
> GPIO exists before requesting it. This way, we're not spreading the
> legacy "-gpio" support any more.
> 
> But I'm unsure, if I should change the string malloc (kasprintf) to the
> fixed length string on the stack as I've done in this patch version.

You don't need suffix for gpiod_count(). Will you need that at all?

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, back to index

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-08 13:25 [PATCH 1/2] gpiolib: Add for_each_gpio_suffix() helper Stefan Roese
2019-08-08 13:25 ` [PATCH 2/2] serial: mctrl_gpio: Support all GPIO suffixes (gpios vs gpio) Stefan Roese
2019-08-08 13:48   ` Andy Shevchenko
2019-08-08 13:59     ` Stefan Roese
2019-08-12 10:53       ` Andy Shevchenko
2019-08-13 11:42         ` Pavel Machek
2019-08-13 12:15           ` Andy Shevchenko
2019-08-12 11:17   ` Geert Uytterhoeven
2019-08-12 11:53     ` Stefan Roese
2019-08-12 12:11       ` Geert Uytterhoeven
2019-08-12 12:31         ` Andy Shevchenko
2019-08-08 13:44 ` [PATCH 1/2] gpiolib: Add for_each_gpio_suffix() helper Andy Shevchenko
2019-08-08 14:13   ` Stefan Roese
2019-08-10  8:27 ` Linus Walleij
2019-08-10  8:48   ` Greg Kroah-Hartman
2019-08-12 11:18   ` Geert Uytterhoeven
2019-08-14  8:48     ` Linus Walleij
2019-08-14 13:17       ` Stefan Roese
2019-08-14 13:32         ` Geert Uytterhoeven
2019-08-14 13:38         ` Andy Shevchenko

Linux-GPIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-gpio/0 linux-gpio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-gpio linux-gpio/ https://lore.kernel.org/linux-gpio \
		linux-gpio@vger.kernel.org linux-gpio@archiver.kernel.org
	public-inbox-index linux-gpio


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-gpio


AGPL code for this site: git clone https://public-inbox.org/ public-inbox