[PATCH v2] iio: imu: inv_mpu6050: fix frequency setting when chip is off

[PATCH v2] iio: imu: inv_mpu6050: fix frequency setting when chip is off

[inv.git-commit]

From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>

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.

Fixes: 111e1abd0045 ("iio: imu: inv_mpu6050: use the common inv_sensors timestamp module")
Cc: stable@vger.kernel.org
Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
---
V2: add missing stable tag

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

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
index 676704f9151f..e6e6e94452a3 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
@@ -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);
@@ -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;
 		result = inv_mpu6050_prepare_fifo(st, false);
 		if (result)
 			goto error_power_off;
--
2.34.1

Re: [PATCH v2] iio: imu: inv_mpu6050: fix frequency setting when chip is off

[Jonathan Cameron]

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, 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!
> 
> Fixes: 111e1abd0045 ("iio: imu: inv_mpu6050: use the common inv_sensors timestamp module")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
> ---
> V2: add missing stable tag
> 
>  drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> index 676704f9151f..e6e6e94452a3 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> @@ -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);
> @@ -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?


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

Re: [PATCH v2] iio: imu: inv_mpu6050: fix frequency setting when chip is off

[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;

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 <jic23@kernel.org>
Sent: Saturday, February 24, 2024 17:22
To: INV Git Commit <INV.git-commit@tdk.com>
Cc: lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; stable@vger.kernel.org <stable@vger.kernel.org>; Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>
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
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, 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!
> 
> Fixes: 111e1abd0045 ("iio: imu: inv_mpu6050: use the common inv_sensors timestamp module")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
> ---
> V2: add missing stable tag
> 
>  drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> index 676704f9151f..e6e6e94452a3 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> @@ -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);
> @@ -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?


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

Re: [PATCH v2] iio: imu: inv_mpu6050: fix frequency setting when chip is off

[Jonathan Cameron]

On Sat, 24 Feb 2024 17:50:26 +0000
Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> wrote:

> 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, 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

> 
> Thanks,
> JB
> 
> 
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Saturday, February 24, 2024 17:22
> To: INV Git Commit <INV.git-commit@tdk.com>
> Cc: lars@metafoo.de <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; stable@vger.kernel.org <stable@vger.kernel.org>; Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>
> 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
> 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, 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!
> > 
> > Fixes: 111e1abd0045 ("iio: imu: inv_mpu6050: use the common inv_sensors timestamp module")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
> > ---
> > V2: add missing stable tag
> > 
> >  drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> > index 676704f9151f..e6e6e94452a3 100644
> > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> > @@ -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);
> > @@ -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?
> 
> 
> >  		result = inv_mpu6050_prepare_fifo(st, false);
> >  		if (result)
> >  			goto error_power_off;
> > --
> > 2.34.1
> >   
>