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

[Rodrigo Alencar]

On 26/05/06 03:08PM, Rodrigo Alencar via B4 Relay wrote:
> From: Rodrigo Alencar <rodrigo.alencar@xxxxxxxxxx>
> 
> 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

Sashiko's feedback:
https://sashiko.dev/#/patchset/20260506-adf41513-iio-driver-v11-0-2b7e99cfe8f2%40analog.com?part=7

There is a lot of stuff here, most of things are good points, but maybe it
was not that bad. It could've been worse with those fractional mode calculations.

...

> +static u64 adf41513_pll_get_rate(struct adf41513_state *st)
> +{
> +	struct adf41513_pll_settings *cfg = &st->settings;
> +
> +	if (cfg->mode != ADF41513_MODE_INVALID)
> +		return cfg->actual_frequency_uhz;
> +
> +	/* get pll settings from regs_hw */
> +	cfg->int_value = FIELD_GET(ADF41513_REG0_INT_MSK, st->regs_hw[ADF41513_REG0]);
> +	cfg->frac1 = FIELD_GET(ADF41513_REG1_FRAC1_MSK, st->regs_hw[ADF41513_REG1]);
> +	cfg->frac2 = FIELD_GET(ADF41513_REG3_FRAC2_MSK, st->regs_hw[ADF41513_REG3]);
> +	cfg->mod2 = FIELD_GET(ADF41513_REG4_MOD2_MSK, st->regs_hw[ADF41513_REG4]);
> +	cfg->r_counter = FIELD_GET(ADF41513_REG5_R_CNT_MSK, st->regs_hw[ADF41513_REG5]);
> +	cfg->ref_doubler = FIELD_GET(ADF41513_REG5_REF_DOUBLER_MSK, st->regs_hw[ADF41513_REG5]);
> +	cfg->ref_div2 = FIELD_GET(ADF41513_REG5_RDIV2_MSK, st->regs_hw[ADF41513_REG5]);
> +	cfg->prescaler = FIELD_GET(ADF41513_REG5_PRESCALER_MSK, st->regs_hw[ADF41513_REG5]);
> +
> +	/* calculate pfd frequency */
> +	cfg->pfd_frequency_uhz = (u64)st->ref_freq_hz * MICRO;
> +	if (cfg->ref_doubler)
> +		cfg->pfd_frequency_uhz <<= 1;
> +	if (cfg->ref_div2)
> +		cfg->pfd_frequency_uhz >>= 1;
> +	cfg->pfd_frequency_uhz = div_u64(cfg->pfd_frequency_uhz, cfg->r_counter);

	If a user writes 0 to the R_CNT field via debugfs direct register access, won't
	this cause a division by zero kernel panic?

The 0 value for R counter means 32, so this is a bug indeed.

> +	cfg->actual_frequency_uhz = (u64)cfg->int_value * cfg->pfd_frequency_uhz;
> +
> +	/* check if int mode is selected */
> +	if (FIELD_GET(ADF41513_REG6_INT_MODE_MSK, st->regs_hw[ADF41513_REG6])) {
> +		cfg->mode = ADF41513_MODE_INTEGER_N;
> +	} else {
> +		cfg->actual_frequency_uhz += mul_u64_u32_div(cfg->pfd_frequency_uhz,
> +							     cfg->frac1,
> +							     ADF41513_FIXED_MODULUS);
> +
> +		/* check if variable modulus is selected */
> +		if (FIELD_GET(ADF41513_REG0_VAR_MOD_MSK, st->regs_hw[ADF41513_REG0])) {
> +			cfg->actual_frequency_uhz +=
> +				mul_u64_u64_div_u64(cfg->frac2,
> +						    cfg->pfd_frequency_uhz,
> +						    (u64)cfg->mod2 * ADF41513_FIXED_MODULUS);

	Similarly, if the MOD2 field is written as 0 via debugfs, will this result in
	a division by zero panic?

I've just checked in hardware and it seems mod2 = 0 behave as mod2 = 1, so it does not fallback to
fixed modulus. Will adjust so that this is consistent.

...

> +static int adf41513_calc_integer_n(struct adf41513_state *st,
> +				   struct adf41513_pll_settings *result)
> +{
> +	u16 max_int = st->chip_info->has_prescaler_8_9 ?
> +		      ADF41513_MAX_INT_8_9 : ADF41513_MAX_INT_4_5;
> +	u64 freq_error_uhz;
> +	u16 int_value = div64_u64_rem(result->target_frequency_uhz, result->pfd_frequency_uhz,
> +				      &freq_error_uhz);

	If the target frequency is very high and the PFD frequency is very low, could
	the 64-bit quotient overflow the u16 int_value and silently truncate?
	This would bypass the subsequent bounds check on int_value and incorrectly
	program the hardware instead of returning -ERANGE.

That would be unusual as max_int is way less than U16_MAX, but maybe possible with very low
ref frequency and very high dividers, so will make int_value as u32.

> +
> +	/* check if freq error is within a tolerance of 1/2 resolution */
> +	if (freq_error_uhz > (result->pfd_frequency_uhz >> 1) && int_value < max_int) {
> +		int_value++;
> +		freq_error_uhz = result->pfd_frequency_uhz - freq_error_uhz;
> +	}
> +
> +	if (freq_error_uhz > st->data.freq_resolution_uhz)
> +		return -ERANGE;
> +
> +	/* set prescaler */
> +	if (st->chip_info->has_prescaler_8_9 && int_value >= ADF41513_MIN_INT_8_9 &&
> +	    int_value <= ADF41513_MAX_INT_8_9)
> +		result->prescaler = 1;
> +	else if (int_value >= ADF41513_MIN_INT_4_5 && int_value <= ADF41513_MAX_INT_4_5)
> +		result->prescaler = 0;
> +	else
> +		return -ERANGE;

...

> +static int adf41513_set_frequency(struct adf41513_state *st, u64 freq_uhz, u16 sync_mask)
> +{
> +	struct adf41513_pll_settings result;
> +	int ret;
> +
> +	ret = adf41513_calc_pll_settings(st, &result, freq_uhz);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* apply computed results to pll settings */
> +	st->settings = result;
> +
> +	dev_dbg(&st->spi->dev,
> +		"%s mode: int=%u, frac1=%u, frac2=%u, mod2=%u, fpdf=%llu Hz, prescaler=%s\n",
> +		(result.mode == ADF41513_MODE_INTEGER_N) ? "integer-n" :
> +		(result.mode == ADF41513_MODE_FIXED_MODULUS) ? "fixed-modulus" : "variable-modulus",
> +		result.int_value, result.frac1, result.frac2, result.mod2,
> +		div64_u64(result.pfd_frequency_uhz, MICRO),
> +		result.prescaler ? "8/9" : "4/5");
> +
> +	st->regs[ADF41513_REG0] = FIELD_PREP(ADF41513_REG0_INT_MSK,
> +					     st->settings.int_value);
> +	if (st->settings.mode == ADF41513_MODE_VARIABLE_MODULUS)
> +		st->regs[ADF41513_REG0] |= ADF41513_REG0_VAR_MOD_MSK;
> +
> +	st->regs[ADF41513_REG1] = FIELD_PREP(ADF41513_REG1_FRAC1_MSK,
> +					     st->settings.frac1);
> +	if (st->settings.mode != ADF41513_MODE_INTEGER_N)
> +		st->regs[ADF41513_REG1] |= ADF41513_REG1_DITHER2_MSK;
> +
> +	st->regs[ADF41513_REG3] = FIELD_PREP(ADF41513_REG3_FRAC2_MSK,
> +					     st->settings.frac2);
> +	FIELD_MODIFY(ADF41513_REG4_MOD2_MSK, &st->regs[ADF41513_REG4],
> +		     st->settings.mod2);
> +	FIELD_MODIFY(ADF41513_REG5_R_CNT_MSK, &st->regs[ADF41513_REG5],
> +		     st->settings.r_counter);

	The driver defines ADF41513_MAX_R_CNT as 32, but the ADF41513_REG5_R_CNT_MSK is
	only 5 bits wide (GENMASK(20, 16)). If r_counter is 32, will this FIELD_MODIFY
	silently truncate it to 0?
	If this is truncated to 0, and the device state is invalidated, a subsequent
	sysfs frequency read would call adf41513_pll_get_rate() and trigger a division
	by zero panic.

The bitfield with zero value behaves as R_CNT = 32, so this should not be a problem,
but I suppose that should be explicit and as pointed out above, it is not being taken
care in adf41513_pll_get_rate().

...

> +static int adf41513_resume(struct adf41513_state *st)
> +{
> +	st->regs[ADF41513_REG6] &= ~ADF41513_REG6_POWER_DOWN_MSK;
> +	return adf41513_sync_config(st, ADF41513_SYNC_DIFF);
> +}

	If regulators are turned off during system suspend, the hardware registers will
	be wiped. However, since regs_hw isn't invalidated during suspend, won't
	ADF41513_SYNC_DIFF falsely assume the hardware is still configured and skip
	reprogramming it?

Indeed, I might need to use SYNC_ALL here when doing power management.

	Also, doesn't the unconditional clearing of ADF41513_REG6_POWER_DOWN_MSK clobber
	any prior explicit powerdown request made via sysfs?

Another good point, may need to have a powerdown field in the state struct to
track this.  

...

> +static int adf41513_parse_fw(struct adf41513_state *st)
> +{

...

> +	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 */

	Could cp_current * cp_resistance overflow the 32-bit boundary?
	If an abnormally large current is provided via devicetree, the overflow could
	pass the subsequent voltage range checks and configure the hardware with an
	incorrect charge pump setting. Should this use mul_u32_u32() or a 64-bit cast?

I can add a bound/max check for cp_current first, as it needs to be between 81 and 7200 micro amps.

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

...

> +static int adf41513_pm_suspend(struct device *dev)
> +{
> +	return adf41513_suspend(dev_get_drvdata(dev));
> +}

	Is it safe to call adf41513_suspend() without acquiring st->lock?
	This function modifies the shared software register cache and executes SPI
	writes. It seems this could race concurrently with sysfs reads/writes or
	IIO core accesses.

Is this a real concern?

...

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

...

> +	st->chip_enable = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_HIGH);
> +	if (IS_ERR(st->chip_enable))
> +		return dev_err_probe(dev, PTR_ERR(st->chip_enable),
> +				     "fail to request chip enable GPIO\n");

...

> +	ret = adf41513_setup(st);

	Is a delay needed between asserting the chip_enable GPIO and sending the first
	SPI command in adf41513_setup()?
	Hardware PLLs typically require a wake-up or stabilization time for internal
	regulators. Without a usleep_range(), could the initial SPI commands be
	dropped by the hardware?

I havent see the need for this, mostly because a lot seems to be happening before the
first SPI write and after the chip enable goes high, so I will ignore this.

> +	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 adf41513_setup() returns an error, the probe function aborts before
	devm_add_action_or_reset() registers the adf41513_power_down action.
	Since chip_enable is acquired with GPIOD_OUT_HIGH, devres will disable the
	regulators but leave the GPIO high. Could this cause the host processor to
	backpower the unpowered synthesizer IC through its ESD diodes?

Another good point, will separate the reset actions for sw powerdown and chip enable
gpio.

-- 
Kind regards,

Rodrigo Alencar