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

[PATCH v5 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. 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    | 33 ++++++++++++++++++++++++++-
 drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c     |  5 ++--
 drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h     | 14 ++++++++++--
 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c    | 13 +++++++++++
 drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c |  4 ++--
 5 files changed, 61 insertions(+), 8 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.
v5:
- Check interrupt status in all cases rather than only in the level triggered
  case, and warn if we get spurious interrupts.
- Rename INV_MPU6050_LEVEL_TRIGGERED to INV_MPU6050_LATCH_INT_EN to match
  datasheet naming.
- Write st->irq_mask prior to device power off to make sure it is fully set.
- Write st->irq_mask instead of 0 in inv_mpu6050_deselect_bypass.
- Make irq_type local instead of part of the driver state, as we use it only at
  probe time and never again.
- Remove the comment about bus lockups, as I have determined them to be
  unrelated.
- Add missing documentation for irq_type and irq_mask.

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index 7d64be353403..b711e6260d9a 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,
@@ -278,6 +280,10 @@ static int inv_mpu6050_init_config(struct iio_dev *indio_dev)
 	if (result)
 		return result;

+	result = regmap_write(st->map, st->reg->int_pin_cfg, st->irq_mask);
+	if (result)
+		return result;
+
 	memcpy(&st->chip_config, hw_info[st->chip_type].config,
 	       sizeof(struct inv_mpu6050_chip_config));
 	result = inv_mpu6050_set_power_itg(st, false);
@@ -882,6 +888,8 @@ 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;
+	int irq_type;

 	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
 	if (!indio_dev)
@@ -913,6 +921,29 @@ 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;
+	}
+
+	irq_type = irqd_get_trigger_type(desc);
+	if (irq_type == IRQF_TRIGGER_RISING)
+		st->irq_mask = INV_MPU6050_ACTIVE_HIGH;
+	else if (irq_type == IRQF_TRIGGER_FALLING)
+		st->irq_mask = INV_MPU6050_ACTIVE_LOW;
+	else if (irq_type == IRQF_TRIGGER_HIGH)
+		st->irq_mask = INV_MPU6050_ACTIVE_HIGH |
+			INV_MPU6050_LATCH_INT_EN;
+	else if (irq_type == IRQF_TRIGGER_LOW)
+		st->irq_mask = INV_MPU6050_ACTIVE_LOW |
+			INV_MPU6050_LATCH_INT_EN;
+	else {
+		dev_err(dev, "Invalid interrupt type 0x%x specified\n",
+			irq_type);
+		return -EBUSY;
+	}
+
 	/* power is turned on inside check chip type*/
 	result = inv_check_and_setup_chip(st);
 	if (result)
@@ -948,7 +979,7 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
 		dev_err(dev, "configure buffer fail %d\n", result);
 		return result;
 	}
-	result = inv_mpu6050_probe_trigger(indio_dev);
+	result = inv_mpu6050_probe_trigger(indio_dev, irq_type);
 	if (result) {
 		dev_err(dev, "trigger probe fail %d\n", result);
 		goto out_unreg_ring;
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
index fcd7a92b6cf8..1b02d2b69174 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, st->irq_mask);
 	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..064e3b28fdb0 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;
@@ -125,6 +127,7 @@ struct inv_mpu6050_hw {
  *  @timestamps:        kfifo queue to store time stamp.
  *  @map		regmap pointer.
  *  @irq		interrupt number.
+ *  @irq_mask		the int_pin_cfg mask to configure interrupt type
  */
 struct inv_mpu6050_state {
 #define TIMESTAMP_FIFO_SIZE 16
@@ -143,6 +146,7 @@ struct inv_mpu6050_state {
 	DECLARE_KFIFO(timestamps, long long, TIMESTAMP_FIFO_SIZE);
 	struct regmap *map;
 	int irq;
+	u16 irq_mask;
 };

 /*register and associated bit definition*/
@@ -159,6 +163,8 @@ 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_RAW_DATA_RDY_INT    0x01
 #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
+/* enable level triggering */
+#define INV_MPU6050_LATCH_INT_EN            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
@@ -287,7 +297,7 @@ enum inv_mpu6050_clock_sel_e {

 irqreturn_t inv_mpu6050_irq_handler(int irq, void *p);
 irqreturn_t inv_mpu6050_read_fifo(int irq, void *p);
-int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev);
+int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev, int irq_type);
 void inv_mpu6050_remove_trigger(struct inv_mpu6050_state *st);
 int inv_reset_fifo(struct iio_dev *indio_dev);
 int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en, u32 mask);
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
index ff81c6aa009d..8f1f637fb972 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
@@ -127,8 +127,21 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
 	u8 data[INV_MPU6050_OUTPUT_DATA_SIZE];
 	u16 fifo_count;
 	s64 timestamp;
+	int int_status;

 	mutex_lock(&st->lock);
+
+	/* ack interrupt and check status */
+	result = regmap_read(st->map, st->reg->int_status, &int_status);
+	if (result)
+		dev_err(regmap_get_device(st->map),
+			"failed to ack interrupt\n");
+	if (!(int_status & INV_MPU6050_BIT_RAW_DATA_RDY_INT)) {
+		dev_warn(regmap_get_device(st->map),
+			"spurious interrupt with status 0x%x\n", int_status);
+		goto end_session;
+	}
+
 	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..b8c5584e4252 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
@@ -117,7 +117,7 @@ static const struct iio_trigger_ops inv_mpu_trigger_ops = {
 	.set_trigger_state = &inv_mpu_data_rdy_trigger_set_state,
 };

-int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev)
+int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev, int irq_type)
 {
 	int ret;
 	struct inv_mpu6050_state *st = iio_priv(indio_dev);
@@ -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,
+			       irq_type,
 			       "inv_mpu",
 			       st->trig);
 	if (ret)
--
2.11.0

[PATCH v5 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 v5 1/2] iio:imu: inv_mpu6050: support more interrupt types

[Jean-Baptiste Maneyrol]

Hello,

some more concerns after a deeper look.

Thanks.
JB

On 09/04/2018 22:21, 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. 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    | 33 ++++++++++++++++++++++++++-
>   drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c     |  5 ++--
>   drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h     | 14 ++++++++++--
>   drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c    | 13 +++++++++++
>   drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c |  4 ++--
>   5 files changed, 61 insertions(+), 8 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.
> v5:
> - Check interrupt status in all cases rather than only in the level triggered
>    case, and warn if we get spurious interrupts.
> - Rename INV_MPU6050_LEVEL_TRIGGERED to INV_MPU6050_LATCH_INT_EN to match
>    datasheet naming.
> - Write st->irq_mask prior to device power off to make sure it is fully set.
> - Write st->irq_mask instead of 0 in inv_mpu6050_deselect_bypass.
> - Make irq_type local instead of part of the driver state, as we use it only at
>    probe time and never again.
> - Remove the comment about bus lockups, as I have determined them to be
>    unrelated.
> - Add missing documentation for irq_type and irq_mask.
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> index 7d64be353403..b711e6260d9a 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,
> @@ -278,6 +280,10 @@ static int inv_mpu6050_init_config(struct iio_dev *indio_dev)
>   	if (result)
>   		return result;
> 
> +	result = regmap_write(st->map, st->reg->int_pin_cfg, st->irq_mask);
> +	if (result)
> +		return result;
> +
>   	memcpy(&st->chip_config, hw_info[st->chip_type].config,
>   	       sizeof(struct inv_mpu6050_chip_config));
>   	result = inv_mpu6050_set_power_itg(st, false);
> @@ -882,6 +888,8 @@ 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;
> +	int irq_type;
> 
>   	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
>   	if (!indio_dev)
> @@ -913,6 +921,29 @@ 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;
I would prefer -ENODEV instead. There is nothing busy as far as I 
understand.

> +	}
> +
> +	irq_type = irqd_get_trigger_type(desc);
> +	if (irq_type == IRQF_TRIGGER_RISING)
> +		st->irq_mask = INV_MPU6050_ACTIVE_HIGH;
> +	else if (irq_type == IRQF_TRIGGER_FALLING)
> +		st->irq_mask = INV_MPU6050_ACTIVE_LOW;
> +	else if (irq_type == IRQF_TRIGGER_HIGH)
> +		st->irq_mask = INV_MPU6050_ACTIVE_HIGH |
> +			INV_MPU6050_LATCH_INT_EN;
> +	else if (irq_type == IRQF_TRIGGER_LOW)
> +		st->irq_mask = INV_MPU6050_ACTIVE_LOW |
> +			INV_MPU6050_LATCH_INT_EN;
> +	else {
> +		dev_err(dev, "Invalid interrupt type 0x%x specified\n",
> +			irq_type);
> +		return -EBUSY;
Same here, -ENODEV makes more sense if I'm understanding well.

> +	}
> +
>   	/* power is turned on inside check chip type*/
>   	result = inv_check_and_setup_chip(st);
>   	if (result)
> @@ -948,7 +979,7 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
>   		dev_err(dev, "configure buffer fail %d\n", result);
>   		return result;
>   	}
> -	result = inv_mpu6050_probe_trigger(indio_dev);
> +	result = inv_mpu6050_probe_trigger(indio_dev, irq_type);
>   	if (result) {
>   		dev_err(dev, "trigger probe fail %d\n", result);
>   		goto out_unreg_ring;
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> index fcd7a92b6cf8..1b02d2b69174 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, st->irq_mask);
>   	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..064e3b28fdb0 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;
> @@ -125,6 +127,7 @@ struct inv_mpu6050_hw {
>    *  @timestamps:        kfifo queue to store time stamp.
>    *  @map		regmap pointer.
>    *  @irq		interrupt number.
> + *  @irq_mask		the int_pin_cfg mask to configure interrupt type
>    */
>   struct inv_mpu6050_state {
>   #define TIMESTAMP_FIFO_SIZE 16
> @@ -143,6 +146,7 @@ struct inv_mpu6050_state {
>   	DECLARE_KFIFO(timestamps, long long, TIMESTAMP_FIFO_SIZE);
>   	struct regmap *map;
>   	int irq;
> +	u16 irq_mask;
It is data for a 8 bits register. u8 should be sufficient.

>   };
> 
>   /*register and associated bit definition*/
> @@ -159,6 +163,8 @@ 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_RAW_DATA_RDY_INT    0x01
>   #define INV_MPU6050_BIT_DATA_RDY_EN         0x01
>   #define INV_MPU6050_BIT_DMP_INT_EN          0x02
Beware you are mixing register and bit defines here. Better put the new 
definition below and let the interrupt enable BIT defines just below the 
corresponding REG define.

> 
> @@ -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
> +/* enable level triggering */
> +#define INV_MPU6050_LATCH_INT_EN            0x20
Would be better to have this amoung the bit defines of register 
INT_PIN_CFG. I would prefer just to add these defines below the 
REG_INT_PIN_CFG:
#define INV_MPU6050_BIT_ACTIVE_LOW		0x80
#define INV_MPU6050_BIT_LATCH_INT_EN		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
> @@ -287,7 +297,7 @@ enum inv_mpu6050_clock_sel_e {
> 
>   irqreturn_t inv_mpu6050_irq_handler(int irq, void *p);
>   irqreturn_t inv_mpu6050_read_fifo(int irq, void *p);
> -int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev);
> +int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev, int irq_type);
>   void inv_mpu6050_remove_trigger(struct inv_mpu6050_state *st);
>   int inv_reset_fifo(struct iio_dev *indio_dev);
>   int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en, u32 mask);
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> index ff81c6aa009d..8f1f637fb972 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> @@ -127,8 +127,21 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
>   	u8 data[INV_MPU6050_OUTPUT_DATA_SIZE];
>   	u16 fifo_count;
>   	s64 timestamp;
> +	int int_status;
> 
>   	mutex_lock(&st->lock);
> +
> +	/* ack interrupt and check status */
> +	result = regmap_read(st->map, st->reg->int_status, &int_status);
> +	if (result)
> +		dev_err(regmap_get_device(st->map),
If we cannot read int status, I would exit with error using flush_fifo 
error path. This would ensure the interrupt would be resetted.

> +			"failed to ack interrupt\n");
> +	if (!(int_status & INV_MPU6050_BIT_RAW_DATA_RDY_INT)) {
> +		dev_warn(regmap_get_device(st->map),
> +			"spurious interrupt with status 0x%x\n", int_status);
> +		goto end_session;
> +	}
> +
>   	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..b8c5584e4252 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> @@ -117,7 +117,7 @@ static const struct iio_trigger_ops inv_mpu_trigger_ops = {
>   	.set_trigger_state = &inv_mpu_data_rdy_trigger_set_state,
>   };
> 
> -int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev)
> +int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev, int irq_type)
>   {
>   	int ret;
>   	struct inv_mpu6050_state *st = iio_priv(indio_dev);
> @@ -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,
> +			       irq_type,
>   			       "inv_mpu",
>   			       st->trig);
>   	if (ret)
> --
> 2.11.0
> 

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

[Martin Kelly]

Thanks, replies inline. I made all the changes you mentioned except the 
one about -EBUSY. Let me know what you think regarding -EBUSY/-ENODEV 
and then I'll send a new revision with all the changes included.

On 04/10/2018 02:06 AM, Jean-Baptiste Maneyrol wrote:
> Hello,
> 
> some more concerns after a deeper look.
> 
> Thanks.
> JB
> 
> On 09/04/2018 22:21, 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. 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    | 33 
>> ++++++++++++++++++++++++++-
>>   drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c     |  5 ++--
>>   drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h     | 14 ++++++++++--
>>   drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c    | 13 +++++++++++
>>   drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c |  4 ++--
>>   5 files changed, 61 insertions(+), 8 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.
>> v5:
>> - Check interrupt status in all cases rather than only in the level 
>> triggered
>>    case, and warn if we get spurious interrupts.
>> - Rename INV_MPU6050_LEVEL_TRIGGERED to INV_MPU6050_LATCH_INT_EN to match
>>    datasheet naming.
>> - Write st->irq_mask prior to device power off to make sure it is 
>> fully set.
>> - Write st->irq_mask instead of 0 in inv_mpu6050_deselect_bypass.
>> - Make irq_type local instead of part of the driver state, as we use 
>> it only at
>>    probe time and never again.
>> - Remove the comment about bus lockups, as I have determined them to be
>>    unrelated.
>> - Add missing documentation for irq_type and irq_mask.
>>
>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c 
>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>> index 7d64be353403..b711e6260d9a 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,
>> @@ -278,6 +280,10 @@ static int inv_mpu6050_init_config(struct iio_dev 
>> *indio_dev)
>>       if (result)
>>           return result;
>>
>> +    result = regmap_write(st->map, st->reg->int_pin_cfg, st->irq_mask);
>> +    if (result)
>> +        return result;
>> +
>>       memcpy(&st->chip_config, hw_info[st->chip_type].config,
>>              sizeof(struct inv_mpu6050_chip_config));
>>       result = inv_mpu6050_set_power_itg(st, false);
>> @@ -882,6 +888,8 @@ 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;
>> +    int irq_type;
>>
>>       indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
>>       if (!indio_dev)
>> @@ -913,6 +921,29 @@ 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;
> I would prefer -ENODEV instead. There is nothing busy as far as I 
> understand.
> 

I picked -EBUSY based on guidance from this thread:

https://lists.gt.net/linux/kernel/1010603

akpm seemed happy with this treatment of -EBUSY and -ENODEV, but I can't 
see whether or not this guidance was formalized.

Please let me know which error code is more appropriate, and I'll change it.

>> +    }
>> +
>> +    irq_type = irqd_get_trigger_type(desc);
>> +    if (irq_type == IRQF_TRIGGER_RISING)
>> +        st->irq_mask = INV_MPU6050_ACTIVE_HIGH;
>> +    else if (irq_type == IRQF_TRIGGER_FALLING)
>> +        st->irq_mask = INV_MPU6050_ACTIVE_LOW;
>> +    else if (irq_type == IRQF_TRIGGER_HIGH)
>> +        st->irq_mask = INV_MPU6050_ACTIVE_HIGH |
>> +            INV_MPU6050_LATCH_INT_EN;
>> +    else if (irq_type == IRQF_TRIGGER_LOW)
>> +        st->irq_mask = INV_MPU6050_ACTIVE_LOW |
>> +            INV_MPU6050_LATCH_INT_EN;
>> +    else {
>> +        dev_err(dev, "Invalid interrupt type 0x%x specified\n",
>> +            irq_type);
>> +        return -EBUSY;
> Same here, -ENODEV makes more sense if I'm understanding well.
> 
>> +    }
>> +
>>       /* power is turned on inside check chip type*/
>>       result = inv_check_and_setup_chip(st);
>>       if (result)
>> @@ -948,7 +979,7 @@ int inv_mpu_core_probe(struct regmap *regmap, int 
>> irq, const char *name,
>>           dev_err(dev, "configure buffer fail %d\n", result);
>>           return result;
>>       }
>> -    result = inv_mpu6050_probe_trigger(indio_dev);
>> +    result = inv_mpu6050_probe_trigger(indio_dev, irq_type);
>>       if (result) {
>>           dev_err(dev, "trigger probe fail %d\n", result);
>>           goto out_unreg_ring;
>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c 
>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
>> index fcd7a92b6cf8..1b02d2b69174 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, st->irq_mask);
>>       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..064e3b28fdb0 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;
>> @@ -125,6 +127,7 @@ struct inv_mpu6050_hw {
>>    *  @timestamps:        kfifo queue to store time stamp.
>>    *  @map        regmap pointer.
>>    *  @irq        interrupt number.
>> + *  @irq_mask        the int_pin_cfg mask to configure interrupt type
>>    */
>>   struct inv_mpu6050_state {
>>   #define TIMESTAMP_FIFO_SIZE 16
>> @@ -143,6 +146,7 @@ struct inv_mpu6050_state {
>>       DECLARE_KFIFO(timestamps, long long, TIMESTAMP_FIFO_SIZE);
>>       struct regmap *map;
>>       int irq;
>> +    u16 irq_mask;
> It is data for a 8 bits register. u8 should be sufficient.
> 

Good catch.

>>   };
>>
>>   /*register and associated bit definition*/
>> @@ -159,6 +163,8 @@ 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_RAW_DATA_RDY_INT    0x01
>>   #define INV_MPU6050_BIT_DATA_RDY_EN         0x01
>>   #define INV_MPU6050_BIT_DMP_INT_EN          0x02
> Beware you are mixing register and bit defines here. Better put the new 
> definition below and let the interrupt enable BIT defines just below the 
> corresponding REG define.
> 
>>
>> @@ -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
>> +/* enable level triggering */
>> +#define INV_MPU6050_LATCH_INT_EN            0x20
> Would be better to have this amoung the bit defines of register 
> INT_PIN_CFG. I would prefer just to add these defines below the 
> REG_INT_PIN_CFG:
> #define INV_MPU6050_BIT_ACTIVE_LOW        0x80
> #define INV_MPU6050_BIT_LATCH_INT_EN        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
>> @@ -287,7 +297,7 @@ enum inv_mpu6050_clock_sel_e {
>>
>>   irqreturn_t inv_mpu6050_irq_handler(int irq, void *p);
>>   irqreturn_t inv_mpu6050_read_fifo(int irq, void *p);
>> -int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev);
>> +int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev, int irq_type);
>>   void inv_mpu6050_remove_trigger(struct inv_mpu6050_state *st);
>>   int inv_reset_fifo(struct iio_dev *indio_dev);
>>   int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en, 
>> u32 mask);
>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c 
>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
>> index ff81c6aa009d..8f1f637fb972 100644
>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
>> @@ -127,8 +127,21 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
>>       u8 data[INV_MPU6050_OUTPUT_DATA_SIZE];
>>       u16 fifo_count;
>>       s64 timestamp;
>> +    int int_status;
>>
>>       mutex_lock(&st->lock);
>> +
>> +    /* ack interrupt and check status */
>> +    result = regmap_read(st->map, st->reg->int_status, &int_status);
>> +    if (result)
>> +        dev_err(regmap_get_device(st->map),
> If we cannot read int status, I would exit with error using flush_fifo 
> error path. This would ensure the interrupt would be resetted.
> 

I will do that. Depending on the error cause, resetting the FIFO may not 
work either (for instance, if you get bus lockup, all I2C transactions 
will fail). But it's at least worth a shot as it will solve some error 
cases.

>> +            "failed to ack interrupt\n");
>> +    if (!(int_status & INV_MPU6050_BIT_RAW_DATA_RDY_INT)) {
>> +        dev_warn(regmap_get_device(st->map),
>> +            "spurious interrupt with status 0x%x\n", int_status);
>> +        goto end_session;
>> +    }
>> +
>>       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..b8c5584e4252 100644
>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
>> @@ -117,7 +117,7 @@ static const struct iio_trigger_ops 
>> inv_mpu_trigger_ops = {
>>       .set_trigger_state = &inv_mpu_data_rdy_trigger_set_state,
>>   };
>>
>> -int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev)
>> +int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev, int irq_type)
>>   {
>>       int ret;
>>       struct inv_mpu6050_state *st = iio_priv(indio_dev);
>> @@ -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,
>> +                   irq_type,
>>                      "inv_mpu",
>>                      st->trig);
>>       if (ret)
>> -- 
>> 2.11.0
>>

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

[Jean-Baptiste Maneyrol]

This is OK for me.

Jonathan will tell us about EBUSY error code for sure if it is not correct.

JB

On 10/04/2018 20:08, Martin Kelly wrote:
> Thanks, replies inline. I made all the changes you mentioned except the 
> one about -EBUSY. Let me know what you think regarding -EBUSY/-ENODEV 
> and then I'll send a new revision with all the changes included.
> 
> On 04/10/2018 02:06 AM, Jean-Baptiste Maneyrol wrote:
>> Hello,
>>
>> some more concerns after a deeper look.
>>
>> Thanks.
>> JB
>>
>> On 09/04/2018 22:21, 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. 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    | 33 
>>> ++++++++++++++++++++++++++-
>>>   drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c     |  5 ++--
>>>   drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h     | 14 ++++++++++--
>>>   drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c    | 13 +++++++++++
>>>   drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c |  4 ++--
>>>   5 files changed, 61 insertions(+), 8 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.
>>> v5:
>>> - Check interrupt status in all cases rather than only in the level 
>>> triggered
>>>    case, and warn if we get spurious interrupts.
>>> - Rename INV_MPU6050_LEVEL_TRIGGERED to INV_MPU6050_LATCH_INT_EN to 
>>> match
>>>    datasheet naming.
>>> - Write st->irq_mask prior to device power off to make sure it is 
>>> fully set.
>>> - Write st->irq_mask instead of 0 in inv_mpu6050_deselect_bypass.
>>> - Make irq_type local instead of part of the driver state, as we use 
>>> it only at
>>>    probe time and never again.
>>> - Remove the comment about bus lockups, as I have determined them to be
>>>    unrelated.
>>> - Add missing documentation for irq_type and irq_mask.
>>>
>>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c 
>>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>>> index 7d64be353403..b711e6260d9a 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,
>>> @@ -278,6 +280,10 @@ static int inv_mpu6050_init_config(struct 
>>> iio_dev *indio_dev)
>>>       if (result)
>>>           return result;
>>>
>>> +    result = regmap_write(st->map, st->reg->int_pin_cfg, st->irq_mask);
>>> +    if (result)
>>> +        return result;
>>> +
>>>       memcpy(&st->chip_config, hw_info[st->chip_type].config,
>>>              sizeof(struct inv_mpu6050_chip_config));
>>>       result = inv_mpu6050_set_power_itg(st, false);
>>> @@ -882,6 +888,8 @@ 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;
>>> +    int irq_type;
>>>
>>>       indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
>>>       if (!indio_dev)
>>> @@ -913,6 +921,29 @@ 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;
>> I would prefer -ENODEV instead. There is nothing busy as far as I 
>> understand.
>>
> 
> I picked -EBUSY based on guidance from this thread:
> 
> https://lists.gt.net/linux/kernel/1010603
> 
> akpm seemed happy with this treatment of -EBUSY and -ENODEV, but I can't 
> see whether or not this guidance was formalized.
> 
> Please let me know which error code is more appropriate, and I'll change 
> it.
> 
>>> +    }
>>> +
>>> +    irq_type = irqd_get_trigger_type(desc);
>>> +    if (irq_type == IRQF_TRIGGER_RISING)
>>> +        st->irq_mask = INV_MPU6050_ACTIVE_HIGH;
>>> +    else if (irq_type == IRQF_TRIGGER_FALLING)
>>> +        st->irq_mask = INV_MPU6050_ACTIVE_LOW;
>>> +    else if (irq_type == IRQF_TRIGGER_HIGH)
>>> +        st->irq_mask = INV_MPU6050_ACTIVE_HIGH |
>>> +            INV_MPU6050_LATCH_INT_EN;
>>> +    else if (irq_type == IRQF_TRIGGER_LOW)
>>> +        st->irq_mask = INV_MPU6050_ACTIVE_LOW |
>>> +            INV_MPU6050_LATCH_INT_EN;
>>> +    else {
>>> +        dev_err(dev, "Invalid interrupt type 0x%x specified\n",
>>> +            irq_type);
>>> +        return -EBUSY;
>> Same here, -ENODEV makes more sense if I'm understanding well.
>>
>>> +    }
>>> +
>>>       /* power is turned on inside check chip type*/
>>>       result = inv_check_and_setup_chip(st);
>>>       if (result)
>>> @@ -948,7 +979,7 @@ int inv_mpu_core_probe(struct regmap *regmap, int 
>>> irq, const char *name,
>>>           dev_err(dev, "configure buffer fail %d\n", result);
>>>           return result;
>>>       }
>>> -    result = inv_mpu6050_probe_trigger(indio_dev);
>>> +    result = inv_mpu6050_probe_trigger(indio_dev, irq_type);
>>>       if (result) {
>>>           dev_err(dev, "trigger probe fail %d\n", result);
>>>           goto out_unreg_ring;
>>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c 
>>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
>>> index fcd7a92b6cf8..1b02d2b69174 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, st->irq_mask);
>>>       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..064e3b28fdb0 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;
>>> @@ -125,6 +127,7 @@ struct inv_mpu6050_hw {
>>>    *  @timestamps:        kfifo queue to store time stamp.
>>>    *  @map        regmap pointer.
>>>    *  @irq        interrupt number.
>>> + *  @irq_mask        the int_pin_cfg mask to configure interrupt type
>>>    */
>>>   struct inv_mpu6050_state {
>>>   #define TIMESTAMP_FIFO_SIZE 16
>>> @@ -143,6 +146,7 @@ struct inv_mpu6050_state {
>>>       DECLARE_KFIFO(timestamps, long long, TIMESTAMP_FIFO_SIZE);
>>>       struct regmap *map;
>>>       int irq;
>>> +    u16 irq_mask;
>> It is data for a 8 bits register. u8 should be sufficient.
>>
> 
> Good catch.
> 
>>>   };
>>>
>>>   /*register and associated bit definition*/
>>> @@ -159,6 +163,8 @@ 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_RAW_DATA_RDY_INT    0x01
>>>   #define INV_MPU6050_BIT_DATA_RDY_EN         0x01
>>>   #define INV_MPU6050_BIT_DMP_INT_EN          0x02
>> Beware you are mixing register and bit defines here. Better put the 
>> new definition below and let the interrupt enable BIT defines just 
>> below the corresponding REG define.
>>
>>>
>>> @@ -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
>>> +/* enable level triggering */
>>> +#define INV_MPU6050_LATCH_INT_EN            0x20
>> Would be better to have this amoung the bit defines of register 
>> INT_PIN_CFG. I would prefer just to add these defines below the 
>> REG_INT_PIN_CFG:
>> #define INV_MPU6050_BIT_ACTIVE_LOW        0x80
>> #define INV_MPU6050_BIT_LATCH_INT_EN        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
>>> @@ -287,7 +297,7 @@ enum inv_mpu6050_clock_sel_e {
>>>
>>>   irqreturn_t inv_mpu6050_irq_handler(int irq, void *p);
>>>   irqreturn_t inv_mpu6050_read_fifo(int irq, void *p);
>>> -int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev);
>>> +int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev, int irq_type);
>>>   void inv_mpu6050_remove_trigger(struct inv_mpu6050_state *st);
>>>   int inv_reset_fifo(struct iio_dev *indio_dev);
>>>   int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool 
>>> en, u32 mask);
>>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c 
>>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
>>> index ff81c6aa009d..8f1f637fb972 100644
>>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
>>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
>>> @@ -127,8 +127,21 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
>>>       u8 data[INV_MPU6050_OUTPUT_DATA_SIZE];
>>>       u16 fifo_count;
>>>       s64 timestamp;
>>> +    int int_status;
>>>
>>>       mutex_lock(&st->lock);
>>> +
>>> +    /* ack interrupt and check status */
>>> +    result = regmap_read(st->map, st->reg->int_status, &int_status);
>>> +    if (result)
>>> +        dev_err(regmap_get_device(st->map),
>> If we cannot read int status, I would exit with error using flush_fifo 
>> error path. This would ensure the interrupt would be resetted.
>>
> 
> I will do that. Depending on the error cause, resetting the FIFO may not 
> work either (for instance, if you get bus lockup, all I2C transactions 
> will fail). But it's at least worth a shot as it will solve some error 
> cases.
> 
>>> +            "failed to ack interrupt\n");
>>> +    if (!(int_status & INV_MPU6050_BIT_RAW_DATA_RDY_INT)) {
>>> +        dev_warn(regmap_get_device(st->map),
>>> +            "spurious interrupt with status 0x%x\n", int_status);
>>> +        goto end_session;
>>> +    }
>>> +
>>>       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..b8c5584e4252 100644
>>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
>>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
>>> @@ -117,7 +117,7 @@ static const struct iio_trigger_ops 
>>> inv_mpu_trigger_ops = {
>>>       .set_trigger_state = &inv_mpu_data_rdy_trigger_set_state,
>>>   };
>>>
>>> -int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev)
>>> +int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev, int irq_type)
>>>   {
>>>       int ret;
>>>       struct inv_mpu6050_state *st = iio_priv(indio_dev);
>>> @@ -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,
>>> +                   irq_type,
>>>                      "inv_mpu",
>>>                      st->trig);
>>>       if (ret)
>>> -- 
>>> 2.11.0
>>>

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

[Martin Kelly]

On 04/11/2018 12:01 AM, Jean-Baptiste Maneyrol wrote:
> This is OK for me.
> 
> Jonathan will tell us about EBUSY error code for sure if it is not correct.
> 
> JB
> 

Sounds good; once we hear from Jonathan, I will submit the next revision.

> On 10/04/2018 20:08, Martin Kelly wrote:
>> Thanks, replies inline. I made all the changes you mentioned except 
>> the one about -EBUSY. Let me know what you think regarding 
>> -EBUSY/-ENODEV and then I'll send a new revision with all the changes 
>> included.
>>
>> On 04/10/2018 02:06 AM, Jean-Baptiste Maneyrol wrote:
>>> Hello,
>>>
>>> some more concerns after a deeper look.
>>>
>>> Thanks.
>>> JB
>>>
>>> On 09/04/2018 22:21, 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. 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    | 33 
>>>> ++++++++++++++++++++++++++-
>>>>   drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c     |  5 ++--
>>>>   drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h     | 14 ++++++++++--
>>>>   drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c    | 13 +++++++++++
>>>>   drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c |  4 ++--
>>>>   5 files changed, 61 insertions(+), 8 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.
>>>> v5:
>>>> - Check interrupt status in all cases rather than only in the level 
>>>> triggered
>>>>    case, and warn if we get spurious interrupts.
>>>> - Rename INV_MPU6050_LEVEL_TRIGGERED to INV_MPU6050_LATCH_INT_EN to 
>>>> match
>>>>    datasheet naming.
>>>> - Write st->irq_mask prior to device power off to make sure it is 
>>>> fully set.
>>>> - Write st->irq_mask instead of 0 in inv_mpu6050_deselect_bypass.
>>>> - Make irq_type local instead of part of the driver state, as we use 
>>>> it only at
>>>>    probe time and never again.
>>>> - Remove the comment about bus lockups, as I have determined them to be
>>>>    unrelated.
>>>> - Add missing documentation for irq_type and irq_mask.
>>>>
>>>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c 
>>>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>>>> index 7d64be353403..b711e6260d9a 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,
>>>> @@ -278,6 +280,10 @@ static int inv_mpu6050_init_config(struct 
>>>> iio_dev *indio_dev)
>>>>       if (result)
>>>>           return result;
>>>>
>>>> +    result = regmap_write(st->map, st->reg->int_pin_cfg, 
>>>> st->irq_mask);
>>>> +    if (result)
>>>> +        return result;
>>>> +
>>>>       memcpy(&st->chip_config, hw_info[st->chip_type].config,
>>>>              sizeof(struct inv_mpu6050_chip_config));
>>>>       result = inv_mpu6050_set_power_itg(st, false);
>>>> @@ -882,6 +888,8 @@ 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;
>>>> +    int irq_type;
>>>>
>>>>       indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
>>>>       if (!indio_dev)
>>>> @@ -913,6 +921,29 @@ 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;
>>> I would prefer -ENODEV instead. There is nothing busy as far as I 
>>> understand.
>>>
>>
>> I picked -EBUSY based on guidance from this thread:
>>
>> https://lists.gt.net/linux/kernel/1010603
>>
>> akpm seemed happy with this treatment of -EBUSY and -ENODEV, but I 
>> can't see whether or not this guidance was formalized.
>>
>> Please let me know which error code is more appropriate, and I'll 
>> change it.
>>
>>>> +    }
>>>> +
>>>> +    irq_type = irqd_get_trigger_type(desc);
>>>> +    if (irq_type == IRQF_TRIGGER_RISING)
>>>> +        st->irq_mask = INV_MPU6050_ACTIVE_HIGH;
>>>> +    else if (irq_type == IRQF_TRIGGER_FALLING)
>>>> +        st->irq_mask = INV_MPU6050_ACTIVE_LOW;
>>>> +    else if (irq_type == IRQF_TRIGGER_HIGH)
>>>> +        st->irq_mask = INV_MPU6050_ACTIVE_HIGH |
>>>> +            INV_MPU6050_LATCH_INT_EN;
>>>> +    else if (irq_type == IRQF_TRIGGER_LOW)
>>>> +        st->irq_mask = INV_MPU6050_ACTIVE_LOW |
>>>> +            INV_MPU6050_LATCH_INT_EN;
>>>> +    else {
>>>> +        dev_err(dev, "Invalid interrupt type 0x%x specified\n",
>>>> +            irq_type);
>>>> +        return -EBUSY;
>>> Same here, -ENODEV makes more sense if I'm understanding well.
>>>
>>>> +    }
>>>> +
>>>>       /* power is turned on inside check chip type*/
>>>>       result = inv_check_and_setup_chip(st);
>>>>       if (result)
>>>> @@ -948,7 +979,7 @@ int inv_mpu_core_probe(struct regmap *regmap, 
>>>> int irq, const char *name,
>>>>           dev_err(dev, "configure buffer fail %d\n", result);
>>>>           return result;
>>>>       }
>>>> -    result = inv_mpu6050_probe_trigger(indio_dev);
>>>> +    result = inv_mpu6050_probe_trigger(indio_dev, irq_type);
>>>>       if (result) {
>>>>           dev_err(dev, "trigger probe fail %d\n", result);
>>>>           goto out_unreg_ring;
>>>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c 
>>>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
>>>> index fcd7a92b6cf8..1b02d2b69174 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, st->irq_mask);
>>>>       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..064e3b28fdb0 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;
>>>> @@ -125,6 +127,7 @@ struct inv_mpu6050_hw {
>>>>    *  @timestamps:        kfifo queue to store time stamp.
>>>>    *  @map        regmap pointer.
>>>>    *  @irq        interrupt number.
>>>> + *  @irq_mask        the int_pin_cfg mask to configure interrupt type
>>>>    */
>>>>   struct inv_mpu6050_state {
>>>>   #define TIMESTAMP_FIFO_SIZE 16
>>>> @@ -143,6 +146,7 @@ struct inv_mpu6050_state {
>>>>       DECLARE_KFIFO(timestamps, long long, TIMESTAMP_FIFO_SIZE);
>>>>       struct regmap *map;
>>>>       int irq;
>>>> +    u16 irq_mask;
>>> It is data for a 8 bits register. u8 should be sufficient.
>>>
>>
>> Good catch.
>>
>>>>   };
>>>>
>>>>   /*register and associated bit definition*/
>>>> @@ -159,6 +163,8 @@ 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_RAW_DATA_RDY_INT    0x01
>>>>   #define INV_MPU6050_BIT_DATA_RDY_EN         0x01
>>>>   #define INV_MPU6050_BIT_DMP_INT_EN          0x02
>>> Beware you are mixing register and bit defines here. Better put the 
>>> new definition below and let the interrupt enable BIT defines just 
>>> below the corresponding REG define.
>>>
>>>>
>>>> @@ -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
>>>> +/* enable level triggering */
>>>> +#define INV_MPU6050_LATCH_INT_EN            0x20
>>> Would be better to have this amoung the bit defines of register 
>>> INT_PIN_CFG. I would prefer just to add these defines below the 
>>> REG_INT_PIN_CFG:
>>> #define INV_MPU6050_BIT_ACTIVE_LOW        0x80
>>> #define INV_MPU6050_BIT_LATCH_INT_EN        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
>>>> @@ -287,7 +297,7 @@ enum inv_mpu6050_clock_sel_e {
>>>>
>>>>   irqreturn_t inv_mpu6050_irq_handler(int irq, void *p);
>>>>   irqreturn_t inv_mpu6050_read_fifo(int irq, void *p);
>>>> -int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev);
>>>> +int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev, int 
>>>> irq_type);
>>>>   void inv_mpu6050_remove_trigger(struct inv_mpu6050_state *st);
>>>>   int inv_reset_fifo(struct iio_dev *indio_dev);
>>>>   int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool 
>>>> en, u32 mask);
>>>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c 
>>>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
>>>> index ff81c6aa009d..8f1f637fb972 100644
>>>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
>>>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
>>>> @@ -127,8 +127,21 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void 
>>>> *p)
>>>>       u8 data[INV_MPU6050_OUTPUT_DATA_SIZE];
>>>>       u16 fifo_count;
>>>>       s64 timestamp;
>>>> +    int int_status;
>>>>
>>>>       mutex_lock(&st->lock);
>>>> +
>>>> +    /* ack interrupt and check status */
>>>> +    result = regmap_read(st->map, st->reg->int_status, &int_status);
>>>> +    if (result)
>>>> +        dev_err(regmap_get_device(st->map),
>>> If we cannot read int status, I would exit with error using 
>>> flush_fifo error path. This would ensure the interrupt would be 
>>> resetted.
>>>
>>
>> I will do that. Depending on the error cause, resetting the FIFO may 
>> not work either (for instance, if you get bus lockup, all I2C 
>> transactions will fail). But it's at least worth a shot as it will 
>> solve some error cases.
>>
>>>> +            "failed to ack interrupt\n");
>>>> +    if (!(int_status & INV_MPU6050_BIT_RAW_DATA_RDY_INT)) {
>>>> +        dev_warn(regmap_get_device(st->map),
>>>> +            "spurious interrupt with status 0x%x\n", int_status);
>>>> +        goto end_session;
>>>> +    }
>>>> +
>>>>       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..b8c5584e4252 100644
>>>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
>>>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
>>>> @@ -117,7 +117,7 @@ static const struct iio_trigger_ops 
>>>> inv_mpu_trigger_ops = {
>>>>       .set_trigger_state = &inv_mpu_data_rdy_trigger_set_state,
>>>>   };
>>>>
>>>> -int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev)
>>>> +int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev, int irq_type)
>>>>   {
>>>>       int ret;
>>>>       struct inv_mpu6050_state *st = iio_priv(indio_dev);
>>>> @@ -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,
>>>> +                   irq_type,
>>>>                      "inv_mpu",
>>>>                      st->trig);
>>>>       if (ret)
>>>> -- 
>>>> 2.11.0
>>>>

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

[Jean-Baptiste Maneyrol]

Hello,

I've done further investigations on this issue, since I wasn't 
understanding why we were loosing interrupts where we shouldn't. And I 
think I found the root cause.

The irq handlers top and bottom are written in a way considering that 
every interrupt will be handled by the top part in the hard irq handler, 
even if the irq thread is still running in the bottom part. That's why 
we are using a FIFO for storing multiple timestamps.

But by using triggered-buffer setup, we are using the ONESHOT irq mode 
that is disabling irq until the thread processing is done. In this way, 
we are never reliably catching data interrupts when the thread is 
running, and there is no need to have a FIFO for storing timestamps.

The correct way would be to have the top part in the hard irq handler 
without using ONESHOT mode, and use only the thread for reading FIFO 
data in the triggered-buffer interrupt.

I have done a patch which is working correctly on my side. But since I 
am not able to trigger the issue in the first time, I cannot guarantee 
this is fixing the problem.

Martin,
is it possible for you to test the following patch and tell me if it is 
working better like that?

For sure, there is a conception issue here, because in the current setup 
having a FIFO for storing timestamps is completely useless.

JB

Subject: [PATCH] iio: imu: inv_mpu6050: use top irq handler in the hard irq
  handler

Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
---
  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c    | 2 +-
  drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c    | 6 +++---
  drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c | 2 +-
  3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c 
b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index 7d64be3..ed883ef 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -941,7 +941,7 @@ int inv_mpu_core_probe(struct regmap *regmap, int 
irq, const char *name,
  	indio_dev->modes = INDIO_BUFFER_TRIGGERED;

  	result = iio_triggered_buffer_setup(indio_dev,
-					    inv_mpu6050_irq_handler,
+					    NULL,
  					    inv_mpu6050_read_fifo,
  					    NULL);
  	if (result) {
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c 
b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
index ff81c6a..6935815 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
@@ -102,8 +102,8 @@ int inv_reset_fifo(struct iio_dev *indio_dev)
   */
  irqreturn_t inv_mpu6050_irq_handler(int irq, void *p)
  {
-	struct iio_poll_func *pf = p;
-	struct iio_dev *indio_dev = pf->indio_dev;
+	struct iio_trigger *trig = p;
+	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
  	struct inv_mpu6050_state *st = iio_priv(indio_dev);
  	s64 timestamp;

@@ -111,7 +111,7 @@ irqreturn_t inv_mpu6050_irq_handler(int irq, void *p)
  	kfifo_in_spinlocked(&st->timestamps, &timestamp, 1,
  			    &st->time_stamp_lock);

-	return IRQ_WAKE_THREAD;
+	return iio_trigger_generic_data_rdy_poll(irq, trig);
  }

  /**
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c 
b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
index f963f9f..1cb9ed7 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
@@ -130,7 +130,7 @@ int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev)
  		return -ENOMEM;

  	ret = devm_request_irq(&indio_dev->dev, st->irq,
-			       &iio_trigger_generic_data_rdy_poll,
+			       &inv_mpu6050_irq_handler,
  			       IRQF_TRIGGER_RISING,
  			       "inv_mpu",
  			       st->trig);
-- 
2.7.4



On 11/04/2018 18:42, Martin Kelly wrote:
> On 04/11/2018 12:01 AM, Jean-Baptiste Maneyrol wrote:
>> This is OK for me.
>>
>> Jonathan will tell us about EBUSY error code for sure if it is not 
>> correct.
>>
>> JB
>>
> 
> Sounds good; once we hear from Jonathan, I will submit the next revision.
> 
>> On 10/04/2018 20:08, Martin Kelly wrote:
>>> Thanks, replies inline. I made all the changes you mentioned except 
>>> the one about -EBUSY. Let me know what you think regarding 
>>> -EBUSY/-ENODEV and then I'll send a new revision with all the changes 
>>> included.
>>>
>>> On 04/10/2018 02:06 AM, Jean-Baptiste Maneyrol wrote:
>>>> Hello,
>>>>
>>>> some more concerns after a deeper look.
>>>>
>>>> Thanks.
>>>> JB
>>>>
>>>> On 09/04/2018 22:21, 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. 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    | 33 
>>>>> ++++++++++++++++++++++++++-
>>>>>   drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c     |  5 ++--
>>>>>   drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h     | 14 ++++++++++--
>>>>>   drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c    | 13 +++++++++++
>>>>>   drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c |  4 ++--
>>>>>   5 files changed, 61 insertions(+), 8 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.
>>>>> v5:
>>>>> - Check interrupt status in all cases rather than only in the level 
>>>>> triggered
>>>>>    case, and warn if we get spurious interrupts.
>>>>> - Rename INV_MPU6050_LEVEL_TRIGGERED to INV_MPU6050_LATCH_INT_EN to 
>>>>> match
>>>>>    datasheet naming.
>>>>> - Write st->irq_mask prior to device power off to make sure it is 
>>>>> fully set.
>>>>> - Write st->irq_mask instead of 0 in inv_mpu6050_deselect_bypass.
>>>>> - Make irq_type local instead of part of the driver state, as we 
>>>>> use it only at
>>>>>    probe time and never again.
>>>>> - Remove the comment about bus lockups, as I have determined them 
>>>>> to be
>>>>>    unrelated.
>>>>> - Add missing documentation for irq_type and irq_mask.
>>>>>
>>>>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c 
>>>>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>>>>> index 7d64be353403..b711e6260d9a 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,
>>>>> @@ -278,6 +280,10 @@ static int inv_mpu6050_init_config(struct 
>>>>> iio_dev *indio_dev)
>>>>>       if (result)
>>>>>           return result;
>>>>>
>>>>> +    result = regmap_write(st->map, st->reg->int_pin_cfg, 
>>>>> st->irq_mask);
>>>>> +    if (result)
>>>>> +        return result;
>>>>> +
>>>>>       memcpy(&st->chip_config, hw_info[st->chip_type].config,
>>>>>              sizeof(struct inv_mpu6050_chip_config));
>>>>>       result = inv_mpu6050_set_power_itg(st, false);
>>>>> @@ -882,6 +888,8 @@ 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;
>>>>> +    int irq_type;
>>>>>
>>>>>       indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
>>>>>       if (!indio_dev)
>>>>> @@ -913,6 +921,29 @@ 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;
>>>> I would prefer -ENODEV instead. There is nothing busy as far as I 
>>>> understand.
>>>>
>>>
>>> I picked -EBUSY based on guidance from this thread:
>>>
>>> https://lists.gt.net/linux/kernel/1010603
>>>
>>> akpm seemed happy with this treatment of -EBUSY and -ENODEV, but I 
>>> can't see whether or not this guidance was formalized.
>>>
>>> Please let me know which error code is more appropriate, and I'll 
>>> change it.
>>>
>>>>> +    }
>>>>> +
>>>>> +    irq_type = irqd_get_trigger_type(desc);
>>>>> +    if (irq_type == IRQF_TRIGGER_RISING)
>>>>> +        st->irq_mask = INV_MPU6050_ACTIVE_HIGH;
>>>>> +    else if (irq_type == IRQF_TRIGGER_FALLING)
>>>>> +        st->irq_mask = INV_MPU6050_ACTIVE_LOW;
>>>>> +    else if (irq_type == IRQF_TRIGGER_HIGH)
>>>>> +        st->irq_mask = INV_MPU6050_ACTIVE_HIGH |
>>>>> +            INV_MPU6050_LATCH_INT_EN;
>>>>> +    else if (irq_type == IRQF_TRIGGER_LOW)
>>>>> +        st->irq_mask = INV_MPU6050_ACTIVE_LOW |
>>>>> +            INV_MPU6050_LATCH_INT_EN;
>>>>> +    else {
>>>>> +        dev_err(dev, "Invalid interrupt type 0x%x specified\n",
>>>>> +            irq_type);
>>>>> +        return -EBUSY;
>>>> Same here, -ENODEV makes more sense if I'm understanding well.
>>>>
>>>>> +    }
>>>>> +
>>>>>       /* power is turned on inside check chip type*/
>>>>>       result = inv_check_and_setup_chip(st);
>>>>>       if (result)
>>>>> @@ -948,7 +979,7 @@ int inv_mpu_core_probe(struct regmap *regmap, 
>>>>> int irq, const char *name,
>>>>>           dev_err(dev, "configure buffer fail %d\n", result);
>>>>>           return result;
>>>>>       }
>>>>> -    result = inv_mpu6050_probe_trigger(indio_dev);
>>>>> +    result = inv_mpu6050_probe_trigger(indio_dev, irq_type);
>>>>>       if (result) {
>>>>>           dev_err(dev, "trigger probe fail %d\n", result);
>>>>>           goto out_unreg_ring;
>>>>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c 
>>>>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
>>>>> index fcd7a92b6cf8..1b02d2b69174 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, st->irq_mask);
>>>>>       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..064e3b28fdb0 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;
>>>>> @@ -125,6 +127,7 @@ struct inv_mpu6050_hw {
>>>>>    *  @timestamps:        kfifo queue to store time stamp.
>>>>>    *  @map        regmap pointer.
>>>>>    *  @irq        interrupt number.
>>>>> + *  @irq_mask        the int_pin_cfg mask to configure interrupt type
>>>>>    */
>>>>>   struct inv_mpu6050_state {
>>>>>   #define TIMESTAMP_FIFO_SIZE 16
>>>>> @@ -143,6 +146,7 @@ struct inv_mpu6050_state {
>>>>>       DECLARE_KFIFO(timestamps, long long, TIMESTAMP_FIFO_SIZE);
>>>>>       struct regmap *map;
>>>>>       int irq;
>>>>> +    u16 irq_mask;
>>>> It is data for a 8 bits register. u8 should be sufficient.
>>>>
>>>
>>> Good catch.
>>>
>>>>>   };
>>>>>
>>>>>   /*register and associated bit definition*/
>>>>> @@ -159,6 +163,8 @@ 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_RAW_DATA_RDY_INT    0x01
>>>>>   #define INV_MPU6050_BIT_DATA_RDY_EN         0x01
>>>>>   #define INV_MPU6050_BIT_DMP_INT_EN          0x02
>>>> Beware you are mixing register and bit defines here. Better put the 
>>>> new definition below and let the interrupt enable BIT defines just 
>>>> below the corresponding REG define.
>>>>
>>>>>
>>>>> @@ -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
>>>>> +/* enable level triggering */
>>>>> +#define INV_MPU6050_LATCH_INT_EN            0x20
>>>> Would be better to have this amoung the bit defines of register 
>>>> INT_PIN_CFG. I would prefer just to add these defines below the 
>>>> REG_INT_PIN_CFG:
>>>> #define INV_MPU6050_BIT_ACTIVE_LOW        0x80
>>>> #define INV_MPU6050_BIT_LATCH_INT_EN        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
>>>>> @@ -287,7 +297,7 @@ enum inv_mpu6050_clock_sel_e {
>>>>>
>>>>>   irqreturn_t inv_mpu6050_irq_handler(int irq, void *p);
>>>>>   irqreturn_t inv_mpu6050_read_fifo(int irq, void *p);
>>>>> -int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev);
>>>>> +int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev, int 
>>>>> irq_type);
>>>>>   void inv_mpu6050_remove_trigger(struct inv_mpu6050_state *st);
>>>>>   int inv_reset_fifo(struct iio_dev *indio_dev);
>>>>>   int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool 
>>>>> en, u32 mask);
>>>>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c 
>>>>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
>>>>> index ff81c6aa009d..8f1f637fb972 100644
>>>>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
>>>>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
>>>>> @@ -127,8 +127,21 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, 
>>>>> void *p)
>>>>>       u8 data[INV_MPU6050_OUTPUT_DATA_SIZE];
>>>>>       u16 fifo_count;
>>>>>       s64 timestamp;
>>>>> +    int int_status;
>>>>>
>>>>>       mutex_lock(&st->lock);
>>>>> +
>>>>> +    /* ack interrupt and check status */
>>>>> +    result = regmap_read(st->map, st->reg->int_status, &int_status);
>>>>> +    if (result)
>>>>> +        dev_err(regmap_get_device(st->map),
>>>> If we cannot read int status, I would exit with error using 
>>>> flush_fifo error path. This would ensure the interrupt would be 
>>>> resetted.
>>>>
>>>
>>> I will do that. Depending on the error cause, resetting the FIFO may 
>>> not work either (for instance, if you get bus lockup, all I2C 
>>> transactions will fail). But it's at least worth a shot as it will 
>>> solve some error cases.
>>>
>>>>> +            "failed to ack interrupt\n");
>>>>> +    if (!(int_status & INV_MPU6050_BIT_RAW_DATA_RDY_INT)) {
>>>>> +        dev_warn(regmap_get_device(st->map),
>>>>> +            "spurious interrupt with status 0x%x\n", int_status);
>>>>> +        goto end_session;
>>>>> +    }
>>>>> +
>>>>>       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..b8c5584e4252 100644
>>>>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
>>>>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
>>>>> @@ -117,7 +117,7 @@ static const struct iio_trigger_ops 
>>>>> inv_mpu_trigger_ops = {
>>>>>       .set_trigger_state = &inv_mpu_data_rdy_trigger_set_state,
>>>>>   };
>>>>>
>>>>> -int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev)
>>>>> +int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev, int 
>>>>> irq_type)
>>>>>   {
>>>>>       int ret;
>>>>>       struct inv_mpu6050_state *st = iio_priv(indio_dev);
>>>>> @@ -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,
>>>>> +                   irq_type,
>>>>>                      "inv_mpu",
>>>>>                      st->trig);
>>>>>       if (ret)
>>>>> -- 
>>>>> 2.11.0
>>>>>

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

[Martin Kelly]

On 04/12/2018 08:01 AM, Jean-Baptiste Maneyrol wrote:
> Hello,
> 
> I've done further investigations on this issue, since I wasn't 
> understanding why we were loosing interrupts where we shouldn't. And I 
> think I found the root cause.
> 
> The irq handlers top and bottom are written in a way considering that 
> every interrupt will be handled by the top part in the hard irq handler, 
> even if the irq thread is still running in the bottom part. That's why 
> we are using a FIFO for storing multiple timestamps.
> 
> But by using triggered-buffer setup, we are using the ONESHOT irq mode 
> that is disabling irq until the thread processing is done. In this way, 
> we are never reliably catching data interrupts when the thread is 
> running, and there is no need to have a FIFO for storing timestamps.
> 
> The correct way would be to have the top part in the hard irq handler 
> without using ONESHOT mode, and use only the thread for reading FIFO 
> data in the triggered-buffer interrupt.
> 
> I have done a patch which is working correctly on my side. But since I 
> am not able to trigger the issue in the first time, I cannot guarantee 
> this is fixing the problem.
> 
> Martin,
> is it possible for you to test the following patch and tell me if it is 
> working better like that?
> 
> For sure, there is a conception issue here, because in the current setup 
> having a FIFO for storing timestamps is completely useless.
> 
> JB
> 

That's a good point, and sounds like a good change to me. Unfortunately 
it does not fix the issue I'm seeing, although I'm guessing it would 
improve it at high frequencies (though that's trickier to measure).

My issue is happening even at 10 Hz, in which the thread has ample time 
to complete before the next interrupt, so I think it's something related 
to the interrupt controller rather than this driver in particular.

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

[Jean-Baptiste Maneyrol]

Hello,

I am now able to reproduce the issue by generating kernel irq (just 
plug/unplug mouse or keyboard is sufficient).

My last modification doesn't solve anything, since event the hard irq 
handler is disabled when processing another interrupt. Having a latched 
interrupt and triggering by level is a workaround, but clearly not perfect.

I think we need to work out a new solution for timestamping correctly 
the data.

JB


On 12/04/2018 20:16, Martin Kelly wrote:
> On 04/12/2018 08:01 AM, Jean-Baptiste Maneyrol wrote:
>> Hello,
>>
>> I've done further investigations on this issue, since I wasn't 
>> understanding why we were loosing interrupts where we shouldn't. And I 
>> think I found the root cause.
>>
>> The irq handlers top and bottom are written in a way considering that 
>> every interrupt will be handled by the top part in the hard irq 
>> handler, even if the irq thread is still running in the bottom part. 
>> That's why we are using a FIFO for storing multiple timestamps.
>>
>> But by using triggered-buffer setup, we are using the ONESHOT irq mode 
>> that is disabling irq until the thread processing is done. In this 
>> way, we are never reliably catching data interrupts when the thread is 
>> running, and there is no need to have a FIFO for storing timestamps.
>>
>> The correct way would be to have the top part in the hard irq handler 
>> without using ONESHOT mode, and use only the thread for reading FIFO 
>> data in the triggered-buffer interrupt.
>>
>> I have done a patch which is working correctly on my side. But since I 
>> am not able to trigger the issue in the first time, I cannot guarantee 
>> this is fixing the problem.
>>
>> Martin,
>> is it possible for you to test the following patch and tell me if it 
>> is working better like that?
>>
>> For sure, there is a conception issue here, because in the current 
>> setup having a FIFO for storing timestamps is completely useless.
>>
>> JB
>>
> 
> That's a good point, and sounds like a good change to me. Unfortunately 
> it does not fix the issue I'm seeing, although I'm guessing it would 
> improve it at high frequencies (though that's trickier to measure).
> 
> My issue is happening even at 10 Hz, in which the thread has ample time 
> to complete before the next interrupt, so I think it's something related 
> to the interrupt controller rather than this driver in particular.

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

[Rob Herring]

On Mon, Apr 09, 2018 at 01:21:28PM -0700, Martin Kelly wrote:
> 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(-)

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

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

[Martin Kelly]

On 04/13/2018 02:25 AM, Jean-Baptiste Maneyrol wrote:
> Hello,
> 
> I am now able to reproduce the issue by generating kernel irq (just 
> plug/unplug mouse or keyboard is sufficient).
> 
> My last modification doesn't solve anything, since event the hard irq 
> handler is disabled when processing another interrupt. Having a latched 
> interrupt and triggering by level is a workaround, but clearly not perfect.
> 


I'm glad you can reproduce the issue now! What board are you using? My 
issues have been with the nanopi neo air (based on the Allwinner H3 SoC).

> I think we need to work out a new solution for timestamping correctly 
> the data.
> 
> JB
> 
>
I think we should try to solve the missing interrupts issue. Without 
solving it, the best we can do is interpolate, as I do in the other patch.

That said, I think supporting more interrupt types and interpolating are 
good ideas regardless. Supporting more interrupt types is useful for 
integrating with more types of hardware, and interpolating is useful for 
being more robust against systems having issues, which can unexpectedly 
happen even if we fix the immediate issue we see here.

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

[Jonathan Cameron]

On Wed, 11 Apr 2018 09:42:14 -0700
Martin Kelly <mkelly@xevo.com> wrote:

> On 04/11/2018 12:01 AM, Jean-Baptiste Maneyrol wrote:
> > This is OK for me.
> > 
> > Jonathan will tell us about EBUSY error code for sure if it is not correct.
> > 
> > JB
> >   
> 
> Sounds good; once we hear from Jonathan, I will submit the next revision.
Optimists.  I can never make my mind up on some of the error codes.

It's not totally silly so I'm happy with EBUSY or ENODEV as you wish.

Jonathan

> 
> > On 10/04/2018 20:08, Martin Kelly wrote:  
> >> Thanks, replies inline. I made all the changes you mentioned except 
> >> the one about -EBUSY. Let me know what you think regarding 
> >> -EBUSY/-ENODEV and then I'll send a new revision with all the changes 
> >> included.
> >>
> >> On 04/10/2018 02:06 AM, Jean-Baptiste Maneyrol wrote:  
> >>> Hello,
> >>>
> >>> some more concerns after a deeper look.
> >>>
> >>> Thanks.
> >>> JB
> >>>
> >>> On 09/04/2018 22:21, 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. 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    | 33 
> >>>> ++++++++++++++++++++++++++-
> >>>>   drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c     |  5 ++--
> >>>>   drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h     | 14 ++++++++++--
> >>>>   drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c    | 13 +++++++++++
> >>>>   drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c |  4 ++--
> >>>>   5 files changed, 61 insertions(+), 8 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.
> >>>> v5:
> >>>> - Check interrupt status in all cases rather than only in the level 
> >>>> triggered
> >>>>    case, and warn if we get spurious interrupts.
> >>>> - Rename INV_MPU6050_LEVEL_TRIGGERED to INV_MPU6050_LATCH_INT_EN to 
> >>>> match
> >>>>    datasheet naming.
> >>>> - Write st->irq_mask prior to device power off to make sure it is 
> >>>> fully set.
> >>>> - Write st->irq_mask instead of 0 in inv_mpu6050_deselect_bypass.
> >>>> - Make irq_type local instead of part of the driver state, as we use 
> >>>> it only at
> >>>>    probe time and never again.
> >>>> - Remove the comment about bus lockups, as I have determined them to be
> >>>>    unrelated.
> >>>> - Add missing documentation for irq_type and irq_mask.
> >>>>
> >>>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c 
> >>>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> >>>> index 7d64be353403..b711e6260d9a 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,
> >>>> @@ -278,6 +280,10 @@ static int inv_mpu6050_init_config(struct 
> >>>> iio_dev *indio_dev)
> >>>>       if (result)
> >>>>           return result;
> >>>>
> >>>> +    result = regmap_write(st->map, st->reg->int_pin_cfg, 
> >>>> st->irq_mask);
> >>>> +    if (result)
> >>>> +        return result;
> >>>> +
> >>>>       memcpy(&st->chip_config, hw_info[st->chip_type].config,
> >>>>              sizeof(struct inv_mpu6050_chip_config));
> >>>>       result = inv_mpu6050_set_power_itg(st, false);
> >>>> @@ -882,6 +888,8 @@ 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;
> >>>> +    int irq_type;
> >>>>
> >>>>       indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> >>>>       if (!indio_dev)
> >>>> @@ -913,6 +921,29 @@ 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;  
> >>> I would prefer -ENODEV instead. There is nothing busy as far as I 
> >>> understand.
> >>>  
> >>
> >> I picked -EBUSY based on guidance from this thread:
> >>
> >> https://lists.gt.net/linux/kernel/1010603
> >>
> >> akpm seemed happy with this treatment of -EBUSY and -ENODEV, but I 
> >> can't see whether or not this guidance was formalized.
> >>
> >> Please let me know which error code is more appropriate, and I'll 
> >> change it.
> >>  
> >>>> +    }
> >>>> +
> >>>> +    irq_type = irqd_get_trigger_type(desc);
> >>>> +    if (irq_type == IRQF_TRIGGER_RISING)
> >>>> +        st->irq_mask = INV_MPU6050_ACTIVE_HIGH;
> >>>> +    else if (irq_type == IRQF_TRIGGER_FALLING)
> >>>> +        st->irq_mask = INV_MPU6050_ACTIVE_LOW;
> >>>> +    else if (irq_type == IRQF_TRIGGER_HIGH)
> >>>> +        st->irq_mask = INV_MPU6050_ACTIVE_HIGH |
> >>>> +            INV_MPU6050_LATCH_INT_EN;
> >>>> +    else if (irq_type == IRQF_TRIGGER_LOW)
> >>>> +        st->irq_mask = INV_MPU6050_ACTIVE_LOW |
> >>>> +            INV_MPU6050_LATCH_INT_EN;
> >>>> +    else {
> >>>> +        dev_err(dev, "Invalid interrupt type 0x%x specified\n",
> >>>> +            irq_type);
> >>>> +        return -EBUSY;  
> >>> Same here, -ENODEV makes more sense if I'm understanding well.
> >>>  
> >>>> +    }
> >>>> +
> >>>>       /* power is turned on inside check chip type*/
> >>>>       result = inv_check_and_setup_chip(st);
> >>>>       if (result)
> >>>> @@ -948,7 +979,7 @@ int inv_mpu_core_probe(struct regmap *regmap, 
> >>>> int irq, const char *name,
> >>>>           dev_err(dev, "configure buffer fail %d\n", result);
> >>>>           return result;
> >>>>       }
> >>>> -    result = inv_mpu6050_probe_trigger(indio_dev);
> >>>> +    result = inv_mpu6050_probe_trigger(indio_dev, irq_type);
> >>>>       if (result) {
> >>>>           dev_err(dev, "trigger probe fail %d\n", result);
> >>>>           goto out_unreg_ring;
> >>>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c 
> >>>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> >>>> index fcd7a92b6cf8..1b02d2b69174 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, st->irq_mask);
> >>>>       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..064e3b28fdb0 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;
> >>>> @@ -125,6 +127,7 @@ struct inv_mpu6050_hw {
> >>>>    *  @timestamps:        kfifo queue to store time stamp.
> >>>>    *  @map        regmap pointer.
> >>>>    *  @irq        interrupt number.
> >>>> + *  @irq_mask        the int_pin_cfg mask to configure interrupt type
> >>>>    */
> >>>>   struct inv_mpu6050_state {
> >>>>   #define TIMESTAMP_FIFO_SIZE 16
> >>>> @@ -143,6 +146,7 @@ struct inv_mpu6050_state {
> >>>>       DECLARE_KFIFO(timestamps, long long, TIMESTAMP_FIFO_SIZE);
> >>>>       struct regmap *map;
> >>>>       int irq;
> >>>> +    u16 irq_mask;  
> >>> It is data for a 8 bits register. u8 should be sufficient.
> >>>  
> >>
> >> Good catch.
> >>  
> >>>>   };
> >>>>
> >>>>   /*register and associated bit definition*/
> >>>> @@ -159,6 +163,8 @@ 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_RAW_DATA_RDY_INT    0x01
> >>>>   #define INV_MPU6050_BIT_DATA_RDY_EN         0x01
> >>>>   #define INV_MPU6050_BIT_DMP_INT_EN          0x02  
> >>> Beware you are mixing register and bit defines here. Better put the 
> >>> new definition below and let the interrupt enable BIT defines just 
> >>> below the corresponding REG define.
> >>>  
> >>>>
> >>>> @@ -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
> >>>> +/* enable level triggering */
> >>>> +#define INV_MPU6050_LATCH_INT_EN            0x20  
> >>> Would be better to have this amoung the bit defines of register 
> >>> INT_PIN_CFG. I would prefer just to add these defines below the 
> >>> REG_INT_PIN_CFG:
> >>> #define INV_MPU6050_BIT_ACTIVE_LOW        0x80
> >>> #define INV_MPU6050_BIT_LATCH_INT_EN        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
> >>>> @@ -287,7 +297,7 @@ enum inv_mpu6050_clock_sel_e {
> >>>>
> >>>>   irqreturn_t inv_mpu6050_irq_handler(int irq, void *p);
> >>>>   irqreturn_t inv_mpu6050_read_fifo(int irq, void *p);
> >>>> -int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev);
> >>>> +int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev, int 
> >>>> irq_type);
> >>>>   void inv_mpu6050_remove_trigger(struct inv_mpu6050_state *st);
> >>>>   int inv_reset_fifo(struct iio_dev *indio_dev);
> >>>>   int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool 
> >>>> en, u32 mask);
> >>>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c 
> >>>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> >>>> index ff81c6aa009d..8f1f637fb972 100644
> >>>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> >>>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> >>>> @@ -127,8 +127,21 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void 
> >>>> *p)
> >>>>       u8 data[INV_MPU6050_OUTPUT_DATA_SIZE];
> >>>>       u16 fifo_count;
> >>>>       s64 timestamp;
> >>>> +    int int_status;
> >>>>
> >>>>       mutex_lock(&st->lock);
> >>>> +
> >>>> +    /* ack interrupt and check status */
> >>>> +    result = regmap_read(st->map, st->reg->int_status, &int_status);
> >>>> +    if (result)
> >>>> +        dev_err(regmap_get_device(st->map),  
> >>> If we cannot read int status, I would exit with error using 
> >>> flush_fifo error path. This would ensure the interrupt would be 
> >>> resetted.
> >>>  
> >>
> >> I will do that. Depending on the error cause, resetting the FIFO may 
> >> not work either (for instance, if you get bus lockup, all I2C 
> >> transactions will fail). But it's at least worth a shot as it will 
> >> solve some error cases.
> >>  
> >>>> +            "failed to ack interrupt\n");
> >>>> +    if (!(int_status & INV_MPU6050_BIT_RAW_DATA_RDY_INT)) {
> >>>> +        dev_warn(regmap_get_device(st->map),
> >>>> +            "spurious interrupt with status 0x%x\n", int_status);
> >>>> +        goto end_session;
> >>>> +    }
> >>>> +
> >>>>       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..b8c5584e4252 100644
> >>>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> >>>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> >>>> @@ -117,7 +117,7 @@ static const struct iio_trigger_ops 
> >>>> inv_mpu_trigger_ops = {
> >>>>       .set_trigger_state = &inv_mpu_data_rdy_trigger_set_state,
> >>>>   };
> >>>>
> >>>> -int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev)
> >>>> +int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev, int irq_type)
> >>>>   {
> >>>>       int ret;
> >>>>       struct inv_mpu6050_state *st = iio_priv(indio_dev);
> >>>> @@ -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,
> >>>> +                   irq_type,
> >>>>                      "inv_mpu",
> >>>>                      st->trig);
> >>>>       if (ret)
> >>>> -- 
> >>>> 2.11.0
> >>>>  

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

[Jonathan Cameron]

On Fri, 13 Apr 2018 09:19:41 -0700
Martin Kelly <mkelly@xevo.com> wrote:

> On 04/13/2018 02:25 AM, Jean-Baptiste Maneyrol wrote:
> > Hello,
> > 
> > I am now able to reproduce the issue by generating kernel irq (just 
> > plug/unplug mouse or keyboard is sufficient).
> > 
> > My last modification doesn't solve anything, since event the hard irq 
> > handler is disabled when processing another interrupt. Having a latched 
> > interrupt and triggering by level is a workaround, but clearly not perfect.
> >   
> 
> 
> I'm glad you can reproduce the issue now! What board are you using? My 
> issues have been with the nanopi neo air (based on the Allwinner H3 SoC).
> 
> > I think we need to work out a new solution for timestamping correctly 
> > the data.
> > 
> > JB
> > 
> >  
> I think we should try to solve the missing interrupts issue. Without 
> solving it, the best we can do is interpolate, as I do in the other patch.
> 
> That said, I think supporting more interrupt types and interpolating are 
> good ideas regardless. Supporting more interrupt types is useful for 
> integrating with more types of hardware, and interpolating is useful for 
> being more robust against systems having issues, which can unexpectedly 
> happen even if we fix the immediate issue we see here.
Agreed on the interrupt types.  Interpolating is till rather 'nasty'
so the case is a little less clear, but I'm being convinced I think...

Anyhow, looking forward to v6  Will want JB reviewed-by or acked-by
on this one!

Jonathan

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

[Martin Kelly]

On 04/15/2018 10:43 AM, Jonathan Cameron wrote:
> On Wed, 11 Apr 2018 09:42:14 -0700
> Martin Kelly <mkelly@xevo.com> wrote:
> 
>> On 04/11/2018 12:01 AM, Jean-Baptiste Maneyrol wrote:
>>> This is OK for me.
>>>
>>> Jonathan will tell us about EBUSY error code for sure if it is not correct.
>>>
>>> JB
>>>    
>>
>> Sounds good; once we hear from Jonathan, I will submit the next revision.
> Optimists.  I can never make my mind up on some of the error codes.
> 
> It's not totally silly so I'm happy with EBUSY or ENODEV as you wish.
> 
> Jonathan
> 

OK, then I will defer to Jean-Baptiste on this. Shall we go with ENODEV?

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

[Jean-Baptiste Maneyrol]

On 15/04/2018 21:05, Martin Kelly wrote:
> On 04/15/2018 10:43 AM, Jonathan Cameron wrote:
>> On Wed, 11 Apr 2018 09:42:14 -0700
>> Martin Kelly <mkelly@xevo.com> wrote:
>>
>>> On 04/11/2018 12:01 AM, Jean-Baptiste Maneyrol wrote:
>>>> This is OK for me.
>>>>
>>>> Jonathan will tell us about EBUSY error code for sure if it is not 
>>>> correct.
>>>>
>>>> JB
>>>
>>> Sounds good; once we hear from Jonathan, I will submit the next 
>>> revision.
>> Optimists.  I can never make my mind up on some of the error codes.
>>
>> It's not totally silly so I'm happy with EBUSY or ENODEV as you wish.
>>
>> Jonathan
>>
> 
> OK, then I will defer to Jean-Baptiste on this. Shall we go with ENODEV?

After looking into the irq kernel code, I think perhaps the best value 
should be EINVAL. What it really means, is that configuration is missing 
in the dts file where it shouldn't. Incorrect value seams more 
meaningful in this case.

Do you agree?

JB

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

[Martin Kelly]

On 04/17/2018 07:10 AM, Jean-Baptiste Maneyrol wrote:
> 
> 
> On 15/04/2018 21:05, Martin Kelly wrote:
>> On 04/15/2018 10:43 AM, Jonathan Cameron wrote:
>>> On Wed, 11 Apr 2018 09:42:14 -0700
>>> Martin Kelly <mkelly@xevo.com> wrote:
>>>
>>>> On 04/11/2018 12:01 AM, Jean-Baptiste Maneyrol wrote:
>>>>> This is OK for me.
>>>>>
>>>>> Jonathan will tell us about EBUSY error code for sure if it is not 
>>>>> correct.
>>>>>
>>>>> JB
>>>>
>>>> Sounds good; once we hear from Jonathan, I will submit the next 
>>>> revision.
>>> Optimists.  I can never make my mind up on some of the error codes.
>>>
>>> It's not totally silly so I'm happy with EBUSY or ENODEV as you wish.
>>>
>>> Jonathan
>>>
>>
>> OK, then I will defer to Jean-Baptiste on this. Shall we go with ENODEV?
> 
> After looking into the irq kernel code, I think perhaps the best value 
> should be EINVAL. What it really means, is that configuration is missing 
> in the dts file where it shouldn't. Incorrect value seams more 
> meaningful in this case.
> 
> Do you agree?
> 
> JB

Yes, EINVAL is fine with me. I didn't use it because there was a note in 
the aforementioned document saying that it's best to use a more specific 
error code. However, none of these error codes have a clear case for 
their use, and it's all a bit muddled, as Jonathan mentioned. I'll send 
a revision with EINVAL later this week when I'm back to my desk with 
hardware to test.