Re: [PATCH v7 10/17] iio: frequency: ad9910: initial driver implementation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Uninitialized profile register caches cause valid hardware defaults to be silently overwritten with zeros upon frequency updates.
- [High] Software power-down sets the REFCLK power-down bit, permanently disabling I/O updates and bricking the device.
- [Medium] Silent failure and state mismatch when switching profiles without configured profile GPIOs.
- [Low] Format string mismatch using `%u` for an `unsigned long` variable.
--

--- Patch [10]: [PATCH v7 10/17] iio: frequency: ad9910: initial driver implementation ---
commit dabebf1bc32a608ba0826f1747d162f60c3129d1
Author: Rodrigo Alencar <rodrigo.alencar@analog.com>
Subject: 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
> index 0000000000000..900c0323a1739
> --- /dev/null
> +++ b/drivers/iio/frequency/ad9910.c
> @@ -0,0 +1,1107 @@

[ ... ]

> +/* CFR1 bit definitions */
> +#define AD9910_CFR1_RAM_ENABLE_MSK		BIT(31)

[ ... ]

> +#define AD9910_CFR1_AUX_DAC_POWER_DOWN_MSK	BIT(4)
> +#define AD9910_CFR1_SOFT_POWER_DOWN_MSK		GENMASK(7, 4)

[Severity: High]
Does this mask inadvertently include the reference clock power-down bit?

The soft power down mask encompasses bit 5, which corresponds to
AD9910_CFR1_REFCLK_INPUT_POWER_DOWN_MSK. When userspace writes to the
powerdown attribute, ad9910_sw_powerdown_set() in 
drivers/iio/frequency/ad9910.c applies this mask to CFR1. 

Powering down the reference clock stops the internal SYNC_CLK. Since the
I/O update mechanism requires SYNC_CLK to latch SPI register writes,
subsequent SPI writes might fail to wake up the device.

[ ... ]

> +static int ad9910_set_sysclk_freq(struct ad9910_state *st, u32 freq_hz,
> +				  bool update)
> +{
> +	struct device *dev = &st->spi->dev;
> +	unsigned long refclk_freq_hz;
> +	u32 sysclk_freq_hz;
> +	u32 tmp32, vco_sel;
> +	int ret;
> +
> +	if (!freq_hz || freq_hz > AD9910_MAX_SYSCLK_HZ)
> +		return -EINVAL;
> +
> +	refclk_freq_hz = clk_get_rate(st->refclk);
> +	if (st->data.pll_enabled) {
> +		if (refclk_freq_hz < AD9910_PLL_IN_MIN_FREQ_HZ ||
> +		    refclk_freq_hz > AD9910_PLL_IN_MAX_FREQ_HZ) {
> +			dev_err(dev,
> +				"REF_CLK freq %u Hz is out of PLL input range\n",
> +				refclk_freq_hz);

[Severity: Low]
Should this use the %lu format specifier?

The variable refclk_freq_hz in drivers/iio/frequency/ad9910.c is of type
unsigned long, so using %u might cause a format string mismatch and
potential truncation on 64-bit systems.

> +			return -ERANGE;
> +		}
> +
> +		tmp32 = DIV_ROUND_CLOSEST(freq_hz, refclk_freq_hz);

[ ... ]

> +	} else {
> +		if (refclk_freq_hz < AD9910_REFDIV2_MIN_FREQ_HZ ||
> +		    refclk_freq_hz > AD9910_REFDIV2_MAX_FREQ_HZ) {
> +			dev_err(dev,
> +				"REF_CLK freq %u Hz is out of divider range\n",
> +				refclk_freq_hz);

[Severity: Low]
Similarly, could this also use %lu to match the unsigned long type of
refclk_freq_hz?

> +			return -ERANGE;
> +		}
> +
> +		tmp32 = DIV_ROUND_CLOSEST(refclk_freq_hz, freq_hz);

[ ... ]

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

[Severity: Medium]
Will this return success even if the hardware state cannot be changed?

During ad9910_probe() in drivers/iio/frequency/ad9910.c, the profile GPIO
array is requested as optional. If these GPIOs are not present on the
system, gpiod_multi_set_value_cansleep() acts as a no-op and returns 0.
As a result, ad9910_profile_set() updates the software state and reports
success back to userspace, but the actual hardware profile remains
unchanged.

[ ... ]

> +static int ad9910_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int val, int val2, long info)
> +{
> +	struct ad9910_state *st = iio_priv(indio_dev);
> +	u64 tmp64;
> +	u32 tmp32;
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_ENABLE:

[ ... ]

> +	case IIO_CHAN_INFO_FREQUENCY:
> +		if (val < 0 || val2 < 0 || val >= st->data.sysclk_freq_hz / 2)
> +			return -EINVAL;
> +
> +		tmp64 = ad9910_rational_scale((u64)val * MICRO + val2, BIT_ULL(32),
> +					      (u64)MICRO * st->data.sysclk_freq_hz);
> +		tmp64 = min_t(u64, tmp64, U32_MAX);
> +		switch (chan->channel) {
> +		case AD9910_CHANNEL_PROFILE_0 ... AD9910_CHANNEL_PROFILE_7:
> +			tmp32 = chan->channel - AD9910_CHANNEL_PROFILE_0;
> +			tmp64 = FIELD_PREP(AD9910_PROFILE_ST_FTW_MSK, tmp64);
> +			return ad9910_reg64_update(st, AD9910_REG_PROFILE(tmp32),
> +						   AD9910_PROFILE_ST_FTW_MSK,
> +						   tmp64, true);

[Severity: High]
Does this silently overwrite the hardware defaults with zeros when the
frequency is updated?

During initialization in ad9910_setup() in drivers/iio/frequency/ad9910.c,
the device is reset to its default state, populating the profile registers
in hardware with non-zero values for the Amplitude Scale Factor. However,
the driver's cached shadow registers in st->reg remain initialized to zero.

When ad9910_reg64_update() computes the new register value, it uses:

    (st->reg[reg].val64 & ~mask)

Since the uninitialized cache is zero, this clears the amplitude scale
factor in the hardware, dropping the output amplitude to zero.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-ad9910-iio-driver-v7-0-a4ec30f63700@analog.com?part=10