| Message ID | 20200623145748.28877-3-geert+renesas@glider.be |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: aggregator: Misc parsing improvements | expand |
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
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.
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
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);
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(-)