Message-ID: <20240127153853.02d4f0d2@jic23-huawei>
Date: Sat, 27 Jan 2024 15:38:53 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Ramona Gradinariu <ramona.gradinariu@...log.com>
Cc: <corbet@....net>, <linux-doc@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>, <nuno.sa@...log.com>,
 <linux-iio@...r.kernel.org>
Subject: Re: [PATCH v2 1/1] docs: iio: add documentation for adis16475
 driver

On Tue, 23 Jan 2024 17:00:29 +0200
Ramona Gradinariu <ramona.gradinariu@...log.com> wrote:

> Add documentation for adis16475 driver which describes
> the driver device files and shows how the user may use the
> ABI for various scenarios (configuration, measurement, etc.).

hi Ramona,

I'm not against more documentation in the tree, but I'd like
a little info in this patch description for why you feel this
particular driver needs it?  Fine to argue they all do if that's
the reasoning!

To be useful I think we need some more detail on the raw data
from the chardev readout and that perhaps belongs in some generic
docs (fine to use this driver as an example).

Thanks,

Jonathan

> 
> Signed-off-by: Ramona Gradinariu <ramona.gradinariu@...log.com>
> ---
> changes in v2:
>  - added adis16475 documentation file to iio index.rst file
>  Documentation/iio/adis16475.rst | 327 ++++++++++++++++++++++++++++++++
>  Documentation/iio/index.rst     |   2 +
>  2 files changed, 329 insertions(+)
>  create mode 100644 Documentation/iio/adis16475.rst
> 
> diff --git a/Documentation/iio/adis16475.rst b/Documentation/iio/adis16475.rst
> new file mode 100644
> index 000000000000..9af054f4af79
> --- /dev/null
> +++ b/Documentation/iio/adis16475.rst
> @@ -0,0 +1,327 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +================
> +ADIS16475 driver
> +================
> +
> +This driver supports Analog Device's IMUs on SPI bus.
> +
> +1. Supported devices
> +====================
> +
> +* `ADIS16465 <https://www.analog.com/ADIS16465>`_
> +* `ADIS16467 <https://www.analog.com/ADIS16467>`_
> +* `ADIS16470 <https://www.analog.com/ADIS16470>`_
> +* `ADIS16475 <https://www.analog.com/ADIS16475>`_
> +* `ADIS16477 <https://www.analog.com/ADIS16477>`_
> +* `ADIS16500 <https://www.analog.com/ADIS16500>`_
> +* `ADIS16505 <https://www.analog.com/ADIS16505>`_
> +* `ADIS16507 <https://www.analog.com/ADIS16507>`_
> +
> +Each supported device is a precision, miniature microelectromechanical system
> +(MEMS) inertial measurement unit (IMU) that includes a triaxial gyroscope and a
> +triaxial accelerometer. Each inertial sensor in the IMU device combines with
> +signal conditioning that optimizes dynamic performance. The factory calibration
> +characterizes each sensor for sensitivity, bias, alignment, linear acceleration
> +(gyroscope bias), and point of percussion (accelerometer location). As a result,
> +each sensor has dynamic compensation formulas that provide accurate sensor
> +measurements over a broad set of conditions.
> +
> +2. Device attributes
> +====================
> +
> +Accelerometer, gyroscope measures are always provided. Furthermore, the driver
> +offers the capability to retrieve the delta angle and the delta velocity
> +measurements computed by the device.
> +
> +The delta angle measurements represent a calculation of angular displacement
> +between each sample update, while  the delta velocity measurements represent a
> +calculation of linear velocity change between each sample update.
> +
> +Finally, temperature data are provided which show a coarse measurement of
> +the temperature inside of the IMU device. This data is most useful for
> +monitoring relative changes in the thermal environment.
> +
> +The signal chain of each inertial sensor (accelerometers and

Wrapping is inconsistent. Please tidy that up so that for all the normal text you
wrap at same line length - this paragraph seems 10 chars shorter than the previous
one.

> +gyroscopes) includes the application of unique correction
> +formulas, which are derived from extensive characterization
> +of bias, sensitivity, alignment, response to linear acceleration
> +(gyroscopes), and point of percussion (accelerometer location)
> +over a temperature range of −40°C to +85°C, for each ADIS device.
> +These correction formulas are not accessible, but users do have
> +the opportunity to adjust the bias for each sensor individually
> +through the calibbias attribute.
> +
> ++-------------------------------------------+----------------------------------------------------------+
> +| 3-Axis Accelerometer related device files | Description                                              |
> ++-------------------------------------------+----------------------------------------------------------+
> +| in_accel_scale                            | Scale for the accelerometer channels.                    |

Probably good to add a sentence explaining where these files are - I'm guessing they are just the ones in the
iio\:deviceX directory and not the sub-directories below that.

> ++-------------------------------------------+----------------------------------------------------------+
> +| in_accel_x_calibbias                      | Calibration offset for the X-axis accelerometer channel. |
> ++-------------------------------------------+----------------------------------------------------------+
> +| accel_calibbias_x                         | x-axis acceleration offset correction                    |
> ++-------------------------------------------+----------------------------------------------------------+
> +| in_accel_x_raw                            | Raw X-axis accelerometer channel value.                  |
> ++-------------------------------------------+----------------------------------------------------------+
> +| accel_calibbias_y                         | y-axis acceleration offset correction                    |

no in_ prefix?

> ++-------------------------------------------+----------------------------------------------------------+
> +| in_accel_y_raw                            | Raw Y-axis accelerometer channel value.                  |
> ++-------------------------------------------+----------------------------------------------------------+
> +| in_accel_z_calibbias                      | Calibration offset for the Z-axis accelerometer channel. |
> ++-------------------------------------------+----------------------------------------------------------+
> +| in_accel_z_raw                            | Raw Z-axis accelerometer channel value.                  |
> ++-------------------------------------------+----------------------------------------------------------+
> +| in_deltavelocity_scale                    | Scale for delta velocity channels.                       |
> ++-------------------------------------------+----------------------------------------------------------+
> +| in_deltavelocity_x_raw                    | Raw X-axis delta velocity channel value.                 |
> ++-------------------------------------------+----------------------------------------------------------+
> +| in_deltavelocity_y_raw                    | Raw Y-axis delta velocity channel value.                 |
> ++-------------------------------------------+----------------------------------------------------------+
> +| in_deltavelocity_z_raw                    | Raw Z-axis delta velocity channel value.                 |
> ++-------------------------------------------+----------------------------------------------------------+
> +
> ++---------------------------------------+------------------------------------------------------+
> +| 3-Axis Gyroscope related device files | Description                                          |
> ++---------------------------------------+------------------------------------------------------+
> +| in_anglvel_scale                      | Scale for the gyroscope channels.                    |
> ++---------------------------------------+------------------------------------------------------+
> +| in_anglvel_x_calibbias                | Calibration offset for the X-axis gyroscope channel. |
> ++---------------------------------------+------------------------------------------------------+
> +| anglvel_calibbias_x                   | x-axis gyroscope offset correction                   |

A before, I think there is a in_ prefix on all these.

> ++---------------------------------------+------------------------------------------------------+
> +| in_anglvel_x_raw                      | Raw X-axis gyroscope channel value.                  |
> ++---------------------------------------+------------------------------------------------------+
..


> +Usage examples
> +--------------
> +
> +Set trigger if not available:

What do you mean by not available?


> +Obtain buffered data:
> +
> +.. code-block:: bash
> +
> +        root:/sys/bus/iio/devices/iio:device0> hexdump /dev/iio\:device0
> +        ...
> +        0044760 3901 0000 ffff f9fe ffff 29ee 0100 f79b
> +        0044770 3901 0000 ffff 98fe ffff 1aef 0100 439a
> +        0044780 3901 0000 ffff b4fe ffff 32ef 0100 c199
> +        0044790 3901 0000 ffff bdfe ffff 20ef 0100 5f9a
> +        00447a0 3901 0000 ffff 37ff ffff 1eef 0100 389b
> +        00447b0 3901 0000 ffff 7dff ffff 96ee 0100 5a9c
> +        ...
I'd use hexdump -C to list them byte wise - otherwise endian effects make
this rather incomprehensible.

Also if you want to list the raw data, good to explain what the various parts are
and how that is derived from the info in scan_elements.
That might be best done in a separate document though that is then referred to from
here. I don't think we have such a userspace document, having long relied on
people figuring it out from the example tools or using google to find talks various
folk have given (or the ADI wiki which might have stuff on this?)


> diff --git a/Documentation/iio/index.rst b/Documentation/iio/index.rst
> index 1b7292c58cd0..0087c0dafe59 100644
> --- a/Documentation/iio/index.rst
> +++ b/Documentation/iio/index.rst
> @@ -12,3 +12,5 @@ Industrial I/O
>     ep93xx_adc
> 
>     bno055
> +
> +   adis16475

Hmm. It's already in a random order but let's not make it worse to fix that up.
Put this before the bno055.

If you want to add some more formatting to this, say to add some section titles and
that would be great as a separate patch.

> --
> 2.34.1
> 

Powered by blists - more mailing lists