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 an 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.
It can be tested with reading from
/sys/bus/iio/devices/iio:deviceX/in_distance_input
Signed-off-by: Song Qiang <[email protected]>
---
Changes in v6:
- Remove '.' in mail address. Google doesn't care, these two
email address is the same when I log in.
- Clean register table.
- Put some seperated statements into one line.
- Change channel type to *_PROCESSED.
- Remove some unneccessary dev_err.
- changes iio device name to vl53l0x.
Changes in v5:
- Correct some spell problems.
- Change tries-- to --tries to fix the count error.
- Add MODULE_DEVICE_TABLE().
- Add some comments.
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.
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 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.
.../bindings/iio/proximity/vl53l0x.txt | 12 ++
MAINTAINERS | 7 +
drivers/iio/proximity/Kconfig | 11 ++
drivers/iio/proximity/Makefile | 2 +
drivers/iio/proximity/vl53l0x-i2c.c | 157 ++++++++++++++++++
5 files changed, 189 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..ab9a9539fec4
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
@@ -0,0 +1,12 @@
+ST 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..349e2bc97cec 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13510,6 +13510,13 @@ L: [email protected]
S: Maintained
F: drivers/i2c/busses/i2c-stm32*
+ST VL53L0X ToF RANGER(I2C) IIO DRIVER
+M: Song Qiang <[email protected]>
+L: [email protected]
+S: Maintained
+F: drivers/iio/proximity/vl53l0x-i2c.c
+F: Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
+
STABLE BRANCH
M: Greg Kroah-Hartman <[email protected]>
L: [email protected]
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..1aad45df8d95
--- /dev/null
+++ b/drivers/iio/proximity/vl53l0x-i2c.c
@@ -0,0 +1,157 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Support for ST VL53L0X FlightSense ToF Ranging Sensor on a i2c bus.
+ *
+ * Copyright (C) 2016 STMicroelectronics Imaging Division.
+ * Copyright (C) 2018 Song Qiang <[email protected]>
+ *
+ * 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,
+ * sensor ID check.
+ */
+
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/delay.h>
+
+#include <linux/iio/iio.h>
+
+#define VL_REG_SYSRANGE_START 0x00
+
+#define VL_REG_SYSRANGE_MODE_MASK GENMASK(3, 0)
+#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_RESULT_INT_STATUS 0x13
+#define VL_REG_RESULT_RANGE_STATUS 0x14
+#define VL_REG_RESULT_RANGE_STATUS_COMPLETE BIT(0)
+
+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;
+ u16 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_STATUS_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;
+
+ /* Values should be between 30~1200 in millimeters. */
+ *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_PROCESSED),
+ },
+};
+
+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)
+ return -EINVAL;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_PROCESSED:
+ ret = vl53l0x_read_proximity(data, chan, val);
+ if (ret < 0)
+ return ret;
+
+ return IIO_VAL_INT;
+ default:
+ 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_READ_I2C_BLOCK | I2C_FUNC_SMBUS_BYTE_DATA))
+ return -EOPNOTSUPP;
+
+ indio_dev->dev.parent = &client->dev;
+ indio_dev->name = "vl53l0x";
+ 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", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, st_vl53l0x_dt_match);
+
+static struct i2c_driver vl53l0x_driver = {
+ .driver = {
+ .name = "vl53l0x-i2c",
+ .of_match_table = st_vl53l0x_dt_match,
+ },
+ .probe_new = vl53l0x_probe,
+};
+module_i2c_driver(vl53l0x_driver);
+
+MODULE_AUTHOR("Song Qiang <[email protected]>");
+MODULE_DESCRIPTION("ST vl53l0x ToF ranging sensor driver");
+MODULE_LICENSE("GPL v2");
--
2.17.1
The first version of this driver issues a measuring request and polling
for a status register in the device for measuring completes.
vl53l0x support configuring GPIO1 on it to generate interrupt to
indicate that new measurement is ready. This patch adds support for
using this mechanisim to reduce cpu cost.
Signed-off-by: Song Qiang <[email protected]>
---
.../bindings/iio/proximity/vl53l0x.txt | 14 +-
drivers/iio/proximity/vl53l0x-i2c.c | 135 +++++++++++++++---
2 files changed, 129 insertions(+), 20 deletions(-)
diff --git a/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
index ab9a9539fec4..40290f8dd70f 100644
--- a/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
+++ b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
@@ -4,9 +4,21 @@ Required properties:
- compatible: must be "st,vl53l0x-i2c"
- reg: i2c address where to find the device
+Optional properties:
+ - interrupts : Interrupt line receiving GPIO1's measuring complete
+ output, supports IRQ_TYPE_EDGE_FALLING only.
+
+ Refer to interrupt-controller/interrupts.txt for generic
+ interrupt client node bindings.
+
Example:
vl53l0x@29 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&vl53l0x_pins>;
+
compatible = "st,vl53l0x-i2c";
reg = <0x29>;
-};
+ interrupt-parent = <&gpio3>;
+ interrupts = <17 IRQ_TYPE_EDGE_FALLING>;
+}
diff --git a/drivers/iio/proximity/vl53l0x-i2c.c b/drivers/iio/proximity/vl53l0x-i2c.c
index 1aad45df8d95..a5cff11f41de 100644
--- a/drivers/iio/proximity/vl53l0x-i2c.c
+++ b/drivers/iio/proximity/vl53l0x-i2c.c
@@ -10,18 +10,21 @@
*
* Default 7-bit i2c slave address 0x29.
*
- * TODO: FIFO buffer, continuous mode, interrupts, range selection,
- * sensor ID check.
+ * TODO: FIFO buffer, continuous mode, range selection.
*/
#include <linux/module.h>
#include <linux/i2c.h>
#include <linux/delay.h>
+#include <linux/interrupt.h>
#include <linux/iio/iio.h>
+#include <stdbool.h>
+
#define VL_REG_SYSRANGE_START 0x00
+/* Mode configuration registers. */
#define VL_REG_SYSRANGE_MODE_MASK GENMASK(3, 0)
#define VL_REG_SYSRANGE_MODE_SINGLESHOT 0x00
#define VL_REG_SYSRANGE_MODE_START_STOP BIT(0)
@@ -29,14 +32,61 @@
#define VL_REG_SYSRANGE_MODE_TIMED BIT(2)
#define VL_REG_SYSRANGE_MODE_HISTOGRAM BIT(3)
+/* Result registers. */
#define VL_REG_RESULT_INT_STATUS 0x13
#define VL_REG_RESULT_RANGE_STATUS 0x14
#define VL_REG_RESULT_RANGE_STATUS_COMPLETE BIT(0)
+/* GPIO function configuration registers. */
+#define VL_REG_GPIO_HV_MUX_ACTIVE_GIGH 0x84
+#define VL_REG_SYS_INT_CFG_GPIO 0x0A
+#define VL_GPIOFUNC_NEW_MEASURE_RDY BIT(2)
+
+/* Interrupt configuration registers. */
+#define VL_REG_SYS_INT_CLEAR 0x0B
+#define VL_REG_RESULT_INT_STATUS 0x13
+#define VL_INT_POLARITY_LOW 0x00
+#define VL_INT_POLARITY_HIGH BIT(0)
+
+/* Should be 0xEE if connection is fine. */
+#define VL_REG_MODEL_ID 0xC0
+
struct vl53l0x_data {
struct i2c_client *client;
+ struct completion measuring_done;
+ bool use_interrupt;
};
+static int vl53l0x_clear_interrupt(struct vl53l0x_data *data)
+{
+ int ret;
+ u8 cnt = 0;
+
+ do {
+ /* bit 0 for measuring interrupt, bit 1 for error interrupt. */
+ i2c_smbus_write_byte_data(data->client,
+ VL_REG_SYS_INT_CLEAR, 1);
+ i2c_smbus_write_byte_data(data->client,
+ VL_REG_SYS_INT_CLEAR, 0);
+ ret = i2c_smbus_read_byte_data(data->client,
+ VL_REG_RESULT_INT_STATUS);
+ cnt++;
+ } while ((ret & 0x07) && (cnt < 3));
+ if (cnt > 2)
+ return -ETIMEDOUT;
+ else
+ return 0;
+}
+
+static irqreturn_t vl53l0x_irq_handler(int irq, void *d)
+{
+ struct vl53l0x_data *data = d;
+
+ complete(&data->measuring_done);
+
+ return IRQ_HANDLED;
+}
+
static int vl53l0x_read_proximity(struct vl53l0x_data *data,
const struct iio_chan_spec *chan,
int *val)
@@ -46,23 +96,31 @@ static int vl53l0x_read_proximity(struct vl53l0x_data *data,
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_STATUS_COMPLETE)
- break;
-
- usleep_range(1000, 5000);
- } while (--tries);
- if (!tries)
- return -ETIMEDOUT;
+ if (data->use_interrupt)
+ reinit_completion(&data->measuring_done);
+
+ i2c_smbus_write_byte_data(client, VL_REG_SYSRANGE_START, 1);
+
+ /* In usual case the longest valid conversion time is less than 70ms. */
+ if (data->use_interrupt) {
+ ret = wait_for_completion_timeout(&data->measuring_done,
+ msecs_to_jiffies(100));
+ if (!ret)
+ return -ETIMEDOUT;
+ vl53l0x_clear_interrupt(data);
+ } else {
+ do {
+ ret = i2c_smbus_read_byte_data(client,
+ VL_REG_RESULT_RANGE_STATUS);
+
+ if (ret & VL_REG_RESULT_RANGE_STATUS_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);
@@ -110,10 +168,21 @@ static const struct iio_info vl53l0x_info = {
.read_raw = vl53l0x_read_raw,
};
+/* Congigure the GPIO1 pin to generate interrupt for measurement ready,
+ * default polarity is level low.
+ */
+static int vl53l0x_config_irq(struct vl53l0x_data *data)
+{
+ i2c_smbus_write_byte_data(data->client, VL_REG_SYS_INT_CFG_GPIO,
+ VL_GPIOFUNC_NEW_MEASURE_RDY);
+ return vl53l0x_clear_interrupt(data);
+}
+
static int vl53l0x_probe(struct i2c_client *client)
{
struct vl53l0x_data *data;
struct iio_dev *indio_dev;
+ int ret;
indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
if (!indio_dev)
@@ -134,6 +203,34 @@ static int vl53l0x_probe(struct i2c_client *client)
indio_dev->num_channels = ARRAY_SIZE(vl53l0x_channels);
indio_dev->modes = INDIO_DIRECT_MODE;
+ if (!client->irq)
+ data->use_interrupt = false;
+ else {
+ data->use_interrupt = true;
+ ret = devm_request_irq(&client->dev,
+ client->irq,
+ vl53l0x_irq_handler,
+ IRQF_TRIGGER_FALLING,
+ indio_dev->name,
+ data);
+ if (ret < 0) {
+ dev_err(&client->dev,
+ "request irq line failed.");
+ return -EINVAL;
+ }
+ vl53l0x_config_irq(data);
+ init_completion(&data->measuring_done);
+ }
+
+ /* After checking this, assuming write and read byte operations should
+ * never fails.
+ */
+ ret = i2c_smbus_read_byte_data(client, VL_REG_MODEL_ID);
+ if (ret != 0xEE) {
+ dev_err(&client->dev, "device not found. ");
+ return -EREMOTEIO;
+ }
+
return devm_iio_device_register(&client->dev, indio_dev);
}
--
2.17.1
Hi,
[This is an automated email]
This commit has been processed because it contains a -stable tag.
The stable tag indicates that it's relevant for the following trees: all
The bot has tested the following trees: v4.18.8, v4.14.70, v4.9.127, v4.4.156, v3.18.122,
v4.18.8: Build OK!
v4.14.70: Failed to apply! Possible dependencies:
22aac3eb0c46 ("MAINTAINERS: add entry for STM32 I2C driver")
v4.9.127: Failed to apply! Possible dependencies:
22aac3eb0c46 ("MAINTAINERS: add entry for STM32 I2C driver")
78f839029e1d ("iio: distance: srf08: add IIO driver for us ranger")
v4.4.156: Failed to apply! Possible dependencies:
22aac3eb0c46 ("MAINTAINERS: add entry for STM32 I2C driver")
78f839029e1d ("iio: distance: srf08: add IIO driver for us ranger")
v3.18.122: Failed to apply! Possible dependencies:
22aac3eb0c46 ("MAINTAINERS: add entry for STM32 I2C driver")
4193c0f1d863 ("iio: driver for Semtech SX9500 proximity solution")
78f839029e1d ("iio: distance: srf08: add IIO driver for us ranger")
Please let us know how to resolve this.
--
Thanks,
Sasha
On Wed, 19 Sep 2018 18:58:37 +0000
Sasha Levin <[email protected]> wrote:
> Hi,
>
> [This is an automated email]
>
> This commit has been processed because it contains a -stable tag.
> The stable tag indicates that it's relevant for the following trees: all
>
> The bot has tested the following trees: v4.18.8, v4.14.70, v4.9.127, v4.4.156, v3.18.122,
>
> v4.18.8: Build OK!
> v4.14.70: Failed to apply! Possible dependencies:
> 22aac3eb0c46 ("MAINTAINERS: add entry for STM32 I2C driver")
>
> v4.9.127: Failed to apply! Possible dependencies:
> 22aac3eb0c46 ("MAINTAINERS: add entry for STM32 I2C driver")
> 78f839029e1d ("iio: distance: srf08: add IIO driver for us ranger")
>
> v4.4.156: Failed to apply! Possible dependencies:
> 22aac3eb0c46 ("MAINTAINERS: add entry for STM32 I2C driver")
> 78f839029e1d ("iio: distance: srf08: add IIO driver for us ranger")
>
> v3.18.122: Failed to apply! Possible dependencies:
> 22aac3eb0c46 ("MAINTAINERS: add entry for STM32 I2C driver")
> 4193c0f1d863 ("iio: driver for Semtech SX9500 proximity solution")
> 78f839029e1d ("iio: distance: srf08: add IIO driver for us ranger")
>
>
> Please let us know how to resolve this.
>
> --
> Thanks,
> Sasha
Hi Sasha,
This is a false detection. The stable tag is in the context of the
additions to MAINTAINERS. I guess that is a rare enough case it's not
worth fixing in your scripts, but thought I'd point it out incase you
want to do so!
Jonathan
On Thu, Sep 20, 2018 at 09:41:39AM +0100, Jonathan Cameron wrote:
>On Wed, 19 Sep 2018 18:58:37 +0000
>Sasha Levin <[email protected]> wrote:
>
>> Hi,
>>
>> [This is an automated email]
>>
>> This commit has been processed because it contains a -stable tag.
>> The stable tag indicates that it's relevant for the following trees: all
>>
>> The bot has tested the following trees: v4.18.8, v4.14.70, v4.9.127, v4.4.156, v3.18.122,
>>
>> v4.18.8: Build OK!
>> v4.14.70: Failed to apply! Possible dependencies:
>> 22aac3eb0c46 ("MAINTAINERS: add entry for STM32 I2C driver")
>>
>> v4.9.127: Failed to apply! Possible dependencies:
>> 22aac3eb0c46 ("MAINTAINERS: add entry for STM32 I2C driver")
>> 78f839029e1d ("iio: distance: srf08: add IIO driver for us ranger")
>>
>> v4.4.156: Failed to apply! Possible dependencies:
>> 22aac3eb0c46 ("MAINTAINERS: add entry for STM32 I2C driver")
>> 78f839029e1d ("iio: distance: srf08: add IIO driver for us ranger")
>>
>> v3.18.122: Failed to apply! Possible dependencies:
>> 22aac3eb0c46 ("MAINTAINERS: add entry for STM32 I2C driver")
>> 4193c0f1d863 ("iio: driver for Semtech SX9500 proximity solution")
>> 78f839029e1d ("iio: distance: srf08: add IIO driver for us ranger")
>>
>>
>> Please let us know how to resolve this.
>>
>> --
>> Thanks,
>> Sasha
>Hi Sasha,
>
>This is a false detection. The stable tag is in the context of the
>additions to MAINTAINERS. I guess that is a rare enough case it's not
>worth fixing in your scripts, but thought I'd point it out incase you
>want to do so!
Hah, interesting!
I'll get it fixed, thanks!
On Tue, 18 Sep 2018 16:24:21 +0800
Song Qiang <[email protected]> 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 an 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.
> It can be tested with reading from
> /sys/bus/iio/devices/iio:deviceX/in_distance_input
>
> Signed-off-by: Song Qiang <[email protected]>
There are a few bits and bobs in here, but as they are all minor and
one at least was me giving you wrong advise, I've fixed it up.
Please check I haven't made a mess of it!
Applied with changes to the togreg branch of iio.git and pushed out
as testing (where it should be visible now) for the autobuilders to
play with it.
Thanks,
Jonathan
> ---
> Changes in v6:
> - Remove '.' in mail address. Google doesn't care, these two
> email address is the same when I log in.
> - Clean register table.
> - Put some seperated statements into one line.
> - Change channel type to *_PROCESSED.
> - Remove some unneccessary dev_err.
> - changes iio device name to vl53l0x.
>
> Changes in v5:
> - Correct some spell problems.
> - Change tries-- to --tries to fix the count error.
> - Add MODULE_DEVICE_TABLE().
> - Add some comments.
>
> 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.
>
> 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 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.
>
> .../bindings/iio/proximity/vl53l0x.txt | 12 ++
> MAINTAINERS | 7 +
> drivers/iio/proximity/Kconfig | 11 ++
> drivers/iio/proximity/Makefile | 2 +
> drivers/iio/proximity/vl53l0x-i2c.c | 157 ++++++++++++++++++
> 5 files changed, 189 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..ab9a9539fec4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> @@ -0,0 +1,12 @@
> +ST 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..349e2bc97cec 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13510,6 +13510,13 @@ L: [email protected]
> S: Maintained
> F: drivers/i2c/busses/i2c-stm32*
>
> +ST VL53L0X ToF RANGER(I2C) IIO DRIVER
> +M: Song Qiang <[email protected]>
> +L: [email protected]
> +S: Maintained
> +F: drivers/iio/proximity/vl53l0x-i2c.c
> +F: Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> +
> STABLE BRANCH
> M: Greg Kroah-Hartman <[email protected]>
> L: [email protected]
> 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..1aad45df8d95
> --- /dev/null
> +++ b/drivers/iio/proximity/vl53l0x-i2c.c
> @@ -0,0 +1,157 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Support for ST VL53L0X FlightSense ToF Ranging Sensor on a i2c bus.
> + *
> + * Copyright (C) 2016 STMicroelectronics Imaging Division.
> + * Copyright (C) 2018 Song Qiang <[email protected]>
> + *
> + * 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,
> + * sensor ID check.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/delay.h>
> +
> +#include <linux/iio/iio.h>
> +
> +#define VL_REG_SYSRANGE_START 0x00
> +
> +#define VL_REG_SYSRANGE_MODE_MASK GENMASK(3, 0)
> +#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_RESULT_INT_STATUS 0x13
> +#define VL_REG_RESULT_RANGE_STATUS 0x14
> +#define VL_REG_RESULT_RANGE_STATUS_COMPLETE BIT(0)
> +
> +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;
> + u16 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_STATUS_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);
Please align with the opening brackets.
> + if (ret < 0)
> + return ret;
> + else if (ret != 12)
> + return -EREMOTEIO;
> +
> + /* Values should be between 30~1200 in millimeters. */
> + *val = le16_to_cpu((buffer[10] << 8) + buffer[11]);
This is wrong. You are effectively the ordering twice...
Ah. Sorry. I failed to check this last time. The units of
in_distance_processed are m, not mm. So this will need
to be _RAW and have a scale of 0.0001.
See Documentation/ABI/testing/sysfs-bus-iio
> +
> + return 0;
> +}
> +
> +static const struct iio_chan_spec vl53l0x_channels[] = {
> + {
> + .type = IIO_DISTANCE,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> + },
> +};
> +
> +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)
> + return -EINVAL;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_PROCESSED:
> + ret = vl53l0x_read_proximity(data, chan, val);
> + if (ret < 0)
> + return ret;
> +
> + return IIO_VAL_INT;
> + default:
> + 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_READ_I2C_BLOCK | I2C_FUNC_SMBUS_BYTE_DATA))
> + return -EOPNOTSUPP;
> +
> + indio_dev->dev.parent = &client->dev;
> + indio_dev->name = "vl53l0x";
> + 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", },
Ah, this suffers from the same thing that was picked up in a driver
you sent later in the week. No need to have -i2c in the compatible
as this is clear from the bus type.
(I might just fix this up).
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, st_vl53l0x_dt_match);
> +
> +static struct i2c_driver vl53l0x_driver = {
> + .driver = {
> + .name = "vl53l0x-i2c",
> + .of_match_table = st_vl53l0x_dt_match,
> + },
> + .probe_new = vl53l0x_probe,
> +};
> +module_i2c_driver(vl53l0x_driver);
> +
> +MODULE_AUTHOR("Song Qiang <[email protected]>");
> +MODULE_DESCRIPTION("ST vl53l0x ToF ranging sensor driver");
> +MODULE_LICENSE("GPL v2");
On Sat, Sep 22, 2018 at 03:46:58PM +0100, Jonathan Cameron wrote:
> On Tue, 18 Sep 2018 16:24:21 +0800
> Song Qiang <[email protected]> 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 an 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.
> > It can be tested with reading from
> > /sys/bus/iio/devices/iio:deviceX/in_distance_input
> >
> > Signed-off-by: Song Qiang <[email protected]>
> There are a few bits and bobs in here, but as they are all minor and
> one at least was me giving you wrong advise, I've fixed it up.
>
> Please check I haven't made a mess of it!
>
> Applied with changes to the togreg branch of iio.git and pushed out
> as testing (where it should be visible now) for the autobuilders to
> play with it.
The SPDX license identifier is inconsistent!
> +// SPDX-License-Identifier: GPL-2.0+
> +MODULE_LICENSE("GPL v2");
216 For 'GNU General Public License (GPL) version 2 only' use:
217 SPDX-License-Identifier: GPL-2.0
218 For 'GNU General Public License (GPL) version 2 or any later version' use:
219 SPDX-License-Identifier: GPL-2.0+
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
On Tue, 18 Sep 2018 16:24:22 +0800
Song Qiang <[email protected]> wrote:
> The first version of this driver issues a measuring request and polling
> for a status register in the device for measuring completes.
> vl53l0x support configuring GPIO1 on it to generate interrupt to
> indicate that new measurement is ready. This patch adds support for
> using this mechanisim to reduce cpu cost.
>
> Signed-off-by: Song Qiang <[email protected]>
Hi Song.
Looks correct in principal but a few things to tidy up before
this is ready to apply.
Also we have an unrelated change in here to check the devices ID.
That should be in it's own patch.
Thanks,
Jonathan
> ---
> .../bindings/iio/proximity/vl53l0x.txt | 14 +-
> drivers/iio/proximity/vl53l0x-i2c.c | 135 +++++++++++++++---
> 2 files changed, 129 insertions(+), 20 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> index ab9a9539fec4..40290f8dd70f 100644
> --- a/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> +++ b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> @@ -4,9 +4,21 @@ Required properties:
> - compatible: must be "st,vl53l0x-i2c"
> - reg: i2c address where to find the device
>
> +Optional properties:
> + - interrupts : Interrupt line receiving GPIO1's measuring complete
> + output, supports IRQ_TYPE_EDGE_FALLING only.
> +
> + Refer to interrupt-controller/interrupts.txt for generic
> + interrupt client node bindings.
> +
> Example:
>
> vl53l0x@29 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&vl53l0x_pins>;
> +
Please drop this from the example. This is board specific rather than
being generally required.
> compatible = "st,vl53l0x-i2c";
> reg = <0x29>;
> -};
> + interrupt-parent = <&gpio3>;
> + interrupts = <17 IRQ_TYPE_EDGE_FALLING>;
> +}
> diff --git a/drivers/iio/proximity/vl53l0x-i2c.c b/drivers/iio/proximity/vl53l0x-i2c.c
> index 1aad45df8d95..a5cff11f41de 100644
> --- a/drivers/iio/proximity/vl53l0x-i2c.c
> +++ b/drivers/iio/proximity/vl53l0x-i2c.c
> @@ -10,18 +10,21 @@
> *
> * Default 7-bit i2c slave address 0x29.
> *
> - * TODO: FIFO buffer, continuous mode, interrupts, range selection,
> - * sensor ID check.
> + * TODO: FIFO buffer, continuous mode, range selection.
> */
>
> #include <linux/module.h>
> #include <linux/i2c.h>
> #include <linux/delay.h>
> +#include <linux/interrupt.h>
>
> #include <linux/iio/iio.h>
>
> +#include <stdbool.h>
> +
> #define VL_REG_SYSRANGE_START 0x00
>
> +/* Mode configuration registers. */
> #define VL_REG_SYSRANGE_MODE_MASK GENMASK(3, 0)
> #define VL_REG_SYSRANGE_MODE_SINGLESHOT 0x00
> #define VL_REG_SYSRANGE_MODE_START_STOP BIT(0)
> @@ -29,14 +32,61 @@
> #define VL_REG_SYSRANGE_MODE_TIMED BIT(2)
> #define VL_REG_SYSRANGE_MODE_HISTOGRAM BIT(3)
>
> +/* Result registers. */
> #define VL_REG_RESULT_INT_STATUS 0x13
> #define VL_REG_RESULT_RANGE_STATUS 0x14
> #define VL_REG_RESULT_RANGE_STATUS_COMPLETE BIT(0)
>
> +/* GPIO function configuration registers. */
> +#define VL_REG_GPIO_HV_MUX_ACTIVE_GIGH 0x84
> +#define VL_REG_SYS_INT_CFG_GPIO 0x0A
> +#define VL_GPIOFUNC_NEW_MEASURE_RDY BIT(2)
> +
> +/* Interrupt configuration registers. */
> +#define VL_REG_SYS_INT_CLEAR 0x0B
> +#define VL_REG_RESULT_INT_STATUS 0x13
> +#define VL_INT_POLARITY_LOW 0x00
> +#define VL_INT_POLARITY_HIGH BIT(0)
> +
> +/* Should be 0xEE if connection is fine. */
> +#define VL_REG_MODEL_ID 0xC0
> +
> struct vl53l0x_data {
> struct i2c_client *client;
> + struct completion measuring_done;
> + bool use_interrupt;
> };
>
> +static int vl53l0x_clear_interrupt(struct vl53l0x_data *data)
> +{
> + int ret;
> + u8 cnt = 0;
> +
> + do {
> + /* bit 0 for measuring interrupt, bit 1 for error interrupt. */
> + i2c_smbus_write_byte_data(data->client,
> + VL_REG_SYS_INT_CLEAR, 1);
All these i2c comms need checking for errors. I2c buses aren't always the most
reliable things.
> + i2c_smbus_write_byte_data(data->client,
> + VL_REG_SYS_INT_CLEAR, 0);
> + ret = i2c_smbus_read_byte_data(data->client,
> + VL_REG_RESULT_INT_STATUS);
> + cnt++;
> + } while ((ret & 0x07) && (cnt < 3));
> + if (cnt > 2)
> + return -ETIMEDOUT;
> + else
> + return 0;
> +}
> +
> +static irqreturn_t vl53l0x_irq_handler(int irq, void *d)
> +{
> + struct vl53l0x_data *data = d;
> +
> + complete(&data->measuring_done);
> +
> + return IRQ_HANDLED;
> +}
> +
> static int vl53l0x_read_proximity(struct vl53l0x_data *data,
> const struct iio_chan_spec *chan,
> int *val)
> @@ -46,23 +96,31 @@ static int vl53l0x_read_proximity(struct vl53l0x_data *data,
> 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_STATUS_COMPLETE)
> - break;
> -
> - usleep_range(1000, 5000);
> - } while (--tries);
> - if (!tries)
> - return -ETIMEDOUT;
> + if (data->use_interrupt)
> + reinit_completion(&data->measuring_done);
This seems a little strange. Should be initialized already on first
pass for example. I don't suppose it matters, but feels a bit odd.
You may need to protect against multiple concurrent readers however...
> +
> + i2c_smbus_write_byte_data(client, VL_REG_SYSRANGE_START, 1);
> +
> + /* In usual case the longest valid conversion time is less than 70ms. */
> + if (data->use_interrupt) {
> + ret = wait_for_completion_timeout(&data->measuring_done,
> + msecs_to_jiffies(100));
> + if (!ret)
> + return -ETIMEDOUT;
> + vl53l0x_clear_interrupt(data);
> + } else {
> + do {
> + ret = i2c_smbus_read_byte_data(client,
> + VL_REG_RESULT_RANGE_STATUS);
> +
> + if (ret & VL_REG_RESULT_RANGE_STATUS_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);
> @@ -110,10 +168,21 @@ static const struct iio_info vl53l0x_info = {
> .read_raw = vl53l0x_read_raw,
> };
>
> +/* Congigure the GPIO1 pin to generate interrupt for measurement ready,
Comment syntax + spell check. Configure For a description like this
nicer to have it in kernel-doc style as well rather than a freeform comment.
> + * default polarity is level low.
> + */
> +static int vl53l0x_config_irq(struct vl53l0x_data *data)
> +{
> + i2c_smbus_write_byte_data(data->client, VL_REG_SYS_INT_CFG_GPIO,
> + VL_GPIOFUNC_NEW_MEASURE_RDY);
Error checking on that write_byte?
> + return vl53l0x_clear_interrupt(data);
> +}
> +
> static int vl53l0x_probe(struct i2c_client *client)
> {
> struct vl53l0x_data *data;
> struct iio_dev *indio_dev;
> + int ret;
>
> indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> if (!indio_dev)
> @@ -134,6 +203,34 @@ static int vl53l0x_probe(struct i2c_client *client)
> indio_dev->num_channels = ARRAY_SIZE(vl53l0x_channels);
> indio_dev->modes = INDIO_DIRECT_MODE;
>
> + if (!client->irq)
> + data->use_interrupt = false;
> + else {
> + data->use_interrupt = true;
> + ret = devm_request_irq(&client->dev,
> + client->irq,
> + vl53l0x_irq_handler,
> + IRQF_TRIGGER_FALLING,
> + indio_dev->name,
> + data);
> + if (ret < 0) {
> + dev_err(&client->dev,
Lots of odd alignment in here. Please tidy up.
> + "request irq line failed.");
> + return -EINVAL;
> + }
> + vl53l0x_config_irq(data);
> + init_completion(&data->measuring_done);
> + }
> +
> + /* After checking this, assuming write and read byte operations should
/*
* After..
*/
> + * never fails.
> + */
> + ret = i2c_smbus_read_byte_data(client, VL_REG_MODEL_ID);
> + if (ret != 0xEE) {
> + dev_err(&client->dev, "device not found. ");
No space after . and \n is normal for a dev_err message.
What does this have to do with interrupt support?
Please break it out as a separate patch.
> + return -EREMOTEIO;
> + }
> +
> return devm_iio_device_register(&client->dev, indio_dev);
> }
>
On Sat, 22 Sep 2018 20:26:58 +0530
Himanshu Jha <[email protected]> wrote:
> On Sat, Sep 22, 2018 at 03:46:58PM +0100, Jonathan Cameron wrote:
> > On Tue, 18 Sep 2018 16:24:21 +0800
> > Song Qiang <[email protected]> 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 an 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.
> > > It can be tested with reading from
> > > /sys/bus/iio/devices/iio:deviceX/in_distance_input
> > >
> > > Signed-off-by: Song Qiang <[email protected]>
> > There are a few bits and bobs in here, but as they are all minor and
> > one at least was me giving you wrong advise, I've fixed it up.
> >
> > Please check I haven't made a mess of it!
> >
> > Applied with changes to the togreg branch of iio.git and pushed out
> > as testing (where it should be visible now) for the autobuilders to
> > play with it.
>
> The SPDX license identifier is inconsistent!
>
> > +// SPDX-License-Identifier: GPL-2.0+
>
> > +MODULE_LICENSE("GPL v2");
>
> 216 For 'GNU General Public License (GPL) version 2 only' use:
> 217 SPDX-License-Identifier: GPL-2.0
> 218 For 'GNU General Public License (GPL) version 2 or any later version' use:
> 219 SPDX-License-Identifier: GPL-2.0+
>
>
Good spot.
I'll fix that up as well.
I'll assume the MODULE_LICENCE is right for now as it seems likely to have come
from the original ST code.
Jonathan
On Sat, Sep 22, 2018 at 03:46:58PM +0100, Jonathan Cameron wrote:
> On Tue, 18 Sep 2018 16:24:21 +0800
> Song Qiang <[email protected]> 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 an 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.
> > It can be tested with reading from
> > /sys/bus/iio/devices/iio:deviceX/in_distance_input
> >
> > Signed-off-by: Song Qiang <[email protected]>
> There are a few bits and bobs in here, but as they are all minor and
> one at least was me giving you wrong advise, I've fixed it up.
>
> Please check I haven't made a mess of it!
>
> Applied with changes to the togreg branch of iio.git and pushed out
> as testing (where it should be visible now) for the autobuilders to
> play with it.
>
> Thanks,
>
> Jonathan
> > ---
> > + indio_dev->name = "vl53l0x";
> > + 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", },
>
> Ah, this suffers from the same thing that was picked up in a driver
> you sent later in the week. No need to have -i2c in the compatible
> as this is clear from the bus type.
>
> (I might just fix this up).
>
Hi Jonathan,
Sorry for the newbie mistakes you and Himanshu pointed out, I'm just
writing them down on my little checklist!
I just checked the code, found that this compatible string was not
getting corrected in the DT binding doc file, sorry that you have
to help correct my mistakes, thanks a lot!
yours,
Song Qiang
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, st_vl53l0x_dt_match);
> > +
> > +static struct i2c_driver vl53l0x_driver = {
> > + .driver = {
> > + .name = "vl53l0x-i2c",
> > + .of_match_table = st_vl53l0x_dt_match,
> > + },
> > + .probe_new = vl53l0x_probe,
> > +};
> > +module_i2c_driver(vl53l0x_driver);
> > +
> > +MODULE_AUTHOR("Song Qiang <[email protected]>");
> > +MODULE_DESCRIPTION("ST vl53l0x ToF ranging sensor driver");
> > +MODULE_LICENSE("GPL v2");
>
On Sat, 22 Sep 2018 23:28:04 +0800
Song Qiang <[email protected]> wrote:
> On Sat, Sep 22, 2018 at 03:46:58PM +0100, Jonathan Cameron wrote:
> > On Tue, 18 Sep 2018 16:24:21 +0800
> > Song Qiang <[email protected]> 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 an 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.
> > > It can be tested with reading from
> > > /sys/bus/iio/devices/iio:deviceX/in_distance_input
> > >
> > > Signed-off-by: Song Qiang <[email protected]>
> > There are a few bits and bobs in here, but as they are all minor and
> > one at least was me giving you wrong advise, I've fixed it up.
> >
> > Please check I haven't made a mess of it!
> >
> > Applied with changes to the togreg branch of iio.git and pushed out
> > as testing (where it should be visible now) for the autobuilders to
> > play with it.
> >
> > Thanks,
> >
> > Jonathan
> > > ---
>
> > > + indio_dev->name = "vl53l0x";
> > > + 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", },
> >
> > Ah, this suffers from the same thing that was picked up in a driver
> > you sent later in the week. No need to have -i2c in the compatible
> > as this is clear from the bus type.
> >
> > (I might just fix this up).
> >
>
> Hi Jonathan,
>
> Sorry for the newbie mistakes you and Himanshu pointed out, I'm just
> writing them down on my little checklist!
> I just checked the code, found that this compatible string was not
> getting corrected in the DT binding doc file, sorry that you have
> to help correct my mistakes, thanks a lot!
Fixed up (and this was my mistake - we all make them - that's what review
is for!)
Thanks,
Jonathan
>
> yours,
> Song Qiang
>
> > > + { }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, st_vl53l0x_dt_match);
> > > +
> > > +static struct i2c_driver vl53l0x_driver = {
> > > + .driver = {
> > > + .name = "vl53l0x-i2c",
> > > + .of_match_table = st_vl53l0x_dt_match,
> > > + },
> > > + .probe_new = vl53l0x_probe,
> > > +};
> > > +module_i2c_driver(vl53l0x_driver);
> > > +
> > > +MODULE_AUTHOR("Song Qiang <[email protected]>");
> > > +MODULE_DESCRIPTION("ST vl53l0x ToF ranging sensor driver");
> > > +MODULE_LICENSE("GPL v2");
> >
On Sat, Sep 22, 2018 at 04:05:23PM +0100, Jonathan Cameron wrote:
> On Tue, 18 Sep 2018 16:24:22 +0800
> Song Qiang <[email protected]> wrote:
>
> > The first version of this driver issues a measuring request and polling
> > for a status register in the device for measuring completes.
> > vl53l0x support configuring GPIO1 on it to generate interrupt to
> > indicate that new measurement is ready. This patch adds support for
> > using this mechanisim to reduce cpu cost.
> >
> > Signed-off-by: Song Qiang <[email protected]>
> Hi Song.
>
> Looks correct in principal but a few things to tidy up before
> this is ready to apply.
>
> Also we have an unrelated change in here to check the devices ID.
> That should be in it's own patch.
>
> Thanks,
>
> Jonathan
> > ---
> > .../bindings/iio/proximity/vl53l0x.txt | 14 +-
This should have been split with the complete binding in one patch
rather than piecemeal driver feature by feature.
> > drivers/iio/proximity/vl53l0x-i2c.c | 135 +++++++++++++++---
> > 2 files changed, 129 insertions(+), 20 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > index ab9a9539fec4..40290f8dd70f 100644
> > --- a/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > +++ b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > @@ -4,9 +4,21 @@ Required properties:
> > - compatible: must be "st,vl53l0x-i2c"
Is there more than one interface on this device, such as SPI? If not,
then '-i2c' should be dropped.
> > - reg: i2c address where to find the device
> >
> > +Optional properties:
> > + - interrupts : Interrupt line receiving GPIO1's measuring complete
> > + output, supports IRQ_TYPE_EDGE_FALLING only.
> > +
> > + Refer to interrupt-controller/interrupts.txt for generic
> > + interrupt client node bindings.
> > +
> > Example:
> >
> > vl53l0x@29 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&vl53l0x_pins>;
> > +
> Please drop this from the example. This is board specific rather than
> being generally required.
>
> > compatible = "st,vl53l0x-i2c";
> > reg = <0x29>;
> > -};
> > + interrupt-parent = <&gpio3>;
> > + interrupts = <17 IRQ_TYPE_EDGE_FALLING>;
> > +}
On Wed, Sep 26, 2018 at 05:46:18PM -0500, Rob Herring wrote:
> On Sat, Sep 22, 2018 at 04:05:23PM +0100, Jonathan Cameron wrote:
> > On Tue, 18 Sep 2018 16:24:22 +0800
> > Song Qiang <[email protected]> wrote:
> >
> > > The first version of this driver issues a measuring request and polling
> > > for a status register in the device for measuring completes.
> > > vl53l0x support configuring GPIO1 on it to generate interrupt to
> > > indicate that new measurement is ready. This patch adds support for
> > > using this mechanisim to reduce cpu cost.
> > >
> > > Signed-off-by: Song Qiang <[email protected]>
> > Hi Song.
> >
> > Looks correct in principal but a few things to tidy up before
> > this is ready to apply.
> >
> > Also we have an unrelated change in here to check the devices ID.
> > That should be in it's own patch.
> >
> > Thanks,
> >
> > Jonathan
> > > ---
> > > .../bindings/iio/proximity/vl53l0x.txt | 14 +-
>
> This should have been split with the complete binding in one patch
> rather than piecemeal driver feature by feature.
>
Hi Rob,
A few days ago when I was submitting this driver, I didn't do it very
well, the function of this driver is limited. I added interrupt support
the next day after you acked my first patch. I thought it's not polite
to add something after someone acked that patch, so I send the interrupt
support as a second patch. The first patch is merged to togreg now, but
this doesn't. I don't know when can I add new functions to the code that
just merged to togreg branch, could you offer some suggestions?
> > > drivers/iio/proximity/vl53l0x-i2c.c | 135 +++++++++++++++---
> > > 2 files changed, 129 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > > index ab9a9539fec4..40290f8dd70f 100644
> > > --- a/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > > +++ b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > > @@ -4,9 +4,21 @@ Required properties:
> > > - compatible: must be "st,vl53l0x-i2c"
>
> Is there more than one interface on this device, such as SPI? If not,
> then '-i2c' should be dropped.
>
Yes, there is a CCI(Camera Control Interface) for communication.
> > > - reg: i2c address where to find the device
> > >
> > > +Optional properties:
> > > + - interrupts : Interrupt line receiving GPIO1's measuring complete
> > > + output, supports IRQ_TYPE_EDGE_FALLING only.
> > > +
> > > + Refer to interrupt-controller/interrupts.txt for generic
> > > + interrupt client node bindings.
> > > +
> > > Example:
> > >
> > > vl53l0x@29 {
> > > + pinctrl-names = "default";
> > > + pinctrl-0 = <&vl53l0x_pins>;
> > > +
> > Please drop this from the example. This is board specific rather than
> > being generally required.
> >
Sure.
yours,
Song Qiang
> > > compatible = "st,vl53l0x-i2c";
> > > reg = <0x29>;
> > > -};
> > > + interrupt-parent = <&gpio3>;
> > > + interrupts = <17 IRQ_TYPE_EDGE_FALLING>;
> > > +}
On Fri, Sep 28, 2018 at 4:36 AM Song Qiang <[email protected]> wrote:
>
> On Wed, Sep 26, 2018 at 05:46:18PM -0500, Rob Herring wrote:
> > On Sat, Sep 22, 2018 at 04:05:23PM +0100, Jonathan Cameron wrote:
> > > On Tue, 18 Sep 2018 16:24:22 +0800
> > > Song Qiang <[email protected]> wrote:
> > >
> > > > The first version of this driver issues a measuring request and polling
> > > > for a status register in the device for measuring completes.
> > > > vl53l0x support configuring GPIO1 on it to generate interrupt to
> > > > indicate that new measurement is ready. This patch adds support for
> > > > using this mechanisim to reduce cpu cost.
> > > >
> > > > Signed-off-by: Song Qiang <[email protected]>
> > > Hi Song.
> > >
> > > Looks correct in principal but a few things to tidy up before
> > > this is ready to apply.
> > >
> > > Also we have an unrelated change in here to check the devices ID.
> > > That should be in it's own patch.
> > >
> > > Thanks,
> > >
> > > Jonathan
> > > > ---
> > > > .../bindings/iio/proximity/vl53l0x.txt | 14 +-
> >
> > This should have been split with the complete binding in one patch
> > rather than piecemeal driver feature by feature.
> >
>
> Hi Rob,
>
> A few days ago when I was submitting this driver, I didn't do it very
> well, the function of this driver is limited. I added interrupt support
> the next day after you acked my first patch. I thought it's not polite
> to add something after someone acked that patch, so I send the interrupt
> support as a second patch. The first patch is merged to togreg now, but
> this doesn't. I don't know when can I add new functions to the code that
> just merged to togreg branch, could you offer some suggestions?
You just needed to state why you didn't add a ack. But really, just
don't send things except as RFC until they are "done".
What to do next depends on Jonathan and whether he wants a follow-up
patch or he will drop the first one.
> > > > drivers/iio/proximity/vl53l0x-i2c.c | 135 +++++++++++++++---
> > > > 2 files changed, 129 insertions(+), 20 deletions(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > > > index ab9a9539fec4..40290f8dd70f 100644
> > > > --- a/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > > > +++ b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > > > @@ -4,9 +4,21 @@ Required properties:
> > > > - compatible: must be "st,vl53l0x-i2c"
> >
> > Is there more than one interface on this device, such as SPI? If not,
> > then '-i2c' should be dropped.
> >
>
> Yes, there is a CCI(Camera Control Interface) for communication.
Isn't CCI just a subset of I2C? I should clarify my question is
whether there's more than 1 mutually exclusive control interface (as
many devices have control and data interfaces) where you could have 2
different drivers. A common example are devices with I2C and SPI
interfaces.
Rob
On Fri, 28 Sep 2018 18:52:13 -0500
Rob Herring <[email protected]> wrote:
> On Fri, Sep 28, 2018 at 4:36 AM Song Qiang <[email protected]> wrote:
> >
> > On Wed, Sep 26, 2018 at 05:46:18PM -0500, Rob Herring wrote:
> > > On Sat, Sep 22, 2018 at 04:05:23PM +0100, Jonathan Cameron wrote:
> > > > On Tue, 18 Sep 2018 16:24:22 +0800
> > > > Song Qiang <[email protected]> wrote:
> > > >
> > > > > The first version of this driver issues a measuring request and polling
> > > > > for a status register in the device for measuring completes.
> > > > > vl53l0x support configuring GPIO1 on it to generate interrupt to
> > > > > indicate that new measurement is ready. This patch adds support for
> > > > > using this mechanisim to reduce cpu cost.
> > > > >
> > > > > Signed-off-by: Song Qiang <[email protected]>
> > > > Hi Song.
> > > >
> > > > Looks correct in principal but a few things to tidy up before
> > > > this is ready to apply.
> > > >
> > > > Also we have an unrelated change in here to check the devices ID.
> > > > That should be in it's own patch.
> > > >
> > > > Thanks,
> > > >
> > > > Jonathan
> > > > > ---
> > > > > .../bindings/iio/proximity/vl53l0x.txt | 14 +-
> > >
> > > This should have been split with the complete binding in one patch
> > > rather than piecemeal driver feature by feature.
> > >
> >
> > Hi Rob,
Hi Rob, Song,
> >
> > A few days ago when I was submitting this driver, I didn't do it very
> > well, the function of this driver is limited. I added interrupt support
> > the next day after you acked my first patch. I thought it's not polite
> > to add something after someone acked that patch, so I send the interrupt
> > support as a second patch. The first patch is merged to togreg now, but
> > this doesn't. I don't know when can I add new functions to the code that
> > just merged to togreg branch, could you offer some suggestions?
>
> You just needed to state why you didn't add a ack. But really, just
> don't send things except as RFC until they are "done".
The RFC bit actually disagree on. This driver could be considered 'done'
with just patch 1. The driver worked, it was useful. When the early
versions of that patch came out Song had no idea how much work it would
be to add interrupt support. As it turns out it was simple - it often isn't :(
>
> What to do next depends on Jonathan and whether he wants a follow-up
> patch or he will drop the first one.
Yeah. I should have picked up on the binding in the second patch and merged
it. I'd seen the first patch a few times before so was happy with it and
applied before actually looking at the second.
If they had come in separate series in my view the partial binding then
extend with 'optional' elements is fine. The number of times I've discovered
issues with documentation of hardware that would have made any binding written
from the docs wrong is non trivial. So in my view it is always a gamble to
write bindings until you have tested they work. I have not problem with
people who are confident and want to add them from the start though.
Obviously this only works for optional elements.
So follow up patch for 'optional' stuff is fine by me. The only real
issue to my mind here is that they were in the same series, so we should
have seen a separate precursor patch giving the binding for all of it.
>
> > > > > drivers/iio/proximity/vl53l0x-i2c.c | 135 +++++++++++++++---
> > > > > 2 files changed, 129 insertions(+), 20 deletions(-)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > > > > index ab9a9539fec4..40290f8dd70f 100644
> > > > > --- a/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > > > > +++ b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > > > > @@ -4,9 +4,21 @@ Required properties:
> > > > > - compatible: must be "st,vl53l0x-i2c"
> > >
> > > Is there more than one interface on this device, such as SPI? If not,
> > > then '-i2c' should be dropped.
> > >
> >
> > Yes, there is a CCI(Camera Control Interface) for communication.
>
> Isn't CCI just a subset of I2C? I should clarify my question is
> whether there's more than 1 mutually exclusive control interface (as
> many devices have control and data interfaces) where you could have 2
> different drivers. A common example are devices with I2C and SPI
> interfaces.
Already fixed up when I applied the first patch. I did more rework on
patch 1 than I'd normally do as I'd sent Song down a dead end with an
incorrect requested change so didn't want to waste more of his time
with a v7 of that patch. This patch 2 was pretty much new so different
matter!
Thanks,
Jonathan
>
> Rob
On Sat, Sep 29, 2018 at 6:10 AM Jonathan Cameron <[email protected]> wrote:
>
> On Fri, 28 Sep 2018 18:52:13 -0500
> Rob Herring <[email protected]> wrote:
>
> > On Fri, Sep 28, 2018 at 4:36 AM Song Qiang <[email protected]> wrote:
> > >
> > > On Wed, Sep 26, 2018 at 05:46:18PM -0500, Rob Herring wrote:
> > > > On Sat, Sep 22, 2018 at 04:05:23PM +0100, Jonathan Cameron wrote:
> > > > > On Tue, 18 Sep 2018 16:24:22 +0800
> > > > > Song Qiang <[email protected]> wrote:
> > > > >
> > > > > > The first version of this driver issues a measuring request and polling
> > > > > > for a status register in the device for measuring completes.
> > > > > > vl53l0x support configuring GPIO1 on it to generate interrupt to
> > > > > > indicate that new measurement is ready. This patch adds support for
> > > > > > using this mechanisim to reduce cpu cost.
> > > > > >
> > > > > > Signed-off-by: Song Qiang <[email protected]>
> > > > > Hi Song.
> > > > >
> > > > > Looks correct in principal but a few things to tidy up before
> > > > > this is ready to apply.
> > > > >
> > > > > Also we have an unrelated change in here to check the devices ID.
> > > > > That should be in it's own patch.
> > > > >
> > > > > Thanks,
> > > > >
> > > > > Jonathan
> > > > > > ---
> > > > > > .../bindings/iio/proximity/vl53l0x.txt | 14 +-
> > > >
> > > > This should have been split with the complete binding in one patch
> > > > rather than piecemeal driver feature by feature.
> > > >
> > >
> > > Hi Rob,
>
> Hi Rob, Song,
>
> > >
> > > A few days ago when I was submitting this driver, I didn't do it very
> > > well, the function of this driver is limited. I added interrupt support
> > > the next day after you acked my first patch. I thought it's not polite
> > > to add something after someone acked that patch, so I send the interrupt
> > > support as a second patch. The first patch is merged to togreg now, but
> > > this doesn't. I don't know when can I add new functions to the code that
> > > just merged to togreg branch, could you offer some suggestions?
> >
> > You just needed to state why you didn't add a ack. But really, just
> > don't send things except as RFC until they are "done".
>
> The RFC bit actually disagree on. This driver could be considered 'done'
> with just patch 1. The driver worked, it was useful. When the early
> versions of that patch came out Song had no idea how much work it would
> be to add interrupt support. As it turns out it was simple - it often isn't :(
I meant specifically in the context of this getting revised within a
number of days. I agree that drivers don't have to be complete, but
bindings should be as complete as possible. You can't foresee
everything, but I don't think that applies in this case.
> > What to do next depends on Jonathan and whether he wants a follow-up
> > patch or he will drop the first one.
>
> Yeah. I should have picked up on the binding in the second patch and merged
> it. I'd seen the first patch a few times before so was happy with it and
> applied before actually looking at the second.
>
> If they had come in separate series in my view the partial binding then
> extend with 'optional' elements is fine. The number of times I've discovered
> issues with documentation of hardware that would have made any binding written
> from the docs wrong is non trivial. So in my view it is always a gamble to
> write bindings until you have tested they work. I have not problem with
> people who are confident and want to add them from the start though.
Well, if they were broken is some way, I don't think backwards
compatibility matters and we can still fix things. I'm not talking
about complex cases here. It is pretty trivial to determine whether a
device has an interrupt or not.
>
> Obviously this only works for optional elements.
>
> So follow up patch for 'optional' stuff is fine by me. The only real
> issue to my mind here is that they were in the same series, so we should
> have seen a separate precursor patch giving the binding for all of it.
Certainly, that can't be avoided sometimes and is fine. It's really
the time frame for this particular case and reviewer bandwidth.
Rob
On 30 September 2018 00:49:43 BST, Rob Herring <[email protected]> wrote:
>On Sat, Sep 29, 2018 at 6:10 AM Jonathan Cameron <[email protected]>
>wrote:
>>
>> On Fri, 28 Sep 2018 18:52:13 -0500
>> Rob Herring <[email protected]> wrote:
>>
>> > On Fri, Sep 28, 2018 at 4:36 AM Song Qiang
><[email protected]> wrote:
>> > >
>> > > On Wed, Sep 26, 2018 at 05:46:18PM -0500, Rob Herring wrote:
>> > > > On Sat, Sep 22, 2018 at 04:05:23PM +0100, Jonathan Cameron
>wrote:
>> > > > > On Tue, 18 Sep 2018 16:24:22 +0800
>> > > > > Song Qiang <[email protected]> wrote:
>> > > > >
>> > > > > > The first version of this driver issues a measuring request
>and polling
>> > > > > > for a status register in the device for measuring
>completes.
>> > > > > > vl53l0x support configuring GPIO1 on it to generate
>interrupt to
>> > > > > > indicate that new measurement is ready. This patch adds
>support for
>> > > > > > using this mechanisim to reduce cpu cost.
>> > > > > >
>> > > > > > Signed-off-by: Song Qiang <[email protected]>
>> > > > > Hi Song.
>> > > > >
>> > > > > Looks correct in principal but a few things to tidy up before
>> > > > > this is ready to apply.
>> > > > >
>> > > > > Also we have an unrelated change in here to check the devices
>ID.
>> > > > > That should be in it's own patch.
>> > > > >
>> > > > > Thanks,
>> > > > >
>> > > > > Jonathan
>> > > > > > ---
>> > > > > > .../bindings/iio/proximity/vl53l0x.txt | 14 +-
>> > > >
>> > > > This should have been split with the complete binding in one
>patch
>> > > > rather than piecemeal driver feature by feature.
>> > > >
>> > >
>> > > Hi Rob,
>>
>> Hi Rob, Song,
>>
>> > >
>> > > A few days ago when I was submitting this driver, I didn't do it
>very
>> > > well, the function of this driver is limited. I added interrupt
>support
>> > > the next day after you acked my first patch. I thought it's not
>polite
>> > > to add something after someone acked that patch, so I send the
>interrupt
>> > > support as a second patch. The first patch is merged to togreg
>now, but
>> > > this doesn't. I don't know when can I add new functions to the
>code that
>> > > just merged to togreg branch, could you offer some suggestions?
>> >
>> > You just needed to state why you didn't add a ack. But really, just
>> > don't send things except as RFC until they are "done".
>>
>> The RFC bit actually disagree on. This driver could be considered
>'done'
>> with just patch 1. The driver worked, it was useful. When the early
>> versions of that patch came out Song had no idea how much work it
>would
>> be to add interrupt support. As it turns out it was simple - it
>often isn't :(
>
>I meant specifically in the context of this getting revised within a
>number of days. I agree that drivers don't have to be complete, but
>bindings should be as complete as possible. You can't foresee
>everything, but I don't think that applies in this case.
>
>> > What to do next depends on Jonathan and whether he wants a
>follow-up
>> > patch or he will drop the first one.
>>
>> Yeah. I should have picked up on the binding in the second patch and
>merged
>> it. I'd seen the first patch a few times before so was happy with it
>and
>> applied before actually looking at the second.
>>
>> If they had come in separate series in my view the partial binding
>then
>> extend with 'optional' elements is fine. The number of times I've
>discovered
>> issues with documentation of hardware that would have made any
>binding written
>> from the docs wrong is non trivial. So in my view it is always a
>gamble to
>> write bindings until you have tested they work. I have not problem
>with
>> people who are confident and want to add them from the start though.
>
>Well, if they were broken is some way, I don't think backwards
>compatibility matters and we can still fix things. I'm not talking
>about complex cases here. It is pretty trivial to determine whether a
>device has an interrupt or not.
>
>>
>> Obviously this only works for optional elements.
>>
>> So follow up patch for 'optional' stuff is fine by me. The only real
>> issue to my mind here is that they were in the same series, so we
>should
>> have seen a separate precursor patch giving the binding for all of
>it.
>
>Certainly, that can't be avoided sometimes and is fine. It's really
>the time frame for this particular case and reviewer bandwidth.
Agreed. The timing wasn't ideal (in this limited sense) Song got this done much faster than I normally expect
when someone says, I'll do this later!
Thanks for clarifying.
Jonathan
>
>Rob
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
On Mon, 1 Oct 2018 11:58:14 +0800
Song Qiang <[email protected]> wrote:
> On Fri, Sep 28, 2018 at 06:52:13PM -0500, Rob Herring wrote:
> > On Fri, Sep 28, 2018 at 4:36 AM Song Qiang <[email protected]> wrote:
> > >
> > > On Wed, Sep 26, 2018 at 05:46:18PM -0500, Rob Herring wrote:
> > > > On Sat, Sep 22, 2018 at 04:05:23PM +0100, Jonathan Cameron wrote:
> > > > > On Tue, 18 Sep 2018 16:24:22 +0800
> > > > > Song Qiang <[email protected]> wrote:
> > > > >
> > > > > > The first version of this driver issues a measuring request and polling
> > > > > > for a status register in the device for measuring completes.
> > > > > > vl53l0x support configuring GPIO1 on it to generate interrupt to
> > > > > > indicate that new measurement is ready. This patch adds support for
> > > > > > using this mechanisim to reduce cpu cost.
> > > > > >
> > > > > > Signed-off-by: Song Qiang <[email protected]>
> > > > > Hi Song.
> > > > >
> > > > > Looks correct in principal but a few things to tidy up before
> > > > > this is ready to apply.
> > > > >
> > > > > Also we have an unrelated change in here to check the devices ID.
> > > > > That should be in it's own patch.
> > > > >
> > > > > Thanks,
> > > > >
> > > > > Jonathan
> > > > > > ---
> > > > > > .../bindings/iio/proximity/vl53l0x.txt | 14 +-
> > > >
> > > > This should have been split with the complete binding in one patch
> > > > rather than piecemeal driver feature by feature.
> > > >
> > >
> > > Hi Rob,
> > >
> > > A few days ago when I was submitting this driver, I didn't do it very
> > > well, the function of this driver is limited. I added interrupt support
> > > the next day after you acked my first patch. I thought it's not polite
> > > to add something after someone acked that patch, so I send the interrupt
> > > support as a second patch. The first patch is merged to togreg now, but
> > > this doesn't. I don't know when can I add new functions to the code that
> > > just merged to togreg branch, could you offer some suggestions?
> >
> > You just needed to state why you didn't add a ack. But really, just
> > don't send things except as RFC until they are "done".
> >
> > What to do next depends on Jonathan and whether he wants a follow-up
> > patch or he will drop the first one.
> >
> > > > > > drivers/iio/proximity/vl53l0x-i2c.c | 135 +++++++++++++++---
> > > > > > 2 files changed, 129 insertions(+), 20 deletions(-)
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > > > > > index ab9a9539fec4..40290f8dd70f 100644
> > > > > > --- a/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > > > > > +++ b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > > > > > @@ -4,9 +4,21 @@ Required properties:
> > > > > > - compatible: must be "st,vl53l0x-i2c"
> > > >
> > > > Is there more than one interface on this device, such as SPI? If not,
> > > > then '-i2c' should be dropped.
> > > >
> > >
> > > Yes, there is a CCI(Camera Control Interface) for communication.
> >
> > Isn't CCI just a subset of I2C? I should clarify my question is
> > whether there's more than 1 mutually exclusive control interface (as
> > many devices have control and data interfaces) where you could have 2
> > different drivers. A common example are devices with I2C and SPI
> > interfaces.
> >
> > Rob
>
> Hi Rob, Jonathan,
>
> I don't know things about CCI very well, and google also tells me little
> about it. Actually, I found it difficult to find a standard definition
> about it. Then I dug into vl53l0x's API source code, and what I can
> tell is when we use these two interfaces, the whole programming
> framework is different, even though the pysical bus of them are likely
> to be the same.
> Source code of CCI uses a 'msm_camera_i2c_fn_t' struct and a
> 'v4l2_file_operations' as hardware interfaces for controlling device,
> while the i2c one just uses generic i2c bus interfaces.
>
> This explanation is for why the file name still contains '-i2c'.
>
File names are easy to change in future compared to device tree bindings
(which may be fixed for ever in an embedded platform).
So this isn't a problem at all.
Thanks,
Jonathan
> yours,
> Song Qiang