Re: [PATCH] iio/hid: Add mount_matrix

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 24 Jun 2022 15:33:41 -0700
Gwendal Grignou <gwendal@chromium.org> wrote:

> ISH based sensors do not naturally return data in the W3C 'natural'
> orientation.
> They returns all data inverted, to match Microsoft Windows requirement:
> [https://docs.microsoft.com/en-us/windows/uwp/devices-sensors/sensors#accelerometer]
> """ If the device has the SimpleOrientation of FaceUp on a table, then
> the accelerometer would read -1 G on the Z axis. """

Probably reference the HID Usage Tables 1.3 spec rather than the MS one.
https://usb.org/sites/default/files/hut1_3_0.pdf
After some waving around of my left and right hand I'm fairly sure that says the same
thing as the MS spec. Section 4.4 Vector Usages 

> While W3C defines [https://www.w3.org/TR/motion-sensors/#accelerometer-sensor]
> """The Accelerometer sensor is an inertial-frame sensor, this means that
> when the device is in free fall, the acceleration is 0 m/s2 in the
> falling direction, and when a device is laying flat on a table, the
> acceleration in upwards direction will be equal to the Earth gravity,
> i.e. g ≡ 9.8 m/s2 as it is measuring the force of the table pushing the
> device upwards."""
> 
> Fixes all HID sensors that defines IIO_MOD_[XYZ] attributes.
> 
> Tested on "HP Spectre x360 Convertible 13" and "Dell XPS 13 9365".
> 
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>

Ah.  Whilst this is a fix, it seems likely to break a whole bunch of existing
users that are compensating for the wrong orientation in userspace.  Also, do
we know how universal this is?  I have a nasty feeling it'll turn out some
HID sensors do it the other way whatever the spec says.  Bastien, are you
carrying a local fix for this in your userspace code?

+CC a few people who are likely to know more on just how bad that will be...

One other thing inline.  The mount matrix you've provided isn't a rotation matrix.

I'd forgotten the annoyance of graphics folks using the right handed sensor
axis whilst nearly all other uses are left handed. It drove me mad many years
ago - every code base that used sensors and rendered the result needed a
flip of the z axis - it was never well documented, so half the time
the code ended up with many axis flips based on people debugging local
orientation problems.  *sigh*


> ---
>  drivers/iio/accel/hid-sensor-accel-3d.c       |  3 +++
>  .../hid-sensors/hid-sensor-attributes.c       | 21 +++++++++++++++++++
>  drivers/iio/gyro/hid-sensor-gyro-3d.c         |  3 +++
>  drivers/iio/magnetometer/hid-sensor-magn-3d.c |  3 +++
>  include/linux/hid-sensor-hub.h                |  2 ++
>  5 files changed, 32 insertions(+)
> 
> diff --git a/drivers/iio/accel/hid-sensor-accel-3d.c b/drivers/iio/accel/hid-sensor-accel-3d.c
> index a2def6f9380a3..980bbd7fba502 100644
> --- a/drivers/iio/accel/hid-sensor-accel-3d.c
> +++ b/drivers/iio/accel/hid-sensor-accel-3d.c
> @@ -59,6 +59,7 @@ static const struct iio_chan_spec accel_3d_channels[] = {
>  		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>  		BIT(IIO_CHAN_INFO_HYSTERESIS),
>  		.scan_index = CHANNEL_SCAN_INDEX_X,
> +		.ext_info = hid_sensor_ext_info,
>  	}, {
>  		.type = IIO_ACCEL,
>  		.modified = 1,
> @@ -69,6 +70,7 @@ static const struct iio_chan_spec accel_3d_channels[] = {
>  		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>  		BIT(IIO_CHAN_INFO_HYSTERESIS),
>  		.scan_index = CHANNEL_SCAN_INDEX_Y,
> +		.ext_info = hid_sensor_ext_info,
>  	}, {
>  		.type = IIO_ACCEL,
>  		.modified = 1,
> @@ -79,6 +81,7 @@ static const struct iio_chan_spec accel_3d_channels[] = {
>  		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>  		BIT(IIO_CHAN_INFO_HYSTERESIS),
>  		.scan_index = CHANNEL_SCAN_INDEX_Z,
> +		.ext_info = hid_sensor_ext_info,
>  	},
>  	IIO_CHAN_SOFT_TIMESTAMP(CHANNEL_SCAN_INDEX_TIMESTAMP)
>  };
> diff --git a/drivers/iio/common/hid-sensors/hid-sensor-attributes.c b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
> index 9b279937a24e0..e367e4b482ef0 100644
> --- a/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
> +++ b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
> @@ -585,6 +585,27 @@ int hid_sensor_parse_common_attributes(struct hid_sensor_hub_device *hsdev,
>  }
>  EXPORT_SYMBOL_NS(hid_sensor_parse_common_attributes, IIO_HID);
>  
> +static const struct iio_mount_matrix hid_sensor_windows_axis = {
> +	.rotation = {
> +		"-1", "0", "0",
> +		"0", "-1", "0",
> +		"0", "0", "-1"

Unless my memory of rotation matrices serves me wrong, that's not a rotation matrix.
(det(R) != 1)

That's a an axis flip from a right handed set of axis to a left handed one.
So to fix this up, you would need to invert the raw readings of at least one axis
rather than rely on the mount matrix or make the scale negative.

Jonathan


> +	}
> +};
> +
> +static const struct iio_mount_matrix *
> +hid_sensor_get_mount_matrix(const struct iio_dev *indio_dev,
> +				const struct iio_chan_spec *chan)
> +{
> +	return &hid_sensor_windows_axis;
> +}
> +
> +const struct iio_chan_spec_ext_info hid_sensor_ext_info[] = {
> +	IIO_MOUNT_MATRIX(IIO_SHARED_BY_TYPE, hid_sensor_get_mount_matrix),
> +	{ }
> +};
> +EXPORT_SYMBOL(hid_sensor_ext_info);
> +
>  MODULE_AUTHOR("Srinivas Pandruvada <srinivas.pandruvada@intel.com>");
>  MODULE_DESCRIPTION("HID Sensor common attribute processing");
>  MODULE_LICENSE("GPL");
> diff --git a/drivers/iio/gyro/hid-sensor-gyro-3d.c b/drivers/iio/gyro/hid-sensor-gyro-3d.c
> index 8f0ad022c7f1b..b852f5166bb21 100644
> --- a/drivers/iio/gyro/hid-sensor-gyro-3d.c
> +++ b/drivers/iio/gyro/hid-sensor-gyro-3d.c
> @@ -58,6 +58,7 @@ static const struct iio_chan_spec gyro_3d_channels[] = {
>  		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>  		BIT(IIO_CHAN_INFO_HYSTERESIS),
>  		.scan_index = CHANNEL_SCAN_INDEX_X,
> +		.ext_info = hid_sensor_ext_info,
>  	}, {
>  		.type = IIO_ANGL_VEL,
>  		.modified = 1,
> @@ -68,6 +69,7 @@ static const struct iio_chan_spec gyro_3d_channels[] = {
>  		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>  		BIT(IIO_CHAN_INFO_HYSTERESIS),
>  		.scan_index = CHANNEL_SCAN_INDEX_Y,
> +		.ext_info = hid_sensor_ext_info,
>  	}, {
>  		.type = IIO_ANGL_VEL,
>  		.modified = 1,
> @@ -78,6 +80,7 @@ static const struct iio_chan_spec gyro_3d_channels[] = {
>  		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>  		BIT(IIO_CHAN_INFO_HYSTERESIS),
>  		.scan_index = CHANNEL_SCAN_INDEX_Z,
> +		.ext_info = hid_sensor_ext_info,
>  	},
>  	IIO_CHAN_SOFT_TIMESTAMP(CHANNEL_SCAN_INDEX_TIMESTAMP)
>  };
> diff --git a/drivers/iio/magnetometer/hid-sensor-magn-3d.c b/drivers/iio/magnetometer/hid-sensor-magn-3d.c
> index e85a3a8eea908..aefbdb9b0869a 100644
> --- a/drivers/iio/magnetometer/hid-sensor-magn-3d.c
> +++ b/drivers/iio/magnetometer/hid-sensor-magn-3d.c
> @@ -74,6 +74,7 @@ static const struct iio_chan_spec magn_3d_channels[] = {
>  		BIT(IIO_CHAN_INFO_SCALE) |
>  		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>  		BIT(IIO_CHAN_INFO_HYSTERESIS),
> +		.ext_info = hid_sensor_ext_info,
>  	}, {
>  		.type = IIO_MAGN,
>  		.modified = 1,
> @@ -83,6 +84,7 @@ static const struct iio_chan_spec magn_3d_channels[] = {
>  		BIT(IIO_CHAN_INFO_SCALE) |
>  		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>  		BIT(IIO_CHAN_INFO_HYSTERESIS),
> +		.ext_info = hid_sensor_ext_info,
>  	}, {
>  		.type = IIO_MAGN,
>  		.modified = 1,
> @@ -92,6 +94,7 @@ static const struct iio_chan_spec magn_3d_channels[] = {
>  		BIT(IIO_CHAN_INFO_SCALE) |
>  		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>  		BIT(IIO_CHAN_INFO_HYSTERESIS),
> +		.ext_info = hid_sensor_ext_info,
>  	}, {
>  		.type = IIO_ROT,
>  		.modified = 1,
> diff --git a/include/linux/hid-sensor-hub.h b/include/linux/hid-sensor-hub.h
> index c27329e2a5ad5..ee7d5b430a785 100644
> --- a/include/linux/hid-sensor-hub.h
> +++ b/include/linux/hid-sensor-hub.h
> @@ -236,6 +236,8 @@ struct hid_sensor_common {
>  	struct work_struct work;
>  };
>  
> +extern const struct iio_chan_spec_ext_info hid_sensor_ext_info[];
> +
>  /* Convert from hid unit expo to regular exponent */
>  static inline int hid_sensor_convert_exponent(int unit_expo)
>  {