| Message ID | 20240219154741.90601-1-inv.git-commit@tdk.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] iio: imu: inv_mpu6050: fix frequency setting when chip is off | expand |
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 >
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
>
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 > > >
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;