Message-ID: <20200425202057.5c8ad612@archlinux>
Date:   Sat, 25 Apr 2020 20:20:57 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Tomasz Duszynski <tomasz.duszynski@...akon.com>
Cc:     Peter Meerwald-Stadler <pmeerw@...erw.net>,
        <linux-iio@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <robh+dt@...nel.org>
Subject: Re: [PATCH 4/6] Documentation: ABI: testing: scd30: document iio
 attributes

On Thu, 23 Apr 2020 17:53:17 +0200
Tomasz Duszynski <tomasz.duszynski@...akon.com> wrote:

> On Wed, Apr 22, 2020 at 06:40:17PM +0200, Peter Meerwald-Stadler wrote:
> > On Wed, 22 Apr 2020, Tomasz Duszynski wrote:
> >  
> > > Add documentation for sensor specific iio attributes.  
> >
> > minor comments below  
> 
> Thanks.
> 
> >  
> > > Signed-off-by: Tomasz Duszynski <tomasz.duszynski@...akon.com>
> > > ---
> > >  Documentation/ABI/testing/sysfs-bus-iio-scd30 | 97 +++++++++++++++++++
> > >  1 file changed, 97 insertions(+)
> > >  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-scd30
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-scd30 b/Documentation/ABI/testing/sysfs-bus-iio-scd30
> > > new file mode 100644
> > > index 000000000000..0431a718447d
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-scd30
> > > @@ -0,0 +1,97 @@
> > > +What:		/sys/bus/iio/devices/iio:deviceX/pressure_comp
> > > +Date:		April 2020
> > > +KernelVersion:	5.8
> > > +Contact:	linux-iio@...r.kernel.org
> > > +Description:
> > > +		Given that sensor's CO2 measurement chamber has fixed volume
> > > +		pressure changes will affect concentration readings. Writing
> > > +		current ambient pressure here will allow senor to make necessary  
> >
> > sensor
> >  
> 
> Okay.
> 
> > > +		adjustments. Upon reading previously set value is returned.
> > > +		Units are millibars.  
> >
> > unit for pressure in IIO is kilopascal (e.g.
> > /sys/bus/iio/devices/iio:deviceX/in_pressure_raw)
> >  
> 
> My thinking here was that since these are sensor specific attributes
> they don't need to stick to iio conventions and millibars were somewhat
> more natural to use. But I guess that's just matter of habit.

You absolutely have to stick to standard units.  Userspace programs
aren't going to come read your docs...

For other sensors that take a calibration value like this we've reported
them via an output channel.  For example the atlas-ph sensor has
an 'output temp' channel used for this purpose.

It's not ideal or totally intuitive but it does let us avoid expanding
the overall ABI.  The argument was something along the lines of
1) Imagine your sensor could control the pressure in the measurement space...
2) An output channel would provide the value to set it to.
3) Now instead we provide a means of saying 'what it is'
4) End result is we write a value and the pressure in the chamber is
   that value :)

As I said not ideal but the best we can do without having to define a lot
of ABI just to deal with compensation factors.

This is a rare case where I would document the 'standard' ABI in here
to make the point that it is actually providing an estimate of the pressure
not controlling it...

> 
> So generally I am okay with reworking all attrs to accept values in iio
> preferred units.
> 
> > > +
> > > +What:		/sys/bus/iio/devices/iio:deviceX/pressure_comp_available
> > > +Date:		April 2020
> > > +KernelVersion:	5.8
> > > +Contact:	linux-iio@...r.kernel.org
> > > +Description:
> > > +		The range of available values in millibars represented as the
> > > +		minimum value, the step and the maximum value, all enclosed in
> > > +		square brackets.
> > > +
> > > +What:		/sys/bus/iio/devices/iio:deviceX/meas_interval
> > > +Date:		January 2020
> > > +KernelVersion:	5.8
> > > +Contact:	linux-iio@...r.kernel.org
> > > +Description:
> > > +		Amount of time between subsequent measurements. Writing this
> > > +		attribute will change measurement interval. Upon reading
> > > +		current measurement interval is returned. Units are seconds.

Use the existing ABI sampling frequency which is sort of the inverse of this.

> > > +
> > > +What:		/sys/bus/iio/devices/iio:deviceX/meas_interval_available
> > > +Date:		April 2020
> > > +KernelVersion:	5.8
> > > +Contact:	linux-iio@...r.kernel.org
> > > +Description:
> > > +		The range of available values in seconds represented as the
> > > +		minimum value, the step and the maximum value, all enclosed in
> > > +		square brackets.
> > > +
> > > +What:		/sys/bus/iio/devices/iio:deviceX/asc
Spends some characters to easy of understanding ;)

auto_calib_proc_enable maybe?  Or can we get away with the 'somewhat standard
calibration (it's used in at least one other driver IIRC)

> > > +Date:		April 2020
> > > +KernelVersion:	5.8
> > > +Contact:	linux-iio@...r.kernel.org
> > > +Description:
> > > +		Writing 1 or 0 to this attribute will respectively activate or
> > > +		deactivate automatic self calibration procedure. Upon reading 1  
> >
> > deactivate automatic self calibration (asc) procedure
> >  
> 
> That shouldn't be too difficult to realize what asc actually stands for after
> reading this short description.
> 
> > > +		is returned if asc is ongoing, 0 otherwise.
> > > +
> > > +What:		/sys/bus/iio/devices/iio:deviceX/frc
> > > +Date:		April 2020
> > > +KernelVersion:	5.8
> > > +Contact:	linux-iio@...r.kernel.org
> > > +Description:
> > > +		Forced recalibration is used to compensate for sensor drifts
> > > +		when a reference value of CO2 concentration in close proximity
> > > +		to the sensor is available. Writing attribute will set frc
> > > +		value. Upon reading current frc is returned. Units are
> > > +		millibars.

Could we implement this by just writing to the main channel value?
Bit of a clunky ABI but sort of logically fits in my head given we are basically
forcing the value we read to be this one?

> > > +
> > > +What:		/sys/bus/iio/devices/iio:deviceX/frc_available
> > > +Date:		April 2020
> > > +KernelVersion:	5.8
> > > +Contact:	linux-iio@...r.kernel.org
> > > +Description:
> > > +		The range of available values in millibars represented as the
> > > +		minimum value, the step and the maximum value, all enclosed in
> > > +		square brackets.
> > > +
> > > +What:		/sys/bus/iio/devices/iio:deviceX/temp_offset
> > > +Date:		April 2020
> > > +KernelVersion:	5.8
> > > +Contact:	linux-iio@...r.kernel.org
> > > +Description:
> > > +		Sensor readings may be affected by ambient temperature.
> > > +		Writing temperature offset will compensate for unwanted changes.
> > > +		Note that written offset gets multiplied by a factor of 100
> > > +		by a sensor internally.
> > > +
> > > +		For example, writing 10 here will correspond to 0.1 degree
> > > +		Celsius.

This sounds like a calibbias to me which is standard ABI.

> > > +
> > > +What:		/sys/bus/iio/devices/iio:deviceX/temp_offset_available
> > > +Date:		April 2020
> > > +KernelVersion:	5.8
> > > +Contact:	linux-iio@...r.kernel.org
> > > +Description:
> > > +		The range of available values in degrees Celsius represented as
> > > +		the minimum value, the step and the maximum value, all enclosed
> > > +		in square brackets.

Wrong units for temperature (which is an odd one as we
lifted them from hwmon before learning the error of our ways and starting to use
SI units as the base).


> > > +
> > > +What:		/sys/bus/iio/devices/iio:deviceX/reset
> > > +Date:		April 2020
> > > +KernelVersion:	5.8
> > > +Contact:	linux-iio@...r.kernel.org
> > > +Description:
> > > +		Software reset mechanism forces sensor into the same state
> > > +		as after powering up without the need for removing power supply.
> > > +		Writing any value will reset sensor.

Not seeing an argument here for why you might want to do that other than on
power up or module probe to get the driver into a known state.
So currently it's a no to this one - just don't expose it to userspace.


> > >  
> >
> > --
> >
> > Peter Meerwald-Stadler
> > Mobile: +43 664 24 44 418  

Powered by blists - more mailing lists