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

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

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

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

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

A few comments inline.

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



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

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

> +

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

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

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

Check ret.

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

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


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

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

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

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

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

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

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

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

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

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

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

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

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

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


> +
> +	return IRQ_HANDLED;
> +}

Hi Jonathan,

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

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

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

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

Agree.

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

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

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

Agree.

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

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

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

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

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

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

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

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

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

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

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

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

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

Best,
L

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

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

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

Only add a tag if Andy gives it.

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

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

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

Ok. Then this is fine to keep.

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

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


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

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

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

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

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

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

Soft reset is massive overkill for an overrun event.

Jonathan

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

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

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

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

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

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

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