Re: [PATCH RFC v2 2/9] iio: frequency: ad9910: initial driver implementation

[Jonathan Cameron]

On Wed, 18 Mar 2026 17:56:02 +0000
Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@xxxxxxxxxx> wrote:

> From: Rodrigo Alencar <rodrigo.alencar@xxxxxxxxxx>
> 
> 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.
> 
> Signed-off-by: Rodrigo Alencar <rodrigo.alencar@xxxxxxxxxx>
Hi Rodrigo

I want some time for the discussion on the ABI to take place and haven't
made any real comments on that here. Focus was more on the code.

So various minor things inline,

Thanks,

Jonathan

> diff --git a/drivers/iio/frequency/Kconfig b/drivers/iio/frequency/Kconfig
> index 583cbdf4e8cd..180e74f62d11 100644
> --- a/drivers/iio/frequency/Kconfig
> +++ b/drivers/iio/frequency/Kconfig

> diff --git a/drivers/iio/frequency/ad9910.c b/drivers/iio/frequency/ad9910.c
> new file mode 100644
> index 000000000000..a362d96cf651
> --- /dev/null
> +++ b/drivers/iio/frequency/ad9910.c
> @@ -0,0 +1,1006 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * AD9910 SPI DDS (Direct Digital Synthesizer) driver
> + *
> + * Copyright 2026 Analog Devices Inc.
> + */
> +
> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>

Generally can avoid including device.h in favour of more specific
headers.  There are a few exceptions where we can't such as actual
dereferencing of struct device, but I don't recall seeing a case in here.

> +
> +#define AD9910_ASF_MAX			(BIT(14) - 1)
> +#define AD9910_POW_MAX			(BIT(16) - 1)
GENMASK() tends to be clearer for max values.



> +
> +static const char * const ad9910_power_supplies[] = {
> +	"dvdd-io33", "avdd33", "dvdd18", "avdd18",
> +};
> +
> +static const char * const ad9910_refclk_out_drv0[] = {
> +	"disabled", "low", "medium", "high",
> +};

These are only used in very localized bits of code. I'd move them down near
them rather than having them up here at the top of the file.


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

Trivial but blank line here. Having one before a simple return statement
just makes it a little easier to read.

> +	return 0;
> +}
>
> +
> +#define AD9910_EXT_INFO(_name, _ident, _shared) { \
> +	.name = _name, \
> +	.read = ad9910_ext_info_read, \
> +	.write = ad9910_ext_info_write, \
> +	.private = _ident, \
> +	.shared = _shared, \

If there are only a few of these, I'd put it long hand rather than
using a macro. Tends to end up easier to read.

> +}
> +
> +static const struct iio_chan_spec_ext_info ad9910_phy_ext_info[] = {
> +	AD9910_EXT_INFO("profile", AD9910_PROFILE, IIO_SEPARATE),
> +	AD9910_EXT_INFO("powerdown", AD9910_POWERDOWN, IIO_SEPARATE),
> +	{ }
> +};

> +static int ad9910_read_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;
> +
> +	guard(mutex)(&st->lock);
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_FREQUENCY:
> +		switch (chan->channel) {
> +		case AD9910_CHANNEL_SINGLE_TONE:

I haven't read on yet, but if you never have any other cases in here,
perhaps us an if() as it will reduce indent of the code that follows.

> +			tmp32 = FIELD_GET(AD9910_PROFILE_ST_FTW_MSK,
> +					  st->reg[AD9910_REG_PROFILE(st->profile)].val64);
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		tmp64 = (u64)tmp32 * st->data.sysclk_freq_hz;
> +		*val = upper_32_bits(tmp64);
> +		*val2 = upper_32_bits((u64)lower_32_bits(tmp64) * MICRO);

I've no idea how this *val2 assignment works... Perhaps some comments?

> +		return IIO_VAL_INT_PLUS_MICRO;
> +	case IIO_CHAN_INFO_PHASE:
> +		switch (chan->channel) {
> +		case AD9910_CHANNEL_SINGLE_TONE:
> +			tmp64 = FIELD_GET(AD9910_PROFILE_ST_POW_MSK,



> +
> +static int ad9910_reg_access(struct iio_dev *indio_dev,
> +			     unsigned int reg,
> +			     unsigned int writeval,
> +			     unsigned int *readval)
> +{
> +	struct ad9910_state *st = iio_priv(indio_dev);
> +	int ret;
> +	u64 tmp64;
> +	u32 tmp32;
> +	u16 tmp16;
> +	bool high32 = FIELD_GET(AD9910_REG_HIGH32_FLAG_MSK, reg);
> +
> +	/*
> +	 * HIGH32 flag is a workaround to allow access to upper 32 bits of
> +	 * 64-bit registers one at a time due to debugfs_reg_access limitations
> +	 * of only supporting 32-bit values.
> +	 */
> +	reg &= ~AD9910_REG_HIGH32_FLAG_MSK;
> +	if (reg >= AD9910_REG_RAM)
> +		return -EINVAL;
> +
> +	guard(mutex)(&st->lock);
> +
> +	switch (reg) {
Split this in to two helpers.  It's rather hard to follow with read and
write paths mixed up in the code flow. 
> +	case AD9910_REG_DRG_LIMIT:
> +	case AD9910_REG_DRG_STEP:
> +	case AD9910_REG_PROFILE0:
Can you do
	case AD9910_REG_PROFILE0 ... AD9910_REG_PROFILE7:
here to help readability?

> +	case AD9910_REG_PROFILE1:
> +	case AD9910_REG_PROFILE2:
> +	case AD9910_REG_PROFILE3:
> +	case AD9910_REG_PROFILE4:
> +	case AD9910_REG_PROFILE5:
> +	case AD9910_REG_PROFILE6:
> +	case AD9910_REG_PROFILE7:
> +		if (readval) {
> +			ret = ad9910_reg64_read(st, reg, &tmp64);
> +			if (ret < 0)
> +				return ret;
> +
> +			if (high32)
> +				*readval = upper_32_bits(tmp64);
> +			else
> +				*readval = lower_32_bits(tmp64);
Might as well return 0 here.

> +		} else {
> +			tmp64 = st->reg[reg].val64;
> +			if (high32)
> +				FIELD_MODIFY(GENMASK_ULL(63, 32), &tmp64, writeval);
> +			else
> +				FIELD_MODIFY(GENMASK_ULL(31, 0), &tmp64, writeval);
> +
> +			return ad9910_reg64_write(st, reg, tmp64, true);
> +		}
> +		break;
> +	case AD9910_REG_POW:
> +		if (!readval)
> +			return ad9910_reg16_write(st, reg, writeval, true);
> +
> +		ret = ad9910_reg16_read(st, reg, &tmp16);
> +		if (ret < 0)
> +			return ret;
> +		*readval = tmp16;
> +		break;
return 0;
> +	default:
> +		if (!readval)
> +			return ad9910_reg32_write(st, reg, writeval, true);
> +
> +		ret = ad9910_reg32_read(st, reg, &tmp32);
> +		if (ret < 0)
> +			return ret;
> +		*readval = tmp32;
> +		break;
and return 0; here.  No point in break to go to as return and nothing else.

> +	}
> +
> +	return ret;
> +}

> +
> +static int ad9910_cfg_sysclk(struct ad9910_state *st, bool update)
> +{
> +	u32 tmp32, cfr3 = AD9910_CFR3_OPEN_MSK;
> +
> +	cfr3 |= AD9910_CFR3_VCO_SEL_MSK |
> +		FIELD_PREP(AD9910_CFR3_DRV0_MSK, st->data.refclk_out_drv);
> +
> +	if (st->data.pll_enabled) {
> +		tmp32 = st->data.pll_charge_pump_current - AD9910_ICP_MIN_uA;
> +		tmp32 = DIV_ROUND_CLOSEST(tmp32, AD9910_ICP_STEP_uA);
> +		cfr3 |= FIELD_PREP(AD9910_CFR3_ICP_MSK, tmp32) |
> +			AD9910_CFR3_PLL_EN_MSK;
> +	} else {
> +		cfr3 |= AD9910_CFR3_ICP_MSK |

For this, be explicit what value you are setting, probably be defining a max value
that the field can take.  Whilst just setting the mask is the same it doesn't
convey the same meaning to someone reading the code.

> +			AD9910_CFR3_REFCLK_DIV_RESETB_MSK |
> +			AD9910_CFR3_PFD_RESET_MSK;
> +	}
> +	st->reg[AD9910_REG_CFR3].val32 = cfr3;
> +
> +	return ad9910_set_sysclk_freq(st, AD9910_PLL_OUT_MAX_FREQ_HZ, update);
> +}
> +
> +static int ad9910_parse_fw(struct ad9910_state *st)
> +{
> +	struct device *dev = &st->spi->dev;
> +	u32 tmp;
> +	int ret;
> +
> +	st->data.pll_enabled = device_property_read_bool(dev, "adi,pll-enable");
> +	if (st->data.pll_enabled) {
> +		tmp = AD9910_ICP_MAX_uA;

Defaulting to max current seems unusual.  What's the motivation? Normal instinct is
go minimum if no other info.

> +		device_property_read_u32(dev, "adi,charge-pump-current-microamp", &tmp);
> +		if (tmp < AD9910_ICP_MIN_uA || tmp > AD9910_ICP_MAX_uA)
> +			return dev_err_probe(dev, -ERANGE,
> +					     "invalid charge pump current %u\n", tmp);
> +		st->data.pll_charge_pump_current = tmp;
> +
> +		st->data.refclk_out_drv = AD9910_REFCLK_OUT_DRV_DISABLED;
> +		ret = device_property_match_property_string(dev,
> +							    "adi,refclk-out-drive-strength",
> +							    ad9910_refclk_out_drv0,
> +							    ARRAY_SIZE(ad9910_refclk_out_drv0));
> +		if (ret >= 0)
> +			st->data.refclk_out_drv = ret;
> +	}
> +
> +	tmp = AD9910_DAC_IOUT_DEFAULT_uA;
> +	device_property_read_u32(dev, "adi,dac-output-current-microamp", &tmp);
> +	if (tmp < AD9910_DAC_IOUT_MIN_uA || tmp > AD9910_DAC_IOUT_MAX_uA)
> +		return dev_err_probe(dev, -ERANGE,
> +				     "Invalid DAC output current %u uA\n", tmp);
> +	st->data.dac_output_current = tmp;
> +
> +	return 0;
> +}
> +
> +static int ad9910_setup(struct ad9910_state *st, struct reset_control *dev_rst)
> +{
> +	u32 reg32;
> +	int ret;
> +
> +	ret = reset_control_deassert(dev_rst);
> +	if (ret)
> +		return ret;
> +
> +	reg32 = AD9910_CFR1_SDIO_INPUT_ONLY_MSK;
> +	ret = ad9910_reg32_write(st, AD9910_REG_CFR1, reg32, false);
Trivial but I'd not bother using the local variable for simple values.
	ret = ad9910_reg32_write(st, AD9910_REG_CFR1,
				 AD9910_CFR1_SDIO_INPUT_ONLY_MSK, false);
is fine.
> +	if (ret)
> +		return ret;
> +
> +	reg32 = AD9910_CFR2_AMP_SCALE_SINGLE_TONE_MSK;

This split seems odd.  Why not combine above and the next block?

> +	reg32 |= AD9910_CFR2_SYNC_TIMING_VAL_DISABLE_MSK |
> +		 AD9910_CFR2_DRG_NO_DWELL_MSK |
> +		 AD9910_CFR2_DATA_ASM_HOLD_LAST_MSK |
> +		 AD9910_CFR2_SYNC_CLK_EN_MSK |
> +		 AD9910_CFR2_PDCLK_ENABLE_MSK;
> +	ret = ad9910_reg32_write(st, AD9910_REG_CFR2, reg32, false);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad9910_cfg_sysclk(st, false);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad9910_set_dac_current(st, false);
> +	if (ret)
> +		return ret;
> +
> +	return ad9910_io_update(st);
> +}
> +
> +static void ad9910_release(void *data)
> +{
> +	struct ad9910_state *st = data;
> +
> +	if (!ad9910_powerdown_set(st, true))
> +		return;
> +
> +	ad9910_reg32_update(st, AD9910_REG_CFR1,
> +			    AD9910_CFR1_SOFT_POWER_DOWN_MSK,
> +			    AD9910_CFR1_SOFT_POWER_DOWN_MSK,
> +			    true);
> +}
> +
> +static int ad9910_probe(struct spi_device *spi)
> +{
> +	struct reset_control *dev_rst;
> +	struct gpio_desc *io_rst_gpio;
> +	struct device *dev = &spi->dev;
> +	struct iio_dev *indio_dev;
> +	struct ad9910_state *st;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->spi = spi;
> +
> +	st->refclk = devm_clk_get_enabled(dev, "ref_clk");
> +	if (IS_ERR(st->refclk))
> +		return dev_err_probe(dev, PTR_ERR(st->refclk),
> +				     "Failed to get reference clock\n");
> +
> +	ret = devm_regulator_bulk_get_enable(dev,
> +					     ARRAY_SIZE(ad9910_power_supplies),
> +					     ad9910_power_supplies);

Maybe we can just call it ad9910_supplies without loss of meaning and have
slightly shorter lines?  Could also drag that const array into scope of this
function perhaps as we only need it in here.

> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get regulators\n");
> +
> +	ret = devm_mutex_init(dev, &st->lock);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->name = "ad9910";
> +	indio_dev->info = &ad9910_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = ad9910_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(ad9910_channels);
> +
> +	dev_rst = devm_reset_control_get_optional_exclusive(dev, NULL);
> +	if (IS_ERR(dev_rst))
> +		return dev_err_probe(dev, PTR_ERR(dev_rst),
> +				     "failed to get device reset control\n");
> +
> +	ret = reset_control_assert(dev_rst);
Do we need this?  I 'think' that the gpio reset controller will ensure that
the reset line is GPIOD_OUT_HIGH, on registering it.
https://elixir.bootlin.com/linux/v6.19.9/source/drivers/reset/reset-gpio.c#L81
and that should I think correspond to asserted.

I was curious why there were _deasserted() variants of the get but not
_asserted() ones and went looking.  Seems assumption is that in general
should already be in asserted state as firmware deals with that (here the dt
binding / gpio reset driver).



> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to assert device reset control\n");

...

> +	ret = ad9910_parse_fw(st);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad9910_setup(st, dev_rst);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "device setup failed\n");
> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	return devm_add_action_or_reset(dev, ad9910_release, st);

What is this undoing?  Very unusual to have a register write that you
want to happen 'before' the userspace interfaces go away in the remove()
path.  Perhaps it is paired with ad9910_setup()?


> +}

>