[RFC PATCH 0/3] io: imu: st_lsm6dsx: wake on acc event

[RFC PATCH 0/3] io: imu: st_lsm6dsx: wake on acc event

[Sean Nyekjaer]

Hi,

The first patch enables the wake event creation in the suspend function,
it hardcodes the accelerometer to low power mode and the gyro is powered down.

The second and third patch is where I have some questions.
Is it okay to create an sysfs entry that can enable and disable the wake
events from the accelerometer?

The third patch is enabling us to set the threshold value.
Obviously this will need to be changed to represent a real value instead
of the raw register value.
Maybe I need to add a threshold avaliable sysfs entry?
Do I set it to a raw value calculated from the scale value or is some have
a better idea?

Finally is this the right approach to enable wake on accelerometer
events?
Please provide some idea's to how we could do it in the best and most
generic way.

/Sean

Sean Nyekjaer (3):
  iio: imu: st_lsm6dsx: add wake on accelerometer threshold
  iio: imu: st_lsm6dsx: add wake on accelerometer enable hook in sysfs
  iio: imu: st_lsm6dsx: add wake on accelerometer threshold hook in
    sysfs

 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |   2 +
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 135 +++++++++++++++++++
 2 files changed, 137 insertions(+)

-- 
2.22.0

[RFC PATCH 1/3] iio: imu: st_lsm6dsx: add wake on accelerometer threshold

[Sean Nyekjaer]

Added wakeup-source option for waking on accelerometer events.
If the wakeup-source is specified, we activate this on our
way down to suspend. We start the accelerometer in
low power mode and wait for it to report a wake event.

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 | 70 ++++++++++++++++++++
 2 files changed, 72 insertions(+)

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
index edcd838037cd..ef4327fd57d6 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
@@ -147,6 +147,7 @@ struct st_lsm6dsx_hw {
 	struct device *dev;
 	struct regmap *regmap;
 	int irq;
+	bool irq_wake;
 
 	struct mutex fifo_lock;
 	struct mutex conf_lock;
@@ -155,6 +156,7 @@ struct st_lsm6dsx_hw {
 	u8 enable_mask;
 	u8 ts_sip;
 	u8 sip;
+	u8 wake_threshold;
 
 	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 aebbe0ddd8d8..092c4d02bd4e 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -36,6 +36,7 @@
 #include <linux/delay.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
+#include <linux/interrupt.h>
 #include <linux/pm.h>
 #include <linux/regmap.h>
 #include <linux/bitfield.h>
@@ -71,6 +72,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)
@@ -428,6 +440,19 @@ 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_wake_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->wake_threshold = threshold;
+
+	return err;
+}
+
 int st_lsm6dsx_sensor_enable(struct st_lsm6dsx_sensor *sensor)
 {
 	int err;
@@ -754,6 +779,8 @@ static int st_lsm6dsx_init_device(struct st_lsm6dsx_hw *hw)
 	if (err < 0)
 		return err;
 
+	st_lsm6dsx_set_wake_threshold(hw, ST_LSM6DSX_DEFAULT_WAKE_THRESH);
+
 	return st_lsm6dsx_init_hw_timer(hw);
 }
 
@@ -853,6 +880,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);
@@ -879,6 +910,26 @@ static int __maybe_unused st_lsm6dsx_suspend(struct device *dev)
 	if (hw->fifo_mode != ST_LSM6DSX_FIFO_BYPASS)
 		err = st_lsm6dsx_flush_fifo(hw);
 
+	if (device_may_wakeup(dev)) {
+		sensor = iio_priv(hw->iio_devs[ST_LSM6DSX_ID_ACC]);
+		err = st_lsm6dsx_sensor_enable(sensor);
+
+		/* 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);
+
+		/* 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);
+
+		/* Enable wake from IRQ */
+		hw->irq_wake = (enable_irq_wake(hw->irq) == 0);
+	}
+
 	return err;
 }
 
@@ -888,6 +939,25 @@ static int __maybe_unused st_lsm6dsx_resume(struct device *dev)
 	struct st_lsm6dsx_sensor *sensor;
 	int i, err = 0;
 
+	if (device_may_wakeup(dev) && hw->irq_wake) {
+		disable_irq_wake(hw->irq);
+		hw->irq_wake = false;
+
+		sensor = iio_priv(hw->iio_devs[ST_LSM6DSX_ID_ACC]);
+		err = st_lsm6dsx_sensor_disable(sensor);
+
+		/* Disable inactivity function */
+		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,
+				0);
+
+		/* Disable wakeup interrupt */
+		err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_MD1_CFG_ADDR,
+				ST_LSM6DSX_REG_MD1_CFG_INT1_WU_MASK,
+				0);
+	}
+
 	for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
 		sensor = iio_priv(hw->iio_devs[i]);
 		if (!(hw->enable_mask & BIT(sensor->id)))
-- 
2.22.0

Re: [RFC PATCH 1/3] iio: imu: st_lsm6dsx: add wake on accelerometer threshold

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 6019 bytes --]

> Added wakeup-source option for waking on accelerometer events.
> If the wakeup-source is specified, we activate this on our
> way down to suspend. We start the accelerometer in
> low power mode and wait for it to report a wake event.
> 

Hi Sean,

thx for working on this, some comments inline.

Regards,
Lorenzo

> 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 | 70 ++++++++++++++++++++
>  2 files changed, 72 insertions(+)
> 
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> index edcd838037cd..ef4327fd57d6 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> @@ -147,6 +147,7 @@ struct st_lsm6dsx_hw {
>  	struct device *dev;
>  	struct regmap *regmap;
>  	int irq;
> +	bool irq_wake;
>  
>  	struct mutex fifo_lock;
>  	struct mutex conf_lock;
> @@ -155,6 +156,7 @@ struct st_lsm6dsx_hw {
>  	u8 enable_mask;
>  	u8 ts_sip;
>  	u8 sip;
> +	u8 wake_threshold;
>  
>  	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 aebbe0ddd8d8..092c4d02bd4e 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> @@ -36,6 +36,7 @@
>  #include <linux/delay.h>
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> +#include <linux/interrupt.h>
>  #include <linux/pm.h>
>  #include <linux/regmap.h>
>  #include <linux/bitfield.h>
> @@ -71,6 +72,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)

have you verified these register maps is the same for all supported devices?

> @@ -428,6 +440,19 @@ 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_wake_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->wake_threshold = threshold;
> +
> +	return err;
> +}

I guess you can move it in patch 3/3

> +
>  int st_lsm6dsx_sensor_enable(struct st_lsm6dsx_sensor *sensor)
>  {
>  	int err;
> @@ -754,6 +779,8 @@ static int st_lsm6dsx_init_device(struct st_lsm6dsx_hw *hw)
>  	if (err < 0)
>  		return err;
>  
> +	st_lsm6dsx_set_wake_threshold(hw, ST_LSM6DSX_DEFAULT_WAKE_THRESH);

we do not need this since default value is 0

> +
>  	return st_lsm6dsx_init_hw_timer(hw);
>  }
>  
> @@ -853,6 +880,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);
> +

you probably need to add the equivalent for the pdata case. Maybe it is better
a dedicated routine

>  	return 0;
>  }
>  EXPORT_SYMBOL(st_lsm6dsx_probe);
> @@ -879,6 +910,26 @@ static int __maybe_unused st_lsm6dsx_suspend(struct device *dev)
>  	if (hw->fifo_mode != ST_LSM6DSX_FIFO_BYPASS)
>  		err = st_lsm6dsx_flush_fifo(hw);
>  
> +	if (device_may_wakeup(dev)) {
> +		sensor = iio_priv(hw->iio_devs[ST_LSM6DSX_ID_ACC]);
> +		err = st_lsm6dsx_sensor_enable(sensor);

better not not disable the acc in this case

> +
> +		/* 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);
> +
> +		/* 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);

I guess we can enable it by default

> +
> +		/* Enable wake from IRQ */
> +		hw->irq_wake = (enable_irq_wake(hw->irq) == 0);

do we really need this? maybe it is enough to just disable
ST_LSM6DSX_REG_TAP_CFG_ADDR in st_lsm6dsx_resume and drop irq_wake

> +	}
> +
>  	return err;
>  }
>  
> @@ -888,6 +939,25 @@ static int __maybe_unused st_lsm6dsx_resume(struct device *dev)
>  	struct st_lsm6dsx_sensor *sensor;
>  	int i, err = 0;
>  
> +	if (device_may_wakeup(dev) && hw->irq_wake) {
> +		disable_irq_wake(hw->irq);
> +		hw->irq_wake = false;
> +
> +		sensor = iio_priv(hw->iio_devs[ST_LSM6DSX_ID_ACC]);
> +		err = st_lsm6dsx_sensor_disable(sensor);

I guess this breaks the acc/sensor hub resume

> +
> +		/* Disable inactivity function */
> +		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,
> +				0);
> +
> +		/* Disable wakeup interrupt */
> +		err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_MD1_CFG_ADDR,
> +				ST_LSM6DSX_REG_MD1_CFG_INT1_WU_MASK,
> +				0);
> +	}
> +
>  	for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
>  		sensor = iio_priv(hw->iio_devs[i]);
>  		if (!(hw->enable_mask & BIT(sensor->id)))
> -- 
> 2.22.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[RFC PATCH 2/3] iio: imu: st_lsm6dsx: add wake on accelerometer enable hook in sysfs

[Sean Nyekjaer]

This adds a wakeup_enabled hook in sysfs.
If wakeup-source is enabled, wake on accelerometer event is default active.

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 31 ++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index 092c4d02bd4e..2c8ad7d65d2f 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -630,15 +630,46 @@ static ssize_t st_lsm6dsx_sysfs_scale_avail(struct device *dev,
 	return len;
 }
 
+static ssize_t st_lsm6dsx_sysfs_get_wakeup_enabled(struct device *dev,
+					    struct device_attribute *attr,
+					    char *buf)
+{
+	struct st_lsm6dsx_sensor *sensor = iio_priv(dev_get_drvdata(dev));
+	struct st_lsm6dsx_hw *hw = sensor->hw;
+
+	if (device_may_wakeup(hw->dev))
+		return sprintf(buf, "%d\n", 1);
+	return sprintf(buf, "%d\n", 0);
+}
+
+static ssize_t st_lsm6dsx_sysfs_set_wakeup_enabled(struct device *dev,
+					    struct device_attribute *attr,
+					    const char *buf, size_t len)
+{
+	struct st_lsm6dsx_sensor *sensor = iio_priv(dev_get_drvdata(dev));
+	struct st_lsm6dsx_hw *hw = sensor->hw;
+
+	if (strncmp(buf, "1", 1) == 0)
+		device_set_wakeup_enable(hw->dev, true);
+	else
+		device_set_wakeup_enable(hw->dev, false);
+
+	return len;
+}
+
 static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(st_lsm6dsx_sysfs_sampling_frequency_avail);
 static IIO_DEVICE_ATTR(in_accel_scale_available, 0444,
 		       st_lsm6dsx_sysfs_scale_avail, NULL, 0);
+static IIO_DEVICE_ATTR(wakeup_enabled, 0644,
+		       st_lsm6dsx_sysfs_get_wakeup_enabled,
+		       st_lsm6dsx_sysfs_set_wakeup_enabled, 0);
 static IIO_DEVICE_ATTR(in_anglvel_scale_available, 0444,
 		       st_lsm6dsx_sysfs_scale_avail, NULL, 0);
 
 static struct attribute *st_lsm6dsx_acc_attributes[] = {
 	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
 	&iio_dev_attr_in_accel_scale_available.dev_attr.attr,
+	&iio_dev_attr_wakeup_enabled.dev_attr.attr,
 	NULL,
 };
 
-- 
2.22.0

Re: [RFC PATCH 2/3] iio: imu: st_lsm6dsx: add wake on accelerometer enable hook in sysfs

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 2442 bytes --]

> This adds a wakeup_enabled hook in sysfs.
> If wakeup-source is enabled, wake on accelerometer event is default active.
> 
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 31 ++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 

same here, what about using write_event_value/write_event_config function
pointer?

> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> index 092c4d02bd4e..2c8ad7d65d2f 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> @@ -630,15 +630,46 @@ static ssize_t st_lsm6dsx_sysfs_scale_avail(struct device *dev,
>  	return len;
>  }
>  
> +static ssize_t st_lsm6dsx_sysfs_get_wakeup_enabled(struct device *dev,
> +					    struct device_attribute *attr,
> +					    char *buf)
> +{
> +	struct st_lsm6dsx_sensor *sensor = iio_priv(dev_get_drvdata(dev));
> +	struct st_lsm6dsx_hw *hw = sensor->hw;
> +
> +	if (device_may_wakeup(hw->dev))
> +		return sprintf(buf, "%d\n", 1);
> +	return sprintf(buf, "%d\n", 0);

what about:

return sprintf(buf, "%d\n", device_may_wakeup(hw->dev));

> +}
> +
> +static ssize_t st_lsm6dsx_sysfs_set_wakeup_enabled(struct device *dev,
> +					    struct device_attribute *attr,
> +					    const char *buf, size_t len)
> +{
> +	struct st_lsm6dsx_sensor *sensor = iio_priv(dev_get_drvdata(dev));
> +	struct st_lsm6dsx_hw *hw = sensor->hw;
> +
> +	if (strncmp(buf, "1", 1) == 0)
> +		device_set_wakeup_enable(hw->dev, true);
> +	else
> +		device_set_wakeup_enable(hw->dev, false);
> +
> +	return len;
> +}
> +
>  static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(st_lsm6dsx_sysfs_sampling_frequency_avail);
>  static IIO_DEVICE_ATTR(in_accel_scale_available, 0444,
>  		       st_lsm6dsx_sysfs_scale_avail, NULL, 0);
> +static IIO_DEVICE_ATTR(wakeup_enabled, 0644,
> +		       st_lsm6dsx_sysfs_get_wakeup_enabled,
> +		       st_lsm6dsx_sysfs_set_wakeup_enabled, 0);
>  static IIO_DEVICE_ATTR(in_anglvel_scale_available, 0444,
>  		       st_lsm6dsx_sysfs_scale_avail, NULL, 0);
>  
>  static struct attribute *st_lsm6dsx_acc_attributes[] = {
>  	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
>  	&iio_dev_attr_in_accel_scale_available.dev_attr.attr,
> +	&iio_dev_attr_wakeup_enabled.dev_attr.attr,
>  	NULL,
>  };
>  
> -- 
> 2.22.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC PATCH 2/3] iio: imu: st_lsm6dsx: add wake on accelerometer enable hook in sysfs

[Jonathan Cameron]

On Fri, 14 Jun 2019 14:26:03 +0200
Sean Nyekjaer <sean@geanix.com> wrote:

> This adds a wakeup_enabled hook in sysfs.
> If wakeup-source is enabled, wake on accelerometer event is default active.
> 
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>

This seems to replicate the stuff that should be there under ../power
to allow the wake up source to turned on and off..

> ---
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 31 ++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> index 092c4d02bd4e..2c8ad7d65d2f 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> @@ -630,15 +630,46 @@ static ssize_t st_lsm6dsx_sysfs_scale_avail(struct device *dev,
>  	return len;
>  }
>  
> +static ssize_t st_lsm6dsx_sysfs_get_wakeup_enabled(struct device *dev,
> +					    struct device_attribute *attr,
> +					    char *buf)
> +{
> +	struct st_lsm6dsx_sensor *sensor = iio_priv(dev_get_drvdata(dev));
> +	struct st_lsm6dsx_hw *hw = sensor->hw;
> +
> +	if (device_may_wakeup(hw->dev))
> +		return sprintf(buf, "%d\n", 1);
> +	return sprintf(buf, "%d\n", 0);
> +}
> +
> +static ssize_t st_lsm6dsx_sysfs_set_wakeup_enabled(struct device *dev,
> +					    struct device_attribute *attr,
> +					    const char *buf, size_t len)
> +{
> +	struct st_lsm6dsx_sensor *sensor = iio_priv(dev_get_drvdata(dev));
> +	struct st_lsm6dsx_hw *hw = sensor->hw;
> +
> +	if (strncmp(buf, "1", 1) == 0)

Can't use the kstrtobool function?  It's rather less strict
but should be unambiguous here.

> +		device_set_wakeup_enable(hw->dev, true);
> +	else
> +		device_set_wakeup_enable(hw->dev, false);
> +
> +	return len;
> +}
> +
>  static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(st_lsm6dsx_sysfs_sampling_frequency_avail);
>  static IIO_DEVICE_ATTR(in_accel_scale_available, 0444,
>  		       st_lsm6dsx_sysfs_scale_avail, NULL, 0);
> +static IIO_DEVICE_ATTR(wakeup_enabled, 0644,
> +		       st_lsm6dsx_sysfs_get_wakeup_enabled,
> +		       st_lsm6dsx_sysfs_set_wakeup_enabled, 0);
>  static IIO_DEVICE_ATTR(in_anglvel_scale_available, 0444,
>  		       st_lsm6dsx_sysfs_scale_avail, NULL, 0);
>  
>  static struct attribute *st_lsm6dsx_acc_attributes[] = {
>  	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
>  	&iio_dev_attr_in_accel_scale_available.dev_attr.attr,
> +	&iio_dev_attr_wakeup_enabled.dev_attr.attr,
>  	NULL,
>  };
>  

Re: [RFC PATCH 2/3] iio: imu: st_lsm6dsx: add wake on accelerometer enable hook in sysfs

[Sean Nyekjaer]

On 16/06/2019 15.30, Jonathan Cameron wrote:
> On Fri, 14 Jun 2019 14:26:03 +0200
> Sean Nyekjaer<sean@geanix.com>  wrote:
> 
>> This adds a wakeup_enabled hook in sysfs.
>> If wakeup-source is enabled, wake on accelerometer event is default active.
>>
>> Signed-off-by: Sean Nyekjaer<sean@geanix.com>
> This seems to replicate the stuff that should be there under ../power
> to allow the wake up source to turned on and off..
> 

Hi,

Doh, I have already done it the correct way :-)
The hook is here:

root@host:~# cat /sys/devices/soc0/***/i2c-1/1-006a/power/wakeup
enabled

/Sean

[RFC PATCH 3/3] iio: imu: st_lsm6dsx: add wake on accelerometer threshold hook in sysfs

[Sean Nyekjaer]

This adds a wakeup threshold hook in sysfs, it enables us to
change the threshold value on the run.
For now this is the raw register value...

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 34 ++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index 2c8ad7d65d2f..cbcd7920f05d 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -657,12 +657,45 @@ static ssize_t st_lsm6dsx_sysfs_set_wakeup_enabled(struct device *dev,
 	return len;
 }
 
+static ssize_t st_lsm6dsx_sysfs_get_wakeup_threshold(struct device *dev,
+					    struct device_attribute *attr,
+					    char *buf)
+{
+	struct st_lsm6dsx_sensor *sensor = iio_priv(dev_get_drvdata(dev));
+	struct st_lsm6dsx_hw *hw = sensor->hw;
+
+	return sprintf(buf, "%d\n", hw->wake_threshold);
+}
+
+static ssize_t st_lsm6dsx_sysfs_set_wakeup_threshold(struct device *dev,
+					    struct device_attribute *attr,
+					    const char *buf, size_t len)
+{
+	struct st_lsm6dsx_sensor *sensor = iio_priv(dev_get_drvdata(dev));
+	struct st_lsm6dsx_hw *hw = sensor->hw;
+	int threshold;
+
+	if (kstrtoint(buf, 0, &threshold))
+		return -EINVAL;
+
+	if ((threshold < 0) || (threshold > 31))
+		return -EINVAL;
+
+	if (!st_lsm6dsx_set_wake_threshold(hw, threshold))
+		return len;
+
+	return -EINVAL;
+}
+
 static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(st_lsm6dsx_sysfs_sampling_frequency_avail);
 static IIO_DEVICE_ATTR(in_accel_scale_available, 0444,
 		       st_lsm6dsx_sysfs_scale_avail, NULL, 0);
 static IIO_DEVICE_ATTR(wakeup_enabled, 0644,
 		       st_lsm6dsx_sysfs_get_wakeup_enabled,
 		       st_lsm6dsx_sysfs_set_wakeup_enabled, 0);
+static IIO_DEVICE_ATTR(wakeup_threshold, 0644,
+		       st_lsm6dsx_sysfs_get_wakeup_threshold,
+		       st_lsm6dsx_sysfs_set_wakeup_threshold, 0);
 static IIO_DEVICE_ATTR(in_anglvel_scale_available, 0444,
 		       st_lsm6dsx_sysfs_scale_avail, NULL, 0);
 
@@ -670,6 +703,7 @@ static struct attribute *st_lsm6dsx_acc_attributes[] = {
 	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
 	&iio_dev_attr_in_accel_scale_available.dev_attr.attr,
 	&iio_dev_attr_wakeup_enabled.dev_attr.attr,
+	&iio_dev_attr_wakeup_threshold.dev_attr.attr,
 	NULL,
 };
 
-- 
2.22.0

Re: [RFC PATCH 3/3] iio: imu: st_lsm6dsx: add wake on accelerometer threshold hook in sysfs

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 2734 bytes --]

> This adds a wakeup threshold hook in sysfs, it enables us to
> change the threshold value on the run.
> For now this is the raw register value...
> 
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 34 ++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 

What about using write_event_config routine pointer for it instead od adding a sysfs
entries?
@Jonathan: what do you think?

Regards,
Lorenzo

> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> index 2c8ad7d65d2f..cbcd7920f05d 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> @@ -657,12 +657,45 @@ static ssize_t st_lsm6dsx_sysfs_set_wakeup_enabled(struct device *dev,
>  	return len;
>  }
>  
> +static ssize_t st_lsm6dsx_sysfs_get_wakeup_threshold(struct device *dev,
> +					    struct device_attribute *attr,
> +					    char *buf)
> +{
> +	struct st_lsm6dsx_sensor *sensor = iio_priv(dev_get_drvdata(dev));
> +	struct st_lsm6dsx_hw *hw = sensor->hw;
> +
> +	return sprintf(buf, "%d\n", hw->wake_threshold);
> +}
> +
> +static ssize_t st_lsm6dsx_sysfs_set_wakeup_threshold(struct device *dev,
> +					    struct device_attribute *attr,
> +					    const char *buf, size_t len)
> +{
> +	struct st_lsm6dsx_sensor *sensor = iio_priv(dev_get_drvdata(dev));
> +	struct st_lsm6dsx_hw *hw = sensor->hw;
> +	int threshold;
> +
> +	if (kstrtoint(buf, 0, &threshold))
> +		return -EINVAL;
> +
> +	if ((threshold < 0) || (threshold > 31))
> +		return -EINVAL;
> +
> +	if (!st_lsm6dsx_set_wake_threshold(hw, threshold))
> +		return len;
> +
> +	return -EINVAL;
> +}
> +
>  static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(st_lsm6dsx_sysfs_sampling_frequency_avail);
>  static IIO_DEVICE_ATTR(in_accel_scale_available, 0444,
>  		       st_lsm6dsx_sysfs_scale_avail, NULL, 0);
>  static IIO_DEVICE_ATTR(wakeup_enabled, 0644,
>  		       st_lsm6dsx_sysfs_get_wakeup_enabled,
>  		       st_lsm6dsx_sysfs_set_wakeup_enabled, 0);
> +static IIO_DEVICE_ATTR(wakeup_threshold, 0644,
> +		       st_lsm6dsx_sysfs_get_wakeup_threshold,
> +		       st_lsm6dsx_sysfs_set_wakeup_threshold, 0);
>  static IIO_DEVICE_ATTR(in_anglvel_scale_available, 0444,
>  		       st_lsm6dsx_sysfs_scale_avail, NULL, 0);
>  
> @@ -670,6 +703,7 @@ static struct attribute *st_lsm6dsx_acc_attributes[] = {
>  	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
>  	&iio_dev_attr_in_accel_scale_available.dev_attr.attr,
>  	&iio_dev_attr_wakeup_enabled.dev_attr.attr,
> +	&iio_dev_attr_wakeup_threshold.dev_attr.attr,
>  	NULL,
>  };
>  
> -- 
> 2.22.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC PATCH 3/3] iio: imu: st_lsm6dsx: add wake on accelerometer threshold hook in sysfs

[Jonathan Cameron]

On Sat, 15 Jun 2019 10:35:58 +0200
Lorenzo Bianconi <lorenzo@kernel.org> wrote:

> > This adds a wakeup threshold hook in sysfs, it enables us to
> > change the threshold value on the run.
> > For now this is the raw register value...
> > 
> > Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> > ---
> >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 34 ++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> >   
> 
> What about using write_event_config routine pointer for it instead od adding a sysfs
> entries?
> @Jonathan: what do you think?
I must admit this support has me a little confused.

Is what is actually going on here that we are implementing generic
threshold events, but instead of reporting them as interrupts and through the
usual userspace pipeline we are using a wakeup aware gpio to bring the
system out of suspend?

If that's the case then I'd like to see these supported as normal IIO events
first, and then look at adding the general ability to use these as wakeup
events if the particular interrupt line happens to support it.  This isn't
really a thing specific to this device, but rather something that we should
be enable on any threshold event.   Clearly for a driver to support it, there
must be a means of preventing it going too deeply to sleep so each
driver is likely to need some specific handling.

To get a remotely predictable interface we might need to have
enables more directly tied to the actual events than they would be
using the power/wakeup interface. That discussion would need to
involve the power management people as well. In particular I can see
you might want different events enabled when going to sleep than
at other times, so we might to have additional wakeup specific
enable attributes, and perhaps thresholds?

I can see something like:
/sys/bus/iio/devices/iio\:device0/events/in_accel_x_thresh_rising_[wakeup]value
/sys/bus/iio/devices/iio\:device0/events/in_accel_x_thresh_rising_wakeup

Making some sense.

Jonathan

> 
> Regards,
> Lorenzo
> 
> > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > index 2c8ad7d65d2f..cbcd7920f05d 100644
> > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > @@ -657,12 +657,45 @@ static ssize_t st_lsm6dsx_sysfs_set_wakeup_enabled(struct device *dev,
> >  	return len;
> >  }
> >  
> > +static ssize_t st_lsm6dsx_sysfs_get_wakeup_threshold(struct device *dev,
> > +					    struct device_attribute *attr,
> > +					    char *buf)
> > +{
> > +	struct st_lsm6dsx_sensor *sensor = iio_priv(dev_get_drvdata(dev));
> > +	struct st_lsm6dsx_hw *hw = sensor->hw;
> > +
> > +	return sprintf(buf, "%d\n", hw->wake_threshold);
> > +}
> > +
> > +static ssize_t st_lsm6dsx_sysfs_set_wakeup_threshold(struct device *dev,
> > +					    struct device_attribute *attr,
> > +					    const char *buf, size_t len)
> > +{
> > +	struct st_lsm6dsx_sensor *sensor = iio_priv(dev_get_drvdata(dev));
> > +	struct st_lsm6dsx_hw *hw = sensor->hw;
> > +	int threshold;
> > +
> > +	if (kstrtoint(buf, 0, &threshold))
> > +		return -EINVAL;
> > +
> > +	if ((threshold < 0) || (threshold > 31))
> > +		return -EINVAL;
> > +
> > +	if (!st_lsm6dsx_set_wake_threshold(hw, threshold))
> > +		return len;
> > +
> > +	return -EINVAL;
> > +}
> > +
> >  static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(st_lsm6dsx_sysfs_sampling_frequency_avail);
> >  static IIO_DEVICE_ATTR(in_accel_scale_available, 0444,
> >  		       st_lsm6dsx_sysfs_scale_avail, NULL, 0);
> >  static IIO_DEVICE_ATTR(wakeup_enabled, 0644,
> >  		       st_lsm6dsx_sysfs_get_wakeup_enabled,
> >  		       st_lsm6dsx_sysfs_set_wakeup_enabled, 0);
> > +static IIO_DEVICE_ATTR(wakeup_threshold, 0644,
> > +		       st_lsm6dsx_sysfs_get_wakeup_threshold,
> > +		       st_lsm6dsx_sysfs_set_wakeup_threshold, 0);
> >  static IIO_DEVICE_ATTR(in_anglvel_scale_available, 0444,
> >  		       st_lsm6dsx_sysfs_scale_avail, NULL, 0);
> >  
> > @@ -670,6 +703,7 @@ static struct attribute *st_lsm6dsx_acc_attributes[] = {
> >  	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
> >  	&iio_dev_attr_in_accel_scale_available.dev_attr.attr,
> >  	&iio_dev_attr_wakeup_enabled.dev_attr.attr,
> > +	&iio_dev_attr_wakeup_threshold.dev_attr.attr,
> >  	NULL,
> >  };
> >  
> > -- 
> > 2.22.0
> >   

Re: [RFC PATCH 0/3] io: imu: st_lsm6dsx: wake on acc event

[Jonathan Cameron]

On Fri, 14 Jun 2019 14:26:01 +0200
Sean Nyekjaer <sean@geanix.com> wrote:

> Hi,
> 
> The first patch enables the wake event creation in the suspend function,
> it hardcodes the accelerometer to low power mode and the gyro is powered down.
> 
> The second and third patch is where I have some questions.
> Is it okay to create an sysfs entry that can enable and disable the wake
> events from the accelerometer?

On that I'm not sure - is there a standard way of configuring wake up events
outside of IIO?

> 
> The third patch is enabling us to set the threshold value.
> Obviously this will need to be changed to represent a real value instead
> of the raw register value.
> Maybe I need to add a threshold avaliable sysfs entry?
> Do I set it to a raw value calculated from the scale value or is some have
> a better idea?
Yes, if a device is providing a _raw channel reading then threshold
values should also be raw.

Available sysfs attribute makes sense if it helps a user, or userspace
program to set the value.

> 
> Finally is this the right approach to enable wake on accelerometer
> events?
> Please provide some idea's to how we could do it in the best and most
> generic way.

It's not something I've come across before, so hopefully someone else
can provide guidance on this!

My only immediate thought is that perhaps this should be a device tree
thing rather than userspace controlled?
There also seems to be some existing infrastructure to control this
in the power directory for a device.

Documentation/ABI/testing/sysfs-devices-power

Thanks,

Jonathan

> 
> /Sean
> 
> Sean Nyekjaer (3):
>   iio: imu: st_lsm6dsx: add wake on accelerometer threshold
>   iio: imu: st_lsm6dsx: add wake on accelerometer enable hook in sysfs
>   iio: imu: st_lsm6dsx: add wake on accelerometer threshold hook in
>     sysfs
> 
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |   2 +
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 135 +++++++++++++++++++
>  2 files changed, 137 insertions(+)
> 

Re: [RFC PATCH 0/3] io: imu: st_lsm6dsx: wake on acc event

[Jonathan Cameron]

On Sun, 16 Jun 2019 14:24:31 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri, 14 Jun 2019 14:26:01 +0200
> Sean Nyekjaer <sean@geanix.com> wrote:
> 
> > Hi,
> > 
> > The first patch enables the wake event creation in the suspend function,
> > it hardcodes the accelerometer to low power mode and the gyro is powered down.
> > 
> > The second and third patch is where I have some questions.
> > Is it okay to create an sysfs entry that can enable and disable the wake
> > events from the accelerometer?  
> 
> On that I'm not sure - is there a standard way of configuring wake up events
> outside of IIO?
> 
> > 
> > The third patch is enabling us to set the threshold value.
> > Obviously this will need to be changed to represent a real value instead
> > of the raw register value.
> > Maybe I need to add a threshold avaliable sysfs entry?
> > Do I set it to a raw value calculated from the scale value or is some have
> > a better idea?  
> Yes, if a device is providing a _raw channel reading then threshold
> values should also be raw.
> 
> Available sysfs attribute makes sense if it helps a user, or userspace
> program to set the value.
> 
> > 
> > Finally is this the right approach to enable wake on accelerometer
> > events?
> > Please provide some idea's to how we could do it in the best and most
> > generic way.  
> 
> It's not something I've come across before, so hopefully someone else
> can provide guidance on this!
> 
> My only immediate thought is that perhaps this should be a device tree
> thing rather than userspace controlled?
doh. Should have read the patches first rather than just replying to the
cover letter ;) Ignore this bit!

> There also seems to be some existing infrastructure to control this
> in the power directory for a device.
> 
> Documentation/ABI/testing/sysfs-devices-power
> 
> Thanks,
> 
> Jonathan
> 
> > 
> > /Sean
> > 
> > Sean Nyekjaer (3):
> >   iio: imu: st_lsm6dsx: add wake on accelerometer threshold
> >   iio: imu: st_lsm6dsx: add wake on accelerometer enable hook in sysfs
> >   iio: imu: st_lsm6dsx: add wake on accelerometer threshold hook in
> >     sysfs
> > 
> >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |   2 +
> >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 135 +++++++++++++++++++
> >  2 files changed, 137 insertions(+)
> >   
>