linux-kernel - Re: [PATCH v11 4/8] iio: accel: adxl345: add inactivity feature

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: <CAFXKEHb4MQk=6hyh-02Fq_XmkQmMiwc-WT4ZSviP6x4XA463mQ@mail.gmail.com>
Date: Thu, 3 Jul 2025 16:59:50 +0200
From: Lothar Rubusch <l.rubusch@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...el.com>
Cc: lars@...afoo.de, Michael.Hennerich@...log.com, jic23@...nel.org, 
	dlechner@...libre.com, nuno.sa@...log.com, andy@...nel.org, corbet@....net, 
	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-doc@...r.kernel.org, eraretuya@...il.com
Subject: Re: [PATCH v11 4/8] iio: accel: adxl345: add inactivity feature

Hi Andy, thank you so much for the (almost) immediate feedback!

On Thu, Jul 3, 2025 at 4:26 PM Andy Shevchenko
<andriy.shevchenko@...el.com> wrote:
>
> On Wed, Jul 02, 2025 at 11:03:11PM +0000, Lothar Rubusch wrote:
> > Add support for the sensor’s inactivity feature in the driver. When both
> > activity and inactivity detection are enabled, the sensor sets a link bit
> > that ties the two functions together. This also enables auto-sleep mode,
> > allowing the sensor to automatically enter sleep state upon detecting
> > inactivity.
> >
> > Inactivity detection relies on a configurable threshold and a specified
> > time period. If sensor measurements remain below the threshold for the
> > defined duration, the sensor transitions to the inactivity state.
> >
> > When an Output Data Rate (ODR) is set, the inactivity time period is
> > automatically adjusted to a sensible default. Higher ODRs result in shorter
> > inactivity timeouts, while lower ODRs allow longer durations-within
> > reasonable upper and lower bounds. This is important because features like
> > auto-sleep operate effectively only between 12.5 Hz and 400 Hz. These
> > defaults are applied when the sample rate is modified, but users can
> > override them by explicitly setting a custom inactivity timeout.
> >
> > Similarly, configuring the g-range provides default threshold values for
> > both activity and inactivity detection. These are implicit defaults meant
> > to simplify configuration, but they can also be manually overridden as
> > needed.
>
> ...
>
> >  #define ADXL345_REG_TAP_SUPPRESS_MSK BIT(3)
> >  #define ADXL345_REG_TAP_SUPPRESS     BIT(3)
> >  #define ADXL345_REG_ACT_AXIS_MSK     GENMASK(6, 4)
> > +#define ADXL345_REG_INACT_AXIS_MSK   GENMASK(2, 0)
> > +#define ADXL345_POWER_CTL_INACT_MSK  (ADXL345_POWER_CTL_AUTO_SLEEP | ADXL345_POWER_CTL_LINK)
> >
> >  #define ADXL345_TAP_Z_EN             BIT(0)
> >  #define ADXL345_TAP_Y_EN             BIT(1)
> >  #define ADXL345_TAP_X_EN             BIT(2)
> >
> > +#define ADXL345_INACT_Z_EN           BIT(0)
> > +#define ADXL345_INACT_Y_EN           BIT(1)
> > +#define ADXL345_INACT_X_EN           BIT(2)
> > +#define ADXL345_INACT_XYZ_EN         (ADXL345_INACT_Z_EN | ADXL345_INACT_Y_EN | ADXL345_INACT_X_EN)
> > +
> >  #define ADXL345_ACT_Z_EN             BIT(4)
> >  #define ADXL345_ACT_Y_EN             BIT(5)
> >  #define ADXL345_ACT_X_EN             BIT(6)
>
> Now it's even more mess. I am lost in understanding which bits/masks are from
> the same offset and which are not.
>

I'm sorry for that. I mean, while the above is supposed to make it
clear where the "values" are coming from, I also could setup something
like the following which is shorter.
+#define ADXL345_INACT_XYZ_EN        GENMASK(2,0)
+#define ADXL345_ACT_XYZ_EN        GENMASK(6,4)

As I understand you, you'd rather prefer to see the latter one in the kernel?

> ...
>
> > +             .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
> > +                     BIT(IIO_EV_INFO_PERIOD),
>
> Same comment about indentation style.
>

I'll take it on the list. As I mentioned, "checkpatch --strict" seems
to complain about function arguments not being aligned, but
assignments this way are passing nicely, like the one above. Depending
on what else comes up, I'll put this CR on the list. Thank you.

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