Re: [PATCH 3/3] iio: proximity: Add Benewake TF-Luna LiDAR driver

[Joshua Crofts <joshua.crofts1@gmail.com>]

On Tue, 20 Oct 2026 00:03:43 +0530
Vidhu Sarwal via B4 Relay <devnull+vidhu.linux.gmail.com@kernel.org> wrote:

> From: Vidhu Sarwal <vidhu.linux@gmail.com>
> 
> Add a driver for the Benewake TF-Luna, a single-point time-of-flight
> LiDAR distance sensor with a range of 0.2 m to 8 m, connected over I2C.
> 
> The sensor is run in trigger mode. Each read starts one measurement and
> waits for the timestamp register to change before reading the result.
> In continuous mode a read can catch the sensor in the middle of updating
> its result registers unless it is synchronised to the data ready output,
> which this driver does not use yet.
> 
> When the signal strength is too low the sensor reports a dummy distance
> of 0, so such readings, as well as overexposed ones, return -ENODATA
> instead of a distance.
> 
> The datasheet is available at:
> https://en.benewake.com/uploadfiles/2025/04/20250430174501874.pdf
> 
> Signed-off-by: Vidhu Sarwal <vidhu.linux@gmail.com>
> ---

Hi Vidhu,

very good for a v1, even Sashiko reported no issues. Minor style issues from
me.

Josh

> +#include <linux/array_size.h>
> +#include <linux/bits.h>
> +#include <linux/cleanup.h>
> +#include <linux/delay.h>
> +#include <linux/dev_printk.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/iopoll.h>
> +#include <linux/mod_devicetable.h>

Remove this, this is included by i2c.h

> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/string.h>
> +#include <linux/time.h>
> +#include <linux/types.h>
> +
> +#include <asm/byteorder.h>
> +
> +#include <linux/iio/iio.h>
> +
> +#define TF_LUNA_REG_DIST		0x00
> +#define TF_LUNA_REG_TICK		0x06
> +#define TF_LUNA_REG_VERSION		0x0A
> +#define TF_LUNA_REG_MODE		0x23
> +#define TF_LUNA_REG_TRIG_ONE_SHOT	0x24
> +#define TF_LUNA_REG_SIGNATURE		0x3C
> +
> +#define TF_LUNA_MODE_TRIGGER		0x01
> +#define TF_LUNA_TRIG_ONE_SHOT		0x01
> +
> +/* Below this the sensor reports a dummy distance of 0 */
> +#define TF_LUNA_AMP_MIN			100
> +#define TF_LUNA_AMP_OVEREXPOSED		0xFFFF
> +
> +/* A measurement takes 10-20 ms in trigger mode */
> +#define TF_LUNA_POLL_US			(2 * USEC_PER_MSEC)
> +#define TF_LUNA_TIMEOUT_US		(100 * USEC_PER_MSEC)

uS is better instead of US.

> +
> +struct tf_luna_data {
> +	struct regmap *regmap;
> +	/* Serialises the trigger, poll and read of a single measurement */
> +	struct mutex lock;
> +};
> +
> +static const struct regmap_config tf_luna_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = TF_LUNA_REG_SIGNATURE + 3,
> +};
> +
> +static int tf_luna_read_tick(struct tf_luna_data *data, u16 *tick)
> +{
> +	__le16 val;
> +	int ret;
> +
> +	ret = regmap_bulk_read(data->regmap, TF_LUNA_REG_TICK, &val, sizeof(val));
> +	if (ret)
> +		return ret;
> +
> +	*tick = le16_to_cpu(val);
> +	return 0;
> +}
> +
> +static int tf_luna_measure(struct tf_luna_data *data, int *dist)
> +{
> +	__le16 buf[2];	/* distance in cm, signal strength */
> +	u16 old_tick, tick, amp;
> +	int ret, err;
> +
> +	guard(mutex)(&data->lock);
> +
> +	ret = tf_luna_read_tick(data, &old_tick);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(data->regmap, TF_LUNA_REG_TRIG_ONE_SHOT,
> +			   TF_LUNA_TRIG_ONE_SHOT);

Why not have a local variable for regmap? It would allow the previous call
to fit on one line.

...

> +static int tf_luna_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct tf_luna_data *data;
> +	struct iio_dev *indio_dev;
> +	unsigned int mode;
> +	u8 signature[4];
> +	u8 version[3];
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +
> +	ret = devm_mutex_init(dev, &data->lock);
> +	if (ret)
> +		return ret;
> +
> +	data->regmap = devm_regmap_init_i2c(client, &tf_luna_regmap_config);
> +	if (IS_ERR(data->regmap))
> +		return dev_err_probe(dev, PTR_ERR(data->regmap),
> +				     "Failed to initialize regmap\n");
> +
> +	ret = devm_regulator_get_enable(dev, "vcc");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable vcc regulator\n");
> +
> +	/* Boot time is not documented, about 350 ms was measured */
> +	fsleep(500 * USEC_PER_MSEC);

msleep() is fine here, saves you the x * USEC_PER_MSEC format.

-- 
Kind regards,
Joshua Crofts