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 <[email protected]>
---
.../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 <[email protected]>
+ *
+ */
+
+#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 <[email protected]>");
+MODULE_DESCRIPTION("ST vl53l0x ToF ranging sensor");
+MODULE_LICENSE("GPL");
--
2.17.1
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 <[email protected]>
> + *
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 <[email protected]>");
> +MODULE_DESCRIPTION("ST vl53l0x ToF ranging sensor");
> +MODULE_LICENSE("GPL");
> --
> 2.17.1
>
--
With Best Regards,
Andy Shevchenko
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 <[email protected]>
> ---
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 <[email protected]>
> + *
> + */
> +
> +#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
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 <[email protected]>
>
> > + *
>
> 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 <[email protected]>");
> > +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
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 <[email protected]>
> > ---
>
> 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 <[email protected]>
> > + *
> > + */
> > +
> > +#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
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 <[email protected]>
> > ---
>
> 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 <[email protected]>
> > + *
> > + */
> > +
> > +#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
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 <[email protected]>
> > ---
>
> 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 <[email protected]>
> > + *
> > + */
> > +
> > +#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
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 <[email protected]>
> > > ---
> >
> > 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 <[email protected]>
> > > + *
> > > + */
> > > +
> > > +#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
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 <[email protected]>
> > > > ---
> > >
> > > 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 <[email protected]>
> > > > + *
> > > > + */
> > > > +
> > > > +#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