| Message ID | 20190808132543.26274-1-sr@denx.de |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] gpiolib: Add for_each_gpio_suffix() helper | expand |
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)
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
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
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
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
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
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
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
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?
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,
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(+)