Re: [PATCH v7 08/11] iio: accel: adxl345: add activity event feature

[Lothar Rubusch]

Hi Jonathan - Hi IIO list,

Please, find some (many) questions inlined down below. Appologies for
the separate
channels last time and not right away fixing them up as array. I did
not want to make extra work.

On Sun, Apr 27, 2025 at 2:48 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Mon, 21 Apr 2025 22:06:38 +0000
> Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote:
>
> > Make the sensor detect and issue interrupts at activity. Activity
> > events are configured by a threshold stored in regmap cache. Initialize
> > the activity threshold register to a reasonable default value in probe.
> > The value is taken from the older ADXL345 input driver, to provide a
> > similar behavior. Reset the activity/inactivity direction enabling
> > register in probe. Reset and initialization shall bring the sensor in a
> > defined initial state to prevent dangling settings when warm restarting
> > the sensor.
> >
> > Activity, ODR configuration together with the range setting prepare the
> > activity/inactivity hystersesis setup, implemented in a follow up patch.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@xxxxxxxxx>
> > ---
> >  drivers/iio/accel/adxl345_core.c | 217 ++++++++++++++++++++++++++++++-
> >  1 file changed, 214 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > index 80b5b8402ced..680981609d83 100644
> > --- a/drivers/iio/accel/adxl345_core.c
> > +++ b/drivers/iio/accel/adxl345_core.c
> > @@ -36,11 +36,16 @@
> >  #define ADXL345_REG_TAP_AXIS_MSK     GENMASK(2, 0)
> >  #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_TAP_Z_EN             BIT(0)
> >  #define ADXL345_TAP_Y_EN             BIT(1)
> >  #define ADXL345_TAP_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)
> > +
> >  /* single/double tap */
> >  enum adxl345_tap_type {
> >       ADXL345_SINGLE_TAP,
> > @@ -64,6 +69,19 @@ 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,
> > +};
> > +
> >  enum adxl345_odr {
> >       ADXL345_ODR_0P10HZ = 0,
> >       ADXL345_ODR_0P20HZ,
> > @@ -154,6 +172,13 @@ struct adxl345_state {
> >  };
> >
> >  static struct iio_event_spec adxl345_events[] = {
> > +     {
> > +             /* activity */
> > +             .type = IIO_EV_TYPE_THRESH,
>
> Is this a threshold, or a magnitude? I'd expect an activity detector
> to be magnitude as it doesn't care which way up the sensor is.
>

This is touching the main points still unclear to me. I tried to put
this into the
following questions. Could you please clarify?

1. Given a measurement "val", and a configured threshold "thr".
A "rising" for IIO_EV_TYPE_THRESH means: val > thr
where a "rising" for IIO_EV_TYPE_MAG means something like: val > |thr|

Q: Do I understand this correctly now?

Q: Is this documented somewhere (especially for reviewing further
EV_TYPE fields)?

Q: I wonder if I missed this for the Tap events. Going by this
definition, then actually the
tap events should be rather MAG events, too. Right?


2. I oriented myself mostly by reading other drivers, for instance the
ADXL367, the ADXL372, or also the more recent ADXL380. I am aware that
there might be differences among different
(Analog) sensors. But all those sensors specify Inactivity (and Activity) as a
IIO_EV_TYPE_THRESH with directions IIO_MOD_X_OR_Y_OR_Z.
Given the above, I implemented Activity and Inactivity events as
IIO_EV_TYPE_THRESH,
now I'm a bit confused.

Q: Why is this different for the ADXL345?

Q: If I implement Activity / Inactivity analogous to the e.g. a
ADXL380, then shouldn't it be IIO_EV_TYPE_THRESH with
IIO_MOD_X_OR_Y_OR_Z? Why not?


3. For the ADXL345, a Freefall signal is on all axis lower than
threshold (magnitude). Thus I push a IIO_MOD_X_AND_Y_AND_Z to a
separate
fake channel. Inactivity will be like Freefall independent of the axis.
The ADXL345 Activity can be configured by axis, as also the event will
respect the axis information.

Q: Setting up the "fake channel" to particuarly push to
IIO_MOD_X_AND_Y_AND_Z, I probably better should also evaluate
IIO_MOD_X_AND_Y_AND_Z in write_event_config(), write_event_value(),
etc. rather than evaluating IIO_MOD_-types as I'm currently
doing?

Q: Activity probably remains in the regular channels for the corresponding axis?


4. I implemented functions like adxl345_write_event_config(),
adxl345_write_event_value() or corresponding
readers, as follows
- THRESH/rising: Activity
- THRESH/falling: Inactivity
- MAG/falling: Freefall

If I change Activity and Inactivity to be both of type MAG, I will end
up with MAG/falling to indicate Freefall or equally Inactivity.
Both on the IIO_MOD_X_AND_Y_AND_Z channel. I admit (ab)using the
IIO_EV_TYPEs to solve my combinatorial issues for event configuration
is probably not as supposed to be.
Given you still ask me to do Inactivity and Freefall as MAG/falling
with IIO_MOD_X_AND_Y_AND_Z. The difference between both IMHO,
is that Activity and Inactivity for the ADXL345 indicate sensor state
changes, where Freefall indicates the particular event. The
sensor is either in "active" or "standby/inactive", where Freefall
just triggers and then retriggers and retriggers...

Q: What is the method to distinguish several similar IIO events, e.g.
to tag them somehow one as Freefall, the other one as Inactivity?

Best,
L

> > +             .dir = IIO_EV_DIR_RISING,
> > +             .mask_separate = BIT(IIO_EV_INFO_ENABLE),
> > +             .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE),
> > +     },
> >       {
> >               /* single tap */
> >               .type = IIO_EV_TYPE_GESTURE,
> > @@ -265,6 +290,99 @@ static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
> >       return regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
> >  }
> >
> Jonathan
>
>