| Message ID | 20230924222559.2038721-3-andreas@kemnade.info (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ARM: omap: omap4-embt2ws: Add IMU on control unit | expand |
On Mon, Sep 25, 2023 at 1:26 AM Andreas Kemnade <andreas@kemnade.info> wrote: > > Some boards fail in magnetometer probe if flag is not set. Which flag? Can you elaborate a bit more? Does it deserve the Fixes tag? ... > unsigned int val; > int ret; > + ret = regmap_update_bits(st->map, 0x1, 0x80, > + st->level_shifter ? 0x80 : 0); This is a bit cryptic, what does 1 stand for? Also unsigned int mask = BIT(7); ... val = st->level_shifter ? mask : 0; > + if (ret) > + return ret; ... > + st->level_shifter = device_property_present(dev, > + "invensense,level-shifter"); It was a recommendation to use _read_bool() when reading the value, while the result will be the same. -- With Best Regards, Andy Shevchenko
Hello, these are very old unsupported chips, thus this is not something I can easily find. Even after doing some archaeology. But when looking at register maps, it should only be used with MPU-9150 and not MPU-9250. I would feel much more comfortable to restrict this fix to MPU-9150 only (by testing chip_type against INV_MPU9150). Thanks, JB From: Andy Shevchenko <andy.shevchenko@gmail.com> Sent: Monday, September 25, 2023 13:07 To: Andreas Kemnade <andreas@kemnade.info> Cc: jic23@kernel.org <jic23@kernel.org>; lars@metafoo.de <lars@metafoo.de>; robh+dt@kernel.org <robh+dt@kernel.org>; krzysztof.kozlowski+dt@linaro.org <krzysztof.kozlowski+dt@linaro.org>; conor+dt@kernel.org <conor+dt@kernel.org>; bcousson@baylibre.com <bcousson@baylibre.com>; tony@atomide.com <tony@atomide.com>; Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; chenhuiz@axis.com <chenhuiz@axis.com>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; devicetree@vger.kernel.org <devicetree@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; linux-omap@vger.kernel.org <linux-omap@vger.kernel.org> Subject: Re: [PATCH 2/3] iio: imu: mpu6050: add level shifter flag On Mon, Sep 25, 2023 at 1: 26 AM Andreas Kemnade <andreas@ kemnade. info> wrote: > > Some boards fail in magnetometer probe if flag is not set. Which flag? Can you elaborate a bit more? Does it deserve the Fixes tag? .. . > unsigned ZjQcmQRYFpfptBannerStart This Message Is From an Untrusted Sender You have not previously corresponded with this sender. ZjQcmQRYFpfptBannerEnd On Mon, Sep 25, 2023 at 1:26 AM Andreas Kemnade <andreas@kemnade.info> wrote: > > Some boards fail in magnetometer probe if flag is not set. Which flag? Can you elaborate a bit more? Does it deserve the Fixes tag? ... > unsigned int val; > int ret; > + ret = regmap_update_bits(st->map, 0x1, 0x80, > + st->level_shifter ? 0x80 : 0); This is a bit cryptic, what does 1 stand for? Also unsigned int mask = BIT(7); ... val = st->level_shifter ? mask : 0; > + if (ret) > + return ret; ... > + st->level_shifter = device_property_present(dev, > + "invensense,level-shifter"); It was a recommendation to use _read_bool() when reading the value, while the result will be the same. -- With Best Regards, Andy Shevchenko
On Mon, 25 Sep 2023 14:07:58 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Sep 25, 2023 at 1:26 AM Andreas Kemnade <andreas@kemnade.info> wrote: > > > > Some boards fail in magnetometer probe if flag is not set. > > Which flag? Can you elaborate a bit more? > well see $subj. The level shifter flag. > Does it deserve the Fixes tag? > As there are only certain boards affected, they would also have to add the flag in dtb, I do not think it deservers a Fixes tag. Even in the board I have here, there are basically two mpu9150 connected per cable, only one of them needs the flag. > ... > > > unsigned int val; > > int ret; > > > + ret = regmap_update_bits(st->map, 0x1, 0x80, > > + st->level_shifter ? 0x80 : 0); > > This is a bit cryptic, what does 1 stand for? > Well, I do not find anything in https://invensense.tdk.com/wp-content/uploads/2015/02/MPU-9150-Register-Map.pdf I have just found a similar line in the ancient 3.0 vendor kernel. No symbolic names there. Regards, Andreas
On Mon, Sep 25, 2023 at 3:04 PM Andreas Kemnade <andreas@kemnade.info> wrote: > On Mon, 25 Sep 2023 14:07:58 +0300 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Mon, Sep 25, 2023 at 1:26 AM Andreas Kemnade <andreas@kemnade.info> wrote: > > > > > > Some boards fail in magnetometer probe if flag is not set. > > > > Which flag? Can you elaborate a bit more? > > > well see $subj. The level shifter flag. Well. it is better to have the commit message being self-contained. > > Does it deserve the Fixes tag? > > > As there are only certain boards affected, they would also have > to add the flag in dtb, I do not think it deservers a Fixes tag. > Even in the board I have here, there are basically two > mpu9150 connected per cable, only one of them needs the flag. OK. ... > > > unsigned int val; > > > int ret; > > > > > + ret = regmap_update_bits(st->map, 0x1, 0x80, > > > + st->level_shifter ? 0x80 : 0); > > > > This is a bit cryptic, what does 1 stand for? > > > Well, I do not find anything in > https://invensense.tdk.com/wp-content/uploads/2015/02/MPU-9150-Register-Map.pdf > I have just found a similar line in the ancient 3.0 vendor kernel. No symbolic > names there. Okay, perhaps a comment on top to point out this ("the code based on v3.0 vendor kernel, the meaning is unknown").
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_aux.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_aux.c index 7327e5723f961..15dbe88ff8c43 100644 --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_aux.c +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_aux.c @@ -71,6 +71,11 @@ int inv_mpu_aux_init(const struct inv_mpu6050_state *st) unsigned int val; int ret; + ret = regmap_update_bits(st->map, 0x1, 0x80, + st->level_shifter ? 0x80 : 0); + if (ret) + return ret; + /* configure i2c master */ val = INV_MPU6050_BITS_I2C_MST_CLK_400KHZ | INV_MPU6050_BIT_WAIT_FOR_ES; diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c index 29f906c884bd8..21cf794a41561 100644 --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c @@ -17,6 +17,7 @@ #include <linux/regulator/consumer.h> #include <linux/pm.h> #include <linux/pm_runtime.h> +#include <linux/property.h> #include <linux/iio/common/inv_sensors_timestamp.h> #include <linux/iio/iio.h> @@ -1495,6 +1496,8 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name, st->irq = irq; st->map = regmap; + st->level_shifter = device_property_present(dev, + "invensense,level-shifter"); pdata = dev_get_platdata(dev); if (!pdata) { result = iio_read_mount_matrix(dev, &st->orientation); diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h index ed5a96e78df05..7eba1439c8093 100644 --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h @@ -203,6 +203,7 @@ struct inv_mpu6050_state { s32 magn_raw_to_gauss[3]; struct iio_mount_matrix magn_orient; unsigned int suspended_sensors; + bool level_shifter; u8 *data; };
Some boards fail in magnetometer probe if flag is not set. Signed-off-by: Andreas Kemnade <andreas@kemnade.info> --- drivers/iio/imu/inv_mpu6050/inv_mpu_aux.c | 5 +++++ drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 3 +++ drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h | 1 + 3 files changed, 9 insertions(+)