* [PATCH] iio: freq: admv1014: Fix warning about dubious x & !y and improve readability @ 2022-03-19 18:14 Jonathan Cameron 2022-04-10 17:16 ` Jonathan Cameron 0 siblings, 1 reply; 4+ messages in thread From: Jonathan Cameron @ 2022-03-19 18:14 UTC (permalink / raw) To: linux-iio; +Cc: Jonathan Cameron, Antoniu Miclaus From: Jonathan Cameron <Jonathan.Cameron@huawei.com> The warning comes from __BF_FIELD_CHECK() specifically BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ? \ ~((_mask) >> __bf_shf(_mask)) & (_val) : 0, \ _pfx "value too large for the field"); \ The code was using !(enum value) which is not particularly easy to follow so replace that with explicit matching and use of ? 0 : 1; or ? 1 : 0; to improve readability. Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Cc: Antoniu Miclaus <antoniu.miclaus@analog.com> --- drivers/iio/frequency/admv1014.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/iio/frequency/admv1014.c b/drivers/iio/frequency/admv1014.c index a7994f8e6b9b..802835efbec7 100644 --- a/drivers/iio/frequency/admv1014.c +++ b/drivers/iio/frequency/admv1014.c @@ -700,8 +700,10 @@ static int admv1014_init(struct admv1014_state *st) ADMV1014_DET_EN_MSK; enable_reg = FIELD_PREP(ADMV1014_P1DB_COMPENSATION_MSK, st->p1db_comp ? 3 : 0) | - FIELD_PREP(ADMV1014_IF_AMP_PD_MSK, !(st->input_mode)) | - FIELD_PREP(ADMV1014_BB_AMP_PD_MSK, st->input_mode) | + FIELD_PREP(ADMV1014_IF_AMP_PD_MSK, + (st->input_mode == ADMV1014_IQ_MODE) ? 0 : 1) | + FIELD_PREP(ADMV1014_BB_AMP_PD_MSK, + (st->input_mode == ADMV1014_IQ_MODE) ? 1 : 0) | FIELD_PREP(ADMV1014_DET_EN_MSK, st->det_en); return __admv1014_spi_update_bits(st, ADMV1014_REG_ENABLE, enable_reg_msk, enable_reg); -- 2.35.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] iio: freq: admv1014: Fix warning about dubious x & !y and improve readability 2022-03-19 18:14 [PATCH] iio: freq: admv1014: Fix warning about dubious x & !y and improve readability Jonathan Cameron @ 2022-04-10 17:16 ` Jonathan Cameron 2022-04-11 7:18 ` Miclaus, Antoniu 0 siblings, 1 reply; 4+ messages in thread From: Jonathan Cameron @ 2022-04-10 17:16 UTC (permalink / raw) To: linux-iio; +Cc: Jonathan Cameron, Antoniu Miclaus On Sat, 19 Mar 2022 18:14:01 +0000 Jonathan Cameron <jic23@kernel.org> wrote: > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > The warning comes from __BF_FIELD_CHECK() > specifically > > BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ? \ > ~((_mask) >> __bf_shf(_mask)) & (_val) : 0, \ > _pfx "value too large for the field"); \ > > The code was using !(enum value) which is not particularly easy to follow > so replace that with explicit matching and use of ? 0 : 1; or ? 1 : 0; > to improve readability. > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Cc: Antoniu Miclaus <antoniu.miclaus@analog.com> Antoniu, or anyone else who has time, can you sanity check this one? I'd like to clean up the warning asap but don't really trust myself enough to not have done something stupid ;) Jonathan > --- > drivers/iio/frequency/admv1014.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/frequency/admv1014.c b/drivers/iio/frequency/admv1014.c > index a7994f8e6b9b..802835efbec7 100644 > --- a/drivers/iio/frequency/admv1014.c > +++ b/drivers/iio/frequency/admv1014.c > @@ -700,8 +700,10 @@ static int admv1014_init(struct admv1014_state *st) > ADMV1014_DET_EN_MSK; > > enable_reg = FIELD_PREP(ADMV1014_P1DB_COMPENSATION_MSK, st->p1db_comp ? 3 : 0) | > - FIELD_PREP(ADMV1014_IF_AMP_PD_MSK, !(st->input_mode)) | > - FIELD_PREP(ADMV1014_BB_AMP_PD_MSK, st->input_mode) | > + FIELD_PREP(ADMV1014_IF_AMP_PD_MSK, > + (st->input_mode == ADMV1014_IQ_MODE) ? 0 : 1) | > + FIELD_PREP(ADMV1014_BB_AMP_PD_MSK, > + (st->input_mode == ADMV1014_IQ_MODE) ? 1 : 0) | > FIELD_PREP(ADMV1014_DET_EN_MSK, st->det_en); > > return __admv1014_spi_update_bits(st, ADMV1014_REG_ENABLE, enable_reg_msk, enable_reg); ^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH] iio: freq: admv1014: Fix warning about dubious x & !y and improve readability 2022-04-10 17:16 ` Jonathan Cameron @ 2022-04-11 7:18 ` Miclaus, Antoniu 2022-04-11 8:44 ` Jonathan Cameron 0 siblings, 1 reply; 4+ messages in thread From: Miclaus, Antoniu @ 2022-04-11 7:18 UTC (permalink / raw) To: Jonathan Cameron, linux-iio@vger.kernel.org; +Cc: Jonathan Cameron > -----Original Message----- > From: Jonathan Cameron <jic23@kernel.org> > Sent: Sunday, April 10, 2022 8:16 PM > To: linux-iio@vger.kernel.org > Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>; Miclaus, Antoniu > <Antoniu.Miclaus@analog.com> > Subject: Re: [PATCH] iio: freq: admv1014: Fix warning about dubious x & !y > and improve readability > > [External] > > On Sat, 19 Mar 2022 18:14:01 +0000 > Jonathan Cameron <jic23@kernel.org> wrote: > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > The warning comes from __BF_FIELD_CHECK() > > specifically > > > > BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ? \ > > ~((_mask) >> __bf_shf(_mask)) & (_val) : 0, \ > > _pfx "value too large for the field"); \ > > > > The code was using !(enum value) which is not particularly easy to follow > > so replace that with explicit matching and use of ? 0 : 1; or ? 1 : 0; > > to improve readability. > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Cc: Antoniu Miclaus <antoniu.miclaus@analog.com> > > Antoniu, or anyone else who has time, can you sanity check this one? > I'd like to clean up the warning asap but don't really trust myself > enough to not have done something stupid ;) > > Jonathan > > > --- > > drivers/iio/frequency/admv1014.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/iio/frequency/admv1014.c > b/drivers/iio/frequency/admv1014.c > > index a7994f8e6b9b..802835efbec7 100644 > > --- a/drivers/iio/frequency/admv1014.c > > +++ b/drivers/iio/frequency/admv1014.c > > @@ -700,8 +700,10 @@ static int admv1014_init(struct admv1014_state > *st) > > ADMV1014_DET_EN_MSK; > > > > enable_reg = FIELD_PREP(ADMV1014_P1DB_COMPENSATION_MSK, > st->p1db_comp ? 3 : 0) | > > - FIELD_PREP(ADMV1014_IF_AMP_PD_MSK, !(st- > >input_mode)) | > > - FIELD_PREP(ADMV1014_BB_AMP_PD_MSK, st- > >input_mode) | > > + FIELD_PREP(ADMV1014_IF_AMP_PD_MSK, > > + (st->input_mode == ADMV1014_IQ_MODE) > ? 0 : 1) | > > + FIELD_PREP(ADMV1014_BB_AMP_PD_MSK, > > + (st->input_mode == ADMV1014_IQ_MODE) > ? 1 : 0) | Hello Jonathan, I think it should be vice-versa: FIELD_PREP(ADMV1014_IF_AMP_PD_MSK, (st->input_mode == ADMV1014_IQ_MODE) ? 1 : 0) | FIELD_PREP(ADMV1014_BB_AMP_PD_MSK, (st->input_mode == ADMV1014_IQ_MODE) ? 0 : 1) | "To set the ADMV1014 in I/Q mode, set BB_AMP_PD (Register 0x03, Bit 8) to 0 and set IF_AMP_PD (Register 0x03, Bit 11) to 1." "To configure the ADMV1014 in IF mode, set BB_AMP_PD (Register 0x03, Bit 8) to 1 and set IF_AMP_PD (Register 0x03, Bit 11) to 0" Regards, > > FIELD_PREP(ADMV1014_DET_EN_MSK, st->det_en); > > > > return __admv1014_spi_update_bits(st, ADMV1014_REG_ENABLE, > enable_reg_msk, enable_reg); ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] iio: freq: admv1014: Fix warning about dubious x & !y and improve readability 2022-04-11 7:18 ` Miclaus, Antoniu @ 2022-04-11 8:44 ` Jonathan Cameron 0 siblings, 0 replies; 4+ messages in thread From: Jonathan Cameron @ 2022-04-11 8:44 UTC (permalink / raw) To: Miclaus, Antoniu; +Cc: Jonathan Cameron, linux-iio@vger.kernel.org On Mon, 11 Apr 2022 07:18:10 +0000 "Miclaus, Antoniu" <Antoniu.Miclaus@analog.com> wrote: > > -----Original Message----- > > From: Jonathan Cameron <jic23@kernel.org> > > Sent: Sunday, April 10, 2022 8:16 PM > > To: linux-iio@vger.kernel.org > > Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>; Miclaus, Antoniu > > <Antoniu.Miclaus@analog.com> > > Subject: Re: [PATCH] iio: freq: admv1014: Fix warning about dubious x & !y > > and improve readability > > > > [External] > > > > On Sat, 19 Mar 2022 18:14:01 +0000 > > Jonathan Cameron <jic23@kernel.org> wrote: > > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > > > The warning comes from __BF_FIELD_CHECK() > > > specifically > > > > > > BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ? \ > > > ~((_mask) >> __bf_shf(_mask)) & (_val) : 0, \ > > > _pfx "value too large for the field"); \ > > > > > > The code was using !(enum value) which is not particularly easy to follow > > > so replace that with explicit matching and use of ? 0 : 1; or ? 1 : 0; > > > to improve readability. > > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > Cc: Antoniu Miclaus <antoniu.miclaus@analog.com> > > > > Antoniu, or anyone else who has time, can you sanity check this one? > > I'd like to clean up the warning asap but don't really trust myself > > enough to not have done something stupid ;) > > > > Jonathan > > > > > --- > > > drivers/iio/frequency/admv1014.c | 6 ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/iio/frequency/admv1014.c > > b/drivers/iio/frequency/admv1014.c > > > index a7994f8e6b9b..802835efbec7 100644 > > > --- a/drivers/iio/frequency/admv1014.c > > > +++ b/drivers/iio/frequency/admv1014.c > > > @@ -700,8 +700,10 @@ static int admv1014_init(struct admv1014_state > > *st) > > > ADMV1014_DET_EN_MSK; > > > > > > enable_reg = FIELD_PREP(ADMV1014_P1DB_COMPENSATION_MSK, > > st->p1db_comp ? 3 : 0) | > > > - FIELD_PREP(ADMV1014_IF_AMP_PD_MSK, !(st- > > >input_mode)) | > > > - FIELD_PREP(ADMV1014_BB_AMP_PD_MSK, st- > > >input_mode) | > > > + FIELD_PREP(ADMV1014_IF_AMP_PD_MSK, > > > + (st->input_mode == ADMV1014_IQ_MODE) > > ? 0 : 1) | > > > + FIELD_PREP(ADMV1014_BB_AMP_PD_MSK, > > > + (st->input_mode == ADMV1014_IQ_MODE) > > ? 1 : 0) | > Hello Jonathan, > > I think it should be vice-versa: > FIELD_PREP(ADMV1014_IF_AMP_PD_MSK, > (st->input_mode == ADMV1014_IQ_MODE) ? 1 : 0) | > FIELD_PREP(ADMV1014_BB_AMP_PD_MSK, > (st->input_mode == ADMV1014_IQ_MODE) ? 0 : 1) | > > "To set the ADMV1014 in I/Q mode, set BB_AMP_PD > (Register 0x03, Bit 8) to 0 and set IF_AMP_PD (Register 0x03, > Bit 11) to 1." > > "To configure the ADMV1014 in IF mode, set BB_AMP_PD > (Register 0x03, Bit 8) to 1 and set IF_AMP_PD (Register 0x03, > Bit 11) to 0" And that's why I need sanity checks on 'obvious' patches. Thanks! You are correct that I've inverted it by matching on the 0 enum value, whereas the original code was effectively starting with the 1 enum value. Keeping closer to the original code it would be: FIELD_PREP(ADMV1014_IF_AMP_PD_MSK, (st->input_mode == ADMV1014_IF_MODE) ? 0 : 1) | FIELD_PREP(ADMV1014_BB_AMP_PD_MSK, (st->input_mode == ADMV1014_IF_MODE) ? 1 : 0) | I'm tempted to go with that version rather than your equivalent one because it keeps the sense of the statements the same so in theory should be a more obvious patch. Will send a v2. Thanks, Jonathan > > Regards, > > > FIELD_PREP(ADMV1014_DET_EN_MSK, st->det_en); > > > > > > return __admv1014_spi_update_bits(st, ADMV1014_REG_ENABLE, > > enable_reg_msk, enable_reg); > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-04-11 8:45 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-03-19 18:14 [PATCH] iio: freq: admv1014: Fix warning about dubious x & !y and improve readability Jonathan Cameron 2022-04-10 17:16 ` Jonathan Cameron 2022-04-11 7:18 ` Miclaus, Antoniu 2022-04-11 8:44 ` Jonathan Cameron
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox