Date:   Sat, 25 Feb 2023 07:56:52 +0000
From:   "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>
To:     Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        Matti Vaittinen <mazziesaccount@...il.com>
CC:     Andy Shevchenko <andy.shevchenko@...il.com>,
        Andrea Merello <andrea.merello@....it>,
        Jagath Jog J <jagathjog1996@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>
Subject: Re: [RFC PATCH] iio: Add some kerneldoc for channel types

On 2/24/23 19:16, Jonathan Cameron wrote:
> On Fri, 24 Feb 2023 14:36:38 +0000
> Jonathan Cameron <Jonathan.Cameron@...wei.com> wrote:
> 
>> On Fri, 24 Feb 2023 15:02:32 +0200
>> Matti Vaittinen <mazziesaccount@...il.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@...il.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! ~~

Powered by blists - more mailing lists