Re: [jic23-iio:testing 6/10] drivers/iio/adc/max14001.c:135:13: warning: result of comparison of constant 18446744073709551615 with expression of type 'typeof (_Generic((mask), char: (unsigned char)0, unsigned char: (unsigned char)0, signed char: (unsigned char)0, unsigned short: (un...

Re: [jic23-iio:testing 6/10] drivers/iio/adc/max14001.c:135:13: warning: result of comparison of constant 18446744073709551615 with expression of type 'typeof (_Generic((mask), char: (unsigned char)0, unsigned char: (unsigned char)0, signed char: (unsigned char)0, unsigned short: (un...

[Jonathan Cameron]

On Thu, 20 Jul 2023 11:29:55 +0200
"Arnd Bergmann" <arnd@arndb.de> wrote:

> On Wed, Jun 21, 2023, at 10:25, Andy Shevchenko wrote:
> > On Wed, Jun 21, 2023 at 10:19 AM kernel test robot <lkp@intel.com> wrote:  
> >>
> >> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git testing
> >> head:   25e201cc6ff270abc062e13ff912292097cb2827
> >> commit: d3e93b67f934a477c5851d575a2278f07c6242fb [6/10] iio: adc: max14001: New driver
> >> config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20230621/202306211545.7b6CdqsL-lkp@intel.com/config)
> >> compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project.git 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
> >> reproduce: (https://download.01.org/0day-ci/archive/20230621/202306211545.7b6CdqsL-lkp@intel.com/reproduce)
> >>
> >> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> >> the same patch/commit), kindly add following tags
> >> | Reported-by: kernel test robot <lkp@intel.com>
> >> | Closes: https://lore.kernel.org/oe-kbuild-all/202306211545.7b6CdqsL-lkp@intel.com/
> >>
> >> All warnings (new ones prefixed by >>):  
> >
> > Okay, you even haven't compiled your code :-(
> > This should probably use one of the functions from bitfield.h.
> > Like u32_encode_bits().  
> 
> I see the warning is now in linux-next. I see it only shows up with
> clang and not gcc, but it would be nice to address it.

Sorry, I managed to forget this one was still outstanding and picked up the driver.

> 
> >  
> >> >> drivers/iio/adc/max14001.c:135:13: warning: result of comparison of constant 18446744073709551615 with expression of type 'typeof (_Generic((mask), char: (unsigned char)0, unsigned char: (unsigned char)0, signed char: (unsigned char)0, unsigned short: (unsigned short)0, short: (unsigned short)0, unsigned int: (unsigned int)0, int: (unsigned int)0, unsigned long: (unsigned long)0, long: (unsigned long)0, unsigned long long: (unsigned long long)0, long long: (unsigned long long)0, default: (mask)))' (aka 'unsigned int') is always false [-Wtautological-constant-out-of-range-compare]  
> >>            reg_data = FIELD_PREP(mask, val);
> >>                       ^~~~~~~~~~~~~~~~~~~~~
> >>    include/linux/bitfield.h:114:3: note: expanded from macro 'FIELD_PREP'
> >>                    __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: ");    \
> >>                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>    include/linux/bitfield.h:71:53: note: expanded from macro '__BF_FIELD_CHECK'
> >>                    BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) >     \
> >>                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
> >>    include/linux/build_bug.h:39:58: note: expanded from macro 'BUILD_BUG_ON_MSG'
> >>    #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
> >>                                        ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
> >>    include/linux/compiler_types.h:397:22: note: expanded from macro 'compiletime_assert'
> >>            _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> >>            ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>    include/linux/compiler_types.h:385:23: note: expanded from macro '_compiletime_assert'
> >>            __compiletime_assert(condition, msg, prefix, suffix)
> >>            ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>    include/linux/compiler_types.h:377:9: note: expanded from macro '__compiletime_assert'
> >>                    if (!(condition))                                       \
> >>                          ^~~~~~~~~
> >>    1 warning generated.  
> 
> It looks like the problem is the use of FIELD_PREP() with a variable
> 'val' instead of a constant. I tried marking the function __always_inline
> so the compiler can see the actual value ('1') that gets passed in
> as a constant, but that did not change anything.
> 
> 
> It also looks suspicious that the value first gets read from the 
> register and then replaced with the FIELD_PREP() output, while the
> original value gets discarded. Is this intentional?

There are a lot of other fields in that register, so unlikely this was the intent.
A number of them default to non 0 as well.

> 
>         ret = max14001_read(st, reg_addr, &reg_data);
>         if (ret)
>                 return ret;
>         reg_data = FIELD_PREP(mask, val);
>         ret = max14001_write(st, reg_addr, reg_data);
> 
> 
> I have managed to shut up the warning by rearranging the code like:
> 
> diff --git a/drivers/iio/adc/max14001.c b/drivers/iio/adc/max14001.c
> index 18ace09601985..f2ce8f8947982 100644
> --- a/drivers/iio/adc/max14001.c
> +++ b/drivers/iio/adc/max14001.c
> @@ -117,11 +117,9 @@ static int max14001_write_verification_reg(struct max14001_state *st,
>  
>  static int max14001_reg_update(struct max14001_state *st,
>                                 unsigned int reg_addr,
> -                               unsigned int mask,
> -                               unsigned int val)
> +                               unsigned int reg_data)
>  {
>         int ret;
> -       unsigned int reg_data;
>  
>         /* Enable SPI Registers Write */
>         ret = max14001_write(st, MAX14001_WEN, MAX14001_WRITE_WEN);
> @@ -132,8 +130,6 @@ static int max14001_reg_update(struct max14001_state *st,
>         if (ret)
>                 return ret;
>  
> -       reg_data = FIELD_PREP(mask, val);
> -
>         ret = max14001_write(st, reg_addr, reg_data);
>         if (ret)
>                 return ret;
> @@ -299,7 +295,7 @@ static int max14001_probe(struct spi_device *spi)
>  
>                 /* select external voltage reference source for the ADC */
>                 ret = max14001_reg_update(st, MAX14001_CFG,
> -                                         MAX14001_CFG_EXRF, 1);
> +                                         FIELD_PREP(MAX14001_CFG_EXRF, 1));
>  
>                 if (ret < 0)
>                         return ret;

I'd prefer to just drop this reg_update function entirely.  Put the call inline
so that we can use FIELD_PREP() directly rather than (after fixing the probably bug)
passing in both the value and the mask.

> 
> but it looks like there is still a bug in max14001_reg_update(),
> so I'd prefer Kim Seer Paller to revisit this issue and submit
> a properly tested patch.

Absolutely agree.   If it's outstanding in few weeks though we can go
with an educated 'guess' for the fix but I'd really rather not if Kim
can post a fix in the meantime.

Jonathan

> 
>        Arnd

Re: [jic23-iio:testing 6/10] drivers/iio/adc/max14001.c:135:13: warning: result of comparison of constant 18446744073709551615 with expression of type 'typeof (_Generic((mask), char: (unsigned char)0, unsigned char: (unsigned char)0, signed char: (unsigned char)0, unsigned short: (un...

[Jonathan Cameron]

On Thu, 20 Jul 2023 19:18:38 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Thu, 20 Jul 2023 11:29:55 +0200
> "Arnd Bergmann" <arnd@arndb.de> wrote:
> 
> > On Wed, Jun 21, 2023, at 10:25, Andy Shevchenko wrote:  
> > > On Wed, Jun 21, 2023 at 10:19 AM kernel test robot <lkp@intel.com> wrote:    
> > >>
> > >> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git testing
> > >> head:   25e201cc6ff270abc062e13ff912292097cb2827
> > >> commit: d3e93b67f934a477c5851d575a2278f07c6242fb [6/10] iio: adc: max14001: New driver
> > >> config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20230621/202306211545.7b6CdqsL-lkp@intel.com/config)
> > >> compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project.git 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
> > >> reproduce: (https://download.01.org/0day-ci/archive/20230621/202306211545.7b6CdqsL-lkp@intel.com/reproduce)
> > >>
> > >> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > >> the same patch/commit), kindly add following tags
> > >> | Reported-by: kernel test robot <lkp@intel.com>
> > >> | Closes: https://lore.kernel.org/oe-kbuild-all/202306211545.7b6CdqsL-lkp@intel.com/
> > >>
> > >> All warnings (new ones prefixed by >>):    
> > >
> > > Okay, you even haven't compiled your code :-(
> > > This should probably use one of the functions from bitfield.h.
> > > Like u32_encode_bits().    
> > 
> > I see the warning is now in linux-next. I see it only shows up with
> > clang and not gcc, but it would be nice to address it.  
> 
> Sorry, I managed to forget this one was still outstanding and picked up the driver.
> 
> >   
> > >    
> > >> >> drivers/iio/adc/max14001.c:135:13: warning: result of comparison of constant 18446744073709551615 with expression of type 'typeof (_Generic((mask), char: (unsigned char)0, unsigned char: (unsigned char)0, signed char: (unsigned char)0, unsigned short: (unsigned short)0, short: (unsigned short)0, unsigned int: (unsigned int)0, int: (unsigned int)0, unsigned long: (unsigned long)0, long: (unsigned long)0, unsigned long long: (unsigned long long)0, long long: (unsigned long long)0, default: (mask)))' (aka 'unsigned int') is always false [-Wtautological-constant-out-of-range-compare]    
> > >>            reg_data = FIELD_PREP(mask, val);
> > >>                       ^~~~~~~~~~~~~~~~~~~~~
> > >>    include/linux/bitfield.h:114:3: note: expanded from macro 'FIELD_PREP'
> > >>                    __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: ");    \
> > >>                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >>    include/linux/bitfield.h:71:53: note: expanded from macro '__BF_FIELD_CHECK'
> > >>                    BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) >     \
> > >>                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
> > >>    include/linux/build_bug.h:39:58: note: expanded from macro 'BUILD_BUG_ON_MSG'
> > >>    #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
> > >>                                        ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
> > >>    include/linux/compiler_types.h:397:22: note: expanded from macro 'compiletime_assert'
> > >>            _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> > >>            ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >>    include/linux/compiler_types.h:385:23: note: expanded from macro '_compiletime_assert'
> > >>            __compiletime_assert(condition, msg, prefix, suffix)
> > >>            ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >>    include/linux/compiler_types.h:377:9: note: expanded from macro '__compiletime_assert'
> > >>                    if (!(condition))                                       \
> > >>                          ^~~~~~~~~
> > >>    1 warning generated.    
> > 
> > It looks like the problem is the use of FIELD_PREP() with a variable
> > 'val' instead of a constant. I tried marking the function __always_inline
> > so the compiler can see the actual value ('1') that gets passed in
> > as a constant, but that did not change anything.
> > 
> > 
> > It also looks suspicious that the value first gets read from the 
> > register and then replaced with the FIELD_PREP() output, while the
> > original value gets discarded. Is this intentional?  
> 
> There are a lot of other fields in that register, so unlikely this was the intent.
> A number of them default to non 0 as well.
> 
> > 
> >         ret = max14001_read(st, reg_addr, &reg_data);
> >         if (ret)
> >                 return ret;
> >         reg_data = FIELD_PREP(mask, val);
> >         ret = max14001_write(st, reg_addr, reg_data);
> > 
> > 
> > I have managed to shut up the warning by rearranging the code like:
> > 
> > diff --git a/drivers/iio/adc/max14001.c b/drivers/iio/adc/max14001.c
> > index 18ace09601985..f2ce8f8947982 100644
> > --- a/drivers/iio/adc/max14001.c
> > +++ b/drivers/iio/adc/max14001.c
> > @@ -117,11 +117,9 @@ static int max14001_write_verification_reg(struct max14001_state *st,
> >  
> >  static int max14001_reg_update(struct max14001_state *st,
> >                                 unsigned int reg_addr,
> > -                               unsigned int mask,
> > -                               unsigned int val)
> > +                               unsigned int reg_data)
> >  {
> >         int ret;
> > -       unsigned int reg_data;
> >  
> >         /* Enable SPI Registers Write */
> >         ret = max14001_write(st, MAX14001_WEN, MAX14001_WRITE_WEN);
> > @@ -132,8 +130,6 @@ static int max14001_reg_update(struct max14001_state *st,
> >         if (ret)
> >                 return ret;
> >  
> > -       reg_data = FIELD_PREP(mask, val);
> > -
> >         ret = max14001_write(st, reg_addr, reg_data);
> >         if (ret)
> >                 return ret;
> > @@ -299,7 +295,7 @@ static int max14001_probe(struct spi_device *spi)
> >  
> >                 /* select external voltage reference source for the ADC */
> >                 ret = max14001_reg_update(st, MAX14001_CFG,
> > -                                         MAX14001_CFG_EXRF, 1);
> > +                                         FIELD_PREP(MAX14001_CFG_EXRF, 1));
> >  
> >                 if (ret < 0)
> >                         return ret;  
> 
> I'd prefer to just drop this reg_update function entirely.  Put the call inline
> so that we can use FIELD_PREP() directly rather than (after fixing the probably bug)
> passing in both the value and the mask.
> 
> > 
> > but it looks like there is still a bug in max14001_reg_update(),
> > so I'd prefer Kim Seer Paller to revisit this issue and submit
> > a properly tested patch.  
> 
> Absolutely agree.   If it's outstanding in few weeks though we can go
> with an educated 'guess' for the fix but I'd really rather not if Kim
> can post a fix in the meantime.
> 
On closer inspection of my inbox, there are other reported problems that
need resolving so I'll do a rebase and drop the driver until this stuff is resolved.

Jonathan

> Jonathan
> 
> > 
> >        Arnd  
> 

RE: [jic23-iio:testing 6/10] drivers/iio/adc/max14001.c:135:13: warning: result of comparison of constant 18446744073709551615 with expression of type 'typeof (_Generic((mask), char: (unsigned char)0, unsigned char: (unsigned char)0, signed char: (unsi...

[Paller, Kim Seer]

> 
> I'd prefer to just drop this reg_update function entirely.  Put the call inline so
> that we can use FIELD_PREP() directly rather than (after fixing the probably
> bug) passing in both the value and the mask.
> 
> >
> > but it looks like there is still a bug in max14001_reg_update(), so
> > I'd prefer Kim Seer Paller to revisit this issue and submit a properly
> > tested patch.
> 
> Absolutely agree.   If it's outstanding in few weeks though we can go
> with an educated 'guess' for the fix but I'd really rather not if Kim can post a fix
> in the meantime.

Thank you all for your input and feedback. I'm currently working on the fix and 
will test it as soon as I have access to the eval boards again. I'll reach out if 
something needs to be clarified.

Thanks,
Kim