Message-ID: <20250421112228.453dfa89@jic23-huawei>
Date: Mon, 21 Apr 2025 11:22:28 +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 v6 09/11] iio: accel: adxl345: add inactivity feature

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.

> 
> >
> >  
> > > +                                                   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