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

[Rodrigo Alencar]

On 26/06/05 03:09PM, Jonathan Cameron wrote:
> On Fri, 5 Jun 2026 12:34:31 +0100
> Rodrigo Alencar <455.rodrigo.alencar@xxxxxxxxx> 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@xxxxxxxxxx> wrote:
> > >   
> > > > From: Rodrigo Alencar <rodrigo.alencar@xxxxxxxxxx>
> > > > 
> > > > 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.

That makes sense, I understand the concern. I didn't take __cleanup()
as ownership transfer. I thought it was just to register actions to variables
when they go out of scope or something. In that case the pointer itself is
being "cleaned up" (with **ppf in the cleanup action) not the memory that it
points to.

-- 
Kind regards,

Rodrigo Alencar