* [PATCH] leds: lm355x: avoid enum conversion warning
@ 2020-05-05 14:19 Arnd Bergmann
2020-05-06 2:44 ` Nathan Chancellor
0 siblings, 1 reply; 5+ messages in thread
From: Arnd Bergmann @ 2020-05-05 14:19 UTC (permalink / raw)
To: Jacek Anaszewski, Pavel Machek, Bryan Wu, G.Shark Jeong
Cc: Arnd Bergmann, Dan Murphy, Linus Walleij, linux-leds,
linux-kernel, clang-built-linux
clang points out that doing arithmetic between diffent enums is usually
a mistake:
drivers/leds/leds-lm355x.c:167:28: warning: bitwise operation between different enumeration types ('enum lm355x_tx2' and 'enum lm355x_ntc') [-Wenum-enum-conversion]
reg_val = pdata->pin_tx2 | pdata->ntc_pin;
~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~
drivers/leds/leds-lm355x.c:178:28: warning: bitwise operation between different enumeration types ('enum lm355x_tx2' and 'enum lm355x_ntc') [-Wenum-enum-conversion]
reg_val = pdata->pin_tx2 | pdata->ntc_pin | pdata->pass_mode;
~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~
In this driver, it is intentional, so add a cast to hide the false-positive
warning. It appears to be the only instance of this warning at the moment.
Fixes: b98d13c72592 ("leds: Add new LED driver for lm355x chips")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/leds/leds-lm355x.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/leds/leds-lm355x.c b/drivers/leds/leds-lm355x.c
index 11ce05249751..b2eb2e1e9c04 100644
--- a/drivers/leds/leds-lm355x.c
+++ b/drivers/leds/leds-lm355x.c
@@ -164,18 +164,19 @@ static int lm355x_chip_init(struct lm355x_chip_data *chip)
/* input and output pins configuration */
switch (chip->type) {
case CHIP_LM3554:
- reg_val = pdata->pin_tx2 | pdata->ntc_pin;
+ reg_val = (u32)pdata->pin_tx2 | (u32)pdata->ntc_pin;
ret = regmap_update_bits(chip->regmap, 0xE0, 0x28, reg_val);
if (ret < 0)
goto out;
- reg_val = pdata->pass_mode;
+ reg_val = (u32)pdata->pass_mode;
ret = regmap_update_bits(chip->regmap, 0xA0, 0x04, reg_val);
if (ret < 0)
goto out;
break;
case CHIP_LM3556:
- reg_val = pdata->pin_tx2 | pdata->ntc_pin | pdata->pass_mode;
+ reg_val = (u32)pdata->pin_tx2 | (u32)pdata->ntc_pin |
+ (u32)pdata->pass_mode;
ret = regmap_update_bits(chip->regmap, 0x0A, 0xC4, reg_val);
if (ret < 0)
goto out;
--
2.26.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] leds: lm355x: avoid enum conversion warning
2020-05-05 14:19 [PATCH] leds: lm355x: avoid enum conversion warning Arnd Bergmann
@ 2020-05-06 2:44 ` Nathan Chancellor
2020-05-06 14:19 ` Arnd Bergmann
0 siblings, 1 reply; 5+ messages in thread
From: Nathan Chancellor @ 2020-05-06 2:44 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Jacek Anaszewski, Pavel Machek, Bryan Wu, G.Shark Jeong,
Dan Murphy, Linus Walleij, linux-leds, linux-kernel,
clang-built-linux
On Tue, May 05, 2020 at 04:19:17PM +0200, Arnd Bergmann wrote:
> clang points out that doing arithmetic between diffent enums is usually
^ different
> a mistake:
>
> drivers/leds/leds-lm355x.c:167:28: warning: bitwise operation between different enumeration types ('enum lm355x_tx2' and 'enum lm355x_ntc') [-Wenum-enum-conversion]
> reg_val = pdata->pin_tx2 | pdata->ntc_pin;
> ~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~
> drivers/leds/leds-lm355x.c:178:28: warning: bitwise operation between different enumeration types ('enum lm355x_tx2' and 'enum lm355x_ntc') [-Wenum-enum-conversion]
> reg_val = pdata->pin_tx2 | pdata->ntc_pin | pdata->pass_mode;
> ~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~
>
> In this driver, it is intentional, so add a cast to hide the false-positive
Not sure that I would call this a false positive. The warning is correct
that there is a bitwise operation between different enumeration types
but we do not care since we are just using the enumerated type for its
integer value in lieu of a #define VAR value.
> warning. It appears to be the only instance of this warning at the moment.
>
> Fixes: b98d13c72592 ("leds: Add new LED driver for lm355x chips")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/leds/leds-lm355x.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/leds/leds-lm355x.c b/drivers/leds/leds-lm355x.c
> index 11ce05249751..b2eb2e1e9c04 100644
> --- a/drivers/leds/leds-lm355x.c
> +++ b/drivers/leds/leds-lm355x.c
> @@ -164,18 +164,19 @@ static int lm355x_chip_init(struct lm355x_chip_data *chip)
> /* input and output pins configuration */
> switch (chip->type) {
> case CHIP_LM3554:
> - reg_val = pdata->pin_tx2 | pdata->ntc_pin;
> + reg_val = (u32)pdata->pin_tx2 | (u32)pdata->ntc_pin;
> ret = regmap_update_bits(chip->regmap, 0xE0, 0x28, reg_val);
> if (ret < 0)
> goto out;
> - reg_val = pdata->pass_mode;
> + reg_val = (u32)pdata->pass_mode;
Is this cast needed? I don't think there should be warning from going
from an enumerated type to unsigned int.
> ret = regmap_update_bits(chip->regmap, 0xA0, 0x04, reg_val);
> if (ret < 0)
> goto out;
> break;
>
> case CHIP_LM3556:
> - reg_val = pdata->pin_tx2 | pdata->ntc_pin | pdata->pass_mode;
> + reg_val = (u32)pdata->pin_tx2 | (u32)pdata->ntc_pin |
> + (u32)pdata->pass_mode;
> ret = regmap_update_bits(chip->regmap, 0x0A, 0xC4, reg_val);
> if (ret < 0)
> goto out;
> --
> 2.26.0
>
With those comments addressed, feel free to add:
Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] leds: lm355x: avoid enum conversion warning
2020-05-06 2:44 ` Nathan Chancellor
@ 2020-05-06 14:19 ` Arnd Bergmann
2020-05-06 15:35 ` Nathan Chancellor
2020-06-04 13:28 ` Pavel Machek
0 siblings, 2 replies; 5+ messages in thread
From: Arnd Bergmann @ 2020-05-06 14:19 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Jacek Anaszewski, Pavel Machek, Bryan Wu, G.Shark Jeong,
Dan Murphy, Linus Walleij, linux-leds, linux-kernel,
clang-built-linux
On Wed, May 6, 2020 at 4:44 AM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> On Tue, May 05, 2020 at 04:19:17PM +0200, Arnd Bergmann wrote:
> > clang points out that doing arithmetic between diffent enums is usually
> ^ different
> > a mistake:
> >
> > drivers/leds/leds-lm355x.c:167:28: warning: bitwise operation between different enumeration types ('enum lm355x_tx2' and 'enum lm355x_ntc') [-Wenum-enum-conversion]
> > reg_val = pdata->pin_tx2 | pdata->ntc_pin;
> > ~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~
> > drivers/leds/leds-lm355x.c:178:28: warning: bitwise operation between different enumeration types ('enum lm355x_tx2' and 'enum lm355x_ntc') [-Wenum-enum-conversion]
> > reg_val = pdata->pin_tx2 | pdata->ntc_pin | pdata->pass_mode;
> > ~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~
> >
> > In this driver, it is intentional, so add a cast to hide the false-positive
>
> Not sure that I would call this a false positive. The warning is correct
> that there is a bitwise operation between different enumeration types
> but we do not care since we are just using the enumerated type for its
> integer value in lieu of a #define VAR value.
Right, I meant that the code works as intended and said "false positive"
to avoid claiming the driver is broken when this was a deliberate
design point.
We do want clang to warn about this though as you say, so I can
rephrase it to explain that both the driver and the compiler work
as intended but they clash in their views of how to do it ;-)
> > - reg_val = pdata->pass_mode;
> > + reg_val = (u32)pdata->pass_mode;
>
> Is this cast needed? I don't think there should be warning from going
> from an enumerated type to unsigned int.
This cast is not needed for warnings, I added it for consistency because
it seemed odd to cast only four of the five enums. I can remove if though
if you think it's clearer without the cast.
There may also be a different solution in completely removing the
lm355x_chip_init() function, as it was added at a time when we
were converting the last board files into devicetree, and there has
never been a board file defining lm355x_platform_data.
There is unfortunately no DT support either in it, so I assume we
could just remove the driver completely, or change it to use a
DT binding similar to
Documentation/devicetree/bindings/leds/leds-lm36*.txt
LED maintainers, any opinions on this?
Arnd
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] leds: lm355x: avoid enum conversion warning
2020-05-06 14:19 ` Arnd Bergmann
@ 2020-05-06 15:35 ` Nathan Chancellor
2020-06-04 13:28 ` Pavel Machek
1 sibling, 0 replies; 5+ messages in thread
From: Nathan Chancellor @ 2020-05-06 15:35 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Jacek Anaszewski, Pavel Machek, Bryan Wu, G.Shark Jeong,
Dan Murphy, Linus Walleij, linux-leds, linux-kernel,
clang-built-linux
On Wed, May 06, 2020 at 04:19:45PM +0200, Arnd Bergmann wrote:
> On Wed, May 6, 2020 at 4:44 AM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> >
> > On Tue, May 05, 2020 at 04:19:17PM +0200, Arnd Bergmann wrote:
> > > clang points out that doing arithmetic between diffent enums is usually
> > ^ different
> > > a mistake:
> > >
> > > drivers/leds/leds-lm355x.c:167:28: warning: bitwise operation between different enumeration types ('enum lm355x_tx2' and 'enum lm355x_ntc') [-Wenum-enum-conversion]
> > > reg_val = pdata->pin_tx2 | pdata->ntc_pin;
> > > ~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~
> > > drivers/leds/leds-lm355x.c:178:28: warning: bitwise operation between different enumeration types ('enum lm355x_tx2' and 'enum lm355x_ntc') [-Wenum-enum-conversion]
> > > reg_val = pdata->pin_tx2 | pdata->ntc_pin | pdata->pass_mode;
> > > ~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~
> > >
> > > In this driver, it is intentional, so add a cast to hide the false-positive
> >
> > Not sure that I would call this a false positive. The warning is correct
> > that there is a bitwise operation between different enumeration types
> > but we do not care since we are just using the enumerated type for its
> > integer value in lieu of a #define VAR value.
>
> Right, I meant that the code works as intended and said "false positive"
> to avoid claiming the driver is broken when this was a deliberate
> design point.
Ack.
> We do want clang to warn about this though as you say, so I can
> rephrase it to explain that both the driver and the compiler work
> as intended but they clash in their views of how to do it ;-)
Yes, that would be good if we don't go a different direction based on
your commends below.
> > > - reg_val = pdata->pass_mode;
> > > + reg_val = (u32)pdata->pass_mode;
> >
> > Is this cast needed? I don't think there should be warning from going
> > from an enumerated type to unsigned int.
>
> This cast is not needed for warnings, I added it for consistency because
> it seemed odd to cast only four of the five enums. I can remove if though
> if you think it's clearer without the cast.
I don't really have a strong opinion but I do think that not having the
cast makes the patch a little more specific/precise.
> There may also be a different solution in completely removing the
> lm355x_chip_init() function, as it was added at a time when we
> were converting the last board files into devicetree, and there has
> never been a board file defining lm355x_platform_data.
>
> There is unfortunately no DT support either in it, so I assume we
> could just remove the driver completely, or change it to use a
> DT binding similar to
> Documentation/devicetree/bindings/leds/leds-lm36*.txt
>
> LED maintainers, any opinions on this?
>
> Arnd
Cheers,
Nathan
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] leds: lm355x: avoid enum conversion warning
2020-05-06 14:19 ` Arnd Bergmann
2020-05-06 15:35 ` Nathan Chancellor
@ 2020-06-04 13:28 ` Pavel Machek
1 sibling, 0 replies; 5+ messages in thread
From: Pavel Machek @ 2020-06-04 13:28 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Nathan Chancellor, Jacek Anaszewski, Bryan Wu, G.Shark Jeong,
Dan Murphy, Linus Walleij, linux-leds, linux-kernel,
clang-built-linux
[-- Attachment #1: Type: text/plain, Size: 1355 bytes --]
Hi!
> > > - reg_val = pdata->pass_mode;
> > > + reg_val = (u32)pdata->pass_mode;
> >
> > Is this cast needed? I don't think there should be warning from going
> > from an enumerated type to unsigned int.
>
> This cast is not needed for warnings, I added it for consistency because
> it seemed odd to cast only four of the five enums. I can remove if though
> if you think it's clearer without the cast.
>
> There may also be a different solution in completely removing the
> lm355x_chip_init() function, as it was added at a time when we
> were converting the last board files into devicetree, and there has
> never been a board file defining lm355x_platform_data.
>
> There is unfortunately no DT support either in it, so I assume we
> could just remove the driver completely, or change it to use a
> DT binding similar to
> Documentation/devicetree/bindings/leds/leds-lm36*.txt
>
> LED maintainers, any opinions on this?
I believe it is too soon to remove the driver; even if new user will
have to add DT support, that is less work than starting from scratch.
I guess I'll just apply your patch to squelch the warnings.
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-06-04 13:28 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-05 14:19 [PATCH] leds: lm355x: avoid enum conversion warning Arnd Bergmann
2020-05-06 2:44 ` Nathan Chancellor
2020-05-06 14:19 ` Arnd Bergmann
2020-05-06 15:35 ` Nathan Chancellor
2020-06-04 13:28 ` Pavel Machek
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.