Add the single tap feature with a threshold in 62.5mg/LSB points and a
scaled duration in us. Keep singletap threshold in regmap cache but
the scaled value of duration in us as member variable.
Both use IIO channels for individual enable of the x/y/z axis. Initializes
threshold and duration with reasonable content. When an interrupt is
caught it will be pushed to the according IIO channel.
Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
drivers/iio/accel/adxl345_core.c | 364 ++++++++++++++++++++++++++++++-
1 file changed, 362 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 0cee81bc1877..d05593c0d513 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -8,6 +8,7 @@
*/
#include <linux/bitfield.h>
+#include <linux/bitops.h>
#include <linux/interrupt.h>
#include <linux/module.h>
#include <linux/property.h>
@@ -17,6 +18,7 @@
#include <linux/iio/iio.h>
#include <linux/iio/sysfs.h>
#include <linux/iio/buffer.h>
+#include <linux/iio/events.h>
#include <linux/iio/kfifo_buf.h>
#include "adxl345.h"
@@ -31,6 +33,33 @@
#define ADXL345_INT1 0
#define ADXL345_INT2 1
+#define ADXL345_REG_TAP_AXIS_MSK GENMASK(2, 0)
+
+enum adxl345_axis {
+ ADXL345_Z_EN = BIT(0),
+ ADXL345_Y_EN = BIT(1),
+ ADXL345_X_EN = BIT(2),
+ /* Suppress double tap detection if value > tap threshold */
+ ADXL345_TAP_SUPPRESS = BIT(3),
+};
+
+/* single/double tap */
+enum adxl345_tap_type {
+ ADXL345_SINGLE_TAP,
+};
+
+static const unsigned int adxl345_tap_int_reg[] = {
+ [ADXL345_SINGLE_TAP] = ADXL345_INT_SINGLE_TAP,
+};
+
+enum adxl345_tap_time_type {
+ ADXL345_TAP_TIME_DUR,
+};
+
+static const unsigned int adxl345_tap_time_reg[] = {
+ [ADXL345_TAP_TIME_DUR] = ADXL345_REG_DUR,
+};
+
struct adxl345_state {
const struct adxl345_chip_info *info;
struct regmap *regmap;
@@ -38,9 +67,23 @@ struct adxl345_state {
int irq;
u8 watermark;
u8 fifo_mode;
+
+ u32 tap_axis_ctrl;
+ u32 tap_duration_us;
+
__le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE + 1] __aligned(IIO_DMA_MINALIGN);
};
+static struct iio_event_spec adxl345_events[] = {
+ {
+ .type = IIO_EV_TYPE_GESTURE,
+ .dir = IIO_EV_DIR_SINGLETAP,
+ .mask_separate = BIT(IIO_EV_INFO_ENABLE),
+ .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
+ BIT(IIO_EV_INFO_TIMEOUT),
+ },
+};
+
#define ADXL345_CHANNEL(index, reg, axis) { \
.type = IIO_ACCEL, \
.modified = 1, \
@@ -57,6 +100,8 @@ struct adxl345_state {
.storagebits = 16, \
.endianness = IIO_LE, \
}, \
+ .event_spec = adxl345_events, \
+ .num_event_specs = ARRAY_SIZE(adxl345_events), \
}
enum adxl345_chans {
@@ -83,6 +128,7 @@ bool adxl345_is_volatile_reg(struct device *dev, unsigned int reg)
case ADXL345_REG_DATA_AXIS(3):
case ADXL345_REG_DATA_AXIS(4):
case ADXL345_REG_DATA_AXIS(5):
+ case ADXL345_REG_ACT_TAP_STATUS:
case ADXL345_REG_FIFO_STATUS:
case ADXL345_REG_INT_SOURCE:
return true;
@@ -112,6 +158,117 @@ static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
return regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
}
+/* tap */
+
+static int adxl345_write_tap_axis(struct adxl345_state *st,
+ enum adxl345_axis axis, bool en)
+{
+ st->tap_axis_ctrl = FIELD_GET(ADXL345_REG_TAP_AXIS_MSK,
+ en ? st->tap_axis_ctrl | axis
+ : st->tap_axis_ctrl & ~axis);
+
+ return regmap_update_bits(st->regmap, ADXL345_REG_TAP_AXIS,
+ ADXL345_REG_TAP_AXIS_MSK,
+ FIELD_PREP(ADXL345_REG_TAP_AXIS_MSK,
+ st->tap_axis_ctrl));
+}
+
+static int _adxl345_set_tap_int(struct adxl345_state *st,
+ enum adxl345_tap_type type, bool state)
+{
+ unsigned int int_map = 0x00;
+ unsigned int tap_threshold;
+ bool axis_valid;
+ bool singletap_args_valid = false;
+ bool en = false;
+ int ret;
+
+ axis_valid = FIELD_GET(ADXL345_REG_TAP_AXIS_MSK, st->tap_axis_ctrl) > 0;
+
+ ret = regmap_read(st->regmap, ADXL345_REG_THRESH_TAP, &tap_threshold);
+ if (ret)
+ return ret;
+
+ /*
+ * Note: A value of 0 for threshold and/or dur may result in undesirable
+ * behavior if single tap/double tap interrupts are enabled.
+ */
+ singletap_args_valid = tap_threshold > 0 && st->tap_duration_us > 0;
+
+ if (type == ADXL345_SINGLE_TAP)
+ en = axis_valid && singletap_args_valid;
+
+ if (state && en)
+ int_map |= adxl345_tap_int_reg[type];
+
+ return regmap_update_bits(st->regmap, ADXL345_REG_INT_ENABLE,
+ adxl345_tap_int_reg[type], int_map);
+}
+
+static int adxl345_is_tap_en(struct adxl345_state *st,
+ enum adxl345_tap_type type, bool *en)
+{
+ int ret;
+ unsigned int regval;
+
+ ret = regmap_read(st->regmap, ADXL345_REG_INT_ENABLE, ®val);
+ if (ret)
+ return ret;
+
+ *en = (adxl345_tap_int_reg[type] & regval) > 0;
+
+ return 0;
+}
+
+static int adxl345_set_singletap_en(struct adxl345_state *st,
+ enum adxl345_axis axis, bool en)
+{
+ int ret;
+
+ ret = adxl345_write_tap_axis(st, axis, en);
+ if (ret)
+ return ret;
+
+ return _adxl345_set_tap_int(st, ADXL345_SINGLE_TAP, en);
+}
+
+static int _adxl345_set_tap_time(struct adxl345_state *st,
+ enum adxl345_tap_time_type type, u32 val_us)
+{
+ unsigned int regval;
+
+ switch (type) {
+ case ADXL345_TAP_TIME_DUR:
+ st->tap_duration_us = val_us;
+ break;
+ }
+
+ /*
+ * The scale factor is 1250us / LSB for tap_window_us and tap_latent_us.
+ * For tap_duration_us the scale factor is 625us / LSB.
+ */
+ if (type == ADXL345_TAP_TIME_DUR)
+ regval = DIV_ROUND_CLOSEST(val_us, 625);
+ else
+ regval = DIV_ROUND_CLOSEST(val_us, 1250);
+
+ return regmap_write(st->regmap, adxl345_tap_time_reg[type], regval);
+}
+
+static int adxl345_set_tap_duration(struct adxl345_state *st, u32 val_int,
+ u32 val_fract_us)
+{
+ /*
+ * Max value is 255 * 625 us = 0.159375 seconds
+ *
+ * Note: the scaling is similar to the scaling in the ADXL380
+ */
+ if (val_int || val_fract_us > 159375)
+ return -EINVAL;
+
+ return _adxl345_set_tap_time(st, ADXL345_TAP_TIME_DUR, val_fract_us);
+}
+
static int adxl345_read_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int *val, int *val2, long mask)
@@ -197,6 +354,160 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
return -EINVAL;
}
+static int adxl345_read_event_config(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir)
+{
+ struct adxl345_state *st = iio_priv(indio_dev);
+ bool int_en;
+ bool axis_en;
+ int ret = -EFAULT;
+
+ switch (type) {
+ case IIO_EV_TYPE_GESTURE:
+ switch (chan->channel2) {
+ case IIO_MOD_X:
+ axis_en = FIELD_GET(ADXL345_X_EN, st->tap_axis_ctrl);
+ break;
+ case IIO_MOD_Y:
+ axis_en = FIELD_GET(ADXL345_Y_EN, st->tap_axis_ctrl);
+ break;
+ case IIO_MOD_Z:
+ axis_en = FIELD_GET(ADXL345_Z_EN, st->tap_axis_ctrl);
+ break;
+ default:
+ axis_en = ADXL345_TAP_SUPPRESS;
+ break;
+ }
+
+ switch (dir) {
+ case IIO_EV_DIR_SINGLETAP:
+ ret = adxl345_is_tap_en(st, ADXL345_SINGLE_TAP, &int_en);
+ if (ret)
+ return ret;
+ return int_en && axis_en;
+ default:
+ return -EINVAL;
+ }
+ default:
+ return -EINVAL;
+ }
+}
+
+static int adxl345_write_event_config(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ int state)
+{
+ struct adxl345_state *st = iio_priv(indio_dev);
+ enum adxl345_axis axis;
+
+ switch (type) {
+ case IIO_EV_TYPE_GESTURE:
+ switch (chan->channel2) {
+ case IIO_MOD_X:
+ axis = ADXL345_X_EN;
+ break;
+ case IIO_MOD_Y:
+ axis = ADXL345_Y_EN;
+ break;
+ case IIO_MOD_Z:
+ axis = ADXL345_Z_EN;
+ break;
+ default:
+ axis = ADXL345_TAP_SUPPRESS;
+ break;
+ }
+
+ switch (dir) {
+ case IIO_EV_DIR_SINGLETAP:
+ return adxl345_set_singletap_en(st, axis, state);
+ default:
+ return -EINVAL;
+ }
+ default:
+ return -EINVAL;
+ }
+}
+
+static int adxl345_read_event_value(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ enum iio_event_info info,
+ int *val, int *val2)
+{
+ struct adxl345_state *st = iio_priv(indio_dev);
+ unsigned int tap_threshold;
+ int ret;
+
+ switch (type) {
+ case IIO_EV_TYPE_GESTURE:
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ /*
+ * The scale factor is 62.5mg/LSB (i.e. 0xFF = 16g) but
+ * not applied here.
+ */
+ ret = regmap_read(st->regmap, ADXL345_REG_THRESH_TAP, &tap_threshold);
+ if (ret)
+ return ret;
+ *val = sign_extend32(tap_threshold, 7);
+ return IIO_VAL_INT;
+ case IIO_EV_INFO_TIMEOUT:
+ *val = st->tap_duration_us;
+ *val2 = 1000000;
+ return IIO_VAL_FRACTIONAL;
+ default:
+ return -EINVAL;
+ }
+ default:
+ return -EINVAL;
+ }
+}
+
+static int adxl345_write_event_value(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ enum iio_event_info info,
+ int val, int val2)
+{
+ struct adxl345_state *st = iio_priv(indio_dev);
+ int ret;
+
+ ret = adxl345_set_measure_en(st, false);
+ if (ret)
+ return ret;
+
+ switch (type) {
+ case IIO_EV_TYPE_GESTURE:
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ ret = regmap_write(st->regmap, ADXL345_REG_THRESH_TAP,
+ min(val, 0xFF));
+ break;
+ case IIO_EV_INFO_TIMEOUT:
+ ret = adxl345_set_tap_duration(st, val, val2);
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
+ if (ret)
+ return ret; /* measurement stays off */
+
+ return adxl345_set_measure_en(st, true);
+}
+
static int adxl345_reg_access(struct iio_dev *indio_dev, unsigned int reg,
unsigned int writeval, unsigned int *readval)
{
@@ -419,6 +730,26 @@ static int adxl345_fifo_push(struct iio_dev *indio_dev,
return 0;
}
+static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
+ enum iio_modifier act_tap_dir)
+{
+ s64 ts = iio_get_time_ns(indio_dev);
+ int ret;
+
+ if (FIELD_GET(ADXL345_INT_SINGLE_TAP, int_stat)) {
+ ret = iio_push_event(indio_dev,
+ IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
+ act_tap_dir,
+ IIO_EV_TYPE_GESTURE,
+ IIO_EV_DIR_SINGLETAP),
+ ts);
+ if (ret)
+ return ret;
+ }
+
+ return -ENOENT;
+}
+
/**
* adxl345_irq_handler() - Handle irqs of the ADXL345.
* @irq: The irq being handled.
@@ -430,12 +761,28 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
{
struct iio_dev *indio_dev = p;
struct adxl345_state *st = iio_priv(indio_dev);
- int int_stat;
- int samples;
+ unsigned int regval;
+ enum iio_modifier act_tap_dir = IIO_NO_MOD;
+ int int_stat, samples, ret;
+
+ if (FIELD_GET(ADXL345_REG_TAP_AXIS_MSK, st->tap_axis_ctrl) > 0) {
+ ret = regmap_read(st->regmap, ADXL345_REG_ACT_TAP_STATUS, ®val);
+ if (ret)
+ return ret;
+ if (FIELD_GET(ADXL345_Z_EN, regval) > 0)
+ act_tap_dir = IIO_MOD_Z;
+ else if (FIELD_GET(ADXL345_Y_EN, regval) > 0)
+ act_tap_dir = IIO_MOD_Y;
+ else if (FIELD_GET(ADXL345_X_EN, regval) > 0)
+ act_tap_dir = IIO_MOD_X;
+ }
if (regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, &int_stat))
return IRQ_NONE;
+ if (adxl345_push_event(indio_dev, int_stat, act_tap_dir) == 0)
+ return IRQ_HANDLED;
+
if (FIELD_GET(ADXL345_INT_WATERMARK, int_stat)) {
samples = adxl345_get_samples(st);
if (samples < 0)
@@ -461,6 +808,10 @@ static const struct iio_info adxl345_info = {
.read_raw = adxl345_read_raw,
.write_raw = adxl345_write_raw,
.write_raw_get_fmt = adxl345_write_raw_get_fmt,
+ .read_event_config = adxl345_read_event_config,
+ .write_event_config = adxl345_write_event_config,
+ .read_event_value = adxl345_read_event_value,
+ .write_event_value = adxl345_write_event_value,
.debugfs_reg_access = &adxl345_reg_access,
.hwfifo_set_watermark = adxl345_set_watermark,
};
@@ -494,6 +845,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
ADXL345_DATA_FORMAT_JUSTIFY |
ADXL345_DATA_FORMAT_FULL_RES |
ADXL345_DATA_FORMAT_SELF_TEST);
+ unsigned int tap_threshold;
int ret;
indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
@@ -507,6 +859,10 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
return -ENODEV;
st->fifo_delay = fifo_delay_default;
+ /* Init with reasonable values */
+ tap_threshold = 48; /* 48 [0x30] -> ~3g */
+ st->tap_duration_us = 16; /* 16 [0x10] -> .010 */
+
indio_dev->name = st->info->name;
indio_dev->info = &adxl345_info;
indio_dev->modes = INDIO_DIRECT_MODE;
@@ -579,6 +935,10 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
if (ret)
return ret;
+ ret = regmap_write(st->regmap, ADXL345_REG_THRESH_TAP, tap_threshold);
+ if (ret)
+ return ret;
+
/* FIFO_STREAM mode is going to be activated later */
ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, &adxl345_buffer_ops);
if (ret)
--
2.39.5On Thu, 20 Feb 2025 10:42:25 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:
> Add the single tap feature with a threshold in 62.5mg/LSB points and a
> scaled duration in us. Keep singletap threshold in regmap cache but
> the scaled value of duration in us as member variable.
>
> Both use IIO channels for individual enable of the x/y/z axis. Initializes
> threshold and duration with reasonable content. When an interrupt is
> caught it will be pushed to the according IIO channel.
>
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
> drivers/iio/accel/adxl345_core.c | 364 ++++++++++++++++++++++++++++++-
> 1 file changed, 362 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 0cee81bc1877..d05593c0d513 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -8,6 +8,7 @@
> */
>
> #include <linux/bitfield.h>
> +#include <linux/bitops.h>
> #include <linux/interrupt.h>
> #include <linux/module.h>
> #include <linux/property.h>
> @@ -17,6 +18,7 @@
> #include <linux/iio/iio.h>
> #include <linux/iio/sysfs.h>
> #include <linux/iio/buffer.h>
> +#include <linux/iio/events.h>
> #include <linux/iio/kfifo_buf.h>
>
> #include "adxl345.h"
> @@ -31,6 +33,33 @@
> #define ADXL345_INT1 0
> #define ADXL345_INT2 1
>
> +#define ADXL345_REG_TAP_AXIS_MSK GENMASK(2, 0)
This is a bit confusing. Here we have a mask for axis that
has 3 bits.
> +
> +enum adxl345_axis {
> + ADXL345_Z_EN = BIT(0),
> + ADXL345_Y_EN = BIT(1),
> + ADXL345_X_EN = BIT(2),
> + /* Suppress double tap detection if value > tap threshold */
> + ADXL345_TAP_SUPPRESS = BIT(3),
and here an enum that is closely related with 4.
> +};
...
> enum adxl345_chans {
> @@ -83,6 +128,7 @@ bool adxl345_is_volatile_reg(struct device *dev, unsigned int reg)
> case ADXL345_REG_DATA_AXIS(3):
> case ADXL345_REG_DATA_AXIS(4):
> case ADXL345_REG_DATA_AXIS(5):
> + case ADXL345_REG_ACT_TAP_STATUS:
ah. I thought this had a full list from earlier patches.
Move this and any later additions back to patch 4.
> case ADXL345_REG_FIFO_STATUS:
> case ADXL345_REG_INT_SOURCE:
> return true;
> @@ -112,6 +158,117 @@ static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
> return regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
> }
>
> +/* tap */
> +
> +static int adxl345_write_tap_axis(struct adxl345_state *st,
> + enum adxl345_axis axis, bool en)
> +{
> + st->tap_axis_ctrl = FIELD_GET(ADXL345_REG_TAP_AXIS_MSK,
> + en ? st->tap_axis_ctrl | axis
> + : st->tap_axis_ctrl & ~axis);
> +
> + return regmap_update_bits(st->regmap, ADXL345_REG_TAP_AXIS,
> + ADXL345_REG_TAP_AXIS_MSK,
> + FIELD_PREP(ADXL345_REG_TAP_AXIS_MSK,
> + st->tap_axis_ctrl));
Given that mask is bottom few bits anyway and you can just define the
tap axis as separate fields.
See below, but I would push the IIO_MOD values down into here. Put the switch
per axis in at this level and then use a simple if statement to
call regmap_clear_bits() / regmap_set_bits() based on enable.
> +}
...
> +static int adxl345_is_tap_en(struct adxl345_state *st,
> + enum adxl345_tap_type type, bool *en)
> +{
> + int ret;
> + unsigned int regval;
> +
> + ret = regmap_read(st->regmap, ADXL345_REG_INT_ENABLE, ®val);
> + if (ret)
> + return ret;
> +
> + *en = (adxl345_tap_int_reg[type] & regval) > 0;
Could use 0/1 return rather than passing a paramter for the output.
I don't mind much either way.
> +
> + return 0;
> +}
> +
> +static int adxl345_set_singletap_en(struct adxl345_state *st,
I'd push the IIO modifier in here instead of axis.
> + enum adxl345_axis axis, bool en)
> +{
> + int ret;
> +
> + ret = adxl345_write_tap_axis(st, axis, en);
And push it into here as well.
> + if (ret)
> + return ret;
> +
> + return _adxl345_set_tap_int(st, ADXL345_SINGLE_TAP, en);
> +}
> +
...
> @@ -197,6 +354,160 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
> return -EINVAL;
> }
>
> +static int adxl345_read_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir)
> +{
> + struct adxl345_state *st = iio_priv(indio_dev);
> + bool int_en;
> + bool axis_en;
> + int ret = -EFAULT;
> +
> + switch (type) {
> + case IIO_EV_TYPE_GESTURE:
> + switch (chan->channel2) {
> + case IIO_MOD_X:
> + axis_en = FIELD_GET(ADXL345_X_EN, st->tap_axis_ctrl);
> + break;
> + case IIO_MOD_Y:
> + axis_en = FIELD_GET(ADXL345_Y_EN, st->tap_axis_ctrl);
> + break;
> + case IIO_MOD_Z:
> + axis_en = FIELD_GET(ADXL345_Z_EN, st->tap_axis_ctrl);
> + break;
> + default:
> + axis_en = ADXL345_TAP_SUPPRESS;
> + break;
> + }
I'd check axis_en here.
if (!axis_en)
return false;
> +
> + switch (dir) {
> + case IIO_EV_DIR_SINGLETAP:
> + ret = adxl345_is_tap_en(st, ADXL345_SINGLE_TAP, &int_en);
> + if (ret)
> + return ret;
> + return int_en && axis_en;
Then this just becomes return int_en;
> + default:
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int adxl345_write_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + int state)
> +{
> + struct adxl345_state *st = iio_priv(indio_dev);
> + enum adxl345_axis axis;
> +
> + switch (type) {
> + case IIO_EV_TYPE_GESTURE:
> + switch (chan->channel2) {
> + case IIO_MOD_X:
> + axis = ADXL345_X_EN;
> + break;
> + case IIO_MOD_Y:
> + axis = ADXL345_Y_EN;
> + break;
> + case IIO_MOD_Z:
> + axis = ADXL345_Z_EN;
> + break;
> + default:
> + axis = ADXL345_TAP_SUPPRESS;
How are we getting another axis here?
> + break;
> + }
> +
> + switch (dir) {
> + case IIO_EV_DIR_SINGLETAP:
> + return adxl345_set_singletap_en(st, axis, state);
As above, pass chan->channel2 into here so we can simplify the eventual
setting of the the register values.
> + default:
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int adxl345_read_event_value(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info,
> + int *val, int *val2)
> +{
> + struct adxl345_state *st = iio_priv(indio_dev);
> + unsigned int tap_threshold;
> + int ret;
> +
> + switch (type) {
> + case IIO_EV_TYPE_GESTURE:
> + switch (info) {
> + case IIO_EV_INFO_VALUE:
> + /*
> + * The scale factor is 62.5mg/LSB (i.e. 0xFF = 16g) but
> + * not applied here.
Maybe say why.
> + */
> + ret = regmap_read(st->regmap, ADXL345_REG_THRESH_TAP, &tap_threshold);
Bit of a long line. Aim for sub 80 chars unless readability is greatly
hurt.
> + if (ret)
> + return ret;
> + *val = sign_extend32(tap_threshold, 7);
> + return IIO_VAL_INT;
> + case IIO_EV_INFO_TIMEOUT:
> + *val = st->tap_duration_us;
> + *val2 = 1000000;
> + return IIO_VAL_FRACTIONAL;
> + default:
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
> +}
...
Hi Jonathan,
I prepared, reorganized and tested a v4 patch set. Given that I see
how busy you must be these days with current increased mail traffic
just in IIO ML when I compare it to some years ago,
I don't want to bother you too much.
Some particular doubts I will inline down below. If possible with your
work flow and to avoid giving you extra work, I'd like to ask you to
read the questions here, but to give your answers right to the
v4 patch set (so, i.e. after having seen the current state of source).
It also should make my points
a bit clearer. Anyway, this is just my idea, since I'm always happy
about any feedback!
On Sun, Mar 2, 2025 at 1:14 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu, 20 Feb 2025 10:42:25 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Add the single tap feature with a threshold in 62.5mg/LSB points and a
> > scaled duration in us. Keep singletap threshold in regmap cache but
> > the scaled value of duration in us as member variable.
> >
> > Both use IIO channels for individual enable of the x/y/z axis. Initializes
> > threshold and duration with reasonable content. When an interrupt is
> > caught it will be pushed to the according IIO channel.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > ---
> > drivers/iio/accel/adxl345_core.c | 364 ++++++++++++++++++++++++++++++-
> > 1 file changed, 362 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > index 0cee81bc1877..d05593c0d513 100644
> > --- a/drivers/iio/accel/adxl345_core.c
> > +++ b/drivers/iio/accel/adxl345_core.c
> > @@ -8,6 +8,7 @@
> > */
> >
> > #include <linux/bitfield.h>
> > +#include <linux/bitops.h>
> > #include <linux/interrupt.h>
> > #include <linux/module.h>
> > #include <linux/property.h>
> > @@ -17,6 +18,7 @@
> > #include <linux/iio/iio.h>
> > #include <linux/iio/sysfs.h>
> > #include <linux/iio/buffer.h>
> > +#include <linux/iio/events.h>
> > #include <linux/iio/kfifo_buf.h>
> >
> > #include "adxl345.h"
> > @@ -31,6 +33,33 @@
> > #define ADXL345_INT1 0
> > #define ADXL345_INT2 1
> >
> > +#define ADXL345_REG_TAP_AXIS_MSK GENMASK(2, 0)
> This is a bit confusing. Here we have a mask for axis that
> has 3 bits.
> > +
> > +enum adxl345_axis {
> > + ADXL345_Z_EN = BIT(0),
> > + ADXL345_Y_EN = BIT(1),
> > + ADXL345_X_EN = BIT(2),
> > + /* Suppress double tap detection if value > tap threshold */
> > + ADXL345_TAP_SUPPRESS = BIT(3),
> and here an enum that is closely related with 4.
I see your point. There are several registers used in the sensor for
directions. A status register for tap and activity directions, and a
activity/inactivity direction register. For Tap, direction enables are
stored using the suppress bit in the fourth position. All those
locations use actually four bit. Partly the upper four, partly the
lower four. That's why I use here four bit for reading and writing.
The locations 0, 1, 2 then can be used directly. Location 3 only
applies to tap detection.
I'll keep this in v4 patches, and hope to understand you correctly
that this is not a "real" issue?
>
> > +};
> ...
>
> > enum adxl345_chans {
> > @@ -83,6 +128,7 @@ bool adxl345_is_volatile_reg(struct device *dev, unsigned int reg)
> > case ADXL345_REG_DATA_AXIS(3):
> > case ADXL345_REG_DATA_AXIS(4):
> > case ADXL345_REG_DATA_AXIS(5):
> > + case ADXL345_REG_ACT_TAP_STATUS:
>
> ah. I thought this had a full list from earlier patches.
> Move this and any later additions back to patch 4.
>
> > case ADXL345_REG_FIFO_STATUS:
> > case ADXL345_REG_INT_SOURCE:
> > return true;
> > @@ -112,6 +158,117 @@ static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
> > return regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
> > }
> >
> > +/* tap */
> > +
> > +static int adxl345_write_tap_axis(struct adxl345_state *st,
> > + enum adxl345_axis axis, bool en)
> > +{
> > + st->tap_axis_ctrl = FIELD_GET(ADXL345_REG_TAP_AXIS_MSK,
> > + en ? st->tap_axis_ctrl | axis
> > + : st->tap_axis_ctrl & ~axis);
> > +
> > + return regmap_update_bits(st->regmap, ADXL345_REG_TAP_AXIS,
> > + ADXL345_REG_TAP_AXIS_MSK,
> > + FIELD_PREP(ADXL345_REG_TAP_AXIS_MSK,
> > + st->tap_axis_ctrl));
> Given that mask is bottom few bits anyway and you can just define the
> tap axis as separate fields.
>
> See below, but I would push the IIO_MOD values down into here. Put the switch
> per axis in at this level and then use a simple if statement to
> call regmap_clear_bits() / regmap_set_bits() based on enable.
>
Thank you for this hint. I reimplemented that, though, I was guessing
quite a bit
how you might like to have the source. Please have a look into
upcoming v4 series and let me
know if I got it wrong.
> > +}
>
> ...
>
> > +static int adxl345_is_tap_en(struct adxl345_state *st,
> > + enum adxl345_tap_type type, bool *en)
> > +{
> > + int ret;
> > + unsigned int regval;
> > +
> > + ret = regmap_read(st->regmap, ADXL345_REG_INT_ENABLE, ®val);
> > + if (ret)
> > + return ret;
> > +
> > + *en = (adxl345_tap_int_reg[type] & regval) > 0;
>
> Could use 0/1 return rather than passing a paramter for the output.
> I don't mind much either way.
>
Hum. I took this rather as suggestion, and I will keep this as is for
now. I'm a bit unsure, on
the one side I'm using the return value for error handling (return
ret), on the other side there
is the enable. If using enable in the return, this would mix up, but
probably not a big issue.
It feels rather then to me, if the function is really needed, or if I
can put this in another place.
Anyway, I think it could/should be fine like that. Let me know in v4
context, what you think.
>
> > +
> > + return 0;
> > +}
> > +
> > +static int adxl345_set_singletap_en(struct adxl345_state *st,
> I'd push the IIO modifier in here instead of axis.
> > + enum adxl345_axis axis, bool en)
> > +{
> > + int ret;
> > +
> > + ret = adxl345_write_tap_axis(st, axis, en);
> And push it into here as well.
>
> > + if (ret)
> > + return ret;
> > +
> > + return _adxl345_set_tap_int(st, ADXL345_SINGLE_TAP, en);
> > +}
> > +
>
> ...
>
> > @@ -197,6 +354,160 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
> > return -EINVAL;
> > }
> >
> > +static int adxl345_read_event_config(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir)
> > +{
> > + struct adxl345_state *st = iio_priv(indio_dev);
> > + bool int_en;
> > + bool axis_en;
> > + int ret = -EFAULT;
> > +
> > + switch (type) {
> > + case IIO_EV_TYPE_GESTURE:
> > + switch (chan->channel2) {
> > + case IIO_MOD_X:
> > + axis_en = FIELD_GET(ADXL345_X_EN, st->tap_axis_ctrl);
> > + break;
> > + case IIO_MOD_Y:
> > + axis_en = FIELD_GET(ADXL345_Y_EN, st->tap_axis_ctrl);
> > + break;
> > + case IIO_MOD_Z:
> > + axis_en = FIELD_GET(ADXL345_Z_EN, st->tap_axis_ctrl);
> > + break;
> > + default:
> > + axis_en = ADXL345_TAP_SUPPRESS;
> > + break;
> > + }
> I'd check axis_en here.
> if (!axis_en)
> return false;
> > +
> > + switch (dir) {
> > + case IIO_EV_DIR_SINGLETAP:
> > + ret = adxl345_is_tap_en(st, ADXL345_SINGLE_TAP, &int_en);
> > + if (ret)
> > + return ret;
> > + return int_en && axis_en;
>
> Then this just becomes return int_en;
>
> > + default:
> > + return -EINVAL;
> > + }
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int adxl345_write_event_config(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir,
> > + int state)
> > +{
> > + struct adxl345_state *st = iio_priv(indio_dev);
> > + enum adxl345_axis axis;
> > +
> > + switch (type) {
> > + case IIO_EV_TYPE_GESTURE:
> > + switch (chan->channel2) {
> > + case IIO_MOD_X:
> > + axis = ADXL345_X_EN;
> > + break;
> > + case IIO_MOD_Y:
> > + axis = ADXL345_Y_EN;
> > + break;
> > + case IIO_MOD_Z:
> > + axis = ADXL345_Z_EN;
> > + break;
> > + default:
> > + axis = ADXL345_TAP_SUPPRESS;
>
> How are we getting another axis here?
:)
> > + break;
> > + }
> > +
> > + switch (dir) {
> > + case IIO_EV_DIR_SINGLETAP:
> > + return adxl345_set_singletap_en(st, axis, state);
>
> As above, pass chan->channel2 into here so we can simplify the eventual
> setting of the the register values.
>
> > + default:
> > + return -EINVAL;
> > + }
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int adxl345_read_event_value(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir,
> > + enum iio_event_info info,
> > + int *val, int *val2)
> > +{
> > + struct adxl345_state *st = iio_priv(indio_dev);
> > + unsigned int tap_threshold;
> > + int ret;
> > +
> > + switch (type) {
> > + case IIO_EV_TYPE_GESTURE:
> > + switch (info) {
> > + case IIO_EV_INFO_VALUE:
> > + /*
> > + * The scale factor is 62.5mg/LSB (i.e. 0xFF = 16g) but
> > + * not applied here.
>
> Maybe say why.
>
Usually I did scaling for the time values. Time values is something I
can understand someone
rather wants to configure in corresponding time units, such as [ms],
[us] or [s] rather than bit
values. For [mg] values, franckly speaking, I imagine this is a bit overkill.
The threshold quite often is rather expect to be higher or lower,
depending a bit on variation of
the measurements. In the context of this rather "cheap" sensor, I
guess I'm not putting up a
seismic instrument, but rather generic tap detection, freefall or
general activity in a general
purpose context such as gaming, or the like. Let me know if this
assumption here is too lazy.
I added a bit more comment to the source, pls have a look into v4.
> > + */
> > + ret = regmap_read(st->regmap, ADXL345_REG_THRESH_TAP, &tap_threshold);
>
> Bit of a long line. Aim for sub 80 chars unless readability is greatly
> hurt.
>
> > + if (ret)
> > + return ret;
> > + *val = sign_extend32(tap_threshold, 7);
> > + return IIO_VAL_INT;
> > + case IIO_EV_INFO_TIMEOUT:
> > + *val = st->tap_duration_us;
> > + *val2 = 1000000;
> > + return IIO_VAL_FRACTIONAL;
> > + default:
> > + return -EINVAL;
> > + }
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> ...
>
On Thu, 13 Mar 2025 17:29:08 +0100
Lothar Rubusch <l.rubusch@gmail.com> wrote:
> Hi Jonathan,
>
> I prepared, reorganized and tested a v4 patch set. Given that I see
> how busy you must be these days with current increased mail traffic
> just in IIO ML when I compare it to some years ago,
> I don't want to bother you too much.
Thanks! It's a travel heavy month which always messes with finding
time for reviews. Too much bored time on planes with not enough space
to open a laptop properly!
> Some particular doubts I will inline down below. If possible with your
> work flow and to avoid giving you extra work, I'd like to ask you to
> read the questions here, but to give your answers right to the
> v4 patch set (so, i.e. after having seen the current state of source).
> It also should make my points
> a bit clearer. Anyway, this is just my idea, since I'm always happy
> about any feedback!
I might reply there as well, but it's easy to put a few comments here.
>
> On Sun, Mar 2, 2025 at 1:14 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Thu, 20 Feb 2025 10:42:25 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >
> > > Add the single tap feature with a threshold in 62.5mg/LSB points and a
> > > scaled duration in us. Keep singletap threshold in regmap cache but
> > > the scaled value of duration in us as member variable.
> > >
> > > Both use IIO channels for individual enable of the x/y/z axis. Initializes
> > > threshold and duration with reasonable content. When an interrupt is
> > > caught it will be pushed to the according IIO channel.
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > ---
> > > drivers/iio/accel/adxl345_core.c | 364 ++++++++++++++++++++++++++++++-
> > > 1 file changed, 362 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > > index 0cee81bc1877..d05593c0d513 100644
> > > --- a/drivers/iio/accel/adxl345_core.c
> > > +++ b/drivers/iio/accel/adxl345_core.c
> > > @@ -8,6 +8,7 @@
> > > */
> > >
> > > #include <linux/bitfield.h>
> > > +#include <linux/bitops.h>
> > > #include <linux/interrupt.h>
> > > #include <linux/module.h>
> > > #include <linux/property.h>
> > > @@ -17,6 +18,7 @@
> > > #include <linux/iio/iio.h>
> > > #include <linux/iio/sysfs.h>
> > > #include <linux/iio/buffer.h>
> > > +#include <linux/iio/events.h>
> > > #include <linux/iio/kfifo_buf.h>
> > >
> > > #include "adxl345.h"
> > > @@ -31,6 +33,33 @@
> > > #define ADXL345_INT1 0
> > > #define ADXL345_INT2 1
> > >
> > > +#define ADXL345_REG_TAP_AXIS_MSK GENMASK(2, 0)
> > This is a bit confusing. Here we have a mask for axis that
> > has 3 bits.
> > > +
> > > +enum adxl345_axis {
> > > + ADXL345_Z_EN = BIT(0),
> > > + ADXL345_Y_EN = BIT(1),
> > > + ADXL345_X_EN = BIT(2),
> > > + /* Suppress double tap detection if value > tap threshold */
> > > + ADXL345_TAP_SUPPRESS = BIT(3),
> > and here an enum that is closely related with 4.
>
> I see your point. There are several registers used in the sensor for
> directions. A status register for tap and activity directions, and a
> activity/inactivity direction register. For Tap, direction enables are
> stored using the suppress bit in the fourth position. All those
> locations use actually four bit. Partly the upper four, partly the
> lower four. That's why I use here four bit for reading and writing.
> The locations 0, 1, 2 then can be used directly. Location 3 only
> applies to tap detection.
>
> I'll keep this in v4 patches, and hope to understand you correctly
> that this is not a "real" issue?
I'd split the AXIS_MSK into two parts. One with just the axes
(make it the | of the 3 separate bits) and another one with suppress bit.
I'm not sure an enum really helps. Maybe better to just have defines.
> > > +
> > > +static int adxl345_read_event_value(struct iio_dev *indio_dev,
> > > + const struct iio_chan_spec *chan,
> > > + enum iio_event_type type,
> > > + enum iio_event_direction dir,
> > > + enum iio_event_info info,
> > > + int *val, int *val2)
> > > +{
> > > + struct adxl345_state *st = iio_priv(indio_dev);
> > > + unsigned int tap_threshold;
> > > + int ret;
> > > +
> > > + switch (type) {
> > > + case IIO_EV_TYPE_GESTURE:
> > > + switch (info) {
> > > + case IIO_EV_INFO_VALUE:
> > > + /*
> > > + * The scale factor is 62.5mg/LSB (i.e. 0xFF = 16g) but
> > > + * not applied here.
> >
> > Maybe say why.
> >
>
> Usually I did scaling for the time values. Time values is something I
> can understand someone
> rather wants to configure in corresponding time units, such as [ms],
> [us] or [s] rather than bit
> values. For [mg] values, franckly speaking, I imagine this is a bit overkill.
>
> The threshold quite often is rather expect to be higher or lower,
> depending a bit on variation of
> the measurements. In the context of this rather "cheap" sensor, I
> guess I'm not putting up a
> seismic instrument, but rather generic tap detection, freefall or
> general activity in a general
> purpose context such as gaming, or the like. Let me know if this
> assumption here is too lazy.
Generally event values would be matched to scale of _RAW but for a
gesture, it is indeed a rather vague thing. I'll take a look at
the new code and think about this a bit.
Jonathan
© 2016 - 2025 Red Hat, Inc.