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

[Nuno Sá]

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>

>  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
>  
>  #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),
> +				       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).

- 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;
>  	}