Add the inactivity feature of the sensor. When activity and inactivity
are enabled, a link bit will be set linking activity and inactivity
handling. Additionally, the auto-sleep mode will be enabled. Due to the
link bit the sensor is going to auto-sleep when inactivity was
detected.
Inactivity detection needs a threshold to be configured, and a time
after which it will go into inactivity state if measurements under
threshold.
When a ODR is configured this time for inactivity is adjusted with a
corresponding reasonable default value, in order to have higher
frequencies and lower inactivity times, and lower sample frequency but
give more time until inactivity. Both with reasonable upper and lower
boundaries, since many of the sensor's features (e.g. auto-sleep) will
need to operate beween 12.5 Hz and 400 Hz. This is a default setting
when actively changing sample frequency, explicitly setting the time
until inactivity will overwrite the default.
Similarly, setting the g-range will provide a default value for the
activity and inactivity thresholds. Both are implicit defaults, but
equally can be overwritten to be explicitly configured.
Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
drivers/iio/accel/adxl345_core.c | 173 ++++++++++++++++++++++++++++++-
1 file changed, 168 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 8f7ea3928cf8..06028eaef9e0 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -37,11 +37,17 @@
#define ADXL345_REG_TAP_SUPPRESS_MSK BIT(3)
#define ADXL345_REG_TAP_SUPPRESS BIT(3)
#define ADXL345_REG_ACT_AXIS_MSK GENMASK(6, 4)
+#define ADXL345_REG_INACT_AXIS_MSK GENMASK(2, 0)
+#define ADXL345_POWER_CTL_INACT_MSK (ADXL345_POWER_CTL_AUTO_SLEEP | ADXL345_POWER_CTL_LINK)
#define ADXL345_TAP_Z_EN BIT(0)
#define ADXL345_TAP_Y_EN BIT(1)
#define ADXL345_TAP_X_EN BIT(2)
+#define ADXL345_INACT_Z_EN BIT(0)
+#define ADXL345_INACT_Y_EN BIT(1)
+#define ADXL345_INACT_X_EN BIT(2)
+
#define ADXL345_ACT_Z_EN BIT(4)
#define ADXL345_ACT_Y_EN BIT(5)
#define ADXL345_ACT_X_EN BIT(6)
@@ -72,14 +78,17 @@ static const unsigned int adxl345_tap_time_reg[] = {
/* activity/inactivity */
enum adxl345_activity_type {
ADXL345_ACTIVITY,
+ ADXL345_INACTIVITY,
};
static const unsigned int adxl345_act_int_reg[] = {
[ADXL345_ACTIVITY] = ADXL345_INT_ACTIVITY,
+ [ADXL345_INACTIVITY] = ADXL345_INT_INACTIVITY,
};
static const unsigned int adxl345_act_thresh_reg[] = {
[ADXL345_ACTIVITY] = ADXL345_REG_THRESH_ACT,
+ [ADXL345_INACTIVITY] = ADXL345_REG_THRESH_INACT,
};
enum adxl345_odr {
@@ -179,6 +188,14 @@ static struct iio_event_spec adxl345_events[] = {
.mask_separate = BIT(IIO_EV_INFO_ENABLE),
.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE),
},
+ {
+ /* inactivity */
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_FALLING,
+ .mask_separate = BIT(IIO_EV_INFO_ENABLE),
+ .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
+ BIT(IIO_EV_INFO_PERIOD),
+ },
{
/* single tap */
.type = IIO_EV_TYPE_GESTURE,
@@ -278,7 +295,8 @@ static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
{
unsigned int val = en ? ADXL345_POWER_CTL_MEASURE : ADXL345_POWER_CTL_STANDBY;
- return regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
+ return regmap_update_bits(st->regmap, ADXL345_REG_POWER_CTL,
+ ADXL345_POWER_CTL_MEASURE, val);
}
/* act/inact */
@@ -310,6 +328,21 @@ static int adxl345_is_act_inact_en(struct adxl345_state *st,
*en = false;
return -EINVAL;
}
+ } else {
+ switch (axis) {
+ case IIO_MOD_X:
+ *en = FIELD_GET(ADXL345_INACT_X_EN, axis_ctrl);
+ break;
+ case IIO_MOD_Y:
+ *en = FIELD_GET(ADXL345_INACT_Y_EN, axis_ctrl);
+ break;
+ case IIO_MOD_Z:
+ *en = FIELD_GET(ADXL345_INACT_Z_EN, axis_ctrl);
+ break;
+ default:
+ *en = false;
+ return -EINVAL;
+ }
}
if (*en) {
@@ -329,6 +362,7 @@ static int adxl345_set_act_inact_en(struct adxl345_state *st,
bool cmd_en)
{
bool en;
+ unsigned int inact_time_s;
unsigned int threshold;
u32 axis_ctrl = 0;
int ret;
@@ -347,6 +381,20 @@ static int adxl345_set_act_inact_en(struct adxl345_state *st,
default:
return -EINVAL;
}
+ } else {
+ switch (axis) {
+ case IIO_MOD_X:
+ axis_ctrl = ADXL345_INACT_X_EN;
+ break;
+ case IIO_MOD_Y:
+ axis_ctrl = ADXL345_INACT_Y_EN;
+ break;
+ case IIO_MOD_Z:
+ axis_ctrl = ADXL345_INACT_Z_EN;
+ break;
+ default:
+ return -EINVAL;
+ }
}
if (cmd_en)
@@ -367,11 +415,69 @@ static int adxl345_set_act_inact_en(struct adxl345_state *st,
if (type == ADXL345_ACTIVITY) {
en = FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, axis_ctrl) &&
threshold;
+ } else {
+ ret = regmap_read(st->regmap, ADXL345_REG_TIME_INACT, &inact_time_s);
+ if (ret)
+ return ret;
+
+ en = FIELD_GET(ADXL345_REG_INACT_AXIS_MSK, axis_ctrl) &&
+ threshold && inact_time_s;
}
- return regmap_update_bits(st->regmap, ADXL345_REG_INT_ENABLE,
- adxl345_act_int_reg[type],
- en ? adxl345_act_int_reg[type] : 0);
+ ret = regmap_update_bits(st->regmap, ADXL345_REG_INT_ENABLE,
+ adxl345_act_int_reg[type],
+ en ? adxl345_act_int_reg[type] : 0);
+ if (ret)
+ return ret;
+
+ return regmap_update_bits(st->regmap, ADXL345_REG_POWER_CTL,
+ ADXL345_POWER_CTL_INACT_MSK,
+ en ? (ADXL345_POWER_CTL_AUTO_SLEEP | ADXL345_POWER_CTL_LINK)
+ : 0);
+}
+
+/**
+ * adxl345_set_inact_time_s - Configure inactivity time explicitly or by ODR.
+ * @st: The sensor state instance.
+ * @val_s: A desired time value, between 0 and 255.
+ *
+ * Inactivity time can be configured between 1 and 255 sec. If a val_s of 0
+ * is configured by a user, then a default inactivity time will be computed.
+ *
+ * In such case, it should take power consumption into consideration. Thus it
+ * shall be shorter for higher frequencies and longer for lower frequencies.
+ * Hence, frequencies above 255 Hz shall default to 10 s and frequencies below
+ * 10 Hz shall result in 255 s to detect inactivity.
+ *
+ * The approach simply subtracts the pre-decimal figure of the configured
+ * sample frequency from 255 s to compute inactivity time [s]. Sub-Hz are thus
+ * ignored in this estimation. The recommended ODRs for various features
+ * (activity/inactivity, sleep modes, free fall, etc.) lie between 12.5 Hz and
+ * 400 Hz, thus higher or lower frequencies will result in the boundary
+ * defaults or need to be explicitly specified via val_s.
+ *
+ * Return: 0 or error value.
+ */
+static int adxl345_set_inact_time_s(struct adxl345_state *st, u32 val_s)
+{
+ unsigned int max_boundary = 255;
+ unsigned int min_boundary = 10;
+ unsigned int val = min(val_s, max_boundary);
+ enum adxl345_odr odr;
+ unsigned int regval;
+ int ret;
+
+ if (val == 0) {
+ ret = regmap_read(st->regmap, ADXL345_REG_BW_RATE, ®val);
+ if (ret)
+ return ret;
+ odr = FIELD_GET(ADXL345_BW_RATE_MSK, regval);
+
+ val = (adxl345_odr_tbl[odr][0] > max_boundary)
+ ? min_boundary : max_boundary - adxl345_odr_tbl[odr][0];
+ }
+
+ return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
}
/* tap */
@@ -837,6 +943,13 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
if (ret)
return ret;
return int_en;
+ case IIO_EV_DIR_FALLING:
+ ret = adxl345_is_act_inact_en(st, chan->channel2,
+ ADXL345_INACTIVITY,
+ &int_en);
+ if (ret)
+ return ret;
+ return int_en;
default:
return -EINVAL;
}
@@ -881,6 +994,9 @@ static int adxl345_write_event_config(struct iio_dev *indio_dev,
case IIO_EV_DIR_RISING:
return adxl345_set_act_inact_en(st, chan->channel2,
ADXL345_ACTIVITY, state);
+ case IIO_EV_DIR_FALLING:
+ return adxl345_set_act_inact_en(st, chan->channel2,
+ ADXL345_INACTIVITY, state);
default:
return -EINVAL;
}
@@ -908,7 +1024,8 @@ static int adxl345_read_event_value(struct iio_dev *indio_dev,
int *val, int *val2)
{
struct adxl345_state *st = iio_priv(indio_dev);
- unsigned int act_threshold;
+ unsigned int act_threshold, inact_threshold;
+ unsigned int inact_time_s;
unsigned int tap_threshold;
unsigned int ff_threshold;
int ret;
@@ -927,9 +1044,24 @@ static int adxl345_read_event_value(struct iio_dev *indio_dev,
*val = act_threshold;
return IIO_VAL_INT;
+ case IIO_EV_DIR_FALLING:
+ ret = regmap_read(st->regmap,
+ adxl345_act_thresh_reg[ADXL345_INACTIVITY],
+ &inact_threshold);
+ if (ret)
+ return ret;
+
+ *val = inact_threshold;
+ return IIO_VAL_INT;
default:
return -EINVAL;
}
+ case IIO_EV_INFO_PERIOD:
+ ret = regmap_read(st->regmap, ADXL345_REG_TIME_INACT, &inact_time_s);
+ if (ret)
+ return ret;
+ *val = inact_time_s;
+ return IIO_VAL_INT;
default:
return -EINVAL;
}
@@ -1010,10 +1142,22 @@ static int adxl345_write_event_value(struct iio_dev *indio_dev,
if (ret)
return ret;
break;
+ case IIO_EV_DIR_FALLING:
+ ret = regmap_write(st->regmap,
+ adxl345_act_thresh_reg[ADXL345_INACTIVITY],
+ val);
+ if (ret)
+ return ret;
+ break;
default:
return -EINVAL;
}
break;
+ case IIO_EV_INFO_PERIOD:
+ ret = adxl345_set_inact_time_s(st, val);
+ if (ret)
+ return ret;
+ break;
default:
return -EINVAL;
}
@@ -1314,6 +1458,17 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
return ret;
}
+ if (FIELD_GET(ADXL345_INT_INACTIVITY, 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_THRESH,
+ IIO_EV_DIR_FALLING),
+ ts);
+ if (ret)
+ return ret;
+ }
+
if (FIELD_GET(ADXL345_INT_FREE_FALL, int_stat)) {
ret = iio_push_event(indio_dev,
IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
@@ -1560,10 +1715,18 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
if (ret)
return ret;
+ ret = regmap_write(st->regmap, ADXL345_REG_TIME_INACT, 3);
+ if (ret)
+ return ret;
+
ret = regmap_write(st->regmap, ADXL345_REG_THRESH_ACT, 6);
if (ret)
return ret;
+ ret = regmap_write(st->regmap, ADXL345_REG_THRESH_INACT, 4);
+ if (ret)
+ return ret;
+
ret = regmap_write(st->regmap, ADXL345_REG_THRESH_TAP, tap_threshold);
if (ret)
return ret;
--
2.39.5On Mon, 14 Apr 2025 18:42:43 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:
> Add the inactivity feature of the sensor. When activity and inactivity
> are enabled, a link bit will be set linking activity and inactivity
> handling. Additionally, the auto-sleep mode will be enabled. Due to the
> link bit the sensor is going to auto-sleep when inactivity was
> detected.
>
> Inactivity detection needs a threshold to be configured, and a time
> after which it will go into inactivity state if measurements under
> threshold.
>
> When a ODR is configured this time for inactivity is adjusted with a
> corresponding reasonable default value, in order to have higher
> frequencies and lower inactivity times, and lower sample frequency but
> give more time until inactivity. Both with reasonable upper and lower
> boundaries, since many of the sensor's features (e.g. auto-sleep) will
> need to operate beween 12.5 Hz and 400 Hz. This is a default setting
> when actively changing sample frequency, explicitly setting the time
> until inactivity will overwrite the default.
>
> Similarly, setting the g-range will provide a default value for the
> activity and inactivity thresholds. Both are implicit defaults, but
> equally can be overwritten to be explicitly configured.
>
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
Hi Lothar,
Patches 6-8 look good to me.
This runs into a similar issue to the freefall one. I haven't dug into
the datasheet but does it report on one channel going inactive, or
all being inactive at the same time? I checked and it is the all
case so we should be both on a pseudo channel to describe it right
and reporting IIO_MOD_X_AND_Y_AND_Z not the OR form.
Sorry again that I'm only realising this on v6 :(
Difference is for Activity the definition is:
"The activity bit is set when acceleration greater than the value
stored in the THRESH_ACT register (Address 0x24) is experienced
on _any_ participating axis, set by the ACT_INACT_CTL register
(Address 0x27)."
vs Inactivity:
"The inactivity bit is set when acceleration of less than the value
stored in the THRESH_INACT register (Address 0x25) is experienced
for more time than is specified in the TIME_INACT
register (Address 0x26) on _all_ participating axes, as set by the
ACT_INACT_CTL register (Address 0x27). "
So all vs any.
> +
> +/**
> + * adxl345_set_inact_time_s - Configure inactivity time explicitly or by ODR.
> + * @st: The sensor state instance.
> + * @val_s: A desired time value, between 0 and 255.
> + *
> + * Inactivity time can be configured between 1 and 255 sec. If a val_s of 0
> + * is configured by a user, then a default inactivity time will be computed.
> + *
> + * In such case, it should take power consumption into consideration. Thus it
> + * shall be shorter for higher frequencies and longer for lower frequencies.
> + * Hence, frequencies above 255 Hz shall default to 10 s and frequencies below
> + * 10 Hz shall result in 255 s to detect inactivity.
> + *
> + * The approach simply subtracts the pre-decimal figure of the configured
> + * sample frequency from 255 s to compute inactivity time [s]. Sub-Hz are thus
> + * ignored in this estimation. The recommended ODRs for various features
> + * (activity/inactivity, sleep modes, free fall, etc.) lie between 12.5 Hz and
> + * 400 Hz, thus higher or lower frequencies will result in the boundary
> + * defaults or need to be explicitly specified via val_s.
> + *
> + * Return: 0 or error value.
> + */
> +static int adxl345_set_inact_time_s(struct adxl345_state *st, u32 val_s)
> +{
> + unsigned int max_boundary = 255;
> + unsigned int min_boundary = 10;
> + unsigned int val = min(val_s, max_boundary);
> + enum adxl345_odr odr;
> + unsigned int regval;
> + int ret;
> +
> + if (val == 0) {
> + ret = regmap_read(st->regmap, ADXL345_REG_BW_RATE, ®val);
> + if (ret)
> + return ret;
> + odr = FIELD_GET(ADXL345_BW_RATE_MSK, regval);
> +
> + val = (adxl345_odr_tbl[odr][0] > max_boundary)
> + ? min_boundary : max_boundary - adxl345_odr_tbl[odr][0];
> + }
> +
> + return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
> }
>
> /* tap */
> @@ -837,6 +943,13 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
> if (ret)
> return ret;
> return int_en;
> + case IIO_EV_DIR_FALLING:
> + ret = adxl345_is_act_inact_en(st, chan->channel2,
Does it makes sense to allow inactivity detection on a subset of channels but then
report it as XYZ? I guess it didn't matter when it was and OR, but if we
change to AND as suggested that is going to be misleading.
we might have to allow separate enables but report an event as the combination
of channels that are enabled X_AND_Y, X_AND_Z etc I guess we can improve activity
channel case as well by doing that with the X_OR_Y etc
> + ADXL345_INACTIVITY,
> + &int_en);
> + if (ret)
> + return ret;
> + return int_en;
> default:
> return -EINVAL;
> }
> @@ -881,6 +994,9 @@ static int adxl345_write_event_config(struct iio_dev *indio_dev,
> case IIO_EV_DIR_RISING:
> return adxl345_set_act_inact_en(st, chan->channel2,
> ADXL345_ACTIVITY, state);
> + case IIO_EV_DIR_FALLING:
> + return adxl345_set_act_inact_en(st, chan->channel2,
> + ADXL345_INACTIVITY, state);
> default:
> return -EINVAL;
> }
> @@ -1314,6 +1458,17 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
> return ret;
> }
>
> + if (FIELD_GET(ADXL345_INT_INACTIVITY, int_stat)) {
> + ret = iio_push_event(indio_dev,
> + IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> + IIO_MOD_X_OR_Y_OR_Z,
So this is our open question. Similar to the free fall case. Do we have the boolean
logic right way around?
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_FALLING),
> + ts);
> + if (ret)
> + return ret;
> + }
> +
Happy Easter (again)!
On Fri, Apr 18, 2025 at 8:34 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon, 14 Apr 2025 18:42:43 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Add the inactivity feature of the sensor. When activity and inactivity
> > are enabled, a link bit will be set linking activity and inactivity
> > handling. Additionally, the auto-sleep mode will be enabled. Due to the
> > link bit the sensor is going to auto-sleep when inactivity was
> > detected.
> >
> > Inactivity detection needs a threshold to be configured, and a time
> > after which it will go into inactivity state if measurements under
> > threshold.
> >
> > When a ODR is configured this time for inactivity is adjusted with a
> > corresponding reasonable default value, in order to have higher
> > frequencies and lower inactivity times, and lower sample frequency but
> > give more time until inactivity. Both with reasonable upper and lower
> > boundaries, since many of the sensor's features (e.g. auto-sleep) will
> > need to operate beween 12.5 Hz and 400 Hz. This is a default setting
> > when actively changing sample frequency, explicitly setting the time
> > until inactivity will overwrite the default.
> >
> > Similarly, setting the g-range will provide a default value for the
> > activity and inactivity thresholds. Both are implicit defaults, but
> > equally can be overwritten to be explicitly configured.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> Hi Lothar,
>
> Patches 6-8 look good to me.
>
> This runs into a similar issue to the freefall one. I haven't dug into
> the datasheet but does it report on one channel going inactive, or
> all being inactive at the same time? I checked and it is the all
> case so we should be both on a pseudo channel to describe it right
> and reporting IIO_MOD_X_AND_Y_AND_Z not the OR form.
>
> Sorry again that I'm only realising this on v6 :(
No problem at all! Sure, I'm still in this phase where counting every
single commit upstream makes my ego greater. On the long run, though,
I guess it's better to build up knowledge and end up with a decent
implementation quality, than just increasing a commit counter. For me
it's fine. I also hope it's not too annoying for you.
>
> Difference is for Activity the definition is:
> "The activity bit is set when acceleration greater than the value
> stored in the THRESH_ACT register (Address 0x24) is experienced
> on _any_ participating axis, set by the ACT_INACT_CTL register
> (Address 0x27)."
> vs Inactivity:
> "The inactivity bit is set when acceleration of less than the value
> stored in the THRESH_INACT register (Address 0x25) is experienced
> for more time than is specified in the TIME_INACT
> register (Address 0x26) on _all_ participating axes, as set by the
> ACT_INACT_CTL register (Address 0x27). "
>
> So all vs any.
>
I think I see your point. At least I change here for inactivity, too,
to AND'ed axis.
IMHO, if I set OR here, the first axis raising the inactivity will put
the sensor to sleep mode,
where AND needs all three axis in inactivity state. I'm not sure if
this works out, I need to verify
it still with the hardware, for now I'll change this to AND.
> > +
> > +/**
> > + * adxl345_set_inact_time_s - Configure inactivity time explicitly or by ODR.
> > + * @st: The sensor state instance.
> > + * @val_s: A desired time value, between 0 and 255.
> > + *
> > + * Inactivity time can be configured between 1 and 255 sec. If a val_s of 0
> > + * is configured by a user, then a default inactivity time will be computed.
> > + *
> > + * In such case, it should take power consumption into consideration. Thus it
> > + * shall be shorter for higher frequencies and longer for lower frequencies.
> > + * Hence, frequencies above 255 Hz shall default to 10 s and frequencies below
> > + * 10 Hz shall result in 255 s to detect inactivity.
> > + *
> > + * The approach simply subtracts the pre-decimal figure of the configured
> > + * sample frequency from 255 s to compute inactivity time [s]. Sub-Hz are thus
> > + * ignored in this estimation. The recommended ODRs for various features
> > + * (activity/inactivity, sleep modes, free fall, etc.) lie between 12.5 Hz and
> > + * 400 Hz, thus higher or lower frequencies will result in the boundary
> > + * defaults or need to be explicitly specified via val_s.
> > + *
> > + * Return: 0 or error value.
> > + */
> > +static int adxl345_set_inact_time_s(struct adxl345_state *st, u32 val_s)
> > +{
> > + unsigned int max_boundary = 255;
> > + unsigned int min_boundary = 10;
> > + unsigned int val = min(val_s, max_boundary);
> > + enum adxl345_odr odr;
> > + unsigned int regval;
> > + int ret;
> > +
> > + if (val == 0) {
> > + ret = regmap_read(st->regmap, ADXL345_REG_BW_RATE, ®val);
> > + if (ret)
> > + return ret;
> > + odr = FIELD_GET(ADXL345_BW_RATE_MSK, regval);
> > +
> > + val = (adxl345_odr_tbl[odr][0] > max_boundary)
> > + ? min_boundary : max_boundary - adxl345_odr_tbl[odr][0];
> > + }
> > +
> > + return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
> > }
> >
> > /* tap */
> > @@ -837,6 +943,13 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
> > if (ret)
> > return ret;
> > return int_en;
> > + case IIO_EV_DIR_FALLING:
> > + ret = adxl345_is_act_inact_en(st, chan->channel2,
>
> Does it makes sense to allow inactivity detection on a subset of channels but then
> report it as XYZ? I guess it didn't matter when it was and OR, but if we
> change to AND as suggested that is going to be misleading.
>
> we might have to allow separate enables but report an event as the combination
> of channels that are enabled X_AND_Y, X_AND_Z etc I guess we can improve activity
> channel case as well by doing that with the X_OR_Y etc
>
Well, initially I guess I only had one enable for inactivity.
This was kind of confusing to me. There is a register to enable
activity and inactivity on a per axis base [ACT_INACT_CTL, 0x27].
The interrupt event will set a single bit for inactivity or activity
[INT_SOURCE, 0x30]. In the interrupt handler further one can read out
the [ACT_TAP_STATUS, 0x2B], which contains tap and activity
directions, but no information about inactivity axis.
In summary, for the ADXL345 inactivity can be configured on a per axis
base, but the event won't tell about the axis that fell into
inactivity, i.e. the first inactivity is supposed to put the sensor
into power save (with link bit and power modes set - I think
inactivity should mainly be seen in the context of their/Analog's
power save concept). As said before, initially I only provided a
single "inactivity enable". Then I saw actually I could set and offer
this per axis. I don't know if there are use cases only to observe
particularly the x-axis for a general power save. Probably rather not.
So, I agree. But if you don't tell me explicitely to replace per axis
enables by a single one, I'll probably leave it as is. It implements
most transparently what the sensor can offer for configuration.
>
>
> > + ADXL345_INACTIVITY,
> > + &int_en);
> > + if (ret)
> > + return ret;
> > + return int_en;
> > default:
> > return -EINVAL;
> > }
> > @@ -881,6 +994,9 @@ static int adxl345_write_event_config(struct iio_dev *indio_dev,
> > case IIO_EV_DIR_RISING:
> > return adxl345_set_act_inact_en(st, chan->channel2,
> > ADXL345_ACTIVITY, state);
> > + case IIO_EV_DIR_FALLING:
> > + return adxl345_set_act_inact_en(st, chan->channel2,
> > + ADXL345_INACTIVITY, state);
> > default:
> > return -EINVAL;
> > }
>
> > @@ -1314,6 +1458,17 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
> > return ret;
> > }
> >
> > + if (FIELD_GET(ADXL345_INT_INACTIVITY, int_stat)) {
> > + ret = iio_push_event(indio_dev,
> > + IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> > + IIO_MOD_X_OR_Y_OR_Z,
>
> So this is our open question. Similar to the free fall case. Do we have the boolean
> logic right way around?
>
> > + IIO_EV_TYPE_THRESH,
> > + IIO_EV_DIR_FALLING),
> > + ts);
> > + if (ret)
> > + return ret;
> > + }
> > +
On Mon, 21 Apr 2025 00:12:17 +0200
Lothar Rubusch <l.rubusch@gmail.com> wrote:
> Happy Easter (again)!
>
> On Fri, Apr 18, 2025 at 8:34 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Mon, 14 Apr 2025 18:42:43 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >
> > > Add the inactivity feature of the sensor. When activity and inactivity
> > > are enabled, a link bit will be set linking activity and inactivity
> > > handling. Additionally, the auto-sleep mode will be enabled. Due to the
> > > link bit the sensor is going to auto-sleep when inactivity was
> > > detected.
> > >
> > > Inactivity detection needs a threshold to be configured, and a time
> > > after which it will go into inactivity state if measurements under
> > > threshold.
> > >
> > > When a ODR is configured this time for inactivity is adjusted with a
> > > corresponding reasonable default value, in order to have higher
> > > frequencies and lower inactivity times, and lower sample frequency but
> > > give more time until inactivity. Both with reasonable upper and lower
> > > boundaries, since many of the sensor's features (e.g. auto-sleep) will
> > > need to operate beween 12.5 Hz and 400 Hz. This is a default setting
> > > when actively changing sample frequency, explicitly setting the time
> > > until inactivity will overwrite the default.
> > >
> > > Similarly, setting the g-range will provide a default value for the
> > > activity and inactivity thresholds. Both are implicit defaults, but
> > > equally can be overwritten to be explicitly configured.
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > Hi Lothar,
> >
> > Patches 6-8 look good to me.
> >
> > This runs into a similar issue to the freefall one. I haven't dug into
> > the datasheet but does it report on one channel going inactive, or
> > all being inactive at the same time? I checked and it is the all
> > case so we should be both on a pseudo channel to describe it right
> > and reporting IIO_MOD_X_AND_Y_AND_Z not the OR form.
> >
> > Sorry again that I'm only realising this on v6 :(
>
> No problem at all! Sure, I'm still in this phase where counting every
> single commit upstream makes my ego greater. On the long run, though,
> I guess it's better to build up knowledge and end up with a decent
> implementation quality, than just increasing a commit counter. For me
> it's fine. I also hope it's not too annoying for you.
>
> >
> > Difference is for Activity the definition is:
> > "The activity bit is set when acceleration greater than the value
> > stored in the THRESH_ACT register (Address 0x24) is experienced
> > on _any_ participating axis, set by the ACT_INACT_CTL register
> > (Address 0x27)."
> > vs Inactivity:
> > "The inactivity bit is set when acceleration of less than the value
> > stored in the THRESH_INACT register (Address 0x25) is experienced
> > for more time than is specified in the TIME_INACT
> > register (Address 0x26) on _all_ participating axes, as set by the
> > ACT_INACT_CTL register (Address 0x27). "
> >
> > So all vs any.
> >
>
> I think I see your point. At least I change here for inactivity, too,
> to AND'ed axis.
>
> IMHO, if I set OR here, the first axis raising the inactivity will put
> the sensor to sleep mode,
> where AND needs all three axis in inactivity state. I'm not sure if
> this works out, I need to verify
> it still with the hardware, for now I'll change this to AND.
I'd be surprised if it worked differently but indeed good to check!
>
> > > +
> > > +/**
> > > + * adxl345_set_inact_time_s - Configure inactivity time explicitly or by ODR.
> > > + * @st: The sensor state instance.
> > > + * @val_s: A desired time value, between 0 and 255.
> > > + *
> > > + * Inactivity time can be configured between 1 and 255 sec. If a val_s of 0
> > > + * is configured by a user, then a default inactivity time will be computed.
> > > + *
> > > + * In such case, it should take power consumption into consideration. Thus it
> > > + * shall be shorter for higher frequencies and longer for lower frequencies.
> > > + * Hence, frequencies above 255 Hz shall default to 10 s and frequencies below
> > > + * 10 Hz shall result in 255 s to detect inactivity.
> > > + *
> > > + * The approach simply subtracts the pre-decimal figure of the configured
> > > + * sample frequency from 255 s to compute inactivity time [s]. Sub-Hz are thus
> > > + * ignored in this estimation. The recommended ODRs for various features
> > > + * (activity/inactivity, sleep modes, free fall, etc.) lie between 12.5 Hz and
> > > + * 400 Hz, thus higher or lower frequencies will result in the boundary
> > > + * defaults or need to be explicitly specified via val_s.
> > > + *
> > > + * Return: 0 or error value.
> > > + */
> > > +static int adxl345_set_inact_time_s(struct adxl345_state *st, u32 val_s)
> > > +{
> > > + unsigned int max_boundary = 255;
> > > + unsigned int min_boundary = 10;
> > > + unsigned int val = min(val_s, max_boundary);
> > > + enum adxl345_odr odr;
> > > + unsigned int regval;
> > > + int ret;
> > > +
> > > + if (val == 0) {
> > > + ret = regmap_read(st->regmap, ADXL345_REG_BW_RATE, ®val);
> > > + if (ret)
> > > + return ret;
> > > + odr = FIELD_GET(ADXL345_BW_RATE_MSK, regval);
> > > +
> > > + val = (adxl345_odr_tbl[odr][0] > max_boundary)
> > > + ? min_boundary : max_boundary - adxl345_odr_tbl[odr][0];
> > > + }
> > > +
> > > + return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
> > > }
> > >
> > > /* tap */
> > > @@ -837,6 +943,13 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
> > > if (ret)
> > > return ret;
> > > return int_en;
> > > + case IIO_EV_DIR_FALLING:
> > > + ret = adxl345_is_act_inact_en(st, chan->channel2,
> >
> > Does it makes sense to allow inactivity detection on a subset of channels but then
> > report it as XYZ? I guess it didn't matter when it was and OR, but if we
> > change to AND as suggested that is going to be misleading.
> >
> > we might have to allow separate enables but report an event as the combination
> > of channels that are enabled X_AND_Y, X_AND_Z etc I guess we can improve activity
> > channel case as well by doing that with the X_OR_Y etc
> >
>
> Well, initially I guess I only had one enable for inactivity.
>
> This was kind of confusing to me. There is a register to enable
> activity and inactivity on a per axis base [ACT_INACT_CTL, 0x27].
Agreed this is a slightly odd concept.
>
> The interrupt event will set a single bit for inactivity or activity
> [INT_SOURCE, 0x30]. In the interrupt handler further one can read out
> the [ACT_TAP_STATUS, 0x2B], which contains tap and activity
> directions, but no information about inactivity axis.
>
> In summary, for the ADXL345 inactivity can be configured on a per axis
> base, but the event won't tell about the axis that fell into
> inactivity, i.e. the first inactivity is supposed to put the sensor
> into power save (with link bit and power modes set - I think
> inactivity should mainly be seen in the context of their/Analog's
> power save concept). As said before, initially I only provided a
> single "inactivity enable". Then I saw actually I could set and offer
> this per axis. I don't know if there are use cases only to observe
> particularly the x-axis for a general power save. Probably rather not.
>
> So, I agree. But if you don't tell me explicitely to replace per axis
> enables by a single one, I'll probably leave it as is. It implements
> most transparently what the sensor can offer for configuration.
The snag is what I mentioned for freefall. It becomes very hard to indicate
to userspace what it might expect for the x&y&z cases. If inactivity requires
them all to be inactive, I think separate enables is going to be really
tricky to build a consistent ABI around :(
Some devices we've had in the past have allowed specific configuration of
and / or for axis combinations. For those we've normally kept clear because
the number of combinations gets sill quickly.
If we don't have a separate channel enable usecase today I think we should
go ahead with general inactivity / activity (and/or as appropriate) and
perhaps solve the per axis case if anyone ever cares about it.
>
> >
> >
> > > + ADXL345_INACTIVITY,
> > > + &int_en);
> > > + if (ret)
> > > + return ret;
> > > + return int_en;
> > > default:
> > > return -EINVAL;
> > > }
> > > @@ -881,6 +994,9 @@ static int adxl345_write_event_config(struct iio_dev *indio_dev,
> > > case IIO_EV_DIR_RISING:
> > > return adxl345_set_act_inact_en(st, chan->channel2,
> > > ADXL345_ACTIVITY, state);
> > > + case IIO_EV_DIR_FALLING:
> > > + return adxl345_set_act_inact_en(st, chan->channel2,
> > > + ADXL345_INACTIVITY, state);
> > > default:
> > > return -EINVAL;
> > > }
> >
> > > @@ -1314,6 +1458,17 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
> > > return ret;
> > > }
> > >
> > > + if (FIELD_GET(ADXL345_INT_INACTIVITY, int_stat)) {
> > > + ret = iio_push_event(indio_dev,
> > > + IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> > > + IIO_MOD_X_OR_Y_OR_Z,
> >
> > So this is our open question. Similar to the free fall case. Do we have the boolean
> > logic right way around?
> >
> > > + IIO_EV_TYPE_THRESH,
> > > + IIO_EV_DIR_FALLING),
> > > + ts);
> > > + if (ret)
> > > + return ret;
> > > + }
> > > +
On Mon, Apr 21, 2025 at 12:22 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon, 21 Apr 2025 00:12:17 +0200
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Happy Easter (again)!
> >
> > On Fri, Apr 18, 2025 at 8:34 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > On Mon, 14 Apr 2025 18:42:43 +0000
> > > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> > >
> > > > Add the inactivity feature of the sensor. When activity and inactivity
> > > > are enabled, a link bit will be set linking activity and inactivity
> > > > handling. Additionally, the auto-sleep mode will be enabled. Due to the
> > > > link bit the sensor is going to auto-sleep when inactivity was
> > > > detected.
> > > >
> > > > Inactivity detection needs a threshold to be configured, and a time
> > > > after which it will go into inactivity state if measurements under
> > > > threshold.
> > > >
> > > > When a ODR is configured this time for inactivity is adjusted with a
> > > > corresponding reasonable default value, in order to have higher
> > > > frequencies and lower inactivity times, and lower sample frequency but
> > > > give more time until inactivity. Both with reasonable upper and lower
> > > > boundaries, since many of the sensor's features (e.g. auto-sleep) will
> > > > need to operate beween 12.5 Hz and 400 Hz. This is a default setting
> > > > when actively changing sample frequency, explicitly setting the time
> > > > until inactivity will overwrite the default.
> > > >
> > > > Similarly, setting the g-range will provide a default value for the
> > > > activity and inactivity thresholds. Both are implicit defaults, but
> > > > equally can be overwritten to be explicitly configured.
> > > >
> > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > Hi Lothar,
> > >
> > > Patches 6-8 look good to me.
> > >
> > > This runs into a similar issue to the freefall one. I haven't dug into
> > > the datasheet but does it report on one channel going inactive, or
> > > all being inactive at the same time? I checked and it is the all
> > > case so we should be both on a pseudo channel to describe it right
> > > and reporting IIO_MOD_X_AND_Y_AND_Z not the OR form.
> > >
> > > Sorry again that I'm only realising this on v6 :(
> >
> > No problem at all! Sure, I'm still in this phase where counting every
> > single commit upstream makes my ego greater. On the long run, though,
> > I guess it's better to build up knowledge and end up with a decent
> > implementation quality, than just increasing a commit counter. For me
> > it's fine. I also hope it's not too annoying for you.
> >
> > >
> > > Difference is for Activity the definition is:
> > > "The activity bit is set when acceleration greater than the value
> > > stored in the THRESH_ACT register (Address 0x24) is experienced
> > > on _any_ participating axis, set by the ACT_INACT_CTL register
> > > (Address 0x27)."
> > > vs Inactivity:
> > > "The inactivity bit is set when acceleration of less than the value
> > > stored in the THRESH_INACT register (Address 0x25) is experienced
> > > for more time than is specified in the TIME_INACT
> > > register (Address 0x26) on _all_ participating axes, as set by the
> > > ACT_INACT_CTL register (Address 0x27). "
> > >
> > > So all vs any.
> > >
> >
> > I think I see your point. At least I change here for inactivity, too,
> > to AND'ed axis.
> >
> > IMHO, if I set OR here, the first axis raising the inactivity will put
> > the sensor to sleep mode,
> > where AND needs all three axis in inactivity state. I'm not sure if
> > this works out, I need to verify
> > it still with the hardware, for now I'll change this to AND.
>
> I'd be surprised if it worked differently but indeed good to check!
>
> >
> > > > +
> > > > +/**
> > > > + * adxl345_set_inact_time_s - Configure inactivity time explicitly or by ODR.
> > > > + * @st: The sensor state instance.
> > > > + * @val_s: A desired time value, between 0 and 255.
> > > > + *
> > > > + * Inactivity time can be configured between 1 and 255 sec. If a val_s of 0
> > > > + * is configured by a user, then a default inactivity time will be computed.
> > > > + *
> > > > + * In such case, it should take power consumption into consideration. Thus it
> > > > + * shall be shorter for higher frequencies and longer for lower frequencies.
> > > > + * Hence, frequencies above 255 Hz shall default to 10 s and frequencies below
> > > > + * 10 Hz shall result in 255 s to detect inactivity.
> > > > + *
> > > > + * The approach simply subtracts the pre-decimal figure of the configured
> > > > + * sample frequency from 255 s to compute inactivity time [s]. Sub-Hz are thus
> > > > + * ignored in this estimation. The recommended ODRs for various features
> > > > + * (activity/inactivity, sleep modes, free fall, etc.) lie between 12.5 Hz and
> > > > + * 400 Hz, thus higher or lower frequencies will result in the boundary
> > > > + * defaults or need to be explicitly specified via val_s.
> > > > + *
> > > > + * Return: 0 or error value.
> > > > + */
> > > > +static int adxl345_set_inact_time_s(struct adxl345_state *st, u32 val_s)
> > > > +{
> > > > + unsigned int max_boundary = 255;
> > > > + unsigned int min_boundary = 10;
> > > > + unsigned int val = min(val_s, max_boundary);
> > > > + enum adxl345_odr odr;
> > > > + unsigned int regval;
> > > > + int ret;
> > > > +
> > > > + if (val == 0) {
> > > > + ret = regmap_read(st->regmap, ADXL345_REG_BW_RATE, ®val);
> > > > + if (ret)
> > > > + return ret;
> > > > + odr = FIELD_GET(ADXL345_BW_RATE_MSK, regval);
> > > > +
> > > > + val = (adxl345_odr_tbl[odr][0] > max_boundary)
> > > > + ? min_boundary : max_boundary - adxl345_odr_tbl[odr][0];
> > > > + }
> > > > +
> > > > + return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
> > > > }
> > > >
> > > > /* tap */
> > > > @@ -837,6 +943,13 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
> > > > if (ret)
> > > > return ret;
> > > > return int_en;
> > > > + case IIO_EV_DIR_FALLING:
> > > > + ret = adxl345_is_act_inact_en(st, chan->channel2,
> > >
> > > Does it makes sense to allow inactivity detection on a subset of channels but then
> > > report it as XYZ? I guess it didn't matter when it was and OR, but if we
> > > change to AND as suggested that is going to be misleading.
> > >
> > > we might have to allow separate enables but report an event as the combination
> > > of channels that are enabled X_AND_Y, X_AND_Z etc I guess we can improve activity
> > > channel case as well by doing that with the X_OR_Y etc
> > >
> >
> > Well, initially I guess I only had one enable for inactivity.
> >
> > This was kind of confusing to me. There is a register to enable
> > activity and inactivity on a per axis base [ACT_INACT_CTL, 0x27].
>
> Agreed this is a slightly odd concept.
>
> >
> > The interrupt event will set a single bit for inactivity or activity
> > [INT_SOURCE, 0x30]. In the interrupt handler further one can read out
> > the [ACT_TAP_STATUS, 0x2B], which contains tap and activity
> > directions, but no information about inactivity axis.
> >
> > In summary, for the ADXL345 inactivity can be configured on a per axis
> > base, but the event won't tell about the axis that fell into
> > inactivity, i.e. the first inactivity is supposed to put the sensor
> > into power save (with link bit and power modes set - I think
> > inactivity should mainly be seen in the context of their/Analog's
> > power save concept). As said before, initially I only provided a
> > single "inactivity enable". Then I saw actually I could set and offer
> > this per axis. I don't know if there are use cases only to observe
> > particularly the x-axis for a general power save. Probably rather not.
> >
> > So, I agree. But if you don't tell me explicitely to replace per axis
> > enables by a single one, I'll probably leave it as is. It implements
> > most transparently what the sensor can offer for configuration.
>
> The snag is what I mentioned for freefall. It becomes very hard to indicate
> to userspace what it might expect for the x&y&z cases. If inactivity requires
> them all to be inactive, I think separate enables is going to be really
> tricky to build a consistent ABI around :(
>
> Some devices we've had in the past have allowed specific configuration of
> and / or for axis combinations. For those we've normally kept clear because
> the number of combinations gets sill quickly.
>
> If we don't have a separate channel enable usecase today I think we should
> go ahead with general inactivity / activity (and/or as appropriate) and
> perhaps solve the per axis case if anyone ever cares about it.
>
Well, I think here we need to distinguish:
Activity: would allow per axis enables and events indicate per axis activity
Inactivity: allows per axis enables, but only a generic inactivity indication
So, also here, what's still missing? When doing it similarly to my
understanding of freefall now, for a v7 of the patches...
Activity:
- I would leave activity as is (is this ok?)
Inactivity:
- I replace single three axis enables by a generic enable, setting and
unsetting all three axis for inactivity
- I need probably also to provide a similar virtual channel
- The axis for this channel are AND'ed
- Now, with the virtual channel, usage will be "separate" instead of
"shared", which will result in a single enable handle in sysfs
Is this a correct understanding of what is +/- missing? Can you agree
to the points I listed up, or is something's missing (documentation of
course later)?
> >
> > >
> > >
> > > > + ADXL345_INACTIVITY,
> > > > + &int_en);
> > > > + if (ret)
> > > > + return ret;
> > > > + return int_en;
> > > > default:
> > > > return -EINVAL;
> > > > }
> > > > @@ -881,6 +994,9 @@ static int adxl345_write_event_config(struct iio_dev *indio_dev,
> > > > case IIO_EV_DIR_RISING:
> > > > return adxl345_set_act_inact_en(st, chan->channel2,
> > > > ADXL345_ACTIVITY, state);
> > > > + case IIO_EV_DIR_FALLING:
> > > > + return adxl345_set_act_inact_en(st, chan->channel2,
> > > > + ADXL345_INACTIVITY, state);
> > > > default:
> > > > return -EINVAL;
> > > > }
> > >
> > > > @@ -1314,6 +1458,17 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
> > > > return ret;
> > > > }
> > > >
> > > > + if (FIELD_GET(ADXL345_INT_INACTIVITY, int_stat)) {
> > > > + ret = iio_push_event(indio_dev,
> > > > + IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> > > > + IIO_MOD_X_OR_Y_OR_Z,
> > >
> > > So this is our open question. Similar to the free fall case. Do we have the boolean
> > > logic right way around?
> > >
> > > > + IIO_EV_TYPE_THRESH,
> > > > + IIO_EV_DIR_FALLING),
> > > > + ts);
> > > > + if (ret)
> > > > + return ret;
> > > > + }
> > > > +
>
On Mon, 21 Apr 2025 15:39:33 +0200
Lothar Rubusch <l.rubusch@gmail.com> wrote:
> On Mon, Apr 21, 2025 at 12:22 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Mon, 21 Apr 2025 00:12:17 +0200
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >
> > > Happy Easter (again)!
> > >
> > > On Fri, Apr 18, 2025 at 8:34 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > > >
> > > > On Mon, 14 Apr 2025 18:42:43 +0000
> > > > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> > > >
> > > > > Add the inactivity feature of the sensor. When activity and inactivity
> > > > > are enabled, a link bit will be set linking activity and inactivity
> > > > > handling. Additionally, the auto-sleep mode will be enabled. Due to the
> > > > > link bit the sensor is going to auto-sleep when inactivity was
> > > > > detected.
> > > > >
> > > > > Inactivity detection needs a threshold to be configured, and a time
> > > > > after which it will go into inactivity state if measurements under
> > > > > threshold.
> > > > >
> > > > > When a ODR is configured this time for inactivity is adjusted with a
> > > > > corresponding reasonable default value, in order to have higher
> > > > > frequencies and lower inactivity times, and lower sample frequency but
> > > > > give more time until inactivity. Both with reasonable upper and lower
> > > > > boundaries, since many of the sensor's features (e.g. auto-sleep) will
> > > > > need to operate beween 12.5 Hz and 400 Hz. This is a default setting
> > > > > when actively changing sample frequency, explicitly setting the time
> > > > > until inactivity will overwrite the default.
> > > > >
> > > > > Similarly, setting the g-range will provide a default value for the
> > > > > activity and inactivity thresholds. Both are implicit defaults, but
> > > > > equally can be overwritten to be explicitly configured.
> > > > >
> > > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > > Hi Lothar,
> > > >
> > > > Patches 6-8 look good to me.
> > > >
> > > > This runs into a similar issue to the freefall one. I haven't dug into
> > > > the datasheet but does it report on one channel going inactive, or
> > > > all being inactive at the same time? I checked and it is the all
> > > > case so we should be both on a pseudo channel to describe it right
> > > > and reporting IIO_MOD_X_AND_Y_AND_Z not the OR form.
> > > >
> > > > Sorry again that I'm only realising this on v6 :(
> > >
> > > No problem at all! Sure, I'm still in this phase where counting every
> > > single commit upstream makes my ego greater. On the long run, though,
> > > I guess it's better to build up knowledge and end up with a decent
> > > implementation quality, than just increasing a commit counter. For me
> > > it's fine. I also hope it's not too annoying for you.
> > >
> > > >
> > > > Difference is for Activity the definition is:
> > > > "The activity bit is set when acceleration greater than the value
> > > > stored in the THRESH_ACT register (Address 0x24) is experienced
> > > > on _any_ participating axis, set by the ACT_INACT_CTL register
> > > > (Address 0x27)."
> > > > vs Inactivity:
> > > > "The inactivity bit is set when acceleration of less than the value
> > > > stored in the THRESH_INACT register (Address 0x25) is experienced
> > > > for more time than is specified in the TIME_INACT
> > > > register (Address 0x26) on _all_ participating axes, as set by the
> > > > ACT_INACT_CTL register (Address 0x27). "
> > > >
> > > > So all vs any.
> > > >
> > >
> > > I think I see your point. At least I change here for inactivity, too,
> > > to AND'ed axis.
> > >
> > > IMHO, if I set OR here, the first axis raising the inactivity will put
> > > the sensor to sleep mode,
> > > where AND needs all three axis in inactivity state. I'm not sure if
> > > this works out, I need to verify
> > > it still with the hardware, for now I'll change this to AND.
> >
> > I'd be surprised if it worked differently but indeed good to check!
> >
> > >
> > > > > +
> > > > > +/**
> > > > > + * adxl345_set_inact_time_s - Configure inactivity time explicitly or by ODR.
> > > > > + * @st: The sensor state instance.
> > > > > + * @val_s: A desired time value, between 0 and 255.
> > > > > + *
> > > > > + * Inactivity time can be configured between 1 and 255 sec. If a val_s of 0
> > > > > + * is configured by a user, then a default inactivity time will be computed.
> > > > > + *
> > > > > + * In such case, it should take power consumption into consideration. Thus it
> > > > > + * shall be shorter for higher frequencies and longer for lower frequencies.
> > > > > + * Hence, frequencies above 255 Hz shall default to 10 s and frequencies below
> > > > > + * 10 Hz shall result in 255 s to detect inactivity.
> > > > > + *
> > > > > + * The approach simply subtracts the pre-decimal figure of the configured
> > > > > + * sample frequency from 255 s to compute inactivity time [s]. Sub-Hz are thus
> > > > > + * ignored in this estimation. The recommended ODRs for various features
> > > > > + * (activity/inactivity, sleep modes, free fall, etc.) lie between 12.5 Hz and
> > > > > + * 400 Hz, thus higher or lower frequencies will result in the boundary
> > > > > + * defaults or need to be explicitly specified via val_s.
> > > > > + *
> > > > > + * Return: 0 or error value.
> > > > > + */
> > > > > +static int adxl345_set_inact_time_s(struct adxl345_state *st, u32 val_s)
> > > > > +{
> > > > > + unsigned int max_boundary = 255;
> > > > > + unsigned int min_boundary = 10;
> > > > > + unsigned int val = min(val_s, max_boundary);
> > > > > + enum adxl345_odr odr;
> > > > > + unsigned int regval;
> > > > > + int ret;
> > > > > +
> > > > > + if (val == 0) {
> > > > > + ret = regmap_read(st->regmap, ADXL345_REG_BW_RATE, ®val);
> > > > > + if (ret)
> > > > > + return ret;
> > > > > + odr = FIELD_GET(ADXL345_BW_RATE_MSK, regval);
> > > > > +
> > > > > + val = (adxl345_odr_tbl[odr][0] > max_boundary)
> > > > > + ? min_boundary : max_boundary - adxl345_odr_tbl[odr][0];
> > > > > + }
> > > > > +
> > > > > + return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
> > > > > }
> > > > >
> > > > > /* tap */
> > > > > @@ -837,6 +943,13 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
> > > > > if (ret)
> > > > > return ret;
> > > > > return int_en;
> > > > > + case IIO_EV_DIR_FALLING:
> > > > > + ret = adxl345_is_act_inact_en(st, chan->channel2,
> > > >
> > > > Does it makes sense to allow inactivity detection on a subset of channels but then
> > > > report it as XYZ? I guess it didn't matter when it was and OR, but if we
> > > > change to AND as suggested that is going to be misleading.
> > > >
> > > > we might have to allow separate enables but report an event as the combination
> > > > of channels that are enabled X_AND_Y, X_AND_Z etc I guess we can improve activity
> > > > channel case as well by doing that with the X_OR_Y etc
> > > >
> > >
> > > Well, initially I guess I only had one enable for inactivity.
> > >
> > > This was kind of confusing to me. There is a register to enable
> > > activity and inactivity on a per axis base [ACT_INACT_CTL, 0x27].
> >
> > Agreed this is a slightly odd concept.
> >
> > >
> > > The interrupt event will set a single bit for inactivity or activity
> > > [INT_SOURCE, 0x30]. In the interrupt handler further one can read out
> > > the [ACT_TAP_STATUS, 0x2B], which contains tap and activity
> > > directions, but no information about inactivity axis.
> > >
> > > In summary, for the ADXL345 inactivity can be configured on a per axis
> > > base, but the event won't tell about the axis that fell into
> > > inactivity, i.e. the first inactivity is supposed to put the sensor
> > > into power save (with link bit and power modes set - I think
> > > inactivity should mainly be seen in the context of their/Analog's
> > > power save concept). As said before, initially I only provided a
> > > single "inactivity enable". Then I saw actually I could set and offer
> > > this per axis. I don't know if there are use cases only to observe
> > > particularly the x-axis for a general power save. Probably rather not.
> > >
> > > So, I agree. But if you don't tell me explicitely to replace per axis
> > > enables by a single one, I'll probably leave it as is. It implements
> > > most transparently what the sensor can offer for configuration.
> >
> > The snag is what I mentioned for freefall. It becomes very hard to indicate
> > to userspace what it might expect for the x&y&z cases. If inactivity requires
> > them all to be inactive, I think separate enables is going to be really
> > tricky to build a consistent ABI around :(
> >
> > Some devices we've had in the past have allowed specific configuration of
> > and / or for axis combinations. For those we've normally kept clear because
> > the number of combinations gets sill quickly.
> >
> > If we don't have a separate channel enable usecase today I think we should
> > go ahead with general inactivity / activity (and/or as appropriate) and
> > perhaps solve the per axis case if anyone ever cares about it.
> >
>
> Well, I think here we need to distinguish:
> Activity: would allow per axis enables and events indicate per axis activity
> Inactivity: allows per axis enables, but only a generic inactivity indication
Ah. I had it in my head it was only one set of per axis enables for the two
types of event. It's not! So indeed your description is what it should be.
>
> So, also here, what's still missing? When doing it similarly to my
> understanding of freefall now, for a v7 of the patches...
>
> Activity:
> - I would leave activity as is (is this ok?)
I think so given the separate enables.
>
> Inactivity:
> - I replace single three axis enables by a generic enable, setting and
> unsetting all three axis for inactivity
> - I need probably also to provide a similar virtual channel
Is it the same one? I think so but maybe I've lost track.
> - The axis for this channel are AND'ed
> - Now, with the virtual channel, usage will be "separate" instead of
> "shared", which will result in a single enable handle in sysfs
>
> Is this a correct understanding of what is +/- missing? Can you agree
> to the points I listed up, or is something's missing (documentation of
> course later)?
Looks good to me!
Jonathan
On Mon, Apr 21, 2025 at 3:54 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon, 21 Apr 2025 15:39:33 +0200
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > On Mon, Apr 21, 2025 at 12:22 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > On Mon, 21 Apr 2025 00:12:17 +0200
> > > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> > >
> > > > Happy Easter (again)!
> > > >
> > > > On Fri, Apr 18, 2025 at 8:34 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > > > >
> > > > > On Mon, 14 Apr 2025 18:42:43 +0000
> > > > > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> > > > >
> > > > > > Add the inactivity feature of the sensor. When activity and inactivity
> > > > > > are enabled, a link bit will be set linking activity and inactivity
> > > > > > handling. Additionally, the auto-sleep mode will be enabled. Due to the
> > > > > > link bit the sensor is going to auto-sleep when inactivity was
> > > > > > detected.
> > > > > >
> > > > > > Inactivity detection needs a threshold to be configured, and a time
> > > > > > after which it will go into inactivity state if measurements under
> > > > > > threshold.
> > > > > >
> > > > > > When a ODR is configured this time for inactivity is adjusted with a
> > > > > > corresponding reasonable default value, in order to have higher
> > > > > > frequencies and lower inactivity times, and lower sample frequency but
> > > > > > give more time until inactivity. Both with reasonable upper and lower
> > > > > > boundaries, since many of the sensor's features (e.g. auto-sleep) will
> > > > > > need to operate beween 12.5 Hz and 400 Hz. This is a default setting
> > > > > > when actively changing sample frequency, explicitly setting the time
> > > > > > until inactivity will overwrite the default.
> > > > > >
> > > > > > Similarly, setting the g-range will provide a default value for the
> > > > > > activity and inactivity thresholds. Both are implicit defaults, but
> > > > > > equally can be overwritten to be explicitly configured.
> > > > > >
> > > > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > > > Hi Lothar,
> > > > >
> > > > > Patches 6-8 look good to me.
> > > > >
> > > > > This runs into a similar issue to the freefall one. I haven't dug into
> > > > > the datasheet but does it report on one channel going inactive, or
> > > > > all being inactive at the same time? I checked and it is the all
> > > > > case so we should be both on a pseudo channel to describe it right
> > > > > and reporting IIO_MOD_X_AND_Y_AND_Z not the OR form.
> > > > >
> > > > > Sorry again that I'm only realising this on v6 :(
> > > >
> > > > No problem at all! Sure, I'm still in this phase where counting every
> > > > single commit upstream makes my ego greater. On the long run, though,
> > > > I guess it's better to build up knowledge and end up with a decent
> > > > implementation quality, than just increasing a commit counter. For me
> > > > it's fine. I also hope it's not too annoying for you.
> > > >
> > > > >
> > > > > Difference is for Activity the definition is:
> > > > > "The activity bit is set when acceleration greater than the value
> > > > > stored in the THRESH_ACT register (Address 0x24) is experienced
> > > > > on _any_ participating axis, set by the ACT_INACT_CTL register
> > > > > (Address 0x27)."
> > > > > vs Inactivity:
> > > > > "The inactivity bit is set when acceleration of less than the value
> > > > > stored in the THRESH_INACT register (Address 0x25) is experienced
> > > > > for more time than is specified in the TIME_INACT
> > > > > register (Address 0x26) on _all_ participating axes, as set by the
> > > > > ACT_INACT_CTL register (Address 0x27). "
> > > > >
> > > > > So all vs any.
> > > > >
> > > >
> > > > I think I see your point. At least I change here for inactivity, too,
> > > > to AND'ed axis.
> > > >
> > > > IMHO, if I set OR here, the first axis raising the inactivity will put
> > > > the sensor to sleep mode,
> > > > where AND needs all three axis in inactivity state. I'm not sure if
> > > > this works out, I need to verify
> > > > it still with the hardware, for now I'll change this to AND.
> > >
> > > I'd be surprised if it worked differently but indeed good to check!
> > >
> > > >
> > > > > > +
> > > > > > +/**
> > > > > > + * adxl345_set_inact_time_s - Configure inactivity time explicitly or by ODR.
> > > > > > + * @st: The sensor state instance.
> > > > > > + * @val_s: A desired time value, between 0 and 255.
> > > > > > + *
> > > > > > + * Inactivity time can be configured between 1 and 255 sec. If a val_s of 0
> > > > > > + * is configured by a user, then a default inactivity time will be computed.
> > > > > > + *
> > > > > > + * In such case, it should take power consumption into consideration. Thus it
> > > > > > + * shall be shorter for higher frequencies and longer for lower frequencies.
> > > > > > + * Hence, frequencies above 255 Hz shall default to 10 s and frequencies below
> > > > > > + * 10 Hz shall result in 255 s to detect inactivity.
> > > > > > + *
> > > > > > + * The approach simply subtracts the pre-decimal figure of the configured
> > > > > > + * sample frequency from 255 s to compute inactivity time [s]. Sub-Hz are thus
> > > > > > + * ignored in this estimation. The recommended ODRs for various features
> > > > > > + * (activity/inactivity, sleep modes, free fall, etc.) lie between 12.5 Hz and
> > > > > > + * 400 Hz, thus higher or lower frequencies will result in the boundary
> > > > > > + * defaults or need to be explicitly specified via val_s.
> > > > > > + *
> > > > > > + * Return: 0 or error value.
> > > > > > + */
> > > > > > +static int adxl345_set_inact_time_s(struct adxl345_state *st, u32 val_s)
> > > > > > +{
> > > > > > + unsigned int max_boundary = 255;
> > > > > > + unsigned int min_boundary = 10;
> > > > > > + unsigned int val = min(val_s, max_boundary);
> > > > > > + enum adxl345_odr odr;
> > > > > > + unsigned int regval;
> > > > > > + int ret;
> > > > > > +
> > > > > > + if (val == 0) {
> > > > > > + ret = regmap_read(st->regmap, ADXL345_REG_BW_RATE, ®val);
> > > > > > + if (ret)
> > > > > > + return ret;
> > > > > > + odr = FIELD_GET(ADXL345_BW_RATE_MSK, regval);
> > > > > > +
> > > > > > + val = (adxl345_odr_tbl[odr][0] > max_boundary)
> > > > > > + ? min_boundary : max_boundary - adxl345_odr_tbl[odr][0];
> > > > > > + }
> > > > > > +
> > > > > > + return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
> > > > > > }
> > > > > >
> > > > > > /* tap */
> > > > > > @@ -837,6 +943,13 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
> > > > > > if (ret)
> > > > > > return ret;
> > > > > > return int_en;
> > > > > > + case IIO_EV_DIR_FALLING:
> > > > > > + ret = adxl345_is_act_inact_en(st, chan->channel2,
> > > > >
> > > > > Does it makes sense to allow inactivity detection on a subset of channels but then
> > > > > report it as XYZ? I guess it didn't matter when it was and OR, but if we
> > > > > change to AND as suggested that is going to be misleading.
> > > > >
> > > > > we might have to allow separate enables but report an event as the combination
> > > > > of channels that are enabled X_AND_Y, X_AND_Z etc I guess we can improve activity
> > > > > channel case as well by doing that with the X_OR_Y etc
> > > > >
> > > >
> > > > Well, initially I guess I only had one enable for inactivity.
> > > >
> > > > This was kind of confusing to me. There is a register to enable
> > > > activity and inactivity on a per axis base [ACT_INACT_CTL, 0x27].
> > >
> > > Agreed this is a slightly odd concept.
> > >
> > > >
> > > > The interrupt event will set a single bit for inactivity or activity
> > > > [INT_SOURCE, 0x30]. In the interrupt handler further one can read out
> > > > the [ACT_TAP_STATUS, 0x2B], which contains tap and activity
> > > > directions, but no information about inactivity axis.
> > > >
> > > > In summary, for the ADXL345 inactivity can be configured on a per axis
> > > > base, but the event won't tell about the axis that fell into
> > > > inactivity, i.e. the first inactivity is supposed to put the sensor
> > > > into power save (with link bit and power modes set - I think
> > > > inactivity should mainly be seen in the context of their/Analog's
> > > > power save concept). As said before, initially I only provided a
> > > > single "inactivity enable". Then I saw actually I could set and offer
> > > > this per axis. I don't know if there are use cases only to observe
> > > > particularly the x-axis for a general power save. Probably rather not.
> > > >
> > > > So, I agree. But if you don't tell me explicitely to replace per axis
> > > > enables by a single one, I'll probably leave it as is. It implements
> > > > most transparently what the sensor can offer for configuration.
> > >
> > > The snag is what I mentioned for freefall. It becomes very hard to indicate
> > > to userspace what it might expect for the x&y&z cases. If inactivity requires
> > > them all to be inactive, I think separate enables is going to be really
> > > tricky to build a consistent ABI around :(
> > >
> > > Some devices we've had in the past have allowed specific configuration of
> > > and / or for axis combinations. For those we've normally kept clear because
> > > the number of combinations gets sill quickly.
> > >
> > > If we don't have a separate channel enable usecase today I think we should
> > > go ahead with general inactivity / activity (and/or as appropriate) and
> > > perhaps solve the per axis case if anyone ever cares about it.
> > >
> >
> > Well, I think here we need to distinguish:
> > Activity: would allow per axis enables and events indicate per axis activity
> > Inactivity: allows per axis enables, but only a generic inactivity indication
>
> Ah. I had it in my head it was only one set of per axis enables for the two
> types of event. It's not! So indeed your description is what it should be.
>
> >
> > So, also here, what's still missing? When doing it similarly to my
> > understanding of freefall now, for a v7 of the patches...
> >
> > Activity:
> > - I would leave activity as is (is this ok?)
>
> I think so given the separate enables.
>
> >
> > Inactivity:
> > - I replace single three axis enables by a generic enable, setting and
> > unsetting all three axis for inactivity
> > - I need probably also to provide a similar virtual channel
>
> Is it the same one? I think so but maybe I've lost track.
>
> > - The axis for this channel are AND'ed
> > - Now, with the virtual channel, usage will be "separate" instead of
> > "shared", which will result in a single enable handle in sysfs
> >
> > Is this a correct understanding of what is +/- missing? Can you agree
> > to the points I listed up, or is something's missing (documentation of
> > course later)?
> Looks good to me!
>
I fixed a v7 together. Eventually, I added a virtual channel for
inactivity and another one for freefall. Pls, let me know if this is
ok. So far it seems to work out perfectly. Now with the '...x&y&z...'
sysfs handle for enabling them. The docs are updated as well.
Best,
L
> Jonathan
>
© 2016 - 2025 Red Hat, Inc.