[PATCH v4 00/11] iio: accel: adxl313: add power-save on activity/inactivity

[PATCH v4 00/11] iio: accel: adxl313: add power-save on activity/inactivity

[Lothar Rubusch]

The patch set covers the following topics:
- add debug register and regmap cache
- prepare iio channel scan_type and scan_index
- prepare interrupt handling
- implement fifo with watermark
- add activity/inactivity together with auto-sleep with link bit
- add ac coupled activity/inactivity, integrate with auto-sleep and link bit
- documentation

Since activity and inactivity here are implemented covering all axis, I
assumed x&y&z and x|y|z, respectively. Thus the driver uses a fake
channel for activity/inactiviy. AC-coupling is similar to other Analog Device
accelerometers, so MAG_ADAPTIVE events are chosen. Combinations are
documented and functionality tested and verified working.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
v3 -> v4:
- squash patches [v3 02/12 + 03/12]: buffer usage into the patch that adds buffered support
- squash patches [v3 07/12 + 08/12]: interrupt handler with watermark implementation
- add patch: (in)activity / AC coupled as `MAG_ADAPTIVE` event
- `ADXL313_MEASUREMENT_MODE`: adjust commit message on removal of define and adding measurement enable function
- remove irq variable from driver data struct, make it a local variable
- `adxl313_core_probe()`: flip logic to condition `int_line != ADXL313_INT_NONE`
- `adxl313_core_probe()`: change mapping interrupts from 0xff to an explicit local mask
- `adxl313_core_probe()`: add comment on FIFO bypass mode
- reduce odd selection of headers to add [`adxl313_core.c`]
- `adxl313_set_fifo()`: this function was turning measurement off/on before changing `fifo_mode`,
   called in postenable and predisable this firstly excluded setting of interrupts, and secondly
   still configured watermark where unnecessary, this function was thus removed (covers unhandled
   return value, and refactoring of function parameters)
- `adxl313_fifo_transfer()`: simplify computation of `sizeof(i*count/2)`
- `adxl313_irq_handler()`: make call of `adxl313_reset_fifo()` conditional to OVERRUN one patch earlier
- includes: rework adding included headers
- activity: change to work with or'd axis and related changes to the fake channel and arrays
- `adxl313_set_act_inact_en()`: generally turn off measurement when adjusting config
  activity/inactivity related config registers, turn measurement on after
- doc: adjust code block highlighting and remove links

v2 -> v3:
- verify keeping trailing comma when it's multi-line assignment [v1 02/12]
- `adxl313_set_fifo()`: verify have two on one line to make it easier to read [v1 07/12]
- `adxl313_fifo_transfer()`: verify removal of useless initialization of ret [v1 07/12]
- `adxl313_fifo_transfer()`: verify usage of array_size() from overflow.h [v1 07/12]
- `adxl313_fifo_transfer()`: verify return 0 here [v1 07/12]
- `adxl313_irq_handler()`: verify "Why do we need the label?" / moving the call under the conditional [v1 07/12]
- verify reorganization of half condition for Activity [v1 09/12] and Inactivity [v1 10/12]
- verify usage of MICRO instead of 1000000
- `adxl313_is_act_inact_en()`: restructure according to return logic value, or negative error
- `adxl313_set_act_inact_en()`: restructure function, use regmap_assign_bits()
- `adxl313_set_act_inact_en()`: verify makeing it a logical split [v1 11/12]
- `adxl313_fifo_transfer()`: change iterator variable type from int to unsigned int [v2 07/12]
- `adxl313_fifo_reset()`: add comment on why reset status registers does not do error check ("At least comment...") [v2 07/12]
- `adxl313_fifo_push()`: change iterator variable from int to unsigned int [v2 08/12]
- `adxl313_fifo_push()`: remove duplicate check for samples being <0 [v2 08/12]
- apply `regmap_assign_bits()` in several places to replace regmap_update_bits() depending on bools
- `adxl313_set_watermark()`: rename mask variable to make it more comprehensive
- removal of duplicate blanks in various places (sry, my keyboard died) [v1 07/12]

v1 -> v2:
- usage of units.h
- simplify approach for return values
---
Lothar Rubusch (11):
  iio: accel: adxl313: add debug register
  iio: accel: adxl313: introduce channel buffer
  iio: accel: adxl313: make use of regmap cache
  iio: accel: adxl313: add function to enable measurement
  iio: accel: adxl313: prepare interrupt handling
  iio: accel: adxl313: add basic interrupt handling for FIFO watermark
  iio: accel: adxl313: add activity sensing
  iio: accel: adxl313: add inactivity sensing
  iio: accel: adxl313: implement power-save on inactivity
  iio: accel: adxl313: add AC coupled activity/inactivity events
  docs: iio: add ADXL313 accelerometer

 Documentation/iio/adxl313.rst    | 289 ++++++++++
 Documentation/iio/index.rst      |   1 +
 drivers/iio/accel/adxl313.h      |  33 +-
 drivers/iio/accel/adxl313_core.c | 905 ++++++++++++++++++++++++++++++-
 drivers/iio/accel/adxl313_i2c.c  |   6 +
 drivers/iio/accel/adxl313_spi.c  |   6 +
 6 files changed, 1229 insertions(+), 11 deletions(-)
 create mode 100644 Documentation/iio/adxl313.rst

-- 
2.39.5

[PATCH v4 01/11] iio: accel: adxl313: add debug register

[Lothar Rubusch]

Add iio debug register for general sensor debugging.

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

diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
index 4de0a41bd679..2f26da5857d4 100644
--- a/drivers/iio/accel/adxl313_core.c
+++ b/drivers/iio/accel/adxl313_core.c
@@ -321,10 +321,21 @@ static int adxl313_write_raw(struct iio_dev *indio_dev,
 	}
 }
 
+static int adxl313_reg_access(struct iio_dev *indio_dev, unsigned int reg,
+			      unsigned int writeval, unsigned int *readval)
+{
+	struct adxl313_data *data = iio_priv(indio_dev);
+
+	if (readval)
+		return regmap_read(data->regmap, reg, readval);
+	return regmap_write(data->regmap, reg, writeval);
+}
+
 static const struct iio_info adxl313_info = {
 	.read_raw	= adxl313_read_raw,
 	.write_raw	= adxl313_write_raw,
 	.read_avail	= adxl313_read_freq_avail,
+	.debugfs_reg_access = &adxl313_reg_access,
 };
 
 static int adxl313_setup(struct device *dev, struct adxl313_data *data,
-- 
2.39.5

[PATCH v4 02/11] iio: accel: adxl313: introduce channel buffer

[Lothar Rubusch]

Add a scan_mask and scan_index to the iio channel. The scan_index
prepares the buffer usage. According to the datasheet, the ADXL313
uses 13 bit in full resolution. Add signedness, storage bits and
endianness.

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

diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
index 2f26da5857d4..06a771bb4726 100644
--- a/drivers/iio/accel/adxl313_core.c
+++ b/drivers/iio/accel/adxl313_core.c
@@ -171,9 +171,10 @@ static const int adxl313_odr_freqs[][2] = {
 	[9] = { 3200, 0 },
 };
 
-#define ADXL313_ACCEL_CHANNEL(index, axis) {				\
+#define ADXL313_ACCEL_CHANNEL(index, reg, axis) {			\
 	.type = IIO_ACCEL,						\
-	.address = index,						\
+	.scan_index = (index),						\
+	.address = (reg),						\
 	.modified = 1,							\
 	.channel2 = IIO_MOD_##axis,					\
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
@@ -183,14 +184,26 @@ static const int adxl313_odr_freqs[][2] = {
 	.info_mask_shared_by_type_available =				\
 		BIT(IIO_CHAN_INFO_SAMP_FREQ),				\
 	.scan_type = {							\
+		.sign = 's',						\
 		.realbits = 13,						\
+		.storagebits = 16,					\
+		.endianness = IIO_BE,					\
 	},								\
 }
 
+enum adxl313_chans {
+	chan_x, chan_y, chan_z,
+};
+
 static const struct iio_chan_spec adxl313_channels[] = {
-	ADXL313_ACCEL_CHANNEL(0, X),
-	ADXL313_ACCEL_CHANNEL(1, Y),
-	ADXL313_ACCEL_CHANNEL(2, Z),
+	ADXL313_ACCEL_CHANNEL(0, chan_x, X),
+	ADXL313_ACCEL_CHANNEL(1, chan_y, Y),
+	ADXL313_ACCEL_CHANNEL(2, chan_z, Z),
+};
+
+static const unsigned long adxl313_scan_masks[] = {
+	BIT(chan_x) | BIT(chan_y) | BIT(chan_z),
+	0
 };
 
 static int adxl313_set_odr(struct adxl313_data *data,
@@ -419,6 +432,7 @@ int adxl313_core_probe(struct device *dev,
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->channels = adxl313_channels;
 	indio_dev->num_channels = ARRAY_SIZE(adxl313_channels);
+	indio_dev->available_scan_masks = adxl313_scan_masks;
 
 	ret = adxl313_setup(dev, data, setup);
 	if (ret) {
-- 
2.39.5

[PATCH v4 03/11] iio: accel: adxl313: make use of regmap cache

[Lothar Rubusch]

Setup regmap cache to cache register configuration. This is a preparatory
step for follow up patches. Using cached settings will help at inerrupt
handling, to generate activity and inactivity events.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 drivers/iio/accel/adxl313.h      |  2 ++
 drivers/iio/accel/adxl313_core.c | 17 +++++++++++++++++
 drivers/iio/accel/adxl313_i2c.c  |  6 ++++++
 drivers/iio/accel/adxl313_spi.c  |  6 ++++++
 4 files changed, 31 insertions(+)

diff --git a/drivers/iio/accel/adxl313.h b/drivers/iio/accel/adxl313.h
index 72f624af4686..fc937bdf83b6 100644
--- a/drivers/iio/accel/adxl313.h
+++ b/drivers/iio/accel/adxl313.h
@@ -54,6 +54,8 @@ extern const struct regmap_access_table adxl312_writable_regs_table;
 extern const struct regmap_access_table adxl313_writable_regs_table;
 extern const struct regmap_access_table adxl314_writable_regs_table;
 
+bool adxl313_is_volatile_reg(struct device *dev, unsigned int reg);
+
 enum adxl313_device_type {
 	ADXL312,
 	ADXL313,
diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
index 06a771bb4726..0c893c286017 100644
--- a/drivers/iio/accel/adxl313_core.c
+++ b/drivers/iio/accel/adxl313_core.c
@@ -46,6 +46,23 @@ const struct regmap_access_table adxl314_readable_regs_table = {
 };
 EXPORT_SYMBOL_NS_GPL(adxl314_readable_regs_table, IIO_ADXL313);
 
+bool adxl313_is_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case ADXL313_REG_DATA_AXIS(0):
+	case ADXL313_REG_DATA_AXIS(1):
+	case ADXL313_REG_DATA_AXIS(2):
+	case ADXL313_REG_DATA_AXIS(3):
+	case ADXL313_REG_DATA_AXIS(4):
+	case ADXL313_REG_DATA_AXIS(5):
+	case ADXL313_REG_FIFO_STATUS:
+		return true;
+	default:
+		return false;
+	}
+}
+EXPORT_SYMBOL_NS_GPL(adxl313_is_volatile_reg, "IIO_ADXL313");
+
 static int adxl312_check_id(struct device *dev,
 			    struct adxl313_data *data)
 {
diff --git a/drivers/iio/accel/adxl313_i2c.c b/drivers/iio/accel/adxl313_i2c.c
index a4cf0cf2c5aa..e8636e8ab14f 100644
--- a/drivers/iio/accel/adxl313_i2c.c
+++ b/drivers/iio/accel/adxl313_i2c.c
@@ -21,6 +21,8 @@ static const struct regmap_config adxl31x_i2c_regmap_config[] = {
 		.rd_table	= &adxl312_readable_regs_table,
 		.wr_table	= &adxl312_writable_regs_table,
 		.max_register	= 0x39,
+		.volatile_reg	= adxl313_is_volatile_reg,
+		.cache_type	= REGCACHE_MAPLE,
 	},
 	[ADXL313] = {
 		.reg_bits	= 8,
@@ -28,6 +30,8 @@ static const struct regmap_config adxl31x_i2c_regmap_config[] = {
 		.rd_table	= &adxl313_readable_regs_table,
 		.wr_table	= &adxl313_writable_regs_table,
 		.max_register	= 0x39,
+		.volatile_reg	= adxl313_is_volatile_reg,
+		.cache_type	= REGCACHE_MAPLE,
 	},
 	[ADXL314] = {
 		.reg_bits	= 8,
@@ -35,6 +39,8 @@ static const struct regmap_config adxl31x_i2c_regmap_config[] = {
 		.rd_table	= &adxl314_readable_regs_table,
 		.wr_table	= &adxl314_writable_regs_table,
 		.max_register	= 0x39,
+		.volatile_reg	= adxl313_is_volatile_reg,
+		.cache_type	= REGCACHE_MAPLE,
 	},
 };
 
diff --git a/drivers/iio/accel/adxl313_spi.c b/drivers/iio/accel/adxl313_spi.c
index 9a16b40bff34..68e323e81aeb 100644
--- a/drivers/iio/accel/adxl313_spi.c
+++ b/drivers/iio/accel/adxl313_spi.c
@@ -24,6 +24,8 @@ static const struct regmap_config adxl31x_spi_regmap_config[] = {
 		.max_register	= 0x39,
 		/* Setting bits 7 and 6 enables multiple-byte read */
 		.read_flag_mask	= BIT(7) | BIT(6),
+		.volatile_reg	= adxl313_is_volatile_reg,
+		.cache_type	= REGCACHE_MAPLE,
 	},
 	[ADXL313] = {
 		.reg_bits	= 8,
@@ -33,6 +35,8 @@ static const struct regmap_config adxl31x_spi_regmap_config[] = {
 		.max_register	= 0x39,
 		/* Setting bits 7 and 6 enables multiple-byte read */
 		.read_flag_mask	= BIT(7) | BIT(6),
+		.volatile_reg	= adxl313_is_volatile_reg,
+		.cache_type	= REGCACHE_MAPLE,
 	},
 	[ADXL314] = {
 		.reg_bits	= 8,
@@ -42,6 +46,8 @@ static const struct regmap_config adxl31x_spi_regmap_config[] = {
 		.max_register	= 0x39,
 		/* Setting bits 7 and 6 enables multiple-byte read */
 		.read_flag_mask	= BIT(7) | BIT(6),
+		.volatile_reg	= adxl313_is_volatile_reg,
+		.cache_type	= REGCACHE_MAPLE,
 	},
 };
 
-- 
2.39.5

[PATCH v4 04/11] iio: accel: adxl313: add function to enable measurement

[Lothar Rubusch]

Rework controlling measurement and standby of the sensor. Therefore,
replace writing the register directly by encapsulating this and dealing
with the return value in a separte function to enable and disable
measurement. This will help to avoid redundant code in all locations
where the sensor configuration needs to be adjusted, thus measurement will
be set to standby, in follow up patches.

Further, reduce the control mask to only the measurement bit. The sleep bit
actually controls a different behavior (not just putting the sensor to
standby for configuration, but turning it into sleep mode) and it is not
used so far. In consequence, there is no need to cover sleep bit and
measurement with the same mask.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 drivers/iio/accel/adxl313.h      |  3 +--
 drivers/iio/accel/adxl313_core.c | 10 +++++++---
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/accel/adxl313.h b/drivers/iio/accel/adxl313.h
index fc937bdf83b6..9bf2facdbf87 100644
--- a/drivers/iio/accel/adxl313.h
+++ b/drivers/iio/accel/adxl313.h
@@ -36,8 +36,7 @@
 #define ADXL313_RATE_MSK		GENMASK(3, 0)
 #define ADXL313_RATE_BASE		6
 
-#define ADXL313_POWER_CTL_MSK		GENMASK(3, 2)
-#define ADXL313_MEASUREMENT_MODE	BIT(3)
+#define ADXL313_POWER_CTL_MSK		BIT(3)
 
 #define ADXL313_RANGE_MSK		GENMASK(1, 0)
 #define ADXL313_RANGE_MAX		3
diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
index 0c893c286017..6170c9daa30f 100644
--- a/drivers/iio/accel/adxl313_core.c
+++ b/drivers/iio/accel/adxl313_core.c
@@ -63,6 +63,12 @@ bool adxl313_is_volatile_reg(struct device *dev, unsigned int reg)
 }
 EXPORT_SYMBOL_NS_GPL(adxl313_is_volatile_reg, "IIO_ADXL313");
 
+static int adxl313_set_measure_en(struct adxl313_data *data, bool en)
+{
+	return regmap_assign_bits(data->regmap, ADXL313_REG_POWER_CTL,
+				  ADXL313_POWER_CTL_MSK, en);
+}
+
 static int adxl312_check_id(struct device *dev,
 			    struct adxl313_data *data)
 {
@@ -410,9 +416,7 @@ static int adxl313_setup(struct device *dev, struct adxl313_data *data,
 	}
 
 	/* Enables measurement mode */
-	return regmap_update_bits(data->regmap, ADXL313_REG_POWER_CTL,
-				  ADXL313_POWER_CTL_MSK,
-				  ADXL313_MEASUREMENT_MODE);
+	return adxl313_set_measure_en(data, true);
 }
 
 /**
-- 
2.39.5

[PATCH v4 05/11] iio: accel: adxl313: prepare interrupt handling

[Lothar Rubusch]

Evaluate the devicetree property for an optional interrupt line, and
configure the interrupt mapping accordingly. When no interrupt line
is defined in the devicetree, keep the FIFO in bypass mode as before.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 drivers/iio/accel/adxl313.h      | 13 +++++++++
 drivers/iio/accel/adxl313_core.c | 49 +++++++++++++++++++++++++++++++-
 2 files changed, 61 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/accel/adxl313.h b/drivers/iio/accel/adxl313.h
index 9bf2facdbf87..ab6b9e11fde4 100644
--- a/drivers/iio/accel/adxl313.h
+++ b/drivers/iio/accel/adxl313.h
@@ -21,7 +21,9 @@
 #define ADXL313_REG_ACT_INACT_CTL	0x27
 #define ADXL313_REG_BW_RATE		0x2C
 #define ADXL313_REG_POWER_CTL		0x2D
+#define ADXL313_REG_INT_ENABLE		0x2E
 #define ADXL313_REG_INT_MAP		0x2F
+#define ADXL313_REG_INT_SOURCE		0x30
 #define ADXL313_REG_DATA_FORMAT		0x31
 #define ADXL313_REG_DATA_AXIS(index)	(0x32 + ((index) * 2))
 #define ADXL313_REG_FIFO_CTL		0x38
@@ -45,6 +47,17 @@
 #define ADXL313_SPI_3WIRE		BIT(6)
 #define ADXL313_I2C_DISABLE		BIT(6)
 
+#define ADXL313_INT_OVERRUN		BIT(0)
+#define ADXL313_INT_WATERMARK		BIT(1)
+#define ADXL313_INT_INACTIVITY		BIT(3)
+#define ADXL313_INT_ACTIVITY		BIT(4)
+#define ADXL313_INT_DREADY		BIT(7)
+
+#define ADXL313_REG_FIFO_CTL_MODE_MSK		GENMASK(7, 6)
+
+#define ADXL313_FIFO_BYPASS			0
+#define ADXL313_FIFO_STREAM			2
+
 extern const struct regmap_access_table adxl312_readable_regs_table;
 extern const struct regmap_access_table adxl313_readable_regs_table;
 extern const struct regmap_access_table adxl314_readable_regs_table;
diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
index 6170c9daa30f..31ce1b218488 100644
--- a/drivers/iio/accel/adxl313_core.c
+++ b/drivers/iio/accel/adxl313_core.c
@@ -8,11 +8,17 @@
  */
 
 #include <linux/bitfield.h>
+#include <linux/interrupt.h>
 #include <linux/module.h>
+#include <linux/property.h>
 #include <linux/regmap.h>
 
 #include "adxl313.h"
 
+#define ADXL313_INT_NONE			U8_MAX
+#define ADXL313_INT1				1
+#define ADXL313_INT2				2
+
 static const struct regmap_range adxl312_readable_reg_range[] = {
 	regmap_reg_range(ADXL313_REG_DEVID0, ADXL313_REG_DEVID0),
 	regmap_reg_range(ADXL313_REG_OFS_AXIS(0), ADXL313_REG_OFS_AXIS(2)),
@@ -436,7 +442,9 @@ int adxl313_core_probe(struct device *dev,
 {
 	struct adxl313_data *data;
 	struct iio_dev *indio_dev;
-	int ret;
+	u8 int_line;
+	u8 int_map_msk;
+	int irq, ret;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
 	if (!indio_dev)
@@ -461,6 +469,45 @@ int adxl313_core_probe(struct device *dev,
 		return ret;
 	}
 
+	int_line = ADXL313_INT1;
+	irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT1");
+	if (irq < 0) {
+		int_line = ADXL313_INT2;
+		irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT2");
+		if (irq < 0)
+			int_line = ADXL313_INT_NONE;
+	}
+
+	if (int_line != ADXL313_INT_NONE) {
+		/* FIFO_STREAM mode */
+		int_map_msk = ADXL313_INT_DREADY | ADXL313_INT_ACTIVITY |
+			ADXL313_INT_INACTIVITY | ADXL313_INT_WATERMARK |
+			ADXL313_INT_OVERRUN;
+		ret = regmap_assign_bits(data->regmap, ADXL313_REG_INT_MAP,
+					 int_map_msk, int_line == ADXL313_INT2);
+		if (ret)
+			return ret;
+	} else {
+		/*
+		 * FIFO_BYPASSED mode
+		 *
+		 * When no interrupt lines are specified, the driver falls back
+		 * to use the sensor in FIFO_BYPASS mode. This means turning off
+		 * internal FIFO and interrupt generation (since there is no
+		 * line specified). Unmaskable interrupts such as overrun or
+		 * data ready won't interfere. Even that a FIFO_STREAM mode w/o
+		 * connected interrupt line might allow for obtaining raw
+		 * measurements, a fallback to disable interrupts when no
+		 * interrupt lines are connected seems to be the cleaner
+		 * solution.
+		 */
+		ret = regmap_write(data->regmap, ADXL313_REG_FIFO_CTL,
+				   FIELD_PREP(ADXL313_REG_FIFO_CTL_MODE_MSK,
+					      ADXL313_FIFO_BYPASS));
+		if (ret)
+			return ret;
+	}
+
 	return devm_iio_device_register(dev, indio_dev);
 }
 EXPORT_SYMBOL_NS_GPL(adxl313_core_probe, IIO_ADXL313);
-- 
2.39.5

[PATCH v4 06/11] iio: accel: adxl313: add basic interrupt handling for FIFO watermark

[Lothar Rubusch]

Prepare the interrupt handler. Add register entries to evaluate the
incoming interrupt. Add functions to clear status registers and reset the
FIFO.

Add FIFO watermark configuration and evaluation. Let a watermark to be
configured. Evaluate the interrupt accordingly. Read out the FIFO content
and push the values to the IIO channel.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 drivers/iio/accel/adxl313.h      |  10 ++
 drivers/iio/accel/adxl313_core.c | 190 +++++++++++++++++++++++++++++++
 2 files changed, 200 insertions(+)

diff --git a/drivers/iio/accel/adxl313.h b/drivers/iio/accel/adxl313.h
index ab6b9e11fde4..4f4a9fd39f6d 100644
--- a/drivers/iio/accel/adxl313.h
+++ b/drivers/iio/accel/adxl313.h
@@ -53,11 +53,19 @@
 #define ADXL313_INT_ACTIVITY		BIT(4)
 #define ADXL313_INT_DREADY		BIT(7)
 
+/* FIFO entries: how many values are stored in the FIFO */
+#define ADXL313_REG_FIFO_STATUS_ENTRIES_MSK	GENMASK(5, 0)
+/* FIFO samples: number of samples needed for watermark (FIFO mode) */
+#define ADXL313_REG_FIFO_CTL_SAMPLES_MSK	GENMASK(4, 0)
 #define ADXL313_REG_FIFO_CTL_MODE_MSK		GENMASK(7, 6)
 
 #define ADXL313_FIFO_BYPASS			0
 #define ADXL313_FIFO_STREAM			2
 
+#define ADXL313_FIFO_SIZE			32
+
+#define ADXL313_NUM_AXIS			3
+
 extern const struct regmap_access_table adxl312_readable_regs_table;
 extern const struct regmap_access_table adxl313_readable_regs_table;
 extern const struct regmap_access_table adxl314_readable_regs_table;
@@ -78,7 +86,9 @@ struct adxl313_data {
 	struct regmap	*regmap;
 	const struct adxl313_chip_info *chip_info;
 	struct mutex	lock; /* lock to protect transf_buf */
+	u8 watermark;
 	__le16		transf_buf __aligned(IIO_DMA_MINALIGN);
+	__le16		fifo_buf[ADXL313_NUM_AXIS * ADXL313_FIFO_SIZE + 1];
 };
 
 struct adxl313_chip_info {
diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
index 31ce1b218488..8a0b5542fb40 100644
--- a/drivers/iio/accel/adxl313_core.c
+++ b/drivers/iio/accel/adxl313_core.c
@@ -10,15 +10,21 @@
 #include <linux/bitfield.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
+#include <linux/overflow.h>
 #include <linux/property.h>
 #include <linux/regmap.h>
 
+#include <linux/iio/buffer.h>
+#include <linux/iio/kfifo_buf.h>
+
 #include "adxl313.h"
 
 #define ADXL313_INT_NONE			U8_MAX
 #define ADXL313_INT1				1
 #define ADXL313_INT2				2
 
+#define ADXL313_REG_XYZ_BASE			ADXL313_REG_DATA_AXIS(0)
+
 static const struct regmap_range adxl312_readable_reg_range[] = {
 	regmap_reg_range(ADXL313_REG_DEVID0, ADXL313_REG_DEVID0),
 	regmap_reg_range(ADXL313_REG_OFS_AXIS(0), ADXL313_REG_OFS_AXIS(2)),
@@ -62,6 +68,7 @@ bool adxl313_is_volatile_reg(struct device *dev, unsigned int reg)
 	case ADXL313_REG_DATA_AXIS(4):
 	case ADXL313_REG_DATA_AXIS(5):
 	case ADXL313_REG_FIFO_STATUS:
+	case ADXL313_REG_INT_SOURCE:
 		return true;
 	default:
 		return false;
@@ -363,6 +370,176 @@ static int adxl313_write_raw(struct iio_dev *indio_dev,
 	}
 }
 
+static int adxl313_set_watermark(struct iio_dev *indio_dev, unsigned int value)
+{
+	struct adxl313_data *data = iio_priv(indio_dev);
+	const unsigned int fifo_mask = 0x1f, interrupt_mask = 0x02;
+	int ret;
+
+	value = min(value, ADXL313_FIFO_SIZE - 1);
+
+	ret = regmap_update_bits(data->regmap, ADXL313_REG_FIFO_CTL,
+				 fifo_mask, value);
+	if (ret)
+		return ret;
+
+	data->watermark = value;
+
+	return regmap_update_bits(data->regmap, ADXL313_REG_INT_ENABLE,
+				  interrupt_mask, ADXL313_INT_WATERMARK);
+}
+
+static int adxl313_get_samples(struct adxl313_data *data)
+{
+	unsigned int regval = 0;
+	int ret;
+
+	ret = regmap_read(data->regmap, ADXL313_REG_FIFO_STATUS, &regval);
+	if (ret)
+		return ret;
+
+	return FIELD_GET(ADXL313_REG_FIFO_STATUS_ENTRIES_MSK, regval);
+}
+
+static int adxl313_fifo_transfer(struct adxl313_data *data, int samples)
+{
+	unsigned int i;
+	int ret;
+
+	for (i = 0; i < samples; i++) {
+		ret = regmap_bulk_read(data->regmap, ADXL313_REG_XYZ_BASE,
+				       data->fifo_buf + (i * ADXL313_NUM_AXIS),
+				       2 * ADXL313_NUM_AXIS);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+/**
+ * adxl313_fifo_reset() - Reset the FIFO and interrupt status registers.
+ * @data: The device data.
+ *
+ * Reset the FIFO status registers. Reading out status registers clears the
+ * FIFO and interrupt configuration. Thus do not evaluate regmap return values.
+ * Ignore particular read register content. Register content is not processed
+ * any further. Therefore the function returns void.
+ */
+static void adxl313_fifo_reset(struct adxl313_data *data)
+{
+	unsigned int regval;
+	int samples;
+
+	adxl313_set_measure_en(data, false);
+
+	samples = adxl313_get_samples(data);
+	if (samples > 0)
+		adxl313_fifo_transfer(data, samples);
+
+	regmap_read(data->regmap, ADXL313_REG_INT_SOURCE, &regval);
+
+	adxl313_set_measure_en(data, true);
+}
+
+static int adxl313_buffer_postenable(struct iio_dev *indio_dev)
+{
+	struct adxl313_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = adxl313_set_measure_en(data, false);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(data->regmap, ADXL313_REG_FIFO_CTL,
+			   FIELD_PREP(ADXL313_REG_FIFO_CTL_SAMPLES_MSK,	data->watermark) |
+			   FIELD_PREP(ADXL313_REG_FIFO_CTL_MODE_MSK, ADXL313_FIFO_STREAM));
+	if (ret)
+		return ret;
+
+	return adxl313_set_measure_en(data, true);
+}
+
+static int adxl313_buffer_predisable(struct iio_dev *indio_dev)
+{
+	struct adxl313_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = adxl313_set_measure_en(data, false);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(data->regmap, ADXL313_REG_FIFO_CTL,
+			   FIELD_PREP(ADXL313_REG_FIFO_CTL_MODE_MSK, ADXL313_FIFO_BYPASS));
+
+	ret = regmap_write(data->regmap, ADXL313_REG_INT_ENABLE, 0);
+	if (ret)
+		return ret;
+
+	return adxl313_set_measure_en(data, true);
+}
+
+static const struct iio_buffer_setup_ops adxl313_buffer_ops = {
+	.postenable = adxl313_buffer_postenable,
+	.predisable = adxl313_buffer_predisable,
+};
+
+static int adxl313_fifo_push(struct iio_dev *indio_dev, int samples)
+{
+	struct adxl313_data *data = iio_priv(indio_dev);
+	unsigned int i;
+	int ret;
+
+	ret = adxl313_fifo_transfer(data, samples);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < ADXL313_NUM_AXIS * samples; i += ADXL313_NUM_AXIS)
+		iio_push_to_buffers(indio_dev, &data->fifo_buf[i]);
+
+	return 0;
+}
+
+static int adxl313_push_event(struct iio_dev *indio_dev, int int_stat)
+{
+	struct adxl313_data *data = iio_priv(indio_dev);
+	int samples;
+
+	if (FIELD_GET(ADXL313_INT_WATERMARK, int_stat)) {
+		samples = adxl313_get_samples(data);
+		if (samples < 0)
+			return samples;
+
+		return adxl313_fifo_push(indio_dev, samples);
+	}
+
+	/* Return error if no event data was pushed to the IIO channel. */
+	return -ENOENT;
+}
+
+static irqreturn_t adxl313_irq_handler(int irq, void *p)
+{
+	struct iio_dev *indio_dev = p;
+	struct adxl313_data *data = iio_priv(indio_dev);
+	int int_stat;
+
+	if (regmap_read(data->regmap, ADXL313_REG_INT_SOURCE, &int_stat))
+		return IRQ_NONE;
+
+	if (adxl313_push_event(indio_dev, int_stat))
+		goto err;
+
+	if (FIELD_GET(ADXL313_INT_OVERRUN, int_stat))
+		goto err;
+
+	return IRQ_HANDLED;
+
+err:
+	adxl313_fifo_reset(data);
+
+	return IRQ_HANDLED;
+}
+
 static int adxl313_reg_access(struct iio_dev *indio_dev, unsigned int reg,
 			      unsigned int writeval, unsigned int *readval)
 {
@@ -377,6 +554,7 @@ static const struct iio_info adxl313_info = {
 	.read_raw	= adxl313_read_raw,
 	.write_raw	= adxl313_write_raw,
 	.read_avail	= adxl313_read_freq_avail,
+	.hwfifo_set_watermark = adxl313_set_watermark,
 	.debugfs_reg_access = &adxl313_reg_access,
 };
 
@@ -487,6 +665,18 @@ int adxl313_core_probe(struct device *dev,
 					 int_map_msk, int_line == ADXL313_INT2);
 		if (ret)
 			return ret;
+
+		ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
+						  &adxl313_buffer_ops);
+		if (ret)
+			return ret;
+
+		ret = devm_request_threaded_irq(dev, irq, NULL,
+						&adxl313_irq_handler,
+						IRQF_SHARED | IRQF_ONESHOT,
+						indio_dev->name, indio_dev);
+		if (ret)
+			return ret;
 	} else {
 		/*
 		 * FIFO_BYPASSED mode
-- 
2.39.5

[PATCH v4 07/11] iio: accel: adxl313: add activity sensing

[Lothar Rubusch]

Add possibilities to set a threshold for activity sensing. Extend the
interrupt handler to process activity interrupts. Provide functions to set
the activity threshold and to enable/disable activity sensing. Further add
a fake channel for having x, y and z axis anded on the iio channel.

This is a preparatory patch. Some of the definitions and functions are
supposed to be extended for inactivity later on.

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

diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
index 8a0b5542fb40..e647c40575ab 100644
--- a/drivers/iio/accel/adxl313_core.c
+++ b/drivers/iio/accel/adxl313_core.c
@@ -13,8 +13,10 @@
 #include <linux/overflow.h>
 #include <linux/property.h>
 #include <linux/regmap.h>
+#include <linux/units.h>
 
 #include <linux/iio/buffer.h>
+#include <linux/iio/events.h>
 #include <linux/iio/kfifo_buf.h>
 
 #include "adxl313.h"
@@ -25,6 +27,21 @@
 
 #define ADXL313_REG_XYZ_BASE			ADXL313_REG_DATA_AXIS(0)
 
+#define ADXL313_ACT_XYZ_EN			GENMASK(6, 4)
+
+/* activity/inactivity */
+enum adxl313_activity_type {
+	ADXL313_ACTIVITY,
+};
+
+static const unsigned int adxl313_act_int_reg[] = {
+	[ADXL313_ACTIVITY] = ADXL313_INT_ACTIVITY,
+};
+
+static const unsigned int adxl313_act_thresh_reg[] = {
+	[ADXL313_ACTIVITY] = ADXL313_REG_THRESH_ACT,
+};
+
 static const struct regmap_range adxl312_readable_reg_range[] = {
 	regmap_reg_range(ADXL313_REG_DEVID0, ADXL313_REG_DEVID0),
 	regmap_reg_range(ADXL313_REG_OFS_AXIS(0), ADXL313_REG_OFS_AXIS(2)),
@@ -227,6 +244,15 @@ static const int adxl313_odr_freqs[][2] = {
 	},								\
 }
 
+static const struct iio_event_spec adxl313_activity_events[] = {
+	{
+		.type = IIO_EV_TYPE_MAG,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
+		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE),
+	},
+};
+
 enum adxl313_chans {
 	chan_x, chan_y, chan_z,
 };
@@ -235,6 +261,14 @@ static const struct iio_chan_spec adxl313_channels[] = {
 	ADXL313_ACCEL_CHANNEL(0, chan_x, X),
 	ADXL313_ACCEL_CHANNEL(1, chan_y, Y),
 	ADXL313_ACCEL_CHANNEL(2, chan_z, Z),
+	{
+		.type = IIO_ACCEL,
+		.modified = 1,
+		.channel2 = IIO_MOD_X_OR_Y_OR_Z,
+		.scan_index = -1, /* Fake channel for axis OR'ing */
+		.event_spec = adxl313_activity_events,
+		.num_event_specs = ARRAY_SIZE(adxl313_activity_events),
+	},
 };
 
 static const unsigned long adxl313_scan_masks[] = {
@@ -297,6 +331,68 @@ static int adxl313_read_freq_avail(struct iio_dev *indio_dev,
 	}
 }
 
+static int adxl313_is_act_inact_en(struct adxl313_data *data,
+				   enum adxl313_activity_type type)
+{
+	unsigned int axis_ctrl;
+	unsigned int regval;
+	int axis_en, int_en, ret;
+
+	ret = regmap_read(data->regmap, ADXL313_REG_ACT_INACT_CTL, &axis_ctrl);
+	if (ret)
+		return ret;
+
+	/* Check if axis for activity are enabled */
+	if (type != ADXL313_ACTIVITY)
+		return 0;
+
+	axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
+
+	/* The axis are enabled, now check if specific interrupt is enabled */
+	ret = regmap_read(data->regmap, ADXL313_REG_INT_ENABLE, &regval);
+	if (ret)
+		return ret;
+
+	int_en = adxl313_act_int_reg[type] & regval;
+
+	return axis_en && int_en;
+}
+
+static int adxl313_set_act_inact_en(struct adxl313_data *data,
+				    enum adxl313_activity_type type,
+				    bool cmd_en)
+{
+	unsigned int axis_ctrl;
+	unsigned int threshold;
+	int ret;
+
+	if (type != ADXL313_ACTIVITY)
+		return 0;
+
+	axis_ctrl = ADXL313_ACT_XYZ_EN;
+
+	ret = adxl313_set_measure_en(data, false);
+	if (ret)
+		return ret;
+
+	ret = regmap_assign_bits(data->regmap, ADXL313_REG_ACT_INACT_CTL,
+				 axis_ctrl, cmd_en);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(data->regmap, adxl313_act_thresh_reg[type], &threshold);
+	if (ret)
+		return ret;
+
+	ret = regmap_assign_bits(data->regmap, ADXL313_REG_INT_ENABLE,
+				 adxl313_act_int_reg[type],
+				 cmd_en && threshold);
+	if (ret)
+		return ret;
+
+	return adxl313_set_measure_en(data, true);
+}
+
 static int adxl313_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan,
 			    int *val, int *val2, long mask)
@@ -370,6 +466,113 @@ static int adxl313_write_raw(struct iio_dev *indio_dev,
 	}
 }
 
+static int adxl313_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 adxl313_data *data = iio_priv(indio_dev);
+
+	if (type != IIO_EV_TYPE_MAG)
+		return -EINVAL;
+
+	switch (dir) {
+	case IIO_EV_DIR_RISING:
+		return adxl313_is_act_inact_en(data, ADXL313_ACTIVITY);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int adxl313_write_event_config(struct iio_dev *indio_dev,
+				      const struct iio_chan_spec *chan,
+				      enum iio_event_type type,
+				      enum iio_event_direction dir,
+				      bool state)
+{
+	struct adxl313_data *data = iio_priv(indio_dev);
+
+	if (type != IIO_EV_TYPE_MAG)
+		return -EINVAL;
+
+	switch (dir) {
+	case IIO_EV_DIR_RISING:
+		return adxl313_set_act_inact_en(data, ADXL313_ACTIVITY, state);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int adxl313_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 adxl313_data *data = iio_priv(indio_dev);
+	unsigned int act_threshold;
+	int ret;
+
+	/* Measurement stays enabled, reading from regmap cache */
+
+	if (type != IIO_EV_TYPE_MAG)
+		return -EINVAL;
+
+	if (info != IIO_EV_INFO_VALUE)
+		return -EINVAL;
+
+	switch (dir) {
+	case IIO_EV_DIR_RISING:
+		ret = regmap_read(data->regmap,
+				  adxl313_act_thresh_reg[ADXL313_ACTIVITY],
+				  &act_threshold);
+		if (ret)
+			return ret;
+		*val = act_threshold * 15625;
+		*val2 = MICRO;
+		return IIO_VAL_FRACTIONAL;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int adxl313_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 adxl313_data *data = iio_priv(indio_dev);
+	unsigned int regval;
+	int ret;
+
+	ret = adxl313_set_measure_en(data, false);
+	if (ret)
+		return ret;
+
+	if (type != IIO_EV_TYPE_MAG)
+		return -EINVAL;
+
+	if (info != IIO_EV_INFO_VALUE)
+		return -EINVAL;
+
+	/* Scale factor 15.625 mg/LSB */
+	regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
+	switch (dir) {
+	case IIO_EV_DIR_RISING:
+		ret = regmap_write(data->regmap,
+				   adxl313_act_thresh_reg[ADXL313_ACTIVITY],
+				   regval);
+		if (ret)
+			return ret;
+		return adxl313_set_measure_en(data, true);
+	default:
+		return -EINVAL;
+	}
+}
+
 static int adxl313_set_watermark(struct iio_dev *indio_dev, unsigned int value)
 {
 	struct adxl313_data *data = iio_priv(indio_dev);
@@ -502,19 +705,32 @@ static int adxl313_fifo_push(struct iio_dev *indio_dev, int samples)
 
 static int adxl313_push_event(struct iio_dev *indio_dev, int int_stat)
 {
+	s64 ts = iio_get_time_ns(indio_dev);
 	struct adxl313_data *data = iio_priv(indio_dev);
 	int samples;
+	int ret = -ENOENT;
+
+	if (FIELD_GET(ADXL313_INT_ACTIVITY, 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_RISING),
+				     ts);
+		if (ret)
+			return ret;
+	}
 
 	if (FIELD_GET(ADXL313_INT_WATERMARK, int_stat)) {
 		samples = adxl313_get_samples(data);
 		if (samples < 0)
 			return samples;
 
-		return adxl313_fifo_push(indio_dev, samples);
+		ret = adxl313_fifo_push(indio_dev, samples);
 	}
 
 	/* Return error if no event data was pushed to the IIO channel. */
-	return -ENOENT;
+	return ret;
 }
 
 static irqreturn_t adxl313_irq_handler(int irq, void *p)
@@ -553,6 +769,10 @@ static int adxl313_reg_access(struct iio_dev *indio_dev, unsigned int reg,
 static const struct iio_info adxl313_info = {
 	.read_raw	= adxl313_read_raw,
 	.write_raw	= adxl313_write_raw,
+	.read_event_config = adxl313_read_event_config,
+	.write_event_config = adxl313_write_event_config,
+	.read_event_value = adxl313_read_event_value,
+	.write_event_value = adxl313_write_event_value,
 	.read_avail	= adxl313_read_freq_avail,
 	.hwfifo_set_watermark = adxl313_set_watermark,
 	.debugfs_reg_access = &adxl313_reg_access,
@@ -666,6 +886,19 @@ int adxl313_core_probe(struct device *dev,
 		if (ret)
 			return ret;
 
+		/*
+		 * Reset or configure the registers with reasonable default
+		 * values. As having 0 in most cases may result in undesirable
+		 * behavior if the interrupts are enabled.
+		 */
+		ret = regmap_write(data->regmap, ADXL313_REG_ACT_INACT_CTL, 0x00);
+		if (ret)
+			return ret;
+
+		ret = regmap_write(data->regmap, ADXL313_REG_THRESH_ACT, 0x52);
+		if (ret)
+			return ret;
+
 		ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
 						  &adxl313_buffer_ops);
 		if (ret)
-- 
2.39.5

[PATCH v4 08/11] iio: accel: adxl313: add inactivity sensing

[Lothar Rubusch]

Extend the interrupt handler to process interrupts as inactivity events.
Add functions to set threshold and period registers for inactivity. Add
functions to enable / disable inactivity. Extend the fake iio channel to
deal with inactivity events on x, y and z combined with AND.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 drivers/iio/accel/adxl313.h      |   2 +
 drivers/iio/accel/adxl313_core.c | 159 +++++++++++++++++++++++++------
 2 files changed, 131 insertions(+), 30 deletions(-)

diff --git a/drivers/iio/accel/adxl313.h b/drivers/iio/accel/adxl313.h
index 4f4a9fd39f6d..d7e8cb44855b 100644
--- a/drivers/iio/accel/adxl313.h
+++ b/drivers/iio/accel/adxl313.h
@@ -18,6 +18,8 @@
 #define ADXL313_REG_SOFT_RESET		0x18
 #define ADXL313_REG_OFS_AXIS(index)	(0x1E + (index))
 #define ADXL313_REG_THRESH_ACT		0x24
+#define ADXL313_REG_THRESH_INACT	0x25
+#define ADXL313_REG_TIME_INACT		0x26
 #define ADXL313_REG_ACT_INACT_CTL	0x27
 #define ADXL313_REG_BW_RATE		0x2C
 #define ADXL313_REG_POWER_CTL		0x2D
diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
index e647c40575ab..c5767d56b0cb 100644
--- a/drivers/iio/accel/adxl313_core.c
+++ b/drivers/iio/accel/adxl313_core.c
@@ -28,18 +28,22 @@
 #define ADXL313_REG_XYZ_BASE			ADXL313_REG_DATA_AXIS(0)
 
 #define ADXL313_ACT_XYZ_EN			GENMASK(6, 4)
+#define ADXL313_INACT_XYZ_EN			GENMASK(2, 0)
 
 /* activity/inactivity */
 enum adxl313_activity_type {
 	ADXL313_ACTIVITY,
+	ADXL313_INACTIVITY,
 };
 
 static const unsigned int adxl313_act_int_reg[] = {
 	[ADXL313_ACTIVITY] = ADXL313_INT_ACTIVITY,
+	[ADXL313_INACTIVITY] = ADXL313_INT_INACTIVITY,
 };
 
 static const unsigned int adxl313_act_thresh_reg[] = {
 	[ADXL313_ACTIVITY] = ADXL313_REG_THRESH_ACT,
+	[ADXL313_INACTIVITY] = ADXL313_REG_THRESH_INACT,
 };
 
 static const struct regmap_range adxl312_readable_reg_range[] = {
@@ -253,6 +257,16 @@ static const struct iio_event_spec adxl313_activity_events[] = {
 	},
 };
 
+static const struct iio_event_spec adxl313_inactivity_events[] = {
+	{
+		.type = IIO_EV_TYPE_MAG,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
+		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
+			BIT(IIO_EV_INFO_PERIOD),
+	},
+};
+
 enum adxl313_chans {
 	chan_x, chan_y, chan_z,
 };
@@ -269,6 +283,14 @@ static const struct iio_chan_spec adxl313_channels[] = {
 		.event_spec = adxl313_activity_events,
 		.num_event_specs = ARRAY_SIZE(adxl313_activity_events),
 	},
+	{
+		.type = IIO_ACCEL,
+		.modified = 1,
+		.channel2 = IIO_MOD_X_AND_Y_AND_Z,
+		.scan_index = -1, /* Fake channel for axis AND'ing */
+		.event_spec = adxl313_inactivity_events,
+		.num_event_specs = ARRAY_SIZE(adxl313_inactivity_events),
+	},
 };
 
 static const unsigned long adxl313_scan_masks[] = {
@@ -331,6 +353,15 @@ static int adxl313_read_freq_avail(struct iio_dev *indio_dev,
 	}
 }
 
+static int adxl313_set_inact_time_s(struct adxl313_data *data,
+				    unsigned int val_s)
+{
+	unsigned int max_boundary = 255;
+	unsigned int val = min(val_s, max_boundary);
+
+	return regmap_write(data->regmap, ADXL313_REG_TIME_INACT, val);
+}
+
 static int adxl313_is_act_inact_en(struct adxl313_data *data,
 				   enum adxl313_activity_type type)
 {
@@ -343,10 +374,10 @@ static int adxl313_is_act_inact_en(struct adxl313_data *data,
 		return ret;
 
 	/* Check if axis for activity are enabled */
-	if (type != ADXL313_ACTIVITY)
-		return 0;
-
-	axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
+	if (type == ADXL313_ACTIVITY)
+		axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
+	else
+		axis_en = FIELD_GET(ADXL313_INACT_XYZ_EN, axis_ctrl);
 
 	/* The axis are enabled, now check if specific interrupt is enabled */
 	ret = regmap_read(data->regmap, ADXL313_REG_INT_ENABLE, &regval);
@@ -364,12 +395,14 @@ static int adxl313_set_act_inact_en(struct adxl313_data *data,
 {
 	unsigned int axis_ctrl;
 	unsigned int threshold;
+	unsigned int inact_time_s;
+	bool en;
 	int ret;
 
-	if (type != ADXL313_ACTIVITY)
-		return 0;
-
-	axis_ctrl = ADXL313_ACT_XYZ_EN;
+	if (type == ADXL313_ACTIVITY)
+		axis_ctrl = ADXL313_ACT_XYZ_EN;
+	else
+		axis_ctrl = ADXL313_INACT_XYZ_EN;
 
 	ret = adxl313_set_measure_en(data, false);
 	if (ret)
@@ -384,9 +417,17 @@ static int adxl313_set_act_inact_en(struct adxl313_data *data,
 	if (ret)
 		return ret;
 
+	en = cmd_en && threshold;
+	if (type == ADXL313_INACTIVITY) {
+		ret = regmap_read(data->regmap, ADXL313_REG_TIME_INACT, &inact_time_s);
+		if (ret)
+			return ret;
+
+		en = en && inact_time_s;
+	}
+
 	ret = regmap_assign_bits(data->regmap, ADXL313_REG_INT_ENABLE,
-				 adxl313_act_int_reg[type],
-				 cmd_en && threshold);
+				 adxl313_act_int_reg[type], en);
 	if (ret)
 		return ret;
 
@@ -479,6 +520,8 @@ static int adxl313_read_event_config(struct iio_dev *indio_dev,
 	switch (dir) {
 	case IIO_EV_DIR_RISING:
 		return adxl313_is_act_inact_en(data, ADXL313_ACTIVITY);
+	case IIO_EV_DIR_FALLING:
+		return adxl313_is_act_inact_en(data, ADXL313_INACTIVITY);
 	default:
 		return -EINVAL;
 	}
@@ -498,6 +541,8 @@ static int adxl313_write_event_config(struct iio_dev *indio_dev,
 	switch (dir) {
 	case IIO_EV_DIR_RISING:
 		return adxl313_set_act_inact_en(data, ADXL313_ACTIVITY, state);
+	case IIO_EV_DIR_FALLING:
+		return adxl313_set_act_inact_en(data, ADXL313_INACTIVITY, state);
 	default:
 		return -EINVAL;
 	}
@@ -512,6 +557,8 @@ static int adxl313_read_event_value(struct iio_dev *indio_dev,
 {
 	struct adxl313_data *data = iio_priv(indio_dev);
 	unsigned int act_threshold;
+	unsigned int inact_threshold;
+	unsigned int inact_time_s;
 	int ret;
 
 	/* Measurement stays enabled, reading from regmap cache */
@@ -519,19 +566,38 @@ static int adxl313_read_event_value(struct iio_dev *indio_dev,
 	if (type != IIO_EV_TYPE_MAG)
 		return -EINVAL;
 
-	if (info != IIO_EV_INFO_VALUE)
-		return -EINVAL;
-
-	switch (dir) {
-	case IIO_EV_DIR_RISING:
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			ret = regmap_read(data->regmap,
+					  adxl313_act_thresh_reg[ADXL313_ACTIVITY],
+					  &act_threshold);
+			if (ret)
+				return ret;
+			*val = act_threshold * 15625;
+			*val2 = MICRO;
+			return IIO_VAL_FRACTIONAL;
+		case IIO_EV_DIR_FALLING:
+			ret = regmap_read(data->regmap,
+					  adxl313_act_thresh_reg[ADXL313_INACTIVITY],
+					  &inact_threshold);
+			if (ret)
+				return ret;
+			*val = inact_threshold * 15625;
+			*val2 = MICRO;
+			return IIO_VAL_FRACTIONAL;
+		default:
+			return -EINVAL;
+		}
+	case IIO_EV_INFO_PERIOD:
 		ret = regmap_read(data->regmap,
-				  adxl313_act_thresh_reg[ADXL313_ACTIVITY],
-				  &act_threshold);
+				  ADXL313_REG_TIME_INACT,
+				  &inact_time_s);
 		if (ret)
 			return ret;
-		*val = act_threshold * 15625;
-		*val2 = MICRO;
-		return IIO_VAL_FRACTIONAL;
+		*val = inact_time_s;
+		return IIO_VAL_INT;
 	default:
 		return -EINVAL;
 	}
@@ -555,16 +621,30 @@ static int adxl313_write_event_value(struct iio_dev *indio_dev,
 	if (type != IIO_EV_TYPE_MAG)
 		return -EINVAL;
 
-	if (info != IIO_EV_INFO_VALUE)
-		return -EINVAL;
-
-	/* Scale factor 15.625 mg/LSB */
-	regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
-	switch (dir) {
-	case IIO_EV_DIR_RISING:
-		ret = regmap_write(data->regmap,
-				   adxl313_act_thresh_reg[ADXL313_ACTIVITY],
-				   regval);
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		/* Scale factor 15.625 mg/LSB */
+		regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			ret = regmap_write(data->regmap,
+					   adxl313_act_thresh_reg[ADXL313_ACTIVITY],
+					   regval);
+			if (ret)
+				return ret;
+			return adxl313_set_measure_en(data, true);
+		case IIO_EV_DIR_FALLING:
+			ret = regmap_write(data->regmap,
+					   adxl313_act_thresh_reg[ADXL313_INACTIVITY],
+					   regval);
+			if (ret)
+				return ret;
+			return adxl313_set_measure_en(data, true);
+		default:
+			return -EINVAL;
+		}
+	case IIO_EV_INFO_PERIOD:
+		ret = adxl313_set_inact_time_s(data, val);
 		if (ret)
 			return ret;
 		return adxl313_set_measure_en(data, true);
@@ -721,6 +801,17 @@ static int adxl313_push_event(struct iio_dev *indio_dev, int int_stat)
 			return ret;
 	}
 
+	if (FIELD_GET(ADXL313_INT_INACTIVITY, int_stat)) {
+		ret = iio_push_event(indio_dev,
+				     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
+							IIO_MOD_X_AND_Y_AND_Z,
+							IIO_EV_TYPE_MAG,
+							IIO_EV_DIR_FALLING),
+				     ts);
+		if (ret)
+			return ret;
+	}
+
 	if (FIELD_GET(ADXL313_INT_WATERMARK, int_stat)) {
 		samples = adxl313_get_samples(data);
 		if (samples < 0)
@@ -895,6 +986,14 @@ int adxl313_core_probe(struct device *dev,
 		if (ret)
 			return ret;
 
+		ret = regmap_write(data->regmap, ADXL313_REG_TIME_INACT, 5);
+		if (ret)
+			return ret;
+
+		ret = regmap_write(data->regmap, ADXL313_REG_THRESH_INACT, 0x4f);
+		if (ret)
+			return ret;
+
 		ret = regmap_write(data->regmap, ADXL313_REG_THRESH_ACT, 0x52);
 		if (ret)
 			return ret;
-- 
2.39.5

[PATCH v4 09/11] iio: accel: adxl313: implement power-save on inactivity

[Lothar Rubusch]

Link activity and inactivity to indicate the internal power-saving state.
Add auto-sleep to be linked to inactivity.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 drivers/iio/accel/adxl313.h      |  3 +++
 drivers/iio/accel/adxl313_core.c | 20 ++++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/drivers/iio/accel/adxl313.h b/drivers/iio/accel/adxl313.h
index d7e8cb44855b..75ef54b60f75 100644
--- a/drivers/iio/accel/adxl313.h
+++ b/drivers/iio/accel/adxl313.h
@@ -41,6 +41,9 @@
 #define ADXL313_RATE_BASE		6
 
 #define ADXL313_POWER_CTL_MSK		BIT(3)
+#define ADXL313_POWER_CTL_INACT_MSK	GENMASK(5, 4)
+#define ADXL313_POWER_CTL_LINK		BIT(5)
+#define ADXL313_POWER_CTL_AUTO_SLEEP	BIT(4)
 
 #define ADXL313_RANGE_MSK		GENMASK(1, 0)
 #define ADXL313_RANGE_MAX		3
diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
index c5767d56b0cb..1598562a38e2 100644
--- a/drivers/iio/accel/adxl313_core.c
+++ b/drivers/iio/accel/adxl313_core.c
@@ -396,6 +396,7 @@ static int adxl313_set_act_inact_en(struct adxl313_data *data,
 	unsigned int axis_ctrl;
 	unsigned int threshold;
 	unsigned int inact_time_s;
+	int act_en, inact_en;
 	bool en;
 	int ret;
 
@@ -431,6 +432,25 @@ static int adxl313_set_act_inact_en(struct adxl313_data *data,
 	if (ret)
 		return ret;
 
+	/*
+	 * Set sleep and link bit only when ACT and INACT are enabled.
+	 */
+	act_en = adxl313_is_act_inact_en(data, ADXL313_ACTIVITY);
+	if (act_en < 0)
+		return act_en;
+
+	inact_en = adxl313_is_act_inact_en(data, ADXL313_INACTIVITY);
+	if (inact_en < 0)
+		return inact_en;
+
+	en = en && act_en && inact_en;
+
+	ret = regmap_assign_bits(data->regmap, ADXL313_REG_POWER_CTL,
+				 (ADXL313_POWER_CTL_AUTO_SLEEP | ADXL313_POWER_CTL_LINK),
+				 en);
+	if (ret)
+		return ret;
+
 	return adxl313_set_measure_en(data, true);
 }
 
-- 
2.39.5

[PATCH v4 10/11] iio: accel: adxl313: add AC coupled activity/inactivity events

[Lothar Rubusch]

Add AC coupling activity and inactivity as MAG_ADAPTIVE events. This adds
up an additional set of threshold and period handles, verifies matching
disabling functionality and extends setting the link bit to complementary
event configurations.

This means, e.g. either ACTIVITY or ACTIVITY_AC can be enabled. The most
recent set will remain configured. Disabling ACTIVITY where ACTIVITY_AC was
enabled is ignored, since it does not match (should be disabling
ACTIVITY_AC). When INACTIVITY or INACTIVITY_AC is also enabled, the link
bit will be set. Note, having the link bit and auto-sleep in place activity
and inactivity indicate the power save state change and thus will only be
triggered once a state transition occurs. Since there is a separate AC bit
for ACTIVITY and for INACTIVITY, events can be linked independently from
each other i.e. ACTIVITY can be linked to INACTIVITY_AC for instance.

When one of both is disabled, the link bit will be removed. Hence, the
remaining event will not indicate a plain state change anymore, but occur
as a periodically triggered inactivity event or for each activity event
above the threshold.

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

diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
index 1598562a38e2..9a0905e30de3 100644
--- a/drivers/iio/accel/adxl313_core.c
+++ b/drivers/iio/accel/adxl313_core.c
@@ -30,20 +30,38 @@
 #define ADXL313_ACT_XYZ_EN			GENMASK(6, 4)
 #define ADXL313_INACT_XYZ_EN			GENMASK(2, 0)
 
+#define ADXL313_REG_ACT_ACDC_MSK		BIT(7)
+#define ADXL313_REG_INACT_ACDC_MSK		BIT(3)
+#define ADXL313_COUPLING_DC			0
+#define ADXL313_COUPLING_AC			1
+
 /* activity/inactivity */
 enum adxl313_activity_type {
 	ADXL313_ACTIVITY,
 	ADXL313_INACTIVITY,
+	ADXL313_ACTIVITY_AC,
+	ADXL313_INACTIVITY_AC,
 };
 
 static const unsigned int adxl313_act_int_reg[] = {
 	[ADXL313_ACTIVITY] = ADXL313_INT_ACTIVITY,
 	[ADXL313_INACTIVITY] = ADXL313_INT_INACTIVITY,
+	[ADXL313_ACTIVITY_AC] = ADXL313_INT_ACTIVITY,
+	[ADXL313_INACTIVITY_AC] = ADXL313_INT_INACTIVITY,
 };
 
 static const unsigned int adxl313_act_thresh_reg[] = {
 	[ADXL313_ACTIVITY] = ADXL313_REG_THRESH_ACT,
 	[ADXL313_INACTIVITY] = ADXL313_REG_THRESH_INACT,
+	[ADXL313_ACTIVITY_AC] = ADXL313_REG_THRESH_ACT,
+	[ADXL313_INACTIVITY_AC] = ADXL313_REG_THRESH_INACT,
+};
+
+static const unsigned int adxl313_act_acdc_msk[] = {
+	[ADXL313_ACTIVITY] = ADXL313_REG_ACT_ACDC_MSK,
+	[ADXL313_INACTIVITY] = ADXL313_REG_INACT_ACDC_MSK,
+	[ADXL313_ACTIVITY_AC] = ADXL313_REG_ACT_ACDC_MSK,
+	[ADXL313_INACTIVITY_AC] = ADXL313_REG_INACT_ACDC_MSK,
 };
 
 static const struct regmap_range adxl312_readable_reg_range[] = {
@@ -255,6 +273,13 @@ static const struct iio_event_spec adxl313_activity_events[] = {
 		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
 		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE),
 	},
+	{
+		/* activity, AC bit set */
+		.type = IIO_EV_TYPE_MAG_ADAPTIVE,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
+		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE),
+	},
 };
 
 static const struct iio_event_spec adxl313_inactivity_events[] = {
@@ -265,6 +290,14 @@ static const struct iio_event_spec adxl313_inactivity_events[] = {
 		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
 			BIT(IIO_EV_INFO_PERIOD),
 	},
+	{
+		/* inactivity, AC bit set */
+		.type = IIO_EV_TYPE_MAG_ADAPTIVE,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
+		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
+			BIT(IIO_EV_INFO_PERIOD),
+	},
 };
 
 enum adxl313_chans {
@@ -362,11 +395,58 @@ static int adxl313_set_inact_time_s(struct adxl313_data *data,
 	return regmap_write(data->regmap, ADXL313_REG_TIME_INACT, val);
 }
 
+/**
+ * adxl313_is_act_inact_ac() - Check if AC coupling is enabled.
+ *
+ * @data: The device data.
+ * @type: The activity or inactivity type.
+ *
+ * Provide a type of activity or inactivity, combined with either AC coupling
+ * set, or default to DC coupling. This function verifies, if the combination is
+ * currently enabled or not.
+ *
+ * Return if the provided activity type has AC coupling enabled or a negative
+ * error value.
+ */
+static int adxl313_is_act_inact_ac(struct adxl313_data *data,
+				   enum adxl313_activity_type type)
+{
+	unsigned int regval;
+	bool coupling;
+	int ret;
+
+	ret = regmap_read(data->regmap, ADXL313_REG_ACT_INACT_CTL, &regval);
+	if (ret)
+		return ret;
+
+	coupling = adxl313_act_acdc_msk[type] & regval;
+
+	if (type == ADXL313_ACTIVITY || type == ADXL313_INACTIVITY)
+		return coupling == ADXL313_COUPLING_DC;
+	else
+		return coupling == ADXL313_COUPLING_AC;
+}
+
+static int adxl313_set_act_inact_ac(struct adxl313_data *data,
+				    enum adxl313_activity_type type)
+{
+	unsigned int coupling;
+
+	if (type == ADXL313_ACTIVITY_AC || type == ADXL313_INACTIVITY_AC)
+		coupling = ADXL313_COUPLING_AC;
+	else
+		coupling = ADXL313_COUPLING_DC;
+
+	return regmap_assign_bits(data->regmap, ADXL313_REG_ACT_INACT_CTL,
+				  adxl313_act_acdc_msk[type], coupling);
+}
+
 static int adxl313_is_act_inact_en(struct adxl313_data *data,
 				   enum adxl313_activity_type type)
 {
 	unsigned int axis_ctrl;
 	unsigned int regval;
+	int coupling;
 	int axis_en, int_en, ret;
 
 	ret = regmap_read(data->regmap, ADXL313_REG_ACT_INACT_CTL, &axis_ctrl);
@@ -374,7 +454,7 @@ static int adxl313_is_act_inact_en(struct adxl313_data *data,
 		return ret;
 
 	/* Check if axis for activity are enabled */
-	if (type == ADXL313_ACTIVITY)
+	if (type == ADXL313_ACTIVITY || type == ADXL313_ACTIVITY_AC)
 		axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
 	else
 		axis_en = FIELD_GET(ADXL313_INACT_XYZ_EN, axis_ctrl);
@@ -386,7 +466,12 @@ static int adxl313_is_act_inact_en(struct adxl313_data *data,
 
 	int_en = adxl313_act_int_reg[type] & regval;
 
-	return axis_en && int_en;
+	/* Return true if configured coupling matches provided type */
+	coupling = adxl313_is_act_inact_ac(data, type);
+	if (coupling < 0)
+		return coupling;
+
+	return axis_en && int_en && coupling;
 }
 
 static int adxl313_set_act_inact_en(struct adxl313_data *data,
@@ -396,15 +481,26 @@ static int adxl313_set_act_inact_en(struct adxl313_data *data,
 	unsigned int axis_ctrl;
 	unsigned int threshold;
 	unsigned int inact_time_s;
-	int act_en, inact_en;
-	bool en;
+	int act_en, inact_en, act_ac_en, inact_ac_en;
+	bool en, act_inact_ac;
 	int ret;
 
-	if (type == ADXL313_ACTIVITY)
+	/*
+	 * In case of turning off, assure turning off a correspondent coupling
+	 * event. In case of not matching coupling, simply return.
+	 */
+	if (!cmd_en) {
+		/* Expected positive true if coupling matches coupling type */
+		if (adxl313_is_act_inact_ac(data, type) <= 0)
+			return 0;
+	}
+
+	if (type == ADXL313_ACTIVITY || type == ADXL313_ACTIVITY_AC)
 		axis_ctrl = ADXL313_ACT_XYZ_EN;
 	else
 		axis_ctrl = ADXL313_INACT_XYZ_EN;
 
+	/* Start modifying configuration registers */
 	ret = adxl313_set_measure_en(data, false);
 	if (ret)
 		return ret;
@@ -414,12 +510,16 @@ static int adxl313_set_act_inact_en(struct adxl313_data *data,
 	if (ret)
 		return ret;
 
+	act_inact_ac = type == ADXL313_ACTIVITY_AC || ADXL313_INACTIVITY_AC;
+	ret = regmap_assign_bits(data->regmap, ADXL313_REG_ACT_INACT_CTL,
+				 adxl313_act_acdc_msk[type], act_inact_ac);
+
 	ret = regmap_read(data->regmap, adxl313_act_thresh_reg[type], &threshold);
 	if (ret)
 		return ret;
 
 	en = cmd_en && threshold;
-	if (type == ADXL313_INACTIVITY) {
+	if (type == ADXL313_INACTIVITY || type == ADXL313_INACTIVITY_AC) {
 		ret = regmap_read(data->regmap, ADXL313_REG_TIME_INACT, &inact_time_s);
 		if (ret)
 			return ret;
@@ -427,6 +527,10 @@ static int adxl313_set_act_inact_en(struct adxl313_data *data,
 		en = en && inact_time_s;
 	}
 
+	ret = adxl313_set_act_inact_ac(data, type);
+	if (ret)
+		return ret;
+
 	ret = regmap_assign_bits(data->regmap, ADXL313_REG_INT_ENABLE,
 				 adxl313_act_int_reg[type], en);
 	if (ret)
@@ -439,10 +543,22 @@ static int adxl313_set_act_inact_en(struct adxl313_data *data,
 	if (act_en < 0)
 		return act_en;
 
+	act_ac_en = adxl313_is_act_inact_en(data, ADXL313_ACTIVITY_AC);
+	if (act_ac_en < 0)
+		return act_ac_en;
+
+	act_en = act_en || act_ac_en;
+
 	inact_en = adxl313_is_act_inact_en(data, ADXL313_INACTIVITY);
 	if (inact_en < 0)
 		return inact_en;
 
+	inact_ac_en = adxl313_is_act_inact_en(data, ADXL313_INACTIVITY_AC);
+	if (inact_ac_en < 0)
+		return inact_ac_en;
+
+	inact_en = inact_en || inact_ac_en;
+
 	en = en && act_en && inact_en;
 
 	ret = regmap_assign_bits(data->regmap, ADXL313_REG_POWER_CTL,
@@ -534,14 +650,25 @@ static int adxl313_read_event_config(struct iio_dev *indio_dev,
 {
 	struct adxl313_data *data = iio_priv(indio_dev);
 
-	if (type != IIO_EV_TYPE_MAG)
-		return -EINVAL;
-
-	switch (dir) {
-	case IIO_EV_DIR_RISING:
-		return adxl313_is_act_inact_en(data, ADXL313_ACTIVITY);
-	case IIO_EV_DIR_FALLING:
-		return adxl313_is_act_inact_en(data, ADXL313_INACTIVITY);
+	switch (type) {
+	case IIO_EV_TYPE_MAG:
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			return adxl313_is_act_inact_en(data, ADXL313_ACTIVITY);
+		case IIO_EV_DIR_FALLING:
+			return adxl313_is_act_inact_en(data, ADXL313_INACTIVITY);
+		default:
+			return -EINVAL;
+		}
+	case IIO_EV_TYPE_MAG_ADAPTIVE:
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			return adxl313_is_act_inact_en(data, ADXL313_ACTIVITY_AC);
+		case IIO_EV_DIR_FALLING:
+			return adxl313_is_act_inact_en(data, ADXL313_INACTIVITY_AC);
+		default:
+			return -EINVAL;
+		}
 	default:
 		return -EINVAL;
 	}
@@ -555,14 +682,33 @@ static int adxl313_write_event_config(struct iio_dev *indio_dev,
 {
 	struct adxl313_data *data = iio_priv(indio_dev);
 
-	if (type != IIO_EV_TYPE_MAG)
-		return -EINVAL;
-
-	switch (dir) {
-	case IIO_EV_DIR_RISING:
-		return adxl313_set_act_inact_en(data, ADXL313_ACTIVITY, state);
-	case IIO_EV_DIR_FALLING:
-		return adxl313_set_act_inact_en(data, ADXL313_INACTIVITY, state);
+	switch (type) {
+	case IIO_EV_TYPE_MAG:
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			return adxl313_set_act_inact_en(data,
+							ADXL313_ACTIVITY,
+							state);
+		case IIO_EV_DIR_FALLING:
+			return adxl313_set_act_inact_en(data,
+							ADXL313_INACTIVITY,
+							state);
+		default:
+			return -EINVAL;
+		}
+	case IIO_EV_TYPE_MAG_ADAPTIVE:
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			return adxl313_set_act_inact_en(data,
+							ADXL313_ACTIVITY_AC,
+							state);
+		case IIO_EV_DIR_FALLING:
+			return adxl313_set_act_inact_en(data,
+							ADXL313_INACTIVITY_AC,
+							state);
+		default:
+			return -EINVAL;
+		}
 	default:
 		return -EINVAL;
 	}
@@ -583,41 +729,79 @@ static int adxl313_read_event_value(struct iio_dev *indio_dev,
 
 	/* Measurement stays enabled, reading from regmap cache */
 
-	if (type != IIO_EV_TYPE_MAG)
-		return -EINVAL;
-
-	switch (info) {
-	case IIO_EV_INFO_VALUE:
-		switch (dir) {
-		case IIO_EV_DIR_RISING:
+	switch (type) {
+	case IIO_EV_TYPE_MAG:
+		switch (info) {
+		case IIO_EV_INFO_VALUE:
+			switch (dir) {
+			case IIO_EV_DIR_RISING:
+				ret = regmap_read(data->regmap,
+						  adxl313_act_thresh_reg[ADXL313_ACTIVITY],
+						  &act_threshold);
+				if (ret)
+					return ret;
+				*val = act_threshold * 15625;
+				*val2 = MICRO;
+				return IIO_VAL_FRACTIONAL;
+			case IIO_EV_DIR_FALLING:
+				ret = regmap_read(data->regmap,
+						  adxl313_act_thresh_reg[ADXL313_INACTIVITY],
+						  &inact_threshold);
+				if (ret)
+					return ret;
+				*val = inact_threshold * 15625;
+				*val2 = MICRO;
+				return IIO_VAL_FRACTIONAL;
+			default:
+				return -EINVAL;
+			}
+		case IIO_EV_INFO_PERIOD:
 			ret = regmap_read(data->regmap,
-					  adxl313_act_thresh_reg[ADXL313_ACTIVITY],
-					  &act_threshold);
+					  ADXL313_REG_TIME_INACT,
+					  &inact_time_s);
 			if (ret)
 				return ret;
-			*val = act_threshold * 15625;
-			*val2 = MICRO;
-			return IIO_VAL_FRACTIONAL;
-		case IIO_EV_DIR_FALLING:
+			*val = inact_time_s;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	case IIO_EV_TYPE_MAG_ADAPTIVE:
+		switch (info) {
+		case IIO_EV_INFO_VALUE:
+			switch (dir) {
+			case IIO_EV_DIR_RISING:
+				ret = regmap_read(data->regmap,
+						  adxl313_act_thresh_reg[ADXL313_ACTIVITY_AC],
+						  &act_threshold);
+				if (ret)
+					return ret;
+				*val = act_threshold * 15625;
+				*val2 = MICRO;
+				return IIO_VAL_FRACTIONAL;
+			case IIO_EV_DIR_FALLING:
+				ret = regmap_read(data->regmap,
+						  adxl313_act_thresh_reg[ADXL313_INACTIVITY_AC],
+						  &inact_threshold);
+				if (ret)
+					return ret;
+				*val = inact_threshold * 15625;
+				*val2 = MICRO;
+				return IIO_VAL_FRACTIONAL;
+			default:
+				return -EINVAL;
+			}
+		case IIO_EV_INFO_PERIOD:
 			ret = regmap_read(data->regmap,
-					  adxl313_act_thresh_reg[ADXL313_INACTIVITY],
-					  &inact_threshold);
+					  ADXL313_REG_TIME_INACT,
+					  &inact_time_s);
 			if (ret)
 				return ret;
-			*val = inact_threshold * 15625;
-			*val2 = MICRO;
-			return IIO_VAL_FRACTIONAL;
+			*val = inact_time_s;
+			return IIO_VAL_INT;
 		default:
 			return -EINVAL;
 		}
-	case IIO_EV_INFO_PERIOD:
-		ret = regmap_read(data->regmap,
-				  ADXL313_REG_TIME_INACT,
-				  &inact_time_s);
-		if (ret)
-			return ret;
-		*val = inact_time_s;
-		return IIO_VAL_INT;
 	default:
 		return -EINVAL;
 	}
@@ -638,36 +822,69 @@ static int adxl313_write_event_value(struct iio_dev *indio_dev,
 	if (ret)
 		return ret;
 
-	if (type != IIO_EV_TYPE_MAG)
-		return -EINVAL;
-
-	switch (info) {
-	case IIO_EV_INFO_VALUE:
-		/* Scale factor 15.625 mg/LSB */
-		regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
-		switch (dir) {
-		case IIO_EV_DIR_RISING:
-			ret = regmap_write(data->regmap,
-					   adxl313_act_thresh_reg[ADXL313_ACTIVITY],
-					   regval);
+	switch (type) {
+	case IIO_EV_TYPE_MAG:
+		switch (info) {
+		case IIO_EV_INFO_VALUE:
+			/* Scale factor 15.625 mg/LSB */
+			regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
+			switch (dir) {
+			case IIO_EV_DIR_RISING:
+				ret = regmap_write(data->regmap,
+						   adxl313_act_thresh_reg[ADXL313_ACTIVITY],
+						   regval);
+				if (ret)
+					return ret;
+				return adxl313_set_measure_en(data, true);
+			case IIO_EV_DIR_FALLING:
+				ret = regmap_write(data->regmap,
+						   adxl313_act_thresh_reg[ADXL313_INACTIVITY],
+						   regval);
+				if (ret)
+					return ret;
+				return adxl313_set_measure_en(data, true);
+			default:
+				return -EINVAL;
+			}
+		case IIO_EV_INFO_PERIOD:
+			ret = adxl313_set_inact_time_s(data, val);
 			if (ret)
 				return ret;
 			return adxl313_set_measure_en(data, true);
-		case IIO_EV_DIR_FALLING:
-			ret = regmap_write(data->regmap,
-					   adxl313_act_thresh_reg[ADXL313_INACTIVITY],
-					   regval);
+		default:
+			return -EINVAL;
+		}
+	case IIO_EV_TYPE_MAG_ADAPTIVE:
+		switch (info) {
+		case IIO_EV_INFO_VALUE:
+			/* Scale factor 15.625 mg/LSB */
+			regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
+			switch (dir) {
+			case IIO_EV_DIR_RISING:
+				ret = regmap_write(data->regmap,
+						   adxl313_act_thresh_reg[ADXL313_ACTIVITY_AC],
+						   regval);
+				if (ret)
+					return ret;
+				return adxl313_set_measure_en(data, true);
+			case IIO_EV_DIR_FALLING:
+				ret = regmap_write(data->regmap,
+						   adxl313_act_thresh_reg[ADXL313_INACTIVITY_AC],
+						   regval);
+				if (ret)
+					return ret;
+				return adxl313_set_measure_en(data, true);
+			default:
+				return -EINVAL;
+			}
+		case IIO_EV_INFO_PERIOD:
+			ret = adxl313_set_inact_time_s(data, val);
 			if (ret)
 				return ret;
 			return adxl313_set_measure_en(data, true);
 		default:
 			return -EINVAL;
 		}
-	case IIO_EV_INFO_PERIOD:
-		ret = adxl313_set_inact_time_s(data, val);
-		if (ret)
-			return ret;
-		return adxl313_set_measure_en(data, true);
 	default:
 		return -EINVAL;
 	}
@@ -807,29 +1024,64 @@ static int adxl313_push_event(struct iio_dev *indio_dev, int int_stat)
 {
 	s64 ts = iio_get_time_ns(indio_dev);
 	struct adxl313_data *data = iio_priv(indio_dev);
+	unsigned int regval;
 	int samples;
 	int ret = -ENOENT;
 
 	if (FIELD_GET(ADXL313_INT_ACTIVITY, 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_RISING),
-				     ts);
+		ret = regmap_read(data->regmap, ADXL313_REG_ACT_INACT_CTL, &regval);
 		if (ret)
 			return ret;
+
+		if (FIELD_GET(ADXL313_REG_ACT_ACDC_MSK, regval)) {
+			/* AC coupled */
+			ret = iio_push_event(indio_dev,
+					     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
+								IIO_MOD_X_OR_Y_OR_Z,
+								IIO_EV_TYPE_MAG_ADAPTIVE,
+								IIO_EV_DIR_RISING),
+					     ts);
+			if (ret)
+				return ret;
+		} else {
+			/* DC coupled, relying on THRESH */
+			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_RISING),
+					     ts);
+			if (ret)
+				return ret;
+		}
 	}
 
 	if (FIELD_GET(ADXL313_INT_INACTIVITY, int_stat)) {
-		ret = iio_push_event(indio_dev,
-				     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
-							IIO_MOD_X_AND_Y_AND_Z,
-							IIO_EV_TYPE_MAG,
-							IIO_EV_DIR_FALLING),
-				     ts);
+		ret = regmap_read(data->regmap, ADXL313_REG_ACT_INACT_CTL, &regval);
 		if (ret)
 			return ret;
+
+		if (FIELD_GET(ADXL313_REG_INACT_ACDC_MSK, regval)) {
+			/* AC coupled */
+			ret = iio_push_event(indio_dev,
+					     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
+								IIO_MOD_X_AND_Y_AND_Z,
+								IIO_EV_TYPE_MAG_ADAPTIVE,
+								IIO_EV_DIR_FALLING),
+					     ts);
+			if (ret)
+				return ret;
+		} else {
+			/* DC coupled, relying on THRESH */
+			ret = iio_push_event(indio_dev,
+					     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
+								IIO_MOD_X_AND_Y_AND_Z,
+								IIO_EV_TYPE_MAG,
+								IIO_EV_DIR_FALLING),
+					     ts);
+			if (ret)
+				return ret;
+		}
 	}
 
 	if (FIELD_GET(ADXL313_INT_WATERMARK, int_stat)) {
-- 
2.39.5

[PATCH v4 11/11] docs: iio: add ADXL313 accelerometer

[Lothar Rubusch]

Add documentation for the ADXL313 accelerometer driver.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 Documentation/iio/adxl313.rst | 289 ++++++++++++++++++++++++++++++++++
 Documentation/iio/index.rst   |   1 +
 2 files changed, 290 insertions(+)
 create mode 100644 Documentation/iio/adxl313.rst

diff --git a/Documentation/iio/adxl313.rst b/Documentation/iio/adxl313.rst
new file mode 100644
index 000000000000..e67171513f4a
--- /dev/null
+++ b/Documentation/iio/adxl313.rst
@@ -0,0 +1,289 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+===============
+ADXL313 driver
+===============
+
+This driver supports Analog Device's ADXL313 on SPI/I2C bus.
+
+1. Supported devices
+====================
+
+* `ADXL313 <https://www.analog.com/ADXL313>`_
+
+The ADXL313is a low noise density, low power, 3-axis accelerometer with
+selectable measurement ranges. The ADXL313 supports the ±0.5 g, ±1 g, ±2 g and
+±4 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 tables show the adxl313 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_scale                                    | Scale for the accelerometer channels.                    |
++---------------------------------------------------+----------------------------------------------------------+
+| 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.                  |
++---------------------------------------------------+----------------------------------------------------------+
+
++---------------------------------------+----------------------------------------------+
+| Miscellaneous device files            | Description                                  |
++---------------------------------------+----------------------------------------------+
+| name                                  | Name of the IIO device.                      |
++---------------------------------------+----------------------------------------------+
+| in_accel_sampling_frequency           | Currently selected sample rate.              |
++---------------------------------------+----------------------------------------------+
+| in_accel_sampling_frequency_available | Available sampling frequency configurations. |
++---------------------------------------+----------------------------------------------+
+
+The iio event related settings, found in ``/sys/bus/iio/devices/iio:deviceX/events``.
+
++---------------------------------------------------+----------------------------------------------------------+
+| in_accel_mag_adaptive_falling_period              | AC coupled inactivity time.                              |
++---------------------------------------------------+----------------------------------------------------------+
+| in_accel_mag_adaptive_falling_value               | AC coupled inactivity threshold.                         |
++---------------------------------------------------+----------------------------------------------------------+
+| in_accel_mag_adaptive_rising_value                | AC coupled activity threshold.                           |
++---------------------------------------------------+----------------------------------------------------------+
+| in_accel_mag_falling_period                       | Inactivity time.                                         |
++---------------------------------------------------+----------------------------------------------------------+
+| in_accel_mag_falling_value                        | Inactivity threshold.                                    |
++---------------------------------------------------+----------------------------------------------------------+
+| in_accel_mag_rising_value                         | Activity threshold.                                      |
++---------------------------------------------------+----------------------------------------------------------+
+| in_accel_x\&y\&z_mag_adaptive_falling_en          | Enable or disable AC coupled inactivity events.          |
++---------------------------------------------------+----------------------------------------------------------+
+| in_accel_x\|y\|z_mag_adaptive_rising_en           | Enable or disable AC coupled activity events.            |
++---------------------------------------------------+----------------------------------------------------------+
+| in_accel_x\&y\&z_mag_falling_en                   | Enable or disable inactivity events.                     |
++---------------------------------------------------+----------------------------------------------------------+
+| in_accel_x\|y\|z_mag_rising_en                    | Enable or disable activity events.                       |
++---------------------------------------------------+----------------------------------------------------------+
+
+The default coupling is DC coupled events. In this case the threshold will
+be in place as such, where for the AC coupled case an adaptive threshold
+(described in the datasheet) will be applied by the sensor. In general activity,
+i.e. ``ACTIVITY`` or ``ACTIVITY_AC`` and inactivity i.e. ``INACTIVITY`` or
+``INACTIVITY_AC``, will be linked with auto-sleep enabled when both are enabled.
+This means in particular ``ACTIVITY`` can also be linked to ``INACTIVITY_AC``
+and vice versa, without problem.
+
+Note here, that ``ACTIVITY`` and ``ACTIVITY_AC`` are mutually exclusive. This
+means, that the most recent configuration will be set. For instance, if
+``ACTIVITY`` is enabled, and ``ACTIVITY_AC`` will be enabled, the sensor driver
+will have ``ACTIVITY`` disabled, but ``ACTIVITY_AC`` enabled. The same is valid
+for inactivity. In case of turning off an event, it has to match to what is
+actually enabled, i.e. enabling ``ACTIVITY_AC`` and then disabling ``ACTIVITY``
+is simply ignored as it is already disabled. Or, as if it was any other not
+enabled event, too.
+
+Channels 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::
+
+        processed value = (_raw + _offset) * _scale
+
+Where _offset and _scale are device attributes. If no _offset attribute is
+present, simply assume its value is 0.
+
+The ADXL313 driver offers data for a single types of channels, the table below
+shows the measurement units for the processed value, which are defined by the
+IIO framework:
+
++-------------------------------------+---------------------------+
+| Channel type                        | Measurement unit          |
++-------------------------------------+---------------------------+
+| Acceleration on X, Y, and Z axis    | Meters per Second squared |
++-------------------------------------+---------------------------+
+
+Usage examples
+--------------
+
+Show device name:
+
+.. code-block:: bash
+
+        root:/sys/bus/iio/devices/iio:device0> cat name
+        adxl313
+
+Show accelerometer channels value:
+
+.. code-block:: bash
+
+        root:/sys/bus/iio/devices/iio:device0> cat in_accel_x_raw
+        2
+        root:/sys/bus/iio/devices/iio:device0> cat in_accel_y_raw
+        -57
+        root:/sys/bus/iio/devices/iio:device0> cat in_accel_z_raw
+        2
+        root:/sys/bus/iio/devices/iio:device0> cat in_accel_scale
+        0.009576806
+
+The accelerometer values will be:
+
+- X-axis acceleration = in_accel_x_raw * in_accel_scale = 0.0191536 m/s^2
+- Y-axis acceleration = in_accel_y_raw * in_accel_scale = -0.5458779 m/s^2
+- Z-axis acceleration = in_accel_z_raw * in_accel_scale = 0.0191536 m/s^2
+
+Set calibration offset for accelerometer channels. Note, that the calibration
+will be rounded according to the graduation of LSB units:
+
+.. 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
+        48
+
+Set sampling frequency:
+
+.. code-block:: bash
+
+        root:/sys/bus/iio/devices/iio:device0> cat in_accel_sampling_frequency
+        100.000000
+        root:/sys/bus/iio/devices/iio:device0> cat in_accel_sampling_frequency_available
+        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 400 > in_accel_sampling_frequency
+        root:/sys/bus/iio/devices/iio:device0> cat in_accel_sampling_frequency
+        400.000000
+
+3. Device buffers and triggers
+==============================
+
+This driver supports IIO buffers.
+
+All devices support retrieving the raw acceleration 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:/sys/bus/iio/devices/iio:device0> hexdump -C /dev/iio\:device0
+        ...
+        000000d0  01 fc 31 00 c7 ff 03 fc  31 00 c7 ff 04 fc 33 00  |..1.....1.....3.|
+        000000e0  c8 ff 03 fc 32 00 c5 ff  ff fc 32 00 c7 ff 0a fc  |....2.....2.....|
+        000000f0  30 00 c8 ff 06 fc 33 00  c7 ff 01 fc 2f 00 c8 ff  |0.....3...../...|
+        00000100  02 fc 32 00 c6 ff 04 fc  33 00 c8 ff 05 fc 33 00  |..2.....3.....3.|
+        00000110  ca ff 02 fc 31 00 c7 ff  02 fc 30 00 c9 ff 09 fc  |....1.....0.....|
+        00000120  35 00 c9 ff 08 fc 35 00  c8 ff 02 fc 31 00 c5 ff  |5.....5.....1...|
+        00000130  03 fc 32 00 c7 ff 04 fc  32 00 c7 ff 02 fc 31 00  |..2.....2.....1.|
+        00000140  c7 ff 08 fc 30 00 c7 ff  02 fc 32 00 c5 ff ff fc  |....0.....2.....|
+        00000150  31 00 c5 ff 04 fc 31 00  c8 ff 03 fc 32 00 c8 ff  |1.....1.....2...|
+        00000160  01 fc 31 00 c7 ff 05 fc  31 00 c3 ff 04 fc 31 00  |..1.....1.....1.|
+        00000170  c5 ff 04 fc 30 00 c7 ff  03 fc 31 00 c9 ff 03 fc  |....0.....1.....|
+        ...
+
+Enabling activity detection:
+
+.. code-block:: bash
+        root:/sys/bus/iio/devices/iio:device0> echo 1.28125 > ./events/in_accel_mag_rising_value
+        root:/sys/bus/iio/devices/iio:device0> echo 1 > ./events/in_accel_x\|y\|z_mag_rising_en
+
+        root:/sys/bus/iio/devices/iio:device0> iio_event_monitor adxl313
+        Found IIO device with name adxl313 with device number 0
+        <only while moving the sensor>
+        Event: time: 1748795762298351281, type: accel(x|y|z), channel: 0, evtype: mag, direction: rising
+        Event: time: 1748795762302653704, type: accel(x|y|z), channel: 0, evtype: mag, direction: rising
+        Event: time: 1748795762304340726, type: accel(x|y|z), channel: 0, evtype: mag, direction: rising
+        ...
+
+Disabling activity detection:
+
+.. code-block:: bash
+        root:/sys/bus/iio/devices/iio:device0> echo 0 > ./events/in_accel_x\|y\|z_mag_rising_en
+        root:/sys/bus/iio/devices/iio:device0> iio_event_monitor adxl313
+        <nothing>
+
+Enabling inactivity detection:
+
+.. code-block:: bash
+        root:/sys/bus/iio/devices/iio:device0> echo 1.234375 > ./events/in_accel_mag_falling_value
+        root:/sys/bus/iio/devices/iio:device0> echo 5 > ./events/in_accel_mag_falling_period
+        root:/sys/bus/iio/devices/iio:device0> echo 1 > ./events/in_accel_x\&y\&z_mag_falling_en
+
+        root:/sys/bus/iio/devices/iio:device0> iio_event_monitor adxl313
+        Found IIO device with name adxl313 with device number 0
+        Event: time: 1748796324115962975, type: accel(x&y&z), channel: 0, evtype: mag, direction: falling
+        Event: time: 1748796329329981772, type: accel(x&y&z), channel: 0, evtype: mag, direction: falling
+        Event: time: 1748796334543399706, type: accel(x&y&z), channel: 0, evtype: mag, direction: falling
+        ...
+        <every 5s now indicates inactivity>
+
+Now, enabling activity, e.g. the AC coupled counter-part ``ACTIVITY_AC``
+
+.. code-block:: bash
+        root:/sys/bus/iio/devices/iio:device0> echo 1.28125 > ./events/in_accel_mag_rising_value
+        root:/sys/bus/iio/devices/iio:device0> echo 1 > ./events/in_accel_x\|y\|z_mag_rising_en
+
+        root:/sys/bus/iio/devices/iio:device0> iio_event_monitor adxl313
+        Found IIO device with name adxl313 with device number 0
+        <some activity with the sensor>
+        Event: time: 1748796880354686777, type: accel(x|y|z), channel: 0, evtype: mag_adaptive, direction: rising
+        <5s of inactivity, then>
+        Event: time: 1748796885543252017, type: accel(x&y&z), channel: 0, evtype: mag, direction: falling
+        <some other activity detected by accelerating the sensor>
+        Event: time: 1748796887756634678, type: accel(x|y|z), channel: 0, evtype: mag_adaptive, direction: rising
+        <again, 5s of inactivity>
+        Event: time: 1748796892964368352, type: accel(x&y&z), channel: 0, evtype: mag, direction: falling
+        <stays like this until next activity in auto-sleep>
+
+Note, when AC coupling is in place, the event type will be of ``mag_adaptive``.
+AC- or DC-coupled (the default) events are used similiarly.
+
+4. IIO Interfacing Tools
+========================
+
+See Documentation/iio/iio_tools.rst for the description of the available IIO
+interfacing tools.
diff --git a/Documentation/iio/index.rst b/Documentation/iio/index.rst
index 2d6afc5a8ed5..c106402a91f7 100644
--- a/Documentation/iio/index.rst
+++ b/Documentation/iio/index.rst
@@ -31,6 +31,7 @@ Industrial I/O Kernel Drivers
    adis16475
    adis16480
    adis16550
+   adxl313
    adxl380
    bno055
    ep93xx_adc
-- 
2.39.5

Re: [PATCH v4 01/11] iio: accel: adxl313: add debug register

[Andy Shevchenko]

On Sun, Jun 1, 2025 at 8:21 PM Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> Add iio debug register for general sensor debugging.

IIO

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 02/11] iio: accel: adxl313: introduce channel buffer

[Andy Shevchenko]

On Sun, Jun 1, 2025 at 8:21 PM Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> Add a scan_mask and scan_index to the iio channel. The scan_index

IIO

> prepares the buffer usage. According to the datasheet, the ADXL313
> uses 13 bit in full resolution. Add signedness, storage bits and

bits
...OR...
13-bit wide data field

> endianness.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 03/11] iio: accel: adxl313: make use of regmap cache

[Andy Shevchenko]

On Sun, Jun 1, 2025 at 8:21 PM Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> Setup regmap cache to cache register configuration. This is a preparatory
> step for follow up patches. Using cached settings will help at interrupt

interrupt

> handling, to generate activity and inactivity events.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 04/11] iio: accel: adxl313: add function to enable measurement

[Andy Shevchenko]

On Sun, Jun 1, 2025 at 8:21 PM Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> Rework controlling measurement and standby of the sensor. Therefore,
> replace writing the register directly by encapsulating this and dealing
> with the return value in a separte function to enable and disable

separate

(Please, go with a spell check before sending a new version)

> measurement. This will help to avoid redundant code in all locations
> where the sensor configuration needs to be adjusted, thus measurement will
> be set to standby, in follow up patches.
>
> Further, reduce the control mask to only the measurement bit. The sleep bit
> actually controls a different behavior (not just putting the sensor to
> standby for configuration, but turning it into sleep mode) and it is not
> used so far. In consequence, there is no need to cover sleep bit and

cover the sleep

> measurement with the same mask.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 05/11] iio: accel: adxl313: prepare interrupt handling

[Andy Shevchenko]

On Sun, Jun 1, 2025 at 8:21 PM Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> Evaluate the devicetree property for an optional interrupt line, and
> configure the interrupt mapping accordingly. When no interrupt line
> is defined in the devicetree, keep the FIFO in bypass mode as before.

...

>         struct adxl313_data *data;
>         struct iio_dev *indio_dev;
> -       int ret;
> +       u8 int_line;
> +       u8 int_map_msk;
> +       int irq, ret;

Why do you need a specific irq variable?

...

> +       int_line = ADXL313_INT1;

Assign this when we are sure that the INT1 is defined. Current
approach is not robust.

> +       irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT1");
> +       if (irq < 0) {
> +               int_line = ADXL313_INT2;
> +               irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT2");
> +               if (irq < 0)
> +                       int_line = ADXL313_INT_NONE;
> +       }

So, the below code does not use the returned vIRQ, moreover, the above
code actually does the IRQ mapping. Why do you need that if the code
doesn't use it?

> +       if (int_line != ADXL313_INT_NONE) {

Why not positive conditional? But see below...

> +               /* FIFO_STREAM mode */
> +               int_map_msk = ADXL313_INT_DREADY | ADXL313_INT_ACTIVITY |
> +                       ADXL313_INT_INACTIVITY | ADXL313_INT_WATERMARK |
> +                       ADXL313_INT_OVERRUN;
> +               ret = regmap_assign_bits(data->regmap, ADXL313_REG_INT_MAP,
> +                                        int_map_msk, int_line == ADXL313_INT2);

This is fragile. It heavily relies on the existence of exactly three
IRQ variants. Instead of defining special case (NONE) simply use
whatever is undefined as the default case

  switch (IRQ type) {
  case 'INT1':
    ...
    break;
  case 'INT2':
    ...
    break;
  default:
    // FIFO bypass mode
    ...
    break;
  }

But still, the main question and confusion here is the absence of the
users of 'irq'.

> +               if (ret)
> +                       return ret;
> +       } else {
> +               /*
> +                * FIFO_BYPASSED mode
> +                *
> +                * When no interrupt lines are specified, the driver falls back
> +                * to use the sensor in FIFO_BYPASS mode. This means turning off
> +                * internal FIFO and interrupt generation (since there is no
> +                * line specified). Unmaskable interrupts such as overrun or
> +                * data ready won't interfere. Even that a FIFO_STREAM mode w/o
> +                * connected interrupt line might allow for obtaining raw
> +                * measurements, a fallback to disable interrupts when no
> +                * interrupt lines are connected seems to be the cleaner
> +                * solution.
> +                */
> +               ret = regmap_write(data->regmap, ADXL313_REG_FIFO_CTL,
> +                                  FIELD_PREP(ADXL313_REG_FIFO_CTL_MODE_MSK,
> +                                             ADXL313_FIFO_BYPASS));
> +               if (ret)
> +                       return ret;
> +       }
> +
>         return devm_iio_device_register(dev, indio_dev);
>  }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 06/11] iio: accel: adxl313: add basic interrupt handling for FIFO watermark

[Andy Shevchenko]

On Sun, Jun 1, 2025 at 8:22 PM Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> Prepare the interrupt handler. Add register entries to evaluate the
> incoming interrupt. Add functions to clear status registers and reset the
> FIFO.
>
> Add FIFO watermark configuration and evaluation. Let a watermark to be
> configured. Evaluate the interrupt accordingly. Read out the FIFO content
> and push the values to the IIO channel.

...

> +static int adxl313_set_watermark(struct iio_dev *indio_dev, unsigned int value)
> +{
> +       struct adxl313_data *data = iio_priv(indio_dev);
> +       const unsigned int fifo_mask = 0x1f, interrupt_mask = 0x02;

GENMASK()
BIT()

> +       int ret;
> +
> +       value = min(value, ADXL313_FIFO_SIZE - 1);
> +
> +       ret = regmap_update_bits(data->regmap, ADXL313_REG_FIFO_CTL,
> +                                fifo_mask, value);
> +       if (ret)
> +               return ret;
> +
> +       data->watermark = value;
> +
> +       return regmap_update_bits(data->regmap, ADXL313_REG_INT_ENABLE,
> +                                 interrupt_mask, ADXL313_INT_WATERMARK);
> +}

...

> +static int adxl313_get_samples(struct adxl313_data *data)
> +{
> +       unsigned int regval = 0;

Useless assignment.

> +       int ret;
> +
> +       ret = regmap_read(data->regmap, ADXL313_REG_FIFO_STATUS, &regval);
> +       if (ret)
> +               return ret;
> +
> +       return FIELD_GET(ADXL313_REG_FIFO_STATUS_ENTRIES_MSK, regval);
> +}

...

> +               ret = devm_request_threaded_irq(dev, irq, NULL,
> +                                               &adxl313_irq_handler,
> +                                               IRQF_SHARED | IRQF_ONESHOT,
> +                                               indio_dev->name, indio_dev);
> +               if (ret)
> +                       return ret;

Now I see the first user of 'irq'. Logically these two patches may not
be split. Or split should be made differently, let's say IRQ type
holding variable + switch case can go in the first preparatory patch
(however it will make a little sense without real users, as it is/will
be a dead code).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 07/11] iio: accel: adxl313: add activity sensing

[Andy Shevchenko]

On Sun, Jun 1, 2025 at 8:22 PM Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> Add possibilities to set a threshold for activity sensing. Extend the
> interrupt handler to process activity interrupts. Provide functions to set
> the activity threshold and to enable/disable activity sensing. Further add
> a fake channel for having x, y and z axis anded on the iio channel.

IIO

And what does the 'anded' mean?

> This is a preparatory patch. Some of the definitions and functions are
> supposed to be extended for inactivity later on.

...

> +static int adxl313_is_act_inact_en(struct adxl313_data *data,
> +                                  enum adxl313_activity_type type)
> +{
> +       unsigned int axis_ctrl;
> +       unsigned int regval;
> +       int axis_en, int_en, ret;
> +
> +       ret = regmap_read(data->regmap, ADXL313_REG_ACT_INACT_CTL, &axis_ctrl);
> +       if (ret)
> +               return ret;
> +
> +       /* Check if axis for activity are enabled */
> +       if (type != ADXL313_ACTIVITY)
> +               return 0;
> +
> +       axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);

If it's false, it will be false anyway. No need to defer the check:

  if (!axis_en)
    return false;

> +       /* The axis are enabled, now check if specific interrupt is enabled */
> +       ret = regmap_read(data->regmap, ADXL313_REG_INT_ENABLE, &regval);
> +       if (ret)
> +               return ret;
> +
> +       int_en = adxl313_act_int_reg[type] & regval;
> +
> +       return axis_en && int_en;

  return ... & regval;

> +}

I have already commented on this a couple of times.

...

> +       /* Scale factor 15.625 mg/LSB */
> +       regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);

I would rather do

val * MICRO + val2

which is read more naturally (we will easily get that the expression
uses MICRO scale).

...

> +       int ret = -ENOENT;
> +
> +       if (FIELD_GET(ADXL313_INT_ACTIVITY, 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_RISING),
> +                                    ts);
> +               if (ret)
> +                       return ret;
> +       }
>
>         if (FIELD_GET(ADXL313_INT_WATERMARK, int_stat)) {
>                 samples = adxl313_get_samples(data);
>                 if (samples < 0)
>                         return samples;
>
> -               return adxl313_fifo_push(indio_dev, samples);
> +               ret = adxl313_fifo_push(indio_dev, samples);

This is not needed...

>         }
>
>         /* Return error if no event data was pushed to the IIO channel. */
> -       return -ENOENT;
> +       return ret;

...and this looks wrong.

Before the case was clear, if we have no respective bit set in the
int_stat, we return ENOENT. Now it depends on the other bit. If this
is correct behaviour, it needs a comment.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 08/11] iio: accel: adxl313: add inactivity sensing

[Andy Shevchenko]

On Sun, Jun 1, 2025 at 8:22 PM Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> Extend the interrupt handler to process interrupts as inactivity events.
> Add functions to set threshold and period registers for inactivity. Add
> functions to enable / disable inactivity. Extend the fake iio channel to

IIO

> deal with inactivity events on x, y and z combined with AND.

...

> +static int adxl313_set_inact_time_s(struct adxl313_data *data,
> +                                   unsigned int val_s)
> +{
> +       unsigned int max_boundary = 255;

This is unclear how it's defined. What is the limit behind? Size of a
bit field? Decimal value from the datasheet?

The forms of (BIT(8) - 1) or GENMASK(7, 0) may be better depending on
the answers to the above questions.

> +       unsigned int val = min(val_s, max_boundary);
> +
> +       return regmap_write(data->regmap, ADXL313_REG_TIME_INACT, val);
> +}

...

> -       axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
> +       if (type == ADXL313_ACTIVITY)
> +               axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
> +       else
> +               axis_en = FIELD_GET(ADXL313_INACT_XYZ_EN, axis_ctrl);

Even with this change my previous comment stays.

...

> +       en = cmd_en && threshold;
> +       if (type == ADXL313_INACTIVITY) {
> +               ret = regmap_read(data->regmap, ADXL313_REG_TIME_INACT, &inact_time_s);
> +               if (ret)
> +                       return ret;
> +
> +               en = en && inact_time_s;
> +       }

...

> -       if (info != IIO_EV_INFO_VALUE)
> -               return -EINVAL;
> -
> -       /* Scale factor 15.625 mg/LSB */
> -       regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
> -       switch (dir) {
> -       case IIO_EV_DIR_RISING:
> -               ret = regmap_write(data->regmap,
> -                                  adxl313_act_thresh_reg[ADXL313_ACTIVITY],
> -                                  regval);

Hmm... This was added by the previous patches, right? Why can't it be
done as a switch case to begin with? I remember one of the previous
versions had some nested switch-cases, perhaps you need to rethink on
how to split the code between functions to avoid too much nesting (add
some helper functions?).

> +       switch (info) {
> +       case IIO_EV_INFO_VALUE:
> +               /* Scale factor 15.625 mg/LSB */
> +               regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
> +               switch (dir) {
> +               case IIO_EV_DIR_RISING:
> +                       ret = regmap_write(data->regmap,
> +                                          adxl313_act_thresh_reg[ADXL313_ACTIVITY],
> +                                          regval);
> +                       if (ret)
> +                               return ret;
> +                       return adxl313_set_measure_en(data, true);
> +               case IIO_EV_DIR_FALLING:
> +                       ret = regmap_write(data->regmap,
> +                                          adxl313_act_thresh_reg[ADXL313_INACTIVITY],
> +                                          regval);
> +                       if (ret)
> +                               return ret;
> +                       return adxl313_set_measure_en(data, true);
> +               default:
> +                       return -EINVAL;
> +               }
> +       case IIO_EV_INFO_PERIOD:
> +               ret = adxl313_set_inact_time_s(data, val);
>                 if (ret)
>                         return ret;
>                 return adxl313_set_measure_en(data, true);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 10/11] iio: accel: adxl313: add AC coupled activity/inactivity events

[Andy Shevchenko]

On Sun, Jun 1, 2025 at 8:22 PM Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> Add AC coupling activity and inactivity as MAG_ADAPTIVE events. This adds
> up an additional set of threshold and period handles, verifies matching
> disabling functionality and extends setting the link bit to complementary
> event configurations.
>
> This means, e.g. either ACTIVITY or ACTIVITY_AC can be enabled. The most
> recent set will remain configured. Disabling ACTIVITY where ACTIVITY_AC was
> enabled is ignored, since it does not match (should be disabling
> ACTIVITY_AC). When INACTIVITY or INACTIVITY_AC is also enabled, the link
> bit will be set. Note, having the link bit and auto-sleep in place activity
> and inactivity indicate the power save state change and thus will only be
> triggered once a state transition occurs. Since there is a separate AC bit
> for ACTIVITY and for INACTIVITY, events can be linked independently from
> each other i.e. ACTIVITY can be linked to INACTIVITY_AC for instance.
>
> When one of both is disabled, the link bit will be removed. Hence, the
> remaining event will not indicate a plain state change anymore, but occur
> as a periodically triggered inactivity event or for each activity event
> above the threshold.

...

> +/**
> + * adxl313_is_act_inact_ac() - Check if AC coupling is enabled.

> + *

Unneeded blank line.

> + * @data: The device data.
> + * @type: The activity or inactivity type.
> + *
> + * Provide a type of activity or inactivity, combined with either AC coupling
> + * set, or default to DC coupling. This function verifies, if the combination is
> + * currently enabled or not.
> + *
> + * Return if the provided activity type has AC coupling enabled or a negative
> + * error value.

Missing Return section. Always try kernel-doc validation when adding
new kernel-doc descriptions.

> + */

...

>         unsigned int regval;
> +       int coupling;

Why? Doesn't 'ret' suffice?

>         int axis_en, int_en, ret;

...

> -       int act_en, inact_en;
> -       bool en;
> +       int act_en, inact_en, act_ac_en, inact_ac_en;
> +       bool en, act_inact_ac;
>         int ret;

For all your patches: try really hard to avoid the ping-pong coding,
i.e. when you add something in one patch in the series and change in
the other for no reason. I.o.w. when the initial code may be written
already in a form that doesn't need further changes (e.g., switch-case
vs. if).

This patch is *very* noisy due to the above. So, just slow down, try a
new approach that you have less '-' lines in the diff:s all over the
code.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 00/11] iio: accel: adxl313: add power-save on activity/inactivity

[Bagas Sanjaya]

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

On Sun, Jun 01, 2025 at 05:21:28PM +0000, Lothar Rubusch wrote:
> The patch set covers the following topics:
> - add debug register and regmap cache
> - prepare iio channel scan_type and scan_index
> - prepare interrupt handling
> - implement fifo with watermark
> - add activity/inactivity together with auto-sleep with link bit
> - add ac coupled activity/inactivity, integrate with auto-sleep and link bit
> - documentation

The series doesn't cleanly apply on iio/testing tree. Base commit or tree?

Confused...

-- 
An old man doll... just what I always wanted! - Clara

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

Re: [PATCH v4 01/11] iio: accel: adxl313: add debug register

[Jonathan Cameron]

On Sun, 1 Jun 2025 22:06:27 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sun, Jun 1, 2025 at 8:21 PM Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >
> > Add iio debug register for general sensor debugging.  
> 
> IIO
> 

Tweaked whilst applying this patch.  Patch stands on it's own so I'll pick it
up now even if the rest of the series needs another version.

Re: [PATCH v4 02/11] iio: accel: adxl313: introduce channel buffer

[Jonathan Cameron]

On Sun,  1 Jun 2025 17:21:30 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add a scan_mask and scan_index to the iio channel. The scan_index
> prepares the buffer usage. According to the datasheet, the ADXL313
> uses 13 bit in full resolution. Add signedness, storage bits and
> endianness.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>

I'd normally expect to see this in the same patch where it is first used.

There is little benefit in adding unused data on it's own - so combine this
with patch 6.  If there was something particularly unusual to discuss
and highlight for review, a separate patch might make sense, but I'm not
seeing that here.

Jonathan

> ---
>  drivers/iio/accel/adxl313_core.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
> index 2f26da5857d4..06a771bb4726 100644
> --- a/drivers/iio/accel/adxl313_core.c
> +++ b/drivers/iio/accel/adxl313_core.c
> @@ -171,9 +171,10 @@ static const int adxl313_odr_freqs[][2] = {
>  	[9] = { 3200, 0 },
>  };
>  
> -#define ADXL313_ACCEL_CHANNEL(index, axis) {				\
> +#define ADXL313_ACCEL_CHANNEL(index, reg, axis) {			\
>  	.type = IIO_ACCEL,						\
> -	.address = index,						\
> +	.scan_index = (index),						\
> +	.address = (reg),						\
>  	.modified = 1,							\
>  	.channel2 = IIO_MOD_##axis,					\
>  	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
> @@ -183,14 +184,26 @@ static const int adxl313_odr_freqs[][2] = {
>  	.info_mask_shared_by_type_available =				\
>  		BIT(IIO_CHAN_INFO_SAMP_FREQ),				\
>  	.scan_type = {							\
> +		.sign = 's',						\
>  		.realbits = 13,						\
> +		.storagebits = 16,					\
> +		.endianness = IIO_BE,					\
>  	},								\
>  }
>  
> +enum adxl313_chans {
> +	chan_x, chan_y, chan_z,
> +};
> +
>  static const struct iio_chan_spec adxl313_channels[] = {
> -	ADXL313_ACCEL_CHANNEL(0, X),
> -	ADXL313_ACCEL_CHANNEL(1, Y),
> -	ADXL313_ACCEL_CHANNEL(2, Z),
> +	ADXL313_ACCEL_CHANNEL(0, chan_x, X),
> +	ADXL313_ACCEL_CHANNEL(1, chan_y, Y),
> +	ADXL313_ACCEL_CHANNEL(2, chan_z, Z),
> +};
> +
> +static const unsigned long adxl313_scan_masks[] = {
> +	BIT(chan_x) | BIT(chan_y) | BIT(chan_z),
> +	0
>  };
>  
>  static int adxl313_set_odr(struct adxl313_data *data,
> @@ -419,6 +432,7 @@ int adxl313_core_probe(struct device *dev,
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->channels = adxl313_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(adxl313_channels);
> +	indio_dev->available_scan_masks = adxl313_scan_masks;
>  
>  	ret = adxl313_setup(dev, data, setup);
>  	if (ret) {

Re: [PATCH v4 03/11] iio: accel: adxl313: make use of regmap cache

[Jonathan Cameron]

On Sun,  1 Jun 2025 17:21:31 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Setup regmap cache to cache register configuration. This is a preparatory
> step for follow up patches. Using cached settings will help at inerrupt
> handling, to generate activity and inactivity events.

The regmap cache will reduce traffic to the device for things like reading
back sampling frequency, so no need to justify this patch with 'future'
stuff.  Justify it with current.   I've applied with the description of
simply

"Setup regmap cache to cache register configuration, reducing bus traffic
for repeated accesses to non volatile registers."

> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl313.h      |  2 ++
>  drivers/iio/accel/adxl313_core.c | 17 +++++++++++++++++
>  drivers/iio/accel/adxl313_i2c.c  |  6 ++++++
>  drivers/iio/accel/adxl313_spi.c  |  6 ++++++
>  4 files changed, 31 insertions(+)
> 
> diff --git a/drivers/iio/accel/adxl313.h b/drivers/iio/accel/adxl313.h
> index 72f624af4686..fc937bdf83b6 100644
> --- a/drivers/iio/accel/adxl313.h
> +++ b/drivers/iio/accel/adxl313.h
> @@ -54,6 +54,8 @@ extern const struct regmap_access_table adxl312_writable_regs_table;
>  extern const struct regmap_access_table adxl313_writable_regs_table;
>  extern const struct regmap_access_table adxl314_writable_regs_table;
>  
> +bool adxl313_is_volatile_reg(struct device *dev, unsigned int reg);
> +
>  enum adxl313_device_type {
>  	ADXL312,
>  	ADXL313,
> diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
> index 06a771bb4726..0c893c286017 100644
> --- a/drivers/iio/accel/adxl313_core.c
> +++ b/drivers/iio/accel/adxl313_core.c
> @@ -46,6 +46,23 @@ const struct regmap_access_table adxl314_readable_regs_table = {
>  };
>  EXPORT_SYMBOL_NS_GPL(adxl314_readable_regs_table, IIO_ADXL313);
>  
> +bool adxl313_is_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case ADXL313_REG_DATA_AXIS(0):
> +	case ADXL313_REG_DATA_AXIS(1):
> +	case ADXL313_REG_DATA_AXIS(2):
> +	case ADXL313_REG_DATA_AXIS(3):
> +	case ADXL313_REG_DATA_AXIS(4):
> +	case ADXL313_REG_DATA_AXIS(5):
> +	case ADXL313_REG_FIFO_STATUS:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +EXPORT_SYMBOL_NS_GPL(adxl313_is_volatile_reg, "IIO_ADXL313");
> +
>  static int adxl312_check_id(struct device *dev,
>  			    struct adxl313_data *data)
>  {
> diff --git a/drivers/iio/accel/adxl313_i2c.c b/drivers/iio/accel/adxl313_i2c.c
> index a4cf0cf2c5aa..e8636e8ab14f 100644
> --- a/drivers/iio/accel/adxl313_i2c.c
> +++ b/drivers/iio/accel/adxl313_i2c.c
> @@ -21,6 +21,8 @@ static const struct regmap_config adxl31x_i2c_regmap_config[] = {
>  		.rd_table	= &adxl312_readable_regs_table,
>  		.wr_table	= &adxl312_writable_regs_table,
>  		.max_register	= 0x39,
> +		.volatile_reg	= adxl313_is_volatile_reg,
> +		.cache_type	= REGCACHE_MAPLE,
>  	},
>  	[ADXL313] = {
>  		.reg_bits	= 8,
> @@ -28,6 +30,8 @@ static const struct regmap_config adxl31x_i2c_regmap_config[] = {
>  		.rd_table	= &adxl313_readable_regs_table,
>  		.wr_table	= &adxl313_writable_regs_table,
>  		.max_register	= 0x39,
> +		.volatile_reg	= adxl313_is_volatile_reg,
> +		.cache_type	= REGCACHE_MAPLE,
>  	},
>  	[ADXL314] = {
>  		.reg_bits	= 8,
> @@ -35,6 +39,8 @@ static const struct regmap_config adxl31x_i2c_regmap_config[] = {
>  		.rd_table	= &adxl314_readable_regs_table,
>  		.wr_table	= &adxl314_writable_regs_table,
>  		.max_register	= 0x39,
> +		.volatile_reg	= adxl313_is_volatile_reg,
> +		.cache_type	= REGCACHE_MAPLE,
>  	},
>  };
>  
> diff --git a/drivers/iio/accel/adxl313_spi.c b/drivers/iio/accel/adxl313_spi.c
> index 9a16b40bff34..68e323e81aeb 100644
> --- a/drivers/iio/accel/adxl313_spi.c
> +++ b/drivers/iio/accel/adxl313_spi.c
> @@ -24,6 +24,8 @@ static const struct regmap_config adxl31x_spi_regmap_config[] = {
>  		.max_register	= 0x39,
>  		/* Setting bits 7 and 6 enables multiple-byte read */
>  		.read_flag_mask	= BIT(7) | BIT(6),
> +		.volatile_reg	= adxl313_is_volatile_reg,
> +		.cache_type	= REGCACHE_MAPLE,
>  	},
>  	[ADXL313] = {
>  		.reg_bits	= 8,
> @@ -33,6 +35,8 @@ static const struct regmap_config adxl31x_spi_regmap_config[] = {
>  		.max_register	= 0x39,
>  		/* Setting bits 7 and 6 enables multiple-byte read */
>  		.read_flag_mask	= BIT(7) | BIT(6),
> +		.volatile_reg	= adxl313_is_volatile_reg,
> +		.cache_type	= REGCACHE_MAPLE,
>  	},
>  	[ADXL314] = {
>  		.reg_bits	= 8,
> @@ -42,6 +46,8 @@ static const struct regmap_config adxl31x_spi_regmap_config[] = {
>  		.max_register	= 0x39,
>  		/* Setting bits 7 and 6 enables multiple-byte read */
>  		.read_flag_mask	= BIT(7) | BIT(6),
> +		.volatile_reg	= adxl313_is_volatile_reg,
> +		.cache_type	= REGCACHE_MAPLE,
>  	},
>  };
>  

Re: [PATCH v4 04/11] iio: accel: adxl313: add function to enable measurement

[Jonathan Cameron]

On Sun,  1 Jun 2025 17:21:32 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Rework controlling measurement and standby of the sensor. Therefore,
> replace writing the register directly by encapsulating this and dealing
> with the return value in a separte function to enable and disable
> measurement. This will help to avoid redundant code in all locations
> where the sensor configuration needs to be adjusted, thus measurement will
> be set to standby, in follow up patches.
> 
> Further, reduce the control mask to only the measurement bit. The sleep bit
> actually controls a different behavior (not just putting the sensor to
> standby for configuration, but turning it into sleep mode) and it is not
> used so far. In consequence, there is no need to cover sleep bit and
> measurement with the same mask.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
This is a good bit to have as a precursor patch (as you have done)
because it is refactoring the existing code.  It doesn't stand on it's
own though given for now there is only one caller, so I won't pick it up
until the patch that uses it is ready to go.

Jonathan

> ---
>  drivers/iio/accel/adxl313.h      |  3 +--
>  drivers/iio/accel/adxl313_core.c | 10 +++++++---
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl313.h b/drivers/iio/accel/adxl313.h
> index fc937bdf83b6..9bf2facdbf87 100644
> --- a/drivers/iio/accel/adxl313.h
> +++ b/drivers/iio/accel/adxl313.h
> @@ -36,8 +36,7 @@
>  #define ADXL313_RATE_MSK		GENMASK(3, 0)
>  #define ADXL313_RATE_BASE		6
>  
> -#define ADXL313_POWER_CTL_MSK		GENMASK(3, 2)
> -#define ADXL313_MEASUREMENT_MODE	BIT(3)
> +#define ADXL313_POWER_CTL_MSK		BIT(3)
>  
>  #define ADXL313_RANGE_MSK		GENMASK(1, 0)
>  #define ADXL313_RANGE_MAX		3
> diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
> index 0c893c286017..6170c9daa30f 100644
> --- a/drivers/iio/accel/adxl313_core.c
> +++ b/drivers/iio/accel/adxl313_core.c
> @@ -63,6 +63,12 @@ bool adxl313_is_volatile_reg(struct device *dev, unsigned int reg)
>  }
>  EXPORT_SYMBOL_NS_GPL(adxl313_is_volatile_reg, "IIO_ADXL313");
>  
> +static int adxl313_set_measure_en(struct adxl313_data *data, bool en)
> +{
> +	return regmap_assign_bits(data->regmap, ADXL313_REG_POWER_CTL,
> +				  ADXL313_POWER_CTL_MSK, en);
> +}
> +
>  static int adxl312_check_id(struct device *dev,
>  			    struct adxl313_data *data)
>  {
> @@ -410,9 +416,7 @@ static int adxl313_setup(struct device *dev, struct adxl313_data *data,
>  	}
>  
>  	/* Enables measurement mode */
> -	return regmap_update_bits(data->regmap, ADXL313_REG_POWER_CTL,
> -				  ADXL313_POWER_CTL_MSK,
> -				  ADXL313_MEASUREMENT_MODE);
> +	return adxl313_set_measure_en(data, true);
>  }
>  
>  /**

Re: [PATCH v4 06/11] iio: accel: adxl313: add basic interrupt handling for FIFO watermark

[Jonathan Cameron]

On Sun, 1 Jun 2025 22:26:42 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sun, Jun 1, 2025 at 8:22 PM Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >
> > Prepare the interrupt handler. Add register entries to evaluate the
> > incoming interrupt. Add functions to clear status registers and reset the
> > FIFO.
> >
> > Add FIFO watermark configuration and evaluation. Let a watermark to be
> > configured. Evaluate the interrupt accordingly. Read out the FIFO content
> > and push the values to the IIO channel.  
> 
> ...
> 
> > +static int adxl313_set_watermark(struct iio_dev *indio_dev, unsigned int value)
> > +{
> > +       struct adxl313_data *data = iio_priv(indio_dev);
> > +       const unsigned int fifo_mask = 0x1f, interrupt_mask = 0x02;  
> 
> GENMASK()
> BIT()
> 
> > +       int ret;
> > +
> > +       value = min(value, ADXL313_FIFO_SIZE - 1);
> > +
> > +       ret = regmap_update_bits(data->regmap, ADXL313_REG_FIFO_CTL,
> > +                                fifo_mask, value);
> > +       if (ret)
> > +               return ret;
> > +
> > +       data->watermark = value;
> > +
> > +       return regmap_update_bits(data->regmap, ADXL313_REG_INT_ENABLE,
> > +                                 interrupt_mask, ADXL313_INT_WATERMARK);
> > +}  
> 
> ...
> 
> > +static int adxl313_get_samples(struct adxl313_data *data)
> > +{
> > +       unsigned int regval = 0;  
> 
> Useless assignment.
> 
> > +       int ret;
> > +
> > +       ret = regmap_read(data->regmap, ADXL313_REG_FIFO_STATUS, &regval);
> > +       if (ret)
> > +               return ret;
> > +
> > +       return FIELD_GET(ADXL313_REG_FIFO_STATUS_ENTRIES_MSK, regval);
> > +}  
> 
> ...
> 
> > +               ret = devm_request_threaded_irq(dev, irq, NULL,
> > +                                               &adxl313_irq_handler,
> > +                                               IRQF_SHARED | IRQF_ONESHOT,
> > +                                               indio_dev->name, indio_dev);
> > +               if (ret)
> > +                       return ret;  
> 
> Now I see the first user of 'irq'. Logically these two patches may not
> be split. Or split should be made differently, let's say IRQ type
> holding variable + switch case can go in the first preparatory patch
> (however it will make a little sense without real users, as it is/will
> be a dead code).
> 

I'd just combine these two patches and patch 2 (which is also dead code
until this one is in place).

Jonathan

Re: [PATCH v4 03/11] iio: accel: adxl313: make use of regmap cache

[Jonathan Cameron]

On Sun, 8 Jun 2025 16:22:15 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Sun,  1 Jun 2025 17:21:31 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
> 
> > Setup regmap cache to cache register configuration. This is a preparatory
> > step for follow up patches. Using cached settings will help at inerrupt
> > handling, to generate activity and inactivity events.  
> 
> The regmap cache will reduce traffic to the device for things like reading
> back sampling frequency, so no need to justify this patch with 'future'
> stuff.  Justify it with current.   I've applied with the description of
> simply
> 
> "Setup regmap cache to cache register configuration, reducing bus traffic
> for repeated accesses to non volatile registers."
> 
Dropped again.  The is_volatile should include all volatile registers
not just ones we happen to be using so far. 

You added debug accesses in previous patch which will not take the volatile
nature into account unless the register is in that switch statement.

Put the all in from the start.

Jonathan

> > 
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > ---
> >  drivers/iio/accel/adxl313.h      |  2 ++
> >  drivers/iio/accel/adxl313_core.c | 17 +++++++++++++++++
> >  drivers/iio/accel/adxl313_i2c.c  |  6 ++++++
> >  drivers/iio/accel/adxl313_spi.c  |  6 ++++++
> >  4 files changed, 31 insertions(+)
> > 
> > diff --git a/drivers/iio/accel/adxl313.h b/drivers/iio/accel/adxl313.h
> > index 72f624af4686..fc937bdf83b6 100644
> > --- a/drivers/iio/accel/adxl313.h
> > +++ b/drivers/iio/accel/adxl313.h
> > @@ -54,6 +54,8 @@ extern const struct regmap_access_table adxl312_writable_regs_table;
> >  extern const struct regmap_access_table adxl313_writable_regs_table;
> >  extern const struct regmap_access_table adxl314_writable_regs_table;
> >  
> > +bool adxl313_is_volatile_reg(struct device *dev, unsigned int reg);
> > +
> >  enum adxl313_device_type {
> >  	ADXL312,
> >  	ADXL313,
> > diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
> > index 06a771bb4726..0c893c286017 100644
> > --- a/drivers/iio/accel/adxl313_core.c
> > +++ b/drivers/iio/accel/adxl313_core.c
> > @@ -46,6 +46,23 @@ const struct regmap_access_table adxl314_readable_regs_table = {
> >  };
> >  EXPORT_SYMBOL_NS_GPL(adxl314_readable_regs_table, IIO_ADXL313);
> >  
> > +bool adxl313_is_volatile_reg(struct device *dev, unsigned int reg)
> > +{
> > +	switch (reg) {
> > +	case ADXL313_REG_DATA_AXIS(0):
> > +	case ADXL313_REG_DATA_AXIS(1):
> > +	case ADXL313_REG_DATA_AXIS(2):
> > +	case ADXL313_REG_DATA_AXIS(3):
> > +	case ADXL313_REG_DATA_AXIS(4):
> > +	case ADXL313_REG_DATA_AXIS(5):
> > +	case ADXL313_REG_FIFO_STATUS:
> > +		return true;
> > +	default:
> > +		return false;
> > +	}
> > +}
> > +EXPORT_SYMBOL_NS_GPL(adxl313_is_volatile_reg, "IIO_ADXL313");
> > +
> >  static int adxl312_check_id(struct device *dev,
> >  			    struct adxl313_data *data)
> >  {
> > diff --git a/drivers/iio/accel/adxl313_i2c.c b/drivers/iio/accel/adxl313_i2c.c
> > index a4cf0cf2c5aa..e8636e8ab14f 100644
> > --- a/drivers/iio/accel/adxl313_i2c.c
> > +++ b/drivers/iio/accel/adxl313_i2c.c
> > @@ -21,6 +21,8 @@ static const struct regmap_config adxl31x_i2c_regmap_config[] = {
> >  		.rd_table	= &adxl312_readable_regs_table,
> >  		.wr_table	= &adxl312_writable_regs_table,
> >  		.max_register	= 0x39,
> > +		.volatile_reg	= adxl313_is_volatile_reg,
> > +		.cache_type	= REGCACHE_MAPLE,
> >  	},
> >  	[ADXL313] = {
> >  		.reg_bits	= 8,
> > @@ -28,6 +30,8 @@ static const struct regmap_config adxl31x_i2c_regmap_config[] = {
> >  		.rd_table	= &adxl313_readable_regs_table,
> >  		.wr_table	= &adxl313_writable_regs_table,
> >  		.max_register	= 0x39,
> > +		.volatile_reg	= adxl313_is_volatile_reg,
> > +		.cache_type	= REGCACHE_MAPLE,
> >  	},
> >  	[ADXL314] = {
> >  		.reg_bits	= 8,
> > @@ -35,6 +39,8 @@ static const struct regmap_config adxl31x_i2c_regmap_config[] = {
> >  		.rd_table	= &adxl314_readable_regs_table,
> >  		.wr_table	= &adxl314_writable_regs_table,
> >  		.max_register	= 0x39,
> > +		.volatile_reg	= adxl313_is_volatile_reg,
> > +		.cache_type	= REGCACHE_MAPLE,
> >  	},
> >  };
> >  
> > diff --git a/drivers/iio/accel/adxl313_spi.c b/drivers/iio/accel/adxl313_spi.c
> > index 9a16b40bff34..68e323e81aeb 100644
> > --- a/drivers/iio/accel/adxl313_spi.c
> > +++ b/drivers/iio/accel/adxl313_spi.c
> > @@ -24,6 +24,8 @@ static const struct regmap_config adxl31x_spi_regmap_config[] = {
> >  		.max_register	= 0x39,
> >  		/* Setting bits 7 and 6 enables multiple-byte read */
> >  		.read_flag_mask	= BIT(7) | BIT(6),
> > +		.volatile_reg	= adxl313_is_volatile_reg,
> > +		.cache_type	= REGCACHE_MAPLE,
> >  	},
> >  	[ADXL313] = {
> >  		.reg_bits	= 8,
> > @@ -33,6 +35,8 @@ static const struct regmap_config adxl31x_spi_regmap_config[] = {
> >  		.max_register	= 0x39,
> >  		/* Setting bits 7 and 6 enables multiple-byte read */
> >  		.read_flag_mask	= BIT(7) | BIT(6),
> > +		.volatile_reg	= adxl313_is_volatile_reg,
> > +		.cache_type	= REGCACHE_MAPLE,
> >  	},
> >  	[ADXL314] = {
> >  		.reg_bits	= 8,
> > @@ -42,6 +46,8 @@ static const struct regmap_config adxl31x_spi_regmap_config[] = {
> >  		.max_register	= 0x39,
> >  		/* Setting bits 7 and 6 enables multiple-byte read */
> >  		.read_flag_mask	= BIT(7) | BIT(6),
> > +		.volatile_reg	= adxl313_is_volatile_reg,
> > +		.cache_type	= REGCACHE_MAPLE,
> >  	},
> >  };
> >    
> 

Re: [PATCH v4 06/11] iio: accel: adxl313: add basic interrupt handling for FIFO watermark

[Jonathan Cameron]

On Sun,  1 Jun 2025 17:21:34 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Prepare the interrupt handler. Add register entries to evaluate the
> incoming interrupt. Add functions to clear status registers and reset the
> FIFO.
> 
> Add FIFO watermark configuration and evaluation. Let a watermark to be
> configured. Evaluate the interrupt accordingly. Read out the FIFO content
> and push the values to the IIO channel.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl313.h      |  10 ++
>  drivers/iio/accel/adxl313_core.c | 190 +++++++++++++++++++++++++++++++
>  2 files changed, 200 insertions(+)
> 
> diff --git a/drivers/iio/accel/adxl313.h b/drivers/iio/accel/adxl313.h
> index ab6b9e11fde4..4f4a9fd39f6d 100644
> --- a/drivers/iio/accel/adxl313.h
> +++ b/drivers/iio/accel/adxl313.h
> @@ -53,11 +53,19 @@
>  #define ADXL313_INT_ACTIVITY		BIT(4)
>  #define ADXL313_INT_DREADY		BIT(7)
>  
> +/* FIFO entries: how many values are stored in the FIFO */
> +#define ADXL313_REG_FIFO_STATUS_ENTRIES_MSK	GENMASK(5, 0)
> +/* FIFO samples: number of samples needed for watermark (FIFO mode) */
> +#define ADXL313_REG_FIFO_CTL_SAMPLES_MSK	GENMASK(4, 0)
>  #define ADXL313_REG_FIFO_CTL_MODE_MSK		GENMASK(7, 6)
>  
>  #define ADXL313_FIFO_BYPASS			0
>  #define ADXL313_FIFO_STREAM			2
>  
> +#define ADXL313_FIFO_SIZE			32
> +
> +#define ADXL313_NUM_AXIS			3
> +
>  extern const struct regmap_access_table adxl312_readable_regs_table;
>  extern const struct regmap_access_table adxl313_readable_regs_table;
>  extern const struct regmap_access_table adxl314_readable_regs_table;
> @@ -78,7 +86,9 @@ struct adxl313_data {
>  	struct regmap	*regmap;
>  	const struct adxl313_chip_info *chip_info;
>  	struct mutex	lock; /* lock to protect transf_buf */
> +	u8 watermark;
>  	__le16		transf_buf __aligned(IIO_DMA_MINALIGN);
> +	__le16		fifo_buf[ADXL313_NUM_AXIS * ADXL313_FIFO_SIZE + 1];
>  };
>  
>  struct adxl313_chip_info {
> diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
> index 31ce1b218488..8a0b5542fb40 100644
> --- a/drivers/iio/accel/adxl313_core.c
> +++ b/drivers/iio/accel/adxl313_core.c
> @@ -10,15 +10,21 @@
>  #include <linux/bitfield.h>
>  #include <linux/interrupt.h>
>  #include <linux/module.h>
> +#include <linux/overflow.h>
>  #include <linux/property.h>
>  #include <linux/regmap.h>
>  
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/kfifo_buf.h>
> +
>  #include "adxl313.h"
>  
>  #define ADXL313_INT_NONE			U8_MAX
>  #define ADXL313_INT1				1
>  #define ADXL313_INT2				2
>  
> +#define ADXL313_REG_XYZ_BASE			ADXL313_REG_DATA_AXIS(0)
> +
>  static const struct regmap_range adxl312_readable_reg_range[] = {
>  	regmap_reg_range(ADXL313_REG_DEVID0, ADXL313_REG_DEVID0),
>  	regmap_reg_range(ADXL313_REG_OFS_AXIS(0), ADXL313_REG_OFS_AXIS(2)),
> @@ -62,6 +68,7 @@ bool adxl313_is_volatile_reg(struct device *dev, unsigned int reg)
>  	case ADXL313_REG_DATA_AXIS(4):
>  	case ADXL313_REG_DATA_AXIS(5):
>  	case ADXL313_REG_FIFO_STATUS:
> +	case ADXL313_REG_INT_SOURCE:
It's always been volatile, whether or  not we were writing it.

Hmm. Given this I'm dropping the regcache patch as I'd missed that was a partial
list when reviewing that one.

>  		return true;
>  	default:
>  		return false;
> @@ -363,6 +370,176 @@ static int adxl313_write_raw(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +static int adxl313_set_watermark(struct iio_dev *indio_dev, unsigned int value)
> +{
> +	struct adxl313_data *data = iio_priv(indio_dev);
> +	const unsigned int fifo_mask = 0x1f, interrupt_mask = 0x02;

Why not just use defines for the fields?  The second one is particularly
confusing as that is just the mask for the watermark interrupt not of
a general 'interrupt' field as the name suggests.


> +	int ret;
> +
> +	value = min(value, ADXL313_FIFO_SIZE - 1);
> +
> +	ret = regmap_update_bits(data->regmap, ADXL313_REG_FIFO_CTL,
> +				 fifo_mask, value);
> +	if (ret)
> +		return ret;
> +
> +	data->watermark = value;
> +
> +	return regmap_update_bits(data->regmap, ADXL313_REG_INT_ENABLE,
> +				  interrupt_mask, ADXL313_INT_WATERMARK);

	return regmap_set_bits(data->regmap, ADXL313_REG_INT_ENABLED,
			       ADXL313_INT_WATERMARK);
> +}


> +
> +static int adxl313_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct adxl313_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = adxl313_set_measure_en(data, false);

I'd like a comment on why we need to disable measurements
here.  Some reference to the datasheet probably and that fifo mode
can only be changed with measurements disabled.

> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(data->regmap, ADXL313_REG_FIFO_CTL,
> +			   FIELD_PREP(ADXL313_REG_FIFO_CTL_SAMPLES_MSK,	data->watermark) |
> +			   FIELD_PREP(ADXL313_REG_FIFO_CTL_MODE_MSK, ADXL313_FIFO_STREAM));
> +	if (ret)
> +		return ret;
> +
> +	return adxl313_set_measure_en(data, true);
> +}

> +static int adxl313_push_event(struct iio_dev *indio_dev, int int_stat)

Can we avoid 'event' naming here.  Events in IIO terms would not include
watermarks used to drain a fifo.

> +{
> +	struct adxl313_data *data = iio_priv(indio_dev);
> +	int samples;
> +
> +	if (FIELD_GET(ADXL313_INT_WATERMARK, int_stat)) {
> +		samples = adxl313_get_samples(data);
> +		if (samples < 0)
> +			return samples;
> +
> +		return adxl313_fifo_push(indio_dev, samples);
> +	}
> +
> +	/* Return error if no event data was pushed to the IIO channel. */
> +	return -ENOENT;
> +}
> +
> +static irqreturn_t adxl313_irq_handler(int irq, void *p)
> +{
> +	struct iio_dev *indio_dev = p;
> +	struct adxl313_data *data = iio_priv(indio_dev);
> +	int int_stat;
> +
> +	if (regmap_read(data->regmap, ADXL313_REG_INT_SOURCE, &int_stat))
> +		return IRQ_NONE;
> +
> +	if (adxl313_push_event(indio_dev, int_stat))
> +		goto err;
> +
> +	if (FIELD_GET(ADXL313_INT_OVERRUN, int_stat))

I suspect that we can have watermark and overrun set.  Whether it is appropriate
to drain the data out and push it to userspace isn't clear to me.  Maybe
add a comment on that so we can refer to it when considering the logic.

> +		goto err;
> +
> +	return IRQ_HANDLED;
> +
> +err:
> +	adxl313_fifo_reset(data);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int adxl313_reg_access(struct iio_dev *indio_dev, unsigned int reg,
>  			      unsigned int writeval, unsigned int *readval)
>  {
> @@ -377,6 +554,7 @@ static const struct iio_info adxl313_info = {
>  	.read_raw	= adxl313_read_raw,
>  	.write_raw	= adxl313_write_raw,
>  	.read_avail	= adxl313_read_freq_avail,
> +	.hwfifo_set_watermark = adxl313_set_watermark,
>  	.debugfs_reg_access = &adxl313_reg_access,
>  };
>  
> @@ -487,6 +665,18 @@ int adxl313_core_probe(struct device *dev,
>  					 int_map_msk, int_line == ADXL313_INT2);
>  		if (ret)
>  			return ret;
> +
> +		ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
> +						  &adxl313_buffer_ops);
> +		if (ret)
> +			return ret;
> +
> +		ret = devm_request_threaded_irq(dev, irq, NULL,
> +						&adxl313_irq_handler,
> +						IRQF_SHARED | IRQF_ONESHOT,
> +						indio_dev->name, indio_dev);
> +		if (ret)
> +			return ret;
>  	} else {
>  		/*
>  		 * FIFO_BYPASSED mode

Re: [PATCH v4 07/11] iio: accel: adxl313: add activity sensing

[Jonathan Cameron]

On Sun,  1 Jun 2025 17:21:35 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add possibilities to set a threshold for activity sensing. Extend the
> interrupt handler to process activity interrupts. Provide functions to set
> the activity threshold and to enable/disable activity sensing. Further add
> a fake channel for having x, y and z axis anded on the iio channel.
> 
> This is a preparatory patch. Some of the definitions and functions are
> supposed to be extended for inactivity later on.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>

Hi Lothar,

My main question from this read through is whether we need to be quite
so careful on disabling measurement when configuring events.  It is rather
unusual if that is necessary and I'm not sure that's what the datasheet
is implying with the vague bit of advice.

>  static const unsigned long adxl313_scan_masks[] = {
> @@ -297,6 +331,68 @@ static int adxl313_read_freq_avail(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +static int adxl313_is_act_inact_en(struct adxl313_data *data,
> +				   enum adxl313_activity_type type)
> +{
> +	unsigned int axis_ctrl;
> +	unsigned int regval;
> +	int axis_en, int_en, ret;
> +
> +	ret = regmap_read(data->regmap, ADXL313_REG_ACT_INACT_CTL, &axis_ctrl);
> +	if (ret)
> +		return ret;
> +
> +	/* Check if axis for activity are enabled */
> +	if (type != ADXL313_ACTIVITY)

As below - only one value possible, so don't check it.

> +		return 0;
> +
> +	axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
> +
> +	/* The axis are enabled, now check if specific interrupt is enabled */
> +	ret = regmap_read(data->regmap, ADXL313_REG_INT_ENABLE, &regval);
> +	if (ret)
> +		return ret;
> +
> +	int_en = adxl313_act_int_reg[type] & regval;
> +
> +	return axis_en && int_en;
> +}
> +
> +static int adxl313_set_act_inact_en(struct adxl313_data *data,
> +				    enum adxl313_activity_type type,
> +				    bool cmd_en)
> +{
> +	unsigned int axis_ctrl;
> +	unsigned int threshold;
> +	int ret;
> +
> +	if (type != ADXL313_ACTIVITY)

As the enum only has one value you can drop this check.
Obviously it's dropped in next patch anyway but better to never
introduce it.

> +		return 0;
> +
> +	axis_ctrl = ADXL313_ACT_XYZ_EN;
> +
> +	ret = adxl313_set_measure_en(data, false);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_assign_bits(data->regmap, ADXL313_REG_ACT_INACT_CTL,
> +				 axis_ctrl, cmd_en);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(data->regmap, adxl313_act_thresh_reg[type], &threshold);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_assign_bits(data->regmap, ADXL313_REG_INT_ENABLE,
> +				 adxl313_act_int_reg[type],
> +				 cmd_en && threshold);
> +	if (ret)
> +		return ret;
> +
> +	return adxl313_set_measure_en(data, true);
> +}
> +
>  static int adxl313_read_raw(struct iio_dev *indio_dev,
>  			    struct iio_chan_spec const *chan,
>  			    int *val, int *val2, long mask)
> @@ -370,6 +466,113 @@ static int adxl313_write_raw(struct iio_dev *indio_dev,
>  	}
>  }

> +
> +static int adxl313_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 adxl313_data *data = iio_priv(indio_dev);
> +	unsigned int act_threshold;
> +	int ret;
> +
> +	/* Measurement stays enabled, reading from regmap cache */

If it isn't safe to read whilst measurements are in progress (as opposed
to maybe getting a small variation in timing) then this seems more
fragile than I'd like (to future code changes for example).

Might need an explicit check on it being cached regcache_reg_cached()
for example though that is very rarely used which makes me dubious
about using it here.


> +
> +	if (type != IIO_EV_TYPE_MAG)
> +		return -EINVAL;
> +
> +	if (info != IIO_EV_INFO_VALUE)
> +		return -EINVAL;
> +
> +	switch (dir) {
> +	case IIO_EV_DIR_RISING:
> +		ret = regmap_read(data->regmap,
> +				  adxl313_act_thresh_reg[ADXL313_ACTIVITY],
> +				  &act_threshold);
> +		if (ret)
> +			return ret;
> +		*val = act_threshold * 15625;
> +		*val2 = MICRO;
> +		return IIO_VAL_FRACTIONAL;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int adxl313_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 adxl313_data *data = iio_priv(indio_dev);
> +	unsigned int regval;
> +	int ret;
> +
> +	ret = adxl313_set_measure_en(data, false);
> +	if (ret)
> +		return ret;
> +
> +	if (type != IIO_EV_TYPE_MAG)
> +		return -EINVAL;
> +
> +	if (info != IIO_EV_INFO_VALUE)
> +		return -EINVAL;
> +
> +	/* Scale factor 15.625 mg/LSB */
> +	regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
> +	switch (dir) {
> +	case IIO_EV_DIR_RISING:
> +		ret = regmap_write(data->regmap,
> +				   adxl313_act_thresh_reg[ADXL313_ACTIVITY],
> +				   regval);

I'm surprised this can only be set with measurement disabled.
Maybe a spec reference.   It's common to tweak event values as events
come in and we generally don't have to stop data flow whilst we do.

There are a few specific bits where the datasheet suggests updating
them has unwanted side effects in measurement mode.  + there is a general
suggestion to do configuration before enabling measurement mode.  
I don't see anything saying it is a problem for this register.

> +		if (ret)
> +			return ret;
> +		return adxl313_set_measure_en(data, true);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  static int adxl313_set_watermark(struct iio_dev *indio_dev, unsigned int value)
>  {
>  	struct adxl313_data *data = iio_priv(indio_dev);
> @@ -502,19 +705,32 @@ static int adxl313_fifo_push(struct iio_dev *indio_dev, int samples)
>  
>  static int adxl313_push_event(struct iio_dev *indio_dev, int int_stat)

Ah. This does not also have events.  Still it's a mix, so maybe
adxl313_handle_interrupts() or something like that.

>  {
> +	s64 ts = iio_get_time_ns(indio_dev);
>  	struct adxl313_data *data = iio_priv(indio_dev);
>  	int samples;
> +	int ret = -ENOENT;
> +
> +	if (FIELD_GET(ADXL313_INT_ACTIVITY, 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_RISING),
> +				     ts);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	if (FIELD_GET(ADXL313_INT_WATERMARK, int_stat)) {
>  		samples = adxl313_get_samples(data);
>  		if (samples < 0)
>  			return samples;
>  
> -		return adxl313_fifo_push(indio_dev, samples);
> +		ret = adxl313_fifo_push(indio_dev, samples);
>  	}
>  
>  	/* Return error if no event data was pushed to the IIO channel. */
> -	return -ENOENT;
> +	return ret;
This handling works, but as Andy observed maybe the comment is now confusing
given ret is mostly not an error.  Perhaps put that where ret is declared
instead, or use a separate mask check at the start to quickly
error out if no bits that we handle are set.
>  }

Re: [PATCH v4 08/11] iio: accel: adxl313: add inactivity sensing

[Jonathan Cameron]

On Sun,  1 Jun 2025 17:21:36 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Extend the interrupt handler to process interrupts as inactivity events.
> Add functions to set threshold and period registers for inactivity. Add
> functions to enable / disable inactivity. Extend the fake iio channel to
> deal with inactivity events on x, y and z combined with AND.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>

> @@ -555,16 +621,30 @@ static int adxl313_write_event_value(struct iio_dev *indio_dev,
>  	if (type != IIO_EV_TYPE_MAG)
>  		return -EINVAL;
>  
> -	if (info != IIO_EV_INFO_VALUE)
> -		return -EINVAL;
> -
> -	/* Scale factor 15.625 mg/LSB */
> -	regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
> -	switch (dir) {
> -	case IIO_EV_DIR_RISING:
> -		ret = regmap_write(data->regmap,
> -				   adxl313_act_thresh_reg[ADXL313_ACTIVITY],
> -				   regval);
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		/* Scale factor 15.625 mg/LSB */
> +		regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
> +		switch (dir) {
> +		case IIO_EV_DIR_RISING:
> +			ret = regmap_write(data->regmap,
> +					   adxl313_act_thresh_reg[ADXL313_ACTIVITY],
> +					   regval);
> +			if (ret)
> +				return ret;
> +			return adxl313_set_measure_en(data, true);
> +		case IIO_EV_DIR_FALLING:
> +			ret = regmap_write(data->regmap,
> +					   adxl313_act_thresh_reg[ADXL313_INACTIVITY],
> +					   regval);
> +			if (ret)
> +				return ret;
> +			return adxl313_set_measure_en(data, true);
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_EV_INFO_PERIOD:
> +		ret = adxl313_set_inact_time_s(data, val);
>  		if (ret)
>  			return ret;
>  		return adxl313_set_measure_en(data, true);

Having the enable in the case statement but he disable outside is misbalanced.
Do it one way or the other (either always disable / enable outside, or both
inside each relevant case statement.   If we need to do this, I'd have
a helper function do_adxl313_write_event_value() or similar that is called
between disabling and enabling measurement mode and contains all the other
stuff in this function (or along those lines anyway).  You can chose
whether or not to reenable measurement mode depending on what is returned
by the helper function.

Re: [PATCH v4 09/11] iio: accel: adxl313: implement power-save on inactivity

[Jonathan Cameron]

On Sun,  1 Jun 2025 17:21:37 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Link activity and inactivity to indicate the internal power-saving state.
> Add auto-sleep to be linked to inactivity.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl313.h      |  3 +++
>  drivers/iio/accel/adxl313_core.c | 20 ++++++++++++++++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/drivers/iio/accel/adxl313.h b/drivers/iio/accel/adxl313.h
> index d7e8cb44855b..75ef54b60f75 100644
> --- a/drivers/iio/accel/adxl313.h
> +++ b/drivers/iio/accel/adxl313.h
> @@ -41,6 +41,9 @@
>  #define ADXL313_RATE_BASE		6
>  
>  #define ADXL313_POWER_CTL_MSK		BIT(3)
> +#define ADXL313_POWER_CTL_INACT_MSK	GENMASK(5, 4)
> +#define ADXL313_POWER_CTL_LINK		BIT(5)
> +#define ADXL313_POWER_CTL_AUTO_SLEEP	BIT(4)
>  
>  #define ADXL313_RANGE_MSK		GENMASK(1, 0)
>  #define ADXL313_RANGE_MAX		3
> diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
> index c5767d56b0cb..1598562a38e2 100644
> --- a/drivers/iio/accel/adxl313_core.c
> +++ b/drivers/iio/accel/adxl313_core.c
> @@ -396,6 +396,7 @@ static int adxl313_set_act_inact_en(struct adxl313_data *data,
>  	unsigned int axis_ctrl;
>  	unsigned int threshold;
>  	unsigned int inact_time_s;
> +	int act_en, inact_en;
>  	bool en;
>  	int ret;
>  
> @@ -431,6 +432,25 @@ static int adxl313_set_act_inact_en(struct adxl313_data *data,
>  	if (ret)
>  		return ret;
>  
> +	/*
> +	 * Set sleep and link bit only when ACT and INACT are enabled.
> +	 */

Single line comment style preferred.

> +	act_en = adxl313_is_act_inact_en(data, ADXL313_ACTIVITY);
> +	if (act_en < 0)
> +		return act_en;
> +
> +	inact_en = adxl313_is_act_inact_en(data, ADXL313_INACTIVITY);
> +	if (inact_en < 0)
> +		return inact_en;
> +
> +	en = en && act_en && inact_en;
> +
> +	ret = regmap_assign_bits(data->regmap, ADXL313_REG_POWER_CTL,
> +				 (ADXL313_POWER_CTL_AUTO_SLEEP | ADXL313_POWER_CTL_LINK),
> +				 en);
> +	if (ret)
> +		return ret;
> +
>  	return adxl313_set_measure_en(data, true);
>  }
>  

Re: [PATCH v4 10/11] iio: accel: adxl313: add AC coupled activity/inactivity events

[Jonathan Cameron]

On Sun,  1 Jun 2025 17:21:38 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add AC coupling activity and inactivity as MAG_ADAPTIVE events. This adds
> up an additional set of threshold and period handles, verifies matching
> disabling functionality and extends setting the link bit to complementary
> event configurations.
> 
> This means, e.g. either ACTIVITY or ACTIVITY_AC can be enabled. The most
> recent set will remain configured. Disabling ACTIVITY where ACTIVITY_AC was
> enabled is ignored, since it does not match (should be disabling
> ACTIVITY_AC). When INACTIVITY or INACTIVITY_AC is also enabled, the link
> bit will be set. Note, having the link bit and auto-sleep in place activity
> and inactivity indicate the power save state change and thus will only be
> triggered once a state transition occurs. Since there is a separate AC bit
> for ACTIVITY and for INACTIVITY, events can be linked independently from
> each other i.e. ACTIVITY can be linked to INACTIVITY_AC for instance.
> 
> When one of both is disabled, the link bit will be removed. Hence, the
> remaining event will not indicate a plain state change anymore, but occur
> as a periodically triggered inactivity event or for each activity event
> above the threshold.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>

Minor thought on rereading this.  If we don't have the link bit set
(and the paired event) the AC events are more accurately described as
MAG_REFERENCED as they are referenced simply to whatever acceleration
was going on when they were first enabled.   Only with the link bit
set (and the other event type enabled) are they actually adapting 
(so MAG_ADAPTIVE).


Maybe there is room to use that to ultimately control whether the
link bit is set or not (putting aside the power aspect of that).
 

Re: [PATCH v4 02/11] iio: accel: adxl313: introduce channel buffer

[Lothar Rubusch]

Hi Andy,

On Sun, Jun 1, 2025 at 9:08 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sun, Jun 1, 2025 at 8:21 PM Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >
> > Add a scan_mask and scan_index to the iio channel. The scan_index
>
> IIO
>
> > prepares the buffer usage. According to the datasheet, the ADXL313
> > uses 13 bit in full resolution. Add signedness, storage bits and
>
> bits
> ...OR...
> 13-bit wide data field
>
> > endianness.

As this is getting very annoying, I tried to set something up
involving checkpatch, codespell and ispell. But I guess w/o
grammatical checking. Just, in case, do I miss some simple tooling
here, any suggestions?

As a consequence, the alternative in the year 2025 is probably using
chatGPT for the commit messages and we probably never will have this
discussion anymore. Hum... ?

>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v4 05/11] iio: accel: adxl313: prepare interrupt handling

[Lothar Rubusch]

Hi Andy,

On Sun, Jun 1, 2025 at 9:21 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sun, Jun 1, 2025 at 8:21 PM Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >
> > Evaluate the devicetree property for an optional interrupt line, and
> > configure the interrupt mapping accordingly. When no interrupt line
> > is defined in the devicetree, keep the FIFO in bypass mode as before.
>
> ...
>
> >         struct adxl313_data *data;
> >         struct iio_dev *indio_dev;
> > -       int ret;
> > +       u8 int_line;
> > +       u8 int_map_msk;
> > +       int irq, ret;
>
> Why do you need a specific irq variable?
>
> ...
>
> > +       int_line = ADXL313_INT1;
>
> Assign this when we are sure that the INT1 is defined. Current
> approach is not robust.
>
> > +       irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT1");
> > +       if (irq < 0) {
> > +               int_line = ADXL313_INT2;
> > +               irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT2");
> > +               if (irq < 0)
> > +                       int_line = ADXL313_INT_NONE;
> > +       }
>
> So, the below code does not use the returned vIRQ, moreover, the above
> code actually does the IRQ mapping. Why do you need that if the code
> doesn't use it?
>
> > +       if (int_line != ADXL313_INT_NONE) {
>
> Why not positive conditional? But see below...
>
> > +               /* FIFO_STREAM mode */
> > +               int_map_msk = ADXL313_INT_DREADY | ADXL313_INT_ACTIVITY |
> > +                       ADXL313_INT_INACTIVITY | ADXL313_INT_WATERMARK |
> > +                       ADXL313_INT_OVERRUN;
> > +               ret = regmap_assign_bits(data->regmap, ADXL313_REG_INT_MAP,
> > +                                        int_map_msk, int_line == ADXL313_INT2);
>
> This is fragile. It heavily relies on the existence of exactly three
> IRQ variants. Instead of defining special case (NONE) simply use
> whatever is undefined as the default case
>
>   switch (IRQ type) {
>   case 'INT1':
>     ...
>     break;
>   case 'INT2':
>     ...
>     break;
>   default:
>     // FIFO bypass mode
>     ...
>     break;
>   }

The idea here is actually to conditionally try to read if optional
interrupt lines are configured in the DT. First I check if INT1 is
configured. If not, I try INT2. Else, no interrupt line was setup. The
interrupt lines just need to be configured in the mapping register.
So, there is actually nothing more to a case INT1 or case INT2.

With this explanation and from how I also interprete your and
Jonathans commit, I'll go to merge some of the patches for a next
version. I won't change to switch/case here. IMHO it is not the
approach for the above idea (might be wrong).

I appreciate your feedback and have taken note of it. Thank you.

>
> But still, the main question and confusion here is the absence of the
> users of 'irq'.
>
> > +               if (ret)
> > +                       return ret;
> > +       } else {
> > +               /*
> > +                * FIFO_BYPASSED mode
> > +                *
> > +                * When no interrupt lines are specified, the driver falls back
> > +                * to use the sensor in FIFO_BYPASS mode. This means turning off
> > +                * internal FIFO and interrupt generation (since there is no
> > +                * line specified). Unmaskable interrupts such as overrun or
> > +                * data ready won't interfere. Even that a FIFO_STREAM mode w/o
> > +                * connected interrupt line might allow for obtaining raw
> > +                * measurements, a fallback to disable interrupts when no
> > +                * interrupt lines are connected seems to be the cleaner
> > +                * solution.
> > +                */
> > +               ret = regmap_write(data->regmap, ADXL313_REG_FIFO_CTL,
> > +                                  FIELD_PREP(ADXL313_REG_FIFO_CTL_MODE_MSK,
> > +                                             ADXL313_FIFO_BYPASS));
> > +               if (ret)
> > +                       return ret;
> > +       }
> > +
> >         return devm_iio_device_register(dev, indio_dev);
> >  }
>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v4 02/11] iio: accel: adxl313: introduce channel buffer

[Andy Shevchenko]

On Wed, Jun 11, 2025 at 10:01:39AM +0200, Lothar Rubusch wrote:
> On Sun, Jun 1, 2025 at 9:08 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Sun, Jun 1, 2025 at 8:21 PM Lothar Rubusch <l.rubusch@gmail.com> wrote:
> > >
> > > Add a scan_mask and scan_index to the iio channel. The scan_index
> >
> > IIO
> >
> > > prepares the buffer usage. According to the datasheet, the ADXL313
> > > uses 13 bit in full resolution. Add signedness, storage bits and
> >
> > bits
> > ...OR...
> > 13-bit wide data field
> >
> > > endianness.
> 
> As this is getting very annoying, I tried to set something up
> involving checkpatch, codespell and ispell. But I guess w/o
> grammatical checking. Just, in case, do I miss some simple tooling
> here, any suggestions?
> 
> As a consequence, the alternative in the year 2025 is probably using
> chatGPT for the commit messages and we probably never will have this
> discussion anymore. Hum... ?

It would be nice to have tool and not waste time on the commit message
unification.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 04/11] iio: accel: adxl313: add function to enable measurement

[Lothar Rubusch]

On Sun, Jun 8, 2025 at 5:27 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sun,  1 Jun 2025 17:21:32 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Rework controlling measurement and standby of the sensor. Therefore,
> > replace writing the register directly by encapsulating this and dealing
> > with the return value in a separte function to enable and disable
> > measurement. This will help to avoid redundant code in all locations
> > where the sensor configuration needs to be adjusted, thus measurement will
> > be set to standby, in follow up patches.
> >
> > Further, reduce the control mask to only the measurement bit. The sleep bit
> > actually controls a different behavior (not just putting the sensor to
> > standby for configuration, but turning it into sleep mode) and it is not
> > used so far. In consequence, there is no need to cover sleep bit and
> > measurement with the same mask.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> This is a good bit to have as a precursor patch (as you have done)
> because it is refactoring the existing code.  It doesn't stand on it's
> own though given for now there is only one caller, so I won't pick it up
> until the patch that uses it is ready to go.
>

So, I'll leave this patch (in case I might refrase the commit message,
and I hope this is ok). I'm going to merge [v4 02/11] [v4 05/11] and
[v4 06/11] for a v5. Let me know if I got this wrong.

> Jonathan
>
> > ---
> >  drivers/iio/accel/adxl313.h      |  3 +--
> >  drivers/iio/accel/adxl313_core.c | 10 +++++++---
> >  2 files changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iio/accel/adxl313.h b/drivers/iio/accel/adxl313.h
> > index fc937bdf83b6..9bf2facdbf87 100644
> > --- a/drivers/iio/accel/adxl313.h
> > +++ b/drivers/iio/accel/adxl313.h
> > @@ -36,8 +36,7 @@
> >  #define ADXL313_RATE_MSK             GENMASK(3, 0)
> >  #define ADXL313_RATE_BASE            6
> >
> > -#define ADXL313_POWER_CTL_MSK                GENMASK(3, 2)
> > -#define ADXL313_MEASUREMENT_MODE     BIT(3)
> > +#define ADXL313_POWER_CTL_MSK                BIT(3)
> >
> >  #define ADXL313_RANGE_MSK            GENMASK(1, 0)
> >  #define ADXL313_RANGE_MAX            3
> > diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
> > index 0c893c286017..6170c9daa30f 100644
> > --- a/drivers/iio/accel/adxl313_core.c
> > +++ b/drivers/iio/accel/adxl313_core.c
> > @@ -63,6 +63,12 @@ bool adxl313_is_volatile_reg(struct device *dev, unsigned int reg)
> >  }
> >  EXPORT_SYMBOL_NS_GPL(adxl313_is_volatile_reg, "IIO_ADXL313");
> >
> > +static int adxl313_set_measure_en(struct adxl313_data *data, bool en)
> > +{
> > +     return regmap_assign_bits(data->regmap, ADXL313_REG_POWER_CTL,
> > +                               ADXL313_POWER_CTL_MSK, en);
> > +}
> > +
> >  static int adxl312_check_id(struct device *dev,
> >                           struct adxl313_data *data)
> >  {
> > @@ -410,9 +416,7 @@ static int adxl313_setup(struct device *dev, struct adxl313_data *data,
> >       }
> >
> >       /* Enables measurement mode */
> > -     return regmap_update_bits(data->regmap, ADXL313_REG_POWER_CTL,
> > -                               ADXL313_POWER_CTL_MSK,
> > -                               ADXL313_MEASUREMENT_MODE);
> > +     return adxl313_set_measure_en(data, true);
> >  }
> >
> >  /**
>

Re: [PATCH v4 03/11] iio: accel: adxl313: make use of regmap cache

[Lothar Rubusch]

On Sun, Jun 8, 2025 at 5:38 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sun, 8 Jun 2025 16:22:15 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
>
> > On Sun,  1 Jun 2025 17:21:31 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >
> > > Setup regmap cache to cache register configuration. This is a preparatory
> > > step for follow up patches. Using cached settings will help at inerrupt
> > > handling, to generate activity and inactivity events.
> >
> > The regmap cache will reduce traffic to the device for things like reading
> > back sampling frequency, so no need to justify this patch with 'future'
> > stuff.  Justify it with current.   I've applied with the description of
> > simply
> >
> > "Setup regmap cache to cache register configuration, reducing bus traffic
> > for repeated accesses to non volatile registers."
> >
> Dropped again.  The is_volatile should include all volatile registers
> not just ones we happen to be using so far.
>

I see among the patches, REG_INT_SOURCE is added later. For a v5 then
I'll prepare a patch which sets up all registers - including
REG_INT_SOURCE right away. Correct?

Then it should be added the following line:
bool adxl313_is_volatile_reg(struct device *dev, unsigned int reg)
{
    switch (reg) {
    case ADXL313_REG_DATA_AXIS(0):
    case ADXL313_REG_DATA_AXIS(1):
    case ADXL313_REG_DATA_AXIS(2):
    case ADXL313_REG_DATA_AXIS(3):
    case ADXL313_REG_DATA_AXIS(4):
    case ADXL313_REG_DATA_AXIS(5):
    case ADXL313_REG_FIFO_STATUS:
+    case ADXL313_REG_INT_SOURCE:
        return true;
    default:
        return false;
    }
}

> You added debug accesses in previous patch which will not take the volatile
> nature into account unless the register is in that switch statement.

This is not quite clear to me. What am I missing here?

When I try to find iio drivers using "debugfs" and having a
"volatile_reg" called specification (using either ranges or by a
function), I could only identify the following drivers:
./drivers/iio/accel/msa311.c
./drivers/iio/adc/ad7380.c
./drivers/iio/adc/ina2xx-adc.c
./drivers/iio/imu/bno055/bno055.c
./drivers/iio/light/gp2ap020a00f.c

I tried to find if there is a special declaration of debug registers
in the volatile_reg list, but could not find any.

Most interesting here was:
./drivers/iio/adc/ad7380.c

It seems to claim a kind of a "direct" access specifier. Should I use
similar calls to `iio_device_claim_direct()` and
`iio_device_release_direct()` here?

 999
1000 static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev,
u32 reg,
1001                                      u32 writeval, u32 *readval)
1002 {
1003         struct ad7380_state *st = iio_priv(indio_dev);
1004         int ret;
1005
1006         if (!iio_device_claim_direct(indio_dev))
1007                 return -EBUSY;
1008
1009         if (readval)
1010                 ret = regmap_read(st->regmap, reg, readval);
1011         else
1012                 ret = regmap_write(st->regmap, reg, writeval);
1013
1014         iio_device_release_direct(indio_dev);
1015
1016         return ret;
1017 }
1018

>
> Put the all in from the start.
>

I guess, in the ADXL313 I'm doing the same approach as for the
ADXL345. If it's wrong / incomplete here, it will need to be fixed in
the ADXL345 as well. Or did I understand something wrong?

> Jonathan
>
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > ---
> > >  drivers/iio/accel/adxl313.h      |  2 ++
> > >  drivers/iio/accel/adxl313_core.c | 17 +++++++++++++++++
> > >  drivers/iio/accel/adxl313_i2c.c  |  6 ++++++
> > >  drivers/iio/accel/adxl313_spi.c  |  6 ++++++
> > >  4 files changed, 31 insertions(+)
> > >
> > > diff --git a/drivers/iio/accel/adxl313.h b/drivers/iio/accel/adxl313.h
> > > index 72f624af4686..fc937bdf83b6 100644
> > > --- a/drivers/iio/accel/adxl313.h
> > > +++ b/drivers/iio/accel/adxl313.h
> > > @@ -54,6 +54,8 @@ extern const struct regmap_access_table adxl312_writable_regs_table;
> > >  extern const struct regmap_access_table adxl313_writable_regs_table;
> > >  extern const struct regmap_access_table adxl314_writable_regs_table;
> > >
> > > +bool adxl313_is_volatile_reg(struct device *dev, unsigned int reg);
> > > +
> > >  enum adxl313_device_type {
> > >     ADXL312,
> > >     ADXL313,
> > > diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
> > > index 06a771bb4726..0c893c286017 100644
> > > --- a/drivers/iio/accel/adxl313_core.c
> > > +++ b/drivers/iio/accel/adxl313_core.c
> > > @@ -46,6 +46,23 @@ const struct regmap_access_table adxl314_readable_regs_table = {
> > >  };
> > >  EXPORT_SYMBOL_NS_GPL(adxl314_readable_regs_table, IIO_ADXL313);
> > >
> > > +bool adxl313_is_volatile_reg(struct device *dev, unsigned int reg)
> > > +{
> > > +   switch (reg) {
> > > +   case ADXL313_REG_DATA_AXIS(0):
> > > +   case ADXL313_REG_DATA_AXIS(1):
> > > +   case ADXL313_REG_DATA_AXIS(2):
> > > +   case ADXL313_REG_DATA_AXIS(3):
> > > +   case ADXL313_REG_DATA_AXIS(4):
> > > +   case ADXL313_REG_DATA_AXIS(5):
> > > +   case ADXL313_REG_FIFO_STATUS:
> > > +           return true;
> > > +   default:
> > > +           return false;
> > > +   }
> > > +}
> > > +EXPORT_SYMBOL_NS_GPL(adxl313_is_volatile_reg, "IIO_ADXL313");
> > > +
> > >  static int adxl312_check_id(struct device *dev,
> > >                         struct adxl313_data *data)
> > >  {
> > > diff --git a/drivers/iio/accel/adxl313_i2c.c b/drivers/iio/accel/adxl313_i2c.c
> > > index a4cf0cf2c5aa..e8636e8ab14f 100644
> > > --- a/drivers/iio/accel/adxl313_i2c.c
> > > +++ b/drivers/iio/accel/adxl313_i2c.c
> > > @@ -21,6 +21,8 @@ static const struct regmap_config adxl31x_i2c_regmap_config[] = {
> > >             .rd_table       = &adxl312_readable_regs_table,
> > >             .wr_table       = &adxl312_writable_regs_table,
> > >             .max_register   = 0x39,
> > > +           .volatile_reg   = adxl313_is_volatile_reg,
> > > +           .cache_type     = REGCACHE_MAPLE,
> > >     },
> > >     [ADXL313] = {
> > >             .reg_bits       = 8,
> > > @@ -28,6 +30,8 @@ static const struct regmap_config adxl31x_i2c_regmap_config[] = {
> > >             .rd_table       = &adxl313_readable_regs_table,
> > >             .wr_table       = &adxl313_writable_regs_table,
> > >             .max_register   = 0x39,
> > > +           .volatile_reg   = adxl313_is_volatile_reg,
> > > +           .cache_type     = REGCACHE_MAPLE,
> > >     },
> > >     [ADXL314] = {
> > >             .reg_bits       = 8,
> > > @@ -35,6 +39,8 @@ static const struct regmap_config adxl31x_i2c_regmap_config[] = {
> > >             .rd_table       = &adxl314_readable_regs_table,
> > >             .wr_table       = &adxl314_writable_regs_table,
> > >             .max_register   = 0x39,
> > > +           .volatile_reg   = adxl313_is_volatile_reg,
> > > +           .cache_type     = REGCACHE_MAPLE,
> > >     },
> > >  };
> > >
> > > diff --git a/drivers/iio/accel/adxl313_spi.c b/drivers/iio/accel/adxl313_spi.c
> > > index 9a16b40bff34..68e323e81aeb 100644
> > > --- a/drivers/iio/accel/adxl313_spi.c
> > > +++ b/drivers/iio/accel/adxl313_spi.c
> > > @@ -24,6 +24,8 @@ static const struct regmap_config adxl31x_spi_regmap_config[] = {
> > >             .max_register   = 0x39,
> > >             /* Setting bits 7 and 6 enables multiple-byte read */
> > >             .read_flag_mask = BIT(7) | BIT(6),
> > > +           .volatile_reg   = adxl313_is_volatile_reg,
> > > +           .cache_type     = REGCACHE_MAPLE,
> > >     },
> > >     [ADXL313] = {
> > >             .reg_bits       = 8,
> > > @@ -33,6 +35,8 @@ static const struct regmap_config adxl31x_spi_regmap_config[] = {
> > >             .max_register   = 0x39,
> > >             /* Setting bits 7 and 6 enables multiple-byte read */
> > >             .read_flag_mask = BIT(7) | BIT(6),
> > > +           .volatile_reg   = adxl313_is_volatile_reg,
> > > +           .cache_type     = REGCACHE_MAPLE,
> > >     },
> > >     [ADXL314] = {
> > >             .reg_bits       = 8,
> > > @@ -42,6 +46,8 @@ static const struct regmap_config adxl31x_spi_regmap_config[] = {
> > >             .max_register   = 0x39,
> > >             /* Setting bits 7 and 6 enables multiple-byte read */
> > >             .read_flag_mask = BIT(7) | BIT(6),
> > > +           .volatile_reg   = adxl313_is_volatile_reg,
> > > +           .cache_type     = REGCACHE_MAPLE,
> > >     },
> > >  };
> > >
> >
>

Re: [PATCH v4 07/11] iio: accel: adxl313: add activity sensing

[Lothar Rubusch]

Hi Andy,

On Sun, Jun 1, 2025 at 9:38 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sun, Jun 1, 2025 at 8:22 PM Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >
> > Add possibilities to set a threshold for activity sensing. Extend the
> > interrupt handler to process activity interrupts. Provide functions to set
> > the activity threshold and to enable/disable activity sensing. Further add
> > a fake channel for having x, y and z axis anded on the iio channel.
>
> IIO
>
> And what does the 'anded' mean?
>
> > This is a preparatory patch. Some of the definitions and functions are
> > supposed to be extended for inactivity later on.
>
> ...
>
> > +static int adxl313_is_act_inact_en(struct adxl313_data *data,
> > +                                  enum adxl313_activity_type type)
> > +{
> > +       unsigned int axis_ctrl;
> > +       unsigned int regval;
> > +       int axis_en, int_en, ret;
> > +
> > +       ret = regmap_read(data->regmap, ADXL313_REG_ACT_INACT_CTL, &axis_ctrl);
> > +       if (ret)
> > +               return ret;
> > +
> > +       /* Check if axis for activity are enabled */
> > +       if (type != ADXL313_ACTIVITY)
> > +               return 0;
> > +
> > +       axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
>
> If it's false, it will be false anyway. No need to defer the check:
>
>   if (!axis_en)
>     return false;
>
> > +       /* The axis are enabled, now check if specific interrupt is enabled */
> > +       ret = regmap_read(data->regmap, ADXL313_REG_INT_ENABLE, &regval);
> > +       if (ret)
> > +               return ret;
> > +
> > +       int_en = adxl313_act_int_reg[type] & regval;
> > +
> > +       return axis_en && int_en;
>
>   return ... & regval;
>
> > +}
>
> I have already commented on this a couple of times.
>
> ...
>
> > +       /* Scale factor 15.625 mg/LSB */
> > +       regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
>
> I would rather do
>
> val * MICRO + val2
>
> which is read more naturally (we will easily get that the expression
> uses MICRO scale).
>
> ...
>
> > +       int ret = -ENOENT;
> > +
> > +       if (FIELD_GET(ADXL313_INT_ACTIVITY, 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_RISING),
> > +                                    ts);
> > +               if (ret)
> > +                       return ret;
> > +       }
> >
> >         if (FIELD_GET(ADXL313_INT_WATERMARK, int_stat)) {
> >                 samples = adxl313_get_samples(data);
> >                 if (samples < 0)
> >                         return samples;
> >
> > -               return adxl313_fifo_push(indio_dev, samples);
> > +               ret = adxl313_fifo_push(indio_dev, samples);
>
> This is not needed...
>

IMHO this will be needed, or shall be needed in the follow up context.

The [going to be renamed] function push_events() shall evaluate the
interrupt status register for the events the driver can handle and
also eventually drain the FIFO in case of watermark. It shall
distinguish between failure, events / drain the FIFO which can be
handled, and events which cannot be handled so far. It's not a if /
else, there can be some event, and some fifo data. Therefore I'd like
not a simple return here, but init a ret var.

I interpreted your reviews, to change the particular implementation as
if there was just activity. Then in a follow up patch, rewrite it
again, now to distinguish just bewteen just activity and inactivity
e.g. by if/else. Eventually rewrite it by a third approach to
distinghish activity, inactivity, AC-coupled activity and AC-coupled
inactivity, might be now switch/case. Eventually you might complain
that my patches contain way too much modification of every line in
every patch.

I'd rather like to start right away with the final structure with just
the first element - e.g. "activity" - leads to results like the above.
Less churn among patches, but having just one element looks like
having taken an over-complicated approach.

Perhaps it's my patch split? Unsure, I tried to note in the commit message:
> This is a preparatory patch. Some of the definitions and functions are
> supposed to be extended for inactivity later on.
Perhaps it needs more feedback here?

Another example is seting up the read/write_event_config() or
read/write_event_value() functions. I mean, eventually this will
become a switch/case implementation. Of course with just one element
switch/case seems to be obvious overkill. Going by your advice, I
changed it to if(!..) return, it's definitely cleaner. Definitely in
the follow up patches this will be rewritten, though.

Please, let me know what is the best approach or what I can improve to
avoid such "ping pong patching" as you name it?

Might be that you're right here in this particular case, but then it
would be better to discuss the final structure, isn't it?


> >         }
> >
> >         /* Return error if no event data was pushed to the IIO channel. */
> > -       return -ENOENT;
> > +       return ret;
>
> ...and this looks wrong.

Well, as I said. Each separate if-condition (not just if-else), could
be ok or not. If ok, the function still shall continue, might be at
the end, also a watermark flag is in the status reg and the FIFO needs
to be drained. It also might be, that some event comes which the
driver does still not handle, but not necessarily an error
(missconfiguration). So, draining the FIFO helps in most cases to
bring a derailed sensor back on track. If not doing so, it silmply
stops working, you would need to turn off and on again, or even power
cycle the setup.

Probably you have a better idea here, but pls have a look into the
final setup. I really appreciate your feedbacks. I understand this is
a rather problematic part of the code. To me it makes sense like this,
but I'd highly appreciate your advice.

>
> Before the case was clear, if we have no respective bit set in the
> int_stat, we return ENOENT. Now it depends on the other bit. If this
> is correct behaviour, it needs a comment.
>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v4 03/11] iio: accel: adxl313: make use of regmap cache

[Jonathan Cameron]

On Wed, 11 Jun 2025 15:48:25 +0200
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> On Sun, Jun 8, 2025 at 5:38 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Sun, 8 Jun 2025 16:22:15 +0100
> > Jonathan Cameron <jic23@kernel.org> wrote:
> >  
> > > On Sun,  1 Jun 2025 17:21:31 +0000
> > > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> > >  
> > > > Setup regmap cache to cache register configuration. This is a preparatory
> > > > step for follow up patches. Using cached settings will help at inerrupt
> > > > handling, to generate activity and inactivity events.  
> > >
> > > The regmap cache will reduce traffic to the device for things like reading
> > > back sampling frequency, so no need to justify this patch with 'future'
> > > stuff.  Justify it with current.   I've applied with the description of
> > > simply
> > >
> > > "Setup regmap cache to cache register configuration, reducing bus traffic
> > > for repeated accesses to non volatile registers."
> > >  
> > Dropped again.  The is_volatile should include all volatile registers
> > not just ones we happen to be using so far.
> >  
> 
> I see among the patches, REG_INT_SOURCE is added later. For a v5 then
> I'll prepare a patch which sets up all registers - including
> REG_INT_SOURCE right away. Correct?
> 
Yes + any others that we aren't using at all yet.

> Then it should be added the following line:
> bool adxl313_is_volatile_reg(struct device *dev, unsigned int reg)
> {
>     switch (reg) {
>     case ADXL313_REG_DATA_AXIS(0):
>     case ADXL313_REG_DATA_AXIS(1):
>     case ADXL313_REG_DATA_AXIS(2):
>     case ADXL313_REG_DATA_AXIS(3):
>     case ADXL313_REG_DATA_AXIS(4):
>     case ADXL313_REG_DATA_AXIS(5):
>     case ADXL313_REG_FIFO_STATUS:
> +    case ADXL313_REG_INT_SOURCE:
>         return true;
>     default:
>         return false;
>     }
> }
> 
> > You added debug accesses in previous patch which will not take the volatile
> > nature into account unless the register is in that switch statement.  
> 
> This is not quite clear to me. What am I missing here?
> 
> When I try to find iio drivers using "debugfs" and having a
> "volatile_reg" called specification (using either ranges or by a
> function), I could only identify the following drivers:
> ./drivers/iio/accel/msa311.c
> ./drivers/iio/adc/ad7380.c
> ./drivers/iio/adc/ina2xx-adc.c
> ./drivers/iio/imu/bno055/bno055.c
> ./drivers/iio/light/gp2ap020a00f.c

It only matters if regcache is involved.  If you don't mark
all the registers volatile + provide debugfs access to them
then only the first read will reach the device.  The result
of that will be stored in cache and served up for future
use of the debug interface (rather than the updated value).

> 
> I tried to find if there is a special declaration of debug registers
> in the volatile_reg list, but could not find any.
> 
> Most interesting here was:
> ./drivers/iio/adc/ad7380.c
> 
> It seems to claim a kind of a "direct" access specifier. Should I use
> similar calls to `iio_device_claim_direct()` and
> `iio_device_release_direct()` here?

Generally we only do that if simply accessing the register is enough
to break comms if done incorrectly. That's normally only on devices
where a mode switch is involved where a device transitions from
register access mode to streaming mode and we don't want a simple
debug read to flip it back again (as that would be a major state
change and rather defeat the point of debug access).

Note sure if that's true for that particular part or not though
(I didn't look).

> 
>  999
> 1000 static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev,
> u32 reg,
> 1001                                      u32 writeval, u32 *readval)
> 1002 {
> 1003         struct ad7380_state *st = iio_priv(indio_dev);
> 1004         int ret;
> 1005
> 1006         if (!iio_device_claim_direct(indio_dev))
> 1007                 return -EBUSY;
> 1008
> 1009         if (readval)
> 1010                 ret = regmap_read(st->regmap, reg, readval);
> 1011         else
> 1012                 ret = regmap_write(st->regmap, reg, writeval);
> 1013
> 1014         iio_device_release_direct(indio_dev);
> 1015
> 1016         return ret;
> 1017 }
> 1018
> 
> >
> > Put the all in from the start.
> >  
> 
> I guess, in the ADXL313 I'm doing the same approach as for the
> ADXL345. If it's wrong / incomplete here, it will need to be fixed in
> the ADXL345 as well. Or did I understand something wrong?

No there is generally no need to prevent debug access just because
buffered mode is in use.  It is possible for someone foolishly
misusing the write to break things of course, but if we remove
the foot gun then the debug interfaces aren't useful in what is
normally our most high performance mode and one we may well be in
when we want to poke the state.

I'm not personally a big fan of any debug interfaces at all in
production drivers, but we've had them a long time so that ship
sailed.

Jonathan

Re: [PATCH v4 07/11] iio: accel: adxl313: add activity sensing

[Andy Shevchenko]

On Wed, Jun 11, 2025 at 04:49:34PM +0200, Lothar Rubusch wrote:
> On Sun, Jun 1, 2025 at 9:38 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Sun, Jun 1, 2025 at 8:22 PM Lothar Rubusch <l.rubusch@gmail.com> wrote:

...

> > > +       int ret = -ENOENT;
> > > +
> > > +       if (FIELD_GET(ADXL313_INT_ACTIVITY, 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_RISING),
> > > +                                    ts);
> > > +               if (ret)
> > > +                       return ret;
> > > +       }
> > >
> > >         if (FIELD_GET(ADXL313_INT_WATERMARK, int_stat)) {
> > >                 samples = adxl313_get_samples(data);
> > >                 if (samples < 0)
> > >                         return samples;
> > >
> > > -               return adxl313_fifo_push(indio_dev, samples);
> > > +               ret = adxl313_fifo_push(indio_dev, samples);
> >
> > This is not needed...
> 
> IMHO this will be needed, or shall be needed in the follow up context.

Right, but wouldn't be better to update at the same time when this new context
appears?

> The [going to be renamed] function push_events() shall evaluate the
> interrupt status register for the events the driver can handle and
> also eventually drain the FIFO in case of watermark. It shall
> distinguish between failure, events / drain the FIFO which can be
> handled, and events which cannot be handled so far. It's not a if /
> else, there can be some event, and some fifo data. Therefore I'd like
> not a simple return here, but init a ret var.
> 
> I interpreted your reviews, to change the particular implementation as
> if there was just activity. Then in a follow up patch, rewrite it
> again, now to distinguish just bewteen just activity and inactivity
> e.g. by if/else. Eventually rewrite it by a third approach to
> distinghish activity, inactivity, AC-coupled activity and AC-coupled
> inactivity, might be now switch/case. Eventually you might complain
> that my patches contain way too much modification of every line in
> every patch.
> 
> I'd rather like to start right away with the final structure with just
> the first element - e.g. "activity" - leads to results like the above.
> Less churn among patches, but having just one element looks like
> having taken an over-complicated approach.
> 
> Perhaps it's my patch split? Unsure, I tried to note in the commit message:
> > This is a preparatory patch. Some of the definitions and functions are
> > supposed to be extended for inactivity later on.
> Perhaps it needs more feedback here?
> 
> Another example is seting up the read/write_event_config() or
> read/write_event_value() functions. I mean, eventually this will
> become a switch/case implementation. Of course with just one element
> switch/case seems to be obvious overkill. Going by your advice, I
> changed it to if(!..) return, it's definitely cleaner. Definitely in
> the follow up patches this will be rewritten, though.
> 
> Please, let me know what is the best approach or what I can improve to
> avoid such "ping pong patching" as you name it?
> 
> Might be that you're right here in this particular case, but then it
> would be better to discuss the final structure, isn't it?

Basically I use the following rule of thumb: I made an approach and look at
the each patch separately and at the series as a whole (with the end result).
If it's too much of rewriting (yes, I admit, that in some cases it's
unavoidable to have some changes as we do feature-by-feature incremental
changes), I try to rethink. Repeat, until the result looks good enough.
I.o.w. you, as the author of this code, can propose something better based on
your knowledge of the HW and vision of what you want at the end.

Yeah, this might require time and a few attempts which one can argue would be
waste of time. But at least this is my personal experience and flow with my
own patches.

> > >         }
> > >
> > >         /* Return error if no event data was pushed to the IIO channel. */
> > > -       return -ENOENT;
> > > +       return ret;
> >
> > ...and this looks wrong.
> 
> Well, as I said. Each separate if-condition (not just if-else), could
> be ok or not. If ok, the function still shall continue, might be at
> the end, also a watermark flag is in the status reg and the FIFO needs
> to be drained. It also might be, that some event comes which the
> driver does still not handle, but not necessarily an error
> (missconfiguration). So, draining the FIFO helps in most cases to
> bring a derailed sensor back on track. If not doing so, it silmply
> stops working, you would need to turn off and on again, or even power
> cycle the setup.
> 
> Probably you have a better idea here, but pls have a look into the
> final setup. I really appreciate your feedbacks. I understand this is
> a rather problematic part of the code. To me it makes sense like this,
> but I'd highly appreciate your advice.

I will do my best. Thanks for your patience!

> > Before the case was clear, if we have no respective bit set in the
> > int_stat, we return ENOENT. Now it depends on the other bit. If this
> > is correct behaviour, it needs a comment.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 04/11] iio: accel: adxl313: add function to enable measurement

[Jonathan Cameron]

On Wed, 11 Jun 2025 10:55:26 +0200
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> On Sun, Jun 8, 2025 at 5:27 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Sun,  1 Jun 2025 17:21:32 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >  
> > > Rework controlling measurement and standby of the sensor. Therefore,
> > > replace writing the register directly by encapsulating this and dealing
> > > with the return value in a separte function to enable and disable
> > > measurement. This will help to avoid redundant code in all locations
> > > where the sensor configuration needs to be adjusted, thus measurement will
> > > be set to standby, in follow up patches.
> > >
> > > Further, reduce the control mask to only the measurement bit. The sleep bit
> > > actually controls a different behavior (not just putting the sensor to
> > > standby for configuration, but turning it into sleep mode) and it is not
> > > used so far. In consequence, there is no need to cover sleep bit and
> > > measurement with the same mask.
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>  
> > This is a good bit to have as a precursor patch (as you have done)
> > because it is refactoring the existing code.  It doesn't stand on it's
> > own though given for now there is only one caller, so I won't pick it up
> > until the patch that uses it is ready to go.
> >  
> 
> So, I'll leave this patch (in case I might refrase the commit message,
> and I hope this is ok). I'm going to merge [v4 02/11] [v4 05/11] and
> [v4 06/11] for a v5. Let me know if I got this wrong.
> 
Sounds good to me.

J

Re: [PATCH v4 07/11] iio: accel: adxl313: add activity sensing

[Lothar Rubusch]

On Sun, Jun 8, 2025 at 6:08 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sun,  1 Jun 2025 17:21:35 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Add possibilities to set a threshold for activity sensing. Extend the
> > interrupt handler to process activity interrupts. Provide functions to set
> > the activity threshold and to enable/disable activity sensing. Further add
> > a fake channel for having x, y and z axis anded on the iio channel.
> >
> > This is a preparatory patch. Some of the definitions and functions are
> > supposed to be extended for inactivity later on.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
>
> Hi Lothar,
>
> My main question from this read through is whether we need to be quite
> so careful on disabling measurement when configuring events.  It is rather
> unusual if that is necessary and I'm not sure that's what the datasheet
> is implying with the vague bit of advice.
>
> >  static const unsigned long adxl313_scan_masks[] = {
> > @@ -297,6 +331,68 @@ static int adxl313_read_freq_avail(struct iio_dev *indio_dev,
> >       }
> >  }
> >
> > +static int adxl313_is_act_inact_en(struct adxl313_data *data,
> > +                                enum adxl313_activity_type type)
> > +{
> > +     unsigned int axis_ctrl;
> > +     unsigned int regval;
> > +     int axis_en, int_en, ret;
> > +
> > +     ret = regmap_read(data->regmap, ADXL313_REG_ACT_INACT_CTL, &axis_ctrl);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Check if axis for activity are enabled */
> > +     if (type != ADXL313_ACTIVITY)
>
> As below - only one value possible, so don't check it.
>
> > +             return 0;
> > +
> > +     axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
> > +
> > +     /* The axis are enabled, now check if specific interrupt is enabled */
> > +     ret = regmap_read(data->regmap, ADXL313_REG_INT_ENABLE, &regval);
> > +     if (ret)
> > +             return ret;
> > +
> > +     int_en = adxl313_act_int_reg[type] & regval;
> > +
> > +     return axis_en && int_en;
> > +}
> > +
> > +static int adxl313_set_act_inact_en(struct adxl313_data *data,
> > +                                 enum adxl313_activity_type type,
> > +                                 bool cmd_en)
> > +{
> > +     unsigned int axis_ctrl;
> > +     unsigned int threshold;
> > +     int ret;
> > +
> > +     if (type != ADXL313_ACTIVITY)
>
> As the enum only has one value you can drop this check.
> Obviously it's dropped in next patch anyway but better to never
> introduce it.
>
> > +             return 0;
> > +
> > +     axis_ctrl = ADXL313_ACT_XYZ_EN;
> > +
> > +     ret = adxl313_set_measure_en(data, false);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = regmap_assign_bits(data->regmap, ADXL313_REG_ACT_INACT_CTL,
> > +                              axis_ctrl, cmd_en);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = regmap_read(data->regmap, adxl313_act_thresh_reg[type], &threshold);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = regmap_assign_bits(data->regmap, ADXL313_REG_INT_ENABLE,
> > +                              adxl313_act_int_reg[type],
> > +                              cmd_en && threshold);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return adxl313_set_measure_en(data, true);
> > +}
> > +
> >  static int adxl313_read_raw(struct iio_dev *indio_dev,
> >                           struct iio_chan_spec const *chan,
> >                           int *val, int *val2, long mask)
> > @@ -370,6 +466,113 @@ static int adxl313_write_raw(struct iio_dev *indio_dev,
> >       }
> >  }
>
> > +
> > +static int adxl313_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 adxl313_data *data = iio_priv(indio_dev);
> > +     unsigned int act_threshold;
> > +     int ret;
> > +
> > +     /* Measurement stays enabled, reading from regmap cache */
>
> If it isn't safe to read whilst measurements are in progress (as opposed
> to maybe getting a small variation in timing) then this seems more
> fragile than I'd like (to future code changes for example).
>
> Might need an explicit check on it being cached regcache_reg_cached()
> for example though that is very rarely used which makes me dubious
> about using it here.
>
>
> > +
> > +     if (type != IIO_EV_TYPE_MAG)
> > +             return -EINVAL;
> > +
> > +     if (info != IIO_EV_INFO_VALUE)
> > +             return -EINVAL;
> > +
> > +     switch (dir) {
> > +     case IIO_EV_DIR_RISING:
> > +             ret = regmap_read(data->regmap,
> > +                               adxl313_act_thresh_reg[ADXL313_ACTIVITY],
> > +                               &act_threshold);
> > +             if (ret)
> > +                     return ret;
> > +             *val = act_threshold * 15625;
> > +             *val2 = MICRO;
> > +             return IIO_VAL_FRACTIONAL;
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +}
> > +
> > +static int adxl313_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 adxl313_data *data = iio_priv(indio_dev);
> > +     unsigned int regval;
> > +     int ret;
> > +
> > +     ret = adxl313_set_measure_en(data, false);
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (type != IIO_EV_TYPE_MAG)
> > +             return -EINVAL;
> > +
> > +     if (info != IIO_EV_INFO_VALUE)
> > +             return -EINVAL;
> > +
> > +     /* Scale factor 15.625 mg/LSB */
> > +     regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
> > +     switch (dir) {
> > +     case IIO_EV_DIR_RISING:
> > +             ret = regmap_write(data->regmap,
> > +                                adxl313_act_thresh_reg[ADXL313_ACTIVITY],
> > +                                regval);
>
> I'm surprised this can only be set with measurement disabled.
> Maybe a spec reference.   It's common to tweak event values as events
> come in and we generally don't have to stop data flow whilst we do.
>
> There are a few specific bits where the datasheet suggests updating
> them has unwanted side effects in measurement mode.  + there is a general
> suggestion to do configuration before enabling measurement mode.
> I don't see anything saying it is a problem for this register.
>

AFAIK there is no issue, nor a big side effect. Changing config
registers might lead to initially wrong measurements. Just the first
measurements might be wrong. I guess this could be a problem if the
sensor had more features and, say, any kind of threshold for some
event then triggered an event wrongly. In case of the ADXL313, there
should not be such a risk. It's then rather about initial wrong
measurements. (Exception: changing the FIFO modes, where turning to
standby is explicitely recommended).

Unfortunately, I could not recall the exact page in the datasheet, but
it matched pretty much my observation also with the ADXL345. So, I'll
give it a try and remove turning off measurement for the
write_event_config()


> > +             if (ret)
> > +                     return ret;
> > +             return adxl313_set_measure_en(data, true);
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +}
> > +
> >  static int adxl313_set_watermark(struct iio_dev *indio_dev, unsigned int value)
> >  {
> >       struct adxl313_data *data = iio_priv(indio_dev);
> > @@ -502,19 +705,32 @@ static int adxl313_fifo_push(struct iio_dev *indio_dev, int samples)
> >
> >  static int adxl313_push_event(struct iio_dev *indio_dev, int int_stat)
>
> Ah. This does not also have events.  Still it's a mix, so maybe
> adxl313_handle_interrupts() or something like that.

I also could break it up into:
- handle interrupt source register events
- drain fifo watermark samples
?

> >  {
> > +     s64 ts = iio_get_time_ns(indio_dev);
> >       struct adxl313_data *data = iio_priv(indio_dev);
> >       int samples;
> > +     int ret = -ENOENT;
> > +
> > +     if (FIELD_GET(ADXL313_INT_ACTIVITY, 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_RISING),
> > +                                  ts);
> > +             if (ret)
> > +                     return ret;
> > +     }
> >
> >       if (FIELD_GET(ADXL313_INT_WATERMARK, int_stat)) {
> >               samples = adxl313_get_samples(data);
> >               if (samples < 0)
> >                       return samples;
> >
> > -             return adxl313_fifo_push(indio_dev, samples);
> > +             ret = adxl313_fifo_push(indio_dev, samples);
> >       }
> >
> >       /* Return error if no event data was pushed to the IIO channel. */
> > -     return -ENOENT;
> > +     return ret;
> This handling works, but as Andy observed maybe the comment is now confusing
> given ret is mostly not an error.  Perhaps put that where ret is declared
> instead, or use a separate mask check at the start to quickly
> error out if no bits that we handle are set.
> >  }

Yes. Andy also pointed out here. I already developed a feeling for
"something's smelly" with this code, but cannot really think of a
better approach. Actually it works, and for me it is somehow logical.
Probably there are better ways to solve this situation in a cleaner
way?

Re: [PATCH v4 07/11] iio: accel: adxl313: add activity sensing

[Jonathan Cameron]

On Wed, 11 Jun 2025 16:49:34 +0200
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Hi Andy,
> 
> On Sun, Jun 1, 2025 at 9:38 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Sun, Jun 1, 2025 at 8:22 PM Lothar Rubusch <l.rubusch@gmail.com> wrote:  
> > >
> > > Add possibilities to set a threshold for activity sensing. Extend the
> > > interrupt handler to process activity interrupts. Provide functions to set
> > > the activity threshold and to enable/disable activity sensing. Further add
> > > a fake channel for having x, y and z axis anded on the iio channel.  
> >
> > IIO
> >
> > And what does the 'anded' mean?
> >  
> > > This is a preparatory patch. Some of the definitions and functions are
> > > supposed to be extended for inactivity later on.  
> >
> > ...
> >  
> > > +static int adxl313_is_act_inact_en(struct adxl313_data *data,
> > > +                                  enum adxl313_activity_type type)
> > > +{
> > > +       unsigned int axis_ctrl;
> > > +       unsigned int regval;
> > > +       int axis_en, int_en, ret;
> > > +
> > > +       ret = regmap_read(data->regmap, ADXL313_REG_ACT_INACT_CTL, &axis_ctrl);
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       /* Check if axis for activity are enabled */
> > > +       if (type != ADXL313_ACTIVITY)
> > > +               return 0;
> > > +
> > > +       axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);  
> >
> > If it's false, it will be false anyway. No need to defer the check:
> >
> >   if (!axis_en)
> >     return false;
> >  
> > > +       /* The axis are enabled, now check if specific interrupt is enabled */
> > > +       ret = regmap_read(data->regmap, ADXL313_REG_INT_ENABLE, &regval);
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       int_en = adxl313_act_int_reg[type] & regval;
> > > +
> > > +       return axis_en && int_en;  
> >
> >   return ... & regval;
> >  
> > > +}  
> >
> > I have already commented on this a couple of times.
> >
> > ...
> >  
> > > +       /* Scale factor 15.625 mg/LSB */
> > > +       regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);  
> >
> > I would rather do
> >
> > val * MICRO + val2
> >
> > which is read more naturally (we will easily get that the expression
> > uses MICRO scale).
> >
> > ...
> >  
> > > +       int ret = -ENOENT;
> > > +
> > > +       if (FIELD_GET(ADXL313_INT_ACTIVITY, 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_RISING),
> > > +                                    ts);
> > > +               if (ret)
> > > +                       return ret;
> > > +       }
> > >
> > >         if (FIELD_GET(ADXL313_INT_WATERMARK, int_stat)) {
> > >                 samples = adxl313_get_samples(data);
> > >                 if (samples < 0)
> > >                         return samples;
> > >
> > > -               return adxl313_fifo_push(indio_dev, samples);
> > > +               ret = adxl313_fifo_push(indio_dev, samples);  
> >
> > This is not needed...
> >  
> 
> IMHO this will be needed, or shall be needed in the follow up context.
> 
> The [going to be renamed] function push_events() shall evaluate the
> interrupt status register for the events the driver can handle and
> also eventually drain the FIFO in case of watermark. It shall
> distinguish between failure, events / drain the FIFO which can be
> handled, and events which cannot be handled so far. It's not a if /
> else, there can be some event, and some fifo data. Therefore I'd like
> not a simple return here, but init a ret var.
> 
> I interpreted your reviews, to change the particular implementation as
> if there was just activity. Then in a follow up patch, rewrite it
> again, now to distinguish just bewteen just activity and inactivity
> e.g. by if/else. Eventually rewrite it by a third approach to
> distinghish activity, inactivity, AC-coupled activity and AC-coupled
> inactivity, might be now switch/case. Eventually you might complain
> that my patches contain way too much modification of every line in
> every patch.
> 
> I'd rather like to start right away with the final structure with just
> the first element - e.g. "activity" - leads to results like the above.
> Less churn among patches, but having just one element looks like
> having taken an over-complicated approach.

I'd do the from the first but with the comment up with where ret is
declared.  

> 
> Perhaps it's my patch split? Unsure, I tried to note in the commit message:
> > This is a preparatory patch. Some of the definitions and functions are
> > supposed to be extended for inactivity later on.  
> Perhaps it needs more feedback here?
> 
> Another example is seting up the read/write_event_config() or
> read/write_event_value() functions. I mean, eventually this will
> become a switch/case implementation. Of course with just one element
> switch/case seems to be obvious overkill. Going by your advice, I
> changed it to if(!..) return, it's definitely cleaner. Definitely in
> the follow up patches this will be rewritten, though.
Don't do that. Just use the switch from the start.

Sometimes we will give review feedback that doesn't take the whole
series into account (because it takes much longer to review a full series
then reread the feedback to spot anything that turned out to be due
to a later change)  In those cases it is fine to just reply to the
comment with - "The switch gathers additional elements in patches X,Y,Z
and so is introduced in this first patch to reduce churn.

> 
> Please, let me know what is the best approach or what I can improve to
> avoid such "ping pong patching" as you name it?
> 
> Might be that you're right here in this particular case, but then it
> would be better to discuss the final structure, isn't it?
> 
> 
> > >         }
> > >
> > >         /* Return error if no event data was pushed to the IIO channel. */
> > > -       return -ENOENT;
> > > +       return ret;  
> >
> > ...and this looks wrong.  
> 
> Well, as I said. Each separate if-condition (not just if-else), could
> be ok or not. If ok, the function still shall continue, might be at
> the end, also a watermark flag is in the status reg and the FIFO needs
> to be drained. It also might be, that some event comes which the
> driver does still not handle, but not necessarily an error
> (missconfiguration). So, draining the FIFO helps in most cases to
> bring a derailed sensor back on track. If not doing so, it silmply
> stops working, you would need to turn off and on again, or even power
> cycle the setup.
> 
> Probably you have a better idea here, but pls have a look into the
> final setup. I really appreciate your feedbacks. I understand this is
> a rather problematic part of the code. To me it makes sense like this,
> but I'd highly appreciate your advice.

The code is correct I think as I said in my review, but it is a little
unusual.  One option is sanity check and return early if none of the
events we support is set.  That removes this path from consideration.

> 
> >
> > Before the case was clear, if we have no respective bit set in the
> > int_stat, we return ENOENT. Now it depends on the other bit. If this
> > is correct behaviour, it needs a comment.
> >
> > --
> > With Best Regards,
> > Andy Shevchenko  

Re: [PATCH v4 07/11] iio: accel: adxl313: add activity sensing

[Andy Shevchenko]

On Wed, Jun 11, 2025 at 04:15:04PM +0100, Jonathan Cameron wrote:
> On Wed, 11 Jun 2025 16:49:34 +0200
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
> > On Sun, Jun 1, 2025 at 9:38 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Sun, Jun 1, 2025 at 8:22 PM Lothar Rubusch <l.rubusch@gmail.com> wrote:  

...

> > > > -               return adxl313_fifo_push(indio_dev, samples);
> > > > +               ret = adxl313_fifo_push(indio_dev, samples);  
> > >
> > > This is not needed...
> > >  
> > 
> > IMHO this will be needed, or shall be needed in the follow up context.
> > 
> > The [going to be renamed] function push_events() shall evaluate the
> > interrupt status register for the events the driver can handle and
> > also eventually drain the FIFO in case of watermark. It shall
> > distinguish between failure, events / drain the FIFO which can be
> > handled, and events which cannot be handled so far. It's not a if /
> > else, there can be some event, and some fifo data. Therefore I'd like
> > not a simple return here, but init a ret var.
> > 
> > I interpreted your reviews, to change the particular implementation as
> > if there was just activity. Then in a follow up patch, rewrite it
> > again, now to distinguish just bewteen just activity and inactivity
> > e.g. by if/else. Eventually rewrite it by a third approach to
> > distinghish activity, inactivity, AC-coupled activity and AC-coupled
> > inactivity, might be now switch/case. Eventually you might complain
> > that my patches contain way too much modification of every line in
> > every patch.
> > 
> > I'd rather like to start right away with the final structure with just
> > the first element - e.g. "activity" - leads to results like the above.
> > Less churn among patches, but having just one element looks like
> > having taken an over-complicated approach.
> 
> I'd do the from the first but with the comment up with where ret is
> declared.  
> 
> > Perhaps it's my patch split? Unsure, I tried to note in the commit message:
> > > This is a preparatory patch. Some of the definitions and functions are
> > > supposed to be extended for inactivity later on.  
> > Perhaps it needs more feedback here?
> > 
> > Another example is seting up the read/write_event_config() or
> > read/write_event_value() functions. I mean, eventually this will
> > become a switch/case implementation. Of course with just one element
> > switch/case seems to be obvious overkill. Going by your advice, I
> > changed it to if(!..) return, it's definitely cleaner. Definitely in
> > the follow up patches this will be rewritten, though.
> Don't do that. Just use the switch from the start.

But at the same time if switch becomes nested and 2+ levels, it's better
to split the inner parts to the helpr functions or so. Doing a switch
with 2+ levels looks ugly independently on the approach taken.

> Sometimes we will give review feedback that doesn't take the whole
> series into account (because it takes much longer to review a full series
> then reread the feedback to spot anything that turned out to be due
> to a later change)  In those cases it is fine to just reply to the
> comment with - "The switch gathers additional elements in patches X,Y,Z
> and so is introduced in this first patch to reduce churn.

Indeed.

> > Please, let me know what is the best approach or what I can improve to
> > avoid such "ping pong patching" as you name it?
> > 
> > Might be that you're right here in this particular case, but then it
> > would be better to discuss the final structure, isn't it?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 08/11] iio: accel: adxl313: add inactivity sensing

[Lothar Rubusch]

On Sun, Jun 1, 2025 at 9:46 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sun, Jun 1, 2025 at 8:22 PM Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >
> > Extend the interrupt handler to process interrupts as inactivity events.
> > Add functions to set threshold and period registers for inactivity. Add
> > functions to enable / disable inactivity. Extend the fake iio channel to
>
> IIO
>
> > deal with inactivity events on x, y and z combined with AND.
>
> ...
>
> > +static int adxl313_set_inact_time_s(struct adxl313_data *data,
> > +                                   unsigned int val_s)
> > +{
> > +       unsigned int max_boundary = 255;
>
> This is unclear how it's defined. What is the limit behind? Size of a
> bit field? Decimal value from the datasheet?
>
> The forms of (BIT(8) - 1) or GENMASK(7, 0) may be better depending on
> the answers to the above questions.
>
> > +       unsigned int val = min(val_s, max_boundary);
> > +
> > +       return regmap_write(data->regmap, ADXL313_REG_TIME_INACT, val);
> > +}
>
> ...
>
> > -       axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
> > +       if (type == ADXL313_ACTIVITY)
> > +               axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
> > +       else
> > +               axis_en = FIELD_GET(ADXL313_INACT_XYZ_EN, axis_ctrl);
>
> Even with this change my previous comment stays.
>
> ...
>
> > +       en = cmd_en && threshold;
> > +       if (type == ADXL313_INACTIVITY) {
> > +               ret = regmap_read(data->regmap, ADXL313_REG_TIME_INACT, &inact_time_s);
> > +               if (ret)
> > +                       return ret;
> > +
> > +               en = en && inact_time_s;
> > +       }
>
> ...
>
> > -       if (info != IIO_EV_INFO_VALUE)
> > -               return -EINVAL;
> > -
> > -       /* Scale factor 15.625 mg/LSB */
> > -       regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
> > -       switch (dir) {
> > -       case IIO_EV_DIR_RISING:
> > -               ret = regmap_write(data->regmap,
> > -                                  adxl313_act_thresh_reg[ADXL313_ACTIVITY],
> > -                                  regval);
>
> Hmm... This was added by the previous patches, right? Why can't it be
> done as a switch case to begin with? I remember one of the previous
> versions had some nested switch-cases, perhaps you need to rethink on
> how to split the code between functions to avoid too much nesting (add
> some helper functions?).

The point here is, as I mentioned in the other mail:
Initially, I wanted to build up the final switch/case struct i.e.
going by MAG/MAG_ADAPTIVE, then INFO_VALUE -> RISING / FALLING and
PERIOD.

This will distinguish properties for four different types of events,
of course it then also will use separate functions. As I uderstood
your review, why starting with switch/case, do
if (!MAG event) then, return right away. I implemented that as I
understood. For further switch/case-ing, I did the same.
Now, patch by patch, it grows. Thus the if-not-back-out lines will be
moved out and replaced by switch/case. Worse, with every level switch
case, all existing code needs indention, thus reading through the
patches show (too) many changes.

How can I improve to help you reviewing this or make the feedback more
useful for me? Or is my approach wrong? I'd like to start with the
switch case right away, then just add up what comes in with every
other patch. If so, you'd only see the changes, since the final
structure of this is already clear, because very similar to all
iio/accel drivers at least (as you probably know better than me).

>
> > +       switch (info) {
> > +       case IIO_EV_INFO_VALUE:
> > +               /* Scale factor 15.625 mg/LSB */
> > +               regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
> > +               switch (dir) {
> > +               case IIO_EV_DIR_RISING:
> > +                       ret = regmap_write(data->regmap,
> > +                                          adxl313_act_thresh_reg[ADXL313_ACTIVITY],
> > +                                          regval);
> > +                       if (ret)
> > +                               return ret;
> > +                       return adxl313_set_measure_en(data, true);
> > +               case IIO_EV_DIR_FALLING:
> > +                       ret = regmap_write(data->regmap,
> > +                                          adxl313_act_thresh_reg[ADXL313_INACTIVITY],
> > +                                          regval);
> > +                       if (ret)
> > +                               return ret;
> > +                       return adxl313_set_measure_en(data, true);
> > +               default:
> > +                       return -EINVAL;
> > +               }
> > +       case IIO_EV_INFO_PERIOD:
> > +               ret = adxl313_set_inact_time_s(data, val);
> >                 if (ret)
> >                         return ret;
> >                 return adxl313_set_measure_en(data, true);
>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v4 07/11] iio: accel: adxl313: add activity sensing

[Jonathan Cameron]

> > >  static int adxl313_set_watermark(struct iio_dev *indio_dev, unsigned int value)
> > >  {
> > >       struct adxl313_data *data = iio_priv(indio_dev);
> > > @@ -502,19 +705,32 @@ static int adxl313_fifo_push(struct iio_dev *indio_dev, int samples)
> > >
> > >  static int adxl313_push_event(struct iio_dev *indio_dev, int int_stat)  
> >
> > Ah. This does not also have events.  Still it's a mix, so maybe
> > adxl313_handle_interrupts() or something like that.  
> 
> I also could break it up into:
> - handle interrupt source register events
> - drain fifo watermark samples
Sure - if that makes more logical sense that break up is fine.

> ?
> 
> > >  {
> > > +     s64 ts = iio_get_time_ns(indio_dev);
> > >       struct adxl313_data *data = iio_priv(indio_dev);
> > >       int samples;
> > > +     int ret = -ENOENT;
> > > +
> > > +     if (FIELD_GET(ADXL313_INT_ACTIVITY, 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_RISING),
> > > +                                  ts);
> > > +             if (ret)
> > > +                     return ret;
> > > +     }
> > >
> > >       if (FIELD_GET(ADXL313_INT_WATERMARK, int_stat)) {
> > >               samples = adxl313_get_samples(data);
> > >               if (samples < 0)
> > >                       return samples;
> > >
> > > -             return adxl313_fifo_push(indio_dev, samples);
> > > +             ret = adxl313_fifo_push(indio_dev, samples);
> > >       }
> > >
> > >       /* Return error if no event data was pushed to the IIO channel. */
> > > -     return -ENOENT;
> > > +     return ret;  
> > This handling works, but as Andy observed maybe the comment is now confusing
> > given ret is mostly not an error.  Perhaps put that where ret is declared
> > instead, or use a separate mask check at the start to quickly
> > error out if no bits that we handle are set.  
> > >  }  
> 
> Yes. Andy also pointed out here. I already developed a feeling for
> "something's smelly" with this code, but cannot really think of a
> better approach. Actually it works, and for me it is somehow logical.
> Probably there are better ways to solve this situation in a cleaner
> way?

The check against a mask at the start is pretty common way to deal with
no status bits (that we understand) set.  Then update that mask
define as you support more bits.

That deals with the odd error case.  It is technically an additional
conditional but the compiler may squash it anyway or if on a decent CPU
it'll be a very easy to predict branch as it should never happen.

Jonathan

> 

Re: [PATCH v4 08/11] iio: accel: adxl313: add inactivity sensing

[Jonathan Cameron]

On Wed, 11 Jun 2025 17:36:49 +0200
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> On Sun, Jun 1, 2025 at 9:46 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Sun, Jun 1, 2025 at 8:22 PM Lothar Rubusch <l.rubusch@gmail.com> wrote:  
> > >
> > > Extend the interrupt handler to process interrupts as inactivity events.
> > > Add functions to set threshold and period registers for inactivity. Add
> > > functions to enable / disable inactivity. Extend the fake iio channel to  
> >
> > IIO
> >  
> > > deal with inactivity events on x, y and z combined with AND.  
> >
> > ...
> >  
> > > +static int adxl313_set_inact_time_s(struct adxl313_data *data,
> > > +                                   unsigned int val_s)
> > > +{
> > > +       unsigned int max_boundary = 255;  
> >
> > This is unclear how it's defined. What is the limit behind? Size of a
> > bit field? Decimal value from the datasheet?
> >
> > The forms of (BIT(8) - 1) or GENMASK(7, 0) may be better depending on
> > the answers to the above questions.
> >  
> > > +       unsigned int val = min(val_s, max_boundary);
> > > +
> > > +       return regmap_write(data->regmap, ADXL313_REG_TIME_INACT, val);
> > > +}  
> >
> > ...
> >  
> > > -       axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
> > > +       if (type == ADXL313_ACTIVITY)
> > > +               axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
> > > +       else
> > > +               axis_en = FIELD_GET(ADXL313_INACT_XYZ_EN, axis_ctrl);  
> >
> > Even with this change my previous comment stays.
> >
> > ...
> >  
> > > +       en = cmd_en && threshold;
> > > +       if (type == ADXL313_INACTIVITY) {
> > > +               ret = regmap_read(data->regmap, ADXL313_REG_TIME_INACT, &inact_time_s);
> > > +               if (ret)
> > > +                       return ret;
> > > +
> > > +               en = en && inact_time_s;
> > > +       }  
> >
> > ...
> >  
> > > -       if (info != IIO_EV_INFO_VALUE)
> > > -               return -EINVAL;
> > > -
> > > -       /* Scale factor 15.625 mg/LSB */
> > > -       regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
> > > -       switch (dir) {
> > > -       case IIO_EV_DIR_RISING:
> > > -               ret = regmap_write(data->regmap,
> > > -                                  adxl313_act_thresh_reg[ADXL313_ACTIVITY],
> > > -                                  regval);  
> >
> > Hmm... This was added by the previous patches, right? Why can't it be
> > done as a switch case to begin with? I remember one of the previous
> > versions had some nested switch-cases, perhaps you need to rethink on
> > how to split the code between functions to avoid too much nesting (add
> > some helper functions?).  
> 
> The point here is, as I mentioned in the other mail:
> Initially, I wanted to build up the final switch/case struct i.e.
> going by MAG/MAG_ADAPTIVE, then INFO_VALUE -> RISING / FALLING and
> PERIOD.
> 
> This will distinguish properties for four different types of events,
> of course it then also will use separate functions. As I uderstood
> your review, why starting with switch/case, do
> if (!MAG event) then, return right away. I implemented that as I
> understood. For further switch/case-ing, I did the same.
> Now, patch by patch, it grows. Thus the if-not-back-out lines will be
> moved out and replaced by switch/case. Worse, with every level switch
> case, all existing code needs indention, thus reading through the
> patches show (too) many changes.
> 
> How can I improve to help you reviewing this or make the feedback more
> useful for me? Or is my approach wrong? I'd like to start with the
> switch case right away, then just add up what comes in with every
> other patch. If so, you'd only see the changes, since the final
> structure of this is already clear, because very similar to all
> iio/accel drivers at least (as you probably know better than me).
As mentioned earlier, reviewers tend to look at patches in a linear
fashion (and forget them more or less entirely between versions posted!)

So feel free to push back on earlier review comments that say 'you could
simplify this as X' with 'I did this because it becomes more complex in patch 4
and this reduces churn'.

Maybe here factoring out some elements into helpers will reduce the churn
anyway (to a couple of lines) and that discussion become unnecessary.

J
> 
> >  
> > > +       switch (info) {
> > > +       case IIO_EV_INFO_VALUE:
> > > +               /* Scale factor 15.625 mg/LSB */
> > > +               regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
> > > +               switch (dir) {
> > > +               case IIO_EV_DIR_RISING:
> > > +                       ret = regmap_write(data->regmap,
> > > +                                          adxl313_act_thresh_reg[ADXL313_ACTIVITY],
> > > +                                          regval);
> > > +                       if (ret)
> > > +                               return ret;
> > > +                       return adxl313_set_measure_en(data, true);
> > > +               case IIO_EV_DIR_FALLING:
> > > +                       ret = regmap_write(data->regmap,
> > > +                                          adxl313_act_thresh_reg[ADXL313_INACTIVITY],
> > > +                                          regval);
> > > +                       if (ret)
> > > +                               return ret;
> > > +                       return adxl313_set_measure_en(data, true);
> > > +               default:
> > > +                       return -EINVAL;
> > > +               }
> > > +       case IIO_EV_INFO_PERIOD:
> > > +               ret = adxl313_set_inact_time_s(data, val);
> > >                 if (ret)
> > >                         return ret;
> > >                 return adxl313_set_measure_en(data, true);  
> >
> > --
> > With Best Regards,
> > Andy Shevchenko  
> 

Re: [PATCH v4 10/11] iio: accel: adxl313: add AC coupled activity/inactivity events

[Lothar Rubusch]

Hi Andy,

On Sun, Jun 1, 2025 at 9:54 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sun, Jun 1, 2025 at 8:22 PM Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >
> > Add AC coupling activity and inactivity as MAG_ADAPTIVE events. This adds
> > up an additional set of threshold and period handles, verifies matching
> > disabling functionality and extends setting the link bit to complementary
> > event configurations.
> >
> > This means, e.g. either ACTIVITY or ACTIVITY_AC can be enabled. The most
> > recent set will remain configured. Disabling ACTIVITY where ACTIVITY_AC was
> > enabled is ignored, since it does not match (should be disabling
> > ACTIVITY_AC). When INACTIVITY or INACTIVITY_AC is also enabled, the link
> > bit will be set. Note, having the link bit and auto-sleep in place activity
> > and inactivity indicate the power save state change and thus will only be
> > triggered once a state transition occurs. Since there is a separate AC bit
> > for ACTIVITY and for INACTIVITY, events can be linked independently from
> > each other i.e. ACTIVITY can be linked to INACTIVITY_AC for instance.
> >
> > When one of both is disabled, the link bit will be removed. Hence, the
> > remaining event will not indicate a plain state change anymore, but occur
> > as a periodically triggered inactivity event or for each activity event
> > above the threshold.
>
> ...
>
> > +/**
> > + * adxl313_is_act_inact_ac() - Check if AC coupling is enabled.
>
> > + *
>
> Unneeded blank line.
>
> > + * @data: The device data.
> > + * @type: The activity or inactivity type.
> > + *
> > + * Provide a type of activity or inactivity, combined with either AC coupling
> > + * set, or default to DC coupling. This function verifies, if the combination is
> > + * currently enabled or not.
> > + *
> > + * Return if the provided activity type has AC coupling enabled or a negative
> > + * error value.
>
> Missing Return section. Always try kernel-doc validation when adding
> new kernel-doc descriptions.
>
> > + */
>
> ...
>
> >         unsigned int regval;
> > +       int coupling;
>
> Why? Doesn't 'ret' suffice?
>

the coupling variable here is rather meant to provide kind of a
semantic context. It shall be checked for being negative (error), or
used in binary decision logic. In fact, could be done with ret as
well, but then in case I'd need to comment that in this case the value
of 'ret' carries either error, or the bool if we have coupling or not.
I'd like to leave it like this, but let me know if better replace it
by ret here.

> >         int axis_en, int_en, ret;
>
> ...
>
> > -       int act_en, inact_en;
> > -       bool en;
> > +       int act_en, inact_en, act_ac_en, inact_ac_en;
> > +       bool en, act_inact_ac;
> >         int ret;
>
> For all your patches: try really hard to avoid the ping-pong coding,
> i.e. when you add something in one patch in the series and change in
> the other for no reason. I.o.w. when the initial code may be written
> already in a form that doesn't need further changes (e.g., switch-case
> vs. if).
>
> This patch is *very* noisy due to the above. So, just slow down, try a
> new approach that you have less '-' lines in the diff:s all over the
> code.

Agree. I tried to follow the review comments. Probably, IMHO it's
mostly about how to separate the patches. Your reviews seem to be
quite focussed on the particular patch w/o taking the context of
follow up patches so much into account. [At least by the way you gave
me feed back here. Actually, by your vast experience I'm pretty sure
you have the context of how such a driver shall look and have an
excellent overview well in mind.]

So, I guess you'd like to stress on certain points. I'm wondering if
it might probably be better to send you this all first in one big
patch, or say rather bigger patches, and then separate pieces out? Let
me know what you think. Thank you so much for the reviews, let's see
how this can be improved here in a v5.

Best,
L

>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v4 10/11] iio: accel: adxl313: add AC coupled activity/inactivity events

[Lothar Rubusch]

Hi Jonathan,

On Sun, Jun 8, 2025 at 6:23 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sun,  1 Jun 2025 17:21:38 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Add AC coupling activity and inactivity as MAG_ADAPTIVE events. This adds
> > up an additional set of threshold and period handles, verifies matching
> > disabling functionality and extends setting the link bit to complementary
> > event configurations.
> >
> > This means, e.g. either ACTIVITY or ACTIVITY_AC can be enabled. The most
> > recent set will remain configured. Disabling ACTIVITY where ACTIVITY_AC was
> > enabled is ignored, since it does not match (should be disabling
> > ACTIVITY_AC). When INACTIVITY or INACTIVITY_AC is also enabled, the link
> > bit will be set. Note, having the link bit and auto-sleep in place activity
> > and inactivity indicate the power save state change and thus will only be
> > triggered once a state transition occurs. Since there is a separate AC bit
> > for ACTIVITY and for INACTIVITY, events can be linked independently from
> > each other i.e. ACTIVITY can be linked to INACTIVITY_AC for instance.
> >
> > When one of both is disabled, the link bit will be removed. Hence, the
> > remaining event will not indicate a plain state change anymore, but occur
> > as a periodically triggered inactivity event or for each activity event
> > above the threshold.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
>
> Minor thought on rereading this.  If we don't have the link bit set
> (and the paired event) the AC events are more accurately described as
> MAG_REFERENCED as they are referenced simply to whatever acceleration
> was going on when they were first enabled.   Only with the link bit
> set (and the other event type enabled) are they actually adapting
> (so MAG_ADAPTIVE).
>

Going by examples, I can follow you as practically I'm aware of the
difference between a plain inactivity setup and a link-bit enabled
inactivity(and activity) setup. Initially I thought of MAG and the
AC-coupled equivalent being MAG_REFERENCED. By your explanation I
understand why you preferred MAG_ADAPTIVE rather. But still all three
configurations are possible.

My idea is, the driver implementation supports all cases in parallel,
at least to a certain extent. I mean, at the current implementation
someone can configure plain activity, or AC-coupled activity, or
respectively, their inactivity equivalents - when both, an activity
type together with an inactivity type are enabled, they will be linked
counter events. I.e. "adaptive" - and auto-sleep will be turned on for
the inactivity periods. Built on using just plain IIO API w/o custom
API calls.

Due to all the possible combinations, this comes at a certain
complexity. In terms of configuration and for instance mapping to MAG,
MAG_REFERNCED or MAG_ADAPTIVE. Here I rely on your feedback. On my
side, I'll try to recycle the automation setup to verify registers are
configured as I like them to be using the sysfs handles (that's btw
the reason why I'm glad to have debugfs on board). So, if you tell me,
to change it rather to MAG_REFERENCED, I'll do it, but then AC-coupled
events will be all MAG_REFRENCED w/ or w/o link bit. Or we come up
with a total different approach, like putting link-bit AC on
MAG_ADAPTIVE and plain AC-coupled on MAG_REFERENCED, but then what
about MAG events w/ or w/o link-bit? hmm, I think current approach
seems to be a good compromise. Let me know what you think.

Best,
L

>
> Maybe there is room to use that to ultimately control whether the
> link bit is set or not (putting aside the power aspect of that).
>

Re: [PATCH v4 00/11] iio: accel: adxl313: add power-save on activity/inactivity

[Lothar Rubusch]

Hi Bagas Sanjaya,

On Mon, Jun 2, 2025 at 3:07 AM Bagas Sanjaya <bagasdotme@gmail.com> wrote:
>
> On Sun, Jun 01, 2025 at 05:21:28PM +0000, Lothar Rubusch wrote:
> > The patch set covers the following topics:
> > - add debug register and regmap cache
> > - prepare iio channel scan_type and scan_index
> > - prepare interrupt handling
> > - implement fifo with watermark
> > - add activity/inactivity together with auto-sleep with link bit
> > - add ac coupled activity/inactivity, integrate with auto-sleep and link bit
> > - documentation
>
> The series doesn't cleanly apply on iio/testing tree. Base commit or tree?
>
> Confused...
>

I'm sorry for that. My base tree is "testing" here:
https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/log/?h=testing

Since this patch could be applied, I guess it could be a good base commit:
https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/?h=testing&id=d4d10d3535639b946007fb7ffb5bff2d878df921

Thank you for reviewing the documentation, I really appreciate your work.
Best,
L

> --
> An old man doll... just what I always wanted! - Clara

Re: [PATCH v4 00/11] iio: accel: adxl313: add power-save on activity/inactivity

[Andy Shevchenko]

On Wed, Jun 11, 2025 at 10:04:17PM +0200, Lothar Rubusch wrote:
> On Mon, Jun 2, 2025 at 3:07 AM Bagas Sanjaya <bagasdotme@gmail.com> wrote:
> >
> > On Sun, Jun 01, 2025 at 05:21:28PM +0000, Lothar Rubusch wrote:
> > > The patch set covers the following topics:
> > > - add debug register and regmap cache
> > > - prepare iio channel scan_type and scan_index
> > > - prepare interrupt handling
> > > - implement fifo with watermark
> > > - add activity/inactivity together with auto-sleep with link bit
> > > - add ac coupled activity/inactivity, integrate with auto-sleep and link bit
> > > - documentation
> >
> > The series doesn't cleanly apply on iio/testing tree. Base commit or tree?
> >
> > Confused...
> 
> I'm sorry for that. My base tree is "testing" here:
> https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/log/?h=testing
> 
> Since this patch could be applied, I guess it could be a good base commit:
> https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/?h=testing&id=d4d10d3535639b946007fb7ffb5bff2d878df921

Don't you use --base when creating series?
If not, use it.

Bagas, you should refer to the last lines of the (cover letter) message where
should be base commit written. When you use `b4` tool, it does automatically
for you find the correct base, so one needs just to call `b4 shazam -M
$MessageID` on top of the clean vanilla tag, and it will do the rest for you.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 10/11] iio: accel: adxl313: add AC coupled activity/inactivity events

[Jonathan Cameron]

On Wed, 11 Jun 2025 21:58:36 +0200
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Hi Jonathan,
> 
> On Sun, Jun 8, 2025 at 6:23 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Sun,  1 Jun 2025 17:21:38 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >  
> > > Add AC coupling activity and inactivity as MAG_ADAPTIVE events. This adds
> > > up an additional set of threshold and period handles, verifies matching
> > > disabling functionality and extends setting the link bit to complementary
> > > event configurations.
> > >
> > > This means, e.g. either ACTIVITY or ACTIVITY_AC can be enabled. The most
> > > recent set will remain configured. Disabling ACTIVITY where ACTIVITY_AC was
> > > enabled is ignored, since it does not match (should be disabling
> > > ACTIVITY_AC). When INACTIVITY or INACTIVITY_AC is also enabled, the link
> > > bit will be set. Note, having the link bit and auto-sleep in place activity
> > > and inactivity indicate the power save state change and thus will only be
> > > triggered once a state transition occurs. Since there is a separate AC bit
> > > for ACTIVITY and for INACTIVITY, events can be linked independently from
> > > each other i.e. ACTIVITY can be linked to INACTIVITY_AC for instance.
> > >
> > > When one of both is disabled, the link bit will be removed. Hence, the
> > > remaining event will not indicate a plain state change anymore, but occur
> > > as a periodically triggered inactivity event or for each activity event
> > > above the threshold.
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>  
> >
> > Minor thought on rereading this.  If we don't have the link bit set
> > (and the paired event) the AC events are more accurately described as
> > MAG_REFERENCED as they are referenced simply to whatever acceleration
> > was going on when they were first enabled.   Only with the link bit
> > set (and the other event type enabled) are they actually adapting
> > (so MAG_ADAPTIVE).
> >  
> 
> Going by examples, I can follow you as practically I'm aware of the
> difference between a plain inactivity setup and a link-bit enabled
> inactivity(and activity) setup. Initially I thought of MAG and the
> AC-coupled equivalent being MAG_REFERENCED. By your explanation I
> understand why you preferred MAG_ADAPTIVE rather. But still all three
> configurations are possible.
> 
> My idea is, the driver implementation supports all cases in parallel,
> at least to a certain extent. I mean, at the current implementation
> someone can configure plain activity, or AC-coupled activity, or
> respectively, their inactivity equivalents - when both, an activity
> type together with an inactivity type are enabled, they will be linked
> counter events. I.e. "adaptive" - and auto-sleep will be turned on for
> the inactivity periods. Built on using just plain IIO API w/o custom
> API calls.
> 
> Due to all the possible combinations, this comes at a certain
> complexity. In terms of configuration and for instance mapping to MAG,
> MAG_REFERNCED or MAG_ADAPTIVE. Here I rely on your feedback. On my
> side, I'll try to recycle the automation setup to verify registers are
> configured as I like them to be using the sysfs handles (that's btw
> the reason why I'm glad to have debugfs on board). So, if you tell me,
> to change it rather to MAG_REFERENCED, I'll do it, but then AC-coupled
> events will be all MAG_REFRENCED w/ or w/o link bit. Or we come up
> with a total different approach, like putting link-bit AC on
> MAG_ADAPTIVE and plain AC-coupled on MAG_REFERENCED, but then what
> about MAG events w/ or w/o link-bit? hmm, I think current approach
> seems to be a good compromise. Let me know what you think.

I don't have a good answer for how to handle this complexity :(
This was more about an earlier question I think you asked about whether
there was a way to have the user opt in to the link bit or not.

I'm fine with treating them all as adaptive and glossing over exactly
how it is adapting but that doesn't get us to link bit control if
we decide that wants to be explicitly exposed at all.

J
> 
> Best,
> L
> 
> >
> > Maybe there is room to use that to ultimately control whether the
> > link bit is set or not (putting aside the power aspect of that).
> >