Message-ID: <20250531173330.4d913f65@jic23-huawei>
Date: Sat, 31 May 2025 17:33:30 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Lothar Rubusch <l.rubusch@...il.com>
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

On Wed, 28 May 2025 22:52:55 +0200
Lothar Rubusch <l.rubusch@...il.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;
> > > +}  
> >  

Powered by blists - more mailing lists