iio:magnetometer:ak8975: fix and enhancements

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 <[email protected]>

---
 drivers/iio/magnetometer/Kconfig  |   2 +
 drivers/iio/magnetometer/ak8975.c | 206 ++++++++++++++++++++++++++++++++------
 2 files changed, 178 insertions(+), 30 deletions(-)

drivers/iio/magnetometer/Kconfig +2 -0

index 021dc53..d9834ed 100644
@@ -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.

drivers/iio/magnetometer/ak8975.c +28 -20

index 89a8ece..530347c 100644
@@ -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]);

drivers/iio/magnetometer/ak8975.c +5 -0

+	return 0;
+}
+
+/*
+ * Retrieve raw flux value for one of the x, y, or z axis.

drivers/iio/magnetometer/ak8975.c +10 -3

+		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:

drivers/iio/magnetometer/ak8975.c +0 -0

 		return IIO_VAL_INT_PLUS_MICRO;
 	}
 	return -EINVAL;

@@ -728,13 +763,25 @@ static const struct attribute_group ak8975_attrs_group = {
 		.channel2 = IIO_MOD_##axis,				\

drivers/iio/magnetometer/ak8975.c +7 -1

+		}							\
 	}
 
 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,

drivers/iio/magnetometer/ak8975.c +15 -0

+		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;
+}
+

drivers/iio/magnetometer/ak8975.c +58 -1

+	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);

Quoted 5 lines · 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.

comments below

Quoted 5 lines

Signed-off-by Gregor Boirie <[email protected]>

---
  drivers/iio/magnetometer/Kconfig  |   2 +
  drivers/iio/magnetometer/ak8975.c | 206 ++++++++++++++++++++++++++++++++------
  2 files changed, 178 insertions(+), 30 deletions(-)

drivers/iio/magnetometer/ak8975.c static int ak8975_read_axis(struct iio_dev *indio_dev, int index, int *val) · line 682

Quoted 3 lines

drivers/iio/magnetometer/ak8975.c +3 -0

+
+	/* 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

Quoted 9 lines

drivers/iio/magnetometer/ak8975.c +9 -0

+	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)
+{

drivers/iio/magnetometer/ak8975.c static int ak8975_read_axis(struct iio_dev *indio_dev, int index, int *val) · line 697

Quoted 6 lines

drivers/iio/magnetometer/ak8975.c +6 -0

+	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

drivers/iio/magnetometer/ak8975.c static int ak8975_read_raw(struct iio_dev *indio_dev, · line 731

Quoted 2 lines

drivers/iio/magnetometer/ak8975.c +1 -1

-		*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

drivers/iio/magnetometer/ak8975.c +5 -0

+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?

drivers/iio/magnetometer/ak8975.c static const char *ak8975_match_acpi_device(struct device *dev, · line 833

Quoted 10 lines

drivers/iio/magnetometer/ak8975.c +10 -0

+
+	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?

Review

Quoted 3 lines

(quoted diff) +2 -0

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

fetch axis does

Quoted 19 lines

drivers/iio/magnetometer/ak8975.c +19 -0

+		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;

drivers/iio/magnetometer/ak8975.c static const char *ak8975_match_acpi_device(struct device *dev, · line 860

Quoted 6 lines

drivers/iio/magnetometer/ak8975.c +6 -0

+
+	/*
+	 * 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