On 26/06/03 01:41PM, Jonathan Cameron wrote:
> On Tue, 02 Jun 2026 17:33:57 +0100
> Rodrigo Alencar via B4 Relay <[email protected]>
> wrote:
>
> > From: Rodrigo Alencar <[email protected]>
> >
> > 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.
...
> > +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] = { };
> > + int ret, ch, i = 0;
> > + bool async_update;
> > + u8 cmd;
> > +
> > + ret = iio_pop_from_buffer(buffer, val);
> > + 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) {
> > + ret = st->ops->write(st, cmd, indio_dev->channels[ch].address,
> > val[i++]);
> > + if (ret)
> > + goto cleanup;
> > + }
> > +
> > + if (st->ops->sync)
> > + ret = st->ops->sync(st); /* flush all pending transfers */
> > +
> > +cleanup:
It turns out that this label is not really needed. When sync() op is available
it must be called regardless of write failure, so the bus data can reset its
state. Then moving "cleanup" up would just make it useless.
> > + if (async_update)
>
> Error paths are always fun. Do we care about setting ldac_gpio to 1 if
> we failed to write the channel values? That will set any that did
> successfully
> update, but not all of them. Note I'm not sure on the right answer for this.
> There may not be one!
I would not see a problem with that, as there is no much we can do with errors
in a interrupt handler.
>
> > + gpiod_set_value_cansleep(st->ldac_gpio, 1);
> > +
> > + mutex_unlock(&st->lock);
> > +out:
> > + iio_trigger_notify_done(indio_dev->trig);
> We get this pattern so often (though not always). Feels like maybe
> we should put some effort into a generic opt in solution for this.
>
> A job for another day but options that come to mind.
> 1) (hideous) a flag
> 2) Maybe an alternative callback. thread_always_complete or
> something like that. Pain to wire through all the calls though
> and injecting the necessary wrapper isn't great either.
> Implementation wise would be a case of popping in a wrapper function
> in iio_trigger_attach_poll() call to request_threaded_irq().
> 3) Maybe a helper macro? Bit ugly as we'd need one to generate
> the wrapper function and another to use the same name for
> the registration function.
>
> Hmm. Those are all ugly (maybe 2 is ok ish). Suggestions welcome!
using a cleanup.h? with something like:
static inline void iio_trigger_always_done(struct iio_poll_func **ppf)
{
iio_trigger_notify_done((*ppf)->indio_dev->trig);
}
static irqreturn_t ad5686_trigger_handler(int irq, void *p)
{
struct iio_poll_func *pf __cleanup(iio_trigger_always_done) = p;
/* ... */
ret = iio_pop_from_buffer(buffer, val);
if (ret)
return IRQ_HANDLED;
/* ... */
return IRQ_HANDLED;
}
>
> > +
> > + return IRQ_HANDLED;
> > +}
--
Kind regards,
Rodrigo Alencar
