Leonard Crestez observed the following phenomenon: when using
hard interrupt triggers (the DRDY line coming out of an ST
sensor) sometimes a new value would arrive while reading the
previous value, due to latencies in the system.
We discovered that the ST hardware as far as can be observed
is designed for level interrupts: the DRDY line will be held
asserted as long as there are new values coming. The interrupt
handler should be re-entered until we're out of values to
handle from the sensor.
If interrupts were handled as occurring on the edges (usually
low-to-high) new values could appear and the line be held
asserted after that, and these values would be missed, the
interrupt handler would also lock up as new data was
available, but as no new edges occurs on the DRDY signal,
nothing happens: the edge detector only detects edges.
To counter this, do the following:
- Accept interrupt lines to be flagged as level interrupts
using IRQF_TRIGGER_HIGH and IRQF_TRIGGER_LOW. If the line
is marked like this (in the device tree node or ACPI
table or similar) it will be utilized as a level IRQ.
We mark the line with IRQF_ONESHOT and mask the IRQ
while processing a sample, then the top half will be
entered again if new values are available.
- If no interrupt type is indicated from the DT/ACPI,
choose IRQF_TRIGGER_HIGH so the above goes into action.
- If we are flagged as using edge interrupts with
IRQF_TRIGGER_RISING or IRQF_TRIGGER_FALLING: remove
IRQF_ONESHOT so that the interrupt line is not
masked while running the thread part of the interrupt.
This way we will never miss an interrupt, then introduce
a loop that polls the data ready registers repeatedly
until no new samples are available, then exit the
interrupt handler. This way we know no new values are
available when the interrupt handler exits and
new (edge) interrupts will be triggered when data arrives.
Take some extra care to update the timestamp in the poll
loop if this happens. The timestamp will not be 100%
perfect, but it will at least be closer to the actual
events. Usually the extra poll loop will handle the new
samples, but once in a blue moon, we get a new IRQ
while exiting the loop, before returning from the
thread IRQ bottom half with IRQ_HANDLED. On these rare
occasions, the removal of IRQF_ONESHOT means the
interrupt will immediately fire again.
Tested successfully on the LIS331DL and L3G4200D by setting
sampling frequency to 400Hz/800Hz and stressing the system:
extra reads in the threaded interrupt handler occurs.
Cc: Giuseppe Barba <giuseppe.barba@xxxxxx>
Cc: Denis Ciocca <denis.ciocca@xxxxxx>
Cc: Crestez Dan Leonard <cdleonard@xxxxxxxxx>
Reported-by: Crestez Dan Leonard <cdleonard@xxxxxxxxx>
Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
---
ChangeLog v6->v7:
- Incorporate Leonards handling of level interrupts into
the code. If we have level IRQs: use them.
- Default to level interrupt handling.
- If we only have edge interrupts: enable the polling loop.
- Leonard it would be awesome if you could test this patch,
maybe both with level and edge irqs on your system? I
could only test the edges.
ChangeLog v5->v6:
- Add a loop counter to the threaded value poll function: let's
just loop here for at maximum 10 loops before we exit and
let the thread re-trigger if more interrupts arrived.
ChangeLog v4->v5:
- Remove the IRQF_ONESHOT flag from the interrupt: this makes
it possible to trigger the top half of the interrupt even
though the bottom half is processing an earlier interrupt.
This makes it possible to gracefully handle interrupts that
come in during sensor reading.
- Add better descriptive comments.
ChangeLog v3->v4:
- Include this patch with the threaded interrupt fix patch.
Adapte the same patch numbering system.
If this works I suggest it be applied to mainline as a fix on
top of the threading patch version 3, so we handle this annoying
lockup bug.
---
drivers/iio/common/st_sensors/st_sensors_buffer.c | 7 +-
drivers/iio/common/st_sensors/st_sensors_trigger.c | 157 +++++++++++++++------
include/linux/iio/common/st_sensors.h | 2 +
3 files changed, 118 insertions(+), 48 deletions(-)
diff --git a/drivers/iio/common/st_sensors/st_sensors_buffer.c b/drivers/iio/common/st_sensors/st_sensors_buffer.c
index f1693dbebb8a..f6873919f188 100644
--- a/drivers/iio/common/st_sensors/st_sensors_buffer.c
+++ b/drivers/iio/common/st_sensors/st_sensors_buffer.c
@@ -59,7 +59,12 @@ irqreturn_t st_sensors_trigger_handler(int irq, void *p)
struct st_sensor_data *sdata = iio_priv(indio_dev);
s64 timestamp;
- /* If we do timetamping here, do it before reading the values */
+ /*
+ * If we do timetamping here, do it before reading the values, because
+ * once we've read the values, new interrupts can occur (when using
+ * the hardware trigger) and the hw_timestamp may get updated.
+ * By storing it in a local variable first, we are safe.
+ */
if (sdata->hw_irq_trigger)
timestamp = sdata->hw_timestamp;
else
diff --git a/drivers/iio/common/st_sensors/st_sensors_trigger.c b/drivers/iio/common/st_sensors/st_sensors_trigger.c
index 296e4ff19ae8..fd77ea147113 100644
--- a/drivers/iio/common/st_sensors/st_sensors_trigger.c
+++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c
@@ -18,6 +18,50 @@
#include "st_sensors_core.h"
/**
+ * st_sensors_new_samples_available() - check if more samples came in
+ * returns:
+ * 0 - no new samples available
+ * 1 - new samples available
+ * negative - error or unknown
+ */
+static int st_sensors_new_samples_available(struct iio_dev *indio_dev,
+ struct st_sensor_data *sdata)
+{
+ u8 status;
+ int ret;
+
+ /* How would I know if I can't check it? */
+ if (!sdata->sensor_settings->drdy_irq.addr_stat_drdy)
+ return -EINVAL;
+
+ /* No scan mask, no interrupt */
+ if (!indio_dev->active_scan_mask)
+ return 0;
+
+ 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,
+ "error checking samples available\n");
+ return ret;
+ }
+ /*
+ * 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 (status & (u8)indio_dev->active_scan_mask[0])
+ return 1;
+
+ return 0;
+}
+
+/**
* st_sensors_irq_handler() - top half of the IRQ-based triggers
* @irq: irq number
* @p: private handler data
@@ -43,44 +87,42 @@ 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;
+ /* Loop for new values maximum 10 times */
+ int loops = 10;
/*
* If this trigger is backed by a hardware interrupt and we have a
- * status register, check if this IRQ came from us
+ * status register, check if this IRQ came from us. Notice that
+ * we will process also if st_sensors_new_samples_available()
+ * returns negative: if we can't check status, then poll
+ * unconditionally.
*/
- 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 (st_sensors_new_samples_available(indio_dev, sdata)) {
+ iio_trigger_poll_chained(p);
+ } else {
+ dev_dbg(sdata->dev, "spurious IRQ\n");
+ return IRQ_NONE;
+ }
- /*
- * 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;
+ /*
+ * If we have proper level IRQs the handler will be re-entered if
+ * the line is still active, so return here and come back in through
+ * the top half if need be.
+ */
+ if (!sdata->edge_irq)
+ return IRQ_HANDLED;
+
+ /*
+ * If we are using egde IRQs, new samples arrived while processing
+ * the IRQ and those may be missed unless we pick them here, so poll
+ * again.
+ */
+ while (loops-- && st_sensors_new_samples_available(indio_dev, sdata)) {
+ dev_dbg(sdata->dev, "more samples came in during polling\n");
+ sdata->hw_timestamp = iio_get_time_ns();
+ iio_trigger_poll_chained(p);
}
-out_poll:
- /* It's our IRQ: proceed to handle the register polling */
- iio_trigger_poll_chained(p);
return IRQ_HANDLED;
}
@@ -107,13 +149,18 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
* If the IRQ is triggered on falling edge, we need to mark the
* interrupt as active low, if the hardware supports this.
*/
- if (irq_trig == IRQF_TRIGGER_FALLING) {
+ switch(irq_trig) {
+ case IRQF_TRIGGER_FALLING:
+ case IRQF_TRIGGER_LOW:
if (!sdata->sensor_settings->drdy_irq.addr_ihl) {
dev_err(&indio_dev->dev,
- "falling edge specified for IRQ but hardware "
- "only support rising edge, will request "
- "rising edge\n");
- irq_trig = IRQF_TRIGGER_RISING;
+ "falling/low specified for IRQ "
+ "but hardware only support rising/high: "
+ "will request rising/high\n");
+ if (irq_trig == IRQF_TRIGGER_FALLING)
+ irq_trig = IRQF_TRIGGER_RISING;
+ if (irq_trig == IRQF_TRIGGER_LOW)
+ irq_trig = IRQF_TRIGGER_HIGH;
} else {
/* Set up INT active low i.e. falling edge */
err = st_sensors_write_data_with_mask(indio_dev,
@@ -122,20 +169,39 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
if (err < 0)
goto iio_trigger_free;
dev_info(&indio_dev->dev,
- "interrupts on the falling edge\n");
+ "interrupts on the falling edge or "
+ "active low level\n");
}
- } else if (irq_trig == IRQF_TRIGGER_RISING) {
+ break;
+ case IRQF_TRIGGER_RISING:
dev_info(&indio_dev->dev,
"interrupts on the rising edge\n");
-
- } else {
+ break;
+ case IRQF_TRIGGER_HIGH:
+ dev_info(&indio_dev->dev,
+ "interrupts active high level\n");
+ break;
+ default:
+ /* This is the most preferred mode, if possible */
dev_err(&indio_dev->dev,
- "unsupported IRQ trigger specified (%lx), only "
- "rising and falling edges supported, enforce "
- "rising edge\n", irq_trig);
- irq_trig = IRQF_TRIGGER_RISING;
+ "unsupported IRQ trigger specified (%lx), enforce "
+ "active high\n", irq_trig);
+ irq_trig = IRQF_TRIGGER_HIGH;
}
+ /* Tell the interrupt handler that we're dealing with edges */
+ if (irq_trig == IRQF_TRIGGER_FALLING ||
+ irq_trig == IRQF_TRIGGER_RISING)
+ sdata->edge_irq = true;
+ else
+ /*
+ * If we're not using edges (i.e. level interrupts) we
+ * just mask off the IRQ, handle one interrupt, then
+ * if the line is still low, we return to the
+ * interrupt handler top half again and start over.
+ */
+ irq_trig |= IRQF_ONESHOT;
+
/*
* If the interrupt pin is Open Drain, by definition this
* means that the interrupt line may be shared with other
@@ -148,9 +214,6 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
sdata->sensor_settings->drdy_irq.addr_stat_drdy)
irq_trig |= IRQF_SHARED;
- /* 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,
diff --git a/include/linux/iio/common/st_sensors.h b/include/linux/iio/common/st_sensors.h
index 99403b19092f..c9efe0f809e5 100644
--- a/include/linux/iio/common/st_sensors.h
+++ b/include/linux/iio/common/st_sensors.h
@@ -223,6 +223,7 @@ 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.
+ * @edge_irq: the IRQ triggers on edges and need special handling.
* @hw_irq_trigger: if we're using the hardware interrupt on the sensor.
* @hw_timestamp: Latest timestamp from the interrupt handler, when in use.
*/
@@ -250,6 +251,7 @@ struct st_sensor_data {
const struct st_sensor_transfer_function *tf;
struct st_sensor_transfer_buffer tb;
+ bool edge_irq;
bool hw_irq_trigger;
s64 hw_timestamp;
};
--
2.4.11
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
