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

[Sirat]

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.

Thanks
Sirat