[PATCH v2 0/5] iio:magnetometer:ak8975: fix and enhancements

[PATCH v2 0/5] iio:magnetometer:ak8975: fix and enhancements

[Gregor Boirie]

Various fixes and enhancements for the ak8975 magnetometers family driver.

Changes since v1:
* get rid of silly le16 to cpu conversion since SMBUS handles this for us when
  reading words (patch 3/6)
* get rid of axes caching and serialize bus access between trigger handler and
  read_raw
* pack triggered buffer sampling bus accesses into a single SMBUS data block
  access
* use channel address within read_raw

Gregor.

Gregor Boirie (5):
  iio:magnetometer:ak8975: fix uninitialized chipset
  iio:magnetometer:ak8975: remove unused field
  iio:magnetometer:ak8975: power regulator support
  iio:magnetometer:ak8975: mounting matrix support
  iio:magnetometer:ak8975: triggered buffer support

 .../bindings/iio/magnetometer/ak8975.txt           |  12 ++
 drivers/iio/magnetometer/Kconfig                   |   2 +
 drivers/iio/magnetometer/ak8975.c                  | 218 ++++++++++++++++++---
 3 files changed, 202 insertions(+), 30 deletions(-)

-- 
2.1.4

[PATCH v2 1/5] iio:magnetometer:ak8975: fix uninitialized chipset

[Gregor Boirie]

ak_def_array bounds are not properly checked in case of ACPI matching
failure. GCC warns with the following message at line 799:
‘chipset’ may be used uninitialized in this function.

Signed-off-by: Gregor Boirie <gregor.boirie@parrot.com>
---
 drivers/iio/magnetometer/ak8975.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
index 9c5c9ef..11059b2 100644
--- a/drivers/iio/magnetometer/ak8975.c
+++ b/drivers/iio/magnetometer/ak8975.c
@@ -774,8 +774,11 @@ static int ak8975_probe(struct i2c_client *client,
 	if (id) {
 		chipset = (enum asahi_compass_chipset)(id->driver_data);
 		name = id->name;
-	} else if (ACPI_HANDLE(&client->dev))
+	} else if (ACPI_HANDLE(&client->dev)) {
 		name = ak8975_match_acpi_device(&client->dev, &chipset);
+		if (!name)
+			return -ENODEV;
+	}
 	else
 		return -ENOSYS;
 
-- 
2.1.4

Re: [PATCH v2 1/5] iio:magnetometer:ak8975: fix uninitialized chipset

[Jonathan Cameron]

On 03/03/16 10:44, Gregor Boirie wrote:
> ak_def_array bounds are not properly checked in case of ACPI matching
> failure. GCC warns with the following message at line 799:
> ‘chipset’ may be used uninitialized in this function.
> 
> Signed-off-by: Gregor Boirie <gregor.boirie@parrot.com>
Applied.
> ---
>  drivers/iio/magnetometer/ak8975.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
> index 9c5c9ef..11059b2 100644
> --- a/drivers/iio/magnetometer/ak8975.c
> +++ b/drivers/iio/magnetometer/ak8975.c
> @@ -774,8 +774,11 @@ static int ak8975_probe(struct i2c_client *client,
>  	if (id) {
>  		chipset = (enum asahi_compass_chipset)(id->driver_data);
>  		name = id->name;
> -	} else if (ACPI_HANDLE(&client->dev))
> +	} else if (ACPI_HANDLE(&client->dev)) {
>  		name = ak8975_match_acpi_device(&client->dev, &chipset);
> +		if (!name)
> +			return -ENODEV;
> +	}
>  	else
>  		return -ENOSYS;
>  
> 

[PATCH v2 2/5] iio:magnetometer:ak8975: remove unused field

[Gregor Boirie]

Remove unused struct ak8975_data attrs field.

Signed-off-by: Gregor Boirie <gregor.boirie@parrot.com>
---
 drivers/iio/magnetometer/ak8975.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
index 11059b2..896b13e3 100644
--- a/drivers/iio/magnetometer/ak8975.c
+++ b/drivers/iio/magnetometer/ak8975.c
@@ -361,7 +361,6 @@ static const struct ak_def ak_def_array[AK_MAX_TYPE] = {
 struct ak8975_data {
 	struct i2c_client	*client;
 	const struct ak_def	*def;
-	struct attribute_group	attrs;
 	struct mutex		lock;
 	u8			asa[3];
 	long			raw_to_gauss[3];
-- 
2.1.4

Re: [PATCH v2 2/5] iio:magnetometer:ak8975: remove unused field

[Jonathan Cameron]

On 03/03/16 10:44, Gregor Boirie wrote:
> Remove unused struct ak8975_data attrs field.
> 
> Signed-off-by: Gregor Boirie <gregor.boirie@parrot.com>
Applied.
> ---
>  drivers/iio/magnetometer/ak8975.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
> index 11059b2..896b13e3 100644
> --- a/drivers/iio/magnetometer/ak8975.c
> +++ b/drivers/iio/magnetometer/ak8975.c
> @@ -361,7 +361,6 @@ static const struct ak_def ak_def_array[AK_MAX_TYPE] = {
>  struct ak8975_data {
>  	struct i2c_client	*client;
>  	const struct ak_def	*def;
> -	struct attribute_group	attrs;
>  	struct mutex		lock;
>  	u8			asa[3];
>  	long			raw_to_gauss[3];
> 

[PATCH v2 3/5] iio:magnetometer:ak8975: power regulator support

[Gregor Boirie]

Add support for an optional regulator which, if found into device-tree,
will power on device at probing time.
The regulator is declared into ak8975 DTS entry as a "vdd-supply" property.

Signed-off-by: Gregor Boirie <gregor.boirie@parrot.com>
---
 .../devicetree/bindings/iio/magnetometer/ak8975.txt      |  2 ++
 drivers/iio/magnetometer/ak8975.c                        | 16 ++++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/magnetometer/ak8975.txt b/Documentation/devicetree/bindings/iio/magnetometer/ak8975.txt
index 011679f..34a3206 100644
--- a/Documentation/devicetree/bindings/iio/magnetometer/ak8975.txt
+++ b/Documentation/devicetree/bindings/iio/magnetometer/ak8975.txt
@@ -8,6 +8,7 @@ Required properties:
 Optional properties:
 
   - gpios : should be device tree identifier of the magnetometer DRDY pin
+  - vdd-supply: an optional regulator that needs to be on to provide VDD
 
 Example:
 
@@ -15,4 +16,5 @@ ak8975@0c {
         compatible = "asahi-kasei,ak8975";
         reg = <0x0c>;
         gpios = <&gpj0 7 0>;
+        vdd-supply = <&ldo_3v3_gnss>;
 };
diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
index 896b13e3..72c03d9 100644
--- a/drivers/iio/magnetometer/ak8975.c
+++ b/drivers/iio/magnetometer/ak8975.c
@@ -32,6 +32,7 @@
 #include <linux/gpio.h>
 #include <linux/of_gpio.h>
 #include <linux/acpi.h>
+#include <linux/regulator/consumer.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
@@ -379,8 +380,23 @@ static int ak8975_who_i_am(struct i2c_client *client,
 			   enum asahi_compass_chipset type)
 {
 	u8 wia_val[2];
+	struct regulator *vdd = devm_regulator_get_optional(&client->dev,
+							    "vdd");
 	int ret;
 
+	/* Enable attached regulator if any. */
+	if (!IS_ERR(vdd)) {
+		ret = regulator_enable(vdd);
+		if (ret) {
+			dev_err(&client->dev, "Failed to enable Vdd supply\n");
+			return ret;
+		}
+	} else {
+		ret = PTR_ERR(vdd);
+		if (ret != -ENODEV)
+			return ret;
+	}
+
 	/*
 	 * Signature for each device:
 	 * Device   |  WIA1      |  WIA2
-- 
2.1.4

Re: [PATCH v2 3/5] iio:magnetometer:ak8975: power regulator support

[Jonathan Cameron]

On 03/03/16 10:44, Gregor Boirie wrote:
> Add support for an optional regulator which, if found into device-tree,
> will power on device at probing time.
> The regulator is declared into ak8975 DTS entry as a "vdd-supply" property.
> 
> Signed-off-by: Gregor Boirie <gregor.boirie@parrot.com>
Applied.
> ---
>  .../devicetree/bindings/iio/magnetometer/ak8975.txt      |  2 ++
>  drivers/iio/magnetometer/ak8975.c                        | 16 ++++++++++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/magnetometer/ak8975.txt b/Documentation/devicetree/bindings/iio/magnetometer/ak8975.txt
> index 011679f..34a3206 100644
> --- a/Documentation/devicetree/bindings/iio/magnetometer/ak8975.txt
> +++ b/Documentation/devicetree/bindings/iio/magnetometer/ak8975.txt
> @@ -8,6 +8,7 @@ Required properties:
>  Optional properties:
>  
>    - gpios : should be device tree identifier of the magnetometer DRDY pin
> +  - vdd-supply: an optional regulator that needs to be on to provide VDD
>  
>  Example:
>  
> @@ -15,4 +16,5 @@ ak8975@0c {
>          compatible = "asahi-kasei,ak8975";
>          reg = <0x0c>;
>          gpios = <&gpj0 7 0>;
> +        vdd-supply = <&ldo_3v3_gnss>;
>  };
> diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
> index 896b13e3..72c03d9 100644
> --- a/drivers/iio/magnetometer/ak8975.c
> +++ b/drivers/iio/magnetometer/ak8975.c
> @@ -32,6 +32,7 @@
>  #include <linux/gpio.h>
>  #include <linux/of_gpio.h>
>  #include <linux/acpi.h>
> +#include <linux/regulator/consumer.h>
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> @@ -379,8 +380,23 @@ static int ak8975_who_i_am(struct i2c_client *client,
>  			   enum asahi_compass_chipset type)
>  {
>  	u8 wia_val[2];
> +	struct regulator *vdd = devm_regulator_get_optional(&client->dev,
> +							    "vdd");
>  	int ret;
>  
> +	/* Enable attached regulator if any. */
> +	if (!IS_ERR(vdd)) {
> +		ret = regulator_enable(vdd);
> +		if (ret) {
> +			dev_err(&client->dev, "Failed to enable Vdd supply\n");
> +			return ret;
> +		}
> +	} else {
> +		ret = PTR_ERR(vdd);
> +		if (ret != -ENODEV)
> +			return ret;
> +	}
> +
>  	/*
>  	 * Signature for each device:
>  	 * Device   |  WIA1      |  WIA2
> 

[PATCH v2 4/5] iio:magnetometer:ak8975: mounting matrix support

[Gregor Boirie]

Expose a rotation matrix to indicate userspace the chip placement with
respect to the overall hardware system. This is needed to adjust
coordinates sampled from a magnetometer chip when its position deviates
from the main hardware system.

Final coordinates computation is delegated to userspace since:
* computation may involve floating point arithmetics ;
* it allows an application to combine adjustments with arbitrary
  transformations.

This 3 dimentional space rotation matrix is expressed as 3x3 array of
strings to support floating point numbers. It may be retrieved from a
"in_magn_matrix" sysfs attribute file. It is declared into ak8975 DTS
entry as a "matrix" property.

Signed-off-by: Gregor Boirie <gregor.boirie@parrot.com>
---
 .../bindings/iio/magnetometer/ak8975.txt           | 10 +++++
 drivers/iio/magnetometer/ak8975.c                  | 46 +++++++++++++++++++++-
 2 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/iio/magnetometer/ak8975.txt b/Documentation/devicetree/bindings/iio/magnetometer/ak8975.txt
index 34a3206..f936f86 100644
--- a/Documentation/devicetree/bindings/iio/magnetometer/ak8975.txt
+++ b/Documentation/devicetree/bindings/iio/magnetometer/ak8975.txt
@@ -9,6 +9,7 @@ Optional properties:
 
   - gpios : should be device tree identifier of the magnetometer DRDY pin
   - vdd-supply: an optional regulator that needs to be on to provide VDD
+  - matrix: an optional 3x3 mounting rotation matrix
 
 Example:
 
@@ -17,4 +18,13 @@ ak8975@0c {
         reg = <0x0c>;
         gpios = <&gpj0 7 0>;
         vdd-supply = <&ldo_3v3_gnss>;
+        matrix = "-0.984807753012208",  /* x0 */
+                 "0",                   /* y0 */
+                 "-0.173648177666930",  /* z0 */
+                 "0",                   /* x1 */
+                 "-1",                  /* y1 */
+                 "0",                   /* z1 */
+                 "-0.173648177666930",  /* x2 */
+                 "0",                   /* y2 */
+                 "0.984807753012208";   /* z2 */
 };
diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
index 72c03d9..95c68952 100644
--- a/drivers/iio/magnetometer/ak8975.c
+++ b/drivers/iio/magnetometer/ak8975.c
@@ -370,6 +370,7 @@ struct ak8975_data {
 	wait_queue_head_t	data_ready_queue;
 	unsigned long		flags;
 	u8			cntl_cache;
+	const char		*matrix[9];
 };
 
 /*
@@ -697,6 +698,28 @@ static int ak8975_read_raw(struct iio_dev *indio_dev,
 	return -EINVAL;
 }
 
+static ssize_t ak8975_show_matrix(struct device *dev,
+				  struct device_attribute *attr,
+				  char *buf)
+{
+	const struct ak8975_data *data = iio_priv(dev_to_iio_dev(dev));
+	const char * const *m = data->matrix;
+
+	return sprintf(buf, "%s, %s, %s; %s, %s, %s; %s, %s, %s\n",
+		       m[0], m[1], m[2], m[3], m[4], m[5], m[6], m[7], m[8]);
+}
+
+static IIO_DEVICE_ATTR(in_magn_matrix, S_IRUGO, ak8975_show_matrix, NULL, -1);
+
+static struct attribute *ak8975_attrs[] = {
+	&iio_dev_attr_in_magn_matrix.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group ak8975_attrs_group = {
+	.attrs = ak8975_attrs
+};
+
 #define AK8975_CHANNEL(axis, index)					\
 	{								\
 		.type = IIO_MAGN,					\
@@ -716,6 +739,12 @@ static const struct iio_info ak8975_info = {
 	.driver_module = THIS_MODULE,
 };
 
+static const struct iio_info ak8975_matrix_info = {
+	.read_raw = &ak8975_read_raw,
+	.attrs = &ak8975_attrs_group,
+	.driver_module = THIS_MODULE,
+};
+
 static const struct acpi_device_id ak_acpi_match[] = {
 	{"AK8975", AK8975},
 	{"AK8963", AK8963},
@@ -785,6 +814,22 @@ static int ak8975_probe(struct i2c_client *client,
 	data->eoc_gpio = eoc_gpio;
 	data->eoc_irq = 0;
 
+	/*
+	 * Rotation matrix is expressed as an array of 3x3 strings to be able
+	 * to represent floating point numbers.
+	 */
+	err = of_property_read_string_array(client->dev.of_node, "matrix",
+					    data->matrix,
+					    ARRAY_SIZE(data->matrix));
+	if (err == ARRAY_SIZE(data->matrix))
+		indio_dev->info = &ak8975_matrix_info;
+	else if (err == -EINVAL)
+		indio_dev->info = &ak8975_info;
+	else if (err >= 0)
+		return -EINVAL;
+	else
+		return err;
+
 	/* id will be NULL when enumerated via ACPI */
 	if (id) {
 		chipset = (enum asahi_compass_chipset)(id->driver_data);
@@ -822,7 +867,6 @@ static int ak8975_probe(struct i2c_client *client,
 	indio_dev->dev.parent = &client->dev;
 	indio_dev->channels = ak8975_channels;
 	indio_dev->num_channels = ARRAY_SIZE(ak8975_channels);
-	indio_dev->info = &ak8975_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->name = name;
 	return devm_iio_device_register(&client->dev, indio_dev);
-- 
2.1.4

Re: [PATCH v2 4/5] iio:magnetometer:ak8975: mounting matrix support

[Jonathan Cameron]

On 03/03/16 10:44, Gregor Boirie wrote:
> Expose a rotation matrix to indicate userspace the chip placement with
> respect to the overall hardware system. This is needed to adjust
> coordinates sampled from a magnetometer chip when its position deviates
> from the main hardware system.
> 
> Final coordinates computation is delegated to userspace since:
> * computation may involve floating point arithmetics ;
> * it allows an application to combine adjustments with arbitrary
>   transformations.
> 
> This 3 dimentional space rotation matrix is expressed as 3x3 array of
> strings to support floating point numbers. It may be retrieved from a
> "in_magn_matrix" sysfs attribute file. It is declared into ak8975 DTS
> entry as a "matrix" property.
> 
> Signed-off-by: Gregor Boirie <gregor.boirie@parrot.com>
This one definitely wants to go to the device tree list. I'd also like a few
IIO related comments as well. I was sure we'd been through this before and
ended up with some sort of solution, but now I can't find it... Oh well.

Definitely needs ABI docs in Documentation/ABI/testing/sysfs-bus-iio
as well.  We need the reference directions to be well specified as
well - how do we define the main hardware orientation?  on yours presumably
the usual approach of using the screen is irrelevant so that definition
could be a little 'interesting' to write down?

I'm also unsure of whether the right thing to do in the 'not provided'
cases is to not export the setting, or to assume that the chip is at some
'standard' orientation and provide that?

Jonathan

> ---
>  .../bindings/iio/magnetometer/ak8975.txt           | 10 +++++
>  drivers/iio/magnetometer/ak8975.c                  | 46 +++++++++++++++++++++-
>  2 files changed, 55 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/magnetometer/ak8975.txt b/Documentation/devicetree/bindings/iio/magnetometer/ak8975.txt
> index 34a3206..f936f86 100644
> --- a/Documentation/devicetree/bindings/iio/magnetometer/ak8975.txt
> +++ b/Documentation/devicetree/bindings/iio/magnetometer/ak8975.txt
> @@ -9,6 +9,7 @@ Optional properties:
>  
>    - gpios : should be device tree identifier of the magnetometer DRDY pin
>    - vdd-supply: an optional regulator that needs to be on to provide VDD
> +  - matrix: an optional 3x3 mounting rotation matrix
>  
>  Example:
>  
> @@ -17,4 +18,13 @@ ak8975@0c {
>          reg = <0x0c>;
>          gpios = <&gpj0 7 0>;
>          vdd-supply = <&ldo_3v3_gnss>;
> +        matrix = "-0.984807753012208",  /* x0 */
> +                 "0",                   /* y0 */
> +                 "-0.173648177666930",  /* z0 */
> +                 "0",                   /* x1 */
> +                 "-1",                  /* y1 */
> +                 "0",                   /* z1 */
> +                 "-0.173648177666930",  /* x2 */
> +                 "0",                   /* y2 */
> +                 "0.984807753012208";   /* z2 */
>  };
> diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
> index 72c03d9..95c68952 100644
> --- a/drivers/iio/magnetometer/ak8975.c
> +++ b/drivers/iio/magnetometer/ak8975.c
> @@ -370,6 +370,7 @@ struct ak8975_data {
>  	wait_queue_head_t	data_ready_queue;
>  	unsigned long		flags;
>  	u8			cntl_cache;
> +	const char		*matrix[9];
>  };
>  
>  /*
> @@ -697,6 +698,28 @@ static int ak8975_read_raw(struct iio_dev *indio_dev,
>  	return -EINVAL;
>  }
>  
> +static ssize_t ak8975_show_matrix(struct device *dev,
> +				  struct device_attribute *attr,
> +				  char *buf)
> +{
> +	const struct ak8975_data *data = iio_priv(dev_to_iio_dev(dev));
> +	const char * const *m = data->matrix;
> +
> +	return sprintf(buf, "%s, %s, %s; %s, %s, %s; %s, %s, %s\n",
> +		       m[0], m[1], m[2], m[3], m[4], m[5], m[6], m[7], m[8]);
> +}
> +
> +static IIO_DEVICE_ATTR(in_magn_matrix, S_IRUGO, ak8975_show_matrix, NULL, -1);
> +
> +static struct attribute *ak8975_attrs[] = {
> +	&iio_dev_attr_in_magn_matrix.dev_attr.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group ak8975_attrs_group = {
> +	.attrs = ak8975_attrs
> +};
> +
>  #define AK8975_CHANNEL(axis, index)					\
>  	{								\
>  		.type = IIO_MAGN,					\
> @@ -716,6 +739,12 @@ static const struct iio_info ak8975_info = {
>  	.driver_module = THIS_MODULE,
>  };
>  
> +static const struct iio_info ak8975_matrix_info = {
> +	.read_raw = &ak8975_read_raw,
> +	.attrs = &ak8975_attrs_group,
> +	.driver_module = THIS_MODULE,
> +};
> +
>  static const struct acpi_device_id ak_acpi_match[] = {
>  	{"AK8975", AK8975},
>  	{"AK8963", AK8963},
> @@ -785,6 +814,22 @@ static int ak8975_probe(struct i2c_client *client,
>  	data->eoc_gpio = eoc_gpio;
>  	data->eoc_irq = 0;
>  
> +	/*
> +	 * Rotation matrix is expressed as an array of 3x3 strings to be able
> +	 * to represent floating point numbers.
> +	 */
> +	err = of_property_read_string_array(client->dev.of_node, "matrix",
> +					    data->matrix,
> +					    ARRAY_SIZE(data->matrix));
> +	if (err == ARRAY_SIZE(data->matrix))
> +		indio_dev->info = &ak8975_matrix_info;
> +	else if (err == -EINVAL)
> +		indio_dev->info = &ak8975_info;
> +	else if (err >= 0)
> +		return -EINVAL;
> +	else
> +		return err;
> +
>  	/* id will be NULL when enumerated via ACPI */
>  	if (id) {
>  		chipset = (enum asahi_compass_chipset)(id->driver_data);
> @@ -822,7 +867,6 @@ static int ak8975_probe(struct i2c_client *client,
>  	indio_dev->dev.parent = &client->dev;
>  	indio_dev->channels = ak8975_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(ak8975_channels);
> -	indio_dev->info = &ak8975_info;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->name = name;
>  	return devm_iio_device_register(&client->dev, indio_dev);
> 

Re: [PATCH v2 4/5] iio:magnetometer:ak8975: mounting matrix support

[Gregor Boirie]

On 03/05/2016 04:36 PM, Jonathan Cameron wrote:
> On 03/03/16 10:44, Gregor Boirie wrote:
>> Expose a rotation matrix to indicate userspace the chip placement with
>> respect to the overall hardware system. This is needed to adjust
>> coordinates sampled from a magnetometer chip when its position deviates
>> from the main hardware system.
>>
>> Final coordinates computation is delegated to userspace since:
>> * computation may involve floating point arithmetics ;
>> * it allows an application to combine adjustments with arbitrary
>>    transformations.
>>
>> This 3 dimentional space rotation matrix is expressed as 3x3 array of
>> strings to support floating point numbers. It may be retrieved from a
>> "in_magn_matrix" sysfs attribute file. It is declared into ak8975 DTS
>> entry as a "matrix" property.
>>
>> Signed-off-by: Gregor Boirie <gregor.boirie@parrot.com>
> This one definitely wants to go to the device tree list. I'd also like a few
> IIO related comments as well. I was sure we'd been through this before and
> ended up with some sort of solution, but now I can't find it... Oh well.
I took inv_mpu_core as an example. However matrices are exposed as integers
and we really need floating point support.
A search on the device-tree mailing list archive does not show up with a
solution to encode floating point numbers into DTS either.

>
> Definitely needs ABI docs in Documentation/ABI/testing/sysfs-bus-iio
> as well. We need the reference directions to be well specified as
> well - how do we define the main hardware orientation?  on yours presumably
> the usual approach of using the screen is irrelevant so that definition
> could be a little 'interesting' to write down?
The standard orientation for us is defined with respect to the drone's head
and the propellers plane, which does not suit the nominal use cases 
indeed...
Although it really deserves some more detailed documentation, I wonder 
if the
question is relevant.
Given that the DTS is board specific (correct me if I'm wrong), why not
consider the reference orientation as board specific as well (and make the
whole thing implicit) ?

>
> I'm also unsure of whether the right thing to do in the 'not provided'
> cases is to not export the setting, or to assume that the chip is at some
> 'standard' orientation and provide that?
As a self-explanatory interface, I suppose sysfs convention would expect 
to expose
this even in the case of an identity matrix.
However, userspace would need to either:
* systematically apply rotation matrix ;
* or detect identity matrix and skip rotation which may require an a-priori
   knowledge of reference orientation (breaking the statement mentioned 
above).

Personally, I'd prefer not exporting the property at all when aligned with
the main hardware orientation : this makes application simpler and 
allows for
better performances.

What do you think ?
Grégor.

>
> Jonathan
>
>> ---
>>   .../bindings/iio/magnetometer/ak8975.txt           | 10 +++++
>>   drivers/iio/magnetometer/ak8975.c                  | 46 +++++++++++++++++++++-
>>   2 files changed, 55 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/iio/magnetometer/ak8975.txt b/Documentation/devicetree/bindings/iio/magnetometer/ak8975.txt
>> index 34a3206..f936f86 100644
>> --- a/Documentation/devicetree/bindings/iio/magnetometer/ak8975.txt
>> +++ b/Documentation/devicetree/bindings/iio/magnetometer/ak8975.txt
>> @@ -9,6 +9,7 @@ Optional properties:
>>   
>>     - gpios : should be device tree identifier of the magnetometer DRDY pin
>>     - vdd-supply: an optional regulator that needs to be on to provide VDD
>> +  - matrix: an optional 3x3 mounting rotation matrix
>>   
>>   Example:
>>   
>> @@ -17,4 +18,13 @@ ak8975@0c {
>>           reg = <0x0c>;
>>           gpios = <&gpj0 7 0>;
>>           vdd-supply = <&ldo_3v3_gnss>;
>> +        matrix = "-0.984807753012208",  /* x0 */
>> +                 "0",                   /* y0 */
>> +                 "-0.173648177666930",  /* z0 */
>> +                 "0",                   /* x1 */
>> +                 "-1",                  /* y1 */
>> +                 "0",                   /* z1 */
>> +                 "-0.173648177666930",  /* x2 */
>> +                 "0",                   /* y2 */
>> +                 "0.984807753012208";   /* z2 */
>>   };
>> diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
>> index 72c03d9..95c68952 100644
>> --- a/drivers/iio/magnetometer/ak8975.c
>> +++ b/drivers/iio/magnetometer/ak8975.c
>> @@ -370,6 +370,7 @@ struct ak8975_data {
>>   	wait_queue_head_t	data_ready_queue;
>>   	unsigned long		flags;
>>   	u8			cntl_cache;
>> +	const char		*matrix[9];
>>   };
>>   
>>   /*
>> @@ -697,6 +698,28 @@ static int ak8975_read_raw(struct iio_dev *indio_dev,
>>   	return -EINVAL;
>>   }
>>   
>> +static ssize_t ak8975_show_matrix(struct device *dev,
>> +				  struct device_attribute *attr,
>> +				  char *buf)
>> +{
>> +	const struct ak8975_data *data = iio_priv(dev_to_iio_dev(dev));
>> +	const char * const *m = data->matrix;
>> +
>> +	return sprintf(buf, "%s, %s, %s; %s, %s, %s; %s, %s, %s\n",
>> +		       m[0], m[1], m[2], m[3], m[4], m[5], m[6], m[7], m[8]);
>> +}
>> +
>> +static IIO_DEVICE_ATTR(in_magn_matrix, S_IRUGO, ak8975_show_matrix, NULL, -1);
>> +
>> +static struct attribute *ak8975_attrs[] = {
>> +	&iio_dev_attr_in_magn_matrix.dev_attr.attr,
>> +	NULL
>> +};
>> +
>> +static const struct attribute_group ak8975_attrs_group = {
>> +	.attrs = ak8975_attrs
>> +};
>> +
>>   #define AK8975_CHANNEL(axis, index)					\
>>   	{								\
>>   		.type = IIO_MAGN,					\
>> @@ -716,6 +739,12 @@ static const struct iio_info ak8975_info = {
>>   	.driver_module = THIS_MODULE,
>>   };
>>   
>> +static const struct iio_info ak8975_matrix_info = {
>> +	.read_raw = &ak8975_read_raw,
>> +	.attrs = &ak8975_attrs_group,
>> +	.driver_module = THIS_MODULE,
>> +};
>> +
>>   static const struct acpi_device_id ak_acpi_match[] = {
>>   	{"AK8975", AK8975},
>>   	{"AK8963", AK8963},
>> @@ -785,6 +814,22 @@ static int ak8975_probe(struct i2c_client *client,
>>   	data->eoc_gpio = eoc_gpio;
>>   	data->eoc_irq = 0;
>>   
>> +	/*
>> +	 * Rotation matrix is expressed as an array of 3x3 strings to be able
>> +	 * to represent floating point numbers.
>> +	 */
>> +	err = of_property_read_string_array(client->dev.of_node, "matrix",
>> +					    data->matrix,
>> +					    ARRAY_SIZE(data->matrix));
>> +	if (err == ARRAY_SIZE(data->matrix))
>> +		indio_dev->info = &ak8975_matrix_info;
>> +	else if (err == -EINVAL)
>> +		indio_dev->info = &ak8975_info;
>> +	else if (err >= 0)
>> +		return -EINVAL;
>> +	else
>> +		return err;
>> +
>>   	/* id will be NULL when enumerated via ACPI */
>>   	if (id) {
>>   		chipset = (enum asahi_compass_chipset)(id->driver_data);
>> @@ -822,7 +867,6 @@ static int ak8975_probe(struct i2c_client *client,
>>   	indio_dev->dev.parent = &client->dev;
>>   	indio_dev->channels = ak8975_channels;
>>   	indio_dev->num_channels = ARRAY_SIZE(ak8975_channels);
>> -	indio_dev->info = &ak8975_info;
>>   	indio_dev->modes = INDIO_DIRECT_MODE;
>>   	indio_dev->name = name;
>>   	return devm_iio_device_register(&client->dev, indio_dev);
>>

Re: [PATCH v2 4/5] iio:magnetometer:ak8975: mounting matrix support

[Jonathan Cameron]

On 09/03/16 10:56, Gregor Boirie wrote:
> 
> On 03/05/2016 04:36 PM, Jonathan Cameron wrote:
>> On 03/03/16 10:44, Gregor Boirie wrote:
>>> Expose a rotation matrix to indicate userspace the chip placement with
>>> respect to the overall hardware system. This is needed to adjust
>>> coordinates sampled from a magnetometer chip when its position deviates
>>> from the main hardware system.
>>>
>>> Final coordinates computation is delegated to userspace since:
>>> * computation may involve floating point arithmetics ;
>>> * it allows an application to combine adjustments with arbitrary
>>>    transformations.
>>>
>>> This 3 dimentional space rotation matrix is expressed as 3x3 array of
>>> strings to support floating point numbers. It may be retrieved from a
>>> "in_magn_matrix" sysfs attribute file. It is declared into ak8975 DTS
>>> entry as a "matrix" property.
>>>
>>> Signed-off-by: Gregor Boirie <gregor.boirie@parrot.com>
>> This one definitely wants to go to the device tree list. I'd also like a few
>> IIO related comments as well. I was sure we'd been through this before and
>> ended up with some sort of solution, but now I can't find it... Oh well.
> I took inv_mpu_core as an example. However matrices are exposed as integers
> and we really need floating point support.
> A search on the device-tree mailing list archive does not show up with a
> solution to encode floating point numbers into DTS either.
Certainly non obvious how to do this.

For anyone device tree related who picks this up (I've added a CC)
we are basically talking about passing a rotation matrix from the device tree
straight through to userspace to describe the relative orientation of
some sensor.  An array of strings clearly works, but is this the best
way to do it?

(btw as this touches device tree bindings the list and maintainers
should have been cc'd from the start - another one of those kernel
conventions that are non obvious to people who haven't hit it before.
Doing this was a big news item a few years back when a policy to keep
bindings reviewed in a manageable way was decided, but if you weren't
active at the time then you wouldn't necessarily know).
> 
>>
>> Definitely needs ABI docs in Documentation/ABI/testing/sysfs-bus-iio
>> as well. We need the reference directions to be well specified as
>> well - how do we define the main hardware orientation?  on yours presumably
>> the usual approach of using the screen is irrelevant so that definition
>> could be a little 'interesting' to write down?
> The standard orientation for us is defined with respect to the drone's head
> and the propellers plane, which does not suit the nominal use cases indeed...
> Although it really deserves some more detailed documentation, I wonder if the
> question is relevant.
> Given that the DTS is board specific (correct me if I'm wrong), why not
> consider the reference orientation as board specific as well (and make the
> whole thing implicit) ?
I think that is fine for your sort of usecase - though might be nice to keep
a bit of documentation somewhere (perhaps as comments in the DTS), or perhaps
more centrally - would be nice for userspace libraries if at least all aircraft
had a similar 'default orientation'.

For the more 'standard' device with a screen I think we would want to standardise
if at all possible.  Most hand held devices have a 'natural' orientation 
(front facing camera at the top for example).  In those cases we are basically looking
at whether the sensor is mounted on the back or the front of the main circuit board and
then of course it can be at any rotation in plane but typically is at 90 degrees
(hence the inv mpu limitations)

I'm guessing there are existing userspace mappings for this.  Perhaps in the Gnome
sensors stuff?

> 
>>
>> I'm also unsure of whether the right thing to do in the 'not provided'
>> cases is to not export the setting, or to assume that the chip is at some
>> 'standard' orientation and provide that?
> As a self-explanatory interface, I suppose sysfs convention would expect to expose
> this even in the case of an identity matrix.
> However, userspace would need to either:
> * systematically apply rotation matrix ;
> * or detect identity matrix and skip rotation which may require an a-priori
>   knowledge of reference orientation (breaking the statement mentioned above).
> 
> Personally, I'd prefer not exporting the property at all when aligned with
> the main hardware orientation : this makes application simpler and allows for
> better performances.
Make sense.  Much as we don't bother exporting scales of 1 or offsets of 0.
> 
> What do you think ?
> Grégor.
> 
>>
>> Jonathan
>>
>>> ---
>>>   .../bindings/iio/magnetometer/ak8975.txt           | 10 +++++
>>>   drivers/iio/magnetometer/ak8975.c                  | 46 +++++++++++++++++++++-
>>>   2 files changed, 55 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/magnetometer/ak8975.txt b/Documentation/devicetree/bindings/iio/magnetometer/ak8975.txt
>>> index 34a3206..f936f86 100644
>>> --- a/Documentation/devicetree/bindings/iio/magnetometer/ak8975.txt
>>> +++ b/Documentation/devicetree/bindings/iio/magnetometer/ak8975.txt
>>> @@ -9,6 +9,7 @@ Optional properties:
>>>       - gpios : should be device tree identifier of the magnetometer DRDY pin
>>>     - vdd-supply: an optional regulator that needs to be on to provide VDD
>>> +  - matrix: an optional 3x3 mounting rotation matrix
>>>     Example:
>>>   @@ -17,4 +18,13 @@ ak8975@0c {
>>>           reg = <0x0c>;
>>>           gpios = <&gpj0 7 0>;
>>>           vdd-supply = <&ldo_3v3_gnss>;
>>> +        matrix = "-0.984807753012208",  /* x0 */
>>> +                 "0",                   /* y0 */
>>> +                 "-0.173648177666930",  /* z0 */
>>> +                 "0",                   /* x1 */
>>> +                 "-1",                  /* y1 */
>>> +                 "0",                   /* z1 */
>>> +                 "-0.173648177666930",  /* x2 */
>>> +                 "0",                   /* y2 */
>>> +                 "0.984807753012208";   /* z2 */
>>>   };
>>> diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
>>> index 72c03d9..95c68952 100644
>>> --- a/drivers/iio/magnetometer/ak8975.c
>>> +++ b/drivers/iio/magnetometer/ak8975.c
>>> @@ -370,6 +370,7 @@ struct ak8975_data {
>>>       wait_queue_head_t    data_ready_queue;
>>>       unsigned long        flags;
>>>       u8            cntl_cache;
>>> +    const char        *matrix[9];
>>>   };
>>>     /*
>>> @@ -697,6 +698,28 @@ static int ak8975_read_raw(struct iio_dev *indio_dev,
>>>       return -EINVAL;
>>>   }
>>>   +static ssize_t ak8975_show_matrix(struct device *dev,
>>> +                  struct device_attribute *attr,
>>> +                  char *buf)
>>> +{
>>> +    const struct ak8975_data *data = iio_priv(dev_to_iio_dev(dev));
>>> +    const char * const *m = data->matrix;
>>> +
>>> +    return sprintf(buf, "%s, %s, %s; %s, %s, %s; %s, %s, %s\n",
>>> +               m[0], m[1], m[2], m[3], m[4], m[5], m[6], m[7], m[8]);
>>> +}
>>> +
>>> +static IIO_DEVICE_ATTR(in_magn_matrix, S_IRUGO, ak8975_show_matrix, NULL, -1);
>>> +
>>> +static struct attribute *ak8975_attrs[] = {
>>> +    &iio_dev_attr_in_magn_matrix.dev_attr.attr,
>>> +    NULL
>>> +};
>>> +
>>> +static const struct attribute_group ak8975_attrs_group = {
>>> +    .attrs = ak8975_attrs
>>> +};
>>> +
>>>   #define AK8975_CHANNEL(axis, index)                    \
>>>       {                                \
>>>           .type = IIO_MAGN,                    \
>>> @@ -716,6 +739,12 @@ static const struct iio_info ak8975_info = {
>>>       .driver_module = THIS_MODULE,
>>>   };
>>>   +static const struct iio_info ak8975_matrix_info = {
>>> +    .read_raw = &ak8975_read_raw,
>>> +    .attrs = &ak8975_attrs_group,
>>> +    .driver_module = THIS_MODULE,
>>> +};
>>> +
>>>   static const struct acpi_device_id ak_acpi_match[] = {
>>>       {"AK8975", AK8975},
>>>       {"AK8963", AK8963},
>>> @@ -785,6 +814,22 @@ static int ak8975_probe(struct i2c_client *client,
>>>       data->eoc_gpio = eoc_gpio;
>>>       data->eoc_irq = 0;
>>>   +    /*
>>> +     * Rotation matrix is expressed as an array of 3x3 strings to be able
>>> +     * to represent floating point numbers.
>>> +     */
>>> +    err = of_property_read_string_array(client->dev.of_node, "matrix",
>>> +                        data->matrix,
>>> +                        ARRAY_SIZE(data->matrix));
>>> +    if (err == ARRAY_SIZE(data->matrix))
>>> +        indio_dev->info = &ak8975_matrix_info;
>>> +    else if (err == -EINVAL)
>>> +        indio_dev->info = &ak8975_info;
>>> +    else if (err >= 0)
>>> +        return -EINVAL;
>>> +    else
>>> +        return err;
>>> +
>>>       /* id will be NULL when enumerated via ACPI */
>>>       if (id) {
>>>           chipset = (enum asahi_compass_chipset)(id->driver_data);
>>> @@ -822,7 +867,6 @@ static int ak8975_probe(struct i2c_client *client,
>>>       indio_dev->dev.parent = &client->dev;
>>>       indio_dev->channels = ak8975_channels;
>>>       indio_dev->num_channels = ARRAY_SIZE(ak8975_channels);
>>> -    indio_dev->info = &ak8975_info;
>>>       indio_dev->modes = INDIO_DIRECT_MODE;
>>>       indio_dev->name = name;
>>>       return devm_iio_device_register(&client->dev, indio_dev);
>>>
> 
> -- 
> 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

Re: [PATCH v2 4/5] iio:magnetometer:ak8975: mounting matrix support

[Rob Herring]

On Wed, Mar 9, 2016 at 2:46 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 09/03/16 10:56, Gregor Boirie wrote:
>>
>> On 03/05/2016 04:36 PM, Jonathan Cameron wrote:
>>> On 03/03/16 10:44, Gregor Boirie wrote:
>>>> Expose a rotation matrix to indicate userspace the chip placement with
>>>> respect to the overall hardware system. This is needed to adjust
>>>> coordinates sampled from a magnetometer chip when its position deviates
>>>> from the main hardware system.
>>>>
>>>> Final coordinates computation is delegated to userspace since:
>>>> * computation may involve floating point arithmetics ;
>>>> * it allows an application to combine adjustments with arbitrary
>>>>    transformations.
>>>>
>>>> This 3 dimentional space rotation matrix is expressed as 3x3 array of
>>>> strings to support floating point numbers. It may be retrieved from a
>>>> "in_magn_matrix" sysfs attribute file. It is declared into ak8975 DTS
>>>> entry as a "matrix" property.
>>>>
>>>> Signed-off-by: Gregor Boirie <gregor.boirie@parrot.com>
>>> This one definitely wants to go to the device tree list. I'd also like a few
>>> IIO related comments as well. I was sure we'd been through this before and
>>> ended up with some sort of solution, but now I can't find it... Oh well.
>> I took inv_mpu_core as an example. However matrices are exposed as integers
>> and we really need floating point support.
>> A search on the device-tree mailing list archive does not show up with a
>> solution to encode floating point numbers into DTS either.
> Certainly non obvious how to do this.
>
> For anyone device tree related who picks this up (I've added a CC)
> we are basically talking about passing a rotation matrix from the device tree
> straight through to userspace to describe the relative orientation of
> some sensor.  An array of strings clearly works, but is this the best
> way to do it?

That's probably okay. I'd format the dts and sysfs to be arranged in a
3x3 matrix rather than a linear list though, but that is just style.

However, there's nothing really preventing us from having floats in
DT. Data is not typed, so it would be a matter of adding the dtc
parsing support. The other aspect would be the limited use of float
types in the kernel (though that should not influence binding design).
Are there cases of the kernel ABI using floats?

Rob

Re: [PATCH v2 4/5] iio:magnetometer:ak8975: mounting matrix support

[Jonathan Cameron]

On 10/03/16 02:15, Rob Herring wrote:
> On Wed, Mar 9, 2016 at 2:46 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 09/03/16 10:56, Gregor Boirie wrote:
>>>
>>> On 03/05/2016 04:36 PM, Jonathan Cameron wrote:
>>>> On 03/03/16 10:44, Gregor Boirie wrote:
>>>>> Expose a rotation matrix to indicate userspace the chip placement with
>>>>> respect to the overall hardware system. This is needed to adjust
>>>>> coordinates sampled from a magnetometer chip when its position deviates
>>>>> from the main hardware system.
>>>>>
>>>>> Final coordinates computation is delegated to userspace since:
>>>>> * computation may involve floating point arithmetics ;
>>>>> * it allows an application to combine adjustments with arbitrary
>>>>>    transformations.
>>>>>
>>>>> This 3 dimentional space rotation matrix is expressed as 3x3 array of
>>>>> strings to support floating point numbers. It may be retrieved from a
>>>>> "in_magn_matrix" sysfs attribute file. It is declared into ak8975 DTS
>>>>> entry as a "matrix" property.
>>>>>
>>>>> Signed-off-by: Gregor Boirie <gregor.boirie@parrot.com>
>>>> This one definitely wants to go to the device tree list. I'd also like a few
>>>> IIO related comments as well. I was sure we'd been through this before and
>>>> ended up with some sort of solution, but now I can't find it... Oh well.
>>> I took inv_mpu_core as an example. However matrices are exposed as integers
>>> and we really need floating point support.
>>> A search on the device-tree mailing list archive does not show up with a
>>> solution to encode floating point numbers into DTS either.
>> Certainly non obvious how to do this.
>>
>> For anyone device tree related who picks this up (I've added a CC)
>> we are basically talking about passing a rotation matrix from the device tree
>> straight through to userspace to describe the relative orientation of
>> some sensor.  An array of strings clearly works, but is this the best
>> way to do it?
> 
> That's probably okay. I'd format the dts and sysfs to be arranged in a
> 3x3 matrix rather than a linear list though, but that is just style.
> 
> However, there's nothing really preventing us from having floats in
> DT. Data is not typed, so it would be a matter of adding the dtc
> parsing support. The other aspect would be the limited use of float
> types in the kernel (though that should not influence binding design).
> Are there cases of the kernel ABI using floats?
Not many cases of true floats, but certainly in IIO we use a lot of
pseudo floats in which an enum is combined with two integers.
Enum effectively specifies the 'fixed point' location and the
pair of integers typically the integer part and the decimal part
(as we often end up with for example scales that are close to 1)
It's a bit hideous in some ways, but seeing as we are doing fixed
point maths with it in the kernel it actually works out reasonably
cleanly internally.

Right now I don't think any of those are passed in from device tree
but it's certainly possible in the future.

Jonathan
> 
> Rob
> --
> 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
> 

Re: [PATCH v2 4/5] iio:magnetometer:ak8975: mounting matrix support

[Gregor Boirie]

On 03/09/2016 09:46 PM, Jonathan Cameron wrote:

[snip...]
> Definitely needs ABI docs in Documentation/ABI/testing/sysfs-bus-iio
> as well. We need the reference directions to be well specified as
> well - how do we define the main hardware orientation?  on yours presumably
> the usual approach of using the screen is irrelevant so that definition
> could be a little 'interesting' to write down?
>> The standard orientation for us is defined with respect to the drone's head
>> and the propellers plane, which does not suit the nominal use cases indeed...
>> Although it really deserves some more detailed documentation, I wonder if the
>> question is relevant.
>> Given that the DTS is board specific (correct me if I'm wrong), why not
>> consider the reference orientation as board specific as well (and make the
>> whole thing implicit) ?
> I think that is fine for your sort of usecase - though might be nice to keep
> a bit of documentation somewhere (perhaps as comments in the DTS), or perhaps
> more centrally - would be nice for userspace libraries if at least all aircraft
> had a similar 'default orientation'.
As you said earlier, things are never that simple. The standard orientation
may vary with types of UAVs (quad-rotor, fixed wings, bizarre home made 
UFOs...)
> For the more 'standard' device with a screen I think we would want to standardise
> if at all possible.  Most hand held devices have a 'natural' orientation
> (front facing camera at the top for example).  In those cases we are basically looking
> at whether the sensor is mounted on the back or the front of the main circuit board and
> then of course it can be at any rotation in plane but typically is at 90 degrees
> (hence the inv mpu limitations)
>
> I'm guessing there are existing userspace mappings for this.  Perhaps in the Gnome
> sensors stuff?
[snip...]

Re: [PATCH v2 4/5] iio:magnetometer:ak8975: mounting matrix support

[Jonathan Cameron]

On 14/03/16 17:19, Gregor Boirie wrote:
> On 03/09/2016 09:46 PM, Jonathan Cameron wrote:
> 
> [snip...]
>> Definitely needs ABI docs in Documentation/ABI/testing/sysfs-bus-iio
>> as well. We need the reference directions to be well specified as
>> well - how do we define the main hardware orientation?  on yours presumably
>> the usual approach of using the screen is irrelevant so that definition
>> could be a little 'interesting' to write down?
>>> The standard orientation for us is defined with respect to the drone's head
>>> and the propellers plane, which does not suit the nominal use cases indeed...
>>> Although it really deserves some more detailed documentation, I wonder if the
>>> question is relevant.
>>> Given that the DTS is board specific (correct me if I'm wrong), why not
>>> consider the reference orientation as board specific as well (and make the
>>> whole thing implicit) ?
>> I think that is fine for your sort of usecase - though might be nice to keep
>> a bit of documentation somewhere (perhaps as comments in the DTS), or perhaps
>> more centrally - would be nice for userspace libraries if at least all aircraft
>> had a similar 'default orientation'.
> As you said earlier, things are never that simple. The standard orientation
> may vary with types of UAVs (quad-rotor, fixed wings, bizarre home made UFOs...)
Sure, but let's document what we can so at least some cases are covered
>> For the more 'standard' device with a screen I think we would want to standardise
>> if at all possible.  Most hand held devices have a 'natural' orientation
>> (front facing camera at the top for example).  In those cases we are basically looking
>> at whether the sensor is mounted on the back or the front of the main circuit board and
>> then of course it can be at any rotation in plane but typically is at 90 degrees
>> (hence the inv mpu limitations)
>>
>> I'm guessing there are existing userspace mappings for this.  Perhaps in the Gnome
>> sensors stuff?
> [snip...]
> -- 
> 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

Re: [PATCH v2 4/5] iio:magnetometer:ak8975: mounting matrix support

[Gregor Boirie]

On 03/15/2016 10:56 PM, Jonathan Cameron wrote:
> On 14/03/16 17:19, Gregor Boirie wrote:
>> On 03/09/2016 09:46 PM, Jonathan Cameron wrote:
>>
>> [snip...]
>>> Definitely needs ABI docs in Documentation/ABI/testing/sysfs-bus-iio
>>> as well. We need the reference directions to be well specified as
>>> well - how do we define the main hardware orientation?  on yours presumably
>>> the usual approach of using the screen is irrelevant so that definition
>>> could be a little 'interesting' to write down?
>>>> The standard orientation for us is defined with respect to the drone's head
>>>> and the propellers plane, which does not suit the nominal use cases indeed...
>>>> Although it really deserves some more detailed documentation, I wonder if the
>>>> question is relevant.
>>>> Given that the DTS is board specific (correct me if I'm wrong), why not
>>>> consider the reference orientation as board specific as well (and make the
>>>> whole thing implicit) ?
>>> I think that is fine for your sort of usecase - though might be nice to keep
>>> a bit of documentation somewhere (perhaps as comments in the DTS), or perhaps
>>> more centrally - would be nice for userspace libraries if at least all aircraft
>>> had a similar 'default orientation'.
>> As you said earlier, things are never that simple. The standard orientation
>> may vary with types of UAVs (quad-rotor, fixed wings, bizarre home made UFOs...)
> Sure, but let's document what we can so at least some cases are covered
Let give it a try... (forgive my approximate english).

  /sys/bus/iio/devices/iio:deviceX/in_magn_matrix:

Mounting matrix for magnetometer sensors. This is a rotation matrix which
informs userspace about sensor chip's orientation relative to the main 
hardware
it is mounted on.
More specifically, main hardware orientation is defined with respect to the
local earth geomagnetic reference frame where :
* Y is in the ground plane and positive towards magnetic North ;
* X is in the ground plane, perpendicular to the North axis and ;
   positive towards the East
* Z is perpendicular to the ground plane and positive upwards.

Sensor orientation is defined with respect to the main hardware 
reference frame.
Given that the rotation matrix is defined in a board specific way 
(platform data
and / or device-tree), the main hardware reference frame definition is 
left to
the implementor's choice.
As an examplary guideline, one can consider that for a hand-held device, a
'natural' orientation would be 'front facing camera at the top'. The main
hardware reference frame could then be described as :
* Y is in the plane of the screen and is positive towards the top of the 
screen ;
* X is in the plane of the screen, perpendicular to Y axis, and positive 
towards
   the right hand side of the screen ;
* Z is perpendicular to the screen plane and positive out of the screen.
Another example for a quadrotor UAV might be :
* Y is in the plane of the propellers is positive towards the front-view 
camera;
* X is in the plane of the propellers, perpendicular to Y axis, and positive
   towards the starboard side of the UAV ;
* Z is perpendicular to propellers plane and positive upwards.

Applications should apply this rotation matrix to samples so that whenmain
hardware reference frame is aligned onto local earth geomagnetic reference
frame, then sensor chip reference frame is also perfectly aligned with it.

Matrix is a 3x3 unitary matrix and typically looks like
[0, 1, 0; 1, 0, 0; 0, 0, -1]. A missing in_magn_matrix sysfs entry means 
sensor
chip and main hardware are perfectly aligned with each other. It would be
identical to an in_magn_matrix file holding the identity matrix
[1, 0, 0; 0, 1, 0; 0, 0, 1].

References:
https://www.w3.org/TR/orientation-event/
http://developer.android.com/reference/android/hardware/SensorEvent.html
https://developers.google.com/project-tango/overview/frames-of-reference#coordinate_frame_pairs
http://www.chrobotics.com/library/understanding-euler-angles

Gregor.

[PATCH v2 5/5] iio:magnetometer:ak8975: triggered buffer support

[Gregor Boirie]

This will be used together with an external trigger (e.g hrtimer based
software trigger).

Signed-off-by: Gregor Boirie <gregor.boirie@parrot.com>
---
 drivers/iio/magnetometer/Kconfig  |   2 +
 drivers/iio/magnetometer/ak8975.c | 150 +++++++++++++++++++++++++++++++-------
 2 files changed, 125 insertions(+), 27 deletions(-)

diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
index 021dc53..d9834ed 100644
--- a/drivers/iio/magnetometer/Kconfig
+++ b/drivers/iio/magnetometer/Kconfig
@@ -9,6 +9,8 @@ config AK8975
 	tristate "Asahi Kasei AK 3-Axis Magnetometer"
 	depends on I2C
 	depends on GPIOLIB || COMPILE_TEST
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
 	help
 	  Say yes here to build support for Asahi Kasei AK8975, AK8963,
 	  AK09911 or AK09912 3-Axis Magnetometer.
diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
index 95c68952..9559ab8 100644
--- a/drivers/iio/magnetometer/ak8975.c
+++ b/drivers/iio/magnetometer/ak8975.c
@@ -36,6 +36,12 @@
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/regulator/consumer.h>
+
 /*
  * Register definitions, as well as various shifts and masks to get at the
  * individual fields of the registers.
@@ -617,22 +623,15 @@ static int wait_conversion_complete_interrupt(struct ak8975_data *data)
 	return ret > 0 ? 0 : -ETIME;
 }
 
-/*
- * Emits the raw flux value for the x, y, or z axis.
- */
-static int ak8975_read_axis(struct iio_dev *indio_dev, int index, int *val)
+static int ak8975_start_read_axis(struct ak8975_data *data,
+				  const struct i2c_client *client)
 {
-	struct ak8975_data *data = iio_priv(indio_dev);
-	struct i2c_client *client = data->client;
-	int ret;
-
-	mutex_lock(&data->lock);
-
 	/* Set up the device for taking a sample. */
-	ret = ak8975_set_mode(data, MODE_ONCE);
-	if (ret < 0) {
+	int ret = ak8975_set_mode(data, MODE_ONCE);
+
+	if (ret) {
 		dev_err(&client->dev, "Error in setting operating mode\n");
-		goto exit;
+		return ret;
 	}
 
 	/* Wait for the conversion to complete. */
@@ -643,7 +642,7 @@ static int ak8975_read_axis(struct iio_dev *indio_dev, int index, int *val)
 	else
 		ret = wait_conversion_complete_polled(data);
 	if (ret < 0)
-		goto exit;
+		return ret;
 
 	/* This will be executed only for non-interrupt based waiting case */
 	if (ret & data->def->ctrl_masks[ST1_DRDY]) {
@@ -651,32 +650,46 @@ static int ak8975_read_axis(struct iio_dev *indio_dev, int index, int *val)
 					       data->def->ctrl_regs[ST2]);
 		if (ret < 0) {
 			dev_err(&client->dev, "Error in reading ST2\n");
-			goto exit;
+			return ret;
 		}
 		if (ret & (data->def->ctrl_masks[ST2_DERR] |
 			   data->def->ctrl_masks[ST2_HOFL])) {
 			dev_err(&client->dev, "ST2 status error 0x%x\n", ret);
-			ret = -EINVAL;
-			goto exit;
+			return -EINVAL;
 		}
 	}
 
-	/* Read the flux value from the appropriate register
-	   (the register is specified in the iio device attributes). */
-	ret = i2c_smbus_read_word_data(client, data->def->data_regs[index]);
-	if (ret < 0) {
-		dev_err(&client->dev, "Read axis data fails\n");
+	return 0;
+}
+
+/*
+ * Retrieve raw flux value for one of the x, y, or z axis.
+ */
+static int ak8975_read_axis(struct ak8975_data *data, int index, int *val)
+{
+	int ret;
+	const struct i2c_client *client = data->client;
+	const struct ak_def *def = data->def;
+
+	mutex_lock(&data->lock);
+
+	ret = ak8975_start_read_axis(data, client);
+	if (ret)
+		goto exit;
+
+	ret = i2c_smbus_read_word_data(client, def->data_regs[index]);
+	if (ret < 0)
 		goto exit;
-	}
 
 	mutex_unlock(&data->lock);
 
 	/* Clamp to valid range. */
-	*val = clamp_t(s16, ret, -data->def->range, data->def->range);
+	*val = clamp_t(s16, ret, -def->range, def->range);
 	return IIO_VAL_INT;
 
 exit:
 	mutex_unlock(&data->lock);
+	dev_err(&client->dev, "Error in reading axis\n");
 	return ret;
 }
 
@@ -689,7 +702,7 @@ static int ak8975_read_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		return ak8975_read_axis(indio_dev, chan->address, val);
+		return ak8975_read_axis(data, chan->address, val);
 	case IIO_CHAN_INFO_SCALE:
 		*val = 0;
 		*val2 = data->raw_to_gauss[chan->address];
@@ -728,12 +741,25 @@ static const struct attribute_group ak8975_attrs_group = {
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
 			     BIT(IIO_CHAN_INFO_SCALE),			\
 		.address = index,					\
+		.scan_index = index,					\
+		.scan_type = {						\
+			.sign = 's',					\
+			.realbits = 16,					\
+			.storagebits = 16,				\
+			.shift = 0,					\
+			.endianness = IIO_CPU				\
+		}							\
 	}
 
 static const struct iio_chan_spec ak8975_channels[] = {
-	AK8975_CHANNEL(X, 0), AK8975_CHANNEL(Y, 1), AK8975_CHANNEL(Z, 2),
+	AK8975_CHANNEL(X, 0),
+	AK8975_CHANNEL(Y, 1),
+	AK8975_CHANNEL(Z, 2),
+	IIO_CHAN_SOFT_TIMESTAMP(3),
 };
 
+static const unsigned long ak8975_scan_masks[] = { 0x7, 0 };
+
 static const struct iio_info ak8975_info = {
 	.read_raw = &ak8975_read_raw,
 	.driver_module = THIS_MODULE,
@@ -768,6 +794,51 @@ static const char *ak8975_match_acpi_device(struct device *dev,
 	return dev_name(dev);
 }
 
+static irqreturn_t ak8975_handle_trigger(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct ak8975_data *data = iio_priv(indio_dev);
+	const struct i2c_client *client = data->client;
+	const struct ak_def *def = data->def;
+	int ret;
+	s16 buff[8]; /* 3 x 16 bits axis values + 1 aligned 64 bits timestamp */
+
+	mutex_lock(&data->lock);
+
+	ret = ak8975_start_read_axis(data, client);
+	if (ret)
+		goto unlock;
+
+	/*
+	 * For each axis, read the flux value from the appropriate register
+	 * (the register is specified in the iio device attributes).
+	 */
+	ret = i2c_smbus_read_i2c_block_data_or_emulated(client,
+							def->data_regs[0],
+							3 * sizeof(buff[0]),
+							(u8 *)buff);
+	if (ret < 0)
+		goto unlock;
+
+	mutex_unlock(&data->lock);
+
+	/* Clamp to valid range. */
+	buff[0] = clamp_t(s16, le16_to_cpu(buff[0]), -def->range, def->range);
+	buff[1] = clamp_t(s16, le16_to_cpu(buff[1]), -def->range, def->range);
+	buff[2] = clamp_t(s16, le16_to_cpu(buff[2]), -def->range, def->range);
+
+	iio_push_to_buffers_with_timestamp(indio_dev, buff, iio_get_time_ns());
+	goto exit;
+
+unlock:
+	mutex_unlock(&data->lock);
+	dev_err(&client->dev, "Error in reading axes block\n");
+exit:
+	iio_trigger_notify_done(indio_dev->trig);
+	return IRQ_HANDLED;
+}
+
 static int ak8975_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
@@ -867,9 +938,33 @@ static int ak8975_probe(struct i2c_client *client,
 	indio_dev->dev.parent = &client->dev;
 	indio_dev->channels = ak8975_channels;
 	indio_dev->num_channels = ARRAY_SIZE(ak8975_channels);
+	indio_dev->available_scan_masks = ak8975_scan_masks;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->name = name;
-	return devm_iio_device_register(&client->dev, indio_dev);
+
+	err = iio_triggered_buffer_setup(indio_dev, NULL, ak8975_handle_trigger,
+					 NULL);
+	if (err) {
+		dev_err(&client->dev, "triggered buffer setup failed\n");
+		return err;
+	}
+
+	err = iio_device_register(indio_dev);
+	if (!err)
+		return 0;
+
+	iio_triggered_buffer_cleanup(indio_dev);
+	dev_err(&client->dev, "device register failed\n");
+	return err;
+}
+
+static int ak8975_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+
+	iio_device_unregister(indio_dev);
+	iio_triggered_buffer_cleanup(indio_dev);
+	return 0;
 }
 
 static const struct i2c_device_id ak8975_id[] = {
@@ -903,6 +998,7 @@ static struct i2c_driver ak8975_driver = {
 		.acpi_match_table = ACPI_PTR(ak_acpi_match),
 	},
 	.probe		= ak8975_probe,
+	.remove		= ak8975_remove,
 	.id_table	= ak8975_id,
 };
 module_i2c_driver(ak8975_driver);
-- 
2.1.4

Re: [PATCH v2 5/5] iio:magnetometer:ak8975: triggered buffer support

[Jonathan Cameron]

On 03/03/16 10:44, Gregor Boirie wrote:
> This will be used together with an external trigger (e.g hrtimer based
> software trigger).
> 
> Signed-off-by: Gregor Boirie <gregor.boirie@parrot.com>
A few little bits and bobs inline.  Mostly stuff that I think could be
a little easier to read (at the cost of the odd extra line of code).

Jonathan
> ---
>  drivers/iio/magnetometer/Kconfig  |   2 +
>  drivers/iio/magnetometer/ak8975.c | 150 +++++++++++++++++++++++++++++++-------
>  2 files changed, 125 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
> index 021dc53..d9834ed 100644
> --- a/drivers/iio/magnetometer/Kconfig
> +++ b/drivers/iio/magnetometer/Kconfig
> @@ -9,6 +9,8 @@ config AK8975
>  	tristate "Asahi Kasei AK 3-Axis Magnetometer"
>  	depends on I2C
>  	depends on GPIOLIB || COMPILE_TEST
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
>  	help
>  	  Say yes here to build support for Asahi Kasei AK8975, AK8963,
>  	  AK09911 or AK09912 3-Axis Magnetometer.
> diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
> index 95c68952..9559ab8 100644
> --- a/drivers/iio/magnetometer/ak8975.c
> +++ b/drivers/iio/magnetometer/ak8975.c
> @@ -36,6 +36,12 @@
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/regulator/consumer.h>
> +
>  /*
>   * Register definitions, as well as various shifts and masks to get at the
>   * individual fields of the registers.
> @@ -617,22 +623,15 @@ static int wait_conversion_complete_interrupt(struct ak8975_data *data)
>  	return ret > 0 ? 0 : -ETIME;
>  }
>  
> -/*
> - * Emits the raw flux value for the x, y, or z axis.
> - */
> -static int ak8975_read_axis(struct iio_dev *indio_dev, int index, int *val)
> +static int ak8975_start_read_axis(struct ak8975_data *data,
> +				  const struct i2c_client *client)
>  {
> -	struct ak8975_data *data = iio_priv(indio_dev);
> -	struct i2c_client *client = data->client;
> -	int ret;
> -
> -	mutex_lock(&data->lock);
> -
>  	/* Set up the device for taking a sample. */
> -	ret = ak8975_set_mode(data, MODE_ONCE);
> -	if (ret < 0) {
> +	int ret = ak8975_set_mode(data, MODE_ONCE);
> +
> +	if (ret) {
>  		dev_err(&client->dev, "Error in setting operating mode\n");
> -		goto exit;
> +		return ret;
>  	}
>  
>  	/* Wait for the conversion to complete. */
> @@ -643,7 +642,7 @@ static int ak8975_read_axis(struct iio_dev *indio_dev, int index, int *val)
>  	else
>  		ret = wait_conversion_complete_polled(data);
>  	if (ret < 0)
> -		goto exit;
> +		return ret;
>  
>  	/* This will be executed only for non-interrupt based waiting case */
>  	if (ret & data->def->ctrl_masks[ST1_DRDY]) {
> @@ -651,32 +650,46 @@ static int ak8975_read_axis(struct iio_dev *indio_dev, int index, int *val)
>  					       data->def->ctrl_regs[ST2]);
>  		if (ret < 0) {
>  			dev_err(&client->dev, "Error in reading ST2\n");
> -			goto exit;
> +			return ret;
>  		}
>  		if (ret & (data->def->ctrl_masks[ST2_DERR] |
>  			   data->def->ctrl_masks[ST2_HOFL])) {
>  			dev_err(&client->dev, "ST2 status error 0x%x\n", ret);
> -			ret = -EINVAL;
> -			goto exit;
> +			return -EINVAL;
>  		}
>  	}
>  
> -	/* Read the flux value from the appropriate register
> -	   (the register is specified in the iio device attributes). */
> -	ret = i2c_smbus_read_word_data(client, data->def->data_regs[index]);
> -	if (ret < 0) {
> -		dev_err(&client->dev, "Read axis data fails\n");
> +	return 0;
> +}
> +
> +/*
> + * Retrieve raw flux value for one of the x, y, or z axis.
Single line comment so should probably have single line comment syntax
(or I'll get a 'fix' patch for it ;)
> + */
> +static int ak8975_read_axis(struct ak8975_data *data, int index, int *val)
> +{
> +	int ret;
> +	const struct i2c_client *client = data->client;
> +	const struct ak_def *def = data->def;
> +
> +	mutex_lock(&data->lock);
> +
> +	ret = ak8975_start_read_axis(data, client);
> +	if (ret)
> +		goto exit;
> +
> +	ret = i2c_smbus_read_word_data(client, def->data_regs[index]);
> +	if (ret < 0)
>  		goto exit;
> -	}
>  
>  	mutex_unlock(&data->lock);
>  
>  	/* Clamp to valid range. */
> -	*val = clamp_t(s16, ret, -data->def->range, data->def->range);
> +	*val = clamp_t(s16, ret, -def->range, def->range);
>  	return IIO_VAL_INT;
>  
>  exit:
>  	mutex_unlock(&data->lock);
> +	dev_err(&client->dev, "Error in reading axis\n");
>  	return ret;
>  }
>  
> @@ -689,7 +702,7 @@ static int ak8975_read_raw(struct iio_dev *indio_dev,
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
Not sure I'd have bothered with this change...
> -		return ak8975_read_axis(indio_dev, chan->address, val);
> +		return ak8975_read_axis(data, chan->address, val);
>  	case IIO_CHAN_INFO_SCALE:
>  		*val = 0;
>  		*val2 = data->raw_to_gauss[chan->address];
> @@ -728,12 +741,25 @@ static const struct attribute_group ak8975_attrs_group = {
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
>  			     BIT(IIO_CHAN_INFO_SCALE),			\
>  		.address = index,					\
> +		.scan_index = index,					\
> +		.scan_type = {						\
> +			.sign = 's',					\
> +			.realbits = 16,					\
> +			.storagebits = 16,				\
> +			.shift = 0,					\
Don't specify shift if it is 0 as that's the default.
> +			.endianness = IIO_CPU				\
> +		}							\
>  	}
>  
>  static const struct iio_chan_spec ak8975_channels[] = {
> -	AK8975_CHANNEL(X, 0), AK8975_CHANNEL(Y, 1), AK8975_CHANNEL(Z, 2),
> +	AK8975_CHANNEL(X, 0),
> +	AK8975_CHANNEL(Y, 1),
> +	AK8975_CHANNEL(Z, 2),
An unrelated change but I guess minor enough that it isn't worth it's own patch
> +	IIO_CHAN_SOFT_TIMESTAMP(3),
>  };
>  
> +static const unsigned long ak8975_scan_masks[] = { 0x7, 0 };
> +
>  static const struct iio_info ak8975_info = {
>  	.read_raw = &ak8975_read_raw,
>  	.driver_module = THIS_MODULE,
> @@ -768,6 +794,51 @@ static const char *ak8975_match_acpi_device(struct device *dev,
>  	return dev_name(dev);
>  }
>  
> +static irqreturn_t ak8975_handle_trigger(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct ak8975_data *data = iio_priv(indio_dev);
> +	const struct i2c_client *client = data->client;
> +	const struct ak_def *def = data->def;
> +	int ret;
> +	s16 buff[8]; /* 3 x 16 bits axis values + 1 aligned 64 bits timestamp */
> +
> +	mutex_lock(&data->lock);
> +
> +	ret = ak8975_start_read_axis(data, client);
> +	if (ret)
> +		goto unlock;
> +
> +	/*
> +	 * For each axis, read the flux value from the appropriate register
> +	 * (the register is specified in the iio device attributes).
> +	 */
> +	ret = i2c_smbus_read_i2c_block_data_or_emulated(client,
> +							def->data_regs[0],
> +							3 * sizeof(buff[0]),
> +							(u8 *)buff);
> +	if (ret < 0)
> +		goto unlock;
> +
> +	mutex_unlock(&data->lock);
> +
> +	/* Clamp to valid range. */
> +	buff[0] = clamp_t(s16, le16_to_cpu(buff[0]), -def->range, def->range);
> +	buff[1] = clamp_t(s16, le16_to_cpu(buff[1]), -def->range, def->range);
> +	buff[2] = clamp_t(s16, le16_to_cpu(buff[2]), -def->range, def->range);
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, buff, iio_get_time_ns());
> +	goto exit;
Clearer to replicate the exit condition here I think and not have the goto.
> +
> +unlock:
> +	mutex_unlock(&data->lock);
> +	dev_err(&client->dev, "Error in reading axes block\n");
> +exit:
> +	iio_trigger_notify_done(indio_dev->trig);
> +	return IRQ_HANDLED;
> +}
> +
>  static int ak8975_probe(struct i2c_client *client,
>  			const struct i2c_device_id *id)
>  {
> @@ -867,9 +938,33 @@ static int ak8975_probe(struct i2c_client *client,
>  	indio_dev->dev.parent = &client->dev;
>  	indio_dev->channels = ak8975_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(ak8975_channels);
> +	indio_dev->available_scan_masks = ak8975_scan_masks;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->name = name;
> -	return devm_iio_device_register(&client->dev, indio_dev);
> +
> +	err = iio_triggered_buffer_setup(indio_dev, NULL, ak8975_handle_trigger,
> +					 NULL);
> +	if (err) {
> +		dev_err(&client->dev, "triggered buffer setup failed\n");
> +		return err;
> +	}
> +
> +	err = iio_device_register(indio_dev);
> +	if (!err)
> +		return 0;
Please keep to convention of error paths being the non 'standard' route.
It adds a small amount of code, but gives slightly easier to read code.
> +
> +	iio_triggered_buffer_cleanup(indio_dev);
> +	dev_err(&client->dev, "device register failed\n");
> +	return err;
> +}
> +
> +static int ak8975_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +
> +	iio_device_unregister(indio_dev);
> +	iio_triggered_buffer_cleanup(indio_dev);
blank line here preferred  (nitpick of the day ;)
> +	return 0;
>  }
>  
>  static const struct i2c_device_id ak8975_id[] = {
> @@ -903,6 +998,7 @@ static struct i2c_driver ak8975_driver = {
>  		.acpi_match_table = ACPI_PTR(ak_acpi_match),
>  	},
>  	.probe		= ak8975_probe,
> +	.remove		= ak8975_remove,
>  	.id_table	= ak8975_id,
>  };
>  module_i2c_driver(ak8975_driver);
>