Add the freefall detection of the sensor together with a threshold and
time parameter. A freefall event is detected if the measuring signal
falls below the threshold.
Introduce a freefall threshold stored in regmap cache, and a freefall
time, having the scaled time value stored as a member variable in the
state instance.
Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
drivers/iio/accel/adxl345_core.c | 125 +++++++++++++++++++++++++++++++
1 file changed, 125 insertions(+)
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index c464c87033fb..ae02826e552b 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -75,6 +75,7 @@ struct adxl345_state {
u32 tap_duration_us;
u32 tap_latent_us;
u32 tap_window_us;
+ u32 ff_time_ms;
__le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE + 1] __aligned(IIO_DMA_MINALIGN);
};
@@ -96,6 +97,14 @@ static struct iio_event_spec adxl345_events[] = {
BIT(IIO_EV_INFO_RESET_TIMEOUT) |
BIT(IIO_EV_INFO_TAP2_MIN_DELAY),
},
+ {
+ /* free fall */
+ .type = IIO_EV_TYPE_MAG,
+ .dir = IIO_EV_DIR_FALLING,
+ .mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE) |
+ BIT(IIO_EV_INFO_VALUE) |
+ BIT(IIO_EV_INFO_PERIOD),
+ },
};
#define ADXL345_CHANNEL(index, reg, axis) { \
@@ -383,6 +392,63 @@ static int adxl345_set_tap_latent(struct adxl345_state *st, u32 val_int,
return _adxl345_set_tap_time(st, ADXL345_TAP_TIME_LATENT, val_fract_us);
}
+/* freefall */
+
+static int adxl345_is_ff_en(struct adxl345_state *st, bool *en)
+{
+ int ret;
+ unsigned int regval;
+
+ ret = regmap_read(st->regmap, ADXL345_REG_INT_ENABLE, ®val);
+ if (ret)
+ return ret;
+
+ *en = FIELD_GET(ADXL345_INT_FREE_FALL, regval) > 0;
+
+ return 0;
+}
+
+static int adxl345_set_ff_en(struct adxl345_state *st, bool cmd_en)
+{
+ unsigned int regval, ff_threshold;
+ bool en;
+ int ret;
+
+ ret = regmap_read(st->regmap, ADXL345_REG_THRESH_FF, &ff_threshold);
+ if (ret)
+ return ret;
+
+ en = cmd_en && ff_threshold > 0 && st->ff_time_ms > 0;
+
+ regval = en ? ADXL345_INT_FREE_FALL : 0x00;
+
+ return regmap_update_bits(st->regmap, ADXL345_REG_INT_ENABLE,
+ ADXL345_INT_FREE_FALL, regval);
+}
+
+static int adxl345_set_ff_time(struct adxl345_state *st, u32 val_int,
+ u32 val_fract_us)
+{
+ unsigned int regval;
+ int val_ms;
+
+ /*
+ * max value is 255 * 5000 us = 1.275000 seconds
+ *
+ * Note: the scaling is similar to the scaling in the ADXL380
+ */
+ if (1000000 * val_int + val_fract_us > 1275000)
+ return -EINVAL;
+
+ val_ms = val_int * 1000 + DIV_ROUND_UP(val_fract_us, 1000);
+ st->ff_time_ms = val_ms;
+
+ regval = DIV_ROUND_CLOSEST(val_ms, 5);
+
+ /* Values between 100ms and 350ms (0x14 to 0x46) are recommended. */
+ return regmap_write(st->regmap, ADXL345_REG_TIME_FF, min(regval, 0xff));
+}
+
static int adxl345_read_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int *val, int *val2, long mask)
@@ -495,6 +561,11 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
default:
return -EINVAL;
}
+ case IIO_EV_TYPE_MAG:
+ ret = adxl345_is_ff_en(st, &int_en);
+ if (ret)
+ return ret;
+ return int_en;
default:
return -EINVAL;
}
@@ -518,6 +589,8 @@ static int adxl345_write_event_config(struct iio_dev *indio_dev,
default:
return -EINVAL;
}
+ case IIO_EV_TYPE_MAG:
+ return adxl345_set_ff_en(st, state);
default:
return -EINVAL;
}
@@ -532,6 +605,7 @@ static int adxl345_read_event_value(struct iio_dev *indio_dev,
{
struct adxl345_state *st = iio_priv(indio_dev);
unsigned int tap_threshold;
+ unsigned int ff_threshold;
int ret;
switch (type) {
@@ -565,6 +639,22 @@ static int adxl345_read_event_value(struct iio_dev *indio_dev,
default:
return -EINVAL;
}
+ case IIO_EV_TYPE_MAG:
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ ret = regmap_read(st->regmap, ADXL345_REG_THRESH_FF,
+ &ff_threshold);
+ if (ret)
+ return ret;
+ *val = ff_threshold;
+ return IIO_VAL_INT;
+ case IIO_EV_INFO_PERIOD:
+ *val = st->ff_time_ms;
+ *val2 = 1000;
+ return IIO_VAL_FRACTIONAL;
+ default:
+ return -EINVAL;
+ }
default:
return -EINVAL;
}
@@ -612,6 +702,22 @@ static int adxl345_write_event_value(struct iio_dev *indio_dev,
return -EINVAL;
}
break;
+ case IIO_EV_TYPE_MAG:
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ ret = regmap_write(st->regmap, ADXL345_REG_THRESH_FF, val);
+ if (ret)
+ return ret;
+ break;
+ case IIO_EV_INFO_PERIOD:
+ ret = adxl345_set_ff_time(st, val, val2);
+ if (ret)
+ return ret;
+ break;
+ default:
+ return -EINVAL;
+ }
+ break;
default:
return -EINVAL;
}
@@ -865,6 +971,17 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
return ret;
}
+ if (FIELD_GET(ADXL345_INT_FREE_FALL, int_stat)) {
+ ret = iio_push_event(indio_dev,
+ IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
+ IIO_MOD_X_OR_Y_OR_Z,
+ IIO_EV_TYPE_MAG,
+ IIO_EV_DIR_FALLING),
+ ts);
+ if (ret)
+ return ret;
+ }
+
if (FIELD_GET(ADXL345_INT_WATERMARK, int_stat)) {
samples = adxl345_get_samples(st);
if (samples < 0)
@@ -973,6 +1090,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
ADXL345_DATA_FORMAT_FULL_RES |
ADXL345_DATA_FORMAT_SELF_TEST);
unsigned int tap_threshold;
+ unsigned int ff_threshold;
int ret;
indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
@@ -992,6 +1110,9 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
st->tap_window_us = 64; /* 64 [0x40] -> .080 */
st->tap_latent_us = 16; /* 16 [0x10] -> .020 */
+ ff_threshold = 8; /* 8 [0x08] */
+ st->ff_time_ms = 32; /* 32 [0x20] -> 0.16 */
+
indio_dev->name = st->info->name;
indio_dev->info = &adxl345_info;
indio_dev->modes = INDIO_DIRECT_MODE;
@@ -1068,6 +1189,10 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
if (ret)
return ret;
+ ret = regmap_write(st->regmap, ADXL345_REG_THRESH_FF, ff_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 Mon, 14 Apr 2025 18:42:39 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:
> Add the freefall detection of the sensor together with a threshold and
> time parameter. A freefall event is detected if the measuring signal
> falls below the threshold.
>
> Introduce a freefall threshold stored in regmap cache, and a freefall
> time, having the scaled time value stored as a member variable in the
> state instance.
>
Reading this I wondered whether we had the event code consistent for
freefall detectors... Or indeed inactivity ones (which are kind of similarish)
:( We don't it seems. The issue is that
freefall is actually that all channels are simultaneously under the the magnitude
threshold, not one of them. So it should I think be
X_AND_Y_AND_Z not X_OR_Y_OR_Z
This is as opposed to activity detectors which tend to be any axis shows
activity and X_OR_Y_OR_Z applies.
Anyhow upshot is I think I lead you astray on this and we should make this
one IIO_MOD_X_AND_Y_AND_Z
A few other things inline.
Unfortunately we don't deal with these events that often so I forget
what we did before :(
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
> drivers/iio/accel/adxl345_core.c | 125 +++++++++++++++++++++++++++++++
> 1 file changed, 125 insertions(+)
>
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index c464c87033fb..ae02826e552b 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -75,6 +75,7 @@ struct adxl345_state {
> u32 tap_duration_us;
> u32 tap_latent_us;
> u32 tap_window_us;
> + u32 ff_time_ms;
>
> __le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE + 1] __aligned(IIO_DMA_MINALIGN);
> };
> @@ -96,6 +97,14 @@ static struct iio_event_spec adxl345_events[] = {
> BIT(IIO_EV_INFO_RESET_TIMEOUT) |
> BIT(IIO_EV_INFO_TAP2_MIN_DELAY),
> },
> + {
> + /* free fall */
> + .type = IIO_EV_TYPE_MAG,
> + .dir = IIO_EV_DIR_FALLING,
> + .mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE) |
> + BIT(IIO_EV_INFO_VALUE) |
> + BIT(IIO_EV_INFO_PERIOD),
> + },
This is creating separate per axis enables, values and period. Does that make
sense? If not you need to spin a kind of virtual channel (with mod X_AND_Y_AND_Z)
and add the events to it.
See how the sca3000 does it for example.
> };
>
> #define ADXL345_CHANNEL(index, reg, axis) { \
> @@ -383,6 +392,63 @@ static int adxl345_set_tap_latent(struct adxl345_state *st, u32 val_int,
> return _adxl345_set_tap_time(st, ADXL345_TAP_TIME_LATENT, val_fract_us);
> }
>
> +/* freefall */
> +
> +static int adxl345_is_ff_en(struct adxl345_state *st, bool *en)
> +{
> + int ret;
> + unsigned int regval;
> +
> + ret = regmap_read(st->regmap, ADXL345_REG_INT_ENABLE, ®val);
> + if (ret)
> + return ret;
> +
> + *en = FIELD_GET(ADXL345_INT_FREE_FALL, regval) > 0;
> +
> + return 0;
> +}
> +
> +static int adxl345_set_ff_en(struct adxl345_state *st, bool cmd_en)
> +{
> + unsigned int regval, ff_threshold;
> + bool en;
> + int ret;
> +
> + ret = regmap_read(st->regmap, ADXL345_REG_THRESH_FF, &ff_threshold);
> + if (ret)
> + return ret;
> +
> + en = cmd_en && ff_threshold > 0 && st->ff_time_ms > 0;
> +
> + regval = en ? ADXL345_INT_FREE_FALL : 0x00;
> +
> + return regmap_update_bits(st->regmap, ADXL345_REG_INT_ENABLE,
> + ADXL345_INT_FREE_FALL, regval);
> +}
> +
> +static int adxl345_set_ff_time(struct adxl345_state *st, u32 val_int,
> + u32 val_fract_us)
> +{
> + unsigned int regval;
> + int val_ms;
> +
> + /*
> + * max value is 255 * 5000 us = 1.275000 seconds
> + *
> + * Note: the scaling is similar to the scaling in the ADXL380
> + */
> + if (1000000 * val_int + val_fract_us > 1275000)
> + return -EINVAL;
> +
> + val_ms = val_int * 1000 + DIV_ROUND_UP(val_fract_us, 1000);
> + st->ff_time_ms = val_ms;
> +
> + regval = DIV_ROUND_CLOSEST(val_ms, 5);
> +
> + /* Values between 100ms and 350ms (0x14 to 0x46) are recommended. */
> + return regmap_write(st->regmap, ADXL345_REG_TIME_FF, min(regval, 0xff));
> +}
> +
> static int adxl345_read_raw(struct iio_dev *indio_dev,
> struct iio_chan_spec const *chan,
> int *val, int *val2, long mask)
> @@ -495,6 +561,11 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
> default:
> return -EINVAL;
> }
> + case IIO_EV_TYPE_MAG:
> + ret = adxl345_is_ff_en(st, &int_en);
> + if (ret)
> + return ret;
> + return int_en;
> default:
> return -EINVAL;
> }
> @@ -518,6 +589,8 @@ static int adxl345_write_event_config(struct iio_dev *indio_dev,
> default:
> return -EINVAL;
> }
> + case IIO_EV_TYPE_MAG:
> + return adxl345_set_ff_en(st, state);
> default:
> return -EINVAL;
> }
> @@ -532,6 +605,7 @@ static int adxl345_read_event_value(struct iio_dev *indio_dev,
> {
> struct adxl345_state *st = iio_priv(indio_dev);
> unsigned int tap_threshold;
> + unsigned int ff_threshold;
> int ret;
>
> switch (type) {
> @@ -565,6 +639,22 @@ static int adxl345_read_event_value(struct iio_dev *indio_dev,
> default:
> return -EINVAL;
> }
> + case IIO_EV_TYPE_MAG:
> + switch (info) {
> + case IIO_EV_INFO_VALUE:
> + ret = regmap_read(st->regmap, ADXL345_REG_THRESH_FF,
> + &ff_threshold);
> + if (ret)
> + return ret;
> + *val = ff_threshold;
> + return IIO_VAL_INT;
> + case IIO_EV_INFO_PERIOD:
> + *val = st->ff_time_ms;
> + *val2 = 1000;
> + return IIO_VAL_FRACTIONAL;
> + default:
> + return -EINVAL;
> + }
> default:
> return -EINVAL;
> }
> @@ -612,6 +702,22 @@ static int adxl345_write_event_value(struct iio_dev *indio_dev,
> return -EINVAL;
> }
> break;
> + case IIO_EV_TYPE_MAG:
> + switch (info) {
> + case IIO_EV_INFO_VALUE:
> + ret = regmap_write(st->regmap, ADXL345_REG_THRESH_FF, val);
> + if (ret)
> + return ret;
> + break;
> + case IIO_EV_INFO_PERIOD:
> + ret = adxl345_set_ff_time(st, val, val2);
> + if (ret)
> + return ret;
> + break;
> + default:
> + return -EINVAL;
> + }
> + break;
> default:
> return -EINVAL;
> }
> @@ -865,6 +971,17 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
> return ret;
> }
>
> + if (FIELD_GET(ADXL345_INT_FREE_FALL, int_stat)) {
> + ret = iio_push_event(indio_dev,
> + IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> + IIO_MOD_X_OR_Y_OR_Z,
This is the event that got me thinking about whether we were doing this right..
> + IIO_EV_TYPE_MAG,
> + IIO_EV_DIR_FALLING),
> + ts);
> + if (ret)
> + return ret;
> + }
> +
Happy Easter!
On Fri, Apr 18, 2025 at 8:23 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon, 14 Apr 2025 18:42:39 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Add the freefall detection of the sensor together with a threshold and
> > time parameter. A freefall event is detected if the measuring signal
> > falls below the threshold.
> >
> > Introduce a freefall threshold stored in regmap cache, and a freefall
> > time, having the scaled time value stored as a member variable in the
> > state instance.
> >
> Reading this I wondered whether we had the event code consistent for
> freefall detectors... Or indeed inactivity ones (which are kind of similarish)
>
> :( We don't it seems. The issue is that
> freefall is actually that all channels are simultaneously under the the magnitude
> threshold, not one of them. So it should I think be
> X_AND_Y_AND_Z not X_OR_Y_OR_Z
>
I change to X_AND_Y_AND_Z.
> This is as opposed to activity detectors which tend to be any axis shows
> activity and X_OR_Y_OR_Z applies.
>
> Anyhow upshot is I think I lead you astray on this and we should make this
> one IIO_MOD_X_AND_Y_AND_Z
>
> A few other things inline.
>
> Unfortunately we don't deal with these events that often so I forget
> what we did before :(
>
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > ---
> > drivers/iio/accel/adxl345_core.c | 125 +++++++++++++++++++++++++++++++
> > 1 file changed, 125 insertions(+)
> >
> > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > index c464c87033fb..ae02826e552b 100644
> > --- a/drivers/iio/accel/adxl345_core.c
> > +++ b/drivers/iio/accel/adxl345_core.c
> > @@ -75,6 +75,7 @@ struct adxl345_state {
> > u32 tap_duration_us;
> > u32 tap_latent_us;
> > u32 tap_window_us;
> > + u32 ff_time_ms;
> >
> > __le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE + 1] __aligned(IIO_DMA_MINALIGN);
> > };
> > @@ -96,6 +97,14 @@ static struct iio_event_spec adxl345_events[] = {
> > BIT(IIO_EV_INFO_RESET_TIMEOUT) |
> > BIT(IIO_EV_INFO_TAP2_MIN_DELAY),
> > },
> > + {
> > + /* free fall */
> > + .type = IIO_EV_TYPE_MAG,
> > + .dir = IIO_EV_DIR_FALLING,
> > + .mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE) |
> > + BIT(IIO_EV_INFO_VALUE) |
> > + BIT(IIO_EV_INFO_PERIOD),
> > + },
> This is creating separate per axis enables, values and period. Does that make
> sense? If not you need to spin a kind of virtual channel (with mod X_AND_Y_AND_Z)
> and add the events to it.
>
> See how the sca3000 does it for example.
Hum, I'm not sure if I understand you correctly. In my driver, I'm
using .mask_shared_by_type, and I verified there appears only one
enable, one value and one period handle.
# ls -l /sys/bus/iio/devices/iio:device0/events/
total 0
...
-rw-r--r-- 1 root root 4096 Apr 20 21:59 in_accel_mag_falling_en
-rw-r--r-- 1 root root 4096 Apr 20 21:59 in_accel_mag_falling_period
-rw-r--r-- 1 root root 4096 Apr 20 21:59 in_accel_mag_falling_value
...
In the sources of sca3000.c I saw this setup with .mask_separate. So,
there I'd expect to see separate enables per axis, or am I wrong? In
the case of the ADXL345, there should only be one freefall enable (in
my driver) and not per axis. So, isn't this what is currently there?
So far I only adjust the or'ing to and'ing the axis for freefall.
> > };
> >
> > #define ADXL345_CHANNEL(index, reg, axis) { \
> > @@ -383,6 +392,63 @@ static int adxl345_set_tap_latent(struct adxl345_state *st, u32 val_int,
> > return _adxl345_set_tap_time(st, ADXL345_TAP_TIME_LATENT, val_fract_us);
> > }
> >
> > +/* freefall */
> > +
> > +static int adxl345_is_ff_en(struct adxl345_state *st, bool *en)
> > +{
> > + int ret;
> > + unsigned int regval;
> > +
> > + ret = regmap_read(st->regmap, ADXL345_REG_INT_ENABLE, ®val);
> > + if (ret)
> > + return ret;
> > +
> > + *en = FIELD_GET(ADXL345_INT_FREE_FALL, regval) > 0;
> > +
> > + return 0;
> > +}
> > +
> > +static int adxl345_set_ff_en(struct adxl345_state *st, bool cmd_en)
> > +{
> > + unsigned int regval, ff_threshold;
> > + bool en;
> > + int ret;
> > +
> > + ret = regmap_read(st->regmap, ADXL345_REG_THRESH_FF, &ff_threshold);
> > + if (ret)
> > + return ret;
> > +
> > + en = cmd_en && ff_threshold > 0 && st->ff_time_ms > 0;
> > +
> > + regval = en ? ADXL345_INT_FREE_FALL : 0x00;
> > +
> > + return regmap_update_bits(st->regmap, ADXL345_REG_INT_ENABLE,
> > + ADXL345_INT_FREE_FALL, regval);
> > +}
> > +
> > +static int adxl345_set_ff_time(struct adxl345_state *st, u32 val_int,
> > + u32 val_fract_us)
> > +{
> > + unsigned int regval;
> > + int val_ms;
> > +
> > + /*
> > + * max value is 255 * 5000 us = 1.275000 seconds
> > + *
> > + * Note: the scaling is similar to the scaling in the ADXL380
> > + */
> > + if (1000000 * val_int + val_fract_us > 1275000)
> > + return -EINVAL;
> > +
> > + val_ms = val_int * 1000 + DIV_ROUND_UP(val_fract_us, 1000);
> > + st->ff_time_ms = val_ms;
> > +
> > + regval = DIV_ROUND_CLOSEST(val_ms, 5);
> > +
> > + /* Values between 100ms and 350ms (0x14 to 0x46) are recommended. */
> > + return regmap_write(st->regmap, ADXL345_REG_TIME_FF, min(regval, 0xff));
> > +}
> > +
> > static int adxl345_read_raw(struct iio_dev *indio_dev,
> > struct iio_chan_spec const *chan,
> > int *val, int *val2, long mask)
> > @@ -495,6 +561,11 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
> > default:
> > return -EINVAL;
> > }
> > + case IIO_EV_TYPE_MAG:
> > + ret = adxl345_is_ff_en(st, &int_en);
> > + if (ret)
> > + return ret;
> > + return int_en;
> > default:
> > return -EINVAL;
> > }
> > @@ -518,6 +589,8 @@ static int adxl345_write_event_config(struct iio_dev *indio_dev,
> > default:
> > return -EINVAL;
> > }
> > + case IIO_EV_TYPE_MAG:
> > + return adxl345_set_ff_en(st, state);
> > default:
> > return -EINVAL;
> > }
> > @@ -532,6 +605,7 @@ static int adxl345_read_event_value(struct iio_dev *indio_dev,
> > {
> > struct adxl345_state *st = iio_priv(indio_dev);
> > unsigned int tap_threshold;
> > + unsigned int ff_threshold;
> > int ret;
> >
> > switch (type) {
> > @@ -565,6 +639,22 @@ static int adxl345_read_event_value(struct iio_dev *indio_dev,
> > default:
> > return -EINVAL;
> > }
> > + case IIO_EV_TYPE_MAG:
> > + switch (info) {
> > + case IIO_EV_INFO_VALUE:
> > + ret = regmap_read(st->regmap, ADXL345_REG_THRESH_FF,
> > + &ff_threshold);
> > + if (ret)
> > + return ret;
> > + *val = ff_threshold;
> > + return IIO_VAL_INT;
> > + case IIO_EV_INFO_PERIOD:
> > + *val = st->ff_time_ms;
> > + *val2 = 1000;
> > + return IIO_VAL_FRACTIONAL;
> > + default:
> > + return -EINVAL;
> > + }
> > default:
> > return -EINVAL;
> > }
> > @@ -612,6 +702,22 @@ static int adxl345_write_event_value(struct iio_dev *indio_dev,
> > return -EINVAL;
> > }
> > break;
> > + case IIO_EV_TYPE_MAG:
> > + switch (info) {
> > + case IIO_EV_INFO_VALUE:
> > + ret = regmap_write(st->regmap, ADXL345_REG_THRESH_FF, val);
> > + if (ret)
> > + return ret;
> > + break;
> > + case IIO_EV_INFO_PERIOD:
> > + ret = adxl345_set_ff_time(st, val, val2);
> > + if (ret)
> > + return ret;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > + break;
> > default:
> > return -EINVAL;
> > }
> > @@ -865,6 +971,17 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
> > return ret;
> > }
> >
> > + if (FIELD_GET(ADXL345_INT_FREE_FALL, int_stat)) {
> > + ret = iio_push_event(indio_dev,
> > + IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> > + IIO_MOD_X_OR_Y_OR_Z,
>
> This is the event that got me thinking about whether we were doing this right..
>
> > + IIO_EV_TYPE_MAG,
> > + IIO_EV_DIR_FALLING),
> > + ts);
> > + if (ret)
> > + return ret;
> > + }
> > +
>
On Sun, 20 Apr 2025 23:26:47 +0200
Lothar Rubusch <l.rubusch@gmail.com> wrote:
> Happy Easter!
>
> On Fri, Apr 18, 2025 at 8:23 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Mon, 14 Apr 2025 18:42:39 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >
> > > Add the freefall detection of the sensor together with a threshold and
> > > time parameter. A freefall event is detected if the measuring signal
> > > falls below the threshold.
> > >
> > > Introduce a freefall threshold stored in regmap cache, and a freefall
> > > time, having the scaled time value stored as a member variable in the
> > > state instance.
> > >
> > Reading this I wondered whether we had the event code consistent for
> > freefall detectors... Or indeed inactivity ones (which are kind of similarish)
> >
> > :( We don't it seems. The issue is that
> > freefall is actually that all channels are simultaneously under the the magnitude
> > threshold, not one of them. So it should I think be
> > X_AND_Y_AND_Z not X_OR_Y_OR_Z
> >
>
> I change to X_AND_Y_AND_Z.
>
> > This is as opposed to activity detectors which tend to be any axis shows
> > activity and X_OR_Y_OR_Z applies.
> >
> > Anyhow upshot is I think I lead you astray on this and we should make this
> > one IIO_MOD_X_AND_Y_AND_Z
> >
> > A few other things inline.
> >
> > Unfortunately we don't deal with these events that often so I forget
> > what we did before :(
> >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > ---
> > > drivers/iio/accel/adxl345_core.c | 125 +++++++++++++++++++++++++++++++
> > > 1 file changed, 125 insertions(+)
> > >
> > > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > > index c464c87033fb..ae02826e552b 100644
> > > --- a/drivers/iio/accel/adxl345_core.c
> > > +++ b/drivers/iio/accel/adxl345_core.c
> > > @@ -75,6 +75,7 @@ struct adxl345_state {
> > > u32 tap_duration_us;
> > > u32 tap_latent_us;
> > > u32 tap_window_us;
> > > + u32 ff_time_ms;
> > >
> > > __le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE + 1] __aligned(IIO_DMA_MINALIGN);
> > > };
> > > @@ -96,6 +97,14 @@ static struct iio_event_spec adxl345_events[] = {
> > > BIT(IIO_EV_INFO_RESET_TIMEOUT) |
> > > BIT(IIO_EV_INFO_TAP2_MIN_DELAY),
> > > },
> > > + {
> > > + /* free fall */
> > > + .type = IIO_EV_TYPE_MAG,
> > > + .dir = IIO_EV_DIR_FALLING,
> > > + .mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE) |
> > > + BIT(IIO_EV_INFO_VALUE) |
> > > + BIT(IIO_EV_INFO_PERIOD),
> > > + },
> > This is creating separate per axis enables, values and period. Does that make
> > sense? If not you need to spin a kind of virtual channel (with mod X_AND_Y_AND_Z)
> > and add the events to it.
> >
> > See how the sca3000 does it for example.
>
> Hum, I'm not sure if I understand you correctly. In my driver, I'm
> using .mask_shared_by_type, and I verified there appears only one
> enable, one value and one period handle.
> # ls -l /sys/bus/iio/devices/iio:device0/events/
> total 0
> ...
> -rw-r--r-- 1 root root 4096 Apr 20 21:59 in_accel_mag_falling_en
> -rw-r--r-- 1 root root 4096 Apr 20 21:59 in_accel_mag_falling_period
> -rw-r--r-- 1 root root 4096 Apr 20 21:59 in_accel_mag_falling_value
> ...
>
> In the sources of sca3000.c I saw this setup with .mask_separate. So,
> there I'd expect to see separate enables per axis, or am I wrong? In
> the case of the ADXL345, there should only be one freefall enable (in
> my driver) and not per axis. So, isn't this what is currently there?
>
> So far I only adjust the or'ing to and'ing the axis for freefall.
So this is a messy corner of the ABI (because these are tricky to describe).
Shared by type usually means there is one attribute applying to all the
axis, but that they are reported separately, or potentially multiple events
/ _OR_ form used if we can distinguish exactly what the event is.
In this case there is no way for userspace to anticipate that the event
that might be generate is X_AND_Y_AND_Z. So for this
the ABI solution we came up with is that virtual channel and separate.
So you get something along the lines of
in_accel_x&y&z_mag_falling_en
in_accel_x&y&z_mag_falling_period
etc
The tricky remaining corner is this only makes sense if we always enable
all axis (which is typical for a freefall detector). If we get a device
that oddly has per axis free fall enables, then it would be hard and I
might argue nonsense to enable them separately anyway. Not true
here though I think.
Note that we may well have drivers using the ABI slightly differently for
freefall events which will be at least partly because I'd forgotten how
we resolved all this complexity long ago (that sca3000 driver is ancient!)
ABI like this is tricky to fix up, but we might want to consider some duplication
in those drivers so we standardize on one form for freefall (even if we have some
stray ABI from other possible solutions).
What we should definitely do is pull together some documentation on multi channel
event handling as the ABI docs are probably not enough.
Jonathan
On Mon, Apr 21, 2025 at 12:16 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sun, 20 Apr 2025 23:26:47 +0200
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Happy Easter!
> >
> > On Fri, Apr 18, 2025 at 8:23 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > On Mon, 14 Apr 2025 18:42:39 +0000
> > > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> > >
> > > > Add the freefall detection of the sensor together with a threshold and
> > > > time parameter. A freefall event is detected if the measuring signal
> > > > falls below the threshold.
> > > >
> > > > Introduce a freefall threshold stored in regmap cache, and a freefall
> > > > time, having the scaled time value stored as a member variable in the
> > > > state instance.
> > > >
> > > Reading this I wondered whether we had the event code consistent for
> > > freefall detectors... Or indeed inactivity ones (which are kind of similarish)
> > >
> > > :( We don't it seems. The issue is that
> > > freefall is actually that all channels are simultaneously under the the magnitude
> > > threshold, not one of them. So it should I think be
> > > X_AND_Y_AND_Z not X_OR_Y_OR_Z
> > >
> >
> > I change to X_AND_Y_AND_Z.
> >
> > > This is as opposed to activity detectors which tend to be any axis shows
> > > activity and X_OR_Y_OR_Z applies.
> > >
> > > Anyhow upshot is I think I lead you astray on this and we should make this
> > > one IIO_MOD_X_AND_Y_AND_Z
> > >
> > > A few other things inline.
> > >
> > > Unfortunately we don't deal with these events that often so I forget
> > > what we did before :(
> > >
> > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > > ---
> > > > drivers/iio/accel/adxl345_core.c | 125 +++++++++++++++++++++++++++++++
> > > > 1 file changed, 125 insertions(+)
> > > >
> > > > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > > > index c464c87033fb..ae02826e552b 100644
> > > > --- a/drivers/iio/accel/adxl345_core.c
> > > > +++ b/drivers/iio/accel/adxl345_core.c
> > > > @@ -75,6 +75,7 @@ struct adxl345_state {
> > > > u32 tap_duration_us;
> > > > u32 tap_latent_us;
> > > > u32 tap_window_us;
> > > > + u32 ff_time_ms;
> > > >
> > > > __le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE + 1] __aligned(IIO_DMA_MINALIGN);
> > > > };
> > > > @@ -96,6 +97,14 @@ static struct iio_event_spec adxl345_events[] = {
> > > > BIT(IIO_EV_INFO_RESET_TIMEOUT) |
> > > > BIT(IIO_EV_INFO_TAP2_MIN_DELAY),
> > > > },
> > > > + {
> > > > + /* free fall */
> > > > + .type = IIO_EV_TYPE_MAG,
> > > > + .dir = IIO_EV_DIR_FALLING,
> > > > + .mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE) |
> > > > + BIT(IIO_EV_INFO_VALUE) |
> > > > + BIT(IIO_EV_INFO_PERIOD),
> > > > + },
> > > This is creating separate per axis enables, values and period. Does that make
> > > sense? If not you need to spin a kind of virtual channel (with mod X_AND_Y_AND_Z)
> > > and add the events to it.
> > >
> > > See how the sca3000 does it for example.
> >
> > Hum, I'm not sure if I understand you correctly. In my driver, I'm
> > using .mask_shared_by_type, and I verified there appears only one
> > enable, one value and one period handle.
> > # ls -l /sys/bus/iio/devices/iio:device0/events/
> > total 0
> > ...
> > -rw-r--r-- 1 root root 4096 Apr 20 21:59 in_accel_mag_falling_en
> > -rw-r--r-- 1 root root 4096 Apr 20 21:59 in_accel_mag_falling_period
> > -rw-r--r-- 1 root root 4096 Apr 20 21:59 in_accel_mag_falling_value
> > ...
> >
> > In the sources of sca3000.c I saw this setup with .mask_separate. So,
> > there I'd expect to see separate enables per axis, or am I wrong? In
> > the case of the ADXL345, there should only be one freefall enable (in
> > my driver) and not per axis. So, isn't this what is currently there?
> >
> > So far I only adjust the or'ing to and'ing the axis for freefall.
>
> So this is a messy corner of the ABI (because these are tricky to describe).
> Shared by type usually means there is one attribute applying to all the
> axis, but that they are reported separately, or potentially multiple events
> / _OR_ form used if we can distinguish exactly what the event is.
>
> In this case there is no way for userspace to anticipate that the event
> that might be generate is X_AND_Y_AND_Z. So for this
> the ABI solution we came up with is that virtual channel and separate.
>
> So you get something along the lines of
> in_accel_x&y&z_mag_falling_en
> in_accel_x&y&z_mag_falling_period
> etc
>
> The tricky remaining corner is this only makes sense if we always enable
> all axis (which is typical for a freefall detector). If we get a device
> that oddly has per axis free fall enables, then it would be hard and I
> might argue nonsense to enable them separately anyway. Not true
> here though I think.
>
> Note that we may well have drivers using the ABI slightly differently for
> freefall events which will be at least partly because I'd forgotten how
> we resolved all this complexity long ago (that sca3000 driver is ancient!)
> ABI like this is tricky to fix up, but we might want to consider some duplication
> in those drivers so we standardize on one form for freefall (even if we have some
> stray ABI from other possible solutions).
>
> What we should definitely do is pull together some documentation on multi channel
> event handling as the ABI docs are probably not enough.
>
As I (begin to) understand now, in case of the sca3000, the virtual
channel is literally an extra channel. That means, we're talking
probably about this here down below, right?
...
512 static const struct iio_chan_spec sca3000_channels[] = {
513 SCA3000_CHAN(0, IIO_MOD_X),
514 SCA3000_CHAN(1, IIO_MOD_Y),
515 SCA3000_CHAN(2, IIO_MOD_Z),
516 {
517 .type = IIO_ACCEL,
518 .modified = 1,
519 .channel2 = IIO_MOD_X_AND_Y_AND_Z,
520 .scan_index = -1, /* Fake channel */
521 .event_spec = &sca3000_freefall_event_spec,
522 .num_event_specs = 1,
523 },
524 };
...
<taken from sca3000.c>
What's now missing for freefall and the ADXL345 and v7 in particular?
- I need to provide a similar channel setup as in the sca3000 snippet
above, the virtual channel
- I need to AND the axis for this channel
- Now, with the virtual channel usage will be "separate" instead of
"shared", which will result in a single enable handle in sysfs
Is this correct?
Sry, I needed to re-read your answer several times. What confuses me
is still a bit the "virtual extra-channel". Probably, I need to build
up a bit more practical experience with the channels. Providing just a
single enable handle already looks good to me. I still don't grok
entirely where this can be / is problematic in such case, but no need
to explain it now in full detail. If the TODOs match at least with my
understanding I will try to implement the above points.
> Jonathan
>
On Mon, 21 Apr 2025 15:31:19 +0200
Lothar Rubusch <l.rubusch@gmail.com> wrote:
> On Mon, Apr 21, 2025 at 12:16 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Sun, 20 Apr 2025 23:26:47 +0200
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >
> > > Happy Easter!
> > >
> > > On Fri, Apr 18, 2025 at 8:23 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > > >
> > > > On Mon, 14 Apr 2025 18:42:39 +0000
> > > > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> > > >
> > > > > Add the freefall detection of the sensor together with a threshold and
> > > > > time parameter. A freefall event is detected if the measuring signal
> > > > > falls below the threshold.
> > > > >
> > > > > Introduce a freefall threshold stored in regmap cache, and a freefall
> > > > > time, having the scaled time value stored as a member variable in the
> > > > > state instance.
> > > > >
> > > > Reading this I wondered whether we had the event code consistent for
> > > > freefall detectors... Or indeed inactivity ones (which are kind of similarish)
> > > >
> > > > :( We don't it seems. The issue is that
> > > > freefall is actually that all channels are simultaneously under the the magnitude
> > > > threshold, not one of them. So it should I think be
> > > > X_AND_Y_AND_Z not X_OR_Y_OR_Z
> > > >
> > >
> > > I change to X_AND_Y_AND_Z.
> > >
> > > > This is as opposed to activity detectors which tend to be any axis shows
> > > > activity and X_OR_Y_OR_Z applies.
> > > >
> > > > Anyhow upshot is I think I lead you astray on this and we should make this
> > > > one IIO_MOD_X_AND_Y_AND_Z
> > > >
> > > > A few other things inline.
> > > >
> > > > Unfortunately we don't deal with these events that often so I forget
> > > > what we did before :(
> > > >
> > > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > > > ---
> > > > > drivers/iio/accel/adxl345_core.c | 125 +++++++++++++++++++++++++++++++
> > > > > 1 file changed, 125 insertions(+)
> > > > >
> > > > > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > > > > index c464c87033fb..ae02826e552b 100644
> > > > > --- a/drivers/iio/accel/adxl345_core.c
> > > > > +++ b/drivers/iio/accel/adxl345_core.c
> > > > > @@ -75,6 +75,7 @@ struct adxl345_state {
> > > > > u32 tap_duration_us;
> > > > > u32 tap_latent_us;
> > > > > u32 tap_window_us;
> > > > > + u32 ff_time_ms;
> > > > >
> > > > > __le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE + 1] __aligned(IIO_DMA_MINALIGN);
> > > > > };
> > > > > @@ -96,6 +97,14 @@ static struct iio_event_spec adxl345_events[] = {
> > > > > BIT(IIO_EV_INFO_RESET_TIMEOUT) |
> > > > > BIT(IIO_EV_INFO_TAP2_MIN_DELAY),
> > > > > },
> > > > > + {
> > > > > + /* free fall */
> > > > > + .type = IIO_EV_TYPE_MAG,
> > > > > + .dir = IIO_EV_DIR_FALLING,
> > > > > + .mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE) |
> > > > > + BIT(IIO_EV_INFO_VALUE) |
> > > > > + BIT(IIO_EV_INFO_PERIOD),
> > > > > + },
> > > > This is creating separate per axis enables, values and period. Does that make
> > > > sense? If not you need to spin a kind of virtual channel (with mod X_AND_Y_AND_Z)
> > > > and add the events to it.
> > > >
> > > > See how the sca3000 does it for example.
> > >
> > > Hum, I'm not sure if I understand you correctly. In my driver, I'm
> > > using .mask_shared_by_type, and I verified there appears only one
> > > enable, one value and one period handle.
> > > # ls -l /sys/bus/iio/devices/iio:device0/events/
> > > total 0
> > > ...
> > > -rw-r--r-- 1 root root 4096 Apr 20 21:59 in_accel_mag_falling_en
> > > -rw-r--r-- 1 root root 4096 Apr 20 21:59 in_accel_mag_falling_period
> > > -rw-r--r-- 1 root root 4096 Apr 20 21:59 in_accel_mag_falling_value
> > > ...
> > >
> > > In the sources of sca3000.c I saw this setup with .mask_separate. So,
> > > there I'd expect to see separate enables per axis, or am I wrong? In
> > > the case of the ADXL345, there should only be one freefall enable (in
> > > my driver) and not per axis. So, isn't this what is currently there?
> > >
> > > So far I only adjust the or'ing to and'ing the axis for freefall.
> >
> > So this is a messy corner of the ABI (because these are tricky to describe).
> > Shared by type usually means there is one attribute applying to all the
> > axis, but that they are reported separately, or potentially multiple events
> > / _OR_ form used if we can distinguish exactly what the event is.
> >
> > In this case there is no way for userspace to anticipate that the event
> > that might be generate is X_AND_Y_AND_Z. So for this
> > the ABI solution we came up with is that virtual channel and separate.
> >
> > So you get something along the lines of
> > in_accel_x&y&z_mag_falling_en
> > in_accel_x&y&z_mag_falling_period
> > etc
> >
> > The tricky remaining corner is this only makes sense if we always enable
> > all axis (which is typical for a freefall detector). If we get a device
> > that oddly has per axis free fall enables, then it would be hard and I
> > might argue nonsense to enable them separately anyway. Not true
> > here though I think.
> >
> > Note that we may well have drivers using the ABI slightly differently for
> > freefall events which will be at least partly because I'd forgotten how
> > we resolved all this complexity long ago (that sca3000 driver is ancient!)
> > ABI like this is tricky to fix up, but we might want to consider some duplication
> > in those drivers so we standardize on one form for freefall (even if we have some
> > stray ABI from other possible solutions).
> >
> > What we should definitely do is pull together some documentation on multi channel
> > event handling as the ABI docs are probably not enough.
> >
>
> As I (begin to) understand now, in case of the sca3000, the virtual
> channel is literally an extra channel. That means, we're talking
> probably about this here down below, right?
> ...
yes. That is what I was referring to.
> 512 static const struct iio_chan_spec sca3000_channels[] = {
> 513 SCA3000_CHAN(0, IIO_MOD_X),
> 514 SCA3000_CHAN(1, IIO_MOD_Y),
> 515 SCA3000_CHAN(2, IIO_MOD_Z),
> 516 {
> 517 .type = IIO_ACCEL,
> 518 .modified = 1,
> 519 .channel2 = IIO_MOD_X_AND_Y_AND_Z,
> 520 .scan_index = -1, /* Fake channel */
> 521 .event_spec = &sca3000_freefall_event_spec,
> 522 .num_event_specs = 1,
> 523 },
> 524 };
> ...
> <taken from sca3000.c>
>
> What's now missing for freefall and the ADXL345 and v7 in particular?
> - I need to provide a similar channel setup as in the sca3000 snippet
> above, the virtual channel
> - I need to AND the axis for this channel
> - Now, with the virtual channel usage will be "separate" instead of
> "shared", which will result in a single enable handle in sysfs
>
> Is this correct?
Yes. I think that's all correct.
>
> Sry, I needed to re-read your answer several times. What confuses me
> is still a bit the "virtual extra-channel".
I made up that term for it as there is no actual traditional channel
there. I should have given a pointer to the actual iio_chan_spec entry.
Sorry about that!
>Probably, I need to build
> up a bit more practical experience with the channels. Providing just a
> single enable handle already looks good to me. I still don't grok
> entirely where this can be / is problematic in such case, but no need
> to explain it now in full detail. If the TODOs match at least with my
> understanding I will try to implement the above points.
These events are just a bit weird when aligned with the simpler ones
we tend to get on ADCs etc. Giving userspace the best visibility we
can is always good. Here it is possible to get that, but in some other
cases we haven't yet come up with a good generic ABI :(
Jonathan
>
> > Jonathan
> >
© 2016 - 2025 Red Hat, Inc.