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

[Andy Shevchenko]

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