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

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

[Lothar Rubusch]

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

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

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

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

-- 
2.39.5

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

[Lothar Rubusch]

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

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 88df9547b..b48bc838c 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
@@ -237,11 +255,11 @@ 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;
 
-- 
2.39.5

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

[Lothar Rubusch]

Remove interrupts from the list of required properties. The ADXL345
does not need interrupts for basic accelerometer functionality. The
sensor offers optional events which can be indicated on one of two
interrupt lines.

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

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

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

[Lothar Rubusch]

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

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

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

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

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

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

[Lothar Rubusch]

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

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index b48bc838c..fb3b45d99 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -11,15 +11,22 @@
 #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>
 
 #include "adxl345.h"
 
+#define ADXL345_INT_NONE		0xff
+#define ADXL345_INT1			0
+#define ADXL345_INT2			1
+
 struct adxl345_state {
+	int irq;
 	const struct adxl345_chip_info *info;
 	struct regmap *regmap;
+	u8 intio;
 };
 
 #define ADXL345_CHANNEL(index, axis) {					\
@@ -213,6 +220,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 
 	st = iio_priv(indio_dev);
 	st->regmap = regmap;
+
 	st->info = device_get_match_data(dev);
 	if (!st->info)
 		return -ENODEV;
@@ -263,6 +271,15 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 	if (ret < 0)
 		return ret;
 
+	st->intio = ADXL345_INT1;
+	st->irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT1");
+	if (st->irq < 0) {
+		st->intio = ADXL345_INT2;
+		st->irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT2");
+		if (st->irq < 0)
+			st->intio = ADXL345_INT_NONE;
+	}
+
 	return devm_iio_device_register(dev, indio_dev);
 }
 EXPORT_SYMBOL_NS_GPL(adxl345_core_probe, IIO_ADXL345);
-- 
2.39.5

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

[Lothar Rubusch]

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

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 drivers/iio/accel/adxl345.h      |  1 +
 drivers/iio/accel/adxl345_core.c | 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 3d5c8719d..6f39f16d3 100644
--- a/drivers/iio/accel/adxl345.h
+++ b/drivers/iio/accel/adxl345.h
@@ -62,6 +62,7 @@ struct adxl345_chip_info {
 };
 
 int adxl345_core_probe(struct device *dev, struct regmap *regmap,
+		       bool fifo_delay_default,
 		       int (*setup)(struct device*, struct regmap*));
 
 #endif /* _ADXL345_H_ */
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index fb3b45d99..fc4f89f22 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -26,6 +26,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;
 };
 
@@ -197,12 +198,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;
@@ -225,6 +235,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 4065b8f7c..28d997c58 100644
--- a/drivers/iio/accel/adxl345_i2c.c
+++ b/drivers/iio/accel/adxl345_i2c.c
@@ -27,7 +27,7 @@ static int adxl345_i2c_probe(struct i2c_client *client)
 	if (IS_ERR(regmap))
 		return dev_err_probe(&client->dev, PTR_ERR(regmap), "Error initializing regmap\n");
 
-	return adxl345_core_probe(&client->dev, regmap, NULL);
+	return adxl345_core_probe(&client->dev, regmap, false, NULL);
 }
 
 static const struct adxl345_chip_info adxl345_i2c_info = {
diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl345_spi.c
index 61fd9a6f5..e03915ece 100644
--- a/drivers/iio/accel/adxl345_spi.c
+++ b/drivers/iio/accel/adxl345_spi.c
@@ -12,6 +12,7 @@
 #include "adxl345.h"
 
 #define ADXL345_MAX_SPI_FREQ_HZ		5000000
+#define ADXL345_MAX_FREQ_NO_FIFO_DELAY	1500000
 
 static const struct regmap_config adxl345_spi_regmap_config = {
 	.reg_bits = 8,
@@ -28,6 +29,7 @@ static int adxl345_spi_setup(struct device *dev, struct regmap *regmap)
 static int adxl345_spi_probe(struct spi_device *spi)
 {
 	struct regmap *regmap;
+	bool needs_delay;
 
 	/* Bail out if max_speed_hz exceeds 5 MHz */
 	if (spi->max_speed_hz > ADXL345_MAX_SPI_FREQ_HZ)
@@ -38,10 +40,11 @@ static int adxl345_spi_probe(struct spi_device *spi)
 	if (IS_ERR(regmap))
 		return dev_err_probe(&spi->dev, PTR_ERR(regmap), "Error initializing regmap\n");
 
+	needs_delay = spi->max_speed_hz > ADXL345_MAX_FREQ_NO_FIFO_DELAY;
 	if (spi->mode & SPI_3WIRE)
-		return adxl345_core_probe(&spi->dev, regmap, adxl345_spi_setup);
+		return adxl345_core_probe(&spi->dev, regmap, needs_delay, adxl345_spi_setup);
 	else
-		return adxl345_core_probe(&spi->dev, regmap, NULL);
+		return adxl345_core_probe(&spi->dev, regmap, needs_delay, NULL);
 }
 
 static const struct adxl345_chip_info adxl345_spi_info = {
-- 
2.39.5

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

[Lothar Rubusch]

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

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

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

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

[Lothar Rubusch]

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

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

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

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

[Rob Herring (Arm)]

On Fri, 13 Dec 2024 21:19:05 +0000, Lothar Rubusch wrote:
> Add interrupt-names INT1 and INT2 for the two interrupt lines of the
> sensor.
> 
> When one of the two interrupt lines is connected, the interrupt as its
> interrupt-name, need to be declared in the devicetree. The driver then
> configures the sensor to indicate its events on either INT1 or INT2.
> 
> If no interrupt is configured, then no interrupt-name should be
> configured, and vice versa. In this case the sensor runs in FIFO BYPASS
> mode. This allows sensor measurements, but none of the sensor events.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  .../devicetree/bindings/iio/accel/adi,adxl345.yaml     | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml:45:17: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml:46:22: [error] string value is redundantly quoted with any quotes (quoted-strings)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241213211909.40896-4-l.rubusch@gmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

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

[Christophe JAILLET]

Le 13/12/2024 à 22:19, Lothar Rubusch a écrit :
> 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.

...

> +static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
> +{
> +	unsigned int val = 0;

Nitpick: useless init

> +
> +	val = (en) ? ADXL345_POWER_CTL_MEASURE : ADXL345_POWER_CTL_STANDBY;

Nitpick: useless () around en.

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

...

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

[Jonathan Cameron]

On Fri, 13 Dec 2024 21:19:03 +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>
Mostly in the interests of trimming down the queue of patches in flight
and because this one has been fine for a few versions without significant
comment.

Applied this patch to the togreg branch of iio.git and pushed out initially
as testing to let 0-day take a look.

Thanks

Jonathan

> ---
>  drivers/iio/accel/adxl345_core.c | 42 +++++++++++++++++++++++---------
>  1 file changed, 30 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 88df9547b..b48bc838c 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
> @@ -237,11 +255,11 @@ 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;
>  

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

[Jonathan Cameron]

On Fri, 13 Dec 2024 21:19:04 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Remove interrupts from the list of required properties. The ADXL345
> does not need interrupts for basic accelerometer functionality. The
> sensor offers optional events which can be indicated on one of two
> interrupt lines.
It is harmless but if you are spinning again, remove the last sentence.
It isn't really anything to do with the reasoning behind this patch
(well covered by the first sentence).  If everyone is fine with this
version I might just drop it whilst applying.

Jonathan

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

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

[Jonathan Cameron]

On Fri, 13 Dec 2024 21:19:05 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

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

Just to repeat what I sent in reply to v6 (well after you'd posted this).
Maybe we can maintain compatibility with the binding before this by adding
a default of INT1.

Then you'd need to drop the dependency on interrupt-names.

I'm not sure though if the checking of number of entries will work against
a default. Give it a go and see what happens :)

We are lucky that we can't have bindings in the wild assuming ordering
of the two interrupts due to the maxItems being set for interrupts.

It's a messy corner, perhaps we should just not bother in the binding,
but keep that default handling in the driver?

DT binding folk, what do you think the best way of handling this is?

Jonathan

> ---
>  .../devicetree/bindings/iio/accel/adi,adxl345.yaml     | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> index bc46ed00f..4f971035b 100644
> --- a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> +++ b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> @@ -37,6 +37,14 @@ properties:
>    interrupts:
>      maxItems: 1

That is not going to work if we have two interrupts specified.
It does at least mean we only need 

>  
> +  interrupt-names:
> +    items:
> +      - enum: [INT1, INT2]
> +
> +dependencies:
> +  interrupts: [ 'interrupt-names' ]
> +  interrupt-names: [ 'interrupts' ]
> +
>  required:
>    - compatible
>    - reg
> @@ -60,6 +68,7 @@ examples:
>              reg = <0x2a>;
>              interrupt-parent = <&gpio0>;
>              interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> +            interrupt-names = "INT1";
>          };
>      };
>    - |
> @@ -78,5 +87,6 @@ examples:
>              spi-cpha;
>              interrupt-parent = <&gpio0>;
>              interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> +            interrupt-names = "INT2";
>          };
>      };

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

[Jonathan Cameron]

On Fri, 13 Dec 2024 21:19:06 +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 | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index b48bc838c..fb3b45d99 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -11,15 +11,22 @@
>  #include <linux/property.h>
>  #include <linux/regmap.h>
>  #include <linux/units.h>
> +#include <linux/interrupt.h>

Keep to local style. Headers in alphabetical order (with IIO ones
separate obviously!)

>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
>  
>  #include "adxl345.h"
>  
> +#define ADXL345_INT_NONE		0xff
> +#define ADXL345_INT1			0
> +#define ADXL345_INT2			1
> +
>  struct adxl345_state {
> +	int irq;

Whilst it doesn't really matter. I'm not seeing any logic in having this as first
element and intio as last.  Might as well put them both at the end.

>  	const struct adxl345_chip_info *info;
>  	struct regmap *regmap;
> +	u8 intio;
>  };
>  
>  #define ADXL345_CHANNEL(index, axis) {					\
> @@ -213,6 +220,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  
>  	st = iio_priv(indio_dev);
>  	st->regmap = regmap;
> +
Check patches for unrelated changes like this and drop them as they are noise.
If you want to tidy this sort of whitespace up, separate patch.

>  	st->info = device_get_match_data(dev);
>  	if (!st->info)
>  		return -ENODEV;
> @@ -263,6 +271,15 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  	if (ret < 0)
>  		return ret;
>  
> +	st->intio = ADXL345_INT1;
> +	st->irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT1");
> +	if (st->irq < 0) {
> +		st->intio = ADXL345_INT2;
> +		st->irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT2");
> +		if (st->irq < 0)
> +			st->intio = ADXL345_INT_NONE;

As in the DT binding, maybe we can fall back to an assumption of default.
So if interrupt names missing we assume INT1.

> +	}
> +
>  	return devm_iio_device_register(dev, indio_dev);
>  }
>  EXPORT_SYMBOL_NS_GPL(adxl345_core_probe, IIO_ADXL345);

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

[Jonathan Cameron]

On Fri, 13 Dec 2024 21:19:08 +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>

I missed a theoretical bug around the dma buffer in previous reviews.
A few other minor things inline.

Thanks,

Jonathan

>  /*
>   * In full-resolution mode, scale factor is maintained at ~4 mg/LSB
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index fc4f89f22..e31a7cb3f 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -15,9 +15,17 @@
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/kfifo_buf.h>
>  
>  #include "adxl345.h"
>  
> +#define ADXL345_FIFO_BYPASS	0
> +#define ADXL345_FIFO_FIFO	1
> +#define ADXL345_FIFO_STREAM	2
> +
> +#define ADXL345_DIRS 3
> +
>  #define ADXL345_INT_NONE		0xff
>  #define ADXL345_INT1			0
>  #define ADXL345_INT2			1
> @@ -26,27 +34,68 @@ struct adxl345_state {
>  	int irq;
>  	const struct adxl345_chip_info *info;
>  	struct regmap *regmap;
> +	__le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE + 1];

Ah.  The old corner case (sorry I missed this in earlier reviews!)

In theory at least regmap makes no additional guarantees on how it uses buffers
on top of those provided by the underlying busses.
So we need to treat bulk reads the same way we do for those buses.
That means we need to allow for direct DMA writes into the buffers
we pass to bulk read.  For that to be safe on all architectures (and not
accidentally overwrite stuff in the same cacheline) we need to use
what is known as a DMA safe buffer.
Long ago we contrived the data layout in IIO to make that easy to
do.  Just move it down to the end of this structure as...


>  	bool fifo_delay; /* delay: delay is needed for SPI */
>  	u8 intio;
> +	u8 int_map;
> +	u8 watermark;
> +	u8 fifo_mode;
this
	__le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE + 1] __aligned(IIO_DMA_MINALIGN);

This structure already has the appropriate alignment (there is padding inserted
in the allocation to make that true) so by doing this for the element we
ask the compiler to make sure it adds sufficient padding before this element
to ensure nothing else is in the same cacheline (if required by a particular
architecture).   C also guarantees that the structure itself is large enough
to ensure padding is added after this element such that the overall structure
size is a multiple of it's most aligned element (this one).
Thus we end up with the buffer in it's own cacheline and none of the mess
of non coherent DMA can cause us problems. 

If interested in learning more look for Wolfram's talk at ELCE a number
of years back on trying to do DMA into the buffers passed to I2C calls.

The 'in theory' bit is because last time I checked regmap is always bounce
buffering the data but there are obvious optimizations that could be made
so it didn't bounce. A long back we asked the maintainer about this and he
said definitely don't assume it won't use the buffers directly.

Note on arm64 for instance there is magic code that bounces all small
DMA transfers, but that is not enabled on all architectures yet.

>  };

>  
> +static const unsigned long adxl345_scan_masks[] = {
> +	BIT(chan_x) | BIT(chan_y) | BIT(chan_z),
> +	0,

Trivial but drop that trailing comma. It's a terminator so we don't want to make it
easy for anyone to accidentally put anything after it!

> +};

>  static int adxl345_read_raw(struct iio_dev *indio_dev,
>  			    struct iio_chan_spec const *chan,
>  			    int *val, int *val2, long mask)
> @@ -132,6 +181,25 @@ 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 > ADXL345_FIFO_SIZE)
> +		value = ADXL345_FIFO_SIZE - 1;

	value = min(value, AD345_FIFO_SIZE - 1);

Shorter and maybe a tiny bit more readable. (trivial comment!)

> +
> +	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;
> +}



>  /**
> @@ -222,6 +499,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  					 ADXL345_DATA_FORMAT_JUSTIFY |
>  					 ADXL345_DATA_FORMAT_FULL_RES |
>  					 ADXL345_DATA_FORMAT_SELF_TEST);
> +	u8 fifo_ctl;

Might as well make this declaration local to where it's used.

>  	int ret;
>  
>  	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> @@ -242,6 +520,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->channels = adxl345_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(adxl345_channels);
> +	indio_dev->available_scan_masks = adxl345_scan_masks;
>  
>  	if (setup) {
>  		/* Perform optional initial bus specific configuration */
> @@ -292,6 +571,25 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  			st->intio = ADXL345_INT_NONE;
>  	}
>  
> +	if (st->intio != ADXL345_INT_NONE) {
> +		/* FIFO_STREAM mode is going to be activated later */
> +		ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, &adxl345_buffer_ops);
> +		if (ret)
> +			return ret;
> +
> +		ret = devm_request_threaded_irq(dev, st->irq, NULL, &adxl345_irq_handler,
> +				IRQF_SHARED | IRQF_ONESHOT,
> +				indio_dev->name, indio_dev);
		ret = devm_request_threaded_irq(dev, st->irq, NULL,
						&adxl345_irq_handler,
						IRQF_SHARED | IRQF_ONESHOT,
						indio_dev->name, indio_dev);

All under 80 chars (still the preference when no reason to do otherwise) and
aligned with opening bracket which is preferred kernel style when there
is no reason to do otherwise.

If you weren't going to be doing a v8 anyway I'd just tweak this whilst applying
but seeing as you are... :)


> +		if (ret)
> +			return ret;
> +	} else {
> +		/* FIFO_BYPASS mode */
> +		fifo_ctl = ADXL345_FIFO_CTL_MODE(ADXL345_FIFO_BYPASS);
> +		ret = regmap_write(st->regmap, ADXL345_REG_FIFO_CTL, fifo_ctl);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	return devm_iio_device_register(dev, indio_dev);
>  }
>  EXPORT_SYMBOL_NS_GPL(adxl345_core_probe, IIO_ADXL345);

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

[Jonathan Cameron]

On Fri, 13 Dec 2024 21:19:09 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

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

That makes sense for now, but longer term I'd attempt to restrict the scope
of these by moving them to the top of core.c

The two bus drivers don't use any of them that I can immediately spot
and if they do it is likely to be very few.

That may be a good first patch for your next series.

Jonathan


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

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

[Lothar Rubusch]

On Sat, Dec 14, 2024 at 12:33 PM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
>
> Le 13/12/2024 à 22:19, Lothar Rubusch a écrit :
> > 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.
>
> ...
>
> > +static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
> > +{
> > +     unsigned int val = 0;
>
> Nitpick: useless init
>
> > +
> > +     val = (en) ? ADXL345_POWER_CTL_MEASURE : ADXL345_POWER_CTL_STANDBY;
>
> Nitpick: useless () around en.

Thank you for pointing out. I agree. This is better just in one line,
initialization directly and no parens. Anyway, since I already can see
the patch on the iio branch, I'll probably better leave it as is for
now (?).

Question: Since I'm currently about to build up tooling for linters
and static checkers. I'm doing checkpatch. I'm running Dan's smatch,
now. I'm correcting indention/spaces/tabs by some emacs settings.
DT/bindings I was doing wrong, and just figured out why after
submitting the last patch (...).

What is the best approach to find such kind of nitpick issues automatically?

Best,
L

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

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

[Lothar Rubusch]

On Sat, Dec 14, 2024 at 1:02 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri, 13 Dec 2024 21:19:03 +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>
> Mostly in the interests of trimming down the queue of patches in flight
> and because this one has been fine for a few versions without significant
> comment.
>
> Applied this patch to the togreg branch of iio.git and pushed out initially
> as testing to let 0-day take a look.
>

Question here: you applied this patch now to the iio branch. Now,
Christophe Jaillet pointed still something out that could be improved,
the function could be shortened to, e.g.

+static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
+{
+     unsigned int val = en ? ADXL345_POWER_CTL_MEASURE :
ADXL345_POWER_CTL_STANDBY;
+
+     return regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
+}

Should I present an improved patch? Or, in case this was urgent, would
it require an additional patch/fix? What would be the way to deal with
such fixes immediately after "applied"?

Best,
L

> Thanks
>
> Jonathan
>
> > ---
> >  drivers/iio/accel/adxl345_core.c | 42 +++++++++++++++++++++++---------
> >  1 file changed, 30 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > index 88df9547b..b48bc838c 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
> > @@ -237,11 +255,11 @@ 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;
> >
>

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

[Jonathan Cameron]

On Sun, 15 Dec 2024 10:41:12 +0100
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> On Sat, Dec 14, 2024 at 1:02 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Fri, 13 Dec 2024 21:19:03 +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>  
> > Mostly in the interests of trimming down the queue of patches in flight
> > and because this one has been fine for a few versions without significant
> > comment.
> >
> > Applied this patch to the togreg branch of iio.git and pushed out initially
> > as testing to let 0-day take a look.
> >  
> 
> Question here: you applied this patch now to the iio branch. Now,
> Christophe Jaillet pointed still something out that could be improved,
> the function could be shortened to, e.g.
> 
> +static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
> +{
> +     unsigned int val = en ? ADXL345_POWER_CTL_MEASURE :
> ADXL345_POWER_CTL_STANDBY;
> +
> +     return regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
> +}
> 
> Should I present an improved patch? Or, in case this was urgent, would
> it require an additional patch/fix? What would be the way to deal with
> such fixes immediately after "applied"?
> 
Send me a patch on top. I may well squash it into the original (particularly
if you stick a note on that being sensible under the --- for the patch!)

There is always a window in which I'm happy to rebase (or pull a patch
if we get feedback needing bigger changes) because I typically give
at least half a week for 0-day to poke the tree before I make a potential
mess in linux-next.  Ultimately I'll rebase after that for a sufficiently
serious issue but for something like this I'll just apply the patch on top
once it's out for linux-next to pick up.

Jonathan

> Best,
> L
> 
> > Thanks
> >
> > Jonathan
> >  
> > > ---
> > >  drivers/iio/accel/adxl345_core.c | 42 +++++++++++++++++++++++---------
> > >  1 file changed, 30 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > > index 88df9547b..b48bc838c 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
> > > @@ -237,11 +255,11 @@ 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;
> > >  
> >  

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

[Conor Dooley]

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

On Sat, Dec 14, 2024 at 12:10:57PM +0000, Jonathan Cameron wrote:
> On Fri, 13 Dec 2024 21:19:05 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
> 
> > Add interrupt-names INT1 and INT2 for the two interrupt lines of the
> > sensor.
> > 
> > When one of the two interrupt lines is connected, the interrupt as its
> > interrupt-name, need to be declared in the devicetree. The driver then
> > configures the sensor to indicate its events on either INT1 or INT2.
> > 
> > If no interrupt is configured, then no interrupt-name should be
> > configured, and vice versa. In this case the sensor runs in FIFO BYPASS
> > mode. This allows sensor measurements, but none of the sensor events.
> > 
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> 
> Just to repeat what I sent in reply to v6 (well after you'd posted this).
> Maybe we can maintain compatibility with the binding before this by adding
> a default of INT1.

But can you make that assumption? If we did, and it's not universally
true, we break systems that had INT2 connected that previously worked.

> Then you'd need to drop the dependency on interrupt-names.
> 
> I'm not sure though if the checking of number of entries will work against
> a default. Give it a go and see what happens :)
> 
> We are lucky that we can't have bindings in the wild assuming ordering
> of the two interrupts due to the maxItems being set for interrupts.
> 
> It's a messy corner, perhaps we should just not bother in the binding,
> but keep that default handling in the driver?
> 
> DT binding folk, what do you think the best way of handling this is?

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

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

[Krzysztof Kozlowski]

On Fri, Dec 13, 2024 at 09:19:04PM +0000, Lothar Rubusch wrote:
> Remove interrupts from the list of required properties. The ADXL345
> does not need interrupts for basic accelerometer functionality. The
> sensor offers optional events which can be indicated on one of two
> interrupt lines.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml | 1 -
>  1 file changed, 1 deletion(-)

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

Best regards,
Krzysztof

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

[Jonathan Cameron]

On Sun, 15 Dec 2024 14:56:58 +0000
Conor Dooley <conor@kernel.org> wrote:

> On Sat, Dec 14, 2024 at 12:10:57PM +0000, Jonathan Cameron wrote:
> > On Fri, 13 Dec 2024 21:19:05 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >   
> > > Add interrupt-names INT1 and INT2 for the two interrupt lines of the
> > > sensor.
> > > 
> > > When one of the two interrupt lines is connected, the interrupt as its
> > > interrupt-name, need to be declared in the devicetree. The driver then
> > > configures the sensor to indicate its events on either INT1 or INT2.
> > > 
> > > If no interrupt is configured, then no interrupt-name should be
> > > configured, and vice versa. In this case the sensor runs in FIFO BYPASS
> > > mode. This allows sensor measurements, but none of the sensor events.
> > > 
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>  
> > 
> > Just to repeat what I sent in reply to v6 (well after you'd posted this).
> > Maybe we can maintain compatibility with the binding before this by adding
> > a default of INT1.  
> 
> But can you make that assumption? If we did, and it's not universally
> true, we break systems that had INT2 connected that previously worked.

I guess there is a possibility of a driver in some other OS assuming INT2, but
seems an odd 'default' choice.  Also odd for a writer of DT for a platform
to assume it.

There is a thing that comes up in spec orgs when discussing whether to
rush out an errata.  "Is this bug something people would get wrong
thinking the answer was clear, or something where the would ask a question?"
Anyone who thinks INT2 is the obvious choice for me falls into the would
ask category.

However, in the linux driver we would would go from assuming no interrupts
to assuming the wrong one.  That's indeed bad.  So I guess this doesn't work.
Oh well no default it is.

Jonathan

> 
> > Then you'd need to drop the dependency on interrupt-names.
> > 
> > I'm not sure though if the checking of number of entries will work against
> > a default. Give it a go and see what happens :)
> > 
> > We are lucky that we can't have bindings in the wild assuming ordering
> > of the two interrupts due to the maxItems being set for interrupts.
> > 
> > It's a messy corner, perhaps we should just not bother in the binding,
> > but keep that default handling in the driver?
> > 
> > DT binding folk, what do you think the best way of handling this is?  

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

[Conor Dooley]

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

On Thu, Dec 19, 2024 at 05:58:15PM +0000, Jonathan Cameron wrote:
> On Sun, 15 Dec 2024 14:56:58 +0000
> Conor Dooley <conor@kernel.org> wrote:
> 
> > On Sat, Dec 14, 2024 at 12:10:57PM +0000, Jonathan Cameron wrote:
> > > On Fri, 13 Dec 2024 21:19:05 +0000
> > > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> > >   
> > > > Add interrupt-names INT1 and INT2 for the two interrupt lines of the
> > > > sensor.
> > > > 
> > > > When one of the two interrupt lines is connected, the interrupt as its
> > > > interrupt-name, need to be declared in the devicetree. The driver then
> > > > configures the sensor to indicate its events on either INT1 or INT2.
> > > > 
> > > > If no interrupt is configured, then no interrupt-name should be
> > > > configured, and vice versa. In this case the sensor runs in FIFO BYPASS
> > > > mode. This allows sensor measurements, but none of the sensor events.
> > > > 
> > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>  
> > > 
> > > Just to repeat what I sent in reply to v6 (well after you'd posted this).
> > > Maybe we can maintain compatibility with the binding before this by adding
> > > a default of INT1.  
> > 
> > But can you make that assumption? If we did, and it's not universally
> > true, we break systems that had INT2 connected that previously worked.
> 
> I guess there is a possibility of a driver in some other OS assuming INT2, but
> seems an odd 'default' choice.

Ye, I think that it is unlikely a driver author would think that way.

> Also odd for a writer of DT for a platform
> to assume it.

I agree, I think it is unlikely that someone would assume it'd work like
this. I think a lack of attention paid to the schematic of the board is
a more likely culprit.

> There is a thing that comes up in spec orgs when discussing whether to
> rush out an errata.  "Is this bug something people would get wrong
> thinking the answer was clear, or something where the would ask a question?"
> Anyone who thinks INT2 is the obvious choice for me falls into the would
> ask category.
> 
> However, in the linux driver we would would go from assuming no interrupts
> to assuming the wrong one.  That's indeed bad.  So I guess this doesn't work.
> Oh well no default it is.

I don't think you really lose anything from having no default. The
driver works just fine without the interrupt, so the albeit small risk
just doesn't seem worth it.

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

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

[Lothar Rubusch]

On Thu, Dec 19, 2024 at 7:21 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Thu, Dec 19, 2024 at 05:58:15PM +0000, Jonathan Cameron wrote:
> > On Sun, 15 Dec 2024 14:56:58 +0000
> > Conor Dooley <conor@kernel.org> wrote:
> >
> > > On Sat, Dec 14, 2024 at 12:10:57PM +0000, Jonathan Cameron wrote:
> > > > On Fri, 13 Dec 2024 21:19:05 +0000
> > > > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> > > >
> > > > > Add interrupt-names INT1 and INT2 for the two interrupt lines of the
> > > > > sensor.
> > > > >
> > > > > When one of the two interrupt lines is connected, the interrupt as its
> > > > > interrupt-name, need to be declared in the devicetree. The driver then
> > > > > configures the sensor to indicate its events on either INT1 or INT2.
> > > > >
> > > > > If no interrupt is configured, then no interrupt-name should be
> > > > > configured, and vice versa. In this case the sensor runs in FIFO BYPASS
> > > > > mode. This allows sensor measurements, but none of the sensor events.
> > > > >
> > > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > >
> > > > Just to repeat what I sent in reply to v6 (well after you'd posted this).
> > > > Maybe we can maintain compatibility with the binding before this by adding
> > > > a default of INT1.
> > >
> > > But can you make that assumption? If we did, and it's not universally
> > > true, we break systems that had INT2 connected that previously worked.
> >
> > I guess there is a possibility of a driver in some other OS assuming INT2, but
> > seems an odd 'default' choice.
>
> Ye, I think that it is unlikely a driver author would think that way.
>
> > Also odd for a writer of DT for a platform
> > to assume it.
>
> I agree, I think it is unlikely that someone would assume it'd work like
> this. I think a lack of attention paid to the schematic of the board is
> a more likely culprit.
>
> > There is a thing that comes up in spec orgs when discussing whether to
> > rush out an errata.  "Is this bug something people would get wrong
> > thinking the answer was clear, or something where the would ask a question?"
> > Anyone who thinks INT2 is the obvious choice for me falls into the would
> > ask category.
> >
> > However, in the linux driver we would would go from assuming no interrupts
> > to assuming the wrong one.  That's indeed bad.  So I guess this doesn't work.
> > Oh well no default it is.
>
> I don't think you really lose anything from having no default. The
> driver works just fine without the interrupt, so the albeit small risk
> just doesn't seem worth it.

Agree. To be honest, I'm not sure if I catch the point here. IMHO,
falling back to FIFO bypass should match with backward compatibility.
Please let me know what I'm missing here.

I would prefer just to check for a specified INT name. Then configure
the specified interrupt line in the probe. In this sense, the
interrupt line is only useful also if the INT name is defined in the
DT. If no INT name is specified, the iio driver will setup FIFO_BYPASS
which is the legacy behavior (according to the datasheet: if none of
the FIFO mode bits are set, defaults to bypass mode). This is the new
behavior.

The old iio driver did not use interrupts at all. It stayed in
FIFO_BYPASS mode (or did not change it, after power on, it assumes
FIFO_BYPASS to my interpretation). Thus declaring the IRQ line yes or
no, with or without INT names - for the iio driver implementation
before this patch series, should not make any difference. It uses
FIFO_BYPASS in all cases.

The input driver (AFAIR we already agreed on ignoring this driver)
needed interrupts. Defining INT names here does not change anything,
either. The input driver configures by default INT1 and simply ignores
what was specified as INT names in the DT.

I cannot really think of any third case here. Please, let me know if
I'm wrong. If not I will keep the above explained behavior, since to
my understanding it should match the desired compatibility
requirements. Am I wrong here?

Sorry for the late answer. Best,
L

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

[Lothar Rubusch]

On Sat, Dec 14, 2024 at 1:39 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri, 13 Dec 2024 21:19:09 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Having interrupts events and FIFO available allows to evaluate the
> > sensor events. Cover the list of interrupt based sensor events. Keep
> > them in the header file for readability.
>
> That makes sense for now, but longer term I'd attempt to restrict the scope
> of these by moving them to the top of core.c
>
> The two bus drivers don't use any of them that I can immediately spot
> and if they do it is likely to be very few.
>
> That may be a good first patch for your next series.

I understand. Actually in v1 of the series I had a patch to move the
defines into _core.c. There were bigger issues, though. Thank you so
much for all the feedback the improvements are impressive to me.

Best,
L

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

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

[Lothar Rubusch]

On Sat, Dec 14, 2024 at 1:36 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri, 13 Dec 2024 21:19:08 +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>
>
> I missed a theoretical bug around the dma buffer in previous reviews.
> A few other minor things inline.
>
> Thanks,
>
> Jonathan
>
> >  /*
> >   * In full-resolution mode, scale factor is maintained at ~4 mg/LSB
> > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > index fc4f89f22..e31a7cb3f 100644
> > --- a/drivers/iio/accel/adxl345_core.c
> > +++ b/drivers/iio/accel/adxl345_core.c
> > @@ -15,9 +15,17 @@
> >
> >  #include <linux/iio/iio.h>
> >  #include <linux/iio/sysfs.h>
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/kfifo_buf.h>
> >
> >  #include "adxl345.h"
> >
> > +#define ADXL345_FIFO_BYPASS  0
> > +#define ADXL345_FIFO_FIFO    1
> > +#define ADXL345_FIFO_STREAM  2
> > +
> > +#define ADXL345_DIRS 3
> > +
> >  #define ADXL345_INT_NONE             0xff
> >  #define ADXL345_INT1                 0
> >  #define ADXL345_INT2                 1
> > @@ -26,27 +34,68 @@ struct adxl345_state {
> >       int irq;
> >       const struct adxl345_chip_info *info;
> >       struct regmap *regmap;
> > +     __le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE + 1];
>
> Ah.  The old corner case (sorry I missed this in earlier reviews!)
>
> In theory at least regmap makes no additional guarantees on how it uses buffers
> on top of those provided by the underlying busses.
> So we need to treat bulk reads the same way we do for those buses.
> That means we need to allow for direct DMA writes into the buffers
> we pass to bulk read.  For that to be safe on all architectures (and not
> accidentally overwrite stuff in the same cacheline) we need to use
> what is known as a DMA safe buffer.
> Long ago we contrived the data layout in IIO to make that easy to
> do.  Just move it down to the end of this structure as...
>
>
> >       bool fifo_delay; /* delay: delay is needed for SPI */
> >       u8 intio;
> > +     u8 int_map;
> > +     u8 watermark;
> > +     u8 fifo_mode;
> this
>         __le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE + 1] __aligned(IIO_DMA_MINALIGN);
>
> This structure already has the appropriate alignment (there is padding inserted
> in the allocation to make that true) so by doing this for the element we
> ask the compiler to make sure it adds sufficient padding before this element
> to ensure nothing else is in the same cacheline (if required by a particular
> architecture).   C also guarantees that the structure itself is large enough
> to ensure padding is added after this element such that the overall structure
> size is a multiple of it's most aligned element (this one).
> Thus we end up with the buffer in it's own cacheline and none of the mess
> of non coherent DMA can cause us problems.
>
> If interested in learning more look for Wolfram's talk at ELCE a number
> of years back on trying to do DMA into the buffers passed to I2C calls.
>
> The 'in theory' bit is because last time I checked regmap is always bounce
> buffering the data but there are obvious optimizations that could be made
> so it didn't bounce. A long back we asked the maintainer about this and he
> said definitely don't assume it won't use the buffers directly.
>
> Note on arm64 for instance there is magic code that bounces all small
> DMA transfers, but that is not enabled on all architectures yet.

Initially, I just copied the buffer definition for the fifo_buf from
other drivers, inclusively the DMA alignment. In the hope 'probably
works similarly' adn without further understanding how the makro
works. Surely w/o knowing how to further verify, that it actually was
working.

When you remarked that my usage of the DMA alignment never could work
that way. I noticed that it was (probably) actually not even needed to
explicitely declare an IIO_DMA_MINALIGN, at least for my setup. So, to
not make it overly complicated, I simply dropped it.

I probably should have asked more questions, you now answered in
detail. I appreciate.

> >  };
>
> >
> > +static const unsigned long adxl345_scan_masks[] = {
> > +     BIT(chan_x) | BIT(chan_y) | BIT(chan_z),
> > +     0,
>
> Trivial but drop that trailing comma. It's a terminator so we don't want to make it
> easy for anyone to accidentally put anything after it!
>
> > +};
>
> >  static int adxl345_read_raw(struct iio_dev *indio_dev,
> >                           struct iio_chan_spec const *chan,
> >                           int *val, int *val2, long mask)
> > @@ -132,6 +181,25 @@ 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 > ADXL345_FIFO_SIZE)
> > +             value = ADXL345_FIFO_SIZE - 1;
>
>         value = min(value, AD345_FIFO_SIZE - 1);
>
> Shorter and maybe a tiny bit more readable. (trivial comment!)
>
> > +
> > +     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;
> > +}
>
>
>
> >  /**
> > @@ -222,6 +499,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> >                                        ADXL345_DATA_FORMAT_JUSTIFY |
> >                                        ADXL345_DATA_FORMAT_FULL_RES |
> >                                        ADXL345_DATA_FORMAT_SELF_TEST);
> > +     u8 fifo_ctl;
>
> Might as well make this declaration local to where it's used.
>
> >       int ret;
> >
> >       indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> > @@ -242,6 +520,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> >       indio_dev->modes = INDIO_DIRECT_MODE;
> >       indio_dev->channels = adxl345_channels;
> >       indio_dev->num_channels = ARRAY_SIZE(adxl345_channels);
> > +     indio_dev->available_scan_masks = adxl345_scan_masks;
> >
> >       if (setup) {
> >               /* Perform optional initial bus specific configuration */
> > @@ -292,6 +571,25 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> >                       st->intio = ADXL345_INT_NONE;
> >       }
> >
> > +     if (st->intio != ADXL345_INT_NONE) {
> > +             /* FIFO_STREAM mode is going to be activated later */
> > +             ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, &adxl345_buffer_ops);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             ret = devm_request_threaded_irq(dev, st->irq, NULL, &adxl345_irq_handler,
> > +                             IRQF_SHARED | IRQF_ONESHOT,
> > +                             indio_dev->name, indio_dev);
>                 ret = devm_request_threaded_irq(dev, st->irq, NULL,
>                                                 &adxl345_irq_handler,
>                                                 IRQF_SHARED | IRQF_ONESHOT,
>                                                 indio_dev->name, indio_dev);
>
> All under 80 chars (still the preference when no reason to do otherwise) and
> aligned with opening bracket which is preferred kernel style when there
> is no reason to do otherwise.
>
> If you weren't going to be doing a v8 anyway I'd just tweak this whilst applying
> but seeing as you are... :)
>
>
> > +             if (ret)
> > +                     return ret;
> > +     } else {
> > +             /* FIFO_BYPASS mode */
> > +             fifo_ctl = ADXL345_FIFO_CTL_MODE(ADXL345_FIFO_BYPASS);
> > +             ret = regmap_write(st->regmap, ADXL345_REG_FIFO_CTL, fifo_ctl);
> > +             if (ret < 0)
> > +                     return ret;
> > +     }
> > +
> >       return devm_iio_device_register(dev, indio_dev);
> >  }
> >  EXPORT_SYMBOL_NS_GPL(adxl345_core_probe, IIO_ADXL345);
>

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

[Conor Dooley]

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

On Wed, Dec 25, 2024 at 02:01:50PM +0100, Lothar Rubusch wrote:
> On Thu, Dec 19, 2024 at 7:21 PM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Thu, Dec 19, 2024 at 05:58:15PM +0000, Jonathan Cameron wrote:
> > > On Sun, 15 Dec 2024 14:56:58 +0000
> > > Conor Dooley <conor@kernel.org> wrote:
> > >
> > > > On Sat, Dec 14, 2024 at 12:10:57PM +0000, Jonathan Cameron wrote:
> > > > > On Fri, 13 Dec 2024 21:19:05 +0000
> > > > > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> > > > >
> > > > > > Add interrupt-names INT1 and INT2 for the two interrupt lines of the
> > > > > > sensor.
> > > > > >
> > > > > > When one of the two interrupt lines is connected, the interrupt as its
> > > > > > interrupt-name, need to be declared in the devicetree. The driver then
> > > > > > configures the sensor to indicate its events on either INT1 or INT2.
> > > > > >
> > > > > > If no interrupt is configured, then no interrupt-name should be
> > > > > > configured, and vice versa. In this case the sensor runs in FIFO BYPASS
> > > > > > mode. This allows sensor measurements, but none of the sensor events.
> > > > > >
> > > > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > > >
> > > > > Just to repeat what I sent in reply to v6 (well after you'd posted this).
> > > > > Maybe we can maintain compatibility with the binding before this by adding
> > > > > a default of INT1.
> > > >
> > > > But can you make that assumption? If we did, and it's not universally
> > > > true, we break systems that had INT2 connected that previously worked.
> > >
> > > I guess there is a possibility of a driver in some other OS assuming INT2, but
> > > seems an odd 'default' choice.
> >
> > Ye, I think that it is unlikely a driver author would think that way.
> >
> > > Also odd for a writer of DT for a platform
> > > to assume it.
> >
> > I agree, I think it is unlikely that someone would assume it'd work like
> > this. I think a lack of attention paid to the schematic of the board is
> > a more likely culprit.
> >
> > > There is a thing that comes up in spec orgs when discussing whether to
> > > rush out an errata.  "Is this bug something people would get wrong
> > > thinking the answer was clear, or something where the would ask a question?"
> > > Anyone who thinks INT2 is the obvious choice for me falls into the would
> > > ask category.
> > >
> > > However, in the linux driver we would would go from assuming no interrupts
> > > to assuming the wrong one.  That's indeed bad.  So I guess this doesn't work.
> > > Oh well no default it is.
> >
> > I don't think you really lose anything from having no default. The
> > driver works just fine without the interrupt, so the albeit small risk
> > just doesn't seem worth it.
> 
> Agree. To be honest, I'm not sure if I catch the point here. IMHO,
> falling back to FIFO bypass should match with backward compatibility.
> Please let me know what I'm missing here.

Ye, falling back to the current behaviour is fine. The only problematic
thing is not checking "for a specified INT name" but assuming the
provided interrupt is either INT1 or INT2 when interrupt-names is not
provided.

> 
> I would prefer just to check for a specified INT name. Then configure
> the specified interrupt line in the probe. In this sense, the
> interrupt line is only useful also if the INT name is defined in the
> DT. If no INT name is specified, the iio driver will setup FIFO_BYPASS
> which is the legacy behavior (according to the datasheet: if none of
> the FIFO mode bits are set, defaults to bypass mode). This is the new
> behavior.
> 
> The old iio driver did not use interrupts at all. It stayed in
> FIFO_BYPASS mode (or did not change it, after power on, it assumes
> FIFO_BYPASS to my interpretation). Thus declaring the IRQ line yes or
> no, with or without INT names - for the iio driver implementation
> before this patch series, should not make any difference. It uses
> FIFO_BYPASS in all cases.
> 
> The input driver (AFAIR we already agreed on ignoring this driver)
> needed interrupts. Defining INT names here does not change anything,
> either. The input driver configures by default INT1 and simply ignores
> what was specified as INT names in the DT.
> 
> I cannot really think of any third case here. Please, let me know if
> I'm wrong. If not I will keep the above explained behavior, since to
> my understanding it should match the desired compatibility
> requirements. Am I wrong here?
> 
> Sorry for the late answer. Best,
> L

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