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:I would prefer -ENODEV instead. There is nothing busy as far as I understand.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@xxxxxxxx> ---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 triggeredcase, 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 atprobe 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.cindex 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 picked -EBUSY based on guidance from this thread: https://lists.gt.net/linux/kernel/1010603akpm 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.cindex 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.hindex 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.
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.}; /*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 0x02Would 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:@@ -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#define INV_MPU6050_BIT_ACTIVE_LOW 0x80 #define INV_MPU6050_BIT_LATCH_INT_EN 0x20If we cannot read int status, I would exit with error using flush_fifo error path. This would ensure the interrupt would be resetted.+ /* 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.cindex 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),
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.cindex 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
-- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
