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

[Andy Shevchenko]

On Mon, Jun 16, 2025 at 1:23 AM 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.

...

> +/**
> + * adxl313_is_act_inact_ac() - Check if AC coupling is enabled.
> + * @data: The device data.
> + * @type: The activity or inactivity type.
> + *
> + * Provide a type of activity or inactivity, combined with either AC coupling
> + * set, or default to DC coupling. This function verifies, if the combination is

Comma is not needed before "if".

> + * currently enabled or not.
> + *
> + * Return: if the provided activity type has AC coupling enabled or a negative
> + * error value.
> + */

...

>  static int adxl313_is_act_inact_en(struct adxl313_data *data,
>                                    enum adxl313_activity_type type)
>  {
>         unsigned int axis_ctrl;
>         unsigned int regval;
> -       int axis_en, ret;
> +       bool axis_en, int_en;

Why is axis_en not bool to begin with?

> +       int ret;

> +       int_en = adxl313_act_int_reg[type] & regval;
> +       if (!int_en)
> +               return false;
> +
> +       /* Check if configured coupling matches provided type */
> +       return adxl313_is_act_inact_ac(data, type);
>  }

...

> +       act_en = act_en || act_ac_en;

Why is this done here and not after inact_*en checks?

>         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;

...somewhere here?

> +       inact_en = inact_en || inact_ac_en;
> +
>         en = en && act_en && inact_en;

...

> +               /* When turning off, check if the correct coupling event was
> +                * specified, this can be misused, e.g.: Having AC-coupled
> +                * activity turned on, and in current call trying to turning off
> +                * a DC-coupled activity shall be caught here.
> +                */

/*
 * Wrong multi-line
 * style. Use this example.
 */

Also, please rephrase a bit or split to more sentences as it's harder
to understand now what you meant. Also this doesn't clearly explain
the ' < 0 --> return 0' cases.

> +               if (adxl313_is_act_inact_ac(data, type) <= 0)
> +                       return 0;

-- 
With Best Regards,
Andy Shevchenko