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

[Jonathan Cameron]

On Mon, 27 Jul 2026 10:28:44 +0100
Rodrigo Alencar <455.rodrigo.alencar@xxxxxxxxx> wrote:

> On 25/07/26 23:46, Jonathan Cameron wrote:
> > On Wed, 22 Jul 2026 16:50:22 +0100
> > Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@xxxxxxxxxx> wrote:
> >   
> > > From: Rodrigo Alencar <rodrigo.alencar@xxxxxxxxxx>
> > > 
> > > 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@xxxxxxxxxx>  
> > 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.  
> 
> Yeah.. it is considering a cancel request right after the fw-update starts,
> with the fw-update failing for an invalid format before the check of the
> cancellation flag. With this, a second attempt on fw-update would fail again
> because the flag would not have been cleared.
> 
> Unusual, but I can have that handled.
> 
> > 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?  
> 
> Here, I focus on protecting the cached states (reverting the successful writes)
> rather than reverting stuff in HW after a SPI failure. The spi failure is delivered
> to userspace, so user should be aware that something is off.
> 
> What would happen if reverting HW states fails for nother SPI failure... I don't
> want to go into that...
> 
> If SPI starts to work again, other write attempts should be able to sync cached
> configs with HW.
Ok. That's fair. 

J
>  
> > > +				 */
> > > +				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;
> > >  		}  
> >   
>