Message-ID: <CAFXKEHb+6xd_F7v7t3sRBoziokr1moNQFdv93ra7X+FKKattnQ@mail.gmail.com>
Date: Wed, 11 Dec 2024 23:32:35 +0100
From: Lothar Rubusch <l.rubusch@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: lars@...afoo.de, Michael.Hennerich@...log.com, robh@...nel.org, 
	krzk+dt@...nel.org, conor+dt@...nel.org, devicetree@...r.kernel.org, 
	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, eraretuya@...il.com
Subject: Re: [PATCH v5 10/10] iio: accel: adxl345: add FIFO with watermark events

Hello Jonathan,
Thank you again. This brought several pieces together for me.
See answeres down below.
Best,
L


On Wed, Dec 11, 2024 at 8:14 PM Jonathan Cameron <jic23@...nel.org> wrote:
>
>
> > > > +}
> > > > +
> > > > +/**
> > > > + * adxl345_fifo_transfer() - Read samples number of elements.
> > > > + * @st: The instance of the state object of this sensor.
> > > > + * @samples: The number of lines in the FIFO referred to as fifo_entry,
> > > > + * a fifo_entry has 3 elements for X, Y and Z direction of 2 bytes each.
> > > > + *
> > > > + * 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.
> > >
> > > Doesn't match the code which is reading just one register lots of times.
> >
> > This is one of my painpoints, regmap_noinc_read() worked here, for a
> > linewise reading of FIFO elements. Say, I could read X0, X1, Y0,... Z1
> > in one command. Also, I tried here regmap_bulk_read(). At all, I find
> > this solution is working, but I'm not sure if there is not a total
> > differnt way to do the read out.
>
> A bulk read is defined as indexing through registers. Eg. ADDR, ADDR + 1, ADDR + 2
> etc.  regmap_noinc_read() just keeps reading the same register, so is typically
> used for fifos.
>
> I opened the datasheet. It seems to say you need to read 3 registers repeatedly
> rather than one 3 times as often.  There isn't a good way to do that sort of
> sequenced read in one go.  So you will need a loop like you have, but it
> should need a bulk read.  Curious it doesn't seem to...
>
> Ah. Device auto increments for both SPI and I2C.  So in that case
> the noinc_read and normal bulk read will actually issue the same thing and
> as these are volatile registers it doesn't matter (it would if you were
> caching the result as the data would end up cached in different places).
>
> It will do the wrong thing though if you have an i2c controller that
> is less capable and can't do large reads.  So you should definitely use
> bulk_read not noinc.
>
> Have you set available_scan_masks?  If not you want to do so as
> per comment I made in the cover letter.
>

For v6 I'll setup available_scan_masks, as I understand its usage. I'm
not sure if I understood what it actually does. I can see it needs the
channel indexes available, but I'll need to read a bit more code.
Hopefully for now it'll be sufficient.

>
> > > > + * @irq: The irq being handled.
> > > > + * @p: The struct iio_device pointer for the device.
> > > > + *
> > > > + * Return: The interrupt was handled.
> > > > + */
> > > > +static irqreturn_t adxl345_event_handler(int irq, void *p)
> > > > +{
> > > > +     struct iio_dev *indio_dev = p;
> > > > +     struct adxl345_state *st = iio_priv(indio_dev);
> > > > +     u8 int_stat;
> > > > +     int samples;
> > > > +
> > > > +     int_stat = adxl345_get_status(st);
> > > > +     if (int_stat < 0)
> > > > +             return IRQ_NONE;
> > > > +
> > > > +     if (int_stat == 0x0)
> > > Doesn't this correspond to 'not our interrupt'?
> > > If that's the case return IRQ_NONE is the right way to go and not reset the
> > > interrupt.  You have registered it as maybe shared, and if it is, then this
> > > is a common thing to happen as interrupt from another device.
> > >
> >
> > Here I see actually
> > +     int_stat = adxl345_get_status(st);
> > +     if (int_stat < 0)
> > +             return IRQ_NONE; // a bus error, reading register not possible
> > ...and then...
> > +     if (int_stat == 0x0)
> > +             // interrupt sources were 0, so IRQ not from our sensor
> >
> > I'm unsure if the first IRQ_NONE here is actually correct. I mean, if
> > the bus is not working,
> > actually any IRQ usage should be considered broken. Is there a way to
> > break out of measuring?
> >

Here actually, I merged them, when returning IRQ_NONE in both
statements. I mean, if the bus is broken, the sensor driver then is
anyway not operational anymore. It cannot process interrupts anymore
be it from someone else or own ones.

When the interrupt source register was 0, then the interrupt was not
ours for sure. As I understand, if such happens, there will be no more
IRQs anyway as long as interrupt source reg and fifo are not cleaned
up.

> It is a much debated thing on what you should return if you have no
> idea if it is our interrupt or not.   There isn't really a right
> answer.  If you get a lot of IRQ_NONE and no one else claims it eventually
> the interrupt will be disabled (to break the interrupt storm freezing the
> machine).
>
>
> > > > +             goto err;
> > > > +
> > > > +     if (int_stat & ADXL345_INT_OVERRUN)
> > > > +             goto err;
> > > > +
> > > > +     if (int_stat & (ADXL345_INT_DATA_READY | ADXL345_INT_WATERMARK)) {
> > >
> > > I think you only ever enable the INT_WATERMARK?  If so does it
> > > make sense to check for DATA_READY as well?
> > >
> >
> > Watermark comes usually with data ready or overrun. I guess for the
> > FIFO watermark, just evaluating watermark is probably sufficient. For
> > other events, it then might be notification w/ a data ready set.
> > Probably better to introduce data ready when it's actually really
> > needed?
>
> Yes.  That dataready is normally used when you are doing capture without
> the fifo and want to read each sample - kind of same as a watermark depth
> of 1, but less hardware turned on.  As such, may be no need to ever support it.
>
>    return dev_err_probe(dev, ret, "Failed to add action or reset\n");
> > > >
> > > > +     if (st->irq > 0) {
> > > > +             dev_dbg(dev, "initialize for FIFO_STREAM mode\n");
> > > > +
> > > > +             ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, &adxl345_buffer_ops);
> > > > +             if (ret)
> > > > +                     return ret;
> > > > +
> > > > +             ret = devm_request_threaded_irq(dev, st->irq, NULL, &adxl345_event_handler,
> > > > +                             IRQF_SHARED | IRQF_ONESHOT,
> > > > +                             indio_dev->name, indio_dev);
> > > > +             if (ret)
> > > > +                     return dev_err_probe(dev, ret, "Failed to setup triggered buffer\n");
> > > > +
> > > > +     } else {
> > > > +             dev_dbg(dev, "initialize for FIFO_BYPASS mode (fallback)\n");
> > > > +
> > > Given you haven't removed this code from elsewhere, was the driver relying
> > > on the defaults after reset before this patch?
> > >
> > > I don't mind having this branch as a form of documentation even if that's
> > > true but maybe add a note to the patch description.
> > >
> >
> > I'm not sure if I get you correctly. The driver before only
> > implemented "BYPASS" mode. This was the case w/o a defined
> > interrupt-name. My intention now is to keep this behavior as fallback.
> > If no IRQ is around, i.e. interrupt + interrupt-name, the sensor
> > driver will operate the sensor in BYPASS mode.
> >
> > I was interpreting this also as the "default behavior", you mentioned
> > in the dt-binding patch? Is this correct?
> >
> > What do you mean when the driver is relying on the defaults after reset?
>
> The driver worked without irq before now.  Which is same as this path.
> So how was the register written here being configured correctly before
> this patch?  I'm guessing it wasn't and that the value written here
> is the default on power up.
>
> Either that or I'm miss understanding what this branch of the if / else is
> about.

Well, I'm pretty convinced the sensor before was operated in
FIFO_BYPASS mode. TBH I'm not sure now if this is default setting, or
explicitely configured. But I remember that also from somewhere in the
datasheet. That's why I used this as fallback.

> Jonathan
>
> >
> > > > +             fifo_ctl = ADXL345_FIFO_CTL_MODE(ADXL345_FIFO_BYPASS);
> > > > +
> > > > +             ret = regmap_write(st->regmap, ADXL345_REG_FIFO_CTL, fifo_ctl);
> > > > +             if (ret < 0)
> > > > +                     return ret;
> > > > +     }
> > > >       return devm_iio_device_register(dev, indio_dev);
> > > >  }
> > > >  EXPORT_SYMBOL_NS_GPL(adxl345_core_probe, IIO_ADXL345);
> > >
> >
>

Powered by blists - more mailing lists