[RFC PATCH 0/3] iio: export mounting matrix

[RFC PATCH 0/3] iio: export mounting matrix

[Octavian Purdila]

This is a RFC patch series that exports information about the way the
sensors are connected to the system so that userspace can correct
readings.

It is inspired by the way MPU6050 exports this information via the
in_{gyro,accel,magn}_matrix sysfs entries.

Octavian Purdila (3):
  iio: export mounting matrix information
  ACPICA: Add _PLD front and back panel constants
  iio: derive the mounting matrix from ACPI _PLD objects

 Documentation/ABI/testing/sysfs-bus-iio | 11 ++++
 drivers/iio/industrialio-core.c         | 93 +++++++++++++++++++++++++++++++++
 include/acpi/acbuffer.h                 |  5 ++
 include/linux/iio/iio.h                 | 55 +++++++++++++++++++
 4 files changed, 164 insertions(+)

-- 
1.9.1

[RFC PATCH 1/3] iio: export mounting matrix information

[Octavian Purdila]

Export a new sysfs attribute that describes the placement of the
sensor on the device in the form of a mounting matrix so that
userspace can do the required correction to get accurate data:

corrected X = MM[0] * X + MM[1] Y + MM[2] * Z
corrected Y = MM[3] * X + MM[4] Y + MM[5] * Z
corrected Z = MM[6] * X + MM[7] Y + MM[8] * Z

This information is read from the device properties of the parent of
the IIO device.

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 Documentation/ABI/testing/sysfs-bus-iio | 11 +++++++++
 drivers/iio/industrialio-core.c         | 42 +++++++++++++++++++++++++++++++++
 include/linux/iio/iio.h                 |  9 +++++++
 3 files changed, 62 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index 866b4ec..d36476e 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -1375,3 +1375,14 @@ Description:
 		The emissivity ratio of the surface in the field of view of the
 		contactless temperature sensor.  Emissivity varies from 0 to 1,
 		with 1 being the emissivity of a black body.
+
+What:		/sys/bus/iio/devices/iio:deviceX/mounting_matrix
+KernelVersion:	4.2
+Contact:	linux-iio@vger.kernel.org
+Description:
+		A list of 9 floating point values that describes the
+		transformation needed to account for the way the sensor has been
+		placed on the PCB:
+		corrected X = MM[0] * X + MM[1] Y + MM[2] * Z
+		corrected Y = MM[3] * X + MM[4] Y + MM[5] * Z
+		corrected Z = MM[6] * X + MM[7] Y + MM[8] * Z
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 7c98bc1..9000c53 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -871,6 +871,45 @@ static ssize_t iio_show_dev_name(struct device *dev,
 
 static DEVICE_ATTR(name, S_IRUGO, iio_show_dev_name, NULL);
 
+static bool iio_get_mounting_matrix(struct iio_dev *indio_dev)
+{
+	int i, err;
+
+	err = device_property_read_u32_array(&indio_dev->dev, "mounting-matrix",
+					     indio_dev->mounting_matrix, 9);
+	if (!err) {
+		int *mm = indio_dev->mounting_matrix;
+
+		for (i = IIO_MM_SIZE - 2; i > 0; i -= 2) {
+			mm[i] = mm[i / 2];
+			mm[i + 1] = 0;
+		}
+
+		return true;
+	}
+
+	return false;
+}
+
+static ssize_t iio_show_mounting_matrix(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	char *tmp = buf;
+	int i;
+
+	for (i = 0; i < IIO_MM_SIZE; i += 2) {
+		tmp += iio_format_value(tmp, IIO_VAL_INT_PLUS_MICRO, 2,
+					&indio_dev->mounting_matrix[i]);
+		if (((i + 2) / 2) % 3)
+			tmp[-1] = ' ';
+	}
+	return tmp - buf;
+}
+
+static DEVICE_ATTR(mounting_matrix, S_IRUGO, iio_show_mounting_matrix, NULL);
+
 static int iio_device_register_sysfs(struct iio_dev *indio_dev)
 {
 	int i, ret = 0, attrcount, attrn, attrcount_orig = 0;
@@ -920,6 +959,9 @@ static int iio_device_register_sysfs(struct iio_dev *indio_dev)
 		indio_dev->chan_attr_group.attrs[attrn++] = &p->dev_attr.attr;
 	if (indio_dev->name)
 		indio_dev->chan_attr_group.attrs[attrn++] = &dev_attr_name.attr;
+	if (iio_get_mounting_matrix(indio_dev))
+		indio_dev->chan_attr_group.attrs[attrn++] =
+			&dev_attr_mounting_matrix.attr;
 
 	indio_dev->groups[indio_dev->groupcounter++] =
 		&indio_dev->chan_attr_group;
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index b1e46ae..c1fa852 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -434,6 +434,8 @@ struct iio_buffer_setup_ops {
 				   const unsigned long *scan_mask);
 };
 
+#define IIO_MM_SIZE		18
+
 /**
  * struct iio_dev - industrial I/O device
  * @id:			[INTERN] used to identify device internally
@@ -469,6 +471,12 @@ struct iio_buffer_setup_ops {
  * @groups:		[INTERN] attribute groups
  * @groupcounter:	[INTERN] index of next attribute group
  * @flags:		[INTERN] file ops related flags including busy flag.
+ * @mounting_matrix:	[INTERN] the mounting matrix is a series of 9
+ *			IIO_VAL_INT_PLUS_MICRO pairs that describes the
+ *			transformation needed to account for the way the sensor
+ *			has been placed on the PCB. See the mounting_matrix
+ *			entry in Documentation/ABI/testing/sysfs-bus-iio about
+ *			details of the transformation operations.
  * @debugfs_dentry:	[INTERN] device specific debugfs dentry.
  * @cached_reg_addr:	[INTERN] cached register address for debugfs reads.
  */
@@ -509,6 +517,7 @@ struct iio_dev {
 	int				groupcounter;
 
 	unsigned long			flags;
+	int				mounting_matrix[IIO_MM_SIZE];
 #if defined(CONFIG_DEBUG_FS)
 	struct dentry			*debugfs_dentry;
 	unsigned			cached_reg_addr;
-- 
1.9.1

[RFC PATCH 2/3] ACPICA: Add _PLD front and back panel constants

[Octavian Purdila]

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 include/acpi/acbuffer.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/acpi/acbuffer.h b/include/acpi/acbuffer.h
index 6b040f4..2be0270 100644
--- a/include/acpi/acbuffer.h
+++ b/include/acpi/acbuffer.h
@@ -139,6 +139,11 @@ struct acpi_pld_info {
 	u16 horizontal_offset;
 };
 
+enum {
+	ACPI_PLD_PANEL_FRONT = 4,
+	ACPI_PLD_PANEL_BACK
+};
+
 /*
  * Macros to:
  *     1) Convert a _PLD buffer to internal struct acpi_pld_info format - ACPI_PLD_GET*
-- 
1.9.1

[RFC PATCH 3/3] iio: derive the mounting matrix from ACPI _PLD objects

[Octavian Purdila]

This patch derives the mounting matrix for a particular IIO device
based ont the ACPI _PLD information. Note that if mounting matrix is
defined in the device properties it overrieds the _PLD information.

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 drivers/iio/industrialio-core.c | 51 +++++++++++++++++++++++++++++++++++++++++
 include/linux/iio/iio.h         | 46 +++++++++++++++++++++++++++++++++++++
 2 files changed, 97 insertions(+)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 9000c53..90ee58a 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -31,6 +31,7 @@
 #include <linux/iio/sysfs.h>
 #include <linux/iio/events.h>
 #include <linux/iio/buffer.h>
+#include <linux/acpi.h>
 
 /* IDA to assign each registered device a unique id */
 static DEFINE_IDA(iio_ida);
@@ -871,6 +872,53 @@ static ssize_t iio_show_dev_name(struct device *dev,
 
 static DEVICE_ATTR(name, S_IRUGO, iio_show_dev_name, NULL);
 
+#if IS_ENABLED(CONFIG_ACPI)
+static bool iio_get_mounting_matrix_acpi(struct iio_dev *indio_dev)
+{
+	acpi_handle h = ACPI_HANDLE(indio_dev->dev.parent);
+	struct acpi_pld_info *info;
+
+	if (ACPI_SUCCESS(acpi_get_physical_device_location(h, &info))) {
+		IIO_MM_SET(indio_dev->mounting_matrix, X, X, 1, 0);
+		IIO_MM_SET(indio_dev->mounting_matrix, Y, Y, 1, 0);
+		IIO_MM_SET(indio_dev->mounting_matrix, Z, Z, 1, 0);
+
+		/* Chip placed on the back panel ; negate x and z */
+		if (info->panel == ACPI_PLD_PANEL_BACK) {
+			IIO_MM_MUL(indio_dev->mounting_matrix, X, X, -1, 0);
+			IIO_MM_MUL(indio_dev->mounting_matrix, Z, Z, -1, 0);
+		}
+
+		switch (info->rotation) {
+		case 2:
+			/* 90 deg clockwise: negate y then swap x,y */
+			IIO_MM_MUL(indio_dev->mounting_matrix, Y, Y, -1, 0);
+			IIO_MM_SWAP(indio_dev->mounting_matrix, X, Y);
+			break;
+		case 4:
+			/* Upside down: negate x and y */
+			IIO_MM_MUL(indio_dev->mounting_matrix, X, X, -1, 0);
+			IIO_MM_MUL(indio_dev->mounting_matrix, Y, Y, -1, 0);
+			break;
+		case 6:
+			/* 90 deg counter clockwise: negate x then swap x,y */
+			IIO_MM_MUL(indio_dev->mounting_matrix, X, X, -1, 0);
+			IIO_MM_SWAP(indio_dev->mounting_matrix, X, Y);
+			break;
+		}
+
+		return true;
+	}
+
+	return false;
+}
+#else
+static bool iio_get_mounting_matrix_acpi(struct iio_dev *indio_dev)
+{
+	return false;
+}
+#endif
+
 static bool iio_get_mounting_matrix(struct iio_dev *indio_dev)
 {
 	int i, err;
@@ -888,6 +936,9 @@ static bool iio_get_mounting_matrix(struct iio_dev *indio_dev)
 		return true;
 	}
 
+	if (ACPI_HANDLE(indio_dev->dev.parent))
+		return iio_get_mounting_matrix_acpi(indio_dev);
+
 	return false;
 }
 
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index c1fa852..feb7813 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -436,6 +436,52 @@ struct iio_buffer_setup_ops {
 
 #define IIO_MM_SIZE		18
 
+enum {
+	IIO_MM_XX0,
+	IIO_MM_XX1,
+	IIO_MM_XY0,
+	IIO_MM_XY1,
+	IIO_MM_XZ0,
+	IIO_MM_XZ1,
+	IIO_MM_YX0,
+	IIO_MM_YX1,
+	IIO_MM_YY0,
+	IIO_MM_YY1,
+	IIO_MM_YZ0,
+	IIO_MM_YZ1,
+	IIO_MM_ZX0,
+	IIO_MM_ZX1,
+	IIO_MM_ZY0,
+	IIO_MM_ZY1,
+	IIO_MM_ZZ0,
+	IIO_MM_ZZ1,
+};
+
+#define IIO_MM_SET(mm, a, b, val0, val1)	\
+	do {					\
+		mm[IIO_MM_##a##b##0] = val0;	\
+		mm[IIO_MM_##a##b##1] = val1;	\
+	} while (0)				\
+
+#define IIO_MM_MUL(mm, a, b, val0, val1)	\
+	do {					\
+		mm[IIO_MM_##a##b##0] *= val0;	\
+		mm[IIO_MM_##a##b##0] *= val1;	\
+	} while (0)				\
+
+#define IIO_MM_SWAP(mm, x, y)					\
+	do {							\
+		int tmp;					\
+								\
+		tmp = mm[IIO_MM_##x##x##0];			\
+		mm[IIO_MM_##x##x##0] = mm[IIO_MM_##y##y##0];	\
+		mm[IIO_MM_##y##y##0] = tmp;			\
+								\
+		tmp = mm[IIO_MM_##x##x##1];			\
+		mm[IIO_MM_##x##x##1] = mm[IIO_MM_##y##y##1];	\
+		mm[IIO_MM_##y##y##1] = tmp;			\
+	} while (0)						\
+
 /**
  * struct iio_dev - industrial I/O device
  * @id:			[INTERN] used to identify device internally
-- 
1.9.1

Re: [RFC PATCH 3/3] iio: derive the mounting matrix from ACPI _PLD objects

[Kuppuswamy Sathyanarayanan]

Since Acpi framework already exports this info to user space, Why not do
this derivation in user space code ? Why do we need new ABI, if the same
can be derived from existing one.

> This patch derives the mounting matrix for a particular IIO device
> based ont the ACPI _PLD information. Note that if mounting matrix is
> defined in the device properties it overrieds the _PLD information.
>
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> ---
>  drivers/iio/industrialio-core.c | 51
> +++++++++++++++++++++++++++++++++++++++++
>  include/linux/iio/iio.h         | 46
> +++++++++++++++++++++++++++++++++++++
>  2 files changed, 97 insertions(+)
>
> diff --git a/drivers/iio/industrialio-core.c
> b/drivers/iio/industrialio-core.c
> index 9000c53..90ee58a 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -31,6 +31,7 @@
>  #include <linux/iio/sysfs.h>
>  #include <linux/iio/events.h>
>  #include <linux/iio/buffer.h>
> +#include <linux/acpi.h>
>
>  /* IDA to assign each registered device a unique id */
>  static DEFINE_IDA(iio_ida);
> @@ -871,6 +872,53 @@ static ssize_t iio_show_dev_name(struct device *dev,
>
>  static DEVICE_ATTR(name, S_IRUGO, iio_show_dev_name, NULL);
>
> +#if IS_ENABLED(CONFIG_ACPI)
> +static bool iio_get_mounting_matrix_acpi(struct iio_dev *indio_dev)
> +{
> +	acpi_handle h = ACPI_HANDLE(indio_dev->dev.parent);
> +	struct acpi_pld_info *info;
> +
> +	if (ACPI_SUCCESS(acpi_get_physical_device_location(h, &info))) {
> +		IIO_MM_SET(indio_dev->mounting_matrix, X, X, 1, 0);
> +		IIO_MM_SET(indio_dev->mounting_matrix, Y, Y, 1, 0);
> +		IIO_MM_SET(indio_dev->mounting_matrix, Z, Z, 1, 0);
> +
> +		/* Chip placed on the back panel ; negate x and z */
> +		if (info->panel == ACPI_PLD_PANEL_BACK) {
> +			IIO_MM_MUL(indio_dev->mounting_matrix, X, X, -1, 0);
> +			IIO_MM_MUL(indio_dev->mounting_matrix, Z, Z, -1, 0);
> +		}
> +
> +		switch (info->rotation) {
> +		case 2:
> +			/* 90 deg clockwise: negate y then swap x,y */
> +			IIO_MM_MUL(indio_dev->mounting_matrix, Y, Y, -1, 0);
> +			IIO_MM_SWAP(indio_dev->mounting_matrix, X, Y);
> +			break;
> +		case 4:
> +			/* Upside down: negate x and y */
> +			IIO_MM_MUL(indio_dev->mounting_matrix, X, X, -1, 0);
> +			IIO_MM_MUL(indio_dev->mounting_matrix, Y, Y, -1, 0);
> +			break;
> +		case 6:
> +			/* 90 deg counter clockwise: negate x then swap x,y */
> +			IIO_MM_MUL(indio_dev->mounting_matrix, X, X, -1, 0);
> +			IIO_MM_SWAP(indio_dev->mounting_matrix, X, Y);
> +			break;
> +		}
> +
> +		return true;
> +	}
> +
> +	return false;
> +}
> +#else
> +static bool iio_get_mounting_matrix_acpi(struct iio_dev *indio_dev)
> +{
> +	return false;
> +}
> +#endif
> +
>  static bool iio_get_mounting_matrix(struct iio_dev *indio_dev)
>  {
>  	int i, err;
> @@ -888,6 +936,9 @@ static bool iio_get_mounting_matrix(struct iio_dev
> *indio_dev)
>  		return true;
>  	}
>
> +	if (ACPI_HANDLE(indio_dev->dev.parent))
> +		return iio_get_mounting_matrix_acpi(indio_dev);
> +
>  	return false;
>  }
>
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index c1fa852..feb7813 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -436,6 +436,52 @@ struct iio_buffer_setup_ops {
>
>  #define IIO_MM_SIZE		18
>
> +enum {
> +	IIO_MM_XX0,
> +	IIO_MM_XX1,
> +	IIO_MM_XY0,
> +	IIO_MM_XY1,
> +	IIO_MM_XZ0,
> +	IIO_MM_XZ1,
> +	IIO_MM_YX0,
> +	IIO_MM_YX1,
> +	IIO_MM_YY0,
> +	IIO_MM_YY1,
> +	IIO_MM_YZ0,
> +	IIO_MM_YZ1,
> +	IIO_MM_ZX0,
> +	IIO_MM_ZX1,
> +	IIO_MM_ZY0,
> +	IIO_MM_ZY1,
> +	IIO_MM_ZZ0,
> +	IIO_MM_ZZ1,
> +};
> +
> +#define IIO_MM_SET(mm, a, b, val0, val1)	\
> +	do {					\
> +		mm[IIO_MM_##a##b##0] = val0;	\
> +		mm[IIO_MM_##a##b##1] = val1;	\
> +	} while (0)				\
> +
> +#define IIO_MM_MUL(mm, a, b, val0, val1)	\
> +	do {					\
> +		mm[IIO_MM_##a##b##0] *= val0;	\
> +		mm[IIO_MM_##a##b##0] *= val1;	\
> +	} while (0)				\
> +
> +#define IIO_MM_SWAP(mm, x, y)					\
> +	do {							\
> +		int tmp;					\
> +								\
> +		tmp = mm[IIO_MM_##x##x##0];			\
> +		mm[IIO_MM_##x##x##0] = mm[IIO_MM_##y##y##0];	\
> +		mm[IIO_MM_##y##y##0] = tmp;			\
> +								\
> +		tmp = mm[IIO_MM_##x##x##1];			\
> +		mm[IIO_MM_##x##x##1] = mm[IIO_MM_##y##y##1];	\
> +		mm[IIO_MM_##y##y##1] = tmp;			\
> +	} while (0)						\
> +
>  /**
>   * struct iio_dev - industrial I/O device
>   * @id:			[INTERN] used to identify device internally
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


-- 
Sathyanarayanan Kuppuswamy

Re: [RFC PATCH 3/3] iio: derive the mounting matrix from ACPI _PLD objects

[Octavian Purdila]

On Mon, Apr 27, 2015 at 6:42 PM, Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> Since Acpi framework already exports this info to user space, Why not do
> this derivation in user space code ? Why do we need new ABI, if the same
> can be derived from existing one.
>

The ABI was added in the previous patch so that we can present the
sensor orientation information to userspace even in the case of device
tree.

The purpose of this patch is to provide a consistent ABI to userspace,
i.e. to avoid doing one thing in the ACPI case and another thing in
the case of device tree.

Re: [RFC PATCH 3/3] iio: derive the mounting matrix from ACPI _PLD objects

[sathyanarayanan kuppuswamy]

Hi

On 04/27/2015 08:54 AM, Octavian Purdila wrote:
> On Mon, Apr 27, 2015 at 6:42 PM, Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>> Since Acpi framework already exports this info to user space, Why not do
>> this derivation in user space code ? Why do we need new ABI, if the same
>> can be derived from existing one.
>>
> The ABI was added in the previous patch so that we can present the
> sensor orientation information to userspace even in the case of device
> tree.
If the main reason for implementing a new ABI is to support DT 
platforms, Why not implement a version of _PLD for device tree ? Don't 
you think it would be much better than adding a new ABI to export 
redundant information ?

Also the location information of the device is not just specific to iio 
drivers. You should consider that we would have similar requirements for 
devices implemented as input or platform drivers.
>
> The purpose of this patch is to provide a consistent ABI to userspace,
> i.e. to avoid doing one thing in the ACPI case and another thing in
> the case of device tree.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

Re: [RFC PATCH 3/3] iio: derive the mounting matrix from ACPI _PLD objects

[Octavian Purdila]

On Tue, Apr 28, 2015 at 12:57 AM, sathyanarayanan kuppuswamy
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> Hi
>
> On 04/27/2015 08:54 AM, Octavian Purdila wrote:
>>
>> On Mon, Apr 27, 2015 at 6:42 PM, Kuppuswamy Sathyanarayanan
>> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>>>
>>> Since Acpi framework already exports this info to user space, Why not do
>>> this derivation in user space code ? Why do we need new ABI, if the same
>>> can be derived from existing one.
>>>
>> The ABI was added in the previous patch so that we can present the
>> sensor orientation information to userspace even in the case of device
>> tree.
>
> If the main reason for implementing a new ABI is to support DT platforms,
> Why not implement a version of _PLD for device tree ? Don't you think it
> would be much better than adding a new ABI to export redundant information ?
>

IMO the mounting matrix is more consistent with the IIO ABIs. Although
I have no issue with repicating _PLD for device tree if people agree
that it is better.

> Also the location information of the device is not just specific to iio
> drivers. You should consider that we would have similar requirements for
> devices implemented as input or platform drivers.

The upstream standard for those sensors where the orientation matters
(accelerometer, gyro, compass) is IIO.

Granted, there are other device types for which the orientation
information may be useful (e.g. camera). However the actual
interpretation and action to be taken is different for each subsystem
(e.g. in the camera case do the correction via V4L2_CID_HFLIP /
V4L2_CID_VFLIP) so I think it is better to expose it at the subsystem
level in a way consistent with the subsystem's ABIs.

Re: [RFC PATCH 3/3] iio: derive the mounting matrix from ACPI _PLD objects

[Sathyanarayanan Kuppuswamy]

Hi Octavian,

On 04/27/2015 07:23 PM, Octavian Purdila wrote:
> On Tue, Apr 28, 2015 at 12:57 AM, sathyanarayanan kuppuswamy
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>> Hi
>>
>> On 04/27/2015 08:54 AM, Octavian Purdila wrote:
>>> On Mon, Apr 27, 2015 at 6:42 PM, Kuppuswamy Sathyanarayanan
>>> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>>>> Since Acpi framework already exports this info to user space, Why not do
>>>> this derivation in user space code ? Why do we need new ABI, if the same
>>>> can be derived from existing one.
>>>>
>>> The ABI was added in the previous patch so that we can present the
>>> sensor orientation information to userspace even in the case of device
>>> tree.
>> If the main reason for implementing a new ABI is to support DT platforms,
>> Why not implement a version of _PLD for device tree ? Don't you think it
>> would be much better than adding a new ABI to export redundant information ?
>>
> IMO the mounting matrix is more consistent with the IIO ABIs. Although
> I have no issue with repicating _PLD for device tree if people agree
> that it is better.
Since your main issue is, device tree lacking ABI to specify location
information, you should consider fixing it there. Let's wait for others
comment on this.

If you think mounting matrix provides more information than what is 
supported
by _PLD,  then we should consider implementing another ABI. AFAIK, that 
is not
the case here.

Adding adding a new ABI to represent the information that can be derived
from existing ABI does not seem to be useful.
>
>> Also the location information of the device is not just specific to iio
>> drivers. You should consider that we would have similar requirements for
>> devices implemented as input or platform drivers.
> The upstream standard for those sensors where the orientation matters
> (accelerometer, gyro, compass) is IIO.
>
> Granted, there are other device types for which the orientation
> information may be useful (e.g. camera). However the actual
> interpretation and action to be taken is different for each subsystem
> (e.g. in the camera case do the correction via V4L2_CID_HFLIP /
> V4L2_CID_VFLIP) so I think it is better to expose it at the subsystem
> level in a way consistent with the subsystem's ABIs.
I agree that location information is used differently at different
sub systems. But my question is why we need  a new ABI ?

Why not handle it in user space ?

-- 
--
Sathyanarayanan KN
Android Kernel Developer

Re: [RFC PATCH 3/3] iio: derive the mounting matrix from ACPI _PLD objects

[Octavian Purdila]

On Mon, May 4, 2015 at 4:11 AM, Sathyanarayanan Kuppuswamy
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> Hi Octavian,
>
> On 04/27/2015 07:23 PM, Octavian Purdila wrote:
>>
>> On Tue, Apr 28, 2015 at 12:57 AM, sathyanarayanan kuppuswamy
>> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>>>
>>> Hi
>>>
>>> On 04/27/2015 08:54 AM, Octavian Purdila wrote:
>>>>
>>>> On Mon, Apr 27, 2015 at 6:42 PM, Kuppuswamy Sathyanarayanan
>>>> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>>>>>
>>>>> Since Acpi framework already exports this info to user space, Why not
>>>>> do
>>>>> this derivation in user space code ? Why do we need new ABI, if the
>>>>> same
>>>>> can be derived from existing one.
>>>>>
>>>> The ABI was added in the previous patch so that we can present the
>>>> sensor orientation information to userspace even in the case of device
>>>> tree.
>>>
>>> If the main reason for implementing a new ABI is to support DT platforms,
>>> Why not implement a version of _PLD for device tree ? Don't you think it
>>> would be much better than adding a new ABI to export redundant
>>> information ?
>>>
>> IMO the mounting matrix is more consistent with the IIO ABIs. Although
>> I have no issue with repicating _PLD for device tree if people agree
>> that it is better.
>
> Since your main issue is, device tree lacking ABI to specify location
> information, you should consider fixing it there. Let's wait for others
> comment on this.
>
> If you think mounting matrix provides more information than what is
> supported
> by _PLD,  then we should consider implementing another ABI. AFAIK, that is
> not
> the case here.
>
> Adding adding a new ABI to represent the information that can be derived
> from existing ABI does not seem to be useful.

AFAICS the ACPI _PLD information is not (yet) exported to userspace. This patch:

http://marc.info/?t=140795040700003&r=1&w=2

does not seem to be merged upstream. So there is no existing ABI to
derive from :)

>>
>>
>>> Also the location information of the device is not just specific to iio
>>> drivers. You should consider that we would have similar requirements for
>>> devices implemented as input or platform drivers.
>>
>> The upstream standard for those sensors where the orientation matters
>> (accelerometer, gyro, compass) is IIO.
>>
>> Granted, there are other device types for which the orientation
>> information may be useful (e.g. camera). However the actual
>> interpretation and action to be taken is different for each subsystem
>> (e.g. in the camera case do the correction via V4L2_CID_HFLIP /
>> V4L2_CID_VFLIP) so I think it is better to expose it at the subsystem
>> level in a way consistent with the subsystem's ABIs.
>
> I agree that location information is used differently at different
> sub systems. But my question is why we need  a new ABI ?
>
> Why not handle it in user space ?
>
> -

Re: [RFC PATCH 3/3] iio: derive the mounting matrix from ACPI _PLD objects

[Jonathan Cameron]

On 28/04/15 03:23, Octavian Purdila wrote:
> On Tue, Apr 28, 2015 at 12:57 AM, sathyanarayanan kuppuswamy
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>> Hi
>>
>> On 04/27/2015 08:54 AM, Octavian Purdila wrote:
>>>
>>> On Mon, Apr 27, 2015 at 6:42 PM, Kuppuswamy Sathyanarayanan
>>> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>>>>
>>>> Since Acpi framework already exports this info to user space, Why not do
>>>> this derivation in user space code ? Why do we need new ABI, if the same
>>>> can be derived from existing one.
>>>>
>>> The ABI was added in the previous patch so that we can present the
>>> sensor orientation information to userspace even in the case of device
>>> tree.
>>
>> If the main reason for implementing a new ABI is to support DT platforms,
>> Why not implement a version of _PLD for device tree ? Don't you think it
>> would be much better than adding a new ABI to export redundant information ?
>>
> 
> IMO the mounting matrix is more consistent with the IIO ABIs. Although
> I have no issue with repicating _PLD for device tree if people agree
> that it is better.
> 
>> Also the location information of the device is not just specific to iio
>> drivers. You should consider that we would have similar requirements for
>> devices implemented as input or platform drivers.
> 
> The upstream standard for those sensors where the orientation matters
> (accelerometer, gyro, compass) is IIO.
It's probably worth pull Dmitry in on this conversation as well (and maybe
the input list). cc'd.

For reference of those who haven't seen this before.  The question here is about
exposing the sensor mounting matrix ( orientation of the sensor frame of reference
relative to some 'magic' orientation - probably the PCB )

Note I'd also throw in here that I'd argue for a combined R T matrix
e.g. [R T] so as to cover cases where we care about the position as well as the
orientation relative to some reference point.  A class case in point would
be rotation measurement from offset accelerometers.

Could go with a full projective geometry matrix, but probably better to keep it
in some base scale e.g.
[R   T]
[000 1]
but not bother exporting the 000 1 as it'll always be the same.


Note I've written this email whilst without net access (free wifi at
Stansted airport is rubbish!) so haven't checked out the existing ACPI ABI.
Will try and remember to do this when I get a moment with working internet.

Jonathan  
> 
> Granted, there are other device types for which the orientation
> information may be useful (e.g. camera). However the actual
> interpretation and action to be taken is different for each subsystem
> (e.g. in the camera case do the correction via V4L2_CID_HFLIP /
> V4L2_CID_VFLIP) so I think it is better to expose it at the subsystem
> level in a way consistent with the subsystem's ABIs.
> 

Re: [RFC PATCH 1/3] iio: export mounting matrix information

[Jonathan Cameron]

On 27/04/15 16:01, Octavian Purdila wrote:
> Export a new sysfs attribute that describes the placement of the
> sensor on the device in the form of a mounting matrix so that
> userspace can do the required correction to get accurate data:
> 
> corrected X = MM[0] * X + MM[1] Y + MM[2] * Z
> corrected Y = MM[3] * X + MM[4] Y + MM[5] * Z
> corrected Z = MM[6] * X + MM[7] Y + MM[8] * Z
> 
> This information is read from the device properties of the parent of
> the IIO device.
> 
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
Hmm.  This could be done with existing code and a new info_mask element.
The disadvantage is you'd have to change the read_raw to read_raw_multi
callback, but that's hardly a big job.  If I could be bothered, I'd
move the whole subsystem over and drop read_raw entirely.

It was introduced precisely for this sort of element.

As I mentioned deeper in the thread.  If we are going to introduce a mounting
matrix, I'd prefer a version that includes translation.  It might not matter
to many devices but it has mattered a lot to me in the past and doesn't hurt
others that much (I used to write papers on how to estimate this stuff
from black box devices).
> ---
>  Documentation/ABI/testing/sysfs-bus-iio | 11 +++++++++
>  drivers/iio/industrialio-core.c         | 42 +++++++++++++++++++++++++++++++++
>  include/linux/iio/iio.h                 |  9 +++++++
>  3 files changed, 62 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index 866b4ec..d36476e 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -1375,3 +1375,14 @@ Description:
>  		The emissivity ratio of the surface in the field of view of the
>  		contactless temperature sensor.  Emissivity varies from 0 to 1,
>  		with 1 being the emissivity of a black body.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/mounting_matrix
> +KernelVersion:	4.2
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		A list of 9 floating point values that describes the
> +		transformation needed to account for the way the sensor has been
> +		placed on the PCB:
> +		corrected X = MM[0] * X + MM[1] Y + MM[2] * Z
> +		corrected Y = MM[3] * X + MM[4] Y + MM[5] * Z
> +		corrected Z = MM[6] * X + MM[7] Y + MM[8] * Z
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 7c98bc1..9000c53 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -871,6 +871,45 @@ static ssize_t iio_show_dev_name(struct device *dev,
>  
>  static DEVICE_ATTR(name, S_IRUGO, iio_show_dev_name, NULL);
>  
> +static bool iio_get_mounting_matrix(struct iio_dev *indio_dev)
> +{
> +	int i, err;
> +
> +	err = device_property_read_u32_array(&indio_dev->dev, "mounting-matrix",
> +					     indio_dev->mounting_matrix, 9);
> +	if (!err) {
> +		int *mm = indio_dev->mounting_matrix;
> +
> +		for (i = IIO_MM_SIZE - 2; i > 0; i -= 2) {
> +			mm[i] = mm[i / 2];
> +			mm[i + 1] = 0;
> +		}
> +
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +static ssize_t iio_show_mounting_matrix(struct device *dev,
> +					struct device_attribute *attr,
> +					char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	char *tmp = buf;
> +	int i;
> +
> +	for (i = 0; i < IIO_MM_SIZE; i += 2) {
> +		tmp += iio_format_value(tmp, IIO_VAL_INT_PLUS_MICRO, 2,
> +					&indio_dev->mounting_matrix[i]);
> +		if (((i + 2) / 2) % 3)
Hmm. if (i != 8) should also do the job with slightly less feedling of
magic..
> +			tmp[-1] = ' ';
> +	}
> +	return tmp - buf;
> +}
> +
> +static DEVICE_ATTR(mounting_matrix, S_IRUGO, iio_show_mounting_matrix, NULL);
> +
>  static int iio_device_register_sysfs(struct iio_dev *indio_dev)
>  {
>  	int i, ret = 0, attrcount, attrn, attrcount_orig = 0;
> @@ -920,6 +959,9 @@ static int iio_device_register_sysfs(struct iio_dev *indio_dev)
>  		indio_dev->chan_attr_group.attrs[attrn++] = &p->dev_attr.attr;
>  	if (indio_dev->name)
>  		indio_dev->chan_attr_group.attrs[attrn++] = &dev_attr_name.attr;
> +	if (iio_get_mounting_matrix(indio_dev))
> +		indio_dev->chan_attr_group.attrs[attrn++] =
> +			&dev_attr_mounting_matrix.attr;
>  
>  	indio_dev->groups[indio_dev->groupcounter++] =
>  		&indio_dev->chan_attr_group;
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index b1e46ae..c1fa852 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -434,6 +434,8 @@ struct iio_buffer_setup_ops {
>  				   const unsigned long *scan_mask);
>  };
>  
> +#define IIO_MM_SIZE		18
> +
>  /**
>   * struct iio_dev - industrial I/O device
>   * @id:			[INTERN] used to identify device internally
> @@ -469,6 +471,12 @@ struct iio_buffer_setup_ops {
>   * @groups:		[INTERN] attribute groups
>   * @groupcounter:	[INTERN] index of next attribute group
>   * @flags:		[INTERN] file ops related flags including busy flag.
Hmm. No absolute need to have this new data in the iio_dev.  I'd do it
via the read_raw_multi callback, though this will require converting read_raw
over to that more flexible version.
> + * @mounting_matrix:	[INTERN] the mounting matrix is a series of 9
> + *			IIO_VAL_INT_PLUS_MICRO pairs that describes the
> + *			transformation needed to account for the way the sensor
> + *			has been placed on the PCB. See the mounting_matrix
> + *			entry in Documentation/ABI/testing/sysfs-bus-iio about
> + *			details of the transformation operations.
>   * @debugfs_dentry:	[INTERN] device specific debugfs dentry.
>   * @cached_reg_addr:	[INTERN] cached register address for debugfs reads.
>   */
> @@ -509,6 +517,7 @@ struct iio_dev {
>  	int				groupcounter;
>  
>  	unsigned long			flags;
> +	int				mounting_matrix[IIO_MM_SIZE];
>  #if defined(CONFIG_DEBUG_FS)
>  	struct dentry			*debugfs_dentry;
>  	unsigned			cached_reg_addr;
> 

Re: [RFC PATCH 3/3] iio: derive the mounting matrix from ACPI _PLD objects

[Jonathan Cameron]

On 27/04/15 16:01, Octavian Purdila wrote:
> This patch derives the mounting matrix for a particular IIO device
> based ont the ACPI _PLD information. Note that if mounting matrix is
> defined in the device properties it overrieds the _PLD information.
> 
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
I have no objection to the functionality, but I'd prefer to separate it more
from the core of IIO.  By all means have some utility functions to help with
what may be a common feature in future, but I don't want to have the data
stored in the main IIO structures. It's not relevant to lots of our devices
afterall!

Reading this (still offline unfortunately), looks like the ACPI provided data
is very very minimalist.  Parts have to be on the panel and oriented at 90 
degree increments?  I suppose it covers a fair range of common hardware devices,
but far from all of them.

I'm not considerably more keen on the IIO interface as a substantial superset
of what ACPI is providing.  Note that we should definitely discuss with
input (joysticks can have mounting matrices too!)
cc'd.  Sorry Dmitry, think I managed to pick up an ancient version of your
email address for an earlier cc.  Was being lazy and didn't check in MAINTAINERS.

Jonathan

> ---
>  drivers/iio/industrialio-core.c | 51 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/iio/iio.h         | 46 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 97 insertions(+)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 9000c53..90ee58a 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -31,6 +31,7 @@
>  #include <linux/iio/sysfs.h>
>  #include <linux/iio/events.h>
>  #include <linux/iio/buffer.h>
> +#include <linux/acpi.h>
>  
>  /* IDA to assign each registered device a unique id */
>  static DEFINE_IDA(iio_ida);
> @@ -871,6 +872,53 @@ static ssize_t iio_show_dev_name(struct device *dev,
>  
>  static DEVICE_ATTR(name, S_IRUGO, iio_show_dev_name, NULL);
>  
> +#if IS_ENABLED(CONFIG_ACPI)
> +static bool iio_get_mounting_matrix_acpi(struct iio_dev *indio_dev)
> +{
> +	acpi_handle h = ACPI_HANDLE(indio_dev->dev.parent);
> +	struct acpi_pld_info *info;
> +
> +	if (ACPI_SUCCESS(acpi_get_physical_device_location(h, &info))) {
> +		IIO_MM_SET(indio_dev->mounting_matrix, X, X, 1, 0);
> +		IIO_MM_SET(indio_dev->mounting_matrix, Y, Y, 1, 0);
> +		IIO_MM_SET(indio_dev->mounting_matrix, Z, Z, 1, 0);
> +
> +		/* Chip placed on the back panel ; negate x and z */
> +		if (info->panel == ACPI_PLD_PANEL_BACK) {
> +			IIO_MM_MUL(indio_dev->mounting_matrix, X, X, -1, 0);
> +			IIO_MM_MUL(indio_dev->mounting_matrix, Z, Z, -1, 0);
> +		}
> +
> +		switch (info->rotation) {
> +		case 2:
> +			/* 90 deg clockwise: negate y then swap x,y */
> +			IIO_MM_MUL(indio_dev->mounting_matrix, Y, Y, -1, 0);
> +			IIO_MM_SWAP(indio_dev->mounting_matrix, X, Y);
> +			break;
> +		case 4:
> +			/* Upside down: negate x and y */
> +			IIO_MM_MUL(indio_dev->mounting_matrix, X, X, -1, 0);
> +			IIO_MM_MUL(indio_dev->mounting_matrix, Y, Y, -1, 0);
> +			break;
> +		case 6:
> +			/* 90 deg counter clockwise: negate x then swap x,y */
> +			IIO_MM_MUL(indio_dev->mounting_matrix, X, X, -1, 0);
> +			IIO_MM_SWAP(indio_dev->mounting_matrix, X, Y);
> +			break;
> +		}
> +
> +		return true;
> +	}
> +
> +	return false;
> +}
> +#else
> +static bool iio_get_mounting_matrix_acpi(struct iio_dev *indio_dev)
> +{
> +	return false;
> +}
> +#endif
> +
>  static bool iio_get_mounting_matrix(struct iio_dev *indio_dev)
>  {
>  	int i, err;
> @@ -888,6 +936,9 @@ static bool iio_get_mounting_matrix(struct iio_dev *indio_dev)
>  		return true;
>  	}
>  
> +	if (ACPI_HANDLE(indio_dev->dev.parent))
> +		return iio_get_mounting_matrix_acpi(indio_dev);
> +
>  	return false;
>  }
>  
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index c1fa852..feb7813 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -436,6 +436,52 @@ struct iio_buffer_setup_ops {
>  
>  #define IIO_MM_SIZE		18
>  
> +enum {
> +	IIO_MM_XX0,
> +	IIO_MM_XX1,
> +	IIO_MM_XY0,
> +	IIO_MM_XY1,
> +	IIO_MM_XZ0,
> +	IIO_MM_XZ1,
> +	IIO_MM_YX0,
> +	IIO_MM_YX1,
> +	IIO_MM_YY0,
> +	IIO_MM_YY1,
> +	IIO_MM_YZ0,
> +	IIO_MM_YZ1,
> +	IIO_MM_ZX0,
> +	IIO_MM_ZX1,
> +	IIO_MM_ZY0,
> +	IIO_MM_ZY1,
> +	IIO_MM_ZZ0,
> +	IIO_MM_ZZ1,
> +};
> +
> +#define IIO_MM_SET(mm, a, b, val0, val1)	\
> +	do {					\
> +		mm[IIO_MM_##a##b##0] = val0;	\
> +		mm[IIO_MM_##a##b##1] = val1;	\
> +	} while (0)				\
> +
> +#define IIO_MM_MUL(mm, a, b, val0, val1)	\
> +	do {					\
> +		mm[IIO_MM_##a##b##0] *= val0;	\
> +		mm[IIO_MM_##a##b##0] *= val1;	\
> +	} while (0)				\
> +
> +#define IIO_MM_SWAP(mm, x, y)					\
> +	do {							\
> +		int tmp;					\
> +								\
> +		tmp = mm[IIO_MM_##x##x##0];			\
> +		mm[IIO_MM_##x##x##0] = mm[IIO_MM_##y##y##0];	\
> +		mm[IIO_MM_##y##y##0] = tmp;			\
> +								\
> +		tmp = mm[IIO_MM_##x##x##1];			\
> +		mm[IIO_MM_##x##x##1] = mm[IIO_MM_##y##y##1];	\
> +		mm[IIO_MM_##y##y##1] = tmp;			\
> +	} while (0)						\
> +
>  /**
>   * struct iio_dev - industrial I/O device
>   * @id:			[INTERN] used to identify device internally
> 

Re: [RFC PATCH 3/3] iio: derive the mounting matrix from ACPI _PLD objects

[Srinivas Pandruvada]

On Mon, 2015-05-04 at 11:21 +0300, Octavian Purdila wrote:
> On Mon, May 4, 2015 at 4:11 AM, Sathyanarayanan Kuppuswamy
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> > Hi Octavian,
> >
> > On 04/27/2015 07:23 PM, Octavian Purdila wrote:
> >>
> >> On Tue, Apr 28, 2015 at 12:57 AM, sathyanarayanan kuppuswamy
> >> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> >>>
> >>> Hi
> >>>
> >>> On 04/27/2015 08:54 AM, Octavian Purdila wrote:
> >>>>
> >>>> On Mon, Apr 27, 2015 at 6:42 PM, Kuppuswamy Sathyanarayanan
> >>>> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> >>>>>
> >>>>> Since Acpi framework already exports this info to user space, Why not
> >>>>> do
> >>>>> this derivation in user space code ? Why do we need new ABI, if the
> >>>>> same
> >>>>> can be derived from existing one.
> >>>>>
> >>>> The ABI was added in the previous patch so that we can present the
> >>>> sensor orientation information to userspace even in the case of device
> >>>> tree.
> >>>
> >>> If the main reason for implementing a new ABI is to support DT platforms,
> >>> Why not implement a version of _PLD for device tree ? Don't you think it
> >>> would be much better than adding a new ABI to export redundant
> >>> information ?
> >>>
> >> IMO the mounting matrix is more consistent with the IIO ABIs. Although
> >> I have no issue with repicating _PLD for device tree if people agree
> >> that it is better.
> >
> > Since your main issue is, device tree lacking ABI to specify location
> > information, you should consider fixing it there. Let's wait for others
> > comment on this.
> >
> > If you think mounting matrix provides more information than what is
> > supported
> > by _PLD,  then we should consider implementing another ABI. AFAIK, that is
> > not
> > the case here.
> >
> > Adding adding a new ABI to represent the information that can be derived
> > from existing ABI does not seem to be useful.
> 
> AFAICS the ACPI _PLD information is not (yet) exported to userspace. This patch:
> 
> http://marc.info/?t=140795040700003&r=1&w=2
> 
> does not seem to be merged upstream. So there is no existing ABI to
> derive from :)
I don't think there is any major objection to this patch. The author
should just try and see if he can replace to device_* calls.

Thanks,
Srinivas

> 
> >>
> >>
> >>> Also the location information of the device is not just specific to iio
> >>> drivers. You should consider that we would have similar requirements for
> >>> devices implemented as input or platform drivers.
> >>
> >> The upstream standard for those sensors where the orientation matters
> >> (accelerometer, gyro, compass) is IIO.
> >>
> >> Granted, there are other device types for which the orientation
> >> information may be useful (e.g. camera). However the actual
> >> interpretation and action to be taken is different for each subsystem
> >> (e.g. in the camera case do the correction via V4L2_CID_HFLIP /
> >> V4L2_CID_VFLIP) so I think it is better to expose it at the subsystem
> >> level in a way consistent with the subsystem's ABIs.
> >
> > I agree that location information is used differently at different
> > sub systems. But my question is why we need  a new ABI ?
> >
> > Why not handle it in user space ?
> >
> > -

Re: [RFC PATCH 1/3] iio: export mounting matrix information

[Octavian Purdila]

On Mon, May 4, 2015 at 2:25 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 27/04/15 16:01, Octavian Purdila wrote:
>> Export a new sysfs attribute that describes the placement of the
>> sensor on the device in the form of a mounting matrix so that
>> userspace can do the required correction to get accurate data:
>>
>> corrected X = MM[0] * X + MM[1] Y + MM[2] * Z
>> corrected Y = MM[3] * X + MM[4] Y + MM[5] * Z
>> corrected Z = MM[6] * X + MM[7] Y + MM[8] * Z
>>
>> This information is read from the device properties of the parent of
>> the IIO device.
>>
>> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> Hmm.  This could be done with existing code and a new info_mask element.
> The disadvantage is you'd have to change the read_raw to read_raw_multi
> callback, but that's hardly a big job.  If I could be bothered, I'd
> move the whole subsystem over and drop read_raw entirely.
>
> It was introduced precisely for this sort of element.

If I understand this correctly this will require changing every driver
that want to expose this information. However, this information is
independent of the driver, as this information is read from the device
tree or ACPI. As such, doesn't this fits better in the IIO core?

> As I mentioned deeper in the thread.  If we are going to introduce a mounting
> matrix, I'd prefer a version that includes translation.  It might not matter
> to many devices but it has mattered a lot to me in the past and doesn't hurt
> others that much (I used to write papers on how to estimate this stuff
> from black box devices).

OK, I will look into that.


>> ---
>>  Documentation/ABI/testing/sysfs-bus-iio | 11 +++++++++
>>  drivers/iio/industrialio-core.c         | 42 +++++++++++++++++++++++++++++++++
>>  include/linux/iio/iio.h                 |  9 +++++++
>>  3 files changed, 62 insertions(+)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
>> index 866b4ec..d36476e 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-iio
>> +++ b/Documentation/ABI/testing/sysfs-bus-iio
>> @@ -1375,3 +1375,14 @@ Description:
>>               The emissivity ratio of the surface in the field of view of the
>>               contactless temperature sensor.  Emissivity varies from 0 to 1,
>>               with 1 being the emissivity of a black body.
>> +
>> +What:                /sys/bus/iio/devices/iio:deviceX/mounting_matrix
>> +KernelVersion:       4.2
>> +Contact:     linux-iio@vger.kernel.org
>> +Description:
>> +             A list of 9 floating point values that describes the
>> +             transformation needed to account for the way the sensor has been
>> +             placed on the PCB:
>> +             corrected X = MM[0] * X + MM[1] Y + MM[2] * Z
>> +             corrected Y = MM[3] * X + MM[4] Y + MM[5] * Z
>> +             corrected Z = MM[6] * X + MM[7] Y + MM[8] * Z
>> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
>> index 7c98bc1..9000c53 100644
>> --- a/drivers/iio/industrialio-core.c
>> +++ b/drivers/iio/industrialio-core.c
>> @@ -871,6 +871,45 @@ static ssize_t iio_show_dev_name(struct device *dev,
>>
>>  static DEVICE_ATTR(name, S_IRUGO, iio_show_dev_name, NULL);
>>
>> +static bool iio_get_mounting_matrix(struct iio_dev *indio_dev)
>> +{
>> +     int i, err;
>> +
>> +     err = device_property_read_u32_array(&indio_dev->dev, "mounting-matrix",
>> +                                          indio_dev->mounting_matrix, 9);
>> +     if (!err) {
>> +             int *mm = indio_dev->mounting_matrix;
>> +
>> +             for (i = IIO_MM_SIZE - 2; i > 0; i -= 2) {
>> +                     mm[i] = mm[i / 2];
>> +                     mm[i + 1] = 0;
>> +             }
>> +
>> +             return true;
>> +     }
>> +
>> +     return false;
>> +}
>> +
>> +static ssize_t iio_show_mounting_matrix(struct device *dev,
>> +                                     struct device_attribute *attr,
>> +                                     char *buf)
>> +{
>> +     struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>> +     char *tmp = buf;
>> +     int i;
>> +
>> +     for (i = 0; i < IIO_MM_SIZE; i += 2) {
>> +             tmp += iio_format_value(tmp, IIO_VAL_INT_PLUS_MICRO, 2,
>> +                                     &indio_dev->mounting_matrix[i]);
>> +             if (((i + 2) / 2) % 3)
> Hmm. if (i != 8) should also do the job with slightly less feedling of
> magic..
>> +                     tmp[-1] = ' ';
>> +     }
>> +     return tmp - buf;
>> +}
>> +
>> +static DEVICE_ATTR(mounting_matrix, S_IRUGO, iio_show_mounting_matrix, NULL);
>> +
>>  static int iio_device_register_sysfs(struct iio_dev *indio_dev)
>>  {
>>       int i, ret = 0, attrcount, attrn, attrcount_orig = 0;
>> @@ -920,6 +959,9 @@ static int iio_device_register_sysfs(struct iio_dev *indio_dev)
>>               indio_dev->chan_attr_group.attrs[attrn++] = &p->dev_attr.attr;
>>       if (indio_dev->name)
>>               indio_dev->chan_attr_group.attrs[attrn++] = &dev_attr_name.attr;
>> +     if (iio_get_mounting_matrix(indio_dev))
>> +             indio_dev->chan_attr_group.attrs[attrn++] =
>> +                     &dev_attr_mounting_matrix.attr;
>>
>>       indio_dev->groups[indio_dev->groupcounter++] =
>>               &indio_dev->chan_attr_group;
>> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
>> index b1e46ae..c1fa852 100644
>> --- a/include/linux/iio/iio.h
>> +++ b/include/linux/iio/iio.h
>> @@ -434,6 +434,8 @@ struct iio_buffer_setup_ops {
>>                                  const unsigned long *scan_mask);
>>  };
>>
>> +#define IIO_MM_SIZE          18
>> +
>>  /**
>>   * struct iio_dev - industrial I/O device
>>   * @id:                      [INTERN] used to identify device internally
>> @@ -469,6 +471,12 @@ struct iio_buffer_setup_ops {
>>   * @groups:          [INTERN] attribute groups
>>   * @groupcounter:    [INTERN] index of next attribute group
>>   * @flags:           [INTERN] file ops related flags including busy flag.
> Hmm. No absolute need to have this new data in the iio_dev.  I'd do it
> via the read_raw_multi callback, though this will require converting read_raw
> over to that more flexible version.
>> + * @mounting_matrix: [INTERN] the mounting matrix is a series of 9
>> + *                   IIO_VAL_INT_PLUS_MICRO pairs that describes the
>> + *                   transformation needed to account for the way the sensor
>> + *                   has been placed on the PCB. See the mounting_matrix
>> + *                   entry in Documentation/ABI/testing/sysfs-bus-iio about
>> + *                   details of the transformation operations.
>>   * @debugfs_dentry:  [INTERN] device specific debugfs dentry.
>>   * @cached_reg_addr: [INTERN] cached register address for debugfs reads.
>>   */
>> @@ -509,6 +517,7 @@ struct iio_dev {
>>       int                             groupcounter;
>>
>>       unsigned long                   flags;
>> +     int                             mounting_matrix[IIO_MM_SIZE];
>>  #if defined(CONFIG_DEBUG_FS)
>>       struct dentry                   *debugfs_dentry;
>>       unsigned                        cached_reg_addr;
>>
>

Re: [RFC PATCH 1/3] iio: export mounting matrix information

[Jonathan Cameron]

On 4 May 2015 18:48:23 GMT+01:00, Octavian Purdila <octavian.purdila@intel.com> wrote:
>On Mon, May 4, 2015 at 2:25 PM, Jonathan Cameron <jic23@kernel.org>
>wrote:
>> On 27/04/15 16:01, Octavian Purdila wrote:
>>> Export a new sysfs attribute that describes the placement of the
>>> sensor on the device in the form of a mounting matrix so that
>>> userspace can do the required correction to get accurate data:
>>>
>>> corrected X = MM[0] * X + MM[1] Y + MM[2] * Z
>>> corrected Y = MM[3] * X + MM[4] Y + MM[5] * Z
>>> corrected Z = MM[6] * X + MM[7] Y + MM[8] * Z
>>>
>>> This information is read from the device properties of the parent of
>>> the IIO device.
>>>
>>> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
>> Hmm.  This could be done with existing code and a new info_mask
>element.
>> The disadvantage is you'd have to change the read_raw to
>read_raw_multi
>> callback, but that's hardly a big job.  If I could be bothered, I'd
>> move the whole subsystem over and drop read_raw entirely.
>>
>> It was introduced precisely for this sort of element.
>
>If I understand this correctly this will require changing every driver
>that want to expose this information. However, this information is
>independent of the driver, as this information is read from the device
>tree or ACPI. As such, doesn't this fits better in the IIO core?
In this case it comes from one of those two but there is no inherent reason it won't come from elsewhere.

Take for example an IMU block with integrated components. The mounting
 matrix is then partly provided by acpi say and partly by values read from the device.

Yes moving to read_raw_multi involves a driver change but that is no bad thing anyway and the change is fairly trivial.
>
>> As I mentioned deeper in the thread.  If we are going to introduce a
>mounting
>> matrix, I'd prefer a version that includes translation.  It might not
>matter
>> to many devices but it has mattered a lot to me in the past and
>doesn't hurt
>> others that much (I used to write papers on how to estimate this
>stuff
>> from black box devices).
>
>OK, I will look into that.
>
>
>>> ---
>>>  Documentation/ABI/testing/sysfs-bus-iio | 11 +++++++++
>>>  drivers/iio/industrialio-core.c         | 42
>+++++++++++++++++++++++++++++++++
>>>  include/linux/iio/iio.h                 |  9 +++++++
>>>  3 files changed, 62 insertions(+)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio
>b/Documentation/ABI/testing/sysfs-bus-iio
>>> index 866b4ec..d36476e 100644
>>> --- a/Documentation/ABI/testing/sysfs-bus-iio
>>> +++ b/Documentation/ABI/testing/sysfs-bus-iio
>>> @@ -1375,3 +1375,14 @@ Description:
>>>               The emissivity ratio of the surface in the field of
>view of the
>>>               contactless temperature sensor.  Emissivity varies
>from 0 to 1,
>>>               with 1 being the emissivity of a black body.
>>> +
>>> +What:               
>/sys/bus/iio/devices/iio:deviceX/mounting_matrix
>>> +KernelVersion:       4.2
>>> +Contact:     linux-iio@vger.kernel.org
>>> +Description:
>>> +             A list of 9 floating point values that describes the
>>> +             transformation needed to account for the way the
>sensor has been
>>> +             placed on the PCB:
>>> +             corrected X = MM[0] * X + MM[1] Y + MM[2] * Z
>>> +             corrected Y = MM[3] * X + MM[4] Y + MM[5] * Z
>>> +             corrected Z = MM[6] * X + MM[7] Y + MM[8] * Z
>>> diff --git a/drivers/iio/industrialio-core.c
>b/drivers/iio/industrialio-core.c
>>> index 7c98bc1..9000c53 100644
>>> --- a/drivers/iio/industrialio-core.c
>>> +++ b/drivers/iio/industrialio-core.c
>>> @@ -871,6 +871,45 @@ static ssize_t iio_show_dev_name(struct device
>*dev,
>>>
>>>  static DEVICE_ATTR(name, S_IRUGO, iio_show_dev_name, NULL);
>>>
>>> +static bool iio_get_mounting_matrix(struct iio_dev *indio_dev)
>>> +{
>>> +     int i, err;
>>> +
>>> +     err = device_property_read_u32_array(&indio_dev->dev,
>"mounting-matrix",
>>> +                                         
>indio_dev->mounting_matrix, 9);
>>> +     if (!err) {
>>> +             int *mm = indio_dev->mounting_matrix;
>>> +
>>> +             for (i = IIO_MM_SIZE - 2; i > 0; i -= 2) {
>>> +                     mm[i] = mm[i / 2];
>>> +                     mm[i + 1] = 0;
>>> +             }
>>> +
>>> +             return true;
>>> +     }
>>> +
>>> +     return false;
>>> +}
>>> +
>>> +static ssize_t iio_show_mounting_matrix(struct device *dev,
>>> +                                     struct device_attribute *attr,
>>> +                                     char *buf)
>>> +{
>>> +     struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> +     char *tmp = buf;
>>> +     int i;
>>> +
>>> +     for (i = 0; i < IIO_MM_SIZE; i += 2) {
>>> +             tmp += iio_format_value(tmp, IIO_VAL_INT_PLUS_MICRO,
>2,
>>> +                                    
>&indio_dev->mounting_matrix[i]);
>>> +             if (((i + 2) / 2) % 3)
>> Hmm. if (i != 8) should also do the job with slightly less feedling
>of
>> magic..
>>> +                     tmp[-1] = ' ';
>>> +     }
>>> +     return tmp - buf;
>>> +}
>>> +
>>> +static DEVICE_ATTR(mounting_matrix, S_IRUGO,
>iio_show_mounting_matrix, NULL);
>>> +
>>>  static int iio_device_register_sysfs(struct iio_dev *indio_dev)
>>>  {
>>>       int i, ret = 0, attrcount, attrn, attrcount_orig = 0;
>>> @@ -920,6 +959,9 @@ static int iio_device_register_sysfs(struct
>iio_dev *indio_dev)
>>>               indio_dev->chan_attr_group.attrs[attrn++] =
>&p->dev_attr.attr;
>>>       if (indio_dev->name)
>>>               indio_dev->chan_attr_group.attrs[attrn++] =
>&dev_attr_name.attr;
>>> +     if (iio_get_mounting_matrix(indio_dev))
>>> +             indio_dev->chan_attr_group.attrs[attrn++] =
>>> +                     &dev_attr_mounting_matrix.attr;
>>>
>>>       indio_dev->groups[indio_dev->groupcounter++] =
>>>               &indio_dev->chan_attr_group;
>>> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
>>> index b1e46ae..c1fa852 100644
>>> --- a/include/linux/iio/iio.h
>>> +++ b/include/linux/iio/iio.h
>>> @@ -434,6 +434,8 @@ struct iio_buffer_setup_ops {
>>>                                  const unsigned long *scan_mask);
>>>  };
>>>
>>> +#define IIO_MM_SIZE          18
>>> +
>>>  /**
>>>   * struct iio_dev - industrial I/O device
>>>   * @id:                      [INTERN] used to identify device
>internally
>>> @@ -469,6 +471,12 @@ struct iio_buffer_setup_ops {
>>>   * @groups:          [INTERN] attribute groups
>>>   * @groupcounter:    [INTERN] index of next attribute group
>>>   * @flags:           [INTERN] file ops related flags including busy
>flag.
>> Hmm. No absolute need to have this new data in the iio_dev.  I'd do
>it
>> via the read_raw_multi callback, though this will require converting
>read_raw
>> over to that more flexible version.
>>> + * @mounting_matrix: [INTERN] the mounting matrix is a series of 9
>>> + *                   IIO_VAL_INT_PLUS_MICRO pairs that describes
>the
>>> + *                   transformation needed to account for the way
>the sensor
>>> + *                   has been placed on the PCB. See the
>mounting_matrix
>>> + *                   entry in
>Documentation/ABI/testing/sysfs-bus-iio about
>>> + *                   details of the transformation operations.
>>>   * @debugfs_dentry:  [INTERN] device specific debugfs dentry.
>>>   * @cached_reg_addr: [INTERN] cached register address for debugfs
>reads.
>>>   */
>>> @@ -509,6 +517,7 @@ struct iio_dev {
>>>       int                             groupcounter;
>>>
>>>       unsigned long                   flags;
>>> +     int                             mounting_matrix[IIO_MM_SIZE];
>>>  #if defined(CONFIG_DEBUG_FS)
>>>       struct dentry                   *debugfs_dentry;
>>>       unsigned                        cached_reg_addr;
>>>
>>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.