[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20250515083638.14ee1292@jic23-huawei>
Date: Thu, 15 May 2025 08:36:38 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Lothar Rubusch <l.rubusch@...il.com>
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
On Wed, 7 May 2025 22:04:29 +0200
Lothar Rubusch <l.rubusch@...il.com> wrote:
> 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
>
yes, so 4 different events for inactivty/activity - only one type
of inactivity enabled at a time and only one type of activity event enabled
> 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.
Not quite. The caching bit is all fine but the sysfs events
have to represent AC/DC and they do that with different event types.
>
> 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?)?
activity_dc is a simple magnitude threshold type event.
activity_ac is one of two options depending on how we think about it.
MAG_REFERENCED if we ignore the reset of the reference when activity_ac and
inactivity_ac are coupled.
MAG_ADAPTIVE if we take it that the base point for the threshold is changing
over time (which is slightly more accurate if inactivity and activity are
coupled).
I think I slightly prefer MAG_ADAPTIVE but it's a corner case because
until the other state occurs we don't adapt and it looks more like
a referenced event.
So we'd have 4 event types for these. 2 for activity and 2 for inactivity.
Jonathan
>
> 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, ®val);
> > > > > + 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, ®val);
> > > > > + 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