Re: [PATCH 4/4] iio: proximity: vl53l0x-i2c: Use raw I2C access and read full device ID

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 19 Jan 2026 18:19:58 +0100
Petr Hodina via B4 Relay <devnull+petr.hodina.protonmail.com@kernel.org> wrote:

> From: Petr Hodina <petr.hodina@protonmail.com>
> 
> Replace SMBus byte reads with raw I2C transfers when reading the device
> identification registers.
> 
> The VL53L0X exposes its model and revision as 16-bit registers, which are
> more accurately accessed using standard I2C send/receive operations.
> This also avoids depending on SMBus byte data support, which is not
> guaranteed on all I2C adapters.

Hmm. I thought it was emulated wherever possible.  See below for
request for a specific comment on what the subtle difference is.
I'm also wondering from this description if it actually works with
smbus but  isn't documented as doing so?
> 
> Read and log both model and revision IDs, and validate the model ID
> during probe to ensure the expected device is present.
> 
> Signed-off-by: Petr Hodina <petr.hodina@protonmail.com>
> ---
>  drivers/iio/proximity/vl53l0x-i2c.c | 45 +++++++++++++++++++++++++++++++------
>  1 file changed, 38 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/proximity/vl53l0x-i2c.c b/drivers/iio/proximity/vl53l0x-i2c.c
> index 6901ce7dd835..a2de4cc16a43 100644
> --- a/drivers/iio/proximity/vl53l0x-i2c.c
> +++ b/drivers/iio/proximity/vl53l0x-i2c.c
> @@ -320,11 +320,35 @@ static const struct iio_trigger_ops vl53l0x_trigger_ops = {
>  	.validate_device = iio_trigger_validate_own_device,
>  };
>  
> +
> +static int vl53l0x_read_word(struct i2c_client *client, u8 reg, u16 *val)
> +{
> +	int ret;
> +	u8 buf[2];
> +
> +	ret = i2c_master_send(client, &reg, 1);

Add a comment that a stop is needed. Otherwise this would
be i2c_smbus_read_word_swapped()

Sigh, seems ST liked to make everyone's life a little harder ;)


> +	if (ret < 0)
> +		return ret;
> +	if (ret != 1)
> +		return -EIO;
> +
> +	ret = i2c_master_recv(client, buf, 2);
> +	if (ret < 0)
> +		return ret;
> +	if (ret != 2)
> +		return -EIO;
> +
> +	*val = (buf[0] << 8) | buf[1];
> +
> +	return 0;
> +}
> +
>  static int vl53l0x_probe(struct i2c_client *client)
>  {
>  	struct vl53l0x_data *data;
>  	struct iio_dev *indio_dev;
>  	int ret;
> +	u16 model, rev;
>  
>  	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>  	if (!indio_dev)
> @@ -339,13 +363,6 @@ static int vl53l0x_probe(struct i2c_client *client)
>  				     I2C_FUNC_SMBUS_BYTE_DATA))
>  		return -EOPNOTSUPP;
>  
> -	ret = i2c_smbus_read_byte_data(data->client, VL_REG_IDENTIFICATION_MODEL_ID);
> -	if (ret < 0)
> -		return -EINVAL;
> -
> -	if (ret != VL53L0X_MODEL_ID_VAL)
> -		dev_info(&client->dev, "Unknown model id: 0x%x", ret);
> -
>  	data->vdd_supply = devm_regulator_get(&client->dev, "vdd");
>  	if (IS_ERR(data->vdd_supply))
>  		return dev_err_probe(&client->dev, PTR_ERR(data->vdd_supply),
> @@ -372,6 +389,20 @@ static int vl53l0x_probe(struct i2c_client *client)
>  	if (ret)
>  		return ret;
>  
> +	ret = vl53l0x_read_word(client, 0xC0, &model);
defines for the register addresses.
I'm curious why they got bigger in a patch that doesn't mention it in the
patch description.

> +	if (ret)
> +		return dev_err_probe(&client->dev, ret, "Failed to read model ID\n");
Given these long lines, it would be good to have a precursor patch
struct device *dev = &client->dev;
and use that throughout probe()

> +
> +	ret = vl53l0x_read_word(client, 0xC2, &rev);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret, "Failed to read revision ID\n");
> +
> +	dev_info(&client->dev, "VL53L0X model=0x%04x rev=0x%04x\n", model, rev);
> +
> +	if ((model >> 8) != VL53L0X_MODEL_ID_VAL)
> +		return dev_err_probe(&client->dev, -ENODEV,
> +			"Unexpected model ID: 0x%04x\n", model);
See comment on other patch. This breaks DT fallback compatibles and
the benefit they bring for new compatible parts being able to be run with 
old kernels.  Just warn and carry on with whatever the firmware told you this
was.

> +
>  	indio_dev->name = "vl53l0x";
>  	indio_dev->info = &vl53l0x_info;
>  	indio_dev->channels = vl53l0x_channels;
>