From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-by2nam01on0054.outbound.protection.outlook.com ([104.47.34.54]:36592 "EHLO NAM01-BY2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1754431AbeDIUXe (ORCPT ); Mon, 9 Apr 2018 16:23:34 -0400 Subject: Re: [PATCH v4 1/2] iio:imu: inv_mpu6050: support more interrupt types References: <20180406220048.29498-1-mkelly@xevo.com> From: Martin Kelly Message-ID: <1e8465b4-811d-ec38-ff47-c3da3d585067@xevo.com> Date: Mon, 9 Apr 2018 13:23:18 -0700 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: devicetree-owner@vger.kernel.org To: Jean-Baptiste Maneyrol , linux-iio@vger.kernel.org Cc: devicetree@vger.kernel.org, Jonathan Cameron List-ID: Thanks, replies inline. I sent version 5 that addresses all these and a few other things I noticed. On 04/09/2018 12:08 PM, Jean-Baptiste Maneyrol wrote: > Hello, > > my comments below. > > JB > > On 07/04/2018 00:00, Martin Kelly wrote: >> Currently, we support only rising edge interrupts, and in fact we assume >> that the interrupt we're given is rising edge (and things won't work if >> it's not). However, the device supports rising edge, falling edge, level >> low, and level high interrupts. >> >> Empirically, on my system, switching to level interrupts has fixed a >> problem I had with significant (~40%) interrupt loss with edge >> interrupts, as well as bus errors at high frequencies (possibly a >> concurrency issue, which I will look into separately). This issue is >> likely related to the SoC I'm using (Allwinner H3), but being able to >> switch the interrupt type is still a very useful workaround. >> >> I tested this with each interrupt type and verified correct behavior in >> a logic analyzer. >> >> Add support for these interrupt types while also eliminating the error >> case of the device tree and driver using different interrupt types. >> >> Signed-off-by: Martin Kelly >> --- >>   drivers/iio/imu/inv_mpu6050/inv_mpu_core.c    | 36 >> +++++++++++++++++++++++++++ >>   drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c     |  5 ++-- >>   drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h     | 12 ++++++++- >>   drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c    |  9 +++++++ >>   drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c |  2 +- >>   5 files changed, 59 insertions(+), 5 deletions(-) >> >> v2: >> - Changed to ACK level interrupts at the start of the bottom half >> thread instead >>    of at the end of it. Without this, the sample timestamps get >> distorted because >>    the time to handle the bottom half thread delays future interrupts. >> With this >>    change, the timestamps appear evenly distributed at the right >> frequency. >> v3: >> - Sent version 2 too quickly. Now that the ACK is moved to the top of the >>    function, the "goto out" logic is unnecessary, so we can clean it up. >> v4: >> - Moved the ACK inside the mutex. >> >> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c >> b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c >> index 7d64be353403..869768d08802 100644 >> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c >> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c >> @@ -24,6 +24,7 @@ >>   #include >>   #include >>   #include >> +#include >>   #include "inv_mpu_iio.h" >>   /* >> @@ -52,6 +53,7 @@ static const struct inv_mpu6050_reg_map reg_set_6500 >> = { >>       .raw_accl               = INV_MPU6050_REG_RAW_ACCEL, >>       .temperature            = INV_MPU6050_REG_TEMPERATURE, >>       .int_enable             = INV_MPU6050_REG_INT_ENABLE, >> +    .int_status             = INV_MPU6050_REG_INT_STATUS, >>       .pwr_mgmt_1             = INV_MPU6050_REG_PWR_MGMT_1, >>       .pwr_mgmt_2             = INV_MPU6050_REG_PWR_MGMT_2, >>       .int_pin_cfg        = INV_MPU6050_REG_INT_PIN_CFG, >> @@ -281,6 +283,10 @@ static int inv_mpu6050_init_config(struct iio_dev >> *indio_dev) >>       memcpy(&st->chip_config, hw_info[st->chip_type].config, >>              sizeof(struct inv_mpu6050_chip_config)); >>       result = inv_mpu6050_set_power_itg(st, false); >> +    if (result) >> +        return result; >> + >> +    result = regmap_write(st->map, st->reg->int_pin_cfg, st->irq_mask); > Better put this before turning power off. Some registers require power > on or the change is ignored > Thanks, good point. >>       return result; >>   } >> @@ -882,6 +888,7 @@ int inv_mpu_core_probe(struct regmap *regmap, int >> irq, const char *name, >>       struct inv_mpu6050_platform_data *pdata; >>       struct device *dev = regmap_get_device(regmap); >>       int result; >> +    struct irq_data *desc; >>       indio_dev = devm_iio_device_alloc(dev, sizeof(*st)); >>       if (!indio_dev) >> @@ -913,6 +920,35 @@ int inv_mpu_core_probe(struct regmap *regmap, int >> irq, const char *name, >>           st->plat_data = *pdata; >>       } >> +    desc = irq_get_irq_data(irq); >> +    if (!desc) { >> +        dev_err(dev, "Could not find IRQ %d\n", irq); >> +        return -EBUSY; >> +    } >> + >> +    st->irq_type = irqd_get_trigger_type(desc); >> +    if (st->irq_type == IRQF_TRIGGER_RISING) { >> +        st->irq_mask = INV_MPU6050_ACTIVE_HIGH | >> +                INV_MPU6050_EDGE_TRIGGERED; >> +        st->irq_level_triggered = false; >> +    } else if (st->irq_type == IRQF_TRIGGER_FALLING) { >> +        st->irq_mask = INV_MPU6050_ACTIVE_LOW | >> +                INV_MPU6050_EDGE_TRIGGERED; >> +        st->irq_level_triggered = false; >> +    } else if (st->irq_type == IRQF_TRIGGER_HIGH) { >> +        st->irq_mask = INV_MPU6050_ACTIVE_HIGH | >> +                INV_MPU6050_LEVEL_TRIGGERED; >> +        st->irq_level_triggered = true; >> +    } else if (st->irq_type == IRQF_TRIGGER_LOW) { >> +        st->irq_mask = INV_MPU6050_ACTIVE_LOW | >> +                INV_MPU6050_LEVEL_TRIGGERED; >> +        st->irq_level_triggered = true; >> +    } else { >> +        dev_err(dev, "Invalid interrupt type 0x%x specified\n", >> +            st->irq_type); >> +        return -EBUSY; >> +    } >> + >>       /* power is turned on inside check chip type*/ >>       result = inv_check_and_setup_chip(st); >>       if (result) >> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c >> b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c >> index fcd7a92b6cf8..bb3064537943 100644 >> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c >> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c >> @@ -44,8 +44,7 @@ static int inv_mpu6050_select_bypass(struct >> i2c_mux_core *muxc, u32 chan_id) >>       if (!ret) { >>           st->powerup_count++; >>           ret = regmap_write(st->map, st->reg->int_pin_cfg, >> -                   INV_MPU6050_INT_PIN_CFG | >> -                   INV_MPU6050_BIT_BYPASS_EN); >> +               st->irq_mask | INV_MPU6050_BIT_BYPASS_EN); >>       } >>   write_error: >>       mutex_unlock(&st->lock); >> @@ -60,7 +59,7 @@ static int inv_mpu6050_deselect_bypass(struct >> i2c_mux_core *muxc, u32 chan_id) >>       mutex_lock(&st->lock); >>       /* It doesn't really mattter, if any of the calls fails */ >> -    regmap_write(st->map, st->reg->int_pin_cfg, >> INV_MPU6050_INT_PIN_CFG); >> +    regmap_write(st->map, st->reg->int_pin_cfg, 0); > We need to write st->irq_mask here or we will loose irq configuration > each time an i2c device under the mux is used. > Good catch. >>       st->powerup_count--; >>       if (!st->powerup_count) >>           regmap_write(st->map, st->reg->pwr_mgmt_1, >> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h >> b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h >> index 065794162d65..2822b638804f 100644 >> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h >> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h >> @@ -40,6 +40,7 @@ >>    *  @raw_accl:        Address of first accel register. >>    *  @temperature:    temperature register >>    *  @int_enable:    Interrupt enable register. >> + *  @int_status:    Interrupt status register. >>    *  @pwr_mgmt_1:    Controls chip's power state and clock source. >>    *  @pwr_mgmt_2:    Controls power state of individual sensors. >>    *  @int_pin_cfg;    Controls interrupt pin configuration. >> @@ -60,6 +61,7 @@ struct inv_mpu6050_reg_map { >>       u8 raw_accl; >>       u8 temperature; >>       u8 int_enable; >> +    u8 int_status; >>       u8 pwr_mgmt_1; >>       u8 pwr_mgmt_2; >>       u8 int_pin_cfg; >> @@ -143,6 +145,9 @@ struct inv_mpu6050_state { >>       DECLARE_KFIFO(timestamps, long long, TIMESTAMP_FIFO_SIZE); >>       struct regmap *map; >>       int irq; >> +    int irq_type; >> +    u16 irq_mask; >> +    bool irq_level_triggered; >>   }; >>   /*register and associated bit definition*/ >> @@ -159,6 +164,7 @@ struct inv_mpu6050_state { >>   #define INV_MPU6050_BITS_GYRO_OUT           0x70 >>   #define INV_MPU6050_REG_INT_ENABLE          0x38 >> +#define INV_MPU6050_REG_INT_STATUS          0x3a >>   #define INV_MPU6050_BIT_DATA_RDY_EN         0x01 >>   #define INV_MPU6050_BIT_DMP_INT_EN          0x02 >> @@ -190,6 +196,11 @@ struct inv_mpu6050_state { >>   #define INV_MPU6050_FIFO_COUNT_BYTE          2 >>   #define INV_MPU6050_FIFO_THRESHOLD           500 >> +#define INV_MPU6050_ACTIVE_HIGH             0x00 >> +#define INV_MPU6050_ACTIVE_LOW              0x80 >> +#define INV_MPU6050_EDGE_TRIGGERED          0x00 >> +#define INV_MPU6050_LEVEL_TRIGGERED         0x20 >> + > Datasheet name for this bit is LATCH_INT_EN. Better use it to be coherent. > Agreed. >>   /* mpu6500 registers */ >>   #define INV_MPU6500_REG_ACCEL_CONFIG_2      0x1D >>   #define INV_MPU6500_REG_ACCEL_OFFSET        0x77 >> @@ -216,7 +227,6 @@ struct inv_mpu6050_state { >>   #define INV_MPU6050_REG_INT_PIN_CFG    0x37 >>   #define INV_MPU6050_BIT_BYPASS_EN    0x2 >> -#define INV_MPU6050_INT_PIN_CFG        0 >>   /* init parameters */ >>   #define INV_MPU6050_INIT_FIFO_RATE           50 >> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c >> b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c >> index ff81c6aa009d..04dc4a11a9d0 100644 >> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c >> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c >> @@ -129,6 +129,15 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p) >>       s64 timestamp; >>       mutex_lock(&st->lock); >> + >> +    if (st->irq_level_triggered) { >> +        result = regmap_read(st->map, st->reg->int_status, >> +                (int *) data); >> +        if (result) >> +            dev_err(regmap_get_device(st->map), >> +                "failed to ack interrupt\n"); >> +    } >> + > I would prefer to delete this irq_level_triggered boolean and read > status in all cases and check the value. This is more coherent and can > be useful in the future for handling WakeOnMotion or FIFO overflow > interrupts. Also interesting to filter out possible spurious interrupt. > Yes, I like that. Changed, and added a warning if we get a spurious interrupt. >>       if (!(st->chip_config.accl_fifo_enable | >>           st->chip_config.gyro_fifo_enable)) >>           goto end_session; >> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c >> b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c >> index f963f9fc98c0..1e7201a43e34 100644 >> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c >> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c >> @@ -131,7 +131,7 @@ int inv_mpu6050_probe_trigger(struct iio_dev >> *indio_dev) >>       ret = devm_request_irq(&indio_dev->dev, st->irq, >>                      &iio_trigger_generic_data_rdy_poll, >> -                   IRQF_TRIGGER_RISING, >> +                   st->irq_type, >>                      "inv_mpu", >>                      st->trig); >>       if (ret) >>