Re: [PATCH v3 12/15] iio: accel: adxl345: add activity event feature

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

Hi Jonathan,

I'm currently about to update the series and like to answer some of
your review comments directly when submitting. Nevertheless, here I'll
anticipate this one. Pls, find my questions inlined down below.

On Tue, Mar 4, 2025 at 2:49 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu, 20 Feb 2025 10:42:31 +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.
> >
> > Activity, together with ODR and range setting are preparing a setup
> > together with inactivity coming in a follow up patch.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> Some questions / comments inline.
>
> This review is has been at random moments whilst travelling (hence
> over several days!) so it may be less than thorough or consistent!
> I should be back to normal sometime next week.
>

No problem, no hurry!

> > @@ -258,6 +284,73 @@ static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
> >       return regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
> >  }
> >
> > +/* act/inact */
> > +
> > +static int adxl345_write_act_axis(struct adxl345_state *st,
> > +                               enum adxl345_activity_type type, bool en)
> > +{
> > +     int ret;
> > +
> > +     /*
> > +      * The ADXL345 allows for individually enabling/disabling axis for
> > +      * activity and inactivity detection, respectively. Here both axis are
> > +      * kept in sync, i.e. an axis will be generally enabled or disabled for
> > +      * both equally, activity and inactivity detection.
>
> Why?  Can definitely see people only being interested in one case
> and not the other.  What advantage is there in always having both
> or neither over separate controls?

Ugh! This happens when I mix writing code and writing English texts,
w/o re-reading it.

Situation: The sensor allows to individually enable / disable x,y and
z axis for r activity and for inactivity. I don't offer this in the
driver. When activity is selected, I always enable all three axis or
disable them. Similar, for inactivity. The question is then actually,
if this is legitimate, or should I really implement enabling/disabling
of each axis individually for activity and similar then for
inactivity? I mean, when not interested in one or the other axis,
someone can fiilter the result. On the other side I can imagine a
small impact in power consumption, when only one axis is used instead
of three axis (?). Since the ADXL345 is [was] one of Analog's fancy
acme-ultra-low-power-sensors, power is definitvely a topic here IMHO.
I can't really estimate the importance.

I guess I'll try to implement it and see how ugly it gets. At least
it's a good exercise. As also, I'll try to bring regmap cache and
clear_bits / set_bits more in here for activity and inactivity in the
next version.

>
> > +      */
> > +     if (type == ADXL345_ACTIVITY) {
> > +             st->act_axis_ctrl = en
> > +                     ? st->act_axis_ctrl | ADXL345_REG_ACT_AXIS_MSK
> > +                     : st->act_axis_ctrl & ~ADXL345_REG_ACT_AXIS_MSK;
> > +
> > +             ret = regmap_update_bits(st->regmap, ADXL345_REG_ACT_INACT_CTRL,
> > +                                      adxl345_act_axis_msk[type],
> > +                                      st->act_axis_ctrl);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +     return 0;
> > +}
>
>
> > +static int adxl345_set_act_inact_en(struct adxl345_state *st,
> > +                                 enum adxl345_activity_type type, bool cmd_en)
> > +{
> > +     bool axis_en, en = false;
> I'm not keen on mix of set and unset in a declaration line.  Better to
> use two lines here as it can be hard to spot when things are or aren't
> initialized when that is not the intent.
>
>         bool en = false;
>         bool axis_en;
>
> > +     unsigned int threshold;
> > +     int ret;
> > +
> > +     ret = adxl345_write_act_axis(st, type, cmd_en);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = regmap_read(st->regmap, adxl345_act_thresh_reg[type], &threshold);
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (type == ADXL345_ACTIVITY) {
> > +             axis_en = FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, st->act_axis_ctrl) > 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);
> > +}
> > +
>
> > @@ -842,6 +972,23 @@ static int adxl345_write_event_value(struct iio_dev *indio_dev,
> >               return ret;
> >
> >       switch (type) {
> > +     case IIO_EV_TYPE_THRESH:
> > +             switch (info) {
> > +             case IIO_EV_INFO_VALUE:
> > +                     switch (dir) {
> > +                     case IIO_EV_DIR_RISING:
> > +                             ret = regmap_write(st->regmap,
> > +                                                adxl345_act_thresh_reg[ADXL345_ACTIVITY],
> > +                                                val);
> > +                             break;
> This collection of breaks and nested functions suggests maybe we can either
> return directly (I've lost track of what happens after this) or that
> we should factor out some of this code to allow direct returns in the
> function we put that code in.  Chasing the breaks is not great if it
> doesn't lead to anything interesting.

I understand, but since I'm using quite a bit configuration for the
sensor, I'm taking advantage
of type, info and dir here. It won't get more complex than that. I'm
[actually] pretty sure, since this
then is almost feature complete.

I don't see a different way how to do it. I mean, I could still
separate handling the "dir" entirely in
a called function. Or, say, implement IIO_EV_TYPE_THRESH handling in a
separate function?
Pls, let me know what you think.

> > +                     default:
> > +                             ret = -EINVAL;
> > +                     }
> > +                     break;
> > +             default:
> > +                     ret = -EINVAL;
> > +             }
> > +             break;
> >       case IIO_EV_TYPE_GESTURE:
> >               switch (info) {
> >               case IIO_EV_INFO_VALUE:
> > @@ -1124,6 +1271,17 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
> >                       return ret;
> >       }
> >
> > +     if (FIELD_GET(ADXL345_INT_ACTIVITY, int_stat)) {
> > +             ret = iio_push_event(indio_dev,
> > +                                  IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> > +                                                     act_tap_dir,
> > +                                                     IIO_EV_TYPE_THRESH,
> > +                                                     IIO_EV_DIR_RISING),
> > +                                  ts);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> >       if (FIELD_GET(ADXL345_INT_FREE_FALL, int_stat)) {
> >               ret = iio_push_event(indio_dev,
> >                                    IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> > @@ -1157,6 +1315,7 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
> >               ret = regmap_read(st->regmap, ADXL345_REG_ACT_TAP_STATUS, &regval);
> >               if (ret)
> >                       return ret;
> > +             /* tap direction */
>
> Belongs in earlier patch?
>
> >               if (FIELD_GET(ADXL345_Z_EN, regval) > 0)
> >                       act_tap_dir = IIO_MOD_Z;
> >               else if (FIELD_GET(ADXL345_Y_EN, regval) > 0)
> > @@ -1165,6 +1324,19 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
> >                       act_tap_dir = IIO_MOD_X;
> >       }
> >
> > +     if (FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, st->act_axis_ctrl) > 0) {
> > +             ret = regmap_read(st->regmap, ADXL345_REG_ACT_TAP_STATUS, &regval);
> > +             if (ret)
> > +                     return ret;
> > +             /* activity direction */
> > +             if (FIELD_GET(ADXL345_Z_EN, regval >> 4) > 0)
>
> I'm not following the shifts here.   That looks like we don't have
> defines that we should but instead use the ones for the lower fields.

The 8-bit register is split as follows:

| 7                |  6         |  5          |  4         |  3
          |  2              |  1             |  0             |
----------------------------------------------------------------------------------------------------------------------
| ACT ac/dc | ACT_X | ACT_Y | ACT_Z | INACT ac/dc | INACT_X | INACT_Y
| INACT_Z |

I thought here, either I shift the ACT_* directions by 4 then use the
general mask for axis (lower 4 bits). Or I introduce an axis enum for
ACT_* and a separate one for INACT_*. Thus, I kept the shift and use
the same ADXL345_*_EN mask. How can I improve this, or can this stay?

>
> > +                     act_tap_dir = IIO_MOD_Z;
> > +             else if (FIELD_GET(ADXL345_Y_EN, regval >> 4) > 0)
> > +                     act_tap_dir = IIO_MOD_Y;
> > +             else if (FIELD_GET(ADXL345_X_EN, regval >> 4) > 0)
> > +                     act_tap_dir = IIO_MOD_X;
> > +     }
>
>

Best,
L