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@gmail.com>
---
drivers/iio/light/vl6180.c | 64 +++++++++++++++++++++++++++++++-------
1 file changed, 52 insertions(+), 12 deletions(-)
diff --git a/drivers/iio/light/vl6180.c b/drivers/iio/light/vl6180.c
index 52a837644..4c2b486e2 100644
--- a/drivers/iio/light/vl6180.c
+++ b/drivers/iio/light/vl6180.c
@@ -33,6 +33,7 @@
#define VL6180_MODEL_ID_VAL 0xb4
/* Configuration registers */
+#define VL6180_MODE_GPIO1 0x011
#define VL6180_INTR_CONFIG 0x014
#define VL6180_INTR_CLEAR 0x015
#define VL6180_OUT_OF_RESET 0x016
@@ -70,6 +71,9 @@
/* bits of the HOLD register */
#define VL6180_HOLD_ON BIT(0)
+/* bit for enabling GPIO1 Interrupt Output */
+#define VL6180_GPIO1_INTR_OUT BIT(4)
+
/* default value for the ALS_IT register */
#define VL6180_ALS_IT_100 0x63 /* 100 ms */
@@ -86,6 +90,7 @@
struct vl6180_data {
struct i2c_client *client;
struct mutex lock;
+ struct completion completion;
unsigned int als_gain_milli;
unsigned int als_it_ms;
unsigned int als_meas_rate;
@@ -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);
+ time_left = wait_for_completion_timeout(&data->completion, 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 irqreturn_t vl6180_threaded_irq(int irq, void *priv)
+{
+ struct iio_dev *indio_dev = priv;
+ struct vl6180_data *data = iio_priv(indio_dev);
+
+ complete(&data->completion);
+ return IRQ_HANDLED;
+}
+
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);
+ 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;
+ }
+
+ init_completion(&data->completion);
+ }
+
return devm_iio_device_register(&client->dev, indio_dev);
}
--
2.43.0On Fri, 4 Oct 2024 20:31:47 +0530
Abhash Jha <abhashkumarjha123@gmail.com> 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@gmail.com>
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);
> }
>
> > + 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.
Yes this makes sense.
> This reinit needs to be before whatever can potentially trigger
> that interrupt.
Can you explain this part, because where can i reinit it, The measurement
starts when we write the START_STOP bit to SYSRANGE_START. So should
it be before that.
I'm kind of confused with this.
Thank you,
Abhash
On Sat, 5 Oct 2024 23:05:25 +0530
Abhash jha <abhashkumarjha123@gmail.com> wrote:
> > > + 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.
> Yes this makes sense.
>
> > This reinit needs to be before whatever can potentially trigger
> > that interrupt.
> Can you explain this part, because where can i reinit it, The measurement
> starts when we write the START_STOP bit to SYSRANGE_START. So should
> it be before that.
Immediately above that write looks right to me. So before writing START_STOP.
That way the completion will be ready for a complete whenever
one turns up.
Jonathan
> I'm kind of confused with this.
>
> Thank you,
> Abhash
© 2016 - 2024 Red Hat, Inc.