Re: [PATCH v6 12/16] iio: frequency: ad9910: add RAM mode support

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 18 Jun 2026 14:27:28 +0100
Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@kernel.org> wrote:

> From: Rodrigo Alencar <rodrigo.alencar@analog.com>
> 
> Add RAM control channel, which includes:
> - RAM data loading via firmware upload interface;
> - Per-profile configuration and DDS core parameter destination as firmware
>   metadata;
> - Profile switching relying on profile channels;
> - Sampling frequency control of the active profile;
> - 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 profile parameters (frequency, phase,
> amplitude) for the single tone mode 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>

> diff --git a/drivers/iio/frequency/ad9910.c b/drivers/iio/frequency/ad9910.c
> index 3fe97aa887c3..c4e179dda715 100644
> --- a/drivers/iio/frequency/ad9910.c
> +++ b/drivers/iio/frequency/ad9910.c

> +static enum fw_upload_err ad9910_ram_fwu_write(struct fw_upload *fw_upload,
> +					       const u8 *data, u32 offset,
> +					       u32 size, u32 *written)
> +{
> +	const struct ad9910_ram_fw *fw_data = (const struct ad9910_ram_fw *)data;
> +	struct ad9910_state *st = fw_upload->dd_handle;
> +	int ret, ret2, idx, wcount;
> +	u64 tmp64, backup;
> +
> +	if (offset != 0)
> +		return FW_UPLOAD_ERR_INVALID_SIZE;
> +
> +	guard(mutex)(&st->lock);
> +
> +	if (st->ram_fwu_cancel)
> +		return FW_UPLOAD_ERR_CANCELED;
> +
> +	if (AD9910_RAM_ENABLED(st))
> +		return FW_UPLOAD_ERR_HW_ERROR;
> +
> +	for (idx = 0; idx < AD9910_NUM_PROFILES; idx++)
> +		st->reg_profile[idx] = get_unaligned_be64(&fw_data->profiles[idx]) |
> +				       AD9910_PROFILE_RAM_OPEN_MSK;
> +
> +	ret = ad9910_reg32_update(st, AD9910_REG_CFR1,
> +				  AD9910_CFR1_RAM_PLAYBACK_DEST_MSK |
> +				  AD9910_CFR1_INT_PROFILE_CTL_MSK,
> +				  get_unaligned_be32(&fw_data->cfr1), true);
> +	if (ret)
> +		return FW_UPLOAD_ERR_RW_ERROR;
> +
> +	wcount = get_unaligned_be16(&fw_data->wcount);
> +	if (!wcount) {
> +		*written = size;
> +		return FW_UPLOAD_ERR_NONE; /* nothing else to write */
> +	}
> +
> +	ret = ad9910_profile_set(st, st->profile);
> +	if (ret)
> +		return FW_UPLOAD_ERR_HW_ERROR;
> +
> +	/* backup profile register and update it with required address range */
> +	backup = st->reg[AD9910_REG_PROFILE(st->profile)].val64;
> +	tmp64 = AD9910_PROFILE_RAM_STEP_RATE_MSK |
> +		FIELD_PREP(AD9910_PROFILE_RAM_START_ADDR_MSK, 0) |
> +		FIELD_PREP(AD9910_PROFILE_RAM_END_ADDR_MSK, wcount - 1);
> +	ret = ad9910_reg64_write(st, AD9910_REG_PROFILE(st->profile), tmp64, true);
> +	if (ret)
> +		return FW_UPLOAD_ERR_RW_ERROR;
> +
> +	memcpy(&st->tx_buf[1], fw_data->words, wcount * AD9910_RAM_WORD_SIZE);
> +
> +	/* write ram data and restore profile register */
> +	ret = ad9910_spi_write(st, AD9910_REG_RAM,
> +			       wcount * AD9910_RAM_WORD_SIZE, false);
> +	ret2 = ad9910_reg64_write(st, AD9910_REG_PROFILE(st->profile), backup, true);
> +	if (ret || ret2)
> +		return FW_UPLOAD_ERR_RW_ERROR;
> +
> +	*written = size;

I'd like a blank line here. Mostly to make that 'good' return more obvious.

> +	return FW_UPLOAD_ERR_NONE;
> +}

>  
> +static inline void ad9910_debugfs_init(struct ad9910_state *st,
> +				       struct iio_dev *indio_dev)
> +{
> +	struct dentry *d = iio_get_debugfs_dentry(indio_dev);
> +	char buf[64];
> +
> +	/*
> +	 * symlinks are created here so iio userspace tools can refer to them
> +	 * as debug attributes.

Maybe worth a reference to appropriate ABI doc here (even if it is introduced
in a later patch)

> +	 */
> +	snprintf(buf, sizeof(buf), "/sys/class/firmware/%s/loading", st->ram_fwu_name);
> +	debugfs_create_symlink("ram_loading", d, buf);
> +
> +	snprintf(buf, sizeof(buf), "/sys/class/firmware/%s/data", st->ram_fwu_name);
> +	debugfs_create_symlink("ram_data", d, buf);
> +}
> +
>  static int ad9910_probe(struct spi_device *spi)
>  {
>  	static const char * const supplies[] = {
> @@ -1561,7 +1876,25 @@ static int ad9910_probe(struct spi_device *spi)
...

> +	ad9910_debugfs_init(st, indio_dev);

Blank line preferred before a simple return like this one.

> +	return 0;
>  }
>  
>  static const struct spi_device_id ad9910_id[] = {
>