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

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 22 Jul 2026 16:50:24 +0100
Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@kernel.org> wrote:

> From: Rodrigo Alencar <rodrigo.alencar@analog.com>
> 
> 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.



> @@ -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.


> +	    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];
> +}