Message-ID: <yfw2zhdymhta57xr6x6dphqggtlpgbs35yzudwlxghbdi4hlnj@spicau723uai>
Date: Wed, 19 Mar 2025 17:59:19 +0100
From: Jorge Marques <gastmaier@...il.com>
To: David Lechner <dlechner@...libre.com>
Cc: Jonathan Cameron <jic23@...nel.org>, 
	Jorge Marques <jorge.marques@...log.com>, Lars-Peter Clausen <lars@...afoo.de>, 
	Michael Hennerich <Michael.Hennerich@...log.com>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Jonathan Corbet <corbet@....net>, linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH 3/4] docs: iio: new docs for ad4052 driver

On Fri, Mar 14, 2025 at 01:56:32PM -0500, David Lechner wrote:
> On 3/14/25 1:13 PM, Jorge Marques wrote:
> > On Mon, Mar 10, 2025 at 07:54:16PM +0000, Jonathan Cameron wrote:
> >> On Sun, 9 Mar 2025 21:49:24 +0100
> >> Jorge Marques <gastmaier@...il.com> wrote:
> >>
> >>>>> +.. list-table:: Driver attributes
> >>>>> +   :header-rows: 1
> >>>>> +
> >>>>> +   * - Attribute
> >>>>> +     - Description
> >>>>> +   * - ``in_voltage0_raw``
> >>>>> +     - Raw ADC voltage value
> >>>>> +   * - ``in_voltage0_oversampling_ratio``
> >>>>> +     - Enable the device's burst averaging mode to over sample using
> >>>>> +       the internal sample rate.
> >>>>> +   * - ``in_voltage0_oversampling_ratio_available``
> >>>>> +     - List of available oversampling values. Value 0 disable the burst
> >>>>> +       averaging mode.
> >>>>> +   * - ``sample_rate``
> >>>>> +     - Device internal sample rate used in the burst averaging mode.
> >>>>> +   * - ``sample_rate_available``
> >>>>> +     - List of available sample rates.  
> >>>>
> >>>> Why not using the standard sampling_frequency[_available] attributes?  
> >>> Because sampling_frequency is the sampling frequency for the pwm trigger
> >>> during buffer readings.
> >>> sample_rate is the internal device clock used during monitor and burst
> >>> averaging modes.
> >>
> >> For an ABI that is very vague and the two use cases seem to be logically
> >> quite different.
> >>
> >> Seems that for each trigger we have an oversampling ratio controlled number
> >> of samples at this rate. It is unusual to be able to control oversampling
> >> rate separately from the trigger clock, hence the lack of ABI.  If
> >> we add something new for this it should something relating to oversampling.
> >> oversampling_frequency perhaps.
> >>
> >> For monitor mode, it is tied to the sampling frequency for most devices.
> >> But there are exceptions.  E.g. the max1363. Trick is to make it an event
> >> ABI property and hence under events/ rather than in the root directory.
> >>
> >> In this case you'll have to store two values and write the appropriate
> >> one into the register to suit a given operating mode.
> >>
> > 
> > If doing buffer captures with oversampling enabled, both sampling
> > frequencies have an impact:
> > 
> > e.g.,
> > oversampling: 4
> > sample_rate: 2MHz
> > PWM sampling frequency: 500KHz
> > 
> > PWM trigger out (CNV)   |       |       |       |       |
> > ADC conversion          ++++    ++++    ++++    ++++    ++++
> > ADC data ready  (GP)       *       *       *       *       *
> > 
> > For monitor mode, it will constantly be doing conversion to check for
> > threshold crossings, at the defined sample_rate.
> > 
> > I like the idea of having the device's sample_rate as
> > conversion_frequency.
> 
> In addition to what makes sense for this chip, we should also consider what
> makes sense other chips with similar features. For example, I am working on
> ad7606c which has control for the oversampling burst frequency (frequency of
> "+" in the diagram above). So it would make sense to have a standard attribute
> that would work for both chips.
> 
> On ad4052, just because we have a single register that controls two different
> functions doesn't mean we have to be limited to a single attribute that controls
> that register.
> 

I looked into the ad7606c driver and summarized below to organize our
ideas:

  PADDING OVERSAMPLING
  --------------------
  Delay between conversions:

  OS_CLOCK(Hz) = 1 / (1+OS_PAD/16)
  
  OS_CLOCK: internal clock, reg

  0x08 OVERSAMPLING
    OS_PAD[7:4]: Extends the internal oversampling period allowing
                 evenly spaced sampling between CONVST rising edges,
                 from 0 to 15
    OS_RATIO[3:0]: from off(1) to 256
    
  Therefore, OS_CLOCK range is therefore 1Hz .. 0.516Hz
  (1) from previous discussion, iio oversampling 1 equals off.

  EXTERNAL OVERSAMPLING CLOCK
  ---------------------------
  Use CONVST as the external trigger for
  each conversion

On AD4052 family:

  BURST AVERAGING MODE
  --------------------

  Delay between conversions

  Total latency:
  (AVG_WIN_LEN-1)/FS_BURST_AUTO + t_CONV

  0x23 AVG_CONFIG
    AVG_WIN_LEN[3:0]: Averaging ratio/number of samples
  0x27 TIMER_CONFIG
    FS_BURST_AUTO[7:4]: from 111Hz to 2 MHz, internal sample rate

  AVERAGING MODE
  --------------
  Use CONVST as the external trigger for
  each conversion

So, we can say that
PADDING OVERSAMPLING == BURST AVERAGING MODE, and
EXTERNAL OVERSAMPLING CLOCK == AVERAGING MODE

> So I would create the events/sampling_frequency{,_available} attributes like
> Jonathan suggested for controlling the sampling frequency in monitor mode and
> introduce new oversampling_burst_frequency{,_available} attributes for
> controlling the conversion frequency when oversampling. When an attribute is
> written, we can cache the requested value in the state struct instead of
> writing it directly to the register on the ADC if we want the attributes to be
> independent. Then only write the register when we enable monitor mode or when
> we start reading samples with oversampling enabled.
> 
> Sure, it is more work to implement it in the driver this way, but that shouldn't
> be an an excuse to do things in a way that isn't compatible with other ADCs.
> 

I am alright with that and will follow the suggestion of having the
values independent through cache.

So, two new attributes will be implemented:

* oversampling_[burst_]frequency{,_available} (new ABI required)
* events/sampling_frequency{,_available}

And I will drop conversion_frequency (early sample_rate) attribute.

Powered by blists - more mailing lists