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

From: Sirat

Date: Thu Mar 26 2026 - 08:26:05 EST

On Thu, Mar 26, 2026 at 4:14 PM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxx> wrote:
>
> 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
>
>
>
Hi Andy!

Thanks for the review again. I will send a small separate cleanup
patch for this once it gets to a more stable stage.

Sirat