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

Jonathan Cameron
2026-04-25T16:34:11Z

On Wed, 15 Apr 2026 10:51:50 +0100
Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@kernel.org> wrote:

> From: Rodrigo Alencar <rodrigo.alencar@analog.com>
>
> The driver is based on existing PLL drivers in the IIO subsystem and
> implements the following key features:
>
> - Integer-N and fractional-N (fixed/variable modulus) synthesis modes
> - High-resolution frequency calculations using microhertz (µHz) precision
> to handle sub-Hz resolution across multi-GHz frequency ranges
> - IIO debugfs interface for direct register access
> - FW property parsing from devicetree including charge pump settings,
> reference path configuration and muxout options
> - Power management support with suspend/resume callbacks
> - Lock detect GPIO monitoring
>
> Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>
Hi Rodrigo

I was thinking the only thing that was still under possible discussion
was the parsing code and related tests, but given that's been quiet
I took what I was thinking would be the final look at this.

Found a few bits of code that are unnecessarily general.
Other stuff is all trivial.

Anyhow, sashiko is now running on linux-iio so please also take a look
at what it thinks of your v11. It's been finding some subtle bugs but
obviously take into account it might be making things up ;)

Thanks,

Jonathan

> diff --git a/drivers/iio/frequency/Makefile b/drivers/iio/frequency/Makefile
> index 70d0e0b70e80..53b4d01414d8 100644
> --- a/drivers/iio/frequency/Makefile
> +++ b/drivers/iio/frequency/Makefile
> @@ -5,6 +5,7 @@
>
> # When adding new entries keep the list in alphabetical order
> obj-$(CONFIG_AD9523) += ad9523.o
> +obj-$(CONFIG_ADF41513) += adf41513.o
> obj-$(CONFIG_ADF4350) += adf4350.o
> obj-$(CONFIG_ADF4371) += adf4371.o
> obj-$(CONFIG_ADF4377) += adf4377.o
> diff --git a/drivers/iio/frequency/adf41513.c b/drivers/iio/frequency/adf41513.c
> new file mode 100644
> index 000000000000..bf2d6c941082
> --- /dev/null
> +++ b/drivers/iio/frequency/adf41513.c

> +
> +static int adf41513_sync_config(struct adf41513_state *st, u16 sync_mask)
> +{
> + __be32 d32;
> + int ret, i;
> +
> + /* write registers in reverse order (R13 to R0)*/
> + for (i = ADF41513_REG13; i >= ADF41513_REG0; i--) {
Maybe
for (int i = ..
given that's now considered fine in kernel code.
You could reduce the scope of d32 and ret but that doesn't bring much advantage here.
> + if (st->regs_hw[i] == st->regs[i] && !(sync_mask & BIT(i)))
> + continue;
> +
> + d32 = cpu_to_be32(st->regs[i] | i);
> + ret = spi_write_then_read(st->spi, &d32, sizeof(d32), NULL, 0);
> + if (ret < 0)
> + return ret;
> + st->regs_hw[i] = st->regs[i];
> + dev_dbg(&st->spi->dev, "REG%d <= 0x%08X\n", i, st->regs[i] | i);
> + }
> +
> + return 0;
> +}

> +
> +static int adf41513_calc_variable_mod(struct adf41513_state *st,
> + struct adf41513_pll_settings *result)
> +{
> + u64 freq_error_uhz, mod2;
> + u32 frac1, frac2;
> + u16 int_value = div64_u64_rem(result->target_frequency_uhz,
> + result->pfd_frequency_uhz,
> + &freq_error_uhz);
> +
> + if (st->chip_info->has_prescaler_8_9 && int_value >= ADF41513_MIN_INT_FRAC_8_9 &&
> + int_value <= ADF41513_MAX_INT_8_9)
> + result->prescaler = 1;
> + else if (int_value >= ADF41513_MIN_INT_FRAC_4_5 && int_value <= ADF41513_MAX_INT_4_5)
> + result->prescaler = 0;
> + else
> + return -ERANGE;
See below for this value getting switched at caller.

> +
> + /* calculate required mod2 based on target resolution / 2 */
> + mod2 = DIV64_U64_ROUND_CLOSEST(result->pfd_frequency_uhz << 1,
> + st->data.freq_resolution_uhz * ADF41513_FIXED_MODULUS);
> + /* ensure mod2 is at least 2 for meaningful operation */
> + mod2 = clamp(mod2, 2, ADF41513_MAX_MOD2);
> +
> + /* 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);
> +
> + /* integer part */
> + result->actual_frequency_uhz = (u64)int_value * result->pfd_frequency_uhz;
> + /* fractional part */
> + result->actual_frequency_uhz += mul_u64_u64_div_u64(mod2 * frac1 + frac2,
> + result->pfd_frequency_uhz,
> + mod2 * ADF41513_FIXED_MODULUS);
> + result->mode = ADF41513_MODE_VARIABLE_MODULUS;
> + result->int_value = int_value;
> + result->frac1 = frac1;
> + result->frac2 = frac2;
> + result->mod2 = mod2;
> +
> + return 0;
> +}
> +
> +static int adf41513_calc_pll_settings(struct adf41513_state *st,
> + struct adf41513_pll_settings *result,
> + u64 rf_out_uhz)
> +{
> + u64 max_rf_freq_uhz = st->chip_info->max_rf_freq_hz * MICRO;
> + u64 min_rf_freq_uhz = ADF41513_MIN_RF_FREQ_HZ * MICRO;
> + u64 pfd_freq_limit_uhz;
> + int ret;
> +
> + if (rf_out_uhz < min_rf_freq_uhz || rf_out_uhz > max_rf_freq_uhz) {
> + dev_err(&st->spi->dev, "RF frequency %llu uHz out of range [%llu, %llu] uHz\n",
> + rf_out_uhz, min_rf_freq_uhz, max_rf_freq_uhz);
> + return -EINVAL;
> + }
> +
> + result->target_frequency_uhz = rf_out_uhz;
> +
> + /* try integer-N first (best phase noise performance) */
> + pfd_freq_limit_uhz = min(div_u64(rf_out_uhz, ADF41513_MIN_INT_4_5),
> + ADF41513_MAX_PFD_FREQ_INT_N_UHZ);
> + ret = adf41513_calc_pfd_frequency(st, result, pfd_freq_limit_uhz);
> + if (ret < 0)
> + return ret;
> +
> + if (adf41513_calc_integer_n(st, result) == 0)
> + return 0;
> +
> + /* try fractional-N: recompute pfd frequency if necessary */
> + pfd_freq_limit_uhz = min(div_u64(rf_out_uhz, ADF41513_MIN_INT_FRAC_4_5),
> + ADF41513_MAX_PFD_FREQ_FRAC_N_UHZ);
> + if (pfd_freq_limit_uhz < result->pfd_frequency_uhz) {
> + ret = adf41513_calc_pfd_frequency(st, result, pfd_freq_limit_uhz);
> + if (ret < 0)
> + return ret;
> + }
> +
> + /* fixed-modulus attempt */
> + if (adf41513_calc_fixed_mod(st, result) == 0)
> + return 0;
> +
> + /* variable-modulus attempt */
> + ret = adf41513_calc_variable_mod(st, result);
> + if (ret < 0) {
> + dev_err(&st->spi->dev,
> + "no valid PLL configuration found for %llu uHz\n",
> + rf_out_uhz);
> + return -EINVAL;

Why eat value of ret? If that does make sense add a comment so
this doesn't get 'cleaned up' in future.

I can sort of see it making sense as all this is a case of all
sorts of different cases failed rather than just this last one.
> + }
> +
> + return 0;
> +}

> +
> +static ssize_t adf41513_read_resolution(struct iio_dev *indio_dev,
> + uintptr_t private,
> + const struct iio_chan_spec *chan,
> + char *buf)
> +{
> + struct adf41513_state *st = iio_priv(indio_dev);
> + int vals[2];
> +
> + guard(mutex)(&st->lock);
> +
> + switch (private) {
> + case ADF41513_FREQ_RESOLUTION:
See below.

> + iio_val_s64_array_populate(st->data.freq_resolution_uhz, vals);
> + return iio_format_value(buf, IIO_VAL_DECIMAL64_MICRO,
> + ARRAY_SIZE(vals), vals);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static ssize_t adf41513_read_powerdown(struct iio_dev *indio_dev,
> + uintptr_t private,
> + const struct iio_chan_spec *chan,
> + char *buf)
> +{
> + struct adf41513_state *st = iio_priv(indio_dev);
> + u32 val;
> +
> + guard(mutex)(&st->lock);
> +
> + switch (private) {
> + case ADF41513_POWER_DOWN:
See below.

> + val = FIELD_GET(ADF41513_REG6_POWER_DOWN_MSK,
> + st->regs_hw[ADF41513_REG6]);
> + return sysfs_emit(buf, "%u\n", val);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static ssize_t adf41513_write_resolution(struct iio_dev *indio_dev,
> + uintptr_t private,
> + const struct iio_chan_spec *chan,
> + const char *buf, size_t len)
> +{
> + struct adf41513_state *st = iio_priv(indio_dev);
> + u64 freq_uhz;
> + int ret;
> +
> + ret = kstrtoudec64(buf, ADF41513_HZ_DECIMAL_SCALE, &freq_uhz);
> + if (ret)
> + return ret;
> +
> + guard(mutex)(&st->lock);
> +
> + switch ((u32)private) {
> + case ADF41513_FREQ_RESOLUTION:
See below.

> + if (freq_uhz == 0 || freq_uhz > ADF41513_MAX_FREQ_RESOLUTION_UHZ)
> + return -EINVAL;
> + st->data.freq_resolution_uhz = freq_uhz;
> + return len;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static ssize_t adf41513_write_powerdown(struct iio_dev *indio_dev,
> + uintptr_t private,
> + const struct iio_chan_spec *chan,
> + const char *buf, size_t len)
> +{
> + struct adf41513_state *st = iio_priv(indio_dev);
> + unsigned long readin;
> + int ret;
> +
> + ret = kstrtoul(buf, 10, &readin);
> + if (ret)
> + return ret;
> +
> + guard(mutex)(&st->lock);
> +
> + switch ((u32)private) {
> + case ADF41513_POWER_DOWN:
Silly question. How do we get a different value of this?

I'm going to guess this is either a code evolution thing where
we have reached a silly state, or you have other code that goes
on top of this series that needs this. If it's the 'more code'
case then do a refactor at start of that series and keep this simple for now.

Similar applies for the resolution functions.

> + if (readin)
> + ret = adf41513_suspend(st);
> + else
> + ret = adf41513_resume(st);
> + break;
Really trivial but I'd prefer.

if (ret)
return ret;

break;

Then unconditionally return len below. Tends to make things a bit
simpler to read if we get more elements in the switch in the longer term
as no need to go look for what happens in the error path.

> + default:
> + return -EINVAL;
> + }
> +
> + return ret ?: len;
> +}
> +
> +#define _ADF41513_EXT_PD_INFO(_name, _ident) { \
> + .name = _name, \
> + .read = adf41513_read_powerdown, \
> + .write = adf41513_write_powerdown, \
> + .private = _ident, \
> + .shared = IIO_SEPARATE, \
> +}
> +
> +#define _ADF41513_EXT_RES_INFO(_name, _ident) { \
> + .name = _name, \
> + .read = adf41513_read_resolution, \
> + .write = adf41513_write_resolution, \
> + .private = _ident, \
> + .shared = IIO_SEPARATE, \
> +}
> +
> +static const struct iio_chan_spec_ext_info adf41513_ext_info[] = {
> + _ADF41513_EXT_RES_INFO("frequency_resolution", ADF41513_FREQ_RESOLUTION),
> + _ADF41513_EXT_PD_INFO("powerdown", ADF41513_POWER_DOWN),
IF these only occur once - which I think is the case, drop the macros.

> + { }
> +};
> +
>