* [PATCH 0/2] gpio: aggregator: Misc parsing improvements
@ 2020-06-23 14:57 Geert Uytterhoeven
2020-06-23 14:57 ` [PATCH 1/2] gpio: aggregator: Drop pre-initialization in get_arg() Geert Uytterhoeven
2020-06-23 14:57 ` [PATCH 2/2] gpio: aggregator: Use bitmap_parselist() for parsing GPIO offsets Geert Uytterhoeven
0 siblings, 2 replies; 6+ messages in thread
From: Geert Uytterhoeven @ 2020-06-23 14:57 UTC (permalink / raw)
To: Linus Walleij, Bartosz Golaszewski, Andy Shevchenko
Cc: linux-gpio, linux-kernel, Geert Uytterhoeven
Hi Linus, Bartosz, Andy,
This patch series contains two improvements for the parsing code in the
GPIO Aggregator.
The second one converts the driver to use bitmap_parselist() for parsing
GPIO offsets and/or ranges, as suggested by Andy[1].
Note that I'm not super happy with the mask[] array on the stack. But
there is no real limit on the number of GPIO lines provided by a single
gpiochip, except for the global ARCH_NR_GPIOS.
I also considered getting rid of the custom isrange() function.
However, the check for "!isrange(offsets)" cannot be replaced by a check
for bitmap_parselist() returning -EINVAL, as bitmap_parselist() happily
accepts an empty list. Hence I'm afraid isrange() has to stay.
Andy also suggested to use strstrip() in get_arg(). However, I see no
point in that, as get_arg() has to find the end of the actual parameter
anyway.
Thanks for your comments!
[1] https://lore.kernel.org/r/20200520121420.GA1867563@smile.fi.intel.com
Geert Uytterhoeven (2):
gpio: aggregator: Drop pre-initialization in get_arg()
gpio: aggregator: Use bitmap_parselist() for parsing GPIO offsets
drivers/gpio/gpio-aggregator.c | 46 ++++++++++++----------------------
1 file changed, 16 insertions(+), 30 deletions(-)
--
2.17.1
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] 6+ messages in thread* [PATCH 1/2] gpio: aggregator: Drop pre-initialization in get_arg() 2020-06-23 14:57 [PATCH 0/2] gpio: aggregator: Misc parsing improvements Geert Uytterhoeven @ 2020-06-23 14:57 ` Geert Uytterhoeven 2020-06-23 14:57 ` [PATCH 2/2] gpio: aggregator: Use bitmap_parselist() for parsing GPIO offsets Geert Uytterhoeven 1 sibling, 0 replies; 6+ messages in thread From: Geert Uytterhoeven @ 2020-06-23 14:57 UTC (permalink / raw) To: Linus Walleij, Bartosz Golaszewski, Andy Shevchenko Cc: linux-gpio, linux-kernel, Geert Uytterhoeven In get_arg(), the variable start is pre-initialized, but overwritten again in the first statement. Rework the assignment to not rely on pre-initialization, to make the code easier to read. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- drivers/gpio/gpio-aggregator.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c index 9b0adbdddbfccb30..62a3fcbd4b4bb106 100644 --- a/drivers/gpio/gpio-aggregator.c +++ b/drivers/gpio/gpio-aggregator.c @@ -38,9 +38,9 @@ static DEFINE_IDR(gpio_aggregator_idr); static char *get_arg(char **args) { - char *start = *args, *end; + char *start, *end; - start = skip_spaces(start); + start = skip_spaces(*args); if (!*start) return NULL; -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] gpio: aggregator: Use bitmap_parselist() for parsing GPIO offsets 2020-06-23 14:57 [PATCH 0/2] gpio: aggregator: Misc parsing improvements Geert Uytterhoeven 2020-06-23 14:57 ` [PATCH 1/2] gpio: aggregator: Drop pre-initialization in get_arg() Geert Uytterhoeven @ 2020-06-23 14:57 ` Geert Uytterhoeven 2020-06-24 12:16 ` Bartosz Golaszewski 1 sibling, 1 reply; 6+ messages in thread From: Geert Uytterhoeven @ 2020-06-23 14:57 UTC (permalink / raw) To: Linus Walleij, Bartosz Golaszewski, Andy Shevchenko Cc: linux-gpio, linux-kernel, Geert Uytterhoeven Replace the custom code to parse GPIO offsets and/or GPIO offset ranges by a call to bitmap_parselist(), and an iteration over the returned bit mask. This should have no impact on the format of the configuration parameters written to the "new_device" virtual file in sysfs. Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- I'm not super happy with the mask[] array, which is on the stack. But there is no real limit on the number of GPIO lines provided by a single gpiochip, except for the global ARCH_NR_GPIOS. --- drivers/gpio/gpio-aggregator.c | 42 ++++++++++++---------------------- 1 file changed, 14 insertions(+), 28 deletions(-) diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c index 62a3fcbd4b4bb106..7b2e7abaece9cdb1 100644 --- a/drivers/gpio/gpio-aggregator.c +++ b/drivers/gpio/gpio-aggregator.c @@ -10,6 +10,7 @@ #include <linux/bitmap.h> #include <linux/bitops.h> #include <linux/ctype.h> +#include <linux/gpio.h> #include <linux/gpio/consumer.h> #include <linux/gpio/driver.h> #include <linux/gpio/machine.h> @@ -111,9 +112,10 @@ static int aggr_add_gpio(struct gpio_aggregator *aggr, const char *key, static int aggr_parse(struct gpio_aggregator *aggr) { - unsigned int first_index, last_index, i, n = 0; - char *name, *offsets, *first, *last, *next; + DECLARE_BITMAP(mask, ARCH_NR_GPIOS); char *args = aggr->args; + unsigned int i, n = 0; + char *name, *offsets; int error; for (name = get_arg(&args), offsets = get_arg(&args); name; @@ -134,32 +136,16 @@ static int aggr_parse(struct gpio_aggregator *aggr) } /* GPIO chip + offset(s) */ - for (first = offsets; *first; first = next) { - next = strchrnul(first, ','); - if (*next) - *next++ = '\0'; - - last = strchr(first, '-'); - if (last) - *last++ = '\0'; - - if (kstrtouint(first, 10, &first_index)) { - pr_err("Cannot parse GPIO index %s\n", first); - return -EINVAL; - } - - if (!last) { - last_index = first_index; - } else if (kstrtouint(last, 10, &last_index)) { - pr_err("Cannot parse GPIO index %s\n", last); - return -EINVAL; - } - - for (i = first_index; i <= last_index; i++) { - error = aggr_add_gpio(aggr, name, i, &n); - if (error) - return error; - } + error = bitmap_parselist(offsets, mask, ARCH_NR_GPIOS); + if (error) { + pr_err("Cannot parse %s: %d\n", offsets, error); + return error; + } + + for_each_set_bit(i, mask, ARCH_NR_GPIOS) { + error = aggr_add_gpio(aggr, name, i, &n); + if (error) + return error; } name = get_arg(&args); -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] gpio: aggregator: Use bitmap_parselist() for parsing GPIO offsets 2020-06-23 14:57 ` [PATCH 2/2] gpio: aggregator: Use bitmap_parselist() for parsing GPIO offsets Geert Uytterhoeven @ 2020-06-24 12:16 ` Bartosz Golaszewski 2020-06-24 13:41 ` Andy Shevchenko 0 siblings, 1 reply; 6+ messages in thread From: Bartosz Golaszewski @ 2020-06-24 12:16 UTC (permalink / raw) To: Geert Uytterhoeven; +Cc: Linus Walleij, Andy Shevchenko, linux-gpio, LKML wt., 23 cze 2020 o 16:57 Geert Uytterhoeven <geert+renesas@glider.be> napisał(a): > > Replace the custom code to parse GPIO offsets and/or GPIO offset ranges > by a call to bitmap_parselist(), and an iteration over the returned bit > mask. > > This should have no impact on the format of the configuration parameters > written to the "new_device" virtual file in sysfs. > > Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > I'm not super happy with the mask[] array, which is on the stack. > But there is no real limit on the number of GPIO lines provided by a > single gpiochip, except for the global ARCH_NR_GPIOS. Why not allocate it with bitmap_zalloc() then? Bart ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] gpio: aggregator: Use bitmap_parselist() for parsing GPIO offsets 2020-06-24 12:16 ` Bartosz Golaszewski @ 2020-06-24 13:41 ` Andy Shevchenko 2020-06-27 7:57 ` Geert Uytterhoeven 0 siblings, 1 reply; 6+ messages in thread From: Andy Shevchenko @ 2020-06-24 13:41 UTC (permalink / raw) To: Bartosz Golaszewski; +Cc: Geert Uytterhoeven, Linus Walleij, linux-gpio, LKML On Wed, Jun 24, 2020 at 3:16 PM Bartosz Golaszewski <bgolaszewski@baylibre.com> wrote: > > wt., 23 cze 2020 o 16:57 Geert Uytterhoeven <geert+renesas@glider.be> > napisał(a): > > > > Replace the custom code to parse GPIO offsets and/or GPIO offset ranges > > by a call to bitmap_parselist(), and an iteration over the returned bit > > mask. > > > > This should have no impact on the format of the configuration parameters > > written to the "new_device" virtual file in sysfs. > > > > Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > --- > > I'm not super happy with the mask[] array, which is on the stack. > > But there is no real limit on the number of GPIO lines provided by a > > single gpiochip, except for the global ARCH_NR_GPIOS. > > Why not allocate it with bitmap_zalloc() then? I haven't got the original messages yet, so my thought is to actually extract a helper from gpiod_get_array_value_complex() or gpiod_set_array_value_complex() for bitmap allocation. But I didn't check if it's suitable here. So, bitmap_zalloc() would be helpful. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] gpio: aggregator: Use bitmap_parselist() for parsing GPIO offsets 2020-06-24 13:41 ` Andy Shevchenko @ 2020-06-27 7:57 ` Geert Uytterhoeven 0 siblings, 0 replies; 6+ messages in thread From: Geert Uytterhoeven @ 2020-06-27 7:57 UTC (permalink / raw) To: Andy Shevchenko; +Cc: Bartosz Golaszewski, Linus Walleij, linux-gpio, LKML Hi Andy, On Wed, Jun 24, 2020 at 3:41 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, Jun 24, 2020 at 3:16 PM Bartosz Golaszewski > <bgolaszewski@baylibre.com> wrote: > > wt., 23 cze 2020 o 16:57 Geert Uytterhoeven <geert+renesas@glider.be> > > napisał(a): > > > > > > Replace the custom code to parse GPIO offsets and/or GPIO offset ranges > > > by a call to bitmap_parselist(), and an iteration over the returned bit > > > mask. > > > > > > This should have no impact on the format of the configuration parameters > > > written to the "new_device" virtual file in sysfs. > > > > > > Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > --- > > > I'm not super happy with the mask[] array, which is on the stack. > > > But there is no real limit on the number of GPIO lines provided by a > > > single gpiochip, except for the global ARCH_NR_GPIOS. > > > > Why not allocate it with bitmap_zalloc() then? Bartosz: Thanks, good idea! > I haven't got the original messages yet, so my thought is to actually > extract a helper from > gpiod_get_array_value_complex() or gpiod_set_array_value_complex() for > bitmap allocation. > But I didn't check if it's suitable here. So, bitmap_zalloc() would be helpful. /me confused This function is not about getting/setting multiple GPIO lines in a gpiochip, but about parsing a list of numbers and ranges. No gpiochip is involved. original message: https://lore.kernel.org/r/20200623145748.28877-3-geert+renesas@glider.be 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] 6+ messages in thread
end of thread, other threads:[~2020-06-27 7:58 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-23 14:57 [PATCH 0/2] gpio: aggregator: Misc parsing improvements Geert Uytterhoeven 2020-06-23 14:57 ` [PATCH 1/2] gpio: aggregator: Drop pre-initialization in get_arg() Geert Uytterhoeven 2020-06-23 14:57 ` [PATCH 2/2] gpio: aggregator: Use bitmap_parselist() for parsing GPIO offsets Geert Uytterhoeven 2020-06-24 12:16 ` Bartosz Golaszewski 2020-06-24 13:41 ` Andy Shevchenko 2020-06-27 7:57 ` Geert Uytterhoeven
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).