iio: imu: inv_mpu6050: fix frequency setting when chip is off

PATCH Applied [email protected] 3 participants 4 messages Feb 19, 2024 – Feb 25, 2024

Subsystems (3)

Stable branch IIO Invensense mpu-6050 IMU driver

lore

Share & tools

lore

Jean-Baptiste Maneyrol

maintainer

2024-02-24 17:51 UTC #2 lore raw

Hello Jonathan,

the problem comes from the fifo rate setting code that is now using these states to compute if FIFO is on:

fifo_on = st->chip_config.accl_fifo_enable ||
          st->chip_config.gyro_fifo_enable ||
          st->chip_config.magn_fifo_enable;
result = inv_sensors_timestamp_update_odr(&st->timestamp, fifo_period, fifo_on);
if (result)
        goto fifo_rate_fail_unlock;

It was not the case before using the inv_sensors_timestamp module.

If we don't set these states back to false, it is not possible to change frequency more than 1 time. This restriction comes from inv_sensors_timestamp module that is not able to handle accurately more than 1 frequency change when sensor FIFO is on. This restriction obviously doesn't exist when FIFO is off.

Thanks,
JB

From: Jonathan Cameron <[email protected]>
Sent: Saturday, February 24, 2024 17:22
To: INV Git Commit <[email protected]>
Cc: [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; Jean-Baptiste Maneyrol <[email protected]>
Subject: Re: [PATCH v2] iio: imu: inv_mpu6050: fix frequency setting when chip is off

On Mon, 19 Feb 2024 15: 47: 41 +0000 inv. git-commit@ tdk. com wrote: > From: Jean-Baptiste Maneyrol <jean-baptiste. maneyrol@ tdk. com> > > Track correctly FIFO state and apply ODR change before starting > the chip. Without the fix, ZjQcmQRYFpfptBannerStart
This Message Is From an External Sender
This message came from outside your organization.

ZjQcmQRYFpfptBannerEnd

Quoted 5 lines · On Mon, 19 Feb 2024 15:47:41 +0000 [email protected] wrote:

From: Jean-Baptiste Maneyrol <[email protected]>
Track correctly FIFO state and apply ODR change before starting the chip. Without the fix, you cannot change ODR more than 1 time when data buffering is off.

Hi Jean-Baptiste.

I think this patch needs a little more explanation.
1) Why has state changed such that we need to update these cached values?
2) What does that have to do with ODR - or is this two unrelated fixes?

I'm sure the fix is right - I just don't fully understand the issues!

Quoted 23 lines

Fixes 111e1abd0045 ("iio: imu: inv_mpu6050: use the common inv_sensors timestamp module")

Cc [email protected]

Signed-off-by Jean-Baptiste Maneyrol <[email protected]>

---
V2: add missing stable tag

 drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c | 5 +++++
 1 file changed, 5 insertions(+)

drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c +1 -0

index 676704f9151f..e6e6e94452a3 100644
@@ -111,6 +111,7 @@ int inv_mpu6050_prepare_fifo(struct inv_mpu6050_state *st, bool enable)
 	if (enable) {
 		/* reset timestamping */
 		inv_sensors_timestamp_reset(&st->timestamp);
+		inv_sensors_timestamp_apply_odr(&st->timestamp, 0, 0, 0);
 		/* reset FIFO */
 		d = st->chip_config.user_ctrl | INV_MPU6050_BIT_FIFO_RST;
 		ret = regmap_write(st->map, st->reg->user_ctrl, d);

drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable) · line 191

Quoted 7 lines

drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c +4 -0

@@ -184,6 +185,10 @@ static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
 		if (result)
 			goto error_power_off;
 	} else {
+		st->chip_config.gyro_fifo_enable = 0;
+		st->chip_config.accl_fifo_enable = 0;
+		st->chip_config.temp_fifo_enable = 0;
+		st->chip_config.magn_fifo_enable = 0;

I think the write to actually do this is in prepare_fifo and it's not conditional on these, so why do we care?

Are these effectively paired with inv_scan_query in the enable path?

Quoted 6 lines

drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c +0 -1

 		result = inv_mpu6050_prepare_fifo(st, false);
 		if (result)
 			goto error_power_off;
--

2.34.1

Jonathan Cameron

maintainer Applied

2024-02-25 11:46 UTC #3 lore raw

Quoted 10 lines · On Sat, 24 Feb 2024 17:50:26 +0000 Jean-Baptiste Maneyrol…

Hello Jonathan,
the problem comes from the fifo rate setting code that is now using these states to compute if FIFO is on:
fifo_on = st->chip_config.accl_fifo_enable ||
          st->chip_config.gyro_fifo_enable ||
          st->chip_config.magn_fifo_enable;
result = inv_sensors_timestamp_update_odr(&st->timestamp, fifo_period, fifo_on);
if (result)
        goto fifo_rate_fail_unlock;

Quoted 3 lines

It was not the case before using the inv_sensors_timestamp module.
If we don't set these states back to false, it is not possible to change frequency more than 1 time. This restriction comes from inv_sensors_timestamp module that is not able to handle accurately more than 1 frequency change when sensor FIFO is on. This restriction obviously doesn't exist when FIFO is off.

Thanks, now I understand. I've added a note to say the restriction to a single pending update of ODR should only apply when the fifo is on. Along with a link to this thread that should be enough to help people understand the issue when consider whether to backport this fix.

Applied to the fixes-togreg branch of iio.git.

Thanks,

Jonathan

Quoted 76 lines

Thanks,
JB
From: Jonathan Cameron <[email protected]>
Sent: Saturday, February 24, 2024 17:22
To: INV Git Commit <[email protected]>
Cc: [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; Jean-Baptiste Maneyrol <[email protected]>
Subject: Re: [PATCH v2] iio: imu: inv_mpu6050: fix frequency setting when chip is off

On Mon, 19 Feb 2024 15: 47: 41 +0000 inv. git-commit@ tdk. com wrote: > From: Jean-Baptiste Maneyrol <jean-baptiste. maneyrol@ tdk. com> > > Track correctly FIFO state and apply ODR change before starting > the chip. Without the fix, ZjQcmQRYFpfptBannerStart
This Message Is From an External Sender
This message came from outside your organization.

ZjQcmQRYFpfptBannerEnd
Quoted 5 lines · On Mon, 19 Feb 2024 15:47:41 +0000 [email protected] wrote:
From: Jean-Baptiste Maneyrol <[email protected]>
Track correctly FIFO state and apply ODR change before starting the chip. Without the fix, you cannot change ODR more than 1 time when data buffering is off.
Hi Jean-Baptiste.
I think this patch needs a little more explanation.
1) Why has state changed such that we need to update these cached values?
2) What does that have to do with ODR - or is this two unrelated fixes?
I'm sure the fix is right - I just don't fully understand the issues!
Quoted 23 lines
Fixes 111e1abd0045 ("iio: imu: inv_mpu6050: use the common inv_sensors timestamp module")
Cc [email protected]
Signed-off-by Jean-Baptiste Maneyrol <[email protected]>
---
V2: add missing stable tag
 drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c | 5 +++++
 1 file changed, 5 insertions(+)
drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c +1 -0
index 676704f9151f..e6e6e94452a3 100644
@@ -111,6 +111,7 @@ int inv_mpu6050_prepare_fifo(struct inv_mpu6050_state *st, bool enable)
 	if (enable) {
 		/* reset timestamping */
 		inv_sensors_timestamp_reset(&st->timestamp);
+		inv_sensors_timestamp_apply_odr(&st->timestamp, 0, 0, 0);
 		/* reset FIFO */
 		d = st->chip_config.user_ctrl | INV_MPU6050_BIT_FIFO_RST;
 		ret = regmap_write(st->map, st->reg->user_ctrl, d);
drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable) · line 191
Quoted 7 lines
drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c +4 -0
@@ -184,6 +185,10 @@ static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
 		if (result)
 			goto error_power_off;
 	} else {
+		st->chip_config.gyro_fifo_enable = 0;
+		st->chip_config.accl_fifo_enable = 0;
+		st->chip_config.temp_fifo_enable = 0;
+		st->chip_config.magn_fifo_enable = 0;  
I think the write to actually do this is in prepare_fifo and it's not conditional on these, so why do we care?
Are these effectively paired with inv_scan_query in the enable path?
Quoted 6 lines
drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c +0 -1
 		result = inv_mpu6050_prepare_fifo(st, false);
 		if (result)
 			goto error_power_off;
--
2.34.1