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

[PATCH v2 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 ponctual and/or buffered data.
The data is the delta value between two position traveled, depend on
CPI (Counts Per Inch) resolution setting chosen. The retrived data is
composed by two values, delta on the X and Y axis.

The devece 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 extention 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 / behavior
- 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 reenable 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: ot: Add docs pat9125
  iio: Add PAT9125 optical tracker sensor

 .../devicetree/bindings/iio/ot/pat9125.txt    |  18 +
 .../devicetree/bindings/vendor-prefixes.txt   |   1 +
 drivers/iio/Kconfig                           |   1 +
 drivers/iio/Makefile                          |   1 +
 drivers/iio/ot/Kconfig                        |  16 +
 drivers/iio/ot/Makefile                       |   6 +
 drivers/iio/ot/pat9125.c                      | 487 ++++++++++++++++++
 7 files changed, 530 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/ot/pat9125.txt
 create mode 100644 drivers/iio/ot/Kconfig
 create mode 100644 drivers/iio/ot/Makefile
 create mode 100644 drivers/iio/ot/pat9125.c

-- 
2.17.1

[PATCH v2 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.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 8162b0eb4b50..dbd889395b74 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -313,6 +313,7 @@ phicomm PHICOMM Co., Ltd.
 phytec	PHYTEC Messtechnik GmbH
 picochip	Picochip Ltd
 pine64	Pine64
+pixart	PixArt Imaging Inc.
 pixcir  PIXCIR MICROELECTRONICS Co., Ltd
 plantower Plantower Co., Ltd
 plathome	Plat'Home Co., Ltd.
-- 
2.17.1

[PATCH v2 2/3] dt-bindings: iio: ot: Add docs pat9125

[Alexandre Mergnat]

Add documentation for the optical tracker PAT9125 and
"ot" directory for Optical Tracker chip.

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

diff --git a/Documentation/devicetree/bindings/iio/ot/pat9125.txt b/Documentation/devicetree/bindings/iio/ot/pat9125.txt
new file mode 100644
index 000000000000..2ffacaafba8e
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/ot/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 v2 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.

This IIO driver allow to read delta position on 2 axis (X and Y).
The values can be taken through ponctual "read_raw" which will issue
a read in the device registers to return the delta between last read/buffering
or subscribe to the data buffer feed automaticaly by a new value using an
IIO trigger. The buffer payload is: |32 bits delta X|32 bits delta Y|timestamp|.
It also provide an IIO trigger which fire when a motion is detected.
This trigger should be reset after each fire by a simple "read_raw" if you aren't
in buffer mode.

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

X and Y axis resolution can be setup independently through module parameter.

The "ot" directory is added to coutain Optical Tracker drivers.

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

diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
index d08aeb41cd07..bdf1bd061168 100644
--- a/drivers/iio/Kconfig
+++ b/drivers/iio/Kconfig
@@ -86,6 +86,7 @@ source "drivers/iio/light/Kconfig"
 source "drivers/iio/magnetometer/Kconfig"
 source "drivers/iio/multiplexer/Kconfig"
 source "drivers/iio/orientation/Kconfig"
+source "drivers/iio/ot/Kconfig"
 if IIO_TRIGGER
    source "drivers/iio/trigger/Kconfig"
 endif #IIO_TRIGGER
diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
index cb5993251381..fdda2e185005 100644
--- a/drivers/iio/Makefile
+++ b/drivers/iio/Makefile
@@ -32,6 +32,7 @@ obj-y += light/
 obj-y += magnetometer/
 obj-y += multiplexer/
 obj-y += orientation/
+obj-y += ot/
 obj-y += potentiometer/
 obj-y += potentiostat/
 obj-y += pressure/
diff --git a/drivers/iio/ot/Kconfig b/drivers/iio/ot/Kconfig
new file mode 100644
index 000000000000..3d17fdaa06ed
--- /dev/null
+++ b/drivers/iio/ot/Kconfig
@@ -0,0 +1,16 @@
+#
+# 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.
+
+endmenu
diff --git a/drivers/iio/ot/Makefile b/drivers/iio/ot/Makefile
new file mode 100644
index 000000000000..cf294917ae2c
--- /dev/null
+++ b/drivers/iio/ot/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/ot/pat9125.c b/drivers/iio/ot/pat9125.c
new file mode 100644
index 000000000000..48c1651b4e6a
--- /dev/null
+++ b/drivers/iio/ot/pat9125.c
@@ -0,0 +1,487 @@
+// 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	0x80
+#define PAT9125_RESET_BIT		0x80
+
+/* Registers' values */
+#define PAT9125_SENSOR_ID_VAL			0x31
+#define PAT9125_DISABLE_WRITE_PROTECT_VAL	0x5A
+#define PAT9125_ENABLE_WRITE_PROTECT_VAL	0x00
+#define PAT9125_CPI_RESOLUTION_X_VAL		0x65
+#define PAT9125_CPI_RESOLUTION_Y_VAL		0xFF
+
+/* 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 */
+	s64 ts;				/* Timestamp */
+	s32 delta_x;
+	s32 delta_y;
+	s32 motion_detected;
+	bool buffer_mode;
+};
+
+static u8 x_resolution = 0x14;
+module_param(x_resolution, byte, 0644);
+MODULE_PARM_DESC(x_resolution, "CPI resolution setting for X axis.");
+
+static u8 y_resolution = 0x14;
+module_param(y_resolution, byte, 0644);
+MODULE_PARM_DESC(y_resolution, "CPI resolution setting for Y axis.");
+
+static const struct iio_event_spec pat9125_event = {
+	.type = IIO_EV_TYPE_THRESH,
+	.dir = IIO_EV_DIR_RISING,
+	.mask_separate = BIT(IIO_EV_INFO_VALUE),
+};
+
+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),
+		.scan_index = 0,
+		.scan_type = {
+			.sign = 's',			\
+			.realbits = 32,			\
+			.storagebits = 32,		\
+			.shift = 0,			\
+			.endianness = IIO_LE,		\
+		},
+		.event_spec = &pat9125_event,
+		.num_event_specs = 1,
+	},
+	{
+		.type = IIO_DISTANCE,
+		.modified = 1,
+		.channel2 = IIO_MOD_Y,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.scan_index = 1,
+		.scan_type = {
+			.sign = 's',			\
+			.realbits = 32,			\
+			.storagebits = 32,		\
+			.shift = 0,			\
+			.endianness = IIO_LE,		\
+		},
+		.event_spec = &pat9125_event,
+		.num_event_specs = 1,
+	},
+	IIO_CHAN_SOFT_TIMESTAMP(2),
+};
+
+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 old_motion_status;
+	int ret;
+
+	old_motion_status = data->motion_detected;
+	data->motion_detected = 0;
+	ret = regmap_read(regmap, PAT9125_MOTION_STATUS_REG, &status);
+	if (ret < 0) {
+		data->motion_detected = old_motion_status;
+		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->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->delta_x;
+			data->delta_x = 0;
+			break;
+		case IIO_MOD_Y:
+			*val = data->delta_y;
+			data->delta_y = 0;
+			break;
+		default:
+			return -EINVAL;
+		}
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+	return -EINVAL;
+}
+
+static irqreturn_t pat9125_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;
+
+	ret = pat9125_read_delta(data);
+	if (ret) {
+		iio_trigger_notify_done(indio_dev->trig);
+		return IRQ_NONE;
+	}
+	data->ts = iio_get_time_ns(indio_dev);
+	memcpy(&buf[0], &data->delta_x, 4);
+	memcpy(&buf[4], &data->delta_y, 4);
+	data->delta_x = 0;
+	data->delta_y = 0;
+	iio_push_to_buffers_with_timestamp(indio_dev, buf, data->ts);
+	iio_trigger_notify_done(indio_dev->trig);
+	return IRQ_HANDLED;
+}
+
+/**
+ * pat9125_event_handler() - handling ring and non ring events
+ * @irq: The irq being handled.
+ * @private: struct iio_device pointer for the device.
+ */
+static irqreturn_t pat9125_event_handler(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct pat9125_data *data = iio_priv(indio_dev);
+
+	iio_trigger_poll(data->indio_trig);
+	data->motion_detected = 1;
+	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;
+	data->buffer_mode = true;
+	/* Release IRQ on the chip */
+	return pat9125_read_delta(data);
+}
+
+static int pat9125_buffer_predisable(struct iio_dev *indio_dev)
+{
+	struct pat9125_data *data = iio_priv(indio_dev);
+
+	data->buffer_mode = false;
+	return iio_triggered_buffer_predisable(indio_dev);
+}
+
+static const struct iio_buffer_setup_ops pat9125_buffer_ops = {
+	.preenable = NULL,
+	.postenable = pat9125_buffer_postenable,
+	.predisable = pat9125_buffer_predisable,
+	.postdisable = NULL,
+};
+
+
+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,
+};
+
+/*
+ * To avoid infinite loop in "iio_trigger_notify_done",
+ * try_reenable must return 0 when it isn't in buffer mode.
+ * This implementation avoid corner case.
+ */
+static int pat9125_trig_try_reen(struct iio_trigger *trig)
+{
+	struct pat9125_data *data = iio_trigger_get_drvdata(trig);
+
+	if (data->buffer_mode)
+		return data->motion_detected;
+	else
+		return 0;
+}
+
+static const struct iio_trigger_ops pat9125_trigger_ops = {
+	.try_reenable = pat9125_trig_try_reen,
+};
+
+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");
+		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;
+	data->motion_detected = 0;
+	data->buffer_mode = false;
+
+	ret = iio_triggered_buffer_setup(indio_dev, NULL, pat9125_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 = iio_trigger_register(data->indio_trig);
+	if (ret) {
+		iio_trigger_unregister(data->indio_trig);
+		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",
+			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",
+			PAT9125_PRD_ID1_REG, ret);
+		return ret;
+	}
+	if (sensor_pid != PAT9125_SENSOR_ID_VAL)
+		return -ENODEV;
+
+	/* 
+	 * 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",
+			PAT9125_CONFIG_REG, ret);
+		return ret;
+	}
+
+	msleep(20);
+
+	ret = regmap_write(data->regmap,
+			 PAT9125_WRITE_PROTEC_REG,
+			 PAT9125_DISABLE_WRITE_PROTECT_VAL);
+	if (ret < 0) {
+		dev_err(&client->dev, "register 0x%x access failed %d",
+			PAT9125_WRITE_PROTEC_REG, ret);
+		return ret;
+	}
+	ret = regmap_write(data->regmap,
+			 PAT9125_RES_X_REG,
+			 x_resolution);
+	if (ret < 0) {
+		dev_err(&client->dev, "register 0x%x access failed %d",
+			PAT9125_RES_X_REG, ret);
+		return ret;
+	}
+	ret = regmap_write(data->regmap,
+			 PAT9125_RES_Y_REG,
+			 y_resolution);
+	if (ret < 0) {
+		dev_err(&client->dev, "register 0x%x access failed %d",
+			PAT9125_RES_Y_REG, ret);
+		return ret;
+	}
+	/* Enable write protection */
+	ret = regmap_write(data->regmap,
+			 PAT9125_WRITE_PROTEC_REG,
+			 PAT9125_ENABLE_WRITE_PROTECT_VAL);
+	if (ret < 0) {
+		dev_err(&client->dev, "register 0x%x access failed %d",
+			PAT9125_WRITE_PROTEC_REG, ret);
+		return ret;
+	}
+
+	ret = devm_iio_device_register(&client->dev, indio_dev);
+	if (ret) {
+		dev_err(&client->dev, "IIO device register failed");
+		iio_triggered_buffer_cleanup(indio_dev);
+		return ret;
+	}
+
+	i2c_set_clientdata(client, indio_dev);
+
+	dev_info(&client->dev, "%s: sensor '%s'\n",
+		 dev_name(&indio_dev->dev),
+		 client->name);
+
+	/* Make read to reset motion bit status */
+	ret = pat9125_read_delta(data);
+	if (ret) {
+		dev_err(&client->dev, "Read register failed");
+		return ret;
+	}
+
+	/* Init GPIO IRQ */
+	if (client->irq) {
+		ret = devm_request_threaded_irq(&client->dev,
+					      client->irq,
+					      pat9125_event_handler,
+					      NULL,
+					      IRQF_TRIGGER_FALLING,
+					      "pat9125",
+					      indio_dev);
+		if (ret) {
+			dev_err(&client->dev, "GPIO IRQ init failed");
+			return ret;
+		}
+	}
+	return 0;
+}
+
+static int pat9125_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = devm_iio_device_alloc(&client->dev, sizeof(struct pat9125_data));
+
+	iio_device_unregister(indio_dev);
+	iio_triggered_buffer_cleanup(indio_dev);
+
+	dev_info(&client->dev, "PAT9125 removed\n");
+
+	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,
+	.remove = pat9125_remove,
+	.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");
\ No newline at end of file
-- 
2.17.1

Re: [PATCH v2 2/3] dt-bindings: iio: ot: Add docs pat9125

[Peter Meerwald-Stadler]

> Add documentation for the optical tracker PAT9125 and
> "ot" directory for Optical Tracker chip.
> 
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> ---
>  .../devicetree/bindings/iio/ot/pat9125.txt     | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/ot/pat9125.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/ot/pat9125.txt b/Documentation/devicetree/bindings/iio/ot/pat9125.txt
> new file mode 100644
> index 000000000000..2ffacaafba8e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/ot/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

maybe delete space before :

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

-- 

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

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

[Peter Meerwald-Stadler]

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


some comments below...
a link to the datasheet would be nice

> This adds support for PixArt Imaging’s miniature low power
> optical navigation chip using LASER light source
> enabling digital surface tracking.
> 
> This IIO driver allow to read delta position on 2 axis (X and Y).

allows

> The values can be taken through ponctual "read_raw" which will issue

punctual

> a read in the device registers to return the delta between last read/buffering
> or subscribe to the data buffer feed automaticaly by a new value using an

automatica_l_ly

> IIO trigger. The buffer payload is: |32 bits delta X|32 bits delta Y|timestamp|.
> It also provide an IIO trigger which fire when a motion is detected.

provides
fires

> This trigger should be reset after each fire by a simple "read_raw" if you aren't
> in buffer mode.
> 
> The possible I2C adresses are 0x73, 0x75 and 0x79.
> 
> X and Y axis resolution can be setup independently through module parameter.

really? can't this be done using _SCALE?

> 
> The "ot" directory is added to coutain Optical Tracker drivers.

contain


how is this thing different from a mouse device?


> 
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> ---
>  drivers/iio/Kconfig      |   1 +
>  drivers/iio/Makefile     |   1 +
>  drivers/iio/ot/Kconfig   |  16 ++
>  drivers/iio/ot/Makefile  |   6 +
>  drivers/iio/ot/pat9125.c | 487 +++++++++++++++++++++++++++++++++++++++
>  5 files changed, 511 insertions(+)
>  create mode 100644 drivers/iio/ot/Kconfig
>  create mode 100644 drivers/iio/ot/Makefile
>  create mode 100644 drivers/iio/ot/pat9125.c
> 
> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
> index d08aeb41cd07..bdf1bd061168 100644
> --- a/drivers/iio/Kconfig
> +++ b/drivers/iio/Kconfig
> @@ -86,6 +86,7 @@ source "drivers/iio/light/Kconfig"
>  source "drivers/iio/magnetometer/Kconfig"
>  source "drivers/iio/multiplexer/Kconfig"
>  source "drivers/iio/orientation/Kconfig"
> +source "drivers/iio/ot/Kconfig"
>  if IIO_TRIGGER
>     source "drivers/iio/trigger/Kconfig"
>  endif #IIO_TRIGGER
> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> index cb5993251381..fdda2e185005 100644
> --- a/drivers/iio/Makefile
> +++ b/drivers/iio/Makefile
> @@ -32,6 +32,7 @@ obj-y += light/
>  obj-y += magnetometer/
>  obj-y += multiplexer/
>  obj-y += orientation/
> +obj-y += ot/
>  obj-y += potentiometer/
>  obj-y += potentiostat/
>  obj-y += pressure/
> diff --git a/drivers/iio/ot/Kconfig b/drivers/iio/ot/Kconfig
> new file mode 100644
> index 000000000000..3d17fdaa06ed
> --- /dev/null
> +++ b/drivers/iio/ot/Kconfig
> @@ -0,0 +1,16 @@
> +#
> +# 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.
> +
> +endmenu
> diff --git a/drivers/iio/ot/Makefile b/drivers/iio/ot/Makefile
> new file mode 100644
> index 000000000000..cf294917ae2c
> --- /dev/null
> +++ b/drivers/iio/ot/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/ot/pat9125.c b/drivers/iio/ot/pat9125.c
> new file mode 100644
> index 000000000000..48c1651b4e6a
> --- /dev/null
> +++ b/drivers/iio/ot/pat9125.c
> @@ -0,0 +1,487 @@
> +// 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	0x80
> +#define PAT9125_RESET_BIT		0x80

could use BIT()

> +
> +/* Registers' values */
> +#define PAT9125_SENSOR_ID_VAL			0x31
> +#define PAT9125_DISABLE_WRITE_PROTECT_VAL	0x5A
> +#define PAT9125_ENABLE_WRITE_PROTECT_VAL	0x00
> +#define PAT9125_CPI_RESOLUTION_X_VAL		0x65
> +#define PAT9125_CPI_RESOLUTION_Y_VAL		0xFF
> +
> +/* 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 */
> +	s64 ts;				/* Timestamp */
> +	s32 delta_x;
> +	s32 delta_y;
> +	s32 motion_detected;
> +	bool buffer_mode;
> +};
> +
> +static u8 x_resolution = 0x14;
> +module_param(x_resolution, byte, 0644);
> +MODULE_PARM_DESC(x_resolution, "CPI resolution setting for X axis.");
> +
> +static u8 y_resolution = 0x14;
> +module_param(y_resolution, byte, 0644);
> +MODULE_PARM_DESC(y_resolution, "CPI resolution setting for Y axis.");

I think these should be exposed as IIO _SCALE

> +
> +static const struct iio_event_spec pat9125_event = {
> +	.type = IIO_EV_TYPE_THRESH,
> +	.dir = IIO_EV_DIR_RISING,
> +	.mask_separate = BIT(IIO_EV_INFO_VALUE),
> +};
> +
> +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),
> +		.scan_index = 0,
> +		.scan_type = {
> +			.sign = 's',			\
> +			.realbits = 32,			\
> +			.storagebits = 32,		\
> +			.shift = 0,			\

no need to specify shift = 0

> +			.endianness = IIO_LE,		\
> +		},
> +		.event_spec = &pat9125_event,
> +		.num_event_specs = 1,
> +	},
> +	{
> +		.type = IIO_DISTANCE,
> +		.modified = 1,
> +		.channel2 = IIO_MOD_Y,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.scan_index = 1,
> +		.scan_type = {
> +			.sign = 's',			\
> +			.realbits = 32,			\
> +			.storagebits = 32,		\
> +			.shift = 0,			\
> +			.endianness = IIO_LE,		\

endianness should be IIO_CPU, no _LE
data is handled by _read_delta() and happily used in arithmetic; maybe 
there is endianness conversion missing in the regmap setup

> +		},
> +		.event_spec = &pat9125_event,
> +		.num_event_specs = 1,
> +	},
> +	IIO_CHAN_SOFT_TIMESTAMP(2),
> +};
> +
> +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 old_motion_status;
> +	int ret;
> +
> +	old_motion_status = data->motion_detected;
> +	data->motion_detected = 0;
> +	ret = regmap_read(regmap, PAT9125_MOTION_STATUS_REG, &status);
> +	if (ret < 0) {
> +		data->motion_detected = old_motion_status;
> +		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->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->delta_x;
> +			data->delta_x = 0;
> +			break;
> +		case IIO_MOD_Y:
> +			*val = data->delta_y;
> +			data->delta_y = 0;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		return IIO_VAL_INT;

I'd move the
return IIO_VAL_INT up, instead of the break;

> +	default:
> +		return -EINVAL;
> +	}
> +	return -EINVAL;

no need, dead code

> +}
> +
> +static irqreturn_t pat9125_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;
> +
> +	ret = pat9125_read_delta(data);
> +	if (ret) {
> +		iio_trigger_notify_done(indio_dev->trig);
> +		return IRQ_NONE;
> +	}
> +	data->ts = iio_get_time_ns(indio_dev);
> +	memcpy(&buf[0], &data->delta_x, 4);
> +	memcpy(&buf[4], &data->delta_y, 4);

why memcpy?
4 == sizeof(s32)

> +	data->delta_x = 0;
> +	data->delta_y = 0;
> +	iio_push_to_buffers_with_timestamp(indio_dev, buf, data->ts);
> +	iio_trigger_notify_done(indio_dev->trig);
> +	return IRQ_HANDLED;
> +}
> +
> +/**
> + * pat9125_event_handler() - handling ring and non ring events
> + * @irq: The irq being handled.
> + * @private: struct iio_device pointer for the device.
> + */
> +static irqreturn_t pat9125_event_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct pat9125_data *data = iio_priv(indio_dev);
> +
> +	iio_trigger_poll(data->indio_trig);
> +	data->motion_detected = 1;
> +	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;
> +	data->buffer_mode = true;
> +	/* Release IRQ on the chip */
> +	return pat9125_read_delta(data);
> +}
> +
> +static int pat9125_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct pat9125_data *data = iio_priv(indio_dev);
> +
> +	data->buffer_mode = false;
> +	return iio_triggered_buffer_predisable(indio_dev);
> +}
> +
> +static const struct iio_buffer_setup_ops pat9125_buffer_ops = {
> +	.preenable = NULL,
> +	.postenable = pat9125_buffer_postenable,
> +	.predisable = pat9125_buffer_predisable,
> +	.postdisable = NULL,
> +};
> +
> +
> +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,
> +};
> +
> +/*
> + * To avoid infinite loop in "iio_trigger_notify_done",
> + * try_reenable must return 0 when it isn't in buffer mode.
> + * This implementation avoid corner case.
> + */
> +static int pat9125_trig_try_reen(struct iio_trigger *trig)

maybe spell out try_reenable to match comment above

> +{
> +	struct pat9125_data *data = iio_trigger_get_drvdata(trig);
> +
> +	if (data->buffer_mode)
> +		return data->motion_detected;
> +	else
> +		return 0;
> +}
> +
> +static const struct iio_trigger_ops pat9125_trigger_ops = {
> +	.try_reenable = pat9125_trig_try_reen,
> +};
> +
> +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");

I'd rather remove the error message, it probably will never happen :)
dev_err() wants \n at the end of the message (here and elsewhere)

> +		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;
> +	data->motion_detected = 0;
> +	data->buffer_mode = false;
> +
> +	ret = iio_triggered_buffer_setup(indio_dev, NULL, pat9125_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 = iio_trigger_register(data->indio_trig);
> +	if (ret) {
> +		iio_trigger_unregister(data->indio_trig);

no need to unregister trigger if trigger_register() failed?

> +		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",
> +			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",
> +			PAT9125_PRD_ID1_REG, ret);
> +		return ret;
> +	}
> +	if (sensor_pid != PAT9125_SENSOR_ID_VAL)
> +		return -ENODEV;
> +
> +	/* 
> +	 * Software reset
> +	 */

single line comment?

> +	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",
> +			PAT9125_CONFIG_REG, ret);
> +		return ret;
> +	}
> +
> +	msleep(20);
> +
> +	ret = regmap_write(data->regmap,
> +			 PAT9125_WRITE_PROTEC_REG,
> +			 PAT9125_DISABLE_WRITE_PROTECT_VAL);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "register 0x%x access failed %d",
> +			PAT9125_WRITE_PROTEC_REG, ret);
> +		return ret;
> +	}
> +	ret = regmap_write(data->regmap,
> +			 PAT9125_RES_X_REG,
> +			 x_resolution);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "register 0x%x access failed %d",
> +			PAT9125_RES_X_REG, ret);
> +		return ret;
> +	}
> +	ret = regmap_write(data->regmap,
> +			 PAT9125_RES_Y_REG,
> +			 y_resolution);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "register 0x%x access failed %d",
> +			PAT9125_RES_Y_REG, ret);
> +		return ret;
> +	}
> +	/* Enable write protection */
> +	ret = regmap_write(data->regmap,
> +			 PAT9125_WRITE_PROTEC_REG,
> +			 PAT9125_ENABLE_WRITE_PROTECT_VAL);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "register 0x%x access failed %d",
> +			PAT9125_WRITE_PROTEC_REG, ret);
> +		return ret;
> +	}
> +
> +	ret = devm_iio_device_register(&client->dev, indio_dev);
> +	if (ret) {
> +		dev_err(&client->dev, "IIO device register failed");
> +		iio_triggered_buffer_cleanup(indio_dev);
> +		return ret;
> +	}
> +
> +	i2c_set_clientdata(client, indio_dev);
> +
> +	dev_info(&client->dev, "%s: sensor '%s'\n",
> +		 dev_name(&indio_dev->dev),
> +		 client->name);

please avoid log output, it just clutters the log

> +
> +	/* Make read to reset motion bit status */
> +	ret = pat9125_read_delta(data);
> +	if (ret) {
> +		dev_err(&client->dev, "Read register failed");
> +		return ret;
> +	}
> +
> +	/* Init GPIO IRQ */
> +	if (client->irq) {
> +		ret = devm_request_threaded_irq(&client->dev,
> +					      client->irq,
> +					      pat9125_event_handler,
> +					      NULL,
> +					      IRQF_TRIGGER_FALLING,
> +					      "pat9125",
> +					      indio_dev);
> +		if (ret) {
> +			dev_err(&client->dev, "GPIO IRQ init failed");
> +			return ret;
> +		}
> +	}
> +	return 0;
> +}
> +
> +static int pat9125_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev = devm_iio_device_alloc(&client->dev, sizeof(struct pat9125_data));
> +
> +	iio_device_unregister(indio_dev);
> +	iio_triggered_buffer_cleanup(indio_dev);
> +
> +	dev_info(&client->dev, "PAT9125 removed\n");
> +
> +	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,
> +	.remove = pat9125_remove,
> +	.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");
> \ No newline at end of file
> 

-- 

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

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

[Alexandre]

Hi Peter,

On 4/23/19 13:33, Peter Meerwald-Stadler wrote:
>
> how is this thing different from a mouse device?

I developed this driver to detect the board movement which can't be 
detected by accelerometer (very slow motion). I admit this use case can 
be handled by an input, and I'm agree with you, PAT9125 driver could be 
an input. But this chip is able to track different kind of motion (flat 
and rotation), and additionally have an interrupt GPIO, so using it like 
input limit the driver potential. This chip is designed to work in 
industrial measurement or embedded systems, and the IIO API match with 
these environments, so IIO is the best way to exploit the entire 
potential of this chip.

As I understand (from 
https://www.kernel.org/doc/html/v4.12/input/event-codes.html#mice ), 
mouse driver must report values when the device move. This feature 
souldn't be mandatory for an optical tracker driver, specially for cases 
where user prefers to use buffer or poll only when he need data.

All other comments will be fixed in V3.

Regards,

Alexandre

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

[Jonathan Cameron]

On Tue, 23 Apr 2019 16:06:56 +0200
Alexandre <amergnat@baylibre.com> wrote:

> Hi Peter,
> 
> On 4/23/19 13:33, Peter Meerwald-Stadler wrote:
> >
> > how is this thing different from a mouse device?  
> 
> I developed this driver to detect the board movement which can't be 
> detected by accelerometer (very slow motion). I admit this use case can 
> be handled by an input, and I'm agree with you, PAT9125 driver could be 
> an input. But this chip is able to track different kind of motion (flat 
> and rotation), and additionally have an interrupt GPIO, so using it like 
> input limit the driver potential. This chip is designed to work in 
> industrial measurement or embedded systems, and the IIO API match with 
> these environments, so IIO is the best way to exploit the entire 
> potential of this chip.
> 
> As I understand (from 
> https://www.kernel.org/doc/html/v4.12/input/event-codes.html#mice ), 
> mouse driver must report values when the device move. This feature 
> souldn't be mandatory for an optical tracker driver, specially for cases 
> where user prefers to use buffer or poll only when he need data.

Given the 'placement' of this driver isn't totally obvious, please
make sure to cc linux-input and Dmitry (maintainer) on V3.

Probably no one will mind, but the last thing we want is someone
else to put forward an input driver and we end up with two drivers
for the same thing.

Thanks,

Jonathan

> 
> All other comments will be fixed in V3.
> 
> Regards,
> 
> Alexandre
> 

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

[Jonathan Cameron]

On Tue, 23 Apr 2019 13:33:13 +0200 (CEST)
Peter Meerwald-Stadler <pmeerw@pmeerw.net> wrote:

> some comments below...
> a link to the datasheet would be nice
> 
> > This adds support for PixArt Imaging’s miniature low power
> > optical navigation chip using LASER light source
> > enabling digital surface tracking.
> > 
> > This IIO driver allow to read delta position on 2 axis (X and Y).  
> 
> allows
> 
> > The values can be taken through ponctual "read_raw" which will issue  
> 
> punctual
> 
> > a read in the device registers to return the delta between last read/buffering
> > or subscribe to the data buffer feed automaticaly by a new value using an  
> 
> automatica_l_ly
> 
> > IIO trigger. The buffer payload is: |32 bits delta X|32 bits delta Y|timestamp|.
> > It also provide an IIO trigger which fire when a motion is detected.  
> 
> provides
> fires
> 
> > This trigger should be reset after each fire by a simple "read_raw" if you aren't
> > in buffer mode.
> > 
> > The possible I2C adresses are 0x73, 0x75 and 0x79.
> > 
> > X and Y axis resolution can be setup independently through module parameter.  
> 
> really? can't this be done using _SCALE?
> 
> > 
> > The "ot" directory is added to coutain Optical Tracker drivers.  
> 
> contain
> 
> 
> how is this thing different from a mouse device?
> 
> 
> > 
> > Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
I've added my reviews to Peter's to avoid repetition.

Jonathan

> > ---
> >  drivers/iio/Kconfig      |   1 +
> >  drivers/iio/Makefile     |   1 +
> >  drivers/iio/ot/Kconfig   |  16 ++
> >  drivers/iio/ot/Makefile  |   6 +
> >  drivers/iio/ot/pat9125.c | 487 +++++++++++++++++++++++++++++++++++++++
> >  5 files changed, 511 insertions(+)
> >  create mode 100644 drivers/iio/ot/Kconfig
> >  create mode 100644 drivers/iio/ot/Makefile
> >  create mode 100644 drivers/iio/ot/pat9125.c
> > 
> > diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
> > index d08aeb41cd07..bdf1bd061168 100644
> > --- a/drivers/iio/Kconfig
> > +++ b/drivers/iio/Kconfig
> > @@ -86,6 +86,7 @@ source "drivers/iio/light/Kconfig"
> >  source "drivers/iio/magnetometer/Kconfig"
> >  source "drivers/iio/multiplexer/Kconfig"
> >  source "drivers/iio/orientation/Kconfig"
> > +source "drivers/iio/ot/Kconfig"
> >  if IIO_TRIGGER
> >     source "drivers/iio/trigger/Kconfig"
> >  endif #IIO_TRIGGER
> > diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> > index cb5993251381..fdda2e185005 100644
> > --- a/drivers/iio/Makefile
> > +++ b/drivers/iio/Makefile
> > @@ -32,6 +32,7 @@ obj-y += light/
> >  obj-y += magnetometer/
> >  obj-y += multiplexer/
> >  obj-y += orientation/
> > +obj-y += ot/
> >  obj-y += potentiometer/
> >  obj-y += potentiostat/
> >  obj-y += pressure/
> > diff --git a/drivers/iio/ot/Kconfig b/drivers/iio/ot/Kconfig
> > new file mode 100644
> > index 000000000000..3d17fdaa06ed
> > --- /dev/null
> > +++ b/drivers/iio/ot/Kconfig
> > @@ -0,0 +1,16 @@
> > +#
> > +# 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.
> > +
> > +endmenu
> > diff --git a/drivers/iio/ot/Makefile b/drivers/iio/ot/Makefile
> > new file mode 100644
> > index 000000000000..cf294917ae2c
> > --- /dev/null
> > +++ b/drivers/iio/ot/Makefile
> > @@ -0,0 +1,6 @@
> > +#
> > +# Makefile for industrial I/O Optical tracker sensor drivers
Hmm. I'd rather group by 'what' not how.

drivers/iio/position/ perhaps?
There are a lot of ways of measuring the same sort of thing.


> > +#
> > +
> > +# When adding new entries keep the list in alphabetical order
> > +obj-$(CONFIG_PAT9125) += pat9125.o
> > diff --git a/drivers/iio/ot/pat9125.c b/drivers/iio/ot/pat9125.c
> > new file mode 100644
> > index 000000000000..48c1651b4e6a
> > --- /dev/null
> > +++ b/drivers/iio/ot/pat9125.c
> > @@ -0,0 +1,487 @@
> > +// 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	0x80
> > +#define PAT9125_RESET_BIT		0x80  
> 
> could use BIT()
> 
> > +
> > +/* Registers' values */
> > +#define PAT9125_SENSOR_ID_VAL			0x31
> > +#define PAT9125_DISABLE_WRITE_PROTECT_VAL	0x5A
> > +#define PAT9125_ENABLE_WRITE_PROTECT_VAL	0x00
> > +#define PAT9125_CPI_RESOLUTION_X_VAL		0x65
> > +#define PAT9125_CPI_RESOLUTION_Y_VAL		0xFF
> > +
> > +/* 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 */
> > +	s64 ts;				/* Timestamp */
As below, ts doesn't need to be in here as it's written
and read in the same function.

> > +	s32 delta_x;
> > +	s32 delta_y;
> > +	s32 motion_detected;

That's a bool, so why s32?

> > +	bool buffer_mode;
we have iio_claim_direct_mode etc to protect against mode
switches. There shouldn't be a need to maintain a separate flag.

> > +};
> > +
> > +static u8 x_resolution = 0x14;
> > +module_param(x_resolution, byte, 0644);
> > +MODULE_PARM_DESC(x_resolution, "CPI resolution setting for X axis.");
> > +
> > +static u8 y_resolution = 0x14;
> > +module_param(y_resolution, byte, 0644);
> > +MODULE_PARM_DESC(y_resolution, "CPI resolution setting for Y axis.");  
> 
> I think these should be exposed as IIO _SCALE

Agreed.  They don't need to be writable but there should be a
means for userspace to find out what they are.
> 
> > +
> > +static const struct iio_event_spec pat9125_event = {
> > +	.type = IIO_EV_TYPE_THRESH,
> > +	.dir = IIO_EV_DIR_RISING,
> > +	.mask_separate = BIT(IIO_EV_INFO_VALUE),
> > +};
> > +
> > +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),
> > +		.scan_index = 0,
> > +		.scan_type = {
> > +			.sign = 's',			\
> > +			.realbits = 32,			\
> > +			.storagebits = 32,		\
> > +			.shift = 0,			\  
> 
> no need to specify shift = 0
> 
> > +			.endianness = IIO_LE,		\
> > +		},
> > +		.event_spec = &pat9125_event,
> > +		.num_event_specs = 1,
> > +	},
> > +	{
> > +		.type = IIO_DISTANCE,
> > +		.modified = 1,
> > +		.channel2 = IIO_MOD_Y,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> > +		.scan_index = 1,
> > +		.scan_type = {
> > +			.sign = 's',			\
> > +			.realbits = 32,			\
> > +			.storagebits = 32,		\
> > +			.shift = 0,			\
> > +			.endianness = IIO_LE,		\  
> 
> endianness should be IIO_CPU, no _LE
> data is handled by _read_delta() and happily used in arithmetic; maybe 
> there is endianness conversion missing in the regmap setup
> 
> > +		},
> > +		.event_spec = &pat9125_event,
> > +		.num_event_specs = 1,
> > +	},
> > +	IIO_CHAN_SOFT_TIMESTAMP(2),
> > +};
> > +
> > +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 old_motion_status;
> > +	int ret;
> > +
> > +	old_motion_status = data->motion_detected;
> > +	data->motion_detected = 0;
> > +	ret = regmap_read(regmap, PAT9125_MOTION_STATUS_REG, &status);
> > +	if (ret < 0) {
> > +		data->motion_detected = old_motion_status;
I don't really understand this dance around motion status.  Perhaps
a comment on why we need to preserve it?

> > +		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->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->delta_x;
I guess my comments on this came in after this version.
Report position relative to power on, not delta_x.
Nothing stops multiple readers, so delta reporting won't
provide any sort of reliable information.

> > +			data->delta_x = 0;
> > +			break;
> > +		case IIO_MOD_Y:
> > +			*val = data->delta_y;
> > +			data->delta_y = 0;
> > +			break;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +		return IIO_VAL_INT;  
> 
> I'd move the
> return IIO_VAL_INT up, instead of the break;
> 
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +	return -EINVAL;  
> 
> no need, dead code
> 
> > +}
> > +
> > +static irqreturn_t pat9125_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;
> > +
> > +	ret = pat9125_read_delta(data);
> > +	if (ret) {
> > +		iio_trigger_notify_done(indio_dev->trig);
I'm going to assume that getting here is an error path?
If so I would generally not mark the trigger done.  The
right option is to put out some sort of error message
and leave it turned off.

> > +		return IRQ_NONE;
> > +	}
> > +	data->ts = iio_get_time_ns(indio_dev);
Use a local variable.

> > +	memcpy(&buf[0], &data->delta_x, 4);
> > +	memcpy(&buf[4], &data->delta_y, 4);  
> 
> why memcpy?
> 4 == sizeof(s32)
> 
> > +	data->delta_x = 0;
> > +	data->delta_y = 0;
> > +	iio_push_to_buffers_with_timestamp(indio_dev, buf, data->ts);
> > +	iio_trigger_notify_done(indio_dev->trig);
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +/**
> > + * pat9125_event_handler() - handling ring and non ring events
> > + * @irq: The irq being handled.
> > + * @private: struct iio_device pointer for the device.
> > + */
> > +static irqreturn_t pat9125_event_handler(int irq, void *private)
> > +{
> > +	struct iio_dev *indio_dev = private;
> > +	struct pat9125_data *data = iio_priv(indio_dev);
> > +
> > +	iio_trigger_poll(data->indio_trig);
> > +	data->motion_detected = 1;

If we have the validation functions provided so this trigger
must be used with the device, then we don't need to set the flag.
we will only call the buffer push function when this has been set.

> > +	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;
> > +	data->buffer_mode = true;
> > +	/* Release IRQ on the chip */
> > +	return pat9125_read_delta(data);

We should in general be safe against this... So how did you get
here with the irq set?

> > +}
> > +
> > +static int pat9125_buffer_predisable(struct iio_dev *indio_dev)
> > +{
> > +	struct pat9125_data *data = iio_priv(indio_dev);
> > +
> > +	data->buffer_mode = false;
We iio_claim_direct_mode to protect against doing things we shouldn't
when in buffered mode.  Mind you, I'm not sure you use this for anything
anyway!

> > +	return iio_triggered_buffer_predisable(indio_dev);
> > +}
> > +
> > +static const struct iio_buffer_setup_ops pat9125_buffer_ops = {
> > +	.preenable = NULL,
> > +	.postenable = pat9125_buffer_postenable,
> > +	.predisable = pat9125_buffer_predisable,
> > +	.postdisable = NULL,
Those you don't set will default to NULL, so don't bother doing it by
hand.

> > +};
> > +
> > +
> > +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,
> > +};
> > +
> > +/*
> > + * To avoid infinite loop in "iio_trigger_notify_done",
> > + * try_reenable must return 0 when it isn't in buffer mode.
> > + * This implementation avoid corner case.
> > + */
> > +static int pat9125_trig_try_reen(struct iio_trigger *trig)  
> 
> maybe spell out try_reenable to match comment above
> 
> > +{
> > +	struct pat9125_data *data = iio_trigger_get_drvdata(trig);
> > +
> > +	if (data->buffer_mode)
> > +		return data->motion_detected;
> > +	else
> > +		return 0;
> > +}
> > +
> > +static const struct iio_trigger_ops pat9125_trigger_ops = {
> > +	.try_reenable = pat9125_trig_try_reen,
> > +};
> > +
> > +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");  
> 
> I'd rather remove the error message, it probably will never happen :)
> dev_err() wants \n at the end of the message (here and elsewhere)
> 
> > +		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;
> > +	data->motion_detected = 0;
> > +	data->buffer_mode = false;
data is from iio_priv which is allocated and cleared anyway so if these
are 'obvious' defaults like there is no need to set them in probe.
If they were non obvious 0 values, you might do so as a form of documentation,
but I don't think that is true here.


> > +
> > +	ret = iio_triggered_buffer_setup(indio_dev, NULL, pat9125_trigger_handler,
> > +					 &pat9125_buffer_ops);
> > +	if (ret) {
> > +		dev_err(&client->dev, "unable to setup triggered buffer\n");
> > +		return ret;
> > +	}
Do not mix devm_ and non devm_ versions of these functions.

The result is that the tear down at in remove is not the precise opposite
of the setup in probe.  This tends to make things much harder to review
as subtle race conditions can occur.

> > +
> > +	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 = iio_trigger_register(data->indio_trig);
> > +	if (ret) {
> > +		iio_trigger_unregister(data->indio_trig);  
> 
> no need to unregister trigger if trigger_register() failed?
> 
> > +		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",
> > +			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",
> > +			PAT9125_PRD_ID1_REG, ret);
> > +		return ret;
> > +	}
> > +	if (sensor_pid != PAT9125_SENSOR_ID_VAL)
> > +		return -ENODEV;
> > +
> > +	/* 
> > +	 * Software reset
> > +	 */  
> 
> single line comment?
Also, given we set a 'reset' bit I'd argue it doesn't state anything
that isn't already apparent.  Hence drop the comment entirely.
Same for some of the others in here.  Comments add a burden of
things that need to be kept up to date as a driver changes.  Don't
put them in unless that burden is worth paying - i.e. they tell
us something that isn't obvious from the code.

> 
> > +	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",
> > +			PAT9125_CONFIG_REG, ret);
> > +		return ret;
> > +	}
> > +
> > +	msleep(20);
> > +
> > +	ret = regmap_write(data->regmap,
> > +			 PAT9125_WRITE_PROTEC_REG,
> > +			 PAT9125_DISABLE_WRITE_PROTECT_VAL);
> > +	if (ret < 0) {
> > +		dev_err(&client->dev, "register 0x%x access failed %d",
> > +			PAT9125_WRITE_PROTEC_REG, ret);
> > +		return ret;
> > +	}
> > +	ret = regmap_write(data->regmap,
> > +			 PAT9125_RES_X_REG,
> > +			 x_resolution);
> > +	if (ret < 0) {
> > +		dev_err(&client->dev, "register 0x%x access failed %d",
> > +			PAT9125_RES_X_REG, ret);
> > +		return ret;
> > +	}
> > +	ret = regmap_write(data->regmap,
> > +			 PAT9125_RES_Y_REG,
> > +			 y_resolution);
> > +	if (ret < 0) {
> > +		dev_err(&client->dev, "register 0x%x access failed %d",
> > +			PAT9125_RES_Y_REG, ret);
> > +		return ret;
> > +	}
> > +	/* Enable write protection */
> > +	ret = regmap_write(data->regmap,
> > +			 PAT9125_WRITE_PROTEC_REG,
> > +			 PAT9125_ENABLE_WRITE_PROTECT_VAL);
> > +	if (ret < 0) {
> > +		dev_err(&client->dev, "register 0x%x access failed %d",
> > +			PAT9125_WRITE_PROTEC_REG, ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = devm_iio_device_register(&client->dev, indio_dev);
> > +	if (ret) {
> > +		dev_err(&client->dev, "IIO device register failed");
> > +		iio_triggered_buffer_cleanup(indio_dev);
> > +		return ret;
> > +	}
> > +
> > +	i2c_set_clientdata(client, indio_dev);
> > +
> > +	dev_info(&client->dev, "%s: sensor '%s'\n",
> > +		 dev_name(&indio_dev->dev),
> > +		 client->name);  
> 
> please avoid log output, it just clutters the log
> 
> > +
> > +	/* Make read to reset motion bit status */
This is occurring after you have exposed the device to userspace
(the device_register above) so I'm going to guess there may be a race
here.  Any setup should occur before that call.

> > +	ret = pat9125_read_delta(data);
> > +	if (ret) {
> > +		dev_err(&client->dev, "Read register failed");
> > +		return ret;
> > +	}
> > +
> > +	/* Init GPIO IRQ */
> > +	if (client->irq) {
> > +		ret = devm_request_threaded_irq(&client->dev,
> > +					      client->irq,
> > +					      pat9125_event_handler,
> > +					      NULL,
> > +					      IRQF_TRIGGER_FALLING,
> > +					      "pat9125",
> > +					      indio_dev);
> > +		if (ret) {
> > +			dev_err(&client->dev, "GPIO IRQ init failed");
> > +			return ret;
> > +		}
Also should be before we start letting userspace play with the device.

> > +	}
> > +	return 0;
> > +}
> > +
> > +static int pat9125_remove(struct i2c_client *client)
> > +{
> > +	struct iio_dev *indio_dev = devm_iio_device_alloc(&client->dev, sizeof(struct pat9125_data));
Umm?  You allocate a new device here...

> > +
> > +	iio_device_unregister(indio_dev);
> > +	iio_triggered_buffer_cleanup(indio_dev);
Use devm_ for all these in probe and the remove isn't needed at all.

> > +
> > +	dev_info(&client->dev, "PAT9125 removed\n");
Drop any prints like this.  There is no information that can't been
easily discovered from other sources and it clutters up the
kernel log.

> > +
> > +	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,
> > +	.remove = pat9125_remove,
> > +	.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");
> > \ No newline at end of file
Please add one.

> >   
> 

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

[Rob Herring]

On Tue, Apr 23, 2019 at 5:40 AM Alexandre Mergnat <amergnat@baylibre.com> 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.txt | 1 +
>  1 file changed, 1 insertion(+)

I've converted this file to json-schema as of v5.2-rc1. See commit
8122de54602e. Sorry, but you will have to rework this patch.

Rob