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);
> + 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.
> + 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
