RE: [PATCH v3 3/4] iio: adc: ad4691: add triggered buffer support

[Sabau, Radu bogdan]

> -----Original Message-----
> From: David Lechner <dlechner@xxxxxxxxxxxx>
> Sent: Saturday, March 14, 2026 8:38 PM

...

> > 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.
> 
> It would really help here to see some timing diagrams to know if we
> are implementing this right.
> 
> For example, it isn't clear that in clocked mode if CNV triggers a
> single conversion in the sequencer (i.e. IIO_SAMP_FREQ should be
> info_mask_separate) or if it triggers the sequence (i.e. IIO_SAMP_FREQ
> should be info_mask_shared_by_all).
> 

The CNV triggers the sequence and IIO_SAMP_FREQ is info_mask_shared_by_all.

As per datasheet page 31 (Accumulator Section), when each accumulator
receives a sample, the ACC_COUNT is increased. In clocked mode we
are setting the ACC_COUNT limit to 1, therefore having one sample per
channel (no oversampling as discussed in previous versions). So each
period of the CNV PWM is respective to one sample of a channel.

> >
> > 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.
> 
> This seems OK, but I would kind of would expect that PWM as CNV to
> only be used for SPI offloading and not without SPI offloading.
> 
> The ADC also has an internal oscillator, so it seems like it would
> be more useful to use that as a conversion trigger rather than
> requiring external hardware.
> 

This CNV is used in triggered buffer mode as well, not only in offload.
In this mode, CNV replaces the internal oscillator so CNV is the
conversion trigger (offload or not), which also introduces the advantage
of having a more flexible sampling rate.

> >
> > 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.
> 
> I'm not really following the argument here. It is quite normal that if
> an hrtimer trigger is set too fast then samples will be missed. So I don't
> see why we wouldn't be able to use it here. This is why we usually have
> a timestamp channel, so we can know roughly when the conversion actually
> took place.
> 

My bad here in this case. I thought no samples were wanted missing in a case
like this. I will try and use iio-trig-hrtimer on the next version.

> >
> > 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]).
> 
> I also don't understand why we are including the status bits in manual
> mode but not in CNV clock mode.
> 

In Manual Mode, status bits are received through SPI, because that's how
the hardware works. However, they are masked by the driver and thus not used.

> >
> > Signed-off-by: Radu Sabau <radu.sabau@xxxxxxxxxx>
> > ---
> >  drivers/iio/adc/Kconfig  |   2 +

...

> > +
> > +		/*
> > +		 * 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);
> 
> It would be more efficient to set up a single SPI message (in buffer enable
> callback) that reads all channels at once rather than doing multiple SPI
> messages.
> 

Similar to what offload does, of course. Thanks for this.

> > +			if (ret)
> > +				goto done;