Re: [PATCH v8 2/2] iio: proximity: add driver for ST VL53L1X ToF sensor

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

On Thu, Mar 26, 2026 at 02:19:42AM +0600, Siratul Islam wrote:
> Add support for the STMicroelectronics VL53L1X Time-of-Flight
> ranging sensor with I2C interface.

Some ideas for small followup amendments.

...

> +#define VL53L1X_REG_SOFT_RESET						0x0000
> +#define VL53L1X_REG_VHV_CONFIG__TIMEOUT_MACROP_LOOP_BOUND		0x0008
> +#define VL53L1X_REG_VHV_CONFIG__INIT					0x000B
> +#define VL53L1X_REG_GPIO_HV_MUX__CTRL					0x0030
> +#define VL53L1X_REG_GPIO__TIO_HV_STATUS					0x0031
> +#define VL53L1X_REG_SYSTEM__INTERRUPT_CONFIG_GPIO			0x0046
> +#define VL53L1X_REG_PHASECAL_CONFIG__TIMEOUT_MACROP			0x004B
> +#define VL53L1X_REG_RANGE_CONFIG__TIMEOUT_MACROP_A			0x005E
> +#define VL53L1X_REG_RANGE_CONFIG__VCSEL_PERIOD_A			0x0060
> +#define VL53L1X_REG_RANGE_CONFIG__TIMEOUT_MACROP_B			0x0061
> +#define VL53L1X_REG_RANGE_CONFIG__VCSEL_PERIOD_B			0x0063
> +#define VL53L1X_REG_RANGE_CONFIG__VALID_PHASE_HIGH			0x0069
> +#define VL53L1X_REG_SYSTEM__INTERMEASUREMENT_PERIOD			0x006C
> +#define VL53L1X_REG_SD_CONFIG__WOI_SD0					0x0078
> +#define VL53L1X_REG_SD_CONFIG__WOI_SD1					0x0079
> +#define VL53L1X_REG_SD_CONFIG__INITIAL_PHASE_SD0			0x007A
> +#define VL53L1X_REG_SD_CONFIG__INITIAL_PHASE_SD1			0x007B
> +#define VL53L1X_REG_SYSTEM__INTERRUPT_CLEAR				0x0086
> +#define VL53L1X_REG_SYSTEM__MODE_START					0x0087
> +#define VL53L1X_REG_RESULT__RANGE_STATUS				0x0089
> +#define VL53L1X_REG_RESULT__FINAL_CROSSTALK_CORRECTED_RANGE_MM_SD0	0x0096
> +#define VL53L1X_REG_RESULT__OSC_CALIBRATE_VAL				0x00DE
> +#define VL53L1X_REG_FIRMWARE__SYSTEM_STATUS				0x00E5
> +#define VL53L1X_REG_IDENTIFICATION__MODEL_ID				0x010F
> +#define VL53L1X_REG_DEFAULT_CONFIG					0x002D

Keep the list ordered by the value?

...

> +static int vl53l1x_chip_init(struct vl53l1x_data *data)
> +{
> +	struct device *dev = regmap_get_device(data->regmap);
> +	unsigned int val;
> +	u16 model_id;
> +	int ret;
> +
> +	if (!data->xshut_reset) {
> +		ret = regmap_write(data->regmap, VL53L1X_REG_SOFT_RESET, 0x00);
> +		if (ret)
> +			return ret;
> +		fsleep(100); /* conservative reset pulse, no spec */
> +
> +		ret = regmap_write(data->regmap, VL53L1X_REG_SOFT_RESET, 0x01);
> +		if (ret)
> +			return ret;
> +		fsleep(1000); /* conservative boot wait, no spec */
> +	}
> +
> +	ret = regmap_read_poll_timeout(data->regmap,
> +				       VL53L1X_REG_FIRMWARE__SYSTEM_STATUS, val,
> +				       val & BIT(0),
> +				       1 * USEC_PER_MSEC,
> +				       100 * USEC_PER_MSEC);

Use logical split

	ret = regmap_read_poll_timeout(data->regmap,
				       VL53L1X_REG_FIRMWARE__SYSTEM_STATUS,
				       val, val & BIT(0),
				       1 * USEC_PER_MSEC, 100 * USEC_PER_MSEC);

> +	if (ret)
> +		return dev_err_probe(dev, ret, "firmware boot timeout\n");
> +
> +	ret = vl53l1x_read_u16(data, VL53L1X_REG_IDENTIFICATION__MODEL_ID,
> +			       &model_id);
> +	if (ret)
> +		return ret;
> +
> +	if (model_id != VL53L1X_MODEL_ID_VAL)
> +		dev_info(dev, "unknown model id: 0x%04x, continuing\n", model_id);
> +
> +	ret = regmap_bulk_write(data->regmap, VL53L1X_REG_DEFAULT_CONFIG,
> +				vl53l1x_default_config,
> +				sizeof(vl53l1x_default_config));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(data->regmap, VL53L1X_REG_GPIO_HV_MUX__CTRL, &val);
> +	if (ret)
> +		return ret;
> +	data->gpio_polarity = !!(val & VL53L1X_GPIO_HV_MUX_POLARITY);
> +
> +	/* Initial ranging cycle for VHV calibration */
> +	ret = vl53l1x_start_ranging(data);
> +	if (ret)
> +		return ret;
> +
> +	/* 1ms poll, 1s timeout covers max timing budgets (per ST Ultra Lite Driver) */
> +	ret = regmap_read_poll_timeout(data->regmap,
> +				       VL53L1X_REG_GPIO__TIO_HV_STATUS, val,
> +				       (val & 1) != data->gpio_polarity,
> +				       1 * USEC_PER_MSEC,
> +				       1000 * USEC_PER_MSEC);

Ditto.

	ret = regmap_read_poll_timeout(data->regmap,
				       VL53L1X_REG_GPIO__TIO_HV_STATUS,
				       val, (val & 1) != data->gpio_polarity,
				       1 * USEC_PER_MSEC, 1 * USEC_PER_SEC);

> +	if (ret)
> +		return ret;
> +
> +	ret = vl53l1x_clear_irq(data);
> +	if (ret)
> +		return ret;
> +
> +	ret = vl53l1x_stop_ranging(data);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(data->regmap,
> +			   VL53L1X_REG_VHV_CONFIG__TIMEOUT_MACROP_LOOP_BOUND,
> +			   VL53L1X_VHV_LOOP_BOUND_TWO);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(data->regmap, VL53L1X_REG_VHV_CONFIG__INIT, 0x00);
> +}

...

> +	if (data->irq) {
> +		reinit_completion(&data->completion);
> +
> +		ret = vl53l1x_clear_irq(data);
> +		if (ret)
> +			return ret;
> +
> +		if (!wait_for_completion_timeout(&data->completion, HZ))
> +			return -ETIMEDOUT;
> +	} else {
> +		unsigned int rdy;
> +
> +		/* 1ms poll, 1s timeout covers max timing budgets (per ST Ultra Lite Driver) */
> +		ret = regmap_read_poll_timeout(data->regmap,
> +					       VL53L1X_REG_GPIO__TIO_HV_STATUS, rdy,
> +					       (rdy & 1) != data->gpio_polarity,
> +					       1 * USEC_PER_MSEC,
> +					       1000 * USEC_PER_MSEC);

Ditto.

		ret = regmap_read_poll_timeout(data->regmap,
					       VL53L1X_REG_GPIO__TIO_HV_STATUS,
					       rdy, (rdy & 1) != data->gpio_polarity,
					       1 * USEC_PER_MSEC, 1 * USEC_PER_SEC);

Yes, in this case they are slightly longer than 80 characters. But
looking at the above this entire call should be a helper, so you can
reuse it here and above.

> +		if (ret)
> +			return ret;
> +	}

-- 
With Best Regards,
Andy Shevchenko