[PATCH v5 0/2] iio: proximity: add PulsedLight LIDAR sensor support

[PATCH v5 0/2] iio: proximity: add PulsedLight LIDAR sensor support

[Matt Ranostay]

This patchset adds support for this range finding LIDAR module, and reports
distance back in meters to centimeter resolution with +/- 2.5 CM of accuracy
at ~40 meters.

Changes from v4:
* moved comment to read command function
* remove un-needed buffer indexing
* renamed device and driver name to pulsedlight-lidar-lite-v2 to resolve
  confusion that v1 will likely not work with this driver

Matt Ranostay (2):
  devicetree: add PulsedLight vendor + device docs
  iio: proximity: add support for PulsedLight LIDAR

 .../devicetree/bindings/i2c/trivial-devices.txt    |   1 +
 .../devicetree/bindings/vendor-prefixes.txt        |   1 +
 drivers/iio/proximity/Kconfig                      |  12 +
 drivers/iio/proximity/Makefile                     |   1 +
 drivers/iio/proximity/pulsedlight-lidar-lite-v2.c  | 292 +++++++++++++++++++++
 5 files changed, 307 insertions(+)
 create mode 100644 drivers/iio/proximity/pulsedlight-lidar-lite-v2.c

-- 
1.9.1

[PATCH 1/2] devicetree: add PulsedLight vendor + device docs

[Matt Ranostay]

Add pulsedlight vendor to vendor-prefixes.txt, and LIDAR device
documentation to trivial-devices.txt

Signed-off-by: Matt Ranostay <mranostay@gmail.com>
---
 Documentation/devicetree/bindings/i2c/trivial-devices.txt | 1 +
 Documentation/devicetree/bindings/vendor-prefixes.txt     | 1 +
 2 files changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/trivial-devices.txt b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
index d77d412..cecd315 100644
--- a/Documentation/devicetree/bindings/i2c/trivial-devices.txt
+++ b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
@@ -80,6 +80,7 @@ oki,ml86v7667		OKI ML86V7667 video decoder
 ovti,ov5642		OV5642: Color CMOS QSXGA (5-megapixel) Image Sensor with OmniBSI and Embedded TrueFocus
 pericom,pt7c4338	Real-time Clock Module
 plx,pex8648		48-Lane, 12-Port PCI Express Gen 2 (5.0 GT/s) Switch
+pulsedlight,lidar-lite-v2	Pulsedlight LIDAR range-finding sensor
 ramtron,24c64		i2c serial eeprom  (24cxx)
 ricoh,r2025sd		I2C bus SERIAL INTERFACE REAL-TIME CLOCK IC
 ricoh,r2221tl		I2C bus SERIAL INTERFACE REAL-TIME CLOCK IC
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 6284568..0f318c3 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -164,6 +164,7 @@ phytec	PHYTEC Messtechnik GmbH
 picochip	Picochip Ltd
 plathome	Plat'Home Co., Ltd.
 pixcir  PIXCIR MICROELECTRONICS Co., Ltd
+pulsedlight	PulsedLight, Inc
 powervr	PowerVR (deprecated, use img)
 qca	Qualcomm Atheros, Inc.
 qcom	Qualcomm Technologies, Inc
-- 
1.9.1

[PATCH 2/2] iio: proximity: add support for PulsedLight LIDAR

[Matt Ranostay]

Add support for the PulsedLight LIDAR rangefinder sensor which allows
high speed (over 300Hz) distance measurements using Barker Coding within
40 meter range.

Support only tested on the "blue label" rev 2, but may work using low
sample frequencies on the original version.

Signed-off-by: Matt Ranostay <mranostay@gmail.com>
---
 drivers/iio/proximity/Kconfig                     |  12 +
 drivers/iio/proximity/Makefile                    |   1 +
 drivers/iio/proximity/pulsedlight-lidar-lite-v2.c | 292 ++++++++++++++++++++++
 3 files changed, 305 insertions(+)
 create mode 100644 drivers/iio/proximity/pulsedlight-lidar-lite-v2.c

diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
index 41a8d8f..e64fc5b 100644
--- a/drivers/iio/proximity/Kconfig
+++ b/drivers/iio/proximity/Kconfig
@@ -20,6 +20,18 @@ endmenu
 
 menu "Proximity sensors"
 
+config LIDAR
+	tristate "PulsedLight LIDAR sensor"
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	depends on I2C
+	help
+	  Say Y to build a driver for PulsedLight LIDAR range finding
+	  sensor.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called lidar.
+
 config SX9500
 	tristate "SX9500 Semtech proximity sensor"
 	select IIO_BUFFER
diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
index 9818dc5..081ae33 100644
--- a/drivers/iio/proximity/Makefile
+++ b/drivers/iio/proximity/Makefile
@@ -4,4 +4,5 @@
 
 # When adding new entries keep the list in alphabetical order
 obj-$(CONFIG_AS3935)		+= as3935.o
+obj-$(CONFIG_LIDAR)		+= pulsedlight-lidar-lite-v2.o
 obj-$(CONFIG_SX9500)		+= sx9500.o
diff --git a/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
new file mode 100644
index 0000000..ff0681b
--- /dev/null
+++ b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
@@ -0,0 +1,292 @@
+/*
+ * pulsedlight-lidar-lite-v2.c - Support for PulsedLight LIDAR sensor
+ *
+ * Copyright (C) 2015 Matt Ranostay <mranostay@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * TODO: runtime pm, interrupt mode, and signal strength reporting
+ *
+ */
+
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/i2c.h>
+#include <linux/delay.h>
+#include <linux/mutex.h>
+#include <linux/module.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
+
+#define LIDAR_REG_CONTROL		0x00
+#define LIDAR_REG_CONTROL_ACQUIRE	BIT(2)
+
+#define LIDAR_REG_STATUS		0x01
+#define LIDAR_REG_STATUS_INVALID	BIT(3)
+#define LIDAR_REG_STATUS_READY		BIT(0)
+
+#define LIDAR_REG_DATA_HBYTE	0x0f
+#define LIDAR_REG_DATA_LBYTE	0x10
+
+#define LIDAR_DRV_NAME "lidar"
+
+struct lidar_data {
+	struct mutex lock;
+	struct iio_dev *indio_dev;
+	struct i2c_client *client;
+
+	u16 buffer[8]; /* 2 byte distance + 8 byte timestamp */
+};
+
+static const struct iio_chan_spec lidar_channels[] = {
+	{
+		.type = IIO_DISTANCE,
+		.info_mask_separate =
+			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
+		.scan_index = 0,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+		},
+	},
+	IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static int lidar_read_byte(struct lidar_data *data, int reg)
+{
+	struct i2c_client *client = data->client;
+	int ret;
+
+	/*
+	 * Device needs a STOP condition between address write, and data read
+	 * so in turn i2c_smbus_read_byte_data cannot be used
+	 */
+
+	ret = i2c_smbus_write_byte(client, reg);
+	if (ret < 0) {
+		dev_err(&client->dev, "cannot write addr value");
+		return ret;
+	}
+
+	ret = i2c_smbus_read_byte(client);
+	if (ret < 0)
+		dev_err(&client->dev, "cannot read data value");
+
+	return ret;
+}
+
+static inline int lidar_write_control(struct lidar_data *data, int val)
+{
+	return i2c_smbus_write_byte_data(data->client, LIDAR_REG_CONTROL, val);
+}
+
+static int lidar_read_measurement(struct lidar_data *data, u16 *reg)
+{
+	int ret;
+	int val;
+
+	ret = lidar_read_byte(data, LIDAR_REG_DATA_HBYTE);
+	if (ret < 0)
+		return ret;
+	val = ret << 8;
+
+	ret = lidar_read_byte(data, LIDAR_REG_DATA_LBYTE);
+	if (ret < 0)
+		return ret;
+
+	val |= ret;
+	*reg = val;
+
+	return 0;
+}
+
+static int lidar_get_measurement(struct lidar_data *data, u16 *reg)
+{
+	struct i2c_client *client = data->client;
+	int tries = 10;
+	int ret;
+
+	/* start sample */
+	ret = lidar_write_control(data, LIDAR_REG_CONTROL_ACQUIRE);
+	if (ret < 0) {
+		dev_err(&client->dev, "cannot send start measurement command");
+		return ret;
+	}
+
+	while (tries--) {
+		usleep_range(1000, 2000);
+
+		ret = lidar_read_byte(data, LIDAR_REG_STATUS);
+		if (ret < 0)
+			break;
+
+		/* return 0 since laser is likely pointed out of range */
+		if (ret & LIDAR_REG_STATUS_INVALID) {
+			*reg = 0;
+			ret = 0;
+			break;
+		}
+
+		/* sample ready to read */
+		if (!(ret & LIDAR_REG_STATUS_READY)) {
+			ret = lidar_read_measurement(data, reg);
+			break;
+		}
+		ret = -EIO;
+	}
+
+	return ret;
+}
+
+static int lidar_read_raw(struct iio_dev *indio_dev,
+			  struct iio_chan_spec const *chan,
+			  int *val, int *val2, long mask)
+{
+	struct lidar_data *data = iio_priv(indio_dev);
+	int ret = -EINVAL;
+
+	mutex_lock(&data->lock);
+
+	if (iio_buffer_enabled(indio_dev) && mask == IIO_CHAN_INFO_RAW) {
+		ret = -EBUSY;
+		goto error_busy;
+	}
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW: {
+		u16 reg;
+
+		ret = lidar_get_measurement(data, &reg);
+		if (!ret) {
+			*val = reg;
+			ret = IIO_VAL_INT;
+		}
+		break;
+	}
+	case IIO_CHAN_INFO_SCALE:
+		*val = 0;
+		*val2 = 10000;
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+	}
+
+error_busy:
+	mutex_unlock(&data->lock);
+
+	return ret;
+}
+
+static irqreturn_t lidar_trigger_handler(int irq, void *private)
+{
+	struct iio_poll_func *pf = private;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct lidar_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = lidar_get_measurement(data, data->buffer);
+	if (!ret) {
+		iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
+						   iio_get_time_ns());
+	} else {
+		dev_err(&data->client->dev, "cannot read LIDAR measurement");
+	}
+
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static const struct iio_info lidar_info = {
+	.driver_module = THIS_MODULE,
+	.read_raw = lidar_read_raw,
+};
+
+static int lidar_probe(struct i2c_client *client,
+		       const struct i2c_device_id *id)
+{
+	struct lidar_data *data;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	indio_dev->info = &lidar_info;
+	indio_dev->name = LIDAR_DRV_NAME;
+	indio_dev->channels = lidar_channels;
+	indio_dev->num_channels = ARRAY_SIZE(lidar_channels);
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	data = iio_priv(indio_dev);
+	i2c_set_clientdata(client, indio_dev);
+
+	data->client = client;
+	data->indio_dev = indio_dev;
+	mutex_init(&data->lock);
+
+	ret = iio_triggered_buffer_setup(indio_dev, NULL,
+					 lidar_trigger_handler, NULL);
+	if (ret)
+		return ret;
+
+	ret = iio_device_register(indio_dev);
+	if (ret)
+		goto error_unreg_buffer;
+
+	return 0;
+
+error_unreg_buffer:
+	iio_triggered_buffer_cleanup(indio_dev);
+
+	return ret;
+}
+
+static int lidar_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+
+	iio_device_unregister(indio_dev);
+	iio_triggered_buffer_cleanup(indio_dev);
+
+	return 0;
+}
+
+static const struct i2c_device_id lidar_id[] = {
+	{"lidar", 0},
+	{ },
+};
+MODULE_DEVICE_TABLE(i2c, lidar_id);
+
+static const struct of_device_id lidar_dt_ids[] = {
+	{ .compatible = "pulsedlight,lidar-lite-v2" },
+	{ }
+};
+
+static struct i2c_driver lidar_driver = {
+	.driver = {
+		.name	= LIDAR_DRV_NAME,
+		.of_match_table	= of_match_ptr(lidar_dt_ids),
+	},
+	.probe		= lidar_probe,
+	.remove		= lidar_remove,
+	.id_table	= lidar_id,
+};
+module_i2c_driver(lidar_driver);
+
+MODULE_AUTHOR("Matt Ranostay <mranostay@gmail.com>");
+MODULE_DESCRIPTION("PulsedLight LIDAR sensor");
+MODULE_LICENSE("GPL");
-- 
1.9.1

Re: [PATCH 2/2] iio: proximity: add support for PulsedLight LIDAR

[Jonathan Cameron]

On 12/08/15 07:01, Matt Ranostay wrote:
> Add support for the PulsedLight LIDAR rangefinder sensor which allows
> high speed (over 300Hz) distance measurements using Barker Coding within
> 40 meter range.
> 
> Support only tested on the "blue label" rev 2, but may work using low
> sample frequencies on the original version.
> 
> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
You missed a few cases of the name lidar being used for device
matching.

Also, just noticed you are holding a local lock only when checking if
bufferred capture is enabled.  You need to hold the indio_dev->mlock to
avoid racing with a buffer enable.

Mostly there now though.

Jonathan
> ---
>  drivers/iio/proximity/Kconfig                     |  12 +
>  drivers/iio/proximity/Makefile                    |   1 +
>  drivers/iio/proximity/pulsedlight-lidar-lite-v2.c | 292 ++++++++++++++++++++++
>  3 files changed, 305 insertions(+)
>  create mode 100644 drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
> 
> diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
> index 41a8d8f..e64fc5b 100644
> --- a/drivers/iio/proximity/Kconfig
> +++ b/drivers/iio/proximity/Kconfig
> @@ -20,6 +20,18 @@ endmenu
>  
>  menu "Proximity sensors"
>  
> +config LIDAR
The config symbol needs more detail as well as it's not a unique
part number.
> +	tristate "PulsedLight LIDAR sensor"
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
> +	depends on I2C
> +	help
> +	  Say Y to build a driver for PulsedLight LIDAR range finding
> +	  sensor.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called lidar.
Not any more it won't.
> +
>  config SX9500
>  	tristate "SX9500 Semtech proximity sensor"
>  	select IIO_BUFFER
> diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
> index 9818dc5..081ae33 100644
> --- a/drivers/iio/proximity/Makefile
> +++ b/drivers/iio/proximity/Makefile
> @@ -4,4 +4,5 @@
>  
>  # When adding new entries keep the list in alphabetical order
>  obj-$(CONFIG_AS3935)		+= as3935.o
> +obj-$(CONFIG_LIDAR)		+= pulsedlight-lidar-lite-v2.o
>  obj-$(CONFIG_SX9500)		+= sx9500.o
> diff --git a/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
> new file mode 100644
> index 0000000..ff0681b
> --- /dev/null
> +++ b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
> @@ -0,0 +1,292 @@
> +/*
> + * pulsedlight-lidar-lite-v2.c - Support for PulsedLight LIDAR sensor
> + *
> + * Copyright (C) 2015 Matt Ranostay <mranostay@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * TODO: runtime pm, interrupt mode, and signal strength reporting
> + *
No blank line here.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/i2c.h>
> +#include <linux/delay.h>
> +#include <linux/mutex.h>
> +#include <linux/module.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +
> +#define LIDAR_REG_CONTROL		0x00
> +#define LIDAR_REG_CONTROL_ACQUIRE	BIT(2)
> +
> +#define LIDAR_REG_STATUS		0x01
> +#define LIDAR_REG_STATUS_INVALID	BIT(3)
> +#define LIDAR_REG_STATUS_READY		BIT(0)
> +
> +#define LIDAR_REG_DATA_HBYTE	0x0f
> +#define LIDAR_REG_DATA_LBYTE	0x10
> +
> +#define LIDAR_DRV_NAME "lidar"
> +
> +struct lidar_data {
> +	struct mutex lock;
> +	struct iio_dev *indio_dev;
> +	struct i2c_client *client;
> +
> +	u16 buffer[8]; /* 2 byte distance + 8 byte timestamp */
> +};
> +
> +static const struct iio_chan_spec lidar_channels[] = {
> +	{
> +		.type = IIO_DISTANCE,
> +		.info_mask_separate =
> +			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> +		.scan_index = 0,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 16,
> +			.storagebits = 16,
> +		},
> +	},
> +	IIO_CHAN_SOFT_TIMESTAMP(1),
> +};
> +
> +static int lidar_read_byte(struct lidar_data *data, int reg)
> +{
> +	struct i2c_client *client = data->client;
> +	int ret;
> +
> +	/*
> +	 * Device needs a STOP condition between address write, and data read
> +	 * so in turn i2c_smbus_read_byte_data cannot be used
> +	 */
> +
> +	ret = i2c_smbus_write_byte(client, reg);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "cannot write addr value");
> +		return ret;
> +	}
> +
> +	ret = i2c_smbus_read_byte(client);
> +	if (ret < 0)
> +		dev_err(&client->dev, "cannot read data value");
> +
> +	return ret;
> +}
> +
> +static inline int lidar_write_control(struct lidar_data *data, int val)
> +{
> +	return i2c_smbus_write_byte_data(data->client, LIDAR_REG_CONTROL, val);
> +}
> +
> +static int lidar_read_measurement(struct lidar_data *data, u16 *reg)
> +{
> +	int ret;
> +	int val;
> +
> +	ret = lidar_read_byte(data, LIDAR_REG_DATA_HBYTE);
> +	if (ret < 0)
> +		return ret;
> +	val = ret << 8;
> +
> +	ret = lidar_read_byte(data, LIDAR_REG_DATA_LBYTE);
> +	if (ret < 0)
> +		return ret;
> +
> +	val |= ret;
> +	*reg = val;
> +
> +	return 0;
> +}
> +
> +static int lidar_get_measurement(struct lidar_data *data, u16 *reg)
> +{
> +	struct i2c_client *client = data->client;
> +	int tries = 10;
> +	int ret;
> +
> +	/* start sample */
> +	ret = lidar_write_control(data, LIDAR_REG_CONTROL_ACQUIRE);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "cannot send start measurement command");
> +		return ret;
> +	}
> +
> +	while (tries--) {
> +		usleep_range(1000, 2000);
> +
> +		ret = lidar_read_byte(data, LIDAR_REG_STATUS);
> +		if (ret < 0)
> +			break;
> +
> +		/* return 0 since laser is likely pointed out of range */
> +		if (ret & LIDAR_REG_STATUS_INVALID) {
> +			*reg = 0;
> +			ret = 0;
> +			break;
> +		}
> +
> +		/* sample ready to read */
> +		if (!(ret & LIDAR_REG_STATUS_READY)) {
> +			ret = lidar_read_measurement(data, reg);
> +			break;
> +		}
> +		ret = -EIO;
> +	}
> +
> +	return ret;
> +}
> +
> +static int lidar_read_raw(struct iio_dev *indio_dev,
> +			  struct iio_chan_spec const *chan,
> +			  int *val, int *val2, long mask)
> +{
> +	struct lidar_data *data = iio_priv(indio_dev);
> +	int ret = -EINVAL;
> +
> +	mutex_lock(&data->lock);
> +

This particular check needs to hold the indio_dev->mlock as that
is the one that protects core state changes (enabling and disabling the
buffer).  You want to hang on to that lock until the read is done.

> +	if (iio_buffer_enabled(indio_dev) && mask == IIO_CHAN_INFO_RAW) {
> +		ret = -EBUSY;
> +		goto error_busy;
> +	}
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW: {
> +		u16 reg;
> +
> +		ret = lidar_get_measurement(data, &reg);
> +		if (!ret) {
> +			*val = reg;
> +			ret = IIO_VAL_INT;
> +		}
> +		break;
> +	}
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = 0;
> +		*val2 = 10000;
> +		ret = IIO_VAL_INT_PLUS_MICRO;
> +		break;
> +	}
> +
> +error_busy:
> +	mutex_unlock(&data->lock);
> +
> +	return ret;
> +}
> +
> +static irqreturn_t lidar_trigger_handler(int irq, void *private)
> +{
> +	struct iio_poll_func *pf = private;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct lidar_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = lidar_get_measurement(data, data->buffer);
> +	if (!ret) {
> +		iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
> +						   iio_get_time_ns());
> +	} else {
> +		dev_err(&data->client->dev, "cannot read LIDAR measurement");
> +	}
> +
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static const struct iio_info lidar_info = {
> +	.driver_module = THIS_MODULE,
> +	.read_raw = lidar_read_raw,
> +};
> +
> +static int lidar_probe(struct i2c_client *client,
> +		       const struct i2c_device_id *id)
> +{
> +	struct lidar_data *data;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	indio_dev->info = &lidar_info;
> +	indio_dev->name = LIDAR_DRV_NAME;
> +	indio_dev->channels = lidar_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(lidar_channels);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	data = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);
> +
> +	data->client = client;
> +	data->indio_dev = indio_dev;
> +	mutex_init(&data->lock);
> +
> +	ret = iio_triggered_buffer_setup(indio_dev, NULL,
> +					 lidar_trigger_handler, NULL);
> +	if (ret)
> +		return ret;
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret)
> +		goto error_unreg_buffer;
> +
> +	return 0;
> +
> +error_unreg_buffer:
> +	iio_triggered_buffer_cleanup(indio_dev);
> +
> +	return ret;
> +}
> +
> +static int lidar_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +
> +	iio_device_unregister(indio_dev);
> +	iio_triggered_buffer_cleanup(indio_dev);
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id lidar_id[] = {
> +	{"lidar", 0},

This one wants a longer name as well. It's afterall used for
matching in some circumstances.

> +	{ },
> +};
> +MODULE_DEVICE_TABLE(i2c, lidar_id);
> +
> +static const struct of_device_id lidar_dt_ids[] = {
> +	{ .compatible = "pulsedlight,lidar-lite-v2" },
> +	{ }
> +};
> +
> +static struct i2c_driver lidar_driver = {
> +	.driver = {
> +		.name	= LIDAR_DRV_NAME,
> +		.of_match_table	= of_match_ptr(lidar_dt_ids),
> +	},
> +	.probe		= lidar_probe,
> +	.remove		= lidar_remove,
> +	.id_table	= lidar_id,
> +};
> +module_i2c_driver(lidar_driver);
> +
> +MODULE_AUTHOR("Matt Ranostay <mranostay@gmail.com>");
> +MODULE_DESCRIPTION("PulsedLight LIDAR sensor");
> +MODULE_LICENSE("GPL");
> 

Re: [PATCH 2/2] iio: proximity: add support for PulsedLight LIDAR

[Matt Ranostay]

On Sun, Aug 16, 2015 at 1:24 AM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 12/08/15 07:01, Matt Ranostay wrote:
>> Add support for the PulsedLight LIDAR rangefinder sensor which allows
>> high speed (over 300Hz) distance measurements using Barker Coding within
>> 40 meter range.
>>
>> Support only tested on the "blue label" rev 2, but may work using low
>> sample frequencies on the original version.
>>
>> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
> You missed a few cases of the name lidar being used for device
> matching.
>
> Also, just noticed you are holding a local lock only when checking if
> bufferred capture is enabled.  You need to hold the indio_dev->mlock to
> avoid racing with a buffer enable.
>
> Mostly there now though.
>
> Jonathan
>> ---
>>  drivers/iio/proximity/Kconfig                     |  12 +
>>  drivers/iio/proximity/Makefile                    |   1 +
>>  drivers/iio/proximity/pulsedlight-lidar-lite-v2.c | 292 ++++++++++++++++++++++
>>  3 files changed, 305 insertions(+)
>>  create mode 100644 drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
>>
>> diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
>> index 41a8d8f..e64fc5b 100644
>> --- a/drivers/iio/proximity/Kconfig
>> +++ b/drivers/iio/proximity/Kconfig
>> @@ -20,6 +20,18 @@ endmenu
>>
>>  menu "Proximity sensors"
>>
>> +config LIDAR
> The config symbol needs more detail as well as it's not a unique
> part number.
>> +     tristate "PulsedLight LIDAR sensor"
>> +     select IIO_BUFFER
>> +     select IIO_TRIGGERED_BUFFER
>> +     depends on I2C
>> +     help
>> +       Say Y to build a driver for PulsedLight LIDAR range finding
>> +       sensor.
>> +
>> +       To compile this driver as a module, choose M here: the
>> +       module will be called lidar.
> Not any more it won't.
>> +
>>  config SX9500
>>       tristate "SX9500 Semtech proximity sensor"
>>       select IIO_BUFFER
>> diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
>> index 9818dc5..081ae33 100644
>> --- a/drivers/iio/proximity/Makefile
>> +++ b/drivers/iio/proximity/Makefile
>> @@ -4,4 +4,5 @@
>>
>>  # When adding new entries keep the list in alphabetical order
>>  obj-$(CONFIG_AS3935)         += as3935.o
>> +obj-$(CONFIG_LIDAR)          += pulsedlight-lidar-lite-v2.o
>>  obj-$(CONFIG_SX9500)         += sx9500.o
>> diff --git a/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
>> new file mode 100644
>> index 0000000..ff0681b
>> --- /dev/null
>> +++ b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
>> @@ -0,0 +1,292 @@
>> +/*
>> + * pulsedlight-lidar-lite-v2.c - Support for PulsedLight LIDAR sensor
>> + *
>> + * Copyright (C) 2015 Matt Ranostay <mranostay@gmail.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * TODO: runtime pm, interrupt mode, and signal strength reporting
>> + *
> No blank line here.
>> + */
>> +
>> +#include <linux/err.h>
>> +#include <linux/init.h>
>> +#include <linux/i2c.h>
>> +#include <linux/delay.h>
>> +#include <linux/mutex.h>
>> +#include <linux/module.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/trigger.h>
>> +#include <linux/iio/triggered_buffer.h>
>> +#include <linux/iio/trigger_consumer.h>
>> +
>> +#define LIDAR_REG_CONTROL            0x00
>> +#define LIDAR_REG_CONTROL_ACQUIRE    BIT(2)
>> +
>> +#define LIDAR_REG_STATUS             0x01
>> +#define LIDAR_REG_STATUS_INVALID     BIT(3)
>> +#define LIDAR_REG_STATUS_READY               BIT(0)
>> +
>> +#define LIDAR_REG_DATA_HBYTE 0x0f
>> +#define LIDAR_REG_DATA_LBYTE 0x10
>> +
>> +#define LIDAR_DRV_NAME "lidar"
>> +
>> +struct lidar_data {
>> +     struct mutex lock;
>> +     struct iio_dev *indio_dev;
>> +     struct i2c_client *client;
>> +
>> +     u16 buffer[8]; /* 2 byte distance + 8 byte timestamp */
>> +};
>> +
>> +static const struct iio_chan_spec lidar_channels[] = {
>> +     {
>> +             .type = IIO_DISTANCE,
>> +             .info_mask_separate =
>> +                     BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
>> +             .scan_index = 0,
>> +             .scan_type = {
>> +                     .sign = 'u',
>> +                     .realbits = 16,
>> +                     .storagebits = 16,
>> +             },
>> +     },
>> +     IIO_CHAN_SOFT_TIMESTAMP(1),
>> +};
>> +
>> +static int lidar_read_byte(struct lidar_data *data, int reg)
>> +{
>> +     struct i2c_client *client = data->client;
>> +     int ret;
>> +
>> +     /*
>> +      * Device needs a STOP condition between address write, and data read
>> +      * so in turn i2c_smbus_read_byte_data cannot be used
>> +      */
>> +
>> +     ret = i2c_smbus_write_byte(client, reg);
>> +     if (ret < 0) {
>> +             dev_err(&client->dev, "cannot write addr value");
>> +             return ret;
>> +     }
>> +
>> +     ret = i2c_smbus_read_byte(client);
>> +     if (ret < 0)
>> +             dev_err(&client->dev, "cannot read data value");
>> +
>> +     return ret;
>> +}
>> +
>> +static inline int lidar_write_control(struct lidar_data *data, int val)
>> +{
>> +     return i2c_smbus_write_byte_data(data->client, LIDAR_REG_CONTROL, val);
>> +}
>> +
>> +static int lidar_read_measurement(struct lidar_data *data, u16 *reg)
>> +{
>> +     int ret;
>> +     int val;
>> +
>> +     ret = lidar_read_byte(data, LIDAR_REG_DATA_HBYTE);
>> +     if (ret < 0)
>> +             return ret;
>> +     val = ret << 8;
>> +
>> +     ret = lidar_read_byte(data, LIDAR_REG_DATA_LBYTE);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     val |= ret;
>> +     *reg = val;
>> +
>> +     return 0;
>> +}
>> +
>> +static int lidar_get_measurement(struct lidar_data *data, u16 *reg)
>> +{
>> +     struct i2c_client *client = data->client;
>> +     int tries = 10;
>> +     int ret;
>> +
>> +     /* start sample */
>> +     ret = lidar_write_control(data, LIDAR_REG_CONTROL_ACQUIRE);
>> +     if (ret < 0) {
>> +             dev_err(&client->dev, "cannot send start measurement command");
>> +             return ret;
>> +     }
>> +
>> +     while (tries--) {
>> +             usleep_range(1000, 2000);
>> +
>> +             ret = lidar_read_byte(data, LIDAR_REG_STATUS);
>> +             if (ret < 0)
>> +                     break;
>> +
>> +             /* return 0 since laser is likely pointed out of range */
>> +             if (ret & LIDAR_REG_STATUS_INVALID) {
>> +                     *reg = 0;
>> +                     ret = 0;
>> +                     break;
>> +             }
>> +
>> +             /* sample ready to read */
>> +             if (!(ret & LIDAR_REG_STATUS_READY)) {
>> +                     ret = lidar_read_measurement(data, reg);
>> +                     break;
>> +             }
>> +             ret = -EIO;
>> +     }
>> +
>> +     return ret;
>> +}
>> +
>> +static int lidar_read_raw(struct iio_dev *indio_dev,
>> +                       struct iio_chan_spec const *chan,
>> +                       int *val, int *val2, long mask)
>> +{
>> +     struct lidar_data *data = iio_priv(indio_dev);
>> +     int ret = -EINVAL;
>> +
>> +     mutex_lock(&data->lock);
>> +
>
> This particular check needs to hold the indio_dev->mlock as that
> is the one that protects core state changes (enabling and disabling the
> buffer).  You want to hang on to that lock until the read is done.

So I guess I can get rid of the data->lock? Since it really isn't used
anywhere else?

>
>> +     if (iio_buffer_enabled(indio_dev) && mask == IIO_CHAN_INFO_RAW) {
>> +             ret = -EBUSY;
>> +             goto error_busy;
>> +     }
>> +
>> +     switch (mask) {
>> +     case IIO_CHAN_INFO_RAW: {
>> +             u16 reg;
>> +
>> +             ret = lidar_get_measurement(data, &reg);
>> +             if (!ret) {
>> +                     *val = reg;
>> +                     ret = IIO_VAL_INT;
>> +             }
>> +             break;
>> +     }
>> +     case IIO_CHAN_INFO_SCALE:
>> +             *val = 0;
>> +             *val2 = 10000;
>> +             ret = IIO_VAL_INT_PLUS_MICRO;
>> +             break;
>> +     }
>> +
>> +error_busy:
>> +     mutex_unlock(&data->lock);
>> +
>> +     return ret;
>> +}
>> +
>> +static irqreturn_t lidar_trigger_handler(int irq, void *private)
>> +{
>> +     struct iio_poll_func *pf = private;
>> +     struct iio_dev *indio_dev = pf->indio_dev;
>> +     struct lidar_data *data = iio_priv(indio_dev);
>> +     int ret;
>> +
>> +     ret = lidar_get_measurement(data, data->buffer);
>> +     if (!ret) {
>> +             iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
>> +                                                iio_get_time_ns());
>> +     } else {
>> +             dev_err(&data->client->dev, "cannot read LIDAR measurement");
>> +     }
>> +
>> +     iio_trigger_notify_done(indio_dev->trig);
>> +
>> +     return IRQ_HANDLED;
>> +}
>> +
>> +static const struct iio_info lidar_info = {
>> +     .driver_module = THIS_MODULE,
>> +     .read_raw = lidar_read_raw,
>> +};
>> +
>> +static int lidar_probe(struct i2c_client *client,
>> +                    const struct i2c_device_id *id)
>> +{
>> +     struct lidar_data *data;
>> +     struct iio_dev *indio_dev;
>> +     int ret;
>> +
>> +     indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>> +     if (!indio_dev)
>> +             return -ENOMEM;
>> +
>> +     indio_dev->info = &lidar_info;
>> +     indio_dev->name = LIDAR_DRV_NAME;
>> +     indio_dev->channels = lidar_channels;
>> +     indio_dev->num_channels = ARRAY_SIZE(lidar_channels);
>> +     indio_dev->modes = INDIO_DIRECT_MODE;
>> +
>> +     data = iio_priv(indio_dev);
>> +     i2c_set_clientdata(client, indio_dev);
>> +
>> +     data->client = client;
>> +     data->indio_dev = indio_dev;
>> +     mutex_init(&data->lock);
>> +
>> +     ret = iio_triggered_buffer_setup(indio_dev, NULL,
>> +                                      lidar_trigger_handler, NULL);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = iio_device_register(indio_dev);
>> +     if (ret)
>> +             goto error_unreg_buffer;
>> +
>> +     return 0;
>> +
>> +error_unreg_buffer:
>> +     iio_triggered_buffer_cleanup(indio_dev);
>> +
>> +     return ret;
>> +}
>> +
>> +static int lidar_remove(struct i2c_client *client)
>> +{
>> +     struct iio_dev *indio_dev = i2c_get_clientdata(client);
>> +
>> +     iio_device_unregister(indio_dev);
>> +     iio_triggered_buffer_cleanup(indio_dev);
>> +
>> +     return 0;
>> +}
>> +
>> +static const struct i2c_device_id lidar_id[] = {
>> +     {"lidar", 0},
>
> This one wants a longer name as well. It's afterall used for
> matching in some circumstances.
>
>> +     { },
>> +};
>> +MODULE_DEVICE_TABLE(i2c, lidar_id);
>> +
>> +static const struct of_device_id lidar_dt_ids[] = {
>> +     { .compatible = "pulsedlight,lidar-lite-v2" },
>> +     { }
>> +};
>> +
>> +static struct i2c_driver lidar_driver = {
>> +     .driver = {
>> +             .name   = LIDAR_DRV_NAME,
>> +             .of_match_table = of_match_ptr(lidar_dt_ids),
>> +     },
>> +     .probe          = lidar_probe,
>> +     .remove         = lidar_remove,
>> +     .id_table       = lidar_id,
>> +};
>> +module_i2c_driver(lidar_driver);
>> +
>> +MODULE_AUTHOR("Matt Ranostay <mranostay@gmail.com>");
>> +MODULE_DESCRIPTION("PulsedLight LIDAR sensor");
>> +MODULE_LICENSE("GPL");
>>
>

[PATCH 0/2] iio: proximity: add PulsedLight LIDAR sensor support

[Matt Ranostay]

PulsedLight "blue label" v2 LIDAR sensor can be polled over 100HZ with
the sysfs + hrtimer iio trigger, or using single measurement raw measurment.

This patchset adds support for this range finding LIDAR module, and reports
distance back in meters to centimeter resolution with +/- 2.5 CM of accuracy
at ~40 meters.

Matt Ranostay (2):
  devicetree: add PulsedLight vendor prefix
  iio: proximity: add support for PulsedLight LIDAR

 .../devicetree/bindings/vendor-prefixes.txt        |   1 +
 drivers/iio/proximity/Kconfig                      |  13 +
 drivers/iio/proximity/Makefile                     |   1 +
 drivers/iio/proximity/lidar.c                      | 309 +++++++++++++++++++++
 4 files changed, 324 insertions(+)
 create mode 100644 drivers/iio/proximity/lidar.c

-- 
1.9.1

[PATCH 2/2] iio: proximity: add support for PulsedLight LIDAR

[Matt Ranostay]

Add support for the PulsedLight LIDAR rangefinder sensor which allows
high speed (over 300Hz) distance measurements using Barker Coding within
40 meter range.

Support only tested on the "blue label" rev 2, but may work using polling
at low sample frequencies on the original version.

Signed-off-by: Matt Ranostay <mranostay@gmail.com>
---
 drivers/iio/proximity/Kconfig  |  13 ++
 drivers/iio/proximity/Makefile |   1 +
 drivers/iio/proximity/lidar.c  | 309 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 323 insertions(+)
 create mode 100644 drivers/iio/proximity/lidar.c

diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
index 41a8d8f..8111e11 100644
--- a/drivers/iio/proximity/Kconfig
+++ b/drivers/iio/proximity/Kconfig
@@ -20,6 +20,19 @@ endmenu
 
 menu "Proximity sensors"
 
+config LIDAR
+	tristate "PulsedLight LIDAR sensor"
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	select REGMAP_I2C
+	depends on I2C
+	help
+	  Say Y to build a driver for PulsedLight LIDAR range finding
+	  sensor.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called lidar.
+
 config SX9500
 	tristate "SX9500 Semtech proximity sensor"
 	select IIO_BUFFER
diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
index 9818dc5..36a95f3 100644
--- a/drivers/iio/proximity/Makefile
+++ b/drivers/iio/proximity/Makefile
@@ -4,4 +4,5 @@
 
 # When adding new entries keep the list in alphabetical order
 obj-$(CONFIG_AS3935)		+= as3935.o
+obj-$(CONFIG_LIDAR)		+= lidar.o
 obj-$(CONFIG_SX9500)		+= sx9500.o
diff --git a/drivers/iio/proximity/lidar.c b/drivers/iio/proximity/lidar.c
new file mode 100644
index 0000000..e56df58
--- /dev/null
+++ b/drivers/iio/proximity/lidar.c
@@ -0,0 +1,309 @@
+/*
+ * lidar.c - Support for PulsedLight LIDAR range finding sensor
+ *
+ * Copyright (C) 2015 Matt Ranostay <mranostay@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * TODO: runtime pm, interrupt mode, and signal strength reporting
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/delay.h>
+#include <linux/mutex.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/triggered_buffer.h>
+
+#define LIDAR_REG_CONTROL	0x00
+#define LIDAR_REG_CONTROL_ACQUIRE	BIT(2)
+
+#define LIDAR_REG_STATUS	0x01
+#define LIDAR_REG_STATUS_RDY	BIT(0)
+
+#define LIDAR_REG_DATA_HBYTE	0x0f
+#define LIDAR_REG_DATA_LBYTE	0x10
+#define LIDAR_REG_DATA_MAX	0xffff
+
+#define LIDAR_DRV_NAME	"lidar"
+
+struct lidar_data {
+	struct mutex lock;
+	struct iio_dev *indio_dev;
+	struct i2c_client *client;
+
+	/* config */
+	int calib_bias;
+
+	u16 buffer[5]; /* 2 byte distance + 8 byte timestamp */
+};
+
+static const struct iio_chan_spec lidar_channels[] = {
+	{
+		.type = IIO_DISTANCE,
+		.info_mask_separate =
+			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_CALIBBIAS),
+		.scan_index = 0,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+		},
+	},
+	IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static inline int lidar_read_byte(struct lidar_data *data, int reg)
+{
+	struct i2c_client *client = data->client;
+	int ret;
+
+	ret = i2c_smbus_write_byte(client, reg);
+	if (ret < 0) {
+		dev_err(&client->dev, "cannot write addr value");
+		return ret;
+	}
+
+	ret = i2c_smbus_read_byte(client);
+	if (ret < 0) {
+		dev_err(&client->dev, "cannot read data value");
+		return ret;
+	}
+
+	return ret;
+}
+
+static inline int lidar_write_control(struct lidar_data *data, int val)
+{
+	return i2c_smbus_write_byte_data(data->client, LIDAR_REG_CONTROL, val);
+}
+
+static int lidar_read_measurement(struct lidar_data *data, u16 *reg)
+{
+	int ret;
+	int val;
+
+	ret = lidar_read_byte(data, LIDAR_REG_DATA_HBYTE);
+	if (ret < 0)
+		return ret;
+	val = ret << 8;
+
+	ret = lidar_read_byte(data, LIDAR_REG_DATA_LBYTE);
+	if (ret < 0)
+		return ret;
+	val |= ret;
+
+	/* correct any possible overflow or underflow */
+	val += data->calib_bias / 10000;
+	if (val < 0)
+		val = 0;
+
+	if (val > LIDAR_REG_DATA_MAX)
+		val = LIDAR_REG_DATA_MAX;
+
+	*reg = val;
+
+	return 0;
+}
+
+static int lidar_get_measurement(struct lidar_data *data, u16 *reg)
+{
+	struct i2c_client *client = data->client;
+	int tries = 10;
+	int ret;
+
+	/* start sample */
+	ret = lidar_write_control(data, LIDAR_REG_CONTROL_ACQUIRE);
+	if (ret < 0) {
+		dev_err(&client->dev, "cannot send start measurement command");
+		return ret;
+	}
+
+	while (tries--) {
+		usleep_range(1000, 2000);
+
+		ret = lidar_read_byte(data, LIDAR_REG_STATUS);
+		if (ret < 0)
+			break;
+
+		/* sample ready to read */
+		if (!(ret & LIDAR_REG_STATUS_RDY)) {
+			ret = lidar_read_measurement(data, reg);
+			break;
+		}
+	}
+
+	return ret;
+}
+
+static int lidar_read_raw(struct iio_dev *indio_dev,
+			  struct iio_chan_spec const *chan,
+			  int *val, int *val2, long mask)
+{
+	struct lidar_data *data = iio_priv(indio_dev);
+	int ret = -EINVAL;
+
+	if (iio_buffer_enabled(indio_dev) && mask == IIO_CHAN_INFO_RAW)
+		return -EBUSY;
+
+	mutex_lock(&data->lock);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW: {
+		u16 reg;
+
+		ret = lidar_get_measurement(data, &reg);
+		if (!ret) {
+			*val = reg / 100;
+			*val2 = (reg % 100) * 10000;
+			ret = IIO_VAL_INT_PLUS_MICRO;
+		}
+		break;
+	}
+	case IIO_CHAN_INFO_CALIBBIAS:
+		*val = 0;
+		*val2 = data->calib_bias;
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+	}
+
+	mutex_unlock(&data->lock);
+
+	return ret;
+}
+
+static int lidar_write_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int val, int val2, long mask)
+{
+	struct lidar_data *data = iio_priv(indio_dev);
+
+	if (mask == IIO_CHAN_INFO_CALIBBIAS) {
+		if (val != 0)
+			return -EINVAL;
+
+		/* cm increments only */
+		if (val2 % 10000)
+			return -EINVAL;
+
+		mutex_lock(&data->lock);
+		data->calib_bias = val2;
+		mutex_unlock(&data->lock);
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static irqreturn_t lidar_trigger_handler(int irq, void *private)
+{
+	struct iio_poll_func *pf = private;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct lidar_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = lidar_get_measurement(data, &data->buffer[0]);
+	if (!ret) {
+		iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
+						   iio_get_time_ns());
+	} else {
+		dev_err(&data->client->dev, "cannot read LIDAR measurement");
+	}
+
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static struct iio_info lidar_info = {
+	.driver_module = THIS_MODULE,
+	.read_raw = lidar_read_raw,
+	.write_raw = lidar_write_raw,
+};
+
+static int lidar_probe(struct i2c_client *client,
+		       const struct i2c_device_id *id)
+{
+	struct lidar_data *data;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	indio_dev->info = &lidar_info;
+	indio_dev->name = LIDAR_DRV_NAME;
+	indio_dev->channels = lidar_channels;
+	indio_dev->num_channels = ARRAY_SIZE(lidar_channels);
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	data = iio_priv(indio_dev);
+	i2c_set_clientdata(client, indio_dev);
+
+	data->client = client;
+	data->indio_dev = indio_dev;
+	mutex_init(&data->lock);
+
+	ret = iio_triggered_buffer_setup(indio_dev, NULL,
+					 lidar_trigger_handler, NULL);
+	if (ret)
+		return ret;
+
+	ret = iio_device_register(indio_dev);
+	if (ret)
+		goto error_unreg_buffer;
+
+	return 0;
+
+error_unreg_buffer:
+	iio_triggered_buffer_cleanup(indio_dev);
+
+	return ret;
+}
+
+static int lidar_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+
+	iio_device_unregister(indio_dev);
+	iio_triggered_buffer_cleanup(indio_dev);
+
+	return 0;
+}
+
+static const struct i2c_device_id lidar_id[] = {
+	{"lidar", 0},
+	{ },
+};
+MODULE_DEVICE_TABLE(i2c, lidar_id);
+
+static struct i2c_driver lidar_driver = {
+	.driver = {
+		.name	= LIDAR_DRV_NAME,
+		.owner	= THIS_MODULE,
+	},
+	.probe		= lidar_probe,
+	.remove		= lidar_remove,
+	.id_table	= lidar_id,
+};
+module_i2c_driver(lidar_driver);
+
+MODULE_AUTHOR("Matt Ranostay <mranostay@gmail.com>");
+MODULE_DESCRIPTION("PulsedLight LIDAR sensor");
+MODULE_LICENSE("GPL");
-- 
1.9.1

Re: [PATCH 2/2] iio: proximity: add support for PulsedLight LIDAR

[Vladimir Barinov]

Hi Matt,

Find minor comments.

On 01.08.2015 06:58, Matt Ranostay wrote:
> Add support for the PulsedLight LIDAR rangefinder sensor which allows
> high speed (over 300Hz) distance measurements using Barker Coding within
> 40 meter range.
>
> Support only tested on the "blue label" rev 2, but may work using polling
> at low sample frequencies on the original version.
>
> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
> ---
>   drivers/iio/proximity/Kconfig  |  13 ++
>   drivers/iio/proximity/Makefile |   1 +
>   drivers/iio/proximity/lidar.c  | 309 +++++++++++++++++++++++++++++++++++++++++
Add ABI documentation 
Documentation/ABI/testing/sysfs-bus-iio-proximity-lidar
>   3 files changed, 323 insertions(+)
>   create mode 100644 drivers/iio/proximity/lidar.c
>
> diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
> index 41a8d8f..8111e11 100644
> --- a/drivers/iio/proximity/Kconfig
> +++ b/drivers/iio/proximity/Kconfig
> @@ -20,6 +20,19 @@ endmenu
>   
>   menu "Proximity sensors"
>   
> +config LIDAR
> +	tristate "PulsedLight LIDAR sensor"
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
> +	select REGMAP_I2C
You did not use REGMAP in your driver.
Probably it would be better to use this API in your driver since the 
chip seems fully SMBUS compatible.
> +	depends on I2C
> +	help
> +	  Say Y to build a driver for PulsedLight LIDAR range finding
> +	  sensor.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called lidar.
> +
>   config SX9500
>   	tristate "SX9500 Semtech proximity sensor"
>   	select IIO_BUFFER
> diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
> index 9818dc5..36a95f3 100644
> --- a/drivers/iio/proximity/Makefile
> +++ b/drivers/iio/proximity/Makefile
> @@ -4,4 +4,5 @@
>   
>   # When adding new entries keep the list in alphabetical order
>   obj-$(CONFIG_AS3935)		+= as3935.o
> +obj-$(CONFIG_LIDAR)		+= lidar.o
>   obj-$(CONFIG_SX9500)		+= sx9500.o
> diff --git a/drivers/iio/proximity/lidar.c b/drivers/iio/proximity/lidar.c
> new file mode 100644
> index 0000000..e56df58
> --- /dev/null
> +++ b/drivers/iio/proximity/lidar.c
> @@ -0,0 +1,309 @@
> +/*
> + * lidar.c - Support for PulsedLight LIDAR range finding sensor
> + *
> + * Copyright (C) 2015 Matt Ranostay <mranostay@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * TODO: runtime pm, interrupt mode, and signal strength reporting
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/delay.h>
> +#include <linux/mutex.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/triggered_buffer.h>
put it in alphabetic order
> +
> +#define LIDAR_REG_CONTROL	0x00
> +#define LIDAR_REG_CONTROL_ACQUIRE	BIT(2)
tab aligned
> +
> +#define LIDAR_REG_STATUS	0x01
> +#define LIDAR_REG_STATUS_RDY	BIT(0)
> +
> +#define LIDAR_REG_DATA_HBYTE	0x0f
> +#define LIDAR_REG_DATA_LBYTE	0x10
> +#define LIDAR_REG_DATA_MAX	0xffff
> +
> +#define LIDAR_DRV_NAME	"lidar"
> +
> +struct lidar_data {
> +	struct mutex lock;
> +	struct iio_dev *indio_dev;
> +	struct i2c_client *client;
> +
> +	/* config */
> +	int calib_bias;
> +
> +	u16 buffer[5]; /* 2 byte distance + 8 byte timestamp */
> +};
> +
> +static const struct iio_chan_spec lidar_channels[] = {
> +	{
> +		.type = IIO_DISTANCE,
> +		.info_mask_separate =
> +			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_CALIBBIAS),
> +		.scan_index = 0,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 16,
> +			.storagebits = 16,
> +		},
> +	},
> +	IIO_CHAN_SOFT_TIMESTAMP(1),
> +};
> +
> +static inline int lidar_read_byte(struct lidar_data *data, int reg)
> +{
> +	struct i2c_client *client = data->client;
> +	int ret;
> +
> +	ret = i2c_smbus_write_byte(client, reg);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "cannot write addr value");
> +		return ret;
> +	}
> +
> +	ret = i2c_smbus_read_byte(client);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "cannot read data value");
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +
> +static inline int lidar_write_control(struct lidar_data *data, int val)
> +{
> +	return i2c_smbus_write_byte_data(data->client, LIDAR_REG_CONTROL, val);
> +}
> +
> +static int lidar_read_measurement(struct lidar_data *data, u16 *reg)
> +{
> +	int ret;
> +	int val;
> +
> +	ret = lidar_read_byte(data, LIDAR_REG_DATA_HBYTE);
> +	if (ret < 0)
> +		return ret;
> +	val = ret << 8;
> +
> +	ret = lidar_read_byte(data, LIDAR_REG_DATA_LBYTE);
> +	if (ret < 0)
> +		return ret;
> +	val |= ret;
> +
> +	/* correct any possible overflow or underflow */
> +	val += data->calib_bias / 10000;
> +	if (val < 0)
> +		val = 0;
> +
> +	if (val > LIDAR_REG_DATA_MAX)
> +		val = LIDAR_REG_DATA_MAX;
> +
> +	*reg = val;
> +
> +	return 0;
> +}
> +
> +static int lidar_get_measurement(struct lidar_data *data, u16 *reg)
> +{
> +	struct i2c_client *client = data->client;
> +	int tries = 10;
> +	int ret;
> +
> +	/* start sample */
> +	ret = lidar_write_control(data, LIDAR_REG_CONTROL_ACQUIRE);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "cannot send start measurement command");
> +		return ret;
> +	}
> +
> +	while (tries--) {
> +		usleep_range(1000, 2000);
> +
> +		ret = lidar_read_byte(data, LIDAR_REG_STATUS);
> +		if (ret < 0)
> +			break;
> +
> +		/* sample ready to read */
> +		if (!(ret & LIDAR_REG_STATUS_RDY)) {
> +			ret = lidar_read_measurement(data, reg);
> +			break;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static int lidar_read_raw(struct iio_dev *indio_dev,
> +			  struct iio_chan_spec const *chan,
> +			  int *val, int *val2, long mask)
> +{
> +	struct lidar_data *data = iio_priv(indio_dev);
> +	int ret = -EINVAL;
> +
> +	if (iio_buffer_enabled(indio_dev) && mask == IIO_CHAN_INFO_RAW)
> +		return -EBUSY;
> +
> +	mutex_lock(&data->lock);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW: {
> +		u16 reg;
> +
> +		ret = lidar_get_measurement(data, &reg);
> +		if (!ret) {
> +			*val = reg / 100;
> +			*val2 = (reg % 100) * 10000;
> +			ret = IIO_VAL_INT_PLUS_MICRO;
> +		}
> +		break;
> +	}
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		*val = 0;
> +		*val2 = data->calib_bias;
> +		ret = IIO_VAL_INT_PLUS_MICRO;
> +		break;
> +	}
> +
> +	mutex_unlock(&data->lock);
> +
> +	return ret;
> +}
> +
> +static int lidar_write_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int val, int val2, long mask)
> +{
> +	struct lidar_data *data = iio_priv(indio_dev);
> +
> +	if (mask == IIO_CHAN_INFO_CALIBBIAS) {
> +		if (val != 0)
> +			return -EINVAL;
> +
> +		/* cm increments only */
> +		if (val2 % 10000)
> +			return -EINVAL;
> +
> +		mutex_lock(&data->lock);
> +		data->calib_bias = val2;
> +		mutex_unlock(&data->lock);
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static irqreturn_t lidar_trigger_handler(int irq, void *private)
> +{
> +	struct iio_poll_func *pf = private;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct lidar_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = lidar_get_measurement(data, &data->buffer[0]);
> +	if (!ret) {
> +		iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
> +						   iio_get_time_ns());
> +	} else {
> +		dev_err(&data->client->dev, "cannot read LIDAR measurement");
> +	}
> +
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static struct iio_info lidar_info = {
> +	.driver_module = THIS_MODULE,
> +	.read_raw = lidar_read_raw,
> +	.write_raw = lidar_write_raw,
> +};
> +
> +static int lidar_probe(struct i2c_client *client,
> +		       const struct i2c_device_id *id)
> +{
> +	struct lidar_data *data;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	indio_dev->info = &lidar_info;
> +	indio_dev->name = LIDAR_DRV_NAME;
> +	indio_dev->channels = lidar_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(lidar_channels);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	data = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);
> +
> +	data->client = client;
> +	data->indio_dev = indio_dev;
> +	mutex_init(&data->lock);
> +
> +	ret = iio_triggered_buffer_setup(indio_dev, NULL,
> +					 lidar_trigger_handler, NULL);
> +	if (ret)
> +		return ret;
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret)
> +		goto error_unreg_buffer;
> +
> +	return 0;
> +
> +error_unreg_buffer:
> +	iio_triggered_buffer_cleanup(indio_dev);
> +
> +	return ret;
> +}
> +
> +static int lidar_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +
> +	iio_device_unregister(indio_dev);
> +	iio_triggered_buffer_cleanup(indio_dev);
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id lidar_id[] = {
> +	{"lidar", 0},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(i2c, lidar_id);
> +
> +static struct i2c_driver lidar_driver = {
> +	.driver = {
> +		.name	= LIDAR_DRV_NAME,
> +		.owner	= THIS_MODULE,
the .owner field is not needed since it is overridden in i2c_register_driver
> +	},
> +	.probe		= lidar_probe,
> +	.remove		= lidar_remove,
> +	.id_table	= lidar_id,
> +};
> +module_i2c_driver(lidar_driver);
> +
> +MODULE_AUTHOR("Matt Ranostay <mranostay@gmail.com>");
> +MODULE_DESCRIPTION("PulsedLight LIDAR sensor");
> +MODULE_LICENSE("GPL");

Re: [PATCH 2/2] iio: proximity: add support for PulsedLight LIDAR

[Jonathan Cameron]

On 1 August 2015 09:39:11 BST, Vladimir Barinov <vladimir.barinov@cogentembedded.com> wrote:
>Hi Matt,
>
>Find minor comments.
>
>On 01.08.2015 06:58, Matt Ranostay wrote:
>> Add support for the PulsedLight LIDAR rangefinder sensor which allows
>> high speed (over 300Hz) distance measurements using Barker Coding
>within
>> 40 meter range.
>>
>> Support only tested on the "blue label" rev 2, but may work using
>polling
>> at low sample frequencies on the original version.
>>
>> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
>> ---
>>   drivers/iio/proximity/Kconfig  |  13 ++
>>   drivers/iio/proximity/Makefile |   1 +
>>   drivers/iio/proximity/lidar.c  | 309
>+++++++++++++++++++++++++++++++++++++++++
>Add ABI documentation 
>Documentation/ABI/testing/sysfs-bus-iio-proximity-lidar
Why?  Reading on phone so may have missed something but all ABI is standard.
Covered by top level docs.

>>   3 files changed, 323 insertions(+)
>>   create mode 100644 drivers/iio/proximity/lidar.c
>>
>> diff --git a/drivers/iio/proximity/Kconfig
>b/drivers/iio/proximity/Kconfig
>> index 41a8d8f..8111e11 100644
>> --- a/drivers/iio/proximity/Kconfig
>> +++ b/drivers/iio/proximity/Kconfig
>> @@ -20,6 +20,19 @@ endmenu
>>   
>>   menu "Proximity sensors"
>>   
>> +config LIDAR
>> +	tristate "PulsedLight LIDAR sensor"
>> +	select IIO_BUFFER
>> +	select IIO_TRIGGERED_BUFFER
>> +	select REGMAP_I2C
>You did not use REGMAP in your driver.
>Probably it would be better to use this API in your driver since the 
>chip seems fully SMBUS compatible.
Unless there is a significant gain don't bother. Now if the device also has an spi bus
or the driver does extensive caching it would be worthwhile.

>> +	depends on I2C
>> +	help
>> +	  Say Y to build a driver for PulsedLight LIDAR range finding
>> +	  sensor.
>> +
>> +	  To compile this driver as a module, choose M here: the
>> +	  module will be called lidar.
>> +
>>   config SX9500
>>   	tristate "SX9500 Semtech proximity sensor"
>>   	select IIO_BUFFER
>> diff --git a/drivers/iio/proximity/Makefile
>b/drivers/iio/proximity/Makefile
>> index 9818dc5..36a95f3 100644
>> --- a/drivers/iio/proximity/Makefile
>> +++ b/drivers/iio/proximity/Makefile
>> @@ -4,4 +4,5 @@
>>   
>>   # When adding new entries keep the list in alphabetical order
>>   obj-$(CONFIG_AS3935)		+= as3935.o
>> +obj-$(CONFIG_LIDAR)		+= lidar.o
>>   obj-$(CONFIG_SX9500)		+= sx9500.o
>> diff --git a/drivers/iio/proximity/lidar.c
>b/drivers/iio/proximity/lidar.c
>> new file mode 100644
>> index 0000000..e56df58
>> --- /dev/null
>> +++ b/drivers/iio/proximity/lidar.c
>> @@ -0,0 +1,309 @@
>> +/*
>> + * lidar.c - Support for PulsedLight LIDAR range finding sensor
>> + *
>> + * Copyright (C) 2015 Matt Ranostay <mranostay@gmail.com>
>> + *
>> + * This program is free software; you can redistribute it and/or
>modify
>> + * it under the terms of the GNU General Public License as published
>by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * TODO: runtime pm, interrupt mode, and signal strength reporting
>> + *
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/init.h>
>> +#include <linux/delay.h>
>> +#include <linux/mutex.h>
>> +#include <linux/err.h>
>> +#include <linux/i2c.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
>> +#include <linux/iio/trigger.h>
>> +#include <linux/iio/trigger_consumer.h>
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/triggered_buffer.h>
>put it in alphabetic order
>> +
>> +#define LIDAR_REG_CONTROL	0x00
>> +#define LIDAR_REG_CONTROL_ACQUIRE	BIT(2)
>tab aligned
>> +
>> +#define LIDAR_REG_STATUS	0x01
>> +#define LIDAR_REG_STATUS_RDY	BIT(0)
>> +
>> +#define LIDAR_REG_DATA_HBYTE	0x0f
>> +#define LIDAR_REG_DATA_LBYTE	0x10
>> +#define LIDAR_REG_DATA_MAX	0xffff
>> +
>> +#define LIDAR_DRV_NAME	"lidar"
>> +
>> +struct lidar_data {
>> +	struct mutex lock;
>> +	struct iio_dev *indio_dev;
>> +	struct i2c_client *client;
>> +
>> +	/* config */
>> +	int calib_bias;
>> +
>> +	u16 buffer[5]; /* 2 byte distance + 8 byte timestamp */
>> +};
>> +
>> +static const struct iio_chan_spec lidar_channels[] = {
>> +	{
>> +		.type = IIO_DISTANCE,
>> +		.info_mask_separate =
>> +			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_CALIBBIAS),
>> +		.scan_index = 0,
>> +		.scan_type = {
>> +			.sign = 'u',
>> +			.realbits = 16,
>> +			.storagebits = 16,
>> +		},
>> +	},
>> +	IIO_CHAN_SOFT_TIMESTAMP(1),
>> +};
>> +
>> +static inline int lidar_read_byte(struct lidar_data *data, int reg)
>> +{
>> +	struct i2c_client *client = data->client;
>> +	int ret;
>> +
>> +	ret = i2c_smbus_write_byte(client, reg);
>> +	if (ret < 0) {
>> +		dev_err(&client->dev, "cannot write addr value");
>> +		return ret;
>> +	}
>> +
>> +	ret = i2c_smbus_read_byte(client);
>> +	if (ret < 0) {
>> +		dev_err(&client->dev, "cannot read data value");
>> +		return ret;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static inline int lidar_write_control(struct lidar_data *data, int
>val)
>> +{
>> +	return i2c_smbus_write_byte_data(data->client, LIDAR_REG_CONTROL,
>val);
>> +}
>> +
>> +static int lidar_read_measurement(struct lidar_data *data, u16 *reg)
>> +{
>> +	int ret;
>> +	int val;
>> +
>> +	ret = lidar_read_byte(data, LIDAR_REG_DATA_HBYTE);
>> +	if (ret < 0)
>> +		return ret;
>> +	val = ret << 8;
>> +
>> +	ret = lidar_read_byte(data, LIDAR_REG_DATA_LBYTE);
>> +	if (ret < 0)
>> +		return ret;
>> +	val |= ret;
>> +
>> +	/* correct any possible overflow or underflow */
>> +	val += data->calib_bias / 10000;
>> +	if (val < 0)
>> +		val = 0;
>> +
>> +	if (val > LIDAR_REG_DATA_MAX)
>> +		val = LIDAR_REG_DATA_MAX;
>> +
>> +	*reg = val;
>> +
>> +	return 0;
>> +}
>> +
>> +static int lidar_get_measurement(struct lidar_data *data, u16 *reg)
>> +{
>> +	struct i2c_client *client = data->client;
>> +	int tries = 10;
>> +	int ret;
>> +
>> +	/* start sample */
>> +	ret = lidar_write_control(data, LIDAR_REG_CONTROL_ACQUIRE);
>> +	if (ret < 0) {
>> +		dev_err(&client->dev, "cannot send start measurement command");
>> +		return ret;
>> +	}
>> +
>> +	while (tries--) {
>> +		usleep_range(1000, 2000);
>> +
>> +		ret = lidar_read_byte(data, LIDAR_REG_STATUS);
>> +		if (ret < 0)
>> +			break;
>> +
>> +		/* sample ready to read */
>> +		if (!(ret & LIDAR_REG_STATUS_RDY)) {
>> +			ret = lidar_read_measurement(data, reg);
>> +			break;
>> +		}
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int lidar_read_raw(struct iio_dev *indio_dev,
>> +			  struct iio_chan_spec const *chan,
>> +			  int *val, int *val2, long mask)
>> +{
>> +	struct lidar_data *data = iio_priv(indio_dev);
>> +	int ret = -EINVAL;
>> +
>> +	if (iio_buffer_enabled(indio_dev) && mask == IIO_CHAN_INFO_RAW)
>> +		return -EBUSY;
>> +
>> +	mutex_lock(&data->lock);
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW: {
>> +		u16 reg;
>> +
>> +		ret = lidar_get_measurement(data, &reg);
>> +		if (!ret) {
>> +			*val = reg / 100;
>> +			*val2 = (reg % 100) * 10000;
>> +			ret = IIO_VAL_INT_PLUS_MICRO;
>> +		}
>> +		break;
>> +	}
>> +	case IIO_CHAN_INFO_CALIBBIAS:
>> +		*val = 0;
>> +		*val2 = data->calib_bias;
>> +		ret = IIO_VAL_INT_PLUS_MICRO;
>> +		break;
>> +	}
>> +
>> +	mutex_unlock(&data->lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static int lidar_write_raw(struct iio_dev *indio_dev,
>> +			   struct iio_chan_spec const *chan,
>> +			   int val, int val2, long mask)
>> +{
>> +	struct lidar_data *data = iio_priv(indio_dev);
>> +
>> +	if (mask == IIO_CHAN_INFO_CALIBBIAS) {
>> +		if (val != 0)
>> +			return -EINVAL;
>> +
>> +		/* cm increments only */
>> +		if (val2 % 10000)
>> +			return -EINVAL;
>> +
>> +		mutex_lock(&data->lock);
>> +		data->calib_bias = val2;
>> +		mutex_unlock(&data->lock);
>> +		return 0;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static irqreturn_t lidar_trigger_handler(int irq, void *private)
>> +{
>> +	struct iio_poll_func *pf = private;
>> +	struct iio_dev *indio_dev = pf->indio_dev;
>> +	struct lidar_data *data = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	ret = lidar_get_measurement(data, &data->buffer[0]);
>> +	if (!ret) {
>> +		iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
>> +						   iio_get_time_ns());
>> +	} else {
>> +		dev_err(&data->client->dev, "cannot read LIDAR measurement");
>> +	}
>> +
>> +	iio_trigger_notify_done(indio_dev->trig);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static struct iio_info lidar_info = {
>> +	.driver_module = THIS_MODULE,
>> +	.read_raw = lidar_read_raw,
>> +	.write_raw = lidar_write_raw,
>> +};
>> +
>> +static int lidar_probe(struct i2c_client *client,
>> +		       const struct i2c_device_id *id)
>> +{
>> +	struct lidar_data *data;
>> +	struct iio_dev *indio_dev;
>> +	int ret;
>> +
>> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>> +	if (!indio_dev)
>> +		return -ENOMEM;
>> +
>> +	indio_dev->info = &lidar_info;
>> +	indio_dev->name = LIDAR_DRV_NAME;
>> +	indio_dev->channels = lidar_channels;
>> +	indio_dev->num_channels = ARRAY_SIZE(lidar_channels);
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +
>> +	data = iio_priv(indio_dev);
>> +	i2c_set_clientdata(client, indio_dev);
>> +
>> +	data->client = client;
>> +	data->indio_dev = indio_dev;
>> +	mutex_init(&data->lock);
>> +
>> +	ret = iio_triggered_buffer_setup(indio_dev, NULL,
>> +					 lidar_trigger_handler, NULL);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = iio_device_register(indio_dev);
>> +	if (ret)
>> +		goto error_unreg_buffer;
>> +
>> +	return 0;
>> +
>> +error_unreg_buffer:
>> +	iio_triggered_buffer_cleanup(indio_dev);
>> +
>> +	return ret;
>> +}
>> +
>> +static int lidar_remove(struct i2c_client *client)
>> +{
>> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
>> +
>> +	iio_device_unregister(indio_dev);
>> +	iio_triggered_buffer_cleanup(indio_dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct i2c_device_id lidar_id[] = {
>> +	{"lidar", 0},
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(i2c, lidar_id);
>> +
>> +static struct i2c_driver lidar_driver = {
>> +	.driver = {
>> +		.name	= LIDAR_DRV_NAME,
>> +		.owner	= THIS_MODULE,
>the .owner field is not needed since it is overridden in
>i2c_register_driver
>> +	},
>> +	.probe		= lidar_probe,
>> +	.remove		= lidar_remove,
>> +	.id_table	= lidar_id,
>> +};
>> +module_i2c_driver(lidar_driver);
>> +
>> +MODULE_AUTHOR("Matt Ranostay <mranostay@gmail.com>");
>> +MODULE_DESCRIPTION("PulsedLight LIDAR sensor");
>> +MODULE_LICENSE("GPL");
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 2/2] iio: proximity: add support for PulsedLight LIDAR

[Matt Ranostay]

On Sat, Aug 1, 2015 at 2:17 PM, Jonathan Cameron
<jic23@jic23.retrosnub.co.uk> wrote:
>
>
>
> On 1 August 2015 09:39:11 BST, Vladimir Barinov <vladimir.barinov@cogentembedded.com> wrote:
>>Hi Matt,
>>
>>Find minor comments.
>>
>>On 01.08.2015 06:58, Matt Ranostay wrote:
>>> Add support for the PulsedLight LIDAR rangefinder sensor which allows
>>> high speed (over 300Hz) distance measurements using Barker Coding
>>within
>>> 40 meter range.
>>>
>>> Support only tested on the "blue label" rev 2, but may work using
>>polling
>>> at low sample frequencies on the original version.
>>>
>>> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
>>> ---
>>>   drivers/iio/proximity/Kconfig  |  13 ++
>>>   drivers/iio/proximity/Makefile |   1 +
>>>   drivers/iio/proximity/lidar.c  | 309
>>+++++++++++++++++++++++++++++++++++++++++
>>Add ABI documentation
>>Documentation/ABI/testing/sysfs-bus-iio-proximity-lidar
> Why?  Reading on phone so may have missed something but all ABI is standard.
> Covered by top level docs.

Only thing that would be documented that IIO_DISTANCE is in meters.
But seems standard.
>
>>>   3 files changed, 323 insertions(+)
>>>   create mode 100644 drivers/iio/proximity/lidar.c
>>>
>>> diff --git a/drivers/iio/proximity/Kconfig
>>b/drivers/iio/proximity/Kconfig
>>> index 41a8d8f..8111e11 100644
>>> --- a/drivers/iio/proximity/Kconfig
>>> +++ b/drivers/iio/proximity/Kconfig
>>> @@ -20,6 +20,19 @@ endmenu
>>>
>>>   menu "Proximity sensors"
>>>
>>> +config LIDAR
>>> +    tristate "PulsedLight LIDAR sensor"
>>> +    select IIO_BUFFER
>>> +    select IIO_TRIGGERED_BUFFER
>>> +    select REGMAP_I2C
>>You did not use REGMAP in your driver.
>>Probably it would be better to use this API in your driver since the
>>chip seems fully SMBUS compatible.
> Unless there is a significant gain don't bother. Now if the device also has an spi bus
> or the driver does extensive caching it would be worthwhile.

Correctly, none of the the data is cacheable so regmap doesn't make sense here.

>
>>> +    depends on I2C
>>> +    help
>>> +      Say Y to build a driver for PulsedLight LIDAR range finding
>>> +      sensor.
>>> +
>>> +      To compile this driver as a module, choose M here: the
>>> +      module will be called lidar.
>>> +
>>>   config SX9500
>>>      tristate "SX9500 Semtech proximity sensor"
>>>      select IIO_BUFFER
>>> diff --git a/drivers/iio/proximity/Makefile
>>b/drivers/iio/proximity/Makefile
>>> index 9818dc5..36a95f3 100644
>>> --- a/drivers/iio/proximity/Makefile
>>> +++ b/drivers/iio/proximity/Makefile
>>> @@ -4,4 +4,5 @@
>>>
>>>   # When adding new entries keep the list in alphabetical order
>>>   obj-$(CONFIG_AS3935)               += as3935.o
>>> +obj-$(CONFIG_LIDAR)         += lidar.o
>>>   obj-$(CONFIG_SX9500)               += sx9500.o
>>> diff --git a/drivers/iio/proximity/lidar.c
>>b/drivers/iio/proximity/lidar.c
>>> new file mode 100644
>>> index 0000000..e56df58
>>> --- /dev/null
>>> +++ b/drivers/iio/proximity/lidar.c
>>> @@ -0,0 +1,309 @@
>>> +/*
>>> + * lidar.c - Support for PulsedLight LIDAR range finding sensor
>>> + *
>>> + * Copyright (C) 2015 Matt Ranostay <mranostay@gmail.com>
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>modify
>>> + * it under the terms of the GNU General Public License as published
>>by
>>> + * the Free Software Foundation; either version 2 of the License, or
>>> + * (at your option) any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + *
>>> + * TODO: runtime pm, interrupt mode, and signal strength reporting
>>> + *
>>> + */
>>> +
>>> +#include <linux/module.h>
>>> +#include <linux/init.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/mutex.h>
>>> +#include <linux/err.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/iio/sysfs.h>
>>> +#include <linux/iio/trigger.h>
>>> +#include <linux/iio/trigger_consumer.h>
>>> +#include <linux/iio/buffer.h>
>>> +#include <linux/iio/triggered_buffer.h>
>>put it in alphabetic order
>>> +
>>> +#define LIDAR_REG_CONTROL   0x00
>>> +#define LIDAR_REG_CONTROL_ACQUIRE   BIT(2)
>>tab aligned
>>> +
>>> +#define LIDAR_REG_STATUS    0x01
>>> +#define LIDAR_REG_STATUS_RDY        BIT(0)
>>> +
>>> +#define LIDAR_REG_DATA_HBYTE        0x0f
>>> +#define LIDAR_REG_DATA_LBYTE        0x10
>>> +#define LIDAR_REG_DATA_MAX  0xffff
>>> +
>>> +#define LIDAR_DRV_NAME      "lidar"
>>> +
>>> +struct lidar_data {
>>> +    struct mutex lock;
>>> +    struct iio_dev *indio_dev;
>>> +    struct i2c_client *client;
>>> +
>>> +    /* config */
>>> +    int calib_bias;
>>> +
>>> +    u16 buffer[5]; /* 2 byte distance + 8 byte timestamp */
>>> +};
>>> +
>>> +static const struct iio_chan_spec lidar_channels[] = {
>>> +    {
>>> +            .type = IIO_DISTANCE,
>>> +            .info_mask_separate =
>>> +                    BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_CALIBBIAS),
>>> +            .scan_index = 0,
>>> +            .scan_type = {
>>> +                    .sign = 'u',
>>> +                    .realbits = 16,
>>> +                    .storagebits = 16,
>>> +            },
>>> +    },
>>> +    IIO_CHAN_SOFT_TIMESTAMP(1),
>>> +};
>>> +
>>> +static inline int lidar_read_byte(struct lidar_data *data, int reg)
>>> +{
>>> +    struct i2c_client *client = data->client;
>>> +    int ret;
>>> +
>>> +    ret = i2c_smbus_write_byte(client, reg);
>>> +    if (ret < 0) {
>>> +            dev_err(&client->dev, "cannot write addr value");
>>> +            return ret;
>>> +    }
>>> +
>>> +    ret = i2c_smbus_read_byte(client);
>>> +    if (ret < 0) {
>>> +            dev_err(&client->dev, "cannot read data value");
>>> +            return ret;
>>> +    }
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static inline int lidar_write_control(struct lidar_data *data, int
>>val)
>>> +{
>>> +    return i2c_smbus_write_byte_data(data->client, LIDAR_REG_CONTROL,
>>val);
>>> +}
>>> +
>>> +static int lidar_read_measurement(struct lidar_data *data, u16 *reg)
>>> +{
>>> +    int ret;
>>> +    int val;
>>> +
>>> +    ret = lidar_read_byte(data, LIDAR_REG_DATA_HBYTE);
>>> +    if (ret < 0)
>>> +            return ret;
>>> +    val = ret << 8;
>>> +
>>> +    ret = lidar_read_byte(data, LIDAR_REG_DATA_LBYTE);
>>> +    if (ret < 0)
>>> +            return ret;
>>> +    val |= ret;
>>> +
>>> +    /* correct any possible overflow or underflow */
>>> +    val += data->calib_bias / 10000;
>>> +    if (val < 0)
>>> +            val = 0;
>>> +
>>> +    if (val > LIDAR_REG_DATA_MAX)
>>> +            val = LIDAR_REG_DATA_MAX;
>>> +
>>> +    *reg = val;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int lidar_get_measurement(struct lidar_data *data, u16 *reg)
>>> +{
>>> +    struct i2c_client *client = data->client;
>>> +    int tries = 10;
>>> +    int ret;
>>> +
>>> +    /* start sample */
>>> +    ret = lidar_write_control(data, LIDAR_REG_CONTROL_ACQUIRE);
>>> +    if (ret < 0) {
>>> +            dev_err(&client->dev, "cannot send start measurement command");
>>> +            return ret;
>>> +    }
>>> +
>>> +    while (tries--) {
>>> +            usleep_range(1000, 2000);
>>> +
>>> +            ret = lidar_read_byte(data, LIDAR_REG_STATUS);
>>> +            if (ret < 0)
>>> +                    break;
>>> +
>>> +            /* sample ready to read */
>>> +            if (!(ret & LIDAR_REG_STATUS_RDY)) {
>>> +                    ret = lidar_read_measurement(data, reg);
>>> +                    break;
>>> +            }
>>> +    }
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static int lidar_read_raw(struct iio_dev *indio_dev,
>>> +                      struct iio_chan_spec const *chan,
>>> +                      int *val, int *val2, long mask)
>>> +{
>>> +    struct lidar_data *data = iio_priv(indio_dev);
>>> +    int ret = -EINVAL;
>>> +
>>> +    if (iio_buffer_enabled(indio_dev) && mask == IIO_CHAN_INFO_RAW)
>>> +            return -EBUSY;
>>> +
>>> +    mutex_lock(&data->lock);
>>> +
>>> +    switch (mask) {
>>> +    case IIO_CHAN_INFO_RAW: {
>>> +            u16 reg;
>>> +
>>> +            ret = lidar_get_measurement(data, &reg);
>>> +            if (!ret) {
>>> +                    *val = reg / 100;
>>> +                    *val2 = (reg % 100) * 10000;
>>> +                    ret = IIO_VAL_INT_PLUS_MICRO;
>>> +            }
>>> +            break;
>>> +    }
>>> +    case IIO_CHAN_INFO_CALIBBIAS:
>>> +            *val = 0;
>>> +            *val2 = data->calib_bias;
>>> +            ret = IIO_VAL_INT_PLUS_MICRO;
>>> +            break;
>>> +    }
>>> +
>>> +    mutex_unlock(&data->lock);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static int lidar_write_raw(struct iio_dev *indio_dev,
>>> +                       struct iio_chan_spec const *chan,
>>> +                       int val, int val2, long mask)
>>> +{
>>> +    struct lidar_data *data = iio_priv(indio_dev);
>>> +
>>> +    if (mask == IIO_CHAN_INFO_CALIBBIAS) {
>>> +            if (val != 0)
>>> +                    return -EINVAL;
>>> +
>>> +            /* cm increments only */
>>> +            if (val2 % 10000)
>>> +                    return -EINVAL;
>>> +
>>> +            mutex_lock(&data->lock);
>>> +            data->calib_bias = val2;
>>> +            mutex_unlock(&data->lock);
>>> +            return 0;
>>> +    }
>>> +
>>> +    return -EINVAL;
>>> +}
>>> +
>>> +static irqreturn_t lidar_trigger_handler(int irq, void *private)
>>> +{
>>> +    struct iio_poll_func *pf = private;
>>> +    struct iio_dev *indio_dev = pf->indio_dev;
>>> +    struct lidar_data *data = iio_priv(indio_dev);
>>> +    int ret;
>>> +
>>> +    ret = lidar_get_measurement(data, &data->buffer[0]);
>>> +    if (!ret) {
>>> +            iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
>>> +                                               iio_get_time_ns());
>>> +    } else {
>>> +            dev_err(&data->client->dev, "cannot read LIDAR measurement");
>>> +    }
>>> +
>>> +    iio_trigger_notify_done(indio_dev->trig);
>>> +
>>> +    return IRQ_HANDLED;
>>> +}
>>> +
>>> +static struct iio_info lidar_info = {
>>> +    .driver_module = THIS_MODULE,
>>> +    .read_raw = lidar_read_raw,
>>> +    .write_raw = lidar_write_raw,
>>> +};
>>> +
>>> +static int lidar_probe(struct i2c_client *client,
>>> +                   const struct i2c_device_id *id)
>>> +{
>>> +    struct lidar_data *data;
>>> +    struct iio_dev *indio_dev;
>>> +    int ret;
>>> +
>>> +    indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>>> +    if (!indio_dev)
>>> +            return -ENOMEM;
>>> +
>>> +    indio_dev->info = &lidar_info;
>>> +    indio_dev->name = LIDAR_DRV_NAME;
>>> +    indio_dev->channels = lidar_channels;
>>> +    indio_dev->num_channels = ARRAY_SIZE(lidar_channels);
>>> +    indio_dev->modes = INDIO_DIRECT_MODE;
>>> +
>>> +    data = iio_priv(indio_dev);
>>> +    i2c_set_clientdata(client, indio_dev);
>>> +
>>> +    data->client = client;
>>> +    data->indio_dev = indio_dev;
>>> +    mutex_init(&data->lock);
>>> +
>>> +    ret = iio_triggered_buffer_setup(indio_dev, NULL,
>>> +                                     lidar_trigger_handler, NULL);
>>> +    if (ret)
>>> +            return ret;
>>> +
>>> +    ret = iio_device_register(indio_dev);
>>> +    if (ret)
>>> +            goto error_unreg_buffer;
>>> +
>>> +    return 0;
>>> +
>>> +error_unreg_buffer:
>>> +    iio_triggered_buffer_cleanup(indio_dev);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static int lidar_remove(struct i2c_client *client)
>>> +{
>>> +    struct iio_dev *indio_dev = i2c_get_clientdata(client);
>>> +
>>> +    iio_device_unregister(indio_dev);
>>> +    iio_triggered_buffer_cleanup(indio_dev);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static const struct i2c_device_id lidar_id[] = {
>>> +    {"lidar", 0},
>>> +    { },
>>> +};
>>> +MODULE_DEVICE_TABLE(i2c, lidar_id);
>>> +
>>> +static struct i2c_driver lidar_driver = {
>>> +    .driver = {
>>> +            .name   = LIDAR_DRV_NAME,
>>> +            .owner  = THIS_MODULE,
>>the .owner field is not needed since it is overridden in
>>i2c_register_driver
>>> +    },
>>> +    .probe          = lidar_probe,
>>> +    .remove         = lidar_remove,
>>> +    .id_table       = lidar_id,
>>> +};
>>> +module_i2c_driver(lidar_driver);
>>> +
>>> +MODULE_AUTHOR("Matt Ranostay <mranostay@gmail.com>");
>>> +MODULE_DESCRIPTION("PulsedLight LIDAR sensor");
>>> +MODULE_LICENSE("GPL");
>>
>>--
>>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>the body of a message to majordomo@vger.kernel.org
>>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> --
> Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 2/2] iio: proximity: add support for PulsedLight LIDAR

[Vladimir Barinov]

On 02.08.2015 01:22, Matt Ranostay wrote:
> On Sat, Aug 1, 2015 at 2:17 PM, Jonathan Cameron
> <jic23@jic23.retrosnub.co.uk> wrote:
>>
>>
>> On 1 August 2015 09:39:11 BST, Vladimir Barinov <vladimir.barinov@cogentembedded.com> wrote:
>>> Hi Matt,
>>>
>>> Find minor comments.
>>>
>>> On 01.08.2015 06:58, Matt Ranostay wrote:
>>>> Add support for the PulsedLight LIDAR rangefinder sensor which allows
>>>> high speed (over 300Hz) distance measurements using Barker Coding
>>> within
>>>> 40 meter range.
>>>>
>>>> Support only tested on the "blue label" rev 2, but may work using
>>> polling
>>>> at low sample frequencies on the original version.
>>>>
>>>> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
>>>> ---
>>>>    drivers/iio/proximity/Kconfig  |  13 ++
>>>>    drivers/iio/proximity/Makefile |   1 +
>>>>    drivers/iio/proximity/lidar.c  | 309
>>> +++++++++++++++++++++++++++++++++++++++++
>>> Add ABI documentation
>>> Documentation/ABI/testing/sysfs-bus-iio-proximity-lidar
>> Why?  Reading on phone so may have missed something but all ABI is standard.
>> Covered by top level docs.
> Only thing that would be documented that IIO_DISTANCE is in meters.
> But seems standard.
I understand.
I was thinking that reading in_distance_raw units needs to be docimented 
(looking on the sample sysfs-bus-iio-proximity-as3935)

Regards,
Vladimir

Re: [PATCH 2/2] iio: proximity: add support for PulsedLight LIDAR

[Jonathan Cameron]

On 01/08/15 23:22, Matt Ranostay wrote:
> On Sat, Aug 1, 2015 at 2:17 PM, Jonathan Cameron
> <jic23@jic23.retrosnub.co.uk> wrote:
>>
>>
>>
>> On 1 August 2015 09:39:11 BST, Vladimir Barinov <vladimir.barinov@cogentembedded.com> wrote:
>>> Hi Matt,
>>>
>>> Find minor comments.
>>>
>>> On 01.08.2015 06:58, Matt Ranostay wrote:
>>>> Add support for the PulsedLight LIDAR rangefinder sensor which allows
>>>> high speed (over 300Hz) distance measurements using Barker Coding
>>> within
>>>> 40 meter range.
>>>>
>>>> Support only tested on the "blue label" rev 2, but may work using
>>> polling
>>>> at low sample frequencies on the original version.
>>>>
>>>> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
>>>> ---
>>>>   drivers/iio/proximity/Kconfig  |  13 ++
>>>>   drivers/iio/proximity/Makefile |   1 +
>>>>   drivers/iio/proximity/lidar.c  | 309
>>> +++++++++++++++++++++++++++++++++++++++++
>>> Add ABI documentation
>>> Documentation/ABI/testing/sysfs-bus-iio-proximity-lidar
>> Why?  Reading on phone so may have missed something but all ABI is standard.
>> Covered by top level docs.
> 
> Only thing that would be documented that IIO_DISTANCE is in meters.
> But seems standard.
This is the perfect point to move that out to the top level docs
(two drivers using it now).
>>
>>>>   3 files changed, 323 insertions(+)
>>>>   create mode 100644 drivers/iio/proximity/lidar.c
>>>>
>>>> diff --git a/drivers/iio/proximity/Kconfig
>>> b/drivers/iio/proximity/Kconfig
>>>> index 41a8d8f..8111e11 100644
>>>> --- a/drivers/iio/proximity/Kconfig
>>>> +++ b/drivers/iio/proximity/Kconfig
>>>> @@ -20,6 +20,19 @@ endmenu
>>>>
>>>>   menu "Proximity sensors"
>>>>
>>>> +config LIDAR
>>>> +    tristate "PulsedLight LIDAR sensor"
>>>> +    select IIO_BUFFER
>>>> +    select IIO_TRIGGERED_BUFFER
>>>> +    select REGMAP_I2C
>>> You did not use REGMAP in your driver.
>>> Probably it would be better to use this API in your driver since the
>>> chip seems fully SMBUS compatible.
>> Unless there is a significant gain don't bother. Now if the device also has an spi bus
>> or the driver does extensive caching it would be worthwhile.
> 
> Correctly, none of the the data is cacheable so regmap doesn't make sense here.
> 
>>
>>>> +    depends on I2C
>>>> +    help
>>>> +      Say Y to build a driver for PulsedLight LIDAR range finding
>>>> +      sensor.
>>>> +
>>>> +      To compile this driver as a module, choose M here: the
>>>> +      module will be called lidar.
>>>> +
>>>>   config SX9500
>>>>      tristate "SX9500 Semtech proximity sensor"
>>>>      select IIO_BUFFER
>>>> diff --git a/drivers/iio/proximity/Makefile
>>> b/drivers/iio/proximity/Makefile
>>>> index 9818dc5..36a95f3 100644
>>>> --- a/drivers/iio/proximity/Makefile
>>>> +++ b/drivers/iio/proximity/Makefile
>>>> @@ -4,4 +4,5 @@
>>>>
>>>>   # When adding new entries keep the list in alphabetical order
>>>>   obj-$(CONFIG_AS3935)               += as3935.o
>>>> +obj-$(CONFIG_LIDAR)         += lidar.o
>>>>   obj-$(CONFIG_SX9500)               += sx9500.o
>>>> diff --git a/drivers/iio/proximity/lidar.c
>>> b/drivers/iio/proximity/lidar.c
>>>> new file mode 100644
>>>> index 0000000..e56df58
>>>> --- /dev/null
>>>> +++ b/drivers/iio/proximity/lidar.c
>>>> @@ -0,0 +1,309 @@
>>>> +/*
>>>> + * lidar.c - Support for PulsedLight LIDAR range finding sensor
>>>> + *
>>>> + * Copyright (C) 2015 Matt Ranostay <mranostay@gmail.com>
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or
>>> modify
>>>> + * it under the terms of the GNU General Public License as published
>>> by
>>>> + * the Free Software Foundation; either version 2 of the License, or
>>>> + * (at your option) any later version.
>>>> + *
>>>> + * This program is distributed in the hope that it will be useful,
>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>>> + * GNU General Public License for more details.
>>>> + *
>>>> + * TODO: runtime pm, interrupt mode, and signal strength reporting
>>>> + *
>>>> + */
>>>> +
>>>> +#include <linux/module.h>
>>>> +#include <linux/init.h>
>>>> +#include <linux/delay.h>
>>>> +#include <linux/mutex.h>
>>>> +#include <linux/err.h>
>>>> +#include <linux/i2c.h>
>>>> +#include <linux/iio/iio.h>
>>>> +#include <linux/iio/sysfs.h>
>>>> +#include <linux/iio/trigger.h>
>>>> +#include <linux/iio/trigger_consumer.h>
>>>> +#include <linux/iio/buffer.h>
>>>> +#include <linux/iio/triggered_buffer.h>
>>> put it in alphabetic order
>>>> +
>>>> +#define LIDAR_REG_CONTROL   0x00
>>>> +#define LIDAR_REG_CONTROL_ACQUIRE   BIT(2)
>>> tab aligned
>>>> +
>>>> +#define LIDAR_REG_STATUS    0x01
>>>> +#define LIDAR_REG_STATUS_RDY        BIT(0)
>>>> +
>>>> +#define LIDAR_REG_DATA_HBYTE        0x0f
>>>> +#define LIDAR_REG_DATA_LBYTE        0x10
>>>> +#define LIDAR_REG_DATA_MAX  0xffff
>>>> +
>>>> +#define LIDAR_DRV_NAME      "lidar"
>>>> +
>>>> +struct lidar_data {
>>>> +    struct mutex lock;
>>>> +    struct iio_dev *indio_dev;
>>>> +    struct i2c_client *client;
>>>> +
>>>> +    /* config */
>>>> +    int calib_bias;
>>>> +
>>>> +    u16 buffer[5]; /* 2 byte distance + 8 byte timestamp */
>>>> +};
>>>> +
>>>> +static const struct iio_chan_spec lidar_channels[] = {
>>>> +    {
>>>> +            .type = IIO_DISTANCE,
>>>> +            .info_mask_separate =
>>>> +                    BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_CALIBBIAS),
>>>> +            .scan_index = 0,
>>>> +            .scan_type = {
>>>> +                    .sign = 'u',
>>>> +                    .realbits = 16,
>>>> +                    .storagebits = 16,
>>>> +            },
>>>> +    },
>>>> +    IIO_CHAN_SOFT_TIMESTAMP(1),
>>>> +};
>>>> +
>>>> +static inline int lidar_read_byte(struct lidar_data *data, int reg)
>>>> +{
>>>> +    struct i2c_client *client = data->client;
>>>> +    int ret;
>>>> +
>>>> +    ret = i2c_smbus_write_byte(client, reg);
>>>> +    if (ret < 0) {
>>>> +            dev_err(&client->dev, "cannot write addr value");
>>>> +            return ret;
>>>> +    }
>>>> +
>>>> +    ret = i2c_smbus_read_byte(client);
>>>> +    if (ret < 0) {
>>>> +            dev_err(&client->dev, "cannot read data value");
>>>> +            return ret;
>>>> +    }
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static inline int lidar_write_control(struct lidar_data *data, int
>>> val)
>>>> +{
>>>> +    return i2c_smbus_write_byte_data(data->client, LIDAR_REG_CONTROL,
>>> val);
>>>> +}
>>>> +
>>>> +static int lidar_read_measurement(struct lidar_data *data, u16 *reg)
>>>> +{
>>>> +    int ret;
>>>> +    int val;
>>>> +
>>>> +    ret = lidar_read_byte(data, LIDAR_REG_DATA_HBYTE);
>>>> +    if (ret < 0)
>>>> +            return ret;
>>>> +    val = ret << 8;
>>>> +
>>>> +    ret = lidar_read_byte(data, LIDAR_REG_DATA_LBYTE);
>>>> +    if (ret < 0)
>>>> +            return ret;
>>>> +    val |= ret;
>>>> +
>>>> +    /* correct any possible overflow or underflow */
>>>> +    val += data->calib_bias / 10000;
>>>> +    if (val < 0)
>>>> +            val = 0;
>>>> +
>>>> +    if (val > LIDAR_REG_DATA_MAX)
>>>> +            val = LIDAR_REG_DATA_MAX;
>>>> +
>>>> +    *reg = val;
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int lidar_get_measurement(struct lidar_data *data, u16 *reg)
>>>> +{
>>>> +    struct i2c_client *client = data->client;
>>>> +    int tries = 10;
>>>> +    int ret;
>>>> +
>>>> +    /* start sample */
>>>> +    ret = lidar_write_control(data, LIDAR_REG_CONTROL_ACQUIRE);
>>>> +    if (ret < 0) {
>>>> +            dev_err(&client->dev, "cannot send start measurement command");
>>>> +            return ret;
>>>> +    }
>>>> +
>>>> +    while (tries--) {
>>>> +            usleep_range(1000, 2000);
>>>> +
>>>> +            ret = lidar_read_byte(data, LIDAR_REG_STATUS);
>>>> +            if (ret < 0)
>>>> +                    break;
>>>> +
>>>> +            /* sample ready to read */
>>>> +            if (!(ret & LIDAR_REG_STATUS_RDY)) {
>>>> +                    ret = lidar_read_measurement(data, reg);
>>>> +                    break;
>>>> +            }
>>>> +    }
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static int lidar_read_raw(struct iio_dev *indio_dev,
>>>> +                      struct iio_chan_spec const *chan,
>>>> +                      int *val, int *val2, long mask)
>>>> +{
>>>> +    struct lidar_data *data = iio_priv(indio_dev);
>>>> +    int ret = -EINVAL;
>>>> +
>>>> +    if (iio_buffer_enabled(indio_dev) && mask == IIO_CHAN_INFO_RAW)
>>>> +            return -EBUSY;
>>>> +
>>>> +    mutex_lock(&data->lock);
>>>> +
>>>> +    switch (mask) {
>>>> +    case IIO_CHAN_INFO_RAW: {
>>>> +            u16 reg;
>>>> +
>>>> +            ret = lidar_get_measurement(data, &reg);
>>>> +            if (!ret) {
>>>> +                    *val = reg / 100;
>>>> +                    *val2 = (reg % 100) * 10000;
>>>> +                    ret = IIO_VAL_INT_PLUS_MICRO;
>>>> +            }
>>>> +            break;
>>>> +    }
>>>> +    case IIO_CHAN_INFO_CALIBBIAS:
>>>> +            *val = 0;
>>>> +            *val2 = data->calib_bias;
>>>> +            ret = IIO_VAL_INT_PLUS_MICRO;
>>>> +            break;
>>>> +    }
>>>> +
>>>> +    mutex_unlock(&data->lock);
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static int lidar_write_raw(struct iio_dev *indio_dev,
>>>> +                       struct iio_chan_spec const *chan,
>>>> +                       int val, int val2, long mask)
>>>> +{
>>>> +    struct lidar_data *data = iio_priv(indio_dev);
>>>> +
>>>> +    if (mask == IIO_CHAN_INFO_CALIBBIAS) {
>>>> +            if (val != 0)
>>>> +                    return -EINVAL;
>>>> +
>>>> +            /* cm increments only */
>>>> +            if (val2 % 10000)
>>>> +                    return -EINVAL;
>>>> +
>>>> +            mutex_lock(&data->lock);
>>>> +            data->calib_bias = val2;
>>>> +            mutex_unlock(&data->lock);
>>>> +            return 0;
>>>> +    }
>>>> +
>>>> +    return -EINVAL;
>>>> +}
>>>> +
>>>> +static irqreturn_t lidar_trigger_handler(int irq, void *private)
>>>> +{
>>>> +    struct iio_poll_func *pf = private;
>>>> +    struct iio_dev *indio_dev = pf->indio_dev;
>>>> +    struct lidar_data *data = iio_priv(indio_dev);
>>>> +    int ret;
>>>> +
>>>> +    ret = lidar_get_measurement(data, &data->buffer[0]);
>>>> +    if (!ret) {
>>>> +            iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
>>>> +                                               iio_get_time_ns());
>>>> +    } else {
>>>> +            dev_err(&data->client->dev, "cannot read LIDAR measurement");
>>>> +    }
>>>> +
>>>> +    iio_trigger_notify_done(indio_dev->trig);
>>>> +
>>>> +    return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +static struct iio_info lidar_info = {
>>>> +    .driver_module = THIS_MODULE,
>>>> +    .read_raw = lidar_read_raw,
>>>> +    .write_raw = lidar_write_raw,
>>>> +};
>>>> +
>>>> +static int lidar_probe(struct i2c_client *client,
>>>> +                   const struct i2c_device_id *id)
>>>> +{
>>>> +    struct lidar_data *data;
>>>> +    struct iio_dev *indio_dev;
>>>> +    int ret;
>>>> +
>>>> +    indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>>>> +    if (!indio_dev)
>>>> +            return -ENOMEM;
>>>> +
>>>> +    indio_dev->info = &lidar_info;
>>>> +    indio_dev->name = LIDAR_DRV_NAME;
>>>> +    indio_dev->channels = lidar_channels;
>>>> +    indio_dev->num_channels = ARRAY_SIZE(lidar_channels);
>>>> +    indio_dev->modes = INDIO_DIRECT_MODE;
>>>> +
>>>> +    data = iio_priv(indio_dev);
>>>> +    i2c_set_clientdata(client, indio_dev);
>>>> +
>>>> +    data->client = client;
>>>> +    data->indio_dev = indio_dev;
>>>> +    mutex_init(&data->lock);
>>>> +
>>>> +    ret = iio_triggered_buffer_setup(indio_dev, NULL,
>>>> +                                     lidar_trigger_handler, NULL);
>>>> +    if (ret)
>>>> +            return ret;
>>>> +
>>>> +    ret = iio_device_register(indio_dev);
>>>> +    if (ret)
>>>> +            goto error_unreg_buffer;
>>>> +
>>>> +    return 0;
>>>> +
>>>> +error_unreg_buffer:
>>>> +    iio_triggered_buffer_cleanup(indio_dev);
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static int lidar_remove(struct i2c_client *client)
>>>> +{
>>>> +    struct iio_dev *indio_dev = i2c_get_clientdata(client);
>>>> +
>>>> +    iio_device_unregister(indio_dev);
>>>> +    iio_triggered_buffer_cleanup(indio_dev);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static const struct i2c_device_id lidar_id[] = {
>>>> +    {"lidar", 0},
>>>> +    { },
>>>> +};
>>>> +MODULE_DEVICE_TABLE(i2c, lidar_id);
>>>> +
>>>> +static struct i2c_driver lidar_driver = {
>>>> +    .driver = {
>>>> +            .name   = LIDAR_DRV_NAME,
>>>> +            .owner  = THIS_MODULE,
>>> the .owner field is not needed since it is overridden in
>>> i2c_register_driver
>>>> +    },
>>>> +    .probe          = lidar_probe,
>>>> +    .remove         = lidar_remove,
>>>> +    .id_table       = lidar_id,
>>>> +};
>>>> +module_i2c_driver(lidar_driver);
>>>> +
>>>> +MODULE_AUTHOR("Matt Ranostay <mranostay@gmail.com>");
>>>> +MODULE_DESCRIPTION("PulsedLight LIDAR sensor");
>>>> +MODULE_LICENSE("GPL");
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>> --
>> Sent from my Android device with K-9 Mail. Please excuse my brevity.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 2/2] iio: proximity: add support for PulsedLight LIDAR

[Lars-Peter Clausen]

On 08/01/2015 05:58 AM, Matt Ranostay wrote:
[...]
> +
> +struct lidar_data {
> +	struct mutex lock;
> +	struct iio_dev *indio_dev;
> +	struct i2c_client *client;
> +
> +	/* config */
> +	int calib_bias;
> +
> +	u16 buffer[5]; /* 2 byte distance + 8 byte timestamp */

Needs to be in its own cacheline to avoid issues if the I2C controller is
using DMA.

	u16 buffer[5] ____cacheline_aligned;

> +};
[...]
> +static inline int lidar_read_byte(struct lidar_data *data, int reg)

I'd drop the inline. The compiler is smart enough to figure out whether it
makes sense to inline it or not.

> +{
> +	struct i2c_client *client = data->client;
> +	int ret;
> +
> +	ret = i2c_smbus_write_byte(client, reg);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "cannot write addr value");
> +		return ret;
> +	}
> +
> +	ret = i2c_smbus_read_byte(client);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "cannot read data value");
> +		return ret;
> +	}

Instead of using a write_byte/read_byte combination rather use
i2c_smbus_read_byte_data(). I think that will do the same, but in one atomic
operation.

> +
> +	return ret;
> +}
[...]
> +static int lidar_read_measurement(struct lidar_data *data, u16 *reg)
> +{
> +	int ret;
> +	int val;
> +
> +	ret = lidar_read_byte(data, LIDAR_REG_DATA_HBYTE);
> +	if (ret < 0)
> +		return ret;
> +	val = ret << 8;
> +
> +	ret = lidar_read_byte(data, LIDAR_REG_DATA_LBYTE);
> +	if (ret < 0)
> +		return ret;
> +	val |= ret;
> +
> +	/* correct any possible overflow or underflow */
> +	val += data->calib_bias / 10000;

Typically calib bias is only meant for a offset correction that is done in
hardware rather than in software. What's the usecase for doing it in
kernelspace rather than in userspace?

> +	if (val < 0)
> +		val = 0;
> +
> +	if (val > LIDAR_REG_DATA_MAX)
> +		val = LIDAR_REG_DATA_MAX;
> +
> +	*reg = val;
> +
> +	return 0;
> +}
[...]
> +static int lidar_read_raw(struct iio_dev *indio_dev,
> +			  struct iio_chan_spec const *chan,
> +			  int *val, int *val2, long mask)
> +{
> +	struct lidar_data *data = iio_priv(indio_dev);
> +	int ret = -EINVAL;
> +
> +	if (iio_buffer_enabled(indio_dev) && mask == IIO_CHAN_INFO_RAW)
> +		return -EBUSY;
> +
> +	mutex_lock(&data->lock);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW: {
> +		u16 reg;
> +
> +		ret = lidar_get_measurement(data, &reg);
> +		if (!ret) {
> +			*val = reg / 100;
> +			*val2 = (reg % 100) * 10000;
> +			ret = IIO_VAL_INT_PLUS_MICRO;

The raw attribute is supposed to return the raw value as a IIO_VAL_INT. A
scale attribute should be used to indicate the conversion factor to get the
proper unit.

> +		}
> +		break;
> +	}
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		*val = 0;
> +		*val2 = data->calib_bias;
> +		ret = IIO_VAL_INT_PLUS_MICRO;
> +		break;
> +	}
> +
> +	mutex_unlock(&data->lock);
> +
> +	return ret;
> +}
[...]
> +static struct iio_info lidar_info = {

const

> +	.driver_module = THIS_MODULE,
> +	.read_raw = lidar_read_raw,
> +	.write_raw = lidar_write_raw,
> +};
[...]
> +static struct i2c_driver lidar_driver = {
> +	.driver = {
> +		.name	= LIDAR_DRV_NAME,
> +		.owner	= THIS_MODULE,

You added a DT vendor prefix, but there is no of match table for the driver.

> +	},
> +	.probe		= lidar_probe,
> +	.remove		= lidar_remove,
> +	.id_table	= lidar_id,
> +};
> +module_i2c_driver(lidar_driver);

Re: [PATCH 2/2] iio: proximity: add support for PulsedLight LIDAR

[Jonathan Cameron]

On 02/08/15 10:42, Lars-Peter Clausen wrote:
> On 08/01/2015 05:58 AM, Matt Ranostay wrote:
> [...]
>> +
>> +struct lidar_data {
>> +	struct mutex lock;
>> +	struct iio_dev *indio_dev;
>> +	struct i2c_client *client;
>> +
>> +	/* config */
>> +	int calib_bias;
>> +
>> +	u16 buffer[5]; /* 2 byte distance + 8 byte timestamp */
> 
> Needs to be in its own cacheline to avoid issues if the I2C controller is
> using DMA.
Would do if spi, but in the case of i2c I thought all bus drivers were
obliged to deal with this rather than leaving it to the client drivers?

Has this changed?

> 
> 	u16 buffer[5] ____cacheline_aligned;
> 
>> +};
> [...]
>> +static inline int lidar_read_byte(struct lidar_data *data, int reg)
> 
> I'd drop the inline. The compiler is smart enough to figure out whether it
> makes sense to inline it or not.
> 
>> +{
>> +	struct i2c_client *client = data->client;
>> +	int ret;
>> +
>> +	ret = i2c_smbus_write_byte(client, reg);
>> +	if (ret < 0) {
>> +		dev_err(&client->dev, "cannot write addr value");
>> +		return ret;
>> +	}
>> +
>> +	ret = i2c_smbus_read_byte(client);
>> +	if (ret < 0) {
>> +		dev_err(&client->dev, "cannot read data value");
>> +		return ret;
>> +	}
> 
> Instead of using a write_byte/read_byte combination rather use
> i2c_smbus_read_byte_data(). I think that will do the same, but in one atomic
> operation.
> 
>> +
>> +	return ret;
>> +}
> [...]
>> +static int lidar_read_measurement(struct lidar_data *data, u16 *reg)
>> +{
>> +	int ret;
>> +	int val;
>> +
>> +	ret = lidar_read_byte(data, LIDAR_REG_DATA_HBYTE);
>> +	if (ret < 0)
>> +		return ret;
>> +	val = ret << 8;
>> +
>> +	ret = lidar_read_byte(data, LIDAR_REG_DATA_LBYTE);
>> +	if (ret < 0)
>> +		return ret;
>> +	val |= ret;
>> +
>> +	/* correct any possible overflow or underflow */
>> +	val += data->calib_bias / 10000;
> 
> Typically calib bias is only meant for a offset correction that is done in
> hardware rather than in software. What's the usecase for doing it in
> kernelspace rather than in userspace?
> 
>> +	if (val < 0)
>> +		val = 0;
>> +
>> +	if (val > LIDAR_REG_DATA_MAX)
>> +		val = LIDAR_REG_DATA_MAX;
>> +
>> +	*reg = val;
>> +
>> +	return 0;
>> +}
> [...]
>> +static int lidar_read_raw(struct iio_dev *indio_dev,
>> +			  struct iio_chan_spec const *chan,
>> +			  int *val, int *val2, long mask)
>> +{
>> +	struct lidar_data *data = iio_priv(indio_dev);
>> +	int ret = -EINVAL;
>> +
>> +	if (iio_buffer_enabled(indio_dev) && mask == IIO_CHAN_INFO_RAW)
>> +		return -EBUSY;
>> +
>> +	mutex_lock(&data->lock);
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW: {
>> +		u16 reg;
>> +
>> +		ret = lidar_get_measurement(data, &reg);
>> +		if (!ret) {
>> +			*val = reg / 100;
>> +			*val2 = (reg % 100) * 10000;
>> +			ret = IIO_VAL_INT_PLUS_MICRO;
> 
> The raw attribute is supposed to return the raw value as a IIO_VAL_INT. A
> scale attribute should be used to indicate the conversion factor to get the
> proper unit.
> 
>> +		}
>> +		break;
>> +	}
>> +	case IIO_CHAN_INFO_CALIBBIAS:
>> +		*val = 0;
>> +		*val2 = data->calib_bias;
>> +		ret = IIO_VAL_INT_PLUS_MICRO;
>> +		break;
>> +	}
>> +
>> +	mutex_unlock(&data->lock);
>> +
>> +	return ret;
>> +}
> [...]
>> +static struct iio_info lidar_info = {
> 
> const
> 
>> +	.driver_module = THIS_MODULE,
>> +	.read_raw = lidar_read_raw,
>> +	.write_raw = lidar_write_raw,
>> +};
> [...]
>> +static struct i2c_driver lidar_driver = {
>> +	.driver = {
>> +		.name	= LIDAR_DRV_NAME,
>> +		.owner	= THIS_MODULE,
> 
> You added a DT vendor prefix, but there is no of match table for the driver.
> 
>> +	},
>> +	.probe		= lidar_probe,
>> +	.remove		= lidar_remove,
>> +	.id_table	= lidar_id,
>> +};
>> +module_i2c_driver(lidar_driver);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 2/2] iio: proximity: add support for PulsedLight LIDAR

[Lars-Peter Clausen]

On 08/02/2015 08:23 PM, Jonathan Cameron wrote:
> On 02/08/15 10:42, Lars-Peter Clausen wrote:
>> On 08/01/2015 05:58 AM, Matt Ranostay wrote:
>> [...]
>>> +
>>> +struct lidar_data {
>>> +	struct mutex lock;
>>> +	struct iio_dev *indio_dev;
>>> +	struct i2c_client *client;
>>> +
>>> +	/* config */
>>> +	int calib_bias;
>>> +
>>> +	u16 buffer[5]; /* 2 byte distance + 8 byte timestamp */
>>
>> Needs to be in its own cacheline to avoid issues if the I2C controller is
>> using DMA.
> Would do if spi, but in the case of i2c I thought all bus drivers were
> obliged to deal with this rather than leaving it to the client drivers?

Where did you find this? There definitely seem to be I2C drivers which
directly map the i2c_msg buffers.

But I just realized that in this case the buffer is not passed to the i2c
driver anyway so we should be fine.

- Lars

Re: [PATCH 2/2] iio: proximity: add support for PulsedLight LIDAR

[Jonathan Cameron]

On 02/08/15 19:39, Lars-Peter Clausen wrote:
> On 08/02/2015 08:23 PM, Jonathan Cameron wrote:
>> On 02/08/15 10:42, Lars-Peter Clausen wrote:
>>> On 08/01/2015 05:58 AM, Matt Ranostay wrote:
>>> [...]
>>>> +
>>>> +struct lidar_data {
>>>> +	struct mutex lock;
>>>> +	struct iio_dev *indio_dev;
>>>> +	struct i2c_client *client;
>>>> +
>>>> +	/* config */
>>>> +	int calib_bias;
>>>> +
>>>> +	u16 buffer[5]; /* 2 byte distance + 8 byte timestamp */
>>>
>>> Needs to be in its own cacheline to avoid issues if the I2C controller is
>>> using DMA.
>> Would do if spi, but in the case of i2c I thought all bus drivers were
>> obliged to deal with this rather than leaving it to the client drivers?
> 
> Where did you find this? There definitely seem to be I2C drivers which
> directly map the i2c_msg buffers.
Right now I can't track down an explicit reference.  However, all sorts
of nastiness occurs if we end up using the i2c_smbus_xfer_emulated as
that uses local buffers on the stack.

Hmm. Will look into this more when I get a chance.  We have a lot of
i2c drivers that don't have aligned buffers...

> 
> But I just realized that in this case the buffer is not passed to the i2c
> driver anyway so we should be fine.
> 
> - Lars
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 2/2] iio: proximity: add support for PulsedLight LIDAR

[Matt Ranostay]

On Sun, Aug 2, 2015 at 2:42 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 08/01/2015 05:58 AM, Matt Ranostay wrote:
> [...]
>> +
>> +struct lidar_data {
>> +     struct mutex lock;
>> +     struct iio_dev *indio_dev;
>> +     struct i2c_client *client;
>> +
>> +     /* config */
>> +     int calib_bias;
>> +
>> +     u16 buffer[5]; /* 2 byte distance + 8 byte timestamp */
>
> Needs to be in its own cacheline to avoid issues if the I2C controller is
> using DMA.

Ah though this was only needed for SPI.

>
>         u16 buffer[5] ____cacheline_aligned;
>
>> +};
> [...]
>> +static inline int lidar_read_byte(struct lidar_data *data, int reg)
>
> I'd drop the inline. The compiler is smart enough to figure out whether it
> makes sense to inline it or not.
>
Got it.

>> +{
>> +     struct i2c_client *client = data->client;
>> +     int ret;
>> +
>> +     ret = i2c_smbus_write_byte(client, reg);
>> +     if (ret < 0) {
>> +             dev_err(&client->dev, "cannot write addr value");
>> +             return ret;
>> +     }
>> +
>> +     ret = i2c_smbus_read_byte(client);
>> +     if (ret < 0) {
>> +             dev_err(&client->dev, "cannot read data value");
>> +             return ret;
>> +     }
>
> Instead of using a write_byte/read_byte combination rather use
> i2c_smbus_read_byte_data(). I think that will do the same, but in one atomic
> operation.
Yes I would normally do that but this device doesn't seem to support
that functionally, always returns zeros.

>
>> +
>> +     return ret;
>> +}
> [...]
>> +static int lidar_read_measurement(struct lidar_data *data, u16 *reg)
>> +{
>> +     int ret;
>> +     int val;
>> +
>> +     ret = lidar_read_byte(data, LIDAR_REG_DATA_HBYTE);
>> +     if (ret < 0)
>> +             return ret;
>> +     val = ret << 8;
>> +
>> +     ret = lidar_read_byte(data, LIDAR_REG_DATA_LBYTE);
>> +     if (ret < 0)
>> +             return ret;
>> +     val |= ret;
>> +
>> +     /* correct any possible overflow or underflow */
>> +     val += data->calib_bias / 10000;
>
> Typically calib bias is only meant for a offset correction that is done in
> hardware rather than in software. What's the usecase for doing it in
> kernelspace rather than in userspace?

There is hardware functionality for this but do does nothing more than
add/subtraction to the final result.
Could just drop it completely.

>
>> +     if (val < 0)
>> +             val = 0;
>> +
>> +     if (val > LIDAR_REG_DATA_MAX)
>> +             val = LIDAR_REG_DATA_MAX;
>> +
>> +     *reg = val;
>> +
>> +     return 0;
>> +}
> [...]
>> +static int lidar_read_raw(struct iio_dev *indio_dev,
>> +                       struct iio_chan_spec const *chan,
>> +                       int *val, int *val2, long mask)
>> +{
>> +     struct lidar_data *data = iio_priv(indio_dev);
>> +     int ret = -EINVAL;
>> +
>> +     if (iio_buffer_enabled(indio_dev) && mask == IIO_CHAN_INFO_RAW)
>> +             return -EBUSY;
>> +
>> +     mutex_lock(&data->lock);
>> +
>> +     switch (mask) {
>> +     case IIO_CHAN_INFO_RAW: {
>> +             u16 reg;
>> +
>> +             ret = lidar_get_measurement(data, &reg);
>> +             if (!ret) {
>> +                     *val = reg / 100;
>> +                     *val2 = (reg % 100) * 10000;
>> +                     ret = IIO_VAL_INT_PLUS_MICRO;
>
> The raw attribute is supposed to return the raw value as a IIO_VAL_INT. A
> scale attribute should be used to indicate the conversion factor to get the
> proper unit.
Ok makes sense.

>
>> +             }
>> +             break;
>> +     }
>> +     case IIO_CHAN_INFO_CALIBBIAS:
>> +             *val = 0;
>> +             *val2 = data->calib_bias;
>> +             ret = IIO_VAL_INT_PLUS_MICRO;
>> +             break;
>> +     }
>> +
>> +     mutex_unlock(&data->lock);
>> +
>> +     return ret;
>> +}
> [...]
>> +static struct iio_info lidar_info = {
>
> const
>
>> +     .driver_module = THIS_MODULE,
>> +     .read_raw = lidar_read_raw,
>> +     .write_raw = lidar_write_raw,
>> +};
> [...]
>> +static struct i2c_driver lidar_driver = {
>> +     .driver = {
>> +             .name   = LIDAR_DRV_NAME,
>> +             .owner  = THIS_MODULE,
>
> You added a DT vendor prefix, but there is no of match table for the driver.

So without of_match_table it isn't needed to have a vendor id?
"pulsedlight,lidar" maps to the i2c_device_id

>
>> +     },
>> +     .probe          = lidar_probe,
>> +     .remove         = lidar_remove,
>> +     .id_table       = lidar_id,
>> +};
>> +module_i2c_driver(lidar_driver);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] iio: proximity: add support for PulsedLight LIDAR

[Lars-Peter Clausen]

On 08/02/2015 11:28 PM, Matt Ranostay wrote:
> On Sun, Aug 2, 2015 at 2:42 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> On 08/01/2015 05:58 AM, Matt Ranostay wrote:
>> [...]
>>> +
>>> +struct lidar_data {
>>> +     struct mutex lock;
>>> +     struct iio_dev *indio_dev;
>>> +     struct i2c_client *client;
>>> +
>>> +     /* config */
>>> +     int calib_bias;
>>> +
>>> +     u16 buffer[5]; /* 2 byte distance + 8 byte timestamp */
>>
>> Needs to be in its own cacheline to avoid issues if the I2C controller is
>> using DMA.
> 
> Ah though this was only needed for SPI.

At least some I2C master drivers directly use the buffer for DMA. But I was
being stupid here anyway, you don't actually pass the buffer itself to the
I2C transfer functions so everything is fine as it was.

> 
>>
>>         u16 buffer[5] ____cacheline_aligned;
>>
>>> +};
>> [...]
>>> +static inline int lidar_read_byte(struct lidar_data *data, int reg)
>>
>> I'd drop the inline. The compiler is smart enough to figure out whether it
>> makes sense to inline it or not.
>>
> Got it.
> 
>>> +{
>>> +     struct i2c_client *client = data->client;
>>> +     int ret;
>>> +
>>> +     ret = i2c_smbus_write_byte(client, reg);
>>> +     if (ret < 0) {
>>> +             dev_err(&client->dev, "cannot write addr value");
>>> +             return ret;
>>> +     }
>>> +
>>> +     ret = i2c_smbus_read_byte(client);
>>> +     if (ret < 0) {
>>> +             dev_err(&client->dev, "cannot read data value");
>>> +             return ret;
>>> +     }
>>
>> Instead of using a write_byte/read_byte combination rather use
>> i2c_smbus_read_byte_data(). I think that will do the same, but in one atomic
>> operation.
> Yes I would normally do that but this device doesn't seem to support
> that functionally, always returns zeros.

That's an interesting device. Maybe add a comment explaining the oddity. I'm
sure I'm not the only one who'll wonder about this.

[...]
>>> +static struct i2c_driver lidar_driver = {
>>> +     .driver = {
>>> +             .name   = LIDAR_DRV_NAME,
>>> +             .owner  = THIS_MODULE,
>>
>> You added a DT vendor prefix, but there is no of match table for the driver.
> 
> So without of_match_table it isn't needed to have a vendor id?
> "pulsedlight,lidar" maps to the i2c_device_id

I thinking the other way around. If you intend to instantiate the device via
devicetree it is better to explicit add a of_device_id table rather than
relying on the implicit mechanism that uses i2c_device_id.

You should also add an entry for the device to
Documentation/devicetree/bindings/i2c/trivial-devices.txt

Re: [PATCH 2/2] iio: proximity: add support for PulsedLight LIDAR

[Matt Ranostay]

On Mon, Aug 3, 2015 at 1:00 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 08/02/2015 11:28 PM, Matt Ranostay wrote:
>> On Sun, Aug 2, 2015 at 2:42 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>> On 08/01/2015 05:58 AM, Matt Ranostay wrote:
>>> [...]
>>>> +
>>>> +struct lidar_data {
>>>> +     struct mutex lock;
>>>> +     struct iio_dev *indio_dev;
>>>> +     struct i2c_client *client;
>>>> +
>>>> +     /* config */
>>>> +     int calib_bias;
>>>> +
>>>> +     u16 buffer[5]; /* 2 byte distance + 8 byte timestamp */
>>>
>>> Needs to be in its own cacheline to avoid issues if the I2C controller is
>>> using DMA.
>>
>> Ah though this was only needed for SPI.
>
> At least some I2C master drivers directly use the buffer for DMA. But I was
> being stupid here anyway, you don't actually pass the buffer itself to the
> I2C transfer functions so everything is fine as it was.

Will back that change out in v3 :).

>
>>
>>>
>>>         u16 buffer[5] ____cacheline_aligned;
>>>
>>>> +};
>>> [...]
>>>> +static inline int lidar_read_byte(struct lidar_data *data, int reg)
>>>
>>> I'd drop the inline. The compiler is smart enough to figure out whether it
>>> makes sense to inline it or not.
>>>
>> Got it.
>>
>>>> +{
>>>> +     struct i2c_client *client = data->client;
>>>> +     int ret;
>>>> +
>>>> +     ret = i2c_smbus_write_byte(client, reg);
>>>> +     if (ret < 0) {
>>>> +             dev_err(&client->dev, "cannot write addr value");
>>>> +             return ret;
>>>> +     }
>>>> +
>>>> +     ret = i2c_smbus_read_byte(client);
>>>> +     if (ret < 0) {
>>>> +             dev_err(&client->dev, "cannot read data value");
>>>> +             return ret;
>>>> +     }
>>>
>>> Instead of using a write_byte/read_byte combination rather use
>>> i2c_smbus_read_byte_data(). I think that will do the same, but in one atomic
>>> operation.
>> Yes I would normally do that but this device doesn't seem to support
>> that functionally, always returns zeros.
>
> That's an interesting device. Maybe add a comment explaining the oddity. I'm
> sure I'm not the only one who'll wonder about this.

Ok will do. It seems to need a STOP condition between the read and
write. Hence why i2c_smbus_read_byte_data doesn't work.

>
> [...]
>>>> +static struct i2c_driver lidar_driver = {
>>>> +     .driver = {
>>>> +             .name   = LIDAR_DRV_NAME,
>>>> +             .owner  = THIS_MODULE,
>>>
>>> You added a DT vendor prefix, but there is no of match table for the driver.
>>
>> So without of_match_table it isn't needed to have a vendor id?
>> "pulsedlight,lidar" maps to the i2c_device_id
>
> I thinking the other way around. If you intend to instantiate the device via
> devicetree it is better to explicit add a of_device_id table rather than
> relying on the implicit mechanism that uses i2c_device_id.
>
> You should also add an entry for the device to
> Documentation/devicetree/bindings/i2c/trivial-devices.txt

Ok seems logical will do in v3.

>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] iio: proximity: add support for PulsedLight LIDAR

[Lars-Peter Clausen]

On 08/01/2015 05:58 AM, Matt Ranostay wrote:
> +static int lidar_read_raw(struct iio_dev *indio_dev,
> +			  struct iio_chan_spec const *chan,
> +			  int *val, int *val2, long mask)
> +{
> +	struct lidar_data *data = iio_priv(indio_dev);
> +	int ret = -EINVAL;
> +
> +	if (iio_buffer_enabled(indio_dev) && mask == IIO_CHAN_INFO_RAW)
> +		return -EBUSY;

For this to work without race conditions you need to hold the
indio_dev->mlock while doing the check and until the conversion is done.

> +
> +	mutex_lock(&data->lock);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW: {
> +		u16 reg;
> +
> +		ret = lidar_get_measurement(data, &reg);
> +		if (!ret) {
> +			*val = reg / 100;
> +			*val2 = (reg % 100) * 10000;
> +			ret = IIO_VAL_INT_PLUS_MICRO;
> +		}
> +		break;
> +	}