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

["Nuno Sá" <noname.nuno@gmail.com>]

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.

- 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[] = {
> > 
>