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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The `adi,refclk-out-drive-strength` property is ignored if the PLL is disabled.
- [Medium] Hardware and software cache mismatch for PROFILE registers when device reset is absent, leading to accidental zeroing of amplitude and phase.
- [Low] Using `kstrtou32` instead of `kstrtobool` for boolean sysfs attribute.
--

--- Patch [10]: [PATCH v10 10/17] iio: frequency: ad9910: initial driver implementation ---
commit ca73a84a1a8f926aea1ac7c0a916c43aaab49400
Author: Rodrigo Alencar <rodrigo.alencar@analog.com>

    iio: frequency: ad9910: initial driver implementation

This commit adds 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..b41b011af2812 100644
> --- /dev/null
> +++ b/drivers/iio/frequency/ad9910.c
[ ... ]
> +static ssize_t ad9910_ext_info_write(struct iio_dev *indio_dev,
> +				     uintptr_t private,
> +				     const struct iio_chan_spec *chan,
> +				     const char *buf, size_t len)
> +{
> +	struct ad9910_state *st = iio_priv(indio_dev);
> +	u32 val32;
> +	int ret;
> +
> +	ret = kstrtou32(buf, 10, &val32);

[Severity: Low]
Could this use kstrtobool() instead of kstrtou32() to parse the boolean
powerdown attribute?

Using kstrtobool() allows userspace tools to use standard boolean character
inputs (like 'Y' or 'N') in addition to numeric inputs, which is standard for
sysfs boolean attributes.

> +	if (ret)
> +		return ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	switch (private) {
> +	case AD9910_POWERDOWN:
> +		ret = ad9910_sw_powerdown_set(st, val32 ? true : false);
> +		if (ret)
> +			return ret;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return len;
> +}
[ ... ]
> +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_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: Medium]
Will this silently overwrite the amplitude and phase configurations if the
device doesn't have a hardware reset line?

If the reset control is optional and absent, the hardware's PROFILE registers
may hold non-zero values (e.g., initialized by a bootloader). However, the
software cache (st->reg) remains zero-initialized. 

When ad9910_reg64_update() performs a read-modify-write against this zeroed
software cache, it will overwrite the hardware's amplitude (ASF) and phase
(POW) fields with 0. Should the driver read the initial hardware values into
the cache or write known default values during setup to avoid this
desynchronization?

> +		default:
> +			return -EINVAL;
> +		}
[ ... ]
> +static int ad9910_parse_fw(struct ad9910_state *st)
> +{
> +	static const char * const refclk_out_drv0[] = {
> +		"disabled", "low", "medium", "high",
> +	};
> +	struct device *dev = &st->spi->dev;
> +	const char *prop;
> +	u32 tmp;
> +	int ret;
> +
> +	st->data.pll_enabled = device_property_read_bool(dev, "adi,pll-enable");
> +	if (st->data.pll_enabled) {
> +		prop = "adi,charge-pump-current-microamp";
> +		if (device_property_present(dev, prop)) {
> +			ret = device_property_read_u32(dev, prop, &tmp);
> +			if (ret)
> +				return dev_err_probe(dev, ret, "property read: %s\n", prop);
> +
> +			if (tmp < AD9910_ICP_MIN_uA || tmp > AD9910_ICP_MAX_uA)
> +				return dev_err_probe(dev, -ERANGE,
> +						     "invalid charge pump current %u\n", tmp);
> +		} else {
> +			tmp = AD9910_ICP_MIN_uA;
> +		}
> +		st->data.pll_charge_pump_current = tmp;
> +
> +		prop = "adi,refclk-out-drive-strength";
> +		if (device_property_present(dev, prop)) {
> +			ret = device_property_match_property_string(dev, prop,
> +								    refclk_out_drv0,
> +								    ARRAY_SIZE(refclk_out_drv0));
> +			if (ret < 0)
> +				return dev_err_probe(dev, ret, "property read: %s\n", prop);
> +
> +			st->data.refclk_out_drv = ret;
> +		}

[Severity: Medium]
Will this ignore the "adi,refclk-out-drive-strength" property when the PLL is
disabled?

Since the AD9910 allows the REFCLK_OUT pin to buffer the reference clock even
when the PLL is bypassed, it seems this property should be parsed independently
of the "adi,pll-enable" property. Otherwise, ad9910_cfg_sysclk() will
unconditionally write 0 to the DRV0 bits in CFR3, disabling the drive strength
when the PLL is bypassed.

> +	}
> +
> +	return 0;
> +}

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