[PATCH v3 0/3] iio:magnetometer:ak8975: fix and enhancements

[PATCH v3 0/3] 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

Changes since v2:
* use devm_regulator_get to allow dummy regulator usage
* ensure regulator is properly disabled
* drop first 3 patches since applied
* document in_magn_matrix sysfs attribute
* single line comments where appropriate
* remove explicit zero init for shift field of iio_chan_spec structure as that's
  the default
* drop unnecessary ak8975_read_axis signature modification
* remove useless cosmetic changes related to ak8975_channels
* normalize and make error handling code paths clearer
* make ak8975_remove less nitpick'able :)

Gregor.

Gregor Boirie (3):
  iio:magnetometer:ak8975: fix missing regulator_disable
  iio:magnetometer:ak8975: mounting matrix support
  iio:magnetometer:ak8975: triggered buffer support

 Documentation/ABI/testing/sysfs-bus-iio-ak8975     |  46 ++++
 .../bindings/iio/magnetometer/ak8975.txt           |  10 +
 drivers/iio/magnetometer/Kconfig                   |   2 +
 drivers/iio/magnetometer/ak8975.c                  | 255 +++++++++++++++++----
 4 files changed, 271 insertions(+), 42 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-ak8975

-- 
2.1.4

[PATCH v3 1/3] iio:magnetometer:ak8975: fix missing regulator_disable

[Gregor Boirie]

Ensure optional regulator is properly disabled when present.

Fixes: 63d5d525cbbc ("iio:magnetometer:ak8975: power regulator support")
Signed-off-by: Gregor Boirie <gregor.boirie-ITF29qwbsa/QT0dZR+AlfA@public.gmane.org>
---
 drivers/iio/magnetometer/ak8975.c | 78 ++++++++++++++++++++++++++++++---------
 1 file changed, 60 insertions(+), 18 deletions(-)

diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
index 72c03d9..48d127a 100644
--- a/drivers/iio/magnetometer/ak8975.c
+++ b/drivers/iio/magnetometer/ak8975.c
@@ -370,8 +370,40 @@ struct ak8975_data {
 	wait_queue_head_t	data_ready_queue;
 	unsigned long		flags;
 	u8			cntl_cache;
+	struct regulator	*vdd;
 };
 
+/* Enable attached power regulator if any. */
+static int ak8975_power_on(struct i2c_client *client)
+{
+	const struct iio_dev *indio_dev = i2c_get_clientdata(client);
+	struct ak8975_data *data = iio_priv(indio_dev);
+	int ret;
+
+	data->vdd = devm_regulator_get(&client->dev, "vdd");
+	if (IS_ERR_OR_NULL(data->vdd)) {
+		ret = PTR_ERR(data->vdd);
+		if (ret == -ENODEV)
+			ret = 0;
+	} else {
+		ret = regulator_enable(data->vdd);
+	}
+
+	if (ret)
+		dev_err(&client->dev, "failed to enable Vdd supply: %d\n", ret);
+	return ret;
+}
+
+/* Disable attached power regulator if any. */
+static void ak8975_power_off(const struct i2c_client *client)
+{
+	const struct iio_dev *indio_dev = i2c_get_clientdata(client);
+	const struct ak8975_data *data = iio_priv(indio_dev);
+
+	if (!IS_ERR_OR_NULL(data->vdd))
+		regulator_disable(data->vdd);
+}
+
 /*
  * Return 0 if the i2c device is the one we expect.
  * return a negative error number otherwise
@@ -380,23 +412,8 @@ 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
@@ -804,10 +821,15 @@ static int ak8975_probe(struct i2c_client *client,
 	}
 
 	data->def = &ak_def_array[chipset];
+
+	err = ak8975_power_on(client);
+	if (err)
+		return err;
+
 	err = ak8975_who_i_am(client, data->def->type);
 	if (err < 0) {
 		dev_err(&client->dev, "Unexpected device\n");
-		return err;
+		goto power_off;
 	}
 	dev_dbg(&client->dev, "Asahi compass chip %s\n", name);
 
@@ -815,7 +837,7 @@ static int ak8975_probe(struct i2c_client *client,
 	err = ak8975_setup(client);
 	if (err < 0) {
 		dev_err(&client->dev, "%s initialization fails\n", name);
-		return err;
+		goto power_off;
 	}
 
 	mutex_init(&data->lock);
@@ -825,7 +847,26 @@ static int ak8975_probe(struct i2c_client *client,
 	indio_dev->info = &ak8975_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->name = name;
-	return devm_iio_device_register(&client->dev, indio_dev);
+
+	err = iio_device_register(indio_dev);
+	if (err)
+		goto power_off;
+
+	return 0;
+
+power_off:
+	ak8975_power_off(client);
+	return err;
+}
+
+static int ak8975_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+
+	iio_device_unregister(indio_dev);
+	ak8975_power_off(client);
+
+	return 0;
 }
 
 static const struct i2c_device_id ak8975_id[] = {
@@ -859,6 +900,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 v3 1/3] iio:magnetometer:ak8975: fix missing regulator_disable

[Jonathan Cameron]

On 17/03/16 16:43, Gregor Boirie wrote:
> Ensure optional regulator is properly disabled when present.
> 
> Fixes: 63d5d525cbbc ("iio:magnetometer:ak8975: power regulator support")
> Signed-off-by: Gregor Boirie <gregor.boirie-ITF29qwbsa/QT0dZR+AlfA@public.gmane.org>
Applied to the togreg branch of iio.git

Thanks,

Jonathan
> ---
>  drivers/iio/magnetometer/ak8975.c | 78 ++++++++++++++++++++++++++++++---------
>  1 file changed, 60 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
> index 72c03d9..48d127a 100644
> --- a/drivers/iio/magnetometer/ak8975.c
> +++ b/drivers/iio/magnetometer/ak8975.c
> @@ -370,8 +370,40 @@ struct ak8975_data {
>  	wait_queue_head_t	data_ready_queue;
>  	unsigned long		flags;
>  	u8			cntl_cache;
> +	struct regulator	*vdd;
>  };
>  
> +/* Enable attached power regulator if any. */
> +static int ak8975_power_on(struct i2c_client *client)
> +{
> +	const struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +	struct ak8975_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	data->vdd = devm_regulator_get(&client->dev, "vdd");
> +	if (IS_ERR_OR_NULL(data->vdd)) {
> +		ret = PTR_ERR(data->vdd);
> +		if (ret == -ENODEV)
> +			ret = 0;
> +	} else {
> +		ret = regulator_enable(data->vdd);
> +	}
> +
> +	if (ret)
> +		dev_err(&client->dev, "failed to enable Vdd supply: %d\n", ret);
> +	return ret;
> +}
> +
> +/* Disable attached power regulator if any. */
> +static void ak8975_power_off(const struct i2c_client *client)
> +{
> +	const struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +	const struct ak8975_data *data = iio_priv(indio_dev);
> +
> +	if (!IS_ERR_OR_NULL(data->vdd))
> +		regulator_disable(data->vdd);
> +}
> +
>  /*
>   * Return 0 if the i2c device is the one we expect.
>   * return a negative error number otherwise
> @@ -380,23 +412,8 @@ 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
> @@ -804,10 +821,15 @@ static int ak8975_probe(struct i2c_client *client,
>  	}
>  
>  	data->def = &ak_def_array[chipset];
> +
> +	err = ak8975_power_on(client);
> +	if (err)
> +		return err;
> +
>  	err = ak8975_who_i_am(client, data->def->type);
>  	if (err < 0) {
>  		dev_err(&client->dev, "Unexpected device\n");
> -		return err;
> +		goto power_off;
>  	}
>  	dev_dbg(&client->dev, "Asahi compass chip %s\n", name);
>  
> @@ -815,7 +837,7 @@ static int ak8975_probe(struct i2c_client *client,
>  	err = ak8975_setup(client);
>  	if (err < 0) {
>  		dev_err(&client->dev, "%s initialization fails\n", name);
> -		return err;
> +		goto power_off;
>  	}
>  
>  	mutex_init(&data->lock);
> @@ -825,7 +847,26 @@ static int ak8975_probe(struct i2c_client *client,
>  	indio_dev->info = &ak8975_info;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->name = name;
> -	return devm_iio_device_register(&client->dev, indio_dev);
> +
> +	err = iio_device_register(indio_dev);
> +	if (err)
> +		goto power_off;
> +
> +	return 0;
> +
> +power_off:
> +	ak8975_power_off(client);
> +	return err;
> +}
> +
> +static int ak8975_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +
> +	iio_device_unregister(indio_dev);
> +	ak8975_power_off(client);
> +
> +	return 0;
>  }
>  
>  static const struct i2c_device_id ak8975_id[] = {
> @@ -859,6 +900,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);
> 

[PATCH v3 2/3] 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-ITF29qwbsa/QT0dZR+AlfA@public.gmane.org>
---
 Documentation/ABI/testing/sysfs-bus-iio-ak8975     | 46 ++++++++++++++++++++++
 .../bindings/iio/magnetometer/ak8975.txt           | 10 +++++
 drivers/iio/magnetometer/ak8975.c                  | 46 +++++++++++++++++++++-
 3 files changed, 101 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-ak8975

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-ak8975 b/Documentation/ABI/testing/sysfs-bus-iio-ak8975
new file mode 100644
index 0000000..fa92ee1
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-ak8975
@@ -0,0 +1,46 @@
+What:           /sys/bus/iio/devices/iio:deviceX/in_magn_matrix
+KernelVersion:  4.6
+Contact:        linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+Description:
+		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 when main 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 exposing the identity
+		matrix [1, 0, 0; 0, 1, 0; 0, 0, 1].
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 48d127a..3814a6a 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];
 	struct regulator	*vdd;
 };
 
@@ -714,6 +715,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,					\
@@ -733,6 +756,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},
@@ -802,6 +831,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);
@@ -844,7 +889,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;
 
-- 
2.1.4

Re: [PATCH v3 2/3] iio:magnetometer:ak8975: mounting matrix support

[Jonathan Cameron]

On 17/03/16 16:43, 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-ITF29qwbsa/QT0dZR+AlfA@public.gmane.org>
Very nice.

Only outstanding question I think is whether it is worth making device
tree explicitly aware of floating point.  If that happens we may
need to play some games to support that and this existing string binding
but I doubt that will be too hard.

Applied to the togreg branch of iio.git

Thanks,

Jonathan
> ---
>  Documentation/ABI/testing/sysfs-bus-iio-ak8975     | 46 ++++++++++++++++++++++
>  .../bindings/iio/magnetometer/ak8975.txt           | 10 +++++
>  drivers/iio/magnetometer/ak8975.c                  | 46 +++++++++++++++++++++-
>  3 files changed, 101 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-ak8975
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-ak8975 b/Documentation/ABI/testing/sysfs-bus-iio-ak8975
> new file mode 100644
> index 0000000..fa92ee1
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-ak8975
> @@ -0,0 +1,46 @@
> +What:           /sys/bus/iio/devices/iio:deviceX/in_magn_matrix
> +KernelVersion:  4.6
> +Contact:        linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> +Description:
> +		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 when main 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 exposing the identity
> +		matrix [1, 0, 0; 0, 1, 0; 0, 0, 1].

Reads well and covers everything so I'm happy with this description.

> 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 48d127a..3814a6a 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];
>  	struct regulator	*vdd;
>  };
>  
> @@ -714,6 +715,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,					\
> @@ -733,6 +756,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},
> @@ -802,6 +831,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);
> @@ -844,7 +889,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;
>  
> 

Re: [PATCH v3 2/3] iio:magnetometer:ak8975: mounting matrix support

[Rob Herring]

On Thu, Mar 17, 2016 at 11:43 AM, Gregor Boirie
<gregor.boirie-ITF29qwbsa/QT0dZR+AlfA@public.gmane.org> 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.

Why is the sysfs interface specific to ak8975?

Furthermore, why is it specific to magnetometer? Couldn't
accelerometers need the same thing? There's a thread discussing a
similar matrix on android-x86[1].


> 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

Perhaps "rotation-matrx" would be a better name in case there's ever
any other matrix needed.

Rob

[1] https://groups.google.com/forum/#!msg/android-x86/LaKhV8cT69o/G1rrVU0_JAAJ

Re: [PATCH v3 2/3] iio:magnetometer:ak8975: mounting matrix support

[Jonathan Cameron]

Backed out patches 2 and 3 for now whilst discussion continues.

On 21/03/16 14:58, Rob Herring wrote:
> On Thu, Mar 17, 2016 at 11:43 AM, Gregor Boirie
> <gregor.boirie-ITF29qwbsa/QT0dZR+AlfA@public.gmane.org> 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.
> 
> Why is the sysfs interface specific to ak8975?
> 
> Furthermore, why is it specific to magnetometer? Couldn't
> accelerometers need the same thing? There's a thread discussing a
> similar matrix on android-x86[1].
> 
> 
>> 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
> 
> Perhaps "rotation-matrx" would be a better name in case there's ever
> any other matrix needed.
> 
> Rob
> 
> [1] https://groups.google.com/forum/#!msg/android-x86/LaKhV8cT69o/G1rrVU0_JAAJ
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v3 2/3] iio:magnetometer:ak8975: mounting matrix support

[Gregor Boirie]

Hi Rob,

On Mon, Mar 21, 2016 at 09:58:46AM -0500, Rob Herring wrote:
> On Thu, Mar 17, 2016 at 11:43 AM, Gregor Boirie
> <gregor.boirie-ITF29qwbsa/QT0dZR+AlfA@public.gmane.org> 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.
>
> Why is the sysfs interface specific to ak8975?
AFAIK, this is just the first IIO driver implementation relying on floating
point numbers. Should a single driver be enough to justify a "generic" API,
I suppose code could be factored out in a similar way to over sampling rate
support. People could call this on a per-driver basis.

>
> Furthermore, why is it specific to magnetometer? Couldn't
> accelerometers need the same thing? There's a thread discussing a
> similar matrix on android-x86[1].
Same may apply to gyro / accelero / imu and magnetometers at least.
inv_mpu_core.c already implements such a rotation matrix exposed as 3x3 integers
array. Should we be smart enough to keep this compatible with existing userspace
apps ?

>
> > 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
>
> Perhaps "rotation-matrx" would be a better name in case there's ever
> any other matrix needed.
What about "mounting-matrix" ?
> 
> Rob
> 
> [1] https://groups.google.com/forum/#!msg/android-x86/LaKhV8cT69o/G1rrVU0_JAAJ

Re: [PATCH v3 2/3] iio:magnetometer:ak8975: mounting matrix support

[Rob Herring]

On Mon, Mar 21, 2016 at 5:21 PM, Gregor Boirie <gregor.boirie-ITF29qwbsa/QT0dZR+AlfA@public.gmane.org> wrote:
> Hi Rob,
>
> On Mon, Mar 21, 2016 at 09:58:46AM -0500, Rob Herring wrote:
>> On Thu, Mar 17, 2016 at 11:43 AM, Gregor Boirie
>> <gregor.boirie-ITF29qwbsa/QT0dZR+AlfA@public.gmane.org> 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.
>>
>> Why is the sysfs interface specific to ak8975?
> AFAIK, this is just the first IIO driver implementation relying on floating
> point numbers. Should a single driver be enough to justify a "generic" API,
> I suppose code could be factored out in a similar way to over sampling rate
> support. People could call this on a per-driver basis.

Given it is an ABI, yes I think so. We don't want to end up with a
bunch of similar yet different interfaces.

>> Furthermore, why is it specific to magnetometer? Couldn't
>> accelerometers need the same thing? There's a thread discussing a
>> similar matrix on android-x86[1].
> Same may apply to gyro / accelero / imu and magnetometers at least.
> inv_mpu_core.c already implements such a rotation matrix exposed as 3x3 integers
> array. Should we be smart enough to keep this compatible with existing userspace
> apps ?

You have to maintain the ABI. If both interfaces can co-exist, then
you can have both and mark the old one as deprecated. In time we can
remove the old one.

>> > 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
>>
>> Perhaps "rotation-matrx" would be a better name in case there's ever
>> any other matrix needed.
> What about "mounting-matrix" ?

Sure.

Rob

Re: [PATCH v3 2/3] iio:magnetometer:ak8975: mounting matrix support

[Jonathan Cameron]

On 22/03/16 12:38, Rob Herring wrote:
> On Mon, Mar 21, 2016 at 5:21 PM, Gregor Boirie <gregor.boirie-ITF29qwbsa/QT0dZR+AlfA@public.gmane.org> wrote:
>> Hi Rob,
>>
>> On Mon, Mar 21, 2016 at 09:58:46AM -0500, Rob Herring wrote:
>>> On Thu, Mar 17, 2016 at 11:43 AM, Gregor Boirie
>>> <gregor.boirie-ITF29qwbsa/QT0dZR+AlfA@public.gmane.org> 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.
>>>
>>> Why is the sysfs interface specific to ak8975?
>> AFAIK, this is just the first IIO driver implementation relying on floating
>> point numbers. Should a single driver be enough to justify a "generic" API,
>> I suppose code could be factored out in a similar way to over sampling rate
>> support. People could call this on a per-driver basis.
> 
> Given it is an ABI, yes I think so. We don't want to end up with a
> bunch of similar yet different interfaces.
> 
Absolutely.  Most interfaces get made up based on one implementation ;)
A second is always nice, but here the interface is obvious enough we don't
need to wait.
>>> Furthermore, why is it specific to magnetometer? Couldn't
>>> accelerometers need the same thing? There's a thread discussing a
>>> similar matrix on android-x86[1].
>> Same may apply to gyro / accelero / imu and magnetometers at least.
>> inv_mpu_core.c already implements such a rotation matrix exposed as 3x3 integers
>> array. Should we be smart enough to keep this compatible with existing userspace
>> apps ?
> 
> You have to maintain the ABI. If both interfaces can co-exist, then
> you can have both and mark the old one as deprecated. In time we can
> remove the old one.
If you want to (or someone else does) it would be good to add the new abi to
inv_mpu_core as well and indeed mark the old one as deprecated. 
The cost of keeping it is negligible, so we may never actually bother to
remove it. 
> 
>>>> 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
>>>
>>> Perhaps "rotation-matrx" would be a better name in case there's ever
>>> any other matrix needed.
>> What about "mounting-matrix" ?
> 
> Sure.
Works for me as well.
> 
> Rob
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v3 2/3] iio:magnetometer:ak8975: mounting matrix support

[Gregor Boirie]

On 03/28/2016 05:03 PM, Jonathan Cameron wrote:

[snip...]

>
> Why is the sysfs interface specific to ak8975?
>>> AFAIK, this is just the first IIO driver implementation relying on floating
>>> point numbers. Should a single driver be enough to justify a "generic" API,
>>> I suppose code could be factored out in a similar way to over sampling rate
>>> support. People could call this on a per-driver basis.
>> Given it is an ABI, yes I think so. We don't want to end up with a
>> bunch of similar yet different interfaces.
>>
> Absolutely.  Most interfaces get made up based on one implementation ;)
> A second is always nice, but here the interface is obvious enough we don't
> need to wait.
Right. I'll post a separate patch then.
>>>> Furthermore, why is it specific to magnetometer? Couldn't
>>>> accelerometers need the same thing? There's a thread discussing a
>>>> similar matrix on android-x86[1].
>>> Same may apply to gyro / accelero / imu and magnetometers at least.
>>> inv_mpu_core.c already implements such a rotation matrix exposed as 3x3 integers
>>> array. Should we be smart enough to keep this compatible with existing userspace
>>> apps ?
>> You have to maintain the ABI. If both interfaces can co-exist, then
>> you can have both and mark the old one as deprecated. In time we can
>> remove the old one.
> If you want to (or someone else does) it would be good to add the new abi to
> inv_mpu_core as well and indeed mark the old one as deprecated.
> The cost of keeping it is negligible, so we may never actually bother to
> remove it.
What's the better way to mark this as deprecated ? dev_warn() ? 
__deprecated ?
Anything else ?
>>>>> 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
>>>> Perhaps "rotation-matrx" would be a better name in case there's ever
>>>> any other matrix needed.
>>> What about "mounting-matrix" ?
>> Sure.
> Works for me as well.
>> Rob
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>

Re: [PATCH v3 2/3] iio:magnetometer:ak8975: mounting matrix support

[Jonathan Cameron]

On 29/03/16 10:44, Gregor Boirie wrote:
> 
> 
> On 03/28/2016 05:03 PM, Jonathan Cameron wrote:
> 
> [snip...]
> 
>>
>> Why is the sysfs interface specific to ak8975?
>>>> AFAIK, this is just the first IIO driver implementation relying on floating
>>>> point numbers. Should a single driver be enough to justify a "generic" API,
>>>> I suppose code could be factored out in a similar way to over sampling rate
>>>> support. People could call this on a per-driver basis.
>>> Given it is an ABI, yes I think so. We don't want to end up with a
>>> bunch of similar yet different interfaces.
>>>
>> Absolutely.  Most interfaces get made up based on one implementation ;)
>> A second is always nice, but here the interface is obvious enough we don't
>> need to wait.
> Right. I'll post a separate patch then.
>>>>> Furthermore, why is it specific to magnetometer? Couldn't
>>>>> accelerometers need the same thing? There's a thread discussing a
>>>>> similar matrix on android-x86[1].
>>>> Same may apply to gyro / accelero / imu and magnetometers at least.
>>>> inv_mpu_core.c already implements such a rotation matrix exposed as 3x3 integers
>>>> array. Should we be smart enough to keep this compatible with existing userspace
>>>> apps ?
>>> You have to maintain the ABI. If both interfaces can co-exist, then
>>> you can have both and mark the old one as deprecated. In time we can
>>> remove the old one.
>> If you want to (or someone else does) it would be good to add the new abi to
>> inv_mpu_core as well and indeed mark the old one as deprecated.
>> The cost of keeping it is negligible, so we may never actually bother to
>> remove it.
> What's the better way to mark this as deprecated ? dev_warn() ? __deprecated ?
> Anything else ?
Probably just documentation at this stage - anything else will be a pita for
anyone using that driver. As long as it is clear in any docs + the code, which
is the preferred option that's fine.  I doubt we'll ever bother removing the
old way.

Jonathan
>>>>>> 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
>>>>> Perhaps "rotation-matrx" would be a better name in case there's ever
>>>>> any other matrix needed.
>>>> What about "mounting-matrix" ?
>>> Sure.
>> Works for me as well.
>>> Rob
>>> -- 
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 3/3] 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-ITF29qwbsa/QT0dZR+AlfA@public.gmane.org>
---
 drivers/iio/magnetometer/Kconfig  |   2 +
 drivers/iio/magnetometer/ak8975.c | 135 +++++++++++++++++++++++++++++++-------
 2 files changed, 112 insertions(+), 25 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 3814a6a..de1663d 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.
@@ -634,22 +640,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);
+	int ret = ak8975_set_mode(data, MODE_ONCE);
+
 	if (ret < 0) {
 		dev_err(&client->dev, "Error in setting operating mode\n");
-		goto exit;
+		return ret;
 	}
 
 	/* Wait for the conversion to complete. */
@@ -660,7 +659,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]) {
@@ -668,32 +667,45 @@ 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 iio_dev *indio_dev, int index, int *val)
+{
+	struct ak8975_data *data = iio_priv(indio_dev);
+	const struct i2c_client *client = data->client;
+	const struct ak_def *def = data->def;
+	int ret;
+
+	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;
 }
 
@@ -745,12 +757,22 @@ 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,				\
+			.endianness = IIO_CPU				\
+		}							\
 	}
 
 static const struct iio_chan_spec ak8975_channels[] = {
 	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,
@@ -785,6 +807,56 @@ static const char *ak8975_match_acpi_device(struct device *dev,
 	return dev_name(dev);
 }
 
+static void ak8975_fill_buffer(struct iio_dev *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());
+	return;
+
+unlock:
+	mutex_unlock(&data->lock);
+	dev_err(&client->dev, "Error in reading axes block\n");
+}
+
+static irqreturn_t ak8975_handle_trigger(int irq, void *p)
+{
+	const struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+
+	ak8975_fill_buffer(indio_dev);
+	iio_trigger_notify_done(indio_dev->trig);
+	return IRQ_HANDLED;
+}
+
 static int ak8975_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
@@ -889,15 +961,27 @@ 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;
 
-	err = iio_device_register(indio_dev);
-	if (err)
+	err = iio_triggered_buffer_setup(indio_dev, NULL, ak8975_handle_trigger,
+					 NULL);
+	if (err) {
+		dev_err(&client->dev, "triggered buffer setup failed\n");
 		goto power_off;
+	}
+
+	err = iio_device_register(indio_dev);
+	if (err) {
+		dev_err(&client->dev, "device register failed\n");
+		goto cleanup_buffer;
+	}
 
 	return 0;
 
+cleanup_buffer:
+	iio_triggered_buffer_cleanup(indio_dev);
 power_off:
 	ak8975_power_off(client);
 	return err;
@@ -908,6 +992,7 @@ 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);
 	ak8975_power_off(client);
 
 	return 0;
-- 
2.1.4

Re: [PATCH v3 3/3] iio:magnetometer:ak8975: triggered buffer support

[Jonathan Cameron]

On 17/03/16 16:43, 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-ITF29qwbsa/QT0dZR+AlfA@public.gmane.org>
Applied.

I had a slight argument with myself over the data resolution.  Its actually
only 13bits, but they pad the rest of the register out to 16 bits.  Hence
whilst technically misleading userspace will have an easier time of it if
we claim it was 16bits in the first place (no need to sign extend)

So have left it as is.

Jonathan
> ---
>  drivers/iio/magnetometer/Kconfig  |   2 +
>  drivers/iio/magnetometer/ak8975.c | 135 +++++++++++++++++++++++++++++++-------
>  2 files changed, 112 insertions(+), 25 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 3814a6a..de1663d 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.
> @@ -634,22 +640,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);
> +	int ret = ak8975_set_mode(data, MODE_ONCE);
> +
>  	if (ret < 0) {
>  		dev_err(&client->dev, "Error in setting operating mode\n");
> -		goto exit;
> +		return ret;
>  	}
>  
>  	/* Wait for the conversion to complete. */
> @@ -660,7 +659,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]) {
> @@ -668,32 +667,45 @@ 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 iio_dev *indio_dev, int index, int *val)
> +{
> +	struct ak8975_data *data = iio_priv(indio_dev);
> +	const struct i2c_client *client = data->client;
> +	const struct ak_def *def = data->def;
> +	int ret;
> +
> +	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;
>  }
>  
> @@ -745,12 +757,22 @@ 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,					\
Data sheet says 13bit.  Why are we claiming 16? Obviously doesn't have
much practical effect if the top 3 bits are simply the sign bit (seems to
be the case).  Anyhow, lets leave this be as it will save userspace doing
the sign extend and it is clearly documented that the top 3 bits will be
correct.
> +			.storagebits = 16,				\
> +			.endianness = IIO_CPU				\
> +		}							\
>  	}
>  
>  static const struct iio_chan_spec ak8975_channels[] = {
>  	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,
> @@ -785,6 +807,56 @@ static const char *ak8975_match_acpi_device(struct device *dev,
>  	return dev_name(dev);
>  }
>  
> +static void ak8975_fill_buffer(struct iio_dev *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());
> +	return;
> +
> +unlock:
> +	mutex_unlock(&data->lock);
> +	dev_err(&client->dev, "Error in reading axes block\n");
> +}
> +
> +static irqreturn_t ak8975_handle_trigger(int irq, void *p)
> +{
> +	const struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +
> +	ak8975_fill_buffer(indio_dev);
> +	iio_trigger_notify_done(indio_dev->trig);
> +	return IRQ_HANDLED;
> +}
> +
>  static int ak8975_probe(struct i2c_client *client,
>  			const struct i2c_device_id *id)
>  {
> @@ -889,15 +961,27 @@ 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;
>  
> -	err = iio_device_register(indio_dev);
> -	if (err)
> +	err = iio_triggered_buffer_setup(indio_dev, NULL, ak8975_handle_trigger,
> +					 NULL);
> +	if (err) {
> +		dev_err(&client->dev, "triggered buffer setup failed\n");
>  		goto power_off;
> +	}
> +
> +	err = iio_device_register(indio_dev);
> +	if (err) {
> +		dev_err(&client->dev, "device register failed\n");
> +		goto cleanup_buffer;
> +	}
>  
>  	return 0;
>  
> +cleanup_buffer:
> +	iio_triggered_buffer_cleanup(indio_dev);
>  power_off:
>  	ak8975_power_off(client);
>  	return err;
> @@ -908,6 +992,7 @@ 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);
>  	ak8975_power_off(client);
>  
>  	return 0;
>