Re: [PATCH v5 06/13] iio: frequency: ad9910: initial driver implementation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Incorrect usage of `device_property_read_u32_array` causes single-value property `output-range-microamp` to be ignored.
- [High] VCO_SEL field is incorrectly set to an out-of-bounds value (7) when the PLL is bypassed.
- [High] Stack out-of-bounds read in `ad9910_profile_set()` triggered by unvalidated GPIO descriptor count.
- [High] Missing resource teardown: device reset line is never asserted on driver removal.
- [Medium] PLL multiplier clamping logic violates the hardware's minimum VCO operating frequency.
- [Medium] Missing hardware initialization delay after reset deassertion can cause SPI communications to fail.
--

commit fb6e130c7d866653f35ab965acf927500a84756c
Author: Rodrigo Alencar <rodrigo.alencar@analog.com>

iio: frequency: ad9910: initial driver implementation

Add the core AD9910 DDS driver infrastructure with single tone mode
support. This includes SPI register access, profile management via GPIO
pins, PLL/DAC configuration from firmware properties, and single tone
frequency/phase/amplitude control through IIO attributes.

> diff --git a/drivers/iio/frequency/ad9910.c b/drivers/iio/frequency/ad9910.c
> new file mode 100644
> index 0000000000000..c7b1e474c92df
> --- /dev/null
> +++ b/drivers/iio/frequency/ad9910.c
[ ... ]
> +static int ad9910_set_sysclk_freq(struct ad9910_state *st, u32 freq_hz,
> +				  bool update)
> +{
[ ... ]
> +		tmp32 = clamp(tmp32, DIV_ROUND_UP(AD9910_PLL_OUT_MIN_FREQ_HZ, refclk_freq_hz),
> +			      AD9910_PLL_OUT_MAX_FREQ_HZ / refclk_freq_hz);
> +		tmp32 = clamp(tmp32, AD9910_PLL_MIN_N, AD9910_PLL_MAX_N);
> +		sysclk_freq_hz = refclk_freq_hz * tmp32;

Is it possible for this second clamp to drop the multiplier below the
minimum frequency constraint? If the input reference clock is 3.2 MHz,
the first clamp would raise the multiplier to 132 to reach 420 MHz, but
the second clamp limits it to 127. This results in a VCO frequency of
406.4 MHz, violating the hardware minimum. Should the driver return
-ERANGE instead if the bounds cannot be satisfied?

[ ... ]
> +	} else {
> +		tmp32 = DIV_ROUND_CLOSEST(refclk_freq_hz, freq_hz);
> +		tmp32 = clamp(tmp32, 1U, 2U);
> +		sysclk_freq_hz = refclk_freq_hz / tmp32;
> +		tmp32 = AD9910_CFR3_VCO_SEL_MSK |
> +			FIELD_PREP(AD9910_CFR3_REFCLK_DIV_BYPASS_MSK, tmp32 % 2);
> +		ret = ad9910_reg32_update(st, AD9910_REG_CFR3,
> +					  AD9910_CFR3_VCO_SEL_MSK |
> +					  AD9910_CFR3_REFCLK_DIV_BYPASS_MSK,
> +					  tmp32, update);

Does setting tmp32 with AD9910_CFR3_VCO_SEL_MSK cause the update function
to write an out-of-bounds value here? Because tmp32 contains the entire
mask, the underlying field will be set to 7, but the valid range for the
vco sel field is 0 to 5.

[ ... ]
> +static int ad9910_profile_set(struct ad9910_state *st, u8 profile)
> +{
> +	DECLARE_BITMAP(values, BITS_PER_TYPE(profile));
> +
> +	st->profile = profile;
> +	values[0] = profile;
> +	gpiod_multi_set_value_cansleep(st->gpio_profile, values);
> +
> +	return 0;
> +}

Could this cause an out-of-bounds read on the stack? The values bitmap is
sized using BITS_PER_TYPE(profile), which allocates space for 1 unsigned
long. If the device tree provides more profile gpios than fit in an unsigned
long, gpiod_multi_set_value_cansleep() might read past the end of the bitmap
when iterating over the descriptors.

[ ... ]
> +static int ad9910_parse_fw(struct ad9910_state *st)
> +{
[ ... ]
> +	tmp[1] = AD9910_DAC_IOUT_DEFAULT_uA;
> +	device_property_read_u32_array(dev, "output-range-microamp", tmp,
> +				       ARRAY_SIZE(tmp));

Will this properly read a single-value property? Because ARRAY_SIZE(tmp)
is passed as the expected element count, device_property_read_u32_array()
might return -EOVERFLOW and leave the tmp array unmodified if only one
value is present in the device tree, causing the property to be silently
ignored.

[ ... ]
> +static int ad9910_setup(struct device *dev, struct ad9910_state *st,
> +			struct reset_control *dev_rst)
> +{
> +	int ret;
> +
> +	ret = reset_control_deassert(dev_rst);
> +	if (ret)
> +		return ret;
> +

Is a teardown action missing for the device reset here? Without adding a
managed action to assert the reset line, the hardware might remain active
after the driver is unloaded or if a subsequent setup step fails.

Might this also require a hardware delay before the first spi write? The
datasheet specifies waiting for the internal state machine to become ready
after a low-to-high transition on the reset line. On fast hosts, the
immediate spi access might be ignored.

> +	ret = ad9910_reg32_write(st, AD9910_REG_CFR1,
> +				 (st->spi->mode & SPI_3WIRE ? 0 :
> +				 AD9910_CFR1_SDIO_INPUT_ONLY_MSK), false);
> +	if (ret)
> +		return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260517-ad9910-iio-driver-v5-0-31599c88314a@analog.com?part=6