Re: [RFC PATCH] iio: Add some kerneldoc for channel types

Re: [RFC PATCH] iio: Add some kerneldoc for channel types

[Jonathan Cameron]

On Fri, 24 Feb 2023 15:02:32 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> For occasional contributor like me navigating the IIO channel types and
> modifiers may be a daunting task. One may have hard time finding out
> what type of channel should be used for device data and what units the
> data should be converted.
> 
> There is a great documentation for the sysfs interfaces though. What is
> missing is mapping of the channel types and modifiers to the sysfs
> documentation (and entries in documentation).
> 
> Give a hand to a driver writer by providing some documentation and by
> pointing to the sysfs document from the kerneldocs of respective enums.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
+CC linux-iio

> ---
> Please note that this RFC patch should not be applied as is. The docs
> have TODO comments regarding units for IIO_ELECTRICALCONDUCTIVITY,
> IIO_PHASE and IIO_RESISTANCE. I'll fix these TODOs, remove RFC and respin
> if anyone familiar with the values provided via sysfs could provide me the
> corret units for these channels. I am also open to any suggestions how
> to better link from enum documentation to specific entry at the IIO sysfs
> documetation.
> 
> Initial discussion about these docs can be found from:
> https://lore.kernel.org/all/0e0d45b7-e582-82b2-9bac-1f70f9dad9f7@gmail.com/
> ---
>  include/uapi/linux/iio/types.h | 140 ++++++++++++++++++++++++++++++++-
>  1 file changed, 139 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
> index c79f2f046a0b..e6329d3cc055 100644
> --- a/include/uapi/linux/iio/types.h
> +++ b/include/uapi/linux/iio/types.h
> @@ -10,7 +10,129 @@
>  
>  #ifndef _UAPI_IIO_TYPES_H_
>  #define _UAPI_IIO_TYPES_H_
> -
> +/**
> + * iio_chan_type - Type of data transferred via IIO channel.
> + *
> + * The 'main' type of data transferred via channel. Please note that most
> + * devices need to specify also a more accurate 'sub category'. See the
> + * enum iio_modifier for this. (For example, IIO_ACCEL channel often needs to
> + * specify the direction. IIO_CONCENTRATION specifies the type of substance
> + * it measures etc).
> + *
> + * Use of correct units is required but scale and offset that user must apply
> + * to channel values can be advertised.
> + *
> + * Please find the detailed documentation for reported values from the
> + * Documentation/ABI/testing/sysfs-bus-iio.
> + *
> + * IIO_ACCEL:		Acceleration, m/s^2
> + *			Doc keyword: in_accel_x_raw
> + *
> + * IIO_ACTIVITY:	Activity state. For example a pedometer signaling
> + *			jogging, walking or staying still.
> + *			Doc keyword: in_activity_still_thresh_rising_en
> + *
> + * IIO_ALTVOLTAGE:
> + *
> + * IIO_ANGL:		Angle of rotation, radians.
> + *			Doc keyword: in_angl_raw
> + *
> + * IIO_ANGL_VEL:	Angular velocity, rad/s
> + *			Doc keyword: in_anglvel_x_raw
> + *
> + * IIO_CAPACITANCE:	Capacitance, nanofarads.
> + *			Doc keyword: in_capacitanceY_raw
> + *
> + * IIO_CCT:
> + *
> + * IIO_CURRENT:		Current, milliamps
> + *			Doc keyword: in_currentY_raw
> + *
> + * IIO_CONCENTRATION:	Reading of a substance, percents. Used for example by
> + *			deviced measuring amount of CO2, O2, ethanol...
> + *			Doc keyword: in_concentration_raw
> + *
> + * IIO_COUNT:		Deprecated, please use counter subsystem.
> + *
> + * IIO_DISTANCE:	Distance in meters. Typically used to report measured
> + *			distance to an object or the distance covered by the
> + *			user
> + *			Doc keyword: in_distance_input
> + *
> + * IIO_ELECTRICALCONDUCTIVITY: electric conductivity, siemens per meter
> + *			Doc keyword: in_electricalconductivity_raw
> + *			TODO: What does "can be processed to siemens per meter"
> + *			mean? Do we have unit requirement?
> + *
> + * IIO_ENERGY:		Energy in Joules. Typically reported by a device
> + *			measuring energy burnt by the user.
> + *			Doc keyword: in_energy_input
> + *
> + * IIO_GRAVITY:		Gravity, m/s^2
> + *			Doc keyword: in_gravity_x_raw
> + *
> + * IIO_HUMIDITYRELATIVE: Relative humidity, percents
> + *			Doc keyword: in_humidityrelative_raw
> + *
> + * IIO_INCLI:		Inclination, degrees
> + *			Doc keyword: in_incli_x_raw
> + *
> + * IIO_INDEX:		Deprecated, please use Counter subsystem
> + *
> + * IIO_INTENSITY:	Unitless intensity.
> + *			Doc keyword: in_intensityY_raw
> + *
> + * IIO_LIGHT:		Visible light intensity, lux
> + *			Doc keyword: in_illuminance_raw
> + *
> + * IIO_MAGN:		Magnetic field, Gauss.
> + *			Doc keyword: in_magn_x_raw
> + *
> + * IIO_MASSCONCENTRATION: Mass concentration, ug / m3
> + *			Doc keyword: in_massconcentration_pm1_input
> + *
> + * IIO_PH:		pH reading, negative base-10 logarithm of hydrodium
> + *			ions in a litre of water
> + *			Doc keyword: in_ph_raw
> + *
> + * IIO_PHASE:		Phase difference, radians
> + *			Doc keyword: in_phaseY_raw
> + *			TODO: What does "can be processed to radians" mean? Do
> + *			we have unit requirement?
> + *
> + * IIO_POSITIONRELATIVE: Relative position.
> + *			Doc keyword: in_positionrelative_x_raw
> + *
> + * IIO_POWER:		Power, milliwatts
> + *			Doc keyword: in_powerY_raw
> + *
> + * IIO_PRESSURE:	Pressure, kilopascal
> + *			Doc keyword: in_pressureY_raw
> + *
> + * IIO_RESISTANCE:	Resistance, ohms
> + *			Doc keyword: in_resistance_raw
> + *			TODO: What means "can be processed..." Do we have unit
> + *			requirement?
> + *
> + * IIO_ROT:		Euler angles, deg
> + *			Doc keyword: in_rot_yaw_raw
> + *
> + * IIO_STEPS:		Steps taken by the user
> + *			Doc keyword: in_steps_input
> + *
> + * IIO_TEMP:		Temperature, milli degrees Celsius
> + *			Doc keyword: in_temp_raw
> + *
> + * IIO_UVINDEX:		UV light intensity index
> + *			Doc keyword: in_uvindex_input
> + *
> + * IIO_VELOCITY:	Current speed (norm or magnitude of the velocity
> + *			vector), m/s
> + *			Doc keyword: in_velocity_sqrt(x^2+y^2+z^2)_input
> + *
> + * IIO_VOLTAGE:		Voltage, millivolts
> + *			Doc keyword: in_voltageY_raw
> + */
>  enum iio_chan_type {
>  	IIO_VOLTAGE,
>  	IIO_CURRENT,
> @@ -49,6 +171,22 @@ enum iio_chan_type {
>  	IIO_MASSCONCENTRATION,
>  };
>  
> +/**
> + * iio_modifier - accurate class for channel data
> + *
> + * IIO_MOD_<X,Y,Z>:	Value represents <X,Y,Z>-axis data.
> + *			Typically used by channels of type:
> + *			IIO_ACCEL, IIO_TEMP, IIO_GRAVITY, IIO_POSITIONRELATIVE,
> + *			IIO_ANGL_VEL, IIO_INCLI, IIO_MAGN
> + * IIO_MOD_LIGHT_BOTH:	Value contains visible and infra red light components
> + * IIO_MOD_LIGHT_IR:	Value represents infra-red radiation
> + * IIO_MOD_LIGHT_<RED, GREEN, BLUE>:
> + *			Value represents visible <red, green, blue>  light
> + * IIO_MOD_LIGHT_CLEAR:	Value represents all visible light frequencies
> + *
> + * Please find the detailed documentation for reported values from the
> + * Documentation/ABI/testing/sysfs-bus-iio.
> + */
>  enum iio_modifier {
>  	IIO_NO_MOD,
>  	IIO_MOD_X,
> 
> base-commit: c9c3395d5e3dcc6daee66c6908354d47bf98cb0c

Re: [RFC PATCH] iio: Add some kerneldoc for channel types

[Jonathan Cameron]

On Fri, 24 Feb 2023 14:36:38 +0000
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Fri, 24 Feb 2023 15:02:32 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
> > For occasional contributor like me navigating the IIO channel types and
> > modifiers may be a daunting task. One may have hard time finding out
> > what type of channel should be used for device data and what units the
> > data should be converted.
> > 
> > There is a great documentation for the sysfs interfaces though. What is
> > missing is mapping of the channel types and modifiers to the sysfs
> > documentation (and entries in documentation).
> > 
> > Give a hand to a driver writer by providing some documentation and by
> > pointing to the sysfs document from the kerneldocs of respective enums.
> > 
> > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>  
> +CC linux-iio
> 

A few quick notes. I'll want to read this a lot more carefully.
I'm not that keen on information in two places but this does have
a lot of references.

> > ---
> > Please note that this RFC patch should not be applied as is. The docs
> > have TODO comments regarding units for IIO_ELECTRICALCONDUCTIVITY,
> > IIO_PHASE and IIO_RESISTANCE. I'll fix these TODOs, remove RFC and respin
> > if anyone familiar with the values provided via sysfs could provide me the
> > corret units for these channels. I am also open to any suggestions how
> > to better link from enum documentation to specific entry at the IIO sysfs
> > documetation.
> > 
> > Initial discussion about these docs can be found from:
> > https://lore.kernel.org/all/0e0d45b7-e582-82b2-9bac-1f70f9dad9f7@gmail.com/
> > ---
> >  include/uapi/linux/iio/types.h | 140 ++++++++++++++++++++++++++++++++-
> >  1 file changed, 139 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
> > index c79f2f046a0b..e6329d3cc055 100644
> > --- a/include/uapi/linux/iio/types.h
> > +++ b/include/uapi/linux/iio/types.h
> > @@ -10,7 +10,129 @@
> >  
> >  #ifndef _UAPI_IIO_TYPES_H_
> >  #define _UAPI_IIO_TYPES_H_
> > -
> > +/**
> > + * iio_chan_type - Type of data transferred via IIO channel.
> > + *
> > + * The 'main' type of data transferred via channel. Please note that most
> > + * devices need to specify also a more accurate 'sub category'. See the
> > + * enum iio_modifier for this. (For example, IIO_ACCEL channel often needs to
> > + * specify the direction. IIO_CONCENTRATION specifies the type of substance
> > + * it measures etc).
> > + *
> > + * Use of correct units is required but scale and offset that user must apply
> > + * to channel values can be advertised.
That's a little vague:.

These reflect the units of the measurement via processed or unit after application
of scale and offset.

> > + *
> > + * Please find the detailed documentation for reported values from the
> > + * Documentation/ABI/testing/sysfs-bus-iio.
> > + *
> > + * IIO_ACCEL:		Acceleration, m/s^2
> > + *			Doc keyword: in_accel_x_raw
> > + *
> > + * IIO_ACTIVITY:	Activity state. For example a pedometer signaling
> > + *			jogging, walking or staying still.
> > + *			Doc keyword: in_activity_still_thresh_rising_en
> > + *
> > + * IIO_ALTVOLTAGE:
IIRC Peak to peak voltage.. So same units as voltage.

> > + *
> > + * IIO_ANGL:		Angle of rotation, radians.
> > + *			Doc keyword: in_angl_raw
> > + *
> > + * IIO_ANGL_VEL:	Angular velocity, rad/s
> > + *			Doc keyword: in_anglvel_x_raw
> > + *
> > + * IIO_CAPACITANCE:	Capacitance, nanofarads.
> > + *			Doc keyword: in_capacitanceY_raw
> > + *
> > + * IIO_CCT:

I had to got look at original patch of this one.  It's correlated color temperature.
Base unit Kelvin, though we currently have no users...


> > + *
> > + * IIO_CURRENT:		Current, milliamps
> > + *			Doc keyword: in_currentY_raw
> > + *
> > + * IIO_CONCENTRATION:	Reading of a substance, percents. Used for example by
> > + *			deviced measuring amount of CO2, O2, ethanol...
> > + *			Doc keyword: in_concentration_raw
> > + *
> > + * IIO_COUNT:		Deprecated, please use counter subsystem.
> > + *
> > + * IIO_DISTANCE:	Distance in meters. Typically used to report measured
> > + *			distance to an object or the distance covered by the
> > + *			user
> > + *			Doc keyword: in_distance_input
> > + *
> > + * IIO_ELECTRICALCONDUCTIVITY: electric conductivity, siemens per meter
> > + *			Doc keyword: in_electricalconductivity_raw
> > + *			TODO: What does "can be processed to siemens per meter"
> > + *			mean? Do we have unit requirement?

Hmm. I'd read that as meaning the unit is seimens per meter - after you've
applied offset and scale to the raw value.


> > + *
> > + * IIO_ENERGY:		Energy in Joules. Typically reported by a device
> > + *			measuring energy burnt by the user.
> > + *			Doc keyword: in_energy_input
> > + *
> > + * IIO_GRAVITY:		Gravity, m/s^2
> > + *			Doc keyword: in_gravity_x_raw
> > + *
> > + * IIO_HUMIDITYRELATIVE: Relative humidity, percents
> > + *			Doc keyword: in_humidityrelative_raw
> > + *
> > + * IIO_INCLI:		Inclination, degrees
> > + *			Doc keyword: in_incli_x_raw
> > + *
> > + * IIO_INDEX:		Deprecated, please use Counter subsystem
> > + *
> > + * IIO_INTENSITY:	Unitless intensity.
> > + *			Doc keyword: in_intensityY_raw
> > + *
> > + * IIO_LIGHT:		Visible light intensity, lux
> > + *			Doc keyword: in_illuminance_raw
> > + *
> > + * IIO_MAGN:		Magnetic field, Gauss.
> > + *			Doc keyword: in_magn_x_raw
> > + *
> > + * IIO_MASSCONCENTRATION: Mass concentration, ug / m3
> > + *			Doc keyword: in_massconcentration_pm1_input
> > + *
> > + * IIO_PH:		pH reading, negative base-10 logarithm of hydrodium
> > + *			ions in a litre of water
> > + *			Doc keyword: in_ph_raw
> > + *
> > + * IIO_PHASE:		Phase difference, radians
> > + *			Doc keyword: in_phaseY_raw
> > + *			TODO: What does "can be processed to radians" mean? Do
> > + *			we have unit requirement?

yes radians.  Same as above. It's just roundabout / less than clear English
usage.

> > + *
> > + * IIO_POSITIONRELATIVE: Relative position.
> > + *			Doc keyword: in_positionrelative_x_raw
> > + *
> > + * IIO_POWER:		Power, milliwatts
> > + *			Doc keyword: in_powerY_raw
> > + *
> > + * IIO_PRESSURE:	Pressure, kilopascal
> > + *			Doc keyword: in_pressureY_raw
> > + *
> > + * IIO_RESISTANCE:	Resistance, ohms
> > + *			Doc keyword: in_resistance_raw
> > + *			TODO: What means "can be processed..." Do we have unit
> > + *			requirement?

Same as above. Unit is ohms.

> > + *
> > + * IIO_ROT:		Euler angles, deg
> > + *			Doc keyword: in_rot_yaw_raw
> > + *
> > + * IIO_STEPS:		Steps taken by the user
> > + *			Doc keyword: in_steps_input
> > + *
> > + * IIO_TEMP:		Temperature, milli degrees Celsius
> > + *			Doc keyword: in_temp_raw
> > + *
> > + * IIO_UVINDEX:		UV light intensity index
> > + *			Doc keyword: in_uvindex_input
> > + *
> > + * IIO_VELOCITY:	Current speed (norm or magnitude of the velocity
> > + *			vector), m/s
> > + *			Doc keyword: in_velocity_sqrt(x^2+y^2+z^2)_input
> > + *
> > + * IIO_VOLTAGE:		Voltage, millivolts
> > + *			Doc keyword: in_voltageY_raw
> > + */
> >  enum iio_chan_type {
> >  	IIO_VOLTAGE,
> >  	IIO_CURRENT,
> > @@ -49,6 +171,22 @@ enum iio_chan_type {
> >  	IIO_MASSCONCENTRATION,
> >  };
> >  
> > +/**
> > + * iio_modifier - accurate class for channel data
> > + *
> > + * IIO_MOD_<X,Y,Z>:	Value represents <X,Y,Z>-axis data.
> > + *			Typically used by channels of type:
> > + *			IIO_ACCEL, IIO_TEMP, IIO_GRAVITY, IIO_POSITIONRELATIVE,
> > + *			IIO_ANGL_VEL, IIO_INCLI, IIO_MAGN
> > + * IIO_MOD_LIGHT_BOTH:	Value contains visible and infra red light components
> > + * IIO_MOD_LIGHT_IR:	Value represents infra-red radiation
> > + * IIO_MOD_LIGHT_<RED, GREEN, BLUE>:
> > + *			Value represents visible <red, green, blue>  light
> > + * IIO_MOD_LIGHT_CLEAR:	Value represents all visible light frequencies
> > + *
> > + * Please find the detailed documentation for reported values from the
> > + * Documentation/ABI/testing/sysfs-bus-iio.
> > + */
> >  enum iio_modifier {
> >  	IIO_NO_MOD,
> >  	IIO_MOD_X,
> > 
> > base-commit: c9c3395d5e3dcc6daee66c6908354d47bf98cb0c  
> 
> 

Re: [RFC PATCH] iio: Add some kerneldoc for channel types

[Vaittinen, Matti]

On 2/24/23 19:16, Jonathan Cameron wrote:
> On Fri, 24 Feb 2023 14:36:38 +0000
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> 
>> On Fri, 24 Feb 2023 15:02:32 +0200
>> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>>
>>> For occasional contributor like me navigating the IIO channel types and
>>> modifiers may be a daunting task. One may have hard time finding out
>>> what type of channel should be used for device data and what units the
>>> data should be converted.
>>>
>>> There is a great documentation for the sysfs interfaces though. What is
>>> missing is mapping of the channel types and modifiers to the sysfs
>>> documentation (and entries in documentation).
>>>
>>> Give a hand to a driver writer by providing some documentation and by
>>> pointing to the sysfs document from the kerneldocs of respective enums.
>>>
>>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>> +CC linux-iio
>>
> 
> A few quick notes. I'll want to read this a lot more carefully.
No problem - I am not in a hurry. Besides, I guess it's in the middle of 
a merge window so I did not really expect this to go anywhere right away :)

> I'm not that keen on information in two places but this does have
> a lot of references.
Yep. We talked about this shortly. I understand the problem of keeping 
information consistent and that is exactly why there is so little 
documentation here and more just a pointer(s) to the correct place in 
the sysfs doc. Still, I also see a problem of not having documentation 
in the enum definitions because this is the place where (in my 
experience) an average developer expects it to be. Please, let me use 
myself as an example when I started drafting an IIO driver for a device 
for first time... The process was roughly as follows:

1. I took the device data-sheet and gained some kind of an understanding 
what it did.
2. I searched for existing in-tree drivers for same category devices.
3. I did read the other driver's code in order to understand how it used 
the IIO to push the data to users.
4. I started drafting my driver.
5. I had plenty of questions about the meaning of the channel info 
defines - and I did try to look for the enums for documentation.
6. As there was no docs in enums, I tried to "guess" suitable enum 
values by enum names.
7. I ran git grep for good enum candidates in the kernel sources
8. I tried to find IIO docs from the net
...

I am pretty sure it would have saved me quite a bit of time if I hit 
some good information at step 5. And I can only assume that I am not an 
exception here. Quite a lot of the "black magic" in IIO lies upon 
understanding the values to the iio_chan_spec. And at least for me it 
was quite intuitive to hit the "ctrl + altGR + ]" when cursor was 
located on the enum value in an existing driver ;) (Don't everyone do 
just that?). [Well, just in case not everyone does that - with my editor 
setup it jumps to the definition of the value under the cursor - which 
is where this patch suggests adding the docs).

>>> ---
>>> Please note that this RFC patch should not be applied as is. The docs
>>> have TODO comments regarding units for IIO_ELECTRICALCONDUCTIVITY,
>>> IIO_PHASE and IIO_RESISTANCE. I'll fix these TODOs, remove RFC and respin
>>> if anyone familiar with the values provided via sysfs could provide me the
>>> corret units for these channels. I am also open to any suggestions how
>>> to better link from enum documentation to specific entry at the IIO sysfs
>>> documetation.
>>>
>>> Initial discussion about these docs can be found from:
>>> https://lore.kernel.org/all/0e0d45b7-e582-82b2-9bac-1f70f9dad9f7@gmail.com/
>>> ---
>>>   include/uapi/linux/iio/types.h | 140 ++++++++++++++++++++++++++++++++-
>>>   1 file changed, 139 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
>>> index c79f2f046a0b..e6329d3cc055 100644
>>> --- a/include/uapi/linux/iio/types.h
>>> +++ b/include/uapi/linux/iio/types.h
>>> @@ -10,7 +10,129 @@
>>>   
>>>   #ifndef _UAPI_IIO_TYPES_H_
>>>   #define _UAPI_IIO_TYPES_H_
>>> -
>>> +/**
>>> + * iio_chan_type - Type of data transferred via IIO channel.
>>> + *
>>> + * The 'main' type of data transferred via channel. Please note that most
>>> + * devices need to specify also a more accurate 'sub category'. See the
>>> + * enum iio_modifier for this. (For example, IIO_ACCEL channel often needs to
>>> + * specify the direction. IIO_CONCENTRATION specifies the type of substance
>>> + * it measures etc).
>>> + *
>>> + * Use of correct units is required but scale and offset that user must apply
>>> + * to channel values can be advertised.
> That's a little vague:.
> 
> These reflect the units of the measurement via processed or unit after application
> of scale and offset.

I like the clarity of your sentence. Thanks. However, from the 
perspective of a developer who has just landed in the IIO-corner of 
kernel - it may not be obvious there is a scale and offset to be 
advertised to users. Hence I'd like to add some small hint about what 
the mentioned scale and offset are - and that the driver can tell user 
that "the data I give to you - or expect from you - must have this scale 
and offset".

How about:
"These reflect the units of the measurement via processed or unit after 
application of scale and offset configured/advertised using the 
IIO_CHAN_INFO_SCALE and IIO_CHAN_INFO_OFFSET". Well, to tell my opinion 
- I think also the iio_chan_info_enum could really be easier to 
understand if it had few lines of explanations appended ;) Maybe I 
should add something there as well, and then just use your suggestion 
with the "see IIO_CHAN_INFO_SCALE and IIO_CHAN_INFO_OFFSET" for scale 
and offset.

>>> + * Please find the detailed documentation for reported values from the
>>> + * Documentation/ABI/testing/sysfs-bus-iio.
>>> + *
>>> + * IIO_ACCEL:		Acceleration, m/s^2
>>> + *			Doc keyword: in_accel_x_raw
>>> + *
>>> + * IIO_ACTIVITY:	Activity state. For example a pedometer signaling
>>> + *			jogging, walking or staying still.
>>> + *			Doc keyword: in_activity_still_thresh_rising_en
>>> + *
>>> + * IIO_ALTVOLTAGE:
> IIRC Peak to peak voltage.. So same units as voltage.
Thanks!

>>> + *
>>> + * IIO_ANGL:		Angle of rotation, radians.
>>> + *			Doc keyword: in_angl_raw
>>> + *
>>> + * IIO_ANGL_VEL:	Angular velocity, rad/s
>>> + *			Doc keyword: in_anglvel_x_raw
>>> + *
>>> + * IIO_CAPACITANCE:	Capacitance, nanofarads.
>>> + *			Doc keyword: in_capacitanceY_raw
>>> + *
>>> + * IIO_CCT:
> 
> I had to got look at original patch of this one.  It's correlated color temperature.
> Base unit Kelvin, though we currently have no users...

Hm. I think this should still be added in the sysfs doc. I bet this is 
used somewhere downstream. And - thanks for the explanation - I will add 
this in the doc. At least I would not have guessed the meaning just by 
the member name CCT.

>>> + *
>>> + * IIO_CURRENT:		Current, milliamps
>>> + *			Doc keyword: in_currentY_raw
>>> + *
>>> + * IIO_CONCENTRATION:	Reading of a substance, percents. Used for example by
>>> + *			deviced measuring amount of CO2, O2, ethanol...
>>> + *			Doc keyword: in_concentration_raw
>>> + *
>>> + * IIO_COUNT:		Deprecated, please use counter subsystem.
>>> + *
>>> + * IIO_DISTANCE:	Distance in meters. Typically used to report measured
>>> + *			distance to an object or the distance covered by the
>>> + *			user
>>> + *			Doc keyword: in_distance_input
>>> + *
>>> + * IIO_ELECTRICALCONDUCTIVITY: electric conductivity, siemens per meter
>>> + *			Doc keyword: in_electricalconductivity_raw
>>> + *			TODO: What does "can be processed to siemens per meter"
>>> + *			mean? Do we have unit requirement?
> 
> Hmm. I'd read that as meaning the unit is seimens per meter - after you've
> applied offset and scale to the raw value.

Ok. Maybe I'll send another patch which changes these "can be processed 
to..." unit descriptions to same format that is used for other types. I 
don't think the "can be processed to" is too definitive as pretty much 
any numbers can be "processed" to represent something when we select 
suitable fitting algorithm ;)

Thanks Jonathan. It has been very nice working with you and the IIO :) I 
think belong to the group of the most open and responsive subsystem 
maintainers I've been working with!

Yours,
	-- Matti

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

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

Re: [RFC PATCH] iio: Add some kerneldoc for channel types

[Jonathan Cameron]

On Sat, 25 Feb 2023 07:56:52 +0000
"Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com> wrote:

> On 2/24/23 19:16, Jonathan Cameron wrote:
> > On Fri, 24 Feb 2023 14:36:38 +0000
> > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> >   
> >> On Fri, 24 Feb 2023 15:02:32 +0200
> >> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >>  
> >>> For occasional contributor like me navigating the IIO channel types and
> >>> modifiers may be a daunting task. One may have hard time finding out
> >>> what type of channel should be used for device data and what units the
> >>> data should be converted.
> >>>
> >>> There is a great documentation for the sysfs interfaces though. What is
> >>> missing is mapping of the channel types and modifiers to the sysfs
> >>> documentation (and entries in documentation).
> >>>
> >>> Give a hand to a driver writer by providing some documentation and by
> >>> pointing to the sysfs document from the kerneldocs of respective enums.
> >>>
> >>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>  
> >> +CC linux-iio
> >>  
> > 
> > A few quick notes. I'll want to read this a lot more carefully.  
> No problem - I am not in a hurry. Besides, I guess it's in the middle of 
> a merge window so I did not really expect this to go anywhere right away :)

Merge windows are quiet for me (touch wood) as I've long since sent pull requests
to Greg KH.  Mind you getting close to end of Qemu cycle and CXL emulation
is keeping me busy these days.

> 
> > I'm not that keen on information in two places but this does have
> > a lot of references.  
> Yep. We talked about this shortly. I understand the problem of keeping 
> information consistent and that is exactly why there is so little 
> documentation here and more just a pointer(s) to the correct place in 
> the sysfs doc. Still, I also see a problem of not having documentation 
> in the enum definitions because this is the place where (in my 
> experience) an average developer expects it to be. Please, let me use 
> myself as an example when I started drafting an IIO driver for a device 
> for first time... The process was roughly as follows:
> 
> 1. I took the device data-sheet and gained some kind of an understanding 
> what it did.
> 2. I searched for existing in-tree drivers for same category devices.
> 3. I did read the other driver's code in order to understand how it used 
> the IIO to push the data to users.
> 4. I started drafting my driver.
> 5. I had plenty of questions about the meaning of the channel info 
> defines - and I did try to look for the enums for documentation.
> 6. As there was no docs in enums, I tried to "guess" suitable enum 
> values by enum names.
> 7. I ran git grep for good enum candidates in the kernel sources
> 8. I tried to find IIO docs from the net
> ...
> 
> I am pretty sure it would have saved me quite a bit of time if I hit 
> some good information at step 5. And I can only assume that I am not an 
> exception here. Quite a lot of the "black magic" in IIO lies upon 
> understanding the values to the iio_chan_spec. And at least for me it 
> was quite intuitive to hit the "ctrl + altGR + ]" when cursor was 
> located on the enum value in an existing driver ;) (Don't everyone do 
> just that?). [Well, just in case not everyone does that - with my editor 
> setup it jumps to the definition of the value under the cursor - which 
> is where this patch suggests adding the docs).

All fair enough - I'm being talked around to adding docs here.

> 
> >>> ---
> >>> Please note that this RFC patch should not be applied as is. The docs
> >>> have TODO comments regarding units for IIO_ELECTRICALCONDUCTIVITY,
> >>> IIO_PHASE and IIO_RESISTANCE. I'll fix these TODOs, remove RFC and respin
> >>> if anyone familiar with the values provided via sysfs could provide me the
> >>> corret units for these channels. I am also open to any suggestions how
> >>> to better link from enum documentation to specific entry at the IIO sysfs
> >>> documetation.
> >>>
> >>> Initial discussion about these docs can be found from:
> >>> https://lore.kernel.org/all/0e0d45b7-e582-82b2-9bac-1f70f9dad9f7@gmail.com/
> >>> ---
> >>>   include/uapi/linux/iio/types.h | 140 ++++++++++++++++++++++++++++++++-
> >>>   1 file changed, 139 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
> >>> index c79f2f046a0b..e6329d3cc055 100644
> >>> --- a/include/uapi/linux/iio/types.h
> >>> +++ b/include/uapi/linux/iio/types.h
> >>> @@ -10,7 +10,129 @@
> >>>   
> >>>   #ifndef _UAPI_IIO_TYPES_H_
> >>>   #define _UAPI_IIO_TYPES_H_
> >>> -
> >>> +/**
> >>> + * iio_chan_type - Type of data transferred via IIO channel.
> >>> + *
> >>> + * The 'main' type of data transferred via channel. Please note that most
> >>> + * devices need to specify also a more accurate 'sub category'. See the
> >>> + * enum iio_modifier for this. (For example, IIO_ACCEL channel often needs to
> >>> + * specify the direction. IIO_CONCENTRATION specifies the type of substance
> >>> + * it measures etc).
> >>> + *
> >>> + * Use of correct units is required but scale and offset that user must apply
> >>> + * to channel values can be advertised.  
> > That's a little vague:.
> > 
> > These reflect the units of the measurement via processed or unit after application
> > of scale and offset.  
> 
> I like the clarity of your sentence. Thanks. However, from the 
> perspective of a developer who has just landed in the IIO-corner of 
> kernel - it may not be obvious there is a scale and offset to be 
> advertised to users. Hence I'd like to add some small hint about what 
> the mentioned scale and offset are - and that the driver can tell user 
> that "the data I give to you - or expect from you - must have this scale 
> and offset".
> 
> How about:
> "These reflect the units of the measurement via processed or unit after 
> application of scale and offset configured/advertised using the 
> IIO_CHAN_INFO_SCALE and IIO_CHAN_INFO_OFFSET". 
Needs more to say where those are set.  "bits in ...."

> Well, to tell my opinion 
> - I think also the iio_chan_info_enum could really be easier to 
> understand if it had few lines of explanations appended ;) Maybe I 
> should add something there as well, and then just use your suggestion 
> with the "see IIO_CHAN_INFO_SCALE and IIO_CHAN_INFO_OFFSET" for scale 
> and offset.

That one is more of a can of worms than this one as some of the definitions
are complex (see the fun around hardwaregain)  I guess some entries can
be 'this one is complex, see the ABI docs'.


> 
> >>> + * Please find the detailed documentation for reported values from the
> >>> + * Documentation/ABI/testing/sysfs-bus-iio.
> >>> + *
> >>> + * IIO_ACCEL:		Acceleration, m/s^2
> >>> + *			Doc keyword: in_accel_x_raw
> >>> + *
> >>> + * IIO_ACTIVITY:	Activity state. For example a pedometer signaling
> >>> + *			jogging, walking or staying still.
> >>> + *			Doc keyword: in_activity_still_thresh_rising_en
> >>> + *
> >>> + * IIO_ALTVOLTAGE:  
> > IIRC Peak to peak voltage.. So same units as voltage.  
> Thanks!
> 
> >>> + *
> >>> + * IIO_ANGL:		Angle of rotation, radians.
> >>> + *			Doc keyword: in_angl_raw
> >>> + *
> >>> + * IIO_ANGL_VEL:	Angular velocity, rad/s
> >>> + *			Doc keyword: in_anglvel_x_raw
> >>> + *
> >>> + * IIO_CAPACITANCE:	Capacitance, nanofarads.
> >>> + *			Doc keyword: in_capacitanceY_raw
> >>> + *
> >>> + * IIO_CCT:  
> > 
> > I had to got look at original patch of this one.  It's correlated color temperature.
> > Base unit Kelvin, though we currently have no users...  
> 
> Hm. I think this should still be added in the sysfs doc. I bet this is 
> used somewhere downstream. And - thanks for the explanation - I will add 
> this in the doc. At least I would not have guessed the meaning just by 
> the member name CCT.

Nor could I.  Git blame to the rescue and some wrangling to deal with a move of
this enum :)

> 
> >>> + *
> >>> + * IIO_CURRENT:		Current, milliamps
> >>> + *			Doc keyword: in_currentY_raw
> >>> + *
> >>> + * IIO_CONCENTRATION:	Reading of a substance, percents. Used for example by
> >>> + *			deviced measuring amount of CO2, O2, ethanol...
> >>> + *			Doc keyword: in_concentration_raw
> >>> + *
> >>> + * IIO_COUNT:		Deprecated, please use counter subsystem.
> >>> + *
> >>> + * IIO_DISTANCE:	Distance in meters. Typically used to report measured
> >>> + *			distance to an object or the distance covered by the
> >>> + *			user
> >>> + *			Doc keyword: in_distance_input
> >>> + *
> >>> + * IIO_ELECTRICALCONDUCTIVITY: electric conductivity, siemens per meter
> >>> + *			Doc keyword: in_electricalconductivity_raw
> >>> + *			TODO: What does "can be processed to siemens per meter"
> >>> + *			mean? Do we have unit requirement?  
> > 
> > Hmm. I'd read that as meaning the unit is seimens per meter - after you've
> > applied offset and scale to the raw value.  
> 
> Ok. Maybe I'll send another patch which changes these "can be processed 
> to..." unit descriptions to same format that is used for other types. I 
> don't think the "can be processed to" is too definitive as pretty much 
> any numbers can be "processed" to represent something when we select 
> suitable fitting algorithm ;)
> 
> Thanks Jonathan. It has been very nice working with you and the IIO :) I 
> think belong to the group of the most open and responsive subsystem 
> maintainers I've been working with!
No problem.  You are are improving things so would be stupid to not help
you with that where time allows!

Jonathan

> 
> Yours,
> 	-- Matti
>