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/[email protected]
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 -- [email protected]
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
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 <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Geert Uytterhoeven <[email protected]>
---
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
wt., 23 cze 2020 o 16:57 Geert Uytterhoeven <[email protected]>
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 <[email protected]>
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> ---
> 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
On Wed, Jun 24, 2020 at 3:16 PM Bartosz Golaszewski
<[email protected]> wrote:
>
> wt., 23 cze 2020 o 16:57 Geert Uytterhoeven <[email protected]>
> 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 <[email protected]>
> > Signed-off-by: Geert Uytterhoeven <[email protected]>
> > ---
> > 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
Hi Andy,
On Wed, Jun 24, 2020 at 3:41 PM Andy Shevchenko
<[email protected]> wrote:
> On Wed, Jun 24, 2020 at 3:16 PM Bartosz Golaszewski
> <[email protected]> wrote:
> > wt., 23 cze 2020 o 16:57 Geert Uytterhoeven <[email protected]>
> > 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 <[email protected]>
> > > Signed-off-by: Geert Uytterhoeven <[email protected]>
> > > ---
> > > 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/[email protected]
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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