On Mon, Jun 23, 2025 at 11:44 AM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxx> 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, ®val);
> > + 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
>
>
