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.
> > + */
> > + 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;
> > }
>
--
Kind regards,
Rodrigo Alencar
