Re: [PATCH v9 10/11] iio: accel: adxl345: extend inactivity time for less than 1s

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 10 Jun 2025 21:59:32 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Inactivity event and free-fall event are actually the same type of sensor
> events. Hence, inactivity detection using a period of 1 to 255s can be
> extended to be used for lower periods than 1s for covering free-fall
> detection.
> 
> When lower periods are defined, the driver automatically will setup
> threshold and period on the free-fall register, while using the
> inactivity threshold and period for periods above 1s. Using the
> free-fall register, no link bit will be set, and therefore no auto-sleep
> can be set in cases where also activity will be enabled.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>

Approach seems reasonable.  One comment on the code below.

>  /**
>   * adxl345_set_inact_time - Configure inactivity time explicitly or by ODR.
>   * @st: The sensor state instance.
> - * @val_s: A desired time value, between 0 and 255.
> + * @val_int: The inactivity time, integer part.
> + * @val_fract: The inactivity time, fractional part when val_int is 0.
>   *
> - * Inactivity time can be configured between 1 and 255 sec. If a val_s of 0
> + * Inactivity time can be configured between 1 and 255 sec. If a val_s of 0.00
>   * is configured by a user, then a default inactivity time will be computed.
>   *
>   * In such case, it should take power consumption into consideration. Thus it
> @@ -355,16 +381,18 @@ static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
>   *
>   * Return: 0 or error value.
>   */
> -static int adxl345_set_inact_time(struct adxl345_state *st, u32 val_s)
> +static int adxl345_set_inact_time(struct adxl345_state *st, u32 val_int,
> +				  u32 val_fract)
>  {
>  	unsigned int max_boundary = 255;
>  	unsigned int min_boundary = 10;
> -	unsigned int val = min(val_s, max_boundary);
> +	unsigned int val;
>  	enum adxl345_odr odr;
>  	unsigned int regval;
>  	int ret;
>  
> -	if (val == 0) {
> +	if (val_int == 0 && val_fract == 0) {
> +		/* Generated inactivity time based on ODR */
>  		ret = regmap_read(st->regmap, ADXL345_REG_BW_RATE, &regval);
>  		if (ret)
>  			return ret;
> @@ -373,9 +401,41 @@ static int adxl345_set_inact_time(struct adxl345_state *st, u32 val_s)
>  
>  		val = (adxl345_odr_tbl[odr][0] > max_boundary)
>  			? min_boundary : max_boundary -	adxl345_odr_tbl[odr][0];
> +
> +		st->inact_time_ms = MILLI * val;
> +
> +		/* Inactivity time in s */
> +		ret = regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
> +		if (ret)
> +			return ret;
> +

Perhaps return in the good path here.

> +	} else if (val_int == 0 && val_fract > 0) {
> +		/* time < 1s, free-fall */
> +
> +		/*
> +		 * Datasheet max. value is 255 * 5000 us = 1.275000 seconds.
> +		 *
> +		 * Recommended values between 100ms and 350ms (0x14 to 0x46)
> +		 */
> +		st->inact_time_ms = DIV_ROUND_UP(val_fract, MILLI);
> +
> +		ret = regmap_write(st->regmap, ADXL345_REG_TIME_FF,
> +				   DIV_ROUND_CLOSEST(val_fract, 5));
> +		if (ret)
> +			return ret;
and here

> +	} else if (val_int > 0) {
> +		/* Time >= 1s, inactivity */
> +		st->inact_time_ms = MILLI * val_int;
> +
> +		ret = regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val_int);
> +		if (ret)
> +			return ret;
and here.

> +	} else {
Or given this covers any case where val_int > 0 do it first
and error out early?

> +		/* Do not support negative or wrong input. */
> +		return -EINVAL;
>  	}
>  
> -	return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
> +	return 0;
>  }