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

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 20 Jul 2025 20:36:09 +0200
Lothar Rubusch <l.rubusch@gmail.com> wrote:

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

I think just using the XYZ_EN is clear enough as we are checking for
'any of' those.

> 
> > Jonathan
> >  
> ...
> Best,
> L