[PATCH] iio: imu: inv_mpu6050: Use as standalone trigger

[PATCH] iio: imu: inv_mpu6050: Use as standalone trigger

[Linus Walleij]

It may happen that the MPU6050 is the only hardware
trigger available on your system such as this:

> lsiio
Device 003: hscdtd008a
Device 001: mpu6050
Device 002: gp2ap002
Device 000: ab8500-gpadc
Trigger 000: mpu6050-dev1

And when you want to use it to read periodically from
your magnetometer like this:

> iio_generic_buffer -a -c 100 -n hscdtd008a -t mpu6050-dev1

Then the following happens:

[  209.951334] Internal error: Oops: 5 [#1] SMP ARM
(...)
[  209.981969] Hardware name: ST-Ericsson Ux5x0 platform (Device Tree Support)
[  209.988925] PC is at inv_scan_query_mpu6050+0x8/0xb8
[  209.993914] LR is at inv_mpu6050_set_enable+0x40/0x194

This is because since we are not using any channels from the
same device, the indio_dev->active_scan_mask is NULL.

Just checking for that and bailing out is however not enough:
we have to enable some kind of FIFO for the readout to work.
So enable the temperature as a dummy FIFO and all works
fine.

Cc: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
index f7b5a70be30f..6946d50b874a 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
@@ -11,6 +11,17 @@ static unsigned int inv_scan_query_mpu6050(struct iio_dev *indio_dev)
 	struct inv_mpu6050_state  *st = iio_priv(indio_dev);
 	unsigned int mask;
 
+	/*
+	 * If the MPU6050 is just used as a trigger, then the scan mask
+	 * is not allocated so we simply enable the temperature channel
+	 * as a dummy and bail out.
+	 */
+	if (!indio_dev->active_scan_mask) {
+		st->chip_config.temp_fifo_enable = true;
+		mask = INV_MPU6050_SENSOR_TEMP;
+		return mask;
+	}
+
 	st->chip_config.gyro_fifo_enable =
 		test_bit(INV_MPU6050_SCAN_GYRO_X,
 			 indio_dev->active_scan_mask) ||
-- 
2.29.2

Re: [PATCH] iio: imu: inv_mpu6050: Use as standalone trigger

[Andy Shevchenko]

On Fri, Mar 19, 2021 at 3:35 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> It may happen that the MPU6050 is the only hardware
> trigger available on your system such as this:
>
> > lsiio
> Device 003: hscdtd008a
> Device 001: mpu6050
> Device 002: gp2ap002
> Device 000: ab8500-gpadc
> Trigger 000: mpu6050-dev1
>
> And when you want to use it to read periodically from
> your magnetometer like this:
>
> > iio_generic_buffer -a -c 100 -n hscdtd008a -t mpu6050-dev1
>
> Then the following happens:
>
> [  209.951334] Internal error: Oops: 5 [#1] SMP ARM
> (...)
> [  209.981969] Hardware name: ST-Ericsson Ux5x0 platform (Device Tree Support)
> [  209.988925] PC is at inv_scan_query_mpu6050+0x8/0xb8
> [  209.993914] LR is at inv_mpu6050_set_enable+0x40/0x194
>
> This is because since we are not using any channels from the
> same device, the indio_dev->active_scan_mask is NULL.
>
> Just checking for that and bailing out is however not enough:
> we have to enable some kind of FIFO for the readout to work.
> So enable the temperature as a dummy FIFO and all works
> fine.

Fixes tag?

> Cc: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> index f7b5a70be30f..6946d50b874a 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> @@ -11,6 +11,17 @@ static unsigned int inv_scan_query_mpu6050(struct iio_dev *indio_dev)
>         struct inv_mpu6050_state  *st = iio_priv(indio_dev);
>         unsigned int mask;
>
> +       /*
> +        * If the MPU6050 is just used as a trigger, then the scan mask
> +        * is not allocated so we simply enable the temperature channel
> +        * as a dummy and bail out.
> +        */
> +       if (!indio_dev->active_scan_mask) {
> +               st->chip_config.temp_fifo_enable = true;

> +               mask = INV_MPU6050_SENSOR_TEMP;
> +               return mask;

A nit-pick:
return INV...;

> +       }
> +
>         st->chip_config.gyro_fifo_enable =
>                 test_bit(INV_MPU6050_SCAN_GYRO_X,
>                          indio_dev->active_scan_mask) ||
> --
> 2.29.2
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: imu: inv_mpu6050: Use as standalone trigger

[Jonathan Cameron]

On Fri, 19 Mar 2021 14:33:57 +0100
Linus Walleij <linus.walleij@linaro.org> wrote:

> It may happen that the MPU6050 is the only hardware
> trigger available on your system such as this:
> 
> > lsiio  
> Device 003: hscdtd008a
> Device 001: mpu6050
> Device 002: gp2ap002
> Device 000: ab8500-gpadc
> Trigger 000: mpu6050-dev1
> 
> And when you want to use it to read periodically from
> your magnetometer like this:
> 
> > iio_generic_buffer -a -c 100 -n hscdtd008a -t mpu6050-dev1  
> 
> Then the following happens:
> 
> [  209.951334] Internal error: Oops: 5 [#1] SMP ARM
> (...)
> [  209.981969] Hardware name: ST-Ericsson Ux5x0 platform (Device Tree Support)
> [  209.988925] PC is at inv_scan_query_mpu6050+0x8/0xb8
> [  209.993914] LR is at inv_mpu6050_set_enable+0x40/0x194
> 
> This is because since we are not using any channels from the
> same device, the indio_dev->active_scan_mask is NULL.
> 
> Just checking for that and bailing out is however not enough:
> we have to enable some kind of FIFO for the readout to work.
> So enable the temperature as a dummy FIFO and all works
> fine.

Ah. This corner case. I have a suspicion a lot of drivers
suffer from this case.  I confess I'm often too lazy to
look at it in a great deal of depth because it's not a particularly
common thing to do - and we long ago dropped the equivalent
triggers that just used a periodic interrupt on the basis that
they didn't add any significant improvement in precision of
capture over an hrtimer based trigger.

Mind you we should definitely close down the bug either way.

Jonathan

> 
> Cc: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> index f7b5a70be30f..6946d50b874a 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> @@ -11,6 +11,17 @@ static unsigned int inv_scan_query_mpu6050(struct iio_dev *indio_dev)
>  	struct inv_mpu6050_state  *st = iio_priv(indio_dev);
>  	unsigned int mask;
>  
> +	/*
> +	 * If the MPU6050 is just used as a trigger, then the scan mask
> +	 * is not allocated so we simply enable the temperature channel
> +	 * as a dummy and bail out.
> +	 */
> +	if (!indio_dev->active_scan_mask) {
> +		st->chip_config.temp_fifo_enable = true;
> +		mask = INV_MPU6050_SENSOR_TEMP;
> +		return mask;
> +	}
> +
>  	st->chip_config.gyro_fifo_enable =
>  		test_bit(INV_MPU6050_SCAN_GYRO_X,
>  			 indio_dev->active_scan_mask) ||

Re: [PATCH] iio: imu: inv_mpu6050: Use as standalone trigger

[Linus Walleij]

On Sat, Mar 20, 2021 at 5:03 PM Jonathan Cameron <jic23@kernel.org> wrote:

> Ah. This corner case. I have a suspicion a lot of drivers
> suffer from this case.

Yeah :/

> I confess I'm often too lazy to
> look at it in a great deal of depth because it's not a particularly
> common thing to do - and we long ago dropped the equivalent
> triggers that just used a periodic interrupt on the basis that
> they didn't add any significant improvement in precision of
> capture over an hrtimer based trigger.

Heh, it works find to use an hrtimer trigger of course.
Just that I am too lazy to set it up in configfs so I go
and poke holes at the implementation by using some
other hardware as trigger like this :)

> Mind you we should definitely close down the bug either way.

I'll find some applicable Fixes: tag and fix Andy's
nitpick and resend as v2.

Yours,
Linus Walleij