| Message ID | 20220319181401.136810-1-jic23@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | iio: freq: admv1014: Fix warning about dubious x & !y and improve readability | expand |
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);
> -----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);
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); >
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);