On 25/07/26 23:56, Jonathan Cameron wrote:
> On Wed, 22 Jul 2026 16:50:24 +0100
> Rodrigo Alencar via B4 Relay <[email protected]>
> wrote:
>
> > From: Rodrigo Alencar <[email protected]>
> >
> > Expose frequency_source, phase_source and amplitude_source attributes in
> > debugfs. Those indicate from which channel the specific DDS parameter is
> > being sourced by returning its label. The implementation follows the
> > priority table found in the datasheet.
> >
> For this one sashiko raised some questions and made me wonder how this
> actually works given it is using active_scan_masks and so far we don't
> have any buffered support in the driver. My guess is you backported
> this from on top of some other code that you haven't posted yet.
>
> Please have another check that this all works with just the series
> posted.
>
> Note that you will need to claim buffer mode successfully to mess
> around with that in paths that aren't inherently only used in buffered
> mode.
Yes, I will drop that part for now.
>
>
> > @@ -2078,6 +2092,171 @@ static int ad9910_setup(struct device *dev, struct
> > ad9910_state *st,
> > return ad9910_io_update(st);
> > }
> >
> > +static inline const char *ad9910_frequency_source_get(struct iio_dev
> > *indio_dev)
> > +{
> > + struct ad9910_state *st = iio_priv(indio_dev);
> > + bool ram_en, mode_en;
> > +
> > + guard(mutex)(&st->lock);
> > +
> > + /* RAM enabled and data destination is frequency */
> > + ram_en = AD9910_RAM_ENABLED(st);
> > + if (ram_en && AD9910_DEST_FREQUENCY ==
> > + FIELD_GET(AD9910_CFR1_RAM_PLAYBACK_DEST_MSK,
> > + st->reg[AD9910_REG_CFR1].val32))
> > + return ad9910_channel_str[AD9910_CHAN_IDX_RAM];
> > +
> > + /* DRG enabled and data destination is frequency */
> > + mode_en = FIELD_GET(AD9910_CFR2_DRG_ENABLE_MSK,
> > + st->reg[AD9910_REG_CFR2].val32);
> > + if (mode_en && AD9910_DEST_FREQUENCY ==
> > + FIELD_GET(AD9910_CFR2_DRG_DEST_MSK,
> > + st->reg[AD9910_REG_CFR2].val32))
> > + return ad9910_channel_str[AD9910_CHAN_IDX_DRG_FREQ];
> > +
> > + /* Parallel data port enabled and data destination is frequency */
> > + mode_en = FIELD_GET(AD9910_CFR2_PARALLEL_DATA_PORT_EN_MSK,
> > + st->reg[AD9910_REG_CFR2].val32);
> > + if (mode_en && indio_dev->active_scan_mask &&
>
> active_scan_mask is only set on a call to iio_enable_buffers.
> So what is this checking? I guess today it will always fail as
> active_scan_mask is NULL. Once that is in use, it will be racy
> however, so you'll need to claim buffered mode (or fail in which
> case the null check isn't needed). That claim will hold it
> in a particular state - under the hood it is taking mlock
> which is what sashiko suggests - that is just meant to be opaque
> to drivers so do it with an explicit attempt to claim buffered mode.
I will have buffer ops implemented, so I better cache the current
scan mask in a state variable and have that protected with the local lock.
I will drop those checks for now.
>
> > + test_bit(AD9910_SCAN_IDX_FREQ, indio_dev->active_scan_mask))
> > + return ad9910_channel_str[AD9910_CHAN_IDX_PARALLEL_FREQ];
> > +
> > + /* FTW: RAM enabled and data destination is phase, amplitude, or polar
> > */
> > + if (ram_en)
> > + return ad9910_channel_str[AD9910_CHAN_IDX_RAM];
> > +
> > + /* single tone profiles */
> > + return ad9910_channel_str[AD9910_CHAN_IDX_PROFILE_0 + st->profile];
> > +}
--
Kind regards,
Rodrigo Alencar
