Discussion:
[systemd-devel] accelerometer mount-matrix quirks
Bastien Nocera
2016-11-22 11:13:16 UTC
Permalink
Hey,

I'm adding support for reading the mount-matrix[1] from accelerometer
devices in iio-sensor-proxy, but we'll need to add a way to override
the mount-matrix in case the data from the device-tree is incorrect, or
missing[2].

I was wondering whether we should ship the hwdb quirks in systemd or
iio-sensor-proxy.

Opinions?

Cheers

[1]: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=dfc57732ad38f93ae6232a3b4e64fd077383a0f1
[2]: ACPI exports it as well, but it's not available from the kernel:
http://marc.info/?l=linux-iio&m=147981183211362&w=2
Lennart Poettering
2016-11-22 16:23:39 UTC
Permalink
Post by Bastien Nocera
Hey,
I'm adding support for reading the mount-matrix[1] from accelerometer
devices in iio-sensor-proxy, but we'll need to add a way to override
the mount-matrix in case the data from the device-tree is incorrect, or
missing[2].
I was wondering whether we should ship the hwdb quirks in systemd or
iio-sensor-proxy.
Opinions?
If it's generic enough then the hwdb is definitely a good place for
things like that. If the concept however only has value for
iio-sensor-proxy specifically then it should probably live in that
package. Of course the lines are blurry there, so compare with the
input case: hwdb carries dpi quirk data for all kinds of mice, and if
this data is like that, then it fits in.

Lennart
--
Lennart Poettering, Red Hat
Bastien Nocera
2016-11-22 16:26:57 UTC
Permalink
Post by Lennart Poettering
Post by Bastien Nocera
Hey,
I'm adding support for reading the mount-matrix[1] from
accelerometer
devices in iio-sensor-proxy, but we'll need to add a way to
override
the mount-matrix in case the data from the device-tree is
incorrect, or
missing[2].
I was wondering whether we should ship the hwdb quirks in systemd or
iio-sensor-proxy.
Opinions?
If it's generic enough then the hwdb is definitely a good place for
things like that. If the concept however only has value for
iio-sensor-proxy specifically then it should probably live in that
package. Of course the lines are blurry there, so compare with the
input case: hwdb carries dpi quirk data for all kinds of mice, and if
this data is like that, then it fits in.
The quirks apply to the devices, and are in the same format as what's
offered by the kernel when the firmware provides it, so it's not iio-
sensor-proxy specific in any way. If we were to replace iio-sensor-
proxy, we'd still use this metadata.

I'll send a patch in as soon as I've done testing on a machine that
does need this quirking.

Cheers
Thomas H.P. Andersen
2016-11-22 22:43:16 UTC
Permalink
Post by Bastien Nocera
Post by Lennart Poettering
Post by Bastien Nocera
Hey,
I'm adding support for reading the mount-matrix[1] from
accelerometer
devices in iio-sensor-proxy, but we'll need to add a way to override
the mount-matrix in case the data from the device-tree is
incorrect, or
missing[2].
I was wondering whether we should ship the hwdb quirks in systemd or
iio-sensor-proxy.
Opinions?
If it's generic enough then the hwdb is definitely a good place for
things like that. If the concept however only has value for
iio-sensor-proxy specifically then it should probably live in that
package. Of course the lines are blurry there, so compare with the
input case: hwdb carries dpi quirk data for all kinds of mice, and if
this data is like that, then it fits in.
The quirks apply to the devices, and are in the same format as what's
offered by the kernel when the firmware provides it, so it's not iio-
sensor-proxy specific in any way. If we were to replace iio-sensor-
proxy, we'd still use this metadata.
I'll send a patch in as soon as I've done testing on a machine that
does need this quirking.
Hi Bastien,

I recently opened a PR to iio-sensor-proxy with a quirk for my laptop.
Since this is my main computer testing is easy. Let me know if I can help.
Bastien Nocera
2016-11-23 11:59:40 UTC
Permalink
Post by Thomas H.P. Andersen
Post by Bastien Nocera
Post by Lennart Poettering
Post by Bastien Nocera
Hey,
I'm adding support for reading the mount-matrix[1] from
accelerometer
devices in iio-sensor-proxy, but we'll need to add a way to override
the mount-matrix in case the data from the device-tree is incorrect, or
missing[2].
I was wondering whether we should ship the hwdb quirks in
systemd
Post by Lennart Poettering
Post by Bastien Nocera
or
iio-sensor-proxy.
Opinions?
If it's generic enough then the hwdb is definitely a good place
for
Post by Lennart Poettering
things like that. If the concept however only has value for
iio-sensor-proxy specifically then it should probably live in
that
Post by Lennart Poettering
package. Of course the lines are blurry there, so compare with
the
Post by Lennart Poettering
input case: hwdb carries dpi quirk data for all kinds of mice,
and if
Post by Lennart Poettering
this data is like that, then it fits in.
The quirks apply to the devices, and are in the same format as what's
offered by the kernel when the firmware provides it, so it's not iio-
sensor-proxy specific in any way. If we were to replace iio-sensor-
proxy, we'd still use this metadata.
I'll send a patch in as soon as I've done testing on a machine that
does need this quirking.
Hi Bastien,
I recently opened a PR to iio-sensor-proxy with a quirk for my
laptop.
I know, and that's what restarted the discussion I had a couple of
months back with Hans about applying the mount-matrix ;)

He also has a machine on which he could test it.
Post by Thomas H.P. Andersen
Since this is my main computer testing is easy. Let me know if I can help.
If you want to give a try to sending a patch with a new "70-
accelerometer.hwdb" file, something along the lines of:

# Documentation[1]

iio:b0003v1111p0000*
  ACCEL_MOUNT_MATRIX="1, 0, 0; 0, 1, 0; 0, 0, 1"

Which matches your device, and which you can retrieve in
iio_buffer_accel_open() in src/drv-iio-buffer-accel.c

Would save me quite some time, as I currently can't fully test the
patch.

Would be great if you could also attach the output of the DSDT for that
machine, to see whether that mount-matrix is already in there in some
form, and we're just not parsing it. We'd need to make a note if in the
hwdb that we should remove that quirk when the DSDT is parsed kernel-
side.

Cheers

[1]: This is what I have so far:

Accelerometer orientation
-------------------------

When the accelerometer is not mounted the same way as the screen, we need
to modify the readings from the accelerometer to make sure that the computed
orientation matches the screen one.

For this, we use a 3x3 matrix, as defined in the commit adding the [mount-matrix property](https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=dfc57732ad38f93ae6232a3b4e64fd077383a0f1)
to IIO devices. This information is usually exported in:
- the device-tree for ARM devices
- the ACPI DSDT for Intel-compatible devices

The latter is currently [not parsed](http://marc.info/?l=linux-iio&m=147981183211362&w=2) as of kernel 4.9-rc5.
If there is no mount-matrix defined in the device-tree or ACPI DSDT, or
the data is incorrect, it can be overridden with a udev property, in the 
`70-accelerometer.hwdb` file, for example:
```
iio:FIXME
  ACCEL_MOUNT_MATRIX="1, 0, 0; 0, 1, 0; 0, 0, 1"
```

Note that some devices have the framebuffer rotation that changes between
the bootloader and the main system, which might mean that the accelerometer
is then wrongly oriented. This is a missing feature in the [i915 kernel driver](https://bugs.freedesktop.org/show_bug.cgi?id=94894)
which needs to be fixed, and won't require quirks.
Bastien Nocera
2016-11-23 12:26:25 UTC
Permalink
On Wed, 2016-11-23 at 12:59 +0100, Bastien Nocera wrote:
<snip>
Post by Bastien Nocera
If you want to give a try to sending a patch with a new "70-
Make that "70-sensors.hwdb" as I think we might add a few more things
related to sensors in the future.
Lennart Poettering
2016-11-23 23:26:19 UTC
Permalink
Post by Bastien Nocera
The latter is currently [not parsed](http://marc.info/?l=linux-iio&m=147981183211362&w=2) as of kernel 4.9-rc5.
If there is no mount-matrix defined in the device-tree or ACPI DSDT, or
the data is incorrect, it can be overridden with a udev property, in the 
```
iio:FIXME
  ACCEL_MOUNT_MATRIX="1, 0, 0; 0, 1, 0; 0, 0, 1"
Not that it matters at all, but so far files such as 70-mouse.hwdb
mostly used spaces separators for multiple fields, and I'd suggest
that here too, just to keep this similar in style...

But then again, this doesn't matter at all, it's entirely up to you,
I'll merge it either way...

Lennart
--
Lennart Poettering, Red Hat
Bastien Nocera
2016-11-24 02:11:46 UTC
Permalink
Post by Lennart Poettering
Post by Bastien Nocera
The latter is currently [not parsed](http://marc.info/?l=linux-iio&
m=147981183211362&w=2) as of kernel 4.9-rc5.
If there is no mount-matrix defined in the device-tree or ACPI DSDT, or
the data is incorrect, it can be overridden with a udev property, in the 
```
iio:FIXME
  ACCEL_MOUNT_MATRIX="1, 0, 0; 0, 1, 0; 0, 0, 1"
Not that it matters at all, but so far files such as 70-mouse.hwdb
mostly used spaces separators for multiple fields, and I'd suggest
that here too, just to keep this similar in style...
The reason why we use this format was explained in the draft
documentation at the bottom. It's the format already used by the
matrix-mount device-tree property. So I'd rather have a single format
to parse for device-tree metadata and the udev quirks.
Post by Lennart Poettering
But then again, this doesn't matter at all, it's entirely up to you,
I'll merge it either way...
Lennart
Loading...