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

[Jonathan Cameron <jic23@kernel.org>]

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.

Jonathan


> 
> ...
> 
> > +		.mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE) |
> > +			BIT(IIO_EV_INFO_VALUE),  
> 
> I would expect one of the below (indentation) styles
> 
> 		.mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE) |
> 				       BIT(IIO_EV_INFO_VALUE),
> 
> 		.mask_shared_by_type =
> 			BIT(IIO_EV_INFO_ENABLE) |
> 			BIT(IIO_EV_INFO_VALUE),
> 
> ...
> 
> >  static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
> > -			      enum iio_modifier tap_dir)
> > +			      enum iio_modifier tap_dir,
> > +			      enum iio_modifier act_dir)  
> 
> Hmm... Why not
> 
> static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
> 			      enum iio_modifier act_dir,
> 			      enum iio_modifier tap_dir)
> 
> ?
>