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: <CAFXKEHYB-JM11tbpdb+sRBZ7pF4oYD5BO5OVxxf1VPU09MXkNQ@mail.gmail.com>
Date: Wed, 11 Jun 2025 19:12:05 +0200
From: Lothar Rubusch <l.rubusch@...il.com>
To: Andy Shevchenko <andy.shevchenko@...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

Hi Andy,

On Sun, Jun 1, 2025 at 9:54 PM Andy Shevchenko
<andy.shevchenko@...il.com> wrote:
>
> 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?
>

the coupling variable here is rather meant to provide kind of a
semantic context. It shall be checked for being negative (error), or
used in binary decision logic. In fact, could be done with ret as
well, but then in case I'd need to comment that in this case the value
of 'ret' carries either error, or the bool if we have coupling or not.
I'd like to leave it like this, but let me know if better replace it
by ret here.

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

Agree. I tried to follow the review comments. Probably, IMHO it's
mostly about how to separate the patches. Your reviews seem to be
quite focussed on the particular patch w/o taking the context of
follow up patches so much into account. [At least by the way you gave
me feed back here. Actually, by your vast experience I'm pretty sure
you have the context of how such a driver shall look and have an
excellent overview well in mind.]

So, I guess you'd like to stress on certain points. I'm wondering if
it might probably be better to send you this all first in one big
patch, or say rather bigger patches, and then separate pieces out? Let
me know what you think. Thank you so much for the reviews, let's see
how this can be improved here in a v5.

Best,
L

>
> --
> With Best Regards,
> Andy Shevchenko