Re: [PATCH RFC v4 06/10] iio: frequency: ad9910: add RAM mode support

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 08 May 2026 18:00:22 +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>

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