| Message ID | 884c8f386541ac572939b8993df7aea6ad99613b.1677331779.git.mazziesaccount@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: Improce kernel docs | expand |
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
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 >
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 + * 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. + * @IIO_CHAN_INFO_PROCESSED: Processed data. Typically driver performs + * computations to convert device data to more + * meaningfull processed values. + * @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. + * @IIO_CHAN_INFO_CALIBSCALE: + * @IIO_CHAN_INFO_CALIBBIAS: + * @IIO_CHAN_INFO_PEAK: Peak value (TODO: Since measurement start?) + * @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?) + * @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. + * @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. + * @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,
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(-)