* [PATCH 0/3] Add PAT6125 optical tracker driver
@ 2019-04-05 9:34 Alexandre Mergnat
2019-04-05 9:34 ` [PATCH 1/3] dt-bindings: Add pixart vendor Alexandre Mergnat
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Alexandre Mergnat @ 2019-04-05 9:34 UTC (permalink / raw)
To: robh+dt, mark.rutland, jic23, knaack.h, lars, pmeerw
Cc: linux-kernel, linux-iio, baylibre-upstreaming, Alexandre Mergnat
PixArt Imaging PAT6125 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.
Unfortunately, the device doesn't support "on-the-fly" configuration.
This patchset :
- Update vendor prefix
- Add the bindings for this device
- Add the device driver
- Add directory for optical tracker devices
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 | 407 +++++++++++++++++++++
7 files changed, 450 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.7.4
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/3] dt-bindings: Add pixart vendor 2019-04-05 9:34 [PATCH 0/3] Add PAT6125 optical tracker driver Alexandre Mergnat @ 2019-04-05 9:34 ` Alexandre Mergnat 2019-04-05 9:34 ` [PATCH 2/3] dt-bindings: iio: ot: Add docs pat9125 Alexandre Mergnat 2019-04-05 9:34 ` [PATCH 3/3] iio: Add PAT9125 optical tracker sensor Alexandre Mergnat 2 siblings, 0 replies; 11+ messages in thread From: Alexandre Mergnat @ 2019-04-05 9:34 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 8162b0e..dbd8893 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.7.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/3] dt-bindings: iio: ot: Add docs pat9125 2019-04-05 9:34 [PATCH 0/3] Add PAT6125 optical tracker driver Alexandre Mergnat 2019-04-05 9:34 ` [PATCH 1/3] dt-bindings: Add pixart vendor Alexandre Mergnat @ 2019-04-05 9:34 ` Alexandre Mergnat 2019-04-05 9:34 ` [PATCH 3/3] iio: Add PAT9125 optical tracker sensor Alexandre Mergnat 2 siblings, 0 replies; 11+ messages in thread From: Alexandre Mergnat @ 2019-04-05 9:34 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> --- Documentation/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 0000000..2ffacaa --- /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.7.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/3] iio: Add PAT9125 optical tracker sensor 2019-04-05 9:34 [PATCH 0/3] Add PAT6125 optical tracker driver Alexandre Mergnat 2019-04-05 9:34 ` [PATCH 1/3] dt-bindings: Add pixart vendor Alexandre Mergnat 2019-04-05 9:34 ` [PATCH 2/3] dt-bindings: iio: ot: Add docs pat9125 Alexandre Mergnat @ 2019-04-05 9:34 ` Alexandre Mergnat 2019-04-07 10:20 ` Jonathan Cameron 2 siblings, 1 reply; 11+ messages in thread From: Alexandre Mergnat @ 2019-04-05 9:34 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 deltas or subscribe to the data buffer feed automaticaly by a new value using a trigger gpio. The buffer payload is: |16 bits delta X|16 bits delta Y|timestamp|. The possible I2C adresses are 0x73, 0x75 and 0x79. Unfortunately, the device configuration must be hardcoded in the initialization function and can't be changed "on-the-fly" in user space due to the lack of configuration interface. 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 | 407 +++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 431 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 d08aeb4..bdf1bd0 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 cb59932..fdda2e1 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 0000000..3d17fda --- /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 0000000..cf29491 --- /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 0000000..f416bfa --- /dev/null +++ b/drivers/iio/ot/pat9125.c @@ -0,0 +1,407 @@ +// SPDX-License-Identifier: (GPL-2.0) +/* + * Copyright (C) 2018 BayLibre, SAS + * Author: Alexandre Mergnat <amergnat@baylibre.com> + */ + +#include <linux/i2c.h> +#include <linux/module.h> +#include <linux/kernel.h> +#include <linux/regmap.h> +#include <linux/delay.h> +#include <linux/interrupt.h> +#include <linux/slab.h> + +#include <linux/iio/iio.h> +#include <linux/iio/sysfs.h> +#include <linux/iio/events.h> +#include <linux/iio/buffer.h> +#include <linux/iio/kfifo_buf.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 + +/* Masks */ +#define PAT9125_VALID_MOTION_DATA_MASK 0x80 +#define PAT9125_RESET_MASK 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 +#define PAT9125_SAMPLED_VAL_BYTE_SIZE 2 /* 12 bits by default */ +#define PAT9125_TIMESTAMP_BYTE_SIZE 8 /* 64 bits */ +#define PAT9125_PAYLOAD_BYTE_SIZE \ + (2 * PAT9125_SAMPLED_VAL_BYTE_SIZE + PAT9125_TIMESTAMP_BYTE_SIZE) +#define PAT9125_REALBITS_XY_VAL \ + (2 * PAT9125_SAMPLED_VAL_BIT_SIZE) +#define PAT9125_STORAGEBITS_XY_VAL \ + (2 * PAT9125_SAMPLED_VAL_BYTE_SIZE) + +struct pat9125_data { + struct i2c_client *client; + struct regmap *regmap; + s16 delta_x; + s16 delta_y; +}; + +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_AND_Y, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), + .address = 0, + .scan_index = 0, + .scan_type = { + .sign = 's', \ + .realbits = 24, \ + .storagebits = 32, \ + .shift = 8, \ + .endianness = IIO_BE, \ + }, + .event_spec = &pat9125_event, + .num_event_specs = 1, + }, + IIO_CHAN_SOFT_TIMESTAMP(1), +}; + +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_xy = 0; + int r; + + r = regmap_read(regmap, PAT9125_MOTION_STATUS_REG, &status); + if (r < 0) + return r; + + /* Check motion bit in bit7 */ + if (status & PAT9125_VALID_MOTION_DATA_MASK) { + r = regmap_read(regmap, PAT9125_DELTA_X_LO_REG, &val_x); + if (r < 0) + return r; + + r = regmap_read(regmap, PAT9125_DELTA_Y_LO_REG, &val_y); + if (r < 0) + return r; + + r = regmap_read(regmap, PAT9125_DELTA_XY_HI_REG, &val_xy); + if (r < 0) + return r; + + data->delta_x = val_x | ((val_xy << 4) & 0xF00); + data->delta_y = val_y | ((val_xy << 8) & 0xF00); + + if (data->delta_x & 0x800) + data->delta_x |= 0xF000; + + if (data->delta_y & 0x800) + data->delta_y |= 0xF000; + } + + 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; + + *val = data->delta_x; + *val2 = data->delta_y; + return IIO_VAL_INT; + default: + return -EINVAL; + } + + return -EINVAL; +} + +/** + * pat9125_read_event_value() - return last sampled value. + **/ +static int pat9125_read_event_value(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir, + enum iio_event_info info, + int *val, + int *val2) +{ + struct pat9125_data *data = iio_priv(indio_dev); + + switch (info) { + case IIO_EV_INFO_VALUE: + val[0] = data->delta_x; + val[1] = data->delta_y; + *val2 = 2; + return IIO_VAL_INT_MULTIPLE; + default: + return -EINVAL; + } +} + +/** + * 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); + int ret = 0; + u8 *payload; + s64 last_timestamp = iio_get_time_ns(indio_dev); + + payload = kmalloc(sizeof(s64) + 2 * + PAT9125_SAMPLED_VAL_BYTE_SIZE, GFP_KERNEL); + + ret = pat9125_read_delta(data); + if (ret) + return ret; + memcpy(&payload[0], &data->delta_x, 2); + memcpy(&payload[2], &data->delta_y, 2); + + iio_push_to_buffers_with_timestamp(indio_dev, payload, last_timestamp); + iio_push_event(indio_dev, + IIO_MOD_EVENT_CODE(IIO_DISTANCE, + 0, + IIO_MOD_X_AND_Y, + IIO_EV_TYPE_THRESH, + IIO_EV_DIR_RISING), + last_timestamp); + + return IRQ_HANDLED; +} + +static int pat9125_configure_ring(struct iio_dev *indio_dev) +{ + struct iio_buffer *buffer; + struct pat9125_data *data = iio_priv(indio_dev); + + buffer = devm_iio_kfifo_allocate(&data->client->dev); + if (!buffer) + return -ENOMEM; + + iio_device_attach_buffer(indio_dev, buffer); + indio_dev->modes |= INDIO_BUFFER_SOFTWARE; + + return 0; +} + +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, + .read_event_value = &pat9125_read_event_value, +}; + +static int pat9125_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + struct pat9125_data *data; + struct iio_dev *indio_dev; + int r, 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); + data->client = client; + + 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; + + r = pat9125_configure_ring(indio_dev); + if (r < 0) { + dev_err(&client->dev, "FIFO buffer allocation failed"); + return r; + } + + 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 */ + r = regmap_read(data->regmap, PAT9125_PRD_ID1_REG, &sensor_pid); + if (r < 0) + goto reg_access_fail; + if (sensor_pid != PAT9125_SENSOR_ID_VAL) + return -ENODEV; + + /* Software reset (i.e. set bit7 to 1). + * It will reset to 0 automatically + */ + r = regmap_write_bits(data->regmap, + PAT9125_CONFIG_REG, + PAT9125_RESET_MASK, + 1); + if (r < 0) + goto reg_access_fail; + + /* Delay 20ms */ + msleep(20); + + /* Disable write protect */ + r = regmap_write(data->regmap, + PAT9125_WRITE_PROTEC_REG, + PAT9125_DISABLE_WRITE_PROTECT_VAL); + if (r < 0) + goto reg_access_fail; + + /* Set X-axis resolution (depends on application) */ + r = regmap_write(data->regmap, + PAT9125_RES_X_REG, + 0x0A); + if (r < 0) + goto reg_access_fail; + + /* Set Y-axis resolution (depends on application) */ + r = regmap_write(data->regmap, + PAT9125_RES_Y_REG, + 0x0A); + if (r < 0) + goto reg_access_fail; + + /* Enable write protection */ + r = regmap_write(data->regmap, + PAT9125_WRITE_PROTEC_REG, + PAT9125_ENABLE_WRITE_PROTECT_VAL); + if (r < 0) + goto reg_access_fail; + + r = devm_iio_device_register(&client->dev, indio_dev); + if (r) { + dev_err(&client->dev, "IIO device register failed"); + return r; + } + + 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 */ + r = pat9125_read_delta(data); + if (r) + goto reg_access_fail; + + /* Init GPIO IRQ */ + if (client->irq) { + r = devm_request_threaded_irq(&client->dev, + client->irq, + NULL, + pat9125_event_handler, + IRQF_TRIGGER_LOW | IRQF_ONESHOT, + "pat9125", + indio_dev); + if (r) + return r; + } + return 0; + +reg_access_fail: + dev_err(&client->dev, "register access failed %d", r); + return r; +} + +static int pat9125_remove(struct i2c_client *client) +{ + 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"); + -- 2.7.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] iio: Add PAT9125 optical tracker sensor 2019-04-05 9:34 ` [PATCH 3/3] iio: Add PAT9125 optical tracker sensor Alexandre Mergnat @ 2019-04-07 10:20 ` Jonathan Cameron 2019-04-16 12:54 ` Alexandre [not found] ` <f8ffca5b-09a8-cad7-4561-bf1263388228@baylibre.com> 0 siblings, 2 replies; 11+ messages in thread From: Jonathan Cameron @ 2019-04-07 10:20 UTC (permalink / raw) To: Alexandre Mergnat Cc: robh+dt, mark.rutland, knaack.h, lars, pmeerw, linux-kernel, linux-iio, baylibre-upstreaming, Dmitry Torokhov, linux-input On Fri, 5 Apr 2019 09:34:30 +0000 Alexandre Mergnat <amergnat@baylibre.com> wrote: > This adds support for PixArt Imaging’s miniature low power > optical navigation chip using LASER light source > enabling digital surface tracking. Hi Alexandre, So I have no problem with this as an IIO driver, but for devices that are somewhat 'on the edge' I always like to get a clear answer to the question: Why not input? I would also argue that, to actually be 'useful' we would typically need some representation of the 'mechanicals' that are providing the motion being measured. Looking at the datasheet this includes, rotating shafts (side or end on), disk edges and flat surface tracking (mouse like). That's easy enough to do with the iio in kernel consumer interface. These are similar to when we handle analog electronic front ends. I you can, please describe what it is being used for in your application as that may give us somewhere to start! + CC Dmitry and linux-input. > > 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 deltas or subscribe > to the data buffer feed automaticaly by a new value using a > trigger gpio. The buffer payload is: |16 bits delta X|16 bits delta Y|timestamp|. > The possible I2C adresses are 0x73, 0x75 and 0x79. > > Unfortunately, the device configuration must be hardcoded in the initialization > function and can't be changed "on-the-fly" in user space due to the lack of > configuration interface. > > The "ot" directory is added to coutain Optical Tracker drivers. From a shared interface point of view we don't really care 'how', but rather 'what'. So arguably this is actually measuring differential position, or sort of 'velocity'. I think it should probably be mapped to simple position though. See below. > > Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> Comments inline. > --- > drivers/iio/Kconfig | 1 + > drivers/iio/Makefile | 1 + > drivers/iio/ot/Kconfig | 16 ++ > drivers/iio/ot/Makefile | 6 + > drivers/iio/ot/pat9125.c | 407 +++++++++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 431 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 d08aeb4..bdf1bd0 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 cb59932..fdda2e1 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 0000000..3d17fda > --- /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 0000000..cf29491 > --- /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 0000000..f416bfa > --- /dev/null > +++ b/drivers/iio/ot/pat9125.c > @@ -0,0 +1,407 @@ > +// SPDX-License-Identifier: (GPL-2.0) > +/* > + * Copyright (C) 2018 BayLibre, SAS Probably going to need to add 2019! > + * Author: Alexandre Mergnat <amergnat@baylibre.com> > + */ > + > +#include <linux/i2c.h> > +#include <linux/module.h> > +#include <linux/kernel.h> > +#include <linux/regmap.h> > +#include <linux/delay.h> > +#include <linux/interrupt.h> > +#include <linux/slab.h> > + > +#include <linux/iio/iio.h> > +#include <linux/iio/sysfs.h> > +#include <linux/iio/events.h> > +#include <linux/iio/buffer.h> > +#include <linux/iio/kfifo_buf.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 > + > +/* Masks */ > +#define PAT9125_VALID_MOTION_DATA_MASK 0x80 > +#define PAT9125_RESET_MASK 0x80 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 These defines actually mostly add to obscure what is going on. In my mind, the values themselves are clearer than the defines. > +#define PAT9125_SAMPLED_VAL_BYTE_SIZE 2 /* 12 bits by default */ > +#define PAT9125_TIMESTAMP_BYTE_SIZE 8 /* 64 bits */ > +#define PAT9125_PAYLOAD_BYTE_SIZE \ > + (2 * PAT9125_SAMPLED_VAL_BYTE_SIZE + PAT9125_TIMESTAMP_BYTE_SIZE) > +#define PAT9125_REALBITS_XY_VAL \ > + (2 * PAT9125_SAMPLED_VAL_BIT_SIZE) > +#define PAT9125_STORAGEBITS_XY_VAL \ > + (2 * PAT9125_SAMPLED_VAL_BYTE_SIZE) > + > +struct pat9125_data { > + struct i2c_client *client; I think client is only used to get dev for debug message purposes. you can get that in a number of other ways including from the regmap. Hence drop client from this structure. > + struct regmap *regmap; > + s16 delta_x; > + s16 delta_y; > +}; > + > +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_AND_Y, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), > + .address = 0, > + .scan_index = 0, > + .scan_type = { > + .sign = 's', \ > + .realbits = 24, \ That is not how mod_x_and_y is used. It only exists for events, not to let you define your own version of channels with two elements. In IIO these are two separate channels and must be represented as such. There is another problem here, in what type of channel this actually is. Now I may have the following wrong as the datasheet is less than super clear!. It's not outputting position at all, but rather a delta value from when it was either: 1. last read? (not sure on this) 2. last fired an interrupt? 3. last took a sample? (fixed sampling frequency). If 1 or 2, I would suggest that you provide absolute position to Linux. So add the value to a software counter and provide that. 32 bits should be plenty of resolution for that. If 3, it's a velocity sensor so report it as velocity not position. If it is actually not possible to report the two channels separately then don't report them at all except via the buffered interface and set the available scan masks so that both are on. > + .storagebits = 32, \ > + .shift = 8, \ > + .endianness = IIO_BE, \ > + }, > + .event_spec = &pat9125_event, > + .num_event_specs = 1, > + }, > + IIO_CHAN_SOFT_TIMESTAMP(1), > +}; > + > +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_xy = 0; val_xy is a confusing name. perhaps val_high_nibbles? > + int r; ret, be consistent with naming. > + > + r = regmap_read(regmap, PAT9125_MOTION_STATUS_REG, &status); > + if (r < 0) > + return r; > + > + /* Check motion bit in bit7 */ Why? This is reading a value, if there is no motion, then is the right answer not to set it to such? i.e. 0. Also we carefully used a macro to hide the fact it is bit7 so don't put it in the comment ;) > + if (status & PAT9125_VALID_MOTION_DATA_MASK) { > + r = regmap_read(regmap, PAT9125_DELTA_X_LO_REG, &val_x); > + if (r < 0) > + return r; One thing that isn't clear from the datasheet I am looking at is whether this is potentially racey. What stops an update whilst we are reading? > + > + r = regmap_read(regmap, PAT9125_DELTA_Y_LO_REG, &val_y); > + if (r < 0) > + return r; > + > + r = regmap_read(regmap, PAT9125_DELTA_XY_HI_REG, &val_xy); > + if (r < 0) > + return r; > + > + data->delta_x = val_x | ((val_xy << 4) & 0xF00); > + data->delta_y = val_y | ((val_xy << 8) & 0xF00); > + > + if (data->delta_x & 0x800) > + data->delta_x |= 0xF000; This looks like sign extension to me. We have standard functions for that.sign_extend32. Note that it's actually cheaper to do a 32 bit sign extend to 32 bits than it is to 16, then assign back to 16 if you want to. > + > + if (data->delta_y & 0x800) > + data->delta_y |= 0xF000; > + } > + > + 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; > + > + *val = data->delta_x; > + *val2 = data->delta_y; That's not how the IIO interface works. You can't just push values into different fields. These are two axis, and are independent. If you absolutely have to read them together then it must be done with the buffered interface and we simply don't provide a read raw interface at all. > + return IIO_VAL_INT; > + default: > + return -EINVAL; > + } > + > + return -EINVAL; > +} > + > +/** > + * pat9125_read_event_value() - return last sampled value. > + **/ > +static int pat9125_read_event_value(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir, > + enum iio_event_info info, > + int *val, > + int *val2) > +{ > + struct pat9125_data *data = iio_priv(indio_dev); > + > + switch (info) { > + case IIO_EV_INFO_VALUE: > + val[0] = data->delta_x; > + val[1] = data->delta_y; Silly question for you. What happens if you set the delta values to 0? Do we get an interrupt which is effectively data ready? If we do, you might want to think about a scheme where that is an option. As things currently stand we have a confusing interface where changing this threshold effects the buffered data output. That should only be the case if this interface is for a trigger, not an event. > + *val2 = 2; > + return IIO_VAL_INT_MULTIPLE; > + default: > + return -EINVAL; > + } > +} > + > +/** > + * pat9125_event_handler() - handling ring and non ring events > + * @irq: The irq being handled. > + * @private: struct iio_device pointer for the device. This blank line doesn't add anything to readability so tidy it up. > + * > + */ > +static irqreturn_t pat9125_event_handler(int irq, void *private) > +{ > + struct iio_dev *indio_dev = private; > + struct pat9125_data *data = iio_priv(indio_dev); > + int ret = 0; ret is always set below, so don't initialise. > + u8 *payload; > + s64 last_timestamp = iio_get_time_ns(indio_dev); > + > + payload = kmalloc(sizeof(s64) + 2 * > + PAT9125_SAMPLED_VAL_BYTE_SIZE, GFP_KERNEL); Why on the stack? It's small and last thing you want to do is a memory allocation ever time. Also, you haven't allowed for the required alignment of the timestamp. This needs to be a multiple of 8 bytes. > + > + ret = pat9125_read_delta(data); > + if (ret) > + return ret; > + memcpy(&payload[0], &data->delta_x, 2); Make payload and array of u16 and you can just assign directly. > + memcpy(&payload[2], &data->delta_y, 2); > + > + iio_push_to_buffers_with_timestamp(indio_dev, payload, last_timestamp); Hmm. This is not how IIO is normally used. So the oddity here is the sensor only produces an interrupt on a change of value. So in IIO terms this would normally be considered a 'value change trigger' driving normal buffered sampling. I think it is also perfectly valid if someone wants evenly spaced samples to read on demand? If so then you definitely want to allow use of high resolution timer triggers and similar. So, create a trigger that is base don the 'event' - there are other drivers doing this. It's possible that you might want that 'trigger' to also call the iio event interface, but usually only if there are usecases where knowing "motion happened" is useful rather than what the motion was. Then use the triggered_buffer helpers to set up all the data capture side of things. > + iio_push_event(indio_dev, > + IIO_MOD_EVENT_CODE(IIO_DISTANCE, > + 0, > + IIO_MOD_X_AND_Y, That one is wrong btw, surely it is IIO_MOD_X_OR_Y. You aren't triggering on motion in 'both X and Y' directions, motion in one of them is enough. > + IIO_EV_TYPE_THRESH, > + IIO_EV_DIR_RISING), > + last_timestamp); > + > + return IRQ_HANDLED; > +} > + > +static int pat9125_configure_ring(struct iio_dev *indio_dev) > +{ > + struct iio_buffer *buffer; > + struct pat9125_data *data = iio_priv(indio_dev); > + > + buffer = devm_iio_kfifo_allocate(&data->client->dev); > + if (!buffer) > + return -ENOMEM; Hmm. Direct use of kfifo's is fairly unusual as it normally means there is something unusual about how the device is being mapped to the IIO elements. > + > + iio_device_attach_buffer(indio_dev, buffer); > + indio_dev->modes |= INDIO_BUFFER_SOFTWARE; > + > + return 0; > +} > + > +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, > + .read_event_value = &pat9125_read_event_value, > +}; > + > +static int pat9125_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct pat9125_data *data; > + struct iio_dev *indio_dev; > + int r, 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); > + data->client = client; > + > + 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; > + > + r = pat9125_configure_ring(indio_dev); I'd rename this, its a fifo, rather than a ring (it was a ring early in IIO's history so the naming still kicks around in a few drivers). > + if (r < 0) { > + dev_err(&client->dev, "FIFO buffer allocation failed"); > + return r; > + } > + > + 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)); This is wrapped onto more lines than necessary. Kernel coding convention is to keep as many parameters on each line as fit under 80 chars. > + return PTR_ERR(data->regmap); > + } > + > + /* Check device ID */ > + r = regmap_read(data->regmap, PAT9125_PRD_ID1_REG, &sensor_pid); > + if (r < 0) > + goto reg_access_fail; > + if (sensor_pid != PAT9125_SENSOR_ID_VAL) > + return -ENODEV; > + > + /* Software reset (i.e. set bit7 to 1). Multiline, comment style in most of the kernel, including IIO is /* * Soft reset... */ > + * It will reset to 0 automatically > + */ > + r = regmap_write_bits(data->regmap, > + PAT9125_CONFIG_REG, > + PAT9125_RESET_MASK, > + 1); > + if (r < 0) > + goto reg_access_fail; > + > + /* Delay 20ms */ > + msleep(20); > + > + /* Disable write protect */ Don't add comments for things that are obvious from the code. (i.e. the sleep above and this one). > + r = regmap_write(data->regmap, > + PAT9125_WRITE_PROTEC_REG, > + PAT9125_DISABLE_WRITE_PROTECT_VAL); > + if (r < 0) > + goto reg_access_fail; > + > + /* Set X-axis resolution (depends on application) */ > + r = regmap_write(data->regmap, > + PAT9125_RES_X_REG, > + 0x0A); Need to control this from somewhere. Device tree binding perhaps? > + if (r < 0) > + goto reg_access_fail; > + > + /* Set Y-axis resolution (depends on application) */ > + r = regmap_write(data->regmap, > + PAT9125_RES_Y_REG, > + 0x0A); > + if (r < 0) > + goto reg_access_fail; > + > + /* Enable write protection */ > + r = regmap_write(data->regmap, > + PAT9125_WRITE_PROTEC_REG, > + PAT9125_ENABLE_WRITE_PROTECT_VAL); > + if (r < 0) > + goto reg_access_fail; > + > + r = devm_iio_device_register(&client->dev, indio_dev); > + if (r) { > + dev_err(&client->dev, "IIO device register failed"); > + return r; > + } > + > + 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 */ > + r = pat9125_read_delta(data); > + if (r) > + goto reg_access_fail; > + > + /* Init GPIO IRQ */ > + if (client->irq) { > + r = devm_request_threaded_irq(&client->dev, > + client->irq, > + NULL, > + pat9125_event_handler, > + IRQF_TRIGGER_LOW | IRQF_ONESHOT, > + "pat9125", > + indio_dev); > + if (r) > + return r; > + } > + return 0; > + > +reg_access_fail: This just makes the code flow harder to follow. If you want to add the print out, just replicate it, with information for example on which register access failed. Only do goto, return paths if there is actual code to run. > + dev_err(&client->dev, "register access failed %d", r); > + return r; > +} > + > +static int pat9125_remove(struct i2c_client *client) > +{ > + 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"); > + Clean up this blank line. Thanks, Jonathan ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] iio: Add PAT9125 optical tracker sensor 2019-04-07 10:20 ` Jonathan Cameron @ 2019-04-16 12:54 ` Alexandre [not found] ` <f8ffca5b-09a8-cad7-4561-bf1263388228@baylibre.com> 1 sibling, 0 replies; 11+ messages in thread From: Alexandre @ 2019-04-16 12:54 UTC (permalink / raw) Cc: linux-kernel, linux-iio, linux-input Hello Jonathan, On 4/7/19 12:20, Jonathan Cameron wrote: > Hi Alexandre, > > So I have no problem with this as an IIO driver, but for devices that > are somewhat 'on the edge' I always like to get a clear answer to the > question: Why not input? > > I would also argue that, to actually be 'useful' we would typically need > some representation of the 'mechanicals' that are providing the motion > being measured. Looking at the datasheet this includes, rotating shafts > (side or end on), disk edges and flat surface tracking (mouse like). > > That's easy enough to do with the iio in kernel consumer interface. These > are similar to when we handle analog electronic front ends. > > I you can, please describe what it is being used for in your application > as that may give us somewhere to start! > > + CC Dmitry and linux-input. 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, like you said, this chip is able to track different kind of motion, 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 it's 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. > If 1 or 2, I would suggest that you provide absolute position to > Linux. So add the value to a software counter and provide that. > 32 bits should be plenty of resolution for that. I can't provide absolute position, only relative. Do you mean using input driver to do that ? If not, how is built the position data? > Silly question for you. What happens if you set the delta values to 0? > Do we get an interrupt which is effectively data ready? > If we do, you might want to think about a scheme where that is an option. > As things currently stand we have a confusing interface where changing this > threshold effects the buffered data output. That should only be the > case if this interface is for a trigger, not an event. I'm not sure to understand your question. Is it possible to read delta_x and delta_y = 0 in special/corner case because internal value continue to be updated after toggled motion_detect pin (used for IRQ) until values registers are read and then motion_detect pin is released: * Chip move (i.e. +2 on X axis and 0 on Y axis) * Motion_detect IRQ trigger and internal reg value is updated (i.e. delta_x = 2 and delta_y = 0) * GPIO IRQ handled but read_value isn't executed yet (timing reason) * Chip move back to it origin point (i.e. -2 on X axis and 0 on Y axis) * Motion_detect IRQ still low because it hasn't been reset by read value and internal reg value is updated (i.e. delta_x = 0 and delta_y = 0) * Read_value is executed, we get delta values = 0. > If it is actually not possible to report the two channels separately > then don't report them at all except via the buffered interface and > set the available scan masks so that both are on. I found a way to keep the consistency between delta x and delta y (without losing data). The first part is to reset a value only when user read it (also when it's buffered). The second part is to add the new value to the old value. With these two mechanism, X and Y will always be consistent: * as possible during a move. * perfectly when move is finished. Regards, Alexandre ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] iio: Add PAT9125 optical tracker sensor @ 2019-04-16 12:54 ` Alexandre 0 siblings, 0 replies; 11+ messages in thread From: Alexandre @ 2019-04-16 12:54 UTC (permalink / raw) Cc: linux-kernel, linux-iio, linux-input Hello Jonathan, On 4/7/19 12:20, Jonathan Cameron wrote: > Hi Alexandre, > > So I have no problem with this as an IIO driver, but for devices that > are somewhat 'on the edge' I always like to get a clear answer to the > question: Why not input? > > I would also argue that, to actually be 'useful' we would typically need > some representation of the 'mechanicals' that are providing the motion > being measured. Looking at the datasheet this includes, rotating shafts > (side or end on), disk edges and flat surface tracking (mouse like). > > That's easy enough to do with the iio in kernel consumer interface. These > are similar to when we handle analog electronic front ends. > > I you can, please describe what it is being used for in your application > as that may give us somewhere to start! > > + CC Dmitry and linux-input. 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, like you said, this chip is able to track different kind of motion, 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 it's 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. > If 1 or 2, I would suggest that you provide absolute position to > Linux. So add the value to a software counter and provide that. > 32 bits should be plenty of resolution for that. I can't provide absolute position, only relative. Do you mean using input driver to do that ? If not, how is built the position data? > Silly question for you. What happens if you set the delta values to 0? > Do we get an interrupt which is effectively data ready? > If we do, you might want to think about a scheme where that is an option. > As things currently stand we have a confusing interface where changing this > threshold effects the buffered data output. That should only be the > case if this interface is for a trigger, not an event. I'm not sure to understand your question. Is it possible to read delta_x and delta_y = 0 in special/corner case because internal value continue to be updated after toggled motion_detect pin (used for IRQ) until values registers are read and then motion_detect pin is released: * Chip move (i.e. +2 on X axis and 0 on Y axis) * Motion_detect IRQ trigger and internal reg value is updated (i.e. delta_x = 2 and delta_y = 0) * GPIO IRQ handled but read_value isn't executed yet (timing reason) * Chip move back to it origin point (i.e. -2 on X axis and 0 on Y axis) * Motion_detect IRQ still low because it hasn't been reset by read value and internal reg value is updated (i.e. delta_x = 0 and delta_y = 0) * Read_value is executed, we get delta values = 0. > If it is actually not possible to report the two channels separately > then don't report them at all except via the buffered interface and > set the available scan masks so that both are on. I found a way to keep the consistency between delta x and delta y (without losing data). The first part is to reset a value only when user read it (also when it's buffered). The second part is to add the new value to the old value. With these two mechanism, X and Y will always be consistent: * as possible during a move. * perfectly when move is finished. Regards, Alexandre ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <f8ffca5b-09a8-cad7-4561-bf1263388228@baylibre.com>]
* Re: [PATCH 3/3] iio: Add PAT9125 optical tracker sensor [not found] ` <f8ffca5b-09a8-cad7-4561-bf1263388228@baylibre.com> @ 2019-04-22 8:42 ` Jonathan Cameron 2019-04-23 8:57 ` Alexandre 0 siblings, 1 reply; 11+ messages in thread From: Jonathan Cameron @ 2019-04-22 8:42 UTC (permalink / raw) To: Alexandre Cc: robh+dt, mark.rutland, knaack.h, lars, pmeerw, linux-kernel, linux-iio, baylibre-upstreaming, Dmitry Torokhov, linux-input On Tue, 16 Apr 2019 14:49:19 +0200 Alexandre <amergnat@baylibre.com> wrote: > Hello Jonathan, > > On 4/7/19 12:20, Jonathan Cameron wrote: > > Hi Alexandre, > > > > So I have no problem with this as an IIO driver, but for devices that > > are somewhat 'on the edge' I always like to get a clear answer to the > > question: Why not input? > > > > I would also argue that, to actually be 'useful' we would typically need > > some representation of the 'mechanicals' that are providing the motion > > being measured. Looking at the datasheet this includes, rotating shafts > > (side or end on), disk edges and flat surface tracking (mouse like). > > > > That's easy enough to do with the iio in kernel consumer interface. These > > are similar to when we handle analog electronic front ends. > > > > I you can, please describe what it is being used for in your application > > as that may give us somewhere to start! > > > > + CC Dmitry and linux-input. > > 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, like you said, this chip is able to track different kind > of motion, 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 it's 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. > > > If 1 or 2, I would suggest that you provide absolute position to > > Linux. So add the value to a software counter and provide that. > > 32 bits should be plenty of resolution for that. > I can't provide absolute position, only relative. Do you mean using > input driver to do that ? If not, how is built the position data? Sorry, I should have been clearer on this. I mean absolute relative to the start point. So on startup you assume absolute position is 0 and go from there. What I can't work out is if the device does internal tracking, or whether each time you read it effectively resets it's internal counters to 0 so the next measurement is relative to the previous one. > > > Silly question for you. What happens if you set the delta values to 0? > > Do we get an interrupt which is effectively data ready? > > If we do, you might want to think about a scheme where that is an option. > > As things currently stand we have a confusing interface where changing this > > threshold effects the buffered data output. That should only be the > > case if this interface is for a trigger, not an event. > > I'm not sure to understand your question. Is it possible to read delta_x > and delta_y = 0 in special/corner case because internal value continue > to be updated after toggled motion_detect pin (used for IRQ) until > values registers are read and then motion_detect pin is released: > > * Chip move (i.e. +2 on X axis and 0 on Y axis) > * Motion_detect IRQ trigger and internal reg value is updated (i.e. > delta_x = 2 and delta_y = 0. > * GPIO IRQ handled but read_value isn't executed yet (timing reason) > * Chip move back to it origin point (i.e. -2 on X axis and 0 on Y axis) > * Motion_detect IRQ still low because it hasn't been reset by read > value and internal reg value is updated (i.e. delta_x = 0 and > delta_y = 0) > * Read_value is executed, we get delta values = 0. Again, I was unclear. Is it possible to set the device to interrupt every time it evaluates whether motion has occured? Not only when it concludes that there has been some motion. That would allow the interrupt to be used as a signal that the device has taken a measurement (data ready signal in other sensors). > > > If it is actually not possible to report the two channels separately > > then don't report them at all except via the buffered interface and > > set the available scan masks so that both are on. > I found a way to keep the consistency between delta x and delta y > (without losing data). The first part is to reset a value only when user > read it (also when it's buffered). The second part is to add the new > value to the old value. With these two mechanism, X and Y will always be > consistent: > > * as possible during a move. > * perfectly when move is finished. Ah. This adding old value to a new value point is what I was getting at (I think) with 'absolute' position above. In industrial control for example you have absolute position by using limit switches to set your baseline. Measurement devices are then capable of either reporting relative position, which is the movement since the last reading was taken, or 'absolute' position which is referenced to some known point. It was this form of absolute position that I was suggesting you use. If you use such a system without a limit switch it is normally called unreference motion. You can do it but then the 0 is where ever your device was at power on. For some systems it doesn't actually matter (conveyor belts for instance where the positions you care about are between things on the belt, not the position of the belt itself). Thanks, Jonathan > > > Regards, > > Alexandre > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] iio: Add PAT9125 optical tracker sensor 2019-04-22 8:42 ` Jonathan Cameron @ 2019-04-23 8:57 ` Alexandre 2019-04-24 12:10 ` Jonathan Cameron 0 siblings, 1 reply; 11+ messages in thread From: Alexandre @ 2019-04-23 8:57 UTC (permalink / raw) To: Jonathan Cameron Cc: robh+dt, mark.rutland, knaack.h, lars, pmeerw, linux-kernel, linux-iio, baylibre-upstreaming, Dmitry Torokhov, linux-input Hi Jonathan, On 4/22/19 10:42, Jonathan Cameron wrote: > On Tue, 16 Apr 2019 14:49:19 +0200 > Alexandre <amergnat@baylibre.com> wrote: > >> Hello Jonathan, >> >> On 4/7/19 12:20, Jonathan Cameron wrote: >>> Hi Alexandre, >>> >>> So I have no problem with this as an IIO driver, but for devices that >>> are somewhat 'on the edge' I always like to get a clear answer to the >>> question: Why not input? >>> >>> I would also argue that, to actually be 'useful' we would typically need >>> some representation of the 'mechanicals' that are providing the motion >>> being measured. Looking at the datasheet this includes, rotating shafts >>> (side or end on), disk edges and flat surface tracking (mouse like). >>> >>> That's easy enough to do with the iio in kernel consumer interface. These >>> are similar to when we handle analog electronic front ends. >>> >>> I you can, please describe what it is being used for in your application >>> as that may give us somewhere to start! >>> >>> + CC Dmitry and linux-input. >> 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, like you said, this chip is able to track different kind >> of motion, 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 it's 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. >> >>> If 1 or 2, I would suggest that you provide absolute position to >>> Linux. So add the value to a software counter and provide that. >>> 32 bits should be plenty of resolution for that. >> I can't provide absolute position, only relative. Do you mean using >> input driver to do that ? If not, how is built the position data? > Sorry, I should have been clearer on this. > I mean absolute relative to the start point. So on startup you assume > absolute position is 0 and go from there. What I can't work out is > if the device does internal tracking, or whether each time you read > it effectively resets it's internal counters to 0 so the next measurement > is relative to the previous one. Each time you read that reset internal counters to 0. >>> Silly question for you. What happens if you set the delta values to 0? >>> Do we get an interrupt which is effectively data ready? >>> If we do, you might want to think about a scheme where that is an option. >>> As things currently stand we have a confusing interface where changing this >>> threshold effects the buffered data output. That should only be the >>> case if this interface is for a trigger, not an event. >> I'm not sure to understand your question. Is it possible to read delta_x >> and delta_y = 0 in special/corner case because internal value continue >> to be updated after toggled motion_detect pin (used for IRQ) until >> values registers are read and then motion_detect pin is released: >> >> * Chip move (i.e. +2 on X axis and 0 on Y axis) >> * Motion_detect IRQ trigger and internal reg value is updated (i.e. >> delta_x = 2 and delta_y = 0. >> * GPIO IRQ handled but read_value isn't executed yet (timing reason) >> * Chip move back to it origin point (i.e. -2 on X axis and 0 on Y axis) >> * Motion_detect IRQ still low because it hasn't been reset by read >> value and internal reg value is updated (i.e. delta_x = 0 and >> delta_y = 0) >> * Read_value is executed, we get delta values = 0. > Again, I was unclear. Is it possible to set the device to interrupt > every time it evaluates whether motion has occured? Not only when it > concludes that there has been some motion. That would allow the interrupt > to be used as a signal that the device has taken a measurement (data > ready signal in other sensors). > I don't know, the datasheet don't describe the role of each bit in registers and I don't found documentation which provide that. I had to do research on example code to retrieve some bits, but got nothing on motion detection pin configuration. >>> If it is actually not possible to report the two channels separately >>> then don't report them at all except via the buffered interface and >>> set the available scan masks so that both are on. >> I found a way to keep the consistency between delta x and delta y >> (without losing data). The first part is to reset a value only when user >> read it (also when it's buffered). The second part is to add the new >> value to the old value. With these two mechanism, X and Y will always be >> consistent: >> >> * as possible during a move. >> * perfectly when move is finished. > Ah. This adding old value to a new value point is what I was getting > at (I think) with 'absolute' position above. > > In industrial control for example you have absolute position by using > limit switches to set your baseline. Measurement devices are then > capable of either reporting relative position, which is the movement > since the last reading was taken, or 'absolute' position which is > referenced to some known point. It was this form of absolute position > that I was suggesting you use. If you use such a system without a > limit switch it is normally called unreference motion. You can do > it but then the 0 is where ever your device was at power on. > For some systems it doesn't actually matter (conveyor belts for > instance where the positions you care about are between things > on the belt, not the position of the belt itself). Ok, I decided to return delta between last read/buffering to stay closer to the hardware mechanism and still coherent with "IIO_CHAN_INFO_RAW". If user want absolute position, he can make an addition of all received value in user space, and that allow him to reset/replace the initial position when he want it. > Thanks, > > Jonathan > >> >> Regards, >> >> Alexandre >> Thanks for your comments, Alexandre ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] iio: Add PAT9125 optical tracker sensor 2019-04-23 8:57 ` Alexandre @ 2019-04-24 12:10 ` Jonathan Cameron 0 siblings, 0 replies; 11+ messages in thread From: Jonathan Cameron @ 2019-04-24 12:10 UTC (permalink / raw) To: Alexandre Cc: Jonathan Cameron, robh+dt, mark.rutland, knaack.h, lars, pmeerw, linux-kernel, linux-iio, baylibre-upstreaming, Dmitry Torokhov, linux-input On Tue, 23 Apr 2019 10:57:40 +0200 Alexandre <amergnat@baylibre.com> wrote: Hi Alexandre, > Hi Jonathan, > > On 4/22/19 10:42, Jonathan Cameron wrote: > > On Tue, 16 Apr 2019 14:49:19 +0200 > > Alexandre <amergnat@baylibre.com> wrote: > > > >> Hello Jonathan, > >> > >> On 4/7/19 12:20, Jonathan Cameron wrote: > >>> Hi Alexandre, > >>> > >>> So I have no problem with this as an IIO driver, but for devices that > >>> are somewhat 'on the edge' I always like to get a clear answer to the > >>> question: Why not input? > >>> > >>> I would also argue that, to actually be 'useful' we would typically need > >>> some representation of the 'mechanicals' that are providing the motion > >>> being measured. Looking at the datasheet this includes, rotating shafts > >>> (side or end on), disk edges and flat surface tracking (mouse like). > >>> > >>> That's easy enough to do with the iio in kernel consumer interface. These > >>> are similar to when we handle analog electronic front ends. > >>> > >>> I you can, please describe what it is being used for in your application > >>> as that may give us somewhere to start! > >>> > >>> + CC Dmitry and linux-input. > >> 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, like you said, this chip is able to track different kind > >> of motion, 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 it's 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. > >> > >>> If 1 or 2, I would suggest that you provide absolute position to > >>> Linux. So add the value to a software counter and provide that. > >>> 32 bits should be plenty of resolution for that. > >> I can't provide absolute position, only relative. Do you mean using > >> input driver to do that ? If not, how is built the position data? > > Sorry, I should have been clearer on this. > > I mean absolute relative to the start point. So on startup you assume > > absolute position is 0 and go from there. What I can't work out is > > if the device does internal tracking, or whether each time you read > > it effectively resets it's internal counters to 0 so the next measurement > > is relative to the previous one. > Each time you read that reset internal counters to 0. > >>> Silly question for you. What happens if you set the delta values to 0? > >>> Do we get an interrupt which is effectively data ready? > >>> If we do, you might want to think about a scheme where that is an option. > >>> As things currently stand we have a confusing interface where changing this > >>> threshold effects the buffered data output. That should only be the > >>> case if this interface is for a trigger, not an event. > >> I'm not sure to understand your question. Is it possible to read delta_x > >> and delta_y = 0 in special/corner case because internal value continue > >> to be updated after toggled motion_detect pin (used for IRQ) until > >> values registers are read and then motion_detect pin is released: > >> > >> * Chip move (i.e. +2 on X axis and 0 on Y axis) > >> * Motion_detect IRQ trigger and internal reg value is updated (i.e. > >> delta_x = 2 and delta_y = 0. > >> * GPIO IRQ handled but read_value isn't executed yet (timing reason) > >> * Chip move back to it origin point (i.e. -2 on X axis and 0 on Y axis) > >> * Motion_detect IRQ still low because it hasn't been reset by read > >> value and internal reg value is updated (i.e. delta_x = 0 and > >> delta_y = 0) > >> * Read_value is executed, we get delta values = 0. > > Again, I was unclear. Is it possible to set the device to interrupt > > every time it evaluates whether motion has occured? Not only when it > > concludes that there has been some motion. That would allow the interrupt > > to be used as a signal that the device has taken a measurement (data > > ready signal in other sensors). > > > I don't know, the datasheet don't describe the role of each bit in > registers and I don't found documentation which provide that. I had to > do research on example code to retrieve some bits, but got nothing on > motion detection pin configuration. That kind of rules out reporting this as a speed as we can't guarantee when the last read occurred. Oh well, was a bit optimistic! > > >>> If it is actually not possible to report the two channels separately > >>> then don't report them at all except via the buffered interface and > >>> set the available scan masks so that both are on. > >> I found a way to keep the consistency between delta x and delta y > >> (without losing data). The first part is to reset a value only when user > >> read it (also when it's buffered). The second part is to add the new > >> value to the old value. With these two mechanism, X and Y will always be > >> consistent: > >> > >> * as possible during a move. > >> * perfectly when move is finished. > > Ah. This adding old value to a new value point is what I was getting > > at (I think) with 'absolute' position above. > > > > In industrial control for example you have absolute position by using > > limit switches to set your baseline. Measurement devices are then > > capable of either reporting relative position, which is the movement > > since the last reading was taken, or 'absolute' position which is > > referenced to some known point. It was this form of absolute position > > that I was suggesting you use. If you use such a system without a > > limit switch it is normally called unreference motion. You can do > > it but then the 0 is where ever your device was at power on. > > For some systems it doesn't actually matter (conveyor belts for > > instance where the positions you care about are between things > > on the belt, not the position of the belt itself). > > Ok, I decided to return delta between last read/buffering to stay closer > to the hardware mechanism and still coherent with "IIO_CHAN_INFO_RAW". > If user want absolute position, he can make an addition of all received > value in user space, and that allow him to reset/replace the initial > position when he want it. Hmm. Unfortunately this doesn't really map to the expectations of userspace in IIO. This particular option is neither position nor speed, but rather a delta position. For that we don't really have a current representation. There is also not guarantee that you don't have multiple concurrent readers. If that happens you will get a delta against something unknown. I think you need to do the addition in kernel to be able to provide userspace with something consistent. Jonathan > > > Thanks, > > > > Jonathan > > > >> > >> Regards, > >> > >> Alexandre > >> > > Thanks for your comments, > > Alexandre > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] iio: Add PAT9125 optical tracker sensor @ 2019-04-24 12:10 ` Jonathan Cameron 0 siblings, 0 replies; 11+ messages in thread From: Jonathan Cameron @ 2019-04-24 12:10 UTC (permalink / raw) To: Alexandre Cc: Jonathan Cameron, robh+dt, mark.rutland, knaack.h, lars, pmeerw, linux-kernel, linux-iio, baylibre-upstreaming, Dmitry Torokhov, linux-input On Tue, 23 Apr 2019 10:57:40 +0200 Alexandre <amergnat@baylibre.com> wrote: Hi Alexandre, > Hi Jonathan, > > On 4/22/19 10:42, Jonathan Cameron wrote: > > On Tue, 16 Apr 2019 14:49:19 +0200 > > Alexandre <amergnat@baylibre.com> wrote: > > > >> Hello Jonathan, > >> > >> On 4/7/19 12:20, Jonathan Cameron wrote: > >>> Hi Alexandre, > >>> > >>> So I have no problem with this as an IIO driver, but for devices that > >>> are somewhat 'on the edge' I always like to get a clear answer to the > >>> question: Why not input? > >>> > >>> I would also argue that, to actually be 'useful' we would typically need > >>> some representation of the 'mechanicals' that are providing the motion > >>> being measured. Looking at the datasheet this includes, rotating shafts > >>> (side or end on), disk edges and flat surface tracking (mouse like). > >>> > >>> That's easy enough to do with the iio in kernel consumer interface. These > >>> are similar to when we handle analog electronic front ends. > >>> > >>> I you can, please describe what it is being used for in your application > >>> as that may give us somewhere to start! > >>> > >>> + CC Dmitry and linux-input. > >> 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, like you said, this chip is able to track different kind > >> of motion, 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 it's 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. > >> > >>> If 1 or 2, I would suggest that you provide absolute position to > >>> Linux. So add the value to a software counter and provide that. > >>> 32 bits should be plenty of resolution for that. > >> I can't provide absolute position, only relative. Do you mean using > >> input driver to do that ? If not, how is built the position data? > > Sorry, I should have been clearer on this. > > I mean absolute relative to the start point. So on startup you assume > > absolute position is 0 and go from there. What I can't work out is > > if the device does internal tracking, or whether each time you read > > it effectively resets it's internal counters to 0 so the next measurement > > is relative to the previous one. > Each time you read that reset internal counters to 0. > >>> Silly question for you. What happens if you set the delta values to 0? > >>> Do we get an interrupt which is effectively data ready? > >>> If we do, you might want to think about a scheme where that is an option. > >>> As things currently stand we have a confusing interface where changing this > >>> threshold effects the buffered data output. That should only be the > >>> case if this interface is for a trigger, not an event. > >> I'm not sure to understand your question. Is it possible to read delta_x > >> and delta_y = 0 in special/corner case because internal value continue > >> to be updated after toggled motion_detect pin (used for IRQ) until > >> values registers are read and then motion_detect pin is released: > >> > >> * Chip move (i.e. +2 on X axis and 0 on Y axis) > >> * Motion_detect IRQ trigger and internal reg value is updated (i.e. > >> delta_x = 2 and delta_y = 0. > >> * GPIO IRQ handled but read_value isn't executed yet (timing reason) > >> * Chip move back to it origin point (i.e. -2 on X axis and 0 on Y axis) > >> * Motion_detect IRQ still low because it hasn't been reset by read > >> value and internal reg value is updated (i.e. delta_x = 0 and > >> delta_y = 0) > >> * Read_value is executed, we get delta values = 0. > > Again, I was unclear. Is it possible to set the device to interrupt > > every time it evaluates whether motion has occured? Not only when it > > concludes that there has been some motion. That would allow the interrupt > > to be used as a signal that the device has taken a measurement (data > > ready signal in other sensors). > > > I don't know, the datasheet don't describe the role of each bit in > registers and I don't found documentation which provide that. I had to > do research on example code to retrieve some bits, but got nothing on > motion detection pin configuration. That kind of rules out reporting this as a speed as we can't guarantee when the last read occurred. Oh well, was a bit optimistic! > > >>> If it is actually not possible to report the two channels separately > >>> then don't report them at all except via the buffered interface and > >>> set the available scan masks so that both are on. > >> I found a way to keep the consistency between delta x and delta y > >> (without losing data). The first part is to reset a value only when user > >> read it (also when it's buffered). The second part is to add the new > >> value to the old value. With these two mechanism, X and Y will always be > >> consistent: > >> > >> * as possible during a move. > >> * perfectly when move is finished. > > Ah. This adding old value to a new value point is what I was getting > > at (I think) with 'absolute' position above. > > > > In industrial control for example you have absolute position by using > > limit switches to set your baseline. Measurement devices are then > > capable of either reporting relative position, which is the movement > > since the last reading was taken, or 'absolute' position which is > > referenced to some known point. It was this form of absolute position > > that I was suggesting you use. If you use such a system without a > > limit switch it is normally called unreference motion. You can do > > it but then the 0 is where ever your device was at power on. > > For some systems it doesn't actually matter (conveyor belts for > > instance where the positions you care about are between things > > on the belt, not the position of the belt itself). > > Ok, I decided to return delta between last read/buffering to stay closer > to the hardware mechanism and still coherent with "IIO_CHAN_INFO_RAW". > If user want absolute position, he can make an addition of all received > value in user space, and that allow him to reset/replace the initial > position when he want it. Hmm. Unfortunately this doesn't really map to the expectations of userspace in IIO. This particular option is neither position nor speed, but rather a delta position. For that we don't really have a current representation. There is also not guarantee that you don't have multiple concurrent readers. If that happens you will get a delta against something unknown. I think you need to do the addition in kernel to be able to provide userspace with something consistent. Jonathan > > > Thanks, > > > > Jonathan > > > >> > >> Regards, > >> > >> Alexandre > >> > > Thanks for your comments, > > Alexandre > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-04-24 12:10 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-05 9:34 [PATCH 0/3] Add PAT6125 optical tracker driver Alexandre Mergnat
2019-04-05 9:34 ` [PATCH 1/3] dt-bindings: Add pixart vendor Alexandre Mergnat
2019-04-05 9:34 ` [PATCH 2/3] dt-bindings: iio: ot: Add docs pat9125 Alexandre Mergnat
2019-04-05 9:34 ` [PATCH 3/3] iio: Add PAT9125 optical tracker sensor Alexandre Mergnat
2019-04-07 10:20 ` Jonathan Cameron
2019-04-16 12:54 ` Alexandre
2019-04-16 12:54 ` Alexandre
[not found] ` <f8ffca5b-09a8-cad7-4561-bf1263388228@baylibre.com>
2019-04-22 8:42 ` Jonathan Cameron
2019-04-23 8:57 ` Alexandre
2019-04-24 12:10 ` Jonathan Cameron
2019-04-24 12:10 ` Jonathan Cameron
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.