Re: [PATCH 2/3] iio: light: vl6180: Added Interrupt support for single shot access

[Jonathan Cameron]

On Fri,  4 Oct 2024 20:31:47 +0530
Abhash Jha <abhashkumarjha123@xxxxxxxxx> wrote:

> Configured the GPIO1 pin to provide output interrupts. And then the
> interrupts are serviced in the `vl6180_measure` function when the
> irq_handler signals that the reading is complete.
> 
> Signed-off-by: Abhash Jha <abhashkumarjha123@xxxxxxxxx>

A few comments inline.

Thanks,

Jonathan

> ---

> @@ -211,6 +216,7 @@ static int vl6180_write_word(struct i2c_client *client, u16 cmd, u16 val)
>  static int vl6180_measure(struct vl6180_data *data, int addr)
>  {
>  	struct i2c_client *client = data->client;
> +	unsigned long time_left;
>  	int tries = 20, ret;
>  	u16 value;
>  
> @@ -221,19 +227,26 @@ static int vl6180_measure(struct vl6180_data *data, int addr)
>  	if (ret < 0)
>  		goto fail;
>  
> -	while (tries--) {
> -		ret = vl6180_read_byte(client, VL6180_INTR_STATUS);
> -		if (ret < 0)
> -			goto fail;
> -
> -		if (ret & vl6180_chan_regs_table[addr].drdy_mask)
> -			break;
> -		msleep(20);
> -	}
> +	if (client->irq) {
> +		reinit_completion(&data->completion);

That's late so there is a race condition. You might be delayed just before this
and finish the measurement before the reint_completion() in which case you'll
clear the complete() that happens in the interrupt handler before
then waiting on it.  This reinit needs to be before whatever can potentially trigger
that interrupt.

> +		time_left = wait_for_completion_timeout(&data->completion, HZ/10);
HZ / 10
> +		if (time_left == 0)
> +			return -ETIMEDOUT;
> +	} else {
> +		while (tries--) {
> +			ret = vl6180_read_byte(client, VL6180_INTR_STATUS);
> +			if (ret < 0)
> +				goto fail;
> +
> +			if (ret & vl6180_chan_regs_table[addr].drdy_mask)
> +				break;
> +			msleep(20);
> +		}
>  
> -	if (tries < 0) {
> -		ret = -EIO;
> -		goto fail;
> +		if (tries < 0) {
> +			ret = -EIO;
> +			goto fail;
> +		}
>  	}
>  
>  	/* Read result value from appropriate registers */
> @@ -479,6 +492,15 @@ static int vl6180_write_raw(struct iio_dev *indio_dev,
>  	}
>  }

...

>  static const struct iio_info vl6180_info = {
>  	.read_raw = vl6180_read_raw,
>  	.write_raw = vl6180_write_raw,
> @@ -514,6 +536,11 @@ static int vl6180_init(struct vl6180_data *data)
>  	if (ret != 0x01)
>  		dev_info(&client->dev, "device is not fresh out of reset\n");
>  
> +	ret = vl6180_write_byte(client, VL6180_MODE_GPIO1,
> +				VL6180_GPIO1_INTR_OUT);

I would only do this if the interrupt is wired. It's probably harmless otherwise
but seems a bit odd if that pin isn't connected to anything.

> +	if (ret < 0)
> +		return ret;
> +
>  	/* Enable ALS and Range ready interrupts */
>  	ret = vl6180_write_byte(client, VL6180_INTR_CONFIG,
>  				VL6180_ALS_READY | VL6180_RANGE_READY);
> @@ -580,6 +607,19 @@ static int vl6180_probe(struct i2c_client *client)
>  	if (ret < 0)
>  		return ret;
>  
> +	if (client->irq) {
> +		ret = devm_request_threaded_irq(&client->dev, client->irq,
> +						NULL, vl6180_threaded_irq,
> +						IRQF_ONESHOT,
> +						indio_dev->name, indio_dev);
> +		if (ret) {
> +			dev_err(&client->dev, "devm_request_irq error: %d\n", ret);
> +			return ret;

return dev_err_probe()

> +		}
> +
> +		init_completion(&data->completion);
> +	}
> +
>  	return devm_iio_device_register(&client->dev, indio_dev);
>  }
>