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

[David Lechner <dlechner@baylibre.com>]

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.

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