Re: [PATCH v14 10/12] iio: frequency: adf41513: features on frequency change

[Rodrigo Alencar]

On 26/05/24 11:15AM, sashiko-bot@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Bleed current is not computed or updated when switching from Integer-N to Fractional-N mode if the PFD frequency remains unchanged.
> --
> 
> commit 299bd27511dd5b2a2354c749631d53bcff158e2b
> Author: Rodrigo Alencar <rodrigo.alencar@xxxxxxxxxx>
> 
> iio: frequency: adf41513: features on frequency change
> 
> This commit adds bleed current configuration, lock detector window adjustments,
> and phase resync support to the ADF41513 driver when changing the PFD
> frequency.
> 
> > diff --git a/drivers/iio/frequency/adf41513.c b/drivers/iio/frequency/adf41513.c
> > --- a/drivers/iio/frequency/adf41513.c
> > +++ b/drivers/iio/frequency/adf41513.c
> 
> [ ... ]
> 
> > +static void adf41513_set_bleed_val(struct adf41513_state *st)
> > +{
> > +	u32 bleed_value, cp_index;
> > +
> > +	if (!(st->regs[ADF41513_REG6] & ADF41513_REG6_BLEED_ENABLE_MSK))
> > +		return;
> 
> [ ... ]
> 
> > @@ -630,6 +725,14 @@ static int adf41513_set_frequency(struct adf41513_state *st, u64 freq_uhz, u16 s
> >  		st->regs[ADF41513_REG6] |= ADF41513_REG6_BLEED_ENABLE_MSK;
> >  	}
> >  
> > +	if (pfd_change) {
> > +		adf41513_set_bleed_val(st);
> 
> [Severity: High]
> Will the bleed current be properly configured if the device switches
> from Integer-N to Fractional-N mode without changing the PFD frequency?
> 
> In adf41513_set_bleed_val(), if the device is previously in Integer-N mode,
> ADF41513_REG6_BLEED_ENABLE_MSK is cleared. If the PFD frequency changes
> while in Integer-N mode, adf41513_set_bleed_val() returns early without
> updating the register.
> 
> If a subsequent frequency change switches the PLL to Fractional-N mode but
> maintains the exact same PFD frequency, pfd_change will be false. Since
> adf41513_set_bleed_val() is only called when pfd_change is true, the bleed
> value configuration gets skipped entirely.
> 
> Could this leave the bleed circuit enabled with a stale or uninitialized
> current value, potentially preventing the PLL from locking?

This make sense. After adding the suggested guard for bleed enabled I should
have move adf41513_set_bleed_val() to be called bellow...

> > +		adf41513_set_phase_resync(st);
> > +	}
> > +
> > +	if (pfd_change || mode_change)
here..
> > +		adf41513_set_ld_window(st);
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260524-adf41513-iio-driver-v14-0-06824d9c15f4@xxxxxxxxxx?part=10

At least, that would be a quick one. We can have another round of reviews on
the decimal parse function... worst case, we can have another week.

-- 
Kind regards,

Rodrigo Alencar