Re: [PATCH] iio: proximity: Add driver support for ST's VL53L0X ToF ranging sensor.
From: Song Qiang
Date: Tue Sep 11 2018 - 02:46:52 EST
On Mon, Sep 10, 2018 at 11:27:47PM +0530, Himanshu Jha wrote:
> On Mon, Sep 10, 2018 at 10:42:59PM +0800, Song Qiang wrote:
> > This driver was originally written by ST in 2016 as a misc input device,
> > and hasn't been maintained for a long time. I grabbed some code from
> > it's API and reformed it to a iio proximity device driver.
> > This version of driver uses i2c bus to talk to the sensor and
> > polling for measuring completes, so no irq line is needed.
> > This version of driver supports only one-shot mode, and it can be
> > tested with reading from
> > /sys/bus/iio/devices/iio:deviceX/in_distance_raw
> >
> > Signed-off-by: Song Qiang <songqiang.1304521@xxxxxxxxx>
> > ---
>
> The Cc list contains developers who might not be relevant
> for the discussion.
>
> So, copy only those people listed by:
>
> $./scripts/get_maintainer.pl <driver.patch>
>
> Don't know why Kate & Greg are cc'ed ?
>
> > .../bindings/iio/proximity/vl53l0x.txt | 12 +
> > drivers/iio/proximity/Kconfig | 13 +
> > drivers/iio/proximity/Makefile | 2 +
> > drivers/iio/proximity/vl53l0x-i2c.c | 295 ++++++++++++++++++
> > 4 files changed, 322 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > create mode 100644 drivers/iio/proximity/vl53l0x-i2c.c
> >
> > diff --git a/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > new file mode 100644
> > index 000000000000..64b69442f08e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > @@ -0,0 +1,12 @@
> > +ST's VL53L0X ToF ranging sensor
> > +
> > +Required properties:
> > + - compatible: must be "st,vl53l0x-i2c"
> > + - reg: i2c address where to find the device
> > +
> > +Example:
> > +
> > +vl53l0x@29 {
> > + compatible = "st,vl53l0x-i2c";
> > + reg = <0x29>;
> > +};
> > diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
> > index f726f9427602..1563a5f9144d 100644
> > --- a/drivers/iio/proximity/Kconfig
> > +++ b/drivers/iio/proximity/Kconfig
> > @@ -79,4 +79,17 @@ config SRF08
> > To compile this driver as a module, choose M here: the
> > module will be called srf08.
> >
> > +config VL53L0X_I2C
> > + tristate "STMicroelectronics VL53L0X ToF ranger sensor (I2C)"
> > + select IIO_BUFFER
> > + select IIO_TRIGGERED_BUFFER
>
> I don't see any buffer/trigger support, so better to remove these
> two options.
>
> > + depends on I2C
> > + help
> > + Say Y here to build a driver for STMicroelectronics VL53L0X
> > + ToF ranger sensors with i2c interface.
> > + This driver can be used to measure the distance of objects.
> > +
> > + To compile this driver as a module, choose M here: the
> > + module will be called vl53l0x-i2c.
>
> `name` attribute will be VL53L0X_DRV_NAME(vl53l0x) if OF matching
> is not used to probe the driver.
>
> > endmenu
> > diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
> > index 4f4ed45e87ef..7cb771665c8b 100644
> > --- a/drivers/iio/proximity/Makefile
> > +++ b/drivers/iio/proximity/Makefile
> > @@ -10,3 +10,5 @@ obj-$(CONFIG_RFD77402) += rfd77402.o
> > obj-$(CONFIG_SRF04) += srf04.o
> > obj-$(CONFIG_SRF08) += srf08.o
> > obj-$(CONFIG_SX9500) += sx9500.o
> > +obj-$(CONFIG_VL53L0X_I2C) += vl53l0x-i2c.o
> > +
> > diff --git a/drivers/iio/proximity/vl53l0x-i2c.c b/drivers/iio/proximity/vl53l0x-i2c.c
> > new file mode 100644
> > index 000000000000..c00713041d30
> > --- /dev/null
> > +++ b/drivers/iio/proximity/vl53l0x-i2c.c
> > @@ -0,0 +1,295 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * vl53l0x-i2c.c - Support for STM VL53L0X FlightSense TOF
> > + * Ranger Sensor on a i2c bus.
> > + *
> > + * Copyright (C) 2016 STMicroelectronics Imaging Division.
> > + * Copyright (C) 2018 Song Qiang <songqiang.1304521@xxxxxxxxx>
> > + *
> > + */
> > +
> > +#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