Re: [PATCH v2] iio: proximity: cleanup fixes for vl53l1x-i2c

[Jonathan Cameron]

On Mon, 08 Jun 2026 16:30:07 +0100
Nuno Sá <noname.nuno@xxxxxxxxx> wrote:

> On Mon, 2026-06-08 at 18:40 +0600, Siratul Islam wrote:
> > Extract data-ready polling into a helper, fix regmap_read_poll_timeout()
> > argument alignment. No functional changes.
> > 
> > Suggested-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
> > Signed-off-by: Siratul Islam <email@xxxxxxxx>
> > ---  
> 
> Normally don't really like these one liner helper APIs but for this cases where it's easy to mess
> up, it makes sense as if __we mess up__, we only need to fix one place.
> 
> Just one comment from me. With that addressed, looks good:
> 
> Reviewed-by: Nuno Sá <nuno.sa@xxxxxxxxxx>
> 
The changes are fine, but there are really 3 different ones in here - so
at least 2 patches (see below for why not 3).

Jonathan

> >  drivers/iio/proximity/vl53l1x-i2c.c | 36 ++++++++++++++---------------
> >  1 file changed, 17 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/iio/proximity/vl53l1x-i2c.c b/drivers/iio/proximity/vl53l1x-i2c.c
> > index 4d9cb3983dba..ed83999cf735 100644
> > --- a/drivers/iio/proximity/vl53l1x-i2c.c
> > +++ b/drivers/iio/proximity/vl53l1x-i2c.c
> > @@ -43,6 +43,7 @@
> >  #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_DEFAULT_CONFIG					0x002D
> >  #define VL53L1X_REG_GPIO_HV_MUX__CTRL					0x0030
> >  #define VL53L1X_REG_GPIO__TIO_HV_STATUS					0x0031
> >  #define VL53L1X_REG_SYSTEM__INTERRUPT_CONFIG_GPIO			0x0046
> > @@ -64,7 +65,6 @@
> >  #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

There are too many things in one patch. This move is unrelated to the rest
so needs to be on its own.

> >  
> >  #define VL53L1X_MODEL_ID_VAL		0xEACC
> >  
> > @@ -191,6 +191,17 @@ static int vl53l1x_stop_ranging(struct vl53l1x_data *data)
> >  			    VL53L1X_MODE_START_STOP);
> >  }
> >  
> > +static int vl53l1x_wait_data_ready(struct vl53l1x_data *data)
> > +{
> > +	unsigned int val;
> > +
> > +	/* 1ms poll, 1s timeout covers max timing budgets (per ST Ultra Lite Driver) */
> > +	return 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);
> > +}
> > +
> >  /*
> >   * Default configuration blob from ST's VL53L1X Ultra Lite Driver
> >   * (STSW-IMG009).
> > @@ -230,10 +241,9 @@ static int vl53l1x_chip_init(struct vl53l1x_data *data)
> >  	}
> >  
> >  	ret = regmap_read_poll_timeout(data->regmap,
> > -				       VL53L1X_REG_FIRMWARE__SYSTEM_STATUS, val,
> > -				       val & BIT(0),
> > -				       1 * USEC_PER_MSEC,
> > -				       100 * USEC_PER_MSEC);
> > +				       VL53L1X_REG_FIRMWARE__SYSTEM_STATUS,
> > +				       val, val & BIT(0),
This one is using BIT(0) and above you are using the value of that (so 1).
I slightly prefer BIT(0) as I assume it is a single bit field.  Even better
would be to use proper field definitions.  So something like
#define VL53L1X_FIRMWARE__SYSTEM_STATUS_BOOTED BIT(0)

> > +				       1 * USEC_PER_MSEC, 100 * USEC_PER_MSEC);  
> 
> This looks unrelated to what you have on the commit message. Either drop it or mention it in the
> message (but I'm really not sure on the added value of the above change - I guess more meaningful
> line break wrt the API arguments).

Separate patch. I don't mind all the reformatting together (so the register definition
movement).  However if you follow the suggestion to use a define to name the BIT(0)
field that can be combined with reflowing this, but not the move above.

> 
> - Nuno Sá
> 
> >  	if (ret)
> >  		return dev_err_probe(dev, ret, "firmware boot timeout\n");
> >  
> > @@ -261,12 +271,7 @@ static int vl53l1x_chip_init(struct vl53l1x_data *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);
> > +	ret = vl53l1x_wait_data_ready(data);
> >  	if (ret)
> >  		return ret;
> >  
> > @@ -461,14 +466,7 @@ static int vl53l1x_read_proximity(struct vl53l1x_data *data, int *val)
> >  		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);
> > +		ret = vl53l1x_wait_data_ready(data);
> >  		if (ret)
> >  			return ret;
> >  	}  
>