[PATCH v3 00/15] iio: accel: adxl345: add interrupt based sensor events

[PATCH v3 00/15] 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>
---
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 (15):
  iio: accel: adxl345: reorganize measurement enable
  iio: accel: adxl345: add debug register access
  iio: accel: adxl345: reorganize irq handler
  iio: accel: adxl345: use regmap cache for INT mapping
  iio: accel: adxl345: move INT enable to regmap cache
  iio: accel: adxl345: add single tap feature
  iio: accel: adxl345: add double tap feature
  iio: accel: adxl345: add double tap suppress bit
  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    |  406 +++++++++
 drivers/iio/accel/adxl345.h      |    7 +-
 drivers/iio/accel/adxl345_core.c | 1449 +++++++++++++++++++++++++++---
 drivers/iio/accel/adxl345_i2c.c  |    2 +
 drivers/iio/accel/adxl345_spi.c  |    2 +
 5 files changed, 1762 insertions(+), 104 deletions(-)
 create mode 100644 Documentation/iio/adxl345.rst

-- 
2.39.5

[PATCH v3 01/15] iio: accel: adxl345: reorganize measurement enable

[Lothar Rubusch]

Move the measurement enable function up in order to have it generically
available.

This is a preparation for upcomming patches. Particular features need
to have measuring off while changing settings, and turned on again
afterwards.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index ed0291bea0f5..c24692c51893 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -76,6 +76,26 @@ static const unsigned long adxl345_scan_masks[] = {
 	0
 };
 
+/**
+ * adxl345_set_measure_en() - Enable and disable measuring.
+ *
+ * @st: The device data.
+ * @en: Enable measurements, else standby mode.
+ *
+ * For lowest power operation, standby mode can be used. In standby mode,
+ * current consumption is supposed to be reduced to 0.1uA (typical). In this
+ * mode no measurements are made. Placing the device into standby mode
+ * preserves the contents of FIFO.
+ *
+ * Return: Returns 0 if successful, or a negative error value.
+ */
+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);
+}
+
 static int adxl345_set_interrupts(struct adxl345_state *st)
 {
 	int ret;
@@ -214,26 +234,6 @@ static int adxl345_write_raw_get_fmt(struct iio_dev *indio_dev,
 	}
 }
 
-/**
- * adxl345_set_measure_en() - Enable and disable measuring.
- *
- * @st: The device data.
- * @en: Enable measurements, else standby mode.
- *
- * For lowest power operation, standby mode can be used. In standby mode,
- * current consumption is supposed to be reduced to 0.1uA (typical). In this
- * mode no measurements are made. Placing the device into standby mode
- * preserves the contents of FIFO.
- *
- * Return: Returns 0 if successful, or a negative error value.
- */
-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);
-}
-
 static void adxl345_powerdown(void *ptr)
 {
 	struct adxl345_state *st = ptr;
-- 
2.39.5

[PATCH v3 02/15] iio: accel: adxl345: add debug register access

[Lothar Rubusch]

Add the possibility to verify the content of the configuration
registers of the sensor in preparation for upcomming feature
implementations.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index c24692c51893..468d562de227 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -202,6 +202,16 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
 	return -EINVAL;
 }
 
+static int adxl345_reg_access(struct iio_dev *indio_dev, unsigned int reg,
+			      unsigned int writeval, unsigned int *readval)
+{
+	struct adxl345_state *st = iio_priv(indio_dev);
+
+	if (readval)
+		return regmap_read(st->regmap, reg, readval);
+	return regmap_write(st->regmap, reg, writeval);
+}
+
 static int adxl345_set_watermark(struct iio_dev *indio_dev, unsigned int value)
 {
 	struct adxl345_state *st = iio_priv(indio_dev);
@@ -467,6 +477,7 @@ 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,
+	.debugfs_reg_access = &adxl345_reg_access,
 	.hwfifo_set_watermark = adxl345_set_watermark,
 };
 
-- 
2.39.5

[PATCH v3 03/15] iio: accel: adxl345: reorganize irq handler

[Lothar Rubusch]

Reorganize the IRQ handler. Move the overrun handling to the bottom.
Overrun leads to reset the interrupt register. This also happens at
evaluation of a particular interrupt event. First evaluate an event
if possible, then fall back to overrun handling. Additionally simplify
fetching the interrupt status function.

Both is in preparation to build interrupt handling up for the handling
of different detected events, implemented in follow up patches.

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

diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
index 517e494ba555..bc6d634bd85c 100644
--- a/drivers/iio/accel/adxl345.h
+++ b/drivers/iio/accel/adxl345.h
@@ -43,7 +43,6 @@
 #define ADXL345_REG_INT_ENABLE		0x2E
 #define ADXL345_REG_INT_MAP		0x2F
 #define ADXL345_REG_INT_SOURCE		0x30
-#define ADXL345_REG_INT_SOURCE_MSK	0xFF
 #define ADXL345_REG_DATA_FORMAT		0x31
 #define ADXL345_REG_XYZ_BASE		0x32
 #define ADXL345_REG_DATA_AXIS(index)				\
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 468d562de227..22c5a9c08459 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -107,8 +107,7 @@ static int adxl345_set_interrupts(struct adxl345_state *st)
 	 * interrupts to the INT1 pin, whereas bits set to 1 send their respective
 	 * interrupts to the INT2 pin. The intio shall convert this accordingly.
 	 */
-	int_map = FIELD_GET(ADXL345_REG_INT_SOURCE_MSK,
-			    st->intio ? st->int_map : ~st->int_map);
+	int_map = st->intio ? st->int_map : ~st->int_map;
 
 	ret = regmap_write(st->regmap, ADXL345_REG_INT_MAP, int_map);
 	if (ret)
@@ -404,18 +403,6 @@ static const struct iio_buffer_setup_ops adxl345_buffer_ops = {
 	.predisable = adxl345_buffer_predisable,
 };
 
-static int adxl345_get_status(struct adxl345_state *st)
-{
-	int ret;
-	unsigned int regval;
-
-	ret = regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, &regval);
-	if (ret < 0)
-		return ret;
-
-	return FIELD_GET(ADXL345_REG_INT_SOURCE_MSK, regval);
-}
-
 static int adxl345_fifo_push(struct iio_dev *indio_dev,
 			     int samples)
 {
@@ -449,14 +436,10 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
 	int int_stat;
 	int samples;
 
-	int_stat = adxl345_get_status(st);
-	if (int_stat <= 0)
+	if (regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, &int_stat))
 		return IRQ_NONE;
 
-	if (int_stat & ADXL345_INT_OVERRUN)
-		goto err;
-
-	if (int_stat & ADXL345_INT_WATERMARK) {
+	if (FIELD_GET(ADXL345_INT_WATERMARK, int_stat)) {
 		samples = adxl345_get_samples(st);
 		if (samples < 0)
 			goto err;
@@ -464,6 +447,10 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
 		if (adxl345_fifo_push(indio_dev, samples) < 0)
 			goto err;
 	}
+
+	if (FIELD_GET(ADXL345_INT_OVERRUN, int_stat))
+		goto err;
+
 	return IRQ_HANDLED;
 
 err:
-- 
2.39.5

[PATCH v3 04/15] iio: accel: adxl345: use regmap cache for INT mapping

[Lothar Rubusch]

Use regmap cache to replace maintaining the member variable intio
for the interrupt mapping state. The interrupt mapping is initialized
when the driver is probed, and it is perfectly cacheable.

The patch will still leave the function set_interrupts(). A follow up
patch takes care of it, when cleaning up the INT enable register
variable.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 drivers/iio/accel/adxl345.h      |  4 ++
 drivers/iio/accel/adxl345_core.c | 63 ++++++++++++++++++++------------
 drivers/iio/accel/adxl345_i2c.c  |  2 +
 drivers/iio/accel/adxl345_spi.c  |  2 +
 4 files changed, 48 insertions(+), 23 deletions(-)

diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
index bc6d634bd85c..a2a81caa292a 100644
--- a/drivers/iio/accel/adxl345.h
+++ b/drivers/iio/accel/adxl345.h
@@ -8,6 +8,8 @@
 #ifndef _ADXL345_H_
 #define _ADXL345_H_
 
+#include <linux/regmap.h>
+
 #define ADXL345_REG_DEVID		0x00
 #define ADXL345_REG_THRESH_TAP		0x1D
 #define ADXL345_REG_OFSX		0x1E
@@ -111,6 +113,8 @@
  */
 #define ADXL375_USCALE	480000
 
+bool adxl345_is_volatile_reg(struct device *dev, unsigned int reg);
+
 struct adxl345_chip_info {
 	const char *name;
 	int uscale;
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 22c5a9c08459..c4cff74a5d10 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -36,7 +36,6 @@ struct adxl345_state {
 	struct regmap *regmap;
 	bool fifo_delay; /* delay: delay is needed for SPI */
 	int irq;
-	u8 intio;
 	u8 int_map;
 	u8 watermark;
 	u8 fifo_mode;
@@ -76,6 +75,24 @@ static const unsigned long adxl345_scan_masks[] = {
 	0
 };
 
+bool adxl345_is_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case ADXL345_REG_DATA_AXIS(0):
+	case ADXL345_REG_DATA_AXIS(1):
+	case ADXL345_REG_DATA_AXIS(2):
+	case ADXL345_REG_DATA_AXIS(3):
+	case ADXL345_REG_DATA_AXIS(4):
+	case ADXL345_REG_DATA_AXIS(5):
+	case ADXL345_REG_FIFO_STATUS:
+	case ADXL345_REG_INT_SOURCE:
+		return true;
+	default:
+		return false;
+	}
+}
+EXPORT_SYMBOL_NS_GPL(adxl345_is_volatile_reg, IIO_ADXL345);
+
 /**
  * adxl345_set_measure_en() - Enable and disable measuring.
  *
@@ -98,22 +115,7 @@ static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
 
 static int adxl345_set_interrupts(struct adxl345_state *st)
 {
-	int ret;
-	unsigned int int_enable = st->int_map;
-	unsigned int int_map;
-
-	/*
-	 * Any bits set to 0 in the INT map register send their respective
-	 * interrupts to the INT1 pin, whereas bits set to 1 send their respective
-	 * interrupts to the INT2 pin. The intio shall convert this accordingly.
-	 */
-	int_map = st->intio ? st->int_map : ~st->int_map;
-
-	ret = regmap_write(st->regmap, ADXL345_REG_INT_MAP, int_map);
-	if (ret)
-		return ret;
-
-	return regmap_write(st->regmap, ADXL345_REG_INT_ENABLE, int_enable);
+	return regmap_write(st->regmap, ADXL345_REG_INT_ENABLE, st->int_map);
 }
 
 static int adxl345_read_raw(struct iio_dev *indio_dev,
@@ -265,6 +267,7 @@ static const struct attribute_group adxl345_attrs_group = {
 
 static int adxl345_set_fifo(struct adxl345_state *st)
 {
+	unsigned int intio;
 	int ret;
 
 	/* FIFO should only be configured while in standby mode */
@@ -272,11 +275,14 @@ static int adxl345_set_fifo(struct adxl345_state *st)
 	if (ret < 0)
 		return ret;
 
+	ret = regmap_read(st->regmap, ADXL345_REG_INT_MAP, &intio);
+	if (ret)
+		return ret;
+
 	ret = regmap_write(st->regmap, ADXL345_REG_FIFO_CTL,
 			   FIELD_PREP(ADXL345_FIFO_CTL_SAMPLES_MSK,
 				      st->watermark) |
-			   FIELD_PREP(ADXL345_FIFO_CTL_TRIGGER_MSK,
-				      st->intio) |
+			   FIELD_PREP(ADXL345_FIFO_CTL_TRIGGER_MSK, intio) |
 			   FIELD_PREP(ADXL345_FIFO_CTL_MODE_MSK,
 				      st->fifo_mode));
 	if (ret < 0)
@@ -492,6 +498,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 	struct adxl345_state *st;
 	struct iio_dev *indio_dev;
 	u32 regval;
+	u8 intio = ADXL345_INT1;
 	unsigned int data_format_mask = (ADXL345_DATA_FORMAT_RANGE |
 					 ADXL345_DATA_FORMAT_JUSTIFY |
 					 ADXL345_DATA_FORMAT_FULL_RES |
@@ -556,16 +563,26 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 	if (ret < 0)
 		return ret;
 
-	st->intio = ADXL345_INT1;
 	st->irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT1");
 	if (st->irq < 0) {
-		st->intio = ADXL345_INT2;
+		intio = ADXL345_INT2;
 		st->irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT2");
 		if (st->irq < 0)
-			st->intio = ADXL345_INT_NONE;
+			intio = ADXL345_INT_NONE;
 	}
 
-	if (st->intio != ADXL345_INT_NONE) {
+	if (intio != ADXL345_INT_NONE) {
+		/*
+		 * Any bits set to 0 in the INT map register send their respective
+		 * interrupts to the INT1 pin, whereas bits set to 1 send their respective
+		 * interrupts to the INT2 pin. The intio shall convert this accordingly.
+		 */
+		regval = intio ? 0xff : 0;
+
+		ret = regmap_write(st->regmap, ADXL345_REG_INT_MAP, regval);
+		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)
diff --git a/drivers/iio/accel/adxl345_i2c.c b/drivers/iio/accel/adxl345_i2c.c
index eb3e0aadf51d..6ce567fd3ba6 100644
--- a/drivers/iio/accel/adxl345_i2c.c
+++ b/drivers/iio/accel/adxl345_i2c.c
@@ -17,6 +17,8 @@
 static const struct regmap_config adxl345_i2c_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
+	.volatile_reg = adxl345_is_volatile_reg,
+	.cache_type = REGCACHE_MAPLE,
 };
 
 static int adxl345_i2c_probe(struct i2c_client *client)
diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl345_spi.c
index e03915ece8b6..200bdf975518 100644
--- a/drivers/iio/accel/adxl345_spi.c
+++ b/drivers/iio/accel/adxl345_spi.c
@@ -19,6 +19,8 @@ static const struct regmap_config adxl345_spi_regmap_config = {
 	.val_bits = 8,
 	 /* Setting bits 7 and 6 enables multiple-byte read */
 	.read_flag_mask = BIT(7) | BIT(6),
+	.volatile_reg = adxl345_is_volatile_reg,
+	.cache_type = REGCACHE_MAPLE,
 };
 
 static int adxl345_spi_setup(struct device *dev, struct regmap *regmap)
-- 
2.39.5

[PATCH v3 05/15] iio: accel: adxl345: move INT enable to regmap cache

[Lothar Rubusch]

Replace the interrupt enable member variable to the regmap cache. This
makes the function set_interrupts() obsolete. The interrupt enable
register is written when the driver is probed. Thus it is perfectly
cacheable.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index c4cff74a5d10..0cee81bc1877 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -36,7 +36,6 @@ struct adxl345_state {
 	struct regmap *regmap;
 	bool fifo_delay; /* delay: delay is needed for SPI */
 	int irq;
-	u8 int_map;
 	u8 watermark;
 	u8 fifo_mode;
 	__le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE + 1] __aligned(IIO_DMA_MINALIGN);
@@ -113,11 +112,6 @@ static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
 	return regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
 }
 
-static int adxl345_set_interrupts(struct adxl345_state *st)
-{
-	return regmap_write(st->regmap, ADXL345_REG_INT_ENABLE, st->int_map);
-}
-
 static int adxl345_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan,
 			    int *val, int *val2, long mask)
@@ -216,7 +210,7 @@ static int adxl345_reg_access(struct iio_dev *indio_dev, unsigned int reg,
 static int adxl345_set_watermark(struct iio_dev *indio_dev, unsigned int value)
 {
 	struct adxl345_state *st = iio_priv(indio_dev);
-	unsigned int fifo_mask = 0x1F;
+	const unsigned int fifo_mask = 0x1F, watermark_mask = 0x02;
 	int ret;
 
 	value = min(value, ADXL345_FIFO_SIZE - 1);
@@ -226,7 +220,10 @@ static int adxl345_set_watermark(struct iio_dev *indio_dev, unsigned int value)
 		return ret;
 
 	st->watermark = value;
-	st->int_map |= ADXL345_INT_WATERMARK;
+	ret = regmap_update_bits(st->regmap, ADXL345_REG_INT_ENABLE, watermark_mask,
+				 ADXL345_INT_WATERMARK);
+	if (ret)
+		return ret;
 
 	return 0;
 }
@@ -380,11 +377,6 @@ static void adxl345_fifo_reset(struct adxl345_state *st)
 static int adxl345_buffer_postenable(struct iio_dev *indio_dev)
 {
 	struct adxl345_state *st = iio_priv(indio_dev);
-	int ret;
-
-	ret = adxl345_set_interrupts(st);
-	if (ret < 0)
-		return ret;
 
 	st->fifo_mode = ADXL345_FIFO_STREAM;
 	return adxl345_set_fifo(st);
@@ -400,8 +392,7 @@ static int adxl345_buffer_predisable(struct iio_dev *indio_dev)
 	if (ret < 0)
 		return ret;
 
-	st->int_map = 0x00;
-	return adxl345_set_interrupts(st);
+	return regmap_write(st->regmap, ADXL345_REG_INT_ENABLE, 0x00);
 }
 
 static const struct iio_buffer_setup_ops adxl345_buffer_ops = {
@@ -523,6 +514,11 @@ 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;
 
+	/* Reset interrupts at start up */
+	ret = regmap_write(st->regmap, ADXL345_REG_INT_ENABLE, 0x00);
+	if (ret)
+		return ret;
+
 	if (setup) {
 		/* Perform optional initial bus specific configuration */
 		ret = setup(dev, st->regmap);
-- 
2.39.5

[PATCH v3 06/15] 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 | 364 ++++++++++++++++++++++++++++++-
 1 file changed, 362 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 0cee81bc1877..d05593c0d513 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,33 @@
 #define ADXL345_INT1			0
 #define ADXL345_INT2			1
 
+#define ADXL345_REG_TAP_AXIS_MSK	GENMASK(2, 0)
+
+enum adxl345_axis {
+	ADXL345_Z_EN = BIT(0),
+	ADXL345_Y_EN = BIT(1),
+	ADXL345_X_EN = BIT(2),
+	/* Suppress double tap detection if value > tap threshold */
+	ADXL345_TAP_SUPPRESS = BIT(3),
+};
+
+/* 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 +67,23 @@ struct adxl345_state {
 	int irq;
 	u8 watermark;
 	u8 fifo_mode;
+
+	u32 tap_axis_ctrl;
+	u32 tap_duration_us;
+
 	__le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE + 1] __aligned(IIO_DMA_MINALIGN);
 };
 
+static struct iio_event_spec adxl345_events[] = {
+	{
+		.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 +100,8 @@ struct adxl345_state {
 		.storagebits = 16,			\
 		.endianness = IIO_LE,			\
 	},						\
+	.event_spec = adxl345_events,			\
+	.num_event_specs = ARRAY_SIZE(adxl345_events),	\
 }
 
 enum adxl345_chans {
@@ -83,6 +128,7 @@ bool adxl345_is_volatile_reg(struct device *dev, unsigned int reg)
 	case ADXL345_REG_DATA_AXIS(3):
 	case ADXL345_REG_DATA_AXIS(4):
 	case ADXL345_REG_DATA_AXIS(5):
+	case ADXL345_REG_ACT_TAP_STATUS:
 	case ADXL345_REG_FIFO_STATUS:
 	case ADXL345_REG_INT_SOURCE:
 		return true;
@@ -112,6 +158,117 @@ 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_write_tap_axis(struct adxl345_state *st,
+				  enum adxl345_axis axis, bool en)
+{
+	st->tap_axis_ctrl = FIELD_GET(ADXL345_REG_TAP_AXIS_MSK,
+				      en ? st->tap_axis_ctrl | axis
+				      : st->tap_axis_ctrl & ~axis);
+
+	return regmap_update_bits(st->regmap, ADXL345_REG_TAP_AXIS,
+				  ADXL345_REG_TAP_AXIS_MSK,
+				  FIELD_PREP(ADXL345_REG_TAP_AXIS_MSK,
+					     st->tap_axis_ctrl));
+}
+
+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;
+	int ret;
+
+	axis_valid = FIELD_GET(ADXL345_REG_TAP_AXIS_MSK, st->tap_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 adxl345_tap_type type, bool *en)
+{
+	int ret;
+	unsigned int regval;
+
+	ret = regmap_read(st->regmap, ADXL345_REG_INT_ENABLE, &regval);
+	if (ret)
+		return ret;
+
+	*en = (adxl345_tap_int_reg[type] & regval) > 0;
+
+	return 0;
+}
+
+static int adxl345_set_singletap_en(struct adxl345_state *st,
+				    enum adxl345_axis axis, bool en)
+{
+	int ret;
+
+	ret = adxl345_write_tap_axis(st, axis, en);
+	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)
@@ -197,6 +354,160 @@ 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;
+	bool axis_en;
+	int ret = -EFAULT;
+
+	switch (type) {
+	case IIO_EV_TYPE_GESTURE:
+		switch (chan->channel2) {
+		case IIO_MOD_X:
+			axis_en = FIELD_GET(ADXL345_X_EN, st->tap_axis_ctrl);
+			break;
+		case IIO_MOD_Y:
+			axis_en = FIELD_GET(ADXL345_Y_EN, st->tap_axis_ctrl);
+			break;
+		case IIO_MOD_Z:
+			axis_en = FIELD_GET(ADXL345_Z_EN, st->tap_axis_ctrl);
+			break;
+		default:
+			axis_en = ADXL345_TAP_SUPPRESS;
+			break;
+		}
+
+		switch (dir) {
+		case IIO_EV_DIR_SINGLETAP:
+			ret = adxl345_is_tap_en(st, ADXL345_SINGLE_TAP, &int_en);
+			if (ret)
+				return ret;
+			return int_en && axis_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,
+				      int state)
+{
+	struct adxl345_state *st = iio_priv(indio_dev);
+	enum adxl345_axis axis;
+
+	switch (type) {
+	case IIO_EV_TYPE_GESTURE:
+		switch (chan->channel2) {
+		case IIO_MOD_X:
+			axis = ADXL345_X_EN;
+			break;
+		case IIO_MOD_Y:
+			axis = ADXL345_Y_EN;
+			break;
+		case IIO_MOD_Z:
+			axis = ADXL345_Z_EN;
+			break;
+		default:
+			axis = ADXL345_TAP_SUPPRESS;
+			break;
+		}
+
+		switch (dir) {
+		case IIO_EV_DIR_SINGLETAP:
+			return adxl345_set_singletap_en(st, axis, 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 is 62.5mg/LSB (i.e. 0xFF = 16g) but
+			 * not applied here.
+			 */
+			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));
+			break;
+		case IIO_EV_INFO_TIMEOUT:
+			ret = adxl345_set_tap_duration(st, val, val2);
+			break;
+		default:
+			ret = -EINVAL;
+			break;
+		}
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	if (ret)
+		return ret; /* measurement stays off */
+
+	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)
 {
@@ -419,6 +730,26 @@ 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,
+			      enum iio_modifier act_tap_dir)
+{
+	s64 ts = iio_get_time_ns(indio_dev);
+	int ret;
+
+	if (FIELD_GET(ADXL345_INT_SINGLE_TAP, int_stat)) {
+		ret = iio_push_event(indio_dev,
+				     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
+							act_tap_dir,
+							IIO_EV_TYPE_GESTURE,
+							IIO_EV_DIR_SINGLETAP),
+				     ts);
+		if (ret)
+			return ret;
+	}
+
+	return -ENOENT;
+}
+
 /**
  * adxl345_irq_handler() - Handle irqs of the ADXL345.
  * @irq: The irq being handled.
@@ -430,12 +761,28 @@ 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;
+	unsigned int regval;
+	enum iio_modifier act_tap_dir  = IIO_NO_MOD;
+	int int_stat, samples, ret;
+
+	if (FIELD_GET(ADXL345_REG_TAP_AXIS_MSK, st->tap_axis_ctrl) > 0) {
+		ret = regmap_read(st->regmap, ADXL345_REG_ACT_TAP_STATUS, &regval);
+		if (ret)
+			return ret;
+		if (FIELD_GET(ADXL345_Z_EN, regval) > 0)
+			act_tap_dir = IIO_MOD_Z;
+		else if (FIELD_GET(ADXL345_Y_EN, regval) > 0)
+			act_tap_dir = IIO_MOD_Y;
+		else if (FIELD_GET(ADXL345_X_EN, regval) > 0)
+			act_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, act_tap_dir) == 0)
+		return IRQ_HANDLED;
+
 	if (FIELD_GET(ADXL345_INT_WATERMARK, int_stat)) {
 		samples = adxl345_get_samples(st);
 		if (samples < 0)
@@ -461,6 +808,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,
 };
@@ -494,6 +845,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));
@@ -507,6 +859,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;
@@ -579,6 +935,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 v3 07/15] 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 | 99 +++++++++++++++++++++++++++++++-
 1 file changed, 98 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index d05593c0d513..c014bdf84e66 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -46,17 +46,23 @@ enum adxl345_axis {
 /* 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,
 };
 
@@ -70,6 +76,8 @@ struct adxl345_state {
 
 	u32 tap_axis_ctrl;
 	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);
 };
@@ -82,6 +90,13 @@ static struct iio_event_spec adxl345_events[] = {
 		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
 			BIT(IIO_EV_INFO_TIMEOUT),
 	},
+	{
+		.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) {					\
@@ -180,6 +195,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;
 	int ret;
 
@@ -195,8 +211,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];
@@ -232,12 +256,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;
@@ -269,6 +304,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)
@@ -387,6 +450,11 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
 			if (ret)
 				return ret;
 			return int_en && axis_en;
+		case IIO_EV_DIR_DOUBLETAP:
+			ret = adxl345_is_tap_en(st, ADXL345_DOUBLE_TAP, &int_en);
+			if (ret)
+				return ret;
+			return int_en;
 		default:
 			return -EINVAL;
 		}
@@ -424,6 +492,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, axis, state);
+		case IIO_EV_DIR_DOUBLETAP:
+			return adxl345_set_doubletap_en(st, state);
 		default:
 			return -EINVAL;
 		}
@@ -460,6 +530,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;
 		}
@@ -492,6 +570,12 @@ static int adxl345_write_event_value(struct iio_dev *indio_dev,
 		case IIO_EV_INFO_TIMEOUT:
 			ret = adxl345_set_tap_duration(st, val, val2);
 			break;
+		case IIO_EV_INFO_RESET_TIMEOUT:
+			ret = adxl345_set_tap_window(st, val, val2);
+			break;
+		case IIO_EV_INFO_TAP2_MIN_DELAY:
+			ret = adxl345_set_tap_latent(st, val, val2);
+			break;
 		default:
 			ret = -EINVAL;
 			break;
@@ -747,6 +831,17 @@ 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,
+							act_tap_dir,
+							IIO_EV_TYPE_GESTURE,
+							IIO_EV_DIR_DOUBLETAP),
+				     ts);
+		if (ret)
+			return ret;
+	}
+
 	return -ENOENT;
 }
 
@@ -862,6 +957,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 v3 08/15] iio: accel: adxl345: add double tap suppress bit

[Lothar Rubusch]

Set the suppress bit feature to the double tap detection, whenever
double tap is enabled.

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 | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index c014bdf84e66..10cd81dd08bb 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -34,6 +34,7 @@
 #define ADXL345_INT2			1
 
 #define ADXL345_REG_TAP_AXIS_MSK	GENMASK(2, 0)
+#define ADXL345_REG_TAP_SUPPRESS_MSK	BIT(3)
 
 enum adxl345_axis {
 	ADXL345_Z_EN = BIT(0),
@@ -258,6 +259,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_TAP_SUPPRESS : 0x00);
+	if (ret)
+		return ret;
+
 	return _adxl345_set_tap_int(st, ADXL345_DOUBLE_TAP, en);
 }
 
-- 
2.39.5

[PATCH v3 09/15] 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 | 122 +++++++++++++++++++++++++++++++
 1 file changed, 122 insertions(+)

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 10cd81dd08bb..7f842e7f371a 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -79,6 +79,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);
 };
@@ -98,6 +99,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) {					\
@@ -345,6 +354,64 @@ 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;
+	const unsigned int freefall_mask = 0x02;
+	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,
+				  freefall_mask, 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)
@@ -471,6 +538,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;
 	}
@@ -510,6 +582,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;
 	}
@@ -524,6 +598,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) {
@@ -554,6 +629,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;
 	}
@@ -594,6 +685,18 @@ static int adxl345_write_event_value(struct iio_dev *indio_dev,
 			break;
 		}
 		break;
+	case IIO_EV_TYPE_MAG:
+		switch (info) {
+		case IIO_EV_INFO_VALUE:
+			ret = regmap_write(st->regmap, ADXL345_REG_THRESH_FF, val);
+			break;
+		case IIO_EV_INFO_PERIOD:
+			ret = adxl345_set_ff_time(st, val, val2);
+			break;
+		default:
+			ret = -EINVAL;
+		}
+		break;
 	default:
 		ret = -EINVAL;
 		break;
@@ -855,6 +958,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;
+	}
+
 	return -ENOENT;
 }
 
@@ -954,6 +1068,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));
@@ -973,6 +1088,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;
@@ -1049,6 +1167,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 v3 10/15] 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. The patch is in
preparation for activity/inactivity 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 | 160 ++++++++++++++++++++++++-------
 2 files changed, 126 insertions(+), 36 deletions(-)

diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
index a2a81caa292a..56db8f8ba032 100644
--- a/drivers/iio/accel/adxl345.h
+++ b/drivers/iio/accel/adxl345.h
@@ -71,7 +71,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 7f842e7f371a..fa169cac5c05 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -67,6 +67,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;
@@ -118,6 +157,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',				\
@@ -412,14 +452,61 @@ 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])
+			break;
+
+	if (i == ARRAY_SIZE(adxl345_odr_tbl))
+		return -EINVAL;
+
+	*odr = i;
+
+	return 0;
+}
+
+static int adxl345_set_odr(struct adxl345_state *st, enum adxl345_odr odr)
+{
+	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;
+
+	return 0;
+}
+
+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) {
@@ -455,14 +542,12 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		ret = regmap_read(st->regmap, ADXL345_REG_BW_RATE, &regval);
-		if (ret < 0)
+		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;
@@ -473,7 +558,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:
@@ -481,20 +571,24 @@ 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);
+		break;
 	case IIO_CHAN_INFO_SAMP_FREQ:
-		n = div_s64(val * NANOHZ_PER_HZ + val2,
-			    ADXL345_BASE_RATE_NANO_HZ);
-
-		return regmap_update_bits(st->regmap, ADXL345_REG_BW_RATE,
-					  ADXL345_BW_RATE,
-					  clamp_val(ilog2(n), 0,
-						    ADXL345_BW_RATE));
+		ret = adxl345_find_odr(st, val, val2, &odr);
+		if (ret)
+			return ret;
+		ret = adxl345_set_odr(st, odr);
+		break;
+	default:
+		return -EINVAL;
 	}
 
-	return -EINVAL;
+	if (ret)
+		return ret;
+
+	return adxl345_set_measure_en(st, true);
 }
 
 static int adxl345_read_event_config(struct iio_dev *indio_dev,
@@ -747,7 +841,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;
 	}
@@ -760,19 +854,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;
@@ -1026,9 +1107,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,
@@ -1098,6 +1179,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 v3 11/15] iio: accel: adxl345: add g-range configuration

[Lothar Rubusch]

Introduce means to configure and work with the available g-ranges
keeping the precision of 13 digits.

This is in preparation for the activity/inactivity feature.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index fa169cac5c05..ab1ab09f348a 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -86,6 +86,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 },
@@ -106,6 +113,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;
@@ -157,7 +191,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',				\
@@ -483,12 +518,48 @@ static int adxl345_set_odr(struct adxl345_state *st, enum adxl345_odr odr)
 	return 0;
 }
 
+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])
+			break;
+
+	if (i == ARRAY_SIZE(adxl345_fullres_range_tbl))
+		return -EINVAL;
+
+	*range = i;
+
+	return 0;
+}
+
+static int adxl345_set_range(struct adxl345_state *st, enum adxl345_range range)
+{
+	int 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;
+
+	return 0;
+}
+
 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;
@@ -507,6 +578,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) {
@@ -525,8 +597,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,
@@ -558,6 +634,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;
 
@@ -581,6 +658,12 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
 			return ret;
 		ret = adxl345_set_odr(st, odr);
 		break;
+	case IIO_CHAN_INFO_SCALE:
+		ret = adxl345_find_range(st, val, val2,	&range);
+		if (ret)
+			return ret;
+		ret = adxl345_set_range(st, range);
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -840,6 +923,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:
-- 
2.39.5

[PATCH v3 12/15] 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.

Activity, together with ODR and range setting are preparing a setup
together with inactivity coming in a follow up patch.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index ab1ab09f348a..f1895925a80b 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -35,6 +35,7 @@
 
 #define ADXL345_REG_TAP_AXIS_MSK	GENMASK(2, 0)
 #define ADXL345_REG_TAP_SUPPRESS_MSK	BIT(3)
+#define ADXL345_REG_ACT_AXIS_MSK	GENMASK(6, 4)
 
 enum adxl345_axis {
 	ADXL345_Z_EN = BIT(0),
@@ -67,6 +68,23 @@ 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,
+};
+
+static const unsigned int adxl345_act_axis_msk[] = {
+	[ADXL345_ACTIVITY] = ADXL345_REG_ACT_AXIS_MSK,
+};
+
 enum adxl345_odr {
 	ADXL345_ODR_0P10HZ = 0,
 	ADXL345_ODR_0P20HZ,
@@ -148,6 +166,7 @@ struct adxl345_state {
 	u8 watermark;
 	u8 fifo_mode;
 
+	u32 act_axis_ctrl;
 	u32 tap_axis_ctrl;
 	u32 tap_duration_us;
 	u32 tap_latent_us;
@@ -158,6 +177,13 @@ struct adxl345_state {
 };
 
 static struct iio_event_spec adxl345_events[] = {
+	{
+		/* activity */
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE) |
+			BIT(IIO_EV_INFO_VALUE),
+	},
 	{
 		.type = IIO_EV_TYPE_GESTURE,
 		.dir = IIO_EV_DIR_SINGLETAP,
@@ -258,6 +284,73 @@ 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_write_act_axis(struct adxl345_state *st,
+				  enum adxl345_activity_type type, bool en)
+{
+	int ret;
+
+	/*
+	 * The ADXL345 allows for individually enabling/disabling axis for
+	 * activity and inactivity detection, respectively. Here both axis are
+	 * kept in sync, i.e. an axis will be generally enabled or disabled for
+	 * both equally, activity and inactivity detection.
+	 */
+	if (type == ADXL345_ACTIVITY) {
+		st->act_axis_ctrl = en
+			? st->act_axis_ctrl | ADXL345_REG_ACT_AXIS_MSK
+			: st->act_axis_ctrl & ~ADXL345_REG_ACT_AXIS_MSK;
+
+		ret = regmap_update_bits(st->regmap, ADXL345_REG_ACT_INACT_CTRL,
+					 adxl345_act_axis_msk[type],
+					 st->act_axis_ctrl);
+		if (ret)
+			return ret;
+	}
+	return 0;
+}
+
+static int adxl345_is_act_inact_en(struct adxl345_state *st,
+				   enum adxl345_activity_type type, bool *en)
+{
+	int ret;
+	unsigned int regval;
+
+	ret = regmap_read(st->regmap, ADXL345_REG_INT_ENABLE, &regval);
+	if (ret)
+		return ret;
+
+	*en = (adxl345_act_int_reg[type] & regval) > 0;
+
+	return 0;
+}
+
+static int adxl345_set_act_inact_en(struct adxl345_state *st,
+				    enum adxl345_activity_type type, bool cmd_en)
+{
+	bool axis_en, en = false;
+	unsigned int threshold;
+	int ret;
+
+	ret = adxl345_write_act_axis(st, type, cmd_en);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(st->regmap, adxl345_act_thresh_reg[type], &threshold);
+	if (ret)
+		return ret;
+
+	if (type == ADXL345_ACTIVITY) {
+		axis_en = FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, st->act_axis_ctrl) > 0;
+		en = axis_en && threshold > 0;
+	}
+
+	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_write_tap_axis(struct adxl345_state *st,
@@ -685,6 +778,16 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
 	int ret = -EFAULT;
 
 	switch (type) {
+	case IIO_EV_TYPE_THRESH:
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			ret = adxl345_is_act_inact_en(st, ADXL345_ACTIVITY, &int_en);
+			if (ret)
+				return ret;
+			return int_en;
+		default:
+			return -EINVAL;
+		}
 	case IIO_EV_TYPE_GESTURE:
 		switch (chan->channel2) {
 		case IIO_MOD_X:
@@ -735,6 +838,13 @@ static int adxl345_write_event_config(struct iio_dev *indio_dev,
 	enum adxl345_axis axis;
 
 	switch (type) {
+	case IIO_EV_TYPE_THRESH:
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			return adxl345_set_act_inact_en(st, ADXL345_ACTIVITY, state);
+		default:
+			return -EINVAL;
+		}
 	case IIO_EV_TYPE_GESTURE:
 		switch (chan->channel2) {
 		case IIO_MOD_X:
@@ -776,9 +886,29 @@ 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;
+	unsigned int act_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:
@@ -842,6 +972,23 @@ 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);
+				break;
+			default:
+				ret = -EINVAL;
+			}
+			break;
+		default:
+			ret = -EINVAL;
+		}
+		break;
 	case IIO_EV_TYPE_GESTURE:
 		switch (info) {
 		case IIO_EV_INFO_VALUE:
@@ -1124,6 +1271,17 @@ 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_tap_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,
@@ -1157,6 +1315,7 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
 		ret = regmap_read(st->regmap, ADXL345_REG_ACT_TAP_STATUS, &regval);
 		if (ret)
 			return ret;
+		/* tap direction */
 		if (FIELD_GET(ADXL345_Z_EN, regval) > 0)
 			act_tap_dir = IIO_MOD_Z;
 		else if (FIELD_GET(ADXL345_Y_EN, regval) > 0)
@@ -1165,6 +1324,19 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
 			act_tap_dir = IIO_MOD_X;
 	}
 
+	if (FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, st->act_axis_ctrl) > 0) {
+		ret = regmap_read(st->regmap, ADXL345_REG_ACT_TAP_STATUS, &regval);
+		if (ret)
+			return ret;
+		/* activity direction */
+		if (FIELD_GET(ADXL345_Z_EN, regval >> 4) > 0)
+			act_tap_dir = IIO_MOD_Z;
+		else if (FIELD_GET(ADXL345_Y_EN, regval >> 4) > 0)
+			act_tap_dir = IIO_MOD_Y;
+		else if (FIELD_GET(ADXL345_X_EN, regval >> 4) > 0)
+			act_tap_dir = IIO_MOD_X;
+	}
+
 	if (regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, &int_stat))
 		return IRQ_NONE;
 
@@ -1248,6 +1420,13 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 		return -ENODEV;
 	st->fifo_delay = fifo_delay_default;
 
+	/*
+	 * If the feature is enabled, scan all axis for activity and or
+	 * inactivity, and set activity and inactivity to the same ac / dc
+	 * setup.
+	 */
+	st->act_axis_ctrl = ADXL345_REG_ACT_AXIS_MSK;
+
 	/* Init with reasonable values */
 	tap_threshold = 48;			/*   48 [0x30] -> ~3g     */
 	st->tap_duration_us = 16;		/*   16 [0x10] -> .010    */
@@ -1273,6 +1452,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 v3 13/15] 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 | 140 +++++++++++++++++++++++++++++--
 1 file changed, 135 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index f1895925a80b..40ec06bf2090 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -36,6 +36,8 @@
 #define ADXL345_REG_TAP_AXIS_MSK	GENMASK(2, 0)
 #define ADXL345_REG_TAP_SUPPRESS_MSK	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)
 
 enum adxl345_axis {
 	ADXL345_Z_EN = BIT(0),
@@ -71,18 +73,22 @@ 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,
 };
 
 static const unsigned int adxl345_act_axis_msk[] = {
 	[ADXL345_ACTIVITY] = ADXL345_REG_ACT_AXIS_MSK,
+	[ADXL345_INACTIVITY] = ADXL345_REG_INACT_AXIS_MSK,
 };
 
 enum adxl345_odr {
@@ -167,6 +173,7 @@ struct adxl345_state {
 	u8 fifo_mode;
 
 	u32 act_axis_ctrl;
+	u32 inact_axis_ctrl;
 	u32 tap_axis_ctrl;
 	u32 tap_duration_us;
 	u32 tap_latent_us;
@@ -184,6 +191,14 @@ static struct iio_event_spec adxl345_events[] = {
 		.mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE) |
 			BIT(IIO_EV_INFO_VALUE),
 	},
+	{
+		/* inactivity */
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE) |
+			BIT(IIO_EV_INFO_VALUE) |
+			BIT(IIO_EV_INFO_PERIOD),
+	},
 	{
 		.type = IIO_EV_TYPE_GESTURE,
 		.dir = IIO_EV_DIR_SINGLETAP,
@@ -307,6 +322,17 @@ static int adxl345_write_act_axis(struct adxl345_state *st,
 					 st->act_axis_ctrl);
 		if (ret)
 			return ret;
+
+	} else {
+		st->inact_axis_ctrl = en
+			? st->inact_axis_ctrl | ADXL345_REG_INACT_AXIS_MSK
+			: st->inact_axis_ctrl & ~ADXL345_REG_INACT_AXIS_MSK;
+
+		ret = regmap_update_bits(st->regmap, ADXL345_REG_ACT_INACT_CTRL,
+					 adxl345_act_axis_msk[type],
+					 st->inact_axis_ctrl);
+		if (ret)
+			return ret;
 	}
 	return 0;
 }
@@ -330,6 +356,7 @@ static int adxl345_set_act_inact_en(struct adxl345_state *st,
 				    enum adxl345_activity_type type, bool cmd_en)
 {
 	bool axis_en, en = false;
+	unsigned int inact_time_s;
 	unsigned int threshold;
 	int ret;
 
@@ -344,11 +371,71 @@ static int adxl345_set_act_inact_en(struct adxl345_state *st,
 	if (type == ADXL345_ACTIVITY) {
 		axis_en = FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, st->act_axis_ctrl) > 0;
 		en = axis_en && threshold > 0;
+	} else {
+		ret = regmap_read(st->regmap, ADXL345_REG_TIME_INACT, &inact_time_s);
+		if (ret)
+			return ret;
+
+		axis_en = FIELD_GET(ADXL345_REG_INACT_AXIS_MSK, st->inact_axis_ctrl) > 0;
+		en = axis_en && threshold > 0 && inact_time_s > 0;
 	}
 
-	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.
+ *
+ * If val_s is 0, a default inactivity time will be computed. 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];
+	}
+
+	ret = regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
+	if (ret)
+		return ret;
+
+	return 0;
 }
 
 /* tap */
@@ -785,6 +872,11 @@ 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, ADXL345_INACTIVITY, &int_en);
+			if (ret)
+				return ret;
+			return int_en;
 		default:
 			return -EINVAL;
 		}
@@ -842,6 +934,8 @@ static int adxl345_write_event_config(struct iio_dev *indio_dev,
 		switch (dir) {
 		case IIO_EV_DIR_RISING:
 			return adxl345_set_act_inact_en(st, ADXL345_ACTIVITY, state);
+		case IIO_EV_DIR_FALLING:
+			return adxl345_set_act_inact_en(st, ADXL345_INACTIVITY, state);
 		default:
 			return -EINVAL;
 		}
@@ -884,9 +978,10 @@ 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 tap_threshold;
+	unsigned int act_threshold, inact_threshold;
+	unsigned int inact_time_s;
 	unsigned int ff_threshold;
-	unsigned int act_threshold;
+	unsigned int tap_threshold;
 	int ret;
 
 	switch (type) {
@@ -903,9 +998,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;
 		}
@@ -981,10 +1091,18 @@ static int adxl345_write_event_value(struct iio_dev *indio_dev,
 						   adxl345_act_thresh_reg[ADXL345_ACTIVITY],
 						   val);
 				break;
+			case IIO_EV_DIR_FALLING:
+				ret = regmap_write(st->regmap,
+						   adxl345_act_thresh_reg[ADXL345_INACTIVITY],
+						   val);
+				break;
 			default:
 				ret = -EINVAL;
 			}
 			break;
+		case IIO_EV_INFO_PERIOD:
+			ret = adxl345_set_inact_time_s(st, val);
+			break;
 		default:
 			ret = -EINVAL;
 		}
@@ -1282,6 +1400,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,
@@ -1426,6 +1555,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 	 * setup.
 	 */
 	st->act_axis_ctrl = ADXL345_REG_ACT_AXIS_MSK;
+	st->inact_axis_ctrl = ADXL345_REG_INACT_AXIS_MSK;
 
 	/* Init with reasonable values */
 	tap_threshold = 48;			/*   48 [0x30] -> ~3g     */
-- 
2.39.5

[PATCH v3 14/15] 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 | 154 +++++++++++++++++++++++++++++++
 1 file changed, 154 insertions(+)

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 40ec06bf2090..7ace835e7824 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -36,7 +36,9 @@
 #define ADXL345_REG_TAP_AXIS_MSK	GENMASK(2, 0)
 #define ADXL345_REG_TAP_SUPPRESS_MSK	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)
 
 enum adxl345_axis {
@@ -86,6 +88,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,
+};
+
 static const unsigned int adxl345_act_axis_msk[] = {
 	[ADXL345_ACTIVITY] = ADXL345_REG_ACT_AXIS_MSK,
 	[ADXL345_INACTIVITY] = ADXL345_REG_INACT_AXIS_MSK,
@@ -221,6 +228,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) {					\
@@ -337,6 +356,69 @@ static int adxl345_write_act_axis(struct adxl345_state *st,
 	return 0;
 }
 
+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) > 0) ? true : false;
+	else
+		*ac = (FIELD_GET(ADXL345_REG_INACT_ACDC_MSK, regval) > 0) ? true : false;
+
+	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 0 selects dc-coupled operation, and a setting of 1
+	 * 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 adxl345_activity_type type, bool *en)
 {
@@ -695,6 +777,11 @@ static int adxl345_set_odr(struct adxl345_state *st, enum adxl345_odr odr)
 	if (ret)
 		return ret;
 
+	/* update inactivity time by ODR */
+	ret = adxl345_set_inact_time_s(st, 0);
+	if (ret)
+		return ret;
+
 	return 0;
 }
 
@@ -718,14 +805,54 @@ 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)
 {
+	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;
+
+	ret = regmap_write(st->regmap, adxl345_act_thresh_reg[ADXL345_INACTIVITY],
+			   inact_threshold);
+	if (ret)
+		return ret;
+
 	return 0;
 }
 
@@ -862,6 +989,8 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
 	struct adxl345_state *st = iio_priv(indio_dev);
 	bool int_en;
 	bool axis_en;
+	bool act_ac;
+	bool inact_ac;
 	int ret = -EFAULT;
 
 	switch (type) {
@@ -915,6 +1044,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;
 	}
@@ -965,6 +1109,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 v3 15/15] 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 | 406 ++++++++++++++++++++++++++++++++++
 1 file changed, 406 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..b77c97ef52e5
--- /dev/null
+++ b/Documentation/iio/adxl345.rst
@@ -0,0 +1,406 @@
+.. 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
+====================
+
+Accelerometer measurements are always provided.
+
+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_thresh_falling_en                  | Enable inactivity detection             |
++---------------------------------------------+-----------------------------------------+
+| in_accel_thresh_falling_period              | Inactivity time in seconds              |
++---------------------------------------------+-----------------------------------------+
+| in_accel_thresh_falling_value               | Inactivity threshold value in 62.5/LSB  |
++---------------------------------------------+-----------------------------------------+
+| in_accel_thresh_rising_en                   | Enable activity detection               |
++---------------------------------------------+-----------------------------------------+
+| 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 both 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.
+
+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.
+
+A double tap event can be invalidated in three ways: If the suppress bit is set,
+any acceleration spike above the threshold already during the latency time
+invalidates the double tap detection immediately, i.e. during latence must not
+occur spikes for double tap detection with suppress bit set.
+A double tap event is invalidated if acceleration lies above the threshold at
+the start of the window time for the double tap detection.
+Additionally a double tap event can be invalidated if an acceleration exceeds
+the time limit for taps, set by duration register, since also for the double
+tap the same value for duration counts, i.e. when rising above threshold the
+fall below threshold has to be within duration time. 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
+        ...
+
+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 v3 03/15] iio: accel: adxl345: reorganize irq handler

[Jonathan Cameron]

On Thu, 20 Feb 2025 10:42:22 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Reorganize the IRQ handler. Move the overrun handling to the bottom.
> Overrun leads to reset the interrupt register. This also happens at
> evaluation of a particular interrupt event. First evaluate an event
> if possible, then fall back to overrun handling. Additionally simplify
> fetching the interrupt status function.
> 
> Both is in preparation to build interrupt handling up for the handling
> of different detected events, implemented in follow up patches.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
Hi Lothar,

In the interests of reducing size of patch set I've applied 1-3 to
the to greg branch of iio.git.  Pushed out as testing for all
the normal reasons.

Thanks,

Jonathan

Re: [PATCH v3 04/15] iio: accel: adxl345: use regmap cache for INT mapping

[Jonathan Cameron]

On Thu, 20 Feb 2025 10:42:23 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Use regmap cache to replace maintaining the member variable intio
> for the interrupt mapping state. The interrupt mapping is initialized
> when the driver is probed, and it is perfectly cacheable.
> 
> The patch will still leave the function set_interrupts(). A follow up
> patch takes care of it, when cleaning up the INT enable register
> variable.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl345.h      |  4 ++
>  drivers/iio/accel/adxl345_core.c | 63 ++++++++++++++++++++------------
>  drivers/iio/accel/adxl345_i2c.c  |  2 +
>  drivers/iio/accel/adxl345_spi.c  |  2 +
>  4 files changed, 48 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> index bc6d634bd85c..a2a81caa292a 100644
> --- a/drivers/iio/accel/adxl345.h
> +++ b/drivers/iio/accel/adxl345.h
> @@ -8,6 +8,8 @@
>  #ifndef _ADXL345_H_
>  #define _ADXL345_H_
>  
> +#include <linux/regmap.h>

Why add this include?

The file should have a forwards def of
struct regmap;
which is currently missing.  If you clean that up in this patch that
is fine (mention it in the patch description though as it isn't
directly related) but I don't see a reason to include regmap.h here.

Given rest if fine I'll tweak this whilst applying. Applied to the
togreg branch of iio.git, pushed out for now as testing for 0-day
to poke at it.

Also move to a newer kernel tree. The changes in export symbol
should be causing you build errors for this path. I'll fix that up.
Quotes now needed around IIO_ADXL345 in the EXPORT_SYMBOL_NS_GPL()
calls. I fixed that up.

Jonathan

Re: [PATCH v3 05/15] iio: accel: adxl345: move INT enable to regmap cache

[Jonathan Cameron]

On Thu, 20 Feb 2025 10:42:24 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Replace the interrupt enable member variable to the regmap cache. This
> makes the function set_interrupts() obsolete. The interrupt enable
> register is written when the driver is probed. Thus it is perfectly
> cacheable.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
Applied.

Re: [PATCH v3 04/15] iio: accel: adxl345: use regmap cache for INT mapping

[Jonathan Cameron]

On Sun, 2 Mar 2025 11:45:03 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Thu, 20 Feb 2025 10:42:23 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
> 
> > Use regmap cache to replace maintaining the member variable intio
> > for the interrupt mapping state. The interrupt mapping is initialized
> > when the driver is probed, and it is perfectly cacheable.
> > 
> > The patch will still leave the function set_interrupts(). A follow up
> > patch takes care of it, when cleaning up the INT enable register
> > variable.
> > 
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > ---
> >  drivers/iio/accel/adxl345.h      |  4 ++
> >  drivers/iio/accel/adxl345_core.c | 63 ++++++++++++++++++++------------
> >  drivers/iio/accel/adxl345_i2c.c  |  2 +
> >  drivers/iio/accel/adxl345_spi.c  |  2 +
> >  4 files changed, 48 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> > index bc6d634bd85c..a2a81caa292a 100644
> > --- a/drivers/iio/accel/adxl345.h
> > +++ b/drivers/iio/accel/adxl345.h
> > @@ -8,6 +8,8 @@
> >  #ifndef _ADXL345_H_
> >  #define _ADXL345_H_
> >  
> > +#include <linux/regmap.h>  
> 
> Why add this include?
> 
> The file should have a forwards def of
> struct regmap;
> which is currently missing.  If you clean that up in this patch that
> is fine (mention it in the patch description though as it isn't
> directly related) but I don't see a reason to include regmap.h here.
> 
> Given rest if fine I'll tweak this whilst applying. Applied to the
> togreg branch of iio.git, pushed out for now as testing for 0-day
> to poke at it.
> 
> Also move to a newer kernel tree. The changes in export symbol
> should be causing you build errors for this path. I'll fix that up.
> Quotes now needed around IIO_ADXL345 in the EXPORT_SYMBOL_NS_GPL()
> calls. I fixed that up.
Dropped again after reviewing later patch.  I think the volatile
stuff should specify all registers that are volatile from the start
not add them as we go.
> 
> Jonathan

Re: [PATCH v3 06/15] iio: accel: adxl345: add single tap feature

[Jonathan Cameron]

On Thu, 20 Feb 2025 10:42:25 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> 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 | 364 ++++++++++++++++++++++++++++++-
>  1 file changed, 362 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 0cee81bc1877..d05593c0d513 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,33 @@
>  #define ADXL345_INT1			0
>  #define ADXL345_INT2			1
>  
> +#define ADXL345_REG_TAP_AXIS_MSK	GENMASK(2, 0)
This is a bit confusing.  Here we have a mask for axis that
has 3 bits.
> +
> +enum adxl345_axis {
> +	ADXL345_Z_EN = BIT(0),
> +	ADXL345_Y_EN = BIT(1),
> +	ADXL345_X_EN = BIT(2),
> +	/* Suppress double tap detection if value > tap threshold */
> +	ADXL345_TAP_SUPPRESS = BIT(3),
and here an enum that is closely related with 4.

> +};
...

>  enum adxl345_chans {
> @@ -83,6 +128,7 @@ bool adxl345_is_volatile_reg(struct device *dev, unsigned int reg)
>  	case ADXL345_REG_DATA_AXIS(3):
>  	case ADXL345_REG_DATA_AXIS(4):
>  	case ADXL345_REG_DATA_AXIS(5):
> +	case ADXL345_REG_ACT_TAP_STATUS:

ah.   I thought this had a full list from earlier patches.
Move this and any later additions back to patch 4.

>  	case ADXL345_REG_FIFO_STATUS:
>  	case ADXL345_REG_INT_SOURCE:
>  		return true;
> @@ -112,6 +158,117 @@ 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_write_tap_axis(struct adxl345_state *st,
> +				  enum adxl345_axis axis, bool en)
> +{
> +	st->tap_axis_ctrl = FIELD_GET(ADXL345_REG_TAP_AXIS_MSK,
> +				      en ? st->tap_axis_ctrl | axis
> +				      : st->tap_axis_ctrl & ~axis);
> +
> +	return regmap_update_bits(st->regmap, ADXL345_REG_TAP_AXIS,
> +				  ADXL345_REG_TAP_AXIS_MSK,
> +				  FIELD_PREP(ADXL345_REG_TAP_AXIS_MSK,
> +					     st->tap_axis_ctrl));
Given that mask is bottom few bits anyway and you can just define the
tap axis as separate fields. 

See below, but I would push the IIO_MOD values down into here. Put the switch
per axis in at this level and then use a simple if statement to
call regmap_clear_bits() / regmap_set_bits() based on enable.

> +}

...

> +static int adxl345_is_tap_en(struct adxl345_state *st,
> +			     enum adxl345_tap_type type, bool *en)
> +{
> +	int ret;
> +	unsigned int regval;
> +
> +	ret = regmap_read(st->regmap, ADXL345_REG_INT_ENABLE, &regval);
> +	if (ret)
> +		return ret;
> +
> +	*en = (adxl345_tap_int_reg[type] & regval) > 0;

Could use 0/1 return rather than passing a paramter for the output.
I don't mind much either way.


> +
> +	return 0;
> +}
> +
> +static int adxl345_set_singletap_en(struct adxl345_state *st,
I'd push the IIO modifier in here instead of axis. 
> +				    enum adxl345_axis axis, bool en)
> +{
> +	int ret;
> +
> +	ret = adxl345_write_tap_axis(st, axis, en);
And push it into here as well.

> +	if (ret)
> +		return ret;
> +
> +	return _adxl345_set_tap_int(st, ADXL345_SINGLE_TAP, en);
> +}
> +

...

> @@ -197,6 +354,160 @@ 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;
> +	bool axis_en;
> +	int ret = -EFAULT;
> +
> +	switch (type) {
> +	case IIO_EV_TYPE_GESTURE:
> +		switch (chan->channel2) {
> +		case IIO_MOD_X:
> +			axis_en = FIELD_GET(ADXL345_X_EN, st->tap_axis_ctrl);
> +			break;
> +		case IIO_MOD_Y:
> +			axis_en = FIELD_GET(ADXL345_Y_EN, st->tap_axis_ctrl);
> +			break;
> +		case IIO_MOD_Z:
> +			axis_en = FIELD_GET(ADXL345_Z_EN, st->tap_axis_ctrl);
> +			break;
> +		default:
> +			axis_en = ADXL345_TAP_SUPPRESS;
> +			break;
> +		}
I'd check axis_en here.
		if (!axis_en)
			return false;
> +
> +		switch (dir) {
> +		case IIO_EV_DIR_SINGLETAP:
> +			ret = adxl345_is_tap_en(st, ADXL345_SINGLE_TAP, &int_en);
> +			if (ret)
> +				return ret;
> +			return int_en && axis_en;

Then this just becomes 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,
> +				      int state)
> +{
> +	struct adxl345_state *st = iio_priv(indio_dev);
> +	enum adxl345_axis axis;
> +
> +	switch (type) {
> +	case IIO_EV_TYPE_GESTURE:
> +		switch (chan->channel2) {
> +		case IIO_MOD_X:
> +			axis = ADXL345_X_EN;
> +			break;
> +		case IIO_MOD_Y:
> +			axis = ADXL345_Y_EN;
> +			break;
> +		case IIO_MOD_Z:
> +			axis = ADXL345_Z_EN;
> +			break;
> +		default:
> +			axis = ADXL345_TAP_SUPPRESS;

How are we getting another axis here?

> +			break;
> +		}
> +
> +		switch (dir) {
> +		case IIO_EV_DIR_SINGLETAP:
> +			return adxl345_set_singletap_en(st, axis, state);

As above, pass chan->channel2 into here so we can simplify the eventual
setting of the the register values.

> +		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 is 62.5mg/LSB (i.e. 0xFF = 16g) but
> +			 * not applied here.

Maybe say why.

> +			 */
> +			ret = regmap_read(st->regmap, ADXL345_REG_THRESH_TAP, &tap_threshold);

Bit of a long line. Aim for sub 80 chars unless readability is greatly
hurt.

> +			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;
> +	}
> +}
...

Re: [PATCH v3 08/15] iio: accel: adxl345: add double tap suppress bit

[Jonathan Cameron]

On Thu, 20 Feb 2025 10:42:27 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Set the suppress bit feature to the double tap detection, whenever
> double tap is enabled.
> 
> 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 | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index c014bdf84e66..10cd81dd08bb 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -34,6 +34,7 @@
>  #define ADXL345_INT2			1
>  
>  #define ADXL345_REG_TAP_AXIS_MSK	GENMASK(2, 0)
> +#define ADXL345_REG_TAP_SUPPRESS_MSK	BIT(3)
>  
>  enum adxl345_axis {
>  	ADXL345_Z_EN = BIT(0),
> @@ -258,6 +259,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
Local style has large comments as sentences so capital letter at start and a full stop
at the end preferred.

> +	 * 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_TAP_SUPPRESS : 0x00);
> +	if (ret)
> +		return ret;
> +
>  	return _adxl345_set_tap_int(st, ADXL345_DOUBLE_TAP, en);
>  }
>  

Re: [PATCH v3 09/15] iio: accel: adxl345: add freefall feature

[Jonathan Cameron]

On Thu, 20 Feb 2025 10:42:28 +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.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
Hi, one thing inline.

> @@ -855,6 +958,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;
> +	}
> +

Seems unlikely to be right. Pushed an event without error yet this function
is returning an error here?

>  	return -ENOENT;
>  }
>  
> @@ -954,6 +1068,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));
> @@ -973,6 +1088,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;
> @@ -1049,6 +1167,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)

Re: [PATCH v3 10/15] iio: accel: adxl345: extend sample frequency adjustments

[Jonathan Cameron]

On Thu, 20 Feb 2025 10:42:29 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> 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. The patch is in
> preparation for activity/inactivity handling. During adjustment of
> bw registers, measuring is disabled and afterwards enabled again.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
Hi Lothar, a few minor things inline.


> +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])
> +			break;
> +
> +	if (i == ARRAY_SIZE(adxl345_odr_tbl))
> +		return -EINVAL;
> +
> +	*odr = i;
> +
> +	return 0;
Unless there will be more to do here later it would be fine to
move this setting *odr = i and return into the loop condition.
Then you can return -EINVAL directly if the loop finishes.
i.e.
	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;

This is a very common pattern when there isn't much
to do on a match.

> +}
> +
> +static int adxl345_set_odr(struct adxl345_state *st, enum adxl345_odr odr)
> +{
> +	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;
I guess this makes sense in later patches, but if it doesn't get more
complex
	return regmap()

> +
> +	return 0;
> +}
> +
> +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) {
> @@ -455,14 +542,12 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		ret = regmap_read(st->regmap, ADXL345_REG_BW_RATE, &regval);
> -		if (ret < 0)
> +		if (ret)
>  			return ret;

Change is reasonable but unrelated to this patch that I can see. Should
really be a separate patch cleaning these up for all regmap calls.
I have no idea if there are others.

> -
> -		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;
> @@ -473,7 +558,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:
> @@ -481,20 +571,24 @@ 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);
I'd do local error handling here...
> +		break;
>  	case IIO_CHAN_INFO_SAMP_FREQ:
> -		n = div_s64(val * NANOHZ_PER_HZ + val2,
> -			    ADXL345_BASE_RATE_NANO_HZ);
> -
> -		return regmap_update_bits(st->regmap, ADXL345_REG_BW_RATE,
> -					  ADXL345_BW_RATE,
> -					  clamp_val(ilog2(n), 0,
> -						    ADXL345_BW_RATE));
> +		ret = adxl345_find_odr(st, val, val2, &odr);
> +		if (ret)
> +			return ret;
> +		ret = adxl345_set_odr(st, odr);
and here just to be consistent with the case above here
		if (ret)
			return ret;
> +		break;
> +	default:
> +		return -EINVAL;
>  	}
>  
> -	return -EINVAL;
> +	if (ret)
This this one is never hit.

> +		return ret;
> +
> +	return adxl345_set_measure_en(st, true);
>  }
>  
>  static int adxl345_read_event_config(struct iio_dev *indio_dev,
> @@ -747,7 +841,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;
>  	}
> @@ -760,19 +854,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"
> -);

Re: [PATCH v3 11/15] iio: accel: adxl345: add g-range configuration

[Jonathan Cameron]

On Thu, 20 Feb 2025 10:42:30 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Introduce means to configure and work with the available g-ranges
> keeping the precision of 13 digits.
> 
> This is in preparation for the activity/inactivity feature.

I'm not really following why adding range control is anything
much to do with that. Mostly we do this to improve accuracy for
low accelerations.

> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>


> @@ -483,12 +518,48 @@ static int adxl345_set_odr(struct adxl345_state *st, enum adxl345_odr odr)
>  	return 0;
>  }
>  
> +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])
> +			break;
Similar to case in earlier patch, maybe set *range and return in here
so that any finish of the loop is an error.
> +
> +	if (i == ARRAY_SIZE(adxl345_fullres_range_tbl))
> +		return -EINVAL;
> +
> +	*range = i;
> +
> +	return 0;
> +}
> +
> +static int adxl345_set_range(struct adxl345_state *st, enum adxl345_range range)
> +{
> +	int 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;
> +

return regmap_update_bits() unless this gets more complex in later patch.

> +	return 0;
> +}
> +

> @@ -558,6 +634,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;
>  
> @@ -581,6 +658,12 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
>  			return ret;
>  		ret = adxl345_set_odr(st, odr);
>  		break;
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = adxl345_find_range(st, val, val2,	&range);
> +		if (ret)
> +			return ret;
> +		ret = adxl345_set_range(st, range);
as in previous I'd have the 	
		if (ret)
			return ret;
here for consistency with the other one immediately above this.
> +		break;
>  	default:
>  		return -EINVAL;
>  	}

Re: [PATCH v3 12/15] iio: accel: adxl345: add activity event feature

[Jonathan Cameron]

On Thu, 20 Feb 2025 10:42:31 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Make the sensor detect and issue interrupts at activity. Activity
> events are configured by a threshold stored in regmap cache.
> 
> Activity, together with ODR and range setting are preparing a setup
> together with inactivity coming in a follow up patch.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
Some questions / comments inline.

This review is has been at random moments whilst travelling (hence
over several days!) so it may be less than thorough or consistent!
I should be back to normal sometime next week.

> @@ -258,6 +284,73 @@ 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_write_act_axis(struct adxl345_state *st,
> +				  enum adxl345_activity_type type, bool en)
> +{
> +	int ret;
> +
> +	/*
> +	 * The ADXL345 allows for individually enabling/disabling axis for
> +	 * activity and inactivity detection, respectively. Here both axis are
> +	 * kept in sync, i.e. an axis will be generally enabled or disabled for
> +	 * both equally, activity and inactivity detection.

Why?  Can definitely see people only being interested in one case
and not the other.  What advantage is there in always having both
or neither over separate controls?

> +	 */
> +	if (type == ADXL345_ACTIVITY) {
> +		st->act_axis_ctrl = en
> +			? st->act_axis_ctrl | ADXL345_REG_ACT_AXIS_MSK
> +			: st->act_axis_ctrl & ~ADXL345_REG_ACT_AXIS_MSK;
> +
> +		ret = regmap_update_bits(st->regmap, ADXL345_REG_ACT_INACT_CTRL,
> +					 adxl345_act_axis_msk[type],
> +					 st->act_axis_ctrl);
> +		if (ret)
> +			return ret;
> +	}
> +	return 0;
> +}


> +static int adxl345_set_act_inact_en(struct adxl345_state *st,
> +				    enum adxl345_activity_type type, bool cmd_en)
> +{
> +	bool axis_en, en = false;
I'm not keen on mix of set and unset in a declaration line.  Better to 
use two lines here as it can be hard to spot when things are or aren't
initialized when that is not the intent.

	bool en = false;
	bool axis_en;

> +	unsigned int threshold;
> +	int ret;
> +
> +	ret = adxl345_write_act_axis(st, type, cmd_en);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(st->regmap, adxl345_act_thresh_reg[type], &threshold);
> +	if (ret)
> +		return ret;
> +
> +	if (type == ADXL345_ACTIVITY) {
> +		axis_en = FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, st->act_axis_ctrl) > 0;
> +		en = axis_en && threshold > 0;
> +	}
> +
> +	return regmap_update_bits(st->regmap, ADXL345_REG_INT_ENABLE,
> +				  adxl345_act_int_reg[type],
> +				  en ? adxl345_act_int_reg[type] : 0);
> +}
> +

> @@ -842,6 +972,23 @@ 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);
> +				break;
This collection of breaks and nested functions suggests maybe we can either
return directly (I've lost track of what happens after this) or that
we should factor out some of this code to allow direct returns in the
function we put that code in.  Chasing the breaks is not great if it
doesn't lead to anything interesting.
> +			default:
> +				ret = -EINVAL;
> +			}
> +			break;
> +		default:
> +			ret = -EINVAL;
> +		}
> +		break;
>  	case IIO_EV_TYPE_GESTURE:
>  		switch (info) {
>  		case IIO_EV_INFO_VALUE:
> @@ -1124,6 +1271,17 @@ 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_tap_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,
> @@ -1157,6 +1315,7 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
>  		ret = regmap_read(st->regmap, ADXL345_REG_ACT_TAP_STATUS, &regval);
>  		if (ret)
>  			return ret;
> +		/* tap direction */

Belongs in earlier patch?

>  		if (FIELD_GET(ADXL345_Z_EN, regval) > 0)
>  			act_tap_dir = IIO_MOD_Z;
>  		else if (FIELD_GET(ADXL345_Y_EN, regval) > 0)
> @@ -1165,6 +1324,19 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
>  			act_tap_dir = IIO_MOD_X;
>  	}
>  
> +	if (FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, st->act_axis_ctrl) > 0) {
> +		ret = regmap_read(st->regmap, ADXL345_REG_ACT_TAP_STATUS, &regval);
> +		if (ret)
> +			return ret;
> +		/* activity direction */
> +		if (FIELD_GET(ADXL345_Z_EN, regval >> 4) > 0)

I'm not following the shifts here.   That looks like we don't have
defines that we should but instead use the ones for the lower fields.

> +			act_tap_dir = IIO_MOD_Z;
> +		else if (FIELD_GET(ADXL345_Y_EN, regval >> 4) > 0)
> +			act_tap_dir = IIO_MOD_Y;
> +		else if (FIELD_GET(ADXL345_X_EN, regval >> 4) > 0)
> +			act_tap_dir = IIO_MOD_X;
> +	}

Re: [PATCH v3 13/15] iio: accel: adxl345: add inactivity feature

[Jonathan Cameron]

On Thu, 20 Feb 2025 10:42:32 +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.
This to me looks like the justification that comment in the previous
patch should mention for why we enable /disable the two together.
> 
> 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>

A few things inline.

> @@ -307,6 +322,17 @@ static int adxl345_write_act_axis(struct adxl345_state *st,
>  					 st->act_axis_ctrl);
>  		if (ret)
>  			return ret;
> +
Probably drop this blank line.
> +	} else {
> +		st->inact_axis_ctrl = en
> +			? st->inact_axis_ctrl | ADXL345_REG_INACT_AXIS_MSK
> +			: st->inact_axis_ctrl & ~ADXL345_REG_INACT_AXIS_MSK;
> +
> +		ret = regmap_update_bits(st->regmap, ADXL345_REG_ACT_INACT_CTRL,
> +					 adxl345_act_axis_msk[type],
> +					 st->inact_axis_ctrl);
> +		if (ret)
> +			return ret;
Maybe it is worth returning in both legs of the if / else?
For this one can just return regmap_update_bits()...

>  	}
>  	return 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.
> + *
> + * If val_s is 0, a default inactivity time will be computed. 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];
> +	}
> +
> +	ret = regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
> +	if (ret)
> +		return ret;

	return regmap_write()

> +
> +	return 0;
>  }

Re: [PATCH v3 14/15] iio: accel: adxl345: add coupling detection for activity/inactivity

[Jonathan Cameron]

On Thu, 20 Feb 2025 10:42:33 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> 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>
> @@ -337,6 +356,69 @@ static int adxl345_write_act_axis(struct adxl345_state *st,
>  	return 0;
>  }
>  
> +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) > 0) ? true : false;
> +	else
> +		*ac = (FIELD_GET(ADXL345_REG_INACT_ACDC_MSK, regval) > 0) ? true : false;
> +
Why the ternaries? 
		*ac = FIELD_GET() > 0;
is a boolean right hand side anyway.

> +	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 0 selects dc-coupled operation, and a setting of 1
false and true rather than 0 / 1?
> +	 * 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.
...
>  static int adxl345_is_act_inact_en(struct adxl345_state *st,
>  				   enum adxl345_activity_type type, bool *en)
>  {
> @@ -695,6 +777,11 @@ static int adxl345_set_odr(struct adxl345_state *st, enum adxl345_odr odr)
>  	if (ret)
>  		return ret;
>  
> +	/* update inactivity time by ODR */
> +	ret = adxl345_set_inact_time_s(st, 0);
> +	if (ret)
> +		return ret;
> +
>  	return 0;

return adxl345_set_inact_time_s()

>  }
>  
> @@ -718,14 +805,54 @@ 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)
>  {
> +	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;
> +
> +	ret = regmap_write(st->regmap, adxl345_act_thresh_reg[ADXL345_INACTIVITY],
> +			   inact_threshold);
> +	if (ret)
> +		return ret;
return regmap_write()
> +
>  	return 0;
>  }

Re: [PATCH v3 15/15] docs: iio: add documentation for adxl345 driver

[Jonathan Cameron]

On Thu, 20 Feb 2025 10:42:34 +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>
> ---
>  Documentation/iio/adxl345.rst | 406 ++++++++++++++++++++++++++++++++++
>  1 file changed, 406 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..b77c97ef52e5
> --- /dev/null
> +++ b/Documentation/iio/adxl345.rst
> @@ -0,0 +1,406 @@
> +.. 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
> +====================
> +
> +Accelerometer measurements are always provided.

I'm not sure what this sentence is telling us. I'd drop it.

...


> +When activity and inactivity events are both 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.
This makes me a little nervous as it's not particularly standard.
However I can't immediately think of a better way to handle it so maybe the
currently assumption of when to enable it is fine.


> +A double tap event can be invalidated in three ways: If the suppress bit is set,
> +any acceleration spike above the threshold already during the latency time
> +invalidates the double tap detection immediately, i.e. during latence must not
latency in the i.e bit? 
> +occur spikes for double tap detection with suppress bit set.

I'm finding the previous sentence hard to understand. Perhaps consider
rewording?

> +A double tap event is invalidated if acceleration lies above the threshold at
> +the start of the window time for the double tap detection.


Thanks,

Jonathan

Re: [PATCH v3 04/15] iio: accel: adxl345: use regmap cache for INT mapping

[Lothar Rubusch]

On Sun, Mar 2, 2025 at 12:45 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu, 20 Feb 2025 10:42:23 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Use regmap cache to replace maintaining the member variable intio
> > for the interrupt mapping state. The interrupt mapping is initialized
> > when the driver is probed, and it is perfectly cacheable.
> >
> > The patch will still leave the function set_interrupts(). A follow up
> > patch takes care of it, when cleaning up the INT enable register
> > variable.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > ---
> >  drivers/iio/accel/adxl345.h      |  4 ++
> >  drivers/iio/accel/adxl345_core.c | 63 ++++++++++++++++++++------------
> >  drivers/iio/accel/adxl345_i2c.c  |  2 +
> >  drivers/iio/accel/adxl345_spi.c  |  2 +
> >  4 files changed, 48 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> > index bc6d634bd85c..a2a81caa292a 100644
> > --- a/drivers/iio/accel/adxl345.h
> > +++ b/drivers/iio/accel/adxl345.h
> > @@ -8,6 +8,8 @@
> >  #ifndef _ADXL345_H_
> >  #define _ADXL345_H_
> >
> > +#include <linux/regmap.h>
>
> Why add this include?
>
> The file should have a forwards def of
> struct regmap;

Sure.

> which is currently missing.  If you clean that up in this patch that
> is fine (mention it in the patch description though as it isn't
> directly related) but I don't see a reason to include regmap.h here.
>
> Given rest if fine I'll tweak this whilst applying. Applied to the
> togreg branch of iio.git, pushed out for now as testing for 0-day
> to poke at it.
>
> Also move to a newer kernel tree. The changes in export symbol
> should be causing you build errors for this path. I'll fix that up.
> Quotes now needed around IIO_ADXL345 in the EXPORT_SYMBOL_NS_GPL()
> calls. I fixed that up.

Yes, this gives/gave errors. I'm sorry, I left this in the patch.

FYI, I'm always patching against recent kernel source of your
"testing" branch of the linux iio repo. Anyway, I'm testing/verifying
on a semi-automized setup here, involving the particular sensor
hardware connected to an RPI. Although the self-compiled pi-kernel is
+/- recent, it usually needs additional tweaks. My pi-kernel still
uses unquoted symbols here, which I already cover by an auxiliary
patch (not supposed to go upstream). I missed to update this patch
here for the added function. Thank you for the hint. Anyway probably a
good idea to update and rebuild my setup pi-kernel. TY

>
> Jonathan

Re: [PATCH v3 12/15] iio: accel: adxl345: add activity event feature

[Lothar Rubusch]

Hi Jonathan,

I'm currently about to update the series and like to answer some of
your review comments directly when submitting. Nevertheless, here I'll
anticipate this one. Pls, find my questions inlined down below.

On Tue, Mar 4, 2025 at 2:49 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu, 20 Feb 2025 10:42:31 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Make the sensor detect and issue interrupts at activity. Activity
> > events are configured by a threshold stored in regmap cache.
> >
> > Activity, together with ODR and range setting are preparing a setup
> > together with inactivity coming in a follow up patch.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> Some questions / comments inline.
>
> This review is has been at random moments whilst travelling (hence
> over several days!) so it may be less than thorough or consistent!
> I should be back to normal sometime next week.
>

No problem, no hurry!

> > @@ -258,6 +284,73 @@ 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_write_act_axis(struct adxl345_state *st,
> > +                               enum adxl345_activity_type type, bool en)
> > +{
> > +     int ret;
> > +
> > +     /*
> > +      * The ADXL345 allows for individually enabling/disabling axis for
> > +      * activity and inactivity detection, respectively. Here both axis are
> > +      * kept in sync, i.e. an axis will be generally enabled or disabled for
> > +      * both equally, activity and inactivity detection.
>
> Why?  Can definitely see people only being interested in one case
> and not the other.  What advantage is there in always having both
> or neither over separate controls?

Ugh! This happens when I mix writing code and writing English texts,
w/o re-reading it.

Situation: The sensor allows to individually enable / disable x,y and
z axis for r activity and for inactivity. I don't offer this in the
driver. When activity is selected, I always enable all three axis or
disable them. Similar, for inactivity. The question is then actually,
if this is legitimate, or should I really implement enabling/disabling
of each axis individually for activity and similar then for
inactivity? I mean, when not interested in one or the other axis,
someone can fiilter the result. On the other side I can imagine a
small impact in power consumption, when only one axis is used instead
of three axis (?). Since the ADXL345 is [was] one of Analog's fancy
acme-ultra-low-power-sensors, power is definitvely a topic here IMHO.
I can't really estimate the importance.

I guess I'll try to implement it and see how ugly it gets. At least
it's a good exercise. As also, I'll try to bring regmap cache and
clear_bits / set_bits more in here for activity and inactivity in the
next version.

>
> > +      */
> > +     if (type == ADXL345_ACTIVITY) {
> > +             st->act_axis_ctrl = en
> > +                     ? st->act_axis_ctrl | ADXL345_REG_ACT_AXIS_MSK
> > +                     : st->act_axis_ctrl & ~ADXL345_REG_ACT_AXIS_MSK;
> > +
> > +             ret = regmap_update_bits(st->regmap, ADXL345_REG_ACT_INACT_CTRL,
> > +                                      adxl345_act_axis_msk[type],
> > +                                      st->act_axis_ctrl);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +     return 0;
> > +}
>
>
> > +static int adxl345_set_act_inact_en(struct adxl345_state *st,
> > +                                 enum adxl345_activity_type type, bool cmd_en)
> > +{
> > +     bool axis_en, en = false;
> I'm not keen on mix of set and unset in a declaration line.  Better to
> use two lines here as it can be hard to spot when things are or aren't
> initialized when that is not the intent.
>
>         bool en = false;
>         bool axis_en;
>
> > +     unsigned int threshold;
> > +     int ret;
> > +
> > +     ret = adxl345_write_act_axis(st, type, cmd_en);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = regmap_read(st->regmap, adxl345_act_thresh_reg[type], &threshold);
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (type == ADXL345_ACTIVITY) {
> > +             axis_en = FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, st->act_axis_ctrl) > 0;
> > +             en = axis_en && threshold > 0;
> > +     }
> > +
> > +     return regmap_update_bits(st->regmap, ADXL345_REG_INT_ENABLE,
> > +                               adxl345_act_int_reg[type],
> > +                               en ? adxl345_act_int_reg[type] : 0);
> > +}
> > +
>
> > @@ -842,6 +972,23 @@ 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);
> > +                             break;
> This collection of breaks and nested functions suggests maybe we can either
> return directly (I've lost track of what happens after this) or that
> we should factor out some of this code to allow direct returns in the
> function we put that code in.  Chasing the breaks is not great if it
> doesn't lead to anything interesting.

I understand, but since I'm using quite a bit configuration for the
sensor, I'm taking advantage
of type, info and dir here. It won't get more complex than that. I'm
[actually] pretty sure, since this
then is almost feature complete.

I don't see a different way how to do it. I mean, I could still
separate handling the "dir" entirely in
a called function. Or, say, implement IIO_EV_TYPE_THRESH handling in a
separate function?
Pls, let me know what you think.

> > +                     default:
> > +                             ret = -EINVAL;
> > +                     }
> > +                     break;
> > +             default:
> > +                     ret = -EINVAL;
> > +             }
> > +             break;
> >       case IIO_EV_TYPE_GESTURE:
> >               switch (info) {
> >               case IIO_EV_INFO_VALUE:
> > @@ -1124,6 +1271,17 @@ 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_tap_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,
> > @@ -1157,6 +1315,7 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
> >               ret = regmap_read(st->regmap, ADXL345_REG_ACT_TAP_STATUS, &regval);
> >               if (ret)
> >                       return ret;
> > +             /* tap direction */
>
> Belongs in earlier patch?
>
> >               if (FIELD_GET(ADXL345_Z_EN, regval) > 0)
> >                       act_tap_dir = IIO_MOD_Z;
> >               else if (FIELD_GET(ADXL345_Y_EN, regval) > 0)
> > @@ -1165,6 +1324,19 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
> >                       act_tap_dir = IIO_MOD_X;
> >       }
> >
> > +     if (FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, st->act_axis_ctrl) > 0) {
> > +             ret = regmap_read(st->regmap, ADXL345_REG_ACT_TAP_STATUS, &regval);
> > +             if (ret)
> > +                     return ret;
> > +             /* activity direction */
> > +             if (FIELD_GET(ADXL345_Z_EN, regval >> 4) > 0)
>
> I'm not following the shifts here.   That looks like we don't have
> defines that we should but instead use the ones for the lower fields.

The 8-bit register is split as follows:

| 7                |  6         |  5          |  4         |  3
          |  2              |  1             |  0             |
----------------------------------------------------------------------------------------------------------------------
| ACT ac/dc | ACT_X | ACT_Y | ACT_Z | INACT ac/dc | INACT_X | INACT_Y
| INACT_Z |

I thought here, either I shift the ACT_* directions by 4 then use the
general mask for axis (lower 4 bits). Or I introduce an axis enum for
ACT_* and a separate one for INACT_*. Thus, I kept the shift and use
the same ADXL345_*_EN mask. How can I improve this, or can this stay?

>
> > +                     act_tap_dir = IIO_MOD_Z;
> > +             else if (FIELD_GET(ADXL345_Y_EN, regval >> 4) > 0)
> > +                     act_tap_dir = IIO_MOD_Y;
> > +             else if (FIELD_GET(ADXL345_X_EN, regval >> 4) > 0)
> > +                     act_tap_dir = IIO_MOD_X;
> > +     }
>
>

Best,
L

Re: [PATCH v3 06/15] iio: accel: adxl345: add single tap feature

[Lothar Rubusch]

Hi Jonathan,

I prepared, reorganized and tested a v4 patch set. Given that I see
how busy you must be these days with current increased mail traffic
just in IIO ML when I compare it to some years ago,
I don't want to bother you too much.
Some particular doubts I will inline down below. If possible with your
work flow and to avoid giving you extra work, I'd like to ask you to
read the questions here, but to give your answers right to the
v4 patch set (so, i.e. after having seen the current state of source).
It also should make my points
a bit clearer. Anyway, this is just my idea, since I'm always happy
about any feedback!

On Sun, Mar 2, 2025 at 1:14 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu, 20 Feb 2025 10:42:25 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > 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 | 364 ++++++++++++++++++++++++++++++-
> >  1 file changed, 362 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > index 0cee81bc1877..d05593c0d513 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,33 @@
> >  #define ADXL345_INT1                 0
> >  #define ADXL345_INT2                 1
> >
> > +#define ADXL345_REG_TAP_AXIS_MSK     GENMASK(2, 0)
> This is a bit confusing.  Here we have a mask for axis that
> has 3 bits.
> > +
> > +enum adxl345_axis {
> > +     ADXL345_Z_EN = BIT(0),
> > +     ADXL345_Y_EN = BIT(1),
> > +     ADXL345_X_EN = BIT(2),
> > +     /* Suppress double tap detection if value > tap threshold */
> > +     ADXL345_TAP_SUPPRESS = BIT(3),
> and here an enum that is closely related with 4.

I see your point. There are several registers used in the sensor for
directions. A status register for tap and activity directions, and a
activity/inactivity direction register. For Tap, direction enables are
stored using the suppress bit in the fourth position. All those
locations use actually four bit. Partly the upper four, partly the
lower four. That's why I use here four bit for reading and writing.
The locations 0, 1, 2 then can be used directly. Location 3 only
applies to tap detection.

I'll keep this in v4 patches, and hope to understand you correctly
that this is not a "real" issue?

>
> > +};
> ...
>
> >  enum adxl345_chans {
> > @@ -83,6 +128,7 @@ bool adxl345_is_volatile_reg(struct device *dev, unsigned int reg)
> >       case ADXL345_REG_DATA_AXIS(3):
> >       case ADXL345_REG_DATA_AXIS(4):
> >       case ADXL345_REG_DATA_AXIS(5):
> > +     case ADXL345_REG_ACT_TAP_STATUS:
>
> ah.   I thought this had a full list from earlier patches.
> Move this and any later additions back to patch 4.
>
> >       case ADXL345_REG_FIFO_STATUS:
> >       case ADXL345_REG_INT_SOURCE:
> >               return true;
> > @@ -112,6 +158,117 @@ 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_write_tap_axis(struct adxl345_state *st,
> > +                               enum adxl345_axis axis, bool en)
> > +{
> > +     st->tap_axis_ctrl = FIELD_GET(ADXL345_REG_TAP_AXIS_MSK,
> > +                                   en ? st->tap_axis_ctrl | axis
> > +                                   : st->tap_axis_ctrl & ~axis);
> > +
> > +     return regmap_update_bits(st->regmap, ADXL345_REG_TAP_AXIS,
> > +                               ADXL345_REG_TAP_AXIS_MSK,
> > +                               FIELD_PREP(ADXL345_REG_TAP_AXIS_MSK,
> > +                                          st->tap_axis_ctrl));
> Given that mask is bottom few bits anyway and you can just define the
> tap axis as separate fields.
>
> See below, but I would push the IIO_MOD values down into here. Put the switch
> per axis in at this level and then use a simple if statement to
> call regmap_clear_bits() / regmap_set_bits() based on enable.
>

Thank you for this hint. I  reimplemented that, though, I was guessing
quite a bit
how you might like to have the source. Please have a look into
upcoming v4 series and let me
know if I got it wrong.

> > +}
>
> ...
>
> > +static int adxl345_is_tap_en(struct adxl345_state *st,
> > +                          enum adxl345_tap_type type, bool *en)
> > +{
> > +     int ret;
> > +     unsigned int regval;
> > +
> > +     ret = regmap_read(st->regmap, ADXL345_REG_INT_ENABLE, &regval);
> > +     if (ret)
> > +             return ret;
> > +
> > +     *en = (adxl345_tap_int_reg[type] & regval) > 0;
>
> Could use 0/1 return rather than passing a paramter for the output.
> I don't mind much either way.
>

Hum. I took this rather as suggestion, and I will keep this as is for
now. I'm a bit unsure, on
the one side I'm using the return value for error handling (return
ret), on the other side there
is the enable. If using enable in the return, this would mix up, but
probably not a big issue.
It feels rather then to me, if the function is really needed, or if I
can put this in another place.
Anyway, I think it could/should be fine like that. Let me know in v4
context, what you think.

>
> > +
> > +     return 0;
> > +}
> > +
> > +static int adxl345_set_singletap_en(struct adxl345_state *st,
> I'd push the IIO modifier in here instead of axis.
> > +                                 enum adxl345_axis axis, bool en)
> > +{
> > +     int ret;
> > +
> > +     ret = adxl345_write_tap_axis(st, axis, en);
> And push it into here as well.
>
> > +     if (ret)
> > +             return ret;
> > +
> > +     return _adxl345_set_tap_int(st, ADXL345_SINGLE_TAP, en);
> > +}
> > +
>
> ...
>
> > @@ -197,6 +354,160 @@ 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;
> > +     bool axis_en;
> > +     int ret = -EFAULT;
> > +
> > +     switch (type) {
> > +     case IIO_EV_TYPE_GESTURE:
> > +             switch (chan->channel2) {
> > +             case IIO_MOD_X:
> > +                     axis_en = FIELD_GET(ADXL345_X_EN, st->tap_axis_ctrl);
> > +                     break;
> > +             case IIO_MOD_Y:
> > +                     axis_en = FIELD_GET(ADXL345_Y_EN, st->tap_axis_ctrl);
> > +                     break;
> > +             case IIO_MOD_Z:
> > +                     axis_en = FIELD_GET(ADXL345_Z_EN, st->tap_axis_ctrl);
> > +                     break;
> > +             default:
> > +                     axis_en = ADXL345_TAP_SUPPRESS;
> > +                     break;
> > +             }
> I'd check axis_en here.
>                 if (!axis_en)
>                         return false;
> > +
> > +             switch (dir) {
> > +             case IIO_EV_DIR_SINGLETAP:
> > +                     ret = adxl345_is_tap_en(st, ADXL345_SINGLE_TAP, &int_en);
> > +                     if (ret)
> > +                             return ret;
> > +                     return int_en && axis_en;
>
> Then this just becomes 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,
> > +                                   int state)
> > +{
> > +     struct adxl345_state *st = iio_priv(indio_dev);
> > +     enum adxl345_axis axis;
> > +
> > +     switch (type) {
> > +     case IIO_EV_TYPE_GESTURE:
> > +             switch (chan->channel2) {
> > +             case IIO_MOD_X:
> > +                     axis = ADXL345_X_EN;
> > +                     break;
> > +             case IIO_MOD_Y:
> > +                     axis = ADXL345_Y_EN;
> > +                     break;
> > +             case IIO_MOD_Z:
> > +                     axis = ADXL345_Z_EN;
> > +                     break;
> > +             default:
> > +                     axis = ADXL345_TAP_SUPPRESS;
>
> How are we getting another axis here?

:)

> > +                     break;
> > +             }
> > +
> > +             switch (dir) {
> > +             case IIO_EV_DIR_SINGLETAP:
> > +                     return adxl345_set_singletap_en(st, axis, state);
>
> As above, pass chan->channel2 into here so we can simplify the eventual
> setting of the the register values.
>
> > +             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 is 62.5mg/LSB (i.e. 0xFF = 16g) but
> > +                      * not applied here.
>
> Maybe say why.
>

Usually I did scaling for the time values. Time values is something I
can understand someone
rather wants to configure in corresponding time units, such as [ms],
[us] or [s] rather than bit
values. For [mg] values, franckly speaking, I imagine this is a bit overkill.

The threshold quite often is rather expect to be higher or lower,
depending a bit on variation of
the measurements. In the context of this rather "cheap" sensor, I
guess I'm not putting up a
seismic instrument, but rather generic tap detection, freefall or
general activity in a general
purpose context such as gaming, or the like. Let me know if this
assumption here is too lazy.

I added a bit more comment to the source, pls have a look into v4.

> > +                      */
> > +                     ret = regmap_read(st->regmap, ADXL345_REG_THRESH_TAP, &tap_threshold);
>
> Bit of a long line. Aim for sub 80 chars unless readability is greatly
> hurt.
>
> > +                     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;
> > +     }
> > +}
> ...
>

Re: [PATCH v3 09/15] iio: accel: adxl345: add freefall feature

[Lothar Rubusch]

On Tue, Mar 4, 2025 at 2:23 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu, 20 Feb 2025 10:42:28 +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.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> Hi, one thing inline.
>
> > @@ -855,6 +958,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;
> > +     }
> > +
>
> Seems unlikely to be right. Pushed an event without error yet this function
> is returning an error here?
>
> >       return -ENOENT;
> >  }
> >

"it worked on my machine" - Of course, you're right. So, I tried to
understand why this "worked". In consequence, I think the best
solution will be to put also fifo handling based on int_stat into this
function, which I currently made a separate patch, you'll see that in
v4.

> > @@ -954,6 +1068,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));
> > @@ -973,6 +1088,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;
> > @@ -1049,6 +1167,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)
>

Re: [PATCH v3 14/15] iio: accel: adxl345: add coupling detection for activity/inactivity

[Lothar Rubusch]

On Tue, Mar 4, 2025 at 2:59 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu, 20 Feb 2025 10:42:33 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > 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>
> > @@ -337,6 +356,69 @@ static int adxl345_write_act_axis(struct adxl345_state *st,
> >       return 0;
> >  }
> >
> > +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) > 0) ? true : false;
> > +     else
> > +             *ac = (FIELD_GET(ADXL345_REG_INACT_ACDC_MSK, regval) > 0) ? true : false;
> > +
> Why the ternaries?
>                 *ac = FIELD_GET() > 0;
> is a boolean right hand side anyway.
>
> > +     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 0 selects dc-coupled operation, and a setting of 1
> false and true rather than 0 / 1?

I don't see where "false" and "true" here has an advantage over 0 and
1, since the text speaks
about setting a bit to 1 or 0. Anyway I changed this in v4.

BTW. in v4 I re-added the comments about the channels for /* single
tap */ and /* double tap */ although this is possible to see from the
iio channel. Since I add comments for freefall, for activity, etc. it
looked cleaner to me, if definitely not ok, let me know please.

> > +      * 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.
> ...
> >  static int adxl345_is_act_inact_en(struct adxl345_state *st,
> >                                  enum adxl345_activity_type type, bool *en)
> >  {
> > @@ -695,6 +777,11 @@ static int adxl345_set_odr(struct adxl345_state *st, enum adxl345_odr odr)
> >       if (ret)
> >               return ret;
> >
> > +     /* update inactivity time by ODR */
> > +     ret = adxl345_set_inact_time_s(st, 0);
> > +     if (ret)
> > +             return ret;
> > +
> >       return 0;
>
> return adxl345_set_inact_time_s()
>
> >  }
> >
> > @@ -718,14 +805,54 @@ 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)
> >  {
> > +     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;
> > +
> > +     ret = regmap_write(st->regmap, adxl345_act_thresh_reg[ADXL345_INACTIVITY],
> > +                        inact_threshold);
> > +     if (ret)
> > +             return ret;
> return regmap_write()
> > +
> >       return 0;
> >  }
>

Re: [PATCH v3 11/15] iio: accel: adxl345: add g-range configuration

[Lothar Rubusch]

On Tue, Mar 4, 2025 at 2:40 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu, 20 Feb 2025 10:42:30 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Introduce means to configure and work with the available g-ranges
> > keeping the precision of 13 digits.
> >
> > This is in preparation for the activity/inactivity feature.
>
> I'm not really following why adding range control is anything
> much to do with that. Mostly we do this to improve accuracy for
> low accelerations.
>

As you probably saw the connection comes a bit over the link in
adjusting the activity/inactivity
parameters (times and threshold) by a given range in the follow up patches.

If the question is rather why at all adding this g-range control. My
idea was that adjusting i.e. lowering precision, less odr, etc might
also help adjusting power consumption. In other words
from a user perspective I assume there is more configuration
possibility. I did not pretend to tune
the implementation for lowest possible power consumption, though. It
was just an idea.

[Also, I was curious about implementing it here. My patch here is
rather meant as a proposal,
if you strongly oppose the idea, pls let me know.]

> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
>
>
> > @@ -483,12 +518,48 @@ static int adxl345_set_odr(struct adxl345_state *st, enum adxl345_odr odr)
> >       return 0;
> >  }
> >
> > +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])
> > +                     break;
> Similar to case in earlier patch, maybe set *range and return in here
> so that any finish of the loop is an error.
> > +
> > +     if (i == ARRAY_SIZE(adxl345_fullres_range_tbl))
> > +             return -EINVAL;
> > +
> > +     *range = i;
> > +
> > +     return 0;
> > +}
> > +
> > +static int adxl345_set_range(struct adxl345_state *st, enum adxl345_range range)
> > +{
> > +     int 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;
> > +
>
> return regmap_update_bits() unless this gets more complex in later patch.
>
> > +     return 0;
> > +}
> > +
>
> > @@ -558,6 +634,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;
> >
> > @@ -581,6 +658,12 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
> >                       return ret;
> >               ret = adxl345_set_odr(st, odr);
> >               break;
> > +     case IIO_CHAN_INFO_SCALE:
> > +             ret = adxl345_find_range(st, val, val2, &range);
> > +             if (ret)
> > +                     return ret;
> > +             ret = adxl345_set_range(st, range);
> as in previous I'd have the
>                 if (ret)
>                         return ret;
> here for consistency with the other one immediately above this.
> > +             break;
> >       default:
> >               return -EINVAL;
> >       }
>

Re: [PATCH v3 06/15] iio: accel: adxl345: add single tap feature

[Jonathan Cameron]

On Thu, 13 Mar 2025 17:29:08 +0100
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Hi Jonathan,
> 
> I prepared, reorganized and tested a v4 patch set. Given that I see
> how busy you must be these days with current increased mail traffic
> just in IIO ML when I compare it to some years ago,
> I don't want to bother you too much.

Thanks!  It's a travel heavy month which always messes with finding
time for reviews.  Too much bored time on planes with not enough space
to open a laptop properly!

> Some particular doubts I will inline down below. If possible with your
> work flow and to avoid giving you extra work, I'd like to ask you to
> read the questions here, but to give your answers right to the
> v4 patch set (so, i.e. after having seen the current state of source).
> It also should make my points
> a bit clearer. Anyway, this is just my idea, since I'm always happy
> about any feedback!

I might reply there as well, but it's easy to put a few comments here.

> 
> On Sun, Mar 2, 2025 at 1:14 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Thu, 20 Feb 2025 10:42:25 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >  
> > > 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 | 364 ++++++++++++++++++++++++++++++-
> > >  1 file changed, 362 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > > index 0cee81bc1877..d05593c0d513 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,33 @@
> > >  #define ADXL345_INT1                 0
> > >  #define ADXL345_INT2                 1
> > >
> > > +#define ADXL345_REG_TAP_AXIS_MSK     GENMASK(2, 0)  
> > This is a bit confusing.  Here we have a mask for axis that
> > has 3 bits.  
> > > +
> > > +enum adxl345_axis {
> > > +     ADXL345_Z_EN = BIT(0),
> > > +     ADXL345_Y_EN = BIT(1),
> > > +     ADXL345_X_EN = BIT(2),
> > > +     /* Suppress double tap detection if value > tap threshold */
> > > +     ADXL345_TAP_SUPPRESS = BIT(3),  
> > and here an enum that is closely related with 4.  
> 
> I see your point. There are several registers used in the sensor for
> directions. A status register for tap and activity directions, and a
> activity/inactivity direction register. For Tap, direction enables are
> stored using the suppress bit in the fourth position. All those
> locations use actually four bit. Partly the upper four, partly the
> lower four. That's why I use here four bit for reading and writing.
> The locations 0, 1, 2 then can be used directly. Location 3 only
> applies to tap detection.
> 
> I'll keep this in v4 patches, and hope to understand you correctly
> that this is not a "real" issue?
I'd split the AXIS_MSK into two parts.  One with just the axes
(make it the | of the 3 separate bits) and another one with suppress bit.
I'm not sure an enum really helps. Maybe better to just have defines.


> > > +
> > > +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 is 62.5mg/LSB (i.e. 0xFF = 16g) but
> > > +                      * not applied here.  
> >
> > Maybe say why.
> >  
> 
> Usually I did scaling for the time values. Time values is something I
> can understand someone
> rather wants to configure in corresponding time units, such as [ms],
> [us] or [s] rather than bit
> values. For [mg] values, franckly speaking, I imagine this is a bit overkill.
> 
> The threshold quite often is rather expect to be higher or lower,
> depending a bit on variation of
> the measurements. In the context of this rather "cheap" sensor, I
> guess I'm not putting up a
> seismic instrument, but rather generic tap detection, freefall or
> general activity in a general
> purpose context such as gaming, or the like. Let me know if this
> assumption here is too lazy.

Generally event values would be matched to scale of _RAW but for a
gesture, it is indeed a rather vague thing.  I'll take a look at 
the new code and think about this a bit.

Jonathan

Re: [PATCH v3 11/15] iio: accel: adxl345: add g-range configuration

[Jonathan Cameron]

On Thu, 13 Mar 2025 17:47:08 +0100
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> On Tue, Mar 4, 2025 at 2:40 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Thu, 20 Feb 2025 10:42:30 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >  
> > > Introduce means to configure and work with the available g-ranges
> > > keeping the precision of 13 digits.
> > >
> > > This is in preparation for the activity/inactivity feature.  
> >
> > I'm not really following why adding range control is anything
> > much to do with that. Mostly we do this to improve accuracy for
> > low accelerations.
> >  
> 
> As you probably saw the connection comes a bit over the link in
> adjusting the activity/inactivity
> parameters (times and threshold) by a given range in the follow up patches.
> 
> If the question is rather why at all adding this g-range control. My
> idea was that adjusting i.e. lowering precision, less odr, etc might
> also help adjusting power consumption. In other words
> from a user perspective I assume there is more configuration
> possibility. I did not pretend to tune
> the implementation for lowest possible power consumption, though. It
> was just an idea.
> 
> [Also, I was curious about implementing it here. My patch here is
> rather meant as a proposal,
> if you strongly oppose the idea, pls let me know.]

Control is fine (and lots of drivers do it). It was just that comment
that had me confused. To me this is a mostly unrelated feature.

It used to be the case when I last regularly used multirange accelerometers
that they had approximately matched the quality of the ADC with that of
the sensor.  So normal reason to reduce range was that it actually gave
you better accuracy as long as you didn't saturate. Mind you, for
the applications I had with sensors on sprinters shoes, all accelerometers
used to saturate even on the highest range setting! Not sure if the
reducing range for noise improvements is true on this particular sensor.

Jonathan

Re: [PATCH v3 12/15] iio: accel: adxl345: add activity event feature

[Jonathan Cameron]

On Tue, 11 Mar 2025 11:55:05 +0100
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Hi Jonathan,
> 
> I'm currently about to update the series and like to answer some of
> your review comments directly when submitting. Nevertheless, here I'll
> anticipate this one. Pls, find my questions inlined down below.
> 
> On Tue, Mar 4, 2025 at 2:49 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Thu, 20 Feb 2025 10:42:31 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >  
> > > Make the sensor detect and issue interrupts at activity. Activity
> > > events are configured by a threshold stored in regmap cache.
> > >
> > > Activity, together with ODR and range setting are preparing a setup
> > > together with inactivity coming in a follow up patch.
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>  
> > Some questions / comments inline.
> >
> > This review is has been at random moments whilst travelling (hence
> > over several days!) so it may be less than thorough or consistent!
> > I should be back to normal sometime next week.
> >  
> 
> No problem, no hurry!
> 
> > > @@ -258,6 +284,73 @@ 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_write_act_axis(struct adxl345_state *st,
> > > +                               enum adxl345_activity_type type, bool en)
> > > +{
> > > +     int ret;
> > > +
> > > +     /*
> > > +      * The ADXL345 allows for individually enabling/disabling axis for
> > > +      * activity and inactivity detection, respectively. Here both axis are
> > > +      * kept in sync, i.e. an axis will be generally enabled or disabled for
> > > +      * both equally, activity and inactivity detection.  
> >
> > Why?  Can definitely see people only being interested in one case
> > and not the other.  What advantage is there in always having both
> > or neither over separate controls?  
> 
> Ugh! This happens when I mix writing code and writing English texts,
> w/o re-reading it.
> 
> Situation: The sensor allows to individually enable / disable x,y and
> z axis for r activity and for inactivity. I don't offer this in the
> driver. When activity is selected, I always enable all three axis or
> disable them. Similar, for inactivity.

Ah. All axis together, not always both inactivty and activity.

> The question is then actually,
> if this is legitimate, or should I really implement enabling/disabling
> of each axis individually for activity and similar then for
> inactivity? I mean, when not interested in one or the other axis,
> someone can fiilter the result.

For some sensors there is no way to distinguish which threshold was hit
(they just provide 1 bit for activity in the status register)
Here it seems we get a single bit that indicates first act or inact
triggering axis (in ACT_TAP_STATUS).  Assuming I read that
text correcty only one bit is set. That's not exactly useful as
it doesn't tell use other axis would have triggered it.

So I think here your approach of all axis enable is perhaps the
right approach. Also report it as an X_OR_Y_OR_Z event and ignore
the indication of which axis perhaps?

> On the other side I can imagine a
> small impact in power consumption, when only one axis is used instead
> of three axis (?). Since the ADXL345 is [was] one of Analog's fancy
> acme-ultra-low-power-sensors, power is definitvely a topic here IMHO.
> I can't really estimate the importance.

I doubt it would make a measurable difference to power usage.

> 
> I guess I'll try to implement it and see how ugly it gets. At least
> it's a good exercise. As also, I'll try to bring regmap cache and
> clear_bits / set_bits more in here for activity and inactivity in the
> next version.
> 

> > > @@ -842,6 +972,23 @@ 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);
> > > +                             break;  
> > This collection of breaks and nested functions suggests maybe we can either
> > return directly (I've lost track of what happens after this) or that
> > we should factor out some of this code to allow direct returns in the
> > function we put that code in.  Chasing the breaks is not great if it
> > doesn't lead to anything interesting.  
> 
> I understand, but since I'm using quite a bit configuration for the
> sensor, I'm taking advantage
> of type, info and dir here. It won't get more complex than that. I'm
> [actually] pretty sure, since this
> then is almost feature complete.
> 
> I don't see a different way how to do it. I mean, I could still
> separate handling the "dir" entirely in
> a called function. Or, say, implement IIO_EV_TYPE_THRESH handling in a
> separate function?
> Pls, let me know what you think.

Maybe factor everything out between the set_measure_en and it's counterpart.
Then you can just return in all paths in the factored out function.
That's nice because anyone reading it doesn't have to chase down to
see what else happens.

It might make sense to break it up further but probably not.
> 
> > > +                     default:
> > > +                             ret = -EINVAL;
> > > +                     }
> > > +                     break;
> > > +             default:
> > > +                     ret = -EINVAL;
> > > +             }
> > > +             break;
> > >       case IIO_EV_TYPE_GESTURE:
> > >               switch (info) {
> > >               case IIO_EV_INFO_VALUE:
> > > @@ -1124,6 +1271,17 @@ 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_tap_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,
> > > @@ -1157,6 +1315,7 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
> > >               ret = regmap_read(st->regmap, ADXL345_REG_ACT_TAP_STATUS, &regval);
> > >               if (ret)
> > >                       return ret;
> > > +             /* tap direction */  
> >
> > Belongs in earlier patch?
> >  
> > >               if (FIELD_GET(ADXL345_Z_EN, regval) > 0)
> > >                       act_tap_dir = IIO_MOD_Z;
> > >               else if (FIELD_GET(ADXL345_Y_EN, regval) > 0)
> > > @@ -1165,6 +1324,19 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
> > >                       act_tap_dir = IIO_MOD_X;
> > >       }
> > >
> > > +     if (FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, st->act_axis_ctrl) > 0) {
> > > +             ret = regmap_read(st->regmap, ADXL345_REG_ACT_TAP_STATUS, &regval);
> > > +             if (ret)
> > > +                     return ret;
> > > +             /* activity direction */
> > > +             if (FIELD_GET(ADXL345_Z_EN, regval >> 4) > 0)  
> >
> > I'm not following the shifts here.   That looks like we don't have
> > defines that we should but instead use the ones for the lower fields.  
> 
> The 8-bit register is split as follows:
> 
> | 7                |  6         |  5          |  4         |  3
>           |  2              |  1             |  0             |
> ----------------------------------------------------------------------------------------------------------------------
> | ACT ac/dc | ACT_X | ACT_Y | ACT_Z | INACT ac/dc | INACT_X | INACT_Y
> | INACT_Z |
> 
> I thought here, either I shift the ACT_* directions by 4 then use the
> general mask for axis (lower 4 bits). Or I introduce an axis enum for
> ACT_* and a separate one for INACT_*. Thus, I kept the shift and use
> the same ADXL345_*_EN mask. How can I improve this, or can this stay?
I'd not use enums here at all unless you actually use the enum
type directly somewhere to enforce allowed values.

Separate defines for inact and act make sense though.  Saves the reader
of this bit of the code having to care about the layout of the fields,

Jonathan


> 
> >  
> > > +                     act_tap_dir = IIO_MOD_Z;
> > > +             else if (FIELD_GET(ADXL345_Y_EN, regval >> 4) > 0)
> > > +                     act_tap_dir = IIO_MOD_Y;
> > > +             else if (FIELD_GET(ADXL345_X_EN, regval >> 4) > 0)
> > > +                     act_tap_dir = IIO_MOD_X;
> > > +     }  
> >
> >  
> 
> Best,
> L