Message-ID: <20250315182402.59f3ccf4@jic23-huawei>
Date: Sat, 15 Mar 2025 18:24:02 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Jorge Marques <gastmaier@...il.com>
Cc: David Lechner <dlechner@...libre.com>, 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, 14 Mar 2025 18:34:46 +0100
Jorge Marques <gastmaier@...il.com> wrote:

> On Mon, Mar 10, 2025 at 07:56:29PM +0000, Jonathan Cameron wrote:
> > On Mon, 10 Mar 2025 09:31:45 -0500
> > David Lechner <dlechner@...libre.com> wrote:
> >   
> > > On 3/9/25 3:49 PM, Jorge Marques wrote:  
> > > >>> +   * - ``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.    
> > > 
> > > I haven't done a chips with a monitor mode yet where we aren't reading
> > > the samples, so hopefully Jonathan will chime in here on the usual way
> > > to handle that.
> > > 
> > > For the burst averaging mode, I understand the need for a separate attribute
> > > now. I would suggest to call this the conversion_frequency rather than
> > > sampling_rate since IIO already defines "sampling" to be the data read
> > > from the chip to Linux even if it is an averaged value, it still counts
> > > as one sample.  
> > 
> > I should have read on.  I'd like this more closely associated with oversampling.
> > As per other reply we use sampling_frequency in the events directory for
> > the monitoring frequency case.  One of our very first drivers did this
> > (max1363) so it's been in the ABI a long time!
> >   
> 
> I get the idea but maybe the datasheet sample rate as conversion_frequency
> and stored as a channel attribute (iio_chan_spec.ext_info) is clear enough.

It's not standard ABI. So no standard userspace code will be aware of it.
we can add to standard ABI, but only if we can't support a feature with
what is already defined. Event then we need to keep it consistent with
existing ABI.

Note that the first step on any ABI is to write documentation for it in 
Documentation/ABI/testing/sysfs-bus-iio-*
That can help people understand what is being proposed and allows discussion
of how to generalize any new ABI to be useful across many drivers.
> 
> The datasheet sample rate affects both the burst averaging mode (oversampling) and
> monitor mode (threshold events).

True, but are both occurring at the same time?  My reading of the situation was
that they weren't but I could be wrong.  If they aren't then just rewrite the register
to a cached value when you change mode.

> 
> The max1363 stores as an event attribute (iio_info.event_attr) and requires iio/sysfs.h include.
> A last option is to store as a general purpose device attribute (iio_info.attrs).
> As a channel attribute, the driver logic is slightly simpler by using the macros.

Agreed that non standard event attributes are a bit trickier to deal with.  They
have never been common enough for us to fix that.  However the ABI exists
and is documented, so that's almost certainly the way to go.

Jonathan
> 
> Jorge

Powered by blists - more mailing lists