Re: [PATCH v9 3/6] iio: adc: ad4691: add triggered buffer support

Andy Shevchenko
2026-05-04T07:57:52Z

On Thu, Apr 30, 2026 at 01:16:45PM +0300, Radu Sabau via B4 Relay wrote:

> 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.

...

> #include <linux/array_size.h>
> #include <linux/bitfield.h>
> -#include <linux/bitops.h>
> +#include <linux/bitmap.h>
> #include <linux/cleanup.h>
> #include <linux/delay.h>
> #include <linux/dev_printk.h>
> #include <linux/device/devres.h>
> +#include <linux/dmaengine.h>
> #include <linux/err.h>
> +#include <linux/interrupt.h>

+ kstrtox.h

> #include <linux/limits.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>

(double check that string.h is here or string_helpers.h in case the latter has
any use)

> #include <linux/units.h>
> #include <linux/unaligned.h>

...

> struct regmap *regmap;
> + struct spi_device *spi;
> +
> + struct pwm_device *conv_trigger;
> + int irq;
> int vref_uV;
> + u32 cnv_period_ns;
> +
> + bool manual_mode;
> bool refbuf_en;
> bool ldo_en;

> struct mutex lock;
> + /*
> + * Per-buffer-enable lifetime resources:
> + * Manual Mode - a pre-built SPI message that clocks out N+1
> + * transfers in one go.
> + * CNV Burst Mode - a pre-built SPI message that clocks out 2*N
> + * transfers in one go.
> + */
> + struct spi_message scan_msg;
> + /* max 16 + 1 NOOP (manual) or 2*16 + 2 (CNV burst). */
> + struct spi_transfer scan_xfers[34];
> + /*
> + * CNV burst: 16 AVG_IN addresses + state-reset address + state-reset
> + * value = 18. Manual: 16 channel cmds + 1 NOOP = 17.
> + */
> + __be16 scan_tx[18] __aligned(IIO_DMA_MINALIGN);
> + /*
> + * Scan buffer: one BE16 slot per active channel, plus timestamp.
> + * DMA-aligned because scan_xfers point rx_buf directly into vals[].
> + */
> + IIO_DECLARE_DMA_BUFFER_WITH_TS(__be16, vals, 16);

Have you run `pahole`? I'm wondering if this aligned member can be coupled with
something that gives lesser gap.

...

> +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;
> +
> + IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
> + if (IIO_DEV_ACQUIRE_FAILED(claim))
> + return -EBUSY;

> +
> +

Single blank line is enough.

> + ret = ad4691_set_pwm_freq(st, freq);
> + if (ret)
> + return ret;
> +
> + return len;
> +}

...

> +static int ad4691_read_scan(struct iio_dev *indio_dev, s64 timestamp)
> +{
> + struct ad4691_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + guard(mutex)(&st->lock);
> +
> + ret = spi_sync(st->spi, &st->scan_msg);
> + if (ret)
> + return ret;
> +
> + /*
> + * rx_buf pointers in scan_xfers point directly into scan.vals, so no
> + * copy is needed. The scan_msg already includes a STATE_RESET at the
> + * end (appended in preenable), so no explicit reset is needed here.
> + */
> + iio_push_to_buffers_with_ts(indio_dev, st->vals, sizeof(st->vals),
> + timestamp);

I would leave it on a single line (note, you can also shorten the timestamp to
ts).

> + return 0;
> +}

...

> +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");

PWM

> + return ad4691_set_pwm_freq(st, st->info->max_rate);
> +}

...

> + /*
> + * The GP pin named in interrupt-names asserts at end-of-conversion.
> + * The IRQ handler stops conversions and fires the IIO trigger so
> + * the trigger handler can read and push the sample to the buffer.
> + * The IRQ is kept disabled until the buffer is enabled.
> + */
> + irq = -ENXIO;
> + for (i = 0; i < ARRAY_SIZE(ad4691_gp_names); i++) {
> + irq = fwnode_irq_get_byname(dev_fwnode(dev),
> + ad4691_gp_names[i]);
> + if (irq > 0)
> + break;

This is problematic in case the above returns EPROBE_DEFER. Can you confirm it
may not ever happen? (Note, I don't know the answer.)

> + }
> + if (irq < 0)
> + return dev_err_probe(dev, irq, "failed to get GP interrupt\n");

--
With Best Regards,
Andy Shevchenko