Message-ID: <20250418193411.406bd974@jic23-huawei>
Date: Fri, 18 Apr 2025 19:34:11 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Lothar Rubusch <l.rubusch@...il.com>
Cc: lars@...afoo.de, Michael.Hennerich@...log.com,
 linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
 eraretuya@...il.com
Subject: Re: [PATCH v6 09/11] iio: accel: adxl345: add inactivity feature

On Mon, 14 Apr 2025 18:42:43 +0000
Lothar Rubusch <l.rubusch@...il.com> wrote:

> Add the inactivity feature of the sensor. 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 time
> after which it will go into inactivity state if measurements under
> threshold.
> 
> When a ODR is configured this time for inactivity is adjusted with a
> corresponding reasonable default value, in order to have higher
> frequencies and 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 beween 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@...il.com>
Hi Lothar,

Patches 6-8 look good to me.

This runs into a similar issue to the freefall one. I haven't dug into
the datasheet but does it report on one channel going inactive, or
all being inactive at the same time?  I checked and it is the all
case so we should be both on a pseudo channel to describe it right
and reporting IIO_MOD_X_AND_Y_AND_Z not the OR form.

Sorry again that I'm only realising this on v6 :(

Difference is for Activity the definition is:
"The activity bit is set when acceleration greater than the value
stored in the THRESH_ACT register (Address 0x24) is experienced
on _any_ participating axis, set by the ACT_INACT_CTL register
(Address 0x27)."
vs Inactivity:
"The inactivity bit is set when acceleration of less than the value
stored in the THRESH_INACT register (Address 0x25) is experienced
for more time than is specified in the TIME_INACT
register (Address 0x26) on _all_ participating axes, as set by the
ACT_INACT_CTL register (Address 0x27). "

So all vs any.

> +
> +/**
> + * adxl345_set_inact_time_s - Configure inactivity time explicitly or by ODR.
> + * @st: The sensor state instance.
> + * @val_s: A desired time value, between 0 and 255.
> + *
> + * Inactivity time can be configured between 1 and 255 sec. If a val_s of 0
> + * 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
> + * shall be shorter for higher frequencies and longer for lower frequencies.
> + * Hence, frequencies above 255 Hz shall default to 10 s and frequencies below
> + * 10 Hz shall result in 255 s to detect inactivity.
> + *
> + * The approach simply subtracts the pre-decimal figure of the configured
> + * sample frequency from 255 s to compute inactivity time [s]. Sub-Hz are thus
> + * ignored in this estimation. The recommended ODRs for various features
> + * (activity/inactivity, sleep modes, free fall, etc.) lie between 12.5 Hz and
> + * 400 Hz, thus higher or lower frequencies will result in the boundary
> + * defaults or need to be explicitly specified via val_s.
> + *
> + * Return: 0 or error value.
> + */
> +static int adxl345_set_inact_time_s(struct adxl345_state *st, u32 val_s)
> +{
> +	unsigned int max_boundary = 255;
> +	unsigned int min_boundary = 10;
> +	unsigned int val = min(val_s, max_boundary);
> +	enum adxl345_odr odr;
> +	unsigned int regval;
> +	int ret;
> +
> +	if (val == 0) {
> +		ret = regmap_read(st->regmap, ADXL345_REG_BW_RATE, &regval);
> +		if (ret)
> +			return ret;
> +		odr = FIELD_GET(ADXL345_BW_RATE_MSK, regval);
> +
> +		val = (adxl345_odr_tbl[odr][0] > max_boundary)
> +			? min_boundary : max_boundary -	adxl345_odr_tbl[odr][0];
> +	}
> +
> +	return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
>  }
>  
>  /* tap */
> @@ -837,6 +943,13 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
>  			if (ret)
>  				return ret;
>  			return int_en;
> +		case IIO_EV_DIR_FALLING:
> +			ret = adxl345_is_act_inact_en(st, chan->channel2,

Does it makes sense to allow inactivity detection on a subset of channels but then
report it as XYZ?  I guess it didn't matter when it was and OR, but if we
change to AND as suggested that is going to be misleading.

we might have to allow separate enables but report an event as the combination
of channels that are enabled X_AND_Y, X_AND_Z etc  I guess we can improve activity
channel case as well by doing that with the X_OR_Y etc



> +						      ADXL345_INACTIVITY,
> +						      &int_en);
> +			if (ret)
> +				return ret;
> +			return int_en;
>  		default:
>  			return -EINVAL;
>  		}
> @@ -881,6 +994,9 @@ static int adxl345_write_event_config(struct iio_dev *indio_dev,
>  		case IIO_EV_DIR_RISING:
>  			return adxl345_set_act_inact_en(st, chan->channel2,
>  							ADXL345_ACTIVITY, state);
> +		case IIO_EV_DIR_FALLING:
> +			return adxl345_set_act_inact_en(st, chan->channel2,
> +							ADXL345_INACTIVITY, state);
>  		default:
>  			return -EINVAL;
>  		}

> @@ -1314,6 +1458,17 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
>  			return ret;
>  	}
>  
> +	if (FIELD_GET(ADXL345_INT_INACTIVITY, int_stat)) {
> +		ret = iio_push_event(indio_dev,
> +				     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> +							IIO_MOD_X_OR_Y_OR_Z,

So this is our open question. Similar to the free fall case. Do we have the boolean
logic right way around?

> +							IIO_EV_TYPE_THRESH,
> +							IIO_EV_DIR_FALLING),
> +				     ts);
> +		if (ret)
> +			return ret;
> +	}
> +

Powered by blists - more mailing lists