[PATCH 1/2] iio: accel: fxls8962af: add threshold event handling

[PATCH 1/2] iio: accel: fxls8962af: add threshold event handling

[Sean Nyekjaer]

Add event channels that controls the creation of motion events.

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
 drivers/iio/accel/fxls8962af-core.c | 257 +++++++++++++++++++++++++++-
 1 file changed, 255 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/accel/fxls8962af-core.c b/drivers/iio/accel/fxls8962af-core.c
index 6b36eb362d07..1b97c82b5b05 100644
--- a/drivers/iio/accel/fxls8962af-core.c
+++ b/drivers/iio/accel/fxls8962af-core.c
@@ -22,6 +22,7 @@
 #include <linux/regmap.h>
 
 #include <linux/iio/buffer.h>
+#include <linux/iio/events.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/kfifo_buf.h>
 #include <linux/iio/sysfs.h>
@@ -30,6 +31,7 @@
 
 #define FXLS8962AF_INT_STATUS			0x00
 #define FXLS8962AF_INT_STATUS_SRC_BOOT		BIT(0)
+#define FXLS8962AF_INT_STATUS_SRC_SDCD_OT	BIT(4)
 #define FXLS8962AF_INT_STATUS_SRC_BUF		BIT(5)
 #define FXLS8962AF_INT_STATUS_SRC_DRDY		BIT(7)
 #define FXLS8962AF_TEMP_OUT			0x01
@@ -73,6 +75,7 @@
 #define FXLS8962AF_ASLP_COUNT_LSB		0x1e
 
 #define FXLS8962AF_INT_EN			0x20
+#define FXLS8962AF_INT_EN_SDCD_OT_EN		BIT(5)
 #define FXLS8962AF_INT_EN_BUF_EN		BIT(6)
 #define FXLS8962AF_INT_PIN_SEL			0x21
 #define FXLS8962AF_INT_PIN_SEL_MASK		GENMASK(7, 0)
@@ -96,8 +99,14 @@
 #define FXLS8962AF_ORIENT_THS_REG		0x2c
 
 #define FXLS8962AF_SDCD_INT_SRC1		0x2d
+#define FXLS8962AF_SDCD_INT_SRC1_X_OT		BIT(5)
+#define FXLS8962AF_SDCD_INT_SRC1_Y_OT		BIT(3)
+#define FXLS8962AF_SDCD_INT_SRC1_Z_OT		BIT(1)
 #define FXLS8962AF_SDCD_INT_SRC2		0x2e
 #define FXLS8962AF_SDCD_CONFIG1			0x2f
+#define FXLS8962AF_SDCD_CONFIG1_Z_OT_EN		BIT(3)
+#define FXLS8962AF_SDCD_CONFIG1_Y_OT_EN		BIT(4)
+#define FXLS8962AF_SDCD_CONFIG1_X_OT_EN		BIT(5)
 #define FXLS8962AF_SDCD_CONFIG2			0x30
 #define FXLS8962AF_SDCD_OT_DBCNT		0x31
 #define FXLS8962AF_SDCD_WT_DBCNT		0x32
@@ -152,6 +161,9 @@ struct fxls8962af_data {
 	int64_t timestamp, old_timestamp;	/* Only used in hw fifo mode. */
 	struct iio_mount_matrix orientation;
 	u8 watermark;
+	u8 enable_event;
+	u16 lower_thres;
+	u16 upper_thres;
 };
 
 const struct regmap_config fxls8962af_regmap_conf = {
@@ -451,6 +463,18 @@ static int fxls8962af_write_raw(struct iio_dev *indio_dev,
 	}
 }
 
+static int fxls8962af_event_setup(struct fxls8962af_data *data, int state)
+{
+	int ret;
+
+	/* Enable wakeup interrupt */
+	ret = regmap_update_bits(data->regmap, FXLS8962AF_INT_EN,
+				 FXLS8962AF_INT_EN_SDCD_OT_EN,
+				 state ? FXLS8962AF_INT_EN_SDCD_OT_EN : 0);
+
+	return ret;
+}
+
 static int fxls8962af_set_watermark(struct iio_dev *indio_dev, unsigned val)
 {
 	struct fxls8962af_data *data = iio_priv(indio_dev);
@@ -463,6 +487,182 @@ static int fxls8962af_set_watermark(struct iio_dev *indio_dev, unsigned val)
 	return 0;
 }
 
+static int fxls8962af_read_event(struct iio_dev *indio_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 fxls8962af_data *data = iio_priv(indio_dev);
+	u16 raw_val;
+	int ret;
+
+	if (type != IIO_EV_TYPE_THRESH)
+		return -EINVAL;
+
+	/*
+	 * Read only upper-threshold register as the lower-threshold register have the
+	 * same value with reversed sign.
+	 */
+	ret = regmap_bulk_read(data->regmap, FXLS8962AF_SDCD_UTHS_LSB,
+			       &raw_val, (chan->scan_type.storagebits / 8));
+	if (ret)
+		return ret;
+
+	*val = sign_extend32(raw_val, chan->scan_type.realbits - 1);
+	*val2 = 0;
+
+	return IIO_VAL_INT;
+}
+
+static int fxls8962af_write_event(struct iio_dev *indio_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 fxls8962af_data *data = iio_priv(indio_dev);
+	int is_active;
+	int ret;
+
+	if (type != IIO_EV_TYPE_THRESH)
+		return -EINVAL;
+
+	if (val < 0 || val > 2047)
+		return -EINVAL;
+
+	if (data->enable_event)
+		return -EBUSY;
+
+	/* Add the same value to the lower-threshold register with a reversed sign */
+	data->lower_thres = (-val & 0x80000000) >> 20 | (val & 0x7FF);
+	data->upper_thres = (val & 0x80000000) >> 20 | (val & 0x7FF);
+
+	is_active = fxls8962af_is_active(data);
+	if (is_active) {
+		ret = fxls8962af_standby(data);
+		if (ret)
+			return ret;
+	}
+
+	ret = regmap_bulk_write(data->regmap, FXLS8962AF_SDCD_LTHS_LSB,
+				&data->lower_thres, (chan->scan_type.storagebits / 8));
+	ret = regmap_bulk_write(data->regmap, FXLS8962AF_SDCD_UTHS_LSB,
+				&data->upper_thres, (chan->scan_type.storagebits / 8));
+
+	if (is_active)
+		ret = fxls8962af_active(data);
+
+	return ret;
+}
+
+static int
+fxls8962af_read_event_config(struct iio_dev *indio_dev,
+			     const struct iio_chan_spec *chan,
+			     enum iio_event_type type,
+			     enum iio_event_direction dir)
+{
+	struct fxls8962af_data *data = iio_priv(indio_dev);
+	int ret;
+
+	if (type != IIO_EV_TYPE_THRESH)
+		return -EINVAL;
+
+	switch (chan->channel2) {
+	case IIO_MOD_X:
+		ret = FXLS8962AF_SDCD_CONFIG1_X_OT_EN & data->enable_event;
+		break;
+	case IIO_MOD_Y:
+		ret = FXLS8962AF_SDCD_CONFIG1_Y_OT_EN & data->enable_event;
+		break;
+	case IIO_MOD_Z:
+		ret = FXLS8962AF_SDCD_CONFIG1_Z_OT_EN & data->enable_event;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return !!ret;
+}
+
+static int
+fxls8962af_write_event_config(struct iio_dev *indio_dev,
+			      const struct iio_chan_spec *chan,
+			      enum iio_event_type type,
+			      enum iio_event_direction dir, int state)
+{
+	struct fxls8962af_data *data = iio_priv(indio_dev);
+	u8 enable_event, enable_bits;
+	int ret;
+
+	if (type != IIO_EV_TYPE_THRESH)
+		return -EINVAL;
+
+	fxls8962af_standby(data);
+
+	switch (chan->channel2) {
+	case IIO_MOD_X:
+		enable_bits = FXLS8962AF_SDCD_CONFIG1_X_OT_EN;
+		break;
+	case IIO_MOD_Y:
+		enable_bits = FXLS8962AF_SDCD_CONFIG1_Y_OT_EN;
+		break;
+	case IIO_MOD_Z:
+		enable_bits = FXLS8962AF_SDCD_CONFIG1_Z_OT_EN;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (state)
+		enable_event = data->enable_event | enable_bits;
+	else
+		enable_event = data->enable_event & ~enable_bits;
+
+	if (data->enable_event == enable_event)
+		return 0;
+
+	/* Enable events */
+	ret = regmap_write(data->regmap, FXLS8962AF_SDCD_CONFIG1, enable_event | 0x80);
+	if (ret)
+		return ret;
+
+	/*
+	 * Enable update of SDCD_REF_X/Y/Z values with the current decimated and
+	 * trimmed X/Y/Z acceleration input data. This allows for acceleration
+	 * slope detection with Data(n) to Data(n–1) always used as the input
+	 * to the window comparator.
+	 */
+	ret = regmap_write(data->regmap, FXLS8962AF_SDCD_CONFIG2, enable_event > 0 ? 0xC0 : 0x00);
+	if (ret)
+		return ret;
+
+	ret = fxls8962af_event_setup(data, state);
+	if (ret)
+		return ret;
+
+	data->enable_event = enable_event;
+
+	if (data->enable_event > 0) {
+		fxls8962af_active(data);
+		ret = fxls8962af_power_on(data);
+	} else {
+		if (!iio_buffer_enabled(indio_dev))
+			ret = fxls8962af_power_off(data);
+	}
+
+	return ret;
+}
+
+static const struct iio_event_spec fxls8962af_event = {
+	.type = IIO_EV_TYPE_THRESH,
+	.dir = IIO_EV_DIR_EITHER,
+	.mask_separate = BIT(IIO_EV_INFO_VALUE) |
+			 BIT(IIO_EV_INFO_ENABLE)
+};
+
 #define FXLS8962AF_CHANNEL(axis, reg, idx) { \
 	.type = IIO_ACCEL, \
 	.address = reg, \
@@ -481,6 +681,8 @@ static int fxls8962af_set_watermark(struct iio_dev *indio_dev, unsigned val)
 		.shift = 4, \
 		.endianness = IIO_BE, \
 	}, \
+	.event_spec = &fxls8962af_event, \
+	.num_event_specs = 1, \
 }
 
 #define FXLS8962AF_TEMP_CHANNEL { \
@@ -522,6 +724,10 @@ static const struct iio_info fxls8962af_info = {
 	.read_raw = &fxls8962af_read_raw,
 	.write_raw = &fxls8962af_write_raw,
 	.write_raw_get_fmt = fxls8962af_write_raw_get_fmt,
+	.read_event_value = fxls8962af_read_event,
+	.write_event_value = fxls8962af_write_event,
+	.read_event_config = fxls8962af_read_event_config,
+	.write_event_config = fxls8962af_write_event_config,
 	.read_avail = fxls8962af_read_avail,
 	.hwfifo_set_watermark = fxls8962af_set_watermark,
 };
@@ -605,7 +811,8 @@ static int fxls8962af_buffer_predisable(struct iio_dev *indio_dev)
 
 	ret = __fxls8962af_fifo_set_mode(data, false);
 
-	fxls8962af_active(data);
+	if (data->enable_event)
+		fxls8962af_active(data);
 
 	return ret;
 }
@@ -614,7 +821,10 @@ static int fxls8962af_buffer_postdisable(struct iio_dev *indio_dev)
 {
 	struct fxls8962af_data *data = iio_priv(indio_dev);
 
-	return fxls8962af_power_off(data);
+	if (data->enable_event == 0)
+		fxls8962af_power_off(data);
+
+	return 0;
 }
 
 static const struct iio_buffer_setup_ops fxls8962af_buffer_ops = {
@@ -725,6 +935,41 @@ static int fxls8962af_fifo_flush(struct iio_dev *indio_dev)
 	return count;
 }
 
+static int fxls8962af_event_interrupt(struct iio_dev *indio_dev)
+{
+	struct fxls8962af_data *data = iio_priv(indio_dev);
+	s64 ts = iio_get_time_ns(indio_dev);
+	unsigned int reg;
+	int ret;
+
+	ret = regmap_read(data->regmap, FXLS8962AF_SDCD_INT_SRC1, &reg);
+	if (ret | (reg < 0))
+		return -EINVAL;
+
+	if (reg & FXLS8962AF_SDCD_INT_SRC1_X_OT)
+		iio_push_event(indio_dev,
+				IIO_MOD_EVENT_CODE(IIO_ACCEL, 0, IIO_MOD_X,
+					IIO_EV_TYPE_THRESH,
+					IIO_EV_DIR_EITHER),
+				ts);
+
+	if (reg & FXLS8962AF_SDCD_INT_SRC1_Y_OT)
+		iio_push_event(indio_dev,
+				IIO_MOD_EVENT_CODE(IIO_ACCEL, 0, IIO_MOD_Y,
+					IIO_EV_TYPE_THRESH,
+					IIO_EV_DIR_EITHER),
+				ts);
+
+	if (reg & FXLS8962AF_SDCD_INT_SRC1_Z_OT)
+		iio_push_event(indio_dev,
+				IIO_MOD_EVENT_CODE(IIO_ACCEL, 0, IIO_MOD_Z,
+					IIO_EV_TYPE_THRESH,
+					IIO_EV_DIR_EITHER),
+				ts);
+
+	return ret;
+}
+
 static irqreturn_t fxls8962af_interrupt(int irq, void *p)
 {
 	struct iio_dev *indio_dev = p;
@@ -744,6 +989,14 @@ static irqreturn_t fxls8962af_interrupt(int irq, void *p)
 		return IRQ_HANDLED;
 	}
 
+	if (reg & FXLS8962AF_INT_STATUS_SRC_SDCD_OT) {
+		ret = fxls8962af_event_interrupt(indio_dev);
+		if (ret < 0)
+			return IRQ_NONE;
+
+		return IRQ_HANDLED;
+	}
+
 	return IRQ_NONE;
 }
 
-- 
2.32.0

[PATCH 2/2] iio: accel: fxls8962af: add wake on event

[Sean Nyekjaer]

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.
If the buffer is enabled it will be deactivated before suspend, as the
buffer is quite small.

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
 drivers/iio/accel/fxls8962af-core.c | 48 +++++++++++++++++++++++++++--
 1 file changed, 46 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/accel/fxls8962af-core.c b/drivers/iio/accel/fxls8962af-core.c
index 1b97c82b5b05..60d50759de12 100644
--- a/drivers/iio/accel/fxls8962af-core.c
+++ b/drivers/iio/accel/fxls8962af-core.c
@@ -160,6 +160,7 @@ struct fxls8962af_data {
 	} scan;
 	int64_t timestamp, old_timestamp;	/* Only used in hw fifo mode. */
 	struct iio_mount_matrix orientation;
+	int irq;
 	u8 watermark;
 	u8 enable_event;
 	u16 lower_thres;
@@ -1114,6 +1115,7 @@ int fxls8962af_core_probe(struct device *dev, struct regmap *regmap, int irq)
 	data = iio_priv(indio_dev);
 	dev_set_drvdata(dev, indio_dev);
 	data->regmap = regmap;
+	data->irq = irq;
 
 	ret = iio_read_mount_matrix(dev, &data->orientation);
 	if (ret)
@@ -1183,6 +1185,9 @@ int fxls8962af_core_probe(struct device *dev, struct regmap *regmap, int irq)
 	if (ret)
 		return ret;
 
+	if (dev_fwnode(dev) && device_property_read_bool(dev, "wakeup-source"))
+		device_init_wakeup(dev, true);
+
 	return devm_iio_device_register(dev, indio_dev);
 }
 EXPORT_SYMBOL_GPL(fxls8962af_core_probe);
@@ -1208,9 +1213,48 @@ static int __maybe_unused fxls8962af_runtime_resume(struct device *dev)
 	return fxls8962af_active(data);
 }
 
+static int __maybe_unused fxls8962af_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct fxls8962af_data *data = iio_priv(indio_dev);
+
+
+	if (device_may_wakeup(dev) && data->enable_event) {
+		enable_irq_wake(data->irq);
+
+		/*
+		 * Disable buffer, as the buffer is so small the device will wake
+		 * almost immediately.
+		 */
+		if (iio_buffer_enabled(indio_dev))
+			fxls8962af_buffer_predisable(indio_dev);
+	} else {
+		fxls8962af_runtime_suspend(dev);
+	}
+
+	return 0;
+}
+
+static int __maybe_unused fxls8962af_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct fxls8962af_data *data = iio_priv(indio_dev);
+
+	if (device_may_wakeup(dev) && data->enable_event) {
+		disable_irq_wake(data->irq);
+
+		if (iio_buffer_enabled(indio_dev))
+			fxls8962af_buffer_postenable(indio_dev);
+	} else {
+		fxls8962af_runtime_resume(dev);
+	}
+
+	return 0;
+}
+
 const struct dev_pm_ops fxls8962af_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
-				pm_runtime_force_resume)
+	SET_SYSTEM_SLEEP_PM_OPS(fxls8962af_suspend,
+				fxls8962af_resume)
 	SET_RUNTIME_PM_OPS(fxls8962af_runtime_suspend,
 			   fxls8962af_runtime_resume, NULL)
 };
-- 
2.32.0

Re: [PATCH 1/2] iio: accel: fxls8962af: add threshold event handling

[Andy Shevchenko]

On Wed, Aug 18, 2021 at 12:29 PM Sean Nyekjaer <sean@geanix.com> wrote:
>
> Add event channels that controls the creation of motion events.

'channel' or 'control' decide which one should be plural.

> +static int fxls8962af_event_setup(struct fxls8962af_data *data, int state)
> +{
> +       int ret;
> +
> +       /* Enable wakeup interrupt */
> +       ret = regmap_update_bits(data->regmap, FXLS8962AF_INT_EN,
> +                                FXLS8962AF_INT_EN_SDCD_OT_EN,
> +                                state ? FXLS8962AF_INT_EN_SDCD_OT_EN : 0);
> +
> +       return ret;

Redundant ret. Besides that use simply

int mask = FXLS8962AF_INT_EN_SDCD_OT_EN;
int value = state ? mask : 0;

return regmap(..., mask, value);

> +}

...

> +       ret = regmap_bulk_read(data->regmap, FXLS8962AF_SDCD_UTHS_LSB,
> +                              &raw_val, (chan->scan_type.storagebits / 8));

Too many parentheses.
Check also the rest of the code for all comments I have given in this email.

> +       if (ret)
> +               return ret;

...

> +       if (val < 0 || val > 2047)
> +               return -EINVAL;

Can be performed as (val >> 11), but the above is more explicit I think.

...

> +       /* Add the same value to the lower-threshold register with a reversed sign */
> +       data->lower_thres = (-val & 0x80000000) >> 20 | (val & 0x7FF);
> +       data->upper_thres = (val & 0x80000000) >> 20 | (val & 0x7FF);

Why is it so complicated?

Wouldn't lower = -val & GENMASK(10, 0); work?
The upper, btw, has a dead code.

...

> +       ret = regmap_bulk_write(data->regmap, FXLS8962AF_SDCD_LTHS_LSB,
> +                               &data->lower_thres, (chan->scan_type.storagebits / 8));

Missed error check.

> +       ret = regmap_bulk_write(data->regmap, FXLS8962AF_SDCD_UTHS_LSB,
> +                               &data->upper_thres, (chan->scan_type.storagebits / 8));

Ditto.

> +       if (is_active)
> +               ret = fxls8962af_active(data);
> +
> +       return ret;

...

> +       switch (chan->channel2) {
> +       case IIO_MOD_X:
> +               ret = FXLS8962AF_SDCD_CONFIG1_X_OT_EN & data->enable_event;
> +               break;
> +       case IIO_MOD_Y:
> +               ret = FXLS8962AF_SDCD_CONFIG1_Y_OT_EN & data->enable_event;
> +               break;
> +       case IIO_MOD_Z:
> +               ret = FXLS8962AF_SDCD_CONFIG1_Z_OT_EN & data->enable_event;
> +               break;
> +       default:
> +               return -EINVAL;
> +       }
> +

> +       return !!ret;

This is strange.

...

> +       ret = regmap_write(data->regmap, FXLS8962AF_SDCD_CONFIG2, enable_event > 0 ? 0xC0 : 0x00);

Redundant ' > 0'

> +       if (ret)
> +               return ret;

...

> +       if (data->enable_event > 0) {

Ditto.

> +               fxls8962af_active(data);
> +               ret = fxls8962af_power_on(data);
> +       } else {
> +               if (!iio_buffer_enabled(indio_dev))
> +                       ret = fxls8962af_power_off(data);
> +       }

...

> +static const struct iio_event_spec fxls8962af_event = {
> +       .type = IIO_EV_TYPE_THRESH,
> +       .dir = IIO_EV_DIR_EITHER,
> +       .mask_separate = BIT(IIO_EV_INFO_VALUE) |
> +                        BIT(IIO_EV_INFO_ENABLE)

+ comma

> +};

...

> +       if (data->enable_event == 0)

if (!enable_event)

> +               fxls8962af_power_off(data);

...

> +       ret = regmap_read(data->regmap, FXLS8962AF_SDCD_INT_SRC1, &reg);
> +       if (ret | (reg < 0))

This is weird and shadows an actual error.

> +               return -EINVAL;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] iio: accel: fxls8962af: add wake on event

[Andy Shevchenko]

On Wed, Aug 18, 2021 at 12:29 PM Sean Nyekjaer <sean@geanix.com> wrote:
>
> This add ways for the SoC to wake from accelerometer wake events.

adds

> In the suspend function we skip disabling the sensor if wakeup-source
> and events are activated.
> If the buffer is enabled it will be deactivated before suspend, as the
> buffer is quite small.

"..., because it is..."
Or what are you saying here?

...

> +       if (dev_fwnode(dev) && device_property_read_bool(dev, "wakeup-source"))

dev_fwnode() is redundant.

> +               device_init_wakeup(dev, true);

...

> +static int __maybe_unused fxls8962af_suspend(struct device *dev)
> +{
> +       struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +       struct fxls8962af_data *data = iio_priv(indio_dev);
> +
> +

One blank line is enough.

> +       if (device_may_wakeup(dev) && data->enable_event) {
> +               enable_irq_wake(data->irq);
> +
> +               /*
> +                * Disable buffer, as the buffer is so small the device will wake
> +                * almost immediately.
> +                */
> +               if (iio_buffer_enabled(indio_dev))
> +                       fxls8962af_buffer_predisable(indio_dev);
> +       } else {
> +               fxls8962af_runtime_suspend(dev);
> +       }
> +
> +       return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] iio: accel: fxls8962af: add wake on event

[Sean Nyekjaer]

On Wed, Aug 18, 2021 at 11:27:41AM +0200, Sean Nyekjaer 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.
> If the buffer is enabled it will be deactivated before suspend, as the
> buffer is quite small.
> 
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>

I notice when using IRQ_TYPE_LEVEL_LOW, the IRQ will loop on resume
until wdg is restarting the system.
If I use IRQ_TYPE_EDGE_FALLING it will only fail once with -13 EACCES.
Maybe I2C isn't up and running when it tries to handle the irq.

/Sean