Message-ID: <CAFXKEHYsMKHMYoBq7U5n02=0wnVmp0_CUrbvxxFFRLJDayS7Kg@mail.gmail.com>
Date: Mon, 21 Apr 2025 15:39:33 +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 v6 09/11] iio: accel: adxl345: add inactivity feature

On Mon, Apr 21, 2025 at 12:22 PM Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Mon, 21 Apr 2025 00:12:17 +0200
> Lothar Rubusch <l.rubusch@...il.com> wrote:
>
> > Happy Easter (again)!
> >
> > On Fri, Apr 18, 2025 at 8:34 PM Jonathan Cameron <jic23@...nel.org> wrote:
> > >
> > > On Mon, 14 Apr 2025 18:42:43 +0000
> > > Lothar Rubusch <l.rubusch@...il.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@...il.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, &regval);
> > > > +             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;
> > > > +     }
> > > > +
>

Powered by blists - more mailing lists