Re: [PATCH v9 13/17] iio: frequency: ad9910: add RAM mode support

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 22 Jul 2026 16:50: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>
As mentioned in reply to an earlier patch, I haven't looked in detail
at the firmware cancel path stuff sashiko is unhappy with.  Whilst
it looks like the sort of esoteric path where maybe it is fine to fail
good to take one more look.

One other thing Sashiko commented on inline. I think that is either right
or a bit more detail is needed in the comment.

Thanks,

Jonathan


> diff --git a/drivers/iio/frequency/ad9910.c b/drivers/iio/frequency/ad9910.c
> index 6c794e1b4b1c..844cc0cc8f3e 100644
> --- a/drivers/iio/frequency/ad9910.c
> +++ b/drivers/iio/frequency/ad9910.c
...

> @@ -1119,7 +1220,7 @@ static int ad9910_write_raw(struct iio_dev *indio_dev,
>  	struct ad9910_state *st = iio_priv(indio_dev);
>  	u64 tmp64;
>  	u32 tmp32;
> -	int ret;
> +	int ret, i;
>  
>  	guard(mutex)(&st->lock);
>  
> @@ -1156,6 +1257,41 @@ static int ad9910_write_raw(struct iio_dev *indio_dev,
>  						   AD9910_CFR2_DRG_DEST_MSK |
>  						   AD9910_CFR2_DRG_ENABLE_MSK,
>  						   tmp32, true);
> +		case AD9910_CHANNEL_RAM:
> +			if (AD9910_RAM_ENABLED(st) == !!val)
> +				return 0;
> +
> +			/* swap profile configs */
> +			for (i = 0; i < AD9910_NUM_PROFILES; i++) {
> +				tmp64 = st->reg[AD9910_REG_PROFILE(i)].val64;
> +				ret = ad9910_reg64_write(st,
> +							 AD9910_REG_PROFILE(i),
> +							 st->reg_profile[i],
> +							 false);
> +				if (ret)
> +					break;
> +				st->reg_profile[i] = tmp64;
> +			}
> +
> +			if (ret) {
> +				/*
> +				 * After the write failure, profiles 0..i-1 were
> +				 * already swapped in SW, but Hw registers are
> +				 * still pending an IO update, so swap them back
> +				 * in SW to keep the state consistent.

Sashiko's follow up question about whether a subsequent use of IO update might
end up with these stale values seems like a reasonable one. Perhaps a little
more detail on why that doesn't matter is needed here?

> +				 */
> +				while (i--) {
> +					tmp64 = st->reg[AD9910_REG_PROFILE(i)].val64;
> +					st->reg[AD9910_REG_PROFILE(i)].val64 = st->reg_profile[i];
> +					st->reg_profile[i] = tmp64;
> +				}
> +				return ret;
> +			}
> +
> +			tmp32 = FIELD_PREP(AD9910_CFR1_RAM_ENABLE_MSK, !!val);
> +			return ad9910_reg32_update(st, AD9910_REG_CFR1,
> +						   AD9910_CFR1_RAM_ENABLE_MSK,
> +						   tmp32, true);
>  		default:
>  			return -EINVAL;
>  		}