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
