Re: [PATCH v5 09/11] iio: accel: adxl345: add inactivity feature

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 14 Apr 2025 15:19:52 +0200
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> On Mon, Mar 31, 2025 at 12:47 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Tue, 18 Mar 2025 23:08:41 +0000
> > Lothar Rubusch <l.rubusch@gmail.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@gmail.com>  
> >
> > A few comments inline.  The magic handling of the value 0 is
> > a bit of unexpected ABI.
> >
> > Jonathan
> >  
> > > @@ -327,6 +358,7 @@ static int adxl345_set_act_inact_en(struct adxl345_state *st,
> > >                                   bool cmd_en)
> > >  {
> > >       bool axis_en, en;
> > > +     unsigned int inact_time_s;
> > >       unsigned int threshold;
> > >       u32 axis_ctrl = 0;
> > >       int ret;
> > > @@ -345,6 +377,20 @@ static int adxl345_set_act_inact_en(struct adxl345_state *st,
> > >               default:
> > >                       return -EINVAL;
> > >               }
> > > +     } else {
> > > +             switch (axis) {
> > > +             case IIO_MOD_X:
> > > +                     axis_ctrl = ADXL345_INACT_X_EN;
> > > +                     break;
> > > +             case IIO_MOD_Y:
> > > +                     axis_ctrl = ADXL345_INACT_Y_EN;
> > > +                     break;
> > > +             case IIO_MOD_Z:
> > > +                     axis_ctrl = ADXL345_INACT_Z_EN;
> > > +                     break;
> > > +             default:
> > > +                     return -EINVAL;
> > > +             }
> > >       }
> > >
> > >       if (cmd_en)
> > > @@ -365,11 +411,67 @@ static int adxl345_set_act_inact_en(struct adxl345_state *st,
> > >       if (type == ADXL345_ACTIVITY) {
> > >               axis_en = FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, axis_ctrl) > 0;
> > >               en = axis_en && threshold > 0;
> > > +     } else {  
> >
> > So previous suggestion on setting en doesn't work but you can still combine
> > the bits other than the type match to simplify code and get rid of axis_en
> > in both paths.
> >  
> > > +             ret = regmap_read(st->regmap, ADXL345_REG_TIME_INACT, &inact_time_s);
> > > +             if (ret)
> > > +                     return ret;
> > > +
> > > +             axis_en = FIELD_GET(ADXL345_REG_INACT_AXIS_MSK, axis_ctrl) > 0;
> > > +             en = axis_en && threshold > 0 && inact_time_s > 0;
> > >       }  
> >  
> > > +/**
> > > + * 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.
> > > + *
> > > + * If val_s is 0, a default inactivity time will be computed. 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.  
> >
> > I'd missed this previously.  I've no problem with a default time being set
> > on driver load, but a later write of 0 should not result in something very different
> > as that's not standard use of the ABI.  If a user wants to go back to a sensible
> > default then they should have stored out what was set initially.
> >
> > I don't mind if you update the default until the point where they first override
> > it, but from there on we should obey what they request or error out if the
> > value requested is not possible.
> >  
> 
> Hm, I'm unsure if I got this wrong. It is not supposed to be an
> automatic feature to kick in and change user configured values,
> actually. Let me try to explain it differently:
> Setting a threshold for an inactivity time in [s] is always applied as
> a user wishes. Setting 0s for inactivity time IMHO does not make much
> sense, where one could also simply disable the sensor event. So, what
> I did now is I implemented when 0s was set by a user for inactivity
> time, it will result in an automatic adjustment of inactivity time,
> depending on range and odr.

This is still a bit unexpected so ABI that is unlikely to be in general
useful.  If userspace can figure out this automatic value I'd rather we
just reject 0 as -EINVAL and let userspace code decide on a better value
given that 'hint'

> 
> In v6 I will try to refrase the text, and double-check it's contained
> in documentation, too. Pls, let me know what you think.
>