Re: [PATCH RFC v2 1/9] dt-bindings: iio: frequency: add ad9910

Re: [PATCH RFC v2 1/9] dt-bindings: iio: frequency: add ad9910

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1320 bytes --]

On Fri, Mar 20, 2026 at 11:21:37AM +0000, Rodrigo Alencar wrote:
> On 26/03/19 05:25PM, Conor Dooley wrote:
> > On Wed, Mar 18, 2026 at 05:56:01PM +0000, Rodrigo Alencar via B4 Relay wrote:
> > > From: Rodrigo Alencar <rodrigo.alencar@analog.com>
> > > 
> > > DT-bindings for AD9910, a 1 GSPS DDS with 14-bit DAC. It includes
> > > configurations for clocks, DAC current, reset and basic GPIO control.
> > > 
> > > Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>
> 
> ...
> 
> > > +
> > > +  clock-names:
> > > +    oneOf:
> > > +      - items:
> > > +          - const: ref_clk
> > 
> > s/_clk//, not like it can be anything else!
> > 
> > > +      - items:
> > > +          - const: ref_clk
> > > +          - const: sync_in
> > > +
> > > +  '#clock-cells':
> > > +    const: 1
> > > +
> > > +  clock-output-names:
> > > +    minItems: 1
> > > +    maxItems: 3
> > > +    items:
> > > +      enum: [ sync_clk, pdclk, sync_out ]
> > 
> > I'd say same here, but then you've got some issues with differentiation,
> > so idk.
> 
> so I've got the names as they are referred in the device pins in the datasheet

Coming back to this one, ye I think it just is less confusing to keep
the _clk ultimately. This looks good then, I think, modulo the RFC-state
of the series.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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@kernel.org> wrote:

> From: Rodrigo Alencar <rodrigo.alencar@analog.com>
> 
> 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@analog.com>
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()?


> +}

> 

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

[Rodrigo Alencar]

On 26/03/22 04:50PM, Jonathan Cameron wrote:
> On Wed, 18 Mar 2026 17:56:02 +0000
> Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@kernel.org> wrote:
> 
> > From: Rodrigo Alencar <rodrigo.alencar@analog.com>
> > 
> > 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@analog.com>

...

> > +#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.

I understood that the usage of devm_add_action_or_reset() would justify
the header.

...

> > +#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.

Next patches will leverage the macro as more ext_info attrs will be introduced.
I suppose we can build the foundation for later extension.
 
> > +}
> > +
> > +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.

Similar, other channels will be introduced here so additions are easier
to review.

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

...

> > +
> > +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.

This is just the default value from the datasheet, ICP should not really matter
when the PLL is disabled, so removing this should be fine.

> 
> > +			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.

ICP_MAX_uA leads to 111 in the CFR3_ICP field, which is the default value when the
device resets or when it powers on. I suppose that if we are not touching that
property, there would be no reason to change that.

...

-- 
Kind regards,

Rodrigo Alencar

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

[Andy Shevchenko]

On Mon, Mar 23, 2026 at 10:34:37AM +0000, Rodrigo Alencar wrote:
> On 26/03/22 04:50PM, Jonathan Cameron wrote:
> > On Wed, 18 Mar 2026 17:56:02 +0000
> > Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@kernel.org> wrote:

...

> > > +#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.
> 
> I understood that the usage of devm_add_action_or_reset() would justify
> the header.

It's in the device/devres.h.

...

> > > +	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.

Agree.

> >  What's the motivation? Normal instinct is go minimum if no other info.
> 
> ICP_MAX_uA leads to 111 in the CFR3_ICP field, which is the default value
> when the device resets or when it powers on. I suppose that if we are not
> touching that property, there would be no reason to change that.

I believe we should think different, id est about potential damages or
current drain. I would expect a minimum or hi-impedance (power off) state
of the related part of the device.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC v2 5/9] iio: frequency: ad9910: add RAM mode support

[Jonathan Cameron]

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

> From: Rodrigo Alencar <rodrigo.alencar@analog.com>
> 
> Add RAM channel with support for profile-based control. This includes:
> - RAM data loading via firmware upload interface;
> - Per-profile RAM configuration (start/end address, step rate, operating
>   mode, dwell control);
> - RAM destination control (frequency, phase, amplitude, polar);
> - RAM operating modes (direct switch, ramp up, bidirectional ramp,
>   continuous bidirectional, continuous recirculate);
> - Profile switching for RAM playback;
> - Sampling frequency control via profile step rate;
> - ram-enable-aware read/write paths that redirect single tone
>   frequency/phase/amplitude access through reg_profile cache when RAM is
>   active;
> 
> When RAM is enabled, the DDS core parameters (frequency, phase, amplitude)
> for the single tone channel are sourced from a shadow register cache
> (reg_profile[]) since the profile registers are repurposed for RAM control.
> 
> Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>
A few minor things. Again I've left discussion of interfaces for docs patches.

> ---
>  drivers/iio/frequency/Kconfig  |   2 +
>  drivers/iio/frequency/ad9910.c | 464 ++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 462 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/frequency/Kconfig b/drivers/iio/frequency/Kconfig
> index 180e74f62d11..a5b2e5cb5269 100644
> --- a/drivers/iio/frequency/Kconfig
> +++ b/drivers/iio/frequency/Kconfig
> @@ -29,6 +29,8 @@ config AD9910
>  	tristate "Analog Devices AD9910 Direct Digital Synthesizer"
>  	depends on SPI
>  	depends on GPIOLIB
> +	select FW_LOADER
> +	select FW_UPLOAD
>  	help
>  	  Say yes here to build support for Analog Devices AD9910
>  	  1 GSPS, 14-Bit DDS with integrated DAC.
> diff --git a/drivers/iio/frequency/ad9910.c b/drivers/iio/frequency/ad9910.c
> index d3367e211dcf..747f4f407536 100644
> --- a/drivers/iio/frequency/ad9910.c
> +++ b/drivers/iio/frequency/ad9910.c

> @@ -1288,12 +1610,26 @@ static int ad9910_write_raw(struct iio_dev *indio_dev,
>  			return ad9910_reg32_update(st, AD9910_REG_DRG_RATE,
>  						   AD9910_DRG_RATE_DEC_MSK,
>  						   tmp32, true);
> +		case AD9910_CHANNEL_RAM:
> +			if (!AD9910_RAM_ENABLED(st)) {
> +				FIELD_MODIFY(AD9910_PROFILE_RAM_STEP_RATE_MSK,
> +					     &st->reg_profile[st->profile], tmp32);
> +				return 0;
> +			}
> +
> +			tmp64 = FIELD_PREP(AD9910_PROFILE_RAM_STEP_RATE_MSK, tmp32);
> +			return ad9910_reg64_update(st, AD9910_REG_PROFILE(st->profile),
> +						   AD9910_PROFILE_RAM_STEP_RATE_MSK,
> +						   tmp64, true);
> +
>  		default:
>  			return -EINVAL;
>  		}
>  	default:
>  		return -EINVAL;
>  	}
> +
> +	return ret;
Seems a bit odd if you can now get here. Probably means some return missing
that would make more sense than a break somewhere above this.
>  }


> +
>  static const struct iio_info ad9910_info = {
>  	.read_raw = ad9910_read_raw,
>  	.write_raw = ad9910_write_raw,
> @@ -1503,6 +1922,13 @@ static int ad9910_setup(struct ad9910_state *st, struct reset_control *dev_rst)
>  	if (ret)
>  		return ret;
>  
> +	for (int i = 0; i < AD9910_NUM_PROFILES; i++) {
> +		st->reg_profile[i] = AD9910_PROFILE_RAM_OPEN_MSK;
Add a definition for maximum value and explicitly write that via FIELD_PREP()
as that will make it easier to see what is going on here.

> +		st->reg_profile[i] |= FIELD_PREP(AD9910_PROFILE_RAM_STEP_RATE_MSK, 1);
> +		st->reg_profile[i] |= FIELD_PREP(AD9910_PROFILE_RAM_END_ADDR_MSK,
> +						 AD9910_RAM_ADDR_MAX);
> +	}

> @@ -1519,6 +1947,24 @@ static void ad9910_release(void *data)
>  			    true);
>  }
>  
> +static inline void ad9910_debugfs_init(struct ad9910_state *st,
> +				       struct iio_dev *indio_dev)
> +{
> +#ifdef CONFIG_DEBUG_FS
Why?  There are stubs for debugfs_create_symlink() and the compiler
should tidyup the rest if that's stubbed out.

Whether this interfaces makes sense is a question I'll leave for ABI docs.

> +	char buf[64];
> +
> +	/*
> +	 * symlinks are created here so iio userspace tools can refer to them
> +	 * as debug attributes.
> +	 */
> +	snprintf(buf, sizeof(buf), "/sys/class/firmware/%s/loading", st->ram_fwu_name);
> +	debugfs_create_symlink("ram_loading", iio_get_debugfs_dentry(indio_dev), buf);
> +
> +	snprintf(buf, sizeof(buf), "/sys/class/firmware/%s/data", st->ram_fwu_name);
> +	debugfs_create_symlink("ram_data", iio_get_debugfs_dentry(indio_dev), buf);
> +#endif
> +}

> 

Re: [PATCH RFC v2 8/9] Documentation: ABI: testing: add docs for ad9910 sysfs entries

[Jonathan Cameron]

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

> From: Rodrigo Alencar <rodrigo.alencar@analog.com>
> 
> Add ABI documentation file for the DDS AD9910 with sysfs entries to
> control Parallel Port, Digital Ramp Generator, RAM and OSK parameters.
> 
> Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>
> ---
Thanks for writing this up.  Let's see how others view it. There is a lot
going on here!  My main comment has been around trying to write the
docs as a generic thing rather than focusing on device details.

Jonathan

>  .../ABI/testing/sysfs-bus-iio-frequency-ad9910     | 182 +++++++++++++++++++++
>  MAINTAINERS                                        |   1 +
>  2 files changed, 183 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-frequency-ad9910 b/Documentation/ABI/testing/sysfs-bus-iio-frequency-ad9910
> new file mode 100644
> index 000000000000..120de494f6b1
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-frequency-ad9910
> @@ -0,0 +1,182 @@
> +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_profile
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Read/write the active profile index [0, 7] from/to the physical
> +		channel. The AD9910 supports 8 profiles, each storing a complete
> +		set of single tone (frequency, phase, amplitude) and RAM playback
> +		parameters.

This one is interesting.  Can we treat them as symbols that we are picking
between?  We have similar DAC ABIs for that already.

Is this picking between them for purposes of configuration or setting which one is
in being output currently?

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_frequency_offset
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Read/write the parallel port frequency offset in Hz. This is the
> +		base frequency tuning word (FTW register) to which the scaled
> +		parallel port data is added during parallel data port modulation.
> +		Valid range is [0, SYSCLK/2).

Ideally think about how these controls generalize (if they do) and avoid
device specific descriptions.

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_frequency_scale
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Read/write the parallel port frequency modulation gain. The value
> +		must be a power of 2 in the [1, 2^15] range. This value scales the
> +		16-bit parallel data port input before adding it to the
> +		frequency_offset value.

Can we provide an _available for this with all 16 values?  Then avoid the specific
device nature of the documentation instead saying see the _available for what is possible.

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_phase_offset
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Read/write the parallel port phase offset in radians. Valid range
> +		is [0, 2*pi/256). This sets the lower 8 bits of the phase offset
> +		word (POW register) used as a base during parallel port polar
> +		modulation.

Given it does full phase shift, I don't think for userspace docs we care about the 8
bits of whatever register.  

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_scale_offset
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Read/write the parallel port amplitude scale offset. Valid range
> +		is [0, 1/256). This sets the lower 6 bits of the amplitude scale
> +		factor (ASF register) used as a base during parallel port polar
> +		modulation.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_destination
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Read/write the digital ramp generator (DRG) or the RAM control
> +		destination parameter. Determines which DDS core parameter is to
> +		be modulated when the child mode channel is enabled.
> +
> +		Available values can be read from the corresponding
> +		out_altvoltageY_destination_available attribute.
> +
> +		Valid values: "polar" (only for RAM control), "frequency", "phase"
> +		and "amplitude"

This is very device specific. Maybe we are better representing these as separate
channels each with their own controls for DRG.  No problem if changing one changes
another.

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_destination_available
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Lists the available destination values for the DRG channel:
> +		"frequency phase amplitude"; or for the RAM control channel:
> +		"frequency phase amplitude polar".
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_operating_mode
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Read/write the DRG or RAM control operating mode. For the DRG
> +		channel it controls the no-dwell behavior of the ramp.
> +
> +		Available values can be read from the corresponding
> +		out_altvoltageY_operating_mode_available attribute.
> +
> +		Valid values for DRG channel:
> +
> +		  - "bidirectional": Normal ramp generation (ramp up then
> +		    down, dwelling at limits).

Some sort of trapezium wave?  Maybe this and continuous forms are combined
and we have a separate dwell time control?

> +		  - "ramp_down": No-dwell low; the ramp resets to upper
> +		    limit upon reaching the lower limit.
> +		  - "ramp_up": No-dwell high; the ramp resets to lower
> +		    limit upon reaching the upper limit.
> +		  - "bidirectional_continuous": Both no-dwell high and low;
> +		    the ramp continuously sweeps without dwelling.

Triangle wave?  bidirectional continuous is a rather confusing term so maybe
we should rethink this one.

> +
> +		Valid values for RAM control channel:
> +
> +		  - "direct_switch": start address defines fixed word to be used
> +		    by the selected profile.
> +		  - "ramp_up": One-shot ramp up through current profile's address
> +		    range.
> +		  - "bidirectional": Ramp up then down through PROFILE0 pin.

Avoid specifics like this.  Can we call external control pin or something like that?

> +		  - "bidirectional_continuous": Continuous ramp up/down
> +		    through current profile's address range.
> +		  - "ramp_up_continuous": Continuous ramp up through
> +		    current profile's address range.

I guess this goes back to start on finishing ramping up?

> +		  - "sequenced": Sequenced playback of RAM profiles up to
> +		    the active profile. Requires active profile > 0.
Is this just running through each profile one after another? (other than profile 0)?
> +		  - "sequenced_continuous": Continuous sequenced playback
> +		    of RAM profiles up to the active profile. Requires
> +		    active profile > 0.
Similar to above, maybe separate out dwell time if that's the difference between
sequenced and sequenced_continuous.

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_operating_mode_available
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		For the DRG channel it lists the available operating mode values:
> +		"bidirectional ramp_down ramp_up bidirectional_continuous".
> +
> +		For the RAM control channel it lists the available operating mode
> +		values:
> +		"direct_switch ramp_up bidirectional bidirectional_continuous
> +		ramp_up_continuous sequenced sequenced_continuous".
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_frequency_step
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Read/write the DRG frequency step size in Hz for ramp up and ramp
> +		down DRG channels. This is the increment/decrement step applied to
> +		the DRG frequency value, which is input to the DDS core and it is
> +		updated at each ramp clock tick when the DRG destination is
> +		set to "frequency". Valid range is [0, sysclk/2).
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_phase_step
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Read/write the DRG phase step size in radians for ramp up and ramp
> +		down DRG channels. This is the increment/decrement step applied to
> +		the DRG phase value, which is input to the DDS core and it is
> +		updated at each ramp clock tick when the DRG destination is
> +		set to "phase". Valid range is [0, 2*pi).
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_scale_step
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		For the DRG ramp up/down channels this is used to read/write the
> +		DRG amplitude step size, which is applied as an
> +		increment/decrement to the DRG amplitude value, input to the DDS
> +		core, updated at each ramp clock tick when the DRG destination is
> +		set to "amplitude". Valid range is [0, 1].
> +
> +		For the OSK channel this is used to read/write the automatic OSK
> +		amplitude ramp step size. Writing a	non-zero value enables
> +		automatic OSK mode and sets the amplitude step size. Writing "0"
> +		disables automatic OSK mode. The value is rounded to the nearest
> +		hardware supported step: 0.000061, 0.000122, 0.000244, or
> +		0.000488.

Those need to come from available attribute. Don't belong in the docs as if we
have them here there is little chance of generalizing later to cover more devices.

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_address_start
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Read/write the RAM start address for the active profile. Defines
> +		the first RAM word address used during playback. Cannot be
> +		changed while RAM mode is enabled. Valid range is [0, 1023].
> +		If set above the current end address, the end address is
> +		automatically adjusted to match.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_address_end
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Read/write the RAM end address for the active profile. Defines
> +		the last RAM word address used during playback. Cannot be
> +		changed while RAM mode is enabled. Valid range is
> +		[address_start, 1023].
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_pinctrl_en
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Read/write the OSK manual external control enable. Writing '1'
> +		enables manual control of the output amplitude envelope via an
> +		external pin. Writing '0' disables it. When enabled, the OSK pin
> +		directly controls the amplitude on/off state rather than using
> +		the automatic OSK ramp.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6403439b530d..edd87ee7da5f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1635,6 +1635,7 @@ M:	Rodrigo Alencar <rodrigo.alencar@analog.com>
>  L:	linux-iio@vger.kernel.org
>  S:	Supported
>  W:	https://ez.analog.com/linux-software-drivers
> +F:	Documentation/ABI/testing/sysfs-bus-iio-frequency-ad9910
>  F:	Documentation/devicetree/bindings/iio/frequency/adi,ad9910.yaml
>  F:	drivers/iio/frequency/ad9910.c
>  
> 

Re: [PATCH RFC v2 8/9] Documentation: ABI: testing: add docs for ad9910 sysfs entries

[Rodrigo Alencar]

On 26/03/22 05:22PM, Jonathan Cameron wrote:
> On Wed, 18 Mar 2026 17:56:08 +0000
> Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@kernel.org> wrote:
> 
> > From: Rodrigo Alencar <rodrigo.alencar@analog.com>
> > 
> > Add ABI documentation file for the DDS AD9910 with sysfs entries to
> > control Parallel Port, Digital Ramp Generator, RAM and OSK parameters.
> > 
> > Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>
> > ---

...

> > +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_profile
> > +KernelVersion:
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Read/write the active profile index [0, 7] from/to the physical
> > +		channel. The AD9910 supports 8 profiles, each storing a complete
> > +		set of single tone (frequency, phase, amplitude) and RAM playback
> > +		parameters.
> 
> This one is interesting.  Can we treat them as symbols that we are picking
> between?  We have similar DAC ABIs for that already.

The profile concept comes from the datasheet and defines sets of configuration
for single tone and RAM control mode. I am not sure how we fit this idea into a
"symbol"

> Is this picking between them for purposes of configuration or setting which one is
> in being output currently?

Well, this is being used for configuration and activating, then, yes, you can only configure
an active profile, but I was not seeing that as an issue. I suppose that simplifies the
ABI a bit.

...

> > +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_destination
> > +KernelVersion:
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Read/write the digital ramp generator (DRG) or the RAM control
> > +		destination parameter. Determines which DDS core parameter is to
> > +		be modulated when the child mode channel is enabled.
> > +
> > +		Available values can be read from the corresponding
> > +		out_altvoltageY_destination_available attribute.
> > +
> > +		Valid values: "polar" (only for RAM control), "frequency", "phase"
> > +		and "amplitude"
> 
> This is very device specific. Maybe we are better representing these as separate
> channels each with their own controls for DRG.  No problem if changing one changes
> another.

You mean removing this generic Y there? Indeed, there are separate configs for each one.

...

> > +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_operating_mode
> > +KernelVersion:
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Read/write the DRG or RAM control operating mode. For the DRG
> > +		channel it controls the no-dwell behavior of the ramp.
> > +
> > +		Available values can be read from the corresponding
> > +		out_altvoltageY_operating_mode_available attribute.
> > +
> > +		Valid values for DRG channel:
> > +
> > +		  - "bidirectional": Normal ramp generation (ramp up then
> > +		    down, dwelling at limits).
> 
> Some sort of trapezium wave?  Maybe this and continuous forms are combined
> and we have a separate dwell time control?

Yes, sort of. Dwell control is made by an external pin (DRCTL), often controlled
by an FPGA to achieve certain required timings. I can say that software control
is not really recommended, unless only a one-shot ramp is necessary.

When adding the IIO backend support, extendend attributes will be added to
support control of dwell times.

> 
> > +		  - "ramp_down": No-dwell low; the ramp resets to upper
> > +		    limit upon reaching the lower limit.
> > +		  - "ramp_up": No-dwell high; the ramp resets to lower
> > +		    limit upon reaching the upper limit.
> > +		  - "bidirectional_continuous": Both no-dwell high and low;
> > +		    the ramp continuously sweeps without dwelling.
> 
> Triangle wave?  bidirectional continuous is a rather confusing term so maybe
> we should rethink this one.

Mostly yes, but not only that. Sawtooth can be achieved as well by changing
the step sizes, also other weird patterns can be achieved by toggling DRCTL pin.
This mode is the most useful when one does not have an FPGA and want to save
resources on controlling the DRCTL pin. That mode name comes from the datasheet,
so I suppose it was fine.

> > +
> > +		Valid values for RAM control channel:
> > +
> > +		  - "direct_switch": start address defines fixed word to be used
> > +		    by the selected profile.
> > +		  - "ramp_up": One-shot ramp up through current profile's address
> > +		    range.
> > +		  - "bidirectional": Ramp up then down through PROFILE0 pin.
> 
> Avoid specifics like this.  Can we call external control pin or something like that?
> 
> > +		  - "bidirectional_continuous": Continuous ramp up/down
> > +		    through current profile's address range.
> > +		  - "ramp_up_continuous": Continuous ramp up through
> > +		    current profile's address range.
> 
> I guess this goes back to start on finishing ramping up?

Yes, any dwell time should be considered when loading the "waveform" into the RAM

> 
> > +		  - "sequenced": Sequenced playback of RAM profiles up to
> > +		    the active profile. Requires active profile > 0.
> Is this just running through each profile one after another? (other than profile 0)?

for this, this would be the actions:
- configure all desired profiles (0 up to X) 
- Set the operating mode to sequenced (profile X would be active at this point)
- Enable RAM mode

When RAM mode is enabled, it would trigger the execution of profiles 0 up to X,
in sequence, according to the configured address range and sample rate for each profile.
When one profile ends the next starts until profile X finishes.

> > +		  - "sequenced_continuous": Continuous sequenced playback
> > +		    of RAM profiles up to the active profile. Requires
> > +		    active profile > 0.
> Similar to above, maybe separate out dwell time if that's the difference between
> sequenced and sequenced_continuous.

The difference here is that when Profile X finishes, Profile 0 starts again.
So the previous one is kind of an one-shot mode of multiple profiles in sequence

...

-- 
Kind regards,

Rodrigo Alencar

Re: [PATCH RFC v2 8/9] Documentation: ABI: testing: add docs for ad9910 sysfs entries

[Jonathan Cameron]

On Mon, 23 Mar 2026 11:36:08 +0000
Rodrigo Alencar <455.rodrigo.alencar@gmail.com> wrote:

> On 26/03/22 05:22PM, Jonathan Cameron wrote:
> > On Wed, 18 Mar 2026 17:56:08 +0000
> > Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@kernel.org> wrote:
> >   
> > > From: Rodrigo Alencar <rodrigo.alencar@analog.com>
> > > 
> > > Add ABI documentation file for the DDS AD9910 with sysfs entries to
> > > control Parallel Port, Digital Ramp Generator, RAM and OSK parameters.
> > > 
> > > Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>
> > > ---  
> 
> ...
> 
> > > +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_profile
> > > +KernelVersion:
> > > +Contact:	linux-iio@vger.kernel.org
> > > +Description:
> > > +		Read/write the active profile index [0, 7] from/to the physical
> > > +		channel. The AD9910 supports 8 profiles, each storing a complete
> > > +		set of single tone (frequency, phase, amplitude) and RAM playback
> > > +		parameters.  
> > 
> > This one is interesting.  Can we treat them as symbols that we are picking
> > between?  We have similar DAC ABIs for that already.  
> 
> The profile concept comes from the datasheet and defines sets of configuration
> for single tone and RAM control mode. I am not sure how we fit this idea into a
> "symbol"

Think of those tones as just different frequencies (the other stuff could be the same)
then this is FSK with 8 symbols.

> 
> > Is this picking between them for purposes of configuration or setting which one is
> > in being output currently?  
> 
> Well, this is being used for configuration and activating, then, yes, you can only configure
> an active profile, but I was not seeing that as an issue. I suppose that simplifies the
> ABI a bit.
If you are only configuring the one that is active, short of a small overhead
of having to configure more than one property why have this at all?

Just leave it in a profile and have userspace reconfigure everything it wants to.

However, I see that in some of them modes below this is more complex
as the profiles are cycled through - for ram playback anyway.
It may make sense to separate the ram case from tone ones.


> 
> ...
> 
> > > +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_destination
> > > +KernelVersion:
> > > +Contact:	linux-iio@vger.kernel.org
> > > +Description:
> > > +		Read/write the digital ramp generator (DRG) or the RAM control
> > > +		destination parameter. Determines which DDS core parameter is to
> > > +		be modulated when the child mode channel is enabled.
> > > +
> > > +		Available values can be read from the corresponding
> > > +		out_altvoltageY_destination_available attribute.
> > > +
> > > +		Valid values: "polar" (only for RAM control), "frequency", "phase"
> > > +		and "amplitude"  
> > 
> > This is very device specific. Maybe we are better representing these as separate
> > channels each with their own controls for DRG.  No problem if changing one changes
> > another.  
> 
> You mean removing this generic Y there? Indeed, there are separate configs for each one.

No, I mean having more channels.  One for each of polar, frequency, phase and frequency for
each channel.  Then enables for which channel is turned on.  Might not work out,
but I'd like you to explore what problems that type of interface would bring.

The aim here is to add as little new ABI as possible as custom ABI is a real
pain for generic userspace.


> 
> ...
> 
> > > +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_operating_mode
> > > +KernelVersion:
> > > +Contact:	linux-iio@vger.kernel.org
> > > +Description:
> > > +		Read/write the DRG or RAM control operating mode. For the DRG
> > > +		channel it controls the no-dwell behavior of the ramp.
> > > +
> > > +		Available values can be read from the corresponding
> > > +		out_altvoltageY_operating_mode_available attribute.
> > > +
> > > +		Valid values for DRG channel:
> > > +
> > > +		  - "bidirectional": Normal ramp generation (ramp up then
> > > +		    down, dwelling at limits).  
> > 
> > Some sort of trapezium wave?  Maybe this and continuous forms are combined
> > and we have a separate dwell time control?  
> 
> Yes, sort of. Dwell control is made by an external pin (DRCTL), often controlled
> by an FPGA to achieve certain required timings. I can say that software control
> is not really recommended, unless only a one-shot ramp is necessary.

So is it worth exposing this software control at all?  Maybe just make it a firmware
description problem as to whether that DRCTL is connected or not.

> 
> When adding the IIO backend support, extendend attributes will be added to
> support control of dwell times.
> 
> >   
> > > +		  - "ramp_down": No-dwell low; the ramp resets to upper
> > > +		    limit upon reaching the lower limit.
> > > +		  - "ramp_up": No-dwell high; the ramp resets to lower
> > > +		    limit upon reaching the upper limit.
> > > +		  - "bidirectional_continuous": Both no-dwell high and low;
> > > +		    the ramp continuously sweeps without dwelling.  
> > 
> > Triangle wave?  bidirectional continuous is a rather confusing term so maybe
> > we should rethink this one.  
> 
> Mostly yes, but not only that. Sawtooth can be achieved as well by changing
> the step sizes, also other weird patterns can be achieved by toggling DRCTL pin.

Sawtooth is kind of a special triangle wave with one very steep side.
Wikipedia even has: "It can also be considered the extreme case of an asymmetric triangle wave"
https://en.wikipedia.org/wiki/Sawtooth_wave

> This mode is the most useful when one does not have an FPGA and want to save
> resources on controlling the DRCTL pin. That mode name comes from the datasheet,
> so I suppose it was fine.

Let us see if we can get more opinions on this.  Whilst I can see the logic of
the datasheet naming, it's a bit obscure.

> 
> > > +
> > > +		Valid values for RAM control channel:
> > > +
> > > +		  - "direct_switch": start address defines fixed word to be used
> > > +		    by the selected profile.
> > > +		  - "ramp_up": One-shot ramp up through current profile's address
> > > +		    range.
> > > +		  - "bidirectional": Ramp up then down through PROFILE0 pin.  
> > 
> > Avoid specifics like this.  Can we call external control pin or something like that?
> >   
> > > +		  - "bidirectional_continuous": Continuous ramp up/down
> > > +		    through current profile's address range.
> > > +		  - "ramp_up_continuous": Continuous ramp up through
> > > +		    current profile's address range.  
> > 
> > I guess this goes back to start on finishing ramping up?  
> 
> Yes, any dwell time should be considered when loading the "waveform" into the RAM
> 
> >   
> > > +		  - "sequenced": Sequenced playback of RAM profiles up to
> > > +		    the active profile. Requires active profile > 0.  
> > Is this just running through each profile one after another? (other than profile 0)?  
> 
> for this, this would be the actions:
> - configure all desired profiles (0 up to X) 
> - Set the operating mode to sequenced (profile X would be active at this point)
> - Enable RAM mode
Ah. This reflects on the profile control above. As I mention there I'm not sure we
shouldn't separate the use of profile for tones (where it is symbol like) to that
for RAM addresses.

For the ram addresses I think I'd expose separate attributes for each (there aren't
that many) rather than a selector + controls.

Another option would be to push the control of this into the firmware files
(some sort of header).  Whether that is sufficient would depend on the usecases
for the device.  It would definitely make for an easier runtime configuration
though!

> 
> When RAM mode is enabled, it would trigger the execution of profiles 0 up to X,
> in sequence, according to the configured address range and sample rate for each profile.
> When one profile ends the next starts until profile X finishes.
> 
> > > +		  - "sequenced_continuous": Continuous sequenced playback
> > > +		    of RAM profiles up to the active profile. Requires
> > > +		    active profile > 0.  
> > Similar to above, maybe separate out dwell time if that's the difference between
> > sequenced and sequenced_continuous.  
> 
> The difference here is that when Profile X finishes, Profile 0 starts again.
> So the previous one is kind of an one-shot mode of multiple profiles in sequence

They had fun designing this didn't they!

Anyhow, I think we'll need to work through a few more versions of this to get
as extensible an interface as possible.

> 
> ...
> 

Re: [PATCH RFC v2 8/9] Documentation: ABI: testing: add docs for ad9910 sysfs entries

[David Lechner]

On 4/12/26 9:51 AM, Jonathan Cameron wrote:
> On Mon, 23 Mar 2026 11:36:08 +0000
> Rodrigo Alencar <455.rodrigo.alencar@gmail.com> wrote:
> 
>> On 26/03/22 05:22PM, Jonathan Cameron wrote:
>>> On Wed, 18 Mar 2026 17:56:08 +0000
>>> Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@kernel.org> wrote:
>>>   
>>>> From: Rodrigo Alencar <rodrigo.alencar@analog.com>
>>>>
>>>> Add ABI documentation file for the DDS AD9910 with sysfs entries to
>>>> control Parallel Port, Digital Ramp Generator, RAM and OSK parameters.
>>>>
>>>> Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>
>>>> ---  
>>
...

>>>   
>>>> +		  - "ramp_down": No-dwell low; the ramp resets to upper
>>>> +		    limit upon reaching the lower limit.
>>>> +		  - "ramp_up": No-dwell high; the ramp resets to lower
>>>> +		    limit upon reaching the upper limit.
>>>> +		  - "bidirectional_continuous": Both no-dwell high and low;
>>>> +		    the ramp continuously sweeps without dwelling.  
>>>
>>> Triangle wave?  bidirectional continuous is a rather confusing term so maybe
>>> we should rethink this one.  
>>
>> Mostly yes, but not only that. Sawtooth can be achieved as well by changing
>> the step sizes, also other weird patterns can be achieved by toggling DRCTL pin.
> 
> Sawtooth is kind of a special triangle wave with one very steep side.
> Wikipedia even has: "It can also be considered the extreme case of an asymmetric triangle wave"
> https://en.wikipedia.org/wiki/Sawtooth_wave
> 
>> This mode is the most useful when one does not have an FPGA and want to save
>> resources on controlling the DRCTL pin. That mode name comes from the datasheet,
>> so I suppose it was fine.
> 
> Let us see if we can get more opinions on this.  Whilst I can see the logic of
> the datasheet naming, it's a bit obscure.
> 
It is the same as ramp_up and ramp_down other than what happens when it hits
the limit? If so, I would call it ramp_up_down.

Re: [PATCH RFC v2 9/9] docs: iio: add documentation for ad9910 driver

[Jonathan Cameron]

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

> From: Rodrigo Alencar <rodrigo.alencar@analog.com>
> 
> Add documentation for the AD9910 DDS IIO driver, which describes channels,
> DDS modes, attributes and ABI usage examples.
> 
> Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>

A few things inline. I've not cropped as there is a lot here and I'd like
it all to remain visible in the reply.


Overall this is a very interesting device so whilst I think we are making
progress it might still take a while to come to an overall conclusion
on the ABI!

> ---
>  Documentation/iio/ad9910.rst | 654 +++++++++++++++++++++++++++++++++++++++++++
>  Documentation/iio/index.rst  |   1 +
>  MAINTAINERS                  |   1 +
>  3 files changed, 656 insertions(+)
> 
> diff --git a/Documentation/iio/ad9910.rst b/Documentation/iio/ad9910.rst
> new file mode 100644
> index 000000000000..116f6af4bc2e
> --- /dev/null
> +++ b/Documentation/iio/ad9910.rst
> @@ -0,0 +1,654 @@
> +.. SPDX-License-Identifier: GPL-2.0-only
> +
> +=============
> +AD9910 driver
> +=============
> +
> +DDS (Direct Digital Synthesizer) driver for the Analog Devices Inc. AD9910.
> +The module name is ``ad9910``.
> +
> +* `AD9910 <https://www.analog.com/en/products/ad9910.html>`_
> +
> +The AD9910 is a 1 GSPS DDS with a 14-bit DAC, driven over SPI. The driver
> +exposes the device through the IIO ``altvoltage`` channel type and supports
> +five DDS operating modes: single tone, parallel port modulation, digital ramp
> +generation (DRG), RAM playback and output shift keying (OSK). The device has
> +8 hardware profiles, each capable of storing independent single tone and RAM
> +playback parameters.
> +
> +
> +Channel hierarchy
> +=================
> +
> +The driver exposes the following IIO output channels, each identified by a
> +unique channel number and a human-readable label:
> +
> +* ``out_altvoltage100``: ``phy``: Physical output: system clock and profile control
> +
> +  * ``out_altvoltage110``: ``single_tone``: Single tone mode: per-profile
> +    frequency, phase, amplitude
> +
> +  * ``out_altvoltage120``: ``parallel_port``: Parallel port modulation: enable
> +    and offset/scale parameters
> +
> +  * ``out_altvoltage130``: ``digital_ramp_generator``: DRG control: enable,
> +    destination, operating mode
> +
> +    * ``out_altvoltage131``: ``digital_ramp_up``: DRG ramp-up parameters:
> +      limits, step sizes, ramp rate
> +    * ``out_altvoltage132``: ``digital_ramp_down``: DRG ramp-down parameters:
> +      limits, step sizes, ramp rate
> +
> +  * ``out_altvoltage140``: ``ram_control``: RAM playback: enable, destination,
> +    operating mode, address range
> +
> +  * ``out_altvoltage150``: ``output_shift_keying``: OSK: enable, amplitude
> +    scale, ramp rate, auto/manual control
> +
> +The ``phy`` channel is the root of the hierarchy. Changing its
> +``sampling_frequency`` reconfigures the system clock (SYSCLK) which affects all
> +other channels. The ``profile`` attribute on this channel selects the active
> +hardware profile (0-7) used by the single tone and RAM channels.
I asked out this profile thing in one of the other patches.  Key to me is
that how we write non active profiles?  The most similar thing we've seen
in the past has been setting other frequencies for FSK or phases for PSK or
more mundane DC DAC output that are symbol based. (often an external signal)

For those we have added an additional index so we can see which symbol we
are changing parameters for.  Here it might need to be done in the channel
numbering. I'm not sure.

> +
> +All mode-specific channels (parallel port, DRG, RAM, OSK) have an ``enable``
> +attribute. The DRG and RAM channels additionally have ``destination`` and
> +``operating_mode`` attributes that configure which DDS core parameter is
> +modulated and how.

I wonder if we flatten things out and have separate channels for each type
of modulation. Might lead to a more standard looking interfaces. We don't really
have a standard path to control one type of thing feeding another, whereas
we do have simple 'enable' interfaces.

> +
> +DDS modes
> +=========
> +
> +The AD9910 supports multiple modes of operation that can be configured
> +independently or in combination. Such modes and their corresponding IIO channels
> +are described in this section. The following tables are extracted from the
> +AD9910 datasheet and summarizes the control parameters for each mode and their
> +priority when multiple sources are enabled simultaneously:
> +
> +.. flat-table:: DDS Frequency Control
> +   :header-rows: 1
> +
> +   * - Priority
> +     - Data Source
> +     - Conditions
> +
> +   * - Highest Priority
> +     - RAM
> +     - RAM enabled and data destination is frequency
> +
> +   * -
> +     - DRG
> +     - DRG enabled and data destination is frequency
> +
> +   * -
> +     - Parallel data and FTW (frequency_offset)
> +     - Parallel data port enabled and data destination is frequency
> +
> +   * -
> +     - FTW (frequency)
> +     - RAM enabled and data destination is not frequency
> +
> +   * -
> +     - FTW (frequency) in single tone channel for the active profile
> +     - DRG enabled and data destination is not frequency
> +
> +   * -
> +     - FTW (frequency) in single tone channel for the active profile
> +     - Parallel data port enabled and data destination is not frequency
> +
> +   * - Lowest Priority
> +     - FTW (frequency) in single tone channel for the active profile
> +     - None
> +
> +.. flat-table:: DDS Phase Control
> +   :header-rows: 1
> +
> +   * - Priority
> +     - Data Source
> +     - Conditions
> +
> +   * - Highest Priority
> +     - RAM
> +     - RAM enabled and data destination is phase or polar
> +
> +   * -
> +     - DRG
> +     - DRG enabled and data destination is phase
> +
> +   * -
> +     - Parallel data port
> +     - Parallel data port enabled and data destination is phase
> +
> +   * -
> +     - Parallel data port and POW register LSBs (phase_offset)
> +     - Parallel data port enabled and data destination is polar
> +
> +   * -
> +     - POW (phase)
> +     - RAM enabled and destination is not phase nor polar
> +
> +   * -
> +     - POW (phase) in single tone channel for the active profile
> +     - DRG enabled and data destination is not phase
> +
> +   * -
> +     - POW (phase) in single tone channel for the active profile
> +     - Parallel data port enabled and data destination is not phase nor polar
> +
> +   * - Lowest Priority
> +     - POW (phase) in single tone channel for the active profile
> +     - None
> +
> +.. flat-table:: DDS Amplitude Control
> +   :header-rows: 1
> +
> +   * - Priority
> +     - Data Source
> +     - Conditions
> +
> +   * - Highest Priority
> +     - OSK generator
> +     - OSK enabled (auto mode)
> +
> +   * -
> +     - ASF register
> +     - OSK enabled (manual mode)
> +
> +   * -
> +     - RAM
> +     - RAM enabled and data destination is amplitude or polar
> +
> +   * -
> +     - DRG
> +     - DRG enabled and data destination is amplitude
> +
> +   * -
> +     - Parallel data port
> +     - Parallel data port enabled and data destination is amplitude
> +
> +   * -
> +     - Parallel data port and ASF register LSBs (scale_offset)
> +     - Parallel data port enabled and data destination is polar
> +
> +   * - Lowest Priority
> +     - ASF (scale) in single tone channel for the active profile
> +     - (Amplitude scale is already enabled by default)
> +
> +Single tone mode
> +----------------
> +
> +Single tone is the baseline operating mode. The ``single_tone`` channel
> +provides per-profile frequency, phase and amplitude control:
> +
> +.. flat-table::
> +   :header-rows: 1
> +
> +   * - Attribute
> +     - Unit
> +     - Description
> +
> +   * - ``out_altvoltage110_frequency``
> +     - Hz
> +     - Output frequency. Range [0, SYSCLK/2). Stored in the active profile's
> +       frequency tuning word (FTW).
> +
> +   * - ``out_altvoltage110_phase``
> +     - rad
> +     - Phase offset. Range [0, 2*pi). Stored in the active profile's phase
> +       offset word (POW).
> +
> +   * - ``out_altvoltage110_scale``
> +     - fractional
> +     - Amplitude scale factor. Range [0, 1]. Stored in the active profile's
> +       amplitude scale factor (ASF).
> +
> +When RAM mode is enabled, single tone parameters are stored in a shadow
> +register and are not written to hardware until RAM mode is disabled.
> +
> +Usage examples
> +^^^^^^^^^^^^^^
> +
> +Set the active profile to 2 and configure a 100 MHz tone:
> +
> +.. code-block:: bash
> +
> +	echo 2 > /sys/bus/iio/devices/iio:device0/out_altvoltage100_profile
> +	echo 100000000 > /sys/bus/iio/devices/iio:device0/out_altvoltage110_frequency
> +	echo 0.5 > /sys/bus/iio/devices/iio:device0/out_altvoltage110_scale
> +	echo 0 > /sys/bus/iio/devices/iio:device0/out_altvoltage110_phase
> +
> +Read back the current single tone frequency:
> +
> +.. code-block:: bash
> +
> +	cat /sys/bus/iio/devices/iio:device0/out_altvoltage110_frequency
> +
> +Parallel port mode
> +------------------
> +
> +When enabled, the parallel port allows real-time modulation of DDS parameters
> +through a 16-bit external data bus.
> +
> +.. flat-table::
> +   :header-rows: 1
> +
> +   * - Attribute
> +     - Unit
> +     - Description
> +
> +   * - ``out_altvoltage120_en``
> +     - boolean
> +     - Enable/disable the parallel data port.
> +
> +   * - ``out_altvoltage120_frequency_scale``
> +     - power-of-2
> +     - FM gain multiplier applied to 16-bit parallel input. Range [1, 32768],
> +       must be a power of 2.
> +
> +   * - ``out_altvoltage120_frequency_offset``
> +     - Hz
> +     - Base FTW to which scaled parallel data is added. Range [0, SYSCLK/2).
> +
> +   * - ``out_altvoltage120_phase_offset``
> +     - rad
> +     - Base phase for polar modulation. Lower 8 bits of POW register.
> +       Range [0, 2*pi/256).
> +
> +   * - ``out_altvoltage120_scale_offset``
> +     - fractional
> +     - Base amplitude for polar modulation. Lower 6 bits of ASF register.
> +       Range [0, 1/256).
> +
> +Usage examples
> +^^^^^^^^^^^^^^
> +
> +Enable parallel port with a frequency scale of 16 and a 50 MHz offset:
> +
> +.. code-block:: bash
> +
> +	echo 1 > /sys/bus/iio/devices/iio:device0/out_altvoltage120_en
> +	echo 16 > /sys/bus/iio/devices/iio:device0/out_altvoltage120_frequency_scale
> +	echo 50000000 > /sys/bus/iio/devices/iio:device0/out_altvoltage120_frequency_offset
> +
> +Digital ramp generator (DRG)
> +----------------------------
> +
> +The DRG produces linear frequency, phase or amplitude sweeps using dedicated
> +hardware. It is controlled through three channels: a parent control channel
> +(``digital_ramp_generator``) and two child ramp channels
> +(``digital_ramp_up``, ``digital_ramp_down``).
> +
> +Control channel attributes
> +^^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +.. flat-table::
> +   :header-rows: 1
> +
> +   * - Attribute
> +     - Unit
> +     - Description
> +
> +   * - ``out_altvoltage130_en``
> +     - boolean
> +     - Enable/disable the DRG.
> +
> +   * - ``out_altvoltage130_destination``
> +     - enum
> +     - Which DDS parameter is swept: ``frequency``, ``phase`` or
> +       ``amplitude``.
> +
> +   * - ``out_altvoltage130_destination_available``
> +     - string
> +     - Lists available destination values.
> +
> +   * - ``out_altvoltage130_operating_mode``
> +     - enum
> +     - Ramp behavior (see table below).
> +
> +   * - ``out_altvoltage130_operating_mode_available``
> +     - string
> +     - Lists available operating mode values.
> +
> +DRG operating modes:
> +
> +.. flat-table::
> +   :header-rows: 1
> +
> +   * - Mode
> +     - Description
> +
> +   * - ``bidirectional``
> +     - Ramp up then down, dwelling at limits.
> +
> +   * - ``ramp_down``
> +     - No-dwell low; resets to upper limit at lower limit.
> +
> +   * - ``ramp_up``
> +     - No-dwell high; resets to lower limit at upper limit.
> +
> +   * - ``bidirectional_continuous``
> +     - Continuous sweep without dwelling at either limit.
> +
> +Ramp channel attributes
> +^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +The ``digital_ramp_up`` (channel 131) and ``digital_ramp_down`` (channel 132)
> +channels share the same attribute set but configure ascending and descending
> +ramp parameters independently:
> +
> +.. flat-table::
> +   :header-rows: 1
> +
> +   * - Attribute
> +     - Unit
> +     - Description
> +
> +   * - ``frequency``
> +     - Hz
> +     - Ramp limit when destination is ``frequency``. Range [0, SYSCLK/2).
> +
> +   * - ``phase``
> +     - rad
> +     - Ramp limit when destination is ``phase``. Range [0, 2*pi).
> +
> +   * - ``scale``
> +     - fractional
> +     - Ramp limit when destination is ``amplitude``. Range [0, 1).
> +
> +   * - ``sampling_frequency``
> +     - Hz
> +     - Ramp clock rate: SYSCLK / (4 * divider).
> +
> +   * - ``frequency_step``
> +     - Hz
> +     - Per-tick frequency increment/decrement when destination is
> +       ``frequency``.
> +
> +   * - ``phase_step``
> +     - rad
> +     - Per-tick phase increment/decrement when destination is ``phase``.
> +
> +   * - ``scale_step``
> +     - fractional
> +     - Per-tick amplitude increment/decrement when destination is
> +       ``amplitude``. Range [0, 1).
> +
> +Usage examples
> +^^^^^^^^^^^^^^
> +
> +Configure a frequency sweep from 10 MHz to 100 MHz at a 1 MHz step:
> +
> +.. code-block:: bash
> +
> +	# Set DRG destination to frequency
> +	echo frequency > /sys/bus/iio/devices/iio:device0/out_altvoltage130_destination
> +
> +	# Set operating mode
> +	echo bidirectional_continuous > /sys/bus/iio/devices/iio:device0/out_altvoltage130_operating_mode
> +
> +	# Set ramp limits
> +	echo 60000000 > /sys/bus/iio/devices/iio:device0/out_altvoltage131_frequency
> +	echo 40000000 > /sys/bus/iio/devices/iio:device0/out_altvoltage132_frequency
> +
> +	# Set ramp step size to 1 MHz
> +	echo 1000000 > /sys/bus/iio/devices/iio:device0/out_altvoltage131_frequency_step
> +	echo 1000000 > /sys/bus/iio/devices/iio:device0/out_altvoltage132_frequency_step
> +
> +	# Set ramp clock rate
> +	echo 50000000 > /sys/bus/iio/devices/iio:device0/out_altvoltage131_sampling_frequency
> +
> +	# Enable the DRG
> +	echo 1 > /sys/bus/iio/devices/iio:device0/out_altvoltage130_en
> +
> +Read the current DRG operating mode:
> +
> +.. code-block:: bash
> +
> +	cat /sys/bus/iio/devices/iio:device0/out_altvoltage130_operating_mode
> +
> +RAM mode
> +--------
> +
> +The AD9910 contains a 1024 x 32-bit RAM that can be loaded with waveform data
> +and played back to modulate frequency, phase, amplitude, or polar (phase +
> +amplitude) parameters.
> +
> +RAM control channel attributes
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +.. flat-table::
> +   :header-rows: 1
> +
> +   * - Attribute
> +     - Unit
> +     - Description
> +
> +   * - ``out_altvoltage140_en``
> +     - boolean
> +     - Enable/disable RAM playback. Toggling swaps profile registers between
> +       single tone and RAM configurations across all 8 profiles.
> +
> +   * - ``out_altvoltage140_destination``
> +     - enum
> +     - RAM data target: ``frequency``, ``phase``, ``amplitude`` or ``polar``.
> +       Cannot be changed while RAM mode is enabled.
> +
> +   * - ``out_altvoltage140_destination_available``
> +     - string
> +     - Lists available destination values.
> +
> +   * - ``out_altvoltage140_operating_mode``
> +     - enum
> +     - Playback behavior (see table below).
> +
> +   * - ``out_altvoltage140_operating_mode_available``
> +     - string
> +     - Lists available operating mode values.
> +
> +   * - ``out_altvoltage140_frequency``
> +     - Hz
> +     - Frequency tuning word used as the single tone frequency when
> +       RAM destination is not ``frequency``. Range [0, SYSCLK/2).
> +
> +   * - ``out_altvoltage140_phase``
> +     - rad
> +     - Phase offset word used as the single tone phase when RAM destination
> +       is not ``phase``. Range [0, 2*pi).
> +
> +   * - ``out_altvoltage140_sampling_frequency``
> +     - Hz
> +     - RAM playback step rate controlling how fast the address counter
> +       advances: SYSCLK / (4 * step_rate). Stored per-profile.
> +
> +   * - ``out_altvoltage140_address_start``
Do we need this flexibility to set the start?  We needed a length, but
if we want different effective start can just load a different image.
> +     - integer
> +     - Start address for the active profile. Range [0, 1023]. Cannot be
> +       changed while RAM mode is enabled. If set above current end address,
> +       end address is automatically adjusted.
> +
> +   * - ``out_altvoltage140_address_end``
> +     - integer
> +     - End address for the active profile. Range [address_start, 1023].
> +       Cannot be changed while RAM mode is enabled.
> +
> +RAM operating modes:
> +
> +.. flat-table::
> +   :header-rows: 1
> +
> +   * - Mode
> +     - Description
> +
> +   * - ``direct_switch``
> +     - Start address defines a fixed word used by the selected profile.
> +
> +   * - ``ramp_up``
> +     - One-shot ramp through the current profile's address range.
> +
> +   * - ``bidirectional``
> +     - Ramp up then down through profile 0's address range.
> +
> +   * - ``bidirectional_continuous``
> +     - Continuous ramp up/down through current profile's address range.
> +
> +   * - ``ramp_up_continuous``
> +     - Continuous ramp up through current profile's address range.
> +
> +   * - ``sequenced``
> +     - Sequential playback from profile 0 to the active profile.
> +       Requires active profile > 0.
> +
> +   * - ``sequenced_continuous``
> +     - Continuous sequential playback. Requires active profile > 0.
> +
> +Loading RAM data
> +^^^^^^^^^^^^^^^^
> +
> +RAM data is loaded through the firmware upload framework. The driver registers
> +a firmware upload device named ``iio_deviceX:ram``. Data must be a multiple of
> +4 bytes (32-bit words) and at most 4096 bytes (1024 words).
> +
> +Usage examples
> +^^^^^^^^^^^^^^
> +
> +Configure RAM mode with frequency destination and load a waveform:
> +
> +.. code-block:: bash
> +
> +	# Set RAM address range for profile 0
> +	echo 0 > /sys/bus/iio/devices/iio:device0/out_altvoltage140_address_start
> +	echo 999 > /sys/bus/iio/devices/iio:device0/out_altvoltage140_address_end
> +
> +	# Set destination and operating mode
> +	echo frequency > /sys/bus/iio/devices/iio:device0/out_altvoltage140_destination
> +	echo ramp_up_continuous > /sys/bus/iio/devices/iio:device0/out_altvoltage140_operating_mode
> +
> +	# Set playback rate
> +	echo 250000 > /sys/bus/iio/devices/iio:device0/out_altvoltage140_sampling_frequency
> +
> +	# Load RAM data via firmware upload
> +	echo 1 > /sys/class/firmware/iio\:device0\:ram/loading
> +	cat waveform.bin > /sys/class/firmware/iio\:device0\:ram/data
> +	echo 0 > /sys/class/firmware/iio\:device0\:ram/loading
> +
> +	# Enable RAM mode
> +	echo 1 > /sys/bus/iio/devices/iio:device0/out_altvoltage140_en
> +
> +Read the current RAM operating mode:
> +
> +.. code-block:: bash
> +
> +	cat /sys/bus/iio/devices/iio:device0/out_altvoltage140_operating_mode
> +
> +Output shift keying (OSK)
> +-------------------------
> +
> +OSK controls the output amplitude envelope, allowing the output to be ramped
> +on/off rather than switched abruptly.
> +
> +.. flat-table::
> +   :header-rows: 1
> +
> +   * - Attribute
> +     - Unit
> +     - Description
> +
> +   * - ``out_altvoltage150_en``
> +     - boolean
> +     - Enable/disable OSK.
> +
> +   * - ``out_altvoltage150_scale``
> +     - fractional
> +     - Target amplitude for the OSK ramp. 14-bit ASF field. Range [0, 1).
> +
> +   * - ``out_altvoltage150_sampling_frequency``
> +     - Hz
> +     - OSK ramp rate: SYSCLK / (4 * divider).
> +
> +   * - ``out_altvoltage150_pinctrl_en``
> +     - boolean
> +     - Enable manual external pin control. When enabled, the OSK pin directly
> +       gates the output on/off instead of using the automatic ramp.
> +
> +   * - ``out_altvoltage150_scale_step``
> +     - fractional
> +     - Automatic OSK amplitude step. Writing non-zero enables automatic OSK
> +       and sets the per-tick increment. Writing ``0`` disables it. Rounded to
> +       nearest hardware step: 0.000061, 0.000122, 0.000244 or 0.000488.
> +
> +Usage examples
> +^^^^^^^^^^^^^^
> +
> +Enable OSK with automatic ramping:
> +
> +.. code-block:: bash
> +
> +	# Set ramp rate
> +	echo 1000000 > /sys/bus/iio/devices/iio:device0/out_altvoltage150_sampling_frequency
> +
> +	# Enable automatic OSK with step size
> +	echo 0.000244 > /sys/bus/iio/devices/iio:device0/out_altvoltage150_scale_step
> +
> +	# Enable OSK
> +	echo 1 > /sys/bus/iio/devices/iio:device0/out_altvoltage150_en
> +
> +Enable manual pin-controlled OSK:
> +
> +.. code-block:: bash
> +
> +	# Set target amplitude to full scale
> +	echo 1.0 > /sys/bus/iio/devices/iio:device0/out_altvoltage150_scale
> +
> +	# Enable manual pin control
> +	echo 1 > /sys/bus/iio/devices/iio:device0/out_altvoltage150_pinctrl_en
> +	echo 1 > /sys/bus/iio/devices/iio:device0/out_altvoltage150_en
> +
> +
> +Physical channel
> +================
> +
> +The ``phy`` channel provides device-level control:
> +
> +.. flat-table::
> +   :header-rows: 1
> +
> +   * - Attribute
> +     - Unit
> +     - Description
> +
> +   * - ``out_altvoltage100_sampling_frequency``
> +     - Hz
> +     - System clock (SYSCLK) frequency. With PLL enabled, configures the PLL
> +       multiplier (range 420-1000 MHz). Without PLL, ref clock can only be
> +       divided by 2.
> +
> +   * - ``out_altvoltage100_profile``
> +     - integer
> +     - Active profile index [0, 7]. Selected via GPIO pins. Each profile
> +       stores an independent set of single tone and RAM playback parameters.
> +
> +   * - ``out_altvoltage100_powerdown``
> +     - boolean
> +     - Software power-down. Writing 1 powers down the digital core, DAC,
> +       reference clock input and auxiliary DAC simultaneously.
> +
> +Usage examples
> +--------------
> +
> +Set the system clock to 1 GHz and select profile 3:
> +
> +.. code-block:: bash
> +
> +	echo 1000000000 > /sys/bus/iio/devices/iio:device0/out_altvoltage100_sampling_frequency
> +	echo 3 > /sys/bus/iio/devices/iio:device0/out_altvoltage100_profile
> +
> +Read current system clock frequency:
> +
> +.. code-block:: bash
> +
> +	cat /sys/bus/iio/devices/iio:device0/out_altvoltage100_sampling_frequency
> +
> +Power down the device:
> +
> +.. code-block:: bash
> +
> +	echo 1 > /sys/bus/iio/devices/iio:device0/out_altvoltage100_powerdown
> diff --git a/Documentation/iio/index.rst b/Documentation/iio/index.rst
> index ba3e609c6a13..55cb1ce84ba8 100644
> --- a/Documentation/iio/index.rst
> +++ b/Documentation/iio/index.rst
> @@ -29,6 +29,7 @@ Industrial I/O Kernel Drivers
>     ad7606
>     ad7625
>     ad7944
> +   ad9910
>     ade9000
>     adis16475
>     adis16480
> diff --git a/MAINTAINERS b/MAINTAINERS
> index edd87ee7da5f..14e4272357ce 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1637,6 +1637,7 @@ S:	Supported
>  W:	https://ez.analog.com/linux-software-drivers
>  F:	Documentation/ABI/testing/sysfs-bus-iio-frequency-ad9910
>  F:	Documentation/devicetree/bindings/iio/frequency/adi,ad9910.yaml
> +F:	Documentation/iio/ad9910.rst
>  F:	drivers/iio/frequency/ad9910.c
>  
>  ANALOG DEVICES INC MAX22007 DRIVER
> 

Re: [PATCH RFC v2 9/9] docs: iio: add documentation for ad9910 driver

[Rodrigo Alencar]

On 26/03/22 05:34PM, Jonathan Cameron wrote:
> On Wed, 18 Mar 2026 17:56:09 +0000
> Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@kernel.org> wrote:
> 
> > From: Rodrigo Alencar <rodrigo.alencar@analog.com>
> > 
> > Add documentation for the AD9910 DDS IIO driver, which describes channels,
> > DDS modes, attributes and ABI usage examples.
> > 
> > Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>

...

> > +Channel hierarchy
> > +=================
> > +
> > +The driver exposes the following IIO output channels, each identified by a
> > +unique channel number and a human-readable label:
> > +
> > +* ``out_altvoltage100``: ``phy``: Physical output: system clock and profile control
> > +
> > +  * ``out_altvoltage110``: ``single_tone``: Single tone mode: per-profile
> > +    frequency, phase, amplitude
> > +
> > +  * ``out_altvoltage120``: ``parallel_port``: Parallel port modulation: enable
> > +    and offset/scale parameters
> > +
> > +  * ``out_altvoltage130``: ``digital_ramp_generator``: DRG control: enable,
> > +    destination, operating mode
> > +
> > +    * ``out_altvoltage131``: ``digital_ramp_up``: DRG ramp-up parameters:
> > +      limits, step sizes, ramp rate
> > +    * ``out_altvoltage132``: ``digital_ramp_down``: DRG ramp-down parameters:
> > +      limits, step sizes, ramp rate
> > +
> > +  * ``out_altvoltage140``: ``ram_control``: RAM playback: enable, destination,
> > +    operating mode, address range
> > +
> > +  * ``out_altvoltage150``: ``output_shift_keying``: OSK: enable, amplitude
> > +    scale, ramp rate, auto/manual control
> > +
> > +The ``phy`` channel is the root of the hierarchy. Changing its
> > +``sampling_frequency`` reconfigures the system clock (SYSCLK) which affects all
> > +other channels. The ``profile`` attribute on this channel selects the active
> > +hardware profile (0-7) used by the single tone and RAM channels.
> I asked out this profile thing in one of the other patches.  Key to me is
> that how we write non active profiles?  The most similar thing we've seen
> in the past has been setting other frequencies for FSK or phases for PSK or
> more mundane DC DAC output that are symbol based. (often an external signal)

Yes, not allowing to configure a non-active profile at this point.
Initially was not seeing this as a problem, but would you think different
channels for each profiles should be created? e.g.:

- out_altvoltage111 ... out_altvoltage118 for single tone profiles; and
- out_altvoltage141 .. out_altvoltage148 for RAM profiles

That would not remove the need for something like the profile attribute.

> For those we have added an additional index so we can see which symbol we
> are changing parameters for.  Here it might need to be done in the channel
> numbering. I'm not sure.
> 
> > +
> > +All mode-specific channels (parallel port, DRG, RAM, OSK) have an ``enable``
> > +attribute. The DRG and RAM channels additionally have ``destination`` and
> > +``operating_mode`` attributes that configure which DDS core parameter is
> > +modulated and how.
> 
> I wonder if we flatten things out and have separate channels for each type
> of modulation. Might lead to a more standard looking interfaces. We don't really
> have a standard path to control one type of thing feeding another, whereas
> we do have simple 'enable' interfaces.

...

> > +RAM mode
> > +--------
> > +
> > +The AD9910 contains a 1024 x 32-bit RAM that can be loaded with waveform data
> > +and played back to modulate frequency, phase, amplitude, or polar (phase +
> > +amplitude) parameters.
> > +
> > +RAM control channel attributes
> > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > +
> > +.. flat-table::
> > +   :header-rows: 1
> > +
> > +   * - Attribute
> > +     - Unit
> > +     - Description
> > +
> > +   * - ``out_altvoltage140_en``
> > +     - boolean
> > +     - Enable/disable RAM playback. Toggling swaps profile registers between
> > +       single tone and RAM configurations across all 8 profiles.

...

> > +   * - ``out_altvoltage140_address_start``
> Do we need this flexibility to set the start?  We needed a length, but
> if we want different effective start can just load a different image.

There is one image being loaded into the entire RAM and each profile may choose
from wich sample it starts and ends its address ramp.
The profiles can have those address ranges to overlap or not, then I suppose
considering different images would just complicate things.

> > +     - integer
> > +     - Start address for the active profile. Range [0, 1023]. Cannot be
> > +       changed while RAM mode is enabled. If set above current end address,
> > +       end address is automatically adjusted.
> > +

...

-- 
Kind regards,

Rodrigo Alencar

Re: [PATCH RFC v2 9/9] docs: iio: add documentation for ad9910 driver

[Jonathan Cameron]

On Mon, 23 Mar 2026 11:58:53 +0000
Rodrigo Alencar <455.rodrigo.alencar@gmail.com> wrote:

> On 26/03/22 05:34PM, Jonathan Cameron wrote:
> > On Wed, 18 Mar 2026 17:56:09 +0000
> > Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@kernel.org> wrote:
> >   
> > > From: Rodrigo Alencar <rodrigo.alencar@analog.com>
> > > 
> > > Add documentation for the AD9910 DDS IIO driver, which describes channels,
> > > DDS modes, attributes and ABI usage examples.
> > > 
> > > Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>  
> 
> ...
> 
> > > +Channel hierarchy
> > > +=================
> > > +
> > > +The driver exposes the following IIO output channels, each identified by a
> > > +unique channel number and a human-readable label:
> > > +
> > > +* ``out_altvoltage100``: ``phy``: Physical output: system clock and profile control
> > > +
> > > +  * ``out_altvoltage110``: ``single_tone``: Single tone mode: per-profile
> > > +    frequency, phase, amplitude
> > > +
> > > +  * ``out_altvoltage120``: ``parallel_port``: Parallel port modulation: enable
> > > +    and offset/scale parameters
> > > +
> > > +  * ``out_altvoltage130``: ``digital_ramp_generator``: DRG control: enable,
> > > +    destination, operating mode
> > > +
> > > +    * ``out_altvoltage131``: ``digital_ramp_up``: DRG ramp-up parameters:
> > > +      limits, step sizes, ramp rate
> > > +    * ``out_altvoltage132``: ``digital_ramp_down``: DRG ramp-down parameters:
> > > +      limits, step sizes, ramp rate
> > > +
> > > +  * ``out_altvoltage140``: ``ram_control``: RAM playback: enable, destination,
> > > +    operating mode, address range
> > > +
> > > +  * ``out_altvoltage150``: ``output_shift_keying``: OSK: enable, amplitude
> > > +    scale, ramp rate, auto/manual control
> > > +
> > > +The ``phy`` channel is the root of the hierarchy. Changing its
> > > +``sampling_frequency`` reconfigures the system clock (SYSCLK) which affects all
> > > +other channels. The ``profile`` attribute on this channel selects the active
> > > +hardware profile (0-7) used by the single tone and RAM channels.  
> > I asked out this profile thing in one of the other patches.  Key to me is
> > that how we write non active profiles?  The most similar thing we've seen
> > in the past has been setting other frequencies for FSK or phases for PSK or
> > more mundane DC DAC output that are symbol based. (often an external signal)  
> 
> Yes, not allowing to configure a non-active profile at this point.
> Initially was not seeing this as a problem, but would you think different
> channels for each profiles should be created? e.g.:
> 
> - out_altvoltage111 ... out_altvoltage118 for single tone profiles; and
> - out_altvoltage141 .. out_altvoltage148 for RAM profiles
> 
> That would not remove the need for something like the profile attribute.
Yes. I'm thinking it would be something along those lines. 

I think the tone and RAM profiles end up with totally different controls
so this should make it clear they aren't related.

> 
> > For those we have added an additional index so we can see which symbol we
> > are changing parameters for.  Here it might need to be done in the channel
> > numbering. I'm not sure.
> >   
> > > +
> > > +All mode-specific channels (parallel port, DRG, RAM, OSK) have an ``enable``
> > > +attribute. The DRG and RAM channels additionally have ``destination`` and
> > > +``operating_mode`` attributes that configure which DDS core parameter is
> > > +modulated and how.  
> > 
> > I wonder if we flatten things out and have separate channels for each type
> > of modulation. Might lead to a more standard looking interfaces. We don't really
> > have a standard path to control one type of thing feeding another, whereas
> > we do have simple 'enable' interfaces.  
> 
> ...
> 
> > > +RAM mode
> > > +--------
> > > +
> > > +The AD9910 contains a 1024 x 32-bit RAM that can be loaded with waveform data
> > > +and played back to modulate frequency, phase, amplitude, or polar (phase +
> > > +amplitude) parameters.
> > > +
> > > +RAM control channel attributes
> > > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > +
> > > +.. flat-table::
> > > +   :header-rows: 1
> > > +
> > > +   * - Attribute
> > > +     - Unit
> > > +     - Description
> > > +
> > > +   * - ``out_altvoltage140_en``
> > > +     - boolean
> > > +     - Enable/disable RAM playback. Toggling swaps profile registers between
> > > +       single tone and RAM configurations across all 8 profiles.  
> 
> ...
> 
> > > +   * - ``out_altvoltage140_address_start``  
> > Do we need this flexibility to set the start?  We needed a length, but
> > if we want different effective start can just load a different image.  
> 
> There is one image being loaded into the entire RAM and each profile may choose
> from wich sample it starts and ends its address ramp.
> The profiles can have those address ranges to overlap or not, then I suppose
> considering different images would just complicate things.

In the ABI docs discussion I raised the question on whether it makes more
sense to push this 'meta data' into the firmware image.  Depends a bit
on how costly loading the image is vs just updating this properties and whether
there are significant real use cases where tweaking what is run from the
image apply. I can easily conjecture some. I just have no idea if they are
real!

Thanks,

Jonathan

> 
> > > +     - integer
> > > +     - Start address for the active profile. Range [0, 1023]. Cannot be
> > > +       changed while RAM mode is enabled. If set above current end address,
> > > +       end address is automatically adjusted.
> > > +  
> 
> ...
> 

Re: [PATCH RFC v2 3/9] iio: frequency: ad9910: add simple parallel port mode support

[Jonathan Cameron]

On Wed, 18 Mar 2026 20:28:34 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Wed, Mar 18, 2026 at 05:56:03PM +0000, Rodrigo Alencar via B4 Relay wrote:
> 
> > Add parallel port channel with frequency scale, frequency offset, phase
> > offset, and amplitude offset extended attributes for configuring the
> > parallel data path.  
> 
> ...
> 
> > +	ret = iio_str_to_fixpoint(buf, MICRO / 10, &val, &val2);  
> 
> I think here we just use 100000 as it's in so many drivers de facto use.
> ideally this should be fixed on API level.

I wouldn't mind a series tidying this up, but if anyone proposes to do
that we'll want to not use the same naming so it is obvious if any
new drivers assume the old scaling.

I can't really remember why we ended up with the odd interface :(

Jonathan

Re: [PATCH RFC v2 3/9] iio: frequency: ad9910: add simple parallel port mode support

[Rodrigo Alencar]

On 26/03/18 08:28PM, Andy Shevchenko wrote:
> On Wed, Mar 18, 2026 at 05:56:03PM +0000, Rodrigo Alencar via B4 Relay wrote:
> 
> > Add parallel port channel with frequency scale, frequency offset, phase
> > offset, and amplitude offset extended attributes for configuring the
> > parallel data path.

...
 
> > +	case IIO_CHAN_INFO_ENABLE:
> > +		val = !!val;
> 
> Only used once, why do we need this...

Next patches introduce more channels here, so the additions are easier to review.
 
> > +		switch (chan->channel) {
> > +		case AD9910_CHANNEL_PARALLEL_PORT:
> > +			tmp32 = FIELD_PREP(AD9910_CFR2_PARALLEL_DATA_PORT_EN_MSK, val);
> 
> ...and not just here?
> 
> > +			return ad9910_reg32_update(st, AD9910_REG_CFR2,
> > +						   AD9910_CFR2_PARALLEL_DATA_PORT_EN_MSK,
> > +						   tmp32, true);
> > +		default:
> > +			return -EINVAL;
> > +		}

-- 
Kind regards,

Rodrigo Alencar

Re: [PATCH RFC v2 3/9] iio: frequency: ad9910: add simple parallel port mode support

[Andy Shevchenko]

On Mon, Mar 23, 2026 at 10:39:06AM +0000, Rodrigo Alencar wrote:
> On 26/03/18 08:28PM, Andy Shevchenko wrote:
> > On Wed, Mar 18, 2026 at 05:56:03PM +0000, Rodrigo Alencar via B4 Relay wrote:
> > 
> > > Add parallel port channel with frequency scale, frequency offset, phase
> > > offset, and amplitude offset extended attributes for configuring the
> > > parallel data path.

...

> > > +	case IIO_CHAN_INFO_ENABLE:
> > > +		val = !!val;
> > 
> > Only used once, why do we need this...
> 
> Next patches introduce more channels here, so the additions are easier to review.

Yeah, but  shouldn't be better to put this in each FIELD_PREP() as it will
immediately show the correctness and the value range without looking backwards
in the code?

> > > +		switch (chan->channel) {
> > > +		case AD9910_CHANNEL_PARALLEL_PORT:
> > > +			tmp32 = FIELD_PREP(AD9910_CFR2_PARALLEL_DATA_PORT_EN_MSK, val);
> > 
> > ...and not just here?
> > 
> > > +			return ad9910_reg32_update(st, AD9910_REG_CFR2,
> > > +						   AD9910_CFR2_PARALLEL_DATA_PORT_EN_MSK,
> > > +						   tmp32, true);
> > > +		default:
> > > +			return -EINVAL;
> > > +		}
> 
> -- 
> Kind regards,
> 
> Rodrigo Alencar

-- 
With Best Regards,
Andy Shevchenko