All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] iio: proximity: Add driver support for ST's VL53L0X ToF ranging sensor.
@ 2018-09-13  2:22 Song Qiang
  2018-09-14 18:43 ` Himanshu Jha
  0 siblings, 1 reply; 12+ messages in thread
From: Song Qiang @ 2018-09-13  2:22 UTC (permalink / raw)
  To: jic23
  Cc: knaack.h, lars, pmeerw, robh+dt, mark.rutland, andriy.shevchenko,
	matt.ranostay, tglx, himanshujha199640, ak, linux-iio,
	devicetree, linux-kernel, Song Qiang

This driver was originally written by ST in 2016 as a misc input device
driver, and hasn't been maintained for a long time. I grabbed some code
from it's API and reformed it into a iio proximity device driver.
This version of driver uses i2c bus to talk to the sensor and
polling for measuring completes, so no irq line is needed.
This version of driver supports only one-shot mode, and it can be
tested with reading from
/sys/bus/iio/devices/iio:deviceX/in_distance_raw

Signed-off-by: Song Qiang <songqiang.1304521@gmail.com>
---
Changes in v2:
	- Clean up the register table.
	- Sort header files declarations.
	- Replace some bit definations with GENMASK() and BIT().
	- Clean up some code and comments that's useless for now.
	- Change the order of some the definations of some variables to reversed
	  xmas tree order.
	- Using do...while() rather while and check.
	- Replace pr_err() with dev_err().
	- Remove device id declaration since we recommend to use DT.
	- Remove .owner = THIS_MODULE.
	- Replace probe() with probe_new() hook.
	- Remove IIO_BUFFER and IIO_TRIGGERED_BUFFER dependences.
	- Change the driver module name to vl53l0x-i2c.
	- Align all the parameters if they are in the same function with open
	  parentheses.
	- Replace iio_device_register() with devm_iio_device_register
	  for better resource management.
	- Remove the vl53l0x_remove() since it's not needed.
	- Remove dev_set_drvdata() since it's already setted above.

Changes in v3:
	- Recover ST's copyright.
	- Clean up indio_dev member in vl53l0x_data struct since it's
	  useless now.
	- Replace __le16_to_cpu() with le16_to_cpu().
	- Remove the iio_device_{claim|release}_direct_mode() since it's
	  only needed when we use buffered mode.
	- Clean up some coding style problems.

Changes in v4:
	- Add datasheet link, default i2c address and TODO list.
	- Make capitalization of defines consistent.
	- Replace i2c_transfer() with i2c_smbus_read_i2c_block_data().
	- Remove IIO_CHAN_SOFT_TIMESTAMP() since no buffer/trigger
	  support.
	- Add information to MAINTAINERS.

 .../bindings/iio/proximity/vl53l0x.txt        |  12 ++
 MAINTAINERS                                   |   7 +
 drivers/iio/proximity/Kconfig                 |  11 ++
 drivers/iio/proximity/Makefile                |   2 +
 drivers/iio/proximity/vl53l0x-i2c.c           | 178 ++++++++++++++++++
 5 files changed, 210 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
 create mode 100644 drivers/iio/proximity/vl53l0x-i2c.c

diff --git a/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
new file mode 100644
index 000000000000..64b69442f08e
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
@@ -0,0 +1,12 @@
+ST's VL53L0X ToF ranging sensor
+
+Required properties:
+	- compatible: must be "st,vl53l0x-i2c"
+	- reg: i2c address where to find the device
+
+Example:
+
+vl53l0x@29 {
+	compatible = "st,vl53l0x-i2c";
+	reg = <0x29>;
+};
diff --git a/MAINTAINERS b/MAINTAINERS
index 967ce8cdd1cc..a35d80e63506 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13510,6 +13510,13 @@ L:	linux-i2c@vger.kernel.org
 S:	Maintained
 F:	drivers/i2c/busses/i2c-stm32*
 
+ST VL53L0X ToF RANGER(I2C) IIO DRIVER
+M:	Song Qiang <songqiang.1304521@gmail.com>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	drivers/iio/proximity/vl53l0x-i2c.c
+F:	Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
+
 STABLE BRANCH
 M:	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
 L:	stable@vger.kernel.org
diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
index f726f9427602..5f421cbd37f3 100644
--- a/drivers/iio/proximity/Kconfig
+++ b/drivers/iio/proximity/Kconfig
@@ -79,4 +79,15 @@ config SRF08
 	  To compile this driver as a module, choose M here: the
 	  module will be called srf08.
 
+config VL53L0X_I2C
+	tristate "STMicroelectronics VL53L0X ToF ranger sensor (I2C)"
+	depends on I2C
+	help
+	  Say Y here to build a driver for STMicroelectronics VL53L0X
+	  ToF ranger sensors with i2c interface.
+	  This driver can be used to measure the distance of objects.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called vl53l0x-i2c.
+
 endmenu
diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
index 4f4ed45e87ef..dedfb5bf3475 100644
--- a/drivers/iio/proximity/Makefile
+++ b/drivers/iio/proximity/Makefile
@@ -10,3 +10,5 @@ obj-$(CONFIG_RFD77402)		+= rfd77402.o
 obj-$(CONFIG_SRF04)		+= srf04.o
 obj-$(CONFIG_SRF08)		+= srf08.o
 obj-$(CONFIG_SX9500)		+= sx9500.o
+obj-$(CONFIG_VL53L0X_I2C)	+= vl53l0x-i2c.o
+
diff --git a/drivers/iio/proximity/vl53l0x-i2c.c b/drivers/iio/proximity/vl53l0x-i2c.c
new file mode 100644
index 000000000000..9f0357231a36
--- /dev/null
+++ b/drivers/iio/proximity/vl53l0x-i2c.c
@@ -0,0 +1,178 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Support for ST's VL53L0X FlightSense ToF Ranger Sensor on a i2c bus.
+ *
+ * Copyright (C) 2016 STMicroelectronics Imaging Division.
+ * Copyright (C) 2018 Song Qiang <songqiang.1304521@gmail.com>
+ *
+ * Datasheet available at
+ * <https://www.st.com/resource/en/datasheet/vl53l0x.pdf>
+ * Default 7-bit I2C slave address 0x29.
+ *
+ * TODO: FIFO buffer, continuous mode, interrupts, range selection.
+ */
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/iio/iio.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+
+#define VL53L0X_DRV_NAME				"vl53l0x-i2c"
+
+#define VL_REG_SYSRANGE_MODE_MASK			GENMASK(3, 0)
+#define VL_REG_SYSRANGE_START				0x00
+#define VL_REG_SYSRANGE_MODE_SINGLESHOT			0x00
+#define VL_REG_SYSRANGE_MODE_START_STOP			BIT(0)
+#define VL_REG_SYSRANGE_MODE_BACKTOBACK			BIT(1)
+#define VL_REG_SYSRANGE_MODE_TIMED			BIT(2)
+#define VL_REG_SYSRANGE_MODE_HISTOGRAM			BIT(3)
+
+#define VL_REG_SYS_SEQUENCE_CFG				BIT(0)
+#define VL_REG_SYS_INTERMEASUREMENT_PERIOD		BIT(2)
+#define VL_REG_SYS_RANGE_CFG				0x09
+
+#define VL_REG_SYS_INT_GPIO_DISABLED			0x00
+#define VL_REG_SYS_INT_GPIO_LEVEL_LOW			BIT(0)
+#define VL_REG_SYS_INT_GPIO_LEVEL_HIGH			BIT(1)
+#define VL_REG_SYS_INT_GPIO_OUT_OF_WINDOW		0x03
+#define VL_REG_SYS_INT_GPIO_NEW_SAMPLE_READY		BIT(2)
+#define VL_REG_SYS_INT_CFG_GPIO				0x0A
+#define VL_REG_SYS_INT_CLEAR				0x0B
+#define VL_REG_GPIO_HV_MUX_ACTIVE_HIGH			0x84
+
+#define VL_REG_RESULT_INT_STATUS			0x13
+#define VL_REG_RESULT_RANGE_STATUS			0x14
+#define VL_REG_RESULT_RANGE_SATTUS_COMPLETE		BIT(0)
+
+#define VL_REG_I2C_SLAVE_DEVICE_ADDRESS			0x8A
+
+#define VL_REG_IDENTIFICATION_MODEL_ID			0xC0
+#define VL_REG_IDENTIFICATION_REVISION_ID		0xC2
+
+struct vl53l0x_data {
+	struct i2c_client *client;
+};
+
+static int vl53l0x_read_proximity(struct vl53l0x_data *data,
+				  const struct iio_chan_spec *chan,
+				  int *val)
+{
+	struct i2c_client *client = data->client;
+	unsigned int tries = 20;
+	u8 buffer[12];
+	int ret;
+
+	ret = i2c_smbus_write_byte_data(client,
+					VL_REG_SYSRANGE_START, 1);
+	if (ret < 0)
+		return ret;
+
+	do {
+		ret = i2c_smbus_read_byte_data(client,
+					       VL_REG_RESULT_RANGE_STATUS);
+		if (ret < 0)
+			return ret;
+
+		if (ret & VL_REG_RESULT_RANGE_SATTUS_COMPLETE)
+			break;
+
+		usleep_range(1000, 5000);
+	} while (tries--);
+	if (!tries)
+		return -ETIMEDOUT;
+
+	ret = i2c_smbus_read_i2c_block_data(client, VL_REG_RESULT_RANGE_STATUS,
+		12, buffer);
+	if (ret < 0)
+		return ret;
+	else if (ret != 12)
+		return -EREMOTEIO;
+
+	*val = le16_to_cpu((buffer[10] << 8) + buffer[11]);
+
+	return 0;
+}
+
+static const struct iio_chan_spec vl53l0x_channels[] = {
+	{
+		.type = IIO_DISTANCE,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW)
+	},
+};
+
+static int vl53l0x_read_raw(struct iio_dev *indio_dev,
+			    const struct iio_chan_spec *chan,
+			    int *val, int *val2, long mask)
+{
+	struct vl53l0x_data *data = iio_priv(indio_dev);
+	int ret;
+
+	if (chan->type != IIO_DISTANCE) {
+		dev_err(&data->client->dev, "iio type error");
+		return -EINVAL;
+	}
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = vl53l0x_read_proximity(data, chan, val);
+		if (ret < 0)
+			dev_err(&data->client->dev,
+				"raw value read error with %d", ret);
+
+		return IIO_VAL_INT;
+	default:
+		dev_err(&data->client->dev, "IIO_CHAN_* not recognzed.");
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info vl53l0x_info = {
+	.read_raw = vl53l0x_read_raw,
+};
+
+static int vl53l0x_probe(struct i2c_client *client)
+{
+	struct vl53l0x_data *data;
+	struct iio_dev *indio_dev;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(indio_dev);
+	data->client = client;
+	i2c_set_clientdata(client, indio_dev);
+
+	if (!i2c_check_functionality(client->adapter,
+				     I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_I2C))
+		return -EOPNOTSUPP;
+
+	indio_dev->dev.parent = &client->dev;
+	indio_dev->name = VL53L0X_DRV_NAME;
+	indio_dev->info = &vl53l0x_info;
+	indio_dev->channels = vl53l0x_channels;
+	indio_dev->num_channels = ARRAY_SIZE(vl53l0x_channels);
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	return devm_iio_device_register(&client->dev, indio_dev);
+}
+
+static const struct of_device_id st_vl53l0x_dt_match[] = {
+	{ .compatible = "st,vl53l0x-i2c", },
+	{ }
+};
+
+static struct i2c_driver vl53l0x_driver = {
+	.driver = {
+		.name = VL53L0X_DRV_NAME,
+		.of_match_table = st_vl53l0x_dt_match,
+	},
+	.probe_new = vl53l0x_probe,
+};
+module_i2c_driver(vl53l0x_driver);
+
+MODULE_AUTHOR("Song Qiang <songqiang.1304521@gmail.com>");
+MODULE_DESCRIPTION("ST vl53l0x ToF ranging sensor");
+MODULE_LICENSE("GPL");
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] iio: proximity: Add driver support for ST's VL53L0X ToF ranging sensor.
  2018-09-13  2:22 [PATCH] iio: proximity: Add driver support for ST's VL53L0X ToF ranging sensor Song Qiang
@ 2018-09-14 18:43 ` Himanshu Jha
  2018-09-15  2:25   ` Song Qiang
  0 siblings, 1 reply; 12+ messages in thread
From: Himanshu Jha @ 2018-09-14 18:43 UTC (permalink / raw)
  To: Song Qiang
  Cc: jic23, knaack.h, lars, pmeerw, robh+dt, mark.rutland,
	andriy.shevchenko, matt.ranostay, tglx, ak, linux-iio,
	devicetree, linux-kernel, Song Qiang

Hi Song,

On Thu, Sep 13, 2018 at 10:22:29AM +0800, Song Qiang wrote:
> This driver was originally written by ST in 2016 as a misc input device
> driver, and hasn't been maintained for a long time. I grabbed some code
> from it's API and reformed it into a iio proximity device driver.
> This version of driver uses i2c bus to talk to the sensor and
> polling for measuring completes, so no irq line is needed.
> This version of driver supports only one-shot mode, and it can be
> tested with reading from
> /sys/bus/iio/devices/iio:deviceX/in_distance_raw
> 
> Signed-off-by: Song Qiang <songqiang.1304521@gmail.com>

I'm sorry but you probably need another revision.

And don't feel embarrassed, I mean this is your first patch
in the kernel and that's an IIO driver!!

I used to fix spelling mistakes, align parameters, ... an year ago
and even those got rejected several times ;)

> ---

> +ST VL53L0X ToF RANGER(I2C) IIO DRIVER
> +M:	Song Qiang <songqiang.1304521@gmail.com>
> +L:	linux-iio@vger.kernel.org
> +S:	Maintained
> +F:	drivers/iio/proximity/vl53l0x-i2c.c
> +F:	Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt

Obligations!

Just don't go "Keyser Söze" once you're done with your major
at University. You would need to test/review patches in future.

Not a big deal though..

> +
> +static const struct of_device_id st_vl53l0x_dt_match[] = {
> +	{ .compatible = "st,vl53l0x-i2c", },
> +	{ }
> +};

MODULE_DEVICE_TABLE(of, st_vl53l0x_dt_match); ?


Thanks
-- 
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] iio: proximity: Add driver support for ST's VL53L0X ToF ranging sensor.
  2018-09-14 18:43 ` Himanshu Jha
@ 2018-09-15  2:25   ` Song Qiang
  0 siblings, 0 replies; 12+ messages in thread
From: Song Qiang @ 2018-09-15  2:25 UTC (permalink / raw)
  To: Himanshu Jha
  Cc: Song Qiang, jic23, knaack.h, lars, pmeerw, robh+dt, mark.rutland,
	andriy.shevchenko, matt.ranostay, tglx, ak, linux-iio,
	devicetree, linux-kernel

On Sat, Sep 15, 2018 at 12:13:22AM +0530, Himanshu Jha wrote:
> Hi Song,
> 
> On Thu, Sep 13, 2018 at 10:22:29AM +0800, Song Qiang wrote:
> > This driver was originally written by ST in 2016 as a misc input device
> > driver, and hasn't been maintained for a long time. I grabbed some code
> > from it's API and reformed it into a iio proximity device driver.
> > This version of driver uses i2c bus to talk to the sensor and
> > polling for measuring completes, so no irq line is needed.
> > This version of driver supports only one-shot mode, and it can be
> > tested with reading from
> > /sys/bus/iio/devices/iio:deviceX/in_distance_raw
> > 
> > Signed-off-by: Song Qiang <songqiang.1304521@gmail.com>
> 
> I'm sorry but you probably need another revision.
> 
> And don't feel embarrassed, I mean this is your first patch
> in the kernel and that's an IIO driver!!
> 
> I used to fix spelling mistakes, align parameters, ... an year ago
> and even those got rejected several times ;)
> 
> > ---
> 
> > +ST VL53L0X ToF RANGER(I2C) IIO DRIVER
> > +M:	Song Qiang <songqiang.1304521@gmail.com>
> > +L:	linux-iio@vger.kernel.org
> > +S:	Maintained
> > +F:	drivers/iio/proximity/vl53l0x-i2c.c
> > +F:	Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> 
> Obligations!
> 
> Just don't go "Keyser Söze" once you're done with your major
> at University. You would need to test/review patches in future.
> 
> Not a big deal though..
> 
> > +
> > +static const struct of_device_id st_vl53l0x_dt_match[] = {
> > +	{ .compatible = "st,vl53l0x-i2c", },
> > +	{ }
> > +};
> 
> MODULE_DEVICE_TABLE(of, st_vl53l0x_dt_match); ?
> 
> 
> Thanks
> -- 
> Himanshu Jha
> Undergraduate Student
> Department of Electronics & Communication
> Guru Tegh Bahadur Institute of Technology

Hi Himanshu,

I think I'll do some double check on it to make sure it's good, compare
my patch with other drivers to see some more problems I miss.
Thanks for your kindness, and with or without this patch accepted, I've
been feeling glad to get connected with the community and learned a lot
from people like you. :)

yours,
Song Qiang

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] iio: proximity: Add driver support for ST's VL53L0X ToF ranging sensor.
  2018-09-11  7:28     ` Himanshu Jha
@ 2018-09-11  7:43       ` Song Qiang
  0 siblings, 0 replies; 12+ messages in thread
From: Song Qiang @ 2018-09-11  7:43 UTC (permalink / raw)
  To: Himanshu Jha
  Cc: jic23, knaack.h, lars, pmeerw, andriy.shevchenko, matt.ranostay,
	ak, linux-iio, linux-kernel

On Tue, Sep 11, 2018 at 12:58:20PM +0530, Himanshu Jha wrote:
> On Tue, Sep 11, 2018 at 02:46:38PM +0800, Song Qiang wrote:
> > On Mon, Sep 10, 2018 at 11:27:47PM +0530, Himanshu Jha wrote:
> > > On Mon, Sep 10, 2018 at 10:42:59PM +0800, Song Qiang wrote:
> > > > This driver was originally written by ST in 2016 as a misc input device,
> > > > and hasn't been maintained for a long time. I grabbed some code from
> > > > it's API and reformed it to a iio proximity device driver.
> > > > This version of driver uses i2c bus to talk to the sensor and
> > > > polling for measuring completes, so no irq line is needed.
> > > > This version of driver supports only one-shot mode, and it can be
> > > > tested with reading from
> > > > /sys/bus/iio/devices/iio:deviceX/in_distance_raw
> > > > 
> > > > Signed-off-by: Song Qiang <songqiang.1304521@gmail.com>
> > > > ---
> > > 
> > > The Cc list contains developers who might not be relevant
> > > for the discussion.
> > > 
> > > So, copy only those people listed by:
> > > 
> > > $./scripts/get_maintainer.pl <driver.patch>
> > > 
> > > Don't know why Kate & Greg are cc'ed ?
> > > 
> > > >  .../bindings/iio/proximity/vl53l0x.txt        |  12 +
> > > >  drivers/iio/proximity/Kconfig                 |  13 +
> > > >  drivers/iio/proximity/Makefile                |   2 +
> > > >  drivers/iio/proximity/vl53l0x-i2c.c           | 295 ++++++++++++++++++
> > > >  4 files changed, 322 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > > >  create mode 100644 drivers/iio/proximity/vl53l0x-i2c.c
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > > > new file mode 100644
> > > > index 000000000000..64b69442f08e
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > > > @@ -0,0 +1,12 @@
> > > > +ST's VL53L0X ToF ranging sensor
> > > > +
> > > > +Required properties:
> > > > +	- compatible: must be "st,vl53l0x-i2c"
> > > > +	- reg: i2c address where to find the device
> > > > +
> > > > +Example:
> > > > +
> > > > +vl53l0x@29 {
> > > > +	compatible = "st,vl53l0x-i2c";
> > > > +	reg = <0x29>;
> > > > +};
> > > > diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
> > > > index f726f9427602..1563a5f9144d 100644
> > > > --- a/drivers/iio/proximity/Kconfig
> > > > +++ b/drivers/iio/proximity/Kconfig
> > > > @@ -79,4 +79,17 @@ config SRF08
> > > >  	  To compile this driver as a module, choose M here: the
> > > >  	  module will be called srf08.
> > > >  
> > > > +config VL53L0X_I2C
> > > > +	tristate "STMicroelectronics VL53L0X ToF ranger sensor (I2C)"
> > > > +	select IIO_BUFFER
> > > > +	select IIO_TRIGGERED_BUFFER
> > > 
> > > I don't see any buffer/trigger support, so better to remove these
> > > two options.
> > > 
> > > > +	depends on I2C
> > > > +	help
> > > > +	  Say Y here to build a driver for STMicroelectronics VL53L0X
> > > > +	  ToF ranger sensors with i2c interface.
> > > > +	  This driver can be used to measure the distance of objects.
> > > > +
> > > > +	  To compile this driver as a module, choose M here: the
> > > > +	  module will be called vl53l0x-i2c.
> > > 
> > > `name` attribute will be VL53L0X_DRV_NAME(vl53l0x) if OF matching
> > > is not used to probe the driver.
> > > 
> > > >  endmenu
> > > > diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
> > > > index 4f4ed45e87ef..7cb771665c8b 100644
> > > > --- a/drivers/iio/proximity/Makefile
> > > > +++ b/drivers/iio/proximity/Makefile
> > > > @@ -10,3 +10,5 @@ obj-$(CONFIG_RFD77402)		+= rfd77402.o
> > > >  obj-$(CONFIG_SRF04)		+= srf04.o
> > > >  obj-$(CONFIG_SRF08)		+= srf08.o
> > > >  obj-$(CONFIG_SX9500)		+= sx9500.o
> > > > +obj-$(CONFIG_VL53L0X_I2C)		+= vl53l0x-i2c.o
> > > > +
> > > > diff --git a/drivers/iio/proximity/vl53l0x-i2c.c b/drivers/iio/proximity/vl53l0x-i2c.c
> > > > new file mode 100644
> > > > index 000000000000..c00713041d30
> > > > --- /dev/null
> > > > +++ b/drivers/iio/proximity/vl53l0x-i2c.c
> > > > @@ -0,0 +1,295 @@
> > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > +/*
> > > > + *  vl53l0x-i2c.c - Support for STM VL53L0X FlightSense TOF
> > > > + *					Ranger Sensor on a i2c bus.
> > > > + *
> > > > + *  Copyright (C) 2016 STMicroelectronics Imaging Division.
> > > > + *  Copyright (C) 2018 Song Qiang <songqiang.1304521@gmail.com>
> > > > + *
> > > > + */
> > > > +
> > > > +#include <linux/module.h>
> > > > +#include <linux/slab.h>
> > > > +#include <linux/i2c.h>
> > > > +#include <linux/delay.h>
> > > > +#include <linux/kernel.h>
> > > > +#include <linux/iio/iio.h>
> > > > +
> > > > +#define VL53L0X_DRV_NAME			"vl53l0x"
> > > > +
> > > > +/* Device register map */
> > > > +#define VL_REG_SYSRANGE_START					0x000
> > > > +#define VL_REG_SYSRANGE_MODE_MASK				0x0F
> > > > +#define VL_REG_SYSRANGE_MODE_START_STOP			0x01
> > > > +#define VL_REG_SYSRANGE_MODE_SINGLESHOT			0x00
> > > > +#define VL_REG_SYSRANGE_MODE_BACKTOBACK			0x02
> > > > +#define VL_REG_SYSRANGE_MODE_TIMED				0x04
> > > > +#define VL_REG_SYSRANGE_MODE_HISTOGRAM			0x08
> > > > +
> > > > +#define VL_REG_SYS_THRESH_HIGH					0x000C
> > > > +#define VL_REG_SYS_THRESH_LOW					0x000E
> > > > +
> > > > +#define VL_REG_SYS_SEQUENCE_CFG					0x0001
> > > > +#define VL_REG_SYS_RANGE_CFG					0x0009
> > > > +#define VL_REG_SYS_INTERMEASUREMENT_PERIOD		0x0004
> > > 
> > > Could you please align all these macros properly.
> > > 
> > > > +#define VL_REG_SYS_INT_CFG_GPIO					0x000A
> > > > +#define VL_REG_SYS_INT_GPIO_DISABLED			0x00
> > > > +#define VL_REG_SYS_INT_GPIO_LEVEL_LOW			0x01
> > > > +#define VL_REG_SYS_INT_GPIO_LEVEL_HIGH			0x02
> > > > +#define VL_REG_SYS_INT_GPIO_OUT_OF_WINDOW		0x03
> > > > +#define VL_REG_SYS_INT_GPIO_NEW_SAMPLE_READY	0x04
> > > > +#define VL_REG_GPIO_HV_MUX_ACTIVE_HIGH			0x0084
> > > > +#define VL_REG_SYS_INT_CLEAR					0x000B
> > > > +
> > > > +/* Result registers */
> > > > +#define VL_REG_RESULT_INT_STATUS				0x0013
> > > > +#define VL_REG_RESULT_RANGE_STATUS				0x0014
> > > > +
> > > > +#define VL_REG_RESULT_CORE_PAGE  1
> > > > +#define VL_REG_RESULT_CORE_AMBIENT_WINDOW_EVENTS_RTN	0x00BC
> > > > +#define VL_REG_RESULT_CORE_RANGING_TOTAL_EVENTS_RTN		0x00C0
> > > > +#define VL_REG_RESULT_CORE_AMBIENT_WINDOW_EVENTS_REF	0x00D0
> > > > +#define VL_REG_RESULT_CORE_RANGING_TOTAL_EVENTS_REF		0x00D4
> > > > +#define VL_REG_RESULT_PEAK_SIGNAL_RATE_REF				0x00B6
> > > > +
> > > > +/* Algo register */
> > > > +#define VL_REG_ALGO_PART_TO_PART_RANGE_OFFSET_MM		0x0028
> > > > +
> > > > +#define VL_REG_I2C_SLAVE_DEVICE_ADDRESS					0x008a
> > > > +
> > > > +/* Check Limit registers */
> > > > +#define VL_REG_MSRC_CFG_CONTROL						0x0060
> > > > +
> > > > +#define VL_REG_PRE_RANGE_CFG_MIN_SNR					0X0027
> > > > +#define VL_REG_PRE_RANGE_CFG_VALID_PHASE_LOW			0x0056
> > > > +#define VL_REG_PRE_RANGE_CFG_VALID_PHASE_HIGH			0x0057
> > > > +#define VL_REG_PRE_RANGE_MIN_COUNT_RATE_RTN_LIMIT		0x0064
> > > > +
> > > > +#define VL_REG_FINAL_RANGE_CFG_MIN_SNR					0X0067
> > > > +#define VL_REG_FINAL_RANGE_CFG_VALID_PHASE_LOW			0x0047
> > > > +#define VL_REG_FINAL_RANGE_CFG_VALID_PHASE_HIGH			0x0048
> > > > +#define VL_REG_FINAL_RANGE_CFG_MIN_COUNT_RATE_RTN_LIMIT	0x0044
> > > > +
> > > > +#define VL_REG_PRE_RANGE_CFG_SIGMA_THRESH_HI			0X0061
> > > > +#define VL_REG_PRE_RANGE_CFG_SIGMA_THRESH_LO			0X0062
> > > > +
> > > > +/* PRE RANGE registers */
> > > > +#define VL_REG_PRE_RANGE_CFG_VCSEL_PERIOD				0x0050
> > > > +#define VL_REG_PRE_RANGE_CFG_TIMEOUT_MACROP_HI			0x0051
> > > > +#define VL_REG_PRE_RANGE_CFG_TIMEOUT_MACROP_LO			0x0052
> > > > +
> > > > +#define VL_REG_SYS_HISTOGRAM_BIN					0x0081
> > > > +#define VL_REG_HISTOGRAM_CFG_INITIAL_PHASE_SELECT		0x0033
> > > > +#define VL_REG_HISTOGRAM_CFG_READOUT_CTRL				0x0055
> > > > +
> > > > +#define VL_REG_FINAL_RANGE_CFG_VCSEL_PERIOD				0x0070
> > > > +#define VL_REG_FINAL_RANGE_CFG_TIMEOUT_MACROP_HI		0x0071
> > > > +#define VL_REG_FINAL_RANGE_CFG_TIMEOUT_MACROP_LO		0x0072
> > > > +#define VL_REG_CROSSTALK_COMPENSATION_PEAK_RATE_MCPS	0x0020
> > > > +
> > > > +#define VL_REG_MSRC_CFG_TIMEOUT_MACROP					0x0046
> > > > +
> > > > +#define VL_REG_SOFT_RESET_GO2_SOFT_RESET_N				0x00bf
> > > > +#define VL_REG_IDENTIFICATION_MODEL_ID					0x00c0
> > > > +#define VL_REG_IDENTIFICATION_REVISION_ID				0x00c2
> > > > +
> > > > +#define VL_REG_OSC_CALIBRATE_VAL					0x00f8
> > > > +
> > > > +#define VL_SIGMA_ESTIMATE_MAX_VALUE					65535
> > > > +/* equivalent to a range sigma of 655.35mm */
> > > > +
> > > > +#define VL_REG_GLOBAL_CFG_VCSEL_WIDTH					0x032
> > > > +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_0			0x0B0
> > > > +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_1			0x0B1
> > > > +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_2			0x0B2
> > > > +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_3			0x0B3
> > > > +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_4			0x0B4
> > > > +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_5			0x0B5
> > > > +#define VL_REG_GLOBAL_CFG_REF_EN_START_SELECT			0xB6
> > > > +#define VL_REG_DYNAMIC_SPAD_NUM_REQUESTED_REF_SPAD		0x4E /* 0x14E */
> > > > +#define VL_REG_DYNAMIC_SPAD_REF_EN_START_OFFSET			0x4F /* 0x14F */
> > > > +#define VL_REG_POWER_MANAGEMENT_GO1_POWER_FORCE			0x80
> > > > +
> > > > +/*
> > > > + * Speed of light in um per 1E-10 Seconds
> > > > + */
> > > > +#define VL_SPEED_OF_LIGHT_IN_AIR					2997
> > > > +#define VL_REG_VHV_CFG_PAD_SCL_SDA__EXTSUP_HV			0x0089
> > > > +#define VL_REG_ALGO_PHASECAL_LIM			0x0030 /* 0x130 */
> > > > +#define VL_REG_ALGO_PHASECAL_CFG_TIMEOUT				0x0030
> > > > +
> > > > +struct vl53l0x_data {
> > > > +	struct i2c_client *client;
> > > > +	struct mutex lock;
> > > 
> > > This lock needs a comment to explain its purpose.
> > > 
> > > > +	int	useLongRange;
> > > 
> > > Weird spacing.
> > > 
> > > > +
> > > > +static int vl53l0x_read_proximity(struct vl53l0x_data *data,
> > > > +				const struct iio_chan_spec *chan,
> > > > +				int *val)
> > > 
> > > Align all these functions to match open parentheses with mix of
> > > tabs + whitespaces(as required):
> > > 
> > > static int vl53l0x_read_proximity(struct vl53l0x_data *data,
> > > 				  const struct iio_chan_spec *chan,
> > > 				  int *val)
> > > 
> > 
> > Hi Himanshu,
> > 
> > I found that some functions like this one is too long for alignment,
> > and if we force the parameters to align then the first parameter
> > must go to the second line, while I havn't seen people done this
> > before. In this case, should I remain others aligned except the
> > first parameter or keep them all aligned with the first parameter
> > in the second line?
> 
> I don't really understand "too long for alignment".
> 
> Refer here and look at all the function parameters alignment:
> https://raw.githubusercontent.com/torvalds/linux/master/drivers/iio/chemical/bme680_core.c
> 
> It just looks more readable and clean.
> 
> > I have corrected the problems you and Andy listed just except this
> > one, hoping for a reply.
> 
> Thanks for repairing them :)
> 
> And your question about commit signers:
> 
> Yes, kate confirmed to me that she got added when SPDX was introduced, so its
> fine. I just told because I had not seen her in any of IIO patches, and
> it looked dubious to me.
> 
> And Greg also doesn't review IIO patches(but Cc him on staging/iio) and
> already gets a blast of other patches from staging, usb, .... + other
> important obligations related to -stable release and stuff.
> 
> 
> -- 
> Himanshu Jha
> Undergraduate Student
> Department of Electronics & Communication
> Guru Tegh Bahadur Institute of Technology

Okay, I think I will just not cc them in future iio patches and give
them more time for other important patches. :)

yours,
Song Qiang

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] iio: proximity: Add driver support for ST's VL53L0X ToF ranging sensor.
  2018-09-11  6:46   ` Song Qiang
@ 2018-09-11  7:28     ` Himanshu Jha
  2018-09-11  7:43       ` Song Qiang
  0 siblings, 1 reply; 12+ messages in thread
From: Himanshu Jha @ 2018-09-11  7:28 UTC (permalink / raw)
  To: Song Qiang
  Cc: jic23, knaack.h, lars, pmeerw, andriy.shevchenko, matt.ranostay,
	ak, gregkh, linux-iio, linux-kernel

On Tue, Sep 11, 2018 at 02:46:38PM +0800, Song Qiang wrote:
> On Mon, Sep 10, 2018 at 11:27:47PM +0530, Himanshu Jha wrote:
> > On Mon, Sep 10, 2018 at 10:42:59PM +0800, Song Qiang wrote:
> > > This driver was originally written by ST in 2016 as a misc input device,
> > > and hasn't been maintained for a long time. I grabbed some code from
> > > it's API and reformed it to a iio proximity device driver.
> > > This version of driver uses i2c bus to talk to the sensor and
> > > polling for measuring completes, so no irq line is needed.
> > > This version of driver supports only one-shot mode, and it can be
> > > tested with reading from
> > > /sys/bus/iio/devices/iio:deviceX/in_distance_raw
> > > 
> > > Signed-off-by: Song Qiang <songqiang.1304521@gmail.com>
> > > ---
> > 
> > The Cc list contains developers who might not be relevant
> > for the discussion.
> > 
> > So, copy only those people listed by:
> > 
> > $./scripts/get_maintainer.pl <driver.patch>
> > 
> > Don't know why Kate & Greg are cc'ed ?
> > 
> > >  .../bindings/iio/proximity/vl53l0x.txt        |  12 +
> > >  drivers/iio/proximity/Kconfig                 |  13 +
> > >  drivers/iio/proximity/Makefile                |   2 +
> > >  drivers/iio/proximity/vl53l0x-i2c.c           | 295 ++++++++++++++++++
> > >  4 files changed, 322 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > >  create mode 100644 drivers/iio/proximity/vl53l0x-i2c.c
> > > 
> > > diff --git a/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > > new file mode 100644
> > > index 000000000000..64b69442f08e
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > > @@ -0,0 +1,12 @@
> > > +ST's VL53L0X ToF ranging sensor
> > > +
> > > +Required properties:
> > > +	- compatible: must be "st,vl53l0x-i2c"
> > > +	- reg: i2c address where to find the device
> > > +
> > > +Example:
> > > +
> > > +vl53l0x@29 {
> > > +	compatible = "st,vl53l0x-i2c";
> > > +	reg = <0x29>;
> > > +};
> > > diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
> > > index f726f9427602..1563a5f9144d 100644
> > > --- a/drivers/iio/proximity/Kconfig
> > > +++ b/drivers/iio/proximity/Kconfig
> > > @@ -79,4 +79,17 @@ config SRF08
> > >  	  To compile this driver as a module, choose M here: the
> > >  	  module will be called srf08.
> > >  
> > > +config VL53L0X_I2C
> > > +	tristate "STMicroelectronics VL53L0X ToF ranger sensor (I2C)"
> > > +	select IIO_BUFFER
> > > +	select IIO_TRIGGERED_BUFFER
> > 
> > I don't see any buffer/trigger support, so better to remove these
> > two options.
> > 
> > > +	depends on I2C
> > > +	help
> > > +	  Say Y here to build a driver for STMicroelectronics VL53L0X
> > > +	  ToF ranger sensors with i2c interface.
> > > +	  This driver can be used to measure the distance of objects.
> > > +
> > > +	  To compile this driver as a module, choose M here: the
> > > +	  module will be called vl53l0x-i2c.
> > 
> > `name` attribute will be VL53L0X_DRV_NAME(vl53l0x) if OF matching
> > is not used to probe the driver.
> > 
> > >  endmenu
> > > diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
> > > index 4f4ed45e87ef..7cb771665c8b 100644
> > > --- a/drivers/iio/proximity/Makefile
> > > +++ b/drivers/iio/proximity/Makefile
> > > @@ -10,3 +10,5 @@ obj-$(CONFIG_RFD77402)		+= rfd77402.o
> > >  obj-$(CONFIG_SRF04)		+= srf04.o
> > >  obj-$(CONFIG_SRF08)		+= srf08.o
> > >  obj-$(CONFIG_SX9500)		+= sx9500.o
> > > +obj-$(CONFIG_VL53L0X_I2C)		+= vl53l0x-i2c.o
> > > +
> > > diff --git a/drivers/iio/proximity/vl53l0x-i2c.c b/drivers/iio/proximity/vl53l0x-i2c.c
> > > new file mode 100644
> > > index 000000000000..c00713041d30
> > > --- /dev/null
> > > +++ b/drivers/iio/proximity/vl53l0x-i2c.c
> > > @@ -0,0 +1,295 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + *  vl53l0x-i2c.c - Support for STM VL53L0X FlightSense TOF
> > > + *					Ranger Sensor on a i2c bus.
> > > + *
> > > + *  Copyright (C) 2016 STMicroelectronics Imaging Division.
> > > + *  Copyright (C) 2018 Song Qiang <songqiang.1304521@gmail.com>
> > > + *
> > > + */
> > > +
> > > +#include <linux/module.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/iio/iio.h>
> > > +
> > > +#define VL53L0X_DRV_NAME			"vl53l0x"
> > > +
> > > +/* Device register map */
> > > +#define VL_REG_SYSRANGE_START					0x000
> > > +#define VL_REG_SYSRANGE_MODE_MASK				0x0F
> > > +#define VL_REG_SYSRANGE_MODE_START_STOP			0x01
> > > +#define VL_REG_SYSRANGE_MODE_SINGLESHOT			0x00
> > > +#define VL_REG_SYSRANGE_MODE_BACKTOBACK			0x02
> > > +#define VL_REG_SYSRANGE_MODE_TIMED				0x04
> > > +#define VL_REG_SYSRANGE_MODE_HISTOGRAM			0x08
> > > +
> > > +#define VL_REG_SYS_THRESH_HIGH					0x000C
> > > +#define VL_REG_SYS_THRESH_LOW					0x000E
> > > +
> > > +#define VL_REG_SYS_SEQUENCE_CFG					0x0001
> > > +#define VL_REG_SYS_RANGE_CFG					0x0009
> > > +#define VL_REG_SYS_INTERMEASUREMENT_PERIOD		0x0004
> > 
> > Could you please align all these macros properly.
> > 
> > > +#define VL_REG_SYS_INT_CFG_GPIO					0x000A
> > > +#define VL_REG_SYS_INT_GPIO_DISABLED			0x00
> > > +#define VL_REG_SYS_INT_GPIO_LEVEL_LOW			0x01
> > > +#define VL_REG_SYS_INT_GPIO_LEVEL_HIGH			0x02
> > > +#define VL_REG_SYS_INT_GPIO_OUT_OF_WINDOW		0x03
> > > +#define VL_REG_SYS_INT_GPIO_NEW_SAMPLE_READY	0x04
> > > +#define VL_REG_GPIO_HV_MUX_ACTIVE_HIGH			0x0084
> > > +#define VL_REG_SYS_INT_CLEAR					0x000B
> > > +
> > > +/* Result registers */
> > > +#define VL_REG_RESULT_INT_STATUS				0x0013
> > > +#define VL_REG_RESULT_RANGE_STATUS				0x0014
> > > +
> > > +#define VL_REG_RESULT_CORE_PAGE  1
> > > +#define VL_REG_RESULT_CORE_AMBIENT_WINDOW_EVENTS_RTN	0x00BC
> > > +#define VL_REG_RESULT_CORE_RANGING_TOTAL_EVENTS_RTN		0x00C0
> > > +#define VL_REG_RESULT_CORE_AMBIENT_WINDOW_EVENTS_REF	0x00D0
> > > +#define VL_REG_RESULT_CORE_RANGING_TOTAL_EVENTS_REF		0x00D4
> > > +#define VL_REG_RESULT_PEAK_SIGNAL_RATE_REF				0x00B6
> > > +
> > > +/* Algo register */
> > > +#define VL_REG_ALGO_PART_TO_PART_RANGE_OFFSET_MM		0x0028
> > > +
> > > +#define VL_REG_I2C_SLAVE_DEVICE_ADDRESS					0x008a
> > > +
> > > +/* Check Limit registers */
> > > +#define VL_REG_MSRC_CFG_CONTROL						0x0060
> > > +
> > > +#define VL_REG_PRE_RANGE_CFG_MIN_SNR					0X0027
> > > +#define VL_REG_PRE_RANGE_CFG_VALID_PHASE_LOW			0x0056
> > > +#define VL_REG_PRE_RANGE_CFG_VALID_PHASE_HIGH			0x0057
> > > +#define VL_REG_PRE_RANGE_MIN_COUNT_RATE_RTN_LIMIT		0x0064
> > > +
> > > +#define VL_REG_FINAL_RANGE_CFG_MIN_SNR					0X0067
> > > +#define VL_REG_FINAL_RANGE_CFG_VALID_PHASE_LOW			0x0047
> > > +#define VL_REG_FINAL_RANGE_CFG_VALID_PHASE_HIGH			0x0048
> > > +#define VL_REG_FINAL_RANGE_CFG_MIN_COUNT_RATE_RTN_LIMIT	0x0044
> > > +
> > > +#define VL_REG_PRE_RANGE_CFG_SIGMA_THRESH_HI			0X0061
> > > +#define VL_REG_PRE_RANGE_CFG_SIGMA_THRESH_LO			0X0062
> > > +
> > > +/* PRE RANGE registers */
> > > +#define VL_REG_PRE_RANGE_CFG_VCSEL_PERIOD				0x0050
> > > +#define VL_REG_PRE_RANGE_CFG_TIMEOUT_MACROP_HI			0x0051
> > > +#define VL_REG_PRE_RANGE_CFG_TIMEOUT_MACROP_LO			0x0052
> > > +
> > > +#define VL_REG_SYS_HISTOGRAM_BIN					0x0081
> > > +#define VL_REG_HISTOGRAM_CFG_INITIAL_PHASE_SELECT		0x0033
> > > +#define VL_REG_HISTOGRAM_CFG_READOUT_CTRL				0x0055
> > > +
> > > +#define VL_REG_FINAL_RANGE_CFG_VCSEL_PERIOD				0x0070
> > > +#define VL_REG_FINAL_RANGE_CFG_TIMEOUT_MACROP_HI		0x0071
> > > +#define VL_REG_FINAL_RANGE_CFG_TIMEOUT_MACROP_LO		0x0072
> > > +#define VL_REG_CROSSTALK_COMPENSATION_PEAK_RATE_MCPS	0x0020
> > > +
> > > +#define VL_REG_MSRC_CFG_TIMEOUT_MACROP					0x0046
> > > +
> > > +#define VL_REG_SOFT_RESET_GO2_SOFT_RESET_N				0x00bf
> > > +#define VL_REG_IDENTIFICATION_MODEL_ID					0x00c0
> > > +#define VL_REG_IDENTIFICATION_REVISION_ID				0x00c2
> > > +
> > > +#define VL_REG_OSC_CALIBRATE_VAL					0x00f8
> > > +
> > > +#define VL_SIGMA_ESTIMATE_MAX_VALUE					65535
> > > +/* equivalent to a range sigma of 655.35mm */
> > > +
> > > +#define VL_REG_GLOBAL_CFG_VCSEL_WIDTH					0x032
> > > +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_0			0x0B0
> > > +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_1			0x0B1
> > > +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_2			0x0B2
> > > +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_3			0x0B3
> > > +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_4			0x0B4
> > > +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_5			0x0B5
> > > +#define VL_REG_GLOBAL_CFG_REF_EN_START_SELECT			0xB6
> > > +#define VL_REG_DYNAMIC_SPAD_NUM_REQUESTED_REF_SPAD		0x4E /* 0x14E */
> > > +#define VL_REG_DYNAMIC_SPAD_REF_EN_START_OFFSET			0x4F /* 0x14F */
> > > +#define VL_REG_POWER_MANAGEMENT_GO1_POWER_FORCE			0x80
> > > +
> > > +/*
> > > + * Speed of light in um per 1E-10 Seconds
> > > + */
> > > +#define VL_SPEED_OF_LIGHT_IN_AIR					2997
> > > +#define VL_REG_VHV_CFG_PAD_SCL_SDA__EXTSUP_HV			0x0089
> > > +#define VL_REG_ALGO_PHASECAL_LIM			0x0030 /* 0x130 */
> > > +#define VL_REG_ALGO_PHASECAL_CFG_TIMEOUT				0x0030
> > > +
> > > +struct vl53l0x_data {
> > > +	struct i2c_client *client;
> > > +	struct mutex lock;
> > 
> > This lock needs a comment to explain its purpose.
> > 
> > > +	int	useLongRange;
> > 
> > Weird spacing.
> > 
> > > +
> > > +static int vl53l0x_read_proximity(struct vl53l0x_data *data,
> > > +				const struct iio_chan_spec *chan,
> > > +				int *val)
> > 
> > Align all these functions to match open parentheses with mix of
> > tabs + whitespaces(as required):
> > 
> > static int vl53l0x_read_proximity(struct vl53l0x_data *data,
> > 				  const struct iio_chan_spec *chan,
> > 				  int *val)
> > 
> 
> Hi Himanshu,
> 
> I found that some functions like this one is too long for alignment,
> and if we force the parameters to align then the first parameter
> must go to the second line, while I havn't seen people done this
> before. In this case, should I remain others aligned except the
> first parameter or keep them all aligned with the first parameter
> in the second line?

I don't really understand "too long for alignment".

Refer here and look at all the function parameters alignment:
https://raw.githubusercontent.com/torvalds/linux/master/drivers/iio/chemical/bme680_core.c

It just looks more readable and clean.

> I have corrected the problems you and Andy listed just except this
> one, hoping for a reply.

Thanks for repairing them :)

And your question about commit signers:

Yes, kate confirmed to me that she got added when SPDX was introduced, so its
fine. I just told because I had not seen her in any of IIO patches, and
it looked dubious to me.

And Greg also doesn't review IIO patches(but Cc him on staging/iio) and
already gets a blast of other patches from staging, usb, .... + other
important obligations related to -stable release and stuff.


-- 
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] iio: proximity: Add driver support for ST's VL53L0X ToF ranging sensor.
  2018-09-10 17:57 ` Himanshu Jha
  2018-09-11  2:47   ` Song Qiang
  2018-09-11  6:46   ` Song Qiang
@ 2018-09-11  7:09   ` Song Qiang
  2 siblings, 0 replies; 12+ messages in thread
From: Song Qiang @ 2018-09-11  7:09 UTC (permalink / raw)
  To: Himanshu Jha; +Cc: linux-kernel

On Mon, Sep 10, 2018 at 11:27:47PM +0530, Himanshu Jha wrote:
> On Mon, Sep 10, 2018 at 10:42:59PM +0800, Song Qiang wrote:
> > This driver was originally written by ST in 2016 as a misc input device,
> > and hasn't been maintained for a long time. I grabbed some code from
> > it's API and reformed it to a iio proximity device driver.
> > This version of driver uses i2c bus to talk to the sensor and
> > polling for measuring completes, so no irq line is needed.
> > This version of driver supports only one-shot mode, and it can be
> > tested with reading from
> > /sys/bus/iio/devices/iio:deviceX/in_distance_raw
> > 
> > Signed-off-by: Song Qiang <songqiang.1304521@gmail.com>
> > ---
> 
> The Cc list contains developers who might not be relevant
> for the discussion.
> 
> So, copy only those people listed by:
> 
> $./scripts/get_maintainer.pl <driver.patch>
> 
> Don't know why Kate & Greg are cc'ed ?
> 
> >  .../bindings/iio/proximity/vl53l0x.txt        |  12 +
> >  drivers/iio/proximity/Kconfig                 |  13 +
> >  drivers/iio/proximity/Makefile                |   2 +
> >  drivers/iio/proximity/vl53l0x-i2c.c           | 295 ++++++++++++++++++
> >  4 files changed, 322 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> >  create mode 100644 drivers/iio/proximity/vl53l0x-i2c.c
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > new file mode 100644
> > index 000000000000..64b69442f08e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > @@ -0,0 +1,12 @@
> > +ST's VL53L0X ToF ranging sensor
> > +
> > +Required properties:
> > +	- compatible: must be "st,vl53l0x-i2c"
> > +	- reg: i2c address where to find the device
> > +
> > +Example:
> > +
> > +vl53l0x@29 {
> > +	compatible = "st,vl53l0x-i2c";
> > +	reg = <0x29>;
> > +};
> > diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
> > index f726f9427602..1563a5f9144d 100644
> > --- a/drivers/iio/proximity/Kconfig
> > +++ b/drivers/iio/proximity/Kconfig
> > @@ -79,4 +79,17 @@ config SRF08
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called srf08.
> >  
> > +config VL53L0X_I2C
> > +	tristate "STMicroelectronics VL53L0X ToF ranger sensor (I2C)"
> > +	select IIO_BUFFER
> > +	select IIO_TRIGGERED_BUFFER
> 
> I don't see any buffer/trigger support, so better to remove these
> two options.
> 
> > +	depends on I2C
> > +	help
> > +	  Say Y here to build a driver for STMicroelectronics VL53L0X
> > +	  ToF ranger sensors with i2c interface.
> > +	  This driver can be used to measure the distance of objects.
> > +
> > +	  To compile this driver as a module, choose M here: the
> > +	  module will be called vl53l0x-i2c.
> 
> `name` attribute will be VL53L0X_DRV_NAME(vl53l0x) if OF matching
> is not used to probe the driver.
> 
> >  endmenu
> > diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
> > index 4f4ed45e87ef..7cb771665c8b 100644
> > --- a/drivers/iio/proximity/Makefile
> > +++ b/drivers/iio/proximity/Makefile
> > @@ -10,3 +10,5 @@ obj-$(CONFIG_RFD77402)		+= rfd77402.o
> >  obj-$(CONFIG_SRF04)		+= srf04.o
> >  obj-$(CONFIG_SRF08)		+= srf08.o
> >  obj-$(CONFIG_SX9500)		+= sx9500.o
> > +obj-$(CONFIG_VL53L0X_I2C)		+= vl53l0x-i2c.o
> > +
> > diff --git a/drivers/iio/proximity/vl53l0x-i2c.c b/drivers/iio/proximity/vl53l0x-i2c.c
> > new file mode 100644
> > index 000000000000..c00713041d30
> > --- /dev/null
> > +++ b/drivers/iio/proximity/vl53l0x-i2c.c
> > @@ -0,0 +1,295 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + *  vl53l0x-i2c.c - Support for STM VL53L0X FlightSense TOF
> > + *					Ranger Sensor on a i2c bus.
> > + *
> > + *  Copyright (C) 2016 STMicroelectronics Imaging Division.
> > + *  Copyright (C) 2018 Song Qiang <songqiang.1304521@gmail.com>
> > + *
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +#include <linux/i2c.h>
> > +#include <linux/delay.h>
> > +#include <linux/kernel.h>
> > +#include <linux/iio/iio.h>
> > +
> > +#define VL53L0X_DRV_NAME			"vl53l0x"
> > +
> > +/* Device register map */
> > +#define VL_REG_SYSRANGE_START					0x000
> > +#define VL_REG_SYSRANGE_MODE_MASK				0x0F
> > +#define VL_REG_SYSRANGE_MODE_START_STOP			0x01
> > +#define VL_REG_SYSRANGE_MODE_SINGLESHOT			0x00
> > +#define VL_REG_SYSRANGE_MODE_BACKTOBACK			0x02
> > +#define VL_REG_SYSRANGE_MODE_TIMED				0x04
> > +#define VL_REG_SYSRANGE_MODE_HISTOGRAM			0x08
> > +
> > +#define VL_REG_SYS_THRESH_HIGH					0x000C
> > +#define VL_REG_SYS_THRESH_LOW					0x000E
> > +
> > +#define VL_REG_SYS_SEQUENCE_CFG					0x0001
> > +#define VL_REG_SYS_RANGE_CFG					0x0009
> > +#define VL_REG_SYS_INTERMEASUREMENT_PERIOD		0x0004
> 
> Could you please align all these macros properly.
> 
> > +#define VL_REG_SYS_INT_CFG_GPIO					0x000A
> > +#define VL_REG_SYS_INT_GPIO_DISABLED			0x00
> > +#define VL_REG_SYS_INT_GPIO_LEVEL_LOW			0x01
> > +#define VL_REG_SYS_INT_GPIO_LEVEL_HIGH			0x02
> > +#define VL_REG_SYS_INT_GPIO_OUT_OF_WINDOW		0x03
> > +#define VL_REG_SYS_INT_GPIO_NEW_SAMPLE_READY	0x04
> > +#define VL_REG_GPIO_HV_MUX_ACTIVE_HIGH			0x0084
> > +#define VL_REG_SYS_INT_CLEAR					0x000B
> > +
> > +/* Result registers */
> > +#define VL_REG_RESULT_INT_STATUS				0x0013
> > +#define VL_REG_RESULT_RANGE_STATUS				0x0014
> > +
> > +#define VL_REG_RESULT_CORE_PAGE  1
> > +#define VL_REG_RESULT_CORE_AMBIENT_WINDOW_EVENTS_RTN	0x00BC
> > +#define VL_REG_RESULT_CORE_RANGING_TOTAL_EVENTS_RTN		0x00C0
> > +#define VL_REG_RESULT_CORE_AMBIENT_WINDOW_EVENTS_REF	0x00D0
> > +#define VL_REG_RESULT_CORE_RANGING_TOTAL_EVENTS_REF		0x00D4
> > +#define VL_REG_RESULT_PEAK_SIGNAL_RATE_REF				0x00B6
> > +
> > +/* Algo register */
> > +#define VL_REG_ALGO_PART_TO_PART_RANGE_OFFSET_MM		0x0028
> > +
> > +#define VL_REG_I2C_SLAVE_DEVICE_ADDRESS					0x008a
> > +
> > +/* Check Limit registers */
> > +#define VL_REG_MSRC_CFG_CONTROL						0x0060
> > +
> > +#define VL_REG_PRE_RANGE_CFG_MIN_SNR					0X0027
> > +#define VL_REG_PRE_RANGE_CFG_VALID_PHASE_LOW			0x0056
> > +#define VL_REG_PRE_RANGE_CFG_VALID_PHASE_HIGH			0x0057
> > +#define VL_REG_PRE_RANGE_MIN_COUNT_RATE_RTN_LIMIT		0x0064
> > +
> > +#define VL_REG_FINAL_RANGE_CFG_MIN_SNR					0X0067
> > +#define VL_REG_FINAL_RANGE_CFG_VALID_PHASE_LOW			0x0047
> > +#define VL_REG_FINAL_RANGE_CFG_VALID_PHASE_HIGH			0x0048
> > +#define VL_REG_FINAL_RANGE_CFG_MIN_COUNT_RATE_RTN_LIMIT	0x0044
> > +
> > +#define VL_REG_PRE_RANGE_CFG_SIGMA_THRESH_HI			0X0061
> > +#define VL_REG_PRE_RANGE_CFG_SIGMA_THRESH_LO			0X0062
> > +
> > +/* PRE RANGE registers */
> > +#define VL_REG_PRE_RANGE_CFG_VCSEL_PERIOD				0x0050
> > +#define VL_REG_PRE_RANGE_CFG_TIMEOUT_MACROP_HI			0x0051
> > +#define VL_REG_PRE_RANGE_CFG_TIMEOUT_MACROP_LO			0x0052
> > +
> > +#define VL_REG_SYS_HISTOGRAM_BIN					0x0081
> > +#define VL_REG_HISTOGRAM_CFG_INITIAL_PHASE_SELECT		0x0033
> > +#define VL_REG_HISTOGRAM_CFG_READOUT_CTRL				0x0055
> > +
> > +#define VL_REG_FINAL_RANGE_CFG_VCSEL_PERIOD				0x0070
> > +#define VL_REG_FINAL_RANGE_CFG_TIMEOUT_MACROP_HI		0x0071
> > +#define VL_REG_FINAL_RANGE_CFG_TIMEOUT_MACROP_LO		0x0072
> > +#define VL_REG_CROSSTALK_COMPENSATION_PEAK_RATE_MCPS	0x0020
> > +
> > +#define VL_REG_MSRC_CFG_TIMEOUT_MACROP					0x0046
> > +
> > +#define VL_REG_SOFT_RESET_GO2_SOFT_RESET_N				0x00bf
> > +#define VL_REG_IDENTIFICATION_MODEL_ID					0x00c0
> > +#define VL_REG_IDENTIFICATION_REVISION_ID				0x00c2
> > +
> > +#define VL_REG_OSC_CALIBRATE_VAL					0x00f8
> > +
> > +#define VL_SIGMA_ESTIMATE_MAX_VALUE					65535
> > +/* equivalent to a range sigma of 655.35mm */
> > +
> > +#define VL_REG_GLOBAL_CFG_VCSEL_WIDTH					0x032
> > +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_0			0x0B0
> > +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_1			0x0B1
> > +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_2			0x0B2
> > +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_3			0x0B3
> > +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_4			0x0B4
> > +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_5			0x0B5
> > +#define VL_REG_GLOBAL_CFG_REF_EN_START_SELECT			0xB6
> > +#define VL_REG_DYNAMIC_SPAD_NUM_REQUESTED_REF_SPAD		0x4E /* 0x14E */
> > +#define VL_REG_DYNAMIC_SPAD_REF_EN_START_OFFSET			0x4F /* 0x14F */
> > +#define VL_REG_POWER_MANAGEMENT_GO1_POWER_FORCE			0x80
> > +
> > +/*
> > + * Speed of light in um per 1E-10 Seconds
> > + */
> > +#define VL_SPEED_OF_LIGHT_IN_AIR					2997
> > +#define VL_REG_VHV_CFG_PAD_SCL_SDA__EXTSUP_HV			0x0089
> > +#define VL_REG_ALGO_PHASECAL_LIM			0x0030 /* 0x130 */
> > +#define VL_REG_ALGO_PHASECAL_CFG_TIMEOUT				0x0030
> > +
> > +struct vl53l0x_data {
> > +	struct i2c_client *client;
> > +	struct mutex lock;
> 
> This lock needs a comment to explain its purpose.
> 
> > +	int	useLongRange;
> 
> Weird spacing.
> 
> > +
> > +static int vl53l0x_read_proximity(struct vl53l0x_data *data,
> > +				const struct iio_chan_spec *chan,
> > +				int *val)
> 
> Align all these functions to match open parentheses with mix of
> tabs + whitespaces(as required):
> 
> static int vl53l0x_read_proximity(struct vl53l0x_data *data,
> 				  const struct iio_chan_spec *chan,
> 				  int *val)
> 
> 
> > +	int ret;
> > +	struct i2c_client *client = data->client;
> > +	int tries = 20;
> > +	u8 buffer[12];
> > +	struct i2c_msg msg[2];
> > +	u8 write_command = VL_REG_RESULT_RANGE_STATUS;
> > +
> > +	ret = i2c_smbus_write_byte_data(data->client,
> > +					VL_REG_SYSRANGE_START, 1);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	while (tries-- > 0) {
> > +		ret = i2c_smbus_read_byte_data(data->client,
> > +						VL_REG_RESULT_RANGE_STATUS);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		if (ret & 0x01)
> > +			break;
> > +		usleep_range(1000, 5000);
> > +	}
> > +
> > +	if (tries < 0)
> > +		return -ETIMEDOUT;
> > +
> > +	msg[0].addr = client->addr;
> > +	msg[0].buf = &write_command;
> > +	msg[0].len = 1;
> > +	msg[0].flags = client->flags | I2C_M_STOP;
> > +
> > +	msg[1].addr = client->addr;
> > +	msg[1].buf = buffer;
> > +	msg[1].len = 12;
> > +	msg[1].flags = client->flags | I2C_M_RD;
> > +
> > +	ret = i2c_transfer(client->adapter, msg, 2);
> > +
> > +	if (ret != 2) {
> > +		pr_err("vl53l0x: consecutively read error. ");
> > +		return ret;
> > +	}
> > +
> > +	*val = __le16_to_cpu((buffer[10] << 8) + buffer[11]);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct iio_chan_spec vl53l0x_channels[] = {
> > +	{
> > +		.type = IIO_DISTANCE,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW)
> > +	},
> > +	IIO_CHAN_SOFT_TIMESTAMP(1),
> > +};
> > +
> > +static int vl53l0x_read_raw(struct iio_dev *indio_dev,
> > +				const struct iio_chan_spec *chan,
> > +				int *val, int *val2, long mask)
> > +{
> > +	struct vl53l0x_data *data = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	if (chan->type != IIO_DISTANCE) {
> > +		pr_err("vl53l0x: iio type error");
> > +		return -EINVAL;
> > +	}
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		ret = iio_device_claim_direct_mode(indio_dev);
> > +		if (ret)
> > +			return ret;
> > +		ret = vl53l0x_read_proximity(data, chan, val);
> > +		if (ret < 0)
> > +			pr_err("vl53l0x: raw value read error with %d", ret);
> > +
> > +		ret = IIO_VAL_INT;
> > +		iio_device_release_direct_mode(indio_dev);
> > +		return ret;
> > +	default:
> > +		pr_err("vl53l0x: IIO_CHAN_* not recognzed.");
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static const struct iio_info vl53l0x_info = {
> > +	.read_raw = vl53l0x_read_raw,
> > +};
> > +
> > +static int vl53l0x_probe(struct i2c_client *client,
> > +				   const struct i2c_device_id *id)
> > +{
> > +	int ret;
> > +	struct vl53l0x_data *data;
> > +	struct iio_dev *indio_dev;
> > +
> > +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	data = iio_priv(indio_dev);
> > +	data->client = client;
> > +	i2c_set_clientdata(client, indio_dev);
> > +	mutex_init(&data->lock);
> > +
> > +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE))
> > +		return -EOPNOTSUPP;
> > +
> > +	indio_dev->dev.parent = &client->dev;
> > +	indio_dev->name = VL53L0X_DRV_NAME;
> > +	indio_dev->info = &vl53l0x_info;
> > +	indio_dev->channels = vl53l0x_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(vl53l0x_channels);
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > +	//probe 0xc0 if the value is 0xEE.
> > +
> > +	ret = iio_device_register(indio_dev);
> 
> Would better be better to use resource managened functions since
> I don't see any point of using vl53l0x_remove() function below.
> Better use devm_iio_device_register()!
> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	dev_set_drvdata(&client->dev, data);
> 
> You already setted it up above using:
> 
> 	i2c_set_clientdata(client, indio_dev);
> 
> > +	return 0;
> > +}
> > +
> > +static int vl53l0x_remove(struct i2c_client *client)
> > +{
> > +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > +	struct vl53l0x_data *data = iio_priv(indio_dev);
> > +
> > +	iio_device_unregister(indio_dev);
> > +	kfree(data);
> > +
> > +	return 0;
> > +}
> 
> Plus all other comments addressed by Andy.
> 
> 
> Thanks
> -- 
> Himanshu Jha
> Undergraduate Student
> Department of Electronics & Communication
> Guru Tegh Bahadur Institute of Technology

Hi Himanshu,

Sorry for the bothering, I think the alignment problem and my question
about alignment of parameters was because my editor's tansize settings.
After I changed them, they are all fine now.

Many thanks for spending time with my patch.

yours,
Song Qiang

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] iio: proximity: Add driver support for ST's VL53L0X ToF ranging sensor.
  2018-09-10 17:57 ` Himanshu Jha
  2018-09-11  2:47   ` Song Qiang
@ 2018-09-11  6:46   ` Song Qiang
  2018-09-11  7:28     ` Himanshu Jha
  2018-09-11  7:09   ` Song Qiang
  2 siblings, 1 reply; 12+ messages in thread
From: Song Qiang @ 2018-09-11  6:46 UTC (permalink / raw)
  To: Himanshu Jha
  Cc: jic23, knaack.h, lars, pmeerw, andriy.shevchenko, matt.ranostay,
	ak, gregkh, linux-iio, linux-kernel

On Mon, Sep 10, 2018 at 11:27:47PM +0530, Himanshu Jha wrote:
> On Mon, Sep 10, 2018 at 10:42:59PM +0800, Song Qiang wrote:
> > This driver was originally written by ST in 2016 as a misc input device,
> > and hasn't been maintained for a long time. I grabbed some code from
> > it's API and reformed it to a iio proximity device driver.
> > This version of driver uses i2c bus to talk to the sensor and
> > polling for measuring completes, so no irq line is needed.
> > This version of driver supports only one-shot mode, and it can be
> > tested with reading from
> > /sys/bus/iio/devices/iio:deviceX/in_distance_raw
> > 
> > Signed-off-by: Song Qiang <songqiang.1304521@gmail.com>
> > ---
> 
> The Cc list contains developers who might not be relevant
> for the discussion.
> 
> So, copy only those people listed by:
> 
> $./scripts/get_maintainer.pl <driver.patch>
> 
> Don't know why Kate & Greg are cc'ed ?
> 
> >  .../bindings/iio/proximity/vl53l0x.txt        |  12 +
> >  drivers/iio/proximity/Kconfig                 |  13 +
> >  drivers/iio/proximity/Makefile                |   2 +
> >  drivers/iio/proximity/vl53l0x-i2c.c           | 295 ++++++++++++++++++
> >  4 files changed, 322 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> >  create mode 100644 drivers/iio/proximity/vl53l0x-i2c.c
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > new file mode 100644
> > index 000000000000..64b69442f08e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > @@ -0,0 +1,12 @@
> > +ST's VL53L0X ToF ranging sensor
> > +
> > +Required properties:
> > +	- compatible: must be "st,vl53l0x-i2c"
> > +	- reg: i2c address where to find the device
> > +
> > +Example:
> > +
> > +vl53l0x@29 {
> > +	compatible = "st,vl53l0x-i2c";
> > +	reg = <0x29>;
> > +};
> > diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
> > index f726f9427602..1563a5f9144d 100644
> > --- a/drivers/iio/proximity/Kconfig
> > +++ b/drivers/iio/proximity/Kconfig
> > @@ -79,4 +79,17 @@ config SRF08
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called srf08.
> >  
> > +config VL53L0X_I2C
> > +	tristate "STMicroelectronics VL53L0X ToF ranger sensor (I2C)"
> > +	select IIO_BUFFER
> > +	select IIO_TRIGGERED_BUFFER
> 
> I don't see any buffer/trigger support, so better to remove these
> two options.
> 
> > +	depends on I2C
> > +	help
> > +	  Say Y here to build a driver for STMicroelectronics VL53L0X
> > +	  ToF ranger sensors with i2c interface.
> > +	  This driver can be used to measure the distance of objects.
> > +
> > +	  To compile this driver as a module, choose M here: the
> > +	  module will be called vl53l0x-i2c.
> 
> `name` attribute will be VL53L0X_DRV_NAME(vl53l0x) if OF matching
> is not used to probe the driver.
> 
> >  endmenu
> > diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
> > index 4f4ed45e87ef..7cb771665c8b 100644
> > --- a/drivers/iio/proximity/Makefile
> > +++ b/drivers/iio/proximity/Makefile
> > @@ -10,3 +10,5 @@ obj-$(CONFIG_RFD77402)		+= rfd77402.o
> >  obj-$(CONFIG_SRF04)		+= srf04.o
> >  obj-$(CONFIG_SRF08)		+= srf08.o
> >  obj-$(CONFIG_SX9500)		+= sx9500.o
> > +obj-$(CONFIG_VL53L0X_I2C)		+= vl53l0x-i2c.o
> > +
> > diff --git a/drivers/iio/proximity/vl53l0x-i2c.c b/drivers/iio/proximity/vl53l0x-i2c.c
> > new file mode 100644
> > index 000000000000..c00713041d30
> > --- /dev/null
> > +++ b/drivers/iio/proximity/vl53l0x-i2c.c
> > @@ -0,0 +1,295 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + *  vl53l0x-i2c.c - Support for STM VL53L0X FlightSense TOF
> > + *					Ranger Sensor on a i2c bus.
> > + *
> > + *  Copyright (C) 2016 STMicroelectronics Imaging Division.
> > + *  Copyright (C) 2018 Song Qiang <songqiang.1304521@gmail.com>
> > + *
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +#include <linux/i2c.h>
> > +#include <linux/delay.h>
> > +#include <linux/kernel.h>
> > +#include <linux/iio/iio.h>
> > +
> > +#define VL53L0X_DRV_NAME			"vl53l0x"
> > +
> > +/* Device register map */
> > +#define VL_REG_SYSRANGE_START					0x000
> > +#define VL_REG_SYSRANGE_MODE_MASK				0x0F
> > +#define VL_REG_SYSRANGE_MODE_START_STOP			0x01
> > +#define VL_REG_SYSRANGE_MODE_SINGLESHOT			0x00
> > +#define VL_REG_SYSRANGE_MODE_BACKTOBACK			0x02
> > +#define VL_REG_SYSRANGE_MODE_TIMED				0x04
> > +#define VL_REG_SYSRANGE_MODE_HISTOGRAM			0x08
> > +
> > +#define VL_REG_SYS_THRESH_HIGH					0x000C
> > +#define VL_REG_SYS_THRESH_LOW					0x000E
> > +
> > +#define VL_REG_SYS_SEQUENCE_CFG					0x0001
> > +#define VL_REG_SYS_RANGE_CFG					0x0009
> > +#define VL_REG_SYS_INTERMEASUREMENT_PERIOD		0x0004
> 
> Could you please align all these macros properly.
> 
> > +#define VL_REG_SYS_INT_CFG_GPIO					0x000A
> > +#define VL_REG_SYS_INT_GPIO_DISABLED			0x00
> > +#define VL_REG_SYS_INT_GPIO_LEVEL_LOW			0x01
> > +#define VL_REG_SYS_INT_GPIO_LEVEL_HIGH			0x02
> > +#define VL_REG_SYS_INT_GPIO_OUT_OF_WINDOW		0x03
> > +#define VL_REG_SYS_INT_GPIO_NEW_SAMPLE_READY	0x04
> > +#define VL_REG_GPIO_HV_MUX_ACTIVE_HIGH			0x0084
> > +#define VL_REG_SYS_INT_CLEAR					0x000B
> > +
> > +/* Result registers */
> > +#define VL_REG_RESULT_INT_STATUS				0x0013
> > +#define VL_REG_RESULT_RANGE_STATUS				0x0014
> > +
> > +#define VL_REG_RESULT_CORE_PAGE  1
> > +#define VL_REG_RESULT_CORE_AMBIENT_WINDOW_EVENTS_RTN	0x00BC
> > +#define VL_REG_RESULT_CORE_RANGING_TOTAL_EVENTS_RTN		0x00C0
> > +#define VL_REG_RESULT_CORE_AMBIENT_WINDOW_EVENTS_REF	0x00D0
> > +#define VL_REG_RESULT_CORE_RANGING_TOTAL_EVENTS_REF		0x00D4
> > +#define VL_REG_RESULT_PEAK_SIGNAL_RATE_REF				0x00B6
> > +
> > +/* Algo register */
> > +#define VL_REG_ALGO_PART_TO_PART_RANGE_OFFSET_MM		0x0028
> > +
> > +#define VL_REG_I2C_SLAVE_DEVICE_ADDRESS					0x008a
> > +
> > +/* Check Limit registers */
> > +#define VL_REG_MSRC_CFG_CONTROL						0x0060
> > +
> > +#define VL_REG_PRE_RANGE_CFG_MIN_SNR					0X0027
> > +#define VL_REG_PRE_RANGE_CFG_VALID_PHASE_LOW			0x0056
> > +#define VL_REG_PRE_RANGE_CFG_VALID_PHASE_HIGH			0x0057
> > +#define VL_REG_PRE_RANGE_MIN_COUNT_RATE_RTN_LIMIT		0x0064
> > +
> > +#define VL_REG_FINAL_RANGE_CFG_MIN_SNR					0X0067
> > +#define VL_REG_FINAL_RANGE_CFG_VALID_PHASE_LOW			0x0047
> > +#define VL_REG_FINAL_RANGE_CFG_VALID_PHASE_HIGH			0x0048
> > +#define VL_REG_FINAL_RANGE_CFG_MIN_COUNT_RATE_RTN_LIMIT	0x0044
> > +
> > +#define VL_REG_PRE_RANGE_CFG_SIGMA_THRESH_HI			0X0061
> > +#define VL_REG_PRE_RANGE_CFG_SIGMA_THRESH_LO			0X0062
> > +
> > +/* PRE RANGE registers */
> > +#define VL_REG_PRE_RANGE_CFG_VCSEL_PERIOD				0x0050
> > +#define VL_REG_PRE_RANGE_CFG_TIMEOUT_MACROP_HI			0x0051
> > +#define VL_REG_PRE_RANGE_CFG_TIMEOUT_MACROP_LO			0x0052
> > +
> > +#define VL_REG_SYS_HISTOGRAM_BIN					0x0081
> > +#define VL_REG_HISTOGRAM_CFG_INITIAL_PHASE_SELECT		0x0033
> > +#define VL_REG_HISTOGRAM_CFG_READOUT_CTRL				0x0055
> > +
> > +#define VL_REG_FINAL_RANGE_CFG_VCSEL_PERIOD				0x0070
> > +#define VL_REG_FINAL_RANGE_CFG_TIMEOUT_MACROP_HI		0x0071
> > +#define VL_REG_FINAL_RANGE_CFG_TIMEOUT_MACROP_LO		0x0072
> > +#define VL_REG_CROSSTALK_COMPENSATION_PEAK_RATE_MCPS	0x0020
> > +
> > +#define VL_REG_MSRC_CFG_TIMEOUT_MACROP					0x0046
> > +
> > +#define VL_REG_SOFT_RESET_GO2_SOFT_RESET_N				0x00bf
> > +#define VL_REG_IDENTIFICATION_MODEL_ID					0x00c0
> > +#define VL_REG_IDENTIFICATION_REVISION_ID				0x00c2
> > +
> > +#define VL_REG_OSC_CALIBRATE_VAL					0x00f8
> > +
> > +#define VL_SIGMA_ESTIMATE_MAX_VALUE					65535
> > +/* equivalent to a range sigma of 655.35mm */
> > +
> > +#define VL_REG_GLOBAL_CFG_VCSEL_WIDTH					0x032
> > +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_0			0x0B0
> > +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_1			0x0B1
> > +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_2			0x0B2
> > +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_3			0x0B3
> > +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_4			0x0B4
> > +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_5			0x0B5
> > +#define VL_REG_GLOBAL_CFG_REF_EN_START_SELECT			0xB6
> > +#define VL_REG_DYNAMIC_SPAD_NUM_REQUESTED_REF_SPAD		0x4E /* 0x14E */
> > +#define VL_REG_DYNAMIC_SPAD_REF_EN_START_OFFSET			0x4F /* 0x14F */
> > +#define VL_REG_POWER_MANAGEMENT_GO1_POWER_FORCE			0x80
> > +
> > +/*
> > + * Speed of light in um per 1E-10 Seconds
> > + */
> > +#define VL_SPEED_OF_LIGHT_IN_AIR					2997
> > +#define VL_REG_VHV_CFG_PAD_SCL_SDA__EXTSUP_HV			0x0089
> > +#define VL_REG_ALGO_PHASECAL_LIM			0x0030 /* 0x130 */
> > +#define VL_REG_ALGO_PHASECAL_CFG_TIMEOUT				0x0030
> > +
> > +struct vl53l0x_data {
> > +	struct i2c_client *client;
> > +	struct mutex lock;
> 
> This lock needs a comment to explain its purpose.
> 
> > +	int	useLongRange;
> 
> Weird spacing.
> 
> > +
> > +static int vl53l0x_read_proximity(struct vl53l0x_data *data,
> > +				const struct iio_chan_spec *chan,
> > +				int *val)
> 
> Align all these functions to match open parentheses with mix of
> tabs + whitespaces(as required):
> 
> static int vl53l0x_read_proximity(struct vl53l0x_data *data,
> 				  const struct iio_chan_spec *chan,
> 				  int *val)
> 

Hi Himanshu,

I found that some functions like this one is too long for alignment,
and if we force the parameters to align then the first parameter
must go to the second line, while I havn't seen people done this
before. In this case, should I remain others aligned except the
first parameter or keep them all aligned with the first parameter
in the second line?
I have corrected the problems you and Andy listed just except this
one, hoping for a reply.

Thanks a lot for your reviewing.

yours,
Song Qiang

> 
> > +	int ret;
> > +	struct i2c_client *client = data->client;
> > +	int tries = 20;
> > +	u8 buffer[12];
> > +	struct i2c_msg msg[2];
> > +	u8 write_command = VL_REG_RESULT_RANGE_STATUS;
> > +
> > +	ret = i2c_smbus_write_byte_data(data->client,
> > +					VL_REG_SYSRANGE_START, 1);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	while (tries-- > 0) {
> > +		ret = i2c_smbus_read_byte_data(data->client,
> > +						VL_REG_RESULT_RANGE_STATUS);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		if (ret & 0x01)
> > +			break;
> > +		usleep_range(1000, 5000);
> > +	}
> > +
> > +	if (tries < 0)
> > +		return -ETIMEDOUT;
> > +
> > +	msg[0].addr = client->addr;
> > +	msg[0].buf = &write_command;
> > +	msg[0].len = 1;
> > +	msg[0].flags = client->flags | I2C_M_STOP;
> > +
> > +	msg[1].addr = client->addr;
> > +	msg[1].buf = buffer;
> > +	msg[1].len = 12;
> > +	msg[1].flags = client->flags | I2C_M_RD;
> > +
> > +	ret = i2c_transfer(client->adapter, msg, 2);
> > +
> > +	if (ret != 2) {
> > +		pr_err("vl53l0x: consecutively read error. ");
> > +		return ret;
> > +	}
> > +
> > +	*val = __le16_to_cpu((buffer[10] << 8) + buffer[11]);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct iio_chan_spec vl53l0x_channels[] = {
> > +	{
> > +		.type = IIO_DISTANCE,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW)
> > +	},
> > +	IIO_CHAN_SOFT_TIMESTAMP(1),
> > +};
> > +
> > +static int vl53l0x_read_raw(struct iio_dev *indio_dev,
> > +				const struct iio_chan_spec *chan,
> > +				int *val, int *val2, long mask)
> > +{
> > +	struct vl53l0x_data *data = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	if (chan->type != IIO_DISTANCE) {
> > +		pr_err("vl53l0x: iio type error");
> > +		return -EINVAL;
> > +	}
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		ret = iio_device_claim_direct_mode(indio_dev);
> > +		if (ret)
> > +			return ret;
> > +		ret = vl53l0x_read_proximity(data, chan, val);
> > +		if (ret < 0)
> > +			pr_err("vl53l0x: raw value read error with %d", ret);
> > +
> > +		ret = IIO_VAL_INT;
> > +		iio_device_release_direct_mode(indio_dev);
> > +		return ret;
> > +	default:
> > +		pr_err("vl53l0x: IIO_CHAN_* not recognzed.");
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static const struct iio_info vl53l0x_info = {
> > +	.read_raw = vl53l0x_read_raw,
> > +};
> > +
> > +static int vl53l0x_probe(struct i2c_client *client,
> > +				   const struct i2c_device_id *id)
> > +{
> > +	int ret;
> > +	struct vl53l0x_data *data;
> > +	struct iio_dev *indio_dev;
> > +
> > +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	data = iio_priv(indio_dev);
> > +	data->client = client;
> > +	i2c_set_clientdata(client, indio_dev);
> > +	mutex_init(&data->lock);
> > +
> > +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE))
> > +		return -EOPNOTSUPP;
> > +
> > +	indio_dev->dev.parent = &client->dev;
> > +	indio_dev->name = VL53L0X_DRV_NAME;
> > +	indio_dev->info = &vl53l0x_info;
> > +	indio_dev->channels = vl53l0x_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(vl53l0x_channels);
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > +	//probe 0xc0 if the value is 0xEE.
> > +
> > +	ret = iio_device_register(indio_dev);
> 
> Would better be better to use resource managened functions since
> I don't see any point of using vl53l0x_remove() function below.
> Better use devm_iio_device_register()!
> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	dev_set_drvdata(&client->dev, data);
> 
> You already setted it up above using:
> 
> 	i2c_set_clientdata(client, indio_dev);
> 
> > +	return 0;
> > +}
> > +
> > +static int vl53l0x_remove(struct i2c_client *client)
> > +{
> > +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > +	struct vl53l0x_data *data = iio_priv(indio_dev);
> > +
> > +	iio_device_unregister(indio_dev);
> > +	kfree(data);
> > +
> > +	return 0;
> > +}
> 
> Plus all other comments addressed by Andy.
> 
> 
> Thanks
> -- 
> Himanshu Jha
> Undergraduate Student
> Department of Electronics & Communication
> Guru Tegh Bahadur Institute of Technology

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] iio: proximity: Add driver support for ST's VL53L0X ToF ranging sensor.
  2018-09-10 17:57 ` Himanshu Jha
@ 2018-09-11  2:47   ` Song Qiang
  2018-09-11  6:46   ` Song Qiang
  2018-09-11  7:09   ` Song Qiang
  2 siblings, 0 replies; 12+ messages in thread
From: Song Qiang @ 2018-09-11  2:47 UTC (permalink / raw)
  To: Himanshu Jha
  Cc: jic23, knaack.h, lars, pmeerw, andriy.shevchenko, matt.ranostay,
	kstewart, pombredanne, gregkh, ak, linux-iio, linux-kernel

On Mon, Sep 10, 2018 at 11:27:47PM +0530, Himanshu Jha wrote:
> On Mon, Sep 10, 2018 at 10:42:59PM +0800, Song Qiang wrote:
> > This driver was originally written by ST in 2016 as a misc input device,
> > and hasn't been maintained for a long time. I grabbed some code from
> > it's API and reformed it to a iio proximity device driver.
> > This version of driver uses i2c bus to talk to the sensor and
> > polling for measuring completes, so no irq line is needed.
> > This version of driver supports only one-shot mode, and it can be
> > tested with reading from
> > /sys/bus/iio/devices/iio:deviceX/in_distance_raw
> > 
> > Signed-off-by: Song Qiang <songqiang.1304521@gmail.com>
> > ---
> 
> The Cc list contains developers who might not be relevant
> for the discussion.
> 
> So, copy only those people listed by:
> 
> $./scripts/get_maintainer.pl <driver.patch>
> 
> Don't know why Kate & Greg are cc'ed ?
> 

Hi Himanshu,

Since this is a new device driver may going to be added into
drivers/iio/proximity folder, I used drivers/iio/proximity as
the parameter of ./scripts/get_maintainer.pl and it just returned
them. I rechecked it and seems like it says Greg and Kate are
commit_signers. I send patches as Greg's speech "Write and Submit
Your First Linux Kernel Patch" told. So, should I just send the
patches to reviewers that get_maintainer.pl returned and stop cc
all the commit_signers?

> >  .../bindings/iio/proximity/vl53l0x.txt        |  12 +
> >  drivers/iio/proximity/Kconfig                 |  13 +
> >  drivers/iio/proximity/Makefile                |   2 +
> >  drivers/iio/proximity/vl53l0x-i2c.c           | 295 ++++++++++++++++++
> >  4 files changed, 322 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> >  create mode 100644 drivers/iio/proximity/vl53l0x-i2c.c
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > new file mode 100644
> > index 000000000000..64b69442f08e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > @@ -0,0 +1,12 @@
> > +ST's VL53L0X ToF ranging sensor
> > +
> > +Required properties:
> > +	- compatible: must be "st,vl53l0x-i2c"
> > +	- reg: i2c address where to find the device
> > +
> > +Example:
> > +
> > +vl53l0x@29 {
> > +	compatible = "st,vl53l0x-i2c";
> > +	reg = <0x29>;
> > +};
> > diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
> > index f726f9427602..1563a5f9144d 100644
> > --- a/drivers/iio/proximity/Kconfig
> > +++ b/drivers/iio/proximity/Kconfig
> > @@ -79,4 +79,17 @@ config SRF08
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called srf08.
> >  
> > +config VL53L0X_I2C
> > +	tristate "STMicroelectronics VL53L0X ToF ranger sensor (I2C)"
> > +	select IIO_BUFFER
> > +	select IIO_TRIGGERED_BUFFER
> 
> I don't see any buffer/trigger support, so better to remove these
> two options.
> 
> > +	depends on I2C
> > +	help
> > +	  Say Y here to build a driver for STMicroelectronics VL53L0X
> > +	  ToF ranger sensors with i2c interface.
> > +	  This driver can be used to measure the distance of objects.
> > +
> > +	  To compile this driver as a module, choose M here: the
> > +	  module will be called vl53l0x-i2c.
> 
> `name` attribute will be VL53L0X_DRV_NAME(vl53l0x) if OF matching
> is not used to probe the driver.
> 
> >  endmenu
> > diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
> > index 4f4ed45e87ef..7cb771665c8b 100644
> > --- a/drivers/iio/proximity/Makefile
> > +++ b/drivers/iio/proximity/Makefile
> > @@ -10,3 +10,5 @@ obj-$(CONFIG_RFD77402)		+= rfd77402.o
> >  obj-$(CONFIG_SRF04)		+= srf04.o
> >  obj-$(CONFIG_SRF08)		+= srf08.o
> >  obj-$(CONFIG_SX9500)		+= sx9500.o
> > +obj-$(CONFIG_VL53L0X_I2C)		+= vl53l0x-i2c.o
> > +
> > diff --git a/drivers/iio/proximity/vl53l0x-i2c.c b/drivers/iio/proximity/vl53l0x-i2c.c
> > new file mode 100644
> > index 000000000000..c00713041d30
> > --- /dev/null
> > +++ b/drivers/iio/proximity/vl53l0x-i2c.c
> > @@ -0,0 +1,295 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + *  vl53l0x-i2c.c - Support for STM VL53L0X FlightSense TOF
> > + *					Ranger Sensor on a i2c bus.
> > + *
> > + *  Copyright (C) 2016 STMicroelectronics Imaging Division.
> > + *  Copyright (C) 2018 Song Qiang <songqiang.1304521@gmail.com>
> > + *
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +#include <linux/i2c.h>
> > +#include <linux/delay.h>
> > +#include <linux/kernel.h>
> > +#include <linux/iio/iio.h>
> > +
> > +#define VL53L0X_DRV_NAME			"vl53l0x"
> > +
> > +/* Device register map */
> > +#define VL_REG_SYSRANGE_START					0x000
> > +#define VL_REG_SYSRANGE_MODE_MASK				0x0F
> > +#define VL_REG_SYSRANGE_MODE_START_STOP			0x01
> > +#define VL_REG_SYSRANGE_MODE_SINGLESHOT			0x00
> > +#define VL_REG_SYSRANGE_MODE_BACKTOBACK			0x02
> > +#define VL_REG_SYSRANGE_MODE_TIMED				0x04
> > +#define VL_REG_SYSRANGE_MODE_HISTOGRAM			0x08
> > +
> > +#define VL_REG_SYS_THRESH_HIGH					0x000C
> > +#define VL_REG_SYS_THRESH_LOW					0x000E
> > +
> > +#define VL_REG_SYS_SEQUENCE_CFG					0x0001
> > +#define VL_REG_SYS_RANGE_CFG					0x0009
> > +#define VL_REG_SYS_INTERMEASUREMENT_PERIOD		0x0004
> 
> Could you please align all these macros properly.
> 
> > +#define VL_REG_SYS_INT_CFG_GPIO					0x000A
> > +#define VL_REG_SYS_INT_GPIO_DISABLED			0x00
> > +#define VL_REG_SYS_INT_GPIO_LEVEL_LOW			0x01
> > +#define VL_REG_SYS_INT_GPIO_LEVEL_HIGH			0x02
> > +#define VL_REG_SYS_INT_GPIO_OUT_OF_WINDOW		0x03
> > +#define VL_REG_SYS_INT_GPIO_NEW_SAMPLE_READY	0x04
> > +#define VL_REG_GPIO_HV_MUX_ACTIVE_HIGH			0x0084
> > +#define VL_REG_SYS_INT_CLEAR					0x000B
> > +
> > +/* Result registers */
> > +#define VL_REG_RESULT_INT_STATUS				0x0013
> > +#define VL_REG_RESULT_RANGE_STATUS				0x0014
> > +
> > +#define VL_REG_RESULT_CORE_PAGE  1
> > +#define VL_REG_RESULT_CORE_AMBIENT_WINDOW_EVENTS_RTN	0x00BC
> > +#define VL_REG_RESULT_CORE_RANGING_TOTAL_EVENTS_RTN		0x00C0
> > +#define VL_REG_RESULT_CORE_AMBIENT_WINDOW_EVENTS_REF	0x00D0
> > +#define VL_REG_RESULT_CORE_RANGING_TOTAL_EVENTS_REF		0x00D4
> > +#define VL_REG_RESULT_PEAK_SIGNAL_RATE_REF				0x00B6
> > +
> > +/* Algo register */
> > +#define VL_REG_ALGO_PART_TO_PART_RANGE_OFFSET_MM		0x0028
> > +
> > +#define VL_REG_I2C_SLAVE_DEVICE_ADDRESS					0x008a
> > +
> > +/* Check Limit registers */
> > +#define VL_REG_MSRC_CFG_CONTROL						0x0060
> > +
> > +#define VL_REG_PRE_RANGE_CFG_MIN_SNR					0X0027
> > +#define VL_REG_PRE_RANGE_CFG_VALID_PHASE_LOW			0x0056
> > +#define VL_REG_PRE_RANGE_CFG_VALID_PHASE_HIGH			0x0057
> > +#define VL_REG_PRE_RANGE_MIN_COUNT_RATE_RTN_LIMIT		0x0064
> > +
> > +#define VL_REG_FINAL_RANGE_CFG_MIN_SNR					0X0067
> > +#define VL_REG_FINAL_RANGE_CFG_VALID_PHASE_LOW			0x0047
> > +#define VL_REG_FINAL_RANGE_CFG_VALID_PHASE_HIGH			0x0048
> > +#define VL_REG_FINAL_RANGE_CFG_MIN_COUNT_RATE_RTN_LIMIT	0x0044
> > +
> > +#define VL_REG_PRE_RANGE_CFG_SIGMA_THRESH_HI			0X0061
> > +#define VL_REG_PRE_RANGE_CFG_SIGMA_THRESH_LO			0X0062
> > +
> > +/* PRE RANGE registers */
> > +#define VL_REG_PRE_RANGE_CFG_VCSEL_PERIOD				0x0050
> > +#define VL_REG_PRE_RANGE_CFG_TIMEOUT_MACROP_HI			0x0051
> > +#define VL_REG_PRE_RANGE_CFG_TIMEOUT_MACROP_LO			0x0052
> > +
> > +#define VL_REG_SYS_HISTOGRAM_BIN					0x0081
> > +#define VL_REG_HISTOGRAM_CFG_INITIAL_PHASE_SELECT		0x0033
> > +#define VL_REG_HISTOGRAM_CFG_READOUT_CTRL				0x0055
> > +
> > +#define VL_REG_FINAL_RANGE_CFG_VCSEL_PERIOD				0x0070
> > +#define VL_REG_FINAL_RANGE_CFG_TIMEOUT_MACROP_HI		0x0071
> > +#define VL_REG_FINAL_RANGE_CFG_TIMEOUT_MACROP_LO		0x0072
> > +#define VL_REG_CROSSTALK_COMPENSATION_PEAK_RATE_MCPS	0x0020
> > +
> > +#define VL_REG_MSRC_CFG_TIMEOUT_MACROP					0x0046
> > +
> > +#define VL_REG_SOFT_RESET_GO2_SOFT_RESET_N				0x00bf
> > +#define VL_REG_IDENTIFICATION_MODEL_ID					0x00c0
> > +#define VL_REG_IDENTIFICATION_REVISION_ID				0x00c2
> > +
> > +#define VL_REG_OSC_CALIBRATE_VAL					0x00f8
> > +
> > +#define VL_SIGMA_ESTIMATE_MAX_VALUE					65535
> > +/* equivalent to a range sigma of 655.35mm */
> > +
> > +#define VL_REG_GLOBAL_CFG_VCSEL_WIDTH					0x032
> > +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_0			0x0B0
> > +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_1			0x0B1
> > +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_2			0x0B2
> > +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_3			0x0B3
> > +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_4			0x0B4
> > +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_5			0x0B5
> > +#define VL_REG_GLOBAL_CFG_REF_EN_START_SELECT			0xB6
> > +#define VL_REG_DYNAMIC_SPAD_NUM_REQUESTED_REF_SPAD		0x4E /* 0x14E */
> > +#define VL_REG_DYNAMIC_SPAD_REF_EN_START_OFFSET			0x4F /* 0x14F */
> > +#define VL_REG_POWER_MANAGEMENT_GO1_POWER_FORCE			0x80
> > +
> > +/*
> > + * Speed of light in um per 1E-10 Seconds
> > + */
> > +#define VL_SPEED_OF_LIGHT_IN_AIR					2997
> > +#define VL_REG_VHV_CFG_PAD_SCL_SDA__EXTSUP_HV			0x0089
> > +#define VL_REG_ALGO_PHASECAL_LIM			0x0030 /* 0x130 */
> > +#define VL_REG_ALGO_PHASECAL_CFG_TIMEOUT				0x0030
> > +
> > +struct vl53l0x_data {
> > +	struct i2c_client *client;
> > +	struct mutex lock;
> 
> This lock needs a comment to explain its purpose.
> 
> > +	int	useLongRange;
> 
> Weird spacing.
> 
> > +
> > +static int vl53l0x_read_proximity(struct vl53l0x_data *data,
> > +				const struct iio_chan_spec *chan,
> > +				int *val)
> 
> Align all these functions to match open parentheses with mix of
> tabs + whitespaces(as required):
> 
> static int vl53l0x_read_proximity(struct vl53l0x_data *data,
> 				  const struct iio_chan_spec *chan,
> 				  int *val)
> 
> 
> > +	int ret;
> > +	struct i2c_client *client = data->client;
> > +	int tries = 20;
> > +	u8 buffer[12];
> > +	struct i2c_msg msg[2];
> > +	u8 write_command = VL_REG_RESULT_RANGE_STATUS;
> > +
> > +	ret = i2c_smbus_write_byte_data(data->client,
> > +					VL_REG_SYSRANGE_START, 1);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	while (tries-- > 0) {
> > +		ret = i2c_smbus_read_byte_data(data->client,
> > +						VL_REG_RESULT_RANGE_STATUS);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		if (ret & 0x01)
> > +			break;
> > +		usleep_range(1000, 5000);
> > +	}
> > +
> > +	if (tries < 0)
> > +		return -ETIMEDOUT;
> > +
> > +	msg[0].addr = client->addr;
> > +	msg[0].buf = &write_command;
> > +	msg[0].len = 1;
> > +	msg[0].flags = client->flags | I2C_M_STOP;
> > +
> > +	msg[1].addr = client->addr;
> > +	msg[1].buf = buffer;
> > +	msg[1].len = 12;
> > +	msg[1].flags = client->flags | I2C_M_RD;
> > +
> > +	ret = i2c_transfer(client->adapter, msg, 2);
> > +
> > +	if (ret != 2) {
> > +		pr_err("vl53l0x: consecutively read error. ");
> > +		return ret;
> > +	}
> > +
> > +	*val = __le16_to_cpu((buffer[10] << 8) + buffer[11]);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct iio_chan_spec vl53l0x_channels[] = {
> > +	{
> > +		.type = IIO_DISTANCE,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW)
> > +	},
> > +	IIO_CHAN_SOFT_TIMESTAMP(1),
> > +};
> > +
> > +static int vl53l0x_read_raw(struct iio_dev *indio_dev,
> > +				const struct iio_chan_spec *chan,
> > +				int *val, int *val2, long mask)
> > +{
> > +	struct vl53l0x_data *data = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	if (chan->type != IIO_DISTANCE) {
> > +		pr_err("vl53l0x: iio type error");
> > +		return -EINVAL;
> > +	}
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		ret = iio_device_claim_direct_mode(indio_dev);
> > +		if (ret)
> > +			return ret;
> > +		ret = vl53l0x_read_proximity(data, chan, val);
> > +		if (ret < 0)
> > +			pr_err("vl53l0x: raw value read error with %d", ret);
> > +
> > +		ret = IIO_VAL_INT;
> > +		iio_device_release_direct_mode(indio_dev);
> > +		return ret;
> > +	default:
> > +		pr_err("vl53l0x: IIO_CHAN_* not recognzed.");
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static const struct iio_info vl53l0x_info = {
> > +	.read_raw = vl53l0x_read_raw,
> > +};
> > +
> > +static int vl53l0x_probe(struct i2c_client *client,
> > +				   const struct i2c_device_id *id)
> > +{
> > +	int ret;
> > +	struct vl53l0x_data *data;
> > +	struct iio_dev *indio_dev;
> > +
> > +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	data = iio_priv(indio_dev);
> > +	data->client = client;
> > +	i2c_set_clientdata(client, indio_dev);
> > +	mutex_init(&data->lock);
> > +
> > +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE))
> > +		return -EOPNOTSUPP;
> > +
> > +	indio_dev->dev.parent = &client->dev;
> > +	indio_dev->name = VL53L0X_DRV_NAME;
> > +	indio_dev->info = &vl53l0x_info;
> > +	indio_dev->channels = vl53l0x_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(vl53l0x_channels);
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > +	//probe 0xc0 if the value is 0xEE.
> > +
> > +	ret = iio_device_register(indio_dev);
> 
> Would better be better to use resource managened functions since
> I don't see any point of using vl53l0x_remove() function below.
> Better use devm_iio_device_register()!
> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	dev_set_drvdata(&client->dev, data);
> 
> You already setted it up above using:
> 
> 	i2c_set_clientdata(client, indio_dev);
> 
> > +	return 0;
> > +}
> > +
> > +static int vl53l0x_remove(struct i2c_client *client)
> > +{
> > +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > +	struct vl53l0x_data *data = iio_priv(indio_dev);
> > +
> > +	iio_device_unregister(indio_dev);
> > +	kfree(data);
> > +
> > +	return 0;
> > +}
> 
> Plus all other comments addressed by Andy.
> 
> 
> Thanks
> -- 
> Himanshu Jha
> Undergraduate Student
> Department of Electronics & Communication
> Guru Tegh Bahadur Institute of Technology

Sorry for the mess in register table and some old methods I used.
I was trying to use some methods that other drivers use in this
folder to avoid mistakes, and seems like I should learn something new.
I'll cleanup the register table as you and Andy says, replace
old methods and remove the mutex and some comments which is useless
for now.

yours,
Song Qiang

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] iio: proximity: Add driver support for ST's VL53L0X ToF ranging sensor.
  2018-09-10 15:12 ` Andy Shevchenko
@ 2018-09-11  2:19   ` Song Qiang
  0 siblings, 0 replies; 12+ messages in thread
From: Song Qiang @ 2018-09-11  2:19 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: jic23, knaack.h, lars, pmeerw, matt.ranostay, kstewart,
	pombredanne, gregkh, ak, linux-iio, linux-kernel

On Mon, Sep 10, 2018 at 06:12:43PM +0300, Andy Shevchenko wrote:
> On Mon, Sep 10, 2018 at 10:42:59PM +0800, Song Qiang wrote:
> > This driver was originally written by ST in 2016 as a misc input device,
> > and hasn't been maintained for a long time. I grabbed some code from
> > it's API and reformed it to a iio proximity device driver.
> > This version of driver uses i2c bus to talk to the sensor and
> > polling for measuring completes, so no irq line is needed.
> > This version of driver supports only one-shot mode, and it can be
> > tested with reading from
> > /sys/bus/iio/devices/iio:deviceX/in_distance_raw
> 
> Brief review for almost style issues...
> 
> > + *  vl53l0x-i2c.c - Support for STM VL53L0X FlightSense TOF
> > + *					Ranger Sensor on a i2c bus.
> 
> One line and without file name.
> 
> > + *
> > + *  Copyright (C) 2016 STMicroelectronics Imaging Division.
> > + *  Copyright (C) 2018 Song Qiang <songqiang.1304521@gmail.com>
> 
> > + *
> 
> Redundant
> 
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +#include <linux/i2c.h>
> > +#include <linux/delay.h>
> > +#include <linux/kernel.h>
> 
> Keep above sorted.
> 
> > +#include <linux/iio/iio.h>
> > +
> > +#define VL53L0X_DRV_NAME			"vl53l0x"
> > +
> > +/* Device register map */
> > +#define VL_REG_SYSRANGE_START					0x000
> 
> 0x0000 ?
> 
> > +#define VL_REG_SYSRANGE_MODE_MASK				0x0F
> 
> GENMASK() ?
> 
> > +#define VL_REG_SYSRANGE_MODE_START_STOP			0x01
> > +#define VL_REG_SYSRANGE_MODE_SINGLESHOT			0x00
> > +#define VL_REG_SYSRANGE_MODE_BACKTOBACK			0x02
> > +#define VL_REG_SYSRANGE_MODE_TIMED				0x04
> > +#define VL_REG_SYSRANGE_MODE_HISTOGRAM			0x08
> 
> BIT() ?
> 
> Above comments related to below definitions as well.
> 
> > +
> > +#define VL_REG_SYS_THRESH_HIGH					0x000C
> > +#define VL_REG_SYS_THRESH_LOW					0x000E
> > +
> > +#define VL_REG_SYS_SEQUENCE_CFG					0x0001
> > +#define VL_REG_SYS_RANGE_CFG					0x0009
> > +#define VL_REG_SYS_INTERMEASUREMENT_PERIOD		0x0004
> > +
> > +#define VL_REG_SYS_INT_CFG_GPIO					0x000A
> 
> If you chose 0xHHHH format for the registers, please, keep the list of them sorted by the offset / address.
> 
> > +#define VL_REG_SYS_INT_GPIO_DISABLED			0x00
> > +#define VL_REG_SYS_INT_GPIO_LEVEL_LOW			0x01
> > +#define VL_REG_SYS_INT_GPIO_LEVEL_HIGH			0x02
> > +#define VL_REG_SYS_INT_GPIO_OUT_OF_WINDOW		0x03
> > +#define VL_REG_SYS_INT_GPIO_NEW_SAMPLE_READY	0x04
> > +#define VL_REG_GPIO_HV_MUX_ACTIVE_HIGH			0x0084
> > +#define VL_REG_SYS_INT_CLEAR					0x000B
> > +
> > +/* Result registers */
> > +#define VL_REG_RESULT_INT_STATUS				0x0013
> > +#define VL_REG_RESULT_RANGE_STATUS				0x0014
> > +
> > +#define VL_REG_RESULT_CORE_PAGE  1
> > +#define VL_REG_RESULT_CORE_AMBIENT_WINDOW_EVENTS_RTN	0x00BC
> > +#define VL_REG_RESULT_CORE_RANGING_TOTAL_EVENTS_RTN		0x00C0
> > +#define VL_REG_RESULT_CORE_AMBIENT_WINDOW_EVENTS_REF	0x00D0
> > +#define VL_REG_RESULT_CORE_RANGING_TOTAL_EVENTS_REF		0x00D4
> > +#define VL_REG_RESULT_PEAK_SIGNAL_RATE_REF				0x00B6
> > +
> > +/* Algo register */
> > +#define VL_REG_ALGO_PART_TO_PART_RANGE_OFFSET_MM		0x0028
> > +
> > +#define VL_REG_I2C_SLAVE_DEVICE_ADDRESS					0x008a
> > +
> > +/* Check Limit registers */
> > +#define VL_REG_MSRC_CFG_CONTROL						0x0060
> > +
> > +#define VL_REG_PRE_RANGE_CFG_MIN_SNR					0X0027
> > +#define VL_REG_PRE_RANGE_CFG_VALID_PHASE_LOW			0x0056
> > +#define VL_REG_PRE_RANGE_CFG_VALID_PHASE_HIGH			0x0057
> > +#define VL_REG_PRE_RANGE_MIN_COUNT_RATE_RTN_LIMIT		0x0064
> > +
> > +#define VL_REG_FINAL_RANGE_CFG_MIN_SNR					0X0067
> > +#define VL_REG_FINAL_RANGE_CFG_VALID_PHASE_LOW			0x0047
> > +#define VL_REG_FINAL_RANGE_CFG_VALID_PHASE_HIGH			0x0048
> > +#define VL_REG_FINAL_RANGE_CFG_MIN_COUNT_RATE_RTN_LIMIT	0x0044
> > +
> 
> > +#define VL_REG_PRE_RANGE_CFG_SIGMA_THRESH_HI			0X0061
> > +#define VL_REG_PRE_RANGE_CFG_SIGMA_THRESH_LO			0X0062
> 
> 0x
> 
> > +
> > +/* PRE RANGE registers */
> > +#define VL_REG_PRE_RANGE_CFG_VCSEL_PERIOD				0x0050
> > +#define VL_REG_PRE_RANGE_CFG_TIMEOUT_MACROP_HI			0x0051
> > +#define VL_REG_PRE_RANGE_CFG_TIMEOUT_MACROP_LO			0x0052
> > +
> > +#define VL_REG_SYS_HISTOGRAM_BIN					0x0081
> > +#define VL_REG_HISTOGRAM_CFG_INITIAL_PHASE_SELECT		0x0033
> > +#define VL_REG_HISTOGRAM_CFG_READOUT_CTRL				0x0055
> > +
> > +#define VL_REG_FINAL_RANGE_CFG_VCSEL_PERIOD				0x0070
> > +#define VL_REG_FINAL_RANGE_CFG_TIMEOUT_MACROP_HI		0x0071
> > +#define VL_REG_FINAL_RANGE_CFG_TIMEOUT_MACROP_LO		0x0072
> > +#define VL_REG_CROSSTALK_COMPENSATION_PEAK_RATE_MCPS	0x0020
> > +
> > +#define VL_REG_MSRC_CFG_TIMEOUT_MACROP					0x0046
> > +
> > +#define VL_REG_SOFT_RESET_GO2_SOFT_RESET_N				0x00bf
> > +#define VL_REG_IDENTIFICATION_MODEL_ID					0x00c0
> > +#define VL_REG_IDENTIFICATION_REVISION_ID				0x00c2
> > +
> > +#define VL_REG_OSC_CALIBRATE_VAL					0x00f8
> > +
> > +#define VL_SIGMA_ESTIMATE_MAX_VALUE					65535
> > +/* equivalent to a range sigma of 655.35mm */
> > +
> > +#define VL_REG_GLOBAL_CFG_VCSEL_WIDTH					0x032
> > +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_0			0x0B0
> > +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_1			0x0B1
> > +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_2			0x0B2
> > +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_3			0x0B3
> > +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_4			0x0B4
> > +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_5			0x0B5
> > +#define VL_REG_GLOBAL_CFG_REF_EN_START_SELECT			0xB6
> 
> 0x00 or 0x for prefix?
> 
> > +#define VL_REG_DYNAMIC_SPAD_NUM_REQUESTED_REF_SPAD		0x4E /* 0x14E */
> > +#define VL_REG_DYNAMIC_SPAD_REF_EN_START_OFFSET			0x4F /* 0x14F */
> > +#define VL_REG_POWER_MANAGEMENT_GO1_POWER_FORCE			0x80
> > +
> > +/*
> > + * Speed of light in um per 1E-10 Seconds
> > + */
> > +#define VL_SPEED_OF_LIGHT_IN_AIR					2997
> > +#define VL_REG_VHV_CFG_PAD_SCL_SDA__EXTSUP_HV			0x0089
> > +#define VL_REG_ALGO_PHASECAL_LIM			0x0030 /* 0x130 */
> > +#define VL_REG_ALGO_PHASECAL_CFG_TIMEOUT				0x0030
> > +
> > +struct vl53l0x_data {
> > +	struct i2c_client *client;
> > +	struct mutex lock;
> > +	int	useLongRange;
> 
> noCamelCasePlease.
> 
> > +};
> > +
> > +static int vl53l0x_read_proximity(struct vl53l0x_data *data,
> > +				const struct iio_chan_spec *chan,
> > +				int *val)
> > +{
> > +	int ret;
> > +	struct i2c_client *client = data->client;
> > +	int tries = 20;
> 
> unsigned int
> 
> > +	u8 buffer[12];
> > +	struct i2c_msg msg[2];
> > +	u8 write_command = VL_REG_RESULT_RANGE_STATUS;
> 
> Reversed xmas tree order, please
> 
> > +
> > +	ret = i2c_smbus_write_byte_data(data->client,
> > +					VL_REG_SYSRANGE_START, 1);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	while (tries-- > 0) {
> 
> 
> Better form is
> 
> do {
> 	...
> } while (--tries);
> if (!tries)
> 	return -ETIMEDOUT;
> 
> > +		ret = i2c_smbus_read_byte_data(data->client,
> > +						VL_REG_RESULT_RANGE_STATUS);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		if (ret & 0x01)
> > +			break;
> > +		usleep_range(1000, 5000);
> > +	}
> > +
> > +	if (tries < 0)
> > +		return -ETIMEDOUT;
> > +
> > +	msg[0].addr = client->addr;
> > +	msg[0].buf = &write_command;
> > +	msg[0].len = 1;
> > +	msg[0].flags = client->flags | I2C_M_STOP;
> > +
> > +	msg[1].addr = client->addr;
> > +	msg[1].buf = buffer;
> > +	msg[1].len = 12;
> > +	msg[1].flags = client->flags | I2C_M_RD;
> > +
> > +	ret = i2c_transfer(client->adapter, msg, 2);
> > +
> 
> > +	if (ret != 2) {
> 
> Shouldn't be simple if (ret < 0)
> 
> > +		pr_err("vl53l0x: consecutively read error. ");
> 
> dev_err(...);
> Also check other prints.
> 
> > +		return ret;
> > +	}
> > +
> > +	*val = __le16_to_cpu((buffer[10] << 8) + buffer[11]);
> > +
> > +	return 0;
> > +}
> 
> > +static int vl53l0x_probe(struct i2c_client *client,
> > +				   const struct i2c_device_id *id)
> 
> (see below about ->probe_new() hook)
> 
> > +{
> > +	int ret;
> > +	struct vl53l0x_data *data;
> > +	struct iio_dev *indio_dev;
> > +
> > +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	data = iio_priv(indio_dev);
> > +	data->client = client;
> > +	i2c_set_clientdata(client, indio_dev);
> > +	mutex_init(&data->lock);
> > +
> > +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE))
> > +		return -EOPNOTSUPP;
> > +
> > +	indio_dev->dev.parent = &client->dev;
> > +	indio_dev->name = VL53L0X_DRV_NAME;
> > +	indio_dev->info = &vl53l0x_info;
> > +	indio_dev->channels = vl53l0x_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(vl53l0x_channels);
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> 
> > +	//probe 0xc0 if the value is 0xEE.
> 
> Useless commwnt  for now
> 
> > +
> > +	ret = iio_device_register(indio_dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	dev_set_drvdata(&client->dev, data);
> > +
> > +	return 0;
> > +}
> > +
> > +static int vl53l0x_remove(struct i2c_client *client)
> > +{
> > +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > +	struct vl53l0x_data *data = iio_priv(indio_dev);
> > +
> > +	iio_device_unregister(indio_dev);
> 
> > +	kfree(data);
> 
> how did you test it?
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct i2c_device_id vl53l0x_id[] = {
> > +	{ VL53L0X_DRV_NAME, 0 },
> 
> > +	{ },
> Terminator better w/o comma
> 
> > +};
> > +MODULE_DEVICE_TABLE(i2c, vl53l0x_id);
> 
> Do you expect this to be used at all?
> 
> > +
> > +static const struct of_device_id st_vl53l0x_dt_match[] = {
> > +	{ .compatible = "st,vl53l0x-i2c", },
> 
> > +	{ },
> 
> Terminator better w/o comma
> 
> > +};
> > +
> > +static struct i2c_driver vl53l0x_driver = {
> > +	.driver = {
> > +		.name	= VL53L0X_DRV_NAME,
> 
> > +		.owner	= THIS_MODULE,
> 
> This done by macro.
> 
> > +		.of_match_table = st_vl53l0x_dt_match,
> > +	},
> > +	.probe	= vl53l0x_probe,
> 
> ->probe_new(), please.
> 
> > +	.remove	= vl53l0x_remove,
> > +	.id_table = vl53l0x_id,
> > +};
> > +module_i2c_driver(vl53l0x_driver);
> > +
> > +MODULE_AUTHOR("Song Qiang <songqiang.1304521@gmail.com>");
> > +MODULE_DESCRIPTION("ST vl53l0x ToF ranging sensor");
> > +MODULE_LICENSE("GPL");
> > -- 
> > 2.17.1
> > 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Hi Andy,

Sorry for the mess in register table, I took it from ST's
API and has only checked it with the checkpatch.pl. 
I didn't know about probe_new() when I was writing this driver,
I'll see what it does and try to use it. 
I'll check the other problems you listed and clean up the
register table.

Thanks a lot for the reviewing.

Yours,
Song Qiang

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] iio: proximity: Add driver support for ST's VL53L0X ToF ranging sensor.
  2018-09-10 14:42 Song Qiang
  2018-09-10 15:12 ` Andy Shevchenko
@ 2018-09-10 17:57 ` Himanshu Jha
  2018-09-11  2:47   ` Song Qiang
                     ` (2 more replies)
  1 sibling, 3 replies; 12+ messages in thread
From: Himanshu Jha @ 2018-09-10 17:57 UTC (permalink / raw)
  To: Song Qiang
  Cc: jic23, knaack.h, lars, pmeerw, andriy.shevchenko, matt.ranostay,
	robh, kstewart, gregkh, linux-iio, linux-kernel, Song Qiang

On Mon, Sep 10, 2018 at 10:42:59PM +0800, Song Qiang wrote:
> This driver was originally written by ST in 2016 as a misc input device,
> and hasn't been maintained for a long time. I grabbed some code from
> it's API and reformed it to a iio proximity device driver.
> This version of driver uses i2c bus to talk to the sensor and
> polling for measuring completes, so no irq line is needed.
> This version of driver supports only one-shot mode, and it can be
> tested with reading from
> /sys/bus/iio/devices/iio:deviceX/in_distance_raw
> 
> Signed-off-by: Song Qiang <songqiang.1304521@gmail.com>
> ---

The Cc list contains developers who might not be relevant
for the discussion.

So, copy only those people listed by:

$./scripts/get_maintainer.pl <driver.patch>

Don't know why Kate & Greg are cc'ed ?

>  .../bindings/iio/proximity/vl53l0x.txt        |  12 +
>  drivers/iio/proximity/Kconfig                 |  13 +
>  drivers/iio/proximity/Makefile                |   2 +
>  drivers/iio/proximity/vl53l0x-i2c.c           | 295 ++++++++++++++++++
>  4 files changed, 322 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
>  create mode 100644 drivers/iio/proximity/vl53l0x-i2c.c
> 
> diff --git a/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> new file mode 100644
> index 000000000000..64b69442f08e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> @@ -0,0 +1,12 @@
> +ST's VL53L0X ToF ranging sensor
> +
> +Required properties:
> +	- compatible: must be "st,vl53l0x-i2c"
> +	- reg: i2c address where to find the device
> +
> +Example:
> +
> +vl53l0x@29 {
> +	compatible = "st,vl53l0x-i2c";
> +	reg = <0x29>;
> +};
> diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
> index f726f9427602..1563a5f9144d 100644
> --- a/drivers/iio/proximity/Kconfig
> +++ b/drivers/iio/proximity/Kconfig
> @@ -79,4 +79,17 @@ config SRF08
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called srf08.
>  
> +config VL53L0X_I2C
> +	tristate "STMicroelectronics VL53L0X ToF ranger sensor (I2C)"
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER

I don't see any buffer/trigger support, so better to remove these
two options.

> +	depends on I2C
> +	help
> +	  Say Y here to build a driver for STMicroelectronics VL53L0X
> +	  ToF ranger sensors with i2c interface.
> +	  This driver can be used to measure the distance of objects.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called vl53l0x-i2c.

`name` attribute will be VL53L0X_DRV_NAME(vl53l0x) if OF matching
is not used to probe the driver.

>  endmenu
> diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
> index 4f4ed45e87ef..7cb771665c8b 100644
> --- a/drivers/iio/proximity/Makefile
> +++ b/drivers/iio/proximity/Makefile
> @@ -10,3 +10,5 @@ obj-$(CONFIG_RFD77402)		+= rfd77402.o
>  obj-$(CONFIG_SRF04)		+= srf04.o
>  obj-$(CONFIG_SRF08)		+= srf08.o
>  obj-$(CONFIG_SX9500)		+= sx9500.o
> +obj-$(CONFIG_VL53L0X_I2C)		+= vl53l0x-i2c.o
> +
> diff --git a/drivers/iio/proximity/vl53l0x-i2c.c b/drivers/iio/proximity/vl53l0x-i2c.c
> new file mode 100644
> index 000000000000..c00713041d30
> --- /dev/null
> +++ b/drivers/iio/proximity/vl53l0x-i2c.c
> @@ -0,0 +1,295 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + *  vl53l0x-i2c.c - Support for STM VL53L0X FlightSense TOF
> + *					Ranger Sensor on a i2c bus.
> + *
> + *  Copyright (C) 2016 STMicroelectronics Imaging Division.
> + *  Copyright (C) 2018 Song Qiang <songqiang.1304521@gmail.com>
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/delay.h>
> +#include <linux/kernel.h>
> +#include <linux/iio/iio.h>
> +
> +#define VL53L0X_DRV_NAME			"vl53l0x"
> +
> +/* Device register map */
> +#define VL_REG_SYSRANGE_START					0x000
> +#define VL_REG_SYSRANGE_MODE_MASK				0x0F
> +#define VL_REG_SYSRANGE_MODE_START_STOP			0x01
> +#define VL_REG_SYSRANGE_MODE_SINGLESHOT			0x00
> +#define VL_REG_SYSRANGE_MODE_BACKTOBACK			0x02
> +#define VL_REG_SYSRANGE_MODE_TIMED				0x04
> +#define VL_REG_SYSRANGE_MODE_HISTOGRAM			0x08
> +
> +#define VL_REG_SYS_THRESH_HIGH					0x000C
> +#define VL_REG_SYS_THRESH_LOW					0x000E
> +
> +#define VL_REG_SYS_SEQUENCE_CFG					0x0001
> +#define VL_REG_SYS_RANGE_CFG					0x0009
> +#define VL_REG_SYS_INTERMEASUREMENT_PERIOD		0x0004

Could you please align all these macros properly.

> +#define VL_REG_SYS_INT_CFG_GPIO					0x000A
> +#define VL_REG_SYS_INT_GPIO_DISABLED			0x00
> +#define VL_REG_SYS_INT_GPIO_LEVEL_LOW			0x01
> +#define VL_REG_SYS_INT_GPIO_LEVEL_HIGH			0x02
> +#define VL_REG_SYS_INT_GPIO_OUT_OF_WINDOW		0x03
> +#define VL_REG_SYS_INT_GPIO_NEW_SAMPLE_READY	0x04
> +#define VL_REG_GPIO_HV_MUX_ACTIVE_HIGH			0x0084
> +#define VL_REG_SYS_INT_CLEAR					0x000B
> +
> +/* Result registers */
> +#define VL_REG_RESULT_INT_STATUS				0x0013
> +#define VL_REG_RESULT_RANGE_STATUS				0x0014
> +
> +#define VL_REG_RESULT_CORE_PAGE  1
> +#define VL_REG_RESULT_CORE_AMBIENT_WINDOW_EVENTS_RTN	0x00BC
> +#define VL_REG_RESULT_CORE_RANGING_TOTAL_EVENTS_RTN		0x00C0
> +#define VL_REG_RESULT_CORE_AMBIENT_WINDOW_EVENTS_REF	0x00D0
> +#define VL_REG_RESULT_CORE_RANGING_TOTAL_EVENTS_REF		0x00D4
> +#define VL_REG_RESULT_PEAK_SIGNAL_RATE_REF				0x00B6
> +
> +/* Algo register */
> +#define VL_REG_ALGO_PART_TO_PART_RANGE_OFFSET_MM		0x0028
> +
> +#define VL_REG_I2C_SLAVE_DEVICE_ADDRESS					0x008a
> +
> +/* Check Limit registers */
> +#define VL_REG_MSRC_CFG_CONTROL						0x0060
> +
> +#define VL_REG_PRE_RANGE_CFG_MIN_SNR					0X0027
> +#define VL_REG_PRE_RANGE_CFG_VALID_PHASE_LOW			0x0056
> +#define VL_REG_PRE_RANGE_CFG_VALID_PHASE_HIGH			0x0057
> +#define VL_REG_PRE_RANGE_MIN_COUNT_RATE_RTN_LIMIT		0x0064
> +
> +#define VL_REG_FINAL_RANGE_CFG_MIN_SNR					0X0067
> +#define VL_REG_FINAL_RANGE_CFG_VALID_PHASE_LOW			0x0047
> +#define VL_REG_FINAL_RANGE_CFG_VALID_PHASE_HIGH			0x0048
> +#define VL_REG_FINAL_RANGE_CFG_MIN_COUNT_RATE_RTN_LIMIT	0x0044
> +
> +#define VL_REG_PRE_RANGE_CFG_SIGMA_THRESH_HI			0X0061
> +#define VL_REG_PRE_RANGE_CFG_SIGMA_THRESH_LO			0X0062
> +
> +/* PRE RANGE registers */
> +#define VL_REG_PRE_RANGE_CFG_VCSEL_PERIOD				0x0050
> +#define VL_REG_PRE_RANGE_CFG_TIMEOUT_MACROP_HI			0x0051
> +#define VL_REG_PRE_RANGE_CFG_TIMEOUT_MACROP_LO			0x0052
> +
> +#define VL_REG_SYS_HISTOGRAM_BIN					0x0081
> +#define VL_REG_HISTOGRAM_CFG_INITIAL_PHASE_SELECT		0x0033
> +#define VL_REG_HISTOGRAM_CFG_READOUT_CTRL				0x0055
> +
> +#define VL_REG_FINAL_RANGE_CFG_VCSEL_PERIOD				0x0070
> +#define VL_REG_FINAL_RANGE_CFG_TIMEOUT_MACROP_HI		0x0071
> +#define VL_REG_FINAL_RANGE_CFG_TIMEOUT_MACROP_LO		0x0072
> +#define VL_REG_CROSSTALK_COMPENSATION_PEAK_RATE_MCPS	0x0020
> +
> +#define VL_REG_MSRC_CFG_TIMEOUT_MACROP					0x0046
> +
> +#define VL_REG_SOFT_RESET_GO2_SOFT_RESET_N				0x00bf
> +#define VL_REG_IDENTIFICATION_MODEL_ID					0x00c0
> +#define VL_REG_IDENTIFICATION_REVISION_ID				0x00c2
> +
> +#define VL_REG_OSC_CALIBRATE_VAL					0x00f8
> +
> +#define VL_SIGMA_ESTIMATE_MAX_VALUE					65535
> +/* equivalent to a range sigma of 655.35mm */
> +
> +#define VL_REG_GLOBAL_CFG_VCSEL_WIDTH					0x032
> +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_0			0x0B0
> +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_1			0x0B1
> +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_2			0x0B2
> +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_3			0x0B3
> +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_4			0x0B4
> +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_5			0x0B5
> +#define VL_REG_GLOBAL_CFG_REF_EN_START_SELECT			0xB6
> +#define VL_REG_DYNAMIC_SPAD_NUM_REQUESTED_REF_SPAD		0x4E /* 0x14E */
> +#define VL_REG_DYNAMIC_SPAD_REF_EN_START_OFFSET			0x4F /* 0x14F */
> +#define VL_REG_POWER_MANAGEMENT_GO1_POWER_FORCE			0x80
> +
> +/*
> + * Speed of light in um per 1E-10 Seconds
> + */
> +#define VL_SPEED_OF_LIGHT_IN_AIR					2997
> +#define VL_REG_VHV_CFG_PAD_SCL_SDA__EXTSUP_HV			0x0089
> +#define VL_REG_ALGO_PHASECAL_LIM			0x0030 /* 0x130 */
> +#define VL_REG_ALGO_PHASECAL_CFG_TIMEOUT				0x0030
> +
> +struct vl53l0x_data {
> +	struct i2c_client *client;
> +	struct mutex lock;

This lock needs a comment to explain its purpose.

> +	int	useLongRange;

Weird spacing.

> +
> +static int vl53l0x_read_proximity(struct vl53l0x_data *data,
> +				const struct iio_chan_spec *chan,
> +				int *val)

Align all these functions to match open parentheses with mix of
tabs + whitespaces(as required):

static int vl53l0x_read_proximity(struct vl53l0x_data *data,
				  const struct iio_chan_spec *chan,
				  int *val)


> +	int ret;
> +	struct i2c_client *client = data->client;
> +	int tries = 20;
> +	u8 buffer[12];
> +	struct i2c_msg msg[2];
> +	u8 write_command = VL_REG_RESULT_RANGE_STATUS;
> +
> +	ret = i2c_smbus_write_byte_data(data->client,
> +					VL_REG_SYSRANGE_START, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	while (tries-- > 0) {
> +		ret = i2c_smbus_read_byte_data(data->client,
> +						VL_REG_RESULT_RANGE_STATUS);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (ret & 0x01)
> +			break;
> +		usleep_range(1000, 5000);
> +	}
> +
> +	if (tries < 0)
> +		return -ETIMEDOUT;
> +
> +	msg[0].addr = client->addr;
> +	msg[0].buf = &write_command;
> +	msg[0].len = 1;
> +	msg[0].flags = client->flags | I2C_M_STOP;
> +
> +	msg[1].addr = client->addr;
> +	msg[1].buf = buffer;
> +	msg[1].len = 12;
> +	msg[1].flags = client->flags | I2C_M_RD;
> +
> +	ret = i2c_transfer(client->adapter, msg, 2);
> +
> +	if (ret != 2) {
> +		pr_err("vl53l0x: consecutively read error. ");
> +		return ret;
> +	}
> +
> +	*val = __le16_to_cpu((buffer[10] << 8) + buffer[11]);
> +
> +	return 0;
> +}
> +
> +static const struct iio_chan_spec vl53l0x_channels[] = {
> +	{
> +		.type = IIO_DISTANCE,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW)
> +	},
> +	IIO_CHAN_SOFT_TIMESTAMP(1),
> +};
> +
> +static int vl53l0x_read_raw(struct iio_dev *indio_dev,
> +				const struct iio_chan_spec *chan,
> +				int *val, int *val2, long mask)
> +{
> +	struct vl53l0x_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (chan->type != IIO_DISTANCE) {
> +		pr_err("vl53l0x: iio type error");
> +		return -EINVAL;
> +	}
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = iio_device_claim_direct_mode(indio_dev);
> +		if (ret)
> +			return ret;
> +		ret = vl53l0x_read_proximity(data, chan, val);
> +		if (ret < 0)
> +			pr_err("vl53l0x: raw value read error with %d", ret);
> +
> +		ret = IIO_VAL_INT;
> +		iio_device_release_direct_mode(indio_dev);
> +		return ret;
> +	default:
> +		pr_err("vl53l0x: IIO_CHAN_* not recognzed.");
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info vl53l0x_info = {
> +	.read_raw = vl53l0x_read_raw,
> +};
> +
> +static int vl53l0x_probe(struct i2c_client *client,
> +				   const struct i2c_device_id *id)
> +{
> +	int ret;
> +	struct vl53l0x_data *data;
> +	struct iio_dev *indio_dev;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	data->client = client;
> +	i2c_set_clientdata(client, indio_dev);
> +	mutex_init(&data->lock);
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE))
> +		return -EOPNOTSUPP;
> +
> +	indio_dev->dev.parent = &client->dev;
> +	indio_dev->name = VL53L0X_DRV_NAME;
> +	indio_dev->info = &vl53l0x_info;
> +	indio_dev->channels = vl53l0x_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(vl53l0x_channels);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	//probe 0xc0 if the value is 0xEE.
> +
> +	ret = iio_device_register(indio_dev);

Would better be better to use resource managened functions since
I don't see any point of using vl53l0x_remove() function below.
Better use devm_iio_device_register()!

> +	if (ret)
> +		return ret;
> +
> +	dev_set_drvdata(&client->dev, data);

You already setted it up above using:

	i2c_set_clientdata(client, indio_dev);

> +	return 0;
> +}
> +
> +static int vl53l0x_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +	struct vl53l0x_data *data = iio_priv(indio_dev);
> +
> +	iio_device_unregister(indio_dev);
> +	kfree(data);
> +
> +	return 0;
> +}

Plus all other comments addressed by Andy.


Thanks
-- 
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] iio: proximity: Add driver support for ST's VL53L0X ToF ranging sensor.
  2018-09-10 14:42 Song Qiang
@ 2018-09-10 15:12 ` Andy Shevchenko
  2018-09-11  2:19   ` Song Qiang
  2018-09-10 17:57 ` Himanshu Jha
  1 sibling, 1 reply; 12+ messages in thread
From: Andy Shevchenko @ 2018-09-10 15:12 UTC (permalink / raw)
  To: Song Qiang
  Cc: jic23, knaack.h, lars, pmeerw, matt.ranostay, robh, kstewart,
	gregkh, linux-iio, linux-kernel, Song Qiang

On Mon, Sep 10, 2018 at 10:42:59PM +0800, Song Qiang wrote:
> This driver was originally written by ST in 2016 as a misc input device,
> and hasn't been maintained for a long time. I grabbed some code from
> it's API and reformed it to a iio proximity device driver.
> This version of driver uses i2c bus to talk to the sensor and
> polling for measuring completes, so no irq line is needed.
> This version of driver supports only one-shot mode, and it can be
> tested with reading from
> /sys/bus/iio/devices/iio:deviceX/in_distance_raw

Brief review for almost style issues...

> + *  vl53l0x-i2c.c - Support for STM VL53L0X FlightSense TOF
> + *					Ranger Sensor on a i2c bus.

One line and without file name.

> + *
> + *  Copyright (C) 2016 STMicroelectronics Imaging Division.
> + *  Copyright (C) 2018 Song Qiang <songqiang.1304521@gmail.com>

> + *

Redundant

> + */
> +
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/delay.h>
> +#include <linux/kernel.h>

Keep above sorted.

> +#include <linux/iio/iio.h>
> +
> +#define VL53L0X_DRV_NAME			"vl53l0x"
> +
> +/* Device register map */
> +#define VL_REG_SYSRANGE_START					0x000

0x0000 ?

> +#define VL_REG_SYSRANGE_MODE_MASK				0x0F

GENMASK() ?

> +#define VL_REG_SYSRANGE_MODE_START_STOP			0x01
> +#define VL_REG_SYSRANGE_MODE_SINGLESHOT			0x00
> +#define VL_REG_SYSRANGE_MODE_BACKTOBACK			0x02
> +#define VL_REG_SYSRANGE_MODE_TIMED				0x04
> +#define VL_REG_SYSRANGE_MODE_HISTOGRAM			0x08

BIT() ?

Above comments related to below definitions as well.

> +
> +#define VL_REG_SYS_THRESH_HIGH					0x000C
> +#define VL_REG_SYS_THRESH_LOW					0x000E
> +
> +#define VL_REG_SYS_SEQUENCE_CFG					0x0001
> +#define VL_REG_SYS_RANGE_CFG					0x0009
> +#define VL_REG_SYS_INTERMEASUREMENT_PERIOD		0x0004
> +
> +#define VL_REG_SYS_INT_CFG_GPIO					0x000A

If you chose 0xHHHH format for the registers, please, keep the list of them sorted by the offset / address.

> +#define VL_REG_SYS_INT_GPIO_DISABLED			0x00
> +#define VL_REG_SYS_INT_GPIO_LEVEL_LOW			0x01
> +#define VL_REG_SYS_INT_GPIO_LEVEL_HIGH			0x02
> +#define VL_REG_SYS_INT_GPIO_OUT_OF_WINDOW		0x03
> +#define VL_REG_SYS_INT_GPIO_NEW_SAMPLE_READY	0x04
> +#define VL_REG_GPIO_HV_MUX_ACTIVE_HIGH			0x0084
> +#define VL_REG_SYS_INT_CLEAR					0x000B
> +
> +/* Result registers */
> +#define VL_REG_RESULT_INT_STATUS				0x0013
> +#define VL_REG_RESULT_RANGE_STATUS				0x0014
> +
> +#define VL_REG_RESULT_CORE_PAGE  1
> +#define VL_REG_RESULT_CORE_AMBIENT_WINDOW_EVENTS_RTN	0x00BC
> +#define VL_REG_RESULT_CORE_RANGING_TOTAL_EVENTS_RTN		0x00C0
> +#define VL_REG_RESULT_CORE_AMBIENT_WINDOW_EVENTS_REF	0x00D0
> +#define VL_REG_RESULT_CORE_RANGING_TOTAL_EVENTS_REF		0x00D4
> +#define VL_REG_RESULT_PEAK_SIGNAL_RATE_REF				0x00B6
> +
> +/* Algo register */
> +#define VL_REG_ALGO_PART_TO_PART_RANGE_OFFSET_MM		0x0028
> +
> +#define VL_REG_I2C_SLAVE_DEVICE_ADDRESS					0x008a
> +
> +/* Check Limit registers */
> +#define VL_REG_MSRC_CFG_CONTROL						0x0060
> +
> +#define VL_REG_PRE_RANGE_CFG_MIN_SNR					0X0027
> +#define VL_REG_PRE_RANGE_CFG_VALID_PHASE_LOW			0x0056
> +#define VL_REG_PRE_RANGE_CFG_VALID_PHASE_HIGH			0x0057
> +#define VL_REG_PRE_RANGE_MIN_COUNT_RATE_RTN_LIMIT		0x0064
> +
> +#define VL_REG_FINAL_RANGE_CFG_MIN_SNR					0X0067
> +#define VL_REG_FINAL_RANGE_CFG_VALID_PHASE_LOW			0x0047
> +#define VL_REG_FINAL_RANGE_CFG_VALID_PHASE_HIGH			0x0048
> +#define VL_REG_FINAL_RANGE_CFG_MIN_COUNT_RATE_RTN_LIMIT	0x0044
> +

> +#define VL_REG_PRE_RANGE_CFG_SIGMA_THRESH_HI			0X0061
> +#define VL_REG_PRE_RANGE_CFG_SIGMA_THRESH_LO			0X0062

0x

> +
> +/* PRE RANGE registers */
> +#define VL_REG_PRE_RANGE_CFG_VCSEL_PERIOD				0x0050
> +#define VL_REG_PRE_RANGE_CFG_TIMEOUT_MACROP_HI			0x0051
> +#define VL_REG_PRE_RANGE_CFG_TIMEOUT_MACROP_LO			0x0052
> +
> +#define VL_REG_SYS_HISTOGRAM_BIN					0x0081
> +#define VL_REG_HISTOGRAM_CFG_INITIAL_PHASE_SELECT		0x0033
> +#define VL_REG_HISTOGRAM_CFG_READOUT_CTRL				0x0055
> +
> +#define VL_REG_FINAL_RANGE_CFG_VCSEL_PERIOD				0x0070
> +#define VL_REG_FINAL_RANGE_CFG_TIMEOUT_MACROP_HI		0x0071
> +#define VL_REG_FINAL_RANGE_CFG_TIMEOUT_MACROP_LO		0x0072
> +#define VL_REG_CROSSTALK_COMPENSATION_PEAK_RATE_MCPS	0x0020
> +
> +#define VL_REG_MSRC_CFG_TIMEOUT_MACROP					0x0046
> +
> +#define VL_REG_SOFT_RESET_GO2_SOFT_RESET_N				0x00bf
> +#define VL_REG_IDENTIFICATION_MODEL_ID					0x00c0
> +#define VL_REG_IDENTIFICATION_REVISION_ID				0x00c2
> +
> +#define VL_REG_OSC_CALIBRATE_VAL					0x00f8
> +
> +#define VL_SIGMA_ESTIMATE_MAX_VALUE					65535
> +/* equivalent to a range sigma of 655.35mm */
> +
> +#define VL_REG_GLOBAL_CFG_VCSEL_WIDTH					0x032
> +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_0			0x0B0
> +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_1			0x0B1
> +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_2			0x0B2
> +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_3			0x0B3
> +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_4			0x0B4
> +#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_5			0x0B5
> +#define VL_REG_GLOBAL_CFG_REF_EN_START_SELECT			0xB6

0x00 or 0x for prefix?

> +#define VL_REG_DYNAMIC_SPAD_NUM_REQUESTED_REF_SPAD		0x4E /* 0x14E */
> +#define VL_REG_DYNAMIC_SPAD_REF_EN_START_OFFSET			0x4F /* 0x14F */
> +#define VL_REG_POWER_MANAGEMENT_GO1_POWER_FORCE			0x80
> +
> +/*
> + * Speed of light in um per 1E-10 Seconds
> + */
> +#define VL_SPEED_OF_LIGHT_IN_AIR					2997
> +#define VL_REG_VHV_CFG_PAD_SCL_SDA__EXTSUP_HV			0x0089
> +#define VL_REG_ALGO_PHASECAL_LIM			0x0030 /* 0x130 */
> +#define VL_REG_ALGO_PHASECAL_CFG_TIMEOUT				0x0030
> +
> +struct vl53l0x_data {
> +	struct i2c_client *client;
> +	struct mutex lock;
> +	int	useLongRange;

noCamelCasePlease.

> +};
> +
> +static int vl53l0x_read_proximity(struct vl53l0x_data *data,
> +				const struct iio_chan_spec *chan,
> +				int *val)
> +{
> +	int ret;
> +	struct i2c_client *client = data->client;
> +	int tries = 20;

unsigned int

> +	u8 buffer[12];
> +	struct i2c_msg msg[2];
> +	u8 write_command = VL_REG_RESULT_RANGE_STATUS;

Reversed xmas tree order, please

> +
> +	ret = i2c_smbus_write_byte_data(data->client,
> +					VL_REG_SYSRANGE_START, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	while (tries-- > 0) {


Better form is

do {
	...
} while (--tries);
if (!tries)
	return -ETIMEDOUT;

> +		ret = i2c_smbus_read_byte_data(data->client,
> +						VL_REG_RESULT_RANGE_STATUS);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (ret & 0x01)
> +			break;
> +		usleep_range(1000, 5000);
> +	}
> +
> +	if (tries < 0)
> +		return -ETIMEDOUT;
> +
> +	msg[0].addr = client->addr;
> +	msg[0].buf = &write_command;
> +	msg[0].len = 1;
> +	msg[0].flags = client->flags | I2C_M_STOP;
> +
> +	msg[1].addr = client->addr;
> +	msg[1].buf = buffer;
> +	msg[1].len = 12;
> +	msg[1].flags = client->flags | I2C_M_RD;
> +
> +	ret = i2c_transfer(client->adapter, msg, 2);
> +

> +	if (ret != 2) {

Shouldn't be simple if (ret < 0)

> +		pr_err("vl53l0x: consecutively read error. ");

dev_err(...);
Also check other prints.

> +		return ret;
> +	}
> +
> +	*val = __le16_to_cpu((buffer[10] << 8) + buffer[11]);
> +
> +	return 0;
> +}

> +static int vl53l0x_probe(struct i2c_client *client,
> +				   const struct i2c_device_id *id)

(see below about ->probe_new() hook)

> +{
> +	int ret;
> +	struct vl53l0x_data *data;
> +	struct iio_dev *indio_dev;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	data->client = client;
> +	i2c_set_clientdata(client, indio_dev);
> +	mutex_init(&data->lock);
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE))
> +		return -EOPNOTSUPP;
> +
> +	indio_dev->dev.parent = &client->dev;
> +	indio_dev->name = VL53L0X_DRV_NAME;
> +	indio_dev->info = &vl53l0x_info;
> +	indio_dev->channels = vl53l0x_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(vl53l0x_channels);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +

> +	//probe 0xc0 if the value is 0xEE.

Useless commwnt  for now

> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	dev_set_drvdata(&client->dev, data);
> +
> +	return 0;
> +}
> +
> +static int vl53l0x_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +	struct vl53l0x_data *data = iio_priv(indio_dev);
> +
> +	iio_device_unregister(indio_dev);

> +	kfree(data);

how did you test it?

> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id vl53l0x_id[] = {
> +	{ VL53L0X_DRV_NAME, 0 },

> +	{ },
Terminator better w/o comma

> +};
> +MODULE_DEVICE_TABLE(i2c, vl53l0x_id);

Do you expect this to be used at all?

> +
> +static const struct of_device_id st_vl53l0x_dt_match[] = {
> +	{ .compatible = "st,vl53l0x-i2c", },

> +	{ },

Terminator better w/o comma

> +};
> +
> +static struct i2c_driver vl53l0x_driver = {
> +	.driver = {
> +		.name	= VL53L0X_DRV_NAME,

> +		.owner	= THIS_MODULE,

This done by macro.

> +		.of_match_table = st_vl53l0x_dt_match,
> +	},
> +	.probe	= vl53l0x_probe,

->probe_new(), please.

> +	.remove	= vl53l0x_remove,
> +	.id_table = vl53l0x_id,
> +};
> +module_i2c_driver(vl53l0x_driver);
> +
> +MODULE_AUTHOR("Song Qiang <songqiang.1304521@gmail.com>");
> +MODULE_DESCRIPTION("ST vl53l0x ToF ranging sensor");
> +MODULE_LICENSE("GPL");
> -- 
> 2.17.1
> 

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH] iio: proximity: Add driver support for ST's VL53L0X ToF ranging sensor.
@ 2018-09-10 14:42 Song Qiang
  2018-09-10 15:12 ` Andy Shevchenko
  2018-09-10 17:57 ` Himanshu Jha
  0 siblings, 2 replies; 12+ messages in thread
From: Song Qiang @ 2018-09-10 14:42 UTC (permalink / raw)
  To: jic23
  Cc: knaack.h, lars, pmeerw, andriy.shevchenko, matt.ranostay, robh,
	kstewart, gregkh, linux-iio, linux-kernel, Song Qiang

This driver was originally written by ST in 2016 as a misc input device,
and hasn't been maintained for a long time. I grabbed some code from
it's API and reformed it to a iio proximity device driver.
This version of driver uses i2c bus to talk to the sensor and
polling for measuring completes, so no irq line is needed.
This version of driver supports only one-shot mode, and it can be
tested with reading from
/sys/bus/iio/devices/iio:deviceX/in_distance_raw

Signed-off-by: Song Qiang <songqiang.1304521@gmail.com>
---
 .../bindings/iio/proximity/vl53l0x.txt        |  12 +
 drivers/iio/proximity/Kconfig                 |  13 +
 drivers/iio/proximity/Makefile                |   2 +
 drivers/iio/proximity/vl53l0x-i2c.c           | 295 ++++++++++++++++++
 4 files changed, 322 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
 create mode 100644 drivers/iio/proximity/vl53l0x-i2c.c

diff --git a/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
new file mode 100644
index 000000000000..64b69442f08e
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
@@ -0,0 +1,12 @@
+ST's VL53L0X ToF ranging sensor
+
+Required properties:
+	- compatible: must be "st,vl53l0x-i2c"
+	- reg: i2c address where to find the device
+
+Example:
+
+vl53l0x@29 {
+	compatible = "st,vl53l0x-i2c";
+	reg = <0x29>;
+};
diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
index f726f9427602..1563a5f9144d 100644
--- a/drivers/iio/proximity/Kconfig
+++ b/drivers/iio/proximity/Kconfig
@@ -79,4 +79,17 @@ config SRF08
 	  To compile this driver as a module, choose M here: the
 	  module will be called srf08.
 
+config VL53L0X_I2C
+	tristate "STMicroelectronics VL53L0X ToF ranger sensor (I2C)"
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	depends on I2C
+	help
+	  Say Y here to build a driver for STMicroelectronics VL53L0X
+	  ToF ranger sensors with i2c interface.
+	  This driver can be used to measure the distance of objects.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called vl53l0x-i2c.
+
 endmenu
diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
index 4f4ed45e87ef..7cb771665c8b 100644
--- a/drivers/iio/proximity/Makefile
+++ b/drivers/iio/proximity/Makefile
@@ -10,3 +10,5 @@ obj-$(CONFIG_RFD77402)		+= rfd77402.o
 obj-$(CONFIG_SRF04)		+= srf04.o
 obj-$(CONFIG_SRF08)		+= srf08.o
 obj-$(CONFIG_SX9500)		+= sx9500.o
+obj-$(CONFIG_VL53L0X_I2C)		+= vl53l0x-i2c.o
+
diff --git a/drivers/iio/proximity/vl53l0x-i2c.c b/drivers/iio/proximity/vl53l0x-i2c.c
new file mode 100644
index 000000000000..c00713041d30
--- /dev/null
+++ b/drivers/iio/proximity/vl53l0x-i2c.c
@@ -0,0 +1,295 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ *  vl53l0x-i2c.c - Support for STM VL53L0X FlightSense TOF
+ *					Ranger Sensor on a i2c bus.
+ *
+ *  Copyright (C) 2016 STMicroelectronics Imaging Division.
+ *  Copyright (C) 2018 Song Qiang <songqiang.1304521@gmail.com>
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/delay.h>
+#include <linux/kernel.h>
+#include <linux/iio/iio.h>
+
+#define VL53L0X_DRV_NAME			"vl53l0x"
+
+/* Device register map */
+#define VL_REG_SYSRANGE_START					0x000
+#define VL_REG_SYSRANGE_MODE_MASK				0x0F
+#define VL_REG_SYSRANGE_MODE_START_STOP			0x01
+#define VL_REG_SYSRANGE_MODE_SINGLESHOT			0x00
+#define VL_REG_SYSRANGE_MODE_BACKTOBACK			0x02
+#define VL_REG_SYSRANGE_MODE_TIMED				0x04
+#define VL_REG_SYSRANGE_MODE_HISTOGRAM			0x08
+
+#define VL_REG_SYS_THRESH_HIGH					0x000C
+#define VL_REG_SYS_THRESH_LOW					0x000E
+
+#define VL_REG_SYS_SEQUENCE_CFG					0x0001
+#define VL_REG_SYS_RANGE_CFG					0x0009
+#define VL_REG_SYS_INTERMEASUREMENT_PERIOD		0x0004
+
+#define VL_REG_SYS_INT_CFG_GPIO					0x000A
+#define VL_REG_SYS_INT_GPIO_DISABLED			0x00
+#define VL_REG_SYS_INT_GPIO_LEVEL_LOW			0x01
+#define VL_REG_SYS_INT_GPIO_LEVEL_HIGH			0x02
+#define VL_REG_SYS_INT_GPIO_OUT_OF_WINDOW		0x03
+#define VL_REG_SYS_INT_GPIO_NEW_SAMPLE_READY	0x04
+#define VL_REG_GPIO_HV_MUX_ACTIVE_HIGH			0x0084
+#define VL_REG_SYS_INT_CLEAR					0x000B
+
+/* Result registers */
+#define VL_REG_RESULT_INT_STATUS				0x0013
+#define VL_REG_RESULT_RANGE_STATUS				0x0014
+
+#define VL_REG_RESULT_CORE_PAGE  1
+#define VL_REG_RESULT_CORE_AMBIENT_WINDOW_EVENTS_RTN	0x00BC
+#define VL_REG_RESULT_CORE_RANGING_TOTAL_EVENTS_RTN		0x00C0
+#define VL_REG_RESULT_CORE_AMBIENT_WINDOW_EVENTS_REF	0x00D0
+#define VL_REG_RESULT_CORE_RANGING_TOTAL_EVENTS_REF		0x00D4
+#define VL_REG_RESULT_PEAK_SIGNAL_RATE_REF				0x00B6
+
+/* Algo register */
+#define VL_REG_ALGO_PART_TO_PART_RANGE_OFFSET_MM		0x0028
+
+#define VL_REG_I2C_SLAVE_DEVICE_ADDRESS					0x008a
+
+/* Check Limit registers */
+#define VL_REG_MSRC_CFG_CONTROL						0x0060
+
+#define VL_REG_PRE_RANGE_CFG_MIN_SNR					0X0027
+#define VL_REG_PRE_RANGE_CFG_VALID_PHASE_LOW			0x0056
+#define VL_REG_PRE_RANGE_CFG_VALID_PHASE_HIGH			0x0057
+#define VL_REG_PRE_RANGE_MIN_COUNT_RATE_RTN_LIMIT		0x0064
+
+#define VL_REG_FINAL_RANGE_CFG_MIN_SNR					0X0067
+#define VL_REG_FINAL_RANGE_CFG_VALID_PHASE_LOW			0x0047
+#define VL_REG_FINAL_RANGE_CFG_VALID_PHASE_HIGH			0x0048
+#define VL_REG_FINAL_RANGE_CFG_MIN_COUNT_RATE_RTN_LIMIT	0x0044
+
+#define VL_REG_PRE_RANGE_CFG_SIGMA_THRESH_HI			0X0061
+#define VL_REG_PRE_RANGE_CFG_SIGMA_THRESH_LO			0X0062
+
+/* PRE RANGE registers */
+#define VL_REG_PRE_RANGE_CFG_VCSEL_PERIOD				0x0050
+#define VL_REG_PRE_RANGE_CFG_TIMEOUT_MACROP_HI			0x0051
+#define VL_REG_PRE_RANGE_CFG_TIMEOUT_MACROP_LO			0x0052
+
+#define VL_REG_SYS_HISTOGRAM_BIN					0x0081
+#define VL_REG_HISTOGRAM_CFG_INITIAL_PHASE_SELECT		0x0033
+#define VL_REG_HISTOGRAM_CFG_READOUT_CTRL				0x0055
+
+#define VL_REG_FINAL_RANGE_CFG_VCSEL_PERIOD				0x0070
+#define VL_REG_FINAL_RANGE_CFG_TIMEOUT_MACROP_HI		0x0071
+#define VL_REG_FINAL_RANGE_CFG_TIMEOUT_MACROP_LO		0x0072
+#define VL_REG_CROSSTALK_COMPENSATION_PEAK_RATE_MCPS	0x0020
+
+#define VL_REG_MSRC_CFG_TIMEOUT_MACROP					0x0046
+
+#define VL_REG_SOFT_RESET_GO2_SOFT_RESET_N				0x00bf
+#define VL_REG_IDENTIFICATION_MODEL_ID					0x00c0
+#define VL_REG_IDENTIFICATION_REVISION_ID				0x00c2
+
+#define VL_REG_OSC_CALIBRATE_VAL					0x00f8
+
+#define VL_SIGMA_ESTIMATE_MAX_VALUE					65535
+/* equivalent to a range sigma of 655.35mm */
+
+#define VL_REG_GLOBAL_CFG_VCSEL_WIDTH					0x032
+#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_0			0x0B0
+#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_1			0x0B1
+#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_2			0x0B2
+#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_3			0x0B3
+#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_4			0x0B4
+#define VL_REG_GLOBAL_CFG_SPAD_ENABLES_REF_5			0x0B5
+#define VL_REG_GLOBAL_CFG_REF_EN_START_SELECT			0xB6
+#define VL_REG_DYNAMIC_SPAD_NUM_REQUESTED_REF_SPAD		0x4E /* 0x14E */
+#define VL_REG_DYNAMIC_SPAD_REF_EN_START_OFFSET			0x4F /* 0x14F */
+#define VL_REG_POWER_MANAGEMENT_GO1_POWER_FORCE			0x80
+
+/*
+ * Speed of light in um per 1E-10 Seconds
+ */
+#define VL_SPEED_OF_LIGHT_IN_AIR					2997
+#define VL_REG_VHV_CFG_PAD_SCL_SDA__EXTSUP_HV			0x0089
+#define VL_REG_ALGO_PHASECAL_LIM			0x0030 /* 0x130 */
+#define VL_REG_ALGO_PHASECAL_CFG_TIMEOUT				0x0030
+
+struct vl53l0x_data {
+	struct i2c_client *client;
+	struct mutex lock;
+	int	useLongRange;
+};
+
+static int vl53l0x_read_proximity(struct vl53l0x_data *data,
+				const struct iio_chan_spec *chan,
+				int *val)
+{
+	int ret;
+	struct i2c_client *client = data->client;
+	int tries = 20;
+	u8 buffer[12];
+	struct i2c_msg msg[2];
+	u8 write_command = VL_REG_RESULT_RANGE_STATUS;
+
+	ret = i2c_smbus_write_byte_data(data->client,
+					VL_REG_SYSRANGE_START, 1);
+	if (ret < 0)
+		return ret;
+
+	while (tries-- > 0) {
+		ret = i2c_smbus_read_byte_data(data->client,
+						VL_REG_RESULT_RANGE_STATUS);
+		if (ret < 0)
+			return ret;
+
+		if (ret & 0x01)
+			break;
+		usleep_range(1000, 5000);
+	}
+
+	if (tries < 0)
+		return -ETIMEDOUT;
+
+	msg[0].addr = client->addr;
+	msg[0].buf = &write_command;
+	msg[0].len = 1;
+	msg[0].flags = client->flags | I2C_M_STOP;
+
+	msg[1].addr = client->addr;
+	msg[1].buf = buffer;
+	msg[1].len = 12;
+	msg[1].flags = client->flags | I2C_M_RD;
+
+	ret = i2c_transfer(client->adapter, msg, 2);
+
+	if (ret != 2) {
+		pr_err("vl53l0x: consecutively read error. ");
+		return ret;
+	}
+
+	*val = __le16_to_cpu((buffer[10] << 8) + buffer[11]);
+
+	return 0;
+}
+
+static const struct iio_chan_spec vl53l0x_channels[] = {
+	{
+		.type = IIO_DISTANCE,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW)
+	},
+	IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static int vl53l0x_read_raw(struct iio_dev *indio_dev,
+				const struct iio_chan_spec *chan,
+				int *val, int *val2, long mask)
+{
+	struct vl53l0x_data *data = iio_priv(indio_dev);
+	int ret;
+
+	if (chan->type != IIO_DISTANCE) {
+		pr_err("vl53l0x: iio type error");
+		return -EINVAL;
+	}
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+		ret = vl53l0x_read_proximity(data, chan, val);
+		if (ret < 0)
+			pr_err("vl53l0x: raw value read error with %d", ret);
+
+		ret = IIO_VAL_INT;
+		iio_device_release_direct_mode(indio_dev);
+		return ret;
+	default:
+		pr_err("vl53l0x: IIO_CHAN_* not recognzed.");
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info vl53l0x_info = {
+	.read_raw = vl53l0x_read_raw,
+};
+
+static int vl53l0x_probe(struct i2c_client *client,
+				   const struct i2c_device_id *id)
+{
+	int ret;
+	struct vl53l0x_data *data;
+	struct iio_dev *indio_dev;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(indio_dev);
+	data->client = client;
+	i2c_set_clientdata(client, indio_dev);
+	mutex_init(&data->lock);
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE))
+		return -EOPNOTSUPP;
+
+	indio_dev->dev.parent = &client->dev;
+	indio_dev->name = VL53L0X_DRV_NAME;
+	indio_dev->info = &vl53l0x_info;
+	indio_dev->channels = vl53l0x_channels;
+	indio_dev->num_channels = ARRAY_SIZE(vl53l0x_channels);
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	//probe 0xc0 if the value is 0xEE.
+
+	ret = iio_device_register(indio_dev);
+	if (ret)
+		return ret;
+
+	dev_set_drvdata(&client->dev, data);
+
+	return 0;
+}
+
+static int vl53l0x_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+	struct vl53l0x_data *data = iio_priv(indio_dev);
+
+	iio_device_unregister(indio_dev);
+	kfree(data);
+
+	return 0;
+}
+
+static const struct i2c_device_id vl53l0x_id[] = {
+	{ VL53L0X_DRV_NAME, 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(i2c, vl53l0x_id);
+
+static const struct of_device_id st_vl53l0x_dt_match[] = {
+	{ .compatible = "st,vl53l0x-i2c", },
+	{ },
+};
+
+static struct i2c_driver vl53l0x_driver = {
+	.driver = {
+		.name	= VL53L0X_DRV_NAME,
+		.owner	= THIS_MODULE,
+		.of_match_table = st_vl53l0x_dt_match,
+	},
+	.probe	= vl53l0x_probe,
+	.remove	= vl53l0x_remove,
+	.id_table = vl53l0x_id,
+};
+module_i2c_driver(vl53l0x_driver);
+
+MODULE_AUTHOR("Song Qiang <songqiang.1304521@gmail.com>");
+MODULE_DESCRIPTION("ST vl53l0x ToF ranging sensor");
+MODULE_LICENSE("GPL");
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2018-09-15  2:26 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-13  2:22 [PATCH] iio: proximity: Add driver support for ST's VL53L0X ToF ranging sensor Song Qiang
2018-09-14 18:43 ` Himanshu Jha
2018-09-15  2:25   ` Song Qiang
  -- strict thread matches above, loose matches on Subject: below --
2018-09-10 14:42 Song Qiang
2018-09-10 15:12 ` Andy Shevchenko
2018-09-11  2:19   ` Song Qiang
2018-09-10 17:57 ` Himanshu Jha
2018-09-11  2:47   ` Song Qiang
2018-09-11  6:46   ` Song Qiang
2018-09-11  7:28     ` Himanshu Jha
2018-09-11  7:43       ` Song Qiang
2018-09-11  7:09   ` Song Qiang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.