Re: [PATCH v11 3/8] iio: accel: adxl345: add activity event feature

[Lothar Rubusch <l.rubusch@gmail.com>]

Hi, I appologize for late replying on this topic.

On Sun, Jul 6, 2025 at 6:09 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu, 3 Jul 2025 17:24:17 +0300
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
>
> > On Wed, Jul 02, 2025 at 11:03:10PM +0000, Lothar Rubusch wrote:
> > > Enable the sensor to detect activity and trigger interrupts accordingly.
> > > Activity events are determined based on a threshold, which is initialized
> > > to a sensible default during probe. This default value is adopted from the
> > > legacy ADXL345 input driver to maintain consistent behavior.
> > >
> > > The combination of activity detection, ODR configuration, and range
> > > settings lays the groundwork for the activity/inactivity hysteresis
> > > mechanism, which will be implemented in a subsequent patch. As such,
> > > portions of this patch prepare switch-case structures to support those
> > > upcoming changes.
> >
> > >  #define ADXL345_REG_TAP_AXIS_MSK   GENMASK(2, 0)
> > >  #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_TAP_Z_EN           BIT(0)
> > >  #define ADXL345_TAP_Y_EN           BIT(1)
> > >  #define ADXL345_TAP_X_EN           BIT(2)
> > >
> > > +#define ADXL345_ACT_Z_EN           BIT(4)
> > > +#define ADXL345_ACT_Y_EN           BIT(5)
> > > +#define ADXL345_ACT_X_EN           BIT(6)
> > > +#define ADXL345_ACT_XYZ_EN         (ADXL345_ACT_Z_EN | ADXL345_ACT_Y_EN | ADXL345_ACT_X_EN)
> >
> > I'm trying to understand the logic behind the placement of the masks and bits.
> > To me it sounds that the above should be rather
> >
> > #define ADXL345_REG_TAP_AXIS_MSK      GENMASK(2, 0)
> > #define ADXL345_TAP_Z_EN              BIT(0)
> > #define ADXL345_TAP_Y_EN              BIT(1)
> > #define ADXL345_TAP_X_EN              BIT(2)
> > #define ADXL345_REG_TAP_SUPPRESS_MSK  BIT(3) // Do we need this at all?
> > #define ADXL345_REG_TAP_SUPPRESS      BIT(3) // or actually this? One is enough, no?
> > #define ADXL345_REG_ACT_AXIS_MSK      GENMASK(6, 4)
> > #define ADXL345_ACT_Z_EN              BIT(4)
> > #define ADXL345_ACT_Y_EN              BIT(5)
> > #define ADXL345_ACT_X_EN              BIT(6)
> > #define ADXL345_ACT_XYZ_EN            (ADXL345_ACT_Z_EN | ADXL345_ACT_Y_EN | ADXL345_ACT_X_EN)
> >
> > (Yes, I know that the mess is preexisted, but try to keep some order in the
> >  pieces you add here.)
>
> FWIW I fully agree on keeping field definitions and field break up together.
>
> The ACT_MSK is a little odd as thing as then we'd expect there to be bits
> within that. So that FIELD_GET(a, ADXL345_REG_ACT_AXIS_MSK) would return
> a value from a list of things like
> ADXL345_REG_ACT_AXIS_VALUE_A and similar.
>
> So I'd not define that as a mask a tall but just use the
> ACT_XYZ_EN for it as then it's clear you are checking for any of the
> 3 bits being set.
>

The reason is that ADXL345_REG_ACT_AXIS_MSK is used in the evaluation
of the incoming interrupt status register for "activity" events, and
ADXL345_ACT_XYZ_EN is supposed to group the enabled axis, when
enabling the sensor feature "activity" in the enable register. At the
end of the day, using only one of them would work for both, but
there's a semantic difference.

Given this explanation, would you prefer to see a separate
ADXL345_REG_ACT_AXIS_MSK and ADXL345_ACT_XYZ_EN as presented here, or
just one ADXL345_ACT_XYZ_EN covering both cases, i.e. the evaluation
of the interrupt status, and enabling activity axis?

> Jonathan
>
...
Best,
L