On 26/06/05 03:09PM, Jonathan Cameron wrote: > On Fri, 5 Jun 2026 12:34:31 +0100 > Rodrigo Alencar <[email protected]> wrote: > > > 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; > > 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
