On Thu, 20 Jul 2023 19:18:38 +0100 Jonathan Cameron <jic23@xxxxxxxxxx> wrote: > On Thu, 20 Jul 2023 11:29:55 +0200 > "Arnd Bergmann" <arnd@xxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx/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@xxxxxxxxx/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@xxxxxxxxx> > > >> | Closes: https://lore.kernel.org/oe-kbuild-all/202306211545.7b6CdqsL-lkp@xxxxxxxxx/ > > >> > > >> 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, ®_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 >
