[PATCH] leds: lm355x: avoid enum conversion warning

[PATCH] leds: lm355x: avoid enum conversion warning

[Arnd Bergmann]

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

Re: [PATCH] leds: lm355x: avoid enum conversion warning

[Nathan Chancellor]

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>

Re: [PATCH] leds: lm355x: avoid enum conversion warning

[Arnd Bergmann]

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

Re: [PATCH] leds: lm355x: avoid enum conversion warning

[Nathan Chancellor]

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

Re: [PATCH] leds: lm355x: avoid enum conversion warning

[Pavel Machek]

[-- 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 --]