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

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 4 Jul 2026 11:50:35 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 7/3/26 9:09 AM, Nuno Sá wrote:
> > On Fri, Jul 03, 2026 at 04:05:44AM +0100, Jonathan Cameron wrote:  
> >> 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)  
> > 
> > I'm not so sure about these links. I mean, I definitely agree we should
> > make it easy for userspace tools like libiio to be able to handle
> > these kind of attributes but using debugfs is questionable to me. Pretty
> > much because this is not a debug thing. It is a real setting for the
> > driver so ideally we would be able to control it (using the existent
> > tools) without enforcing one to mount debugfs (I know that most of the
> > times it's always mounted but still feels wrong to tie "real
> > functionality" to debugfs). 
> > 
> > Having said the above, some suggestions:
> > 
> > 1. Make the iio_dev the parent so that the attr name is just "ram" and
> > it will be a subdir /sys/bus/iio/iio:deviceN/ram/.
> > 2. Propose a new helper for the firmware_loader code so we can get
> > struct device from struct fw_upload then we can easily create a sysfs
> > symlink.
> > 3. Name the attr as dev_name(iio_dev):attr so that it becomes
> > iio:deviceN:attr_name.
> > 
> > Now that I think about it, 2. does not make much sense when compared to
> > 1. And If I'm not missing anything both 1. and 3. can be sanely parsable
> > from userspace (being 3. maybe a bit more reliable). And yes, both require
> > user space tools (in this case libiio) to support a new type of
> > attribute (firmware) but that is another problem.  
> 
> I would say that udev should be used to find the firmware device from
> userspace rather than creating symlinks. And you probably don't even
> need udev if you already have the `iio-device:<ID>` value. The path
> is always going to be `/sys/class/firmware/iio-device:<ID>:ram/`. It
> seems like the same amount of work for usespace to computer the path
> either way, so might as well keep it simple and leave out the symlink
> stuff.
> 
Seems reasonable.  We can always come back and add stuff later if a
strong usecases emerges.  Much easier than ripping it out!

Jonathan

> > 
> > - Nuno Sá  
> >>  
> >>> +	 */
> >>> +	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[] = {
> >>>  
> >>  
> 
>