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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Tue, Oct 20, 2026 at 12:03:43AM +0530, Vidhu Sarwal via B4 Relay wrote:
> 
> 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.

On top of my previous reply.

...

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

It also makes sense to have local variable here

	struct regmap *map;


> +	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");

	map = devm_regmap_init_i2c(client, &tf_luna_regmap_config);
	if (IS_ERR(map))
		return dev_err_probe(dev, PTR_ERR(map), "Failed to initialize regmap\n");
	data->regmap = map;

> +	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);
> +
> +	ret = regmap_bulk_read(data->regmap, TF_LUNA_REG_SIGNATURE, signature,
> +			       sizeof(signature));

Make it split logically and taking into account the above either

	ret = regmap_bulk_read(map, TF_LUNA_REG_SIGNATURE,
			       signature, sizeof(signature));

OR

	ret = regmap_bulk_read(map, TF_LUNA_REG_SIGNATURE, signature, sizeof(signature));

(perhaps you can shorten the 'signature' to 'sign' to fit).

> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to read signature\n");
> +
> +	if (memcmp(signature, "LUNA", sizeof(signature)))

Hmm... No. Please, make signature to be __?e32 and use a definition for this
FourCC [1] in the hexadecimal with the comment, and compare as regular integers...

> +		dev_info(dev, "Unknown signature %*ph, continuing\n",
> +			 (int)sizeof(signature), signature);

...and here use %*p4c?.

#define	TF_LUNA_SIGN	0x... /* "LUNA" in ASCII */

	__?e32 signature;
	u32 fourcc;
	...
	ret = regmap_bulk_read(map, TF_LUNA_REG_SIGNATURE, signature, sizeof(signature));
	...
	fourcc = ?e32_to_cpu(signature);
	if (fourcc != TF_LUNA_SIGN)
		dev_info(dev, "Unknown signature %p4c?, continuing\n", &fourcc);

where '?' represents either 'b' or 'l' depending on what you have and what you
want to see printed.

> +	ret = regmap_bulk_read(data->regmap, TF_LUNA_REG_VERSION, version,
> +			       sizeof(version));

	ret = regmap_bulk_read(map, TF_LUNA_REG_VERSION, version, sizeof(version));

(or 'version' --> 'ver', for example).

> +	if (ret)
> +		return ret;
>
> +	dev_dbg(dev, "Firmware version %u.%u.%u\n",
> +		version[2], version[1], version[0]);

Yeah, I need to post my series to also make this to be a pointer extension.

> +	/*
> +	 * In continuous mode a read can catch the sensor updating its result
> +	 * registers unless it is synchronised to the data ready output. Use
> +	 * trigger mode and start each measurement on demand instead.
> +	 */
> +	ret = regmap_write(data->regmap, TF_LUNA_REG_MODE, TF_LUNA_MODE_TRIGGER);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * The sensor silently drops writes it is not ready for, so check that
> +	 * the mode was set. A write takes up to 100 ms to read back.
> +	 */
> +	fsleep(100 * USEC_PER_MSEC);

msleep() as it's known to be big enough.

> +	ret = regmap_read(data->regmap, TF_LUNA_REG_MODE, &mode);
> +	if (ret)
> +		return ret;
> +
> +	if (mode != TF_LUNA_MODE_TRIGGER)
> +		return dev_err_probe(dev, -EIO, "Failed to set trigger mode\n");
> +
> +	indio_dev->name = "tf-luna";
> +	indio_dev->info = &tf_luna_info;
> +	indio_dev->channels = tf_luna_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(tf_luna_channels);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

[1]: https://en.wikipedia.org/wiki/FourCC

-- 
With Best Regards,
Andy Shevchenko