Message-ID: <CAFXKEHaupT2RZqNaQnLjiKpCodHSHsswhRXCKAmGh_Tnr3iXJQ@mail.gmail.com>
Date: Wed, 7 May 2025 22:04:29 +0200
From: Lothar Rubusch <l.rubusch@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: lars@...afoo.de, Michael.Hennerich@...log.com, linux-iio@...r.kernel.org, 
	linux-kernel@...r.kernel.org, eraretuya@...il.com
Subject: Re: [PATCH v7 10/11] iio: accel: adxl345: add coupling detection for activity/inactivity

Hi Jonathan,

On Sun, May 4, 2025 at 12:39 PM Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Thu, 1 May 2025 09:35:29 +0200
> Lothar Rubusch <l.rubusch@...il.com> wrote:
>
> > On Sun, Apr 27, 2025 at 3:00 PM Jonathan Cameron <jic23@...nel.org> wrote:
> > >
> > > On Mon, 21 Apr 2025 22:06:40 +0000
> > > Lothar Rubusch <l.rubusch@...il.com> wrote:
> > >
> > > > Add coupling activity/inactivity detection by the AC/DC bit. This is an
> > > > addititional enhancement for the detection of activity states and
> > > > completes the activity / inactivity feature of the ADXL345.
> > > >
> > > > Signed-off-by: Lothar Rubusch <l.rubusch@...il.com>
> > >
> > > I've dragged the table from the earlier patch into this one that actually uses it.
> > > However I'm a little unsure on exactly how we present this feature.
> > >
> > > So until those questions are resolved I've dropped the patch (you'll need
> > > to rebase on my testing branch and fix up missing table for v8).
> > >
> > > The bit that made me not apply this series (with some tweaks) was that
> > > I'd expect enabling AC events to be visible as disabling of DC ones.
> > >
> >
> > There are no AC events, nor DC ones.
> >
> > Think of AC- or DC-coupled detection as modes of operating the
> > ACTIVITY/INACTIVITY
> > detection. The events are ACTIVITY or INACTIVITY.
>
> That's where we differ. For generalizing an ABI we absolutely should
> not think of them as modes - that's an implementation detail only
>
> > It has effect on how
> > the sensor detects
> > if it needs to trigger an (IN)/ACTIVITY event. DC is just going by the
> > configured thresholds,
> > where AC is supposed to apply some more elaborate way of declaring ACTIVITY or
> > INACTIVITY.
> >
> > The fact that you imply on this means to me, at least I explained it
> > wrong, or need to clarify
> > better.
>
> Not at all (I think).  This is a question of datasheet representation vs
> what we are doing in the ABI.  Lets have a thought experiment.
>
> If the device had separate enables, thresholds and periods for
> DC activity, DC inactivity, AC activity and AC inactivity would you be
> thinking of them as modes of one event?  They would be separate event types
> that could be all enabled at the same time.
>
> Now step 2.  We have many sensors that have limited numbers of highly programmable
> event detection engines (the adis IMUs for instance have only a few such engines
> but many types of events). For those we treat them as a fifo.  If you request
> 3 events and the device has 2 detectors you get the last two that were asked
> for.  We do this because we don't want a detector with 10s of modes with
> the parameter meaning changing for each one.  The ABI should look the same
> as independent detectors.  Some models are more complex and have more such
> engines, but the interface remains the same.
>
> So here we have a pair of less flexible engines that can support either
> AC or DC for inactivity/activity.  We should represent that the same
> way we represent the separate engines case.  Hence when you enable
> activity AC, our short (1 element) fifo means we disable activity DC.
>
> Which is why treating them as separate engines (with rules on which can
> be enabled at any time) is the right way to go.  IIRC correctly long
> ago we had various experiments with interfaces around this (including
> mode based ones) and ended up with this approach that looks like a
> whole bunch of event detectors with a mux that means we can only use
> a subset at a time.
>
> Jonathan
>

Thank  you so much for this explanation. It makes it much clearer to
me now to understand what
you were writing about before.

Anyway, I just wanted to confirm what is missing here. I'll need to
distinguish the following events:
- activity_ac
- inactivity_ac
- activity_dc
- inactivity_dc

For the implementation, each shall be a separate event, i.e. each will
have separate handles in sysfs:
- event config: enable
- event value: threshold
- event value: time

Internally I will use the regmap cached flag for AC/DC to distinguish
if the ACTIVITY or INACTIVITY event belongs to activity_ac or
inactivity_dc. Threshold and time values will be specific to ACTIVITY
and INACTIVITY, respectively. Since, the events will be still activity
/ inactivity, as their registers will be so, too. The distinction thus
will be rather a question of the presented handles in sysfs.
All events will be type MAG, and dir RISING (activity) or FALLING
(inactivity). As said above, dc /  ac coupling I will read from the
cache.

Is this approach sufficient to solve the required adjustments, or do
the events themselves need further information about coupling (I guess
so, how can I represent this in the events?)?

Best,
L

>
> >
> > > Also, I just noticed you aren't pushing the new event types.
> > >
> > > These controls need to look like a separate event detector hardware block
> > > with it's own controls + its own event codes.  The fact only this or
> > > the DC version can be enabled at any time should only be exposed in the
> > > reported state, not apparent via what files we expose etc.  On some
> > > other device they may be independent hardware blocks.
> > >
> > > Note I'd also expect to see value controls for these new events. You may
> > > need to cache the values and update on event change if the meaning is
> > > very different.   That's because the expectation would be an event
> > > setup sequence from userspace is:
> > >
> > > 1) Set value of threshold
> > > 2) Enable event
> > >
> > > On a change of event (due to shared hardware) The value set may scramble
> > > the event already enabled.
> > >
> > > So write the values into a cache and update to the right one when changing
> > > event.
> > >
> >
> > Might be that I got you wrong here, but I assume the above does
> > actually not apply.
> >
> > Best,
> > L
> >
> > > > ---
> > > >  drivers/iio/accel/adxl345_core.c | 162 ++++++++++++++++++++++++++++++-
> > > >  1 file changed, 159 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > > > index b25efcad069b..c07ad5774c8a 100644
> > > > --- a/drivers/iio/accel/adxl345_core.c
> > > > +++ b/drivers/iio/accel/adxl345_core.c
> > > > @@ -37,7 +37,9 @@
> > > >  #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_ACT_ACDC_MSK     BIT(7)
> > > >  #define ADXL345_REG_INACT_AXIS_MSK   GENMASK(2, 0)
> > > > +#define ADXL345_REG_INACT_ACDC_MSK   BIT(3)
> > > >  #define ADXL345_POWER_CTL_INACT_MSK  (ADXL345_POWER_CTL_AUTO_SLEEP | ADXL345_POWER_CTL_LINK)
> > > >
> > > >  #define ADXL345_TAP_Z_EN             BIT(0)
> > > > @@ -91,6 +93,11 @@ static const unsigned int adxl345_act_thresh_reg[] = {
> > > >       [ADXL345_INACTIVITY] = ADXL345_REG_THRESH_INACT,
> > > >  };
> > > >
> > > > +static const unsigned int adxl345_act_acdc_msk[] = {
> > > > +     [ADXL345_ACTIVITY] = ADXL345_REG_ACT_ACDC_MSK,
> > > > +     [ADXL345_INACTIVITY] = ADXL345_REG_INACT_ACDC_MSK,
> > > > +};
> > > > +
> > > >  enum adxl345_odr {
> > > >       ADXL345_ODR_0P10HZ = 0,
> > > >       ADXL345_ODR_0P20HZ,
> > > > @@ -204,6 +211,18 @@ static struct iio_event_spec adxl345_events[] = {
> > > >                       BIT(IIO_EV_INFO_RESET_TIMEOUT) |
> > > >                       BIT(IIO_EV_INFO_TAP2_MIN_DELAY),
> > > >       },
> > > > +     {
> > > > +             /* activity, activity - ac bit */
> > > Comment says activity and inactivity but channel type wise this
> > > is just activity (as rising)
> > >
> > > > +             .type = IIO_EV_TYPE_MAG_REFERENCED,
> > > > +             .dir = IIO_EV_DIR_RISING,
> > > > +             .mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE),
> > > > +     },
> > > > +     {
> > > > +             /* activity, inactivity - ac bit */
> > >
> > > Likewise this seems to be inactivity.  Should this be in the x&y&z
> > > channel, not this one?
> > >
> > > > +             .type = IIO_EV_TYPE_MAG_REFERENCED,
> > > > +             .dir = IIO_EV_DIR_FALLING,
> > > > +             .mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE),
> > > > +     },
> > > >  };
> > > >
> > > >  #define ADXL345_CHANNEL(index, reg, axis) {                                  \
> > > > @@ -320,6 +339,69 @@ static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
> > > >
> > > >  /* act/inact */
> > > >
> > > > +static int adxl345_is_act_inact_ac(struct adxl345_state *st,
> > > > +                                enum adxl345_activity_type type, bool *ac)
> > > > +{
> > > > +     unsigned int regval;
> > > > +     int ret;
> > > > +
> > > > +     ret = regmap_read(st->regmap, ADXL345_REG_ACT_INACT_CTRL, &regval);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     if (type == ADXL345_ACTIVITY)
> > > > +             *ac = FIELD_GET(ADXL345_REG_ACT_ACDC_MSK, regval);
> > > > +     else
> > > > +             *ac = FIELD_GET(ADXL345_REG_INACT_ACDC_MSK, regval);
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static int adxl345_set_act_inact_ac(struct adxl345_state *st,
> > > > +                                 enum adxl345_activity_type type, bool ac)
> > > > +{
> > > > +     unsigned int act_inact_ac = ac ? 0xff : 0x00;
> > > > +
> > > > +     /*
> > > > +      * A setting of false selects dc-coupled operation, and a setting of
> > > > +      * true enables ac-coupled operation. In dc-coupled operation, the
> > > > +      * current acceleration magnitude is compared directly with
> > > > +      * ADXL345_REG_THRESH_ACT and ADXL345_REG_THRESH_INACT to determine
> > > > +      * whether activity or inactivity is detected.
> > > > +      *
> > > > +      * In ac-coupled operation for activity detection, the acceleration
> > > > +      * value at the start of activity detection is taken as a reference
> > > > +      * value. New samples of acceleration are then compared to this
> > > > +      * reference value, and if the magnitude of the difference exceeds the
> > > > +      * ADXL345_REG_THRESH_ACT value, the device triggers an activity
> > > > +      * interrupt.
> > > > +      *
> > > > +      * Similarly, in ac-coupled operation for inactivity detection, a
> > > > +      * reference value is used for comparison and is updated whenever the
> > > > +      * device exceeds the inactivity threshold. After the reference value
> > > > +      * is selected, the device compares the magnitude of the difference
> > > > +      * between the reference value and the current acceleration with
> > > > +      * ADXL345_REG_THRESH_INACT. If the difference is less than the value in
> > > > +      * ADXL345_REG_THRESH_INACT for the time in ADXL345_REG_TIME_INACT, the
> > > > +      * device is considered inactive and the inactivity interrupt is
> > > > +      * triggered. [quoted from p. 24, ADXL345 datasheet Rev. G]
> > > > +      *
> > > > +      * In a conclusion, the first acceleration snapshot sample which hit the
> > > > +      * threshold in a particular direction is always taken as acceleration
> > > > +      * reference value to that direction. Since for the hardware activity
> > > > +      * and inactivity depend on the x/y/z axis, so do ac and dc coupling.
> > > > +      * Note, this sw driver always enables or disables all three x/y/z axis
> > > > +      * for detection via act_axis_ctrl and inact_axis_ctrl, respectively.
> > > > +      * Where in dc-coupling samples are compared against the thresholds, in
> > > > +      * ac-coupling measurement difference to the first acceleration
> > > > +      * reference value are compared against the threshold. So, ac-coupling
> > > > +      * allows for a bit more dynamic compensation depending on the initial
> > > > +      * sample.
> > > > +      */
> > > > +     return regmap_update_bits(st->regmap, ADXL345_REG_ACT_INACT_CTRL,
> > > > +                              adxl345_act_acdc_msk[type], act_inact_ac);
> > > > +}
> > >
> > > >  static int adxl345_find_range(struct adxl345_state *st, int val, int val2,
> > > > @@ -797,9 +886,51 @@ static int adxl345_find_range(struct adxl345_state *st, int val, int val2,
> > > >
> > > >  static int adxl345_set_range(struct adxl345_state *st, enum adxl345_range range)
> > > >  {
> > > > -     return regmap_update_bits(st->regmap, ADXL345_REG_DATA_FORMAT,
> > > > +     unsigned int act_threshold, inact_threshold;
> > > > +     unsigned int range_old;
> > > > +     unsigned int regval;
> > > > +     int ret;
> > > > +
> > > > +     ret = regmap_read(st->regmap, ADXL345_REG_DATA_FORMAT, &regval);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +     range_old = FIELD_GET(ADXL345_DATA_FORMAT_RANGE, regval);
> > > > +
> > > > +     ret = regmap_read(st->regmap,
> > > > +                       adxl345_act_thresh_reg[ADXL345_ACTIVITY],
> > > > +                       &act_threshold);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     ret = regmap_read(st->regmap,
> > > > +                       adxl345_act_thresh_reg[ADXL345_INACTIVITY],
> > > > +                       &inact_threshold);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     ret = regmap_update_bits(st->regmap, ADXL345_REG_DATA_FORMAT,
> > > >                                ADXL345_DATA_FORMAT_RANGE,
> > > >                                FIELD_PREP(ADXL345_DATA_FORMAT_RANGE, range));
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     act_threshold = act_threshold
> > > > +             * adxl345_range_factor_tbl[range_old]
> > > > +             / adxl345_range_factor_tbl[range];
> > > > +     act_threshold = min(255, max(1, inact_threshold));
> > > > +
> > > This is first use of the range table. So introduce that in this patch.
> > >
> > > > +     inact_threshold = inact_threshold
> > > > +             * adxl345_range_factor_tbl[range_old]
> > > > +             / adxl345_range_factor_tbl[range];
> > > > +     inact_threshold = min(255, max(1, inact_threshold));
> > > > +
> > > > +     ret = regmap_write(st->regmap, adxl345_act_thresh_reg[ADXL345_ACTIVITY],
> > > > +                        act_threshold);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     return regmap_write(st->regmap, adxl345_act_thresh_reg[ADXL345_INACTIVITY],
> > > > +                        inact_threshold);
> > > >  }
> > > >
> > > >  static int adxl345_read_avail(struct iio_dev *indio_dev,
> > > > @@ -938,7 +1069,7 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
> > > >                                    enum iio_event_direction dir)
> > > >  {
> > > >       struct adxl345_state *st = iio_priv(indio_dev);
> > > > -     bool int_en;
> > > > +     bool int_en, act_ac, inact_ac;
> > > >       int ret;
> > > >
> > > >       switch (type) {
> > > > @@ -983,6 +1114,21 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
> > > >               if (ret)
> > > >                       return ret;
> > > >               return int_en;
> > > > +     case IIO_EV_TYPE_MAG_REFERENCED:
> > > > +             switch (dir) {
> > > > +             case IIO_EV_DIR_RISING:
> > > > +                     ret = adxl345_is_act_inact_ac(st, ADXL345_ACTIVITY, &act_ac);
> > >
> > > Do we not need a check in the enabling of the DC events as well?  If we have enabled
> > > AC the DC one should report disabled (and if we enable that again then we should
> > > update this.
> > >
> > > > +                     if (ret)
> > > > +                             return ret;
> > > > +                     return act_ac;
> > > > +             case IIO_EV_DIR_FALLING:
> > > > +                     ret = adxl345_is_act_inact_ac(st, ADXL345_INACTIVITY, &inact_ac);
> > > > +                     if (ret)
> > > > +                             return ret;
> > > > +                     return inact_ac;
> > > > +             default:
> > > > +                     return -EINVAL;
> > > > +             }
> > > >       default:
> > > >               return -EINVAL;
> > > >       }
> > > > @@ -1019,6 +1165,16 @@ static int adxl345_write_event_config(struct iio_dev *indio_dev,
> > > >               }
> > > >       case IIO_EV_TYPE_MAG:
> > > >               return adxl345_set_ff_en(st, state);
> > > > +     case IIO_EV_TYPE_MAG_REFERENCED:
> > > > +             switch (dir) {
> > > > +             case IIO_EV_DIR_RISING:
> > > > +                     return adxl345_set_act_inact_ac(st, ADXL345_ACTIVITY, state);
> > >
> > > Similar to read path.  The DC events should be affected by this as well as the AC ones.
> > >
> > > > +             case IIO_EV_DIR_FALLING:
> > > > +                     return adxl345_set_act_inact_ac(st, ADXL345_INACTIVITY, state);
> > > > +             default:
> > > > +                     return -EINVAL;
> > > > +             }
> > > > +
> > > >       default:
> > > >               return -EINVAL;
> > > >       }
> > >
>

Powered by blists - more mailing lists