Message-ID: <20180911070911.GA387@Eros>
Date:   Tue, 11 Sep 2018 15:09:11 +0800
From:   Song Qiang <songqiang1304521@...il.com>
To:     Himanshu Jha <himanshujha199640@...il.com>
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: proximity: Add driver support for ST's VL53L0X ToF
 ranging sensor.

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

Hi Himanshu,

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

Many thanks for spending time with my patch.

yours,
Song Qiang

Powered by blists - more mailing lists