[PATCH v1 0/3] gpio: aggregator: Further improvements

[PATCH v1 0/3] gpio: aggregator: Further improvements

[Andy Shevchenko]

Patch 1 makes sparse happy about locking and, in my opinion, improves
readability, though increases LOC count.
Patch 2 simplifies parser by using existing helpers.
Patch 3 refactors loop in parser for better readability in my opinion.

It might be you have different opinions about patches 1 and 3, we may discuss.

Compile tested only.

Andy Shevchenko (3):
  gpio: aggregator: Refactor ->{get,set}_multiple() to make Sparse happy
  gpio: aggregator: Simplify isrange() by  using get_option()
  gpio: aggregator: Assign name and offsets only once in a loop

 drivers/gpio/gpio-aggregator.c | 110 +++++++++++++++++----------------
 1 file changed, 57 insertions(+), 53 deletions(-)

-- 
2.27.0

[PATCH v1 1/3] gpio: aggregator: Refactor ->{get,set}_multiple() to make Sparse happy

[Andy Shevchenko]

Sparse can't see locking scheme used in ->get_multiple() and
->set_multiple() callbacks.
  CHECK   .../drivers/gpio/gpio-aggregator.c
  .../spinlock.h:409:9: warning: context imbalance in 'gpio_fwd_get_multiple' - unexpected unlock
  .../spinlock.h:409:9: warning: context imbalance in 'gpio_fwd_set_multiple' - unexpected unlock

Refactor them to have better readability and make Sparse happy.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-aggregator.c | 70 +++++++++++++++++++++-------------
 1 file changed, 43 insertions(+), 27 deletions(-)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 424a3d25350b..5be166e73381 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -333,20 +333,14 @@ static int gpio_fwd_get(struct gpio_chip *chip, unsigned int offset)
 	return gpiod_get_value(fwd->descs[offset]);
 }
 
-static int gpio_fwd_get_multiple(struct gpio_chip *chip, unsigned long *mask,
+static int gpio_fwd_get_multiple(struct gpiochip_fwd *fwd, unsigned long *mask,
 				 unsigned long *bits)
 {
-	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
-	unsigned long *values, flags = 0;
 	struct gpio_desc **descs;
+	unsigned long *values;
 	unsigned int i, j = 0;
 	int error;
 
-	if (chip->can_sleep)
-		mutex_lock(&fwd->mlock);
-	else
-		spin_lock_irqsave(&fwd->slock, flags);
-
 	/* Both values bitmap and desc pointers are stored in tmp[] */
 	values = &fwd->tmp[0];
 	descs = (void *)&fwd->tmp[BITS_TO_LONGS(fwd->chip.ngpio)];
@@ -356,16 +350,32 @@ static int gpio_fwd_get_multiple(struct gpio_chip *chip, unsigned long *mask,
 		descs[j++] = fwd->descs[i];
 
 	error = gpiod_get_array_value(j, descs, NULL, values);
-	if (!error) {
-		j = 0;
-		for_each_set_bit(i, mask, fwd->chip.ngpio)
-			__assign_bit(i, bits, test_bit(j++, values));
-	}
+	if (error)
+		return error;
 
-	if (chip->can_sleep)
+	j = 0;
+	for_each_set_bit(i, mask, fwd->chip.ngpio)
+		__assign_bit(i, bits, test_bit(j++, values));
+
+	return 0;
+}
+
+static int gpio_fwd_get_multiple_locked(struct gpio_chip *chip,
+				        unsigned long *mask, unsigned long *bits)
+{
+	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
+	unsigned long flags;
+	int error;
+
+	if (chip->can_sleep) {
+		mutex_lock(&fwd->mlock);
+		error = gpio_fwd_get_multiple(fwd, mask, bits);
 		mutex_unlock(&fwd->mlock);
-	else
+	} else {
+		spin_lock_irqsave(&fwd->slock, flags);
+		error = gpio_fwd_get_multiple(fwd, mask, bits);
 		spin_unlock_irqrestore(&fwd->slock, flags);
+	}
 
 	return error;
 }
@@ -377,19 +387,13 @@ static void gpio_fwd_set(struct gpio_chip *chip, unsigned int offset, int value)
 	gpiod_set_value(fwd->descs[offset], value);
 }
 
-static void gpio_fwd_set_multiple(struct gpio_chip *chip, unsigned long *mask,
+static void gpio_fwd_set_multiple(struct gpiochip_fwd *fwd, unsigned long *mask,
 				  unsigned long *bits)
 {
-	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
-	unsigned long *values, flags = 0;
 	struct gpio_desc **descs;
+	unsigned long *values;
 	unsigned int i, j = 0;
 
-	if (chip->can_sleep)
-		mutex_lock(&fwd->mlock);
-	else
-		spin_lock_irqsave(&fwd->slock, flags);
-
 	/* Both values bitmap and desc pointers are stored in tmp[] */
 	values = &fwd->tmp[0];
 	descs = (void *)&fwd->tmp[BITS_TO_LONGS(fwd->chip.ngpio)];
@@ -400,11 +404,23 @@ static void gpio_fwd_set_multiple(struct gpio_chip *chip, unsigned long *mask,
 	}
 
 	gpiod_set_array_value(j, descs, NULL, values);
+}
 
-	if (chip->can_sleep)
+static void gpio_fwd_set_multiple_locked(struct gpio_chip *chip,
+				         unsigned long *mask, unsigned long *bits)
+{
+	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
+	unsigned long flags;
+
+	if (chip->can_sleep) {
+		mutex_lock(&fwd->mlock);
+		gpio_fwd_set_multiple(fwd, mask, bits);
 		mutex_unlock(&fwd->mlock);
-	else
+	} else {
+		spin_lock_irqsave(&fwd->slock, flags);
+		gpio_fwd_set_multiple(fwd, mask, bits);
 		spin_unlock_irqrestore(&fwd->slock, flags);
+	}
 }
 
 static int gpio_fwd_set_config(struct gpio_chip *chip, unsigned int offset,
@@ -470,9 +486,9 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
 	chip->direction_input = gpio_fwd_direction_input;
 	chip->direction_output = gpio_fwd_direction_output;
 	chip->get = gpio_fwd_get;
-	chip->get_multiple = gpio_fwd_get_multiple;
+	chip->get_multiple = gpio_fwd_get_multiple_locked;
 	chip->set = gpio_fwd_set;
-	chip->set_multiple = gpio_fwd_set_multiple;
+	chip->set_multiple = gpio_fwd_set_multiple_locked;
 	chip->base = -1;
 	chip->ngpio = ngpios;
 	fwd->descs = descs;
-- 
2.27.0

[PATCH v1 2/3] gpio: aggregator: Simplify isrange() by using get_option()

[Andy Shevchenko]

We already have a nice helper called get_option() which can be used
to validate the input format. Simplify isrange() by using it.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-aggregator.c | 29 ++++++++---------------------
 1 file changed, 8 insertions(+), 21 deletions(-)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 5be166e73381..de9ae622ca23 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -64,30 +64,17 @@ static char *get_arg(char **args)
 
 static bool isrange(const char *s)
 {
-	size_t n;
+	char **args = (char **)&s;
+	int dummy;
+	int res;
 
-	if (IS_ERR_OR_NULL(s))
+	if (IS_ERR_OR_NULL(s) || *s == '\0')
 		return false;
 
-	while (1) {
-		n = strspn(s, "0123456789");
-		if (!n)
-			return false;
-
-		s += n;
-
-		switch (*s++) {
-		case '\0':
-			return true;
-
-		case '-':
-		case ',':
-			break;
-
-		default:
-			return false;
-		}
-	}
+	do {
+		res = get_option(args, &dummy);
+	} while (res);
+	return **args == '\0';
 }
 
 static int aggr_add_gpio(struct gpio_aggregator *aggr, const char *key,
-- 
2.27.0

[PATCH v1 3/3] gpio: aggregator: Assign name and offsets only once in a loop

[Andy Shevchenko]

The for-loop looks a bit hard to read when we extract two arguments
per iteration. The 'do {} while (true)' makes it easier to read
despite being infinite loop.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-aggregator.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index de9ae622ca23..962ec9373d6f 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -109,14 +109,17 @@ static int aggr_parse(struct gpio_aggregator *aggr)
 	if (!bitmap)
 		return -ENOMEM;
 
-	for (name = get_arg(&args), offsets = get_arg(&args); name;
-	     offsets = get_arg(&args)) {
+	do {
+		name = get_arg(&args);
+		if (!name)
+			break;
 		if (IS_ERR(name)) {
 			pr_err("Cannot get GPIO specifier: %pe\n", name);
 			error = PTR_ERR(name);
 			goto free_bitmap;
 		}
 
+		offsets = get_arg(&args);
 		if (!isrange(offsets)) {
 			/* Named GPIO line */
 			error = aggr_add_gpio(aggr, name, U16_MAX, &n);
@@ -139,9 +142,7 @@ static int aggr_parse(struct gpio_aggregator *aggr)
 			if (error)
 				goto free_bitmap;
 		}
-
-		name = get_arg(&args);
-	}
+	} while (true);
 
 	if (!n) {
 		pr_err("No GPIOs specified\n");
-- 
2.27.0

Re: [PATCH v1 3/3] gpio: aggregator: Assign name and offsets only once in a loop

[Andy Shevchenko]

On Sun, Jul 19, 2020 at 12:26 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> The for-loop looks a bit hard to read when we extract two arguments
> per iteration. The 'do {} while (true)' makes it easier to read
> despite being infinite loop.

This is not gonna work.
Please, discard this patch from the series.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 0/3] gpio: aggregator: Further improvements

[Linus Walleij]

On Sat, Jul 18, 2020 at 11:26 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> Patch 1 makes sparse happy about locking and, in my opinion, improves
> readability, though increases LOC count.
> Patch 2 simplifies parser by using existing helpers.
> Patch 3 refactors loop in parser for better readability in my opinion.

I see you retracted patch 3, but patch 1 & 2 looks good to me, I just would
like Geert to have a look at them first, Geert?

Yours,
Linus Walleij

Re: [PATCH v1 2/3] gpio: aggregator: Simplify isrange() by using get_option()

[Geert Uytterhoeven]

Hi Andy,

On Sat, Jul 18, 2020 at 11:26 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> We already have a nice helper called get_option() which can be used
> to validate the input format. Simplify isrange() by using it.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks for your patch!

> --- a/drivers/gpio/gpio-aggregator.c
> +++ b/drivers/gpio/gpio-aggregator.c
> @@ -64,30 +64,17 @@ static char *get_arg(char **args)
>
>  static bool isrange(const char *s)
>  {
> -       size_t n;
> +       char **args = (char **)&s;
> +       int dummy;
> +       int res;
>
> -       if (IS_ERR_OR_NULL(s))
> +       if (IS_ERR_OR_NULL(s) || *s == '\0')
>                 return false;
>
> -       while (1) {
> -               n = strspn(s, "0123456789");
> -               if (!n)
> -                       return false;
> -
> -               s += n;
> -
> -               switch (*s++) {
> -               case '\0':
> -                       return true;
> -
> -               case '-':
> -               case ',':
> -                       break;
> -
> -               default:
> -                       return false;
> -               }
> -       }
> +       do {
> +               res = get_option(args, &dummy);
> +       } while (res);
> +       return **args == '\0';
>  }

This change makes isrange() return true for all of the following,
although they are not valid ranges:
    -7
    --7
    7-
    7--9

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

Re: [PATCH v1 1/3] gpio: aggregator: Refactor ->{get,set}_multiple() to make Sparse happy

[Geert Uytterhoeven]

Hi Andy,

On Sat, Jul 18, 2020 at 11:26 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Sparse can't see locking scheme used in ->get_multiple() and
> ->set_multiple() callbacks.
>   CHECK   .../drivers/gpio/gpio-aggregator.c
>   .../spinlock.h:409:9: warning: context imbalance in 'gpio_fwd_get_multiple' - unexpected unlock
>   .../spinlock.h:409:9: warning: context imbalance in 'gpio_fwd_set_multiple' - unexpected unlock
>
> Refactor them to have better readability and make Sparse happy.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks for your patch!

> --- a/drivers/gpio/gpio-aggregator.c
> +++ b/drivers/gpio/gpio-aggregator.c

> +static int gpio_fwd_get_multiple_locked(struct gpio_chip *chip,
> +                                       unsigned long *mask, unsigned long *bits)

Trading one static analysis tool error for a different one? ;-)

ERROR: code indent should use tabs where possible

> @@ -400,11 +404,23 @@ static void gpio_fwd_set_multiple(struct gpio_chip *chip, unsigned long *mask,
>         }
>
>         gpiod_set_array_value(j, descs, NULL, values);
> +}
>
> -       if (chip->can_sleep)
> +static void gpio_fwd_set_multiple_locked(struct gpio_chip *chip,
> +                                        unsigned long *mask, unsigned long *bits)

ERROR: code indent should use tabs where possible

With the above checkpatch errors fixed:
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Code size impact is +52 bytes with arm-linux-gnueabihf-gcc 7.5.0.

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

Re: [PATCH v1 1/3] gpio: aggregator: Refactor ->{get,set}_multiple() to make Sparse happy

[Andy Shevchenko]

On Wed, Aug 05, 2020 at 04:56:06PM +0200, Geert Uytterhoeven wrote:
> On Sat, Jul 18, 2020 at 11:26 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:

...

> ERROR: code indent should use tabs where possible

...

> ERROR: code indent should use tabs where possible

> With the above checkpatch errors fixed:
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thanks, I'll send it separately, patch 2 needs to be thought thru a bit more.

-- 
With Best Regards,
Andy Shevchenko