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

[Lothar Rubusch]

On Mon, Mar 31, 2025 at 12:47 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Tue, 18 Mar 2025 23:08:41 +0000
> Lothar Rubusch <l.rubusch@xxxxxxxxx> 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@xxxxxxxxx>
>
> 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.

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.

> > + *
> > + * 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);
> >  }
> >
>
> > @@ -1546,10 +1697,18 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> >               if (ret)
> >                       return ret;
> >
> > +             ret = regmap_write(st->regmap, ADXL345_REG_TIME_INACT, 3);
> > +             if (ret)
> > +                     return ret;
> > +
> >               ret = regmap_write(st->regmap, ADXL345_REG_THRESH_ACT, 6);
> >               if (ret)
> >                       return ret;
> >
> > +             ret = regmap_write(st->regmap, ADXL345_REG_THRESH_INACT, 4);
>
> Comments on defaults are good.
>
> > +             if (ret)
> > +                     return ret;
> > +
> >               ret = regmap_write(st->regmap, ADXL345_REG_THRESH_TAP, tap_threshold);
> >               if (ret)
> >                       return ret;
>