Re: [PATCH v6 4/8] iio: frequency: adf41513: driver implementation

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 30 Jan 2026 10:06:45 +0000
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
> 
> The driver uses 64-bit microhertz values throughout PLL calculations to
> maintain precision when working with frequencies that exceed 32-bit Hz
> representation while requiring fractional Hz resolution.
> 
> When merging, ADF41513_HZ_PER_GHZ must be dropped in favor of
> HZ_PER_GHZ defined in linux/units.h.
> 
> Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>
Hi Rodrigo

A few minor suggestions inline from a reread.

Thanks

Jonathan

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

> +
> +struct adf41513_state {
> +	const struct adf41513_chip_info *chip_info;
> +	struct spi_device *spi;
> +	struct gpio_desc *lock_detect;
> +	struct gpio_desc *chip_enable;
> +	struct clk *ref_clk;
> +	u32 ref_freq_hz;
> +
> +	/*
> +	 * Lock for accessing device registers. Some operations require
> +	 * multiple consecutive R/W operations, during which the device
> +	 * shouldn't be interrupted. The buffers are also shared across
> +	 * all operations so need to be protected on stand alone reads and
> +	 * writes.
> +	 */
> +	struct mutex lock;
> +
> +	/* Cached register values */
> +	u32 regs[ADF41513_REG_NUM];
> +	u32 regs_hw[ADF41513_REG_NUM];
> +
> +	struct adf41513_data data;
> +	struct adf41513_pll_settings settings;
> +
> +	/*
> +	 * DMA (thus cache coherency maintenance) may require that
> +	 * transfer buffers live in their own cache lines.
> +	 */
> +	__be32 buf __aligned(IIO_DMA_MINALIGN);

Given this is very small, you could just use spi_write_the_read() with 
zero length reads. That bounces the data anyway so doesn't need a DMA safe buffer.

> +};


> +
> +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;
> +
> +	ret = adf41513_calc_integer_n(st, result);

When we have a series of methods to try it can be cleaner to return early in
the good paths. Given you don't return early error values, but rther just
try the next method you can do.

	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;
	}

	return 0;
}
		
> +	if (ret < 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 */
> +		ret = adf41513_calc_fixed_mod(st, result);
> +		if (ret < 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;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +


> +
> +static int adf41513_parse_fw(struct adf41513_state *st)
> +{
> +	struct device *dev = &st->spi->dev;
> +	int ret;

Trivial:
Where no other reason for a particular order of declarations, nice to just use
reverse xmas tree.

> +	u32 tmp, cp_resistance, cp_current;
> +

...

> +static int adf41513_probe(struct spi_device *spi)
> +{
...

> +	ret = devm_mutex_init(dev, &st->lock);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->name = spi_get_device_id(spi)->name;

I'd prefer we avoided this look up in the the device_id and instead
just put the name string for each part in the chip_info structure.

We've had far too many odd bugs around mismatches between firmware
tables to try to use any of the match tables for this. Better to have
one source of truth that we can get form any match table.

> +	indio_dev->info = &adf41513_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = &adf41513_chan;
> +	indio_dev->num_channels = 1;
> +
> +	ret = adf41513_setup(st);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "failed to setup device\n");
> +
> +	ret = devm_add_action_or_reset(dev, adf41513_power_down, st);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to add power down action\n");
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}