Re: [PATCH v2 10/12] iio: imu: inv_icm42600: add accurate timestamping
On Wed, 27 May 2020 20:57:09 +0200
Jean-Baptiste Maneyrol wrote:
> Add a timestamping mechanism for buffer that provides accurate
> event timestamps when using watermark. This mechanism estimates
> device internal clock by comparing FIFO interrupts delta time and
> device elapsed time computed by parsing FIFO data.
>
> Take interrupt timestamp in hard irq handler and add IIO device
> specific timestamp structures in device private allocation.
I haven't checked the maths or anything, but basic principle seems
sound. I'm wondering if we want to document somewhere what the
various ways we do this sort of timestamp generation are. They
give me a headache normally and it would be good to point people
to a reference. Still that's a job for another day.
I commented on the (lack) of need for force 8 byte alignment inline.
Jonathan
>
> Signed-off-by: Jean-Baptiste Maneyrol
> ---
> drivers/iio/imu/inv_icm42600/Makefile | 1 +
> drivers/iio/imu/inv_icm42600/inv_icm42600.h | 5 +
> .../iio/imu/inv_icm42600/inv_icm42600_accel.c | 40 +++-
> .../imu/inv_icm42600/inv_icm42600_buffer.c| 28 +++
> .../iio/imu/inv_icm42600/inv_icm42600_core.c | 17 +-
> .../iio/imu/inv_icm42600/inv_icm42600_gyro.c | 40 +++-
> .../imu/inv_icm42600/inv_icm42600_timestamp.c | 195 ++
> .../imu/inv_icm42600/inv_icm42600_timestamp.h | 85
> 8 files changed, 398 insertions(+), 13 deletions(-)
> create mode 100644 drivers/iio/imu/inv_icm42600/inv_icm42600_timestamp.c
> create mode 100644 drivers/iio/imu/inv_icm42600/inv_icm42600_timestamp.h
>
> diff --git a/drivers/iio/imu/inv_icm42600/Makefile
> b/drivers/iio/imu/inv_icm42600/Makefile
> index 0f49f6df3647..291714d9aa54 100644
> --- a/drivers/iio/imu/inv_icm42600/Makefile
> +++ b/drivers/iio/imu/inv_icm42600/Makefile
> @@ -6,6 +6,7 @@ inv-icm42600-y += inv_icm42600_gyro.o
> inv-icm42600-y += inv_icm42600_accel.o
> inv-icm42600-y += inv_icm42600_temp.o
> inv-icm42600-y += inv_icm42600_buffer.o
> +inv-icm42600-y += inv_icm42600_timestamp.o
>
> obj-$(CONFIG_INV_ICM42600_I2C) += inv-icm42600-i2c.o
> inv-icm42600-i2c-y += inv_icm42600_i2c.o
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> index 4d5811562a61..2de0dd7675fb 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> @@ -126,6 +126,7 @@ struct inv_icm42600_suspended {
> * @indio_accel:accelerometer IIO device.
> * @buffer: data transfer buffer aligned for DMA.
> * @fifo: FIFO management structure.
> + * @timestamp: interrupt timestamps.
> */
> struct inv_icm42600_state {
> struct mutex lock;
> @@ -141,6 +142,10 @@ struct inv_icm42600_state {
> struct iio_dev *indio_accel;
> uint8_t buffer[2] cacheline_aligned;
> struct inv_icm42600_fifo fifo;
> + struct {
> + int64_t gyro;
> + int64_t accel;
> + } timestamp;
> };
>
> /* Virtual register addresses: @bank on MSB (4 upper bits), @address on LSB
> */
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> index c73ce204efc6..ec1d124c1471 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> @@ -17,6 +17,7 @@
> #include "inv_icm42600.h"
> #include "inv_icm42600_temp.h"
> #include "inv_icm42600_buffer.h"
> +#include "inv_icm42600_timestamp.h"
>
> #define INV_ICM42600_ACCEL_CHAN(_modifier, _index, _ext_info)
> \
> { \
> @@ -50,6 +51,7 @@ enum inv_icm42600_accel_scan {
> INV_ICM42600_ACCEL_SCAN_Y,
> INV_ICM42600_ACCEL_SCAN_Z,
> INV_ICM42600_ACCEL_SCAN_TEMP,
> + INV_ICM42600_ACCEL_SCAN_TIMESTAMP,
> };
>
> static const struct iio_chan_spec_ext_info inv_icm42600_accel_ext_infos[] = {
> @@ -65,13 +67,15 @@ static const struct iio_chan_spec
> inv_icm42600_accel_channels[] = {
> INV_ICM42600_ACCEL_CHAN(IIO_MOD_Z, INV_ICM42600_ACCEL_SCAN_Z,
> inv_icm42600_accel_ext_infos),
> INV_ICM42600_TEMP_CHAN(INV_ICM42600_ACCEL_SCAN_TEMP),
> + IIO_CHAN_SOFT_TIMESTAMP(INV_ICM42600_ACCEL_SCAN_TIMESTAMP),
> };
>
> -/* IIO buffer data: 8 bytes */
> +/* IIO buffer data: 16 bytes */
> struct inv_icm42600_accel_buffer {
> struct inv_icm42600_fifo_sensor_data accel;
> int8_t temp;
> uint8_t padding;
> + int64_t timestamp;
So this falls into the open question I have in the cleanup set on timestamp
alignment. What you have here guarantees that we have the correct spacing, but
it allows the alignment of the whole structure to be 4 bytes on x86_32
I don't 'think' that's a problem because the relevant unaligned 8 bytes write
and read should be fine at 4 byte alignment. Most other archs will
do 8 byte
[PATCH v2 10/12] iio: imu: inv_icm42600: add accurate timestamping
Add a timestamping mechanism for buffer that provides accurate
event timestamps when using watermark. This mechanism estimates
device internal clock by comparing FIFO interrupts delta time and
device elapsed time computed by parsing FIFO data.
Take interrupt timestamp in hard irq handler and add IIO device
specific timestamp structures in device private allocation.
Signed-off-by: Jean-Baptiste Maneyrol
---
drivers/iio/imu/inv_icm42600/Makefile | 1 +
drivers/iio/imu/inv_icm42600/inv_icm42600.h | 5 +
.../iio/imu/inv_icm42600/inv_icm42600_accel.c | 40 +++-
.../imu/inv_icm42600/inv_icm42600_buffer.c| 28 +++
.../iio/imu/inv_icm42600/inv_icm42600_core.c | 17 +-
.../iio/imu/inv_icm42600/inv_icm42600_gyro.c | 40 +++-
.../imu/inv_icm42600/inv_icm42600_timestamp.c | 195 ++
.../imu/inv_icm42600/inv_icm42600_timestamp.h | 85
8 files changed, 398 insertions(+), 13 deletions(-)
create mode 100644 drivers/iio/imu/inv_icm42600/inv_icm42600_timestamp.c
create mode 100644 drivers/iio/imu/inv_icm42600/inv_icm42600_timestamp.h
diff --git a/drivers/iio/imu/inv_icm42600/Makefile
b/drivers/iio/imu/inv_icm42600/Makefile
index 0f49f6df3647..291714d9aa54 100644
--- a/drivers/iio/imu/inv_icm42600/Makefile
+++ b/drivers/iio/imu/inv_icm42600/Makefile
@@ -6,6 +6,7 @@ inv-icm42600-y += inv_icm42600_gyro.o
inv-icm42600-y += inv_icm42600_accel.o
inv-icm42600-y += inv_icm42600_temp.o
inv-icm42600-y += inv_icm42600_buffer.o
+inv-icm42600-y += inv_icm42600_timestamp.o
obj-$(CONFIG_INV_ICM42600_I2C) += inv-icm42600-i2c.o
inv-icm42600-i2c-y += inv_icm42600_i2c.o
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
index 4d5811562a61..2de0dd7675fb 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
@@ -126,6 +126,7 @@ struct inv_icm42600_suspended {
* @indio_accel: accelerometer IIO device.
* @buffer: data transfer buffer aligned for DMA.
* @fifo: FIFO management structure.
+ * @timestamp:interrupt timestamps.
*/
struct inv_icm42600_state {
struct mutex lock;
@@ -141,6 +142,10 @@ struct inv_icm42600_state {
struct iio_dev *indio_accel;
uint8_t buffer[2] cacheline_aligned;
struct inv_icm42600_fifo fifo;
+ struct {
+ int64_t gyro;
+ int64_t accel;
+ } timestamp;
};
/* Virtual register addresses: @bank on MSB (4 upper bits), @address on LSB */
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
index c73ce204efc6..ec1d124c1471 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
@@ -17,6 +17,7 @@
#include "inv_icm42600.h"
#include "inv_icm42600_temp.h"
#include "inv_icm42600_buffer.h"
+#include "inv_icm42600_timestamp.h"
#define INV_ICM42600_ACCEL_CHAN(_modifier, _index, _ext_info) \
{ \
@@ -50,6 +51,7 @@ enum inv_icm42600_accel_scan {
INV_ICM42600_ACCEL_SCAN_Y,
INV_ICM42600_ACCEL_SCAN_Z,
INV_ICM42600_ACCEL_SCAN_TEMP,
+ INV_ICM42600_ACCEL_SCAN_TIMESTAMP,
};
static const struct iio_chan_spec_ext_info inv_icm42600_accel_ext_infos[] = {
@@ -65,13 +67,15 @@ static const struct iio_chan_spec
inv_icm42600_accel_channels[] = {
INV_ICM42600_ACCEL_CHAN(IIO_MOD_Z, INV_ICM42600_ACCEL_SCAN_Z,
inv_icm42600_accel_ext_infos),
INV_ICM42600_TEMP_CHAN(INV_ICM42600_ACCEL_SCAN_TEMP),
+ IIO_CHAN_SOFT_TIMESTAMP(INV_ICM42600_ACCEL_SCAN_TIMESTAMP),
};
-/* IIO buffer data: 8 bytes */
+/* IIO buffer data: 16 bytes */
struct inv_icm42600_accel_buffer {
struct inv_icm42600_fifo_sensor_data accel;
int8_t temp;
uint8_t padding;
+ int64_t timestamp;
};
#define INV_ICM42600_SCAN_MASK_ACCEL_3AXIS \
@@ -92,6 +96,7 @@ static int inv_icm42600_accel_update_scan_mode(struct iio_dev
*indio_dev,
const unsigned long *scan_mask)
{
struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
+ struct inv_icm42600_timestamp *ts = iio_priv(indio_dev);
struct inv_icm42600_sensor_conf conf = INV_ICM42600_SENSOR_CONF_INIT;
unsigned int fifo_en = 0;
unsigned int sleep_temp = 0;
@@ -119,6 +124,7 @@ static int inv_icm42600_accel_update_scan_mode(struct
iio_dev *indio_dev,
}
/* update data FIFO write */
+ inv_icm42600_timestamp_apply_odr(ts, 0, 0, 0);
ret = inv_icm42600_buffer_set_fifo_en(st, fifo_en | st->fifo.en);
if (ret)
goto out_unlock;
@@ -299,9 +305,11 @@ static int inv_icm42600_accel_read_odr(struct
inv_icm42600_state *st,
return IIO_VAL_INT_PLUS_MICRO;
}
