* [PATCH v3] iio: imu: inv_mpu6050: test whoami first and against all known values
@ 2017-06-06 10:29 Jean-Baptiste Maneyrol
2017-06-11 12:58 ` Jonathan Cameron
0 siblings, 1 reply; 4+ messages in thread
From: Jean-Baptiste Maneyrol @ 2017-06-06 10:29 UTC (permalink / raw)
To: linux-iio
SPI bus is never generating error during transfer, so to check if
a chip is correctly connected on a SPI bus we enforce whoami check
to be correct. In this way we can assure SPI probe is failing if
there is no chip connected.
Fixes: cec0154556f8 ("iio: inv_mpu6050: Check WHO_AM_I register on probe")
Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
Cc: <stable@vger.kernel.org>
---
drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 33 ++++++++++++++++++++++----=
----
1 file changed, 24 insertions(+), 9 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 96dabbd..af1b536 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -770,27 +770,42 @@ static int inv_check_and_setup_chip(struct inv_mpu605=
0_state *st)
{
int result;
unsigned int regval;
+ int i;
=20
st->hw =3D &hw_info[st->chip_type];
st->reg =3D hw_info[st->chip_type].reg;
=20
- /* reset to make sure previous state are not there */
- result =3D regmap_write(st->map, st->reg->pwr_mgmt_1,
- INV_MPU6050_BIT_H_RESET);
- if (result)
- return result;
- msleep(INV_MPU6050_POWER_UP_TIME);
-
/* check chip self-identification */
result =3D regmap_read(st->map, INV_MPU6050_REG_WHOAMI, ®val);
if (result)
return result;
if (regval !=3D st->hw->whoami) {
- dev_warn(regmap_get_device(st->map),
- "whoami mismatch got %#02x expected %#02hhx for %s\n",
+ /* check whoami against all possible values */
+ for (i =3D 0; i < INV_NUM_PARTS; ++i) {
+ if (regval =3D=3D hw_info[i].whoami) {
+ dev_warn(regmap_get_device(st->map),
+ "whoami mismatch got %#02x (%s)"
+ "expected %#02hhx (%s)\n",
+ regval, hw_info[i].name,
+ st->hw->whoami, st->hw->name);
+ break;
+ }
+ }
+ if (i >=3D INV_NUM_PARTS) {
+ dev_err(regmap_get_device(st->map),
+ "invalid whoami %#02x expected %#02hhx (%s)\n",
regval, st->hw->whoami, st->hw->name);
+ return -ENODEV;
+ }
}
=20
+ /* reset to make sure previous state are not there */
+ result =3D regmap_write(st->map, st->reg->pwr_mgmt_1,
+ INV_MPU6050_BIT_H_RESET);
+ if (result)
+ return result;
+ msleep(INV_MPU6050_POWER_UP_TIME);
+
/*
* toggle power state. After reset, the sleep bit could be on
* or off depending on the OTP settings. Toggling power would
--=20
1.9.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v3] iio: imu: inv_mpu6050: test whoami first and against all known values
2017-06-06 10:29 [PATCH v3] iio: imu: inv_mpu6050: test whoami first and against all known values Jean-Baptiste Maneyrol
@ 2017-06-11 12:58 ` Jonathan Cameron
2017-06-12 9:18 ` Jean-Baptiste Maneyrol
0 siblings, 1 reply; 4+ messages in thread
From: Jonathan Cameron @ 2017-06-11 12:58 UTC (permalink / raw)
To: Jean-Baptiste Maneyrol; +Cc: linux-iio
On Tue, 6 Jun 2017 10:29:52 +0000
Jean-Baptiste Maneyrol <JManeyrol@invensense.com> wrote:
> SPI bus is never generating error during transfer, so to check if
> a chip is correctly connected on a SPI bus we enforce whoami check
> to be correct. In this way we can assure SPI probe is failing if
> there is no chip connected.
>
> Fixes: cec0154556f8 ("iio: inv_mpu6050: Check WHO_AM_I register on probe")
> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
> Cc: <stable@vger.kernel.org>
Hi Jean-Baptiste.
I'm happy with the patch, however it's a fix for an issue with a platform
rather than the driver itself. That needs to be explained in the patch
description. In particular do you have some examples we can list to
justify the change?
Otherwise, I'm thinking this is a 'nice to have' that wants to go via
the next merge window rather than going in as a fix.
Jonathan
> ---
> drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 33 ++++++++++++++++++++++--------
> 1 file changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> index 96dabbd..af1b536 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> @@ -770,27 +770,42 @@ static int inv_check_and_setup_chip(struct inv_mpu6050_state *st)
> {
> int result;
> unsigned int regval;
> + int i;
>
> st->hw = &hw_info[st->chip_type];
> st->reg = hw_info[st->chip_type].reg;
>
> - /* reset to make sure previous state are not there */
> - result = regmap_write(st->map, st->reg->pwr_mgmt_1,
> - INV_MPU6050_BIT_H_RESET);
> - if (result)
> - return result;
> - msleep(INV_MPU6050_POWER_UP_TIME);
> -
> /* check chip self-identification */
> result = regmap_read(st->map, INV_MPU6050_REG_WHOAMI, ®val);
> if (result)
> return result;
> if (regval != st->hw->whoami) {
> - dev_warn(regmap_get_device(st->map),
> - "whoami mismatch got %#02x expected %#02hhx for %s\n",
> + /* check whoami against all possible values */
> + for (i = 0; i < INV_NUM_PARTS; ++i) {
> + if (regval == hw_info[i].whoami) {
> + dev_warn(regmap_get_device(st->map),
> + "whoami mismatch got %#02x (%s)"
> + "expected %#02hhx (%s)\n",
> + regval, hw_info[i].name,
> + st->hw->whoami, st->hw->name);
> + break;
> + }
> + }
> + if (i >= INV_NUM_PARTS) {
> + dev_err(regmap_get_device(st->map),
> + "invalid whoami %#02x expected %#02hhx (%s)\n",
> regval, st->hw->whoami, st->hw->name);
> + return -ENODEV;
> + }
> }
>
> + /* reset to make sure previous state are not there */
> + result = regmap_write(st->map, st->reg->pwr_mgmt_1,
> + INV_MPU6050_BIT_H_RESET);
> + if (result)
> + return result;
> + msleep(INV_MPU6050_POWER_UP_TIME);
> +
> /*
> * toggle power state. After reset, the sleep bit could be on
> * or off depending on the OTP settings. Toggling power would
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] iio: imu: inv_mpu6050: test whoami first and against all known values
2017-06-11 12:58 ` Jonathan Cameron
@ 2017-06-12 9:18 ` Jean-Baptiste Maneyrol
2017-06-13 20:20 ` Jonathan Cameron
0 siblings, 1 reply; 4+ messages in thread
From: Jean-Baptiste Maneyrol @ 2017-06-12 9:18 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-iio
>From: Jonathan Cameron <jic23@kernel.org>
>Sent: Sunday, June 11, 2017 14:58
>To: Jean-Baptiste Maneyrol
>Cc: linux-iio
>Subject: Re: [PATCH v3] iio: imu: inv_mpu6050: test whoami first and again=
st all known values
> =20
>On Tue, 6 Jun 2017 10:29:52 +0000
>Jean-Baptiste Maneyrol <JManeyrol@invensense.com> wrote:
>
>> SPI bus is never generating error during transfer, so to check if
>> a chip is correctly connected on a SPI bus we enforce whoami check
>> to be correct. In this way we can assure SPI probe is failing if
>> there is no chip connected.
>>=20
>> Fixes: cec0154556f8 ("iio: inv_mpu6050: Check WHO_AM_I register on probe=
")
>> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
>> Cc: <stable@vger.kernel.org>
>Hi Jean-Baptiste.
>
>I'm happy with the patch, however it's a fix for an issue with a platform
>rather than the driver itself. That needs to be explained in the patch
>description.=A0 In particular do you have some examples we can list to
>justify the change?
>
>Otherwise, I'm thinking this is a 'nice to have' that wants to go via
>the next merge window rather than going in as a fix.
>
>Jonathan
Hi Jonathan,
I think that this is really a driver issue. Having a probe function that do=
esn't
return an error when there is no chip responding is more of a driver issue =
in my
own opinion. This makes things more difficult to debug it there is any hard=
ware
issue.
This is affecting our internal test platform which is completely custom. I
don't know if adding this example is interesting or not.
In any case, no problem with me to have this going only into the next merge
window.
Thanks for your review.
Jean-Baptiste
>> ---
>>=A0 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 33 +++++++++++++++++++++=
+--------
>>=A0 1 file changed, 24 insertions(+), 9 deletions(-)
>>=20
>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/im=
u/inv_mpu6050/inv_mpu_core.c
> index 96dabbd..af1b536 100644
>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>> @@ -770,27 +770,42 @@ static int inv_check_and_setup_chip(struct inv_mpu=
6050_state *st)
>>=A0 {
>>=A0=A0=A0=A0=A0=A0=A0 int result;
>>=A0=A0=A0=A0=A0=A0=A0 unsigned int regval;
>> +=A0=A0=A0=A0 int i;
>>=A0=20
>>=A0=A0=A0=A0=A0=A0=A0 st->hw=A0 =3D &hw_info[st->chip_type];
>>=A0=A0=A0=A0=A0=A0=A0 st->reg =3D hw_info[st->chip_type].reg;
>>=A0=20
>> -=A0=A0=A0=A0 /* reset to make sure previous state are not there */
>> -=A0=A0=A0=A0 result =3D regmap_write(st->map, st->reg->pwr_mgmt_1,
>> -=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0=A0=A0 INV_MPU6050_BIT_H_RESET);
>> -=A0=A0=A0=A0 if (result)
>> -=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 return result;
>> -=A0=A0=A0=A0 msleep(INV_MPU6050_POWER_UP_TIME);
>> -
>>=A0=A0=A0=A0=A0=A0=A0 /* check chip self-identification */
>>=A0=A0=A0=A0=A0=A0=A0 result =3D regmap_read(st->map, INV_MPU6050_REG_WHO=
AMI, ®val);
>>=A0=A0=A0=A0=A0=A0=A0 if (result)
>>=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 return result;
>>=A0=A0=A0=A0=A0=A0=A0 if (regval !=3D st->hw->whoami) {
>> -=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 dev_warn(regmap_get_device(st->map=
),
>> -=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0=A0=A0=A0=A0 "whoami mismatch got %#02x expected %#02hhx for %s\n",
>> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 /* check whoami against all possib=
le values */
>> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 for (i =3D 0; i < INV_NUM_PARTS; +=
+i) {
>> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 if (regval=
=3D=3D hw_info[i].whoami) {
>> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0=A0=A0=A0=A0 dev_warn(regmap_get_device(st->map),
>> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 "whoami mismatch got %#02x (%s)"
>> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 "expected %#02hhx (%s)\n",
>> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 regval, hw_info[i].name,
>> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 st->hw->whoami, st->hw->name);
>> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0=A0=A0=A0=A0 break;
>> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 }
>> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 }
>> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 if (i >=3D INV_NUM_PARTS) {
>> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 dev_err(re=
gmap_get_device(st->map),
>> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0=A0=A0=A0=A0 "invalid whoami %#02x expected %#02hhx (%s)\n",
>>=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0=A0=A0=A0=A0=A0=A0 regval, st->hw->whoami, st->hw->name);
>> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 return -EN=
ODEV;
>> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 }
>>=A0=A0=A0=A0=A0=A0=A0 }
>>=A0=20
>> +=A0=A0=A0=A0 /* reset to make sure previous state are not there */
>> +=A0=A0=A0=A0 result =3D regmap_write(st->map, st->reg->pwr_mgmt_1,
>> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0=A0=A0 INV_MPU6050_BIT_H_RESET);
>> +=A0=A0=A0=A0 if (result)
>> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 return result;
>> +=A0=A0=A0=A0 msleep(INV_MPU6050_POWER_UP_TIME);
>> +
>>=A0=A0=A0=A0=A0=A0=A0 /*
>>=A0=A0=A0=A0=A0=A0=A0=A0 * toggle power state. After reset, the sleep bit=
could be on
>>=A0=A0=A0=A0=A0=A0=A0=A0 * or off depending on the OTP settings. Toggling=
power would
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] iio: imu: inv_mpu6050: test whoami first and against all known values
2017-06-12 9:18 ` Jean-Baptiste Maneyrol
@ 2017-06-13 20:20 ` Jonathan Cameron
0 siblings, 0 replies; 4+ messages in thread
From: Jonathan Cameron @ 2017-06-13 20:20 UTC (permalink / raw)
To: Jean-Baptiste Maneyrol; +Cc: linux-iio
On Mon, 12 Jun 2017 09:18:11 +0000
Jean-Baptiste Maneyrol <JManeyrol@invensense.com> wrote:
> >From: Jonathan Cameron <jic23@kernel.org>
> >Sent: Sunday, June 11, 2017 14:58
> >To: Jean-Baptiste Maneyrol
> >Cc: linux-iio
> >Subject: Re: [PATCH v3] iio: imu: inv_mpu6050: test whoami first and against all known values
> >
> >On Tue, 6 Jun 2017 10:29:52 +0000
> >Jean-Baptiste Maneyrol <JManeyrol@invensense.com> wrote:
> >
> >> SPI bus is never generating error during transfer, so to check if
> >> a chip is correctly connected on a SPI bus we enforce whoami check
> >> to be correct. In this way we can assure SPI probe is failing if
> >> there is no chip connected.
> >>
> >> Fixes: cec0154556f8 ("iio: inv_mpu6050: Check WHO_AM_I register on probe")
> >> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
> >> Cc: <stable@vger.kernel.org>
> >Hi Jean-Baptiste.
> >
> >I'm happy with the patch, however it's a fix for an issue with a platform
> >rather than the driver itself. That needs to be explained in the patch
> >description. In particular do you have some examples we can list to
> >justify the change?
> >
> >Otherwise, I'm thinking this is a 'nice to have' that wants to go via
> >the next merge window rather than going in as a fix.
> >
> >Jonathan
> Hi Jonathan,
>
> I think that this is really a driver issue. Having a probe function that doesn't
> return an error when there is no chip responding is more of a driver issue in my
> own opinion. This makes things more difficult to debug it there is any hardware
> issue.
Easy to argue both ways ;)A
a platform that says a chips it there that isn't is a platform issue to me.
Numerous SPI chips are completely impossible to probe for. Query and ADC and
it returns a value of 0. Well, maybe the value is zero ;)
>
> This is affecting our internal test platform which is completely custom. I
> don't know if adding this example is interesting or not.
>
> In any case, no problem with me to have this going only into the next merge
> window.
I'll queue it up - but will drop the stable tag as I don't really want this
pulled back across stable kernels unless we have boards out there that
actually need it. I've left the fixes tag in, but added a comment to why
I'm not marking it for stable.
Applied to the togreg branch of iio.git
Thanks,
Jonathan
>
> Thanks for your review.
> Jean-Baptiste
> >> ---
> >> drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 33 ++++++++++++++++++++++--------
> >> 1 file changed, 24 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> > index 96dabbd..af1b536 100644
> >> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> >> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> >> @@ -770,27 +770,42 @@ static int inv_check_and_setup_chip(struct inv_mpu6050_state *st)
> >> {
> >> int result;
> >> unsigned int regval;
> >> + int i;
> >>
> >> st->hw = &hw_info[st->chip_type];
> >> st->reg = hw_info[st->chip_type].reg;
> >>
> >> - /* reset to make sure previous state are not there */
> >> - result = regmap_write(st->map, st->reg->pwr_mgmt_1,
> >> - INV_MPU6050_BIT_H_RESET);
> >> - if (result)
> >> - return result;
> >> - msleep(INV_MPU6050_POWER_UP_TIME);
> >> -
> >> /* check chip self-identification */
> >> result = regmap_read(st->map, INV_MPU6050_REG_WHOAMI, ®val);
> >> if (result)
> >> return result;
> >> if (regval != st->hw->whoami) {
> >> - dev_warn(regmap_get_device(st->map),
> >> - "whoami mismatch got %#02x expected %#02hhx for %s\n",
> >> + /* check whoami against all possible values */
> >> + for (i = 0; i < INV_NUM_PARTS; ++i) {
> >> + if (regval == hw_info[i].whoami) {
> >> + dev_warn(regmap_get_device(st->map),
> >> + "whoami mismatch got %#02x (%s)"
> >> + "expected %#02hhx (%s)\n",
> >> + regval, hw_info[i].name,
> >> + st->hw->whoami, st->hw->name);
> >> + break;
> >> + }
> >> + }
> >> + if (i >= INV_NUM_PARTS) {
> >> + dev_err(regmap_get_device(st->map),
> >> + "invalid whoami %#02x expected %#02hhx (%s)\n",
> >> regval, st->hw->whoami, st->hw->name);
> >> + return -ENODEV;
> >> + }
> >> }
> >>
> >> + /* reset to make sure previous state are not there */
> >> + result = regmap_write(st->map, st->reg->pwr_mgmt_1,
> >> + INV_MPU6050_BIT_H_RESET);
> >> + if (result)
> >> + return result;
> >> + msleep(INV_MPU6050_POWER_UP_TIME);
> >> +
> >> /*
> >> * toggle power state. After reset, the sleep bit could be on
> >> * or off depending on the OTP settings. Toggling power would
> --
> 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] 4+ messages in thread
end of thread, other threads:[~2017-06-13 20:20 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-06-06 10:29 [PATCH v3] iio: imu: inv_mpu6050: test whoami first and against all known values Jean-Baptiste Maneyrol
2017-06-11 12:58 ` Jonathan Cameron
2017-06-12 9:18 ` Jean-Baptiste Maneyrol
2017-06-13 20:20 ` 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).