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

[PATCH v4 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 is a relative position from where start the device on X and Y
axis, depend on CPI (Counts Per Inch) resolution setting chosen.

The device support CPI configuration through IIO interface.

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

Change since v3:
- Replace delta value by relative position
- Improve write protected reg function by removing print log and obvious
  returns
- Handle error in postenable buffer 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)

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

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                | 506 ++++++++++++++++++
 7 files changed, 552 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 v4 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 18b79c4cf7d5..120529f40c7c 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -705,6 +705,8 @@ patternProperties:
     description: Pine64
   "^pineriver,.*":
     description: Shenzhen PineRiver Designs Co., Ltd.
+  "^pixart,.*":
+    description: PixArt Imaging Inc.
   "^pixcir,.*":
     description: PIXCIR MICROELECTRONICS Co., Ltd
   "^plantower,.*":
-- 
2.17.1

[PATCH v4 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 v4 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.

Features and datasheet: [0]

This IIO driver allows to read relative position from where the system
started on X and Y axis through two way:
  - 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.
  - Buffer read. Data 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.

[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 | 506 +++++++++++++++++++++++++++++++++
 5 files changed, 532 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 5bd51853b15e..aca6fcbceeab 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..2f04777e0790
--- /dev/null
+++ b/drivers/iio/position/pat9125.c
@@ -0,0 +1,506 @@
+// 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 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)
+		ret = regmap_write(data->regmap, reg_addr, reg_value);
+
+	/* Try to put back write protection everytime */
+	ret |= regmap_write(data->regmap,
+		PAT9125_WRITE_PROTEC_REG,
+		PAT9125_ENABLE_WRITE_PROTECT_VAL);
+
+	return ret;
+}
+/**
+ * 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;
+	}
+	return 0;
+}
+
+/**
+ * pat9125_read_raw() - Sample and return the value(s)
+ * function to the associated channel info enum.
+ *
+ * @indio_dev:	Industrial I/O device.
+ * @chan:	Specification of a single channel.
+ * @val:	Contain the elements making up the returned value.
+ * @val2:	Not used.
+ * @mask:	(enum iio_chan_info_enum) Type of the info attribute.
+ *
+ * Zero will be returned on success, negative value otherwise.
+ **/
+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.
+ *
+ * @indio_dev:	Industrial I/O device.
+ * @chan:	Specification of a single channel.
+ * @val:	Value write in the channel.
+ * @val2:	Not used.
+ * @mask:	(enum iio_chan_info_enum) Type of the info attribute.
+ *
+ * Zero will be returned on success, negative value otherwise.
+ **/
+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: Pos_X (4) | Pos_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->position_x;
+	*((s32 *)&buf[sizeof(s32)]) = data->position_y;
+	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;
+}
+
+/**
+ * pat9125_buffer_postenable() - Buffer post enable actions
+ *
+ * @indio_dev:	Industrial I/O device.
+ */
+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 */
+	ret = pat9125_read_delta(data);
+
+	/* iio_trigger_detach_poll_func isn't reachable, so use this function */
+	if (ret)
+		ret = iio_triggered_buffer_predisable(indio_dev);
+
+	return ret;
+}
+
+static const struct iio_buffer_setup_ops pat9125_buffer_ops = {
+	.postenable = pat9125_buffer_postenable,
+};
+
+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 is 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;
+	}
+
+	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 v4 3/3] iio: Add PAT9125 optical tracker sensor

[Jonathan Cameron]

On Sat, 13 Jul 2019 10:04:55 +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.
> 
> Features and datasheet: [0]
> 
> This IIO driver allows to read relative position from where the system
> started on X and Y axis through two way:
>   - 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.
>   - Buffer read. Data 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.
> 
> [0]: http://www.pixart.com/products-detail/72/PAT9125EL-TKIT___TKMT
> 
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
Hi Alexandre,

Sorry for the lack of reply to the previous version. To much jet
lag and travelling recently so I'm just catching up today.

I still want to get the bottom of why the level interrupt
approach isn't working.

I've played these games with using edge interrupts and
try_reenable tricks in the past and they very rarely work out
in the long term.

The issue with try_reenable calling iio_poll_trigger
looks to be a bug in the IIO core, so a patch to fix
that would be welcome!

A few minor other bits inline.

Jonathan

> ---
>  drivers/iio/Kconfig            |   1 +
>  drivers/iio/Makefile           |   1 +
>  drivers/iio/position/Kconfig   |  18 ++
>  drivers/iio/position/Makefile  |   6 +
>  drivers/iio/position/pat9125.c | 506 +++++++++++++++++++++++++++++++++
>  5 files changed, 532 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 5bd51853b15e..aca6fcbceeab 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..2f04777e0790
> --- /dev/null
> +++ b/drivers/iio/position/pat9125.c
> @@ -0,0 +1,506 @@
> +// 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)

Please use naming that associates these with which register
they are in.

> +
> +/* 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 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)
> +		ret = regmap_write(data->regmap, reg_addr, reg_value);
> +
> +	/* Try to put back write protection everytime */
> +	ret |= regmap_write(data->regmap,
> +		PAT9125_WRITE_PROTEC_REG,
> +		PAT9125_ENABLE_WRITE_PROTECT_VAL);

This ret |= trick leads to scrambled error codes.  So don't do it,
use two return variables and return the first one to give an
error if one occurs.

> +
> +	return ret;
> +}
> +/**
> + * 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;

Some of these are assigned anyway in all paths that use them.

> +	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;
> +	}
> +	return 0;
> +}
> +
> +/**
> + * pat9125_read_raw() - Sample and return the value(s)
> + * function to the associated channel info enum.
> + *
> + * @indio_dev:	Industrial I/O device.
> + * @chan:	Specification of a single channel.
> + * @val:	Contain the elements making up the returned value.
> + * @val2:	Not used.
> + * @mask:	(enum iio_chan_info_enum) Type of the info attribute.
> + *
> + * Zero will be returned on success, negative value otherwise.
> + **/
> +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
These else against an error case are not common kernel idiom for error handling..

			if (ret)
				return ret;
			return IIO_VAL_INT;

> +				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.
> + *
> + * @indio_dev:	Industrial I/O device.
> + * @chan:	Specification of a single channel.
> + * @val:	Value write in the channel.
> + * @val2:	Not used.
> + * @mask:	(enum iio_chan_info_enum) Type of the info attribute.
> + *
> + * Zero will be returned on success, negative value otherwise.
> + **/
Kernel doc style is normally
*/ at end.

> +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: Pos_X (4) | Pos_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->position_x;
> +	*((s32 *)&buf[sizeof(s32)]) = data->position_y;
> +	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;
> +}
> +
> +/**
> + * pat9125_buffer_postenable() - Buffer post enable actions
> + *
> + * @indio_dev:	Industrial I/O device.
> + */
> +static int pat9125_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct pat9125_data *data = iio_priv(indio_dev);
> +	int ret = 0;

Check for any other cases of this.
Doesn't need to be assigned as is assigned in all paths.

> +
> +	ret = iio_triggered_buffer_postenable(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	/* Release interrupt pin on the device */
> +	ret = pat9125_read_delta(data);
> +
> +	/* iio_trigger_detach_poll_func isn't reachable, so use this function */

Slightly odd comment.  We need to unwind iio_triggered_buffer_postenable, so
iio_triggered_buffer_predisable is the right function anyway..

> +	if (ret)
> +		ret = iio_triggered_buffer_predisable(indio_dev);
> +
> +	return ret;
> +}
> +
> +static const struct iio_buffer_setup_ops pat9125_buffer_ops = {
> +	.postenable = pat9125_buffer_postenable,
> +};
> +
> +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 is not in
> + * buffer mode and kernel warning due to nested IRQ thread,
> + * this function must return 0.

Two things here.
For infinite loop prevention it would be cleaner to have an explicit
countdown in here (so let it loop N times).
I would also like to see a warning if it times out.

The point about not calling the iio_trigger_poll that would result from
a failed try reenable looks like a core bug to me.

I don't think try_reenable is ever called from interrupt context.
IIRC The interrupt code used to be a lot laxer on that so it probably
wouldn't have caused a problem originally but does now.
Hence please send a fix patch for the core code to switch to
the chained version.


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

Drop the return ret out of these brackets.  One of the static checkers
picks up on this sequence and will moan otherwise (can't recall which).

> +	}
> +
> +	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 v4 2/3] dt-bindings: iio: position: Add docs pat9125

[Jonathan Cameron]

On Sat, 13 Jul 2019 10:04:54 +0200
Alexandre Mergnat <amergnat@baylibre.com> wrote:

> 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>
Whilst this one predates my statement that I wanted all bindings
in YAML going forwards, it will want converting at some stage
and if you have time now it would be great to do so!

Thanks,

Jonathan

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

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

[Peter Meerwald-Stadler]

[-- Attachment #1: Type: text/plain, Size: 19433 bytes --]

On Sat, 13 Jul 2019, Alexandre Mergnat wrote:

some comments below inline

> This adds support for PixArt Imaging’s miniature low power optical
> navigation chip using LASER light source enabling digital surface tracking.
> 
> Features and datasheet: [0]
> 
> This IIO driver allows to read relative position from where the system
> started on X and Y axis through two way:
>   - 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.
>   - Buffer read. Data can be retrieved using triggered buffer subscription
>   (i.e. iio_readdev). The buffer payload is:
>     |32 bits delta X|32 bits delta Y|timestamp|.

32 bits are not actually needed, 16 bits would be enough
 
> 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.

is there IIO documentation that distance_scale is in CPI?
_scale should not be just a byte, but have some unit if possible

> More details on the datasheet.
> 
> The "position" directory is added to contain drivers which can provide
> position data.
> 
> [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 | 506 +++++++++++++++++++++++++++++++++
>  5 files changed, 532 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 5bd51853b15e..aca6fcbceeab 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..2f04777e0790
> --- /dev/null
> +++ b/drivers/iio/position/pat9125.c
> @@ -0,0 +1,506 @@
> +// 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*/

add space before */

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

PROTEC_T_ maybe

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

SHUT_T_ER  maybe

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

value lowercase

> +#define PAT9125_SAMPLED_VAL_BIT_SIZE		12
> +
> +struct pat9125_data {
> +	struct regmap *regmap;
> +	struct iio_trigger *indio_trig;	/* Motion detection */
> +	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.

protected

> + *
> + * @regmap: Pointer to I2C register map.

the first argument is indio_dev, not regmap

> + * @reg_addr: Register address.
> + * @reg_value: Value to be write in register.

written

> + *
> + * A value of zero will be returned on success, a negative errno will

errno has a particular meaning in C runtime, maybe 'negative value' (here 
and elsewhere)

> + * be returned in error cases.
> + */
> +static int pat9125_write_pretected_reg(struct iio_dev *indio_dev,

protected

> +	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)
> +		ret = regmap_write(data->regmap, reg_addr, reg_value);
> +
> +	/* Try to put back write protection everytime */
> +	ret |= regmap_write(data->regmap,
> +		PAT9125_WRITE_PROTEC_REG,
> +		PAT9125_ENABLE_WRITE_PROTECT_VAL);
> +
> +	return ret;
> +}
> +/**
> + * 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;

I'd rather use s32 type for val_x and val_y due to sign_extend32()

> +	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;
> +	}
> +	return 0;
> +}
> +
> +/**
> + * pat9125_read_raw() - Sample and return the value(s)
> + * function to the associated channel info enum.
> + *
> + * @indio_dev:	Industrial I/O device.
> + * @chan:	Specification of a single channel.
> + * @val:	Contain the elements making up the returned value.

Contains

> + * @val2:	Not used.
> + * @mask:	(enum iio_chan_info_enum) Type of the info attribute.
> + *
> + * Zero will be returned on success, negative value otherwise.
> + **/
> +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

maybe no 'else', just return (feel free to ignore)

> +				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.
> + *
> + * @indio_dev:	Industrial I/O device.
> + * @chan:	Specification of a single channel.
> + * @val:	Value write in the channel.
> + * @val2:	Not used.
> + * @mask:	(enum iio_chan_info_enum) Type of the info attribute.
> + *
> + * Zero will be returned on success, negative value otherwise.
> + **/
> +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);

just return, no need for ret temporary variable

> +			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: Pos_X (4) | Pos_Y (4) | Timestamp (8) */

maybe define buf as 
s32 buf[4];
then ...

> +	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->position_x;
> +	*((s32 *)&buf[sizeof(s32)]) = data->position_y;

... can simple say
buf[0] = data->position_x;
buf[1] = ...

> +	iio_push_to_buffers_with_timestamp(indio_dev, buf, timestamp);
> +	iio_trigger_notify_done(indio_dev->trig);

newline missing for readability

> +	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;
> +}
> +
> +/**
> + * pat9125_buffer_postenable() - Buffer post enable actions
> + *
> + * @indio_dev:	Industrial I/O device.
> + */
> +static int pat9125_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct pat9125_data *data = iio_priv(indio_dev);
> +	int ret = 0;

no need for initialization

> +
> +	ret = iio_triggered_buffer_postenable(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	/* Release interrupt pin on the device */
> +	ret = pat9125_read_delta(data);
> +
> +	/* iio_trigger_detach_poll_func isn't reachable, so use this function */
> +	if (ret)
> +		ret = iio_triggered_buffer_predisable(indio_dev);
> +
> +	return ret;
> +}
> +
> +static const struct iio_buffer_setup_ops pat9125_buffer_ops = {
> +	.postenable = pat9125_buffer_postenable,
> +};
> +
> +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 is 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);

what if regmap_read() fails?
error handling missing

> +		if (status & PAT9125_VALID_MOTION_DATA_BIT) {
> +			data->sampling = false;

data->sampling = false;
could be set before the status check, it is set in all cases

> +			iio_trigger_poll_chained(data->indio_trig);
> +			return 0;
> +		}
> +	}
> +	data->sampling = false;

newline missing

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

probably not worth having that the error message

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

I'd rather put some logging here; what is the actual value obtained?

> +		return -ENODEV;
> +
> +	/* Switch to bank0 (Magic number)*/
> +	ret = regmap_write(data->regmap, 0x7F, 0x00);

maybe a #define for 0x7f, PAT9125_BANK_SWITCH_REG

> +	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;
> +	}
> +
> +	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_ prefix please

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

-- 

Peter Meerwald-Stadler
Mobile: +43 664 24 44 418

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

[Rob Herring]

On Sat, 13 Jul 2019 10:04:53 +0200, Alexandre Mergnat wrote:
> 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(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

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

[Rob Herring]

On Sat, 13 Jul 2019 10:04:53 +0200, Alexandre Mergnat wrote:
> 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(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH v4 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.

Features and datasheet: [0]

This IIO driver allows to read relative position from where the system
started on X and Y axis through two way:
  - 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.
  - Buffer read. Data 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.

[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 | 506 +++++++++++++++++++++++++++++++++
 5 files changed, 532 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 5bd51853b15e..aca6fcbceeab 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..2f04777e0790
--- /dev/null
+++ b/drivers/iio/position/pat9125.c
@@ -0,0 +1,506 @@
+// 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 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)
+		ret = regmap_write(data->regmap, reg_addr, reg_value);
+
+	/* Try to put back write protection everytime */
+	ret |= regmap_write(data->regmap,
+		PAT9125_WRITE_PROTEC_REG,
+		PAT9125_ENABLE_WRITE_PROTECT_VAL);
+
+	return ret;
+}
+/**
+ * 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;
+	}
+	return 0;
+}
+
+/**
+ * pat9125_read_raw() - Sample and return the value(s)
+ * function to the associated channel info enum.
+ *
+ * @indio_dev:	Industrial I/O device.
+ * @chan:	Specification of a single channel.
+ * @val:	Contain the elements making up the returned value.
+ * @val2:	Not used.
+ * @mask:	(enum iio_chan_info_enum) Type of the info attribute.
+ *
+ * Zero will be returned on success, negative value otherwise.
+ **/
+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.
+ *
+ * @indio_dev:	Industrial I/O device.
+ * @chan:	Specification of a single channel.
+ * @val:	Value write in the channel.
+ * @val2:	Not used.
+ * @mask:	(enum iio_chan_info_enum) Type of the info attribute.
+ *
+ * Zero will be returned on success, negative value otherwise.
+ **/
+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: Pos_X (4) | Pos_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->position_x;
+	*((s32 *)&buf[sizeof(s32)]) = data->position_y;
+	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;
+}
+
+/**
+ * pat9125_buffer_postenable() - Buffer post enable actions
+ *
+ * @indio_dev:	Industrial I/O device.
+ */
+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 */
+	ret = pat9125_read_delta(data);
+
+	/* iio_trigger_detach_poll_func isn't reachable, so use this function */
+	if (ret)
+		ret = iio_triggered_buffer_predisable(indio_dev);
+
+	return ret;
+}
+
+static const struct iio_buffer_setup_ops pat9125_buffer_ops = {
+	.postenable = pat9125_buffer_postenable,
+};
+
+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 is 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;
+	}
+
+	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