On Mon, 27 Jul 2026 10:28:44 +0100 Rodrigo Alencar <[email protected]> wrote:
> On 25/07/26 23:46, Jonathan Cameron wrote: > > On Wed, 22 Jul 2026 16:50: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]> > > 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; > > > } > > >
