>From: Jonathan Cameron <jic23@xxxxxxxxxx>
>Sent: Tuesday, May 16, 2017 20:06
>To: Jean-Baptiste Maneyrol; linux-iio@xxxxxxxxxxxxxxx
>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 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.
>>
>> Thanks. Jean-Baptiste
>>
>> From: Jonathan Cameron <jic23@xxxxxxxxxx>
>> Sent: Sunday, May 14, 2017 16:49
>> To: Jean-Baptiste Maneyrol; linux-iio@xxxxxxxxxxxxxxx
>> 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@xxxxxxxxxxxxxx>
>> 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
>>
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
