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

[Sirat]

On Sat, Mar 7, 2026 at 7:59 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Tue,  3 Mar 2026 15:02:41 +0600
> Siratul Islam <email@xxxxxxxx> wrote:
...
> Hi,
>
> Various comments inline.
>
> Pretty nice for a v1 ;)
>
> Jonathan
>
Hi! Thank you for the review!
Please find my responses below regarding the changes for v2
>
...
> > + */
> > +
> > +#include <linux/bits.h>
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/i2c.h>
> > +#include <linux/irq.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>
>
> Check that you have includes for all things used directly in this
> driver. For example dev_err()
>
> We use a slightly relaxed version of include what you use for IIO
> (and the kernel in general). So sometimes there are headers that
> are well documented as including others, but when that's not true
> we don't make assumptions and so include everything relevant even
> if it happens to be included via another path today.
>
I've added linux/device.h, linux/bitfield.h, linux/completion.h, and
linux/mod_devicetable.h. Hopefully this covers everything. For dev_err(),
I was conflicted between linux/dev_printk.h and linux/device.h, but
ended up using the latter.
>
...
> > +#define VL53L1X_IDENTIFICATION__MODEL_ID 0x010F
>
> Register addresses are a case where it can be helpful to use tabs
> to make the addresses all line up.
>
Done
>
> >
> > +static const struct regmap_config vl53l1x_regmap_config = {
> > +     .reg_bits = 16,
> > +     .val_bits = 8,
> > +     .reg_format_endian = REGMAP_ENDIAN_BIG,
>
> 8 bit registers don't really have an endian. This doesn't (I think)
> apply to multiregister writes.
>
Done
>
>
> > +static int vl53l1x_chip_init(struct vl53l1x_data *data)
> > +{
> > +     struct device *dev = &data->client->dev;
> > +     unsigned int val;
> > +     u16 model_id;
> > +     int tries = 200;
> > +     bool ready;
> > +     int ret;
> > +
> > +     ret = regmap_write(data->regmap, VL53L1X_SOFT_RESET, 0x00);
> Is this useful if we have a hardware reset? It's common to do
> hardware if possible and software only if it isn't.
>
The device is now hardware-reset via the XSHUT pin during power-on, so
chip_init only does the software reset sequence when no XSHUT GPIO is
available
>
> > +     if (ret)
> > +             return ret;
> > +     usleep_range(100, 200);
> fsleep() and ideally a spec reference in a comment for why this value.
> > +
> > +     ret = regmap_write(data->regmap, VL53L1X_SOFT_RESET, 0x01);
> > +     if (ret)
> > +             return ret;
> > +     usleep_range(1000, 2000);
> fsleep
>
Switched all usleep_range() to fsleep() and added comments with
why the values
>
> > +
...
> > +
> > +     if (model_id != VL53L1X_MODEL_ID_VAL) {
> > +             dev_err(dev, "unknown model id: 0x%04x\n", model_id);
>
> Don't error out in this case. It breaks any possibility of a future
> backwards compatible chip being supported by this driver via a fallback
> dt compatible.  The most we do is print an informational message
> to help with debug.
>
> We do have some old drivers that we still need to fix up that are
> doing this sort of hard check on an who am I register.
>
>
> > +             return -ENODEV;
> > +     }
> ...
>
Changed to dev_info() and let probe continue.
>
> > +
> > +static int vl53l1x_set_distance_mode(struct vl53l1x_data *data,
> > +                                  enum vl53l1x_distance_mode mode)
> > +{
> > +     int ret;
> > +
> > +     switch (mode) {
> > +     case VL53L1X_SHORT:
> Can we encode all the values into a structure.  Then make this just
> a pick of the structure, followed by writing the values out.
> should end up more compact that this due to the reuse of all the regmap
> calls and error handling.
>
I refactored this using reg_sequence arrays. I also used
regmap_multi_reg_write()
as David suggested in his review below.
>
> > +             ret = regmap_write(data->regmap,
> > +                                VL53L1X_PHASECAL_CONFIG__TIMEOUT_MACROP,
> > +                                0x14);
...
> > +
> > +static int vl53l1x_set_timing_budget(struct vl53l1x_data *data, u16 budget_ms)
> Add some documentation as it's not obvious what this is doing.
>
Added a comment explaining the timing budget.
The pre-computed register values come from ST ULD.
>
...
> > +
> > +                     usleep_range(1000, 5000);
>
> fsleep()
>
> Also add a comment to say why this many tries with this sleep.
> Check for all other places this comment applies.
>
Done
>
...
>
> > +
> > +static int vl53l1x_validate_trigger(struct iio_dev *indio_dev,
> > +                                 struct iio_trigger *trig)
> > +{
> > +     struct vl53l1x_data *data = iio_priv(indio_dev);
> > +
> > +     return data->trig == trig ? 0 : -EINVAL;
>
> Can we use the generic iio_validate_own_trigger()?
>
> Cheats a bit and relies on the parents being the same (which they
> always should be as it's one device.  Advantage
> is no need for a trig variable in data.
>
Yes! Switched to iio_validate_own_trigger()
>
...
> > +
> > +     ret = vl53l1x_read_u16(data,
> > +                            VL53L1X_RESULT__FINAL_CROSSTALK_CORRECTED_RANGE_MM_SD0,
> > +                                &scan.distance);
>
> Another odd bit of alignment. Should match the line above.
>
Fixed.
>
...
> > +
> > +static irqreturn_t vl53l1x_threaded_irq(int irq, void *priv)
> > +{
> > +     struct iio_dev *indio_dev = priv;
> > +     struct vl53l1x_data *data = iio_priv(indio_dev);
> > +
> > +     if (iio_buffer_enabled(indio_dev))
> Neither of these needs to be done in a threaded hander.
> I'd make this a non threaded irq handler and mark the irq
> as IRQF_NO_THREAD as you'll then be using iio_trigger_poll()
>
> The advantage of that is it allows better timestamp gathering
> as nearer the interrupt.
>
Changed to a hardirq handler with iio_trigger_poll() and
IRQF_NO_THREAD. Also switched to devm_request_irq() since
there's no threaded part anymore.
>
...
> > +     if (!irq_flags)
> > +             irq_flags = IRQF_TRIGGER_FALLING;
> Leave that to firmware.  We used to do this a lot in drivers, but it
> was always the wrong approach. The snag is we can't fix drivers that do
> it now without risking regressions. However we don't want to introduce
> new ones.
>
Done. Dropped it.
>
...
> > +
> > +static void vl53l1x_power_off(void *_data)
>
> > +{
> > +     struct vl53l1x_data *data = _data;
> > +
> > +     vl53l1x_stop_ranging(data);
>
> This looks to be undoing something that wasn't done in power_on?
> If so probably wants to be a separate devm cleanup.
>
> Whilst it may not matter as turning things off tends to be idempotent
> it will be easier to follow the code if we only stop what we started.
>
Split into a separate devm_add_action_or_reset() for stop_ranging,
registered right after start_ranging in probe.
>
> > +     gpiod_set_value_cansleep(data->reset_gpio, 1);
>
> It's unusual to put a device into reset on power down.  Is this
> pin perhaps misnamed?
>
> If it is just a reset, we'd normally break that out as separate
> from the power control and force a reset sequence in probe.
>
It's actually the XSHUT pin, not a reset. I wanted to keep standard naming.
But that might become confusing. Renamed it to xshut_gpio and updated
the DT binding to "xshut-gpios".
>
...
> > +     gpiod_set_value_cansleep(data->reset_gpio, 0);
> > +     usleep_range(3200, 5000);
>
> fsleep() preferred.  Adds a standard amount of slack.
>
Done
>
...
> > +     if (!i2c_check_functionality(client->adapter,
> > +                                  I2C_FUNC_SMBUS_READ_I2C_BLOCK |
> > +                                          I2C_FUNC_SMBUS_BYTE_DATA))
> Align this as:
>         if (!i2c_check_functionality(client->adapter,
>                                      I2C_FUNC_SMBUS_READ_I2C_BLOCK |
>                                      I2C_FUNC_SMBUS_BYTE_DATA))
>
> Seems like an extra tab snuck in from somewhere.
>
Fixed.
>
...
> > +             ret = devm_iio_triggered_buffer_setup(&client->dev,
> > +                                                   indio_dev, NULL,
> > +                                                       &vl53l1x_trigger_handler,
> > +                                                       &vl53l1x_buffer_setup_ops);
>
> Strange alignment.
>
Fixed.
>
...
> > +static const struct i2c_device_id vl53l1x_id[] = { { "vl53l1x" }, { } };
> Format this same as
>
> static const struct i2c_device_id vl53l1x_id[] = {
>         { "vl53l1x" },
>         { }
> };
>
Done
>
...
> > +
> > +MODULE_AUTHOR("Siratul Islam <email@xxxxxxxx>");
> > +MODULE_DESCRIPTION("ST VL53L1X ToF ranging sensor driver");
> > +MODULE_LICENSE("Dual BSD/GPL");
>
>

Thanks again,
Sirat