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

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 5 Jun 2026 12:34:31 +0100
Rodrigo Alencar <455.rodrigo.alencar@gmail.com> wrote:

> On 26/06/03 01:41PM, Jonathan Cameron wrote:
> > On Tue, 02 Jun 2026 17:33:57 +0100
> > Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@kernel.org> wrote:
> >   
> > > From: Rodrigo Alencar <rodrigo.alencar@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.  
> 
> ...
> 
> > > +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;

That's not a nice pattern given the lack of any local constructing. The ownership
transfer isn't obvious as both p and pf are really same type (it's slightly
hidden by the void * nature of p) - ideally we'd want p to be unusable after
that transfer.  Could do something hideous like 
		struct iio_poll_func *pf __cleanup(iio_trigger_always_done) =
			__get_and_null(p, NULL);
where p is set NULL so it becomes dead but that is ugly and not what that
is for so I doubt it would be popular and so we'd end up with yet another
weird macro.  There is some precedence with take_fd() but the use of that
is complex and I think it is only used to grab ownership from a local CLASS()
defined cleanup.

So it would work, but I've actively argued against this style elsewhere
in the kernel so don't really want it in IIO either!

Key disadvantage is that it is yet another weird bit of cleanup.h stuff for
people to learn and we have enough of those already.

So indeed an option but I'm not really liking it.

Jonathan

> 		/* ... */
> 
> 		ret = iio_pop_from_buffer(buffer, val);
> 		if (ret)
> 			return IRQ_HANDLED;
> 
> 		/* ... */
> 
> 		return IRQ_HANDLED;
> 	}
> 
> >   
> > > +
> > > +	return IRQ_HANDLED;
> > > +}  
>