| Message ID | 20241211230648.205806-3-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, Dec 11, 2024 at 11:06:43PM +0000, Lothar Rubusch wrote: > Extend the list of constants. Keep them the header file for readability. > The defines allow the implementation of events like watermark, single > tap, double tap, freefall, etc. We don't store constants just to store constants, so this commit does not have reason to be separate. We store constants/defines only to implement the driver. Merge these with the users... unless you want to say there are no users of this at all, but then make it clear: move the patch to the end. Best regards, Krzysztof
Hi Krzysztof, Thank you so much for reviewing. On Thu, Dec 12, 2024 at 9:07 AM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On Wed, Dec 11, 2024 at 11:06:43PM +0000, Lothar Rubusch wrote: > > Extend the list of constants. Keep them the header file for readability. > > The defines allow the implementation of events like watermark, single > > tap, double tap, freefall, etc. > > We don't store constants just to store constants, so this commit does > not have reason to be separate. We store constants/defines only to > implement the driver. Merge these with the users... unless you want to > say there are no users of this at all, but then make it clear: move the > patch to the end. > I see your point. The defines are needed for the current introduction of the FIFO usage, connected with the watermark feature. Some of it is related to upcoming features, such as mentioned in the comment (tap events, freefall, powersafe, selftest, etc). This patch series now on FIFO/watermark are just the first step to get a solid reviewed common base. Further features are upcoming. I did not split up the constants. All the specified registers will be needed to allow for their configuration and setup. I understand it's no organig growth by immediate need, as I understand, but giving IMHO a bit flexibility then in implementing what is the next feature, since all registers are already defined. Pls, let me know, if you prefer me to only introduce immediately needed constants for a current specific feature? Best, L > Best regards, > Krzysztof >
On Thu, 12 Dec 2024 10:37:55 +0100 Lothar Rubusch <l.rubusch@gmail.com> wrote: > Hi Krzysztof, > Thank you so much for reviewing. > > On Thu, Dec 12, 2024 at 9:07 AM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > > > On Wed, Dec 11, 2024 at 11:06:43PM +0000, Lothar Rubusch wrote: > > > Extend the list of constants. Keep them the header file for readability. > > > The defines allow the implementation of events like watermark, single > > > tap, double tap, freefall, etc. > > > > We don't store constants just to store constants, so this commit does > > not have reason to be separate. We store constants/defines only to > > implement the driver. Merge these with the users... unless you want to > > say there are no users of this at all, but then make it clear: move the > > patch to the end. > > > > I see your point. > > The defines are needed for the current introduction of the FIFO usage, > connected with the watermark feature. Some of it is related to > upcoming features, such as mentioned in the comment (tap events, > freefall, powersafe, selftest, etc). > > This patch series now on FIFO/watermark are just the first step to get > a solid reviewed common base. Further features are upcoming. I did not > split up the constants. All the specified registers will be needed to > allow for their configuration and setup. I understand it's no organig > growth by immediate need, as I understand, but giving IMHO a bit > flexibility then in implementing what is the next feature, since all > registers are already defined. > > Pls, let me know, if you prefer me to only introduce immediately > needed constants for a current specific feature? That would be the normal way to do it in a series that is adding those features. There are cases where we do blanket includes of all registers etc in one patch but they tend to be autogenerated from another source (so annoying to split up) rather than introduced alongside features. Also tends to be more common for first posting of a driver rather than adding new features when the author of the driver decided to do a subset (so follow the local style). Jonathan > Best, > L > > > Best regards, > > Krzysztof > >
diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h index 3d5c8719d..9c73474c6 100644 --- a/drivers/iio/accel/adxl345.h +++ b/drivers/iio/accel/adxl345.h @@ -9,37 +9,103 @@ #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 +#define ADXL345_REG_INT_MAP 0x2F +#define ADXL345_REG_INT_SOURCE 0x30 #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_DEVID 0xE5 +#define ADXL345_FIFO_CTL_SAMPLES(x) FIELD_PREP(GENMASK(4, 0), x) +/* 0: INT1, 1: INT2 */ +#define ADXL345_FIFO_CTL_TRIGGER(x) FIELD_PREP(BIT(5), x) +#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_MEASURE BIT(3) #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 #define ADXL345_DATA_FORMAT_16G 3 -#define ADXL345_DEVID 0xE5 +/* + * FIFO stores a maximum of 32 entries, which equates to a maximum of 33 entries + * available at any given time because an additional entry is available at the + * output filter of the device. + * + * (see datasheet FIFO_STATUS description on "Entries Bits") + */ +#define ADXL345_FIFO_SIZE 33 /* * In full-resolution mode, scale factor is maintained at ~4 mg/LSB
Extend the list of constants. Keep them the header file for readability. The defines allow the implementation of events like watermark, single tap, double tap, freefall, etc. Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com> --- drivers/iio/accel/adxl345.h | 92 +++++++++++++++++++++++++++++++------ 1 file changed, 79 insertions(+), 13 deletions(-)