* [PATCH v1 0/3] gpio: aggregator: Further improvements
@ 2020-07-18 21:26 Andy Shevchenko
2020-07-18 21:26 ` [PATCH v1 1/3] gpio: aggregator: Refactor ->{get,set}_multiple() to make Sparse happy Andy Shevchenko
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Andy Shevchenko @ 2020-07-18 21:26 UTC (permalink / raw)
To: Linus Walleij, Bartosz Golaszewski, linux-gpio, Geert Uytterhoeven
Cc: 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v1 1/3] gpio: aggregator: Refactor ->{get,set}_multiple() to make Sparse happy
2020-07-18 21:26 [PATCH v1 0/3] gpio: aggregator: Further improvements Andy Shevchenko
@ 2020-07-18 21:26 ` Andy Shevchenko
2020-08-05 14:56 ` Geert Uytterhoeven
2020-07-18 21:26 ` [PATCH v1 2/3] gpio: aggregator: Simplify isrange() by using get_option() Andy Shevchenko
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2020-07-18 21:26 UTC (permalink / raw)
To: Linus Walleij, Bartosz Golaszewski, linux-gpio, Geert Uytterhoeven
Cc: 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v1 2/3] gpio: aggregator: Simplify isrange() by using get_option()
2020-07-18 21:26 [PATCH v1 0/3] gpio: aggregator: Further improvements Andy Shevchenko
2020-07-18 21:26 ` [PATCH v1 1/3] gpio: aggregator: Refactor ->{get,set}_multiple() to make Sparse happy Andy Shevchenko
@ 2020-07-18 21:26 ` Andy Shevchenko
2020-08-05 14:37 ` Geert Uytterhoeven
2020-07-18 21:26 ` [PATCH v1 3/3] gpio: aggregator: Assign name and offsets only once in a loop Andy Shevchenko
2020-07-21 9:46 ` [PATCH v1 0/3] gpio: aggregator: Further improvements Linus Walleij
3 siblings, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2020-07-18 21:26 UTC (permalink / raw)
To: Linus Walleij, Bartosz Golaszewski, linux-gpio, Geert Uytterhoeven
Cc: 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v1 3/3] gpio: aggregator: Assign name and offsets only once in a loop
2020-07-18 21:26 [PATCH v1 0/3] gpio: aggregator: Further improvements Andy Shevchenko
2020-07-18 21:26 ` [PATCH v1 1/3] gpio: aggregator: Refactor ->{get,set}_multiple() to make Sparse happy Andy Shevchenko
2020-07-18 21:26 ` [PATCH v1 2/3] gpio: aggregator: Simplify isrange() by using get_option() Andy Shevchenko
@ 2020-07-18 21:26 ` Andy Shevchenko
2020-07-19 10:12 ` Andy Shevchenko
2020-07-21 9:46 ` [PATCH v1 0/3] gpio: aggregator: Further improvements Linus Walleij
3 siblings, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2020-07-18 21:26 UTC (permalink / raw)
To: Linus Walleij, Bartosz Golaszewski, linux-gpio, Geert Uytterhoeven
Cc: 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v1 3/3] gpio: aggregator: Assign name and offsets only once in a loop
2020-07-18 21:26 ` [PATCH v1 3/3] gpio: aggregator: Assign name and offsets only once in a loop Andy Shevchenko
@ 2020-07-19 10:12 ` Andy Shevchenko
0 siblings, 0 replies; 9+ messages in thread
From: Andy Shevchenko @ 2020-07-19 10:12 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Linus Walleij, Bartosz Golaszewski, open list:GPIO SUBSYSTEM,
Geert Uytterhoeven
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 0/3] gpio: aggregator: Further improvements
2020-07-18 21:26 [PATCH v1 0/3] gpio: aggregator: Further improvements Andy Shevchenko
` (2 preceding siblings ...)
2020-07-18 21:26 ` [PATCH v1 3/3] gpio: aggregator: Assign name and offsets only once in a loop Andy Shevchenko
@ 2020-07-21 9:46 ` Linus Walleij
3 siblings, 0 replies; 9+ messages in thread
From: Linus Walleij @ 2020-07-21 9:46 UTC (permalink / raw)
To: Andy Shevchenko, Geert Uytterhoeven
Cc: Bartosz Golaszewski, open list:GPIO SUBSYSTEM
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 2/3] gpio: aggregator: Simplify isrange() by using get_option()
2020-07-18 21:26 ` [PATCH v1 2/3] gpio: aggregator: Simplify isrange() by using get_option() Andy Shevchenko
@ 2020-08-05 14:37 ` Geert Uytterhoeven
0 siblings, 0 replies; 9+ messages in thread
From: Geert Uytterhoeven @ 2020-08-05 14:37 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Linus Walleij, Bartosz Golaszewski, open list:GPIO SUBSYSTEM
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/3] gpio: aggregator: Refactor ->{get,set}_multiple() to make Sparse happy
2020-07-18 21:26 ` [PATCH v1 1/3] gpio: aggregator: Refactor ->{get,set}_multiple() to make Sparse happy Andy Shevchenko
@ 2020-08-05 14:56 ` Geert Uytterhoeven
2020-08-10 11:09 ` Andy Shevchenko
0 siblings, 1 reply; 9+ messages in thread
From: Geert Uytterhoeven @ 2020-08-05 14:56 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Linus Walleij, Bartosz Golaszewski, open list:GPIO SUBSYSTEM
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/3] gpio: aggregator: Refactor ->{get,set}_multiple() to make Sparse happy
2020-08-05 14:56 ` Geert Uytterhoeven
@ 2020-08-10 11:09 ` Andy Shevchenko
0 siblings, 0 replies; 9+ messages in thread
From: Andy Shevchenko @ 2020-08-10 11:09 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Linus Walleij, Bartosz Golaszewski, open list:GPIO SUBSYSTEM
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-08-10 11:33 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-18 21:26 [PATCH v1 0/3] gpio: aggregator: Further improvements Andy Shevchenko
2020-07-18 21:26 ` [PATCH v1 1/3] gpio: aggregator: Refactor ->{get,set}_multiple() to make Sparse happy Andy Shevchenko
2020-08-05 14:56 ` Geert Uytterhoeven
2020-08-10 11:09 ` Andy Shevchenko
2020-07-18 21:26 ` [PATCH v1 2/3] gpio: aggregator: Simplify isrange() by using get_option() Andy Shevchenko
2020-08-05 14:37 ` Geert Uytterhoeven
2020-07-18 21:26 ` [PATCH v1 3/3] gpio: aggregator: Assign name and offsets only once in a loop Andy Shevchenko
2020-07-19 10:12 ` Andy Shevchenko
2020-07-21 9:46 ` [PATCH v1 0/3] gpio: aggregator: Further improvements Linus Walleij
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).