Re: [PATCH v12 07/11] iio: frequency: adf41513: driver implementation

[Rodrigo Alencar]

On 26/05/11 10:43PM, sashiko-bot@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> - [High] Hardware state is lost if system suspend occurs while device is powered down.
> - [High] `adf41513_calc_fixed_mod` uses uninitialized `regs_hw` cache during probe, causing an incorrect initial output frequency.
> - [High] Arithmetic underflow in `adf41513_calc_fixed_mod` causes valid frequency configurations to fail with `-ERANGE`.
> - [Medium] The phase adjustment bit (`ADF41513_REG2_PHASE_ADJ_MSK`) is set but never cleared, causing unintended phase shifts on subsequent frequency updates.
> - [Medium] Software state is permanently modified before SPI write succeeds, causing a state mismatch on SPI failure.
> --

So those seem to be new issues.

> > +static int adf41513_calc_fixed_mod(struct adf41513_state *st,
> > +				   struct adf41513_pll_settings *result)
> > +{
> > +	u64 resolution_uhz = div_u64(result->pfd_frequency_uhz, ADF41513_FIXED_MODULUS);
> > +	u64 target_frequency_uhz = result->target_frequency_uhz;
> > +	u64 freq_error_uhz;
> > +	u32 int_val, frac1;
> > +	bool lsb_p1_offset = !FIELD_GET(ADF41513_REG5_LSB_P1_MSK, st->regs_hw[ADF41513_REG5]);
> 
> During setup, st->regs_hw is initialized to 0xFFFFFFFF to force a full write
> on the first sync.
> 
> Since regs_hw is all 1s, lsb_p1_offset will evaluate to false here, but the
> true intended configuration in st->regs has this bit as 0.
> 
> Will this cause the algorithm to skip subtracting the offset while the hardware
> actually adds it? Should this check read from st->regs[ADF41513_REG5] instead?

That makes sense and we can use st->regs instead of st->regs_hw. This is unlikely to
happen because during setup() we are often going for integer-n mode. Also the
resolution error of f_pfd/2^26 is very minimal for this initial configuration,
userspace can adjust the value later at runtime if relying on fixed-modulus mode.
However this is something I would want to fix...

> [ ... ]
> 
> > +	/* compute frac1 and fixed modulus error */
> > +	frac1 = mul_u64_u64_div_u64(freq_error_uhz, ADF41513_FIXED_MODULUS,
> > +				    result->pfd_frequency_uhz);
> > +	freq_error_uhz -= mul_u64_u32_div(result->pfd_frequency_uhz, frac1,
> > +					  ADF41513_FIXED_MODULUS);
> > +
> > +	/* check if freq error is within a tolerance of 1/2 resolution */
> > +	if (freq_error_uhz > (resolution_uhz >> 1) && frac1 < (ADF41513_FIXED_MODULUS - 1)) {
> > +		frac1++;
> > +		freq_error_uhz = resolution_uhz - freq_error_uhz;
> 
> The resolution_uhz calculation uses div_u64(), which truncates the true
> frequency step.
> 
> Because of this truncation, is it possible for freq_error_uhz to be strictly
> greater than resolution_uhz here?
> 
> If so, does the unsigned subtraction resolution_uhz - freq_error_uhz underflow
> and produce a large value that falsely triggers the -ERANGE error below?

This is also something we can fix. Nice that this was caught... it is a hard one.

> [ ... ]
> 
> > +static int adf41513_set_frequency(struct adf41513_state *st, u64 freq_uhz, u16 sync_mask)
> > +{
> > +	struct adf41513_pll_settings result;
> > +	int ret;
> > +
> > +	ret = adf41513_calc_pll_settings(st, &result, freq_uhz);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* apply computed results to pll settings */
> > +	st->settings = result;
> 
> [ ... ]
> 
> > +	if (st->settings.mode == ADF41513_MODE_INTEGER_N) {
> > +		st->regs[ADF41513_REG6] |= ADF41513_REG6_INT_MODE_MSK;
> > +		st->regs[ADF41513_REG6] &= ~ADF41513_REG6_BLEED_ENABLE_MSK;
> > +	} else {
> > +		st->regs[ADF41513_REG6] &= ~ADF41513_REG6_INT_MODE_MSK;
> > +		st->regs[ADF41513_REG6] |= ADF41513_REG6_BLEED_ENABLE_MSK;
> > +	}
> > +
> > +	return adf41513_sync_config(st, sync_mask | ADF41513_SYNC_REG0);
> > +}
> 
> If the adf41513_sync_config() SPI write fails, st->settings and st->regs
> have already been updated with the new configuration.
> 
> Will a subsequent read of the frequency channel via adf41513_pll_get_rate()
> return this failed frequency instead of the actual physical hardware frequency?

Not seeing much problem with that. Reversing the state on every spi write failure
would be too much effort for an error that is already reported to the user.
Most likely another spi write attempt might fail as well, so would it be a platform
problem?

> [ ... ]
> 
> > +static int adf41513_write_raw(struct iio_dev *indio_dev,
> > +			      struct iio_chan_spec const *chan,
> > +			      int val, int val2, long info)
> > +{
> 
> [ ... ]
> 
> > +	case IIO_CHAN_INFO_PHASE:
> > +		if (tmp64 >= ADF41513_MAX_PHASE_MICRORAD)
> > +			return -EINVAL;
> > +
> > +		phase_val = DIV_U64_ROUND_CLOSEST(tmp64 << 12,
> > +						  ADF41513_MAX_PHASE_MICRORAD);
> > +		phase_val = min(phase_val, ADF41513_MAX_PHASE_VAL);
> > +		st->regs[ADF41513_REG2] |= ADF41513_REG2_PHASE_ADJ_MSK;
> > +		FIELD_MODIFY(ADF41513_REG2_PHASE_VAL_MSK,
> > +			     &st->regs[ADF41513_REG2], phase_val);
> > +		return adf41513_sync_config(st, ADF41513_SYNC_REG0);
> 
> The ADF41513_REG2_PHASE_ADJ_MSK bit is set in the software shadow register,
> but does not appear to be cleared after the shift completes.
> 
> Because the hardware evaluates the PHASE_ADJ bit in REG2 on every REG0 write,
> could this cause unintended phase shifts during future frequency updates?

We don't need to clear that, if phase adjust is set then the phase shift is going
to be applied. There is no phase accumulation or side effects. I think the
datasheet could improve on that, but later on it says:

	The phase word in Register 2 controls the RF output phase. As this
	word is changed from 0 to 2^12, the RF output phase changes over a
	360° range in steps of phase value × 360°/2^12.

> [ ... ]
> 
> > +static int adf41513_pm_resume(struct device *dev)
> > +{
> > +	struct adf41513_state *st = dev_get_drvdata(dev);
> > +
> > +	guard(mutex)(&st->lock);
> > +	if (st->powerdown)
> > +		return 0; /* nothing to do */
> > +
> > +	return adf41513_resume(st);
> > +}
> 
> If the system cuts power to the regulators during sleep, the chip wakes up
> in its default Power-On Reset (POR) state.
> 
> If st->powerdown is true, this function returns early and st->regs_hw is
> not invalidated.
> 
> Could this leave the software state out of sync with the physical hardware,
> causing future partial writes to leave the chip unconfigured?

That is not an issue because partial writes would not power the device.
Also the resume() function makes sure to write all registers so a power-up
sequence is completed.

> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260510-adf41513-iio-driver-v12-0-34af2ed2779f@xxxxxxxxxx?part=7

-- 
Kind regards,

Rodrigo Alencar