Re: [PATCH] iio: frequency: adf4377: remove duplicate/repeating constant

[Christophe JAILLET]

Le 11/02/2023 à 17:20, Deepak R Varma a écrit :
> On Sat, Feb 11, 2023 at 03:57:51PM +0100, Christophe JAILLET wrote:
> > Le 11/02/2023 à 08:07, Deepak R Varma a écrit :
> > > Constant ADF4377_0000_SOFT_RESET_R_MSK is unnecessarily or'ed with
> > > itself. Remove the redundant constant from the expression.
> > > Issue identified using doublebitand.cocci Coccinelle semantic patch.
> > >
> > > Signed-off-by: Deepak R Varma <drv-asAA5fHt7EIAvxtiuMwx3w@xxxxxxxxxxxxxxxx>
> > > ---
> > >    drivers/iio/frequency/adf4377.c | 4 ++--
> > >    1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/iio/frequency/adf4377.c b/drivers/iio/frequency/adf4377.c
> > > index 26abecbd51e0..caefd7ea6b14 100644
> > > --- a/drivers/iio/frequency/adf4377.c
> > > +++ b/drivers/iio/frequency/adf4377.c
> > > @@ -495,8 +495,8 @@ static int adf4377_soft_reset(struct adf4377_state *st)
> > >    		return ret;
> > >    	return regmap_read_poll_timeout(st->regmap, 0x0, read_val,
> > > -					!(read_val & (ADF4377_0000_SOFT_RESET_R_MSK |
> > > -					ADF4377_0000_SOFT_RESET_R_MSK)), 200, 200 * 100);
> > > +					!(read_val & ADF4377_0000_SOFT_RESET_R_MSK),
> > > +					200, 200 * 100);
> >
> > Based on the code just above, it is likely that one is expected to be
> > ADF4377_0000_SOFT_RESET_MSK.
>
> Hello CJ,
> I agree, that appears to be a close possibility. I also tried looking to the
> data sheet, but could not conclude.
> How can I make sure if the other one should be ADF4377_0000_SOFT_RESET_MSK?
>
> Thank you,
> ./drv

I don't have the hardware, but based on the datasheet, table 43, 
ADF4377_0000_SOFT_RESET_R_MSK is a repeat of ADF4377_0000_SOFT_RESET_MSK.

My understanding is that the regmap_update_bits() a few lines above sets 
bits to request the chip to reset. Both bits have the same meaning and I 
guess that writing only one would be enough or the chip require both to 
be written for some kind of "security" feature maybe.

Anyway, on reset, both are zeroed.
That is what is tested by regmap_read_poll_timeout().

So the code should work fine as-is, but to keep the logic, I would 
definitively use ADF4377_0000_SOFT_RESET_MSK | 
ADF4377_0000_SOFT_RESET_R_MSK.


Finally, the driver is new and the author is in copy, so I guess that he 
would be in the best position to confirm all that.

CJ

> > CJ
> >
> > >    }
> > >    static int adf4377_get_freq(struct adf4377_state *st, u64 *freq)