* [PATCH] iio: imu: inv_mpu6050: test whoami against invalid values and do it first @ 2017-05-12 12:45 Jean-Baptiste Maneyrol 2017-05-14 14:49 ` Jonathan Cameron 0 siblings, 1 reply; 6+ messages in thread From: Jean-Baptiste Maneyrol @ 2017-05-12 12:45 UTC (permalink / raw) To: linux-iio@vger.kernel.org SPI bus is never generating error during transfer, so a simple way to check if a chip is correctly connected on a SPI bus is to check a fixed value lik= e whoami against valid values. 0x00 or 0xff can never happened. It is better to do this test first since if there is a problem with a DTS and an incorre= ct chip is wired instead, a write can be more damaging than a read. Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> --- drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 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..42fb135 100644 --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c @@ -774,23 +774,28 @@ static int inv_check_and_setup_chip(struct inv_mpu605= 0_state *st) 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) { + if (regval =3D=3D 0x00 || regval =3D=3D 0xff) { + dev_err(regmap_get_device(st->map), + "invalid whoami probably io error\n"); + return -EIO; + } dev_warn(regmap_get_device(st->map), "whoami mismatch got %#02x expected %#02hhx for %s\n", regval, st->hw->whoami, st->hw->name); } =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] 6+ messages in thread
* Re: [PATCH] iio: imu: inv_mpu6050: test whoami against invalid values and do it first 2017-05-12 12:45 [PATCH] iio: imu: inv_mpu6050: test whoami against invalid values and do it first Jean-Baptiste Maneyrol @ 2017-05-14 14:49 ` Jonathan Cameron 2017-05-15 8:42 ` Jean-Baptiste Maneyrol 0 siblings, 1 reply; 6+ messages in thread From: Jonathan Cameron @ 2017-05-14 14:49 UTC (permalink / raw) To: Jean-Baptiste Maneyrol, linux-iio@vger.kernel.org On 12/05/17 13:45, Jean-Baptiste Maneyrol wrote: > SPI bus is never generating error during transfer, so a simple way to check > if a chip is correctly connected on a SPI bus is to check a fixed value like > whoami against valid values. 0x00 or 0xff can never happened. It is better > to do this test first since if there is a problem with a DTS and an incorrect > chip is wired instead, a write can be more damaging than a read. > > Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> Hmm. I'd a little unsure on this one. If we start getting into the general game of protecting against every wrong DTS it may be a slippery slope. Is there an actual platform out there that you know of that this will protect? If so I can probably be persuaded. If not I would actually prefer if we checked against the specified ID rather than relying on a different chip producing either high or low consistently when hit with this query. So perhaps make the ID check a failure if it doesn't match any of the supported parts? Bit of a pain for new devices as they won't work immediately but conversely protects them against issues due to incorrect identification as well. Jonathan > --- > drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 19 ++++++++++++------- > 1 file changed, 12 insertions(+), 7 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..42fb135 100644 > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c > @@ -774,23 +774,28 @@ static int inv_check_and_setup_chip(struct inv_mpu6050_state *st) > 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) { > + if (regval == 0x00 || regval == 0xff) { > + dev_err(regmap_get_device(st->map), > + "invalid whoami probably io error\n"); > + return -EIO; > + } > dev_warn(regmap_get_device(st->map), > "whoami mismatch got %#02x expected %#02hhx for %s\n", > regval, st->hw->whoami, st->hw->name); > } > > + /* 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] 6+ messages in thread
* Re: [PATCH] iio: imu: inv_mpu6050: test whoami against invalid values and do it first 2017-05-14 14:49 ` Jonathan Cameron @ 2017-05-15 8:42 ` Jean-Baptiste Maneyrol 2017-05-16 18:06 ` Jonathan Cameron 0 siblings, 1 reply; 6+ messages in thread From: Jean-Baptiste Maneyrol @ 2017-05-15 8:42 UTC (permalink / raw) To: Jonathan Cameron, linux-iio@vger.kernel.org Hello, this is mainly useful for our test platforms where we want to support both = I2C and SPI configuration with the same kernel image. For that purpose we d= eclare 2 chips, 1 on I2C and 1 on SPI bus. But on the hardware there is onl= y 1 chip connected (you can plug it in a I2C or SPI slot as required). And = we expect the driver probe to fail properly when there is no chip connected= on the corresponding bus (wasn't the case with SPI version). In any case if there is a SPI wiring issue somewhere, it is useful to have = the driver probe failing correctly and reporting the issue. Since SPI trans= fer are not acked, there always will be a response for every transaction ev= en if there is no chip behind (this is 0x00 on my platform, I don't know if= there is some "standard" response here). That's why I am testing whoami re= ading against these values to ensure there is a real chip behind. I think it is useful to keep the check against ID not preventing the driver= probe to succeed. We have a lot of similar chips with different IDs, so it= can be helpful. Otherwise, I can switch to checking against ID and add all= known IDs. Thanks. Jean-Baptiste From: Jonathan Cameron <jic23@kernel.org> Sent: Sunday, May 14, 2017 16:49 To: Jean-Baptiste Maneyrol; linux-iio@vger.kernel.org Subject: Re: [PATCH] iio: imu: inv_mpu6050: test whoami against invalid val= ues and do it first =A0 =20 On 12/05/17 13:45, Jean-Baptiste Maneyrol wrote: > SPI bus is never generating error during transfer, so a simple way to che= ck > if a chip is correctly connected on a SPI bus is to check a fixed value l= ike > whoami against valid values. 0x00 or 0xff can never happened. It is bette= r > to do this test first since if there is a problem with a DTS and an incor= rect > chip is wired instead, a write can be more damaging than a read. >=20 > Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> Hmm. I'd a little unsure on this one.=A0 If we start getting into the general game of protecting against every wrong DTS it may be a slippery slope. Is there an actual platform out there that you know of that this will protect?=A0 If so I can probably be persuaded. If not I would actually prefer if we checked against the specified ID rather than relying on a different chip producing either high or low consistently when hit with this query. So perhaps make the ID check a failure if it doesn't match any of the supported parts?=A0 Bit of a pain for new devices as they won't work immediately but conversely protects them against issues due to incorrect identification as well. Jonathan > --- >=A0=A0 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 19 ++++++++++++------- >=A0=A0 1 file changed, 12 insertions(+), 7 deletions(-) >=20 > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu= /inv_mpu6050/inv_mpu_core.c > index 96dabbd..42fb135 100644 > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c > @@ -774,23 +774,28 @@ static int inv_check_and_setup_chip(struct inv_mpu6= 050_state *st) >=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=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_WHOA= MI, ®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 if (regval =3D=3D 0x00 || regval = =3D=3D 0xff) { > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 dev_err(reg= map_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 "invalid whoami probably io error\n"); > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 return -EIO= ; > +=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(s= t->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 "whoami mismatch got %#02x expected %#02hhx for %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=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 >=20 = ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iio: imu: inv_mpu6050: test whoami against invalid values and do it first 2017-05-15 8:42 ` Jean-Baptiste Maneyrol @ 2017-05-16 18:06 ` Jonathan Cameron 2017-05-17 15:03 ` Jean-Baptiste Maneyrol 0 siblings, 1 reply; 6+ messages in thread From: Jonathan Cameron @ 2017-05-16 18:06 UTC (permalink / raw) To: Jean-Baptiste Maneyrol, linux-iio@vger.kernel.org On 15/05/17 09:42, Jean-Baptiste Maneyrol wrote: > Hello, > > this is mainly useful for our test platforms where we want to support > both I2C and SPI configuration with the same kernel image. For that > purpose we declare 2 chips, 1 on I2C and 1 on SPI bus. But on the > hardware there is only 1 chip connected (you can plug it in a I2C or > SPI slot as required). And we expect the driver probe to fail > properly when there is no chip connected on the corresponding bus > (wasn't the case with SPI version). > > In any case if there is a SPI wiring issue somewhere, it is useful to > have the driver probe failing correctly and reporting the issue. > Since SPI transfer are not acked, there always will be a response for > every transaction even if there is no chip behind (this is 0x00 on my > platform, I don't know if there is some "standard" response here). > That's why I am testing whoami reading against these values to ensure > there is a real chip behind. > > I think it is useful to keep the check against ID not preventing the > driver probe to succeed. We have a lot of similar chips with > different IDs, so it can be helpful. Otherwise, I can switch to > checking against ID and add all known IDs. That would be be my preference. Check against all known and report but don't fail on the 'it is the wrong one' case we all know happens out in the wild! Jonathan p.s. Please don't top post and please wrap lines at 80 chars. I've just broken some threading email clients by wrapping it in this reply... (minor point!) > > Thanks. Jean-Baptiste > > From: Jonathan Cameron <jic23@kernel.org> > Sent: Sunday, May 14, 2017 16:49 > To: Jean-Baptiste Maneyrol; linux-iio@vger.kernel.org > Subject: Re: [PATCH] iio: imu: inv_mpu6050: test whoami against invalid values and do it first > > On 12/05/17 13:45, Jean-Baptiste Maneyrol wrote: >> SPI bus is never generating error during transfer, so a simple way to check >> if a chip is correctly connected on a SPI bus is to check a fixed value like >> whoami against valid values. 0x00 or 0xff can never happened. It is better >> to do this test first since if there is a problem with a DTS and an incorrect >> chip is wired instead, a write can be more damaging than a read. >> >> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> > Hmm. I'd a little unsure on this one. If we start getting into the > general game of protecting against every wrong DTS it may be a slippery > slope. > > Is there an actual platform out there that you know of that this will > protect? If so I can probably be persuaded. > > If not I would actually prefer if we checked against the specified ID > rather than relying on a different chip producing either high or low > consistently when hit with this query. > > So perhaps make the ID check a failure if it doesn't match any of the > supported parts? Bit of a pain for new devices as they won't work > immediately but conversely protects them against issues due to incorrect > identification as well. > > Jonathan > >> --- >> drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 19 ++++++++++++------- >> 1 file changed, 12 insertions(+), 7 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..42fb135 100644 >> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c >> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c >> @@ -774,23 +774,28 @@ static int inv_check_and_setup_chip(struct inv_mpu6050_state *st) >> 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) { >> + if (regval == 0x00 || regval == 0xff) { >> + dev_err(regmap_get_device(st->map), >> + "invalid whoami probably io error\n"); >> + return -EIO; >> + } >> dev_warn(regmap_get_device(st->map), >> "whoami mismatch got %#02x expected %#02hhx for %s\n", >> regval, st->hw->whoami, st->hw->name); >> } >> >> + /* 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] 6+ messages in thread
* Re: [PATCH] iio: imu: inv_mpu6050: test whoami against invalid values and do it first 2017-05-16 18:06 ` Jonathan Cameron @ 2017-05-17 15:03 ` Jean-Baptiste Maneyrol 2017-05-21 11:43 ` Jonathan Cameron 0 siblings, 1 reply; 6+ messages in thread From: Jean-Baptiste Maneyrol @ 2017-05-17 15:03 UTC (permalink / raw) To: Jonathan Cameron, linux-iio@vger.kernel.org >From: Jonathan Cameron <jic23@kernel.org> >Sent: Tuesday, May 16, 2017 20:06 >To: Jean-Baptiste Maneyrol; linux-iio@vger.kernel.org >Subject: Re: [PATCH] iio: imu: inv_mpu6050: test whoami against invalid va= lues and do it first >=A0 =20 >On 15/05/17 09:42, Jean-Baptiste Maneyrol wrote: >> Hello, >>=20 >> this is mainly useful for our test platforms where we want to support >> both I2C and SPI configuration with the same kernel image. For that >> purpose we declare 2 chips, 1 on I2C and 1 on SPI bus. But on the >> hardware there is only 1 chip connected (you can plug it in a I2C or >> SPI slot as required). And we expect the driver probe to fail >> properly when there is no chip connected on the corresponding bus >> (wasn't the case with SPI version). >>=20 >> In any case if there is a SPI wiring issue somewhere, it is useful to >> have the driver probe failing correctly and reporting the issue. >> Since SPI transfer are not acked, there always will be a response for >> every transaction even if there is no chip behind (this is 0x00 on my >> platform, I don't know if there is some "standard" response here). >> That's why I am testing whoami reading against these values to ensure >> there is a real chip behind. >>=20 >> I think it is useful to keep the check against ID not preventing the >> driver probe to succeed. We have a lot of similar chips with >> different IDs, so it can be helpful. Otherwise, I can switch to >> checking against ID and add all known IDs. >That would be be my preference.=A0 Check against all known and >report but don't fail on the 'it is the wrong one' case we >all know happens out in the wild! > >Jonathan > >p.s. Please don't top post and please wrap lines at 80 chars. >I've just broken some threading email clients by wrapping >it in this reply... (minor point!) We agree on this point, but we are digressing from the issue. Sorry my explanations were a bit too long. The main point is that the SPI driver probe is not failing when there is no chip connected or when the wiring is broken. This is a blocking issue for my use case and not very clean anyway. The problem is that SPI transactions are never failing even if there is a transfer problem. That's not the case with I2C where every transactions are acked. My solution is to read whoami and test if the value is clearly invalid (0x00 or 0xff are obvious invalid whoami that will be never used in any invensense chips, and are possible return values for a not working SPI wiring). So that we are not blocking the driver probe for future possible whoami. Is this approach correct for you? Otherwise if you have a better idea, it would be welcome. Thanks for your help. JB P.S.: Hope this e-mail is OK. I'm stuck with Outlook WebApp, but that's not that bad to use. >>=20 >> Thanks. Jean-Baptiste >>=20 >> From: Jonathan Cameron <jic23@kernel.org> >> Sent: Sunday, May 14, 2017 16:49 >> To: Jean-Baptiste Maneyrol; linux-iio@vger.kernel.org >> Subject: Re: [PATCH] iio: imu: inv_mpu6050: test whoami against invalid = values and do it first >>=A0=A0=A0=A0=A0=20 >> On 12/05/17 13:45, Jean-Baptiste Maneyrol wrote: >>> SPI bus is never generating error during transfer, so a simple way to c= heck >>> if a chip is correctly connected on a SPI bus is to check a fixed value= like >>> whoami against valid values. 0x00 or 0xff can never happened. It is bet= ter >>> to do this test first since if there is a problem with a DTS and an inc= orrect >>> chip is wired instead, a write can be more damaging than a read. >>> >>> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> >> Hmm. I'd a little unsure on this one.=A0 If we start getting into the >> general game of protecting against every wrong DTS it may be a slippery >> slope. >>=20 >> Is there an actual platform out there that you know of that this will >> protect?=A0 If so I can probably be persuaded. >>=20 >> If not I would actually prefer if we checked against the specified ID >> rather than relying on a different chip producing either high or low >> consistently when hit with this query. >>=20 >> So perhaps make the ID check a failure if it doesn't match any of the >> supported parts?=A0 Bit of a pain for new devices as they won't work >> immediately but conversely protects them against issues due to incorrect >> identification as well. >>=20 >> Jonathan >>=20 >>> --- >>=A0=A0=A0=A0 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 19 ++++++++++++= ------- >>=A0=A0=A0=A0 1 file changed, 12 insertions(+), 7 deletions(-) >> >> 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..42fb135 100644 >> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c >> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c >> @@ -774,23 +774,28 @@ static int inv_check_and_setup_chip(struct inv_mpu= 6050_state *st) >>=A0=A0=A0=A0=A0=A0=A0=A0=A0 st->hw=A0 =3D &hw_info[st->chip_type]; >>=A0=A0=A0=A0=A0=A0=A0=A0=A0 st->reg =3D hw_info[st->chip_type].reg; >>=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 /* check chip self-identification */ >>=A0=A0=A0=A0=A0=A0=A0=A0=A0 result =3D regmap_read(st->map, INV_MPU6050_R= EG_WHOAMI, ®val); >>=A0=A0=A0=A0=A0=A0=A0=A0=A0 if (result) >>=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 return result; >>=A0=A0=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 if (regval =3D=3D 0x00 || regval = =3D=3D 0xff) { >> +=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=A0=A0=A0=A0=A0=A0=A0=A0 "invalid whoami probably io error\n= "); >> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 return -EI= O; >> +=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 dev_warn(regmap_get_d= evice(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 "whoami mismatch got %#02x expected %#02hhx for= %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 regval, st->hw->whoami, st->hw->name); >>=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=A0=A0=A0=A0 * toggle power state. After reset, the sle= ep bit could be on >>=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 * or off depending on the OTP settings. To= ggling power would >> >=20 >=A0=A0=A0=A0=A0=20 >=20 = ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iio: imu: inv_mpu6050: test whoami against invalid values and do it first 2017-05-17 15:03 ` Jean-Baptiste Maneyrol @ 2017-05-21 11:43 ` Jonathan Cameron 0 siblings, 0 replies; 6+ messages in thread From: Jonathan Cameron @ 2017-05-21 11:43 UTC (permalink / raw) To: Jean-Baptiste Maneyrol, linux-iio@vger.kernel.org On 17/05/17 16:03, Jean-Baptiste Maneyrol wrote: > >> From: Jonathan Cameron <jic23@kernel.org> >> Sent: Tuesday, May 16, 2017 20:06 >> To: Jean-Baptiste Maneyrol; linux-iio@vger.kernel.org >> Subject: Re: [PATCH] iio: imu: inv_mpu6050: test whoami against invalid values and do it first >> >> On 15/05/17 09:42, Jean-Baptiste Maneyrol wrote: >>> Hello, >>> >>> this is mainly useful for our test platforms where we want to support >>> both I2C and SPI configuration with the same kernel image. For that >>> purpose we declare 2 chips, 1 on I2C and 1 on SPI bus. But on the >>> hardware there is only 1 chip connected (you can plug it in a I2C or >>> SPI slot as required). And we expect the driver probe to fail >>> properly when there is no chip connected on the corresponding bus >>> (wasn't the case with SPI version). >>> >>> In any case if there is a SPI wiring issue somewhere, it is useful to >>> have the driver probe failing correctly and reporting the issue. >>> Since SPI transfer are not acked, there always will be a response for >>> every transaction even if there is no chip behind (this is 0x00 on my >>> platform, I don't know if there is some "standard" response here). >>> That's why I am testing whoami reading against these values to ensure >>> there is a real chip behind. >>> >>> I think it is useful to keep the check against ID not preventing the >>> driver probe to succeed. We have a lot of similar chips with >>> different IDs, so it can be helpful. Otherwise, I can switch to >>> checking against ID and add all known IDs. >> That would be be my preference. Check against all known and >> report but don't fail on the 'it is the wrong one' case we >> all know happens out in the wild! >> >> Jonathan >> >> p.s. Please don't top post and please wrap lines at 80 chars. >> I've just broken some threading email clients by wrapping >> it in this reply... (minor point!) > > We agree on this point, but we are digressing from the issue. > Sorry my explanations were a bit too long. > > The main point is that the SPI driver probe is not failing when > there is no chip connected or when the wiring is broken. > This is a blocking issue for my use case and not very clean anyway. > The problem is that SPI transactions are never failing even if there is > a transfer problem. That's not the case with I2C where every > transactions are acked. > > My solution is to read whoami and test if the value is clearly invalid > (0x00 or 0xff are obvious invalid whoami that will be never used in any > invensense chips, and are possible return values for a > not working SPI wiring). So that we are not blocking the driver probe for > future possible whoamiI would rather we did block on unknown future whoami values. The part that is present could be something entirely different. The 0x00 and 0xff is a kind of convenient hack. Unless they are clamped one way or the other you might well get something inbetween on a floating pin and a random response. I can see where you are coming from for wanting to allow future flexibility for whoami values changing, but I'd rather have it clean and consistent. Normally when there is a whoami change we need minor changes in the driver anyway to support it properly... > > Is this approach correct for you? Otherwise if you have a better idea, > it would be welcome. > > Thanks for your help. > > JB > > P.S.: Hope this e-mail is OK. I'm stuck with Outlook WebApp, > but that's not that bad to use. You have my sympathies ;) > >>> >>> Thanks. Jean-Baptiste >>> >>> From: Jonathan Cameron <jic23@kernel.org> >>> Sent: Sunday, May 14, 2017 16:49 >>> To: Jean-Baptiste Maneyrol; linux-iio@vger.kernel.org >>> Subject: Re: [PATCH] iio: imu: inv_mpu6050: test whoami against invalid values and do it first >>> >>> On 12/05/17 13:45, Jean-Baptiste Maneyrol wrote: >>>> SPI bus is never generating error during transfer, so a simple way to check >>>> if a chip is correctly connected on a SPI bus is to check a fixed value like >>>> whoami against valid values. 0x00 or 0xff can never happened. It is better >>>> to do this test first since if there is a problem with a DTS and an incorrect >>>> chip is wired instead, a write can be more damaging than a read. >>>> >>>> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> >>> Hmm. I'd a little unsure on this one. If we start getting into the >>> general game of protecting against every wrong DTS it may be a slippery >>> slope. >>> >>> Is there an actual platform out there that you know of that this will >>> protect? If so I can probably be persuaded. >>> >>> If not I would actually prefer if we checked against the specified ID >>> rather than relying on a different chip producing either high or low >>> consistently when hit with this query. >>> >>> So perhaps make the ID check a failure if it doesn't match any of the >>> supported parts? Bit of a pain for new devices as they won't work >>> immediately but conversely protects them against issues due to incorrect >>> identification as well. >>> >>> Jonathan >>> >>>> --- >>> drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 19 ++++++++++++------- >>> 1 file changed, 12 insertions(+), 7 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..42fb135 100644 >>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c >>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c >>> @@ -774,23 +774,28 @@ static int inv_check_and_setup_chip(struct inv_mpu6050_state *st) >>> 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) { >>> + if (regval == 0x00 || regval == 0xff) { >>> + dev_err(regmap_get_device(st->map), >>> + "invalid whoami probably io error\n"); >>> + return -EIO; >>> + } >>> dev_warn(regmap_get_device(st->map), >>> "whoami mismatch got %#02x expected %#02hhx for %s\n", >>> regval, st->hw->whoami, st->hw->name); >>> } >>> >>> + /* 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] 6+ messages in thread
end of thread, other threads:[~2017-05-21 11:43 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-05-12 12:45 [PATCH] iio: imu: inv_mpu6050: test whoami against invalid values and do it first Jean-Baptiste Maneyrol 2017-05-14 14:49 ` Jonathan Cameron 2017-05-15 8:42 ` Jean-Baptiste Maneyrol 2017-05-16 18:06 ` Jonathan Cameron 2017-05-17 15:03 ` Jean-Baptiste Maneyrol 2017-05-21 11:43 ` 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).