RE: [PATCH] iio: freq: admv1014: Fix warning about dubious x & !y and improve readability

["Miclaus, Antoniu" <Antoniu.Miclaus@xxxxxxxxxx>]

> -----Original Message-----
> From: Jonathan Cameron <jic23@xxxxxxxxxx>
> Sent: Sunday, April 10, 2022 8:16 PM
> To: linux-iio@xxxxxxxxxxxxxxx
> Cc: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>; Miclaus, Antoniu
> <Antoniu.Miclaus@xxxxxxxxxx>
> 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@xxxxxxxxxx> wrote:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > Cc: Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx>
> 
> 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);