| Message ID | 20241225181338.69672-8-l.rubusch@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: accel: adxl345: add FIFO operating with IRQ triggered watermark events | expand |
On Wed, 25 Dec 2024 18:13:38 +0000 Lothar Rubusch <l.rubusch@gmail.com> wrote: > Having interrupts events and FIFO available allows to evaluate the > sensor events. Cover the list of interrupt based sensor events. Keep > them in the header file for readability. > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com> One comment inline Thanks, Jonathan > #define ADXL345_REG_BW_RATE 0x2C > #define ADXL345_REG_POWER_CTL 0x2D > #define ADXL345_REG_INT_ENABLE 0x2E > @@ -34,20 +59,40 @@ > #define ADXL345_FIFO_CTL_MODE(x) FIELD_PREP(GENMASK(7, 6), x) > > #define ADXL345_INT_DATA_READY BIT(7) > +#define ADXL345_INT_SINGLE_TAP BIT(6) > +#define ADXL345_INT_DOUBLE_TAP BIT(5) > +#define ADXL345_INT_ACTIVITY BIT(4) > +#define ADXL345_INT_INACTIVITY BIT(3) > +#define ADXL345_INT_FREE_FALL BIT(2) > #define ADXL345_INT_WATERMARK BIT(1) > #define ADXL345_INT_OVERRUN BIT(0) > + > +#define ADXL345_S_TAP_MSK ADXL345_INT_SINGLE_TAP > +#define ADXL345_D_TAP_MSK ADXL345_INT_DOUBLE_TAP Why have these?
On Sat, Dec 28, 2024 at 3:47 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Wed, 25 Dec 2024 18:13:38 +0000 > Lothar Rubusch <l.rubusch@gmail.com> wrote: > > > Having interrupts events and FIFO available allows to evaluate the > > sensor events. Cover the list of interrupt based sensor events. Keep > > them in the header file for readability. > > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com> > > One comment inline > > Thanks, > > Jonathan > > > #define ADXL345_REG_BW_RATE 0x2C > > #define ADXL345_REG_POWER_CTL 0x2D > > #define ADXL345_REG_INT_ENABLE 0x2E > > @@ -34,20 +59,40 @@ > > #define ADXL345_FIFO_CTL_MODE(x) FIELD_PREP(GENMASK(7, 6), x) > > > > #define ADXL345_INT_DATA_READY BIT(7) > > +#define ADXL345_INT_SINGLE_TAP BIT(6) > > +#define ADXL345_INT_DOUBLE_TAP BIT(5) > > +#define ADXL345_INT_ACTIVITY BIT(4) > > +#define ADXL345_INT_INACTIVITY BIT(3) > > +#define ADXL345_INT_FREE_FALL BIT(2) > > #define ADXL345_INT_WATERMARK BIT(1) > > #define ADXL345_INT_OVERRUN BIT(0) > > + > > +#define ADXL345_S_TAP_MSK ADXL345_INT_SINGLE_TAP > > +#define ADXL345_D_TAP_MSK ADXL345_INT_DOUBLE_TAP > > Why have these? > To be honest, I'm unsure to keep this patch within this series now. Initially, ..... long story short, having FIFO and interrupt handling now, it is possible to apply and use those ADXL345_INT_ constants. On the other side, having this more and more separated out of other patches, it becomes clear there is still some implementation pending to really use them. I'd like to focus this series rather on FIFO and interrupt mechanism. Especially when it comes to the ADXL345_S_TAP_MSK defines, which probably make even less sense here, if I look at it now. What would you recommend - Keep it? Drop it? Add just the ADXL345_INT_ fields w/o the _MSKs? > >
On Sat, 28 Dec 2024 16:48:11 +0100 Lothar Rubusch <l.rubusch@gmail.com> wrote: > On Sat, Dec 28, 2024 at 3:47 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Wed, 25 Dec 2024 18:13:38 +0000 > > Lothar Rubusch <l.rubusch@gmail.com> wrote: > > > > > Having interrupts events and FIFO available allows to evaluate the > > > sensor events. Cover the list of interrupt based sensor events. Keep > > > them in the header file for readability. > > > > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com> > > > > One comment inline > > > > Thanks, > > > > Jonathan > > > > > #define ADXL345_REG_BW_RATE 0x2C > > > #define ADXL345_REG_POWER_CTL 0x2D > > > #define ADXL345_REG_INT_ENABLE 0x2E > > > @@ -34,20 +59,40 @@ > > > #define ADXL345_FIFO_CTL_MODE(x) FIELD_PREP(GENMASK(7, 6), x) > > > > > > #define ADXL345_INT_DATA_READY BIT(7) > > > +#define ADXL345_INT_SINGLE_TAP BIT(6) > > > +#define ADXL345_INT_DOUBLE_TAP BIT(5) > > > +#define ADXL345_INT_ACTIVITY BIT(4) > > > +#define ADXL345_INT_INACTIVITY BIT(3) > > > +#define ADXL345_INT_FREE_FALL BIT(2) > > > #define ADXL345_INT_WATERMARK BIT(1) > > > #define ADXL345_INT_OVERRUN BIT(0) > > > + > > > +#define ADXL345_S_TAP_MSK ADXL345_INT_SINGLE_TAP > > > +#define ADXL345_D_TAP_MSK ADXL345_INT_DOUBLE_TAP > > > > Why have these? > > > > To be honest, I'm unsure to keep this patch within this series now. > > Initially, ..... long story short, having FIFO and interrupt handling > now, it is possible to apply and use those ADXL345_INT_ constants. On > the other side, having this more and more separated out of other > patches, it becomes clear there is still some implementation pending > to really use them. I'd like to focus this series rather on FIFO and > interrupt mechanism. > > Especially when it comes to the ADXL345_S_TAP_MSK defines, which > probably make even less sense here, if I look at it now. > > What would you recommend - Keep it? Drop it? Add just the ADXL345_INT_ > fields w/o the _MSKs? I don't mind that much either way so I'll go with what ever you did in v9 (not looked yet :) Jonathan > > > > >
diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h index bf9e86cff..df3977bda 100644 --- a/drivers/iio/accel/adxl345.h +++ b/drivers/iio/accel/adxl345.h @@ -9,10 +9,35 @@ #define _ADXL345_H_ #define ADXL345_REG_DEVID 0x00 +#define ADXL345_REG_THRESH_TAP 0x1D #define ADXL345_REG_OFSX 0x1E #define ADXL345_REG_OFSY 0x1F #define ADXL345_REG_OFSZ 0x20 #define ADXL345_REG_OFS_AXIS(index) (ADXL345_REG_OFSX + (index)) + +/* Tap duration */ +#define ADXL345_REG_DUR 0x21 +/* Tap latency */ +#define ADXL345_REG_LATENT 0x22 +/* Tap window */ +#define ADXL345_REG_WINDOW 0x23 +/* Activity threshold */ +#define ADXL345_REG_THRESH_ACT 0x24 +/* Inactivity threshold */ +#define ADXL345_REG_THRESH_INACT 0x25 +/* Inactivity time */ +#define ADXL345_REG_TIME_INACT 0x26 +/* Axis enable control for activity and inactivity detection */ +#define ADXL345_REG_ACT_INACT_CTRL 0x27 +/* Free-fall threshold */ +#define ADXL345_REG_THRESH_FF 0x28 +/* Free-fall time */ +#define ADXL345_REG_TIME_FF 0x29 +/* Axis control for single tap or double tap */ +#define ADXL345_REG_TAP_AXIS 0x2A +/* Source of single tap or double tap */ +#define ADXL345_REG_ACT_TAP_STATUS 0x2B +/* Data rate and power mode control */ #define ADXL345_REG_BW_RATE 0x2C #define ADXL345_REG_POWER_CTL 0x2D #define ADXL345_REG_INT_ENABLE 0x2E @@ -34,20 +59,40 @@ #define ADXL345_FIFO_CTL_MODE(x) FIELD_PREP(GENMASK(7, 6), x) #define ADXL345_INT_DATA_READY BIT(7) +#define ADXL345_INT_SINGLE_TAP BIT(6) +#define ADXL345_INT_DOUBLE_TAP BIT(5) +#define ADXL345_INT_ACTIVITY BIT(4) +#define ADXL345_INT_INACTIVITY BIT(3) +#define ADXL345_INT_FREE_FALL BIT(2) #define ADXL345_INT_WATERMARK BIT(1) #define ADXL345_INT_OVERRUN BIT(0) + +#define ADXL345_S_TAP_MSK ADXL345_INT_SINGLE_TAP +#define ADXL345_D_TAP_MSK ADXL345_INT_DOUBLE_TAP + +/* + * BW_RATE bits - Bandwidth and output data rate. The default value is + * 0x0A, which translates to a 100 Hz output data rate + */ #define ADXL345_BW_RATE GENMASK(3, 0) +#define ADXL345_BW_LOW_POWER BIT(4) #define ADXL345_BASE_RATE_NANO_HZ 97656250LL #define ADXL345_POWER_CTL_STANDBY 0x00 +#define ADXL345_POWER_CTL_WAKEUP GENMASK(1, 0) +#define ADXL345_POWER_CTL_SLEEP BIT(2) #define ADXL345_POWER_CTL_MEASURE BIT(3) +#define ADXL345_POWER_CTL_AUTO_SLEEP BIT(4) +#define ADXL345_POWER_CTL_LINK BIT(5) -#define ADXL345_DATA_FORMAT_RANGE GENMASK(1, 0) /* Set the g range */ -#define ADXL345_DATA_FORMAT_JUSTIFY BIT(2) /* Left-justified (MSB) mode */ -#define ADXL345_DATA_FORMAT_FULL_RES BIT(3) /* Up to 13-bits resolution */ -#define ADXL345_DATA_FORMAT_SPI_3WIRE BIT(6) /* 3-wire SPI mode */ -#define ADXL345_DATA_FORMAT_SELF_TEST BIT(7) /* Enable a self test */ - +/* Set the g range */ +#define ADXL345_DATA_FORMAT_RANGE GENMASK(1, 0) +/* Data is left justified */ +#define ADXL345_DATA_FORMAT_JUSTIFY BIT(2) +/* Up to 13-bits resolution */ +#define ADXL345_DATA_FORMAT_FULL_RES BIT(3) +#define ADXL345_DATA_FORMAT_SPI_3WIRE BIT(6) +#define ADXL345_DATA_FORMAT_SELF_TEST BIT(7) #define ADXL345_DATA_FORMAT_2G 0 #define ADXL345_DATA_FORMAT_4G 1 #define ADXL345_DATA_FORMAT_8G 2
Having interrupts events and FIFO available allows to evaluate the sensor events. Cover the list of interrupt based sensor events. Keep them in the header file for readability. Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com> --- drivers/iio/accel/adxl345.h | 57 +++++++++++++++++++++++++++++++++---- 1 file changed, 51 insertions(+), 6 deletions(-)