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

[Jonathan Cameron <jic23@kernel.org>]

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