Re: [PATCH v5 7/8] iio: accel: adxl313: add AC coupled activity/inactivity events

[Jonathan Cameron]

On Sun, 15 Jun 2025 22:22:57 +0000
Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote:

> Introduce AC-coupled activity and inactivity as MAG_ADAPTIVE events.
> This adds a new set of threshold and duration configuration options,
> ensures proper handling of event disabling, and extends the use of the
> link bit to support complementary event configurations.
> 
> For example, either ACTIVITY or ACTIVITY_AC can be enabled, but only the
> most recently set configuration will remain active. Disabling ACTIVITY
> will have no effect if ACTIVITY_AC is currently enabled, as the event
> types must match (i.e., ACTIVITY_AC must be explicitly disabled). When
> either INACTIVITY or INACTIVITY_AC is enabled alongside an activity
> event, the link bit is set.
> 
> With the link bit and auto-sleep enabled, activity and inactivity events
> represent changes in the sensor's power-saving state and are only
> triggered upon actual state transitions. Since AC coupling uses separate
> bits for activity and inactivity, each can be configured independently.
> For instance, ACTIVITY can be linked with INACTIVITY_AC.
> 
> If one of the linked events is disabled, the link bit is cleared. In
> that case, the remaining event will no longer reflect a state transition
> but will instead trigger based on periodic inactivity or whenever the
> activity threshold is exceeded.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@xxxxxxxxx>

One small suggestion below.

>  
>  static int adxl313_set_act_inact_linkbit(struct adxl313_data *data, bool en)
>  {
> -	int act_en, inact_en;
> +	int act_en, inact_en, act_ac_en, inact_ac_en;
>  
>  	act_en = adxl313_is_act_inact_en(data, ADXL313_ACTIVITY);
>  	if (act_en < 0)
>  		return act_en;
>  
> +	act_ac_en = adxl313_is_act_inact_en(data, ADXL313_ACTIVITY_AC);
> +	if (act_ac_en < 0)
> +		return act_ac_en;
> +
> +	act_en = act_en || act_ac_en;

All ends up a little confusing as act_en changes meaning as it is built
up.  Maybe better to just have act_dc_en in earlier patch then so
the boolean complexity all in one place?...

> +
>  	inact_en = adxl313_is_act_inact_en(data, ADXL313_INACTIVITY);
>  	if (inact_en < 0)
>  		return inact_en;
>  
> +	inact_ac_en = adxl313_is_act_inact_en(data, ADXL313_INACTIVITY_AC);
> +	if (inact_ac_en < 0)
> +		return inact_ac_en;
> +
> +	inact_en = inact_en || inact_ac_en;
> +
>  	en = en && act_en && inact_en;

	en = en && (act_dc_en || act_ac_en) && (inact_dc_en || inact_ac_en);

>  
>  	return regmap_assign_bits(data->regmap, ADXL313_REG_POWER_CTL,