iio: st_sensors: switch to a threaded interrupt

commit 98ad8b41f58dff6b30713d7f09ae3834b8df7ded
("iio: st_sensors: verify interrupt event to status") caused
a regression when reading ST sensors from a HRTimer trigger
rather than the intrinsic interrupts: the HRTimer may
trigger faster than the sensor provides new values, and
as the check against new values available as a cause of
the interrupt trigger was done in the poll function,
this would bail out of the HRTimer interrupt with
IRQ_NONE.

So clearly we need to only check the new values available
from the proper interrupt handler and not from the poll
function, which should rather just read the raw values
from the registers, put them into the buffer and be happy.

To achieve this: switch the ST Sensors over to using a true
threaded interrupt handler.

In the interrupt thread, check if new values are available,
else yield to the (potential) next device on the same
interrupt line to check the registers. If the interrupt
was ours, proceed to poll the values.

Instead of relying on iio_trigger_generic_data_rdy_poll() as
a top half to wake up the thread that polls the sensor for
new data, have the thread call iio_trigger_poll_chained()
after determining that is is the proper source of the
interrupt. This is modelled on drivers/iio/accel/mma8452.c
which is already using a properly threaded interrupt handler.

In order to get the same precision in timestamps as
previously, where samples would be timestamped in the
poll function pf->timestamp when calling
iio_trigger_generic_data_rdy_poll() we introduce a
local timestamp in the sensor data, set it in the top half
(fastpath) of the interrupt handler and provide that to the
core when calling iio_push_to_buffers_with_timestamp().

Additionally: if the active scanmask is not set for the
sensor no IRQs should be enabled and we need to bail out
with IRQ_NONE. This can happen if spurious IRQs fire when
installing the threaded interrupt handler.

Tested with hard interrupt triggers on LIS331DL, then also
tested with hrtimers on the same sensor by creating a 75Hz
HRTimer and using it to poll the sensor.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
Cc: Giuseppe Barba <giuseppe.barba@st.com>
Cc: Denis Ciocca <denis.ciocca@st.com>
Reported-by: Crestez Dan Leonard <cdleonard@gmail.com>
Tested-by: Crestez Dan Leonard <cdleonard@gmail.com>
Tested-by: Jonathan Cameron <jic23@kernel.org>
Fixes: 97865fe41322 ("iio: st_sensors: verify interrupt event to status")
Signed-off-by: Jonathan Cameron <jic23@kernel.org>

diff --git a/drivers/iio/accel/st_accel_buffer.c b/drivers/iio/accel/st_accel_buffer.c
index a1e642ee13d6a4f8af5eb0f6a9c92bafe51460d9..7fddc137e91ed7942e91a42a70f19f6320cfc0df 100644 (file)
--- a/drivers/iio/accel/st_accel_buffer.c
+++ b/drivers/iio/accel/st_accel_buffer.c
@@ -91,7 +91,7 @@ static const struct iio_buffer_setup_ops st_accel_buffer_setup_ops = {
 
 int st_accel_allocate_ring(struct iio_dev *indio_dev)
 {
-       return iio_triggered_buffer_setup(indio_dev, &iio_pollfunc_store_time,
+       return iio_triggered_buffer_setup(indio_dev, NULL,
                &st_sensors_trigger_handler, &st_accel_buffer_setup_ops);
 }
 

diff --git a/drivers/iio/accel/st_accel_core.c b/drivers/iio/accel/st_accel_core.c
index dc73f2d85e6d102ef719557e4db2b08c68901ab5..4d95bfc4786cafc6de2fcc659950860596193877 100644 (file)
--- a/drivers/iio/accel/st_accel_core.c
+++ b/drivers/iio/accel/st_accel_core.c
@@ -741,6 +741,7 @@ static const struct iio_info accel_info = {
 static const struct iio_trigger_ops st_accel_trigger_ops = {
        .owner = THIS_MODULE,
        .set_trigger_state = ST_ACCEL_TRIGGER_SET_STATE,
+       .validate_device = st_sensors_validate_device,
 };
 #define ST_ACCEL_TRIGGER_OPS (&st_accel_trigger_ops)
 #else

diff --git a/drivers/iio/common/st_sensors/st_sensors_buffer.c b/drivers/iio/common/st_sensors/st_sensors_buffer.c
index c55898543a47d9a2db596e89f59c5416b19bb0b7..f1693dbebb8ac23359035786ef26b6607191058d 100644 (file)
--- a/drivers/iio/common/st_sensors/st_sensors_buffer.c
+++ b/drivers/iio/common/st_sensors/st_sensors_buffer.c
@@ -57,31 +57,20 @@ irqreturn_t st_sensors_trigger_handler(int irq, void *p)
        struct iio_poll_func *pf = p;
        struct iio_dev *indio_dev = pf->indio_dev;
        struct st_sensor_data *sdata = iio_priv(indio_dev);
+       s64 timestamp;
 
-       /* If we have a status register, check if this IRQ came from us */
-       if (sdata->sensor_settings->drdy_irq.addr_stat_drdy) {
-               u8 status;
-
-               len = sdata->tf->read_byte(&sdata->tb, sdata->dev,
-                          sdata->sensor_settings->drdy_irq.addr_stat_drdy,
-                          &status);
-               if (len < 0)
-                       dev_err(sdata->dev, "could not read channel status\n");
-
-               /*
-                * If this was not caused by any channels on this sensor,
-                * return IRQ_NONE
-                */
-               if (!(status & (u8)indio_dev->active_scan_mask[0]))
-                       return IRQ_NONE;
-       }
+       /* If we do timetamping here, do it before reading the values */
+       if (sdata->hw_irq_trigger)
+               timestamp = sdata->hw_timestamp;
+       else
+               timestamp = iio_get_time_ns();
 
        len = st_sensors_get_buffer_element(indio_dev, sdata->buffer_data);
        if (len < 0)
                goto st_sensors_get_buffer_element_error;
 
        iio_push_to_buffers_with_timestamp(indio_dev, sdata->buffer_data,
-               pf->timestamp);
+                                          timestamp);
 
 st_sensors_get_buffer_element_error:
        iio_trigger_notify_done(indio_dev->trig);

diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
index dffe006921694840c24356f44bd400299e33b8f3..928ee68fcc5fa1a9ab0238bdba687a57d8d249cf 100644 (file)
--- a/drivers/iio/common/st_sensors/st_sensors_core.c
+++ b/drivers/iio/common/st_sensors/st_sensors_core.c
@@ -424,6 +424,9 @@ int st_sensors_set_dataready_irq(struct iio_dev *indio_dev, bool enable)
        else
                drdy_mask = sdata->sensor_settings->drdy_irq.mask_int2;
 
+       /* Flag to the poll function that the hardware trigger is in use */
+       sdata->hw_irq_trigger = enable;
+
        /* Enable/Disable the interrupt generator for data ready. */
        err = st_sensors_write_data_with_mask(indio_dev,
                                        sdata->sensor_settings->drdy_irq.addr,

diff --git a/drivers/iio/common/st_sensors/st_sensors_trigger.c b/drivers/iio/common/st_sensors/st_sensors_trigger.c
index da72279fcf99cbd6eac0f48c8965837566a8cdc1..1f59bcc0f143b96c505769ae9b37d63305955ea4 100644 (file)
--- a/drivers/iio/common/st_sensors/st_sensors_trigger.c
+++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c
@@ -17,6 +17,73 @@
 #include <linux/iio/common/st_sensors.h>
 #include "st_sensors_core.h"
 
+/**
+ * st_sensors_irq_handler() - top half of the IRQ-based triggers
+ * @irq: irq number
+ * @p: private handler data
+ */
+irqreturn_t st_sensors_irq_handler(int irq, void *p)
+{
+       struct iio_trigger *trig = p;
+       struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+       struct st_sensor_data *sdata = iio_priv(indio_dev);
+
+       /* Get the time stamp as close in time as possible */
+       sdata->hw_timestamp = iio_get_time_ns();
+       return IRQ_WAKE_THREAD;
+}
+
+/**
+ * st_sensors_irq_thread() - bottom half of the IRQ-based triggers
+ * @irq: irq number
+ * @p: private handler data
+ */
+irqreturn_t st_sensors_irq_thread(int irq, void *p)
+{
+       struct iio_trigger *trig = p;
+       struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+       struct st_sensor_data *sdata = iio_priv(indio_dev);
+       int ret;
+
+       /*
+        * If this trigger is backed by a hardware interrupt and we have a
+        * status register, check if this IRQ came from us
+        */
+       if (sdata->sensor_settings->drdy_irq.addr_stat_drdy) {
+               u8 status;
+
+               ret = sdata->tf->read_byte(&sdata->tb, sdata->dev,
+                          sdata->sensor_settings->drdy_irq.addr_stat_drdy,
+                          &status);
+               if (ret < 0) {
+                       dev_err(sdata->dev, "could not read channel status\n");
+                       goto out_poll;
+               }
+               /*
+                * the lower bits of .active_scan_mask[0] is directly mapped
+                * to the channels on the sensor: either bit 0 for
+                * one-dimensional sensors, or e.g. x,y,z for accelerometers,
+                * gyroscopes or magnetometers. No sensor use more than 3
+                * channels, so cut the other status bits here.
+                */
+               status &= 0x07;
+
+               /*
+                * If this was not caused by any channels on this sensor,
+                * return IRQ_NONE
+                */
+               if (!indio_dev->active_scan_mask)
+                       return IRQ_NONE;
+               if (!(status & (u8)indio_dev->active_scan_mask[0]))
+                       return IRQ_NONE;
+       }
+
+out_poll:
+       /* It's our IRQ: proceed to handle the register polling */
+       iio_trigger_poll_chained(p);
+       return IRQ_HANDLED;
+}
+
 int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
                                const struct iio_trigger_ops *trigger_ops)
 {
@@ -77,9 +144,12 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
            sdata->sensor_settings->drdy_irq.addr_stat_drdy)
                irq_trig |= IRQF_SHARED;
 
-       err = request_threaded_irq(irq,
-                       iio_trigger_generic_data_rdy_poll,
-                       NULL,
+       /* Let's create an interrupt thread masking the hard IRQ here */
+       irq_trig |= IRQF_ONESHOT;
+
+       err = request_threaded_irq(sdata->get_irq_data_ready(indio_dev),
+                       st_sensors_irq_handler,
+                       st_sensors_irq_thread,
                        irq_trig,
                        sdata->trig->name,
                        sdata->trig);
@@ -119,6 +189,18 @@ void st_sensors_deallocate_trigger(struct iio_dev *indio_dev)
 }
 EXPORT_SYMBOL(st_sensors_deallocate_trigger);
 
+int st_sensors_validate_device(struct iio_trigger *trig,
+                              struct iio_dev *indio_dev)
+{
+       struct iio_dev *indio = iio_trigger_get_drvdata(trig);
+
+       if (indio != indio_dev)
+               return -EINVAL;
+
+       return 0;
+}
+EXPORT_SYMBOL(st_sensors_validate_device);
+
 MODULE_AUTHOR("Denis Ciocca <denis.ciocca@st.com>");
 MODULE_DESCRIPTION("STMicroelectronics ST-sensors trigger");
 MODULE_LICENSE("GPL v2");

diff --git a/drivers/iio/gyro/st_gyro_buffer.c b/drivers/iio/gyro/st_gyro_buffer.c
index d67b17b6a7aab1abd0077e297c5e0f9769eee893..a5377044e42f0ccf128f1fd9bfb0ce02bba0a0a8 100644 (file)
--- a/drivers/iio/gyro/st_gyro_buffer.c
+++ b/drivers/iio/gyro/st_gyro_buffer.c
@@ -91,7 +91,7 @@ static const struct iio_buffer_setup_ops st_gyro_buffer_setup_ops = {
 
 int st_gyro_allocate_ring(struct iio_dev *indio_dev)
 {
-       return iio_triggered_buffer_setup(indio_dev, &iio_pollfunc_store_time,
+       return iio_triggered_buffer_setup(indio_dev, NULL,
                &st_sensors_trigger_handler, &st_gyro_buffer_setup_ops);
 }
 

diff --git a/drivers/iio/gyro/st_gyro_core.c b/drivers/iio/gyro/st_gyro_core.c
index 52a3c87c375ca62c027f66aa3de89d2c01c6fae7..a8012955a1f6d47130f703ced0df6a5e6c424407 100644 (file)
--- a/drivers/iio/gyro/st_gyro_core.c
+++ b/drivers/iio/gyro/st_gyro_core.c
@@ -409,6 +409,7 @@ static const struct iio_info gyro_info = {
 static const struct iio_trigger_ops st_gyro_trigger_ops = {
        .owner = THIS_MODULE,
        .set_trigger_state = ST_GYRO_TRIGGER_SET_STATE,
+       .validate_device = st_sensors_validate_device,
 };
 #define ST_GYRO_TRIGGER_OPS (&st_gyro_trigger_ops)
 #else

diff --git a/drivers/iio/magnetometer/st_magn_buffer.c b/drivers/iio/magnetometer/st_magn_buffer.c
index ecd3bd0a97693420c13bfae8fc583ff74854edf8..0a9e8fadfa9de8a66dd3d1fee8d82f81cc6b10a4 100644 (file)
--- a/drivers/iio/magnetometer/st_magn_buffer.c
+++ b/drivers/iio/magnetometer/st_magn_buffer.c
@@ -82,7 +82,7 @@ static const struct iio_buffer_setup_ops st_magn_buffer_setup_ops = {
 
 int st_magn_allocate_ring(struct iio_dev *indio_dev)
 {
-       return iio_triggered_buffer_setup(indio_dev, &iio_pollfunc_store_time,
+       return iio_triggered_buffer_setup(indio_dev, NULL,
                &st_sensors_trigger_handler, &st_magn_buffer_setup_ops);
 }
 

diff --git a/drivers/iio/magnetometer/st_magn_core.c b/drivers/iio/magnetometer/st_magn_core.c
index 62036d2a9956aa426cdde5ce71906e4e82663510..8250fc322c56754aa23ea4c877ff3414b1f58d81 100644 (file)
--- a/drivers/iio/magnetometer/st_magn_core.c
+++ b/drivers/iio/magnetometer/st_magn_core.c
@@ -572,6 +572,7 @@ static const struct iio_info magn_info = {
 static const struct iio_trigger_ops st_magn_trigger_ops = {
        .owner = THIS_MODULE,
        .set_trigger_state = ST_MAGN_TRIGGER_SET_STATE,
+       .validate_device = st_sensors_validate_device,
 };
 #define ST_MAGN_TRIGGER_OPS (&st_magn_trigger_ops)
 #else

diff --git a/drivers/iio/pressure/st_pressure_buffer.c b/drivers/iio/pressure/st_pressure_buffer.c
index 2ff53f222352ca66d4057ff054732e6a0a581db6..99468d0a64e74ce3f5ab0701f6d38e9f77c8c49c 100644 (file)
--- a/drivers/iio/pressure/st_pressure_buffer.c
+++ b/drivers/iio/pressure/st_pressure_buffer.c
@@ -82,7 +82,7 @@ static const struct iio_buffer_setup_ops st_press_buffer_setup_ops = {
 
 int st_press_allocate_ring(struct iio_dev *indio_dev)
 {
-       return iio_triggered_buffer_setup(indio_dev, &iio_pollfunc_store_time,
+       return iio_triggered_buffer_setup(indio_dev, NULL,
                &st_sensors_trigger_handler, &st_press_buffer_setup_ops);
 }
 

diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
index 257b58ac67791279ff0c440591bf4908cb7decb8..92a118c3c4acf3bc1529c4350dde5154b41c4abb 100644 (file)
--- a/drivers/iio/pressure/st_pressure_core.c
+++ b/drivers/iio/pressure/st_pressure_core.c
@@ -445,6 +445,7 @@ static const struct iio_info press_info = {
 static const struct iio_trigger_ops st_press_trigger_ops = {
        .owner = THIS_MODULE,
        .set_trigger_state = ST_PRESS_TRIGGER_SET_STATE,
+       .validate_device = st_sensors_validate_device,
 };
 #define ST_PRESS_TRIGGER_OPS (&st_press_trigger_ops)
 #else

diff --git a/include/linux/iio/common/st_sensors.h b/include/linux/iio/common/st_sensors.h
index d029ffac0d691ab51065cd960ab53a926e9bbf39..99403b19092f2da0ecadaffd21e9fc8065539434 100644 (file)
--- a/include/linux/iio/common/st_sensors.h
+++ b/include/linux/iio/common/st_sensors.h
@@ -223,6 +223,8 @@ struct st_sensor_settings {
  * @get_irq_data_ready: Function to get the IRQ used for data ready signal.
  * @tf: Transfer function structure used by I/O operations.
  * @tb: Transfer buffers and mutex used by I/O operations.
+ * @hw_irq_trigger: if we're using the hardware interrupt on the sensor.
+ * @hw_timestamp: Latest timestamp from the interrupt handler, when in use.
  */
 struct st_sensor_data {
        struct device *dev;
@@ -247,6 +249,9 @@ struct st_sensor_data {
 
        const struct st_sensor_transfer_function *tf;
        struct st_sensor_transfer_buffer tb;
+
+       bool hw_irq_trigger;
+       s64 hw_timestamp;
 };
 
 #ifdef CONFIG_IIO_BUFFER
@@ -260,7 +265,8 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
                                const struct iio_trigger_ops *trigger_ops);
 
 void st_sensors_deallocate_trigger(struct iio_dev *indio_dev);
-
+int st_sensors_validate_device(struct iio_trigger *trig,
+                              struct iio_dev *indio_dev);
 #else
 static inline int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
                                const struct iio_trigger_ops *trigger_ops)
@@ -271,6 +277,7 @@ static inline void st_sensors_deallocate_trigger(struct iio_dev *indio_dev)
 {
        return;
 }
+#define st_sensors_validate_device NULL
 #endif
 
 int st_sensors_init_sensor(struct iio_dev *indio_dev,