Re: [PATCH v5 3/4] iio: adc: ad4691: add triggered buffer support
From: Nuno Sá
Date: Sat Mar 28 2026 - 11:59:05 EST
Hi Radu,
Looking good. Just some minor stuff.
On Fri, 2026-03-27 at 13:07 +0200, Radu Sabau via B4 Relay wrote:
> From: Radu Sabau <radu.sabau@xxxxxxxxxx>
>
> Add buffered capture support using the IIO triggered buffer framework.
>
> CNV Burst Mode: the GP pin identified by interrupt-names in the device
> tree is configured as DATA_READY output. The IRQ handler stops
> conversions and fires the IIO trigger; the trigger handler executes a
> pre-built SPI message that reads all active channels from the AVG_IN
> accumulator registers and then resets accumulator state and restarts
> conversions for the next cycle.
>
> Manual Mode: CNV is tied to SPI CS so each transfer simultaneously
> reads the previous result and starts the next conversion (pipelined
> N+1 scheme). At preenable time a pre-built, optimised SPI message of
> N+1 transfers is constructed (N channel reads plus one NOOP to drain
> the pipeline). The trigger handler executes the message in a single
> spi_sync() call and collects the results. An external trigger (e.g.
> iio-trig-hrtimer) is required to drive the trigger at the desired
> sample rate.
>
> Both modes share the same trigger handler and push a complete scan —
> one u16 slot per channel at its scan_index position, followed by a
> timestamp — to the IIO buffer via iio_push_to_buffers_with_ts().
>
> The CNV Burst Mode sampling frequency (PWM period) is exposed as a
> buffer-level attribute via IIO_DEVICE_ATTR.
>
> Signed-off-by: Radu Sabau <radu.sabau@xxxxxxxxxx>
> ---
> drivers/iio/adc/Kconfig | 2 +
> drivers/iio/adc/ad4691.c | 616 ++++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 612 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 3685a03aa8dc..d498f16c0816 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -142,6 +142,8 @@ config AD4170_4
> config AD4691
> tristate "Analog Devices AD4691 Family ADC Driver"
> depends on SPI
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
> select REGMAP
> help
> Say yes here to build support for Analog Devices AD4691 Family MuxSAR
> diff --git a/drivers/iio/adc/ad4691.c b/drivers/iio/adc/ad4691.c
> index f930efdb9d8c..b5a7646b46ca 100644
> --- a/drivers/iio/adc/ad4691.c
> +++ b/drivers/iio/adc/ad4691.c
> @@ -4,14 +4,18 @@
> * Author: Radu Sabau <radu.sabau@xxxxxxxxxx>
> */
> #include <linux/bitfield.h>
> +#include <linux/bitmap.h>
> #include <linux/bitops.h>
> #include <linux/cleanup.h>
> #include <linux/delay.h>
> #include <linux/device.h>
> #include <linux/err.h>
> +#include <linux/interrupt.h>
> #include <linux/math.h>
> #include <linux/module.h>
> #include <linux/mod_devicetable.h>
> +#include <linux/property.h>
> +#include <linux/pwm.h>
> #include <linux/regmap.h>
> #include <linux/regulator/consumer.h>
> #include <linux/reset.h>
> @@ -19,7 +23,12 @@
> #include <linux/units.h>
> #include <linux/unaligned.h>
>
...
>
> +static void ad4691_free_scan_bufs(struct ad4691_state *st)
> +{
> + kfree(st->scan_xfers);
> + kfree(st->scan_tx);
> + kfree(st->scan_rx);
> + st->scan_xfers = NULL;
> + st->scan_tx = NULL;
> + st->scan_rx = NULL;
Do we need NULL. Are you actually checking for that anywhere?
> +}
> +
> +static int ad4691_manual_buffer_preenable(struct iio_dev *indio_dev)
> +{
> + struct ad4691_state *st = iio_priv(indio_dev);
> + struct device *dev = regmap_get_device(st->regmap);
> + struct spi_device *spi = to_spi_device(dev);
> + unsigned int n_active = bitmap_weight(indio_dev->active_scan_mask,
> + indio_dev->masklength);
ditto
> + unsigned int n_xfers = n_active + 1;
> + unsigned int k, i;
> + int ret;
> +
> + st->scan_xfers = kcalloc(n_xfers, sizeof(*st->scan_xfers), GFP_KERNEL);
> + if (!st->scan_xfers)
> + return -ENOMEM;
> +
> + st->scan_tx = kcalloc(n_xfers, sizeof(*st->scan_tx), GFP_KERNEL);
> + if (!st->scan_tx) {
> + kfree(st->scan_xfers);
> + return -ENOMEM;
> + }
> +
> + st->scan_rx = kcalloc(n_xfers, sizeof(*st->scan_rx), GFP_KERNEL);
> + if (!st->scan_rx) {
> + kfree(st->scan_tx);
> + kfree(st->scan_xfers);
> + return -ENOMEM;
> + }
> +
> + spi_message_init(&st->scan_msg);
> +
> + k = 0;
> + iio_for_each_active_channel(indio_dev, i) {
> + st->scan_tx[k] = cpu_to_be16(AD4691_ADC_CHAN(i));
> + st->scan_xfers[k].tx_buf = &st->scan_tx[k];
> + st->scan_xfers[k].rx_buf = &st->scan_rx[k];
> + st->scan_xfers[k].len = sizeof(__be16);
> + st->scan_xfers[k].cs_change = 1;
> + spi_message_add_tail(&st->scan_xfers[k], &st->scan_msg);
> + k++;
> + }
> +
> + /* Final NOOP transfer to retrieve last channel's result. */
> + st->scan_tx[k] = cpu_to_be16(AD4691_NOOP);
> + st->scan_xfers[k].tx_buf = &st->scan_tx[k];
> + st->scan_xfers[k].rx_buf = &st->scan_rx[k];
> + st->scan_xfers[k].len = sizeof(__be16);
> + spi_message_add_tail(&st->scan_xfers[k], &st->scan_msg);
> +
> + st->scan_msg.spi = spi;
> +
> + ret = spi_optimize_message(spi, &st->scan_msg);
> + if (ret) {
> + ad4691_free_scan_bufs(st);
> + return ret;
> + }
> +
> + ret = ad4691_enter_conversion_mode(st);
> + if (ret) {
> + spi_unoptimize_message(&st->scan_msg);
> + ad4691_free_scan_bufs(st);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int ad4691_manual_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> + struct ad4691_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + ret = ad4691_exit_conversion_mode(st);
> + spi_unoptimize_message(&st->scan_msg);
> + ad4691_free_scan_bufs(st);
> + return ret;
> +}
> +
> +static const struct iio_buffer_setup_ops ad4691_manual_buffer_setup_ops = {
> + .preenable = &ad4691_manual_buffer_preenable,
> + .postdisable = &ad4691_manual_buffer_postdisable,
> +};
> +
> +static int ad4691_cnv_burst_buffer_preenable(struct iio_dev *indio_dev)
> +{
> + struct ad4691_state *st = iio_priv(indio_dev);
> + struct device *dev = regmap_get_device(st->regmap);
> + struct spi_device *spi = to_spi_device(dev);
> + unsigned int n_active = bitmap_weight(indio_dev->active_scan_mask,
> + indio_dev->masklength);
masklength is __private. Use iio_get_masklength(). I would expect sparse to
complain about the above.
> + unsigned int bit, k, i;
> + int ret;
> +
> + st->scan_xfers = kcalloc(2 * n_active, sizeof(*st->scan_xfers),
> GFP_KERNEL);
> + if (!st->scan_xfers)
> + return -ENOMEM;
> +
> + st->scan_tx = kcalloc(n_active, sizeof(*st->scan_tx), GFP_KERNEL);
> + if (!st->scan_tx) {
> + kfree(st->scan_xfers);
> + return -ENOMEM;
> + }
> +
> + st->scan_rx = kcalloc(n_active, sizeof(*st->scan_rx), GFP_KERNEL);
> + if (!st->scan_rx) {
> + kfree(st->scan_tx);
> + kfree(st->scan_xfers);
> + return -ENOMEM;
> + }
> +
> + spi_message_init(&st->scan_msg);
> +
> + /*
> + * Each AVG_IN read needs two transfers: a 2-byte address write phase
> + * followed by a 2-byte data read phase. CS toggles between channels
> + * (cs_change=1 on the read phase of all but the last channel).
> + */
> + k = 0;
> + iio_for_each_active_channel(indio_dev, i) {
> + st->scan_tx[k] = cpu_to_be16(0x8000 | AD4691_AVG_IN(i));
> + st->scan_xfers[2 * k].tx_buf = &st->scan_tx[k];
> + st->scan_xfers[2 * k].len = sizeof(__be16);
> + spi_message_add_tail(&st->scan_xfers[2 * k], &st->scan_msg);
> + st->scan_xfers[2 * k + 1].rx_buf = &st->scan_rx[k];
> + st->scan_xfers[2 * k + 1].len = sizeof(__be16);
> + if (k < n_active - 1)
> + st->scan_xfers[2 * k + 1].cs_change = 1;
> + spi_message_add_tail(&st->scan_xfers[2 * k + 1], &st->scan_msg);
> + k++;
> + }
> +
> + st->scan_msg.spi = spi;
> +
> + ret = spi_optimize_message(spi, &st->scan_msg);
> + if (ret) {
> + ad4691_free_scan_bufs(st);
> +
Why not goto here?
Alternatively, to simplify error handling with buffers you can use some helper
variables with __free(kfree) and then in the end assign to st with no_free_ptr().
> return ret;
> + }
> +
> + ret = regmap_write(st->regmap, AD4691_ACC_MASK_REG,
> + (u16)~bitmap_read(indio_dev->active_scan_mask, 0,
> + indio_dev->masklength));
> + if (ret)
> + goto err;
> +
> + ret = regmap_write(st->regmap, AD4691_STD_SEQ_CONFIG,
> + bitmap_read(indio_dev->active_scan_mask, 0,
> + indio_dev->masklength));
> + if (ret)
> + goto err;
> +
> + iio_for_each_active_channel(indio_dev, bit) {
> + ret = regmap_write(st->regmap, AD4691_ACC_DEPTH_IN(bit),
> + st->osr[bit]);
> + if (ret)
> + goto err;
> + }
> +
> + ret = ad4691_enter_conversion_mode(st);
> + if (ret)
> + goto err;
> +
> + ret = ad4691_sampling_enable(st, true);
> + if (ret)
> + goto err;
> +
> + enable_irq(st->irq);
> + return 0;
> +err:
> + spi_unoptimize_message(&st->scan_msg);
> + ad4691_free_scan_bufs(st);
> + return ret;
> +}
> +
> +static int ad4691_cnv_burst_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> + struct ad4691_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + disable_irq(st->irq);
> +
> + ret = ad4691_sampling_enable(st, false);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->regmap, AD4691_STD_SEQ_CONFIG,
> + AD4691_SEQ_ALL_CHANNELS_OFF);
> + if (ret)
> + return ret;
> +
> + ret = ad4691_exit_conversion_mode(st);
> + spi_unoptimize_message(&st->scan_msg);
> + ad4691_free_scan_bufs(st);
> + return ret;
> +}
> +
> +static const struct iio_buffer_setup_ops ad4691_cnv_burst_buffer_setup_ops = {
> + .preenable = &ad4691_cnv_burst_buffer_preenable,
> + .postdisable = &ad4691_cnv_burst_buffer_postdisable,
> +};
> +
> +static ssize_t sampling_frequency_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct ad4691_state *st = iio_priv(indio_dev);
> +
> + return sysfs_emit(buf, "%u\n", (u32)(NSEC_PER_SEC / st->cnv_period_ns));
> +}
> +
> +static ssize_t sampling_frequency_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct ad4691_state *st = iio_priv(indio_dev);
> + int freq, ret;
> +
> + ret = kstrtoint(buf, 10, &freq);
> + if (ret)
> + return ret;
> +
> + guard(mutex)(&st->lock);
> +
> + ret = ad4691_set_pwm_freq(st, freq);
> + if (ret)
> + return ret;
> +
Question. Is it safe or does it make sense to change the above while buffering?
Same question might be valid for other stuff like oversampling.
> + return len;
> +}
> +
> +static IIO_DEVICE_ATTR(sampling_frequency, 0644,
> + sampling_frequency_show,
> + sampling_frequency_store, 0);
> +
> +static const struct iio_dev_attr *ad4691_buffer_attrs[] = {
> + &iio_dev_attr_sampling_frequency,
> + NULL
> +};
> +
> +static irqreturn_t ad4691_irq(int irq, void *private)
> +{
> + struct iio_dev *indio_dev = private;
> + struct ad4691_state *st = iio_priv(indio_dev);
> +
> + /*
> + * GPx has asserted: stop conversions before reading so the
> + * accumulator does not continue sampling while the trigger handler
> + * processes the data. Then fire the IIO trigger to push the sample
> + * to the buffer.
> + */
> + ad4691_sampling_enable(st, false);
> + iio_trigger_poll(indio_dev->trig);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static const struct iio_trigger_ops ad4691_trigger_ops = {
> + .validate_device = iio_trigger_validate_own_device,
> +};
> +
> +static int ad4691_read_scan(struct iio_dev *indio_dev, s64 timestamp)
> +{
> + struct ad4691_state *st = iio_priv(indio_dev);
> + unsigned int i, k = 0;
> + int ret;
> +
> + guard(mutex)(&st->lock);
> +
> + ret = spi_sync(st->scan_msg.spi, &st->scan_msg);
> + if (ret)
> + return ret;
> +
> + if (st->manual_mode) {
> + iio_for_each_active_channel(indio_dev, i) {
> + st->scan.vals[i] = be16_to_cpu(st->scan_rx[k + 1]);
> + k++;
> + }
> + } else {
> + iio_for_each_active_channel(indio_dev, i) {
> + st->scan.vals[i] = be16_to_cpu(st->scan_rx[k]);
> + k++;
> + }
> +
> + ret = regmap_write(st->regmap, AD4691_STATE_RESET_REG,
> + AD4691_STATE_RESET_ALL);
> + if (ret)
> + return ret;
> +
> + ret = ad4691_sampling_enable(st, true);
> + if (ret)
> + return ret;
> + }
> +
> + iio_push_to_buffers_with_ts(indio_dev, &st->scan, sizeof(st->scan),
> + timestamp);
> + return 0;
> +}
> +
> +static irqreturn_t ad4691_trigger_handler(int irq, void *p)
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *indio_dev = pf->indio_dev;
> +
> + ad4691_read_scan(indio_dev, pf->timestamp);
> + iio_trigger_notify_done(indio_dev->trig);
> + return IRQ_HANDLED;
> +}
> +
> static const struct iio_info ad4691_info = {
> .read_raw = &ad4691_read_raw,
> .write_raw = &ad4691_write_raw,
> @@ -493,6 +982,18 @@ static const struct iio_info ad4691_info = {
> .debugfs_reg_access = &ad4691_reg_access,
> };
>
> +static int ad4691_pwm_setup(struct ad4691_state *st)
> +{
> + struct device *dev = regmap_get_device(st->regmap);
> +
> + st->conv_trigger = devm_pwm_get(dev, "cnv");
> + if (IS_ERR(st->conv_trigger))
> + return dev_err_probe(dev, PTR_ERR(st->conv_trigger),
> + "Failed to get cnv pwm\n");
> +
> + return ad4691_set_pwm_freq(st, st->info->max_rate);
> +}
> +
> static int ad4691_regulator_setup(struct ad4691_state *st)
> {
> struct device *dev = regmap_get_device(st->regmap);
> @@ -557,8 +1058,25 @@ static int ad4691_config(struct ad4691_state *st)
> {
> struct device *dev = regmap_get_device(st->regmap);
> enum ad4691_ref_ctrl ref_val;
> + unsigned int gp_num;
> int ret;
>
> + /*
> + * Determine buffer conversion mode from DT: if a PWM is provided it
> + * drives the CNV pin (CNV_BURST_MODE); otherwise CNV is tied to CS
> + * and each SPI transfer triggers a conversion (MANUAL_MODE).
> + * Both modes idle in AUTONOMOUS mode so that read_raw can use the
> + * internal oscillator without disturbing the hardware configuration.
> + */
> + if (device_property_present(dev, "pwms")) {
> + st->manual_mode = false;
> + ret = ad4691_pwm_setup(st);
> + if (ret)
> + return ret;
> + } else {
> + st->manual_mode = true;
> + }
> +
> switch (st->vref_uV) {
> case AD4691_VREF_uV_MIN ... AD4691_VREF_2P5_uV_MAX:
> ref_val = AD4691_VREF_2P5;
> @@ -609,7 +1127,87 @@ static int ad4691_config(struct ad4691_state *st)
> if (ret)
> return dev_err_probe(dev, ret, "Failed to write ADC_SETUP\n");
>
> - return 0;
> + if (st->manual_mode)
> + return 0;
> +
> + for (gp_num = 0; gp_num < ARRAY_SIZE(ad4691_gp_names); gp_num++) {
> + if (fwnode_irq_get_byname(dev_fwnode(dev),
> + ad4691_gp_names[gp_num]) > 0)
Don't love this line break. I'm also a bit puzzled. How does the above differs from
the trigger code? I guess this should be the same GP pin?
- Nuno Sá