Re: [PATCH v8 6/7] iio: accel: adxl345: add FIFO with watermark events

Jonathan Cameron
2025-01-04T12:49:55Z

On Sat, 28 Dec 2024 17:11:58 +0100
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> On Sat, Dec 28, 2024 at 3:45 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Wed, 25 Dec 2024 18:13:37 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >
> > > Add a basic setup for FIFO with configurable watermark. Add a handler
> > > for watermark interrupt events and extend the channel for the
> > > scan_index needed for the iio channel. The sensor is configurable to use
> > > a FIFO_BYPASSED mode or a FIFO_STREAM mode. For the FIFO_STREAM mode now
> > > a watermark can be configured, or disabled by setting 0. Further features
> > > require a working FIFO setup.
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > ---
> > > drivers/iio/accel/adxl345.h | 27 ++-
> > > drivers/iio/accel/adxl345_core.c | 305 ++++++++++++++++++++++++++++++-
> > > 2 files changed, 321 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> > > index 6f39f16d3..bf9e86cff 100644
> > > --- a/drivers/iio/accel/adxl345.h
> > > +++ b/drivers/iio/accel/adxl345.h
> > > @@ -15,18 +15,32 @@
> > > #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_INT_ENABLE 0x2E
> > > +#define ADXL345_REG_INT_MAP 0x2F
> > > +#define ADXL345_REG_INT_SOURCE 0x30
> > > +#define ADXL345_REG_INT_SOURCE_MSK 0xFF
> > > #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_REG_XYZ_BASE 0x32
> > > +#define ADXL345_REG_DATA_AXIS(index) \
> > > + (ADXL345_REG_XYZ_BASE + (index) * sizeof(__le16))
> > >
> > > +#define ADXL345_REG_FIFO_CTL 0x38
> > > +#define ADXL345_REG_FIFO_STATUS 0x39
> > > +#define ADXL345_REG_FIFO_STATUS_MSK 0x3F
> > > +
> > > +#define ADXL345_FIFO_CTL_SAMPLES(x) FIELD_PREP(GENMASK(4, 0), x)
> > These need linux/bitfield.h to be included.
> >
>
> Sure, I can do this.
>
> > However, that got me looking closer at this and it should be done
> > differently.
> >
> > Just define the masks in here and put the FIELD_PREP() inline in the
> > c file. Same for the various other FIELD_PREP macros in here.
> >
> > It may seem convenient to wrap all this up here, but in general
> > I'd rather see that these are simple FIELD_PREP() calls where they
> > are used inline.
> >
>
> Ok, I understand, the masks stay in the adxl345.h and the FIELD_RPEP()
> macros better go to where they are used, i.e. the adxl345_core.c.
>
> Thank you for reviewing this. I was a bit unsure about using the
> FIELD_PREP() / GENMASK() combinations correctly. The code seems to
> work for me, though. For future usage of those macros: Generally my
> usage of those macros is correct?

Yes, the usage is fine otherwise.

>
> Do I understand correctly, on the long run it would generally be
> cleaner, also to migrate the defines to the adxl345_core.c? IMHO,
> probably most of the constants in the adxl345.h can be private to
> adxl345_core.c, and probably better should.

Exactly.

Thanks,

Jonathan

>
> Best regards,
> L