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

[Sirat]

On Sat, Mar 7, 2026 at 11:17 PM David Lechner <dlechner@xxxxxxxxxxxx> wrote:
>
> On 3/3/26 3:02 AM, Siratul Islam wrote:
> > Add support for the STMicroelectronics VL53L1X Time-of-Flight
> > ranging sensor with I2C interface.
> >
>
> ...
>
Hi, Thank you for reviewing the patch! Please find my responses below.
>
>
> > +static int vl53l1x_set_distance_mode(struct vl53l1x_data *data,
...
> > +             if (ret)
> > +                     return ret;
> > +             ret = regmap_write(data->regmap,
> > +                                VL53L1X_SD_CONFIG__INITIAL_PHASE_SD1, 6);
>
> Maybe worth using regmap_multi_reg_write() above and below?
>
Done. Put the register values into const reg_sequence arrays
per mode and use regmap_multi_reg_write(). Much cleaner.
>
>
> > +     case IIO_CHAN_INFO_RAW:
>
> What is here is perfectly fine, but I'm sure someone will eventually
> want to change it to IIO_DEV_ACQUIRE_DIRECT_MODE().
>
> > +             if (!iio_device_claim_direct(indio_dev))
> > +                     return -EBUSY;
> > +             ret = vl53l1x_read_proximity(data, val);
> > +             iio_device_release_direct(indio_dev);
>
Noted. keeping iio_device_claim_direct() for now.
>
> > +
...
> > +     ret = regmap_read(data->regmap, VL53L1X_RESULT__RANGE_STATUS,
> > +                       &range_status);
> > +     if (ret || (range_status & VL53L1X_RANGE_STATUS_MASK) !=
>
> Probably best to use FIELD_GET() here and redefine VL53L1X_RANGE_STATUS_VALID
> accrodingly.
>
Switched to FIELD_GET() in both the trigger handler and read_proximity.
>
...
> > +
> > +static int vl53l1x_configure_irq(struct i2c_client *client,
> > +                              struct iio_dev *indio_dev)
>
> Would be more logical to move this function closer to probe.
>
Moved it right above probe.
>
> > +{
> > +     struct vl53l1x_data *data = iio_priv(indio_dev);
> > +     int irq_flags = irq_get_trigger_type(client->irq);
> > +     int ret;
> > +
> > +     if (!irq_flags)
> > +             irq_flags = IRQF_TRIGGER_FALLING;
> > +
> > +     ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> > +                                     vl53l1x_threaded_irq,
> > +                                     irq_flags | IRQF_ONESHOT,
> > +                                     indio_dev->name, indio_dev);
> > +     if (ret) {
> > +             dev_err(&client->dev, "devm_request_irq error: %d\n", ret);
>
>
> Can use return dev_err_probe() here since this is only called in probe.
>
Done for both error paths.
>
> > +             return ret;
> > +     }
> > +
> > +     ret = regmap_write(data->regmap, VL53L1X_SYSTEM__INTERRUPT_CONFIG_GPIO,
> > +                        VL53L1X_INT_NEW_SAMPLE_READY);
> > +     if (ret)
> > +             dev_err(&client->dev, "failed to configure IRQ: %d\n", ret);
>
> ditto
>
> > +
...
> > +
> > +static const struct iio_buffer_setup_ops vl53l1x_buffer_setup_ops = {
> > +     .postenable = &vl53l1x_buffer_postenable,
> > +     .postdisable = &vl53l1x_buffer_postdisable,
> > +};
>
> These are not symetric. It either needs to be postenable/predisable
> or preenable/postdisable.
>
Fixed.
>

Thanks again,
Sirat