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 | 365 ++++++++++++++++++++++++++++++-
1 file changed, 363 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index d4c1a6f1559f..cfdbc73c96b4 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_duration_us;
+
__le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE + 1] __aligned(IIO_DMA_MINALIGN);
};
+static struct iio_event_spec adxl345_events[] = {
+ {
+ /* single tap */
+ .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 {
@@ -113,6 +158,150 @@ 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_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;
+ u32 tap_axis_ctrl;
+ int ret;
+
+ ret = regmap_read(st->regmap, ADXL345_REG_TAP_AXIS, &tap_axis_ctrl);
+ if (ret)
+ return ret;
+
+ axis_valid = FIELD_GET(ADXL345_REG_TAP_AXIS_MSK, 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 iio_modifier axis,
+ enum adxl345_tap_type type, bool *en)
+{
+ unsigned int regval;
+ bool axis_en;
+ u32 axis_ctrl;
+ int ret;
+
+ ret = regmap_read(st->regmap, ADXL345_REG_TAP_AXIS, &axis_ctrl);
+ if (ret)
+ return ret;
+
+ switch (axis) {
+ case IIO_MOD_X:
+ axis_en = FIELD_GET(ADXL345_X_EN, axis_ctrl);
+ break;
+ case IIO_MOD_Y:
+ axis_en = FIELD_GET(ADXL345_Y_EN, axis_ctrl);
+ break;
+ case IIO_MOD_Z:
+ axis_en = FIELD_GET(ADXL345_Z_EN, axis_ctrl);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ 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 iio_modifier axis, bool en)
+{
+ int ret;
+ enum adxl345_axis axis_ctrl;
+
+ switch (axis) {
+ case IIO_MOD_X:
+ axis_ctrl = ADXL345_X_EN;
+ break;
+ case IIO_MOD_Y:
+ axis_ctrl = ADXL345_Y_EN;
+ break;
+ case IIO_MOD_Z:
+ axis_ctrl = ADXL345_Z_EN;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ if (en)
+ ret = regmap_set_bits(st->regmap, ADXL345_REG_TAP_AXIS,
+ axis_ctrl);
+ else
+ ret = regmap_clear_bits(st->regmap, ADXL345_REG_TAP_AXIS,
+ axis_ctrl);
+ 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)
@@ -198,6 +387,132 @@ 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;
+ int ret = -EFAULT;
+
+ switch (type) {
+ case IIO_EV_TYPE_GESTURE:
+ switch (dir) {
+ case IIO_EV_DIR_SINGLETAP:
+ ret = adxl345_is_tap_en(st, chan->channel2,
+ ADXL345_SINGLE_TAP, &int_en);
+ if (ret)
+ return ret;
+ 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);
+
+ switch (type) {
+ case IIO_EV_TYPE_GESTURE:
+ switch (dir) {
+ case IIO_EV_DIR_SINGLETAP:
+ return adxl345_set_singletap_en(st, chan->channel2, 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 would be 62.5mg/LSB (i.e. 0xFF = 16g) but
+ * not applied here. In context of this general purpose sensor,
+ * what imports is rather signal intensity than the absolute
+ * measured g value.
+ */
+ 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)
{
@@ -420,12 +735,24 @@ 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)
+static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
+ enum iio_modifier tap_dir)
{
+ s64 ts = iio_get_time_ns(indio_dev);
struct adxl345_state *st = iio_priv(indio_dev);
int samples;
int ret = -ENOENT;
+ if (FIELD_GET(ADXL345_INT_SINGLE_TAP, int_stat)) {
+ ret = iio_push_event(indio_dev,
+ IIO_MOD_EVENT_CODE(IIO_ACCEL, 0, tap_dir,
+ IIO_EV_TYPE_GESTURE,
+ IIO_EV_DIR_SINGLETAP),
+ ts);
+ if (ret)
+ return ret;
+ }
+
if (FIELD_GET(ADXL345_INT_WATERMARK, int_stat)) {
samples = adxl345_get_samples(st);
if (samples < 0)
@@ -449,12 +776,33 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
{
struct iio_dev *indio_dev = p;
struct adxl345_state *st = iio_priv(indio_dev);
+ unsigned int regval;
+ enum iio_modifier tap_dir = IIO_NO_MOD;
+ u32 axis_ctrl;
int int_stat;
+ int ret;
+
+ ret = regmap_read(st->regmap, ADXL345_REG_TAP_AXIS, &axis_ctrl);
+ if (ret)
+ return IRQ_NONE;
+
+ if (FIELD_GET(ADXL345_REG_TAP_AXIS_MSK, axis_ctrl)) {
+ ret = regmap_read(st->regmap, ADXL345_REG_ACT_TAP_STATUS, ®val);
+ if (ret)
+ return IRQ_NONE;
+
+ if (FIELD_GET(ADXL345_Z_EN, regval))
+ tap_dir = IIO_MOD_Z;
+ else if (FIELD_GET(ADXL345_Y_EN, regval))
+ tap_dir = IIO_MOD_Y;
+ else if (FIELD_GET(ADXL345_X_EN, regval))
+ 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))
+ if (adxl345_push_event(indio_dev, int_stat, tap_dir))
goto err;
if (FIELD_GET(ADXL345_INT_OVERRUN, int_stat))
@@ -473,6 +821,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,
};
@@ -506,6 +858,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));
@@ -519,6 +872,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;
@@ -591,6 +948,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, 13 Mar 2025 16:50:40 +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>
Hi Lothar,
A few things in here are from the discussion that was continuing
on v3 so I may have said more replying to that.
Anyhow, for now I'll hold off on applying from this point on as
a few more things to respond to inline.
Jonathan
>
> #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),
> +};
As per feedback (after you sent this!) on v3, I'd drop
the last value out of the enum, or just use defines and a u8 for
the one place this is used for local variable storage.
> @@ -198,6 +387,132 @@ 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;
> + int ret = -EFAULT;
Not used?
> +
> + switch (type) {
> + case IIO_EV_TYPE_GESTURE:
> + switch (dir) {
> + case IIO_EV_DIR_SINGLETAP:
> + ret = adxl345_is_tap_en(st, chan->channel2,
> + ADXL345_SINGLE_TAP, &int_en);
> + if (ret)
> + return ret;
> + return int_en;
> + 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;
> +
So in my brief reply to the v3 discussion I suggested perhaps
factoring out everything from here...
> + 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;
> + }
to here, so as to allow simple direct returns.
I think that will make the code more readable given the need to reenable
measurements and that you want to leave it off on error.
> +
> + if (ret)
> + return ret; /* measurement stays off */
> +
> + return adxl345_set_measure_en(st, true);
> +}
On Sun, Mar 16, 2025 at 12:22 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu, 13 Mar 2025 16:50:40 +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>
>
> Hi Lothar,
>
> A few things in here are from the discussion that was continuing
> on v3 so I may have said more replying to that.
>
> Anyhow, for now I'll hold off on applying from this point on as
> a few more things to respond to inline.
>
> Jonathan
>
> >
> > #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),
> > +};
> As per feedback (after you sent this!) on v3, I'd drop
> the last value out of the enum, or just use defines and a u8 for
> the one place this is used for local variable storage.
>
>
> > @@ -198,6 +387,132 @@ 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;
> > + int ret = -EFAULT;
> Not used?
>
> > +
> > + switch (type) {
> > + case IIO_EV_TYPE_GESTURE:
> > + switch (dir) {
> > + case IIO_EV_DIR_SINGLETAP:
> > + ret = adxl345_is_tap_en(st, chan->channel2,
> > + ADXL345_SINGLE_TAP, &int_en);
> > + if (ret)
> > + return ret;
> > + return int_en;
> > + 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;
> > +
> So in my brief reply to the v3 discussion I suggested perhaps
> factoring out everything from here...
> > + 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;
> > + }
> to here, so as to allow simple direct returns.
>
> I think that will make the code more readable given the need to reenable
> measurements and that you want to leave it off on error.
>
Sorry for replying again on this topic. Pls, find my solution in v5.
After some thinking, I implemented it now using returns directly leaving the
measurement on/off as is. I'm unsure if it actually makes sense, after an error
here to turn measurement on again? I can imagine a situation where a wrong
input might result in an error. Nothing is changed, and measurement
could/should continue. Now, it will probably stop, in case of wrong
input. But is
wrong input actually an issue here?
As other alternative, I can think of is to shift measurement on/off
into the called
functions directly. I think, this approach was used also in the
ADXL380 and seems
to be common. Let me know what you think.
> > +
> > + if (ret)
> > + return ret; /* measurement stays off */
> > +
> > + return adxl345_set_measure_en(st, true);
> > +}
>
On Wed, 19 Mar 2025 00:08:24 +0100
Lothar Rubusch <l.rubusch@gmail.com> wrote:
> On Sun, Mar 16, 2025 at 12:22 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Thu, 13 Mar 2025 16:50:40 +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>
> >
> > Hi Lothar,
> >
> > A few things in here are from the discussion that was continuing
> > on v3 so I may have said more replying to that.
> >
> > Anyhow, for now I'll hold off on applying from this point on as
> > a few more things to respond to inline.
> >
> > Jonathan
> >
> > >
> > > #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),
> > > +};
> > As per feedback (after you sent this!) on v3, I'd drop
> > the last value out of the enum, or just use defines and a u8 for
> > the one place this is used for local variable storage.
> >
> >
> > > @@ -198,6 +387,132 @@ 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;
> > > + int ret = -EFAULT;
> > Not used?
> >
> > > +
> > > + switch (type) {
> > > + case IIO_EV_TYPE_GESTURE:
> > > + switch (dir) {
> > > + case IIO_EV_DIR_SINGLETAP:
> > > + ret = adxl345_is_tap_en(st, chan->channel2,
> > > + ADXL345_SINGLE_TAP, &int_en);
> > > + if (ret)
> > > + return ret;
> > > + return int_en;
> > > + 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;
> > > +
> > So in my brief reply to the v3 discussion I suggested perhaps
> > factoring out everything from here...
> > > + 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;
> > > + }
> > to here, so as to allow simple direct returns.
> >
> > I think that will make the code more readable given the need to reenable
> > measurements and that you want to leave it off on error.
> >
>
> Sorry for replying again on this topic. Pls, find my solution in v5.
>
> After some thinking, I implemented it now using returns directly leaving the
> measurement on/off as is. I'm unsure if it actually makes sense, after an error
> here to turn measurement on again? I can imagine a situation where a wrong
> input might result in an error. Nothing is changed, and measurement
> could/should continue. Now, it will probably stop, in case of wrong
> input. But is
> wrong input actually an issue here?
If a userspace control input is out of range etc, then returning an error
but leaving things on makes sense. If what we see is a comms error
it gets less clear on what we should do.
>
> As other alternative, I can think of is to shift measurement on/off
> into the called
> functions directly. I think, this approach was used also in the
> ADXL380 and seems
> to be common. Let me know what you think.
>
Moving it up or down a layer can work by allowing direct returns and always
trying to reenable if that makes sense.
Sadly in error handling there is often not a right answer on what to do!
Jonathan
> > > +
> > > + if (ret)
> > > + return ret; /* measurement stays off */
> > > +
> > > + return adxl345_set_measure_en(st, true);
> > > +}
> >
On Sun, Mar 16, 2025 at 12:22 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu, 13 Mar 2025 16:50:40 +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>
>
> Hi Lothar,
>
> A few things in here are from the discussion that was continuing
> on v3 so I may have said more replying to that.
>
> Anyhow, for now I'll hold off on applying from this point on as
> a few more things to respond to inline.
>
> Jonathan
>
> >
> > #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),
> > +};
> As per feedback (after you sent this!) on v3, I'd drop
> the last value out of the enum, or just use defines and a u8 for
> the one place this is used for local variable storage.
>
>
> > @@ -198,6 +387,132 @@ 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;
> > + int ret = -EFAULT;
> Not used?
>
> > +
> > + switch (type) {
> > + case IIO_EV_TYPE_GESTURE:
> > + switch (dir) {
> > + case IIO_EV_DIR_SINGLETAP:
> > + ret = adxl345_is_tap_en(st, chan->channel2,
> > + ADXL345_SINGLE_TAP, &int_en);
> > + if (ret)
> > + return ret;
> > + return int_en;
> > + 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;
> > +
> So in my brief reply to the v3 discussion I suggested perhaps
> factoring out everything from here...
> > + 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;
> > + }
> to here, so as to allow simple direct returns.
>
> I think that will make the code more readable given the need to reenable
> measurements and that you want to leave it off on error.
>
> > +
> > + if (ret)
> > + return ret; /* measurement stays off */
> > +
> > + return adxl345_set_measure_en(st, true);
> > +}
>
Hi Jonathan, my reason to leave this part unchanged was the following:
As you can see above, 'return adxl345_set_measure_en(st, true);' - I'm
actually need to turn off measurement before changing values, then to
turn it on again after. In case of error, doing a simple "return ret"
would keep measurement generally off. I think I should generally try
to turn it on again after.
Alternatively I could do something with goto/label to turn measurement
on, but then return ret. My choice here was to use this ret approach
together with break, at cost of readability. Any advice?
© 2016 - 2025 Red Hat, Inc.