Inconsistent SMO8840 accelerometer data between Windows and Linux

Inconsistent SMO8840 accelerometer data between Windows and Linux

[Daniel Drake]

Hi,

We are working with SMO8840 sensors integrated into several Acer
all-in-one consumer PCs and we would like to make the accelerometer
work with iio-sensor-proxy so that the display is auto-rotated based
on the physical orientation of the device.

One option is to inspect each device and then apply a mount matrix
override in systemd, but we are looking for a generic solution. On
Windows 10 the same functionality "just works" with the standard
drivers from ST.

Looking at the Windows 10 situation, I can see that the ACPI DSDT
includes orientation data for the accelerometer device, in a package
named _ONT. Using an in-memory DSDT override I was able to remove this
package in order to examine the "raw" uncorrected data from Windows.

I could then deduce the meanings of the 6 values in _ONT: the first 3
are axis ordering, so the values "1 0 2" imply that the data arrives
in order of axis Y, X, Z, or in other words X and Y are swapped. The
final 3 values are inversion flags, values "0 0 1" mean that the Z
values must have their sign flipped.

On this product the values are as above, 1 0 2 0 0 1 (swap X and Y,
negate Z). acpidump:
https://gist.github.com/dsd/588762aaa3b5601ff7fc6a8692755877

However, looking to implement the same _ONT logic on Linux, I was met
with the surprise that the values read by st-accel-i2c do not match
the Windows values taken even when _ONT is removed.

This is a "landscape-first" device like the one illustrated here:
https://blogs.windows.com/buildingapps/2015/09/03/new-sensor-features-in-windows-10/

Here is the data. I took Windows measurements using the "Sensors" app
in the app store, and Linux measurements from sysfs in_accel_[xyz]_raw
and manually normalized into the range -1 to +1.

1. Placed flat on the desk, face up "looking up at the sky"
Windows (with _ONT): (0, 0, -1)
Windows (no _ONT): (0, 0, 1)
Linux: (0, 0, -1)

2. Normal landscape orientation (i.e. picture the device positioned
the right way up and stuck to the wall)
Windows (with _ONT): (0, -1, 0)
Windows (no _ONT): (-1, 0, 0)
Linux: (0, 1, 0)

3. Rotated 90 degrees right from normal landscape orientation
Windows (with _ONT): (1, 0, 0)
Windows (no _ONT): (0, 1, 0)
Linux: (1, 0, 0)

4. Rotated 90 degrees left from normal landscape orientation
Windows (with _ONT): (-1, 0, 0)
Windows (no _ONT): (0, -1, 0)
Linux: (-1, 0, 0)

5. Rotated 180 degrees from normal landscape orientation (i.e. upside down)
Windows (with _ONT): (0, 1, 0)
Windows (no _ONT): (1, 0, 0)
Linux: (0, -1, 0)

Why is the data read by Linux inconsistent with the data from Windows
even after I remove the _ONT orientation adjustment?

Thanks
Daniel

Re: Inconsistent SMO8840 accelerometer data between Windows and Linux

[Jonathan Cameron]

On Tue, 27 Nov 2018 14:54:15 +0800
Daniel Drake <drake@endlessm.com> wrote:

> Hi,
> 
> We are working with SMO8840 sensors integrated into several Acer
> all-in-one consumer PCs and we would like to make the accelerometer
> work with iio-sensor-proxy so that the display is auto-rotated based
> on the physical orientation of the device.
> 
> One option is to inspect each device and then apply a mount matrix
> override in systemd, but we are looking for a generic solution. On
> Windows 10 the same functionality "just works" with the standard
> drivers from ST.
> 
> Looking at the Windows 10 situation, I can see that the ACPI DSDT
> includes orientation data for the accelerometer device, in a package
> named _ONT. Using an in-memory DSDT override I was able to remove this
> package in order to examine the "raw" uncorrected data from Windows.
> 
> I could then deduce the meanings of the 6 values in _ONT: the first 3
> are axis ordering, so the values "1 0 2" imply that the data arrives
> in order of axis Y, X, Z, or in other words X and Y are swapped. The
> final 3 values are inversion flags, values "0 0 1" mean that the Z
> values must have their sign flipped.

Ouch, they could at least have done something that inherently maintained
the handedness of the axes rather than doing it as a swap axis and
separate correction for the handedness.

Or even, perhaps actually bothered to look at adding their _ONT
mapping to the ACPI spec.  It would be a stretch for me to justify
that one in the day job or I'd be tempted...  When will people learn
that they shouldn't just make up acpi elements.  There are perfectly
good ways  of adding manufacturer data.. *sigh*

Great digging by the way!  If others with ST sensors could dump their
DSDT and use iASL to decompile it to see if they have an _ONT entry
that would be great!

> 
> On this product the values are as above, 1 0 2 0 0 1 (swap X and Y,
> negate Z). acpidump:
> https://gist.github.com/dsd/588762aaa3b5601ff7fc6a8692755877
> 
> However, looking to implement the same _ONT logic on Linux, I was met
> with the surprise that the values read by st-accel-i2c do not match
> the Windows values taken even when _ONT is removed.
> 
> This is a "landscape-first" device like the one illustrated here:
> https://blogs.windows.com/buildingapps/2015/09/03/new-sensor-features-in-windows-10/
> 
> Here is the data. I took Windows measurements using the "Sensors" app
> in the app store, and Linux measurements from sysfs in_accel_[xyz]_raw
> and manually normalized into the range -1 to +1.
> 
> 1. Placed flat on the desk, face up "looking up at the sky"
> Windows (with _ONT): (0, 0, -1)
> Windows (no _ONT): (0, 0, 1)
> Linux: (0, 0, -1)

So their default is flip the z axis.

> 
> 2. Normal landscape orientation (i.e. picture the device positioned
> the right way up and stuck to the wall)
> Windows (with _ONT): (0, -1, 0)
> Windows (no _ONT): (-1, 0, 0)
> Linux: (0, 1, 0)

Then swap x and y (the flip is handedness correction in conjunction with the
zaxis flip above.

> 
> 3. Rotated 90 degrees right from normal landscape orientation
> Windows (with _ONT): (1, 0, 0)
> Windows (no _ONT): (0, 1, 0)
> Linux: (1, 0, 0)

And finally just swap x and y.

> 
> 4. Rotated 90 degrees left from normal landscape orientation
> Windows (with _ONT): (-1, 0, 0)
> Windows (no _ONT): (0, -1, 0)
> Linux: (-1, 0, 0)
> 
> 5. Rotated 180 degrees from normal landscape orientation (i.e. upside down)
> Windows (with _ONT): (0, 1, 0)
> Windows (no _ONT): (1, 0, 0)
> Linux: (0, -1, 0)
> 
> Why is the data read by Linux inconsistent with the data from Windows
> even after I remove the _ONT orientation adjustment?

Given I'm fairly sure we basically just spit out the data directly
from the sensor, the most likely explanation is that ST hard coded
their driver to apply a transform for the first device they even
mounted it in.  When another came along with it mounted differently
they figured they needed to apply some sort of transform.

So their default is to first apply the following:

1) Swap x and y,
2) Flip z and resulting x (which was y).

If we want to support their ONT we'll need to do this first, then
apply ONT on top of that by forming the relevant mount matrix and
outputting that to userspace.

If you fancy having a go it would be good to have this support!

Bastien / Hans if you could check my logic (as the two people who
most often deal with this sort of horribleness!) that would be great.

Thanks,

Jonathan

> 
> Thanks
> Daniel

Re: Inconsistent SMO8840 accelerometer data between Windows and Linux

[Jonathan Cameron]

On Sat, 1 Dec 2018 16:28:52 +0000
Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote:

> On Tue, 27 Nov 2018 14:54:15 +0800
> Daniel Drake <drake@endlessm.com> wrote:
> 
> > Hi,
> > 
> > We are working with SMO8840 sensors integrated into several Acer
> > all-in-one consumer PCs and we would like to make the accelerometer
> > work with iio-sensor-proxy so that the display is auto-rotated based
> > on the physical orientation of the device.
> > 
> > One option is to inspect each device and then apply a mount matrix
> > override in systemd, but we are looking for a generic solution. On
> > Windows 10 the same functionality "just works" with the standard
> > drivers from ST.
> > 
> > Looking at the Windows 10 situation, I can see that the ACPI DSDT
> > includes orientation data for the accelerometer device, in a package
> > named _ONT. Using an in-memory DSDT override I was able to remove this
> > package in order to examine the "raw" uncorrected data from Windows.
> > 
> > I could then deduce the meanings of the 6 values in _ONT: the first 3
> > are axis ordering, so the values "1 0 2" imply that the data arrives
> > in order of axis Y, X, Z, or in other words X and Y are swapped. The
> > final 3 values are inversion flags, values "0 0 1" mean that the Z
> > values must have their sign flipped.  
> 
> Ouch, they could at least have done something that inherently maintained
> the handedness of the axes rather than doing it as a swap axis and
> separate correction for the handedness.
> 
> Or even, perhaps actually bothered to look at adding their _ONT
> mapping to the ACPI spec.  It would be a stretch for me to justify
> that one in the day job or I'd be tempted...  When will people learn
> that they shouldn't just make up acpi elements.  There are perfectly
> good ways  of adding manufacturer data.. *sigh*
> 
> Great digging by the way!  If others with ST sensors could dump their
> DSDT and use iASL to decompile it to see if they have an _ONT entry
> that would be great!
> 
> > 
> > On this product the values are as above, 1 0 2 0 0 1 (swap X and Y,
> > negate Z). acpidump:
> > https://gist.github.com/dsd/588762aaa3b5601ff7fc6a8692755877
> > 
> > However, looking to implement the same _ONT logic on Linux, I was met
> > with the surprise that the values read by st-accel-i2c do not match
> > the Windows values taken even when _ONT is removed.
> > 
> > This is a "landscape-first" device like the one illustrated here:
> > https://blogs.windows.com/buildingapps/2015/09/03/new-sensor-features-in-windows-10/
> > 
> > Here is the data. I took Windows measurements using the "Sensors" app
> > in the app store, and Linux measurements from sysfs in_accel_[xyz]_raw
> > and manually normalized into the range -1 to +1.
> > 
> > 1. Placed flat on the desk, face up "looking up at the sky"
> > Windows (with _ONT): (0, 0, -1)
> > Windows (no _ONT): (0, 0, 1)
> > Linux: (0, 0, -1)  
> 
> So their default is flip the z axis.
> 
> > 
> > 2. Normal landscape orientation (i.e. picture the device positioned
> > the right way up and stuck to the wall)
> > Windows (with _ONT): (0, -1, 0)
> > Windows (no _ONT): (-1, 0, 0)
> > Linux: (0, 1, 0)  
> 
> Then swap x and y (the flip is handedness correction in conjunction with the
> zaxis flip above.
> 
> > 
> > 3. Rotated 90 degrees right from normal landscape orientation
> > Windows (with _ONT): (1, 0, 0)
> > Windows (no _ONT): (0, 1, 0)
> > Linux: (1, 0, 0)  
> 
> And finally just swap x and y.
> 
> > 
> > 4. Rotated 90 degrees left from normal landscape orientation
> > Windows (with _ONT): (-1, 0, 0)
> > Windows (no _ONT): (0, -1, 0)
> > Linux: (-1, 0, 0)
> > 
> > 5. Rotated 180 degrees from normal landscape orientation (i.e. upside down)
> > Windows (with _ONT): (0, 1, 0)
> > Windows (no _ONT): (1, 0, 0)
> > Linux: (0, -1, 0)
> > 
> > Why is the data read by Linux inconsistent with the data from Windows
> > even after I remove the _ONT orientation adjustment?  
> 
> Given I'm fairly sure we basically just spit out the data directly
> from the sensor, the most likely explanation is that ST hard coded
> their driver to apply a transform for the first device they even
> mounted it in.  When another came along with it mounted differently
> they figured they needed to apply some sort of transform.
> 
> So their default is to first apply the following:
> 
> 1) Swap x and y,
> 2) Flip z and resulting x (which was y).
> 
> If we want to support their ONT we'll need to do this first, then
> apply ONT on top of that by forming the relevant mount matrix and
> outputting that to userspace.
> 
> If you fancy having a go it would be good to have this support!
> 
> Bastien / Hans if you could check my logic (as the two people who
> most often deal with this sort of horribleness!) that would be great.
> 
I should also have asked... Denis, can you check if there is any
documentation of this _ONT anywhere? 

Example dumps in any patch would also be great.  I have a horrible
feeling the above 'default' transform will turn out to not be consistent
across devices... Just a hunch :)

Jonathan
 
> Thanks,
> 
> Jonathan
> 
> > 
> > Thanks
> > Daniel  
> 

Re: Inconsistent SMO8840 accelerometer data between Windows and Linux

[Hans de Goede]

Hi,

On 01-12-18 17:28, Jonathan Cameron wrote:
> On Tue, 27 Nov 2018 14:54:15 +0800
> Daniel Drake <drake@endlessm.com> wrote:
> 
>> Hi,
>>
>> We are working with SMO8840 sensors integrated into several Acer
>> all-in-one consumer PCs and we would like to make the accelerometer
>> work with iio-sensor-proxy so that the display is auto-rotated based
>> on the physical orientation of the device.
>>
>> One option is to inspect each device and then apply a mount matrix
>> override in systemd, but we are looking for a generic solution. On
>> Windows 10 the same functionality "just works" with the standard
>> drivers from ST.
>>
>> Looking at the Windows 10 situation, I can see that the ACPI DSDT
>> includes orientation data for the accelerometer device, in a package
>> named _ONT. Using an in-memory DSDT override I was able to remove this
>> package in order to examine the "raw" uncorrected data from Windows.
>>
>> I could then deduce the meanings of the 6 values in _ONT: the first 3
>> are axis ordering, so the values "1 0 2" imply that the data arrives
>> in order of axis Y, X, Z, or in other words X and Y are swapped. The
>> final 3 values are inversion flags, values "0 0 1" mean that the Z
>> values must have their sign flipped.
> 
> Ouch, they could at least have done something that inherently maintained
> the handedness of the axes rather than doing it as a swap axis and
> separate correction for the handedness.
> 
> Or even, perhaps actually bothered to look at adding their _ONT
> mapping to the ACPI spec.  It would be a stretch for me to justify
> that one in the day job or I'd be tempted...  When will people learn
> that they shouldn't just make up acpi elements.  There are perfectly
> good ways  of adding manufacturer data.. *sigh*
> 
> Great digging by the way!  If others with ST sensors could dump their
> DSDT and use iASL to decompile it to see if they have an _ONT entry
> that would be great!

Great digging indeed, thanks Daniel.

Unfortunately it seems that the _ONT attribute is not universally
useful.

Out of the 75 DSTDs which I have an archive of (most of which are
2-in-1s or just tablets, so they have an accelerometer) only 3
actually have an _ONT method:

Generic 7 inch tablet from "GP-electronic":  using a KIOX000A accelerometer
MSI S100 tablet:                             using a SMO8500 accelerometer
Onda V975w tablet:                           using a SMO8500 accelerometer

But the 3 _ONT methods are all in (disabled _STA returns 0) SMO8821 nodes.

SMO8821 / SMO8840 are newer then the SMO8500, so perhaps that this _ONT
thing is something which we are going to see on more new devices with
ST / SMO sensors.

But I cannot help to confirm that the interpretation from this email thread
is correct.

Also this seems to be a ST / SMO sensor thing only.

Regards,

Hans




>> On this product the values are as above, 1 0 2 0 0 1 (swap X and Y,
>> negate Z). acpidump:
>> https://gist.github.com/dsd/588762aaa3b5601ff7fc6a8692755877
>>
>> However, looking to implement the same _ONT logic on Linux, I was met
>> with the surprise that the values read by st-accel-i2c do not match
>> the Windows values taken even when _ONT is removed.
>>
>> This is a "landscape-first" device like the one illustrated here:
>> https://blogs.windows.com/buildingapps/2015/09/03/new-sensor-features-in-windows-10/
>>
>> Here is the data. I took Windows measurements using the "Sensors" app
>> in the app store, and Linux measurements from sysfs in_accel_[xyz]_raw
>> and manually normalized into the range -1 to +1.
>>
>> 1. Placed flat on the desk, face up "looking up at the sky"
>> Windows (with _ONT): (0, 0, -1)
>> Windows (no _ONT): (0, 0, 1)
>> Linux: (0, 0, -1)
> 
> So their default is flip the z axis.
> 
>>
>> 2. Normal landscape orientation (i.e. picture the device positioned
>> the right way up and stuck to the wall)
>> Windows (with _ONT): (0, -1, 0)
>> Windows (no _ONT): (-1, 0, 0)
>> Linux: (0, 1, 0)
> 
> Then swap x and y (the flip is handedness correction in conjunction with the
> zaxis flip above.
> 
>>
>> 3. Rotated 90 degrees right from normal landscape orientation
>> Windows (with _ONT): (1, 0, 0)
>> Windows (no _ONT): (0, 1, 0)
>> Linux: (1, 0, 0)
> 
> And finally just swap x and y.
> 
>>
>> 4. Rotated 90 degrees left from normal landscape orientation
>> Windows (with _ONT): (-1, 0, 0)
>> Windows (no _ONT): (0, -1, 0)
>> Linux: (-1, 0, 0)
>>
>> 5. Rotated 180 degrees from normal landscape orientation (i.e. upside down)
>> Windows (with _ONT): (0, 1, 0)
>> Windows (no _ONT): (1, 0, 0)
>> Linux: (0, -1, 0)
>>
>> Why is the data read by Linux inconsistent with the data from Windows
>> even after I remove the _ONT orientation adjustment?
> 
> Given I'm fairly sure we basically just spit out the data directly
> from the sensor, the most likely explanation is that ST hard coded
> their driver to apply a transform for the first device they even
> mounted it in.  When another came along with it mounted differently
> they figured they needed to apply some sort of transform.
> 
> So their default is to first apply the following:
> 
> 1) Swap x and y,
> 2) Flip z and resulting x (which was y).
> 
> If we want to support their ONT we'll need to do this first, then
> apply ONT on top of that by forming the relevant mount matrix and
> outputting that to userspace.
> 
> If you fancy having a go it would be good to have this support!
> 
> Bastien / Hans if you could check my logic (as the two people who
> most often deal with this sort of horribleness!) that would be great.
> 
> Thanks,
> 
> Jonathan
> 
>>
>> Thanks
>> Daniel
> 

[PATCH] Re: Inconsistent SMO8840 accelerometer data between Windows and Linux

[Daniel Drake]

Jonathan Cameron wrote:
> If we want to support their ONT we'll need to do this first, then
> apply ONT on top of that by forming the relevant mount matrix and
> outputting that to userspace.
> If you fancy having a go it would be good to have this support!

Cool, I didn't realise that there was a way to export mount matrices.
It looks like iio-sensor-proxy doesn't read that attribute yet, I will
submit a patch for that.

For the kernel changes, here is my initial attempt, comments welcome.
I had to drop the const typing from iio_mount_matrix otherwise it's
difficult to dynamically construct it.

Also I'm not sure whether we should apply this "default_ont" like Windows
does even when _ONT is not present?

I would also love to hear from ST about how this situation happened and
if this is the right way forward.

Regarding other devices with _ONT, the only other device we found
is Asus P4540UQ, a system that we worked on 2 years ago and no longer
have access to. I don't think we tested the accelerometer. The ACPI data
indicates SMO8820, with Y and Z axis values requiring sign flip.

Thanks
Daniel
---
 drivers/iio/accel/st_accel_core.c     | 161 +++++++++++++++++++++++++-
 include/linux/iio/common/st_sensors.h |   1 +
 include/linux/iio/iio.h               |   2 +-
 3 files changed, 162 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/accel/st_accel_core.c b/drivers/iio/accel/st_accel_core.c
index 3e6fd5a8ac5b..3eb4e4b55596 100644
--- a/drivers/iio/accel/st_accel_core.c
+++ b/drivers/iio/accel/st_accel_core.c
@@ -11,6 +11,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/slab.h>
+#include <linux/acpi.h>
 #include <linux/errno.h>
 #include <linux/types.h>
 #include <linux/mutex.h>
@@ -917,12 +918,157 @@ static const struct iio_trigger_ops st_accel_trigger_ops = {
 #define ST_ACCEL_TRIGGER_OPS NULL
 #endif
 
+static const struct iio_mount_matrix *
+get_mount_matrix(const struct iio_dev *indio_dev,
+		 const struct iio_chan_spec *chan)
+{
+	struct st_sensor_data *adata = iio_priv(indio_dev);
+
+	return adata->mount_matrix;
+}
+
+static const struct iio_chan_spec_ext_info mount_matrix_ext_info[] = {
+	IIO_MOUNT_MATRIX(IIO_SHARED_BY_ALL, get_mount_matrix),
+	{ },
+};
+
+/* Read ST-specific _ONT orientation data from ACPI and generate an
+ * appropriate mount matrix.
+ */
+static int apply_acpi_orientation(struct iio_dev *indio_dev,
+				  struct iio_chan_spec *channels)
+{
+	struct st_sensor_data *adata = iio_priv(indio_dev);
+	struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
+	struct acpi_device *adev;
+	union acpi_object *ont;
+	union acpi_object *elements;
+	acpi_status status;
+	int ret = -EINVAL;
+	unsigned int val;
+	int i, j;
+	int final_ont[3][3] = { 0 };
+
+	/* For some reason, ST's _ONT translation does not apply directly
+	 * to the data read from the sensor. Another translation must be
+	 * performed first, as described by the matrix below. Perhaps
+	 * ST required this specific translation for the first product
+	 * where the device was mounted?
+	 */
+	const int default_ont[3][3] = {
+		{  0,  1,  0 },
+		{ -1,  0,  0 },
+		{  0,  0, -1 },
+	};
+
+
+	adev = ACPI_COMPANION(adata->dev);
+	if (!adev)
+		return 0;
+
+	/* Read _ONT data, which should be a package of 6 integers. */
+	status = acpi_evaluate_object(adev->handle, "_ONT", NULL, &buffer);
+	if (ACPI_FAILURE(status)) {
+		ret = status;
+		goto out;
+	}
+
+	ont = buffer.pointer;
+	if (ont->type != ACPI_TYPE_PACKAGE || ont->package.count != 6)
+		goto out;
+
+	/* The first 3 integers provide axis order information.
+	 * e.g. 0 1 2 would indicate normal X,Y,Z ordering.
+	 * e.g. 1 0 2 indicates that data arrives in order Y,X,Z.
+	 */
+	elements = ont->package.elements;
+	for (i = 0; i < 3; i++) {
+		if (elements[i].type != ACPI_TYPE_INTEGER)
+			goto out;
+
+		val = elements[i].integer.value;
+		if (val < 0 || val > 2)
+			goto out;
+
+		/* Avoiding full matrix multiplication, we simply reorder the
+		 * columns in the default_ont matrix according to the
+		 * ordering provided by _ONT.
+		 */
+		final_ont[0][i] = default_ont[0][val];
+		final_ont[1][i] = default_ont[1][val];
+		final_ont[2][i] = default_ont[2][val];
+	}
+
+	/* The final 3 integers provide sign flip information.
+	 * 0 means no change, 1 means flip.
+	 * e.g. 0 0 1 means that Z data should be sign-flipped.
+	 * This is applied after the axis reordering from above.
+	 */
+	elements += 3;
+	for (i = 0; i < 3; i++) {
+		if (elements[i].type != ACPI_TYPE_INTEGER)
+			goto out;
+
+		val = elements[i].integer.value;
+		if (val != 0 && val != 1)
+			goto out;
+		if (!val)
+			continue;
+
+		/* Flip the values in the indicated column */
+		final_ont[0][i] *= -1;
+		final_ont[1][i] *= -1;
+		final_ont[2][i] *= -1;
+	}
+
+	/* Convert our integer matrix to a string-based iio_mount_matrix */
+	adata->mount_matrix = devm_kmalloc(&indio_dev->dev,
+					   sizeof(*adata->mount_matrix),
+					   GFP_KERNEL);
+	if (!adata->mount_matrix)
+		goto out;
+
+	for (i = 0; i < 3; i++) {
+		for (j = 0; j < 3; j++) {
+			int matrix_val = final_ont[i][j];
+			char *str_value;
+
+			switch (matrix_val) {
+			case -1:
+				str_value = "-1";
+				break;
+			case 0:
+				str_value = "0";
+				break;
+			case 1:
+				str_value = "1";
+				break;
+			default:
+				goto out;
+			}
+			adata->mount_matrix->rotation[i * 3 + j] = str_value;
+		}
+	}
+
+	/* Expose the mount matrix via ext_info */
+	for (i = 0; i < indio_dev->num_channels; i++)
+		channels[i].ext_info = mount_matrix_ext_info;
+
+	ret = 0;
+
+out:
+	kfree(buffer.pointer);
+	return ret;
+}
+
 int st_accel_common_probe(struct iio_dev *indio_dev)
 {
 	struct st_sensor_data *adata = iio_priv(indio_dev);
 	struct st_sensors_platform_data *pdata =
 		(struct st_sensors_platform_data *)adata->dev->platform_data;
 	int irq = adata->get_irq_data_ready(indio_dev);
+	struct iio_chan_spec *channels;
+	size_t channels_size;
 	int err;
 
 	indio_dev->modes = INDIO_DIRECT_MODE;
@@ -941,9 +1087,22 @@ int st_accel_common_probe(struct iio_dev *indio_dev)
 
 	adata->num_data_channels = ST_ACCEL_NUMBER_DATA_CHANNELS;
 	adata->multiread_bit = adata->sensor_settings->multi_read_bit;
-	indio_dev->channels = adata->sensor_settings->ch;
 	indio_dev->num_channels = ST_SENSORS_NUMBER_ALL_CHANNELS;
 
+	channels_size = indio_dev->num_channels * sizeof(struct iio_chan_spec);
+	channels = devm_kmemdup(&indio_dev->dev,
+				adata->sensor_settings->ch,
+				channels_size, GFP_KERNEL);
+	if (!channels) {
+		err = -ENOMEM;
+		goto st_accel_power_off;
+	}
+
+	err = apply_acpi_orientation(indio_dev, channels);
+	if (err < 0)
+		goto st_accel_power_off;
+
+	indio_dev->channels = channels;
 	adata->current_fullscale = (struct st_sensor_fullscale_avl *)
 					&adata->sensor_settings->fs.fs_avl[0];
 	adata->odr = adata->sensor_settings->odr.odr_avl[0].hz;
diff --git a/include/linux/iio/common/st_sensors.h b/include/linux/iio/common/st_sensors.h
index f9bd6e8ab138..fe15694128ac 100644
--- a/include/linux/iio/common/st_sensors.h
+++ b/include/linux/iio/common/st_sensors.h
@@ -260,6 +260,7 @@ struct st_sensor_settings {
 struct st_sensor_data {
 	struct device *dev;
 	struct iio_trigger *trig;
+	struct iio_mount_matrix *mount_matrix;
 	struct st_sensor_settings *sensor_settings;
 	struct st_sensor_fullscale_avl *current_fullscale;
 	struct regulator *vdd;
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index a74cb177dc6f..d3094ffeb9da 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -125,7 +125,7 @@ ssize_t iio_enum_write(struct iio_dev *indio_dev,
  *            main hardware
  */
 struct iio_mount_matrix {
-	const char *rotation[9];
+	char *rotation[9];
 };
 
 ssize_t iio_show_mount_matrix(struct iio_dev *indio_dev, uintptr_t priv,
-- 
2.19.1

Re: [PATCH] Re: Inconsistent SMO8840 accelerometer data between Windows and Linux

[Jonathan Cameron]

On Mon,  3 Dec 2018 14:57:07 +0800
Daniel Drake <drake@endlessm.com> wrote:

> Jonathan Cameron wrote:
> > If we want to support their ONT we'll need to do this first, then
> > apply ONT on top of that by forming the relevant mount matrix and
> > outputting that to userspace.
> > If you fancy having a go it would be good to have this support!  
> 
> Cool, I didn't realise that there was a way to export mount matrices.
> It looks like iio-sensor-proxy doesn't read that attribute yet, I will
> submit a patch for that.
> 
> For the kernel changes, here is my initial attempt, comments welcome.
> I had to drop the const typing from iio_mount_matrix otherwise it's
> difficult to dynamically construct it.
> 
> Also I'm not sure whether we should apply this "default_ont" like Windows
> does even when _ONT is not present?
> 
> I would also love to hear from ST about how this situation happened and
> if this is the right way forward.
> 
> Regarding other devices with _ONT, the only other device we found
> is Asus P4540UQ, a system that we worked on 2 years ago and no longer
> have access to. I don't think we tested the accelerometer. The ACPI data
> indicates SMO8820, with Y and Z axis values requiring sign flip.
> 
> Thanks
> Daniel
Code looks good to me.  Hopefully we'll get some info on where this came from
and whether there is any standardization around it at all!

Jonathan
> ---
>  drivers/iio/accel/st_accel_core.c     | 161 +++++++++++++++++++++++++-
>  include/linux/iio/common/st_sensors.h |   1 +
>  include/linux/iio/iio.h               |   2 +-
>  3 files changed, 162 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/accel/st_accel_core.c b/drivers/iio/accel/st_accel_core.c
> index 3e6fd5a8ac5b..3eb4e4b55596 100644
> --- a/drivers/iio/accel/st_accel_core.c
> +++ b/drivers/iio/accel/st_accel_core.c
> @@ -11,6 +11,7 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
> +#include <linux/acpi.h>
>  #include <linux/errno.h>
>  #include <linux/types.h>
>  #include <linux/mutex.h>
> @@ -917,12 +918,157 @@ static const struct iio_trigger_ops st_accel_trigger_ops = {
>  #define ST_ACCEL_TRIGGER_OPS NULL
>  #endif
>  
> +static const struct iio_mount_matrix *
> +get_mount_matrix(const struct iio_dev *indio_dev,
> +		 const struct iio_chan_spec *chan)
> +{
> +	struct st_sensor_data *adata = iio_priv(indio_dev);
> +
> +	return adata->mount_matrix;
> +}
> +
> +static const struct iio_chan_spec_ext_info mount_matrix_ext_info[] = {
> +	IIO_MOUNT_MATRIX(IIO_SHARED_BY_ALL, get_mount_matrix),
> +	{ },
> +};
> +
> +/* Read ST-specific _ONT orientation data from ACPI and generate an
> + * appropriate mount matrix.
> + */
> +static int apply_acpi_orientation(struct iio_dev *indio_dev,
> +				  struct iio_chan_spec *channels)
> +{
> +	struct st_sensor_data *adata = iio_priv(indio_dev);
> +	struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
> +	struct acpi_device *adev;
> +	union acpi_object *ont;
> +	union acpi_object *elements;
> +	acpi_status status;
> +	int ret = -EINVAL;
> +	unsigned int val;
> +	int i, j;
> +	int final_ont[3][3] = { 0 };
> +
> +	/* For some reason, ST's _ONT translation does not apply directly
> +	 * to the data read from the sensor. Another translation must be
> +	 * performed first, as described by the matrix below. Perhaps
> +	 * ST required this specific translation for the first product
> +	 * where the device was mounted?
> +	 */
> +	const int default_ont[3][3] = {
> +		{  0,  1,  0 },
> +		{ -1,  0,  0 },
> +		{  0,  0, -1 },
> +	};
> +
> +
> +	adev = ACPI_COMPANION(adata->dev);
> +	if (!adev)
> +		return 0;
> +
> +	/* Read _ONT data, which should be a package of 6 integers. */
> +	status = acpi_evaluate_object(adev->handle, "_ONT", NULL, &buffer);
> +	if (ACPI_FAILURE(status)) {
> +		ret = status;
> +		goto out;
> +	}
> +
> +	ont = buffer.pointer;
> +	if (ont->type != ACPI_TYPE_PACKAGE || ont->package.count != 6)
> +		goto out;
> +
> +	/* The first 3 integers provide axis order information.
> +	 * e.g. 0 1 2 would indicate normal X,Y,Z ordering.
> +	 * e.g. 1 0 2 indicates that data arrives in order Y,X,Z.
> +	 */
> +	elements = ont->package.elements;
> +	for (i = 0; i < 3; i++) {
> +		if (elements[i].type != ACPI_TYPE_INTEGER)
> +			goto out;
> +
> +		val = elements[i].integer.value;
> +		if (val < 0 || val > 2)
> +			goto out;
> +
> +		/* Avoiding full matrix multiplication, we simply reorder the
> +		 * columns in the default_ont matrix according to the
> +		 * ordering provided by _ONT.
> +		 */
> +		final_ont[0][i] = default_ont[0][val];
> +		final_ont[1][i] = default_ont[1][val];
> +		final_ont[2][i] = default_ont[2][val];
> +	}
> +
> +	/* The final 3 integers provide sign flip information.
> +	 * 0 means no change, 1 means flip.
> +	 * e.g. 0 0 1 means that Z data should be sign-flipped.
> +	 * This is applied after the axis reordering from above.
> +	 */
> +	elements += 3;
> +	for (i = 0; i < 3; i++) {
> +		if (elements[i].type != ACPI_TYPE_INTEGER)
> +			goto out;
> +
> +		val = elements[i].integer.value;
> +		if (val != 0 && val != 1)
> +			goto out;
> +		if (!val)
> +			continue;
> +
> +		/* Flip the values in the indicated column */
> +		final_ont[0][i] *= -1;
> +		final_ont[1][i] *= -1;
> +		final_ont[2][i] *= -1;
> +	}
> +
> +	/* Convert our integer matrix to a string-based iio_mount_matrix */
> +	adata->mount_matrix = devm_kmalloc(&indio_dev->dev,
> +					   sizeof(*adata->mount_matrix),
> +					   GFP_KERNEL);
> +	if (!adata->mount_matrix)
> +		goto out;
> +
> +	for (i = 0; i < 3; i++) {
> +		for (j = 0; j < 3; j++) {
> +			int matrix_val = final_ont[i][j];
> +			char *str_value;
> +
> +			switch (matrix_val) {
> +			case -1:
> +				str_value = "-1";
> +				break;
> +			case 0:
> +				str_value = "0";
> +				break;
> +			case 1:
> +				str_value = "1";
> +				break;
> +			default:
> +				goto out;
> +			}
> +			adata->mount_matrix->rotation[i * 3 + j] = str_value;
> +		}
> +	}
> +
> +	/* Expose the mount matrix via ext_info */
> +	for (i = 0; i < indio_dev->num_channels; i++)
> +		channels[i].ext_info = mount_matrix_ext_info;
> +
> +	ret = 0;
> +
> +out:
> +	kfree(buffer.pointer);
> +	return ret;
> +}
> +
>  int st_accel_common_probe(struct iio_dev *indio_dev)
>  {
>  	struct st_sensor_data *adata = iio_priv(indio_dev);
>  	struct st_sensors_platform_data *pdata =
>  		(struct st_sensors_platform_data *)adata->dev->platform_data;
>  	int irq = adata->get_irq_data_ready(indio_dev);
> +	struct iio_chan_spec *channels;
> +	size_t channels_size;
>  	int err;
>  
>  	indio_dev->modes = INDIO_DIRECT_MODE;
> @@ -941,9 +1087,22 @@ int st_accel_common_probe(struct iio_dev *indio_dev)
>  
>  	adata->num_data_channels = ST_ACCEL_NUMBER_DATA_CHANNELS;
>  	adata->multiread_bit = adata->sensor_settings->multi_read_bit;
> -	indio_dev->channels = adata->sensor_settings->ch;
>  	indio_dev->num_channels = ST_SENSORS_NUMBER_ALL_CHANNELS;
>  
> +	channels_size = indio_dev->num_channels * sizeof(struct iio_chan_spec);
> +	channels = devm_kmemdup(&indio_dev->dev,
> +				adata->sensor_settings->ch,
> +				channels_size, GFP_KERNEL);
> +	if (!channels) {
> +		err = -ENOMEM;
> +		goto st_accel_power_off;
> +	}
> +
> +	err = apply_acpi_orientation(indio_dev, channels);
> +	if (err < 0)
> +		goto st_accel_power_off;
> +
> +	indio_dev->channels = channels;
>  	adata->current_fullscale = (struct st_sensor_fullscale_avl *)
>  					&adata->sensor_settings->fs.fs_avl[0];
>  	adata->odr = adata->sensor_settings->odr.odr_avl[0].hz;
> diff --git a/include/linux/iio/common/st_sensors.h b/include/linux/iio/common/st_sensors.h
> index f9bd6e8ab138..fe15694128ac 100644
> --- a/include/linux/iio/common/st_sensors.h
> +++ b/include/linux/iio/common/st_sensors.h
> @@ -260,6 +260,7 @@ struct st_sensor_settings {
>  struct st_sensor_data {
>  	struct device *dev;
>  	struct iio_trigger *trig;
> +	struct iio_mount_matrix *mount_matrix;
>  	struct st_sensor_settings *sensor_settings;
>  	struct st_sensor_fullscale_avl *current_fullscale;
>  	struct regulator *vdd;
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index a74cb177dc6f..d3094ffeb9da 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -125,7 +125,7 @@ ssize_t iio_enum_write(struct iio_dev *indio_dev,
>   *            main hardware
>   */
>  struct iio_mount_matrix {
> -	const char *rotation[9];
> +	char *rotation[9];
>  };
>  
>  ssize_t iio_show_mount_matrix(struct iio_dev *indio_dev, uintptr_t priv,