| Message ID | 20210319133357.541183-1-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: imu: inv_mpu6050: Use as standalone trigger | expand |
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 >
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) ||
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
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) ||