Make the sensor detect and issue interrupts at activity. Activity
events are configured by a threshold stored in regmap cache.
Activity, together with ODR and range setting are preparing a setup
together with inactivity coming in a follow up patch.
Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
drivers/iio/accel/adxl345_core.c | 183 +++++++++++++++++++++++++++++++
1 file changed, 183 insertions(+)
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index ab1ab09f348a..f1895925a80b 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -35,6 +35,7 @@
#define ADXL345_REG_TAP_AXIS_MSK GENMASK(2, 0)
#define ADXL345_REG_TAP_SUPPRESS_MSK BIT(3)
+#define ADXL345_REG_ACT_AXIS_MSK GENMASK(6, 4)
enum adxl345_axis {
ADXL345_Z_EN = BIT(0),
@@ -67,6 +68,23 @@ static const unsigned int adxl345_tap_time_reg[] = {
[ADXL345_TAP_TIME_DUR] = ADXL345_REG_DUR,
};
+/* activity/inactivity */
+enum adxl345_activity_type {
+ ADXL345_ACTIVITY,
+};
+
+static const unsigned int adxl345_act_int_reg[] = {
+ [ADXL345_ACTIVITY] = ADXL345_INT_ACTIVITY,
+};
+
+static const unsigned int adxl345_act_thresh_reg[] = {
+ [ADXL345_ACTIVITY] = ADXL345_REG_THRESH_ACT,
+};
+
+static const unsigned int adxl345_act_axis_msk[] = {
+ [ADXL345_ACTIVITY] = ADXL345_REG_ACT_AXIS_MSK,
+};
+
enum adxl345_odr {
ADXL345_ODR_0P10HZ = 0,
ADXL345_ODR_0P20HZ,
@@ -148,6 +166,7 @@ struct adxl345_state {
u8 watermark;
u8 fifo_mode;
+ u32 act_axis_ctrl;
u32 tap_axis_ctrl;
u32 tap_duration_us;
u32 tap_latent_us;
@@ -158,6 +177,13 @@ struct adxl345_state {
};
static struct iio_event_spec adxl345_events[] = {
+ {
+ /* activity */
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_RISING,
+ .mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE) |
+ BIT(IIO_EV_INFO_VALUE),
+ },
{
.type = IIO_EV_TYPE_GESTURE,
.dir = IIO_EV_DIR_SINGLETAP,
@@ -258,6 +284,73 @@ static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
return regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
}
+/* act/inact */
+
+static int adxl345_write_act_axis(struct adxl345_state *st,
+ enum adxl345_activity_type type, bool en)
+{
+ int ret;
+
+ /*
+ * The ADXL345 allows for individually enabling/disabling axis for
+ * activity and inactivity detection, respectively. Here both axis are
+ * kept in sync, i.e. an axis will be generally enabled or disabled for
+ * both equally, activity and inactivity detection.
+ */
+ if (type == ADXL345_ACTIVITY) {
+ st->act_axis_ctrl = en
+ ? st->act_axis_ctrl | ADXL345_REG_ACT_AXIS_MSK
+ : st->act_axis_ctrl & ~ADXL345_REG_ACT_AXIS_MSK;
+
+ ret = regmap_update_bits(st->regmap, ADXL345_REG_ACT_INACT_CTRL,
+ adxl345_act_axis_msk[type],
+ st->act_axis_ctrl);
+ if (ret)
+ return ret;
+ }
+ return 0;
+}
+
+static int adxl345_is_act_inact_en(struct adxl345_state *st,
+ enum adxl345_activity_type type, bool *en)
+{
+ int ret;
+ unsigned int regval;
+
+ ret = regmap_read(st->regmap, ADXL345_REG_INT_ENABLE, ®val);
+ if (ret)
+ return ret;
+
+ *en = (adxl345_act_int_reg[type] & regval) > 0;
+
+ return 0;
+}
+
+static int adxl345_set_act_inact_en(struct adxl345_state *st,
+ enum adxl345_activity_type type, bool cmd_en)
+{
+ bool axis_en, en = false;
+ unsigned int threshold;
+ int ret;
+
+ ret = adxl345_write_act_axis(st, type, cmd_en);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(st->regmap, adxl345_act_thresh_reg[type], &threshold);
+ if (ret)
+ return ret;
+
+ if (type == ADXL345_ACTIVITY) {
+ axis_en = FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, st->act_axis_ctrl) > 0;
+ en = axis_en && threshold > 0;
+ }
+
+ return regmap_update_bits(st->regmap, ADXL345_REG_INT_ENABLE,
+ adxl345_act_int_reg[type],
+ en ? adxl345_act_int_reg[type] : 0);
+}
+
/* tap */
static int adxl345_write_tap_axis(struct adxl345_state *st,
@@ -685,6 +778,16 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
int ret = -EFAULT;
switch (type) {
+ case IIO_EV_TYPE_THRESH:
+ switch (dir) {
+ case IIO_EV_DIR_RISING:
+ ret = adxl345_is_act_inact_en(st, ADXL345_ACTIVITY, &int_en);
+ if (ret)
+ return ret;
+ return int_en;
+ default:
+ return -EINVAL;
+ }
case IIO_EV_TYPE_GESTURE:
switch (chan->channel2) {
case IIO_MOD_X:
@@ -735,6 +838,13 @@ static int adxl345_write_event_config(struct iio_dev *indio_dev,
enum adxl345_axis axis;
switch (type) {
+ case IIO_EV_TYPE_THRESH:
+ switch (dir) {
+ case IIO_EV_DIR_RISING:
+ return adxl345_set_act_inact_en(st, ADXL345_ACTIVITY, state);
+ default:
+ return -EINVAL;
+ }
case IIO_EV_TYPE_GESTURE:
switch (chan->channel2) {
case IIO_MOD_X:
@@ -776,9 +886,29 @@ 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;
+ unsigned int act_threshold;
int ret;
switch (type) {
+ case IIO_EV_TYPE_THRESH:
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ switch (dir) {
+ case IIO_EV_DIR_RISING:
+ ret = regmap_read(st->regmap,
+ adxl345_act_thresh_reg[ADXL345_ACTIVITY],
+ &act_threshold);
+ if (ret)
+ return ret;
+
+ *val = act_threshold;
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+ default:
+ return -EINVAL;
+ }
case IIO_EV_TYPE_GESTURE:
switch (info) {
case IIO_EV_INFO_VALUE:
@@ -842,6 +972,23 @@ static int adxl345_write_event_value(struct iio_dev *indio_dev,
return ret;
switch (type) {
+ case IIO_EV_TYPE_THRESH:
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ switch (dir) {
+ case IIO_EV_DIR_RISING:
+ ret = regmap_write(st->regmap,
+ adxl345_act_thresh_reg[ADXL345_ACTIVITY],
+ val);
+ break;
+ default:
+ ret = -EINVAL;
+ }
+ break;
+ default:
+ ret = -EINVAL;
+ }
+ break;
case IIO_EV_TYPE_GESTURE:
switch (info) {
case IIO_EV_INFO_VALUE:
@@ -1124,6 +1271,17 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
return ret;
}
+ if (FIELD_GET(ADXL345_INT_ACTIVITY, int_stat)) {
+ ret = iio_push_event(indio_dev,
+ IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
+ act_tap_dir,
+ IIO_EV_TYPE_THRESH,
+ IIO_EV_DIR_RISING),
+ 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,
@@ -1157,6 +1315,7 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
ret = regmap_read(st->regmap, ADXL345_REG_ACT_TAP_STATUS, ®val);
if (ret)
return ret;
+ /* tap direction */
if (FIELD_GET(ADXL345_Z_EN, regval) > 0)
act_tap_dir = IIO_MOD_Z;
else if (FIELD_GET(ADXL345_Y_EN, regval) > 0)
@@ -1165,6 +1324,19 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
act_tap_dir = IIO_MOD_X;
}
+ if (FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, st->act_axis_ctrl) > 0) {
+ ret = regmap_read(st->regmap, ADXL345_REG_ACT_TAP_STATUS, ®val);
+ if (ret)
+ return ret;
+ /* activity direction */
+ if (FIELD_GET(ADXL345_Z_EN, regval >> 4) > 0)
+ act_tap_dir = IIO_MOD_Z;
+ else if (FIELD_GET(ADXL345_Y_EN, regval >> 4) > 0)
+ act_tap_dir = IIO_MOD_Y;
+ else if (FIELD_GET(ADXL345_X_EN, regval >> 4) > 0)
+ act_tap_dir = IIO_MOD_X;
+ }
+
if (regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, &int_stat))
return IRQ_NONE;
@@ -1248,6 +1420,13 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
return -ENODEV;
st->fifo_delay = fifo_delay_default;
+ /*
+ * If the feature is enabled, scan all axis for activity and or
+ * inactivity, and set activity and inactivity to the same ac / dc
+ * setup.
+ */
+ st->act_axis_ctrl = ADXL345_REG_ACT_AXIS_MSK;
+
/* Init with reasonable values */
tap_threshold = 48; /* 48 [0x30] -> ~3g */
st->tap_duration_us = 16; /* 16 [0x10] -> .010 */
@@ -1273,6 +1452,10 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
if (ret)
return ret;
+ ret = adxl345_set_range(st, ADXL345_16G_RANGE);
+ if (ret)
+ return ret;
+
/* Reset interrupts at start up */
ret = regmap_write(st->regmap, ADXL345_REG_INT_ENABLE, 0x00);
if (ret)
--
2.39.5On Thu, 20 Feb 2025 10:42:31 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:
> Make the sensor detect and issue interrupts at activity. Activity
> events are configured by a threshold stored in regmap cache.
>
> Activity, together with ODR and range setting are preparing a setup
> together with inactivity coming in a follow up patch.
>
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
Some questions / comments inline.
This review is has been at random moments whilst travelling (hence
over several days!) so it may be less than thorough or consistent!
I should be back to normal sometime next week.
> @@ -258,6 +284,73 @@ static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
> return regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
> }
>
> +/* act/inact */
> +
> +static int adxl345_write_act_axis(struct adxl345_state *st,
> + enum adxl345_activity_type type, bool en)
> +{
> + int ret;
> +
> + /*
> + * The ADXL345 allows for individually enabling/disabling axis for
> + * activity and inactivity detection, respectively. Here both axis are
> + * kept in sync, i.e. an axis will be generally enabled or disabled for
> + * both equally, activity and inactivity detection.
Why? Can definitely see people only being interested in one case
and not the other. What advantage is there in always having both
or neither over separate controls?
> + */
> + if (type == ADXL345_ACTIVITY) {
> + st->act_axis_ctrl = en
> + ? st->act_axis_ctrl | ADXL345_REG_ACT_AXIS_MSK
> + : st->act_axis_ctrl & ~ADXL345_REG_ACT_AXIS_MSK;
> +
> + ret = regmap_update_bits(st->regmap, ADXL345_REG_ACT_INACT_CTRL,
> + adxl345_act_axis_msk[type],
> + st->act_axis_ctrl);
> + if (ret)
> + return ret;
> + }
> + return 0;
> +}
> +static int adxl345_set_act_inact_en(struct adxl345_state *st,
> + enum adxl345_activity_type type, bool cmd_en)
> +{
> + bool axis_en, en = false;
I'm not keen on mix of set and unset in a declaration line. Better to
use two lines here as it can be hard to spot when things are or aren't
initialized when that is not the intent.
bool en = false;
bool axis_en;
> + unsigned int threshold;
> + int ret;
> +
> + ret = adxl345_write_act_axis(st, type, cmd_en);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(st->regmap, adxl345_act_thresh_reg[type], &threshold);
> + if (ret)
> + return ret;
> +
> + if (type == ADXL345_ACTIVITY) {
> + axis_en = FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, st->act_axis_ctrl) > 0;
> + en = axis_en && threshold > 0;
> + }
> +
> + return regmap_update_bits(st->regmap, ADXL345_REG_INT_ENABLE,
> + adxl345_act_int_reg[type],
> + en ? adxl345_act_int_reg[type] : 0);
> +}
> +
> @@ -842,6 +972,23 @@ static int adxl345_write_event_value(struct iio_dev *indio_dev,
> return ret;
>
> switch (type) {
> + case IIO_EV_TYPE_THRESH:
> + switch (info) {
> + case IIO_EV_INFO_VALUE:
> + switch (dir) {
> + case IIO_EV_DIR_RISING:
> + ret = regmap_write(st->regmap,
> + adxl345_act_thresh_reg[ADXL345_ACTIVITY],
> + val);
> + break;
This collection of breaks and nested functions suggests maybe we can either
return directly (I've lost track of what happens after this) or that
we should factor out some of this code to allow direct returns in the
function we put that code in. Chasing the breaks is not great if it
doesn't lead to anything interesting.
> + default:
> + ret = -EINVAL;
> + }
> + break;
> + default:
> + ret = -EINVAL;
> + }
> + break;
> case IIO_EV_TYPE_GESTURE:
> switch (info) {
> case IIO_EV_INFO_VALUE:
> @@ -1124,6 +1271,17 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
> return ret;
> }
>
> + if (FIELD_GET(ADXL345_INT_ACTIVITY, int_stat)) {
> + ret = iio_push_event(indio_dev,
> + IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> + act_tap_dir,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_RISING),
> + 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,
> @@ -1157,6 +1315,7 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
> ret = regmap_read(st->regmap, ADXL345_REG_ACT_TAP_STATUS, ®val);
> if (ret)
> return ret;
> + /* tap direction */
Belongs in earlier patch?
> if (FIELD_GET(ADXL345_Z_EN, regval) > 0)
> act_tap_dir = IIO_MOD_Z;
> else if (FIELD_GET(ADXL345_Y_EN, regval) > 0)
> @@ -1165,6 +1324,19 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
> act_tap_dir = IIO_MOD_X;
> }
>
> + if (FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, st->act_axis_ctrl) > 0) {
> + ret = regmap_read(st->regmap, ADXL345_REG_ACT_TAP_STATUS, ®val);
> + if (ret)
> + return ret;
> + /* activity direction */
> + if (FIELD_GET(ADXL345_Z_EN, regval >> 4) > 0)
I'm not following the shifts here. That looks like we don't have
defines that we should but instead use the ones for the lower fields.
> + act_tap_dir = IIO_MOD_Z;
> + else if (FIELD_GET(ADXL345_Y_EN, regval >> 4) > 0)
> + act_tap_dir = IIO_MOD_Y;
> + else if (FIELD_GET(ADXL345_X_EN, regval >> 4) > 0)
> + act_tap_dir = IIO_MOD_X;
> + }
Hi Jonathan,
I'm currently about to update the series and like to answer some of
your review comments directly when submitting. Nevertheless, here I'll
anticipate this one. Pls, find my questions inlined down below.
On Tue, Mar 4, 2025 at 2:49 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu, 20 Feb 2025 10:42:31 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Make the sensor detect and issue interrupts at activity. Activity
> > events are configured by a threshold stored in regmap cache.
> >
> > Activity, together with ODR and range setting are preparing a setup
> > together with inactivity coming in a follow up patch.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> Some questions / comments inline.
>
> This review is has been at random moments whilst travelling (hence
> over several days!) so it may be less than thorough or consistent!
> I should be back to normal sometime next week.
>
No problem, no hurry!
> > @@ -258,6 +284,73 @@ static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
> > return regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
> > }
> >
> > +/* act/inact */
> > +
> > +static int adxl345_write_act_axis(struct adxl345_state *st,
> > + enum adxl345_activity_type type, bool en)
> > +{
> > + int ret;
> > +
> > + /*
> > + * The ADXL345 allows for individually enabling/disabling axis for
> > + * activity and inactivity detection, respectively. Here both axis are
> > + * kept in sync, i.e. an axis will be generally enabled or disabled for
> > + * both equally, activity and inactivity detection.
>
> Why? Can definitely see people only being interested in one case
> and not the other. What advantage is there in always having both
> or neither over separate controls?
Ugh! This happens when I mix writing code and writing English texts,
w/o re-reading it.
Situation: The sensor allows to individually enable / disable x,y and
z axis for r activity and for inactivity. I don't offer this in the
driver. When activity is selected, I always enable all three axis or
disable them. Similar, for inactivity. The question is then actually,
if this is legitimate, or should I really implement enabling/disabling
of each axis individually for activity and similar then for
inactivity? I mean, when not interested in one or the other axis,
someone can fiilter the result. On the other side I can imagine a
small impact in power consumption, when only one axis is used instead
of three axis (?). Since the ADXL345 is [was] one of Analog's fancy
acme-ultra-low-power-sensors, power is definitvely a topic here IMHO.
I can't really estimate the importance.
I guess I'll try to implement it and see how ugly it gets. At least
it's a good exercise. As also, I'll try to bring regmap cache and
clear_bits / set_bits more in here for activity and inactivity in the
next version.
>
> > + */
> > + if (type == ADXL345_ACTIVITY) {
> > + st->act_axis_ctrl = en
> > + ? st->act_axis_ctrl | ADXL345_REG_ACT_AXIS_MSK
> > + : st->act_axis_ctrl & ~ADXL345_REG_ACT_AXIS_MSK;
> > +
> > + ret = regmap_update_bits(st->regmap, ADXL345_REG_ACT_INACT_CTRL,
> > + adxl345_act_axis_msk[type],
> > + st->act_axis_ctrl);
> > + if (ret)
> > + return ret;
> > + }
> > + return 0;
> > +}
>
>
> > +static int adxl345_set_act_inact_en(struct adxl345_state *st,
> > + enum adxl345_activity_type type, bool cmd_en)
> > +{
> > + bool axis_en, en = false;
> I'm not keen on mix of set and unset in a declaration line. Better to
> use two lines here as it can be hard to spot when things are or aren't
> initialized when that is not the intent.
>
> bool en = false;
> bool axis_en;
>
> > + unsigned int threshold;
> > + int ret;
> > +
> > + ret = adxl345_write_act_axis(st, type, cmd_en);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_read(st->regmap, adxl345_act_thresh_reg[type], &threshold);
> > + if (ret)
> > + return ret;
> > +
> > + if (type == ADXL345_ACTIVITY) {
> > + axis_en = FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, st->act_axis_ctrl) > 0;
> > + en = axis_en && threshold > 0;
> > + }
> > +
> > + return regmap_update_bits(st->regmap, ADXL345_REG_INT_ENABLE,
> > + adxl345_act_int_reg[type],
> > + en ? adxl345_act_int_reg[type] : 0);
> > +}
> > +
>
> > @@ -842,6 +972,23 @@ static int adxl345_write_event_value(struct iio_dev *indio_dev,
> > return ret;
> >
> > switch (type) {
> > + case IIO_EV_TYPE_THRESH:
> > + switch (info) {
> > + case IIO_EV_INFO_VALUE:
> > + switch (dir) {
> > + case IIO_EV_DIR_RISING:
> > + ret = regmap_write(st->regmap,
> > + adxl345_act_thresh_reg[ADXL345_ACTIVITY],
> > + val);
> > + break;
> This collection of breaks and nested functions suggests maybe we can either
> return directly (I've lost track of what happens after this) or that
> we should factor out some of this code to allow direct returns in the
> function we put that code in. Chasing the breaks is not great if it
> doesn't lead to anything interesting.
I understand, but since I'm using quite a bit configuration for the
sensor, I'm taking advantage
of type, info and dir here. It won't get more complex than that. I'm
[actually] pretty sure, since this
then is almost feature complete.
I don't see a different way how to do it. I mean, I could still
separate handling the "dir" entirely in
a called function. Or, say, implement IIO_EV_TYPE_THRESH handling in a
separate function?
Pls, let me know what you think.
> > + default:
> > + ret = -EINVAL;
> > + }
> > + break;
> > + default:
> > + ret = -EINVAL;
> > + }
> > + break;
> > case IIO_EV_TYPE_GESTURE:
> > switch (info) {
> > case IIO_EV_INFO_VALUE:
> > @@ -1124,6 +1271,17 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
> > return ret;
> > }
> >
> > + if (FIELD_GET(ADXL345_INT_ACTIVITY, int_stat)) {
> > + ret = iio_push_event(indio_dev,
> > + IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> > + act_tap_dir,
> > + IIO_EV_TYPE_THRESH,
> > + IIO_EV_DIR_RISING),
> > + 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,
> > @@ -1157,6 +1315,7 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
> > ret = regmap_read(st->regmap, ADXL345_REG_ACT_TAP_STATUS, ®val);
> > if (ret)
> > return ret;
> > + /* tap direction */
>
> Belongs in earlier patch?
>
> > if (FIELD_GET(ADXL345_Z_EN, regval) > 0)
> > act_tap_dir = IIO_MOD_Z;
> > else if (FIELD_GET(ADXL345_Y_EN, regval) > 0)
> > @@ -1165,6 +1324,19 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
> > act_tap_dir = IIO_MOD_X;
> > }
> >
> > + if (FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, st->act_axis_ctrl) > 0) {
> > + ret = regmap_read(st->regmap, ADXL345_REG_ACT_TAP_STATUS, ®val);
> > + if (ret)
> > + return ret;
> > + /* activity direction */
> > + if (FIELD_GET(ADXL345_Z_EN, regval >> 4) > 0)
>
> I'm not following the shifts here. That looks like we don't have
> defines that we should but instead use the ones for the lower fields.
The 8-bit register is split as follows:
| 7 | 6 | 5 | 4 | 3
| 2 | 1 | 0 |
----------------------------------------------------------------------------------------------------------------------
| ACT ac/dc | ACT_X | ACT_Y | ACT_Z | INACT ac/dc | INACT_X | INACT_Y
| INACT_Z |
I thought here, either I shift the ACT_* directions by 4 then use the
general mask for axis (lower 4 bits). Or I introduce an axis enum for
ACT_* and a separate one for INACT_*. Thus, I kept the shift and use
the same ADXL345_*_EN mask. How can I improve this, or can this stay?
>
> > + act_tap_dir = IIO_MOD_Z;
> > + else if (FIELD_GET(ADXL345_Y_EN, regval >> 4) > 0)
> > + act_tap_dir = IIO_MOD_Y;
> > + else if (FIELD_GET(ADXL345_X_EN, regval >> 4) > 0)
> > + act_tap_dir = IIO_MOD_X;
> > + }
>
>
Best,
L
On Tue, 11 Mar 2025 11:55:05 +0100
Lothar Rubusch <l.rubusch@gmail.com> wrote:
> Hi Jonathan,
>
> I'm currently about to update the series and like to answer some of
> your review comments directly when submitting. Nevertheless, here I'll
> anticipate this one. Pls, find my questions inlined down below.
>
> On Tue, Mar 4, 2025 at 2:49 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Thu, 20 Feb 2025 10:42:31 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >
> > > Make the sensor detect and issue interrupts at activity. Activity
> > > events are configured by a threshold stored in regmap cache.
> > >
> > > Activity, together with ODR and range setting are preparing a setup
> > > together with inactivity coming in a follow up patch.
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > Some questions / comments inline.
> >
> > This review is has been at random moments whilst travelling (hence
> > over several days!) so it may be less than thorough or consistent!
> > I should be back to normal sometime next week.
> >
>
> No problem, no hurry!
>
> > > @@ -258,6 +284,73 @@ static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
> > > return regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
> > > }
> > >
> > > +/* act/inact */
> > > +
> > > +static int adxl345_write_act_axis(struct adxl345_state *st,
> > > + enum adxl345_activity_type type, bool en)
> > > +{
> > > + int ret;
> > > +
> > > + /*
> > > + * The ADXL345 allows for individually enabling/disabling axis for
> > > + * activity and inactivity detection, respectively. Here both axis are
> > > + * kept in sync, i.e. an axis will be generally enabled or disabled for
> > > + * both equally, activity and inactivity detection.
> >
> > Why? Can definitely see people only being interested in one case
> > and not the other. What advantage is there in always having both
> > or neither over separate controls?
>
> Ugh! This happens when I mix writing code and writing English texts,
> w/o re-reading it.
>
> Situation: The sensor allows to individually enable / disable x,y and
> z axis for r activity and for inactivity. I don't offer this in the
> driver. When activity is selected, I always enable all three axis or
> disable them. Similar, for inactivity.
Ah. All axis together, not always both inactivty and activity.
> The question is then actually,
> if this is legitimate, or should I really implement enabling/disabling
> of each axis individually for activity and similar then for
> inactivity? I mean, when not interested in one or the other axis,
> someone can fiilter the result.
For some sensors there is no way to distinguish which threshold was hit
(they just provide 1 bit for activity in the status register)
Here it seems we get a single bit that indicates first act or inact
triggering axis (in ACT_TAP_STATUS). Assuming I read that
text correcty only one bit is set. That's not exactly useful as
it doesn't tell use other axis would have triggered it.
So I think here your approach of all axis enable is perhaps the
right approach. Also report it as an X_OR_Y_OR_Z event and ignore
the indication of which axis perhaps?
> On the other side I can imagine a
> small impact in power consumption, when only one axis is used instead
> of three axis (?). Since the ADXL345 is [was] one of Analog's fancy
> acme-ultra-low-power-sensors, power is definitvely a topic here IMHO.
> I can't really estimate the importance.
I doubt it would make a measurable difference to power usage.
>
> I guess I'll try to implement it and see how ugly it gets. At least
> it's a good exercise. As also, I'll try to bring regmap cache and
> clear_bits / set_bits more in here for activity and inactivity in the
> next version.
>
> > > @@ -842,6 +972,23 @@ static int adxl345_write_event_value(struct iio_dev *indio_dev,
> > > return ret;
> > >
> > > switch (type) {
> > > + case IIO_EV_TYPE_THRESH:
> > > + switch (info) {
> > > + case IIO_EV_INFO_VALUE:
> > > + switch (dir) {
> > > + case IIO_EV_DIR_RISING:
> > > + ret = regmap_write(st->regmap,
> > > + adxl345_act_thresh_reg[ADXL345_ACTIVITY],
> > > + val);
> > > + break;
> > This collection of breaks and nested functions suggests maybe we can either
> > return directly (I've lost track of what happens after this) or that
> > we should factor out some of this code to allow direct returns in the
> > function we put that code in. Chasing the breaks is not great if it
> > doesn't lead to anything interesting.
>
> I understand, but since I'm using quite a bit configuration for the
> sensor, I'm taking advantage
> of type, info and dir here. It won't get more complex than that. I'm
> [actually] pretty sure, since this
> then is almost feature complete.
>
> I don't see a different way how to do it. I mean, I could still
> separate handling the "dir" entirely in
> a called function. Or, say, implement IIO_EV_TYPE_THRESH handling in a
> separate function?
> Pls, let me know what you think.
Maybe factor everything out between the set_measure_en and it's counterpart.
Then you can just return in all paths in the factored out function.
That's nice because anyone reading it doesn't have to chase down to
see what else happens.
It might make sense to break it up further but probably not.
>
> > > + default:
> > > + ret = -EINVAL;
> > > + }
> > > + break;
> > > + default:
> > > + ret = -EINVAL;
> > > + }
> > > + break;
> > > case IIO_EV_TYPE_GESTURE:
> > > switch (info) {
> > > case IIO_EV_INFO_VALUE:
> > > @@ -1124,6 +1271,17 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
> > > return ret;
> > > }
> > >
> > > + if (FIELD_GET(ADXL345_INT_ACTIVITY, int_stat)) {
> > > + ret = iio_push_event(indio_dev,
> > > + IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> > > + act_tap_dir,
> > > + IIO_EV_TYPE_THRESH,
> > > + IIO_EV_DIR_RISING),
> > > + 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,
> > > @@ -1157,6 +1315,7 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
> > > ret = regmap_read(st->regmap, ADXL345_REG_ACT_TAP_STATUS, ®val);
> > > if (ret)
> > > return ret;
> > > + /* tap direction */
> >
> > Belongs in earlier patch?
> >
> > > if (FIELD_GET(ADXL345_Z_EN, regval) > 0)
> > > act_tap_dir = IIO_MOD_Z;
> > > else if (FIELD_GET(ADXL345_Y_EN, regval) > 0)
> > > @@ -1165,6 +1324,19 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
> > > act_tap_dir = IIO_MOD_X;
> > > }
> > >
> > > + if (FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, st->act_axis_ctrl) > 0) {
> > > + ret = regmap_read(st->regmap, ADXL345_REG_ACT_TAP_STATUS, ®val);
> > > + if (ret)
> > > + return ret;
> > > + /* activity direction */
> > > + if (FIELD_GET(ADXL345_Z_EN, regval >> 4) > 0)
> >
> > I'm not following the shifts here. That looks like we don't have
> > defines that we should but instead use the ones for the lower fields.
>
> The 8-bit register is split as follows:
>
> | 7 | 6 | 5 | 4 | 3
> | 2 | 1 | 0 |
> ----------------------------------------------------------------------------------------------------------------------
> | ACT ac/dc | ACT_X | ACT_Y | ACT_Z | INACT ac/dc | INACT_X | INACT_Y
> | INACT_Z |
>
> I thought here, either I shift the ACT_* directions by 4 then use the
> general mask for axis (lower 4 bits). Or I introduce an axis enum for
> ACT_* and a separate one for INACT_*. Thus, I kept the shift and use
> the same ADXL345_*_EN mask. How can I improve this, or can this stay?
I'd not use enums here at all unless you actually use the enum
type directly somewhere to enforce allowed values.
Separate defines for inact and act make sense though. Saves the reader
of this bit of the code having to care about the layout of the fields,
Jonathan
>
> >
> > > + act_tap_dir = IIO_MOD_Z;
> > > + else if (FIELD_GET(ADXL345_Y_EN, regval >> 4) > 0)
> > > + act_tap_dir = IIO_MOD_Y;
> > > + else if (FIELD_GET(ADXL345_X_EN, regval >> 4) > 0)
> > > + act_tap_dir = IIO_MOD_X;
> > > + }
> >
> >
>
> Best,
> L
© 2016 - 2025 Red Hat, Inc.