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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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

...

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

?

-- 
With Best Regards,
Andy Shevchenko