[PATCH 0/9] iio mount matrix - revitalize missing bindings documentation and provide code for bmc150, bmg160, bma180, itg3200, hmc584x

[PATCH 0/9] iio mount matrix - revitalize missing bindings documentation and provide code for bmc150, bmg160, bma180, itg3200, hmc584x

[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.

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

-- 
2.19.1

[PATCH 1/9] iio: document bindings for mounting matrices

[H. Nikolaus Schaller]

From: Linus Walleij <linus.walleij@linaro.org>

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 <gregor.boirie@parrot.com>
Cc: Sebastian Reichel <sre@kernel.org>
Cc: Samu Onkalo <samu.onkalo@intel.com>
Cc: devicetree@vger.kernel.org
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 .../devicetree/bindings/iio/mount-matrix.txt  | 108 ++++++++++++++++++
 1 file changed, 108 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/mount-matrix.txt

diff --git a/Documentation/devicetree/bindings/iio/mount-matrix.txt b/Documentation/devicetree/bindings/iio/mount-matrix.txt
new file mode 100644
index 000000000000..a3714727f739
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/mount-matrix.txt
@@ -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";
-- 
2.19.1

[PATCH 2/9] iio: bindings: clarifications for mount-matrix bindings

[H. Nikolaus Schaller]

This adds some clarifications and drawings to make the
orientation of the X/Y/Z axis more clear and better
prescribe how the values of the matrix are intended to
be used.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 .../devicetree/bindings/iio/mount-matrix.txt  | 60 ++++++++++++++++++-
 1 file changed, 57 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/mount-matrix.txt b/Documentation/devicetree/bindings/iio/mount-matrix.txt
index a3714727f739..003279fd735b 100644
--- a/Documentation/devicetree/bindings/iio/mount-matrix.txt
+++ b/Documentation/devicetree/bindings/iio/mount-matrix.txt
@@ -23,6 +23,8 @@ 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.
 
+??? whatabout y-axis orientation - bottop-up or top-down?
+
 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.
@@ -46,6 +48,20 @@ Device-to-world examples for some three-dimensional sensor types:
   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.
 
+
+     (---------)
+     !         !           y: +g
+     !         !             ^
+     !         !             !
+     !         !
+     !         !  x; -g <- z: +g  -> x: +g
+     ! 1  2  3 !
+     ! 4  5  6 !             !
+     ! 7  8  9 !             v
+     ! *  0  # !           y: -g
+     (---------)
+
+
 - 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
@@ -53,6 +69,22 @@ Device-to-world examples for some three-dimensional sensor types:
   perpendicular to the North axis and positive towards the East and (z) is
   perpendicular to the ground plane and positive upwards.
 
+
+     ^^^ North: y > 0
+
+     (---------)
+     !         !
+     !         !
+     !         !
+     !         !  >
+     !         !  > North: x > 0
+     ! 1  2  3 !  >
+     ! 4  5  6 !
+     ! 7  8  9 !
+     ! *  0  # !
+     (---------)
+
+
 - 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
@@ -60,6 +92,20 @@ Device-to-world examples for some three-dimensional sensor types:
   along the (z) axis if rotated clockwise, and a positive value if rotated
   counter-clockwise according to the right-hand rule.
 
+
+     (---------)     y > 0
+     !         !     v---\
+     !         !
+     !         !
+     !         !      <--\
+     !         !         ! z > 0
+     ! 1  2  3 !       --/
+     ! 4  5  6 !
+     ! 7  8  9 !
+     ! *  0  # !
+     (---------)
+
+
 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.
@@ -76,9 +122,15 @@ https://en.wikipedia.org/wiki/Rotation_matrix
 
 The mounting matrix has the layout:
 
- (x0, y0, z0)
- (x1, y1, z1)
- (x2, y2, z3)
+ (mxx, myx, mzx)
+ (mxy, myy, mzy)
+ (mxz, myz, mzz)
+
+Values are intended to be multiplied as:
+
+  x' = mxx * x + myx * y + mzx * z
+  y' = mxy * x + myy * y + mzy * z
+  z' = mxz * x + myz * y + mzz * z
 
 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
@@ -106,3 +158,5 @@ upside-down:
 mount-matrix = "0.998", "0.054", "0",
                "-0.054", "0.998", "0",
                "0", "0", "1";
+
+??? does not match "180 degrees" - factors indicate ca. 3 degrees compensation
-- 
2.19.1

[PATCH 3/9] iio: accel: bmc150: add mount matrix support

[H. Nikolaus Schaller]

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 <hns@goldelico.com>
---
 drivers/iio/accel/bmc150-accel-core.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index 383c802eb5b8..9178846cfddc 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -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);
 }
 
+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;
+}
+
+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;
-- 
2.19.1

[PATCH 4/9] iio: accel: bma180: add mount matrix support

[H. Nikolaus Schaller]

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 <hns@goldelico.com>
---
 drivers/iio/accel/bma180.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/iio/accel/bma180.c b/drivers/iio/accel/bma180.c
index cb9765a3de60..248be67e4f41 100644
--- a/drivers/iio/accel/bma180.c
+++ b/drivers/iio/accel/bma180.c
@@ -116,6 +116,7 @@ struct bma180_data {
 	struct i2c_client *client;
 	struct iio_trigger *trig;
 	const struct bma180_part_info *part_info;
+	struct iio_mount_matrix orientation;
 	struct mutex mutex;
 	bool sleep_state;
 	int scale;
@@ -561,6 +562,13 @@ static int bma180_set_power_mode(struct iio_dev *indio_dev,
 	return ret;
 }
 
+static const struct iio_mount_matrix *
+bma180_accel_get_mount_matrix(const struct iio_dev *indio_dev,
+				const struct iio_chan_spec *chan)
+{
+	return &((struct bma180_data *)iio_priv(indio_dev))->orientation;
+}
+
 static const struct iio_enum bma180_power_mode_enum = {
 	.items = bma180_power_modes,
 	.num_items = ARRAY_SIZE(bma180_power_modes),
@@ -571,6 +579,7 @@ static const struct iio_enum bma180_power_mode_enum = {
 static const struct iio_chan_spec_ext_info bma180_ext_info[] = {
 	IIO_ENUM("power_mode", true, &bma180_power_mode_enum),
 	IIO_ENUM_AVAILABLE("power_mode", &bma180_power_mode_enum),
+	IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, bma180_accel_get_mount_matrix),
 	{ },
 };
 
@@ -722,6 +731,11 @@ static int bma180_probe(struct i2c_client *client,
 		chip = id->driver_data;
 	data->part_info = &bma180_part_info[chip];
 
+	ret = of_iio_read_mount_matrix(&client->dev, "mount-matrix",
+				&data->orientation);
+	if (ret)
+		return ret;
+
 	ret = data->part_info->chip_config(data);
 	if (ret < 0)
 		goto err_chip_disable;
-- 
2.19.1

[PATCH 5/9] iio: accel: bma180: convert to devm

[H. Nikolaus Schaller]

This simplifies the code a little.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/iio/accel/bma180.c | 56 ++++++++++++++------------------------
 1 file changed, 21 insertions(+), 35 deletions(-)

diff --git a/drivers/iio/accel/bma180.c b/drivers/iio/accel/bma180.c
index 248be67e4f41..3f5ee2d495d3 100644
--- a/drivers/iio/accel/bma180.c
+++ b/drivers/iio/accel/bma180.c
@@ -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;
@@ -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);
-	}
-
 	mutex_lock(&data->mutex);
 	data->part_info->chip_disable(data);
 	mutex_unlock(&data->mutex);
-- 
2.19.1

[PATCH 6/9] iio: gyro: bmg160: add mount matrix support

[H. Nikolaus Schaller]

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 <hns@goldelico.com>
---
 drivers/iio/gyro/bmg160_core.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/iio/gyro/bmg160_core.c b/drivers/iio/gyro/bmg160_core.c
index 63ca31628a93..7ae59be9a5ef 100644
--- a/drivers/iio/gyro/bmg160_core.c
+++ b/drivers/iio/gyro/bmg160_core.c
@@ -102,6 +102,7 @@ struct bmg160_data {
 	struct regmap *regmap;
 	struct iio_trigger *dready_trig;
 	struct iio_trigger *motion_trig;
+	struct iio_mount_matrix orientation;
 	struct mutex mutex;
 	s16 buffer[8];
 	u32 dps_range;
@@ -794,6 +795,18 @@ static int bmg160_write_event_config(struct iio_dev *indio_dev,
 	return 0;
 }
 
+static const struct iio_mount_matrix *
+bmg160_get_mount_matrix(const struct iio_dev *indio_dev,
+			 const struct iio_chan_spec *chan)
+{
+	return &((struct bmg160_data *)iio_priv(indio_dev))->orientation;
+}
+
+static const struct iio_chan_spec_ext_info bmg160_ext_info[] = {
+	IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, bmg160_get_mount_matrix),
+	{ },
+};
+
 static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("100 200 400 1000 2000");
 
 static IIO_CONST_ATTR(in_anglvel_scale_available,
@@ -831,6 +844,7 @@ static const struct iio_event_spec bmg160_event = {
 		.storagebits = 16,					\
 		.endianness = IIO_LE,					\
 	},								\
+	.ext_info = bmg160_ext_info,					\
 	.event_spec = &bmg160_event,					\
 	.num_event_specs = 1						\
 }
@@ -1075,6 +1089,11 @@ int bmg160_core_probe(struct device *dev, struct regmap *regmap, int irq,
 	data->irq = irq;
 	data->regmap = regmap;
 
+	ret = of_iio_read_mount_matrix(dev, "mount-matrix",
+				&data->orientation);
+	if (ret)
+		return ret;
+
 	ret = bmg160_chip_init(data);
 	if (ret < 0)
 		return ret;
-- 
2.19.1

[PATCH 7/9] iio: gyro: itg3200: add mount matrix support

[H. Nikolaus Schaller]

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 <hns@goldelico.com>
---
 drivers/iio/gyro/itg3200_core.c  | 18 ++++++++++++++++++
 include/linux/iio/gyro/itg3200.h |  1 +
 2 files changed, 19 insertions(+)

diff --git a/drivers/iio/gyro/itg3200_core.c b/drivers/iio/gyro/itg3200_core.c
index 7adecb562c81..b4bf721d309f 100644
--- a/drivers/iio/gyro/itg3200_core.c
+++ b/drivers/iio/gyro/itg3200_core.c
@@ -242,6 +242,18 @@ static int itg3200_initial_setup(struct iio_dev *indio_dev)
 	return ret;
 }
 
+static const struct iio_mount_matrix *
+itg3200_get_mount_matrix(const struct iio_dev *indio_dev,
+			  const struct iio_chan_spec *chan)
+{
+	return &((struct itg3200 *)iio_priv(indio_dev))->orientation;
+}
+
+static const struct iio_chan_spec_ext_info itg3200_ext_info[] = {
+	IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, itg3200_get_mount_matrix),
+	{ },
+};
+
 #define ITG3200_ST						\
 	{ .sign = 's', .realbits = 16, .storagebits = 16, .endianness = IIO_BE }
 
@@ -255,6 +267,7 @@ static int itg3200_initial_setup(struct iio_dev *indio_dev)
 	.address = ITG3200_REG_GYRO_ ## _mod ## OUT_H, \
 	.scan_index = ITG3200_SCAN_GYRO_ ## _mod, \
 	.scan_type = ITG3200_ST, \
+	.ext_info = itg3200_ext_info, \
 }
 
 static const struct iio_chan_spec itg3200_channels[] = {
@@ -297,6 +310,11 @@ static int itg3200_probe(struct i2c_client *client,
 
 	st = iio_priv(indio_dev);
 
+	ret = of_iio_read_mount_matrix(&client->dev, "mount-matrix",
+				&st->orientation);
+	if (ret)
+		return ret;
+
 	i2c_set_clientdata(client, indio_dev);
 	st->i2c = client;
 
diff --git a/include/linux/iio/gyro/itg3200.h b/include/linux/iio/gyro/itg3200.h
index 2a820850f284..0a30fddccfb3 100644
--- a/include/linux/iio/gyro/itg3200.h
+++ b/include/linux/iio/gyro/itg3200.h
@@ -104,6 +104,7 @@
 struct itg3200 {
 	struct i2c_client	*i2c;
 	struct iio_trigger	*trig;
+	struct iio_mount_matrix orientation;
 };
 
 enum ITG3200_SCAN_INDEX {
-- 
2.19.1

[PATCH 8/9] iio: magnetometer: bmc150: add mount matrix support

[H. Nikolaus Schaller]

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 <hns@goldelico.com>
---
 drivers/iio/magnetometer/bmc150_magn.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/iio/magnetometer/bmc150_magn.c b/drivers/iio/magnetometer/bmc150_magn.c
index d91cb845e3d6..48e3a16432b4 100644
--- a/drivers/iio/magnetometer/bmc150_magn.c
+++ b/drivers/iio/magnetometer/bmc150_magn.c
@@ -143,6 +143,7 @@ struct bmc150_magn_data {
 	 */
 	struct mutex mutex;
 	struct regmap *regmap;
+	struct iio_mount_matrix orientation;
 	/* 4 x 32 bits for x, y z, 4 bytes align, 64 bits timestamp */
 	s32 buffer[6];
 	struct iio_trigger *dready_trig;
@@ -612,6 +613,18 @@ static ssize_t bmc150_magn_show_samp_freq_avail(struct device *dev,
 	return len;
 }
 
+static const struct iio_mount_matrix *
+bmc150_magn_get_mount_matrix(const struct iio_dev *indio_dev,
+			      const struct iio_chan_spec *chan)
+{
+	return &((struct bmc150_magn_data *)iio_priv(indio_dev))->orientation;
+}
+
+static const struct iio_chan_spec_ext_info bmc150_magn_ext_info[] = {
+	IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, bmc150_magn_get_mount_matrix),
+	{ },
+};
+
 static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(bmc150_magn_show_samp_freq_avail);
 
 static struct attribute *bmc150_magn_attributes[] = {
@@ -638,6 +651,7 @@ static const struct attribute_group bmc150_magn_attrs_group = {
 		.storagebits = 32,					\
 		.endianness = IIO_LE					\
 	},								\
+	.ext_info = bmc150_magn_ext_info,				\
 }
 
 static const struct iio_chan_spec bmc150_magn_channels[] = {
@@ -861,6 +875,11 @@ int bmc150_magn_probe(struct device *dev, struct regmap *regmap,
 	data->irq = irq;
 	data->dev = dev;
 
+	ret = of_iio_read_mount_matrix(dev, "mount-matrix",
+				&data->orientation);
+	if (ret)
+		return ret;
+
 	if (!name && ACPI_HANDLE(dev))
 		name = bmc150_magn_match_acpi_device(dev);
 
-- 
2.19.1

[PATCH 9/9] iio: magnetometer: hmc5843: add mount matrix support

[H. Nikolaus Schaller]

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 <hns@goldelico.com>
---
 drivers/iio/magnetometer/hmc5843.h      |  1 +
 drivers/iio/magnetometer/hmc5843_core.c | 14 ++++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/drivers/iio/magnetometer/hmc5843.h b/drivers/iio/magnetometer/hmc5843.h
index a75224cf99df..e3e22d2508d3 100644
--- a/drivers/iio/magnetometer/hmc5843.h
+++ b/drivers/iio/magnetometer/hmc5843.h
@@ -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];
 };
 
diff --git a/drivers/iio/magnetometer/hmc5843_core.c b/drivers/iio/magnetometer/hmc5843_core.c
index ada142fb7aa3..e6b6da70c152 100644
--- a/drivers/iio/magnetometer/hmc5843_core.c
+++ b/drivers/iio/magnetometer/hmc5843_core.c
@@ -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;
-- 
2.19.1

Re: [PATCH 3/9] iio: accel: bmc150: add mount matrix support

[Andy Shevchenko]

On Wed, Feb 20, 2019 at 03:00:50PM +0100, H. Nikolaus Schaller wrote:
> This patch allows to read a mount-matrix device tree
> property and report to user-space or in-kernel iio
> clients.

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

?

> +}
> +
> +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.

> +};

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/9] iio: accel: bma180: convert to devm

[Andy Shevchenko]

On Wed, Feb 20, 2019 at 03:00:52PM +0100, H. Nikolaus Schaller wrote:
> This simplifies the code a little.

> -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:

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

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/9] iio: document bindings for mounting matrices

[Jonathan Cameron]

On Wed, 20 Feb 2019 15:00:48 +0100
"H. Nikolaus Schaller" <hns@goldelico.com> wrote:

> From: Linus Walleij <linus.walleij@linaro.org>
> 
> 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 <gregor.boirie@parrot.com>
> Cc: Sebastian Reichel <sre@kernel.org>
> Cc: Samu Onkalo <samu.onkalo@intel.com>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
Hmm. I looked back and seems there were still some outstanding questions
on this last time around.

https://lore.kernel.org/linux-iio/a6d866f2-ee20-282b-def0-f65de2177aee@kernel.org/

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

> ---
>  .../devicetree/bindings/iio/mount-matrix.txt  | 108 ++++++++++++++++++
>  1 file changed, 108 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/mount-matrix.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/mount-matrix.txt b/Documentation/devicetree/bindings/iio/mount-matrix.txt
> new file mode 100644
> index 000000000000..a3714727f739
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/mount-matrix.txt
> @@ -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";

Re: [PATCH 0/9] iio mount matrix - revitalize missing bindings documentation and provide code for bmc150, bmg160, bma180, itg3200, hmc584x

[Andy Shevchenko]

On Wed, Feb 20, 2019 at 03:00:47PM +0100, H. Nikolaus Schaller wrote:
> 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.

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.

> 
> 
> 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
> 
> -- 
> 2.19.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/9] iio: accel: bmc150: add mount matrix support

[H. Nikolaus Schaller]

> Am 20.02.2019 um 17:07 schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> 
> On Wed, Feb 20, 2019 at 03:00:50PM +0100, H. Nikolaus Schaller wrote:
>> This patch allows to read a mount-matrix device tree
>> property and report to user-space or in-kernel iio
>> clients.
> 
>> +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;
> 
> ?

I think I did copy it verbatim from some other iio driver:

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/iio/magnetometer/ak8975.c?h=next-20190220#n745

and checkpatch did not complain.

But yes, it can and should be improved since it seems that
I picked the only bad example as template...

> 
>> +}
>> +
>> +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.

Ok.

> 
>> +};
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH 3/9] iio: accel: bmc150: add mount matrix support

[Jonathan Cameron]

On Wed, 20 Feb 2019 15:00:50 +0100
"H. Nikolaus Schaller" <hns@goldelico.com> wrote:

> 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 <hns@goldelico.com>
This will clash with Andy's current patch, but I'll fix that up if
needed. Otherwise, one trivial suggestion inline.

Jonathan

> ---
>  drivers/iio/accel/bmc150-accel-core.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
> index 383c802eb5b8..9178846cfddc 100644
> --- a/drivers/iio/accel/bmc150-accel-core.c
> +++ b/drivers/iio/accel/bmc150-accel-core.c
> @@ -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);
>  }
>  
> +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!

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

Re: [PATCH 5/9] iio: accel: bma180: convert to devm

[H. Nikolaus Schaller]

> Am 20.02.2019 um 17:09 schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> 
> On Wed, Feb 20, 2019 at 03:00:52PM +0100, H. Nikolaus Schaller wrote:
>> This simplifies the code a little.
> 
>> -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:
> 
> Please, leave the original label name as it's more self-explanatory.

Ok.

> 
>> 	data->part_info->chip_disable(data);
>> 
>> 	return ret;
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH 5/9] iio: accel: bma180: convert to devm

[Jonathan Cameron]

On Wed, 20 Feb 2019 15:00:52 +0100
"H. Nikolaus Schaller" <hns@goldelico.com> wrote:

> 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
> 
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  drivers/iio/accel/bma180.c | 56 ++++++++++++++------------------------
>  1 file changed, 21 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/iio/accel/bma180.c b/drivers/iio/accel/bma180.c
> index 248be67e4f41..3f5ee2d495d3 100644
> --- a/drivers/iio/accel/bma180.c
> +++ b/drivers/iio/accel/bma180.c
> @@ -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;
> @@ -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.

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

Re: [PATCH 1/9] iio: document bindings for mounting matrices

[H. Nikolaus Schaller]

> Am 20.02.2019 um 17:10 schrieb Jonathan Cameron <jic23@kernel.org>:
> 
> On Wed, 20 Feb 2019 15:00:48 +0100
> "H. Nikolaus Schaller" <hns@goldelico.com> wrote:
> 
>> From: Linus Walleij <linus.walleij@linaro.org>
>> 
>> 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 <gregor.boirie@parrot.com>
>> Cc: Sebastian Reichel <sre@kernel.org>
>> Cc: Samu Onkalo <samu.onkalo@intel.com>
>> Cc: devicetree@vger.kernel.org
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> Hmm. I looked back and seems there were still some outstanding questions
> on this last time around.
> 
> https://lore.kernel.org/linux-iio/a6d866f2-ee20-282b-def0-f65de2177aee@kernel.org/
> 
> 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.

What I am most interested at the moment is to get an unambigous
description for accelerometers first. The otehr topic was that I
did not clearly see how the matrix is applied to the raw values
(one could do row index first or column index first).

BR and thanks,
Nikolaus

> 
> Jonathan
> 
>> ---
>> .../devicetree/bindings/iio/mount-matrix.txt  | 108 ++++++++++++++++++
>> 1 file changed, 108 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/iio/mount-matrix.txt
>> 
>> diff --git a/Documentation/devicetree/bindings/iio/mount-matrix.txt b/Documentation/devicetree/bindings/iio/mount-matrix.txt
>> new file mode 100644
>> index 000000000000..a3714727f739
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/mount-matrix.txt
>> @@ -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";
> 

Re: [PATCH 9/9] iio: magnetometer: hmc5843: add mount matrix support

[Jonathan Cameron]

On Wed, 20 Feb 2019 15:00:56 +0100
"H. Nikolaus Schaller" <hns@goldelico.com> wrote:

> 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 <hns@goldelico.com>
The rest of these are all fine, though I suggest considering
a local variable rather than the fiddly bit of casting.

Jonathan
> ---
>  drivers/iio/magnetometer/hmc5843.h      |  1 +
>  drivers/iio/magnetometer/hmc5843_core.c | 14 ++++++++++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/iio/magnetometer/hmc5843.h b/drivers/iio/magnetometer/hmc5843.h
> index a75224cf99df..e3e22d2508d3 100644
> --- a/drivers/iio/magnetometer/hmc5843.h
> +++ b/drivers/iio/magnetometer/hmc5843.h
> @@ -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];
>  };
>  
> diff --git a/drivers/iio/magnetometer/hmc5843_core.c b/drivers/iio/magnetometer/hmc5843_core.c
> index ada142fb7aa3..e6b6da70c152 100644
> --- a/drivers/iio/magnetometer/hmc5843_core.c
> +++ b/drivers/iio/magnetometer/hmc5843_core.c
> @@ -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;

Re: [PATCH 3/9] iio: accel: bmc150: add mount matrix support

[H. Nikolaus Schaller]

> Am 20.02.2019 um 17:14 schrieb Jonathan Cameron <jic23@kernel.org>:
> 
> On Wed, 20 Feb 2019 15:00:50 +0100
> "H. Nikolaus Schaller" <hns@goldelico.com> wrote:
> 
>> 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 <hns@goldelico.com>
> This will clash with Andy's current patch, but I'll fix that up if
> needed. Otherwise, one trivial suggestion inline.

Ok, thanks!

> 
> Jonathan
> 
>> ---
>> drivers/iio/accel/bmc150-accel-core.c | 19 +++++++++++++++++++
>> 1 file changed, 19 insertions(+)
>> 
>> diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
>> index 383c802eb5b8..9178846cfddc 100644
>> --- a/drivers/iio/accel/bmc150-accel-core.c
>> +++ b/drivers/iio/accel/bmc150-accel-core.c
>> @@ -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);
>> }
>> 
>> +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

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

BR and thanks,
Nikolaus

Re: [PATCH 5/9] iio: accel: bma180: convert to devm

[H. Nikolaus Schaller]

Hi,

> Am 20.02.2019 um 17:18 schrieb Jonathan Cameron <jic23@kernel.org>:
> 
> On Wed, 20 Feb 2019 15:00:52 +0100
> "H. Nikolaus Schaller" <hns@goldelico.com> wrote:
> 
>> 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.

Ok, I see. I just was working on that driver and thought that introducing
devm is a good idea here.

But since it does not improve any function (contrary to the mount-matrix
patches), let's drop it for the moment.

BR and thanks,
Nikolaus

> 
> Jonathan
>> 
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> drivers/iio/accel/bma180.c | 56 ++++++++++++++------------------------
>> 1 file changed, 21 insertions(+), 35 deletions(-)
>> 
>> diff --git a/drivers/iio/accel/bma180.c b/drivers/iio/accel/bma180.c
>> index 248be67e4f41..3f5ee2d495d3 100644
>> --- a/drivers/iio/accel/bma180.c
>> +++ b/drivers/iio/accel/bma180.c
>> @@ -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;
>> @@ -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.
> 
>> 	mutex_lock(&data->mutex);
>> 	data->part_info->chip_disable(data);
>> 	mutex_unlock(&data->mutex);
> 

Re: [PATCH 9/9] iio: magnetometer: hmc5843: add mount matrix support

[H. Nikolaus Schaller]

> Am 20.02.2019 um 17:19 schrieb Jonathan Cameron <jic23@kernel.org>:
> 
> On Wed, 20 Feb 2019 15:00:56 +0100
> "H. Nikolaus Schaller" <hns@goldelico.com> wrote:
> 
>> 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 <hns@goldelico.com>
> The rest of these are all fine, though I suggest considering
> a local variable rather than the fiddly bit of casting.

Ok, fine.

I will wait some days for further comments than then
submit a v2 with all known improvements.

BR and thanks,
Nikolaus

> 
> Jonathan
>> ---
>> drivers/iio/magnetometer/hmc5843.h      |  1 +
>> drivers/iio/magnetometer/hmc5843_core.c | 14 ++++++++++++++
>> 2 files changed, 15 insertions(+)
>> 
>> diff --git a/drivers/iio/magnetometer/hmc5843.h b/drivers/iio/magnetometer/hmc5843.h
>> index a75224cf99df..e3e22d2508d3 100644
>> --- a/drivers/iio/magnetometer/hmc5843.h
>> +++ b/drivers/iio/magnetometer/hmc5843.h
>> @@ -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];
>> };
>> 
>> diff --git a/drivers/iio/magnetometer/hmc5843_core.c b/drivers/iio/magnetometer/hmc5843_core.c
>> index ada142fb7aa3..e6b6da70c152 100644
>> --- a/drivers/iio/magnetometer/hmc5843_core.c
>> +++ b/drivers/iio/magnetometer/hmc5843_core.c
>> @@ -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;
> 

Re: [PATCH 0/9] iio mount matrix - revitalize missing bindings documentation and provide code for bmc150, bmg160, bma180, itg3200, hmc584x

[H. Nikolaus Schaller]

Hi Andy,

> Am 20.02.2019 um 17:13 schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> 
> On Wed, Feb 20, 2019 at 03:00:47PM +0100, H. Nikolaus Schaller wrote:
>> 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.
> 
> 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.

Ok, that looks like a good proposal which avoids compile issues right
from beginning of v2.

BR and thanks,
Nikolaus

> 
>> 
>> 
>> 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
>> 
>> -- 
>> 2.19.1
>> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH 3/9] iio: accel: bmc150: add mount matrix support

[Jonathan Cameron]

On Wed, 20 Feb 2019 17:20:29 +0100
"H. Nikolaus Schaller" <hns@goldelico.com> wrote:

> > Am 20.02.2019 um 17:14 schrieb Jonathan Cameron <jic23@kernel.org>:
> > 
> > On Wed, 20 Feb 2019 15:00:50 +0100
> > "H. Nikolaus Schaller" <hns@goldelico.com> wrote:
> >   
> >> 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 <hns@goldelico.com>  
> > This will clash with Andy's current patch, but I'll fix that up if
> > needed. Otherwise, one trivial suggestion inline.  
> 
> Ok, thanks!
> 
> > 
> > Jonathan
> >   
> >> ---
> >> drivers/iio/accel/bmc150-accel-core.c | 19 +++++++++++++++++++
> >> 1 file changed, 19 insertions(+)
> >> 
> >> diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
> >> index 383c802eb5b8..9178846cfddc 100644
> >> --- a/drivers/iio/accel/bmc150-accel-core.c
> >> +++ b/drivers/iio/accel/bmc150-accel-core.c
> >> @@ -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);
> >> }
> >> 
> >> +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

If you like, feel free to send patches to tidy those up as well!
Then no one else can be unlucky on the same thing in future.

Jonathan

> 
> >   
> >> +}
> >> +
> >> +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;  
> >   
> 
> BR and thanks,
> Nikolaus
> 

Re: [PATCH 1/9] iio: document bindings for mounting matrices

[Linus Walleij]

On Wed, Feb 20, 2019 at 5:18 PM H. Nikolaus Schaller <hns@goldelico.com> wrote:
> > Am 20.02.2019 um 17:10 schrieb Jonathan Cameron <jic23@kernel.org>:
> > On Wed, 20 Feb 2019 15:00:48 +0100
> > "H. Nikolaus Schaller" <hns@goldelico.com> wrote:

> > Hmm. I looked back and seems there were still some outstanding questions
> > on this last time around.
> >
> > https://lore.kernel.org/linux-iio/a6d866f2-ee20-282b-def0-f65de2177aee@kernel.org/
> >
> > 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.

I agree. I guess I ran out of steam somewhere when I had to
look around for my linear algebra textbooks and course notes
and I could not find them :D

> 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.

Oh I trust you 100% to do the right thing with this, just edit it and
sign it off a second time. Whatever becomes the author is not
something I care much about either, whatever makes sense
to you.

Yours,
Linus Walleij