On Wed, 22 Apr 2026 15:45:55 +0100 Rodrigo Alencar via B4 Relay <[email protected]> wrote:
> From: Rodrigo Alencar <[email protected]> > > Trigger handler is implemented by leveraging the LDAC gpio when it is > available. Multiple channel writes can be flushed at once with the sync() > operation. > > Signed-off-by: Rodrigo Alencar <[email protected]> A few comments inline. > diff --git a/drivers/iio/dac/ad5686.c b/drivers/iio/dac/ad5686.c > index a065c614c874..bec951afe8d0 100644 > --- a/drivers/iio/dac/ad5686.c > +++ b/drivers/iio/dac/ad5686.c > @@ -506,6 +512,55 @@ 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); > + const struct iio_chan_spec *chan; > + u16 val[AD5686_MAX_CHANNELS]; I may be wrong but I suspect the static analysers won't like the fact that only part of this is initialised and they can't tell how much of it is then used. We might need some sanity checks to keep them happy even though we know they will always be fine (branch predictors should quickly make them near cost free). > + int ret, ch, i = 0; > + bool async_update; > + u8 cmd; > + > + ret = iio_pop_from_buffer(buffer, val); At somepoint we should probably add a sanity check on buffer size to that. > + 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) { > + chan = &indio_dev->channels[ch]; > + ret = st->ops->write(st, cmd, chan->address, > + val[i++] << chan->scan_type.shift); > + if (ret) > + break; I'd use a goto for this. > + } > + > + if (!ret && st->ops->sync) Then this becomes only if (st->ops->sync(st)) > + ret = st->ops->sync(st); /* flush all pending transfers */ > + and label probably ends up here. > + if (async_update) > + gpiod_set_value_cansleep(st->ldac_gpio, 1); > + > + mutex_unlock(&st->lock); > +out: > + iio_trigger_notify_done(indio_dev->trig); > + > + return IRQ_HANDLED; > +}
