* [PATCH v2 0/3] Add PAT9125 optical tracker driver @ 2019-04-23 10:40 Alexandre Mergnat 2019-04-23 10:40 ` [PATCH v2 1/3] dt-bindings: Add pixart vendor Alexandre Mergnat ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Alexandre Mergnat @ 2019-04-23 10:40 UTC (permalink / raw) To: robh+dt, mark.rutland, jic23, knaack.h, lars, pmeerw Cc: linux-kernel, linux-iio, baylibre-upstreaming, 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 1/3] dt-bindings: Add pixart vendor 2019-04-23 10:40 [PATCH v2 0/3] Add PAT9125 optical tracker driver Alexandre Mergnat @ 2019-04-23 10:40 ` Alexandre Mergnat 2019-05-17 2:35 ` Rob Herring 2019-04-23 10:40 ` [PATCH v2 2/3] dt-bindings: iio: ot: Add docs pat9125 Alexandre Mergnat 2019-04-23 10:40 ` [PATCH v2 3/3] iio: Add PAT9125 optical tracker sensor Alexandre Mergnat 2 siblings, 1 reply; 10+ messages in thread From: Alexandre Mergnat @ 2019-04-23 10:40 UTC (permalink / raw) To: robh+dt, mark.rutland, jic23, knaack.h, lars, pmeerw Cc: linux-kernel, linux-iio, baylibre-upstreaming, 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: Add pixart vendor 2019-04-23 10:40 ` [PATCH v2 1/3] dt-bindings: Add pixart vendor Alexandre Mergnat @ 2019-05-17 2:35 ` Rob Herring 0 siblings, 0 replies; 10+ messages in thread From: Rob Herring @ 2019-05-17 2:35 UTC (permalink / raw) To: Alexandre Mergnat Cc: Mark Rutland, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald, linux-kernel, open list:IIO SUBSYSTEM AND DRIVERS, baylibre-upstreaming 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 2/3] dt-bindings: iio: ot: Add docs pat9125 2019-04-23 10:40 [PATCH v2 0/3] Add PAT9125 optical tracker driver Alexandre Mergnat 2019-04-23 10:40 ` [PATCH v2 1/3] dt-bindings: Add pixart vendor Alexandre Mergnat @ 2019-04-23 10:40 ` Alexandre Mergnat 2019-04-23 11:01 ` Peter Meerwald-Stadler 2019-04-23 10:40 ` [PATCH v2 3/3] iio: Add PAT9125 optical tracker sensor Alexandre Mergnat 2 siblings, 1 reply; 10+ messages in thread From: Alexandre Mergnat @ 2019-04-23 10:40 UTC (permalink / raw) To: robh+dt, mark.rutland, jic23, knaack.h, lars, pmeerw Cc: linux-kernel, linux-iio, baylibre-upstreaming, 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/3] dt-bindings: iio: ot: Add docs pat9125 2019-04-23 10:40 ` [PATCH v2 2/3] dt-bindings: iio: ot: Add docs pat9125 Alexandre Mergnat @ 2019-04-23 11:01 ` Peter Meerwald-Stadler 0 siblings, 0 replies; 10+ messages in thread From: Peter Meerwald-Stadler @ 2019-04-23 11:01 UTC (permalink / raw) To: Alexandre Mergnat Cc: robh+dt, mark.rutland, jic23, linux-kernel, linux-iio, baylibre-upstreaming > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 3/3] iio: Add PAT9125 optical tracker sensor 2019-04-23 10:40 [PATCH v2 0/3] Add PAT9125 optical tracker driver Alexandre Mergnat 2019-04-23 10:40 ` [PATCH v2 1/3] dt-bindings: Add pixart vendor Alexandre Mergnat 2019-04-23 10:40 ` [PATCH v2 2/3] dt-bindings: iio: ot: Add docs pat9125 Alexandre Mergnat @ 2019-04-23 10:40 ` Alexandre Mergnat 2019-04-23 11:33 ` Peter Meerwald-Stadler 2 siblings, 1 reply; 10+ messages in thread From: Alexandre Mergnat @ 2019-04-23 10:40 UTC (permalink / raw) To: robh+dt, mark.rutland, jic23, knaack.h, lars, pmeerw Cc: linux-kernel, linux-iio, baylibre-upstreaming, 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 3/3] iio: Add PAT9125 optical tracker sensor 2019-04-23 10:40 ` [PATCH v2 3/3] iio: Add PAT9125 optical tracker sensor Alexandre Mergnat @ 2019-04-23 11:33 ` Peter Meerwald-Stadler 2019-04-23 14:06 ` Alexandre 2019-04-27 12:52 ` Jonathan Cameron 0 siblings, 2 replies; 10+ messages in thread From: Peter Meerwald-Stadler @ 2019-04-23 11:33 UTC (permalink / raw) To: Alexandre Mergnat; +Cc: linux-kernel, linux-iio, baylibre-upstreaming [-- 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 3/3] iio: Add PAT9125 optical tracker sensor 2019-04-23 11:33 ` Peter Meerwald-Stadler @ 2019-04-23 14:06 ` Alexandre 2019-04-27 12:11 ` Jonathan Cameron 2019-04-27 12:52 ` Jonathan Cameron 1 sibling, 1 reply; 10+ messages in thread From: Alexandre @ 2019-04-23 14:06 UTC (permalink / raw) To: Peter Meerwald-Stadler; +Cc: linux-kernel, linux-iio, baylibre-upstreaming 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 3/3] iio: Add PAT9125 optical tracker sensor 2019-04-23 14:06 ` Alexandre @ 2019-04-27 12:11 ` Jonathan Cameron 0 siblings, 0 replies; 10+ messages in thread From: Jonathan Cameron @ 2019-04-27 12:11 UTC (permalink / raw) To: Alexandre Cc: Peter Meerwald-Stadler, linux-kernel, linux-iio, baylibre-upstreaming 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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 3/3] iio: Add PAT9125 optical tracker sensor 2019-04-23 11:33 ` Peter Meerwald-Stadler 2019-04-23 14:06 ` Alexandre @ 2019-04-27 12:52 ` Jonathan Cameron 1 sibling, 0 replies; 10+ messages in thread From: Jonathan Cameron @ 2019-04-27 12:52 UTC (permalink / raw) To: Peter Meerwald-Stadler Cc: Alexandre Mergnat, linux-kernel, linux-iio, baylibre-upstreaming 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. > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-05-17 2:36 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-04-23 10:40 [PATCH v2 0/3] Add PAT9125 optical tracker driver Alexandre Mergnat 2019-04-23 10:40 ` [PATCH v2 1/3] dt-bindings: Add pixart vendor Alexandre Mergnat 2019-05-17 2:35 ` Rob Herring 2019-04-23 10:40 ` [PATCH v2 2/3] dt-bindings: iio: ot: Add docs pat9125 Alexandre Mergnat 2019-04-23 11:01 ` Peter Meerwald-Stadler 2019-04-23 10:40 ` [PATCH v2 3/3] iio: Add PAT9125 optical tracker sensor Alexandre Mergnat 2019-04-23 11:33 ` Peter Meerwald-Stadler 2019-04-23 14:06 ` Alexandre 2019-04-27 12:11 ` Jonathan Cameron 2019-04-27 12:52 ` Jonathan Cameron
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).