[PATCH v5 00/10] iio: accel: adxl345: add FIFO operating with IRQ triggered watermark events

[PATCH v5 00/10] 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>
---
Although I tried to implement most of the requested changes, now the code
is simplified and clearer, I still encounter some issues.

1) Unsure if my way reading out the FIFO elements with a regmap_noinc_read()
   is supposed to be like that. TBH, isn't there a better way, say, to
   configure the channel correctly and this is handled by the iio API
   internally? As I understood from v2 there might be a way using
   available data, also in iio_info I see now as buffer attributes an
   available data field. Where can I find information how to use it or
   did I get this wrong?

2) Overrun handling: I'm trying to reset the FIFO and registers. Unsure,
   if this is the correct dealing here.

3) I can see the IRQs coming in, and with a `watch -n 0.1 iio_info` I can
   see the correct fields changing. I tried the follwoing down below,
   but the iio_readdev shows me the following result. I don't quite
   understand if I still have an issue here, or if this is a calibration
   thing?

# iio_info 
Library version: 0.23 (git tag: v0.23)
Compiled with backends: local xml ip usb
IIO context created with local backend.
Backend version: 0.23 (git tag: v0.23)
Backend description string: Linux dut1138 6.6.21-lothar02 #3 SMP PREEMPT Wed Nov  6 21:21:14 UTC 2024 aarch64
IIO context has 2 attributes:
	local,kernel: 6.6.21-lothar02
	uri: local:
IIO context has 1 devices:
	iio:device0: adxl345 (buffer capable)
		3 channels found:
			accel_x:  (input, index: 0, format: le:s13/16>>0)
			4 channel-specific attributes found:
				attr  0: calibbias value: 0
				attr  1: raw value: -14                          <--- CHANGES
				attr  2: sampling_frequency value: 100.000000000
				attr  3: scale value: 0.038300
			accel_y:  (input, index: 1, format: le:s13/16>>0)
			4 channel-specific attributes found:
				attr  0: calibbias value: 0
				attr  1: raw value: 6                            <--- CHANGES
				attr  2: sampling_frequency value: 100.000000000
				attr  3: scale value: 0.038300
			accel_z:  (input, index: 2, format: le:s13/16>>0)
			4 channel-specific attributes found:
				attr  0: calibbias value: 0
				attr  1: raw value: 247                          <--- CHANGES
				attr  2: sampling_frequency value: 100.000000000
				attr  3: scale value: 0.038300
		2 device-specific attributes found:
				attr  0: sampling_frequency_available value: 0.09765625 0.1953125 0.390625 0.78125 1.5625 3.125 6.25 12.5 25 50 100 200 400 800 1600 3200
				attr  1: waiting_for_supplier value: 0
		3 buffer-specific attributes found:
				attr  0: data_available value: 13
				attr  1: direction value: in
				attr  2: watermark value: 15
		No trigger on this device

  Above I marked what keeps changing with "CHANGES", that's what I expect. Then with readdev
  I obtain the following result.

# iio_attr -c adxl345
dev 'adxl345', channel 'accel_x' (input, index: 0, format: le:s13/16>>0), found 4 channel-specific attributes
dev 'adxl345', channel 'accel_y' (input, index: 1, format: le:s13/16>>0), found 4 channel-specific attributes
dev 'adxl345', channel 'accel_z' (input, index: 2, format: le:s13/16>>0), found 4 channel-specific attributes
# echo 1 > ./scan_elements/in_accel_x_en
# echo 1 > ./scan_elements/in_accel_y_en
# echo 1 > ./scan_elements/in_accel_z_en
# echo 32 > ./buffer0/length
# echo 15 > ./buffer0/watermark
# echo 1 > ./buffer0/enable
# iio_readdev -b 16 -s 21 adxl345 > samples.dat
# hexdump -d ./samples.dat 
0000000   65523   00006   00248   65523   00005   00235   65522   00006
0000010   00248   65522   00006   00248   65521   00005   00247   65522
0000020   00007   00249   65523   00005   00249   65521   00006   00248
0000030   65522   00006   00248   65522   00006   00250   65522   00006
0000040   00249   65522   00005   00248   65523   00005   00248   65521
0000050   00007   00248   65521   00006   00250   65522   00005   00248
0000060   65521   00006   00248   65522   00007   00247   65522   00006
0000070   00248   65522   00006   00248   65521   00004   00250        
000007e

  Am I doing this actually correctly?

---
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 (10):
  iio: accel: adxl345: refrase comment on probe
  iio: accel: adxl345: rename variable data to st
  iio: accel: adxl345: measure right-justified
  iio: accel: adxl345: add function to switch measuring mode
  iio: accel: adxl345: complete list of defines
  dt-bindings: iio: accel: add interrupt-names
  iio: accel: adxl345: introduce interrupt handling
  iio: accel: adxl345: initialize FIFO delay value for SPI
  iio: accel: adxl345: prepare channel for scan_index
  iio: accel: adxl345: add FIFO with watermark events

 .../bindings/iio/accel/adi,adxl345.yaml       |   7 +
 drivers/iio/accel/adxl345.h                   |  90 +++-
 drivers/iio/accel/adxl345_core.c              | 427 ++++++++++++++++--
 drivers/iio/accel/adxl345_i2c.c               |   2 +-
 drivers/iio/accel/adxl345_spi.c               |   7 +-
 5 files changed, 478 insertions(+), 55 deletions(-)

-- 
2.39.2

[PATCH v5 01/10] iio: accel: adxl345: refrase comment on probe

[Lothar Rubusch]

Refrase comment on the probe function, avoid naming different hardware.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 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 006ce66c0aa..eb3ce4434a5 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -169,8 +169,7 @@ static void adxl345_powerdown(void *regmap)
 }
 
 /**
- * adxl345_core_probe() - probe and setup for the adxl345 accelerometer,
- *                        also covers the adlx375 accelerometer
+ * adxl345_core_probe() - Probe and setup for the accelerometer.
  * @dev:	Driver model representation of the device
  * @regmap:	Regmap instance for the device
  * @setup:	Setup routine to be executed right before the standard device
-- 
2.39.2

[PATCH v5 02/10] iio: accel: adxl345: rename variable data to st

[Lothar Rubusch]

Rename the locally used variable data to st. The st refers to "state",
representing the internal state of the driver object. Further it
prepares the usage of an internal data pointer needed for the
implementation of the sensor features.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index eb3ce4434a5..88df9547bd6 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -17,7 +17,7 @@
 
 #include "adxl345.h"
 
-struct adxl345_data {
+struct adxl345_state {
 	const struct adxl345_chip_info *info;
 	struct regmap *regmap;
 };
@@ -43,7 +43,7 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan,
 			    int *val, int *val2, long mask)
 {
-	struct adxl345_data *data = iio_priv(indio_dev);
+	struct adxl345_state *st = iio_priv(indio_dev);
 	__le16 accel;
 	long long samp_freq_nhz;
 	unsigned int regval;
@@ -56,7 +56,7 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
 		 * ADXL345_REG_DATA(X0/Y0/Z0) contain the least significant byte
 		 * and ADXL345_REG_DATA(X0/Y0/Z0) + 1 the most significant byte
 		 */
-		ret = regmap_bulk_read(data->regmap,
+		ret = regmap_bulk_read(st->regmap,
 				       ADXL345_REG_DATA_AXIS(chan->address),
 				       &accel, sizeof(accel));
 		if (ret < 0)
@@ -66,10 +66,10 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
 		*val = 0;
-		*val2 = data->info->uscale;
+		*val2 = st->info->uscale;
 		return IIO_VAL_INT_PLUS_MICRO;
 	case IIO_CHAN_INFO_CALIBBIAS:
-		ret = regmap_read(data->regmap,
+		ret = regmap_read(st->regmap,
 				  ADXL345_REG_OFS_AXIS(chan->address), &regval);
 		if (ret < 0)
 			return ret;
@@ -81,7 +81,7 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
 
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SAMP_FREQ:
-		ret = regmap_read(data->regmap, ADXL345_REG_BW_RATE, &regval);
+		ret = regmap_read(st->regmap, ADXL345_REG_BW_RATE, &regval);
 		if (ret < 0)
 			return ret;
 
@@ -99,7 +99,7 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
 			     struct iio_chan_spec const *chan,
 			     int val, int val2, long mask)
 {
-	struct adxl345_data *data = iio_priv(indio_dev);
+	struct adxl345_state *st = iio_priv(indio_dev);
 	s64 n;
 
 	switch (mask) {
@@ -108,14 +108,14 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
 		 * 8-bit resolution at +/- 2g, that is 4x accel data scale
 		 * factor
 		 */
-		return regmap_write(data->regmap,
+		return regmap_write(st->regmap,
 				    ADXL345_REG_OFS_AXIS(chan->address),
 				    val / 4);
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		n = div_s64(val * NANOHZ_PER_HZ + val2,
 			    ADXL345_BASE_RATE_NANO_HZ);
 
-		return regmap_update_bits(data->regmap, ADXL345_REG_BW_RATE,
+		return regmap_update_bits(st->regmap, ADXL345_REG_BW_RATE,
 					  ADXL345_BW_RATE,
 					  clamp_val(ilog2(n), 0,
 						    ADXL345_BW_RATE));
@@ -180,7 +180,7 @@ static void adxl345_powerdown(void *regmap)
 int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 		       int (*setup)(struct device*, struct regmap*))
 {
-	struct adxl345_data *data;
+	struct adxl345_state *st;
 	struct iio_dev *indio_dev;
 	u32 regval;
 	unsigned int data_format_mask = (ADXL345_DATA_FORMAT_RANGE |
@@ -189,17 +189,17 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 					 ADXL345_DATA_FORMAT_SELF_TEST);
 	int ret;
 
-	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
 	if (!indio_dev)
 		return -ENOMEM;
 
-	data = iio_priv(indio_dev);
-	data->regmap = regmap;
-	data->info = device_get_match_data(dev);
-	if (!data->info)
+	st = iio_priv(indio_dev);
+	st->regmap = regmap;
+	st->info = device_get_match_data(dev);
+	if (!st->info)
 		return -ENODEV;
 
-	indio_dev->name = data->info->name;
+	indio_dev->name = st->info->name;
 	indio_dev->info = &adxl345_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->channels = adxl345_channels;
@@ -207,12 +207,12 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 
 	if (setup) {
 		/* Perform optional initial bus specific configuration */
-		ret = setup(dev, data->regmap);
+		ret = setup(dev, st->regmap);
 		if (ret)
 			return ret;
 
 		/* Enable full-resolution mode */
-		ret = regmap_update_bits(data->regmap, ADXL345_REG_DATA_FORMAT,
+		ret = regmap_update_bits(st->regmap, ADXL345_REG_DATA_FORMAT,
 					 data_format_mask,
 					 ADXL345_DATA_FORMAT_FULL_RES);
 		if (ret)
@@ -221,14 +221,14 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 
 	} else {
 		/* Enable full-resolution mode (init all data_format bits) */
-		ret = regmap_write(data->regmap, ADXL345_REG_DATA_FORMAT,
+		ret = regmap_write(st->regmap, ADXL345_REG_DATA_FORMAT,
 				   ADXL345_DATA_FORMAT_FULL_RES);
 		if (ret)
 			return dev_err_probe(dev, ret,
 					     "Failed to set data range\n");
 	}
 
-	ret = regmap_read(data->regmap, ADXL345_REG_DEVID, &regval);
+	ret = regmap_read(st->regmap, ADXL345_REG_DEVID, &regval);
 	if (ret < 0)
 		return dev_err_probe(dev, ret, "Error reading device ID\n");
 
@@ -237,11 +237,11 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 				     regval, ADXL345_DEVID);
 
 	/* Enable measurement mode */
-	ret = adxl345_powerup(data->regmap);
+	ret = adxl345_powerup(st->regmap);
 	if (ret < 0)
 		return dev_err_probe(dev, ret, "Failed to enable measurement mode\n");
 
-	ret = devm_add_action_or_reset(dev, adxl345_powerdown, data->regmap);
+	ret = devm_add_action_or_reset(dev, adxl345_powerdown, st->regmap);
 	if (ret < 0)
 		return ret;
 
-- 
2.39.2

[PATCH v5 03/10] iio: accel: adxl345: measure right-justified

[Lothar Rubusch]

Make measurements right-justified, since it is the default for the
driver and sensor. By not setting the ADXL345_DATA_FORMAT_JUSTIFY bit,
the data becomes right-judstified. This was the original setting, there
is no reason to change it to left-justified, where right-justified
simplifies working on the registers.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 88df9547bd6..98ff37271f1 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -184,7 +184,6 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 	struct iio_dev *indio_dev;
 	u32 regval;
 	unsigned int data_format_mask = (ADXL345_DATA_FORMAT_RANGE |
-					 ADXL345_DATA_FORMAT_JUSTIFY |
 					 ADXL345_DATA_FORMAT_FULL_RES |
 					 ADXL345_DATA_FORMAT_SELF_TEST);
 	int ret;
-- 
2.39.2

[PATCH v5 04/10] 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>
---
 drivers/iio/accel/adxl345_core.c | 44 ++++++++++++++++++++++----------
 1 file changed, 31 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 98ff37271f1..1d020b0d79c 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -138,6 +138,34 @@ static int adxl345_write_raw_get_fmt(struct iio_dev *indio_dev,
 	}
 }
 
+/**
+ * adxl345_set_measure_en() - Enable and disable measuring.
+ *
+ * @st: The device data.
+ * @en: Enable measurements, else standby mode.
+ *
+ * For lowest power operation, standby mode can be used. In standby mode,
+ * current consumption is supposed to be reduced to 0.1uA (typical). In this
+ * mode no measurements are made. Placing the device into standby mode
+ * preserves the contents of FIFO.
+ *
+ * Return: Returns 0 if successful, or a negative error value.
+ */
+static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
+{
+	unsigned int val = 0;
+
+	val = (en) ? ADXL345_POWER_CTL_MEASURE : ADXL345_POWER_CTL_STANDBY;
+	return regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
+}
+
+static void adxl345_powerdown(void *ptr)
+{
+	struct adxl345_state *st = ptr;
+
+	adxl345_set_measure_en(st, false);
+}
+
 static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
 "0.09765625 0.1953125 0.390625 0.78125 1.5625 3.125 6.25 12.5 25 50 100 200 400 800 1600 3200"
 );
@@ -158,16 +186,6 @@ static const struct iio_info adxl345_info = {
 	.write_raw_get_fmt	= adxl345_write_raw_get_fmt,
 };
 
-static int adxl345_powerup(void *regmap)
-{
-	return regmap_write(regmap, ADXL345_REG_POWER_CTL, ADXL345_POWER_CTL_MEASURE);
-}
-
-static void adxl345_powerdown(void *regmap)
-{
-	regmap_write(regmap, ADXL345_REG_POWER_CTL, ADXL345_POWER_CTL_STANDBY);
-}
-
 /**
  * adxl345_core_probe() - Probe and setup for the accelerometer.
  * @dev:	Driver model representation of the device
@@ -236,13 +254,13 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 				     regval, ADXL345_DEVID);
 
 	/* Enable measurement mode */
-	ret = adxl345_powerup(st->regmap);
+	ret = adxl345_set_measure_en(st, true);
 	if (ret < 0)
 		return dev_err_probe(dev, ret, "Failed to enable measurement mode\n");
 
-	ret = devm_add_action_or_reset(dev, adxl345_powerdown, st->regmap);
+	ret = devm_add_action_or_reset(dev, adxl345_powerdown, st);
 	if (ret < 0)
-		return ret;
+		return dev_err_probe(dev, ret, "Failed to add action or reset\n");
 
 	return devm_iio_device_register(dev, indio_dev);
 }
-- 
2.39.2

[PATCH v5 05/10] iio: accel: adxl345: complete list of defines

[Lothar Rubusch]

Extend the list of constants. Keep them the header file for readability.
The defines allow the implementation of events like watermark, single
tap, double tap, freefall, etc.

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

diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
index 3d5c8719db3..ed81d5cf445 100644
--- a/drivers/iio/accel/adxl345.h
+++ b/drivers/iio/accel/adxl345.h
@@ -9,37 +9,102 @@
 #define _ADXL345_H_
 
 #define ADXL345_REG_DEVID		0x00
+#define ADXL345_REG_THRESH_TAP	0x1D
 #define ADXL345_REG_OFSX		0x1E
 #define ADXL345_REG_OFSY		0x1F
 #define ADXL345_REG_OFSZ		0x20
-#define ADXL345_REG_OFS_AXIS(index)	(ADXL345_REG_OFSX + (index))
+/* Tap duration */
+#define ADXL345_REG_DUR		0x21
+/* Tap latency */
+#define ADXL345_REG_LATENT		0x22
+/* Tap window */
+#define ADXL345_REG_WINDOW		0x23
+/* Activity threshold */
+#define ADXL345_REG_THRESH_ACT		0x24
+/* Inactivity threshold */
+#define ADXL345_REG_THRESH_INACT	0x25
+/* Inactivity time */
+#define ADXL345_REG_TIME_INACT	0x26
+/* Axis enable control for activity and inactivity detection */
+#define ADXL345_REG_ACT_INACT_CTRL	0x27
+/* Free-fall threshold */
+#define ADXL345_REG_THRESH_FF		0x28
+/* Free-fall time */
+#define ADXL345_REG_TIME_FF		0x29
+/* Axis control for single tap or double tap */
+#define ADXL345_REG_TAP_AXIS		0x2A
+/* Source of single tap or double tap */
+#define ADXL345_REG_ACT_TAP_STATUS	0x2B
+/* Data rate and power mode control */
 #define ADXL345_REG_BW_RATE		0x2C
 #define ADXL345_REG_POWER_CTL		0x2D
+#define ADXL345_REG_INT_ENABLE		0x2E
+#define ADXL345_REG_INT_MAP		0x2F
+#define ADXL345_REG_INT_SOURCE		0x30
 #define ADXL345_REG_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_DEVID			0xE5
+
+#define ADXL345_FIFO_CTL_SAMLPES(x)	(0x1f & (x))
+#define ADXL345_FIFO_CTL_TRIGGER(x)	(0x20 & ((x) << 5)) /* 0: INT1, 1: INT2 */
+#define ADXL345_FIFO_CTL_MODE(x)	(0xc0 & ((x) << 6))
 
+#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
+
+#define ADXL345_INT1			0
+#define ADXL345_INT2			1
+
+/*
+ * 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_MEASURE	BIT(3)
 #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_IS_LEFT_JUSTIFIED	BIT(2)
 #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 */
-
+#define ADXL345_DATA_FORMAT_SPI_3WIRE	BIT(6)
+#define ADXL345_DATA_FORMAT_SELF_TEST	BIT(7)
 #define ADXL345_DATA_FORMAT_2G		0
 #define ADXL345_DATA_FORMAT_4G		1
 #define ADXL345_DATA_FORMAT_8G		2
 #define ADXL345_DATA_FORMAT_16G		3
 
-#define ADXL345_DEVID			0xE5
+#define ADXL345_REG_OFS_AXIS(index)	(ADXL345_REG_OFSX + (index))
+
+/*
+ * FIFO stores a maximum of 32 entries, which equates to a maximum of 33 entries
+ * available at any given time because an additional entry is available at the
+ * output filter of the device.
+ *
+ * (see datasheet FIFO_STATUS description on "Entries Bits")
+ */
+#define ADXL345_FIFO_SIZE  33
 
 /*
  * In full-resolution mode, scale factor is maintained at ~4 mg/LSB
-- 
2.39.2

[PATCH v5 06/10] dt-bindings: iio: accel: add interrupt-names

[Lothar Rubusch]

Add interrupt-names INT1 and INT2 for the two interrupt lines of the
sensor. Only one line will be connected for incoming events. The driver
needs to be configured accordingly. If no interrupt line is set up, the
sensor will still measure, but no events are possible.

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

diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
index 280ed479ef5..67e2c029a6c 100644
--- a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
+++ b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
@@ -37,6 +37,11 @@ properties:
   interrupts:
     maxItems: 1
 
+  interrupt-names:
+    description: Use either INT1 or INT2 for events, or ignore events.
+    items:
+      - enum: [INT1, INT2]
+
 required:
   - compatible
   - reg
@@ -61,6 +66,7 @@ examples:
             reg = <0x2a>;
             interrupt-parent = <&gpio0>;
             interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
+            interrupt-names = "INT1";
         };
     };
   - |
@@ -79,5 +85,6 @@ examples:
             spi-cpha;
             interrupt-parent = <&gpio0>;
             interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
+            interrupt-names = "INT2";
         };
     };
-- 
2.39.2

[PATCH v5 07/10] 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 | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 1d020b0d79c..e0a8b32239f 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -11,6 +11,7 @@
 #include <linux/property.h>
 #include <linux/regmap.h>
 #include <linux/units.h>
+#include <linux/interrupt.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
@@ -18,8 +19,10 @@
 #include "adxl345.h"
 
 struct adxl345_state {
+	int irq;
 	const struct adxl345_chip_info *info;
 	struct regmap *regmap;
+	u8 intio;
 };
 
 #define ADXL345_CHANNEL(index, axis) {					\
@@ -212,6 +215,17 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 
 	st = iio_priv(indio_dev);
 	st->regmap = regmap;
+
+	st->intio = -1;
+	st->irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT1");
+	if (st->irq > 0)
+		st->intio = ADXL345_INT1;
+	else {
+		st->irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT2");
+		if (st->irq > 0)
+			st->intio = ADXL345_INT2;
+	}
+
 	st->info = device_get_match_data(dev);
 	if (!st->info)
 		return -ENODEV;
-- 
2.39.2

[PATCH v5 08/10] 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 | 12 ++++++++++++
 drivers/iio/accel/adxl345_i2c.c  |  2 +-
 drivers/iio/accel/adxl345_spi.c  |  7 +++++--
 4 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
index ed81d5cf445..c07709350d3 100644
--- a/drivers/iio/accel/adxl345.h
+++ b/drivers/iio/accel/adxl345.h
@@ -127,6 +127,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 e0a8b32239f..0696e908bdf 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -22,6 +22,7 @@ struct adxl345_state {
 	int irq;
 	const struct adxl345_chip_info *info;
 	struct regmap *regmap;
+	bool fifo_delay; /* delay: delay is needed for SPI */
 	u8 intio;
 };
 
@@ -193,12 +194,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;
@@ -230,6 +240,8 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 	if (!st->info)
 		return -ENODEV;
 
+	st->fifo_delay = fifo_delay_default;
+
 	indio_dev->name = st->info->name;
 	indio_dev->info = &adxl345_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
diff --git a/drivers/iio/accel/adxl345_i2c.c b/drivers/iio/accel/adxl345_i2c.c
index 4065b8f7c8a..28d997c5860 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 61fd9a6f5fc..9829d5d3d43 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.2

[PATCH v5 09/10] iio: accel: adxl345: prepare channel for scan_index

[Lothar Rubusch]

Add separate fields for register and index to the channel definition.
The scan_index is set up with the kfifo in the follow up patches.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 0696e908bdf..3067a70c54e 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -26,21 +26,26 @@ struct adxl345_state {
 	u8 intio;
 };
 
-#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),				\
 }
 
+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 int adxl345_read_raw(struct iio_dev *indio_dev,
-- 
2.39.2

[PATCH v5 10/10] 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_core.c | 300 +++++++++++++++++++++++++++++++
 1 file changed, 300 insertions(+)

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 3067a70c54e..58ed82d66dc 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -15,15 +15,28 @@
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/events.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
+
 struct adxl345_state {
 	int irq;
 	const struct adxl345_chip_info *info;
 	struct regmap *regmap;
+	__le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE];
 	bool fifo_delay; /* delay: delay is needed for SPI */
 	u8 intio;
+	u8 int_map;
+	u8 watermark;
+	u8 fifo_mode;
 };
 
 #define ADXL345_CHANNEL(index, reg, axis) {					\
@@ -36,6 +49,13 @@ struct adxl345_state {
 	.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,			\
+		.shift = 0,				\
+		.endianness = IIO_LE,			\
+	},						\
 }
 
 enum adxl345_chans {
@@ -48,6 +68,25 @@ static const struct iio_chan_spec adxl345_channels[] = {
 	ADXL345_CHANNEL(2, chan_z, Z),
 };
 
+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 = 0xFF & (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)
@@ -133,6 +172,31 @@ 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;
+
+	if (value == 0) {
+		st->int_map &= ~ADXL345_INT_WATERMARK;
+		return 0;
+	}
+
+	if (value > ADXL345_FIFO_SIZE)
+		value = ADXL345_FIFO_SIZE;
+
+	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)
@@ -188,11 +252,224 @@ static const struct attribute_group adxl345_attrs_group = {
 	.attrs = adxl345_attrs,
 };
 
+static int adxl345_set_fifo(struct adxl345_state *st)
+{
+	u8 fifo_ctl;
+	int ret;
+
+	/* FIFO should only be configured while in standby mode */
+	ret = adxl345_set_measure_en(st, false);
+	if (ret < 0)
+		return ret;
+
+	fifo_ctl = ADXL345_FIFO_CTL_SAMLPES(st->watermark) |
+		ADXL345_FIFO_CTL_TRIGGER(st->intio) |
+		ADXL345_FIFO_CTL_MODE(st->fifo_mode);
+
+	ret = regmap_write(st->regmap, ADXL345_REG_FIFO_CTL, fifo_ctl);
+	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 0x3f & 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,
+ * a fifo_entry has 3 elements for X, Y and Z direction of 2 bytes each.
+ *
+ * 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 at once.
+ *
+ * Return: 0 or error value.
+ */
+static int adxl345_fifo_transfer(struct adxl345_state *st, int samples)
+{
+	size_t count;
+	int i, ret;
+
+	count = sizeof(st->fifo_buf[0]) * ADXL345_DIRS;
+	for (i = 0; i < samples; i++) {
+		ret = regmap_noinc_read(st->regmap, ADXL345_REG_XYZ_BASE,
+				st->fifo_buf + (i * count / 2), count);
+		if (ret < 0)
+			return ret;
+	}
+	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->int_map = 0x00;
+
+	ret = adxl345_set_interrupts(st);
+	if (ret < 0)
+		return ret;
+
+	st->fifo_mode = ADXL345_FIFO_BYPASS;
+	return adxl345_set_fifo(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 (0xff & 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) {
+		/*
+		 * 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);
+
+		iio_push_to_buffers(indio_dev, &st->fifo_buf[i]);
+	}
+
+	return 0;
+}
+
+/**
+ * adxl345_event_handler() - Handle events 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_event_handler(int irq, void *p)
+{
+	struct iio_dev *indio_dev = p;
+	struct adxl345_state *st = iio_priv(indio_dev);
+	u8 int_stat;
+	int samples;
+
+	int_stat = adxl345_get_status(st);
+	if (int_stat < 0)
+		return IRQ_NONE;
+
+	if (int_stat == 0x0)
+		goto err;
+
+	if (int_stat & ADXL345_INT_OVERRUN)
+		goto err;
+
+	if (int_stat & (ADXL345_INT_DATA_READY | 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,
 };
 
 /**
@@ -222,6 +499,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 	unsigned int data_format_mask = (ADXL345_DATA_FORMAT_RANGE |
 					 ADXL345_DATA_FORMAT_FULL_RES |
 					 ADXL345_DATA_FORMAT_SELF_TEST);
+	u8 fifo_ctl;
 	int ret;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
@@ -293,6 +571,28 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 	if (ret < 0)
 		return dev_err_probe(dev, ret, "Failed to add action or reset\n");
 
+	if (st->irq > 0) {
+		dev_dbg(dev, "initialize for FIFO_STREAM mode\n");
+
+		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_event_handler,
+				IRQF_SHARED | IRQF_ONESHOT,
+				indio_dev->name, indio_dev);
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to setup triggered buffer\n");
+
+	} else {
+		dev_dbg(dev, "initialize for FIFO_BYPASS mode (fallback)\n");
+
+		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.2

Re: [PATCH v5 06/10] dt-bindings: iio: accel: add interrupt-names

[Conor Dooley]

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

On Thu, Dec 05, 2024 at 05:13:39PM +0000, Lothar Rubusch wrote:
> Add interrupt-names INT1 and INT2 for the two interrupt lines of the
> sensor. Only one line will be connected for incoming events. The driver
> needs to be configured accordingly. If no interrupt line is set up, the
> sensor will still measure, but no events are possible.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  .../devicetree/bindings/iio/accel/adi,adxl345.yaml         | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> index 280ed479ef5..67e2c029a6c 100644
> --- a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> +++ b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> @@ -37,6 +37,11 @@ properties:
>    interrupts:
>      maxItems: 1
>  
> +  interrupt-names:
> +    description: Use either INT1 or INT2 for events, or ignore events.
> +    items:
> +      - enum: [INT1, INT2]

The description for this ", or ignore events" does not make sense. Just
drop it, it's clear what happens if you don't provide interrupts.

However, interrupts is a required property but interrupt-names is not.
Seems rather pointless not making interrupt-names a required property
(in the binding!) since if you only add interrupts and not
interrupt-names you can't even use the interrupt as you do not know
whether or not it is INT1 or INT2?

> +
>  required:
>    - compatible
>    - reg
> @@ -61,6 +66,7 @@ examples:
>              reg = <0x2a>;
>              interrupt-parent = <&gpio0>;
>              interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> +            interrupt-names = "INT1";
>          };
>      };
>    - |
> @@ -79,5 +85,6 @@ examples:
>              spi-cpha;
>              interrupt-parent = <&gpio0>;
>              interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> +            interrupt-names = "INT2";
>          };
>      };
> -- 
> 2.39.2
> 

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

Re: [PATCH v5 06/10] dt-bindings: iio: accel: add interrupt-names

[Lothar Rubusch]

On Thu, Dec 5, 2024 at 6:54 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Thu, Dec 05, 2024 at 05:13:39PM +0000, Lothar Rubusch wrote:
> > Add interrupt-names INT1 and INT2 for the two interrupt lines of the
> > sensor. Only one line will be connected for incoming events. The driver
> > needs to be configured accordingly. If no interrupt line is set up, the
> > sensor will still measure, but no events are possible.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > ---
> >  .../devicetree/bindings/iio/accel/adi,adxl345.yaml         | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> > index 280ed479ef5..67e2c029a6c 100644
> > --- a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> > +++ b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> > @@ -37,6 +37,11 @@ properties:
> >    interrupts:
> >      maxItems: 1
> >
> > +  interrupt-names:
> > +    description: Use either INT1 or INT2 for events, or ignore events.
> > +    items:
> > +      - enum: [INT1, INT2]
>
> The description for this ", or ignore events" does not make sense. Just
> drop it, it's clear what happens if you don't provide interrupts.
>
> However, interrupts is a required property but interrupt-names is not.
> Seems rather pointless not making interrupt-names a required property
> (in the binding!) since if you only add interrupts and not
> interrupt-names you can't even use the interrupt as you do not know
> whether or not it is INT1 or INT2?

What I meant is, yes, the sensor needs an interrupt line.
Interrupt-names is optional. The sensor always can measure. When
interrupt-names is specified, though, the sensor will setup a FIFO and
can use events, such as data ready, watermark, single tap, freefall,
etc. Without the interrupt-names, the sensor goes into a "FIFO bypass
mode" without its specific events.

Hence, I better drop the description entirely, since it rather seems
to be confusing.

Best,
L

> > +
> >  required:
> >    - compatible
> >    - reg
> > @@ -61,6 +66,7 @@ examples:
> >              reg = <0x2a>;
> >              interrupt-parent = <&gpio0>;
> >              interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> > +            interrupt-names = "INT1";
> >          };
> >      };
> >    - |
> > @@ -79,5 +85,6 @@ examples:
> >              spi-cpha;
> >              interrupt-parent = <&gpio0>;
> >              interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> > +            interrupt-names = "INT2";
> >          };
> >      };
> > --
> > 2.39.2
> >

Re: [PATCH v5 06/10] dt-bindings: iio: accel: add interrupt-names

[Conor Dooley]

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

On Thu, Dec 05, 2024 at 08:41:52PM +0100, Lothar Rubusch wrote:
> On Thu, Dec 5, 2024 at 6:54 PM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Thu, Dec 05, 2024 at 05:13:39PM +0000, Lothar Rubusch wrote:
> > > Add interrupt-names INT1 and INT2 for the two interrupt lines of the
> > > sensor. Only one line will be connected for incoming events. The driver
> > > needs to be configured accordingly. If no interrupt line is set up, the
> > > sensor will still measure, but no events are possible.
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > ---
> > >  .../devicetree/bindings/iio/accel/adi,adxl345.yaml         | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> > > index 280ed479ef5..67e2c029a6c 100644
> > > --- a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> > > +++ b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> > > @@ -37,6 +37,11 @@ properties:
> > >    interrupts:
> > >      maxItems: 1
> > >
> > > +  interrupt-names:
> > > +    description: Use either INT1 or INT2 for events, or ignore events.
> > > +    items:
> > > +      - enum: [INT1, INT2]
> >
> > The description for this ", or ignore events" does not make sense. Just
> > drop it, it's clear what happens if you don't provide interrupts.
> >
> > However, interrupts is a required property but interrupt-names is not.
> > Seems rather pointless not making interrupt-names a required property
> > (in the binding!) since if you only add interrupts and not
> > interrupt-names you can't even use the interrupt as you do not know
> > whether or not it is INT1 or INT2?
> 
> What I meant is, yes, the sensor needs an interrupt line.
> Interrupt-names is optional. The sensor always can measure. When
> interrupt-names is specified, though, the sensor will setup a FIFO and
> can use events, such as data ready, watermark, single tap, freefall,
> etc. Without the interrupt-names, the sensor goes into a "FIFO bypass
> mode" without its specific events.

What I'm talking about here is how it is ultimately pointless for
interrupts to be a required property if it can never be used without
interrupt-names as you cannot know which interrupt is in use. I think
both should be made mandatory or neither.

> Hence, I better drop the description entirely, since it rather seems
> to be confusing.

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

Re: [PATCH v5 06/10] dt-bindings: iio: accel: add interrupt-names

[Lothar Rubusch]

On Fri, Dec 6, 2024 at 6:08 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Thu, Dec 05, 2024 at 08:41:52PM +0100, Lothar Rubusch wrote:
> > On Thu, Dec 5, 2024 at 6:54 PM Conor Dooley <conor@kernel.org> wrote:
> > >
> > > On Thu, Dec 05, 2024 at 05:13:39PM +0000, Lothar Rubusch wrote:
> > > > Add interrupt-names INT1 and INT2 for the two interrupt lines of the
> > > > sensor. Only one line will be connected for incoming events. The driver
> > > > needs to be configured accordingly. If no interrupt line is set up, the
> > > > sensor will still measure, but no events are possible.
> > > >
> > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > > ---
> > > >  .../devicetree/bindings/iio/accel/adi,adxl345.yaml         | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> > > > index 280ed479ef5..67e2c029a6c 100644
> > > > --- a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> > > > +++ b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> > > > @@ -37,6 +37,11 @@ properties:
> > > >    interrupts:
> > > >      maxItems: 1
> > > >
> > > > +  interrupt-names:
> > > > +    description: Use either INT1 or INT2 for events, or ignore events.
> > > > +    items:
> > > > +      - enum: [INT1, INT2]
> > >
> > > The description for this ", or ignore events" does not make sense. Just
> > > drop it, it's clear what happens if you don't provide interrupts.
> > >
> > > However, interrupts is a required property but interrupt-names is not.
> > > Seems rather pointless not making interrupt-names a required property
> > > (in the binding!) since if you only add interrupts and not
> > > interrupt-names you can't even use the interrupt as you do not know
> > > whether or not it is INT1 or INT2?
> >
> > What I meant is, yes, the sensor needs an interrupt line.
> > Interrupt-names is optional. The sensor always can measure. When
> > interrupt-names is specified, though, the sensor will setup a FIFO and
> > can use events, such as data ready, watermark, single tap, freefall,
> > etc. Without the interrupt-names, the sensor goes into a "FIFO bypass
> > mode" without its specific events.
>
> What I'm talking about here is how it is ultimately pointless for
> interrupts to be a required property if it can never be used without
> interrupt-names as you cannot know which interrupt is in use. I think
> both should be made mandatory or neither.
>

Ah, now I can see your point. I agree that it should be equally
mandatory as the interrupt. Legacy implementations used simply always
just INT1. I'd like to make it configurable in the IIO driver but
tried to avoid the DT topic for now (which was not a smart decision
either). Hence, I added the interrupt-names.
I'm unsure should I make "interrupt-names" a required property now?
What about the existing DTS files using this sensor? There are no
interrupt-names specified, so if made required, the missing
interrupt-names there would break binding check, or not?

> > Hence, I better drop the description entirely, since it rather seems
> > to be confusing.

Re: [PATCH v5 06/10] dt-bindings: iio: accel: add interrupt-names

[Lothar Rubusch]

On Fri, Dec 6, 2024 at 6:29 PM Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> On Fri, Dec 6, 2024 at 6:08 PM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Thu, Dec 05, 2024 at 08:41:52PM +0100, Lothar Rubusch wrote:
> > > On Thu, Dec 5, 2024 at 6:54 PM Conor Dooley <conor@kernel.org> wrote:
> > > >
> > > > On Thu, Dec 05, 2024 at 05:13:39PM +0000, Lothar Rubusch wrote:
> > > > > Add interrupt-names INT1 and INT2 for the two interrupt lines of the
> > > > > sensor. Only one line will be connected for incoming events. The driver
> > > > > needs to be configured accordingly. If no interrupt line is set up, the
> > > > > sensor will still measure, but no events are possible.
> > > > >
> > > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > > > ---
> > > > >  .../devicetree/bindings/iio/accel/adi,adxl345.yaml         | 7 +++++++
> > > > >  1 file changed, 7 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> > > > > index 280ed479ef5..67e2c029a6c 100644
> > > > > --- a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> > > > > +++ b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> > > > > @@ -37,6 +37,11 @@ properties:
> > > > >    interrupts:
> > > > >      maxItems: 1
> > > > >
> > > > > +  interrupt-names:
> > > > > +    description: Use either INT1 or INT2 for events, or ignore events.
> > > > > +    items:
> > > > > +      - enum: [INT1, INT2]
> > > >
> > > > The description for this ", or ignore events" does not make sense. Just
> > > > drop it, it's clear what happens if you don't provide interrupts.
> > > >
> > > > However, interrupts is a required property but interrupt-names is not.
> > > > Seems rather pointless not making interrupt-names a required property
> > > > (in the binding!) since if you only add interrupts and not
> > > > interrupt-names you can't even use the interrupt as you do not know
> > > > whether or not it is INT1 or INT2?
> > >
> > > What I meant is, yes, the sensor needs an interrupt line.
> > > Interrupt-names is optional. The sensor always can measure. When
> > > interrupt-names is specified, though, the sensor will setup a FIFO and
> > > can use events, such as data ready, watermark, single tap, freefall,
> > > etc. Without the interrupt-names, the sensor goes into a "FIFO bypass
> > > mode" without its specific events.
> >
> > What I'm talking about here is how it is ultimately pointless for
> > interrupts to be a required property if it can never be used without
> > interrupt-names as you cannot know which interrupt is in use. I think
> > both should be made mandatory or neither.
> >
>
> Ah, now I can see your point. I agree that it should be equally
> mandatory as the interrupt. Legacy implementations used simply always
> just INT1. I'd like to make it configurable in the IIO driver but
> tried to avoid the DT topic for now (which was not a smart decision
> either). Hence, I added the interrupt-names.
> I'm unsure should I make "interrupt-names" a required property now?
> What about the existing DTS files using this sensor? There are no
> interrupt-names specified, so if made required, the missing
> interrupt-names there would break binding check, or not?
>

Sorry, I have to clarify myself, yesterday I was not focussed..

1. I agree this is kind of half way. Either, both are required or none of them.
If both were required, also the older DTS files using the ADXL345 would
need to be "fixed". If I add interrupt-names, it works with my patches for the
"newer" IIO driver, because since I implement it it's using interrupt-names.
The older input driver for that using interrupt, does not use interrupt-names.
Hence, it requires the interrupt in the DT. But it does not require
interrupt-names
(historical stuff).

2. AFAIK the sensor can operate w/o interrupts.
A) w/o INT line: measuring is possible; FIFO bypassed; no events
B) w/ INT line: measuring is possible; can use FIFO; events are possible
When setting the interrupt in DT, the interrupt line name can/could be
configured also via SW (setting up the registers of the sensor). So, it's not
impossible. This is AFAIR the approach in the legacy input driver. Now, there
is devicetree, and both should probably be better configured somewhere in
the DT

3. IMHO neither one, not the interrupt, nor the interrupt-names need to be
a required DT-binding.
If interrupt is required and interrupt-names not, it's a half-way approach,
which leaves specifying the IRQ line open to be solved partly in DT
(declaration of the interrupt) and partly in SW (configuration of the
interrupt line to use), e.g. hardcoded or configurable somewhere in the
driver via sysfs or the like. Not nice.

Pls, let me know what you think, and in case, if I need to take some
action, here.
Best,
L


> > > Hence, I better drop the description entirely, since it rather seems
> > > to be confusing.

Re: [PATCH v5 06/10] dt-bindings: iio: accel: add interrupt-names

[Jonathan Cameron]

On Fri, 6 Dec 2024 18:29:48 +0100
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> On Fri, Dec 6, 2024 at 6:08 PM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Thu, Dec 05, 2024 at 08:41:52PM +0100, Lothar Rubusch wrote:  
> > > On Thu, Dec 5, 2024 at 6:54 PM Conor Dooley <conor@kernel.org> wrote:  
> > > >
> > > > On Thu, Dec 05, 2024 at 05:13:39PM +0000, Lothar Rubusch wrote:  
> > > > > Add interrupt-names INT1 and INT2 for the two interrupt lines of the
> > > > > sensor. Only one line will be connected for incoming events. The driver
> > > > > needs to be configured accordingly. If no interrupt line is set up, the
> > > > > sensor will still measure, but no events are possible.
> > > > >
> > > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > > > ---
> > > > >  .../devicetree/bindings/iio/accel/adi,adxl345.yaml         | 7 +++++++
> > > > >  1 file changed, 7 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
Side note, but patch name must include what device it is!

dt-bindings: iio: accel: adxl345: ...

> > > > > index 280ed479ef5..67e2c029a6c 100644
> > > > > --- a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> > > > > +++ b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> > > > > @@ -37,6 +37,11 @@ properties:
> > > > >    interrupts:
> > > > >      maxItems: 1
> > > > >
> > > > > +  interrupt-names:
> > > > > +    description: Use either INT1 or INT2 for events, or ignore events.
> > > > > +    items:
> > > > > +      - enum: [INT1, INT2]  
> > > >
> > > > The description for this ", or ignore events" does not make sense. Just
> > > > drop it, it's clear what happens if you don't provide interrupts.
> > > >
> > > > However, interrupts is a required property but interrupt-names is not.
> > > > Seems rather pointless not making interrupt-names a required property
> > > > (in the binding!) since if you only add interrupts and not
> > > > interrupt-names you can't even use the interrupt as you do not know
> > > > whether or not it is INT1 or INT2?  
> > >
> > > What I meant is, yes, the sensor needs an interrupt line.
> > > Interrupt-names is optional. The sensor always can measure. When
> > > interrupt-names is specified, though, the sensor will setup a FIFO and
> > > can use events, such as data ready, watermark, single tap, freefall,
> > > etc. Without the interrupt-names, the sensor goes into a "FIFO bypass
> > > mode" without its specific events.  
> >
> > What I'm talking about here is how it is ultimately pointless for
> > interrupts to be a required property if it can never be used without
> > interrupt-names as you cannot know which interrupt is in use. I think
> > both should be made mandatory or neither.
> >  
> 
> Ah, now I can see your point. I agree that it should be equally
> mandatory as the interrupt. Legacy implementations used simply always
> just INT1. I'd like to make it configurable in the IIO driver but
> tried to avoid the DT topic for now (which was not a smart decision
> either). Hence, I added the interrupt-names.
> I'm unsure should I make "interrupt-names" a required property now?
> What about the existing DTS files using this sensor? There are no
> interrupt-names specified, so if made required, the missing
> interrupt-names there would break binding check, or not?

Neither should be required.  The driver isn't currently using interrupts
and I presume it is functional?  So I'd just drop the required on interrupts.
Now a condition that says interrupt-names is needed if interrupts is supplied
would be a useful addition (IIRC there are examples of that in tree).

So interrupts being required is a bug that we should fix by just
dropping that.  

Jonathan



> 
> > > Hence, I better drop the description entirely, since it rather seems
> > > to be confusing.  

Re: [PATCH v5 06/10] dt-bindings: iio: accel: add interrupt-names

[Jonathan Cameron]

On Sat, 7 Dec 2024 13:10:22 +0100
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> On Fri, Dec 6, 2024 at 6:29 PM Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >
> > On Fri, Dec 6, 2024 at 6:08 PM Conor Dooley <conor@kernel.org> wrote:  
> > >
> > > On Thu, Dec 05, 2024 at 08:41:52PM +0100, Lothar Rubusch wrote:  
> > > > On Thu, Dec 5, 2024 at 6:54 PM Conor Dooley <conor@kernel.org> wrote:  
> > > > >
> > > > > On Thu, Dec 05, 2024 at 05:13:39PM +0000, Lothar Rubusch wrote:  
> > > > > > Add interrupt-names INT1 and INT2 for the two interrupt lines of the
> > > > > > sensor. Only one line will be connected for incoming events. The driver
> > > > > > needs to be configured accordingly. If no interrupt line is set up, the
> > > > > > sensor will still measure, but no events are possible.
> > > > > >
> > > > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > > > > ---
> > > > > >  .../devicetree/bindings/iio/accel/adi,adxl345.yaml         | 7 +++++++
> > > > > >  1 file changed, 7 insertions(+)
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> > > > > > index 280ed479ef5..67e2c029a6c 100644
> > > > > > --- a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> > > > > > +++ b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> > > > > > @@ -37,6 +37,11 @@ properties:
> > > > > >    interrupts:
> > > > > >      maxItems: 1
> > > > > >
> > > > > > +  interrupt-names:
> > > > > > +    description: Use either INT1 or INT2 for events, or ignore events.
> > > > > > +    items:
> > > > > > +      - enum: [INT1, INT2]  
> > > > >
> > > > > The description for this ", or ignore events" does not make sense. Just
> > > > > drop it, it's clear what happens if you don't provide interrupts.
> > > > >
> > > > > However, interrupts is a required property but interrupt-names is not.
> > > > > Seems rather pointless not making interrupt-names a required property
> > > > > (in the binding!) since if you only add interrupts and not
> > > > > interrupt-names you can't even use the interrupt as you do not know
> > > > > whether or not it is INT1 or INT2?  
> > > >
> > > > What I meant is, yes, the sensor needs an interrupt line.
> > > > Interrupt-names is optional. The sensor always can measure. When
> > > > interrupt-names is specified, though, the sensor will setup a FIFO and
> > > > can use events, such as data ready, watermark, single tap, freefall,
> > > > etc. Without the interrupt-names, the sensor goes into a "FIFO bypass
> > > > mode" without its specific events.  
> > >
> > > What I'm talking about here is how it is ultimately pointless for
> > > interrupts to be a required property if it can never be used without
> > > interrupt-names as you cannot know which interrupt is in use. I think
> > > both should be made mandatory or neither.
> > >  
> >
> > Ah, now I can see your point. I agree that it should be equally
> > mandatory as the interrupt. Legacy implementations used simply always
> > just INT1. I'd like to make it configurable in the IIO driver but
> > tried to avoid the DT topic for now (which was not a smart decision
> > either). Hence, I added the interrupt-names.
> > I'm unsure should I make "interrupt-names" a required property now?
> > What about the existing DTS files using this sensor? There are no
> > interrupt-names specified, so if made required, the missing
> > interrupt-names there would break binding check, or not?
> >  
> 
> Sorry, I have to clarify myself, yesterday I was not focussed..
> 
> 1. I agree this is kind of half way. Either, both are required or none of them.
> If both were required, also the older DTS files using the ADXL345 would
> need to be "fixed".

Easy. If they aren't both provided, no interrupts are used.
Driver carries on working bug less functionality.  That's fine.

> If I add interrupt-names, it works with my patches for the
> "newer" IIO driver, because since I implement it it's using interrupt-names.
> The older input driver for that using interrupt, does not use interrupt-names.
> Hence, it requires the interrupt in the DT. But it does not require
> interrupt-names
> (historical stuff).

We don't care.  The required list should be about requirements for the
hardware to function in a useful fashion, not if the driver currently supports
that mode.  So it should never have been required even if the driver at the
time required it because no one had done the work to make it work without.

In theory you could provide a default for interrupt-names I guess if
do want to be nice to the legacy driver.

> 
> 2. AFAIK the sensor can operate w/o interrupts.
> A) w/o INT line: measuring is possible; FIFO bypassed; no events
> B) w/ INT line: measuring is possible; can use FIFO; events are possible
> When setting the interrupt in DT, the interrupt line name can/could be
> configured also via SW (setting up the registers of the sensor). So, it's not
> impossible. This is AFAIR the approach in the legacy input driver. Now, there
> is devicetree, and both should probably be better configured somewhere in
> the DT

Agreed no interrupts are required for device to do something useful.
(not sure I follow the rest of this entry).
> 
> 3. IMHO neither one, not the interrupt, nor the interrupt-names need to be
> a required DT-binding.
> If interrupt is required and interrupt-names not, it's a half-way approach,
> which leaves specifying the IRQ line open to be solved partly in DT
> (declaration of the interrupt) and partly in SW (configuration of the
> interrupt line to use), e.g. hardcoded or configurable somewhere in the
> driver via sysfs or the like. Not nice.

Only way I can see to be nice about this is to specify a default.
However, if someone is using the input driver and we have interrupt names
that don't match the default, all bets are off.  That setup doesn't work
today anyway, so do we care? I don't think so.

So in conclusion. Drop the required entry for interrupts, but consider
if a default can work for interrupt-names, or whether we should add
the logic to require interrupt-names if interrupts are provided.

Jonathan

> 
> Pls, let me know what you think, and in case, if I need to take some
> action, here.
> Best,
> L
> 
> 
> > > > Hence, I better drop the description entirely, since it rather seems
> > > > to be confusing.  

Re: [PATCH v5 01/10] iio: accel: adxl345: refrase comment on probe

[Jonathan Cameron]

On Thu,  5 Dec 2024 17:13:34 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Refrase comment on the probe function, avoid naming different hardware.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
I'll pick up the ones that are ready to apply so as to shorten v6 etc.

Applied this one.

> ---
>  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 006ce66c0aa..eb3ce4434a5 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -169,8 +169,7 @@ static void adxl345_powerdown(void *regmap)
>  }
>  
>  /**
> - * adxl345_core_probe() - probe and setup for the adxl345 accelerometer,
> - *                        also covers the adlx375 accelerometer
> + * adxl345_core_probe() - Probe and setup for the accelerometer.
>   * @dev:	Driver model representation of the device
>   * @regmap:	Regmap instance for the device
>   * @setup:	Setup routine to be executed right before the standard device

Re: [PATCH v5 02/10] iio: accel: adxl345: rename variable data to st

[Jonathan Cameron]

On Thu,  5 Dec 2024 17:13:35 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Rename the locally used variable data to st. The st refers to "state",
> representing the internal state of the driver object. Further it
> prepares the usage of an internal data pointer needed for the
> implementation of the sensor features.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
Applied to the togreg branch of iio.git. Initially pushed out as testing
to let the bots take a look.

Re: [PATCH v5 03/10] iio: accel: adxl345: measure right-justified

[Jonathan Cameron]

On Thu,  5 Dec 2024 17:13:36 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Make measurements right-justified, since it is the default for the
> driver and sensor. By not setting the ADXL345_DATA_FORMAT_JUSTIFY bit,
> the data becomes right-judstified. This was the original setting, there
> is no reason to change it to left-justified, where right-justified
> simplifies working on the registers.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>

I'm still confused by this one.  Does this change affect the data output
to userspace?  If seems like it definitely should. If it does we have
an ABI regression somewhere. Is it currently broken and wasn't at some
earlier stage, or is this the patch breaking things?

If it worked and currently doesn't send a fix.  If this changes a previously
working ABI then drop this patch.  Alternative being to fix up the scale
handling to incorporate this justification change.

Jonathan

> ---
>  drivers/iio/accel/adxl345_core.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 88df9547bd6..98ff37271f1 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -184,7 +184,6 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  	struct iio_dev *indio_dev;
>  	u32 regval;
>  	unsigned int data_format_mask = (ADXL345_DATA_FORMAT_RANGE |
> -					 ADXL345_DATA_FORMAT_JUSTIFY |
>  					 ADXL345_DATA_FORMAT_FULL_RES |
>  					 ADXL345_DATA_FORMAT_SELF_TEST);
>  	int ret;

Re: [PATCH v5 04/10] iio: accel: adxl345: add function to switch measuring mode

[Jonathan Cameron]

On Thu,  5 Dec 2024 17:13:37 +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>
Hi Lothar,

One trivial comment inline.

Jonathan

> ---
>  drivers/iio/accel/adxl345_core.c | 44 ++++++++++++++++++++++----------
>  1 file changed, 31 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 98ff37271f1..1d020b0d79c 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -138,6 +138,34 @@ static int adxl345_write_raw_get_fmt(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +/**
> + * adxl345_set_measure_en() - Enable and disable measuring.
> + *
> + * @st: The device data.
> + * @en: Enable measurements, else standby mode.
> + *
> + * For lowest power operation, standby mode can be used. In standby mode,
> + * current consumption is supposed to be reduced to 0.1uA (typical). In this
> + * mode no measurements are made. Placing the device into standby mode
> + * preserves the contents of FIFO.
> + *
> + * Return: Returns 0 if successful, or a negative error value.
> + */
> +static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
> +{
> +	unsigned int val = 0;
> +
> +	val = (en) ? ADXL345_POWER_CTL_MEASURE : ADXL345_POWER_CTL_STANDBY;
> +	return regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
> +}
> +
> +static void adxl345_powerdown(void *ptr)
> +{
> +	struct adxl345_state *st = ptr;
> +
> +	adxl345_set_measure_en(st, false);
> +}
> +
>  static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
>  "0.09765625 0.1953125 0.390625 0.78125 1.5625 3.125 6.25 12.5 25 50 100 200 400 800 1600 3200"
>  );
> @@ -158,16 +186,6 @@ static const struct iio_info adxl345_info = {
>  	.write_raw_get_fmt	= adxl345_write_raw_get_fmt,
>  };
>  
> -static int adxl345_powerup(void *regmap)
> -{
> -	return regmap_write(regmap, ADXL345_REG_POWER_CTL, ADXL345_POWER_CTL_MEASURE);
> -}
> -
> -static void adxl345_powerdown(void *regmap)
> -{
> -	regmap_write(regmap, ADXL345_REG_POWER_CTL, ADXL345_POWER_CTL_STANDBY);
> -}
> -
>  /**
>   * adxl345_core_probe() - Probe and setup for the accelerometer.
>   * @dev:	Driver model representation of the device
> @@ -236,13 +254,13 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  				     regval, ADXL345_DEVID);
>  
>  	/* Enable measurement mode */
> -	ret = adxl345_powerup(st->regmap);
> +	ret = adxl345_set_measure_en(st, true);
>  	if (ret < 0)
>  		return dev_err_probe(dev, ret, "Failed to enable measurement mode\n");
>  
> -	ret = devm_add_action_or_reset(dev, adxl345_powerdown, st->regmap);
> +	ret = devm_add_action_or_reset(dev, adxl345_powerdown, st);
>  	if (ret < 0)
> -		return ret;
> +		return dev_err_probe(dev, ret, "Failed to add action or reset\n");
You will never see that message, though arguably that's an implementation detail.

The only error that devm_add_action_or_reset() returns is -ENOMEM;
dev_err_probe() doesn't print on -ENOMEM because enough screaming occurs at
other layers.

I normally don't bother commenting on this one if it's introduced as one of
many messages, but here you are adding just this one so I have commented.


>  
>  	return devm_iio_device_register(dev, indio_dev);
>  }

Re: [PATCH v5 05/10] iio: accel: adxl345: complete list of defines

[Jonathan Cameron]

On Thu,  5 Dec 2024 17:13:38 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Extend the list of constants. Keep them the header file for readability.
> The defines allow the implementation of events like watermark, single
> tap, double tap, freefall, etc.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl345.h | 89 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 77 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> index 3d5c8719db3..ed81d5cf445 100644
> --- a/drivers/iio/accel/adxl345.h
> +++ b/drivers/iio/accel/adxl345.h
> @@ -9,37 +9,102 @@
>  #define _ADXL345_H_
>  
>  #define ADXL345_REG_DEVID		0x00
> +#define ADXL345_REG_THRESH_TAP	0x1D
>  #define ADXL345_REG_OFSX		0x1E
>  #define ADXL345_REG_OFSY		0x1F
>  #define ADXL345_REG_OFSZ		0x20
> -#define ADXL345_REG_OFS_AXIS(index)	(ADXL345_REG_OFSX + (index))
> +/* Tap duration */
> +#define ADXL345_REG_DUR		0x21
> +/* Tap latency */
> +#define ADXL345_REG_LATENT		0x22
> +/* Tap window */
> +#define ADXL345_REG_WINDOW		0x23
> +/* Activity threshold */
> +#define ADXL345_REG_THRESH_ACT		0x24
> +/* Inactivity threshold */
> +#define ADXL345_REG_THRESH_INACT	0x25
> +/* Inactivity time */
> +#define ADXL345_REG_TIME_INACT	0x26
> +/* Axis enable control for activity and inactivity detection */
> +#define ADXL345_REG_ACT_INACT_CTRL	0x27
> +/* Free-fall threshold */
> +#define ADXL345_REG_THRESH_FF		0x28
> +/* Free-fall time */
> +#define ADXL345_REG_TIME_FF		0x29
> +/* Axis control for single tap or double tap */
> +#define ADXL345_REG_TAP_AXIS		0x2A
> +/* Source of single tap or double tap */
> +#define ADXL345_REG_ACT_TAP_STATUS	0x2B
> +/* Data rate and power mode control */
>  #define ADXL345_REG_BW_RATE		0x2C
>  #define ADXL345_REG_POWER_CTL		0x2D
> +#define ADXL345_REG_INT_ENABLE		0x2E
> +#define ADXL345_REG_INT_MAP		0x2F
> +#define ADXL345_REG_INT_SOURCE		0x30
>  #define ADXL345_REG_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_DEVID			0xE5
> +
> +#define ADXL345_FIFO_CTL_SAMLPES(x)	(0x1f & (x))

SAMPLES?

> +#define ADXL345_FIFO_CTL_TRIGGER(x)	(0x20 & ((x) << 5)) /* 0: INT1, 1: INT2 */
> +#define ADXL345_FIFO_CTL_MODE(x)	(0xc0 & ((x) << 6))

Supply the mask only and use FIELD_PREP() for these.


>  
> +#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
> +
> +#define ADXL345_INT1			0
> +#define ADXL345_INT2			1
> +
> +/*
> + * 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_MEASURE	BIT(3)
>  #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_IS_LEFT_JUSTIFIED	BIT(2)
>  #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 */
> -
> +#define ADXL345_DATA_FORMAT_SPI_3WIRE	BIT(6)
> +#define ADXL345_DATA_FORMAT_SELF_TEST	BIT(7)
>  #define ADXL345_DATA_FORMAT_2G		0
>  #define ADXL345_DATA_FORMAT_4G		1
>  #define ADXL345_DATA_FORMAT_8G		2
>  #define ADXL345_DATA_FORMAT_16G		3
>  
> -#define ADXL345_DEVID			0xE5
> +#define ADXL345_REG_OFS_AXIS(index)	(ADXL345_REG_OFSX + (index))
I'm not sure on logic of moving this to the end. Seems fine next
to the definitions of the individual axis to me.

> +
> +/*
> + * FIFO stores a maximum of 32 entries, which equates to a maximum of 33 entries
> + * available at any given time because an additional entry is available at the
> + * output filter of the device.
> + *
> + * (see datasheet FIFO_STATUS description on "Entries Bits")
> + */
> +#define ADXL345_FIFO_SIZE  33
>  
>  /*
>   * In full-resolution mode, scale factor is maintained at ~4 mg/LSB

Re: [PATCH v5 07/10] iio: accel: adxl345: introduce interrupt handling

[Jonathan Cameron]

On Thu,  5 Dec 2024 17:13:40 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> 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 | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 1d020b0d79c..e0a8b32239f 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -11,6 +11,7 @@
>  #include <linux/property.h>
>  #include <linux/regmap.h>
>  #include <linux/units.h>
> +#include <linux/interrupt.h>
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> @@ -18,8 +19,10 @@
>  #include "adxl345.h"
>  
>  struct adxl345_state {
> +	int irq;
Ordering is non obvious. I'd keep the pointers first and have
this before intio.
>  	const struct adxl345_chip_info *info;
>  	struct regmap *regmap;
> +	u8 intio;
>  };
>  
>  #define ADXL345_CHANNEL(index, axis) {					\
> @@ -212,6 +215,17 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  
>  	st = iio_priv(indio_dev);
>  	st->regmap = regmap;
> +
> +	st->intio = -1;
It's a u8 so negative doesn't seem sensible choice.

> +	st->irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT1");
> +	if (st->irq > 0)
Where one leg needs {} convention is to use it for all of them.
https://www.kernel.org/doc/html/v4.10/process/coding-style.html#placing-braces-and-spaces
last part.



> +		st->intio = ADXL345_INT1;
> +	else {
> +		st->irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT2");
> +		if (st->irq > 0)
> +			st->intio = ADXL345_INT2;
> +	}
> +
>  	st->info = device_get_match_data(dev);
>  	if (!st->info)
>  		return -ENODEV;

Re: [PATCH v5 08/10] iio: accel: adxl345: initialize FIFO delay value for SPI

[Jonathan Cameron]

On Thu,  5 Dec 2024 17:13:41 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> 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>
Hi Lothar,

1 trivial comment below.

Jonathan
> diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl345_spi.c
> index 61fd9a6f5fc..9829d5d3d43 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);

Excess brackets.  I'd drop them.

>  	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 = {

Re: [PATCH v5 09/10] iio: accel: adxl345: prepare channel for scan_index

[Jonathan Cameron]

On Thu,  5 Dec 2024 17:13:42 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add separate fields for register and index to the channel definition.
> The scan_index is set up with the kfifo in the follow up patches.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
I'd just squash this with the net patch. It's a trivial change and there
isn't anything specific to really highlight in this description.
Also, scan_index separate from the stuff about the format seems odd.

Jonathan

> ---
>  drivers/iio/accel/adxl345_core.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 0696e908bdf..3067a70c54e 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -26,21 +26,26 @@ struct adxl345_state {
>  	u8 intio;
>  };
>  
> -#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),				\
>  }
>  
> +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 int adxl345_read_raw(struct iio_dev *indio_dev,

Re: [PATCH v5 10/10] iio: accel: adxl345: add FIFO with watermark events

[Jonathan Cameron]

On Thu,  5 Dec 2024 17:13:43 +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>
Various comments inline.

Thanks,

Jonathan

> ---
>  drivers/iio/accel/adxl345_core.c | 300 +++++++++++++++++++++++++++++++
>  1 file changed, 300 insertions(+)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 3067a70c54e..58ed82d66dc 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -15,15 +15,28 @@
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/events.h>

I'm not seeing any use of this header yet.  Bring it in when you need it.

> +#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
> +
>  struct adxl345_state {
>  	int irq;
>  	const struct adxl345_chip_info *info;
>  	struct regmap *regmap;
> +	__le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE];
>  	bool fifo_delay; /* delay: delay is needed for SPI */
>  	u8 intio;
> +	u8 int_map;
> +	u8 watermark;
> +	u8 fifo_mode;
>  };
>  
>  #define ADXL345_CHANNEL(index, reg, axis) {					\
> @@ -36,6 +49,13 @@ struct adxl345_state {
>  	.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,			\
> +		.shift = 0,				\

No need to specify shift of 0. It's the 'obvious' default and C will set it to 0
for you anyway.

> +		.endianness = IIO_LE,			\
> +	},						\
>  }
>  
>  enum adxl345_chans {
> @@ -48,6 +68,25 @@ static const struct iio_chan_spec adxl345_channels[] = {
>  	ADXL345_CHANNEL(2, chan_z, Z),
>  };
>  
> +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

	/*
	 * Any bits....


> +	 * 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 = 0xFF & (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);
> +}


> +/**
> + * 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 0x3f & regval;
FIELD_GET() for all stuff like this with appropriate #define for the mask.

> +}
> +
> +/**
> + * 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,
> + * a fifo_entry has 3 elements for X, Y and Z direction of 2 bytes each.
> + *
> + * 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 at once.

Doesn't match the code which is reading just one register lots of times.

> + *
> + * Return: 0 or error value.
> + */
> +static int adxl345_fifo_transfer(struct adxl345_state *st, int samples)
> +{
> +	size_t count;
> +	int i, ret;
> +
> +	count = sizeof(st->fifo_buf[0]) * ADXL345_DIRS;
> +	for (i = 0; i < samples; i++) {
> +		ret = regmap_noinc_read(st->regmap, ADXL345_REG_XYZ_BASE,
> +				st->fifo_buf + (i * count / 2), count);
> +		if (ret < 0)
> +			return ret;

This is where I'd expect to see the delay mentioned below.

> +	}
> +	return ret;
> +}
> +

> +static int adxl345_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct adxl345_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	st->int_map = 0x00;
> +
> +	ret = adxl345_set_interrupts(st);
> +	if (ret < 0)
> +		return ret;
> +
> +	st->fifo_mode = ADXL345_FIFO_BYPASS;
> +	return adxl345_set_fifo(st);
I'd normally expect order in predisable to be reverse of that in postenable.
Why isn't it?  That is why is the set_fifo here after set_interrupts and
not before it.  Add a comment.

> +}
> +
> +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 (0xff & regval);

No brackets needed.

> +}
> +
> +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) {
> +		/*
> +		 * 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);

I'm not following why a delay here helps.  At this point you're read masses of
data from the fifo without the delays you mention.

> +
> +		iio_push_to_buffers(indio_dev, &st->fifo_buf[i]);
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * adxl345_event_handler() - Handle events of the ADXL345.

Up to you but...
Given it's an IIO driver and that we have a very specific meaning
for events, maybe just call this adxl345_irq_handler()?

> + * @irq: The irq being handled.
> + * @p: The struct iio_device pointer for the device.
> + *
> + * Return: The interrupt was handled.
> + */
> +static irqreturn_t adxl345_event_handler(int irq, void *p)
> +{
> +	struct iio_dev *indio_dev = p;
> +	struct adxl345_state *st = iio_priv(indio_dev);
> +	u8 int_stat;
> +	int samples;
> +
> +	int_stat = adxl345_get_status(st);
> +	if (int_stat < 0)
> +		return IRQ_NONE;
> +
> +	if (int_stat == 0x0)
Doesn't this correspond to 'not our interrupt'?
If that's the case return IRQ_NONE is the right way to go and not reset the
interrupt.  You have registered it as maybe shared, and if it is, then this
is a common thing to happen as interrupt from another device.

> +		goto err;
> +
> +	if (int_stat & ADXL345_INT_OVERRUN)
> +		goto err;
> +
> +	if (int_stat & (ADXL345_INT_DATA_READY | ADXL345_INT_WATERMARK)) {

I think you only ever enable the INT_WATERMARK?  If so does it
make sense to check for DATA_READY as well?

> +		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,
>  };
>  
>  /**
> @@ -222,6 +499,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  	unsigned int data_format_mask = (ADXL345_DATA_FORMAT_RANGE |
>  					 ADXL345_DATA_FORMAT_FULL_RES |
>  					 ADXL345_DATA_FORMAT_SELF_TEST);
> +	u8 fifo_ctl;
>  	int ret;
>  
>  	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> @@ -293,6 +571,28 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  	if (ret < 0)
>  		return dev_err_probe(dev, ret, "Failed to add action or reset\n");
>  
> +	if (st->irq > 0) {
> +		dev_dbg(dev, "initialize for FIFO_STREAM mode\n");
> +
> +		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_event_handler,
> +				IRQF_SHARED | IRQF_ONESHOT,
> +				indio_dev->name, indio_dev);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "Failed to setup triggered buffer\n");
> +
> +	} else {
> +		dev_dbg(dev, "initialize for FIFO_BYPASS mode (fallback)\n");
> +
Given you haven't removed this code from elsewhere, was the driver relying
on the defaults after reset before this patch? 

I don't mind having this branch as a form of documentation even if that's
true but maybe add a note to the patch description.

> +		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 v5 03/10] iio: accel: adxl345: measure right-justified

[Lothar Rubusch]

Dear IIO-ML, Hi Jonathan!

On Sun, Dec 8, 2024 at 2:35 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu,  5 Dec 2024 17:13:36 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Make measurements right-justified, since it is the default for the
> > driver and sensor. By not setting the ADXL345_DATA_FORMAT_JUSTIFY bit,
> > the data becomes right-judstified. This was the original setting, there
> > is no reason to change it to left-justified, where right-justified
> > simplifies working on the registers.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
>
> I'm still confused by this one.  Does this change affect the data output
> to userspace?  If seems like it definitely should. If it does we have
> an ABI regression somewhere. Is it currently broken and wasn't at some
> earlier stage, or is this the patch breaking things?

No, it should not affect the userspace.

This setting opens the mask for regmap/update bits to allow for
changing the data format.
My point is rather, does it actually makes sense to allow to change
the data format, since
the driver will use just one format. The bit was never applied, it's
just the mask here.

May I ask you, if you could also could give me a brief feedback to the
three questions in
the cover letter to this series?

I would really appreciate, since I'm still unsure if I actually
verified everything correctly.
From what I did about this bit, I removed and set the justified bit in
STREAM and in
BYPASSED mode (current mode), without any difference in the results in
iio_info or
iio_readdev. The numbers look generally odd to me, though. And, I'd
rather like to ask
to still wait with applying the patches, if this is ok for you? But,
perhaps with the answers
of the cover letter items, it could become clearer to me. I'm still
about to measure and
verify against the old and the input driver results as comparison.

Best,
L


> If it worked and currently doesn't send a fix.  If this changes a previously
> working ABI then drop this patch.  Alternative being to fix up the scale
> handling to incorporate this justification change.
>
> Jonathan
>
> > ---
> >  drivers/iio/accel/adxl345_core.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > index 88df9547bd6..98ff37271f1 100644
> > --- a/drivers/iio/accel/adxl345_core.c
> > +++ b/drivers/iio/accel/adxl345_core.c
> > @@ -184,7 +184,6 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> >       struct iio_dev *indio_dev;
> >       u32 regval;
> >       unsigned int data_format_mask = (ADXL345_DATA_FORMAT_RANGE |
> > -                                      ADXL345_DATA_FORMAT_JUSTIFY |
> >                                        ADXL345_DATA_FORMAT_FULL_RES |
> >                                        ADXL345_DATA_FORMAT_SELF_TEST);
> >       int ret;
>

Re: [PATCH v5 10/10] iio: accel: adxl345: add FIFO with watermark events

[Dan Carpenter]

Hi Lothar,

kernel test robot noticed the following build warnings:

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Lothar-Rubusch/iio-accel-adxl345-refrase-comment-on-probe/20241206-011802
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20241205171343.308963-11-l.rubusch%40gmail.com
patch subject: [PATCH v5 10/10] iio: accel: adxl345: add FIFO with watermark events
config: nios2-randconfig-r072-20241210 (https://download.01.org/0day-ci/archive/20241210/202412101132.Kj6R6i3h-lkp@intel.com/config)
compiler: nios2-linux-gcc (GCC) 14.2.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202412101132.Kj6R6i3h-lkp@intel.com/

smatch warnings:
drivers/iio/accel/adxl345_core.c:441 adxl345_event_handler() warn: unsigned 'int_stat' is never less than zero.

vim +/ret +321 drivers/iio/accel/adxl345_core.c

55d2386488598bb Lothar Rubusch 2024-12-05  433  static irqreturn_t adxl345_event_handler(int irq, void *p)
55d2386488598bb Lothar Rubusch 2024-12-05  434  {
55d2386488598bb Lothar Rubusch 2024-12-05  435  	struct iio_dev *indio_dev = p;
55d2386488598bb Lothar Rubusch 2024-12-05  436  	struct adxl345_state *st = iio_priv(indio_dev);
55d2386488598bb Lothar Rubusch 2024-12-05  437  	u8 int_stat;
                                                        ^^^^^^^^^^^

55d2386488598bb Lothar Rubusch 2024-12-05  438  	int samples;
55d2386488598bb Lothar Rubusch 2024-12-05  439  
55d2386488598bb Lothar Rubusch 2024-12-05  440  	int_stat = adxl345_get_status(st);
55d2386488598bb Lothar Rubusch 2024-12-05 @441  	if (int_stat < 0)
                                                            ^^^^^^^^^^^^
signedness bug

55d2386488598bb Lothar Rubusch 2024-12-05  442  		return IRQ_NONE;
55d2386488598bb Lothar Rubusch 2024-12-05  443  
55d2386488598bb Lothar Rubusch 2024-12-05  444  	if (int_stat == 0x0)
55d2386488598bb Lothar Rubusch 2024-12-05  445  		goto err;
55d2386488598bb Lothar Rubusch 2024-12-05  446  
55d2386488598bb Lothar Rubusch 2024-12-05  447  	if (int_stat & ADXL345_INT_OVERRUN)
55d2386488598bb Lothar Rubusch 2024-12-05  448  		goto err;
55d2386488598bb Lothar Rubusch 2024-12-05  449  
55d2386488598bb Lothar Rubusch 2024-12-05  450  	if (int_stat & (ADXL345_INT_DATA_READY | ADXL345_INT_WATERMARK)) {
55d2386488598bb Lothar Rubusch 2024-12-05  451  		samples = adxl345_get_samples(st);
55d2386488598bb Lothar Rubusch 2024-12-05  452  		if (samples < 0)
55d2386488598bb Lothar Rubusch 2024-12-05  453  			goto err;
55d2386488598bb Lothar Rubusch 2024-12-05  454  
55d2386488598bb Lothar Rubusch 2024-12-05  455  		if (adxl345_fifo_push(indio_dev, samples) < 0)
55d2386488598bb Lothar Rubusch 2024-12-05  456  			goto err;
55d2386488598bb Lothar Rubusch 2024-12-05  457  
55d2386488598bb Lothar Rubusch 2024-12-05  458  	}
55d2386488598bb Lothar Rubusch 2024-12-05  459  	return IRQ_HANDLED;
55d2386488598bb Lothar Rubusch 2024-12-05  460  
55d2386488598bb Lothar Rubusch 2024-12-05  461  err:
55d2386488598bb Lothar Rubusch 2024-12-05  462  	adxl345_fifo_reset(st);
55d2386488598bb Lothar Rubusch 2024-12-05  463  
55d2386488598bb Lothar Rubusch 2024-12-05  464  	return IRQ_HANDLED;
55d2386488598bb Lothar Rubusch 2024-12-05  465  }

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v5 02/10] iio: accel: adxl345: rename variable data to st

[Lothar Rubusch]

Hi,

On Sun, Dec 8, 2024 at 2:28 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu,  5 Dec 2024 17:13:35 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Rename the locally used variable data to st. The st refers to "state",
> > representing the internal state of the driver object. Further it
> > prepares the usage of an internal data pointer needed for the
> > implementation of the sensor features.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> Applied to the togreg branch of iio.git. Initially pushed out as testing
> to let the bots take a look.

Should I actually drop the "applied" patches in a v6? Or, may I keep them?

I see that Dan Carpenters smatch now comes up with some issues. So, do
the fixes go into v6 here, or better separate?

Best,
L

Re: [PATCH v5 04/10] iio: accel: adxl345: add function to switch measuring mode

[Lothar Rubusch]

On Sun, Dec 8, 2024 at 2:42 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu,  5 Dec 2024 17:13:37 +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>
> Hi Lothar,
>
> One trivial comment inline.
>
> Jonathan
>
> > ---
> >  drivers/iio/accel/adxl345_core.c | 44 ++++++++++++++++++++++----------
> >  1 file changed, 31 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > index 98ff37271f1..1d020b0d79c 100644
> > --- a/drivers/iio/accel/adxl345_core.c
> > +++ b/drivers/iio/accel/adxl345_core.c
> > @@ -138,6 +138,34 @@ static int adxl345_write_raw_get_fmt(struct iio_dev *indio_dev,
> >       }
> >  }
> >
> > +/**
> > + * adxl345_set_measure_en() - Enable and disable measuring.
> > + *
> > + * @st: The device data.
> > + * @en: Enable measurements, else standby mode.
> > + *
> > + * For lowest power operation, standby mode can be used. In standby mode,
> > + * current consumption is supposed to be reduced to 0.1uA (typical). In this
> > + * mode no measurements are made. Placing the device into standby mode
> > + * preserves the contents of FIFO.
> > + *
> > + * Return: Returns 0 if successful, or a negative error value.
> > + */
> > +static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
> > +{
> > +     unsigned int val = 0;
> > +
> > +     val = (en) ? ADXL345_POWER_CTL_MEASURE : ADXL345_POWER_CTL_STANDBY;
> > +     return regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
> > +}
> > +
> > +static void adxl345_powerdown(void *ptr)
> > +{
> > +     struct adxl345_state *st = ptr;
> > +
> > +     adxl345_set_measure_en(st, false);
> > +}
> > +
> >  static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
> >  "0.09765625 0.1953125 0.390625 0.78125 1.5625 3.125 6.25 12.5 25 50 100 200 400 800 1600 3200"
> >  );
> > @@ -158,16 +186,6 @@ static const struct iio_info adxl345_info = {
> >       .write_raw_get_fmt      = adxl345_write_raw_get_fmt,
> >  };
> >
> > -static int adxl345_powerup(void *regmap)
> > -{
> > -     return regmap_write(regmap, ADXL345_REG_POWER_CTL, ADXL345_POWER_CTL_MEASURE);
> > -}
> > -
> > -static void adxl345_powerdown(void *regmap)
> > -{
> > -     regmap_write(regmap, ADXL345_REG_POWER_CTL, ADXL345_POWER_CTL_STANDBY);
> > -}
> > -
> >  /**
> >   * adxl345_core_probe() - Probe and setup for the accelerometer.
> >   * @dev:     Driver model representation of the device
> > @@ -236,13 +254,13 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> >                                    regval, ADXL345_DEVID);
> >
> >       /* Enable measurement mode */
> > -     ret = adxl345_powerup(st->regmap);
> > +     ret = adxl345_set_measure_en(st, true);
> >       if (ret < 0)
> >               return dev_err_probe(dev, ret, "Failed to enable measurement mode\n");
> >
> > -     ret = devm_add_action_or_reset(dev, adxl345_powerdown, st->regmap);
> > +     ret = devm_add_action_or_reset(dev, adxl345_powerdown, st);
> >       if (ret < 0)
> > -             return ret;
> > +             return dev_err_probe(dev, ret, "Failed to add action or reset\n");
> You will never see that message, though arguably that's an implementation detail.
>
> The only error that devm_add_action_or_reset() returns is -ENOMEM;
> dev_err_probe() doesn't print on -ENOMEM because enough screaming occurs at
> other layers.
>
> I normally don't bother commenting on this one if it's introduced as one of
> many messages, but here you are adding just this one so I have commented.

Thank you so much. I highly appreciate all the time you spend on
reviewing and giving
feedback!! For me there is no hurry with this driver. I'd prefer to go
a bit more picky
feedback, according to your patience. Hopefully, I will learn for
future patches. Already
when I take a look on the way from v1 of just this driver. Awesome!

>
> >
> >       return devm_iio_device_register(dev, indio_dev);
> >  }
>

Re: [PATCH v5 10/10] iio: accel: adxl345: add FIFO with watermark events

[Lothar Rubusch]

Hi Jonathan,
Find my answers down below.
Best,
L

On Sun, Dec 8, 2024 at 5:34 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu,  5 Dec 2024 17:13:43 +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>
> Various comments inline.
>
> Thanks,
>
> Jonathan
>
> > ---
> >  drivers/iio/accel/adxl345_core.c | 300 +++++++++++++++++++++++++++++++
> >  1 file changed, 300 insertions(+)
> >
> > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > index 3067a70c54e..58ed82d66dc 100644
> > --- a/drivers/iio/accel/adxl345_core.c
> > +++ b/drivers/iio/accel/adxl345_core.c
> > @@ -15,15 +15,28 @@
> >
> >  #include <linux/iio/iio.h>
> >  #include <linux/iio/sysfs.h>
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/events.h>
>
> I'm not seeing any use of this header yet.  Bring it in when you need it.
>
> > +#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
> > +
> >  struct adxl345_state {
> >       int irq;
> >       const struct adxl345_chip_info *info;
> >       struct regmap *regmap;
> > +     __le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE];
> >       bool fifo_delay; /* delay: delay is needed for SPI */
> >       u8 intio;
> > +     u8 int_map;
> > +     u8 watermark;
> > +     u8 fifo_mode;
> >  };
> >
> >  #define ADXL345_CHANNEL(index, reg, axis) {                                  \
> > @@ -36,6 +49,13 @@ struct adxl345_state {
> >       .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,                      \
> > +             .shift = 0,                             \
>
> No need to specify shift of 0. It's the 'obvious' default and C will set it to 0
> for you anyway.
>
> > +             .endianness = IIO_LE,                   \
> > +     },                                              \
> >  }
> >
> >  enum adxl345_chans {
> > @@ -48,6 +68,25 @@ static const struct iio_chan_spec adxl345_channels[] = {
> >       ADXL345_CHANNEL(2, chan_z, Z),
> >  };
> >
> > +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
>
>         /*
>          * Any bits....
>
>
> > +      * 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 = 0xFF & (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);
> > +}
>
>
> > +/**
> > + * 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 0x3f & regval;
> FIELD_GET() for all stuff like this with appropriate #define for the mask.
>
> > +}
> > +
> > +/**
> > + * 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,
> > + * a fifo_entry has 3 elements for X, Y and Z direction of 2 bytes each.
> > + *
> > + * 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 at once.
>
> Doesn't match the code which is reading just one register lots of times.

This is one of my painpoints, regmap_noinc_read() worked here, for a
linewise reading of FIFO elements. Say, I could read X0, X1, Y0,... Z1
in one command. Also, I tried here regmap_bulk_read(). At all, I find
this solution is working, but I'm not sure if there is not a total
differnt way to do the read out.

> > + *
> > + * Return: 0 or error value.
> > + */
> > +static int adxl345_fifo_transfer(struct adxl345_state *st, int samples)
> > +{
> > +     size_t count;
> > +     int i, ret;
> > +
> > +     count = sizeof(st->fifo_buf[0]) * ADXL345_DIRS;
> > +     for (i = 0; i < samples; i++) {
> > +             ret = regmap_noinc_read(st->regmap, ADXL345_REG_XYZ_BASE,
> > +                             st->fifo_buf + (i * count / 2), count);
> > +             if (ret < 0)
> > +                     return ret;
>
> This is where I'd expect to see the delay mentioned below.

I agree with the delay, I'll move it.

> > +     }
> > +     return ret;
> > +}
> > +
>
> > +static int adxl345_buffer_predisable(struct iio_dev *indio_dev)
> > +{
> > +     struct adxl345_state *st = iio_priv(indio_dev);
> > +     int ret;
> > +
> > +     st->int_map = 0x00;
> > +
> > +     ret = adxl345_set_interrupts(st);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     st->fifo_mode = ADXL345_FIFO_BYPASS;
> > +     return adxl345_set_fifo(st);
> I'd normally expect order in predisable to be reverse of that in postenable.
> Why isn't it?  That is why is the set_fifo here after set_interrupts and
> not before it.  Add a comment.
>
> > +}
> > +
> > +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 (0xff & regval);
>
> No brackets needed.
>
> > +}
> > +
> > +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) {
> > +             /*
> > +              * 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);
>
> I'm not following why a delay here helps.  At this point you're read masses of
> data from the fifo without the delays you mention.
>
> > +
> > +             iio_push_to_buffers(indio_dev, &st->fifo_buf[i]);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +/**
> > + * adxl345_event_handler() - Handle events of the ADXL345.
>
> Up to you but...
> Given it's an IIO driver and that we have a very specific meaning
> for events, maybe just call this adxl345_irq_handler()?
>
> > + * @irq: The irq being handled.
> > + * @p: The struct iio_device pointer for the device.
> > + *
> > + * Return: The interrupt was handled.
> > + */
> > +static irqreturn_t adxl345_event_handler(int irq, void *p)
> > +{
> > +     struct iio_dev *indio_dev = p;
> > +     struct adxl345_state *st = iio_priv(indio_dev);
> > +     u8 int_stat;
> > +     int samples;
> > +
> > +     int_stat = adxl345_get_status(st);
> > +     if (int_stat < 0)
> > +             return IRQ_NONE;
> > +
> > +     if (int_stat == 0x0)
> Doesn't this correspond to 'not our interrupt'?
> If that's the case return IRQ_NONE is the right way to go and not reset the
> interrupt.  You have registered it as maybe shared, and if it is, then this
> is a common thing to happen as interrupt from another device.
>

Here I see actually
+     int_stat = adxl345_get_status(st);
+     if (int_stat < 0)
+             return IRQ_NONE; // a bus error, reading register not possible
...and then...
+     if (int_stat == 0x0)
+             // interrupt sources were 0, so IRQ not from our sensor

I'm unsure if the first IRQ_NONE here is actually correct. I mean, if
the bus is not working,
actually any IRQ usage should be considered broken. Is there a way to
break out of measuring?

> > +             goto err;
> > +
> > +     if (int_stat & ADXL345_INT_OVERRUN)
> > +             goto err;
> > +
> > +     if (int_stat & (ADXL345_INT_DATA_READY | ADXL345_INT_WATERMARK)) {
>
> I think you only ever enable the INT_WATERMARK?  If so does it
> make sense to check for DATA_READY as well?
>

Watermark comes usually with data ready or overrun. I guess for the
FIFO watermark, just evaluating watermark is probably sufficient. For
other events, it then might be notification w/ a data ready set.
Probably better to introduce data ready when it's actually really
needed?

> > +             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,
> >  };
> >
> >  /**
> > @@ -222,6 +499,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> >       unsigned int data_format_mask = (ADXL345_DATA_FORMAT_RANGE |
> >                                        ADXL345_DATA_FORMAT_FULL_RES |
> >                                        ADXL345_DATA_FORMAT_SELF_TEST);
> > +     u8 fifo_ctl;
> >       int ret;
> >
> >       indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> > @@ -293,6 +571,28 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> >       if (ret < 0)
> >               return dev_err_probe(dev, ret, "Failed to add action or reset\n");
> >
> > +     if (st->irq > 0) {
> > +             dev_dbg(dev, "initialize for FIFO_STREAM mode\n");
> > +
> > +             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_event_handler,
> > +                             IRQF_SHARED | IRQF_ONESHOT,
> > +                             indio_dev->name, indio_dev);
> > +             if (ret)
> > +                     return dev_err_probe(dev, ret, "Failed to setup triggered buffer\n");
> > +
> > +     } else {
> > +             dev_dbg(dev, "initialize for FIFO_BYPASS mode (fallback)\n");
> > +
> Given you haven't removed this code from elsewhere, was the driver relying
> on the defaults after reset before this patch?
>
> I don't mind having this branch as a form of documentation even if that's
> true but maybe add a note to the patch description.
>

I'm not sure if I get you correctly. The driver before only
implemented "BYPASS" mode. This was the case w/o a defined
interrupt-name. My intention now is to keep this behavior as fallback.
If no IRQ is around, i.e. interrupt + interrupt-name, the sensor
driver will operate the sensor in BYPASS mode.

I was interpreting this also as the "default behavior", you mentioned
in the dt-binding patch? Is this correct?

What do you mean when the driver is relying on the defaults after reset?

> > +             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 v5 02/10] iio: accel: adxl345: rename variable data to st

[Jonathan Cameron]

On Tue, 10 Dec 2024 18:31:57 +0100
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Hi,
> 
> On Sun, Dec 8, 2024 at 2:28 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Thu,  5 Dec 2024 17:13:35 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >  
> > > Rename the locally used variable data to st. The st refers to "state",
> > > representing the internal state of the driver object. Further it
> > > prepares the usage of an internal data pointer needed for the
> > > implementation of the sensor features.
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>  
> > Applied to the togreg branch of iio.git. Initially pushed out as testing
> > to let the bots take a look.  
> 
> Should I actually drop the "applied" patches in a v6? Or, may I keep them?

Rebase on top of my testing branch or just drop them. Either works.

> 
> I see that Dan Carpenters smatch now comes up with some issues. So, do
> the fixes go into v6 here, or better separate?
If for things I've picked up, then separate fixup at the beginning of
your v6.  I might squash them into original patches if I haven't pushed
out as togreg yet or they are particularly bad and I think worth potentially
work giving others a messy rebase.

Jonathan


> 
> Best,
> L

Re: [PATCH v5 00/10] iio: accel: adxl345: add FIFO operating with IRQ triggered watermark events

[Jonathan Cameron]

On Thu,  5 Dec 2024 17:13:33 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> 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>
> ---
> Although I tried to implement most of the requested changes, now the code
> is simplified and clearer, I still encounter some issues.
> 
> 1) Unsure if my way reading out the FIFO elements with a regmap_noinc_read()
>    is supposed to be like that. 

That is pretty standard looking.

>    TBH, isn't there a better way, say, to
>    configure the channel correctly and this is handled by the iio API
>    internally? As I understood from v2 there might be a way using
>    available data, also in iio_info I see now as buffer attributes an
>    available data field. Where can I find information how to use it or
>    did I get this wrong?

Could you give a reference for that. I'm not sure what you are referring to.
if you mean available_scan_masks then that is used to allow the core
to demux data if you have to read all channels out (or some subset) rather
than any combination.

We don't have a bulk put to the buffers. Have thought about it in the past
but it reality it's not that much efficient in the core because in many cases
we have to reorganize the data.




> 
> 2) Overrun handling: I'm trying to reset the FIFO and registers. Unsure,
>    if this is the correct dealing here.

It's a tricky corner. Sometimes people just drain the buffer on basis device
recovers, some other devices need a reset.  
There isn't a good way to communication it to userspace though.

> 
> 3) I can see the IRQs coming in, and with a `watch -n 0.1 iio_info` I can
>    see the correct fields changing. I tried the follwoing down below,
>    but the iio_readdev shows me the following result. I don't quite
>    understand if I still have an issue here, or if this is a calibration
>    thing?
> 
> # iio_info 
> Library version: 0.23 (git tag: v0.23)
> Compiled with backends: local xml ip usb
> IIO context created with local backend.
> Backend version: 0.23 (git tag: v0.23)
> Backend description string: Linux dut1138 6.6.21-lothar02 #3 SMP PREEMPT Wed Nov  6 21:21:14 UTC 2024 aarch64
> IIO context has 2 attributes:
> 	local,kernel: 6.6.21-lothar02
> 	uri: local:
> IIO context has 1 devices:
> 	iio:device0: adxl345 (buffer capable)
> 		3 channels found:
> 			accel_x:  (input, index: 0, format: le:s13/16>>0)
> 			4 channel-specific attributes found:
> 				attr  0: calibbias value: 0
> 				attr  1: raw value: -14                          <--- CHANGES
> 				attr  2: sampling_frequency value: 100.000000000
> 				attr  3: scale value: 0.038300
> 			accel_y:  (input, index: 1, format: le:s13/16>>0)
> 			4 channel-specific attributes found:
> 				attr  0: calibbias value: 0
> 				attr  1: raw value: 6                            <--- CHANGES
> 				attr  2: sampling_frequency value: 100.000000000
> 				attr  3: scale value: 0.038300
> 			accel_z:  (input, index: 2, format: le:s13/16>>0)
> 			4 channel-specific attributes found:
> 				attr  0: calibbias value: 0
> 				attr  1: raw value: 247                          <--- CHANGES
> 				attr  2: sampling_frequency value: 100.000000000
> 				attr  3: scale value: 0.038300
> 		2 device-specific attributes found:
> 				attr  0: sampling_frequency_available value: 0.09765625 0.1953125 0.390625 0.78125 1.5625 3.125 6.25 12.5 25 50 100 200 400 800 1600 3200
> 				attr  1: waiting_for_supplier value: 0
> 		3 buffer-specific attributes found:
> 				attr  0: data_available value: 13
> 				attr  1: direction value: in
> 				attr  2: watermark value: 15
> 		No trigger on this device
> 
>   Above I marked what keeps changing with "CHANGES", that's what I expect. Then with readdev
>   I obtain the following result.
> 
> # iio_attr -c adxl345
> dev 'adxl345', channel 'accel_x' (input, index: 0, format: le:s13/16>>0), found 4 channel-specific attributes
> dev 'adxl345', channel 'accel_y' (input, index: 1, format: le:s13/16>>0), found 4 channel-specific attributes
> dev 'adxl345', channel 'accel_z' (input, index: 2, format: le:s13/16>>0), found 4 channel-specific attributes
> # echo 1 > ./scan_elements/in_accel_x_en
> # echo 1 > ./scan_elements/in_accel_y_en
> # echo 1 > ./scan_elements/in_accel_z_en
> # echo 32 > ./buffer0/length
> # echo 15 > ./buffer0/watermark
> # echo 1 > ./buffer0/enable
> # iio_readdev -b 16 -s 21 adxl345 > samples.dat
> # hexdump -d ./samples.dat 
> 0000000   65523   00006   00248   65523   00005   00235   65522   00006
> 0000010   00248   65522   00006   00248   65521   00005   00247   65522
> 0000020   00007   00249   65523   00005   00249   65521   00006   00248
> 0000030   65522   00006   00248   65522   00006   00250   65522   00006
> 0000040   00249   65522   00005   00248   65523   00005   00248   65521
> 0000050   00007   00248   65521   00006   00250   65522   00005   00248
> 0000060   65521   00006   00248   65522   00007   00247   65522   00006
> 0000070   00248   65522   00006   00248   65521   00004   00250        
> 000007e
> 
>   Am I doing this actually correctly?

I'm sorry to say I'm guessing here don't use this tool.  I'm still stuck in using my own tooling
(mostly the stuff in the kernel tree in tools/iio) that predate anyone writing nice
userspace code :)

What do you think looks wrong?  You should have 3 16 bit values.
You haven't turned on the timestamp so they will be tightly packed
That hex dump looks plausible if it's storing the actual raw buffer data.

To convert that to real data you need to apply masking and scale factors.

Jonathan


> 
> ---
> 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 (10):
>   iio: accel: adxl345: refrase comment on probe
>   iio: accel: adxl345: rename variable data to st
>   iio: accel: adxl345: measure right-justified
>   iio: accel: adxl345: add function to switch measuring mode
>   iio: accel: adxl345: complete list of defines
>   dt-bindings: iio: accel: add interrupt-names
>   iio: accel: adxl345: introduce interrupt handling
>   iio: accel: adxl345: initialize FIFO delay value for SPI
>   iio: accel: adxl345: prepare channel for scan_index
>   iio: accel: adxl345: add FIFO with watermark events
> 
>  .../bindings/iio/accel/adi,adxl345.yaml       |   7 +
>  drivers/iio/accel/adxl345.h                   |  90 +++-
>  drivers/iio/accel/adxl345_core.c              | 427 ++++++++++++++++--
>  drivers/iio/accel/adxl345_i2c.c               |   2 +-
>  drivers/iio/accel/adxl345_spi.c               |   7 +-
>  5 files changed, 478 insertions(+), 55 deletions(-)
> 

Re: [PATCH v5 03/10] iio: accel: adxl345: measure right-justified

[Jonathan Cameron]

On Mon, 9 Dec 2024 23:18:45 +0100
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Dear IIO-ML, Hi Jonathan!
> 
> On Sun, Dec 8, 2024 at 2:35 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Thu,  5 Dec 2024 17:13:36 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >  
> > > Make measurements right-justified, since it is the default for the
> > > driver and sensor. By not setting the ADXL345_DATA_FORMAT_JUSTIFY bit,
> > > the data becomes right-judstified. This was the original setting, there
> > > is no reason to change it to left-justified, where right-justified
> > > simplifies working on the registers.
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>  
> >
> > I'm still confused by this one.  Does this change affect the data output
> > to userspace?  If seems like it definitely should. If it does we have
> > an ABI regression somewhere. Is it currently broken and wasn't at some
> > earlier stage, or is this the patch breaking things?  
> 
> No, it should not affect the userspace.
> 
> This setting opens the mask for regmap/update bits to allow for
> changing the data format.
> My point is rather, does it actually makes sense to allow to change
> the data format, since
> the driver will use just one format. The bit was never applied, it's
> just the mask here.
Ah.  Got it.

> 
> May I ask you, if you could also could give me a brief feedback to the
> three questions in
> the cover letter to this series?
Reading cover letters? Never! :)
Thanks for the heads up. I tend to skip straight past them unless
I am looking for something specific... oops.

> 
> I would really appreciate, since I'm still unsure if I actually
> verified everything correctly.
> From what I did about this bit, I removed and set the justified bit in
> STREAM and in
> BYPASSED mode (current mode), without any difference in the results in
> iio_info or
> iio_readdev. The numbers look generally odd to me, though. And, I'd
> rather like to ask
> to still wait with applying the patches, if this is ok for you? But,
> perhaps with the answers
> of the cover letter items, it could become clearer to me. I'm still
> about to measure and
> verify against the old and the input driver results as comparison.

I'd use the tools/iio tooling. It pretty prints channel data.  I suspect
there are tools in the set you are using that do that but I'm not the
person to ask.

Jonathan

> 
> Best,
> L
> 
> 
> > If it worked and currently doesn't send a fix.  If this changes a previously
> > working ABI then drop this patch.  Alternative being to fix up the scale
> > handling to incorporate this justification change.
> >
> > Jonathan
> >  
> > > ---
> > >  drivers/iio/accel/adxl345_core.c | 1 -
> > >  1 file changed, 1 deletion(-)
> > >
> > > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > > index 88df9547bd6..98ff37271f1 100644
> > > --- a/drivers/iio/accel/adxl345_core.c
> > > +++ b/drivers/iio/accel/adxl345_core.c
> > > @@ -184,7 +184,6 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> > >       struct iio_dev *indio_dev;
> > >       u32 regval;
> > >       unsigned int data_format_mask = (ADXL345_DATA_FORMAT_RANGE |
> > > -                                      ADXL345_DATA_FORMAT_JUSTIFY |
> > >                                        ADXL345_DATA_FORMAT_FULL_RES |
> > >                                        ADXL345_DATA_FORMAT_SELF_TEST);
> > >       int ret;  
> >  

Re: [PATCH v5 10/10] iio: accel: adxl345: add FIFO with watermark events

[Jonathan Cameron]

> > > +}
> > > +
> > > +/**
> > > + * 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,
> > > + * a fifo_entry has 3 elements for X, Y and Z direction of 2 bytes each.
> > > + *
> > > + * 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 at once.  
> >
> > Doesn't match the code which is reading just one register lots of times.  
> 
> This is one of my painpoints, regmap_noinc_read() worked here, for a
> linewise reading of FIFO elements. Say, I could read X0, X1, Y0,... Z1
> in one command. Also, I tried here regmap_bulk_read(). At all, I find
> this solution is working, but I'm not sure if there is not a total
> differnt way to do the read out.

A bulk read is defined as indexing through registers. Eg. ADDR, ADDR + 1, ADDR + 2
etc.  regmap_noinc_read() just keeps reading the same register, so is typically
used for fifos.

I opened the datasheet. It seems to say you need to read 3 registers repeatedly
rather than one 3 times as often.  There isn't a good way to do that sort of
sequenced read in one go.  So you will need a loop like you have, but it
should need a bulk read.  Curious it doesn't seem to...

Ah. Device auto increments for both SPI and I2C.  So in that case
the noinc_read and normal bulk read will actually issue the same thing and
as these are volatile registers it doesn't matter (it would if you were
caching the result as the data would end up cached in different places).

It will do the wrong thing though if you have an i2c controller that
is less capable and can't do large reads.  So you should definitely use
bulk_read not noinc.

Have you set available_scan_masks?  If not you want to do so as
per comment I made in the cover letter.



> > > + * @irq: The irq being handled.
> > > + * @p: The struct iio_device pointer for the device.
> > > + *
> > > + * Return: The interrupt was handled.
> > > + */
> > > +static irqreturn_t adxl345_event_handler(int irq, void *p)
> > > +{
> > > +     struct iio_dev *indio_dev = p;
> > > +     struct adxl345_state *st = iio_priv(indio_dev);
> > > +     u8 int_stat;
> > > +     int samples;
> > > +
> > > +     int_stat = adxl345_get_status(st);
> > > +     if (int_stat < 0)
> > > +             return IRQ_NONE;
> > > +
> > > +     if (int_stat == 0x0)  
> > Doesn't this correspond to 'not our interrupt'?
> > If that's the case return IRQ_NONE is the right way to go and not reset the
> > interrupt.  You have registered it as maybe shared, and if it is, then this
> > is a common thing to happen as interrupt from another device.
> >  
> 
> Here I see actually
> +     int_stat = adxl345_get_status(st);
> +     if (int_stat < 0)
> +             return IRQ_NONE; // a bus error, reading register not possible
> ...and then...
> +     if (int_stat == 0x0)
> +             // interrupt sources were 0, so IRQ not from our sensor
> 
> I'm unsure if the first IRQ_NONE here is actually correct. I mean, if
> the bus is not working,
> actually any IRQ usage should be considered broken. Is there a way to
> break out of measuring?
> 

It is a much debated thing on what you should return if you have no
idea if it is our interrupt or not.   There isn't really a right
answer.  If you get a lot of IRQ_NONE and no one else claims it eventually
the interrupt will be disabled (to break the interrupt storm freezing the
machine).


> > > +             goto err;
> > > +
> > > +     if (int_stat & ADXL345_INT_OVERRUN)
> > > +             goto err;
> > > +
> > > +     if (int_stat & (ADXL345_INT_DATA_READY | ADXL345_INT_WATERMARK)) {  
> >
> > I think you only ever enable the INT_WATERMARK?  If so does it
> > make sense to check for DATA_READY as well?
> >  
> 
> Watermark comes usually with data ready or overrun. I guess for the
> FIFO watermark, just evaluating watermark is probably sufficient. For
> other events, it then might be notification w/ a data ready set.
> Probably better to introduce data ready when it's actually really
> needed?

Yes.  That dataready is normally used when you are doing capture without
the fifo and want to read each sample - kind of same as a watermark depth
of 1, but less hardware turned on.  As such, may be no need to ever support it.

   return dev_err_probe(dev, ret, "Failed to add action or reset\n");
> > >
> > > +     if (st->irq > 0) {
> > > +             dev_dbg(dev, "initialize for FIFO_STREAM mode\n");
> > > +
> > > +             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_event_handler,
> > > +                             IRQF_SHARED | IRQF_ONESHOT,
> > > +                             indio_dev->name, indio_dev);
> > > +             if (ret)
> > > +                     return dev_err_probe(dev, ret, "Failed to setup triggered buffer\n");
> > > +
> > > +     } else {
> > > +             dev_dbg(dev, "initialize for FIFO_BYPASS mode (fallback)\n");
> > > +  
> > Given you haven't removed this code from elsewhere, was the driver relying
> > on the defaults after reset before this patch?
> >
> > I don't mind having this branch as a form of documentation even if that's
> > true but maybe add a note to the patch description.
> >  
> 
> I'm not sure if I get you correctly. The driver before only
> implemented "BYPASS" mode. This was the case w/o a defined
> interrupt-name. My intention now is to keep this behavior as fallback.
> If no IRQ is around, i.e. interrupt + interrupt-name, the sensor
> driver will operate the sensor in BYPASS mode.
> 
> I was interpreting this also as the "default behavior", you mentioned
> in the dt-binding patch? Is this correct?
> 
> What do you mean when the driver is relying on the defaults after reset?

The driver worked without irq before now.  Which is same as this path.
So how was the register written here being configured correctly before
this patch?  I'm guessing it wasn't and that the value written here
is the default on power up.

Either that or I'm miss understanding what this branch of the if / else is
about.

Jonathan

> 
> > > +             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 v5 10/10] iio: accel: adxl345: add FIFO with watermark events

[Jonathan Cameron]

On Tue, 10 Dec 2024 11:47:37 +0300
Dan Carpenter <dan.carpenter@linaro.org> wrote:

> Hi Lothar,
> 
> kernel test robot noticed the following build warnings:

Ah. This was the report you were referring to I guess.  Just fix these in v6 version
of this patch.

Thanks,

Jonathan

> 
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Lothar-Rubusch/iio-accel-adxl345-refrase-comment-on-probe/20241206-011802
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
> patch link:    https://lore.kernel.org/r/20241205171343.308963-11-l.rubusch%40gmail.com
> patch subject: [PATCH v5 10/10] iio: accel: adxl345: add FIFO with watermark events
> config: nios2-randconfig-r072-20241210 (https://download.01.org/0day-ci/archive/20241210/202412101132.Kj6R6i3h-lkp@intel.com/config)
> compiler: nios2-linux-gcc (GCC) 14.2.0
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> | Closes: https://lore.kernel.org/r/202412101132.Kj6R6i3h-lkp@intel.com/
> 
> smatch warnings:
> drivers/iio/accel/adxl345_core.c:441 adxl345_event_handler() warn: unsigned 'int_stat' is never less than zero.
> 
> vim +/ret +321 drivers/iio/accel/adxl345_core.c
> 
> 55d2386488598bb Lothar Rubusch 2024-12-05  433  static irqreturn_t adxl345_event_handler(int irq, void *p)
> 55d2386488598bb Lothar Rubusch 2024-12-05  434  {
> 55d2386488598bb Lothar Rubusch 2024-12-05  435  	struct iio_dev *indio_dev = p;
> 55d2386488598bb Lothar Rubusch 2024-12-05  436  	struct adxl345_state *st = iio_priv(indio_dev);
> 55d2386488598bb Lothar Rubusch 2024-12-05  437  	u8 int_stat;
>                                                         ^^^^^^^^^^^
> 
> 55d2386488598bb Lothar Rubusch 2024-12-05  438  	int samples;
> 55d2386488598bb Lothar Rubusch 2024-12-05  439  
> 55d2386488598bb Lothar Rubusch 2024-12-05  440  	int_stat = adxl345_get_status(st);
> 55d2386488598bb Lothar Rubusch 2024-12-05 @441  	if (int_stat < 0)
>                                                             ^^^^^^^^^^^^
> signedness bug
> 
> 55d2386488598bb Lothar Rubusch 2024-12-05  442  		return IRQ_NONE;
> 55d2386488598bb Lothar Rubusch 2024-12-05  443  
> 55d2386488598bb Lothar Rubusch 2024-12-05  444  	if (int_stat == 0x0)
> 55d2386488598bb Lothar Rubusch 2024-12-05  445  		goto err;
> 55d2386488598bb Lothar Rubusch 2024-12-05  446  
> 55d2386488598bb Lothar Rubusch 2024-12-05  447  	if (int_stat & ADXL345_INT_OVERRUN)
> 55d2386488598bb Lothar Rubusch 2024-12-05  448  		goto err;
> 55d2386488598bb Lothar Rubusch 2024-12-05  449  
> 55d2386488598bb Lothar Rubusch 2024-12-05  450  	if (int_stat & (ADXL345_INT_DATA_READY | ADXL345_INT_WATERMARK)) {
> 55d2386488598bb Lothar Rubusch 2024-12-05  451  		samples = adxl345_get_samples(st);
> 55d2386488598bb Lothar Rubusch 2024-12-05  452  		if (samples < 0)
> 55d2386488598bb Lothar Rubusch 2024-12-05  453  			goto err;
> 55d2386488598bb Lothar Rubusch 2024-12-05  454  
> 55d2386488598bb Lothar Rubusch 2024-12-05  455  		if (adxl345_fifo_push(indio_dev, samples) < 0)
> 55d2386488598bb Lothar Rubusch 2024-12-05  456  			goto err;
> 55d2386488598bb Lothar Rubusch 2024-12-05  457  
> 55d2386488598bb Lothar Rubusch 2024-12-05  458  	}
> 55d2386488598bb Lothar Rubusch 2024-12-05  459  	return IRQ_HANDLED;
> 55d2386488598bb Lothar Rubusch 2024-12-05  460  
> 55d2386488598bb Lothar Rubusch 2024-12-05  461  err:
> 55d2386488598bb Lothar Rubusch 2024-12-05  462  	adxl345_fifo_reset(st);
> 55d2386488598bb Lothar Rubusch 2024-12-05  463  
> 55d2386488598bb Lothar Rubusch 2024-12-05  464  	return IRQ_HANDLED;
> 55d2386488598bb Lothar Rubusch 2024-12-05  465  }
> 

Re: [PATCH v5 10/10] iio: accel: adxl345: add FIFO with watermark events

[Lothar Rubusch]

Hello Jonathan,
Thank you again. This brought several pieces together for me.
See answeres down below.
Best,
L


On Wed, Dec 11, 2024 at 8:14 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
>
> > > > +}
> > > > +
> > > > +/**
> > > > + * 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,
> > > > + * a fifo_entry has 3 elements for X, Y and Z direction of 2 bytes each.
> > > > + *
> > > > + * 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 at once.
> > >
> > > Doesn't match the code which is reading just one register lots of times.
> >
> > This is one of my painpoints, regmap_noinc_read() worked here, for a
> > linewise reading of FIFO elements. Say, I could read X0, X1, Y0,... Z1
> > in one command. Also, I tried here regmap_bulk_read(). At all, I find
> > this solution is working, but I'm not sure if there is not a total
> > differnt way to do the read out.
>
> A bulk read is defined as indexing through registers. Eg. ADDR, ADDR + 1, ADDR + 2
> etc.  regmap_noinc_read() just keeps reading the same register, so is typically
> used for fifos.
>
> I opened the datasheet. It seems to say you need to read 3 registers repeatedly
> rather than one 3 times as often.  There isn't a good way to do that sort of
> sequenced read in one go.  So you will need a loop like you have, but it
> should need a bulk read.  Curious it doesn't seem to...
>
> Ah. Device auto increments for both SPI and I2C.  So in that case
> the noinc_read and normal bulk read will actually issue the same thing and
> as these are volatile registers it doesn't matter (it would if you were
> caching the result as the data would end up cached in different places).
>
> It will do the wrong thing though if you have an i2c controller that
> is less capable and can't do large reads.  So you should definitely use
> bulk_read not noinc.
>
> Have you set available_scan_masks?  If not you want to do so as
> per comment I made in the cover letter.
>

For v6 I'll setup available_scan_masks, as I understand its usage. I'm
not sure if I understood what it actually does. I can see it needs the
channel indexes available, but I'll need to read a bit more code.
Hopefully for now it'll be sufficient.

>
> > > > + * @irq: The irq being handled.
> > > > + * @p: The struct iio_device pointer for the device.
> > > > + *
> > > > + * Return: The interrupt was handled.
> > > > + */
> > > > +static irqreturn_t adxl345_event_handler(int irq, void *p)
> > > > +{
> > > > +     struct iio_dev *indio_dev = p;
> > > > +     struct adxl345_state *st = iio_priv(indio_dev);
> > > > +     u8 int_stat;
> > > > +     int samples;
> > > > +
> > > > +     int_stat = adxl345_get_status(st);
> > > > +     if (int_stat < 0)
> > > > +             return IRQ_NONE;
> > > > +
> > > > +     if (int_stat == 0x0)
> > > Doesn't this correspond to 'not our interrupt'?
> > > If that's the case return IRQ_NONE is the right way to go and not reset the
> > > interrupt.  You have registered it as maybe shared, and if it is, then this
> > > is a common thing to happen as interrupt from another device.
> > >
> >
> > Here I see actually
> > +     int_stat = adxl345_get_status(st);
> > +     if (int_stat < 0)
> > +             return IRQ_NONE; // a bus error, reading register not possible
> > ...and then...
> > +     if (int_stat == 0x0)
> > +             // interrupt sources were 0, so IRQ not from our sensor
> >
> > I'm unsure if the first IRQ_NONE here is actually correct. I mean, if
> > the bus is not working,
> > actually any IRQ usage should be considered broken. Is there a way to
> > break out of measuring?
> >

Here actually, I merged them, when returning IRQ_NONE in both
statements. I mean, if the bus is broken, the sensor driver then is
anyway not operational anymore. It cannot process interrupts anymore
be it from someone else or own ones.

When the interrupt source register was 0, then the interrupt was not
ours for sure. As I understand, if such happens, there will be no more
IRQs anyway as long as interrupt source reg and fifo are not cleaned
up.

> It is a much debated thing on what you should return if you have no
> idea if it is our interrupt or not.   There isn't really a right
> answer.  If you get a lot of IRQ_NONE and no one else claims it eventually
> the interrupt will be disabled (to break the interrupt storm freezing the
> machine).
>
>
> > > > +             goto err;
> > > > +
> > > > +     if (int_stat & ADXL345_INT_OVERRUN)
> > > > +             goto err;
> > > > +
> > > > +     if (int_stat & (ADXL345_INT_DATA_READY | ADXL345_INT_WATERMARK)) {
> > >
> > > I think you only ever enable the INT_WATERMARK?  If so does it
> > > make sense to check for DATA_READY as well?
> > >
> >
> > Watermark comes usually with data ready or overrun. I guess for the
> > FIFO watermark, just evaluating watermark is probably sufficient. For
> > other events, it then might be notification w/ a data ready set.
> > Probably better to introduce data ready when it's actually really
> > needed?
>
> Yes.  That dataready is normally used when you are doing capture without
> the fifo and want to read each sample - kind of same as a watermark depth
> of 1, but less hardware turned on.  As such, may be no need to ever support it.
>
>    return dev_err_probe(dev, ret, "Failed to add action or reset\n");
> > > >
> > > > +     if (st->irq > 0) {
> > > > +             dev_dbg(dev, "initialize for FIFO_STREAM mode\n");
> > > > +
> > > > +             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_event_handler,
> > > > +                             IRQF_SHARED | IRQF_ONESHOT,
> > > > +                             indio_dev->name, indio_dev);
> > > > +             if (ret)
> > > > +                     return dev_err_probe(dev, ret, "Failed to setup triggered buffer\n");
> > > > +
> > > > +     } else {
> > > > +             dev_dbg(dev, "initialize for FIFO_BYPASS mode (fallback)\n");
> > > > +
> > > Given you haven't removed this code from elsewhere, was the driver relying
> > > on the defaults after reset before this patch?
> > >
> > > I don't mind having this branch as a form of documentation even if that's
> > > true but maybe add a note to the patch description.
> > >
> >
> > I'm not sure if I get you correctly. The driver before only
> > implemented "BYPASS" mode. This was the case w/o a defined
> > interrupt-name. My intention now is to keep this behavior as fallback.
> > If no IRQ is around, i.e. interrupt + interrupt-name, the sensor
> > driver will operate the sensor in BYPASS mode.
> >
> > I was interpreting this also as the "default behavior", you mentioned
> > in the dt-binding patch? Is this correct?
> >
> > What do you mean when the driver is relying on the defaults after reset?
>
> The driver worked without irq before now.  Which is same as this path.
> So how was the register written here being configured correctly before
> this patch?  I'm guessing it wasn't and that the value written here
> is the default on power up.
>
> Either that or I'm miss understanding what this branch of the if / else is
> about.

Well, I'm pretty convinced the sensor before was operated in
FIFO_BYPASS mode. TBH I'm not sure now if this is default setting, or
explicitely configured. But I remember that also from somewhere in the
datasheet. That's why I used this as fallback.

> Jonathan
>
> >
> > > > +             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);
> > >
> >
>