Message-ID: <20250427140032.3112f51b@jic23-huawei>
Date: Sun, 27 Apr 2025 14:00:32 +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 v7 10/11] iio: accel: adxl345: add coupling detection
 for activity/inactivity

On Mon, 21 Apr 2025 22:06:40 +0000
Lothar Rubusch <l.rubusch@...il.com> wrote:

> Add coupling activity/inactivity detection by the AC/DC bit. This is an
> addititional enhancement for the detection of activity states and
> completes the activity / inactivity feature of the ADXL345.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@...il.com>

I've dragged the table from the earlier patch into this one that actually uses it.
However I'm a little unsure on exactly how we present this feature.

So until those questions are resolved I've dropped the patch (you'll need
to rebase on my testing branch and fix up missing table for v8).

The bit that made me not apply this series (with some tweaks) was that
I'd expect enabling AC events to be visible as disabling of DC ones.

Also, I just noticed you aren't pushing the new event types.

These controls need to look like a separate event detector hardware block
with it's own controls + its own event codes.  The fact only this or
the DC version can be enabled at any time should only be exposed in the
reported state, not apparent via what files we expose etc.  On some
other device they may be independent hardware blocks.

Note I'd also expect to see value controls for these new events. You may
need to cache the values and update on event change if the meaning is
very different.   That's because the expectation would be an event
setup sequence from userspace is:

1) Set value of threshold
2) Enable event

On a change of event (due to shared hardware) The value set may scramble
the event already enabled.

So write the values into a cache and update to the right one when changing
event.

> ---
>  drivers/iio/accel/adxl345_core.c | 162 ++++++++++++++++++++++++++++++-
>  1 file changed, 159 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index b25efcad069b..c07ad5774c8a 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -37,7 +37,9 @@
>  #define ADXL345_REG_TAP_SUPPRESS_MSK	BIT(3)
>  #define ADXL345_REG_TAP_SUPPRESS	BIT(3)
>  #define ADXL345_REG_ACT_AXIS_MSK	GENMASK(6, 4)
> +#define ADXL345_REG_ACT_ACDC_MSK	BIT(7)
>  #define ADXL345_REG_INACT_AXIS_MSK	GENMASK(2, 0)
> +#define ADXL345_REG_INACT_ACDC_MSK	BIT(3)
>  #define ADXL345_POWER_CTL_INACT_MSK	(ADXL345_POWER_CTL_AUTO_SLEEP | ADXL345_POWER_CTL_LINK)
>  
>  #define ADXL345_TAP_Z_EN		BIT(0)
> @@ -91,6 +93,11 @@ static const unsigned int adxl345_act_thresh_reg[] = {
>  	[ADXL345_INACTIVITY] = ADXL345_REG_THRESH_INACT,
>  };
>  
> +static const unsigned int adxl345_act_acdc_msk[] = {
> +	[ADXL345_ACTIVITY] = ADXL345_REG_ACT_ACDC_MSK,
> +	[ADXL345_INACTIVITY] = ADXL345_REG_INACT_ACDC_MSK,
> +};
> +
>  enum adxl345_odr {
>  	ADXL345_ODR_0P10HZ = 0,
>  	ADXL345_ODR_0P20HZ,
> @@ -204,6 +211,18 @@ static struct iio_event_spec adxl345_events[] = {
>  			BIT(IIO_EV_INFO_RESET_TIMEOUT) |
>  			BIT(IIO_EV_INFO_TAP2_MIN_DELAY),
>  	},
> +	{
> +		/* activity, activity - ac bit */
Comment says activity and inactivity but channel type wise this
is just activity (as rising)

> +		.type = IIO_EV_TYPE_MAG_REFERENCED,
> +		.dir = IIO_EV_DIR_RISING,
> +		.mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE),
> +	},
> +	{
> +		/* activity, inactivity - ac bit */

Likewise this seems to be inactivity.  Should this be in the x&y&z
channel, not this one?

> +		.type = IIO_EV_TYPE_MAG_REFERENCED,
> +		.dir = IIO_EV_DIR_FALLING,
> +		.mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE),
> +	},
>  };
>  
>  #define ADXL345_CHANNEL(index, reg, axis) {					\
> @@ -320,6 +339,69 @@ static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
>  
>  /* act/inact */
>  
> +static int adxl345_is_act_inact_ac(struct adxl345_state *st,
> +				   enum adxl345_activity_type type, bool *ac)
> +{
> +	unsigned int regval;
> +	int ret;
> +
> +	ret = regmap_read(st->regmap, ADXL345_REG_ACT_INACT_CTRL, &regval);
> +	if (ret)
> +		return ret;
> +
> +	if (type == ADXL345_ACTIVITY)
> +		*ac = FIELD_GET(ADXL345_REG_ACT_ACDC_MSK, regval);
> +	else
> +		*ac = FIELD_GET(ADXL345_REG_INACT_ACDC_MSK, regval);
> +
> +	return 0;
> +}
> +
> +static int adxl345_set_act_inact_ac(struct adxl345_state *st,
> +				    enum adxl345_activity_type type, bool ac)
> +{
> +	unsigned int act_inact_ac = ac ? 0xff : 0x00;
> +
> +	/*
> +	 * A setting of false selects dc-coupled operation, and a setting of
> +	 * true enables ac-coupled operation. In dc-coupled operation, the
> +	 * current acceleration magnitude is compared directly with
> +	 * ADXL345_REG_THRESH_ACT and ADXL345_REG_THRESH_INACT to determine
> +	 * whether activity or inactivity is detected.
> +	 *
> +	 * In ac-coupled operation for activity detection, the acceleration
> +	 * value at the start of activity detection is taken as a reference
> +	 * value. New samples of acceleration are then compared to this
> +	 * reference value, and if the magnitude of the difference exceeds the
> +	 * ADXL345_REG_THRESH_ACT value, the device triggers an activity
> +	 * interrupt.
> +	 *
> +	 * Similarly, in ac-coupled operation for inactivity detection, a
> +	 * reference value is used for comparison and is updated whenever the
> +	 * device exceeds the inactivity threshold. After the reference value
> +	 * is selected, the device compares the magnitude of the difference
> +	 * between the reference value and the current acceleration with
> +	 * ADXL345_REG_THRESH_INACT. If the difference is less than the value in
> +	 * ADXL345_REG_THRESH_INACT for the time in ADXL345_REG_TIME_INACT, the
> +	 * device is considered inactive and the inactivity interrupt is
> +	 * triggered. [quoted from p. 24, ADXL345 datasheet Rev. G]
> +	 *
> +	 * In a conclusion, the first acceleration snapshot sample which hit the
> +	 * threshold in a particular direction is always taken as acceleration
> +	 * reference value to that direction. Since for the hardware activity
> +	 * and inactivity depend on the x/y/z axis, so do ac and dc coupling.
> +	 * Note, this sw driver always enables or disables all three x/y/z axis
> +	 * for detection via act_axis_ctrl and inact_axis_ctrl, respectively.
> +	 * Where in dc-coupling samples are compared against the thresholds, in
> +	 * ac-coupling measurement difference to the first acceleration
> +	 * reference value are compared against the threshold. So, ac-coupling
> +	 * allows for a bit more dynamic compensation depending on the initial
> +	 * sample.
> +	 */
> +	return regmap_update_bits(st->regmap, ADXL345_REG_ACT_INACT_CTRL,
> +				 adxl345_act_acdc_msk[type], act_inact_ac);
> +}

>  static int adxl345_find_range(struct adxl345_state *st, int val, int val2,
> @@ -797,9 +886,51 @@ static int adxl345_find_range(struct adxl345_state *st, int val, int val2,
>  
>  static int adxl345_set_range(struct adxl345_state *st, enum adxl345_range range)
>  {
> -	return regmap_update_bits(st->regmap, ADXL345_REG_DATA_FORMAT,
> +	unsigned int act_threshold, inact_threshold;
> +	unsigned int range_old;
> +	unsigned int regval;
> +	int ret;
> +
> +	ret = regmap_read(st->regmap, ADXL345_REG_DATA_FORMAT, &regval);
> +	if (ret)
> +		return ret;
> +	range_old = FIELD_GET(ADXL345_DATA_FORMAT_RANGE, regval);
> +
> +	ret = regmap_read(st->regmap,
> +			  adxl345_act_thresh_reg[ADXL345_ACTIVITY],
> +			  &act_threshold);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(st->regmap,
> +			  adxl345_act_thresh_reg[ADXL345_INACTIVITY],
> +			  &inact_threshold);
> +	if (ret)
> +		return 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;
> +
> +	act_threshold = act_threshold
> +		* adxl345_range_factor_tbl[range_old]
> +		/ adxl345_range_factor_tbl[range];
> +	act_threshold = min(255, max(1, inact_threshold));
> +
This is first use of the range table. So introduce that in this patch.

> +	inact_threshold = inact_threshold
> +		* adxl345_range_factor_tbl[range_old]
> +		/ adxl345_range_factor_tbl[range];
> +	inact_threshold = min(255, max(1, inact_threshold));
> +
> +	ret = regmap_write(st->regmap, adxl345_act_thresh_reg[ADXL345_ACTIVITY],
> +			   act_threshold);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(st->regmap, adxl345_act_thresh_reg[ADXL345_INACTIVITY],
> +			   inact_threshold);
>  }
>  
>  static int adxl345_read_avail(struct iio_dev *indio_dev,
> @@ -938,7 +1069,7 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
>  				     enum iio_event_direction dir)
>  {
>  	struct adxl345_state *st = iio_priv(indio_dev);
> -	bool int_en;
> +	bool int_en, act_ac, inact_ac;
>  	int ret;
>  
>  	switch (type) {
> @@ -983,6 +1114,21 @@ static int adxl345_read_event_config(struct iio_dev *indio_dev,
>  		if (ret)
>  			return ret;
>  		return int_en;
> +	case IIO_EV_TYPE_MAG_REFERENCED:
> +		switch (dir) {
> +		case IIO_EV_DIR_RISING:
> +			ret = adxl345_is_act_inact_ac(st, ADXL345_ACTIVITY, &act_ac);

Do we not need a check in the enabling of the DC events as well?  If we have enabled
AC the DC one should report disabled (and if we enable that again then we should
update this.

> +			if (ret)
> +				return ret;
> +			return act_ac;
> +		case IIO_EV_DIR_FALLING:
> +			ret = adxl345_is_act_inact_ac(st, ADXL345_INACTIVITY, &inact_ac);
> +			if (ret)
> +				return ret;
> +			return inact_ac;
> +		default:
> +			return -EINVAL;
> +		}
>  	default:
>  		return -EINVAL;
>  	}
> @@ -1019,6 +1165,16 @@ static int adxl345_write_event_config(struct iio_dev *indio_dev,
>  		}
>  	case IIO_EV_TYPE_MAG:
>  		return adxl345_set_ff_en(st, state);
> +	case IIO_EV_TYPE_MAG_REFERENCED:
> +		switch (dir) {
> +		case IIO_EV_DIR_RISING:
> +			return adxl345_set_act_inact_ac(st, ADXL345_ACTIVITY, state);

Similar to read path.  The DC events should be affected by this as well as the AC ones.

> +		case IIO_EV_DIR_FALLING:
> +			return adxl345_set_act_inact_ac(st, ADXL345_INACTIVITY, state);
> +		default:
> +			return -EINVAL;
> +		}
> +
>  	default:
>  		return -EINVAL;
>  	}

Powered by blists - more mailing lists