Re: [PATCH 21/22] iio: dac: ad5686: add triggered buffer support

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 22 Apr 2026 15:45:55 +0100
Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@kernel.org> wrote:

> From: Rodrigo Alencar <rodrigo.alencar@analog.com>
> 
> Trigger handler is implemented by leveraging the LDAC gpio when it is
> available. Multiple channel writes can be flushed at once with the sync()
> operation.
> 
> Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>

A few comments inline.

> diff --git a/drivers/iio/dac/ad5686.c b/drivers/iio/dac/ad5686.c
> index a065c614c874..bec951afe8d0 100644
> --- a/drivers/iio/dac/ad5686.c
> +++ b/drivers/iio/dac/ad5686.c

> @@ -506,6 +512,55 @@ const struct ad5686_chip_info ad5679r_chip_info = {
>  };
>  EXPORT_SYMBOL_NS_GPL(ad5679r_chip_info, "IIO_AD5686");
>  
> +static irqreturn_t ad5686_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct iio_buffer *buffer = indio_dev->buffer;
> +	struct ad5686_state *st = iio_priv(indio_dev);
> +	const struct iio_chan_spec *chan;
> +	u16 val[AD5686_MAX_CHANNELS];

I may be wrong but I suspect the static analysers won't like the
fact that only part of this is initialised and they can't
tell how much of it is then used. We might need some sanity checks
to keep them happy even though we know they will always be fine
(branch predictors should quickly make them near cost free).

> +	int ret, ch, i = 0;
> +	bool async_update;
> +	u8 cmd;
> +
> +	ret = iio_pop_from_buffer(buffer, val);
At somepoint we should probably add a sanity check on buffer size to that.
> +	if (ret)
> +		goto out;
> +
> +	mutex_lock(&st->lock);
> +
> +	async_update = st->ldac_gpio && bitmap_weight(indio_dev->active_scan_mask,
> +						      iio_get_masklength(indio_dev)) > 1;
> +	if (async_update) {
> +		/* use ldac to update all channels simultaneously */
> +		cmd = AD5686_CMD_WRITE_INPUT_N;
> +		gpiod_set_value_cansleep(st->ldac_gpio, 0);
> +	} else {
> +		cmd = AD5686_CMD_WRITE_INPUT_N_UPDATE_N;
> +	}
> +
> +	iio_for_each_active_channel(indio_dev, ch) {
> +		chan = &indio_dev->channels[ch];
> +		ret = st->ops->write(st, cmd, chan->address,
> +				     val[i++] << chan->scan_type.shift);
> +		if (ret)
> +			break;
I'd use a goto for this.
> +	}
> +
> +	if (!ret && st->ops->sync)

Then this becomes only
	if (st->ops->sync(st))

> +		ret = st->ops->sync(st); /* flush all pending transfers */
> +

and label probably ends up here.

> +	if (async_update)
> +		gpiod_set_value_cansleep(st->ldac_gpio, 1);
> +
> +	mutex_unlock(&st->lock);
> +out:
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}