Re: [PATCH v4 3/7] iio: frequency: adf41513: driver implementation

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 16 Jan 2026 14:32:22 +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,

A couple of additional comments for this version.

Thanks,

Jonathan

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



> +
> +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 phase_urad;
> +	u16 phase_val;
> +
> +	guard(mutex)(&st->lock);
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_PHASE:
> +		phase_urad = (u64)val * MICRO + val2;
> +		if (val < 0 || val2 < 0 || phase_urad >= ADF41513_MAX_PHASE_MICRORAD)

Check val and val2 before setting phase_urad.  Whilst it's not a bug it
is a lot less readable to perform checks on inputs after you've already used
them to compute something.
		if (val < 0 || val2 < 0)
			return -EINVAL;

		phase_urad = ...
		if (phase_urad >= ...)
			return -EINVAL;

> +			return -EINVAL;
> +
> +		phase_val = DIV_U64_ROUND_CLOSEST(phase_urad << 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);
> +		return adf41513_sync_config(st, ADF41513_SYNC_REG0);
> +	default:
> +		return -EINVAL;
> +	}
> +}

> +
> +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_HZ;
> +	ret = device_property_read_u32(dev, "adi,power-up-frequency-mhz", &tmp);
> +	if (!ret) {
Can easily do the same as below here  No precision issue given definition of
the _MAX_RF_FREQ_HZ value includes a larger power 10 multiplier.
	
	tmp = ADF41510_MAX_RF_FREQ_HZ / HZ_PER_MHZ;
	device_property_read_u32(dev, "adi,power-up-frequency-mhz", &tmp;
	st->data.power_up_frequency_hz = (u64)tmp * HZ_PER_MHZ;
	if (st...

or use a local u64 for a temporary you only assign to power_up_frequency_hz
after checks. That would be more consistent with the code that follows.

> +		st->data.power_up_frequency_hz = (u64)tmp * HZ_PER_MHZ;
> +		if (st->data.power_up_frequency_hz < ADF41513_MIN_RF_FREQ_HZ ||
> +		    st->data.power_up_frequency_hz > ADF41513_MAX_RF_FREQ_HZ)
> +			return dev_err_probe(dev, -ERANGE,
> +					     "power-up frequency %llu Hz out of range\n",
> +					     st->data.power_up_frequency_hz);
> +	}
> +
> +	tmp = ADF41513_MIN_R_CNT;
> +	device_property_read_u32(dev, "adi,reference-div-factor", &tmp);
> +	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;
> +	device_property_read_u32(dev, "adi,charge-pump-resistor-ohms", &cp_resistance);
> +	if (cp_resistance < ADF41513_MIN_R_SET || cp_resistance > ADF41513_MAX_R_SET)
> +		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;

This leaves some odd corner cases.
If DT defines cp_resistance but not cp_current then we ignore the cp_resitance.
If you want to insist it is either both or nothing, that needs enforcing in the dt-binding.
I think I slightly prefer this option..

Alternative is define a default current such that the maths works to give the DEFAULT_CP_VOLTAGE_mV
if both properties use defaults and use that here + document in the dt-binding as the default
for this property.   That may mean if only one property is set we reject the pair and fail
to probe.  You have comment about valid combinations in the dt-binding so that's fine.
 
> +	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)
> +			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");
> +
> +	tmp = ADF41513_LD_COUNT_MIN;
> +	device_property_read_u32(dev, "adi,lock-detector-count", &tmp);
> +	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;
> +}