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

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

[Jonathan Cameron]

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

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

[Alexandre]

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

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

[Jonathan Cameron]

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
> 

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

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

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

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

[Jonathan Cameron]

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
>