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

[PATCH v2 00/22] 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.

The series will include the public adxl345.h used also in the
corresponding older input driver. In brief, the data fields seem to be
identical when implementing it for IIO, the file is already in public
include, and to avoid duplication.

The series is meant as base. Implementation of single tap, double tap,
freefall, activity/inactivity on top is relatively straight forward and
will be upcoming, as soon as this patch set is stabilized.

The series reverts the data justification mode of the measurements
(right and left-justified) since it comes in more handy. Further the
series reverts moving out constant defines to the header file and moves
them back into the source file. This is kind of embarassing, and
definitely not on purpose.
When implementing the features, it became clear what was actually used
to be in the header. I hope this is still acceptible, for the learning
curve.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
v1 -> v2: Fix comments according to Documentation/doc-guide/kernel-doc.rst
          and missing static declaration of function.
---
Lothar Rubusch (22):
  iio: accel: adxl345: fix comment on probe
  iio: accel: adxl345: rename variable data to st
  iio: accel: adxl345: rename struct adxl34x_state
  iio: accel: adxl345: rename to adxl34x_channels
  iio: accel: adxl345: measure right-justified
  iio: accel: adxl345: add function to switch measuring
  iio: accel: adxl345: initialize IRQ number
  iio: accel: adxl345: initialize FIFO delay value for SPI
  iio: accel: adxl345: unexpose private defines
  iio: accel: adxl345: set interrupt line to INT1
  iio: accel: adxl345: import adxl345 general data
  iio: accel: adxl345: elaborate iio channel definition
  iio: accel: adxl345: add trigger handler
  iio: accel: adxl345: read FIFO entries
  iio: accel: adxl345: reset the FIFO on error
  iio: accel: adxl345: register trigger ops
  iio: accel: adxl345: push FIFO data to iio
  iio: accel: adxl345: start measure at buffer en/disable
  iio: accel: adxl345: prepare FIFO watermark handling
  iio: accel: adxl345: use FIFO with watermark IRQ
  iio: accel: adxl345: sync FIFO reading with sensor
  iio: accel: adxl345: add debug printout

 drivers/iio/accel/adxl345.h      |  34 +-
 drivers/iio/accel/adxl345_core.c | 887 +++++++++++++++++++++++++++++--
 drivers/iio/accel/adxl345_i2c.c  |   2 +-
 drivers/iio/accel/adxl345_spi.c  |  11 +-
 4 files changed, 845 insertions(+), 89 deletions(-)

-- 
2.39.5

[PATCH v2 01/22] iio: accel: adxl345: fix comment on probe

[Lothar Rubusch]

Fix comment on the probe function. Add covered sensors and fix typo.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 006ce66c0a..d121caf839 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -170,7 +170,7 @@ static void adxl345_powerdown(void *regmap)
 
 /**
  * adxl345_core_probe() - probe and setup for the adxl345 accelerometer,
- *                        also covers the adlx375 accelerometer
+ *                        also covers the adxl375 and adxl346 accelerometer
  * @dev:	Driver model representation of the device
  * @regmap:	Regmap instance for the device
  * @setup:	Setup routine to be executed right before the standard device
-- 
2.39.5

[PATCH v2 02/22] iio: accel: adxl345: rename variable data to st

[Lothar Rubusch]

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

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

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

[PATCH v2 03/22] iio: accel: adxl345: rename struct adxl34x_state

[Lothar Rubusch]

Rename the struct "adxl345_data" to "adxl34x_state". First, the
data structure is supposed to be extended to represent state rather than
only hold sensor data. The data will be a separate member pointer.
Second, the driver not only covers the adxl345 accelerometer, it also
supports the adxl345, adxl346 and adxl375. Thus "adxl34x_" is a choice
for a common prefix.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 3fb7a7b1b7..30896555a4 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -17,7 +17,7 @@
 
 #include "adxl345.h"
 
-struct adxl345_data {
+struct adxl34x_state {
 	const struct adxl345_chip_info *info;
 	struct regmap *regmap;
 };
@@ -43,7 +43,7 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan,
 			    int *val, int *val2, long mask)
 {
-	struct adxl345_data *st = iio_priv(indio_dev);
+	struct adxl34x_state *st = iio_priv(indio_dev);
 	__le16 accel;
 	long long samp_freq_nhz;
 	unsigned int regval;
@@ -99,7 +99,7 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
 			     struct iio_chan_spec const *chan,
 			     int val, int val2, long mask)
 {
-	struct adxl345_data *st = iio_priv(indio_dev);
+	struct adxl34x_state *st = iio_priv(indio_dev);
 	s64 n;
 
 	switch (mask) {
@@ -181,7 +181,7 @@ static void adxl345_powerdown(void *regmap)
 int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 		       int (*setup)(struct device*, struct regmap*))
 {
-	struct adxl345_data *st;
+	struct adxl34x_state *st;
 	struct iio_dev *indio_dev;
 	u32 regval;
 	unsigned int data_format_mask = (ADXL345_DATA_FORMAT_RANGE |
-- 
2.39.5

[PATCH v2 04/22] iio: accel: adxl345: rename to adxl34x_channels

[Lothar Rubusch]

Rename the "adxl345_channels" to "adxl34x_channels". The driver
supports several Analog accelerometers equally, e.g. adxl346 and
adxl375.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 30896555a4..2b62e79248 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -33,7 +33,7 @@ struct adxl34x_state {
 		BIT(IIO_CHAN_INFO_SAMP_FREQ),				\
 }
 
-static const struct iio_chan_spec adxl345_channels[] = {
+static const struct iio_chan_spec adxl34x_channels[] = {
 	ADXL345_CHANNEL(0, X),
 	ADXL345_CHANNEL(1, Y),
 	ADXL345_CHANNEL(2, Z),
@@ -203,8 +203,8 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 	indio_dev->name = st->info->name;
 	indio_dev->info = &adxl345_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
-	indio_dev->channels = adxl345_channels;
-	indio_dev->num_channels = ARRAY_SIZE(adxl345_channels);
+	indio_dev->channels = adxl34x_channels;
+	indio_dev->num_channels = ARRAY_SIZE(adxl34x_channels);
 
 	if (setup) {
 		/* Perform optional initial bus specific configuration */
-- 
2.39.5

[PATCH v2 05/22] iio: accel: adxl345: measure right-justified

[Lothar Rubusch]

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

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 2b62e79248..926e397678 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -184,8 +184,13 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 	struct adxl34x_state *st;
 	struct iio_dev *indio_dev;
 	u32 regval;
+
+	/* NB: ADXL345_DATA_FORMAT_JUSTIFY or 0:
+	 * do right-justified: 0, then adjust resolution according to 10-bit
+	 * through 13-bit in channel - this is the default behavior, and can
+	 * be modified here by oring ADXL345_DATA_FORMAT_JUSTIFY
+	 */
 	unsigned int data_format_mask = (ADXL345_DATA_FORMAT_RANGE |
-					 ADXL345_DATA_FORMAT_JUSTIFY |
 					 ADXL345_DATA_FORMAT_FULL_RES |
 					 ADXL345_DATA_FORMAT_SELF_TEST);
 	int ret;
-- 
2.39.5

[PATCH v2 06/22] iio: accel: adxl345: add function to switch measuring

[Lothar Rubusch]

Replace the powerup / powerdown functions by a generic function to put
the sensor in STANDBY, or MEASURE mode. This is needed for several
features of the accelerometer. It allows to change e.g. FIFO settings.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 926e397678..81688a9eaf 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -138,6 +138,39 @@ 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 adxl34x_state *st, bool en)
+{
+	unsigned int val = 0;
+	int ret;
+
+	val = (en) ? ADXL345_POWER_CTL_MEASURE : ADXL345_POWER_CTL_STANDBY;
+	ret = regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
+	if (ret)
+		return -EINVAL;
+
+	return 0;
+}
+
+static void adxl345_powerdown(void *ptr)
+{
+	struct adxl34x_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 +191,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 adxl345 accelerometer,
  *                        also covers the adxl375 and adxl346 accelerometer
@@ -242,14 +265,12 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 		return dev_err_probe(dev, -ENODEV, "Invalid device ID: %x, expected %x\n",
 				     regval, ADXL345_DEVID);
 
-	/* Enable measurement mode */
-	ret = adxl345_powerup(st->regmap);
+	ret = devm_add_action_or_reset(dev, adxl345_powerdown, st);
 	if (ret < 0)
-		return dev_err_probe(dev, ret, "Failed to enable measurement mode\n");
+		return dev_err_probe(dev, ret, "Failed to add action or reset\n");
 
-	ret = devm_add_action_or_reset(dev, adxl345_powerdown, st->regmap);
-	if (ret < 0)
-		return ret;
+	/* Enable measurement mode */
+	adxl345_set_measure_en(st, true);
 
 	return devm_iio_device_register(dev, indio_dev);
 }
-- 
2.39.5

[PATCH v2 07/22] iio: accel: adxl345: initialize IRQ number

[Lothar Rubusch]

Add the possibility to claim an interrupt and init the state structure
with interrupt number and interrupt line to use. The adxl345 can use
two different interrupt lines, mainly to signal FIFO watermark events,
single or double tap, activity, etc. Hence, having the interrupt line
available is crucial to implement such features.

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

diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
index 3d5c8719db..cf4132715c 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,
+		       int irq,
 		       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 81688a9eaf..902bd3568b 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -11,6 +11,7 @@
 #include <linux/property.h>
 #include <linux/regmap.h>
 #include <linux/units.h>
+#include <linux/interrupt.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
@@ -18,6 +19,7 @@
 #include "adxl345.h"
 
 struct adxl34x_state {
+	int irq;
 	const struct adxl345_chip_info *info;
 	struct regmap *regmap;
 };
@@ -196,12 +198,14 @@ static const struct iio_info adxl345_info = {
  *                        also covers the adxl375 and adxl346 accelerometer
  * @dev:	Driver model representation of the device
  * @regmap:	Regmap instance for the device
+ * @irq:	Interrupt handling for async usage
  * @setup:	Setup routine to be executed right before the standard device
  *		setup
  *
  * Return: 0 on success, negative errno on error
  */
 int adxl345_core_probe(struct device *dev, struct regmap *regmap,
+		       int irq,
 		       int (*setup)(struct device*, struct regmap*))
 {
 	struct adxl34x_state *st;
@@ -224,6 +228,8 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 
 	st = iio_priv(indio_dev);
 	st->regmap = regmap;
+
+	st->irq = irq;
 	st->info = device_get_match_data(dev);
 	if (!st->info)
 		return -ENODEV;
diff --git a/drivers/iio/accel/adxl345_i2c.c b/drivers/iio/accel/adxl345_i2c.c
index 4065b8f7c8..604b706c29 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, client->irq, 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 61fd9a6f5f..39e7d71e1d 100644
--- a/drivers/iio/accel/adxl345_spi.c
+++ b/drivers/iio/accel/adxl345_spi.c
@@ -39,9 +39,13 @@ static int adxl345_spi_probe(struct spi_device *spi)
 		return dev_err_probe(&spi->dev, PTR_ERR(regmap), "Error initializing regmap\n");
 
 	if (spi->mode & SPI_3WIRE)
-		return adxl345_core_probe(&spi->dev, regmap, adxl345_spi_setup);
+		return adxl345_core_probe(&spi->dev, regmap,
+					  spi->irq,
+					  adxl345_spi_setup);
 	else
-		return adxl345_core_probe(&spi->dev, regmap, NULL);
+		return adxl345_core_probe(&spi->dev, regmap,
+					  spi->irq,
+					  NULL);
 }
 
 static const struct adxl345_chip_info adxl345_spi_info = {
-- 
2.39.5

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

diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
index cf4132715c..4ba493f636 100644
--- a/drivers/iio/accel/adxl345.h
+++ b/drivers/iio/accel/adxl345.h
@@ -62,7 +62,7 @@ struct adxl345_chip_info {
 };
 
 int adxl345_core_probe(struct device *dev, struct regmap *regmap,
-		       int irq,
+		       int irq, 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 902bd3568b..51b229cc44 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -22,6 +22,7 @@ struct adxl34x_state {
 	int irq;
 	const struct adxl345_chip_info *info;
 	struct regmap *regmap;
+	bool fifo_delay; /* delay: delay is needed for SPI */
 };
 
 #define ADXL345_CHANNEL(index, axis) {					\
@@ -199,13 +200,14 @@ static const struct iio_info adxl345_info = {
  * @dev:	Driver model representation of the device
  * @regmap:	Regmap instance for the device
  * @irq:	Interrupt handling for async usage
+ * @fifo_delay_default: Using FIFO with SPI needs delay
  * @setup:	Setup routine to be executed right before the standard device
  *		setup
  *
  * Return: 0 on success, negative errno on error
  */
 int adxl345_core_probe(struct device *dev, struct regmap *regmap,
-		       int irq,
+		       int irq, bool fifo_delay_default,
 		       int (*setup)(struct device*, struct regmap*))
 {
 	struct adxl34x_state *st;
@@ -234,6 +236,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 604b706c29..fa1b7e7026 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, client->irq, NULL);
+	return adxl345_core_probe(&client->dev, regmap, client->irq, 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 39e7d71e1d..75940d9c1c 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,
@@ -41,10 +42,12 @@ static int adxl345_spi_probe(struct spi_device *spi)
 	if (spi->mode & SPI_3WIRE)
 		return adxl345_core_probe(&spi->dev, regmap,
 					  spi->irq,
+					  spi->max_speed_hz > ADXL345_MAX_FREQ_NO_FIFO_DELAY,
 					  adxl345_spi_setup);
 	else
 		return adxl345_core_probe(&spi->dev, regmap,
 					  spi->irq,
+					  spi->max_speed_hz > ADXL345_MAX_FREQ_NO_FIFO_DELAY,
 					  NULL);
 }
 
-- 
2.39.5

[PATCH v2 09/22] iio: accel: adxl345: unexpose private defines

[Lothar Rubusch]

For the implementation of features like FIFO-usage, watermark, single
tap, double tap, freefall, etc. most of the constants do not need to be
exposed in the header file, but are rather of private nature. Reduce
namespace pollution by moving them to the core source file.

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

diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
index 4ba493f636..c9d2a82fa6 100644
--- a/drivers/iio/accel/adxl345.h
+++ b/drivers/iio/accel/adxl345.h
@@ -8,38 +8,9 @@
 #ifndef _ADXL345_H_
 #define _ADXL345_H_
 
-#define ADXL345_REG_DEVID		0x00
-#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))
-#define ADXL345_REG_BW_RATE		0x2C
-#define ADXL345_REG_POWER_CTL		0x2D
-#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_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_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 */
+/* Regs and bits needed to be declared globally */
+#define ADXL345_REG_DATA_FORMAT		0x31 /* r/w  Data format control */
 #define ADXL345_DATA_FORMAT_SPI_3WIRE	BIT(6)	/* 3-wire SPI mode */
-#define ADXL345_DATA_FORMAT_SELF_TEST	BIT(7)	/* Enable a self test */
-
-#define ADXL345_DATA_FORMAT_2G		0
-#define ADXL345_DATA_FORMAT_4G		1
-#define ADXL345_DATA_FORMAT_8G		2
-#define ADXL345_DATA_FORMAT_16G		3
-
-#define ADXL345_DEVID			0xE5
 
 /*
  * 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 51b229cc44..c8d9e1f9e0 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -18,6 +18,114 @@
 
 #include "adxl345.h"
 
+/* ADXL345 register map */
+#define ADXL345_REG_DEVID		0x00 /* r    Device ID */
+#define ADXL345_REG_THRESH_TAP	0x1D /* r/w  Tap Threshold */
+#define ADXL345_REG_OFSX		0x1E /* r/w  X-axis offset */
+#define ADXL345_REG_OFSY		0x1F /* r/w  Y-axis offset */
+#define ADXL345_REG_OFSZ		0x20 /* r/w  Z-axis offset */
+#define ADXL345_REG_DUR		0x21 /* r/w  Tap duration */
+#define ADXL345_REG_LATENT		0x22 /* r/w  Tap latency */
+#define ADXL345_REG_WINDOW		0x23 /* r/w  Tap window */
+#define ADXL345_REG_THRESH_ACT		0x24 /* r/w  Activity threshold */
+#define ADXL345_REG_THRESH_INACT	0x25 /* r/w  Inactivity threshold */
+#define ADXL345_REG_TIME_INACT	0x26 /* r/w  Inactivity time */
+#define ADXL345_REG_ACT_INACT_CTRL	0x27 /* r/w  Axis enable control for */
+					     /*      activity and inactivity */
+					     /*      detection */
+#define ADXL345_REG_THRESH_FF		0x28 /* r/w  Free-fall threshold */
+#define ADXL345_REG_TIME_FF		0x29 /* r/w  Free-fall time */
+#define ADXL345_REG_TAP_AXIS		0x2A /* r/w  Axis control for */
+					     /*      single tap or double tap */
+#define ADXL345_REG_ACT_TAP_STATUS	0x2B /* r    Source of single tap or */
+					     /*      double tap */
+#define ADXL345_REG_BW_RATE		0x2C /* r/w  Data rate and power */
+					     /*        mode control */
+#define ADXL345_REG_POWER_CTL		0x2D /* r/w  Power-saving features */
+#define ADXL345_REG_INT_ENABLE		0x2E /* r/w  Interrupt enable control */
+#define ADXL345_REG_INT_MAP		0x2F /* r/w  Interrupt mapping */
+					     /*      control */
+#define ADXL345_REG_INT_SOURCE		0x30 /* r    Source of interrupts */
+/* NB: ADXL345_REG_DATA_FORMAT		0x31  r/w  Data format control,
+ *   (defined in header)
+ */
+
+#define ADXL345_REG_XYZ_BASE		0x32 /* r    Base address to read out */
+					     /*      X-, Y- and Z-axis data 0 */
+					     /*      and 1 */
+#define ADXL345_REG_DATA_AXIS(index)				\
+	(ADXL345_REG_XYZ_BASE + (index) * sizeof(__le16))
+/* NB: having DATAX0 and DATAX1 makes 2x sizeof(__le16) */
+
+#define ADXL345_REG_FIFO_CTL		0x38 /* r/w  FIFO control */
+#define ADXL345_REG_FIFO_STATUS		0x39 /* r    FIFO status */
+
+/* DEVID(s) */
+#define ADXL345_DEVID			0xE5
+
+/* FIFO */
+#define ADXL345_FIFO_CTL_SAMLPES(x)	(0x1f & (x))
+#define ADXL345_FIFO_CTL_TRIGGER(x)	(0x20 & ((x) << 5)) /* set 1: INT2, 0: INT1 */
+#define ADXL345_FIFO_CTL_MODE(x)	(0xc0 & ((x) << 6))
+
+/* INT_ENABLE, INT_MAP, INT_SOURCE bits */
+#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 ADXL34X_S_TAP_MSK	ADXL345_INT_SINGLE_TAP
+#define ADXL34X_D_TAP_MSK	ADXL345_INT_DOUBLE_TAP
+
+/* INT1 or INT2 */
+#define ADXL345_INT1			0
+#define ADXL345_INT2			1
+
+/* BW_RATE bits - Bandwidth and output data rate. The default value is
+ * 0x0A, which translates to a 100 Hz output data rate
+ */
+#define ADXL345_BW_RATE			GENMASK(3, 0)
+#define ADXL345_BW_LOW_POWER	BIT(4)
+#define ADXL345_BASE_RATE_NANO_HZ	97656250LL
+
+/* POWER_CTL bits */
+#define ADXL345_POWER_CTL_STANDBY	0x00
+
+/* NB:
+ * BIT(0), BIT(1) for explicit wakeup (not implemented)
+ * BIT(2) for explicit sleep (not implemented)
+ */
+#define ADXL345_POWER_CTL_MEASURE	BIT(3)
+#define ADXL345_POWER_CTL_AUTO_SLEEP	BIT(4)
+#define ADXL345_POWER_CTL_LINK	BIT(5)
+
+/* DATA_FORMAT bits */
+#define ADXL345_DATA_FORMAT_RANGE	GENMASK(1, 0)	/* Set the g range */
+#define ADXL345_DATA_FORMAT_JUSTIFY	BIT(2)	/* 1: left-justified (MSB) mode, 0: right-just'd */
+#define ADXL345_DATA_FORMAT_FULL_RES	BIT(3)	/* Up to 13-bits resolution */
+/* NB: BIT(6): 3-wire SPI mode (defined in header) */
+
+#define ADXL345_DATA_FORMAT_SELF_TEST	BIT(7)	/* Enable a self test */
+#define ADXL345_DATA_FORMAT_2G		0
+#define ADXL345_DATA_FORMAT_4G		1
+#define ADXL345_DATA_FORMAT_8G		2
+#define ADXL345_DATA_FORMAT_16G		3
+
+#define ADXL345_REG_OFS_AXIS(index)	(ADXL345_REG_OFSX + (index))
+
+/* The ADXL345 include a 32 sample FIFO
+ *
+ * FIFO stores a maximum of 32 entries, which equates to a maximum of 33
+ * entries available at any given time because an additional entry is available
+ * at the output filter of the device.
+ * (see datasheet FIFO_STATUS description on "Entries Bits")
+ */
+#define ADXL34x_FIFO_SIZE  33
+
 struct adxl34x_state {
 	int irq;
 	const struct adxl345_chip_info *info;
-- 
2.39.5

[PATCH v2 10/22] iio: accel: adxl345: set interrupt line to INT1

[Lothar Rubusch]

The adxl345 sensor uses one of two interrupt lines, INT1 or INT2. The
interrupt lines are used to signal feature events such as watermark
reached, single tap, double tap, activity, etc. Only one interrupt line
is used and must be configured. The adxl345 default is to use INT1 and
in many installations only INT1 is even connected. Therefore configure
INT1 as the sensor's default interrupt line.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index c8d9e1f9e0..32163cfe6f 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -131,6 +131,7 @@ struct adxl34x_state {
 	const struct adxl345_chip_info *info;
 	struct regmap *regmap;
 	bool fifo_delay; /* delay: delay is needed for SPI */
+	u8 intio;
 };
 
 #define ADXL345_CHANNEL(index, axis) {					\
@@ -345,6 +346,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 		return -ENODEV;
 
 	st->fifo_delay = fifo_delay_default;
+	st->intio = ADXL345_INT1;
 
 	indio_dev->name = st->info->name;
 	indio_dev->info = &adxl345_info;
-- 
2.39.5

[PATCH v2 11/22] iio: accel: adxl345: import adxl345 general data

[Lothar Rubusch]

Use struct adxl34x_platform_data from the included public header of the
input driver for ADXL34x with the following argumentation for this
approach:

- The iio driver for the ADXL34x covers features also implemented in
  the older input driver. The iio driver will implement the same
  features but can also benefit from including the common header and
  struct adxl34x_platform_data. Once complete, the input driver could
  be faded out.

- The fields in the input driver are identical to the fields the IIO
  implementation will need. Including the header over reimplementing,
  avoids a duplication of almost identical headers in IIO and iio.

- The header for the input driver is public. It provides a public
  interface for adxl34x related implementation and is not private to
  the input system.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 32163cfe6f..b16887ec1c 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -16,6 +16,8 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 
+#include <linux/input/adxl34x.h>
+
 #include "adxl345.h"
 
 /* ADXL345 register map */
@@ -126,10 +128,15 @@
  */
 #define ADXL34x_FIFO_SIZE  33
 
+static const struct adxl34x_platform_data adxl345_default_init = {
+	.tap_axis_control = ADXL_TAP_X_EN | ADXL_TAP_Y_EN | ADXL_TAP_Z_EN,
+};
+
 struct adxl34x_state {
 	int irq;
 	const struct adxl345_chip_info *info;
 	struct regmap *regmap;
+	struct adxl34x_platform_data data;  /* watermark, fifo_mode, etc */
 	bool fifo_delay; /* delay: delay is needed for SPI */
 	u8 intio;
 };
@@ -331,6 +338,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 	unsigned int data_format_mask = (ADXL345_DATA_FORMAT_RANGE |
 					 ADXL345_DATA_FORMAT_FULL_RES |
 					 ADXL345_DATA_FORMAT_SELF_TEST);
+	const struct adxl34x_platform_data *data;
 	int ret;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
@@ -346,6 +354,16 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 		return -ENODEV;
 
 	st->fifo_delay = fifo_delay_default;
+	data = dev_get_platdata(dev);
+	if (!data) {
+		dev_dbg(dev, "No platform data: Using default initialization\n");
+		data = &adxl345_default_init;
+	}
+	st->data = *data;
+
+	/* some reasonable pre-initialization */
+	st->data.act_axis_control = 0xFF;
+
 	st->intio = ADXL345_INT1;
 
 	indio_dev->name = st->info->name;
-- 
2.39.5

[PATCH v2 12/22] iio: accel: adxl345: elaborate iio channel definition

[Lothar Rubusch]

Make the channel definition ready to allow for feature implementation
for this accelerometer sensor.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index b16887ec1c..f98ddef4c5 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -141,21 +141,33 @@ struct adxl34x_state {
 	u8 intio;
 };
 
-#define ADXL345_CHANNEL(index, axis) {					\
-	.type = IIO_ACCEL,						\
-	.modified = 1,							\
-	.channel2 = IIO_MOD_##axis,					\
-	.address = index,						\
-	.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),				\
+#define ADXL34x_CHANNEL(index, reg, axis) {				\
+		.type = IIO_ACCEL,					\
+			.address = (reg),				\
+			.modified = 1,					\
+			.channel2 = IIO_MOD_##axis,			\
+			.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,			\
+				.shift = 0,				\
+				.endianness = IIO_LE,			\
+			},						\
 }
 
+enum adxl34x_chans {
+	chan_x, chan_y, chan_z,
+};
+
 static const struct iio_chan_spec adxl34x_channels[] = {
-	ADXL345_CHANNEL(0, X),
-	ADXL345_CHANNEL(1, Y),
-	ADXL345_CHANNEL(2, Z),
+	ADXL34x_CHANNEL(0, chan_x, X),
+	ADXL34x_CHANNEL(1, chan_y, Y),
+	ADXL34x_CHANNEL(2, chan_z, Z),
 };
 
 static int adxl345_read_raw(struct iio_dev *indio_dev,
-- 
2.39.5

[PATCH v2 13/22] iio: accel: adxl345: add trigger handler

[Lothar Rubusch]

Add basic setup to the interrupt handler function and prepare probe
for using and not using the FIFO on the adxl345. Interrupt handler and
basic structure integration is needed to evaluate interrupt source
register. This is crucial for implementing further features of the
sensor.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index f98ddef4c5..508de81bb9 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -15,6 +15,9 @@
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
 
 #include <linux/input/adxl34x.h>
 
@@ -137,6 +140,7 @@ struct adxl34x_state {
 	const struct adxl345_chip_info *info;
 	struct regmap *regmap;
 	struct adxl34x_platform_data data;  /* watermark, fifo_mode, etc */
+	u8 int_map;
 	bool fifo_delay; /* delay: delay is needed for SPI */
 	u8 intio;
 };
@@ -302,6 +306,10 @@ static void adxl345_powerdown(void *ptr)
 	adxl345_set_measure_en(st, false);
 }
 
+static const struct iio_dev_attr *adxl345_fifo_attributes[] = {
+	NULL,
+};
+
 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"
 );
@@ -315,6 +323,75 @@ static const struct attribute_group adxl345_attrs_group = {
 	.attrs = adxl345_attrs,
 };
 
+static const struct iio_buffer_setup_ops adxl345_buffer_ops = {
+};
+
+static int adxl345_get_status(struct adxl34x_state *st, u8 *int_stat)
+{
+	int ret;
+	unsigned int regval;
+
+	*int_stat = 0;
+	ret = regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, &regval);
+	if (ret) {
+		pr_warn("%s(): Failed to read INT_SOURCE register\n", __func__);
+		return -EINVAL;
+	}
+
+	*int_stat = 0xff & regval;
+	pr_debug("%s(): int_stat 0x%02X (INT_SOURCE)\n", __func__, *int_stat);
+
+	return 0;
+}
+
+/**
+ * irqreturn_t adxl345_trigger_handler() - Interrupt handler used for several
+ *                                         features of the ADXL345.
+ * @irq: The interrupt number.
+ * @p: The iio poll function instance, used to derive the device and data.
+ *
+ * Having an interrupt imples FIFO_STREAM mode was enabled. Since this is given
+ * there will no be further test for being in FIFO_BYPASS mode. FIFO_TRIGGER
+ * and FIFO_FIFO mode (being similar to FIFO_STREAM mode) are not separately
+ * implemented so far. Both should be work smoothly with the same way of
+ * interrupt handling.
+ *
+ * Return: Describe if the interrupt was handled.
+ */
+static irqreturn_t adxl345_trigger_handler(int irq, void *p)
+{
+	struct iio_dev *indio_dev = ((struct iio_poll_func *) p)->indio_dev;
+	struct adxl34x_state *st = iio_priv(indio_dev);
+	u8 int_stat;
+	int ret;
+
+	ret = adxl345_get_status(st, &int_stat);
+	if (ret < 0)
+		goto done;
+
+	/* Ignore already read event by reissued too fast */
+	if (int_stat == 0x0)
+		goto done;
+
+	/* evaluation */
+
+	if (int_stat & ADXL345_INT_OVERRUN) {
+		pr_debug("%s(): OVERRUN event detected\n", __func__);
+		goto done;
+	}
+
+	if (int_stat & (ADXL345_INT_DATA_READY | ADXL345_INT_WATERMARK))
+		pr_debug("%s(): WATERMARK or DATA_READY event detected\n", __func__);
+
+	goto done;
+done:
+
+	if (indio_dev)
+		iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
 static const struct iio_info adxl345_info = {
 	.attrs		= &adxl345_attrs_group,
 	.read_raw	= adxl345_read_raw,
@@ -351,6 +428,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 					 ADXL345_DATA_FORMAT_FULL_RES |
 					 ADXL345_DATA_FORMAT_SELF_TEST);
 	const struct adxl34x_platform_data *data;
+	u8 fifo_ctl;
 	int ret;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
@@ -419,9 +497,32 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 	if (ret < 0)
 		return dev_err_probe(dev, ret, "Failed to add action or reset\n");
 
-	/* Enable measurement mode */
-	adxl345_set_measure_en(st, true);
+	/* Basic common initialization of the driver is done now */
+
+	if (st->irq) { /* Initialization to prepare for FIFO_STREAM mode */
+		ret = devm_iio_triggered_buffer_setup_ext(dev, indio_dev,
+							  NULL,
+							  adxl345_trigger_handler,
+							  IIO_BUFFER_DIRECTION_IN,
+							  &adxl345_buffer_ops,
+							  adxl345_fifo_attributes);
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to setup triggered buffer\n");
+
+	} else { /* Initialization to prepare for FIFO_BYPASS mode (fallback) */
 
+		/* The following defaults to 0x00, anyway */
+		fifo_ctl = 0x00 | ADXL345_FIFO_CTL_MODE(ADXL_FIFO_BYPASS);
+
+		dev_dbg(dev, "fifo_ctl 0x%02X [0x00]\n", fifo_ctl);
+		ret = regmap_write(st->regmap, ADXL345_REG_FIFO_CTL, fifo_ctl);
+		if (ret < 0)
+			return ret;
+
+		/* Enable measurement mode */
+		adxl345_set_measure_en(st, true);
+	}
+	dev_dbg(dev, "Driver operational\n");
 	return devm_iio_device_register(dev, indio_dev);
 }
 EXPORT_SYMBOL_NS_GPL(adxl345_core_probe, IIO_ADXL345);
-- 
2.39.5

[PATCH v2 14/22] iio: accel: adxl345: read FIFO entries

[Lothar Rubusch]

Add a function to read the elements of the adxl345 FIFO. This will
flush the FIFO, and brings it into a ready state. The read out
is used to read the elemnts and to reset the fifo again. The cleanup
equally needs a read on the INT_SOURCE register.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 508de81bb9..40e78dbdb0 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -140,6 +140,8 @@ struct adxl34x_state {
 	const struct adxl345_chip_info *info;
 	struct regmap *regmap;
 	struct adxl34x_platform_data data;  /* watermark, fifo_mode, etc */
+
+	__le16 fifo_buf[3 * ADXL34x_FIFO_SIZE] __aligned(IIO_DMA_MINALIGN);
 	u8 int_map;
 	bool fifo_delay; /* delay: delay is needed for SPI */
 	u8 intio;
@@ -323,6 +325,37 @@ static const struct attribute_group adxl345_attrs_group = {
 	.attrs = adxl345_attrs,
 };
 
+/**
+ * adxl345_get_fifo_entries() - Read number of FIFO entries into *fifo_entries.
+ * @st: The initialized state instance of this driver.
+ * @fifo_entries: A field to be initialized by this function with the number of
+ * FIFO entries.
+ *
+ * Since one read on the FIFO is reading all three axis, X, Y and Z in one, the
+ * number of FIFO entries corresponds to the number of triples of those values.
+ * Note, this sensor does not support treating any axis individually, or
+ * exclude them from measuring.
+ *
+ * Return: 0 or error value.
+ */
+static int adxl345_get_fifo_entries(struct adxl34x_state *st, int *fifo_entries)
+{
+	unsigned int regval = 0;
+	int ret;
+
+	ret = regmap_read(st->regmap, ADXL345_REG_FIFO_STATUS, &regval);
+	if (ret) {
+		pr_warn("%s(): Failed to read FIFO_STATUS register\n", __func__);
+		*fifo_entries = 0;
+		return ret;
+	}
+
+	*fifo_entries = 0x3f & regval;
+	pr_debug("%s(): fifo_entries %d\n", __func__, *fifo_entries);
+
+	return 0;
+}
+
 static const struct iio_buffer_setup_ops adxl345_buffer_ops = {
 };
 
@@ -363,6 +396,7 @@ static irqreturn_t adxl345_trigger_handler(int irq, void *p)
 	struct iio_dev *indio_dev = ((struct iio_poll_func *) p)->indio_dev;
 	struct adxl34x_state *st = iio_priv(indio_dev);
 	u8 int_stat;
+	int fifo_entries;
 	int ret;
 
 	ret = adxl345_get_status(st, &int_stat);
@@ -380,9 +414,11 @@ static irqreturn_t adxl345_trigger_handler(int irq, void *p)
 		goto done;
 	}
 
-	if (int_stat & (ADXL345_INT_DATA_READY | ADXL345_INT_WATERMARK))
+	if (int_stat & (ADXL345_INT_DATA_READY | ADXL345_INT_WATERMARK)) {
 		pr_debug("%s(): WATERMARK or DATA_READY event detected\n", __func__);
-
+		if (adxl345_get_fifo_entries(st, &fifo_entries) < 0)
+			goto done;
+	}
 	goto done;
 done:
 
-- 
2.39.5

[PATCH v2 15/22] iio: accel: adxl345: reset the FIFO on error

[Lothar Rubusch]

Add a function to empty the FIFO and reset the INT_SOURCE register.
Reading out is used to reset the fifo again. For cleanup also a read
on the INT_SOURCE register is needed to allow the adxl345 to issue
interrupts again. Without clearing the fields no further interrupts
will happen.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 40e78dbdb0..82bd5c2b78 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -356,6 +356,61 @@ static int adxl345_get_fifo_entries(struct adxl34x_state *st, int *fifo_entries)
 	return 0;
 }
 
+/**
+ * adxl345_read_fifo_elements() - Read fifo_entries number of elements.
+ * @st: The instance of the state object of this sensor.
+ * @fifo_entries: The number of lines in the FIFO referred to as fifo_entry,
+ * a fifo_entry has 3 elements for X, Y and Z direction of 2 bytes each.
+ *
+ * The FIFO of the sensor is read linewise. The read measurement values are
+ * queued in the corresponding data structure in *st.
+ *
+ * It is recommended that a multiple-byte read of all registers be performed to
+ * prevent a change in data between reads of sequential registers. That is to
+ * read out the data registers X0, X1, Y0, Y1, Z0, Z1 at once.
+ *
+ * Return: 0 or error value.
+ */
+static int adxl345_read_fifo_elements(struct adxl34x_state *st, int fifo_entries)
+{
+	size_t count, ndirs = 3;
+	int i, ret;
+
+	count = 2 * ndirs; /* 2 byte per direction */
+	for (i = 0; i < fifo_entries; i++) {
+		ret = regmap_noinc_read(st->regmap, ADXL345_REG_XYZ_BASE,
+				st->fifo_buf + (i * count / 2), count);
+		if (ret) {
+			pr_warn("%s(): regmap_noinc_read() failed\n", __func__);
+			return -EFAULT;
+		}
+	}
+
+	return 0;
+}
+
+/**
+ * adxl345_empty_fifo() - Empty the FIFO.
+ * @st: The instance to the state object of the sensor.
+ *
+ * Reading all elements of the FIFO linewise empties the FIFO. Reading th
+ * interrupt source register resets the sensor. This is needed also in case of
+ * overflow or error handling to reenable the sensor to issue interrupts.
+ */
+static void adxl345_empty_fifo(struct adxl34x_state *st)
+{
+	int regval;
+	int fifo_entries;
+
+	/* In case the HW is not "clean" just read out remaining elements */
+	adxl345_get_fifo_entries(st, &fifo_entries);
+	if (fifo_entries > 0)
+		adxl345_read_fifo_elements(st, fifo_entries);
+
+	/* Reset the INT_SOURCE register by reading the register */
+	regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, &regval);
+}
+
 static const struct iio_buffer_setup_ops adxl345_buffer_ops = {
 };
 
@@ -401,30 +456,34 @@ static irqreturn_t adxl345_trigger_handler(int irq, void *p)
 
 	ret = adxl345_get_status(st, &int_stat);
 	if (ret < 0)
-		goto done;
+		goto err;
 
 	/* Ignore already read event by reissued too fast */
 	if (int_stat == 0x0)
-		goto done;
+		goto err;
 
 	/* evaluation */
 
 	if (int_stat & ADXL345_INT_OVERRUN) {
 		pr_debug("%s(): OVERRUN event detected\n", __func__);
-		goto done;
+		goto err;
 	}
 
 	if (int_stat & (ADXL345_INT_DATA_READY | ADXL345_INT_WATERMARK)) {
 		pr_debug("%s(): WATERMARK or DATA_READY event detected\n", __func__);
 		if (adxl345_get_fifo_entries(st, &fifo_entries) < 0)
-			goto done;
-	}
-	goto done;
-done:
+			goto err;
 
-	if (indio_dev)
 		iio_trigger_notify_done(indio_dev->trig);
+	}
 
+	goto done;
+err:
+	iio_trigger_notify_done(indio_dev->trig);
+	adxl345_empty_fifo(st);
+	return IRQ_NONE;
+
+done:
 	return IRQ_HANDLED;
 }
 
-- 
2.39.5

[PATCH v2 16/22] iio: accel: adxl345: register trigger ops

[Lothar Rubusch]

Add trigger options to the sensor driver. Reacting to the sensor events
communicated by IRQ, the FIFO handling and the trigger will be core
events for further feature implementation.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 82bd5c2b78..d58e1994ff 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -15,6 +15,9 @@
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/events.h>
+#include <linux/iio/kfifo_buf.h>
 #include <linux/iio/trigger.h>
 #include <linux/iio/trigger_consumer.h>
 #include <linux/iio/triggered_buffer.h>
@@ -140,11 +143,13 @@ struct adxl34x_state {
 	const struct adxl345_chip_info *info;
 	struct regmap *regmap;
 	struct adxl34x_platform_data data;  /* watermark, fifo_mode, etc */
+	struct iio_trigger *trig_dready;
 
 	__le16 fifo_buf[3 * ADXL34x_FIFO_SIZE] __aligned(IIO_DMA_MINALIGN);
 	u8 int_map;
 	bool fifo_delay; /* delay: delay is needed for SPI */
 	u8 intio;
+	bool watermark_en;
 };
 
 #define ADXL34x_CHANNEL(index, reg, axis) {				\
@@ -432,6 +437,35 @@ static int adxl345_get_status(struct adxl34x_state *st, u8 *int_stat)
 	return 0;
 }
 
+/* data ready trigger */
+
+static int adxl345_trig_dready(struct iio_trigger *trig, bool state)
+{
+	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+	struct adxl34x_state *st = iio_priv(indio_dev);
+
+	st->int_map = 0x00;
+	if (state) {
+		/* Setting also ADXL345_INT_DATA_READY results in just a single
+		 * generated interrupt, and no continuously re-generation. NB that the
+		 * INT_DATA_READY as well as the INT_OVERRUN are managed automatically,
+		 * setting their bits here is not needed.
+		 */
+		if (st->watermark_en)
+			st->int_map |= ADXL345_INT_WATERMARK;
+
+		pr_debug("%s(): preparing st->int_map 0x%02X\n",
+			 __func__, st->int_map);
+	}
+
+	return 0;
+}
+
+static const struct iio_trigger_ops adxl34x_trig_dready_ops = {
+	.validate_device = &iio_trigger_validate_own_device,
+	.set_trigger_state = adxl345_trig_dready,
+};
+
 /**
  * irqreturn_t adxl345_trigger_handler() - Interrupt handler used for several
  *                                         features of the ADXL345.
-- 
2.39.5

[PATCH v2 17/22] iio: accel: adxl345: push FIFO data to iio

[Lothar Rubusch]

Add FIFO and hwfifo handling. Add some functions to deal with FIFO
entries and configuration. This feature will be needed for e.g.
watermark setting.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index d58e1994ff..a653774db8 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -437,6 +437,41 @@ static int adxl345_get_status(struct adxl34x_state *st, u8 *int_stat)
 	return 0;
 }
 
+static int adxl345_push_fifo_data(struct iio_dev *indio_dev,
+				  u8 status,
+				  int fifo_entries)
+{
+	struct adxl34x_state *st = iio_priv(indio_dev);
+	int ndirs = 3; /* 3 directions */
+	int i, ret;
+
+	if (fifo_entries <= 0)
+		return true;
+
+	ret = adxl345_read_fifo_elements(st, fifo_entries);
+	if (ret)
+		return false;
+
+	for (i = 0; i < ndirs * fifo_entries; i += ndirs) {
+		/* 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 && (fifo_entries > 1))
+			udelay(3);
+
+		iio_push_to_buffers(indio_dev, &st->fifo_buf[i]);
+	}
+
+	return true;
+}
+
 /* data ready trigger */
 
 static int adxl345_trig_dready(struct iio_trigger *trig, bool state)
@@ -508,6 +543,9 @@ static irqreturn_t adxl345_trigger_handler(int irq, void *p)
 		if (adxl345_get_fifo_entries(st, &fifo_entries) < 0)
 			goto err;
 
+		if (adxl345_push_fifo_data(indio_dev, int_stat, fifo_entries) < 0)
+			goto err;
+
 		iio_trigger_notify_done(indio_dev->trig);
 	}
 
-- 
2.39.5

[PATCH v2 18/22] iio: accel: adxl345: start measure at buffer en/disable

[Lothar Rubusch]

Add and initialize the buffer options to use the FIFO and watermark
feature of the adxl345 sensor. In this way measure enable will happen
in at enabling the buffer.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index a653774db8..b57a123ac9 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -181,6 +181,28 @@ static const struct iio_chan_spec adxl34x_channels[] = {
 	ADXL34x_CHANNEL(2, chan_z, Z),
 };
 
+static int adxl345_set_interrupts(struct adxl34x_state *st)
+{
+	int ret;
+	unsigned int int_enable = st->int_map;
+	unsigned int int_map;
+
+	/* Any bits set to 0 in the INT map register send their respective
+	 * interrupts to the INT1 pin, whereas bits set to 1 send their respective
+	 * interrupts to the INT2 pin. The intio shall convert this accordingly.
+	 */
+	int_map = 0xFF & (st->intio ? st->int_map : ~st->int_map);
+	pr_debug("%s(): Setting INT_MAP 0x%02X\n", __func__, int_map);
+
+	ret = regmap_write(st->regmap, ADXL345_REG_INT_MAP, int_map);
+	if (ret)
+		return ret;
+
+	pr_debug("%s(): Setting INT_ENABLE 0x%02X\n", __func__, int_enable);
+	ret = regmap_write(st->regmap, ADXL345_REG_INT_ENABLE, int_enable);
+	return ret;
+}
+
 static int adxl345_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan,
 			    int *val, int *val2, long mask)
@@ -330,6 +352,41 @@ static const struct attribute_group adxl345_attrs_group = {
 	.attrs = adxl345_attrs,
 };
 
+static int adxl345_set_fifo(struct adxl34x_state *st)
+{
+	struct adxl34x_platform_data *data = &st->data;
+	u8 fifo_ctl;
+	int ret;
+
+	/* FIFO should be configured while in standby mode */
+	adxl345_set_measure_en(st, false);
+
+	/* The watermark bit is set when the number of samples in FIFO
+	 * equals the value stored in the samples bits (register
+	 * FIFO_CTL). The watermark bit is cleared automatically when
+	 * FIFO is read, and the content returns to a value below the
+	 * value stored in the samples bits.
+	 */
+	fifo_ctl = 0x00 |
+		ADXL345_FIFO_CTL_SAMLPES(data->watermark) |
+		ADXL345_FIFO_CTL_TRIGGER(st->intio) |
+		ADXL345_FIFO_CTL_MODE(data->fifo_mode);
+
+	pr_debug("%s(): fifo_ctl 0x%02X\n", __func__, fifo_ctl);
+
+	/* The watermark bit is set when the number of samples in FIFO
+	 * equals the value stored in the samples bits (register
+	 * FIFO_CTL). The watermark bit is cleared automatically when
+	 * FIFO is read, and the content returns to a value below the
+	 * value stored in the samples bits.
+	 */
+	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_fifo_entries() - Read number of FIFO entries into *fifo_entries.
  * @st: The initialized state instance of this driver.
@@ -416,7 +473,50 @@ static void adxl345_empty_fifo(struct adxl34x_state *st)
 	regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, &regval);
 }
 
+static int adxl345_buffer_postenable(struct iio_dev *indio_dev)
+{
+	struct adxl34x_state *st = iio_priv(indio_dev);
+	struct adxl34x_platform_data *data = &st->data;
+	int ret;
+
+	ret = adxl345_set_interrupts(st);
+	if (ret)
+		return -EINVAL;
+
+	/* Default to FIFO mode: STREAM, since it covers the general usage
+	 * and does not bypass the FIFO
+	 */
+	data->fifo_mode = ADXL_FIFO_STREAM;
+	adxl345_set_fifo(st);
+
+	return 0;
+}
+
+static int adxl345_buffer_predisable(struct iio_dev *indio_dev)
+{
+	struct adxl34x_state *st = iio_priv(indio_dev);
+	struct adxl34x_platform_data *data = &st->data;
+	int ret;
+
+	/* Turn off interrupts */
+	st->int_map = 0x00;
+
+	ret = adxl345_set_interrupts(st);
+	if (ret) {
+		pr_warn("%s(): Failed to disable INTs\n", __func__);
+		return -EINVAL;
+	}
+
+	/* Set FIFO mode: BYPASS, according to the situation */
+	data->fifo_mode = ADXL_FIFO_BYPASS;
+	adxl345_set_fifo(st);
+
+	return 0;
+}
+
 static const struct iio_buffer_setup_ops adxl345_buffer_ops = {
+	.postenable = adxl345_buffer_postenable,
+	.predisable = adxl345_buffer_predisable,
 };
 
 static int adxl345_get_status(struct adxl34x_state *st, u8 *int_stat)
@@ -625,7 +725,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 
 	indio_dev->name = st->info->name;
 	indio_dev->info = &adxl345_info;
-	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
 	indio_dev->channels = adxl34x_channels;
 	indio_dev->num_channels = ARRAY_SIZE(adxl34x_channels);
 
@@ -685,9 +785,6 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 		ret = regmap_write(st->regmap, ADXL345_REG_FIFO_CTL, fifo_ctl);
 		if (ret < 0)
 			return ret;
-
-		/* Enable measurement mode */
-		adxl345_set_measure_en(st, true);
 	}
 	dev_dbg(dev, "Driver operational\n");
 	return devm_iio_device_register(dev, indio_dev);
-- 
2.39.5

[PATCH v2 19/22] iio: accel: adxl345: prepare FIFO watermark handling

[Lothar Rubusch]

Add the feature of the adxl345 and related sensors to manage a FIFO in
stream mode by a watermark level. Provide means to set the watermark
through the IIO api and sysfs interface.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index b57a123ac9..f7b937fb16 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -288,6 +288,26 @@ 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 adxl34x_state *st = iio_priv(indio_dev);
+	struct adxl34x_platform_data *data = &st->data;
+	unsigned int fifo_mask = 0x1F;
+	int ret;
+
+	if (value > ADXL34x_FIFO_SIZE)
+		value = ADXL34x_FIFO_SIZE;
+	pr_debug("%s(): set watermark to 0x%02x\n", __func__, value);
+
+	ret = regmap_update_bits(st->regmap, ADXL345_REG_FIFO_CTL,
+				 fifo_mask, value);
+	if (ret)
+		return ret;
+
+	data->watermark = value;
+	return 0;
+}
+
 static int adxl345_write_raw_get_fmt(struct iio_dev *indio_dev,
 				     struct iio_chan_spec const *chan,
 				     long mask)
@@ -335,7 +355,76 @@ static void adxl345_powerdown(void *ptr)
 	adxl345_set_measure_en(st, false);
 }
 
+/* fifo */
+
+static ssize_t adxl345_get_fifo_enabled(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct adxl34x_state *st = iio_priv(indio_dev);
+	struct adxl34x_platform_data *data = &st->data;
+
+	return sysfs_emit(buf, "%d\n", data->fifo_mode);
+}
+
+static ssize_t adxl345_get_fifo_watermark(struct device *dev,
+					  struct device_attribute *attr,
+					  char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct adxl34x_state *st = iio_priv(indio_dev);
+	struct adxl34x_platform_data *data = &st->data;
+
+	return sprintf(buf, "%d\n", data->watermark);
+}
+
+static ssize_t watermark_en_show(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct adxl34x_state *st = iio_priv(indio_dev);
+
+	return sysfs_emit(buf, "%d\n", st->watermark_en);
+}
+
+static ssize_t watermark_en_store(struct device *dev,
+				  struct device_attribute *attr,
+				  const char *buf, size_t len)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct adxl34x_state *st = iio_priv(indio_dev);
+	bool val;
+
+	if (kstrtobool(buf, &val))
+		return -EINVAL;
+	st->watermark_en = val;
+	return len;
+}
+
+/*
+ * NB: The buffer/hwfifo_watermark is a read-only entry to display the
+ * currently set hardware FIFO watermark. First set a value to buffer0/length.
+ * This allows to configure buffer0/watermark. After enabling buffer0/enable
+ * the hwfifo_watermark shall show the configured FIFO watermark value.
+ *
+ * ref: Documentation/ABI/testing/sysfs-bus-iio
+ */
+IIO_STATIC_CONST_DEVICE_ATTR(hwfifo_watermark_min, "1");
+IIO_STATIC_CONST_DEVICE_ATTR(hwfifo_watermark_max,
+			     __stringify(ADXL34x_FIFO_SIZE));
+static IIO_DEVICE_ATTR(hwfifo_watermark, 0444,
+		       adxl345_get_fifo_watermark, NULL, 0);
+static IIO_DEVICE_ATTR(hwfifo_enabled, 0444,
+		       adxl345_get_fifo_enabled, NULL, 0);
+
+static IIO_DEVICE_ATTR_RW(watermark_en, 0);
+
 static const struct iio_dev_attr *adxl345_fifo_attributes[] = {
+	&iio_dev_attr_hwfifo_watermark_min,
+	&iio_dev_attr_hwfifo_watermark_max,
+	&iio_dev_attr_hwfifo_watermark,
+	&iio_dev_attr_hwfifo_enabled,
 	NULL,
 };
 
@@ -345,6 +434,7 @@ static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
 
 static struct attribute *adxl345_attrs[] = {
 	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
+	&iio_dev_attr_watermark_en.dev_attr.attr,
 	NULL
 };
 
@@ -664,6 +754,7 @@ static const struct iio_info adxl345_info = {
 	.read_raw	= adxl345_read_raw,
 	.write_raw	= adxl345_write_raw,
 	.write_raw_get_fmt	= adxl345_write_raw_get_fmt,
+	.hwfifo_set_watermark = adxl345_set_watermark,
 };
 
 /**
@@ -721,6 +812,9 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 	/* some reasonable pre-initialization */
 	st->data.act_axis_control = 0xFF;
 
+	/* default is all features turned off */
+	st->watermark_en = 0;
+
 	st->intio = ADXL345_INT1;
 
 	indio_dev->name = st->info->name;
-- 
2.39.5

[PATCH v2 20/22] iio: accel: adxl345: use FIFO with watermark IRQ

[Lothar Rubusch]

Enable the watermark feature of the adxl345 sensor. The feature uses a
FIFO to be configured by the IIO fifo sysfs handles. The sensor
configures the FIFO to streaming mode for measurements, or bypass for
configuration. The sensor issues an interrupt on the configured line to
signal several signals (data available, overrun or watermark reached).
The setup allows for extension of further features based on the
interrupt handler and the FIFO setup.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index f7b937fb16..07c336211f 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -870,6 +870,45 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 		if (ret)
 			return dev_err_probe(dev, ret, "Failed to setup triggered buffer\n");
 
+		dev_dbg(dev, "IRQ: allocating data ready trigger\n");
+		st->trig_dready = devm_iio_trigger_alloc(dev,
+							 "%s-dev%d-dready",
+							 indio_dev->name,
+							 iio_device_id(indio_dev));
+		if (!st->trig_dready)
+			return dev_err_probe(dev, -ENOMEM,
+					     "Failed to setup triggered buffer\n");
+		dev_dbg(dev, "IRQ: allocating data ready trigger ok\n");
+
+		st->trig_dready->ops = &adxl34x_trig_dready_ops;
+		dev_dbg(dev, "IRQ: Setting data ready trigger ops ok\n");
+
+		iio_trigger_set_drvdata(st->trig_dready, indio_dev);
+		dev_dbg(dev, "IRQ: Setting up data ready trigger as driver data ok\n");
+
+		ret = devm_iio_trigger_register(dev, st->trig_dready);
+		if (ret < 0)
+			return dev_err_probe(dev, ret,
+					     "Failed to register dready trigger\n");
+		dev_dbg(dev, "Registering data ready trigger ok\n");
+
+		indio_dev->trig = iio_trigger_get(st->trig_dready);
+
+		dev_dbg(dev, "Requesting threaded IRQ, indio_dev->name '%s'\n",
+			indio_dev->name);
+
+		ret = devm_request_threaded_irq(dev, st->irq,
+						iio_trigger_generic_data_rdy_poll,
+						NULL,
+						IRQF_TRIGGER_RISING | IRQF_ONESHOT,
+						indio_dev->name, st->trig_dready);
+		if (ret < 0)
+			return dev_err_probe(dev, ret, "Failed to request IRQ handler\n");
+		dev_dbg(dev, "Requesting threaded IRQ handler ok\n");
+
+		/* Cleanup */
+		adxl345_empty_fifo(st);
+
 	} else { /* Initialization to prepare for FIFO_BYPASS mode (fallback) */
 
 		/* The following defaults to 0x00, anyway */
-- 
2.39.5

[PATCH v2 21/22] iio: accel: adxl345: sync FIFO reading with sensor

[Lothar Rubusch]

Pause the measurement while reading fifo values. Initially an interrupt
is triggered if watermark of the FIFO is reached, or in case of
OVERRUN. The sensor stays mute until FIFO is cleared and interrupts are
read. Situations now can arise when the watermark is configured to a
lower value. While reading the values, new values arrive such that a
permanent OVERRUN state of the FIFO is reached, i.e. either the FIFO
never gets emptied entirely because of permanently new arriving
measurements. No more interrupts will be issued and the setup results
in OVERRUN. To avoid such situation, stop measuring while solving an
OVERRUN condition and generally reading FIFO entries.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 07c336211f..c79f0f5e3b 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -730,6 +730,11 @@ static irqreturn_t adxl345_trigger_handler(int irq, void *p)
 
 	if (int_stat & (ADXL345_INT_DATA_READY | ADXL345_INT_WATERMARK)) {
 		pr_debug("%s(): WATERMARK or DATA_READY event detected\n", __func__);
+
+		/* Pause measuring, at low watermarks this would easily brick the
+		 * sensor in permanent OVERRUN state
+		 */
+		adxl345_set_measure_en(st, false);
 		if (adxl345_get_fifo_entries(st, &fifo_entries) < 0)
 			goto err;
 
@@ -737,12 +742,15 @@ static irqreturn_t adxl345_trigger_handler(int irq, void *p)
 			goto err;
 
 		iio_trigger_notify_done(indio_dev->trig);
+		adxl345_set_measure_en(st, true);
 	}
 
 	goto done;
 err:
 	iio_trigger_notify_done(indio_dev->trig);
+	adxl345_set_measure_en(st, false);
 	adxl345_empty_fifo(st);
+	adxl345_set_measure_en(st, true);
 	return IRQ_NONE;
 
 done:
-- 
2.39.5

[PATCH v2 22/22] iio: accel: adxl345: add debug printout

[Lothar Rubusch]

Add debug prints to allow to debug the sensor based on dynamic debug.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index c79f0f5e3b..8d2166a057 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -26,6 +26,9 @@
 
 #include "adxl345.h"
 
+/* debugging registers */
+#define DEBUG_ADXL345 0
+
 /* ADXL345 register map */
 #define ADXL345_REG_DEVID		0x00 /* r    Device ID */
 #define ADXL345_REG_THRESH_TAP	0x1D /* r/w  Tap Threshold */
@@ -181,6 +184,78 @@ static const struct iio_chan_spec adxl34x_channels[] = {
 	ADXL34x_CHANNEL(2, chan_z, Z),
 };
 
+/*
+ * Debugging
+ */
+
+__maybe_unused
+static void adxl345_debug_registers(const char *func, struct adxl34x_state *st)
+{
+#if DEBUG_ADXL345 == 1
+	struct regmap *regmap = st->regmap;
+	unsigned int regval = 0;
+
+	regmap_read(regmap, ADXL345_REG_THRESH_TAP, &regval);
+	pr_debug("%s(): DEBUG - 0x%02X\t- ADXL345_REG_THRESH_TAP\n",
+			func, 0xff & regval);
+
+	regmap_read(regmap, ADXL345_REG_DUR, &regval);
+	pr_debug("%s(): DEBUG - 0x%02X\t- ADXL345_REG_DUR\n",
+			func, 0xff & regval);
+
+	regmap_read(regmap, ADXL345_REG_LATENT, &regval);
+	pr_debug("%s(): DEBUG - 0x%02X\t- ADXL345_REG_LATENT\n",
+			func, 0xff & regval);
+
+	regmap_read(regmap, ADXL345_REG_WINDOW, &regval);
+	pr_debug("%s(): DEBUG - 0x%02X\t- ADXL345_REG_WINDOW\n",
+			func, 0xff & regval);
+
+	regmap_read(regmap, ADXL345_REG_ACT_TAP_STATUS, &regval);
+	pr_debug("%s(): DEBUG - 0x%02X\t- ADXL345_REG_ACT_TAP_STATUS\n",
+			func, 0xff & regval);
+
+	regmap_read(regmap, ADXL345_REG_POWER_CTL, &regval);
+	pr_debug("%s(): DEBUG - 0x%02X\t- ADXL345_REG_POWER_CTL\n",
+			func, 0xff & regval);
+
+	regmap_read(regmap, ADXL345_REG_INT_ENABLE, &regval);
+	pr_debug("%s(): DEBUG - 0x%02X\t- ADXL345_REG_INT_ENABLE\n",
+			func, 0xff & regval);
+
+	regmap_read(regmap, ADXL345_REG_INT_MAP, &regval);
+	pr_debug("%s(): DEBUG - 0x%02X\t- ADXL345_REG_INT_MAP\n",
+			func, 0xff & regval);
+
+	regmap_read(regmap, ADXL345_REG_INT_SOURCE, &regval);
+	pr_debug("%s(): DEBUG - 0x%02X\t- ADXL345_REG_INT_SOURCE\n",
+			func, 0xff & regval);
+
+	regmap_read(regmap, ADXL345_REG_FIFO_CTL, &regval);
+	pr_debug("%s(): DEBUG - 0x%02X\t- ADXL345_REG_FIFO_CTL\n",
+			func, 0xff & regval);
+
+	regmap_read(regmap, ADXL345_REG_FIFO_STATUS, &regval);
+	pr_debug("%s(): DEBUG - 0x%02X\t- ADXL345_REG_FIFO_STATUS\n",
+			func, 0xff & regval);
+# endif
+}
+
+__maybe_unused
+static void adxl345_debug_fifo(const char *func, s16 *fifo_buf, int entries_line)
+{
+#if DEBUG_ADXL345 == 1
+	s16 xval, yval, zval;
+
+	xval = fifo_buf[0 + entries_line];
+	yval = fifo_buf[1 + entries_line];
+	zval = fifo_buf[2 + entries_line];
+
+	pr_debug("%s(): FIFO[%d] - x=%d, y=%d, z=%d\n",
+		func, entries_line/3, xval, yval, zval);
+#endif
+}
+
 static int adxl345_set_interrupts(struct adxl34x_state *st)
 {
 	int ret;
@@ -528,6 +603,8 @@ static int adxl345_read_fifo_elements(struct adxl34x_state *st, int fifo_entries
 	size_t count, ndirs = 3;
 	int i, ret;
 
+	pr_debug("%s(): fifo_entries = %d\n", __func__, fifo_entries);
+
 	count = 2 * ndirs; /* 2 byte per direction */
 	for (i = 0; i < fifo_entries; i++) {
 		ret = regmap_noinc_read(st->regmap, ADXL345_REG_XYZ_BASE,
@@ -537,6 +614,7 @@ static int adxl345_read_fifo_elements(struct adxl34x_state *st, int fifo_entries
 			return -EFAULT;
 		}
 	}
+	adxl345_debug_registers(__func__, st);
 
 	return 0;
 }
@@ -656,6 +734,7 @@ static int adxl345_push_fifo_data(struct iio_dev *indio_dev,
 		if (st->fifo_delay && (fifo_entries > 1))
 			udelay(3);
 
+		adxl345_debug_fifo(__func__, st->fifo_buf, i);
 		iio_push_to_buffers(indio_dev, &st->fifo_buf[i]);
 	}
 
@@ -683,6 +762,7 @@ static int adxl345_trig_dready(struct iio_trigger *trig, bool state)
 			 __func__, st->int_map);
 	}
 
+	adxl345_debug_registers(__func__, st);
 	return 0;
 }
 
@@ -713,6 +793,7 @@ static irqreturn_t adxl345_trigger_handler(int irq, void *p)
 	int fifo_entries;
 	int ret;
 
+	pr_debug("%s(): IRQ caught!\n", __func__);
 	ret = adxl345_get_status(st, &int_stat);
 	if (ret < 0)
 		goto err;
@@ -747,6 +828,7 @@ static irqreturn_t adxl345_trigger_handler(int irq, void *p)
 
 	goto done;
 err:
+	pr_debug("%s(): error termination\n", __func__);
 	iio_trigger_notify_done(indio_dev->trig);
 	adxl345_set_measure_en(st, false);
 	adxl345_empty_fifo(st);
@@ -754,6 +836,7 @@ static irqreturn_t adxl345_trigger_handler(int irq, void *p)
 	return IRQ_NONE;
 
 done:
+	pr_debug("%s(): regular termination\n", __func__);
 	return IRQ_HANDLED;
 }
 
@@ -804,6 +887,9 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 	st = iio_priv(indio_dev);
 	st->regmap = regmap;
 
+	dev_dbg(dev, "irq '%d'\n", irq);
+	if (!irq) /* fall back to bypass mode w/o IRQ */
+		dev_dbg(dev, "no IRQ, FIFO mode will stay in BYPASS_MODE\n");
 	st->irq = irq;
 	st->info = device_get_match_data(dev);
 	if (!st->info)
@@ -831,7 +917,11 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 	indio_dev->channels = adxl34x_channels;
 	indio_dev->num_channels = ARRAY_SIZE(adxl34x_channels);
 
+	dev_dbg(dev, "setting up indio_dev ok\n");
+
 	if (setup) {
+		dev_dbg(dev, "setup() was provided\n");
+
 		/* Perform optional initial bus specific configuration */
 		ret = setup(dev, st->regmap);
 		if (ret)
@@ -846,6 +936,8 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 					     "Failed to set data range\n");
 
 	} else {
+		dev_dbg(dev, "No setup() provided\n");
+
 		/* Enable full-resolution mode (init all data_format bits) */
 		ret = regmap_write(st->regmap, ADXL345_REG_DATA_FORMAT,
 				   ADXL345_DATA_FORMAT_FULL_RES);
@@ -854,6 +946,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 					     "Failed to set data range\n");
 	}
 
+	dev_dbg(dev, "Retrieving DEVID\n");
 	ret = regmap_read(st->regmap, ADXL345_REG_DEVID, &regval);
 	if (ret < 0)
 		return dev_err_probe(dev, ret, "Error reading device ID\n");
@@ -861,7 +954,9 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 	if (regval != ADXL345_DEVID)
 		return dev_err_probe(dev, -ENODEV, "Invalid device ID: %x, expected %x\n",
 				     regval, ADXL345_DEVID);
+	dev_dbg(dev, "Retrieving DEVID ok\n");
 
+	dev_dbg(dev, "Registering power down function\n");
 	ret = devm_add_action_or_reset(dev, adxl345_powerdown, st);
 	if (ret < 0)
 		return dev_err_probe(dev, ret, "Failed to add action or reset\n");
-- 
2.39.5

Re: [PATCH v2 01/22] iio: accel: adxl345: fix comment on probe

[Jonathan Cameron]

On Sun, 17 Nov 2024 18:26:30 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Fix comment on the probe function. Add covered sensors and fix typo.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl345_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 006ce66c0a..d121caf839 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -170,7 +170,7 @@ static void adxl345_powerdown(void *regmap)
>  
>  /**
>   * adxl345_core_probe() - probe and setup for the adxl345 accelerometer,
> - *                        also covers the adlx375 accelerometer
> + *                        also covers the adxl375 and adxl346 accelerometer
This is too easy to miss updating and to my eyes it doesn't makes sense to
have the list here. How about,

probe and setup for the supported accelerometers.
of just
probe and setup for the accelerometer.

>   * @dev:	Driver model representation of the device
>   * @regmap:	Regmap instance for the device
>   * @setup:	Setup routine to be executed right before the standard device

Re: [PATCH v2 03/22] iio: accel: adxl345: rename struct adxl34x_state

[Jonathan Cameron]

On Sun, 17 Nov 2024 18:26:32 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Rename the struct "adxl345_data" to "adxl34x_state". First, the
> data structure is supposed to be extended to represent state rather than
> only hold sensor data. The data will be a separate member pointer.
> Second, the driver not only covers the adxl345 accelerometer, it also
> supports the adxl345, adxl346 and adxl375. Thus "adxl34x_" is a choice
> for a common prefix.
No to the wild card x.  Long experience has shown that manufacturers
almost never respect naming sequences so we simply name everything after
one supported part number.  It seems like a good idea, but we've been
bitten before.  

I'm fine with the _data to _state change but please combine with the previous
patch to void unnecessary churn where the same line gets updated twice.

These two changes are very closely related so fine to do them in one patch
in my opinion.

> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl345_core.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 3fb7a7b1b7..30896555a4 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -17,7 +17,7 @@
>  
>  #include "adxl345.h"
>  
> -struct adxl345_data {
> +struct adxl34x_state {
>  	const struct adxl345_chip_info *info;
>  	struct regmap *regmap;
>  };
> @@ -43,7 +43,7 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
>  			    struct iio_chan_spec const *chan,
>  			    int *val, int *val2, long mask)
>  {
> -	struct adxl345_data *st = iio_priv(indio_dev);
> +	struct adxl34x_state *st = iio_priv(indio_dev);
>  	__le16 accel;
>  	long long samp_freq_nhz;
>  	unsigned int regval;
> @@ -99,7 +99,7 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
>  			     struct iio_chan_spec const *chan,
>  			     int val, int val2, long mask)
>  {
> -	struct adxl345_data *st = iio_priv(indio_dev);
> +	struct adxl34x_state *st = iio_priv(indio_dev);
>  	s64 n;
>  
>  	switch (mask) {
> @@ -181,7 +181,7 @@ static void adxl345_powerdown(void *regmap)
>  int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  		       int (*setup)(struct device*, struct regmap*))
>  {
> -	struct adxl345_data *st;
> +	struct adxl34x_state *st;
>  	struct iio_dev *indio_dev;
>  	u32 regval;
>  	unsigned int data_format_mask = (ADXL345_DATA_FORMAT_RANGE |

Re: [PATCH v2 04/22] iio: accel: adxl345: rename to adxl34x_channels

[Jonathan Cameron]

On Sun, 17 Nov 2024 18:26:33 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Rename the "adxl345_channels" to "adxl34x_channels". The driver
> supports several Analog accelerometers equally, e.g. adxl346 and
> adxl375.
As with previous a no to this.

We have a few drivers doing wild cards, but most are from before
I realised how often this goes wrong and started pretty strongly
insisting on naming after a single support part. It seems
so attractive, but ends up hurting us more than the confusion of
a single name applying to other parts as well.

Jonathan

> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl345_core.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 30896555a4..2b62e79248 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -33,7 +33,7 @@ struct adxl34x_state {
>  		BIT(IIO_CHAN_INFO_SAMP_FREQ),				\
>  }
>  
> -static const struct iio_chan_spec adxl345_channels[] = {
> +static const struct iio_chan_spec adxl34x_channels[] = {
>  	ADXL345_CHANNEL(0, X),
>  	ADXL345_CHANNEL(1, Y),
>  	ADXL345_CHANNEL(2, Z),
> @@ -203,8 +203,8 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  	indio_dev->name = st->info->name;
>  	indio_dev->info = &adxl345_info;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
> -	indio_dev->channels = adxl345_channels;
> -	indio_dev->num_channels = ARRAY_SIZE(adxl345_channels);
> +	indio_dev->channels = adxl34x_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(adxl34x_channels);
>  
>  	if (setup) {
>  		/* Perform optional initial bus specific configuration */

Re: [PATCH v2 05/22] iio: accel: adxl345: measure right-justified

[Jonathan Cameron]

On Sun, 17 Nov 2024 18:26:34 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

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

Surely this can't be changed independent of other changes as it will
change the format of the data we are processing?

Each change must stand on it's own so that I can apply up to any
point in your patch set and have everything continue to work.


Jonathan

> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl345_core.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 2b62e79248..926e397678 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -184,8 +184,13 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  	struct adxl34x_state *st;
>  	struct iio_dev *indio_dev;
>  	u32 regval;
> +
> +	/* NB: ADXL345_DATA_FORMAT_JUSTIFY or 0:
	/*
	 * NB: AD...

is the multiline comment style all IIO drivers use (and most of the kernel
except for networking.

> +	 * do right-justified: 0, then adjust resolution according to 10-bit
> +	 * through 13-bit in channel - this is the default behavior, and can
> +	 * be modified here by oring ADXL345_DATA_FORMAT_JUSTIFY
> +	 */
>  	unsigned int data_format_mask = (ADXL345_DATA_FORMAT_RANGE |
> -					 ADXL345_DATA_FORMAT_JUSTIFY |
>  					 ADXL345_DATA_FORMAT_FULL_RES |
>  					 ADXL345_DATA_FORMAT_SELF_TEST);
>  	int ret;

Re: [PATCH v2 06/22] iio: accel: adxl345: add function to switch measuring

[Jonathan Cameron]

On Sun, 17 Nov 2024 18:26:35 +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. This is needed for several
> features of the accelerometer. It allows to change e.g. FIFO settings.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl345_core.c | 53 ++++++++++++++++++++++----------
>  1 file changed, 37 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 926e397678..81688a9eaf 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -138,6 +138,39 @@ 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 adxl34x_state *st, bool en)
> +{
> +	unsigned int val = 0;
> +	int ret;
> +
> +	val = (en) ? ADXL345_POWER_CTL_MEASURE : ADXL345_POWER_CTL_STANDBY;
> +	ret = regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
> +	if (ret)
> +		return -EINVAL;
Don't eat the error response from regmap_write. I will probably be more
useful than simple -EINVAL if we get a problem.

	return regmap_write()

> +
> +	return 0;
> +}
> +
> +static void adxl345_powerdown(void *ptr)
> +{
> +	struct adxl34x_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 +191,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 adxl345 accelerometer,
>   *                        also covers the adxl375 and adxl346 accelerometer
> @@ -242,14 +265,12 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  		return dev_err_probe(dev, -ENODEV, "Invalid device ID: %x, expected %x\n",
>  				     regval, ADXL345_DEVID);
>  
> -	/* Enable measurement mode */
> -	ret = adxl345_powerup(st->regmap);
> +	ret = devm_add_action_or_reset(dev, adxl345_powerdown, st);

This should be done after the call to turn the device on.  Otherwise we are powering
down a device that is already powered down if this returns an error.
So original ordering looks correct to me.


>  	if (ret < 0)
> -		return dev_err_probe(dev, ret, "Failed to enable measurement mode\n");
> +		return dev_err_probe(dev, ret, "Failed to add action or reset\n");
>  
> -	ret = devm_add_action_or_reset(dev, adxl345_powerdown, st->regmap);
> -	if (ret < 0)
> -		return ret;
> +	/* Enable measurement mode */
> +	adxl345_set_measure_en(st, true);
Check if this fails.

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

Re: [PATCH v2 07/22] iio: accel: adxl345: initialize IRQ number

[Jonathan Cameron]

On Sun, 17 Nov 2024 18:26:36 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add the possibility to claim an interrupt and init the state structure
> with interrupt number and interrupt line to use. The adxl345 can use
> two different interrupt lines, mainly to signal FIFO watermark events,
> single or double tap, activity, etc. Hence, having the interrupt line
> available is crucial to implement such features.

If there are two interrupt lines, you need to be more clever.
Imagine only one of them is wired. How do you know which one it is?

The query needs to be done by name.  When there are multiple interrupts
the ones found in spi and i2c structures could be anything, so don't use
those.

See fwnode_irq_get_by_name()


> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl345.h      | 1 +
>  drivers/iio/accel/adxl345_core.c | 6 ++++++
>  drivers/iio/accel/adxl345_i2c.c  | 2 +-
>  drivers/iio/accel/adxl345_spi.c  | 8 ++++++--
>  4 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> index 3d5c8719db..cf4132715c 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,
> +		       int irq,
>  		       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 81688a9eaf..902bd3568b 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -11,6 +11,7 @@
>  #include <linux/property.h>
>  #include <linux/regmap.h>
>  #include <linux/units.h>
> +#include <linux/interrupt.h>
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> @@ -18,6 +19,7 @@
>  #include "adxl345.h"
>  
>  struct adxl34x_state {
> +	int irq;
>  	const struct adxl345_chip_info *info;
>  	struct regmap *regmap;
>  };
> @@ -196,12 +198,14 @@ static const struct iio_info adxl345_info = {
>   *                        also covers the adxl375 and adxl346 accelerometer
>   * @dev:	Driver model representation of the device
>   * @regmap:	Regmap instance for the device
> + * @irq:	Interrupt handling for async usage

This is an integer, not a handling.   See if you can come up with clearer comment.

>   * @setup:	Setup routine to be executed right before the standard device
>   *		setup
>   *
>   * Return: 0 on success, negative errno on error
>   */
>  int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> +		       int irq,
>  		       int (*setup)(struct device*, struct regmap*))
>  {
>  	struct adxl34x_state *st;
> @@ -224,6 +228,8 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  
>  	st = iio_priv(indio_dev);
>  	st->regmap = regmap;
> +
> +	st->irq = irq;
>  	st->info = device_get_match_data(dev);
>  	if (!st->info)
>  		return -ENODEV;
> diff --git a/drivers/iio/accel/adxl345_i2c.c b/drivers/iio/accel/adxl345_i2c.c
> index 4065b8f7c8..604b706c29 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, client->irq, 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 61fd9a6f5f..39e7d71e1d 100644
> --- a/drivers/iio/accel/adxl345_spi.c
> +++ b/drivers/iio/accel/adxl345_spi.c
> @@ -39,9 +39,13 @@ static int adxl345_spi_probe(struct spi_device *spi)
>  		return dev_err_probe(&spi->dev, PTR_ERR(regmap), "Error initializing regmap\n");
>  
>  	if (spi->mode & SPI_3WIRE)
> -		return adxl345_core_probe(&spi->dev, regmap, adxl345_spi_setup);
> +		return adxl345_core_probe(&spi->dev, regmap,
> +					  spi->irq,
> +					  adxl345_spi_setup);
Very early wrap. I think spi->irq fits on the line above.

>  	else
> -		return adxl345_core_probe(&spi->dev, regmap, NULL);
> +		return adxl345_core_probe(&spi->dev, regmap,
> +					  spi->irq,
> +					  NULL);
>  }
>  
>  static const struct adxl345_chip_info adxl345_spi_info = {

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

[Jonathan Cameron]

On Sun, 17 Nov 2024 18:26:37 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add the possibility to delay FIFO access when SPI is used. According to
> the datasheet this is needed for the adxl345. When initialization
> happens over SPI the need for delay is to be signalized, and the delay
> will be used.
A specific reference to a section of the specification might be useful
here.

One trivial comment inline, but otherwise looks fine to me.

> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl345.h      | 2 +-
>  drivers/iio/accel/adxl345_core.c | 6 +++++-
>  drivers/iio/accel/adxl345_i2c.c  | 2 +-
>  drivers/iio/accel/adxl345_spi.c  | 3 +++
>  4 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> index cf4132715c..4ba493f636 100644
> --- a/drivers/iio/accel/adxl345.h
> +++ b/drivers/iio/accel/adxl345.h
> @@ -62,7 +62,7 @@ struct adxl345_chip_info {
>  };
>  
>  int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> -		       int irq,
> +		       int irq, 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 902bd3568b..51b229cc44 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -22,6 +22,7 @@ struct adxl34x_state {
>  	int irq;
>  	const struct adxl345_chip_info *info;
>  	struct regmap *regmap;
> +	bool fifo_delay; /* delay: delay is needed for SPI */
>  };
>  
>  #define ADXL345_CHANNEL(index, axis) {					\
> @@ -199,13 +200,14 @@ static const struct iio_info adxl345_info = {
>   * @dev:	Driver model representation of the device
>   * @regmap:	Regmap instance for the device
>   * @irq:	Interrupt handling for async usage
> + * @fifo_delay_default: Using FIFO with SPI needs delay
>   * @setup:	Setup routine to be executed right before the standard device
>   *		setup
>   *
>   * Return: 0 on success, negative errno on error
>   */
>  int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> -		       int irq,
> +		       int irq, bool fifo_delay_default,
>  		       int (*setup)(struct device*, struct regmap*))
>  {
>  	struct adxl34x_state *st;
> @@ -234,6 +236,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 604b706c29..fa1b7e7026 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, client->irq, NULL);
> +	return adxl345_core_probe(&client->dev, regmap, client->irq, 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 39e7d71e1d..75940d9c1c 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,
> @@ -41,10 +42,12 @@ static int adxl345_spi_probe(struct spi_device *spi)
>  	if (spi->mode & SPI_3WIRE)
>  		return adxl345_core_probe(&spi->dev, regmap,
>  					  spi->irq,
> +					  spi->max_speed_hz > ADXL345_MAX_FREQ_NO_FIFO_DELAY,
>  					  adxl345_spi_setup);
>  	else
>  		return adxl345_core_probe(&spi->dev, regmap,
>  					  spi->irq,
> +					  spi->max_speed_hz > ADXL345_MAX_FREQ_NO_FIFO_DELAY,

use a local variable to establish this without the very long line.

>  					  NULL);
>  }
>  

Re: [PATCH v2 09/22] iio: accel: adxl345: unexpose private defines

[Jonathan Cameron]

On Sun, 17 Nov 2024 18:26:38 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> For the implementation of features like FIFO-usage, watermark, single
> tap, double tap, freefall, etc. most of the constants do not need to be
> exposed in the header file, but are rather of private nature. Reduce
> namespace pollution by moving them to the core source file.
Whilst I get where you are coming from with this, breaking up
where these are between some in the header and some in the main code
tends to hurt readability and ease of checking the definitions.

So I would prefer these remain in the header.

Jonathan

> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 51b229cc44..c8d9e1f9e0 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -18,6 +18,114 @@
>  
>  #include "adxl345.h"

> +/* POWER_CTL bits */
Drop all the extra comments unless they add something not obvious
for the naming.
> +#define ADXL345_POWER_CTL_STANDBY	0x00
> +
> +/* NB:
> + * BIT(0), BIT(1) for explicit wakeup (not implemented)
> + * BIT(2) for explicit sleep (not implemented)
Define them and don't use them and the comment isn't needed.

> + */
> +#define ADXL345_POWER_CTL_MEASURE	BIT(3)
> +#define ADXL345_POWER_CTL_AUTO_SLEEP	BIT(4)
> +#define ADXL345_POWER_CTL_LINK	BIT(5)
> +
> +/* DATA_FORMAT bits */

The naming of the defines makes that clear. So the comment doesn't
add much.

> +#define ADXL345_DATA_FORMAT_RANGE	GENMASK(1, 0)	/* Set the g range */
> +#define ADXL345_DATA_FORMAT_JUSTIFY	BIT(2)	/* 1: left-justified (MSB) mode, 0: right-just'd */
If the comment is needed move it to the line above.
better yet, use a name that means the comment isn't needed.
ADXL345_DATA_FORMAT_LEFT_JUSTIFY
for example where a value of 0 means left and 1 doesn't (hence right)

You are just moving it though, so perhaps not worth improving.

> +#define ADXL345_DATA_FORMAT_FULL_RES	BIT(3)	/* Up to 13-bits resolution */
> +/* NB: BIT(6): 3-wire SPI mode (defined in header) */
This is the sort of comment that indicates the problem with splitting
things between header and here. I'd prefer to just keep it all in the header.

> +
> +#define ADXL345_DATA_FORMAT_SELF_TEST	BIT(7)	/* Enable a self test */
> +#define ADXL345_DATA_FORMAT_2G		0
> +#define ADXL345_DATA_FORMAT_4G		1
> +#define ADXL345_DATA_FORMAT_8G		2
> +#define ADXL345_DATA_FORMAT_16G		3
> +
> +#define ADXL345_REG_OFS_AXIS(index)	(ADXL345_REG_OFSX + (index))
> +
> +/* The ADXL345 include a 32 sample FIFO
Comment syntax 
/*
 * The ADXL345 includes a 32 sample FIFO.
> + *
> + * FIFO stores a maximum of 32 entries, which equates to a maximum of 33
> + * entries available at any given time because an additional entry is available
> + * at the output filter of the device.
> + * (see datasheet FIFO_STATUS description on "Entries Bits")
> + */
> +#define ADXL34x_FIFO_SIZE  33
> +
>  struct adxl34x_state {
>  	int irq;
>  	const struct adxl345_chip_info *info;

Re: [PATCH v2 10/22] iio: accel: adxl345: set interrupt line to INT1

[Jonathan Cameron]

On Sun, 17 Nov 2024 18:26:39 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> The adxl345 sensor uses one of two interrupt lines, INT1 or INT2. The
> interrupt lines are used to signal feature events such as watermark
> reached, single tap, double tap, activity, etc. Only one interrupt line
> is used and must be configured. The adxl345 default is to use INT1 and
> in many installations only INT1 is even connected. Therefore configure
> INT1 as the sensor's default interrupt line.
In others only INT2 is connected. Hence earlier comment and you will need
the driver to detect which is connected by using interrupt names
and then set this up appropriately.

It would be fine to also have the driver just fail to probe on the
INT2 only case, but you must check for it rather than routing interrupts
to a potentially unconnected pin.

Jonathan

> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl345_core.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index c8d9e1f9e0..32163cfe6f 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -131,6 +131,7 @@ struct adxl34x_state {
>  	const struct adxl345_chip_info *info;
>  	struct regmap *regmap;
>  	bool fifo_delay; /* delay: delay is needed for SPI */
> +	u8 intio;
>  };
>  
>  #define ADXL345_CHANNEL(index, axis) {					\
> @@ -345,6 +346,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  		return -ENODEV;
>  
>  	st->fifo_delay = fifo_delay_default;
> +	st->intio = ADXL345_INT1;
>  
>  	indio_dev->name = st->info->name;
>  	indio_dev->info = &adxl345_info;

Re: [PATCH v2 11/22] iio: accel: adxl345: import adxl345 general data

[Jonathan Cameron]

On Sun, 17 Nov 2024 18:26:40 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Use struct adxl34x_platform_data from the included public header of the
> input driver for ADXL34x with the following argumentation for this
> approach:
> 
> - The iio driver for the ADXL34x covers features also implemented in
>   the older input driver. The iio driver will implement the same
>   features but can also benefit from including the common header and
>   struct adxl34x_platform_data. Once complete, the input driver could
>   be faded out.

In general platform_data is long dead means that predates hardware configuratino
coming from device tree.

Such a header is legacy that we want to get rid of, not reuse.

> 
> - The fields in the input driver are identical to the fields the IIO
>   implementation will need. Including the header over reimplementing,
>   avoids a duplication of almost identical headers in IIO and iio.
> 
> - The header for the input driver is public. It provides a public
>   interface for adxl34x related implementation and is not private to
>   the input system.

There are no users of that header in the upstream kernel. Better
move would be to proposed deleting it and hard coding the defaults the
input driver.  That should shake out if anyone is using it in a downstream
kernel (I'd be surprised if they are).



> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl345_core.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 32163cfe6f..b16887ec1c 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -16,6 +16,8 @@
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
>  
> +#include <linux/input/adxl34x.h>
> +
>  #include "adxl345.h"
>  
>  /* ADXL345 register map */
> @@ -126,10 +128,15 @@
>   */
>  #define ADXL34x_FIFO_SIZE  33
>  
> +static const struct adxl34x_platform_data adxl345_default_init = {
> +	.tap_axis_control = ADXL_TAP_X_EN | ADXL_TAP_Y_EN | ADXL_TAP_Z_EN,
> +};
> +
>  struct adxl34x_state {
>  	int irq;
>  	const struct adxl345_chip_info *info;
>  	struct regmap *regmap;
> +	struct adxl34x_platform_data data;  /* watermark, fifo_mode, etc */
>  	bool fifo_delay; /* delay: delay is needed for SPI */
>  	u8 intio;
>  };
> @@ -331,6 +338,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  	unsigned int data_format_mask = (ADXL345_DATA_FORMAT_RANGE |
>  					 ADXL345_DATA_FORMAT_FULL_RES |
>  					 ADXL345_DATA_FORMAT_SELF_TEST);
> +	const struct adxl34x_platform_data *data;
>  	int ret;
>  
>  	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> @@ -346,6 +354,16 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  		return -ENODEV;
>  
>  	st->fifo_delay = fifo_delay_default;
> +	data = dev_get_platdata(dev);
> +	if (!data) {
> +		dev_dbg(dev, "No platform data: Using default initialization\n");
> +		data = &adxl345_default_init;
> +	}
> +	st->data = *data;
> +
> +	/* some reasonable pre-initialization */
> +	st->data.act_axis_control = 0xFF;
> +
>  	st->intio = ADXL345_INT1;
>  
>  	indio_dev->name = st->info->name;

Re: [PATCH v2 12/22] iio: accel: adxl345: elaborate iio channel definition

[Jonathan Cameron]

On Sun, 17 Nov 2024 18:26:41 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Make the channel definition ready to allow for feature implementation
> for this accelerometer sensor.

Add what you need for each patch that makes use of the new data.
A separate change like this is not easy to review without seeing those
usecases and a reviewer should not need to go look for them in other
patches.

> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl345_core.c | 36 +++++++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index b16887ec1c..f98ddef4c5 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -141,21 +141,33 @@ struct adxl34x_state {
>  	u8 intio;
>  };
>  
> -#define ADXL345_CHANNEL(index, axis) {					\
> -	.type = IIO_ACCEL,						\
> -	.modified = 1,							\
> -	.channel2 = IIO_MOD_##axis,					\
> -	.address = index,						\
> -	.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),				\
> +#define ADXL34x_CHANNEL(index, reg, axis) {				\
> +		.type = IIO_ACCEL,					\

No reason to change the indentation.
That just makes this patch hard to read as I can't immeidately see what changed.

> +			.address = (reg),				\
> +			.modified = 1,					\
> +			.channel2 = IIO_MOD_##axis,			\
> +			.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 = {					\
Bring the scan stuff in as part of a patch that makes use of it. Not
beofre that.

> +				.sign = 's',				\
> +				.realbits = 13,				\
> +				.storagebits = 16,			\
> +				.shift = 0,				\
> +				.endianness = IIO_LE,			\
> +			},						\
>  }
>  
> +enum adxl34x_chans {
> +	chan_x, chan_y, chan_z,
> +};
> +
>  static const struct iio_chan_spec adxl34x_channels[] = {
> -	ADXL345_CHANNEL(0, X),
> -	ADXL345_CHANNEL(1, Y),
> -	ADXL345_CHANNEL(2, Z),
> +	ADXL34x_CHANNEL(0, chan_x, X),
> +	ADXL34x_CHANNEL(1, chan_y, Y),
> +	ADXL34x_CHANNEL(2, chan_z, Z),
>  };
>  
>  static int adxl345_read_raw(struct iio_dev *indio_dev,

Re: [PATCH v2 13/22] iio: accel: adxl345: add trigger handler

[Jonathan Cameron]

On Sun, 17 Nov 2024 18:26:42 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add basic setup to the interrupt handler function and prepare probe
> for using and not using the FIFO on the adxl345. Interrupt handler and
> basic structure integration is needed to evaluate interrupt source
> register. This is crucial for implementing further features of the
> sensor.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl345_core.c | 105 ++++++++++++++++++++++++++++++-
>  1 file changed, 103 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index f98ddef4c5..508de81bb9 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -15,6 +15,9 @@
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
Probably not appropriate to have a trigger here unless you make it work
with other triggers (e.g. Sysfs, hrtimer etc).
When a fifo is involved the usual control is no trigger means fifo, applying
a trigger means not through fifo.

>  
>  #include <linux/input/adxl34x.h>
>  
> @@ -137,6 +140,7 @@ struct adxl34x_state {
>  	const struct adxl345_chip_info *info;
>  	struct regmap *regmap;
>  	struct adxl34x_platform_data data;  /* watermark, fifo_mode, etc */
> +	u8 int_map;
Introduce only when used...
>  	bool fifo_delay; /* delay: delay is needed for SPI */
>  	u8 intio;
>  };
> @@ -302,6 +306,10 @@ static void adxl345_powerdown(void *ptr)
>  	adxl345_set_measure_en(st, false);
>  }
>  
> +static const struct iio_dev_attr *adxl345_fifo_attributes[] = {
> +	NULL,
Bring this in when it has content, not before.

> +};
> +
>  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"
>  );
> @@ -315,6 +323,75 @@ static const struct attribute_group adxl345_attrs_group = {
>  	.attrs = adxl345_attrs,
>  };
>  
> +static const struct iio_buffer_setup_ops adxl345_buffer_ops = {
> +};
> +
> +static int adxl345_get_status(struct adxl34x_state *st, u8 *int_stat)
> +{
> +	int ret;
> +	unsigned int regval;
> +
> +	*int_stat = 0;

Why not return it?
Negative error, value if positive.

> +	ret = regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, &regval);
> +	if (ret) {
> +		pr_warn("%s(): Failed to read INT_SOURCE register\n", __func__);
dev_err()
Though odd to do that for this specific read and no others.

> +		return -EINVAL;
> +	}
> +
> +	*int_stat = 0xff & regval;
> +	pr_debug("%s(): int_stat 0x%02X (INT_SOURCE)\n", __func__, *int_stat);
Clean these debug statements out to only keep the particularly useful ones.
> +
> +	return 0;
> +}
> +
> +/**
> + * irqreturn_t adxl345_trigger_handler() - Interrupt handler used for several
> + *                                         features of the ADXL345.
If it is used for anything other than triggers, you should rethink that naming.

You are probably better off registering the interrupt directly.  See comments
on not using a trigger below. They aren't always appropriate.

> + * @irq: The interrupt number.
> + * @p: The iio poll function instance, used to derive the device and data.
> + *
> + * Having an interrupt imples FIFO_STREAM mode was enabled. Since this is given
> + * there will no be further test for being in FIFO_BYPASS mode. FIFO_TRIGGER
> + * and FIFO_FIFO mode (being similar to FIFO_STREAM mode) are not separately
> + * implemented so far. Both should be work smoothly with the same way of
> + * interrupt handling.
> + *
> + * Return: Describe if the interrupt was handled.
> + */
> +static irqreturn_t adxl345_trigger_handler(int irq, void *p)
> +{
> +	struct iio_dev *indio_dev = ((struct iio_poll_func *) p)->indio_dev;
> +	struct adxl34x_state *st = iio_priv(indio_dev);
> +	u8 int_stat;
> +	int ret;
> +
> +	ret = adxl345_get_status(st, &int_stat);
> +	if (ret < 0)
> +		goto done;
> +
> +	/* Ignore already read event by reissued too fast */
> +	if (int_stat == 0x0)
> +		goto done;
> +
> +	/* evaluation */
Don't see this comment as useful. Probably drop it or expand to make
it more useful.
> +
> +	if (int_stat & ADXL345_INT_OVERRUN) {
> +		pr_debug("%s(): OVERRUN event detected\n", __func__);
dev_dbg()
> +		goto done;
> +	}
> +
> +	if (int_stat & (ADXL345_INT_DATA_READY | ADXL345_INT_WATERMARK))
> +		pr_debug("%s(): WATERMARK or DATA_READY event detected\n", __func__);
Bring this in only when you actually do something in the handler.
Much better to supply a full function in one go so it can be easily reviewed.
> +
> +	goto done;
> +done:
> +
> +	if (indio_dev)
> +		iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static const struct iio_info adxl345_info = {
>  	.attrs		= &adxl345_attrs_group,
>  	.read_raw	= adxl345_read_raw,
> @@ -351,6 +428,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  					 ADXL345_DATA_FORMAT_FULL_RES |
>  					 ADXL345_DATA_FORMAT_SELF_TEST);
>  	const struct adxl34x_platform_data *data;
> +	u8 fifo_ctl;
>  	int ret;
>  
>  	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> @@ -419,9 +497,32 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  	if (ret < 0)
>  		return dev_err_probe(dev, ret, "Failed to add action or reset\n");
>  
> -	/* Enable measurement mode */
> -	adxl345_set_measure_en(st, true);
> +	/* Basic common initialization of the driver is done now */

Code flow comments like this are rarely useful and often become wrong
as a driver evolves. Better to not add it.

> +
> +	if (st->irq) { /* Initialization to prepare for FIFO_STREAM mode */
> +		ret = devm_iio_triggered_buffer_setup_ext(dev, indio_dev,
If you are supporting only fifo mode, why use a triggered buffer?
Those require one interrupt (or trigger) per scan (here read of all the axes).

For a device using only fifo based accesses it is fine to have no trigger.
If you look around you will find various drivers calling 
devm_iio_kfifo_buffer_setup() directly - look at how they do things.
There is an ext version of that as well.


> +							  NULL,
> +							  adxl345_trigger_handler,
> +							  IIO_BUFFER_DIRECTION_IN,
> +							  &adxl345_buffer_ops,
> +							  adxl345_fifo_attributes);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "Failed to setup triggered buffer\n");
> +
> +	} else { /* Initialization to prepare for FIFO_BYPASS mode (fallback) */
>  
> +		/* The following defaults to 0x00, anyway */
Not a lot of point in an | with 0x00 
I'm not sure the comment adds any value.

> +		fifo_ctl = 0x00 | ADXL345_FIFO_CTL_MODE(ADXL_FIFO_BYPASS);
> +
> +		dev_dbg(dev, "fifo_ctl 0x%02X [0x00]\n", fifo_ctl);

Not generally worth keeping this level of low level dbg in driver. There is infrastructure
in regmap for tracing if you really need to know.

> +		ret = regmap_write(st->regmap, ADXL345_REG_FIFO_CTL, fifo_ctl);
> +		if (ret < 0)
> +			return ret;
> +
> +		/* Enable measurement mode */
> +		adxl345_set_measure_en(st, true);
Check for errors.

> +	}
> +	dev_dbg(dev, "Driver operational\n");
Drop this from code for upstream. It is wrong anyway as if the next call fails
it is definitely not operational.

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

Re: [PATCH v2 14/22] iio: accel: adxl345: read FIFO entries

[Jonathan Cameron]

On Sun, 17 Nov 2024 18:26:43 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add a function to read the elements of the adxl345 FIFO. This will
> flush the FIFO, and brings it into a ready state. The read out
> is used to read the elemnts and to reset the fifo again. The cleanup
> equally needs a read on the INT_SOURCE register.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl345_core.c | 40 ++++++++++++++++++++++++++++++--
>  1 file changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 508de81bb9..40e78dbdb0 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -140,6 +140,8 @@ struct adxl34x_state {
>  	const struct adxl345_chip_info *info;
>  	struct regmap *regmap;
>  	struct adxl34x_platform_data data;  /* watermark, fifo_mode, etc */
> +
> +	__le16 fifo_buf[3 * ADXL34x_FIFO_SIZE] __aligned(IIO_DMA_MINALIGN);
That doesn't work.  Look at why we have IIO_DMA_MINALIGN().
Hint is that the buffer needs to end up in a cacheline with nothing that might
be written by software in the same line.  int_map and below will be so this
is not doing it's job.

Also not used in this patch.

>  	u8 int_map;
>  	bool fifo_delay; /* delay: delay is needed for SPI */
>  	u8 intio;
> @@ -323,6 +325,37 @@ static const struct attribute_group adxl345_attrs_group = {
>  	.attrs = adxl345_attrs,
>  };
>  
> +/**
> + * adxl345_get_fifo_entries() - Read number of FIFO entries into *fifo_entries.
> + * @st: The initialized state instance of this driver.
> + * @fifo_entries: A field to be initialized by this function with the number of
> + * FIFO entries.
> + *
> + * Since one read on the FIFO is reading all three axis, X, Y and Z in one, the
> + * number of FIFO entries corresponds to the number of triples of those values.
> + * Note, this sensor does not support treating any axis individually, or
> + * exclude them from measuring.
> + *
> + * Return: 0 or error value.
> + */
> +static int adxl345_get_fifo_entries(struct adxl34x_state *st, int *fifo_entries)
> +{
> +	unsigned int regval = 0;
> +	int ret;
> +
> +	ret = regmap_read(st->regmap, ADXL345_REG_FIFO_STATUS, &regval);
> +	if (ret) {
> +		pr_warn("%s(): Failed to read FIFO_STATUS register\n", __func__);
As before, seems overkill to warn int his path but not really anywhere else.
dev_err() if you do want to.
> +		*fifo_entries = 0;
> +		return ret;
> +	}
> +
> +	*fifo_entries = 0x3f & regval;
> +	pr_debug("%s(): fifo_entries %d\n", __func__, *fifo_entries);
> +
> +	return 0;
return the value (similar comment to previous patch review.)
> +}
> +
>  static const struct iio_buffer_setup_ops adxl345_buffer_ops = {
>  };
>  
> @@ -363,6 +396,7 @@ static irqreturn_t adxl345_trigger_handler(int irq, void *p)
>  	struct iio_dev *indio_dev = ((struct iio_poll_func *) p)->indio_dev;
>  	struct adxl34x_state *st = iio_priv(indio_dev);
>  	u8 int_stat;
> +	int fifo_entries;
>  	int ret;
>  
>  	ret = adxl345_get_status(st, &int_stat);
> @@ -380,9 +414,11 @@ static irqreturn_t adxl345_trigger_handler(int irq, void *p)
>  		goto done;
>  	}
>  
> -	if (int_stat & (ADXL345_INT_DATA_READY | ADXL345_INT_WATERMARK))
> +	if (int_stat & (ADXL345_INT_DATA_READY | ADXL345_INT_WATERMARK)) {
>  		pr_debug("%s(): WATERMARK or DATA_READY event detected\n", __func__);
> -
> +		if (adxl345_get_fifo_entries(st, &fifo_entries) < 0)
Do it all in one go. I want to see interrupt through to data hitting the kfifo +
registration of that kfifo all in one patch.

It is not complicated enough to need to jump through this very slow introduction
of code.

Jonathan

> +			goto done;
> +	}
>  	goto done;
>  done:
>  

Re: [PATCH v2 15/22] iio: accel: adxl345: reset the FIFO on error

[Jonathan Cameron]

On Sun, 17 Nov 2024 18:26:44 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add a function to empty the FIFO and reset the INT_SOURCE register.
> Reading out is used to reset the fifo again. For cleanup also a read
> on the INT_SOURCE register is needed to allow the adxl345 to issue
> interrupts again. Without clearing the fields no further interrupts
> will happen.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl345_core.c | 75 ++++++++++++++++++++++++++++----
>  1 file changed, 67 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 40e78dbdb0..82bd5c2b78 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -356,6 +356,61 @@ static int adxl345_get_fifo_entries(struct adxl34x_state *st, int *fifo_entries)
>  	return 0;
>  }
>  
> +/**
> + * adxl345_read_fifo_elements() - Read fifo_entries number of elements.
> + * @st: The instance of the state object of this sensor.
> + * @fifo_entries: The number of lines in the FIFO referred to as fifo_entry,
> + * a fifo_entry has 3 elements for X, Y and Z direction of 2 bytes each.
> + *
> + * The FIFO of the sensor is read linewise. The read measurement values are
> + * queued in the corresponding data structure in *st.
> + *
> + * It is recommended that a multiple-byte read of all registers be performed to
> + * prevent a change in data between reads of sequential registers. That is to
> + * read out the data registers X0, X1, Y0, Y1, Z0, Z1 at once.

To ensure this, set avail_scan_modes.
Then if the user requests a subset, the IIO core code will extract what is necessary
from the read of everythign.


> + *
> + * Return: 0 or error value.
> + */
> +static int adxl345_read_fifo_elements(struct adxl34x_state *st, int fifo_entries)
> +{
> +	size_t count, ndirs = 3;
> +	int i, ret;
> +
> +	count = 2 * ndirs; /* 2 byte per direction */
sizeof(st->fifo_buf[0] * ndirs);

> +	for (i = 0; i < fifo_entries; i++) {
> +		ret = regmap_noinc_read(st->regmap, ADXL345_REG_XYZ_BASE,
> +				st->fifo_buf + (i * count / 2), count);
> +		if (ret) {
> +			pr_warn("%s(): regmap_noinc_read() failed\n", __func__);
> +			return -EFAULT;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * adxl345_empty_fifo() - Empty the FIFO.
> + * @st: The instance to the state object of the sensor.
> + *
> + * Reading all elements of the FIFO linewise empties the FIFO. Reading th
> + * interrupt source register resets the sensor. This is needed also in case of
> + * overflow or error handling to reenable the sensor to issue interrupts.
> + */
> +static void adxl345_empty_fifo(struct adxl34x_state *st)
> +{
> +	int regval;
> +	int fifo_entries;
> +
> +	/* In case the HW is not "clean" just read out remaining elements */
> +	adxl345_get_fifo_entries(st, &fifo_entries);
> +	if (fifo_entries > 0)
> +		adxl345_read_fifo_elements(st, fifo_entries);
> +
> +	/* Reset the INT_SOURCE register by reading the register */
> +	regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, &regval);
> +}
> +
>  static const struct iio_buffer_setup_ops adxl345_buffer_ops = {
>  };
>  
> @@ -401,30 +456,34 @@ static irqreturn_t adxl345_trigger_handler(int irq, void *p)
>  
>  	ret = adxl345_get_status(st, &int_stat);
>  	if (ret < 0)
> -		goto done;
> +		goto err;
All this churn just makes things less readable.  Better to have the bulk
of the addition of fifo handling in one patch. It won't be too large
for review.
>  
>  	/* Ignore already read event by reissued too fast */
>  	if (int_stat == 0x0)
> -		goto done;
> +		goto err;
>  
>  	/* evaluation */
>  
>  	if (int_stat & ADXL345_INT_OVERRUN) {
>  		pr_debug("%s(): OVERRUN event detected\n", __func__);
> -		goto done;
> +		goto err;
>  	}
>  
>  	if (int_stat & (ADXL345_INT_DATA_READY | ADXL345_INT_WATERMARK)) {
>  		pr_debug("%s(): WATERMARK or DATA_READY event detected\n", __func__);
>  		if (adxl345_get_fifo_entries(st, &fifo_entries) < 0)
> -			goto done;
> -	}
> -	goto done;
> -done:
> +			goto err;
>  
> -	if (indio_dev)
>  		iio_trigger_notify_done(indio_dev->trig);
> +	}
>  
> +	goto done;
> +err:
> +	iio_trigger_notify_done(indio_dev->trig);
> +	adxl345_empty_fifo(st);
> +	return IRQ_NONE;
NONE is probably a bad idea. Something went wrong but that doesn't
mean it wasn't our interrupt.  In most cases it is better to just
return that we handled it.  IRQ_NONE might be valid if the status
said it wasn't ours.

> +
> +done:
>  	return IRQ_HANDLED;
return where you have goto done and get rid of this.

>  }
>  

Re: [PATCH v2 16/22] iio: accel: adxl345: register trigger ops

[Jonathan Cameron]

On Sun, 17 Nov 2024 18:26:45 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add trigger options to the sensor driver. Reacting to the sensor events
> communicated by IRQ, the FIFO handling and the trigger will be core
> events for further feature implementation.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl345_core.c | 34 ++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 82bd5c2b78..d58e1994ff 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -15,6 +15,9 @@
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/kfifo_buf.h>
>  #include <linux/iio/trigger.h>
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/iio/triggered_buffer.h>
> @@ -140,11 +143,13 @@ struct adxl34x_state {
>  	const struct adxl345_chip_info *info;
>  	struct regmap *regmap;
>  	struct adxl34x_platform_data data;  /* watermark, fifo_mode, etc */
> +	struct iio_trigger *trig_dready;
>  
>  	__le16 fifo_buf[3 * ADXL34x_FIFO_SIZE] __aligned(IIO_DMA_MINALIGN);
>  	u8 int_map;
>  	bool fifo_delay; /* delay: delay is needed for SPI */
>  	u8 intio;
> +	bool watermark_en;
>  };
>  
>  #define ADXL34x_CHANNEL(index, reg, axis) {				\
> @@ -432,6 +437,35 @@ static int adxl345_get_status(struct adxl34x_state *st, u8 *int_stat)
>  	return 0;
>  }
>  
> +/* data ready trigger */

Data ready is a term used for a single sample being ready. 
Rename this as watermark.

However, as before I'd just get rid of the trigger entirely as it's not useful.
> +
> +static int adxl345_trig_dready(struct iio_trigger *trig, bool state)
> +{
> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> +	struct adxl34x_state *st = iio_priv(indio_dev);
> +
> +	st->int_map = 0x00;
> +	if (state) {
> +		/* Setting also ADXL345_INT_DATA_READY results in just a single
> +		 * generated interrupt, and no continuously re-generation. NB that the
> +		 * INT_DATA_READY as well as the INT_OVERRUN are managed automatically,
> +		 * setting their bits here is not needed.
> +		 */
> +		if (st->watermark_en)
> +			st->int_map |= ADXL345_INT_WATERMARK;
> +
> +		pr_debug("%s(): preparing st->int_map 0x%02X\n",
> +			 __func__, st->int_map);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct iio_trigger_ops adxl34x_trig_dready_ops = {
> +	.validate_device = &iio_trigger_validate_own_device,
> +	.set_trigger_state = adxl345_trig_dready,
> +};
> +
>  /**
>   * irqreturn_t adxl345_trigger_handler() - Interrupt handler used for several
>   *                                         features of the ADXL345.

Re: [PATCH v2 17/22] iio: accel: adxl345: push FIFO data to iio

[Jonathan Cameron]

On Sun, 17 Nov 2024 18:26:46 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add FIFO and hwfifo handling. Add some functions to deal with FIFO
> entries and configuration. This feature will be needed for e.g.
> watermark setting.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl345_core.c | 38 ++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index d58e1994ff..a653774db8 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -437,6 +437,41 @@ static int adxl345_get_status(struct adxl34x_state *st, u8 *int_stat)
>  	return 0;
>  }
>  
> +static int adxl345_push_fifo_data(struct iio_dev *indio_dev,
> +				  u8 status,
> +				  int fifo_entries)
> +{
> +	struct adxl34x_state *st = iio_priv(indio_dev);
> +	int ndirs = 3; /* 3 directions */
It's const. Maybe a define is appropriate instead?
> +	int i, ret;
> +
> +	if (fifo_entries <= 0)
> +		return true;
It returns an int. Also how did you get in here with negative fifo
entries?  That rather suggests something went wrong at the caller.

> +
> +	ret = adxl345_read_fifo_elements(st, fifo_entries);
> +	if (ret)
> +		return false;
> +
> +	for (i = 0; i < ndirs * fifo_entries; i += ndirs) {
> +		/* To ensure that the FIFO has completely popped, there must be at least 5
Comment format.

> +		 * 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 && (fifo_entries > 1))
> +			udelay(3);
> +
> +		iio_push_to_buffers(indio_dev, &st->fifo_buf[i]);
> +	}
> +
> +	return true;
> +}
> +

Re: [PATCH v2 18/22] iio: accel: adxl345: start measure at buffer en/disable

[Jonathan Cameron]

On Sun, 17 Nov 2024 18:26:47 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add and initialize the buffer options to use the FIFO and watermark
> feature of the adxl345 sensor. In this way measure enable will happen
> in at enabling the buffer.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
Various minor comments.  In general I haven't commented on every
instance of formatting problems etc. Please generalize though all the
patches based on comments that are made.

Thanks,

Jonathan

> ---
>  drivers/iio/accel/adxl345_core.c | 105 +++++++++++++++++++++++++++++--
>  1 file changed, 101 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index a653774db8..b57a123ac9 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -181,6 +181,28 @@ static const struct iio_chan_spec adxl34x_channels[] = {
>  	ADXL34x_CHANNEL(2, chan_z, Z),
>  };
>  
> +static int adxl345_set_interrupts(struct adxl34x_state *st)
> +{
> +	int ret;
> +	unsigned int int_enable = st->int_map;
> +	unsigned int int_map;
> +
> +	/* Any bits set to 0 in the INT map register send their respective
> +	 * interrupts to the INT1 pin, whereas bits set to 1 send their respective
> +	 * interrupts to the INT2 pin. The intio shall convert this accordingly.
> +	 */
> +	int_map = 0xFF & (st->intio ? st->int_map : ~st->int_map);
> +	pr_debug("%s(): Setting INT_MAP 0x%02X\n", __func__, int_map);
> +
> +	ret = regmap_write(st->regmap, ADXL345_REG_INT_MAP, int_map);
> +	if (ret)
> +		return ret;
> +
> +	pr_debug("%s(): Setting INT_ENABLE 0x%02X\n", __func__, int_enable);
> +	ret = regmap_write(st->regmap, ADXL345_REG_INT_ENABLE, int_enable);
> +	return ret;
> +}
> +
>  static int adxl345_read_raw(struct iio_dev *indio_dev,
>  			    struct iio_chan_spec const *chan,
>  			    int *val, int *val2, long mask)
> @@ -330,6 +352,41 @@ static const struct attribute_group adxl345_attrs_group = {
>  	.attrs = adxl345_attrs,
>  };
>  
> +static int adxl345_set_fifo(struct adxl34x_state *st)
> +{
> +	struct adxl34x_platform_data *data = &st->data;
> +	u8 fifo_ctl;
> +	int ret;
> +
> +	/* FIFO should be configured while in standby mode */
> +	adxl345_set_measure_en(st, false);
Check for errors.
> +
> +	/* The watermark bit is set when the number of samples in FIFO
> +	 * equals the value stored in the samples bits (register
> +	 * FIFO_CTL). The watermark bit is cleared automatically when
> +	 * FIFO is read, and the content returns to a value below the
> +	 * value stored in the samples bits.
> +	 */
> +	fifo_ctl = 0x00 |
What does the 0x00 bring?  Get rid of that.
> +		ADXL345_FIFO_CTL_SAMLPES(data->watermark) |
> +		ADXL345_FIFO_CTL_TRIGGER(st->intio) |
> +		ADXL345_FIFO_CTL_MODE(data->fifo_mode);
> +
> +	pr_debug("%s(): fifo_ctl 0x%02X\n", __func__, fifo_ctl);
> +
> +	/* The watermark bit is set when the number of samples in FIFO
> +	 * equals the value stored in the samples bits (register
> +	 * FIFO_CTL). The watermark bit is cleared automatically when
> +	 * FIFO is read, and the content returns to a value below the
> +	 * value stored in the samples bits.
> +	 */
> +	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_fifo_entries() - Read number of FIFO entries into *fifo_entries.
>   * @st: The initialized state instance of this driver.
> @@ -416,7 +473,50 @@ static void adxl345_empty_fifo(struct adxl34x_state *st)
>  	regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, &regval);
>  }
>  
> +static int adxl345_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct adxl34x_state *st = iio_priv(indio_dev);
> +	struct adxl34x_platform_data *data = &st->data;
> +	int ret;
> +
> +	ret = adxl345_set_interrupts(st);
> +	if (ret)
> +		return -EINVAL;
> +
> +	/* Default to FIFO mode: STREAM, since it covers the general usage
> +	 * and does not bypass the FIFO
> +	 */
> +	data->fifo_mode = ADXL_FIFO_STREAM;
> +	adxl345_set_fifo(st);
check for errors and probably try and unwind what was done in here if you
get one.

> +
> +	return 0;
> +}
> +
> +static int adxl345_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct adxl34x_state *st = iio_priv(indio_dev);
> +	struct adxl34x_platform_data *data = &st->data;
> +	int ret;
> +
> +	/* Turn off interrupts */
> +	st->int_map = 0x00;
> +
> +	ret = adxl345_set_interrupts(st);
> +	if (ret) {
> +		pr_warn("%s(): Failed to disable INTs\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	/* Set FIFO mode: BYPASS, according to the situation */
> +	data->fifo_mode = ADXL_FIFO_BYPASS;
> +	adxl345_set_fifo(st);
always check for errors.

> +
> +	return 0;
> +}

>  
>  static int adxl345_get_status(struct adxl34x_state *st, u8 *int_stat)
> @@ -625,7 +725,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  
>  	indio_dev->name = st->info->name;
>  	indio_dev->info = &adxl345_info;
> -	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;

That is set when you register a triggered buffer.

>  	indio_dev->channels = adxl34x_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(adxl34x_channels);

Re: [PATCH v2 19/22] iio: accel: adxl345: prepare FIFO watermark handling

[Jonathan Cameron]

On Sun, 17 Nov 2024 18:26:48 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add the feature of the adxl345 and related sensors to manage a FIFO in
> stream mode by a watermark level. Provide means to set the watermark
> through the IIO api and sysfs interface.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>

> +/*
> + * NB: The buffer/hwfifo_watermark is a read-only entry to display the
> + * currently set hardware FIFO watermark. First set a value to buffer0/length.
> + * This allows to configure buffer0/watermark. After enabling buffer0/enable
> + * the hwfifo_watermark shall show the configured FIFO watermark value.
> + *
> + * ref: Documentation/ABI/testing/sysfs-bus-iio
> + */
> +IIO_STATIC_CONST_DEVICE_ATTR(hwfifo_watermark_min, "1");
> +IIO_STATIC_CONST_DEVICE_ATTR(hwfifo_watermark_max,
> +			     __stringify(ADXL34x_FIFO_SIZE));
> +static IIO_DEVICE_ATTR(hwfifo_watermark, 0444,
> +		       adxl345_get_fifo_watermark, NULL, 0);
> +static IIO_DEVICE_ATTR(hwfifo_enabled, 0444,
> +		       adxl345_get_fifo_enabled, NULL, 0);
> +
> +static IIO_DEVICE_ATTR_RW(watermark_en, 0);
> +
>  static const struct iio_dev_attr *adxl345_fifo_attributes[] = {
> +	&iio_dev_attr_hwfifo_watermark_min,
> +	&iio_dev_attr_hwfifo_watermark_max,
> +	&iio_dev_attr_hwfifo_watermark,
> +	&iio_dev_attr_hwfifo_enabled,
>  	NULL,
Introduce the whole array only when you bring in the entries (so here).
>  };
>  
> @@ -345,6 +434,7 @@ static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
>  
>  static struct attribute *adxl345_attrs[] = {
>  	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
> +	&iio_dev_attr_watermark_en.dev_attr.attr,

Non standard ABI.  This should be indirectly controlled by the
watermark on the software buffer IIRC. (it's been a while
since I last looked at that). 

If the watermark passed to hwfifo_set_watermark is 0 then turn it off.
Otherwise turn it on.


Jonathan

>  	NULL
>  };

Re: [PATCH v2 20/22] iio: accel: adxl345: use FIFO with watermark IRQ

[Jonathan Cameron]

On Sun, 17 Nov 2024 18:26:49 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Enable the watermark feature of the adxl345 sensor. The feature uses a
> FIFO to be configured by the IIO fifo sysfs handles. The sensor
> configures the FIFO to streaming mode for measurements, or bypass for
> configuration. The sensor issues an interrupt on the configured line to
> signal several signals (data available, overrun or watermark reached).
> The setup allows for extension of further features based on the
> interrupt handler and the FIFO setup.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl345_core.c | 39 ++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index f7b937fb16..07c336211f 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -870,6 +870,45 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  		if (ret)
>  			return dev_err_probe(dev, ret, "Failed to setup triggered buffer\n");
>  
> +		dev_dbg(dev, "IRQ: allocating data ready trigger\n");
> +		st->trig_dready = devm_iio_trigger_alloc(dev,
> +							 "%s-dev%d-dready",
> +							 indio_dev->name,
> +							 iio_device_id(indio_dev));
> +		if (!st->trig_dready)
> +			return dev_err_probe(dev, -ENOMEM,
> +					     "Failed to setup triggered buffer\n");
> +		dev_dbg(dev, "IRQ: allocating data ready trigger ok\n");
Drop these debug prints from code you want to go upstream. Mostly it is easy
to find out what they provide via other means and they hurt code redability.
> +
> +		st->trig_dready->ops = &adxl34x_trig_dready_ops;
> +		dev_dbg(dev, "IRQ: Setting data ready trigger ops ok\n");
> +
> +		iio_trigger_set_drvdata(st->trig_dready, indio_dev);
> +		dev_dbg(dev, "IRQ: Setting up data ready trigger as driver data ok\n");
> +
> +		ret = devm_iio_trigger_register(dev, st->trig_dready);
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to register dready trigger\n");
> +		dev_dbg(dev, "Registering data ready trigger ok\n");
> +
> +		indio_dev->trig = iio_trigger_get(st->trig_dready);
you want to set it as immutable as this code doesn't currently work with other
triggers or with this removed.  This is subject to just getting rid of the trigger
in general a suggested earlier.
> +
> +		dev_dbg(dev, "Requesting threaded IRQ, indio_dev->name '%s'\n",
> +			indio_dev->name);
> +
> +		ret = devm_request_threaded_irq(dev, st->irq,
> +						iio_trigger_generic_data_rdy_poll,
It's not. It's on a watermark interrupt. That's what strongly implies treating this
as a trigger is a bad idea.
> +						NULL,
> +						IRQF_TRIGGER_RISING | IRQF_ONESHOT,

Direction should be coming from firmware so not set here.

Note there are some older drivers where we do set it in the driver, and we can't
'fix' those because there may be hardware with broken firmware relying on that.
However no new cases of this.

> +						indio_dev->name, st->trig_dready);
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret, "Failed to request IRQ handler\n");
> +		dev_dbg(dev, "Requesting threaded IRQ handler ok\n");
> +
> +		/* Cleanup */
> +		adxl345_empty_fifo(st);
> +
>  	} else { /* Initialization to prepare for FIFO_BYPASS mode (fallback) */
>  
>  		/* The following defaults to 0x00, anyway */

Re: [PATCH v2 21/22] iio: accel: adxl345: sync FIFO reading with sensor

[Jonathan Cameron]

On Sun, 17 Nov 2024 18:26:50 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Pause the measurement while reading fifo values. Initially an interrupt
> is triggered if watermark of the FIFO is reached, or in case of
> OVERRUN. The sensor stays mute until FIFO is cleared and interrupts are
> read. Situations now can arise when the watermark is configured to a
> lower value. While reading the values, new values arrive such that a
> permanent OVERRUN state of the FIFO is reached, i.e. either the FIFO
> never gets emptied entirely because of permanently new arriving
> measurements. No more interrupts will be issued and the setup results
> in OVERRUN. To avoid such situation, stop measuring while solving an
> OVERRUN condition and generally reading FIFO entries.
Whilst solving overrun it makes sense.  Whilst dealing with a watermark
interrupt not so much.  The whole point of those is to allow capture
of data without missing samples. We should continue to acquire data
during the readback.

> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl345_core.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 07c336211f..c79f0f5e3b 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -730,6 +730,11 @@ static irqreturn_t adxl345_trigger_handler(int irq, void *p)
>  
>  	if (int_stat & (ADXL345_INT_DATA_READY | ADXL345_INT_WATERMARK)) {
>  		pr_debug("%s(): WATERMARK or DATA_READY event detected\n", __func__);
> +
> +		/* Pause measuring, at low watermarks this would easily brick the
> +		 * sensor in permanent OVERRUN state
> +		 */
> +		adxl345_set_measure_en(st, false);
>  		if (adxl345_get_fifo_entries(st, &fifo_entries) < 0)
>  			goto err;
>  
> @@ -737,12 +742,15 @@ static irqreturn_t adxl345_trigger_handler(int irq, void *p)
>  			goto err;
>  
>  		iio_trigger_notify_done(indio_dev->trig);
> +		adxl345_set_measure_en(st, true);
>  	}
>  
>  	goto done;
>  err:
>  	iio_trigger_notify_done(indio_dev->trig);
> +	adxl345_set_measure_en(st, false);
>  	adxl345_empty_fifo(st);
> +	adxl345_set_measure_en(st, true);
>  	return IRQ_NONE;
>  
>  done:

Re: [PATCH v2 22/22] iio: accel: adxl345: add debug printout

[Jonathan Cameron]

On Sun, 17 Nov 2024 18:26:51 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add debug prints to allow to debug the sensor based on dynamic debug.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
We have infrastructure to read registers stuff in debugfs. Use that not
prints to the kernel log.

For the others stuff only keep stuff that we can get via ftrace
or similar if we really want to know the fine detail of what ran.

Jonathan


> ---
>  drivers/iio/accel/adxl345_core.c | 95 ++++++++++++++++++++++++++++++++
>  1 file changed, 95 insertions(+)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index c79f0f5e3b..8d2166a057 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -26,6 +26,9 @@
>  
>  #include "adxl345.h"
>  
> +/* debugging registers */
> +#define DEBUG_ADXL345 0
> +
>  /* ADXL345 register map */
>  #define ADXL345_REG_DEVID		0x00 /* r    Device ID */
>  #define ADXL345_REG_THRESH_TAP	0x1D /* r/w  Tap Threshold */
> @@ -181,6 +184,78 @@ static const struct iio_chan_spec adxl34x_channels[] = {
>  	ADXL34x_CHANNEL(2, chan_z, Z),
>  };
>  
> +/*
> + * Debugging
> + */
> +
> +__maybe_unused
> +static void adxl345_debug_registers(const char *func, struct adxl34x_state *st)
> +{
> +#if DEBUG_ADXL345 == 1
> +	struct regmap *regmap = st->regmap;
> +	unsigned int regval = 0;
> +
> +	regmap_read(regmap, ADXL345_REG_THRESH_TAP, &regval);
> +	pr_debug("%s(): DEBUG - 0x%02X\t- ADXL345_REG_THRESH_TAP\n",
> +			func, 0xff & regval);
> +
> +	regmap_read(regmap, ADXL345_REG_DUR, &regval);
> +	pr_debug("%s(): DEBUG - 0x%02X\t- ADXL345_REG_DUR\n",
> +			func, 0xff & regval);
> +
> +	regmap_read(regmap, ADXL345_REG_LATENT, &regval);
> +	pr_debug("%s(): DEBUG - 0x%02X\t- ADXL345_REG_LATENT\n",
> +			func, 0xff & regval);
> +
> +	regmap_read(regmap, ADXL345_REG_WINDOW, &regval);
> +	pr_debug("%s(): DEBUG - 0x%02X\t- ADXL345_REG_WINDOW\n",
> +			func, 0xff & regval);
> +
> +	regmap_read(regmap, ADXL345_REG_ACT_TAP_STATUS, &regval);
> +	pr_debug("%s(): DEBUG - 0x%02X\t- ADXL345_REG_ACT_TAP_STATUS\n",
> +			func, 0xff & regval);
> +
> +	regmap_read(regmap, ADXL345_REG_POWER_CTL, &regval);
> +	pr_debug("%s(): DEBUG - 0x%02X\t- ADXL345_REG_POWER_CTL\n",
> +			func, 0xff & regval);
> +
> +	regmap_read(regmap, ADXL345_REG_INT_ENABLE, &regval);
> +	pr_debug("%s(): DEBUG - 0x%02X\t- ADXL345_REG_INT_ENABLE\n",
> +			func, 0xff & regval);
> +
> +	regmap_read(regmap, ADXL345_REG_INT_MAP, &regval);
> +	pr_debug("%s(): DEBUG - 0x%02X\t- ADXL345_REG_INT_MAP\n",
> +			func, 0xff & regval);
> +
> +	regmap_read(regmap, ADXL345_REG_INT_SOURCE, &regval);
> +	pr_debug("%s(): DEBUG - 0x%02X\t- ADXL345_REG_INT_SOURCE\n",
> +			func, 0xff & regval);
> +
> +	regmap_read(regmap, ADXL345_REG_FIFO_CTL, &regval);
> +	pr_debug("%s(): DEBUG - 0x%02X\t- ADXL345_REG_FIFO_CTL\n",
> +			func, 0xff & regval);
> +
> +	regmap_read(regmap, ADXL345_REG_FIFO_STATUS, &regval);
> +	pr_debug("%s(): DEBUG - 0x%02X\t- ADXL345_REG_FIFO_STATUS\n",
> +			func, 0xff & regval);
> +# endif
> +}
> +
> +__maybe_unused
> +static void adxl345_debug_fifo(const char *func, s16 *fifo_buf, int entries_line)
> +{
> +#if DEBUG_ADXL345 == 1
> +	s16 xval, yval, zval;
> +
> +	xval = fifo_buf[0 + entries_line];
> +	yval = fifo_buf[1 + entries_line];
> +	zval = fifo_buf[2 + entries_line];
> +
> +	pr_debug("%s(): FIFO[%d] - x=%d, y=%d, z=%d\n",
> +		func, entries_line/3, xval, yval, zval);
> +#endif
> +}
> +
>  static int adxl345_set_interrupts(struct adxl34x_state *st)
>  {
>  	int ret;
> @@ -528,6 +603,8 @@ static int adxl345_read_fifo_elements(struct adxl34x_state *st, int fifo_entries
>  	size_t count, ndirs = 3;
>  	int i, ret;
>  
> +	pr_debug("%s(): fifo_entries = %d\n", __func__, fifo_entries);
> +
>  	count = 2 * ndirs; /* 2 byte per direction */
>  	for (i = 0; i < fifo_entries; i++) {
>  		ret = regmap_noinc_read(st->regmap, ADXL345_REG_XYZ_BASE,
> @@ -537,6 +614,7 @@ static int adxl345_read_fifo_elements(struct adxl34x_state *st, int fifo_entries
>  			return -EFAULT;
>  		}
>  	}
> +	adxl345_debug_registers(__func__, st);
>  
>  	return 0;
>  }
> @@ -656,6 +734,7 @@ static int adxl345_push_fifo_data(struct iio_dev *indio_dev,
>  		if (st->fifo_delay && (fifo_entries > 1))
>  			udelay(3);
>  
> +		adxl345_debug_fifo(__func__, st->fifo_buf, i);
>  		iio_push_to_buffers(indio_dev, &st->fifo_buf[i]);
>  	}
>  
> @@ -683,6 +762,7 @@ static int adxl345_trig_dready(struct iio_trigger *trig, bool state)
>  			 __func__, st->int_map);
>  	}
>  
> +	adxl345_debug_registers(__func__, st);
>  	return 0;
>  }
>  
> @@ -713,6 +793,7 @@ static irqreturn_t adxl345_trigger_handler(int irq, void *p)
>  	int fifo_entries;
>  	int ret;
>  
> +	pr_debug("%s(): IRQ caught!\n", __func__);
>  	ret = adxl345_get_status(st, &int_stat);
>  	if (ret < 0)
>  		goto err;
> @@ -747,6 +828,7 @@ static irqreturn_t adxl345_trigger_handler(int irq, void *p)
>  
>  	goto done;
>  err:
> +	pr_debug("%s(): error termination\n", __func__);
>  	iio_trigger_notify_done(indio_dev->trig);
>  	adxl345_set_measure_en(st, false);
>  	adxl345_empty_fifo(st);
> @@ -754,6 +836,7 @@ static irqreturn_t adxl345_trigger_handler(int irq, void *p)
>  	return IRQ_NONE;
>  
>  done:
> +	pr_debug("%s(): regular termination\n", __func__);
>  	return IRQ_HANDLED;
>  }
>  
> @@ -804,6 +887,9 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  	st = iio_priv(indio_dev);
>  	st->regmap = regmap;
>  
> +	dev_dbg(dev, "irq '%d'\n", irq);
> +	if (!irq) /* fall back to bypass mode w/o IRQ */
> +		dev_dbg(dev, "no IRQ, FIFO mode will stay in BYPASS_MODE\n");
>  	st->irq = irq;
>  	st->info = device_get_match_data(dev);
>  	if (!st->info)
> @@ -831,7 +917,11 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  	indio_dev->channels = adxl34x_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(adxl34x_channels);
>  
> +	dev_dbg(dev, "setting up indio_dev ok\n");
> +
>  	if (setup) {
> +		dev_dbg(dev, "setup() was provided\n");
> +
>  		/* Perform optional initial bus specific configuration */
>  		ret = setup(dev, st->regmap);
>  		if (ret)
> @@ -846,6 +936,8 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  					     "Failed to set data range\n");
>  
>  	} else {
> +		dev_dbg(dev, "No setup() provided\n");
> +
>  		/* Enable full-resolution mode (init all data_format bits) */
>  		ret = regmap_write(st->regmap, ADXL345_REG_DATA_FORMAT,
>  				   ADXL345_DATA_FORMAT_FULL_RES);
> @@ -854,6 +946,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  					     "Failed to set data range\n");
>  	}
>  
> +	dev_dbg(dev, "Retrieving DEVID\n");
>  	ret = regmap_read(st->regmap, ADXL345_REG_DEVID, &regval);
>  	if (ret < 0)
>  		return dev_err_probe(dev, ret, "Error reading device ID\n");
> @@ -861,7 +954,9 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  	if (regval != ADXL345_DEVID)
>  		return dev_err_probe(dev, -ENODEV, "Invalid device ID: %x, expected %x\n",
>  				     regval, ADXL345_DEVID);
> +	dev_dbg(dev, "Retrieving DEVID ok\n");
>  
> +	dev_dbg(dev, "Registering power down function\n");
>  	ret = devm_add_action_or_reset(dev, adxl345_powerdown, st);
>  	if (ret < 0)
>  		return dev_err_probe(dev, ret, "Failed to add action or reset\n");

Re: [PATCH v2 05/22] iio: accel: adxl345: measure right-justified

[Lothar Rubusch]

Dear IIO Mailing-List, Hi Jonathan!

Thank you so much for the review. As you probably saw, most (all?) of
my commits have a huge invisible question mark attached. Most of my
questions you answered clearly. On particular topics I'd like to get
back, though. Generally I will try to apply the requested changes to
best of my understanding.

On Sun, Nov 24, 2024 at 7:07 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sun, 17 Nov 2024 18:26:34 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Make measurements right-justified, since it is the default for the
> > driver and sensor. By not setting the ADXL345_DATA_FORMAT_JUSTIFY bit,
> > the data becomes right-judstified. This was the original setting, there
> > is no reason to change it to left-justified, where right-justified
> > simplifies working on the registers.
>
> Surely this can't be changed independent of other changes as it will
> change the format of the data we are processing?
>
> Each change must stand on it's own so that I can apply up to any
> point in your patch set and have everything continue to work.

This is probably not quite clear. Originally the driver was
right-justified. One of my last commits
(f68ebfe1501bf1110eebf5e968c4d9186cba8706) changed the driver to work
with left-justified measurements. So, I feel changing the orginal
behavior is wrong, and here I try to re-establish the original driver
behavior.

When looking at the datasheet right-justified data seems to be easier
to handle, but I don't have any personal preference.

Lothar

[...]
> > ---
> >  drivers/iio/accel/adxl345_core.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > index 2b62e79248..926e397678 100644
> > --- a/drivers/iio/accel/adxl345_core.c
> > +++ b/drivers/iio/accel/adxl345_core.c
> > @@ -184,8 +184,13 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> >       struct adxl34x_state *st;
> >       struct iio_dev *indio_dev;
> >       u32 regval;
> > +
> > +     /* NB: ADXL345_DATA_FORMAT_JUSTIFY or 0:
>         /*
>          * NB: AD...
>
> is the multiline comment style all IIO drivers use (and most of the kernel
> except for networking.
>
> > +      * do right-justified: 0, then adjust resolution according to 10-bit
> > +      * through 13-bit in channel - this is the default behavior, and can
> > +      * be modified here by oring ADXL345_DATA_FORMAT_JUSTIFY
> > +      */
> >       unsigned int data_format_mask = (ADXL345_DATA_FORMAT_RANGE |
> > -                                      ADXL345_DATA_FORMAT_JUSTIFY |
> >                                        ADXL345_DATA_FORMAT_FULL_RES |
> >                                        ADXL345_DATA_FORMAT_SELF_TEST);
> >       int ret;
>

Re: [PATCH v2 07/22] iio: accel: adxl345: initialize IRQ number

[Lothar Rubusch]

On Sun, Nov 24, 2024 at 7:14 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sun, 17 Nov 2024 18:26:36 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Add the possibility to claim an interrupt and init the state structure
> > with interrupt number and interrupt line to use. The adxl345 can use
> > two different interrupt lines, mainly to signal FIFO watermark events,
> > single or double tap, activity, etc. Hence, having the interrupt line
> > available is crucial to implement such features.
>
> If there are two interrupt lines, you need to be more clever.
> Imagine only one of them is wired. How do you know which one it is?
>
> The query needs to be done by name.  When there are multiple interrupts
> the ones found in spi and i2c structures could be anything, so don't use
> those.
>
> See fwnode_irq_get_by_name()

One of my bigger question marks is this here: The sensor has two
possible interrupt lines, INT1 or INT2, on which it will notify. But,
only one is connected. Hence, the irq passed e.g. by SPI will
automatically refer to the used one. Only this one is going to be
configured as irq. Or, am I getting this wrong? Alternatively, no
interrupt line is connected. This was the initial driver setup. In
this case, it's generally BYPASS-mode for the FIFO and any further
feature is not possible due to a lack of notification.

My questions now arise, when it comes to configure the IRQ line in the
sensor registers. The sensor needs to be configured, that the used
interrupt line for notifiction is INT1 or INT2. In the later patch of
this series - "set interrupt line to INT1" - I init it with "INT1".
But, this actually then can be configured. I can think of yet another
/sysfs handle (could be dynamically changed at runtime, I'm not sure
if this makes sense); by an additional devicetree binding (currently
my preferred idea, I'd default to INT1, but let it configure to INT1
or INT2 - or better default to bypass mode?), or is there an IIO way
of configuring it to INT1/2, or a better way? I rather tend to DT. But
this is still not part of this patch set.

Do I miss a point here and should still apply the
fwnode_irq_get_by_name()? This looks a bit like the DT approach, isn't
it?

Lothar

> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > ---
> >  drivers/iio/accel/adxl345.h      | 1 +
> >  drivers/iio/accel/adxl345_core.c | 6 ++++++
> >  drivers/iio/accel/adxl345_i2c.c  | 2 +-
> >  drivers/iio/accel/adxl345_spi.c  | 8 ++++++--
> >  4 files changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> > index 3d5c8719db..cf4132715c 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,
> > +                    int irq,
> >                      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 81688a9eaf..902bd3568b 100644
> > --- a/drivers/iio/accel/adxl345_core.c
> > +++ b/drivers/iio/accel/adxl345_core.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/property.h>
> >  #include <linux/regmap.h>
> >  #include <linux/units.h>
> > +#include <linux/interrupt.h>
> >
> >  #include <linux/iio/iio.h>
> >  #include <linux/iio/sysfs.h>
> > @@ -18,6 +19,7 @@
> >  #include "adxl345.h"
> >
> >  struct adxl34x_state {
> > +     int irq;
> >       const struct adxl345_chip_info *info;
> >       struct regmap *regmap;
> >  };
> > @@ -196,12 +198,14 @@ static const struct iio_info adxl345_info = {
> >   *                        also covers the adxl375 and adxl346 accelerometer
> >   * @dev:     Driver model representation of the device
> >   * @regmap:  Regmap instance for the device
> > + * @irq:     Interrupt handling for async usage
>
> This is an integer, not a handling.   See if you can come up with clearer comment.
>
> >   * @setup:   Setup routine to be executed right before the standard device
> >   *           setup
> >   *
> >   * Return: 0 on success, negative errno on error
> >   */
> >  int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> > +                    int irq,
> >                      int (*setup)(struct device*, struct regmap*))
> >  {
> >       struct adxl34x_state *st;
> > @@ -224,6 +228,8 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> >
> >       st = iio_priv(indio_dev);
> >       st->regmap = regmap;
> > +
> > +     st->irq = irq;
> >       st->info = device_get_match_data(dev);
> >       if (!st->info)
> >               return -ENODEV;
> > diff --git a/drivers/iio/accel/adxl345_i2c.c b/drivers/iio/accel/adxl345_i2c.c
> > index 4065b8f7c8..604b706c29 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, client->irq, 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 61fd9a6f5f..39e7d71e1d 100644
> > --- a/drivers/iio/accel/adxl345_spi.c
> > +++ b/drivers/iio/accel/adxl345_spi.c
> > @@ -39,9 +39,13 @@ static int adxl345_spi_probe(struct spi_device *spi)
> >               return dev_err_probe(&spi->dev, PTR_ERR(regmap), "Error initializing regmap\n");
> >
> >       if (spi->mode & SPI_3WIRE)
> > -             return adxl345_core_probe(&spi->dev, regmap, adxl345_spi_setup);
> > +             return adxl345_core_probe(&spi->dev, regmap,
> > +                                       spi->irq,
> > +                                       adxl345_spi_setup);
> Very early wrap. I think spi->irq fits on the line above.
>
> >       else
> > -             return adxl345_core_probe(&spi->dev, regmap, NULL);
> > +             return adxl345_core_probe(&spi->dev, regmap,
> > +                                       spi->irq,
> > +                                       NULL);
> >  }
> >
> >  static const struct adxl345_chip_info adxl345_spi_info = {
>

Re: [PATCH v2 05/22] iio: accel: adxl345: measure right-justified

[Jonathan Cameron]

On Tue, 26 Nov 2024 14:51:19 +0100
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Dear IIO Mailing-List, Hi Jonathan!
> 
> Thank you so much for the review. As you probably saw, most (all?) of
> my commits have a huge invisible question mark attached. Most of my
> questions you answered clearly. On particular topics I'd like to get
> back, though. Generally I will try to apply the requested changes to
> best of my understanding.
> 
> On Sun, Nov 24, 2024 at 7:07 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Sun, 17 Nov 2024 18:26:34 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >  
> > > Make measurements right-justified, since it is the default for the
> > > driver and sensor. By not setting the ADXL345_DATA_FORMAT_JUSTIFY bit,
> > > the data becomes right-judstified. This was the original setting, there
> > > is no reason to change it to left-justified, where right-justified
> > > simplifies working on the registers.  
> >
> > Surely this can't be changed independent of other changes as it will
> > change the format of the data we are processing?
> >
> > Each change must stand on it's own so that I can apply up to any
> > point in your patch set and have everything continue to work.  
> 
> This is probably not quite clear. Originally the driver was
> right-justified. One of my last commits
> (f68ebfe1501bf1110eebf5e968c4d9186cba8706) changed the driver to work
> with left-justified measurements. So, I feel changing the orginal
> behavior is wrong, and here I try to re-establish the original driver
> behavior.
> 
> When looking at the datasheet right-justified data seems to be easier
> to handle, but I don't have any personal preference.
Ok.  I'm still a little confused. Can userspace see any result from either
the earlier patch or this one?

Jonathan

> 
> Lothar
> 
> [...]
> > > ---
> > >  drivers/iio/accel/adxl345_core.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > > index 2b62e79248..926e397678 100644
> > > --- a/drivers/iio/accel/adxl345_core.c
> > > +++ b/drivers/iio/accel/adxl345_core.c
> > > @@ -184,8 +184,13 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> > >       struct adxl34x_state *st;
> > >       struct iio_dev *indio_dev;
> > >       u32 regval;
> > > +
> > > +     /* NB: ADXL345_DATA_FORMAT_JUSTIFY or 0:  
> >         /*
> >          * NB: AD...
> >
> > is the multiline comment style all IIO drivers use (and most of the kernel
> > except for networking.
> >  
> > > +      * do right-justified: 0, then adjust resolution according to 10-bit
> > > +      * through 13-bit in channel - this is the default behavior, and can
> > > +      * be modified here by oring ADXL345_DATA_FORMAT_JUSTIFY
> > > +      */
> > >       unsigned int data_format_mask = (ADXL345_DATA_FORMAT_RANGE |
> > > -                                      ADXL345_DATA_FORMAT_JUSTIFY |
> > >                                        ADXL345_DATA_FORMAT_FULL_RES |
> > >                                        ADXL345_DATA_FORMAT_SELF_TEST);
> > >       int ret;  
> >  

Re: [PATCH v2 07/22] iio: accel: adxl345: initialize IRQ number

[Jonathan Cameron]

On Tue, 26 Nov 2024 17:16:07 +0100
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> On Sun, Nov 24, 2024 at 7:14 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Sun, 17 Nov 2024 18:26:36 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >  
> > > Add the possibility to claim an interrupt and init the state structure
> > > with interrupt number and interrupt line to use. The adxl345 can use
> > > two different interrupt lines, mainly to signal FIFO watermark events,
> > > single or double tap, activity, etc. Hence, having the interrupt line
> > > available is crucial to implement such features.  
> >
> > If there are two interrupt lines, you need to be more clever.
> > Imagine only one of them is wired. How do you know which one it is?
> >
> > The query needs to be done by name.  When there are multiple interrupts
> > the ones found in spi and i2c structures could be anything, so don't use
> > those.
> >
> > See fwnode_irq_get_by_name()  
> 
> One of my bigger question marks is this here: The sensor has two
> possible interrupt lines, INT1 or INT2, on which it will notify. But,
> only one is connected. Hence, the irq passed e.g. by SPI will
> automatically refer to the used one. Only this one is going to be
> configured as irq. Or, am I getting this wrong? Alternatively, no
> interrupt line is connected. This was the initial driver setup. In
> this case, it's generally BYPASS-mode for the FIFO and any further
> feature is not possible due to a lack of notification.
> 
> My questions now arise, when it comes to configure the IRQ line in the
> sensor registers. The sensor needs to be configured, that the used
> interrupt line for notifiction is INT1 or INT2. In the later patch of
> this series - "set interrupt line to INT1" - I init it with "INT1".
> But, this actually then can be configured. I can think of yet another
> /sysfs handle (could be dynamically changed at runtime, I'm not sure
> if this makes sense); by an additional devicetree binding (currently
> my preferred idea, I'd default to INT1, but let it configure to INT1
> or INT2 - or better default to bypass mode?), or is there an IIO way
> of configuring it to INT1/2, or a better way? I rather tend to DT. But
> this is still not part of this patch set.

> 
> Do I miss a point here and should still apply the
> fwnode_irq_get_by_name()? This looks a bit like the DT approach, isn't
> it?

Yes - this function does exactly what you need here (it is a very common
situation! Try grepping for INT2 and you should see examples)

To fill in the information you need:
1. Try getting INT1 by name. If succeeds use that and set config on basis INT1 is wired.
2. If not try getting INT2. If that succeeds then use INT2. 
3. If that fails no interrupts.

The binding should 'require' interrupt-names so that you always know
which one is which in the interrupt properties.

If the binding just had one interrupt before, then we add some text to say that
it is only valid if the provided interrupt is INT1.

Jonathan

Re: [PATCH v2 15/22] iio: accel: adxl345: reset the FIFO on error

[Lothar Rubusch]

On Sun, Nov 24, 2024 at 7:54 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sun, 17 Nov 2024 18:26:44 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Add a function to empty the FIFO and reset the INT_SOURCE register.
> > Reading out is used to reset the fifo again. For cleanup also a read
> > on the INT_SOURCE register is needed to allow the adxl345 to issue
> > interrupts again. Without clearing the fields no further interrupts
> > will happen.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > ---
> >  drivers/iio/accel/adxl345_core.c | 75 ++++++++++++++++++++++++++++----
> >  1 file changed, 67 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > index 40e78dbdb0..82bd5c2b78 100644
> > --- a/drivers/iio/accel/adxl345_core.c
> > +++ b/drivers/iio/accel/adxl345_core.c
> > @@ -356,6 +356,61 @@ static int adxl345_get_fifo_entries(struct adxl34x_state *st, int *fifo_entries)
> >       return 0;
> >  }
> >
> > +/**
> > + * adxl345_read_fifo_elements() - Read fifo_entries number of elements.
> > + * @st: The instance of the state object of this sensor.
> > + * @fifo_entries: The number of lines in the FIFO referred to as fifo_entry,
> > + * a fifo_entry has 3 elements for X, Y and Z direction of 2 bytes each.
> > + *
> > + * The FIFO of the sensor is read linewise. The read measurement values are
> > + * queued in the corresponding data structure in *st.
> > + *
> > + * It is recommended that a multiple-byte read of all registers be performed to
> > + * prevent a change in data between reads of sequential registers. That is to
> > + * read out the data registers X0, X1, Y0, Y1, Z0, Z1 at once.
>
> To ensure this, set avail_scan_modes.
> Then if the user requests a subset, the IIO core code will extract what is necessary
> from the read of everythign.

Very intersting. Unfortunately, I could not find "avail_scan_modes".
Did you mean "avail_scan_masks"? I saw some drivers operating with
such. Could you give me some more keywords, that I could have a look
how this is done, pls?

This must be (one of) the reason(s) why with noinc read I only managed
to read 3 directions from the FIFO at a time. Actually, I guess, it
should be possible to dump the entire FIFO.

Lothar

> > + *
> > + * Return: 0 or error value.
> > + */
> > +static int adxl345_read_fifo_elements(struct adxl34x_state *st, int fifo_entries)
> > +{
> > +     size_t count, ndirs = 3;
> > +     int i, ret;
> > +
> > +     count = 2 * ndirs; /* 2 byte per direction */
> sizeof(st->fifo_buf[0] * ndirs);
>
> > +     for (i = 0; i < fifo_entries; i++) {
> > +             ret = regmap_noinc_read(st->regmap, ADXL345_REG_XYZ_BASE,
> > +                             st->fifo_buf + (i * count / 2), count);
> > +             if (ret) {
> > +                     pr_warn("%s(): regmap_noinc_read() failed\n", __func__);
> > +                     return -EFAULT;
> > +             }
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +/**
> > + * adxl345_empty_fifo() - Empty the FIFO.
> > + * @st: The instance to the state object of the sensor.
> > + *
> > + * Reading all elements of the FIFO linewise empties the FIFO. Reading th
> > + * interrupt source register resets the sensor. This is needed also in case of
> > + * overflow or error handling to reenable the sensor to issue interrupts.
> > + */
> > +static void adxl345_empty_fifo(struct adxl34x_state *st)
> > +{
> > +     int regval;
> > +     int fifo_entries;
> > +
> > +     /* In case the HW is not "clean" just read out remaining elements */
> > +     adxl345_get_fifo_entries(st, &fifo_entries);
> > +     if (fifo_entries > 0)
> > +             adxl345_read_fifo_elements(st, fifo_entries);
> > +
> > +     /* Reset the INT_SOURCE register by reading the register */
> > +     regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, &regval);
> > +}
> > +
> >  static const struct iio_buffer_setup_ops adxl345_buffer_ops = {
> >  };
> >
> > @@ -401,30 +456,34 @@ static irqreturn_t adxl345_trigger_handler(int irq, void *p)
> >
> >       ret = adxl345_get_status(st, &int_stat);
> >       if (ret < 0)
> > -             goto done;
> > +             goto err;
> All this churn just makes things less readable.  Better to have the bulk
> of the addition of fifo handling in one patch. It won't be too large
> for review.
> >
> >       /* Ignore already read event by reissued too fast */
> >       if (int_stat == 0x0)
> > -             goto done;
> > +             goto err;
> >
> >       /* evaluation */
> >
> >       if (int_stat & ADXL345_INT_OVERRUN) {
> >               pr_debug("%s(): OVERRUN event detected\n", __func__);
> > -             goto done;
> > +             goto err;
> >       }
> >
> >       if (int_stat & (ADXL345_INT_DATA_READY | ADXL345_INT_WATERMARK)) {
> >               pr_debug("%s(): WATERMARK or DATA_READY event detected\n", __func__);
> >               if (adxl345_get_fifo_entries(st, &fifo_entries) < 0)
> > -                     goto done;
> > -     }
> > -     goto done;
> > -done:
> > +                     goto err;
> >
> > -     if (indio_dev)
> >               iio_trigger_notify_done(indio_dev->trig);
> > +     }
> >
> > +     goto done;
> > +err:
> > +     iio_trigger_notify_done(indio_dev->trig);
> > +     adxl345_empty_fifo(st);
> > +     return IRQ_NONE;
> NONE is probably a bad idea. Something went wrong but that doesn't
> mean it wasn't our interrupt.  In most cases it is better to just
> return that we handled it.  IRQ_NONE might be valid if the status
> said it wasn't ours.
>
> > +
> > +done:
> >       return IRQ_HANDLED;
> return where you have goto done and get rid of this.
>
> >  }
> >
>