[1/2] gpiolib: Add for_each_gpio_suffix() helper
diff mbox series

Message ID 20190808132543.26274-1-sr@denx.de
State New
Headers show
Series
  • [1/2] gpiolib: Add for_each_gpio_suffix() helper
Related show

Commit Message

Stefan Roese Aug. 8, 2019, 1:25 p.m. UTC
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(+)

Comments

Andy Shevchenko Aug. 8, 2019, 1:44 p.m. UTC | #1
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)
Stefan Roese Aug. 8, 2019, 2:13 p.m. UTC | #2
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
Linus Walleij Aug. 10, 2019, 8:27 a.m. UTC | #3
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
Greg KH Aug. 10, 2019, 8:48 a.m. UTC | #4
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
Geert Uytterhoeven Aug. 12, 2019, 11:18 a.m. UTC | #5
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
Linus Walleij Aug. 14, 2019, 8:48 a.m. UTC | #6
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
Stefan Roese Aug. 14, 2019, 1:17 p.m. UTC | #7
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
Geert Uytterhoeven Aug. 14, 2019, 1:32 p.m. UTC | #8
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
Andy Shevchenko Aug. 14, 2019, 1:38 p.m. UTC | #9
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?

Patch
diff mbox series

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,