On Fri, 08 May 2026 18:00:22 +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]> Minor stuff inline. > + > +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; > + > + /* copy ram profiles */ > + for (idx = 0; idx < AD9910_NUM_PROFILES; idx++) > + st->reg_profile[idx] = > get_unaligned_be64(&fw_data->profiles[idx]) | > + AD9910_PROFILE_RAM_OPEN_MSK; > + > + /* update CFR1 */ Comment is kind of obvious. Maybe say more or drop it. > + 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_be32(&fw_data->wcount); > + if (!wcount) { > + *written = size; > + return FW_UPLOAD_ERR_NONE; /* nothing else to write */ > + } > + > + /* ensure profile is selected */ Comment is not adding value unless there is more to say. > + 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; > + > + /* populate words into tx_buf[1:] */ Another comment that doesn't add value given the code is obviously doing that. If nothing else to say remove it. > + 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; > + return FW_UPLOAD_ERR_NONE; > +} > static int ad9910_probe(struct spi_device *spi) > { > static const char * const supplies[] = { > @@ -1688,7 +1991,21 @@ static int ad9910_probe(struct spi_device *spi) > if (ret) > return dev_err_probe(dev, ret, "device setup failed\n"); > > - return devm_iio_device_register(dev, indio_dev); > + ret = devm_iio_device_register(dev, indio_dev); > + if (ret) > + return ret; > + > + snprintf(st->ram_fwu_name, sizeof(st->ram_fwu_name), "%s:ram", > + dev_name(&indio_dev->dev)); > + st->ram_fwu = firmware_upload_register(THIS_MODULE, dev, > st->ram_fwu_name, > + &ad9910_ram_fwu_ops, st); > + if (IS_ERR(st->ram_fwu)) > + return dev_err_probe(dev, PTR_ERR(st->ram_fwu), > + "failed to register ram upload ops\n"); > + > + ad9910_debugfs_init(st, indio_dev); > + > + return devm_add_action_or_reset(dev, ad9910_ram_fwu_unregister, > st->ram_fwu); Why is this only after we've registered the device? At that point userspace stuff is exposed, so any risk of a race with this bit not having finished yet? Perhaps a comment would be useful. > } > > static const struct spi_device_id ad9910_id[] = { >
