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

[Vaittinen, Matti]

On 4/8/23 13:30, Jonathan Cameron wrote:
> On Sat, 25 Feb 2023 15:55:25 +0200
> Matti Vaittinen <mazziesaccount@xxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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! ~~