| Message ID | 20190302165455.6594-1-cerg2010cerg2010@mail.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: gyro: mpu3050: fix chip ID reading | expand |
On Sat, 2 Mar 2019 19:54:55 +0300 Sergey Larin <cerg2010cerg2010@mail.ru> wrote: > According to the datasheet, the last bit of CHIP_ID register controls > I2C bus, and the first one is unused. Handle this correctly. > > Signed-off-by: Sergey Larin <cerg2010cerg2010@mail.ru> Fix certainly seems to be correct, but seeing as we only support i2c for this chip and that I assume the bottom bit is actually set (though reserved), currently this is a tidy up rather than a fix. Hence I'll take this the slow way rather than quickly as I would a fix with actual effect. Let me know if I've missed something! Jonathan > --- > drivers/iio/gyro/mpu3050-core.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/iio/gyro/mpu3050-core.c b/drivers/iio/gyro/mpu3050-core.c > index 77fac81a3adc..5ddebede31a6 100644 > --- a/drivers/iio/gyro/mpu3050-core.c > +++ b/drivers/iio/gyro/mpu3050-core.c > @@ -29,7 +29,8 @@ > > #include "mpu3050.h" > > -#define MPU3050_CHIP_ID 0x69 > +#define MPU3050_CHIP_ID 0x68 > +#define MPU3050_CHIP_ID_MASK 0x7E > > /* > * Register map: anything suffixed *_H is a big-endian high byte and always > @@ -1176,8 +1177,9 @@ int mpu3050_common_probe(struct device *dev, > goto err_power_down; > } > > - if (val != MPU3050_CHIP_ID) { > - dev_err(dev, "unsupported chip id %02x\n", (u8)val); > + if ((val & MPU3050_CHIP_ID_MASK) != MPU3050_CHIP_ID) { > + dev_err(dev, "unsupported chip id %02x\n", > + (u8)(val & MPU3050_CHIP_ID_MASK)); > ret = -ENODEV; > goto err_power_down; > }
On Sat, Mar 02, 2019 at 06:08:24PM +0000, Jonathan Cameron wrote: > On Sat, 2 Mar 2019 19:54:55 +0300 > Sergey Larin <cerg2010cerg2010@mail.ru> wrote: > > > According to the datasheet, the last bit of CHIP_ID register controls > > I2C bus, and the first one is unused. Handle this correctly. > > > > Signed-off-by: Sergey Larin <cerg2010cerg2010@mail.ru> > Fix certainly seems to be correct, but seeing as we only support i2c for > this chip and that I assume the bottom bit is actually set (though > reserved), currently this is a tidy up rather than a fix. > > Hence I'll take this the slow way rather than quickly as I would > a fix with actual effect. > > Let me know if I've missed something! > > Jonathan That's actually a fix for me. On my device, the bottom bit is set so driver reports an error. With this patch, driver probes OK, prints chip info to dmesg (die ID, revision, etc) and reports the data to sysfs, which looks correct. > > > --- > > drivers/iio/gyro/mpu3050-core.c | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/iio/gyro/mpu3050-core.c b/drivers/iio/gyro/mpu3050-core.c > > index 77fac81a3adc..5ddebede31a6 100644 > > --- a/drivers/iio/gyro/mpu3050-core.c > > +++ b/drivers/iio/gyro/mpu3050-core.c > > @@ -29,7 +29,8 @@ > > > > #include "mpu3050.h" > > > > -#define MPU3050_CHIP_ID 0x69 > > +#define MPU3050_CHIP_ID 0x68 > > +#define MPU3050_CHIP_ID_MASK 0x7E > > > > /* > > * Register map: anything suffixed *_H is a big-endian high byte and always > > @@ -1176,8 +1177,9 @@ int mpu3050_common_probe(struct device *dev, > > goto err_power_down; > > } > > > > - if (val != MPU3050_CHIP_ID) { > > - dev_err(dev, "unsupported chip id %02x\n", (u8)val); > > + if ((val & MPU3050_CHIP_ID_MASK) != MPU3050_CHIP_ID) { > > + dev_err(dev, "unsupported chip id %02x\n", > > + (u8)(val & MPU3050_CHIP_ID_MASK)); > > ret = -ENODEV; > > goto err_power_down; > > } >
On Sat, Mar 2, 2019 at 5:55 PM Sergey Larin <cerg2010cerg2010@mail.ru> wrote: > According to the datasheet, the last bit of CHIP_ID register controls > I2C bus, and the first one is unused. Handle this correctly. > > Signed-off-by: Sergey Larin <cerg2010cerg2010@mail.ru> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Sat, 2 Mar 2019 21:48:30 +0300 Sergey Larin <cerg2010cerg2010@mail.ru> wrote: > On Sat, Mar 02, 2019 at 06:08:24PM +0000, Jonathan Cameron wrote: > > On Sat, 2 Mar 2019 19:54:55 +0300 > > Sergey Larin <cerg2010cerg2010@mail.ru> wrote: > > > > > According to the datasheet, the last bit of CHIP_ID register controls > > > I2C bus, and the first one is unused. Handle this correctly. > > > > > > Signed-off-by: Sergey Larin <cerg2010cerg2010@mail.ru> > > Fix certainly seems to be correct, but seeing as we only support i2c for > > this chip and that I assume the bottom bit is actually set (though > > reserved), currently this is a tidy up rather than a fix. > > > > Hence I'll take this the slow way rather than quickly as I would > > a fix with actual effect. > > > > Let me know if I've missed something! > > > > Jonathan > > That's actually a fix for me. On my device, the bottom bit is set so driver > reports an error. With this patch, driver probes OK, prints chip info to > dmesg (die ID, revision, etc) and reports the data to sysfs, which looks > correct. Ah fair enough. Moved to the fixes-togreg branch with a note added to the description that there are real parts out there where this matters. Also added Linus' tag. This will upstream sometime after rc1 in a few weeks time. Thanks, Jonathan > > > > > > --- > > > drivers/iio/gyro/mpu3050-core.c | 8 +++++--- > > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/iio/gyro/mpu3050-core.c b/drivers/iio/gyro/mpu3050-core.c > > > index 77fac81a3adc..5ddebede31a6 100644 > > > --- a/drivers/iio/gyro/mpu3050-core.c > > > +++ b/drivers/iio/gyro/mpu3050-core.c > > > @@ -29,7 +29,8 @@ > > > > > > #include "mpu3050.h" > > > > > > -#define MPU3050_CHIP_ID 0x69 > > > +#define MPU3050_CHIP_ID 0x68 > > > +#define MPU3050_CHIP_ID_MASK 0x7E > > > > > > /* > > > * Register map: anything suffixed *_H is a big-endian high byte and always > > > @@ -1176,8 +1177,9 @@ int mpu3050_common_probe(struct device *dev, > > > goto err_power_down; > > > } > > > > > > - if (val != MPU3050_CHIP_ID) { > > > - dev_err(dev, "unsupported chip id %02x\n", (u8)val); > > > + if ((val & MPU3050_CHIP_ID_MASK) != MPU3050_CHIP_ID) { > > > + dev_err(dev, "unsupported chip id %02x\n", > > > + (u8)(val & MPU3050_CHIP_ID_MASK)); > > > ret = -ENODEV; > > > goto err_power_down; > > > } > >
diff --git a/drivers/iio/gyro/mpu3050-core.c b/drivers/iio/gyro/mpu3050-core.c index 77fac81a3adc..5ddebede31a6 100644 --- a/drivers/iio/gyro/mpu3050-core.c +++ b/drivers/iio/gyro/mpu3050-core.c @@ -29,7 +29,8 @@ #include "mpu3050.h" -#define MPU3050_CHIP_ID 0x69 +#define MPU3050_CHIP_ID 0x68 +#define MPU3050_CHIP_ID_MASK 0x7E /* * Register map: anything suffixed *_H is a big-endian high byte and always @@ -1176,8 +1177,9 @@ int mpu3050_common_probe(struct device *dev, goto err_power_down; } - if (val != MPU3050_CHIP_ID) { - dev_err(dev, "unsupported chip id %02x\n", (u8)val); + if ((val & MPU3050_CHIP_ID_MASK) != MPU3050_CHIP_ID) { + dev_err(dev, "unsupported chip id %02x\n", + (u8)(val & MPU3050_CHIP_ID_MASK)); ret = -ENODEV; goto err_power_down; }
According to the datasheet, the last bit of CHIP_ID register controls I2C bus, and the first one is unused. Handle this correctly. Signed-off-by: Sergey Larin <cerg2010cerg2010@mail.ru> --- drivers/iio/gyro/mpu3050-core.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)