* [PATCH 1/5] iio: imu: st_lsm6dsx: move interrupt thread to core
2019-06-18 12:59 [PATCH 0/5] iio: imu: st_lsm6dsx: add event reporting and wakeup Sean Nyekjaer
@ 2019-06-18 12:59 ` Sean Nyekjaer
2019-06-18 15:57 ` Lorenzo Bianconi
2019-06-18 12:59 ` [PATCH 2/5] iio: imu: st_lsm6dsx: add motion events Sean Nyekjaer
` (3 subsequent siblings)
4 siblings, 1 reply; 23+ messages in thread
From: Sean Nyekjaer @ 2019-06-18 12:59 UTC (permalink / raw)
To: linux-iio, jic23; +Cc: Sean Nyekjaer, lorenzo.bianconi83, martin
This prepares the interrupt to be used for other stuff than
fifo reading -> event readings.
Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h | 1 +
.../iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 80 +----------------
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 87 +++++++++++++++++++
3 files changed, 90 insertions(+), 78 deletions(-)
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
index edcd838037cd..a5e373680e9c 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
@@ -175,5 +175,6 @@ int st_lsm6dsx_update_watermark(struct st_lsm6dsx_sensor *sensor,
int st_lsm6dsx_flush_fifo(struct st_lsm6dsx_hw *hw);
int st_lsm6dsx_set_fifo_mode(struct st_lsm6dsx_hw *hw,
enum st_lsm6dsx_fifo_mode fifo_mode);
+int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw);
#endif /* ST_LSM6DSX_H */
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
index 631360b14ca7..a1ed61a64a64 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
@@ -25,8 +25,6 @@
* Licensed under the GPL-2.
*/
#include <linux/module.h>
-#include <linux/interrupt.h>
-#include <linux/irq.h>
#include <linux/iio/kfifo_buf.h>
#include <linux/iio/iio.h>
#include <linux/iio/buffer.h>
@@ -37,10 +35,6 @@
#include "st_lsm6dsx.h"
-#define ST_LSM6DSX_REG_HLACTIVE_ADDR 0x12
-#define ST_LSM6DSX_REG_HLACTIVE_MASK BIT(5)
-#define ST_LSM6DSX_REG_PP_OD_ADDR 0x12
-#define ST_LSM6DSX_REG_PP_OD_MASK BIT(4)
#define ST_LSM6DSX_REG_FIFO_MODE_ADDR 0x0a
#define ST_LSM6DSX_FIFO_MODE_MASK GENMASK(2, 0)
#define ST_LSM6DSX_FIFO_ODR_MASK GENMASK(6, 3)
@@ -282,7 +276,7 @@ static inline int st_lsm6dsx_read_block(struct st_lsm6dsx_hw *hw, u8 *data,
*
* Return: Number of bytes read from the FIFO
*/
-static int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw)
+int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw)
{
u16 fifo_len, pattern_len = hw->sip * ST_LSM6DSX_SAMPLE_SIZE;
u16 fifo_diff_mask = hw->settings->fifo_ops.fifo_diff.mask;
@@ -465,25 +459,6 @@ static int st_lsm6dsx_update_fifo(struct iio_dev *iio_dev, bool enable)
return err;
}
-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;
-}
-
-static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
-{
- struct st_lsm6dsx_hw *hw = private;
- int count;
-
- mutex_lock(&hw->fifo_lock);
- count = st_lsm6dsx_read_fifo(hw);
- mutex_unlock(&hw->fifo_lock);
-
- return !count ? IRQ_NONE : IRQ_HANDLED;
-}
-
static int st_lsm6dsx_buffer_preenable(struct iio_dev *iio_dev)
{
return st_lsm6dsx_update_fifo(iio_dev, true);
@@ -501,59 +476,8 @@ static const struct iio_buffer_setup_ops st_lsm6dsx_buffer_ops = {
int st_lsm6dsx_fifo_setup(struct st_lsm6dsx_hw *hw)
{
- struct device_node *np = hw->dev->of_node;
- struct st_sensors_platform_data *pdata;
struct iio_buffer *buffer;
- unsigned long irq_type;
- bool irq_active_low;
- int i, err;
-
- irq_type = irqd_get_trigger_type(irq_get_irq_data(hw->irq));
-
- switch (irq_type) {
- case IRQF_TRIGGER_HIGH:
- case IRQF_TRIGGER_RISING:
- irq_active_low = false;
- break;
- case IRQF_TRIGGER_LOW:
- case IRQF_TRIGGER_FALLING:
- irq_active_low = true;
- break;
- default:
- dev_info(hw->dev, "mode %lx unsupported\n", irq_type);
- return -EINVAL;
- }
-
- err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_HLACTIVE_ADDR,
- ST_LSM6DSX_REG_HLACTIVE_MASK,
- FIELD_PREP(ST_LSM6DSX_REG_HLACTIVE_MASK,
- irq_active_low));
- if (err < 0)
- return err;
-
- pdata = (struct st_sensors_platform_data *)hw->dev->platform_data;
- if ((np && of_property_read_bool(np, "drive-open-drain")) ||
- (pdata && pdata->open_drain)) {
- err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_PP_OD_ADDR,
- ST_LSM6DSX_REG_PP_OD_MASK,
- FIELD_PREP(ST_LSM6DSX_REG_PP_OD_MASK,
- 1));
- if (err < 0)
- return err;
-
- irq_type |= IRQF_SHARED;
- }
-
- err = devm_request_threaded_irq(hw->dev, hw->irq,
- st_lsm6dsx_handler_irq,
- st_lsm6dsx_handler_thread,
- irq_type | IRQF_ONESHOT,
- "lsm6dsx", hw);
- if (err) {
- dev_err(hw->dev, "failed to request trigger irq %d\n",
- hw->irq);
- return err;
- }
+ int i;
for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
buffer = devm_iio_kfifo_allocate(hw->dev);
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index aebbe0ddd8d8..b5d3fa354de7 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -36,6 +36,8 @@
#include <linux/delay.h>
#include <linux/iio/iio.h>
#include <linux/iio/sysfs.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
#include <linux/pm.h>
#include <linux/regmap.h>
#include <linux/bitfield.h>
@@ -55,6 +57,11 @@
#define ST_LSM6DSX_REG_INT2_ON_INT1_ADDR 0x13
#define ST_LSM6DSX_REG_INT2_ON_INT1_MASK BIT(5)
+#define ST_LSM6DSX_REG_HLACTIVE_ADDR 0x12
+#define ST_LSM6DSX_REG_HLACTIVE_MASK BIT(5)
+#define ST_LSM6DSX_REG_PP_OD_ADDR 0x12
+#define ST_LSM6DSX_REG_PP_OD_MASK BIT(4)
+
#define ST_LSM6DSX_REG_ACC_ODR_ADDR 0x10
#define ST_LSM6DSX_REG_ACC_ODR_MASK GENMASK(7, 4)
#define ST_LSM6DSX_REG_ACC_FS_ADDR 0x10
@@ -804,6 +811,83 @@ static struct iio_dev *st_lsm6dsx_alloc_iiodev(struct st_lsm6dsx_hw *hw,
return iio_dev;
}
+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;
+}
+
+static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
+{
+ struct st_lsm6dsx_hw *hw = private;
+ int count;
+
+ mutex_lock(&hw->fifo_lock);
+ count = st_lsm6dsx_read_fifo(hw);
+ mutex_unlock(&hw->fifo_lock);
+
+ return !count ? IRQ_NONE : IRQ_HANDLED;
+}
+
+int st_lsm6dsx_irq_setup(struct st_lsm6dsx_hw *hw)
+{
+ struct st_sensors_platform_data *pdata;
+ struct device_node *np = hw->dev->of_node;
+ unsigned long irq_type;
+ bool irq_active_low;
+ int err;
+
+ irq_type = irqd_get_trigger_type(irq_get_irq_data(hw->irq));
+
+ switch (irq_type) {
+ case IRQF_TRIGGER_HIGH:
+ case IRQF_TRIGGER_RISING:
+ irq_active_low = false;
+ break;
+ case IRQF_TRIGGER_LOW:
+ case IRQF_TRIGGER_FALLING:
+ irq_active_low = true;
+ break;
+ default:
+ dev_info(hw->dev, "mode %lx unsupported\n", irq_type);
+ return -EINVAL;
+ }
+
+ err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_HLACTIVE_ADDR,
+ ST_LSM6DSX_REG_HLACTIVE_MASK,
+ FIELD_PREP(ST_LSM6DSX_REG_HLACTIVE_MASK,
+ irq_active_low));
+ if (err < 0)
+ return err;
+
+ pdata = (struct st_sensors_platform_data *)hw->dev->platform_data;
+ if ((np && of_property_read_bool(np, "drive-open-drain")) ||
+ (pdata && pdata->open_drain)) {
+ err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_PP_OD_ADDR,
+ ST_LSM6DSX_REG_PP_OD_MASK,
+ FIELD_PREP(ST_LSM6DSX_REG_PP_OD_MASK,
+ 1));
+ if (err < 0)
+ return err;
+
+ irq_type |= IRQF_SHARED;
+ }
+
+ err = devm_request_threaded_irq(hw->dev, hw->irq,
+ st_lsm6dsx_handler_irq,
+ st_lsm6dsx_handler_thread,
+ irq_type | IRQF_ONESHOT,
+ "lsm6dsx", hw);
+ if (err) {
+ dev_err(hw->dev, "failed to request trigger irq %d\n",
+ hw->irq);
+ return err;
+ }
+
+ return err;
+}
+
int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
struct regmap *regmap)
{
@@ -842,6 +926,9 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
return err;
if (hw->irq > 0) {
+ err = st_lsm6dsx_irq_setup(hw);
+ if (err < 0)
+ return err;
err = st_lsm6dsx_fifo_setup(hw);
if (err < 0)
return err;
--
2.22.0
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 1/5] iio: imu: st_lsm6dsx: move interrupt thread to core
2019-06-18 12:59 ` [PATCH 1/5] iio: imu: st_lsm6dsx: move interrupt thread to core Sean Nyekjaer
@ 2019-06-18 15:57 ` Lorenzo Bianconi
2019-06-18 16:56 ` Sean Nyekjær
0 siblings, 1 reply; 23+ messages in thread
From: Lorenzo Bianconi @ 2019-06-18 15:57 UTC (permalink / raw)
To: Sean Nyekjaer; +Cc: linux-iio, jic23, lorenzo.bianconi83, martin
[-- Attachment #1: Type: text/plain, Size: 8713 bytes --]
> This prepares the interrupt to be used for other stuff than
> fifo reading -> event readings.
>
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h | 1 +
> .../iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 80 +----------------
> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 87 +++++++++++++++++++
> 3 files changed, 90 insertions(+), 78 deletions(-)
>
I can't see why we need this patch
Regards,
Lorenzo
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> index edcd838037cd..a5e373680e9c 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> @@ -175,5 +175,6 @@ int st_lsm6dsx_update_watermark(struct st_lsm6dsx_sensor *sensor,
> int st_lsm6dsx_flush_fifo(struct st_lsm6dsx_hw *hw);
> int st_lsm6dsx_set_fifo_mode(struct st_lsm6dsx_hw *hw,
> enum st_lsm6dsx_fifo_mode fifo_mode);
> +int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw);
>
> #endif /* ST_LSM6DSX_H */
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> index 631360b14ca7..a1ed61a64a64 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> @@ -25,8 +25,6 @@
> * Licensed under the GPL-2.
> */
> #include <linux/module.h>
> -#include <linux/interrupt.h>
> -#include <linux/irq.h>
> #include <linux/iio/kfifo_buf.h>
> #include <linux/iio/iio.h>
> #include <linux/iio/buffer.h>
> @@ -37,10 +35,6 @@
>
> #include "st_lsm6dsx.h"
>
> -#define ST_LSM6DSX_REG_HLACTIVE_ADDR 0x12
> -#define ST_LSM6DSX_REG_HLACTIVE_MASK BIT(5)
> -#define ST_LSM6DSX_REG_PP_OD_ADDR 0x12
> -#define ST_LSM6DSX_REG_PP_OD_MASK BIT(4)
> #define ST_LSM6DSX_REG_FIFO_MODE_ADDR 0x0a
> #define ST_LSM6DSX_FIFO_MODE_MASK GENMASK(2, 0)
> #define ST_LSM6DSX_FIFO_ODR_MASK GENMASK(6, 3)
> @@ -282,7 +276,7 @@ static inline int st_lsm6dsx_read_block(struct st_lsm6dsx_hw *hw, u8 *data,
> *
> * Return: Number of bytes read from the FIFO
> */
> -static int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw)
> +int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw)
> {
> u16 fifo_len, pattern_len = hw->sip * ST_LSM6DSX_SAMPLE_SIZE;
> u16 fifo_diff_mask = hw->settings->fifo_ops.fifo_diff.mask;
> @@ -465,25 +459,6 @@ static int st_lsm6dsx_update_fifo(struct iio_dev *iio_dev, bool enable)
> return err;
> }
>
> -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;
> -}
> -
> -static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
> -{
> - struct st_lsm6dsx_hw *hw = private;
> - int count;
> -
> - mutex_lock(&hw->fifo_lock);
> - count = st_lsm6dsx_read_fifo(hw);
> - mutex_unlock(&hw->fifo_lock);
> -
> - return !count ? IRQ_NONE : IRQ_HANDLED;
> -}
> -
> static int st_lsm6dsx_buffer_preenable(struct iio_dev *iio_dev)
> {
> return st_lsm6dsx_update_fifo(iio_dev, true);
> @@ -501,59 +476,8 @@ static const struct iio_buffer_setup_ops st_lsm6dsx_buffer_ops = {
>
> int st_lsm6dsx_fifo_setup(struct st_lsm6dsx_hw *hw)
> {
> - struct device_node *np = hw->dev->of_node;
> - struct st_sensors_platform_data *pdata;
> struct iio_buffer *buffer;
> - unsigned long irq_type;
> - bool irq_active_low;
> - int i, err;
> -
> - irq_type = irqd_get_trigger_type(irq_get_irq_data(hw->irq));
> -
> - switch (irq_type) {
> - case IRQF_TRIGGER_HIGH:
> - case IRQF_TRIGGER_RISING:
> - irq_active_low = false;
> - break;
> - case IRQF_TRIGGER_LOW:
> - case IRQF_TRIGGER_FALLING:
> - irq_active_low = true;
> - break;
> - default:
> - dev_info(hw->dev, "mode %lx unsupported\n", irq_type);
> - return -EINVAL;
> - }
> -
> - err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_HLACTIVE_ADDR,
> - ST_LSM6DSX_REG_HLACTIVE_MASK,
> - FIELD_PREP(ST_LSM6DSX_REG_HLACTIVE_MASK,
> - irq_active_low));
> - if (err < 0)
> - return err;
> -
> - pdata = (struct st_sensors_platform_data *)hw->dev->platform_data;
> - if ((np && of_property_read_bool(np, "drive-open-drain")) ||
> - (pdata && pdata->open_drain)) {
> - err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_PP_OD_ADDR,
> - ST_LSM6DSX_REG_PP_OD_MASK,
> - FIELD_PREP(ST_LSM6DSX_REG_PP_OD_MASK,
> - 1));
> - if (err < 0)
> - return err;
> -
> - irq_type |= IRQF_SHARED;
> - }
> -
> - err = devm_request_threaded_irq(hw->dev, hw->irq,
> - st_lsm6dsx_handler_irq,
> - st_lsm6dsx_handler_thread,
> - irq_type | IRQF_ONESHOT,
> - "lsm6dsx", hw);
> - if (err) {
> - dev_err(hw->dev, "failed to request trigger irq %d\n",
> - hw->irq);
> - return err;
> - }
> + int i;
>
> for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
> buffer = devm_iio_kfifo_allocate(hw->dev);
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> index aebbe0ddd8d8..b5d3fa354de7 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> @@ -36,6 +36,8 @@
> #include <linux/delay.h>
> #include <linux/iio/iio.h>
> #include <linux/iio/sysfs.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> #include <linux/pm.h>
> #include <linux/regmap.h>
> #include <linux/bitfield.h>
> @@ -55,6 +57,11 @@
> #define ST_LSM6DSX_REG_INT2_ON_INT1_ADDR 0x13
> #define ST_LSM6DSX_REG_INT2_ON_INT1_MASK BIT(5)
>
> +#define ST_LSM6DSX_REG_HLACTIVE_ADDR 0x12
> +#define ST_LSM6DSX_REG_HLACTIVE_MASK BIT(5)
> +#define ST_LSM6DSX_REG_PP_OD_ADDR 0x12
> +#define ST_LSM6DSX_REG_PP_OD_MASK BIT(4)
> +
> #define ST_LSM6DSX_REG_ACC_ODR_ADDR 0x10
> #define ST_LSM6DSX_REG_ACC_ODR_MASK GENMASK(7, 4)
> #define ST_LSM6DSX_REG_ACC_FS_ADDR 0x10
> @@ -804,6 +811,83 @@ static struct iio_dev *st_lsm6dsx_alloc_iiodev(struct st_lsm6dsx_hw *hw,
> return iio_dev;
> }
>
> +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;
> +}
> +
> +static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
> +{
> + struct st_lsm6dsx_hw *hw = private;
> + int count;
> +
> + mutex_lock(&hw->fifo_lock);
> + count = st_lsm6dsx_read_fifo(hw);
> + mutex_unlock(&hw->fifo_lock);
> +
> + return !count ? IRQ_NONE : IRQ_HANDLED;
> +}
> +
> +int st_lsm6dsx_irq_setup(struct st_lsm6dsx_hw *hw)
> +{
> + struct st_sensors_platform_data *pdata;
> + struct device_node *np = hw->dev->of_node;
> + unsigned long irq_type;
> + bool irq_active_low;
> + int err;
> +
> + irq_type = irqd_get_trigger_type(irq_get_irq_data(hw->irq));
> +
> + switch (irq_type) {
> + case IRQF_TRIGGER_HIGH:
> + case IRQF_TRIGGER_RISING:
> + irq_active_low = false;
> + break;
> + case IRQF_TRIGGER_LOW:
> + case IRQF_TRIGGER_FALLING:
> + irq_active_low = true;
> + break;
> + default:
> + dev_info(hw->dev, "mode %lx unsupported\n", irq_type);
> + return -EINVAL;
> + }
> +
> + err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_HLACTIVE_ADDR,
> + ST_LSM6DSX_REG_HLACTIVE_MASK,
> + FIELD_PREP(ST_LSM6DSX_REG_HLACTIVE_MASK,
> + irq_active_low));
> + if (err < 0)
> + return err;
> +
> + pdata = (struct st_sensors_platform_data *)hw->dev->platform_data;
> + if ((np && of_property_read_bool(np, "drive-open-drain")) ||
> + (pdata && pdata->open_drain)) {
> + err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_PP_OD_ADDR,
> + ST_LSM6DSX_REG_PP_OD_MASK,
> + FIELD_PREP(ST_LSM6DSX_REG_PP_OD_MASK,
> + 1));
> + if (err < 0)
> + return err;
> +
> + irq_type |= IRQF_SHARED;
> + }
> +
> + err = devm_request_threaded_irq(hw->dev, hw->irq,
> + st_lsm6dsx_handler_irq,
> + st_lsm6dsx_handler_thread,
> + irq_type | IRQF_ONESHOT,
> + "lsm6dsx", hw);
> + if (err) {
> + dev_err(hw->dev, "failed to request trigger irq %d\n",
> + hw->irq);
> + return err;
> + }
> +
> + return err;
> +}
> +
> int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
> struct regmap *regmap)
> {
> @@ -842,6 +926,9 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
> return err;
>
> if (hw->irq > 0) {
> + err = st_lsm6dsx_irq_setup(hw);
> + if (err < 0)
> + return err;
> err = st_lsm6dsx_fifo_setup(hw);
> if (err < 0)
> return err;
> --
> 2.22.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/5] iio: imu: st_lsm6dsx: move interrupt thread to core
2019-06-18 15:57 ` Lorenzo Bianconi
@ 2019-06-18 16:56 ` Sean Nyekjær
2019-06-19 5:58 ` Lorenzo Bianconi
0 siblings, 1 reply; 23+ messages in thread
From: Sean Nyekjær @ 2019-06-18 16:56 UTC (permalink / raw)
To: Lorenzo Bianconi; +Cc: linux-iio, jic23, lorenzo.bianconi83, martin
On 18 Jun 2019, at 17.57, Lorenzo Bianconi <lorenzo@kernel.org> wrote:
>> This prepares the interrupt to be used for other stuff than
>> fifo reading -> event readings.
>>
>> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
>> ---
>> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h | 1 +
>> .../iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 80 +----------------
>> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 87 +++++++++++++++++++
>> 3 files changed, 90 insertions(+), 78 deletions(-)
>>
>
> I can't see why we need this patch
>
> Regards,
> Lorenzo
The interrupt handling isn’t only for fifo reading, so I think it’s correct to move it out of the buffer handling file.
/Sean
>
>> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
>> index edcd838037cd..a5e373680e9c 100644
>> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
>> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
>> @@ -175,5 +175,6 @@ int st_lsm6dsx_update_watermark(struct st_lsm6dsx_sensor *sensor,
>> int st_lsm6dsx_flush_fifo(struct st_lsm6dsx_hw *hw);
>> int st_lsm6dsx_set_fifo_mode(struct st_lsm6dsx_hw *hw,
>> enum st_lsm6dsx_fifo_mode fifo_mode);
>> +int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw);
>>
>> #endif /* ST_LSM6DSX_H */
>> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
>> index 631360b14ca7..a1ed61a64a64 100644
>> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
>> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
>> @@ -25,8 +25,6 @@
>> * Licensed under the GPL-2.
>> */
>> #include <linux/module.h>
>> -#include <linux/interrupt.h>
>> -#include <linux/irq.h>
>> #include <linux/iio/kfifo_buf.h>
>> #include <linux/iio/iio.h>
>> #include <linux/iio/buffer.h>
>> @@ -37,10 +35,6 @@
>>
>> #include "st_lsm6dsx.h"
>>
>> -#define ST_LSM6DSX_REG_HLACTIVE_ADDR 0x12
>> -#define ST_LSM6DSX_REG_HLACTIVE_MASK BIT(5)
>> -#define ST_LSM6DSX_REG_PP_OD_ADDR 0x12
>> -#define ST_LSM6DSX_REG_PP_OD_MASK BIT(4)
>> #define ST_LSM6DSX_REG_FIFO_MODE_ADDR 0x0a
>> #define ST_LSM6DSX_FIFO_MODE_MASK GENMASK(2, 0)
>> #define ST_LSM6DSX_FIFO_ODR_MASK GENMASK(6, 3)
>> @@ -282,7 +276,7 @@ static inline int st_lsm6dsx_read_block(struct st_lsm6dsx_hw *hw, u8 *data,
>> *
>> * Return: Number of bytes read from the FIFO
>> */
>> -static int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw)
>> +int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw)
>> {
>> u16 fifo_len, pattern_len = hw->sip * ST_LSM6DSX_SAMPLE_SIZE;
>> u16 fifo_diff_mask = hw->settings->fifo_ops.fifo_diff.mask;
>> @@ -465,25 +459,6 @@ static int st_lsm6dsx_update_fifo(struct iio_dev *iio_dev, bool enable)
>> return err;
>> }
>>
>> -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;
>> -}
>> -
>> -static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
>> -{
>> - struct st_lsm6dsx_hw *hw = private;
>> - int count;
>> -
>> - mutex_lock(&hw->fifo_lock);
>> - count = st_lsm6dsx_read_fifo(hw);
>> - mutex_unlock(&hw->fifo_lock);
>> -
>> - return !count ? IRQ_NONE : IRQ_HANDLED;
>> -}
>> -
>> static int st_lsm6dsx_buffer_preenable(struct iio_dev *iio_dev)
>> {
>> return st_lsm6dsx_update_fifo(iio_dev, true);
>> @@ -501,59 +476,8 @@ static const struct iio_buffer_setup_ops st_lsm6dsx_buffer_ops = {
>>
>> int st_lsm6dsx_fifo_setup(struct st_lsm6dsx_hw *hw)
>> {
>> - struct device_node *np = hw->dev->of_node;
>> - struct st_sensors_platform_data *pdata;
>> struct iio_buffer *buffer;
>> - unsigned long irq_type;
>> - bool irq_active_low;
>> - int i, err;
>> -
>> - irq_type = irqd_get_trigger_type(irq_get_irq_data(hw->irq));
>> -
>> - switch (irq_type) {
>> - case IRQF_TRIGGER_HIGH:
>> - case IRQF_TRIGGER_RISING:
>> - irq_active_low = false;
>> - break;
>> - case IRQF_TRIGGER_LOW:
>> - case IRQF_TRIGGER_FALLING:
>> - irq_active_low = true;
>> - break;
>> - default:
>> - dev_info(hw->dev, "mode %lx unsupported\n", irq_type);
>> - return -EINVAL;
>> - }
>> -
>> - err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_HLACTIVE_ADDR,
>> - ST_LSM6DSX_REG_HLACTIVE_MASK,
>> - FIELD_PREP(ST_LSM6DSX_REG_HLACTIVE_MASK,
>> - irq_active_low));
>> - if (err < 0)
>> - return err;
>> -
>> - pdata = (struct st_sensors_platform_data *)hw->dev->platform_data;
>> - if ((np && of_property_read_bool(np, "drive-open-drain")) ||
>> - (pdata && pdata->open_drain)) {
>> - err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_PP_OD_ADDR,
>> - ST_LSM6DSX_REG_PP_OD_MASK,
>> - FIELD_PREP(ST_LSM6DSX_REG_PP_OD_MASK,
>> - 1));
>> - if (err < 0)
>> - return err;
>> -
>> - irq_type |= IRQF_SHARED;
>> - }
>> -
>> - err = devm_request_threaded_irq(hw->dev, hw->irq,
>> - st_lsm6dsx_handler_irq,
>> - st_lsm6dsx_handler_thread,
>> - irq_type | IRQF_ONESHOT,
>> - "lsm6dsx", hw);
>> - if (err) {
>> - dev_err(hw->dev, "failed to request trigger irq %d\n",
>> - hw->irq);
>> - return err;
>> - }
>> + int i;
>>
>> for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
>> buffer = devm_iio_kfifo_allocate(hw->dev);
>> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> index aebbe0ddd8d8..b5d3fa354de7 100644
>> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> @@ -36,6 +36,8 @@
>> #include <linux/delay.h>
>> #include <linux/iio/iio.h>
>> #include <linux/iio/sysfs.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>> #include <linux/pm.h>
>> #include <linux/regmap.h>
>> #include <linux/bitfield.h>
>> @@ -55,6 +57,11 @@
>> #define ST_LSM6DSX_REG_INT2_ON_INT1_ADDR 0x13
>> #define ST_LSM6DSX_REG_INT2_ON_INT1_MASK BIT(5)
>>
>> +#define ST_LSM6DSX_REG_HLACTIVE_ADDR 0x12
>> +#define ST_LSM6DSX_REG_HLACTIVE_MASK BIT(5)
>> +#define ST_LSM6DSX_REG_PP_OD_ADDR 0x12
>> +#define ST_LSM6DSX_REG_PP_OD_MASK BIT(4)
>> +
>> #define ST_LSM6DSX_REG_ACC_ODR_ADDR 0x10
>> #define ST_LSM6DSX_REG_ACC_ODR_MASK GENMASK(7, 4)
>> #define ST_LSM6DSX_REG_ACC_FS_ADDR 0x10
>> @@ -804,6 +811,83 @@ static struct iio_dev *st_lsm6dsx_alloc_iiodev(struct st_lsm6dsx_hw *hw,
>> return iio_dev;
>> }
>>
>> +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;
>> +}
>> +
>> +static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
>> +{
>> + struct st_lsm6dsx_hw *hw = private;
>> + int count;
>> +
>> + mutex_lock(&hw->fifo_lock);
>> + count = st_lsm6dsx_read_fifo(hw);
>> + mutex_unlock(&hw->fifo_lock);
>> +
>> + return !count ? IRQ_NONE : IRQ_HANDLED;
>> +}
>> +
>> +int st_lsm6dsx_irq_setup(struct st_lsm6dsx_hw *hw)
>> +{
>> + struct st_sensors_platform_data *pdata;
>> + struct device_node *np = hw->dev->of_node;
>> + unsigned long irq_type;
>> + bool irq_active_low;
>> + int err;
>> +
>> + irq_type = irqd_get_trigger_type(irq_get_irq_data(hw->irq));
>> +
>> + switch (irq_type) {
>> + case IRQF_TRIGGER_HIGH:
>> + case IRQF_TRIGGER_RISING:
>> + irq_active_low = false;
>> + break;
>> + case IRQF_TRIGGER_LOW:
>> + case IRQF_TRIGGER_FALLING:
>> + irq_active_low = true;
>> + break;
>> + default:
>> + dev_info(hw->dev, "mode %lx unsupported\n", irq_type);
>> + return -EINVAL;
>> + }
>> +
>> + err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_HLACTIVE_ADDR,
>> + ST_LSM6DSX_REG_HLACTIVE_MASK,
>> + FIELD_PREP(ST_LSM6DSX_REG_HLACTIVE_MASK,
>> + irq_active_low));
>> + if (err < 0)
>> + return err;
>> +
>> + pdata = (struct st_sensors_platform_data *)hw->dev->platform_data;
>> + if ((np && of_property_read_bool(np, "drive-open-drain")) ||
>> + (pdata && pdata->open_drain)) {
>> + err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_PP_OD_ADDR,
>> + ST_LSM6DSX_REG_PP_OD_MASK,
>> + FIELD_PREP(ST_LSM6DSX_REG_PP_OD_MASK,
>> + 1));
>> + if (err < 0)
>> + return err;
>> +
>> + irq_type |= IRQF_SHARED;
>> + }
>> +
>> + err = devm_request_threaded_irq(hw->dev, hw->irq,
>> + st_lsm6dsx_handler_irq,
>> + st_lsm6dsx_handler_thread,
>> + irq_type | IRQF_ONESHOT,
>> + "lsm6dsx", hw);
>> + if (err) {
>> + dev_err(hw->dev, "failed to request trigger irq %d\n",
>> + hw->irq);
>> + return err;
>> + }
>> +
>> + return err;
>> +}
>> +
>> int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
>> struct regmap *regmap)
>> {
>> @@ -842,6 +926,9 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
>> return err;
>>
>> if (hw->irq > 0) {
>> + err = st_lsm6dsx_irq_setup(hw);
>> + if (err < 0)
>> + return err;
>> err = st_lsm6dsx_fifo_setup(hw);
>> if (err < 0)
>> return err;
>> --
>> 2.22.0
>>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/5] iio: imu: st_lsm6dsx: move interrupt thread to core
2019-06-18 16:56 ` Sean Nyekjær
@ 2019-06-19 5:58 ` Lorenzo Bianconi
2019-06-19 6:01 ` Sean Nyekjaer
0 siblings, 1 reply; 23+ messages in thread
From: Lorenzo Bianconi @ 2019-06-19 5:58 UTC (permalink / raw)
To: Sean Nyekjær; +Cc: linux-iio, jic23, lorenzo.bianconi83, martin
[-- Attachment #1: Type: text/plain, Size: 10692 bytes --]
>
> On 18 Jun 2019, at 17.57, Lorenzo Bianconi <lorenzo@kernel.org> wrote:
>
> >> This prepares the interrupt to be used for other stuff than
> >> fifo reading -> event readings.
> >>
> >> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> >> ---
> >> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h | 1 +
> >> .../iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 80 +----------------
> >> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 87 +++++++++++++++++++
> >> 3 files changed, 90 insertions(+), 78 deletions(-)
> >>
> >
> > I can't see why we need this patch
> >
> > Regards,
> > Lorenzo
>
> The interrupt handling isn’t only for fifo reading, so I think it’s correct to move it out of the buffer handling file.
Uhm, re-thinking about it I agree, it will be useful even for upcoming I3C
support. Btw I think you are using a prehistoric version of the driver ;)
Regards,
Lorenzo
>
> /Sean
> >
> >> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> >> index edcd838037cd..a5e373680e9c 100644
> >> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> >> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> >> @@ -175,5 +175,6 @@ int st_lsm6dsx_update_watermark(struct st_lsm6dsx_sensor *sensor,
> >> int st_lsm6dsx_flush_fifo(struct st_lsm6dsx_hw *hw);
> >> int st_lsm6dsx_set_fifo_mode(struct st_lsm6dsx_hw *hw,
> >> enum st_lsm6dsx_fifo_mode fifo_mode);
> >> +int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw);
> >>
> >> #endif /* ST_LSM6DSX_H */
> >> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> >> index 631360b14ca7..a1ed61a64a64 100644
> >> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> >> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> >> @@ -25,8 +25,6 @@
> >> * Licensed under the GPL-2.
> >> */
> >> #include <linux/module.h>
> >> -#include <linux/interrupt.h>
> >> -#include <linux/irq.h>
> >> #include <linux/iio/kfifo_buf.h>
> >> #include <linux/iio/iio.h>
> >> #include <linux/iio/buffer.h>
> >> @@ -37,10 +35,6 @@
> >>
> >> #include "st_lsm6dsx.h"
> >>
> >> -#define ST_LSM6DSX_REG_HLACTIVE_ADDR 0x12
> >> -#define ST_LSM6DSX_REG_HLACTIVE_MASK BIT(5)
> >> -#define ST_LSM6DSX_REG_PP_OD_ADDR 0x12
> >> -#define ST_LSM6DSX_REG_PP_OD_MASK BIT(4)
> >> #define ST_LSM6DSX_REG_FIFO_MODE_ADDR 0x0a
> >> #define ST_LSM6DSX_FIFO_MODE_MASK GENMASK(2, 0)
> >> #define ST_LSM6DSX_FIFO_ODR_MASK GENMASK(6, 3)
> >> @@ -282,7 +276,7 @@ static inline int st_lsm6dsx_read_block(struct st_lsm6dsx_hw *hw, u8 *data,
> >> *
> >> * Return: Number of bytes read from the FIFO
> >> */
> >> -static int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw)
> >> +int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw)
> >> {
> >> u16 fifo_len, pattern_len = hw->sip * ST_LSM6DSX_SAMPLE_SIZE;
> >> u16 fifo_diff_mask = hw->settings->fifo_ops.fifo_diff.mask;
> >> @@ -465,25 +459,6 @@ static int st_lsm6dsx_update_fifo(struct iio_dev *iio_dev, bool enable)
> >> return err;
> >> }
> >>
> >> -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;
> >> -}
> >> -
> >> -static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
> >> -{
> >> - struct st_lsm6dsx_hw *hw = private;
> >> - int count;
> >> -
> >> - mutex_lock(&hw->fifo_lock);
> >> - count = st_lsm6dsx_read_fifo(hw);
> >> - mutex_unlock(&hw->fifo_lock);
> >> -
> >> - return !count ? IRQ_NONE : IRQ_HANDLED;
> >> -}
> >> -
> >> static int st_lsm6dsx_buffer_preenable(struct iio_dev *iio_dev)
> >> {
> >> return st_lsm6dsx_update_fifo(iio_dev, true);
> >> @@ -501,59 +476,8 @@ static const struct iio_buffer_setup_ops st_lsm6dsx_buffer_ops = {
> >>
> >> int st_lsm6dsx_fifo_setup(struct st_lsm6dsx_hw *hw)
> >> {
> >> - struct device_node *np = hw->dev->of_node;
> >> - struct st_sensors_platform_data *pdata;
> >> struct iio_buffer *buffer;
> >> - unsigned long irq_type;
> >> - bool irq_active_low;
> >> - int i, err;
> >> -
> >> - irq_type = irqd_get_trigger_type(irq_get_irq_data(hw->irq));
> >> -
> >> - switch (irq_type) {
> >> - case IRQF_TRIGGER_HIGH:
> >> - case IRQF_TRIGGER_RISING:
> >> - irq_active_low = false;
> >> - break;
> >> - case IRQF_TRIGGER_LOW:
> >> - case IRQF_TRIGGER_FALLING:
> >> - irq_active_low = true;
> >> - break;
> >> - default:
> >> - dev_info(hw->dev, "mode %lx unsupported\n", irq_type);
> >> - return -EINVAL;
> >> - }
> >> -
> >> - err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_HLACTIVE_ADDR,
> >> - ST_LSM6DSX_REG_HLACTIVE_MASK,
> >> - FIELD_PREP(ST_LSM6DSX_REG_HLACTIVE_MASK,
> >> - irq_active_low));
> >> - if (err < 0)
> >> - return err;
> >> -
> >> - pdata = (struct st_sensors_platform_data *)hw->dev->platform_data;
> >> - if ((np && of_property_read_bool(np, "drive-open-drain")) ||
> >> - (pdata && pdata->open_drain)) {
> >> - err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_PP_OD_ADDR,
> >> - ST_LSM6DSX_REG_PP_OD_MASK,
> >> - FIELD_PREP(ST_LSM6DSX_REG_PP_OD_MASK,
> >> - 1));
> >> - if (err < 0)
> >> - return err;
> >> -
> >> - irq_type |= IRQF_SHARED;
> >> - }
> >> -
> >> - err = devm_request_threaded_irq(hw->dev, hw->irq,
> >> - st_lsm6dsx_handler_irq,
> >> - st_lsm6dsx_handler_thread,
> >> - irq_type | IRQF_ONESHOT,
> >> - "lsm6dsx", hw);
> >> - if (err) {
> >> - dev_err(hw->dev, "failed to request trigger irq %d\n",
> >> - hw->irq);
> >> - return err;
> >> - }
> >> + int i;
> >>
> >> for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
> >> buffer = devm_iio_kfifo_allocate(hw->dev);
> >> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> >> index aebbe0ddd8d8..b5d3fa354de7 100644
> >> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> >> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> >> @@ -36,6 +36,8 @@
> >> #include <linux/delay.h>
> >> #include <linux/iio/iio.h>
> >> #include <linux/iio/sysfs.h>
> >> +#include <linux/interrupt.h>
> >> +#include <linux/irq.h>
> >> #include <linux/pm.h>
> >> #include <linux/regmap.h>
> >> #include <linux/bitfield.h>
> >> @@ -55,6 +57,11 @@
> >> #define ST_LSM6DSX_REG_INT2_ON_INT1_ADDR 0x13
> >> #define ST_LSM6DSX_REG_INT2_ON_INT1_MASK BIT(5)
> >>
> >> +#define ST_LSM6DSX_REG_HLACTIVE_ADDR 0x12
> >> +#define ST_LSM6DSX_REG_HLACTIVE_MASK BIT(5)
> >> +#define ST_LSM6DSX_REG_PP_OD_ADDR 0x12
> >> +#define ST_LSM6DSX_REG_PP_OD_MASK BIT(4)
> >> +
> >> #define ST_LSM6DSX_REG_ACC_ODR_ADDR 0x10
> >> #define ST_LSM6DSX_REG_ACC_ODR_MASK GENMASK(7, 4)
> >> #define ST_LSM6DSX_REG_ACC_FS_ADDR 0x10
> >> @@ -804,6 +811,83 @@ static struct iio_dev *st_lsm6dsx_alloc_iiodev(struct st_lsm6dsx_hw *hw,
> >> return iio_dev;
> >> }
> >>
> >> +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;
> >> +}
> >> +
> >> +static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
> >> +{
> >> + struct st_lsm6dsx_hw *hw = private;
> >> + int count;
> >> +
> >> + mutex_lock(&hw->fifo_lock);
> >> + count = st_lsm6dsx_read_fifo(hw);
> >> + mutex_unlock(&hw->fifo_lock);
> >> +
> >> + return !count ? IRQ_NONE : IRQ_HANDLED;
> >> +}
> >> +
> >> +int st_lsm6dsx_irq_setup(struct st_lsm6dsx_hw *hw)
> >> +{
> >> + struct st_sensors_platform_data *pdata;
> >> + struct device_node *np = hw->dev->of_node;
> >> + unsigned long irq_type;
> >> + bool irq_active_low;
> >> + int err;
> >> +
> >> + irq_type = irqd_get_trigger_type(irq_get_irq_data(hw->irq));
> >> +
> >> + switch (irq_type) {
> >> + case IRQF_TRIGGER_HIGH:
> >> + case IRQF_TRIGGER_RISING:
> >> + irq_active_low = false;
> >> + break;
> >> + case IRQF_TRIGGER_LOW:
> >> + case IRQF_TRIGGER_FALLING:
> >> + irq_active_low = true;
> >> + break;
> >> + default:
> >> + dev_info(hw->dev, "mode %lx unsupported\n", irq_type);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_HLACTIVE_ADDR,
> >> + ST_LSM6DSX_REG_HLACTIVE_MASK,
> >> + FIELD_PREP(ST_LSM6DSX_REG_HLACTIVE_MASK,
> >> + irq_active_low));
> >> + if (err < 0)
> >> + return err;
> >> +
> >> + pdata = (struct st_sensors_platform_data *)hw->dev->platform_data;
> >> + if ((np && of_property_read_bool(np, "drive-open-drain")) ||
> >> + (pdata && pdata->open_drain)) {
> >> + err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_PP_OD_ADDR,
> >> + ST_LSM6DSX_REG_PP_OD_MASK,
> >> + FIELD_PREP(ST_LSM6DSX_REG_PP_OD_MASK,
> >> + 1));
> >> + if (err < 0)
> >> + return err;
> >> +
> >> + irq_type |= IRQF_SHARED;
> >> + }
> >> +
> >> + err = devm_request_threaded_irq(hw->dev, hw->irq,
> >> + st_lsm6dsx_handler_irq,
> >> + st_lsm6dsx_handler_thread,
> >> + irq_type | IRQF_ONESHOT,
> >> + "lsm6dsx", hw);
> >> + if (err) {
> >> + dev_err(hw->dev, "failed to request trigger irq %d\n",
> >> + hw->irq);
> >> + return err;
> >> + }
> >> +
> >> + return err;
> >> +}
> >> +
> >> int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
> >> struct regmap *regmap)
> >> {
> >> @@ -842,6 +926,9 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
> >> return err;
> >>
> >> if (hw->irq > 0) {
> >> + err = st_lsm6dsx_irq_setup(hw);
> >> + if (err < 0)
> >> + return err;
> >> err = st_lsm6dsx_fifo_setup(hw);
> >> if (err < 0)
> >> return err;
> >> --
> >> 2.22.0
> >>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/5] iio: imu: st_lsm6dsx: move interrupt thread to core
2019-06-19 5:58 ` Lorenzo Bianconi
@ 2019-06-19 6:01 ` Sean Nyekjaer
0 siblings, 0 replies; 23+ messages in thread
From: Sean Nyekjaer @ 2019-06-19 6:01 UTC (permalink / raw)
To: Lorenzo Bianconi; +Cc: linux-iio, jic23, lorenzo.bianconi83, martin
On 19/06/2019 07.58, Lorenzo Bianconi wrote:
>>
>>> I can't see why we need this patch
>>>
>>> Regards,
>>> Lorenzo
>>
>> The interrupt handling isn’t only for fifo reading, so I think it’s correct to move it out of the buffer handling file.
>
> Uhm, re-thinking about it I agree, it will be useful even for upcoming I3C
> support. Btw I think you are using a prehistoric version of the driver ;)
>
> Regards,
> Lorenzo
>
Yes I'm on 4.19... will rebase to iio "testing" for V2
/Sean
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/5] iio: imu: st_lsm6dsx: add motion events
2019-06-18 12:59 [PATCH 0/5] iio: imu: st_lsm6dsx: add event reporting and wakeup Sean Nyekjaer
2019-06-18 12:59 ` [PATCH 1/5] iio: imu: st_lsm6dsx: move interrupt thread to core Sean Nyekjaer
@ 2019-06-18 12:59 ` Sean Nyekjaer
2019-06-18 15:49 ` Lorenzo Bianconi
2019-06-18 12:59 ` [PATCH 3/5] iio: imu: st_lsm6dsx: add wakeup-source option Sean Nyekjaer
` (2 subsequent siblings)
4 siblings, 1 reply; 23+ messages in thread
From: Sean Nyekjaer @ 2019-06-18 12:59 UTC (permalink / raw)
To: linux-iio, jic23; +Cc: Sean Nyekjaer, lorenzo.bianconi83, martin
Add event channels that controls the creation of motion events.
Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h | 2 +
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 148 ++++++++++++++++++-
2 files changed, 144 insertions(+), 6 deletions(-)
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
index a5e373680e9c..966cc6e91c7f 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
@@ -155,6 +155,8 @@ struct st_lsm6dsx_hw {
u8 enable_mask;
u8 ts_sip;
u8 sip;
+ u8 event_threshold;
+ bool enable_event;
u8 *buff;
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index b5d3fa354de7..351c46f01662 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -78,6 +78,17 @@
#define ST_LSM6DSX_REG_GYRO_OUT_Y_L_ADDR 0x24
#define ST_LSM6DSX_REG_GYRO_OUT_Z_L_ADDR 0x26
+#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)
+
+#define ST_LSM6DSX_DEFAULT_WAKE_THRESH 0
+#define ST_LSM6DSX_REG_WAKE_UP_THS_ADDR 0x5B
+#define ST_LSM6DSX_REG_WAKE_UP_THS_THRES_MASK GENMASK(5, 0)
+
+#define ST_LSM6DSX_REG_MD1_CFG_ADDR 0x5E
+#define ST_LSM6DSX_REG_MD1_CFG_INT1_WU_MASK BIT(5)
+
#define ST_LSM6DSX_ACC_FS_2G_GAIN IIO_G_TO_M_S_2(61)
#define ST_LSM6DSX_ACC_FS_4G_GAIN IIO_G_TO_M_S_2(122)
#define ST_LSM6DSX_ACC_FS_8G_GAIN IIO_G_TO_M_S_2(244)
@@ -303,6 +314,13 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
},
};
+static const struct iio_event_spec st_lsm6dsx_event = {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_EITHER,
+ .mask_separate = BIT(IIO_EV_INFO_VALUE) |
+ BIT(IIO_EV_INFO_ENABLE)
+};
+
#define ST_LSM6DSX_CHANNEL(chan_type, addr, mod, scan_idx) \
{ \
.type = chan_type, \
@@ -319,6 +337,8 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
.storagebits = 16, \
.endianness = IIO_LE, \
}, \
+ .event_spec = &st_lsm6dsx_event, \
+ .num_event_specs = 1, \
}
static const struct iio_chan_spec st_lsm6dsx_acc_channels[] = {
@@ -435,6 +455,20 @@ static int st_lsm6dsx_set_odr(struct st_lsm6dsx_sensor *sensor, u16 odr)
ST_LSM6DSX_SHIFT_VAL(val, reg->mask));
}
+static int st_lsm6dsx_set_event_threshold(struct st_lsm6dsx_hw *hw, u8 threshold)
+{
+ int err = 0;
+
+ err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_WAKE_UP_THS_ADDR,
+ ST_LSM6DSX_REG_WAKE_UP_THS_THRES_MASK,
+ threshold);
+
+ if (!err)
+ hw->event_threshold = threshold;
+
+ return err;
+}
+
int st_lsm6dsx_sensor_enable(struct st_lsm6dsx_sensor *sensor)
{
int err;
@@ -472,18 +506,21 @@ static int st_lsm6dsx_read_oneshot(struct st_lsm6dsx_sensor *sensor,
int err, delay;
__le16 data;
- err = st_lsm6dsx_sensor_enable(sensor);
- if (err < 0)
- return err;
+ if (!hw->enable_event) {
+ err = st_lsm6dsx_sensor_enable(sensor);
+ if (err < 0)
+ return err;
- delay = 1000000 / sensor->odr;
- usleep_range(delay, 2 * delay);
+ delay = 1000000 / sensor->odr;
+ usleep_range(delay, 2 * delay);
+ }
err = regmap_bulk_read(hw->regmap, addr, &data, sizeof(data));
if (err < 0)
return err;
- st_lsm6dsx_sensor_disable(sensor);
+ if (!hw->enable_event)
+ st_lsm6dsx_sensor_disable(sensor);
*val = (s16)le16_to_cpu(data);
@@ -556,6 +593,75 @@ static int st_lsm6dsx_write_raw(struct iio_dev *iio_dev,
return err;
}
+static int st_lsm6dsx_read_event(struct iio_dev *iio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ enum iio_event_info info,
+ int *val, int *val2)
+{
+ struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
+
+ *val2 = 0;
+ *val = sensor->hw->event_threshold;
+
+ return IIO_VAL_INT;
+}
+
+static int st_lsm6dsx_write_event(struct iio_dev *iio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ enum iio_event_info info,
+ int val, int val2)
+{
+ struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
+ struct st_lsm6dsx_hw *hw = sensor->hw;
+
+ if (!hw->enable_event)
+ return -EBUSY;
+
+ if ((val < 0) || (val > 31))
+ return -EINVAL;
+
+ if (st_lsm6dsx_set_event_threshold(sensor->hw, val))
+ return -EINVAL;
+
+ return 0;
+}
+
+static int st_lsm6dsx_read_event_config(struct iio_dev *iio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir)
+{
+ struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
+ struct st_lsm6dsx_hw *hw = sensor->hw;
+
+ return hw->enable_event;
+}
+
+static int st_lsm6dsx_write_event_config(struct iio_dev *iio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ int state)
+{
+ struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
+ struct st_lsm6dsx_hw *hw = sensor->hw;
+
+ if (state && hw->enable_event)
+ return 0;
+
+ hw->enable_event = state;
+ if (state)
+ st_lsm6dsx_sensor_enable(sensor);
+ else
+ st_lsm6dsx_sensor_disable(sensor);
+
+ return 0;
+}
+
static int st_lsm6dsx_set_watermark(struct iio_dev *iio_dev, unsigned int val)
{
struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
@@ -632,6 +738,10 @@ static const struct iio_info st_lsm6dsx_acc_info = {
.attrs = &st_lsm6dsx_acc_attribute_group,
.read_raw = st_lsm6dsx_read_raw,
.write_raw = st_lsm6dsx_write_raw,
+ .read_event_value = st_lsm6dsx_read_event,
+ .write_event_value = st_lsm6dsx_write_event,
+ .read_event_config = st_lsm6dsx_read_event_config,
+ .write_event_config = st_lsm6dsx_write_event_config,
.hwfifo_set_watermark = st_lsm6dsx_set_watermark,
};
@@ -761,6 +871,8 @@ static int st_lsm6dsx_init_device(struct st_lsm6dsx_hw *hw)
if (err < 0)
return err;
+ st_lsm6dsx_set_event_threshold(hw, ST_LSM6DSX_DEFAULT_WAKE_THRESH);
+
return st_lsm6dsx_init_hw_timer(hw);
}
@@ -811,6 +923,27 @@ static struct iio_dev *st_lsm6dsx_alloc_iiodev(struct st_lsm6dsx_hw *hw,
return iio_dev;
}
+int st_lsm6dsx_event_setup(struct st_lsm6dsx_hw *hw)
+{
+ int err;
+
+ /* Enable inactivity function - low power ACC, GYRO powered-down */
+ err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_TAP_CFG_ADDR,
+ ST_LSM6DSX_REG_TAP_CFG_INT_EN_MASK |
+ ST_LSM6DSX_REG_TAP_CFG_INACT_EN_MASK,
+ ST_LSM6DSX_REG_TAP_CFG_INT_EN_MASK |
+ ST_LSM6DSX_REG_TAP_CFG_INACT_EN_MASK);
+ if (err < 0)
+ return err;
+
+ /* Enable wakeup interrupt */
+ err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_MD1_CFG_ADDR,
+ ST_LSM6DSX_REG_MD1_CFG_INT1_WU_MASK,
+ ST_LSM6DSX_REG_MD1_CFG_INT1_WU_MASK);
+
+ return err;
+}
+
static irqreturn_t st_lsm6dsx_handler_irq(int irq, void *private)
{
struct st_lsm6dsx_hw *hw = private;
@@ -932,6 +1065,9 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
err = st_lsm6dsx_fifo_setup(hw);
if (err < 0)
return err;
+ err = st_lsm6dsx_event_setup(hw);
+ if (err < 0)
+ return err;
}
for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
--
2.22.0
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 2/5] iio: imu: st_lsm6dsx: add motion events
2019-06-18 12:59 ` [PATCH 2/5] iio: imu: st_lsm6dsx: add motion events Sean Nyekjaer
@ 2019-06-18 15:49 ` Lorenzo Bianconi
2019-06-18 19:14 ` Sean Nyekjaer
0 siblings, 1 reply; 23+ messages in thread
From: Lorenzo Bianconi @ 2019-06-18 15:49 UTC (permalink / raw)
To: Sean Nyekjaer; +Cc: linux-iio, jic23, lorenzo.bianconi83, martin
[-- Attachment #1: Type: text/plain, Size: 8617 bytes --]
> Add event channels that controls the creation of motion events.
>
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h | 2 +
> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 148 ++++++++++++++++++-
> 2 files changed, 144 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> index a5e373680e9c..966cc6e91c7f 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> @@ -155,6 +155,8 @@ struct st_lsm6dsx_hw {
> u8 enable_mask;
> u8 ts_sip;
> u8 sip;
> + u8 event_threshold;
> + bool enable_event;
>
> u8 *buff;
>
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> index b5d3fa354de7..351c46f01662 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> @@ -78,6 +78,17 @@
> #define ST_LSM6DSX_REG_GYRO_OUT_Y_L_ADDR 0x24
> #define ST_LSM6DSX_REG_GYRO_OUT_Z_L_ADDR 0x26
>
> +#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)
> +
> +#define ST_LSM6DSX_DEFAULT_WAKE_THRESH 0
> +#define ST_LSM6DSX_REG_WAKE_UP_THS_ADDR 0x5B
> +#define ST_LSM6DSX_REG_WAKE_UP_THS_THRES_MASK GENMASK(5, 0)
Could you please verify this registermap is used for all supported devices?
(e.g. LSM6DS3 or LSM6DS3H)
> +
> +#define ST_LSM6DSX_REG_MD1_CFG_ADDR 0x5E
> +#define ST_LSM6DSX_REG_MD1_CFG_INT1_WU_MASK BIT(5)
> +
> #define ST_LSM6DSX_ACC_FS_2G_GAIN IIO_G_TO_M_S_2(61)
> #define ST_LSM6DSX_ACC_FS_4G_GAIN IIO_G_TO_M_S_2(122)
> #define ST_LSM6DSX_ACC_FS_8G_GAIN IIO_G_TO_M_S_2(244)
> @@ -303,6 +314,13 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
> },
> };
>
> +static const struct iio_event_spec st_lsm6dsx_event = {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_EITHER,
> + .mask_separate = BIT(IIO_EV_INFO_VALUE) |
> + BIT(IIO_EV_INFO_ENABLE)
> +};
> +
> #define ST_LSM6DSX_CHANNEL(chan_type, addr, mod, scan_idx) \
> { \
> .type = chan_type, \
> @@ -319,6 +337,8 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
> .storagebits = 16, \
> .endianness = IIO_LE, \
> }, \
> + .event_spec = &st_lsm6dsx_event, \
> + .num_event_specs = 1, \
> }
ST_LSM6DSX_CHANNEL is used even for sensor-hub so I do not think you can use in
this way
>
> static const struct iio_chan_spec st_lsm6dsx_acc_channels[] = {
> @@ -435,6 +455,20 @@ static int st_lsm6dsx_set_odr(struct st_lsm6dsx_sensor *sensor, u16 odr)
> ST_LSM6DSX_SHIFT_VAL(val, reg->mask));
> }
>
> +static int st_lsm6dsx_set_event_threshold(struct st_lsm6dsx_hw *hw, u8 threshold)
> +{
> + int err = 0;
> +
> + err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_WAKE_UP_THS_ADDR,
> + ST_LSM6DSX_REG_WAKE_UP_THS_THRES_MASK,
> + threshold);
> +
> + if (!err)
> + hw->event_threshold = threshold;
> +
> + return err;
I think we can just move this configuration in st_lsm6dsx_write_event()
> +}
> +
> int st_lsm6dsx_sensor_enable(struct st_lsm6dsx_sensor *sensor)
> {
> int err;
> @@ -472,18 +506,21 @@ static int st_lsm6dsx_read_oneshot(struct st_lsm6dsx_sensor *sensor,
> int err, delay;
> __le16 data;
>
> - err = st_lsm6dsx_sensor_enable(sensor);
> - if (err < 0)
> - return err;
> + if (!hw->enable_event) {
> + err = st_lsm6dsx_sensor_enable(sensor);
> + if (err < 0)
> + return err;
>
> - delay = 1000000 / sensor->odr;
> - usleep_range(delay, 2 * delay);
> + delay = 1000000 / sensor->odr;
> + usleep_range(delay, 2 * delay);
> + }
>
> err = regmap_bulk_read(hw->regmap, addr, &data, sizeof(data));
> if (err < 0)
> return err;
>
> - st_lsm6dsx_sensor_disable(sensor);
> + if (!hw->enable_event)
> + st_lsm6dsx_sensor_disable(sensor);
>
> *val = (s16)le16_to_cpu(data);
>
> @@ -556,6 +593,75 @@ static int st_lsm6dsx_write_raw(struct iio_dev *iio_dev,
> return err;
> }
>
> +static int st_lsm6dsx_read_event(struct iio_dev *iio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info,
> + int *val, int *val2)
> +{
> + struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
> +
> + *val2 = 0;
> + *val = sensor->hw->event_threshold;
> +
> + return IIO_VAL_INT;
> +}
> +
> +static int st_lsm6dsx_write_event(struct iio_dev *iio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info,
> + int val, int val2)
> +{
> + struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
> + struct st_lsm6dsx_hw *hw = sensor->hw;
> +
> + if (!hw->enable_event)
> + return -EBUSY;
> +
> + if ((val < 0) || (val > 31))
unnecessary brackets
> + return -EINVAL;
> +
> + if (st_lsm6dsx_set_event_threshold(sensor->hw, val))
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int st_lsm6dsx_read_event_config(struct iio_dev *iio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir)
> +{
> + struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
> + struct st_lsm6dsx_hw *hw = sensor->hw;
> +
> + return hw->enable_event;
> +}
> +
> +static int st_lsm6dsx_write_event_config(struct iio_dev *iio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + int state)
> +{
> + struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
> + struct st_lsm6dsx_hw *hw = sensor->hw;
> +
> + if (state && hw->enable_event)
> + return 0;
> +
> + hw->enable_event = state;
> + if (state)
> + st_lsm6dsx_sensor_enable(sensor);
please correct me if I am wrong but in this way we break normal operation (e.g.
if we are reading acc data from the FIFO). You need to check enabled_mask.
Moreover we should check event type
> + else
> + st_lsm6dsx_sensor_disable(sensor);
> +
> + return 0;
> +}
> +
> static int st_lsm6dsx_set_watermark(struct iio_dev *iio_dev, unsigned int val)
> {
> struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
> @@ -632,6 +738,10 @@ static const struct iio_info st_lsm6dsx_acc_info = {
> .attrs = &st_lsm6dsx_acc_attribute_group,
> .read_raw = st_lsm6dsx_read_raw,
> .write_raw = st_lsm6dsx_write_raw,
> + .read_event_value = st_lsm6dsx_read_event,
> + .write_event_value = st_lsm6dsx_write_event,
> + .read_event_config = st_lsm6dsx_read_event_config,
> + .write_event_config = st_lsm6dsx_write_event_config,
> .hwfifo_set_watermark = st_lsm6dsx_set_watermark,
> };
>
> @@ -761,6 +871,8 @@ static int st_lsm6dsx_init_device(struct st_lsm6dsx_hw *hw)
> if (err < 0)
> return err;
>
> + st_lsm6dsx_set_event_threshold(hw, ST_LSM6DSX_DEFAULT_WAKE_THRESH);
we do not need this since default value is already 0
> +
> return st_lsm6dsx_init_hw_timer(hw);
> }
>
> @@ -811,6 +923,27 @@ static struct iio_dev *st_lsm6dsx_alloc_iiodev(struct st_lsm6dsx_hw *hw,
> return iio_dev;
> }
>
> +int st_lsm6dsx_event_setup(struct st_lsm6dsx_hw *hw)
> +{
> + int err;
> +
> + /* Enable inactivity function - low power ACC, GYRO powered-down */
> + err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_TAP_CFG_ADDR,
> + ST_LSM6DSX_REG_TAP_CFG_INT_EN_MASK |
> + ST_LSM6DSX_REG_TAP_CFG_INACT_EN_MASK,
> + ST_LSM6DSX_REG_TAP_CFG_INT_EN_MASK |
> + ST_LSM6DSX_REG_TAP_CFG_INACT_EN_MASK);
> + if (err < 0)
> + return err;
> +
> + /* Enable wakeup interrupt */
> + err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_MD1_CFG_ADDR,
> + ST_LSM6DSX_REG_MD1_CFG_INT1_WU_MASK,
> + ST_LSM6DSX_REG_MD1_CFG_INT1_WU_MASK);
> +
> + return err;
> +}
> +
> static irqreturn_t st_lsm6dsx_handler_irq(int irq, void *private)
> {
> struct st_lsm6dsx_hw *hw = private;
> @@ -932,6 +1065,9 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
> err = st_lsm6dsx_fifo_setup(hw);
> if (err < 0)
> return err;
> + err = st_lsm6dsx_event_setup(hw);
> + if (err < 0)
> + return err;
> }
>
> for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
> --
> 2.22.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/5] iio: imu: st_lsm6dsx: add motion events
2019-06-18 15:49 ` Lorenzo Bianconi
@ 2019-06-18 19:14 ` Sean Nyekjaer
2019-06-18 20:21 ` Lorenzo Bianconi
0 siblings, 1 reply; 23+ messages in thread
From: Sean Nyekjaer @ 2019-06-18 19:14 UTC (permalink / raw)
To: Lorenzo Bianconi; +Cc: linux-iio, jic23, lorenzo.bianconi83, martin
On 18/06/2019 17.49, Lorenzo Bianconi wrote:
>> Add event channels that controls the creation of motion events.
>>
>> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
>> ---
>> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h | 2 +
>> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 148 ++++++++++++++++++-
>> 2 files changed, 144 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
>> index a5e373680e9c..966cc6e91c7f 100644
>> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
>> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
>> @@ -155,6 +155,8 @@ struct st_lsm6dsx_hw {
>> u8 enable_mask;
>> u8 ts_sip;
>> u8 sip;
>> + u8 event_threshold;
>> + bool enable_event;
>>
>> u8 *buff;
>>
>> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> index b5d3fa354de7..351c46f01662 100644
>> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> @@ -78,6 +78,17 @@
>> #define ST_LSM6DSX_REG_GYRO_OUT_Y_L_ADDR 0x24
>> #define ST_LSM6DSX_REG_GYRO_OUT_Z_L_ADDR 0x26
>>
>> +#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)
>> +
>> +#define ST_LSM6DSX_DEFAULT_WAKE_THRESH 0
>> +#define ST_LSM6DSX_REG_WAKE_UP_THS_ADDR 0x5B
>> +#define ST_LSM6DSX_REG_WAKE_UP_THS_THRES_MASK GENMASK(5, 0)
>
> Could you please verify this registermap is used for all supported devices?
> (e.g. LSM6DS3 or LSM6DS3H)
They have the same registers and the bits meaning the same.
But I can't test on those devices...
>
>> +
>> +#define ST_LSM6DSX_REG_MD1_CFG_ADDR 0x5E
>> +#define ST_LSM6DSX_REG_MD1_CFG_INT1_WU_MASK BIT(5)
>> +
>> #define ST_LSM6DSX_ACC_FS_2G_GAIN IIO_G_TO_M_S_2(61)
>> #define ST_LSM6DSX_ACC_FS_4G_GAIN IIO_G_TO_M_S_2(122)
>> #define ST_LSM6DSX_ACC_FS_8G_GAIN IIO_G_TO_M_S_2(244)
>> @@ -303,6 +314,13 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>> },
>> };
>>
>> +static const struct iio_event_spec st_lsm6dsx_event = {
>> + .type = IIO_EV_TYPE_THRESH,
>> + .dir = IIO_EV_DIR_EITHER,
>> + .mask_separate = BIT(IIO_EV_INFO_VALUE) |
>> + BIT(IIO_EV_INFO_ENABLE)
>> +};
>> +
>> #define ST_LSM6DSX_CHANNEL(chan_type, addr, mod, scan_idx) \
>> { \
>> .type = chan_type, \
>> @@ -319,6 +337,8 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>> .storagebits = 16, \
>> .endianness = IIO_LE, \
>> }, \
>> + .event_spec = &st_lsm6dsx_event, \
>> + .num_event_specs = 1, \
>> }
>
> ST_LSM6DSX_CHANNEL is used even for sensor-hub so I do not think you can use in
> this way
I see, will need to come up with a solution to avoid the event channels
on the gyro.
>
>>
>> static const struct iio_chan_spec st_lsm6dsx_acc_channels[] = {
>> @@ -435,6 +455,20 @@ static int st_lsm6dsx_set_odr(struct st_lsm6dsx_sensor *sensor, u16 odr)
>> ST_LSM6DSX_SHIFT_VAL(val, reg->mask));
>> }
>>
>> +static int st_lsm6dsx_set_event_threshold(struct st_lsm6dsx_hw *hw, u8 threshold)
>> +{
>> + int err = 0;
>> +
>> + err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_WAKE_UP_THS_ADDR,
>> + ST_LSM6DSX_REG_WAKE_UP_THS_THRES_MASK,
>> + threshold);
>> +
>> + if (!err)
>> + hw->event_threshold = threshold;
>> +
>> + return err;
>
> I think we can just move this configuration in st_lsm6dsx_write_event()
We could, should I then call st_lsm6dsx_write_event() from the probe
function?
>
>> +}
>> +
>> int st_lsm6dsx_sensor_enable(struct st_lsm6dsx_sensor *sensor)
>> {
>> int err;
>> @@ -472,18 +506,21 @@ static int st_lsm6dsx_read_oneshot(struct st_lsm6dsx_sensor *sensor,
>> int err, delay;
>> __le16 data;
>>
>> - err = st_lsm6dsx_sensor_enable(sensor);
>> - if (err < 0)
>> - return err;
>> + if (!hw->enable_event) {
>> + err = st_lsm6dsx_sensor_enable(sensor);
>> + if (err < 0)
>> + return err;
>>
>> - delay = 1000000 / sensor->odr;
>> - usleep_range(delay, 2 * delay);
>> + delay = 1000000 / sensor->odr;
>> + usleep_range(delay, 2 * delay);
>> + }
>>
>> err = regmap_bulk_read(hw->regmap, addr, &data, sizeof(data));
>> if (err < 0)
>> return err;
>>
>> - st_lsm6dsx_sensor_disable(sensor);
>> + if (!hw->enable_event)
>> + st_lsm6dsx_sensor_disable(sensor);
>>
>> *val = (s16)le16_to_cpu(data);
>>
>> @@ -556,6 +593,75 @@ static int st_lsm6dsx_write_raw(struct iio_dev *iio_dev,
>> return err;
>> }
>>
>> +static int st_lsm6dsx_read_event(struct iio_dev *iio_dev,
>> + const struct iio_chan_spec *chan,
>> + enum iio_event_type type,
>> + enum iio_event_direction dir,
>> + enum iio_event_info info,
>> + int *val, int *val2)
>> +{
>> + struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
>> +
>> + *val2 = 0;
>> + *val = sensor->hw->event_threshold;
>> +
>> + return IIO_VAL_INT;
>> +}
>> +
>> +static int st_lsm6dsx_write_event(struct iio_dev *iio_dev,
>> + const struct iio_chan_spec *chan,
>> + enum iio_event_type type,
>> + enum iio_event_direction dir,
>> + enum iio_event_info info,
>> + int val, int val2)
>> +{
>> + struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
>> + struct st_lsm6dsx_hw *hw = sensor->hw;
>> +
>> + if (!hw->enable_event)
>> + return -EBUSY;
>> +
>> + if ((val < 0) || (val > 31))
>
> unnecessary brackets
Will remove them :-)
>
>> + return -EINVAL;
>> +
>> + if (st_lsm6dsx_set_event_threshold(sensor->hw, val))
>> + return -EINVAL;
>> +
>> + return 0;
>> +}
>> +
>> +static int st_lsm6dsx_read_event_config(struct iio_dev *iio_dev,
>> + const struct iio_chan_spec *chan,
>> + enum iio_event_type type,
>> + enum iio_event_direction dir)
>> +{
>> + struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
>> + struct st_lsm6dsx_hw *hw = sensor->hw;
>> +
>> + return hw->enable_event;
>> +}
>> +
>> +static int st_lsm6dsx_write_event_config(struct iio_dev *iio_dev,
>> + const struct iio_chan_spec *chan,
>> + enum iio_event_type type,
>> + enum iio_event_direction dir,
>> + int state)
>> +{
>> + struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
>> + struct st_lsm6dsx_hw *hw = sensor->hw;
>> +
>> + if (state && hw->enable_event)
>> + return 0;
>> +
>> + hw->enable_event = state;
>> + if (state)
>> + st_lsm6dsx_sensor_enable(sensor);
>
> please correct me if I am wrong but in this way we break normal operation (e.g.
> if we are reading acc data from the FIFO). You need to check enabled_mask.
Will change to the enabled_mask
> Moreover we should check event type
Do we, we only have one type?
>
>> + else
>> + st_lsm6dsx_sensor_disable(sensor);
>> +
>> + return 0;
>> +}
>> +
>> static int st_lsm6dsx_set_watermark(struct iio_dev *iio_dev, unsigned int val)
>> {
>> struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
>> @@ -632,6 +738,10 @@ static const struct iio_info st_lsm6dsx_acc_info = {
>> .attrs = &st_lsm6dsx_acc_attribute_group,
>> .read_raw = st_lsm6dsx_read_raw,
>> .write_raw = st_lsm6dsx_write_raw,
>> + .read_event_value = st_lsm6dsx_read_event,
>> + .write_event_value = st_lsm6dsx_write_event,
>> + .read_event_config = st_lsm6dsx_read_event_config,
>> + .write_event_config = st_lsm6dsx_write_event_config,
>> .hwfifo_set_watermark = st_lsm6dsx_set_watermark,
>> };
>>
>> @@ -761,6 +871,8 @@ static int st_lsm6dsx_init_device(struct st_lsm6dsx_hw *hw)
>> if (err < 0)
>> return err;
>>
>> + st_lsm6dsx_set_event_threshold(hw, ST_LSM6DSX_DEFAULT_WAKE_THRESH);
>
> we do not need this since default value is already 0
I actually think we do need it, if we come up from a reboot we don't
know the value stored in the register.
>
>> +
>> return st_lsm6dsx_init_hw_timer(hw);
>> }
>>
>> @@ -811,6 +923,27 @@ static struct iio_dev *st_lsm6dsx_alloc_iiodev(struct st_lsm6dsx_hw *hw,
>> return iio_dev;
>> }
>>
>> +int st_lsm6dsx_event_setup(struct st_lsm6dsx_hw *hw)
>> +{
>> + int err;
>> +
>> + /* Enable inactivity function - low power ACC, GYRO powered-down */
>> + err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_TAP_CFG_ADDR,
>> + ST_LSM6DSX_REG_TAP_CFG_INT_EN_MASK |
>> + ST_LSM6DSX_REG_TAP_CFG_INACT_EN_MASK,
>> + ST_LSM6DSX_REG_TAP_CFG_INT_EN_MASK |
>> + ST_LSM6DSX_REG_TAP_CFG_INACT_EN_MASK);
>> + if (err < 0)
>> + return err;
>> +
>> + /* Enable wakeup interrupt */
>> + err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_MD1_CFG_ADDR,
>> + ST_LSM6DSX_REG_MD1_CFG_INT1_WU_MASK,
>> + ST_LSM6DSX_REG_MD1_CFG_INT1_WU_MASK);
>> +
>> + return err;
>> +}
>> +
>> static irqreturn_t st_lsm6dsx_handler_irq(int irq, void *private)
>> {
>> struct st_lsm6dsx_hw *hw = private;
>> @@ -932,6 +1065,9 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
>> err = st_lsm6dsx_fifo_setup(hw);
>> if (err < 0)
>> return err;
>> + err = st_lsm6dsx_event_setup(hw);
>> + if (err < 0)
>> + return err;
>> }
>>
>> for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
>> --
>> 2.22.0
>>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/5] iio: imu: st_lsm6dsx: add motion events
2019-06-18 19:14 ` Sean Nyekjaer
@ 2019-06-18 20:21 ` Lorenzo Bianconi
2019-06-19 6:30 ` Sean Nyekjaer
0 siblings, 1 reply; 23+ messages in thread
From: Lorenzo Bianconi @ 2019-06-18 20:21 UTC (permalink / raw)
To: Sean Nyekjaer; +Cc: linux-iio, jic23, lorenzo.bianconi83, martin
[-- Attachment #1: Type: text/plain, Size: 10669 bytes --]
On Jun 18, Sean Nyekjaer wrote:
>
>
> On 18/06/2019 17.49, Lorenzo Bianconi wrote:
> > > Add event channels that controls the creation of motion events.
> > >
> > > Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> > > ---
> > > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h | 2 +
> > > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 148 ++++++++++++++++++-
> > > 2 files changed, 144 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> > > index a5e373680e9c..966cc6e91c7f 100644
> > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> > > @@ -155,6 +155,8 @@ struct st_lsm6dsx_hw {
> > > u8 enable_mask;
> > > u8 ts_sip;
> > > u8 sip;
> > > + u8 event_threshold;
> > > + bool enable_event;
> > > u8 *buff;
> > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > index b5d3fa354de7..351c46f01662 100644
> > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > @@ -78,6 +78,17 @@
> > > #define ST_LSM6DSX_REG_GYRO_OUT_Y_L_ADDR 0x24
> > > #define ST_LSM6DSX_REG_GYRO_OUT_Z_L_ADDR 0x26
> > > +#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)
> > > +
> > > +#define ST_LSM6DSX_DEFAULT_WAKE_THRESH 0
> > > +#define ST_LSM6DSX_REG_WAKE_UP_THS_ADDR 0x5B
> > > +#define ST_LSM6DSX_REG_WAKE_UP_THS_THRES_MASK GENMASK(5, 0)
> >
> > Could you please verify this registermap is used for all supported devices?
> > (e.g. LSM6DS3 or LSM6DS3H)
>
> They have the same registers and the bits meaning the same.
> But I can't test on those devices...
I do think so since BIT(7) of 0x58 TAP_CFG in LSM6DS3/LSM6DS3H is used to
enable hw timestamp...moreover I think there are other differences
>
> >
> > > +
> > > +#define ST_LSM6DSX_REG_MD1_CFG_ADDR 0x5E
> > > +#define ST_LSM6DSX_REG_MD1_CFG_INT1_WU_MASK BIT(5)
> > > +
> > > #define ST_LSM6DSX_ACC_FS_2G_GAIN IIO_G_TO_M_S_2(61)
> > > #define ST_LSM6DSX_ACC_FS_4G_GAIN IIO_G_TO_M_S_2(122)
> > > #define ST_LSM6DSX_ACC_FS_8G_GAIN IIO_G_TO_M_S_2(244)
> > > @@ -303,6 +314,13 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
> > > },
> > > };
> > > +static const struct iio_event_spec st_lsm6dsx_event = {
> > > + .type = IIO_EV_TYPE_THRESH,
> > > + .dir = IIO_EV_DIR_EITHER,
> > > + .mask_separate = BIT(IIO_EV_INFO_VALUE) |
> > > + BIT(IIO_EV_INFO_ENABLE)
> > > +};
> > > +
> > > #define ST_LSM6DSX_CHANNEL(chan_type, addr, mod, scan_idx) \
> > > { \
> > > .type = chan_type, \
> > > @@ -319,6 +337,8 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
> > > .storagebits = 16, \
> > > .endianness = IIO_LE, \
> > > }, \
> > > + .event_spec = &st_lsm6dsx_event, \
> > > + .num_event_specs = 1, \
> > > }
> >
> > ST_LSM6DSX_CHANNEL is used even for sensor-hub so I do not think you can use in
> > this way
>
> I see, will need to come up with a solution to avoid the event channels on
> the gyro.
>
> >
> > > static const struct iio_chan_spec st_lsm6dsx_acc_channels[] = {
> > > @@ -435,6 +455,20 @@ static int st_lsm6dsx_set_odr(struct st_lsm6dsx_sensor *sensor, u16 odr)
> > > ST_LSM6DSX_SHIFT_VAL(val, reg->mask));
> > > }
> > > +static int st_lsm6dsx_set_event_threshold(struct st_lsm6dsx_hw *hw, u8 threshold)
> > > +{
> > > + int err = 0;
> > > +
> > > + err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_WAKE_UP_THS_ADDR,
> > > + ST_LSM6DSX_REG_WAKE_UP_THS_THRES_MASK,
> > > + threshold);
> > > +
> > > + if (!err)
> > > + hw->event_threshold = threshold;
> > > +
> > > + return err;
> >
> > I think we can just move this configuration in st_lsm6dsx_write_event()
>
> We could, should I then call st_lsm6dsx_write_event() from the probe
> function?
nope, just run regmap_update_bits() in st_lsm6dsx_write_event (You do not need
to call st_lsm6dsx_set_event_threshold during the probe since the default value
is already 0)
>
> >
> > > +}
> > > +
> > > int st_lsm6dsx_sensor_enable(struct st_lsm6dsx_sensor *sensor)
> > > {
> > > int err;
> > > @@ -472,18 +506,21 @@ static int st_lsm6dsx_read_oneshot(struct st_lsm6dsx_sensor *sensor,
> > > int err, delay;
> > > __le16 data;
> > > - err = st_lsm6dsx_sensor_enable(sensor);
> > > - if (err < 0)
> > > - return err;
> > > + if (!hw->enable_event) {
> > > + err = st_lsm6dsx_sensor_enable(sensor);
> > > + if (err < 0)
> > > + return err;
> > > - delay = 1000000 / sensor->odr;
> > > - usleep_range(delay, 2 * delay);
> > > + delay = 1000000 / sensor->odr;
> > > + usleep_range(delay, 2 * delay);
> > > + }
> > > err = regmap_bulk_read(hw->regmap, addr, &data, sizeof(data));
> > > if (err < 0)
> > > return err;
> > > - st_lsm6dsx_sensor_disable(sensor);
> > > + if (!hw->enable_event)
> > > + st_lsm6dsx_sensor_disable(sensor);
> > > *val = (s16)le16_to_cpu(data);
> > > @@ -556,6 +593,75 @@ static int st_lsm6dsx_write_raw(struct iio_dev *iio_dev,
> > > return err;
> > > }
> > > +static int st_lsm6dsx_read_event(struct iio_dev *iio_dev,
> > > + const struct iio_chan_spec *chan,
> > > + enum iio_event_type type,
> > > + enum iio_event_direction dir,
> > > + enum iio_event_info info,
> > > + int *val, int *val2)
> > > +{
> > > + struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
> > > +
> > > + *val2 = 0;
> > > + *val = sensor->hw->event_threshold;
> > > +
> > > + return IIO_VAL_INT;
> > > +}
> > > +
> > > +static int st_lsm6dsx_write_event(struct iio_dev *iio_dev,
> > > + const struct iio_chan_spec *chan,
> > > + enum iio_event_type type,
> > > + enum iio_event_direction dir,
> > > + enum iio_event_info info,
> > > + int val, int val2)
> > > +{
> > > + struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
> > > + struct st_lsm6dsx_hw *hw = sensor->hw;
> > > +
> > > + if (!hw->enable_event)
> > > + return -EBUSY;
> > > +
> > > + if ((val < 0) || (val > 31))
> >
> > unnecessary brackets
>
> Will remove them :-)
>
> >
> > > + return -EINVAL;
> > > +
> > > + if (st_lsm6dsx_set_event_threshold(sensor->hw, val))
> > > + return -EINVAL;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int st_lsm6dsx_read_event_config(struct iio_dev *iio_dev,
> > > + const struct iio_chan_spec *chan,
> > > + enum iio_event_type type,
> > > + enum iio_event_direction dir)
> > > +{
> > > + struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
> > > + struct st_lsm6dsx_hw *hw = sensor->hw;
> > > +
> > > + return hw->enable_event;
> > > +}
> > > +
> > > +static int st_lsm6dsx_write_event_config(struct iio_dev *iio_dev,
> > > + const struct iio_chan_spec *chan,
> > > + enum iio_event_type type,
> > > + enum iio_event_direction dir,
> > > + int state)
> > > +{
> > > + struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
> > > + struct st_lsm6dsx_hw *hw = sensor->hw;
> > > +
> > > + if (state && hw->enable_event)
> > > + return 0;
> > > +
> > > + hw->enable_event = state;
> > > + if (state)
> > > + st_lsm6dsx_sensor_enable(sensor);
> >
> > please correct me if I am wrong but in this way we break normal operation (e.g.
> > if we are reading acc data from the FIFO). You need to check enabled_mask.
>
> Will change to the enabled_mask
>
> > Moreover we should check event type
>
> Do we, we only have one type?
for the moment...
>
> >
> > > + else
> > > + st_lsm6dsx_sensor_disable(sensor);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > static int st_lsm6dsx_set_watermark(struct iio_dev *iio_dev, unsigned int val)
> > > {
> > > struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
> > > @@ -632,6 +738,10 @@ static const struct iio_info st_lsm6dsx_acc_info = {
> > > .attrs = &st_lsm6dsx_acc_attribute_group,
> > > .read_raw = st_lsm6dsx_read_raw,
> > > .write_raw = st_lsm6dsx_write_raw,
> > > + .read_event_value = st_lsm6dsx_read_event,
> > > + .write_event_value = st_lsm6dsx_write_event,
> > > + .read_event_config = st_lsm6dsx_read_event_config,
> > > + .write_event_config = st_lsm6dsx_write_event_config,
> > > .hwfifo_set_watermark = st_lsm6dsx_set_watermark,
> > > };
> > > @@ -761,6 +871,8 @@ static int st_lsm6dsx_init_device(struct st_lsm6dsx_hw *hw)
> > > if (err < 0)
> > > return err;
> > > + st_lsm6dsx_set_event_threshold(hw, ST_LSM6DSX_DEFAULT_WAKE_THRESH);
> >
> > we do not need this since default value is already 0
>
> I actually think we do need it, if we come up from a reboot we don't know
> the value stored in the register.
Nope, we perform a sw reset during probe since the default value are loaded
>
> >
> > > +
> > > return st_lsm6dsx_init_hw_timer(hw);
> > > }
> > > @@ -811,6 +923,27 @@ static struct iio_dev *st_lsm6dsx_alloc_iiodev(struct st_lsm6dsx_hw *hw,
> > > return iio_dev;
> > > }
> > > +int st_lsm6dsx_event_setup(struct st_lsm6dsx_hw *hw)
> > > +{
> > > + int err;
> > > +
> > > + /* Enable inactivity function - low power ACC, GYRO powered-down */
> > > + err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_TAP_CFG_ADDR,
> > > + ST_LSM6DSX_REG_TAP_CFG_INT_EN_MASK |
> > > + ST_LSM6DSX_REG_TAP_CFG_INACT_EN_MASK,
> > > + ST_LSM6DSX_REG_TAP_CFG_INT_EN_MASK |
> > > + ST_LSM6DSX_REG_TAP_CFG_INACT_EN_MASK);
> > > + if (err < 0)
> > > + return err;
> > > +
> > > + /* Enable wakeup interrupt */
> > > + err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_MD1_CFG_ADDR,
> > > + ST_LSM6DSX_REG_MD1_CFG_INT1_WU_MASK,
> > > + ST_LSM6DSX_REG_MD1_CFG_INT1_WU_MASK);
> > > +
> > > + return err;
> > > +}
> > > +
> > > static irqreturn_t st_lsm6dsx_handler_irq(int irq, void *private)
> > > {
> > > struct st_lsm6dsx_hw *hw = private;
> > > @@ -932,6 +1065,9 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
> > > err = st_lsm6dsx_fifo_setup(hw);
> > > if (err < 0)
> > > return err;
> > > + err = st_lsm6dsx_event_setup(hw);
> > > + if (err < 0)
> > > + return err;
> > > }
> > > for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
> > > --
> > > 2.22.0
> > >
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/5] iio: imu: st_lsm6dsx: add motion events
2019-06-18 20:21 ` Lorenzo Bianconi
@ 2019-06-19 6:30 ` Sean Nyekjaer
0 siblings, 0 replies; 23+ messages in thread
From: Sean Nyekjaer @ 2019-06-19 6:30 UTC (permalink / raw)
To: Lorenzo Bianconi; +Cc: linux-iio, jic23, lorenzo.bianconi83, martin
On 18/06/2019 22.21, Lorenzo Bianconi wrote:
>>>
>>> Could you please verify this registermap is used for all supported devices?
>>> (e.g. LSM6DS3 or LSM6DS3H)
>>
>> They have the same registers and the bits meaning the same.
>> But I can't test on those devices...
>
> I do think so since BIT(7) of 0x58 TAP_CFG in LSM6DS3/LSM6DS3H is used to
> enable hw timestamp...moreover I think there are other differences
>
The LSM6DS3/LSM6DS3H have the inactivity function enable in
WAKE_UP_THS 0x5B, BIT(6), ISM330 have that in TAP_CFG 0x58.
The rest of the WAKE_UP_THS have the same register layout
WAKE_UP_SRC 0x1B have the same register layout
MD1_CFG 0x5E have the same register layout
/Sean
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 3/5] iio: imu: st_lsm6dsx: add wakeup-source option
2019-06-18 12:59 [PATCH 0/5] iio: imu: st_lsm6dsx: add event reporting and wakeup Sean Nyekjaer
2019-06-18 12:59 ` [PATCH 1/5] iio: imu: st_lsm6dsx: move interrupt thread to core Sean Nyekjaer
2019-06-18 12:59 ` [PATCH 2/5] iio: imu: st_lsm6dsx: add motion events Sean Nyekjaer
@ 2019-06-18 12:59 ` Sean Nyekjaer
2019-06-18 15:51 ` Lorenzo Bianconi
2019-06-18 12:59 ` [PATCH 4/5] iio: imu: st_lsm6dsx: always enter interrupt thread Sean Nyekjaer
2019-06-18 12:59 ` [PATCH 5/5] iio: imu: st_lsm6dsx: add motion report function and call from interrupt Sean Nyekjaer
4 siblings, 1 reply; 23+ messages in thread
From: Sean Nyekjaer @ 2019-06-18 12:59 UTC (permalink / raw)
To: linux-iio, jic23; +Cc: Sean Nyekjaer, lorenzo.bianconi83, martin
this add ways for the SoC to wake from accelerometer wake events.
In the suspend function we skip disabling the sensor if wakeup-source
and events are activated.
Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index 351c46f01662..59a34894e495 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -1076,6 +1076,10 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
return err;
}
+ if (dev->of_node)
+ if (of_property_read_bool(dev->of_node, "wakeup-source"))
+ device_init_wakeup(dev, true);
+
return 0;
}
EXPORT_SYMBOL(st_lsm6dsx_probe);
@@ -1088,6 +1092,12 @@ static int __maybe_unused st_lsm6dsx_suspend(struct device *dev)
int i, err = 0;
for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
+ if (device_may_wakeup(dev) && (i == ST_LSM6DSX_ID_ACC)) {
+ /* Enable wake from IRQ */
+ enable_irq_wake(hw->irq);
+ continue;
+ }
+
sensor = iio_priv(hw->iio_devs[i]);
if (!(hw->enable_mask & BIT(sensor->id)))
continue;
@@ -1112,6 +1122,11 @@ static int __maybe_unused st_lsm6dsx_resume(struct device *dev)
int i, err = 0;
for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
+ if (device_may_wakeup(dev) && (i == ST_LSM6DSX_ID_ACC)) {
+ disable_irq_wake(hw->irq);
+ continue;
+ }
+
sensor = iio_priv(hw->iio_devs[i]);
if (!(hw->enable_mask & BIT(sensor->id)))
continue;
--
2.22.0
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 3/5] iio: imu: st_lsm6dsx: add wakeup-source option
2019-06-18 12:59 ` [PATCH 3/5] iio: imu: st_lsm6dsx: add wakeup-source option Sean Nyekjaer
@ 2019-06-18 15:51 ` Lorenzo Bianconi
2019-06-18 19:19 ` Sean Nyekjaer
0 siblings, 1 reply; 23+ messages in thread
From: Lorenzo Bianconi @ 2019-06-18 15:51 UTC (permalink / raw)
To: Sean Nyekjaer; +Cc: linux-iio, jic23, lorenzo.bianconi83, martin
[-- Attachment #1: Type: text/plain, Size: 1833 bytes --]
> this add ways for the SoC to wake from accelerometer wake events.
>
> In the suspend function we skip disabling the sensor if wakeup-source
> and events are activated.
>
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> index 351c46f01662..59a34894e495 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> @@ -1076,6 +1076,10 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
> return err;
> }
>
> + if (dev->of_node)
> + if (of_property_read_bool(dev->of_node, "wakeup-source"))
> + device_init_wakeup(dev, true);
> +
> return 0;
> }
> EXPORT_SYMBOL(st_lsm6dsx_probe);
> @@ -1088,6 +1092,12 @@ static int __maybe_unused st_lsm6dsx_suspend(struct device *dev)
> int i, err = 0;
>
> for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
> + if (device_may_wakeup(dev) && (i == ST_LSM6DSX_ID_ACC)) {
> + /* Enable wake from IRQ */
> + enable_irq_wake(hw->irq);
> + continue;
This is breaking buffering mode
> + }
> +
> sensor = iio_priv(hw->iio_devs[i]);
> if (!(hw->enable_mask & BIT(sensor->id)))
> continue;
> @@ -1112,6 +1122,11 @@ static int __maybe_unused st_lsm6dsx_resume(struct device *dev)
> int i, err = 0;
>
> for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
> + if (device_may_wakeup(dev) && (i == ST_LSM6DSX_ID_ACC)) {
> + disable_irq_wake(hw->irq);
same here
> + continue;
> + }
> +
> sensor = iio_priv(hw->iio_devs[i]);
> if (!(hw->enable_mask & BIT(sensor->id)))
> continue;
> --
> 2.22.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/5] iio: imu: st_lsm6dsx: add wakeup-source option
2019-06-18 15:51 ` Lorenzo Bianconi
@ 2019-06-18 19:19 ` Sean Nyekjaer
2019-06-18 20:27 ` Lorenzo Bianconi
0 siblings, 1 reply; 23+ messages in thread
From: Sean Nyekjaer @ 2019-06-18 19:19 UTC (permalink / raw)
To: Lorenzo Bianconi; +Cc: linux-iio, jic23, lorenzo.bianconi83, martin
On 18/06/2019 17.51, Lorenzo Bianconi wrote:
>> this add ways for the SoC to wake from accelerometer wake events.
>>
>> In the suspend function we skip disabling the sensor if wakeup-source
>> and events are activated.
>>
>> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
>> ---
>> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 15 +++++++++++++++
>> 1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> index 351c46f01662..59a34894e495 100644
>> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> @@ -1076,6 +1076,10 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
>> return err;
>> }
>>
>> + if (dev->of_node)
>> + if (of_property_read_bool(dev->of_node, "wakeup-source"))
>> + device_init_wakeup(dev, true);
>> +
>> return 0;
>> }
>> EXPORT_SYMBOL(st_lsm6dsx_probe);
>> @@ -1088,6 +1092,12 @@ static int __maybe_unused st_lsm6dsx_suspend(struct device *dev)
>> int i, err = 0;
>>
>> for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
>> + if (device_may_wakeup(dev) && (i == ST_LSM6DSX_ID_ACC)) {
>> + /* Enable wake from IRQ */
>> + enable_irq_wake(hw->irq);
>> + continue;
>
> This is breaking buffering mode
Hmm, what is missing? :-)
We need the accelerometer to continue to run in suspend, but skip
writing to the internal fifo.
>
>> + }
>> +
>> sensor = iio_priv(hw->iio_devs[i]);
>> if (!(hw->enable_mask & BIT(sensor->id)))
>> continue;
>> @@ -1112,6 +1122,11 @@ static int __maybe_unused st_lsm6dsx_resume(struct device *dev)
>> int i, err = 0;
>>
>> for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
>> + if (device_may_wakeup(dev) && (i == ST_LSM6DSX_ID_ACC)) {
>> + disable_irq_wake(hw->irq);
>
> same here
>
>> + continue;
>> + }
>> +
>> sensor = iio_priv(hw->iio_devs[i]);
>> if (!(hw->enable_mask & BIT(sensor->id)))
>> continue;
>> --
>> 2.22.0
>>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/5] iio: imu: st_lsm6dsx: add wakeup-source option
2019-06-18 19:19 ` Sean Nyekjaer
@ 2019-06-18 20:27 ` Lorenzo Bianconi
2019-07-01 11:06 ` Sean Nyekjaer
0 siblings, 1 reply; 23+ messages in thread
From: Lorenzo Bianconi @ 2019-06-18 20:27 UTC (permalink / raw)
To: Sean Nyekjaer; +Cc: linux-iio, jic23, lorenzo.bianconi83, martin
[-- Attachment #1: Type: text/plain, Size: 2377 bytes --]
On Jun 18, Sean Nyekjaer wrote:
>
>
> On 18/06/2019 17.51, Lorenzo Bianconi wrote:
> > > this add ways for the SoC to wake from accelerometer wake events.
> > >
> > > In the suspend function we skip disabling the sensor if wakeup-source
> > > and events are activated.
> > >
> > > Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> > > ---
> > > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 15 +++++++++++++++
> > > 1 file changed, 15 insertions(+)
> > >
> > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > index 351c46f01662..59a34894e495 100644
> > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > @@ -1076,6 +1076,10 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
> > > return err;
> > > }
> > > + if (dev->of_node)
> > > + if (of_property_read_bool(dev->of_node, "wakeup-source"))
> > > + device_init_wakeup(dev, true);
> > > +
> > > return 0;
> > > }
> > > EXPORT_SYMBOL(st_lsm6dsx_probe);
> > > @@ -1088,6 +1092,12 @@ static int __maybe_unused st_lsm6dsx_suspend(struct device *dev)
> > > int i, err = 0;
> > > for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
> > > + if (device_may_wakeup(dev) && (i == ST_LSM6DSX_ID_ACC)) {
> > > + /* Enable wake from IRQ */
> > > + enable_irq_wake(hw->irq);
> > > + continue;
> >
> > This is breaking buffering mode
>
> Hmm, what is missing? :-)
> We need the accelerometer to continue to run in suspend, but skip writing to
> the internal fifo.
I think we should keep the accel enabled, but we need to put the FIFO in bypas
mode
>
> >
> > > + }
> > > +
> > > sensor = iio_priv(hw->iio_devs[i]);
> > > if (!(hw->enable_mask & BIT(sensor->id)))
> > > continue;
> > > @@ -1112,6 +1122,11 @@ static int __maybe_unused st_lsm6dsx_resume(struct device *dev)
> > > int i, err = 0;
> > > for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
> > > + if (device_may_wakeup(dev) && (i == ST_LSM6DSX_ID_ACC)) {
> > > + disable_irq_wake(hw->irq);
> >
> > same here
> >
> > > + continue;
> > > + }
> > > +
> > > sensor = iio_priv(hw->iio_devs[i]);
> > > if (!(hw->enable_mask & BIT(sensor->id)))
> > > continue;
> > > --
> > > 2.22.0
> > >
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/5] iio: imu: st_lsm6dsx: add wakeup-source option
2019-06-18 20:27 ` Lorenzo Bianconi
@ 2019-07-01 11:06 ` Sean Nyekjaer
0 siblings, 0 replies; 23+ messages in thread
From: Sean Nyekjaer @ 2019-07-01 11:06 UTC (permalink / raw)
To: Lorenzo Bianconi; +Cc: linux-iio, jic23, lorenzo.bianconi83, martin
On 18/06/2019 22.27, Lorenzo Bianconi wrote:
> On Jun 18, Sean Nyekjaer wrote:
>>
>>
>> On 18/06/2019 17.51, Lorenzo Bianconi wrote:
>>>> this add ways for the SoC to wake from accelerometer wake events.
>>>>
>>>> In the suspend function we skip disabling the sensor if wakeup-source
>>>> and events are activated.
>>>>
>>>> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
>>>> ---
>>>> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 15 +++++++++++++++
>>>> 1 file changed, 15 insertions(+)
>>>>
>>>> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>>>> index 351c46f01662..59a34894e495 100644
>>>> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>>>> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>>>> @@ -1076,6 +1076,10 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
>>>> return err;
>>>> }
>>>> + if (dev->of_node)
>>>> + if (of_property_read_bool(dev->of_node, "wakeup-source"))
>>>> + device_init_wakeup(dev, true);
>>>> +
>>>> return 0;
>>>> }
>>>> EXPORT_SYMBOL(st_lsm6dsx_probe);
>>>> @@ -1088,6 +1092,12 @@ static int __maybe_unused st_lsm6dsx_suspend(struct device *dev)
>>>> int i, err = 0;
>>>> for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
>>>> + if (device_may_wakeup(dev) && (i == ST_LSM6DSX_ID_ACC)) {
>>>> + /* Enable wake from IRQ */
>>>> + enable_irq_wake(hw->irq);
>>>> + continue;
>>>
>>> This is breaking buffering mode
>>
>> Hmm, what is missing? :-)
>> We need the accelerometer to continue to run in suspend, but skip writing to
>> the internal fifo.
>
> I think we should keep the accel enabled, but we need to put the FIFO in bypas
> mode
>
I think it's exactly whats happening here, we keep the accel enabled and
set the FIFO in bypass mode after.
st_lsm6dsx_flush_fifo (which sets the bypass mode) is called after the
for loop.
/Sean
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 4/5] iio: imu: st_lsm6dsx: always enter interrupt thread
2019-06-18 12:59 [PATCH 0/5] iio: imu: st_lsm6dsx: add event reporting and wakeup Sean Nyekjaer
` (2 preceding siblings ...)
2019-06-18 12:59 ` [PATCH 3/5] iio: imu: st_lsm6dsx: add wakeup-source option Sean Nyekjaer
@ 2019-06-18 12:59 ` Sean Nyekjaer
2019-06-18 15:55 ` Lorenzo Bianconi
2019-06-18 12:59 ` [PATCH 5/5] iio: imu: st_lsm6dsx: add motion report function and call from interrupt Sean Nyekjaer
4 siblings, 1 reply; 23+ messages in thread
From: Sean Nyekjaer @ 2019-06-18 12:59 UTC (permalink / raw)
To: linux-iio, jic23; +Cc: Sean Nyekjaer, lorenzo.bianconi83, martin
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;
}
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;
}
--
2.22.0
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 4/5] iio: imu: st_lsm6dsx: always enter interrupt thread
2019-06-18 12:59 ` [PATCH 4/5] iio: imu: st_lsm6dsx: always enter interrupt thread Sean Nyekjaer
@ 2019-06-18 15:55 ` Lorenzo Bianconi
2019-06-18 19:31 ` Sean Nyekjaer
0 siblings, 1 reply; 23+ messages in thread
From: Lorenzo Bianconi @ 2019-06-18 15:55 UTC (permalink / raw)
To: Sean Nyekjaer; +Cc: linux-iio, jic23, lorenzo.bianconi83, martin
[-- Attachment #1: Type: text/plain, Size: 2323 bytes --]
> 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
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/5] iio: imu: st_lsm6dsx: always enter interrupt thread
2019-06-18 15:55 ` Lorenzo Bianconi
@ 2019-06-18 19:31 ` Sean Nyekjaer
2019-06-18 20:24 ` Lorenzo Bianconi
0 siblings, 1 reply; 23+ messages in thread
From: Sean Nyekjaer @ 2019-06-18 19:31 UTC (permalink / raw)
To: Lorenzo Bianconi; +Cc: linux-iio, jic23, lorenzo.bianconi83, martin
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...
--
Best regards,
Sean Nyekjær
Embedded Linux Consultant
+45 42427326
sean@geanix.com
Geanix ApS
https://geanix.com
DK39600706
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/5] iio: imu: st_lsm6dsx: always enter interrupt thread
2019-06-18 19:31 ` Sean Nyekjaer
@ 2019-06-18 20:24 ` Lorenzo Bianconi
2019-06-22 9:55 ` Jonathan Cameron
0 siblings, 1 reply; 23+ messages in thread
From: Lorenzo Bianconi @ 2019-06-18 20:24 UTC (permalink / raw)
To: Sean Nyekjaer; +Cc: linux-iio, jic23, lorenzo.bianconi83, martin
[-- Attachment #1: Type: text/plain, Size: 2794 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/5] iio: imu: st_lsm6dsx: always enter interrupt thread
2019-06-18 20:24 ` Lorenzo Bianconi
@ 2019-06-22 9:55 ` Jonathan Cameron
2019-06-22 12:23 ` Lorenzo Bianconi
0 siblings, 1 reply; 23+ messages in thread
From: Jonathan Cameron @ 2019-06-22 9:55 UTC (permalink / raw)
To: Lorenzo Bianconi; +Cc: Sean Nyekjaer, linux-iio, lorenzo.bianconi83, martin
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/5] iio: imu: st_lsm6dsx: always enter interrupt thread
2019-06-22 9:55 ` Jonathan Cameron
@ 2019-06-22 12:23 ` Lorenzo Bianconi
0 siblings, 0 replies; 23+ messages in thread
From: Lorenzo Bianconi @ 2019-06-22 12:23 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: Lorenzo Bianconi, Sean Nyekjaer, linux-iio, martin
>
> 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
>
--
UNIX is Sexy: who | grep -i blonde | talk; cd ~; wine; talk; touch;
unzip; touch; strip; gasp; finger; gasp; mount; fsck; more; yes; gasp;
umount; make clean; sleep
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 5/5] iio: imu: st_lsm6dsx: add motion report function and call from interrupt
2019-06-18 12:59 [PATCH 0/5] iio: imu: st_lsm6dsx: add event reporting and wakeup Sean Nyekjaer
` (3 preceding siblings ...)
2019-06-18 12:59 ` [PATCH 4/5] iio: imu: st_lsm6dsx: always enter interrupt thread Sean Nyekjaer
@ 2019-06-18 12:59 ` Sean Nyekjaer
4 siblings, 0 replies; 23+ messages in thread
From: Sean Nyekjaer @ 2019-06-18 12:59 UTC (permalink / raw)
To: linux-iio, jic23; +Cc: Sean Nyekjaer, lorenzo.bianconi83, martin
Report iio motion events to iio subsystem
Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 36 ++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index 76aec5024d83..7b66799acf4d 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -34,6 +34,7 @@
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/delay.h>
+#include <linux/iio/events.h>
#include <linux/iio/iio.h>
#include <linux/iio/sysfs.h>
#include <linux/interrupt.h>
@@ -949,6 +950,39 @@ int st_lsm6dsx_event_setup(struct st_lsm6dsx_hw *hw)
return err;
}
+int st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw, int data)
+{
+ s64 timestamp = iio_get_time_ns(hw->iio_devs[ST_LSM6DSX_ID_ACC]);
+
+ if (data & ST_LSM6DSX_REG_WAKE_UP_SRC_Z_WU_MASK)
+ iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
+ IIO_MOD_EVENT_CODE(IIO_ACCEL,
+ 0,
+ IIO_MOD_Z,
+ IIO_EV_TYPE_THRESH,
+ IIO_EV_DIR_EITHER),
+ timestamp);
+
+ if (data & ST_LSM6DSX_REG_WAKE_UP_SRC_Y_WU_MASK)
+ iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
+ IIO_MOD_EVENT_CODE(IIO_ACCEL,
+ 0,
+ IIO_MOD_Y,
+ IIO_EV_TYPE_THRESH,
+ IIO_EV_DIR_EITHER),
+ timestamp);
+
+ if (data & ST_LSM6DSX_REG_WAKE_UP_SRC_X_WU_MASK)
+ iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
+ IIO_MOD_EVENT_CODE(IIO_ACCEL,
+ 0,
+ IIO_MOD_X,
+ IIO_EV_TYPE_THRESH,
+ IIO_EV_DIR_EITHER),
+ timestamp);
+
+ return 0;
+}
static irqreturn_t st_lsm6dsx_handler_irq(int irq, void *private)
{
@@ -967,6 +1001,8 @@ static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
if (err < 0)
goto try_fifo;
+ if (data && ST_LSM6DSX_REG_WAKE_UP_SRC_WU_MASK)
+ st_lsm6dsx_report_motion_event(hw, data);
}
try_fifo:
--
2.22.0
^ permalink raw reply related [flat|nested] 23+ messages in thread