Re: [PATCH v1 11/12] iio: accel: adxl345: add activity feature

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 28 Jan 2025 12:00:59 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add the handling of activity events, also add sysfs entries to
> configure threshold values to trigger the event. Allow to push the
> event over to the iio channel.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>

I was going to guess these were rate of change detectors or ones
at least slightly compensating for g, but superficially
looks like a straight forward rising mag threshold.  Not seeing need
for new ABI.

We've had those interfaces with accelerometers for a long time.
Key is to map the pretty names in the datasheet into what is actually
being detected. That allows for a lot better generalization across manufacturers.

> ---
>  drivers/iio/accel/adxl345_core.c | 158 ++++++++++++++++++++++++++++++-
>  1 file changed, 154 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 62d75d28b6fc..94c3ad818ba5 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -121,6 +121,8 @@
>  
>  #define ADXL345_REG_TAP_AXIS_MSK	GENMASK(2, 0)
>  #define ADXL345_REG_TAP_SUPPRESS_MSK	BIT(3)
> +#define ADXL345_REG_ACT_AXIS_MSK	GENMASK(6, 4)
> +#define ADXL345_REG_ACT_ACDC_MSK	BIT(7)
>  
>  enum adxl345_axis {
>  	ADXL345_Z_EN = BIT(0),
> @@ -163,6 +165,10 @@ struct adxl345_state {
>  	u8 watermark;
>  	u8 fifo_mode;
>  
> +	u32 act_axis_ctrl;
> +	bool act_ac;
> +	u8 act_value;
> +
>  	u32 tap_axis_ctrl;
>  	u8 tap_threshold;
>  	u32 tap_duration_us;
> @@ -177,6 +183,11 @@ struct adxl345_state {
>  };
>  
>  static struct iio_event_spec adxl345_events[] = {
> +	{
> +		/* activity */
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_RISING,
> +	},
>  	{
>  		/* single tap */
>  		.type = IIO_EV_TYPE_GESTURE,
> @@ -276,6 +287,117 @@ static inline int adxl345_write_interrupts(struct adxl345_state *st)
>  	return regmap_write(st->regmap, ADXL345_REG_INT_ENABLE, st->int_map);
>  }
>  
> +/* act/inact */
> +
> +static int adxl345_write_act_axis(struct adxl345_state *st, bool en)
> +{
> +	int ret;
> +
> +	/*
> +	 * A setting of 0 selects dc-coupled operation, and a setting of 1
> +	 * enables ac-coupled operation. In dc-coupled operation, the current
> +	 * acceleration magnitude is compared directly with THRESH_ACT and
> +	 * 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
> +	 * 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
> +	 * THRESH_INACT. If the difference is less than the value in
> +	 * THRESH_INACT for the time in TIME_INACT, the device is  considered
> +	 * inactive and the inactivity interrupt is triggered.
> +	 */
> +	ret = regmap_update_bits(st->regmap, ADXL345_REG_ACT_INACT_CTRL,
> +				 ADXL345_REG_ACT_ACDC_MSK, st->act_ac);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * The ADXL345 allows for individually enabling/disabling axis for
> +	 * activity and inactivity detection, respectively. Here both axis are
> +	 * kept in sync, i.e. an axis will be generally enabled or disabled for
> +	 * both equally, activity and inactivity detection.
> +	 */
> +	st->act_axis_ctrl = en
> +		? st->act_axis_ctrl | ADXL345_REG_ACT_AXIS_MSK
> +		: st->act_axis_ctrl & ~ADXL345_REG_ACT_AXIS_MSK;
> +
> +	ret = regmap_update_bits(st->regmap, ADXL345_REG_ACT_INACT_CTRL,
> +				 ADXL345_REG_ACT_AXIS_MSK,
> +				 st->act_axis_ctrl);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int adxl345_set_act_int(struct adxl345_state *st)
> +{
> +	bool args_valid;
> +	bool axis_en;
> +
> +	axis_en = FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, st->act_axis_ctrl) > 0;
> +	args_valid = axis_en && st->act_value > 0;
> +	adxl345_intmap_switch_bit(st, args_valid, ADXL345_INT_ACTIVITY);
> +
> +	return adxl345_write_interrupts(st);
> +}
> +
> +static int _adxl345_is_act_en(struct adxl345_state *st, bool *en)
> +{
> +	int ret;
> +	unsigned int regval;
> +
> +	ret = adxl345_read_interrupts(st, &regval);
> +	if (ret)
> +		return ret;
> +
> +	*en = FIELD_GET(ADXL345_INT_ACTIVITY, regval) > 0;
> +
> +	return 0;
> +}
> +
> +static int _adxl345_set_act_en(struct adxl345_state *st, bool en)
> +{
> +	int ret;
> +
> +	ret = adxl345_write_act_axis(st, en);
> +	if (ret)
> +		return ret;
> +
> +	return adxl345_set_act_int(st);
> +}
> +
> +static int adxl345_is_act_en(struct adxl345_state *st, bool *en)
> +{
> +	return _adxl345_is_act_en(st, en);
> +}
> +
> +static int adxl345_set_act_en(struct adxl345_state *st, bool en)
> +{
> +	return _adxl345_set_act_en(st, en);
> +}
> +
> +static int _adxl345_set_act_value(struct adxl345_state *st, u8 val)
> +{
> +	st->act_value = val;
> +
> +	return regmap_write(st->regmap, ADXL345_REG_THRESH_ACT, val);
> +}
> +
> +static int adxl345_set_act_value(struct adxl345_state *st, u8 val)
> +{
> +	return _adxl345_set_act_value(st, val);
> +}