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

[Sabau, Radu bogdan]

> -----Original Message-----
> From: David Lechner <dlechner@xxxxxxxxxxxx>
> Sent: Monday, March 16, 2026 6:45 PM
> To: Sabau, Radu bogdan <Radu.Sabau@xxxxxxxxxx>; Lars-Peter Clausen
> <lars@xxxxxxxxxx>; Hennerich, Michael <Michael.Hennerich@xxxxxxxxxx>;
> Jonathan Cameron <jic23@xxxxxxxxxx>; Sa, Nuno <Nuno.Sa@xxxxxxxxxx>;
> Andy Shevchenko <andy@xxxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>;
> Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>; Conor Dooley
> <conor+dt@xxxxxxxxxx>; Uwe Kleine-König <ukleinek@xxxxxxxxxx>; Liam
> Girdwood <lgirdwood@xxxxxxxxx>; Mark Brown <broonie@xxxxxxxxxx>; Linus
> Walleij <linusw@xxxxxxxxxx>; Bartosz Golaszewski <brgl@xxxxxxxxxx>; Philipp
> Zabel <p.zabel@xxxxxxxxxxxxxx>
> Cc: linux-iio@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; linux-pwm@xxxxxxxxxxxxxxx; linux-
> gpio@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v3 3/4] iio: adc: ad4691: add triggered buffer support
> 
> [External]
> 
> On 3/16/26 10:56 AM, Sabau, Radu bogdan wrote:
> >
> >
> >> -----Original Message-----
> >> From: David Lechner <dlechner@xxxxxxxxxxxx>
> >> Sent: Monday, March 16, 2026 5:38 PM
> >> To: Sabau, Radu bogdan <Radu.Sabau@xxxxxxxxxx>; Lars-Peter Clausen
> >> <lars@xxxxxxxxxx>; Hennerich, Michael
> <Michael.Hennerich@xxxxxxxxxx>;
> >> Jonathan Cameron <jic23@xxxxxxxxxx>; Sa, Nuno <Nuno.Sa@xxxxxxxxxx>;
> >> Andy Shevchenko <andy@xxxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>;
> >> Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>; Conor Dooley
> >> <conor+dt@xxxxxxxxxx>; Uwe Kleine-König <ukleinek@xxxxxxxxxx>; Liam
> >> Girdwood <lgirdwood@xxxxxxxxx>; Mark Brown <broonie@xxxxxxxxxx>;
> Linus
> >> Walleij <linusw@xxxxxxxxxx>; Bartosz Golaszewski <brgl@xxxxxxxxxx>;
> Philipp
> >> Zabel <p.zabel@xxxxxxxxxxxxxx>
> >> Cc: linux-iio@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
> >> kernel@xxxxxxxxxxxxxxx; linux-pwm@xxxxxxxxxxxxxxx; linux-
> >> gpio@xxxxxxxxxxxxxxx
> >> Subject: Re: [PATCH v3 3/4] iio: adc: ad4691: add triggered buffer support
> >>
> >> [External]
> >>
> >> On 3/16/26 8:22 AM, Sabau, Radu bogdan wrote:
> >>>
> >>>
> >>>> -----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.
> >>
> >> Assuming that "a" channel means "one" channel...
> >>
> >> In this case then sampling_frequency should be per channel (separate).
> >>
> >> A sampling_frequency that is shared_by_all means that each period of
> >> CNV should trigger one sample each for _all_ channels. In other words,
> >> the sampling frequency gives one complete set of samples for all enabled
> >> channels pushed to the buffer.
> >>
> >
> > Oh, ok then, will have them separate. I assumed that since the PWM period
> > is constant with each pulse, then the sampling rate will be the same for
> > each channel, thus having them as shared_by_all, but I assume you are
> > right about this in this case, I will have them as separate in this case, the
> > update will happen in the previous patch upon next version.
> >
> Does the sampling stop after one "burst" (reading each enabled channel
> once)?
> 
> If yes, then what controls when the next set of samples starts?
> 
> Looking at Figure 63 in the datasheet for CNV Clock mode, it looks like it
> depends entirely on how long the SPI message takes. So the actual sample rate
> is going to be quite random instead of the sum of each channel as the IIO ABI
> says it should. It seems a waste of the PWM to do it this way since we end
> up with a random sample rate.
> 
> It seems to me like the CNV Burst mode would actually be better suited to
> how IIO usually does things. In this case, the PWM frequency would control
> the effective sample rate (one PWM pulse triggers one complete set of
> samples) and the internal oscillator controls triggering each individual
> conversion.
> 
> In this setup, we would still have the info_mask_separate IIO_SAMP_FREQ,
> but it would control the internal oscillator. Then we would have a separate
> buffer0/sampling_frequency attribute that controlled the PWM frequency.
> 
> Then, as long as the PWM frequency was slow enough that the SPI message
> can be done, it can make samples with almost no jitter. This is why I would
> expect PWM to almost always be used with SPI offload though, otherwise
> it has to be quite slow compared to what the chip is capable of.
> 
> I suppose the CNV Clock mode could also be made to work with the typical
> IIO trigger so that we could control the actual sample rate. It just
> wouldn't be as precise.
> 
> 
> 
> If you have some examples of how this chip should actually be used in the
> real world, that could help pick what is the right thing to do here.

You are indeed right. For CNV Clock Mode, the actual sample rate is 
random and what is exposed as 'sample rate' is not actually according
to what the IIO ABI says, nor respecting to what already exists in the
kernel. I connected a Logic Analyzer and the GP0 interrupt is not actually
constant in this case, but for CNV Burst Mode it is (I have used first version
driver from this series).

Also, I think your suggestion that the sampling frequency of the channels
should refer to the internal oscillator and the sampling frequency of the
buffer to be the actual PWM frequency of CNV is a very good one.
This way CNV Burst Mode complies with the ABI and works as expected
with both typical IIO trigger and offload mode. More than this setting
the ACC_COUNT to 1 (as it is right now) also avoids oversampling although
CNV Burst is used, so I think switching from CNV Clock to CNV Burst would
be a really good mode. Thanks for this!

I will address the rest of the comments and also change the CNV Clock Mode
for CNV Burst Mode.