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

[David Lechner]

On 3/3/26 3:02 AM, Siratul Islam wrote:
> Add support for the STMicroelectronics VL53L1X Time-of-Flight
> ranging sensor with I2C interface.
> 

...

> +static int vl53l1x_set_distance_mode(struct vl53l1x_data *data,
> +				     enum vl53l1x_distance_mode mode)
> +{
> +	int ret;
> +
> +	switch (mode) {
> +	case VL53L1X_SHORT:
> +		ret = regmap_write(data->regmap,
> +				   VL53L1X_PHASECAL_CONFIG__TIMEOUT_MACROP,
> +				   0x14);
> +		if (ret)
> +			return ret;
> +		ret = regmap_write(data->regmap,
> +				   VL53L1X_RANGE_CONFIG__VCSEL_PERIOD_A, 0x07);
> +		if (ret)
> +			return ret;
> +		ret = regmap_write(data->regmap,
> +				   VL53L1X_RANGE_CONFIG__VCSEL_PERIOD_B, 0x05);
> +		if (ret)
> +			return ret;
> +		ret = regmap_write(data->regmap,
> +				   VL53L1X_RANGE_CONFIG__VALID_PHASE_HIGH,
> +				   0x38);
> +		if (ret)
> +			return ret;
> +		ret = regmap_write(data->regmap, VL53L1X_SD_CONFIG__WOI_SD0,
> +				   0x07);
> +		if (ret)
> +			return ret;
> +		ret = regmap_write(data->regmap, VL53L1X_SD_CONFIG__WOI_SD1,
> +				   0x05);
> +		if (ret)
> +			return ret;
> +		ret = regmap_write(data->regmap,
> +				   VL53L1X_SD_CONFIG__INITIAL_PHASE_SD0, 6);
> +		if (ret)
> +			return ret;
> +		ret = regmap_write(data->regmap,
> +				   VL53L1X_SD_CONFIG__INITIAL_PHASE_SD1, 6);

Maybe worth using regmap_multi_reg_write() above and below?

> +		break;
> +	case VL53L1X_LONG:
> +		ret = regmap_write(data->regmap,
> +				   VL53L1X_PHASECAL_CONFIG__TIMEOUT_MACROP,
> +				   0x0A);
> +		if (ret)
> +			return ret;
> +		ret = regmap_write(data->regmap,
> +				   VL53L1X_RANGE_CONFIG__VCSEL_PERIOD_A, 0x0F);
> +		if (ret)
> +			return ret;
> +		ret = regmap_write(data->regmap,
> +				   VL53L1X_RANGE_CONFIG__VCSEL_PERIOD_B, 0x0D);
> +		if (ret)
> +			return ret;
> +		ret = regmap_write(data->regmap,
> +				   VL53L1X_RANGE_CONFIG__VALID_PHASE_HIGH,
> +				   0xB8);
> +		if (ret)
> +			return ret;
> +		ret = regmap_write(data->regmap, VL53L1X_SD_CONFIG__WOI_SD0,
> +				   0x0F);
> +		if (ret)
> +			return ret;
> +		ret = regmap_write(data->regmap, VL53L1X_SD_CONFIG__WOI_SD1,
> +				   0x0D);
> +		if (ret)
> +			return ret;
> +		ret = regmap_write(data->regmap,
> +				   VL53L1X_SD_CONFIG__INITIAL_PHASE_SD0, 14);
> +		if (ret)
> +			return ret;
> +		ret = regmap_write(data->regmap,
> +				   VL53L1X_SD_CONFIG__INITIAL_PHASE_SD1, 14);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (ret)
> +		return ret;
> +
> +	data->distance_mode = mode;
> +	return 0;
> +}
> +

...

> +	return ret;
> +}
> +
> +static const struct iio_chan_spec vl53l1x_channels[] = {
> +	{
> +		.type = IIO_DISTANCE,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +		.scan_index = 0,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 16,
> +			.storagebits = 16,
> +		},
> +	},
> +	IIO_CHAN_SOFT_TIMESTAMP(1),
> +};
> +
> +static int vl53l1x_read_raw(struct iio_dev *indio_dev,
> +			    const struct iio_chan_spec *chan, int *val,
> +			    int *val2, long mask)
> +{
> +	struct vl53l1x_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (chan->type != IIO_DISTANCE)
> +		return -EINVAL;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:

What is here is perfectly fine, but I'm sure someone will eventually
want to change it to IIO_DEV_ACQUIRE_DIRECT_MODE().

> +		if (!iio_device_claim_direct(indio_dev))
> +			return -EBUSY;
> +		ret = vl53l1x_read_proximity(data, val);
> +		iio_device_release_direct(indio_dev);
> +		if (ret)
> +			return ret;
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = 0;
> +		*val2 = 1000;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int vl53l1x_validate_trigger(struct iio_dev *indio_dev,
> +				    struct iio_trigger *trig)
> +{
> +	struct vl53l1x_data *data = iio_priv(indio_dev);
> +
> +	return data->trig == trig ? 0 : -EINVAL;
> +}
> +
> +static const struct iio_info vl53l1x_info = {
> +	.read_raw = vl53l1x_read_raw,
> +	.validate_trigger = vl53l1x_validate_trigger,
> +};
> +
> +static irqreturn_t vl53l1x_trigger_handler(int irq, void *priv)
> +{
> +	struct iio_poll_func *pf = priv;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct vl53l1x_data *data = iio_priv(indio_dev);
> +	struct {
> +		u16 distance;
> +		aligned_s64 timestamp;
> +	} scan = {};
> +	unsigned int range_status;
> +	int ret;
> +
> +	ret = regmap_read(data->regmap, VL53L1X_RESULT__RANGE_STATUS,
> +			  &range_status);
> +	if (ret || (range_status & VL53L1X_RANGE_STATUS_MASK) !=

Probably best to use FIELD_GET() here and redefine VL53L1X_RANGE_STATUS_VALID
accrodingly.

> +			   VL53L1X_RANGE_STATUS_VALID)
> +		goto done;
> +
> +	ret = vl53l1x_read_u16(data,
> +			       VL53L1X_RESULT__FINAL_CROSSTALK_CORRECTED_RANGE_MM_SD0,
> +				   &scan.distance);
> +	if (ret)
> +		goto done;
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, &scan,
> +					   iio_get_time_ns(indio_dev));
> +
> +done:
> +	iio_trigger_notify_done(indio_dev->trig);
> +	vl53l1x_clear_irq(data);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t vl53l1x_threaded_irq(int irq, void *priv)
> +{
> +	struct iio_dev *indio_dev = priv;
> +	struct vl53l1x_data *data = iio_priv(indio_dev);
> +
> +	if (iio_buffer_enabled(indio_dev))
> +		iio_trigger_poll_nested(indio_dev->trig);
> +	else
> +		complete(&data->completion);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int vl53l1x_configure_irq(struct i2c_client *client,
> +				 struct iio_dev *indio_dev)

Would be more logical to move this function closer to probe.

> +{
> +	struct vl53l1x_data *data = iio_priv(indio_dev);
> +	int irq_flags = irq_get_trigger_type(client->irq);
> +	int ret;
> +
> +	if (!irq_flags)
> +		irq_flags = IRQF_TRIGGER_FALLING;
> +
> +	ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> +					vl53l1x_threaded_irq,
> +					irq_flags | IRQF_ONESHOT,
> +					indio_dev->name, indio_dev);
> +	if (ret) {
> +		dev_err(&client->dev, "devm_request_irq error: %d\n", ret);


Can use return dev_err_probe() here since this is only called in probe.


> +		return ret;
> +	}
> +
> +	ret = regmap_write(data->regmap, VL53L1X_SYSTEM__INTERRUPT_CONFIG_GPIO,
> +			   VL53L1X_INT_NEW_SAMPLE_READY);
> +	if (ret)
> +		dev_err(&client->dev, "failed to configure IRQ: %d\n", ret);

ditto

> +
> +	return ret;
> +}
> +
> +static int vl53l1x_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct vl53l1x_data *data = iio_priv(indio_dev);
> +
> +	return vl53l1x_start_ranging(data);
> +}
> +
> +static int vl53l1x_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> +	struct vl53l1x_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = vl53l1x_stop_ranging(data);
> +	if (ret)
> +		return ret;
> +
> +	reinit_completion(&data->completion);
> +	wait_for_completion_timeout(&data->completion, HZ / 10);
> +
> +	return vl53l1x_clear_irq(data);
> +}
> +
> +static const struct iio_buffer_setup_ops vl53l1x_buffer_setup_ops = {
> +	.postenable = &vl53l1x_buffer_postenable,
> +	.postdisable = &vl53l1x_buffer_postdisable,
> +};

These are not symetric. It either needs to be postenable/predisable
or preenable/postdisable.