[PATCH v8 0/7] iio: accel: adxl345: add FIFO operating with IRQ triggered watermark events

[PATCH v8 0/7] iio: accel: adxl345: add FIFO operating with IRQ triggered watermark events

[Lothar Rubusch]

The adxl345 sensor offers several features. Most of them are based on
using the hardware FIFO and reacting on events coming in on an interrupt
line. Add access to configure and read out the FIFO, handling of interrupts
and configuration and application of the watermark feature on that FIFO.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
v7 -> v8:
- remove dt-binding quotation marks
- add DMA alignment
- fix indention, formatting and text alignment
v6 -> v7:
- reorder dt-binding patches
- extracted FIFO specific from constants list
- reorder constants list in header patch to the end
- verify watermark input is within valid range
v5 -> v6:
- dropped justify patch, since unnecessary change to format mask
- added separate dt-bindings patch to remove required interrupts property
- merged FIFO watermark patches
- reworked bitfield handling
- group irq setup in probe()
- several type fixes by smatch and tools
v4 -> v5:
- fix dt-binding for enum array of INT1 and INT2
v3 -> v4:
- fix dt-binding indention 
v2 -> v3:
- reorganize commits, merge the watermark handling
- INT lines are defined by binding
- kfifo is prepared by devm_iio_kfifo_buffer_setup()
- event handler is registered w/ devm_request_threaded_irq()
v1 -> v2:
Fix comments according to Documentation/doc-guide/kernel-doc.rst
and missing static declaration of function.
---

Lothar Rubusch (7):
  iio: accel: adxl345: add function to switch measuring mode
  dt-bindings: iio: accel: adxl345: make interrupts not a required
    property
  dt-bindings: iio: accel: adxl345: add interrupt-names
  iio: accel: adxl345: introduce interrupt handling
  iio: accel: adxl345: initialize FIFO delay value for SPI
  iio: accel: adxl345: add FIFO with watermark events
  iio: accel: adxl345: complete the list of defines

 .../bindings/iio/accel/adi,adxl345.yaml       |  11 +-
 drivers/iio/accel/adxl345.h                   |  85 ++++-
 drivers/iio/accel/adxl345_core.c              | 335 +++++++++++++++++-
 drivers/iio/accel/adxl345_i2c.c               |   2 +-
 drivers/iio/accel/adxl345_spi.c               |   7 +-
 5 files changed, 417 insertions(+), 23 deletions(-)

-- 
2.39.5

[PATCH v8 1/7] iio: accel: adxl345: add function to switch measuring mode

[Lothar Rubusch]

Replace the powerup / powerdown functions by a generic function to put
the sensor in STANDBY, or MEASURE mode. When configuring the FIFO for
several features of the accelerometer, it is recommended to put
measuring in STANDBY mode.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
Please squash with a3fb9f5202c3de0ca84848a475f59a0e0584d9fc

Just introduces a cosmetic simplification to the already applied
commit, under the same title.

 drivers/iio/accel/adxl345_core.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index b48bc838c..27d70a1f0 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -153,9 +153,8 @@ static int adxl345_write_raw_get_fmt(struct iio_dev *indio_dev,
  */
 static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
 {
-	unsigned int val = 0;
+	unsigned int val = en ? ADXL345_POWER_CTL_MEASURE : ADXL345_POWER_CTL_STANDBY;
 
-	val = (en) ? ADXL345_POWER_CTL_MEASURE : ADXL345_POWER_CTL_STANDBY;
 	return regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
 }
 
-- 
2.39.5

[PATCH v8 2/7] dt-bindings: iio: accel: adxl345: make interrupts not a required property

[Lothar Rubusch]

Remove interrupts from the list of required properties. The ADXL345
does not need interrupts for basic accelerometer functionality.

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml | 1 -
 1 file changed, 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
index 280ed479e..bc46ed00f 100644
--- a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
+++ b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
@@ -40,7 +40,6 @@ properties:
 required:
   - compatible
   - reg
-  - interrupts
 
 allOf:
   - $ref: /schemas/spi/spi-peripheral-props.yaml#
-- 
2.39.5

[PATCH v8 3/7] dt-bindings: iio: accel: adxl345: add interrupt-names

[Lothar Rubusch]

Add interrupt-names INT1 and INT2 for the two interrupt lines of the
sensor.

When one of the two interrupt lines is connected, the interrupt as its
interrupt-name, need to be declared in the devicetree. The driver then
configures the sensor to indicate its events on either INT1 or INT2.

If no interrupt is configured, then no interrupt-name should be
configured, and vice versa. In this case the sensor runs in FIFO BYPASS
mode. This allows sensor measurements, but none of the sensor events.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 .../devicetree/bindings/iio/accel/adi,adxl345.yaml     | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
index bc46ed00f..84d949392 100644
--- a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
+++ b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
@@ -37,6 +37,14 @@ properties:
   interrupts:
     maxItems: 1
 
+  interrupt-names:
+    items:
+      - enum: [INT1, INT2]
+
+dependencies:
+  interrupts: [ interrupt-names ]
+  interrupt-names: [ interrupts ]
+
 required:
   - compatible
   - reg
@@ -60,6 +68,7 @@ examples:
             reg = <0x2a>;
             interrupt-parent = <&gpio0>;
             interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
+            interrupt-names = "INT1";
         };
     };
   - |
@@ -78,5 +87,6 @@ examples:
             spi-cpha;
             interrupt-parent = <&gpio0>;
             interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
+            interrupt-names = "INT2";
         };
     };
-- 
2.39.5

[PATCH v8 4/7] iio: accel: adxl345: introduce interrupt handling

[Lothar Rubusch]

Add the possibility to claim an interrupt. Init the state structure
with an interrupt line obtained from the DT. The adxl345 can use
two different interrupt lines for event handling. Only one is used.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 27d70a1f0..134e72540 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -7,6 +7,7 @@
  * Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ADXL345.pdf
  */
 
+#include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/property.h>
 #include <linux/regmap.h>
@@ -17,9 +18,15 @@
 
 #include "adxl345.h"
 
+#define ADXL345_INT_NONE		0xff
+#define ADXL345_INT1			0
+#define ADXL345_INT2			1
+
 struct adxl345_state {
 	const struct adxl345_chip_info *info;
 	struct regmap *regmap;
+	int irq;
+	u8 intio;
 };
 
 #define ADXL345_CHANNEL(index, axis) {					\
@@ -262,6 +269,15 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 	if (ret < 0)
 		return ret;
 
+	st->intio = ADXL345_INT1;
+	st->irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT1");
+	if (st->irq < 0) {
+		st->intio = ADXL345_INT2;
+		st->irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT2");
+		if (st->irq < 0)
+			st->intio = ADXL345_INT_NONE;
+	}
+
 	return devm_iio_device_register(dev, indio_dev);
 }
 EXPORT_SYMBOL_NS_GPL(adxl345_core_probe, IIO_ADXL345);
-- 
2.39.5

[PATCH v8 5/7] iio: accel: adxl345: initialize FIFO delay value for SPI

[Lothar Rubusch]

Add the possibility to delay FIFO access when SPI is used. According to
the datasheet this is needed for the adxl345. When initialization
happens over SPI the need for delay is to be signalized, and the delay
will be used.

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

diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
index 3d5c8719d..6f39f16d3 100644
--- a/drivers/iio/accel/adxl345.h
+++ b/drivers/iio/accel/adxl345.h
@@ -62,6 +62,7 @@ struct adxl345_chip_info {
 };
 
 int adxl345_core_probe(struct device *dev, struct regmap *regmap,
+		       bool fifo_delay_default,
 		       int (*setup)(struct device*, struct regmap*));
 
 #endif /* _ADXL345_H_ */
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 134e72540..987a0fe03 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -25,6 +25,7 @@
 struct adxl345_state {
 	const struct adxl345_chip_info *info;
 	struct regmap *regmap;
+	bool fifo_delay; /* delay: delay is needed for SPI */
 	int irq;
 	u8 intio;
 };
@@ -196,12 +197,21 @@ static const struct iio_info adxl345_info = {
  * adxl345_core_probe() - Probe and setup for the accelerometer.
  * @dev:	Driver model representation of the device
  * @regmap:	Regmap instance for the device
+ * @fifo_delay_default: Using FIFO with SPI needs delay
  * @setup:	Setup routine to be executed right before the standard device
  *		setup
  *
+ * For SPI operation greater than 1.6 MHz, it is necessary to deassert the CS
+ * pin to ensure a total delay of 5 us; otherwise, the delay is not sufficient.
+ * The total delay necessary for 5 MHz operation is at most 3.4 us. This is not
+ * a concern when using I2C mode because the communication rate is low enough
+ * to ensure a sufficient delay between FIFO reads.
+ * Ref: "Retrieving Data from FIFO", p. 21 of 36, Data Sheet ADXL345 Rev. G
+ *
  * Return: 0 on success, negative errno on error
  */
 int adxl345_core_probe(struct device *dev, struct regmap *regmap,
+		       bool fifo_delay_default,
 		       int (*setup)(struct device*, struct regmap*))
 {
 	struct adxl345_state *st;
@@ -222,6 +232,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 	st->info = device_get_match_data(dev);
 	if (!st->info)
 		return -ENODEV;
+	st->fifo_delay = fifo_delay_default;
 
 	indio_dev->name = st->info->name;
 	indio_dev->info = &adxl345_info;
diff --git a/drivers/iio/accel/adxl345_i2c.c b/drivers/iio/accel/adxl345_i2c.c
index e550bc078..eb3e0aadf 100644
--- a/drivers/iio/accel/adxl345_i2c.c
+++ b/drivers/iio/accel/adxl345_i2c.c
@@ -27,7 +27,7 @@ static int adxl345_i2c_probe(struct i2c_client *client)
 	if (IS_ERR(regmap))
 		return dev_err_probe(&client->dev, PTR_ERR(regmap), "Error initializing regmap\n");
 
-	return adxl345_core_probe(&client->dev, regmap, NULL);
+	return adxl345_core_probe(&client->dev, regmap, false, NULL);
 }
 
 static const struct adxl345_chip_info adxl345_i2c_info = {
diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl345_spi.c
index 61fd9a6f5..e03915ece 100644
--- a/drivers/iio/accel/adxl345_spi.c
+++ b/drivers/iio/accel/adxl345_spi.c
@@ -12,6 +12,7 @@
 #include "adxl345.h"
 
 #define ADXL345_MAX_SPI_FREQ_HZ		5000000
+#define ADXL345_MAX_FREQ_NO_FIFO_DELAY	1500000
 
 static const struct regmap_config adxl345_spi_regmap_config = {
 	.reg_bits = 8,
@@ -28,6 +29,7 @@ static int adxl345_spi_setup(struct device *dev, struct regmap *regmap)
 static int adxl345_spi_probe(struct spi_device *spi)
 {
 	struct regmap *regmap;
+	bool needs_delay;
 
 	/* Bail out if max_speed_hz exceeds 5 MHz */
 	if (spi->max_speed_hz > ADXL345_MAX_SPI_FREQ_HZ)
@@ -38,10 +40,11 @@ static int adxl345_spi_probe(struct spi_device *spi)
 	if (IS_ERR(regmap))
 		return dev_err_probe(&spi->dev, PTR_ERR(regmap), "Error initializing regmap\n");
 
+	needs_delay = spi->max_speed_hz > ADXL345_MAX_FREQ_NO_FIFO_DELAY;
 	if (spi->mode & SPI_3WIRE)
-		return adxl345_core_probe(&spi->dev, regmap, adxl345_spi_setup);
+		return adxl345_core_probe(&spi->dev, regmap, needs_delay, adxl345_spi_setup);
 	else
-		return adxl345_core_probe(&spi->dev, regmap, NULL);
+		return adxl345_core_probe(&spi->dev, regmap, needs_delay, NULL);
 }
 
 static const struct adxl345_chip_info adxl345_spi_info = {
-- 
2.39.5

[PATCH v8 6/7] iio: accel: adxl345: add FIFO with watermark events

[Lothar Rubusch]

Add a basic setup for FIFO with configurable watermark. Add a handler
for watermark interrupt events and extend the channel for the
scan_index needed for the iio channel. The sensor is configurable to use
a FIFO_BYPASSED mode or a FIFO_STREAM mode. For the FIFO_STREAM mode now
a watermark can be configured, or disabled by setting 0. Further features
require a working FIFO setup.

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

diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
index 6f39f16d3..bf9e86cff 100644
--- a/drivers/iio/accel/adxl345.h
+++ b/drivers/iio/accel/adxl345.h
@@ -15,18 +15,32 @@
 #define ADXL345_REG_OFS_AXIS(index)	(ADXL345_REG_OFSX + (index))
 #define ADXL345_REG_BW_RATE		0x2C
 #define ADXL345_REG_POWER_CTL		0x2D
+#define ADXL345_REG_INT_ENABLE		0x2E
+#define ADXL345_REG_INT_MAP		0x2F
+#define ADXL345_REG_INT_SOURCE		0x30
+#define ADXL345_REG_INT_SOURCE_MSK	0xFF
 #define ADXL345_REG_DATA_FORMAT		0x31
-#define ADXL345_REG_DATAX0		0x32
-#define ADXL345_REG_DATAY0		0x34
-#define ADXL345_REG_DATAZ0		0x36
-#define ADXL345_REG_DATA_AXIS(index)	\
-	(ADXL345_REG_DATAX0 + (index) * sizeof(__le16))
+#define ADXL345_REG_XYZ_BASE		0x32
+#define ADXL345_REG_DATA_AXIS(index)				\
+	(ADXL345_REG_XYZ_BASE + (index) * sizeof(__le16))
 
+#define ADXL345_REG_FIFO_CTL		0x38
+#define ADXL345_REG_FIFO_STATUS	0x39
+#define ADXL345_REG_FIFO_STATUS_MSK	0x3F
+
+#define ADXL345_FIFO_CTL_SAMPLES(x)	FIELD_PREP(GENMASK(4, 0), x)
+/* 0: INT1, 1: INT2 */
+#define ADXL345_FIFO_CTL_TRIGGER(x)	FIELD_PREP(BIT(5), x)
+#define ADXL345_FIFO_CTL_MODE(x)	FIELD_PREP(GENMASK(7, 6), x)
+
+#define ADXL345_INT_DATA_READY		BIT(7)
+#define ADXL345_INT_WATERMARK		BIT(1)
+#define ADXL345_INT_OVERRUN		BIT(0)
 #define ADXL345_BW_RATE			GENMASK(3, 0)
 #define ADXL345_BASE_RATE_NANO_HZ	97656250LL
 
-#define ADXL345_POWER_CTL_MEASURE	BIT(3)
 #define ADXL345_POWER_CTL_STANDBY	0x00
+#define ADXL345_POWER_CTL_MEASURE	BIT(3)
 
 #define ADXL345_DATA_FORMAT_RANGE	GENMASK(1, 0)	/* Set the g range */
 #define ADXL345_DATA_FORMAT_JUSTIFY	BIT(2)	/* Left-justified (MSB) mode */
@@ -40,6 +54,7 @@
 #define ADXL345_DATA_FORMAT_16G		3
 
 #define ADXL345_DEVID			0xE5
+#define ADXL345_FIFO_SIZE		32
 
 /*
  * In full-resolution mode, scale factor is maintained at ~4 mg/LSB
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 987a0fe03..738960298 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -15,9 +15,17 @@
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/kfifo_buf.h>
 
 #include "adxl345.h"
 
+#define ADXL345_FIFO_BYPASS	0
+#define ADXL345_FIFO_FIFO	1
+#define ADXL345_FIFO_STREAM	2
+
+#define ADXL345_DIRS 3
+
 #define ADXL345_INT_NONE		0xff
 #define ADXL345_INT1			0
 #define ADXL345_INT2			1
@@ -28,25 +36,66 @@ struct adxl345_state {
 	bool fifo_delay; /* delay: delay is needed for SPI */
 	int irq;
 	u8 intio;
+	u8 int_map;
+	u8 watermark;
+	u8 fifo_mode;
+	__le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE + 1] __aligned(IIO_DMA_MINALIGN);
 };
 
-#define ADXL345_CHANNEL(index, axis) {					\
+#define ADXL345_CHANNEL(index, reg, axis) {					\
 	.type = IIO_ACCEL,						\
 	.modified = 1,							\
 	.channel2 = IIO_MOD_##axis,					\
-	.address = index,						\
+	.address = (reg),						\
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
 		BIT(IIO_CHAN_INFO_CALIBBIAS),				\
 	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |		\
 		BIT(IIO_CHAN_INFO_SAMP_FREQ),				\
+	.scan_index = (index),				\
+	.scan_type = {					\
+		.sign = 's',				\
+		.realbits = 13,				\
+		.storagebits = 16,			\
+		.endianness = IIO_LE,			\
+	},						\
 }
 
+enum adxl345_chans {
+	chan_x, chan_y, chan_z,
+};
+
 static const struct iio_chan_spec adxl345_channels[] = {
-	ADXL345_CHANNEL(0, X),
-	ADXL345_CHANNEL(1, Y),
-	ADXL345_CHANNEL(2, Z),
+	ADXL345_CHANNEL(0, chan_x, X),
+	ADXL345_CHANNEL(1, chan_y, Y),
+	ADXL345_CHANNEL(2, chan_z, Z),
 };
 
+static const unsigned long adxl345_scan_masks[] = {
+	BIT(chan_x) | BIT(chan_y) | BIT(chan_z),
+	0
+};
+
+static int adxl345_set_interrupts(struct adxl345_state *st)
+{
+	int ret;
+	unsigned int int_enable = st->int_map;
+	unsigned int int_map;
+
+	/*
+	 * Any bits set to 0 in the INT map register send their respective
+	 * interrupts to the INT1 pin, whereas bits set to 1 send their respective
+	 * interrupts to the INT2 pin. The intio shall convert this accordingly.
+	 */
+	int_map = FIELD_GET(ADXL345_REG_INT_SOURCE_MSK,
+			    st->intio ? st->int_map : ~st->int_map);
+
+	ret = regmap_write(st->regmap, ADXL345_REG_INT_MAP, int_map);
+	if (ret)
+		return ret;
+
+	return regmap_write(st->regmap, ADXL345_REG_INT_ENABLE, int_enable);
+}
+
 static int adxl345_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan,
 			    int *val, int *val2, long mask)
@@ -132,6 +181,24 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
 	return -EINVAL;
 }
 
+static int adxl345_set_watermark(struct iio_dev *indio_dev, unsigned int value)
+{
+	struct adxl345_state *st = iio_priv(indio_dev);
+	unsigned int fifo_mask = 0x1F;
+	int ret;
+
+	value = min(value, ADXL345_FIFO_SIZE - 1);
+
+	ret = regmap_update_bits(st->regmap, ADXL345_REG_FIFO_CTL, fifo_mask, value);
+	if (ret)
+		return ret;
+
+	st->watermark = value;
+	st->int_map |= ADXL345_INT_WATERMARK;
+
+	return 0;
+}
+
 static int adxl345_write_raw_get_fmt(struct iio_dev *indio_dev,
 				     struct iio_chan_spec const *chan,
 				     long mask)
@@ -186,11 +253,217 @@ static const struct attribute_group adxl345_attrs_group = {
 	.attrs = adxl345_attrs,
 };
 
+static int adxl345_set_fifo(struct adxl345_state *st)
+{
+	int ret;
+
+	/* FIFO should only be configured while in standby mode */
+	ret = adxl345_set_measure_en(st, false);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_write(st->regmap, ADXL345_REG_FIFO_CTL,
+			   ADXL345_FIFO_CTL_SAMPLES(st->watermark) |
+			   ADXL345_FIFO_CTL_TRIGGER(st->intio) |
+			   ADXL345_FIFO_CTL_MODE(st->fifo_mode));
+	if (ret < 0)
+		return ret;
+
+	return adxl345_set_measure_en(st, true);
+}
+
+/**
+ * adxl345_get_samples() - Read number of FIFO entries.
+ * @st: The initialized state instance of this driver.
+ *
+ * The sensor does not support treating any axis individually, or exclude them
+ * from measuring.
+ *
+ * Return: negative error, or value.
+ */
+static int adxl345_get_samples(struct adxl345_state *st)
+{
+	unsigned int regval = 0;
+	int ret;
+
+	ret = regmap_read(st->regmap, ADXL345_REG_FIFO_STATUS, &regval);
+	if (ret < 0)
+		return ret;
+
+	return FIELD_GET(ADXL345_REG_FIFO_STATUS_MSK, regval);
+}
+
+/**
+ * adxl345_fifo_transfer() - Read samples number of elements.
+ * @st: The instance of the state object of this sensor.
+ * @samples: The number of lines in the FIFO referred to as fifo_entry.
+ *
+ * It is recommended that a multiple-byte read of all registers be performed to
+ * prevent a change in data between reads of sequential registers. That is to
+ * read out the data registers X0, X1, Y0, Y1, Z0, Z1, i.e. 6 bytes at once.
+ *
+ * Return: 0 or error value.
+ */
+static int adxl345_fifo_transfer(struct adxl345_state *st, int samples)
+{
+	size_t count;
+	int i, ret = 0;
+
+	/* count is the 3x the fifo_buf element size, hence 6B */
+	count = sizeof(st->fifo_buf[0]) * ADXL345_DIRS;
+	for (i = 0; i < samples; i++) {
+		/* read 3x 2 byte elements from base address into next fifo_buf position */
+		ret = regmap_bulk_read(st->regmap, ADXL345_REG_XYZ_BASE,
+				       st->fifo_buf + (i * count / 2), count);
+		if (ret < 0)
+			return ret;
+
+		/*
+		 * To ensure that the FIFO has completely popped, there must be at least 5
+		 * us between the end of reading the data registers, signified by the
+		 * transition to register 0x38 from 0x37 or the CS pin going high, and the
+		 * start of new reads of the FIFO or reading the FIFO_STATUS register. For
+		 * SPI operation at 1.5 MHz or lower, the register addressing portion of the
+		 * transmission is sufficient delay to ensure the FIFO has completely
+		 * popped. It is necessary for SPI operation greater than 1.5 MHz to
+		 * de-assert the CS pin to ensure a total of 5 us, which is at most 3.4 us
+		 * at 5 MHz operation.
+		 */
+		if (st->fifo_delay && samples > 1)
+			udelay(3);
+	}
+	return ret;
+}
+
+/**
+ * adxl345_fifo_reset() - Empty the FIFO in error condition.
+ * @st: The instance to the state object of the sensor.
+ *
+ * Read all elements of the FIFO. Reading the interrupt source register
+ * resets the sensor.
+ */
+static void adxl345_fifo_reset(struct adxl345_state *st)
+{
+	int regval;
+	int samples;
+
+	adxl345_set_measure_en(st, false);
+
+	samples = adxl345_get_samples(st);
+	if (samples > 0)
+		adxl345_fifo_transfer(st, samples);
+
+	regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, &regval);
+
+	adxl345_set_measure_en(st, true);
+}
+
+static int adxl345_buffer_postenable(struct iio_dev *indio_dev)
+{
+	struct adxl345_state *st = iio_priv(indio_dev);
+	int ret;
+
+	ret = adxl345_set_interrupts(st);
+	if (ret < 0)
+		return ret;
+
+	st->fifo_mode = ADXL345_FIFO_STREAM;
+	return adxl345_set_fifo(st);
+}
+
+static int adxl345_buffer_predisable(struct iio_dev *indio_dev)
+{
+	struct adxl345_state *st = iio_priv(indio_dev);
+	int ret;
+
+	st->fifo_mode = ADXL345_FIFO_BYPASS;
+	ret = adxl345_set_fifo(st);
+	if (ret < 0)
+		return ret;
+
+	st->int_map = 0x00;
+	return adxl345_set_interrupts(st);
+}
+
+static const struct iio_buffer_setup_ops adxl345_buffer_ops = {
+	.postenable = adxl345_buffer_postenable,
+	.predisable = adxl345_buffer_predisable,
+};
+
+static int adxl345_get_status(struct adxl345_state *st)
+{
+	int ret;
+	unsigned int regval;
+
+	ret = regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, &regval);
+	if (ret < 0)
+		return ret;
+
+	return FIELD_GET(ADXL345_REG_INT_SOURCE_MSK, regval);
+}
+
+static int adxl345_fifo_push(struct iio_dev *indio_dev,
+			     int samples)
+{
+	struct adxl345_state *st = iio_priv(indio_dev);
+	int i, ret;
+
+	if (samples <= 0)
+		return -EINVAL;
+
+	ret = adxl345_fifo_transfer(st, samples);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < ADXL345_DIRS * samples; i += ADXL345_DIRS)
+		iio_push_to_buffers(indio_dev, &st->fifo_buf[i]);
+
+	return 0;
+}
+
+/**
+ * adxl345_irq_handler() - Handle irqs of the ADXL345.
+ * @irq: The irq being handled.
+ * @p: The struct iio_device pointer for the device.
+ *
+ * Return: The interrupt was handled.
+ */
+static irqreturn_t adxl345_irq_handler(int irq, void *p)
+{
+	struct iio_dev *indio_dev = p;
+	struct adxl345_state *st = iio_priv(indio_dev);
+	int int_stat;
+	int samples;
+
+	int_stat = adxl345_get_status(st);
+	if (int_stat <= 0)
+		return IRQ_NONE;
+
+	if (int_stat & ADXL345_INT_OVERRUN)
+		goto err;
+
+	if (int_stat & ADXL345_INT_WATERMARK) {
+		samples = adxl345_get_samples(st);
+		if (samples < 0)
+			goto err;
+
+		if (adxl345_fifo_push(indio_dev, samples) < 0)
+			goto err;
+	}
+	return IRQ_HANDLED;
+
+err:
+	adxl345_fifo_reset(st);
+
+	return IRQ_HANDLED;
+}
+
 static const struct iio_info adxl345_info = {
 	.attrs		= &adxl345_attrs_group,
 	.read_raw	= adxl345_read_raw,
 	.write_raw	= adxl345_write_raw,
 	.write_raw_get_fmt	= adxl345_write_raw_get_fmt,
+	.hwfifo_set_watermark = adxl345_set_watermark,
 };
 
 /**
@@ -221,6 +494,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 					 ADXL345_DATA_FORMAT_JUSTIFY |
 					 ADXL345_DATA_FORMAT_FULL_RES |
 					 ADXL345_DATA_FORMAT_SELF_TEST);
+	u8 fifo_ctl;
 	int ret;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
@@ -239,6 +513,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->channels = adxl345_channels;
 	indio_dev->num_channels = ARRAY_SIZE(adxl345_channels);
+	indio_dev->available_scan_masks = adxl345_scan_masks;
 
 	if (setup) {
 		/* Perform optional initial bus specific configuration */
@@ -289,6 +564,26 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 			st->intio = ADXL345_INT_NONE;
 	}
 
+	if (st->intio != ADXL345_INT_NONE) {
+		/* FIFO_STREAM mode is going to be activated later */
+		ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, &adxl345_buffer_ops);
+		if (ret)
+			return ret;
+
+		ret = devm_request_threaded_irq(dev, st->irq, NULL,
+						&adxl345_irq_handler,
+						IRQF_SHARED | IRQF_ONESHOT,
+						indio_dev->name, indio_dev);
+		if (ret)
+			return ret;
+	} else {
+		/* FIFO_BYPASS mode */
+		fifo_ctl = ADXL345_FIFO_CTL_MODE(ADXL345_FIFO_BYPASS);
+		ret = regmap_write(st->regmap, ADXL345_REG_FIFO_CTL, fifo_ctl);
+		if (ret < 0)
+			return ret;
+	}
+
 	return devm_iio_device_register(dev, indio_dev);
 }
 EXPORT_SYMBOL_NS_GPL(adxl345_core_probe, IIO_ADXL345);
-- 
2.39.5

[PATCH v8 7/7] iio: accel: adxl345: complete the list of defines

[Lothar Rubusch]

Having interrupts events and FIFO available allows to evaluate the
sensor events. Cover the list of interrupt based sensor events. Keep
them in the header file for readability.

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

diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
index bf9e86cff..df3977bda 100644
--- a/drivers/iio/accel/adxl345.h
+++ b/drivers/iio/accel/adxl345.h
@@ -9,10 +9,35 @@
 #define _ADXL345_H_
 
 #define ADXL345_REG_DEVID		0x00
+#define ADXL345_REG_THRESH_TAP		0x1D
 #define ADXL345_REG_OFSX		0x1E
 #define ADXL345_REG_OFSY		0x1F
 #define ADXL345_REG_OFSZ		0x20
 #define ADXL345_REG_OFS_AXIS(index)	(ADXL345_REG_OFSX + (index))
+
+/* Tap duration */
+#define ADXL345_REG_DUR		0x21
+/* Tap latency */
+#define ADXL345_REG_LATENT		0x22
+/* Tap window */
+#define ADXL345_REG_WINDOW		0x23
+/* Activity threshold */
+#define ADXL345_REG_THRESH_ACT		0x24
+/* Inactivity threshold */
+#define ADXL345_REG_THRESH_INACT	0x25
+/* Inactivity time */
+#define ADXL345_REG_TIME_INACT		0x26
+/* Axis enable control for activity and inactivity detection */
+#define ADXL345_REG_ACT_INACT_CTRL	0x27
+/* Free-fall threshold */
+#define ADXL345_REG_THRESH_FF		0x28
+/* Free-fall time */
+#define ADXL345_REG_TIME_FF		0x29
+/* Axis control for single tap or double tap */
+#define ADXL345_REG_TAP_AXIS		0x2A
+/* Source of single tap or double tap */
+#define ADXL345_REG_ACT_TAP_STATUS	0x2B
+/* Data rate and power mode control */
 #define ADXL345_REG_BW_RATE		0x2C
 #define ADXL345_REG_POWER_CTL		0x2D
 #define ADXL345_REG_INT_ENABLE		0x2E
@@ -34,20 +59,40 @@
 #define ADXL345_FIFO_CTL_MODE(x)	FIELD_PREP(GENMASK(7, 6), x)
 
 #define ADXL345_INT_DATA_READY		BIT(7)
+#define ADXL345_INT_SINGLE_TAP		BIT(6)
+#define ADXL345_INT_DOUBLE_TAP		BIT(5)
+#define ADXL345_INT_ACTIVITY		BIT(4)
+#define ADXL345_INT_INACTIVITY		BIT(3)
+#define ADXL345_INT_FREE_FALL		BIT(2)
 #define ADXL345_INT_WATERMARK		BIT(1)
 #define ADXL345_INT_OVERRUN		BIT(0)
+
+#define ADXL345_S_TAP_MSK	ADXL345_INT_SINGLE_TAP
+#define ADXL345_D_TAP_MSK	ADXL345_INT_DOUBLE_TAP
+
+/*
+ * BW_RATE bits - Bandwidth and output data rate. The default value is
+ * 0x0A, which translates to a 100 Hz output data rate
+ */
 #define ADXL345_BW_RATE			GENMASK(3, 0)
+#define ADXL345_BW_LOW_POWER		BIT(4)
 #define ADXL345_BASE_RATE_NANO_HZ	97656250LL
 
 #define ADXL345_POWER_CTL_STANDBY	0x00
+#define ADXL345_POWER_CTL_WAKEUP	GENMASK(1, 0)
+#define ADXL345_POWER_CTL_SLEEP	BIT(2)
 #define ADXL345_POWER_CTL_MEASURE	BIT(3)
+#define ADXL345_POWER_CTL_AUTO_SLEEP	BIT(4)
+#define ADXL345_POWER_CTL_LINK		BIT(5)
 
-#define ADXL345_DATA_FORMAT_RANGE	GENMASK(1, 0)	/* Set the g range */
-#define ADXL345_DATA_FORMAT_JUSTIFY	BIT(2)	/* Left-justified (MSB) mode */
-#define ADXL345_DATA_FORMAT_FULL_RES	BIT(3)	/* Up to 13-bits resolution */
-#define ADXL345_DATA_FORMAT_SPI_3WIRE	BIT(6)	/* 3-wire SPI mode */
-#define ADXL345_DATA_FORMAT_SELF_TEST	BIT(7)	/* Enable a self test */
-
+/* Set the g range */
+#define ADXL345_DATA_FORMAT_RANGE	GENMASK(1, 0)
+/* Data is left justified */
+#define ADXL345_DATA_FORMAT_JUSTIFY	BIT(2)
+/* Up to 13-bits resolution */
+#define ADXL345_DATA_FORMAT_FULL_RES	BIT(3)
+#define ADXL345_DATA_FORMAT_SPI_3WIRE	BIT(6)
+#define ADXL345_DATA_FORMAT_SELF_TEST	BIT(7)
 #define ADXL345_DATA_FORMAT_2G		0
 #define ADXL345_DATA_FORMAT_4G		1
 #define ADXL345_DATA_FORMAT_8G		2
-- 
2.39.5

Re: [PATCH v8 3/7] dt-bindings: iio: accel: adxl345: add interrupt-names

[Krzysztof Kozlowski]

On Wed, Dec 25, 2024 at 06:13:34PM +0000, Lothar Rubusch wrote:
> Add interrupt-names INT1 and INT2 for the two interrupt lines of the
> sensor.
> 
> When one of the two interrupt lines is connected, the interrupt as its
> interrupt-name, need to be declared in the devicetree. The driver then
> configures the sensor to indicate its events on either INT1 or INT2.
> 
> If no interrupt is configured, then no interrupt-name should be
> configured, and vice versa. In this case the sensor runs in FIFO BYPASS
> mode. This allows sensor measurements, but none of the sensor events.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v8 1/7] iio: accel: adxl345: add function to switch measuring mode

[Jonathan Cameron]

On Wed, 25 Dec 2024 18:13:32 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Replace the powerup / powerdown functions by a generic function to put
> the sensor in STANDBY, or MEASURE mode. When configuring the FIFO for
> several features of the accelerometer, it is recommended to put
> measuring in STANDBY mode.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
> Please squash with a3fb9f5202c3de0ca84848a475f59a0e0584d9fc
Even when just sending something you'd like squashed please give
meaningful title etc because it might not get squashed and so may need
to stand on it's own.

Anyhow, I have squashed it so no problem this time.

Jonathan

> 
> Just introduces a cosmetic simplification to the already applied
> commit, under the same title.
> 
>  drivers/iio/accel/adxl345_core.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index b48bc838c..27d70a1f0 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -153,9 +153,8 @@ static int adxl345_write_raw_get_fmt(struct iio_dev *indio_dev,
>   */
>  static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
>  {
> -	unsigned int val = 0;
> +	unsigned int val = en ? ADXL345_POWER_CTL_MEASURE : ADXL345_POWER_CTL_STANDBY;
>  
> -	val = (en) ? ADXL345_POWER_CTL_MEASURE : ADXL345_POWER_CTL_STANDBY;
>  	return regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
>  }
>  

Re: [PATCH v8 2/7] dt-bindings: iio: accel: adxl345: make interrupts not a required property

[Jonathan Cameron]

On Wed, 25 Dec 2024 18:13:33 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Remove interrupts from the list of required properties. The ADXL345
> does not need interrupts for basic accelerometer functionality.
> 
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
Applied.

Thanks,

J
> ---
>  Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> index 280ed479e..bc46ed00f 100644
> --- a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> +++ b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> @@ -40,7 +40,6 @@ properties:
>  required:
>    - compatible
>    - reg
> -  - interrupts
>  
>  allOf:
>    - $ref: /schemas/spi/spi-peripheral-props.yaml#

Re: [PATCH v8 3/7] dt-bindings: iio: accel: adxl345: add interrupt-names

[Jonathan Cameron]

On Fri, 27 Dec 2024 08:58:00 +0100
Krzysztof Kozlowski <krzk@kernel.org> wrote:

> On Wed, Dec 25, 2024 at 06:13:34PM +0000, Lothar Rubusch wrote:
> > Add interrupt-names INT1 and INT2 for the two interrupt lines of the
> > sensor.
> > 
> > When one of the two interrupt lines is connected, the interrupt as its
> > interrupt-name, need to be declared in the devicetree. The driver then
> > configures the sensor to indicate its events on either INT1 or INT2.
> > 
> > If no interrupt is configured, then no interrupt-name should be
> > configured, and vice versa. In this case the sensor runs in FIFO BYPASS
> > mode. This allows sensor measurements, but none of the sensor events.
> > 
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>  
> 
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> Best regards,
> Krzysztof
> 
Applied. thanks

Jonathan

Re: [PATCH v8 6/7] iio: accel: adxl345: add FIFO with watermark events

[Jonathan Cameron]

On Wed, 25 Dec 2024 18:13:37 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add a basic setup for FIFO with configurable watermark. Add a handler
> for watermark interrupt events and extend the channel for the
> scan_index needed for the iio channel. The sensor is configurable to use
> a FIFO_BYPASSED mode or a FIFO_STREAM mode. For the FIFO_STREAM mode now
> a watermark can be configured, or disabled by setting 0. Further features
> require a working FIFO setup.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl345.h      |  27 ++-
>  drivers/iio/accel/adxl345_core.c | 305 ++++++++++++++++++++++++++++++-
>  2 files changed, 321 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> index 6f39f16d3..bf9e86cff 100644
> --- a/drivers/iio/accel/adxl345.h
> +++ b/drivers/iio/accel/adxl345.h
> @@ -15,18 +15,32 @@
>  #define ADXL345_REG_OFS_AXIS(index)	(ADXL345_REG_OFSX + (index))
>  #define ADXL345_REG_BW_RATE		0x2C
>  #define ADXL345_REG_POWER_CTL		0x2D
> +#define ADXL345_REG_INT_ENABLE		0x2E
> +#define ADXL345_REG_INT_MAP		0x2F
> +#define ADXL345_REG_INT_SOURCE		0x30
> +#define ADXL345_REG_INT_SOURCE_MSK	0xFF
>  #define ADXL345_REG_DATA_FORMAT		0x31
> -#define ADXL345_REG_DATAX0		0x32
> -#define ADXL345_REG_DATAY0		0x34
> -#define ADXL345_REG_DATAZ0		0x36
> -#define ADXL345_REG_DATA_AXIS(index)	\
> -	(ADXL345_REG_DATAX0 + (index) * sizeof(__le16))
> +#define ADXL345_REG_XYZ_BASE		0x32
> +#define ADXL345_REG_DATA_AXIS(index)				\
> +	(ADXL345_REG_XYZ_BASE + (index) * sizeof(__le16))
>  
> +#define ADXL345_REG_FIFO_CTL		0x38
> +#define ADXL345_REG_FIFO_STATUS	0x39
> +#define ADXL345_REG_FIFO_STATUS_MSK	0x3F
> +
> +#define ADXL345_FIFO_CTL_SAMPLES(x)	FIELD_PREP(GENMASK(4, 0), x)
These need linux/bitfield.h to be included.

However, that got me looking closer at this and it should be done
differently.

Just define the masks in here and put the FIELD_PREP() inline in the
c file.  Same for the various other FIELD_PREP macros in here.

It may seem convenient to wrap all this up here, but in general
I'd rather see that these are simple FIELD_PREP() calls where they
are used inline.

Thanks,

Jonathan

> +/* 0: INT1, 1: INT2 */
> +#define ADXL345_FIFO_CTL_TRIGGER(x)	FIELD_PREP(BIT(5), x)
> +#define ADXL345_FIFO_CTL_MODE(x)	FIELD_PREP(GENMASK(7, 6), x)
> +
> +#define ADXL345_INT_DATA_READY		BIT(7)
> +#define ADXL345_INT_WATERMARK		BIT(1)
> +#define ADXL345_INT_OVERRUN		BIT(0)
>  #define ADXL345_BW_RATE			GENMASK(3, 0)
>  #define ADXL345_BASE_RATE_NANO_HZ	97656250LL
>  
> -#define ADXL345_POWER_CTL_MEASURE	BIT(3)
>  #define ADXL345_POWER_CTL_STANDBY	0x00
> +#define ADXL345_POWER_CTL_MEASURE	BIT(3)
>  
>  #define ADXL345_DATA_FORMAT_RANGE	GENMASK(1, 0)	/* Set the g range */
>  #define ADXL345_DATA_FORMAT_JUSTIFY	BIT(2)	/* Left-justified (MSB) mode */
> @@ -40,6 +54,7 @@
>  #define ADXL345_DATA_FORMAT_16G		3
>  
>  #define ADXL345_DEVID			0xE5
> +#define ADXL345_FIFO_SIZE		32
>  
>  /*
>   * In full-resolution mode, scale factor is maintained at ~4 mg/LSB
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 987a0fe03..738960298 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -15,9 +15,17 @@
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/kfifo_buf.h>
>  
>  #include "adxl345.h"
>  
> +#define ADXL345_FIFO_BYPASS	0
> +#define ADXL345_FIFO_FIFO	1
> +#define ADXL345_FIFO_STREAM	2
> +
> +#define ADXL345_DIRS 3
> +
>  #define ADXL345_INT_NONE		0xff
>  #define ADXL345_INT1			0
>  #define ADXL345_INT2			1
> @@ -28,25 +36,66 @@ struct adxl345_state {
>  	bool fifo_delay; /* delay: delay is needed for SPI */
>  	int irq;
>  	u8 intio;
> +	u8 int_map;
> +	u8 watermark;
> +	u8 fifo_mode;
> +	__le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE + 1] __aligned(IIO_DMA_MINALIGN);
>  };
>  
> -#define ADXL345_CHANNEL(index, axis) {					\
> +#define ADXL345_CHANNEL(index, reg, axis) {					\
>  	.type = IIO_ACCEL,						\
>  	.modified = 1,							\
>  	.channel2 = IIO_MOD_##axis,					\
> -	.address = index,						\
> +	.address = (reg),						\
>  	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
>  		BIT(IIO_CHAN_INFO_CALIBBIAS),				\
>  	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |		\
>  		BIT(IIO_CHAN_INFO_SAMP_FREQ),				\
> +	.scan_index = (index),				\
> +	.scan_type = {					\
> +		.sign = 's',				\
> +		.realbits = 13,				\
> +		.storagebits = 16,			\
> +		.endianness = IIO_LE,			\
> +	},						\
>  }
>  
> +enum adxl345_chans {
> +	chan_x, chan_y, chan_z,
> +};
> +
>  static const struct iio_chan_spec adxl345_channels[] = {
> -	ADXL345_CHANNEL(0, X),
> -	ADXL345_CHANNEL(1, Y),
> -	ADXL345_CHANNEL(2, Z),
> +	ADXL345_CHANNEL(0, chan_x, X),
> +	ADXL345_CHANNEL(1, chan_y, Y),
> +	ADXL345_CHANNEL(2, chan_z, Z),
>  };
>  
> +static const unsigned long adxl345_scan_masks[] = {
> +	BIT(chan_x) | BIT(chan_y) | BIT(chan_z),
> +	0
> +};
> +
> +static int adxl345_set_interrupts(struct adxl345_state *st)
> +{
> +	int ret;
> +	unsigned int int_enable = st->int_map;
> +	unsigned int int_map;
> +
> +	/*
> +	 * Any bits set to 0 in the INT map register send their respective
> +	 * interrupts to the INT1 pin, whereas bits set to 1 send their respective
> +	 * interrupts to the INT2 pin. The intio shall convert this accordingly.
> +	 */
> +	int_map = FIELD_GET(ADXL345_REG_INT_SOURCE_MSK,
> +			    st->intio ? st->int_map : ~st->int_map);
> +
> +	ret = regmap_write(st->regmap, ADXL345_REG_INT_MAP, int_map);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(st->regmap, ADXL345_REG_INT_ENABLE, int_enable);
> +}
> +
>  static int adxl345_read_raw(struct iio_dev *indio_dev,
>  			    struct iio_chan_spec const *chan,
>  			    int *val, int *val2, long mask)
> @@ -132,6 +181,24 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
>  	return -EINVAL;
>  }
>  
> +static int adxl345_set_watermark(struct iio_dev *indio_dev, unsigned int value)
> +{
> +	struct adxl345_state *st = iio_priv(indio_dev);
> +	unsigned int fifo_mask = 0x1F;
> +	int ret;
> +
> +	value = min(value, ADXL345_FIFO_SIZE - 1);
> +
> +	ret = regmap_update_bits(st->regmap, ADXL345_REG_FIFO_CTL, fifo_mask, value);
> +	if (ret)
> +		return ret;
> +
> +	st->watermark = value;
> +	st->int_map |= ADXL345_INT_WATERMARK;
> +
> +	return 0;
> +}
> +
>  static int adxl345_write_raw_get_fmt(struct iio_dev *indio_dev,
>  				     struct iio_chan_spec const *chan,
>  				     long mask)
> @@ -186,11 +253,217 @@ static const struct attribute_group adxl345_attrs_group = {
>  	.attrs = adxl345_attrs,
>  };
>  
> +static int adxl345_set_fifo(struct adxl345_state *st)
> +{
> +	int ret;
> +
> +	/* FIFO should only be configured while in standby mode */
> +	ret = adxl345_set_measure_en(st, false);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, ADXL345_REG_FIFO_CTL,
> +			   ADXL345_FIFO_CTL_SAMPLES(st->watermark) |
> +			   ADXL345_FIFO_CTL_TRIGGER(st->intio) |
> +			   ADXL345_FIFO_CTL_MODE(st->fifo_mode));
> +	if (ret < 0)
> +		return ret;
> +
> +	return adxl345_set_measure_en(st, true);
> +}
> +
> +/**
> + * adxl345_get_samples() - Read number of FIFO entries.
> + * @st: The initialized state instance of this driver.
> + *
> + * The sensor does not support treating any axis individually, or exclude them
> + * from measuring.
> + *
> + * Return: negative error, or value.
> + */
> +static int adxl345_get_samples(struct adxl345_state *st)
> +{
> +	unsigned int regval = 0;
> +	int ret;
> +
> +	ret = regmap_read(st->regmap, ADXL345_REG_FIFO_STATUS, &regval);
> +	if (ret < 0)
> +		return ret;
> +
> +	return FIELD_GET(ADXL345_REG_FIFO_STATUS_MSK, regval);
> +}
> +
> +/**
> + * adxl345_fifo_transfer() - Read samples number of elements.
> + * @st: The instance of the state object of this sensor.
> + * @samples: The number of lines in the FIFO referred to as fifo_entry.
> + *
> + * It is recommended that a multiple-byte read of all registers be performed to
> + * prevent a change in data between reads of sequential registers. That is to
> + * read out the data registers X0, X1, Y0, Y1, Z0, Z1, i.e. 6 bytes at once.
> + *
> + * Return: 0 or error value.
> + */
> +static int adxl345_fifo_transfer(struct adxl345_state *st, int samples)
> +{
> +	size_t count;
> +	int i, ret = 0;
> +
> +	/* count is the 3x the fifo_buf element size, hence 6B */
> +	count = sizeof(st->fifo_buf[0]) * ADXL345_DIRS;
> +	for (i = 0; i < samples; i++) {
> +		/* read 3x 2 byte elements from base address into next fifo_buf position */
> +		ret = regmap_bulk_read(st->regmap, ADXL345_REG_XYZ_BASE,
> +				       st->fifo_buf + (i * count / 2), count);
> +		if (ret < 0)
> +			return ret;
> +
> +		/*
> +		 * To ensure that the FIFO has completely popped, there must be at least 5
> +		 * us between the end of reading the data registers, signified by the
> +		 * transition to register 0x38 from 0x37 or the CS pin going high, and the
> +		 * start of new reads of the FIFO or reading the FIFO_STATUS register. For
> +		 * SPI operation at 1.5 MHz or lower, the register addressing portion of the
> +		 * transmission is sufficient delay to ensure the FIFO has completely
> +		 * popped. It is necessary for SPI operation greater than 1.5 MHz to
> +		 * de-assert the CS pin to ensure a total of 5 us, which is at most 3.4 us
> +		 * at 5 MHz operation.
> +		 */
> +		if (st->fifo_delay && samples > 1)
> +			udelay(3);
> +	}
> +	return ret;
> +}
> +
> +/**
> + * adxl345_fifo_reset() - Empty the FIFO in error condition.
> + * @st: The instance to the state object of the sensor.
> + *
> + * Read all elements of the FIFO. Reading the interrupt source register
> + * resets the sensor.
> + */
> +static void adxl345_fifo_reset(struct adxl345_state *st)
> +{
> +	int regval;
> +	int samples;
> +
> +	adxl345_set_measure_en(st, false);
> +
> +	samples = adxl345_get_samples(st);
> +	if (samples > 0)
> +		adxl345_fifo_transfer(st, samples);
> +
> +	regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, &regval);
> +
> +	adxl345_set_measure_en(st, true);
> +}
> +
> +static int adxl345_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct adxl345_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = adxl345_set_interrupts(st);
> +	if (ret < 0)
> +		return ret;
> +
> +	st->fifo_mode = ADXL345_FIFO_STREAM;
> +	return adxl345_set_fifo(st);
> +}
> +
> +static int adxl345_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct adxl345_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	st->fifo_mode = ADXL345_FIFO_BYPASS;
> +	ret = adxl345_set_fifo(st);
> +	if (ret < 0)
> +		return ret;
> +
> +	st->int_map = 0x00;
> +	return adxl345_set_interrupts(st);
> +}
> +
> +static const struct iio_buffer_setup_ops adxl345_buffer_ops = {
> +	.postenable = adxl345_buffer_postenable,
> +	.predisable = adxl345_buffer_predisable,
> +};
> +
> +static int adxl345_get_status(struct adxl345_state *st)
> +{
> +	int ret;
> +	unsigned int regval;
> +
> +	ret = regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, &regval);
> +	if (ret < 0)
> +		return ret;
> +
> +	return FIELD_GET(ADXL345_REG_INT_SOURCE_MSK, regval);
> +}
> +
> +static int adxl345_fifo_push(struct iio_dev *indio_dev,
> +			     int samples)
> +{
> +	struct adxl345_state *st = iio_priv(indio_dev);
> +	int i, ret;
> +
> +	if (samples <= 0)
> +		return -EINVAL;
> +
> +	ret = adxl345_fifo_transfer(st, samples);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < ADXL345_DIRS * samples; i += ADXL345_DIRS)
> +		iio_push_to_buffers(indio_dev, &st->fifo_buf[i]);
> +
> +	return 0;
> +}
> +
> +/**
> + * adxl345_irq_handler() - Handle irqs of the ADXL345.
> + * @irq: The irq being handled.
> + * @p: The struct iio_device pointer for the device.
> + *
> + * Return: The interrupt was handled.
> + */
> +static irqreturn_t adxl345_irq_handler(int irq, void *p)
> +{
> +	struct iio_dev *indio_dev = p;
> +	struct adxl345_state *st = iio_priv(indio_dev);
> +	int int_stat;
> +	int samples;
> +
> +	int_stat = adxl345_get_status(st);
> +	if (int_stat <= 0)
> +		return IRQ_NONE;
> +
> +	if (int_stat & ADXL345_INT_OVERRUN)
> +		goto err;
> +
> +	if (int_stat & ADXL345_INT_WATERMARK) {
> +		samples = adxl345_get_samples(st);
> +		if (samples < 0)
> +			goto err;
> +
> +		if (adxl345_fifo_push(indio_dev, samples) < 0)
> +			goto err;
> +	}
> +	return IRQ_HANDLED;
> +
> +err:
> +	adxl345_fifo_reset(st);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static const struct iio_info adxl345_info = {
>  	.attrs		= &adxl345_attrs_group,
>  	.read_raw	= adxl345_read_raw,
>  	.write_raw	= adxl345_write_raw,
>  	.write_raw_get_fmt	= adxl345_write_raw_get_fmt,
> +	.hwfifo_set_watermark = adxl345_set_watermark,
>  };
>  
>  /**
> @@ -221,6 +494,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  					 ADXL345_DATA_FORMAT_JUSTIFY |
>  					 ADXL345_DATA_FORMAT_FULL_RES |
>  					 ADXL345_DATA_FORMAT_SELF_TEST);
> +	u8 fifo_ctl;
>  	int ret;
>  
>  	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> @@ -239,6 +513,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->channels = adxl345_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(adxl345_channels);
> +	indio_dev->available_scan_masks = adxl345_scan_masks;
>  
>  	if (setup) {
>  		/* Perform optional initial bus specific configuration */
> @@ -289,6 +564,26 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  			st->intio = ADXL345_INT_NONE;
>  	}
>  
> +	if (st->intio != ADXL345_INT_NONE) {
> +		/* FIFO_STREAM mode is going to be activated later */
> +		ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, &adxl345_buffer_ops);
> +		if (ret)
> +			return ret;
> +
> +		ret = devm_request_threaded_irq(dev, st->irq, NULL,
> +						&adxl345_irq_handler,
> +						IRQF_SHARED | IRQF_ONESHOT,
> +						indio_dev->name, indio_dev);
> +		if (ret)
> +			return ret;
> +	} else {
> +		/* FIFO_BYPASS mode */
> +		fifo_ctl = ADXL345_FIFO_CTL_MODE(ADXL345_FIFO_BYPASS);
> +		ret = regmap_write(st->regmap, ADXL345_REG_FIFO_CTL, fifo_ctl);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	return devm_iio_device_register(dev, indio_dev);
>  }
>  EXPORT_SYMBOL_NS_GPL(adxl345_core_probe, IIO_ADXL345);

Re: [PATCH v8 7/7] iio: accel: adxl345: complete the list of defines

[Jonathan Cameron]

On Wed, 25 Dec 2024 18:13:38 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Having interrupts events and FIFO available allows to evaluate the
> sensor events. Cover the list of interrupt based sensor events. Keep
> them in the header file for readability.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>

One comment inline

Thanks,

Jonathan

>  #define ADXL345_REG_BW_RATE		0x2C
>  #define ADXL345_REG_POWER_CTL		0x2D
>  #define ADXL345_REG_INT_ENABLE		0x2E
> @@ -34,20 +59,40 @@
>  #define ADXL345_FIFO_CTL_MODE(x)	FIELD_PREP(GENMASK(7, 6), x)
>  
>  #define ADXL345_INT_DATA_READY		BIT(7)
> +#define ADXL345_INT_SINGLE_TAP		BIT(6)
> +#define ADXL345_INT_DOUBLE_TAP		BIT(5)
> +#define ADXL345_INT_ACTIVITY		BIT(4)
> +#define ADXL345_INT_INACTIVITY		BIT(3)
> +#define ADXL345_INT_FREE_FALL		BIT(2)
>  #define ADXL345_INT_WATERMARK		BIT(1)
>  #define ADXL345_INT_OVERRUN		BIT(0)
> +
> +#define ADXL345_S_TAP_MSK	ADXL345_INT_SINGLE_TAP
> +#define ADXL345_D_TAP_MSK	ADXL345_INT_DOUBLE_TAP

Why have these?

Re: [PATCH v8 7/7] iio: accel: adxl345: complete the list of defines

[Lothar Rubusch]

On Sat, Dec 28, 2024 at 3:47 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Wed, 25 Dec 2024 18:13:38 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Having interrupts events and FIFO available allows to evaluate the
> > sensor events. Cover the list of interrupt based sensor events. Keep
> > them in the header file for readability.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
>
> One comment inline
>
> Thanks,
>
> Jonathan
>
> >  #define ADXL345_REG_BW_RATE          0x2C
> >  #define ADXL345_REG_POWER_CTL                0x2D
> >  #define ADXL345_REG_INT_ENABLE               0x2E
> > @@ -34,20 +59,40 @@
> >  #define ADXL345_FIFO_CTL_MODE(x)     FIELD_PREP(GENMASK(7, 6), x)
> >
> >  #define ADXL345_INT_DATA_READY               BIT(7)
> > +#define ADXL345_INT_SINGLE_TAP               BIT(6)
> > +#define ADXL345_INT_DOUBLE_TAP               BIT(5)
> > +#define ADXL345_INT_ACTIVITY         BIT(4)
> > +#define ADXL345_INT_INACTIVITY               BIT(3)
> > +#define ADXL345_INT_FREE_FALL                BIT(2)
> >  #define ADXL345_INT_WATERMARK                BIT(1)
> >  #define ADXL345_INT_OVERRUN          BIT(0)
> > +
> > +#define ADXL345_S_TAP_MSK    ADXL345_INT_SINGLE_TAP
> > +#define ADXL345_D_TAP_MSK    ADXL345_INT_DOUBLE_TAP
>
> Why have these?
>

To be honest, I'm unsure to keep this patch within this series now.

Initially, ..... long story short, having FIFO and interrupt handling
now, it is possible to apply and use those ADXL345_INT_ constants. On
the other side, having this more and more separated out of other
patches, it becomes clear there is still some implementation pending
to really use them. I'd like to focus this series rather on FIFO and
interrupt mechanism.

Especially when it comes to the ADXL345_S_TAP_MSK defines, which
probably make even less sense here, if I look at it now.

What would you recommend - Keep it? Drop it? Add just the ADXL345_INT_
fields w/o the _MSKs?

>
>

Re: [PATCH v8 6/7] iio: accel: adxl345: add FIFO with watermark events

[Lothar Rubusch]

On Sat, Dec 28, 2024 at 3:45 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Wed, 25 Dec 2024 18:13:37 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Add a basic setup for FIFO with configurable watermark. Add a handler
> > for watermark interrupt events and extend the channel for the
> > scan_index needed for the iio channel. The sensor is configurable to use
> > a FIFO_BYPASSED mode or a FIFO_STREAM mode. For the FIFO_STREAM mode now
> > a watermark can be configured, or disabled by setting 0. Further features
> > require a working FIFO setup.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > ---
> >  drivers/iio/accel/adxl345.h      |  27 ++-
> >  drivers/iio/accel/adxl345_core.c | 305 ++++++++++++++++++++++++++++++-
> >  2 files changed, 321 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> > index 6f39f16d3..bf9e86cff 100644
> > --- a/drivers/iio/accel/adxl345.h
> > +++ b/drivers/iio/accel/adxl345.h
> > @@ -15,18 +15,32 @@
> >  #define ADXL345_REG_OFS_AXIS(index)  (ADXL345_REG_OFSX + (index))
> >  #define ADXL345_REG_BW_RATE          0x2C
> >  #define ADXL345_REG_POWER_CTL                0x2D
> > +#define ADXL345_REG_INT_ENABLE               0x2E
> > +#define ADXL345_REG_INT_MAP          0x2F
> > +#define ADXL345_REG_INT_SOURCE               0x30
> > +#define ADXL345_REG_INT_SOURCE_MSK   0xFF
> >  #define ADXL345_REG_DATA_FORMAT              0x31
> > -#define ADXL345_REG_DATAX0           0x32
> > -#define ADXL345_REG_DATAY0           0x34
> > -#define ADXL345_REG_DATAZ0           0x36
> > -#define ADXL345_REG_DATA_AXIS(index) \
> > -     (ADXL345_REG_DATAX0 + (index) * sizeof(__le16))
> > +#define ADXL345_REG_XYZ_BASE         0x32
> > +#define ADXL345_REG_DATA_AXIS(index)                         \
> > +     (ADXL345_REG_XYZ_BASE + (index) * sizeof(__le16))
> >
> > +#define ADXL345_REG_FIFO_CTL         0x38
> > +#define ADXL345_REG_FIFO_STATUS      0x39
> > +#define ADXL345_REG_FIFO_STATUS_MSK  0x3F
> > +
> > +#define ADXL345_FIFO_CTL_SAMPLES(x)  FIELD_PREP(GENMASK(4, 0), x)
> These need linux/bitfield.h to be included.
>

Sure, I can do this.

> However, that got me looking closer at this and it should be done
> differently.
>
> Just define the masks in here and put the FIELD_PREP() inline in the
> c file.  Same for the various other FIELD_PREP macros in here.
>
> It may seem convenient to wrap all this up here, but in general
> I'd rather see that these are simple FIELD_PREP() calls where they
> are used inline.
>

Ok, I understand, the masks stay in the adxl345.h and the FIELD_RPEP()
macros better go to where they are used, i.e. the adxl345_core.c.

Thank you for reviewing this. I was a bit unsure about using the
FIELD_PREP() / GENMASK() combinations correctly. The code seems to
work for me, though. For future usage of those macros: Generally my
usage of those macros is correct?

Do I understand correctly, on the long run it would generally be
cleaner, also to migrate the defines to the adxl345_core.c? IMHO,
probably most of the constants in the adxl345.h can be private to
adxl345_core.c, and probably better should.

Best regards,
L

> Thanks,
>
> Jonathan
>
> > +/* 0: INT1, 1: INT2 */
> > +#define ADXL345_FIFO_CTL_TRIGGER(x)  FIELD_PREP(BIT(5), x)
> > +#define ADXL345_FIFO_CTL_MODE(x)     FIELD_PREP(GENMASK(7, 6), x)
> > +
> > +#define ADXL345_INT_DATA_READY               BIT(7)
> > +#define ADXL345_INT_WATERMARK                BIT(1)
> > +#define ADXL345_INT_OVERRUN          BIT(0)
> >  #define ADXL345_BW_RATE                      GENMASK(3, 0)
> >  #define ADXL345_BASE_RATE_NANO_HZ    97656250LL
> >
> > -#define ADXL345_POWER_CTL_MEASURE    BIT(3)
> >  #define ADXL345_POWER_CTL_STANDBY    0x00
> > +#define ADXL345_POWER_CTL_MEASURE    BIT(3)
> >
> >  #define ADXL345_DATA_FORMAT_RANGE    GENMASK(1, 0)   /* Set the g range */
> >  #define ADXL345_DATA_FORMAT_JUSTIFY  BIT(2)  /* Left-justified (MSB) mode */
> > @@ -40,6 +54,7 @@
> >  #define ADXL345_DATA_FORMAT_16G              3
> >
> >  #define ADXL345_DEVID                        0xE5
> > +#define ADXL345_FIFO_SIZE            32
> >
> >  /*
> >   * In full-resolution mode, scale factor is maintained at ~4 mg/LSB
> > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > index 987a0fe03..738960298 100644
> > --- a/drivers/iio/accel/adxl345_core.c
> > +++ b/drivers/iio/accel/adxl345_core.c
> > @@ -15,9 +15,17 @@
> >
> >  #include <linux/iio/iio.h>
> >  #include <linux/iio/sysfs.h>
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/kfifo_buf.h>
> >
> >  #include "adxl345.h"
> >
> > +#define ADXL345_FIFO_BYPASS  0
> > +#define ADXL345_FIFO_FIFO    1
> > +#define ADXL345_FIFO_STREAM  2
> > +
> > +#define ADXL345_DIRS 3
> > +
> >  #define ADXL345_INT_NONE             0xff
> >  #define ADXL345_INT1                 0
> >  #define ADXL345_INT2                 1
> > @@ -28,25 +36,66 @@ struct adxl345_state {
> >       bool fifo_delay; /* delay: delay is needed for SPI */
> >       int irq;
> >       u8 intio;
> > +     u8 int_map;
> > +     u8 watermark;
> > +     u8 fifo_mode;
> > +     __le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE + 1] __aligned(IIO_DMA_MINALIGN);
> >  };
> >
> > -#define ADXL345_CHANNEL(index, axis) {                                       \
> > +#define ADXL345_CHANNEL(index, reg, axis) {                                  \
> >       .type = IIO_ACCEL,                                              \
> >       .modified = 1,                                                  \
> >       .channel2 = IIO_MOD_##axis,                                     \
> > -     .address = index,                                               \
> > +     .address = (reg),                                               \
> >       .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |                  \
> >               BIT(IIO_CHAN_INFO_CALIBBIAS),                           \
> >       .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |          \
> >               BIT(IIO_CHAN_INFO_SAMP_FREQ),                           \
> > +     .scan_index = (index),                          \
> > +     .scan_type = {                                  \
> > +             .sign = 's',                            \
> > +             .realbits = 13,                         \
> > +             .storagebits = 16,                      \
> > +             .endianness = IIO_LE,                   \
> > +     },                                              \
> >  }
> >
> > +enum adxl345_chans {
> > +     chan_x, chan_y, chan_z,
> > +};
> > +
> >  static const struct iio_chan_spec adxl345_channels[] = {
> > -     ADXL345_CHANNEL(0, X),
> > -     ADXL345_CHANNEL(1, Y),
> > -     ADXL345_CHANNEL(2, Z),
> > +     ADXL345_CHANNEL(0, chan_x, X),
> > +     ADXL345_CHANNEL(1, chan_y, Y),
> > +     ADXL345_CHANNEL(2, chan_z, Z),
> >  };
> >
> > +static const unsigned long adxl345_scan_masks[] = {
> > +     BIT(chan_x) | BIT(chan_y) | BIT(chan_z),
> > +     0
> > +};
> > +
> > +static int adxl345_set_interrupts(struct adxl345_state *st)
> > +{
> > +     int ret;
> > +     unsigned int int_enable = st->int_map;
> > +     unsigned int int_map;
> > +
> > +     /*
> > +      * Any bits set to 0 in the INT map register send their respective
> > +      * interrupts to the INT1 pin, whereas bits set to 1 send their respective
> > +      * interrupts to the INT2 pin. The intio shall convert this accordingly.
> > +      */
> > +     int_map = FIELD_GET(ADXL345_REG_INT_SOURCE_MSK,
> > +                         st->intio ? st->int_map : ~st->int_map);
> > +
> > +     ret = regmap_write(st->regmap, ADXL345_REG_INT_MAP, int_map);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return regmap_write(st->regmap, ADXL345_REG_INT_ENABLE, int_enable);
> > +}
> > +
> >  static int adxl345_read_raw(struct iio_dev *indio_dev,
> >                           struct iio_chan_spec const *chan,
> >                           int *val, int *val2, long mask)
> > @@ -132,6 +181,24 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
> >       return -EINVAL;
> >  }
> >
> > +static int adxl345_set_watermark(struct iio_dev *indio_dev, unsigned int value)
> > +{
> > +     struct adxl345_state *st = iio_priv(indio_dev);
> > +     unsigned int fifo_mask = 0x1F;
> > +     int ret;
> > +
> > +     value = min(value, ADXL345_FIFO_SIZE - 1);
> > +
> > +     ret = regmap_update_bits(st->regmap, ADXL345_REG_FIFO_CTL, fifo_mask, value);
> > +     if (ret)
> > +             return ret;
> > +
> > +     st->watermark = value;
> > +     st->int_map |= ADXL345_INT_WATERMARK;
> > +
> > +     return 0;
> > +}
> > +
> >  static int adxl345_write_raw_get_fmt(struct iio_dev *indio_dev,
> >                                    struct iio_chan_spec const *chan,
> >                                    long mask)
> > @@ -186,11 +253,217 @@ static const struct attribute_group adxl345_attrs_group = {
> >       .attrs = adxl345_attrs,
> >  };
> >
> > +static int adxl345_set_fifo(struct adxl345_state *st)
> > +{
> > +     int ret;
> > +
> > +     /* FIFO should only be configured while in standby mode */
> > +     ret = adxl345_set_measure_en(st, false);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     ret = regmap_write(st->regmap, ADXL345_REG_FIFO_CTL,
> > +                        ADXL345_FIFO_CTL_SAMPLES(st->watermark) |
> > +                        ADXL345_FIFO_CTL_TRIGGER(st->intio) |
> > +                        ADXL345_FIFO_CTL_MODE(st->fifo_mode));
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     return adxl345_set_measure_en(st, true);
> > +}
> > +
> > +/**
> > + * adxl345_get_samples() - Read number of FIFO entries.
> > + * @st: The initialized state instance of this driver.
> > + *
> > + * The sensor does not support treating any axis individually, or exclude them
> > + * from measuring.
> > + *
> > + * Return: negative error, or value.
> > + */
> > +static int adxl345_get_samples(struct adxl345_state *st)
> > +{
> > +     unsigned int regval = 0;
> > +     int ret;
> > +
> > +     ret = regmap_read(st->regmap, ADXL345_REG_FIFO_STATUS, &regval);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     return FIELD_GET(ADXL345_REG_FIFO_STATUS_MSK, regval);
> > +}
> > +
> > +/**
> > + * adxl345_fifo_transfer() - Read samples number of elements.
> > + * @st: The instance of the state object of this sensor.
> > + * @samples: The number of lines in the FIFO referred to as fifo_entry.
> > + *
> > + * It is recommended that a multiple-byte read of all registers be performed to
> > + * prevent a change in data between reads of sequential registers. That is to
> > + * read out the data registers X0, X1, Y0, Y1, Z0, Z1, i.e. 6 bytes at once.
> > + *
> > + * Return: 0 or error value.
> > + */
> > +static int adxl345_fifo_transfer(struct adxl345_state *st, int samples)
> > +{
> > +     size_t count;
> > +     int i, ret = 0;
> > +
> > +     /* count is the 3x the fifo_buf element size, hence 6B */
> > +     count = sizeof(st->fifo_buf[0]) * ADXL345_DIRS;
> > +     for (i = 0; i < samples; i++) {
> > +             /* read 3x 2 byte elements from base address into next fifo_buf position */
> > +             ret = regmap_bulk_read(st->regmap, ADXL345_REG_XYZ_BASE,
> > +                                    st->fifo_buf + (i * count / 2), count);
> > +             if (ret < 0)
> > +                     return ret;
> > +
> > +             /*
> > +              * To ensure that the FIFO has completely popped, there must be at least 5
> > +              * us between the end of reading the data registers, signified by the
> > +              * transition to register 0x38 from 0x37 or the CS pin going high, and the
> > +              * start of new reads of the FIFO or reading the FIFO_STATUS register. For
> > +              * SPI operation at 1.5 MHz or lower, the register addressing portion of the
> > +              * transmission is sufficient delay to ensure the FIFO has completely
> > +              * popped. It is necessary for SPI operation greater than 1.5 MHz to
> > +              * de-assert the CS pin to ensure a total of 5 us, which is at most 3.4 us
> > +              * at 5 MHz operation.
> > +              */
> > +             if (st->fifo_delay && samples > 1)
> > +                     udelay(3);
> > +     }
> > +     return ret;
> > +}
> > +
> > +/**
> > + * adxl345_fifo_reset() - Empty the FIFO in error condition.
> > + * @st: The instance to the state object of the sensor.
> > + *
> > + * Read all elements of the FIFO. Reading the interrupt source register
> > + * resets the sensor.
> > + */
> > +static void adxl345_fifo_reset(struct adxl345_state *st)
> > +{
> > +     int regval;
> > +     int samples;
> > +
> > +     adxl345_set_measure_en(st, false);
> > +
> > +     samples = adxl345_get_samples(st);
> > +     if (samples > 0)
> > +             adxl345_fifo_transfer(st, samples);
> > +
> > +     regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, &regval);
> > +
> > +     adxl345_set_measure_en(st, true);
> > +}
> > +
> > +static int adxl345_buffer_postenable(struct iio_dev *indio_dev)
> > +{
> > +     struct adxl345_state *st = iio_priv(indio_dev);
> > +     int ret;
> > +
> > +     ret = adxl345_set_interrupts(st);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     st->fifo_mode = ADXL345_FIFO_STREAM;
> > +     return adxl345_set_fifo(st);
> > +}
> > +
> > +static int adxl345_buffer_predisable(struct iio_dev *indio_dev)
> > +{
> > +     struct adxl345_state *st = iio_priv(indio_dev);
> > +     int ret;
> > +
> > +     st->fifo_mode = ADXL345_FIFO_BYPASS;
> > +     ret = adxl345_set_fifo(st);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     st->int_map = 0x00;
> > +     return adxl345_set_interrupts(st);
> > +}
> > +
> > +static const struct iio_buffer_setup_ops adxl345_buffer_ops = {
> > +     .postenable = adxl345_buffer_postenable,
> > +     .predisable = adxl345_buffer_predisable,
> > +};
> > +
> > +static int adxl345_get_status(struct adxl345_state *st)
> > +{
> > +     int ret;
> > +     unsigned int regval;
> > +
> > +     ret = regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, &regval);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     return FIELD_GET(ADXL345_REG_INT_SOURCE_MSK, regval);
> > +}
> > +
> > +static int adxl345_fifo_push(struct iio_dev *indio_dev,
> > +                          int samples)
> > +{
> > +     struct adxl345_state *st = iio_priv(indio_dev);
> > +     int i, ret;
> > +
> > +     if (samples <= 0)
> > +             return -EINVAL;
> > +
> > +     ret = adxl345_fifo_transfer(st, samples);
> > +     if (ret)
> > +             return ret;
> > +
> > +     for (i = 0; i < ADXL345_DIRS * samples; i += ADXL345_DIRS)
> > +             iio_push_to_buffers(indio_dev, &st->fifo_buf[i]);
> > +
> > +     return 0;
> > +}
> > +
> > +/**
> > + * adxl345_irq_handler() - Handle irqs of the ADXL345.
> > + * @irq: The irq being handled.
> > + * @p: The struct iio_device pointer for the device.
> > + *
> > + * Return: The interrupt was handled.
> > + */
> > +static irqreturn_t adxl345_irq_handler(int irq, void *p)
> > +{
> > +     struct iio_dev *indio_dev = p;
> > +     struct adxl345_state *st = iio_priv(indio_dev);
> > +     int int_stat;
> > +     int samples;
> > +
> > +     int_stat = adxl345_get_status(st);
> > +     if (int_stat <= 0)
> > +             return IRQ_NONE;
> > +
> > +     if (int_stat & ADXL345_INT_OVERRUN)
> > +             goto err;
> > +
> > +     if (int_stat & ADXL345_INT_WATERMARK) {
> > +             samples = adxl345_get_samples(st);
> > +             if (samples < 0)
> > +                     goto err;
> > +
> > +             if (adxl345_fifo_push(indio_dev, samples) < 0)
> > +                     goto err;
> > +     }
> > +     return IRQ_HANDLED;
> > +
> > +err:
> > +     adxl345_fifo_reset(st);
> > +
> > +     return IRQ_HANDLED;
> > +}
> > +
> >  static const struct iio_info adxl345_info = {
> >       .attrs          = &adxl345_attrs_group,
> >       .read_raw       = adxl345_read_raw,
> >       .write_raw      = adxl345_write_raw,
> >       .write_raw_get_fmt      = adxl345_write_raw_get_fmt,
> > +     .hwfifo_set_watermark = adxl345_set_watermark,
> >  };
> >
> >  /**
> > @@ -221,6 +494,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> >                                        ADXL345_DATA_FORMAT_JUSTIFY |
> >                                        ADXL345_DATA_FORMAT_FULL_RES |
> >                                        ADXL345_DATA_FORMAT_SELF_TEST);
> > +     u8 fifo_ctl;
> >       int ret;
> >
> >       indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> > @@ -239,6 +513,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> >       indio_dev->modes = INDIO_DIRECT_MODE;
> >       indio_dev->channels = adxl345_channels;
> >       indio_dev->num_channels = ARRAY_SIZE(adxl345_channels);
> > +     indio_dev->available_scan_masks = adxl345_scan_masks;
> >
> >       if (setup) {
> >               /* Perform optional initial bus specific configuration */
> > @@ -289,6 +564,26 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> >                       st->intio = ADXL345_INT_NONE;
> >       }
> >
> > +     if (st->intio != ADXL345_INT_NONE) {
> > +             /* FIFO_STREAM mode is going to be activated later */
> > +             ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, &adxl345_buffer_ops);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             ret = devm_request_threaded_irq(dev, st->irq, NULL,
> > +                                             &adxl345_irq_handler,
> > +                                             IRQF_SHARED | IRQF_ONESHOT,
> > +                                             indio_dev->name, indio_dev);
> > +             if (ret)
> > +                     return ret;
> > +     } else {
> > +             /* FIFO_BYPASS mode */
> > +             fifo_ctl = ADXL345_FIFO_CTL_MODE(ADXL345_FIFO_BYPASS);
> > +             ret = regmap_write(st->regmap, ADXL345_REG_FIFO_CTL, fifo_ctl);
> > +             if (ret < 0)
> > +                     return ret;
> > +     }
> > +
> >       return devm_iio_device_register(dev, indio_dev);
> >  }
> >  EXPORT_SYMBOL_NS_GPL(adxl345_core_probe, IIO_ADXL345);
>

Re: [PATCH v8 6/7] iio: accel: adxl345: add FIFO with watermark events

[Jonathan Cameron]

On Sat, 28 Dec 2024 17:11:58 +0100
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> On Sat, Dec 28, 2024 at 3:45 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Wed, 25 Dec 2024 18:13:37 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >  
> > > Add a basic setup for FIFO with configurable watermark. Add a handler
> > > for watermark interrupt events and extend the channel for the
> > > scan_index needed for the iio channel. The sensor is configurable to use
> > > a FIFO_BYPASSED mode or a FIFO_STREAM mode. For the FIFO_STREAM mode now
> > > a watermark can be configured, or disabled by setting 0. Further features
> > > require a working FIFO setup.
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > ---
> > >  drivers/iio/accel/adxl345.h      |  27 ++-
> > >  drivers/iio/accel/adxl345_core.c | 305 ++++++++++++++++++++++++++++++-
> > >  2 files changed, 321 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> > > index 6f39f16d3..bf9e86cff 100644
> > > --- a/drivers/iio/accel/adxl345.h
> > > +++ b/drivers/iio/accel/adxl345.h
> > > @@ -15,18 +15,32 @@
> > >  #define ADXL345_REG_OFS_AXIS(index)  (ADXL345_REG_OFSX + (index))
> > >  #define ADXL345_REG_BW_RATE          0x2C
> > >  #define ADXL345_REG_POWER_CTL                0x2D
> > > +#define ADXL345_REG_INT_ENABLE               0x2E
> > > +#define ADXL345_REG_INT_MAP          0x2F
> > > +#define ADXL345_REG_INT_SOURCE               0x30
> > > +#define ADXL345_REG_INT_SOURCE_MSK   0xFF
> > >  #define ADXL345_REG_DATA_FORMAT              0x31
> > > -#define ADXL345_REG_DATAX0           0x32
> > > -#define ADXL345_REG_DATAY0           0x34
> > > -#define ADXL345_REG_DATAZ0           0x36
> > > -#define ADXL345_REG_DATA_AXIS(index) \
> > > -     (ADXL345_REG_DATAX0 + (index) * sizeof(__le16))
> > > +#define ADXL345_REG_XYZ_BASE         0x32
> > > +#define ADXL345_REG_DATA_AXIS(index)                         \
> > > +     (ADXL345_REG_XYZ_BASE + (index) * sizeof(__le16))
> > >
> > > +#define ADXL345_REG_FIFO_CTL         0x38
> > > +#define ADXL345_REG_FIFO_STATUS      0x39
> > > +#define ADXL345_REG_FIFO_STATUS_MSK  0x3F
> > > +
> > > +#define ADXL345_FIFO_CTL_SAMPLES(x)  FIELD_PREP(GENMASK(4, 0), x)  
> > These need linux/bitfield.h to be included.
> >  
> 
> Sure, I can do this.
> 
> > However, that got me looking closer at this and it should be done
> > differently.
> >
> > Just define the masks in here and put the FIELD_PREP() inline in the
> > c file.  Same for the various other FIELD_PREP macros in here.
> >
> > It may seem convenient to wrap all this up here, but in general
> > I'd rather see that these are simple FIELD_PREP() calls where they
> > are used inline.
> >  
> 
> Ok, I understand, the masks stay in the adxl345.h and the FIELD_RPEP()
> macros better go to where they are used, i.e. the adxl345_core.c.
> 
> Thank you for reviewing this. I was a bit unsure about using the
> FIELD_PREP() / GENMASK() combinations correctly. The code seems to
> work for me, though. For future usage of those macros: Generally my
> usage of those macros is correct?

Yes, the usage is fine otherwise.

> 
> Do I understand correctly, on the long run it would generally be
> cleaner, also to migrate the defines to the adxl345_core.c? IMHO,
> probably most of the constants in the adxl345.h can be private to
> adxl345_core.c, and probably better should.

Exactly.

Thanks,

Jonathan

> 
> Best regards,
> L

Re: [PATCH v8 7/7] iio: accel: adxl345: complete the list of defines

[Jonathan Cameron]

On Sat, 28 Dec 2024 16:48:11 +0100
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> On Sat, Dec 28, 2024 at 3:47 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Wed, 25 Dec 2024 18:13:38 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >  
> > > Having interrupts events and FIFO available allows to evaluate the
> > > sensor events. Cover the list of interrupt based sensor events. Keep
> > > them in the header file for readability.
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>  
> >
> > One comment inline
> >
> > Thanks,
> >
> > Jonathan
> >  
> > >  #define ADXL345_REG_BW_RATE          0x2C
> > >  #define ADXL345_REG_POWER_CTL                0x2D
> > >  #define ADXL345_REG_INT_ENABLE               0x2E
> > > @@ -34,20 +59,40 @@
> > >  #define ADXL345_FIFO_CTL_MODE(x)     FIELD_PREP(GENMASK(7, 6), x)
> > >
> > >  #define ADXL345_INT_DATA_READY               BIT(7)
> > > +#define ADXL345_INT_SINGLE_TAP               BIT(6)
> > > +#define ADXL345_INT_DOUBLE_TAP               BIT(5)
> > > +#define ADXL345_INT_ACTIVITY         BIT(4)
> > > +#define ADXL345_INT_INACTIVITY               BIT(3)
> > > +#define ADXL345_INT_FREE_FALL                BIT(2)
> > >  #define ADXL345_INT_WATERMARK                BIT(1)
> > >  #define ADXL345_INT_OVERRUN          BIT(0)
> > > +
> > > +#define ADXL345_S_TAP_MSK    ADXL345_INT_SINGLE_TAP
> > > +#define ADXL345_D_TAP_MSK    ADXL345_INT_DOUBLE_TAP  
> >
> > Why have these?
> >  
> 
> To be honest, I'm unsure to keep this patch within this series now.
> 
> Initially, ..... long story short, having FIFO and interrupt handling
> now, it is possible to apply and use those ADXL345_INT_ constants. On
> the other side, having this more and more separated out of other
> patches, it becomes clear there is still some implementation pending
> to really use them. I'd like to focus this series rather on FIFO and
> interrupt mechanism.
> 
> Especially when it comes to the ADXL345_S_TAP_MSK defines, which
> probably make even less sense here, if I look at it now.
> 
> What would you recommend - Keep it? Drop it? Add just the ADXL345_INT_
> fields w/o the _MSKs?

I don't mind that much either way so I'll go with what ever you did in
v9 (not looked yet :)

Jonathan

> 
> >
> >