Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 8 Apr 2023 11:30:29 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     Matti Vaittinen <mazziesaccount@gmail.com>
Cc:     Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Andrea Merello <andrea.merello@iit.it>,
        Andy Shevchenko <andy.shevchenko@gmail.com>,
        Jagath Jog J <jagathjog1996@gmail.com>,
        linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/3] iio: add documentation for iio_chan_info_enum
Message-ID: <20230408113029.35429440@jic23-huawei>
In-Reply-To: <884c8f386541ac572939b8993df7aea6ad99613b.1677331779.git.mazziesaccount@gmail.com>
References: <cover.1677331779.git.mazziesaccount@gmail.com>
        <884c8f386541ac572939b8993df7aea6ad99613b.1677331779.git.mazziesaccount@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Sat, 25 Feb 2023 15:55:25 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Values in the iio_chan_info_enum are crucial for understanding the
> characteristics of an IIO channel and the data delivered via IIO channel.
> Give a hand to developers who do their first set of IIO drivers.
> 
> Add some documentation to these channel specifiers.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> ---
> Please note that I did only add documentation for entries I am familiar
> with. I did still add doc placeholders for all of the enum entries to
> ease seeing which entries could still be documented. Hopefully this
> encourages people to add missing pieces of documentation.

Good to hear the optimism :) 

I'll add it to my activities for boring journeys (with good internet
as probably need datasheets).  Note I'm reviewing this on a train
(having ignored it for a few weeks ;)

> ---
>  include/linux/iio/types.h | 46 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
> index 82faa98c719a..c8e3288ca24b 100644
> --- a/include/linux/iio/types.h
> +++ b/include/linux/iio/types.h
> @@ -35,7 +35,51 @@ enum iio_available_type {
>  	IIO_AVAIL_LIST,
>  	IIO_AVAIL_RANGE,
>  };
> -
> +/**
> + * enum iio_chan_info_enum - Information related to a IIO channel
> + *
> + * Many IIO channels have extra properties. Typically these properties can be
"extra" glosses over the fact that some of these almost always exist.
E.g. raw.

IIO channels have a range of properties that may be read from userspace
(via sysfs attributes) or from other drivers using the in kernel IIO consumer
interfaces.  These properties are read / written using the read_raw...


> + * read / written by user using the read_raw or write_raw callbacks in the
> + * struct iio_info.
> + *
> + * @IIO_CHAN_INFO_RAW:		Raw channel data as provided by device. Scale
> + *				and offset are often required to convert these
> + *				values to meaningful units.

to base units as defined in the IIO ABI (link)

> + * @IIO_CHAN_INFO_PROCESSED:	Processed data. Typically driver performs
> + *				computations to convert device data to more
> + *				meaningfull processed values.

Typically a driver performs computations to convert device data to the 
base units defined in the IIO ABI (link)

> + * @IIO_CHAN_INFO_SCALE:	Scale to be applied to data in order to convert
> + *				it to units mandated by the channel type.
> + * @IIO_CHAN_INFO_OFFSET:	Offset to be applied to data in order to convert
> + *				it to units mandated by the channel type.

Add ordering info.  "Applied before scale."

> + * @IIO_CHAN_INFO_CALIBSCALE:
> + * @IIO_CHAN_INFO_CALIBBIAS:
> + * @IIO_CHAN_INFO_PEAK:		Peak value (TODO: Since measurement start?)

IIRC not that consistent. Some devices have it from device reset (so start), some
do it on a short time scale (thing of a voltage channel measuring a sine wave -
instantaneous reading is the current voltage, peak can be the peak in the cycle).
Others again do it on an 'event detection basis'. 
Sometimes constructive ambiguity can be handy in documentation ;)

> + * @IIO_CHAN_INFO_PEAK_SCALE:	Scale to be applied to the peak value in order
> + *				to convert it to units mandated by the channel
> + *				type.
> + * @IIO_CHAN_INFO_QUADRATURE_CORRECTION_RAW:
> + * @IIO_CHAN_INFO_AVERAGE_RAW:	Average of raw values (TODO: Since measurement
> + *				start or just for some undefined time?)

Again, not that tightly defined (IIRC).  Average of raw values over a device specific time period.

> + * @IIO_CHAN_INFO_SAMP_FREQ:	Sampling frequency for device.
> + * @IIO_CHAN_INFO_FREQUENCY:
> + * @IIO_CHAN_INFO_PHASE:
> + * @IIO_CHAN_INFO_HARDWAREGAIN:	Amplification applied by the hardware.
Given how often this is done wrong I'd love to call out something like:
"SCALE should be used for control if the HARDWAREGAIN directly affects the
 channel RAW measurement". Examples of HARDWAREGAIN include amplification of
 the light signal in a time of flight sensor."


> + * @IIO_CHAN_INFO_HYSTERESIS:
> + * @IIO_CHAN_INFO_HYSTERESIS_RELATIVE:
> + * @IIO_CHAN_INFO_INT_TIME:	Integration time. Time during which the data is
> + *				accumulated by the device.

Unit? (seconds I think).

> + * @IIO_CHAN_INFO_ENABLE:
> + * @IIO_CHAN_INFO_CALIBHEIGHT:
> + * @IIO_CHAN_INFO_CALIBWEIGHT:
> + * @IIO_CHAN_INFO_DEBOUNCE_COUNT:
> + * @IIO_CHAN_INFO_DEBOUNCE_TIME:
> + * @IIO_CHAN_INFO_CALIBEMISSIVITY:
> + * @IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> + * @IIO_CHAN_INFO_THERMOCOUPLE_TYPE:
> + * @IIO_CHAN_INFO_CALIBAMBIENT:
> + * @IIO_CHAN_INFO_ZEROPOINT:
> + */
>  enum iio_chan_info_enum {
>  	IIO_CHAN_INFO_RAW = 0,
>  	IIO_CHAN_INFO_PROCESSED,