Thread (9 messages) 9 messages, 3 authors, 2026-03-13

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

From: Andy Shevchenko <hidden>
Date: 2026-03-11 21:08:11
Also in: linux-gpio, linux-iio, linux-pwm, lkml

On Tue, Mar 10, 2026 at 04:32:24PM +0200, Radu Sabau via B4 Relay wrote:
Add buffered capture support using the IIO triggered buffer framework.

Both operating modes share a single IIO trigger and trigger handler.
The handler builds a complete scan — one u32 slot per channel at its
scan_index position, followed by a timestamp — and pushes it to the
IIO buffer in a single iio_push_to_buffers_with_ts() call.

For CNV Clock Mode the GP0 pin is configured as DATA_READY output. The
IRQ handler stops conversions and fires the IIO trigger; the trigger
handler reads accumulated results from the AVG_IN registers via regmap
and restarts conversions for the next cycle.

For Manual Mode there is no DATA_READY signal; CNV is tied to SPI CS
so conversions are triggered by CS assertion rather than by a dedicated
pin. The standard iio-trig-hrtimer module is not used because the timer
period must be derived from the SPI clock rate and the number of active
channels: the pipelined protocol requires N+1 SPI transfers per scan
(the first result is garbage and is discarded), so the minimum period
depends on both the SPI speed and the live channel count at buffer
enable time. A driver-private hrtimer whose period is recomputed by
buffer_postenable is simpler and avoids requiring the user to configure
an external trigger with the correct hardware-derived period.

Manual mode channels use storagebits=32 (shift=8, realbits=16) so all
channel slots in the scan buffer are uniformly sized regardless of the
SPI wire format (24-bit transfer, 16-bit ADC data in bits[23:8]).
Many comments from previous patch are applicable here.

...
+static irqreturn_t ad4691_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct ad4691_state *st = iio_priv(indio_dev);
+	unsigned int val;
+	int ret, i;
Why is 'i' signed?
+	mutex_lock(&st->lock);
No guard()()?
+	if (st->adc_mode == AD4691_MANUAL_MODE) {
+		unsigned int prev_val;
+		int prev_chan = -1;
+
+		/*
+		 * MANUAL_MODE with CNV tied to CS: each transfer triggers a
+		 * conversion AND returns the previous conversion's result.
+		 * First transfer returns garbage, so we do N+1 transfers for
+		 * N channels. Collect all results into scan.vals[], then push
+		 * the complete scan once.
+		 */
+		iio_for_each_active_channel(indio_dev, i) {
+			ret = ad4691_transfer(st, AD4691_ADC_CHAN(i), &val);
+			if (ret)
+				goto done;
+
+			if (prev_chan >= 0)
+				st->scan.vals[prev_chan] = prev_val;
+			prev_val = val;
+			prev_chan = i;
+		}
+
+		/* Final NOOP transfer to retrieve last channel's result */
+		ret = ad4691_transfer(st, AD4691_NOOP, &val);
+		if (ret)
+			goto done;
+
+		st->scan.vals[prev_chan] = val;
+	} else {
+		for (i = 0; i < st->chip->num_channels; i++) {
+			if (BIT(i) & *indio_dev->active_scan_mask) {
NIH for_each_set_bit().
+				ret = regmap_read(st->regmap, AD4691_AVG_IN(i), &val);
+				if (ret)
+					goto done;
+
+				st->scan.vals[i] = val;
+			}
+		}
+
+		regmap_write(st->regmap, AD4691_STATE_RESET_REG, AD4691_STATE_RESET_ALL);
+
+		/* Restart conversions for the next trigger cycle. */
+		ad4691_sampling_enable(st, true);
+	}
+
+	iio_push_to_buffers_with_ts(indio_dev, &st->scan, sizeof(st->scan),
+				    pf->timestamp);
+
+done:
+	iio_trigger_notify_done(indio_dev->trig);
+	mutex_unlock(&st->lock);
+	return IRQ_HANDLED;
+}
...
+	st->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
+					  indio_dev->name,
+					  iio_device_id(indio_dev));
+	if (!st->trig)
+		return dev_err_probe(dev, -ENOMEM,
+				     "Failed to allocate IIO trigger\n");
No. Ask your senior colleagues why.

...
+		irq = fwnode_irq_get(dev_fwnode(dev), 0);
+		if (irq <= 0)
' = 0' ?!
+			return dev_err_probe(dev, irq ? irq : -ENOENT,
+					     "failed to get DATA_READY interrupt\n");
This ugly ternary will gone.

...
+		ret = devm_request_threaded_irq(dev, irq, NULL,
+						&ad4691_irq,
+						IRQF_ONESHOT,
+						indio_dev->name, indio_dev);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "request irq %d failed\n", irq);
Also no. Similar reason as above.

-- 
With Best Regards,
Andy Shevchenko

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help