Re: [PATCH v3 2/6] iio: frequency: adf41513: driver implementation

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 08 Jan 2026 12:14:51 +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.
> 
> Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>
Hi Rodrigo,

Just one significant point (though it's repeated a few times!).
I think you can simplify the firmware parsing code by changing how you
set the defaults.  That should both make it more readable and make
it more obvious that the necessary checks have parsed when you have
a mixture of default and values from DT.

thanks,

Jonathan

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

> +
> +static ssize_t adf41513_read_uhz(struct iio_dev *indio_dev,
> +				 uintptr_t private,
> +				 const struct iio_chan_spec *chan,
> +				 char *buf)
> +{
> +	struct adf41513_state *st = iio_priv(indio_dev);
> +	u64 freq_uhz;
> +
> +	guard(mutex)(&st->lock);
> +
> +	switch ((u32)private) {
> +	case ADF41513_FREQ:
> +		freq_uhz = adf41513_pll_get_rate(st);
> +		if (st->lock_detect)
> +			if (!gpiod_get_value_cansleep(st->lock_detect)) {
Trivial, ignore if you like:

Might as well combine the conditions
		if (st->lock_detect &&
		    !gpio_get_value_can_sleep(st->lock_detect)) {
		}
given the first is just a check on whether the second makes sense or not.

> +				dev_dbg(&st->spi->dev, "PLL un-locked\n");
> +				return -EBUSY;
> +			}
> +		break;

> +
> +static int adf41513_reg_access(struct iio_dev *indio_dev,
> +			       unsigned int reg,
> +			       unsigned int writeval,
> +			       unsigned int *readval)

static int adf41513_reg_access(struct iio_dev *indio_dev, unsigned int reg,
			       unsigned int writeval, unsigned int *readval)

would be fine for wrap here and save us a few lines of scrolling.

> +
> +static int adf41513_parse_fw(struct adf41513_state *st)
> +{
> +	struct device *dev = &st->spi->dev;
> +	int ret;
> +	u32 tmp, cp_resistance, cp_current;
> +
> +	/* power-up frequency */
> +	st->data.power_up_frequency_hz = ADF41510_MAX_RF_FREQ;
> +	ret = device_property_read_u32(dev, "adi,power-up-frequency-mhz", &tmp);
> +	if (!ret) {
> +		st->data.power_up_frequency_hz = (u64)tmp * HZ_PER_MHZ;
> +		if (st->data.power_up_frequency_hz < ADF41513_MIN_RF_FREQ ||
> +		    st->data.power_up_frequency_hz > ADF41513_MAX_RF_FREQ)
> +			return dev_err_probe(dev, -ERANGE,
> +					     "power-up frequency %llu Hz out of range\n",
> +					     st->data.power_up_frequency_hz);
> +	}
> +
> +	st->data.ref_div_factor = ADF41513_MIN_R_CNT;
Small thing, but for all of these, if you instead set the temporary variable
to whatever is the DT default (not the register value it maps to) then the handling
ends up simpler.  We don't care if we have to do a small amount of unnecessary maths
if the default is used.

	tmp = ADF41513_MIN_R_CNT;
	device_property_read_u32(dev, "adi,....", &tmp);
	if (tmp < ......)
		return dev_err_probe();

	st->data.ref_div_factor = tmp;

etc.  If you want to check ret for the explicit return value that means no property
then that's fine too but I've always been a bit relaxed on these.


> +	ret = device_property_read_u32(dev, "adi,reference-div-factor", &tmp);
> +	if (!ret) {
> +		if (tmp < ADF41513_MIN_R_CNT || tmp > ADF41513_MAX_R_CNT)
> +			return dev_err_probe(dev, -ERANGE,
> +					     "invalid reference div factor %u\n", tmp);
> +		st->data.ref_div_factor = tmp;
> +	}
> +
> +	st->data.ref_doubler_en = device_property_read_bool(dev, "adi,reference-doubler-enable");
> +	st->data.ref_div2_en = device_property_read_bool(dev, "adi,reference-div2-enable");
> +
> +	cp_resistance = ADF41513_DEFAULT_R_SET;
> +	ret = device_property_read_u32(dev, "adi,charge-pump-resistor-ohms", &cp_resistance);
> +	if (!ret && (cp_resistance < ADF41513_MIN_R_SET || cp_resistance > ADF41513_MAX_R_SET))

Don't need the if (!ret) bit, as the default will pass the other tests.

> +		return dev_err_probe(dev, -ERANGE, "R_SET %u Ohms out of range\n", cp_resistance);
> +
> +	st->data.charge_pump_voltage_mv = ADF41513_DEFAULT_CP_VOLTAGE_mV;
> +	ret = device_property_read_u32(dev, "adi,charge-pump-current-microamp", &cp_current);
> +	if (!ret) {
> +		tmp = DIV_ROUND_CLOSEST(cp_current * cp_resistance, MILLI); /* convert to mV */
> +		if (tmp < ADF41513_MIN_CP_VOLTAGE_mV || tmp > ADF41513_MAX_CP_VOLTAGE_mV)
One advantage of the suggested approach above is that we don't have to think
carefully on whether the default here * a custom value for cp_resistance would fail this
test because we check that explicitly.

> +			return dev_err_probe(dev, -ERANGE, "I_CP %u uA (%u Ohms) out of range\n",
> +					     cp_current, cp_resistance);
> +		st->data.charge_pump_voltage_mv = tmp;
> +	}
> +
> +	st->data.phase_detector_polarity =
> +		device_property_read_bool(dev, "adi,phase-detector-polarity-positive-enable");
> +
> +	st->data.logic_lvl_1v8_en = device_property_read_bool(dev, "adi,logic-level-1v8-enable");
> +
> +	st->data.lock_detect_count = ADF41513_LD_COUNT_MIN;
> +	ret = device_property_read_u32(dev, "adi,lock-detector-count", &tmp);
> +	if (!ret) {
> +		if (tmp < ADF41513_LD_COUNT_FAST_MIN || tmp > ADF41513_LD_COUNT_MAX ||
> +		    !is_power_of_2(tmp))
> +			return dev_err_probe(dev, -ERANGE,
> +					     "invalid lock detect count: %u\n", tmp);
> +		st->data.lock_detect_count = tmp;
> +	}
> +
> +	st->data.freq_resolution_uhz = MICROHZ_PER_HZ;
> +
> +	return 0;
> +}