Re: [PATCH] iio: proximity: Add driver support for ST's VL53L0X ToF ranging sensor.

From: Himanshu Jha
Date: Mon Sep 10 2018 - 13:58:02 EST

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)


> + 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