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