linux-kernel - Re: [PATCH v4 10/11] iio: accel: adxl313: add AC coupled activity/inactivity events

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75Ve+f4c-aVD3vMfi1NP7vohJWDFFO+F4ckYCKkw2iYDsFw@mail.gmail.com>
Date: Sun, 1 Jun 2025 22:53:43 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Lothar Rubusch <l.rubusch@...il.com>
Cc: jic23@...nel.org, dlechner@...libre.com, nuno.sa@...log.com, 
	andy@...nel.org, corbet@....net, lucas.p.stankus@...il.com, lars@...afoo.de, 
	Michael.Hennerich@...log.com, bagasdotme@...il.com, linux-iio@...r.kernel.org, 
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 10/11] iio: accel: adxl313: add AC coupled
 activity/inactivity events

On Sun, Jun 1, 2025 at 8:22 PM Lothar Rubusch <l.rubusch@...il.com> wrote:
>
> Add AC coupling activity and inactivity as MAG_ADAPTIVE events. This adds
> up an additional set of threshold and period handles, verifies matching
> disabling functionality and extends setting the link bit to complementary
> event configurations.
>
> This means, e.g. either ACTIVITY or ACTIVITY_AC can be enabled. The most
> recent set will remain configured. Disabling ACTIVITY where ACTIVITY_AC was
> enabled is ignored, since it does not match (should be disabling
> ACTIVITY_AC). When INACTIVITY or INACTIVITY_AC is also enabled, the link
> bit will be set. Note, having the link bit and auto-sleep in place activity
> and inactivity indicate the power save state change and thus will only be
> triggered once a state transition occurs. Since there is a separate AC bit
> for ACTIVITY and for INACTIVITY, events can be linked independently from
> each other i.e. ACTIVITY can be linked to INACTIVITY_AC for instance.
>
> When one of both is disabled, the link bit will be removed. Hence, the
> remaining event will not indicate a plain state change anymore, but occur
> as a periodically triggered inactivity event or for each activity event
> above the threshold.

...

> +/**
> + * adxl313_is_act_inact_ac() - Check if AC coupling is enabled.

> + *

Unneeded blank line.

> + * @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
> + * currently enabled or not.
> + *
> + * Return if the provided activity type has AC coupling enabled or a negative
> + * error value.

Missing Return section. Always try kernel-doc validation when adding
new kernel-doc descriptions.

> + */

...

>         unsigned int regval;
> +       int coupling;

Why? Doesn't 'ret' suffice?

>         int axis_en, int_en, ret;

...

> -       int act_en, inact_en;
> -       bool en;
> +       int act_en, inact_en, act_ac_en, inact_ac_en;
> +       bool en, act_inact_ac;
>         int ret;

For all your patches: try really hard to avoid the ping-pong coding,
i.e. when you add something in one patch in the series and change in
the other for no reason. I.o.w. when the initial code may be written
already in a form that doesn't need further changes (e.g., switch-case
vs. if).

This patch is *very* noisy due to the above. So, just slow down, try a
new approach that you have less '-' lines in the diff:s all over the
code.

-- 
With Best Regards,
Andy Shevchenko