[PATCH v1 0/6] iio:magnetometer:ak8975: fix and enhancements

[PATCH v1 0/6] iio:magnetometer:ak8975: fix and enhancements

[Gregor Boirie]

Various fixes and enhancements for the ak8975 magnetometers family driver.

Regards,
Gregor.

Gregor Boirie (6):
  iio:magnetometer:ak8975: fix uninitialized chipset
  iio:magnetometer:ak8975: remove unused field
  iio:magnetometer:ak8975: use explicit endianness
  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                  | 273 ++++++++++++++++++---
 3 files changed, 255 insertions(+), 32 deletions(-)

-- 
2.1.4

[PATCH v1 1/6] 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

[PATCH v1 2/6] 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

[PATCH v1 3/6] iio:magnetometer:ak8975: use explicit endianness

[Gregor Boirie]

ak8963, ak8975, ak09911 and ak09912 all store measurements in 2's
complement and little endian.
This enforces le16 to CPU endianness conversion of raw measurements
fetched from I2C bus.

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

diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
index 896b13e3..46c6bd7 100644
--- a/drivers/iio/magnetometer/ak8975.c
+++ b/drivers/iio/magnetometer/ak8975.c
@@ -655,7 +655,8 @@ static int ak8975_read_axis(struct iio_dev *indio_dev, int index, int *val)
 	mutex_unlock(&data->lock);
 
 	/* Clamp to valid range. */
-	*val = clamp_t(s16, ret, -data->def->range, data->def->range);
+	*val = clamp_t(s16, le16_to_cpu((s16)ret), -data->def->range,
+		       data->def->range);
 	return IIO_VAL_INT;
 
 exit:
-- 
2.1.4

[PATCH v1 4/6] 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 46c6bd7..141954a 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

[PATCH v1 5/6] 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 141954a..89a8ece 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];
 };
 
 /*
@@ -698,6 +699,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,					\
@@ -717,6 +740,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},
@@ -786,6 +815,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);
@@ -823,7 +868,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

[PATCH v1 6/6] iio:magnetometer:ak8975: triggered buffer support

[Gregor Boirie]

This will be used together with an external trigger (e.g hrtimer based
software trigger).
Samples of current acquisition round are cached in RAM in order to allow
simultaneous userspace access to buffer content and synchronous
"read_raw" API.

Signed-off-by: Gregor Boirie <gregor.boirie@parrot.com>
---
 drivers/iio/magnetometer/Kconfig  |   2 +
 drivers/iio/magnetometer/ak8975.c | 206 ++++++++++++++++++++++++++++++++------
 2 files changed, 178 insertions(+), 30 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 89a8ece..530347c 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.
@@ -358,9 +364,13 @@ static const struct ak_def ak_def_array[AK_MAX_TYPE] = {
 
 /*
  * Per-instance context data for the device.
+ *
+ * Note : buff holds cached values of channel samples. It is large enough to
+ *        contain 3 x 16 bits axes values and 1 x aligned 64 bits timestamp.
  */
 struct ak8975_data {
 	struct i2c_client	*client;
+	s16			buff[8];
 	const struct ak_def	*def;
 	struct mutex		lock;
 	u8			asa[3];
@@ -617,22 +627,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 +646,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,29 +654,61 @@ 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]);
+	return 0;
+}
+
+static int ak8975_fetch_axis(const struct i2c_client *client,
+			     const struct ak_def *def,
+			     int index,
+			     s16 *axis)
+{
+	int ret = i2c_smbus_read_word_data(client, def->data_regs[index]);
+
 	if (ret < 0) {
-		dev_err(&client->dev, "Read axis data fails\n");
-		goto exit;
+		dev_err(&client->dev, "Axis data fetch failed\n");
+		return ret;
+	}
+
+	/* Clamp to valid range. */
+	*axis = clamp_t(s16, le16_to_cpu((s16)ret), -def->range, def->range);
+	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, struct ak8975_data *data,
+			    int index, int *val)
+{
+	int ret;
+	s16 *axis = &data->buff[index];
+
+	mutex_lock(&data->lock);
+
+	if (!iio_buffer_enabled(indio_dev)) {
+		const struct i2c_client *client = data->client;
+
+		ret = ak8975_start_read_axis(data, client);
+		if (ret)
+			goto exit;
+
+		ret = ak8975_fetch_axis(client, data->def, index, axis);
+		if (ret < 0)
+			goto exit;
 	}
 
 	mutex_unlock(&data->lock);
 
-	/* Clamp to valid range. */
-	*val = clamp_t(s16, le16_to_cpu((s16)ret), -data->def->range,
-		       data->def->range);
+	*val = *axis;
 	return IIO_VAL_INT;
 
 exit:
@@ -690,10 +725,10 @@ 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(indio_dev, data, chan->scan_index, val);
 	case IIO_CHAN_INFO_SCALE:
 		*val = 0;
-		*val2 = data->raw_to_gauss[chan->address];
+		*val2 = data->raw_to_gauss[chan->scan_index];
 		return IIO_VAL_INT_PLUS_MICRO;
 	}
 	return -EINVAL;
@@ -728,13 +763,25 @@ static const struct attribute_group ak8975_attrs_group = {
 		.channel2 = IIO_MOD_##axis,				\
 		.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_LE				\
+		}							\
 	}
 
 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,
@@ -769,6 +816,80 @@ static const char *ak8975_match_acpi_device(struct device *dev,
 	return dev_name(dev);
 }
 
+static int ak8975_fetch_all(const struct i2c_client *client,
+			    struct ak8975_data *data)
+{
+	int ret;
+	s16 x, y, z;
+
+	ret = ak8975_start_read_axis(data, client);
+	if (ret)
+		return ret;
+
+	/*
+	 * For each axis, read the flux value from the appropriate register
+	 * (the register is specified in the iio device attributes).
+	 * Use a temporary storage to preserve 3D coordinate consistency.
+	 */
+	ret = ak8975_fetch_axis(client, data->def, 0, &x);
+	if (ret)
+		return ret;
+	ret = ak8975_fetch_axis(client, data->def, 1, &y);
+	if (ret)
+		return ret;
+	ret = ak8975_fetch_axis(client, data->def, 2, &z);
+	if (ret)
+		return ret;
+
+	data->buff[0] = x;
+	data->buff[1] = y;
+	data->buff[2] = z;
+
+	return 0;
+}
+
+static int ak8975_buffer_postenable(struct iio_dev *indio_dev)
+{
+	struct ak8975_data *data = iio_priv(indio_dev);
+	int ret;
+
+	/*
+	 * Update channels so that a client getting samples through read_raw
+	 * retrieves valid data when buffering has just been enabled but first
+	 * sampling round is not yet completed.
+	 */
+	mutex_lock(&data->lock);
+	ret = ak8975_fetch_all(data->client, data);
+	mutex_unlock(&data->lock);
+	if (ret)
+		return ret;
+
+	return iio_triggered_buffer_postenable(indio_dev);
+}
+
+static const struct iio_buffer_setup_ops ak8975_buffer_ops = {
+	.postenable = ak8975_buffer_postenable,
+	.predisable = &iio_triggered_buffer_predisable
+};
+
+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);
+	int ret;
+
+	mutex_lock(&data->lock);
+	ret = ak8975_fetch_all(data->client, data);
+	mutex_unlock(&data->lock);
+
+	if (!ret)
+		iio_push_to_buffers_with_timestamp(indio_dev, data->buff,
+						   iio_get_time_ns());
+	iio_trigger_notify_done(indio_dev->trig);
+	return IRQ_HANDLED;
+}
+
 static int ak8975_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
@@ -868,9 +989,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,
+					 &ak8975_buffer_ops);
+	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[] = {
@@ -904,6 +1049,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 v1 6/6] iio:magnetometer:ak8975: triggered buffer support

[Peter Meerwald-Stadler]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 11290 bytes --]

On Wed, 2 Mar 2016, Gregor Boirie wrote:

> This will be used together with an external trigger (e.g hrtimer based
> software trigger).
> Samples of current acquisition round are cached in RAM in order to allow
> simultaneous userspace access to buffer content and synchronous
> "read_raw" API.

comments below
 
> Signed-off-by: Gregor Boirie <gregor.boirie@parrot.com>
> ---
>  drivers/iio/magnetometer/Kconfig  |   2 +
>  drivers/iio/magnetometer/ak8975.c | 206 ++++++++++++++++++++++++++++++++------
>  2 files changed, 178 insertions(+), 30 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 89a8ece..530347c 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.
> @@ -358,9 +364,13 @@ static const struct ak_def ak_def_array[AK_MAX_TYPE] = {
>  
>  /*
>   * Per-instance context data for the device.
> + *
> + * Note : buff holds cached values of channel samples. It is large enough to
> + *        contain 3 x 16 bits axes values and 1 x aligned 64 bits timestamp.
>   */
>  struct ak8975_data {
>  	struct i2c_client	*client;
> +	s16			buff[8];
>  	const struct ak_def	*def;
>  	struct mutex		lock;
>  	u8			asa[3];
> @@ -617,22 +627,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 +646,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,29 +654,61 @@ 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]);
> +	return 0;
> +}
> +
> +static int ak8975_fetch_axis(const struct i2c_client *client,
> +			     const struct ak_def *def,
> +			     int index,
> +			     s16 *axis)
> +{
> +	int ret = i2c_smbus_read_word_data(client, def->data_regs[index]);
> +
>  	if (ret < 0) {
> -		dev_err(&client->dev, "Read axis data fails\n");
> -		goto exit;
> +		dev_err(&client->dev, "Axis data fetch failed\n");
> +		return ret;
> +	}
> +
> +	/* Clamp to valid range. */
> +	*axis = clamp_t(s16, le16_to_cpu((s16)ret), -def->range, def->range);

this looks wrong; i2c_smbus_read_word_data() reads two bytes (on chip 
in little endian) and converts them to a word (in cpu endianness), so 
le16_to_cpu() in not necessary

so NAK on patch 3/6

> +	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, struct ak8975_data *data,
> +			    int index, int *val)
> +{
> +	int ret;
> +	s16 *axis = &data->buff[index];
> +
> +	mutex_lock(&data->lock);
> +
> +	if (!iio_buffer_enabled(indio_dev)) {

most drivers return -EBUSY when buffering is enabled and a single read is 
performed

> +		const struct i2c_client *client = data->client;
> +
> +		ret = ak8975_start_read_axis(data, client);
> +		if (ret)
> +			goto exit;
> +
> +		ret = ak8975_fetch_axis(client, data->def, index, axis);
> +		if (ret < 0)
> +			goto exit;
>  	}
>  
>  	mutex_unlock(&data->lock);
>  
> -	/* Clamp to valid range. */
> -	*val = clamp_t(s16, le16_to_cpu((s16)ret), -data->def->range,
> -		       data->def->range);
> +	*val = *axis;
>  	return IIO_VAL_INT;
>  
>  exit:
> @@ -690,10 +725,10 @@ 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(indio_dev, data, chan->scan_index, val);
>  	case IIO_CHAN_INFO_SCALE:
>  		*val = 0;
> -		*val2 = data->raw_to_gauss[chan->address];
> +		*val2 = data->raw_to_gauss[chan->scan_index];

read_raw() is about reading without buffering, but scan_index relates to 
buffering -- probably a reason to keep .address

>  		return IIO_VAL_INT_PLUS_MICRO;
>  	}
>  	return -EINVAL;
> @@ -728,13 +763,25 @@ static const struct attribute_group ak8975_attrs_group = {
>  		.channel2 = IIO_MOD_##axis,				\
>  		.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_LE				\

fetch_axis() does endianness conversion, so should be 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,
> @@ -769,6 +816,80 @@ static const char *ak8975_match_acpi_device(struct device *dev,
>  	return dev_name(dev);
>  }
>  
> +static int ak8975_fetch_all(const struct i2c_client *client,
> +			    struct ak8975_data *data)
> +{
> +	int ret;
> +	s16 x, y, z;

I'd rather not use the per-device cache/buffer; if buffer mode is enable, 
read_raw() simply returns -EBUSY
why not use have a local variable here and save the copying?

> +
> +	ret = ak8975_start_read_axis(data, client);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * For each axis, read the flux value from the appropriate register
> +	 * (the register is specified in the iio device attributes).
> +	 * Use a temporary storage to preserve 3D coordinate consistency.
> +	 */

it is rather inefficient to set up three separate i2c transactions; can't 
the data be transferred in one go?

> +	ret = ak8975_fetch_axis(client, data->def, 0, &x);
> +	if (ret)

fetch axis does

> +		return ret;
> +	ret = ak8975_fetch_axis(client, data->def, 1, &y);
> +	if (ret)
> +		return ret;
> +	ret = ak8975_fetch_axis(client, data->def, 2, &z);
> +	if (ret)
> +		return ret;
> +
> +	data->buff[0] = x;
> +	data->buff[1] = y;
> +	data->buff[2] = z;
> +
> +	return 0;
> +}
> +
> +static int ak8975_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct ak8975_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	/*
> +	 * Update channels so that a client getting samples through read_raw
> +	 * retrieves valid data when buffering has just been enabled but first
> +	 * sampling round is not yet completed.
> +	 */

this function is not needed if -EBUSY is returned in read_raw when 
buffering is enabled

> +	mutex_lock(&data->lock);
> +	ret = ak8975_fetch_all(data->client, data);
> +	mutex_unlock(&data->lock);
> +	if (ret)
> +		return ret;
> +
> +	return iio_triggered_buffer_postenable(indio_dev);
> +}
> +
> +static const struct iio_buffer_setup_ops ak8975_buffer_ops = {
> +	.postenable = ak8975_buffer_postenable,
> +	.predisable = &iio_triggered_buffer_predisable
> +};
> +
> +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);
> +	int ret;
> +
> +	mutex_lock(&data->lock);
> +	ret = ak8975_fetch_all(data->client, data);
> +	mutex_unlock(&data->lock);
> +
> +	if (!ret)
> +		iio_push_to_buffers_with_timestamp(indio_dev, data->buff,
> +						   iio_get_time_ns());
> +	iio_trigger_notify_done(indio_dev->trig);
> +	return IRQ_HANDLED;
> +}
> +
>  static int ak8975_probe(struct i2c_client *client,
>  			const struct i2c_device_id *id)
>  {
> @@ -868,9 +989,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,
> +					 &ak8975_buffer_ops);
> +	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[] = {
> @@ -904,6 +1049,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);
> 

-- 

Peter Meerwald-Stadler
+43-664-2444418 (mobile)

Re: [PATCH v1 6/6] iio:magnetometer:ak8975: triggered buffer support

[Gregor Boirie]

On 03/02/2016 12:36 PM, Peter Meerwald-Stadler wrote:
> On Wed, 2 Mar 2016, Gregor Boirie wrote:
>
>> This will be used together with an external trigger (e.g hrtimer based
>> software trigger).
>> Samples of current acquisition round are cached in RAM in order to allow
>> simultaneous userspace access to buffer content and synchronous
>> "read_raw" API.
> comments below
>   
>> Signed-off-by: Gregor Boirie <gregor.boirie@parrot.com>
>> ---
>>   drivers/iio/magnetometer/Kconfig  |   2 +
>>   drivers/iio/magnetometer/ak8975.c | 206 ++++++++++++++++++++++++++++++++------
>>   2 files changed, 178 insertions(+), 30 deletions(-)
[snip...]
>> +
>> +	/* Clamp to valid range. */
>> +	*axis = clamp_t(s16, le16_to_cpu((s16)ret), -def->range, def->range);
> this looks wrong; i2c_smbus_read_word_data() reads two bytes (on chip
> in little endian) and converts them to a word (in cpu endianness), so
> le16_to_cpu() in not necessary
>
> so NAK on patch 3/6
oops... silly mistake !
>> +	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, struct ak8975_data *data,
>> +			    int index, int *val)
>> +{
>> +	int ret;
>> +	s16 *axis = &data->buff[index];
>> +
>> +	mutex_lock(&data->lock);
>> +
>> +	if (!iio_buffer_enabled(indio_dev)) {
> most drivers return -EBUSY when buffering is enabled and a single read is
> performed
 From a previous discussion here:
http://www.spinics.net/lists/linux-iio/msg22948.html

 From my understanding I have 2 options here:
1) simply return EBUSY when buffering is enabled as you suggest ;
2) lock for exclusive bus access and serialize accesses between trigger 
handler
   and read_raw.

I can see no reason to prevent from concurrent access. I would prefere 
getting
rid of this overly complex "caching" stuff and implement point 2).
Unless somebody states otherwise, I will follow your guidance here.

[snip...]
>> -		*val2 = data->raw_to_gauss[chan->address];
>> +		*val2 = data->raw_to_gauss[chan->scan_index];
> read_raw() is about reading without buffering, but scan_index relates to
> buffering -- probably a reason to keep .address
ok.

[snip...]
> +static int ak8975_fetch_all(const struct i2c_client *client,
> +			    struct ak8975_data *data)
> +{
> +	int ret;
> +	s16 x, y, z;
> I'd rather not use the per-device cache/buffer; if buffer mode is enable,
> read_raw() simply returns -EBUSY
> why not use have a local variable here and save the copying?
>
>> +
>> +	ret = ak8975_start_read_axis(data, client);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/*
>> +	 * For each axis, read the flux value from the appropriate register
>> +	 * (the register is specified in the iio device attributes).
>> +	 * Use a temporary storage to preserve 3D coordinate consistency.
>> +	 */
> it is rather inefficient to set up three separate i2c transactions; can't
> the data be transferred in one go?
Agreed.
>
>> +	ret = ak8975_fetch_axis(client, data->def, 0, &x);
>> +	if (ret)
> fetch axis does
>
>> +		return ret;
>> +	ret = ak8975_fetch_axis(client, data->def, 1, &y);
>> +	if (ret)
>> +		return ret;
>> +	ret = ak8975_fetch_axis(client, data->def, 2, &z);
>> +	if (ret)
>> +		return ret;
>> +
>> +	data->buff[0] = x;
>> +	data->buff[1] = y;
>> +	data->buff[2] = z;
>> +
>> +	return 0;
>> +}
>> +
>> +static int ak8975_buffer_postenable(struct iio_dev *indio_dev)
>> +{
>> +	struct ak8975_data *data = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	/*
>> +	 * Update channels so that a client getting samples through read_raw
>> +	 * retrieves valid data when buffering has just been enabled but first
>> +	 * sampling round is not yet completed.
>> +	 */
> this function is not needed if -EBUSY is returned in read_raw when
> buffering is enabled
>
>
see above.
[snip...]

Thanks.