2023-09-24 22:28:25

by Andreas Kemnade

[permalink] [raw]
Subject: [PATCH 0/3] ARM: omap: omap4-embt2ws: Add IMU on control unit

Contrary to the IMU at the head, a bit needs to be set to
make probe of the magnetometer successful.

Andreas Kemnade (3):
dt-bindings: iio: imu: mpu6050: Add level shifter
iio: imu: mpu6050: add level shifter flag
ARM: dts: omap: omap4-embt2ws: Add IMU at control unit

.../bindings/iio/imu/invensense,mpu6050.yaml | 2 ++
.../boot/dts/ti/omap/omap4-epson-embt2ws.dts | 17 ++++++++++++++++-
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 +
5 files changed, 27 insertions(+), 1 deletion(-)

--
2.39.2


2023-09-24 22:28:25

by Andreas Kemnade

[permalink] [raw]
Subject: [PATCH 2/3] iio: imu: mpu6050: add level shifter flag

Some boards fail in magnetometer probe if flag is not set.

Signed-off-by: Andreas Kemnade <[email protected]>
---
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(+)

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;
};

--
2.39.2

2023-09-25 12:17:19

by Andreas Kemnade

[permalink] [raw]
Subject: Re: [PATCH 2/3] iio: imu: mpu6050: add level shifter flag

On Mon, 25 Sep 2023 14:07:58 +0300
Andy Shevchenko <[email protected]> wrote:

> On Mon, Sep 25, 2023 at 1:26 AM Andreas Kemnade <[email protected]> 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

2023-09-25 14:19:20

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH 2/3] iio: imu: mpu6050: add level shifter flag

On Mon, Sep 25, 2023 at 1:26 AM Andreas Kemnade <[email protected]> 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

2023-09-25 15:05:57

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH 2/3] iio: imu: mpu6050: add level shifter flag

On Mon, Sep 25, 2023 at 3:04 PM Andreas Kemnade <[email protected]> wrote:
> On Mon, 25 Sep 2023 14:07:58 +0300
> Andy Shevchenko <[email protected]> wrote:
> > On Mon, Sep 25, 2023 at 1:26 AM Andreas Kemnade <[email protected]> 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").

--
With Best Regards,
Andy Shevchenko

2023-09-25 16:43:46

by Jean-Baptiste Maneyrol

[permalink] [raw]
Subject: Re: [PATCH 2/3] iio: imu: mpu6050: add level shifter flag

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 <[email protected]>
Sent: Monday, September 25, 2023 13:07
To: Andreas Kemnade <[email protected]>
Cc: [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; Jean-Baptiste Maneyrol <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>
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 <[email protected]> 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