| Message ID | 20190610092945.6330-4-amergnat@baylibre.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add PAT9125 optical tracker driver | expand |
On Mon, 10 Jun 2019 11:29:45 +0200 Alexandre Mergnat <amergnat@baylibre.com> wrote: > This adds support for PixArt Imaging’s miniature low power optical > navigation chip using LASER light source enabling digital surface tracking. > > Feature and datasheet: [0] > > This IIO driver allows to read delta or relative position on X and Y axis: > - The position relative to where the system started can be taken through > punctual "read_raw" which will issue a read in the device registers to > get the delta between last/current read and return the addition of all > the deltas. > - The delta can be retrieved using triggered buffer subscription > (i.e. iio_readdev). The buffer payload is: > |32 bits delta X|32 bits delta Y|timestamp|. > > The possible I2C addresses are 0x73, 0x75 and 0x79. > > X and Y axis CPI resolution can be get/set independently through IIO_SCALE. > The range value is 0-255 which means: > - 0 to ~1,275 Counts Per Inch on flat surface. > - 0 to ~630 Counts Per Rev on 1.0mm diameter STS shaft at 1.0mm distance. > More details on the datasheet. > > The "position" directory is added to contain drivers which can provide > position data. > > Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> > > [0]: http://www.pixart.com/products-detail/72/PAT9125EL-TKIT___TKMT > > Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> Hi Alexandre, Getting close but a few more bits and pieces inline. I'm a little confused on why the buffered reads are giving the delta values whilst we have fixed up the sysfs reads to give the more useful position values (and hence not need to know when last read was). We should be consistent and give positions from the buffered path as well. Thanks, Jonathan > --- > drivers/iio/Kconfig | 1 + > drivers/iio/Makefile | 1 + > drivers/iio/position/Kconfig | 18 ++ > drivers/iio/position/Makefile | 6 + > drivers/iio/position/pat9125.c | 499 +++++++++++++++++++++++++++++++++ > 5 files changed, 525 insertions(+) > create mode 100644 drivers/iio/position/Kconfig > create mode 100644 drivers/iio/position/Makefile > create mode 100644 drivers/iio/position/pat9125.c > > diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig > index 1d736a4952ab..23d9780640e7 100644 > --- a/drivers/iio/Kconfig > +++ b/drivers/iio/Kconfig > @@ -85,6 +85,7 @@ source "drivers/iio/light/Kconfig" > source "drivers/iio/magnetometer/Kconfig" > source "drivers/iio/multiplexer/Kconfig" > source "drivers/iio/orientation/Kconfig" > +source "drivers/iio/position/Kconfig" > if IIO_TRIGGER > source "drivers/iio/trigger/Kconfig" > endif #IIO_TRIGGER > diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile > index bff682ad1cfb..1712011c0f4a 100644 > --- a/drivers/iio/Makefile > +++ b/drivers/iio/Makefile > @@ -31,6 +31,7 @@ obj-y += light/ > obj-y += magnetometer/ > obj-y += multiplexer/ > obj-y += orientation/ > +obj-y += position/ > obj-y += potentiometer/ > obj-y += potentiostat/ > obj-y += pressure/ > diff --git a/drivers/iio/position/Kconfig b/drivers/iio/position/Kconfig > new file mode 100644 > index 000000000000..1cf28896511c > --- /dev/null > +++ b/drivers/iio/position/Kconfig > @@ -0,0 +1,18 @@ > +# > +# Optical tracker sensors > +# > +# When adding new entries keep the list in alphabetical order > + > +menu "Optical tracker sensors" > + > +config PAT9125 > + tristate "Optical tracker PAT9125 I2C driver" > + depends on I2C > + select IIO_BUFFER > + help > + Say yes here to build support for PAT9125 optical tracker > + sensors. > + > + To compile this driver as a module, say M here: the module will > + be called pat9125. > +endmenu > diff --git a/drivers/iio/position/Makefile b/drivers/iio/position/Makefile > new file mode 100644 > index 000000000000..cf294917ae2c > --- /dev/null > +++ b/drivers/iio/position/Makefile > @@ -0,0 +1,6 @@ > +# > +# Makefile for industrial I/O Optical tracker sensor drivers > +# > + > +# When adding new entries keep the list in alphabetical order > +obj-$(CONFIG_PAT9125) += pat9125.o > diff --git a/drivers/iio/position/pat9125.c b/drivers/iio/position/pat9125.c > new file mode 100644 > index 000000000000..22bf729bec9b > --- /dev/null > +++ b/drivers/iio/position/pat9125.c > @@ -0,0 +1,499 @@ > +// SPDX-License-Identifier: (GPL-2.0) > +/* > + * Copyright (C) 2019 BayLibre, SAS > + * Author: Alexandre Mergnat <amergnat@baylibre.com> > + */ > + > +#include <linux/bitops.h> > +#include <linux/delay.h> > +#include <linux/interrupt.h> > +#include <linux/i2c.h> > +#include <linux/iio/iio.h> > +#include <linux/iio/sysfs.h> > +#include <linux/iio/events.h> > +#include <linux/iio/buffer.h> > +#include <linux/iio/trigger.h> > +#include <linux/iio/trigger_consumer.h> > +#include <linux/iio/triggered_buffer.h> > +#include <linux/iio/kfifo_buf.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/regmap.h> > +#include <linux/slab.h> > + > +/* I2C Address function to ID pin*/ > +#define PAT9125_I2C_ADDR_HI 0x73 > +#define PAT9125_I2C_ADDR_LO 0x75 > +#define PAT9125_I2C_ADDR_NC 0x79 > + > +/* Registers */ > +#define PAT9125_PRD_ID1_REG 0x00 > +#define PAT9125_PRD_ID2_REG 0x01 > +#define PAT9125_MOTION_STATUS_REG 0x02 > +#define PAT9125_DELTA_X_LO_REG 0x03 > +#define PAT9125_DELTA_Y_LO_REG 0x04 > +#define PAT9125_OP_MODE_REG 0x05 > +#define PAT9125_CONFIG_REG 0x06 > +#define PAT9125_WRITE_PROTEC_REG 0x09 > +#define PAT9125_SLEEP1_REG 0x0A > +#define PAT9125_SLEEP2_REG 0x0B > +#define PAT9125_RES_X_REG 0x0D > +#define PAT9125_RES_Y_REG 0x0E > +#define PAT9125_DELTA_XY_HI_REG 0x12 > +#define PAT9125_SHUTER_REG 0x14 > +#define PAT9125_FRAME_AVG_REG 0x17 > +#define PAT9125_ORIENTATION_REG 0x19 > + > +/* Bits */ > +#define PAT9125_VALID_MOTION_DATA_BIT BIT(7) > +#define PAT9125_RESET_BIT BIT(7) > + > +/* Registers' values */ > +#define PAT9125_SENSOR_ID_VAL 0x31 > +#define PAT9125_DISABLE_WRITE_PROTECT_VAL 0x5A > +#define PAT9125_ENABLE_WRITE_PROTECT_VAL 0x00 > + > +/* Default Value of sampled value size */ > +#define PAT9125_SAMPLED_VAL_BIT_SIZE 12 > + > +struct pat9125_data { > + struct regmap *regmap; > + struct iio_trigger *indio_trig; /* Motion detection */ > + s32 delta_x; > + s32 delta_y; > + s32 position_x; > + s32 position_y; > + bool sampling; > +}; > + > +static const struct iio_chan_spec pat9125_channels[] = { > + { > + .type = IIO_DISTANCE, > + .modified = 1, > + .channel2 = IIO_MOD_X, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > + BIT(IIO_CHAN_INFO_SCALE), > + .scan_index = 0, > + .scan_type = { > + .sign = 's', > + .realbits = 32, > + .storagebits = 32, > + .endianness = IIO_CPU, > + }, > + }, > + { > + .type = IIO_DISTANCE, > + .modified = 1, > + .channel2 = IIO_MOD_Y, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > + BIT(IIO_CHAN_INFO_SCALE), > + .scan_index = 1, > + .scan_type = { > + .sign = 's', > + .realbits = 32, > + .storagebits = 32, > + .endianness = IIO_CPU, > + }, > + }, > + IIO_CHAN_SOFT_TIMESTAMP(2), > +}; > + > +/** > + * pat9125_write_pretected_reg() - Write value in protected register. > + * > + * @regmap: Pointer to I2C register map. > + * @reg_addr: Register address. > + * @reg_value: Value to be write in register. > + * > + * A value of zero will be returned on success, a negative errno will > + * be returned in error cases. > + */ > +static int pat9125_write_pretected_reg(struct iio_dev *indio_dev, > + u8 reg_addr, u8 reg_value) > +{ > + struct pat9125_data *data = iio_priv(indio_dev); > + int ret; > + > + ret = regmap_write(data->regmap, > + PAT9125_WRITE_PROTEC_REG, > + PAT9125_DISABLE_WRITE_PROTECT_VAL); > + if (ret < 0) { > + dev_err(indio_dev->dev.parent, "register 0x%x access failed %d\n", > + PAT9125_WRITE_PROTEC_REG, ret); > + return ret; > + } > + > + ret = regmap_write(data->regmap, reg_addr, reg_value); > + if (ret < 0) { > + dev_err(indio_dev->dev.parent, "register 0x%x access failed %d\n", > + reg_addr, ret); > + return ret; > + } > + > + ret = regmap_write(data->regmap, > + PAT9125_WRITE_PROTEC_REG, > + PAT9125_ENABLE_WRITE_PROTECT_VAL); > + if (ret < 0) { > + dev_err(indio_dev->dev.parent, "register 0x%x access failed %d\n", > + PAT9125_WRITE_PROTEC_REG, ret); > + return ret; We'll probably get a patch from some script for this so nice to cleanup now. regmap_write always returns 0 for success and negative otherwise. Hence we can just check if (ret). Having done that we can also drop the return ret above out of the brackets and save a few lines of code by getting rid of the return 0 that follows. Just checkign if (ret) in general is probably a worthwhile cleanup for these regmap calls. > + } > + return 0; > +} > +/** > + * pat9125_read_delta() - Read delta value, update delta & position data. > + * > + * @data: Driver's data structure. > + * > + * A value of zero will be returned on success, a negative errno will > + * be returned in error cases. > + */ > +static int pat9125_read_delta(struct pat9125_data *data) > +{ > + struct regmap *regmap = data->regmap; > + int status = 0; > + int val_x = 0; > + int val_y = 0; > + int val_high_nibbles = 0; > + int ret; > + > + ret = regmap_read(regmap, PAT9125_MOTION_STATUS_REG, &status); > + if (ret < 0) > + return ret; > + > + /* Check if motion is detected */ > + if (status & PAT9125_VALID_MOTION_DATA_BIT) { > + ret = regmap_read(regmap, PAT9125_DELTA_X_LO_REG, &val_x); > + if (ret < 0) > + return ret; > + > + ret = regmap_read(regmap, PAT9125_DELTA_Y_LO_REG, &val_y); > + if (ret < 0) > + return ret; > + > + ret = regmap_read(regmap, PAT9125_DELTA_XY_HI_REG, > + &val_high_nibbles); > + if (ret < 0) > + return ret; > + > + val_x |= (val_high_nibbles << 4) & 0xF00; > + val_y |= (val_high_nibbles << 8) & 0xF00; > + val_x = sign_extend32(val_x, > + PAT9125_SAMPLED_VAL_BIT_SIZE - 1); > + val_y = sign_extend32(val_y, > + PAT9125_SAMPLED_VAL_BIT_SIZE - 1); > + data->position_x += val_x; > + data->position_y += val_y; > + data->delta_x = val_x; > + data->delta_y = val_y; > + } > + return 0; > +} > + > +/** > + * pat9125_read_raw() - Sample and return the value(s) > + * function to the associated channel info enum. > + **/ > +static int pat9125_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, int *val2, long mask) > +{ > + struct pat9125_data *data = iio_priv(indio_dev); > + int ret; > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + ret = pat9125_read_delta(data); > + if (ret) > + return ret; > + switch (chan->channel2) { > + case IIO_MOD_X: > + *val = data->position_x; > + return IIO_VAL_INT; > + case IIO_MOD_Y: > + *val = data->position_y; > + return IIO_VAL_INT; > + default: > + return -EINVAL; > + } > + case IIO_CHAN_INFO_SCALE: > + switch (chan->channel2) { > + case IIO_MOD_X: > + ret = regmap_read(data->regmap, PAT9125_RES_X_REG, val); > + if (ret) > + return ret; > + else > + return IIO_VAL_INT; > + case IIO_MOD_Y: > + ret = regmap_read(data->regmap, PAT9125_RES_Y_REG, val); > + if (ret) > + return ret; > + else > + return IIO_VAL_INT; > + default: > + return -EINVAL; > + } > + default: > + return -EINVAL; > + } > +} > + > +/** > + * pat9125_write_raw() - Write the value(s) > + * function to the associated channel info enum. > + **/ */ Tidy these up throughout. If you are going to do kernel-doc (which is good) you should also document all of the parameters. > +static int pat9125_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, int val, > + int val2, long mask) > +{ > + int ret; > + > + switch (mask) { > + case IIO_CHAN_INFO_SCALE: > + switch (chan->channel2) { > + case IIO_MOD_X: > + ret = pat9125_write_pretected_reg(indio_dev, > + PAT9125_RES_X_REG, val); > + return ret; > + case IIO_MOD_Y: > + ret = pat9125_write_pretected_reg(indio_dev, > + PAT9125_RES_Y_REG, val); > + return ret; > + default: > + return -EINVAL; > + } > + default: > + return -EINVAL; > + } > +} > + > +static irqreturn_t pat9125_threaded_trigger_handler(int irq, void *p) > +{ > + struct iio_poll_func *pf = p; > + struct iio_dev *indio_dev = pf->indio_dev; > + struct pat9125_data *data = iio_priv(indio_dev); > + u8 buf[16]; /* Payload: Delta_X (4) | Delta_Y (4) | Timestamp (8) */ > + int ret; > + s64 timestamp; > + > + data->sampling = true; > + ret = pat9125_read_delta(data); > + if (ret) { > + dev_err(indio_dev->dev.parent, "Read delta failed %d\n", ret); > + return IRQ_NONE; > + } > + timestamp = iio_get_time_ns(indio_dev); > + *((s32 *)&buf[0]) = data->delta_x; > + *((s32 *)&buf[sizeof(s32)]) = data->delta_y; Why are we putting the delta values in the buffer rather than the positions? > + data->delta_x = 0; > + data->delta_y = 0; > + iio_push_to_buffers_with_timestamp(indio_dev, buf, timestamp); > + iio_trigger_notify_done(indio_dev->trig); > + return IRQ_HANDLED; > +} > + > +/** > + * pat9125_threaded_event_handler() - Threaded motion detection event handler > + * @irq: The irq being handled. > + * @private: struct iio_device pointer for the device. > + */ > +static irqreturn_t pat9125_threaded_event_handler(int irq, void *private) > +{ > + struct iio_dev *indio_dev = private; > + struct pat9125_data *data = iio_priv(indio_dev); > + > + iio_trigger_poll_chained(data->indio_trig); > + return IRQ_HANDLED; > +} > + > +static int pat9125_buffer_postenable(struct iio_dev *indio_dev) > +{ > + struct pat9125_data *data = iio_priv(indio_dev); > + int ret = 0; > + > + ret = iio_triggered_buffer_postenable(indio_dev); > + if (ret) > + return ret; > + /* Release interrupt pin on the device */ > + return pat9125_read_delta(data); If this has an error, we should be handling the unwind of iio_triggered_buffer_postenable. That is iio_triggered_buffer_predisable. (Alexandru has been cleaning these up recently so I'm more aware of this than normal!) > +} > + > +static const struct iio_buffer_setup_ops pat9125_buffer_ops = { > + .postenable = pat9125_buffer_postenable, > + .predisable = iio_triggered_buffer_predisable, > +}; > + One blank line is almost always enough (nitpick of the day ;) > + > +static const struct regmap_config pat9125_regmap_config = { > + .reg_bits = 8, > + .val_bits = 8, > +}; > + > +static const struct iio_info pat9125_info = { > + .read_raw = pat9125_read_raw, > + .write_raw = pat9125_write_raw, > +}; > + > +/* > + * To detect if a new value is available, register status is checked. This > + * method is safer than using a flag on GPIO IRQ to track event while sampling > + * because falling edge is missed when device trig just after a read reg value > + * (that happen for fast motions or high CPI setting). So we have an edge triggered interrupt that doesn't have a 'minimum low' period? If so then the only safe way to handle it would be as a level interrupt. Can you do that here? (I once had the delights of a sensor like this tied to a edge sensitive only interrupt, but thankfully those are a rare thing these days). > + * > + * Note: To avoid infinite loop in "iio_trigger_notify_done" when it si not in is > + * buffer mode and kernel warning due to nested IRQ thread, > + * this function must return 0. > + */ > +static int pat9125_trig_try_reenable(struct iio_trigger *trig) > +{ > + struct pat9125_data *data = iio_trigger_get_drvdata(trig); > + struct regmap *regmap = data->regmap; > + int status = 0; > + > + if (data->sampling) { > + regmap_read(regmap, PAT9125_MOTION_STATUS_REG, &status); > + if (status & PAT9125_VALID_MOTION_DATA_BIT) { > + data->sampling = false; So we only ever do 2 reads? Why can't we be unlucky on timing twice in a row? > + iio_trigger_poll_chained(data->indio_trig); > + return 0; > + } > + } > + data->sampling = false; > + return 0; > +} > + > +static const struct iio_trigger_ops pat9125_trigger_ops = { > + .try_reenable = pat9125_trig_try_reenable, > +}; > + > +static int pat9125_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct pat9125_data *data; > + struct iio_dev *indio_dev; > + int ret, sensor_pid; > + > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); > + if (!indio_dev) { > + dev_err(&client->dev, "IIO device allocation failed\n"); > + return -ENOMEM; > + } > + > + data = iio_priv(indio_dev); > + indio_dev->dev.parent = &client->dev; > + indio_dev->name = id->name; > + indio_dev->channels = pat9125_channels; > + indio_dev->num_channels = ARRAY_SIZE(pat9125_channels); > + indio_dev->info = &pat9125_info; > + indio_dev->modes = INDIO_DIRECT_MODE; > + > + ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL, > + pat9125_threaded_trigger_handler, &pat9125_buffer_ops); > + if (ret) { > + dev_err(&client->dev, "unable to setup triggered buffer\n"); > + return ret; > + } > + > + data->indio_trig = devm_iio_trigger_alloc(&client->dev, "%s-dev%d", > + indio_dev->name, indio_dev->id); > + if (!data->indio_trig) > + return -ENOMEM; > + data->indio_trig->dev.parent = &client->dev; > + data->indio_trig->ops = &pat9125_trigger_ops; > + iio_trigger_set_drvdata(data->indio_trig, data); > + ret = devm_iio_trigger_register(&client->dev, data->indio_trig); > + if (ret) { > + dev_err(&client->dev, "unable to register trigger\n"); > + return ret; > + } > + > + data->regmap = devm_regmap_init_i2c(client, &pat9125_regmap_config); > + if (IS_ERR(data->regmap)) { > + dev_err(&client->dev, "regmap init failed %ld\n", > + PTR_ERR(data->regmap)); > + return PTR_ERR(data->regmap); > + } > + > + /* Check device ID */ > + ret = regmap_read(data->regmap, PAT9125_PRD_ID1_REG, &sensor_pid); > + if (ret < 0) { > + dev_err(&client->dev, "register 0x%x access failed %d\n", > + PAT9125_PRD_ID1_REG, ret); > + return ret; > + } > + if (sensor_pid != PAT9125_SENSOR_ID_VAL) > + return -ENODEV; > + > + /* Switch to bank0 (Magic number)*/ > + ret = regmap_write(data->regmap, 0x7F, 0x00); > + if (ret < 0) { > + dev_err(indio_dev->dev.parent, "register 0x%x access failed %d\n", > + 0x7F, ret); > + return ret; > + } > + > + /* Software reset */ > + ret = regmap_write_bits(data->regmap, > + PAT9125_CONFIG_REG, > + PAT9125_RESET_BIT, > + 1); > + if (ret < 0) { > + dev_err(&client->dev, "register 0x%x access failed %d\n", > + PAT9125_CONFIG_REG, ret); > + return ret; > + } > + > + msleep(20); > + > + /* Init GPIO IRQ */ > + if (client->irq) { > + ret = devm_request_threaded_irq(&client->dev, > + client->irq, > + NULL, > + pat9125_threaded_event_handler, > + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > + "pat9125", > + indio_dev); > + if (ret) { > + dev_err(&client->dev, "GPIO IRQ init failed\n"); > + return ret; > + } > + } > + > + ret = devm_iio_device_register(&client->dev, indio_dev); > + if (ret) { > + dev_err(&client->dev, "IIO device register failed\n"); > + return ret; > + } > + > + i2c_set_clientdata(client, indio_dev); Why? You have now gotten rid of remove (which is great). I'm guessing that was using i2c_get_clientdata? > + return 0; > +} > + > +static const struct i2c_device_id pat9125_id[] = { > + { "pat9125", 0 }, > + {} > +}; > +MODULE_DEVICE_TABLE(i2c, pat9125_id); > + > +static const unsigned short normal_i2c[] = { > + PAT9125_I2C_ADDR_HI, > + PAT9125_I2C_ADDR_LO, > + PAT9125_I2C_ADDR_NC, > + I2C_CLIENT_END > +}; > + > +static struct i2c_driver pat9125_driver = { > + .driver = { > + .name = "pat9125", > + }, > + .probe = pat9125_probe, > + .address_list = normal_i2c, > + .id_table = pat9125_id, > +}; > + > +module_i2c_driver(pat9125_driver); > + > +MODULE_AUTHOR("Alexandre Mergnat <amergnat@baylibre.com>"); > +MODULE_DESCRIPTION("Optical Tracking sensor"); > +MODULE_LICENSE("GPL");
Le dim. 16 juin 2019 à 17:39, Jonathan Cameron <jic23@kernel.org> a écrit : > > On Mon, 10 Jun 2019 11:29:45 +0200 > Alexandre Mergnat <amergnat@baylibre.com> wrote: ... > > > +/* > > + * To detect if a new value is available, register status is checked. This > > + * method is safer than using a flag on GPIO IRQ to track event while sampling > > + * because falling edge is missed when device trig just after a read reg value > > + * (that happen for fast motions or high CPI setting). > > So we have an edge triggered interrupt that doesn't have a 'minimum low' > period? If so then the only safe way to handle it would be as a level > interrupt. Can you do that here? > (I once had the delights of a sensor like this tied to a edge sensitive only > interrupt, but thankfully those are a rare thing these days). > Trigger level is the first setup I tried (and retried during modifications) but it cannot works despite of ONESHOT flag. I'm wrong or it's probably due to nested_irq because it works when I reset interrupt (by reading data) during one of the IRQ thread, that what I did in my V1 patch. I spent a lot of time to try to use level trigger but this is the best way I found to do it properly without corner cases. The result with nested IRQ and low level trigger is a spamming IRQ (probably due to IRQ no more masked during nested IRQ thread) who that stuck the board because it hasn't time to make an I2C read to reset interrupt pin. > > + * buffer mode and kernel warning due to nested IRQ thread, > > + * this function must return 0. > > + */ > > +static int pat9125_trig_try_reenable(struct iio_trigger *trig) > > +{ > > + struct pat9125_data *data = iio_trigger_get_drvdata(trig); > > + struct regmap *regmap = data->regmap; > > + int status = 0; > > + > > + if (data->sampling) { > > + regmap_read(regmap, PAT9125_MOTION_STATUS_REG, &status); > > + if (status & PAT9125_VALID_MOTION_DATA_BIT) { > > + data->sampling = false; > So we only ever do 2 reads? Why can't we be unlucky on timing > twice in a row? That can works indefinitely, I tested for some retry in a row by moving the chip fastly. If the method blocked at 2 readings, I should have been stuck during this test. If read status return "New data available", a new read value is done through the same process (that mean data->sampling put to true) thanks to nested IRQ thread which will call try_reenable again and then re-check pat9125 status.
On Thu, 11 Jul 2019 21:39:10 +0200 Alexandre Mergnat <amergnat@baylibre.com> wrote: > Le dim. 16 juin 2019 à 17:39, Jonathan Cameron <jic23@kernel.org> a écrit : > > > > On Mon, 10 Jun 2019 11:29:45 +0200 > > Alexandre Mergnat <amergnat@baylibre.com> wrote: > ... > > > > > +/* > > > + * To detect if a new value is available, register status is checked. This > > > + * method is safer than using a flag on GPIO IRQ to track event while sampling > > > + * because falling edge is missed when device trig just after a read reg value > > > + * (that happen for fast motions or high CPI setting). > > > > So we have an edge triggered interrupt that doesn't have a 'minimum low' > > period? If so then the only safe way to handle it would be as a level > > interrupt. Can you do that here? > > (I once had the delights of a sensor like this tied to a edge sensitive only > > interrupt, but thankfully those are a rare thing these days). > > > > Trigger level is the first setup I tried (and retried during > modifications) but it cannot > works despite of ONESHOT flag. I'm wrong or it's probably due to > nested_irq because > it works when I reset interrupt (by reading data) during one of the > IRQ thread, that what > I did in my V1 patch. I spent a lot of time to try to use level > trigger but this is the > best way I found to do it properly without corner cases. > The result with nested IRQ and low level trigger is a spamming IRQ > (probably due to IRQ no more masked during nested IRQ thread) who that stuck the > board because it hasn't time to make an I2C read to reset interrupt pin. I'm not totally following the explanation. The level interrupt should remain masked until the irq_thread completes, as long as you have the irq as IRQF_ONESHOT. The nesting shouldn't matter for this as the interrupt should only be unmasked once we the poll_func handler has completed. If the device is sampling fast enough, you might find that it's taken another sample so you get an immediate new interrupt. If that's the case, then only option is to reducing the sampling rate. Of course, it's always possible we have a bug, but there are quite a few drivers doing things with level interrupts with no problem, so I'm a little doubtful. > > > > > + * buffer mode and kernel warning due to nested IRQ thread, > > > + * this function must return 0. > > > + */ > > > +static int pat9125_trig_try_reenable(struct iio_trigger *trig) > > > +{ > > > + struct pat9125_data *data = iio_trigger_get_drvdata(trig); > > > + struct regmap *regmap = data->regmap; > > > + int status = 0; > > > + > > > + if (data->sampling) { > > > + regmap_read(regmap, PAT9125_MOTION_STATUS_REG, &status); > > > + if (status & PAT9125_VALID_MOTION_DATA_BIT) { > > > + data->sampling = false; > > So we only ever do 2 reads? Why can't we be unlucky on timing > > twice in a row? > > That can works indefinitely, I tested for some retry in a row by > moving the chip fastly. > If the method blocked at 2 readings, I should have been stuck during this test. > > If read status return "New data available", a new read value is done > through the same > process (that mean data->sampling put to true) thanks to nested IRQ > thread which will > call try_reenable again and then re-check pat9125 status. Hmm. I'm not totally happy with a test that is based on failing to trigger it. We need a theoretical argument for why it can't happen to back up this code. Jonathan
diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig index 1d736a4952ab..23d9780640e7 100644 --- a/drivers/iio/Kconfig +++ b/drivers/iio/Kconfig @@ -85,6 +85,7 @@ source "drivers/iio/light/Kconfig" source "drivers/iio/magnetometer/Kconfig" source "drivers/iio/multiplexer/Kconfig" source "drivers/iio/orientation/Kconfig" +source "drivers/iio/position/Kconfig" if IIO_TRIGGER source "drivers/iio/trigger/Kconfig" endif #IIO_TRIGGER diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile index bff682ad1cfb..1712011c0f4a 100644 --- a/drivers/iio/Makefile +++ b/drivers/iio/Makefile @@ -31,6 +31,7 @@ obj-y += light/ obj-y += magnetometer/ obj-y += multiplexer/ obj-y += orientation/ +obj-y += position/ obj-y += potentiometer/ obj-y += potentiostat/ obj-y += pressure/ diff --git a/drivers/iio/position/Kconfig b/drivers/iio/position/Kconfig new file mode 100644 index 000000000000..1cf28896511c --- /dev/null +++ b/drivers/iio/position/Kconfig @@ -0,0 +1,18 @@ +# +# Optical tracker sensors +# +# When adding new entries keep the list in alphabetical order + +menu "Optical tracker sensors" + +config PAT9125 + tristate "Optical tracker PAT9125 I2C driver" + depends on I2C + select IIO_BUFFER + help + Say yes here to build support for PAT9125 optical tracker + sensors. + + To compile this driver as a module, say M here: the module will + be called pat9125. +endmenu diff --git a/drivers/iio/position/Makefile b/drivers/iio/position/Makefile new file mode 100644 index 000000000000..cf294917ae2c --- /dev/null +++ b/drivers/iio/position/Makefile @@ -0,0 +1,6 @@ +# +# Makefile for industrial I/O Optical tracker sensor drivers +# + +# When adding new entries keep the list in alphabetical order +obj-$(CONFIG_PAT9125) += pat9125.o diff --git a/drivers/iio/position/pat9125.c b/drivers/iio/position/pat9125.c new file mode 100644 index 000000000000..22bf729bec9b --- /dev/null +++ b/drivers/iio/position/pat9125.c @@ -0,0 +1,499 @@ +// SPDX-License-Identifier: (GPL-2.0) +/* + * Copyright (C) 2019 BayLibre, SAS + * Author: Alexandre Mergnat <amergnat@baylibre.com> + */ + +#include <linux/bitops.h> +#include <linux/delay.h> +#include <linux/interrupt.h> +#include <linux/i2c.h> +#include <linux/iio/iio.h> +#include <linux/iio/sysfs.h> +#include <linux/iio/events.h> +#include <linux/iio/buffer.h> +#include <linux/iio/trigger.h> +#include <linux/iio/trigger_consumer.h> +#include <linux/iio/triggered_buffer.h> +#include <linux/iio/kfifo_buf.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/regmap.h> +#include <linux/slab.h> + +/* I2C Address function to ID pin*/ +#define PAT9125_I2C_ADDR_HI 0x73 +#define PAT9125_I2C_ADDR_LO 0x75 +#define PAT9125_I2C_ADDR_NC 0x79 + +/* Registers */ +#define PAT9125_PRD_ID1_REG 0x00 +#define PAT9125_PRD_ID2_REG 0x01 +#define PAT9125_MOTION_STATUS_REG 0x02 +#define PAT9125_DELTA_X_LO_REG 0x03 +#define PAT9125_DELTA_Y_LO_REG 0x04 +#define PAT9125_OP_MODE_REG 0x05 +#define PAT9125_CONFIG_REG 0x06 +#define PAT9125_WRITE_PROTEC_REG 0x09 +#define PAT9125_SLEEP1_REG 0x0A +#define PAT9125_SLEEP2_REG 0x0B +#define PAT9125_RES_X_REG 0x0D +#define PAT9125_RES_Y_REG 0x0E +#define PAT9125_DELTA_XY_HI_REG 0x12 +#define PAT9125_SHUTER_REG 0x14 +#define PAT9125_FRAME_AVG_REG 0x17 +#define PAT9125_ORIENTATION_REG 0x19 + +/* Bits */ +#define PAT9125_VALID_MOTION_DATA_BIT BIT(7) +#define PAT9125_RESET_BIT BIT(7) + +/* Registers' values */ +#define PAT9125_SENSOR_ID_VAL 0x31 +#define PAT9125_DISABLE_WRITE_PROTECT_VAL 0x5A +#define PAT9125_ENABLE_WRITE_PROTECT_VAL 0x00 + +/* Default Value of sampled value size */ +#define PAT9125_SAMPLED_VAL_BIT_SIZE 12 + +struct pat9125_data { + struct regmap *regmap; + struct iio_trigger *indio_trig; /* Motion detection */ + s32 delta_x; + s32 delta_y; + s32 position_x; + s32 position_y; + bool sampling; +}; + +static const struct iio_chan_spec pat9125_channels[] = { + { + .type = IIO_DISTANCE, + .modified = 1, + .channel2 = IIO_MOD_X, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE), + .scan_index = 0, + .scan_type = { + .sign = 's', + .realbits = 32, + .storagebits = 32, + .endianness = IIO_CPU, + }, + }, + { + .type = IIO_DISTANCE, + .modified = 1, + .channel2 = IIO_MOD_Y, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE), + .scan_index = 1, + .scan_type = { + .sign = 's', + .realbits = 32, + .storagebits = 32, + .endianness = IIO_CPU, + }, + }, + IIO_CHAN_SOFT_TIMESTAMP(2), +}; + +/** + * pat9125_write_pretected_reg() - Write value in protected register. + * + * @regmap: Pointer to I2C register map. + * @reg_addr: Register address. + * @reg_value: Value to be write in register. + * + * A value of zero will be returned on success, a negative errno will + * be returned in error cases. + */ +static int pat9125_write_pretected_reg(struct iio_dev *indio_dev, + u8 reg_addr, u8 reg_value) +{ + struct pat9125_data *data = iio_priv(indio_dev); + int ret; + + ret = regmap_write(data->regmap, + PAT9125_WRITE_PROTEC_REG, + PAT9125_DISABLE_WRITE_PROTECT_VAL); + if (ret < 0) { + dev_err(indio_dev->dev.parent, "register 0x%x access failed %d\n", + PAT9125_WRITE_PROTEC_REG, ret); + return ret; + } + + ret = regmap_write(data->regmap, reg_addr, reg_value); + if (ret < 0) { + dev_err(indio_dev->dev.parent, "register 0x%x access failed %d\n", + reg_addr, ret); + return ret; + } + + ret = regmap_write(data->regmap, + PAT9125_WRITE_PROTEC_REG, + PAT9125_ENABLE_WRITE_PROTECT_VAL); + if (ret < 0) { + dev_err(indio_dev->dev.parent, "register 0x%x access failed %d\n", + PAT9125_WRITE_PROTEC_REG, ret); + return ret; + } + return 0; +} +/** + * pat9125_read_delta() - Read delta value, update delta & position data. + * + * @data: Driver's data structure. + * + * A value of zero will be returned on success, a negative errno will + * be returned in error cases. + */ +static int pat9125_read_delta(struct pat9125_data *data) +{ + struct regmap *regmap = data->regmap; + int status = 0; + int val_x = 0; + int val_y = 0; + int val_high_nibbles = 0; + int ret; + + ret = regmap_read(regmap, PAT9125_MOTION_STATUS_REG, &status); + if (ret < 0) + return ret; + + /* Check if motion is detected */ + if (status & PAT9125_VALID_MOTION_DATA_BIT) { + ret = regmap_read(regmap, PAT9125_DELTA_X_LO_REG, &val_x); + if (ret < 0) + return ret; + + ret = regmap_read(regmap, PAT9125_DELTA_Y_LO_REG, &val_y); + if (ret < 0) + return ret; + + ret = regmap_read(regmap, PAT9125_DELTA_XY_HI_REG, + &val_high_nibbles); + if (ret < 0) + return ret; + + val_x |= (val_high_nibbles << 4) & 0xF00; + val_y |= (val_high_nibbles << 8) & 0xF00; + val_x = sign_extend32(val_x, + PAT9125_SAMPLED_VAL_BIT_SIZE - 1); + val_y = sign_extend32(val_y, + PAT9125_SAMPLED_VAL_BIT_SIZE - 1); + data->position_x += val_x; + data->position_y += val_y; + data->delta_x = val_x; + data->delta_y = val_y; + } + return 0; +} + +/** + * pat9125_read_raw() - Sample and return the value(s) + * function to the associated channel info enum. + **/ +static int pat9125_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int *val, int *val2, long mask) +{ + struct pat9125_data *data = iio_priv(indio_dev); + int ret; + + switch (mask) { + case IIO_CHAN_INFO_RAW: + ret = pat9125_read_delta(data); + if (ret) + return ret; + switch (chan->channel2) { + case IIO_MOD_X: + *val = data->position_x; + return IIO_VAL_INT; + case IIO_MOD_Y: + *val = data->position_y; + return IIO_VAL_INT; + default: + return -EINVAL; + } + case IIO_CHAN_INFO_SCALE: + switch (chan->channel2) { + case IIO_MOD_X: + ret = regmap_read(data->regmap, PAT9125_RES_X_REG, val); + if (ret) + return ret; + else + return IIO_VAL_INT; + case IIO_MOD_Y: + ret = regmap_read(data->regmap, PAT9125_RES_Y_REG, val); + if (ret) + return ret; + else + return IIO_VAL_INT; + default: + return -EINVAL; + } + default: + return -EINVAL; + } +} + +/** + * pat9125_write_raw() - Write the value(s) + * function to the associated channel info enum. + **/ +static int pat9125_write_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, int val, + int val2, long mask) +{ + int ret; + + switch (mask) { + case IIO_CHAN_INFO_SCALE: + switch (chan->channel2) { + case IIO_MOD_X: + ret = pat9125_write_pretected_reg(indio_dev, + PAT9125_RES_X_REG, val); + return ret; + case IIO_MOD_Y: + ret = pat9125_write_pretected_reg(indio_dev, + PAT9125_RES_Y_REG, val); + return ret; + default: + return -EINVAL; + } + default: + return -EINVAL; + } +} + +static irqreturn_t pat9125_threaded_trigger_handler(int irq, void *p) +{ + struct iio_poll_func *pf = p; + struct iio_dev *indio_dev = pf->indio_dev; + struct pat9125_data *data = iio_priv(indio_dev); + u8 buf[16]; /* Payload: Delta_X (4) | Delta_Y (4) | Timestamp (8) */ + int ret; + s64 timestamp; + + data->sampling = true; + ret = pat9125_read_delta(data); + if (ret) { + dev_err(indio_dev->dev.parent, "Read delta failed %d\n", ret); + return IRQ_NONE; + } + timestamp = iio_get_time_ns(indio_dev); + *((s32 *)&buf[0]) = data->delta_x; + *((s32 *)&buf[sizeof(s32)]) = data->delta_y; + data->delta_x = 0; + data->delta_y = 0; + iio_push_to_buffers_with_timestamp(indio_dev, buf, timestamp); + iio_trigger_notify_done(indio_dev->trig); + return IRQ_HANDLED; +} + +/** + * pat9125_threaded_event_handler() - Threaded motion detection event handler + * @irq: The irq being handled. + * @private: struct iio_device pointer for the device. + */ +static irqreturn_t pat9125_threaded_event_handler(int irq, void *private) +{ + struct iio_dev *indio_dev = private; + struct pat9125_data *data = iio_priv(indio_dev); + + iio_trigger_poll_chained(data->indio_trig); + return IRQ_HANDLED; +} + +static int pat9125_buffer_postenable(struct iio_dev *indio_dev) +{ + struct pat9125_data *data = iio_priv(indio_dev); + int ret = 0; + + ret = iio_triggered_buffer_postenable(indio_dev); + if (ret) + return ret; + /* Release interrupt pin on the device */ + return pat9125_read_delta(data); +} + +static const struct iio_buffer_setup_ops pat9125_buffer_ops = { + .postenable = pat9125_buffer_postenable, + .predisable = iio_triggered_buffer_predisable, +}; + + +static const struct regmap_config pat9125_regmap_config = { + .reg_bits = 8, + .val_bits = 8, +}; + +static const struct iio_info pat9125_info = { + .read_raw = pat9125_read_raw, + .write_raw = pat9125_write_raw, +}; + +/* + * To detect if a new value is available, register status is checked. This + * method is safer than using a flag on GPIO IRQ to track event while sampling + * because falling edge is missed when device trig just after a read reg value + * (that happen for fast motions or high CPI setting). + * + * Note: To avoid infinite loop in "iio_trigger_notify_done" when it si not in + * buffer mode and kernel warning due to nested IRQ thread, + * this function must return 0. + */ +static int pat9125_trig_try_reenable(struct iio_trigger *trig) +{ + struct pat9125_data *data = iio_trigger_get_drvdata(trig); + struct regmap *regmap = data->regmap; + int status = 0; + + if (data->sampling) { + regmap_read(regmap, PAT9125_MOTION_STATUS_REG, &status); + if (status & PAT9125_VALID_MOTION_DATA_BIT) { + data->sampling = false; + iio_trigger_poll_chained(data->indio_trig); + return 0; + } + } + data->sampling = false; + return 0; +} + +static const struct iio_trigger_ops pat9125_trigger_ops = { + .try_reenable = pat9125_trig_try_reenable, +}; + +static int pat9125_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + struct pat9125_data *data; + struct iio_dev *indio_dev; + int ret, sensor_pid; + + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); + if (!indio_dev) { + dev_err(&client->dev, "IIO device allocation failed\n"); + return -ENOMEM; + } + + data = iio_priv(indio_dev); + indio_dev->dev.parent = &client->dev; + indio_dev->name = id->name; + indio_dev->channels = pat9125_channels; + indio_dev->num_channels = ARRAY_SIZE(pat9125_channels); + indio_dev->info = &pat9125_info; + indio_dev->modes = INDIO_DIRECT_MODE; + + ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL, + pat9125_threaded_trigger_handler, &pat9125_buffer_ops); + if (ret) { + dev_err(&client->dev, "unable to setup triggered buffer\n"); + return ret; + } + + data->indio_trig = devm_iio_trigger_alloc(&client->dev, "%s-dev%d", + indio_dev->name, indio_dev->id); + if (!data->indio_trig) + return -ENOMEM; + data->indio_trig->dev.parent = &client->dev; + data->indio_trig->ops = &pat9125_trigger_ops; + iio_trigger_set_drvdata(data->indio_trig, data); + ret = devm_iio_trigger_register(&client->dev, data->indio_trig); + if (ret) { + dev_err(&client->dev, "unable to register trigger\n"); + return ret; + } + + data->regmap = devm_regmap_init_i2c(client, &pat9125_regmap_config); + if (IS_ERR(data->regmap)) { + dev_err(&client->dev, "regmap init failed %ld\n", + PTR_ERR(data->regmap)); + return PTR_ERR(data->regmap); + } + + /* Check device ID */ + ret = regmap_read(data->regmap, PAT9125_PRD_ID1_REG, &sensor_pid); + if (ret < 0) { + dev_err(&client->dev, "register 0x%x access failed %d\n", + PAT9125_PRD_ID1_REG, ret); + return ret; + } + if (sensor_pid != PAT9125_SENSOR_ID_VAL) + return -ENODEV; + + /* Switch to bank0 (Magic number)*/ + ret = regmap_write(data->regmap, 0x7F, 0x00); + if (ret < 0) { + dev_err(indio_dev->dev.parent, "register 0x%x access failed %d\n", + 0x7F, ret); + return ret; + } + + /* Software reset */ + ret = regmap_write_bits(data->regmap, + PAT9125_CONFIG_REG, + PAT9125_RESET_BIT, + 1); + if (ret < 0) { + dev_err(&client->dev, "register 0x%x access failed %d\n", + PAT9125_CONFIG_REG, ret); + return ret; + } + + msleep(20); + + /* Init GPIO IRQ */ + if (client->irq) { + ret = devm_request_threaded_irq(&client->dev, + client->irq, + NULL, + pat9125_threaded_event_handler, + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, + "pat9125", + indio_dev); + if (ret) { + dev_err(&client->dev, "GPIO IRQ init failed\n"); + return ret; + } + } + + ret = devm_iio_device_register(&client->dev, indio_dev); + if (ret) { + dev_err(&client->dev, "IIO device register failed\n"); + return ret; + } + + i2c_set_clientdata(client, indio_dev); + return 0; +} + +static const struct i2c_device_id pat9125_id[] = { + { "pat9125", 0 }, + {} +}; +MODULE_DEVICE_TABLE(i2c, pat9125_id); + +static const unsigned short normal_i2c[] = { + PAT9125_I2C_ADDR_HI, + PAT9125_I2C_ADDR_LO, + PAT9125_I2C_ADDR_NC, + I2C_CLIENT_END +}; + +static struct i2c_driver pat9125_driver = { + .driver = { + .name = "pat9125", + }, + .probe = pat9125_probe, + .address_list = normal_i2c, + .id_table = pat9125_id, +}; + +module_i2c_driver(pat9125_driver); + +MODULE_AUTHOR("Alexandre Mergnat <amergnat@baylibre.com>"); +MODULE_DESCRIPTION("Optical Tracking sensor"); +MODULE_LICENSE("GPL");
This adds support for PixArt Imaging’s miniature low power optical navigation chip using LASER light source enabling digital surface tracking. Feature and datasheet: [0] This IIO driver allows to read delta or relative position on X and Y axis: - The position relative to where the system started can be taken through punctual "read_raw" which will issue a read in the device registers to get the delta between last/current read and return the addition of all the deltas. - The delta can be retrieved using triggered buffer subscription (i.e. iio_readdev). The buffer payload is: |32 bits delta X|32 bits delta Y|timestamp|. The possible I2C addresses are 0x73, 0x75 and 0x79. X and Y axis CPI resolution can be get/set independently through IIO_SCALE. The range value is 0-255 which means: - 0 to ~1,275 Counts Per Inch on flat surface. - 0 to ~630 Counts Per Rev on 1.0mm diameter STS shaft at 1.0mm distance. More details on the datasheet. The "position" directory is added to contain drivers which can provide position data. Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> [0]: http://www.pixart.com/products-detail/72/PAT9125EL-TKIT___TKMT Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> --- drivers/iio/Kconfig | 1 + drivers/iio/Makefile | 1 + drivers/iio/position/Kconfig | 18 ++ drivers/iio/position/Makefile | 6 + drivers/iio/position/pat9125.c | 499 +++++++++++++++++++++++++++++++++ 5 files changed, 525 insertions(+) create mode 100644 drivers/iio/position/Kconfig create mode 100644 drivers/iio/position/Makefile create mode 100644 drivers/iio/position/pat9125.c