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

[Jonathan Cameron <jic23@kernel.org>]

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

>