Message-ID: <CAFXKEHan_7+BVshb12JZLH8CJtSPuwv=H_vC2kUWkS411wsqaA@mail.gmail.com>
Date: Mon, 23 Jun 2025 23:06:44 +0200
From: Lothar Rubusch <l.rubusch@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...el.com>
Cc: lars@...afoo.de, Michael.Hennerich@...log.com, jic23@...nel.org, 
	dlechner@...libre.com, nuno.sa@...log.com, andy@...nel.org, corbet@....net, 
	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-doc@...r.kernel.org, eraretuya@...il.com
Subject: Re: [PATCH v10 4/7] iio: accel: adxl345: add inactivity feature

On Mon, Jun 23, 2025 at 11:44 AM Andy Shevchenko
<andriy.shevchenko@...el.com> wrote:
>
> On Sun, Jun 22, 2025 at 03:50:07PM +0000, Lothar Rubusch wrote:
> > Add support for the sensor’s inactivity feature in the driver. When both
> > activity and inactivity detection are enabled, the sensor sets a link bit
> > that ties the two functions together. This also enables auto-sleep mode,
> > allowing the sensor to automatically enter sleep state upon detecting
> > inactivity.
> >
> > Inactivity detection relies on a configurable threshold and a specified
> > time period. If sensor measurements remain below the threshold for the
> > defined duration, the sensor transitions to the inactivity state.
> >
> > When an Output Data Rate (ODR) is set, the inactivity time period is
> > automatically adjusted to a sensible default. Higher ODRs result in shorter
> > inactivity timeouts, while lower ODRs allow longer durations-within
> > reasonable upper and lower bounds. This is important because features like
> > auto-sleep operate effectively only between 12.5 Hz and 400 Hz. These
> > defaults are applied when the sample rate is modified, but users can
> > override them by explicitly setting a custom inactivity timeout.
> >
> > Similarly, configuring the g-range provides default threshold values for
> > both activity and inactivity detection. These are implicit defaults meant
> > to simplify configuration, but they can also be manually overridden as
> > needed.
>
> ...
>
> > +static int adxl345_set_inact_time(struct adxl345_state *st, u32 val_s)
> > +{
> > +     int max_boundary = U8_MAX;
> > +     int min_boundary = 10;
> > +     unsigned int val = min(val_s, U8_MAX);
>
> Wondering if it's possible to refer here to max_boundary?
> In any case, split this assignment since it will be easier
> to maintain.
>
> > +     enum adxl345_odr odr;
> > +     unsigned int regval;
> > +     int ret;
>
>         val = min(val_s, max_boundary);
>

Well, yes, that's what I had initially. Then min() needed unsigned
int, where clamp() - down below - needed signed int. At the end of the
day, I set up min() here, but later this will disappear. I was
wondering, if it's actually needed anymore, when doing clamp() anyway.

The story is a bit longer, since original version (I think I never
submitted) I started with clamp(), ran into signed / unsigned and
difference from max, that I skipped clamp() until when you complained
about it: "use clamp()"

Long story short, I'll verify this in my tests, but probably I'll
rather drop a call to min() here.

> > +     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 = clamp(max_boundary - adxl345_odr_tbl[odr][0],
> > +                         min_boundary, max_boundary);
> > +     }
> > +
> > +     return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
> > +}
>
> ...
>
> > +     case ADXL345_INACTIVITY:
> > +             en = FIELD_GET(ADXL345_INACT_X_EN, axis_ctrl) |
> > +                     FIELD_GET(ADXL345_INACT_Y_EN, axis_ctrl) |
> > +                     FIELD_GET(ADXL345_INACT_Z_EN, axis_ctrl);
>
> As I pointed out earlier. the indentation is supposed to be on the same colomn
> for 'F' letters.
>

Let me allow a stupid question, when you mean on the same column, the
above is wrong? Can you give me an example here how to fix it?

Best,
L

> > +             if (!en)
> > +                     return false;
> > +             break;
>
> ...
>
> > +                     ret = regmap_read(st->regmap,
> > +                                       ADXL345_REG_TIME_INACT,
> > +                                       &period);
>
> There is plenty of room on the previous lines. Depending on the next
> changes (which I believe unlikely touch this) it may be packed to two
> lines with a logical split, like
>
>                         ret = regmap_read(st->regmap,
>                                           ADXL345_REG_TIME_INACT, &period);
>
> It again seems the byproduct of the too strict settings of the wrap limit in
> your editor.
>
> ...
>
> > +     case ADXL345_INACTIVITY:
> > +             axis_ctrl = ADXL345_INACT_X_EN | ADXL345_INACT_Y_EN |
> > +                             ADXL345_INACT_Z_EN;
>
> Consider
>                 axis_ctrl =
>                         ADXL345_INACT_X_EN | ADXL345_INACT_Y_EN | ADXL345_INACT_Z_EN;
>
> (yes, I see that it's longer than 80, but it might worth doing it for the sake of
>  consistency with the previous suggestion).
>
>
> ...
>
> >  static int adxl345_set_range(struct adxl345_state *st, enum adxl345_range range)
> >  {
> > -     return regmap_update_bits(st->regmap, ADXL345_REG_DATA_FORMAT,
>
> > +     int ret;
> > +
> > +     ret = regmap_update_bits(st->regmap, ADXL345_REG_DATA_FORMAT,
> >                                ADXL345_DATA_FORMAT_RANGE,
> >                                FIELD_PREP(ADXL345_DATA_FORMAT_RANGE, range));
> > +     if (ret)
> > +             return ret;
>
> If it's a code from the previous patch, it might make sense to introduce ret
> there.
>
> >  }
>
> ...
>
> > +     case IIO_EV_INFO_PERIOD:
> > +             ret = regmap_read(st->regmap,
> > +                               ADXL345_REG_TIME_INACT,
> > +                               &period);
>
> Too short lines.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Powered by blists - more mailing lists