[v2,2/3] iio: add documentation for iio_chan_info_enum

  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.
---
 include/linux/iio/types.h | 46 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)
  

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,
  

On 4/8/23 13:30, Jonathan Cameron wrote:
> 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)

This is just great. I like adding the link to ABI doc here! Thanks!

>> + * @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."

So, let's try to tell people that HARDWAREGAIN is typically not the 
thing they are interested in :) I think this is exactly the way these 
docs should help both the driver authors as well as the poor sod 
reviewing all the driver patches ;)

>> + * @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).

Really...? Can you please double check this?

I believe the BU27034 uses micro seconds. I thought that was correct 
approach - but if it is not then we can probably still change it w/o 
breaking userland...


Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~
  

On 4/8/23 13:30, Jonathan Cameron wrote:
> On Sat, 25 Feb 2023 15:55:25 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> + * @IIO_CHAN_INFO_INT_TIME:	Integration time. Time during which the data is
>> + *				accumulated by the device.
> 
> Unit? (seconds I think).

Holy moly. Thanks for bringing this up now. I just checked this and the 
API doc indeed says clearly and loud the unit is in seconds. This means 
the BU27034 driver as well as the gain-time-scale helper does this 
wrong. I hope you can postpone sending them upstream until this gets 
fixed. I'll try to cook incremental patch on top of the iio/togreg - but 
I am not sure if I can do it today as I need to run after an hour... 
Sorry and thanks!

Yours,
	-- Matti
  

> >> + * @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).  
> 
> Really...? Can you please double check this?
> 
> I believe the BU27034 uses micro seconds. I thought that was correct 
> approach - but if it is not then we can probably still change it w/o 
> breaking userland...

It's seconds in the ABI docs.  Thankfully time to fix the
driver.  No huge rush as long as we do it in time to slip in before
release.

In the early days of IIO we got most of our scaling from hwmon.
It became clear after a while that those weren't appropriate because
they tended to not have enough precision.  Hence things shifted (with
a few exceptions) to the SI unit without mili etc. I think the time
related ones all came after that change of approach.

Thanks,

Jonathan


> 
> 
> Yours,
> 	-- Matti
>
  

On Wed, 12 Apr 2023 11:52:05 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 4/8/23 13:30, Jonathan Cameron wrote:
> > On Sat, 25 Feb 2023 15:55:25 +0200
> > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >   
> >> + * @IIO_CHAN_INFO_INT_TIME:	Integration time. Time during which the data is
> >> + *				accumulated by the device.  
> > 
> > Unit? (seconds I think).  
> 
> Holy moly. Thanks for bringing this up now. I just checked this and the 
> API doc indeed says clearly and loud the unit is in seconds. This means 
> the BU27034 driver as well as the gain-time-scale helper does this 
> wrong. I hope you can postpone sending them upstream until this gets 
> fixed. I'll try to cook incremental patch on top of the iio/togreg - but 
> I am not sure if I can do it today as I need to run after an hour... 
> Sorry and thanks!

It's a fix so we have time.  This one we'd take even if the driver
had made it to a release, but then we'd manage to annoy people.

Jonathan

> 
> Yours,
> 	-- Matti
>