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 <[email protected]>
>> wrote:
>>
>>> From: Rodrigo Alencar <[email protected]>
>>>
>>> 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 <[email protected]>
>>
>>> 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[] = {
>>>
>>
