[PATCH v4 1/2] iio:imu: inv_mpu6050: support more interrupt types

[PATCH v4 1/2] iio:imu: inv_mpu6050: support more interrupt types

[Martin Kelly]

Currently, we support only rising edge interrupts, and in fact we assume
that the interrupt we're given is rising edge (and things won't work if
it's not). However, the device supports rising edge, falling edge, level
low, and level high interrupts.

Empirically, on my system, switching to level interrupts has fixed a
problem I had with significant (~40%) interrupt loss with edge
interrupts, as well as bus errors at high frequencies (possibly a
concurrency issue, which I will look into separately). This issue is
likely related to the SoC I'm using (Allwinner H3), but being able to
switch the interrupt type is still a very useful workaround.

I tested this with each interrupt type and verified correct behavior in
a logic analyzer.

Add support for these interrupt types while also eliminating the error
case of the device tree and driver using different interrupt types.

Signed-off-by: Martin Kelly <mkelly@xevo.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c    | 36 +++++++++++++++++++++++++++
 drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c     |  5 ++--
 drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h     | 12 ++++++++-
 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c    |  9 +++++++
 drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c |  2 +-
 5 files changed, 59 insertions(+), 5 deletions(-)

v2:
- Changed to ACK level interrupts at the start of the bottom half thread instead
  of at the end of it. Without this, the sample timestamps get distorted because
  the time to handle the bottom half thread delays future interrupts. With this
  change, the timestamps appear evenly distributed at the right frequency.
v3:
- Sent version 2 too quickly. Now that the ACK is moved to the top of the
  function, the "goto out" logic is unnecessary, so we can clean it up.
v4:
- Moved the ACK inside the mutex.

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index 7d64be353403..869768d08802 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -24,6 +24,7 @@
 #include <linux/spinlock.h>
 #include <linux/iio/iio.h>
 #include <linux/acpi.h>
+#include <linux/platform_device.h>
 #include "inv_mpu_iio.h"
 
 /*
@@ -52,6 +53,7 @@ static const struct inv_mpu6050_reg_map reg_set_6500 = {
 	.raw_accl               = INV_MPU6050_REG_RAW_ACCEL,
 	.temperature            = INV_MPU6050_REG_TEMPERATURE,
 	.int_enable             = INV_MPU6050_REG_INT_ENABLE,
+	.int_status             = INV_MPU6050_REG_INT_STATUS,
 	.pwr_mgmt_1             = INV_MPU6050_REG_PWR_MGMT_1,
 	.pwr_mgmt_2             = INV_MPU6050_REG_PWR_MGMT_2,
 	.int_pin_cfg		= INV_MPU6050_REG_INT_PIN_CFG,
@@ -281,6 +283,10 @@ static int inv_mpu6050_init_config(struct iio_dev *indio_dev)
 	memcpy(&st->chip_config, hw_info[st->chip_type].config,
 	       sizeof(struct inv_mpu6050_chip_config));
 	result = inv_mpu6050_set_power_itg(st, false);
+	if (result)
+		return result;
+
+	result = regmap_write(st->map, st->reg->int_pin_cfg, st->irq_mask);
 
 	return result;
 }
@@ -882,6 +888,7 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
 	struct inv_mpu6050_platform_data *pdata;
 	struct device *dev = regmap_get_device(regmap);
 	int result;
+	struct irq_data *desc;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
 	if (!indio_dev)
@@ -913,6 +920,35 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
 		st->plat_data = *pdata;
 	}
 
+	desc = irq_get_irq_data(irq);
+	if (!desc) {
+		dev_err(dev, "Could not find IRQ %d\n", irq);
+		return -EBUSY;
+	}
+
+	st->irq_type = irqd_get_trigger_type(desc);
+	if (st->irq_type == IRQF_TRIGGER_RISING) {
+		st->irq_mask = INV_MPU6050_ACTIVE_HIGH |
+				INV_MPU6050_EDGE_TRIGGERED;
+		st->irq_level_triggered = false;
+	} else if (st->irq_type == IRQF_TRIGGER_FALLING) {
+		st->irq_mask = INV_MPU6050_ACTIVE_LOW |
+				INV_MPU6050_EDGE_TRIGGERED;
+		st->irq_level_triggered = false;
+	} else if (st->irq_type == IRQF_TRIGGER_HIGH) {
+		st->irq_mask = INV_MPU6050_ACTIVE_HIGH |
+				INV_MPU6050_LEVEL_TRIGGERED;
+		st->irq_level_triggered = true;
+	} else if (st->irq_type == IRQF_TRIGGER_LOW) {
+		st->irq_mask = INV_MPU6050_ACTIVE_LOW |
+				INV_MPU6050_LEVEL_TRIGGERED;
+		st->irq_level_triggered = true;
+	} else {
+		dev_err(dev, "Invalid interrupt type 0x%x specified\n",
+			st->irq_type);
+		return -EBUSY;
+	}
+
 	/* power is turned on inside check chip type*/
 	result = inv_check_and_setup_chip(st);
 	if (result)
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
index fcd7a92b6cf8..bb3064537943 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
@@ -44,8 +44,7 @@ static int inv_mpu6050_select_bypass(struct i2c_mux_core *muxc, u32 chan_id)
 	if (!ret) {
 		st->powerup_count++;
 		ret = regmap_write(st->map, st->reg->int_pin_cfg,
-				   INV_MPU6050_INT_PIN_CFG |
-				   INV_MPU6050_BIT_BYPASS_EN);
+			   st->irq_mask | INV_MPU6050_BIT_BYPASS_EN);
 	}
 write_error:
 	mutex_unlock(&st->lock);
@@ -60,7 +59,7 @@ static int inv_mpu6050_deselect_bypass(struct i2c_mux_core *muxc, u32 chan_id)
 
 	mutex_lock(&st->lock);
 	/* It doesn't really mattter, if any of the calls fails */
-	regmap_write(st->map, st->reg->int_pin_cfg, INV_MPU6050_INT_PIN_CFG);
+	regmap_write(st->map, st->reg->int_pin_cfg, 0);
 	st->powerup_count--;
 	if (!st->powerup_count)
 		regmap_write(st->map, st->reg->pwr_mgmt_1,
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
index 065794162d65..2822b638804f 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
@@ -40,6 +40,7 @@
  *  @raw_accl:		Address of first accel register.
  *  @temperature:	temperature register
  *  @int_enable:	Interrupt enable register.
+ *  @int_status:	Interrupt status register.
  *  @pwr_mgmt_1:	Controls chip's power state and clock source.
  *  @pwr_mgmt_2:	Controls power state of individual sensors.
  *  @int_pin_cfg;	Controls interrupt pin configuration.
@@ -60,6 +61,7 @@ struct inv_mpu6050_reg_map {
 	u8 raw_accl;
 	u8 temperature;
 	u8 int_enable;
+	u8 int_status;
 	u8 pwr_mgmt_1;
 	u8 pwr_mgmt_2;
 	u8 int_pin_cfg;
@@ -143,6 +145,9 @@ struct inv_mpu6050_state {
 	DECLARE_KFIFO(timestamps, long long, TIMESTAMP_FIFO_SIZE);
 	struct regmap *map;
 	int irq;
+	int irq_type;
+	u16 irq_mask;
+	bool irq_level_triggered;
 };
 
 /*register and associated bit definition*/
@@ -159,6 +164,7 @@ struct inv_mpu6050_state {
 #define INV_MPU6050_BITS_GYRO_OUT           0x70
 
 #define INV_MPU6050_REG_INT_ENABLE          0x38
+#define INV_MPU6050_REG_INT_STATUS          0x3a
 #define INV_MPU6050_BIT_DATA_RDY_EN         0x01
 #define INV_MPU6050_BIT_DMP_INT_EN          0x02
 
@@ -190,6 +196,11 @@ struct inv_mpu6050_state {
 #define INV_MPU6050_FIFO_COUNT_BYTE          2
 #define INV_MPU6050_FIFO_THRESHOLD           500
 
+#define INV_MPU6050_ACTIVE_HIGH             0x00
+#define INV_MPU6050_ACTIVE_LOW              0x80
+#define INV_MPU6050_EDGE_TRIGGERED          0x00
+#define INV_MPU6050_LEVEL_TRIGGERED         0x20
+
 /* mpu6500 registers */
 #define INV_MPU6500_REG_ACCEL_CONFIG_2      0x1D
 #define INV_MPU6500_REG_ACCEL_OFFSET        0x77
@@ -216,7 +227,6 @@ struct inv_mpu6050_state {
 
 #define INV_MPU6050_REG_INT_PIN_CFG	0x37
 #define INV_MPU6050_BIT_BYPASS_EN	0x2
-#define INV_MPU6050_INT_PIN_CFG		0
 
 /* init parameters */
 #define INV_MPU6050_INIT_FIFO_RATE           50
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
index ff81c6aa009d..04dc4a11a9d0 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
@@ -129,6 +129,15 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
 	s64 timestamp;
 
 	mutex_lock(&st->lock);
+
+	if (st->irq_level_triggered) {
+		result = regmap_read(st->map, st->reg->int_status,
+				(int *) data);
+		if (result)
+			dev_err(regmap_get_device(st->map),
+				"failed to ack interrupt\n");
+	}
+
 	if (!(st->chip_config.accl_fifo_enable |
 		st->chip_config.gyro_fifo_enable))
 		goto end_session;
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
index f963f9fc98c0..1e7201a43e34 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
@@ -131,7 +131,7 @@ int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev)
 
 	ret = devm_request_irq(&indio_dev->dev, st->irq,
 			       &iio_trigger_generic_data_rdy_poll,
-			       IRQF_TRIGGER_RISING,
+			       st->irq_type,
 			       "inv_mpu",
 			       st->trig);
 	if (ret)
-- 
2.11.0

[PATCH v4 2/2] dt-bindings: iio:imu:mpu6050: support more interrupt types

[Martin Kelly]

Document that the inv_mpu6050 driver now supports falling edge, rising
edge, level low, and level high interrupt types, rather than just rising
edge.

The language used is the same as that in st_lsm6dsx.txt.

Signed-off-by: Martin Kelly <mkelly@xevo.com>
---
 Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt b/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt
index 2b4514592f83..6b106d5ef298 100644
--- a/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt
+++ b/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt
@@ -11,7 +11,12 @@ Required properties:
 		"invensense,icm20608"
  - reg : the I2C address of the sensor
  - interrupt-parent : should be the phandle for the interrupt controller
- - interrupts : interrupt mapping for GPIO IRQ
+ - interrupts: interrupt mapping for IRQ. It should be configured with flags
+   IRQ_TYPE_LEVEL_HIGH, IRQ_TYPE_EDGE_RISING, IRQ_TYPE_LEVEL_LOW or
+   IRQ_TYPE_EDGE_FALLING.
+
+  Refer to interrupt-controller/interrupts.txt for generic interrupt client node
+  bindings.
 
 Optional properties:
  - mount-matrix: an optional 3x3 mounting rotation matrix
@@ -24,7 +29,7 @@ Example:
 		compatible = "invensense,mpu6050";
 		reg = <0x68>;
 		interrupt-parent = <&gpio1>;
-		interrupts = <18 1>;
+		interrupts = <18 IRQ_TYPE_EDGE_RISING>;
 		mount-matrix = "-0.984807753012208",  /* x0 */
 		               "0",                   /* y0 */
 		               "-0.173648177666930",  /* z0 */
@@ -41,7 +46,7 @@ Example:
 		compatible = "invensense,mpu9250";
 		reg = <0x68>;
 		interrupt-parent = <&gpio3>;
-		interrupts = <21 1>;
+		interrupts = <21 IRQ_TYPE_LEVEL_HIGH>;
 		i2c-gate {
 			#address-cells = <1>;
 			#size-cells = <0>;
-- 
2.11.0

Re: [PATCH v4 2/2] dt-bindings: iio:imu:mpu6050: support more interrupt types

[Rob Herring]

On Fri, Apr 06, 2018 at 03:00:48PM -0700, Martin Kelly wrote:
> Document that the inv_mpu6050 driver now supports falling edge, rising

It's really about what the h/w supports for bindings.

> edge, level low, and level high interrupt types, rather than just rising
> edge.
> 
> The language used is the same as that in st_lsm6dsx.txt.
> 
> Signed-off-by: Martin Kelly <mkelly@xevo.com>
> ---
>  Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v4 2/2] dt-bindings: iio:imu:mpu6050: support more interrupt types

[Martin Kelly]

On 04/10/2018 07:12 AM, Rob Herring wrote:
> On Fri, Apr 06, 2018 at 03:00:48PM -0700, Martin Kelly wrote:
>> Document that the inv_mpu6050 driver now supports falling edge, rising
> 
> It's really about what the h/w supports for bindings.
> 

I understand that about devicetree in general, though in this case the 
device supports all these types but the driver did not and would 
silently fail if you gave the wrong type. I wanted to document that it's 
now safe to use the other interrupt types, while before the 
documentation told you (correctly) to use only rising edge.

If you think it's a good idea, I will change the description to 
"Document that the MPU9250 supports ..." to clarify.

>> edge, level low, and level high interrupt types, rather than just rising
>> edge.
>>
>> The language used is the same as that in st_lsm6dsx.txt.
>>
>> Signed-off-by: Martin Kelly <mkelly@xevo.com>
>> ---
>>   Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt | 11 ++++++++---
>>   1 file changed, 8 insertions(+), 3 deletions(-)
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> 

Re: [PATCH v4 1/2] iio:imu: inv_mpu6050: support more interrupt types

[Jean-Baptiste Maneyrol]

Hello,

my comments below.

JB

On 07/04/2018 00:00, Martin Kelly wrote:
> Currently, we support only rising edge interrupts, and in fact we assume
> that the interrupt we're given is rising edge (and things won't work if
> it's not). However, the device supports rising edge, falling edge, level
> low, and level high interrupts.
> 
> Empirically, on my system, switching to level interrupts has fixed a
> problem I had with significant (~40%) interrupt loss with edge
> interrupts, as well as bus errors at high frequencies (possibly a
> concurrency issue, which I will look into separately). This issue is
> likely related to the SoC I'm using (Allwinner H3), but being able to
> switch the interrupt type is still a very useful workaround.
> 
> I tested this with each interrupt type and verified correct behavior in
> a logic analyzer.
> 
> Add support for these interrupt types while also eliminating the error
> case of the device tree and driver using different interrupt types.
> 
> Signed-off-by: Martin Kelly <mkelly@xevo.com>
> ---
>   drivers/iio/imu/inv_mpu6050/inv_mpu_core.c    | 36 +++++++++++++++++++++++++++
>   drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c     |  5 ++--
>   drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h     | 12 ++++++++-
>   drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c    |  9 +++++++
>   drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c |  2 +-
>   5 files changed, 59 insertions(+), 5 deletions(-)
> 
> v2:
> - Changed to ACK level interrupts at the start of the bottom half thread instead
>    of at the end of it. Without this, the sample timestamps get distorted because
>    the time to handle the bottom half thread delays future interrupts. With this
>    change, the timestamps appear evenly distributed at the right frequency.
> v3:
> - Sent version 2 too quickly. Now that the ACK is moved to the top of the
>    function, the "goto out" logic is unnecessary, so we can clean it up.
> v4:
> - Moved the ACK inside the mutex.
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> index 7d64be353403..869768d08802 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> @@ -24,6 +24,7 @@
>   #include <linux/spinlock.h>
>   #include <linux/iio/iio.h>
>   #include <linux/acpi.h>
> +#include <linux/platform_device.h>
>   #include "inv_mpu_iio.h"
>   
>   /*
> @@ -52,6 +53,7 @@ static const struct inv_mpu6050_reg_map reg_set_6500 = {
>   	.raw_accl               = INV_MPU6050_REG_RAW_ACCEL,
>   	.temperature            = INV_MPU6050_REG_TEMPERATURE,
>   	.int_enable             = INV_MPU6050_REG_INT_ENABLE,
> +	.int_status             = INV_MPU6050_REG_INT_STATUS,
>   	.pwr_mgmt_1             = INV_MPU6050_REG_PWR_MGMT_1,
>   	.pwr_mgmt_2             = INV_MPU6050_REG_PWR_MGMT_2,
>   	.int_pin_cfg		= INV_MPU6050_REG_INT_PIN_CFG,
> @@ -281,6 +283,10 @@ static int inv_mpu6050_init_config(struct iio_dev *indio_dev)
>   	memcpy(&st->chip_config, hw_info[st->chip_type].config,
>   	       sizeof(struct inv_mpu6050_chip_config));
>   	result = inv_mpu6050_set_power_itg(st, false);
> +	if (result)
> +		return result;
> +
> +	result = regmap_write(st->map, st->reg->int_pin_cfg, st->irq_mask);
Better put this before turning power off. Some registers require power 
on or the change is ignored.

>   
>   	return result;
>   }
> @@ -882,6 +888,7 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
>   	struct inv_mpu6050_platform_data *pdata;
>   	struct device *dev = regmap_get_device(regmap);
>   	int result;
> +	struct irq_data *desc;
>   
>   	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
>   	if (!indio_dev)
> @@ -913,6 +920,35 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
>   		st->plat_data = *pdata;
>   	}
>   
> +	desc = irq_get_irq_data(irq);
> +	if (!desc) {
> +		dev_err(dev, "Could not find IRQ %d\n", irq);
> +		return -EBUSY;
> +	}
> +
> +	st->irq_type = irqd_get_trigger_type(desc);
> +	if (st->irq_type == IRQF_TRIGGER_RISING) {
> +		st->irq_mask = INV_MPU6050_ACTIVE_HIGH |
> +				INV_MPU6050_EDGE_TRIGGERED;
> +		st->irq_level_triggered = false;
> +	} else if (st->irq_type == IRQF_TRIGGER_FALLING) {
> +		st->irq_mask = INV_MPU6050_ACTIVE_LOW |
> +				INV_MPU6050_EDGE_TRIGGERED;
> +		st->irq_level_triggered = false;
> +	} else if (st->irq_type == IRQF_TRIGGER_HIGH) {
> +		st->irq_mask = INV_MPU6050_ACTIVE_HIGH |
> +				INV_MPU6050_LEVEL_TRIGGERED;
> +		st->irq_level_triggered = true;
> +	} else if (st->irq_type == IRQF_TRIGGER_LOW) {
> +		st->irq_mask = INV_MPU6050_ACTIVE_LOW |
> +				INV_MPU6050_LEVEL_TRIGGERED;
> +		st->irq_level_triggered = true;
> +	} else {
> +		dev_err(dev, "Invalid interrupt type 0x%x specified\n",
> +			st->irq_type);
> +		return -EBUSY;
> +	}
> +
>   	/* power is turned on inside check chip type*/
>   	result = inv_check_and_setup_chip(st);
>   	if (result)
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> index fcd7a92b6cf8..bb3064537943 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> @@ -44,8 +44,7 @@ static int inv_mpu6050_select_bypass(struct i2c_mux_core *muxc, u32 chan_id)
>   	if (!ret) {
>   		st->powerup_count++;
>   		ret = regmap_write(st->map, st->reg->int_pin_cfg,
> -				   INV_MPU6050_INT_PIN_CFG |
> -				   INV_MPU6050_BIT_BYPASS_EN);
> +			   st->irq_mask | INV_MPU6050_BIT_BYPASS_EN);
>   	}
>   write_error:
>   	mutex_unlock(&st->lock);
> @@ -60,7 +59,7 @@ static int inv_mpu6050_deselect_bypass(struct i2c_mux_core *muxc, u32 chan_id)
>   
>   	mutex_lock(&st->lock);
>   	/* It doesn't really mattter, if any of the calls fails */
> -	regmap_write(st->map, st->reg->int_pin_cfg, INV_MPU6050_INT_PIN_CFG);
> +	regmap_write(st->map, st->reg->int_pin_cfg, 0);
We need to write st->irq_mask here or we will loose irq configuration 
each time an i2c device under the mux is used.

>   	st->powerup_count--;
>   	if (!st->powerup_count)
>   		regmap_write(st->map, st->reg->pwr_mgmt_1,
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> index 065794162d65..2822b638804f 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> @@ -40,6 +40,7 @@
>    *  @raw_accl:		Address of first accel register.
>    *  @temperature:	temperature register
>    *  @int_enable:	Interrupt enable register.
> + *  @int_status:	Interrupt status register.
>    *  @pwr_mgmt_1:	Controls chip's power state and clock source.
>    *  @pwr_mgmt_2:	Controls power state of individual sensors.
>    *  @int_pin_cfg;	Controls interrupt pin configuration.
> @@ -60,6 +61,7 @@ struct inv_mpu6050_reg_map {
>   	u8 raw_accl;
>   	u8 temperature;
>   	u8 int_enable;
> +	u8 int_status;
>   	u8 pwr_mgmt_1;
>   	u8 pwr_mgmt_2;
>   	u8 int_pin_cfg;
> @@ -143,6 +145,9 @@ struct inv_mpu6050_state {
>   	DECLARE_KFIFO(timestamps, long long, TIMESTAMP_FIFO_SIZE);
>   	struct regmap *map;
>   	int irq;
> +	int irq_type;
> +	u16 irq_mask;
> +	bool irq_level_triggered;
>   };
>   
>   /*register and associated bit definition*/
> @@ -159,6 +164,7 @@ struct inv_mpu6050_state {
>   #define INV_MPU6050_BITS_GYRO_OUT           0x70
>   
>   #define INV_MPU6050_REG_INT_ENABLE          0x38
> +#define INV_MPU6050_REG_INT_STATUS          0x3a
>   #define INV_MPU6050_BIT_DATA_RDY_EN         0x01
>   #define INV_MPU6050_BIT_DMP_INT_EN          0x02
>   
> @@ -190,6 +196,11 @@ struct inv_mpu6050_state {
>   #define INV_MPU6050_FIFO_COUNT_BYTE          2
>   #define INV_MPU6050_FIFO_THRESHOLD           500
>   
> +#define INV_MPU6050_ACTIVE_HIGH             0x00
> +#define INV_MPU6050_ACTIVE_LOW              0x80
> +#define INV_MPU6050_EDGE_TRIGGERED          0x00
> +#define INV_MPU6050_LEVEL_TRIGGERED         0x20
> +
Datasheet name for this bit is LATCH_INT_EN. Better use it to be coherent.

>   /* mpu6500 registers */
>   #define INV_MPU6500_REG_ACCEL_CONFIG_2      0x1D
>   #define INV_MPU6500_REG_ACCEL_OFFSET        0x77
> @@ -216,7 +227,6 @@ struct inv_mpu6050_state {
>   
>   #define INV_MPU6050_REG_INT_PIN_CFG	0x37
>   #define INV_MPU6050_BIT_BYPASS_EN	0x2
> -#define INV_MPU6050_INT_PIN_CFG		0
>   
>   /* init parameters */
>   #define INV_MPU6050_INIT_FIFO_RATE           50
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> index ff81c6aa009d..04dc4a11a9d0 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> @@ -129,6 +129,15 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
>   	s64 timestamp;
>   
>   	mutex_lock(&st->lock);
> +
> +	if (st->irq_level_triggered) {
> +		result = regmap_read(st->map, st->reg->int_status,
> +				(int *) data);
> +		if (result)
> +			dev_err(regmap_get_device(st->map),
> +				"failed to ack interrupt\n");
> +	}
> +
I would prefer to delete this irq_level_triggered boolean and read 
status in all cases and check the value. This is more coherent and can 
be useful in the future for handling WakeOnMotion or FIFO overflow 
interrupts. Also interesting to filter out possible spurious interrupt.

>   	if (!(st->chip_config.accl_fifo_enable |
>   		st->chip_config.gyro_fifo_enable))
>   		goto end_session;
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> index f963f9fc98c0..1e7201a43e34 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> @@ -131,7 +131,7 @@ int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev)
>   
>   	ret = devm_request_irq(&indio_dev->dev, st->irq,
>   			       &iio_trigger_generic_data_rdy_poll,
> -			       IRQF_TRIGGER_RISING,
> +			       st->irq_type,
>   			       "inv_mpu",
>   			       st->trig);
>   	if (ret)
> 

Re: [PATCH v4 1/2] iio:imu: inv_mpu6050: support more interrupt types

[Martin Kelly]

Thanks, replies inline. I sent version 5 that addresses all these and a 
few other things I noticed.

On 04/09/2018 12:08 PM, Jean-Baptiste Maneyrol wrote:
> Hello,
> 
> my comments below.
> 
> JB
> 
> On 07/04/2018 00:00, Martin Kelly wrote:
>> Currently, we support only rising edge interrupts, and in fact we assume
>> that the interrupt we're given is rising edge (and things won't work if
>> it's not). However, the device supports rising edge, falling edge, level
>> low, and level high interrupts.
>>
>> Empirically, on my system, switching to level interrupts has fixed a
>> problem I had with significant (~40%) interrupt loss with edge
>> interrupts, as well as bus errors at high frequencies (possibly a
>> concurrency issue, which I will look into separately). This issue is
>> likely related to the SoC I'm using (Allwinner H3), but being able to
>> switch the interrupt type is still a very useful workaround.
>>
>> I tested this with each interrupt type and verified correct behavior in
>> a logic analyzer.
>>
>> Add support for these interrupt types while also eliminating the error
>> case of the device tree and driver using different interrupt types.
>>
>> Signed-off-by: Martin Kelly <mkelly@xevo.com>
>> ---
>>   drivers/iio/imu/inv_mpu6050/inv_mpu_core.c    | 36 
>> +++++++++++++++++++++++++++
>>   drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c     |  5 ++--
>>   drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h     | 12 ++++++++-
>>   drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c    |  9 +++++++
>>   drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c |  2 +-
>>   5 files changed, 59 insertions(+), 5 deletions(-)
>>
>> v2:
>> - Changed to ACK level interrupts at the start of the bottom half 
>> thread instead
>>    of at the end of it. Without this, the sample timestamps get 
>> distorted because
>>    the time to handle the bottom half thread delays future interrupts. 
>> With this
>>    change, the timestamps appear evenly distributed at the right 
>> frequency.
>> v3:
>> - Sent version 2 too quickly. Now that the ACK is moved to the top of the
>>    function, the "goto out" logic is unnecessary, so we can clean it up.
>> v4:
>> - Moved the ACK inside the mutex.
>>
>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c 
>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>> index 7d64be353403..869768d08802 100644
>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>> @@ -24,6 +24,7 @@
>>   #include <linux/spinlock.h>
>>   #include <linux/iio/iio.h>
>>   #include <linux/acpi.h>
>> +#include <linux/platform_device.h>
>>   #include "inv_mpu_iio.h"
>>   /*
>> @@ -52,6 +53,7 @@ static const struct inv_mpu6050_reg_map reg_set_6500 
>> = {
>>       .raw_accl               = INV_MPU6050_REG_RAW_ACCEL,
>>       .temperature            = INV_MPU6050_REG_TEMPERATURE,
>>       .int_enable             = INV_MPU6050_REG_INT_ENABLE,
>> +    .int_status             = INV_MPU6050_REG_INT_STATUS,
>>       .pwr_mgmt_1             = INV_MPU6050_REG_PWR_MGMT_1,
>>       .pwr_mgmt_2             = INV_MPU6050_REG_PWR_MGMT_2,
>>       .int_pin_cfg        = INV_MPU6050_REG_INT_PIN_CFG,
>> @@ -281,6 +283,10 @@ static int inv_mpu6050_init_config(struct iio_dev 
>> *indio_dev)
>>       memcpy(&st->chip_config, hw_info[st->chip_type].config,
>>              sizeof(struct inv_mpu6050_chip_config));
>>       result = inv_mpu6050_set_power_itg(st, false);
>> +    if (result)
>> +        return result;
>> +
>> +    result = regmap_write(st->map, st->reg->int_pin_cfg, st->irq_mask);
> Better put this before turning power off. Some registers require power 
> on or the change is ignored >

Thanks, good point.

>>       return result;
>>   }
>> @@ -882,6 +888,7 @@ int inv_mpu_core_probe(struct regmap *regmap, int 
>> irq, const char *name,
>>       struct inv_mpu6050_platform_data *pdata;
>>       struct device *dev = regmap_get_device(regmap);
>>       int result;
>> +    struct irq_data *desc;
>>       indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
>>       if (!indio_dev)
>> @@ -913,6 +920,35 @@ int inv_mpu_core_probe(struct regmap *regmap, int 
>> irq, const char *name,
>>           st->plat_data = *pdata;
>>       }
>> +    desc = irq_get_irq_data(irq);
>> +    if (!desc) {
>> +        dev_err(dev, "Could not find IRQ %d\n", irq);
>> +        return -EBUSY;
>> +    }
>> +
>> +    st->irq_type = irqd_get_trigger_type(desc);
>> +    if (st->irq_type == IRQF_TRIGGER_RISING) {
>> +        st->irq_mask = INV_MPU6050_ACTIVE_HIGH |
>> +                INV_MPU6050_EDGE_TRIGGERED;
>> +        st->irq_level_triggered = false;
>> +    } else if (st->irq_type == IRQF_TRIGGER_FALLING) {
>> +        st->irq_mask = INV_MPU6050_ACTIVE_LOW |
>> +                INV_MPU6050_EDGE_TRIGGERED;
>> +        st->irq_level_triggered = false;
>> +    } else if (st->irq_type == IRQF_TRIGGER_HIGH) {
>> +        st->irq_mask = INV_MPU6050_ACTIVE_HIGH |
>> +                INV_MPU6050_LEVEL_TRIGGERED;
>> +        st->irq_level_triggered = true;
>> +    } else if (st->irq_type == IRQF_TRIGGER_LOW) {
>> +        st->irq_mask = INV_MPU6050_ACTIVE_LOW |
>> +                INV_MPU6050_LEVEL_TRIGGERED;
>> +        st->irq_level_triggered = true;
>> +    } else {
>> +        dev_err(dev, "Invalid interrupt type 0x%x specified\n",
>> +            st->irq_type);
>> +        return -EBUSY;
>> +    }
>> +
>>       /* power is turned on inside check chip type*/
>>       result = inv_check_and_setup_chip(st);
>>       if (result)
>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c 
>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
>> index fcd7a92b6cf8..bb3064537943 100644
>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
>> @@ -44,8 +44,7 @@ static int inv_mpu6050_select_bypass(struct 
>> i2c_mux_core *muxc, u32 chan_id)
>>       if (!ret) {
>>           st->powerup_count++;
>>           ret = regmap_write(st->map, st->reg->int_pin_cfg,
>> -                   INV_MPU6050_INT_PIN_CFG |
>> -                   INV_MPU6050_BIT_BYPASS_EN);
>> +               st->irq_mask | INV_MPU6050_BIT_BYPASS_EN);
>>       }
>>   write_error:
>>       mutex_unlock(&st->lock);
>> @@ -60,7 +59,7 @@ static int inv_mpu6050_deselect_bypass(struct 
>> i2c_mux_core *muxc, u32 chan_id)
>>       mutex_lock(&st->lock);
>>       /* It doesn't really mattter, if any of the calls fails */
>> -    regmap_write(st->map, st->reg->int_pin_cfg, 
>> INV_MPU6050_INT_PIN_CFG);
>> +    regmap_write(st->map, st->reg->int_pin_cfg, 0);
> We need to write st->irq_mask here or we will loose irq configuration 
> each time an i2c device under the mux is used.
> 

Good catch.

>>       st->powerup_count--;
>>       if (!st->powerup_count)
>>           regmap_write(st->map, st->reg->pwr_mgmt_1,
>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h 
>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>> index 065794162d65..2822b638804f 100644
>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>> @@ -40,6 +40,7 @@
>>    *  @raw_accl:        Address of first accel register.
>>    *  @temperature:    temperature register
>>    *  @int_enable:    Interrupt enable register.
>> + *  @int_status:    Interrupt status register.
>>    *  @pwr_mgmt_1:    Controls chip's power state and clock source.
>>    *  @pwr_mgmt_2:    Controls power state of individual sensors.
>>    *  @int_pin_cfg;    Controls interrupt pin configuration.
>> @@ -60,6 +61,7 @@ struct inv_mpu6050_reg_map {
>>       u8 raw_accl;
>>       u8 temperature;
>>       u8 int_enable;
>> +    u8 int_status;
>>       u8 pwr_mgmt_1;
>>       u8 pwr_mgmt_2;
>>       u8 int_pin_cfg;
>> @@ -143,6 +145,9 @@ struct inv_mpu6050_state {
>>       DECLARE_KFIFO(timestamps, long long, TIMESTAMP_FIFO_SIZE);
>>       struct regmap *map;
>>       int irq;
>> +    int irq_type;
>> +    u16 irq_mask;
>> +    bool irq_level_triggered;
>>   };
>>   /*register and associated bit definition*/
>> @@ -159,6 +164,7 @@ struct inv_mpu6050_state {
>>   #define INV_MPU6050_BITS_GYRO_OUT           0x70
>>   #define INV_MPU6050_REG_INT_ENABLE          0x38
>> +#define INV_MPU6050_REG_INT_STATUS          0x3a
>>   #define INV_MPU6050_BIT_DATA_RDY_EN         0x01
>>   #define INV_MPU6050_BIT_DMP_INT_EN          0x02
>> @@ -190,6 +196,11 @@ struct inv_mpu6050_state {
>>   #define INV_MPU6050_FIFO_COUNT_BYTE          2
>>   #define INV_MPU6050_FIFO_THRESHOLD           500
>> +#define INV_MPU6050_ACTIVE_HIGH             0x00
>> +#define INV_MPU6050_ACTIVE_LOW              0x80
>> +#define INV_MPU6050_EDGE_TRIGGERED          0x00
>> +#define INV_MPU6050_LEVEL_TRIGGERED         0x20
>> +
> Datasheet name for this bit is LATCH_INT_EN. Better use it to be coherent.
> 

Agreed.

>>   /* mpu6500 registers */
>>   #define INV_MPU6500_REG_ACCEL_CONFIG_2      0x1D
>>   #define INV_MPU6500_REG_ACCEL_OFFSET        0x77
>> @@ -216,7 +227,6 @@ struct inv_mpu6050_state {
>>   #define INV_MPU6050_REG_INT_PIN_CFG    0x37
>>   #define INV_MPU6050_BIT_BYPASS_EN    0x2
>> -#define INV_MPU6050_INT_PIN_CFG        0
>>   /* init parameters */
>>   #define INV_MPU6050_INIT_FIFO_RATE           50
>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c 
>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
>> index ff81c6aa009d..04dc4a11a9d0 100644
>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
>> @@ -129,6 +129,15 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
>>       s64 timestamp;
>>       mutex_lock(&st->lock);
>> +
>> +    if (st->irq_level_triggered) {
>> +        result = regmap_read(st->map, st->reg->int_status,
>> +                (int *) data);
>> +        if (result)
>> +            dev_err(regmap_get_device(st->map),
>> +                "failed to ack interrupt\n");
>> +    }
>> +
> I would prefer to delete this irq_level_triggered boolean and read 
> status in all cases and check the value. This is more coherent and can 
> be useful in the future for handling WakeOnMotion or FIFO overflow 
> interrupts. Also interesting to filter out possible spurious interrupt.
> 

Yes, I like that. Changed, and added a warning if we get a spurious 
interrupt.

>>       if (!(st->chip_config.accl_fifo_enable |
>>           st->chip_config.gyro_fifo_enable))
>>           goto end_session;
>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c 
>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
>> index f963f9fc98c0..1e7201a43e34 100644
>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
>> @@ -131,7 +131,7 @@ int inv_mpu6050_probe_trigger(struct iio_dev 
>> *indio_dev)
>>       ret = devm_request_irq(&indio_dev->dev, st->irq,
>>                      &iio_trigger_generic_data_rdy_poll,
>> -                   IRQF_TRIGGER_RISING,
>> +                   st->irq_type,
>>                      "inv_mpu",
>>                      st->trig);
>>       if (ret)
>>