linux-kernel - Re: [PATCH v2 2/3] iio: add documentation for iio_chan_info

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <64ab56d2-7d67-4432-6c81-1d63b41811d6@fi.rohmeurope.com>
Date:   Tue, 11 Apr 2023 08:27:08 +0000
From:   "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>
To:     Jonathan Cameron <jic23@...nel.org>,
        Matti Vaittinen <mazziesaccount@...il.com>
CC:     Lars-Peter Clausen <lars@...afoo.de>,
        Andrea Merello <andrea.merello@....it>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Jagath Jog J <jagathjog1996@...il.com>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/3] iio: add documentation for iio_chan_info_enum

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