* [PATCH 1/5] iio: imu: inv_mpu6050: replace timestamp fifo by generic timestamp
@ 2018-05-14 21:05 Jean-Baptiste Maneyrol
2018-05-14 21:05 ` [PATCH 2/5] iio: imu: inv_mpu6050: switch to use sample rate divider Jean-Baptiste Maneyrol
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: Jean-Baptiste Maneyrol @ 2018-05-14 21:05 UTC (permalink / raw)
To: linux-iio; +Cc: Jean-Baptiste Maneyrol
Using a fifo for storing timestamps is useless since the interrupt
is in one-shot mode, preventing the hard irq handler to be called
when the irq thread is running. Instead use the generic timestamp
function iio_pollfunc_store_time.
Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
---
drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 6 +---
drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h | 10 -------
drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 44 +-----------------------------
3 files changed, 2 insertions(+), 58 deletions(-)
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index 43fba5f..1e7e750 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -20,8 +20,6 @@
#include <linux/jiffies.h>
#include <linux/irq.h>
#include <linux/interrupt.h>
-#include <linux/kfifo.h>
-#include <linux/spinlock.h>
#include <linux/iio/iio.h>
#include <linux/acpi.h>
#include <linux/platform_device.h>
@@ -996,7 +994,7 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
indio_dev->modes = INDIO_BUFFER_TRIGGERED;
result = devm_iio_triggered_buffer_setup(dev, indio_dev,
- inv_mpu6050_irq_handler,
+ iio_pollfunc_store_time,
inv_mpu6050_read_fifo,
NULL);
if (result) {
@@ -1009,8 +1007,6 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
return result;
}
- INIT_KFIFO(st->timestamps);
- spin_lock_init(&st->time_stamp_lock);
result = devm_iio_device_register(dev, indio_dev);
if (result) {
dev_err(dev, "IIO register fail %d\n", result);
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
index c54da77..a92ddd4 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
@@ -12,8 +12,6 @@
*/
#include <linux/i2c.h>
#include <linux/i2c-mux.h>
-#include <linux/kfifo.h>
-#include <linux/spinlock.h>
#include <linux/mutex.h>
#include <linux/iio/iio.h>
#include <linux/iio/buffer.h>
@@ -116,36 +114,30 @@ struct inv_mpu6050_hw {
/*
* struct inv_mpu6050_state - Driver state variables.
- * @TIMESTAMP_FIFO_SIZE: fifo size for timestamp.
* @lock: Chip access lock.
* @trig: IIO trigger.
* @chip_config: Cached attribute information.
* @reg: Map of important registers.
* @hw: Other hardware-specific information.
* @chip_type: chip type.
- * @time_stamp_lock: spin lock to time stamp.
* @plat_data: platform data (deprecated in favor of @orientation).
* @orientation: sensor chip orientation relative to main hardware.
- * @timestamps: kfifo queue to store time stamp.
* @map regmap pointer.
* @irq interrupt number.
* @irq_mask the int_pin_cfg mask to configure interrupt type.
*/
struct inv_mpu6050_state {
-#define TIMESTAMP_FIFO_SIZE 16
struct mutex lock;
struct iio_trigger *trig;
struct inv_mpu6050_chip_config chip_config;
const struct inv_mpu6050_reg_map *reg;
const struct inv_mpu6050_hw *hw;
enum inv_devices chip_type;
- spinlock_t time_stamp_lock;
struct i2c_mux_core *muxc;
struct i2c_client *mux_client;
unsigned int powerup_count;
struct inv_mpu6050_platform_data plat_data;
struct iio_mount_matrix orientation;
- DECLARE_KFIFO(timestamps, long long, TIMESTAMP_FIFO_SIZE);
struct regmap *map;
int irq;
u8 irq_mask;
@@ -234,7 +226,6 @@ struct inv_mpu6050_state {
/* init parameters */
#define INV_MPU6050_INIT_FIFO_RATE 50
-#define INV_MPU6050_TIME_STAMP_TOR 5
#define INV_MPU6050_MAX_FIFO_RATE 1000
#define INV_MPU6050_MIN_FIFO_RATE 4
#define INV_MPU6050_ONE_K_HZ 1000
@@ -300,7 +291,6 @@ enum inv_mpu6050_clock_sel_e {
NUM_CLK
};
-irqreturn_t inv_mpu6050_irq_handler(int irq, void *p);
irqreturn_t inv_mpu6050_read_fifo(int irq, void *p);
int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev, int irq_type);
int inv_reset_fifo(struct iio_dev *indio_dev);
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
index 1795418..5436d18 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
@@ -19,20 +19,9 @@
#include <linux/jiffies.h>
#include <linux/irq.h>
#include <linux/interrupt.h>
-#include <linux/kfifo.h>
#include <linux/poll.h>
#include "inv_mpu_iio.h"
-static void inv_clear_kfifo(struct inv_mpu6050_state *st)
-{
- unsigned long flags;
-
- /* take the spin lock sem to avoid interrupt kick in */
- spin_lock_irqsave(&st->time_stamp_lock, flags);
- kfifo_reset(&st->timestamps);
- spin_unlock_irqrestore(&st->time_stamp_lock, flags);
-}
-
int inv_reset_fifo(struct iio_dev *indio_dev)
{
int result;
@@ -62,9 +51,6 @@ int inv_reset_fifo(struct iio_dev *indio_dev)
if (result)
goto reset_fifo_fail;
- /* clear timestamps fifo */
- inv_clear_kfifo(st);
-
/* enable interrupt */
if (st->chip_config.accl_fifo_enable ||
st->chip_config.gyro_fifo_enable) {
@@ -99,23 +85,6 @@ int inv_reset_fifo(struct iio_dev *indio_dev)
}
/**
- * inv_mpu6050_irq_handler() - Cache a timestamp at each data ready interrupt.
- */
-irqreturn_t inv_mpu6050_irq_handler(int irq, void *p)
-{
- struct iio_poll_func *pf = p;
- struct iio_dev *indio_dev = pf->indio_dev;
- struct inv_mpu6050_state *st = iio_priv(indio_dev);
- s64 timestamp;
-
- timestamp = iio_get_time_ns(indio_dev);
- kfifo_in_spinlocked(&st->timestamps, ×tamp, 1,
- &st->time_stamp_lock);
-
- return IRQ_WAKE_THREAD;
-}
-
-/**
* inv_mpu6050_read_fifo() - Transfer data from hardware FIFO to KFIFO.
*/
irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
@@ -127,7 +96,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
int result;
u8 data[INV_MPU6050_OUTPUT_DATA_SIZE];
u16 fifo_count;
- s64 timestamp;
+ s64 timestamp = pf->timestamp;
int int_status;
mutex_lock(&st->lock);
@@ -171,28 +140,17 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
goto flush_fifo;
if (fifo_count > INV_MPU6050_FIFO_THRESHOLD)
goto flush_fifo;
- /* Timestamp mismatch. */
- if (kfifo_len(&st->timestamps) >
- fifo_count / bytes_per_datum + INV_MPU6050_TIME_STAMP_TOR)
- goto flush_fifo;
do {
result = regmap_bulk_read(st->map, st->reg->fifo_r_w,
data, bytes_per_datum);
if (result)
goto flush_fifo;
-
- result = kfifo_out(&st->timestamps, ×tamp, 1);
- /* when there is no timestamp, put timestamp as 0 */
- if (result == 0)
- timestamp = 0;
-
/* skip first samples if needed */
if (st->skip_samples)
st->skip_samples--;
else
iio_push_to_buffers_with_timestamp(indio_dev, data,
timestamp);
-
fifo_count -= bytes_per_datum;
} while (fifo_count >= bytes_per_datum);
--
2.7.4
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/5] iio: imu: inv_mpu6050: switch to use sample rate divider
2018-05-14 21:05 [PATCH 1/5] iio: imu: inv_mpu6050: replace timestamp fifo by generic timestamp Jean-Baptiste Maneyrol
@ 2018-05-14 21:05 ` Jean-Baptiste Maneyrol
2018-05-15 19:08 ` Martin Kelly
2018-05-14 21:05 ` [PATCH 3/5] iio: imu: inv_mpu6050: fix fifo count reading Jean-Baptiste Maneyrol
` (2 subsequent siblings)
3 siblings, 1 reply; 14+ messages in thread
From: Jean-Baptiste Maneyrol @ 2018-05-14 21:05 UTC (permalink / raw)
To: linux-iio; +Cc: Jean-Baptiste Maneyrol
Instead of storing fifo rate in Hz, store the chip internal sample
rate divider. This will be more useful for timestamping. There
are both equivalent.
Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
---
drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 15 +++++++++------
drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h | 4 ++--
2 files changed, 11 insertions(+), 8 deletions(-)
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index 1e7e750..9e5c5e7 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -82,7 +82,7 @@ static const struct inv_mpu6050_reg_map reg_set_6050 = {
static const struct inv_mpu6050_chip_config chip_config_6050 = {
.fsr = INV_MPU6050_FSR_2000DPS,
.lpf = INV_MPU6050_FILTER_20HZ,
- .fifo_rate = INV_MPU6050_INIT_FIFO_RATE,
+ .divider = (INV_MPU6050_ONE_K_HZ / INV_MPU6050_INIT_FIFO_RATE) - 1,
.gyro_fifo_enable = false,
.accl_fifo_enable = false,
.accl_fs = INV_MPU6050_FS_02G,
@@ -628,7 +628,7 @@ static ssize_t
inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
- s32 fifo_rate;
+ int fifo_rate;
u8 d;
int result;
struct iio_dev *indio_dev = dev_to_iio_dev(dev);
@@ -644,8 +644,12 @@ inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr,
if (result)
return result;
+ /* compute freq divider and update fifo rate in case of truncation */
+ d = INV_MPU6050_ONE_K_HZ / fifo_rate - 1;
+ fifo_rate = INV_MPU6050_ONE_K_HZ / (d + 1);
+
mutex_lock(&st->lock);
- if (fifo_rate == st->chip_config.fifo_rate) {
+ if (d == st->chip_config.divider) {
result = 0;
goto fifo_rate_fail_unlock;
}
@@ -653,11 +657,10 @@ inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr,
if (result)
goto fifo_rate_fail_unlock;
- d = INV_MPU6050_ONE_K_HZ / fifo_rate - 1;
result = regmap_write(st->map, st->reg->sample_rate_div, d);
if (result)
goto fifo_rate_fail_power_off;
- st->chip_config.fifo_rate = fifo_rate;
+ st->chip_config.divider = d;
result = inv_mpu6050_set_lpf(st, fifo_rate);
if (result)
@@ -685,7 +688,7 @@ inv_fifo_rate_show(struct device *dev, struct device_attribute *attr,
unsigned fifo_rate;
mutex_lock(&st->lock);
- fifo_rate = st->chip_config.fifo_rate;
+ fifo_rate = INV_MPU6050_ONE_K_HZ / (st->chip_config.divider + 1);
mutex_unlock(&st->lock);
return scnprintf(buf, PAGE_SIZE, "%u\n", fifo_rate);
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
index a92ddd4..8d9044c 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
@@ -86,7 +86,7 @@ enum inv_devices {
* @accl_fs: accel full scale range.
* @accl_fifo_enable: enable accel data output
* @gyro_fifo_enable: enable gyro data output
- * @fifo_rate: FIFO update rate.
+ * @divider: sample rate divider.
*/
struct inv_mpu6050_chip_config {
unsigned int fsr:2;
@@ -94,7 +94,7 @@ struct inv_mpu6050_chip_config {
unsigned int accl_fs:2;
unsigned int accl_fifo_enable:1;
unsigned int gyro_fifo_enable:1;
- u16 fifo_rate;
+ u8 divider;
u8 user_ctrl;
};
--
2.7.4
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/5] iio: imu: inv_mpu6050: switch to use sample rate divider
2018-05-14 21:05 ` [PATCH 2/5] iio: imu: inv_mpu6050: switch to use sample rate divider Jean-Baptiste Maneyrol
@ 2018-05-15 19:08 ` Martin Kelly
2018-05-16 7:08 ` Jean-Baptiste Maneyrol
0 siblings, 1 reply; 14+ messages in thread
From: Martin Kelly @ 2018-05-15 19:08 UTC (permalink / raw)
To: Jean-Baptiste Maneyrol, linux-iio
On 05/14/2018 02:05 PM, Jean-Baptiste Maneyrol wrote:
> Instead of storing fifo rate in Hz, store the chip internal sample
> rate divider. This will be more useful for timestamping. There
> are both equivalent.
>
> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
> ---
> drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 15 +++++++++------
> drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h | 4 ++--
> 2 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> index 1e7e750..9e5c5e7 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> @@ -82,7 +82,7 @@ static const struct inv_mpu6050_reg_map reg_set_6050 = {
> static const struct inv_mpu6050_chip_config chip_config_6050 = {
> .fsr = INV_MPU6050_FSR_2000DPS,
> .lpf = INV_MPU6050_FILTER_20HZ,
> - .fifo_rate = INV_MPU6050_INIT_FIFO_RATE,
> + .divider = (INV_MPU6050_ONE_K_HZ / INV_MPU6050_INIT_FIFO_RATE) - 1,
> .gyro_fifo_enable = false,
> .accl_fifo_enable = false,
> .accl_fs = INV_MPU6050_FS_02G,
> @@ -628,7 +628,7 @@ static ssize_t
> inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr,
> const char *buf, size_t count)
> {
> - s32 fifo_rate;
> + int fifo_rate;
> u8 d;
Since the divider is a u8, I think the fifo_rate should also be unsigned
(I think u8 would fit it, but unsigned int should be fine too).
> int result;
> struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> @@ -644,8 +644,12 @@ inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr,
> if (result)
> return result;
>
> + /* compute freq divider and update fifo rate in case of truncation */
> + d = INV_MPU6050_ONE_K_HZ / fifo_rate - 1;
> + fifo_rate = INV_MPU6050_ONE_K_HZ / (d + 1);
> +
I don't quite understand why we need to set fifo_rate again. Could you
explain?
> mutex_lock(&st->lock);
> - if (fifo_rate == st->chip_config.fifo_rate) {
> + if (d == st->chip_config.divider) {
> result = 0;
> goto fifo_rate_fail_unlock;
> }
> @@ -653,11 +657,10 @@ inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr,
> if (result)
> goto fifo_rate_fail_unlock;
>
> - d = INV_MPU6050_ONE_K_HZ / fifo_rate - 1;
> result = regmap_write(st->map, st->reg->sample_rate_div, d);
> if (result)
> goto fifo_rate_fail_power_off;
> - st->chip_config.fifo_rate = fifo_rate;
> + st->chip_config.divider = d;
>
> result = inv_mpu6050_set_lpf(st, fifo_rate);
> if (result)
> @@ -685,7 +688,7 @@ inv_fifo_rate_show(struct device *dev, struct device_attribute *attr,
> unsigned fifo_rate;
>
> mutex_lock(&st->lock);
> - fifo_rate = st->chip_config.fifo_rate;
> + fifo_rate = INV_MPU6050_ONE_K_HZ / (st->chip_config.divider + 1);
Since we use this same line twice, I think it would be useful to put it
in a function inv_calculate_fifo_rate(u8 divider) or similar.
> mutex_unlock(&st->lock);
>
> return scnprintf(buf, PAGE_SIZE, "%u\n", fifo_rate);
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> index a92ddd4..8d9044c 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> @@ -86,7 +86,7 @@ enum inv_devices {
> * @accl_fs: accel full scale range.
> * @accl_fifo_enable: enable accel data output
> * @gyro_fifo_enable: enable gyro data output
> - * @fifo_rate: FIFO update rate.
> + * @divider: sample rate divider.
> */
> struct inv_mpu6050_chip_config {
> unsigned int fsr:2;
> @@ -94,7 +94,7 @@ struct inv_mpu6050_chip_config {
> unsigned int accl_fs:2;
> unsigned int accl_fifo_enable:1;
> unsigned int gyro_fifo_enable:1;
> - u16 fifo_rate;
> + u8 divider;
> u8 user_ctrl;
> };
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/5] iio: imu: inv_mpu6050: switch to use sample rate divider
2018-05-15 19:08 ` Martin Kelly
@ 2018-05-16 7:08 ` Jean-Baptiste Maneyrol
2018-05-16 16:19 ` Martin Kelly
0 siblings, 1 reply; 14+ messages in thread
From: Jean-Baptiste Maneyrol @ 2018-05-16 7:08 UTC (permalink / raw)
To: Martin Kelly, linux-iio
On 15/05/2018 21:08, Martin Kelly wrote:
> CAUTION: This email originated from outside of the organization. Please
> make sure the sender is who they say they are and do not click links or
> open attachments unless you recognize the sender and know the content is
> safe.
>
>
> On 05/14/2018 02:05 PM, Jean-Baptiste Maneyrol wrote:
>> Instead of storing fifo rate in Hz, store the chip internal sample
>> rate divider. This will be more useful for timestamping. There
>> are both equivalent.
>>
>> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
>> ---
>> drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 15 +++++++++------
>> drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h | 4 ++--
>> 2 files changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>> index 1e7e750..9e5c5e7 100644
>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>> @@ -82,7 +82,7 @@ static const struct inv_mpu6050_reg_map reg_set_6050
>> = {
>> static const struct inv_mpu6050_chip_config chip_config_6050 = {
>> .fsr = INV_MPU6050_FSR_2000DPS,
>> .lpf = INV_MPU6050_FILTER_20HZ,
>> - .fifo_rate = INV_MPU6050_INIT_FIFO_RATE,
>> + .divider = (INV_MPU6050_ONE_K_HZ / INV_MPU6050_INIT_FIFO_RATE) - 1,
>> .gyro_fifo_enable = false,
>> .accl_fifo_enable = false,
>> .accl_fs = INV_MPU6050_FS_02G,
>> @@ -628,7 +628,7 @@ static ssize_t
>> inv_mpu6050_fifo_rate_store(struct device *dev, struct
>> device_attribute *attr,
>> const char *buf, size_t count)
>> {
>> - s32 fifo_rate;
>> + int fifo_rate;
>> u8 d;
>
> Since the divider is a u8, I think the fifo_rate should also be unsigned
> (I think u8 would fit it, but unsigned int should be fine too).
fifo_rate is taken from sysfs entry using kstrtoint which requires a
signed int. The user can enter any number, even negative ones. Using a
signed integer for reading the entry enables to catch negative number
and return EINVAL for them.
>
>> int result;
>> struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>> @@ -644,8 +644,12 @@ inv_mpu6050_fifo_rate_store(struct device *dev,
>> struct device_attribute *attr,
>> if (result)
>> return result;
>>
>> + /* compute freq divider and update fifo rate in case of
>> truncation */
>> + d = INV_MPU6050_ONE_K_HZ / fifo_rate - 1;
>> + fifo_rate = INV_MPU6050_ONE_K_HZ / (d + 1);
>> +
>
> I don't quite understand why we need to set fifo_rate again. Could you
> explain?
Since fifo_rate is an arbitrary number coming from userspace, it can be
a rate that is not supported by the chip (not a divider of 1KHz). For
dealing with this case, I compute first the divider using provided
value. This computation will return a truncated number that is not
providing necessary the fifo rate asked if it is not supported. So I
compute back the fifo rate using this divider to store the real value.
>
>> mutex_lock(&st->lock);
>> - if (fifo_rate == st->chip_config.fifo_rate) {
>> + if (d == st->chip_config.divider) {
>> result = 0;
>> goto fifo_rate_fail_unlock;
>> }
>> @@ -653,11 +657,10 @@ inv_mpu6050_fifo_rate_store(struct device *dev,
>> struct device_attribute *attr,
>> if (result)
>> goto fifo_rate_fail_unlock;
>>
>> - d = INV_MPU6050_ONE_K_HZ / fifo_rate - 1;
>> result = regmap_write(st->map, st->reg->sample_rate_div, d);
>> if (result)
>> goto fifo_rate_fail_power_off;
>> - st->chip_config.fifo_rate = fifo_rate;
>> + st->chip_config.divider = d;
>>
>> result = inv_mpu6050_set_lpf(st, fifo_rate);
>> if (result)
>> @@ -685,7 +688,7 @@ inv_fifo_rate_show(struct device *dev, struct
>> device_attribute *attr,
>> unsigned fifo_rate;
>>
>> mutex_lock(&st->lock);
>> - fifo_rate = st->chip_config.fifo_rate;
>> + fifo_rate = INV_MPU6050_ONE_K_HZ / (st->chip_config.divider + 1);
>
> Since we use this same line twice, I think it would be useful to put it
> in a function inv_calculate_fifo_rate(u8 divider) or similar.
That is indeed possible.
>
>> mutex_unlock(&st->lock);
>>
>> return scnprintf(buf, PAGE_SIZE, "%u\n", fifo_rate);
>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>> index a92ddd4..8d9044c 100644
>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>> @@ -86,7 +86,7 @@ enum inv_devices {
>> * @accl_fs: accel full scale range.
>> * @accl_fifo_enable: enable accel data output
>> * @gyro_fifo_enable: enable gyro data output
>> - * @fifo_rate: FIFO update rate.
>> + * @divider: sample rate divider.
>> */
>> struct inv_mpu6050_chip_config {
>> unsigned int fsr:2;
>> @@ -94,7 +94,7 @@ struct inv_mpu6050_chip_config {
>> unsigned int accl_fs:2;
>> unsigned int accl_fifo_enable:1;
>> unsigned int gyro_fifo_enable:1;
>> - u16 fifo_rate;
>> + u8 divider;
>> u8 user_ctrl;
>> };
>>
>>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/5] iio: imu: inv_mpu6050: switch to use sample rate divider
2018-05-16 7:08 ` Jean-Baptiste Maneyrol
@ 2018-05-16 16:19 ` Martin Kelly
0 siblings, 0 replies; 14+ messages in thread
From: Martin Kelly @ 2018-05-16 16:19 UTC (permalink / raw)
To: Jean-Baptiste Maneyrol, linux-iio
On 05/16/2018 12:08 AM, Jean-Baptiste Maneyrol wrote:
>
>
> On 15/05/2018 21:08, Martin Kelly wrote:
>> CAUTION: This email originated from outside of the organization.
>> Please make sure the sender is who they say they are and do not click
>> links or open attachments unless you recognize the sender and know the
>> content is safe.
>>
>>
>> On 05/14/2018 02:05 PM, Jean-Baptiste Maneyrol wrote:
>>> Instead of storing fifo rate in Hz, store the chip internal sample
>>> rate divider. This will be more useful for timestamping. There
>>> are both equivalent.
>>>
>>> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
>>> ---
>>> drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 15 +++++++++------
>>> drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h | 4 ++--
>>> 2 files changed, 11 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>>> index 1e7e750..9e5c5e7 100644
>>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>>> @@ -82,7 +82,7 @@ static const struct inv_mpu6050_reg_map
>>> reg_set_6050 = {
>>> static const struct inv_mpu6050_chip_config chip_config_6050 = {
>>> .fsr = INV_MPU6050_FSR_2000DPS,
>>> .lpf = INV_MPU6050_FILTER_20HZ,
>>> - .fifo_rate = INV_MPU6050_INIT_FIFO_RATE,
>>> + .divider = (INV_MPU6050_ONE_K_HZ / INV_MPU6050_INIT_FIFO_RATE)
>>> - 1,
>>> .gyro_fifo_enable = false,
>>> .accl_fifo_enable = false,
>>> .accl_fs = INV_MPU6050_FS_02G,
>>> @@ -628,7 +628,7 @@ static ssize_t
>>> inv_mpu6050_fifo_rate_store(struct device *dev, struct
>>> device_attribute *attr,
>>> const char *buf, size_t count)
>>> {
>>> - s32 fifo_rate;
>>> + int fifo_rate;
>>> u8 d;
>>
>> Since the divider is a u8, I think the fifo_rate should also be unsigned
>> (I think u8 would fit it, but unsigned int should be fine too).
> fifo_rate is taken from sysfs entry using kstrtoint which requires a
> signed int. The user can enter any number, even negative ones. Using a
> signed integer for reading the entry enables to catch negative number
> and return EINVAL for them.
>
Yes, you are right. In this case we have a check for the range 4-1000,
so the division will be safe. I suspect the compiler will generate a
warning here about mixing types, but I think it's not turned on.
>>
>>> int result;
>>> struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> @@ -644,8 +644,12 @@ inv_mpu6050_fifo_rate_store(struct device *dev,
>>> struct device_attribute *attr,
>>> if (result)
>>> return result;
>>>
>>> + /* compute freq divider and update fifo rate in case of
>>> truncation */
>>> + d = INV_MPU6050_ONE_K_HZ / fifo_rate - 1;
>>> + fifo_rate = INV_MPU6050_ONE_K_HZ / (d + 1);
>>> +
>>
>> I don't quite understand why we need to set fifo_rate again. Could you
>> explain?
> Since fifo_rate is an arbitrary number coming from userspace, it can be
> a rate that is not supported by the chip (not a divider of 1KHz). For
> dealing with this case, I compute first the divider using provided
> value. This computation will return a truncated number that is not
> providing necessary the fifo rate asked if it is not supported. So I
> compute back the fifo rate using this divider to store the real value.
>
Yes, makes sense. If the user gives an evenly divisible number, they
will be the same, but if not, we need to calculate in the way you do.
>>
>>> mutex_lock(&st->lock);
>>> - if (fifo_rate == st->chip_config.fifo_rate) {
>>> + if (d == st->chip_config.divider) {
>>> result = 0;
>>> goto fifo_rate_fail_unlock;
>>> }
>>> @@ -653,11 +657,10 @@ inv_mpu6050_fifo_rate_store(struct device *dev,
>>> struct device_attribute *attr,
>>> if (result)
>>> goto fifo_rate_fail_unlock;
>>>
>>> - d = INV_MPU6050_ONE_K_HZ / fifo_rate - 1;
>>> result = regmap_write(st->map, st->reg->sample_rate_div, d);
>>> if (result)
>>> goto fifo_rate_fail_power_off;
>>> - st->chip_config.fifo_rate = fifo_rate;
>>> + st->chip_config.divider = d;
>>>
>>> result = inv_mpu6050_set_lpf(st, fifo_rate);
>>> if (result)
>>> @@ -685,7 +688,7 @@ inv_fifo_rate_show(struct device *dev, struct
>>> device_attribute *attr,
>>> unsigned fifo_rate;
>>>
>>> mutex_lock(&st->lock);
>>> - fifo_rate = st->chip_config.fifo_rate;
>>> + fifo_rate = INV_MPU6050_ONE_K_HZ / (st->chip_config.divider + 1);
>>
>> Since we use this same line twice, I think it would be useful to put it
>> in a function inv_calculate_fifo_rate(u8 divider) or similar.
> That is indeed possible.
>
>>
>>> mutex_unlock(&st->lock);
>>>
>>> return scnprintf(buf, PAGE_SIZE, "%u\n", fifo_rate);
>>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>>> index a92ddd4..8d9044c 100644
>>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>>> @@ -86,7 +86,7 @@ enum inv_devices {
>>> * @accl_fs: accel full scale range.
>>> * @accl_fifo_enable: enable accel data output
>>> * @gyro_fifo_enable: enable gyro data output
>>> - * @fifo_rate: FIFO update rate.
>>> + * @divider: sample rate divider.
>>> */
>>> struct inv_mpu6050_chip_config {
>>> unsigned int fsr:2;
>>> @@ -94,7 +94,7 @@ struct inv_mpu6050_chip_config {
>>> unsigned int accl_fs:2;
>>> unsigned int accl_fifo_enable:1;
>>> unsigned int gyro_fifo_enable:1;
>>> - u16 fifo_rate;
>>> + u8 divider;
>>> u8 user_ctrl;
>>> };
>>>
>>>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/5] iio: imu: inv_mpu6050: fix fifo count reading
2018-05-14 21:05 [PATCH 1/5] iio: imu: inv_mpu6050: replace timestamp fifo by generic timestamp Jean-Baptiste Maneyrol
2018-05-14 21:05 ` [PATCH 2/5] iio: imu: inv_mpu6050: switch to use sample rate divider Jean-Baptiste Maneyrol
@ 2018-05-14 21:05 ` Jean-Baptiste Maneyrol
2018-05-15 19:09 ` Martin Kelly
2018-05-20 10:20 ` Jonathan Cameron
2018-05-14 21:05 ` [PATCH 4/5] iio: imu: inv_mpu6050: better fifo overflow handling Jean-Baptiste Maneyrol
2018-05-14 21:05 ` [PATCH 5/5] iio: imu: inv_mpu6050: new timestamp mechanism Jean-Baptiste Maneyrol
3 siblings, 2 replies; 14+ messages in thread
From: Jean-Baptiste Maneyrol @ 2018-05-14 21:05 UTC (permalink / raw)
To: linux-iio; +Cc: Jean-Baptiste Maneyrol
Use unaligned access since buffer is a bytes table. Truncate fifo
count to read only complete datum.
Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
---
drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 24 +++++++++++-------------
1 file changed, 11 insertions(+), 13 deletions(-)
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
index 5436d18..7724888 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
@@ -20,6 +20,7 @@
#include <linux/irq.h>
#include <linux/interrupt.h>
#include <linux/poll.h>
+#include <asm/unaligned.h>
#include "inv_mpu_iio.h"
int inv_reset_fifo(struct iio_dev *indio_dev)
@@ -98,6 +99,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
u16 fifo_count;
s64 timestamp = pf->timestamp;
int int_status;
+ size_t i, nb;
mutex_lock(&st->lock);
@@ -132,27 +134,23 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
INV_MPU6050_FIFO_COUNT_BYTE);
if (result)
goto end_session;
- fifo_count = be16_to_cpup((__be16 *)(&data[0]));
- if (fifo_count < bytes_per_datum)
- goto end_session;
- /* fifo count can't be an odd number. If it is odd, reset the FIFO. */
- if (fifo_count & 1)
- goto flush_fifo;
+ fifo_count = get_unaligned_be16(&data[0]);
if (fifo_count > INV_MPU6050_FIFO_THRESHOLD)
goto flush_fifo;
- do {
+ /* compute and process all complete datum */
+ nb = fifo_count / bytes_per_datum;
+ for (i = 0; i < nb; ++i) {
result = regmap_bulk_read(st->map, st->reg->fifo_r_w,
data, bytes_per_datum);
if (result)
goto flush_fifo;
/* skip first samples if needed */
- if (st->skip_samples)
+ if (st->skip_samples) {
st->skip_samples--;
- else
- iio_push_to_buffers_with_timestamp(indio_dev, data,
- timestamp);
- fifo_count -= bytes_per_datum;
- } while (fifo_count >= bytes_per_datum);
+ continue;
+ }
+ iio_push_to_buffers_with_timestamp(indio_dev, data, timestamp);
+ }
end_session:
mutex_unlock(&st->lock);
--
2.7.4
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 3/5] iio: imu: inv_mpu6050: fix fifo count reading
2018-05-14 21:05 ` [PATCH 3/5] iio: imu: inv_mpu6050: fix fifo count reading Jean-Baptiste Maneyrol
@ 2018-05-15 19:09 ` Martin Kelly
2018-05-20 10:20 ` Jonathan Cameron
1 sibling, 0 replies; 14+ messages in thread
From: Martin Kelly @ 2018-05-15 19:09 UTC (permalink / raw)
To: Jean-Baptiste Maneyrol, linux-iio
On 05/14/2018 02:05 PM, Jean-Baptiste Maneyrol wrote:
> Use unaligned access since buffer is a bytes table. Truncate fifo
> count to read only complete datum.
>
> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
> ---
> drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 24 +++++++++++-------------
> 1 file changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> index 5436d18..7724888 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> @@ -20,6 +20,7 @@
> #include <linux/irq.h>
> #include <linux/interrupt.h>
> #include <linux/poll.h>
> +#include <asm/unaligned.h>
> #include "inv_mpu_iio.h"
>
> int inv_reset_fifo(struct iio_dev *indio_dev)
> @@ -98,6 +99,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
> u16 fifo_count;
> s64 timestamp = pf->timestamp;
> int int_status;
> + size_t i, nb;
>
> mutex_lock(&st->lock);
>
> @@ -132,27 +134,23 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
> INV_MPU6050_FIFO_COUNT_BYTE);
> if (result)
> goto end_session;
> - fifo_count = be16_to_cpup((__be16 *)(&data[0]));
> - if (fifo_count < bytes_per_datum)
> - goto end_session;
> - /* fifo count can't be an odd number. If it is odd, reset the FIFO. */
> - if (fifo_count & 1)
> - goto flush_fifo;
> + fifo_count = get_unaligned_be16(&data[0]);
> if (fifo_count > INV_MPU6050_FIFO_THRESHOLD)
> goto flush_fifo;
> - do {
> + /* compute and process all complete datum */
> + nb = fifo_count / bytes_per_datum;
> + for (i = 0; i < nb; ++i) {
> result = regmap_bulk_read(st->map, st->reg->fifo_r_w,
> data, bytes_per_datum);
> if (result)
> goto flush_fifo;
> /* skip first samples if needed */
> - if (st->skip_samples)
> + if (st->skip_samples) {
> st->skip_samples--;
> - else
> - iio_push_to_buffers_with_timestamp(indio_dev, data,
> - timestamp);
> - fifo_count -= bytes_per_datum;
> - } while (fifo_count >= bytes_per_datum);
> + continue;
> + }
> + iio_push_to_buffers_with_timestamp(indio_dev, data, timestamp);
> + }
>
> end_session:
> mutex_unlock(&st->lock);
>
Reviewed-by: Martin Kelly <mkelly@xevo.com>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/5] iio: imu: inv_mpu6050: fix fifo count reading
2018-05-14 21:05 ` [PATCH 3/5] iio: imu: inv_mpu6050: fix fifo count reading Jean-Baptiste Maneyrol
2018-05-15 19:09 ` Martin Kelly
@ 2018-05-20 10:20 ` Jonathan Cameron
1 sibling, 0 replies; 14+ messages in thread
From: Jonathan Cameron @ 2018-05-20 10:20 UTC (permalink / raw)
To: Jean-Baptiste Maneyrol; +Cc: linux-iio
On Mon, 14 May 2018 23:05:53 +0200
Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote:
> Use unaligned access since buffer is a bytes table. Truncate fifo
> count to read only complete datum.
Hmm. Probably cheaper to enforce the alignment, but I suppose it
doesn't really matter.
Series looks fine except for the minor stuff you have already
said you'll address. I'll take a close look at V2 before applying.
Thanks,
Jonathan
>
> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
> ---
> drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 24 +++++++++++-------------
> 1 file changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> index 5436d18..7724888 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> @@ -20,6 +20,7 @@
> #include <linux/irq.h>
> #include <linux/interrupt.h>
> #include <linux/poll.h>
> +#include <asm/unaligned.h>
> #include "inv_mpu_iio.h"
>
> int inv_reset_fifo(struct iio_dev *indio_dev)
> @@ -98,6 +99,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
> u16 fifo_count;
> s64 timestamp = pf->timestamp;
> int int_status;
> + size_t i, nb;
>
> mutex_lock(&st->lock);
>
> @@ -132,27 +134,23 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
> INV_MPU6050_FIFO_COUNT_BYTE);
> if (result)
> goto end_session;
> - fifo_count = be16_to_cpup((__be16 *)(&data[0]));
> - if (fifo_count < bytes_per_datum)
> - goto end_session;
> - /* fifo count can't be an odd number. If it is odd, reset the FIFO. */
> - if (fifo_count & 1)
> - goto flush_fifo;
> + fifo_count = get_unaligned_be16(&data[0]);
> if (fifo_count > INV_MPU6050_FIFO_THRESHOLD)
> goto flush_fifo;
> - do {
> + /* compute and process all complete datum */
> + nb = fifo_count / bytes_per_datum;
> + for (i = 0; i < nb; ++i) {
> result = regmap_bulk_read(st->map, st->reg->fifo_r_w,
> data, bytes_per_datum);
> if (result)
> goto flush_fifo;
> /* skip first samples if needed */
> - if (st->skip_samples)
> + if (st->skip_samples) {
> st->skip_samples--;
> - else
> - iio_push_to_buffers_with_timestamp(indio_dev, data,
> - timestamp);
> - fifo_count -= bytes_per_datum;
> - } while (fifo_count >= bytes_per_datum);
> + continue;
> + }
> + iio_push_to_buffers_with_timestamp(indio_dev, data, timestamp);
> + }
>
> end_session:
> mutex_unlock(&st->lock);
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 4/5] iio: imu: inv_mpu6050: better fifo overflow handling
2018-05-14 21:05 [PATCH 1/5] iio: imu: inv_mpu6050: replace timestamp fifo by generic timestamp Jean-Baptiste Maneyrol
2018-05-14 21:05 ` [PATCH 2/5] iio: imu: inv_mpu6050: switch to use sample rate divider Jean-Baptiste Maneyrol
2018-05-14 21:05 ` [PATCH 3/5] iio: imu: inv_mpu6050: fix fifo count reading Jean-Baptiste Maneyrol
@ 2018-05-14 21:05 ` Jean-Baptiste Maneyrol
2018-05-15 19:09 ` Martin Kelly
2018-05-14 21:05 ` [PATCH 5/5] iio: imu: inv_mpu6050: new timestamp mechanism Jean-Baptiste Maneyrol
3 siblings, 1 reply; 14+ messages in thread
From: Jean-Baptiste Maneyrol @ 2018-05-14 21:05 UTC (permalink / raw)
To: linux-iio; +Cc: Jean-Baptiste Maneyrol
Use fifo overflow bit from int status rather than using an
arbitrary threshold.
Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
---
drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h | 2 +-
drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 5 +++--
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
index 8d9044c..09a7e1c 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
@@ -166,6 +166,7 @@ struct inv_mpu6050_state {
#define INV_MPU6050_REG_RAW_GYRO 0x43
#define INV_MPU6050_REG_INT_STATUS 0x3A
+#define INV_MPU6050_BIT_FIFO_OVERFLOW_INT 0x10
#define INV_MPU6050_BIT_RAW_DATA_RDY_INT 0x01
#define INV_MPU6050_REG_USER_CTRL 0x6A
@@ -190,7 +191,6 @@ struct inv_mpu6050_state {
#define INV_MPU6050_BYTES_PER_3AXIS_SENSOR 6
#define INV_MPU6050_FIFO_COUNT_BYTE 2
-#define INV_MPU6050_FIFO_THRESHOLD 500
/* mpu6500 registers */
#define INV_MPU6500_REG_ACCEL_CONFIG_2 0x1D
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
index 7724888..7a4aaed 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
@@ -110,6 +110,9 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
"failed to ack interrupt\n");
goto flush_fifo;
}
+ /* handle fifo overflow by reseting fifo */
+ if (int_status & INV_MPU6050_BIT_FIFO_OVERFLOW_INT)
+ goto flush_fifo;
if (!(int_status & INV_MPU6050_BIT_RAW_DATA_RDY_INT)) {
dev_warn(regmap_get_device(st->map),
"spurious interrupt with status 0x%x\n", int_status);
@@ -135,8 +138,6 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
if (result)
goto end_session;
fifo_count = get_unaligned_be16(&data[0]);
- if (fifo_count > INV_MPU6050_FIFO_THRESHOLD)
- goto flush_fifo;
/* compute and process all complete datum */
nb = fifo_count / bytes_per_datum;
for (i = 0; i < nb; ++i) {
--
2.7.4
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 4/5] iio: imu: inv_mpu6050: better fifo overflow handling
2018-05-14 21:05 ` [PATCH 4/5] iio: imu: inv_mpu6050: better fifo overflow handling Jean-Baptiste Maneyrol
@ 2018-05-15 19:09 ` Martin Kelly
0 siblings, 0 replies; 14+ messages in thread
From: Martin Kelly @ 2018-05-15 19:09 UTC (permalink / raw)
To: Jean-Baptiste Maneyrol, linux-iio
On 05/14/2018 02:05 PM, Jean-Baptiste Maneyrol wrote:
> Use fifo overflow bit from int status rather than using an
> arbitrary threshold.
>
> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
> ---
> drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h | 2 +-
> drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 5 +++--
> 2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> index 8d9044c..09a7e1c 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> @@ -166,6 +166,7 @@ struct inv_mpu6050_state {
> #define INV_MPU6050_REG_RAW_GYRO 0x43
>
> #define INV_MPU6050_REG_INT_STATUS 0x3A
> +#define INV_MPU6050_BIT_FIFO_OVERFLOW_INT 0x10
> #define INV_MPU6050_BIT_RAW_DATA_RDY_INT 0x01
>
> #define INV_MPU6050_REG_USER_CTRL 0x6A
> @@ -190,7 +191,6 @@ struct inv_mpu6050_state {
>
> #define INV_MPU6050_BYTES_PER_3AXIS_SENSOR 6
> #define INV_MPU6050_FIFO_COUNT_BYTE 2
> -#define INV_MPU6050_FIFO_THRESHOLD 500
>
> /* mpu6500 registers */
> #define INV_MPU6500_REG_ACCEL_CONFIG_2 0x1D
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> index 7724888..7a4aaed 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> @@ -110,6 +110,9 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
> "failed to ack interrupt\n");
> goto flush_fifo;
> }
> + /* handle fifo overflow by reseting fifo */
> + if (int_status & INV_MPU6050_BIT_FIFO_OVERFLOW_INT)
> + goto flush_fifo;
> if (!(int_status & INV_MPU6050_BIT_RAW_DATA_RDY_INT)) {
> dev_warn(regmap_get_device(st->map),
> "spurious interrupt with status 0x%x\n", int_status);
> @@ -135,8 +138,6 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
> if (result)
> goto end_session;
> fifo_count = get_unaligned_be16(&data[0]);
> - if (fifo_count > INV_MPU6050_FIFO_THRESHOLD)
> - goto flush_fifo;
> /* compute and process all complete datum */
> nb = fifo_count / bytes_per_datum;
> for (i = 0; i < nb; ++i) {
>
Reviewed-by: Martin Kelly <mkelly@xevo.com>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 5/5] iio: imu: inv_mpu6050: new timestamp mechanism
2018-05-14 21:05 [PATCH 1/5] iio: imu: inv_mpu6050: replace timestamp fifo by generic timestamp Jean-Baptiste Maneyrol
` (2 preceding siblings ...)
2018-05-14 21:05 ` [PATCH 4/5] iio: imu: inv_mpu6050: better fifo overflow handling Jean-Baptiste Maneyrol
@ 2018-05-14 21:05 ` Jean-Baptiste Maneyrol
2018-05-15 19:31 ` Martin Kelly
3 siblings, 1 reply; 14+ messages in thread
From: Jean-Baptiste Maneyrol @ 2018-05-14 21:05 UTC (permalink / raw)
To: linux-iio; +Cc: Jean-Baptiste Maneyrol
Check validity of interrupt timestamps by computing time between
2 interrupts. If it matches the chip frequency modulo 4%, it is
used as the data timestamp and also for estimating the chip
frequency measured from the system. Otherwise timestamp is
computed using the estimated chip frequency.
Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
---
drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 7 +++
drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h | 8 +++
drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 78 +++++++++++++++++++++++++++++-
3 files changed, 92 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index 9e5c5e7..ade6294 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -295,6 +295,13 @@ static int inv_mpu6050_init_config(struct iio_dev *indio_dev)
memcpy(&st->chip_config, hw_info[st->chip_type].config,
sizeof(struct inv_mpu6050_chip_config));
+ /*
+ * Internal chip period is 1ms (1kHz).
+ * Let's use at the beginning the theorical value before measuring
+ * with interrupt timestamps.
+ */
+ st->chip_period = NSEC_PER_MSEC;
+
return inv_mpu6050_set_power_itg(st, false);
error_power_off:
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
index 09a7e1c..b78640c 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
@@ -125,6 +125,9 @@ struct inv_mpu6050_hw {
* @map regmap pointer.
* @irq interrupt number.
* @irq_mask the int_pin_cfg mask to configure interrupt type.
+ * @chip_period: chip internal period estimation (~1kHz).
+ * @it_timestamp: timestamp from previous interrupt.
+ * @data_timestamp: timestamp for next data sample.
*/
struct inv_mpu6050_state {
struct mutex lock;
@@ -142,6 +145,9 @@ struct inv_mpu6050_state {
int irq;
u8 irq_mask;
unsigned skip_samples;
+ s64 chip_period;
+ s64 it_timestamp;
+ s64 data_timestamp;
};
/*register and associated bit definition*/
@@ -223,6 +229,8 @@ struct inv_mpu6050_state {
#define INV_MPU6050_LATCH_INT_EN 0x20
#define INV_MPU6050_BIT_BYPASS_EN 0x2
+/* Allowed timestamp period jitter in percent */
+#define INV_MPU6050_TS_PERIOD_JITTER 4
/* init parameters */
#define INV_MPU6050_INIT_FIFO_RATE 50
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
index 7a4aaed..e06e509 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
@@ -23,12 +23,86 @@
#include <asm/unaligned.h>
#include "inv_mpu_iio.h"
+/**
+ * inv_mpu6050_add_timestamp() - Add a new interrupt timestamp
+ *
+ * @st: driver state
+ * @timestamp: the interrupt timestamp
+ * @nb: number of data set in the fifo
+ *
+ * This function uses interrupt timestamps to estimate the chip period and
+ * to choose the data timestamp to come.
+ */
+static void inv_mpu6050_add_timestamp(struct inv_mpu6050_state *st,
+ s64 timestamp, size_t nb)
+{
+ /* Period boundaries for accepting timestamp */
+ const s64 period_min =
+ (NSEC_PER_MSEC * (100 - INV_MPU6050_TS_PERIOD_JITTER)) / 100;
+ const s64 period_max =
+ (NSEC_PER_MSEC * (100 + INV_MPU6050_TS_PERIOD_JITTER)) / 100;
+ const unsigned divider = st->chip_config.divider + 1;
+ s64 val;
+ bool use_it_timestamp = false;
+
+ if (st->it_timestamp == 0) {
+ /* not initialized, forced to use it timestamp */
+ use_it_timestamp = true;
+ } else if (nb == 1) {
+ /*
+ * Validate the use of it timestamp by checking if interrupt
+ * has been delayed.
+ * nb > 1 means interrupt was delayed for more than 1 sample,
+ * so it's obviously not good.
+ * Compute the chip period between 2 interrupts for validating.
+ */
+ val = (timestamp - st->it_timestamp) / divider;
+ if (val > period_min && val < period_max) {
+ /* update chip period and use it timestamp */
+ st->chip_period = (st->chip_period + val) / 2;
+ use_it_timestamp = true;
+ }
+ }
+
+ if (use_it_timestamp) {
+ /* manage case of multiple samples in the fifo (nb > 1) */
+ val = (nb - 1) * st->chip_period * divider;
+ st->data_timestamp = timestamp - val;
+ }
+
+ /* save it timestamp */
+ st->it_timestamp = timestamp;
+}
+
+/**
+ * inv_mpu6050_get_timestamp() - Return the current data timestamp
+ *
+ * @st: driver state
+ * @return: current data timestamp
+ *
+ * This function returns the current data timestamp and prepares for next one.
+ */
+static s64 inv_mpu6050_get_timestamp(struct inv_mpu6050_state *st)
+{
+ const unsigned divider = st->chip_config.divider + 1;
+ s64 ts;
+
+ /* return current data timestamp and increment */
+ ts = st->data_timestamp;
+ st->data_timestamp += st->chip_period * divider;
+
+ return ts;
+}
+
int inv_reset_fifo(struct iio_dev *indio_dev)
{
int result;
u8 d;
struct inv_mpu6050_state *st = iio_priv(indio_dev);
+ /* reset it timestamp validation */
+ st->it_timestamp = 0;
+
/* disable interrupt */
result = regmap_write(st->map, st->reg->int_enable, 0);
if (result) {
@@ -97,7 +171,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
int result;
u8 data[INV_MPU6050_OUTPUT_DATA_SIZE];
u16 fifo_count;
- s64 timestamp = pf->timestamp;
+ s64 timestamp;
int int_status;
size_t i, nb;
@@ -140,6 +214,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
fifo_count = get_unaligned_be16(&data[0]);
/* compute and process all complete datum */
nb = fifo_count / bytes_per_datum;
+ inv_mpu6050_add_timestamp(st, pf->timestamp, nb);
for (i = 0; i < nb; ++i) {
result = regmap_bulk_read(st->map, st->reg->fifo_r_w,
data, bytes_per_datum);
@@ -150,6 +225,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
st->skip_samples--;
continue;
}
+ timestamp = inv_mpu6050_get_timestamp(st);
iio_push_to_buffers_with_timestamp(indio_dev, data, timestamp);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 5/5] iio: imu: inv_mpu6050: new timestamp mechanism
2018-05-14 21:05 ` [PATCH 5/5] iio: imu: inv_mpu6050: new timestamp mechanism Jean-Baptiste Maneyrol
@ 2018-05-15 19:31 ` Martin Kelly
2018-05-16 7:33 ` Jean-Baptiste Maneyrol
0 siblings, 1 reply; 14+ messages in thread
From: Martin Kelly @ 2018-05-15 19:31 UTC (permalink / raw)
To: Jean-Baptiste Maneyrol, linux-iio
On 05/14/2018 02:05 PM, Jean-Baptiste Maneyrol wrote:
> Check validity of interrupt timestamps by computing time between
> 2 interrupts. If it matches the chip frequency modulo 4%, it is
> used as the data timestamp and also for estimating the chip
> frequency measured from the system. Otherwise timestamp is
> computed using the estimated chip frequency.
>
> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
> ---
> drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 7 +++
> drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h | 8 +++
> drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 78 +++++++++++++++++++++++++++++-
> 3 files changed, 92 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> index 9e5c5e7..ade6294 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> @@ -295,6 +295,13 @@ static int inv_mpu6050_init_config(struct iio_dev *indio_dev)
> memcpy(&st->chip_config, hw_info[st->chip_type].config,
> sizeof(struct inv_mpu6050_chip_config));
>
> + /*
> + * Internal chip period is 1ms (1kHz).
> + * Let's use at the beginning the theorical value before measuring
> + * with interrupt timestamps.
> + */
> + st->chip_period = NSEC_PER_MSEC;
> +
> return inv_mpu6050_set_power_itg(st, false);
>
> error_power_off:
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> index 09a7e1c..b78640c 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> @@ -125,6 +125,9 @@ struct inv_mpu6050_hw {
> * @map regmap pointer.
> * @irq interrupt number.
> * @irq_mask the int_pin_cfg mask to configure interrupt type.
> + * @chip_period: chip internal period estimation (~1kHz).
> + * @it_timestamp: timestamp from previous interrupt.
> + * @data_timestamp: timestamp for next data sample.
> */
> struct inv_mpu6050_state {
> struct mutex lock;
> @@ -142,6 +145,9 @@ struct inv_mpu6050_state {
> int irq;
> u8 irq_mask;
> unsigned skip_samples;
> + s64 chip_period;
> + s64 it_timestamp;
> + s64 data_timestamp;
> };
>
> /*register and associated bit definition*/
> @@ -223,6 +229,8 @@ struct inv_mpu6050_state {
> #define INV_MPU6050_LATCH_INT_EN 0x20
> #define INV_MPU6050_BIT_BYPASS_EN 0x2
>
> +/* Allowed timestamp period jitter in percent */
> +#define INV_MPU6050_TS_PERIOD_JITTER 4
>
> /* init parameters */
> #define INV_MPU6050_INIT_FIFO_RATE 50
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> index 7a4aaed..e06e509 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> @@ -23,12 +23,86 @@
> #include <asm/unaligned.h>
> #include "inv_mpu_iio.h"
>
> +/**
> + * inv_mpu6050_add_timestamp() - Add a new interrupt timestamp
> + *
> + * @st: driver state
> + * @timestamp: the interrupt timestamp
> + * @nb: number of data set in the fifo
> + *
> + * This function uses interrupt timestamps to estimate the chip period and
> + * to choose the data timestamp to come.
> + */
> +static void inv_mpu6050_add_timestamp(struct inv_mpu6050_state *st,
> + s64 timestamp, size_t nb)
I think it would be clearer if this were called something like
inv_mpu6050_update_ts_estimate(), as we're not really attaching a
timestamp to a sample here (what I first thought) but are just updating
our estimation data.
> +{
> + /* Period boundaries for accepting timestamp */
> + const s64 period_min =
> + (NSEC_PER_MSEC * (100 - INV_MPU6050_TS_PERIOD_JITTER)) / 100;
> + const s64 period_max =
> + (NSEC_PER_MSEC * (100 + INV_MPU6050_TS_PERIOD_JITTER)) / 100;
> + const unsigned divider = st->chip_config.divider + 1;
I think this variable is better named fifo_rate to avoid confusion with
st->chip_config.divider (and if we refactor to calculate it with a
function, we should call that function here).
> + s64 val;
> + bool use_it_timestamp = false;
> +
> + if (st->it_timestamp == 0) > + /* not initialized, forced to use it timestamp */
nit: should probably be "forced to use it_timestamp"
> + use_it_timestamp = true;
> + } else if (nb == 1) {
> + /*
> + * Validate the use of it timestamp by checking if interrupt
> + * has been delayed.
> + * nb > 1 means interrupt was delayed for more than 1 sample,
> + * so it's obviously not good.
> + * Compute the chip period between 2 interrupts for validating.
> + */
> + val = (timestamp - st->it_timestamp) / divider;
> + if (val > period_min && val < period_max) {
> + /* update chip period and use it timestamp */
> + st->chip_period = (st->chip_period + val) / 2;
> + use_it_timestamp = true;
> + }
> + }
> +
> + if (use_it_timestamp) {
> + /* manage case of multiple samples in the fifo (nb > 1) */
> + val = (nb - 1) * st->chip_period * divider;
> + st->data_timestamp = timestamp - val;
Since this is complex code, I think naming "val" something more
descriptive would be helpful for understanding. A would suggest a name
like "gap_estimate", but others would work too.
> + }
> +
> + /* save it timestamp */
> + st->it_timestamp = timestamp;
> +}
> +
> +/**
> + * inv_mpu6050_get_timestamp() - Return the current data timestamp
> + *
> + * @st: driver state
> + * @return: current data timestamp
> + *
> + * This function returns the current data timestamp and prepares for next one.
> + */
> +static s64 inv_mpu6050_get_timestamp(struct inv_mpu6050_state *st)
> +{
> + const unsigned divider = st->chip_config.divider + 1;
Same here; I think this should be called fifo_rate.
> + s64 ts;
> +
> + /* return current data timestamp and increment */
> + ts = st->data_timestamp;
> + st->data_timestamp += st->chip_period * divider;
> +
> + return ts;
> +}
> +
> int inv_reset_fifo(struct iio_dev *indio_dev)
> {
> int result;
> u8 d;
> struct inv_mpu6050_state *st = iio_priv(indio_dev);
>
> + /* reset it timestamp validation */
> + st->it_timestamp = 0;
> +
> /* disable interrupt */
> result = regmap_write(st->map, st->reg->int_enable, 0);
> if (result) {
> @@ -97,7 +171,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
> int result;
> u8 data[INV_MPU6050_OUTPUT_DATA_SIZE];
> u16 fifo_count;
> - s64 timestamp = pf->timestamp;
> + s64 timestamp;
> int int_status;
> size_t i, nb;
>
> @@ -140,6 +214,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
> fifo_count = get_unaligned_be16(&data[0]);
> /* compute and process all complete datum */
> nb = fifo_count / bytes_per_datum;
> + inv_mpu6050_add_timestamp(st, pf->timestamp, nb);
> for (i = 0; i < nb; ++i) {
> result = regmap_bulk_read(st->map, st->reg->fifo_r_w,
> data, bytes_per_datum);
> @@ -150,6 +225,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
> st->skip_samples--;
> continue;
> }
> + timestamp = inv_mpu6050_get_timestamp(st);
> iio_push_to_buffers_with_timestamp(indio_dev, data, timestamp);
> }
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 5/5] iio: imu: inv_mpu6050: new timestamp mechanism
2018-05-15 19:31 ` Martin Kelly
@ 2018-05-16 7:33 ` Jean-Baptiste Maneyrol
2018-05-16 16:27 ` Martin Kelly
0 siblings, 1 reply; 14+ messages in thread
From: Jean-Baptiste Maneyrol @ 2018-05-16 7:33 UTC (permalink / raw)
To: Martin Kelly, linux-iio
On 15/05/2018 21:31, Martin Kelly wrote:
> CAUTION: This email originated from outside of the organization. Please
> make sure the sender is who they say they are and do not click links or
> open attachments unless you recognize the sender and know the content is
> safe.
>
>
> On 05/14/2018 02:05 PM, Jean-Baptiste Maneyrol wrote:
>> Check validity of interrupt timestamps by computing time between
>> 2 interrupts. If it matches the chip frequency modulo 4%, it is
>> used as the data timestamp and also for estimating the chip
>> frequency measured from the system. Otherwise timestamp is
>> computed using the estimated chip frequency.
>>
>> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
>> ---
>> drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 7 +++
>> drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h | 8 +++
>> drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 78
>> +++++++++++++++++++++++++++++-
>> 3 files changed, 92 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>> index 9e5c5e7..ade6294 100644
>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>> @@ -295,6 +295,13 @@ static int inv_mpu6050_init_config(struct iio_dev
>> *indio_dev)
>> memcpy(&st->chip_config, hw_info[st->chip_type].config,
>> sizeof(struct inv_mpu6050_chip_config));
>>
>> + /*
>> + * Internal chip period is 1ms (1kHz).
>> + * Let's use at the beginning the theorical value before measuring
>> + * with interrupt timestamps.
>> + */
>> + st->chip_period = NSEC_PER_MSEC;
>> +
>> return inv_mpu6050_set_power_itg(st, false);
>>
>> error_power_off:
>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>> index 09a7e1c..b78640c 100644
>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>> @@ -125,6 +125,9 @@ struct inv_mpu6050_hw {
>> * @map regmap pointer.
>> * @irq interrupt number.
>> * @irq_mask the int_pin_cfg mask to configure
>> interrupt type.
>> + * @chip_period: chip internal period estimation (~1kHz).
>> + * @it_timestamp: timestamp from previous interrupt.
>> + * @data_timestamp: timestamp for next data sample.
>> */
>> struct inv_mpu6050_state {
>> struct mutex lock;
>> @@ -142,6 +145,9 @@ struct inv_mpu6050_state {
>> int irq;
>> u8 irq_mask;
>> unsigned skip_samples;
>> + s64 chip_period;
>> + s64 it_timestamp;
>> + s64 data_timestamp;
>> };
>>
>> /*register and associated bit definition*/
>> @@ -223,6 +229,8 @@ struct inv_mpu6050_state {
>> #define INV_MPU6050_LATCH_INT_EN 0x20
>> #define INV_MPU6050_BIT_BYPASS_EN 0x2
>>
>> +/* Allowed timestamp period jitter in percent */
>> +#define INV_MPU6050_TS_PERIOD_JITTER 4
>>
>> /* init parameters */
>> #define INV_MPU6050_INIT_FIFO_RATE 50
>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
>> index 7a4aaed..e06e509 100644
>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
>> @@ -23,12 +23,86 @@
>> #include <asm/unaligned.h>
>> #include "inv_mpu_iio.h"
>>
>> +/**
>> + * inv_mpu6050_add_timestamp() - Add a new interrupt timestamp
>> + *
>> + * @st: driver state
>> + * @timestamp: the interrupt timestamp
>> + * @nb: number of data set in the fifo
>> + *
>> + * This function uses interrupt timestamps to estimate the chip
>> period and
>> + * to choose the data timestamp to come.
>> + */
>> +static void inv_mpu6050_add_timestamp(struct inv_mpu6050_state *st,
>> + s64 timestamp, size_t nb)
>
> I think it would be clearer if this were called something like
> inv_mpu6050_update_ts_estimate(), as we're not really attaching a
> timestamp to a sample here (what I first thought) but are just updating
> our estimation data.
In fact, we are adding a new timestamp for estimating the chip internal
frequency. But a better name would be good I agree.
>
>> +{
>> + /* Period boundaries for accepting timestamp */
>> + const s64 period_min =
>> + (NSEC_PER_MSEC * (100 - INV_MPU6050_TS_PERIOD_JITTER)) /
>> 100;
>> + const s64 period_max =
>> + (NSEC_PER_MSEC * (100 + INV_MPU6050_TS_PERIOD_JITTER)) /
>> 100;
>> + const unsigned divider = st->chip_config.divider + 1;
>
> I think this variable is better named fifo_rate to avoid confusion with
> st->chip_config.divider (and if we refactor to calculate it with a
> function, we should call that function here).
This is not fifo rate, but the real frequency divider, which is chip
divider + 1. Here fifo_rate = 1KHz / divider. If you have an idea about
a better name, I'm interested.
>
>> + s64 val;
>> + bool use_it_timestamp = false;
>> +
>> + if (st->it_timestamp == 0) > + /* not initialized,
>> forced to use it timestamp */
>
> nit: should probably be "forced to use it_timestamp"
it_timestamp is the it (means interrupt) timestamp.
>
>> + use_it_timestamp = true;
>> + } else if (nb == 1) {
>> + /*
>> + * Validate the use of it timestamp by checking if
>> interrupt
>> + * has been delayed.
>> + * nb > 1 means interrupt was delayed for more than 1
>> sample,
>> + * so it's obviously not good.
>> + * Compute the chip period between 2 interrupts for
>> validating.
>> + */
>> + val = (timestamp - st->it_timestamp) / divider;
>> + if (val > period_min && val < period_max) {
>> + /* update chip period and use it timestamp */
>> + st->chip_period = (st->chip_period + val) / 2;
>> + use_it_timestamp = true;
>> + }
>> + }
>> +
>> + if (use_it_timestamp) {
>> + /* manage case of multiple samples in the fifo (nb > 1) */
>> + val = (nb - 1) * st->chip_period * divider;
>> + st->data_timestamp = timestamp - val;
>
> Since this is complex code, I think naming "val" something more
> descriptive would be helpful for understanding. A would suggest a name
> like "gap_estimate", but others would work too.
I can declare 2 variables with better names instead.
>
>> + }
>> +
>> + /* save it timestamp */
>> + st->it_timestamp = timestamp;
>> +}
>> +
>> +/**
>> + * inv_mpu6050_get_timestamp() - Return the current data timestamp
>> + *
>> + * @st: driver state
>> + * @return: current data timestamp
>> + *
>> + * This function returns the current data timestamp and prepares for
>> next one.
>> + */
>> +static s64 inv_mpu6050_get_timestamp(struct inv_mpu6050_state *st)
>> +{
>> + const unsigned divider = st->chip_config.divider + 1;
>
> Same here; I think this should be called fifo_rate.
Real frequency divider like above.
>
>> + s64 ts;
>> +
>> + /* return current data timestamp and increment */
>> + ts = st->data_timestamp;
>> + st->data_timestamp += st->chip_period * divider;
>> +
>> + return ts;
>> +}
>> +
>> int inv_reset_fifo(struct iio_dev *indio_dev)
>> {
>> int result;
>> u8 d;
>> struct inv_mpu6050_state *st = iio_priv(indio_dev);
>>
>> + /* reset it timestamp validation */
>> + st->it_timestamp = 0;
>> +
>> /* disable interrupt */
>> result = regmap_write(st->map, st->reg->int_enable, 0);
>> if (result) {
>> @@ -97,7 +171,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
>> int result;
>> u8 data[INV_MPU6050_OUTPUT_DATA_SIZE];
>> u16 fifo_count;
>> - s64 timestamp = pf->timestamp;
>> + s64 timestamp;
>> int int_status;
>> size_t i, nb;
>>
>> @@ -140,6 +214,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
>> fifo_count = get_unaligned_be16(&data[0]);
>> /* compute and process all complete datum */
>> nb = fifo_count / bytes_per_datum;
>> + inv_mpu6050_add_timestamp(st, pf->timestamp, nb);
>> for (i = 0; i < nb; ++i) {
>> result = regmap_bulk_read(st->map, st->reg->fifo_r_w,
>> data, bytes_per_datum);
>> @@ -150,6 +225,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
>> st->skip_samples--;
>> continue;
>> }
>> + timestamp = inv_mpu6050_get_timestamp(st);
>> iio_push_to_buffers_with_timestamp(indio_dev, data,
>> timestamp);
>> }
>>
>>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 5/5] iio: imu: inv_mpu6050: new timestamp mechanism
2018-05-16 7:33 ` Jean-Baptiste Maneyrol
@ 2018-05-16 16:27 ` Martin Kelly
0 siblings, 0 replies; 14+ messages in thread
From: Martin Kelly @ 2018-05-16 16:27 UTC (permalink / raw)
To: Jean-Baptiste Maneyrol, linux-iio
On 05/16/2018 12:33 AM, Jean-Baptiste Maneyrol wrote:
>
>
> On 15/05/2018 21:31, Martin Kelly wrote:
>> CAUTION: This email originated from outside of the organization.
>> Please make sure the sender is who they say they are and do not click
>> links or open attachments unless you recognize the sender and know the
>> content is safe.
>>
>>
>> On 05/14/2018 02:05 PM, Jean-Baptiste Maneyrol wrote:
>>> Check validity of interrupt timestamps by computing time between
>>> 2 interrupts. If it matches the chip frequency modulo 4%, it is
>>> used as the data timestamp and also for estimating the chip
>>> frequency measured from the system. Otherwise timestamp is
>>> computed using the estimated chip frequency.
>>>
>>> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
>>> ---
>>> drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 7 +++
>>> drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h | 8 +++
>>> drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 78
>>> +++++++++++++++++++++++++++++-
>>> 3 files changed, 92 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>>> index 9e5c5e7..ade6294 100644
>>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>>> @@ -295,6 +295,13 @@ static int inv_mpu6050_init_config(struct
>>> iio_dev *indio_dev)
>>> memcpy(&st->chip_config, hw_info[st->chip_type].config,
>>> sizeof(struct inv_mpu6050_chip_config));
>>>
>>> + /*
>>> + * Internal chip period is 1ms (1kHz).
>>> + * Let's use at the beginning the theorical value before measuring
>>> + * with interrupt timestamps.
>>> + */
>>> + st->chip_period = NSEC_PER_MSEC;
>>> +
>>> return inv_mpu6050_set_power_itg(st, false);
>>>
>>> error_power_off:
>>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>>> index 09a7e1c..b78640c 100644
>>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>>> @@ -125,6 +125,9 @@ struct inv_mpu6050_hw {
>>> * @map regmap pointer.
>>> * @irq interrupt number.
>>> * @irq_mask the int_pin_cfg mask to configure
>>> interrupt type.
>>> + * @chip_period: chip internal period estimation (~1kHz).
>>> + * @it_timestamp: timestamp from previous interrupt.
>>> + * @data_timestamp: timestamp for next data sample.
>>> */
>>> struct inv_mpu6050_state {
>>> struct mutex lock;
>>> @@ -142,6 +145,9 @@ struct inv_mpu6050_state {
>>> int irq;
>>> u8 irq_mask;
>>> unsigned skip_samples;
>>> + s64 chip_period;
>>> + s64 it_timestamp;
>>> + s64 data_timestamp;
>>> };
>>>
>>> /*register and associated bit definition*/
>>> @@ -223,6 +229,8 @@ struct inv_mpu6050_state {
>>> #define INV_MPU6050_LATCH_INT_EN 0x20
>>> #define INV_MPU6050_BIT_BYPASS_EN 0x2
>>>
>>> +/* Allowed timestamp period jitter in percent */
>>> +#define INV_MPU6050_TS_PERIOD_JITTER 4
>>>
>>> /* init parameters */
>>> #define INV_MPU6050_INIT_FIFO_RATE 50
>>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
>>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
>>> index 7a4aaed..e06e509 100644
>>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
>>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
>>> @@ -23,12 +23,86 @@
>>> #include <asm/unaligned.h>
>>> #include "inv_mpu_iio.h"
>>>
>>> +/**
>>> + * inv_mpu6050_add_timestamp() - Add a new interrupt timestamp
>>> + *
>>> + * @st: driver state
>>> + * @timestamp: the interrupt timestamp
>>> + * @nb: number of data set in the fifo
>>> + *
>>> + * This function uses interrupt timestamps to estimate the chip
>>> period and
>>> + * to choose the data timestamp to come.
>>> + */
>>> +static void inv_mpu6050_add_timestamp(struct inv_mpu6050_state *st,
>>> + s64 timestamp, size_t nb)
>>
>> I think it would be clearer if this were called something like
>> inv_mpu6050_update_ts_estimate(), as we're not really attaching a
>> timestamp to a sample here (what I first thought) but are just updating
>> our estimation data.
> In fact, we are adding a new timestamp for estimating the chip internal
> frequency. But a better name would be good I agree.
>
>>
>>> +{
>>> + /* Period boundaries for accepting timestamp */
>>> + const s64 period_min =
>>> + (NSEC_PER_MSEC * (100 - INV_MPU6050_TS_PERIOD_JITTER))
>>> / 100;
>>> + const s64 period_max =
>>> + (NSEC_PER_MSEC * (100 + INV_MPU6050_TS_PERIOD_JITTER))
>>> / 100;
>>> + const unsigned divider = st->chip_config.divider + 1;
>>
>> I think this variable is better named fifo_rate to avoid confusion with
>> st->chip_config.divider (and if we refactor to calculate it with a
>> function, we should call that function here).
> This is not fifo rate, but the real frequency divider, which is chip
> divider + 1. Here fifo_rate = 1KHz / divider. If you have an idea about
> a better name, I'm interested.
>
A few ideas:
- Rename st->chip_config.divider to st->chip_config.freq_divider (and
set it to 1 higher). When setting it in the chip registers, subtract 1.
This would avoid the repeated code of "st->chip_config.divider + 1" and
make it clearer exactly what the divider is.
- Call the variable freq_divider (as opposed to chip_divider, say).
- Use a get_freq_divider() function to get the frequency divider from
the st->chip_config.divider (the chip divider), with a comment in the
function explaining how the two are 1 different from each other.
I think basically anything that lets the reader understand the
relationship between the dividers without combing through the datasheet
would be helpful.
>>
>>> + s64 val;
>>> + bool use_it_timestamp = false;
>>> +
>>> + if (st->it_timestamp == 0) > + /* not initialized,
>>> forced to use it timestamp */
>>
>> nit: should probably be "forced to use it_timestamp"
> it_timestamp is the it (means interrupt) timestamp.
>
I was actually just referring to "it timestamp" vs "it_timestamp" (since
"it_timestamp" is the variable). That said, as a small change, I think
"int_timestamp" would be clearer than "it_timestamp".
>>
>>> + use_it_timestamp = true;
>>> + } else if (nb == 1) {
>>> + /*
>>> + * Validate the use of it timestamp by checking if
>>> interrupt
>>> + * has been delayed.
>>> + * nb > 1 means interrupt was delayed for more than 1
>>> sample,
>>> + * so it's obviously not good.
>>> + * Compute the chip period between 2 interrupts for
>>> validating.
>>> + */
>>> + val = (timestamp - st->it_timestamp) / divider;
>>> + if (val > period_min && val < period_max) {
>>> + /* update chip period and use it timestamp */
>>> + st->chip_period = (st->chip_period + val) / 2;
>>> + use_it_timestamp = true;
>>> + }
>>> + }
>>> +
>>> + if (use_it_timestamp) {
>>> + /* manage case of multiple samples in the fifo (nb > 1) */
>>> + val = (nb - 1) * st->chip_period * divider;
>>> + st->data_timestamp = timestamp - val;
>>
>> Since this is complex code, I think naming "val" something more
>> descriptive would be helpful for understanding. A would suggest a name
>> like "gap_estimate", but others would work too.
> I can declare 2 variables with better names instead.
>
That sounds good.
>>
>>> + }
>>> +
>>> + /* save it timestamp */
>>> + st->it_timestamp = timestamp;
>>> +}
>>> +
>>> +/**
>>> + * inv_mpu6050_get_timestamp() - Return the current data timestamp
>>> + *
>>> + * @st: driver state
>>> + * @return: current data timestamp
>>> + *
>>> + * This function returns the current data timestamp and prepares
>>> for next one.
>>> + */
>>> +static s64 inv_mpu6050_get_timestamp(struct inv_mpu6050_state *st)
>>> +{
>>> + const unsigned divider = st->chip_config.divider + 1;
>>
>> Same here; I think this should be called fifo_rate.
> Real frequency divider like above.
>
>>
>>> + s64 ts;
>>> +
>>> + /* return current data timestamp and increment */
>>> + ts = st->data_timestamp;
>>> + st->data_timestamp += st->chip_period * divider;
>>> +
>>> + return ts;
>>> +}
>>> +
>>> int inv_reset_fifo(struct iio_dev *indio_dev)
>>> {
>>> int result;
>>> u8 d;
>>> struct inv_mpu6050_state *st = iio_priv(indio_dev);
>>>
>>> + /* reset it timestamp validation */
>>> + st->it_timestamp = 0;
>>> +
>>> /* disable interrupt */
>>> result = regmap_write(st->map, st->reg->int_enable, 0);
>>> if (result) {
>>> @@ -97,7 +171,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
>>> int result;
>>> u8 data[INV_MPU6050_OUTPUT_DATA_SIZE];
>>> u16 fifo_count;
>>> - s64 timestamp = pf->timestamp;
>>> + s64 timestamp;
>>> int int_status;
>>> size_t i, nb;
>>>
>>> @@ -140,6 +214,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
>>> fifo_count = get_unaligned_be16(&data[0]);
>>> /* compute and process all complete datum */
>>> nb = fifo_count / bytes_per_datum;
>>> + inv_mpu6050_add_timestamp(st, pf->timestamp, nb);
>>> for (i = 0; i < nb; ++i) {
>>> result = regmap_bulk_read(st->map, st->reg->fifo_r_w,
>>> data, bytes_per_datum);
>>> @@ -150,6 +225,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
>>> st->skip_samples--;
>>> continue;
>>> }
>>> + timestamp = inv_mpu6050_get_timestamp(st);
>>> iio_push_to_buffers_with_timestamp(indio_dev, data,
>>> timestamp);
>>> }
>>>
>>>
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2018-05-20 10:20 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-14 21:05 [PATCH 1/5] iio: imu: inv_mpu6050: replace timestamp fifo by generic timestamp Jean-Baptiste Maneyrol
2018-05-14 21:05 ` [PATCH 2/5] iio: imu: inv_mpu6050: switch to use sample rate divider Jean-Baptiste Maneyrol
2018-05-15 19:08 ` Martin Kelly
2018-05-16 7:08 ` Jean-Baptiste Maneyrol
2018-05-16 16:19 ` Martin Kelly
2018-05-14 21:05 ` [PATCH 3/5] iio: imu: inv_mpu6050: fix fifo count reading Jean-Baptiste Maneyrol
2018-05-15 19:09 ` Martin Kelly
2018-05-20 10:20 ` Jonathan Cameron
2018-05-14 21:05 ` [PATCH 4/5] iio: imu: inv_mpu6050: better fifo overflow handling Jean-Baptiste Maneyrol
2018-05-15 19:09 ` Martin Kelly
2018-05-14 21:05 ` [PATCH 5/5] iio: imu: inv_mpu6050: new timestamp mechanism Jean-Baptiste Maneyrol
2018-05-15 19:31 ` Martin Kelly
2018-05-16 7:33 ` Jean-Baptiste Maneyrol
2018-05-16 16:27 ` Martin Kelly
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.