[PATCH v1 00/12] iio: accel: adxl345: add interrupt based sensor events

[PATCH v1 00/12] iio: accel: adxl345: add interrupt based sensor events

[Lothar Rubusch]

Add several interrupt based sensor detection events:
- single tap
- double tap
- free fall
- activity
- inactivity

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

The sensor has further features still not covered:
- g-ranges scaled by different ODRs, especially for activity / inactivity
  threshold is not covered so far. There seems to be a particularity with
  the ADXL345 as annotated on some analog FAQ.

- Various thinks like low power, sleep mode, etc. are (still) not covered
  here, others (ACDC bit, selftest, etc.) currently are hard coded or not
  covered.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
Questions:
- Do I need a mutex/lock protection as this is the case e.g. in the ADXL367
  or the ADXL380?
  Actually, I understand those cases as protecting access to the state
  object by different channels, temperature and accelerometer. I'm unsure
  if this is a correct understanding, where for the ADXL345 there should
  not be any issue. At most, a currently displayed value on sysfs is
  (still) not updated. So, IMHO I can rely on the internal protections in
  regmap no further mutex is needed. Please, can you give me a feedback
  here?

- FIELD_PREP/FIELD_GET: I'd like to use arrays of enum indexed elements
  to allow for more generic function implementation passing just a "type"
  field, e.g. at single tap/double tap or activity/inactivity handling.
  When it comes to filtering out bits using FIELD_GET/FIELD_PREP, it says
  that this enum array element is not "const enough". Is there a
  work-around?

Lothar Rubusch (12):
  iio: accel: adxl345: migrate constants to core
  iio: accel: adxl345: reorganize measurement enable
  iio: accel: adxl345: add debug register access
  iio: accel: adxl345: reorganize irq handler
  iio: accel: adxl345: improve access to the interrupt enable register
  iio: accel: adxl345: add single tap feature
  iio: accel: adxl345: show tap status and direction
  iio: accel: adxl345: add double tap feature
  iio: accel: adxl345: add double tap suppress bit
  iio: accel: adxl345: add freefall feature
  iio: accel: adxl345: add activity feature
  iio: accel: adxl345: add inactivity feature

 drivers/iio/accel/adxl345.h      |   86 ---
 drivers/iio/accel/adxl345_core.c | 1150 ++++++++++++++++++++++++++++--
 2 files changed, 1099 insertions(+), 137 deletions(-)

-- 
2.39.5

[PATCH v1 01/12] iio: accel: adxl345: migrate constants to core

[Lothar Rubusch]

The set of constants does not need to be exposed. Move constants to core
to reduce namespace polution.

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

diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
index 517e494ba555..b5257dafb742 100644
--- a/drivers/iio/accel/adxl345.h
+++ b/drivers/iio/accel/adxl345.h
@@ -8,94 +8,8 @@
 #ifndef _ADXL345_H_
 #define _ADXL345_H_
 
-#define ADXL345_REG_DEVID		0x00
-#define ADXL345_REG_THRESH_TAP		0x1D
-#define ADXL345_REG_OFSX		0x1E
-#define ADXL345_REG_OFSY		0x1F
-#define ADXL345_REG_OFSZ		0x20
-#define ADXL345_REG_OFS_AXIS(index)	(ADXL345_REG_OFSX + (index))
-
-/* Tap duration */
-#define ADXL345_REG_DUR		0x21
-/* Tap latency */
-#define ADXL345_REG_LATENT		0x22
-/* Tap window */
-#define ADXL345_REG_WINDOW		0x23
-/* Activity threshold */
-#define ADXL345_REG_THRESH_ACT		0x24
-/* Inactivity threshold */
-#define ADXL345_REG_THRESH_INACT	0x25
-/* Inactivity time */
-#define ADXL345_REG_TIME_INACT		0x26
-/* Axis enable control for activity and inactivity detection */
-#define ADXL345_REG_ACT_INACT_CTRL	0x27
-/* Free-fall threshold */
-#define ADXL345_REG_THRESH_FF		0x28
-/* Free-fall time */
-#define ADXL345_REG_TIME_FF		0x29
-/* Axis control for single tap or double tap */
-#define ADXL345_REG_TAP_AXIS		0x2A
-/* Source of single tap or double tap */
-#define ADXL345_REG_ACT_TAP_STATUS	0x2B
-/* Data rate and power mode control */
-#define ADXL345_REG_BW_RATE		0x2C
-#define ADXL345_REG_POWER_CTL		0x2D
-#define ADXL345_REG_INT_ENABLE		0x2E
-#define ADXL345_REG_INT_MAP		0x2F
-#define ADXL345_REG_INT_SOURCE		0x30
-#define ADXL345_REG_INT_SOURCE_MSK	0xFF
 #define ADXL345_REG_DATA_FORMAT		0x31
-#define ADXL345_REG_XYZ_BASE		0x32
-#define ADXL345_REG_DATA_AXIS(index)				\
-	(ADXL345_REG_XYZ_BASE + (index) * sizeof(__le16))
-
-#define ADXL345_REG_FIFO_CTL		0x38
-#define ADXL345_FIFO_CTL_SAMPLES_MSK	GENMASK(4, 0)
-/* 0: INT1, 1: INT2 */
-#define ADXL345_FIFO_CTL_TRIGGER_MSK	BIT(5)
-#define ADXL345_FIFO_CTL_MODE_MSK	GENMASK(7, 6)
-#define ADXL345_REG_FIFO_STATUS	0x39
-#define ADXL345_REG_FIFO_STATUS_MSK	0x3F
-
-#define ADXL345_INT_OVERRUN		BIT(0)
-#define ADXL345_INT_WATERMARK		BIT(1)
-#define ADXL345_INT_FREE_FALL		BIT(2)
-#define ADXL345_INT_INACTIVITY		BIT(3)
-#define ADXL345_INT_ACTIVITY		BIT(4)
-#define ADXL345_INT_DOUBLE_TAP		BIT(5)
-#define ADXL345_INT_SINGLE_TAP		BIT(6)
-#define ADXL345_INT_DATA_READY		BIT(7)
-
-/*
- * BW_RATE bits - Bandwidth and output data rate. The default value is
- * 0x0A, which translates to a 100 Hz output data rate
- */
-#define ADXL345_BW_RATE			GENMASK(3, 0)
-#define ADXL345_BW_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)
-#define ADXL345_POWER_CTL_AUTO_SLEEP	BIT(4)
-#define ADXL345_POWER_CTL_LINK		BIT(5)
-
-/* Set the g range */
-#define ADXL345_DATA_FORMAT_RANGE	GENMASK(1, 0)
-/* Data is left justified */
-#define ADXL345_DATA_FORMAT_JUSTIFY	BIT(2)
-/* Up to 13-bits resolution */
-#define ADXL345_DATA_FORMAT_FULL_RES	BIT(3)
 #define ADXL345_DATA_FORMAT_SPI_3WIRE	BIT(6)
-#define ADXL345_DATA_FORMAT_SELF_TEST	BIT(7)
-#define ADXL345_DATA_FORMAT_2G		0
-#define ADXL345_DATA_FORMAT_4G		1
-#define ADXL345_DATA_FORMAT_8G		2
-#define ADXL345_DATA_FORMAT_16G		3
-
-#define ADXL345_DEVID			0xE5
-#define ADXL345_FIFO_SIZE		32
 
 /*
  * In full-resolution mode, scale factor is maintained at ~4 mg/LSB
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index ed0291bea0f5..ffdb03ed7a25 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -14,13 +14,100 @@
 #include <linux/regmap.h>
 #include <linux/units.h>
 
-#include <linux/iio/iio.h>
-#include <linux/iio/sysfs.h>
 #include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
 #include <linux/iio/kfifo_buf.h>
+#include <linux/iio/sysfs.h>
 
 #include "adxl345.h"
 
+#define ADXL345_REG_DEVID		0x00
+#define ADXL345_REG_THRESH_TAP		0x1D
+#define ADXL345_REG_OFSX		0x1E
+#define ADXL345_REG_OFSY		0x1F
+#define ADXL345_REG_OFSZ		0x20
+#define ADXL345_REG_OFS_AXIS(index)	(ADXL345_REG_OFSX + (index))
+
+/* Tap duration */
+#define ADXL345_REG_DUR			0x21
+/* Tap latency */
+#define ADXL345_REG_LATENT		0x22
+/* Tap window */
+#define ADXL345_REG_WINDOW		0x23
+/* Activity threshold */
+#define ADXL345_REG_THRESH_ACT		0x24
+/* Inactivity threshold */
+#define ADXL345_REG_THRESH_INACT	0x25
+/* Inactivity time */
+#define ADXL345_REG_TIME_INACT		0x26
+/* Axis enable control for activity and inactivity detection */
+#define ADXL345_REG_ACT_INACT_CTRL	0x27
+/* Free-fall threshold */
+#define ADXL345_REG_THRESH_FF		0x28
+/* Free-fall time */
+#define ADXL345_REG_TIME_FF		0x29
+/* Axis control for single tap or double tap */
+#define ADXL345_REG_TAP_AXIS		0x2A
+/* Source of single tap or double tap */
+#define ADXL345_REG_ACT_TAP_STATUS	0x2B
+/* Data rate and power mode control */
+#define ADXL345_REG_BW_RATE		0x2C
+#define ADXL345_REG_POWER_CTL		0x2D
+#define ADXL345_REG_INT_ENABLE		0x2E
+#define ADXL345_REG_INT_MAP		0x2F
+#define ADXL345_REG_INT_SOURCE		0x30
+#define ADXL345_REG_INT_SOURCE_MSK	0xFF
+#define ADXL345_REG_XYZ_BASE		0x32
+#define ADXL345_REG_DATA_AXIS(index)				\
+	(ADXL345_REG_XYZ_BASE + (index) * sizeof(__le16))
+
+#define ADXL345_REG_FIFO_CTL		0x38
+#define ADXL345_FIFO_CTL_SAMPLES_MSK	GENMASK(4, 0)
+/* 0: INT1, 1: INT2 */
+#define ADXL345_FIFO_CTL_TRIGGER_MSK	BIT(5)
+#define ADXL345_FIFO_CTL_MODE_MSK	GENMASK(7, 6)
+#define ADXL345_REG_FIFO_STATUS	0x39
+#define ADXL345_REG_FIFO_STATUS_MSK	0x3F
+
+#define ADXL345_INT_OVERRUN		BIT(0)
+#define ADXL345_INT_WATERMARK		BIT(1)
+#define ADXL345_INT_FREE_FALL		BIT(2)
+#define ADXL345_INT_INACTIVITY		BIT(3)
+#define ADXL345_INT_ACTIVITY		BIT(4)
+#define ADXL345_INT_DOUBLE_TAP		BIT(5)
+#define ADXL345_INT_SINGLE_TAP		BIT(6)
+#define ADXL345_INT_DATA_READY		BIT(7)
+
+/*
+ * BW_RATE bits - Bandwidth and output data rate. The default value is
+ * 0x0A, which translates to a 100 Hz output data rate
+ */
+#define ADXL345_BW_RATE			GENMASK(3, 0)
+#define ADXL345_BW_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)
+#define ADXL345_POWER_CTL_AUTO_SLEEP	BIT(4)
+#define ADXL345_POWER_CTL_LINK		BIT(5)
+
+/* Set the g range */
+#define ADXL345_DATA_FORMAT_RANGE	GENMASK(1, 0)
+/* Data is left justified */
+#define ADXL345_DATA_FORMAT_JUSTIFY	BIT(2)
+/* Up to 13-bits resolution */
+#define ADXL345_DATA_FORMAT_FULL_RES	BIT(3)
+#define ADXL345_DATA_FORMAT_SELF_TEST	BIT(7)
+#define ADXL345_DATA_FORMAT_2G		0
+#define ADXL345_DATA_FORMAT_4G		1
+#define ADXL345_DATA_FORMAT_8G		2
+#define ADXL345_DATA_FORMAT_16G		3
+
+#define ADXL345_DEVID			0xE5
+#define ADXL345_FIFO_SIZE		32
+
 #define ADXL345_FIFO_BYPASS	0
 #define ADXL345_FIFO_FIFO	1
 #define ADXL345_FIFO_STREAM	2
-- 
2.39.5

Re: [PATCH v1 01/12] iio: accel: adxl345: migrate constants to core

[Jonathan Cameron]

On Tue, 28 Jan 2025 12:00:49 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> The set of constants does not need to be exposed. Move constants to core
> to reduce namespace polution.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
Hi Lothar,


> -#define ADXL345_REG_INT_MAP		0x2F
> -#define ADXL345_REG_INT_SOURCE		0x30
> -#define ADXL345_REG_INT_SOURCE_MSK	0xFF
>  #define ADXL345_REG_DATA_FORMAT		0x31

Normally I'd be entirely in favour of this, but...
I'm not sure we want to leave one random register here
and move the rest.

Se can move the stuff that isn't register related, but
for the registers I'd prefer to keep them in one place
and I can't see a clean way to move them all to the core.c
file. Even separating reg address and fields within it
makes for a harder check against a datasheet etc.

So I think all we can move is the fifo size :(


> -#define ADXL345_REG_XYZ_BASE		0x32
> -#define ADXL345_REG_DATA_AXIS(index)				\
> -	(ADXL345_REG_XYZ_BASE + (index) * sizeof(__le16))

Re: [PATCH v1 01/12] iio: accel: adxl345: migrate constants to core

[Lothar Rubusch]

Hi Jonathan,

On Sat, Feb 1, 2025 at 5:36 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 28 Jan 2025 12:00:49 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > The set of constants does not need to be exposed. Move constants to core
> > to reduce namespace polution.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> Hi Lothar,
>
>
> > -#define ADXL345_REG_INT_MAP          0x2F
> > -#define ADXL345_REG_INT_SOURCE               0x30
> > -#define ADXL345_REG_INT_SOURCE_MSK   0xFF
> >  #define ADXL345_REG_DATA_FORMAT              0x31
>
> Normally I'd be entirely in favour of this, but...
> I'm not sure we want to leave one random register here
> and move the rest.
>
> Se can move the stuff that isn't register related, but
> for the registers I'd prefer to keep them in one place
> and I can't see a clean way to move them all to the core.c
> file. Even separating reg address and fields within it
> makes for a harder check against a datasheet etc.
>
> So I think all we can move is the fifo size :(
>

I understood that it could be one of the first follow up patches to move those
defines (parts of them?) over to core, as here in this mail:
https://lore.kernel.org/linux-iio/20241214123926.0b42ea59@jic23-huawei/
Anyway, I already had presented moving the constants before, when you
had decided to keep them in the header. I thought you changed your mind
on that, but I don't want to bother you with the same issue over and over
again, probably I missunderstood that here.

I leave the constants in the .h file then, no problem. :) I can understand the
intention to keep the things rather together in one place. There seem to be
pros & cons for both.

>
> > -#define ADXL345_REG_XYZ_BASE         0x32
> > -#define ADXL345_REG_DATA_AXIS(index)                         \
> > -     (ADXL345_REG_XYZ_BASE + (index) * sizeof(__le16))
>

Re: [PATCH v1 01/12] iio: accel: adxl345: migrate constants to core

[Jonathan Cameron]

On Tue, 4 Feb 2025 15:13:34 +0100
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Hi Jonathan,
> 
> On Sat, Feb 1, 2025 at 5:36 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Tue, 28 Jan 2025 12:00:49 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >  
> > > The set of constants does not need to be exposed. Move constants to core
> > > to reduce namespace polution.
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>  
> > Hi Lothar,
> >
> >  
> > > -#define ADXL345_REG_INT_MAP          0x2F
> > > -#define ADXL345_REG_INT_SOURCE               0x30
> > > -#define ADXL345_REG_INT_SOURCE_MSK   0xFF
> > >  #define ADXL345_REG_DATA_FORMAT              0x31  
> >
> > Normally I'd be entirely in favour of this, but...
> > I'm not sure we want to leave one random register here
> > and move the rest.
> >
> > Se can move the stuff that isn't register related, but
> > for the registers I'd prefer to keep them in one place
> > and I can't see a clean way to move them all to the core.c
> > file. Even separating reg address and fields within it
> > makes for a harder check against a datasheet etc.
> >
> > So I think all we can move is the fifo size :(
> >  
> 
> I understood that it could be one of the first follow up patches to move those
> defines (parts of them?) over to core, as here in this mail:
> https://lore.kernel.org/linux-iio/20241214123926.0b42ea59@jic23-huawei/
> Anyway, I already had presented moving the constants before, when you
> had decided to keep them in the header. I thought you changed your mind
> on that, but I don't want to bother you with the same issue over and over
> again, probably I missunderstood that here.

I'd failed to realize we had to leave one behind :( 

Sorry for the misdirection!

Jonathan
> 
> I leave the constants in the .h file then, no problem. :) I can understand the
> intention to keep the things rather together in one place. There seem to be
> pros & cons for both.
> 
> >  
> > > -#define ADXL345_REG_XYZ_BASE         0x32
> > > -#define ADXL345_REG_DATA_AXIS(index)                         \
> > > -     (ADXL345_REG_XYZ_BASE + (index) * sizeof(__le16))  
> >  
> 

[PATCH v1 02/12] iio: accel: adxl345: reorganize measurement enable

[Lothar Rubusch]

In order to have this function generically available a position at the
top makes more sense. In upcomming patches for particular features the
function needs to be available, to turn off measuring while changing
settings, and turn it on again afterwards.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index ffdb03ed7a25..142f12117627 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -163,6 +163,33 @@ static const unsigned long adxl345_scan_masks[] = {
 	0
 };
 
+/**
+ * adxl345_set_measure_en() - Enable and disable measuring.
+ *
+ * @st: The device data.
+ * @en: Enable measurements, else standby mode.
+ *
+ * For lowest power operation, standby mode can be used. In standby mode,
+ * current consumption is supposed to be reduced to 0.1uA (typical). In this
+ * mode no measurements are made. Placing the device into standby mode
+ * preserves the contents of FIFO.
+ *
+ * Return: Returns 0 if successful, or a negative error value.
+ */
+static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
+{
+	unsigned int val = en ? ADXL345_POWER_CTL_MEASURE : ADXL345_POWER_CTL_STANDBY;
+
+	return regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
+}
+
+static void adxl345_powerdown(void *ptr)
+{
+	struct adxl345_state *st = ptr;
+
+	adxl345_set_measure_en(st, false);
+}
+
 static int adxl345_set_interrupts(struct adxl345_state *st)
 {
 	int ret;
@@ -301,33 +328,6 @@ static int adxl345_write_raw_get_fmt(struct iio_dev *indio_dev,
 	}
 }
 
-/**
- * adxl345_set_measure_en() - Enable and disable measuring.
- *
- * @st: The device data.
- * @en: Enable measurements, else standby mode.
- *
- * For lowest power operation, standby mode can be used. In standby mode,
- * current consumption is supposed to be reduced to 0.1uA (typical). In this
- * mode no measurements are made. Placing the device into standby mode
- * preserves the contents of FIFO.
- *
- * Return: Returns 0 if successful, or a negative error value.
- */
-static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
-{
-	unsigned int val = en ? ADXL345_POWER_CTL_MEASURE : ADXL345_POWER_CTL_STANDBY;
-
-	return regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
-}
-
-static void adxl345_powerdown(void *ptr)
-{
-	struct adxl345_state *st = ptr;
-
-	adxl345_set_measure_en(st, false);
-}
-
 static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
 "0.09765625 0.1953125 0.390625 0.78125 1.5625 3.125 6.25 12.5 25 50 100 200 400 800 1600 3200"
 );
-- 
2.39.5

Re: [PATCH v1 02/12] iio: accel: adxl345: reorganize measurement enable

[Jonathan Cameron]

On Tue, 28 Jan 2025 12:00:50 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> In order to have this function generically available a position at the
> top makes more sense. In upcomming patches for particular features the
> function needs to be available, to turn off measuring while changing
> settings, and turn it on again afterwards.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl345_core.c | 54 ++++++++++++++++----------------
>  1 file changed, 27 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index ffdb03ed7a25..142f12117627 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -163,6 +163,33 @@ static const unsigned long adxl345_scan_masks[] = {
>  	0
>  };
>  
> +/**
> + * adxl345_set_measure_en() - Enable and disable measuring.
> + *
> + * @st: The device data.
> + * @en: Enable measurements, else standby mode.
> + *
> + * For lowest power operation, standby mode can be used. In standby mode,
> + * current consumption is supposed to be reduced to 0.1uA (typical). In this
> + * mode no measurements are made. Placing the device into standby mode
> + * preserves the contents of FIFO.
> + *
> + * Return: Returns 0 if successful, or a negative error value.
> + */
> +static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
> +{
> +	unsigned int val = en ? ADXL345_POWER_CTL_MEASURE : ADXL345_POWER_CTL_STANDBY;
> +
> +	return regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
> +}
> +
> +static void adxl345_powerdown(void *ptr)
> +{
> +	struct adxl345_state *st = ptr;
> +
> +	adxl345_set_measure_en(st, false);
> +}
Why move powerdown?  This looks to be a devm callback, if that's all it used for
leave that down near wherever it is used.
Moving set_measure_en makes sense, just not this user of it.

> +
>  static int adxl345_set_interrupts(struct adxl345_state *st)
>  {
>  	int ret;
> @@ -301,33 +328,6 @@ static int adxl345_write_raw_get_fmt(struct iio_dev *indio_dev,
>  	}
>  }
>  
> -/**
> - * adxl345_set_measure_en() - Enable and disable measuring.
> - *
> - * @st: The device data.
> - * @en: Enable measurements, else standby mode.
> - *
> - * For lowest power operation, standby mode can be used. In standby mode,
> - * current consumption is supposed to be reduced to 0.1uA (typical). In this
> - * mode no measurements are made. Placing the device into standby mode
> - * preserves the contents of FIFO.
> - *
> - * Return: Returns 0 if successful, or a negative error value.
> - */
> -static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
> -{
> -	unsigned int val = en ? ADXL345_POWER_CTL_MEASURE : ADXL345_POWER_CTL_STANDBY;
> -
> -	return regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
> -}
> -
> -static void adxl345_powerdown(void *ptr)
> -{
> -	struct adxl345_state *st = ptr;
> -
> -	adxl345_set_measure_en(st, false);
> -}
> -
>  static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
>  "0.09765625 0.1953125 0.390625 0.78125 1.5625 3.125 6.25 12.5 25 50 100 200 400 800 1600 3200"
>  );

[PATCH v1 03/12] iio: accel: adxl345: add debug register access

[Lothar Rubusch]

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

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 142f12117627..8fbf0a43746f 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -296,6 +296,16 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
 	return -EINVAL;
 }
 
+static int adxl345_reg_access(struct iio_dev *indio_dev, unsigned int reg,
+			      unsigned int writeval, unsigned int *readval)
+{
+	struct adxl345_state *st = iio_priv(indio_dev);
+
+	if (readval)
+		return regmap_read(st->regmap, reg, readval);
+	return regmap_write(st->regmap, reg, writeval);
+}
+
 static int adxl345_set_watermark(struct iio_dev *indio_dev, unsigned int value)
 {
 	struct adxl345_state *st = iio_priv(indio_dev);
@@ -554,6 +564,7 @@ static const struct iio_info adxl345_info = {
 	.read_raw	= adxl345_read_raw,
 	.write_raw	= adxl345_write_raw,
 	.write_raw_get_fmt	= adxl345_write_raw_get_fmt,
+	.debugfs_reg_access = &adxl345_reg_access,
 	.hwfifo_set_watermark = adxl345_set_watermark,
 };
 
-- 
2.39.5

[PATCH v1 04/12] iio: accel: adxl345: reorganize irq handler

[Lothar Rubusch]

Reorganize the IRQ handler. Move the overrun handling to the bottom.
Overrun leads to reset the interrupt register. This also happens at
evaluation of a particular interrupt event. So, actually it makes more
sense to evaluate the event if possible, and only fall back to pure
overrun handling as a last resort. Further simplify fetching the
interrupt status function. Both is in preparation to build interrupt
handling up for the handling of different detected events, implemented
in follow up patches.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 8fbf0a43746f..7ee50a0b23ea 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -491,16 +491,9 @@ static const struct iio_buffer_setup_ops adxl345_buffer_ops = {
 	.predisable = adxl345_buffer_predisable,
 };
 
-static int adxl345_get_status(struct adxl345_state *st)
+static int adxl345_get_status(struct adxl345_state *st, unsigned int *int_stat)
 {
-	int ret;
-	unsigned int regval;
-
-	ret = regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, &regval);
-	if (ret < 0)
-		return ret;
-
-	return FIELD_GET(ADXL345_REG_INT_SOURCE_MSK, regval);
+	return regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, int_stat);
 }
 
 static int adxl345_fifo_push(struct iio_dev *indio_dev,
@@ -536,14 +529,10 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
 	int int_stat;
 	int samples;
 
-	int_stat = adxl345_get_status(st);
-	if (int_stat <= 0)
+	if (adxl345_get_status(st, &int_stat))
 		return IRQ_NONE;
 
-	if (int_stat & ADXL345_INT_OVERRUN)
-		goto err;
-
-	if (int_stat & ADXL345_INT_WATERMARK) {
+	if (FIELD_GET(ADXL345_INT_WATERMARK, int_stat)) {
 		samples = adxl345_get_samples(st);
 		if (samples < 0)
 			goto err;
@@ -551,6 +540,10 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
 		if (adxl345_fifo_push(indio_dev, samples) < 0)
 			goto err;
 	}
+
+	if (FIELD_GET(ADXL345_INT_OVERRUN, int_stat))
+		goto err;
+
 	return IRQ_HANDLED;
 
 err:
-- 
2.39.5

Re: [PATCH v1 04/12] iio: accel: adxl345: reorganize irq handler

[Jonathan Cameron]

On Tue, 28 Jan 2025 12:00:52 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Reorganize the IRQ handler. Move the overrun handling to the bottom.
> Overrun leads to reset the interrupt register. This also happens at
> evaluation of a particular interrupt event. So, actually it makes more
> sense to evaluate the event if possible, and only fall back to pure
> overrun handling as a last resort. Further simplify fetching the
> interrupt status function. Both is in preparation to build interrupt

Both are preparatory steps to build

> handling up for the handling of different detected events, implemented
> in follow up patches.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>

> ---
>  drivers/iio/accel/adxl345_core.c | 23 ++++++++---------------
>  1 file changed, 8 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 8fbf0a43746f..7ee50a0b23ea 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -491,16 +491,9 @@ static const struct iio_buffer_setup_ops adxl345_buffer_ops = {
>  	.predisable = adxl345_buffer_predisable,
>  };
>  
> -static int adxl345_get_status(struct adxl345_state *st)
> +static int adxl345_get_status(struct adxl345_state *st, unsigned int *int_stat)
>  {
> -	int ret;
> -	unsigned int regval;
> -
> -	ret = regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, &regval);
> -	if (ret < 0)
> -		return ret;
> -
> -	return FIELD_GET(ADXL345_REG_INT_SOURCE_MSK, regval);

Maybe worth commenting in the patch description that this is the whole register
anyway.  Makes it obvious why this isn't a functional change.

Also delete that MSK value as it is rather pointless!
> +	return regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, int_stat);
>  }
>  
>  static int adxl345_fifo_push(struct iio_dev *indio_dev,
> @@ -536,14 +529,10 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
>  	int int_stat;
>  	int samples;
>  
> -	int_stat = adxl345_get_status(st);
> -	if (int_stat <= 0)
> +	if (adxl345_get_status(st, &int_stat))
>  		return IRQ_NONE;
>  
> -	if (int_stat & ADXL345_INT_OVERRUN)
> -		goto err;
> -
> -	if (int_stat & ADXL345_INT_WATERMARK) {
> +	if (FIELD_GET(ADXL345_INT_WATERMARK, int_stat)) {
>  		samples = adxl345_get_samples(st);
>  		if (samples < 0)
>  			goto err;
> @@ -551,6 +540,10 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
>  		if (adxl345_fifo_push(indio_dev, samples) < 0)
>  			goto err;
>  	}
> +
> +	if (FIELD_GET(ADXL345_INT_OVERRUN, int_stat))
> +		goto err;
> +
>  	return IRQ_HANDLED;
>  
>  err:

[PATCH v1 05/12] iio: accel: adxl345: improve access to the interrupt enable register

[Lothar Rubusch]

Split the current set_interrupts() functionality. Separate writing the
interrupt map from writing the interrupt enable register.

Move writing the interrupt map into the probe(). The interrupt map will
setup which event finally will go over the INT line. Thus, all events
are mapped to this interrupt line now once at the beginning.

On the other side the function set_interrupts() will now be focussed on
enabling interrupts for event features. Thus it will be renamed to
write_interrupts() to better distinguish from further usage of get/set
in the conext of the sensor features.

Also, add the missing initial reset of the interrupt enable register.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 7ee50a0b23ea..b55f6774b1e9 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -190,25 +190,9 @@ static void adxl345_powerdown(void *ptr)
 	adxl345_set_measure_en(st, false);
 }
 
-static int adxl345_set_interrupts(struct adxl345_state *st)
+static inline int adxl345_write_interrupts(struct adxl345_state *st)
 {
-	int ret;
-	unsigned int int_enable = st->int_map;
-	unsigned int int_map;
-
-	/*
-	 * Any bits set to 0 in the INT map register send their respective
-	 * interrupts to the INT1 pin, whereas bits set to 1 send their respective
-	 * interrupts to the INT2 pin. The intio shall convert this accordingly.
-	 */
-	int_map = FIELD_GET(ADXL345_REG_INT_SOURCE_MSK,
-			    st->intio ? st->int_map : ~st->int_map);
-
-	ret = regmap_write(st->regmap, ADXL345_REG_INT_MAP, int_map);
-	if (ret)
-		return ret;
-
-	return regmap_write(st->regmap, ADXL345_REG_INT_ENABLE, int_enable);
+	return regmap_write(st->regmap, ADXL345_REG_INT_ENABLE, st->int_map);
 }
 
 static int adxl345_read_raw(struct iio_dev *indio_dev,
@@ -464,7 +448,7 @@ static int adxl345_buffer_postenable(struct iio_dev *indio_dev)
 	struct adxl345_state *st = iio_priv(indio_dev);
 	int ret;
 
-	ret = adxl345_set_interrupts(st);
+	ret = adxl345_write_interrupts(st);
 	if (ret < 0)
 		return ret;
 
@@ -483,7 +467,7 @@ static int adxl345_buffer_predisable(struct iio_dev *indio_dev)
 		return ret;
 
 	st->int_map = 0x00;
-	return adxl345_set_interrupts(st);
+	return adxl345_write_interrupts(st);
 }
 
 static const struct iio_buffer_setup_ops adxl345_buffer_ops = {
@@ -602,6 +586,8 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 		return -ENODEV;
 	st->fifo_delay = fifo_delay_default;
 
+	st->int_map = 0x00;			/* reset interrupts */
+
 	indio_dev->name = st->info->name;
 	indio_dev->info = &adxl345_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
@@ -609,6 +595,11 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 	indio_dev->num_channels = ARRAY_SIZE(adxl345_channels);
 	indio_dev->available_scan_masks = adxl345_scan_masks;
 
+	/* Reset interrupts at start up */
+	ret = adxl345_write_interrupts(st);
+	if (ret)
+		return ret;
+
 	if (setup) {
 		/* Perform optional initial bus specific configuration */
 		ret = setup(dev, st->regmap);
@@ -659,6 +650,18 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 	}
 
 	if (st->intio != ADXL345_INT_NONE) {
+		/*
+		 * Any bits set to 0 in the INT map register send their respective
+		 * interrupts to the INT1 pin, whereas bits set to 1 send their respective
+		 * interrupts to the INT2 pin. The intio shall convert this accordingly.
+		 */
+		regval = st->intio ? ADXL345_REG_INT_SOURCE_MSK
+			: ~ADXL345_REG_INT_SOURCE_MSK;
+
+		ret = regmap_write(st->regmap, ADXL345_REG_INT_MAP, regval);
+		if (ret)
+			return ret;
+
 		/* FIFO_STREAM mode is going to be activated later */
 		ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, &adxl345_buffer_ops);
 		if (ret)
-- 
2.39.5

Re: [PATCH v1 05/12] iio: accel: adxl345: improve access to the interrupt enable register

[Jonathan Cameron]

On Tue, 28 Jan 2025 12:00:53 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Split the current set_interrupts() functionality. Separate writing the
> interrupt map from writing the interrupt enable register.
Makes sense.

> 
> Move writing the interrupt map into the probe(). The interrupt map will
> setup which event finally will go over the INT line. Thus, all events
> are mapped to this interrupt line now once at the beginning.
> 
> On the other side the function set_interrupts() will now be focussed on
> enabling interrupts for event features. Thus it will be renamed to
> write_interrupts() to better distinguish from further usage of get/set
> in the conext of the sensor features.

For this, I'm not yet seeing why we need a function to do this. May
become clear later.

> 
> Also, add the missing initial reset of the interrupt enable register.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl345_core.c | 43 +++++++++++++++++---------------
>  1 file changed, 23 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 7ee50a0b23ea..b55f6774b1e9 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -190,25 +190,9 @@ static void adxl345_powerdown(void *ptr)
>  	adxl345_set_measure_en(st, false);
>  }
>  
> -static int adxl345_set_interrupts(struct adxl345_state *st)
> +static inline int adxl345_write_interrupts(struct adxl345_state *st)
>  {
> -	int ret;
> -	unsigned int int_enable = st->int_map;
> -	unsigned int int_map;
> -
> -	/*
> -	 * Any bits set to 0 in the INT map register send their respective
> -	 * interrupts to the INT1 pin, whereas bits set to 1 send their respective
> -	 * interrupts to the INT2 pin. The intio shall convert this accordingly.
> -	 */
> -	int_map = FIELD_GET(ADXL345_REG_INT_SOURCE_MSK,
> -			    st->intio ? st->int_map : ~st->int_map);
> -
> -	ret = regmap_write(st->regmap, ADXL345_REG_INT_MAP, int_map);
> -	if (ret)
> -		return ret;
> -
> -	return regmap_write(st->regmap, ADXL345_REG_INT_ENABLE, int_enable);
> +	return regmap_write(st->regmap, ADXL345_REG_INT_ENABLE, st->int_map);
Not sure the function adds much.  Maybe it will later, but my gut
feeling on what is here would be to just do the regamp_write() inline
where this function is currently called.
>  }
>  

>  
>  static const struct iio_buffer_setup_ops adxl345_buffer_ops = {
> @@ -602,6 +586,8 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  		return -ENODEV;
>  	st->fifo_delay = fifo_delay_default;
>  
> +	st->int_map = 0x00;			/* reset interrupts */
> +
>  	indio_dev->name = st->info->name;
>  	indio_dev->info = &adxl345_info;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
> @@ -609,6 +595,11 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  	indio_dev->num_channels = ARRAY_SIZE(adxl345_channels);
>  	indio_dev->available_scan_masks = adxl345_scan_masks;
>  
> +	/* Reset interrupts at start up */
> +	ret = adxl345_write_interrupts(st);
> +	if (ret)
> +		return ret;
> +
>  	if (setup) {
>  		/* Perform optional initial bus specific configuration */
>  		ret = setup(dev, st->regmap);
> @@ -659,6 +650,18 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  	}
>  
>  	if (st->intio != ADXL345_INT_NONE) {
> +		/*
> +		 * Any bits set to 0 in the INT map register send their respective
> +		 * interrupts to the INT1 pin, whereas bits set to 1 send their respective
> +		 * interrupts to the INT2 pin. The intio shall convert this accordingly.
> +		 */
> +		regval = st->intio ? ADXL345_REG_INT_SOURCE_MSK
> +			: ~ADXL345_REG_INT_SOURCE_MSK;

This mask is another slightly odd one as it just means all bits in the register.
Maybe better just using values here? 0x00 vs 0xFF

> +
> +		ret = regmap_write(st->regmap, ADXL345_REG_INT_MAP, regval);
> +		if (ret)
> +			return ret;
> +
>  		/* FIFO_STREAM mode is going to be activated later */
>  		ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, &adxl345_buffer_ops);
>  		if (ret)

[PATCH v1 06/12] iio: accel: adxl345: add single tap feature

[Lothar Rubusch]

Add the single tap feature with a threshold in 62.5mg/LSB points and a
scaled duration in us. Keep singletap threshold by means of IIO but add
sysfs entry for the duration. Using a sysfs entry allow for a clearer
naming of the handle to improve usage. Extend the channels for single
enable x/y/z axis of the feature but also check if threshold (a.k.a
"value") and duration have reasonable content. When an interrupt is
caught it will be pushed to the according IIO channel.

The function call structure is in preparation to be extended for an
upcoming doubletap feature in the follow up patches.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index b55f6774b1e9..0d991f3ec10c 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -15,6 +15,7 @@
 #include <linux/units.h>
 
 #include <linux/iio/buffer.h>
+#include <linux/iio/events.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/kfifo_buf.h>
 #include <linux/iio/sysfs.h>
@@ -118,6 +119,16 @@
 #define ADXL345_INT1			0
 #define ADXL345_INT2			1
 
+#define ADXL345_REG_TAP_AXIS_MSK	GENMASK(2, 0)
+
+enum adxl345_axis {
+	ADXL345_Z_EN = BIT(0),
+	ADXL345_Y_EN = BIT(1),
+	ADXL345_X_EN = BIT(2),
+	/* Suppress double tap detection if value > tap threshold */
+	ADXL345_TAP_SUPPRESS = BIT(3),
+};
+
 struct adxl345_state {
 	const struct adxl345_chip_info *info;
 	struct regmap *regmap;
@@ -127,9 +138,24 @@ struct adxl345_state {
 	u8 int_map;
 	u8 watermark;
 	u8 fifo_mode;
+
+	u32 tap_axis_ctrl;
+	u8 tap_threshold;
+	u32 tap_duration_us;
+
 	__le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE + 1] __aligned(IIO_DMA_MINALIGN);
 };
 
+static struct iio_event_spec adxl345_events[] = {
+	{
+		/* single tap */
+		.type = IIO_EV_TYPE_GESTURE,
+		.dir = IIO_EV_DIR_SINGLETAP,
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
+		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE),
+	},
+};
+
 #define ADXL345_CHANNEL(index, reg, axis) {					\
 	.type = IIO_ACCEL,						\
 	.modified = 1,							\
@@ -146,6 +172,8 @@ struct adxl345_state {
 		.storagebits = 16,			\
 		.endianness = IIO_LE,			\
 	},						\
+	.event_spec = adxl345_events,				\
+	.num_event_specs = ARRAY_SIZE(adxl345_events),	\
 }
 
 enum adxl345_chans {
@@ -190,11 +218,121 @@ static void adxl345_powerdown(void *ptr)
 	adxl345_set_measure_en(st, false);
 }
 
+static inline void adxl345_intmap_switch_bit(struct adxl345_state *st,
+					     bool condition, u8 bit)
+{
+	st->int_map = condition ? st->int_map | bit : st->int_map & ~bit;
+}
+
+static inline int adxl345_read_interrupts(struct adxl345_state *st,
+					  unsigned int *interrupts)
+{
+	return regmap_read(st->regmap, ADXL345_REG_INT_ENABLE, interrupts);
+}
+
 static inline int adxl345_write_interrupts(struct adxl345_state *st)
 {
 	return regmap_write(st->regmap, ADXL345_REG_INT_ENABLE, st->int_map);
 }
 
+/* tap */
+
+static int adxl345_write_tap_axis(struct adxl345_state *st,
+				  enum adxl345_axis axis, bool en)
+{
+	st->tap_axis_ctrl = FIELD_GET(ADXL345_REG_TAP_AXIS_MSK,
+				      en ? st->tap_axis_ctrl | axis
+				      : st->tap_axis_ctrl & ~axis);
+
+	return regmap_update_bits(st->regmap, ADXL345_REG_TAP_AXIS,
+				  ADXL345_REG_TAP_AXIS_MSK,
+				  FIELD_PREP(ADXL345_REG_TAP_AXIS_MSK,
+					     st->tap_axis_ctrl));
+}
+
+static int _adxl345_set_tap_int(struct adxl345_state *st, bool state)
+{
+	bool axis_valid;
+	bool singletap_args_valid = false;
+	bool en = false;
+
+	axis_valid = FIELD_GET(ADXL345_REG_TAP_AXIS_MSK, st->tap_axis_ctrl) > 0;
+
+	/*
+	 * Note: A value of 0 for threshold and/or dur may result in undesirable
+	 *	 behavior if single tap/double tap interrupts are enabled.
+	 */
+	singletap_args_valid = st->tap_threshold > 0 && st->tap_duration_us > 0;
+
+	en = axis_valid && singletap_args_valid;
+
+	adxl345_intmap_switch_bit(st, state && en, ADXL345_INT_SINGLE_TAP);
+
+	return adxl345_write_interrupts(st);
+}
+
+static int adxl345_is_tap_en(struct adxl345_state *st, bool *en)
+{
+	int ret;
+	unsigned int regval;
+
+	ret = adxl345_read_interrupts(st, &regval);
+	if (ret)
+		return ret;
+
+	*en = FIELD_GET(ADXL345_INT_SINGLE_TAP, regval) > 0;
+
+	return 0;
+}
+
+static int adxl345_set_singletap_en(struct adxl345_state *st,
+				    enum adxl345_axis axis, bool en)
+{
+	int ret;
+
+	ret = adxl345_write_tap_axis(st, axis, en);
+	if (ret)
+		return ret;
+
+	return _adxl345_set_tap_int(st, en);
+}
+
+static int adxl345_set_tap_value(struct adxl345_state *st, u8 val)
+{
+	st->tap_threshold = val;
+
+	return regmap_write(st->regmap, ADXL345_REG_THRESH_TAP, min(val, 0xFF));
+}
+
+static int _adxl345_set_tap_time(struct adxl345_state *st, u32 val_us)
+{
+	unsigned int regval;
+
+	st->tap_duration_us = val_us;
+
+	/*
+	 * The scale factor is 1250us / LSB for tap_window_us and tap_latent_us.
+	 * For tap_duration_us the scale factor is 625us / LSB.
+	 */
+	regval = DIV_ROUND_CLOSEST(val_us, 625);
+
+	return regmap_write(st->regmap, ADXL345_REG_DUR, regval);
+}
+
+static int adxl345_set_tap_duration(struct adxl345_state *st, u32 val_int,
+				    u32 val_fract_us)
+{
+	/*
+	 * Max value is 255 * 625 us = 0.159375 seconds
+	 *
+	 * Note: the scaling is similar to the scaling in the ADXL380
+	 */
+	if (val_int || val_fract_us > 159375)
+		return -EINVAL;
+
+	return _adxl345_set_tap_time(st, val_fract_us);
+}
+
 static int adxl345_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan,
 			    int *val, int *val2, long mask)
@@ -275,6 +413,141 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
 					  ADXL345_BW_RATE,
 					  clamp_val(ilog2(n), 0,
 						    ADXL345_BW_RATE));
+	default:
+		return -EINVAL;
+	}
+
+	return -EINVAL;
+}
+
+static int adxl345_read_event_config(struct iio_dev *indio_dev,
+				     const struct iio_chan_spec *chan,
+				     enum iio_event_type type,
+				     enum iio_event_direction dir)
+{
+	struct adxl345_state *st = iio_priv(indio_dev);
+	bool int_en;
+	bool axis_en;
+	int ret = -EFAULT;
+
+	switch (type) {
+	case IIO_EV_TYPE_GESTURE:
+		switch (dir) {
+		case IIO_EV_DIR_SINGLETAP:
+			switch (chan->channel2) {
+			case IIO_MOD_X:
+				axis_en = FIELD_GET(ADXL345_X_EN, st->tap_axis_ctrl);
+				break;
+			case IIO_MOD_Y:
+				axis_en = FIELD_GET(ADXL345_Y_EN, st->tap_axis_ctrl);
+				break;
+			case IIO_MOD_Z:
+				axis_en = FIELD_GET(ADXL345_Z_EN, st->tap_axis_ctrl);
+				break;
+			default:
+				return -EINVAL;
+			}
+
+			ret = adxl345_is_tap_en(st, &int_en);
+			if (ret)
+				return ret;
+			return int_en && axis_en;
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
+static int adxl345_write_event_config(struct iio_dev *indio_dev,
+				      const struct iio_chan_spec *chan,
+				      enum iio_event_type type,
+				      enum iio_event_direction dir,
+				      int state)
+{
+	struct adxl345_state *st = iio_priv(indio_dev);
+	enum adxl345_axis axis;
+
+	if (type != IIO_EV_TYPE_GESTURE)
+		return -EINVAL;
+
+	switch (dir) {
+	case IIO_EV_DIR_SINGLETAP:
+		switch (chan->channel2) {
+		case IIO_MOD_X:
+			axis = ADXL345_X_EN;
+			break;
+		case IIO_MOD_Y:
+			axis = ADXL345_Y_EN;
+			break;
+		case IIO_MOD_Z:
+			axis = ADXL345_Z_EN;
+			break;
+		default:
+			return -EINVAL;
+		}
+
+		return adxl345_set_singletap_en(st, axis, state);
+	default:
+		return -EINVAL;
+	}
+
+	return -EINVAL;
+}
+
+static int adxl345_read_event_value(struct iio_dev *indio_dev, const struct iio_chan_spec *chan,
+				    enum iio_event_type type, enum iio_event_direction dir,
+				    enum iio_event_info info, int *val, int *val2)
+{
+	struct adxl345_state *st = iio_priv(indio_dev);
+
+	if (type != IIO_EV_TYPE_GESTURE)
+		return -EINVAL;
+
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		/*
+		 * The scale factor is 62.5mg/LSB (i.e. 0xFF = 16g) but
+		 * not applied here.
+		 */
+		*val = sign_extend32(st->tap_threshold, 7);
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+
+	return -EINVAL;
+}
+
+static int adxl345_write_event_value(struct iio_dev *indio_dev,
+				     const struct iio_chan_spec *chan,
+				     enum iio_event_type type,
+				     enum iio_event_direction dir,
+				     enum iio_event_info info,
+				     int val, int val2)
+{
+	struct adxl345_state *st = iio_priv(indio_dev);
+	int ret;
+
+	if (type != IIO_EV_TYPE_GESTURE)
+		return -EINVAL;
+
+	if (info == IIO_EV_INFO_VALUE) {
+		if (val < 0 || val > 255)
+			return -EINVAL;
+
+		ret = adxl345_set_measure_en(st, false);
+		if (ret)
+			return ret;
+
+		ret = adxl345_set_tap_value(st, val);
+		if (ret)
+			return ret;
+
+		return adxl345_set_measure_en(st, true);
 	}
 
 	return -EINVAL;
@@ -322,6 +595,58 @@ static int adxl345_write_raw_get_fmt(struct iio_dev *indio_dev,
 	}
 }
 
+#define ADXL345_generate_iio_dev_attr_FRACTIONAL(A, B, C, D, E)		\
+	static ssize_t in_accel_##A##_##C##_##E##_show(struct device *dev, \
+						       struct device_attribute *attr, \
+						       char *buf)	\
+	{								\
+		struct iio_dev *indio_dev = dev_to_iio_dev(dev);	\
+		struct adxl345_state *st = iio_priv(indio_dev);		\
+		int vals[2];						\
+									\
+		vals[0] = st->B##_##C##_##E;				\
+		vals[1] = D;						\
+									\
+		return iio_format_value(buf, IIO_VAL_FRACTIONAL, 2, vals); \
+	}								\
+									\
+	static ssize_t in_accel_##A##_##C##_##E##_store(struct device *dev, \
+							struct device_attribute *attr, \
+							const char *buf, size_t len) \
+	{								\
+		struct iio_dev *indio_dev = dev_to_iio_dev(dev);	\
+		struct adxl345_state *st = iio_priv(indio_dev);		\
+		int val_int, val_fract_us, ret;				\
+									\
+		ret = iio_str_to_fixpoint(buf, 100000, &val_int, &val_fract_us); \
+		if (ret)						\
+			return ret;					\
+									\
+		ret = adxl345_set_measure_en(st, false);		\
+		if (ret)						\
+			return ret;					\
+									\
+		adxl345_set_##B##_##C(st, val_int, val_fract_us);	\
+									\
+		ret = adxl345_set_measure_en(st, true);			\
+		if (ret)						\
+			return ret;					\
+									\
+		return len;						\
+	}								\
+	static IIO_DEVICE_ATTR_RW(in_accel_##A##_##C##_##E, 0)
+
+ADXL345_generate_iio_dev_attr_FRACTIONAL(gesture_singletap, tap, duration, MICRO, us);
+
+static struct attribute *adxl345_event_attrs[] = {
+	&iio_dev_attr_in_accel_gesture_singletap_duration_us.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group adxl345_event_attrs_group = {
+	.attrs = adxl345_event_attrs,
+};
+
 static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
 "0.09765625 0.1953125 0.390625 0.78125 1.5625 3.125 6.25 12.5 25 50 100 200 400 800 1600 3200"
 );
@@ -477,6 +802,17 @@ static const struct iio_buffer_setup_ops adxl345_buffer_ops = {
 
 static int adxl345_get_status(struct adxl345_state *st, unsigned int *int_stat)
 {
+	unsigned int regval;
+	bool check_tap_stat;
+
+	check_tap_stat = FIELD_GET(ADXL345_REG_TAP_AXIS_MSK, st->tap_axis_ctrl) > 0;
+
+	if (check_tap_stat) {
+		/* ACT_TAP_STATUS should be read before clearing the interrupt */
+		if (regmap_read(st->regmap, ADXL345_REG_ACT_TAP_STATUS, &regval))
+			return -EINVAL;
+	}
+
 	return regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, int_stat);
 }
 
@@ -499,6 +835,25 @@ static int adxl345_fifo_push(struct iio_dev *indio_dev,
 	return 0;
 }
 
+static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat)
+{
+	s64 ts = iio_get_time_ns(indio_dev);
+	int ret;
+
+	if (FIELD_GET(ADXL345_INT_SINGLE_TAP, int_stat)) {
+		ret = iio_push_event(indio_dev,
+				     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
+							IIO_MOD_X_OR_Y_OR_Z,
+							IIO_EV_TYPE_GESTURE,
+							IIO_EV_DIR_SINGLETAP),
+				     ts);
+		if (ret)
+			return ret;
+	}
+
+	return -ENOENT;
+}
+
 /**
  * adxl345_irq_handler() - Handle irqs of the ADXL345.
  * @irq: The irq being handled.
@@ -516,6 +871,9 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
 	if (adxl345_get_status(st, &int_stat))
 		return IRQ_NONE;
 
+	if (adxl345_push_event(indio_dev, int_stat) == 0)
+		return IRQ_HANDLED;
+
 	if (FIELD_GET(ADXL345_INT_WATERMARK, int_stat)) {
 		samples = adxl345_get_samples(st);
 		if (samples < 0)
@@ -538,9 +896,14 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
 
 static const struct iio_info adxl345_info = {
 	.attrs		= &adxl345_attrs_group,
+	.event_attrs	= &adxl345_event_attrs_group,
 	.read_raw	= adxl345_read_raw,
 	.write_raw	= adxl345_write_raw,
 	.write_raw_get_fmt	= adxl345_write_raw_get_fmt,
+	.read_event_config = adxl345_read_event_config,
+	.write_event_config = adxl345_write_event_config,
+	.read_event_value = adxl345_read_event_value,
+	.write_event_value = adxl345_write_event_value,
 	.debugfs_reg_access = &adxl345_reg_access,
 	.hwfifo_set_watermark = adxl345_set_watermark,
 };
@@ -588,6 +951,10 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 
 	st->int_map = 0x00;			/* reset interrupts */
 
+	/* Init with reasonable values */
+	st->tap_threshold = 35;			/*   35 [0x23]            */
+	st->tap_duration_us = 3;		/*    3 [0x03] -> .001875 */
+
 	indio_dev->name = st->info->name;
 	indio_dev->info = &adxl345_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
-- 
2.39.5

Re: [PATCH v1 06/12] iio: accel: adxl345: add single tap feature

[Jonathan Cameron]

On Tue, 28 Jan 2025 12:00:54 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add the single tap feature with a threshold in 62.5mg/LSB points and a
> scaled duration in us. Keep singletap threshold by means of IIO but add
> sysfs entry for the duration. Using a sysfs entry allow for a clearer
> naming of the handle to improve usage. Extend the channels for single
> enable x/y/z axis of the feature but also check if threshold (a.k.a
> "value") and duration have reasonable content. When an interrupt is
> caught it will be pushed to the according IIO channel.
> 
> The function call structure is in preparation to be extended for an
> upcoming doubletap feature in the follow up patches.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
The duration ABI isn't standard, so it should come with ABI docs and
some explanation of why we can't use existing ABI.  The postfix of _us
whilst it seems sensible is not inline with existing IIO ABI. Times
are always in seconds.

> +static inline void adxl345_intmap_switch_bit(struct adxl345_state *st,
> +					     bool condition, u8 bit)
> +{
> +	st->int_map = condition ? st->int_map | bit : st->int_map & ~bit;

I'm not convinced the wrapper is that useful.
Maybe can use __clear_bit() and __set_bit()

> +}
> +
> +static inline int adxl345_read_interrupts(struct adxl345_state *st,
> +					  unsigned int *interrupts)
> +{
> +	return regmap_read(st->regmap, ADXL345_REG_INT_ENABLE, interrupts);

I don't see an advantage in this wrapper.  Why not just call regmap
directly. It's pretty self documenting!

> +}
> +
>  static inline int adxl345_write_interrupts(struct adxl345_state *st)
>  {
>  	return regmap_write(st->regmap, ADXL345_REG_INT_ENABLE, st->int_map);
>  }

>  static int adxl345_read_raw(struct iio_dev *indio_dev,
>  			    struct iio_chan_spec const *chan,
>  			    int *val, int *val2, long mask)
> @@ -275,6 +413,141 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
>  					  ADXL345_BW_RATE,
>  					  clamp_val(ilog2(n), 0,
>  						    ADXL345_BW_RATE));
> +	default:
> +		return -EINVAL;
> +	}
> +
Doubt we can get here.


> +	return -EINVAL;
> +}
> +
> +static int adxl345_read_event_config(struct iio_dev *indio_dev,
> +				     const struct iio_chan_spec *chan,
> +				     enum iio_event_type type,
> +				     enum iio_event_direction dir)
> +{
> +	struct adxl345_state *st = iio_priv(indio_dev);
> +	bool int_en;
> +	bool axis_en;
> +	int ret = -EFAULT;
> +
> +	switch (type) {
> +	case IIO_EV_TYPE_GESTURE:
> +		switch (dir) {
> +		case IIO_EV_DIR_SINGLETAP:
> +			switch (chan->channel2) {
> +			case IIO_MOD_X:
> +				axis_en = FIELD_GET(ADXL345_X_EN, st->tap_axis_ctrl);
> +				break;
> +			case IIO_MOD_Y:
> +				axis_en = FIELD_GET(ADXL345_Y_EN, st->tap_axis_ctrl);
> +				break;
> +			case IIO_MOD_Z:
> +				axis_en = FIELD_GET(ADXL345_Z_EN, st->tap_axis_ctrl);
> +				break;
> +			default:
> +				return -EINVAL;
> +			}
> +
> +			ret = adxl345_is_tap_en(st, &int_en);
> +			if (ret)
> +				return ret;
> +			return int_en && axis_en;
> +		default:
> +			return -EINVAL;
> +		}
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return ret;

Can't get here I think.

> +}

> +#define ADXL345_generate_iio_dev_attr_FRACTIONAL(A, B, C, D, E)		\
> +	static ssize_t in_accel_##A##_##C##_##E##_show(struct device *dev, \
> +						       struct device_attribute *attr, \
> +						       char *buf)	\
> +	{								\
> +		struct iio_dev *indio_dev = dev_to_iio_dev(dev);	\
> +		struct adxl345_state *st = iio_priv(indio_dev);		\
> +		int vals[2];						\
> +									\
> +		vals[0] = st->B##_##C##_##E;				\
> +		vals[1] = D;						\
> +									\
> +		return iio_format_value(buf, IIO_VAL_FRACTIONAL, 2, vals); \
> +	}								\
> +									\
> +	static ssize_t in_accel_##A##_##C##_##E##_store(struct device *dev, \
> +							struct device_attribute *attr, \
> +							const char *buf, size_t len) \
> +	{								\
> +		struct iio_dev *indio_dev = dev_to_iio_dev(dev);	\
> +		struct adxl345_state *st = iio_priv(indio_dev);		\
> +		int val_int, val_fract_us, ret;				\
> +									\
> +		ret = iio_str_to_fixpoint(buf, 100000, &val_int, &val_fract_us); \
> +		if (ret)						\
> +			return ret;					\
> +									\
> +		ret = adxl345_set_measure_en(st, false);		\
> +		if (ret)						\
> +			return ret;					\
> +									\
> +		adxl345_set_##B##_##C(st, val_int, val_fract_us);	\
> +									\
> +		ret = adxl345_set_measure_en(st, true);			\
> +		if (ret)						\
> +			return ret;					\
> +									\
> +		return len;						\
> +	}								\
> +	static IIO_DEVICE_ATTR_RW(in_accel_##A##_##C##_##E, 0)
> +
> +ADXL345_generate_iio_dev_attr_FRACTIONAL(gesture_singletap, tap, duration, MICRO, us);
> +
> +static struct attribute *adxl345_event_attrs[] = {
> +	&iio_dev_attr_in_accel_gesture_singletap_duration_us.dev_attr.attr,

New ABI so I should be seeing ABI docs.
Also durations (anything time related) in IIO ABI is in seconds.
Does this not map to existing documented ABI of
in_accel_gesture_tap_wait_dur?


> +	NULL
> +};
> +
> +static const struct attribute_group adxl345_event_attrs_group = {
> +	.attrs = adxl345_event_attrs,
> +};
> +
>  static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
>  "0.09765625 0.1953125 0.390625 0.78125 1.5625 3.125 6.25 12.5 25 50 100 200 400 800 1600 3200"
>  );
> @@ -477,6 +802,17 @@ static const struct iio_buffer_setup_ops adxl345_buffer_ops = {
>  
>  static int adxl345_get_status(struct adxl345_state *st, unsigned int *int_stat)
>  {
> +	unsigned int regval;
> +	bool check_tap_stat;
> +
> +	check_tap_stat = FIELD_GET(ADXL345_REG_TAP_AXIS_MSK, st->tap_axis_ctrl) > 0;
> +
> +	if (check_tap_stat) {
> +		/* ACT_TAP_STATUS should be read before clearing the interrupt */
> +		if (regmap_read(st->regmap, ADXL345_REG_ACT_TAP_STATUS, &regval))
> +			return -EINVAL;
Don't eat the return value. It might be useful.
		ret = regmap_read()
		if (ret)
			return ret;
> +	}
> +
>  	return regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, int_stat);
>  }
>  
> @@ -499,6 +835,25 @@ static int adxl345_fifo_push(struct iio_dev *indio_dev,
>  	return 0;
>  }

>  /**
>   * adxl345_irq_handler() - Handle irqs of the ADXL345.
>   * @irq: The irq being handled.
> @@ -516,6 +871,9 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
>  	if (adxl345_get_status(st, &int_stat))
>  		return IRQ_NONE;
>  
> +	if (adxl345_push_event(indio_dev, int_stat) == 0)
> +		return IRQ_HANDLED;

Can have multiple interrupts at once. Say an event that happens to
occur very close to watermark. So I'd normally expect to
carry on looking for interrupts.

> +
>  	if (FIELD_GET(ADXL345_INT_WATERMARK, int_stat)) {
>  		samples = adxl345_get_samples(st);
>  		if (samples < 0)
> @@ -538,9 +896,14 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
>  
>  static const struct iio_info adxl345_info = {
>  	.attrs		= &adxl345_attrs_group,
> +	.event_attrs	= &adxl345_event_attrs_group,
>  	.read_raw	= adxl345_read_raw,
>  	.write_raw	= adxl345_write_raw,
>  	.write_raw_get_fmt	= adxl345_write_raw_get_fmt,
> +	.read_event_config = adxl345_read_event_config,
> +	.write_event_config = adxl345_write_event_config,
> +	.read_event_value = adxl345_read_event_value,
> +	.write_event_value = adxl345_write_event_value,
>  	.debugfs_reg_access = &adxl345_reg_access,
>  	.hwfifo_set_watermark = adxl345_set_watermark,
>  };
> @@ -588,6 +951,10 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  
>  	st->int_map = 0x00;			/* reset interrupts */
>  
> +	/* Init with reasonable values */
> +	st->tap_threshold = 35;			/*   35 [0x23]            */
> +	st->tap_duration_us = 3;		/*    3 [0x03] -> .001875 */
> +
>  	indio_dev->name = st->info->name;
>  	indio_dev->info = &adxl345_info;
>  	indio_dev->modes = INDIO_DIRECT_MODE;

[PATCH v1 07/12] iio: accel: adxl345: show tap status and direction

[Lothar Rubusch]

Provide information in the iio tap event about the tap direction. This
can be verified using 'iio_event_monior adxl345'. Reading out the
ACT_TAP_STATUS register is also in preparation for activity events.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 0d991f3ec10c..7831ec511941 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -800,17 +800,26 @@ static const struct iio_buffer_setup_ops adxl345_buffer_ops = {
 	.predisable = adxl345_buffer_predisable,
 };
 
-static int adxl345_get_status(struct adxl345_state *st, unsigned int *int_stat)
+static int adxl345_get_status(struct adxl345_state *st, unsigned int *int_stat,
+			      enum iio_modifier *act_tap_dir)
 {
 	unsigned int regval;
 	bool check_tap_stat;
 
+	*act_tap_dir = IIO_NO_MOD;
 	check_tap_stat = FIELD_GET(ADXL345_REG_TAP_AXIS_MSK, st->tap_axis_ctrl) > 0;
 
 	if (check_tap_stat) {
 		/* ACT_TAP_STATUS should be read before clearing the interrupt */
 		if (regmap_read(st->regmap, ADXL345_REG_ACT_TAP_STATUS, &regval))
 			return -EINVAL;
+
+		if (FIELD_GET(ADXL345_Z_EN, regval) > 0)
+			*act_tap_dir = IIO_MOD_Z;
+		else if (FIELD_GET(ADXL345_Y_EN, regval) > 0)
+			*act_tap_dir = IIO_MOD_Y;
+		else if (FIELD_GET(ADXL345_X_EN, regval) > 0)
+			*act_tap_dir = IIO_MOD_X;
 	}
 
 	return regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, int_stat);
@@ -835,7 +844,8 @@ static int adxl345_fifo_push(struct iio_dev *indio_dev,
 	return 0;
 }
 
-static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat)
+static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
+			      enum iio_modifier act_tap_dir)
 {
 	s64 ts = iio_get_time_ns(indio_dev);
 	int ret;
@@ -843,7 +853,7 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat)
 	if (FIELD_GET(ADXL345_INT_SINGLE_TAP, int_stat)) {
 		ret = iio_push_event(indio_dev,
 				     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
-							IIO_MOD_X_OR_Y_OR_Z,
+							act_tap_dir,
 							IIO_EV_TYPE_GESTURE,
 							IIO_EV_DIR_SINGLETAP),
 				     ts);
@@ -866,12 +876,13 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
 	struct iio_dev *indio_dev = p;
 	struct adxl345_state *st = iio_priv(indio_dev);
 	int int_stat;
+	enum iio_modifier act_tap_dir;
 	int samples;
 
-	if (adxl345_get_status(st, &int_stat))
+	if (adxl345_get_status(st, &int_stat, &act_tap_dir))
 		return IRQ_NONE;
 
-	if (adxl345_push_event(indio_dev, int_stat) == 0)
+	if (adxl345_push_event(indio_dev, int_stat, act_tap_dir) == 0)
 		return IRQ_HANDLED;
 
 	if (FIELD_GET(ADXL345_INT_WATERMARK, int_stat)) {
-- 
2.39.5

Re: [PATCH v1 07/12] iio: accel: adxl345: show tap status and direction

[Jonathan Cameron]

On Tue, 28 Jan 2025 12:00:55 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Provide information in the iio tap event about the tap direction. This
> can be verified using 'iio_event_monior adxl345'. Reading out the
> ACT_TAP_STATUS register is also in preparation for activity events.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>

So, this confuses me a little.  You have separate controls on
enabling the directions but only a single bit to indicate that a tap
was detected. Can only one be enabled at time?  If not how does
reporting only one direction make sense?

That aside, why not squash this with the previous patch?


Jonathan


> ---
>  drivers/iio/accel/adxl345_core.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 0d991f3ec10c..7831ec511941 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -800,17 +800,26 @@ static const struct iio_buffer_setup_ops adxl345_buffer_ops = {
>  	.predisable = adxl345_buffer_predisable,
>  };
>  
> -static int adxl345_get_status(struct adxl345_state *st, unsigned int *int_stat)
> +static int adxl345_get_status(struct adxl345_state *st, unsigned int *int_stat,
> +			      enum iio_modifier *act_tap_dir)
>  {
>  	unsigned int regval;
>  	bool check_tap_stat;
>  
> +	*act_tap_dir = IIO_NO_MOD;
>  	check_tap_stat = FIELD_GET(ADXL345_REG_TAP_AXIS_MSK, st->tap_axis_ctrl) > 0;
>  
>  	if (check_tap_stat) {
>  		/* ACT_TAP_STATUS should be read before clearing the interrupt */
>  		if (regmap_read(st->regmap, ADXL345_REG_ACT_TAP_STATUS, &regval))
>  			return -EINVAL;
> +
> +		if (FIELD_GET(ADXL345_Z_EN, regval) > 0)
> +			*act_tap_dir = IIO_MOD_Z;
> +		else if (FIELD_GET(ADXL345_Y_EN, regval) > 0)
> +			*act_tap_dir = IIO_MOD_Y;
> +		else if (FIELD_GET(ADXL345_X_EN, regval) > 0)
> +			*act_tap_dir = IIO_MOD_X;
>  	}
>  
>  	return regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, int_stat);
> @@ -835,7 +844,8 @@ static int adxl345_fifo_push(struct iio_dev *indio_dev,
>  	return 0;
>  }
>  
> -static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat)
> +static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
> +			      enum iio_modifier act_tap_dir)
>  {
>  	s64 ts = iio_get_time_ns(indio_dev);
>  	int ret;
> @@ -843,7 +853,7 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat)
>  	if (FIELD_GET(ADXL345_INT_SINGLE_TAP, int_stat)) {
>  		ret = iio_push_event(indio_dev,
>  				     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> -							IIO_MOD_X_OR_Y_OR_Z,
> +							act_tap_dir,
>  							IIO_EV_TYPE_GESTURE,
>  							IIO_EV_DIR_SINGLETAP),
>  				     ts);
> @@ -866,12 +876,13 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
>  	struct iio_dev *indio_dev = p;
>  	struct adxl345_state *st = iio_priv(indio_dev);
>  	int int_stat;
> +	enum iio_modifier act_tap_dir;
>  	int samples;
>  
> -	if (adxl345_get_status(st, &int_stat))
> +	if (adxl345_get_status(st, &int_stat, &act_tap_dir))
>  		return IRQ_NONE;
>  
> -	if (adxl345_push_event(indio_dev, int_stat) == 0)
> +	if (adxl345_push_event(indio_dev, int_stat, act_tap_dir) == 0)
>  		return IRQ_HANDLED;
>  
>  	if (FIELD_GET(ADXL345_INT_WATERMARK, int_stat)) {

[PATCH v1 08/12] iio: accel: adxl345: add double tap feature

[Lothar Rubusch]

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

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 7831ec511941..f9e5f47ba303 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -129,6 +129,29 @@ enum adxl345_axis {
 	ADXL345_TAP_SUPPRESS = BIT(3),
 };
 
+/* single/double tap */
+enum adxl345_tap_type {
+	ADXL345_SINGLE_TAP,
+	ADXL345_DOUBLE_TAP
+};
+
+static const unsigned int adxl345_tap_int_reg[2] = {
+	[ADXL345_SINGLE_TAP] = ADXL345_INT_SINGLE_TAP,
+	[ADXL345_DOUBLE_TAP] = ADXL345_INT_DOUBLE_TAP,
+};
+
+enum adxl345_tap_time_type {
+	ADXL345_TAP_TIME_LATENT,
+	ADXL345_TAP_TIME_WINDOW,
+	ADXL345_TAP_TIME_DUR
+};
+
+static const unsigned int adxl345_tap_time_reg[3] = {
+	[ADXL345_TAP_TIME_LATENT] = ADXL345_REG_LATENT,
+	[ADXL345_TAP_TIME_WINDOW] = ADXL345_REG_WINDOW,
+	[ADXL345_TAP_TIME_DUR] = ADXL345_REG_DUR,
+};
+
 struct adxl345_state {
 	const struct adxl345_chip_info *info;
 	struct regmap *regmap;
@@ -142,6 +165,8 @@ struct adxl345_state {
 	u32 tap_axis_ctrl;
 	u8 tap_threshold;
 	u32 tap_duration_us;
+	u32 tap_latent_us;
+	u32 tap_window_us;
 
 	__le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE + 1] __aligned(IIO_DMA_MINALIGN);
 };
@@ -154,6 +179,12 @@ static struct iio_event_spec adxl345_events[] = {
 		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
 		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE),
 	},
+	{
+		/* double tap */
+		.type = IIO_EV_TYPE_GESTURE,
+		.dir = IIO_EV_DIR_DOUBLETAP,
+		.mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE),
+	},
 };
 
 #define ADXL345_CHANNEL(index, reg, axis) {					\
@@ -250,10 +281,12 @@ static int adxl345_write_tap_axis(struct adxl345_state *st,
 					     st->tap_axis_ctrl));
 }
 
-static int _adxl345_set_tap_int(struct adxl345_state *st, bool state)
+static int _adxl345_set_tap_int(struct adxl345_state *st,
+				enum adxl345_tap_type type, bool state)
 {
 	bool axis_valid;
 	bool singletap_args_valid = false;
+	bool doubletap_args_valid = false;
 	bool en = false;
 
 	axis_valid = FIELD_GET(ADXL345_REG_TAP_AXIS_MSK, st->tap_axis_ctrl) > 0;
@@ -264,14 +297,24 @@ static int _adxl345_set_tap_int(struct adxl345_state *st, bool state)
 	 */
 	singletap_args_valid = st->tap_threshold > 0 && st->tap_duration_us > 0;
 
-	en = axis_valid && singletap_args_valid;
+	if (type == ADXL345_SINGLE_TAP) {
+		en = axis_valid && singletap_args_valid;
+	} else {
+		/* doubletap: Window must be equal or greater than latent! */
+		doubletap_args_valid = st->tap_latent_us > 0 &&
+			st->tap_window_us > 0 &&
+			st->tap_window_us >= st->tap_latent_us;
+
+		en = axis_valid && singletap_args_valid && doubletap_args_valid;
+	}
 
-	adxl345_intmap_switch_bit(st, state && en, ADXL345_INT_SINGLE_TAP);
+	adxl345_intmap_switch_bit(st, state && en, adxl345_tap_int_reg[type]);
 
 	return adxl345_write_interrupts(st);
 }
 
-static int adxl345_is_tap_en(struct adxl345_state *st, bool *en)
+static int adxl345_is_tap_en(struct adxl345_state *st,
+			     enum adxl345_tap_type type, bool *en)
 {
 	int ret;
 	unsigned int regval;
@@ -280,7 +323,8 @@ static int adxl345_is_tap_en(struct adxl345_state *st, bool *en)
 	if (ret)
 		return ret;
 
-	*en = FIELD_GET(ADXL345_INT_SINGLE_TAP, regval) > 0;
+	// TODO FIELD_GET() seems not possible here due to construct 'not const', any ideas?
+	*en = (adxl345_tap_int_reg[type] & regval) > 0;
 
 	return 0;
 }
@@ -294,7 +338,12 @@ static int adxl345_set_singletap_en(struct adxl345_state *st,
 	if (ret)
 		return ret;
 
-	return _adxl345_set_tap_int(st, en);
+	return _adxl345_set_tap_int(st, ADXL345_SINGLE_TAP, en);
+}
+
+static int adxl345_set_doubletap_en(struct adxl345_state *st, bool en)
+{
+	return _adxl345_set_tap_int(st, ADXL345_DOUBLE_TAP, en);
 }
 
 static int adxl345_set_tap_value(struct adxl345_state *st, u8 val)
@@ -304,19 +353,33 @@ static int adxl345_set_tap_value(struct adxl345_state *st, u8 val)
 	return regmap_write(st->regmap, ADXL345_REG_THRESH_TAP, min(val, 0xFF));
 }
 
-static int _adxl345_set_tap_time(struct adxl345_state *st, u32 val_us)
+static int _adxl345_set_tap_time(struct adxl345_state *st,
+				 enum adxl345_tap_time_type type, u32 val_us)
 {
 	unsigned int regval;
 
-	st->tap_duration_us = val_us;
+	switch (type) {
+	case ADXL345_TAP_TIME_WINDOW:
+		st->tap_window_us = val_us;
+		break;
+	case ADXL345_TAP_TIME_LATENT:
+		st->tap_latent_us = val_us;
+		break;
+	case ADXL345_TAP_TIME_DUR:
+		st->tap_duration_us = val_us;
+		break;
+	}
 
 	/*
 	 * The scale factor is 1250us / LSB for tap_window_us and tap_latent_us.
 	 * For tap_duration_us the scale factor is 625us / LSB.
 	 */
-	regval = DIV_ROUND_CLOSEST(val_us, 625);
+	if (type == ADXL345_TAP_TIME_DUR)
+		regval = DIV_ROUND_CLOSEST(val_us, 625);
+	else
+		regval = DIV_ROUND_CLOSEST(val_us, 1250);
 
-	return regmap_write(st->regmap, ADXL345_REG_DUR, regval);
+	return regmap_write(st->regmap, adxl345_tap_time_reg[type], regval);
 }
 
 static int adxl345_set_tap_duration(struct adxl345_state *st, u32 val_int,
@@ -330,7 +393,35 @@ static int adxl345_set_tap_duration(struct adxl345_state *st, u32 val_int,
 	if (val_int || val_fract_us > 159375)
 		return -EINVAL;
 
-	return _adxl345_set_tap_time(st, val_fract_us);
+	return _adxl345_set_tap_time(st, ADXL345_TAP_TIME_DUR, val_fract_us);
+}
+
+static int adxl345_set_tap_window(struct adxl345_state *st, u32 val_int,
+				  u32 val_fract_us)
+{
+	/*
+	 * Max value is 255 * 1250 us = 0.318750 seconds
+	 *
+	 * Note: the scaling is similar to the scaling in the ADXL380
+	 */
+	if (val_int || val_fract_us > 318750)
+		return -EINVAL;
+
+	return _adxl345_set_tap_time(st, ADXL345_TAP_TIME_WINDOW, val_fract_us);
+}
+
+static int adxl345_set_tap_latent(struct adxl345_state *st, u32 val_int,
+				  u32 val_fract_us)
+{
+	/*
+	 * Max value is 255 * 1250 us = 0.318750 seconds
+	 *
+	 * Note: the scaling is similar to the scaling in the ADXL380
+	 */
+	if (val_int || val_fract_us > 318750)
+		return -EINVAL;
+
+	return _adxl345_set_tap_time(st, ADXL345_TAP_TIME_LATENT, val_fract_us);
 }
 
 static int adxl345_read_raw(struct iio_dev *indio_dev,
@@ -448,10 +539,15 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
 				return -EINVAL;
 			}
 
-			ret = adxl345_is_tap_en(st, &int_en);
+			ret = adxl345_is_tap_en(st, ADXL345_SINGLE_TAP, &int_en);
 			if (ret)
 				return ret;
 			return int_en && axis_en;
+		case IIO_EV_DIR_DOUBLETAP:
+			ret = adxl345_is_tap_en(st, ADXL345_DOUBLE_TAP, &int_en);
+			if (ret)
+				return ret;
+			return int_en;
 		default:
 			return -EINVAL;
 		}
@@ -491,6 +587,8 @@ static int adxl345_write_event_config(struct iio_dev *indio_dev,
 		}
 
 		return adxl345_set_singletap_en(st, axis, state);
+	case IIO_EV_DIR_DOUBLETAP:
+		return adxl345_set_doubletap_en(st, state);
 	default:
 		return -EINVAL;
 	}
@@ -637,9 +735,13 @@ static int adxl345_write_raw_get_fmt(struct iio_dev *indio_dev,
 	static IIO_DEVICE_ATTR_RW(in_accel_##A##_##C##_##E, 0)
 
 ADXL345_generate_iio_dev_attr_FRACTIONAL(gesture_singletap, tap, duration, MICRO, us);
+ADXL345_generate_iio_dev_attr_FRACTIONAL(gesture_doubletap, tap, window, MICRO, us);
+ADXL345_generate_iio_dev_attr_FRACTIONAL(gesture_doubletap, tap, latent, MICRO, us);
 
 static struct attribute *adxl345_event_attrs[] = {
 	&iio_dev_attr_in_accel_gesture_singletap_duration_us.dev_attr.attr,
+	&iio_dev_attr_in_accel_gesture_doubletap_latent_us.dev_attr.attr,
+	&iio_dev_attr_in_accel_gesture_doubletap_window_us.dev_attr.attr,
 	NULL
 };
 
@@ -861,6 +963,17 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
 			return ret;
 	}
 
+	if (FIELD_GET(ADXL345_INT_DOUBLE_TAP, int_stat)) {
+		ret = iio_push_event(indio_dev,
+				     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
+							act_tap_dir,
+							IIO_EV_TYPE_GESTURE,
+							IIO_EV_DIR_DOUBLETAP),
+				     ts);
+		if (ret)
+			return ret;
+	}
+
 	return -ENOENT;
 }
 
@@ -965,6 +1078,8 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 	/* Init with reasonable values */
 	st->tap_threshold = 35;			/*   35 [0x23]            */
 	st->tap_duration_us = 3;		/*    3 [0x03] -> .001875 */
+	st->tap_window_us = 20;			/*   20 [0x14] -> .025    */
+	st->tap_latent_us = 20;			/*   20 [0x14] -> .025    */
 
 	indio_dev->name = st->info->name;
 	indio_dev->info = &adxl345_info;
-- 
2.39.5

Re: [PATCH v1 08/12] iio: accel: adxl345: add double tap feature

[Jonathan Cameron]

On Tue, 28 Jan 2025 12:00:56 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add the double tap feature of the sensor. The interrupt handler needs
> to catch and forwared the event to the IIO channel. The single tap
> implementation now is extended to deal with double tap as well.
As with earlier patch, new ABI must be documented.  Also we need
a strong argument why the existing ABI is not suitable.

Tap detection algorithms are annoyingly specific to manufacturers
but we did make an effort to come up with a general description a
while back so I'd hope perhaps we can map to that.

> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl345_core.c | 139 ++++++++++++++++++++++++++++---
>  1 file changed, 127 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 7831ec511941..f9e5f47ba303 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -129,6 +129,29 @@ enum adxl345_axis {
>  	ADXL345_TAP_SUPPRESS = BIT(3),
>  };
>  
> +/* single/double tap */
> +enum adxl345_tap_type {
> +	ADXL345_SINGLE_TAP,
> +	ADXL345_DOUBLE_TAP
trailing comma. Maybe there will be a triple tap sometime.
Anyhow, other than 'terminating' entries we should always have trailing commas.

> +};
> +
> +static const unsigned int adxl345_tap_int_reg[2] = {
> +	[ADXL345_SINGLE_TAP] = ADXL345_INT_SINGLE_TAP,
> +	[ADXL345_DOUBLE_TAP] = ADXL345_INT_DOUBLE_TAP,
> +};
> +
> +enum adxl345_tap_time_type {
> +	ADXL345_TAP_TIME_LATENT,
> +	ADXL345_TAP_TIME_WINDOW,
> +	ADXL345_TAP_TIME_DUR
Comma.

> +};
> +
> +static const unsigned int adxl345_tap_time_reg[3] = {
> +	[ADXL345_TAP_TIME_LATENT] = ADXL345_REG_LATENT,
> +	[ADXL345_TAP_TIME_WINDOW] = ADXL345_REG_WINDOW,
> +	[ADXL345_TAP_TIME_DUR] = ADXL345_REG_DUR,
> +};
> +
>  struct adxl345_state {
>  	const struct adxl345_chip_info *info;
>  	struct regmap *regmap;
> @@ -142,6 +165,8 @@ struct adxl345_state {
>  	u32 tap_axis_ctrl;
>  	u8 tap_threshold;
>  	u32 tap_duration_us;
> +	u32 tap_latent_us;
> +	u32 tap_window_us;
>  
>  	__le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE + 1] __aligned(IIO_DMA_MINALIGN);
>  };
> @@ -154,6 +179,12 @@ static struct iio_event_spec adxl345_events[] = {
>  		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
>  		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE),
>  	},
> +	{
> +		/* double tap */
> +		.type = IIO_EV_TYPE_GESTURE,
> +		.dir = IIO_EV_DIR_DOUBLETAP,
> +		.mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE),
> +	},
>  };
>  
>  #define ADXL345_CHANNEL(index, reg, axis) {					\
> @@ -250,10 +281,12 @@ static int adxl345_write_tap_axis(struct adxl345_state *st,
>  					     st->tap_axis_ctrl));
>  }
>  

> -static int adxl345_is_tap_en(struct adxl345_state *st, bool *en)
> +static int adxl345_is_tap_en(struct adxl345_state *st,
> +			     enum adxl345_tap_type type, bool *en)
>  {
>  	int ret;
>  	unsigned int regval;
> @@ -280,7 +323,8 @@ static int adxl345_is_tap_en(struct adxl345_state *st, bool *en)
>  	if (ret)
>  		return ret;
>  
> -	*en = FIELD_GET(ADXL345_INT_SINGLE_TAP, regval) > 0;
> +	// TODO FIELD_GET() seems not possible here due to construct 'not const', any ideas?

There is a new function to solve this proposed on list, but not upstream yet.
For now roll your own as you have done.


> +	*en = (adxl345_tap_int_reg[type] & regval) > 0;
>  
>  	return 0;
>  }
> @@ -294,7 +338,12 @@ static int adxl345_set_singletap_en(struct adxl345_state *st,
>  	if (ret)
>  		return ret;
>  
> -	return _adxl345_set_tap_int(st, en);
> +	return _adxl345_set_tap_int(st, ADXL345_SINGLE_TAP, en);
> +}
> +
> +static int adxl345_set_doubletap_en(struct adxl345_state *st, bool en)
> +{
> +	return _adxl345_set_tap_int(st, ADXL345_DOUBLE_TAP, en);
>  }
>  
>  static int adxl345_set_tap_value(struct adxl345_state *st, u8 val)
> @@ -304,19 +353,33 @@ static int adxl345_set_tap_value(struct adxl345_state *st, u8 val)
>  	return regmap_write(st->regmap, ADXL345_REG_THRESH_TAP, min(val, 0xFF));
>  }
>  
> -static int _adxl345_set_tap_time(struct adxl345_state *st, u32 val_us)
> +static int _adxl345_set_tap_time(struct adxl345_state *st,
> +				 enum adxl345_tap_time_type type, u32 val_us)
>  {
>  	unsigned int regval;
>  
> -	st->tap_duration_us = val_us;
> +	switch (type) {
> +	case ADXL345_TAP_TIME_WINDOW:
> +		st->tap_window_us = val_us;
> +		break;
> +	case ADXL345_TAP_TIME_LATENT:
> +		st->tap_latent_us = val_us;
> +		break;
> +	case ADXL345_TAP_TIME_DUR:
> +		st->tap_duration_us = val_us;
> +		break;
> +	}
>  
>  	/*
>  	 * The scale factor is 1250us / LSB for tap_window_us and tap_latent_us.
>  	 * For tap_duration_us the scale factor is 625us / LSB.
>  	 */
> -	regval = DIV_ROUND_CLOSEST(val_us, 625);
> +	if (type == ADXL345_TAP_TIME_DUR)
> +		regval = DIV_ROUND_CLOSEST(val_us, 625);
> +	else
> +		regval = DIV_ROUND_CLOSEST(val_us, 1250);
>  
> -	return regmap_write(st->regmap, ADXL345_REG_DUR, regval);
> +	return regmap_write(st->regmap, adxl345_tap_time_reg[type], regval);
Use this in the earlier patch, even though it would only have one entry at that point.
There is little point in introducing infrastructure that rewrites a patch
just after that patch.  Just put it in the first one with a note
saying it is useful shortly.

>  }
>  
>  static int adxl345_set_tap_duration(struct adxl345_state *st, u32 val_int,
> @@ -330,7 +393,35 @@ static int adxl345_set_tap_duration(struct adxl345_state *st, u32 val_int,
>  	if (val_int || val_fract_us > 159375)
>  		return -EINVAL;
>  
> -	return _adxl345_set_tap_time(st, val_fract_us);
> +	return _adxl345_set_tap_time(st, ADXL345_TAP_TIME_DUR, val_fract_us);
> +}

> @@ -637,9 +735,13 @@ static int adxl345_write_raw_get_fmt(struct iio_dev *indio_dev,
>  	static IIO_DEVICE_ATTR_RW(in_accel_##A##_##C##_##E, 0)
>  
>  ADXL345_generate_iio_dev_attr_FRACTIONAL(gesture_singletap, tap, duration, MICRO, us);
> +ADXL345_generate_iio_dev_attr_FRACTIONAL(gesture_doubletap, tap, window, MICRO, us);
> +ADXL345_generate_iio_dev_attr_FRACTIONAL(gesture_doubletap, tap, latent, MICRO, us);
>  
>  static struct attribute *adxl345_event_attrs[] = {
>  	&iio_dev_attr_in_accel_gesture_singletap_duration_us.dev_attr.attr,
> +	&iio_dev_attr_in_accel_gesture_doubletap_latent_us.dev_attr.attr,
> +	&iio_dev_attr_in_accel_gesture_doubletap_window_us.dev_attr.attr,

As above.  ABI docs missing and times in IIO are all seconds.  us to seconds is easy
given you just use the formatting functions and pass val == 0.

>  	NULL
>  };
>  
> @@ -861,6 +963,17 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
>  			return ret;
>  	}
>  
> +	if (FIELD_GET(ADXL345_INT_DOUBLE_TAP, int_stat)) {
> +		ret = iio_push_event(indio_dev,
> +				     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> +							act_tap_dir,
> +							IIO_EV_TYPE_GESTURE,
> +							IIO_EV_DIR_DOUBLETAP),
> +				     ts);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return -ENOENT;
>  }
>  
> @@ -965,6 +1078,8 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  	/* Init with reasonable values */
>  	st->tap_threshold = 35;			/*   35 [0x23]            */
>  	st->tap_duration_us = 3;		/*    3 [0x03] -> .001875 */
> +	st->tap_window_us = 20;			/*   20 [0x14] -> .025    */
> +	st->tap_latent_us = 20;			/*   20 [0x14] -> .025    */
>  
>  	indio_dev->name = st->info->name;
>  	indio_dev->info = &adxl345_info;

[PATCH v1 09/12] iio: accel: adxl345: add double tap suppress bit

[Lothar Rubusch]

Add the suppress bit feature to make double tap (in)sensitive to the
configured threshold value for the tap feature. The feature is being
enabled by a sysfs handle for enabling. This is also needed for further
features in follow up patches.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index f9e5f47ba303..ef0a12fd59be 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -120,6 +120,7 @@
 #define ADXL345_INT2			1
 
 #define ADXL345_REG_TAP_AXIS_MSK	GENMASK(2, 0)
+#define ADXL345_REG_TAP_SUPPRESS_MSK	BIT(3)
 
 enum adxl345_axis {
 	ADXL345_Z_EN = BIT(0),
@@ -167,6 +168,7 @@ struct adxl345_state {
 	u32 tap_duration_us;
 	u32 tap_latent_us;
 	u32 tap_window_us;
+	bool tap_suppressed;
 
 	__le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE + 1] __aligned(IIO_DMA_MINALIGN);
 };
@@ -346,6 +348,22 @@ static int adxl345_set_doubletap_en(struct adxl345_state *st, bool en)
 	return _adxl345_set_tap_int(st, ADXL345_DOUBLE_TAP, en);
 }
 
+static int adxl345_is_suppressed_en(struct adxl345_state *st, bool *en)
+{
+	*en = st->tap_suppressed;
+
+	return 0;
+}
+
+static int adxl345_set_suppressed_en(struct adxl345_state *st, bool en)
+{
+	st->tap_suppressed = en;
+
+	return regmap_update_bits(st->regmap, ADXL345_REG_TAP_AXIS,
+				  ADXL345_REG_TAP_SUPPRESS_MSK,
+				  en ? ADXL345_TAP_SUPPRESS : ~ADXL345_TAP_SUPPRESS);
+}
+
 static int adxl345_set_tap_value(struct adxl345_state *st, u8 val)
 {
 	st->tap_threshold = val;
@@ -693,6 +711,52 @@ static int adxl345_write_raw_get_fmt(struct iio_dev *indio_dev,
 	}
 }
 
+#define ADXL345_generate_iio_dev_attr_EN(A, B)				\
+	static ssize_t in_accel_##A##_##B##_en_show(struct device *dev, \
+						    struct device_attribute *attr, \
+						    char *buf)		\
+	{								\
+		struct iio_dev *indio_dev = dev_to_iio_dev(dev);	\
+		struct adxl345_state *st = iio_priv(indio_dev);		\
+		bool en;						\
+		int val, ret;						\
+									\
+		ret = adxl345_is_##B##_en(st, &en);			\
+		if (ret)						\
+			return ret;					\
+		val = en ? 1 : 0;					\
+									\
+		return iio_format_value(buf, IIO_VAL_INT, 1, &val);	\
+	}								\
+									\
+	static ssize_t in_accel_##A##_##B##_en_store(struct device *dev, \
+						     struct device_attribute *attr, \
+						     const char *buf, size_t len) \
+	{								\
+		struct iio_dev *indio_dev = dev_to_iio_dev(dev);	\
+		struct adxl345_state *st = iio_priv(indio_dev);		\
+		int val, ret;						\
+									\
+		ret = kstrtoint(buf, 0, &val);				\
+		if (ret)						\
+			return ret;					\
+									\
+		ret = adxl345_set_measure_en(st, false);		\
+		if (ret)						\
+			return ret;					\
+									\
+		ret = adxl345_set_##B##_en(st, val > 0);		\
+		if (ret)						\
+			return ret;					\
+									\
+		ret =  adxl345_set_measure_en(st, true);		\
+		if (ret)						\
+			return ret;					\
+									\
+		return len;						\
+	}								\
+	static IIO_DEVICE_ATTR_RW(in_accel_##A##_##B##_en, 0)
+
 #define ADXL345_generate_iio_dev_attr_FRACTIONAL(A, B, C, D, E)		\
 	static ssize_t in_accel_##A##_##C##_##E##_show(struct device *dev, \
 						       struct device_attribute *attr, \
@@ -738,8 +802,11 @@ ADXL345_generate_iio_dev_attr_FRACTIONAL(gesture_singletap, tap, duration, MICRO
 ADXL345_generate_iio_dev_attr_FRACTIONAL(gesture_doubletap, tap, window, MICRO, us);
 ADXL345_generate_iio_dev_attr_FRACTIONAL(gesture_doubletap, tap, latent, MICRO, us);
 
+ADXL345_generate_iio_dev_attr_EN(gesture_doubletap, suppressed);
+
 static struct attribute *adxl345_event_attrs[] = {
 	&iio_dev_attr_in_accel_gesture_singletap_duration_us.dev_attr.attr,
+	&iio_dev_attr_in_accel_gesture_doubletap_suppressed_en.dev_attr.attr,
 	&iio_dev_attr_in_accel_gesture_doubletap_latent_us.dev_attr.attr,
 	&iio_dev_attr_in_accel_gesture_doubletap_window_us.dev_attr.attr,
 	NULL
-- 
2.39.5

Re: [PATCH v1 09/12] iio: accel: adxl345: add double tap suppress bit

[Jonathan Cameron]

On Tue, 28 Jan 2025 12:00:57 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add the suppress bit feature to make double tap (in)sensitive to the
> configured threshold value for the tap feature. The feature is being
> enabled by a sysfs handle for enabling. This is also needed for further
> features in follow up patches.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>

Why would you change this?  From that description it sounds like this
should always be enabled or disabled (I'm not entirely sure). Not
something I'd expect a user to actually tweak.

Anyhow, new ABI. docs missing so hard to know what this is.

> ---
>  drivers/iio/accel/adxl345_core.c | 67 ++++++++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index f9e5f47ba303..ef0a12fd59be 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -120,6 +120,7 @@
>  #define ADXL345_INT2			1
>  
>  #define ADXL345_REG_TAP_AXIS_MSK	GENMASK(2, 0)
> +#define ADXL345_REG_TAP_SUPPRESS_MSK	BIT(3)
>  
>  enum adxl345_axis {
>  	ADXL345_Z_EN = BIT(0),
> @@ -167,6 +168,7 @@ struct adxl345_state {
>  	u32 tap_duration_us;
>  	u32 tap_latent_us;
>  	u32 tap_window_us;
> +	bool tap_suppressed;
>  
>  	__le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE + 1] __aligned(IIO_DMA_MINALIGN);
>  };
> @@ -346,6 +348,22 @@ static int adxl345_set_doubletap_en(struct adxl345_state *st, bool en)
>  	return _adxl345_set_tap_int(st, ADXL345_DOUBLE_TAP, en);
>  }
>  
> +static int adxl345_is_suppressed_en(struct adxl345_state *st, bool *en)
> +{
> +	*en = st->tap_suppressed;
> +
> +	return 0;
> +}
> +
> +static int adxl345_set_suppressed_en(struct adxl345_state *st, bool en)
> +{
> +	st->tap_suppressed = en;
> +
> +	return regmap_update_bits(st->regmap, ADXL345_REG_TAP_AXIS,
> +				  ADXL345_REG_TAP_SUPPRESS_MSK,
> +				  en ? ADXL345_TAP_SUPPRESS : ~ADXL345_TAP_SUPPRESS);
> +}
> +
>  static int adxl345_set_tap_value(struct adxl345_state *st, u8 val)
>  {
>  	st->tap_threshold = val;
> @@ -693,6 +711,52 @@ static int adxl345_write_raw_get_fmt(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +#define ADXL345_generate_iio_dev_attr_EN(A, B)				\
> +	static ssize_t in_accel_##A##_##B##_en_show(struct device *dev, \
> +						    struct device_attribute *attr, \
> +						    char *buf)		\
> +	{								\
> +		struct iio_dev *indio_dev = dev_to_iio_dev(dev);	\
> +		struct adxl345_state *st = iio_priv(indio_dev);		\
> +		bool en;						\
> +		int val, ret;						\
> +									\
> +		ret = adxl345_is_##B##_en(st, &en);			\
> +		if (ret)						\
> +			return ret;					\
> +		val = en ? 1 : 0;					\
> +									\
> +		return iio_format_value(buf, IIO_VAL_INT, 1, &val);	\
> +	}								\
> +									\
> +	static ssize_t in_accel_##A##_##B##_en_store(struct device *dev, \
> +						     struct device_attribute *attr, \
> +						     const char *buf, size_t len) \
> +	{								\
> +		struct iio_dev *indio_dev = dev_to_iio_dev(dev);	\
> +		struct adxl345_state *st = iio_priv(indio_dev);		\
> +		int val, ret;						\
> +									\
> +		ret = kstrtoint(buf, 0, &val);				\
> +		if (ret)						\
> +			return ret;					\
> +									\
> +		ret = adxl345_set_measure_en(st, false);		\
> +		if (ret)						\
> +			return ret;					\
> +									\
> +		ret = adxl345_set_##B##_en(st, val > 0);		\
> +		if (ret)						\
> +			return ret;					\
> +									\
> +		ret =  adxl345_set_measure_en(st, true);		\
> +		if (ret)						\
> +			return ret;					\
> +									\
> +		return len;						\
> +	}								\
> +	static IIO_DEVICE_ATTR_RW(in_accel_##A##_##B##_en, 0)
> +
>  #define ADXL345_generate_iio_dev_attr_FRACTIONAL(A, B, C, D, E)		\
>  	static ssize_t in_accel_##A##_##C##_##E##_show(struct device *dev, \
>  						       struct device_attribute *attr, \
> @@ -738,8 +802,11 @@ ADXL345_generate_iio_dev_attr_FRACTIONAL(gesture_singletap, tap, duration, MICRO
>  ADXL345_generate_iio_dev_attr_FRACTIONAL(gesture_doubletap, tap, window, MICRO, us);
>  ADXL345_generate_iio_dev_attr_FRACTIONAL(gesture_doubletap, tap, latent, MICRO, us);
>  
> +ADXL345_generate_iio_dev_attr_EN(gesture_doubletap, suppressed);
> +
>  static struct attribute *adxl345_event_attrs[] = {
>  	&iio_dev_attr_in_accel_gesture_singletap_duration_us.dev_attr.attr,
> +	&iio_dev_attr_in_accel_gesture_doubletap_suppressed_en.dev_attr.attr,
>  	&iio_dev_attr_in_accel_gesture_doubletap_latent_us.dev_attr.attr,
>  	&iio_dev_attr_in_accel_gesture_doubletap_window_us.dev_attr.attr,
>  	NULL

[PATCH v1 10/12] iio: accel: adxl345: add freefall feature

[Lothar Rubusch]

Add the freefall detection of the sensor together with a threshold and
time parameter. Add sysfs handle to enable/disable the feature and
introduce another sysfs macro. This is the third sysfs macro for sysfs
handles of this sensor. The three sysfs macros allow to cover all
sensor features and parameters.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index ef0a12fd59be..62d75d28b6fc 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -170,6 +170,9 @@ struct adxl345_state {
 	u32 tap_window_us;
 	bool tap_suppressed;
 
+	u8 ff_value;
+	u32 ff_time_ms;
+
 	__le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE + 1] __aligned(IIO_DMA_MINALIGN);
 };
 
@@ -187,6 +190,11 @@ static struct iio_event_spec adxl345_events[] = {
 		.dir = IIO_EV_DIR_DOUBLETAP,
 		.mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE),
 	},
+	{
+		/* free fall */
+		.type = IIO_EV_TYPE_MAG,
+		.dir = IIO_EV_DIR_FALLING,
+	},
 };
 
 #define ADXL345_CHANNEL(index, reg, axis) {					\
@@ -442,6 +450,61 @@ static int adxl345_set_tap_latent(struct adxl345_state *st, u32 val_int,
 	return _adxl345_set_tap_time(st, ADXL345_TAP_TIME_LATENT, val_fract_us);
 }
 
+/* ff */
+
+static int adxl345_is_ff_en(struct adxl345_state *st, bool *en)
+{
+	int ret;
+	unsigned int regval;
+
+	ret = adxl345_read_interrupts(st, &regval);
+	if (ret)
+		return ret;
+
+	*en = FIELD_GET(ADXL345_INT_FREE_FALL, st->int_map) > 0;
+
+	return 0;
+}
+
+static int adxl345_set_ff_en(struct adxl345_state *st, bool en)
+{
+	bool ff_en = en && st->ff_value > 0 && st->ff_time_ms > 0;
+
+	adxl345_intmap_switch_bit(st, ff_en, ADXL345_INT_FREE_FALL);
+
+	return adxl345_write_interrupts(st);
+}
+
+static int adxl345_set_ff_value(struct adxl345_state *st, u8 val)
+{
+	st->ff_value = val;
+
+	return regmap_write(st->regmap, ADXL345_REG_THRESH_FF, val);
+}
+
+static int adxl345_set_ff_time(struct adxl345_state *st, u32 val_int,
+			       u32 val_fract_us)
+{
+	unsigned int regval;
+	int val_ms;
+
+	/*
+	 * max value is 255 * 5000 us = 1.275000 seconds
+	 *
+	 * Note: the scaling is similar to the scaling in the ADXL380
+	 */
+	if (1000000 * val_int + val_fract_us > 1275000)
+		return -EINVAL;
+
+	val_ms = val_int * 1000 + DIV_ROUND_UP(val_fract_us, 1000);
+	st->ff_time_ms = val_ms;
+
+	regval = DIV_ROUND_CLOSEST(val_ms, 5);
+
+	/* Values between 100ms and 350ms (0x14 to 0x46) are recommended. */
+	return regmap_write(st->regmap, ADXL345_REG_TIME_FF, min(regval, 0xff));
+}
+
 static int adxl345_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan,
 			    int *val, int *val2, long mask)
@@ -711,6 +774,49 @@ static int adxl345_write_raw_get_fmt(struct iio_dev *indio_dev,
 	}
 }
 
+#define ADXL345_generate_iio_dev_attr_INT(A, B, C)			\
+	static ssize_t in_accel_##A##_##B##_##C##_show(struct device *dev, \
+						       struct device_attribute *attr, \
+						       char *buf)	\
+	{								\
+		struct iio_dev *indio_dev = dev_to_iio_dev(dev);	\
+		struct adxl345_state *st = iio_priv(indio_dev);		\
+		int val;						\
+									\
+		val = st->B##_##C;					\
+									\
+		return iio_format_value(buf, IIO_VAL_INT, 1, &val);	\
+	}								\
+									\
+	static ssize_t in_accel_##A##_##B##_##C##_store(struct device *dev, \
+							struct device_attribute *attr, \
+							const char *buf, size_t len) \
+	{								\
+		struct iio_dev *indio_dev = dev_to_iio_dev(dev);	\
+		struct adxl345_state *st = iio_priv(indio_dev);		\
+		int val, ret;						\
+									\
+		ret = kstrtoint(buf, 0, &val);				\
+		if (ret)						\
+			return ret;					\
+									\
+		if (val < 0 || val > 255)				\
+			return -EINVAL;					\
+									\
+		ret = adxl345_set_measure_en(st, false);		\
+		if (ret)						\
+			return ret;					\
+									\
+		adxl345_set_##B##_##C(st, val);				\
+									\
+		ret = adxl345_set_measure_en(st, true);			\
+		if (ret)						\
+			return ret;					\
+									\
+		return len;						\
+	}								\
+	static IIO_DEVICE_ATTR_RW(in_accel_##A##_##B##_##C, 0)
+
 #define ADXL345_generate_iio_dev_attr_EN(A, B)				\
 	static ssize_t in_accel_##A##_##B##_en_show(struct device *dev, \
 						    struct device_attribute *attr, \
@@ -798,13 +904,20 @@ static int adxl345_write_raw_get_fmt(struct iio_dev *indio_dev,
 	}								\
 	static IIO_DEVICE_ATTR_RW(in_accel_##A##_##C##_##E, 0)
 
+ADXL345_generate_iio_dev_attr_INT(freefall, ff, value);
+
 ADXL345_generate_iio_dev_attr_FRACTIONAL(gesture_singletap, tap, duration, MICRO, us);
 ADXL345_generate_iio_dev_attr_FRACTIONAL(gesture_doubletap, tap, window, MICRO, us);
 ADXL345_generate_iio_dev_attr_FRACTIONAL(gesture_doubletap, tap, latent, MICRO, us);
+ADXL345_generate_iio_dev_attr_FRACTIONAL(freefall, ff, time, MILLI, ms);
 
+ADXL345_generate_iio_dev_attr_EN(freefall, ff);
 ADXL345_generate_iio_dev_attr_EN(gesture_doubletap, suppressed);
 
 static struct attribute *adxl345_event_attrs[] = {
+	&iio_dev_attr_in_accel_freefall_ff_en.dev_attr.attr,
+	&iio_dev_attr_in_accel_freefall_ff_value.dev_attr.attr,
+	&iio_dev_attr_in_accel_freefall_time_ms.dev_attr.attr,
 	&iio_dev_attr_in_accel_gesture_singletap_duration_us.dev_attr.attr,
 	&iio_dev_attr_in_accel_gesture_doubletap_suppressed_en.dev_attr.attr,
 	&iio_dev_attr_in_accel_gesture_doubletap_latent_us.dev_attr.attr,
@@ -1041,6 +1154,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;
+	}
+
 	return -ENOENT;
 }
 
@@ -1148,6 +1272,9 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 	st->tap_window_us = 20;			/*   20 [0x14] -> .025    */
 	st->tap_latent_us = 20;			/*   20 [0x14] -> .025    */
 
+	st->ff_value = 8;			/*    8 [0x08]            */
+	st->ff_time_ms = 32;			/*   32 [0x20] -> 0.16    */
+
 	indio_dev->name = st->info->name;
 	indio_dev->info = &adxl345_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
-- 
2.39.5

Re: [PATCH v1 10/12] iio: accel: adxl345: add freefall feature

[Jonathan Cameron]

On Tue, 28 Jan 2025 12:00:58 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add the freefall detection of the sensor together with a threshold and
> time parameter. Add sysfs handle to enable/disable the feature and
> introduce another sysfs macro. This is the third sysfs macro for sysfs
> handles of this sensor. The three sysfs macros allow to cover all
> sensor features and parameters.

They might but they are new ABI, without docs and we have a lot of freefall
detectors supported (pretty much all accelerometers support this)
so needing new ABI is a surprise. Key thing as you've noted
on the type is free fall is just detection of approximately zero magnitude
acceleration on all axes at the same time.

That ABI is the way to do this, not something new.

Jonathan


> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl345_core.c | 127 +++++++++++++++++++++++++++++++
>  1 file changed, 127 insertions(+)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index ef0a12fd59be..62d75d28b6fc 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -170,6 +170,9 @@ struct adxl345_state {
>  	u32 tap_window_us;
>  	bool tap_suppressed;
>  
> +	u8 ff_value;
> +	u32 ff_time_ms;
> +
>  	__le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE + 1] __aligned(IIO_DMA_MINALIGN);
>  };
>  
> @@ -187,6 +190,11 @@ static struct iio_event_spec adxl345_events[] = {
>  		.dir = IIO_EV_DIR_DOUBLETAP,
>  		.mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE),
>  	},
> +	{
> +		/* free fall */
> +		.type = IIO_EV_TYPE_MAG,
> +		.dir = IIO_EV_DIR_FALLING,

This is correct, but you invent freefall attributes otherwise?
It should just use attributes of falling magnitude threshold detection.
Lots of existing precedence for this one.

> +	},
>  };
>  
>  #define ADXL345_CHANNEL(index, reg, axis) {					\
> @@ -442,6 +450,61 @@ static int adxl345_set_tap_latent(struct adxl345_state *st, u32 val_int,
>  	return _adxl345_set_tap_time(st, ADXL345_TAP_TIME_LATENT, val_fract_us);
>  }
>  
> +/* ff */
> +
> +static int adxl345_is_ff_en(struct adxl345_state *st, bool *en)
> +{
> +	int ret;
> +	unsigned int regval;
> +
> +	ret = adxl345_read_interrupts(st, &regval);
> +	if (ret)
> +		return ret;
> +
> +	*en = FIELD_GET(ADXL345_INT_FREE_FALL, st->int_map) > 0;
> +
> +	return 0;
> +}
> +
> +static int adxl345_set_ff_en(struct adxl345_state *st, bool en)
> +{
> +	bool ff_en = en && st->ff_value > 0 && st->ff_time_ms > 0;
> +
> +	adxl345_intmap_switch_bit(st, ff_en, ADXL345_INT_FREE_FALL);
> +
> +	return adxl345_write_interrupts(st);
> +}
> +
> +static int adxl345_set_ff_value(struct adxl345_state *st, u8 val)
> +{
> +	st->ff_value = val;
> +
> +	return regmap_write(st->regmap, ADXL345_REG_THRESH_FF, val);
This is just a magnitude threshold. Why not use standard ABI?

> +}
> +
> +static int adxl345_set_ff_time(struct adxl345_state *st, u32 val_int,
> +			       u32 val_fract_us)
I'm guessing this is time before free fall detection after falling
below a threshold?

Thats in_accel_mag_falling_period in standard ABI and IIRC is in seconds.


> +{
> +	unsigned int regval;
> +	int val_ms;
> +
> +	/*
> +	 * max value is 255 * 5000 us = 1.275000 seconds
> +	 *
> +	 * Note: the scaling is similar to the scaling in the ADXL380
> +	 */
> +	if (1000000 * val_int + val_fract_us > 1275000)
> +		return -EINVAL;
> +
> +	val_ms = val_int * 1000 + DIV_ROUND_UP(val_fract_us, 1000);
> +	st->ff_time_ms = val_ms;
> +
> +	regval = DIV_ROUND_CLOSEST(val_ms, 5);
> +
> +	/* Values between 100ms and 350ms (0x14 to 0x46) are recommended. */
> +	return regmap_write(st->regmap, ADXL345_REG_TIME_FF, min(regval, 0xff));
> +}
> +
>  static int adxl345_read_raw(struct iio_dev *indio_dev,
>  			    struct iio_chan_spec const *chan,
>  			    int *val, int *val2, long mask)
> @@ -711,6 +774,49 @@ static int adxl345_write_raw_get_fmt(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +#define ADXL345_generate_iio_dev_attr_INT(A, B, C)			\
> +	static ssize_t in_accel_##A##_##B##_##C##_show(struct device *dev, \
> +						       struct device_attribute *attr, \
> +						       char *buf)	\
> +	{								\
> +		struct iio_dev *indio_dev = dev_to_iio_dev(dev);	\
> +		struct adxl345_state *st = iio_priv(indio_dev);		\
> +		int val;						\
> +									\
> +		val = st->B##_##C;					\
> +									\
> +		return iio_format_value(buf, IIO_VAL_INT, 1, &val);	\
> +	}								\
> +									\
> +	static ssize_t in_accel_##A##_##B##_##C##_store(struct device *dev, \
> +							struct device_attribute *attr, \
> +							const char *buf, size_t len) \
> +	{								\
> +		struct iio_dev *indio_dev = dev_to_iio_dev(dev);	\
> +		struct adxl345_state *st = iio_priv(indio_dev);		\
> +		int val, ret;						\
> +									\
> +		ret = kstrtoint(buf, 0, &val);				\
> +		if (ret)						\
> +			return ret;					\
> +									\
> +		if (val < 0 || val > 255)				\
> +			return -EINVAL;					\
> +									\
> +		ret = adxl345_set_measure_en(st, false);		\
> +		if (ret)						\
> +			return ret;					\
> +									\
> +		adxl345_set_##B##_##C(st, val);				\
> +									\
> +		ret = adxl345_set_measure_en(st, true);			\
> +		if (ret)						\
> +			return ret;					\
> +									\
> +		return len;						\
> +	}								\
> +	static IIO_DEVICE_ATTR_RW(in_accel_##A##_##B##_##C, 0)
> +
>  #define ADXL345_generate_iio_dev_attr_EN(A, B)				\
>  	static ssize_t in_accel_##A##_##B##_en_show(struct device *dev, \
>  						    struct device_attribute *attr, \
> @@ -798,13 +904,20 @@ static int adxl345_write_raw_get_fmt(struct iio_dev *indio_dev,
>  	}								\
>  	static IIO_DEVICE_ATTR_RW(in_accel_##A##_##C##_##E, 0)
>  
> +ADXL345_generate_iio_dev_attr_INT(freefall, ff, value);
> +
>  ADXL345_generate_iio_dev_attr_FRACTIONAL(gesture_singletap, tap, duration, MICRO, us);
>  ADXL345_generate_iio_dev_attr_FRACTIONAL(gesture_doubletap, tap, window, MICRO, us);
>  ADXL345_generate_iio_dev_attr_FRACTIONAL(gesture_doubletap, tap, latent, MICRO, us);
> +ADXL345_generate_iio_dev_attr_FRACTIONAL(freefall, ff, time, MILLI, ms);
>  
> +ADXL345_generate_iio_dev_attr_EN(freefall, ff);
>  ADXL345_generate_iio_dev_attr_EN(gesture_doubletap, suppressed);
>  
>  static struct attribute *adxl345_event_attrs[] = {
> +	&iio_dev_attr_in_accel_freefall_ff_en.dev_attr.attr,
> +	&iio_dev_attr_in_accel_freefall_ff_value.dev_attr.attr,
> +	&iio_dev_attr_in_accel_freefall_time_ms.dev_attr.attr,
>  	&iio_dev_attr_in_accel_gesture_singletap_duration_us.dev_attr.attr,
>  	&iio_dev_attr_in_accel_gesture_doubletap_suppressed_en.dev_attr.attr,
>  	&iio_dev_attr_in_accel_gesture_doubletap_latent_us.dev_attr.attr,
> @@ -1041,6 +1154,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;
> +	}
> +
>  	return -ENOENT;
>  }
>  
> @@ -1148,6 +1272,9 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  	st->tap_window_us = 20;			/*   20 [0x14] -> .025    */
>  	st->tap_latent_us = 20;			/*   20 [0x14] -> .025    */
>  
> +	st->ff_value = 8;			/*    8 [0x08]            */
> +	st->ff_time_ms = 32;			/*   32 [0x20] -> 0.16    */
> +
>  	indio_dev->name = st->info->name;
>  	indio_dev->info = &adxl345_info;
>  	indio_dev->modes = INDIO_DIRECT_MODE;

[PATCH v1 11/12] iio: accel: adxl345: add activity feature

[Lothar Rubusch]

Add the handling of activity events, also add sysfs entries to
configure threshold values to trigger the event. Allow to push the
event over to the iio channel.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 62d75d28b6fc..94c3ad818ba5 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -121,6 +121,8 @@
 
 #define ADXL345_REG_TAP_AXIS_MSK	GENMASK(2, 0)
 #define ADXL345_REG_TAP_SUPPRESS_MSK	BIT(3)
+#define ADXL345_REG_ACT_AXIS_MSK	GENMASK(6, 4)
+#define ADXL345_REG_ACT_ACDC_MSK	BIT(7)
 
 enum adxl345_axis {
 	ADXL345_Z_EN = BIT(0),
@@ -163,6 +165,10 @@ struct adxl345_state {
 	u8 watermark;
 	u8 fifo_mode;
 
+	u32 act_axis_ctrl;
+	bool act_ac;
+	u8 act_value;
+
 	u32 tap_axis_ctrl;
 	u8 tap_threshold;
 	u32 tap_duration_us;
@@ -177,6 +183,11 @@ struct adxl345_state {
 };
 
 static struct iio_event_spec adxl345_events[] = {
+	{
+		/* activity */
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+	},
 	{
 		/* single tap */
 		.type = IIO_EV_TYPE_GESTURE,
@@ -276,6 +287,117 @@ static inline int adxl345_write_interrupts(struct adxl345_state *st)
 	return regmap_write(st->regmap, ADXL345_REG_INT_ENABLE, st->int_map);
 }
 
+/* act/inact */
+
+static int adxl345_write_act_axis(struct adxl345_state *st, bool en)
+{
+	int ret;
+
+	/*
+	 * A setting of 0 selects dc-coupled operation, and a setting of 1
+	 * enables ac-coupled operation. In dc-coupled operation, the current
+	 * acceleration magnitude is compared directly with THRESH_ACT and
+	 * 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
+	 * 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
+	 * THRESH_INACT. If the difference is less than the value in
+	 * THRESH_INACT for the time in TIME_INACT, the device is  considered
+	 * inactive and the inactivity interrupt is triggered.
+	 */
+	ret = regmap_update_bits(st->regmap, ADXL345_REG_ACT_INACT_CTRL,
+				 ADXL345_REG_ACT_ACDC_MSK, st->act_ac);
+	if (ret)
+		return ret;
+
+	/*
+	 * The ADXL345 allows for individually enabling/disabling axis for
+	 * activity and inactivity detection, respectively. Here both axis are
+	 * kept in sync, i.e. an axis will be generally enabled or disabled for
+	 * both equally, activity and inactivity detection.
+	 */
+	st->act_axis_ctrl = en
+		? st->act_axis_ctrl | ADXL345_REG_ACT_AXIS_MSK
+		: st->act_axis_ctrl & ~ADXL345_REG_ACT_AXIS_MSK;
+
+	ret = regmap_update_bits(st->regmap, ADXL345_REG_ACT_INACT_CTRL,
+				 ADXL345_REG_ACT_AXIS_MSK,
+				 st->act_axis_ctrl);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int adxl345_set_act_int(struct adxl345_state *st)
+{
+	bool args_valid;
+	bool axis_en;
+
+	axis_en = FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, st->act_axis_ctrl) > 0;
+	args_valid = axis_en && st->act_value > 0;
+	adxl345_intmap_switch_bit(st, args_valid, ADXL345_INT_ACTIVITY);
+
+	return adxl345_write_interrupts(st);
+}
+
+static int _adxl345_is_act_en(struct adxl345_state *st, bool *en)
+{
+	int ret;
+	unsigned int regval;
+
+	ret = adxl345_read_interrupts(st, &regval);
+	if (ret)
+		return ret;
+
+	*en = FIELD_GET(ADXL345_INT_ACTIVITY, regval) > 0;
+
+	return 0;
+}
+
+static int _adxl345_set_act_en(struct adxl345_state *st, bool en)
+{
+	int ret;
+
+	ret = adxl345_write_act_axis(st, en);
+	if (ret)
+		return ret;
+
+	return adxl345_set_act_int(st);
+}
+
+static int adxl345_is_act_en(struct adxl345_state *st, bool *en)
+{
+	return _adxl345_is_act_en(st, en);
+}
+
+static int adxl345_set_act_en(struct adxl345_state *st, bool en)
+{
+	return _adxl345_set_act_en(st, en);
+}
+
+static int _adxl345_set_act_value(struct adxl345_state *st, u8 val)
+{
+	st->act_value = val;
+
+	return regmap_write(st->regmap, ADXL345_REG_THRESH_ACT, val);
+}
+
+static int adxl345_set_act_value(struct adxl345_state *st, u8 val)
+{
+	return _adxl345_set_act_value(st, val);
+}
+
 /* tap */
 
 static int adxl345_write_tap_axis(struct adxl345_state *st,
@@ -904,6 +1026,7 @@ static int adxl345_write_raw_get_fmt(struct iio_dev *indio_dev,
 	}								\
 	static IIO_DEVICE_ATTR_RW(in_accel_##A##_##C##_##E, 0)
 
+ADXL345_generate_iio_dev_attr_INT(activity, act, value);
 ADXL345_generate_iio_dev_attr_INT(freefall, ff, value);
 
 ADXL345_generate_iio_dev_attr_FRACTIONAL(gesture_singletap, tap, duration, MICRO, us);
@@ -911,10 +1034,13 @@ ADXL345_generate_iio_dev_attr_FRACTIONAL(gesture_doubletap, tap, window, MICRO,
 ADXL345_generate_iio_dev_attr_FRACTIONAL(gesture_doubletap, tap, latent, MICRO, us);
 ADXL345_generate_iio_dev_attr_FRACTIONAL(freefall, ff, time, MILLI, ms);
 
+ADXL345_generate_iio_dev_attr_EN(activity, act);
 ADXL345_generate_iio_dev_attr_EN(freefall, ff);
 ADXL345_generate_iio_dev_attr_EN(gesture_doubletap, suppressed);
 
 static struct attribute *adxl345_event_attrs[] = {
+	&iio_dev_attr_in_accel_activity_act_en.dev_attr.attr,
+	&iio_dev_attr_in_accel_activity_act_value.dev_attr.attr,
 	&iio_dev_attr_in_accel_freefall_ff_en.dev_attr.attr,
 	&iio_dev_attr_in_accel_freefall_ff_value.dev_attr.attr,
 	&iio_dev_attr_in_accel_freefall_time_ms.dev_attr.attr,
@@ -1087,20 +1213,25 @@ static int adxl345_get_status(struct adxl345_state *st, unsigned int *int_stat,
 {
 	unsigned int regval;
 	bool check_tap_stat;
+	bool check_act_stat;
 
 	*act_tap_dir = IIO_NO_MOD;
 	check_tap_stat = FIELD_GET(ADXL345_REG_TAP_AXIS_MSK, st->tap_axis_ctrl) > 0;
+	check_act_stat = FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, st->act_axis_ctrl) > 0;
 
-	if (check_tap_stat) {
+	if (check_tap_stat || check_act_stat) {
 		/* ACT_TAP_STATUS should be read before clearing the interrupt */
 		if (regmap_read(st->regmap, ADXL345_REG_ACT_TAP_STATUS, &regval))
 			return -EINVAL;
 
-		if (FIELD_GET(ADXL345_Z_EN, regval) > 0)
+		if ((FIELD_GET(ADXL345_Z_EN, regval >> 4)
+				| FIELD_GET(ADXL345_Z_EN, regval)) > 0)
 			*act_tap_dir = IIO_MOD_Z;
-		else if (FIELD_GET(ADXL345_Y_EN, regval) > 0)
+		else if ((FIELD_GET(ADXL345_Y_EN, regval >> 4)
+				| FIELD_GET(ADXL345_Y_EN, regval)) > 0)
 			*act_tap_dir = IIO_MOD_Y;
-		else if (FIELD_GET(ADXL345_X_EN, regval) > 0)
+		else if ((FIELD_GET(ADXL345_X_EN, regval >> 4)
+				| FIELD_GET(ADXL345_X_EN, regval)) > 0)
 			*act_tap_dir = IIO_MOD_X;
 	}
 
@@ -1154,6 +1285,17 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
 			return ret;
 	}
 
+	if (FIELD_GET(ADXL345_INT_ACTIVITY, int_stat)) {
+		ret = iio_push_event(indio_dev,
+				     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
+							act_tap_dir,
+							IIO_EV_TYPE_THRESH,
+							IIO_EV_DIR_RISING),
+				     ts);
+		if (ret)
+			return ret;
+	}
+
 	if (FIELD_GET(ADXL345_INT_FREE_FALL, int_stat)) {
 		ret = iio_push_event(indio_dev,
 				     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
@@ -1264,6 +1406,13 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 		return -ENODEV;
 	st->fifo_delay = fifo_delay_default;
 
+	/*
+	 * If the feature is enabled, scan all axis for activity and or
+	 * inactivity, and set activity and inactivity to the same ac / dc
+	 * setup.
+	 */
+	st->act_axis_ctrl = ADXL345_REG_ACT_AXIS_MSK;
+	st->act_ac = 0;
 	st->int_map = 0x00;			/* reset interrupts */
 
 	/* Init with reasonable values */
@@ -1272,6 +1421,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 	st->tap_window_us = 20;			/*   20 [0x14] -> .025    */
 	st->tap_latent_us = 20;			/*   20 [0x14] -> .025    */
 
+	st->act_value = 6;			/*    6 [0x06]            */
 	st->ff_value = 8;			/*    8 [0x08]            */
 	st->ff_time_ms = 32;			/*   32 [0x20] -> 0.16    */
 
-- 
2.39.5

Re: [PATCH v1 11/12] iio: accel: adxl345: add activity feature

[Jonathan Cameron]

On Tue, 28 Jan 2025 12:00:59 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add the handling of activity events, also add sysfs entries to
> configure threshold values to trigger the event. Allow to push the
> event over to the iio channel.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>

I was going to guess these were rate of change detectors or ones
at least slightly compensating for g, but superficially
looks like a straight forward rising mag threshold.  Not seeing need
for new ABI.

We've had those interfaces with accelerometers for a long time.
Key is to map the pretty names in the datasheet into what is actually
being detected. That allows for a lot better generalization across manufacturers.

> ---
>  drivers/iio/accel/adxl345_core.c | 158 ++++++++++++++++++++++++++++++-
>  1 file changed, 154 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 62d75d28b6fc..94c3ad818ba5 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -121,6 +121,8 @@
>  
>  #define ADXL345_REG_TAP_AXIS_MSK	GENMASK(2, 0)
>  #define ADXL345_REG_TAP_SUPPRESS_MSK	BIT(3)
> +#define ADXL345_REG_ACT_AXIS_MSK	GENMASK(6, 4)
> +#define ADXL345_REG_ACT_ACDC_MSK	BIT(7)
>  
>  enum adxl345_axis {
>  	ADXL345_Z_EN = BIT(0),
> @@ -163,6 +165,10 @@ struct adxl345_state {
>  	u8 watermark;
>  	u8 fifo_mode;
>  
> +	u32 act_axis_ctrl;
> +	bool act_ac;
> +	u8 act_value;
> +
>  	u32 tap_axis_ctrl;
>  	u8 tap_threshold;
>  	u32 tap_duration_us;
> @@ -177,6 +183,11 @@ struct adxl345_state {
>  };
>  
>  static struct iio_event_spec adxl345_events[] = {
> +	{
> +		/* activity */
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_RISING,
> +	},
>  	{
>  		/* single tap */
>  		.type = IIO_EV_TYPE_GESTURE,
> @@ -276,6 +287,117 @@ static inline int adxl345_write_interrupts(struct adxl345_state *st)
>  	return regmap_write(st->regmap, ADXL345_REG_INT_ENABLE, st->int_map);
>  }
>  
> +/* act/inact */
> +
> +static int adxl345_write_act_axis(struct adxl345_state *st, bool en)
> +{
> +	int ret;
> +
> +	/*
> +	 * A setting of 0 selects dc-coupled operation, and a setting of 1
> +	 * enables ac-coupled operation. In dc-coupled operation, the current
> +	 * acceleration magnitude is compared directly with THRESH_ACT and
> +	 * 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
> +	 * 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
> +	 * THRESH_INACT. If the difference is less than the value in
> +	 * THRESH_INACT for the time in TIME_INACT, the device is  considered
> +	 * inactive and the inactivity interrupt is triggered.
> +	 */
> +	ret = regmap_update_bits(st->regmap, ADXL345_REG_ACT_INACT_CTRL,
> +				 ADXL345_REG_ACT_ACDC_MSK, st->act_ac);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * The ADXL345 allows for individually enabling/disabling axis for
> +	 * activity and inactivity detection, respectively. Here both axis are
> +	 * kept in sync, i.e. an axis will be generally enabled or disabled for
> +	 * both equally, activity and inactivity detection.
> +	 */
> +	st->act_axis_ctrl = en
> +		? st->act_axis_ctrl | ADXL345_REG_ACT_AXIS_MSK
> +		: st->act_axis_ctrl & ~ADXL345_REG_ACT_AXIS_MSK;
> +
> +	ret = regmap_update_bits(st->regmap, ADXL345_REG_ACT_INACT_CTRL,
> +				 ADXL345_REG_ACT_AXIS_MSK,
> +				 st->act_axis_ctrl);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int adxl345_set_act_int(struct adxl345_state *st)
> +{
> +	bool args_valid;
> +	bool axis_en;
> +
> +	axis_en = FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, st->act_axis_ctrl) > 0;
> +	args_valid = axis_en && st->act_value > 0;
> +	adxl345_intmap_switch_bit(st, args_valid, ADXL345_INT_ACTIVITY);
> +
> +	return adxl345_write_interrupts(st);
> +}
> +
> +static int _adxl345_is_act_en(struct adxl345_state *st, bool *en)
> +{
> +	int ret;
> +	unsigned int regval;
> +
> +	ret = adxl345_read_interrupts(st, &regval);
> +	if (ret)
> +		return ret;
> +
> +	*en = FIELD_GET(ADXL345_INT_ACTIVITY, regval) > 0;
> +
> +	return 0;
> +}
> +
> +static int _adxl345_set_act_en(struct adxl345_state *st, bool en)
> +{
> +	int ret;
> +
> +	ret = adxl345_write_act_axis(st, en);
> +	if (ret)
> +		return ret;
> +
> +	return adxl345_set_act_int(st);
> +}
> +
> +static int adxl345_is_act_en(struct adxl345_state *st, bool *en)
> +{
> +	return _adxl345_is_act_en(st, en);
> +}
> +
> +static int adxl345_set_act_en(struct adxl345_state *st, bool en)
> +{
> +	return _adxl345_set_act_en(st, en);
> +}
> +
> +static int _adxl345_set_act_value(struct adxl345_state *st, u8 val)
> +{
> +	st->act_value = val;
> +
> +	return regmap_write(st->regmap, ADXL345_REG_THRESH_ACT, val);
> +}
> +
> +static int adxl345_set_act_value(struct adxl345_state *st, u8 val)
> +{
> +	return _adxl345_set_act_value(st, val);
> +}

Re: [PATCH v1 11/12] iio: accel: adxl345: add activity feature

[Lothar Rubusch]

On Sat, Feb 1, 2025 at 6:27 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 28 Jan 2025 12:00:59 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Add the handling of activity events, also add sysfs entries to
> > configure threshold values to trigger the event. Allow to push the
> > event over to the iio channel.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
>
> I was going to guess these were rate of change detectors or ones
> at least slightly compensating for g, but superficially
> looks like a straight forward rising mag threshold.  Not seeing need
> for new ABI.
>
> We've had those interfaces with accelerometers for a long time.
> Key is to map the pretty names in the datasheet into what is actually
> being detected. That allows for a lot better generalization across manufacturers.
>

I understand. The adxl345 should actually be nothing fancy. I prepare
something more IIO-like event handling for the next version of the
patches. Thank you for the feedback.

> > ---
> >  drivers/iio/accel/adxl345_core.c | 158 ++++++++++++++++++++++++++++++-
> >  1 file changed, 154 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > index 62d75d28b6fc..94c3ad818ba5 100644
> > --- a/drivers/iio/accel/adxl345_core.c
> > +++ b/drivers/iio/accel/adxl345_core.c
> > @@ -121,6 +121,8 @@
> >
> >  #define ADXL345_REG_TAP_AXIS_MSK     GENMASK(2, 0)
> >  #define ADXL345_REG_TAP_SUPPRESS_MSK BIT(3)
> > +#define ADXL345_REG_ACT_AXIS_MSK     GENMASK(6, 4)
> > +#define ADXL345_REG_ACT_ACDC_MSK     BIT(7)
> >
> >  enum adxl345_axis {
> >       ADXL345_Z_EN = BIT(0),
> > @@ -163,6 +165,10 @@ struct adxl345_state {
> >       u8 watermark;
> >       u8 fifo_mode;
> >
> > +     u32 act_axis_ctrl;
> > +     bool act_ac;
> > +     u8 act_value;
> > +
> >       u32 tap_axis_ctrl;
> >       u8 tap_threshold;
> >       u32 tap_duration_us;
> > @@ -177,6 +183,11 @@ struct adxl345_state {
> >  };
> >
> >  static struct iio_event_spec adxl345_events[] = {
> > +     {
> > +             /* activity */
> > +             .type = IIO_EV_TYPE_THRESH,
> > +             .dir = IIO_EV_DIR_RISING,
> > +     },
> >       {
> >               /* single tap */
> >               .type = IIO_EV_TYPE_GESTURE,
> > @@ -276,6 +287,117 @@ static inline int adxl345_write_interrupts(struct adxl345_state *st)
> >       return regmap_write(st->regmap, ADXL345_REG_INT_ENABLE, st->int_map);
> >  }
> >
> > +/* act/inact */
> > +
> > +static int adxl345_write_act_axis(struct adxl345_state *st, bool en)
> > +{
> > +     int ret;
> > +
> > +     /*
> > +      * A setting of 0 selects dc-coupled operation, and a setting of 1
> > +      * enables ac-coupled operation. In dc-coupled operation, the current
> > +      * acceleration magnitude is compared directly with THRESH_ACT and
> > +      * 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
> > +      * 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
> > +      * THRESH_INACT. If the difference is less than the value in
> > +      * THRESH_INACT for the time in TIME_INACT, the device is  considered
> > +      * inactive and the inactivity interrupt is triggered.
> > +      */
> > +     ret = regmap_update_bits(st->regmap, ADXL345_REG_ACT_INACT_CTRL,
> > +                              ADXL345_REG_ACT_ACDC_MSK, st->act_ac);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /*
> > +      * The ADXL345 allows for individually enabling/disabling axis for
> > +      * activity and inactivity detection, respectively. Here both axis are
> > +      * kept in sync, i.e. an axis will be generally enabled or disabled for
> > +      * both equally, activity and inactivity detection.
> > +      */
> > +     st->act_axis_ctrl = en
> > +             ? st->act_axis_ctrl | ADXL345_REG_ACT_AXIS_MSK
> > +             : st->act_axis_ctrl & ~ADXL345_REG_ACT_AXIS_MSK;
> > +
> > +     ret = regmap_update_bits(st->regmap, ADXL345_REG_ACT_INACT_CTRL,
> > +                              ADXL345_REG_ACT_AXIS_MSK,
> > +                              st->act_axis_ctrl);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return 0;
> > +}
> > +
> > +static int adxl345_set_act_int(struct adxl345_state *st)
> > +{
> > +     bool args_valid;
> > +     bool axis_en;
> > +
> > +     axis_en = FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, st->act_axis_ctrl) > 0;
> > +     args_valid = axis_en && st->act_value > 0;
> > +     adxl345_intmap_switch_bit(st, args_valid, ADXL345_INT_ACTIVITY);
> > +
> > +     return adxl345_write_interrupts(st);
> > +}
> > +
> > +static int _adxl345_is_act_en(struct adxl345_state *st, bool *en)
> > +{
> > +     int ret;
> > +     unsigned int regval;
> > +
> > +     ret = adxl345_read_interrupts(st, &regval);
> > +     if (ret)
> > +             return ret;
> > +
> > +     *en = FIELD_GET(ADXL345_INT_ACTIVITY, regval) > 0;
> > +
> > +     return 0;
> > +}
> > +
> > +static int _adxl345_set_act_en(struct adxl345_state *st, bool en)
> > +{
> > +     int ret;
> > +
> > +     ret = adxl345_write_act_axis(st, en);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return adxl345_set_act_int(st);
> > +}
> > +
> > +static int adxl345_is_act_en(struct adxl345_state *st, bool *en)
> > +{
> > +     return _adxl345_is_act_en(st, en);
> > +}
> > +
> > +static int adxl345_set_act_en(struct adxl345_state *st, bool en)
> > +{
> > +     return _adxl345_set_act_en(st, en);
> > +}
> > +
> > +static int _adxl345_set_act_value(struct adxl345_state *st, u8 val)
> > +{
> > +     st->act_value = val;
> > +
> > +     return regmap_write(st->regmap, ADXL345_REG_THRESH_ACT, val);
> > +}
> > +
> > +static int adxl345_set_act_value(struct adxl345_state *st, u8 val)
> > +{
> > +     return _adxl345_set_act_value(st, val);
> > +}
>

[PATCH v1 12/12] iio: accel: adxl345: add inactivity feature

[Lothar Rubusch]

Add handling for inactivity events, together with threshold and timeout
to trigger the event. The event could be used for ODR and g range
reduction for power saving using low power modes or sleep modes, not
covered by this patch.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 94c3ad818ba5..b9f42c56e8f1 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -123,6 +123,8 @@
 #define ADXL345_REG_TAP_SUPPRESS_MSK	BIT(3)
 #define ADXL345_REG_ACT_AXIS_MSK	GENMASK(6, 4)
 #define ADXL345_REG_ACT_ACDC_MSK	BIT(7)
+#define ADXL345_REG_INACT_AXIS_MSK	GENMASK(2, 0)
+#define ADXL345_REG_INACT_ACDC_MSK	BIT(3)
 
 enum adxl345_axis {
 	ADXL345_Z_EN = BIT(0),
@@ -155,6 +157,32 @@ static const unsigned int adxl345_tap_time_reg[3] = {
 	[ADXL345_TAP_TIME_DUR] = ADXL345_REG_DUR,
 };
 
+/* activity/inactivity */
+enum adxl345_activity_type {
+	ADXL345_ACTIVITY,
+	ADXL345_INACTIVITY
+};
+
+static const unsigned int adxl345_act_int_reg[2] = {
+	[ADXL345_ACTIVITY] = ADXL345_INT_ACTIVITY,
+	[ADXL345_INACTIVITY] = ADXL345_INT_INACTIVITY,
+};
+
+static const unsigned int adxl345_act_thresh_reg[2] = {
+	[ADXL345_ACTIVITY] = ADXL345_REG_THRESH_ACT,
+	[ADXL345_INACTIVITY] = ADXL345_REG_THRESH_INACT,
+};
+
+static const unsigned int adxl345_act_acdc_msk[2] = {
+	[ADXL345_ACTIVITY] = ADXL345_REG_ACT_ACDC_MSK,
+	[ADXL345_INACTIVITY] = ADXL345_REG_INACT_ACDC_MSK
+};
+
+static const unsigned int adxl345_act_axis_msk[2] = {
+	[ADXL345_ACTIVITY] = ADXL345_REG_ACT_AXIS_MSK,
+	[ADXL345_INACTIVITY] = ADXL345_REG_INACT_AXIS_MSK
+};
+
 struct adxl345_state {
 	const struct adxl345_chip_info *info;
 	struct regmap *regmap;
@@ -169,6 +197,11 @@ struct adxl345_state {
 	bool act_ac;
 	u8 act_value;
 
+	u32 inact_axis_ctrl;
+	bool inact_ac;
+	u8 inact_value;
+	u8 inact_time_s;
+
 	u32 tap_axis_ctrl;
 	u8 tap_threshold;
 	u32 tap_duration_us;
@@ -188,6 +221,11 @@ static struct iio_event_spec adxl345_events[] = {
 		.type = IIO_EV_TYPE_THRESH,
 		.dir = IIO_EV_DIR_RISING,
 	},
+	{
+		/* inactivity */
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+	},
 	{
 		/* single tap */
 		.type = IIO_EV_TYPE_GESTURE,
@@ -289,7 +327,8 @@ static inline int adxl345_write_interrupts(struct adxl345_state *st)
 
 /* act/inact */
 
-static int adxl345_write_act_axis(struct adxl345_state *st, bool en)
+static int adxl345_write_act_axis(struct adxl345_state *st,
+				  enum adxl345_activity_type type, bool en)
 {
 	int ret;
 
@@ -316,7 +355,9 @@ static int adxl345_write_act_axis(struct adxl345_state *st, bool en)
 	 * inactive and the inactivity interrupt is triggered.
 	 */
 	ret = regmap_update_bits(st->regmap, ADXL345_REG_ACT_INACT_CTRL,
-				 ADXL345_REG_ACT_ACDC_MSK, st->act_ac);
+				 adxl345_act_acdc_msk[type],
+				 (type == ADXL345_ACTIVITY
+				  ? st->act_ac : st->inact_ac));
 	if (ret)
 		return ret;
 
@@ -326,32 +367,52 @@ static int adxl345_write_act_axis(struct adxl345_state *st, bool en)
 	 * kept in sync, i.e. an axis will be generally enabled or disabled for
 	 * both equally, activity and inactivity detection.
 	 */
-	st->act_axis_ctrl = en
-		? st->act_axis_ctrl | ADXL345_REG_ACT_AXIS_MSK
-		: st->act_axis_ctrl & ~ADXL345_REG_ACT_AXIS_MSK;
+	if (type == ADXL345_ACTIVITY) {
+		st->act_axis_ctrl = en
+			? st->act_axis_ctrl | ADXL345_REG_ACT_AXIS_MSK
+			: st->act_axis_ctrl & ~ADXL345_REG_ACT_AXIS_MSK;
+
+		ret = regmap_update_bits(st->regmap, ADXL345_REG_ACT_INACT_CTRL,
+					 adxl345_act_axis_msk[type],
+					 st->act_axis_ctrl);
+		if (ret)
+			return ret;
 
-	ret = regmap_update_bits(st->regmap, ADXL345_REG_ACT_INACT_CTRL,
-				 ADXL345_REG_ACT_AXIS_MSK,
-				 st->act_axis_ctrl);
-	if (ret)
-		return ret;
+	} else {
+		st->inact_axis_ctrl = en
+			? st->inact_axis_ctrl | ADXL345_REG_INACT_AXIS_MSK
+			: st->inact_axis_ctrl & ~ADXL345_REG_INACT_AXIS_MSK;
 
+		ret = regmap_update_bits(st->regmap, ADXL345_REG_ACT_INACT_CTRL,
+					 adxl345_act_axis_msk[type],
+					 st->inact_axis_ctrl);
+		if (ret)
+			return ret;
+	}
 	return 0;
 }
 
-static int adxl345_set_act_int(struct adxl345_state *st)
+static int adxl345_set_act_int(struct adxl345_state *st,
+			       enum adxl345_activity_type type)
 {
 	bool args_valid;
 	bool axis_en;
 
-	axis_en = FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, st->act_axis_ctrl) > 0;
-	args_valid = axis_en && st->act_value > 0;
-	adxl345_intmap_switch_bit(st, args_valid, ADXL345_INT_ACTIVITY);
+	if (type == ADXL345_ACTIVITY) {
+		axis_en = FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, st->act_axis_ctrl) > 0;
+		args_valid = axis_en && st->act_value > 0;
+	} else {
+		axis_en = FIELD_GET(ADXL345_REG_INACT_AXIS_MSK, st->inact_axis_ctrl) > 0;
+		args_valid = axis_en && st->inact_value > 0 &&
+			st->inact_time_s > 0;
+	}
+	adxl345_intmap_switch_bit(st, args_valid, adxl345_act_int_reg[type]);
 
 	return adxl345_write_interrupts(st);
 }
 
-static int _adxl345_is_act_en(struct adxl345_state *st, bool *en)
+static int _adxl345_is_act_en(struct adxl345_state *st,
+			      enum adxl345_activity_type type, bool *en)
 {
 	int ret;
 	unsigned int regval;
@@ -360,42 +421,76 @@ static int _adxl345_is_act_en(struct adxl345_state *st, bool *en)
 	if (ret)
 		return ret;
 
-	*en = FIELD_GET(ADXL345_INT_ACTIVITY, regval) > 0;
+	*en = (adxl345_act_int_reg[type] & regval) > 0;
 
 	return 0;
 }
 
-static int _adxl345_set_act_en(struct adxl345_state *st, bool en)
+static int _adxl345_set_act_en(struct adxl345_state *st,
+			       enum adxl345_activity_type type, bool en)
 {
 	int ret;
 
-	ret = adxl345_write_act_axis(st, en);
+	ret = adxl345_write_act_axis(st, type, en);
 	if (ret)
 		return ret;
 
-	return adxl345_set_act_int(st);
+	return adxl345_set_act_int(st, type);
 }
 
 static int adxl345_is_act_en(struct adxl345_state *st, bool *en)
 {
-	return _adxl345_is_act_en(st, en);
+	return _adxl345_is_act_en(st, ADXL345_ACTIVITY, en);
 }
 
 static int adxl345_set_act_en(struct adxl345_state *st, bool en)
 {
-	return _adxl345_set_act_en(st, en);
+	return _adxl345_set_act_en(st, ADXL345_ACTIVITY, en);
 }
 
-static int _adxl345_set_act_value(struct adxl345_state *st, u8 val)
+static int adxl345_is_inact_en(struct adxl345_state *st, bool *en)
 {
-	st->act_value = val;
+	return _adxl345_is_act_en(st, ADXL345_INACTIVITY, en);
+}
 
-	return regmap_write(st->regmap, ADXL345_REG_THRESH_ACT, val);
+static int adxl345_set_inact_en(struct adxl345_state *st, bool en)
+{
+	return _adxl345_set_act_en(st, ADXL345_INACTIVITY, en);
+}
+
+static int _adxl345_set_act_value(struct adxl345_state *st,
+				  enum adxl345_activity_type type, u8 val)
+{
+	switch (type) {
+	case ADXL345_ACTIVITY:
+		st->act_value = val;
+		break;
+	case ADXL345_INACTIVITY:
+		st->inact_value = val;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return regmap_write(st->regmap, adxl345_act_thresh_reg[type], val);
 }
 
 static int adxl345_set_act_value(struct adxl345_state *st, u8 val)
 {
-	return _adxl345_set_act_value(st, val);
+	return _adxl345_set_act_value(st, ADXL345_ACTIVITY, val);
+}
+
+static int adxl345_set_inact_value(struct adxl345_state *st, u8 val)
+{
+	return _adxl345_set_act_value(st, ADXL345_INACTIVITY, val);
+}
+
+static int adxl345_set_inact_time_s(struct adxl345_state *st, u32 val_s)
+{
+	st->inact_time_s = min(val_s, 0xff);
+
+	return regmap_write(st->regmap, ADXL345_REG_TIME_INACT,
+			    st->inact_time_s);
 }
 
 /* tap */
@@ -1027,6 +1122,8 @@ static int adxl345_write_raw_get_fmt(struct iio_dev *indio_dev,
 	static IIO_DEVICE_ATTR_RW(in_accel_##A##_##C##_##E, 0)
 
 ADXL345_generate_iio_dev_attr_INT(activity, act, value);
+ADXL345_generate_iio_dev_attr_INT(activity, inact, value);
+ADXL345_generate_iio_dev_attr_INT(activity, inact, time_s);
 ADXL345_generate_iio_dev_attr_INT(freefall, ff, value);
 
 ADXL345_generate_iio_dev_attr_FRACTIONAL(gesture_singletap, tap, duration, MICRO, us);
@@ -1035,12 +1132,16 @@ ADXL345_generate_iio_dev_attr_FRACTIONAL(gesture_doubletap, tap, latent, MICRO,
 ADXL345_generate_iio_dev_attr_FRACTIONAL(freefall, ff, time, MILLI, ms);
 
 ADXL345_generate_iio_dev_attr_EN(activity, act);
+ADXL345_generate_iio_dev_attr_EN(activity, inact);
 ADXL345_generate_iio_dev_attr_EN(freefall, ff);
 ADXL345_generate_iio_dev_attr_EN(gesture_doubletap, suppressed);
 
 static struct attribute *adxl345_event_attrs[] = {
 	&iio_dev_attr_in_accel_activity_act_en.dev_attr.attr,
 	&iio_dev_attr_in_accel_activity_act_value.dev_attr.attr,
+	&iio_dev_attr_in_accel_activity_inact_en.dev_attr.attr,
+	&iio_dev_attr_in_accel_activity_inact_value.dev_attr.attr,
+	&iio_dev_attr_in_accel_activity_inact_time_s.dev_attr.attr,
 	&iio_dev_attr_in_accel_freefall_ff_en.dev_attr.attr,
 	&iio_dev_attr_in_accel_freefall_ff_value.dev_attr.attr,
 	&iio_dev_attr_in_accel_freefall_time_ms.dev_attr.attr,
@@ -1296,6 +1397,17 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
 			return ret;
 	}
 
+	if (FIELD_GET(ADXL345_INT_INACTIVITY, int_stat)) {
+		ret = iio_push_event(indio_dev,
+				     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
+							IIO_MOD_X_OR_Y_OR_Z,
+							IIO_EV_TYPE_THRESH,
+							IIO_EV_DIR_FALLING),
+				     ts);
+		if (ret)
+			return ret;
+	}
+
 	if (FIELD_GET(ADXL345_INT_FREE_FALL, int_stat)) {
 		ret = iio_push_event(indio_dev,
 				     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
@@ -1412,6 +1524,8 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 	 * setup.
 	 */
 	st->act_axis_ctrl = ADXL345_REG_ACT_AXIS_MSK;
+	st->inact_axis_ctrl = ADXL345_REG_INACT_AXIS_MSK;
+	st->inact_ac = 0;			/*    0 [dc]              */
 	st->act_ac = 0;
 	st->int_map = 0x00;			/* reset interrupts */
 
@@ -1422,6 +1536,9 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 	st->tap_latent_us = 20;			/*   20 [0x14] -> .025    */
 
 	st->act_value = 6;			/*    6 [0x06]            */
+	st->inact_value = 4;			/*    4 [0x04]            */
+	st->inact_time_s = 3;			/*    3 [0x03] -> 3       */
+
 	st->ff_value = 8;			/*    8 [0x08]            */
 	st->ff_time_ms = 32;			/*   32 [0x20] -> 0.16    */
 
-- 
2.39.5

Re: [PATCH v1 12/12] iio: accel: adxl345: add inactivity feature

[Jonathan Cameron]

On Tue, 28 Jan 2025 12:01:00 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add handling for inactivity events, together with threshold and timeout
> to trigger the event. The event could be used for ODR and g range
> reduction for power saving using low power modes or sleep modes, not
> covered by this patch.

This is a falling magnitude threshold.  The complexity you'll hit
here is we don't support more than one on a given axis. So this clashes
with free fall detection.

Argument for that long ago was that it complicates the interface a lot
and is mostly pointless as we take action of the first event.
Whether that matters here depends on whether you can do freefall as
all axis (which is all that makes sense as falling devices tend to flip)
and this on a per axis basis. In which case can easily support both with
standard ABI. Just need a magic channel for the freefall configuration
that has only events and sets X_AND_Y_AND_Z as the direction as all
must be below the threshold to see an interrupt.

For the AC / DC thing we normally support that as TYPE_MAG_REFERENCED
which might also be a good choice.

> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl345_core.c | 167 ++++++++++++++++++++++++++-----
>  1 file changed, 142 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 94c3ad818ba5..b9f42c56e8f1 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -123,6 +123,8 @@
>  #define ADXL345_REG_TAP_SUPPRESS_MSK	BIT(3)
>  #define ADXL345_REG_ACT_AXIS_MSK	GENMASK(6, 4)
>  #define ADXL345_REG_ACT_ACDC_MSK	BIT(7)
> +#define ADXL345_REG_INACT_AXIS_MSK	GENMASK(2, 0)
> +#define ADXL345_REG_INACT_ACDC_MSK	BIT(3)
>  
>  enum adxl345_axis {
>  	ADXL345_Z_EN = BIT(0),
> @@ -155,6 +157,32 @@ static const unsigned int adxl345_tap_time_reg[3] = {
>  	[ADXL345_TAP_TIME_DUR] = ADXL345_REG_DUR,
>  };
>  
> +/* activity/inactivity */
> +enum adxl345_activity_type {
> +	ADXL345_ACTIVITY,
> +	ADXL345_INACTIVITY
> +};
> +
> +static const unsigned int adxl345_act_int_reg[2] = {
> +	[ADXL345_ACTIVITY] = ADXL345_INT_ACTIVITY,
> +	[ADXL345_INACTIVITY] = ADXL345_INT_INACTIVITY,
> +};
> +
> +static const unsigned int adxl345_act_thresh_reg[2] = {
> +	[ADXL345_ACTIVITY] = ADXL345_REG_THRESH_ACT,
> +	[ADXL345_INACTIVITY] = ADXL345_REG_THRESH_INACT,
> +};
> +
> +static const unsigned int adxl345_act_acdc_msk[2] = {
> +	[ADXL345_ACTIVITY] = ADXL345_REG_ACT_ACDC_MSK,
> +	[ADXL345_INACTIVITY] = ADXL345_REG_INACT_ACDC_MSK
> +};
> +
> +static const unsigned int adxl345_act_axis_msk[2] = {
> +	[ADXL345_ACTIVITY] = ADXL345_REG_ACT_AXIS_MSK,
> +	[ADXL345_INACTIVITY] = ADXL345_REG_INACT_AXIS_MSK
> +};
> +
>  struct adxl345_state {
>  	const struct adxl345_chip_info *info;
>  	struct regmap *regmap;
> @@ -169,6 +197,11 @@ struct adxl345_state {
>  	bool act_ac;
>  	u8 act_value;
>  
> +	u32 inact_axis_ctrl;
> +	bool inact_ac;
> +	u8 inact_value;
> +	u8 inact_time_s;
> +
>  	u32 tap_axis_ctrl;
>  	u8 tap_threshold;
>  	u32 tap_duration_us;
> @@ -188,6 +221,11 @@ static struct iio_event_spec adxl345_events[] = {
>  		.type = IIO_EV_TYPE_THRESH,
>  		.dir = IIO_EV_DIR_RISING,
>  	},
> +	{
> +		/* inactivity */
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_FALLING,
> +	},
>  	{
>  		/* single tap */
>  		.type = IIO_EV_TYPE_GESTURE,
> @@ -289,7 +327,8 @@ static inline int adxl345_write_interrupts(struct adxl345_state *st)
>  
>  /* act/inact */
>  
> -static int adxl345_write_act_axis(struct adxl345_state *st, bool en)
> +static int adxl345_write_act_axis(struct adxl345_state *st,
> +				  enum adxl345_activity_type type, bool en)
>  {
>  	int ret;
>  
> @@ -316,7 +355,9 @@ static int adxl345_write_act_axis(struct adxl345_state *st, bool en)
>  	 * inactive and the inactivity interrupt is triggered.
>  	 */
>  	ret = regmap_update_bits(st->regmap, ADXL345_REG_ACT_INACT_CTRL,
> -				 ADXL345_REG_ACT_ACDC_MSK, st->act_ac);
> +				 adxl345_act_acdc_msk[type],
> +				 (type == ADXL345_ACTIVITY
> +				  ? st->act_ac : st->inact_ac));
>  	if (ret)
>  		return ret;
>  
> @@ -326,32 +367,52 @@ static int adxl345_write_act_axis(struct adxl345_state *st, bool en)
>  	 * kept in sync, i.e. an axis will be generally enabled or disabled for
>  	 * both equally, activity and inactivity detection.
>  	 */
> -	st->act_axis_ctrl = en
> -		? st->act_axis_ctrl | ADXL345_REG_ACT_AXIS_MSK
> -		: st->act_axis_ctrl & ~ADXL345_REG_ACT_AXIS_MSK;
> +	if (type == ADXL345_ACTIVITY) {
> +		st->act_axis_ctrl = en
> +			? st->act_axis_ctrl | ADXL345_REG_ACT_AXIS_MSK
> +			: st->act_axis_ctrl & ~ADXL345_REG_ACT_AXIS_MSK;
> +
> +		ret = regmap_update_bits(st->regmap, ADXL345_REG_ACT_INACT_CTRL,
> +					 adxl345_act_axis_msk[type],
> +					 st->act_axis_ctrl);
> +		if (ret)
> +			return ret;
>  
> -	ret = regmap_update_bits(st->regmap, ADXL345_REG_ACT_INACT_CTRL,
> -				 ADXL345_REG_ACT_AXIS_MSK,
> -				 st->act_axis_ctrl);
> -	if (ret)
> -		return ret;
> +	} else {
> +		st->inact_axis_ctrl = en
> +			? st->inact_axis_ctrl | ADXL345_REG_INACT_AXIS_MSK
> +			: st->inact_axis_ctrl & ~ADXL345_REG_INACT_AXIS_MSK;
>  
> +		ret = regmap_update_bits(st->regmap, ADXL345_REG_ACT_INACT_CTRL,
> +					 adxl345_act_axis_msk[type],
> +					 st->inact_axis_ctrl);
> +		if (ret)
> +			return ret;
> +	}
>  	return 0;
>  }
>  
> -static int adxl345_set_act_int(struct adxl345_state *st)
> +static int adxl345_set_act_int(struct adxl345_state *st,
> +			       enum adxl345_activity_type type)
>  {
>  	bool args_valid;
>  	bool axis_en;
>  
> -	axis_en = FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, st->act_axis_ctrl) > 0;
> -	args_valid = axis_en && st->act_value > 0;
> -	adxl345_intmap_switch_bit(st, args_valid, ADXL345_INT_ACTIVITY);
> +	if (type == ADXL345_ACTIVITY) {
> +		axis_en = FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, st->act_axis_ctrl) > 0;
> +		args_valid = axis_en && st->act_value > 0;
> +	} else {
> +		axis_en = FIELD_GET(ADXL345_REG_INACT_AXIS_MSK, st->inact_axis_ctrl) > 0;
> +		args_valid = axis_en && st->inact_value > 0 &&
> +			st->inact_time_s > 0;
> +	}
> +	adxl345_intmap_switch_bit(st, args_valid, adxl345_act_int_reg[type]);
>  
>  	return adxl345_write_interrupts(st);
>  }
>  
> -static int _adxl345_is_act_en(struct adxl345_state *st, bool *en)
> +static int _adxl345_is_act_en(struct adxl345_state *st,
> +			      enum adxl345_activity_type type, bool *en)
>  {
>  	int ret;
>  	unsigned int regval;
> @@ -360,42 +421,76 @@ static int _adxl345_is_act_en(struct adxl345_state *st, bool *en)
>  	if (ret)
>  		return ret;
>  
> -	*en = FIELD_GET(ADXL345_INT_ACTIVITY, regval) > 0;
> +	*en = (adxl345_act_int_reg[type] & regval) > 0;
>  
>  	return 0;
>  }
>  
> -static int _adxl345_set_act_en(struct adxl345_state *st, bool en)
> +static int _adxl345_set_act_en(struct adxl345_state *st,
> +			       enum adxl345_activity_type type, bool en)
>  {
>  	int ret;
>  
> -	ret = adxl345_write_act_axis(st, en);
> +	ret = adxl345_write_act_axis(st, type, en);
>  	if (ret)
>  		return ret;
>  
> -	return adxl345_set_act_int(st);
> +	return adxl345_set_act_int(st, type);
>  }
>  
>  static int adxl345_is_act_en(struct adxl345_state *st, bool *en)
>  {
> -	return _adxl345_is_act_en(st, en);
> +	return _adxl345_is_act_en(st, ADXL345_ACTIVITY, en);
>  }
>  
>  static int adxl345_set_act_en(struct adxl345_state *st, bool en)
>  {
> -	return _adxl345_set_act_en(st, en);
> +	return _adxl345_set_act_en(st, ADXL345_ACTIVITY, en);
>  }
>  
> -static int _adxl345_set_act_value(struct adxl345_state *st, u8 val)
> +static int adxl345_is_inact_en(struct adxl345_state *st, bool *en)
>  {
> -	st->act_value = val;
> +	return _adxl345_is_act_en(st, ADXL345_INACTIVITY, en);
> +}
>  
> -	return regmap_write(st->regmap, ADXL345_REG_THRESH_ACT, val);
> +static int adxl345_set_inact_en(struct adxl345_state *st, bool en)
> +{
> +	return _adxl345_set_act_en(st, ADXL345_INACTIVITY, en);
> +}
> +
> +static int _adxl345_set_act_value(struct adxl345_state *st,
> +				  enum adxl345_activity_type type, u8 val)
> +{
> +	switch (type) {
> +	case ADXL345_ACTIVITY:
> +		st->act_value = val;
> +		break;
> +	case ADXL345_INACTIVITY:
> +		st->inact_value = val;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return regmap_write(st->regmap, adxl345_act_thresh_reg[type], val);
>  }
>  
>  static int adxl345_set_act_value(struct adxl345_state *st, u8 val)
>  {
> -	return _adxl345_set_act_value(st, val);
> +	return _adxl345_set_act_value(st, ADXL345_ACTIVITY, val);
> +}
> +
> +static int adxl345_set_inact_value(struct adxl345_state *st, u8 val)
> +{
> +	return _adxl345_set_act_value(st, ADXL345_INACTIVITY, val);
> +}
> +
> +static int adxl345_set_inact_time_s(struct adxl345_state *st, u32 val_s)
> +{
> +	st->inact_time_s = min(val_s, 0xff);
> +
> +	return regmap_write(st->regmap, ADXL345_REG_TIME_INACT,
> +			    st->inact_time_s);
>  }
>  
>  /* tap */
> @@ -1027,6 +1122,8 @@ static int adxl345_write_raw_get_fmt(struct iio_dev *indio_dev,
>  	static IIO_DEVICE_ATTR_RW(in_accel_##A##_##C##_##E, 0)
>  
>  ADXL345_generate_iio_dev_attr_INT(activity, act, value);
> +ADXL345_generate_iio_dev_attr_INT(activity, inact, value);
> +ADXL345_generate_iio_dev_attr_INT(activity, inact, time_s);
>  ADXL345_generate_iio_dev_attr_INT(freefall, ff, value);
>  
>  ADXL345_generate_iio_dev_attr_FRACTIONAL(gesture_singletap, tap, duration, MICRO, us);
> @@ -1035,12 +1132,16 @@ ADXL345_generate_iio_dev_attr_FRACTIONAL(gesture_doubletap, tap, latent, MICRO,
>  ADXL345_generate_iio_dev_attr_FRACTIONAL(freefall, ff, time, MILLI, ms);
>  
>  ADXL345_generate_iio_dev_attr_EN(activity, act);
> +ADXL345_generate_iio_dev_attr_EN(activity, inact);
>  ADXL345_generate_iio_dev_attr_EN(freefall, ff);
>  ADXL345_generate_iio_dev_attr_EN(gesture_doubletap, suppressed);
>  
>  static struct attribute *adxl345_event_attrs[] = {
>  	&iio_dev_attr_in_accel_activity_act_en.dev_attr.attr,
>  	&iio_dev_attr_in_accel_activity_act_value.dev_attr.attr,
> +	&iio_dev_attr_in_accel_activity_inact_en.dev_attr.attr,
> +	&iio_dev_attr_in_accel_activity_inact_value.dev_attr.attr,
> +	&iio_dev_attr_in_accel_activity_inact_time_s.dev_attr.attr,
>  	&iio_dev_attr_in_accel_freefall_ff_en.dev_attr.attr,
>  	&iio_dev_attr_in_accel_freefall_ff_value.dev_attr.attr,
>  	&iio_dev_attr_in_accel_freefall_time_ms.dev_attr.attr,
> @@ -1296,6 +1397,17 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
>  			return ret;
>  	}
>  
> +	if (FIELD_GET(ADXL345_INT_INACTIVITY, int_stat)) {
> +		ret = iio_push_event(indio_dev,
> +				     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> +							IIO_MOD_X_OR_Y_OR_Z,
> +							IIO_EV_TYPE_THRESH,
> +							IIO_EV_DIR_FALLING),
> +				     ts);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	if (FIELD_GET(ADXL345_INT_FREE_FALL, int_stat)) {
>  		ret = iio_push_event(indio_dev,
>  				     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> @@ -1412,6 +1524,8 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  	 * setup.
>  	 */
>  	st->act_axis_ctrl = ADXL345_REG_ACT_AXIS_MSK;
> +	st->inact_axis_ctrl = ADXL345_REG_INACT_AXIS_MSK;
> +	st->inact_ac = 0;			/*    0 [dc]              */
>  	st->act_ac = 0;
>  	st->int_map = 0x00;			/* reset interrupts */
>  
> @@ -1422,6 +1536,9 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  	st->tap_latent_us = 20;			/*   20 [0x14] -> .025    */
>  
>  	st->act_value = 6;			/*    6 [0x06]            */
> +	st->inact_value = 4;			/*    4 [0x04]            */
> +	st->inact_time_s = 3;			/*    3 [0x03] -> 3       */
> +
>  	st->ff_value = 8;			/*    8 [0x08]            */
>  	st->ff_time_ms = 32;			/*   32 [0x20] -> 0.16    */
>  

Re: [PATCH v1 00/12] iio: accel: adxl345: add interrupt based sensor events

[Jonathan Cameron]

On Tue, 28 Jan 2025 12:00:48 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add several interrupt based sensor detection events:
> - single tap
> - double tap
> - free fall
> - activity
> - inactivity
> 
> All the needed parameters for each and methods of adjusting them, and
> forward a resulting IIO event for each to the IIO channel.

So my main feedback here is to be much more reluctant to add new ABI.
Anything you add is unused by all existing code and if it is unique
to a driver probably never going to be used by anyone other than you.

We have a bunch of accelerometers in tree and as they go wrt to events
this one isn't even particularly complex.  The existing ABI covers
their events reasonably well so take a look at how they do it.  Often
it's a case of mapping names for the application of an event (free fall
detection, activity detection etc) to what what they are actually detecting.
Those generalize a lot better across different sensor types.  It's almost
always a threshold of some type. The tap / double tap are more complex
but we put quite a lot of effort into coming up with a general
description a year or so back.  There may new things but most of the
ABI is already there.

> 
> The sensor has further features still not covered:
> - g-ranges scaled by different ODRs, especially for activity / inactivity
>   threshold is not covered so far. There seems to be a particularity with
>   the ADXL345 as annotated on some analog FAQ.
> 
> - Various thinks like low power, sleep mode, etc. are (still) not covered
>   here, others (ACDC bit, selftest, etc.) currently are hard coded or not
>   covered.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
> Questions:
> - Do I need a mutex/lock protection as this is the case e.g. in the ADXL367
>   or the ADXL380?
>   Actually, I understand those cases as protecting access to the state
>   object by different channels, temperature and accelerometer. I'm unsure
>   if this is a correct understanding, where for the ADXL345 there should
>   not be any issue. At most, a currently displayed value on sysfs is
>   (still) not updated. So, IMHO I can rely on the internal protections in
>   regmap no further mutex is needed. Please, can you give me a feedback
>   here?

If you have an read modify write actions triggered by sysfs writes they
can race (not serialization between different file writes).
That's why you tend to need the mutex.

> 
> - FIELD_PREP/FIELD_GET: I'd like to use arrays of enum indexed elements
>   to allow for more generic function implementation passing just a "type"
>   field, e.g. at single tap/double tap or activity/inactivity handling.
>   When it comes to filtering out bits using FIELD_GET/FIELD_PREP, it says
>   that this enum array element is not "const enough". Is there a
>   work-around?
I don't have it to hand but there is a patch set trying to add non
const versions of these that went to my other email.

For now just carry a local version as we don't want to end up waiting
for that patch to merge.

> 
> Lothar Rubusch (12):
>   iio: accel: adxl345: migrate constants to core
>   iio: accel: adxl345: reorganize measurement enable
>   iio: accel: adxl345: add debug register access
>   iio: accel: adxl345: reorganize irq handler
>   iio: accel: adxl345: improve access to the interrupt enable register
>   iio: accel: adxl345: add single tap feature
>   iio: accel: adxl345: show tap status and direction
>   iio: accel: adxl345: add double tap feature
>   iio: accel: adxl345: add double tap suppress bit
>   iio: accel: adxl345: add freefall feature
>   iio: accel: adxl345: add activity feature
>   iio: accel: adxl345: add inactivity feature
> 
>  drivers/iio/accel/adxl345.h      |   86 ---
>  drivers/iio/accel/adxl345_core.c | 1150 ++++++++++++++++++++++++++++--
>  2 files changed, 1099 insertions(+), 137 deletions(-)
> 

Re: [PATCH v1 00/12] iio: accel: adxl345: add interrupt based sensor events

[Lothar Rubusch]

Hi Jonathan, please find my answers below.

On Sat, Feb 1, 2025 at 6:48 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 28 Jan 2025 12:00:48 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Add several interrupt based sensor detection events:
> > - single tap
> > - double tap
> > - free fall
> > - activity
> > - inactivity
> >
> > All the needed parameters for each and methods of adjusting them, and
> > forward a resulting IIO event for each to the IIO channel.
>
> So my main feedback here is to be much more reluctant to add new ABI.
> Anything you add is unused by all existing code and if it is unique
> to a driver probably never going to be used by anyone other than you.
>
> We have a bunch of accelerometers in tree and as they go wrt to events
> this one isn't even particularly complex.  The existing ABI covers
> their events reasonably well so take a look at how they do it.  Often
> it's a case of mapping names for the application of an event (free fall
> detection, activity detection etc) to what what they are actually detecting.
> Those generalize a lot better across different sensor types.  It's almost
> always a threshold of some type. The tap / double tap are more complex
> but we put quite a lot of effort into coming up with a general
> description a year or so back.  There may new things but most of the
> ABI is already there.
>
Please, understand my patches in "v1" rather as a huge set of questions,
than a proposal of reinventing the IIO ABI :)
My intention is actually not to extend/rewrite the ABI. It rather shows my lack
of knowledge combined with the curiosity of how to actually use the
(IIO) APIs for
such implementation. I know this is still tedious, but I'm sure it will become
better.

My dilemma was/is the following: I did an initial implementation more similar
to e.g. ADXL380 for such events, and the sca3000.c for the freefall event.
IMHO the names for the sysfs handles were not at all intuitively mappable to the
fields I liked to operate, such as tap duration, window, latent, etc.
The other alternative I saw, was setting up sysfs myself, IMHO clearer naming
but actually not really using IIO's event_config/event_value.

Personally, I don't have any preference. If there really is no way to change
naming of the sysfs handles, then it's probably a question of
documentation. If I
can make it more intuitive for a user who knows the sensor, but not
the internals
of IIO, then I'd prefer to use the names referred and documented in the sensor's
datasheet.

In summary, I see your point. So, I redo this patch set as I did in the initial
approach to show better what I mean. Probably I'm just using it in a wrong way.
Thank you for the feedback so far, I'll try to use it where it still applies.

> >
> > The sensor has further features still not covered:
> > - g-ranges scaled by different ODRs, especially for activity / inactivity
> >   threshold is not covered so far. There seems to be a particularity with
> >   the ADXL345 as annotated on some analog FAQ.
> >
> > - Various thinks like low power, sleep mode, etc. are (still) not covered
> >   here, others (ACDC bit, selftest, etc.) currently are hard coded or not
> >   covered.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > ---
> > Questions:
> > - Do I need a mutex/lock protection as this is the case e.g. in the ADXL367
> >   or the ADXL380?
> >   Actually, I understand those cases as protecting access to the state
> >   object by different channels, temperature and accelerometer. I'm unsure
> >   if this is a correct understanding, where for the ADXL345 there should
> >   not be any issue. At most, a currently displayed value on sysfs is
> >   (still) not updated. So, IMHO I can rely on the internal protections in
> >   regmap no further mutex is needed. Please, can you give me a feedback
> >   here?
>
> If you have an read modify write actions triggered by sysfs writes they
> can race (not serialization between different file writes).
> That's why you tend to need the mutex.
>
This explains it clearly, ty.

> >
> > - FIELD_PREP/FIELD_GET: I'd like to use arrays of enum indexed elements
> >   to allow for more generic function implementation passing just a "type"
> >   field, e.g. at single tap/double tap or activity/inactivity handling.
> >   When it comes to filtering out bits using FIELD_GET/FIELD_PREP, it says
> >   that this enum array element is not "const enough". Is there a
> >   work-around?
> I don't have it to hand but there is a patch set trying to add non
> const versions of these that went to my other email.
>
> For now just carry a local version as we don't want to end up waiting
> for that patch to merge.

I understand. I'll do another implementation approach, I'll see. Good to know,
anyway.


> >
> > Lothar Rubusch (12):
> >   iio: accel: adxl345: migrate constants to core
> >   iio: accel: adxl345: reorganize measurement enable
> >   iio: accel: adxl345: add debug register access
> >   iio: accel: adxl345: reorganize irq handler
> >   iio: accel: adxl345: improve access to the interrupt enable register
> >   iio: accel: adxl345: add single tap feature
> >   iio: accel: adxl345: show tap status and direction
> >   iio: accel: adxl345: add double tap feature
> >   iio: accel: adxl345: add double tap suppress bit
> >   iio: accel: adxl345: add freefall feature
> >   iio: accel: adxl345: add activity feature
> >   iio: accel: adxl345: add inactivity feature
> >
> >  drivers/iio/accel/adxl345.h      |   86 ---
> >  drivers/iio/accel/adxl345_core.c | 1150 ++++++++++++++++++++++++++++--
> >  2 files changed, 1099 insertions(+), 137 deletions(-)
> >
>

Re: [PATCH v1 00/12] iio: accel: adxl345: add interrupt based sensor events

[Jonathan Cameron]

On Tue, 4 Feb 2025 14:40:17 +0100
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Hi Jonathan, please find my answers below.
> 
> On Sat, Feb 1, 2025 at 6:48 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Tue, 28 Jan 2025 12:00:48 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >  
> > > Add several interrupt based sensor detection events:
> > > - single tap
> > > - double tap
> > > - free fall
> > > - activity
> > > - inactivity
> > >
> > > All the needed parameters for each and methods of adjusting them, and
> > > forward a resulting IIO event for each to the IIO channel.  
> >
> > So my main feedback here is to be much more reluctant to add new ABI.
> > Anything you add is unused by all existing code and if it is unique
> > to a driver probably never going to be used by anyone other than you.
> >
> > We have a bunch of accelerometers in tree and as they go wrt to events
> > this one isn't even particularly complex.  The existing ABI covers
> > their events reasonably well so take a look at how they do it.  Often
> > it's a case of mapping names for the application of an event (free fall
> > detection, activity detection etc) to what what they are actually detecting.
> > Those generalize a lot better across different sensor types.  It's almost
> > always a threshold of some type. The tap / double tap are more complex
> > but we put quite a lot of effort into coming up with a general
> > description a year or so back.  There may new things but most of the
> > ABI is already there.
> >  
> Please, understand my patches in "v1" rather as a huge set of questions,
> than a proposal of reinventing the IIO ABI :)
> My intention is actually not to extend/rewrite the ABI. It rather shows my lack
> of knowledge combined with the curiosity of how to actually use the
> (IIO) APIs for
> such implementation. I know this is still tedious, but I'm sure it will become
> better.
> 
> My dilemma was/is the following: I did an initial implementation more similar
> to e.g. ADXL380 for such events, and the sca3000.c for the freefall event.
> IMHO the names for the sysfs handles were not at all intuitively mappable to the
> fields I liked to operate, such as tap duration, window, latent, etc.
> The other alternative I saw, was setting up sysfs myself, IMHO clearer naming
> but actually not really using IIO's event_config/event_value.
> 
> Personally, I don't have any preference. If there really is no way to change
> naming of the sysfs handles, then it's probably a question of
> documentation. If I
> can make it more intuitive for a user who knows the sensor, but not
> the internals
> of IIO, then I'd prefer to use the names referred and documented in the sensor's
> datasheet.

ABI has to be consistent across lots of different device types.  Things
that on accelerometers mean freefall are totally different for ADCs etc.
That's one of the challenges of an ABI that is meant to cover many devices.
There is also the challenge that one devices idea of how to detect what seems
like a common thing can be totally different to another with controls
that don't line up at all.  By targetting what is actually being measured
rather than what the device datasheet says it is for, we tend to get better
generalisation and more meaningful control parameters.  It gets fiddly
around things that are sort of dumb classifiers like tap detectors
or more complex activity classifiers as we don't always get useful
documentation on what the controls are doing.

Many users will make use of userspace libraries. If they are focused on
just accelerometers they get to present an interface to the next layer
up that is accelerometer specific.


> 
> In summary, I see your point. So, I redo this patch set as I did in the initial
> approach to show better what I mean. Probably I'm just using it in a wrong way.
> Thank you for the feedback so far, I'll try to use it where it still applies.

Feel free to send out an ABI RFC in cases where it's not obvious.  That
can save some time by getting at least outline agreement on the direction
before code is ready.


Jonathan