Message-ID: <20241208180457.11c01806@jic23-huawei>
Date: Sun, 8 Dec 2024 18:04:57 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Marcelo Schmitt <marcelo.schmitt1@...il.com>
Cc: David Lechner <dlechner@...libre.com>, Marcelo Schmitt
 <marcelo.schmitt@...log.com>, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org, lars@...afoo.de, Guillaume Ranquet
 <granquet@...libre.com>, Uwe Kleine-König
 <u.kleine-koenig@...libre.com>
Subject: Re: [PATCH 1/1] Documentation: ABI: IIO: Re-add
 filter_type/filter_mode

On Tue, 3 Dec 2024 09:19:30 -0300
Marcelo Schmitt <marcelo.schmitt1@...il.com> wrote:

> On 12/02, David Lechner wrote:
> > On 12/2/24 12:22 PM, Marcelo Schmitt wrote:  
> > > The ad4130 driver exports in_voltageY-voltageZ_filter_mode and
> > > in_voltage-voltage_filter_mode_available attributes to user space.
> > > The ad7779 driver exports filter_type and filter_type_available.
> > > Add (back again) documentation for filter_type/filter_mode attributes.
> > > 
> > > Fixes: 01bb12922b60 ("Documentation: ABI: added filter mode doc in sysfs-bus-iio")
> > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@...log.com>
> > > ---
> > > Digressing a bit away from the specific ABI used by ad4130 and ad7779,
> > > the sinc3/4/5 filters are called `filter_mode` in ad4130 driver while other
> > > drivers (ad7779, ad7124, ad7768-1) call sinc3/4/5 filters a `filter_type`.
> > > Datasheets use the term `filter type`.
> > > 
> > > Depending on the particular ADC chip/design, the sinc3/4/5 filter configuration
> > > may have an impact on the output data rate (ODR) (which is equivalent to the
> > > sampling frequency for SAR ADCs - `sampling_frequency` ABI), 3dB cutoff
> > > frequency of the filter (`_low_pass_3db_frequency` attributes), or settling
> > > time.
> > > 
> > > ad7768-1 sets sinc3/4/5 according to the sampling_frequency attribute. No
> > > filter_type attribute.
> > > 
> > > ad7173 sets the filter_type according to sampling_frequency too, though it
> > > looks like support for only one filter type is implemented.
> > > 
> > > ad7124 sets sinc3/4/5 filters according to a filter_low_pass_3db_frequency
> > > attribute so it doesn't export filter type attributes to user space.
> > > Missing `in_voltageY-voltageZ_filter_low_pass_3db_frequency` documentation?
> > > follow up patch?  
> > 
> > cc: Guillaume and Uwe since they are working on these last two drivers
> > currently. Maybe something they could address?
> >   
> > > 
> > >  Documentation/ABI/testing/sysfs-bus-iio | 14 ++++++++++++++
> > >  1 file changed, 14 insertions(+)
> > > 
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> > > index f83bd6829285..704c9033cb5b 100644
> > > --- a/Documentation/ABI/testing/sysfs-bus-iio
> > > +++ b/Documentation/ABI/testing/sysfs-bus-iio
> > > @@ -2268,6 +2268,20 @@ Description:
> > >  		An example format is 16-bytes, 2-digits-per-byte, HEX-string
> > >  		representing the sensor unique ID number.
> > >  
> > > +What:		/sys/bus/iio/devices/iio:deviceX/filter_type
> > > +What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY-voltageZ_filter_mode
> > > +voltageY_filter_type_available
> > > +KernelVersion:	6.1
> > > +Contact:	linux-iio@...r.kernel.org
> > > +Description:
> > > +		Set the filter mode of the channel. When the filter mode
> > > +		changes, the in_voltageY-voltageZ_sampling_frequency and
> > > +		in_voltageY-voltageZ_sampling_frequency_available attributes
> > > +		might also change to accommodate the new filter mode.
> > > +		If the current sampling frequency is out of range for the new
> > > +		filter mode, the sampling frequency will be changed to the
> > > +		closest valid one.  
> > 
> > I think it can be safely assumed that changing any IIO attribute can
> > cause any other to change, so we probably don't need to mention the
> > sampling frequency interaction here, especially since it doesn't apply
> > to every possible user of these attributes.  
> 
> Besides these filter attributes, the _offset attribute was allowed to change
> after a change in _scale for a different driver so I'm also thinking IIO
> attribute changes are allowed to cause updates to other device attributes.

They are.  There is no practical way to design an ABI where that doesn't
happen unfortunately. We try to keep the changes as intuitive as possible
(so don't change unless we have to, find nearest value etc) where it is
reasonably easy to do.


> The description above is roughly the same that was removed in 01bb12922b60.
> Can think of something more accurate if that would be appreciated.
> 
> Jonathan, let me know if you prefer to re-add ABI doc as it was or if we
> can re-add an updated version of it.
I'm fine with updating in the same patch.

> 
> > 
> > Some other useful things to add instead:
> > * Mention that the values are the same as the ones listed in the
> >   "..._available" attribute docs.  
> Sure, will do if going to update the ABI description.
> 
> > * We should deprecate one of the names and recommend the other for
> >   future drivers to use. Since "type" is used more than once and
> >   "mode" only once, it seems natural to keep using "type" going
> >   forward.  
> Agree.

If mode is only used once, perhaps move it to a driver specific file?
Fine to cross reference to the main file from there.

Jonathan

> 
> >   
> > > +
> > >  What:		/sys/bus/iio/devices/iio:deviceX/filter_type_available
> > >  What:		/sys/bus/iio/devices/iio:deviceX/in_voltage-voltage_filter_mode_available
> > >  KernelVersion:	6.1
> > >   

Powered by blists - more mailing lists