On Tue, 10 Jun 2025 21:59:30 +0000
Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote:
> Add the inactivity feature of the sensor to the driver. When activity
> and inactivity are enabled, a link bit will be set linking activity and
> inactivity handling. Additionally, the auto-sleep mode will be enabled.
> Due to the link bit the sensor is going to auto-sleep when inactivity
> was detected.
>
> Inactivity detection needs a threshold to be configured and a period of
> time in seconds. After, it will transition to inactivity state, if
> measurements stay below inactivity threshold.
>
> When a ODR is configured the period for inactivity is adjusted with a
> corresponding reasonable default value, in order to have higher
> frequencies, lower inactivity times, and lower sample frequency but
> give more time until inactivity. Both with reasonable upper and lower
> boundaries, since many of the sensor's features (e.g. auto-sleep) will
> need to operate between 12.5 Hz and 400 Hz. This is a default setting
> when actively changing sample frequency, explicitly setting the time
> until inactivity will overwrite the default.
>
> Similarly, setting the g-range will provide a default value for the
> activity and inactivity thresholds. Both are implicit defaults, but
> equally can be overwritten to be explicitly configured.
>
> Signed-off-by: Lothar Rubusch <l.rubusch@xxxxxxxxx>
Just a few trivial comments from me.
J
> ---
> drivers/iio/accel/adxl345_core.c | 238 +++++++++++++++++++++++++++++--
> 1 file changed, 227 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 04b9f155872f..1670315ebd63 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -334,12 +423,37 @@ static int adxl345_set_act_inact_en(struct adxl345_state *st,
> en = FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, axis_ctrl) &&
> threshold;
> break;
> + case ADXL345_INACTIVITY:
> + ret = regmap_read(st->regmap, ADXL345_REG_TIME_INACT, &inact_time_s);
> + if (ret)
> + return ret;
> +
> + en = FIELD_GET(ADXL345_REG_INACT_AXIS_MSK, axis_ctrl) &&
> + threshold && inact_time_s;
> + break;
> default:
> return -EINVAL;
> }
>
> ret = regmap_assign_bits(st->regmap, ADXL345_REG_INT_ENABLE,
> - adxl345_act_int_reg[type], cmd_en && en);
> + adxl345_act_int_reg[type], cmd_en);
> + if (ret)
> + return ret;
> +
> + /* Set sleep and link bit when ACT and INACT are enabled. */
> + act_en = adxl345_is_act_inact_en(st, ADXL345_ACTIVITY);
> + if (act_en < 0)
> + return act_en;
> +
> + inact_en = adxl345_is_act_inact_en(st, ADXL345_INACTIVITY);
> + if (inact_en < 0)
> + return inact_en;
> +
> + en = en && act_en && inact_en;
> +
For consistency with above, I'd drop this blank line.
> + ret = regmap_assign_bits(st->regmap, ADXL345_REG_POWER_CTL,
> + (ADXL345_POWER_CTL_AUTO_SLEEP | ADXL345_POWER_CTL_LINK),
Not sure that inner set of brackets are adding anything much to readability.
> + en);
> if (ret)
> return ret;
>
> @@ -887,19 +1072,31 @@ static int adxl345_write_event_value(struct iio_dev *indio_dev,
> case IIO_EV_TYPE_MAG:
> switch (info) {
> case IIO_EV_INFO_VALUE:
> + val = DIV_ROUND_CLOSEST(val * MICRO + val2, 62500);
Put it here in the earlier patch. Though that might not makes sense when
you use some more helpers as Andy suggested. No one really likes 3 levels
of switch!
> switch (dir) {
> case IIO_EV_DIR_RISING:
> - val = DIV_ROUND_CLOSEST(val * MICRO + val2, 62500);
> ret = regmap_write(st->regmap,
> adxl345_act_thresh_reg[ADXL345_ACTIVITY],
> val);
> if (ret)
> return ret;
> break;
> + case IIO_EV_DIR_FALLING:
> + ret = regmap_write(st->regmap,
> + adxl345_act_thresh_reg[ADXL345_INACTIVITY],
> + val);
> + if (ret)
> + return ret;
> + break;
