* [PATCH 1/5] iio: imu: inv_mpu6050: fix error path not turning chip back off
@ 2017-07-13 13:30 Jean-Baptiste Maneyrol
2017-07-15 11:44 ` Jonathan Cameron
0 siblings, 1 reply; 5+ messages in thread
From: Jean-Baptiste Maneyrol @ 2017-07-13 13:30 UTC (permalink / raw)
To: linux-iio
Some functions are turning the chip on and not back off in error
path. With set_power function using a reference counter this is
keeping the chip on forever.
Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
---
drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 12 ++++++++----
drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c | 23 +++++++++++++++--------
2 files changed, 23 insertions(+), 12 deletions(-)
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/i=
nv_mpu6050/inv_mpu_core.c
index 44830bc..b1a8159 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -262,27 +262,31 @@ static int inv_mpu6050_init_config(struct iio_dev *in=
dio_dev)
d =3D (INV_MPU6050_FSR_2000DPS << INV_MPU6050_GYRO_CONFIG_FSR_SHIFT);
result =3D regmap_write(st->map, st->reg->gyro_config, d);
if (result)
- return result;
+ goto error_power_off;
=20
result =3D inv_mpu6050_set_lpf_regs(st, INV_MPU6050_FILTER_20HZ);
if (result)
- return result;
+ goto error_power_off;
=20
d =3D INV_MPU6050_ONE_K_HZ / INV_MPU6050_INIT_FIFO_RATE - 1;
result =3D regmap_write(st->map, st->reg->sample_rate_div, d);
if (result)
- return result;
+ goto error_power_off;
=20
d =3D (INV_MPU6050_FS_02G << INV_MPU6050_ACCL_CONFIG_FSR_SHIFT);
result =3D regmap_write(st->map, st->reg->accl_config, d);
if (result)
- return result;
+ goto error_power_off;
=20
memcpy(&st->chip_config, hw_info[st->chip_type].config,
sizeof(struct inv_mpu6050_chip_config));
result =3D inv_mpu6050_set_power_itg(st, false);
=20
return result;
+
+error_power_off:
+ inv_mpu6050_set_power_itg(st, false);
+ return result;
}
=20
static int inv_mpu6050_sensor_set(struct inv_mpu6050_state *st, int reg,
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/im=
u/inv_mpu6050/inv_mpu_trigger.c
index 540070f..85c2732 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
@@ -53,45 +53,52 @@ static int inv_mpu6050_set_enable(struct iio_dev *indio=
_dev, bool enable)
result =3D inv_mpu6050_switch_engine(st, true,
INV_MPU6050_BIT_PWR_GYRO_STBY);
if (result)
- return result;
+ goto error_power_off;
}
if (st->chip_config.accl_fifo_enable) {
result =3D inv_mpu6050_switch_engine(st, true,
INV_MPU6050_BIT_PWR_ACCL_STBY);
if (result)
- return result;
+ goto error_power_off;
}
result =3D inv_reset_fifo(indio_dev);
if (result)
- return result;
+ goto error_power_off;
} else {
result =3D regmap_write(st->map, st->reg->fifo_en, 0);
if (result)
- return result;
+ goto error_power_off;
=20
result =3D regmap_write(st->map, st->reg->int_enable, 0);
if (result)
- return result;
+ goto error_power_off;
=20
result =3D regmap_write(st->map, st->reg->user_ctrl, 0);
if (result)
- return result;
+ goto error_power_off;
=20
result =3D inv_mpu6050_switch_engine(st, false,
INV_MPU6050_BIT_PWR_GYRO_STBY);
if (result)
- return result;
+ goto error_power_off;
+ st->chip_config.gyro_fifo_enable =3D false;
=20
result =3D inv_mpu6050_switch_engine(st, false,
INV_MPU6050_BIT_PWR_ACCL_STBY);
if (result)
- return result;
+ goto error_power_off;
+ st->chip_config.accl_fifo_enable =3D false;
+
result =3D inv_mpu6050_set_power_itg(st, false);
if (result)
return result;
}
=20
return 0;
+
+error_power_off:
+ inv_mpu6050_set_power_itg(st, false);
+ return result;
}
=20
/**
--=20
1.9.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH 1/5] iio: imu: inv_mpu6050: fix error path not turning chip back off
2017-07-13 13:30 [PATCH 1/5] iio: imu: inv_mpu6050: fix error path not turning chip back off Jean-Baptiste Maneyrol
@ 2017-07-15 11:44 ` Jonathan Cameron
2017-09-25 19:01 ` Jonathan Cameron
0 siblings, 1 reply; 5+ messages in thread
From: Jonathan Cameron @ 2017-07-15 11:44 UTC (permalink / raw)
To: Jean-Baptiste Maneyrol; +Cc: linux-iio
On Thu, 13 Jul 2017 13:30:04 +0000
Jean-Baptiste Maneyrol <JManeyrol@invensense.com> wrote:
> Some functions are turning the chip on and not back off in error
> path. With set_power function using a reference counter this is
> keeping the chip on forever.
>
> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
Good fix in general, but there is one line that seems to be doing
something else (marking the fifo disabled in a non error path).
May well be a good change, but it wants explaining and probably
to be in a separate patch.
Thanks,
Jonathan
> ---
> drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 12 ++++++++----
> drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c | 23 +++++++++++++++--------
> 2 files changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> index 44830bc..b1a8159 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> @@ -262,27 +262,31 @@ static int inv_mpu6050_init_config(struct iio_dev *indio_dev)
> d = (INV_MPU6050_FSR_2000DPS << INV_MPU6050_GYRO_CONFIG_FSR_SHIFT);
> result = regmap_write(st->map, st->reg->gyro_config, d);
> if (result)
> - return result;
> + goto error_power_off;
>
> result = inv_mpu6050_set_lpf_regs(st, INV_MPU6050_FILTER_20HZ);
> if (result)
> - return result;
> + goto error_power_off;
>
> d = INV_MPU6050_ONE_K_HZ / INV_MPU6050_INIT_FIFO_RATE - 1;
> result = regmap_write(st->map, st->reg->sample_rate_div, d);
> if (result)
> - return result;
> + goto error_power_off;
>
> d = (INV_MPU6050_FS_02G << INV_MPU6050_ACCL_CONFIG_FSR_SHIFT);
> result = regmap_write(st->map, st->reg->accl_config, d);
> if (result)
> - return result;
> + goto error_power_off;
>
> memcpy(&st->chip_config, hw_info[st->chip_type].config,
> sizeof(struct inv_mpu6050_chip_config));
> result = inv_mpu6050_set_power_itg(st, false);
>
> return result;
> +
> +error_power_off:
> + inv_mpu6050_set_power_itg(st, false);
> + return result;
> }
>
> static int inv_mpu6050_sensor_set(struct inv_mpu6050_state *st, int reg,
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> index 540070f..85c2732 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> @@ -53,45 +53,52 @@ static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
> result = inv_mpu6050_switch_engine(st, true,
> INV_MPU6050_BIT_PWR_GYRO_STBY);
> if (result)
> - return result;
> + goto error_power_off;
> }
> if (st->chip_config.accl_fifo_enable) {
> result = inv_mpu6050_switch_engine(st, true,
> INV_MPU6050_BIT_PWR_ACCL_STBY);
> if (result)
> - return result;
> + goto error_power_off;
> }
> result = inv_reset_fifo(indio_dev);
> if (result)
> - return result;
> + goto error_power_off;
> } else {
> result = regmap_write(st->map, st->reg->fifo_en, 0);
> if (result)
> - return result;
> + goto error_power_off;
>
> result = regmap_write(st->map, st->reg->int_enable, 0);
> if (result)
> - return result;
> + goto error_power_off;
>
> result = regmap_write(st->map, st->reg->user_ctrl, 0);
> if (result)
> - return result;
> + goto error_power_off;
>
> result = inv_mpu6050_switch_engine(st, false,
> INV_MPU6050_BIT_PWR_GYRO_STBY);
> if (result)
> - return result;
> + goto error_power_off;
> + st->chip_config.gyro_fifo_enable = false;
>
> result = inv_mpu6050_switch_engine(st, false,
> INV_MPU6050_BIT_PWR_ACCL_STBY);
> if (result)
> - return result;
> + goto error_power_off;
> + st->chip_config.accl_fifo_enable = false;
This one line doesn't fit with what the the patch is described
as doing. Looks like an independent change.
Could you explain why it is here please?
> +
> result = inv_mpu6050_set_power_itg(st, false);
> if (result)
> return result;
> }
>
> return 0;
> +
> +error_power_off:
> + inv_mpu6050_set_power_itg(st, false);
> + return result;
> }
>
> /**
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 1/5] iio: imu: inv_mpu6050: fix error path not turning chip back off
2017-07-15 11:44 ` Jonathan Cameron
@ 2017-09-25 19:01 ` Jonathan Cameron
2017-09-26 12:06 ` Jean-Baptiste Maneyrol
0 siblings, 1 reply; 5+ messages in thread
From: Jonathan Cameron @ 2017-09-25 19:01 UTC (permalink / raw)
To: Jean-Baptiste Maneyrol; +Cc: linux-iio
On Sat, 15 Jul 2017 12:44:47 +0100
Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote:
> On Thu, 13 Jul 2017 13:30:04 +0000
> Jean-Baptiste Maneyrol <JManeyrol@invensense.com> wrote:
>
> > Some functions are turning the chip on and not back off in error
> > path. With set_power function using a reference counter this is
> > keeping the chip on forever.
> >
> > Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
> Good fix in general, but there is one line that seems to be doing
> something else (marking the fifo disabled in a non error path).
> May well be a good change, but it wants explaining and probably
> to be in a separate patch.
Hi,
I see that a new version of this series is still to come.
Any idea when/if you will get to it?
Thanks,
Jonathan
>
> Thanks,
>
> Jonathan
> > ---
> > drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 12 ++++++++----
> > drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c | 23 +++++++++++++++--------
> > 2 files changed, 23 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> > index 44830bc..b1a8159 100644
> > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> > @@ -262,27 +262,31 @@ static int inv_mpu6050_init_config(struct iio_dev *indio_dev)
> > d = (INV_MPU6050_FSR_2000DPS << INV_MPU6050_GYRO_CONFIG_FSR_SHIFT);
> > result = regmap_write(st->map, st->reg->gyro_config, d);
> > if (result)
> > - return result;
> > + goto error_power_off;
> >
> > result = inv_mpu6050_set_lpf_regs(st, INV_MPU6050_FILTER_20HZ);
> > if (result)
> > - return result;
> > + goto error_power_off;
> >
> > d = INV_MPU6050_ONE_K_HZ / INV_MPU6050_INIT_FIFO_RATE - 1;
> > result = regmap_write(st->map, st->reg->sample_rate_div, d);
> > if (result)
> > - return result;
> > + goto error_power_off;
> >
> > d = (INV_MPU6050_FS_02G << INV_MPU6050_ACCL_CONFIG_FSR_SHIFT);
> > result = regmap_write(st->map, st->reg->accl_config, d);
> > if (result)
> > - return result;
> > + goto error_power_off;
> >
> > memcpy(&st->chip_config, hw_info[st->chip_type].config,
> > sizeof(struct inv_mpu6050_chip_config));
> > result = inv_mpu6050_set_power_itg(st, false);
> >
> > return result;
> > +
> > +error_power_off:
> > + inv_mpu6050_set_power_itg(st, false);
> > + return result;
> > }
> >
> > static int inv_mpu6050_sensor_set(struct inv_mpu6050_state *st, int reg,
> > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> > index 540070f..85c2732 100644
> > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> > @@ -53,45 +53,52 @@ static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
> > result = inv_mpu6050_switch_engine(st, true,
> > INV_MPU6050_BIT_PWR_GYRO_STBY);
> > if (result)
> > - return result;
> > + goto error_power_off;
> > }
> > if (st->chip_config.accl_fifo_enable) {
> > result = inv_mpu6050_switch_engine(st, true,
> > INV_MPU6050_BIT_PWR_ACCL_STBY);
> > if (result)
> > - return result;
> > + goto error_power_off;
> > }
> > result = inv_reset_fifo(indio_dev);
> > if (result)
> > - return result;
> > + goto error_power_off;
> > } else {
> > result = regmap_write(st->map, st->reg->fifo_en, 0);
> > if (result)
> > - return result;
> > + goto error_power_off;
> >
> > result = regmap_write(st->map, st->reg->int_enable, 0);
> > if (result)
> > - return result;
> > + goto error_power_off;
> >
> > result = regmap_write(st->map, st->reg->user_ctrl, 0);
> > if (result)
> > - return result;
> > + goto error_power_off;
> >
> > result = inv_mpu6050_switch_engine(st, false,
> > INV_MPU6050_BIT_PWR_GYRO_STBY);
> > if (result)
> > - return result;
> > + goto error_power_off;
> > + st->chip_config.gyro_fifo_enable = false;
> >
> > result = inv_mpu6050_switch_engine(st, false,
> > INV_MPU6050_BIT_PWR_ACCL_STBY);
> > if (result)
> > - return result;
> > + goto error_power_off;
> > + st->chip_config.accl_fifo_enable = false;
> This one line doesn't fit with what the the patch is described
> as doing. Looks like an independent change.
>
> Could you explain why it is here please?
>
> > +
> > result = inv_mpu6050_set_power_itg(st, false);
> > if (result)
> > return result;
> > }
> >
> > return 0;
> > +
> > +error_power_off:
> > + inv_mpu6050_set_power_itg(st, false);
> > + return result;
> > }
> >
> > /**
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 5+ messages in thread* RE: [PATCH 1/5] iio: imu: inv_mpu6050: fix error path not turning chip back off
2017-09-25 19:01 ` Jonathan Cameron
@ 2017-09-26 12:06 ` Jean-Baptiste Maneyrol
2017-09-30 17:34 ` Jonathan Cameron
0 siblings, 1 reply; 5+ messages in thread
From: Jean-Baptiste Maneyrol @ 2017-09-26 12:06 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-iio
Hello Jonathan,
I would have liked to tell you that it is coming soon.
But I don't see it coming before long (several months). If I have some free=
time somewhere, I will try to have a look to rework at least some patches.
Jean-Baptiste
-----Original Message-----
From: Jonathan Cameron [mailto:jic23@jic23.retrosnub.co.uk]=20
Sent: lundi 25 septembre 2017 21:01
To: Jean-Baptiste Maneyrol <JManeyrol@invensense.com>
Cc: linux-iio <linux-iio@vger.kernel.org>
Subject: Re: [PATCH 1/5] iio: imu: inv_mpu6050: fix error path not turning =
chip back off
On Sat, 15 Jul 2017 12:44:47 +0100
Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote:
> On Thu, 13 Jul 2017 13:30:04 +0000
> Jean-Baptiste Maneyrol <JManeyrol@invensense.com> wrote:
>=20
> > Some functions are turning the chip on and not back off in error=20
> > path. With set_power function using a reference counter this is=20
> > keeping the chip on forever.
> >=20
> > Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
> Good fix in general, but there is one line that seems to be doing=20
> something else (marking the fifo disabled in a non error path).
> May well be a good change, but it wants explaining and probably to be=20
> in a separate patch.
Hi,
I see that a new version of this series is still to come.
Any idea when/if you will get to it?
Thanks,
Jonathan
>=20
> Thanks,
>=20
> Jonathan
> > ---
> > drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 12 ++++++++----
> > drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c | 23=20
> > +++++++++++++++--------
> > 2 files changed, 23 insertions(+), 12 deletions(-)
> >=20
> > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c=20
> > b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> > index 44830bc..b1a8159 100644
> > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> > @@ -262,27 +262,31 @@ static int inv_mpu6050_init_config(struct iio_dev=
*indio_dev)
> > d =3D (INV_MPU6050_FSR_2000DPS << INV_MPU6050_GYRO_CONFIG_FSR_SHIFT);
> > result =3D regmap_write(st->map, st->reg->gyro_config, d);
> > if (result)
> > - return result;
> > + goto error_power_off;
> > =20
> > result =3D inv_mpu6050_set_lpf_regs(st, INV_MPU6050_FILTER_20HZ);
> > if (result)
> > - return result;
> > + goto error_power_off;
> > =20
> > d =3D INV_MPU6050_ONE_K_HZ / INV_MPU6050_INIT_FIFO_RATE - 1;
> > result =3D regmap_write(st->map, st->reg->sample_rate_div, d);
> > if (result)
> > - return result;
> > + goto error_power_off;
> > =20
> > d =3D (INV_MPU6050_FS_02G << INV_MPU6050_ACCL_CONFIG_FSR_SHIFT);
> > result =3D regmap_write(st->map, st->reg->accl_config, d);
> > if (result)
> > - return result;
> > + goto error_power_off;
> > =20
> > memcpy(&st->chip_config, hw_info[st->chip_type].config,
> > sizeof(struct inv_mpu6050_chip_config));
> > result =3D inv_mpu6050_set_power_itg(st, false);
> > =20
> > return result;
> > +
> > +error_power_off:
> > + inv_mpu6050_set_power_itg(st, false);
> > + return result;
> > }
> > =20
> > static int inv_mpu6050_sensor_set(struct inv_mpu6050_state *st,=20
> > int reg, diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c=20
> > b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> > index 540070f..85c2732 100644
> > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> > @@ -53,45 +53,52 @@ static int inv_mpu6050_set_enable(struct iio_dev *i=
ndio_dev, bool enable)
> > result =3D inv_mpu6050_switch_engine(st, true,
> > INV_MPU6050_BIT_PWR_GYRO_STBY);
> > if (result)
> > - return result;
> > + goto error_power_off;
> > }
> > if (st->chip_config.accl_fifo_enable) {
> > result =3D inv_mpu6050_switch_engine(st, true,
> > INV_MPU6050_BIT_PWR_ACCL_STBY);
> > if (result)
> > - return result;
> > + goto error_power_off;
> > }
> > result =3D inv_reset_fifo(indio_dev);
> > if (result)
> > - return result;
> > + goto error_power_off;
> > } else {
> > result =3D regmap_write(st->map, st->reg->fifo_en, 0);
> > if (result)
> > - return result;
> > + goto error_power_off;
> > =20
> > result =3D regmap_write(st->map, st->reg->int_enable, 0);
> > if (result)
> > - return result;
> > + goto error_power_off;
> > =20
> > result =3D regmap_write(st->map, st->reg->user_ctrl, 0);
> > if (result)
> > - return result;
> > + goto error_power_off;
> > =20
> > result =3D inv_mpu6050_switch_engine(st, false,
> > INV_MPU6050_BIT_PWR_GYRO_STBY);
> > if (result)
> > - return result;
> > + goto error_power_off;
> > + st->chip_config.gyro_fifo_enable =3D false;
> > =20
> > result =3D inv_mpu6050_switch_engine(st, false,
> > INV_MPU6050_BIT_PWR_ACCL_STBY);
> > if (result)
> > - return result;
> > + goto error_power_off;
> > + st->chip_config.accl_fifo_enable =3D false;
> This one line doesn't fit with what the the patch is described as=20
> doing. Looks like an independent change.
>=20
> Could you explain why it is here please?
>=20
> > +
> > result =3D inv_mpu6050_set_power_itg(st, false);
> > if (result)
> > return result;
> > }
> > =20
> > return 0;
> > +
> > +error_power_off:
> > + inv_mpu6050_set_power_itg(st, false);
> > + return result;
> > }
> > =20
> > /**
>=20
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio"=20
> in the body of a message to majordomo@vger.kernel.org More majordomo=20
> info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 1/5] iio: imu: inv_mpu6050: fix error path not turning chip back off
2017-09-26 12:06 ` Jean-Baptiste Maneyrol
@ 2017-09-30 17:34 ` Jonathan Cameron
0 siblings, 0 replies; 5+ messages in thread
From: Jonathan Cameron @ 2017-09-30 17:34 UTC (permalink / raw)
To: Jean-Baptiste Maneyrol; +Cc: linux-iio
On Tue, 26 Sep 2017 12:06:09 +0000
Jean-Baptiste Maneyrol <JManeyrol@invensense.com> wrote:
> Hello Jonathan,
>
> I would have liked to tell you that it is coming soon.
> But I don't see it coming before long (several months). If I have some free time somewhere, I will try to have a look to rework at least some patches.
I know the feeling all to well. Good luck finding some time!
Jonathan
>
> Jean-Baptiste
>
> -----Original Message-----
> From: Jonathan Cameron [mailto:jic23@jic23.retrosnub.co.uk]
> Sent: lundi 25 septembre 2017 21:01
> To: Jean-Baptiste Maneyrol <JManeyrol@invensense.com>
> Cc: linux-iio <linux-iio@vger.kernel.org>
> Subject: Re: [PATCH 1/5] iio: imu: inv_mpu6050: fix error path not turning chip back off
>
> On Sat, 15 Jul 2017 12:44:47 +0100
> Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote:
>
> > On Thu, 13 Jul 2017 13:30:04 +0000
> > Jean-Baptiste Maneyrol <JManeyrol@invensense.com> wrote:
> >
> > > Some functions are turning the chip on and not back off in error
> > > path. With set_power function using a reference counter this is
> > > keeping the chip on forever.
> > >
> > > Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
> > Good fix in general, but there is one line that seems to be doing
> > something else (marking the fifo disabled in a non error path).
> > May well be a good change, but it wants explaining and probably to be
> > in a separate patch.
>
> Hi,
>
> I see that a new version of this series is still to come.
> Any idea when/if you will get to it?
>
> Thanks,
>
> Jonathan
> >
> > Thanks,
> >
> > Jonathan
> > > ---
> > > drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 12 ++++++++----
> > > drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c | 23
> > > +++++++++++++++--------
> > > 2 files changed, 23 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> > > b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> > > index 44830bc..b1a8159 100644
> > > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> > > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> > > @@ -262,27 +262,31 @@ static int inv_mpu6050_init_config(struct iio_dev *indio_dev)
> > > d = (INV_MPU6050_FSR_2000DPS << INV_MPU6050_GYRO_CONFIG_FSR_SHIFT);
> > > result = regmap_write(st->map, st->reg->gyro_config, d);
> > > if (result)
> > > - return result;
> > > + goto error_power_off;
> > >
> > > result = inv_mpu6050_set_lpf_regs(st, INV_MPU6050_FILTER_20HZ);
> > > if (result)
> > > - return result;
> > > + goto error_power_off;
> > >
> > > d = INV_MPU6050_ONE_K_HZ / INV_MPU6050_INIT_FIFO_RATE - 1;
> > > result = regmap_write(st->map, st->reg->sample_rate_div, d);
> > > if (result)
> > > - return result;
> > > + goto error_power_off;
> > >
> > > d = (INV_MPU6050_FS_02G << INV_MPU6050_ACCL_CONFIG_FSR_SHIFT);
> > > result = regmap_write(st->map, st->reg->accl_config, d);
> > > if (result)
> > > - return result;
> > > + goto error_power_off;
> > >
> > > memcpy(&st->chip_config, hw_info[st->chip_type].config,
> > > sizeof(struct inv_mpu6050_chip_config));
> > > result = inv_mpu6050_set_power_itg(st, false);
> > >
> > > return result;
> > > +
> > > +error_power_off:
> > > + inv_mpu6050_set_power_itg(st, false);
> > > + return result;
> > > }
> > >
> > > static int inv_mpu6050_sensor_set(struct inv_mpu6050_state *st,
> > > int reg, diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> > > b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> > > index 540070f..85c2732 100644
> > > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> > > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> > > @@ -53,45 +53,52 @@ static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
> > > result = inv_mpu6050_switch_engine(st, true,
> > > INV_MPU6050_BIT_PWR_GYRO_STBY);
> > > if (result)
> > > - return result;
> > > + goto error_power_off;
> > > }
> > > if (st->chip_config.accl_fifo_enable) {
> > > result = inv_mpu6050_switch_engine(st, true,
> > > INV_MPU6050_BIT_PWR_ACCL_STBY);
> > > if (result)
> > > - return result;
> > > + goto error_power_off;
> > > }
> > > result = inv_reset_fifo(indio_dev);
> > > if (result)
> > > - return result;
> > > + goto error_power_off;
> > > } else {
> > > result = regmap_write(st->map, st->reg->fifo_en, 0);
> > > if (result)
> > > - return result;
> > > + goto error_power_off;
> > >
> > > result = regmap_write(st->map, st->reg->int_enable, 0);
> > > if (result)
> > > - return result;
> > > + goto error_power_off;
> > >
> > > result = regmap_write(st->map, st->reg->user_ctrl, 0);
> > > if (result)
> > > - return result;
> > > + goto error_power_off;
> > >
> > > result = inv_mpu6050_switch_engine(st, false,
> > > INV_MPU6050_BIT_PWR_GYRO_STBY);
> > > if (result)
> > > - return result;
> > > + goto error_power_off;
> > > + st->chip_config.gyro_fifo_enable = false;
> > >
> > > result = inv_mpu6050_switch_engine(st, false,
> > > INV_MPU6050_BIT_PWR_ACCL_STBY);
> > > if (result)
> > > - return result;
> > > + goto error_power_off;
> > > + st->chip_config.accl_fifo_enable = false;
> > This one line doesn't fit with what the the patch is described as
> > doing. Looks like an independent change.
> >
> > Could you explain why it is here please?
> >
> > > +
> > > result = inv_mpu6050_set_power_itg(st, false);
> > > if (result)
> > > return result;
> > > }
> > >
> > > return 0;
> > > +
> > > +error_power_off:
> > > + inv_mpu6050_set_power_itg(st, false);
> > > + return result;
> > > }
> > >
> > > /**
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-iio"
> > in the body of a message to majordomo@vger.kernel.org More majordomo
> > info at http://vger.kernel.org/majordomo-info.html
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-09-30 17:34 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-07-13 13:30 [PATCH 1/5] iio: imu: inv_mpu6050: fix error path not turning chip back off Jean-Baptiste Maneyrol
2017-07-15 11:44 ` Jonathan Cameron
2017-09-25 19:01 ` Jonathan Cameron
2017-09-26 12:06 ` Jean-Baptiste Maneyrol
2017-09-30 17:34 ` Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).