* [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; +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
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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.