Message-ID: <CAFXKEHaM1xEk-v7rXdKoxdXKV-k=_Leu+hMBukDyKoWr3irVRQ@mail.gmail.com>
Date: Wed, 28 May 2025 22:52:55 +0200
From: Lothar Rubusch <l.rubusch@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: dlechner@...libre.com, nuno.sa@...log.com, andy@...nel.org, corbet@....net, 
	lucas.p.stankus@...il.com, lars@...afoo.de, Michael.Hennerich@...log.com, 
	linux-iio@...r.kernel.org, linux-doc@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 07/12] iio: accel: adxl313: add basic interrupt handling

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@...nel.org> wrote:
>
> On Fri, 23 May 2025 22:35:18 +0000
> Lothar Rubusch <l.rubusch@...il.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@...il.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;
> > +}
>

Powered by blists - more mailing lists