* [PATCH] pinctrl: aspeed: fix regmap error handling
@ 2016-09-09 9:26 Arnd Bergmann
2016-09-12 1:22 ` Joel Stanley
2016-09-13 8:42 ` Linus Walleij
0 siblings, 2 replies; 4+ messages in thread
From: Arnd Bergmann @ 2016-09-09 9:26 UTC (permalink / raw)
To: Linus Walleij
Cc: Arnd Bergmann, Andrew Jeffery, Joel Stanley, linux-gpio, linux-kernel
The newly added aspeed driver tries to check for a negative return
value from a pinctrl function, but stores the intermediate value in
a 'bool' variable, which cannot work:
drivers/pinctrl/aspeed/pinctrl-aspeed.c: In function 'aspeed_sig_expr_set':
drivers/pinctrl/aspeed/pinctrl-aspeed.c:192:11: error: comparison of constant '0' with boolean expression is always false [-Werror=bool-compare]
This slightly reworks the logic to use an explicit comparison with zero
before assigning to the temporary variable.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/pinctrl/aspeed/pinctrl-aspeed.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.c b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
index 7d461fc30d3c..0391f9f13f3e 100644
--- a/drivers/pinctrl/aspeed/pinctrl-aspeed.c
+++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
@@ -187,10 +187,10 @@ static bool aspeed_sig_expr_set(const struct aspeed_sig_expr *expr,
continue;
ret = regmap_update_bits(map, desc->reg, desc->mask,
- pattern << __ffs(desc->mask));
+ pattern << __ffs(desc->mask)) == 0;
- if (ret < 0)
- return false;
+ if (!ret)
+ return ret;
}
return aspeed_sig_expr_eval(expr, enable, map);
--
2.9.0
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] pinctrl: aspeed: fix regmap error handling
2016-09-09 9:26 [PATCH] pinctrl: aspeed: fix regmap error handling Arnd Bergmann
@ 2016-09-12 1:22 ` Joel Stanley
2016-09-12 1:55 ` Andrew Jeffery
2016-09-13 8:42 ` Linus Walleij
1 sibling, 1 reply; 4+ messages in thread
From: Joel Stanley @ 2016-09-12 1:22 UTC (permalink / raw)
To: Arnd Bergmann, Linus Walleij
Cc: Andrew Jeffery, linux-gpio, linux-kernel, colin.king
On Fri, Sep 9, 2016 at 6:56 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> The newly added aspeed driver tries to check for a negative return
> value from a pinctrl function, but stores the intermediate value in
> a 'bool' variable, which cannot work:
>
> drivers/pinctrl/aspeed/pinctrl-aspeed.c: In function 'aspeed_sig_expr_set':
> drivers/pinctrl/aspeed/pinctrl-aspeed.c:192:11: error: comparison of constant '0' with boolean expression is always false [-Werror=bool-compare]
>
> This slightly reworks the logic to use an explicit comparison with zero
> before assigning to the temporary variable.
Thanks for finding this Arnd.
Colin also found the issue. Thanks Colin!
I think we should take this version of the fix. Linus, can you put
this in your tree please?
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Joel Stanley <joel@jms.id.au>
Cheers,
Joel
> ---
> drivers/pinctrl/aspeed/pinctrl-aspeed.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.c b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
> index 7d461fc30d3c..0391f9f13f3e 100644
> --- a/drivers/pinctrl/aspeed/pinctrl-aspeed.c
> +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
> @@ -187,10 +187,10 @@ static bool aspeed_sig_expr_set(const struct aspeed_sig_expr *expr,
> continue;
>
> ret = regmap_update_bits(map, desc->reg, desc->mask,
> - pattern << __ffs(desc->mask));
> + pattern << __ffs(desc->mask)) == 0;
>
> - if (ret < 0)
> - return false;
> + if (!ret)
> + return ret;
> }
>
> return aspeed_sig_expr_eval(expr, enable, map);
> --
> 2.9.0
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] pinctrl: aspeed: fix regmap error handling
2016-09-12 1:22 ` Joel Stanley
@ 2016-09-12 1:55 ` Andrew Jeffery
0 siblings, 0 replies; 4+ messages in thread
From: Andrew Jeffery @ 2016-09-12 1:55 UTC (permalink / raw)
To: Joel Stanley, Arnd Bergmann, Linus Walleij
Cc: linux-gpio, linux-kernel, colin.king
[-- Attachment #1: Type: text/plain, Size: 2513 bytes --]
On Mon, 2016-09-12 at 10:52 +0930, Joel Stanley wrote:
> On Fri, Sep 9, 2016 at 6:56 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > The newly added aspeed driver tries to check for a negative return
> > value from a pinctrl function, but stores the intermediate value in
> > a 'bool' variable, which cannot work:
> >
> > drivers/pinctrl/aspeed/pinctrl-aspeed.c: In function 'aspeed_sig_expr_set':
> > drivers/pinctrl/aspeed/pinctrl-aspeed.c:192:11: error: comparison of constant '0' with boolean expression is always false [-Werror=bool-compare]
> >
> > This slightly reworks the logic to use an explicit comparison with zero
> > before assigning to the temporary variable.
> Thanks for finding this Arnd.
>
> Colin also found the issue. Thanks Colin!
>
> I think we should take this version of the fix. Linus, can you put
> this in your tree please?
+1 on each point above.
I made a change to the loop body in response to feedback in one of the
earlier iterations and unfortunately this looks to have slipped past
me. I will look at adding a bit more static analysis to my patch
submission checklist!
>
> >
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Acked-by: Joel Stanley <joel@jms.id.au>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
>
> Cheers,
>
> Joel
>
> >
> > ---
> > drivers/pinctrl/aspeed/pinctrl-aspeed.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.c b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
> > index 7d461fc30d3c..0391f9f13f3e 100644
> > --- a/drivers/pinctrl/aspeed/pinctrl-aspeed.c
> > +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
> > @@ -187,10 +187,10 @@ static bool aspeed_sig_expr_set(const struct aspeed_sig_expr *expr,
> > continue;
> >
> > ret = regmap_update_bits(map, desc->reg, desc->mask,
> > - pattern << __ffs(desc->mask));
> > + pattern << __ffs(desc->mask)) == 0;
> >
> > - if (ret < 0)
> > - return false;
> > + if (!ret)
> > + return ret;
> > }
> >
> > return aspeed_sig_expr_eval(expr, enable, map);
> > --
> > 2.9.0
> >
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] pinctrl: aspeed: fix regmap error handling
2016-09-09 9:26 [PATCH] pinctrl: aspeed: fix regmap error handling Arnd Bergmann
2016-09-12 1:22 ` Joel Stanley
@ 2016-09-13 8:42 ` Linus Walleij
1 sibling, 0 replies; 4+ messages in thread
From: Linus Walleij @ 2016-09-13 8:42 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Andrew Jeffery, Joel Stanley, linux-gpio, linux-kernel
On Fri, Sep 9, 2016 at 11:26 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> The newly added aspeed driver tries to check for a negative return
> value from a pinctrl function, but stores the intermediate value in
> a 'bool' variable, which cannot work:
>
> drivers/pinctrl/aspeed/pinctrl-aspeed.c: In function 'aspeed_sig_expr_set':
> drivers/pinctrl/aspeed/pinctrl-aspeed.c:192:11: error: comparison of constant '0' with boolean expression is always false [-Werror=bool-compare]
>
> This slightly reworks the logic to use an explicit comparison with zero
> before assigning to the temporary variable.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Patch applied with the ACKs.
Also added Colin as Reported-by.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-09-13 8:42 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-09 9:26 [PATCH] pinctrl: aspeed: fix regmap error handling Arnd Bergmann
2016-09-12 1:22 ` Joel Stanley
2016-09-12 1:55 ` Andrew Jeffery
2016-09-13 8:42 ` Linus Walleij
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®