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

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

[Lothar Rubusch]

Add several interrupt based sensor detection events:
- refactoring and fixes
- activity
- inactivity
- documentation

In brief, this implementation uses the inactivity registers for inactivity
events with a period >1s. For a configured period <1s the free-fall
registers will be used. The related features, such as setting the link-bit,
auto-sleep, the AC-coupling events, etc. are turned on/off accordingly. For
instance, activity and inactivity enabled imply a link-bit set. Disabling
one, removes the link-bit. Setting e.g. activity AC with inactivity DC, sets
the link bit and so on.

Due to several items found for the ADXL313, I tried to apply those refacs
here as well. I'm still unsure, if I should rathe wrap them up in one big
refac patch or keep them separated by the items as I tried here.

By the number of possible combinations of configurations and their
verification, as additionally the refactorings - I implemented an automated
verification by a labgrid/pytest setup with over 300 test steps and hardware
in the loop. Hopefully the setup is mostly correct. Further I extended the
codespell githook by an ispell check (which gave better results).

Let's see how good that works out. Somehow I'm sure you guys will (still)
find something...

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
v8 -> v9:
- githook: apply codespell checks, extend by spellcheck using ispell
- refac: apply `regmap_assing_bits()` in several places
- refac: remove ADXL345_POWER_CTL_STANDBY in adxl345.h not needed anymore
- refac: rename `ADXL345_ACT_INACT_DC/AC` to `ADXL345_COUPLING_DC/AC`
- refac: remove variable irq from `struct adxl345_state`
- refac: apply expressions, such as MILLI or MICRO from linux/units.h
- refac: apply (missing) scaling factor 62.5mg/LSB to tap detection
- change `IIO_EV_TYPE_MAG_REFERENCED` to `IIO_EV_TYPE_MAG_ADAPTIVE`
- `adxl345_fifo_transfer()`: eliminate variable for expression count / 2
- `adxl345_is_act_inact_ac()`: make return boolean, or negative error
- make activity enable cover x, y and z axis together, while signals come
  on particular axis
- `adxl345_read_mag_value()`: separate `MAG` and `MAG_ADAPTIVE` event
  value read and write function to reduce redundant code
- `adxl345_read_mag_config()`: separate `MAG` and `MAG_ADAPTIVE` event
  config read and write functions to reduce redundant code
- `adxl345_set_act_inact_en()`: move linkbit detection out to
  `adxl345_set_act_inact_linkbit()`
- `adxl345_set_act_inact_en()`: fix unsetting register INT ENABLE at
  disabling feature(s)
- apply scaling factor 62.5mg/LSB to activity/inactivity
- apply scaling factor 62.5mg/LSB to activity AC/inactivity AC
- drop dedicated freefall patch
- add patch: fix missing scale factor for tap detection
- add patch: make irq a function local variable
- add patch: simplify measure enable
- add patch: simplify interrupt mapping
- add patch: simplify FIFO reading
- add patch: replace magic numbers by unit expressions
- rename `adxl345_set_inact_time_s()` to `adxl345_set_inact_time()`
- `adxl345_set_inact_time()`: implement inactivity using inactivity
  register (period 1s or higher), freefall register (below 1s) or let it
  setup a period based on given ODR (0s provided)
- doc: update documentation

v7 -> v8:
- activity/inactivity are MAG events
- separate AC coupled activity/inactivity events as MAG_REFERENCED events,
  since AC coupling introduces a (some kind of) reference relation
- since freefall and inactivity (DC coupled) are then actually identical,
  this results in a challenging situation for the freefall patch. Thus,
  the freefall patch is moved to end of this series (before documentation)
- freefall: provide separate sysfs handles to configure and enable freefall
- documentation: update sections on activity/inactivity, freefall, event
  names and examples

v6 -> v7:
- freefall: add a virtual channel, replace OR'ing the axis by AND'ing them
- inactivity: add a virtual channel, replace OR'ing the axis by AND'ing them

v5 -> v6:
- replace bool axis_en for tap and activity/inactivity
- apply freefall bit mask
- change `measure_en` to use `regmap_update_bits()` for POWER_CTL register
- fix comments and update documentation, particularly on inactivity time

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

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

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

v1 -> v2:
- implementation of all events (but tap2 suppress bit) by means IIO ABI
- add sample frequency / ODR configuration
- add g ranges configuration
- add activity/inactivity using auto-sleep and powersave
- add dynamic adjustment of default values for
  activity/inactivity thresholds and time for inactivity based on ODR
  and g range (can be overwritten)
- add sensor documentation
---
Lothar Rubusch (11):
  iio: accel: adxl345: apply scale factor to tap threshold
  iio: accel: adxl345: make data struct variable irq function local
  iio: accel: adxl345: simplify measure enable
  iio: accel: adxl345: simplify interrupt mapping
  iio: accel: adxl345: simplify reading the FIFO
  iio: accel: adxl345: replace magic numbers by unit expressions
  iio: accel: adxl345: add activity event feature
  iio: accel: adxl345: add inactivity feature
  iio: accel: adxl345: add coupling detection for activity/inactivity
  iio: accel: adxl345: extend inactivity time for less than 1s
  docs: iio: add documentation for adxl345 driver

 Documentation/iio/adxl345.rst    | 429 +++++++++++++++++
 Documentation/iio/index.rst      |   1 +
 drivers/iio/accel/adxl345.h      |   1 -
 drivers/iio/accel/adxl345_core.c | 793 +++++++++++++++++++++++++++++--
 4 files changed, 1195 insertions(+), 29 deletions(-)
 create mode 100644 Documentation/iio/adxl345.rst

-- 
2.39.5

[PATCH v9 01/11] iio: accel: adxl345: apply scale factor to tap threshold

[Lothar Rubusch]

The threshold for tap detection was still not scaled. The datasheet sets
a scale factor of 62.5mg/LSB. Remove commit about not scaled threshold
for tap detection, and apply scaling to it.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 7c093c0241de..d80efb68d113 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -697,17 +697,15 @@ static int adxl345_read_event_value(struct iio_dev *indio_dev,
 		switch (info) {
 		case IIO_EV_INFO_VALUE:
 			/*
-			 * The scale factor would be 62.5mg/LSB (i.e. 0xFF = 16g) but
-			 * not applied here. In context of this general purpose sensor,
-			 * what imports is rather signal intensity than the absolute
-			 * measured g value.
+			 * Scale factor is 62.5mg/LSB i.e. 0xff = 16g
 			 */
 			ret = regmap_read(st->regmap, ADXL345_REG_THRESH_TAP,
 					  &tap_threshold);
 			if (ret)
 				return ret;
-			*val = sign_extend32(tap_threshold, 7);
-			return IIO_VAL_INT;
+			*val = 62500 * sign_extend32(tap_threshold, 7);
+			*val2 = MICRO;
+			return IIO_VAL_FRACTIONAL;
 		case IIO_EV_INFO_TIMEOUT:
 			*val = st->tap_duration_us;
 			*val2 = 1000000;
@@ -746,6 +744,7 @@ static int adxl345_write_event_value(struct iio_dev *indio_dev,
 	case IIO_EV_TYPE_GESTURE:
 		switch (info) {
 		case IIO_EV_INFO_VALUE:
+			val = DIV_ROUND_CLOSEST(val * MICRO + val2, 62500);
 			ret = regmap_write(st->regmap, ADXL345_REG_THRESH_TAP,
 					   min(val, 0xFF));
 			if (ret)
-- 
2.39.5

[PATCH v9 02/11] iio: accel: adxl345: make data struct variable irq function local

[Lothar Rubusch]

Remove variable irq from the struct state and make it a function local
variable, because it is not necessary to be kept struct-wise.

This is a refactoring change and should not impact functionality.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index d80efb68d113..07abab82d093 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -133,7 +133,6 @@ struct adxl345_state {
 	const struct adxl345_chip_info *info;
 	struct regmap *regmap;
 	bool fifo_delay; /* delay: delay is needed for SPI */
-	int irq;
 	u8 watermark;
 	u8 fifo_mode;
 
@@ -1119,6 +1118,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 					 ADXL345_DATA_FORMAT_FULL_RES |
 					 ADXL345_DATA_FORMAT_SELF_TEST);
 	unsigned int tap_threshold;
+	int irq;
 	int ret;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
@@ -1203,11 +1203,11 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 	if (ret)
 		return ret;
 
-	st->irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT1");
-	if (st->irq < 0) {
+	irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT1");
+	if (irq < 0) {
 		intio = ADXL345_INT2;
-		st->irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT2");
-		if (st->irq < 0)
+		irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT2");
+		if (irq < 0)
 			intio = ADXL345_INT_NONE;
 	}
 
@@ -1232,7 +1232,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 		if (ret)
 			return ret;
 
-		ret = devm_request_threaded_irq(dev, st->irq, NULL,
+		ret = devm_request_threaded_irq(dev, irq, NULL,
 						&adxl345_irq_handler,
 						IRQF_SHARED | IRQF_ONESHOT,
 						indio_dev->name, indio_dev);
-- 
2.39.5

[PATCH v9 03/11] iio: accel: adxl345: simplify measure enable

[Lothar Rubusch]

Simplify the function to enable or disable measurement. Replace the
separate decision logic and call to regmap_update_bits() by a single
call to regmap_assign_bits() taking a boolean argument directly.

This is a refactoring change and should not impact functionality.

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

diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
index 6c1f96406136..9385affdefe3 100644
--- a/drivers/iio/accel/adxl345.h
+++ b/drivers/iio/accel/adxl345.h
@@ -73,7 +73,6 @@
 #define ADXL345_BW_LOW_POWER		BIT(4)
 #define ADXL345_BASE_RATE_NANO_HZ	97656250LL
 
-#define ADXL345_POWER_CTL_STANDBY	0x00
 #define ADXL345_POWER_CTL_WAKEUP	GENMASK(1, 0)
 #define ADXL345_POWER_CTL_SLEEP	BIT(2)
 #define ADXL345_POWER_CTL_MEASURE	BIT(3)
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 07abab82d093..cae9e37e216f 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -233,9 +233,8 @@ EXPORT_SYMBOL_NS_GPL(adxl345_is_volatile_reg, "IIO_ADXL345");
  */
 static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
 {
-	unsigned int val = en ? ADXL345_POWER_CTL_MEASURE : ADXL345_POWER_CTL_STANDBY;
-
-	return regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
+	return regmap_assign_bits(st->regmap, ADXL345_REG_POWER_CTL,
+				  ADXL345_POWER_CTL_MEASURE, en);
 }
 
 /* tap */
-- 
2.39.5

[PATCH v9 04/11] iio: accel: adxl345: simplify interrupt mapping

[Lothar Rubusch]

Replace mapping all sensor interrupts to the corresponding interrupt
line using regmap_assign_bits() since it takes a boolean directly.
Further prefer the units.h identifier to cover the full register when bits
are set.

This is a refactoring change and should not impact functionality.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index cae9e37e216f..18c625d323ba 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -1216,9 +1216,8 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 		 * interrupts to the INT1 pin, whereas bits set to 1 send their respective
 		 * interrupts to the INT2 pin. The intio shall convert this accordingly.
 		 */
-		regval = intio ? 0xff : 0;
-
-		ret = regmap_write(st->regmap, ADXL345_REG_INT_MAP, regval);
+		ret = regmap_assign_bits(st->regmap, ADXL345_REG_INT_MAP,
+					 U8_MAX, intio);
 		if (ret)
 			return ret;
 
-- 
2.39.5

[PATCH v9 05/11] iio: accel: adxl345: simplify reading the FIFO

[Lothar Rubusch]

Bulk reading from the FIFO can be simplified. Remove unnecessary
variables and simplify reading sets of x-, y- and z-axis measurements.

This is a refactoring change and should not impact functionality.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 18c625d323ba..dcfbfe4cac0f 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -884,15 +884,13 @@ static int adxl345_get_samples(struct adxl345_state *st)
  */
 static int adxl345_fifo_transfer(struct adxl345_state *st, int samples)
 {
-	size_t count;
 	int i, ret = 0;
 
-	/* count is the 3x the fifo_buf element size, hence 6B */
-	count = sizeof(st->fifo_buf[0]) * ADXL345_DIRS;
 	for (i = 0; i < samples; i++) {
 		/* read 3x 2 byte elements from base address into next fifo_buf position */
 		ret = regmap_bulk_read(st->regmap, ADXL345_REG_XYZ_BASE,
-				       st->fifo_buf + (i * count / 2), count);
+				       st->fifo_buf + (i * ADXL345_DIRS),
+				       2 * ADXL345_DIRS);
 		if (ret)
 			return ret;
 
-- 
2.39.5

[PATCH v9 06/11] iio: accel: adxl345: replace magic numbers by unit expressions

[Lothar Rubusch]

Replace absolute numbers by their expressions from units.h to avoid
using magic numbers. Use uniform expressions to clarify their usage.

This is a refactoring change and should not impact functionality.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index dcfbfe4cac0f..2c4f045c741c 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -706,15 +706,15 @@ static int adxl345_read_event_value(struct iio_dev *indio_dev,
 			return IIO_VAL_FRACTIONAL;
 		case IIO_EV_INFO_TIMEOUT:
 			*val = st->tap_duration_us;
-			*val2 = 1000000;
+			*val2 = MICRO;
 			return IIO_VAL_FRACTIONAL;
 		case IIO_EV_INFO_RESET_TIMEOUT:
 			*val = st->tap_window_us;
-			*val2 = 1000000;
+			*val2 = MICRO;
 			return IIO_VAL_FRACTIONAL;
 		case IIO_EV_INFO_TAP2_MIN_DELAY:
 			*val = st->tap_latent_us;
-			*val2 = 1000000;
+			*val2 = MICRO;
 			return IIO_VAL_FRACTIONAL;
 		default:
 			return -EINVAL;
-- 
2.39.5

[PATCH v9 07/11] iio: accel: adxl345: add activity event feature

[Lothar Rubusch]

Make the sensor detect and issue interrupts at activity. Activity
events are configured by a threshold. Initialize the activity threshold
register to a reasonable default value in probe. The value is taken from
the older ADXL345 input driver, to provide a similar behavior.

Activity, ODR configuration together with the range setting prepare the
activity/inactivity hysteresis setup, implemented in a follow up patch.
Thus parts of this patch prepare switch/case setups for the follow up
patches.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 2c4f045c741c..04b9f155872f 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -36,11 +36,16 @@
 #define ADXL345_REG_TAP_AXIS_MSK	GENMASK(2, 0)
 #define ADXL345_REG_TAP_SUPPRESS_MSK	BIT(3)
 #define ADXL345_REG_TAP_SUPPRESS	BIT(3)
+#define ADXL345_REG_ACT_AXIS_MSK	GENMASK(6, 4)
 
 #define ADXL345_TAP_Z_EN		BIT(0)
 #define ADXL345_TAP_Y_EN		BIT(1)
 #define ADXL345_TAP_X_EN		BIT(2)
 
+#define ADXL345_ACT_Z_EN		BIT(4)
+#define ADXL345_ACT_Y_EN		BIT(5)
+#define ADXL345_ACT_X_EN		BIT(6)
+
 /* single/double tap */
 enum adxl345_tap_type {
 	ADXL345_SINGLE_TAP,
@@ -64,6 +69,19 @@ static const unsigned int adxl345_tap_time_reg[] = {
 	[ADXL345_TAP_TIME_DUR] = ADXL345_REG_DUR,
 };
 
+/* activity/inactivity */
+enum adxl345_activity_type {
+	ADXL345_ACTIVITY,
+};
+
+static const unsigned int adxl345_act_int_reg[] = {
+	[ADXL345_ACTIVITY] = ADXL345_INT_ACTIVITY,
+};
+
+static const unsigned int adxl345_act_thresh_reg[] = {
+	[ADXL345_ACTIVITY] = ADXL345_REG_THRESH_ACT,
+};
+
 enum adxl345_odr {
 	ADXL345_ODR_0P10HZ = 0,
 	ADXL345_ODR_0P20HZ,
@@ -144,6 +162,13 @@ struct adxl345_state {
 };
 
 static struct iio_event_spec adxl345_events[] = {
+	{
+		/* activity */
+		.type = IIO_EV_TYPE_MAG,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE) |
+			BIT(IIO_EV_INFO_VALUE),
+	},
 	{
 		/* single tap */
 		.type = IIO_EV_TYPE_GESTURE,
@@ -237,6 +262,90 @@ static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
 				  ADXL345_POWER_CTL_MEASURE, en);
 }
 
+/* act/inact */
+
+static int adxl345_is_act_inact_en(struct adxl345_state *st,
+				   enum adxl345_activity_type type)
+{
+	unsigned int regval;
+	u32 axis_ctrl;
+	bool en;
+	int ret;
+
+	ret = regmap_read(st->regmap, ADXL345_REG_ACT_INACT_CTRL, &axis_ctrl);
+	if (ret)
+		return ret;
+
+	switch (type) {
+	case ADXL345_ACTIVITY:
+		en = FIELD_GET(ADXL345_ACT_X_EN, axis_ctrl) |
+			FIELD_GET(ADXL345_ACT_Y_EN, axis_ctrl) |
+			FIELD_GET(ADXL345_ACT_Z_EN, axis_ctrl);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (!en)
+		return en;
+
+	/* Check if corresponding interrupts are enabled */
+	ret = regmap_read(st->regmap, ADXL345_REG_INT_ENABLE, &regval);
+	if (ret)
+		return ret;
+
+	return adxl345_act_int_reg[type] & regval;
+}
+
+static int adxl345_set_act_inact_en(struct adxl345_state *st,
+				    enum adxl345_activity_type type,
+				    bool cmd_en)
+{
+	bool en;
+	unsigned int threshold;
+	u32 axis_ctrl;
+	int ret;
+
+	if (type == ADXL345_ACTIVITY) {
+		axis_ctrl = ADXL345_ACT_X_EN | ADXL345_ACT_Y_EN |
+				ADXL345_ACT_Z_EN;
+	} else {
+		axis_ctrl = 0x00;
+	}
+
+	/* Start configuring the sensor registers */
+	ret = adxl345_set_measure_en(st, false);
+	if (ret)
+		return ret;
+
+	ret = regmap_assign_bits(st->regmap, ADXL345_REG_ACT_INACT_CTRL,
+				 axis_ctrl, cmd_en);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(st->regmap, adxl345_act_thresh_reg[type], &threshold);
+	if (ret)
+		return ret;
+
+	en = false;
+
+	switch (type) {
+	case ADXL345_ACTIVITY:
+		en = FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, axis_ctrl) &&
+			threshold;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = regmap_assign_bits(st->regmap, ADXL345_REG_INT_ENABLE,
+				 adxl345_act_int_reg[type], cmd_en && en);
+	if (ret)
+		return ret;
+
+	return adxl345_set_measure_en(st, true);
+}
+
 /* tap */
 
 static int _adxl345_set_tap_int(struct adxl345_state *st,
@@ -634,6 +743,13 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
 	int ret;
 
 	switch (type) {
+	case IIO_EV_TYPE_MAG:
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			return adxl345_is_act_inact_en(st, ADXL345_ACTIVITY);
+		default:
+			return -EINVAL;
+		}
 	case IIO_EV_TYPE_GESTURE:
 		switch (dir) {
 		case IIO_EV_DIR_SINGLETAP:
@@ -665,6 +781,15 @@ static int adxl345_write_event_config(struct iio_dev *indio_dev,
 	struct adxl345_state *st = iio_priv(indio_dev);
 
 	switch (type) {
+	case IIO_EV_TYPE_MAG:
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			return adxl345_set_act_inact_en(st,
+							ADXL345_ACTIVITY,
+							state);
+		default:
+			return -EINVAL;
+		}
 	case IIO_EV_TYPE_GESTURE:
 		switch (dir) {
 		case IIO_EV_DIR_SINGLETAP:
@@ -687,10 +812,30 @@ static int adxl345_read_event_value(struct iio_dev *indio_dev,
 				    int *val, int *val2)
 {
 	struct adxl345_state *st = iio_priv(indio_dev);
+	unsigned int act_threshold;
 	unsigned int tap_threshold;
 	int ret;
 
 	switch (type) {
+	case IIO_EV_TYPE_MAG:
+		switch (info) {
+		case IIO_EV_INFO_VALUE:
+			switch (dir) {
+			case IIO_EV_DIR_RISING:
+				ret = regmap_read(st->regmap,
+						  adxl345_act_thresh_reg[ADXL345_ACTIVITY],
+						  &act_threshold);
+				if (ret)
+					return ret;
+				*val = 62500 * act_threshold;
+				*val2 = MICRO;
+				return IIO_VAL_FRACTIONAL;
+			default:
+				return -EINVAL;
+			}
+		default:
+			return -EINVAL;
+		}
 	case IIO_EV_TYPE_GESTURE:
 		switch (info) {
 		case IIO_EV_INFO_VALUE:
@@ -739,6 +884,26 @@ static int adxl345_write_event_value(struct iio_dev *indio_dev,
 		return ret;
 
 	switch (type) {
+	case IIO_EV_TYPE_MAG:
+		switch (info) {
+		case IIO_EV_INFO_VALUE:
+			switch (dir) {
+			case IIO_EV_DIR_RISING:
+				val = DIV_ROUND_CLOSEST(val * MICRO + val2, 62500);
+				ret = regmap_write(st->regmap,
+						   adxl345_act_thresh_reg[ADXL345_ACTIVITY],
+						   val);
+				if (ret)
+					return ret;
+				break;
+			default:
+				return -EINVAL;
+			}
+			break;
+		default:
+			return -EINVAL;
+		}
+		break;
 	case IIO_EV_TYPE_GESTURE:
 		switch (info) {
 		case IIO_EV_INFO_VALUE:
@@ -980,7 +1145,8 @@ static int adxl345_fifo_push(struct iio_dev *indio_dev,
 }
 
 static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
-			      enum iio_modifier tap_dir)
+			      enum iio_modifier tap_dir,
+			      enum iio_modifier act_dir)
 {
 	s64 ts = iio_get_time_ns(indio_dev);
 	struct adxl345_state *st = iio_priv(indio_dev);
@@ -1007,6 +1173,16 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
 			return ret;
 	}
 
+	if (FIELD_GET(ADXL345_INT_ACTIVITY, int_stat)) {
+		ret = iio_push_event(indio_dev,
+				     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0, act_dir,
+							IIO_EV_TYPE_MAG,
+							IIO_EV_DIR_RISING),
+				     ts);
+		if (ret)
+			return ret;
+	}
+
 	if (FIELD_GET(ADXL345_INT_WATERMARK, int_stat)) {
 		samples = adxl345_get_samples(st);
 		if (samples < 0)
@@ -1034,6 +1210,7 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
 	struct adxl345_state *st = iio_priv(indio_dev);
 	unsigned int regval;
 	enum iio_modifier tap_dir = IIO_NO_MOD;
+	enum iio_modifier act_dir = IIO_NO_MOD;
 	u32 axis_ctrl;
 	int int_stat;
 	int ret;
@@ -1042,7 +1219,8 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
 	if (ret)
 		return IRQ_NONE;
 
-	if (FIELD_GET(ADXL345_REG_TAP_AXIS_MSK, axis_ctrl)) {
+	if (FIELD_GET(ADXL345_REG_TAP_AXIS_MSK, axis_ctrl) ||
+	    FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, axis_ctrl)) {
 		ret = regmap_read(st->regmap, ADXL345_REG_ACT_TAP_STATUS, &regval);
 		if (ret)
 			return IRQ_NONE;
@@ -1053,12 +1231,19 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
 			tap_dir = IIO_MOD_Y;
 		else if (FIELD_GET(ADXL345_TAP_X_EN, regval))
 			tap_dir = IIO_MOD_X;
+
+		if (FIELD_GET(ADXL345_ACT_Z_EN, regval))
+			act_dir = IIO_MOD_Z;
+		else if (FIELD_GET(ADXL345_ACT_Y_EN, regval))
+			act_dir = IIO_MOD_Y;
+		else if (FIELD_GET(ADXL345_ACT_X_EN, regval))
+			act_dir = IIO_MOD_X;
 	}
 
 	if (regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, &int_stat))
 		return IRQ_NONE;
 
-	if (adxl345_push_event(indio_dev, int_stat, tap_dir))
+	if (adxl345_push_event(indio_dev, int_stat, tap_dir, act_dir))
 		goto err;
 
 	if (FIELD_GET(ADXL345_INT_OVERRUN, int_stat))
@@ -1219,6 +1404,20 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 		if (ret)
 			return ret;
 
+		/*
+		 * Initialization with reasonable values to simplify operation
+		 * of the sensor. The default values are partly taken from the
+		 * older input driver for the ADXL345, and partly based on
+		 * recommendations in the datasheet.
+		 */
+		ret = regmap_write(st->regmap, ADXL345_REG_ACT_INACT_CTRL, 0);
+		if (ret)
+			return ret;
+
+		ret = regmap_write(st->regmap, ADXL345_REG_THRESH_ACT, 6);
+		if (ret)
+			return ret;
+
 		ret = regmap_write(st->regmap, ADXL345_REG_THRESH_TAP, tap_threshold);
 		if (ret)
 			return ret;
-- 
2.39.5

[PATCH v9 08/11] iio: accel: adxl345: add inactivity feature

[Lothar Rubusch]

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

Inactivity detection needs a threshold to be configured and a period of
time in seconds. After, it will transition to inactivity state, if
measurements stay below inactivity threshold.

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

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

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 04b9f155872f..1670315ebd63 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -37,11 +37,17 @@
 #define ADXL345_REG_TAP_SUPPRESS_MSK	BIT(3)
 #define ADXL345_REG_TAP_SUPPRESS	BIT(3)
 #define ADXL345_REG_ACT_AXIS_MSK	GENMASK(6, 4)
+#define ADXL345_REG_INACT_AXIS_MSK	GENMASK(2, 0)
+#define ADXL345_POWER_CTL_INACT_MSK	(ADXL345_POWER_CTL_AUTO_SLEEP | ADXL345_POWER_CTL_LINK)
 
 #define ADXL345_TAP_Z_EN		BIT(0)
 #define ADXL345_TAP_Y_EN		BIT(1)
 #define ADXL345_TAP_X_EN		BIT(2)
 
+#define ADXL345_INACT_Z_EN		BIT(0)
+#define ADXL345_INACT_Y_EN		BIT(1)
+#define ADXL345_INACT_X_EN		BIT(2)
+
 #define ADXL345_ACT_Z_EN		BIT(4)
 #define ADXL345_ACT_Y_EN		BIT(5)
 #define ADXL345_ACT_X_EN		BIT(6)
@@ -72,14 +78,17 @@ static const unsigned int adxl345_tap_time_reg[] = {
 /* activity/inactivity */
 enum adxl345_activity_type {
 	ADXL345_ACTIVITY,
+	ADXL345_INACTIVITY,
 };
 
 static const unsigned int adxl345_act_int_reg[] = {
 	[ADXL345_ACTIVITY] = ADXL345_INT_ACTIVITY,
+	[ADXL345_INACTIVITY] = ADXL345_INT_INACTIVITY,
 };
 
 static const unsigned int adxl345_act_thresh_reg[] = {
 	[ADXL345_ACTIVITY] = ADXL345_REG_THRESH_ACT,
+	[ADXL345_INACTIVITY] = ADXL345_REG_THRESH_INACT,
 };
 
 enum adxl345_odr {
@@ -147,6 +156,14 @@ static const int adxl345_fullres_range_tbl[][2] = {
 	[ADXL345_16G_RANGE] = { 0, 38312 },
 };
 
+/* scaling */
+static const int adxl345_range_factor_tbl[] = {
+	[ADXL345_2G_RANGE]  = 1,
+	[ADXL345_4G_RANGE]  = 2,
+	[ADXL345_8G_RANGE]  = 4,
+	[ADXL345_16G_RANGE] = 8,
+};
+
 struct adxl345_state {
 	const struct adxl345_chip_info *info;
 	struct regmap *regmap;
@@ -213,10 +230,29 @@ enum adxl345_chans {
 	chan_x, chan_y, chan_z,
 };
 
+static const struct iio_event_spec adxl345_fake_chan_events[] = {
+	{
+		/* inactivity */
+		.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),
+	},
+};
+
 static const struct iio_chan_spec adxl345_channels[] = {
 	ADXL345_CHANNEL(0, chan_x, X),
 	ADXL345_CHANNEL(1, chan_y, Y),
 	ADXL345_CHANNEL(2, chan_z, Z),
+	{
+		.type = IIO_ACCEL,
+		.modified = 1,
+		.channel2 = IIO_MOD_X_AND_Y_AND_Z,
+		.scan_index = -1, /* Fake channel */
+		.event_spec = adxl345_fake_chan_events,
+		.num_event_specs = ARRAY_SIZE(adxl345_fake_chan_events),
+	},
 };
 
 static const unsigned long adxl345_scan_masks[] = {
@@ -264,6 +300,51 @@ static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
 
 /* act/inact */
 
+/**
+ * adxl345_set_inact_time - Configure inactivity time explicitly or by ODR.
+ * @st: The sensor state instance.
+ * @val_s: A desired time value, between 0 and 255.
+ *
+ * Inactivity time can be configured between 1 and 255 sec. If a val_s of 0
+ * is configured by a user, then a default inactivity time will be computed.
+ *
+ * In such case, it should take power consumption into consideration. Thus it
+ * shall be shorter for higher frequencies and longer for lower frequencies.
+ * Hence, frequencies above 255 Hz shall default to 10 s and frequencies below
+ * 10 Hz shall result in 255 s to detect inactivity.
+ *
+ * The approach simply subtracts the pre-decimal figure of the configured
+ * sample frequency from 255 s to compute inactivity time [s]. Sub-Hz are thus
+ * ignored in this estimation. The recommended ODRs for various features
+ * (activity/inactivity, sleep modes, free fall, etc.) lie between 12.5 Hz and
+ * 400 Hz, thus higher or lower frequencies will result in the boundary
+ * defaults or need to be explicitly specified via val_s.
+ *
+ * Return: 0 or error value.
+ */
+static int adxl345_set_inact_time(struct adxl345_state *st, u32 val_s)
+{
+	unsigned int max_boundary = 255;
+	unsigned int min_boundary = 10;
+	unsigned int val = min(val_s, max_boundary);
+	enum adxl345_odr odr;
+	unsigned int regval;
+	int ret;
+
+	if (val == 0) {
+		ret = regmap_read(st->regmap, ADXL345_REG_BW_RATE, &regval);
+		if (ret)
+			return ret;
+
+		odr = FIELD_GET(ADXL345_BW_RATE_MSK, regval);
+
+		val = (adxl345_odr_tbl[odr][0] > max_boundary)
+			? min_boundary : max_boundary -	adxl345_odr_tbl[odr][0];
+	}
+
+	return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
+}
+
 static int adxl345_is_act_inact_en(struct adxl345_state *st,
 				   enum adxl345_activity_type type)
 {
@@ -282,6 +363,11 @@ static int adxl345_is_act_inact_en(struct adxl345_state *st,
 			FIELD_GET(ADXL345_ACT_Y_EN, axis_ctrl) |
 			FIELD_GET(ADXL345_ACT_Z_EN, axis_ctrl);
 		break;
+	case ADXL345_INACTIVITY:
+		en = FIELD_GET(ADXL345_INACT_X_EN, axis_ctrl) |
+			FIELD_GET(ADXL345_INACT_Y_EN, axis_ctrl) |
+			FIELD_GET(ADXL345_INACT_Z_EN, axis_ctrl);
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -302,22 +388,25 @@ static int adxl345_set_act_inact_en(struct adxl345_state *st,
 				    bool cmd_en)
 {
 	bool en;
+	unsigned int inact_time_s;
 	unsigned int threshold;
 	u32 axis_ctrl;
+	int act_en, inact_en;
 	int ret;
 
+	/* Start configuring the sensor registers */
+	ret = adxl345_set_measure_en(st, false);
+	if (ret)
+		return ret;
+
 	if (type == ADXL345_ACTIVITY) {
 		axis_ctrl = ADXL345_ACT_X_EN | ADXL345_ACT_Y_EN |
 				ADXL345_ACT_Z_EN;
 	} else {
-		axis_ctrl = 0x00;
+		axis_ctrl = ADXL345_INACT_X_EN | ADXL345_INACT_Y_EN |
+				ADXL345_INACT_Z_EN;
 	}
 
-	/* Start configuring the sensor registers */
-	ret = adxl345_set_measure_en(st, false);
-	if (ret)
-		return ret;
-
 	ret = regmap_assign_bits(st->regmap, ADXL345_REG_ACT_INACT_CTRL,
 				 axis_ctrl, cmd_en);
 	if (ret)
@@ -334,12 +423,37 @@ static int adxl345_set_act_inact_en(struct adxl345_state *st,
 		en = FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, axis_ctrl) &&
 			threshold;
 		break;
+	case ADXL345_INACTIVITY:
+		ret = regmap_read(st->regmap, ADXL345_REG_TIME_INACT, &inact_time_s);
+		if (ret)
+			return ret;
+
+		en = FIELD_GET(ADXL345_REG_INACT_AXIS_MSK, axis_ctrl) &&
+			threshold && inact_time_s;
+		break;
 	default:
 		return -EINVAL;
 	}
 
 	ret = regmap_assign_bits(st->regmap, ADXL345_REG_INT_ENABLE,
-				 adxl345_act_int_reg[type], cmd_en && en);
+				 adxl345_act_int_reg[type], cmd_en);
+	if (ret)
+		return ret;
+
+	/* Set sleep and link bit when ACT and INACT are enabled. */
+	act_en = adxl345_is_act_inact_en(st, ADXL345_ACTIVITY);
+	if (act_en < 0)
+		return act_en;
+
+	inact_en = adxl345_is_act_inact_en(st, ADXL345_INACTIVITY);
+	if (inact_en < 0)
+		return inact_en;
+
+	en = en && act_en && inact_en;
+
+	ret = regmap_assign_bits(st->regmap, ADXL345_REG_POWER_CTL,
+				 (ADXL345_POWER_CTL_AUTO_SLEEP | ADXL345_POWER_CTL_LINK),
+				 en);
 	if (ret)
 		return ret;
 
@@ -575,9 +689,16 @@ static int adxl345_find_odr(struct adxl345_state *st, int val,
 
 static int adxl345_set_odr(struct adxl345_state *st, enum adxl345_odr odr)
 {
-	return regmap_update_bits(st->regmap, ADXL345_REG_BW_RATE,
+	int ret;
+
+	ret = regmap_update_bits(st->regmap, ADXL345_REG_BW_RATE,
 				 ADXL345_BW_RATE_MSK,
 				 FIELD_PREP(ADXL345_BW_RATE_MSK, odr));
+	if (ret)
+		return ret;
+
+	/* update inactivity time by ODR */
+	return adxl345_set_inact_time(st, 0);
 }
 
 static int adxl345_find_range(struct adxl345_state *st, int val, int val2,
@@ -598,9 +719,49 @@ static int adxl345_find_range(struct adxl345_state *st, int val, int val2,
 
 static int adxl345_set_range(struct adxl345_state *st, enum adxl345_range range)
 {
-	return regmap_update_bits(st->regmap, ADXL345_REG_DATA_FORMAT,
+	unsigned int act_threshold, inact_threshold;
+	unsigned int range_old;
+	unsigned int regval;
+	int ret;
+
+	ret = regmap_read(st->regmap, ADXL345_REG_DATA_FORMAT, &regval);
+	if (ret)
+		return ret;
+	range_old = FIELD_GET(ADXL345_DATA_FORMAT_RANGE, regval);
+
+	ret = regmap_read(st->regmap,
+			  adxl345_act_thresh_reg[ADXL345_ACTIVITY],
+			  &act_threshold);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(st->regmap,
+			  adxl345_act_thresh_reg[ADXL345_INACTIVITY],
+			  &inact_threshold);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(st->regmap, ADXL345_REG_DATA_FORMAT,
 				 ADXL345_DATA_FORMAT_RANGE,
 				 FIELD_PREP(ADXL345_DATA_FORMAT_RANGE, range));
+	if (ret)
+		return ret;
+
+	act_threshold = act_threshold * adxl345_range_factor_tbl[range_old]
+		/ adxl345_range_factor_tbl[range];
+	act_threshold = min(U8_MAX, max(1, act_threshold));
+
+	inact_threshold = inact_threshold * adxl345_range_factor_tbl[range_old]
+		/ adxl345_range_factor_tbl[range];
+	inact_threshold = min(U8_MAX, max(1, inact_threshold));
+
+	ret = regmap_write(st->regmap, adxl345_act_thresh_reg[ADXL345_ACTIVITY],
+			   act_threshold);
+	if (ret)
+		return ret;
+
+	return regmap_write(st->regmap, adxl345_act_thresh_reg[ADXL345_INACTIVITY],
+			   inact_threshold);
 }
 
 static int adxl345_read_avail(struct iio_dev *indio_dev,
@@ -747,6 +908,8 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
 		switch (dir) {
 		case IIO_EV_DIR_RISING:
 			return adxl345_is_act_inact_en(st, ADXL345_ACTIVITY);
+		case IIO_EV_DIR_FALLING:
+			return adxl345_is_act_inact_en(st, ADXL345_INACTIVITY);
 		default:
 			return -EINVAL;
 		}
@@ -787,6 +950,10 @@ static int adxl345_write_event_config(struct iio_dev *indio_dev,
 			return adxl345_set_act_inact_en(st,
 							ADXL345_ACTIVITY,
 							state);
+		case IIO_EV_DIR_FALLING:
+			return adxl345_set_act_inact_en(st,
+							ADXL345_INACTIVITY,
+							state);
 		default:
 			return -EINVAL;
 		}
@@ -812,7 +979,8 @@ static int adxl345_read_event_value(struct iio_dev *indio_dev,
 				    int *val, int *val2)
 {
 	struct adxl345_state *st = iio_priv(indio_dev);
-	unsigned int act_threshold;
+	unsigned int act_threshold, inact_threshold;
+	unsigned int inact_time_s;
 	unsigned int tap_threshold;
 	int ret;
 
@@ -830,9 +998,26 @@ static int adxl345_read_event_value(struct iio_dev *indio_dev,
 				*val = 62500 * act_threshold;
 				*val2 = MICRO;
 				return IIO_VAL_FRACTIONAL;
+			case IIO_EV_DIR_FALLING:
+				ret = regmap_read(st->regmap,
+						  adxl345_act_thresh_reg[ADXL345_INACTIVITY],
+						  &inact_threshold);
+				if (ret)
+					return ret;
+				*val = 62500 * inact_threshold;
+				*val2 = MICRO;
+				return IIO_VAL_FRACTIONAL;
 			default:
 				return -EINVAL;
 			}
+		case IIO_EV_INFO_PERIOD:
+			ret = regmap_read(st->regmap,
+					  ADXL345_REG_TIME_INACT,
+					  &inact_time_s);
+			if (ret)
+				return ret;
+			*val = inact_time_s;
+			return IIO_VAL_INT;
 		default:
 			return -EINVAL;
 		}
@@ -887,19 +1072,31 @@ static int adxl345_write_event_value(struct iio_dev *indio_dev,
 	case IIO_EV_TYPE_MAG:
 		switch (info) {
 		case IIO_EV_INFO_VALUE:
+			val = DIV_ROUND_CLOSEST(val * MICRO + val2, 62500);
 			switch (dir) {
 			case IIO_EV_DIR_RISING:
-				val = DIV_ROUND_CLOSEST(val * MICRO + val2, 62500);
 				ret = regmap_write(st->regmap,
 						   adxl345_act_thresh_reg[ADXL345_ACTIVITY],
 						   val);
 				if (ret)
 					return ret;
 				break;
+			case IIO_EV_DIR_FALLING:
+				ret = regmap_write(st->regmap,
+						   adxl345_act_thresh_reg[ADXL345_INACTIVITY],
+						   val);
+				if (ret)
+					return ret;
+				break;
 			default:
 				return -EINVAL;
 			}
 			break;
+		case IIO_EV_INFO_PERIOD:
+			ret = adxl345_set_inact_time(st, val);
+			if (ret)
+				return ret;
+			break;
 		default:
 			return -EINVAL;
 		}
@@ -1183,6 +1380,17 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
 			return ret;
 	}
 
+	if (FIELD_GET(ADXL345_INT_INACTIVITY, int_stat)) {
+		ret = iio_push_event(indio_dev,
+				     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
+							IIO_MOD_X_AND_Y_AND_Z,
+							IIO_EV_TYPE_MAG,
+							IIO_EV_DIR_FALLING),
+				     ts);
+		if (ret)
+			return ret;
+	}
+
 	if (FIELD_GET(ADXL345_INT_WATERMARK, int_stat)) {
 		samples = adxl345_get_samples(st);
 		if (samples < 0)
@@ -1414,10 +1622,18 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 		if (ret)
 			return ret;
 
+		ret = regmap_write(st->regmap, ADXL345_REG_TIME_INACT, 0x37);
+		if (ret)
+			return ret;
+
 		ret = regmap_write(st->regmap, ADXL345_REG_THRESH_ACT, 6);
 		if (ret)
 			return ret;
 
+		ret = regmap_write(st->regmap, ADXL345_REG_THRESH_INACT, 4);
+		if (ret)
+			return ret;
+
 		ret = regmap_write(st->regmap, ADXL345_REG_THRESH_TAP, tap_threshold);
 		if (ret)
 			return ret;
-- 
2.39.5

[PATCH v9 09/11] iio: accel: adxl345: add coupling detection for activity/inactivity

[Lothar Rubusch]

Add adaptive coupling activity/inactivity detection by setting the AC/DC
bit. This is an additional configuration to the activity and inactivity
magnitude events, respectively. DC-coupled event generation takes the
configured threshold values directly, where AC-coupled event generation
references to the acceleration value at the start of the activity
detection. New samples of acceleration are then compared to this
reference.

Both types are implemented using MAG for DC-coupled activity/inactivity,
but MAG_ADAPTIVE for AC-coupled activity inactivity events. Threshold and
periods can be configured by different sysfs handles, but share the same
registers at the sensor. Therefore activity and inactivity,
respectively, cannot be configured with AC- and DC-coupling at the same
time, e.g. configuring DC-coupled and AC-coupled activity will result in
AC-coupled activity, or generally the most recent one being configured.

This patch implicitly regroups adxl345_read/write_event_value() and
adxl345_read/write_event_config() where now redundant parts, due to
similar event handling for AC-coupling, are moved to a separate function
adxl345_read/write_mag_config() and adxl345_read/write_mag_value(),
respectively.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 1670315ebd63..6a4af559ad9e 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -37,7 +37,9 @@
 #define ADXL345_REG_TAP_SUPPRESS_MSK	BIT(3)
 #define ADXL345_REG_TAP_SUPPRESS	BIT(3)
 #define ADXL345_REG_ACT_AXIS_MSK	GENMASK(6, 4)
+#define ADXL345_REG_ACT_ACDC_MSK	BIT(7)
 #define ADXL345_REG_INACT_AXIS_MSK	GENMASK(2, 0)
+#define ADXL345_REG_INACT_ACDC_MSK	BIT(3)
 #define ADXL345_POWER_CTL_INACT_MSK	(ADXL345_POWER_CTL_AUTO_SLEEP | ADXL345_POWER_CTL_LINK)
 
 #define ADXL345_TAP_Z_EN		BIT(0)
@@ -52,6 +54,9 @@
 #define ADXL345_ACT_Y_EN		BIT(5)
 #define ADXL345_ACT_X_EN		BIT(6)
 
+#define ADXL345_COUPLING_DC		0
+#define ADXL345_COUPLING_AC		1
+
 /* single/double tap */
 enum adxl345_tap_type {
 	ADXL345_SINGLE_TAP,
@@ -79,16 +84,29 @@ static const unsigned int adxl345_tap_time_reg[] = {
 enum adxl345_activity_type {
 	ADXL345_ACTIVITY,
 	ADXL345_INACTIVITY,
+	ADXL345_ACTIVITY_AC,
+	ADXL345_INACTIVITY_AC,
 };
 
 static const unsigned int adxl345_act_int_reg[] = {
 	[ADXL345_ACTIVITY] = ADXL345_INT_ACTIVITY,
 	[ADXL345_INACTIVITY] = ADXL345_INT_INACTIVITY,
+	[ADXL345_ACTIVITY_AC] = ADXL345_INT_ACTIVITY,
+	[ADXL345_INACTIVITY_AC] = ADXL345_INT_INACTIVITY,
 };
 
 static const unsigned int adxl345_act_thresh_reg[] = {
 	[ADXL345_ACTIVITY] = ADXL345_REG_THRESH_ACT,
 	[ADXL345_INACTIVITY] = ADXL345_REG_THRESH_INACT,
+	[ADXL345_ACTIVITY_AC] = ADXL345_REG_THRESH_ACT,
+	[ADXL345_INACTIVITY_AC] = ADXL345_REG_THRESH_INACT,
+};
+
+static const unsigned int adxl345_act_acdc_msk[] = {
+	[ADXL345_ACTIVITY] = ADXL345_REG_ACT_ACDC_MSK,
+	[ADXL345_INACTIVITY] = ADXL345_REG_INACT_ACDC_MSK,
+	[ADXL345_ACTIVITY_AC] = ADXL345_REG_ACT_ACDC_MSK,
+	[ADXL345_INACTIVITY_AC] = ADXL345_REG_INACT_ACDC_MSK,
 };
 
 enum adxl345_odr {
@@ -186,6 +204,13 @@ static struct iio_event_spec adxl345_events[] = {
 		.mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE) |
 			BIT(IIO_EV_INFO_VALUE),
 	},
+	{
+		/* activity, ac bit set */
+		.type = IIO_EV_TYPE_MAG_ADAPTIVE,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE) |
+			BIT(IIO_EV_INFO_VALUE),
+	},
 	{
 		/* single tap */
 		.type = IIO_EV_TYPE_GESTURE,
@@ -239,6 +264,14 @@ static const struct iio_event_spec adxl345_fake_chan_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),
+	},
 };
 
 static const struct iio_chan_spec adxl345_channels[] = {
@@ -345,12 +378,108 @@ static int adxl345_set_inact_time(struct adxl345_state *st, u32 val_s)
 	return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
 }
 
+/**
+ * adxl345_is_act_inact_ac() - Verify if AC or DC coupling is currently enabled.
+ *
+ * @st: The device data.
+ * @type: The activity or inactivity type.
+ *
+ * Given a type of activity / inactivity combined with either AC coupling set or
+ * default to DC, this function verifies if the combination is currently
+ * configured, hence enabled or not.
+ *
+ * Return: true if configured coupling matches the provided type, else a negative
+ *         error value.
+ */
+static int adxl345_is_act_inact_ac(struct adxl345_state *st,
+				   enum adxl345_activity_type type)
+{
+	unsigned int regval;
+	bool coupling;
+	int ret;
+
+	ret = regmap_read(st->regmap, ADXL345_REG_ACT_INACT_CTRL, &regval);
+	if (ret)
+		return ret;
+
+	coupling = adxl345_act_acdc_msk[type] & regval;
+
+	if (type == ADXL345_ACTIVITY || type == ADXL345_INACTIVITY)
+		return coupling == ADXL345_COUPLING_DC;
+	else
+		return coupling == ADXL345_COUPLING_AC;
+
+	return 0;
+}
+
+/**
+ * adxl345_set_act_inact_ac() - Configure AC coupling or DC coupling.
+ *
+ * @st: The device data.
+ * @type: Provide a type of activity or inactivity.
+ *
+ * Enables AC coupling or DC coupling depending on the provided type argument.
+ * Note: Activity and inactivity can be either AC coupled or DC coupled not
+ * both at the same time.
+ *
+ * Return: 0 if successful, else error value.
+ */
+static int adxl345_set_act_inact_ac(struct adxl345_state *st,
+				    enum adxl345_activity_type type)
+{
+	unsigned int coupling;
+
+	if (type == ADXL345_ACTIVITY_AC || type == ADXL345_INACTIVITY_AC)
+		coupling = ADXL345_COUPLING_AC;
+	else
+		coupling = ADXL345_COUPLING_DC;
+
+	/*
+	 * A setting of false selects dc-coupled operation, and a setting of
+	 * true enables ac-coupled operation. In dc-coupled operation, the
+	 * current acceleration magnitude is compared directly with
+	 * ADXL345_REG_THRESH_ACT and ADXL345_REG_THRESH_INACT to determine
+	 * whether activity or inactivity is detected.
+	 *
+	 * In ac-coupled operation for activity detection, the acceleration
+	 * value at the start of activity detection is taken as a reference
+	 * value. New samples of acceleration are then compared to this
+	 * reference value, and if the magnitude of the difference exceeds the
+	 * ADXL345_REG_THRESH_ACT value, the device triggers an activity
+	 * interrupt.
+	 *
+	 * Similarly, in ac-coupled operation for inactivity detection, a
+	 * reference value is used for comparison and is updated whenever the
+	 * device exceeds the inactivity threshold. After the reference value
+	 * is selected, the device compares the magnitude of the difference
+	 * between the reference value and the current acceleration with
+	 * ADXL345_REG_THRESH_INACT. If the difference is less than the value in
+	 * ADXL345_REG_THRESH_INACT for the time in ADXL345_REG_TIME_INACT, the
+	 * device is considered inactive and the inactivity interrupt is
+	 * triggered. [quoted from p. 24, ADXL345 datasheet Rev. G]
+	 *
+	 * In a conclusion, the first acceleration snapshot sample which hit the
+	 * threshold in a particular direction is always taken as acceleration
+	 * reference value to that direction. Since for the hardware activity
+	 * and inactivity depend on the x/y/z axis, so do ac and dc coupling.
+	 * Note, this sw driver always enables or disables all three x/y/z axis
+	 * for detection via act_axis_ctrl and inact_axis_ctrl, respectively.
+	 * Where in dc-coupling samples are compared against the thresholds, in
+	 * ac-coupling measurement difference to the first acceleration
+	 * reference value are compared against the threshold. So, ac-coupling
+	 * allows for a bit more dynamic compensation depending on the initial
+	 * sample.
+	 */
+	return regmap_assign_bits(st->regmap, ADXL345_REG_ACT_INACT_CTRL,
+				  adxl345_act_acdc_msk[type], coupling);
+}
+
 static int adxl345_is_act_inact_en(struct adxl345_state *st,
 				   enum adxl345_activity_type type)
 {
 	unsigned int regval;
 	u32 axis_ctrl;
-	bool en;
+	bool coupling_en, int_en, en;
 	int ret;
 
 	ret = regmap_read(st->regmap, ADXL345_REG_ACT_INACT_CTRL, &axis_ctrl);
@@ -359,11 +488,13 @@ static int adxl345_is_act_inact_en(struct adxl345_state *st,
 
 	switch (type) {
 	case ADXL345_ACTIVITY:
+	case ADXL345_ACTIVITY_AC:
 		en = FIELD_GET(ADXL345_ACT_X_EN, axis_ctrl) |
 			FIELD_GET(ADXL345_ACT_Y_EN, axis_ctrl) |
 			FIELD_GET(ADXL345_ACT_Z_EN, axis_ctrl);
 		break;
 	case ADXL345_INACTIVITY:
+	case ADXL345_INACTIVITY_AC:
 		en = FIELD_GET(ADXL345_INACT_X_EN, axis_ctrl) |
 			FIELD_GET(ADXL345_INACT_Y_EN, axis_ctrl) |
 			FIELD_GET(ADXL345_INACT_Z_EN, axis_ctrl);
@@ -379,8 +510,48 @@ static int adxl345_is_act_inact_en(struct adxl345_state *st,
 	ret = regmap_read(st->regmap, ADXL345_REG_INT_ENABLE, &regval);
 	if (ret)
 		return ret;
+	int_en = adxl345_act_int_reg[type] & regval;
+
+	/* Check if coupling is enabled */
+	ret = adxl345_is_act_inact_ac(st, type);
+	if (ret < 0)
+		return ret;
+	coupling_en = ret;
 
-	return adxl345_act_int_reg[type] & regval;
+	return coupling_en && int_en;
+}
+
+static int adxl345_set_act_inact_linkbit(struct adxl345_state *st,
+					 enum adxl345_activity_type type,
+					 bool en)
+{
+	int act_en, act_ac_en, inact_en, inact_ac_en;
+
+	act_en = adxl345_is_act_inact_en(st, ADXL345_ACTIVITY);
+	if (act_en < 0)
+		return act_en;
+
+	act_ac_en = adxl345_is_act_inact_en(st, ADXL345_ACTIVITY_AC);
+	if (act_ac_en < 0)
+		return act_ac_en;
+
+	act_en = act_en || act_ac_en;
+
+	inact_en = adxl345_is_act_inact_en(st, ADXL345_INACTIVITY);
+	if (inact_en < 0)
+		return inact_en;
+
+	inact_ac_en = adxl345_is_act_inact_en(st, ADXL345_INACTIVITY_AC);
+	if (inact_ac_en < 0)
+		return inact_ac_en;
+
+	inact_en = inact_en || inact_ac_en;
+
+	en = en && act_en && inact_en;
+
+	return regmap_assign_bits(st->regmap, ADXL345_REG_POWER_CTL,
+				  (ADXL345_POWER_CTL_AUTO_SLEEP | ADXL345_POWER_CTL_LINK),
+				  en);
 }
 
 static int adxl345_set_act_inact_en(struct adxl345_state *st,
@@ -391,15 +562,24 @@ static int adxl345_set_act_inact_en(struct adxl345_state *st,
 	unsigned int inact_time_s;
 	unsigned int threshold;
 	u32 axis_ctrl;
-	int act_en, inact_en;
 	int ret;
 
+	/*
+	 * 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 (adxl345_is_act_inact_ac(st, type) <= 0)
+			return 0;
+	}
+
 	/* Start configuring the sensor registers */
 	ret = adxl345_set_measure_en(st, false);
 	if (ret)
 		return ret;
 
-	if (type == ADXL345_ACTIVITY) {
+	if (type == ADXL345_ACTIVITY || type == ADXL345_ACTIVITY_AC) {
 		axis_ctrl = ADXL345_ACT_X_EN | ADXL345_ACT_Y_EN |
 				ADXL345_ACT_Z_EN;
 	} else {
@@ -407,12 +587,16 @@ static int adxl345_set_act_inact_en(struct adxl345_state *st,
 				ADXL345_INACT_Z_EN;
 	}
 
+	ret = regmap_read(st->regmap, adxl345_act_thresh_reg[type], &threshold);
+	if (ret)
+		return ret;
+
 	ret = regmap_assign_bits(st->regmap, ADXL345_REG_ACT_INACT_CTRL,
 				 axis_ctrl, cmd_en);
 	if (ret)
 		return ret;
 
-	ret = regmap_read(st->regmap, adxl345_act_thresh_reg[type], &threshold);
+	ret = adxl345_set_act_inact_ac(st, type);
 	if (ret)
 		return ret;
 
@@ -420,10 +604,12 @@ static int adxl345_set_act_inact_en(struct adxl345_state *st,
 
 	switch (type) {
 	case ADXL345_ACTIVITY:
+	case ADXL345_ACTIVITY_AC:
 		en = FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, axis_ctrl) &&
 			threshold;
 		break;
 	case ADXL345_INACTIVITY:
+	case ADXL345_INACTIVITY_AC:
 		ret = regmap_read(st->regmap, ADXL345_REG_TIME_INACT, &inact_time_s);
 		if (ret)
 			return ret;
@@ -435,25 +621,14 @@ static int adxl345_set_act_inact_en(struct adxl345_state *st,
 		return -EINVAL;
 	}
 
+	/* Enable interrupts accordingly. */
 	ret = regmap_assign_bits(st->regmap, ADXL345_REG_INT_ENABLE,
-				 adxl345_act_int_reg[type], cmd_en);
+				 adxl345_act_int_reg[type], cmd_en && en);
 	if (ret)
 		return ret;
 
 	/* Set sleep and link bit when ACT and INACT are enabled. */
-	act_en = adxl345_is_act_inact_en(st, ADXL345_ACTIVITY);
-	if (act_en < 0)
-		return act_en;
-
-	inact_en = adxl345_is_act_inact_en(st, ADXL345_INACTIVITY);
-	if (inact_en < 0)
-		return inact_en;
-
-	en = en && act_en && inact_en;
-
-	ret = regmap_assign_bits(st->regmap, ADXL345_REG_POWER_CTL,
-				 (ADXL345_POWER_CTL_AUTO_SLEEP | ADXL345_POWER_CTL_LINK),
-				 en);
+	ret = adxl345_set_act_inact_linkbit(st, type, en);
 	if (ret)
 		return ret;
 
@@ -894,6 +1069,40 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
 	return adxl345_set_measure_en(st, true);
 }
 
+static int adxl345_read_mag_config(struct adxl345_state *st,
+				   const struct iio_chan_spec *chan,
+				   enum iio_event_direction dir,
+				   enum adxl345_activity_type type_act,
+				   enum adxl345_activity_type type_inact)
+{
+	switch (dir) {
+	case IIO_EV_DIR_RISING:
+		return adxl345_is_act_inact_en(st, type_act);
+	case IIO_EV_DIR_FALLING:
+		return adxl345_is_act_inact_en(st, type_inact);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int adxl345_write_mag_config(struct adxl345_state *st,
+				    const struct iio_chan_spec *chan,
+				    enum iio_event_direction dir,
+				    enum iio_event_type type,
+				    enum adxl345_activity_type type_act,
+				    enum adxl345_activity_type type_inact,
+				    bool state)
+{
+	switch (dir) {
+	case IIO_EV_DIR_RISING:
+		return adxl345_set_act_inact_en(st, type_act, state);
+	case IIO_EV_DIR_FALLING:
+		return adxl345_set_act_inact_en(st, type_inact, state);
+	default:
+		return -EINVAL;
+	}
+}
+
 static int adxl345_read_event_config(struct iio_dev *indio_dev,
 				     const struct iio_chan_spec *chan,
 				     enum iio_event_type type,
@@ -905,14 +1114,13 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
 
 	switch (type) {
 	case IIO_EV_TYPE_MAG:
-		switch (dir) {
-		case IIO_EV_DIR_RISING:
-			return adxl345_is_act_inact_en(st, ADXL345_ACTIVITY);
-		case IIO_EV_DIR_FALLING:
-			return adxl345_is_act_inact_en(st, ADXL345_INACTIVITY);
-		default:
-			return -EINVAL;
-		}
+		return adxl345_read_mag_config(st, chan, dir,
+					       ADXL345_ACTIVITY,
+					       ADXL345_INACTIVITY);
+	case IIO_EV_TYPE_MAG_ADAPTIVE:
+		return adxl345_read_mag_config(st, chan, dir,
+					       ADXL345_ACTIVITY_AC,
+					       ADXL345_INACTIVITY_AC);
 	case IIO_EV_TYPE_GESTURE:
 		switch (dir) {
 		case IIO_EV_DIR_SINGLETAP:
@@ -945,27 +1153,100 @@ static int adxl345_write_event_config(struct iio_dev *indio_dev,
 
 	switch (type) {
 	case IIO_EV_TYPE_MAG:
+		return adxl345_write_mag_config(st, chan, dir, type,
+						ADXL345_ACTIVITY,
+						ADXL345_INACTIVITY,
+						state);
+	case IIO_EV_TYPE_MAG_ADAPTIVE:
+		return adxl345_write_mag_config(st, chan, dir, type,
+						ADXL345_ACTIVITY_AC,
+						ADXL345_INACTIVITY_AC,
+						state);
+	case IIO_EV_TYPE_GESTURE:
+		switch (dir) {
+		case IIO_EV_DIR_SINGLETAP:
+			return adxl345_set_singletap_en(st, chan->channel2, state);
+		case IIO_EV_DIR_DOUBLETAP:
+			return adxl345_set_doubletap_en(st, state);
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+}
+
+static int adxl345_read_mag_value(struct adxl345_state *st,
+				  enum adxl345_activity_type type_act,
+				  enum adxl345_activity_type type_inact, /* TODO needed */
+				  enum iio_event_direction dir,
+				  enum iio_event_info info,
+				  int *val, int *val2)
+{
+	unsigned int act_threshold, inact_threshold;
+	int ret;
+
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
 		switch (dir) {
 		case IIO_EV_DIR_RISING:
-			return adxl345_set_act_inact_en(st,
-							ADXL345_ACTIVITY,
-							state);
+			ret = regmap_read(st->regmap,
+					  adxl345_act_thresh_reg[type_act],
+					  &act_threshold);
+			if (ret)
+				return ret;
+			*val = 62500 * act_threshold;
+			*val2 = MICRO;
+			return IIO_VAL_FRACTIONAL;
 		case IIO_EV_DIR_FALLING:
-			return adxl345_set_act_inact_en(st,
-							ADXL345_INACTIVITY,
-							state);
+			ret = regmap_read(st->regmap,
+					  adxl345_act_thresh_reg[type_inact],
+					  &inact_threshold);
+			if (ret)
+				return ret;
+			*val = 62500 * inact_threshold;
+			*val2 = MICRO;
+			return IIO_VAL_FRACTIONAL;
 		default:
 			return -EINVAL;
 		}
-	case IIO_EV_TYPE_GESTURE:
+	case IIO_EV_INFO_PERIOD:
+		ret = regmap_read(st->regmap,
+				  ADXL345_REG_TIME_INACT,
+				  val);
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int adxl345_write_mag_value(struct adxl345_state *st,
+				   enum adxl345_activity_type type_act,
+				   enum adxl345_activity_type type_inact,
+				   enum iio_event_direction dir,
+				   enum iio_event_info info,
+				   int val, int val2)
+{
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		/*
+		 * Scaling factor 62.5mg/LSB, i.e. ~16g corresponds to 0xff
+		 */
+		val = DIV_ROUND_CLOSEST(val * MICRO + val2, 62500);
 		switch (dir) {
-		case IIO_EV_DIR_SINGLETAP:
-			return adxl345_set_singletap_en(st, chan->channel2, state);
-		case IIO_EV_DIR_DOUBLETAP:
-			return adxl345_set_doubletap_en(st, state);
+		case IIO_EV_DIR_RISING:
+			return regmap_write(st->regmap,
+					    adxl345_act_thresh_reg[type_act],
+					    val);
+		case IIO_EV_DIR_FALLING:
+			return regmap_write(st->regmap,
+					    adxl345_act_thresh_reg[type_inact],
+					    val);
 		default:
 			return -EINVAL;
 		}
+	case IIO_EV_INFO_PERIOD:
+		return adxl345_set_inact_time(st, val);
 	default:
 		return -EINVAL;
 	}
@@ -979,48 +1260,18 @@ static int adxl345_read_event_value(struct iio_dev *indio_dev,
 				    int *val, int *val2)
 {
 	struct adxl345_state *st = iio_priv(indio_dev);
-	unsigned int act_threshold, inact_threshold;
-	unsigned int inact_time_s;
 	unsigned int tap_threshold;
 	int ret;
 
 	switch (type) {
 	case IIO_EV_TYPE_MAG:
-		switch (info) {
-		case IIO_EV_INFO_VALUE:
-			switch (dir) {
-			case IIO_EV_DIR_RISING:
-				ret = regmap_read(st->regmap,
-						  adxl345_act_thresh_reg[ADXL345_ACTIVITY],
-						  &act_threshold);
-				if (ret)
-					return ret;
-				*val = 62500 * act_threshold;
-				*val2 = MICRO;
-				return IIO_VAL_FRACTIONAL;
-			case IIO_EV_DIR_FALLING:
-				ret = regmap_read(st->regmap,
-						  adxl345_act_thresh_reg[ADXL345_INACTIVITY],
-						  &inact_threshold);
-				if (ret)
-					return ret;
-				*val = 62500 * inact_threshold;
-				*val2 = MICRO;
-				return IIO_VAL_FRACTIONAL;
-			default:
-				return -EINVAL;
-			}
-		case IIO_EV_INFO_PERIOD:
-			ret = regmap_read(st->regmap,
-					  ADXL345_REG_TIME_INACT,
-					  &inact_time_s);
-			if (ret)
-				return ret;
-			*val = inact_time_s;
-			return IIO_VAL_INT;
-		default:
-			return -EINVAL;
-		}
+		return adxl345_read_mag_value(st, ADXL345_ACTIVITY,
+					      ADXL345_INACTIVITY, dir, info,
+					      val, val2);
+	case IIO_EV_TYPE_MAG_ADAPTIVE:
+		return adxl345_read_mag_value(st, ADXL345_ACTIVITY_AC,
+					      ADXL345_INACTIVITY_AC, dir, info,
+					      val, val2);
 	case IIO_EV_TYPE_GESTURE:
 		switch (info) {
 		case IIO_EV_INFO_VALUE:
@@ -1070,36 +1321,20 @@ static int adxl345_write_event_value(struct iio_dev *indio_dev,
 
 	switch (type) {
 	case IIO_EV_TYPE_MAG:
-		switch (info) {
-		case IIO_EV_INFO_VALUE:
-			val = DIV_ROUND_CLOSEST(val * MICRO + val2, 62500);
-			switch (dir) {
-			case IIO_EV_DIR_RISING:
-				ret = regmap_write(st->regmap,
-						   adxl345_act_thresh_reg[ADXL345_ACTIVITY],
-						   val);
-				if (ret)
-					return ret;
-				break;
-			case IIO_EV_DIR_FALLING:
-				ret = regmap_write(st->regmap,
-						   adxl345_act_thresh_reg[ADXL345_INACTIVITY],
-						   val);
-				if (ret)
-					return ret;
-				break;
-			default:
-				return -EINVAL;
-			}
-			break;
-		case IIO_EV_INFO_PERIOD:
-			ret = adxl345_set_inact_time(st, val);
-			if (ret)
-				return ret;
-			break;
-		default:
-			return -EINVAL;
-		}
+		ret = adxl345_write_mag_value(st,
+					      ADXL345_ACTIVITY,
+					      ADXL345_INACTIVITY,
+					      dir, info, val, val2);
+		if (ret)
+			return ret;
+		break;
+	case IIO_EV_TYPE_MAG_ADAPTIVE:
+		ret = adxl345_write_mag_value(st,
+					      ADXL345_ACTIVITY_AC,
+					      ADXL345_INACTIVITY_AC,
+					      dir, info, val, val2);
+		if (ret)
+			return ret;
 		break;
 	case IIO_EV_TYPE_GESTURE:
 		switch (info) {
@@ -1347,6 +1582,7 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
 {
 	s64 ts = iio_get_time_ns(indio_dev);
 	struct adxl345_state *st = iio_priv(indio_dev);
+	unsigned int regval;
 	int samples;
 	int ret = -ENOENT;
 
@@ -1371,22 +1607,52 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
 	}
 
 	if (FIELD_GET(ADXL345_INT_ACTIVITY, int_stat)) {
-		ret = iio_push_event(indio_dev,
-				     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0, act_dir,
-							IIO_EV_TYPE_MAG,
-							IIO_EV_DIR_RISING),
-				     ts);
+		ret = regmap_read(st->regmap, ADXL345_REG_ACT_INACT_CTRL, &regval);
+		if (ret)
+			return ret;
+
+		if (FIELD_GET(ADXL345_REG_ACT_ACDC_MSK, regval)) {
+			/* AC coupled */
+			ret = iio_push_event(indio_dev,
+					     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0, act_dir,
+								IIO_EV_TYPE_MAG_ADAPTIVE,
+								IIO_EV_DIR_RISING),
+					     ts);
+
+		} else {
+			/* DC coupled, relying on THRESH */
+			ret = iio_push_event(indio_dev,
+					     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0, act_dir,
+								IIO_EV_TYPE_MAG,
+								IIO_EV_DIR_RISING),
+					     ts);
+		}
 		if (ret)
 			return ret;
 	}
 
 	if (FIELD_GET(ADXL345_INT_INACTIVITY, int_stat)) {
-		ret = iio_push_event(indio_dev,
-				     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
-							IIO_MOD_X_AND_Y_AND_Z,
-							IIO_EV_TYPE_MAG,
-							IIO_EV_DIR_FALLING),
-				     ts);
+		ret = regmap_read(st->regmap, ADXL345_REG_ACT_INACT_CTRL, &regval);
+		if (ret)
+			return ret;
+
+		if (FIELD_GET(ADXL345_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);
+		} 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;
 	}
-- 
2.39.5

[PATCH v9 10/11] iio: accel: adxl345: extend inactivity time for less than 1s

[Lothar Rubusch]

Inactivity event and free-fall event are actually the same type of sensor
events. Hence, inactivity detection using a period of 1 to 255s can be
extended to be used for lower periods than 1s for covering free-fall
detection.

When lower periods are defined, the driver automatically will setup
threshold and period on the free-fall register, while using the
inactivity threshold and period for periods above 1s. Using the
free-fall register, no link bit will be set, and therefore no auto-sleep
can be set in cases where also activity will be enabled.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 6a4af559ad9e..7afc120b4a45 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -86,6 +86,7 @@ enum adxl345_activity_type {
 	ADXL345_INACTIVITY,
 	ADXL345_ACTIVITY_AC,
 	ADXL345_INACTIVITY_AC,
+	ADXL345_INACTIVITY_FF,
 };
 
 static const unsigned int adxl345_act_int_reg[] = {
@@ -93,6 +94,7 @@ static const unsigned int adxl345_act_int_reg[] = {
 	[ADXL345_INACTIVITY] = ADXL345_INT_INACTIVITY,
 	[ADXL345_ACTIVITY_AC] = ADXL345_INT_ACTIVITY,
 	[ADXL345_INACTIVITY_AC] = ADXL345_INT_INACTIVITY,
+	[ADXL345_INACTIVITY_FF] = ADXL345_INT_FREE_FALL,
 };
 
 static const unsigned int adxl345_act_thresh_reg[] = {
@@ -100,6 +102,7 @@ static const unsigned int adxl345_act_thresh_reg[] = {
 	[ADXL345_INACTIVITY] = ADXL345_REG_THRESH_INACT,
 	[ADXL345_ACTIVITY_AC] = ADXL345_REG_THRESH_ACT,
 	[ADXL345_INACTIVITY_AC] = ADXL345_REG_THRESH_INACT,
+	[ADXL345_INACTIVITY_FF] = ADXL345_REG_THRESH_FF,
 };
 
 static const unsigned int adxl345_act_acdc_msk[] = {
@@ -107,6 +110,7 @@ static const unsigned int adxl345_act_acdc_msk[] = {
 	[ADXL345_INACTIVITY] = ADXL345_REG_INACT_ACDC_MSK,
 	[ADXL345_ACTIVITY_AC] = ADXL345_REG_ACT_ACDC_MSK,
 	[ADXL345_INACTIVITY_AC] = ADXL345_REG_INACT_ACDC_MSK,
+	[ADXL345_INACTIVITY_FF] = 0x00,
 };
 
 enum adxl345_odr {
@@ -189,6 +193,9 @@ struct adxl345_state {
 	u8 watermark;
 	u8 fifo_mode;
 
+	u8 inact_threshold;
+	u32 inact_time_ms;
+
 	u32 tap_duration_us;
 	u32 tap_latent_us;
 	u32 tap_window_us;
@@ -333,12 +340,31 @@ static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
 
 /* act/inact */
 
+static int adxl345_set_inact_threshold(struct adxl345_state *st,
+				       unsigned int threshold)
+{
+	int ret;
+
+	st->inact_threshold = min(U8_MAX, threshold);
+
+	ret = regmap_write(st->regmap,
+			   adxl345_act_thresh_reg[ADXL345_INACTIVITY],
+			   st->inact_threshold);
+	if (ret)
+		return ret;
+
+	return regmap_write(st->regmap,
+			    adxl345_act_thresh_reg[ADXL345_INACTIVITY_FF],
+			    st->inact_threshold);
+}
+
 /**
  * adxl345_set_inact_time - Configure inactivity time explicitly or by ODR.
  * @st: The sensor state instance.
- * @val_s: A desired time value, between 0 and 255.
+ * @val_int: The inactivity time, integer part.
+ * @val_fract: The inactivity time, fractional part when val_int is 0.
  *
- * Inactivity time can be configured between 1 and 255 sec. If a val_s of 0
+ * Inactivity time can be configured between 1 and 255 sec. If a val_s of 0.00
  * is configured by a user, then a default inactivity time will be computed.
  *
  * In such case, it should take power consumption into consideration. Thus it
@@ -355,16 +381,18 @@ static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
  *
  * Return: 0 or error value.
  */
-static int adxl345_set_inact_time(struct adxl345_state *st, u32 val_s)
+static int adxl345_set_inact_time(struct adxl345_state *st, u32 val_int,
+				  u32 val_fract)
 {
 	unsigned int max_boundary = 255;
 	unsigned int min_boundary = 10;
-	unsigned int val = min(val_s, max_boundary);
+	unsigned int val;
 	enum adxl345_odr odr;
 	unsigned int regval;
 	int ret;
 
-	if (val == 0) {
+	if (val_int == 0 && val_fract == 0) {
+		/* Generated inactivity time based on ODR */
 		ret = regmap_read(st->regmap, ADXL345_REG_BW_RATE, &regval);
 		if (ret)
 			return ret;
@@ -373,9 +401,41 @@ static int adxl345_set_inact_time(struct adxl345_state *st, u32 val_s)
 
 		val = (adxl345_odr_tbl[odr][0] > max_boundary)
 			? min_boundary : max_boundary -	adxl345_odr_tbl[odr][0];
+
+		st->inact_time_ms = MILLI * val;
+
+		/* Inactivity time in s */
+		ret = regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
+		if (ret)
+			return ret;
+
+	} else if (val_int == 0 && val_fract > 0) {
+		/* time < 1s, free-fall */
+
+		/*
+		 * Datasheet max. value is 255 * 5000 us = 1.275000 seconds.
+		 *
+		 * Recommended values between 100ms and 350ms (0x14 to 0x46)
+		 */
+		st->inact_time_ms = DIV_ROUND_UP(val_fract, MILLI);
+
+		ret = regmap_write(st->regmap, ADXL345_REG_TIME_FF,
+				   DIV_ROUND_CLOSEST(val_fract, 5));
+		if (ret)
+			return ret;
+	} else if (val_int > 0) {
+		/* Time >= 1s, inactivity */
+		st->inact_time_ms = MILLI * val_int;
+
+		ret = regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val_int);
+		if (ret)
+			return ret;
+	} else {
+		/* Do not support negative or wrong input. */
+		return -EINVAL;
 	}
 
-	return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
+	return 0;
 }
 
 /**
@@ -398,6 +458,9 @@ static int adxl345_is_act_inact_ac(struct adxl345_state *st,
 	bool coupling;
 	int ret;
 
+	if (type == ADXL345_INACTIVITY_FF)
+		return 1;
+
 	ret = regmap_read(st->regmap, ADXL345_REG_ACT_INACT_CTRL, &regval);
 	if (ret)
 		return ret;
@@ -499,6 +562,9 @@ static int adxl345_is_act_inact_en(struct adxl345_state *st,
 			FIELD_GET(ADXL345_INACT_Y_EN, axis_ctrl) |
 			FIELD_GET(ADXL345_INACT_Z_EN, axis_ctrl);
 		break;
+	case ADXL345_INACTIVITY_FF:
+		en = true;
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -537,15 +603,19 @@ static int adxl345_set_act_inact_linkbit(struct adxl345_state *st,
 
 	act_en = act_en || act_ac_en;
 
-	inact_en = adxl345_is_act_inact_en(st, ADXL345_INACTIVITY);
-	if (inact_en < 0)
-		return inact_en;
+	if (type == ADXL345_INACTIVITY_FF) {
+		inact_en = false;
+	} else {
+		inact_en = adxl345_is_act_inact_en(st, ADXL345_INACTIVITY);
+		if (inact_en < 0)
+			return inact_en;
 
-	inact_ac_en = adxl345_is_act_inact_en(st, ADXL345_INACTIVITY_AC);
-	if (inact_ac_en < 0)
-		return inact_ac_en;
+		inact_ac_en = adxl345_is_act_inact_en(st, ADXL345_INACTIVITY_AC);
+		if (inact_ac_en < 0)
+			return inact_ac_en;
 
-	inact_en = inact_en || inact_ac_en;
+		inact_en = inact_en || inact_ac_en;
+	}
 
 	en = en && act_en && inact_en;
 
@@ -559,7 +629,6 @@ static int adxl345_set_act_inact_en(struct adxl345_state *st,
 				    bool cmd_en)
 {
 	bool en;
-	unsigned int inact_time_s;
 	unsigned int threshold;
 	u32 axis_ctrl;
 	int ret;
@@ -582,14 +651,16 @@ static int adxl345_set_act_inact_en(struct adxl345_state *st,
 	if (type == ADXL345_ACTIVITY || type == ADXL345_ACTIVITY_AC) {
 		axis_ctrl = ADXL345_ACT_X_EN | ADXL345_ACT_Y_EN |
 				ADXL345_ACT_Z_EN;
+
+		ret = regmap_read(st->regmap, adxl345_act_thresh_reg[type], &threshold);
+		if (ret)
+			return ret;
 	} else {
 		axis_ctrl = ADXL345_INACT_X_EN | ADXL345_INACT_Y_EN |
 				ADXL345_INACT_Z_EN;
-	}
 
-	ret = regmap_read(st->regmap, adxl345_act_thresh_reg[type], &threshold);
-	if (ret)
-		return ret;
+		threshold = st->inact_threshold;
+	}
 
 	ret = regmap_assign_bits(st->regmap, ADXL345_REG_ACT_INACT_CTRL,
 				 axis_ctrl, cmd_en);
@@ -610,12 +681,9 @@ static int adxl345_set_act_inact_en(struct adxl345_state *st,
 		break;
 	case ADXL345_INACTIVITY:
 	case ADXL345_INACTIVITY_AC:
-		ret = regmap_read(st->regmap, ADXL345_REG_TIME_INACT, &inact_time_s);
-		if (ret)
-			return ret;
-
+	case ADXL345_INACTIVITY_FF:
 		en = FIELD_GET(ADXL345_REG_INACT_AXIS_MSK, axis_ctrl) &&
-			threshold && inact_time_s;
+			threshold && st->inact_time_ms;
 		break;
 	default:
 		return -EINVAL;
@@ -873,7 +941,7 @@ static int adxl345_set_odr(struct adxl345_state *st, enum adxl345_odr odr)
 		return ret;
 
 	/* update inactivity time by ODR */
-	return adxl345_set_inact_time(st, 0);
+	return adxl345_set_inact_time(st, 0, 0);
 }
 
 static int adxl345_find_range(struct adxl345_state *st, int val, int val2,
@@ -910,12 +978,6 @@ static int adxl345_set_range(struct adxl345_state *st, enum adxl345_range range)
 	if (ret)
 		return ret;
 
-	ret = regmap_read(st->regmap,
-			  adxl345_act_thresh_reg[ADXL345_INACTIVITY],
-			  &inact_threshold);
-	if (ret)
-		return ret;
-
 	ret = regmap_update_bits(st->regmap, ADXL345_REG_DATA_FORMAT,
 				 ADXL345_DATA_FORMAT_RANGE,
 				 FIELD_PREP(ADXL345_DATA_FORMAT_RANGE, range));
@@ -926,6 +988,7 @@ static int adxl345_set_range(struct adxl345_state *st, enum adxl345_range range)
 		/ adxl345_range_factor_tbl[range];
 	act_threshold = min(U8_MAX, max(1, act_threshold));
 
+	inact_threshold = st->inact_threshold;
 	inact_threshold = inact_threshold * adxl345_range_factor_tbl[range_old]
 		/ adxl345_range_factor_tbl[range];
 	inact_threshold = min(U8_MAX, max(1, inact_threshold));
@@ -935,8 +998,7 @@ static int adxl345_set_range(struct adxl345_state *st, enum adxl345_range range)
 	if (ret)
 		return ret;
 
-	return regmap_write(st->regmap, adxl345_act_thresh_reg[ADXL345_INACTIVITY],
-			   inact_threshold);
+	return adxl345_set_inact_threshold(st, inact_threshold);
 }
 
 static int adxl345_read_avail(struct iio_dev *indio_dev,
@@ -1183,7 +1245,7 @@ static int adxl345_read_mag_value(struct adxl345_state *st,
 				  enum iio_event_info info,
 				  int *val, int *val2)
 {
-	unsigned int act_threshold, inact_threshold;
+	unsigned int act_threshold;
 	int ret;
 
 	switch (info) {
@@ -1199,22 +1261,16 @@ static int adxl345_read_mag_value(struct adxl345_state *st,
 			*val2 = MICRO;
 			return IIO_VAL_FRACTIONAL;
 		case IIO_EV_DIR_FALLING:
-			ret = regmap_read(st->regmap,
-					  adxl345_act_thresh_reg[type_inact],
-					  &inact_threshold);
-			if (ret)
-				return ret;
-			*val = 62500 * inact_threshold;
+			*val = 62500 * st->inact_threshold;
 			*val2 = MICRO;
 			return IIO_VAL_FRACTIONAL;
 		default:
 			return -EINVAL;
 		}
 	case IIO_EV_INFO_PERIOD:
-		ret = regmap_read(st->regmap,
-				  ADXL345_REG_TIME_INACT,
-				  val);
-		return IIO_VAL_INT;
+		*val = st->inact_time_ms;
+		*val2 = MILLI;
+		return IIO_VAL_FRACTIONAL;
 	default:
 		return -EINVAL;
 	}
@@ -1239,14 +1295,12 @@ static int adxl345_write_mag_value(struct adxl345_state *st,
 					    adxl345_act_thresh_reg[type_act],
 					    val);
 		case IIO_EV_DIR_FALLING:
-			return regmap_write(st->regmap,
-					    adxl345_act_thresh_reg[type_inact],
-					    val);
+			return adxl345_set_inact_threshold(st, val);
 		default:
 			return -EINVAL;
 		}
 	case IIO_EV_INFO_PERIOD:
-		return adxl345_set_inact_time(st, val);
+		return adxl345_set_inact_time(st, val, val2);
 	default:
 		return -EINVAL;
 	}
@@ -1657,6 +1711,17 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
 			return ret;
 	}
 
+	if (FIELD_GET(ADXL345_INT_FREE_FALL, int_stat)) {
+		ret = iio_push_event(indio_dev,
+				     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
+							IIO_MOD_X_AND_Y_AND_Z,
+							IIO_EV_TYPE_MAG,
+							IIO_EV_DIR_FALLING),
+				     ts);
+		if (ret)
+			return ret;
+	}
+
 	if (FIELD_GET(ADXL345_INT_WATERMARK, int_stat)) {
 		samples = adxl345_get_samples(st);
 		if (samples < 0)
@@ -1888,15 +1953,11 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 		if (ret)
 			return ret;
 
-		ret = regmap_write(st->regmap, ADXL345_REG_TIME_INACT, 0x37);
-		if (ret)
-			return ret;
-
 		ret = regmap_write(st->regmap, ADXL345_REG_THRESH_ACT, 6);
 		if (ret)
 			return ret;
 
-		ret = regmap_write(st->regmap, ADXL345_REG_THRESH_INACT, 4);
+		ret = adxl345_set_inact_threshold(st, 4);
 		if (ret)
 			return ret;
 
-- 
2.39.5

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

[Lothar Rubusch]

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

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

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

Re: [PATCH v9 04/11] iio: accel: adxl345: simplify interrupt mapping

[Andy Shevchenko]

On Tue, Jun 10, 2025 at 09:59:26PM +0000, Lothar Rubusch wrote:
> Replace mapping all sensor interrupts to the corresponding interrupt
> line using regmap_assign_bits() since it takes a boolean directly.
> Further prefer the units.h identifier to cover the full register when bits
> are set.

...

> -		regval = intio ? 0xff : 0;
> -
> -		ret = regmap_write(st->regmap, ADXL345_REG_INT_MAP, regval);
> +		ret = regmap_assign_bits(st->regmap, ADXL345_REG_INT_MAP,
> +					 U8_MAX, intio);

I'm wondering if GENMASK() is actually better to point out to the amount and
exact bits in the bitfield? After all this is HW register we program, right?

>  		if (ret)
>  			return ret;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v9 07/11] iio: accel: adxl345: add activity event feature

[Andy Shevchenko]

On Tue, Jun 10, 2025 at 09:59:29PM +0000, Lothar Rubusch wrote:
> Make the sensor detect and issue interrupts at activity. Activity
> events are configured by a threshold. Initialize the activity threshold
> register to a reasonable default value in probe. The value is taken from
> the older ADXL345 input driver, to provide a similar behavior.
> 
> Activity, ODR configuration together with the range setting prepare the
> activity/inactivity hysteresis setup, implemented in a follow up patch.
> Thus parts of this patch prepare switch/case setups for the follow up
> patches.

...

> +/* act/inact */

Useless comment. If you want it to stay, decrypt it and make it useful.

...

> +static int adxl345_is_act_inact_en(struct adxl345_state *st,
> +				   enum adxl345_activity_type type)
> +{
> +	unsigned int regval;
> +	u32 axis_ctrl;
> +	bool en;
> +	int ret;
> +
> +	ret = regmap_read(st->regmap, ADXL345_REG_ACT_INACT_CTRL, &axis_ctrl);
> +	if (ret)
> +		return ret;
> +
> +	switch (type) {
> +	case ADXL345_ACTIVITY:
> +		en = FIELD_GET(ADXL345_ACT_X_EN, axis_ctrl) |
> +			FIELD_GET(ADXL345_ACT_Y_EN, axis_ctrl) |
> +			FIELD_GET(ADXL345_ACT_Z_EN, axis_ctrl);

Something happened to the indentation.
Ditto for several places in the code (upper and lower from this).

> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (!en)
> +		return en;
> +
> +	/* Check if corresponding interrupts are enabled */
> +	ret = regmap_read(st->regmap, ADXL345_REG_INT_ENABLE, &regval);
> +	if (ret)
> +		return ret;
> +
> +	return adxl345_act_int_reg[type] & regval;
> +}

...

> +	if (type == ADXL345_ACTIVITY) {
> +		axis_ctrl = ADXL345_ACT_X_EN | ADXL345_ACT_Y_EN |
> +				ADXL345_ACT_Z_EN;
> +	} else {
> +		axis_ctrl = 0x00;
> +	}

Besides an indentation issue, {} are redundant.

...

> +	en = false;

This line makes no sense. When it will, it should be there, not in this change.

> +	switch (type) {
> +	case ADXL345_ACTIVITY:
> +		en = FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, axis_ctrl) &&
> +			threshold;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

...

>  	switch (type) {
> +	case IIO_EV_TYPE_MAG:
> +		switch (info) {
> +		case IIO_EV_INFO_VALUE:
> +			switch (dir) {
> +			case IIO_EV_DIR_RISING:
> +				ret = regmap_read(st->regmap,
> +						  adxl345_act_thresh_reg[ADXL345_ACTIVITY],
> +						  &act_threshold);
> +				if (ret)
> +					return ret;
> +				*val = 62500 * act_threshold;
> +				*val2 = MICRO;
> +				return IIO_VAL_FRACTIONAL;
> +			default:
> +				return -EINVAL;
> +			}
> +		default:
> +			return -EINVAL;
> +		}

As I mentioned before, try to avoid nested switch cases like this. Use helpers
to make this gone to 1 level or so.

>  	case IIO_EV_TYPE_GESTURE:
>  		switch (info) {
>  		case IIO_EV_INFO_VALUE:

Ditto for other similar cases.

...

>  static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
> -			      enum iio_modifier tap_dir)
> +			      enum iio_modifier tap_dir,
> +			      enum iio_modifier act_dir)

If the order of parameters is not so important, I would squeeze new one to be
before the last argument.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v9 08/11] iio: accel: adxl345: add inactivity feature

[Andy Shevchenko]

On Tue, Jun 10, 2025 at 09:59:30PM +0000, Lothar Rubusch wrote:
> Add the inactivity feature of the sensor to the driver. When activity
> and inactivity are enabled, a link bit will be set linking activity and
> inactivity handling. Additionally, the auto-sleep mode will be enabled.
> Due to the link bit the sensor is going to auto-sleep when inactivity
> was detected.
> 
> Inactivity detection needs a threshold to be configured and a period of
> time in seconds. After, it will transition to inactivity state, if
> measurements stay below inactivity threshold.
> 
> When a ODR is configured the period for inactivity is adjusted with a
> corresponding reasonable default value, in order to have higher
> frequencies, lower inactivity times, and lower sample frequency but
> give more time until inactivity. Both with reasonable upper and lower
> boundaries, since many of the sensor's features (e.g. auto-sleep) will
> need to operate between 12.5 Hz and 400 Hz. This is a default setting
> when actively changing sample frequency, explicitly setting the time
> until inactivity will overwrite the default.
> 
> Similarly, setting the g-range will provide a default value for the
> activity and inactivity thresholds. Both are implicit defaults, but
> equally can be overwritten to be explicitly configured.

...

> +static const struct iio_event_spec adxl345_fake_chan_events[] = {
> +	{
> +		/* inactivity */
> +		.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),

Slightly better

		.mask_shared_by_type =
			BIT(IIO_EV_INFO_VALUE) |
			BIT(IIO_EV_INFO_PERIOD),

> +	},
> +};

And the same for other similar cases.

...

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

> +		val = (adxl345_odr_tbl[odr][0] > max_boundary)
> +			? min_boundary : max_boundary -	adxl345_odr_tbl[odr][0];

clamp() ?

> +	}
> +
> +	return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
> +}

...

>  	if (type == ADXL345_ACTIVITY) {
>  		axis_ctrl = ADXL345_ACT_X_EN | ADXL345_ACT_Y_EN |
>  				ADXL345_ACT_Z_EN;
>  	} else {
> -		axis_ctrl = 0x00;
> +		axis_ctrl = ADXL345_INACT_X_EN | ADXL345_INACT_Y_EN |
> +				ADXL345_INACT_Z_EN;
>  	}

Now this can be as simple as

	axis_ctrl = ADXL345_ACT_X_EN;
	if (type == ADXL345_ACTIVITY)
		axis_ctrl |= ADXL345_ACT_Y_EN | ADXL345_ACT_Z_EN;
	else
		axis_ctrl |= ADXL345_INACT_Y_EN | ADXL345_INACT_Z_EN;

Yeah, I don't know how to make the diff better (it gets worse), but the end
result is better.

One way, which I don't like much is to previously have this conditional written as:

	axis_ctrl = ADXL345_ACT_X_EN;
	if (type == ADXL345_ACTIVITY)
		axis_ctrl |= ADXL345_ACT_Y_EN | ADXL345_ACT_Z_EN;
	else
		axis_ctrl = 0;

...

> +	ret = regmap_assign_bits(st->regmap, ADXL345_REG_POWER_CTL,
> +				 (ADXL345_POWER_CTL_AUTO_SLEEP | ADXL345_POWER_CTL_LINK),

Unneeded parentheses.

> +				 en);
>  	if (ret)
>  		return ret;

...

>  static int adxl345_set_odr(struct adxl345_state *st, enum adxl345_odr odr)
>  {
> -	return regmap_update_bits(st->regmap, ADXL345_REG_BW_RATE,
> +	int ret;
> +
> +	ret = regmap_update_bits(st->regmap, ADXL345_REG_BW_RATE,
>  				 ADXL345_BW_RATE_MSK,
>  				 FIELD_PREP(ADXL345_BW_RATE_MSK, odr));
> +	if (ret)
> +		return ret;
> +
> +	/* update inactivity time by ODR */
> +	return adxl345_set_inact_time(st, 0);

Okay, in this case the initial form of

	int ret;

	ret = ...
	if (ret)
		return ret;

	return 0;


will be better with the respectful comment (as Jonathan suggested) in that
change that this is not optimal as standalone change, but it will help reduce
churn in the next change(s).

>  }

...

>  static int adxl345_set_range(struct adxl345_state *st, enum adxl345_range range)
>  {
> -	return regmap_update_bits(st->regmap, ADXL345_REG_DATA_FORMAT,

Same here.

> +	unsigned int act_threshold, inact_threshold;
> +	unsigned int range_old;
> +	unsigned int regval;
> +	int ret;
> +
> +	ret = regmap_read(st->regmap, ADXL345_REG_DATA_FORMAT, &regval);
> +	if (ret)
> +		return ret;
> +	range_old = FIELD_GET(ADXL345_DATA_FORMAT_RANGE, regval);
> +
> +	ret = regmap_read(st->regmap,
> +			  adxl345_act_thresh_reg[ADXL345_ACTIVITY],
> +			  &act_threshold);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(st->regmap,
> +			  adxl345_act_thresh_reg[ADXL345_INACTIVITY],
> +			  &inact_threshold);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(st->regmap, ADXL345_REG_DATA_FORMAT,
>  				 ADXL345_DATA_FORMAT_RANGE,
>  				 FIELD_PREP(ADXL345_DATA_FORMAT_RANGE, range));
> +	if (ret)
> +		return ret;
> +
> +	act_threshold = act_threshold * adxl345_range_factor_tbl[range_old]
> +		/ adxl345_range_factor_tbl[range];
> +	act_threshold = min(U8_MAX, max(1, act_threshold));
> +
> +	inact_threshold = inact_threshold * adxl345_range_factor_tbl[range_old]
> +		/ adxl345_range_factor_tbl[range];
> +	inact_threshold = min(U8_MAX, max(1, inact_threshold));
> +
> +	ret = regmap_write(st->regmap, adxl345_act_thresh_reg[ADXL345_ACTIVITY],
> +			   act_threshold);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(st->regmap, adxl345_act_thresh_reg[ADXL345_INACTIVITY],
> +			   inact_threshold);
>  }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v9 01/11] iio: accel: adxl345: apply scale factor to tap threshold

[Jonathan Cameron]

On Tue, 10 Jun 2025 21:59:23 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> The threshold for tap detection was still not scaled. The datasheet sets
> a scale factor of 62.5mg/LSB. Remove commit about not scaled threshold
> for tap detection, and apply scaling to it.
> 

Given tap detection algorithms are not generally well defined and not a simple
threshold (generally) what scaling should we be aiming for here?
Even if it were a simple threshold, when a channel provides _raw the
expectation is that event config is vs _raw, not the base units.

So if this doesn't care about the current fullscale range (which the
comment implied was the case) it would need to rescale when the
IIO_INFO_SCALE changes.

That comment is I think indicating we decided to gloss over the
detail because it's going into a (potentially) non trivial algorithm anyway.

Jonathan


> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl345_core.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 7c093c0241de..d80efb68d113 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -697,17 +697,15 @@ static int adxl345_read_event_value(struct iio_dev *indio_dev,
>  		switch (info) {
>  		case IIO_EV_INFO_VALUE:
>  			/*
> -			 * The scale factor would be 62.5mg/LSB (i.e. 0xFF = 16g) but
> -			 * not applied here. In context of this general purpose sensor,
> -			 * what imports is rather signal intensity than the absolute
> -			 * measured g value.
> +			 * Scale factor is 62.5mg/LSB i.e. 0xff = 16g
>  			 */
>  			ret = regmap_read(st->regmap, ADXL345_REG_THRESH_TAP,
>  					  &tap_threshold);
>  			if (ret)
>  				return ret;
> -			*val = sign_extend32(tap_threshold, 7);
> -			return IIO_VAL_INT;
> +			*val = 62500 * sign_extend32(tap_threshold, 7);
> +			*val2 = MICRO;
> +			return IIO_VAL_FRACTIONAL;
>  		case IIO_EV_INFO_TIMEOUT:
>  			*val = st->tap_duration_us;
>  			*val2 = 1000000;
> @@ -746,6 +744,7 @@ static int adxl345_write_event_value(struct iio_dev *indio_dev,
>  	case IIO_EV_TYPE_GESTURE:
>  		switch (info) {
>  		case IIO_EV_INFO_VALUE:
> +			val = DIV_ROUND_CLOSEST(val * MICRO + val2, 62500);
>  			ret = regmap_write(st->regmap, ADXL345_REG_THRESH_TAP,
>  					   min(val, 0xFF));
>  			if (ret)

Re: [PATCH v9 02/11] iio: accel: adxl345: make data struct variable irq function local

[Jonathan Cameron]

On Tue, 10 Jun 2025 21:59:24 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Remove variable irq from the struct state and make it a function local
> variable, because it is not necessary to be kept struct-wise.
> 
> This is a refactoring change and should not impact functionality.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
Applied as this one stands on its own as a good little cleanup.

Thanks,

Jonathan

Re: [PATCH v9 03/11] iio: accel: adxl345: simplify measure enable

[Jonathan Cameron]

On Tue, 10 Jun 2025 21:59:25 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Simplify the function to enable or disable measurement. Replace the
> separate decision logic and call to regmap_update_bits() by a single
> call to regmap_assign_bits() taking a boolean argument directly.
> 
> This is a refactoring change and should not impact functionality.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
Another good stand along change.

Applied.

> ---
>  drivers/iio/accel/adxl345.h      | 1 -
>  drivers/iio/accel/adxl345_core.c | 5 ++---
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> index 6c1f96406136..9385affdefe3 100644
> --- a/drivers/iio/accel/adxl345.h
> +++ b/drivers/iio/accel/adxl345.h
> @@ -73,7 +73,6 @@
>  #define ADXL345_BW_LOW_POWER		BIT(4)
>  #define ADXL345_BASE_RATE_NANO_HZ	97656250LL
>  
> -#define ADXL345_POWER_CTL_STANDBY	0x00
>  #define ADXL345_POWER_CTL_WAKEUP	GENMASK(1, 0)
>  #define ADXL345_POWER_CTL_SLEEP	BIT(2)
>  #define ADXL345_POWER_CTL_MEASURE	BIT(3)
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 07abab82d093..cae9e37e216f 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -233,9 +233,8 @@ EXPORT_SYMBOL_NS_GPL(adxl345_is_volatile_reg, "IIO_ADXL345");
>   */
>  static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
>  {
> -	unsigned int val = en ? ADXL345_POWER_CTL_MEASURE : ADXL345_POWER_CTL_STANDBY;
> -
> -	return regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
> +	return regmap_assign_bits(st->regmap, ADXL345_REG_POWER_CTL,
> +				  ADXL345_POWER_CTL_MEASURE, en);
>  }
>  
>  /* tap */

Re: [PATCH v9 05/11] iio: accel: adxl345: simplify reading the FIFO

[Jonathan Cameron]

On Tue, 10 Jun 2025 21:59:27 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Bulk reading from the FIFO can be simplified. Remove unnecessary
> variables and simplify reading sets of x-, y- and z-axis measurements.
> 
> This is a refactoring change and should not impact functionality.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl345_core.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 18c625d323ba..dcfbfe4cac0f 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -884,15 +884,13 @@ static int adxl345_get_samples(struct adxl345_state *st)
>   */
>  static int adxl345_fifo_transfer(struct adxl345_state *st, int samples)
>  {
> -	size_t count;
>  	int i, ret = 0;
>  
> -	/* count is the 3x the fifo_buf element size, hence 6B */
> -	count = sizeof(st->fifo_buf[0]) * ADXL345_DIRS;
>  	for (i = 0; i < samples; i++) {
>  		/* read 3x 2 byte elements from base address into next fifo_buf position */
>  		ret = regmap_bulk_read(st->regmap, ADXL345_REG_XYZ_BASE,
> -				       st->fifo_buf + (i * count / 2), count);
> +				       st->fifo_buf + (i * ADXL345_DIRS),
> +				       2 * ADXL345_DIRS);
				       sizeof(st->fifo_buf[0]) * ADXL345_DIRS);

As it's not locally obvious where that 2 comes from.

>  		if (ret)
>  			return ret;
>  

Re: [PATCH v9 06/11] iio: accel: adxl345: replace magic numbers by unit expressions

[Jonathan Cameron]

On Tue, 10 Jun 2025 21:59:28 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Replace absolute numbers by their expressions from units.h to avoid
> using magic numbers. Use uniform expressions to clarify their usage.
> 
> This is a refactoring change and should not impact functionality.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
Nice. Applied with a little fuzz due to the earlier patches that I haven't
picked up yet.

Thanks,

J
> ---
>  drivers/iio/accel/adxl345_core.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index dcfbfe4cac0f..2c4f045c741c 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -706,15 +706,15 @@ static int adxl345_read_event_value(struct iio_dev *indio_dev,
>  			return IIO_VAL_FRACTIONAL;
>  		case IIO_EV_INFO_TIMEOUT:
>  			*val = st->tap_duration_us;
> -			*val2 = 1000000;
> +			*val2 = MICRO;
>  			return IIO_VAL_FRACTIONAL;
>  		case IIO_EV_INFO_RESET_TIMEOUT:
>  			*val = st->tap_window_us;
> -			*val2 = 1000000;
> +			*val2 = MICRO;
>  			return IIO_VAL_FRACTIONAL;
>  		case IIO_EV_INFO_TAP2_MIN_DELAY:
>  			*val = st->tap_latent_us;
> -			*val2 = 1000000;
> +			*val2 = MICRO;
>  			return IIO_VAL_FRACTIONAL;
>  		default:
>  			return -EINVAL;

Re: [PATCH v9 08/11] iio: accel: adxl345: add inactivity feature

[Jonathan Cameron]

> ...
> 
> >  	if (type == ADXL345_ACTIVITY) {
> >  		axis_ctrl = ADXL345_ACT_X_EN | ADXL345_ACT_Y_EN |
> >  				ADXL345_ACT_Z_EN;
> >  	} else {
> > -		axis_ctrl = 0x00;
> > +		axis_ctrl = ADXL345_INACT_X_EN | ADXL345_INACT_Y_EN |
> > +				ADXL345_INACT_Z_EN;
> >  	}  
> 
> Now this can be as simple as
> 
> 	axis_ctrl = ADXL345_ACT_X_EN;

That flag is only set in the activity case.  Confused with ADXL345_INACT_X_EN?
(initially I thought you'd run into a bug!)

> 	if (type == ADXL345_ACTIVITY)
> 		axis_ctrl |= ADXL345_ACT_Y_EN | ADXL345_ACT_Z_EN;
> 	else
> 		axis_ctrl |= ADXL345_INACT_Y_EN | ADXL345_INACT_Z_EN;
> 
> Yeah, I don't know how to make the diff better (it gets worse), but the end
> result is better.
> 
> One way, which I don't like much is to previously have this conditional written as:
> 
> 	axis_ctrl = ADXL345_ACT_X_EN;
> 	if (type == ADXL345_ACTIVITY)
> 		axis_ctrl |= ADXL345_ACT_Y_EN | ADXL345_ACT_Z_EN;
> 	else
> 		axis_ctrl = 0;
> 
> ...
> 
> > +	ret = regmap_assign_bits(st->regmap, ADXL345_REG_POWER_CTL,
> > +				 (ADXL345_POWER_CTL_AUTO_SLEEP | ADXL345_POWER_CTL_LINK),  
> 
> Unneeded parentheses.
> 
> > +				 en);
> >  	if (ret)
> >  		return ret;  
> 
> ...
> 
> >  static int adxl345_set_odr(struct adxl345_state *st, enum adxl345_odr odr)
> >  {
> > -	return regmap_update_bits(st->regmap, ADXL345_REG_BW_RATE,
> > +	int ret;
> > +
> > +	ret = regmap_update_bits(st->regmap, ADXL345_REG_BW_RATE,
> >  				 ADXL345_BW_RATE_MSK,
> >  				 FIELD_PREP(ADXL345_BW_RATE_MSK, odr));
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* update inactivity time by ODR */
> > +	return adxl345_set_inact_time(st, 0);  
> 
> Okay, in this case the initial form of
> 
> 	int ret;
> 
> 	ret = ...
> 	if (ret)
> 		return ret;
> 
> 	return 0;
> 
> 
> will be better with the respectful comment (as Jonathan suggested) in that
> change that this is not optimal as standalone change, but it will help reduce
> churn in the next change(s).
> 
Worth noting we (reviewers) may well forget about this and moan on
some future version. If we do, feel free to remind us!

Re: [PATCH v9 08/11] iio: accel: adxl345: add inactivity feature

[Jonathan Cameron]

On Tue, 10 Jun 2025 21:59:30 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

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

J
> ---
>  drivers/iio/accel/adxl345_core.c | 238 +++++++++++++++++++++++++++++--
>  1 file changed, 227 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 04b9f155872f..1670315ebd63 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c

> @@ -334,12 +423,37 @@ static int adxl345_set_act_inact_en(struct adxl345_state *st,
>  		en = FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, axis_ctrl) &&
>  			threshold;
>  		break;
> +	case ADXL345_INACTIVITY:
> +		ret = regmap_read(st->regmap, ADXL345_REG_TIME_INACT, &inact_time_s);
> +		if (ret)
> +			return ret;
> +
> +		en = FIELD_GET(ADXL345_REG_INACT_AXIS_MSK, axis_ctrl) &&
> +			threshold && inact_time_s;
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
>  
>  	ret = regmap_assign_bits(st->regmap, ADXL345_REG_INT_ENABLE,
> -				 adxl345_act_int_reg[type], cmd_en && en);
> +				 adxl345_act_int_reg[type], cmd_en);
> +	if (ret)
> +		return ret;
> +
> +	/* Set sleep and link bit when ACT and INACT are enabled. */
> +	act_en = adxl345_is_act_inact_en(st, ADXL345_ACTIVITY);
> +	if (act_en < 0)
> +		return act_en;
> +
> +	inact_en = adxl345_is_act_inact_en(st, ADXL345_INACTIVITY);
> +	if (inact_en < 0)
> +		return inact_en;
> +
> +	en = en && act_en && inact_en;
> +

For consistency with above, I'd drop this blank line.

> +	ret = regmap_assign_bits(st->regmap, ADXL345_REG_POWER_CTL,
> +				 (ADXL345_POWER_CTL_AUTO_SLEEP | ADXL345_POWER_CTL_LINK),

Not sure that inner set of brackets are adding anything much to readability.

> +				 en);
>  	if (ret)
>  		return ret;
>  


> @@ -887,19 +1072,31 @@ static int adxl345_write_event_value(struct iio_dev *indio_dev,
>  	case IIO_EV_TYPE_MAG:
>  		switch (info) {
>  		case IIO_EV_INFO_VALUE:
> +			val = DIV_ROUND_CLOSEST(val * MICRO + val2, 62500);

Put it here in the earlier patch.  Though that might not makes sense when
you use some more helpers as Andy suggested.  No one really likes 3 levels
of switch!

>  			switch (dir) {
>  			case IIO_EV_DIR_RISING:
> -				val = DIV_ROUND_CLOSEST(val * MICRO + val2, 62500);
>  				ret = regmap_write(st->regmap,
>  						   adxl345_act_thresh_reg[ADXL345_ACTIVITY],
>  						   val);
>  				if (ret)
>  					return ret;
>  				break;
> +			case IIO_EV_DIR_FALLING:
> +				ret = regmap_write(st->regmap,
> +						   adxl345_act_thresh_reg[ADXL345_INACTIVITY],
> +						   val);
> +				if (ret)
> +					return ret;
> +				break;

Re: [PATCH v9 09/11] iio: accel: adxl345: add coupling detection for activity/inactivity

[Jonathan Cameron]

On Tue, 10 Jun 2025 21:59:31 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add adaptive coupling activity/inactivity detection by setting the AC/DC
> bit. This is an additional configuration to the activity and inactivity
> magnitude events, respectively. DC-coupled event generation takes the
> configured threshold values directly, where AC-coupled event generation
> references to the acceleration value at the start of the activity
> detection. New samples of acceleration are then compared to this
> reference.
> 
> Both types are implemented using MAG for DC-coupled activity/inactivity,
> but MAG_ADAPTIVE for AC-coupled activity inactivity events. Threshold and
> periods can be configured by different sysfs handles, but share the same
> registers at the sensor. Therefore activity and inactivity,
> respectively, cannot be configured with AC- and DC-coupling at the same
> time, e.g. configuring DC-coupled and AC-coupled activity will result in
> AC-coupled activity, or generally the most recent one being configured.
> 
> This patch implicitly regroups adxl345_read/write_event_value() and
> adxl345_read/write_event_config() where now redundant parts, due to
> similar event handling for AC-coupling, are moved to a separate function
> adxl345_read/write_mag_config() and adxl345_read/write_mag_value(),
> respectively.

Can we introduce these helpers before this patch? It has become rather
hard to read with the code movement and new stuff combined.

Thanks

Jonathan

Re: [PATCH v9 10/11] iio: accel: adxl345: extend inactivity time for less than 1s

[Jonathan Cameron]

On Tue, 10 Jun 2025 21:59:32 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Inactivity event and free-fall event are actually the same type of sensor
> events. Hence, inactivity detection using a period of 1 to 255s can be
> extended to be used for lower periods than 1s for covering free-fall
> detection.
> 
> When lower periods are defined, the driver automatically will setup
> threshold and period on the free-fall register, while using the
> inactivity threshold and period for periods above 1s. Using the
> free-fall register, no link bit will be set, and therefore no auto-sleep
> can be set in cases where also activity will be enabled.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>

Approach seems reasonable.  One comment on the code below.

>  /**
>   * adxl345_set_inact_time - Configure inactivity time explicitly or by ODR.
>   * @st: The sensor state instance.
> - * @val_s: A desired time value, between 0 and 255.
> + * @val_int: The inactivity time, integer part.
> + * @val_fract: The inactivity time, fractional part when val_int is 0.
>   *
> - * Inactivity time can be configured between 1 and 255 sec. If a val_s of 0
> + * Inactivity time can be configured between 1 and 255 sec. If a val_s of 0.00
>   * is configured by a user, then a default inactivity time will be computed.
>   *
>   * In such case, it should take power consumption into consideration. Thus it
> @@ -355,16 +381,18 @@ static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
>   *
>   * Return: 0 or error value.
>   */
> -static int adxl345_set_inact_time(struct adxl345_state *st, u32 val_s)
> +static int adxl345_set_inact_time(struct adxl345_state *st, u32 val_int,
> +				  u32 val_fract)
>  {
>  	unsigned int max_boundary = 255;
>  	unsigned int min_boundary = 10;
> -	unsigned int val = min(val_s, max_boundary);
> +	unsigned int val;
>  	enum adxl345_odr odr;
>  	unsigned int regval;
>  	int ret;
>  
> -	if (val == 0) {
> +	if (val_int == 0 && val_fract == 0) {
> +		/* Generated inactivity time based on ODR */
>  		ret = regmap_read(st->regmap, ADXL345_REG_BW_RATE, &regval);
>  		if (ret)
>  			return ret;
> @@ -373,9 +401,41 @@ static int adxl345_set_inact_time(struct adxl345_state *st, u32 val_s)
>  
>  		val = (adxl345_odr_tbl[odr][0] > max_boundary)
>  			? min_boundary : max_boundary -	adxl345_odr_tbl[odr][0];
> +
> +		st->inact_time_ms = MILLI * val;
> +
> +		/* Inactivity time in s */
> +		ret = regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
> +		if (ret)
> +			return ret;
> +

Perhaps return in the good path here.

> +	} else if (val_int == 0 && val_fract > 0) {
> +		/* time < 1s, free-fall */
> +
> +		/*
> +		 * Datasheet max. value is 255 * 5000 us = 1.275000 seconds.
> +		 *
> +		 * Recommended values between 100ms and 350ms (0x14 to 0x46)
> +		 */
> +		st->inact_time_ms = DIV_ROUND_UP(val_fract, MILLI);
> +
> +		ret = regmap_write(st->regmap, ADXL345_REG_TIME_FF,
> +				   DIV_ROUND_CLOSEST(val_fract, 5));
> +		if (ret)
> +			return ret;
and here

> +	} else if (val_int > 0) {
> +		/* Time >= 1s, inactivity */
> +		st->inact_time_ms = MILLI * val_int;
> +
> +		ret = regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val_int);
> +		if (ret)
> +			return ret;
and here.

> +	} else {
Or given this covers any case where val_int > 0 do it first
and error out early?

> +		/* Do not support negative or wrong input. */
> +		return -EINVAL;
>  	}
>  
> -	return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
> +	return 0;
>  }

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

[Jonathan Cameron]

On Tue, 10 Jun 2025 21:59:33 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

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

Trivial but wrap commit descriptions at 75 chars.

The main comment on this is that, when talking about datasheet terms / settings
etc it would be good to reflect them back to the IIO controls that actually allow
us to change them.

Otherwise seems reasonable to me.

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


> +
> +Sensor Events
> +-------------
> +
> +Particular IIO events will be triggered by the corresponding interrupts. The
> +sensor driver supports no or one active INT line, where the sensor has two
> +possible INT IOs. Configure the used INT line in the devicetree. If no INT line
> +is configured, the sensor falls back to FIFO bypass mode and no events are
> +possible, only X, Y and Z axis measurements are possible.
> +
> +The following table shows the ADXL345 related device files, found in the
> +specific device folder path ``/sys/bus/iio/devices/iio:deviceX/events``.
> +Note, the default activity/inactivity is DC coupled. Thus only AC coupled
> +activity and inactivity are mentioned explicitly.

This paragraph probably wants to talk about the mapping of AC coupled to 'adaptive'
I couldn't relate it directly to the table without that.

> +
> ++---------------------------------------------+---------------------------------------------+
> +| Event handle                                | Description                                 |
> ++---------------------------------------------+---------------------------------------------+
> +| in_accel_gesture_doubletap_en               | Enable double tap detection on all axis     |
> ++---------------------------------------------+---------------------------------------------+
> +| in_accel_gesture_doubletap_reset_timeout    | Double tap window in [us]                   |
> ++---------------------------------------------+---------------------------------------------+
> +| in_accel_gesture_doubletap_tap2_min_delay   | Double tap latent in [us]                   |
> ++---------------------------------------------+---------------------------------------------+
> +| in_accel_gesture_singletap_timeout          | Single tap duration in [us]                 |
> ++---------------------------------------------+---------------------------------------------+
> +| in_accel_gesture_singletap_value            | Single tap threshold value in 62.5/LSB      |
> ++---------------------------------------------+---------------------------------------------+
> +| in_accel_mag_falling_period                 | Inactivity time in seconds                  |
> ++---------------------------------------------+---------------------------------------------+
> +| in_accel_mag_falling_value                  | Inactivity threshold value in 62.5/LSB      |
> ++---------------------------------------------+---------------------------------------------+
> +| in_accel_mag_adaptive_rising_en             | Enable AC coupled activity on X axis        |
> ++---------------------------------------------+---------------------------------------------+
> +| in_accel_mag_adaptive_falling_period        | AC coupled inactivity time in seconds       |
> ++---------------------------------------------+---------------------------------------------+
> +| in_accel_mag_adaptive_falling_value         | AC coupled inactivity threshold in 62.5/LSB |
> ++---------------------------------------------+---------------------------------------------+
> +| in_accel_mag_adaptive_rising_value          | AC coupled activity threshold in 62.5/LSB   |
> ++---------------------------------------------+---------------------------------------------+
> +| in_accel_mag_rising_en                      | Enable activity detection on X axis         |
> ++---------------------------------------------+---------------------------------------------+
> +| in_accel_mag_rising_value                   | Activity threshold value in 62.5/LSB        |
> ++---------------------------------------------+---------------------------------------------+
> +| in_accel_x_gesture_singletap_en             | Enable single tap detection on X axis       |
> ++---------------------------------------------+---------------------------------------------+
> +| in_accel_x&y&z_mag_falling_en               | Enable inactivity detection on all axis     |
> ++---------------------------------------------+---------------------------------------------+
> +| in_accel_x&y&z_mag_adaptive_falling_en      | Enable AC coupled inactivity on all axis    |
> ++---------------------------------------------+---------------------------------------------+
> +| in_accel_y_gesture_singletap_en             | Enable single tap detection on Y axis       |
> ++---------------------------------------------+---------------------------------------------+
> +| in_accel_z_gesture_singletap_en             | Enable single tap detection on Z axis       |
> ++---------------------------------------------+---------------------------------------------+
> +
> +Find a detailed description of a particular functionality in the sensor
> +datasheet.
> +
> +Setting the **ODR** explicitly will result in estimated adjusted default values

Say how to set ODR etc in IIO terms as well perhaps?

> +for the inactivity time detection, where higher frequencies shall default to
> +longer wait periods, and vice versa. It is also possible to explicitly
> +configure inactivity wait times, if the defaulting approach does not match
> +application requirements. Setting 0 here, will fall back to default setting.

I'm not particularly keen on that 0 aspect as it's unintuitive.  Why do we need
a means to go back to the default? 

> +
> +The **g range** configuration also tries to estimate activity and inactivity
> +thresholds when switching to another g range. The default range will be
> +factorized by the relation of old range divided by new range. The value never
> +becomes 0 and will be at least 1 and at most 255 i.e. 62.5g/LSB according to
> +the datasheet. Nevertheless activity and inactivity thresholds can be
> +overwritten by explicit values.

Re: [PATCH v9 08/11] iio: accel: adxl345: add inactivity feature

[Andy Shevchenko]

On Sat, Jun 14, 2025 at 4:55 PM Jonathan Cameron <jic23@kernel.org> wrote:

...

> > >     if (type == ADXL345_ACTIVITY) {
> > >             axis_ctrl = ADXL345_ACT_X_EN | ADXL345_ACT_Y_EN |
> > >                             ADXL345_ACT_Z_EN;
> > >     } else {
> > > -           axis_ctrl = 0x00;
> > > +           axis_ctrl = ADXL345_INACT_X_EN | ADXL345_INACT_Y_EN |
> > > +                           ADXL345_INACT_Z_EN;
> > >     }
> >
> > Now this can be as simple as
> >
> >       axis_ctrl = ADXL345_ACT_X_EN;
>
> That flag is only set in the activity case.  Confused with ADXL345_INACT_X_EN?
> (initially I thought you'd run into a bug!)

Ouch, you are right! Please, discard my above suggestion, it's indeed
simply wrong.

> >       if (type == ADXL345_ACTIVITY)
> >               axis_ctrl |= ADXL345_ACT_Y_EN | ADXL345_ACT_Z_EN;
> >       else
> >               axis_ctrl |= ADXL345_INACT_Y_EN | ADXL345_INACT_Z_EN;
> >
> > Yeah, I don't know how to make the diff better (it gets worse), but the end
> > result is better.
> >
> > One way, which I don't like much is to previously have this conditional written as:
> >
> >       axis_ctrl = ADXL345_ACT_X_EN;
> >       if (type == ADXL345_ACTIVITY)
> >               axis_ctrl |= ADXL345_ACT_Y_EN | ADXL345_ACT_Z_EN;
> >       else
> >               axis_ctrl = 0;
> >


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v9 05/11] iio: accel: adxl345: simplify reading the FIFO

[Lothar Rubusch]

Hi Jonathan,

On Sat, Jun 14, 2025 at 3:47 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 10 Jun 2025 21:59:27 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Bulk reading from the FIFO can be simplified. Remove unnecessary
> > variables and simplify reading sets of x-, y- and z-axis measurements.
> >
> > This is a refactoring change and should not impact functionality.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > ---
> >  drivers/iio/accel/adxl345_core.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > index 18c625d323ba..dcfbfe4cac0f 100644
> > --- a/drivers/iio/accel/adxl345_core.c
> > +++ b/drivers/iio/accel/adxl345_core.c
> > @@ -884,15 +884,13 @@ static int adxl345_get_samples(struct adxl345_state *st)
> >   */
> >  static int adxl345_fifo_transfer(struct adxl345_state *st, int samples)
> >  {
> > -     size_t count;
> >       int i, ret = 0;
> >
> > -     /* count is the 3x the fifo_buf element size, hence 6B */
> > -     count = sizeof(st->fifo_buf[0]) * ADXL345_DIRS;
> >       for (i = 0; i < samples; i++) {
> >               /* read 3x 2 byte elements from base address into next fifo_buf position */
> >               ret = regmap_bulk_read(st->regmap, ADXL345_REG_XYZ_BASE,
> > -                                    st->fifo_buf + (i * count / 2), count);
> > +                                    st->fifo_buf + (i * ADXL345_DIRS),
> > +                                    2 * ADXL345_DIRS);
>                                        sizeof(st->fifo_buf[0]) * ADXL345_DIRS);
>
> As it's not locally obvious where that 2 comes from.

Hm, I left the comment some lines above: "read 3x 2 byte elements from
base address into next fifo_buf position" - so, the 2 comes from the 2
bytes. 3x because of the 3-axis sensor, so 3 axis times 2 bytes. How
can I improve it? The patch is just a simplification, removing that
unnecessary count variable.

>
> >               if (ret)
> >                       return ret;
> >
>

Re: [PATCH v9 01/11] iio: accel: adxl345: apply scale factor to tap threshold

[Lothar Rubusch]

On Sat, Jun 14, 2025 at 3:42 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 10 Jun 2025 21:59:23 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > The threshold for tap detection was still not scaled. The datasheet sets
> > a scale factor of 62.5mg/LSB. Remove commit about not scaled threshold
> > for tap detection, and apply scaling to it.
> >
>
> Given tap detection algorithms are not generally well defined and not a simple
> threshold (generally) what scaling should we be aiming for here?
> Even if it were a simple threshold, when a channel provides _raw the
> expectation is that event config is vs _raw, not the base units.
>
> So if this doesn't care about the current fullscale range (which the
> comment implied was the case) it would need to rescale when the
> IIO_INFO_SCALE changes.
>
> That comment is I think indicating we decided to gloss over the
> detail because it's going into a (potentially) non trivial algorithm anyway.
>
> Jonathan
>

Well, the tap threshold so far was around in "raw" LSB bits. At that
time I only left the comment that the value is not scaled. The current
patch is just putting now the scale factor and the sysfs handle then
will take values of 'g' and not just raw bits. This is like for the
other scaled values such as periods.

I think at the time I left the thresholds a bit out, because for me
it's clear what a time is. But I'm not sure, if actually the
thresholds are going so much by the unit values. So, in particular
what is missing here? Is it just about the commit message, or does it
need technical further adjustments?

Best,
L
>
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > ---
> >  drivers/iio/accel/adxl345_core.c | 11 +++++------
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > index 7c093c0241de..d80efb68d113 100644
> > --- a/drivers/iio/accel/adxl345_core.c
> > +++ b/drivers/iio/accel/adxl345_core.c
> > @@ -697,17 +697,15 @@ static int adxl345_read_event_value(struct iio_dev *indio_dev,
> >               switch (info) {
> >               case IIO_EV_INFO_VALUE:
> >                       /*
> > -                      * The scale factor would be 62.5mg/LSB (i.e. 0xFF = 16g) but
> > -                      * not applied here. In context of this general purpose sensor,
> > -                      * what imports is rather signal intensity than the absolute
> > -                      * measured g value.
> > +                      * Scale factor is 62.5mg/LSB i.e. 0xff = 16g
> >                        */
> >                       ret = regmap_read(st->regmap, ADXL345_REG_THRESH_TAP,
> >                                         &tap_threshold);
> >                       if (ret)
> >                               return ret;
> > -                     *val = sign_extend32(tap_threshold, 7);
> > -                     return IIO_VAL_INT;
> > +                     *val = 62500 * sign_extend32(tap_threshold, 7);
> > +                     *val2 = MICRO;
> > +                     return IIO_VAL_FRACTIONAL;
> >               case IIO_EV_INFO_TIMEOUT:
> >                       *val = st->tap_duration_us;
> >                       *val2 = 1000000;
> > @@ -746,6 +744,7 @@ static int adxl345_write_event_value(struct iio_dev *indio_dev,
> >       case IIO_EV_TYPE_GESTURE:
> >               switch (info) {
> >               case IIO_EV_INFO_VALUE:
> > +                     val = DIV_ROUND_CLOSEST(val * MICRO + val2, 62500);
> >                       ret = regmap_write(st->regmap, ADXL345_REG_THRESH_TAP,
> >                                          min(val, 0xFF));
> >                       if (ret)
>

Re: [PATCH v9 01/11] iio: accel: adxl345: apply scale factor to tap threshold

[Jonathan Cameron]

On Mon, 16 Jun 2025 00:20:49 +0200
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> On Sat, Jun 14, 2025 at 3:42 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Tue, 10 Jun 2025 21:59:23 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >  
> > > The threshold for tap detection was still not scaled. The datasheet sets
> > > a scale factor of 62.5mg/LSB. Remove commit about not scaled threshold
> > > for tap detection, and apply scaling to it.
> > >  
> >
> > Given tap detection algorithms are not generally well defined and not a simple
> > threshold (generally) what scaling should we be aiming for here?
> > Even if it were a simple threshold, when a channel provides _raw the
> > expectation is that event config is vs _raw, not the base units.
> >
> > So if this doesn't care about the current fullscale range (which the
> > comment implied was the case) it would need to rescale when the
> > IIO_INFO_SCALE changes.
> >
> > That comment is I think indicating we decided to gloss over the
> > detail because it's going into a (potentially) non trivial algorithm anyway.
> >
> > Jonathan
> >  
> 
> Well, the tap threshold so far was around in "raw" LSB bits. At that
> time I only left the comment that the value is not scaled. The current
> patch is just putting now the scale factor and the sysfs handle then
> will take values of 'g' and not just raw bits. This is like for the
> other scaled values such as periods.

Tricky corner because tap isn't a simple threshold - it it were I'd have
a cleaner argument.

If we were doing this it would need to be scalling to m/s^2 not g but
that's not important for this discussion.

Huh. For thresholds I thought we had this clear in the ABI docs, but we don't.
The ABI doc refers to having _raw_ in the name which I'm not sure has been true 
in a very long time.  The convention is intended to be if the channel
has _raw the thresholds are in that unit (i.e. ADC counts) and if not
they are in the processed value units.

It has to be this way because of non linear sensors.  We have cases
where there isn't a transform we can sensibly convert in the kernel
to set a 'raw' threshold.   (involves cube roots for instance).
As a side note, those sensors are one of the few cases where we have
both _RAW and _PROCESSED because the thresholds have to relate to _RAW
but we need _PROCESSED to give standard units.
 
Now for this case where it's kind of tangentially connected by the
particular algorithm to the raw reading things are non obvious.
The tap detector could just as easily be a threshold on jerk -
rate of change of acceleration or some 'score' calculated from
a bunch of inputs in which case we couldn't apply a scaling.

> 
> I think at the time I left the thresholds a bit out, because for me
> it's clear what a time is. But I'm not sure, if actually the
> thresholds are going so much by the unit values. So, in particular
> what is missing here? Is it just about the commit message, or does it
> need technical further adjustments?

I don't think the patch is needed. For this particular parameter there
isn't a clear concept of scale (putting aside that for this particular
sensor there is one).  Thus it's a twiddle control. No need to connect
it to real world units at all.  Also change this is an ABI change
so we should do it only if we are considering the change to be fixing
a bug.

Jonathan

> 
> Best,
> L
> >  
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > ---
> > >  drivers/iio/accel/adxl345_core.c | 11 +++++------
> > >  1 file changed, 5 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > > index 7c093c0241de..d80efb68d113 100644
> > > --- a/drivers/iio/accel/adxl345_core.c
> > > +++ b/drivers/iio/accel/adxl345_core.c
> > > @@ -697,17 +697,15 @@ static int adxl345_read_event_value(struct iio_dev *indio_dev,
> > >               switch (info) {
> > >               case IIO_EV_INFO_VALUE:
> > >                       /*
> > > -                      * The scale factor would be 62.5mg/LSB (i.e. 0xFF = 16g) but
> > > -                      * not applied here. In context of this general purpose sensor,
> > > -                      * what imports is rather signal intensity than the absolute
> > > -                      * measured g value.
> > > +                      * Scale factor is 62.5mg/LSB i.e. 0xff = 16g
> > >                        */
> > >                       ret = regmap_read(st->regmap, ADXL345_REG_THRESH_TAP,
> > >                                         &tap_threshold);
> > >                       if (ret)
> > >                               return ret;
> > > -                     *val = sign_extend32(tap_threshold, 7);
> > > -                     return IIO_VAL_INT;
> > > +                     *val = 62500 * sign_extend32(tap_threshold, 7);
> > > +                     *val2 = MICRO;
> > > +                     return IIO_VAL_FRACTIONAL;
> > >               case IIO_EV_INFO_TIMEOUT:
> > >                       *val = st->tap_duration_us;
> > >                       *val2 = 1000000;
> > > @@ -746,6 +744,7 @@ static int adxl345_write_event_value(struct iio_dev *indio_dev,
> > >       case IIO_EV_TYPE_GESTURE:
> > >               switch (info) {
> > >               case IIO_EV_INFO_VALUE:
> > > +                     val = DIV_ROUND_CLOSEST(val * MICRO + val2, 62500);
> > >                       ret = regmap_write(st->regmap, ADXL345_REG_THRESH_TAP,
> > >                                          min(val, 0xFF));
> > >                       if (ret)  
> >  

Re: [PATCH v9 05/11] iio: accel: adxl345: simplify reading the FIFO

[Jonathan Cameron]

On Mon, 16 Jun 2025 00:14:47 +0200
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Hi Jonathan,
> 
> On Sat, Jun 14, 2025 at 3:47 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Tue, 10 Jun 2025 21:59:27 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >  
> > > Bulk reading from the FIFO can be simplified. Remove unnecessary
> > > variables and simplify reading sets of x-, y- and z-axis measurements.
> > >
> > > This is a refactoring change and should not impact functionality.
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > ---
> > >  drivers/iio/accel/adxl345_core.c | 6 ++----
> > >  1 file changed, 2 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > > index 18c625d323ba..dcfbfe4cac0f 100644
> > > --- a/drivers/iio/accel/adxl345_core.c
> > > +++ b/drivers/iio/accel/adxl345_core.c
> > > @@ -884,15 +884,13 @@ static int adxl345_get_samples(struct adxl345_state *st)
> > >   */
> > >  static int adxl345_fifo_transfer(struct adxl345_state *st, int samples)
> > >  {
> > > -     size_t count;
> > >       int i, ret = 0;
> > >
> > > -     /* count is the 3x the fifo_buf element size, hence 6B */
> > > -     count = sizeof(st->fifo_buf[0]) * ADXL345_DIRS;
> > >       for (i = 0; i < samples; i++) {
> > >               /* read 3x 2 byte elements from base address into next fifo_buf position */
> > >               ret = regmap_bulk_read(st->regmap, ADXL345_REG_XYZ_BASE,
> > > -                                    st->fifo_buf + (i * count / 2), count);
> > > +                                    st->fifo_buf + (i * ADXL345_DIRS),
> > > +                                    2 * ADXL345_DIRS);  
> >                                        sizeof(st->fifo_buf[0]) * ADXL345_DIRS);
> >
> > As it's not locally obvious where that 2 comes from.  
> 
> Hm, I left the comment some lines above: "read 3x 2 byte elements from
> base address into next fifo_buf position" - so, the 2 comes from the 2
> bytes. 3x because of the 3-axis sensor, so 3 axis times 2 bytes. How
> can I improve it? The patch is just a simplification, removing that
> unnecessary count variable.

 sizeof(st->fifo_buf[0]) * ADXL345_DIRS);

improves it by making it clear where the 2 comes from.
Getting rid of count is good, but that remaining 2 * 
should be replaced by using sizeof() the buffer element.

The comment would then be unnecessary as we are sizing it directly from
the fifo storage.  Fine to leave it if you like though.

Jonathan


> 
> >  
> > >               if (ret)
> > >                       return ret;
> > >  
> >  

Re: [PATCH v9 07/11] iio: accel: adxl345: add activity event feature

[Lothar Rubusch]

Hi Andy,

[...]
> ...
>
> > +static int adxl345_is_act_inact_en(struct adxl345_state *st,
> > +                                enum adxl345_activity_type type)
> > +{
> > +     unsigned int regval;
> > +     u32 axis_ctrl;
> > +     bool en;
> > +     int ret;
> > +
> > +     ret = regmap_read(st->regmap, ADXL345_REG_ACT_INACT_CTRL, &axis_ctrl);
> > +     if (ret)
> > +             return ret;
> > +
> > +     switch (type) {
> > +     case ADXL345_ACTIVITY:
> > +             en = FIELD_GET(ADXL345_ACT_X_EN, axis_ctrl) |
> > +                     FIELD_GET(ADXL345_ACT_Y_EN, axis_ctrl) |
> > +                     FIELD_GET(ADXL345_ACT_Z_EN, axis_ctrl);
>
> Something happened to the indentation.
> Ditto for several places in the code (upper and lower from this).
>

What is the matter with the indention here? I'm doing `checkpatch.pl
--strict --codespell` on that and don't get an issue? Would you expect
cases like the FIELD_GET() calls on the next line, linked by a binary
OR, to be indented by yet another TAB?

Best,
L

> > +             break;
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +
> > +     if (!en)
> > +             return en;
> > +
> > +     /* Check if corresponding interrupts are enabled */
> > +     ret = regmap_read(st->regmap, ADXL345_REG_INT_ENABLE, &regval);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return adxl345_act_int_reg[type] & regval;
> > +}
>
> ...
>
> > +     if (type == ADXL345_ACTIVITY) {
> > +             axis_ctrl = ADXL345_ACT_X_EN | ADXL345_ACT_Y_EN |
> > +                             ADXL345_ACT_Z_EN;
> > +     } else {
> > +             axis_ctrl = 0x00;
> > +     }
>
> Besides an indentation issue, {} are redundant.
>
> ...
>
> > +     en = false;
>
> This line makes no sense. When it will, it should be there, not in this change.
>
> > +     switch (type) {
> > +     case ADXL345_ACTIVITY:
> > +             en = FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, axis_ctrl) &&
> > +                     threshold;
> > +             break;
> > +     default:
> > +             return -EINVAL;
> > +     }
>
> ...
>
> >       switch (type) {
> > +     case IIO_EV_TYPE_MAG:
> > +             switch (info) {
> > +             case IIO_EV_INFO_VALUE:
> > +                     switch (dir) {
> > +                     case IIO_EV_DIR_RISING:
> > +                             ret = regmap_read(st->regmap,
> > +                                               adxl345_act_thresh_reg[ADXL345_ACTIVITY],
> > +                                               &act_threshold);
> > +                             if (ret)
> > +                                     return ret;
> > +                             *val = 62500 * act_threshold;
> > +                             *val2 = MICRO;
> > +                             return IIO_VAL_FRACTIONAL;
> > +                     default:
> > +                             return -EINVAL;
> > +                     }
> > +             default:
> > +                     return -EINVAL;
> > +             }
>
> As I mentioned before, try to avoid nested switch cases like this. Use helpers
> to make this gone to 1 level or so.
>
> >       case IIO_EV_TYPE_GESTURE:
> >               switch (info) {
> >               case IIO_EV_INFO_VALUE:
>
> Ditto for other similar cases.
>
> ...
>
> >  static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
> > -                           enum iio_modifier tap_dir)
> > +                           enum iio_modifier tap_dir,
> > +                           enum iio_modifier act_dir)
>
> If the order of parameters is not so important, I would squeeze new one to be
> before the last argument.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v9 01/11] iio: accel: adxl345: apply scale factor to tap threshold

[Lothar Rubusch]

Hi Jonathan,

On Sat, Jun 21, 2025 at 6:55 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon, 16 Jun 2025 00:20:49 +0200
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > On Sat, Jun 14, 2025 at 3:42 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > On Tue, 10 Jun 2025 21:59:23 +0000
> > > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> > >
> > > > The threshold for tap detection was still not scaled. The datasheet sets
> > > > a scale factor of 62.5mg/LSB. Remove commit about not scaled threshold
> > > > for tap detection, and apply scaling to it.
> > > >
> > >
> > > Given tap detection algorithms are not generally well defined and not a simple
> > > threshold (generally) what scaling should we be aiming for here?
> > > Even if it were a simple threshold, when a channel provides _raw the
> > > expectation is that event config is vs _raw, not the base units.
> > >
> > > So if this doesn't care about the current fullscale range (which the
> > > comment implied was the case) it would need to rescale when the
> > > IIO_INFO_SCALE changes.
> > >
> > > That comment is I think indicating we decided to gloss over the
> > > detail because it's going into a (potentially) non trivial algorithm anyway.
> > >
> > > Jonathan
> > >
> >
> > Well, the tap threshold so far was around in "raw" LSB bits. At that
> > time I only left the comment that the value is not scaled. The current
> > patch is just putting now the scale factor and the sysfs handle then
> > will take values of 'g' and not just raw bits. This is like for the
> > other scaled values such as periods.
>
> Tricky corner because tap isn't a simple threshold - it it were I'd have
> a cleaner argument.
>
> If we were doing this it would need to be scalling to m/s^2 not g but
> that's not important for this discussion.
>
> Huh. For thresholds I thought we had this clear in the ABI docs, but we don't.
> The ABI doc refers to having _raw_ in the name which I'm not sure has been true
> in a very long time.  The convention is intended to be if the channel
> has _raw the thresholds are in that unit (i.e. ADC counts) and if not
> they are in the processed value units.
>
> It has to be this way because of non linear sensors.  We have cases
> where there isn't a transform we can sensibly convert in the kernel
> to set a 'raw' threshold.   (involves cube roots for instance).
> As a side note, those sensors are one of the few cases where we have
> both _RAW and _PROCESSED because the thresholds have to relate to _RAW
> but we need _PROCESSED to give standard units.
>
> Now for this case where it's kind of tangentially connected by the
> particular algorithm to the raw reading things are non obvious.
> The tap detector could just as easily be a threshold on jerk -
> rate of change of acceleration or some 'score' calculated from
> a bunch of inputs in which case we couldn't apply a scaling.
>
> >
> > I think at the time I left the thresholds a bit out, because for me
> > it's clear what a time is. But I'm not sure, if actually the
> > thresholds are going so much by the unit values. So, in particular
> > what is missing here? Is it just about the commit message, or does it
> > need technical further adjustments?
>
> I don't think the patch is needed. For this particular parameter there
> isn't a clear concept of scale (putting aside that for this particular
> sensor there is one).  Thus it's a twiddle control. No need to connect
> it to real world units at all.  Also change this is an ABI change
> so we should do it only if we are considering the change to be fixing
> a bug.
>

Great to hear! To be honest, I was a bit worried that finally I missed
scaling the threshold to units of g. Then I made it right just by
chance, using the raw values. Patch will be dropped in v10.

Best,
L

> Jonathan
>
> >
> > Best,
> > L
> > >
> > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > > ---
> > > >  drivers/iio/accel/adxl345_core.c | 11 +++++------
> > > >  1 file changed, 5 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > > > index 7c093c0241de..d80efb68d113 100644
> > > > --- a/drivers/iio/accel/adxl345_core.c
> > > > +++ b/drivers/iio/accel/adxl345_core.c
> > > > @@ -697,17 +697,15 @@ static int adxl345_read_event_value(struct iio_dev *indio_dev,
> > > >               switch (info) {
> > > >               case IIO_EV_INFO_VALUE:
> > > >                       /*
> > > > -                      * The scale factor would be 62.5mg/LSB (i.e. 0xFF = 16g) but
> > > > -                      * not applied here. In context of this general purpose sensor,
> > > > -                      * what imports is rather signal intensity than the absolute
> > > > -                      * measured g value.
> > > > +                      * Scale factor is 62.5mg/LSB i.e. 0xff = 16g
> > > >                        */
> > > >                       ret = regmap_read(st->regmap, ADXL345_REG_THRESH_TAP,
> > > >                                         &tap_threshold);
> > > >                       if (ret)
> > > >                               return ret;
> > > > -                     *val = sign_extend32(tap_threshold, 7);
> > > > -                     return IIO_VAL_INT;
> > > > +                     *val = 62500 * sign_extend32(tap_threshold, 7);
> > > > +                     *val2 = MICRO;
> > > > +                     return IIO_VAL_FRACTIONAL;
> > > >               case IIO_EV_INFO_TIMEOUT:
> > > >                       *val = st->tap_duration_us;
> > > >                       *val2 = 1000000;
> > > > @@ -746,6 +744,7 @@ static int adxl345_write_event_value(struct iio_dev *indio_dev,
> > > >       case IIO_EV_TYPE_GESTURE:
> > > >               switch (info) {
> > > >               case IIO_EV_INFO_VALUE:
> > > > +                     val = DIV_ROUND_CLOSEST(val * MICRO + val2, 62500);
> > > >                       ret = regmap_write(st->regmap, ADXL345_REG_THRESH_TAP,
> > > >                                          min(val, 0xFF));
> > > >                       if (ret)
> > >
>

Re: [PATCH v9 08/11] iio: accel: adxl345: add inactivity feature

[Lothar Rubusch]

On Thu, Jun 12, 2025 at 2:15 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Tue, Jun 10, 2025 at 09:59:30PM +0000, Lothar Rubusch wrote:
> > Add the inactivity feature of the sensor to the driver. When activity
> > and inactivity are enabled, a link bit will be set linking activity and
> > inactivity handling. Additionally, the auto-sleep mode will be enabled.
> > Due to the link bit the sensor is going to auto-sleep when inactivity
> > was detected.
> >
> > Inactivity detection needs a threshold to be configured and a period of
> > time in seconds. After, it will transition to inactivity state, if
> > measurements stay below inactivity threshold.
> >
> > When a ODR is configured the period for inactivity is adjusted with a
> > corresponding reasonable default value, in order to have higher
> > frequencies, lower inactivity times, and lower sample frequency but
> > give more time until inactivity. Both with reasonable upper and lower
> > boundaries, since many of the sensor's features (e.g. auto-sleep) will
> > need to operate between 12.5 Hz and 400 Hz. This is a default setting
> > when actively changing sample frequency, explicitly setting the time
> > until inactivity will overwrite the default.
> >
> > Similarly, setting the g-range will provide a default value for the
> > activity and inactivity thresholds. Both are implicit defaults, but
> > equally can be overwritten to be explicitly configured.
>
> ...
>
> > +static const struct iio_event_spec adxl345_fake_chan_events[] = {
> > +     {
> > +             /* inactivity */
> > +             .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),
>
> Slightly better
>
>                 .mask_shared_by_type =
>                         BIT(IIO_EV_INFO_VALUE) |
>                         BIT(IIO_EV_INFO_PERIOD),
>
> > +     },
> > +};
>
> And the same for other similar cases.
>
> ...
>
> > +/**
> > + * adxl345_set_inact_time - Configure inactivity time explicitly or by ODR.
> > + * @st: The sensor state instance.
> > + * @val_s: A desired time value, between 0 and 255.
> > + *
> > + * Inactivity time can be configured between 1 and 255 sec. If a val_s of 0
> > + * is configured by a user, then a default inactivity time will be computed.
> > + *
> > + * In such case, it should take power consumption into consideration. Thus it
> > + * shall be shorter for higher frequencies and longer for lower frequencies.
> > + * Hence, frequencies above 255 Hz shall default to 10 s and frequencies below
> > + * 10 Hz shall result in 255 s to detect inactivity.
> > + *
> > + * The approach simply subtracts the pre-decimal figure of the configured
> > + * sample frequency from 255 s to compute inactivity time [s]. Sub-Hz are thus
> > + * ignored in this estimation. The recommended ODRs for various features
> > + * (activity/inactivity, sleep modes, free fall, etc.) lie between 12.5 Hz and
> > + * 400 Hz, thus higher or lower frequencies will result in the boundary
> > + * defaults or need to be explicitly specified via val_s.
> > + *
> > + * Return: 0 or error value.
> > + */
> > +static int adxl345_set_inact_time(struct adxl345_state *st, u32 val_s)
> > +{
> > +     unsigned int max_boundary = 255;
> > +     unsigned int min_boundary = 10;
> > +     unsigned int val = min(val_s, max_boundary);
> > +     enum adxl345_odr odr;
> > +     unsigned int regval;
> > +     int ret;
> > +
> > +     if (val == 0) {
> > +             ret = regmap_read(st->regmap, ADXL345_REG_BW_RATE, &regval);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             odr = FIELD_GET(ADXL345_BW_RATE_MSK, regval);
>
> > +             val = (adxl345_odr_tbl[odr][0] > max_boundary)
> > +                     ? min_boundary : max_boundary - adxl345_odr_tbl[odr][0];
>
> clamp() ?
>

Isn't clamp() dealing with signed ints? Also, I'll take the diff from
max_boundary here. So, I'll try staying with the current line and hope
it's fine.

Best,
L

> > +     }
> > +
> > +     return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
> > +}
>
> ...
>
> >       if (type == ADXL345_ACTIVITY) {
> >               axis_ctrl = ADXL345_ACT_X_EN | ADXL345_ACT_Y_EN |
> >                               ADXL345_ACT_Z_EN;
> >       } else {
> > -             axis_ctrl = 0x00;
> > +             axis_ctrl = ADXL345_INACT_X_EN | ADXL345_INACT_Y_EN |
> > +                             ADXL345_INACT_Z_EN;
> >       }
>
> Now this can be as simple as
>
>         axis_ctrl = ADXL345_ACT_X_EN;
>         if (type == ADXL345_ACTIVITY)
>                 axis_ctrl |= ADXL345_ACT_Y_EN | ADXL345_ACT_Z_EN;
>         else
>                 axis_ctrl |= ADXL345_INACT_Y_EN | ADXL345_INACT_Z_EN;
>
> Yeah, I don't know how to make the diff better (it gets worse), but the end
> result is better.
>
> One way, which I don't like much is to previously have this conditional written as:
>
>         axis_ctrl = ADXL345_ACT_X_EN;
>         if (type == ADXL345_ACTIVITY)
>                 axis_ctrl |= ADXL345_ACT_Y_EN | ADXL345_ACT_Z_EN;
>         else
>                 axis_ctrl = 0;
>
> ...
>
> > +     ret = regmap_assign_bits(st->regmap, ADXL345_REG_POWER_CTL,
> > +                              (ADXL345_POWER_CTL_AUTO_SLEEP | ADXL345_POWER_CTL_LINK),
>
> Unneeded parentheses.
>
> > +                              en);
> >       if (ret)
> >               return ret;
>
> ...
>
> >  static int adxl345_set_odr(struct adxl345_state *st, enum adxl345_odr odr)
> >  {
> > -     return regmap_update_bits(st->regmap, ADXL345_REG_BW_RATE,
> > +     int ret;
> > +
> > +     ret = regmap_update_bits(st->regmap, ADXL345_REG_BW_RATE,
> >                                ADXL345_BW_RATE_MSK,
> >                                FIELD_PREP(ADXL345_BW_RATE_MSK, odr));
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* update inactivity time by ODR */
> > +     return adxl345_set_inact_time(st, 0);
>
> Okay, in this case the initial form of
>
>         int ret;
>
>         ret = ...
>         if (ret)
>                 return ret;
>
>         return 0;
>
>
> will be better with the respectful comment (as Jonathan suggested) in that
> change that this is not optimal as standalone change, but it will help reduce
> churn in the next change(s).
>
> >  }
>
> ...
>
> >  static int adxl345_set_range(struct adxl345_state *st, enum adxl345_range range)
> >  {
> > -     return regmap_update_bits(st->regmap, ADXL345_REG_DATA_FORMAT,
>
> Same here.
>
> > +     unsigned int act_threshold, inact_threshold;
> > +     unsigned int range_old;
> > +     unsigned int regval;
> > +     int ret;
> > +
> > +     ret = regmap_read(st->regmap, ADXL345_REG_DATA_FORMAT, &regval);
> > +     if (ret)
> > +             return ret;
> > +     range_old = FIELD_GET(ADXL345_DATA_FORMAT_RANGE, regval);
> > +
> > +     ret = regmap_read(st->regmap,
> > +                       adxl345_act_thresh_reg[ADXL345_ACTIVITY],
> > +                       &act_threshold);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = regmap_read(st->regmap,
> > +                       adxl345_act_thresh_reg[ADXL345_INACTIVITY],
> > +                       &inact_threshold);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = regmap_update_bits(st->regmap, ADXL345_REG_DATA_FORMAT,
> >                                ADXL345_DATA_FORMAT_RANGE,
> >                                FIELD_PREP(ADXL345_DATA_FORMAT_RANGE, range));
> > +     if (ret)
> > +             return ret;
> > +
> > +     act_threshold = act_threshold * adxl345_range_factor_tbl[range_old]
> > +             / adxl345_range_factor_tbl[range];
> > +     act_threshold = min(U8_MAX, max(1, act_threshold));
> > +
> > +     inact_threshold = inact_threshold * adxl345_range_factor_tbl[range_old]
> > +             / adxl345_range_factor_tbl[range];
> > +     inact_threshold = min(U8_MAX, max(1, inact_threshold));
> > +
> > +     ret = regmap_write(st->regmap, adxl345_act_thresh_reg[ADXL345_ACTIVITY],
> > +                        act_threshold);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return regmap_write(st->regmap, adxl345_act_thresh_reg[ADXL345_INACTIVITY],
> > +                        inact_threshold);
> >  }
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v9 08/11] iio: accel: adxl345: add inactivity feature

[Andy Shevchenko]

On Sat, Jun 21, 2025 at 9:54 PM Lothar Rubusch <l.rubusch@gmail.com> wrote:
> On Thu, Jun 12, 2025 at 2:15 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Tue, Jun 10, 2025 at 09:59:30PM +0000, Lothar Rubusch wrote:

Please, remove the pieces of email you agree with, or comment on why
you disagree. Leaving tons of the pieces without comments is confusing
at bare minimum.

...

> > > +             val = (adxl345_odr_tbl[odr][0] > max_boundary)
> > > +                     ? min_boundary : max_boundary - adxl345_odr_tbl[odr][0];
> >
> > clamp() ?
>
> Isn't clamp() dealing with signed ints?

clamp() is a macro.

> Also, I'll take the diff from
> max_boundary here.

How does it affect usage of the clamp()?

> So, I'll try staying with the current line and hope
> it's fine.

I suggest you spend some time thinking about this expression on how to
make it easier to read and understand. In my opinion clamp() helps a
lot in this case.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v9 08/11] iio: accel: adxl345: add inactivity feature

[Andy Shevchenko]

On Sat, Jun 21, 2025 at 10:24 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Sat, Jun 21, 2025 at 9:54 PM Lothar Rubusch <l.rubusch@gmail.com> wrote:
> > On Thu, Jun 12, 2025 at 2:15 PM Andy Shevchenko
> > <andriy.shevchenko@intel.com> wrote:
> > > On Tue, Jun 10, 2025 at 09:59:30PM +0000, Lothar Rubusch wrote:

...

> > > > +             val = (adxl345_odr_tbl[odr][0] > max_boundary)
> > > > +                     ? min_boundary : max_boundary - adxl345_odr_tbl[odr][0];
> > >
> > > clamp() ?
> >
> > Isn't clamp() dealing with signed ints?
>
> clamp() is a macro.
>
> > Also, I'll take the diff from
> > max_boundary here.
>
> How does it affect usage of the clamp()?

I see what you mean, but this can be done by clamping the signed values.
So, in any case try to improve this and add a comment explaining why
the maximum boundary is adjusted.

> > So, I'll try staying with the current line and hope
> > it's fine.
>
> I suggest you spend some time thinking about this expression on how to
> make it easier to read and understand. In my opinion clamp() helps a
> lot in this case.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v9 07/11] iio: accel: adxl345: add activity event feature

[Andy Shevchenko]

Sat, Jun 21, 2025 at 08:06:49PM +0200, Lothar Rubusch kirjoitti:

[...]

> > > +     ret = regmap_read(st->regmap, ADXL345_REG_ACT_INACT_CTRL, &axis_ctrl);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     switch (type) {
> > > +     case ADXL345_ACTIVITY:
> > > +             en = FIELD_GET(ADXL345_ACT_X_EN, axis_ctrl) |
> > > +                     FIELD_GET(ADXL345_ACT_Y_EN, axis_ctrl) |
> > > +                     FIELD_GET(ADXL345_ACT_Z_EN, axis_ctrl);
> >
> > Something happened to the indentation.
> > Ditto for several places in the code (upper and lower from this).
> 
> What is the matter with the indention here? I'm doing `checkpatch.pl
> --strict --codespell` on that and don't get an issue? Would you expect
> cases like the FIELD_GET() calls on the next line, linked by a binary
> OR, to be indented by yet another TAB?

All 'F':s should be on the same column (since the email already mangled
[not by me], here is just an example).

             en = FIELD_GET(ADXL345_ACT_X_EN, axis_ctrl) |
                  FIELD_GET(ADXL345_ACT_Y_EN, axis_ctrl) |
                  FIELD_GET(ADXL345_ACT_Z_EN, axis_ctrl);

> > > +             break;
> > > +     default:
> > > +             return -EINVAL;
> > > +     }
> > > +
> > > +     if (!en)
> > > +             return en;
> > > +
> > > +     /* Check if corresponding interrupts are enabled */
> > > +     ret = regmap_read(st->regmap, ADXL345_REG_INT_ENABLE, &regval);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     return adxl345_act_int_reg[type] & regval;
> > > +}

...

Really, cut the unrelated context in the replies!

-- 
With Best Regards,
Andy Shevchenko