linux-kernel - Re: [PATCH v11 3/8] iio: accel: adxl345: add activity event 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: <aGaSkQHQKCqfrVXF@smile.fi.intel.com>
Date: Thu, 3 Jul 2025 17:24:17 +0300
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Lothar Rubusch <l.rubusch@...il.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 3/8] iio: accel: adxl345: add activity event feature

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