Message-ID: <CAFXKEHbPB6vBf6MRTEoQY13p+EPqSeFMP2uPh46BavXFsXTmag@mail.gmail.com>
Date: Tue, 22 Apr 2025 00:11:15 +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 3:54 PM Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Mon, 21 Apr 2025 15:39:33 +0200
> Lothar Rubusch <l.rubusch@...il.com> wrote:
>
> > 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
>
> Ah. I had it in my head it was only one set of per axis enables for the two
> types of event. It's not! So indeed your description is what it should be.
>
> >
> > 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?)
>
> I think so given the separate enables.
>
> >
> > 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
>
> Is it the same one?  I think so but maybe I've lost track.
>
> > - 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)?
> Looks good to me!
>

I fixed a v7 together. Eventually, I added a virtual channel for
inactivity and another one for freefall. Pls, let me know if this is
ok. So far it seems to work out perfectly. Now with the '...x&y&z...'
sysfs handle for enabling them. The docs are updated as well.

Best,
L

> Jonathan
>

Powered by blists - more mailing lists