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

[Jonathan Cameron]

On Wed, 25 Mar 2026 22:33:22 +0600
Sirat <email@xxxxxxxx> wrote:

> On Wed, Mar 25, 2026 at 8:47 PM Jonathan Cameron
> <jonathan.cameron@xxxxxxxxxx> wrote:
> >
> > On Wed, 25 Mar 2026 12:32:23 +0600
> > Siratul Islam <email@xxxxxxxx> wrote:
> >  
> > > Add support for the STMicroelectronics VL53L1X Time-of-Flight
> > > ranging sensor with I2C interface.
> > >
> > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
> > > Signed-off-by: Siratul Islam <email@xxxxxxxx>  
> >
> > Hi Siratul,
> >
> > I used this as a bit of an experiment as you'd +CC lkml and took a close
> > read of the feedback google's AI bot produced.
> > https://sashiko.dev/#/patchset/20260325063254.18062-1-email%40sirat.me
> >
> > Be very careful when considering the output. There are some things
> > in here that are not true, or we don't generally defend against (like the
> > interrupt type related comments).
> >
> > The iio_trigger_get() is (I think) a more general problem so ignore that
> > for your driver - will just be a tiny memory leak and stop the
> > module being easily unloaded.  I'll take a look at that one on a more general
> > basis.
> >
> > The regmap one is where these bots excel in that they sometimes go deeper
> > in analysis than a typical person focused on one driver.
> >
> > The stuff on using standard devm_ wasn't from the bot and I think will make
> > things rather simpler.
> >
> > Thanks,
> >
> > Jonathan
> >
> >  
> > > ---
> > >  MAINTAINERS                         |   1 +
> > >  drivers/iio/proximity/Kconfig       |  15 +
> > >  drivers/iio/proximity/Makefile      |   1 +
> > >  drivers/iio/proximity/vl53l1x-i2c.c | 795 ++++++++++++++++++++++++++++
> > >  4 files changed, 812 insertions(+)
> > >  create mode 100644 drivers/iio/proximity/vl53l1x-i2c.c
> > >  
> >  
> > > diff --git a/drivers/iio/proximity/vl53l1x-i2c.c b/drivers/iio/proximity/vl53l1x-i2c.c
> > > new file mode 100644
> > > index 000000000000..085bff04f5d9
> > > --- /dev/null
> > > +++ b/drivers/iio/proximity/vl53l1x-i2c.c  
> ...
> > > +
> > > +static int vl53l1x_probe(struct i2c_client *client)
> > > +{
> > > +     struct device *dev = &client->dev;
> > > +     struct vl53l1x_data *data;
> > > +     struct iio_dev *indio_dev;
> > > +     int ret;
> > > +
> > > +     if (!i2c_check_functionality(client->adapter,
> > > +                                  I2C_FUNC_SMBUS_READ_I2C_BLOCK |
> > > +                                  I2C_FUNC_SMBUS_BYTE_DATA))  
> > Sashiko had an interesting comment on this...
> > https://sashiko.dev/#/patchset/20260325063254.18062-1-email%40sirat.me
> > (note in general be careful with this tools feedback, it has a significant
> > false positive rate!)
> >
> > "Does this check needlessly reject pure I2C adapters?
> > The driver configures regmap with reg_bits = 16, which forces regmap to use
> > raw I2C transfers requiring I2C_FUNC_I2C. Checking for SMBUS_READ_I2C_BLOCK
> > might prevent the driver from loading on pure I2C adapters that can perfectly
> > support the device via regmap."
> >
> > By by reading the comment is actually wrong, however... It did made me look.
> > With regbits == 16 & valbits == 8
> > The regmap code checks for I2C_FUNC_SMBUS_I2C_BLOCK
> > (and if not present fails).
> >
> > So the I2C_FUNC_SMBUS_BYTE_DATA seems unused and
> > a broader check on both read and write versions of I2C_BLOCK seems
> > appropriate.
> >
> > However, not a lot of point in checking it at all given regmap does
> > so for us. So I'd drop this check.
> >
> > If anyone is bored, we should probably take a look to see if there
> > are other redundant (or wrong) calls for this.
> >
> > There is another point about trigger references that might be
> > correct (but not unique to this driver so ignore it).  That warrants
> > some investigation - I'll take a look in near future.
> >  
> I will drop the check altogether here then.
> ...
> > > +     /*
> > > +      * XSHUT held low puts the chip in hardware standby. All register
> > > +      * state is lost on de-assert so this is functionally a reset.
> > > +      */
> > > +     data->xshut_reset = devm_reset_control_get_optional_exclusive(dev, NULL);  
> >
> > If you can switch to devm_regulator_get_enabled() this can I think be
> >         ret = devm_reset_control_get_optional_exclusive_deasserted()
> > removing the need to register the action to power off.
> >
> > If you do that remember we still need the sleep after this call.
> >  
> So I will move the sleep after
> devm_reset_control_get_optional_exclusive_deasserted() then.
> >
> >  
> Thanks for the detailed review, Jonathan!
> 
> A quick question. Since these are some small  fixes, mostly removals,
> and the patch has seen quite some revisions, does the 1 week wait rule
> still apply here? I am still getting used to this mailing workflow
> coming from the PR workflow of Github and it will help me in the
> future.

Find to send now. Particularly given my recent email saying
cut off for this cycle is lunch time tomorrow (due to my travel).

If not for the dt-binding which is a little more complex change, I might
otherwise have just taken this version and tweaked it whilst applying.

Thanks

Jonathan



> 
> Thanks
> Sirat