Re: [PATCH v9 15/17] iio: frequency: ad9910: show channel priority in debugfs

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