Re: [PATCH v3 08/10] iio: frequency: ad9910: support iio backend

[Andy Shevchenko]

On Tue, Nov 03, 2026 at 11:53:04AM -0300, Rodrigo Alencar via B4 Relay wrote:
> 
> The AD9910 parallel data port allows modulation of the DDS core parameters
> at up to the PDCLK rate, which requires a high-speed data source. Add
> support for an optional IIO backend that provides such interface, with
> data streamed into the device through an output buffer.
> 
> The parallel port channels get scan indexes and scan types according to
> the data destination: 14-bit amplitude, 16-bit phase, 16-bit frequency,
> or the 8-bit amplitude and phase pair for polar modulation. As only one
> destination can be active at a time, this is enforced by the available
> scan masks. On buffer enable, the destination is configured in the
> backend and the parallel data port is enabled. The sampling_frequency
> attribute of the parallel port channels controls the backend data rate.
> 
> The backend is also notified about profile changes, as it may drive the
> PROFILE[2:0] pins, and the debugfs source attributes now consider the
> parallel data port when reporting which channel controls each DDS core
> parameter.

...

> +enum {
> +	AD9910_SCAN_IDX_AMP = 0,

Why explicit assignment? Either we do all (for HW bits) or none and
let the C programming language standard take care about these.

> +	AD9910_SCAN_IDX_PHASE,
> +	AD9910_SCAN_IDX_FREQ,
> +	/* polar phase goes first, as it is placed in the lower byte, D[7:0] */
> +	AD9910_SCAN_IDX_POLAR_PHASE,
> +	AD9910_SCAN_IDX_POLAR_AMP,
> +};

...

> +static u32 ad9910_buffer_chan_idx(struct iio_dev *indio_dev)
> +{
> +	unsigned int scan_idx = find_first_bit(indio_dev->active_scan_mask,
> +					       iio_get_masklength(indio_dev));

Split assignment.

> +	return ad9910_scan_chan_idx[scan_idx];

Who guarantees the scan_idx to be in the boundaries?

> +}

...

> +static int ad9910_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> +	struct ad9910_state *st = iio_priv(indio_dev);
> +	int ret, ret2;
> +
> +	guard(mutex)(&st->lock);
> +
> +	st->scan_mask = 0;
> +	ret = ad9910_reg32_update(st, AD9910_REG_CFR2,
> +				  AD9910_CFR2_PARALLEL_DATA_PORT_EN_MSK,
> +				  0, true);
> +
> +	/* disable the backend channel regardless, return the first error */
> +	ret2 = iio_backend_chan_disable(st->back,
> +					ad9910_buffer_chan_idx(indio_dev));


I believe the second part of the previous comment should be split to standalone
comment here:

	/* Return first occurred error */

Maybe even with more elaboration on how the caller is supposed to react in both
cases.

> +	return ret ?: ret2;
> +}

-- 
With Best Regards,
Andy Shevchenko