| Message ID | 20190618125939.105903-5-sean@geanix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: imu: st_lsm6dsx: add event reporting and wakeup | expand |
> The interrupt source can come from multiple sources, fifo and wake interrupts. > Enter interrupt thread to check which interrupt that has fired. > > Signed-off-by: Sean Nyekjaer <sean@geanix.com> > --- > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 30 +++++++++++++++----- > 1 file changed, 23 insertions(+), 7 deletions(-) > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > index 59a34894e495..76aec5024d83 100644 > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > @@ -78,6 +78,12 @@ > #define ST_LSM6DSX_REG_GYRO_OUT_Y_L_ADDR 0x24 > #define ST_LSM6DSX_REG_GYRO_OUT_Z_L_ADDR 0x26 > > +#define ST_LSM6DSX_REG_WAKE_UP_SRC_ADDR 0x1B > +#define ST_LSM6DSX_REG_WAKE_UP_SRC_Z_WU_MASK BIT(0) > +#define ST_LSM6DSX_REG_WAKE_UP_SRC_Y_WU_MASK BIT(1) > +#define ST_LSM6DSX_REG_WAKE_UP_SRC_X_WU_MASK BIT(2) > +#define ST_LSM6DSX_REG_WAKE_UP_SRC_WU_MASK BIT(4) > + > #define ST_LSM6DSX_REG_TAP_CFG_ADDR 0x58 > #define ST_LSM6DSX_REG_TAP_CFG_INT_EN_MASK BIT(7) > #define ST_LSM6DSX_REG_TAP_CFG_INACT_EN_MASK GENMASK(6, 5) > @@ -946,19 +952,29 @@ int st_lsm6dsx_event_setup(struct st_lsm6dsx_hw *hw) > > static irqreturn_t st_lsm6dsx_handler_irq(int irq, void *private) > { > - struct st_lsm6dsx_hw *hw = private; > - > - return hw->sip > 0 ? IRQ_WAKE_THREAD : IRQ_NONE; > + return IRQ_WAKE_THREAD; I guess this will break shared interrupt, isn't it? > } > > static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private) > { > struct st_lsm6dsx_hw *hw = private; > - int count; > + int count = 0; > + int data, err; > + > + if (hw->enable_event) { > + err = regmap_read(hw->regmap, > + ST_LSM6DSX_REG_WAKE_UP_SRC_ADDR, &data); > + if (err < 0) > + goto try_fifo; You can simplify this just doing something like: if (err < 0 && !hw->sip) return IRQ_NONE; > + > + } > > - mutex_lock(&hw->fifo_lock); > - count = st_lsm6dsx_read_fifo(hw); > - mutex_unlock(&hw->fifo_lock); > +try_fifo: > + if (hw->sip > 0) { > + mutex_lock(&hw->fifo_lock); > + count = st_lsm6dsx_read_fifo(hw); > + mutex_unlock(&hw->fifo_lock); > + } > > return !count ? IRQ_NONE : IRQ_HANDLED; > } > -- > 2.22.0 >
On 18/06/2019 17.55, Lorenzo Bianconi wrote: >> The interrupt source can come from multiple sources, fifo and wake interrupts. >> Enter interrupt thread to check which interrupt that has fired. >> >> Signed-off-by: Sean Nyekjaer <sean@geanix.com> >> --- >> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 30 +++++++++++++++----- >> 1 file changed, 23 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c >> index 59a34894e495..76aec5024d83 100644 >> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c >> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c >> @@ -78,6 +78,12 @@ >> #define ST_LSM6DSX_REG_GYRO_OUT_Y_L_ADDR 0x24 >> #define ST_LSM6DSX_REG_GYRO_OUT_Z_L_ADDR 0x26 >> >> +#define ST_LSM6DSX_REG_WAKE_UP_SRC_ADDR 0x1B >> +#define ST_LSM6DSX_REG_WAKE_UP_SRC_Z_WU_MASK BIT(0) >> +#define ST_LSM6DSX_REG_WAKE_UP_SRC_Y_WU_MASK BIT(1) >> +#define ST_LSM6DSX_REG_WAKE_UP_SRC_X_WU_MASK BIT(2) >> +#define ST_LSM6DSX_REG_WAKE_UP_SRC_WU_MASK BIT(4) >> + >> #define ST_LSM6DSX_REG_TAP_CFG_ADDR 0x58 >> #define ST_LSM6DSX_REG_TAP_CFG_INT_EN_MASK BIT(7) >> #define ST_LSM6DSX_REG_TAP_CFG_INACT_EN_MASK GENMASK(6, 5) >> @@ -946,19 +952,29 @@ int st_lsm6dsx_event_setup(struct st_lsm6dsx_hw *hw) >> >> static irqreturn_t st_lsm6dsx_handler_irq(int irq, void *private) >> { >> - struct st_lsm6dsx_hw *hw = private; >> - >> - return hw->sip > 0 ? IRQ_WAKE_THREAD : IRQ_NONE; >> + return IRQ_WAKE_THREAD; > > I guess this will break shared interrupt, isn't it? When you write shared interrupt you mean: the drdy-int-pin stuff? I need to add so we can use all this functionality with the INT2 as well... > >> } >> >> static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private) >> { >> struct st_lsm6dsx_hw *hw = private; >> - int count; >> + int count = 0; >> + int data, err; >> + >> + if (hw->enable_event) { >> + err = regmap_read(hw->regmap, >> + ST_LSM6DSX_REG_WAKE_UP_SRC_ADDR, &data); >> + if (err < 0) >> + goto try_fifo; > > You can simplify this just doing something like: > > if (err < 0 && !hw->sip) > return IRQ_NONE; > Will apply :-) Thanks for the review Lorenzo...
On Jun 18, Sean Nyekjaer wrote: > > > On 18/06/2019 17.55, Lorenzo Bianconi wrote: > > > The interrupt source can come from multiple sources, fifo and wake interrupts. > > > Enter interrupt thread to check which interrupt that has fired. > > > > > > Signed-off-by: Sean Nyekjaer <sean@geanix.com> > > > --- > > > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 30 +++++++++++++++----- > > > 1 file changed, 23 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > > > index 59a34894e495..76aec5024d83 100644 > > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > > > @@ -78,6 +78,12 @@ > > > #define ST_LSM6DSX_REG_GYRO_OUT_Y_L_ADDR 0x24 > > > #define ST_LSM6DSX_REG_GYRO_OUT_Z_L_ADDR 0x26 > > > +#define ST_LSM6DSX_REG_WAKE_UP_SRC_ADDR 0x1B > > > +#define ST_LSM6DSX_REG_WAKE_UP_SRC_Z_WU_MASK BIT(0) > > > +#define ST_LSM6DSX_REG_WAKE_UP_SRC_Y_WU_MASK BIT(1) > > > +#define ST_LSM6DSX_REG_WAKE_UP_SRC_X_WU_MASK BIT(2) > > > +#define ST_LSM6DSX_REG_WAKE_UP_SRC_WU_MASK BIT(4) > > > + > > > #define ST_LSM6DSX_REG_TAP_CFG_ADDR 0x58 > > > #define ST_LSM6DSX_REG_TAP_CFG_INT_EN_MASK BIT(7) > > > #define ST_LSM6DSX_REG_TAP_CFG_INACT_EN_MASK GENMASK(6, 5) > > > @@ -946,19 +952,29 @@ int st_lsm6dsx_event_setup(struct st_lsm6dsx_hw *hw) > > > static irqreturn_t st_lsm6dsx_handler_irq(int irq, void *private) > > > { > > > - struct st_lsm6dsx_hw *hw = private; > > > - > > > - return hw->sip > 0 ? IRQ_WAKE_THREAD : IRQ_NONE; > > > + return IRQ_WAKE_THREAD; > > > > I guess this will break shared interrupt, isn't it? > > When you write shared interrupt you mean: the drdy-int-pin stuff? > I need to add so we can use all this functionality with the INT2 as well... nope, shared IRQ line with other devices (when you set drive-open-drain dts property) > > > > > > } > > > static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private) > > > { > > > struct st_lsm6dsx_hw *hw = private; > > > - int count; > > > + int count = 0; > > > + int data, err; > > > + > > > + if (hw->enable_event) { > > > + err = regmap_read(hw->regmap, > > > + ST_LSM6DSX_REG_WAKE_UP_SRC_ADDR, &data); > > > + if (err < 0) > > > + goto try_fifo; > > > > You can simplify this just doing something like: > > > > if (err < 0 && !hw->sip) > > return IRQ_NONE; > > > Will apply :-) > > Thanks for the review Lorenzo... Thanks for working on this :) Regards, Lorenzo > > -- > Best regards, > Sean Nyekjær > Embedded Linux Consultant > > +45 42427326 > sean@geanix.com > > Geanix ApS > https://geanix.com > DK39600706
On Tue, 18 Jun 2019 22:24:14 +0200 Lorenzo Bianconi <lorenzo@kernel.org> wrote: > On Jun 18, Sean Nyekjaer wrote: > > > > > > On 18/06/2019 17.55, Lorenzo Bianconi wrote: > > > > The interrupt source can come from multiple sources, fifo and wake interrupts. > > > > Enter interrupt thread to check which interrupt that has fired. > > > > > > > > Signed-off-by: Sean Nyekjaer <sean@geanix.com> > > > > --- > > > > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 30 +++++++++++++++----- > > > > 1 file changed, 23 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > > > > index 59a34894e495..76aec5024d83 100644 > > > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > > > > @@ -78,6 +78,12 @@ > > > > #define ST_LSM6DSX_REG_GYRO_OUT_Y_L_ADDR 0x24 > > > > #define ST_LSM6DSX_REG_GYRO_OUT_Z_L_ADDR 0x26 > > > > +#define ST_LSM6DSX_REG_WAKE_UP_SRC_ADDR 0x1B > > > > +#define ST_LSM6DSX_REG_WAKE_UP_SRC_Z_WU_MASK BIT(0) > > > > +#define ST_LSM6DSX_REG_WAKE_UP_SRC_Y_WU_MASK BIT(1) > > > > +#define ST_LSM6DSX_REG_WAKE_UP_SRC_X_WU_MASK BIT(2) > > > > +#define ST_LSM6DSX_REG_WAKE_UP_SRC_WU_MASK BIT(4) > > > > + > > > > #define ST_LSM6DSX_REG_TAP_CFG_ADDR 0x58 > > > > #define ST_LSM6DSX_REG_TAP_CFG_INT_EN_MASK BIT(7) > > > > #define ST_LSM6DSX_REG_TAP_CFG_INACT_EN_MASK GENMASK(6, 5) > > > > @@ -946,19 +952,29 @@ int st_lsm6dsx_event_setup(struct st_lsm6dsx_hw *hw) > > > > static irqreturn_t st_lsm6dsx_handler_irq(int irq, void *private) > > > > { > > > > - struct st_lsm6dsx_hw *hw = private; > > > > - > > > > - return hw->sip > 0 ? IRQ_WAKE_THREAD : IRQ_NONE; > > > > + return IRQ_WAKE_THREAD; > > > > > > I guess this will break shared interrupt, isn't it? > > > > When you write shared interrupt you mean: the drdy-int-pin stuff? > > I need to add so we can use all this functionality with the INT2 as well... > > nope, shared IRQ line with other devices (when you set drive-open-drain dts > property) It's been a while since I looked at this, but... It shouldn't be broken. When using shared interrupts, all interrupt handlers tend to get run, whether or not a given one return IRQ_NONE. Nothing stops multiple devices setting their interrupt lines at the same time. See __handle_irq_event_percpu in kernel/irq/handle.c which is probably the right code. No return values are taken notice of until all registered handlers have run. Jonathan > > > > > > > > > > } > > > > static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private) > > > > { > > > > struct st_lsm6dsx_hw *hw = private; > > > > - int count; > > > > + int count = 0; > > > > + int data, err; > > > > + > > > > + if (hw->enable_event) { > > > > + err = regmap_read(hw->regmap, > > > > + ST_LSM6DSX_REG_WAKE_UP_SRC_ADDR, &data); > > > > + if (err < 0) > > > > + goto try_fifo; > > > > > > You can simplify this just doing something like: > > > > > > if (err < 0 && !hw->sip) > > > return IRQ_NONE; > > > > > Will apply :-) > > > > Thanks for the review Lorenzo... > > Thanks for working on this :) > > Regards, > Lorenzo > > > > > -- > > Best regards, > > Sean Nyekjær > > Embedded Linux Consultant > > > > +45 42427326 > > sean@geanix.com > > > > Geanix ApS > > https://geanix.com > > DK39600706
> > On Tue, 18 Jun 2019 22:24:14 +0200 > Lorenzo Bianconi <lorenzo@kernel.org> wrote: > > > On Jun 18, Sean Nyekjaer wrote: > > > > > > > > > On 18/06/2019 17.55, Lorenzo Bianconi wrote: > > > > > The interrupt source can come from multiple sources, fifo and wake interrupts. > > > > > Enter interrupt thread to check which interrupt that has fired. > > > > > > > > > > Signed-off-by: Sean Nyekjaer <sean@geanix.com> > > > > > --- > > > > > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 30 +++++++++++++++----- > > > > > 1 file changed, 23 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > > > > > index 59a34894e495..76aec5024d83 100644 > > > > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > > > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > > > > > @@ -78,6 +78,12 @@ > > > > > #define ST_LSM6DSX_REG_GYRO_OUT_Y_L_ADDR 0x24 > > > > > #define ST_LSM6DSX_REG_GYRO_OUT_Z_L_ADDR 0x26 > > > > > +#define ST_LSM6DSX_REG_WAKE_UP_SRC_ADDR 0x1B > > > > > +#define ST_LSM6DSX_REG_WAKE_UP_SRC_Z_WU_MASK BIT(0) > > > > > +#define ST_LSM6DSX_REG_WAKE_UP_SRC_Y_WU_MASK BIT(1) > > > > > +#define ST_LSM6DSX_REG_WAKE_UP_SRC_X_WU_MASK BIT(2) > > > > > +#define ST_LSM6DSX_REG_WAKE_UP_SRC_WU_MASK BIT(4) > > > > > + > > > > > #define ST_LSM6DSX_REG_TAP_CFG_ADDR 0x58 > > > > > #define ST_LSM6DSX_REG_TAP_CFG_INT_EN_MASK BIT(7) > > > > > #define ST_LSM6DSX_REG_TAP_CFG_INACT_EN_MASK GENMASK(6, 5) > > > > > @@ -946,19 +952,29 @@ int st_lsm6dsx_event_setup(struct st_lsm6dsx_hw *hw) > > > > > static irqreturn_t st_lsm6dsx_handler_irq(int irq, void *private) > > > > > { > > > > > - struct st_lsm6dsx_hw *hw = private; > > > > > - > > > > > - return hw->sip > 0 ? IRQ_WAKE_THREAD : IRQ_NONE; > > > > > + return IRQ_WAKE_THREAD; > > > > > > > > I guess this will break shared interrupt, isn't it? > > > > > > When you write shared interrupt you mean: the drdy-int-pin stuff? > > > I need to add so we can use all this functionality with the INT2 as well... > > > > nope, shared IRQ line with other devices (when you set drive-open-drain dts > > property) > > It's been a while since I looked at this, but... > > It shouldn't be broken. When using shared interrupts, all interrupt handlers > tend to get run, whether or not a given one return IRQ_NONE. > > Nothing stops multiple devices setting their interrupt lines at the same > time. > > See __handle_irq_event_percpu in kernel/irq/handle.c which is probably > the right code. No return values are taken notice of until all registered > handlers have run. Ops, right. I do not know why I was thinking returning IRQ_NONE stops the iteration over action list. Thanks for pointing it out :) Regards, Lorenzo > > Jonathan > > > > > > > > > > > > > > > } > > > > > static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private) > > > > > { > > > > > struct st_lsm6dsx_hw *hw = private; > > > > > - int count; > > > > > + int count = 0; > > > > > + int data, err; > > > > > + > > > > > + if (hw->enable_event) { > > > > > + err = regmap_read(hw->regmap, > > > > > + ST_LSM6DSX_REG_WAKE_UP_SRC_ADDR, &data); > > > > > + if (err < 0) > > > > > + goto try_fifo; > > > > > > > > You can simplify this just doing something like: > > > > > > > > if (err < 0 && !hw->sip) > > > > return IRQ_NONE; > > > > > > > Will apply :-) > > > > > > Thanks for the review Lorenzo... > > > > Thanks for working on this :) > > > > Regards, > > Lorenzo > > > > > > > > -- > > > Best regards, > > > Sean Nyekjær > > > Embedded Linux Consultant > > > > > > +45 42427326 > > > sean@geanix.com > > > > > > Geanix ApS > > > https://geanix.com > > > DK39600706 >
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c index 59a34894e495..76aec5024d83 100644 --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c @@ -78,6 +78,12 @@ #define ST_LSM6DSX_REG_GYRO_OUT_Y_L_ADDR 0x24 #define ST_LSM6DSX_REG_GYRO_OUT_Z_L_ADDR 0x26 +#define ST_LSM6DSX_REG_WAKE_UP_SRC_ADDR 0x1B +#define ST_LSM6DSX_REG_WAKE_UP_SRC_Z_WU_MASK BIT(0) +#define ST_LSM6DSX_REG_WAKE_UP_SRC_Y_WU_MASK BIT(1) +#define ST_LSM6DSX_REG_WAKE_UP_SRC_X_WU_MASK BIT(2) +#define ST_LSM6DSX_REG_WAKE_UP_SRC_WU_MASK BIT(4) + #define ST_LSM6DSX_REG_TAP_CFG_ADDR 0x58 #define ST_LSM6DSX_REG_TAP_CFG_INT_EN_MASK BIT(7) #define ST_LSM6DSX_REG_TAP_CFG_INACT_EN_MASK GENMASK(6, 5) @@ -946,19 +952,29 @@ int st_lsm6dsx_event_setup(struct st_lsm6dsx_hw *hw) static irqreturn_t st_lsm6dsx_handler_irq(int irq, void *private) { - struct st_lsm6dsx_hw *hw = private; - - return hw->sip > 0 ? IRQ_WAKE_THREAD : IRQ_NONE; + return IRQ_WAKE_THREAD; } static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private) { struct st_lsm6dsx_hw *hw = private; - int count; + int count = 0; + int data, err; + + if (hw->enable_event) { + err = regmap_read(hw->regmap, + ST_LSM6DSX_REG_WAKE_UP_SRC_ADDR, &data); + if (err < 0) + goto try_fifo; + + } - mutex_lock(&hw->fifo_lock); - count = st_lsm6dsx_read_fifo(hw); - mutex_unlock(&hw->fifo_lock); +try_fifo: + if (hw->sip > 0) { + mutex_lock(&hw->fifo_lock); + count = st_lsm6dsx_read_fifo(hw); + mutex_unlock(&hw->fifo_lock); + } return !count ? IRQ_NONE : IRQ_HANDLED; }
The interrupt source can come from multiple sources, fifo and wake interrupts. Enter interrupt thread to check which interrupt that has fired. Signed-off-by: Sean Nyekjaer <sean@geanix.com> --- drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 30 +++++++++++++++----- 1 file changed, 23 insertions(+), 7 deletions(-)