Re: [PATCH v5 08/11] iio: accel: adxl345: add activity event feature

[Lothar Rubusch <l.rubusch@gmail.com>]

On Mon, Mar 31, 2025 at 12:40 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 18 Mar 2025 23:08:40 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Make the sensor detect and issue interrupts at activity. Activity
> > events are configured by a threshold stored in regmap cache. Initialize
> > the activity threshold register to a reasonable default value in probe.
> > The value is taken from the older ADXL345 input driver, to provide a
> > similar behavior. Reset the activity/inactivity direction enabling
> > register in probe. Reset and initialization shall bring the sensor in a
> > defined initial state to prevent dangling settings when warm restarting
> > the sensor.
> >
> > Activity, ODR configuration together with the range setting prepare the
> > activity/inactivity hystersesis setup, implemented in a follow up patch.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
>
> > +static int adxl345_is_act_inact_en(struct adxl345_state *st,
> > +                                enum iio_modifier axis,
> > +                                enum adxl345_activity_type type, bool *en)
> > +{
> > +     unsigned int regval;
> > +     bool axis_en;
> > +     u32 axis_ctrl;
> > +     int ret;
> > +
> > +     ret = regmap_read(st->regmap, ADXL345_REG_ACT_INACT_CTRL, &axis_ctrl);
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (type == ADXL345_ACTIVITY) {
> > +             switch (axis) {
> > +             case IIO_MOD_X:
> > +                     axis_en = FIELD_GET(ADXL345_ACT_X_EN, axis_ctrl);
> > +                     break;
> > +             case IIO_MOD_Y:
> > +                     axis_en = FIELD_GET(ADXL345_ACT_Y_EN, axis_ctrl);
> > +                     break;
> > +             case IIO_MOD_Z:
> > +                     axis_en = FIELD_GET(ADXL345_ACT_Z_EN, axis_ctrl);
> Same as in earlier patch; axis_en is never used.
> > +                     break;
> > +             default:
> > +                     return -EINVAL;
> > +             }
> > +     }
> > +
> > +     ret = regmap_read(st->regmap, ADXL345_REG_INT_ENABLE, &regval);
> > +     if (ret)
> > +             return ret;
> > +
> > +     *en = (adxl345_act_int_reg[type] & regval) > 0;
> > +
> > +     return 0;
> > +}
> > +
> > +static int adxl345_set_act_inact_en(struct adxl345_state *st,
> > +                                 enum iio_modifier axis,
> > +                                 enum adxl345_activity_type type,
> > +                                 bool cmd_en)
> > +{
> > +     bool axis_en, en;
> > +     unsigned int threshold;
> > +     u32 axis_ctrl = 0;
> > +     int ret;
> > +
> > +     if (type == ADXL345_ACTIVITY) {
> > +             switch (axis) {
> > +             case IIO_MOD_X:
> > +                     axis_ctrl = ADXL345_ACT_X_EN;
> > +                     break;
> > +             case IIO_MOD_Y:
> > +                     axis_ctrl = ADXL345_ACT_Y_EN;
> > +                     break;
> > +             case IIO_MOD_Z:
> > +                     axis_ctrl = ADXL345_ACT_Z_EN;
> > +                     break;
> > +             default:
> > +                     return -EINVAL;
> > +             }
> > +     }
> > +
> > +     if (cmd_en)
> > +             ret = regmap_set_bits(st->regmap,
> > +                                   ADXL345_REG_ACT_INACT_CTRL, axis_ctrl);
> > +     else
> > +             ret = regmap_clear_bits(st->regmap,
> > +                                     ADXL345_REG_ACT_INACT_CTRL, axis_ctrl);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = regmap_read(st->regmap, adxl345_act_thresh_reg[type], &threshold);
> > +     if (ret)
> > +             return ret;
> > +
> > +     en = false;
> > +
> > +     if (type == ADXL345_ACTIVITY) {
> > +             axis_en = FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, axis_ctrl) > 0;
> The > 0 doesn't add anything as this can't be negative.
>
> Drag declaration of axis_en down here as only used in this block.
> or just combine with previous and next bit as
>                 en = (type === ADXL345_ACTIVITY) &&
>                      FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, axis_ctrl) &&
>                      (threshold > 0);
>
> > +             en = axis_en && threshold > 0;
> > +     }
> > +
> > +     return regmap_update_bits(st->regmap, ADXL345_REG_INT_ENABLE,
> > +                               adxl345_act_int_reg[type],
> > +                               en ? adxl345_act_int_reg[type] : 0);
> > +}
> > +
> >  /* tap */
> >
>
>
>
> > @@ -1347,6 +1542,14 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> >               if (ret)
> >                       return ret;
> >
> > +             ret = regmap_write(st->regmap, ADXL345_REG_ACT_INACT_CTRL, 0);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             ret = regmap_write(st->regmap, ADXL345_REG_THRESH_ACT, 6);
>
> 6 is a fairly random number. Add a comment for why this default.
>

My general intention is to provide +/- reasonable default configs,
rather than leave it all to 0 or undefined, to allow to turn the event
on and already catch at least something. In many cases those
will be the default values from the older input driver, to keep a bit
of a compatibility.
Particular cases have actually recommendations in the datasheet and
differ slightly to the input
driver. I'm aware that the input driver probably is not a golden
standard, but it worked at least for
my tests. I may leave a general comment on the section, pls have a
look into v6 if this is ok.

> > +             if (ret)
> > +                     return ret;
> > +
> >               ret = regmap_write(st->regmap, ADXL345_REG_THRESH_TAP, tap_threshold);
> >               if (ret)
> >                       return ret;
>