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

[sashiko-bot@kernel.org]

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.
--

commit 5877506cf62b15b22549417e5996d3ed6f4bfd57
Author: Rodrigo Alencar <rodrigo.alencar@analog.com>
Subject: iio: frequency: adf41513: driver implementation

This commit introduces a driver for the Analog Devices ADF41513 PLL
frequency synthesizer. The driver supports integer-N and fractional-N
synthesis modes, high-resolution microhertz frequency calculations, and
includes power management suspend/resume callbacks.

> diff --git a/drivers/iio/frequency/adf41513.c b/drivers/iio/frequency/adf41513.c
> --- /dev/null
> +++ b/drivers/iio/frequency/adf41513.c

[ ... ]

> +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?

[ ... ]

> +	/* 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?

[ ... ]

> +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?

[ ... ]

> +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?

[ ... ]

> +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?

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