[PATCH v6 00/11] iio: accel: adxl345: add interrupt based sensor events

[PATCH v6 00/11] iio: accel: adxl345: add interrupt based sensor events

[Lothar Rubusch]

Add several interrupt based sensor detection events:
- single tap
- double tap
- free fall
- activity
- inactivity
- sample frequency
- full frequency g range approach
- documentation

All the needed parameters for each and methods of adjusting them, and
forward a resulting IIO event for each to the IIO channel.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
v5 -> v6:
- replace bool axis_en for tap and activity/inactivity
- apply freefall bit mask
- change `measure_en` to use `regmap_update_bits()` for POWER_CTL register
- fix comments and update documentation, particularly on inactivity time

v4 -> v5:
- read_config_value() and write_config_value() now use direct returns,
  in case of a failure, measurement stays turned off
- fifo evaluation returns 0 in case of success
- axis enum turned into three different set of defines for tap, act and inact
- turn the suppress bit into a separate define macro
- variable naming, generally use axis_ctrl for similar variables

v3 -> v4:
- rename patch "add double tap suppress bit" to
  "set the tap suppress bit permanently" to make it more comprehensive
- added patch "cleanup regmap return values"
- added patch "introduce adxl345_push_event function", as a solution
  to the return value problem, group all int_stat evaluating pieces
  in the same function
- tap, act and inact axis enabling are using now regmap cache
- activity enable depending on individual axis now, as the sensor offers
  such feature
- inactivity enable depending on individual axis now, as the sensor offers
  such feature
- fix bug in previous patch: separate axis direction in interrupt handler
  sharing the same variable for tap and activity, if tap and activity
  enabled together
- refac of the direction identification of previous patch: only read
  act/tap axis once now in interrupt handler if both is enabled
- fix bug in previous patch: return value of pushed event in interrupt
  handler
- several cleanups

v2 -> v3:
- generally introduction of regmap cache for all directly stored 8-bit
  values, specification of volatile regs and cleanup
- moving thresholds, unchanged values and flags to regmap cache, in
  consequence removal of corresponding member values of the state
  instance
- removal of intio and int_map member fields due to regmap cache, thus
  split of set_interrupts() patches in two parts
- rework documentation
- rework of ac-bit comment

v1 -> v2:
- implementation of all events (but tap2 suppress bit) by means IIO ABI
- add sample frequency / ODR configuration
- add g ranges configuration
- add activity/inactivity using auto-sleep and powersave
- add dynamic adjustment of default values for
  activity/inactivity thresholds and time for inactivity based on ODR
  and g range (can be overwritten)
- add sensor documentation
---
Lothar Rubusch (11):
  iio: accel: adxl345: introduce adxl345_push_event function
  iio: accel: adxl345: add single tap feature
  iio: accel: adxl345: add double tap feature
  iio: accel: adxl345: set the tap suppress bit permanently
  iio: accel: adxl345: add freefall feature
  iio: accel: adxl345: extend sample frequency adjustments
  iio: accel: adxl345: add g-range configuration
  iio: accel: adxl345: add activity event feature
  iio: accel: adxl345: add inactivity feature
  iio: accel: adxl345: add coupling detection for activity/inactivity
  docs: iio: add documentation for adxl345 driver

 Documentation/iio/adxl345.rst    |  429 +++++++++
 drivers/iio/accel/adxl345.h      |    2 +-
 drivers/iio/accel/adxl345_core.c | 1405 +++++++++++++++++++++++++++++-
 3 files changed, 1790 insertions(+), 46 deletions(-)
 create mode 100644 Documentation/iio/adxl345.rst

-- 
2.39.5

[PATCH v6 01/11] iio: accel: adxl345: introduce adxl345_push_event function

[Lothar Rubusch]

Move the fifo handling into a separate function. This is a preparation
for a generic handling of the interrupt status register results. The
function is supposed to handle particular sensor events, and later to
forward them to the iio channel. This is needed to read out the interrupt
status register.

The function shall return occurring errors, if any, or 0 in case of
handled events or read fifo content. Thus migrate fifo read-out and push
fifo content to iio channels into this function to be built up with
additional event handling.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 drivers/iio/accel/adxl345_core.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 1b4d9e60c87d..a98fb7fc748e 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -416,6 +416,23 @@ static int adxl345_fifo_push(struct iio_dev *indio_dev,
 	return 0;
 }
 
+static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat)
+{
+	struct adxl345_state *st = iio_priv(indio_dev);
+	int samples;
+
+	if (FIELD_GET(ADXL345_INT_WATERMARK, int_stat)) {
+		samples = adxl345_get_samples(st);
+		if (samples < 0)
+			return -EINVAL;
+
+		if (adxl345_fifo_push(indio_dev, samples) < 0)
+			return -EINVAL;
+	}
+
+	return 0;
+}
+
 /**
  * adxl345_irq_handler() - Handle irqs of the ADXL345.
  * @irq: The irq being handled.
@@ -428,19 +445,12 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
 	struct iio_dev *indio_dev = p;
 	struct adxl345_state *st = iio_priv(indio_dev);
 	int int_stat;
-	int samples;
 
 	if (regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, &int_stat))
 		return IRQ_NONE;
 
-	if (FIELD_GET(ADXL345_INT_WATERMARK, int_stat)) {
-		samples = adxl345_get_samples(st);
-		if (samples < 0)
-			goto err;
-
-		if (adxl345_fifo_push(indio_dev, samples) < 0)
-			goto err;
-	}
+	if (adxl345_push_event(indio_dev, int_stat))
+		goto err;
 
 	if (FIELD_GET(ADXL345_INT_OVERRUN, int_stat))
 		goto err;
-- 
2.39.5

[PATCH v6 02/11] iio: accel: adxl345: add single tap feature

[Lothar Rubusch]

Add the single tap feature with a threshold in 62.5mg/LSB points and a
scaled duration in us. Keep singletap threshold in regmap cache but
the scaled value of duration in us as member variable.

Both use IIO channels for individual enable of the x/y/z axis. Initializes
threshold and duration with reasonable content. When an interrupt is
caught it will be pushed to the according IIO channel.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 drivers/iio/accel/adxl345_core.c | 372 ++++++++++++++++++++++++++++++-
 1 file changed, 369 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index a98fb7fc748e..ccb25c35ac07 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -8,6 +8,7 @@
  */
 
 #include <linux/bitfield.h>
+#include <linux/bitops.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/property.h>
@@ -17,6 +18,7 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 #include <linux/iio/buffer.h>
+#include <linux/iio/events.h>
 #include <linux/iio/kfifo_buf.h>
 
 #include "adxl345.h"
@@ -31,6 +33,29 @@
 #define ADXL345_INT1			0
 #define ADXL345_INT2			1
 
+#define ADXL345_REG_TAP_AXIS_MSK	GENMASK(2, 0)
+
+#define ADXL345_TAP_Z_EN		BIT(0)
+#define ADXL345_TAP_Y_EN		BIT(1)
+#define ADXL345_TAP_X_EN		BIT(2)
+
+/* single/double tap */
+enum adxl345_tap_type {
+	ADXL345_SINGLE_TAP,
+};
+
+static const unsigned int adxl345_tap_int_reg[] = {
+	[ADXL345_SINGLE_TAP] = ADXL345_INT_SINGLE_TAP,
+};
+
+enum adxl345_tap_time_type {
+	ADXL345_TAP_TIME_DUR,
+};
+
+static const unsigned int adxl345_tap_time_reg[] = {
+	[ADXL345_TAP_TIME_DUR] = ADXL345_REG_DUR,
+};
+
 struct adxl345_state {
 	const struct adxl345_chip_info *info;
 	struct regmap *regmap;
@@ -38,9 +63,23 @@ struct adxl345_state {
 	int irq;
 	u8 watermark;
 	u8 fifo_mode;
+
+	u32 tap_duration_us;
+
 	__le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE + 1] __aligned(IIO_DMA_MINALIGN);
 };
 
+static struct iio_event_spec adxl345_events[] = {
+	{
+		/* single tap */
+		.type = IIO_EV_TYPE_GESTURE,
+		.dir = IIO_EV_DIR_SINGLETAP,
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
+		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
+			BIT(IIO_EV_INFO_TIMEOUT),
+	},
+};
+
 #define ADXL345_CHANNEL(index, reg, axis) {					\
 	.type = IIO_ACCEL,						\
 	.modified = 1,							\
@@ -57,6 +96,8 @@ struct adxl345_state {
 		.storagebits = 16,			\
 		.endianness = IIO_LE,			\
 	},						\
+	.event_spec = adxl345_events,			\
+	.num_event_specs = ARRAY_SIZE(adxl345_events),	\
 }
 
 enum adxl345_chans {
@@ -113,6 +154,157 @@ static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
 	return regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
 }
 
+/* tap */
+
+static int _adxl345_set_tap_int(struct adxl345_state *st,
+				enum adxl345_tap_type type, bool state)
+{
+	unsigned int int_map = 0x00;
+	unsigned int tap_threshold;
+	bool axis_valid;
+	bool singletap_args_valid = false;
+	bool en = false;
+	u32 axis_ctrl;
+	int ret;
+
+	ret = regmap_read(st->regmap, ADXL345_REG_TAP_AXIS, &axis_ctrl);
+	if (ret)
+		return ret;
+
+	axis_valid = FIELD_GET(ADXL345_REG_TAP_AXIS_MSK, axis_ctrl) > 0;
+
+	ret = regmap_read(st->regmap, ADXL345_REG_THRESH_TAP, &tap_threshold);
+	if (ret)
+		return ret;
+
+	/*
+	 * Note: A value of 0 for threshold and/or dur may result in undesirable
+	 *	 behavior if single tap/double tap interrupts are enabled.
+	 */
+	singletap_args_valid = tap_threshold > 0 && st->tap_duration_us > 0;
+
+	if (type == ADXL345_SINGLE_TAP)
+		en = axis_valid && singletap_args_valid;
+
+	if (state && en)
+		int_map |= adxl345_tap_int_reg[type];
+
+	return regmap_update_bits(st->regmap, ADXL345_REG_INT_ENABLE,
+				  adxl345_tap_int_reg[type], int_map);
+}
+
+static int adxl345_is_tap_en(struct adxl345_state *st,
+			     enum iio_modifier axis,
+			     enum adxl345_tap_type type, bool *en)
+{
+	unsigned int regval;
+	u32 axis_ctrl;
+	int ret;
+
+	ret = regmap_read(st->regmap, ADXL345_REG_TAP_AXIS, &axis_ctrl);
+	if (ret)
+		return ret;
+
+	/* Verify if axis is enabled for the tap detection. */
+	switch (axis) {
+	case IIO_MOD_X:
+		*en = FIELD_GET(ADXL345_TAP_X_EN, axis_ctrl);
+		break;
+	case IIO_MOD_Y:
+		*en = FIELD_GET(ADXL345_TAP_Y_EN, axis_ctrl);
+		break;
+	case IIO_MOD_Z:
+		*en = FIELD_GET(ADXL345_TAP_Z_EN, axis_ctrl);
+		break;
+	default:
+		*en = false;
+		return -EINVAL;
+	}
+
+	if (*en) {
+		/*
+		 * If axis allow for tap detection, verify if the interrupt is
+		 * enabled for tap detection.
+		 */
+		ret = regmap_read(st->regmap, ADXL345_REG_INT_ENABLE, &regval);
+		if (ret)
+			return ret;
+
+		*en = adxl345_tap_int_reg[type] & regval;
+	}
+
+	return 0;
+}
+
+static int adxl345_set_singletap_en(struct adxl345_state *st,
+				    enum iio_modifier axis, bool en)
+{
+	int ret;
+	u32 axis_ctrl;
+
+	switch (axis) {
+	case IIO_MOD_X:
+		axis_ctrl = ADXL345_TAP_X_EN;
+		break;
+	case IIO_MOD_Y:
+		axis_ctrl = ADXL345_TAP_Y_EN;
+		break;
+	case IIO_MOD_Z:
+		axis_ctrl = ADXL345_TAP_Z_EN;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (en)
+		ret = regmap_set_bits(st->regmap, ADXL345_REG_TAP_AXIS,
+				      axis_ctrl);
+	else
+		ret = regmap_clear_bits(st->regmap, ADXL345_REG_TAP_AXIS,
+					axis_ctrl);
+	if (ret)
+		return ret;
+
+	return _adxl345_set_tap_int(st, ADXL345_SINGLE_TAP, en);
+}
+
+static int _adxl345_set_tap_time(struct adxl345_state *st,
+				 enum adxl345_tap_time_type type, u32 val_us)
+{
+	unsigned int regval;
+
+	switch (type) {
+	case ADXL345_TAP_TIME_DUR:
+		st->tap_duration_us = val_us;
+		break;
+	}
+
+	/*
+	 * The scale factor is 1250us / LSB for tap_window_us and tap_latent_us.
+	 * For tap_duration_us the scale factor is 625us / LSB.
+	 */
+	if (type == ADXL345_TAP_TIME_DUR)
+		regval = DIV_ROUND_CLOSEST(val_us, 625);
+	else
+		regval = DIV_ROUND_CLOSEST(val_us, 1250);
+
+	return regmap_write(st->regmap, adxl345_tap_time_reg[type], regval);
+}
+
+static int adxl345_set_tap_duration(struct adxl345_state *st, u32 val_int,
+				    u32 val_fract_us)
+{
+	/*
+	 * Max value is 255 * 625 us = 0.159375 seconds
+	 *
+	 * Note: the scaling is similar to the scaling in the ADXL380
+	 */
+	if (val_int || val_fract_us > 159375)
+		return -EINVAL;
+
+	return _adxl345_set_tap_time(st, ADXL345_TAP_TIME_DUR, val_fract_us);
+}
+
 static int adxl345_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan,
 			    int *val, int *val2, long mask)
@@ -198,6 +390,131 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
 	return -EINVAL;
 }
 
+static int adxl345_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 adxl345_state *st = iio_priv(indio_dev);
+	bool int_en;
+	int ret;
+
+	switch (type) {
+	case IIO_EV_TYPE_GESTURE:
+		switch (dir) {
+		case IIO_EV_DIR_SINGLETAP:
+			ret = adxl345_is_tap_en(st, chan->channel2,
+						ADXL345_SINGLE_TAP, &int_en);
+			if (ret)
+				return ret;
+			return int_en;
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+}
+
+static int adxl345_write_event_config(struct iio_dev *indio_dev,
+				      const struct iio_chan_spec *chan,
+				      enum iio_event_type type,
+				      enum iio_event_direction dir,
+				      bool state)
+{
+	struct adxl345_state *st = iio_priv(indio_dev);
+
+	switch (type) {
+	case IIO_EV_TYPE_GESTURE:
+		switch (dir) {
+		case IIO_EV_DIR_SINGLETAP:
+			return adxl345_set_singletap_en(st, chan->channel2, state);
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+}
+
+static int adxl345_read_event_value(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 adxl345_state *st = iio_priv(indio_dev);
+	unsigned int tap_threshold;
+	int ret;
+
+	switch (type) {
+	case IIO_EV_TYPE_GESTURE:
+		switch (info) {
+		case IIO_EV_INFO_VALUE:
+			/*
+			 * The scale factor would be 62.5mg/LSB (i.e. 0xFF = 16g) but
+			 * not applied here. In context of this general purpose sensor,
+			 * what imports is rather signal intensity than the absolute
+			 * measured g value.
+			 */
+			ret = regmap_read(st->regmap, ADXL345_REG_THRESH_TAP,
+					  &tap_threshold);
+			if (ret)
+				return ret;
+			*val = sign_extend32(tap_threshold, 7);
+			return IIO_VAL_INT;
+		case IIO_EV_INFO_TIMEOUT:
+			*val = st->tap_duration_us;
+			*val2 = 1000000;
+			return IIO_VAL_FRACTIONAL;
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+}
+
+static int adxl345_write_event_value(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 adxl345_state *st = iio_priv(indio_dev);
+	int ret;
+
+	ret = adxl345_set_measure_en(st, false);
+	if (ret)
+		return ret;
+
+	switch (type) {
+	case IIO_EV_TYPE_GESTURE:
+		switch (info) {
+		case IIO_EV_INFO_VALUE:
+			ret = regmap_write(st->regmap, ADXL345_REG_THRESH_TAP,
+					   min(val, 0xFF));
+			if (ret)
+				return ret;
+			break;
+		case IIO_EV_INFO_TIMEOUT:
+			ret = adxl345_set_tap_duration(st, val, val2);
+			if (ret)
+				return ret;
+			break;
+		default:
+			return -EINVAL;
+		}
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return adxl345_set_measure_en(st, true);
+}
+
 static int adxl345_reg_access(struct iio_dev *indio_dev, unsigned int reg,
 			      unsigned int writeval, unsigned int *readval)
 {
@@ -416,10 +733,23 @@ static int adxl345_fifo_push(struct iio_dev *indio_dev,
 	return 0;
 }
 
-static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat)
+static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
+			      enum iio_modifier tap_dir)
 {
+	s64 ts = iio_get_time_ns(indio_dev);
 	struct adxl345_state *st = iio_priv(indio_dev);
 	int samples;
+	int ret = -ENOENT;
+
+	if (FIELD_GET(ADXL345_INT_SINGLE_TAP, int_stat)) {
+		ret = iio_push_event(indio_dev,
+				     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0, tap_dir,
+							IIO_EV_TYPE_GESTURE,
+							IIO_EV_DIR_SINGLETAP),
+				     ts);
+		if (ret)
+			return ret;
+	}
 
 	if (FIELD_GET(ADXL345_INT_WATERMARK, int_stat)) {
 		samples = adxl345_get_samples(st);
@@ -428,9 +758,11 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat)
 
 		if (adxl345_fifo_push(indio_dev, samples) < 0)
 			return -EINVAL;
+
+		ret = 0;
 	}
 
-	return 0;
+	return ret;
 }
 
 /**
@@ -444,12 +776,33 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
 {
 	struct iio_dev *indio_dev = p;
 	struct adxl345_state *st = iio_priv(indio_dev);
+	unsigned int regval;
+	enum iio_modifier tap_dir = IIO_NO_MOD;
+	u32 axis_ctrl;
 	int int_stat;
+	int ret;
+
+	ret = regmap_read(st->regmap, ADXL345_REG_TAP_AXIS, &axis_ctrl);
+	if (ret)
+		return IRQ_NONE;
+
+	if (FIELD_GET(ADXL345_REG_TAP_AXIS_MSK, axis_ctrl)) {
+		ret = regmap_read(st->regmap, ADXL345_REG_ACT_TAP_STATUS, &regval);
+		if (ret)
+			return IRQ_NONE;
+
+		if (FIELD_GET(ADXL345_TAP_Z_EN, regval))
+			tap_dir = IIO_MOD_Z;
+		else if (FIELD_GET(ADXL345_TAP_Y_EN, regval))
+			tap_dir = IIO_MOD_Y;
+		else if (FIELD_GET(ADXL345_TAP_X_EN, regval))
+			tap_dir = IIO_MOD_X;
+	}
 
 	if (regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, &int_stat))
 		return IRQ_NONE;
 
-	if (adxl345_push_event(indio_dev, int_stat))
+	if (adxl345_push_event(indio_dev, int_stat, tap_dir))
 		goto err;
 
 	if (FIELD_GET(ADXL345_INT_OVERRUN, int_stat))
@@ -468,6 +821,10 @@ static const struct iio_info adxl345_info = {
 	.read_raw	= adxl345_read_raw,
 	.write_raw	= adxl345_write_raw,
 	.write_raw_get_fmt	= adxl345_write_raw_get_fmt,
+	.read_event_config = adxl345_read_event_config,
+	.write_event_config = adxl345_write_event_config,
+	.read_event_value = adxl345_read_event_value,
+	.write_event_value = adxl345_write_event_value,
 	.debugfs_reg_access = &adxl345_reg_access,
 	.hwfifo_set_watermark = adxl345_set_watermark,
 };
@@ -501,6 +858,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 					 ADXL345_DATA_FORMAT_JUSTIFY |
 					 ADXL345_DATA_FORMAT_FULL_RES |
 					 ADXL345_DATA_FORMAT_SELF_TEST);
+	unsigned int tap_threshold;
 	int ret;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
@@ -514,6 +872,10 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 		return -ENODEV;
 	st->fifo_delay = fifo_delay_default;
 
+	/* Init with reasonable values */
+	tap_threshold = 48;			/*   48 [0x30] -> ~3g     */
+	st->tap_duration_us = 16;		/*   16 [0x10] -> .010    */
+
 	indio_dev->name = st->info->name;
 	indio_dev->info = &adxl345_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
@@ -586,6 +948,10 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 		if (ret)
 			return ret;
 
+		ret = regmap_write(st->regmap, ADXL345_REG_THRESH_TAP, tap_threshold);
+		if (ret)
+			return ret;
+
 		/* FIFO_STREAM mode is going to be activated later */
 		ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, &adxl345_buffer_ops);
 		if (ret)
-- 
2.39.5

[PATCH v6 03/11] iio: accel: adxl345: add double tap feature

[Lothar Rubusch]

Add the double tap feature of the sensor. The interrupt handler needs
to catch and forward the event to the IIO channel. The single tap
implementation now is extended to deal with double tap as well.

Doubletap introduces window and latency times, both in us. Since both
times are scaled, the 8-bit register value is stored in hardware,
where the scaled value in [us] is stored as member variable.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 drivers/iio/accel/adxl345_core.c | 104 ++++++++++++++++++++++++++++++-
 1 file changed, 103 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index ccb25c35ac07..a95f1c218c0c 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -42,17 +42,23 @@
 /* single/double tap */
 enum adxl345_tap_type {
 	ADXL345_SINGLE_TAP,
+	ADXL345_DOUBLE_TAP,
 };
 
 static const unsigned int adxl345_tap_int_reg[] = {
 	[ADXL345_SINGLE_TAP] = ADXL345_INT_SINGLE_TAP,
+	[ADXL345_DOUBLE_TAP] = ADXL345_INT_DOUBLE_TAP,
 };
 
 enum adxl345_tap_time_type {
+	ADXL345_TAP_TIME_LATENT,
+	ADXL345_TAP_TIME_WINDOW,
 	ADXL345_TAP_TIME_DUR,
 };
 
 static const unsigned int adxl345_tap_time_reg[] = {
+	[ADXL345_TAP_TIME_LATENT] = ADXL345_REG_LATENT,
+	[ADXL345_TAP_TIME_WINDOW] = ADXL345_REG_WINDOW,
 	[ADXL345_TAP_TIME_DUR] = ADXL345_REG_DUR,
 };
 
@@ -65,6 +71,8 @@ struct adxl345_state {
 	u8 fifo_mode;
 
 	u32 tap_duration_us;
+	u32 tap_latent_us;
+	u32 tap_window_us;
 
 	__le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE + 1] __aligned(IIO_DMA_MINALIGN);
 };
@@ -78,6 +86,14 @@ static struct iio_event_spec adxl345_events[] = {
 		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
 			BIT(IIO_EV_INFO_TIMEOUT),
 	},
+	{
+		/* double tap */
+		.type = IIO_EV_TYPE_GESTURE,
+		.dir = IIO_EV_DIR_DOUBLETAP,
+		.mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE) |
+			BIT(IIO_EV_INFO_RESET_TIMEOUT) |
+			BIT(IIO_EV_INFO_TAP2_MIN_DELAY),
+	},
 };
 
 #define ADXL345_CHANNEL(index, reg, axis) {					\
@@ -163,6 +179,7 @@ static int _adxl345_set_tap_int(struct adxl345_state *st,
 	unsigned int tap_threshold;
 	bool axis_valid;
 	bool singletap_args_valid = false;
+	bool doubletap_args_valid = false;
 	bool en = false;
 	u32 axis_ctrl;
 	int ret;
@@ -183,8 +200,16 @@ static int _adxl345_set_tap_int(struct adxl345_state *st,
 	 */
 	singletap_args_valid = tap_threshold > 0 && st->tap_duration_us > 0;
 
-	if (type == ADXL345_SINGLE_TAP)
+	if (type == ADXL345_SINGLE_TAP) {
 		en = axis_valid && singletap_args_valid;
+	} else {
+		/* doubletap: Window must be equal or greater than latent! */
+		doubletap_args_valid = st->tap_latent_us > 0 &&
+			st->tap_window_us > 0 &&
+			st->tap_window_us >= st->tap_latent_us;
+
+		en = axis_valid && singletap_args_valid && doubletap_args_valid;
+	}
 
 	if (state && en)
 		int_map |= adxl345_tap_int_reg[type];
@@ -268,12 +293,23 @@ static int adxl345_set_singletap_en(struct adxl345_state *st,
 	return _adxl345_set_tap_int(st, ADXL345_SINGLE_TAP, en);
 }
 
+static int adxl345_set_doubletap_en(struct adxl345_state *st, bool en)
+{
+	return _adxl345_set_tap_int(st, ADXL345_DOUBLE_TAP, en);
+}
+
 static int _adxl345_set_tap_time(struct adxl345_state *st,
 				 enum adxl345_tap_time_type type, u32 val_us)
 {
 	unsigned int regval;
 
 	switch (type) {
+	case ADXL345_TAP_TIME_WINDOW:
+		st->tap_window_us = val_us;
+		break;
+	case ADXL345_TAP_TIME_LATENT:
+		st->tap_latent_us = val_us;
+		break;
 	case ADXL345_TAP_TIME_DUR:
 		st->tap_duration_us = val_us;
 		break;
@@ -305,6 +341,34 @@ static int adxl345_set_tap_duration(struct adxl345_state *st, u32 val_int,
 	return _adxl345_set_tap_time(st, ADXL345_TAP_TIME_DUR, val_fract_us);
 }
 
+static int adxl345_set_tap_window(struct adxl345_state *st, u32 val_int,
+				  u32 val_fract_us)
+{
+	/*
+	 * Max value is 255 * 1250 us = 0.318750 seconds
+	 *
+	 * Note: the scaling is similar to the scaling in the ADXL380
+	 */
+	if (val_int || val_fract_us > 318750)
+		return -EINVAL;
+
+	return _adxl345_set_tap_time(st, ADXL345_TAP_TIME_WINDOW, val_fract_us);
+}
+
+static int adxl345_set_tap_latent(struct adxl345_state *st, u32 val_int,
+				  u32 val_fract_us)
+{
+	/*
+	 * Max value is 255 * 1250 us = 0.318750 seconds
+	 *
+	 * Note: the scaling is similar to the scaling in the ADXL380
+	 */
+	if (val_int || val_fract_us > 318750)
+		return -EINVAL;
+
+	return _adxl345_set_tap_time(st, ADXL345_TAP_TIME_LATENT, val_fract_us);
+}
+
 static int adxl345_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan,
 			    int *val, int *val2, long mask)
@@ -408,6 +472,12 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
 			if (ret)
 				return ret;
 			return int_en;
+		case IIO_EV_DIR_DOUBLETAP:
+			ret = adxl345_is_tap_en(st, chan->channel2,
+						ADXL345_DOUBLE_TAP, &int_en);
+			if (ret)
+				return ret;
+			return int_en;
 		default:
 			return -EINVAL;
 		}
@@ -429,6 +499,8 @@ static int adxl345_write_event_config(struct iio_dev *indio_dev,
 		switch (dir) {
 		case IIO_EV_DIR_SINGLETAP:
 			return adxl345_set_singletap_en(st, chan->channel2, state);
+		case IIO_EV_DIR_DOUBLETAP:
+			return adxl345_set_doubletap_en(st, state);
 		default:
 			return -EINVAL;
 		}
@@ -468,6 +540,14 @@ static int adxl345_read_event_value(struct iio_dev *indio_dev,
 			*val = st->tap_duration_us;
 			*val2 = 1000000;
 			return IIO_VAL_FRACTIONAL;
+		case IIO_EV_INFO_RESET_TIMEOUT:
+			*val = st->tap_window_us;
+			*val2 = 1000000;
+			return IIO_VAL_FRACTIONAL;
+		case IIO_EV_INFO_TAP2_MIN_DELAY:
+			*val = st->tap_latent_us;
+			*val2 = 1000000;
+			return IIO_VAL_FRACTIONAL;
 		default:
 			return -EINVAL;
 		}
@@ -504,6 +584,16 @@ static int adxl345_write_event_value(struct iio_dev *indio_dev,
 			if (ret)
 				return ret;
 			break;
+		case IIO_EV_INFO_RESET_TIMEOUT:
+			ret = adxl345_set_tap_window(st, val, val2);
+			if (ret)
+				return ret;
+			break;
+		case IIO_EV_INFO_TAP2_MIN_DELAY:
+			ret = adxl345_set_tap_latent(st, val, val2);
+			if (ret)
+				return ret;
+			break;
 		default:
 			return -EINVAL;
 		}
@@ -751,6 +841,16 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
 			return ret;
 	}
 
+	if (FIELD_GET(ADXL345_INT_DOUBLE_TAP, int_stat)) {
+		ret = iio_push_event(indio_dev,
+				     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0, tap_dir,
+							IIO_EV_TYPE_GESTURE,
+							IIO_EV_DIR_DOUBLETAP),
+				     ts);
+		if (ret)
+			return ret;
+	}
+
 	if (FIELD_GET(ADXL345_INT_WATERMARK, int_stat)) {
 		samples = adxl345_get_samples(st);
 		if (samples < 0)
@@ -875,6 +975,8 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 	/* Init with reasonable values */
 	tap_threshold = 48;			/*   48 [0x30] -> ~3g     */
 	st->tap_duration_us = 16;		/*   16 [0x10] -> .010    */
+	st->tap_window_us = 64;			/*   64 [0x40] -> .080    */
+	st->tap_latent_us = 16;			/*   16 [0x10] -> .020    */
 
 	indio_dev->name = st->info->name;
 	indio_dev->info = &adxl345_info;
-- 
2.39.5

[PATCH v6 04/11] iio: accel: adxl345: set the tap suppress bit permanently

[Lothar Rubusch]

Set the suppress bit feature to the double tap detection, whenever
double tap is enabled. This impedes the suppress bit dangling in any
state, and thus varying in sensitivity for double tap detection.

Any tap event is defined by a rising signal edge above threshold, i.e.
duration time starts counting; and the falling edge under threshold
within duration time, i.e. then the tap event is issued. This means
duration is used individually for each tap event.

For double tap detection after a single tap, a latency time needs to be
specified. Usually tap events, i.e. spikes above and returning below
threshold will be ignored within latency. After latency, the window
time starts counting for a second tap detection which has to happen
within a duration time.

If the suppress bit is not set, spikes within latency time are ignored.
Setting the suppress bit will invalidate the double tap function. The
sensor will thus be able to save the window time for double tap
detection, and follow a more strict definition of what signal qualifies
for a double tap.

In a summary having the suppress bit set, fewer signal spikes will be
considered as double taps. This is an optional add on to double tap,
thus a separate patch.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 drivers/iio/accel/adxl345_core.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index a95f1c218c0c..c464c87033fb 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -34,6 +34,8 @@
 #define ADXL345_INT2			1
 
 #define ADXL345_REG_TAP_AXIS_MSK	GENMASK(2, 0)
+#define ADXL345_REG_TAP_SUPPRESS_MSK	BIT(3)
+#define ADXL345_REG_TAP_SUPPRESS	BIT(3)
 
 #define ADXL345_TAP_Z_EN		BIT(0)
 #define ADXL345_TAP_Y_EN		BIT(1)
@@ -295,6 +297,18 @@ static int adxl345_set_singletap_en(struct adxl345_state *st,
 
 static int adxl345_set_doubletap_en(struct adxl345_state *st, bool en)
 {
+	int ret;
+
+	/*
+	 * Generally suppress detection of spikes during the latency period as
+	 * double taps here, this is fully optional for double tap detection
+	 */
+	ret = regmap_update_bits(st->regmap, ADXL345_REG_TAP_AXIS,
+				 ADXL345_REG_TAP_SUPPRESS_MSK,
+				 en ? ADXL345_REG_TAP_SUPPRESS : 0x00);
+	if (ret)
+		return ret;
+
 	return _adxl345_set_tap_int(st, ADXL345_DOUBLE_TAP, en);
 }
 
-- 
2.39.5

[PATCH v6 05/11] iio: accel: adxl345: add freefall feature

[Lothar Rubusch]

Add the freefall detection of the sensor together with a threshold and
time parameter. A freefall event is detected if the measuring signal
falls below the threshold.

Introduce a freefall threshold stored in regmap cache, and a freefall
time, having the scaled time value stored as a member variable in the
state instance.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 drivers/iio/accel/adxl345_core.c | 125 +++++++++++++++++++++++++++++++
 1 file changed, 125 insertions(+)

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index c464c87033fb..ae02826e552b 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -75,6 +75,7 @@ struct adxl345_state {
 	u32 tap_duration_us;
 	u32 tap_latent_us;
 	u32 tap_window_us;
+	u32 ff_time_ms;
 
 	__le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE + 1] __aligned(IIO_DMA_MINALIGN);
 };
@@ -96,6 +97,14 @@ static struct iio_event_spec adxl345_events[] = {
 			BIT(IIO_EV_INFO_RESET_TIMEOUT) |
 			BIT(IIO_EV_INFO_TAP2_MIN_DELAY),
 	},
+	{
+		/* free fall */
+		.type = IIO_EV_TYPE_MAG,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE) |
+			BIT(IIO_EV_INFO_VALUE) |
+			BIT(IIO_EV_INFO_PERIOD),
+	},
 };
 
 #define ADXL345_CHANNEL(index, reg, axis) {					\
@@ -383,6 +392,63 @@ static int adxl345_set_tap_latent(struct adxl345_state *st, u32 val_int,
 	return _adxl345_set_tap_time(st, ADXL345_TAP_TIME_LATENT, val_fract_us);
 }
 
+/* freefall */
+
+static int adxl345_is_ff_en(struct adxl345_state *st, bool *en)
+{
+	int ret;
+	unsigned int regval;
+
+	ret = regmap_read(st->regmap, ADXL345_REG_INT_ENABLE, &regval);
+	if (ret)
+		return ret;
+
+	*en = FIELD_GET(ADXL345_INT_FREE_FALL, regval) > 0;
+
+	return 0;
+}
+
+static int adxl345_set_ff_en(struct adxl345_state *st, bool cmd_en)
+{
+	unsigned int regval, ff_threshold;
+	bool en;
+	int ret;
+
+	ret = regmap_read(st->regmap, ADXL345_REG_THRESH_FF, &ff_threshold);
+	if (ret)
+		return ret;
+
+	en = cmd_en && ff_threshold > 0 && st->ff_time_ms > 0;
+
+	regval = en ? ADXL345_INT_FREE_FALL : 0x00;
+
+	return regmap_update_bits(st->regmap, ADXL345_REG_INT_ENABLE,
+				  ADXL345_INT_FREE_FALL, regval);
+}
+
+static int adxl345_set_ff_time(struct adxl345_state *st, u32 val_int,
+			       u32 val_fract_us)
+{
+	unsigned int regval;
+	int val_ms;
+
+	/*
+	 * max value is 255 * 5000 us = 1.275000 seconds
+	 *
+	 * Note: the scaling is similar to the scaling in the ADXL380
+	 */
+	if (1000000 * val_int + val_fract_us > 1275000)
+		return -EINVAL;
+
+	val_ms = val_int * 1000 + DIV_ROUND_UP(val_fract_us, 1000);
+	st->ff_time_ms = val_ms;
+
+	regval = DIV_ROUND_CLOSEST(val_ms, 5);
+
+	/* Values between 100ms and 350ms (0x14 to 0x46) are recommended. */
+	return regmap_write(st->regmap, ADXL345_REG_TIME_FF, min(regval, 0xff));
+}
+
 static int adxl345_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan,
 			    int *val, int *val2, long mask)
@@ -495,6 +561,11 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
 		default:
 			return -EINVAL;
 		}
+	case IIO_EV_TYPE_MAG:
+		ret = adxl345_is_ff_en(st, &int_en);
+		if (ret)
+			return ret;
+		return int_en;
 	default:
 		return -EINVAL;
 	}
@@ -518,6 +589,8 @@ static int adxl345_write_event_config(struct iio_dev *indio_dev,
 		default:
 			return -EINVAL;
 		}
+	case IIO_EV_TYPE_MAG:
+		return adxl345_set_ff_en(st, state);
 	default:
 		return -EINVAL;
 	}
@@ -532,6 +605,7 @@ static int adxl345_read_event_value(struct iio_dev *indio_dev,
 {
 	struct adxl345_state *st = iio_priv(indio_dev);
 	unsigned int tap_threshold;
+	unsigned int ff_threshold;
 	int ret;
 
 	switch (type) {
@@ -565,6 +639,22 @@ static int adxl345_read_event_value(struct iio_dev *indio_dev,
 		default:
 			return -EINVAL;
 		}
+	case IIO_EV_TYPE_MAG:
+		switch (info) {
+		case IIO_EV_INFO_VALUE:
+			ret = regmap_read(st->regmap, ADXL345_REG_THRESH_FF,
+					  &ff_threshold);
+			if (ret)
+				return ret;
+			*val = ff_threshold;
+			return IIO_VAL_INT;
+		case IIO_EV_INFO_PERIOD:
+			*val = st->ff_time_ms;
+			*val2 = 1000;
+			return IIO_VAL_FRACTIONAL;
+		default:
+			return -EINVAL;
+		}
 	default:
 		return -EINVAL;
 	}
@@ -612,6 +702,22 @@ static int adxl345_write_event_value(struct iio_dev *indio_dev,
 			return -EINVAL;
 		}
 		break;
+	case IIO_EV_TYPE_MAG:
+		switch (info) {
+		case IIO_EV_INFO_VALUE:
+			ret = regmap_write(st->regmap, ADXL345_REG_THRESH_FF, val);
+			if (ret)
+				return ret;
+			break;
+		case IIO_EV_INFO_PERIOD:
+			ret = adxl345_set_ff_time(st, val, val2);
+			if (ret)
+				return ret;
+			break;
+		default:
+			return -EINVAL;
+		}
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -865,6 +971,17 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
 			return ret;
 	}
 
+	if (FIELD_GET(ADXL345_INT_FREE_FALL, int_stat)) {
+		ret = iio_push_event(indio_dev,
+				     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
+							IIO_MOD_X_OR_Y_OR_Z,
+							IIO_EV_TYPE_MAG,
+							IIO_EV_DIR_FALLING),
+				     ts);
+		if (ret)
+			return ret;
+	}
+
 	if (FIELD_GET(ADXL345_INT_WATERMARK, int_stat)) {
 		samples = adxl345_get_samples(st);
 		if (samples < 0)
@@ -973,6 +1090,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 					 ADXL345_DATA_FORMAT_FULL_RES |
 					 ADXL345_DATA_FORMAT_SELF_TEST);
 	unsigned int tap_threshold;
+	unsigned int ff_threshold;
 	int ret;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
@@ -992,6 +1110,9 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 	st->tap_window_us = 64;			/*   64 [0x40] -> .080    */
 	st->tap_latent_us = 16;			/*   16 [0x10] -> .020    */
 
+	ff_threshold = 8;			/*    8 [0x08]            */
+	st->ff_time_ms = 32;			/*   32 [0x20] -> 0.16    */
+
 	indio_dev->name = st->info->name;
 	indio_dev->info = &adxl345_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
@@ -1068,6 +1189,10 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 		if (ret)
 			return ret;
 
+		ret = regmap_write(st->regmap, ADXL345_REG_THRESH_FF, ff_threshold);
+		if (ret)
+			return ret;
+
 		/* FIFO_STREAM mode is going to be activated later */
 		ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, &adxl345_buffer_ops);
 		if (ret)
-- 
2.39.5

[PATCH v6 06/11] iio: accel: adxl345: extend sample frequency adjustments

[Lothar Rubusch]

Introduce enums and functions to work with the sample frequency
adjustments. Let the sample frequency adjust via IIO and configure
a reasonable default.

Replace the old static sample frequency handling. During adjustment of
bw registers, measuring is disabled and afterwards enabled again.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 drivers/iio/accel/adxl345.h      |   2 +-
 drivers/iio/accel/adxl345_core.c | 150 ++++++++++++++++++++++++-------
 2 files changed, 118 insertions(+), 34 deletions(-)

diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
index 7d482dd595fa..6c1f96406136 100644
--- a/drivers/iio/accel/adxl345.h
+++ b/drivers/iio/accel/adxl345.h
@@ -69,7 +69,7 @@
  * BW_RATE bits - Bandwidth and output data rate. The default value is
  * 0x0A, which translates to a 100 Hz output data rate
  */
-#define ADXL345_BW_RATE			GENMASK(3, 0)
+#define ADXL345_BW_RATE_MSK		GENMASK(3, 0)
 #define ADXL345_BW_LOW_POWER		BIT(4)
 #define ADXL345_BASE_RATE_NANO_HZ	97656250LL
 
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index ae02826e552b..3db757edfad3 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -64,6 +64,45 @@ static const unsigned int adxl345_tap_time_reg[] = {
 	[ADXL345_TAP_TIME_DUR] = ADXL345_REG_DUR,
 };
 
+enum adxl345_odr {
+	ADXL345_ODR_0P10HZ = 0,
+	ADXL345_ODR_0P20HZ,
+	ADXL345_ODR_0P39HZ,
+	ADXL345_ODR_0P78HZ,
+	ADXL345_ODR_1P56HZ,
+	ADXL345_ODR_3P13HZ,
+	ADXL345_ODR_6P25HZ,
+	ADXL345_ODR_12P50HZ,
+	ADXL345_ODR_25HZ,
+	ADXL345_ODR_50HZ,
+	ADXL345_ODR_100HZ,
+	ADXL345_ODR_200HZ,
+	ADXL345_ODR_400HZ,
+	ADXL345_ODR_800HZ,
+	ADXL345_ODR_1600HZ,
+	ADXL345_ODR_3200HZ,
+};
+
+/* Certain features recommend 12.5 Hz - 400 Hz ODR */
+static const int adxl345_odr_tbl[][2] = {
+	[ADXL345_ODR_0P10HZ]	= {    0,  97000 },
+	[ADXL345_ODR_0P20HZ]	= {    0, 195000 },
+	[ADXL345_ODR_0P39HZ]	= {    0, 390000 },
+	[ADXL345_ODR_0P78HZ]	= {    0, 781000 },
+	[ADXL345_ODR_1P56HZ]	= {    1, 562000 },
+	[ADXL345_ODR_3P13HZ]	= {    3, 125000 },
+	[ADXL345_ODR_6P25HZ]	= {    6, 250000 },
+	[ADXL345_ODR_12P50HZ]	= {   12, 500000 },
+	[ADXL345_ODR_25HZ]	= {   25, 0 },
+	[ADXL345_ODR_50HZ]	= {   50, 0 },
+	[ADXL345_ODR_100HZ]	= {  100, 0 },
+	[ADXL345_ODR_200HZ]	= {  200, 0 },
+	[ADXL345_ODR_400HZ]	= {  400, 0 },
+	[ADXL345_ODR_800HZ]	= {  800, 0 },
+	[ADXL345_ODR_1600HZ]	= { 1600, 0 },
+	[ADXL345_ODR_3200HZ]	= { 3200, 0 },
+};
+
 struct adxl345_state {
 	const struct adxl345_chip_info *info;
 	struct regmap *regmap;
@@ -116,6 +155,7 @@ static struct iio_event_spec adxl345_events[] = {
 		BIT(IIO_CHAN_INFO_CALIBBIAS),				\
 	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |		\
 		BIT(IIO_CHAN_INFO_SAMP_FREQ),				\
+	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),		\
 	.scan_index = (index),				\
 	.scan_type = {					\
 		.sign = 's',				\
@@ -449,14 +489,53 @@ static int adxl345_set_ff_time(struct adxl345_state *st, u32 val_int,
 	return regmap_write(st->regmap, ADXL345_REG_TIME_FF, min(regval, 0xff));
 }
 
+static int adxl345_find_odr(struct adxl345_state *st, int val,
+			    int val2, enum adxl345_odr *odr)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(adxl345_odr_tbl); i++) {
+		if (val == adxl345_odr_tbl[i][0] &&
+		    val2 == adxl345_odr_tbl[i][1]) {
+			*odr = i;
+			return 0;
+		}
+	}
+
+	return -EINVAL;
+}
+
+static int adxl345_set_odr(struct adxl345_state *st, enum adxl345_odr odr)
+{
+	return regmap_update_bits(st->regmap, ADXL345_REG_BW_RATE,
+				 ADXL345_BW_RATE_MSK,
+				 FIELD_PREP(ADXL345_BW_RATE_MSK, odr));
+}
+
+static int adxl345_read_avail(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      const int **vals, int *type,
+			      int *length, long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*vals = (int *)adxl345_odr_tbl;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		*length = ARRAY_SIZE(adxl345_odr_tbl) * 2;
+		return IIO_AVAIL_LIST;
+	}
+
+	return -EINVAL;
+}
+
 static int adxl345_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan,
 			    int *val, int *val2, long mask)
 {
 	struct adxl345_state *st = iio_priv(indio_dev);
 	__le16 accel;
-	long long samp_freq_nhz;
 	unsigned int regval;
+	enum adxl345_odr odr;
 	int ret;
 
 	switch (mask) {
@@ -494,12 +573,10 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
 		ret = regmap_read(st->regmap, ADXL345_REG_BW_RATE, &regval);
 		if (ret)
 			return ret;
-
-		samp_freq_nhz = ADXL345_BASE_RATE_NANO_HZ <<
-				(regval & ADXL345_BW_RATE);
-		*val = div_s64_rem(samp_freq_nhz, NANOHZ_PER_HZ, val2);
-
-		return IIO_VAL_INT_PLUS_NANO;
+		odr = FIELD_GET(ADXL345_BW_RATE_MSK, regval);
+		*val = adxl345_odr_tbl[odr][0];
+		*val2 = adxl345_odr_tbl[odr][1];
+		return IIO_VAL_INT_PLUS_MICRO;
 	}
 
 	return -EINVAL;
@@ -510,7 +587,12 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
 			     int val, int val2, long mask)
 {
 	struct adxl345_state *st = iio_priv(indio_dev);
-	s64 n;
+	enum adxl345_odr odr;
+	int ret;
+
+	ret = adxl345_set_measure_en(st, false);
+	if (ret)
+		return ret;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_CALIBBIAS:
@@ -518,20 +600,26 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
 		 * 8-bit resolution at +/- 2g, that is 4x accel data scale
 		 * factor
 		 */
-		return regmap_write(st->regmap,
-				    ADXL345_REG_OFS_AXIS(chan->address),
-				    val / 4);
+		ret = regmap_write(st->regmap,
+				   ADXL345_REG_OFS_AXIS(chan->address),
+				   val / 4);
+		if (ret)
+			return ret;
+		break;
 	case IIO_CHAN_INFO_SAMP_FREQ:
-		n = div_s64(val * NANOHZ_PER_HZ + val2,
-			    ADXL345_BASE_RATE_NANO_HZ);
+		ret = adxl345_find_odr(st, val, val2, &odr);
+		if (ret)
+			return ret;
 
-		return regmap_update_bits(st->regmap, ADXL345_REG_BW_RATE,
-					  ADXL345_BW_RATE,
-					  clamp_val(ilog2(n), 0,
-						    ADXL345_BW_RATE));
+		ret = adxl345_set_odr(st, odr);
+		if (ret)
+			return ret;
+		break;
+	default:
+		return -EINVAL;
 	}
 
-	return -EINVAL;
+	return adxl345_set_measure_en(st, true);
 }
 
 static int adxl345_read_event_config(struct iio_dev *indio_dev,
@@ -760,7 +848,7 @@ static int adxl345_write_raw_get_fmt(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_CALIBBIAS:
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SAMP_FREQ:
-		return IIO_VAL_INT_PLUS_NANO;
+		return IIO_VAL_INT_PLUS_MICRO;
 	default:
 		return -EINVAL;
 	}
@@ -773,19 +861,6 @@ static void adxl345_powerdown(void *ptr)
 	adxl345_set_measure_en(st, false);
 }
 
-static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
-"0.09765625 0.1953125 0.390625 0.78125 1.5625 3.125 6.25 12.5 25 50 100 200 400 800 1600 3200"
-);
-
-static struct attribute *adxl345_attrs[] = {
-	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
-	NULL
-};
-
-static const struct attribute_group adxl345_attrs_group = {
-	.attrs = adxl345_attrs,
-};
-
 static int adxl345_set_fifo(struct adxl345_state *st)
 {
 	unsigned int intio;
@@ -1048,9 +1123,9 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
 }
 
 static const struct iio_info adxl345_info = {
-	.attrs		= &adxl345_attrs_group,
 	.read_raw	= adxl345_read_raw,
 	.write_raw	= adxl345_write_raw,
+	.read_avail	= adxl345_read_avail,
 	.write_raw_get_fmt	= adxl345_write_raw_get_fmt,
 	.read_event_config = adxl345_read_event_config,
 	.write_event_config = adxl345_write_event_config,
@@ -1120,6 +1195,15 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 	indio_dev->num_channels = ARRAY_SIZE(adxl345_channels);
 	indio_dev->available_scan_masks = adxl345_scan_masks;
 
+	/*
+	 * Using I2C at 100kHz would limit the maximum ODR to 200Hz, operation
+	 * at an output rate above the recommended maximum may result in
+	 * undesired behavior.
+	 */
+	ret = adxl345_set_odr(st, ADXL345_ODR_200HZ);
+	if (ret)
+		return ret;
+
 	/* Reset interrupts at start up */
 	ret = regmap_write(st->regmap, ADXL345_REG_INT_ENABLE, 0x00);
 	if (ret)
-- 
2.39.5

[PATCH v6 07/11] iio: accel: adxl345: add g-range configuration

[Lothar Rubusch]

Introduce a mechanism to be able to configure and work with the available
g-ranges keeping the precision of 13 digits.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 drivers/iio/accel/adxl345_core.c | 90 ++++++++++++++++++++++++++++++--
 1 file changed, 87 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 3db757edfad3..166f549ba375 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -83,6 +83,13 @@ enum adxl345_odr {
 	ADXL345_ODR_3200HZ,
 };
 
+enum adxl345_range {
+	ADXL345_2G_RANGE = 0,
+	ADXL345_4G_RANGE,
+	ADXL345_8G_RANGE,
+	ADXL345_16G_RANGE,
+};
+
 /* Certain features recommend 12.5 Hz - 400 Hz ODR */
 static const int adxl345_odr_tbl[][2] = {
 	[ADXL345_ODR_0P10HZ]	= {    0,  97000 },
@@ -103,6 +110,33 @@ static const int adxl345_odr_tbl[][2] = {
 	[ADXL345_ODR_3200HZ]	= { 3200, 0 },
 };
 
+/*
+ * Full resolution frequency table:
+ * (g * 2 * 9.80665) / (2^(resolution) - 1)
+ *
+ * resolution := 13 (full)
+ * g := 2|4|8|16
+ *
+ *  2g at 13bit: 0.004789
+ *  4g at 13bit: 0.009578
+ *  8g at 13bit: 0.019156
+ * 16g at 16bit: 0.038312
+ */
+static const int adxl345_fullres_range_tbl[][2] = {
+	[ADXL345_2G_RANGE]  = { 0, 4789 },
+	[ADXL345_4G_RANGE]  = { 0, 9578 },
+	[ADXL345_8G_RANGE]  = { 0, 19156 },
+	[ADXL345_16G_RANGE] = { 0, 38312 },
+};
+
+/* scaling */
+static const int adxl345_range_factor_tbl[] = {
+	[ADXL345_2G_RANGE]  = 1,
+	[ADXL345_4G_RANGE]  = 2,
+	[ADXL345_8G_RANGE]  = 4,
+	[ADXL345_16G_RANGE] = 8,
+};
+
 struct adxl345_state {
 	const struct adxl345_chip_info *info;
 	struct regmap *regmap;
@@ -155,7 +189,8 @@ static struct iio_event_spec adxl345_events[] = {
 		BIT(IIO_CHAN_INFO_CALIBBIAS),				\
 	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |		\
 		BIT(IIO_CHAN_INFO_SAMP_FREQ),				\
-	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),		\
+	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE) | \
+		BIT(IIO_CHAN_INFO_SAMP_FREQ),		\
 	.scan_index = (index),				\
 	.scan_type = {					\
 		.sign = 's',				\
@@ -512,12 +547,40 @@ static int adxl345_set_odr(struct adxl345_state *st, enum adxl345_odr odr)
 				 FIELD_PREP(ADXL345_BW_RATE_MSK, odr));
 }
 
+static int adxl345_find_range(struct adxl345_state *st, int val, int val2,
+			      enum adxl345_range *range)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(adxl345_fullres_range_tbl); i++) {
+		if (val == adxl345_fullres_range_tbl[i][0] &&
+		    val2 == adxl345_fullres_range_tbl[i][1]) {
+			*range = i;
+			return 0;
+		}
+	}
+
+	return -EINVAL;
+}
+
+static int adxl345_set_range(struct adxl345_state *st, enum adxl345_range range)
+{
+	return regmap_update_bits(st->regmap, ADXL345_REG_DATA_FORMAT,
+				 ADXL345_DATA_FORMAT_RANGE,
+				 FIELD_PREP(ADXL345_DATA_FORMAT_RANGE, range));
+}
+
 static int adxl345_read_avail(struct iio_dev *indio_dev,
 			      struct iio_chan_spec const *chan,
 			      const int **vals, int *type,
 			      int *length, long mask)
 {
 	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		*vals = (int *)adxl345_fullres_range_tbl;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		*length = ARRAY_SIZE(adxl345_fullres_range_tbl) * 2;
+		return IIO_AVAIL_LIST;
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		*vals = (int *)adxl345_odr_tbl;
 		*type = IIO_VAL_INT_PLUS_MICRO;
@@ -536,6 +599,7 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
 	__le16 accel;
 	unsigned int regval;
 	enum adxl345_odr odr;
+	enum adxl345_range range;
 	int ret;
 
 	switch (mask) {
@@ -554,8 +618,12 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
 		*val = sign_extend32(le16_to_cpu(accel), 12);
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
-		*val = 0;
-		*val2 = st->info->uscale;
+		ret = regmap_read(st->regmap, ADXL345_REG_DATA_FORMAT, &regval);
+		if (ret)
+			return ret;
+		range = FIELD_GET(ADXL345_DATA_FORMAT_RANGE, regval);
+		*val = adxl345_fullres_range_tbl[range][0];
+		*val2 = adxl345_fullres_range_tbl[range][1];
 		return IIO_VAL_INT_PLUS_MICRO;
 	case IIO_CHAN_INFO_CALIBBIAS:
 		ret = regmap_read(st->regmap,
@@ -587,6 +655,7 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
 			     int val, int val2, long mask)
 {
 	struct adxl345_state *st = iio_priv(indio_dev);
+	enum adxl345_range range;
 	enum adxl345_odr odr;
 	int ret;
 
@@ -615,6 +684,15 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
 		if (ret)
 			return ret;
 		break;
+	case IIO_CHAN_INFO_SCALE:
+		ret = adxl345_find_range(st, val, val2,	&range);
+		if (ret)
+			return ret;
+
+		ret = adxl345_set_range(st, range);
+		if (ret)
+			return ret;
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -847,6 +925,8 @@ static int adxl345_write_raw_get_fmt(struct iio_dev *indio_dev,
 	switch (mask) {
 	case IIO_CHAN_INFO_CALIBBIAS:
 		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		return IIO_VAL_INT_PLUS_MICRO;
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		return IIO_VAL_INT_PLUS_MICRO;
 	default:
@@ -1204,6 +1284,10 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 	if (ret)
 		return ret;
 
+	ret = adxl345_set_range(st, ADXL345_16G_RANGE);
+	if (ret)
+		return ret;
+
 	/* Reset interrupts at start up */
 	ret = regmap_write(st->regmap, ADXL345_REG_INT_ENABLE, 0x00);
 	if (ret)
-- 
2.39.5

[PATCH v6 08/11] iio: accel: adxl345: add activity event feature

[Lothar Rubusch]

Make the sensor detect and issue interrupts at activity. Activity
events are configured by a threshold stored in regmap cache. Initialize
the activity threshold register to a reasonable default value in probe.
The value is taken from the older ADXL345 input driver, to provide a
similar behavior. Reset the activity/inactivity direction enabling
register in probe. Reset and initialization shall bring the sensor in a
defined initial state to prevent dangling settings when warm restarting
the sensor.

Activity, ODR configuration together with the range setting prepare the
activity/inactivity hystersesis setup, implemented in a follow up patch.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 drivers/iio/accel/adxl345_core.c | 217 ++++++++++++++++++++++++++++++-
 1 file changed, 214 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 166f549ba375..8f7ea3928cf8 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -36,11 +36,16 @@
 #define ADXL345_REG_TAP_AXIS_MSK	GENMASK(2, 0)
 #define ADXL345_REG_TAP_SUPPRESS_MSK	BIT(3)
 #define ADXL345_REG_TAP_SUPPRESS	BIT(3)
+#define ADXL345_REG_ACT_AXIS_MSK	GENMASK(6, 4)
 
 #define ADXL345_TAP_Z_EN		BIT(0)
 #define ADXL345_TAP_Y_EN		BIT(1)
 #define ADXL345_TAP_X_EN		BIT(2)
 
+#define ADXL345_ACT_Z_EN		BIT(4)
+#define ADXL345_ACT_Y_EN		BIT(5)
+#define ADXL345_ACT_X_EN		BIT(6)
+
 /* single/double tap */
 enum adxl345_tap_type {
 	ADXL345_SINGLE_TAP,
@@ -64,6 +69,19 @@ static const unsigned int adxl345_tap_time_reg[] = {
 	[ADXL345_TAP_TIME_DUR] = ADXL345_REG_DUR,
 };
 
+/* activity/inactivity */
+enum adxl345_activity_type {
+	ADXL345_ACTIVITY,
+};
+
+static const unsigned int adxl345_act_int_reg[] = {
+	[ADXL345_ACTIVITY] = ADXL345_INT_ACTIVITY,
+};
+
+static const unsigned int adxl345_act_thresh_reg[] = {
+	[ADXL345_ACTIVITY] = ADXL345_REG_THRESH_ACT,
+};
+
 enum adxl345_odr {
 	ADXL345_ODR_0P10HZ = 0,
 	ADXL345_ODR_0P20HZ,
@@ -154,6 +172,13 @@ struct adxl345_state {
 };
 
 static struct iio_event_spec adxl345_events[] = {
+	{
+		/* activity */
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
+		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE),
+	},
 	{
 		/* single tap */
 		.type = IIO_EV_TYPE_GESTURE,
@@ -256,6 +281,99 @@ static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
 	return regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
 }
 
+/* act/inact */
+
+static int adxl345_is_act_inact_en(struct adxl345_state *st,
+				   enum iio_modifier axis,
+				   enum adxl345_activity_type type, bool *en)
+{
+	unsigned int regval;
+	u32 axis_ctrl;
+	int ret;
+
+	ret = regmap_read(st->regmap, ADXL345_REG_ACT_INACT_CTRL, &axis_ctrl);
+	if (ret)
+		return ret;
+
+	if (type == ADXL345_ACTIVITY) {
+		switch (axis) {
+		case IIO_MOD_X:
+			*en = FIELD_GET(ADXL345_ACT_X_EN, axis_ctrl);
+			break;
+		case IIO_MOD_Y:
+			*en = FIELD_GET(ADXL345_ACT_Y_EN, axis_ctrl);
+			break;
+		case IIO_MOD_Z:
+			*en = FIELD_GET(ADXL345_ACT_Z_EN, axis_ctrl);
+			break;
+		default:
+			*en = false;
+			return -EINVAL;
+		}
+	}
+
+	if (*en) {
+		ret = regmap_read(st->regmap, ADXL345_REG_INT_ENABLE, &regval);
+		if (ret)
+			return ret;
+
+		*en = adxl345_act_int_reg[type] & regval;
+	}
+
+	return 0;
+}
+
+static int adxl345_set_act_inact_en(struct adxl345_state *st,
+				    enum iio_modifier axis,
+				    enum adxl345_activity_type type,
+				    bool cmd_en)
+{
+	bool en;
+	unsigned int threshold;
+	u32 axis_ctrl = 0;
+	int ret;
+
+	if (type == ADXL345_ACTIVITY) {
+		switch (axis) {
+		case IIO_MOD_X:
+			axis_ctrl = ADXL345_ACT_X_EN;
+			break;
+		case IIO_MOD_Y:
+			axis_ctrl = ADXL345_ACT_Y_EN;
+			break;
+		case IIO_MOD_Z:
+			axis_ctrl = ADXL345_ACT_Z_EN;
+			break;
+		default:
+			return -EINVAL;
+		}
+	}
+
+	if (cmd_en)
+		ret = regmap_set_bits(st->regmap,
+				      ADXL345_REG_ACT_INACT_CTRL, axis_ctrl);
+	else
+		ret = regmap_clear_bits(st->regmap,
+					ADXL345_REG_ACT_INACT_CTRL, axis_ctrl);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(st->regmap, adxl345_act_thresh_reg[type], &threshold);
+	if (ret)
+		return ret;
+
+	en = false;
+
+	if (type == ADXL345_ACTIVITY) {
+		en = FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, axis_ctrl) &&
+			threshold;
+	}
+
+	return regmap_update_bits(st->regmap, ADXL345_REG_INT_ENABLE,
+				  adxl345_act_int_reg[type],
+				  en ? adxl345_act_int_reg[type] : 0);
+}
+
 /* tap */
 
 static int _adxl345_set_tap_int(struct adxl345_state *st,
@@ -710,6 +828,18 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
 	int ret;
 
 	switch (type) {
+	case IIO_EV_TYPE_THRESH:
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			ret = adxl345_is_act_inact_en(st, chan->channel2,
+						      ADXL345_ACTIVITY,
+						      &int_en);
+			if (ret)
+				return ret;
+			return int_en;
+		default:
+			return -EINVAL;
+		}
 	case IIO_EV_TYPE_GESTURE:
 		switch (dir) {
 		case IIO_EV_DIR_SINGLETAP:
@@ -746,6 +876,14 @@ static int adxl345_write_event_config(struct iio_dev *indio_dev,
 	struct adxl345_state *st = iio_priv(indio_dev);
 
 	switch (type) {
+	case IIO_EV_TYPE_THRESH:
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			return adxl345_set_act_inact_en(st, chan->channel2,
+							ADXL345_ACTIVITY, state);
+		default:
+			return -EINVAL;
+		}
 	case IIO_EV_TYPE_GESTURE:
 		switch (dir) {
 		case IIO_EV_DIR_SINGLETAP:
@@ -770,11 +908,31 @@ static int adxl345_read_event_value(struct iio_dev *indio_dev,
 				    int *val, int *val2)
 {
 	struct adxl345_state *st = iio_priv(indio_dev);
+	unsigned int act_threshold;
 	unsigned int tap_threshold;
 	unsigned int ff_threshold;
 	int ret;
 
 	switch (type) {
+	case IIO_EV_TYPE_THRESH:
+		switch (info) {
+		case IIO_EV_INFO_VALUE:
+			switch (dir) {
+			case IIO_EV_DIR_RISING:
+				ret = regmap_read(st->regmap,
+						  adxl345_act_thresh_reg[ADXL345_ACTIVITY],
+						  &act_threshold);
+				if (ret)
+					return ret;
+
+				*val = act_threshold;
+				return IIO_VAL_INT;
+			default:
+				return -EINVAL;
+			}
+		default:
+			return -EINVAL;
+		}
 	case IIO_EV_TYPE_GESTURE:
 		switch (info) {
 		case IIO_EV_INFO_VALUE:
@@ -841,6 +999,25 @@ static int adxl345_write_event_value(struct iio_dev *indio_dev,
 		return ret;
 
 	switch (type) {
+	case IIO_EV_TYPE_THRESH:
+		switch (info) {
+		case IIO_EV_INFO_VALUE:
+			switch (dir) {
+			case IIO_EV_DIR_RISING:
+				ret = regmap_write(st->regmap,
+						   adxl345_act_thresh_reg[ADXL345_ACTIVITY],
+						   val);
+				if (ret)
+					return ret;
+				break;
+			default:
+				return -EINVAL;
+			}
+			break;
+		default:
+			return -EINVAL;
+		}
+		break;
 	case IIO_EV_TYPE_GESTURE:
 		switch (info) {
 		case IIO_EV_INFO_VALUE:
@@ -1099,7 +1276,8 @@ static int adxl345_fifo_push(struct iio_dev *indio_dev,
 }
 
 static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
-			      enum iio_modifier tap_dir)
+			      enum iio_modifier tap_dir,
+			      enum iio_modifier act_dir)
 {
 	s64 ts = iio_get_time_ns(indio_dev);
 	struct adxl345_state *st = iio_priv(indio_dev);
@@ -1126,6 +1304,16 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
 			return ret;
 	}
 
+	if (FIELD_GET(ADXL345_INT_ACTIVITY, int_stat)) {
+		ret = iio_push_event(indio_dev,
+				     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0, act_dir,
+							IIO_EV_TYPE_THRESH,
+							IIO_EV_DIR_RISING),
+				     ts);
+		if (ret)
+			return ret;
+	}
+
 	if (FIELD_GET(ADXL345_INT_FREE_FALL, int_stat)) {
 		ret = iio_push_event(indio_dev,
 				     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
@@ -1164,6 +1352,7 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
 	struct adxl345_state *st = iio_priv(indio_dev);
 	unsigned int regval;
 	enum iio_modifier tap_dir = IIO_NO_MOD;
+	enum iio_modifier act_dir = IIO_NO_MOD;
 	u32 axis_ctrl;
 	int int_stat;
 	int ret;
@@ -1172,7 +1361,8 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
 	if (ret)
 		return IRQ_NONE;
 
-	if (FIELD_GET(ADXL345_REG_TAP_AXIS_MSK, axis_ctrl)) {
+	if (FIELD_GET(ADXL345_REG_TAP_AXIS_MSK, axis_ctrl) ||
+	    FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, axis_ctrl)) {
 		ret = regmap_read(st->regmap, ADXL345_REG_ACT_TAP_STATUS, &regval);
 		if (ret)
 			return IRQ_NONE;
@@ -1183,12 +1373,19 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
 			tap_dir = IIO_MOD_Y;
 		else if (FIELD_GET(ADXL345_TAP_X_EN, regval))
 			tap_dir = IIO_MOD_X;
+
+		if (FIELD_GET(ADXL345_ACT_Z_EN, regval))
+			act_dir = IIO_MOD_Z;
+		else if (FIELD_GET(ADXL345_ACT_Y_EN, regval))
+			act_dir = IIO_MOD_Y;
+		else if (FIELD_GET(ADXL345_ACT_X_EN, regval))
+			act_dir = IIO_MOD_X;
 	}
 
 	if (regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, &int_stat))
 		return IRQ_NONE;
 
-	if (adxl345_push_event(indio_dev, int_stat, tap_dir))
+	if (adxl345_push_event(indio_dev, int_stat, tap_dir, act_dir))
 		goto err;
 
 	if (FIELD_GET(ADXL345_INT_OVERRUN, int_stat))
@@ -1353,6 +1550,20 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 		if (ret)
 			return ret;
 
+		/*
+		 * Initialization with reasonable values to simplify operation
+		 * of the sensor. The default values are partly taken from the
+		 * older input driver for the ADXL345, and partly based on
+		 * recommendations in the datasheet.
+		 */
+		ret = regmap_write(st->regmap, ADXL345_REG_ACT_INACT_CTRL, 0);
+		if (ret)
+			return ret;
+
+		ret = regmap_write(st->regmap, ADXL345_REG_THRESH_ACT, 6);
+		if (ret)
+			return ret;
+
 		ret = regmap_write(st->regmap, ADXL345_REG_THRESH_TAP, tap_threshold);
 		if (ret)
 			return ret;
-- 
2.39.5

[PATCH v6 09/11] iio: accel: adxl345: add inactivity feature

[Lothar Rubusch]

Add the inactivity feature of the sensor. When activity and inactivity
are enabled, a link bit will be set linking activity and inactivity
handling. Additionally, the auto-sleep mode will be enabled. Due to the
link bit the sensor is going to auto-sleep when inactivity was
detected.

Inactivity detection needs a threshold to be configured, and a time
after which it will go into inactivity state if measurements under
threshold.

When a ODR is configured this time for inactivity is adjusted with a
corresponding reasonable default value, in order to have higher
frequencies and lower inactivity times, and lower sample frequency but
give more time until inactivity. Both with reasonable upper and lower
boundaries, since many of the sensor's features (e.g. auto-sleep) will
need to operate beween 12.5 Hz and 400 Hz. This is a default setting
when actively changing sample frequency, explicitly setting the time
until inactivity will overwrite the default.

Similarly, setting the g-range will provide a default value for the
activity and inactivity thresholds. Both are implicit defaults, but
equally can be overwritten to be explicitly configured.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 drivers/iio/accel/adxl345_core.c | 173 ++++++++++++++++++++++++++++++-
 1 file changed, 168 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 8f7ea3928cf8..06028eaef9e0 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -37,11 +37,17 @@
 #define ADXL345_REG_TAP_SUPPRESS_MSK	BIT(3)
 #define ADXL345_REG_TAP_SUPPRESS	BIT(3)
 #define ADXL345_REG_ACT_AXIS_MSK	GENMASK(6, 4)
+#define ADXL345_REG_INACT_AXIS_MSK	GENMASK(2, 0)
+#define ADXL345_POWER_CTL_INACT_MSK	(ADXL345_POWER_CTL_AUTO_SLEEP | ADXL345_POWER_CTL_LINK)
 
 #define ADXL345_TAP_Z_EN		BIT(0)
 #define ADXL345_TAP_Y_EN		BIT(1)
 #define ADXL345_TAP_X_EN		BIT(2)
 
+#define ADXL345_INACT_Z_EN		BIT(0)
+#define ADXL345_INACT_Y_EN		BIT(1)
+#define ADXL345_INACT_X_EN		BIT(2)
+
 #define ADXL345_ACT_Z_EN		BIT(4)
 #define ADXL345_ACT_Y_EN		BIT(5)
 #define ADXL345_ACT_X_EN		BIT(6)
@@ -72,14 +78,17 @@ static const unsigned int adxl345_tap_time_reg[] = {
 /* activity/inactivity */
 enum adxl345_activity_type {
 	ADXL345_ACTIVITY,
+	ADXL345_INACTIVITY,
 };
 
 static const unsigned int adxl345_act_int_reg[] = {
 	[ADXL345_ACTIVITY] = ADXL345_INT_ACTIVITY,
+	[ADXL345_INACTIVITY] = ADXL345_INT_INACTIVITY,
 };
 
 static const unsigned int adxl345_act_thresh_reg[] = {
 	[ADXL345_ACTIVITY] = ADXL345_REG_THRESH_ACT,
+	[ADXL345_INACTIVITY] = ADXL345_REG_THRESH_INACT,
 };
 
 enum adxl345_odr {
@@ -179,6 +188,14 @@ static struct iio_event_spec adxl345_events[] = {
 		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
 		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE),
 	},
+	{
+		/* inactivity */
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
+		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
+			BIT(IIO_EV_INFO_PERIOD),
+	},
 	{
 		/* single tap */
 		.type = IIO_EV_TYPE_GESTURE,
@@ -278,7 +295,8 @@ static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
 {
 	unsigned int val = en ? ADXL345_POWER_CTL_MEASURE : ADXL345_POWER_CTL_STANDBY;
 
-	return regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
+	return regmap_update_bits(st->regmap, ADXL345_REG_POWER_CTL,
+				  ADXL345_POWER_CTL_MEASURE, val);
 }
 
 /* act/inact */
@@ -310,6 +328,21 @@ static int adxl345_is_act_inact_en(struct adxl345_state *st,
 			*en = false;
 			return -EINVAL;
 		}
+	} else {
+		switch (axis) {
+		case IIO_MOD_X:
+			*en = FIELD_GET(ADXL345_INACT_X_EN, axis_ctrl);
+			break;
+		case IIO_MOD_Y:
+			*en = FIELD_GET(ADXL345_INACT_Y_EN, axis_ctrl);
+			break;
+		case IIO_MOD_Z:
+			*en = FIELD_GET(ADXL345_INACT_Z_EN, axis_ctrl);
+			break;
+		default:
+			*en = false;
+			return -EINVAL;
+		}
 	}
 
 	if (*en) {
@@ -329,6 +362,7 @@ static int adxl345_set_act_inact_en(struct adxl345_state *st,
 				    bool cmd_en)
 {
 	bool en;
+	unsigned int inact_time_s;
 	unsigned int threshold;
 	u32 axis_ctrl = 0;
 	int ret;
@@ -347,6 +381,20 @@ static int adxl345_set_act_inact_en(struct adxl345_state *st,
 		default:
 			return -EINVAL;
 		}
+	} else {
+		switch (axis) {
+		case IIO_MOD_X:
+			axis_ctrl = ADXL345_INACT_X_EN;
+			break;
+		case IIO_MOD_Y:
+			axis_ctrl = ADXL345_INACT_Y_EN;
+			break;
+		case IIO_MOD_Z:
+			axis_ctrl = ADXL345_INACT_Z_EN;
+			break;
+		default:
+			return -EINVAL;
+		}
 	}
 
 	if (cmd_en)
@@ -367,11 +415,69 @@ static int adxl345_set_act_inact_en(struct adxl345_state *st,
 	if (type == ADXL345_ACTIVITY) {
 		en = FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, axis_ctrl) &&
 			threshold;
+	} else {
+		ret = regmap_read(st->regmap, ADXL345_REG_TIME_INACT, &inact_time_s);
+		if (ret)
+			return ret;
+
+		en = FIELD_GET(ADXL345_REG_INACT_AXIS_MSK, axis_ctrl) &&
+			threshold && inact_time_s;
 	}
 
-	return regmap_update_bits(st->regmap, ADXL345_REG_INT_ENABLE,
-				  adxl345_act_int_reg[type],
-				  en ? adxl345_act_int_reg[type] : 0);
+	ret = regmap_update_bits(st->regmap, ADXL345_REG_INT_ENABLE,
+				 adxl345_act_int_reg[type],
+				 en ? adxl345_act_int_reg[type] : 0);
+	if (ret)
+		return ret;
+
+	return regmap_update_bits(st->regmap, ADXL345_REG_POWER_CTL,
+				  ADXL345_POWER_CTL_INACT_MSK,
+				  en ? (ADXL345_POWER_CTL_AUTO_SLEEP | ADXL345_POWER_CTL_LINK)
+					: 0);
+}
+
+/**
+ * adxl345_set_inact_time_s - Configure inactivity time explicitly or by ODR.
+ * @st: The sensor state instance.
+ * @val_s: A desired time value, between 0 and 255.
+ *
+ * Inactivity time can be configured between 1 and 255 sec. If a val_s of 0
+ * is configured by a user, then a default inactivity time will be computed.
+ *
+ * In such case, it should take power consumption into consideration. Thus it
+ * shall be shorter for higher frequencies and longer for lower frequencies.
+ * Hence, frequencies above 255 Hz shall default to 10 s and frequencies below
+ * 10 Hz shall result in 255 s to detect inactivity.
+ *
+ * The approach simply subtracts the pre-decimal figure of the configured
+ * sample frequency from 255 s to compute inactivity time [s]. Sub-Hz are thus
+ * ignored in this estimation. The recommended ODRs for various features
+ * (activity/inactivity, sleep modes, free fall, etc.) lie between 12.5 Hz and
+ * 400 Hz, thus higher or lower frequencies will result in the boundary
+ * defaults or need to be explicitly specified via val_s.
+ *
+ * Return: 0 or error value.
+ */
+static int adxl345_set_inact_time_s(struct adxl345_state *st, u32 val_s)
+{
+	unsigned int max_boundary = 255;
+	unsigned int min_boundary = 10;
+	unsigned int val = min(val_s, max_boundary);
+	enum adxl345_odr odr;
+	unsigned int regval;
+	int ret;
+
+	if (val == 0) {
+		ret = regmap_read(st->regmap, ADXL345_REG_BW_RATE, &regval);
+		if (ret)
+			return ret;
+		odr = FIELD_GET(ADXL345_BW_RATE_MSK, regval);
+
+		val = (adxl345_odr_tbl[odr][0] > max_boundary)
+			? min_boundary : max_boundary -	adxl345_odr_tbl[odr][0];
+	}
+
+	return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
 }
 
 /* tap */
@@ -837,6 +943,13 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
 			if (ret)
 				return ret;
 			return int_en;
+		case IIO_EV_DIR_FALLING:
+			ret = adxl345_is_act_inact_en(st, chan->channel2,
+						      ADXL345_INACTIVITY,
+						      &int_en);
+			if (ret)
+				return ret;
+			return int_en;
 		default:
 			return -EINVAL;
 		}
@@ -881,6 +994,9 @@ static int adxl345_write_event_config(struct iio_dev *indio_dev,
 		case IIO_EV_DIR_RISING:
 			return adxl345_set_act_inact_en(st, chan->channel2,
 							ADXL345_ACTIVITY, state);
+		case IIO_EV_DIR_FALLING:
+			return adxl345_set_act_inact_en(st, chan->channel2,
+							ADXL345_INACTIVITY, state);
 		default:
 			return -EINVAL;
 		}
@@ -908,7 +1024,8 @@ static int adxl345_read_event_value(struct iio_dev *indio_dev,
 				    int *val, int *val2)
 {
 	struct adxl345_state *st = iio_priv(indio_dev);
-	unsigned int act_threshold;
+	unsigned int act_threshold, inact_threshold;
+	unsigned int inact_time_s;
 	unsigned int tap_threshold;
 	unsigned int ff_threshold;
 	int ret;
@@ -927,9 +1044,24 @@ static int adxl345_read_event_value(struct iio_dev *indio_dev,
 
 				*val = act_threshold;
 				return IIO_VAL_INT;
+			case IIO_EV_DIR_FALLING:
+				ret = regmap_read(st->regmap,
+						  adxl345_act_thresh_reg[ADXL345_INACTIVITY],
+						  &inact_threshold);
+				if (ret)
+					return ret;
+
+				*val = inact_threshold;
+				return IIO_VAL_INT;
 			default:
 				return -EINVAL;
 			}
+		case IIO_EV_INFO_PERIOD:
+			ret = regmap_read(st->regmap, ADXL345_REG_TIME_INACT, &inact_time_s);
+			if (ret)
+				return ret;
+			*val = inact_time_s;
+			return IIO_VAL_INT;
 		default:
 			return -EINVAL;
 		}
@@ -1010,10 +1142,22 @@ static int adxl345_write_event_value(struct iio_dev *indio_dev,
 				if (ret)
 					return ret;
 				break;
+			case IIO_EV_DIR_FALLING:
+				ret = regmap_write(st->regmap,
+						   adxl345_act_thresh_reg[ADXL345_INACTIVITY],
+						   val);
+				if (ret)
+					return ret;
+				break;
 			default:
 				return -EINVAL;
 			}
 			break;
+		case IIO_EV_INFO_PERIOD:
+			ret = adxl345_set_inact_time_s(st, val);
+			if (ret)
+				return ret;
+			break;
 		default:
 			return -EINVAL;
 		}
@@ -1314,6 +1458,17 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
 			return ret;
 	}
 
+	if (FIELD_GET(ADXL345_INT_INACTIVITY, int_stat)) {
+		ret = iio_push_event(indio_dev,
+				     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
+							IIO_MOD_X_OR_Y_OR_Z,
+							IIO_EV_TYPE_THRESH,
+							IIO_EV_DIR_FALLING),
+				     ts);
+		if (ret)
+			return ret;
+	}
+
 	if (FIELD_GET(ADXL345_INT_FREE_FALL, int_stat)) {
 		ret = iio_push_event(indio_dev,
 				     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
@@ -1560,10 +1715,18 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 		if (ret)
 			return ret;
 
+		ret = regmap_write(st->regmap, ADXL345_REG_TIME_INACT, 3);
+		if (ret)
+			return ret;
+
 		ret = regmap_write(st->regmap, ADXL345_REG_THRESH_ACT, 6);
 		if (ret)
 			return ret;
 
+		ret = regmap_write(st->regmap, ADXL345_REG_THRESH_INACT, 4);
+		if (ret)
+			return ret;
+
 		ret = regmap_write(st->regmap, ADXL345_REG_THRESH_TAP, tap_threshold);
 		if (ret)
 			return ret;
-- 
2.39.5

[PATCH v6 10/11] iio: accel: adxl345: add coupling detection for activity/inactivity

[Lothar Rubusch]

Add coupling activity/inactivity detection by the AC/DC bit. This is an
addititional enhancement for the detection of activity states and
completes the activity / inactivity feature of the ADXL345.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 drivers/iio/accel/adxl345_core.c | 162 ++++++++++++++++++++++++++++++-
 1 file changed, 159 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 06028eaef9e0..fe0201c7bec6 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -37,7 +37,9 @@
 #define ADXL345_REG_TAP_SUPPRESS_MSK	BIT(3)
 #define ADXL345_REG_TAP_SUPPRESS	BIT(3)
 #define ADXL345_REG_ACT_AXIS_MSK	GENMASK(6, 4)
+#define ADXL345_REG_ACT_ACDC_MSK	BIT(7)
 #define ADXL345_REG_INACT_AXIS_MSK	GENMASK(2, 0)
+#define ADXL345_REG_INACT_ACDC_MSK	BIT(3)
 #define ADXL345_POWER_CTL_INACT_MSK	(ADXL345_POWER_CTL_AUTO_SLEEP | ADXL345_POWER_CTL_LINK)
 
 #define ADXL345_TAP_Z_EN		BIT(0)
@@ -91,6 +93,11 @@ static const unsigned int adxl345_act_thresh_reg[] = {
 	[ADXL345_INACTIVITY] = ADXL345_REG_THRESH_INACT,
 };
 
+static const unsigned int adxl345_act_acdc_msk[] = {
+	[ADXL345_ACTIVITY] = ADXL345_REG_ACT_ACDC_MSK,
+	[ADXL345_INACTIVITY] = ADXL345_REG_INACT_ACDC_MSK,
+};
+
 enum adxl345_odr {
 	ADXL345_ODR_0P10HZ = 0,
 	ADXL345_ODR_0P20HZ,
@@ -220,6 +227,18 @@ static struct iio_event_spec adxl345_events[] = {
 			BIT(IIO_EV_INFO_VALUE) |
 			BIT(IIO_EV_INFO_PERIOD),
 	},
+	{
+		/* activity, activity - ac bit */
+		.type = IIO_EV_TYPE_MAG_REFERENCED,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE),
+	},
+	{
+		/* activity, inactivity - ac bit */
+		.type = IIO_EV_TYPE_MAG_REFERENCED,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE),
+	},
 };
 
 #define ADXL345_CHANNEL(index, reg, axis) {					\
@@ -301,6 +320,69 @@ static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
 
 /* act/inact */
 
+static int adxl345_is_act_inact_ac(struct adxl345_state *st,
+				   enum adxl345_activity_type type, bool *ac)
+{
+	unsigned int regval;
+	int ret;
+
+	ret = regmap_read(st->regmap, ADXL345_REG_ACT_INACT_CTRL, &regval);
+	if (ret)
+		return ret;
+
+	if (type == ADXL345_ACTIVITY)
+		*ac = FIELD_GET(ADXL345_REG_ACT_ACDC_MSK, regval);
+	else
+		*ac = FIELD_GET(ADXL345_REG_INACT_ACDC_MSK, regval);
+
+	return 0;
+}
+
+static int adxl345_set_act_inact_ac(struct adxl345_state *st,
+				    enum adxl345_activity_type type, bool ac)
+{
+	unsigned int act_inact_ac = ac ? 0xff : 0x00;
+
+	/*
+	 * A setting of false selects dc-coupled operation, and a setting of
+	 * true enables ac-coupled operation. In dc-coupled operation, the
+	 * current acceleration magnitude is compared directly with
+	 * ADXL345_REG_THRESH_ACT and ADXL345_REG_THRESH_INACT to determine
+	 * whether activity or inactivity is detected.
+	 *
+	 * In ac-coupled operation for activity detection, the acceleration
+	 * value at the start of activity detection is taken as a reference
+	 * value. New samples of acceleration are then compared to this
+	 * reference value, and if the magnitude of the difference exceeds the
+	 * ADXL345_REG_THRESH_ACT value, the device triggers an activity
+	 * interrupt.
+	 *
+	 * Similarly, in ac-coupled operation for inactivity detection, a
+	 * reference value is used for comparison and is updated whenever the
+	 * device exceeds the inactivity threshold. After the reference value
+	 * is selected, the device compares the magnitude of the difference
+	 * between the reference value and the current acceleration with
+	 * ADXL345_REG_THRESH_INACT. If the difference is less than the value in
+	 * ADXL345_REG_THRESH_INACT for the time in ADXL345_REG_TIME_INACT, the
+	 * device is considered inactive and the inactivity interrupt is
+	 * triggered. [quoted from p. 24, ADXL345 datasheet Rev. G]
+	 *
+	 * In a conclusion, the first acceleration snapshot sample which hit the
+	 * threshold in a particular direction is always taken as acceleration
+	 * reference value to that direction. Since for the hardware activity
+	 * and inactivity depend on the x/y/z axis, so do ac and dc coupling.
+	 * Note, this sw driver always enables or disables all three x/y/z axis
+	 * for detection via act_axis_ctrl and inact_axis_ctrl, respectively.
+	 * Where in dc-coupling samples are compared against the thresholds, in
+	 * ac-coupling measurement difference to the first acceleration
+	 * reference value are compared against the threshold. So, ac-coupling
+	 * allows for a bit more dynamic compensation depending on the initial
+	 * sample.
+	 */
+	return regmap_update_bits(st->regmap, ADXL345_REG_ACT_INACT_CTRL,
+				 adxl345_act_acdc_msk[type], act_inact_ac);
+}
+
 static int adxl345_is_act_inact_en(struct adxl345_state *st,
 				   enum iio_modifier axis,
 				   enum adxl345_activity_type type, bool *en)
@@ -766,9 +848,16 @@ static int adxl345_find_odr(struct adxl345_state *st, int val,
 
 static int adxl345_set_odr(struct adxl345_state *st, enum adxl345_odr odr)
 {
-	return regmap_update_bits(st->regmap, ADXL345_REG_BW_RATE,
+	int ret;
+
+	ret = regmap_update_bits(st->regmap, ADXL345_REG_BW_RATE,
 				 ADXL345_BW_RATE_MSK,
 				 FIELD_PREP(ADXL345_BW_RATE_MSK, odr));
+	if (ret)
+		return ret;
+
+	/* update inactivity time by ODR */
+	return adxl345_set_inact_time_s(st, 0);
 }
 
 static int adxl345_find_range(struct adxl345_state *st, int val, int val2,
@@ -789,9 +878,51 @@ static int adxl345_find_range(struct adxl345_state *st, int val, int val2,
 
 static int adxl345_set_range(struct adxl345_state *st, enum adxl345_range range)
 {
-	return regmap_update_bits(st->regmap, ADXL345_REG_DATA_FORMAT,
+	unsigned int act_threshold, inact_threshold;
+	unsigned int range_old;
+	unsigned int regval;
+	int ret;
+
+	ret = regmap_read(st->regmap, ADXL345_REG_DATA_FORMAT, &regval);
+	if (ret)
+		return ret;
+	range_old = FIELD_GET(ADXL345_DATA_FORMAT_RANGE, regval);
+
+	ret = regmap_read(st->regmap,
+			  adxl345_act_thresh_reg[ADXL345_ACTIVITY],
+			  &act_threshold);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(st->regmap,
+			  adxl345_act_thresh_reg[ADXL345_INACTIVITY],
+			  &inact_threshold);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(st->regmap, ADXL345_REG_DATA_FORMAT,
 				 ADXL345_DATA_FORMAT_RANGE,
 				 FIELD_PREP(ADXL345_DATA_FORMAT_RANGE, range));
+	if (ret)
+		return ret;
+
+	act_threshold = act_threshold
+		* adxl345_range_factor_tbl[range_old]
+		/ adxl345_range_factor_tbl[range];
+	act_threshold = min(255, max(1, inact_threshold));
+
+	inact_threshold = inact_threshold
+		* adxl345_range_factor_tbl[range_old]
+		/ adxl345_range_factor_tbl[range];
+	inact_threshold = min(255, max(1, inact_threshold));
+
+	ret = regmap_write(st->regmap, adxl345_act_thresh_reg[ADXL345_ACTIVITY],
+			   act_threshold);
+	if (ret)
+		return ret;
+
+	return regmap_write(st->regmap, adxl345_act_thresh_reg[ADXL345_INACTIVITY],
+			   inact_threshold);
 }
 
 static int adxl345_read_avail(struct iio_dev *indio_dev,
@@ -930,7 +1061,7 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
 				     enum iio_event_direction dir)
 {
 	struct adxl345_state *st = iio_priv(indio_dev);
-	bool int_en;
+	bool int_en, act_ac, inact_ac;
 	int ret;
 
 	switch (type) {
@@ -975,6 +1106,21 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
 		if (ret)
 			return ret;
 		return int_en;
+	case IIO_EV_TYPE_MAG_REFERENCED:
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			ret = adxl345_is_act_inact_ac(st, ADXL345_ACTIVITY, &act_ac);
+			if (ret)
+				return ret;
+			return act_ac;
+		case IIO_EV_DIR_FALLING:
+			ret = adxl345_is_act_inact_ac(st, ADXL345_INACTIVITY, &inact_ac);
+			if (ret)
+				return ret;
+			return inact_ac;
+		default:
+			return -EINVAL;
+		}
 	default:
 		return -EINVAL;
 	}
@@ -1011,6 +1157,16 @@ static int adxl345_write_event_config(struct iio_dev *indio_dev,
 		}
 	case IIO_EV_TYPE_MAG:
 		return adxl345_set_ff_en(st, state);
+	case IIO_EV_TYPE_MAG_REFERENCED:
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			return adxl345_set_act_inact_ac(st, ADXL345_ACTIVITY, state);
+		case IIO_EV_DIR_FALLING:
+			return adxl345_set_act_inact_ac(st, ADXL345_INACTIVITY, state);
+		default:
+			return -EINVAL;
+		}
+
 	default:
 		return -EINVAL;
 	}
-- 
2.39.5

[PATCH v6 11/11] docs: iio: add documentation for adxl345 driver

[Lothar Rubusch]

The documentation describes the ADXL345 driver, IIO interface,
interface usage and configuration.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 Documentation/iio/adxl345.rst | 429 ++++++++++++++++++++++++++++++++++
 1 file changed, 429 insertions(+)
 create mode 100644 Documentation/iio/adxl345.rst

diff --git a/Documentation/iio/adxl345.rst b/Documentation/iio/adxl345.rst
new file mode 100644
index 000000000000..975e4645a857
--- /dev/null
+++ b/Documentation/iio/adxl345.rst
@@ -0,0 +1,429 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+===============
+ADXL345 driver
+===============
+
+This driver supports Analog Device's ADXL345/375 on SPI/I2C bus.
+
+1. Supported Devices
+====================
+
+* `ADXL345 <https://www.analog.com/ADXL345>`_
+* `ADXL375 <https://www.analog.com/ADXL375>`_
+
+The ADXL345 is a generic purpose low power, 3-axis accelerometer with selectable
+measurement ranges. The ADXL345 supports the ±2 g, ±4 g, ±8 g, and ±16 g ranges.
+
+2. Device Attributes
+====================
+
+Each IIO device, has a device folder under ``/sys/bus/iio/devices/iio:deviceX``,
+where X is the IIO index of the device. Under these folders reside a set of
+device files, depending on the characteristics and features of the hardware
+device in questions. These files are consistently generalized and documented in
+the IIO ABI documentation.
+
+The following table shows the ADXL345 related device files, found in the
+specific device folder path ``/sys/bus/iio/devices/iio:deviceX``.
+
++-------------------------------------------+----------------------------------------------------------+
+| 3-Axis Accelerometer related device files | Description                                              |
++-------------------------------------------+----------------------------------------------------------+
+| in_accel_sampling_frequency               | Currently selected sample rate.                          |
++-------------------------------------------+----------------------------------------------------------+
+| in_accel_sampling_frequency_available     | Available sampling frequency configurations.             |
++-------------------------------------------+----------------------------------------------------------+
+| in_accel_scale                            | Scale/range for the accelerometer channels.              |
++-------------------------------------------+----------------------------------------------------------+
+| in_accel_scale_available                  | Available scale ranges for the accelerometer channel.    |
++-------------------------------------------+----------------------------------------------------------+
+| in_accel_x_calibbias                      | Calibration offset for the X-axis accelerometer channel. |
++-------------------------------------------+----------------------------------------------------------+
+| in_accel_x_raw                            | Raw X-axis accelerometer channel value.                  |
++-------------------------------------------+----------------------------------------------------------+
+| in_accel_y_calibbias                      | y-axis acceleration offset correction                    |
++-------------------------------------------+----------------------------------------------------------+
+| in_accel_y_raw                            | Raw Y-axis accelerometer channel value.                  |
++-------------------------------------------+----------------------------------------------------------+
+| in_accel_z_calibbias                      | Calibration offset for the Z-axis accelerometer channel. |
++-------------------------------------------+----------------------------------------------------------+
+| in_accel_z_raw                            | Raw Z-axis accelerometer channel value.                  |
++-------------------------------------------+----------------------------------------------------------+
+
+Channel Processed Values
+-------------------------
+
+A channel value can be read from its _raw attribute. The value returned is the
+raw value as reported by the devices. To get the processed value of the channel,
+apply the following formula:
+
+.. code-block:: bash
+
+        processed value = (_raw + _offset) * _scale
+
+Where _offset and _scale are device attributes. If no _offset attribute is
+present, simply assume its value is 0.
+
++-------------------------------------+---------------------------+
+| Channel type                        | Measurement unit          |
++-------------------------------------+---------------------------+
+| Acceleration on X, Y, and Z axis    | Meters per second squared |
++-------------------------------------+---------------------------+
+
+Sensor Events
+-------------
+
+Particular IIO events will be triggered by the corresponding interrupts. The
+sensor driver supports no or one active INT line, where the sensor has two
+possible INT IOs. Configure the used INT line in the devicetree. If no INT line
+is configured, the sensor falls back to FIFO bypass mode and no events are
+possible, only X, Y and Z axis measurements are possible.
+
+The following table shows the ADXL345 related device files, found in the
+specific device folder path ``/sys/bus/iio/devices/iio:deviceX/events``.
+
++---------------------------------------------+-----------------------------------------+
+| Event handle                                | Description                             |
++---------------------------------------------+-----------------------------------------+
+| in_accel_gesture_doubletap_en               | Enable double tap detection on all axis |
++---------------------------------------------+-----------------------------------------+
+| in_accel_gesture_doubletap_reset_timeout    | Double tap window in [us]               |
++---------------------------------------------+-----------------------------------------+
+| in_accel_gesture_doubletap_tap2_min_delay   | Double tap latent in [us]               |
++---------------------------------------------+-----------------------------------------+
+| in_accel_gesture_singletap_timeout          | Single tap duration in [us]             |
++---------------------------------------------+-----------------------------------------+
+| in_accel_gesture_singletap_value            | Single tap threshold value in 62.5/LSB  |
++---------------------------------------------+-----------------------------------------+
+| in_accel_mag_falling_en                     | Enable free fall detection              |
++---------------------------------------------+-----------------------------------------+
+| in_accel_mag_falling_period                 | Free fall time in [us]                  |
++---------------------------------------------+-----------------------------------------+
+| in_accel_mag_falling_value                  | Free fall threshold value in 62.5/LSB   |
++---------------------------------------------+-----------------------------------------+
+| in_accel_mag_referenced_falling_en          | Set 1 to AC-coupled inactivity, 0 for DC|
++---------------------------------------------+-----------------------------------------+
+| in_accel_mag_referenced_rising_en           | Set 1 to AC-coupled activity, 0 for DC  |
++---------------------------------------------+-----------------------------------------+
+| in_accel_x_thresh_falling_en                | Enable inactivity detection on X axis   |
++---------------------------------------------+-----------------------------------------+
+| in_accel_y_thresh_falling_en                | Enable inactivity detection on Y axis   |
++---------------------------------------------+-----------------------------------------+
+| in_accel_z_thresh_falling_en                | Enable inactivity detection on Z axis   |
++---------------------------------------------+-----------------------------------------+
+| in_accel_thresh_falling_period              | Inactivity time in seconds              |
++---------------------------------------------+-----------------------------------------+
+| in_accel_thresh_falling_value               | Inactivity threshold value in 62.5/LSB  |
++---------------------------------------------+-----------------------------------------+
+| in_accel_x_thresh_rising_en                 | Enable activity detection on X axis     |
++---------------------------------------------+-----------------------------------------+
+| in_accel_y_thresh_rising_en                 | Enable activity detection on Y axis     |
++---------------------------------------------+-----------------------------------------+
+| in_accel_z_thresh_rising_en                 | Enable activity detection on Z axis     |
++---------------------------------------------+-----------------------------------------+
+| in_accel_thresh_rising_value                | Activity threshold value in 62.5/LSB    |
++---------------------------------------------+-----------------------------------------+
+| in_accel_x_gesture_singletap_en             | Enable single tap detection on X axis   |
++---------------------------------------------+-----------------------------------------+
+| in_accel_y_gesture_singletap_en             | Enable single tap detection on Y axis   |
++---------------------------------------------+-----------------------------------------+
+| in_accel_z_gesture_singletap_en             | Enable single tap detection on Z axis   |
++---------------------------------------------+-----------------------------------------+
+
+Find a detailed description of a particular functionality in the sensor
+datasheet.
+
+Setting the **ODR** explicitly will result in estimated adjusted default values
+for the inactivity time detection, where higher frequencies shall default to
+longer wait periods, and vice versa. It is also possible to explicetly
+configure inactivity wait times, if the defaulting approach does not match
+application requirements. Setting 0 here, will fall back to default setting.
+
+The **g range** configuration also tries to estimate activity and inactivity
+thresholds when switching to another g range. The default range will be
+factorized by the relation of old range divided by new range. The value never
+becomes 0 and will be at least 1 and at most 255 i.e. 62.5g/LSB according to
+the datasheet. Nevertheless activity and inactivity thresholds can be
+overwritten by explicit values.
+
+When **activity** and **inactivity** events are enabled, the driver automatically
+will implement its hysteresis solution by setting link bit and autosleep bit.
+The link bit serially links the activity and inactivity functions. On the other
+side, the autosleep function switches the sensor to sleep mode if the
+inactivity function is enabled. This will reduce current consumption to the
+sub-12.5Hz rate. Inactivity time can be configured between 1s and 255s. When 0
+is configured as inactivity time, the driver will define a reasonable value
+depending on a heuristic approach to optimize power consumption.
+
+In **dc-coupled** operation, the current acceleration magnitude is compared
+directly with THRESH_ACT and THRESH_INACT registers to determine whether
+activity or inactivity was detected. In ac-coupled operation for activity
+detection, the acceleration value at the start of activity detection is taken
+as a reference value. New samples are then compared to this reference value.
+Note, ac-coupling and dc-coupling are individually set for activity and/or
+inactivity detection. Activity and inactivity detection are dependent on the
+direction, i.e. the x/y/z axis where this driver generally enables all
+directions. Also, the direction settings are particular to activity and
+inactivity detection, respectively.
+
+**Single tap** detection can be configured according to the datasheet by specifying
+threshold and duration. If only the single tap is in use, the single tap
+interrupt is triggered when the acceleration goes above threshold (i.e. DUR
+start) and below the threshold, as long as duration is not exceeded. If single
+tap and double tap are in use, the single tap is triggered when the doulbe tap
+event has been either validated or invalidated.
+
+For **double tap** configure additionally window and latency in [us]. Latency
+starts counting when the single tap goes below threshold and is a waiting
+period, any spikes here are ignored for double tap detection. After latency,
+the window starts. Any rise above threshold, with a consequent fall below
+threshold within window time, rises a double tap signal when going below
+threshold.
+
+Double tap event detection is best described in the datasheet. After a
+single tap event was detected, a double tap event can be detected. Therefore the
+signal must match several criteria, and detection can also considered invalid
+for three reasons:
+* If the **suppress bit** is set and when still in the tap latency period, any
+measurement of acceleration spike above the tap threshold invalidates double tap
+detection immediately, i.e. during latency must not occur spikes for double tap
+detection when the suppress bit is set.
+* A double tap event is considered invalid, if acceleration lies above the
+threshold at the start of the window time for double tap.
+* Additionally, double tap detection can be considered invalid, if an
+acceleration exceeds the time limit for taps, set by duration register.
+Note, since for double tap the same duration counts, i.e. when rising above
+threshold, a consequent falling below threshold has to be within duration time.
+Also note, the suppress bit is generally set when double tap is enabled.
+
+A **free fall** event will be detected if the signal goes below the configured
+threshold, for the configured time [us].
+
+Note, that activity/inactivy, as also freefall is recommended for 12.5 Hz ODR
+up to 400 Hz.
+
+Usage Examples
+--------------
+
+Show device name:
+
+.. code-block:: bash
+
+        root:/sys/bus/iio/devices/iio:device0> cat name
+        adxl345
+
+Show accelerometer channels value:
+
+.. code-block:: bash
+
+        root:/sys/bus/iio/devices/iio:device0> cat in_accel_x_raw
+        -1
+        root:/sys/bus/iio/devices/iio:device0> cat in_accel_y_raw
+        2
+        root:/sys/bus/iio/devices/iio:device0> cat in_accel_z_raw
+        -253
+
+Set calibration offset for accelerometer channels:
+
+.. code-block:: bash
+
+        root:/sys/bus/iio/devices/iio:device0> cat in_accel_x_calibbias
+        0
+
+        root:/sys/bus/iio/devices/iio:device0> echo 50 > in_accel_x_calibbias
+        root:/sys/bus/iio/devices/iio:device0> cat in_accel_x_calibbias
+        50
+
+Given the 13-bit full resolution, the available ranges are calculated by the
+following forumla:
+
+.. code-block:: bash
+
+        (g * 2 * 9.80665) / (2^(resolution) - 1) * 100; for g := 2|4|8|16
+
+Scale range configuration:
+
+.. code-block:: bash
+
+        root:/sys/bus/iio/devices/iio:device0> cat ./in_accel_scale
+        0.478899
+        root:/sys/bus/iio/devices/iio:device0> cat ./in_accel_scale_available
+        0.478899 0.957798 1.915595 3.831190
+
+        root:/sys/bus/iio/devices/iio:device0> echo 1.915595 > ./in_accel_scale
+        root:/sys/bus/iio/devices/iio:device0> cat ./in_accel_scale
+        1.915595
+
+Set output data rate (ODR):
+
+.. code-block:: bash
+
+        root:/sys/bus/iio/devices/iio:device0> cat ./in_accel_sampling_frequency
+        200.000000
+
+        root:/sys/bus/iio/devices/iio:device0> cat ./in_accel_sampling_frequency_available
+        0.097000 0.195000 0.390000 0.781000 1.562000 3.125000 6.250000 12.500000 25.000000 50.000000 100.000000 200.000000 400.000000 800.000000 1600.000000 3200.000000
+
+        root:/sys/bus/iio/devices/iio:device0> echo 1.562000 > ./in_accel_sampling_frequency
+        root:/sys/bus/iio/devices/iio:device0> cat ./in_accel_sampling_frequency
+        1.562000
+
+Configure one or several events:
+
+.. code-block:: bash
+
+        root:> cd /sys/bus/iio/devices/iio:device0
+
+        root:/sys/bus/iio/devices/iio:device0> echo 1 > ./buffer0/in_accel_x_en
+        root:/sys/bus/iio/devices/iio:device0> echo 1 > ./buffer0/in_accel_y_en
+        root:/sys/bus/iio/devices/iio:device0> echo 1 > ./buffer0/in_accel_z_en
+
+        root:/sys/bus/iio/devices/iio:device0> echo 1 > ./scan_elements/in_accel_x_en
+        root:/sys/bus/iio/devices/iio:device0> echo 1 > ./scan_elements/in_accel_y_en
+        root:/sys/bus/iio/devices/iio:device0> echo 1 > ./scan_elements/in_accel_z_en
+
+        root:/sys/bus/iio/devices/iio:device0> echo 14   > ./in_accel_x_calibbias
+        root:/sys/bus/iio/devices/iio:device0> echo 2    > ./in_accel_y_calibbias
+        root:/sys/bus/iio/devices/iio:device0> echo -250 > ./in_accel_z_calibbias
+
+        root:/sys/bus/iio/devices/iio:device0> echo 24 > ./buffer0/length
+
+        ## activity, threshold [62.5/LSB]
+        root:/sys/bus/iio/devices/iio:device0> echo 6 > ./events/in_accel_thresh_rising_value
+
+        ## inactivity, threshold, [62.5/LSB]
+        root:/sys/bus/iio/devices/iio:device0> echo 4 > ./events/in_accel_thresh_falling_value
+
+        ## inactivity, time [s]
+        root:/sys/bus/iio/devices/iio:device0> echo 3 > ./events/in_accel_thresh_falling_period
+
+        ## singletap, threshold
+        root:/sys/bus/iio/devices/iio:device0> echo 35 > ./events/in_accel_gesture_singletap_value
+
+        ## singletap, duration [us]
+        root:/sys/bus/iio/devices/iio:device0> echo 0.001875  > ./events/in_accel_gesture_singletap_timeout
+
+        ## doubletap, window [us]
+        root:/sys/bus/iio/devices/iio:device0> echo 0.025 > ./events/in_accel_gesture_doubletap_reset_timeout
+
+        ## doubletap, latent [us]
+        root:/sys/bus/iio/devices/iio:device0> echo 0.025 > ./events/in_accel_gesture_doubletap_tap2_min_delay
+
+        ## freefall, threshold [62.5/LSB]
+        root:/sys/bus/iio/devices/iio:device0> echo 8 > ./events/in_accel_mag_falling_value
+
+        ## freefall, time [ms]
+        root:/sys/bus/iio/devices/iio:device0> echo 1.25 > ./events/in_accel_mag_falling_period
+
+        ## activity, enable
+        root:/sys/bus/iio/devices/iio:device0> echo 1 > ./events/in_accel_thresh_rising_en
+
+        ## inactivity, enable
+        root:/sys/bus/iio/devices/iio:device0> echo 1 > ./events/in_accel_thresh_falling_en
+
+        ## freefall, enable
+        root:/sys/bus/iio/devices/iio:device0> echo 1 > ./events/in_accel_mag_falling_en
+
+        ## singletap, enable
+        root:/sys/bus/iio/devices/iio:device0> echo 1 > ./events/in_accel_x_gesture_singletap_en
+        root:/sys/bus/iio/devices/iio:device0> echo 1 > ./events/in_accel_y_gesture_singletap_en
+        root:/sys/bus/iio/devices/iio:device0> echo 1 > ./events/in_accel_z_gesture_singletap_en
+
+        ## doubletap, enable
+        root:/sys/bus/iio/devices/iio:device0> echo 1 > ./events/in_accel_gesture_doubletap_en
+
+Verify incoming events:
+
+.. code-block:: bash
+
+        root:# iio_event_monitor adxl345
+        Found IIO device with name adxl345 with device number 0
+        Event: time: 1739063415957073383, type: accel(z), channel: 0, evtype: thresh, direction: rising
+        Event: time: 1739063415963770218, type: accel(z), channel: 0, evtype: thresh, direction: rising
+        Event: time: 1739063416002563061, type: accel(z), channel: 0, evtype: gesture, direction: singletap
+        Event: time: 1739063426271128739, type: accel(x|y|z), channel: 0, evtype: thresh, direction: falling
+        Event: time: 1739063436539080713, type: accel(x|y|z), channel: 0, evtype: thresh, direction: falling
+        Event: time: 1739063438357970381, type: accel(z), channel: 0, evtype: thresh, direction: rising
+        Event: time: 1739063446726161586, type: accel(z), channel: 0, evtype: thresh, direction: rising
+        Event: time: 1739063446727892670, type: accel(z), channel: 0, evtype: thresh, direction: rising
+        Event: time: 1739063446743019768, type: accel(z), channel: 0, evtype: thresh, direction: rising
+        Event: time: 1739063446744650696, type: accel(z), channel: 0, evtype: thresh, direction: rising
+        Event: time: 1739063446763559386, type: accel(z), channel: 0, evtype: gesture, direction: singletap
+        Event: time: 1739063448818126480, type: accel(x|y|z), channel: 0, evtype: thresh, direction: falling
+        ...
+
+Activity and inactivity belong together and indicate state changes as follows
+
+.. code-block:: bash
+
+        root:# iio_event_monitor adxl345
+        Found IIO device with name adxl345 with device number 0
+        Event: time: 1744648001133946293, type: accel(x), channel: 0, evtype: thresh, direction: rising
+          <after inactivity time elapsed>
+        Event: time: 1744648057724775499, type: accel(x|y|z), channel: 0, evtype: thresh, direction: falling
+        ...
+
+3. Device Buffers
+=================
+
+This driver supports IIO buffers.
+
+All devices support retrieving the raw acceleration and temperature measurements
+using buffers.
+
+Usage examples
+--------------
+
+Select channels for buffer read:
+
+.. code-block:: bash
+
+        root:/sys/bus/iio/devices/iio:device0> echo 1 > scan_elements/in_accel_x_en
+        root:/sys/bus/iio/devices/iio:device0> echo 1 > scan_elements/in_accel_y_en
+        root:/sys/bus/iio/devices/iio:device0> echo 1 > scan_elements/in_accel_z_en
+
+Set the number of samples to be stored in the buffer:
+
+.. code-block:: bash
+
+        root:/sys/bus/iio/devices/iio:device0> echo 10 > buffer/length
+
+Enable buffer readings:
+
+.. code-block:: bash
+
+        root:/sys/bus/iio/devices/iio:device0> echo 1 > buffer/enable
+
+Obtain buffered data:
+
+.. code-block:: bash
+
+        root:> iio_readdev -b 16 -s 1024 adxl345 | hexdump -d
+        WARNING: High-speed mode not enabled
+        0000000   00003   00012   00013   00005   00010   00011   00005   00011
+        0000010   00013   00004   00012   00011   00003   00012   00014   00007
+        0000020   00011   00013   00004   00013   00014   00003   00012   00013
+        0000030   00004   00012   00013   00005   00011   00011   00005   00012
+        0000040   00014   00005   00012   00014   00004   00010   00012   00004
+        0000050   00013   00011   00003   00011   00012   00005   00011   00013
+        0000060   00003   00012   00012   00003   00012   00012   00004   00012
+        0000070   00012   00003   00013   00013   00003   00013   00012   00005
+        0000080   00012   00013   00003   00011   00012   00005   00012   00013
+        0000090   00003   00013   00011   00005   00013   00014   00003   00012
+        00000a0   00012   00003   00012   00013   00004   00012   00015   00004
+        00000b0   00014   00011   00003   00014   00013   00004   00012   00011
+        00000c0   00004   00012   00013   00004   00014   00011   00004   00013
+        00000d0   00012   00002   00014   00012   00005   00012   00013   00005
+        00000e0   00013   00013   00003   00013   00013   00005   00012   00013
+        00000f0   00004   00014   00015   00005   00012   00011   00005   00012
+        ...
+
+See ``Documentation/iio/iio_devbuf.rst`` for more information about how buffered
+data is structured.
+
+4. IIO Interfacing Tools
+========================
+
+See ``Documentation/iio/iio_tools.rst`` for the description of the available IIO
+interfacing tools.
-- 
2.39.5

Re: [PATCH v6 04/11] iio: accel: adxl345: set the tap suppress bit permanently

[Jonathan Cameron]

On Mon, 14 Apr 2025 18:42:38 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Set the suppress bit feature to the double tap detection, whenever
> double tap is enabled. This impedes the suppress bit dangling in any
> state, and thus varying in sensitivity for double tap detection.
> 
> Any tap event is defined by a rising signal edge above threshold, i.e.
> duration time starts counting; and the falling edge under threshold
> within duration time, i.e. then the tap event is issued. This means
> duration is used individually for each tap event.
> 
> For double tap detection after a single tap, a latency time needs to be
> specified. Usually tap events, i.e. spikes above and returning below
> threshold will be ignored within latency. After latency, the window
> time starts counting for a second tap detection which has to happen
> within a duration time.
> 
> If the suppress bit is not set, spikes within latency time are ignored.
> Setting the suppress bit will invalidate the double tap function. The
> sensor will thus be able to save the window time for double tap
> detection, and follow a more strict definition of what signal qualifies
> for a double tap.
> 
> In a summary having the suppress bit set, fewer signal spikes will be
> considered as double taps. This is an optional add on to double tap,
> thus a separate patch.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
Applied 1-4.

Thanks,
Jonathan

Re: [PATCH v6 05/11] iio: accel: adxl345: add freefall feature

[Jonathan Cameron]

On Mon, 14 Apr 2025 18:42:39 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add the freefall detection of the sensor together with a threshold and
> time parameter. A freefall event is detected if the measuring signal
> falls below the threshold.
> 
> Introduce a freefall threshold stored in regmap cache, and a freefall
> time, having the scaled time value stored as a member variable in the
> state instance.
> 
Reading this I wondered whether we had the event code consistent for
freefall detectors... Or indeed inactivity ones (which are kind of similarish)

:( We don't it seems.  The issue is that
freefall is actually that all channels are simultaneously under the the magnitude
threshold, not one of them.  So it should I think be
X_AND_Y_AND_Z not X_OR_Y_OR_Z

This is as opposed to activity detectors which tend to be any axis shows
activity and X_OR_Y_OR_Z applies.

Anyhow upshot is I think I lead you astray on this and we should make this
one IIO_MOD_X_AND_Y_AND_Z 

A few other things inline.

Unfortunately we don't deal with these events that often so I forget
what we did before :(

> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl345_core.c | 125 +++++++++++++++++++++++++++++++
>  1 file changed, 125 insertions(+)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index c464c87033fb..ae02826e552b 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -75,6 +75,7 @@ struct adxl345_state {
>  	u32 tap_duration_us;
>  	u32 tap_latent_us;
>  	u32 tap_window_us;
> +	u32 ff_time_ms;
>  
>  	__le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE + 1] __aligned(IIO_DMA_MINALIGN);
>  };
> @@ -96,6 +97,14 @@ static struct iio_event_spec adxl345_events[] = {
>  			BIT(IIO_EV_INFO_RESET_TIMEOUT) |
>  			BIT(IIO_EV_INFO_TAP2_MIN_DELAY),
>  	},
> +	{
> +		/* free fall */
> +		.type = IIO_EV_TYPE_MAG,
> +		.dir = IIO_EV_DIR_FALLING,
> +		.mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE) |
> +			BIT(IIO_EV_INFO_VALUE) |
> +			BIT(IIO_EV_INFO_PERIOD),
> +	},
This is creating separate per axis enables, values and period. Does that make
sense?  If not you need to spin a kind of virtual channel (with mod X_AND_Y_AND_Z)
and add the events to it.

See how the sca3000 does it for example.
>  };
>  
>  #define ADXL345_CHANNEL(index, reg, axis) {					\
> @@ -383,6 +392,63 @@ static int adxl345_set_tap_latent(struct adxl345_state *st, u32 val_int,
>  	return _adxl345_set_tap_time(st, ADXL345_TAP_TIME_LATENT, val_fract_us);
>  }
>  
> +/* freefall */
> +
> +static int adxl345_is_ff_en(struct adxl345_state *st, bool *en)
> +{
> +	int ret;
> +	unsigned int regval;
> +
> +	ret = regmap_read(st->regmap, ADXL345_REG_INT_ENABLE, &regval);
> +	if (ret)
> +		return ret;
> +
> +	*en = FIELD_GET(ADXL345_INT_FREE_FALL, regval) > 0;
> +
> +	return 0;
> +}
> +
> +static int adxl345_set_ff_en(struct adxl345_state *st, bool cmd_en)
> +{
> +	unsigned int regval, ff_threshold;
> +	bool en;
> +	int ret;
> +
> +	ret = regmap_read(st->regmap, ADXL345_REG_THRESH_FF, &ff_threshold);
> +	if (ret)
> +		return ret;
> +
> +	en = cmd_en && ff_threshold > 0 && st->ff_time_ms > 0;
> +
> +	regval = en ? ADXL345_INT_FREE_FALL : 0x00;
> +
> +	return regmap_update_bits(st->regmap, ADXL345_REG_INT_ENABLE,
> +				  ADXL345_INT_FREE_FALL, regval);
> +}
> +
> +static int adxl345_set_ff_time(struct adxl345_state *st, u32 val_int,
> +			       u32 val_fract_us)
> +{
> +	unsigned int regval;
> +	int val_ms;
> +
> +	/*
> +	 * max value is 255 * 5000 us = 1.275000 seconds
> +	 *
> +	 * Note: the scaling is similar to the scaling in the ADXL380
> +	 */
> +	if (1000000 * val_int + val_fract_us > 1275000)
> +		return -EINVAL;
> +
> +	val_ms = val_int * 1000 + DIV_ROUND_UP(val_fract_us, 1000);
> +	st->ff_time_ms = val_ms;
> +
> +	regval = DIV_ROUND_CLOSEST(val_ms, 5);
> +
> +	/* Values between 100ms and 350ms (0x14 to 0x46) are recommended. */
> +	return regmap_write(st->regmap, ADXL345_REG_TIME_FF, min(regval, 0xff));
> +}
> +
>  static int adxl345_read_raw(struct iio_dev *indio_dev,
>  			    struct iio_chan_spec const *chan,
>  			    int *val, int *val2, long mask)
> @@ -495,6 +561,11 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
>  		default:
>  			return -EINVAL;
>  		}
> +	case IIO_EV_TYPE_MAG:
> +		ret = adxl345_is_ff_en(st, &int_en);
> +		if (ret)
> +			return ret;
> +		return int_en;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -518,6 +589,8 @@ static int adxl345_write_event_config(struct iio_dev *indio_dev,
>  		default:
>  			return -EINVAL;
>  		}
> +	case IIO_EV_TYPE_MAG:
> +		return adxl345_set_ff_en(st, state);
>  	default:
>  		return -EINVAL;
>  	}
> @@ -532,6 +605,7 @@ static int adxl345_read_event_value(struct iio_dev *indio_dev,
>  {
>  	struct adxl345_state *st = iio_priv(indio_dev);
>  	unsigned int tap_threshold;
> +	unsigned int ff_threshold;
>  	int ret;
>  
>  	switch (type) {
> @@ -565,6 +639,22 @@ static int adxl345_read_event_value(struct iio_dev *indio_dev,
>  		default:
>  			return -EINVAL;
>  		}
> +	case IIO_EV_TYPE_MAG:
> +		switch (info) {
> +		case IIO_EV_INFO_VALUE:
> +			ret = regmap_read(st->regmap, ADXL345_REG_THRESH_FF,
> +					  &ff_threshold);
> +			if (ret)
> +				return ret;
> +			*val = ff_threshold;
> +			return IIO_VAL_INT;
> +		case IIO_EV_INFO_PERIOD:
> +			*val = st->ff_time_ms;
> +			*val2 = 1000;
> +			return IIO_VAL_FRACTIONAL;
> +		default:
> +			return -EINVAL;
> +		}
>  	default:
>  		return -EINVAL;
>  	}
> @@ -612,6 +702,22 @@ static int adxl345_write_event_value(struct iio_dev *indio_dev,
>  			return -EINVAL;
>  		}
>  		break;
> +	case IIO_EV_TYPE_MAG:
> +		switch (info) {
> +		case IIO_EV_INFO_VALUE:
> +			ret = regmap_write(st->regmap, ADXL345_REG_THRESH_FF, val);
> +			if (ret)
> +				return ret;
> +			break;
> +		case IIO_EV_INFO_PERIOD:
> +			ret = adxl345_set_ff_time(st, val, val2);
> +			if (ret)
> +				return ret;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -865,6 +971,17 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
>  			return ret;
>  	}
>  
> +	if (FIELD_GET(ADXL345_INT_FREE_FALL, int_stat)) {
> +		ret = iio_push_event(indio_dev,
> +				     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> +							IIO_MOD_X_OR_Y_OR_Z,

This is the event that got me thinking about whether we were doing this right..

> +							IIO_EV_TYPE_MAG,
> +							IIO_EV_DIR_FALLING),
> +				     ts);
> +		if (ret)
> +			return ret;
> +	}
> +

Re: [PATCH v6 09/11] iio: accel: adxl345: add inactivity feature

[Jonathan Cameron]

On Mon, 14 Apr 2025 18:42:43 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add the inactivity feature of the sensor. When activity and inactivity
> are enabled, a link bit will be set linking activity and inactivity
> handling. Additionally, the auto-sleep mode will be enabled. Due to the
> link bit the sensor is going to auto-sleep when inactivity was
> detected.
> 
> Inactivity detection needs a threshold to be configured, and a time
> after which it will go into inactivity state if measurements under
> threshold.
> 
> When a ODR is configured this time for inactivity is adjusted with a
> corresponding reasonable default value, in order to have higher
> frequencies and lower inactivity times, and lower sample frequency but
> give more time until inactivity. Both with reasonable upper and lower
> boundaries, since many of the sensor's features (e.g. auto-sleep) will
> need to operate beween 12.5 Hz and 400 Hz. This is a default setting
> when actively changing sample frequency, explicitly setting the time
> until inactivity will overwrite the default.
> 
> Similarly, setting the g-range will provide a default value for the
> activity and inactivity thresholds. Both are implicit defaults, but
> equally can be overwritten to be explicitly configured.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
Hi Lothar,

Patches 6-8 look good to me.

This runs into a similar issue to the freefall one. I haven't dug into
the datasheet but does it report on one channel going inactive, or
all being inactive at the same time?  I checked and it is the all
case so we should be both on a pseudo channel to describe it right
and reporting IIO_MOD_X_AND_Y_AND_Z not the OR form.

Sorry again that I'm only realising this on v6 :(

Difference is for Activity the definition is:
"The activity bit is set when acceleration greater than the value
stored in the THRESH_ACT register (Address 0x24) is experienced
on _any_ participating axis, set by the ACT_INACT_CTL register
(Address 0x27)."
vs Inactivity:
"The inactivity bit is set when acceleration of less than the value
stored in the THRESH_INACT register (Address 0x25) is experienced
for more time than is specified in the TIME_INACT
register (Address 0x26) on _all_ participating axes, as set by the
ACT_INACT_CTL register (Address 0x27). "

So all vs any.

> +
> +/**
> + * adxl345_set_inact_time_s - Configure inactivity time explicitly or by ODR.
> + * @st: The sensor state instance.
> + * @val_s: A desired time value, between 0 and 255.
> + *
> + * Inactivity time can be configured between 1 and 255 sec. If a val_s of 0
> + * is configured by a user, then a default inactivity time will be computed.
> + *
> + * In such case, it should take power consumption into consideration. Thus it
> + * shall be shorter for higher frequencies and longer for lower frequencies.
> + * Hence, frequencies above 255 Hz shall default to 10 s and frequencies below
> + * 10 Hz shall result in 255 s to detect inactivity.
> + *
> + * The approach simply subtracts the pre-decimal figure of the configured
> + * sample frequency from 255 s to compute inactivity time [s]. Sub-Hz are thus
> + * ignored in this estimation. The recommended ODRs for various features
> + * (activity/inactivity, sleep modes, free fall, etc.) lie between 12.5 Hz and
> + * 400 Hz, thus higher or lower frequencies will result in the boundary
> + * defaults or need to be explicitly specified via val_s.
> + *
> + * Return: 0 or error value.
> + */
> +static int adxl345_set_inact_time_s(struct adxl345_state *st, u32 val_s)
> +{
> +	unsigned int max_boundary = 255;
> +	unsigned int min_boundary = 10;
> +	unsigned int val = min(val_s, max_boundary);
> +	enum adxl345_odr odr;
> +	unsigned int regval;
> +	int ret;
> +
> +	if (val == 0) {
> +		ret = regmap_read(st->regmap, ADXL345_REG_BW_RATE, &regval);
> +		if (ret)
> +			return ret;
> +		odr = FIELD_GET(ADXL345_BW_RATE_MSK, regval);
> +
> +		val = (adxl345_odr_tbl[odr][0] > max_boundary)
> +			? min_boundary : max_boundary -	adxl345_odr_tbl[odr][0];
> +	}
> +
> +	return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
>  }
>  
>  /* tap */
> @@ -837,6 +943,13 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
>  			if (ret)
>  				return ret;
>  			return int_en;
> +		case IIO_EV_DIR_FALLING:
> +			ret = adxl345_is_act_inact_en(st, chan->channel2,

Does it makes sense to allow inactivity detection on a subset of channels but then
report it as XYZ?  I guess it didn't matter when it was and OR, but if we
change to AND as suggested that is going to be misleading.

we might have to allow separate enables but report an event as the combination
of channels that are enabled X_AND_Y, X_AND_Z etc  I guess we can improve activity
channel case as well by doing that with the X_OR_Y etc



> +						      ADXL345_INACTIVITY,
> +						      &int_en);
> +			if (ret)
> +				return ret;
> +			return int_en;
>  		default:
>  			return -EINVAL;
>  		}
> @@ -881,6 +994,9 @@ static int adxl345_write_event_config(struct iio_dev *indio_dev,
>  		case IIO_EV_DIR_RISING:
>  			return adxl345_set_act_inact_en(st, chan->channel2,
>  							ADXL345_ACTIVITY, state);
> +		case IIO_EV_DIR_FALLING:
> +			return adxl345_set_act_inact_en(st, chan->channel2,
> +							ADXL345_INACTIVITY, state);
>  		default:
>  			return -EINVAL;
>  		}

> @@ -1314,6 +1458,17 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
>  			return ret;
>  	}
>  
> +	if (FIELD_GET(ADXL345_INT_INACTIVITY, int_stat)) {
> +		ret = iio_push_event(indio_dev,
> +				     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> +							IIO_MOD_X_OR_Y_OR_Z,

So this is our open question. Similar to the free fall case. Do we have the boolean
logic right way around?

> +							IIO_EV_TYPE_THRESH,
> +							IIO_EV_DIR_FALLING),
> +				     ts);
> +		if (ret)
> +			return ret;
> +	}
> +

Re: [PATCH v6 11/11] docs: iio: add documentation for adxl345 driver

[Jonathan Cameron]

On Mon, 14 Apr 2025 18:42:45 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> The documentation describes the ADXL345 driver, IIO interface,
> interface usage and configuration.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>

This probably wants updating to reflect the subtle _all_ vs _any_
definitions for the various events.

> +When **activity** and **inactivity** events are enabled, the driver automatically
> +will implement its hysteresis solution by setting link bit and autosleep bit.
> +The link bit serially links the activity and inactivity functions. On the other
> +side, the autosleep function switches the sensor to sleep mode if the
> +inactivity function is enabled. This will reduce current consumption to the
> +sub-12.5Hz rate. Inactivity time can be configured between 1s and 255s. When 0
> +is configured as inactivity time, the driver will define a reasonable value
> +depending on a heuristic approach to optimize power consumption.

> +A **free fall** event will be detected if the signal goes below the configured
> +threshold, for the configured time [us].
> +
Also the examples later in the document will change with the introduction of
a pseudo channel for freefall (where there is only one set of parameters for
all channels). For activity / inactivity the per channel enables mean we keep
those on the individual channels.  It's always a bit odd to report an event
for a compound channel that doesn't have it to enable, but we've never found
a better way to handle that.

Jonathan

Re: [PATCH v6 05/11] iio: accel: adxl345: add freefall feature

[Lothar Rubusch]

Happy Easter!

On Fri, Apr 18, 2025 at 8:23 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon, 14 Apr 2025 18:42:39 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Add the freefall detection of the sensor together with a threshold and
> > time parameter. A freefall event is detected if the measuring signal
> > falls below the threshold.
> >
> > Introduce a freefall threshold stored in regmap cache, and a freefall
> > time, having the scaled time value stored as a member variable in the
> > state instance.
> >
> Reading this I wondered whether we had the event code consistent for
> freefall detectors... Or indeed inactivity ones (which are kind of similarish)
>
> :( We don't it seems.  The issue is that
> freefall is actually that all channels are simultaneously under the the magnitude
> threshold, not one of them.  So it should I think be
> X_AND_Y_AND_Z not X_OR_Y_OR_Z
>

I change to X_AND_Y_AND_Z.

> This is as opposed to activity detectors which tend to be any axis shows
> activity and X_OR_Y_OR_Z applies.
>
> Anyhow upshot is I think I lead you astray on this and we should make this
> one IIO_MOD_X_AND_Y_AND_Z
>
> A few other things inline.
>
> Unfortunately we don't deal with these events that often so I forget
> what we did before :(
>
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > ---
> >  drivers/iio/accel/adxl345_core.c | 125 +++++++++++++++++++++++++++++++
> >  1 file changed, 125 insertions(+)
> >
> > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > index c464c87033fb..ae02826e552b 100644
> > --- a/drivers/iio/accel/adxl345_core.c
> > +++ b/drivers/iio/accel/adxl345_core.c
> > @@ -75,6 +75,7 @@ struct adxl345_state {
> >       u32 tap_duration_us;
> >       u32 tap_latent_us;
> >       u32 tap_window_us;
> > +     u32 ff_time_ms;
> >
> >       __le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE + 1] __aligned(IIO_DMA_MINALIGN);
> >  };
> > @@ -96,6 +97,14 @@ static struct iio_event_spec adxl345_events[] = {
> >                       BIT(IIO_EV_INFO_RESET_TIMEOUT) |
> >                       BIT(IIO_EV_INFO_TAP2_MIN_DELAY),
> >       },
> > +     {
> > +             /* free fall */
> > +             .type = IIO_EV_TYPE_MAG,
> > +             .dir = IIO_EV_DIR_FALLING,
> > +             .mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE) |
> > +                     BIT(IIO_EV_INFO_VALUE) |
> > +                     BIT(IIO_EV_INFO_PERIOD),
> > +     },
> This is creating separate per axis enables, values and period. Does that make
> sense?  If not you need to spin a kind of virtual channel (with mod X_AND_Y_AND_Z)
> and add the events to it.
>
> See how the sca3000 does it for example.

Hum, I'm not sure if I understand you correctly. In my driver, I'm
using .mask_shared_by_type, and I verified there appears only one
enable, one value and one period handle.
# ls -l /sys/bus/iio/devices/iio:device0/events/
total 0
...
-rw-r--r-- 1 root root 4096 Apr 20 21:59 in_accel_mag_falling_en
-rw-r--r-- 1 root root 4096 Apr 20 21:59 in_accel_mag_falling_period
-rw-r--r-- 1 root root 4096 Apr 20 21:59 in_accel_mag_falling_value
...

In the sources of sca3000.c I saw this setup with .mask_separate. So,
there I'd expect to see separate enables per axis, or am I wrong? In
the case of the ADXL345, there should only be one freefall enable (in
my driver) and not per axis. So, isn't this what is currently there?

So far I only adjust the or'ing to and'ing the axis for freefall.

> >  };
> >
> >  #define ADXL345_CHANNEL(index, reg, axis) {                                  \
> > @@ -383,6 +392,63 @@ static int adxl345_set_tap_latent(struct adxl345_state *st, u32 val_int,
> >       return _adxl345_set_tap_time(st, ADXL345_TAP_TIME_LATENT, val_fract_us);
> >  }
> >
> > +/* freefall */
> > +
> > +static int adxl345_is_ff_en(struct adxl345_state *st, bool *en)
> > +{
> > +     int ret;
> > +     unsigned int regval;
> > +
> > +     ret = regmap_read(st->regmap, ADXL345_REG_INT_ENABLE, &regval);
> > +     if (ret)
> > +             return ret;
> > +
> > +     *en = FIELD_GET(ADXL345_INT_FREE_FALL, regval) > 0;
> > +
> > +     return 0;
> > +}
> > +
> > +static int adxl345_set_ff_en(struct adxl345_state *st, bool cmd_en)
> > +{
> > +     unsigned int regval, ff_threshold;
> > +     bool en;
> > +     int ret;
> > +
> > +     ret = regmap_read(st->regmap, ADXL345_REG_THRESH_FF, &ff_threshold);
> > +     if (ret)
> > +             return ret;
> > +
> > +     en = cmd_en && ff_threshold > 0 && st->ff_time_ms > 0;
> > +
> > +     regval = en ? ADXL345_INT_FREE_FALL : 0x00;
> > +
> > +     return regmap_update_bits(st->regmap, ADXL345_REG_INT_ENABLE,
> > +                               ADXL345_INT_FREE_FALL, regval);
> > +}
> > +
> > +static int adxl345_set_ff_time(struct adxl345_state *st, u32 val_int,
> > +                            u32 val_fract_us)
> > +{
> > +     unsigned int regval;
> > +     int val_ms;
> > +
> > +     /*
> > +      * max value is 255 * 5000 us = 1.275000 seconds
> > +      *
> > +      * Note: the scaling is similar to the scaling in the ADXL380
> > +      */
> > +     if (1000000 * val_int + val_fract_us > 1275000)
> > +             return -EINVAL;
> > +
> > +     val_ms = val_int * 1000 + DIV_ROUND_UP(val_fract_us, 1000);
> > +     st->ff_time_ms = val_ms;
> > +
> > +     regval = DIV_ROUND_CLOSEST(val_ms, 5);
> > +
> > +     /* Values between 100ms and 350ms (0x14 to 0x46) are recommended. */
> > +     return regmap_write(st->regmap, ADXL345_REG_TIME_FF, min(regval, 0xff));
> > +}
> > +
> >  static int adxl345_read_raw(struct iio_dev *indio_dev,
> >                           struct iio_chan_spec const *chan,
> >                           int *val, int *val2, long mask)
> > @@ -495,6 +561,11 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
> >               default:
> >                       return -EINVAL;
> >               }
> > +     case IIO_EV_TYPE_MAG:
> > +             ret = adxl345_is_ff_en(st, &int_en);
> > +             if (ret)
> > +                     return ret;
> > +             return int_en;
> >       default:
> >               return -EINVAL;
> >       }
> > @@ -518,6 +589,8 @@ static int adxl345_write_event_config(struct iio_dev *indio_dev,
> >               default:
> >                       return -EINVAL;
> >               }
> > +     case IIO_EV_TYPE_MAG:
> > +             return adxl345_set_ff_en(st, state);
> >       default:
> >               return -EINVAL;
> >       }
> > @@ -532,6 +605,7 @@ static int adxl345_read_event_value(struct iio_dev *indio_dev,
> >  {
> >       struct adxl345_state *st = iio_priv(indio_dev);
> >       unsigned int tap_threshold;
> > +     unsigned int ff_threshold;
> >       int ret;
> >
> >       switch (type) {
> > @@ -565,6 +639,22 @@ static int adxl345_read_event_value(struct iio_dev *indio_dev,
> >               default:
> >                       return -EINVAL;
> >               }
> > +     case IIO_EV_TYPE_MAG:
> > +             switch (info) {
> > +             case IIO_EV_INFO_VALUE:
> > +                     ret = regmap_read(st->regmap, ADXL345_REG_THRESH_FF,
> > +                                       &ff_threshold);
> > +                     if (ret)
> > +                             return ret;
> > +                     *val = ff_threshold;
> > +                     return IIO_VAL_INT;
> > +             case IIO_EV_INFO_PERIOD:
> > +                     *val = st->ff_time_ms;
> > +                     *val2 = 1000;
> > +                     return IIO_VAL_FRACTIONAL;
> > +             default:
> > +                     return -EINVAL;
> > +             }
> >       default:
> >               return -EINVAL;
> >       }
> > @@ -612,6 +702,22 @@ static int adxl345_write_event_value(struct iio_dev *indio_dev,
> >                       return -EINVAL;
> >               }
> >               break;
> > +     case IIO_EV_TYPE_MAG:
> > +             switch (info) {
> > +             case IIO_EV_INFO_VALUE:
> > +                     ret = regmap_write(st->regmap, ADXL345_REG_THRESH_FF, val);
> > +                     if (ret)
> > +                             return ret;
> > +                     break;
> > +             case IIO_EV_INFO_PERIOD:
> > +                     ret = adxl345_set_ff_time(st, val, val2);
> > +                     if (ret)
> > +                             return ret;
> > +                     break;
> > +             default:
> > +                     return -EINVAL;
> > +             }
> > +             break;
> >       default:
> >               return -EINVAL;
> >       }
> > @@ -865,6 +971,17 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
> >                       return ret;
> >       }
> >
> > +     if (FIELD_GET(ADXL345_INT_FREE_FALL, int_stat)) {
> > +             ret = iio_push_event(indio_dev,
> > +                                  IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> > +                                                     IIO_MOD_X_OR_Y_OR_Z,
>
> This is the event that got me thinking about whether we were doing this right..
>
> > +                                                     IIO_EV_TYPE_MAG,
> > +                                                     IIO_EV_DIR_FALLING),
> > +                                  ts);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
>

Re: [PATCH v6 09/11] iio: accel: adxl345: add inactivity feature

[Lothar Rubusch]

Happy Easter (again)!

On Fri, Apr 18, 2025 at 8:34 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon, 14 Apr 2025 18:42:43 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Add the inactivity feature of the sensor. When activity and inactivity
> > are enabled, a link bit will be set linking activity and inactivity
> > handling. Additionally, the auto-sleep mode will be enabled. Due to the
> > link bit the sensor is going to auto-sleep when inactivity was
> > detected.
> >
> > Inactivity detection needs a threshold to be configured, and a time
> > after which it will go into inactivity state if measurements under
> > threshold.
> >
> > When a ODR is configured this time for inactivity is adjusted with a
> > corresponding reasonable default value, in order to have higher
> > frequencies and lower inactivity times, and lower sample frequency but
> > give more time until inactivity. Both with reasonable upper and lower
> > boundaries, since many of the sensor's features (e.g. auto-sleep) will
> > need to operate beween 12.5 Hz and 400 Hz. This is a default setting
> > when actively changing sample frequency, explicitly setting the time
> > until inactivity will overwrite the default.
> >
> > Similarly, setting the g-range will provide a default value for the
> > activity and inactivity thresholds. Both are implicit defaults, but
> > equally can be overwritten to be explicitly configured.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> Hi Lothar,
>
> Patches 6-8 look good to me.
>
> This runs into a similar issue to the freefall one. I haven't dug into
> the datasheet but does it report on one channel going inactive, or
> all being inactive at the same time?  I checked and it is the all
> case so we should be both on a pseudo channel to describe it right
> and reporting IIO_MOD_X_AND_Y_AND_Z not the OR form.
>
> Sorry again that I'm only realising this on v6 :(

No problem at all! Sure, I'm still in this phase where counting every
single commit upstream makes my ego greater. On the long run, though,
I guess it's better to build up knowledge and end up with a decent
implementation quality, than just increasing a commit counter. For me
it's fine. I also hope it's not too annoying for you.

>
> Difference is for Activity the definition is:
> "The activity bit is set when acceleration greater than the value
> stored in the THRESH_ACT register (Address 0x24) is experienced
> on _any_ participating axis, set by the ACT_INACT_CTL register
> (Address 0x27)."
> vs Inactivity:
> "The inactivity bit is set when acceleration of less than the value
> stored in the THRESH_INACT register (Address 0x25) is experienced
> for more time than is specified in the TIME_INACT
> register (Address 0x26) on _all_ participating axes, as set by the
> ACT_INACT_CTL register (Address 0x27). "
>
> So all vs any.
>

I think I  see your point. At least I change here for inactivity, too,
to AND'ed axis.

IMHO, if I set OR here, the first axis raising the inactivity will put
the sensor to sleep mode,
where AND needs all three axis in inactivity state. I'm not sure if
this works out, I need to verify
it still with the hardware, for now I'll change this to AND.

> > +
> > +/**
> > + * adxl345_set_inact_time_s - Configure inactivity time explicitly or by ODR.
> > + * @st: The sensor state instance.
> > + * @val_s: A desired time value, between 0 and 255.
> > + *
> > + * Inactivity time can be configured between 1 and 255 sec. If a val_s of 0
> > + * is configured by a user, then a default inactivity time will be computed.
> > + *
> > + * In such case, it should take power consumption into consideration. Thus it
> > + * shall be shorter for higher frequencies and longer for lower frequencies.
> > + * Hence, frequencies above 255 Hz shall default to 10 s and frequencies below
> > + * 10 Hz shall result in 255 s to detect inactivity.
> > + *
> > + * The approach simply subtracts the pre-decimal figure of the configured
> > + * sample frequency from 255 s to compute inactivity time [s]. Sub-Hz are thus
> > + * ignored in this estimation. The recommended ODRs for various features
> > + * (activity/inactivity, sleep modes, free fall, etc.) lie between 12.5 Hz and
> > + * 400 Hz, thus higher or lower frequencies will result in the boundary
> > + * defaults or need to be explicitly specified via val_s.
> > + *
> > + * Return: 0 or error value.
> > + */
> > +static int adxl345_set_inact_time_s(struct adxl345_state *st, u32 val_s)
> > +{
> > +     unsigned int max_boundary = 255;
> > +     unsigned int min_boundary = 10;
> > +     unsigned int val = min(val_s, max_boundary);
> > +     enum adxl345_odr odr;
> > +     unsigned int regval;
> > +     int ret;
> > +
> > +     if (val == 0) {
> > +             ret = regmap_read(st->regmap, ADXL345_REG_BW_RATE, &regval);
> > +             if (ret)
> > +                     return ret;
> > +             odr = FIELD_GET(ADXL345_BW_RATE_MSK, regval);
> > +
> > +             val = (adxl345_odr_tbl[odr][0] > max_boundary)
> > +                     ? min_boundary : max_boundary - adxl345_odr_tbl[odr][0];
> > +     }
> > +
> > +     return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
> >  }
> >
> >  /* tap */
> > @@ -837,6 +943,13 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
> >                       if (ret)
> >                               return ret;
> >                       return int_en;
> > +             case IIO_EV_DIR_FALLING:
> > +                     ret = adxl345_is_act_inact_en(st, chan->channel2,
>
> Does it makes sense to allow inactivity detection on a subset of channels but then
> report it as XYZ?  I guess it didn't matter when it was and OR, but if we
> change to AND as suggested that is going to be misleading.
>
> we might have to allow separate enables but report an event as the combination
> of channels that are enabled X_AND_Y, X_AND_Z etc  I guess we can improve activity
> channel case as well by doing that with the X_OR_Y etc
>

Well, initially I guess I only had one enable for inactivity.

This was kind of confusing to me. There is a register to enable
activity and inactivity on a per axis base [ACT_INACT_CTL, 0x27].

The interrupt event will set a single bit for inactivity or activity
[INT_SOURCE, 0x30]. In the interrupt handler further one can read out
the [ACT_TAP_STATUS, 0x2B], which contains tap and activity
directions, but no information about inactivity axis.

In summary, for the ADXL345 inactivity can be configured on a per axis
base, but the event won't tell about the axis that fell into
inactivity, i.e. the first inactivity is supposed to put the sensor
into power save (with link bit and power modes set - I think
inactivity should mainly be seen in the context of their/Analog's
power save concept). As said before, initially I only provided a
single "inactivity enable". Then I saw actually I could set and offer
this per axis. I don't know if there are use cases only to observe
particularly the x-axis for a general power save. Probably rather not.

So, I agree. But if you don't tell me explicitely to replace per axis
enables by a single one, I'll probably leave it as is. It implements
most transparently what the sensor can offer for configuration.

>
>
> > +                                                   ADXL345_INACTIVITY,
> > +                                                   &int_en);
> > +                     if (ret)
> > +                             return ret;
> > +                     return int_en;
> >               default:
> >                       return -EINVAL;
> >               }
> > @@ -881,6 +994,9 @@ static int adxl345_write_event_config(struct iio_dev *indio_dev,
> >               case IIO_EV_DIR_RISING:
> >                       return adxl345_set_act_inact_en(st, chan->channel2,
> >                                                       ADXL345_ACTIVITY, state);
> > +             case IIO_EV_DIR_FALLING:
> > +                     return adxl345_set_act_inact_en(st, chan->channel2,
> > +                                                     ADXL345_INACTIVITY, state);
> >               default:
> >                       return -EINVAL;
> >               }
>
> > @@ -1314,6 +1458,17 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
> >                       return ret;
> >       }
> >
> > +     if (FIELD_GET(ADXL345_INT_INACTIVITY, int_stat)) {
> > +             ret = iio_push_event(indio_dev,
> > +                                  IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> > +                                                     IIO_MOD_X_OR_Y_OR_Z,
>
> So this is our open question. Similar to the free fall case. Do we have the boolean
> logic right way around?
>
> > +                                                     IIO_EV_TYPE_THRESH,
> > +                                                     IIO_EV_DIR_FALLING),
> > +                                  ts);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +

Re: [PATCH v6 05/11] iio: accel: adxl345: add freefall feature

[Jonathan Cameron]

On Sun, 20 Apr 2025 23:26:47 +0200
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Happy Easter!
> 
> On Fri, Apr 18, 2025 at 8:23 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Mon, 14 Apr 2025 18:42:39 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >  
> > > Add the freefall detection of the sensor together with a threshold and
> > > time parameter. A freefall event is detected if the measuring signal
> > > falls below the threshold.
> > >
> > > Introduce a freefall threshold stored in regmap cache, and a freefall
> > > time, having the scaled time value stored as a member variable in the
> > > state instance.
> > >  
> > Reading this I wondered whether we had the event code consistent for
> > freefall detectors... Or indeed inactivity ones (which are kind of similarish)
> >
> > :( We don't it seems.  The issue is that
> > freefall is actually that all channels are simultaneously under the the magnitude
> > threshold, not one of them.  So it should I think be
> > X_AND_Y_AND_Z not X_OR_Y_OR_Z
> >  
> 
> I change to X_AND_Y_AND_Z.
> 
> > This is as opposed to activity detectors which tend to be any axis shows
> > activity and X_OR_Y_OR_Z applies.
> >
> > Anyhow upshot is I think I lead you astray on this and we should make this
> > one IIO_MOD_X_AND_Y_AND_Z
> >
> > A few other things inline.
> >
> > Unfortunately we don't deal with these events that often so I forget
> > what we did before :(
> >  
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > ---
> > >  drivers/iio/accel/adxl345_core.c | 125 +++++++++++++++++++++++++++++++
> > >  1 file changed, 125 insertions(+)
> > >
> > > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > > index c464c87033fb..ae02826e552b 100644
> > > --- a/drivers/iio/accel/adxl345_core.c
> > > +++ b/drivers/iio/accel/adxl345_core.c
> > > @@ -75,6 +75,7 @@ struct adxl345_state {
> > >       u32 tap_duration_us;
> > >       u32 tap_latent_us;
> > >       u32 tap_window_us;
> > > +     u32 ff_time_ms;
> > >
> > >       __le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE + 1] __aligned(IIO_DMA_MINALIGN);
> > >  };
> > > @@ -96,6 +97,14 @@ static struct iio_event_spec adxl345_events[] = {
> > >                       BIT(IIO_EV_INFO_RESET_TIMEOUT) |
> > >                       BIT(IIO_EV_INFO_TAP2_MIN_DELAY),
> > >       },
> > > +     {
> > > +             /* free fall */
> > > +             .type = IIO_EV_TYPE_MAG,
> > > +             .dir = IIO_EV_DIR_FALLING,
> > > +             .mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE) |
> > > +                     BIT(IIO_EV_INFO_VALUE) |
> > > +                     BIT(IIO_EV_INFO_PERIOD),
> > > +     },  
> > This is creating separate per axis enables, values and period. Does that make
> > sense?  If not you need to spin a kind of virtual channel (with mod X_AND_Y_AND_Z)
> > and add the events to it.
> >
> > See how the sca3000 does it for example.  
> 
> Hum, I'm not sure if I understand you correctly. In my driver, I'm
> using .mask_shared_by_type, and I verified there appears only one
> enable, one value and one period handle.
> # ls -l /sys/bus/iio/devices/iio:device0/events/
> total 0
> ...
> -rw-r--r-- 1 root root 4096 Apr 20 21:59 in_accel_mag_falling_en
> -rw-r--r-- 1 root root 4096 Apr 20 21:59 in_accel_mag_falling_period
> -rw-r--r-- 1 root root 4096 Apr 20 21:59 in_accel_mag_falling_value
> ...
> 
> In the sources of sca3000.c I saw this setup with .mask_separate. So,
> there I'd expect to see separate enables per axis, or am I wrong? In
> the case of the ADXL345, there should only be one freefall enable (in
> my driver) and not per axis. So, isn't this what is currently there?
> 
> So far I only adjust the or'ing to and'ing the axis for freefall.

So this is a messy corner of the ABI (because these are tricky to describe).
Shared by type usually means there is one attribute applying to all the
axis, but that they are reported separately, or potentially multiple events
/ _OR_ form used if we can distinguish exactly what the event is.

In this case there is no way for userspace to anticipate that the event
that might be generate is X_AND_Y_AND_Z.  So for this
the ABI solution we came up with is that virtual channel and separate.

So you get something along the lines of
in_accel_x&y&z_mag_falling_en
in_accel_x&y&z_mag_falling_period
etc

The tricky remaining corner is this only makes sense if we always enable
all axis (which is typical for a freefall detector). If we get a device
that oddly has per axis free fall enables, then it would be hard and I
might argue nonsense to enable them separately anyway.  Not true
here though I think.

Note that we may well have drivers using the ABI slightly differently for
freefall events which will be at least partly because I'd forgotten how
we resolved all this complexity long ago (that sca3000 driver is ancient!)
ABI like this is tricky to fix up, but we might want to consider some duplication
in those drivers so we standardize on one form for freefall (even if we have some
stray ABI from other possible solutions).

What we should definitely do is pull together some documentation on multi channel
event handling as the ABI docs are probably not enough.

Jonathan

Re: [PATCH v6 09/11] iio: accel: adxl345: add inactivity feature

[Jonathan Cameron]

On Mon, 21 Apr 2025 00:12:17 +0200
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Happy Easter (again)!
> 
> On Fri, Apr 18, 2025 at 8:34 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Mon, 14 Apr 2025 18:42:43 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >  
> > > Add the inactivity feature of the sensor. When activity and inactivity
> > > are enabled, a link bit will be set linking activity and inactivity
> > > handling. Additionally, the auto-sleep mode will be enabled. Due to the
> > > link bit the sensor is going to auto-sleep when inactivity was
> > > detected.
> > >
> > > Inactivity detection needs a threshold to be configured, and a time
> > > after which it will go into inactivity state if measurements under
> > > threshold.
> > >
> > > When a ODR is configured this time for inactivity is adjusted with a
> > > corresponding reasonable default value, in order to have higher
> > > frequencies and lower inactivity times, and lower sample frequency but
> > > give more time until inactivity. Both with reasonable upper and lower
> > > boundaries, since many of the sensor's features (e.g. auto-sleep) will
> > > need to operate beween 12.5 Hz and 400 Hz. This is a default setting
> > > when actively changing sample frequency, explicitly setting the time
> > > until inactivity will overwrite the default.
> > >
> > > Similarly, setting the g-range will provide a default value for the
> > > activity and inactivity thresholds. Both are implicit defaults, but
> > > equally can be overwritten to be explicitly configured.
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>  
> > Hi Lothar,
> >
> > Patches 6-8 look good to me.
> >
> > This runs into a similar issue to the freefall one. I haven't dug into
> > the datasheet but does it report on one channel going inactive, or
> > all being inactive at the same time?  I checked and it is the all
> > case so we should be both on a pseudo channel to describe it right
> > and reporting IIO_MOD_X_AND_Y_AND_Z not the OR form.
> >
> > Sorry again that I'm only realising this on v6 :(  
> 
> No problem at all! Sure, I'm still in this phase where counting every
> single commit upstream makes my ego greater. On the long run, though,
> I guess it's better to build up knowledge and end up with a decent
> implementation quality, than just increasing a commit counter. For me
> it's fine. I also hope it's not too annoying for you.
> 
> >
> > Difference is for Activity the definition is:
> > "The activity bit is set when acceleration greater than the value
> > stored in the THRESH_ACT register (Address 0x24) is experienced
> > on _any_ participating axis, set by the ACT_INACT_CTL register
> > (Address 0x27)."
> > vs Inactivity:
> > "The inactivity bit is set when acceleration of less than the value
> > stored in the THRESH_INACT register (Address 0x25) is experienced
> > for more time than is specified in the TIME_INACT
> > register (Address 0x26) on _all_ participating axes, as set by the
> > ACT_INACT_CTL register (Address 0x27). "
> >
> > So all vs any.
> >  
> 
> I think I  see your point. At least I change here for inactivity, too,
> to AND'ed axis.
> 
> IMHO, if I set OR here, the first axis raising the inactivity will put
> the sensor to sleep mode,
> where AND needs all three axis in inactivity state. I'm not sure if
> this works out, I need to verify
> it still with the hardware, for now I'll change this to AND.

I'd be surprised if it worked differently but indeed good to check!

> 
> > > +
> > > +/**
> > > + * adxl345_set_inact_time_s - Configure inactivity time explicitly or by ODR.
> > > + * @st: The sensor state instance.
> > > + * @val_s: A desired time value, between 0 and 255.
> > > + *
> > > + * Inactivity time can be configured between 1 and 255 sec. If a val_s of 0
> > > + * is configured by a user, then a default inactivity time will be computed.
> > > + *
> > > + * In such case, it should take power consumption into consideration. Thus it
> > > + * shall be shorter for higher frequencies and longer for lower frequencies.
> > > + * Hence, frequencies above 255 Hz shall default to 10 s and frequencies below
> > > + * 10 Hz shall result in 255 s to detect inactivity.
> > > + *
> > > + * The approach simply subtracts the pre-decimal figure of the configured
> > > + * sample frequency from 255 s to compute inactivity time [s]. Sub-Hz are thus
> > > + * ignored in this estimation. The recommended ODRs for various features
> > > + * (activity/inactivity, sleep modes, free fall, etc.) lie between 12.5 Hz and
> > > + * 400 Hz, thus higher or lower frequencies will result in the boundary
> > > + * defaults or need to be explicitly specified via val_s.
> > > + *
> > > + * Return: 0 or error value.
> > > + */
> > > +static int adxl345_set_inact_time_s(struct adxl345_state *st, u32 val_s)
> > > +{
> > > +     unsigned int max_boundary = 255;
> > > +     unsigned int min_boundary = 10;
> > > +     unsigned int val = min(val_s, max_boundary);
> > > +     enum adxl345_odr odr;
> > > +     unsigned int regval;
> > > +     int ret;
> > > +
> > > +     if (val == 0) {
> > > +             ret = regmap_read(st->regmap, ADXL345_REG_BW_RATE, &regval);
> > > +             if (ret)
> > > +                     return ret;
> > > +             odr = FIELD_GET(ADXL345_BW_RATE_MSK, regval);
> > > +
> > > +             val = (adxl345_odr_tbl[odr][0] > max_boundary)
> > > +                     ? min_boundary : max_boundary - adxl345_odr_tbl[odr][0];
> > > +     }
> > > +
> > > +     return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
> > >  }
> > >
> > >  /* tap */
> > > @@ -837,6 +943,13 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
> > >                       if (ret)
> > >                               return ret;
> > >                       return int_en;
> > > +             case IIO_EV_DIR_FALLING:
> > > +                     ret = adxl345_is_act_inact_en(st, chan->channel2,  
> >
> > Does it makes sense to allow inactivity detection on a subset of channels but then
> > report it as XYZ?  I guess it didn't matter when it was and OR, but if we
> > change to AND as suggested that is going to be misleading.
> >
> > we might have to allow separate enables but report an event as the combination
> > of channels that are enabled X_AND_Y, X_AND_Z etc  I guess we can improve activity
> > channel case as well by doing that with the X_OR_Y etc
> >  
> 
> Well, initially I guess I only had one enable for inactivity.
> 
> This was kind of confusing to me. There is a register to enable
> activity and inactivity on a per axis base [ACT_INACT_CTL, 0x27].

Agreed this is a slightly odd concept.

> 
> The interrupt event will set a single bit for inactivity or activity
> [INT_SOURCE, 0x30]. In the interrupt handler further one can read out
> the [ACT_TAP_STATUS, 0x2B], which contains tap and activity
> directions, but no information about inactivity axis.
> 
> In summary, for the ADXL345 inactivity can be configured on a per axis
> base, but the event won't tell about the axis that fell into
> inactivity, i.e. the first inactivity is supposed to put the sensor
> into power save (with link bit and power modes set - I think
> inactivity should mainly be seen in the context of their/Analog's
> power save concept). As said before, initially I only provided a
> single "inactivity enable". Then I saw actually I could set and offer
> this per axis. I don't know if there are use cases only to observe
> particularly the x-axis for a general power save. Probably rather not.
> 
> So, I agree. But if you don't tell me explicitely to replace per axis
> enables by a single one, I'll probably leave it as is. It implements
> most transparently what the sensor can offer for configuration.

The snag is what I mentioned for freefall. It becomes very hard to indicate
to userspace what it might expect for the x&y&z cases.  If inactivity requires
them all to be inactive, I think separate enables is going to be really
tricky to build a consistent ABI around :(

Some devices we've had in the past have allowed specific configuration of
and / or for axis combinations. For those we've normally kept clear because
the number of combinations gets sill quickly.

If we don't have a separate channel enable usecase today I think we should
go ahead with general inactivity / activity (and/or as appropriate) and
perhaps solve the per axis case if anyone ever cares about it.

> 
> >
> >  
> > > +                                                   ADXL345_INACTIVITY,
> > > +                                                   &int_en);
> > > +                     if (ret)
> > > +                             return ret;
> > > +                     return int_en;
> > >               default:
> > >                       return -EINVAL;
> > >               }
> > > @@ -881,6 +994,9 @@ static int adxl345_write_event_config(struct iio_dev *indio_dev,
> > >               case IIO_EV_DIR_RISING:
> > >                       return adxl345_set_act_inact_en(st, chan->channel2,
> > >                                                       ADXL345_ACTIVITY, state);
> > > +             case IIO_EV_DIR_FALLING:
> > > +                     return adxl345_set_act_inact_en(st, chan->channel2,
> > > +                                                     ADXL345_INACTIVITY, state);
> > >               default:
> > >                       return -EINVAL;
> > >               }  
> >  
> > > @@ -1314,6 +1458,17 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
> > >                       return ret;
> > >       }
> > >
> > > +     if (FIELD_GET(ADXL345_INT_INACTIVITY, int_stat)) {
> > > +             ret = iio_push_event(indio_dev,
> > > +                                  IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> > > +                                                     IIO_MOD_X_OR_Y_OR_Z,  
> >
> > So this is our open question. Similar to the free fall case. Do we have the boolean
> > logic right way around?
> >  
> > > +                                                     IIO_EV_TYPE_THRESH,
> > > +                                                     IIO_EV_DIR_FALLING),
> > > +                                  ts);
> > > +             if (ret)
> > > +                     return ret;
> > > +     }
> > > +  

Re: [PATCH v6 05/11] iio: accel: adxl345: add freefall feature

[Lothar Rubusch]

On Mon, Apr 21, 2025 at 12:16 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sun, 20 Apr 2025 23:26:47 +0200
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Happy Easter!
> >
> > On Fri, Apr 18, 2025 at 8:23 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > On Mon, 14 Apr 2025 18:42:39 +0000
> > > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> > >
> > > > Add the freefall detection of the sensor together with a threshold and
> > > > time parameter. A freefall event is detected if the measuring signal
> > > > falls below the threshold.
> > > >
> > > > Introduce a freefall threshold stored in regmap cache, and a freefall
> > > > time, having the scaled time value stored as a member variable in the
> > > > state instance.
> > > >
> > > Reading this I wondered whether we had the event code consistent for
> > > freefall detectors... Or indeed inactivity ones (which are kind of similarish)
> > >
> > > :( We don't it seems.  The issue is that
> > > freefall is actually that all channels are simultaneously under the the magnitude
> > > threshold, not one of them.  So it should I think be
> > > X_AND_Y_AND_Z not X_OR_Y_OR_Z
> > >
> >
> > I change to X_AND_Y_AND_Z.
> >
> > > This is as opposed to activity detectors which tend to be any axis shows
> > > activity and X_OR_Y_OR_Z applies.
> > >
> > > Anyhow upshot is I think I lead you astray on this and we should make this
> > > one IIO_MOD_X_AND_Y_AND_Z
> > >
> > > A few other things inline.
> > >
> > > Unfortunately we don't deal with these events that often so I forget
> > > what we did before :(
> > >
> > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > > ---
> > > >  drivers/iio/accel/adxl345_core.c | 125 +++++++++++++++++++++++++++++++
> > > >  1 file changed, 125 insertions(+)
> > > >
> > > > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > > > index c464c87033fb..ae02826e552b 100644
> > > > --- a/drivers/iio/accel/adxl345_core.c
> > > > +++ b/drivers/iio/accel/adxl345_core.c
> > > > @@ -75,6 +75,7 @@ struct adxl345_state {
> > > >       u32 tap_duration_us;
> > > >       u32 tap_latent_us;
> > > >       u32 tap_window_us;
> > > > +     u32 ff_time_ms;
> > > >
> > > >       __le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE + 1] __aligned(IIO_DMA_MINALIGN);
> > > >  };
> > > > @@ -96,6 +97,14 @@ static struct iio_event_spec adxl345_events[] = {
> > > >                       BIT(IIO_EV_INFO_RESET_TIMEOUT) |
> > > >                       BIT(IIO_EV_INFO_TAP2_MIN_DELAY),
> > > >       },
> > > > +     {
> > > > +             /* free fall */
> > > > +             .type = IIO_EV_TYPE_MAG,
> > > > +             .dir = IIO_EV_DIR_FALLING,
> > > > +             .mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE) |
> > > > +                     BIT(IIO_EV_INFO_VALUE) |
> > > > +                     BIT(IIO_EV_INFO_PERIOD),
> > > > +     },
> > > This is creating separate per axis enables, values and period. Does that make
> > > sense?  If not you need to spin a kind of virtual channel (with mod X_AND_Y_AND_Z)
> > > and add the events to it.
> > >
> > > See how the sca3000 does it for example.
> >
> > Hum, I'm not sure if I understand you correctly. In my driver, I'm
> > using .mask_shared_by_type, and I verified there appears only one
> > enable, one value and one period handle.
> > # ls -l /sys/bus/iio/devices/iio:device0/events/
> > total 0
> > ...
> > -rw-r--r-- 1 root root 4096 Apr 20 21:59 in_accel_mag_falling_en
> > -rw-r--r-- 1 root root 4096 Apr 20 21:59 in_accel_mag_falling_period
> > -rw-r--r-- 1 root root 4096 Apr 20 21:59 in_accel_mag_falling_value
> > ...
> >
> > In the sources of sca3000.c I saw this setup with .mask_separate. So,
> > there I'd expect to see separate enables per axis, or am I wrong? In
> > the case of the ADXL345, there should only be one freefall enable (in
> > my driver) and not per axis. So, isn't this what is currently there?
> >
> > So far I only adjust the or'ing to and'ing the axis for freefall.
>
> So this is a messy corner of the ABI (because these are tricky to describe).
> Shared by type usually means there is one attribute applying to all the
> axis, but that they are reported separately, or potentially multiple events
> / _OR_ form used if we can distinguish exactly what the event is.
>
> In this case there is no way for userspace to anticipate that the event
> that might be generate is X_AND_Y_AND_Z.  So for this
> the ABI solution we came up with is that virtual channel and separate.
>
> So you get something along the lines of
> in_accel_x&y&z_mag_falling_en
> in_accel_x&y&z_mag_falling_period
> etc
>
> The tricky remaining corner is this only makes sense if we always enable
> all axis (which is typical for a freefall detector). If we get a device
> that oddly has per axis free fall enables, then it would be hard and I
> might argue nonsense to enable them separately anyway.  Not true
> here though I think.
>
> Note that we may well have drivers using the ABI slightly differently for
> freefall events which will be at least partly because I'd forgotten how
> we resolved all this complexity long ago (that sca3000 driver is ancient!)
> ABI like this is tricky to fix up, but we might want to consider some duplication
> in those drivers so we standardize on one form for freefall (even if we have some
> stray ABI from other possible solutions).
>
> What we should definitely do is pull together some documentation on multi channel
> event handling as the ABI docs are probably not enough.
>

As I (begin to) understand now, in case of the sca3000, the virtual
channel is literally an extra channel. That means, we're talking
probably about this here down below, right?
...
 512 static const struct iio_chan_spec sca3000_channels[] = {
 513         SCA3000_CHAN(0, IIO_MOD_X),
 514         SCA3000_CHAN(1, IIO_MOD_Y),
 515         SCA3000_CHAN(2, IIO_MOD_Z),
 516         {
 517                 .type = IIO_ACCEL,
 518                 .modified = 1,
 519                 .channel2 = IIO_MOD_X_AND_Y_AND_Z,
 520                 .scan_index = -1, /* Fake channel */
 521                 .event_spec = &sca3000_freefall_event_spec,
 522                 .num_event_specs = 1,
 523         },
 524 };
...
<taken from sca3000.c>

What's now missing for freefall and the ADXL345 and v7 in particular?
- I need to provide a similar channel setup as in  the sca3000 snippet
above, the virtual channel
- I need to AND the axis for this channel
- Now, with the virtual channel usage will be "separate" instead of
"shared", which will result in a single enable handle in sysfs

Is this correct?

Sry, I needed to re-read your answer several times. What confuses me
is still a bit the "virtual extra-channel". Probably, I need to build
up a bit more practical experience with the channels. Providing just a
single enable handle already looks good to me. I still don't grok
entirely where this can be / is problematic in such case, but no need
to explain it now in full detail. If the TODOs match at least with my
understanding I will try to implement the above points.

> Jonathan
>

Re: [PATCH v6 09/11] iio: accel: adxl345: add inactivity feature

[Lothar Rubusch]

On Mon, Apr 21, 2025 at 12:22 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon, 21 Apr 2025 00:12:17 +0200
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Happy Easter (again)!
> >
> > On Fri, Apr 18, 2025 at 8:34 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > On Mon, 14 Apr 2025 18:42:43 +0000
> > > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> > >
> > > > Add the inactivity feature of the sensor. When activity and inactivity
> > > > are enabled, a link bit will be set linking activity and inactivity
> > > > handling. Additionally, the auto-sleep mode will be enabled. Due to the
> > > > link bit the sensor is going to auto-sleep when inactivity was
> > > > detected.
> > > >
> > > > Inactivity detection needs a threshold to be configured, and a time
> > > > after which it will go into inactivity state if measurements under
> > > > threshold.
> > > >
> > > > When a ODR is configured this time for inactivity is adjusted with a
> > > > corresponding reasonable default value, in order to have higher
> > > > frequencies and lower inactivity times, and lower sample frequency but
> > > > give more time until inactivity. Both with reasonable upper and lower
> > > > boundaries, since many of the sensor's features (e.g. auto-sleep) will
> > > > need to operate beween 12.5 Hz and 400 Hz. This is a default setting
> > > > when actively changing sample frequency, explicitly setting the time
> > > > until inactivity will overwrite the default.
> > > >
> > > > Similarly, setting the g-range will provide a default value for the
> > > > activity and inactivity thresholds. Both are implicit defaults, but
> > > > equally can be overwritten to be explicitly configured.
> > > >
> > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > Hi Lothar,
> > >
> > > Patches 6-8 look good to me.
> > >
> > > This runs into a similar issue to the freefall one. I haven't dug into
> > > the datasheet but does it report on one channel going inactive, or
> > > all being inactive at the same time?  I checked and it is the all
> > > case so we should be both on a pseudo channel to describe it right
> > > and reporting IIO_MOD_X_AND_Y_AND_Z not the OR form.
> > >
> > > Sorry again that I'm only realising this on v6 :(
> >
> > No problem at all! Sure, I'm still in this phase where counting every
> > single commit upstream makes my ego greater. On the long run, though,
> > I guess it's better to build up knowledge and end up with a decent
> > implementation quality, than just increasing a commit counter. For me
> > it's fine. I also hope it's not too annoying for you.
> >
> > >
> > > Difference is for Activity the definition is:
> > > "The activity bit is set when acceleration greater than the value
> > > stored in the THRESH_ACT register (Address 0x24) is experienced
> > > on _any_ participating axis, set by the ACT_INACT_CTL register
> > > (Address 0x27)."
> > > vs Inactivity:
> > > "The inactivity bit is set when acceleration of less than the value
> > > stored in the THRESH_INACT register (Address 0x25) is experienced
> > > for more time than is specified in the TIME_INACT
> > > register (Address 0x26) on _all_ participating axes, as set by the
> > > ACT_INACT_CTL register (Address 0x27). "
> > >
> > > So all vs any.
> > >
> >
> > I think I  see your point. At least I change here for inactivity, too,
> > to AND'ed axis.
> >
> > IMHO, if I set OR here, the first axis raising the inactivity will put
> > the sensor to sleep mode,
> > where AND needs all three axis in inactivity state. I'm not sure if
> > this works out, I need to verify
> > it still with the hardware, for now I'll change this to AND.
>
> I'd be surprised if it worked differently but indeed good to check!
>
> >
> > > > +
> > > > +/**
> > > > + * adxl345_set_inact_time_s - Configure inactivity time explicitly or by ODR.
> > > > + * @st: The sensor state instance.
> > > > + * @val_s: A desired time value, between 0 and 255.
> > > > + *
> > > > + * Inactivity time can be configured between 1 and 255 sec. If a val_s of 0
> > > > + * is configured by a user, then a default inactivity time will be computed.
> > > > + *
> > > > + * In such case, it should take power consumption into consideration. Thus it
> > > > + * shall be shorter for higher frequencies and longer for lower frequencies.
> > > > + * Hence, frequencies above 255 Hz shall default to 10 s and frequencies below
> > > > + * 10 Hz shall result in 255 s to detect inactivity.
> > > > + *
> > > > + * The approach simply subtracts the pre-decimal figure of the configured
> > > > + * sample frequency from 255 s to compute inactivity time [s]. Sub-Hz are thus
> > > > + * ignored in this estimation. The recommended ODRs for various features
> > > > + * (activity/inactivity, sleep modes, free fall, etc.) lie between 12.5 Hz and
> > > > + * 400 Hz, thus higher or lower frequencies will result in the boundary
> > > > + * defaults or need to be explicitly specified via val_s.
> > > > + *
> > > > + * Return: 0 or error value.
> > > > + */
> > > > +static int adxl345_set_inact_time_s(struct adxl345_state *st, u32 val_s)
> > > > +{
> > > > +     unsigned int max_boundary = 255;
> > > > +     unsigned int min_boundary = 10;
> > > > +     unsigned int val = min(val_s, max_boundary);
> > > > +     enum adxl345_odr odr;
> > > > +     unsigned int regval;
> > > > +     int ret;
> > > > +
> > > > +     if (val == 0) {
> > > > +             ret = regmap_read(st->regmap, ADXL345_REG_BW_RATE, &regval);
> > > > +             if (ret)
> > > > +                     return ret;
> > > > +             odr = FIELD_GET(ADXL345_BW_RATE_MSK, regval);
> > > > +
> > > > +             val = (adxl345_odr_tbl[odr][0] > max_boundary)
> > > > +                     ? min_boundary : max_boundary - adxl345_odr_tbl[odr][0];
> > > > +     }
> > > > +
> > > > +     return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
> > > >  }
> > > >
> > > >  /* tap */
> > > > @@ -837,6 +943,13 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
> > > >                       if (ret)
> > > >                               return ret;
> > > >                       return int_en;
> > > > +             case IIO_EV_DIR_FALLING:
> > > > +                     ret = adxl345_is_act_inact_en(st, chan->channel2,
> > >
> > > Does it makes sense to allow inactivity detection on a subset of channels but then
> > > report it as XYZ?  I guess it didn't matter when it was and OR, but if we
> > > change to AND as suggested that is going to be misleading.
> > >
> > > we might have to allow separate enables but report an event as the combination
> > > of channels that are enabled X_AND_Y, X_AND_Z etc  I guess we can improve activity
> > > channel case as well by doing that with the X_OR_Y etc
> > >
> >
> > Well, initially I guess I only had one enable for inactivity.
> >
> > This was kind of confusing to me. There is a register to enable
> > activity and inactivity on a per axis base [ACT_INACT_CTL, 0x27].
>
> Agreed this is a slightly odd concept.
>
> >
> > The interrupt event will set a single bit for inactivity or activity
> > [INT_SOURCE, 0x30]. In the interrupt handler further one can read out
> > the [ACT_TAP_STATUS, 0x2B], which contains tap and activity
> > directions, but no information about inactivity axis.
> >
> > In summary, for the ADXL345 inactivity can be configured on a per axis
> > base, but the event won't tell about the axis that fell into
> > inactivity, i.e. the first inactivity is supposed to put the sensor
> > into power save (with link bit and power modes set - I think
> > inactivity should mainly be seen in the context of their/Analog's
> > power save concept). As said before, initially I only provided a
> > single "inactivity enable". Then I saw actually I could set and offer
> > this per axis. I don't know if there are use cases only to observe
> > particularly the x-axis for a general power save. Probably rather not.
> >
> > So, I agree. But if you don't tell me explicitely to replace per axis
> > enables by a single one, I'll probably leave it as is. It implements
> > most transparently what the sensor can offer for configuration.
>
> The snag is what I mentioned for freefall. It becomes very hard to indicate
> to userspace what it might expect for the x&y&z cases.  If inactivity requires
> them all to be inactive, I think separate enables is going to be really
> tricky to build a consistent ABI around :(
>
> Some devices we've had in the past have allowed specific configuration of
> and / or for axis combinations. For those we've normally kept clear because
> the number of combinations gets sill quickly.
>
> If we don't have a separate channel enable usecase today I think we should
> go ahead with general inactivity / activity (and/or as appropriate) and
> perhaps solve the per axis case if anyone ever cares about it.
>

Well, I think here we need to distinguish:
Activity: would allow per axis enables and events indicate per axis activity
Inactivity: allows per axis enables, but only a generic inactivity indication

So, also here, what's still missing? When doing it similarly  to my
understanding of freefall now, for a v7 of the patches...

Activity:
- I would leave activity as is (is this ok?)

Inactivity:
- I replace single three axis enables by a generic enable, setting and
unsetting all three axis for inactivity
- I need probably also to provide a similar virtual channel
- The axis for this channel are AND'ed
- Now, with the virtual channel, usage will be "separate" instead of
"shared", which will result in a single enable handle in sysfs

Is this a correct understanding of what is +/- missing? Can you agree
to the points I listed up, or is something's missing (documentation of
course later)?

> >
> > >
> > >
> > > > +                                                   ADXL345_INACTIVITY,
> > > > +                                                   &int_en);
> > > > +                     if (ret)
> > > > +                             return ret;
> > > > +                     return int_en;
> > > >               default:
> > > >                       return -EINVAL;
> > > >               }
> > > > @@ -881,6 +994,9 @@ static int adxl345_write_event_config(struct iio_dev *indio_dev,
> > > >               case IIO_EV_DIR_RISING:
> > > >                       return adxl345_set_act_inact_en(st, chan->channel2,
> > > >                                                       ADXL345_ACTIVITY, state);
> > > > +             case IIO_EV_DIR_FALLING:
> > > > +                     return adxl345_set_act_inact_en(st, chan->channel2,
> > > > +                                                     ADXL345_INACTIVITY, state);
> > > >               default:
> > > >                       return -EINVAL;
> > > >               }
> > >
> > > > @@ -1314,6 +1458,17 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
> > > >                       return ret;
> > > >       }
> > > >
> > > > +     if (FIELD_GET(ADXL345_INT_INACTIVITY, int_stat)) {
> > > > +             ret = iio_push_event(indio_dev,
> > > > +                                  IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> > > > +                                                     IIO_MOD_X_OR_Y_OR_Z,
> > >
> > > So this is our open question. Similar to the free fall case. Do we have the boolean
> > > logic right way around?
> > >
> > > > +                                                     IIO_EV_TYPE_THRESH,
> > > > +                                                     IIO_EV_DIR_FALLING),
> > > > +                                  ts);
> > > > +             if (ret)
> > > > +                     return ret;
> > > > +     }
> > > > +
>

Re: [PATCH v6 05/11] iio: accel: adxl345: add freefall feature

[Jonathan Cameron]

On Mon, 21 Apr 2025 15:31:19 +0200
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> On Mon, Apr 21, 2025 at 12:16 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Sun, 20 Apr 2025 23:26:47 +0200
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >  
> > > Happy Easter!
> > >
> > > On Fri, Apr 18, 2025 at 8:23 PM Jonathan Cameron <jic23@kernel.org> wrote:  
> > > >
> > > > On Mon, 14 Apr 2025 18:42:39 +0000
> > > > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> > > >  
> > > > > Add the freefall detection of the sensor together with a threshold and
> > > > > time parameter. A freefall event is detected if the measuring signal
> > > > > falls below the threshold.
> > > > >
> > > > > Introduce a freefall threshold stored in regmap cache, and a freefall
> > > > > time, having the scaled time value stored as a member variable in the
> > > > > state instance.
> > > > >  
> > > > Reading this I wondered whether we had the event code consistent for
> > > > freefall detectors... Or indeed inactivity ones (which are kind of similarish)
> > > >
> > > > :( We don't it seems.  The issue is that
> > > > freefall is actually that all channels are simultaneously under the the magnitude
> > > > threshold, not one of them.  So it should I think be
> > > > X_AND_Y_AND_Z not X_OR_Y_OR_Z
> > > >  
> > >
> > > I change to X_AND_Y_AND_Z.
> > >  
> > > > This is as opposed to activity detectors which tend to be any axis shows
> > > > activity and X_OR_Y_OR_Z applies.
> > > >
> > > > Anyhow upshot is I think I lead you astray on this and we should make this
> > > > one IIO_MOD_X_AND_Y_AND_Z
> > > >
> > > > A few other things inline.
> > > >
> > > > Unfortunately we don't deal with these events that often so I forget
> > > > what we did before :(
> > > >  
> > > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > > > ---
> > > > >  drivers/iio/accel/adxl345_core.c | 125 +++++++++++++++++++++++++++++++
> > > > >  1 file changed, 125 insertions(+)
> > > > >
> > > > > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > > > > index c464c87033fb..ae02826e552b 100644
> > > > > --- a/drivers/iio/accel/adxl345_core.c
> > > > > +++ b/drivers/iio/accel/adxl345_core.c
> > > > > @@ -75,6 +75,7 @@ struct adxl345_state {
> > > > >       u32 tap_duration_us;
> > > > >       u32 tap_latent_us;
> > > > >       u32 tap_window_us;
> > > > > +     u32 ff_time_ms;
> > > > >
> > > > >       __le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE + 1] __aligned(IIO_DMA_MINALIGN);
> > > > >  };
> > > > > @@ -96,6 +97,14 @@ static struct iio_event_spec adxl345_events[] = {
> > > > >                       BIT(IIO_EV_INFO_RESET_TIMEOUT) |
> > > > >                       BIT(IIO_EV_INFO_TAP2_MIN_DELAY),
> > > > >       },
> > > > > +     {
> > > > > +             /* free fall */
> > > > > +             .type = IIO_EV_TYPE_MAG,
> > > > > +             .dir = IIO_EV_DIR_FALLING,
> > > > > +             .mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE) |
> > > > > +                     BIT(IIO_EV_INFO_VALUE) |
> > > > > +                     BIT(IIO_EV_INFO_PERIOD),
> > > > > +     },  
> > > > This is creating separate per axis enables, values and period. Does that make
> > > > sense?  If not you need to spin a kind of virtual channel (with mod X_AND_Y_AND_Z)
> > > > and add the events to it.
> > > >
> > > > See how the sca3000 does it for example.  
> > >
> > > Hum, I'm not sure if I understand you correctly. In my driver, I'm
> > > using .mask_shared_by_type, and I verified there appears only one
> > > enable, one value and one period handle.
> > > # ls -l /sys/bus/iio/devices/iio:device0/events/
> > > total 0
> > > ...
> > > -rw-r--r-- 1 root root 4096 Apr 20 21:59 in_accel_mag_falling_en
> > > -rw-r--r-- 1 root root 4096 Apr 20 21:59 in_accel_mag_falling_period
> > > -rw-r--r-- 1 root root 4096 Apr 20 21:59 in_accel_mag_falling_value
> > > ...
> > >
> > > In the sources of sca3000.c I saw this setup with .mask_separate. So,
> > > there I'd expect to see separate enables per axis, or am I wrong? In
> > > the case of the ADXL345, there should only be one freefall enable (in
> > > my driver) and not per axis. So, isn't this what is currently there?
> > >
> > > So far I only adjust the or'ing to and'ing the axis for freefall.  
> >
> > So this is a messy corner of the ABI (because these are tricky to describe).
> > Shared by type usually means there is one attribute applying to all the
> > axis, but that they are reported separately, or potentially multiple events
> > / _OR_ form used if we can distinguish exactly what the event is.
> >
> > In this case there is no way for userspace to anticipate that the event
> > that might be generate is X_AND_Y_AND_Z.  So for this
> > the ABI solution we came up with is that virtual channel and separate.
> >
> > So you get something along the lines of
> > in_accel_x&y&z_mag_falling_en
> > in_accel_x&y&z_mag_falling_period
> > etc
> >
> > The tricky remaining corner is this only makes sense if we always enable
> > all axis (which is typical for a freefall detector). If we get a device
> > that oddly has per axis free fall enables, then it would be hard and I
> > might argue nonsense to enable them separately anyway.  Not true
> > here though I think.
> >
> > Note that we may well have drivers using the ABI slightly differently for
> > freefall events which will be at least partly because I'd forgotten how
> > we resolved all this complexity long ago (that sca3000 driver is ancient!)
> > ABI like this is tricky to fix up, but we might want to consider some duplication
> > in those drivers so we standardize on one form for freefall (even if we have some
> > stray ABI from other possible solutions).
> >
> > What we should definitely do is pull together some documentation on multi channel
> > event handling as the ABI docs are probably not enough.
> >  
> 
> As I (begin to) understand now, in case of the sca3000, the virtual
> channel is literally an extra channel. That means, we're talking
> probably about this here down below, right?
> ...
yes. That is what I was referring to.

>  512 static const struct iio_chan_spec sca3000_channels[] = {
>  513         SCA3000_CHAN(0, IIO_MOD_X),
>  514         SCA3000_CHAN(1, IIO_MOD_Y),
>  515         SCA3000_CHAN(2, IIO_MOD_Z),
>  516         {
>  517                 .type = IIO_ACCEL,
>  518                 .modified = 1,
>  519                 .channel2 = IIO_MOD_X_AND_Y_AND_Z,
>  520                 .scan_index = -1, /* Fake channel */
>  521                 .event_spec = &sca3000_freefall_event_spec,
>  522                 .num_event_specs = 1,
>  523         },
>  524 };
> ...
> <taken from sca3000.c>
> 
> What's now missing for freefall and the ADXL345 and v7 in particular?
> - I need to provide a similar channel setup as in  the sca3000 snippet
> above, the virtual channel
> - I need to AND the axis for this channel
> - Now, with the virtual channel usage will be "separate" instead of
> "shared", which will result in a single enable handle in sysfs
> 
> Is this correct?

Yes.  I think that's all correct.

> 
> Sry, I needed to re-read your answer several times. What confuses me
> is still a bit the "virtual extra-channel". 

I made up that term for it as there is no actual traditional channel
there.  I should have given a pointer to the actual iio_chan_spec entry.
Sorry about that!

>Probably, I need to build
> up a bit more practical experience with the channels. Providing just a
> single enable handle already looks good to me. I still don't grok
> entirely where this can be / is problematic in such case, but no need
> to explain it now in full detail. If the TODOs match at least with my
> understanding I will try to implement the above points.

These events are just a bit weird when aligned with the simpler ones
we tend to get on ADCs etc.  Giving userspace the best visibility we
can is always good.  Here it is possible to get that, but in some other
cases we haven't yet come up with a good generic ABI :(

Jonathan


> 
> > Jonathan
> >  

Re: [PATCH v6 09/11] iio: accel: adxl345: add inactivity feature

[Jonathan Cameron]

On Mon, 21 Apr 2025 15:39:33 +0200
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> On Mon, Apr 21, 2025 at 12:22 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Mon, 21 Apr 2025 00:12:17 +0200
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >  
> > > Happy Easter (again)!
> > >
> > > On Fri, Apr 18, 2025 at 8:34 PM Jonathan Cameron <jic23@kernel.org> wrote:  
> > > >
> > > > On Mon, 14 Apr 2025 18:42:43 +0000
> > > > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> > > >  
> > > > > Add the inactivity feature of the sensor. When activity and inactivity
> > > > > are enabled, a link bit will be set linking activity and inactivity
> > > > > handling. Additionally, the auto-sleep mode will be enabled. Due to the
> > > > > link bit the sensor is going to auto-sleep when inactivity was
> > > > > detected.
> > > > >
> > > > > Inactivity detection needs a threshold to be configured, and a time
> > > > > after which it will go into inactivity state if measurements under
> > > > > threshold.
> > > > >
> > > > > When a ODR is configured this time for inactivity is adjusted with a
> > > > > corresponding reasonable default value, in order to have higher
> > > > > frequencies and lower inactivity times, and lower sample frequency but
> > > > > give more time until inactivity. Both with reasonable upper and lower
> > > > > boundaries, since many of the sensor's features (e.g. auto-sleep) will
> > > > > need to operate beween 12.5 Hz and 400 Hz. This is a default setting
> > > > > when actively changing sample frequency, explicitly setting the time
> > > > > until inactivity will overwrite the default.
> > > > >
> > > > > Similarly, setting the g-range will provide a default value for the
> > > > > activity and inactivity thresholds. Both are implicit defaults, but
> > > > > equally can be overwritten to be explicitly configured.
> > > > >
> > > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>  
> > > > Hi Lothar,
> > > >
> > > > Patches 6-8 look good to me.
> > > >
> > > > This runs into a similar issue to the freefall one. I haven't dug into
> > > > the datasheet but does it report on one channel going inactive, or
> > > > all being inactive at the same time?  I checked and it is the all
> > > > case so we should be both on a pseudo channel to describe it right
> > > > and reporting IIO_MOD_X_AND_Y_AND_Z not the OR form.
> > > >
> > > > Sorry again that I'm only realising this on v6 :(  
> > >
> > > No problem at all! Sure, I'm still in this phase where counting every
> > > single commit upstream makes my ego greater. On the long run, though,
> > > I guess it's better to build up knowledge and end up with a decent
> > > implementation quality, than just increasing a commit counter. For me
> > > it's fine. I also hope it's not too annoying for you.
> > >  
> > > >
> > > > Difference is for Activity the definition is:
> > > > "The activity bit is set when acceleration greater than the value
> > > > stored in the THRESH_ACT register (Address 0x24) is experienced
> > > > on _any_ participating axis, set by the ACT_INACT_CTL register
> > > > (Address 0x27)."
> > > > vs Inactivity:
> > > > "The inactivity bit is set when acceleration of less than the value
> > > > stored in the THRESH_INACT register (Address 0x25) is experienced
> > > > for more time than is specified in the TIME_INACT
> > > > register (Address 0x26) on _all_ participating axes, as set by the
> > > > ACT_INACT_CTL register (Address 0x27). "
> > > >
> > > > So all vs any.
> > > >  
> > >
> > > I think I  see your point. At least I change here for inactivity, too,
> > > to AND'ed axis.
> > >
> > > IMHO, if I set OR here, the first axis raising the inactivity will put
> > > the sensor to sleep mode,
> > > where AND needs all three axis in inactivity state. I'm not sure if
> > > this works out, I need to verify
> > > it still with the hardware, for now I'll change this to AND.  
> >
> > I'd be surprised if it worked differently but indeed good to check!
> >  
> > >  
> > > > > +
> > > > > +/**
> > > > > + * adxl345_set_inact_time_s - Configure inactivity time explicitly or by ODR.
> > > > > + * @st: The sensor state instance.
> > > > > + * @val_s: A desired time value, between 0 and 255.
> > > > > + *
> > > > > + * Inactivity time can be configured between 1 and 255 sec. If a val_s of 0
> > > > > + * is configured by a user, then a default inactivity time will be computed.
> > > > > + *
> > > > > + * In such case, it should take power consumption into consideration. Thus it
> > > > > + * shall be shorter for higher frequencies and longer for lower frequencies.
> > > > > + * Hence, frequencies above 255 Hz shall default to 10 s and frequencies below
> > > > > + * 10 Hz shall result in 255 s to detect inactivity.
> > > > > + *
> > > > > + * The approach simply subtracts the pre-decimal figure of the configured
> > > > > + * sample frequency from 255 s to compute inactivity time [s]. Sub-Hz are thus
> > > > > + * ignored in this estimation. The recommended ODRs for various features
> > > > > + * (activity/inactivity, sleep modes, free fall, etc.) lie between 12.5 Hz and
> > > > > + * 400 Hz, thus higher or lower frequencies will result in the boundary
> > > > > + * defaults or need to be explicitly specified via val_s.
> > > > > + *
> > > > > + * Return: 0 or error value.
> > > > > + */
> > > > > +static int adxl345_set_inact_time_s(struct adxl345_state *st, u32 val_s)
> > > > > +{
> > > > > +     unsigned int max_boundary = 255;
> > > > > +     unsigned int min_boundary = 10;
> > > > > +     unsigned int val = min(val_s, max_boundary);
> > > > > +     enum adxl345_odr odr;
> > > > > +     unsigned int regval;
> > > > > +     int ret;
> > > > > +
> > > > > +     if (val == 0) {
> > > > > +             ret = regmap_read(st->regmap, ADXL345_REG_BW_RATE, &regval);
> > > > > +             if (ret)
> > > > > +                     return ret;
> > > > > +             odr = FIELD_GET(ADXL345_BW_RATE_MSK, regval);
> > > > > +
> > > > > +             val = (adxl345_odr_tbl[odr][0] > max_boundary)
> > > > > +                     ? min_boundary : max_boundary - adxl345_odr_tbl[odr][0];
> > > > > +     }
> > > > > +
> > > > > +     return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
> > > > >  }
> > > > >
> > > > >  /* tap */
> > > > > @@ -837,6 +943,13 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
> > > > >                       if (ret)
> > > > >                               return ret;
> > > > >                       return int_en;
> > > > > +             case IIO_EV_DIR_FALLING:
> > > > > +                     ret = adxl345_is_act_inact_en(st, chan->channel2,  
> > > >
> > > > Does it makes sense to allow inactivity detection on a subset of channels but then
> > > > report it as XYZ?  I guess it didn't matter when it was and OR, but if we
> > > > change to AND as suggested that is going to be misleading.
> > > >
> > > > we might have to allow separate enables but report an event as the combination
> > > > of channels that are enabled X_AND_Y, X_AND_Z etc  I guess we can improve activity
> > > > channel case as well by doing that with the X_OR_Y etc
> > > >  
> > >
> > > Well, initially I guess I only had one enable for inactivity.
> > >
> > > This was kind of confusing to me. There is a register to enable
> > > activity and inactivity on a per axis base [ACT_INACT_CTL, 0x27].  
> >
> > Agreed this is a slightly odd concept.
> >  
> > >
> > > The interrupt event will set a single bit for inactivity or activity
> > > [INT_SOURCE, 0x30]. In the interrupt handler further one can read out
> > > the [ACT_TAP_STATUS, 0x2B], which contains tap and activity
> > > directions, but no information about inactivity axis.
> > >
> > > In summary, for the ADXL345 inactivity can be configured on a per axis
> > > base, but the event won't tell about the axis that fell into
> > > inactivity, i.e. the first inactivity is supposed to put the sensor
> > > into power save (with link bit and power modes set - I think
> > > inactivity should mainly be seen in the context of their/Analog's
> > > power save concept). As said before, initially I only provided a
> > > single "inactivity enable". Then I saw actually I could set and offer
> > > this per axis. I don't know if there are use cases only to observe
> > > particularly the x-axis for a general power save. Probably rather not.
> > >
> > > So, I agree. But if you don't tell me explicitely to replace per axis
> > > enables by a single one, I'll probably leave it as is. It implements
> > > most transparently what the sensor can offer for configuration.  
> >
> > The snag is what I mentioned for freefall. It becomes very hard to indicate
> > to userspace what it might expect for the x&y&z cases.  If inactivity requires
> > them all to be inactive, I think separate enables is going to be really
> > tricky to build a consistent ABI around :(
> >
> > Some devices we've had in the past have allowed specific configuration of
> > and / or for axis combinations. For those we've normally kept clear because
> > the number of combinations gets sill quickly.
> >
> > If we don't have a separate channel enable usecase today I think we should
> > go ahead with general inactivity / activity (and/or as appropriate) and
> > perhaps solve the per axis case if anyone ever cares about it.
> >  
> 
> Well, I think here we need to distinguish:
> Activity: would allow per axis enables and events indicate per axis activity
> Inactivity: allows per axis enables, but only a generic inactivity indication

Ah. I had it in my head it was only one set of per axis enables for the two
types of event. It's not! So indeed your description is what it should be.

> 
> So, also here, what's still missing? When doing it similarly  to my
> understanding of freefall now, for a v7 of the patches...
> 
> Activity:
> - I would leave activity as is (is this ok?)

I think so given the separate enables.

> 
> Inactivity:
> - I replace single three axis enables by a generic enable, setting and
> unsetting all three axis for inactivity
> - I need probably also to provide a similar virtual channel

Is it the same one?  I think so but maybe I've lost track.

> - The axis for this channel are AND'ed
> - Now, with the virtual channel, usage will be "separate" instead of
> "shared", which will result in a single enable handle in sysfs
> 
> Is this a correct understanding of what is +/- missing? Can you agree
> to the points I listed up, or is something's missing (documentation of
> course later)?
Looks good to me!

Jonathan

Re: [PATCH v6 09/11] iio: accel: adxl345: add inactivity feature

[Lothar Rubusch]

On Mon, Apr 21, 2025 at 3:54 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon, 21 Apr 2025 15:39:33 +0200
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > On Mon, Apr 21, 2025 at 12:22 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > On Mon, 21 Apr 2025 00:12:17 +0200
> > > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> > >
> > > > Happy Easter (again)!
> > > >
> > > > On Fri, Apr 18, 2025 at 8:34 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > > > >
> > > > > On Mon, 14 Apr 2025 18:42:43 +0000
> > > > > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> > > > >
> > > > > > Add the inactivity feature of the sensor. When activity and inactivity
> > > > > > are enabled, a link bit will be set linking activity and inactivity
> > > > > > handling. Additionally, the auto-sleep mode will be enabled. Due to the
> > > > > > link bit the sensor is going to auto-sleep when inactivity was
> > > > > > detected.
> > > > > >
> > > > > > Inactivity detection needs a threshold to be configured, and a time
> > > > > > after which it will go into inactivity state if measurements under
> > > > > > threshold.
> > > > > >
> > > > > > When a ODR is configured this time for inactivity is adjusted with a
> > > > > > corresponding reasonable default value, in order to have higher
> > > > > > frequencies and lower inactivity times, and lower sample frequency but
> > > > > > give more time until inactivity. Both with reasonable upper and lower
> > > > > > boundaries, since many of the sensor's features (e.g. auto-sleep) will
> > > > > > need to operate beween 12.5 Hz and 400 Hz. This is a default setting
> > > > > > when actively changing sample frequency, explicitly setting the time
> > > > > > until inactivity will overwrite the default.
> > > > > >
> > > > > > Similarly, setting the g-range will provide a default value for the
> > > > > > activity and inactivity thresholds. Both are implicit defaults, but
> > > > > > equally can be overwritten to be explicitly configured.
> > > > > >
> > > > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > > > Hi Lothar,
> > > > >
> > > > > Patches 6-8 look good to me.
> > > > >
> > > > > This runs into a similar issue to the freefall one. I haven't dug into
> > > > > the datasheet but does it report on one channel going inactive, or
> > > > > all being inactive at the same time?  I checked and it is the all
> > > > > case so we should be both on a pseudo channel to describe it right
> > > > > and reporting IIO_MOD_X_AND_Y_AND_Z not the OR form.
> > > > >
> > > > > Sorry again that I'm only realising this on v6 :(
> > > >
> > > > No problem at all! Sure, I'm still in this phase where counting every
> > > > single commit upstream makes my ego greater. On the long run, though,
> > > > I guess it's better to build up knowledge and end up with a decent
> > > > implementation quality, than just increasing a commit counter. For me
> > > > it's fine. I also hope it's not too annoying for you.
> > > >
> > > > >
> > > > > Difference is for Activity the definition is:
> > > > > "The activity bit is set when acceleration greater than the value
> > > > > stored in the THRESH_ACT register (Address 0x24) is experienced
> > > > > on _any_ participating axis, set by the ACT_INACT_CTL register
> > > > > (Address 0x27)."
> > > > > vs Inactivity:
> > > > > "The inactivity bit is set when acceleration of less than the value
> > > > > stored in the THRESH_INACT register (Address 0x25) is experienced
> > > > > for more time than is specified in the TIME_INACT
> > > > > register (Address 0x26) on _all_ participating axes, as set by the
> > > > > ACT_INACT_CTL register (Address 0x27). "
> > > > >
> > > > > So all vs any.
> > > > >
> > > >
> > > > I think I  see your point. At least I change here for inactivity, too,
> > > > to AND'ed axis.
> > > >
> > > > IMHO, if I set OR here, the first axis raising the inactivity will put
> > > > the sensor to sleep mode,
> > > > where AND needs all three axis in inactivity state. I'm not sure if
> > > > this works out, I need to verify
> > > > it still with the hardware, for now I'll change this to AND.
> > >
> > > I'd be surprised if it worked differently but indeed good to check!
> > >
> > > >
> > > > > > +
> > > > > > +/**
> > > > > > + * adxl345_set_inact_time_s - Configure inactivity time explicitly or by ODR.
> > > > > > + * @st: The sensor state instance.
> > > > > > + * @val_s: A desired time value, between 0 and 255.
> > > > > > + *
> > > > > > + * Inactivity time can be configured between 1 and 255 sec. If a val_s of 0
> > > > > > + * is configured by a user, then a default inactivity time will be computed.
> > > > > > + *
> > > > > > + * In such case, it should take power consumption into consideration. Thus it
> > > > > > + * shall be shorter for higher frequencies and longer for lower frequencies.
> > > > > > + * Hence, frequencies above 255 Hz shall default to 10 s and frequencies below
> > > > > > + * 10 Hz shall result in 255 s to detect inactivity.
> > > > > > + *
> > > > > > + * The approach simply subtracts the pre-decimal figure of the configured
> > > > > > + * sample frequency from 255 s to compute inactivity time [s]. Sub-Hz are thus
> > > > > > + * ignored in this estimation. The recommended ODRs for various features
> > > > > > + * (activity/inactivity, sleep modes, free fall, etc.) lie between 12.5 Hz and
> > > > > > + * 400 Hz, thus higher or lower frequencies will result in the boundary
> > > > > > + * defaults or need to be explicitly specified via val_s.
> > > > > > + *
> > > > > > + * Return: 0 or error value.
> > > > > > + */
> > > > > > +static int adxl345_set_inact_time_s(struct adxl345_state *st, u32 val_s)
> > > > > > +{
> > > > > > +     unsigned int max_boundary = 255;
> > > > > > +     unsigned int min_boundary = 10;
> > > > > > +     unsigned int val = min(val_s, max_boundary);
> > > > > > +     enum adxl345_odr odr;
> > > > > > +     unsigned int regval;
> > > > > > +     int ret;
> > > > > > +
> > > > > > +     if (val == 0) {
> > > > > > +             ret = regmap_read(st->regmap, ADXL345_REG_BW_RATE, &regval);
> > > > > > +             if (ret)
> > > > > > +                     return ret;
> > > > > > +             odr = FIELD_GET(ADXL345_BW_RATE_MSK, regval);
> > > > > > +
> > > > > > +             val = (adxl345_odr_tbl[odr][0] > max_boundary)
> > > > > > +                     ? min_boundary : max_boundary - adxl345_odr_tbl[odr][0];
> > > > > > +     }
> > > > > > +
> > > > > > +     return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
> > > > > >  }
> > > > > >
> > > > > >  /* tap */
> > > > > > @@ -837,6 +943,13 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
> > > > > >                       if (ret)
> > > > > >                               return ret;
> > > > > >                       return int_en;
> > > > > > +             case IIO_EV_DIR_FALLING:
> > > > > > +                     ret = adxl345_is_act_inact_en(st, chan->channel2,
> > > > >
> > > > > Does it makes sense to allow inactivity detection on a subset of channels but then
> > > > > report it as XYZ?  I guess it didn't matter when it was and OR, but if we
> > > > > change to AND as suggested that is going to be misleading.
> > > > >
> > > > > we might have to allow separate enables but report an event as the combination
> > > > > of channels that are enabled X_AND_Y, X_AND_Z etc  I guess we can improve activity
> > > > > channel case as well by doing that with the X_OR_Y etc
> > > > >
> > > >
> > > > Well, initially I guess I only had one enable for inactivity.
> > > >
> > > > This was kind of confusing to me. There is a register to enable
> > > > activity and inactivity on a per axis base [ACT_INACT_CTL, 0x27].
> > >
> > > Agreed this is a slightly odd concept.
> > >
> > > >
> > > > The interrupt event will set a single bit for inactivity or activity
> > > > [INT_SOURCE, 0x30]. In the interrupt handler further one can read out
> > > > the [ACT_TAP_STATUS, 0x2B], which contains tap and activity
> > > > directions, but no information about inactivity axis.
> > > >
> > > > In summary, for the ADXL345 inactivity can be configured on a per axis
> > > > base, but the event won't tell about the axis that fell into
> > > > inactivity, i.e. the first inactivity is supposed to put the sensor
> > > > into power save (with link bit and power modes set - I think
> > > > inactivity should mainly be seen in the context of their/Analog's
> > > > power save concept). As said before, initially I only provided a
> > > > single "inactivity enable". Then I saw actually I could set and offer
> > > > this per axis. I don't know if there are use cases only to observe
> > > > particularly the x-axis for a general power save. Probably rather not.
> > > >
> > > > So, I agree. But if you don't tell me explicitely to replace per axis
> > > > enables by a single one, I'll probably leave it as is. It implements
> > > > most transparently what the sensor can offer for configuration.
> > >
> > > The snag is what I mentioned for freefall. It becomes very hard to indicate
> > > to userspace what it might expect for the x&y&z cases.  If inactivity requires
> > > them all to be inactive, I think separate enables is going to be really
> > > tricky to build a consistent ABI around :(
> > >
> > > Some devices we've had in the past have allowed specific configuration of
> > > and / or for axis combinations. For those we've normally kept clear because
> > > the number of combinations gets sill quickly.
> > >
> > > If we don't have a separate channel enable usecase today I think we should
> > > go ahead with general inactivity / activity (and/or as appropriate) and
> > > perhaps solve the per axis case if anyone ever cares about it.
> > >
> >
> > Well, I think here we need to distinguish:
> > Activity: would allow per axis enables and events indicate per axis activity
> > Inactivity: allows per axis enables, but only a generic inactivity indication
>
> Ah. I had it in my head it was only one set of per axis enables for the two
> types of event. It's not! So indeed your description is what it should be.
>
> >
> > So, also here, what's still missing? When doing it similarly  to my
> > understanding of freefall now, for a v7 of the patches...
> >
> > Activity:
> > - I would leave activity as is (is this ok?)
>
> I think so given the separate enables.
>
> >
> > Inactivity:
> > - I replace single three axis enables by a generic enable, setting and
> > unsetting all three axis for inactivity
> > - I need probably also to provide a similar virtual channel
>
> Is it the same one?  I think so but maybe I've lost track.
>
> > - The axis for this channel are AND'ed
> > - Now, with the virtual channel, usage will be "separate" instead of
> > "shared", which will result in a single enable handle in sysfs
> >
> > Is this a correct understanding of what is +/- missing? Can you agree
> > to the points I listed up, or is something's missing (documentation of
> > course later)?
> Looks good to me!
>

I fixed a v7 together. Eventually, I added a virtual channel for
inactivity and another one for freefall. Pls, let me know if this is
ok. So far it seems to work out perfectly. Now with the '...x&y&z...'
sysfs handle for enabling them. The docs are updated as well.

Best,
L

> Jonathan
>

Re: [PATCH v6 04/11] iio: accel: adxl345: set the tap suppress bit permanently

[oryakerbay]


Sent from my iPhone