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

[Jonathan Cameron <jic23@kernel.org>]

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.

> +	{ }
> +};
> +
>