[PATCH 0/2] gpio: aggregator: Misc parsing improvements

[PATCH 0/2] gpio: aggregator: Misc parsing improvements

[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

[PATCH 1/2] gpio: aggregator: Drop pre-initialization in get_arg()

[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

[PATCH 2/2] gpio: aggregator: Use bitmap_parselist() for parsing GPIO offsets

[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

Re: [PATCH 2/2] gpio: aggregator: Use bitmap_parselist() for parsing GPIO offsets

[Bartosz Golaszewski]

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

Re: [PATCH 2/2] gpio: aggregator: Use bitmap_parselist() for parsing GPIO offsets

[Andy Shevchenko]

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

Re: [PATCH 2/2] gpio: aggregator: Use bitmap_parselist() for parsing GPIO offsets

[Geert Uytterhoeven]

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