Re: [PATCH v6 5/8] iio: dac: ad3552r: changes to use FIELD_PREP

[Nuno Sá]

On Mon, 2024-10-14 at 16:14 -0500, David Lechner wrote:
> On 10/14/24 5:08 AM, Angelo Dureghello wrote:
> > From: Angelo Dureghello <adureghello@xxxxxxxxxxxx>
> > 
> > Changes to use FIELD_PREP, so that driver-specific ad3552r_field_prep
> > is removed. Variables (arrays) that was used to call ad3552r_field_prep
> > are removed too.
> > 
> > Signed-off-by: Angelo Dureghello <adureghello@xxxxxxxxxxxx>
> > ---
> 
> Found one likely bug. The rest are suggestions to keep the static
> analyzers happy.
> 
> 				\
> > @@ -510,8 +416,14 @@ static int ad3552r_write_raw(struct iio_dev *indio_dev,
> >  					val);
> >  		break;
> >  	case IIO_CHAN_INFO_ENABLE:
> > -		err = ad3552r_set_ch_value(dac, AD3552R_CH_DAC_POWERDOWN,
> > -					   chan->channel, !val);
> > +		if (chan->channel == 0)
> > +			val = FIELD_PREP(AD3552R_MASK_CH_DAC_POWERDOWN(0),
> > !val);
> > +		else
> > +			val = FIELD_PREP(AD3552R_MASK_CH_DAC_POWERDOWN(1),
> > !val);
> 
> In the past, I've had bots (Sparse, IIRC) complain about using !val
> with FIELD_PREP. Alternative is to write it as val ? 1 : 0.
> 

Hmm, I'm fairly sure I also suffered from that warning. AFAICT, there's nothing wrong
with the code so I would not make it less readable just to keep the tool happy (it
seems to me that the tool is the one that needs to make this right). But this is just
me - yeah, not a fan of the ternary operator :)

Anyways, no strong feelings so if you go with the above, I won't really complain...
just my 2 cents.

- Nuno Sá 
>