[PATCH v3 0/3] Add PAT9125 optical tracker driver

[PATCH v3 0/3] Add PAT9125 optical tracker driver

[Alexandre Mergnat]

PixArt Imaging PAT9125 is a miniature low power optical navigation chip
using LASER light source enabling digital surface tracking.

This device driver use IIO API to provide punctual and/or buffered data.
The data could be relative position or delta value, both on X and Y axis,
depend on CPI (Counts Per Inch) resolution setting chosen.

The device support configuration through module parameter interface.

This patchset :
- Update vendor prefix
- Add the bindings for this device
- Add the device driver
- Add directory for optical tracker devices

Change since v1:
- Fix typo
- Rename some defines / variables
- Remove I2C client from driver structure
- Change type of delta_x and delta_y from s16 to s32 to simplify signed
  operations
- Add module parameter for axis resolution
- Replace "IIO_MOD_X_AND_Y" by "IIO_MOD_X" and "IIO_MOD_Y"
- Add sign extension macro
- Improve read value algorithm to avoid data loss
- Implement a trigger handler function which can work with any IIO trigger,
  independently of it own GPIO IRQ, to match with IIO requirement/behaviour
- Replace iio push event function by iio trigger poll in GPIO IRQ handler
- Use triggered_buffer helpers to replace kfifo use, setup buffer,
  implement enable/disable setup buffer operations, IIO trigger allocation
  and re-enable operations
- Remove useless "goto"
- Change GPIO IRQ handler from planified thread to IRQ thread
- Change GPIO IRQ trigger from low level and one shot to falling edge
- Add device unregister and buffer cleanup to driver remove function

Change since v2:
- Fix typo
- Add constructor webpage and datasheet in commit message
- Use BIT() macro for define bit mask
- Remove shift from IIO channel spec structure
- Replace IIO_LE by IIO_CPU from IIO channel spec structure
- Replace memcpy() by cast (s32)
- Rename "pat9125_trig_try_reen" to "pat9125_trig_try_reenable"
- Add carriage return (\n) at the end of each "dev_err" function
- Remove "iio_trigger_unregister" in case of "iio_trigger_register" fail,
  register function already manage it
- Remove log which print device name in case of successful initialization
- Fix enabled IRQ flag warning during nested IRQ thread
- Improve retry algo now based on status register
- Remove "ts", "motion_detected" and "buffer_mode" from pat9125_data
  structure
- Rename all "ot" directories to "position"
- Polling sample through IIO_CHAN_INFO_RAW now return position value
  (relative to the position at initialization time) instead of delta
  position
- Clean iio_buffer_setup_ops structure by removing NULL pointer.
- Use devm_iio_ function for all init functions and then delete
  "pat9125_remove"
- Move device_register at the end of probe function
- Replace MODULE_PARM_DESC by IIO_SCALE to set axis resolution (CPI)

Alexandre Mergnat (3):
  dt-bindings: Add pixart vendor
  dt-bindings: iio: position: Add docs pat9125
  iio: Add PAT9125 optical tracker sensor

 .../bindings/iio/position/pat9125.txt         |  18 +
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 drivers/iio/Kconfig                           |   1 +
 drivers/iio/Makefile                          |   1 +
 drivers/iio/position/Kconfig                  |  18 +
 drivers/iio/position/Makefile                 |   6 +
 drivers/iio/position/pat9125.c                | 499 ++++++++++++++++++
 7 files changed, 545 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/position/pat9125.txt
 create mode 100644 drivers/iio/position/Kconfig
 create mode 100644 drivers/iio/position/Makefile
 create mode 100644 drivers/iio/position/pat9125.c

-- 
2.17.1

[PATCH v3 1/3] dt-bindings: Add pixart vendor

[Alexandre Mergnat]

PixArt Imaging Inc. is expertized in CMOS image sensors (CIS),
capacitive touch controllers and related imaging application development.

Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 33a65a45e319..ac5060e8de8d 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -661,6 +661,8 @@ patternProperties:
     description: Picochip Ltd
   "^pine64,.*":
     description: Pine64
+  "^pixart,.*":
+    description: PixArt Imaging Inc.
   "^pixcir,.*":
     description: PIXCIR MICROELECTRONICS Co., Ltd
   "^plantower,.*":
-- 
2.17.1

[PATCH v3 2/3] dt-bindings: iio: position: Add docs pat9125

[Alexandre Mergnat]

Add documentation for the optical tracker PAT9125 and
"position" directory for chip which can provides position data.

Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
---
 .../bindings/iio/position/pat9125.txt          | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/position/pat9125.txt

diff --git a/Documentation/devicetree/bindings/iio/position/pat9125.txt b/Documentation/devicetree/bindings/iio/position/pat9125.txt
new file mode 100644
index 000000000000..4028aeef9b42
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/position/pat9125.txt
@@ -0,0 +1,18 @@
+PixArt Imaging PAT9125 Optical Tracking Miniature Chip device driver
+
+Required properties:
+	- compatible: must be "pixart,pat9125"
+	- reg: i2c address where to find the device
+	- interrupts: the sole interrupt generated by the device
+
+	Refer to interrupt-controller/interrupts.txt for generic
+	interrupt client node bindings.
+
+Example:
+
+pat9125@75 {
+	compatible = "pixart,pat9125";
+	reg = <0x75>;
+	interrupt-parent = <&gpio3>;
+	interrupts = <12 IRQ_TYPE_EDGE_FALLING>;
+};
-- 
2.17.1

[PATCH v3 3/3] iio: Add PAT9125 optical tracker sensor

[Alexandre Mergnat]

This adds support for PixArt Imaging’s miniature low power optical
navigation chip using LASER light source enabling digital surface tracking.

Feature and datasheet: [0]

This IIO driver allows to read delta or relative position on X and Y axis:
  - The position relative to where the system started can be taken through
  punctual "read_raw" which will issue a read in the device registers to
  get the delta between last/current read and return the addition of all
  the deltas.
  - The delta can be retrieved using triggered buffer subscription
  (i.e. iio_readdev). The buffer payload is:
    |32 bits delta X|32 bits delta Y|timestamp|.

The possible I2C addresses are 0x73, 0x75 and 0x79.

X and Y axis CPI resolution can be get/set independently through IIO_SCALE.
The range value is 0-255 which means:
  - 0 to ~1,275 Counts Per Inch on flat surface.
  - 0 to ~630 Counts Per Rev on 1.0mm diameter STS shaft at 1.0mm distance.
More details on the datasheet.

The "position" directory is added to contain drivers which can provide
position data.

Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>

[0]: http://www.pixart.com/products-detail/72/PAT9125EL-TKIT___TKMT

Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
---
 drivers/iio/Kconfig            |   1 +
 drivers/iio/Makefile           |   1 +
 drivers/iio/position/Kconfig   |  18 ++
 drivers/iio/position/Makefile  |   6 +
 drivers/iio/position/pat9125.c | 499 +++++++++++++++++++++++++++++++++
 5 files changed, 525 insertions(+)
 create mode 100644 drivers/iio/position/Kconfig
 create mode 100644 drivers/iio/position/Makefile
 create mode 100644 drivers/iio/position/pat9125.c

diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
index 1d736a4952ab..23d9780640e7 100644
--- a/drivers/iio/Kconfig
+++ b/drivers/iio/Kconfig
@@ -85,6 +85,7 @@ source "drivers/iio/light/Kconfig"
 source "drivers/iio/magnetometer/Kconfig"
 source "drivers/iio/multiplexer/Kconfig"
 source "drivers/iio/orientation/Kconfig"
+source "drivers/iio/position/Kconfig"
 if IIO_TRIGGER
    source "drivers/iio/trigger/Kconfig"
 endif #IIO_TRIGGER
diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
index bff682ad1cfb..1712011c0f4a 100644
--- a/drivers/iio/Makefile
+++ b/drivers/iio/Makefile
@@ -31,6 +31,7 @@ obj-y += light/
 obj-y += magnetometer/
 obj-y += multiplexer/
 obj-y += orientation/
+obj-y += position/
 obj-y += potentiometer/
 obj-y += potentiostat/
 obj-y += pressure/
diff --git a/drivers/iio/position/Kconfig b/drivers/iio/position/Kconfig
new file mode 100644
index 000000000000..1cf28896511c
--- /dev/null
+++ b/drivers/iio/position/Kconfig
@@ -0,0 +1,18 @@
+#
+# Optical tracker sensors
+#
+# When adding new entries keep the list in alphabetical order
+
+menu "Optical tracker sensors"
+
+config PAT9125
+	tristate "Optical tracker PAT9125 I2C driver"
+	depends on I2C
+	select IIO_BUFFER
+	help
+	  Say yes here to build support for PAT9125 optical tracker
+	  sensors.
+
+          To compile this driver as a module, say M here: the module will
+          be called pat9125.
+endmenu
diff --git a/drivers/iio/position/Makefile b/drivers/iio/position/Makefile
new file mode 100644
index 000000000000..cf294917ae2c
--- /dev/null
+++ b/drivers/iio/position/Makefile
@@ -0,0 +1,6 @@
+#
+# Makefile for industrial I/O Optical tracker sensor drivers
+#
+
+# When adding new entries keep the list in alphabetical order
+obj-$(CONFIG_PAT9125) += pat9125.o
diff --git a/drivers/iio/position/pat9125.c b/drivers/iio/position/pat9125.c
new file mode 100644
index 000000000000..22bf729bec9b
--- /dev/null
+++ b/drivers/iio/position/pat9125.c
@@ -0,0 +1,499 @@
+// SPDX-License-Identifier: (GPL-2.0)
+/*
+ * Copyright (C) 2019 BayLibre, SAS
+ * Author: Alexandre Mergnat <amergnat@baylibre.com>
+ */
+
+#include <linux/bitops.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/i2c.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/events.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/iio/kfifo_buf.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+/* I2C Address function to ID pin*/
+#define PAT9125_I2C_ADDR_HI		0x73
+#define PAT9125_I2C_ADDR_LO		0x75
+#define PAT9125_I2C_ADDR_NC		0x79
+
+/* Registers */
+#define PAT9125_PRD_ID1_REG		0x00
+#define PAT9125_PRD_ID2_REG		0x01
+#define PAT9125_MOTION_STATUS_REG	0x02
+#define PAT9125_DELTA_X_LO_REG		0x03
+#define PAT9125_DELTA_Y_LO_REG		0x04
+#define PAT9125_OP_MODE_REG		0x05
+#define PAT9125_CONFIG_REG		0x06
+#define PAT9125_WRITE_PROTEC_REG	0x09
+#define PAT9125_SLEEP1_REG		0x0A
+#define PAT9125_SLEEP2_REG		0x0B
+#define PAT9125_RES_X_REG		0x0D
+#define PAT9125_RES_Y_REG		0x0E
+#define PAT9125_DELTA_XY_HI_REG		0x12
+#define PAT9125_SHUTER_REG		0x14
+#define PAT9125_FRAME_AVG_REG		0x17
+#define PAT9125_ORIENTATION_REG		0x19
+
+/* Bits */
+#define PAT9125_VALID_MOTION_DATA_BIT	BIT(7)
+#define PAT9125_RESET_BIT		BIT(7)
+
+/* Registers' values */
+#define PAT9125_SENSOR_ID_VAL			0x31
+#define PAT9125_DISABLE_WRITE_PROTECT_VAL	0x5A
+#define PAT9125_ENABLE_WRITE_PROTECT_VAL	0x00
+
+/* Default Value of sampled value size */
+#define PAT9125_SAMPLED_VAL_BIT_SIZE		12
+
+struct pat9125_data {
+	struct regmap *regmap;
+	struct iio_trigger *indio_trig;	/* Motion detection */
+	s32 delta_x;
+	s32 delta_y;
+	s32 position_x;
+	s32 position_y;
+	bool sampling;
+};
+
+static const struct iio_chan_spec pat9125_channels[] = {
+	{
+		.type = IIO_DISTANCE,
+		.modified = 1,
+		.channel2 = IIO_MOD_X,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+			BIT(IIO_CHAN_INFO_SCALE),
+		.scan_index = 0,
+		.scan_type = {
+			.sign = 's',
+			.realbits = 32,
+			.storagebits = 32,
+			.endianness = IIO_CPU,
+		},
+	},
+	{
+		.type = IIO_DISTANCE,
+		.modified = 1,
+		.channel2 = IIO_MOD_Y,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+			BIT(IIO_CHAN_INFO_SCALE),
+		.scan_index = 1,
+		.scan_type = {
+			.sign = 's',
+			.realbits = 32,
+			.storagebits = 32,
+			.endianness = IIO_CPU,
+		},
+	},
+	IIO_CHAN_SOFT_TIMESTAMP(2),
+};
+
+/**
+ * pat9125_write_pretected_reg() - Write value in protected register.
+ *
+ * @regmap: Pointer to I2C register map.
+ * @reg_addr: Register address.
+ * @reg_value: Value to be write in register.
+ *
+ * A value of zero will be returned on success, a negative errno will
+ * be returned in error cases.
+ */
+static int pat9125_write_pretected_reg(struct iio_dev *indio_dev,
+	u8 reg_addr, u8 reg_value)
+{
+	struct pat9125_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = regmap_write(data->regmap,
+			 PAT9125_WRITE_PROTEC_REG,
+			 PAT9125_DISABLE_WRITE_PROTECT_VAL);
+	if (ret < 0) {
+		dev_err(indio_dev->dev.parent, "register 0x%x access failed %d\n",
+			PAT9125_WRITE_PROTEC_REG, ret);
+		return ret;
+	}
+
+	ret = regmap_write(data->regmap, reg_addr, reg_value);
+	if (ret < 0) {
+		dev_err(indio_dev->dev.parent, "register 0x%x access failed %d\n",
+			reg_addr, ret);
+		return ret;
+	}
+
+	ret = regmap_write(data->regmap,
+			 PAT9125_WRITE_PROTEC_REG,
+			 PAT9125_ENABLE_WRITE_PROTECT_VAL);
+	if (ret < 0) {
+		dev_err(indio_dev->dev.parent, "register 0x%x access failed %d\n",
+			PAT9125_WRITE_PROTEC_REG, ret);
+		return ret;
+	}
+	return 0;
+}
+/**
+ * pat9125_read_delta() - Read delta value, update delta & position data.
+ *
+ * @data: Driver's data structure.
+ *
+ * A value of zero will be returned on success, a negative errno will
+ * be returned in error cases.
+ */
+static int pat9125_read_delta(struct pat9125_data *data)
+{
+	struct regmap *regmap = data->regmap;
+	int status = 0;
+	int val_x = 0;
+	int val_y = 0;
+	int val_high_nibbles = 0;
+	int ret;
+
+	ret = regmap_read(regmap, PAT9125_MOTION_STATUS_REG, &status);
+	if (ret < 0)
+		return ret;
+
+	/* Check if motion is detected */
+	if (status & PAT9125_VALID_MOTION_DATA_BIT) {
+		ret = regmap_read(regmap, PAT9125_DELTA_X_LO_REG, &val_x);
+		if (ret < 0)
+			return ret;
+
+		ret = regmap_read(regmap, PAT9125_DELTA_Y_LO_REG, &val_y);
+		if (ret < 0)
+			return ret;
+
+		ret = regmap_read(regmap, PAT9125_DELTA_XY_HI_REG,
+			&val_high_nibbles);
+		if (ret < 0)
+			return ret;
+
+		val_x |= (val_high_nibbles << 4) & 0xF00;
+		val_y |= (val_high_nibbles << 8) & 0xF00;
+		val_x = sign_extend32(val_x,
+			PAT9125_SAMPLED_VAL_BIT_SIZE - 1);
+		val_y = sign_extend32(val_y,
+			PAT9125_SAMPLED_VAL_BIT_SIZE - 1);
+		data->position_x += val_x;
+		data->position_y += val_y;
+		data->delta_x = val_x;
+		data->delta_y = val_y;
+	}
+	return 0;
+}
+
+/**
+ * pat9125_read_raw() - Sample and return the value(s)
+ * function to the associated channel info enum.
+ **/
+static int pat9125_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long mask)
+{
+	struct pat9125_data *data = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = pat9125_read_delta(data);
+		if (ret)
+			return ret;
+		switch (chan->channel2) {
+		case IIO_MOD_X:
+			*val = data->position_x;
+			return IIO_VAL_INT;
+		case IIO_MOD_Y:
+			*val = data->position_y;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->channel2) {
+		case IIO_MOD_X:
+			ret = regmap_read(data->regmap, PAT9125_RES_X_REG, val);
+			if (ret)
+				return ret;
+			else
+				return IIO_VAL_INT;
+		case IIO_MOD_Y:
+			ret = regmap_read(data->regmap, PAT9125_RES_Y_REG, val);
+			if (ret)
+				return ret;
+			else
+				return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+}
+
+/**
+ * pat9125_write_raw() - Write the value(s)
+ * function to the associated channel info enum.
+ **/
+static int pat9125_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan, int val,
+			     int val2, long mask)
+{
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->channel2) {
+		case IIO_MOD_X:
+			ret = pat9125_write_pretected_reg(indio_dev,
+				PAT9125_RES_X_REG, val);
+			return ret;
+		case IIO_MOD_Y:
+			ret = pat9125_write_pretected_reg(indio_dev,
+				PAT9125_RES_Y_REG, val);
+			return ret;
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+}
+
+static irqreturn_t pat9125_threaded_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct pat9125_data *data = iio_priv(indio_dev);
+	u8 buf[16]; /* Payload: Delta_X (4) | Delta_Y (4) | Timestamp (8) */
+	int ret;
+	s64 timestamp;
+
+	data->sampling = true;
+	ret = pat9125_read_delta(data);
+	if (ret) {
+		dev_err(indio_dev->dev.parent, "Read delta failed %d\n", ret);
+		return IRQ_NONE;
+	}
+	timestamp = iio_get_time_ns(indio_dev);
+	*((s32 *)&buf[0]) = data->delta_x;
+	*((s32 *)&buf[sizeof(s32)]) = data->delta_y;
+	data->delta_x = 0;
+	data->delta_y = 0;
+	iio_push_to_buffers_with_timestamp(indio_dev, buf, timestamp);
+	iio_trigger_notify_done(indio_dev->trig);
+	return IRQ_HANDLED;
+}
+
+/**
+ * pat9125_threaded_event_handler() - Threaded motion detection event handler
+ * @irq: The irq being handled.
+ * @private: struct iio_device pointer for the device.
+ */
+static irqreturn_t pat9125_threaded_event_handler(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct pat9125_data *data = iio_priv(indio_dev);
+
+	iio_trigger_poll_chained(data->indio_trig);
+	return IRQ_HANDLED;
+}
+
+static int pat9125_buffer_postenable(struct iio_dev *indio_dev)
+{
+	struct pat9125_data *data = iio_priv(indio_dev);
+	int ret = 0;
+
+	ret = iio_triggered_buffer_postenable(indio_dev);
+	if (ret)
+		return ret;
+	/* Release interrupt pin on the device */
+	return pat9125_read_delta(data);
+}
+
+static const struct iio_buffer_setup_ops pat9125_buffer_ops = {
+	.postenable = pat9125_buffer_postenable,
+	.predisable = iio_triggered_buffer_predisable,
+};
+
+
+static const struct regmap_config pat9125_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
+static const struct iio_info pat9125_info = {
+	.read_raw = pat9125_read_raw,
+	.write_raw = pat9125_write_raw,
+};
+
+/*
+ * To detect if a new value is available, register status is checked. This
+ * method is safer than using a flag on GPIO IRQ to track event while sampling
+ * because falling edge is missed when device trig just after a read reg value
+ * (that happen for fast motions or high CPI setting).
+ *
+ * Note: To avoid infinite loop in "iio_trigger_notify_done" when it si not in
+ * buffer mode and kernel warning due to nested IRQ thread,
+ * this function must return 0.
+ */
+static int pat9125_trig_try_reenable(struct iio_trigger *trig)
+{
+	struct pat9125_data *data = iio_trigger_get_drvdata(trig);
+	struct regmap *regmap = data->regmap;
+	int status = 0;
+
+	if (data->sampling) {
+		regmap_read(regmap, PAT9125_MOTION_STATUS_REG, &status);
+		if (status & PAT9125_VALID_MOTION_DATA_BIT) {
+			data->sampling = false;
+			iio_trigger_poll_chained(data->indio_trig);
+			return 0;
+		}
+	}
+	data->sampling = false;
+	return 0;
+}
+
+static const struct iio_trigger_ops pat9125_trigger_ops = {
+	.try_reenable = pat9125_trig_try_reenable,
+};
+
+static int pat9125_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id)
+{
+	struct pat9125_data *data;
+	struct iio_dev *indio_dev;
+	int ret, sensor_pid;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	if (!indio_dev) {
+		dev_err(&client->dev, "IIO device allocation failed\n");
+		return -ENOMEM;
+	}
+
+	data = iio_priv(indio_dev);
+	indio_dev->dev.parent = &client->dev;
+	indio_dev->name = id->name;
+	indio_dev->channels = pat9125_channels;
+	indio_dev->num_channels = ARRAY_SIZE(pat9125_channels);
+	indio_dev->info = &pat9125_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
+		pat9125_threaded_trigger_handler, &pat9125_buffer_ops);
+	if (ret) {
+		dev_err(&client->dev, "unable to setup triggered buffer\n");
+		return ret;
+	}
+
+	data->indio_trig = devm_iio_trigger_alloc(&client->dev, "%s-dev%d",
+		indio_dev->name, indio_dev->id);
+	if (!data->indio_trig)
+		return -ENOMEM;
+	data->indio_trig->dev.parent = &client->dev;
+	data->indio_trig->ops = &pat9125_trigger_ops;
+	iio_trigger_set_drvdata(data->indio_trig, data);
+	ret = devm_iio_trigger_register(&client->dev, data->indio_trig);
+	if (ret) {
+		dev_err(&client->dev, "unable to register trigger\n");
+		return ret;
+	}
+
+	data->regmap = devm_regmap_init_i2c(client, &pat9125_regmap_config);
+	if (IS_ERR(data->regmap)) {
+		dev_err(&client->dev, "regmap init failed %ld\n",
+			PTR_ERR(data->regmap));
+		return PTR_ERR(data->regmap);
+	}
+
+	/* Check device ID */
+	ret = regmap_read(data->regmap, PAT9125_PRD_ID1_REG, &sensor_pid);
+	if (ret < 0) {
+		dev_err(&client->dev, "register 0x%x access failed %d\n",
+			PAT9125_PRD_ID1_REG, ret);
+		return ret;
+	}
+	if (sensor_pid != PAT9125_SENSOR_ID_VAL)
+		return -ENODEV;
+
+	/* Switch to bank0 (Magic number)*/
+	ret = regmap_write(data->regmap, 0x7F, 0x00);
+	if (ret < 0) {
+		dev_err(indio_dev->dev.parent, "register 0x%x access failed %d\n",
+			0x7F, ret);
+		return ret;
+	}
+
+	/* Software reset */
+	ret = regmap_write_bits(data->regmap,
+			      PAT9125_CONFIG_REG,
+			      PAT9125_RESET_BIT,
+			      1);
+	if (ret < 0) {
+		dev_err(&client->dev, "register 0x%x access failed %d\n",
+			PAT9125_CONFIG_REG, ret);
+		return ret;
+	}
+
+	msleep(20);
+
+	/* Init GPIO IRQ */
+	if (client->irq) {
+		ret = devm_request_threaded_irq(&client->dev,
+			client->irq,
+			NULL,
+			pat9125_threaded_event_handler,
+			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+			"pat9125",
+			indio_dev);
+		if (ret) {
+			dev_err(&client->dev, "GPIO IRQ init failed\n");
+			return ret;
+		}
+	}
+
+	ret = devm_iio_device_register(&client->dev, indio_dev);
+	if (ret) {
+		dev_err(&client->dev, "IIO device register failed\n");
+		return ret;
+	}
+
+	i2c_set_clientdata(client, indio_dev);
+	return 0;
+}
+
+static const struct i2c_device_id pat9125_id[] = {
+	{ "pat9125", 0 },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, pat9125_id);
+
+static const unsigned short normal_i2c[] = {
+	PAT9125_I2C_ADDR_HI,
+	PAT9125_I2C_ADDR_LO,
+	PAT9125_I2C_ADDR_NC,
+	I2C_CLIENT_END
+};
+
+static struct i2c_driver pat9125_driver = {
+	.driver = {
+		.name = "pat9125",
+	},
+	.probe = pat9125_probe,
+	.address_list = normal_i2c,
+	.id_table = pat9125_id,
+};
+
+module_i2c_driver(pat9125_driver);
+
+MODULE_AUTHOR("Alexandre Mergnat <amergnat@baylibre.com>");
+MODULE_DESCRIPTION("Optical Tracking sensor");
+MODULE_LICENSE("GPL");
-- 
2.17.1

Re: [PATCH v3 3/3] iio: Add PAT9125 optical tracker sensor

[Jonathan Cameron]

On Mon, 10 Jun 2019 11:29:45 +0200
Alexandre Mergnat <amergnat@baylibre.com> wrote:

> This adds support for PixArt Imaging’s miniature low power optical
> navigation chip using LASER light source enabling digital surface tracking.
> 
> Feature and datasheet: [0]
> 
> This IIO driver allows to read delta or relative position on X and Y axis:
>   - The position relative to where the system started can be taken through
>   punctual "read_raw" which will issue a read in the device registers to
>   get the delta between last/current read and return the addition of all
>   the deltas.
>   - The delta can be retrieved using triggered buffer subscription
>   (i.e. iio_readdev). The buffer payload is:
>     |32 bits delta X|32 bits delta Y|timestamp|.
> 
> The possible I2C addresses are 0x73, 0x75 and 0x79.
> 
> X and Y axis CPI resolution can be get/set independently through IIO_SCALE.
> The range value is 0-255 which means:
>   - 0 to ~1,275 Counts Per Inch on flat surface.
>   - 0 to ~630 Counts Per Rev on 1.0mm diameter STS shaft at 1.0mm distance.
> More details on the datasheet.
> 
> The "position" directory is added to contain drivers which can provide
> position data.
> 
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> 
> [0]: http://www.pixart.com/products-detail/72/PAT9125EL-TKIT___TKMT
> 
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>

Hi Alexandre,

Getting close but a few more bits and pieces inline.

I'm a little confused on why the buffered reads are giving the delta
values whilst we have fixed up the sysfs reads to give the more useful
position values (and hence not need to know when last read was).
We should be consistent and give positions from the buffered path as well.

Thanks,

Jonathan

> ---
>  drivers/iio/Kconfig            |   1 +
>  drivers/iio/Makefile           |   1 +
>  drivers/iio/position/Kconfig   |  18 ++
>  drivers/iio/position/Makefile  |   6 +
>  drivers/iio/position/pat9125.c | 499 +++++++++++++++++++++++++++++++++
>  5 files changed, 525 insertions(+)
>  create mode 100644 drivers/iio/position/Kconfig
>  create mode 100644 drivers/iio/position/Makefile
>  create mode 100644 drivers/iio/position/pat9125.c
> 
> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
> index 1d736a4952ab..23d9780640e7 100644
> --- a/drivers/iio/Kconfig
> +++ b/drivers/iio/Kconfig
> @@ -85,6 +85,7 @@ source "drivers/iio/light/Kconfig"
>  source "drivers/iio/magnetometer/Kconfig"
>  source "drivers/iio/multiplexer/Kconfig"
>  source "drivers/iio/orientation/Kconfig"
> +source "drivers/iio/position/Kconfig"
>  if IIO_TRIGGER
>     source "drivers/iio/trigger/Kconfig"
>  endif #IIO_TRIGGER
> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> index bff682ad1cfb..1712011c0f4a 100644
> --- a/drivers/iio/Makefile
> +++ b/drivers/iio/Makefile
> @@ -31,6 +31,7 @@ obj-y += light/
>  obj-y += magnetometer/
>  obj-y += multiplexer/
>  obj-y += orientation/
> +obj-y += position/
>  obj-y += potentiometer/
>  obj-y += potentiostat/
>  obj-y += pressure/
> diff --git a/drivers/iio/position/Kconfig b/drivers/iio/position/Kconfig
> new file mode 100644
> index 000000000000..1cf28896511c
> --- /dev/null
> +++ b/drivers/iio/position/Kconfig
> @@ -0,0 +1,18 @@
> +#
> +# Optical tracker sensors
> +#
> +# When adding new entries keep the list in alphabetical order
> +
> +menu "Optical tracker sensors"
> +
> +config PAT9125
> +	tristate "Optical tracker PAT9125 I2C driver"
> +	depends on I2C
> +	select IIO_BUFFER
> +	help
> +	  Say yes here to build support for PAT9125 optical tracker
> +	  sensors.
> +
> +          To compile this driver as a module, say M here: the module will
> +          be called pat9125.
> +endmenu
> diff --git a/drivers/iio/position/Makefile b/drivers/iio/position/Makefile
> new file mode 100644
> index 000000000000..cf294917ae2c
> --- /dev/null
> +++ b/drivers/iio/position/Makefile
> @@ -0,0 +1,6 @@
> +#
> +# Makefile for industrial I/O Optical tracker sensor drivers
> +#
> +
> +# When adding new entries keep the list in alphabetical order
> +obj-$(CONFIG_PAT9125) += pat9125.o
> diff --git a/drivers/iio/position/pat9125.c b/drivers/iio/position/pat9125.c
> new file mode 100644
> index 000000000000..22bf729bec9b
> --- /dev/null
> +++ b/drivers/iio/position/pat9125.c
> @@ -0,0 +1,499 @@
> +// SPDX-License-Identifier: (GPL-2.0)
> +/*
> + * Copyright (C) 2019 BayLibre, SAS
> + * Author: Alexandre Mergnat <amergnat@baylibre.com>
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/events.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/iio/kfifo_buf.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +/* I2C Address function to ID pin*/
> +#define PAT9125_I2C_ADDR_HI		0x73
> +#define PAT9125_I2C_ADDR_LO		0x75
> +#define PAT9125_I2C_ADDR_NC		0x79
> +
> +/* Registers */
> +#define PAT9125_PRD_ID1_REG		0x00
> +#define PAT9125_PRD_ID2_REG		0x01
> +#define PAT9125_MOTION_STATUS_REG	0x02
> +#define PAT9125_DELTA_X_LO_REG		0x03
> +#define PAT9125_DELTA_Y_LO_REG		0x04
> +#define PAT9125_OP_MODE_REG		0x05
> +#define PAT9125_CONFIG_REG		0x06
> +#define PAT9125_WRITE_PROTEC_REG	0x09
> +#define PAT9125_SLEEP1_REG		0x0A
> +#define PAT9125_SLEEP2_REG		0x0B
> +#define PAT9125_RES_X_REG		0x0D
> +#define PAT9125_RES_Y_REG		0x0E
> +#define PAT9125_DELTA_XY_HI_REG		0x12
> +#define PAT9125_SHUTER_REG		0x14
> +#define PAT9125_FRAME_AVG_REG		0x17
> +#define PAT9125_ORIENTATION_REG		0x19
> +
> +/* Bits */
> +#define PAT9125_VALID_MOTION_DATA_BIT	BIT(7)
> +#define PAT9125_RESET_BIT		BIT(7)
> +
> +/* Registers' values */
> +#define PAT9125_SENSOR_ID_VAL			0x31
> +#define PAT9125_DISABLE_WRITE_PROTECT_VAL	0x5A
> +#define PAT9125_ENABLE_WRITE_PROTECT_VAL	0x00
> +
> +/* Default Value of sampled value size */
> +#define PAT9125_SAMPLED_VAL_BIT_SIZE		12
> +
> +struct pat9125_data {
> +	struct regmap *regmap;
> +	struct iio_trigger *indio_trig;	/* Motion detection */
> +	s32 delta_x;
> +	s32 delta_y;
> +	s32 position_x;
> +	s32 position_y;
> +	bool sampling;
> +};
> +
> +static const struct iio_chan_spec pat9125_channels[] = {
> +	{
> +		.type = IIO_DISTANCE,
> +		.modified = 1,
> +		.channel2 = IIO_MOD_X,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +			BIT(IIO_CHAN_INFO_SCALE),
> +		.scan_index = 0,
> +		.scan_type = {
> +			.sign = 's',
> +			.realbits = 32,
> +			.storagebits = 32,
> +			.endianness = IIO_CPU,
> +		},
> +	},
> +	{
> +		.type = IIO_DISTANCE,
> +		.modified = 1,
> +		.channel2 = IIO_MOD_Y,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +			BIT(IIO_CHAN_INFO_SCALE),
> +		.scan_index = 1,
> +		.scan_type = {
> +			.sign = 's',
> +			.realbits = 32,
> +			.storagebits = 32,
> +			.endianness = IIO_CPU,
> +		},
> +	},
> +	IIO_CHAN_SOFT_TIMESTAMP(2),
> +};
> +
> +/**
> + * pat9125_write_pretected_reg() - Write value in protected register.
> + *
> + * @regmap: Pointer to I2C register map.
> + * @reg_addr: Register address.
> + * @reg_value: Value to be write in register.
> + *
> + * A value of zero will be returned on success, a negative errno will
> + * be returned in error cases.
> + */
> +static int pat9125_write_pretected_reg(struct iio_dev *indio_dev,
> +	u8 reg_addr, u8 reg_value)
> +{
> +	struct pat9125_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = regmap_write(data->regmap,
> +			 PAT9125_WRITE_PROTEC_REG,
> +			 PAT9125_DISABLE_WRITE_PROTECT_VAL);
> +	if (ret < 0) {
> +		dev_err(indio_dev->dev.parent, "register 0x%x access failed %d\n",
> +			PAT9125_WRITE_PROTEC_REG, ret);
> +		return ret;
> +	}
> +
> +	ret = regmap_write(data->regmap, reg_addr, reg_value);
> +	if (ret < 0) {
> +		dev_err(indio_dev->dev.parent, "register 0x%x access failed %d\n",
> +			reg_addr, ret);
> +		return ret;
> +	}
> +
> +	ret = regmap_write(data->regmap,
> +			 PAT9125_WRITE_PROTEC_REG,
> +			 PAT9125_ENABLE_WRITE_PROTECT_VAL);
> +	if (ret < 0) {
> +		dev_err(indio_dev->dev.parent, "register 0x%x access failed %d\n",
> +			PAT9125_WRITE_PROTEC_REG, ret);
> +		return ret;
We'll probably get a patch from some script for this so nice to
cleanup now.

regmap_write always returns 0 for success and negative otherwise.

Hence we can just check if (ret).  Having done that we can also drop
the return ret above out of the brackets and save a few lines of code
by getting rid of the return 0 that follows.

Just checkign if (ret) in general is probably a worthwhile cleanup
for these regmap calls.


> +	}
> +	return 0;
> +}
> +/**
> + * pat9125_read_delta() - Read delta value, update delta & position data.
> + *
> + * @data: Driver's data structure.
> + *
> + * A value of zero will be returned on success, a negative errno will
> + * be returned in error cases.
> + */
> +static int pat9125_read_delta(struct pat9125_data *data)
> +{
> +	struct regmap *regmap = data->regmap;
> +	int status = 0;
> +	int val_x = 0;
> +	int val_y = 0;
> +	int val_high_nibbles = 0;
> +	int ret;
> +
> +	ret = regmap_read(regmap, PAT9125_MOTION_STATUS_REG, &status);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Check if motion is detected */
> +	if (status & PAT9125_VALID_MOTION_DATA_BIT) {
> +		ret = regmap_read(regmap, PAT9125_DELTA_X_LO_REG, &val_x);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = regmap_read(regmap, PAT9125_DELTA_Y_LO_REG, &val_y);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = regmap_read(regmap, PAT9125_DELTA_XY_HI_REG,
> +			&val_high_nibbles);
> +		if (ret < 0)
> +			return ret;
> +
> +		val_x |= (val_high_nibbles << 4) & 0xF00;
> +		val_y |= (val_high_nibbles << 8) & 0xF00;
> +		val_x = sign_extend32(val_x,
> +			PAT9125_SAMPLED_VAL_BIT_SIZE - 1);
> +		val_y = sign_extend32(val_y,
> +			PAT9125_SAMPLED_VAL_BIT_SIZE - 1);
> +		data->position_x += val_x;
> +		data->position_y += val_y;
> +		data->delta_x = val_x;
> +		data->delta_y = val_y;
> +	}
> +	return 0;
> +}
> +
> +/**
> + * pat9125_read_raw() - Sample and return the value(s)
> + * function to the associated channel info enum.
> + **/
> +static int pat9125_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	struct pat9125_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = pat9125_read_delta(data);
> +		if (ret)
> +			return ret;
> +		switch (chan->channel2) {
> +		case IIO_MOD_X:
> +			*val = data->position_x;
> +			return IIO_VAL_INT;
> +		case IIO_MOD_Y:
> +			*val = data->position_y;
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->channel2) {
> +		case IIO_MOD_X:
> +			ret = regmap_read(data->regmap, PAT9125_RES_X_REG, val);
> +			if (ret)
> +				return ret;
> +			else
> +				return IIO_VAL_INT;
> +		case IIO_MOD_Y:
> +			ret = regmap_read(data->regmap, PAT9125_RES_Y_REG, val);
> +			if (ret)
> +				return ret;
> +			else
> +				return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +/**
> + * pat9125_write_raw() - Write the value(s)
> + * function to the associated channel info enum.
> + **/
*/

Tidy these up throughout.  If you are going to do
kernel-doc (which is good) you should also document all of the
parameters.

> +static int pat9125_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan, int val,
> +			     int val2, long mask)
> +{
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->channel2) {
> +		case IIO_MOD_X:
> +			ret = pat9125_write_pretected_reg(indio_dev,
> +				PAT9125_RES_X_REG, val);
> +			return ret;
> +		case IIO_MOD_Y:
> +			ret = pat9125_write_pretected_reg(indio_dev,
> +				PAT9125_RES_Y_REG, val);
> +			return ret;
> +		default:
> +			return -EINVAL;
> +		}
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static irqreturn_t pat9125_threaded_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct pat9125_data *data = iio_priv(indio_dev);
> +	u8 buf[16]; /* Payload: Delta_X (4) | Delta_Y (4) | Timestamp (8) */
> +	int ret;
> +	s64 timestamp;
> +
> +	data->sampling = true;
> +	ret = pat9125_read_delta(data);
> +	if (ret) {
> +		dev_err(indio_dev->dev.parent, "Read delta failed %d\n", ret);
> +		return IRQ_NONE;
> +	}
> +	timestamp = iio_get_time_ns(indio_dev);
> +	*((s32 *)&buf[0]) = data->delta_x;
> +	*((s32 *)&buf[sizeof(s32)]) = data->delta_y;

Why are we putting the delta values in the buffer rather than the positions?

> +	data->delta_x = 0;
> +	data->delta_y = 0;
> +	iio_push_to_buffers_with_timestamp(indio_dev, buf, timestamp);
> +	iio_trigger_notify_done(indio_dev->trig);
> +	return IRQ_HANDLED;
> +}
> +
> +/**
> + * pat9125_threaded_event_handler() - Threaded motion detection event handler
> + * @irq: The irq being handled.
> + * @private: struct iio_device pointer for the device.
> + */
> +static irqreturn_t pat9125_threaded_event_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct pat9125_data *data = iio_priv(indio_dev);
> +
> +	iio_trigger_poll_chained(data->indio_trig);
> +	return IRQ_HANDLED;
> +}
> +
> +static int pat9125_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct pat9125_data *data = iio_priv(indio_dev);
> +	int ret = 0;
> +
> +	ret = iio_triggered_buffer_postenable(indio_dev);
> +	if (ret)
> +		return ret;
> +	/* Release interrupt pin on the device */
> +	return pat9125_read_delta(data);

If this has an error, we should be handling the unwind of
iio_triggered_buffer_postenable.  That is
iio_triggered_buffer_predisable.

(Alexandru has been cleaning these up recently so I'm more
aware of this than normal!)

> +}
> +
> +static const struct iio_buffer_setup_ops pat9125_buffer_ops = {
> +	.postenable = pat9125_buffer_postenable,
> +	.predisable = iio_triggered_buffer_predisable,
> +};
> +
One blank line is almost always enough (nitpick of the day ;)
> +
> +static const struct regmap_config pat9125_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +};
> +
> +static const struct iio_info pat9125_info = {
> +	.read_raw = pat9125_read_raw,
> +	.write_raw = pat9125_write_raw,
> +};
> +
> +/*
> + * To detect if a new value is available, register status is checked. This
> + * method is safer than using a flag on GPIO IRQ to track event while sampling
> + * because falling edge is missed when device trig just after a read reg value
> + * (that happen for fast motions or high CPI setting).

So we have an edge triggered interrupt that doesn't have a 'minimum low'
period? If so then the only safe way to handle it would be as a level
interrupt. Can you do that here?
(I once had the delights of a sensor like this tied to a edge sensitive only
interrupt, but thankfully those are a rare thing these days).

> + *
> + * Note: To avoid infinite loop in "iio_trigger_notify_done" when it si not in

is

> + * buffer mode and kernel warning due to nested IRQ thread,
> + * this function must return 0.
> + */
> +static int pat9125_trig_try_reenable(struct iio_trigger *trig)
> +{
> +	struct pat9125_data *data = iio_trigger_get_drvdata(trig);
> +	struct regmap *regmap = data->regmap;
> +	int status = 0;
> +
> +	if (data->sampling) {
> +		regmap_read(regmap, PAT9125_MOTION_STATUS_REG, &status);
> +		if (status & PAT9125_VALID_MOTION_DATA_BIT) {
> +			data->sampling = false;
So we only ever do 2 reads?  Why can't we be unlucky on timing
twice in a row?

> +			iio_trigger_poll_chained(data->indio_trig);
> +			return 0;
> +		}
> +	}
> +	data->sampling = false;
> +	return 0;
> +}
> +
> +static const struct iio_trigger_ops pat9125_trigger_ops = {
> +	.try_reenable = pat9125_trig_try_reenable,
> +};
> +
> +static int pat9125_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	struct pat9125_data *data;
> +	struct iio_dev *indio_dev;
> +	int ret, sensor_pid;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +	if (!indio_dev) {
> +		dev_err(&client->dev, "IIO device allocation failed\n");
> +		return -ENOMEM;
> +	}
> +
> +	data = iio_priv(indio_dev);
> +	indio_dev->dev.parent = &client->dev;
> +	indio_dev->name = id->name;
> +	indio_dev->channels = pat9125_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(pat9125_channels);
> +	indio_dev->info = &pat9125_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
> +		pat9125_threaded_trigger_handler, &pat9125_buffer_ops);
> +	if (ret) {
> +		dev_err(&client->dev, "unable to setup triggered buffer\n");
> +		return ret;
> +	}
> +
> +	data->indio_trig = devm_iio_trigger_alloc(&client->dev, "%s-dev%d",
> +		indio_dev->name, indio_dev->id);
> +	if (!data->indio_trig)
> +		return -ENOMEM;
> +	data->indio_trig->dev.parent = &client->dev;
> +	data->indio_trig->ops = &pat9125_trigger_ops;
> +	iio_trigger_set_drvdata(data->indio_trig, data);
> +	ret = devm_iio_trigger_register(&client->dev, data->indio_trig);
> +	if (ret) {
> +		dev_err(&client->dev, "unable to register trigger\n");
> +		return ret;
> +	}
> +
> +	data->regmap = devm_regmap_init_i2c(client, &pat9125_regmap_config);
> +	if (IS_ERR(data->regmap)) {
> +		dev_err(&client->dev, "regmap init failed %ld\n",
> +			PTR_ERR(data->regmap));
> +		return PTR_ERR(data->regmap);
> +	}
> +
> +	/* Check device ID */
> +	ret = regmap_read(data->regmap, PAT9125_PRD_ID1_REG, &sensor_pid);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "register 0x%x access failed %d\n",
> +			PAT9125_PRD_ID1_REG, ret);
> +		return ret;
> +	}
> +	if (sensor_pid != PAT9125_SENSOR_ID_VAL)
> +		return -ENODEV;
> +
> +	/* Switch to bank0 (Magic number)*/
> +	ret = regmap_write(data->regmap, 0x7F, 0x00);
> +	if (ret < 0) {
> +		dev_err(indio_dev->dev.parent, "register 0x%x access failed %d\n",
> +			0x7F, ret);
> +		return ret;
> +	}
> +
> +	/* Software reset */
> +	ret = regmap_write_bits(data->regmap,
> +			      PAT9125_CONFIG_REG,
> +			      PAT9125_RESET_BIT,
> +			      1);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "register 0x%x access failed %d\n",
> +			PAT9125_CONFIG_REG, ret);
> +		return ret;
> +	}
> +
> +	msleep(20);
> +
> +	/* Init GPIO IRQ */
> +	if (client->irq) {
> +		ret = devm_request_threaded_irq(&client->dev,
> +			client->irq,
> +			NULL,
> +			pat9125_threaded_event_handler,
> +			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +			"pat9125",
> +			indio_dev);
> +		if (ret) {
> +			dev_err(&client->dev, "GPIO IRQ init failed\n");
> +			return ret;
> +		}
> +	}
> +
> +	ret = devm_iio_device_register(&client->dev, indio_dev);
> +	if (ret) {
> +		dev_err(&client->dev, "IIO device register failed\n");
> +		return ret;
> +	}
> +
> +	i2c_set_clientdata(client, indio_dev);

Why?  You have now gotten rid of remove (which is great).
I'm guessing that was using i2c_get_clientdata?

> +	return 0;
> +}
> +
> +static const struct i2c_device_id pat9125_id[] = {
> +	{ "pat9125", 0 },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, pat9125_id);
> +
> +static const unsigned short normal_i2c[] = {
> +	PAT9125_I2C_ADDR_HI,
> +	PAT9125_I2C_ADDR_LO,
> +	PAT9125_I2C_ADDR_NC,
> +	I2C_CLIENT_END
> +};
> +
> +static struct i2c_driver pat9125_driver = {
> +	.driver = {
> +		.name = "pat9125",
> +	},
> +	.probe = pat9125_probe,
> +	.address_list = normal_i2c,
> +	.id_table = pat9125_id,
> +};
> +
> +module_i2c_driver(pat9125_driver);
> +
> +MODULE_AUTHOR("Alexandre Mergnat <amergnat@baylibre.com>");
> +MODULE_DESCRIPTION("Optical Tracking sensor");
> +MODULE_LICENSE("GPL");

Re: [PATCH v3 3/3] iio: Add PAT9125 optical tracker sensor

[Alexandre Mergnat]

Le dim. 16 juin 2019 à 17:39, Jonathan Cameron <jic23@kernel.org> a écrit :
>
> On Mon, 10 Jun 2019 11:29:45 +0200
> Alexandre Mergnat <amergnat@baylibre.com> wrote:
 ...
>
> > +/*
> > + * To detect if a new value is available, register status is checked. This
> > + * method is safer than using a flag on GPIO IRQ to track event while sampling
> > + * because falling edge is missed when device trig just after a read reg value
> > + * (that happen for fast motions or high CPI setting).
>
> So we have an edge triggered interrupt that doesn't have a 'minimum low'
> period? If so then the only safe way to handle it would be as a level
> interrupt. Can you do that here?
> (I once had the delights of a sensor like this tied to a edge sensitive only
> interrupt, but thankfully those are a rare thing these days).
>

Trigger level is the first setup I tried (and retried during
modifications) but it cannot
works despite of ONESHOT flag. I'm wrong or it's probably due to
nested_irq because
it works when I reset interrupt (by reading data) during one of the
IRQ thread, that what
I did in my V1 patch. I spent a lot of time to try to use level
trigger but this is the
best way I found to do it properly without corner cases.
The result with nested IRQ and low level trigger is a spamming IRQ
(probably due to IRQ no more masked during nested IRQ thread) who that stuck the
board because it hasn't time to make an I2C read to reset interrupt pin.


> > + * buffer mode and kernel warning due to nested IRQ thread,
> > + * this function must return 0.
> > + */
> > +static int pat9125_trig_try_reenable(struct iio_trigger *trig)
> > +{
> > +     struct pat9125_data *data = iio_trigger_get_drvdata(trig);
> > +     struct regmap *regmap = data->regmap;
> > +     int status = 0;
> > +
> > +     if (data->sampling) {
> > +             regmap_read(regmap, PAT9125_MOTION_STATUS_REG, &status);
> > +             if (status & PAT9125_VALID_MOTION_DATA_BIT) {
> > +                     data->sampling = false;
> So we only ever do 2 reads?  Why can't we be unlucky on timing
> twice in a row?

That can works indefinitely, I tested for some retry in a row by
moving the chip fastly.
If the method blocked at 2 readings, I should have been stuck during this test.

If read status return "New data available", a new read value is done
through the same
process (that mean data->sampling put to true) thanks to nested IRQ
thread which will
call try_reenable again and then re-check pat9125 status.

Re: [PATCH v3 3/3] iio: Add PAT9125 optical tracker sensor

[Jonathan Cameron]

On Thu, 11 Jul 2019 21:39:10 +0200
Alexandre Mergnat <amergnat@baylibre.com> wrote:

> Le dim. 16 juin 2019 à 17:39, Jonathan Cameron <jic23@kernel.org> a écrit :
> >
> > On Mon, 10 Jun 2019 11:29:45 +0200
> > Alexandre Mergnat <amergnat@baylibre.com> wrote:  
>  ...
> >  
> > > +/*
> > > + * To detect if a new value is available, register status is checked. This
> > > + * method is safer than using a flag on GPIO IRQ to track event while sampling
> > > + * because falling edge is missed when device trig just after a read reg value
> > > + * (that happen for fast motions or high CPI setting).  
> >
> > So we have an edge triggered interrupt that doesn't have a 'minimum low'
> > period? If so then the only safe way to handle it would be as a level
> > interrupt. Can you do that here?
> > (I once had the delights of a sensor like this tied to a edge sensitive only
> > interrupt, but thankfully those are a rare thing these days).
> >  
> 
> Trigger level is the first setup I tried (and retried during
> modifications) but it cannot
> works despite of ONESHOT flag. I'm wrong or it's probably due to
> nested_irq because
> it works when I reset interrupt (by reading data) during one of the
> IRQ thread, that what
> I did in my V1 patch. I spent a lot of time to try to use level
> trigger but this is the
> best way I found to do it properly without corner cases.
> The result with nested IRQ and low level trigger is a spamming IRQ
> (probably due to IRQ no more masked during nested IRQ thread) who that stuck the
> board because it hasn't time to make an I2C read to reset interrupt pin.

I'm not totally following the explanation.  The level interrupt
should remain masked until the irq_thread completes, as long
as you have the irq as IRQF_ONESHOT. The nesting shouldn't
matter for this as the interrupt should only be
unmasked once we the poll_func handler has completed.

If the device is sampling fast enough, you might find that it's
taken another sample so you get an immediate new interrupt.
If that's the case, then only option is to reducing the sampling
rate.

Of course, it's always possible we have a bug, but there
are quite a few drivers doing things with level interrupts
with no problem, so I'm a little doubtful.

> 
> 
> > > + * buffer mode and kernel warning due to nested IRQ thread,
> > > + * this function must return 0.
> > > + */
> > > +static int pat9125_trig_try_reenable(struct iio_trigger *trig)
> > > +{
> > > +     struct pat9125_data *data = iio_trigger_get_drvdata(trig);
> > > +     struct regmap *regmap = data->regmap;
> > > +     int status = 0;
> > > +
> > > +     if (data->sampling) {
> > > +             regmap_read(regmap, PAT9125_MOTION_STATUS_REG, &status);
> > > +             if (status & PAT9125_VALID_MOTION_DATA_BIT) {
> > > +                     data->sampling = false;  
> > So we only ever do 2 reads?  Why can't we be unlucky on timing
> > twice in a row?  
> 
> That can works indefinitely, I tested for some retry in a row by
> moving the chip fastly.
> If the method blocked at 2 readings, I should have been stuck during this test.
> 
> If read status return "New data available", a new read value is done
> through the same
> process (that mean data->sampling put to true) thanks to nested IRQ
> thread which will
> call try_reenable again and then re-check pat9125 status.
Hmm. I'm not totally happy with a test that is based on failing to
trigger it.   We need a theoretical argument for why it can't happen
to back up this code.

Jonathan