iio mount matrix - revitalize missing bindings documentation and provide code for bmc150, bmg160, bma180, itg3200, hmc584x

From: Linus Walleij <[email protected]>

The mounting matrix for sensors was introduced in commit dfc57732ad38 ("iio:core: mounting matrix support")

However the device tree bindings are very terse and since this is a widely applicable property, we need a proper binding for it that the other bindings can reference. This will also be useful for other operating systems and sensor engineering at large.

I think all 3D sensors should support it, the current situation is probably that the mounting information is confined in magic userspace components rather than using the mounting matrix, which is not good for portability and reuse.

Cc Gregor Boirie <[email protected]>

Cc Sebastian Reichel <[email protected]>

Cc Samu Onkalo <[email protected]>

Cc [email protected]

Signed-off-by Linus Walleij <[email protected]>

---
 .../devicetree/bindings/iio/mount-matrix.txt  | 108 ++++++++++++++++++
 1 file changed, 108 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/mount-matrix.txt

Documentation/devicetree/bindings/iio/mount-matrix.txt new file +108 -0

new file mode 100644
index 000000000000..a3714727f739
@@ -0,0 +1,108 @@
+Mounting matrix
+
+The mounting matrix is a device tree property used to orient any IIO device
+that produce three-dimensional data in relation to the world where it is
+deployed.
+
+The purpose of the mounting matrix is to translate the sensor frame of
+reference into the device frame of reference using a translation matrix as
+defined in linear algebra.
+
+The typical usecase is that where a component has an internal representation
+of the (x,y,z) triplets, such as different registers to read these coordinates,
+and thus implying that the component should be mounted in a certain orientation
+relative to some specific device frame of reference.
+
+For example a device with some kind of screen, where the user is supposed to
+interact with the environment using an accelerometer, gyroscope or magnetometer
+mounted on the same chassis as this screen, will likely take the screen as
+reference to (x,y,z) orientation, with (x,y) corresponding to these axes on the
+screen and (z) being depth, the axis perpendicular to the screen.
+
+For a screen you probably want (x) coordinates to go from negative on the left
+to positive on the right and (z) depth to be negative under the screen and
+positive in front of it, toward the face of the user.
+
+A sensor can be mounted in any angle along the axes relative to the frame of
+reference. This means that the sensor may be flipped upside-down, left-right,
+or tilted at any angle relative to the frame of reference.
+
+Another frame of reference is how the device with its sensor relates to the
+external world, the environment where the device is deployed. Usually the data
+from the sensor is used to figure out how the device is oriented with respect
+to this world. When using the mounting matrix, the sensor and device orientation
+becomes identical and we can focus on the data as it relates to the surrounding
+world.
+
+Device-to-world examples for some three-dimensional sensor types:
+
+- Accelerometers have their world frame of reference toward the center of
+  gravity, usually to the core of the planet. A reading of the (x,y,z) values
+  from the sensor will give a projection of the gravity vector through the
+  device relative to the center of the planet, i.e. relative to its surface at
+  this point. Up and down in the world relative to the device frame of
+  reference can thus be determined. and users would likely expect a value of
+  9.81 m/s^2 upwards along the (z) axis, i.e. out of the screen when the device
+  is held with its screen flat on the planets surface and 0 on the other axes,
+  as the gravity vector is projected 1:1 onto the sensors (z)-axis.
+
+- Magnetometers (compasses) have their world frame of reference relative to the
+  geomagnetic field. The system orientation vis-a-vis the world is defined with
+  respect to the local earth geomagnetic reference frame where (y) is in the
+  ground plane and positive towards magnetic North, (x) is in the ground plane,
+  perpendicular to the North axis and positive towards the East and (z) is
+  perpendicular to the ground plane and positive upwards.
+
+- Gyroscopes detects the movement relative the device itself. The angular
+  velocity is defined as orthogonal to the plane of rotation, so if you put the
+  device on a flat surface and spin it around the z axis (such as rotating a
+  device with a screen lying flat on a table), you should get a negative value
+  along the (z) axis if rotated clockwise, and a positive value if rotated
+  counter-clockwise according to the right-hand rule.
+
+So unless the sensor is ideally mounted, we need a means to indicate the
+relative orientation of any given sensor of this type with respect to the
+frame of reference.
+
+To achieve this, use the device tree property "mount-matrix" for the sensor.
+This supplies a 3x3 rotation matrix in the strict linear algebraic sense,
+to orient the senor axes relative to a desired point of reference. This means
+the resulting values from the sensor, after scaling to proper units, should be
+multiplied by this matrix to give the proper vectors values in three-dimensional
+space, relative to the device or world point of reference.
+
+For more information, consult:
+https://en.wikipedia.org/wiki/Rotation_matrix
+
+The mounting matrix has the layout:
+
+ (x0, y0, z0)
+ (x1, y1, z1)
+ (x2, y2, z3)
+
+And it is represented as an array of strings containing the real values for
+producing the transformation matrix. The real values use a decimal point and
+a minus (-) to indicate a negative value.
+
+Examples:
+
+Identity matrix (nothing happens to the coordinates, which means the device was
+mechanically mounted in an ideal way and we need no transformation):
+
+mount-matrix = "1", "0", "0",
+               "0", "1", "0",
+               "0", "0", "1";
+
+The sensor is mounted 30 degrees (Pi/6 radians) tilted along the X axis, so we
+compensate by performing a -30 degrees rotation around the X axis:
+
+mount-matrix = "1", "0", "0",
+               "0", "0.866", "0.5",
+               "0", "-0.5", "0.866";
+
+The sensor is flipped 180 degrees (Pi radians) around the Z axis, i.e. mounted
+upside-down:
+
+mount-matrix = "0.998", "0.054", "0",
+               "-0.054", "0.998", "0",
+               "0", "0", "1";

Am 20.02.2019 um 17:10 schrieb Jonathan Cameron <[email protected]>:

Quoted 15 lines · On Wed, 20 Feb 2019 15:00:48 +0100 "H. Nikolaus Schaller" <[email protected]>…

From: Linus Walleij <[email protected]>

The mounting matrix for sensors was introduced in commit dfc57732ad38 ("iio:core: mounting matrix support")

However the device tree bindings are very terse and since this is a widely applicable property, we need a proper binding for it that the other bindings can reference. This will also be useful for other operating systems and sensor engineering at large.

I think all 3D sensors should support it, the current situation is probably that the mounting information is confined in magic userspace components rather than using the mounting matrix, which is not good for portability and reuse.

Quoted 5 lines

Cc Gregor Boirie <[email protected]>

Cc Sebastian Reichel <[email protected]>

Cc Samu Onkalo <[email protected]>

Cc [email protected]

Signed-off-by Linus Walleij <[email protected]>

Hmm. I looked back and seems there were still some outstanding questions on this last time around.

https://lore.kernel.org/linux-iio/[email protected]/

Particularly hard as ever to define the magnetic planes when near the magnetic poles when in 3D.

That needs cleaning up ideally before we apply this.

Jonathan

Quoted 119 lines

---
.../devicetree/bindings/iio/mount-matrix.txt | 108 ++++++++++++++++++

1 file changed, 108 insertions(+)

create mode 100644 Documentation/devicetree/bindings/iio/mount-matrix.txt

Documentation/devicetree/bindings/iio/mount-matrix.txt new file +108 -0

new file mode 100644
index 000000000000..a3714727f739
@@ -0,0 +1,108 @@
+Mounting matrix
+
+The mounting matrix is a device tree property used to orient any IIO device
+that produce three-dimensional data in relation to the world where it is
+deployed.
+
+The purpose of the mounting matrix is to translate the sensor frame of
+reference into the device frame of reference using a translation matrix as
+defined in linear algebra.
+
+The typical usecase is that where a component has an internal representation
+of the (x,y,z) triplets, such as different registers to read these coordinates,
+and thus implying that the component should be mounted in a certain orientation
+relative to some specific device frame of reference.
+
+For example a device with some kind of screen, where the user is supposed to
+interact with the environment using an accelerometer, gyroscope or magnetometer
+mounted on the same chassis as this screen, will likely take the screen as
+reference to (x,y,z) orientation, with (x,y) corresponding to these axes on the
+screen and (z) being depth, the axis perpendicular to the screen.
+
+For a screen you probably want (x) coordinates to go from negative on the left
+to positive on the right and (z) depth to be negative under the screen and
+positive in front of it, toward the face of the user.
+
+A sensor can be mounted in any angle along the axes relative to the frame of
+reference. This means that the sensor may be flipped upside-down, left-right,
+or tilted at any angle relative to the frame of reference.
+
+Another frame of reference is how the device with its sensor relates to the
+external world, the environment where the device is deployed. Usually the data
+from the sensor is used to figure out how the device is oriented with respect
+to this world. When using the mounting matrix, the sensor and device orientation
+becomes identical and we can focus on the data as it relates to the surrounding
+world.
+
+Device-to-world examples for some three-dimensional sensor types:
+
+- Accelerometers have their world frame of reference toward the center of
+  gravity, usually to the core of the planet. A reading of the (x,y,z) values
+  from the sensor will give a projection of the gravity vector through the
+  device relative to the center of the planet, i.e. relative to its surface at
+  this point. Up and down in the world relative to the device frame of
+  reference can thus be determined. and users would likely expect a value of
+  9.81 m/s^2 upwards along the (z) axis, i.e. out of the screen when the device
+  is held with its screen flat on the planets surface and 0 on the other axes,
+  as the gravity vector is projected 1:1 onto the sensors (z)-axis.
+
+- Magnetometers (compasses) have their world frame of reference relative to the
+  geomagnetic field. The system orientation vis-a-vis the world is defined with
+  respect to the local earth geomagnetic reference frame where (y) is in the
+  ground plane and positive towards magnetic North, (x) is in the ground plane,
+  perpendicular to the North axis and positive towards the East and (z) is
+  perpendicular to the ground plane and positive upwards.
+
+- Gyroscopes detects the movement relative the device itself. The angular
+  velocity is defined as orthogonal to the plane of rotation, so if you put the
+  device on a flat surface and spin it around the z axis (such as rotating a
+  device with a screen lying flat on a table), you should get a negative value
+  along the (z) axis if rotated clockwise, and a positive value if rotated
+  counter-clockwise according to the right-hand rule.
+
+So unless the sensor is ideally mounted, we need a means to indicate the
+relative orientation of any given sensor of this type with respect to the
+frame of reference.
+
+To achieve this, use the device tree property "mount-matrix" for the sensor.
+This supplies a 3x3 rotation matrix in the strict linear algebraic sense,
+to orient the senor axes relative to a desired point of reference. This means
+the resulting values from the sensor, after scaling to proper units, should be
+multiplied by this matrix to give the proper vectors values in three-dimensional
+space, relative to the device or world point of reference.
+
+For more information, consult:
+https://en.wikipedia.org/wiki/Rotation_matrix
+
+The mounting matrix has the layout:
+
+ (x0, y0, z0)
+ (x1, y1, z1)
+ (x2, y2, z3)
+
+And it is represented as an array of strings containing the real values for
+producing the transformation matrix. The real values use a decimal point and
+a minus (-) to indicate a negative value.
+
+Examples:
+
+Identity matrix (nothing happens to the coordinates, which means the device was
+mechanically mounted in an ideal way and we need no transformation):
+
+mount-matrix = "1", "0", "0",
+               "0", "1", "0",
+               "0", "0", "1";
+
+The sensor is mounted 30 degrees (Pi/6 radians) tilted along the X axis, so we
+compensate by performing a -30 degrees rotation around the X axis:
+
+mount-matrix = "1", "0", "0",
+               "0", "0.866", "0.5",
+               "0", "-0.5", "0.866";
+
+The sensor is flipped 180 degrees (Pi radians) around the Z axis, i.e. mounted
+upside-down:
+
+mount-matrix = "0.998", "0.054", "0",
+               "-0.054", "0.998", "0",
+               "0", "0", "1";

Quoted 7 lines

Am 20.02.2019 um 17:10 schrieb Jonathan Cameron <[email protected]>:
On Wed, 20 Feb 2019 15:00:48 +0100
"H. Nikolaus Schaller" <[email protected]> wrote:

Hmm. I looked back and seems there were still some outstanding questions on this last time around.

Quoted 6 lines

https://lore.kernel.org/linux-iio/[email protected]/

Particularly hard as ever to define the magnetic planes when near the magnetic poles when in 3D.

That needs cleaning up ideally before we apply this.

Agreed. I already placed my proposal for improvement on top of that, but I did not try to solve the magnetic planes issue.

Maybe Linus could squeeze the patches and edit a new one that solves all issues.

+};

Am 20.02.2019 um 17:07 schrieb Andy Shevchenko <[email protected]>:

drivers/iio/accel/bmc150-accel-core.c static ssize_t bmc150_accel_get_fifo_state(struct device *dev, · line 804

Quoted 9 lines · H. Nikolaus Schaller

This patch allows to read a mount-matrix device tree property and report to user-space or in-kernel iio clients.

drivers/iio/accel/bmc150-accel-core.c +5 -0

+static const struct iio_mount_matrix *
+bmc150_accel_get_mount_matrix(const struct iio_dev *indio_dev,
+				const struct iio_chan_spec *chan)
+{
+	return &((struct bmc150_accel_data *)iio_priv(indio_dev))->orientation;

It's hard to read.

Can you split such lines in your series to something like

struct bmc150_accel_data *data = iio_priv(indio_dev);

return &data->orientation;

?

drivers/iio/accel/bmc150-accel-core.c static ssize_t bmc150_accel_get_fifo_state(struct device *dev, · line 809

Quoted 6 lines

drivers/iio/accel/bmc150-accel-core.c +5 -0

+}
+
+static const struct iio_chan_spec_ext_info bmc150_accel_ext_info[] = {
+	IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, bmc150_accel_get_mount_matrix),
+	{ },

Terminator lines better without comma.

Quoted 2 lines

+};

Signature · 2 lines

With Best Regards,
Andy Shevchenko

This patch allows to read a mount-matrix device tree property and report to user-space or in-kernel iio clients.

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
 drivers/iio/accel/bmc150-accel-core.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

drivers/iio/accel/bmc150-accel-core.c +1 -0

index 383c802eb5b8..9178846cfddc 100644
@@ -204,6 +204,7 @@ struct bmc150_accel_data {
 	int ev_enable_state;
 	int64_t timestamp, old_timestamp; /* Only used in hw fifo mode. */
 	const struct bmc150_accel_chip_info *chip_info;
+	struct iio_mount_matrix orientation;
 };
 
 static const struct {

drivers/iio/accel/bmc150-accel-core.c +13 -0

+}
+
+static const struct iio_chan_spec_ext_info bmc150_accel_ext_info[] = {
+	IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, bmc150_accel_get_mount_matrix),
+	{ },
+};
+
 static IIO_CONST_ATTR(hwfifo_watermark_min, "1");
 static IIO_CONST_ATTR(hwfifo_watermark_max,
 		      __stringify(BMC150_ACCEL_FIFO_LENGTH));

@@ -978,6 +991,7 @@ static const struct iio_event_spec bmc150_accel_event = {
 		.shift = 16 - (bits),					\
 		.endianness = IIO_LE,					\
 	},								\
+	.ext_info = bmc150_accel_ext_info,				\
 	.event_spec = &bmc150_accel_event,				\
 	.num_event_specs = 1						\
 }

@@ -1555,6 +1569,11 @@ int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq,
 
 	data->regmap = regmap;
 
+	ret = of_iio_read_mount_matrix(dev, "mount-matrix",
+				       &data->orientation);
+	if (ret)
+		return ret;
+
 	ret = bmc150_accel_chip_init(data);
 	if (ret < 0)
 		return ret;

Am 20.02.2019 um 17:14 schrieb Jonathan Cameron <[email protected]>:

On Wed, 20 Feb 2019 15:00:50 +0100
"H. Nikolaus Schaller" <[email protected]> wrote:

Quoted 5 lines

This patch allows to read a mount-matrix device tree property and report to user-space or in-kernel iio clients.

Signed-off-by H. Nikolaus Schaller <[email protected]>

This will clash with Andy's current patch, but I'll fix that up if needed. Otherwise, one trivial suggestion inline.

Jonathan

Quoted 20 lines

---
drivers/iio/accel/bmc150-accel-core.c | 19 +++++++++++++++++++

1 file changed, 19 insertions(+)

drivers/iio/accel/bmc150-accel-core.c +1 -0

index 383c802eb5b8..9178846cfddc 100644
@@ -204,6 +204,7 @@ struct bmc150_accel_data {
 	int ev_enable_state;
 	int64_t timestamp, old_timestamp; /* Only used in hw fifo mode. */
 	const struct bmc150_accel_chip_info *chip_info;
+	struct iio_mount_matrix orientation;
 };
 
 static const struct {

@@ -796,6 +797,18 @@ static ssize_t bmc150_accel_get_fifo_state(struct device *dev,
 	return sprintf(buf, "%d\n", state);
 }
 

drivers/iio/accel/bmc150-accel-core.c static ssize_t bmc150_accel_get_fifo_state(struct device *dev, · line 804

Quoted 5 lines

drivers/iio/accel/bmc150-accel-core.c +5 -0

+static const struct iio_mount_matrix *
+bmc150_accel_get_mount_matrix(const struct iio_dev *indio_dev,
+				const struct iio_chan_spec *chan)
+{
+	return &((struct bmc150_accel_data *)iio_priv(indio_dev))->orientation;

I would use a local variable as that casting is less than simple to parse to my eyes anyway!

Quoted 23 lines

drivers/iio/accel/bmc150-accel-core.c +8 -0

+}
+
+static const struct iio_chan_spec_ext_info bmc150_accel_ext_info[] = {
+	IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, bmc150_accel_get_mount_matrix),
+	{ },
+};
+
 static IIO_CONST_ATTR(hwfifo_watermark_min, "1");
 static IIO_CONST_ATTR(hwfifo_watermark_max,
 		      __stringify(BMC150_ACCEL_FIFO_LENGTH));

@@ -978,6 +991,7 @@ static const struct iio_event_spec bmc150_accel_event = {
 		.shift = 16 - (bits),					\
 		.endianness = IIO_LE,					\
 	},								\
+	.ext_info = bmc150_accel_ext_info,				\
 	.event_spec = &bmc150_accel_event,				\
 	.num_event_specs = 1						\
 }

@@ -1555,6 +1569,11 @@ int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq,
 
 	data->regmap = regmap;
 

drivers/iio/accel/bmc150-accel-core.c int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq, · line 1579

Quoted 8 lines

drivers/iio/accel/bmc150-accel-core.c +5 -0

+	ret = of_iio_read_mount_matrix(dev, "mount-matrix",
+				       &data->orientation);
+	if (ret)
+		return ret;
+
 	ret = bmc150_accel_chip_init(data);
 	if (ret < 0)
 		return ret;

Quoted 5 lines

Am 20.02.2019 um 17:14 schrieb Jonathan Cameron <[email protected]>:

On Wed, 20 Feb 2019 15:00:50 +0100
"H. Nikolaus Schaller" <[email protected]> wrote:

Quoted 7 lines

Quoted 5 lines

This patch allows to read a mount-matrix device tree property and report to user-space or in-kernel iio clients.

Signed-off-by H. Nikolaus Schaller <[email protected]>

This will clash with Andy's current patch, but I'll fix that up if needed. Otherwise, one trivial suggestion inline.

Ok, thanks!

Quoted 23 lines

Jonathan

Quoted 20 lines

---
drivers/iio/accel/bmc150-accel-core.c | 19 +++++++++++++++++++

1 file changed, 19 insertions(+)

drivers/iio/accel/bmc150-accel-core.c +1 -0

index 383c802eb5b8..9178846cfddc 100644
@@ -204,6 +204,7 @@ struct bmc150_accel_data {
 	int ev_enable_state;
 	int64_t timestamp, old_timestamp; /* Only used in hw fifo mode. */
 	const struct bmc150_accel_chip_info *chip_info;
+	struct iio_mount_matrix orientation;
 };
 
 static const struct {

@@ -796,6 +797,18 @@ static ssize_t bmc150_accel_get_fifo_state(struct device *dev,
 	return sprintf(buf, "%d\n", state);
 }
 

Quoted 8 lines

drivers/iio/accel/bmc150-accel-core.c static ssize_t bmc150_accel_get_fifo_state(struct device *dev, · line 804

Quoted 5 lines

drivers/iio/accel/bmc150-accel-core.c +5 -0

+static const struct iio_mount_matrix *
+bmc150_accel_get_mount_matrix(const struct iio_dev *indio_dev,
+				const struct iio_chan_spec *chan)
+{
+	return &((struct bmc150_accel_data *)iio_priv(indio_dev))->orientation;  

I would use a local variable as that casting is less than simple to parse to my eyes anyway!

Yes, Andy already suggested that. I just happend to pick one of the only two bad examples from all iio drivers as template...

drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
drivers/iio/magnetometer/ak8975.c

Quoted 24 lines

Quoted 24 lines

drivers/iio/accel/bmc150-accel-core.c +8 -0

+}
+
+static const struct iio_chan_spec_ext_info bmc150_accel_ext_info[] = {
+	IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, bmc150_accel_get_mount_matrix),
+	{ },
+};
+
 static IIO_CONST_ATTR(hwfifo_watermark_min, "1");
 static IIO_CONST_ATTR(hwfifo_watermark_max,
 		      __stringify(BMC150_ACCEL_FIFO_LENGTH));

@@ -978,6 +991,7 @@ static const struct iio_event_spec bmc150_accel_event = {
 		.shift = 16 - (bits),					\
 		.endianness = IIO_LE,					\
 	},								\
+	.ext_info = bmc150_accel_ext_info,				\
 	.event_spec = &bmc150_accel_event,				\
 	.num_event_specs = 1						\
 }

@@ -1555,6 +1569,11 @@ int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq,
 
 	data->regmap = regmap;
 

Quoted 9 lines

drivers/iio/accel/bmc150-accel-core.c int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq, · line 1579

Quoted 8 lines

drivers/iio/accel/bmc150-accel-core.c +5 -0

+	ret = of_iio_read_mount_matrix(dev, "mount-matrix",
+				       &data->orientation);
+	if (ret)
+		return ret;
+
 	ret = bmc150_accel_chip_init(data);
 	if (ret < 0)
 		return ret;  

BR and thanks,
Nikolaus

drivers/iio/accel/bma180.c +0 -0

 	data->part_info->chip_disable(data);
 
 	return ret;

Am 20.02.2019 um 17:09 schrieb Andy Shevchenko <[email protected]>:

Quoted 9 lines · H. Nikolaus Schaller

This simplifies the code a little.

drivers/iio/accel/bma180.c +0 -7

-err_buffer_cleanup:
-	iio_triggered_buffer_cleanup(indio_dev);
-err_trigger_unregister:
-	if (data->trig)
-		iio_trigger_unregister(data->trig);
-err_trigger_free:
-	iio_trigger_free(data->trig);

drivers/iio/accel/bma180.c static int bma180_probe(struct i2c_client *client, · line 792

Quoted 2 lines

drivers/iio/accel/bma180.c +1 -1

-err_chip_disable:
+err:

Please, leave the original label name as it's more self-explanatory.

Quoted 4 lines

	data->part_info->chip_disable(data);

	return ret;

Signature · 2 lines

With Best Regards,
Andy Shevchenko

This simplifies the code a little.

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
 drivers/iio/accel/bma180.c | 56 ++++++++++++++------------------------
 1 file changed, 21 insertions(+), 35 deletions(-)

drivers/iio/accel/bma180.c +21 -28

index 248be67e4f41..3f5ee2d495d3 100644
@@ -738,7 +738,7 @@ static int bma180_probe(struct i2c_client *client,
 
 	ret = data->part_info->chip_config(data);
 	if (ret < 0)
-		goto err_chip_disable;
+		goto err;
 
 	mutex_init(&data->mutex);
 	indio_dev->dev.parent = &client->dev;

@@ -748,12 +748,25 @@ static int bma180_probe(struct i2c_client *client,
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &bma180_info;
 
+	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
+			bma180_trigger_handler, NULL);
+	if (ret < 0) {
+		dev_err(&client->dev, "unable to setup iio triggered buffer\n");
+		goto err;
+	}
+
+	ret = devm_iio_device_register(&client->dev, indio_dev);
+	if (ret < 0) {
+		dev_err(&client->dev, "unable to register iio device\n");
+		goto err;
+	}
+
 	if (client->irq > 0) {
-		data->trig = iio_trigger_alloc("%s-dev%d", indio_dev->name,
-			indio_dev->id);
+		data->trig = devm_iio_trigger_alloc(&client->dev, "%s-dev%d",
+			indio_dev->name, indio_dev->id);
 		if (!data->trig) {
 			ret = -ENOMEM;
-			goto err_chip_disable;
+			goto err;
 		}
 
 		ret = devm_request_irq(&client->dev, client->irq,

@@ -761,7 +774,7 @@ static int bma180_probe(struct i2c_client *client,
 			"bma180_event", data->trig);
 		if (ret) {
 			dev_err(&client->dev, "unable to request IRQ\n");
-			goto err_trigger_free;
+			goto err;
 		}
 
 		data->trig->dev.parent = &client->dev;

@@ -769,34 +782,14 @@ static int bma180_probe(struct i2c_client *client,
 		iio_trigger_set_drvdata(data->trig, indio_dev);
 		indio_dev->trig = iio_trigger_get(data->trig);
 
-		ret = iio_trigger_register(data->trig);
+		ret = devm_iio_trigger_register(&client->dev, data->trig);
 		if (ret)
-			goto err_trigger_free;
-	}
-
-	ret = iio_triggered_buffer_setup(indio_dev, NULL,
-			bma180_trigger_handler, NULL);
-	if (ret < 0) {
-		dev_err(&client->dev, "unable to setup iio triggered buffer\n");
-		goto err_trigger_unregister;
-	}
-
-	ret = iio_device_register(indio_dev);
-	if (ret < 0) {
-		dev_err(&client->dev, "unable to register iio device\n");
-		goto err_buffer_cleanup;
+			goto err;
 	}
 
 	return 0;
 
-err_buffer_cleanup:
-	iio_triggered_buffer_cleanup(indio_dev);
-err_trigger_unregister:
-	if (data->trig)
-		iio_trigger_unregister(data->trig);
-err_trigger_free:
-	iio_trigger_free(data->trig);
-err_chip_disable:
+err:
 	data->part_info->chip_disable(data);
 
 	return ret;

drivers/iio/accel/bma180.c +0 -0

 	mutex_lock(&data->mutex);
 	data->part_info->chip_disable(data);
 	mutex_unlock(&data->mutex);

Am 20.02.2019 um 17:18 schrieb Jonathan Cameron <[email protected]>:

Quoted 1 line · On Wed, 20 Feb 2019 15:00:52 +0100 "H. Nikolaus Schaller" <[email protected]>…

This simplifies the code a little.

It does, but at the cost of introducing potential race conditions. Please don't do this. See below for why and a suggestion on how to resolve things if you want to make this change safely.

Jonathan

Quoted 96 lines

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
drivers/iio/accel/bma180.c | 56 ++++++++++++++------------------------

1 file changed, 21 insertions(+), 35 deletions(-)

drivers/iio/accel/bma180.c +21 -28

index 248be67e4f41..3f5ee2d495d3 100644
@@ -738,7 +738,7 @@ static int bma180_probe(struct i2c_client *client,
 
 	ret = data->part_info->chip_config(data);
 	if (ret < 0)
-		goto err_chip_disable;
+		goto err;
 
 	mutex_init(&data->mutex);
 	indio_dev->dev.parent = &client->dev;

@@ -748,12 +748,25 @@ static int bma180_probe(struct i2c_client *client,
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &bma180_info;
 
+	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
+			bma180_trigger_handler, NULL);
+	if (ret < 0) {
+		dev_err(&client->dev, "unable to setup iio triggered buffer\n");
+		goto err;
+	}
+
+	ret = devm_iio_device_register(&client->dev, indio_dev);
+	if (ret < 0) {
+		dev_err(&client->dev, "unable to register iio device\n");
+		goto err;
+	}
+
 	if (client->irq > 0) {
-		data->trig = iio_trigger_alloc("%s-dev%d", indio_dev->name,
-			indio_dev->id);
+		data->trig = devm_iio_trigger_alloc(&client->dev, "%s-dev%d",
+			indio_dev->name, indio_dev->id);
 		if (!data->trig) {
 			ret = -ENOMEM;
-			goto err_chip_disable;
+			goto err;
 		}
 
 		ret = devm_request_irq(&client->dev, client->irq,

@@ -761,7 +774,7 @@ static int bma180_probe(struct i2c_client *client,
 			"bma180_event", data->trig);
 		if (ret) {
 			dev_err(&client->dev, "unable to request IRQ\n");
-			goto err_trigger_free;
+			goto err;
 		}
 
 		data->trig->dev.parent = &client->dev;

@@ -769,34 +782,14 @@ static int bma180_probe(struct i2c_client *client,
 		iio_trigger_set_drvdata(data->trig, indio_dev);
 		indio_dev->trig = iio_trigger_get(data->trig);
 
-		ret = iio_trigger_register(data->trig);
+		ret = devm_iio_trigger_register(&client->dev, data->trig);
 		if (ret)
-			goto err_trigger_free;
-	}
-
-	ret = iio_triggered_buffer_setup(indio_dev, NULL,
-			bma180_trigger_handler, NULL);
-	if (ret < 0) {
-		dev_err(&client->dev, "unable to setup iio triggered buffer\n");
-		goto err_trigger_unregister;
-	}
-
-	ret = iio_device_register(indio_dev);
-	if (ret < 0) {
-		dev_err(&client->dev, "unable to register iio device\n");
-		goto err_buffer_cleanup;
+			goto err;
 	}
 
 	return 0;
 
-err_buffer_cleanup:
-	iio_triggered_buffer_cleanup(indio_dev);
-err_trigger_unregister:
-	if (data->trig)
-		iio_trigger_unregister(data->trig);
-err_trigger_free:
-	iio_trigger_free(data->trig);
-err_chip_disable:
+err:
 	data->part_info->chip_disable(data);
 
 	return ret;

drivers/iio/accel/bma180.c static int bma180_remove(struct i2c_client *client) · line 816

Quoted 10 lines

drivers/iio/accel/bma180.c +0 -7

@@ -807,13 +800,6 @@ static int bma180_remove(struct i2c_client *client)
 	struct iio_dev *indio_dev = i2c_get_clientdata(client);
 	struct bma180_data *data = iio_priv(indio_dev);
 
-	iio_device_unregister(indio_dev);
-	iio_triggered_buffer_cleanup(indio_dev);
-	if (data->trig) {
-		iio_trigger_unregister(data->trig);
-		iio_trigger_free(data->trig);
-	}
-

Now we disable the device before removing it's userspace interface. Not a good idea.

Generally I will insist on the remove order always being the precise opposite of probe simply because it is obviously correct. That includes cases which can be simply argued to be fine (not this one which isn't). The reason is readability trumps saving a few lines of code and it's a lot easier to check a probe and remove function against each other if the order is maintained.

Of course, there are ways to do this by making all the unwind managed, but you haven't done this here. devm_add_action_or_reset is handy for this if you want to do it.

Quoted 3 lines

drivers/iio/accel/bma180.c +0 -0

 	mutex_lock(&data->mutex);
 	data->part_info->chip_disable(data);
 	mutex_unlock(&data->mutex);

This patch allows to read a mount-matrix device tree property and report to user-space or in-kernel iio clients.

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
 drivers/iio/magnetometer/hmc5843.h      |  1 +
 drivers/iio/magnetometer/hmc5843_core.c | 14 ++++++++++++++
 2 files changed, 15 insertions(+)

drivers/iio/magnetometer/hmc5843.h +1 -0

index a75224cf99df..e3e22d2508d3 100644
@@ -43,6 +43,7 @@ struct hmc5843_data {
 	struct mutex lock;
 	struct regmap *regmap;
 	const struct hmc5843_chip_info *variant;
+	struct iio_mount_matrix orientation;
 	__be16 buffer[8];
 };
 

drivers/iio/magnetometer/hmc5843_core.c +14 -0

index ada142fb7aa3..e6b6da70c152 100644
@@ -237,6 +237,13 @@ int hmc5843_set_measurement_configuration(struct iio_dev *indio_dev,
 	return hmc5843_set_meas_conf(data, meas_conf);
 }
 
+static const struct iio_mount_matrix *
+hmc5843_get_mount_matrix(const struct iio_dev *indio_dev,
+			  const struct iio_chan_spec *chan)
+{
+	return &((struct hmc5843_data *)iio_priv(indio_dev))->orientation;
+}
+
 static const struct iio_enum hmc5843_meas_conf_enum = {
 	.items = hmc5843_meas_conf_modes,
 	.num_items = ARRAY_SIZE(hmc5843_meas_conf_modes),

@@ -247,6 +254,7 @@ static const struct iio_enum hmc5843_meas_conf_enum = {
 static const struct iio_chan_spec_ext_info hmc5843_ext_info[] = {
 	IIO_ENUM("meas_conf", true, &hmc5843_meas_conf_enum),
 	IIO_ENUM_AVAILABLE("meas_conf", &hmc5843_meas_conf_enum),
+	IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, hmc5843_get_mount_matrix),
 	{ },
 };
 

@@ -260,6 +268,7 @@ static const struct iio_enum hmc5983_meas_conf_enum = {
 static const struct iio_chan_spec_ext_info hmc5983_ext_info[] = {
 	IIO_ENUM("meas_conf", true, &hmc5983_meas_conf_enum),
 	IIO_ENUM_AVAILABLE("meas_conf", &hmc5983_meas_conf_enum),
+	IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, hmc5843_get_mount_matrix),
 	{ },
 };
 

@@ -635,6 +644,11 @@ int hmc5843_common_probe(struct device *dev, struct regmap *regmap,
 	data->variant = &hmc5843_chip_info_tbl[id];
 	mutex_init(&data->lock);
 
+	ret = of_iio_read_mount_matrix(dev, "mount-matrix",
+				&data->orientation);
+	if (ret)
+		return ret;
+
 	indio_dev->dev.parent = dev;
 	indio_dev->name = name;
 	indio_dev->info = &hmc5843_info;

Am 20.02.2019 um 17:19 schrieb Jonathan Cameron <[email protected]>:

On Wed, 20 Feb 2019 15:00:56 +0100
"H. Nikolaus Schaller" <[email protected]> wrote:

Quoted 5 lines

This patch allows to read a mount-matrix device tree property and report to user-space or in-kernel iio clients.

Signed-off-by H. Nikolaus Schaller <[email protected]>

The rest of these are all fine, though I suggest considering a local variable rather than the fiddly bit of casting.

Jonathan

Quoted 63 lines

---
drivers/iio/magnetometer/hmc5843.h | 1 +
drivers/iio/magnetometer/hmc5843_core.c | 14 ++++++++++++++

2 files changed, 15 insertions(+)

drivers/iio/magnetometer/hmc5843.h +1 -0

index a75224cf99df..e3e22d2508d3 100644
@@ -43,6 +43,7 @@ struct hmc5843_data {
 	struct mutex lock;
 	struct regmap *regmap;
 	const struct hmc5843_chip_info *variant;
+	struct iio_mount_matrix orientation;
 	__be16 buffer[8];
 };
 

drivers/iio/magnetometer/hmc5843_core.c +14 -0

index ada142fb7aa3..e6b6da70c152 100644
@@ -237,6 +237,13 @@ int hmc5843_set_measurement_configuration(struct iio_dev *indio_dev,
 	return hmc5843_set_meas_conf(data, meas_conf);
 }
 
+static const struct iio_mount_matrix *
+hmc5843_get_mount_matrix(const struct iio_dev *indio_dev,
+			  const struct iio_chan_spec *chan)
+{
+	return &((struct hmc5843_data *)iio_priv(indio_dev))->orientation;
+}
+
 static const struct iio_enum hmc5843_meas_conf_enum = {
 	.items = hmc5843_meas_conf_modes,
 	.num_items = ARRAY_SIZE(hmc5843_meas_conf_modes),

@@ -247,6 +254,7 @@ static const struct iio_enum hmc5843_meas_conf_enum = {
 static const struct iio_chan_spec_ext_info hmc5843_ext_info[] = {
 	IIO_ENUM("meas_conf", true, &hmc5843_meas_conf_enum),
 	IIO_ENUM_AVAILABLE("meas_conf", &hmc5843_meas_conf_enum),
+	IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, hmc5843_get_mount_matrix),
 	{ },
 };
 

@@ -260,6 +268,7 @@ static const struct iio_enum hmc5983_meas_conf_enum = {
 static const struct iio_chan_spec_ext_info hmc5983_ext_info[] = {
 	IIO_ENUM("meas_conf", true, &hmc5983_meas_conf_enum),
 	IIO_ENUM_AVAILABLE("meas_conf", &hmc5983_meas_conf_enum),
+	IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, hmc5843_get_mount_matrix),
 	{ },
 };
 

@@ -635,6 +644,11 @@ int hmc5843_common_probe(struct device *dev, struct regmap *regmap,
 	data->variant = &hmc5843_chip_info_tbl[id];
 	mutex_init(&data->lock);
 
+	ret = of_iio_read_mount_matrix(dev, "mount-matrix",
+				&data->orientation);
+	if (ret)
+		return ret;
+
 	indio_dev->dev.parent = dev;
 	indio_dev->name = name;
 	indio_dev->info = &hmc5843_info;

This patch series adds the mount-matrix to several iio sensor drivers used in handheld devices.

The mount-matrix translates the quite arbitrary orientation of the sensor on some printed circuit board to user-tangible orientation in handheld devices that relates to typical screen orientation.

There was a bindings documentation by Linus Walleij but the patch did not make it into mainline. Therefore I resend it here.

Next I have added some clarifications (at least I hope it clarifies) in a second patch.

Finally, the patch set implements the hooks for the mount matrix in several iio drivers: bmc150, bma180, bmg160, itg3200, hmc5843. This includes also one patch for the bma180 to convert it to devm API.

We use them in different variants of the omap3-gta04 so a separate patch set will provide device tree additions for them.

H. Nikolaus Schaller (8):
iio: bindings: clarifications for mount-matrix bindings
iio: accel: bmc150: add mount matrix support
iio: accel: bma180: add mount matrix support
iio: accel: bma180: convert to devm
iio: gyro: bmg160: add mount matrix support
iio: gyro: itg3200: add mount matrix support
iio: magnetometer: bmc150: add mount matrix support
iio: magnetometer: hmc5843: add mount matrix support

Linus Walleij (1):
iio: document bindings for mounting matrices

 .../devicetree/bindings/iio/mount-matrix.txt  | 162 ++++++++++++++++++
 drivers/iio/accel/bma180.c                    |  70 ++++----
 drivers/iio/accel/bmc150-accel-core.c         |  19 ++
 drivers/iio/gyro/bmg160_core.c                |  19 ++
 drivers/iio/gyro/itg3200_core.c               |  18 ++
 drivers/iio/magnetometer/bmc150_magn.c        |  19 ++
 drivers/iio/magnetometer/hmc5843.h            |   1 +
 drivers/iio/magnetometer/hmc5843_core.c       |  14 ++
 include/linux/iio/gyro/itg3200.h              |   1 +
 9 files changed, 288 insertions(+), 35 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/mount-matrix.txt

Signature · 1 line

2.19.1

Am 20.02.2019 um 17:13 schrieb Andy Shevchenko <[email protected]>:

Quoted 13 lines · H. Nikolaus Schaller

This patch series adds the mount-matrix to several iio sensor drivers used in handheld devices.

The mount-matrix translates the quite arbitrary orientation of the sensor on some printed circuit board to user-tangible orientation in handheld devices that relates to typical screen orientation.

There was a bindings documentation by Linus Walleij but the patch did not make it into mainline. Therefore I resend it here.

Next I have added some clarifications (at least I hope it clarifies) in a second patch.

Quoted 6 lines

Finally, the patch set implements the hooks for the mount matrix in several iio drivers: bmc150, bma180, bmg160, itg3200, hmc5843. This includes also one patch for the bma180 to convert it to devm API.

We use them in different variants of the omap3-gta04 so a separate patch set will provide device tree additions for them.

Thanks for the patch, overall good stuff there. Couple of things, though:
- address my comments (consider them against entire series)
- check my patch I sent today to support this from ACPI

I wouldn't like to spread more drivers use specific of_foo_bar stuff where it's easily to support everything (note: device_property_* calls supports software fw nodes as well).

Perhaps, you may incorporate my patch into your nice series.

Quoted 31 lines

H. Nikolaus Schaller (8):
iio: bindings: clarifications for mount-matrix bindings
iio: accel: bmc150: add mount matrix support
iio: accel: bma180: add mount matrix support
iio: accel: bma180: convert to devm
iio: gyro: bmg160: add mount matrix support
iio: gyro: itg3200: add mount matrix support
iio: magnetometer: bmc150: add mount matrix support
iio: magnetometer: hmc5843: add mount matrix support

Linus Walleij (1):
iio: document bindings for mounting matrices

.../devicetree/bindings/iio/mount-matrix.txt  | 162 ++++++++++++++++++
drivers/iio/accel/bma180.c                    |  70 ++++----
drivers/iio/accel/bmc150-accel-core.c         |  19 ++
drivers/iio/gyro/bmg160_core.c                |  19 ++
drivers/iio/gyro/itg3200_core.c               |  18 ++
drivers/iio/magnetometer/bmc150_magn.c        |  19 ++
drivers/iio/magnetometer/hmc5843.h            |   1 +
drivers/iio/magnetometer/hmc5843_core.c       |  14 ++
include/linux/iio/gyro/itg3200.h              |   1 +
9 files changed, 288 insertions(+), 35 deletions(-)

create mode 100644 Documentation/devicetree/bindings/iio/mount-matrix.txt

Signature · 1 line

2.19.1

Signature · 2 lines

With Best Regards,
Andy Shevchenko