Linux-LEDs Archive on lore.kernel.org
 help / color / Atom feed
From: Nathan Chancellor <natechancellor@gmail.com>
To: Arnd Bergmann <arnd@arndb.de>
Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>,
	Pavel Machek <pavel@ucw.cz>, Bryan Wu <bryan.wu@canonical.com>,
	"G.Shark Jeong" <gshark.jeong@gmail.com>,
	Dan Murphy <dmurphy@ti.com>,
	Linus Walleij <linus.walleij@linaro.org>,
	linux-leds@vger.kernel.org,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	clang-built-linux <clang-built-linux@googlegroups.com>
Subject: Re: [PATCH] leds: lm355x: avoid enum conversion warning
Date: Wed, 6 May 2020 08:35:50 -0700
Message-ID: <20200506153550.GB1213645@ubuntu-s3-xlarge-x86> (raw)
In-Reply-To: <CAK8P3a3wqiXAx2GuKJjY90qLGNnyBNTst6LW7n6tRDvzvBQVVQ@mail.gmail.com>

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

      reply index

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-05 14:19 Arnd Bergmann
2020-05-06  2:44 ` Nathan Chancellor
2020-05-06 14:19   ` Arnd Bergmann
2020-05-06 15:35     ` Nathan Chancellor [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200506153550.GB1213645@ubuntu-s3-xlarge-x86 \
    --to=natechancellor@gmail.com \
    --cc=arnd@arndb.de \
    --cc=bryan.wu@canonical.com \
    --cc=clang-built-linux@googlegroups.com \
    --cc=dmurphy@ti.com \
    --cc=gshark.jeong@gmail.com \
    --cc=jacek.anaszewski@gmail.com \
    --cc=linus.walleij@linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-leds@vger.kernel.org \
    --cc=pavel@ucw.cz \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-LEDs Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-leds/0 linux-leds/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-leds linux-leds/ https://lore.kernel.org/linux-leds \
		linux-leds@vger.kernel.org
	public-inbox-index linux-leds

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-leds


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git