Re: [PATCH v9 08/11] iio: accel: adxl345: add inactivity feature

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 10 Jun 2025 21:59:30 +0000
Lothar Rubusch <l.rubusch@gmail.com> 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@gmail.com>
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;