Evaluate the devicetree property for an optional interrupt line, and
configure the interrupt mapping accordingly. When no interrupt line
is defined in the devicetree, keep the FIFO in bypass mode as before.
Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
drivers/iio/accel/adxl313.h | 13 +++++++++
drivers/iio/accel/adxl313_core.c | 49 +++++++++++++++++++++++++++++++-
2 files changed, 61 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/accel/adxl313.h b/drivers/iio/accel/adxl313.h
index 9bf2facdbf87..ab6b9e11fde4 100644
--- a/drivers/iio/accel/adxl313.h
+++ b/drivers/iio/accel/adxl313.h
@@ -21,7 +21,9 @@
#define ADXL313_REG_ACT_INACT_CTL 0x27
#define ADXL313_REG_BW_RATE 0x2C
#define ADXL313_REG_POWER_CTL 0x2D
+#define ADXL313_REG_INT_ENABLE 0x2E
#define ADXL313_REG_INT_MAP 0x2F
+#define ADXL313_REG_INT_SOURCE 0x30
#define ADXL313_REG_DATA_FORMAT 0x31
#define ADXL313_REG_DATA_AXIS(index) (0x32 + ((index) * 2))
#define ADXL313_REG_FIFO_CTL 0x38
@@ -45,6 +47,17 @@
#define ADXL313_SPI_3WIRE BIT(6)
#define ADXL313_I2C_DISABLE BIT(6)
+#define ADXL313_INT_OVERRUN BIT(0)
+#define ADXL313_INT_WATERMARK BIT(1)
+#define ADXL313_INT_INACTIVITY BIT(3)
+#define ADXL313_INT_ACTIVITY BIT(4)
+#define ADXL313_INT_DREADY BIT(7)
+
+#define ADXL313_REG_FIFO_CTL_MODE_MSK GENMASK(7, 6)
+
+#define ADXL313_FIFO_BYPASS 0
+#define ADXL313_FIFO_STREAM 2
+
extern const struct regmap_access_table adxl312_readable_regs_table;
extern const struct regmap_access_table adxl313_readable_regs_table;
extern const struct regmap_access_table adxl314_readable_regs_table;
diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
index 6170c9daa30f..31ce1b218488 100644
--- a/drivers/iio/accel/adxl313_core.c
+++ b/drivers/iio/accel/adxl313_core.c
@@ -8,11 +8,17 @@
*/
#include <linux/bitfield.h>
+#include <linux/interrupt.h>
#include <linux/module.h>
+#include <linux/property.h>
#include <linux/regmap.h>
#include "adxl313.h"
+#define ADXL313_INT_NONE U8_MAX
+#define ADXL313_INT1 1
+#define ADXL313_INT2 2
+
static const struct regmap_range adxl312_readable_reg_range[] = {
regmap_reg_range(ADXL313_REG_DEVID0, ADXL313_REG_DEVID0),
regmap_reg_range(ADXL313_REG_OFS_AXIS(0), ADXL313_REG_OFS_AXIS(2)),
@@ -436,7 +442,9 @@ int adxl313_core_probe(struct device *dev,
{
struct adxl313_data *data;
struct iio_dev *indio_dev;
- int ret;
+ u8 int_line;
+ u8 int_map_msk;
+ int irq, ret;
indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
if (!indio_dev)
@@ -461,6 +469,45 @@ int adxl313_core_probe(struct device *dev,
return ret;
}
+ int_line = ADXL313_INT1;
+ irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT1");
+ if (irq < 0) {
+ int_line = ADXL313_INT2;
+ irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT2");
+ if (irq < 0)
+ int_line = ADXL313_INT_NONE;
+ }
+
+ if (int_line != ADXL313_INT_NONE) {
+ /* FIFO_STREAM mode */
+ int_map_msk = ADXL313_INT_DREADY | ADXL313_INT_ACTIVITY |
+ ADXL313_INT_INACTIVITY | ADXL313_INT_WATERMARK |
+ ADXL313_INT_OVERRUN;
+ ret = regmap_assign_bits(data->regmap, ADXL313_REG_INT_MAP,
+ int_map_msk, int_line == ADXL313_INT2);
+ if (ret)
+ return ret;
+ } else {
+ /*
+ * FIFO_BYPASSED mode
+ *
+ * When no interrupt lines are specified, the driver falls back
+ * to use the sensor in FIFO_BYPASS mode. This means turning off
+ * internal FIFO and interrupt generation (since there is no
+ * line specified). Unmaskable interrupts such as overrun or
+ * data ready won't interfere. Even that a FIFO_STREAM mode w/o
+ * connected interrupt line might allow for obtaining raw
+ * measurements, a fallback to disable interrupts when no
+ * interrupt lines are connected seems to be the cleaner
+ * solution.
+ */
+ ret = regmap_write(data->regmap, ADXL313_REG_FIFO_CTL,
+ FIELD_PREP(ADXL313_REG_FIFO_CTL_MODE_MSK,
+ ADXL313_FIFO_BYPASS));
+ if (ret)
+ return ret;
+ }
+
return devm_iio_device_register(dev, indio_dev);
}
EXPORT_SYMBOL_NS_GPL(adxl313_core_probe, IIO_ADXL313);
--
2.39.5On Sun, Jun 1, 2025 at 8:21 PM Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> Evaluate the devicetree property for an optional interrupt line, and
> configure the interrupt mapping accordingly. When no interrupt line
> is defined in the devicetree, keep the FIFO in bypass mode as before.
...
> struct adxl313_data *data;
> struct iio_dev *indio_dev;
> - int ret;
> + u8 int_line;
> + u8 int_map_msk;
> + int irq, ret;
Why do you need a specific irq variable?
...
> + int_line = ADXL313_INT1;
Assign this when we are sure that the INT1 is defined. Current
approach is not robust.
> + irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT1");
> + if (irq < 0) {
> + int_line = ADXL313_INT2;
> + irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT2");
> + if (irq < 0)
> + int_line = ADXL313_INT_NONE;
> + }
So, the below code does not use the returned vIRQ, moreover, the above
code actually does the IRQ mapping. Why do you need that if the code
doesn't use it?
> + if (int_line != ADXL313_INT_NONE) {
Why not positive conditional? But see below...
> + /* FIFO_STREAM mode */
> + int_map_msk = ADXL313_INT_DREADY | ADXL313_INT_ACTIVITY |
> + ADXL313_INT_INACTIVITY | ADXL313_INT_WATERMARK |
> + ADXL313_INT_OVERRUN;
> + ret = regmap_assign_bits(data->regmap, ADXL313_REG_INT_MAP,
> + int_map_msk, int_line == ADXL313_INT2);
This is fragile. It heavily relies on the existence of exactly three
IRQ variants. Instead of defining special case (NONE) simply use
whatever is undefined as the default case
switch (IRQ type) {
case 'INT1':
...
break;
case 'INT2':
...
break;
default:
// FIFO bypass mode
...
break;
}
But still, the main question and confusion here is the absence of the
users of 'irq'.
> + if (ret)
> + return ret;
> + } else {
> + /*
> + * FIFO_BYPASSED mode
> + *
> + * When no interrupt lines are specified, the driver falls back
> + * to use the sensor in FIFO_BYPASS mode. This means turning off
> + * internal FIFO and interrupt generation (since there is no
> + * line specified). Unmaskable interrupts such as overrun or
> + * data ready won't interfere. Even that a FIFO_STREAM mode w/o
> + * connected interrupt line might allow for obtaining raw
> + * measurements, a fallback to disable interrupts when no
> + * interrupt lines are connected seems to be the cleaner
> + * solution.
> + */
> + ret = regmap_write(data->regmap, ADXL313_REG_FIFO_CTL,
> + FIELD_PREP(ADXL313_REG_FIFO_CTL_MODE_MSK,
> + ADXL313_FIFO_BYPASS));
> + if (ret)
> + return ret;
> + }
> +
> return devm_iio_device_register(dev, indio_dev);
> }
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On Sun, Jun 1, 2025 at 9:21 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sun, Jun 1, 2025 at 8:21 PM Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >
> > Evaluate the devicetree property for an optional interrupt line, and
> > configure the interrupt mapping accordingly. When no interrupt line
> > is defined in the devicetree, keep the FIFO in bypass mode as before.
>
> ...
>
> > struct adxl313_data *data;
> > struct iio_dev *indio_dev;
> > - int ret;
> > + u8 int_line;
> > + u8 int_map_msk;
> > + int irq, ret;
>
> Why do you need a specific irq variable?
>
> ...
>
> > + int_line = ADXL313_INT1;
>
> Assign this when we are sure that the INT1 is defined. Current
> approach is not robust.
>
> > + irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT1");
> > + if (irq < 0) {
> > + int_line = ADXL313_INT2;
> > + irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT2");
> > + if (irq < 0)
> > + int_line = ADXL313_INT_NONE;
> > + }
>
> So, the below code does not use the returned vIRQ, moreover, the above
> code actually does the IRQ mapping. Why do you need that if the code
> doesn't use it?
>
> > + if (int_line != ADXL313_INT_NONE) {
>
> Why not positive conditional? But see below...
>
> > + /* FIFO_STREAM mode */
> > + int_map_msk = ADXL313_INT_DREADY | ADXL313_INT_ACTIVITY |
> > + ADXL313_INT_INACTIVITY | ADXL313_INT_WATERMARK |
> > + ADXL313_INT_OVERRUN;
> > + ret = regmap_assign_bits(data->regmap, ADXL313_REG_INT_MAP,
> > + int_map_msk, int_line == ADXL313_INT2);
>
> This is fragile. It heavily relies on the existence of exactly three
> IRQ variants. Instead of defining special case (NONE) simply use
> whatever is undefined as the default case
>
> switch (IRQ type) {
> case 'INT1':
> ...
> break;
> case 'INT2':
> ...
> break;
> default:
> // FIFO bypass mode
> ...
> break;
> }
The idea here is actually to conditionally try to read if optional
interrupt lines are configured in the DT. First I check if INT1 is
configured. If not, I try INT2. Else, no interrupt line was setup. The
interrupt lines just need to be configured in the mapping register.
So, there is actually nothing more to a case INT1 or case INT2.
With this explanation and from how I also interprete your and
Jonathans commit, I'll go to merge some of the patches for a next
version. I won't change to switch/case here. IMHO it is not the
approach for the above idea (might be wrong).
I appreciate your feedback and have taken note of it. Thank you.
>
> But still, the main question and confusion here is the absence of the
> users of 'irq'.
>
> > + if (ret)
> > + return ret;
> > + } else {
> > + /*
> > + * FIFO_BYPASSED mode
> > + *
> > + * When no interrupt lines are specified, the driver falls back
> > + * to use the sensor in FIFO_BYPASS mode. This means turning off
> > + * internal FIFO and interrupt generation (since there is no
> > + * line specified). Unmaskable interrupts such as overrun or
> > + * data ready won't interfere. Even that a FIFO_STREAM mode w/o
> > + * connected interrupt line might allow for obtaining raw
> > + * measurements, a fallback to disable interrupts when no
> > + * interrupt lines are connected seems to be the cleaner
> > + * solution.
> > + */
> > + ret = regmap_write(data->regmap, ADXL313_REG_FIFO_CTL,
> > + FIELD_PREP(ADXL313_REG_FIFO_CTL_MODE_MSK,
> > + ADXL313_FIFO_BYPASS));
> > + if (ret)
> > + return ret;
> > + }
> > +
> > return devm_iio_device_register(dev, indio_dev);
> > }
>
> --
> With Best Regards,
> Andy Shevchenko
© 2016 - 2026 Red Hat, Inc.