Re: [PATCH v4 3/4] iio: adc: ad4691: add triggered buffer support

["Uwe Kleine-König" <ukleinek@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 12093 bytes --]

On Fri, Mar 20, 2026 at 01:03:57PM +0200, Radu Sabau via B4 Relay wrote:
> From: Radu Sabau <radu.sabau@analog.com>
> 
> 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@analog.com>
> ---
>  drivers/iio/adc/Kconfig  |   2 +
>  drivers/iio/adc/ad4691.c | 584 +++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 571 insertions(+), 15 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 5e02eb44ca44..db776de32846 100644
> --- a/drivers/iio/adc/ad4691.c
> +++ b/drivers/iio/adc/ad4691.c
> @@ -9,9 +9,12 @@
>  #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 +22,12 @@
>  #include <linux/units.h>
>  #include <linux/unaligned.h>
>  
> +#include <linux/iio/buffer.h>
>  #include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
>  
>  #define AD4691_VREF_uV_MIN			2400000
>  #define AD4691_VREF_uV_MAX			5250000
> @@ -28,6 +36,8 @@
>  #define AD4691_VREF_3P3_uV_MAX			3750000
>  #define AD4691_VREF_4P096_uV_MAX		4500000
>  
> +#define AD4691_CNV_DUTY_CYCLE_NS		380
> +
>  #define AD4691_SPI_CONFIG_A_REG			0x000
>  #define AD4691_SW_RESET				(BIT(7) | BIT(0))
>  
> @@ -35,6 +45,7 @@
>  #define AD4691_CLAMP_STATUS1_REG		0x01A
>  #define AD4691_CLAMP_STATUS2_REG		0x01B
>  #define AD4691_DEVICE_SETUP			0x020
> +#define AD4691_MANUAL_MODE			BIT(2)
>  #define AD4691_LDO_EN				BIT(4)
>  #define AD4691_REF_CTRL				0x021
>  #define AD4691_REF_CTRL_MASK			GENMASK(4, 2)
> @@ -42,21 +53,29 @@
>  #define AD4691_OSC_FREQ_REG			0x023
>  #define AD4691_OSC_FREQ_MASK			GENMASK(3, 0)
>  #define AD4691_STD_SEQ_CONFIG			0x025
> +#define AD4691_SEQ_ALL_CHANNELS_OFF		0x00
>  #define AD4691_SPARE_CONTROL			0x02A
>  
> +#define AD4691_NOOP				0x00
> +#define AD4691_ADC_CHAN(ch)			((0x10 + (ch)) << 3)
> +
>  #define AD4691_OSC_EN_REG			0x180
>  #define AD4691_STATE_RESET_REG			0x181
>  #define AD4691_STATE_RESET_ALL			0x01
>  #define AD4691_ADC_SETUP			0x182
> -#define AD4691_AUTONOMOUS_MODE_VAL		0x02
> +#define AD4691_CNV_BURST_MODE			0x01
> +#define AD4691_AUTONOMOUS_MODE			0x02
>  /*
>   * ACC_MASK_REG covers both mask bytes via ADDR_DESCENDING SPI: writing a
>   * 16-bit BE value to 0x185 auto-decrements to 0x184 for the second byte.
>   */
>  #define AD4691_ACC_MASK_REG			0x185
>  #define AD4691_ACC_COUNT_LIMIT(n)		(0x186 + (n))
> +#define AD4691_ACC_COUNT_VAL			0x1
>  #define AD4691_GPIO_MODE1_REG			0x196
>  #define AD4691_GPIO_MODE2_REG			0x197
> +#define AD4691_GP_MODE_MASK			GENMASK(3, 0)
> +#define AD4691_GP_MODE_DATA_READY		0x06
>  #define AD4691_GPIO_READ			0x1A0
>  #define AD4691_ACC_STATUS_FULL1_REG		0x1B0
>  #define AD4691_ACC_STATUS_FULL2_REG		0x1B1
> @@ -121,6 +140,7 @@ static const struct iio_chan_spec ad4691_channels[] = {
>  	AD4691_CHANNEL(13),
>  	AD4691_CHANNEL(14),
>  	AD4691_CHANNEL(15),
> +	IIO_CHAN_SOFT_TIMESTAMP(16),
>  };
>  
>  static const struct iio_chan_spec ad4693_channels[] = {
> @@ -132,6 +152,7 @@ static const struct iio_chan_spec ad4693_channels[] = {
>  	AD4691_CHANNEL(5),
>  	AD4691_CHANNEL(6),
>  	AD4691_CHANNEL(7),
> +	IIO_CHAN_SOFT_TIMESTAMP(16),
>  };
>  
>  /*
> @@ -189,16 +210,63 @@ static const struct ad4691_chip_info ad4694_chip_info = {
>  struct ad4691_state {
>  	const struct ad4691_chip_info	*info;
>  	struct regmap			*regmap;
> +
> +	struct pwm_device		*conv_trigger;
> +	struct iio_trigger		*trig;
> +	int				irq;
> +
> +	bool				manual_mode;
> +
>  	int				vref_uV;
>  	bool				refbuf_en;
>  	bool				ldo_en;
> +	u32				cnv_period_ns;
>  	/*
>  	 * Synchronize access to members of the driver state, and ensure
>  	 * atomicity of consecutive SPI operations.
>  	 */
>  	struct mutex			lock;
> +	/*
> +	 * Per-buffer-enabl ree lifetimesources:
> +	 * 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.
> +	 */
> +	void				*scan_devm_group;
> +	struct spi_message		scan_msg;
> +	struct spi_transfer		*scan_xfers;
> +	__be16				*scan_tx;
> +	__be16				*scan_rx;
> +	/* Scan buffer: one slot per channel (u16) plus timestamp */
> +	struct {
> +		u16 vals[16];
> +		s64 ts __aligned(8);
> +	} scan __aligned(IIO_DMA_MINALIGN);
>  };
>  
> +/*
> + * Configure the given GP pin (0-3) as DATA_READY output.
> + * GP0/GP1 → GPIO_MODE1_REG, GP2/GP3 → GPIO_MODE2_REG.
> + * Even pins occupy bits [3:0], odd pins bits [7:4].
> + */
> +static int ad4691_gpio_setup(struct ad4691_state *st, unsigned int gp_num)
> +{
> +	unsigned int shift = 4 * (gp_num % 2);
> +
> +	return regmap_update_bits(st->regmap,
> +				  AD4691_GPIO_MODE1_REG + gp_num / 2,
> +				  AD4691_GP_MODE_MASK << shift,
> +				  AD4691_GP_MODE_DATA_READY << shift);
> +}
> +
> +static void ad4691_disable_pwm(void *data)
> +{
> +	struct pwm_state state = { .enabled = false };
> +
> +	pwm_apply_might_sleep(data, &state);
> +}
> +
>  static int ad4691_reg_read(void *context, unsigned int reg, unsigned int *val)
>  {
>  	struct spi_device *spi = context;
> @@ -341,14 +409,16 @@ static int ad4691_get_sampling_freq(struct ad4691_state *st, int *val)
>  static int ad4691_set_sampling_freq(struct iio_dev *indio_dev, int freq)
>  {
>  	struct ad4691_state *st = iio_priv(indio_dev);
> -	unsigned int start = (st->info->max_rate == HZ_PER_MHZ) ? 0 : 1;
>  	unsigned int i;
>  
> +	if (freq > st->info->max_rate)
> +		return -EINVAL;
> +
>  	IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
>  	if (IIO_DEV_ACQUIRE_FAILED(claim))
>  		return -EBUSY;
>  
> -	for (i = start; i < ARRAY_SIZE(ad4691_osc_freqs); i++) {
> +	for (i = 0; i < ARRAY_SIZE(ad4691_osc_freqs); i++) {
>  		if ((int)ad4691_osc_freqs[i] == freq)
>  			return regmap_update_bits(st->regmap, AD4691_OSC_FREQ_REG,
>  						  AD4691_OSC_FREQ_MASK, i);
> @@ -363,7 +433,10 @@ static int ad4691_read_avail(struct iio_dev *indio_dev,
>  			     int *length, long mask)
>  {
>  	struct ad4691_state *st = iio_priv(indio_dev);
> -	unsigned int start = (st->info->max_rate == HZ_PER_MHZ) ? 0 : 1;
> +	unsigned int start;
> +
> +	/* Skip frequencies that exceed this chip's maximum rate. */
> +	start = (ad4691_osc_freqs[0] > st->info->max_rate) ? 1 : 0;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SAMP_FREQ:
> @@ -386,8 +459,7 @@ static int ad4691_single_shot_read(struct iio_dev *indio_dev,
>  	guard(mutex)(&st->lock);
>  
>  	/*
> -	 * Use AUTONOMOUS mode for single-shot reads. The chip always
> -	 * operates in AUTONOMOUS mode in this driver revision.
> +	 * Use AUTONOMOUS mode for single-shot reads.
>  	 */
>  	ret = regmap_write(st->regmap, AD4691_STATE_RESET_REG,
>  			   AD4691_STATE_RESET_ALL);
> @@ -417,8 +489,7 @@ static int ad4691_single_shot_read(struct iio_dev *indio_dev,
>  	 * conversion to complete.
>  	 */
>  	fsleep(DIV_ROUND_UP(2 * USEC_PER_SEC,
> -			    ad4691_osc_freqs[FIELD_GET(AD4691_OSC_FREQ_MASK,
> -						       reg_val)]));
> +		ad4691_osc_freqs[FIELD_GET(AD4691_OSC_FREQ_MASK, reg_val)]));
>  
>  	ret = regmap_write(st->regmap, AD4691_OSC_EN_REG, 0);
>  	if (ret)
> @@ -488,6 +559,374 @@ static int ad4691_reg_access(struct iio_dev *indio_dev, unsigned int reg,
>  	return regmap_write(st->regmap, reg, writeval);
>  }
>  
> +static int ad4691_set_pwm_freq(struct ad4691_state *st, int freq)
> +{
> +	if (!freq)
> +		return -EINVAL;
> +
> +	st->cnv_period_ns = DIV_ROUND_CLOSEST(NSEC_PER_SEC, freq);

I wrote something about divisions in an earlier revision already.
Ideally there was a iio-specific policy how to convert a frequency to a
period.

Until such a policy exists (maybe even with a helper function), let me
point out that if you want to pick the period that is nearest to 1/freq,
most of the time

	st->cnv_period_ns = DIV_ROUND_UP(NSEC_PER_SEC, freq);

is the better value to use. And in the remaining cases this is still a
very good value.

The analysis goes as follows:

Let Π be the function mapping an integer period request to the actually
implemented period (a rational number in general).

Let P := DIV_ROUND_DOWN(NSEC_PER_SEC, freq) and ε := P - Π(P).
With Π(x) ≤ x, we have ε ≥ 0.

The analysis is moot if DIV_ROUND_CLOSEST() and DIV_ROUND_UP() yield the
same value, so we can assume

	(P + ẟ) * freq = NSEC_PER_SEC

with 0 < ẟ < 0.5. The values to consider are Π(P) and Π(P+1).

The former is the better value if:

	  abs(P + ẟ - Π(P)) < abs(P + ẟ - Π(P+1))
	⟺ ẟ + ε < abs(P + ẟ - Π(P+1))

With Π(x + 1) ≥ Π(x) this can only hold if

	  Π(P+1) > P + 2ẟ + ε
	⟹ 1 > 2ẟ + ε

So we'd need that ε = P - Π(P) < 1, that is a possible period quite near
to P and another possible period value in the interval ]P + 2ẟ + ε; P + 1].

In that case Π(P+1) is a worse value than Π(P), but still

	abs(P + ẟ - Π(P+1)) < 1

, so even in this unlikely situation using P+1 is quite good.

Another advantage of DIV_ROUND_UP(NSEC_PER_SEC, freq) over
DIV_ROUND_CLOSEST(NSEC_PER_SEC, freq) is that it yields a sensible
period for freq > 2*NSEC_PER_SEC.

> +	return 0;
> +}
> +
> +static int ad4691_sampling_enable(struct ad4691_state *st, bool enable)
> +{
> +	struct pwm_state conv_state = { };
> +
> +	conv_state.period = st->cnv_period_ns;
> +	conv_state.duty_cycle = AD4691_CNV_DUTY_CYCLE_NS;
> +	conv_state.polarity = PWM_POLARITY_NORMAL;
> +	conv_state.enabled = enable;

this can be written as:

	static int ad4691_sampling_enable(struct ad4691_state *st, bool enable)
	{
		struct pwm_state conv_state = {
			.period = st->cnv_period_ns;
			.duty_cycle = AD4691_CNV_DUTY_CYCLE_NS;
			.polarity = PWM_POLARITY_NORMAL;
			.enabled = enable;
		};
		...

	}

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]