Re: [PATCH v4 11/12] iio: dac: ad5686: add triggered buffer support

[Jonathan Cameron]

On Tue, 23 Jun 2026 11:55:51 +0100
Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog....@kernel.org> 
wrote:

> From: Rodrigo Alencar <rodrigo.alen...@analog.com>
> 
> Implement trigger handler by leveraging the LDAC gpio to update all DAC
> channels at once when it is available. Also, the multiple channel writes
> can be flushed at once with the sync() operation.
> 
> Signed-off-by: Rodrigo Alencar <rodrigo.alen...@analog.com>
Hi Rodrigo

A follow up on the rework you did for this version,

Thanks,

Jonathan

> diff --git a/drivers/iio/dac/ad5686.c b/drivers/iio/dac/ad5686.c
> index db175e77b0b7..4dc681eb077d 100644
> --- a/drivers/iio/dac/ad5686.c
> +++ b/drivers/iio/dac/ad5686.c

> @@ -467,6 +472,60 @@ 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);
> +     u16 val[AD5686_MAX_CHANNELS] = { };
> +     unsigned int scan_count, ch, i;
> +     bool async_update;
> +     int ret;
> +     u8 cmd;
> +
> +     ret = iio_pop_from_buffer(buffer, val);
> +     if (ret) {
> +             iio_trigger_notify_done(indio_dev->trig);

I think I'd prefer a wrapper to this if we are going to always say
it is IRQ_HANDLED (which is reasonable here I think)
Something like the following (unfortunately I only just read the
previous version thread so didn't head off the approach you have here.


static void do_ad5686_trigger_handler(struct iio_dev *indio_dev)
{
        struct iio_buffer *buffer = indio_dev->buffer;
        struct ad5686_state *st = iio_priv(indio_dev);
        u16 val[AD5686_MAX_CHANNELS] = { };
        int ret;

        ret = iio_pop_from_buffer(buffer, val);
        if (ret) {
                iio_trigger_notify_done(indio_dev->trig);

        guard(mutex)(&st->lock);

        guts of current function
}

static irqreturn_t ad5686_trigger_handler(int irq, void *p)
{
        struct iio_poll_func *pf = p;
        struct iio_dev *indio_dev = pf->indio_dev;

        do_ad5686_trigger_handler(indio_dev);
        iio_trigger_notify_done(indio_dev->trig);
        return IRQ_HANDLED;
}



> +             return IRQ_HANDLED;
> +     }
> +
> +     guard(mutex)(&st->lock);
> +
> +     scan_count = bitmap_weight(indio_dev->active_scan_mask,
> +                                iio_get_masklength(indio_dev));
> +     async_update = st->ldac_gpio && scan_count > 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;
> +     }
> +
> +     i = 0;
> +     iio_for_each_active_channel(indio_dev, ch) {
> +             ret = st->ops->write(st, cmd, indio_dev->channels[ch].address, 
> val[i++]);
> +             if (ret)
> +                     break;
> +     }
> +
> +     /*
> +      * If sync() is available, it is called here regardless of write
> +      * failure to allow bus implementation to reset. In that case, partial
> +      * writes are unlikely as the write operations would just queue up
> +      * the transfers.
> +      */
> +     if (st->ops->sync)
> +             st->ops->sync(st);
> +
> +     if (async_update)
> +             gpiod_set_value_cansleep(st->ldac_gpio, 1);
> +
> +     iio_trigger_notify_done(indio_dev->trig);
> +     return IRQ_HANDLED;
> +}