On Sat, 4 Jul 2026 11:50:35 -0500 David Lechner <[email protected]> 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 > >> <[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. > 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[] = { > >>> > >> > >
