Re: [PATCH v13 08/12] iio: frequency: adf41513: driver implementation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Variable modulus fractional calculation (frac2) can mathematically exceed the mod2 limit due to truncation error magnification.
- [High] Cached software state is modified before SPI writes and is not reverted if the write fails, causing state corruption.
- [Medium] Repeated `dev_warn` inside the frequency calculation loop causes kernel log spam.
--

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

This commit adds support for the Analog Devices ADF41513 26.5 GHz
Integer-N/Fractional-N PLL Frequency Synthesizer.

> diff --git a/drivers/iio/frequency/adf41513.c b/drivers/iio/frequency/adf41513.c
> new file mode 100644
> index 0000000000000..20ea7e82818f2
> --- /dev/null
> +++ b/drivers/iio/frequency/adf41513.c

[ ... ]

> +static int adf41513_calc_pfd_frequency(struct adf41513_state *st,
> +				       struct adf41513_pll_settings *result,
> +				       u64 fpfd_limit_uhz)
> +{
> +	result->ref_div2 = st->data.ref_div2_en;
> +	result->ref_doubler = st->data.ref_doubler_en;
> +
> +	if (st->data.ref_doubler_en &&
> +	    st->ref_freq_hz > ADF41513_MAX_REF_FREQ_DOUBLER_HZ) {
> +		result->ref_doubler = 0;
> +		dev_warn(&st->spi->dev, "Disabling ref doubler due to high reference frequency\n");

Will this cause kernel log spam if an application frequently changes the
frequency? Since ref_doubler_en and ref_freq_hz are constant after probe,
could we validate this once during initialization or use dev_warn_once()
here?

> +	}

[ ... ]

> +static int adf41513_calc_variable_mod(struct adf41513_state *st,
> +				      struct adf41513_pll_settings *result)
> +{

[ ... ]

> +	/* calculate frac1 and frac2 */
> +	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);
> +	frac2 = mul_u64_u64_div_u64(freq_error_uhz, mod2 * ADF41513_FIXED_MODULUS,
> +				    result->pfd_frequency_uhz);

Can this calculation result in a frac2 value that exceeds mod2? 

Because mul_u64_u32_div() truncates the remainder during the integer
division, the subtracted value might be smaller than the exact mathematical
contribution of frac1. This leaves freq_error_uhz artificially larger than
its theoretical bound. 

When frac2 is computed by multiplying this enlarged error by
(mod2 * ADF41513_FIXED_MODULUS), couldn't the result exceed mod2? If
the hardware requires frac2 < mod2, should we cap frac2 or handle the
remainder differently?

> +
> +	/* integer part */
> +	result->actual_frequency_uhz = (u64)int_val * result->pfd_frequency_uhz;

[ ... ]

> +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 adf41513_sync_config() fails at the end of this function, does this leave
st->settings corrupted? If so, subsequent reads might incorrectly report the
failed frequency as active.

> +
> +	dev_dbg(&st->spi->dev,

[ ... ]

> +static int adf41513_suspend(struct adf41513_state *st)
> +{
> +	st->regs[ADF41513_REG6] |= FIELD_PREP(ADF41513_REG6_POWER_DOWN_MSK, 1);
> +	return adf41513_sync_config(st, ADF41513_SYNC_DIFF);

Similarly, if adf41513_sync_config() fails here, the power-down bit remains
set in st->regs[ADF41513_REG6]. Could this inadvertently power down the PLL
during a later register sync?

> +}

[ ... ]

> +static int adf41513_write_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      int val, int val2, long info)
> +{
> +	struct adf41513_state *st = iio_priv(indio_dev);
> +	u64 tmp64 = iio_val_s64_compose(val, val2);
> +	u16 phase_val;
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_FREQUENCY:
> +		return adf41513_set_frequency(st, tmp64, ADF41513_SYNC_DIFF);
> +	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);
> +		ret = adf41513_sync_config(st, ADF41513_SYNC_REG0);
> +		if (ret)
> +			return ret;
> +
> +		/* clear phase adjust for the next sync */
> +		st->regs[ADF41513_REG2] &= ~ADF41513_REG2_PHASE_ADJ_MSK;

If adf41513_sync_config() fails above, the early return leaves the
ADF41513_REG2_PHASE_ADJ_MSK bit set in st->regs. Could this cause unintended
phase adjustments during subsequent frequency changes or register syncs?

> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260517-adf41513-iio-driver-v13-0-bb6e134a360f@analog.com?part=8