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

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

[Lothar Rubusch]

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

Since activity and inactivity here are implemented covering all axis, I
assumed x&y&z. Thus the driver uses a fake channel for activity/inactiviy.

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

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

 Documentation/iio/adxl313.rst    | 196 ++++++++++
 Documentation/iio/index.rst      |   1 +
 drivers/iio/accel/adxl313.h      |  35 +-
 drivers/iio/accel/adxl313_core.c | 625 ++++++++++++++++++++++++++++++-
 drivers/iio/accel/adxl313_i2c.c  |   6 +
 drivers/iio/accel/adxl313_spi.c  |   6 +
 6 files changed, 859 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/iio/adxl313.rst

-- 
2.39.5

[PATCH v3 01/12] iio: accel: adxl313: add debug register

[Lothar Rubusch]

Add iio debug register for general sensor debugging.

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

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

[PATCH v3 02/12] iio: accel: adxl313: introduce channel scan_index

[Lothar Rubusch]

Add a scan_mask and scan_index to the iio channel. The scan_index
prepares the buffer usage.

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

diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
index 2f26da5857d4..9c2f3af1d19f 100644
--- a/drivers/iio/accel/adxl313_core.c
+++ b/drivers/iio/accel/adxl313_core.c
@@ -171,9 +171,10 @@ static const int adxl313_odr_freqs[][2] = {
 	[9] = { 3200, 0 },
 };
 
-#define ADXL313_ACCEL_CHANNEL(index, axis) {				\
+#define ADXL313_ACCEL_CHANNEL(index, reg, axis) {			\
 	.type = IIO_ACCEL,						\
-	.address = index,						\
+	.scan_index = (index),						\
+	.address = (reg),						\
 	.modified = 1,							\
 	.channel2 = IIO_MOD_##axis,					\
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
@@ -187,10 +188,19 @@ static const int adxl313_odr_freqs[][2] = {
 	},								\
 }
 
+enum adxl313_chans {
+	chan_x, chan_y, chan_z,
+};
+
 static const struct iio_chan_spec adxl313_channels[] = {
-	ADXL313_ACCEL_CHANNEL(0, X),
-	ADXL313_ACCEL_CHANNEL(1, Y),
-	ADXL313_ACCEL_CHANNEL(2, Z),
+	ADXL313_ACCEL_CHANNEL(0, chan_x, X),
+	ADXL313_ACCEL_CHANNEL(1, chan_y, Y),
+	ADXL313_ACCEL_CHANNEL(2, chan_z, Z),
+};
+
+static const unsigned long adxl313_scan_masks[] = {
+	BIT(chan_x) | BIT(chan_y) | BIT(chan_z),
+	0
 };
 
 static int adxl313_set_odr(struct adxl313_data *data,
@@ -419,6 +429,7 @@ int adxl313_core_probe(struct device *dev,
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->channels = adxl313_channels;
 	indio_dev->num_channels = ARRAY_SIZE(adxl313_channels);
+	indio_dev->available_scan_masks = adxl313_scan_masks;
 
 	ret = adxl313_setup(dev, data, setup);
 	if (ret) {
-- 
2.39.5

Re: [PATCH v3 02/12] iio: accel: adxl313: introduce channel scan_index

[Jonathan Cameron]

On Fri, 23 May 2025 22:35:13 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add a scan_mask and scan_index to the iio channel. The scan_index
> prepares the buffer usage.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>

Squash stuff that is only needed for buffer usage into the patch
that adds buffered support.  This is a case where I'm not convinced
the code is complex enough to warrant a multi step approach.


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

[PATCH v3 03/12] iio: accel: adxl313: configure scan type for buffer

[Lothar Rubusch]

According to the datasheet the ADXL313 uses 13 bit in full resolution.
Also, add signedness, storage bits and endianness.

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

diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
index 9c2f3af1d19f..06a771bb4726 100644
--- a/drivers/iio/accel/adxl313_core.c
+++ b/drivers/iio/accel/adxl313_core.c
@@ -184,7 +184,10 @@ static const int adxl313_odr_freqs[][2] = {
 	.info_mask_shared_by_type_available =				\
 		BIT(IIO_CHAN_INFO_SAMP_FREQ),				\
 	.scan_type = {							\
+		.sign = 's',						\
 		.realbits = 13,						\
+		.storagebits = 16,					\
+		.endianness = IIO_BE,					\
 	},								\
 }
 
-- 
2.39.5

Re: [PATCH v3 03/12] iio: accel: adxl313: configure scan type for buffer

[Jonathan Cameron]

On Fri, 23 May 2025 22:35:14 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> According to the datasheet the ADXL313 uses 13 bit in full resolution.
> Also, add signedness, storage bits and endianness.

Another one to squash with the patch that introduces buffered support.

> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl313_core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
> index 9c2f3af1d19f..06a771bb4726 100644
> --- a/drivers/iio/accel/adxl313_core.c
> +++ b/drivers/iio/accel/adxl313_core.c
> @@ -184,7 +184,10 @@ static const int adxl313_odr_freqs[][2] = {
>  	.info_mask_shared_by_type_available =				\
>  		BIT(IIO_CHAN_INFO_SAMP_FREQ),				\
>  	.scan_type = {							\
> +		.sign = 's',						\
>  		.realbits = 13,						\
> +		.storagebits = 16,					\
> +		.endianness = IIO_BE,					\
>  	},								\
>  }
>  

[PATCH v3 04/12] iio: accel: adxl313: make use of regmap cache

[Lothar Rubusch]

Setup regmap cache to cache register configuration. This is a preparatory
step for follow up patches, to allow easy acces to the cached
configuration.

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

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

Re: [PATCH v3 04/12] iio: accel: adxl313: make use of regmap cache

[Jonathan Cameron]

On Fri, 23 May 2025 22:35:15 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Setup regmap cache to cache register configuration. This is a preparatory
> step for follow up patches, to allow easy acces to the cached
> configuration.

I think this stands on it's own given registers like the calibbias
are already both written and read from.  So I'd generalize the justification
to simply reducing unnecessary bus traffic.

Jonathan

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

Re: [PATCH v3 04/12] iio: accel: adxl313: make use of regmap cache

[Lothar Rubusch]

Hi,

On Sun, May 25, 2025 at 2:22 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri, 23 May 2025 22:35:15 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Setup regmap cache to cache register configuration. This is a preparatory
> > step for follow up patches, to allow easy acces to the cached
> > configuration.
>
> I think this stands on it's own given registers like the calibbias
> are already both written and read from.  So I'd generalize the justification
> to simply reducing unnecessary bus traffic.
>

I (think I) need regmap cache especially for activity / inactivity.
For instance, using cached settings should make it easier to verify
what was enabled when evaluating incomming interrupts. I will rework
the commit message here.

Best,
L

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

[PATCH v3 05/12] iio: accel: adxl313: add function to enable measurement

[Lothar Rubusch]

Add a function to enable measurement. The data-sheet recomments turning of
measurement while modifying certain config registers. This is a preparatory
step.

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

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

Re: [PATCH v3 05/12] iio: accel: adxl313: add function to enable measurement

[Jonathan Cameron]

On Fri, 23 May 2025 22:35:16 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add a function to enable measurement. The data-sheet recomments turning of
> measurement while modifying certain config registers. This is a preparatory
> step.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl313.h      |  3 +--
>  drivers/iio/accel/adxl313_core.c | 10 +++++++---
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl313.h b/drivers/iio/accel/adxl313.h
> index fc937bdf83b6..9bf2facdbf87 100644
> --- a/drivers/iio/accel/adxl313.h
> +++ b/drivers/iio/accel/adxl313.h
> @@ -36,8 +36,7 @@
>  #define ADXL313_RATE_MSK		GENMASK(3, 0)
>  #define ADXL313_RATE_BASE		6
>  
> -#define ADXL313_POWER_CTL_MSK		GENMASK(3, 2)
> -#define ADXL313_MEASUREMENT_MODE	BIT(3)
> +#define ADXL313_POWER_CTL_MSK		BIT(3)
>  
>  #define ADXL313_RANGE_MSK		GENMASK(1, 0)
>  #define ADXL313_RANGE_MAX		3
> diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
> index 0c893c286017..6170c9daa30f 100644
> --- a/drivers/iio/accel/adxl313_core.c
> +++ b/drivers/iio/accel/adxl313_core.c
> @@ -63,6 +63,12 @@ bool adxl313_is_volatile_reg(struct device *dev, unsigned int reg)
>  }
>  EXPORT_SYMBOL_NS_GPL(adxl313_is_volatile_reg, "IIO_ADXL313");
>  
> +static int adxl313_set_measure_en(struct adxl313_data *data, bool en)
> +{
> +	return regmap_assign_bits(data->regmap, ADXL313_REG_POWER_CTL,
> +				  ADXL313_POWER_CTL_MSK, en);
> +}
> +
>  static int adxl312_check_id(struct device *dev,
>  			    struct adxl313_data *data)
>  {
> @@ -410,9 +416,7 @@ static int adxl313_setup(struct device *dev, struct adxl313_data *data,
>  	}
>  
>  	/* Enables measurement mode */
> -	return regmap_update_bits(data->regmap, ADXL313_REG_POWER_CTL,
> -				  ADXL313_POWER_CTL_MSK,
> -				  ADXL313_MEASUREMENT_MODE);
> +	return adxl313_set_measure_en(data, true);
The original code is also clearing the sleep bit.

I'd expect the patch description to have stated why no longer doing that
is fine. I guess no one ever sets it?

>  }
>  
>  /**

[PATCH v3 06/12] iio: accel: adxl313: prepare interrupt handling

[Lothar Rubusch]

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

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

diff --git a/drivers/iio/accel/adxl313.h b/drivers/iio/accel/adxl313.h
index 9bf2facdbf87..ab109d1c359e 100644
--- a/drivers/iio/accel/adxl313.h
+++ b/drivers/iio/accel/adxl313.h
@@ -21,7 +21,9 @@
 #define ADXL313_REG_ACT_INACT_CTL	0x27
 #define ADXL313_REG_BW_RATE		0x2C
 #define ADXL313_REG_POWER_CTL		0x2D
+#define ADXL313_REG_INT_ENABLE		0x2E
 #define ADXL313_REG_INT_MAP		0x2F
+#define ADXL313_REG_INT_SOURCE		0x30
 #define ADXL313_REG_DATA_FORMAT		0x31
 #define ADXL313_REG_DATA_AXIS(index)	(0x32 + ((index) * 2))
 #define ADXL313_REG_FIFO_CTL		0x38
@@ -45,6 +47,11 @@
 #define ADXL313_SPI_3WIRE		BIT(6)
 #define ADXL313_I2C_DISABLE		BIT(6)
 
+#define ADXL313_REG_FIFO_CTL_MODE_MSK		GENMASK(7, 6)
+
+#define ADXL313_FIFO_BYPASS			0
+#define ADXL313_FIFO_STREAM			2
+
 extern const struct regmap_access_table adxl312_readable_regs_table;
 extern const struct regmap_access_table adxl313_readable_regs_table;
 extern const struct regmap_access_table adxl314_readable_regs_table;
@@ -65,6 +72,7 @@ struct adxl313_data {
 	struct regmap	*regmap;
 	const struct adxl313_chip_info *chip_info;
 	struct mutex	lock; /* lock to protect transf_buf */
+	int irq;
 	__le16		transf_buf __aligned(IIO_DMA_MINALIGN);
 };
 
diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
index 6170c9daa30f..9db318a03eea 100644
--- a/drivers/iio/accel/adxl313_core.c
+++ b/drivers/iio/accel/adxl313_core.c
@@ -8,11 +8,17 @@
  */
 
 #include <linux/bitfield.h>
+#include <linux/interrupt.h>
 #include <linux/module.h>
+#include <linux/property.h>
 #include <linux/regmap.h>
 
 #include "adxl313.h"
 
+#define ADXL313_INT_NONE			U8_MAX
+#define ADXL313_INT1				1
+#define ADXL313_INT2				2
+
 static const struct regmap_range adxl312_readable_reg_range[] = {
 	regmap_reg_range(ADXL313_REG_DEVID0, ADXL313_REG_DEVID0),
 	regmap_reg_range(ADXL313_REG_OFS_AXIS(0), ADXL313_REG_OFS_AXIS(2)),
@@ -436,6 +442,7 @@ int adxl313_core_probe(struct device *dev,
 {
 	struct adxl313_data *data;
 	struct iio_dev *indio_dev;
+	u8 int_line;
 	int ret;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
@@ -461,6 +468,30 @@ int adxl313_core_probe(struct device *dev,
 		return ret;
 	}
 
+	int_line = ADXL313_INT1;
+	data->irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT1");
+	if (data->irq < 0) {
+		int_line = ADXL313_INT2;
+		data->irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT2");
+		if (data->irq < 0)
+			int_line = ADXL313_INT_NONE;
+	}
+
+	if (int_line == ADXL313_INT1 || int_line == ADXL313_INT2) {
+		/* FIFO_STREAM mode */
+		ret = regmap_assign_bits(data->regmap, ADXL313_REG_INT_MAP,
+					 0xff, int_line == ADXL313_INT2);
+		if (ret)
+			return ret;
+	} else {
+		/* FIFO_BYPASSED mode */
+		ret = regmap_write(data->regmap, ADXL313_REG_FIFO_CTL,
+				   FIELD_PREP(ADXL313_REG_FIFO_CTL_MODE_MSK,
+					      ADXL313_FIFO_BYPASS));
+		if (ret)
+			return ret;
+	}
+
 	return devm_iio_device_register(dev, indio_dev);
 }
 EXPORT_SYMBOL_NS_GPL(adxl313_core_probe, IIO_ADXL313);
-- 
2.39.5

Re: [PATCH v3 06/12] iio: accel: adxl313: prepare interrupt handling

[Jonathan Cameron]

On Fri, 23 May 2025 22:35:17 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Evaluate the devicetree property for an optional interrupt line, and
> configure the interrupt mapping accordingly. When no interrupt line
> is defined in the devicetree, keep the FIFO in bypass mode as before.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl313.h      |  8 ++++++++
>  drivers/iio/accel/adxl313_core.c | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 39 insertions(+)
> 
> diff --git a/drivers/iio/accel/adxl313.h b/drivers/iio/accel/adxl313.h
> index 9bf2facdbf87..ab109d1c359e 100644
> --- a/drivers/iio/accel/adxl313.h
> +++ b/drivers/iio/accel/adxl313.h
> @@ -21,7 +21,9 @@
>  #define ADXL313_REG_ACT_INACT_CTL	0x27
>  #define ADXL313_REG_BW_RATE		0x2C
>  #define ADXL313_REG_POWER_CTL		0x2D
> +#define ADXL313_REG_INT_ENABLE		0x2E
>  #define ADXL313_REG_INT_MAP		0x2F
> +#define ADXL313_REG_INT_SOURCE		0x30
>  #define ADXL313_REG_DATA_FORMAT		0x31
>  #define ADXL313_REG_DATA_AXIS(index)	(0x32 + ((index) * 2))
>  #define ADXL313_REG_FIFO_CTL		0x38
> @@ -45,6 +47,11 @@
>  #define ADXL313_SPI_3WIRE		BIT(6)
>  #define ADXL313_I2C_DISABLE		BIT(6)
>  
> +#define ADXL313_REG_FIFO_CTL_MODE_MSK		GENMASK(7, 6)
> +
> +#define ADXL313_FIFO_BYPASS			0
> +#define ADXL313_FIFO_STREAM			2
> +
>  extern const struct regmap_access_table adxl312_readable_regs_table;
>  extern const struct regmap_access_table adxl313_readable_regs_table;
>  extern const struct regmap_access_table adxl314_readable_regs_table;
> @@ -65,6 +72,7 @@ struct adxl313_data {
>  	struct regmap	*regmap;
>  	const struct adxl313_chip_info *chip_info;
>  	struct mutex	lock; /* lock to protect transf_buf */
> +	int irq;

Curious.  Why do we need to keep this around?  Normally we only need
the actual interrupt number in the probe() function.

>  	__le16		transf_buf __aligned(IIO_DMA_MINALIGN);
>  };
>  
> diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
> index 6170c9daa30f..9db318a03eea 100644
> --- a/drivers/iio/accel/adxl313_core.c
> +++ b/drivers/iio/accel/adxl313_core.c
> @@ -8,11 +8,17 @@
>   */
>  
>  #include <linux/bitfield.h>
> +#include <linux/interrupt.h>
>  #include <linux/module.h>
> +#include <linux/property.h>
>  #include <linux/regmap.h>
>  
>  #include "adxl313.h"
>  
> +#define ADXL313_INT_NONE			U8_MAX
> +#define ADXL313_INT1				1
> +#define ADXL313_INT2				2
> +
>  static const struct regmap_range adxl312_readable_reg_range[] = {
>  	regmap_reg_range(ADXL313_REG_DEVID0, ADXL313_REG_DEVID0),
>  	regmap_reg_range(ADXL313_REG_OFS_AXIS(0), ADXL313_REG_OFS_AXIS(2)),
> @@ -436,6 +442,7 @@ int adxl313_core_probe(struct device *dev,
>  {
>  	struct adxl313_data *data;
>  	struct iio_dev *indio_dev;
> +	u8 int_line;
>  	int ret;
>  
>  	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> @@ -461,6 +468,30 @@ int adxl313_core_probe(struct device *dev,
>  		return ret;
>  	}
>  
> +	int_line = ADXL313_INT1;
> +	data->irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT1");
> +	if (data->irq < 0) {
> +		int_line = ADXL313_INT2;
> +		data->irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT2");
> +		if (data->irq < 0)
> +			int_line = ADXL313_INT_NONE;
> +	}
> +
> +	if (int_line == ADXL313_INT1 || int_line == ADXL313_INT2) {

Why not int_line != ADXL313_INT_NONE ?
Or flip the logic so that you do that case first.

> +		/* FIFO_STREAM mode */
> +		ret = regmap_assign_bits(data->regmap, ADXL313_REG_INT_MAP,

A number of bits in this register are give in datasheet as always 0.
As a general rule writing bits documented like that is unwise. Sometimes
they have undocumented side effects.

> +					 0xff, int_line == ADXL313_INT2);
> +		if (ret)
> +			return ret;
> +	} else {
> +		/* FIFO_BYPASSED mode */

I'd like the comment to say why you bypass the fifo in this case.
In theory nothing stops us polling for the watermark. I don't mind
the driver not doing that because all reasonable boards will wire
the interrupt if they want fifo support, but we should talk a little
more about why here.

> +		ret = regmap_write(data->regmap, ADXL313_REG_FIFO_CTL,
> +				   FIELD_PREP(ADXL313_REG_FIFO_CTL_MODE_MSK,
> +					      ADXL313_FIFO_BYPASS));
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return devm_iio_device_register(dev, indio_dev);
>  }
>  EXPORT_SYMBOL_NS_GPL(adxl313_core_probe, IIO_ADXL313);

[PATCH v3 07/12] iio: accel: adxl313: add basic interrupt handling

[Lothar Rubusch]

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

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

diff --git a/drivers/iio/accel/adxl313.h b/drivers/iio/accel/adxl313.h
index ab109d1c359e..9cdad55dd350 100644
--- a/drivers/iio/accel/adxl313.h
+++ b/drivers/iio/accel/adxl313.h
@@ -47,11 +47,25 @@
 #define ADXL313_SPI_3WIRE		BIT(6)
 #define ADXL313_I2C_DISABLE		BIT(6)
 
+#define ADXL313_INT_OVERRUN		BIT(0)
+#define ADXL313_INT_WATERMARK		BIT(1)
+#define ADXL313_INT_INACTIVITY		BIT(3)
+#define ADXL313_INT_ACTIVITY		BIT(4)
+#define ADXL313_INT_DREADY		BIT(7)
+
+/* FIFO entries: how many values are stored in the FIFO */
+#define ADXL313_REG_FIFO_STATUS_ENTRIES_MSK	GENMASK(5, 0)
+/* FIFO samples: number of samples needed for watermark (FIFO mode) */
+#define ADXL313_REG_FIFO_CTL_SAMPLES_MSK	GENMASK(4, 0)
 #define ADXL313_REG_FIFO_CTL_MODE_MSK		GENMASK(7, 6)
 
 #define ADXL313_FIFO_BYPASS			0
 #define ADXL313_FIFO_STREAM			2
 
+#define ADXL313_FIFO_SIZE			32
+
+#define ADXL313_NUM_AXIS			3
+
 extern const struct regmap_access_table adxl312_readable_regs_table;
 extern const struct regmap_access_table adxl313_readable_regs_table;
 extern const struct regmap_access_table adxl314_readable_regs_table;
@@ -73,7 +87,9 @@ struct adxl313_data {
 	const struct adxl313_chip_info *chip_info;
 	struct mutex	lock; /* lock to protect transf_buf */
 	int irq;
+	u8 fifo_mode;
 	__le16		transf_buf __aligned(IIO_DMA_MINALIGN);
+	__le16		fifo_buf[ADXL313_NUM_AXIS * ADXL313_FIFO_SIZE + 1];
 };
 
 struct adxl313_chip_info {
diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
index 9db318a03eea..1e085f0c61a0 100644
--- a/drivers/iio/accel/adxl313_core.c
+++ b/drivers/iio/accel/adxl313_core.c
@@ -10,15 +10,24 @@
 #include <linux/bitfield.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
+#include <linux/overflow.h>
 #include <linux/property.h>
 #include <linux/regmap.h>
 
+#include <linux/iio/buffer.h>
+#include <linux/iio/events.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/kfifo_buf.h>
+#include <linux/iio/sysfs.h>
+
 #include "adxl313.h"
 
 #define ADXL313_INT_NONE			U8_MAX
 #define ADXL313_INT1				1
 #define ADXL313_INT2				2
 
+#define ADXL313_REG_XYZ_BASE			ADXL313_REG_DATA_AXIS(0)
+
 static const struct regmap_range adxl312_readable_reg_range[] = {
 	regmap_reg_range(ADXL313_REG_DEVID0, ADXL313_REG_DEVID0),
 	regmap_reg_range(ADXL313_REG_OFS_AXIS(0), ADXL313_REG_OFS_AXIS(2)),
@@ -62,6 +71,7 @@ bool adxl313_is_volatile_reg(struct device *dev, unsigned int reg)
 	case ADXL313_REG_DATA_AXIS(4):
 	case ADXL313_REG_DATA_AXIS(5):
 	case ADXL313_REG_FIFO_STATUS:
+	case ADXL313_REG_INT_SOURCE:
 		return true;
 	default:
 		return false;
@@ -363,6 +373,118 @@ static int adxl313_write_raw(struct iio_dev *indio_dev,
 	}
 }
 
+static int adxl313_get_samples(struct adxl313_data *data)
+{
+	unsigned int regval = 0;
+	int ret;
+
+	ret = regmap_read(data->regmap, ADXL313_REG_FIFO_STATUS, &regval);
+	if (ret)
+		return ret;
+
+	return FIELD_GET(ADXL313_REG_FIFO_STATUS_ENTRIES_MSK, regval);
+}
+
+static int adxl313_set_fifo(struct adxl313_data *data)
+{
+	unsigned int int_line;
+	int ret;
+
+	ret = adxl313_set_measure_en(data, false);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(data->regmap, ADXL313_REG_INT_MAP, &int_line);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(data->regmap, ADXL313_REG_FIFO_CTL,
+			   FIELD_PREP(ADXL313_REG_FIFO_CTL_MODE_MSK, data->fifo_mode));
+
+	return adxl313_set_measure_en(data, true);
+}
+
+static int adxl313_fifo_transfer(struct adxl313_data *data, int samples)
+{
+	size_t count;
+	unsigned int i;
+	int ret;
+
+	count = array_size(sizeof(data->fifo_buf[0]), ADXL313_NUM_AXIS);
+	for (i = 0; i < samples; i++) {
+		ret = regmap_bulk_read(data->regmap, ADXL313_REG_XYZ_BASE,
+				       data->fifo_buf + (i * count / 2), count);
+		if (ret)
+			return ret;
+	}
+	return 0;
+}
+
+/**
+ * adxl313_fifo_reset() - Reset the FIFO and interrupt status registers.
+ * @data: The device data.
+ *
+ * Reset the FIFO status registers. Reading out status registers clears the
+ * FIFO and interrupt configuration. Thus do not evaluate regmap return values.
+ * Ignore particular read register content. Register content is not processed
+ * any further. Therefore the function returns void.
+ */
+static void adxl313_fifo_reset(struct adxl313_data *data)
+{
+	unsigned int regval;
+	int samples;
+
+	adxl313_set_measure_en(data, false);
+
+	samples = adxl313_get_samples(data);
+	if (samples > 0)
+		adxl313_fifo_transfer(data, samples);
+
+	regmap_read(data->regmap, ADXL313_REG_INT_SOURCE, &regval);
+
+	adxl313_set_measure_en(data, true);
+}
+
+static int adxl313_buffer_postenable(struct iio_dev *indio_dev)
+{
+	struct adxl313_data *data = iio_priv(indio_dev);
+
+	data->fifo_mode = ADXL313_FIFO_STREAM;
+	return adxl313_set_fifo(data);
+}
+
+static int adxl313_buffer_predisable(struct iio_dev *indio_dev)
+{
+	struct adxl313_data *data = iio_priv(indio_dev);
+	int ret;
+
+	data->fifo_mode = ADXL313_FIFO_BYPASS;
+	ret = adxl313_set_fifo(data);
+	if (ret)
+		return ret;
+
+	return regmap_write(data->regmap, ADXL313_REG_INT_ENABLE, 0);
+}
+
+static const struct iio_buffer_setup_ops adxl313_buffer_ops = {
+	.postenable = adxl313_buffer_postenable,
+	.predisable = adxl313_buffer_predisable,
+};
+
+static irqreturn_t adxl313_irq_handler(int irq, void *p)
+{
+	struct iio_dev *indio_dev = p;
+	struct adxl313_data *data = iio_priv(indio_dev);
+	int int_stat;
+
+	if (regmap_read(data->regmap, ADXL313_REG_INT_SOURCE, &int_stat))
+		return IRQ_NONE;
+
+	adxl313_fifo_reset(data);
+
+	return IRQ_HANDLED;
+}
+
 static int adxl313_reg_access(struct iio_dev *indio_dev, unsigned int reg,
 			      unsigned int writeval, unsigned int *readval)
 {
@@ -483,6 +605,18 @@ int adxl313_core_probe(struct device *dev,
 					 0xff, int_line == ADXL313_INT2);
 		if (ret)
 			return ret;
+
+		ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
+						  &adxl313_buffer_ops);
+		if (ret)
+			return ret;
+
+		ret = devm_request_threaded_irq(dev, data->irq, NULL,
+						&adxl313_irq_handler,
+						IRQF_SHARED | IRQF_ONESHOT,
+						indio_dev->name, indio_dev);
+		if (ret)
+			return ret;
 	} else {
 		/* FIFO_BYPASSED mode */
 		ret = regmap_write(data->regmap, ADXL313_REG_FIFO_CTL,
-- 
2.39.5

Re: [PATCH v3 07/12] iio: accel: adxl313: add basic interrupt handling

[Jonathan Cameron]

On Fri, 23 May 2025 22:35:18 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Prepare the interrupt handler. Add register entries to evaluate the
> incoming interrupt. Add functions to clear status registers and reset the
> FIFO.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
Hi Lothar,

A few comments inline.

> ---
>  drivers/iio/accel/adxl313.h      |  16 ++++
>  drivers/iio/accel/adxl313_core.c | 134 +++++++++++++++++++++++++++++++
>  2 files changed, 150 insertions(+)



>  struct adxl313_chip_info {
> diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
> index 9db318a03eea..1e085f0c61a0 100644
> --- a/drivers/iio/accel/adxl313_core.c
> +++ b/drivers/iio/accel/adxl313_core.c
> @@ -10,15 +10,24 @@
>  #include <linux/bitfield.h>
>  #include <linux/interrupt.h>
>  #include <linux/module.h>
> +#include <linux/overflow.h>
>  #include <linux/property.h>
>  #include <linux/regmap.h>
>  
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/kfifo_buf.h>
> +#include <linux/iio/sysfs.h>

This is an odd selection of headers to add now. Why do we need them but didn't
before?  Some of these aren't used yet so drop them (events.h, sysfs.h I think)

> +

>  static const struct regmap_range adxl312_readable_reg_range[] = {
>  	regmap_reg_range(ADXL313_REG_DEVID0, ADXL313_REG_DEVID0),
>  	regmap_reg_range(ADXL313_REG_OFS_AXIS(0), ADXL313_REG_OFS_AXIS(2)),
> @@ -62,6 +71,7 @@ bool adxl313_is_volatile_reg(struct device *dev, unsigned int reg)
>  	case ADXL313_REG_DATA_AXIS(4):
>  	case ADXL313_REG_DATA_AXIS(5):
>  	case ADXL313_REG_FIFO_STATUS:
> +	case ADXL313_REG_INT_SOURCE:
>  		return true;
>  	default:
>  		return false;
> @@ -363,6 +373,118 @@ static int adxl313_write_raw(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +static int adxl313_get_samples(struct adxl313_data *data)

I doubt this gets called from multiple places. I'd just put
the code inline and no have this helper at all.

> +{
> +	unsigned int regval = 0;
> +	int ret;
> +
> +	ret = regmap_read(data->regmap, ADXL313_REG_FIFO_STATUS, &regval);
> +	if (ret)
> +		return ret;
> +
> +	return FIELD_GET(ADXL313_REG_FIFO_STATUS_ENTRIES_MSK, regval);
> +}
> +
> +static int adxl313_set_fifo(struct adxl313_data *data)
> +{
> +	unsigned int int_line;
> +	int ret;
> +
> +	ret = adxl313_set_measure_en(data, false);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(data->regmap, ADXL313_REG_INT_MAP, &int_line);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(data->regmap, ADXL313_REG_FIFO_CTL,
> +			   FIELD_PREP(ADXL313_REG_FIFO_CTL_MODE_MSK, data->fifo_mode));

Check ret.

> +
> +	return adxl313_set_measure_en(data, true);
> +}
> +
> +static int adxl313_fifo_transfer(struct adxl313_data *data, int samples)
> +{
> +	size_t count;
> +	unsigned int i;
> +	int ret;
> +
> +	count = array_size(sizeof(data->fifo_buf[0]), ADXL313_NUM_AXIS);
> +	for (i = 0; i < samples; i++) {
> +		ret = regmap_bulk_read(data->regmap, ADXL313_REG_XYZ_BASE,
> +				       data->fifo_buf + (i * count / 2), count);

that 2 is I'd guessed based on size of some data store element?  
I'd guess sizeof(data->fifo_buf[0]) is appropriate.


> +		if (ret)
> +			return ret;
> +	}
> +	return 0;
> +}
> +
> +/**
> + * adxl313_fifo_reset() - Reset the FIFO and interrupt status registers.
> + * @data: The device data.
> + *
> + * Reset the FIFO status registers. Reading out status registers clears the

I think you already read it before calling this. So how is it ever set?

> + * FIFO and interrupt configuration. Thus do not evaluate regmap return values.
> + * Ignore particular read register content. Register content is not processed
> + * any further. Therefore the function returns void.
> + */
> +static void adxl313_fifo_reset(struct adxl313_data *data)

As below.  This isn't a reset.  Fifo reset is normally the term used
for when we have lost tracking of what is in the fifo and drop all data,
not normal readback.

> +{
> +	unsigned int regval;
> +	int samples;
> +
> +	adxl313_set_measure_en(data, false);
Disabling measurement to read a fifo is unusual -  is this really necessary
as it presumably puts a gap in the data, which is what we are trying
to avoid by using a fifo.

> +
> +	samples = adxl313_get_samples(data);
> +	if (samples > 0)
> +		adxl313_fifo_transfer(data, samples);
> +
> +	regmap_read(data->regmap, ADXL313_REG_INT_SOURCE, &regval);

Not processing the convents of INT_SOURCE every time you read it
introduces race conditions.  This logic needs a rethink so that
never happens.  I guess this is why you are disabling measurement
to stop the status changing?  Just whatever each read of INT_SOURCE
tells us we need to handle and all should be fine without disabling
measurement.  That read should only clear bits that are set, so no
race conditions.

> +
> +	adxl313_set_measure_en(data, true);
> +}
> +
> +static int adxl313_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct adxl313_data *data = iio_priv(indio_dev);
> +
> +	data->fifo_mode = ADXL313_FIFO_STREAM;

If you always set fifo_mode before calling _set_fifo() probably better
to pass the value in as a separate parameter and store it as necessary
inside that function.

> +	return adxl313_set_fifo(data);
> +}
> +
> +static int adxl313_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct adxl313_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	data->fifo_mode = ADXL313_FIFO_BYPASS;
> +	ret = adxl313_set_fifo(data);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(data->regmap, ADXL313_REG_INT_ENABLE, 0);
> +}
> +
> +static const struct iio_buffer_setup_ops adxl313_buffer_ops = {
> +	.postenable = adxl313_buffer_postenable,
> +	.predisable = adxl313_buffer_predisable,
> +};
> +
> +static irqreturn_t adxl313_irq_handler(int irq, void *p)
> +{
> +	struct iio_dev *indio_dev = p;
> +	struct adxl313_data *data = iio_priv(indio_dev);
> +	int int_stat;
> +
> +	if (regmap_read(data->regmap, ADXL313_REG_INT_SOURCE, &int_stat))

Failure to read is one thing we should handle, but also we should handle
int_stat telling us there were no interrupts set for this device.

> +		return IRQ_NONE;
> +
> +	adxl313_fifo_reset(data);

Given we don't know it had anything to do with the fifo at this point
resetting the fifo doesn't make much sense.  I'd expect a check
on int_status, probably for overrun, before doing this.

Ah. On closer inspection this isn't resetting the fifo, it's just
reading it.  Rename that function and make it dependent on what
was in int_stat.


> +
> +	return IRQ_HANDLED;
> +}

Re: [PATCH v3 07/12] iio: accel: adxl313: add basic interrupt handling

[Jonathan Cameron]

> > +/**
> > + * adxl313_fifo_reset() - Reset the FIFO and interrupt status registers.
> > + * @data: The device data.
> > + *
> > + * Reset the FIFO status registers. Reading out status registers clears the  
> 
> I think you already read it before calling this. So how is it ever set?
> 
> > + * FIFO and interrupt configuration. Thus do not evaluate regmap return values.
> > + * Ignore particular read register content. Register content is not processed
> > + * any further. Therefore the function returns void.
> > + */
> > +static void adxl313_fifo_reset(struct adxl313_data *data)  
> 
> As below.  This isn't a reset.  Fifo reset is normally the term used
> for when we have lost tracking of what is in the fifo and drop all data,
> not normal readback.

Ok. After next patch it became more obvious how this is being used.

I'd combine the patches probably to avoid confusion or the odd state
that any interrupt resets the fifo after this patch.

> 
> > +{
> > +	unsigned int regval;
> > +	int samples;
> > +
> > +	adxl313_set_measure_en(data, false);  
> Disabling measurement to read a fifo is unusual -  is this really necessary
> as it presumably puts a gap in the data, which is what we are trying
> to avoid by using a fifo.

This makes more sense as well if we are in a reset condition.
I just got thrown off by the lack of a 'good' path in this patch.

> 
> > +
> > +	samples = adxl313_get_samples(data);
> > +	if (samples > 0)
> > +		adxl313_fifo_transfer(data, samples);
> > +
> > +	regmap_read(data->regmap, ADXL313_REG_INT_SOURCE, &regval);  
> 
> Not processing the convents of INT_SOURCE every time you read it
> introduces race conditions.  This logic needs a rethink so that
> never happens.  I guess this is why you are disabling measurement
> to stop the status changing?  Just whatever each read of INT_SOURCE
> tells us we need to handle and all should be fine without disabling
> measurement.  That read should only clear bits that are set, so no
> race conditions.
> 
> > +
> > +	adxl313_set_measure_en(data, true);
> > +}

Re: [PATCH v3 07/12] iio: accel: adxl313: add basic interrupt handling

[Lothar Rubusch]

Hi Jonathan,

I feel here either I have some missunderstanding or it needs more
(better?) explanation. Perhaps I'm using the wrong terminology.

One point, I forgot, do I actually need to add a Reviewed-by tag or
something like that for Andys review? Or if so, they will let me know,
I guess?

First of all, introducing the adxl313_fifo_reset(data) here is the
crucial part. So, the title I chose is not matching with the topic, or
is too general. I'll answer and try to better explain down below.

On Sun, May 25, 2025 at 2:48 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri, 23 May 2025 22:35:18 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Prepare the interrupt handler. Add register entries to evaluate the
> > incoming interrupt. Add functions to clear status registers and reset the
> > FIFO.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> Hi Lothar,
>
> A few comments inline.
>
> > ---
> >  drivers/iio/accel/adxl313.h      |  16 ++++
> >  drivers/iio/accel/adxl313_core.c | 134 +++++++++++++++++++++++++++++++
> >  2 files changed, 150 insertions(+)
>
>
>
> >  struct adxl313_chip_info {
> > diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
> > index 9db318a03eea..1e085f0c61a0 100644
> > --- a/drivers/iio/accel/adxl313_core.c
> > +++ b/drivers/iio/accel/adxl313_core.c
> > @@ -10,15 +10,24 @@
> >  #include <linux/bitfield.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/module.h>
> > +#include <linux/overflow.h>
> >  #include <linux/property.h>
> >  #include <linux/regmap.h>
> >
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/events.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/kfifo_buf.h>
> > +#include <linux/iio/sysfs.h>
>
> This is an odd selection of headers to add now. Why do we need them but didn't
> before?  Some of these aren't used yet so drop them (events.h, sysfs.h I think)
>

Agree.

> > +
>
> >  static const struct regmap_range adxl312_readable_reg_range[] = {
> >       regmap_reg_range(ADXL313_REG_DEVID0, ADXL313_REG_DEVID0),
> >       regmap_reg_range(ADXL313_REG_OFS_AXIS(0), ADXL313_REG_OFS_AXIS(2)),
> > @@ -62,6 +71,7 @@ bool adxl313_is_volatile_reg(struct device *dev, unsigned int reg)
> >       case ADXL313_REG_DATA_AXIS(4):
> >       case ADXL313_REG_DATA_AXIS(5):
> >       case ADXL313_REG_FIFO_STATUS:
> > +     case ADXL313_REG_INT_SOURCE:
> >               return true;
> >       default:
> >               return false;
> > @@ -363,6 +373,118 @@ static int adxl313_write_raw(struct iio_dev *indio_dev,
> >       }
> >  }
> >
> > +static int adxl313_get_samples(struct adxl313_data *data)
>
> I doubt this gets called from multiple places. I'd just put
> the code inline and no have this helper at all.
>

It will be a called at least in two places. First, when reading the
measurements and second when clearing the fifo in the reset.

> > +{
> > +     unsigned int regval = 0;
> > +     int ret;
> > +
> > +     ret = regmap_read(data->regmap, ADXL313_REG_FIFO_STATUS, &regval);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return FIELD_GET(ADXL313_REG_FIFO_STATUS_ENTRIES_MSK, regval);
> > +}
> > +
> > +static int adxl313_set_fifo(struct adxl313_data *data)
> > +{
> > +     unsigned int int_line;
> > +     int ret;
> > +
> > +     ret = adxl313_set_measure_en(data, false);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = regmap_read(data->regmap, ADXL313_REG_INT_MAP, &int_line);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = regmap_write(data->regmap, ADXL313_REG_FIFO_CTL,
> > +                        FIELD_PREP(ADXL313_REG_FIFO_CTL_MODE_MSK, data->fifo_mode));
>
> Check ret.
>

Agree.

> > +
> > +     return adxl313_set_measure_en(data, true);
> > +}
> > +
> > +static int adxl313_fifo_transfer(struct adxl313_data *data, int samples)
> > +{
> > +     size_t count;
> > +     unsigned int i;
> > +     int ret;
> > +
> > +     count = array_size(sizeof(data->fifo_buf[0]), ADXL313_NUM_AXIS);
> > +     for (i = 0; i < samples; i++) {
> > +             ret = regmap_bulk_read(data->regmap, ADXL313_REG_XYZ_BASE,
> > +                                    data->fifo_buf + (i * count / 2), count);
>
> that 2 is I'd guessed based on size of some data store element?
> I'd guess sizeof(data->fifo_buf[0]) is appropriate.
>

My calculation was the following:
* samples := number of "lines" in the FIFO e.g. by watermark
* count := number of bytes per "line"
* ADXL313_NUM_AXIS := 3 for the three axis here
There's a bulk read per "line" of the FIFO. A "line" comprises
measurement for x, y and z axis. Each measurement consists of 2 bytes,
i.e. count has 6 bytes.

At a second look now, probably count/2 can be replaced directly by
ADXL313_NUM_AXIS. If so, I don't need the count variable. I see,
count/2 being already a constant expression here smells somehow. I
guess, this might be your point? I'll change that and need verify.

>
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +     return 0;
> > +}
> > +
> > +/**
> > + * adxl313_fifo_reset() - Reset the FIFO and interrupt status registers.
> > + * @data: The device data.
> > + *
> > + * Reset the FIFO status registers. Reading out status registers clears the
>
> I think you already read it before calling this. So how is it ever set?
>
> > + * FIFO and interrupt configuration. Thus do not evaluate regmap return values.
> > + * Ignore particular read register content. Register content is not processed
> > + * any further. Therefore the function returns void.
> > + */
> > +static void adxl313_fifo_reset(struct adxl313_data *data)
>
> As below.  This isn't a reset.  Fifo reset is normally the term used
> for when we have lost tracking of what is in the fifo and drop all data,
> not normal readback.
>
> > +{
> > +     unsigned int regval;
> > +     int samples;
> > +
> > +     adxl313_set_measure_en(data, false);
> Disabling measurement to read a fifo is unusual -  is this really necessary
> as it presumably puts a gap in the data, which is what we are trying
> to avoid by using a fifo.
>
> > +
> > +     samples = adxl313_get_samples(data);
> > +     if (samples > 0)
> > +             adxl313_fifo_transfer(data, samples);
> > +
> > +     regmap_read(data->regmap, ADXL313_REG_INT_SOURCE, &regval);
>
> Not processing the convents of INT_SOURCE every time you read it
> introduces race conditions.  This logic needs a rethink so that
> never happens.  I guess this is why you are disabling measurement
> to stop the status changing?  Just whatever each read of INT_SOURCE
> tells us we need to handle and all should be fine without disabling
> measurement.  That read should only clear bits that are set, so no
> race conditions.
>

When the ADXL345 triggers an interrupt for e.g. watermark, data ready,
or overrun,... it will stop from triggerring further interrupts until
the status registers, INT_SOURCE and FIFO_STATUS are cleared. This I
call "reset". In consequence the FIFO will simply run full.

Usually when the interrupt handler reads the interrupt status
(INT_SOURCE). In case of, say, watermark, it then reads the
FIFO_STATUS to obtain number of entries and reads this number of
samples by a linewise bulk read from the sensor DATA registers.
Reading all FIFO entries from the DATA register clears FIFO_STATUS,
and this clears INT_SOURCE.

Now, in case of error or overrun, I'd use this reset function as a
fallback error handling. I stop measurement i.e. I set the sensor to
standby. The sensor should not accept further measurements. Then I
read out the fifo entries to clear FIFO_STATUS and I (already) read
INT_SOURCE to clear interrupt status. Eventually I turn on measurement
to bring the sensor back to operational. I ignore the read
measurements, I'm reading here.

As alternative approaches I also saw for similar sensors (not Linux)
to e.g. switch FIFO_STREAM mode to FIFO_BYPASS and back. This works
here too, but only for the FIFO_STATUS not for INT_SOURCE. Another
idea I can imagine with the ADXL313, there is a soft reset register,
but never tried that.

In this initial patch, the reset function will resume the interrupt
handler function. With the follow up patches, this will form rather
the error handling. It is easy to get into this kind of overrun
situation, if the interrupt handler is still not working correctly.
I'm actually pretty confident, that it now works only as a fallback
error handling, but perhaps I'm doing something wrong here?

> > +
> > +     adxl313_set_measure_en(data, true);
> > +}
> > +
> > +static int adxl313_buffer_postenable(struct iio_dev *indio_dev)
> > +{
> > +     struct adxl313_data *data = iio_priv(indio_dev);
> > +
> > +     data->fifo_mode = ADXL313_FIFO_STREAM;
>
> If you always set fifo_mode before calling _set_fifo() probably better
> to pass the value in as a separate parameter and store it as necessary
> inside that function.
>

It might be that this is even in the cache. I'll verify this.

> > +     return adxl313_set_fifo(data);
> > +}
> > +
> > +static int adxl313_buffer_predisable(struct iio_dev *indio_dev)
> > +{
> > +     struct adxl313_data *data = iio_priv(indio_dev);
> > +     int ret;
> > +
> > +     data->fifo_mode = ADXL313_FIFO_BYPASS;
> > +     ret = adxl313_set_fifo(data);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return regmap_write(data->regmap, ADXL313_REG_INT_ENABLE, 0);
> > +}
> > +
> > +static const struct iio_buffer_setup_ops adxl313_buffer_ops = {
> > +     .postenable = adxl313_buffer_postenable,
> > +     .predisable = adxl313_buffer_predisable,
> > +};
> > +
> > +static irqreturn_t adxl313_irq_handler(int irq, void *p)
> > +{
> > +     struct iio_dev *indio_dev = p;
> > +     struct adxl313_data *data = iio_priv(indio_dev);
> > +     int int_stat;
> > +
> > +     if (regmap_read(data->regmap, ADXL313_REG_INT_SOURCE, &int_stat))
>
> Failure to read is one thing we should handle, but also we should handle
> int_stat telling us there were no interrupts set for this device.
>
> > +             return IRQ_NONE;
> > +
> > +     adxl313_fifo_reset(data);
>
> Given we don't know it had anything to do with the fifo at this point
> resetting the fifo doesn't make much sense.  I'd expect a check
> on int_status, probably for overrun, before doing this.
>
> Ah. On closer inspection this isn't resetting the fifo, it's just
> reading it.  Rename that function and make it dependent on what
> was in int_stat.
>

As mentioned before, I used the term "reset" to clear the status
registers. This can occur for typically overrun, but also would cover
all events which are still not handled by the interrupt handler. I
could give it a try to see if the soft reset here would be a better
fit.

Best,
L

>
> > +
> > +     return IRQ_HANDLED;
> > +}
>

Re: [PATCH v3 07/12] iio: accel: adxl313: add basic interrupt handling

[Jonathan Cameron]

On Wed, 28 May 2025 22:52:55 +0200
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Hi Jonathan,
> 
> I feel here either I have some missunderstanding or it needs more
> (better?) explanation. Perhaps I'm using the wrong terminology.
> 
> One point, I forgot, do I actually need to add a Reviewed-by tag or
> something like that for Andys review? Or if so, they will let me know,
> I guess?

Only add a tag if Andy gives it.

> 
> First of all, introducing the adxl313_fifo_reset(data) here is the
> crucial part. So, the title I chose is not matching with the topic, or
> is too general. I'll answer and try to better explain down below.

I'd misunderstood somewhat what we had here which probably didn't help.
Partly this was down to patch break up and you putting something called
reset in temporarily covering the normal read path (threshold interrupt).

> >  
> > >  static const struct regmap_range adxl312_readable_reg_range[] = {
> > >       regmap_reg_range(ADXL313_REG_DEVID0, ADXL313_REG_DEVID0),
> > >       regmap_reg_range(ADXL313_REG_OFS_AXIS(0), ADXL313_REG_OFS_AXIS(2)),
> > > @@ -62,6 +71,7 @@ bool adxl313_is_volatile_reg(struct device *dev, unsigned int reg)
> > >       case ADXL313_REG_DATA_AXIS(4):
> > >       case ADXL313_REG_DATA_AXIS(5):
> > >       case ADXL313_REG_FIFO_STATUS:
> > > +     case ADXL313_REG_INT_SOURCE:
> > >               return true;
> > >       default:
> > >               return false;
> > > @@ -363,6 +373,118 @@ static int adxl313_write_raw(struct iio_dev *indio_dev,
> > >       }
> > >  }
> > >
> > > +static int adxl313_get_samples(struct adxl313_data *data)  
> >
> > I doubt this gets called from multiple places. I'd just put
> > the code inline and no have this helper at all.
> >  
> 
> It will be a called at least in two places. First, when reading the
> measurements and second when clearing the fifo in the reset.

Ok. Then this is fine to keep.

> > > +static int adxl313_fifo_transfer(struct adxl313_data *data, int samples)
> > > +{
> > > +     size_t count;
> > > +     unsigned int i;
> > > +     int ret;
> > > +
> > > +     count = array_size(sizeof(data->fifo_buf[0]), ADXL313_NUM_AXIS);
> > > +     for (i = 0; i < samples; i++) {
> > > +             ret = regmap_bulk_read(data->regmap, ADXL313_REG_XYZ_BASE,
> > > +                                    data->fifo_buf + (i * count / 2), count);  
> >
> > that 2 is I'd guessed based on size of some data store element?
> > I'd guess sizeof(data->fifo_buf[0]) is appropriate.
> >  
> 
> My calculation was the following:
> * samples := number of "lines" in the FIFO e.g. by watermark
> * count := number of bytes per "line"
> * ADXL313_NUM_AXIS := 3 for the three axis here
> There's a bulk read per "line" of the FIFO. A "line" comprises
> measurement for x, y and z axis. Each measurement consists of 2 bytes,
> i.e. count has 6 bytes.
> 
> At a second look now, probably count/2 can be replaced directly by
> ADXL313_NUM_AXIS. If so, I don't need the count variable. I see,
> count/2 being already a constant expression here smells somehow. I
> guess, this might be your point? I'll change that and need verify.

I was only commenting on the 2.  But sure, using ADXL313_NUM_AXIS
resolves that and is better still.
Not sure I'd bother with array_size() here, probably simply
using sizeof(data->fifo_buf[0]) * ADXL313_NUM_AXIS for that
final parameter is fine given we know it can't over flow and it's
the size of a subset of a larger array rather than an array
(kind of anyway!)


> >  
> > > +             if (ret)
> > > +                     return ret;
> > > +     }
> > > +     return 0;
> > > +}
> > > +
> > > +/**
> > > + * adxl313_fifo_reset() - Reset the FIFO and interrupt status registers.
> > > + * @data: The device data.
> > > + *
> > > + * Reset the FIFO status registers. Reading out status registers clears the  
> >
> > I think you already read it before calling this. So how is it ever set?
> >  
> > > + * FIFO and interrupt configuration. Thus do not evaluate regmap return values.
> > > + * Ignore particular read register content. Register content is not processed
> > > + * any further. Therefore the function returns void.
> > > + */
> > > +static void adxl313_fifo_reset(struct adxl313_data *data)  
> >
> > As below.  This isn't a reset.  Fifo reset is normally the term used
> > for when we have lost tracking of what is in the fifo and drop all data,
> > not normal readback.
> >  
> > > +{
> > > +     unsigned int regval;
> > > +     int samples;
> > > +
> > > +     adxl313_set_measure_en(data, false);  
> > Disabling measurement to read a fifo is unusual -  is this really necessary
> > as it presumably puts a gap in the data, which is what we are trying
> > to avoid by using a fifo.
> >  
> > > +
> > > +     samples = adxl313_get_samples(data);
> > > +     if (samples > 0)
> > > +             adxl313_fifo_transfer(data, samples);
> > > +
> > > +     regmap_read(data->regmap, ADXL313_REG_INT_SOURCE, &regval);  
> >
> > Not processing the convents of INT_SOURCE every time you read it
> > introduces race conditions.  This logic needs a rethink so that
> > never happens.  I guess this is why you are disabling measurement
> > to stop the status changing?  Just whatever each read of INT_SOURCE
> > tells us we need to handle and all should be fine without disabling
> > measurement.  That read should only clear bits that are set, so no
> > race conditions.
> >  
> 
> When the ADXL345 triggers an interrupt for e.g. watermark, data ready,
> or overrun,... it will stop from triggerring further interrupts until
> the status registers, INT_SOURCE and FIFO_STATUS are cleared. This I
> call "reset". In consequence the FIFO will simply run full.

Hmm.  I'd not use the reset term so broadly.  Reset for a fifo typically means
dropping all data on the floor after an overflow or other error condition.

> 
> Usually when the interrupt handler reads the interrupt status
> (INT_SOURCE). In case of, say, watermark, it then reads the
> FIFO_STATUS to obtain number of entries and reads this number of
> samples by a linewise bulk read from the sensor DATA registers.
> Reading all FIFO entries from the DATA register clears FIFO_STATUS,
> and this clears INT_SOURCE.
> 
> Now, in case of error or overrun, I'd use this reset function as a
> fallback error handling. I stop measurement i.e. I set the sensor to
> standby. The sensor should not accept further measurements. Then I
> read out the fifo entries to clear FIFO_STATUS and I (already) read
> INT_SOURCE to clear interrupt status. Eventually I turn on measurement
> to bring the sensor back to operational. I ignore the read
> measurements, I'm reading here.
> 
> As alternative approaches I also saw for similar sensors (not Linux)
> to e.g. switch FIFO_STREAM mode to FIFO_BYPASS and back. This works
> here too, but only for the FIFO_STATUS not for INT_SOURCE. Another
> idea I can imagine with the ADXL313, there is a soft reset register,
> but never tried that.
> 
> In this initial patch, the reset function will resume the interrupt
> handler function. With the follow up patches, this will form rather
> the error handling. It is easy to get into this kind of overrun
> situation, if the interrupt handler is still not working correctly.
> I'm actually pretty confident, that it now works only as a fallback
> error handling, but perhaps I'm doing something wrong here?

This is where I got confused - because it wasn't just the error
paths. All made sense after later patches.  Stopping measurements when
you enter an error condition is fine.
> 

> > > +             return IRQ_NONE;
> > > +
> > > +     adxl313_fifo_reset(data);  
> >
> > Given we don't know it had anything to do with the fifo at this point
> > resetting the fifo doesn't make much sense.  I'd expect a check
> > on int_status, probably for overrun, before doing this.
> >
> > Ah. On closer inspection this isn't resetting the fifo, it's just
> > reading it.  Rename that function and make it dependent on what
> > was in int_stat.
> >  
> 
> As mentioned before, I used the term "reset" to clear the status
> registers. This can occur for typically overrun, but also would cover
> all events which are still not handled by the interrupt handler. I
> could give it a try to see if the soft reset here would be a better
> fit.

No need to change the code, just don't use the term reset if you
are not clearing all data (without using it) and any outstanding
interrupt status.  Pick a different term clear_int for example.

Soft reset is massive overkill for an overrun event.

Jonathan

> 
> Best,
> L
> 
> >  
> > > +
> > > +     return IRQ_HANDLED;
> > > +}  
> >  

[PATCH v3 08/12] iio: accel: adxl313: add FIFO watermark

[Lothar Rubusch]

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

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

diff --git a/drivers/iio/accel/adxl313.h b/drivers/iio/accel/adxl313.h
index 9cdad55dd350..8c68cff7569c 100644
--- a/drivers/iio/accel/adxl313.h
+++ b/drivers/iio/accel/adxl313.h
@@ -87,6 +87,7 @@ struct adxl313_data {
 	const struct adxl313_chip_info *chip_info;
 	struct mutex	lock; /* lock to protect transf_buf */
 	int irq;
+	u8 watermark;
 	u8 fifo_mode;
 	__le16		transf_buf __aligned(IIO_DMA_MINALIGN);
 	__le16		fifo_buf[ADXL313_NUM_AXIS * ADXL313_FIFO_SIZE + 1];
diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
index 1e085f0c61a0..80991cd9bd79 100644
--- a/drivers/iio/accel/adxl313_core.c
+++ b/drivers/iio/accel/adxl313_core.c
@@ -373,6 +373,25 @@ static int adxl313_write_raw(struct iio_dev *indio_dev,
 	}
 }
 
+static int adxl313_set_watermark(struct iio_dev *indio_dev, unsigned int value)
+{
+	struct adxl313_data *data = iio_priv(indio_dev);
+	const unsigned int fifo_mask = 0x1f, interrupt_mask = 0x02;
+	int ret;
+
+	value = min(value, ADXL313_FIFO_SIZE - 1);
+
+	ret = regmap_update_bits(data->regmap, ADXL313_REG_FIFO_CTL,
+				 fifo_mask, value);
+	if (ret)
+		return ret;
+
+	data->watermark = value;
+
+	return regmap_update_bits(data->regmap, ADXL313_REG_INT_ENABLE,
+				  interrupt_mask, ADXL313_INT_WATERMARK);
+}
+
 static int adxl313_get_samples(struct adxl313_data *data)
 {
 	unsigned int regval = 0;
@@ -399,6 +418,7 @@ static int adxl313_set_fifo(struct adxl313_data *data)
 		return ret;
 
 	ret = regmap_write(data->regmap, ADXL313_REG_FIFO_CTL,
+			   FIELD_PREP(ADXL313_REG_FIFO_CTL_SAMPLES_MSK,	data->watermark) |
 			   FIELD_PREP(ADXL313_REG_FIFO_CTL_MODE_MSK, data->fifo_mode));
 
 	return adxl313_set_measure_en(data, true);
@@ -471,6 +491,39 @@ static const struct iio_buffer_setup_ops adxl313_buffer_ops = {
 	.predisable = adxl313_buffer_predisable,
 };
 
+static int adxl313_fifo_push(struct iio_dev *indio_dev, int samples)
+{
+	struct adxl313_data *data = iio_priv(indio_dev);
+	unsigned int i;
+	int ret;
+
+	ret = adxl313_fifo_transfer(data, samples);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < ADXL313_NUM_AXIS * samples; i += ADXL313_NUM_AXIS)
+		iio_push_to_buffers(indio_dev, &data->fifo_buf[i]);
+
+	return 0;
+}
+
+static int adxl313_push_event(struct iio_dev *indio_dev, int int_stat)
+{
+	struct adxl313_data *data = iio_priv(indio_dev);
+	int samples;
+
+	if (FIELD_GET(ADXL313_INT_WATERMARK, int_stat)) {
+		samples = adxl313_get_samples(data);
+		if (samples < 0)
+			return samples;
+
+		return adxl313_fifo_push(indio_dev, samples);
+	}
+
+	/* Return error if no event data was pushed to the IIO channel. */
+	return -ENOENT;
+}
+
 static irqreturn_t adxl313_irq_handler(int irq, void *p)
 {
 	struct iio_dev *indio_dev = p;
@@ -480,6 +533,15 @@ static irqreturn_t adxl313_irq_handler(int irq, void *p)
 	if (regmap_read(data->regmap, ADXL313_REG_INT_SOURCE, &int_stat))
 		return IRQ_NONE;
 
+	if (adxl313_push_event(indio_dev, int_stat))
+		goto err;
+
+	if (FIELD_GET(ADXL313_INT_OVERRUN, int_stat))
+		goto err;
+
+	return IRQ_HANDLED;
+
+err:
 	adxl313_fifo_reset(data);
 
 	return IRQ_HANDLED;
@@ -499,6 +561,7 @@ static const struct iio_info adxl313_info = {
 	.read_raw	= adxl313_read_raw,
 	.write_raw	= adxl313_write_raw,
 	.read_avail	= adxl313_read_freq_avail,
+	.hwfifo_set_watermark = adxl313_set_watermark,
 	.debugfs_reg_access = &adxl313_reg_access,
 };
 
-- 
2.39.5

Re: [PATCH v3 08/12] iio: accel: adxl313: add FIFO watermark

[Jonathan Cameron]

On Fri, 23 May 2025 22:35:19 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add FIFO watermark configuration and evaluation. Let a watermark to be
> configured. Evaluate the interrupt accordingly. Read out the FIFO content
> and push the values to the IIO channel.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
Hi Lothar,

I'd rethink the patch split a little and combine this with the initial interrupt
support.  That will avoid confusion over when the reset fires.

> diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
> index 1e085f0c61a0..80991cd9bd79 100644
> --- a/drivers/iio/accel/adxl313_core.c
> +++ b/drivers/iio/accel/adxl313_core.c
> @@ -373,6 +373,25 @@ static int adxl313_write_raw(struct iio_dev *indio_dev,

> +
> +static int adxl313_push_event(struct iio_dev *indio_dev, int int_stat)
I'd avoid the term 'event' for this as your only use so far is something
we'd not consider an 'event' in IIO terms.

I'd be tempted to just keep this as code directly in the _irq_handler()
but maybe it will become less manageable in later patches.

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

Ah. This is all making a little more sense.  I think partly the issue
here is this got split up into a few too many patches.  FIFO plus
initial interrupt support in one would have been better perhaps.

Jonathan

[PATCH v3 09/12] iio: accel: adxl313: add activity sensing

[Lothar Rubusch]

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

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

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

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

Re: [PATCH v3 09/12] iio: accel: adxl313: add activity sensing

[Jonathan Cameron]

On Fri, 23 May 2025 22:35:20 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

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

I see this hardware is similar to our friend the axl345 so some of the outcomes
of final reviews on that series may apply here as well.

> ---
>  drivers/iio/accel/adxl313_core.c | 229 ++++++++++++++++++++++++++++++-
>  1 file changed, 227 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
> index 80991cd9bd79..74bb7cfe8a55 100644
> --- a/drivers/iio/accel/adxl313_core.c
> +++ b/drivers/iio/accel/adxl313_core.c

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

If all 3 axis perhaps?  Or If any axis?  I'm not sure what intent is here.


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

Re: [PATCH v3 09/12] iio: accel: adxl313: add activity sensing

[Lothar Rubusch]

Hi Jonathan,

On Sun, May 25, 2025 at 3:04 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri, 23 May 2025 22:35:20 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Add possibilities to set a threshold for activity sensing. Extend the
> > interrupt handler to process activity interrupts. Provide functions to set
> > the activity threshold and to enable/disable activity sensing. Further add
> > a fake channel for having x, y and z axis anded on the iio channel.
> >
> > This is a preparatory patch. Some of the definitions and functions are
> > supposed to be extended for inactivity later on.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> One comment I found confusing.
>
> I see this hardware is similar to our friend the axl345 so some of the outcomes
> of final reviews on that series may apply here as well.

Yes. To be honest with you, I already saw several places, where I
probably need to send you some refac for the ADXL345 as well.
Implementing the same type of source a second time, sometimes leads
[me] to different[/better?] solutions and brings different insights.

>
> > ---
> >  drivers/iio/accel/adxl313_core.c | 229 ++++++++++++++++++++++++++++++-
> >  1 file changed, 227 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
> > index 80991cd9bd79..74bb7cfe8a55 100644
> > --- a/drivers/iio/accel/adxl313_core.c
> > +++ b/drivers/iio/accel/adxl313_core.c
>
> >  static const unsigned long adxl313_scan_masks[] = {
> > @@ -300,6 +334,60 @@ static int adxl313_read_freq_avail(struct iio_dev *indio_dev,
> >       }
> >  }
> >
> > +static int adxl313_is_act_inact_en(struct adxl313_data *data,
> > +                                enum adxl313_activity_type type)
> > +{
> > +     unsigned int axis_ctrl;
> > +     unsigned int regval;
> > +     int axis_en, int_en, ret;
> > +
> > +     ret = regmap_read(data->regmap, ADXL313_REG_ACT_INACT_CTL, &axis_ctrl);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Check if axis for activity are enabled */
>
> If all 3 axis perhaps?  Or If any axis?  I'm not sure what intent is here.

For the ADXL313 I do generally all axis, i.e. x-, y-, z-axis - enabled
and disabled, respectively. I'll modify the comment.

Sry about spamming the ML with my emails about the reset function. I
oversaw your other mail. Patches will be merged.

Best,
L

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

Re: [PATCH v3 09/12] iio: accel: adxl313: add activity sensing

[Jonathan Cameron]

On Thu, 29 May 2025 18:22:50 +0200
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Hi Jonathan,
> 
> On Sun, May 25, 2025 at 3:04 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Fri, 23 May 2025 22:35:20 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >  
> > > Add possibilities to set a threshold for activity sensing. Extend the
> > > interrupt handler to process activity interrupts. Provide functions to set
> > > the activity threshold and to enable/disable activity sensing. Further add
> > > a fake channel for having x, y and z axis anded on the iio channel.
> > >
> > > This is a preparatory patch. Some of the definitions and functions are
> > > supposed to be extended for inactivity later on.
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>  
> > One comment I found confusing.
> >
> > I see this hardware is similar to our friend the axl345 so some of the outcomes
> > of final reviews on that series may apply here as well.  
> 
> Yes. To be honest with you, I already saw several places, where I
> probably need to send you some refac for the ADXL345 as well.
> Implementing the same type of source a second time, sometimes leads
> [me] to different[/better?] solutions and brings different insights.
> 
> >  
> > > ---
> > >  drivers/iio/accel/adxl313_core.c | 229 ++++++++++++++++++++++++++++++-
> > >  1 file changed, 227 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
> > > index 80991cd9bd79..74bb7cfe8a55 100644
> > > --- a/drivers/iio/accel/adxl313_core.c
> > > +++ b/drivers/iio/accel/adxl313_core.c  
> >  
> > >  static const unsigned long adxl313_scan_masks[] = {
> > > @@ -300,6 +334,60 @@ static int adxl313_read_freq_avail(struct iio_dev *indio_dev,
> > >       }
> > >  }
> > >
> > > +static int adxl313_is_act_inact_en(struct adxl313_data *data,
> > > +                                enum adxl313_activity_type type)
> > > +{
> > > +     unsigned int axis_ctrl;
> > > +     unsigned int regval;
> > > +     int axis_en, int_en, ret;
> > > +
> > > +     ret = regmap_read(data->regmap, ADXL313_REG_ACT_INACT_CTL, &axis_ctrl);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     /* Check if axis for activity are enabled */  
> >
> > If all 3 axis perhaps?  Or If any axis?  I'm not sure what intent is here.  
> 
> For the ADXL313 I do generally all axis, i.e. x-, y-, z-axis - enabled
> and disabled, respectively. I'll modify the comment.
> 
> Sry about spamming the ML with my emails about the reset function. I
> oversaw your other mail. Patches will be merged.

Lol. I did the same thing just now. Don't worry about it!

J
> 
> Best,
> L
> 
> >  
> > > +     if (type != ADXL313_ACTIVITY)
> > > +             return 0;
> > > +
> > > +     axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
> > > +
> > > +     /* The axis are enabled, now check if specific interrupt is enabled */
> > > +     ret = regmap_read(data->regmap, ADXL313_REG_INT_ENABLE, &regval);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     int_en = adxl313_act_int_reg[type] & regval;
> > > +
> > > +     return axis_en && int_en;
> > > +}  
> >  

Re: [PATCH v3 09/12] iio: accel: adxl313: add activity sensing

[Jonathan Cameron]

m adxl313_chans {
>  	chan_x, chan_y, chan_z,
>  };
> @@ -238,6 +264,14 @@ static const struct iio_chan_spec adxl313_channels[] = {
>  	ADXL313_ACCEL_CHANNEL(0, chan_x, X),
>  	ADXL313_ACCEL_CHANNEL(1, chan_y, Y),
>  	ADXL313_ACCEL_CHANNEL(2, chan_z, Z),
> +	{
> +		.type = IIO_ACCEL,
> +		.modified = 1,
> +		.channel2 = IIO_MOD_X_AND_Y_AND_Z,
I'm confused. If this is for activity it would be very unusual for it
to be X&Y&Z. 

"A setting of 1 enables x-, y-, or z-axis participation in detecting
activity or inactivity. A setting of 0 excludes the selected axis from
participation. If all axes are excluded, the function is disabled. For
activity detection, all participating axes are logically OR’ed, causing
the activity function to trigger when any of the participating axes
exceeds the threshold. For inactivity detection, all participating axes
are logically AND’ed, causing the inactivity function to trigger only
if all participating axes are below the threshold for the specified
period of time."

Which matches with what I'd expect.

> +		.scan_index = -1, /* Fake channel for axis AND'ing */
> +		.event_spec = adxl313_fake_chan_events,
> +		.num_event_specs = ARRAY_SIZE(adxl313_fake_chan_events),
> +	},
>  };
>  
>  static const unsigned long adxl313_scan_masks[] = {
> @@ -300,6 +334,60 @@ static int adxl313_read_freq_avail(struct iio_dev *indio_dev,
>  	}
>  }

[PATCH v3 10/12] iio: accel: adxl313: add inactivity sensing

[Lothar Rubusch]

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

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

diff --git a/drivers/iio/accel/adxl313.h b/drivers/iio/accel/adxl313.h
index 8c68cff7569c..374e4bfe6e05 100644
--- a/drivers/iio/accel/adxl313.h
+++ b/drivers/iio/accel/adxl313.h
@@ -18,6 +18,8 @@
 #define ADXL313_REG_SOFT_RESET		0x18
 #define ADXL313_REG_OFS_AXIS(index)	(0x1E + (index))
 #define ADXL313_REG_THRESH_ACT		0x24
+#define ADXL313_REG_THRESH_INACT	0x25
+#define ADXL313_REG_TIME_INACT		0x26
 #define ADXL313_REG_ACT_INACT_CTL	0x27
 #define ADXL313_REG_BW_RATE		0x2C
 #define ADXL313_REG_POWER_CTL		0x2D
diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
index 74bb7cfe8a55..f7baca814da7 100644
--- a/drivers/iio/accel/adxl313_core.c
+++ b/drivers/iio/accel/adxl313_core.c
@@ -30,18 +30,22 @@
 #define ADXL313_REG_XYZ_BASE			ADXL313_REG_DATA_AXIS(0)
 
 #define ADXL313_ACT_XYZ_EN			GENMASK(6, 4)
+#define ADXL313_INACT_XYZ_EN			GENMASK(2, 0)
 
 /* activity/inactivity */
 enum adxl313_activity_type {
 	ADXL313_ACTIVITY,
+	ADXL313_INACTIVITY,
 };
 
 static const unsigned int adxl313_act_int_reg[] = {
 	[ADXL313_ACTIVITY] = ADXL313_INT_ACTIVITY,
+	[ADXL313_INACTIVITY] = ADXL313_INT_INACTIVITY,
 };
 
 static const unsigned int adxl313_act_thresh_reg[] = {
 	[ADXL313_ACTIVITY] = ADXL313_REG_THRESH_ACT,
+	[ADXL313_INACTIVITY] = ADXL313_REG_THRESH_INACT,
 };
 
 static const struct regmap_range adxl312_readable_reg_range[] = {
@@ -254,6 +258,14 @@ static const struct iio_event_spec adxl313_fake_chan_events[] = {
 		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
 		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE),
 	},
+	{
+		/* inactivity */
+		.type = IIO_EV_TYPE_MAG,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
+		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
+			BIT(IIO_EV_INFO_PERIOD),
+	},
 };
 
 enum adxl313_chans {
@@ -334,6 +346,15 @@ static int adxl313_read_freq_avail(struct iio_dev *indio_dev,
 	}
 }
 
+static int adxl313_set_inact_time_s(struct adxl313_data *data,
+				    unsigned int val_s)
+{
+	unsigned int max_boundary = 255;
+	unsigned int val = min(val_s, max_boundary);
+
+	return regmap_write(data->regmap, ADXL313_REG_TIME_INACT, val);
+}
+
 static int adxl313_is_act_inact_en(struct adxl313_data *data,
 				   enum adxl313_activity_type type)
 {
@@ -346,10 +367,10 @@ static int adxl313_is_act_inact_en(struct adxl313_data *data,
 		return ret;
 
 	/* Check if axis for activity are enabled */
-	if (type != ADXL313_ACTIVITY)
-		return 0;
-
-	axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
+	if (type == ADXL313_ACTIVITY)
+		axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
+	else
+		axis_en = FIELD_GET(ADXL313_INACT_XYZ_EN, axis_ctrl);
 
 	/* The axis are enabled, now check if specific interrupt is enabled */
 	ret = regmap_read(data->regmap, ADXL313_REG_INT_ENABLE, &regval);
@@ -367,12 +388,14 @@ static int adxl313_set_act_inact_en(struct adxl313_data *data,
 {
 	unsigned int axis_ctrl;
 	unsigned int threshold;
+	unsigned int inact_time_s;
+	bool en;
 	int ret;
 
-	if (type != ADXL313_ACTIVITY)
-		return 0;
-
-	axis_ctrl = ADXL313_ACT_XYZ_EN;
+	if (type == ADXL313_ACTIVITY)
+		axis_ctrl = ADXL313_ACT_XYZ_EN;
+	else
+		axis_ctrl = ADXL313_INACT_XYZ_EN;
 
 	ret = regmap_assign_bits(data->regmap, ADXL313_REG_ACT_INACT_CTL,
 				 axis_ctrl, cmd_en);
@@ -383,9 +406,17 @@ static int adxl313_set_act_inact_en(struct adxl313_data *data,
 	if (ret)
 		return ret;
 
+	en = cmd_en && threshold;
+	if (type == ADXL313_INACTIVITY) {
+		ret = regmap_read(data->regmap, ADXL313_REG_TIME_INACT, &inact_time_s);
+		if (ret)
+			return ret;
+
+		en = en && inact_time_s;
+	}
+
 	return regmap_assign_bits(data->regmap, ADXL313_REG_INT_ENABLE,
-				  adxl313_act_int_reg[type],
-				  cmd_en && threshold);
+				  adxl313_act_int_reg[type], en);
 }
 
 static int adxl313_read_raw(struct iio_dev *indio_dev,
@@ -474,6 +505,8 @@ static int adxl313_read_event_config(struct iio_dev *indio_dev,
 	switch (dir) {
 	case IIO_EV_DIR_RISING:
 		return adxl313_is_act_inact_en(data, ADXL313_ACTIVITY);
+	case IIO_EV_DIR_FALLING:
+		return adxl313_is_act_inact_en(data, ADXL313_INACTIVITY);
 	default:
 		return -EINVAL;
 	}
@@ -493,6 +526,8 @@ static int adxl313_write_event_config(struct iio_dev *indio_dev,
 	switch (dir) {
 	case IIO_EV_DIR_RISING:
 		return adxl313_set_act_inact_en(data, ADXL313_ACTIVITY, state);
+	case IIO_EV_DIR_FALLING:
+		return adxl313_set_act_inact_en(data, ADXL313_INACTIVITY, state);
 	default:
 		return -EINVAL;
 	}
@@ -507,6 +542,8 @@ static int adxl313_read_event_value(struct iio_dev *indio_dev,
 {
 	struct adxl313_data *data = iio_priv(indio_dev);
 	unsigned int act_threshold;
+	unsigned int inact_threshold;
+	unsigned int inact_time_s;
 	int ret;
 
 	/* Measurement stays enabled, reading from regmap cache */
@@ -514,19 +551,38 @@ static int adxl313_read_event_value(struct iio_dev *indio_dev,
 	if (type != IIO_EV_TYPE_MAG)
 		return -EINVAL;
 
-	if (info != IIO_EV_INFO_VALUE)
-		return -EINVAL;
-
-	switch (dir) {
-	case IIO_EV_DIR_RISING:
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			ret = regmap_read(data->regmap,
+					  adxl313_act_thresh_reg[ADXL313_ACTIVITY],
+					  &act_threshold);
+			if (ret)
+				return ret;
+			*val = act_threshold * 15625;
+			*val2 = MICRO;
+			return IIO_VAL_FRACTIONAL;
+		case IIO_EV_DIR_FALLING:
+			ret = regmap_read(data->regmap,
+					  adxl313_act_thresh_reg[ADXL313_INACTIVITY],
+					  &inact_threshold);
+			if (ret)
+				return ret;
+			*val = inact_threshold * 15625;
+			*val2 = MICRO;
+			return IIO_VAL_FRACTIONAL;
+		default:
+			return -EINVAL;
+		}
+	case IIO_EV_INFO_PERIOD:
 		ret = regmap_read(data->regmap,
-				  adxl313_act_thresh_reg[ADXL313_ACTIVITY],
-				  &act_threshold);
+				  ADXL313_REG_TIME_INACT,
+				  &inact_time_s);
 		if (ret)
 			return ret;
-		*val = act_threshold * 15625;
-		*val2 = MICRO;
-		return IIO_VAL_FRACTIONAL;
+		*val = inact_time_s;
+		return IIO_VAL_INT;
 	default:
 		return -EINVAL;
 	}
@@ -550,16 +606,30 @@ static int adxl313_write_event_value(struct iio_dev *indio_dev,
 	if (type != IIO_EV_TYPE_MAG)
 		return -EINVAL;
 
-	if (info != IIO_EV_INFO_VALUE)
-		return -EINVAL;
-
-	/* Scale factor 15.625 mg/LSB */
-	regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
-	switch (dir) {
-	case IIO_EV_DIR_RISING:
-		ret = regmap_write(data->regmap,
-				   adxl313_act_thresh_reg[ADXL313_ACTIVITY],
-				   regval);
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		/* The scale factor is 15.625 mg/LSB */
+		regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			ret = regmap_write(data->regmap,
+					   adxl313_act_thresh_reg[ADXL313_ACTIVITY],
+					   regval);
+			if (ret)
+				return ret;
+			return adxl313_set_measure_en(data, true);
+		case IIO_EV_DIR_FALLING:
+			ret = regmap_write(data->regmap,
+					   adxl313_act_thresh_reg[ADXL313_INACTIVITY],
+					   regval);
+			if (ret)
+				return ret;
+			return adxl313_set_measure_en(data, true);
+		default:
+			return -EINVAL;
+		}
+	case IIO_EV_INFO_PERIOD:
+		ret = adxl313_set_inact_time_s(data, val);
 		if (ret)
 			return ret;
 		return adxl313_set_measure_en(data, true);
@@ -720,6 +790,17 @@ static int adxl313_push_event(struct iio_dev *indio_dev, int int_stat)
 			return ret;
 	}
 
+	if (FIELD_GET(ADXL313_INT_INACTIVITY, int_stat)) {
+		ret = iio_push_event(indio_dev,
+				     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
+							IIO_MOD_X_AND_Y_AND_Z,
+							IIO_EV_TYPE_MAG,
+							IIO_EV_DIR_FALLING),
+				     ts);
+		if (ret)
+			return ret;
+	}
+
 	if (FIELD_GET(ADXL313_INT_WATERMARK, int_stat)) {
 		samples = adxl313_get_samples(data);
 		if (samples < 0)
@@ -890,6 +971,14 @@ int adxl313_core_probe(struct device *dev,
 		if (ret)
 			return ret;
 
+		ret = regmap_write(data->regmap, ADXL313_REG_TIME_INACT, 5);
+		if (ret)
+			return ret;
+
+		ret = regmap_write(data->regmap, ADXL313_REG_THRESH_INACT, 0x4f);
+		if (ret)
+			return ret;
+
 		ret = regmap_write(data->regmap, ADXL313_REG_THRESH_ACT, 0x52);
 		if (ret)
 			return ret;
-- 
2.39.5

Re: [PATCH v3 10/12] iio: accel: adxl313: add inactivity sensing

[Jonathan Cameron]

On Fri, 23 May 2025 22:35:21 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

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

Hi Lothar,  Comments inline.
>  static const struct regmap_range adxl312_readable_reg_range[] = {
> @@ -254,6 +258,14 @@ static const struct iio_event_spec adxl313_fake_chan_events[] = {
>  		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
>  		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE),
>  	},
> +	{
> +		/* inactivity */

Ah. I only noticed this here, but the axis for these two types of
detection should be different so awe shouldn't see them in a single
array of events.


> +		.type = IIO_EV_TYPE_MAG,
> +		.dir = IIO_EV_DIR_FALLING,
> +		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
> +		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
> +			BIT(IIO_EV_INFO_PERIOD),
> +	},
>  };

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

I got lost in earlier discussion you were having with Andy on this, but
personally, if a series is going to introduce a simple test then flip
it to a switch statement later, I'd rather see the switch statement from
the start and reduce the churn a little.

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

[PATCH v3 11/12] iio: accel: adxl313: implement power-save on inactivity

[Lothar Rubusch]

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

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

diff --git a/drivers/iio/accel/adxl313.h b/drivers/iio/accel/adxl313.h
index 374e4bfe6e05..fae2378fa4ed 100644
--- a/drivers/iio/accel/adxl313.h
+++ b/drivers/iio/accel/adxl313.h
@@ -41,6 +41,9 @@
 #define ADXL313_RATE_BASE		6
 
 #define ADXL313_POWER_CTL_MSK		BIT(3)
+#define ADXL313_POWER_CTL_INACT_MSK	GENMASK(5, 4)
+#define ADXL313_POWER_CTL_LINK		BIT(5)
+#define ADXL313_POWER_CTL_AUTO_SLEEP	BIT(4)
 
 #define ADXL313_RANGE_MSK		GENMASK(1, 0)
 #define ADXL313_RANGE_MAX		3
diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
index f7baca814da7..d65380901f66 100644
--- a/drivers/iio/accel/adxl313_core.c
+++ b/drivers/iio/accel/adxl313_core.c
@@ -389,6 +389,7 @@ static int adxl313_set_act_inact_en(struct adxl313_data *data,
 	unsigned int axis_ctrl;
 	unsigned int threshold;
 	unsigned int inact_time_s;
+	int act_en, inact_en;
 	bool en;
 	int ret;
 
@@ -415,8 +416,28 @@ static int adxl313_set_act_inact_en(struct adxl313_data *data,
 		en = en && inact_time_s;
 	}
 
-	return regmap_assign_bits(data->regmap, ADXL313_REG_INT_ENABLE,
-				  adxl313_act_int_reg[type], en);
+	ret = regmap_assign_bits(data->regmap, ADXL313_REG_INT_ENABLE,
+				 adxl313_act_int_reg[type], en);
+	if (ret)
+		return ret;
+
+	/*
+	 * Advanced power saving: Set sleep and link bit only when ACT and INACT
+	 * are set. Check enable against regmap cached values.
+	 */
+	act_en = adxl313_is_act_inact_en(data, ADXL313_ACTIVITY);
+	if (act_en < 0)
+		return act_en;
+
+	inact_en = adxl313_is_act_inact_en(data, ADXL313_INACTIVITY);
+	if (inact_en < 0)
+		return inact_en;
+
+	en = en && act_en && inact_en;
+
+	return regmap_assign_bits(data->regmap, ADXL313_REG_POWER_CTL,
+				  (ADXL313_POWER_CTL_AUTO_SLEEP | ADXL313_POWER_CTL_LINK),
+				  en);
 }
 
 static int adxl313_read_raw(struct iio_dev *indio_dev,
-- 
2.39.5

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

[Jonathan Cameron]

On Fri, 23 May 2025 22:35:22 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Link activity and inactivity to indicate the internal power-saving state.
> Add auto-sleep to be linked to inactivity.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl313.h      |  3 +++
>  drivers/iio/accel/adxl313_core.c | 25 +++++++++++++++++++++++--
>  2 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl313.h b/drivers/iio/accel/adxl313.h
> index 374e4bfe6e05..fae2378fa4ed 100644
> --- a/drivers/iio/accel/adxl313.h
> +++ b/drivers/iio/accel/adxl313.h
> @@ -41,6 +41,9 @@
>  #define ADXL313_RATE_BASE		6
>  
>  #define ADXL313_POWER_CTL_MSK		BIT(3)
> +#define ADXL313_POWER_CTL_INACT_MSK	GENMASK(5, 4)
> +#define ADXL313_POWER_CTL_LINK		BIT(5)
> +#define ADXL313_POWER_CTL_AUTO_SLEEP	BIT(4)
>  
>  #define ADXL313_RANGE_MSK		GENMASK(1, 0)
>  #define ADXL313_RANGE_MAX		3
> diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
> index f7baca814da7..d65380901f66 100644
> --- a/drivers/iio/accel/adxl313_core.c
> +++ b/drivers/iio/accel/adxl313_core.c
> @@ -389,6 +389,7 @@ static int adxl313_set_act_inact_en(struct adxl313_data *data,
>  	unsigned int axis_ctrl;
>  	unsigned int threshold;
>  	unsigned int inact_time_s;
> +	int act_en, inact_en;
>  	bool en;
>  	int ret;
>  
> @@ -415,8 +416,28 @@ static int adxl313_set_act_inact_en(struct adxl313_data *data,
>  		en = en && inact_time_s;
>  	}
>  
> -	return regmap_assign_bits(data->regmap, ADXL313_REG_INT_ENABLE,
> -				  adxl313_act_int_reg[type], en);
> +	ret = regmap_assign_bits(data->regmap, ADXL313_REG_INT_ENABLE,
> +				 adxl313_act_int_reg[type], en);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Advanced power saving: Set sleep and link bit only when ACT and INACT
> +	 * are set. Check enable against regmap cached values.

No need to mention regmap cache.  That's just an optimization to avoid us
going and asking the device for register contents.  It should not be
a fundamental part of how the driver works - rather it should just allow
us to not bother with our own optimizations when it already does the job.

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

For the clearing of link there advice in the spec to exit measurement mode and reenter it
that I'm not spotting here.  See "Link bit" section.


>  }
>  
>  static int adxl313_read_raw(struct iio_dev *indio_dev,

[PATCH v3 12/12] docs: iio: add ADXL313 accelerometer

[Lothar Rubusch]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 9652 bytes --]

Add documentation for the ADXL313 accelerometer driver.

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

diff --git a/Documentation/iio/adxl313.rst b/Documentation/iio/adxl313.rst
new file mode 100644
index 000000000000..8c4e2d141594
--- /dev/null
+++ b/Documentation/iio/adxl313.rst
@@ -0,0 +1,196 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+===============
+ADXL313 driver
+===============
+
+This driver supports Analog Device's ADXL313 on SPI/I2C bus.
+
+1. Supported devices
+====================
+
+* `ADXL313 <https://www.analog.com/ADXL313>`_
+
+The ADXL313is a low noise density, low power, 3-axis accelerometer with
+selectable measurement ranges. The ADXL313 supports the ±0.5 g, ±1 g, ±2 g and
+±4 g ranges.
+
+2. Device attributes
+====================
+
+Accelerometer measurements are always provided.
+
+Temperature data are also provided. This data can be used to monitor the
+internal system temperature or to improve the temperature stability of the
+device via calibration.
+
+Each IIO device, has a device folder under ``/sys/bus/iio/devices/iio:deviceX``,
+where X is the IIO index of the device. Under these folders reside a set of
+device files, depending on the characteristics and features of the hardware
+device in questions. These files are consistently generalized and documented in
+the IIO ABI documentation.
+
+The following tables show the adxl313 related device files, found in the
+specific device folder path ``/sys/bus/iio/devices/iio:deviceX``.
+
++---------------------------------------------------+----------------------------------------------------------+
+| 3-Axis Accelerometer related device files         | Description                                              |
++---------------------------------------------------+----------------------------------------------------------+
+| in_accel_scale                                    | Scale for the accelerometer channels.                    |
++---------------------------------------------------+----------------------------------------------------------+
+| in_accel_x_calibbias                              | Calibration offset for the X-axis accelerometer channel. |
++---------------------------------------------------+----------------------------------------------------------+
+| in_accel_x_raw                                    | Raw X-axis accelerometer channel value.                  |
++---------------------------------------------------+----------------------------------------------------------+
+| in_accel_y_calibbias                              | y-axis acceleration offset correction                    |
++---------------------------------------------------+----------------------------------------------------------+
+| in_accel_y_raw                                    | Raw Y-axis accelerometer channel value.                  |
++---------------------------------------------------+----------------------------------------------------------+
+| in_accel_z_calibbias                              | Calibration offset for the Z-axis accelerometer channel. |
++---------------------------------------------------+----------------------------------------------------------+
+| in_accel_z_raw                                    | Raw Z-axis accelerometer channel value.                  |
++---------------------------------------------------+----------------------------------------------------------+
+
++---------------------------------------+----------------------------------------------+
+| Miscellaneous device files            | Description                                  |
++---------------------------------------+----------------------------------------------+
+| name                                  | Name of the IIO device.                      |
++---------------------------------------+----------------------------------------------+
+| in_accel_sampling_frequency           | Currently selected sample rate.              |
++---------------------------------------+----------------------------------------------+
+| in_accel_sampling_frequency_available | Available sampling frequency configurations. |
++---------------------------------------+----------------------------------------------+
+
+Channels processed values
+-------------------------
+
+A channel value can be read from its _raw attribute. The value returned is the
+raw value as reported by the devices. To get the processed value of the channel,
+apply the following formula:
+
+.. code-block:: bash
+
+        processed value = (_raw + _offset) * _scale
+
+Where _offset and _scale are device attributes. If no _offset attribute is
+present, simply assume its value is 0.
+
+The ADXL313 driver offers data for a single types of channels, the table below
+shows the measurement units for the processed value, which are defined by the
+IIO framework:
+
++-------------------------------------+---------------------------+
+| Channel type                        | Measurement unit          |
++-------------------------------------+---------------------------+
+| Acceleration on X, Y, and Z axis    | Meters per Second squared |
++-------------------------------------+---------------------------+
+
+Usage examples
+--------------
+
+Show device name:
+
+.. code-block:: bash
+
+        root:/sys/bus/iio/devices/iio:device0> cat name
+        adxl313
+
+Show accelerometer channels value:
+
+.. code-block:: bash
+
+        root:/sys/bus/iio/devices/iio:device0> cat in_accel_x_raw
+        2
+        root:/sys/bus/iio/devices/iio:device0> cat in_accel_y_raw
+        -57
+        root:/sys/bus/iio/devices/iio:device0> cat in_accel_z_raw
+        2
+        root:/sys/bus/iio/devices/iio:device0> cat in_accel_scale
+        0.009576806
+
+- X-axis acceleration = in_accel_x_raw * in_accel_scale = 0.0191536 m/s^2
+- Y-axis acceleration = in_accel_y_raw * in_accel_scale = -0.5458779 m/s^2
+- Z-axis acceleration = in_accel_z_raw * in_accel_scale = 0.0191536 m/s^2
+
+Set calibration offset for accelerometer channels. Note, the calibration will be
+rounded according to the graduation of LSB units:
+
+.. code-block:: bash
+
+        root:/sys/bus/iio/devices/iio:device0> cat in_accel_x_calibbias
+        0
+
+        root:/sys/bus/iio/devices/iio:device0> echo 50 > in_accel_x_calibbias
+        root:/sys/bus/iio/devices/iio:device0> cat in_accel_x_calibbias
+        48
+
+Set sampling frequency:
+
+.. code-block:: bash
+
+        root:/sys/bus/iio/devices/iio:device0> cat in_accel_sampling_frequency
+        100.000000
+        root:/sys/bus/iio/devices/iio:device0> cat in_accel_sampling_frequency_available
+        6.250000 12.500000 25.000000 50.000000 100.000000 200.000000 400.000000 800.000000 1600.000000 3200.000000
+
+        root:/sys/bus/iio/devices/iio:device0> echo 400 > in_accel_sampling_frequency
+        root:/sys/bus/iio/devices/iio:device0> cat in_accel_sampling_frequency
+        400.000000
+
+3. Device buffers
+=================
+
+This driver supports IIO buffers.
+
+All devices support retrieving the raw acceleration measurements using buffers.
+
+Usage examples
+--------------
+
+Select channels for buffer read:
+
+.. code-block:: bash
+
+        root:/sys/bus/iio/devices/iio:device0> echo 1 > scan_elements/in_accel_x_en
+        root:/sys/bus/iio/devices/iio:device0> echo 1 > scan_elements/in_accel_y_en
+        root:/sys/bus/iio/devices/iio:device0> echo 1 > scan_elements/in_accel_z_en
+
+Set the number of samples to be stored in the buffer:
+
+.. code-block:: bash
+
+        root:/sys/bus/iio/devices/iio:device0> echo 10 > buffer/length
+
+Enable buffer readings:
+
+.. code-block:: bash
+
+        root:/sys/bus/iio/devices/iio:device0> echo 1 > buffer/enable
+
+Obtain buffered data:
+
+.. code-block:: bash
+
+        root:/sys/bus/iio/devices/iio:device0> hexdump -C /dev/iio\:device0
+        ...
+        000000d0  01 fc 31 00 c7 ff 03 fc  31 00 c7 ff 04 fc 33 00  |..1.....1.....3.|
+        000000e0  c8 ff 03 fc 32 00 c5 ff  ff fc 32 00 c7 ff 0a fc  |....2.....2.....|
+        000000f0  30 00 c8 ff 06 fc 33 00  c7 ff 01 fc 2f 00 c8 ff  |0.....3...../...|
+        00000100  02 fc 32 00 c6 ff 04 fc  33 00 c8 ff 05 fc 33 00  |..2.....3.....3.|
+        00000110  ca ff 02 fc 31 00 c7 ff  02 fc 30 00 c9 ff 09 fc  |....1.....0.....|
+        00000120  35 00 c9 ff 08 fc 35 00  c8 ff 02 fc 31 00 c5 ff  |5.....5.....1...|
+        00000130  03 fc 32 00 c7 ff 04 fc  32 00 c7 ff 02 fc 31 00  |..2.....2.....1.|
+        00000140  c7 ff 08 fc 30 00 c7 ff  02 fc 32 00 c5 ff ff fc  |....0.....2.....|
+        00000150  31 00 c5 ff 04 fc 31 00  c8 ff 03 fc 32 00 c8 ff  |1.....1.....2...|
+        00000160  01 fc 31 00 c7 ff 05 fc  31 00 c3 ff 04 fc 31 00  |..1.....1.....1.|
+        00000170  c5 ff 04 fc 30 00 c7 ff  03 fc 31 00 c9 ff 03 fc  |....0.....1.....|
+        ...
+
+See ``Documentation/iio/iio_devbuf.rst`` for more information about how buffered
+data is structured.
+
+4. IIO Interfacing Tools
+========================
+
+See ``Documentation/iio/iio_tools.rst`` for the description of the available IIO
+interfacing tools.
diff --git a/Documentation/iio/index.rst b/Documentation/iio/index.rst
index 2d6afc5a8ed5..c106402a91f7 100644
--- a/Documentation/iio/index.rst
+++ b/Documentation/iio/index.rst
@@ -31,6 +31,7 @@ Industrial I/O Kernel Drivers
    adis16475
    adis16480
    adis16550
+   adxl313
    adxl380
    bno055
    ep93xx_adc
-- 
2.39.5

Re: [PATCH v3 12/12] docs: iio: add ADXL313 accelerometer

[Jonathan Cameron]

On Fri, 23 May 2025 22:35:23 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add documentation for the ADXL313 accelerometer driver.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
Looks fine to me.

Re: [PATCH v3 12/12] docs: iio: add ADXL313 accelerometer

[Bagas Sanjaya]

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

On Fri, May 23, 2025 at 10:35:23PM +0000, Lothar Rubusch wrote:
> +A channel value can be read from its _raw attribute. The value returned is the
> +raw value as reported by the devices. To get the processed value of the channel,
> +apply the following formula:
> +
> +.. code-block:: bash
> +
> +        processed value = (_raw + _offset) * _scale

No syntax highlighting should be appropriate for this block.

> +Show accelerometer channels value:
> +
> +.. code-block:: bash
> +
> +        root:/sys/bus/iio/devices/iio:device0> cat in_accel_x_raw
> +        2
> +        root:/sys/bus/iio/devices/iio:device0> cat in_accel_y_raw
> +        -57
> +        root:/sys/bus/iio/devices/iio:device0> cat in_accel_z_raw
> +        2
> +        root:/sys/bus/iio/devices/iio:device0> cat in_accel_scale
> +        0.009576806
> +

The accelerometer values will be:

> +- X-axis acceleration = in_accel_x_raw * in_accel_scale = 0.0191536 m/s^2
> +- Y-axis acceleration = in_accel_y_raw * in_accel_scale = -0.5458779 m/s^2
> +- Z-axis acceleration = in_accel_z_raw * in_accel_scale = 0.0191536 m/s^2
> +
> +Set calibration offset for accelerometer channels. Note, the calibration will be
> +rounded according to the graduation of LSB units:

"Note that the calibration ..."

> +See ``Documentation/iio/iio_devbuf.rst`` for more information about how buffered
> +data is structured.
> +
> +4. IIO Interfacing Tools
> +========================
> +
> +See ``Documentation/iio/iio_tools.rst`` for the description of the available IIO

Do not inline docs cross-references to make them internal links.

Thanks.

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

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

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

[Jonathan Cameron]

On Fri, 23 May 2025 22:35:11 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> The patch set covers the following topics:
> - add debug register and regmap cache
> - prepare iio channel scan_type and scan_index
> - prepare interrupt handling
> - implement fifo with watermark
> - add activity/inactivity together with auto-sleep with link bit
> - documentation
> 
> Since activity and inactivity here are implemented covering all axis, I
> assumed x&y&z. Thus the driver uses a fake channel for activity/inactiviy.

Hi Lothar,

I think on this occasion you were a bit too speedy in sending out a new
version.  Might have been better to wait at least 1-2 weeks at this point
in the cycle, or until you had a few more reviews in at least.

I don't mind that much, but it does create noise on the list and tends
to reduce the focus patch sets get a little.

Jonathan

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

[Lothar Rubusch]

On Sun, May 25, 2025 at 2:50 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri, 23 May 2025 22:35:11 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > The patch set covers the following topics:
> > - add debug register and regmap cache
> > - prepare iio channel scan_type and scan_index
> > - prepare interrupt handling
> > - implement fifo with watermark
> > - add activity/inactivity together with auto-sleep with link bit
> > - documentation
> >
> > Since activity and inactivity here are implemented covering all axis, I
> > assumed x&y&z. Thus the driver uses a fake channel for activity/inactiviy.
>
> Hi Lothar,
>
> I think on this occasion you were a bit too speedy in sending out a new
> version.  Might have been better to wait at least 1-2 weeks at this point
> in the cycle, or until you had a few more reviews in at least.
>
> I don't mind that much, but it does create noise on the list and tends
> to reduce the focus patch sets get a little.
>

Hi Jonathan & ML, thank you for this hint.

I assumed Andy was just "taking over" here. In consequence the rounds
were based on his reviews. For the future, I better await your (any
IIO maintainers') reviews, until going into next round?
I accept how you like to work on this. Nevertheless, isn't it more
efficient when I resubmit right after Andy's review (if I can), then
you review and I re-submit again? In this case I would go through my
codes thoroughly twice, which usually improves quality of the result,
IMHO. Since only the most recent version of my patches should actually
be considered, the older ones could simply be ignored (not sure if it
is possible to flag this somenow from your maintainer side). I can see
the point, though, where this increases the number of mails on the
list. Nvm, just an idea. I'll wait in future.

ADXL313: I neither care much about the number of rounds, nor about the
split of patches. Thus I split rather a bit too much and you tell me
how I shall merge (I think that's easier than sending you in a big
blob patch and figuring out then what and how to separate). Pls, let
me know if you oppose to this approach?

BTW, I also still had a more recent version of the ADXL345 series,
containing the freefall and inactivity story. Current
question/proposal: Freefall and inactivity, send out the same MAG
event. An Idea could be, that userland software simply has logic to
distinguish on timing, but the kernelspace driver here is doing just
the same IIO event.

Long story short - I shifted freefall to the end (also in oder to
easily rather exclude it from that series). I expect this NOT to be
the final round. First, there is the freefall situation (actually I
expect objections from your side. If so, I'll exclude freefall from
here). Second, by some of Andys reviews I feel I also should improve
the ADXL345 a bit. I learned about regmap_assign_bits() which comes in
very handy. So, if you tell me the freefall approach in ADXL345 is
nonsense, I'll exclude it from this series.

Best,
L


> Jonathan

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

[Jonathan Cameron]

On Sun, 25 May 2025 16:54:11 +0200
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> On Sun, May 25, 2025 at 2:50 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Fri, 23 May 2025 22:35:11 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >  
> > > The patch set covers the following topics:
> > > - add debug register and regmap cache
> > > - prepare iio channel scan_type and scan_index
> > > - prepare interrupt handling
> > > - implement fifo with watermark
> > > - add activity/inactivity together with auto-sleep with link bit
> > > - documentation
> > >
> > > Since activity and inactivity here are implemented covering all axis, I
> > > assumed x&y&z. Thus the driver uses a fake channel for activity/inactiviy.  
> >
> > Hi Lothar,
> >
> > I think on this occasion you were a bit too speedy in sending out a new
> > version.  Might have been better to wait at least 1-2 weeks at this point
> > in the cycle, or until you had a few more reviews in at least.
> >
> > I don't mind that much, but it does create noise on the list and tends
> > to reduce the focus patch sets get a little.
> >  
> 
> Hi Jonathan & ML, thank you for this hint.
> 
> I assumed Andy was just "taking over" here. In consequence the rounds
> were based on his reviews. For the future, I better await your (any
> IIO maintainers') reviews, until going into next round?

We don't separate quite like that and I'm always keen to have more
people review code - so when things are going well you'll probably
get 2 or 3 people reviewing a series at some point as it is revised.

Mind you people (including me!) sometimes hide from the controversial
ABI discussions :)

So as a general rule unless we are in a rush (e.g. little tweaks
just prior to a merge) it's better to give some time and gather up
multiple reviews.  IIO tends to move quicker than some parts of the
kernel (as we have a good bunch of reviewers) but I'd still generally
not send more than one version a week for first few versions at least.

> I accept how you like to work on this. Nevertheless, isn't it more
> efficient when I resubmit right after Andy's review (if I can), then
> you review and I re-submit again? In this case I would go through my
> codes thoroughly twice, which usually improves quality of the result,
> IMHO. Since only the most recent version of my patches should actually
> be considered, the older ones could simply be ignored (not sure if it
> is possible to flag this somenow from your maintainer side). I can see
> the point, though, where this increases the number of mails on the
> list. Nvm, just an idea. I'll wait in future.

It's not a clear cut thing but I still tend to scan read through all the
comments on previous versions to avoid getting into an ill informed
argument with another reviewer, so there is little advantage in sending
a new version unless there are either major changes or review feedback
has settled down.

Also there is always testing and a small amount of overhead in a patch
series preparation so it's not free for you either. A bit of batching
up of fixing multiple sets of comments can help there too!


> 
> ADXL313: I neither care much about the number of rounds, nor about the
> split of patches. Thus I split rather a bit too much and you tell me
> how I shall merge (I think that's easier than sending you in a big
> blob patch and figuring out then what and how to separate). Pls, let
> me know if you oppose to this approach?

We all get this balance wrong sometimes and it doesn't matter if it
ends up slightly off.  Patches that just change a few values that
aren't used until quite a few patches later are generally a bad idea.

Also size of patch plays into this.  If it's under 500 lines
it is much less likely you'll get a request to split (as long as
arguably it's all one feature) than if it is larger than that.

> 
> BTW, I also still had a more recent version of the ADXL345 series,
> containing the freefall and inactivity story. Current
> question/proposal: Freefall and inactivity, send out the same MAG
> event. An Idea could be, that userland software simply has logic to
> distinguish on timing, but the kernelspace driver here is doing just
> the same IIO event.
> 
> Long story short - I shifted freefall to the end (also in oder to
> easily rather exclude it from that series). I expect this NOT to be
> the final round. First, there is the freefall situation (actually I
> expect objections from your side. If so, I'll exclude freefall from
> here). Second, by some of Andys reviews I feel I also should improve
> the ADXL345 a bit. I learned about regmap_assign_bits() which comes in
> very handy. So, if you tell me the freefall approach in ADXL345 is
> nonsense, I'll exclude it from this series.

I got to that a bit later in the day.  I was expecting it to take
more thought so left it to nearly the end when catching up.

I've replied in that thread to the freefall proposal. It's not
nonsense of course, I just don't think we have figured out a way
to avoid the expectations built into userspace code that is already
out there.

Note I'm also not sure about the innovative use of IIO_MOD_STILL
(which is a human motion thing to indicate not running, walking etc)
that the cros_ec activity sensor that was posted this week used.
That was rather unexpected. They do have the small advantage over
most drivers that they also control the relevant userspace (chrome OS
I believe). 

Seems like we are in a period of pushing against the boundaries
of the ABI :(

BTW regmap_assign_bits() was new to me too ;)

Have a good remainder of the weekend!

Jonathan
> 
> Best,
> L
> 
> 
> > Jonathan