[PATCH 0/9] st_lsm6dsx: add tap event detection

[PATCH 0/9] st_lsm6dsx: add tap event detection

[Francesco Lavra]

The bulk of this patch set consists of reworking the existing code for event
detection (which supports IIO_EV_TYPE_THRESH events only) in order to make it
generic to accommodate different event types. Actual support for tap events is
implemented in the last patch.
Tested on LSMDSV16X.

Francesco Lavra (9):
  iio: imu: st_lsm6dsx: dynamically initialize iio_chan_spec data
  iio: imu: st_lsm6dsx: make event_settings more generic
  iio: imu: st_lsm6dsx: move wakeup event enable mask to event_src
  iio: imu: st_lsm6dsx: dynamically allocate iio_event_spec structs
  iio: imu: st_lsm6dsx: rework code to check for enabled events
  iio: imu: st_lsm6dsx: remove event_threshold field from hw struct
  iio: imu: st_lsm6dsx: make event management functions generic
  iio: imu: st_lsm6dsx: add event configurability on a per axis basis
  iio: imu: stm_lsm6dsx: add tap event detection

 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  73 +--
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 653 +++++++++++--------
 2 files changed, 422 insertions(+), 304 deletions(-)

-- 
2.39.5

[PATCH 1/9] iio: imu: st_lsm6dsx: dynamically initialize iio_chan_spec data

[Francesco Lavra]

Using the ST_LSM6DSX_CHANNEL_ACC() macro as a static initializer
for the iio_chan_spec struct arrays makes all sensors advertise
channel event capabilities regardless of whether they actually
support event generation. And if userspace tries to configure
accelerometer wakeup events on a sensor device that does not
support them (e.g. LSM6DS0), st_lsm6dsx_write_event() dereferences
a NULL pointer when trying to write to the wakeup register.
Replace usage of the ST_LSM6DSX_CHANNEL_ACC() and
ST_LSM6DSX_CHANNEL() macros with dynamic allocation and
initialization of struct iio_chan_spec arrays, where the
st_lsm6dsx_event structure is only used for sensors that support
wakeup events; besides fixing the above bug, this serves as a
preliminary step for adding support for more event types.

Signed-off-by: Francesco Lavra <flavra@baylibre.com>
---
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  26 +--
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 164 ++++++++-----------
 2 files changed, 71 insertions(+), 119 deletions(-)

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
index a4f558899767..db863bd1898d 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
@@ -80,27 +80,6 @@ enum st_lsm6dsx_hw_id {
 					 * ST_LSM6DSX_TAGGED_SAMPLE_SIZE)
 #define ST_LSM6DSX_SHIFT_VAL(val, mask)	(((val) << __ffs(mask)) & (mask))
 
-#define ST_LSM6DSX_CHANNEL_ACC(chan_type, addr, mod, scan_idx)		\
-{									\
-	.type = chan_type,						\
-	.address = addr,						\
-	.modified = 1,							\
-	.channel2 = mod,						\
-	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
-	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),		\
-	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
-	.scan_index = scan_idx,						\
-	.scan_type = {							\
-		.sign = 's',						\
-		.realbits = 16,						\
-		.storagebits = 16,					\
-		.endianness = IIO_LE,					\
-	},								\
-	.event_spec = &st_lsm6dsx_event,				\
-	.ext_info = st_lsm6dsx_ext_info,				\
-	.num_event_specs = 1,						\
-}
-
 #define ST_LSM6DSX_CHANNEL(chan_type, addr, mod, scan_idx)		\
 {									\
 	.type = chan_type,						\
@@ -328,10 +307,7 @@ struct st_lsm6dsx_settings {
 		const char *name;
 		u8 wai;
 	} id[ST_LSM6DSX_MAX_ID];
-	struct {
-		const struct iio_chan_spec *chan;
-		int len;
-	} channels[2];
+	u8 chan_addr_base[2];
 	struct {
 		struct st_lsm6dsx_reg irq1;
 		struct st_lsm6dsx_reg irq2;
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index 216160549b5a..17b46e15cce5 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -96,26 +96,7 @@
 
 #define ST_LSM6DSX_TS_SENSITIVITY		25000UL /* 25us */
 
-static const struct iio_chan_spec st_lsm6dsx_acc_channels[] = {
-	ST_LSM6DSX_CHANNEL_ACC(IIO_ACCEL, 0x28, IIO_MOD_X, 0),
-	ST_LSM6DSX_CHANNEL_ACC(IIO_ACCEL, 0x2a, IIO_MOD_Y, 1),
-	ST_LSM6DSX_CHANNEL_ACC(IIO_ACCEL, 0x2c, IIO_MOD_Z, 2),
-	IIO_CHAN_SOFT_TIMESTAMP(3),
-};
-
-static const struct iio_chan_spec st_lsm6dsx_gyro_channels[] = {
-	ST_LSM6DSX_CHANNEL(IIO_ANGL_VEL, 0x22, IIO_MOD_X, 0),
-	ST_LSM6DSX_CHANNEL(IIO_ANGL_VEL, 0x24, IIO_MOD_Y, 1),
-	ST_LSM6DSX_CHANNEL(IIO_ANGL_VEL, 0x26, IIO_MOD_Z, 2),
-	IIO_CHAN_SOFT_TIMESTAMP(3),
-};
-
-static const struct iio_chan_spec st_lsm6ds0_gyro_channels[] = {
-	ST_LSM6DSX_CHANNEL(IIO_ANGL_VEL, 0x18, IIO_MOD_X, 0),
-	ST_LSM6DSX_CHANNEL(IIO_ANGL_VEL, 0x1a, IIO_MOD_Y, 1),
-	ST_LSM6DSX_CHANNEL(IIO_ANGL_VEL, 0x1c, IIO_MOD_Z, 2),
-	IIO_CHAN_SOFT_TIMESTAMP(3),
-};
+#define ST_LSM6DSX_CHAN_COUNT		4
 
 static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 	{
@@ -142,15 +123,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 				.wai = 0x68,
 			},
 		},
-		.channels = {
-			[ST_LSM6DSX_ID_ACC] = {
-				.chan = st_lsm6dsx_acc_channels,
-				.len = ARRAY_SIZE(st_lsm6dsx_acc_channels),
-			},
-			[ST_LSM6DSX_ID_GYRO] = {
-				.chan = st_lsm6ds0_gyro_channels,
-				.len = ARRAY_SIZE(st_lsm6ds0_gyro_channels),
-			},
+		.chan_addr_base = {
+			[ST_LSM6DSX_ID_ACC] = 0x28,
+			[ST_LSM6DSX_ID_GYRO] = 0x18,
 		},
 		.odr_table = {
 			[ST_LSM6DSX_ID_ACC] = {
@@ -246,15 +221,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 				.wai = 0x69,
 			},
 		},
-		.channels = {
-			[ST_LSM6DSX_ID_ACC] = {
-				.chan = st_lsm6dsx_acc_channels,
-				.len = ARRAY_SIZE(st_lsm6dsx_acc_channels),
-			},
-			[ST_LSM6DSX_ID_GYRO] = {
-				.chan = st_lsm6dsx_gyro_channels,
-				.len = ARRAY_SIZE(st_lsm6dsx_gyro_channels),
-			},
+		.chan_addr_base = {
+			[ST_LSM6DSX_ID_ACC] = 0x28,
+			[ST_LSM6DSX_ID_GYRO] = 0x22,
 		},
 		.odr_table = {
 			[ST_LSM6DSX_ID_ACC] = {
@@ -412,15 +381,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 				.wai = 0x69,
 			},
 		},
-		.channels = {
-			[ST_LSM6DSX_ID_ACC] = {
-				.chan = st_lsm6dsx_acc_channels,
-				.len = ARRAY_SIZE(st_lsm6dsx_acc_channels),
-			},
-			[ST_LSM6DSX_ID_GYRO] = {
-				.chan = st_lsm6dsx_gyro_channels,
-				.len = ARRAY_SIZE(st_lsm6dsx_gyro_channels),
-			},
+		.chan_addr_base = {
+			[ST_LSM6DSX_ID_ACC] = 0x28,
+			[ST_LSM6DSX_ID_GYRO] = 0x22,
 		},
 		.odr_table = {
 			[ST_LSM6DSX_ID_ACC] = {
@@ -590,15 +553,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 				.wai = 0x6a,
 			},
 		},
-		.channels = {
-			[ST_LSM6DSX_ID_ACC] = {
-				.chan = st_lsm6dsx_acc_channels,
-				.len = ARRAY_SIZE(st_lsm6dsx_acc_channels),
-			},
-			[ST_LSM6DSX_ID_GYRO] = {
-				.chan = st_lsm6dsx_gyro_channels,
-				.len = ARRAY_SIZE(st_lsm6dsx_gyro_channels),
-			},
+		.chan_addr_base = {
+			[ST_LSM6DSX_ID_ACC] = 0x28,
+			[ST_LSM6DSX_ID_GYRO] = 0x22,
 		},
 		.odr_table = {
 			[ST_LSM6DSX_ID_ACC] = {
@@ -847,15 +804,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 				.wai = 0x6d,
 			},
 		},
-		.channels = {
-			[ST_LSM6DSX_ID_ACC] = {
-				.chan = st_lsm6dsx_acc_channels,
-				.len = ARRAY_SIZE(st_lsm6dsx_acc_channels),
-			},
-			[ST_LSM6DSX_ID_GYRO] = {
-				.chan = st_lsm6dsx_gyro_channels,
-				.len = ARRAY_SIZE(st_lsm6dsx_gyro_channels),
-			},
+		.chan_addr_base = {
+			[ST_LSM6DSX_ID_ACC] = 0x28,
+			[ST_LSM6DSX_ID_GYRO] = 0x22,
 		},
 		.drdy_mask = {
 			.addr = 0x13,
@@ -1060,15 +1011,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 				.wai = 0x6b,
 			},
 		},
-		.channels = {
-			[ST_LSM6DSX_ID_ACC] = {
-				.chan = st_lsm6dsx_acc_channels,
-				.len = ARRAY_SIZE(st_lsm6dsx_acc_channels),
-			},
-			[ST_LSM6DSX_ID_GYRO] = {
-				.chan = st_lsm6dsx_gyro_channels,
-				.len = ARRAY_SIZE(st_lsm6dsx_gyro_channels),
-			},
+		.chan_addr_base = {
+			[ST_LSM6DSX_ID_ACC] = 0x28,
+			[ST_LSM6DSX_ID_GYRO] = 0x22,
 		},
 		.drdy_mask = {
 			.addr = 0x13,
@@ -1237,15 +1182,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 				.wai = 0x70,
 			},
 		},
-		.channels = {
-			[ST_LSM6DSX_ID_ACC] = {
-				.chan = st_lsm6dsx_acc_channels,
-				.len = ARRAY_SIZE(st_lsm6dsx_acc_channels),
-			},
-			[ST_LSM6DSX_ID_GYRO] = {
-				.chan = st_lsm6dsx_gyro_channels,
-				.len = ARRAY_SIZE(st_lsm6dsx_gyro_channels),
-			},
+		.chan_addr_base = {
+			[ST_LSM6DSX_ID_ACC] = 0x28,
+			[ST_LSM6DSX_ID_GYRO] = 0x22,
 		},
 		.drdy_mask = {
 			.addr = 0x13,
@@ -1443,15 +1382,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 				.wai = 0x22,
 			}
 		},
-		.channels = {
-			[ST_LSM6DSX_ID_ACC] = {
-				.chan = st_lsm6dsx_acc_channels,
-				.len = ARRAY_SIZE(st_lsm6dsx_acc_channels),
-			},
-			[ST_LSM6DSX_ID_GYRO] = {
-				.chan = st_lsm6dsx_gyro_channels,
-				.len = ARRAY_SIZE(st_lsm6dsx_gyro_channels),
-			},
+		.chan_addr_base = {
+			[ST_LSM6DSX_ID_ACC] = 0x28,
+			[ST_LSM6DSX_ID_GYRO] = 0x22,
 		},
 		.odr_table = {
 			[ST_LSM6DSX_ID_ACC] = {
@@ -2366,21 +2299,64 @@ static int st_lsm6dsx_init_device(struct st_lsm6dsx_hw *hw)
 	return st_lsm6dsx_init_hw_timer(hw);
 }
 
+static int st_lsm6dsx_chan_init(struct iio_chan_spec *channels, struct st_lsm6dsx_hw *hw,
+				enum st_lsm6dsx_sensor_id id, int index)
+{
+	struct iio_chan_spec *chan = &channels[index];
+
+	chan->type = (id == ST_LSM6DSX_ID_ACC) ? IIO_ACCEL : IIO_ANGL_VEL;
+	chan->address = hw->settings->chan_addr_base[id] + index * ST_LSM6DSX_CHAN_SIZE;
+	chan->modified = 1;
+	chan->channel2 = IIO_MOD_X + index;
+	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
+	chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
+	chan->info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ);
+	chan->scan_index = index;
+	chan->scan_type.sign = 's';
+	chan->scan_type.realbits = 16;
+	chan->scan_type.storagebits = 16;
+	chan->scan_type.endianness = IIO_LE;
+	chan->ext_info = st_lsm6dsx_ext_info;
+	if (id == ST_LSM6DSX_ID_ACC) {
+		if (hw->settings->event_settings.wakeup_reg.addr) {
+			chan->event_spec = &st_lsm6dsx_event;
+			chan->num_event_specs = 1;
+		}
+	}
+	return 0;
+}
+
 static struct iio_dev *st_lsm6dsx_alloc_iiodev(struct st_lsm6dsx_hw *hw,
 					       enum st_lsm6dsx_sensor_id id,
 					       const char *name)
 {
 	struct st_lsm6dsx_sensor *sensor;
 	struct iio_dev *iio_dev;
+	struct iio_chan_spec *channels;
+	int i;
 
 	iio_dev = devm_iio_device_alloc(hw->dev, sizeof(*sensor));
 	if (!iio_dev)
 		return NULL;
 
+	channels = devm_kzalloc(hw->dev, sizeof(*channels) * ST_LSM6DSX_CHAN_COUNT, GFP_KERNEL);
+	if (!channels)
+		return NULL;
+
+	for (i = 0; i < 3; i++) {
+		if (st_lsm6dsx_chan_init(channels, hw, id, i) < 0)
+			return NULL;
+	}
+	channels[3].type = IIO_TIMESTAMP;
+	channels[3].channel = -1;
+	channels[3].scan_index = 3;
+	channels[3].scan_type.sign = 's';
+	channels[3].scan_type.realbits = 64;
+	channels[3].scan_type.storagebits = 64;
 	iio_dev->modes = INDIO_DIRECT_MODE;
 	iio_dev->available_scan_masks = st_lsm6dsx_available_scan_masks;
-	iio_dev->channels = hw->settings->channels[id].chan;
-	iio_dev->num_channels = hw->settings->channels[id].len;
+	iio_dev->channels = channels;
+	iio_dev->num_channels = ST_LSM6DSX_CHAN_COUNT;
 
 	sensor = iio_priv(iio_dev);
 	sensor->id = id;
-- 
2.39.5

Re: [PATCH 1/9] iio: imu: st_lsm6dsx: dynamically initialize iio_chan_spec data

[Andy Shevchenko]

On Thu, Oct 30, 2025 at 08:27:44AM +0100, Francesco Lavra wrote:
> Using the ST_LSM6DSX_CHANNEL_ACC() macro as a static initializer
> for the iio_chan_spec struct arrays makes all sensors advertise
> channel event capabilities regardless of whether they actually
> support event generation. And if userspace tries to configure
> accelerometer wakeup events on a sensor device that does not
> support them (e.g. LSM6DS0), st_lsm6dsx_write_event() dereferences
> a NULL pointer when trying to write to the wakeup register.
> Replace usage of the ST_LSM6DSX_CHANNEL_ACC() and
> ST_LSM6DSX_CHANNEL() macros with dynamic allocation and
> initialization of struct iio_chan_spec arrays, where the
> st_lsm6dsx_event structure is only used for sensors that support
> wakeup events; besides fixing the above bug, this serves as a
> preliminary step for adding support for more event types.


Sounds like a bug fix. Fixes tag?

...

> +static int st_lsm6dsx_chan_init(struct iio_chan_spec *channels, struct st_lsm6dsx_hw *hw,
> +				enum st_lsm6dsx_sensor_id id, int index)
> +{
> +	struct iio_chan_spec *chan = &channels[index];
> +
> +	chan->type = (id == ST_LSM6DSX_ID_ACC) ? IIO_ACCEL : IIO_ANGL_VEL;
> +	chan->address = hw->settings->chan_addr_base[id] + index * ST_LSM6DSX_CHAN_SIZE;
> +	chan->modified = 1;
> +	chan->channel2 = IIO_MOD_X + index;
> +	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> +	chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
> +	chan->info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ);
> +	chan->scan_index = index;
> +	chan->scan_type.sign = 's';
> +	chan->scan_type.realbits = 16;
> +	chan->scan_type.storagebits = 16;
> +	chan->scan_type.endianness = IIO_LE;
> +	chan->ext_info = st_lsm6dsx_ext_info;

+ blank line

> +	if (id == ST_LSM6DSX_ID_ACC) {
> +		if (hw->settings->event_settings.wakeup_reg.addr) {
> +			chan->event_spec = &st_lsm6dsx_event;
> +			chan->num_event_specs = 1;
> +		}
> +	}

if (foo) { if (bar) {}  } == if (foo && bar).

Based on this I'm in doubt what to suggest here as to me sounds like those
couple of lines might deserve for a helper.

Hence two options:
1) do an equivalent conditional and reduce indentation level;
2) do a helper with the inner conditional.

+ blank line

> +	return 0;
> +}

...

> +	channels = devm_kzalloc(hw->dev, sizeof(*channels) * ST_LSM6DSX_CHAN_COUNT, GFP_KERNEL);

devm_kcalloc()

> +	if (!channels)
> +		return NULL;

I would expect comment here...

> +	for (i = 0; i < 3; i++) {

3 might need to be defined.

> +		if (st_lsm6dsx_chan_init(channels, hw, id, i) < 0)
> +			return NULL;
> +	}

+ blank line

...and perhaps here to explain what's going on here.

> +	channels[3].type = IIO_TIMESTAMP;
> +	channels[3].channel = -1;
> +	channels[3].scan_index = 3;
> +	channels[3].scan_type.sign = 's';
> +	channels[3].scan_type.realbits = 64;
> +	channels[3].scan_type.storagebits = 64;

+ blank line.

>  	iio_dev->modes = INDIO_DIRECT_MODE;
>  	iio_dev->available_scan_masks = st_lsm6dsx_available_scan_masks;
> -	iio_dev->channels = hw->settings->channels[id].chan;
> -	iio_dev->num_channels = hw->settings->channels[id].len;
> +	iio_dev->channels = channels;
> +	iio_dev->num_channels = ST_LSM6DSX_CHAN_COUNT;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/9] iio: imu: st_lsm6dsx: dynamically initialize iio_chan_spec data

[Francesco Lavra]

On Thu, 2025-10-30 at 09:57 +0200, Andy Shevchenko wrote:
> On Thu, Oct 30, 2025 at 08:27:44AM +0100, Francesco Lavra wrote:
> > Using the ST_LSM6DSX_CHANNEL_ACC() macro as a static initializer
> > for the iio_chan_spec struct arrays makes all sensors advertise
> > channel event capabilities regardless of whether they actually
> > support event generation. And if userspace tries to configure
> > accelerometer wakeup events on a sensor device that does not
> > support them (e.g. LSM6DS0), st_lsm6dsx_write_event() dereferences
> > a NULL pointer when trying to write to the wakeup register.
> > Replace usage of the ST_LSM6DSX_CHANNEL_ACC() and
> > ST_LSM6DSX_CHANNEL() macros with dynamic allocation and
> > initialization of struct iio_chan_spec arrays, where the
> > st_lsm6dsx_event structure is only used for sensors that support
> > wakeup events; besides fixing the above bug, this serves as a
> > preliminary step for adding support for more event types.
> 
> 
> Sounds like a bug fix. Fixes tag?

Will add

> 
> > +static int st_lsm6dsx_chan_init(struct iio_chan_spec *channels, struct
> > st_lsm6dsx_hw *hw,
> > +                               enum st_lsm6dsx_sensor_id id, int
> > index)
> > +{
> > +       struct iio_chan_spec *chan = &channels[index];
> > +
> > +       chan->type = (id == ST_LSM6DSX_ID_ACC) ? IIO_ACCEL :
> > IIO_ANGL_VEL;
> > +       chan->address = hw->settings->chan_addr_base[id] + index *
> > ST_LSM6DSX_CHAN_SIZE;
> > +       chan->modified = 1;
> > +       chan->channel2 = IIO_MOD_X + index;
> > +       chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> > +       chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
> > +       chan->info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ);
> > +       chan->scan_index = index;
> > +       chan->scan_type.sign = 's';
> > +       chan->scan_type.realbits = 16;
> > +       chan->scan_type.storagebits = 16;
> > +       chan->scan_type.endianness = IIO_LE;
> > +       chan->ext_info = st_lsm6dsx_ext_info;
> 
> + blank line
> 
> > +       if (id == ST_LSM6DSX_ID_ACC) {
> > +               if (hw->settings->event_settings.wakeup_reg.addr) {
> > +                       chan->event_spec = &st_lsm6dsx_event;
> > +                       chan->num_event_specs = 1;
> > +               }
> > +       }
> 
> if (foo) { if (bar) {}  } == if (foo && bar).
> 
> Based on this I'm in doubt what to suggest here as to me sounds like
> those
> couple of lines might deserve for a helper.
> 
> Hence two options:
> 1) do an equivalent conditional and reduce indentation level;
> 2) do a helper with the inner conditional.

Will do a helper with the inner conditional.

> + blank line
> 
> > +       return 0;
> > +}
> 
> ...
> 
> > +       channels = devm_kzalloc(hw->dev, sizeof(*channels) *
> > ST_LSM6DSX_CHAN_COUNT, GFP_KERNEL);
> 
> devm_kcalloc()
> 
> > +       if (!channels)
> > +               return NULL;
> 
> I would expect comment here...

This function returns a pointer to the struct iio_dev it allocates and
initializes; if there are any errors, it returns NULL. What kind of comment
do you expect here? It seems obvious that it's returning NULL because of an
allocation error.

> 
> > +       for (i = 0; i < 3; i++) {
> 
> 3 might need to be defined.

Will make an enum to replace the ST_LSM6DSX_CHAN_COUNT #define, and use an
enum value instead of 3.

> 
> > +               if (st_lsm6dsx_chan_init(channels, hw, id, i) < 0)
> > +                       return NULL;
> > +       }
> 
> + blank line
> 
> ...and perhaps here to explain what's going on here.

Same here, what comment do you expect?

> > +       channels[3].type = IIO_TIMESTAMP;
> > +       channels[3].channel = -1;
> > +       channels[3].scan_index = 3;
> > +       channels[3].scan_type.sign = 's';
> > +       channels[3].scan_type.realbits = 64;
> > +       channels[3].scan_type.storagebits = 64;
> 
> + blank line.
> 
> >         iio_dev->modes = INDIO_DIRECT_MODE;
> >         iio_dev->available_scan_masks =
> > st_lsm6dsx_available_scan_masks;
> > -       iio_dev->channels = hw->settings->channels[id].chan;
> > -       iio_dev->num_channels = hw->settings->channels[id].len;
> > +       iio_dev->channels = channels;
> > +       iio_dev->num_channels = ST_LSM6DSX_CHAN_COUNT;
> 

Re: [PATCH 1/9] iio: imu: st_lsm6dsx: dynamically initialize iio_chan_spec data

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 11806 bytes --]

> Using the ST_LSM6DSX_CHANNEL_ACC() macro as a static initializer
> for the iio_chan_spec struct arrays makes all sensors advertise
> channel event capabilities regardless of whether they actually
> support event generation. And if userspace tries to configure
> accelerometer wakeup events on a sensor device that does not
> support them (e.g. LSM6DS0), st_lsm6dsx_write_event() dereferences
> a NULL pointer when trying to write to the wakeup register.
> Replace usage of the ST_LSM6DSX_CHANNEL_ACC() and
> ST_LSM6DSX_CHANNEL() macros with dynamic allocation and
> initialization of struct iio_chan_spec arrays, where the
> st_lsm6dsx_event structure is only used for sensors that support
> wakeup events; besides fixing the above bug, this serves as a
> preliminary step for adding support for more event types.

I agree we are missing the Fixes tag here.

> 
> Signed-off-by: Francesco Lavra <flavra@baylibre.com>
> ---
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  26 +--
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 164 ++++++++-----------
>  2 files changed, 71 insertions(+), 119 deletions(-)
> 
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> index a4f558899767..db863bd1898d 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> @@ -80,27 +80,6 @@ enum st_lsm6dsx_hw_id {
>  					 * ST_LSM6DSX_TAGGED_SAMPLE_SIZE)
>  #define ST_LSM6DSX_SHIFT_VAL(val, mask)	(((val) << __ffs(mask)) & (mask))
>  
> -#define ST_LSM6DSX_CHANNEL_ACC(chan_type, addr, mod, scan_idx)		\
> -{									\
> -	.type = chan_type,						\
> -	.address = addr,						\
> -	.modified = 1,							\
> -	.channel2 = mod,						\
> -	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
> -	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),		\
> -	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
> -	.scan_index = scan_idx,						\
> -	.scan_type = {							\
> -		.sign = 's',						\
> -		.realbits = 16,						\
> -		.storagebits = 16,					\
> -		.endianness = IIO_LE,					\
> -	},								\
> -	.event_spec = &st_lsm6dsx_event,				\
> -	.ext_info = st_lsm6dsx_ext_info,				\
> -	.num_event_specs = 1,						\
> -}
> -
>  #define ST_LSM6DSX_CHANNEL(chan_type, addr, mod, scan_idx)		\
>  {									\
>  	.type = chan_type,						\
> @@ -328,10 +307,7 @@ struct st_lsm6dsx_settings {
>  		const char *name;
>  		u8 wai;
>  	} id[ST_LSM6DSX_MAX_ID];
> -	struct {
> -		const struct iio_chan_spec *chan;
> -		int len;
> -	} channels[2];
> +	u8 chan_addr_base[2];

nit: chan_addr[2]

>  	struct {
>  		struct st_lsm6dsx_reg irq1;
>  		struct st_lsm6dsx_reg irq2;
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> index 216160549b5a..17b46e15cce5 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> @@ -96,26 +96,7 @@
>  
>  #define ST_LSM6DSX_TS_SENSITIVITY		25000UL /* 25us */
>  
> -static const struct iio_chan_spec st_lsm6dsx_acc_channels[] = {
> -	ST_LSM6DSX_CHANNEL_ACC(IIO_ACCEL, 0x28, IIO_MOD_X, 0),
> -	ST_LSM6DSX_CHANNEL_ACC(IIO_ACCEL, 0x2a, IIO_MOD_Y, 1),
> -	ST_LSM6DSX_CHANNEL_ACC(IIO_ACCEL, 0x2c, IIO_MOD_Z, 2),
> -	IIO_CHAN_SOFT_TIMESTAMP(3),
> -};
> -
> -static const struct iio_chan_spec st_lsm6dsx_gyro_channels[] = {
> -	ST_LSM6DSX_CHANNEL(IIO_ANGL_VEL, 0x22, IIO_MOD_X, 0),
> -	ST_LSM6DSX_CHANNEL(IIO_ANGL_VEL, 0x24, IIO_MOD_Y, 1),
> -	ST_LSM6DSX_CHANNEL(IIO_ANGL_VEL, 0x26, IIO_MOD_Z, 2),
> -	IIO_CHAN_SOFT_TIMESTAMP(3),
> -};
> -
> -static const struct iio_chan_spec st_lsm6ds0_gyro_channels[] = {
> -	ST_LSM6DSX_CHANNEL(IIO_ANGL_VEL, 0x18, IIO_MOD_X, 0),
> -	ST_LSM6DSX_CHANNEL(IIO_ANGL_VEL, 0x1a, IIO_MOD_Y, 1),
> -	ST_LSM6DSX_CHANNEL(IIO_ANGL_VEL, 0x1c, IIO_MOD_Z, 2),
> -	IIO_CHAN_SOFT_TIMESTAMP(3),
> -};
> +#define ST_LSM6DSX_CHAN_COUNT		4
>  
>  static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>  	{
> @@ -142,15 +123,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>  				.wai = 0x68,
>  			},
>  		},
> -		.channels = {
> -			[ST_LSM6DSX_ID_ACC] = {
> -				.chan = st_lsm6dsx_acc_channels,
> -				.len = ARRAY_SIZE(st_lsm6dsx_acc_channels),
> -			},
> -			[ST_LSM6DSX_ID_GYRO] = {
> -				.chan = st_lsm6ds0_gyro_channels,
> -				.len = ARRAY_SIZE(st_lsm6ds0_gyro_channels),
> -			},
> +		.chan_addr_base = {
> +			[ST_LSM6DSX_ID_ACC] = 0x28,
> +			[ST_LSM6DSX_ID_GYRO] = 0x18,
>  		},
>  		.odr_table = {
>  			[ST_LSM6DSX_ID_ACC] = {
> @@ -246,15 +221,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>  				.wai = 0x69,
>  			},
>  		},
> -		.channels = {
> -			[ST_LSM6DSX_ID_ACC] = {
> -				.chan = st_lsm6dsx_acc_channels,
> -				.len = ARRAY_SIZE(st_lsm6dsx_acc_channels),
> -			},
> -			[ST_LSM6DSX_ID_GYRO] = {
> -				.chan = st_lsm6dsx_gyro_channels,
> -				.len = ARRAY_SIZE(st_lsm6dsx_gyro_channels),
> -			},
> +		.chan_addr_base = {
> +			[ST_LSM6DSX_ID_ACC] = 0x28,
> +			[ST_LSM6DSX_ID_GYRO] = 0x22,
>  		},
>  		.odr_table = {
>  			[ST_LSM6DSX_ID_ACC] = {
> @@ -412,15 +381,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>  				.wai = 0x69,
>  			},
>  		},
> -		.channels = {
> -			[ST_LSM6DSX_ID_ACC] = {
> -				.chan = st_lsm6dsx_acc_channels,
> -				.len = ARRAY_SIZE(st_lsm6dsx_acc_channels),
> -			},
> -			[ST_LSM6DSX_ID_GYRO] = {
> -				.chan = st_lsm6dsx_gyro_channels,
> -				.len = ARRAY_SIZE(st_lsm6dsx_gyro_channels),
> -			},
> +		.chan_addr_base = {
> +			[ST_LSM6DSX_ID_ACC] = 0x28,
> +			[ST_LSM6DSX_ID_GYRO] = 0x22,
>  		},
>  		.odr_table = {
>  			[ST_LSM6DSX_ID_ACC] = {
> @@ -590,15 +553,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>  				.wai = 0x6a,
>  			},
>  		},
> -		.channels = {
> -			[ST_LSM6DSX_ID_ACC] = {
> -				.chan = st_lsm6dsx_acc_channels,
> -				.len = ARRAY_SIZE(st_lsm6dsx_acc_channels),
> -			},
> -			[ST_LSM6DSX_ID_GYRO] = {
> -				.chan = st_lsm6dsx_gyro_channels,
> -				.len = ARRAY_SIZE(st_lsm6dsx_gyro_channels),
> -			},
> +		.chan_addr_base = {
> +			[ST_LSM6DSX_ID_ACC] = 0x28,
> +			[ST_LSM6DSX_ID_GYRO] = 0x22,
>  		},
>  		.odr_table = {
>  			[ST_LSM6DSX_ID_ACC] = {
> @@ -847,15 +804,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>  				.wai = 0x6d,
>  			},
>  		},
> -		.channels = {
> -			[ST_LSM6DSX_ID_ACC] = {
> -				.chan = st_lsm6dsx_acc_channels,
> -				.len = ARRAY_SIZE(st_lsm6dsx_acc_channels),
> -			},
> -			[ST_LSM6DSX_ID_GYRO] = {
> -				.chan = st_lsm6dsx_gyro_channels,
> -				.len = ARRAY_SIZE(st_lsm6dsx_gyro_channels),
> -			},
> +		.chan_addr_base = {
> +			[ST_LSM6DSX_ID_ACC] = 0x28,
> +			[ST_LSM6DSX_ID_GYRO] = 0x22,
>  		},
>  		.drdy_mask = {
>  			.addr = 0x13,
> @@ -1060,15 +1011,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>  				.wai = 0x6b,
>  			},
>  		},
> -		.channels = {
> -			[ST_LSM6DSX_ID_ACC] = {
> -				.chan = st_lsm6dsx_acc_channels,
> -				.len = ARRAY_SIZE(st_lsm6dsx_acc_channels),
> -			},
> -			[ST_LSM6DSX_ID_GYRO] = {
> -				.chan = st_lsm6dsx_gyro_channels,
> -				.len = ARRAY_SIZE(st_lsm6dsx_gyro_channels),
> -			},
> +		.chan_addr_base = {
> +			[ST_LSM6DSX_ID_ACC] = 0x28,
> +			[ST_LSM6DSX_ID_GYRO] = 0x22,
>  		},
>  		.drdy_mask = {
>  			.addr = 0x13,
> @@ -1237,15 +1182,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>  				.wai = 0x70,
>  			},
>  		},
> -		.channels = {
> -			[ST_LSM6DSX_ID_ACC] = {
> -				.chan = st_lsm6dsx_acc_channels,
> -				.len = ARRAY_SIZE(st_lsm6dsx_acc_channels),
> -			},
> -			[ST_LSM6DSX_ID_GYRO] = {
> -				.chan = st_lsm6dsx_gyro_channels,
> -				.len = ARRAY_SIZE(st_lsm6dsx_gyro_channels),
> -			},
> +		.chan_addr_base = {
> +			[ST_LSM6DSX_ID_ACC] = 0x28,
> +			[ST_LSM6DSX_ID_GYRO] = 0x22,
>  		},
>  		.drdy_mask = {
>  			.addr = 0x13,
> @@ -1443,15 +1382,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>  				.wai = 0x22,
>  			}
>  		},
> -		.channels = {
> -			[ST_LSM6DSX_ID_ACC] = {
> -				.chan = st_lsm6dsx_acc_channels,
> -				.len = ARRAY_SIZE(st_lsm6dsx_acc_channels),
> -			},
> -			[ST_LSM6DSX_ID_GYRO] = {
> -				.chan = st_lsm6dsx_gyro_channels,
> -				.len = ARRAY_SIZE(st_lsm6dsx_gyro_channels),
> -			},
> +		.chan_addr_base = {
> +			[ST_LSM6DSX_ID_ACC] = 0x28,
> +			[ST_LSM6DSX_ID_GYRO] = 0x22,
>  		},
>  		.odr_table = {
>  			[ST_LSM6DSX_ID_ACC] = {
> @@ -2366,21 +2299,64 @@ static int st_lsm6dsx_init_device(struct st_lsm6dsx_hw *hw)
>  	return st_lsm6dsx_init_hw_timer(hw);
>  }
>  

> +static int st_lsm6dsx_chan_init(struct iio_chan_spec *channels, struct st_lsm6dsx_hw *hw,
> +				enum st_lsm6dsx_sensor_id id, int index)

please try to respect the 79 column limit (I still like it :))

> +{
> +	struct iio_chan_spec *chan = &channels[index];
> +
> +	chan->type = (id == ST_LSM6DSX_ID_ACC) ? IIO_ACCEL : IIO_ANGL_VEL;

I think you should return an error here if id is not ST_LSM6DSX_ID_ACC or
ST_LSM6DSX_ID_GYRO.

> +	chan->address = hw->settings->chan_addr_base[id] + index * ST_LSM6DSX_CHAN_SIZE;
> +	chan->modified = 1;
> +	chan->channel2 = IIO_MOD_X + index;
> +	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> +	chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
> +	chan->info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ);
> +	chan->scan_index = index;
> +	chan->scan_type.sign = 's';
> +	chan->scan_type.realbits = 16;
> +	chan->scan_type.storagebits = 16;
> +	chan->scan_type.endianness = IIO_LE;

what about reducing the scope of ST_LSM6DSX_CHANNEL_ACC/ST_LSM6DSX_CHANNEL here
to improve the iio_chan_spec struct initialization since most of the fields are
always the same between different sensors.

> +	chan->ext_info = st_lsm6dsx_ext_info;
> +	if (id == ST_LSM6DSX_ID_ACC) {
> +		if (hw->settings->event_settings.wakeup_reg.addr) {

	if (id == ST_LSM6DSX_ID_ACC &&
	    hw->settings->event_settings.wakeup_reg.addr) {
	    ...
	}

> +			chan->event_spec = &st_lsm6dsx_event;
> +			chan->num_event_specs = 1;
> +		}
> +	}
> +	return 0;
> +}
> +
>  static struct iio_dev *st_lsm6dsx_alloc_iiodev(struct st_lsm6dsx_hw *hw,
>  					       enum st_lsm6dsx_sensor_id id,
>  					       const char *name)
>  {
>  	struct st_lsm6dsx_sensor *sensor;
>  	struct iio_dev *iio_dev;
> +	struct iio_chan_spec *channels;

nit: chan to be consistent

> +	int i;
>  
>  	iio_dev = devm_iio_device_alloc(hw->dev, sizeof(*sensor));
>  	if (!iio_dev)
>  		return NULL;
>  
> +	channels = devm_kzalloc(hw->dev, sizeof(*channels) * ST_LSM6DSX_CHAN_COUNT, GFP_KERNEL);

79 column limit here. I guess you can use even devm_kcalloc() here.

> +	if (!channels)
> +		return NULL;
> +
> +	for (i = 0; i < 3; i++) {
> +		if (st_lsm6dsx_chan_init(channels, hw, id, i) < 0)
> +			return NULL;
> +	}

new line here.

> +	channels[3].type = IIO_TIMESTAMP;
> +	channels[3].channel = -1;
> +	channels[3].scan_index = 3;
> +	channels[3].scan_type.sign = 's';
> +	channels[3].scan_type.realbits = 64;
> +	channels[3].scan_type.storagebits = 64;
>  	iio_dev->modes = INDIO_DIRECT_MODE;
>  	iio_dev->available_scan_masks = st_lsm6dsx_available_scan_masks;
> -	iio_dev->channels = hw->settings->channels[id].chan;
> -	iio_dev->num_channels = hw->settings->channels[id].len;
> +	iio_dev->channels = channels;
> +	iio_dev->num_channels = ST_LSM6DSX_CHAN_COUNT;
>  
>  	sensor = iio_priv(iio_dev);
>  	sensor->id = id;
> -- 
> 2.39.5
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/9] iio: imu: st_lsm6dsx: dynamically initialize iio_chan_spec data

[Francesco Lavra]

[-- Attachment #1: Type: text/plain, Size: 5494 bytes --]

On Thu, 2025-10-30 at 17:42 +0100, Lorenzo Bianconi wrote:
> > Using the ST_LSM6DSX_CHANNEL_ACC() macro as a static initializer
> > for the iio_chan_spec struct arrays makes all sensors advertise
> > channel event capabilities regardless of whether they actually
> > support event generation. And if userspace tries to configure
> > accelerometer wakeup events on a sensor device that does not
> > support them (e.g. LSM6DS0), st_lsm6dsx_write_event() dereferences
> > a NULL pointer when trying to write to the wakeup register.
> > Replace usage of the ST_LSM6DSX_CHANNEL_ACC() and
> > ST_LSM6DSX_CHANNEL() macros with dynamic allocation and
> > initialization of struct iio_chan_spec arrays, where the
> > st_lsm6dsx_event structure is only used for sensors that support
> > wakeup events; besides fixing the above bug, this serves as a
> > preliminary step for adding support for more event types.
> 
> I agree we are missing the Fixes tag here.

Ack

[...]


> > +static int st_lsm6dsx_chan_init(struct iio_chan_spec *channels, struct
> > st_lsm6dsx_hw *hw,
> > +                               enum st_lsm6dsx_sensor_id id, int
> > index)
> 
> please try to respect the 79 column limit (I still like it :))

OK

> > +{
> > +       struct iio_chan_spec *chan = &channels[index];
> > +
> > +       chan->type = (id == ST_LSM6DSX_ID_ACC) ? IIO_ACCEL :
> > IIO_ANGL_VEL;
> 
> I think you should return an error here if id is not ST_LSM6DSX_ID_ACC or
> ST_LSM6DSX_ID_GYRO.

Will do

> > +       chan->address = hw->settings->chan_addr_base[id] + index *
> > ST_LSM6DSX_CHAN_SIZE;
> > +       chan->modified = 1;
> > +       chan->channel2 = IIO_MOD_X + index;
> > +       chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> > +       chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
> > +       chan->info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ);
> > +       chan->scan_index = index;
> > +       chan->scan_type.sign = 's';
> > +       chan->scan_type.realbits = 16;
> > +       chan->scan_type.storagebits = 16;
> > +       chan->scan_type.endianness = IIO_LE;
> 
> what about reducing the scope of
> ST_LSM6DSX_CHANNEL_ACC/ST_LSM6DSX_CHANNEL here
> to improve the iio_chan_spec struct initialization since most of the
> fields are
> always the same between different sensors.

Do you mean declaring a local struct variable initialized via
ST_LSM6DSX_CHANNEL() and then copying its contents to the dynamically
allocated struct? I'm not clear what benefits that would give us; in fact,
I think it would increase the code size (both in terms of LOC and compiled
binary size), besides the additional overhead of memory copying.

> 
> > +       chan->ext_info = st_lsm6dsx_ext_info;
> > +       if (id == ST_LSM6DSX_ID_ACC) {
> > +               if (hw->settings->event_settings.wakeup_reg.addr) {
> 
>         if (id == ST_LSM6DSX_ID_ACC &&
>             hw->settings->event_settings.wakeup_reg.addr) {
>             ...
>         }
> 
> > +                       chan->event_spec = &st_lsm6dsx_event;
> > +                       chan->num_event_specs = 1;
> > +               }
> > +       }
> > +       return 0;
> > +}
> > +
> >  static struct iio_dev *st_lsm6dsx_alloc_iiodev(struct st_lsm6dsx_hw
> > *hw,
> >                                                enum
> > st_lsm6dsx_sensor_id id,
> >                                                const char *name)
> >  {
> >         struct st_lsm6dsx_sensor *sensor;
> >         struct iio_dev *iio_dev;
> > +       struct iio_chan_spec *channels;
> 
> nit: chan to be consistent
> 
> > +       int i;
> >  
> >         iio_dev = devm_iio_device_alloc(hw->dev, sizeof(*sensor));
> >         if (!iio_dev)
> >                 return NULL;
> >  
> > +       channels = devm_kzalloc(hw->dev, sizeof(*channels) *
> > ST_LSM6DSX_CHAN_COUNT, GFP_KERNEL);
> 
> 79 column limit here. I guess you can use even devm_kcalloc() here.

Will do

> > +       if (!channels)
> > +               return NULL;
> > +
> > +       for (i = 0; i < 3; i++) {
> > +               if (st_lsm6dsx_chan_init(channels, hw, id, i) < 0)
> > +                       return NULL;
> > +       }
> 
> new line here.
> 
> > +       channels[3].type = IIO_TIMESTAMP;
> > +       channels[3].channel = -1;
> > +       channels[3].scan_index = 3;
> > +       channels[3].scan_type.sign = 's';
> > +       channels[3].scan_type.realbits = 64;
> > +       channels[3].scan_type.storagebits = 64;
> >         iio_dev->modes = INDIO_DIRECT_MODE;
> >         iio_dev->available_scan_masks =
> > st_lsm6dsx_available_scan_masks;
> > -       iio_dev->channels = hw->settings->channels[id].chan;
> > -       iio_dev->num_channels = hw->settings->channels[id].len;
> > +       iio_dev->channels = channels;
> > +       iio_dev->num_channels = ST_LSM6DSX_CHAN_COUNT;
> >  
> >         sensor = iio_priv(iio_dev);
> >         sensor->id = id;
> > -- 
> > 2.39.5
> > 


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 1/9] iio: imu: st_lsm6dsx: dynamically initialize iio_chan_spec data

[Andy Shevchenko]

On Fri, Oct 31, 2025 at 09:04:58AM +0100, Francesco Lavra wrote:
> On Thu, 2025-10-30 at 17:42 +0100, Lorenzo Bianconi wrote:

[...]

> > > +static int st_lsm6dsx_chan_init(struct iio_chan_spec *channels, struct
> > > st_lsm6dsx_hw *hw,
> > > +                               enum st_lsm6dsx_sensor_id id, int
> > > index)
> > 
> > please try to respect the 79 column limit (I still like it :))
> 
> OK

FWIW, the limit is the exact 80. Don't waste that 1 characters when it's the case.

(And moreover there is a note in the documentation that allows, and we follow
 that from time to time in IIO, slightly longer lines if it really increases
 readability.)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/9] iio: imu: st_lsm6dsx: dynamically initialize iio_chan_spec data

[Francesco Lavra]

[-- Attachment #1: Type: text/plain, Size: 583 bytes --]

[missed this one]

On Thu, 2025-10-30 at 17:42 +0100, Lorenzo Bianconi wrote:
> 
> > +       chan->ext_info = st_lsm6dsx_ext_info;
> > +       if (id == ST_LSM6DSX_ID_ACC) {
> > +               if (hw->settings->event_settings.wakeup_reg.addr) {
> 
>         if (id == ST_LSM6DSX_ID_ACC &&
>             hw->settings->event_settings.wakeup_reg.addr) {
>             ...
>         }

In patch 4/9, the inner conditional will be replaced by more generic code,
so we would revert to if (id == ST_LSM6DSX_ID_ACC) [...]


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 1/9] iio: imu: st_lsm6dsx: dynamically initialize iio_chan_spec data

[Andy Shevchenko]

On Fri, Oct 31, 2025 at 09:26:19AM +0100, Francesco Lavra wrote:
> On Thu, 2025-10-30 at 17:42 +0100, Lorenzo Bianconi wrote:

> > > +       chan->ext_info = st_lsm6dsx_ext_info;
> > > +       if (id == ST_LSM6DSX_ID_ACC) {
> > > +               if (hw->settings->event_settings.wakeup_reg.addr) {
> > 
> >         if (id == ST_LSM6DSX_ID_ACC &&
> >             hw->settings->event_settings.wakeup_reg.addr) {
> >             ...
> >         }
> 
> In patch 4/9, the inner conditional will be replaced by more generic code,
> so we would revert to if (id == ST_LSM6DSX_ID_ACC) [...]

Hmm... The obvious follow up question is why can't we stick with the original
conditional to begin with?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/9] iio: imu: st_lsm6dsx: dynamically initialize iio_chan_spec data

[Francesco Lavra]

[-- Attachment #1: Type: text/plain, Size: 1247 bytes --]

On Fri, 2025-10-31 at 10:32 +0200, Andy Shevchenko wrote:
> On Fri, Oct 31, 2025 at 09:26:19AM +0100, Francesco Lavra wrote:
> > On Thu, 2025-10-30 at 17:42 +0100, Lorenzo Bianconi wrote:
> 
> > > > +       chan->ext_info = st_lsm6dsx_ext_info;
> > > > +       if (id == ST_LSM6DSX_ID_ACC) {
> > > > +               if (hw->settings->event_settings.wakeup_reg.addr) {
> > > 
> > >         if (id == ST_LSM6DSX_ID_ACC &&
> > >             hw->settings->event_settings.wakeup_reg.addr) {
> > >             ...
> > >         }
> > 
> > In patch 4/9, the inner conditional will be replaced by more generic
> > code,
> > so we would revert to if (id == ST_LSM6DSX_ID_ACC) [...]
> 
> Hmm... The obvious follow up question is why can't we stick with the
> original
> conditional to begin with?

There is no original conditional, this is new code.
So the code here is `if (cond1) {if (cond2) {}}`; in patch 4/9 it will
become `if (cond1) {something else}`.
Or, better yet, as you suggested earlier, in the next revision the code
here will be `if (cond1) helper()`, and in the patch 4/9 this will stay the
same and only the code inside the helper will change.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 1/9] iio: imu: st_lsm6dsx: dynamically initialize iio_chan_spec data

[Jonathan Cameron]

On Thu, 30 Oct 2025 08:27:44 +0100
Francesco Lavra <flavra@baylibre.com> wrote:

> Using the ST_LSM6DSX_CHANNEL_ACC() macro as a static initializer
> for the iio_chan_spec struct arrays makes all sensors advertise
> channel event capabilities regardless of whether they actually
> support event generation. And if userspace tries to configure
> accelerometer wakeup events on a sensor device that does not
> support them (e.g. LSM6DS0), st_lsm6dsx_write_event() dereferences
> a NULL pointer when trying to write to the wakeup register.
> Replace usage of the ST_LSM6DSX_CHANNEL_ACC() and
> ST_LSM6DSX_CHANNEL() macros with dynamic allocation and
> initialization of struct iio_chan_spec arrays, where the
> st_lsm6dsx_event structure is only used for sensors that support
> wakeup events; besides fixing the above bug, this serves as a
> preliminary step for adding support for more event types.
> 
> Signed-off-by: Francesco Lavra <flavra@baylibre.com>

In cases where there are only a small number of options for what the channel
arrays should contain, my normal preference would be more data over moving
the complexity into code.  That is have two struct iio_chan_spec arrays and
pick between them based on availability of the interrupt.

I haven't checked the whole series yet, but how many channel arrays
would we need to support the features you are introducing here? That is
how many different combinations exist in the supported chips?

Jonathan

Re: [PATCH 1/9] iio: imu: st_lsm6dsx: dynamically initialize iio_chan_spec data

[Francesco Lavra]

[-- Attachment #1: Type: text/plain, Size: 1939 bytes --]

On Sun, 2025-11-02 at 11:16 +0000, Jonathan Cameron wrote:
> On Thu, 30 Oct 2025 08:27:44 +0100
> Francesco Lavra <flavra@baylibre.com> wrote:
> 
> > Using the ST_LSM6DSX_CHANNEL_ACC() macro as a static initializer
> > for the iio_chan_spec struct arrays makes all sensors advertise
> > channel event capabilities regardless of whether they actually
> > support event generation. And if userspace tries to configure
> > accelerometer wakeup events on a sensor device that does not
> > support them (e.g. LSM6DS0), st_lsm6dsx_write_event() dereferences
> > a NULL pointer when trying to write to the wakeup register.
> > Replace usage of the ST_LSM6DSX_CHANNEL_ACC() and
> > ST_LSM6DSX_CHANNEL() macros with dynamic allocation and
> > initialization of struct iio_chan_spec arrays, where the
> > st_lsm6dsx_event structure is only used for sensors that support
> > wakeup events; besides fixing the above bug, this serves as a
> > preliminary step for adding support for more event types.
> > 
> > Signed-off-by: Francesco Lavra <flavra@baylibre.com>
> 
> In cases where there are only a small number of options for what the
> channel
> arrays should contain, my normal preference would be more data over
> moving
> the complexity into code.  That is have two struct iio_chan_spec arrays
> and
> pick between them based on availability of the interrupt.
> 
> I haven't checked the whole series yet, but how many channel arrays
> would we need to support the features you are introducing here? That is
> how many different combinations exist in the supported chips?

In the current code there are 3 struct iio_chan_spec arrays; we would need
one more to fix the above bug, and one more to add tap event support; so a
total of 5 arrays (each of length 4).
As for struct iio_event_spec, the current code has one array (of length 1),
and to add tap event support we would need another array (of length 2).

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 1/9] iio: imu: st_lsm6dsx: dynamically initialize iio_chan_spec data

[Jonathan Cameron]

On Mon, 03 Nov 2025 10:24:54 +0100
Francesco Lavra <flavra@baylibre.com> wrote:

> On Sun, 2025-11-02 at 11:16 +0000, Jonathan Cameron wrote:
> > On Thu, 30 Oct 2025 08:27:44 +0100
> > Francesco Lavra <flavra@baylibre.com> wrote:
> >   
> > > Using the ST_LSM6DSX_CHANNEL_ACC() macro as a static initializer
> > > for the iio_chan_spec struct arrays makes all sensors advertise
> > > channel event capabilities regardless of whether they actually
> > > support event generation. And if userspace tries to configure
> > > accelerometer wakeup events on a sensor device that does not
> > > support them (e.g. LSM6DS0), st_lsm6dsx_write_event() dereferences
> > > a NULL pointer when trying to write to the wakeup register.
> > > Replace usage of the ST_LSM6DSX_CHANNEL_ACC() and
> > > ST_LSM6DSX_CHANNEL() macros with dynamic allocation and
> > > initialization of struct iio_chan_spec arrays, where the
> > > st_lsm6dsx_event structure is only used for sensors that support
> > > wakeup events; besides fixing the above bug, this serves as a
> > > preliminary step for adding support for more event types.
> > > 
> > > Signed-off-by: Francesco Lavra <flavra@baylibre.com>  
> > 
> > In cases where there are only a small number of options for what the
> > channel
> > arrays should contain, my normal preference would be more data over
> > moving
> > the complexity into code.  That is have two struct iio_chan_spec arrays
> > and
> > pick between them based on availability of the interrupt.
> > 
> > I haven't checked the whole series yet, but how many channel arrays
> > would we need to support the features you are introducing here? That is
> > how many different combinations exist in the supported chips?  
> 
> In the current code there are 3 struct iio_chan_spec arrays; we would need
> one more to fix the above bug, and one more to add tap event support; so a
> total of 5 arrays (each of length 4).
> As for struct iio_event_spec, the current code has one array (of length 1),
> and to add tap event support we would need another array (of length 2).

That sounds small enough to me that I'd prefer const data that you pick between
rather than dynamic creation.

Jonathan

[PATCH 2/9] iio: imu: st_lsm6dsx: make event_settings more generic

[Francesco Lavra]

The st_lsm6dsx_event_settings structure contains fields specific
for one event type (wakeup). In preparation for adding support for
more event types, introduce an event id enum and a generic event
source structure, and replace wakeup-specific data in struct
st_lsm6dsx_event_settings with an array of event source structures.

Signed-off-by: Francesco Lavra <flavra@baylibre.com>
---
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  21 ++-
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 136 +++++++++++--------
 2 files changed, 96 insertions(+), 61 deletions(-)

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
index db863bd1898d..05689887f7ec 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
@@ -221,14 +221,23 @@ struct st_lsm6dsx_shub_settings {
 	u8 pause;
 };
 
+enum st_lsm6dsx_event_id {
+	ST_LSM6DSX_EVENT_WAKEUP,
+	ST_LSM6DSX_EVENT_MAX
+};
+
+struct st_lsm6dsx_event_src {
+	struct st_lsm6dsx_reg value;
+	u8 status_reg;
+	u8 status_mask;
+	u8 status_x_mask;
+	u8 status_y_mask;
+	u8 status_z_mask;
+};
+
 struct st_lsm6dsx_event_settings {
 	struct st_lsm6dsx_reg enable_reg;
-	struct st_lsm6dsx_reg wakeup_reg;
-	u8 wakeup_src_reg;
-	u8 wakeup_src_status_mask;
-	u8 wakeup_src_z_mask;
-	u8 wakeup_src_y_mask;
-	u8 wakeup_src_x_mask;
+	struct st_lsm6dsx_event_src sources[ST_LSM6DSX_EVENT_MAX];
 };
 
 enum st_lsm6dsx_ext_sensor_id {
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index 17b46e15cce5..bb4c4c531128 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -350,15 +350,19 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 			},
 		},
 		.event_settings = {
-			.wakeup_reg = {
-				.addr = 0x5B,
-				.mask = GENMASK(5, 0),
+			.sources = {
+				[ST_LSM6DSX_EVENT_WAKEUP] = {
+					.value = {
+						.addr = 0x5b,
+						.mask = GENMASK(5, 0),
+					},
+					.status_reg = 0x1b,
+					.status_mask = BIT(3),
+					.status_z_mask = BIT(0),
+					.status_y_mask = BIT(1),
+					.status_x_mask = BIT(2),
+				},
 			},
-			.wakeup_src_reg = 0x1b,
-			.wakeup_src_status_mask = BIT(3),
-			.wakeup_src_z_mask = BIT(0),
-			.wakeup_src_y_mask = BIT(1),
-			.wakeup_src_x_mask = BIT(2),
 		},
 	},
 	{
@@ -510,15 +514,19 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 			},
 		},
 		.event_settings = {
-			.wakeup_reg = {
-				.addr = 0x5B,
-				.mask = GENMASK(5, 0),
+			.sources = {
+				[ST_LSM6DSX_EVENT_WAKEUP] = {
+					.value = {
+						.addr = 0x5b,
+						.mask = GENMASK(5, 0),
+					},
+					.status_reg = 0x1b,
+					.status_mask = BIT(3),
+					.status_z_mask = BIT(0),
+					.status_y_mask = BIT(1),
+					.status_x_mask = BIT(2),
+				},
 			},
-			.wakeup_src_reg = 0x1b,
-			.wakeup_src_status_mask = BIT(3),
-			.wakeup_src_z_mask = BIT(0),
-			.wakeup_src_y_mask = BIT(1),
-			.wakeup_src_x_mask = BIT(2),
 		},
 	},
 	{
@@ -741,15 +749,19 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 				.addr = 0x58,
 				.mask = BIT(7),
 			},
-			.wakeup_reg = {
-				.addr = 0x5B,
-				.mask = GENMASK(5, 0),
+			.sources = {
+				[ST_LSM6DSX_EVENT_WAKEUP] = {
+					.value = {
+						.addr = 0x5b,
+						.mask = GENMASK(5, 0),
+					},
+					.status_reg = 0x1b,
+					.status_mask = BIT(3),
+					.status_z_mask = BIT(0),
+					.status_y_mask = BIT(1),
+					.status_x_mask = BIT(2),
+				},
 			},
-			.wakeup_src_reg = 0x1b,
-			.wakeup_src_status_mask = BIT(3),
-			.wakeup_src_z_mask = BIT(0),
-			.wakeup_src_y_mask = BIT(1),
-			.wakeup_src_x_mask = BIT(2),
 		},
 	},
 	{
@@ -972,15 +984,19 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 				.addr = 0x58,
 				.mask = BIT(7),
 			},
-			.wakeup_reg = {
-				.addr = 0x5b,
-				.mask = GENMASK(5, 0),
+			.sources = {
+				[ST_LSM6DSX_EVENT_WAKEUP] = {
+					.value = {
+						.addr = 0x5b,
+						.mask = GENMASK(5, 0),
+					},
+					.status_reg = 0x1b,
+					.status_mask = BIT(3),
+					.status_z_mask = BIT(0),
+					.status_y_mask = BIT(1),
+					.status_x_mask = BIT(2),
+				},
 			},
-			.wakeup_src_reg = 0x1b,
-			.wakeup_src_status_mask = BIT(3),
-			.wakeup_src_z_mask = BIT(0),
-			.wakeup_src_y_mask = BIT(1),
-			.wakeup_src_x_mask = BIT(2),
 		},
 	},
 	{
@@ -1147,15 +1163,19 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 				.addr = 0x58,
 				.mask = BIT(7),
 			},
-			.wakeup_reg = {
-				.addr = 0x5B,
-				.mask = GENMASK(5, 0),
+			.sources = {
+				[ST_LSM6DSX_EVENT_WAKEUP] = {
+					.value = {
+						.addr = 0x5b,
+						.mask = GENMASK(5, 0),
+					},
+					.status_reg = 0x1b,
+					.status_mask = BIT(3),
+					.status_z_mask = BIT(0),
+					.status_y_mask = BIT(1),
+					.status_x_mask = BIT(2),
+				},
 			},
-			.wakeup_src_reg = 0x1b,
-			.wakeup_src_status_mask = BIT(3),
-			.wakeup_src_z_mask = BIT(0),
-			.wakeup_src_y_mask = BIT(1),
-			.wakeup_src_x_mask = BIT(2),
 		},
 	},
 	{
@@ -1347,15 +1367,19 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 				.addr = 0x50,
 				.mask = BIT(7),
 			},
-			.wakeup_reg = {
-				.addr = 0x5b,
-				.mask = GENMASK(5, 0),
+			.sources = {
+				[ST_LSM6DSX_EVENT_WAKEUP] = {
+					.value = {
+						.addr = 0x5b,
+						.mask = GENMASK(5, 0),
+					},
+					.status_reg = 0x45,
+					.status_mask = BIT(3),
+					.status_z_mask = BIT(0),
+					.status_y_mask = BIT(1),
+					.status_x_mask = BIT(2),
+				},
 			},
-			.wakeup_src_reg = 0x45,
-			.wakeup_src_status_mask = BIT(3),
-			.wakeup_src_z_mask = BIT(0),
-			.wakeup_src_y_mask = BIT(1),
-			.wakeup_src_x_mask = BIT(2),
 		},
 	},
 	{
@@ -1861,7 +1885,7 @@ st_lsm6dsx_write_event(struct iio_dev *iio_dev,
 	if (val < 0 || val > 31)
 		return -EINVAL;
 
-	reg = &hw->settings->event_settings.wakeup_reg;
+	reg = &hw->settings->event_settings.sources[ST_LSM6DSX_EVENT_WAKEUP].value;
 	data = ST_LSM6DSX_SHIFT_VAL(val, reg->mask);
 	err = st_lsm6dsx_update_bits_locked(hw, reg->addr,
 					    reg->mask, data);
@@ -2318,7 +2342,7 @@ static int st_lsm6dsx_chan_init(struct iio_chan_spec *channels, struct st_lsm6ds
 	chan->scan_type.endianness = IIO_LE;
 	chan->ext_info = st_lsm6dsx_ext_info;
 	if (id == ST_LSM6DSX_ID_ACC) {
-		if (hw->settings->event_settings.wakeup_reg.addr) {
+		if (hw->settings->event_settings.sources[ST_LSM6DSX_EVENT_WAKEUP].value.addr) {
 			chan->event_spec = &st_lsm6dsx_event;
 			chan->num_event_specs = 1;
 		}
@@ -2389,6 +2413,7 @@ static bool
 st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw)
 {
 	const struct st_lsm6dsx_event_settings *event_settings;
+	const struct st_lsm6dsx_event_src *event_src;
 	int err, data;
 	s64 timestamp;
 
@@ -2396,13 +2421,14 @@ st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw)
 		return false;
 
 	event_settings = &hw->settings->event_settings;
-	err = st_lsm6dsx_read_locked(hw, event_settings->wakeup_src_reg,
+	event_src = &event_settings->sources[ST_LSM6DSX_EVENT_WAKEUP];
+	err = st_lsm6dsx_read_locked(hw, event_src->status_reg,
 				     &data, sizeof(data));
 	if (err < 0)
 		return false;
 
 	timestamp = iio_get_time_ns(hw->iio_devs[ST_LSM6DSX_ID_ACC]);
-	if ((data & hw->settings->event_settings.wakeup_src_z_mask) &&
+	if ((data & event_src->status_z_mask) &&
 	    (hw->enable_event & BIT(IIO_MOD_Z)))
 		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
 			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
@@ -2412,7 +2438,7 @@ st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw)
 						  IIO_EV_DIR_EITHER),
 						  timestamp);
 
-	if ((data & hw->settings->event_settings.wakeup_src_y_mask) &&
+	if ((data & event_src->status_y_mask) &&
 	    (hw->enable_event & BIT(IIO_MOD_Y)))
 		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
 			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
@@ -2422,7 +2448,7 @@ st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw)
 						  IIO_EV_DIR_EITHER),
 						  timestamp);
 
-	if ((data & hw->settings->event_settings.wakeup_src_x_mask) &&
+	if ((data & event_src->status_x_mask) &&
 	    (hw->enable_event & BIT(IIO_MOD_X)))
 		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
 			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
@@ -2432,7 +2458,7 @@ st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw)
 						  IIO_EV_DIR_EITHER),
 						  timestamp);
 
-	return data & event_settings->wakeup_src_status_mask;
+	return data & event_src->status_mask;
 }
 
 static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
-- 
2.39.5

Re: [PATCH 2/9] iio: imu: st_lsm6dsx: make event_settings more generic

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 9521 bytes --]

On Oct 30, Francesco Lavra wrote:
> The st_lsm6dsx_event_settings structure contains fields specific
> for one event type (wakeup). In preparation for adding support for
> more event types, introduce an event id enum and a generic event
> source structure, and replace wakeup-specific data in struct
> st_lsm6dsx_event_settings with an array of event source structures.
> 
> Signed-off-by: Francesco Lavra <flavra@baylibre.com>
> ---
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  21 ++-
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 136 +++++++++++--------
>  2 files changed, 96 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> index db863bd1898d..05689887f7ec 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> @@ -221,14 +221,23 @@ struct st_lsm6dsx_shub_settings {
>  	u8 pause;
>  };
>  
> +enum st_lsm6dsx_event_id {
> +	ST_LSM6DSX_EVENT_WAKEUP,
> +	ST_LSM6DSX_EVENT_MAX
> +};
> +
> +struct st_lsm6dsx_event_src {
> +	struct st_lsm6dsx_reg value;
> +	u8 status_reg;
> +	u8 status_mask;
> +	u8 status_x_mask;
> +	u8 status_y_mask;
> +	u8 status_z_mask;

you can use st_lsm6dsx_reg here.

> +};
> +
>  struct st_lsm6dsx_event_settings {
>  	struct st_lsm6dsx_reg enable_reg;
> -	struct st_lsm6dsx_reg wakeup_reg;
> -	u8 wakeup_src_reg;
> -	u8 wakeup_src_status_mask;
> -	u8 wakeup_src_z_mask;
> -	u8 wakeup_src_y_mask;
> -	u8 wakeup_src_x_mask;
> +	struct st_lsm6dsx_event_src sources[ST_LSM6DSX_EVENT_MAX];
>  };
>  
>  enum st_lsm6dsx_ext_sensor_id {
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> index 17b46e15cce5..bb4c4c531128 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> @@ -350,15 +350,19 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>  			},
>  		},
>  		.event_settings = {
> -			.wakeup_reg = {
> -				.addr = 0x5B,
> -				.mask = GENMASK(5, 0),
> +			.sources = {
> +				[ST_LSM6DSX_EVENT_WAKEUP] = {
> +					.value = {
> +						.addr = 0x5b,
> +						.mask = GENMASK(5, 0),
> +					},
> +					.status_reg = 0x1b,
> +					.status_mask = BIT(3),
> +					.status_z_mask = BIT(0),
> +					.status_y_mask = BIT(1),
> +					.status_x_mask = BIT(2),
> +				},
>  			},
> -			.wakeup_src_reg = 0x1b,
> -			.wakeup_src_status_mask = BIT(3),
> -			.wakeup_src_z_mask = BIT(0),
> -			.wakeup_src_y_mask = BIT(1),
> -			.wakeup_src_x_mask = BIT(2),
>  		},
>  	},
>  	{
> @@ -510,15 +514,19 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>  			},
>  		},
>  		.event_settings = {
> -			.wakeup_reg = {
> -				.addr = 0x5B,
> -				.mask = GENMASK(5, 0),
> +			.sources = {
> +				[ST_LSM6DSX_EVENT_WAKEUP] = {
> +					.value = {
> +						.addr = 0x5b,
> +						.mask = GENMASK(5, 0),
> +					},
> +					.status_reg = 0x1b,
> +					.status_mask = BIT(3),
> +					.status_z_mask = BIT(0),
> +					.status_y_mask = BIT(1),
> +					.status_x_mask = BIT(2),
> +				},
>  			},
> -			.wakeup_src_reg = 0x1b,
> -			.wakeup_src_status_mask = BIT(3),
> -			.wakeup_src_z_mask = BIT(0),
> -			.wakeup_src_y_mask = BIT(1),
> -			.wakeup_src_x_mask = BIT(2),
>  		},
>  	},
>  	{
> @@ -741,15 +749,19 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>  				.addr = 0x58,
>  				.mask = BIT(7),
>  			},
> -			.wakeup_reg = {
> -				.addr = 0x5B,
> -				.mask = GENMASK(5, 0),
> +			.sources = {
> +				[ST_LSM6DSX_EVENT_WAKEUP] = {
> +					.value = {
> +						.addr = 0x5b,
> +						.mask = GENMASK(5, 0),
> +					},
> +					.status_reg = 0x1b,
> +					.status_mask = BIT(3),
> +					.status_z_mask = BIT(0),
> +					.status_y_mask = BIT(1),
> +					.status_x_mask = BIT(2),
> +				},
>  			},
> -			.wakeup_src_reg = 0x1b,
> -			.wakeup_src_status_mask = BIT(3),
> -			.wakeup_src_z_mask = BIT(0),
> -			.wakeup_src_y_mask = BIT(1),
> -			.wakeup_src_x_mask = BIT(2),
>  		},
>  	},
>  	{
> @@ -972,15 +984,19 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>  				.addr = 0x58,
>  				.mask = BIT(7),
>  			},
> -			.wakeup_reg = {
> -				.addr = 0x5b,
> -				.mask = GENMASK(5, 0),
> +			.sources = {
> +				[ST_LSM6DSX_EVENT_WAKEUP] = {
> +					.value = {
> +						.addr = 0x5b,
> +						.mask = GENMASK(5, 0),
> +					},
> +					.status_reg = 0x1b,
> +					.status_mask = BIT(3),
> +					.status_z_mask = BIT(0),
> +					.status_y_mask = BIT(1),
> +					.status_x_mask = BIT(2),
> +				},
>  			},
> -			.wakeup_src_reg = 0x1b,
> -			.wakeup_src_status_mask = BIT(3),
> -			.wakeup_src_z_mask = BIT(0),
> -			.wakeup_src_y_mask = BIT(1),
> -			.wakeup_src_x_mask = BIT(2),
>  		},
>  	},
>  	{
> @@ -1147,15 +1163,19 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>  				.addr = 0x58,
>  				.mask = BIT(7),
>  			},
> -			.wakeup_reg = {
> -				.addr = 0x5B,
> -				.mask = GENMASK(5, 0),
> +			.sources = {
> +				[ST_LSM6DSX_EVENT_WAKEUP] = {
> +					.value = {
> +						.addr = 0x5b,
> +						.mask = GENMASK(5, 0),
> +					},
> +					.status_reg = 0x1b,
> +					.status_mask = BIT(3),
> +					.status_z_mask = BIT(0),
> +					.status_y_mask = BIT(1),
> +					.status_x_mask = BIT(2),
> +				},
>  			},
> -			.wakeup_src_reg = 0x1b,
> -			.wakeup_src_status_mask = BIT(3),
> -			.wakeup_src_z_mask = BIT(0),
> -			.wakeup_src_y_mask = BIT(1),
> -			.wakeup_src_x_mask = BIT(2),
>  		},
>  	},
>  	{
> @@ -1347,15 +1367,19 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>  				.addr = 0x50,
>  				.mask = BIT(7),
>  			},
> -			.wakeup_reg = {
> -				.addr = 0x5b,
> -				.mask = GENMASK(5, 0),
> +			.sources = {
> +				[ST_LSM6DSX_EVENT_WAKEUP] = {
> +					.value = {
> +						.addr = 0x5b,
> +						.mask = GENMASK(5, 0),
> +					},
> +					.status_reg = 0x45,
> +					.status_mask = BIT(3),
> +					.status_z_mask = BIT(0),
> +					.status_y_mask = BIT(1),
> +					.status_x_mask = BIT(2),
> +				},
>  			},
> -			.wakeup_src_reg = 0x45,
> -			.wakeup_src_status_mask = BIT(3),
> -			.wakeup_src_z_mask = BIT(0),
> -			.wakeup_src_y_mask = BIT(1),
> -			.wakeup_src_x_mask = BIT(2),
>  		},
>  	},
>  	{
> @@ -1861,7 +1885,7 @@ st_lsm6dsx_write_event(struct iio_dev *iio_dev,
>  	if (val < 0 || val > 31)
>  		return -EINVAL;
>  
> -	reg = &hw->settings->event_settings.wakeup_reg;
> +	reg = &hw->settings->event_settings.sources[ST_LSM6DSX_EVENT_WAKEUP].value;
>  	data = ST_LSM6DSX_SHIFT_VAL(val, reg->mask);
>  	err = st_lsm6dsx_update_bits_locked(hw, reg->addr,
>  					    reg->mask, data);
> @@ -2318,7 +2342,7 @@ static int st_lsm6dsx_chan_init(struct iio_chan_spec *channels, struct st_lsm6ds
>  	chan->scan_type.endianness = IIO_LE;
>  	chan->ext_info = st_lsm6dsx_ext_info;
>  	if (id == ST_LSM6DSX_ID_ACC) {
> -		if (hw->settings->event_settings.wakeup_reg.addr) {
> +		if (hw->settings->event_settings.sources[ST_LSM6DSX_EVENT_WAKEUP].value.addr) {
>  			chan->event_spec = &st_lsm6dsx_event;
>  			chan->num_event_specs = 1;
>  		}
> @@ -2389,6 +2413,7 @@ static bool
>  st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw)
>  {
>  	const struct st_lsm6dsx_event_settings *event_settings;
> +	const struct st_lsm6dsx_event_src *event_src;
>  	int err, data;
>  	s64 timestamp;
>  
> @@ -2396,13 +2421,14 @@ st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw)
>  		return false;
>  
>  	event_settings = &hw->settings->event_settings;
> -	err = st_lsm6dsx_read_locked(hw, event_settings->wakeup_src_reg,
> +	event_src = &event_settings->sources[ST_LSM6DSX_EVENT_WAKEUP];
> +	err = st_lsm6dsx_read_locked(hw, event_src->status_reg,
>  				     &data, sizeof(data));
>  	if (err < 0)
>  		return false;
>  
>  	timestamp = iio_get_time_ns(hw->iio_devs[ST_LSM6DSX_ID_ACC]);
> -	if ((data & hw->settings->event_settings.wakeup_src_z_mask) &&
> +	if ((data & event_src->status_z_mask) &&
>  	    (hw->enable_event & BIT(IIO_MOD_Z)))
>  		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
>  			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
> @@ -2412,7 +2438,7 @@ st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw)
>  						  IIO_EV_DIR_EITHER),
>  						  timestamp);
>  
> -	if ((data & hw->settings->event_settings.wakeup_src_y_mask) &&
> +	if ((data & event_src->status_y_mask) &&
>  	    (hw->enable_event & BIT(IIO_MOD_Y)))
>  		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
>  			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
> @@ -2422,7 +2448,7 @@ st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw)
>  						  IIO_EV_DIR_EITHER),
>  						  timestamp);
>  
> -	if ((data & hw->settings->event_settings.wakeup_src_x_mask) &&
> +	if ((data & event_src->status_x_mask) &&
>  	    (hw->enable_event & BIT(IIO_MOD_X)))
>  		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
>  			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
> @@ -2432,7 +2458,7 @@ st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw)
>  						  IIO_EV_DIR_EITHER),
>  						  timestamp);
>  
> -	return data & event_settings->wakeup_src_status_mask;
> +	return data & event_src->status_mask;
>  }
>  
>  static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
> -- 
> 2.39.5
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/9] iio: imu: st_lsm6dsx: make event_settings more generic

[Francesco Lavra]

On Thu, 2025-10-30 at 17:44 +0100, Lorenzo Bianconi wrote:
> On Oct 30, Francesco Lavra wrote:
> > The st_lsm6dsx_event_settings structure contains fields specific
> > for one event type (wakeup). In preparation for adding support for
> > more event types, introduce an event id enum and a generic event
> > source structure, and replace wakeup-specific data in struct
> > st_lsm6dsx_event_settings with an array of event source structures.
> > 
> > Signed-off-by: Francesco Lavra <flavra@baylibre.com>
> > ---
> >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  21 ++-
> >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 136 +++++++++++--------
> >  2 files changed, 96 insertions(+), 61 deletions(-)
> > 
> > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> > b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> > index db863bd1898d..05689887f7ec 100644
> > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> > @@ -221,14 +221,23 @@ struct st_lsm6dsx_shub_settings {
> >         u8 pause;
> >  };
> >  
> > +enum st_lsm6dsx_event_id {
> > +       ST_LSM6DSX_EVENT_WAKEUP,
> > +       ST_LSM6DSX_EVENT_MAX
> > +};
> > +
> > +struct st_lsm6dsx_event_src {
> > +       struct st_lsm6dsx_reg value;
> > +       u8 status_reg;
> > +       u8 status_mask;
> > +       u8 status_x_mask;
> > +       u8 status_y_mask;
> > +       u8 status_z_mask;
> 
> you can use st_lsm6dsx_reg here.

Do you mean replacing the status_* fields with 4 struct st_lsm6dsx_reg
fields (one for the global status flag, and one for each axis)?

[PATCH 3/9] iio: imu: st_lsm6dsx: move wakeup event enable mask to event_src

[Francesco Lavra]

The mask value being assigned to the irq1_func and irq2_func fields
of the irq_config struct is specific to a single event source (i.e.
the wakeup event), and as such it should be separate from the
definition of the interrupt function registers, which cover
multiple event sources.
In preparation for adding support for more event types, change the
irq1_func and irq2_func type from an {address, mask} pair to an
address, and move the mask value to a new field of struct
st_lsm6dsx_event_src. No functional changes.

Signed-off-by: Francesco Lavra <flavra@baylibre.com>
---
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  7 +-
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 80 +++++++-------------
 2 files changed, 30 insertions(+), 57 deletions(-)

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
index 05689887f7ec..5c73156b714a 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
@@ -228,6 +228,7 @@ enum st_lsm6dsx_event_id {
 
 struct st_lsm6dsx_event_src {
 	struct st_lsm6dsx_reg value;
+	u8 enable_mask;
 	u8 status_reg;
 	u8 status_mask;
 	u8 status_x_mask;
@@ -320,8 +321,8 @@ struct st_lsm6dsx_settings {
 	struct {
 		struct st_lsm6dsx_reg irq1;
 		struct st_lsm6dsx_reg irq2;
-		struct st_lsm6dsx_reg irq1_func;
-		struct st_lsm6dsx_reg irq2_func;
+		u8 irq1_func;
+		u8 irq2_func;
 		struct st_lsm6dsx_reg lir;
 		struct st_lsm6dsx_reg clear_on_read;
 		struct st_lsm6dsx_reg hla;
@@ -420,7 +421,7 @@ struct st_lsm6dsx_hw {
 	u8 ts_sip;
 	u8 sip;
 
-	const struct st_lsm6dsx_reg *irq_routing;
+	u8 irq_routing;
 	u8 event_threshold;
 	u8 enable_event;
 
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index bb4c4c531128..4bae5da8910e 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -290,14 +290,8 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 				.addr = 0x58,
 				.mask = BIT(0),
 			},
-			.irq1_func = {
-				.addr = 0x5e,
-				.mask = BIT(5),
-			},
-			.irq2_func = {
-				.addr = 0x5f,
-				.mask = BIT(5),
-			},
+			.irq1_func = 0x5e,
+			.irq2_func = 0x5f,
 			.hla = {
 				.addr = 0x12,
 				.mask = BIT(5),
@@ -356,6 +350,7 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 						.addr = 0x5b,
 						.mask = GENMASK(5, 0),
 					},
+					.enable_mask = BIT(5),
 					.status_reg = 0x1b,
 					.status_mask = BIT(3),
 					.status_z_mask = BIT(0),
@@ -454,14 +449,8 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 				.addr = 0x58,
 				.mask = BIT(0),
 			},
-			.irq1_func = {
-				.addr = 0x5e,
-				.mask = BIT(5),
-			},
-			.irq2_func = {
-				.addr = 0x5f,
-				.mask = BIT(5),
-			},
+			.irq1_func = 0x5e,
+			.irq2_func = 0x5f,
 			.hla = {
 				.addr = 0x12,
 				.mask = BIT(5),
@@ -520,6 +509,7 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 						.addr = 0x5b,
 						.mask = GENMASK(5, 0),
 					},
+					.enable_mask = BIT(5),
 					.status_reg = 0x1b,
 					.status_mask = BIT(3),
 					.status_z_mask = BIT(0),
@@ -648,14 +638,8 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 				.addr = 0x58,
 				.mask = BIT(0),
 			},
-			.irq1_func = {
-				.addr = 0x5e,
-				.mask = BIT(5),
-			},
-			.irq2_func = {
-				.addr = 0x5f,
-				.mask = BIT(5),
-			},
+			.irq1_func = 0x5e,
+			.irq2_func = 0x5f,
 			.hla = {
 				.addr = 0x12,
 				.mask = BIT(5),
@@ -755,6 +739,7 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 						.addr = 0x5b,
 						.mask = GENMASK(5, 0),
 					},
+					.enable_mask = BIT(5),
 					.status_reg = 0x1b,
 					.status_mask = BIT(3),
 					.status_z_mask = BIT(0),
@@ -895,14 +880,8 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 				.addr = 0x56,
 				.mask = BIT(6),
 			},
-			.irq1_func = {
-				.addr = 0x5e,
-				.mask = BIT(5),
-			},
-			.irq2_func = {
-				.addr = 0x5f,
-				.mask = BIT(5),
-			},
+			.irq1_func = 0x5e,
+			.irq2_func = 0x5f,
 			.hla = {
 				.addr = 0x12,
 				.mask = BIT(5),
@@ -990,6 +969,7 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 						.addr = 0x5b,
 						.mask = GENMASK(5, 0),
 					},
+					.enable_mask = BIT(5),
 					.status_reg = 0x1b,
 					.status_mask = BIT(3),
 					.status_z_mask = BIT(0),
@@ -1106,14 +1086,8 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 				.addr = 0x56,
 				.mask = BIT(6),
 			},
-			.irq1_func = {
-				.addr = 0x5e,
-				.mask = BIT(5),
-			},
-			.irq2_func = {
-				.addr = 0x5f,
-				.mask = BIT(5),
-			},
+			.irq1_func = 0x5e,
+			.irq2_func = 0x5f,
 			.hla = {
 				.addr = 0x12,
 				.mask = BIT(5),
@@ -1169,6 +1143,7 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 						.addr = 0x5b,
 						.mask = GENMASK(5, 0),
 					},
+					.enable_mask = BIT(5),
 					.status_reg = 0x1b,
 					.status_mask = BIT(3),
 					.status_z_mask = BIT(0),
@@ -1279,14 +1254,8 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 				.addr = 0x56,
 				.mask = BIT(0),
 			},
-			.irq1_func = {
-				.addr = 0x5e,
-				.mask = BIT(5),
-			},
-			.irq2_func = {
-				.addr = 0x5f,
-				.mask = BIT(5),
-			},
+			.irq1_func = 0x5e,
+			.irq2_func = 0x5f,
 			.hla = {
 				.addr = 0x03,
 				.mask = BIT(4),
@@ -1373,6 +1342,7 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 						.addr = 0x5b,
 						.mask = GENMASK(5, 0),
 					},
+					.enable_mask = BIT(5),
 					.status_reg = 0x45,
 					.status_mask = BIT(3),
 					.status_z_mask = BIT(0),
@@ -1825,10 +1795,11 @@ static int st_lsm6dsx_write_raw(struct iio_dev *iio_dev,
 static int st_lsm6dsx_event_setup(struct st_lsm6dsx_hw *hw, bool state)
 {
 	const struct st_lsm6dsx_reg *reg;
+	u8 enable_mask;
 	unsigned int data;
 	int err;
 
-	if (!hw->settings->irq_config.irq1_func.addr)
+	if (!hw->irq_routing)
 		return -ENOTSUPP;
 
 	reg = &hw->settings->event_settings.enable_reg;
@@ -1841,9 +1812,10 @@ static int st_lsm6dsx_event_setup(struct st_lsm6dsx_hw *hw, bool state)
 	}
 
 	/* Enable wakeup interrupt */
-	data = ST_LSM6DSX_SHIFT_VAL(state, hw->irq_routing->mask);
-	return st_lsm6dsx_update_bits_locked(hw, hw->irq_routing->addr,
-					     hw->irq_routing->mask, data);
+	enable_mask = hw->settings->event_settings.sources[ST_LSM6DSX_EVENT_WAKEUP].enable_mask;
+	data = ST_LSM6DSX_SHIFT_VAL(state, enable_mask);
+	return st_lsm6dsx_update_bits_locked(hw, hw->irq_routing,
+					     enable_mask, data);
 }
 
 static int st_lsm6dsx_read_event(struct iio_dev *iio_dev,
@@ -2097,11 +2069,11 @@ st_lsm6dsx_get_drdy_reg(struct st_lsm6dsx_hw *hw,
 
 	switch (drdy_pin) {
 	case 1:
-		hw->irq_routing = &hw->settings->irq_config.irq1_func;
+		hw->irq_routing = hw->settings->irq_config.irq1_func;
 		*drdy_reg = &hw->settings->irq_config.irq1;
 		break;
 	case 2:
-		hw->irq_routing = &hw->settings->irq_config.irq2_func;
+		hw->irq_routing = hw->settings->irq_config.irq2_func;
 		*drdy_reg = &hw->settings->irq_config.irq2;
 		break;
 	default:
-- 
2.39.5

Re: [PATCH 3/9] iio: imu: st_lsm6dsx: move wakeup event enable mask to event_src

[Andy Shevchenko]

On Thu, Oct 30, 2025 at 08:27:46AM +0100, Francesco Lavra wrote:
> The mask value being assigned to the irq1_func and irq2_func fields
> of the irq_config struct is specific to a single event source (i.e.
> the wakeup event), and as such it should be separate from the
> definition of the interrupt function registers, which cover
> multiple event sources.
> In preparation for adding support for more event types, change the
> irq1_func and irq2_func type from an {address, mask} pair to an
> address, and move the mask value to a new field of struct
> st_lsm6dsx_event_src. No functional changes.

...

>  	/* Enable wakeup interrupt */
> -	data = ST_LSM6DSX_SHIFT_VAL(state, hw->irq_routing->mask);
> -	return st_lsm6dsx_update_bits_locked(hw, hw->irq_routing->addr,
> -					     hw->irq_routing->mask, data);
> +	enable_mask = hw->settings->event_settings.sources[ST_LSM6DSX_EVENT_WAKEUP].enable_mask;
> +	data = ST_LSM6DSX_SHIFT_VAL(state, enable_mask);
> +	return st_lsm6dsx_update_bits_locked(hw, hw->irq_routing,
> +					     enable_mask, data);

I would go with one line here (despite on having it 85 characters long).

-- 
With Best Regards,
Andy Shevchenko

[PATCH 4/9] iio: imu: st_lsm6dsx: dynamically allocate iio_event_spec structs

[Francesco Lavra]

In preparation for adding support for more event types, drop the
static declaration of a single struct iio_event_spec variable, in
favor of allocating and initializing the iio_event_spec array
dynamically, so that it can contain more than one entry if a given
sensor supports more than one event source.

Signed-off-by: Francesco Lavra <flavra@baylibre.com>
---
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  7 -----
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 30 ++++++++++++++++++--
 2 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
index 5c73156b714a..ec4efb29c4cc 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
@@ -439,13 +439,6 @@ struct st_lsm6dsx_hw {
 	} scan[ST_LSM6DSX_ID_MAX];
 };
 
-static __maybe_unused const struct iio_event_spec st_lsm6dsx_event = {
-	.type = IIO_EV_TYPE_THRESH,
-	.dir = IIO_EV_DIR_EITHER,
-	.mask_separate = BIT(IIO_EV_INFO_VALUE) |
-			 BIT(IIO_EV_INFO_ENABLE)
-};
-
 static __maybe_unused const unsigned long st_lsm6dsx_available_scan_masks[] = {
 	0x7, 0x0,
 };
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index 4bae5da8910e..76025971c05d 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -2314,9 +2314,33 @@ static int st_lsm6dsx_chan_init(struct iio_chan_spec *channels, struct st_lsm6ds
 	chan->scan_type.endianness = IIO_LE;
 	chan->ext_info = st_lsm6dsx_ext_info;
 	if (id == ST_LSM6DSX_ID_ACC) {
-		if (hw->settings->event_settings.sources[ST_LSM6DSX_EVENT_WAKEUP].value.addr) {
-			chan->event_spec = &st_lsm6dsx_event;
-			chan->num_event_specs = 1;
+		const struct st_lsm6dsx_event_src *event_src;
+		unsigned int event_sources;
+		int event;
+
+		event_src = hw->settings->event_settings.sources;
+		event_sources = 0;
+		for (event = 0; event < ST_LSM6DSX_EVENT_MAX; event++) {
+			if (event_src[event].status_reg) {
+				event_sources |= BIT(event);
+				chan->num_event_specs++;
+			}
+		}
+		if (event_sources) {
+			struct iio_event_spec *event_spec;
+
+			event_spec = devm_kzalloc(hw->dev,
+						  chan->num_event_specs * sizeof(*event_spec),
+						  GFP_KERNEL);
+			if (!event_spec)
+				return -ENOMEM;
+			chan->event_spec = event_spec;
+			if (event_sources & BIT(ST_LSM6DSX_EVENT_WAKEUP)) {
+				event_spec->type = IIO_EV_TYPE_THRESH;
+				event_spec->dir = IIO_EV_DIR_EITHER;
+				event_spec->mask_separate = BIT(IIO_EV_INFO_VALUE) |
+							    BIT(IIO_EV_INFO_ENABLE);
+			}
 		}
 	}
 	return 0;
-- 
2.39.5

Re: [PATCH 4/9] iio: imu: st_lsm6dsx: dynamically allocate iio_event_spec structs

[Jonathan Cameron]

On Thu, 30 Oct 2025 08:27:47 +0100
Francesco Lavra <flavra@baylibre.com> wrote:

> In preparation for adding support for more event types, drop the
> static declaration of a single struct iio_event_spec variable, in
> favor of allocating and initializing the iio_event_spec array
> dynamically, so that it can contain more than one entry if a given
> sensor supports more than one event source.
> 
> Signed-off-by: Francesco Lavra <flavra@baylibre.com>

Similar comment for this to the dynamic channel creation.
Unless it is really quite a large number of combinations I'd normally go
for separate iio_chan_spec structures with pointers to separate iio_event_spec
structures.  Whilst this adds a fair bit of data it is easy to review
as set of such structures for each device against the datasheet.
The code to do it dynamically often gets really fiddly as it has to translate
between different representations of the same thing.

You tend to get a device model specific iio_chan_spec structure array (or a set
of related devices share one).

Jonathan

[PATCH 5/9] iio: imu: st_lsm6dsx: rework code to check for enabled events

[Francesco Lavra]

The enable_event field in struct st_lsm6dsx_hw does not lend itself
well to handling multiple event sources, so it will have to be
modified to add support for more event sources. As a preparatory
step, remove references to this field from code that does not deal
with event management; rework the st_lsm6dsx_check_events()
function so that it returns whether any events are currently
enabled on a given sensor.

Signed-off-by: Francesco Lavra <flavra@baylibre.com>
---
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index 76025971c05d..157bc2615dc6 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -1670,11 +1670,11 @@ __st_lsm6dsx_sensor_set_enable(struct st_lsm6dsx_sensor *sensor,
 }
 
 static int
-st_lsm6dsx_check_events(struct st_lsm6dsx_sensor *sensor, bool enable)
+st_lsm6dsx_check_events(struct st_lsm6dsx_sensor *sensor)
 {
 	struct st_lsm6dsx_hw *hw = sensor->hw;
 
-	if (sensor->id == ST_LSM6DSX_ID_GYRO || enable)
+	if (sensor->id != ST_LSM6DSX_ID_ACC)
 		return 0;
 
 	return hw->enable_event;
@@ -1683,7 +1683,7 @@ st_lsm6dsx_check_events(struct st_lsm6dsx_sensor *sensor, bool enable)
 int st_lsm6dsx_sensor_set_enable(struct st_lsm6dsx_sensor *sensor,
 				 bool enable)
 {
-	if (st_lsm6dsx_check_events(sensor, enable))
+	if (st_lsm6dsx_check_events(sensor))
 		return 0;
 
 	return __st_lsm6dsx_sensor_set_enable(sensor, enable);
@@ -1711,11 +1711,9 @@ static int st_lsm6dsx_read_oneshot(struct st_lsm6dsx_sensor *sensor,
 	if (err < 0)
 		return err;
 
-	if (!hw->enable_event) {
-		err = st_lsm6dsx_sensor_set_enable(sensor, false);
-		if (err < 0)
-			return err;
-	}
+	err = st_lsm6dsx_sensor_set_enable(sensor, false);
+	if (err < 0)
+		return err;
 
 	*val = (s16)le16_to_cpu(data);
 
@@ -2743,7 +2741,7 @@ static int st_lsm6dsx_suspend(struct device *dev)
 			continue;
 
 		if (device_may_wakeup(dev) &&
-		    sensor->id == ST_LSM6DSX_ID_ACC && hw->enable_event) {
+		    st_lsm6dsx_check_events(sensor)) {
 			/* Enable wake from IRQ */
 			enable_irq_wake(hw->irq);
 			continue;
@@ -2774,7 +2772,7 @@ static int st_lsm6dsx_resume(struct device *dev)
 
 		sensor = iio_priv(hw->iio_devs[i]);
 		if (device_may_wakeup(dev) &&
-		    sensor->id == ST_LSM6DSX_ID_ACC && hw->enable_event)
+		    st_lsm6dsx_check_events(sensor))
 			disable_irq_wake(hw->irq);
 
 		if (!(hw->suspend_mask & BIT(sensor->id)))
-- 
2.39.5

[PATCH 6/9] iio: imu: st_lsm6dsx: remove event_threshold field from hw struct

[Francesco Lavra]

This field is used to store the wakeup event detection threshold
value. When adding support for more event types, some of which may
have different threshold values for different axes, storing all
threshold values for all event sources would be cumbersome. Thus,
remove this field altogether, and read the currently configured
value from the sensor when requested by userspace.

Signed-off-by: Francesco Lavra <flavra@baylibre.com>
---
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  2 --
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 12 +++++++++---
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
index ec4efb29c4cc..98aa3cfb711b 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
@@ -398,7 +398,6 @@ struct st_lsm6dsx_sensor {
  * @sip: Total number of samples (acc/gyro/ts) in a given pattern.
  * @buff: Device read buffer.
  * @irq_routing: pointer to interrupt routing configuration.
- * @event_threshold: wakeup event threshold.
  * @enable_event: enabled event bitmask.
  * @iio_devs: Pointers to acc/gyro iio_dev instances.
  * @settings: Pointer to the specific sensor settings in use.
@@ -422,7 +421,6 @@ struct st_lsm6dsx_hw {
 	u8 sip;
 
 	u8 irq_routing;
-	u8 event_threshold;
 	u8 enable_event;
 
 	u8 *buff;
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index 157bc2615dc6..ea145e15cd36 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -1825,12 +1825,20 @@ static int st_lsm6dsx_read_event(struct iio_dev *iio_dev,
 {
 	struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
 	struct st_lsm6dsx_hw *hw = sensor->hw;
+	const struct st_lsm6dsx_reg *reg;
+	u8 data;
+	int err;
 
 	if (type != IIO_EV_TYPE_THRESH)
 		return -EINVAL;
 
+	reg = &hw->settings->event_settings.sources[ST_LSM6DSX_EVENT_WAKEUP].value;
+	err = st_lsm6dsx_read_locked(hw, reg->addr, &data, sizeof(data));
+	if (err < 0)
+		return err;
+
 	*val2 = 0;
-	*val = hw->event_threshold;
+	*val = (data & reg->mask) >> __ffs(reg->mask);
 
 	return IIO_VAL_INT;
 }
@@ -1862,8 +1870,6 @@ st_lsm6dsx_write_event(struct iio_dev *iio_dev,
 	if (err < 0)
 		return -EINVAL;
 
-	hw->event_threshold = val;
-
 	return 0;
 }
 
-- 
2.39.5

Re: [PATCH 6/9] iio: imu: st_lsm6dsx: remove event_threshold field from hw struct

[Andy Shevchenko]

On Thu, Oct 30, 2025 at 08:27:49AM +0100, Francesco Lavra wrote:
> This field is used to store the wakeup event detection threshold
> value. When adding support for more event types, some of which may
> have different threshold values for different axes, storing all
> threshold values for all event sources would be cumbersome. Thus,
> remove this field altogether, and read the currently configured
> value from the sensor when requested by userspace.

...

> +	*val = (data & reg->mask) >> __ffs(reg->mask);

Seems like yet another candidate for field_get() macro.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 6/9] iio: imu: st_lsm6dsx: remove event_threshold field from hw struct

[Francesco Lavra]

[-- Attachment #1: Type: text/plain, Size: 889 bytes --]

On Thu, 2025-10-30 at 10:01 +0200, Andy Shevchenko wrote:
> On Thu, Oct 30, 2025 at 08:27:49AM +0100, Francesco Lavra wrote:
> > This field is used to store the wakeup event detection threshold
> > value. When adding support for more event types, some of which may
> > have different threshold values for different axes, storing all
> > threshold values for all event sources would be cumbersome. Thus,
> > remove this field altogether, and read the currently configured
> > value from the sensor when requested by userspace.
> 
> ...
> 
> > +       *val = (data & reg->mask) >> __ffs(reg->mask);
> 
> Seems like yet another candidate for field_get() macro.

FIELD_GET() can only be used with compile-time constant masks.
And apparently this is the case with u8_get_bits() too, because you get a
"bad bitfield mask" compiler error if you try to use u8_get_bits().

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 6/9] iio: imu: st_lsm6dsx: remove event_threshold field from hw struct

[Andy Shevchenko]

On Thu, Oct 30, 2025 at 12:10:08PM +0100, Francesco Lavra wrote:
> On Thu, 2025-10-30 at 10:01 +0200, Andy Shevchenko wrote:
> > On Thu, Oct 30, 2025 at 08:27:49AM +0100, Francesco Lavra wrote:

...

> > > +       *val = (data & reg->mask) >> __ffs(reg->mask);
> > 
> > Seems like yet another candidate for field_get() macro.
> 
> FIELD_GET() can only be used with compile-time constant masks.
> And apparently this is the case with u8_get_bits() too, because you get a
> "bad bitfield mask" compiler error if you try to use u8_get_bits().

We are talking about different things.
Here are the pointers to what I'm talking:

- git grep -n -w 'field_get'
- https://lore.kernel.org/linux-gpio/cover.1761588465.git.geert+renesas@glider.be/

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 6/9] iio: imu: st_lsm6dsx: remove event_threshold field from hw struct

[Jonathan Cameron]

On Thu, 30 Oct 2025 15:49:37 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Thu, Oct 30, 2025 at 12:10:08PM +0100, Francesco Lavra wrote:
> > On Thu, 2025-10-30 at 10:01 +0200, Andy Shevchenko wrote:  
> > > On Thu, Oct 30, 2025 at 08:27:49AM +0100, Francesco Lavra wrote:  
> 
> ...
> 
> > > > +       *val = (data & reg->mask) >> __ffs(reg->mask);  
> > > 
> > > Seems like yet another candidate for field_get() macro.  
> > 
> > FIELD_GET() can only be used with compile-time constant masks.
> > And apparently this is the case with u8_get_bits() too, because you get a
> > "bad bitfield mask" compiler error if you try to use u8_get_bits().  
> 
> We are talking about different things.
> Here are the pointers to what I'm talking:
> 
> - git grep -n -w 'field_get'
> - https://lore.kernel.org/linux-gpio/cover.1761588465.git.geert+renesas@glider.be/
> 
True that it will be a usecase for that, but given plan is to merge that through
a different tree in next merge window, it's not available for us yet.  Hence would
be a follow up patch next cycle.

Jonathan

Re: [PATCH 6/9] iio: imu: st_lsm6dsx: remove event_threshold field from hw struct

[Andy Shevchenko]

On Sun, Nov 2, 2025 at 1:30 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Thu, 30 Oct 2025 15:49:37 +0200
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > On Thu, Oct 30, 2025 at 12:10:08PM +0100, Francesco Lavra wrote:
> > > On Thu, 2025-10-30 at 10:01 +0200, Andy Shevchenko wrote:
> > > > On Thu, Oct 30, 2025 at 08:27:49AM +0100, Francesco Lavra wrote:

...

> > > > > +       *val = (data & reg->mask) >> __ffs(reg->mask);
> > > >
> > > > Seems like yet another candidate for field_get() macro.
> > >
> > > FIELD_GET() can only be used with compile-time constant masks.
> > > And apparently this is the case with u8_get_bits() too, because you get a
> > > "bad bitfield mask" compiler error if you try to use u8_get_bits().
> >
> > We are talking about different things.
> > Here are the pointers to what I'm talking:
> >
> > - git grep -n -w 'field_get'
> > - https://lore.kernel.org/linux-gpio/cover.1761588465.git.geert+renesas@glider.be/
> >
> True that it will be a usecase for that, but given plan is to merge that through
> a different tree in next merge window, it's not available for us yet.  Hence would
> be a follow up patch next cycle.

Yes, but we can still define them here. Dunno either with #under or
under (namespaced) names, but still possible to use now.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 6/9] iio: imu: st_lsm6dsx: remove event_threshold field from hw struct

[Francesco Lavra]

[-- Attachment #1: Type: text/plain, Size: 1535 bytes --]

On Sun, 2025-11-02 at 15:45 +0200, Andy Shevchenko wrote:
> On Sun, Nov 2, 2025 at 1:30 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > On Thu, 30 Oct 2025 15:49:37 +0200
> > Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > > On Thu, Oct 30, 2025 at 12:10:08PM +0100, Francesco Lavra wrote:
> > > > On Thu, 2025-10-30 at 10:01 +0200, Andy Shevchenko wrote:
> > > > > On Thu, Oct 30, 2025 at 08:27:49AM +0100, Francesco Lavra wrote:
> 
> ...
> 
> > > > > > +       *val = (data & reg->mask) >> __ffs(reg->mask);
> > > > > 
> > > > > Seems like yet another candidate for field_get() macro.
> > > > 
> > > > FIELD_GET() can only be used with compile-time constant masks.
> > > > And apparently this is the case with u8_get_bits() too, because you
> > > > get a
> > > > "bad bitfield mask" compiler error if you try to use u8_get_bits().
> > > 
> > > We are talking about different things.
> > > Here are the pointers to what I'm talking:
> > > 
> > > - git grep -n -w 'field_get'
> > > -
> > > https://lore.kernel.org/linux-gpio/cover.1761588465.git.geert+renesas@glider.be/
> > > 
> > True that it will be a usecase for that, but given plan is to merge
> > that through
> > a different tree in next merge window, it's not available for us yet. 
> > Hence would
> > be a follow up patch next cycle.
> 
> Yes, but we can still define them here. Dunno either with #under or
> under (namespaced) names, but still possible to use now.

OK, I will define an ST_LSM6DSX_FIELD_GET() macro.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 6/9] iio: imu: st_lsm6dsx: remove event_threshold field from hw struct

[Andy Shevchenko]

On Mon, Nov 03, 2025 at 10:34:28AM +0100, Francesco Lavra wrote:
> On Sun, 2025-11-02 at 15:45 +0200, Andy Shevchenko wrote:
> > On Sun, Nov 2, 2025 at 1:30 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > > On Thu, 30 Oct 2025 15:49:37 +0200
> > > Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > > > On Thu, Oct 30, 2025 at 12:10:08PM +0100, Francesco Lavra wrote:
> > > > > On Thu, 2025-10-30 at 10:01 +0200, Andy Shevchenko wrote:
> > > > > > On Thu, Oct 30, 2025 at 08:27:49AM +0100, Francesco Lavra wrote:

...

> > > > > > > +       *val = (data & reg->mask) >> __ffs(reg->mask);
> > > > > > 
> > > > > > Seems like yet another candidate for field_get() macro.
> > > > > 
> > > > > FIELD_GET() can only be used with compile-time constant masks.
> > > > > And apparently this is the case with u8_get_bits() too, because you
> > > > > get a
> > > > > "bad bitfield mask" compiler error if you try to use u8_get_bits().
> > > > 
> > > > We are talking about different things.
> > > > Here are the pointers to what I'm talking:
> > > > 
> > > > - git grep -n -w 'field_get'
> > > > -
> > > > https://lore.kernel.org/linux-gpio/cover.1761588465.git.geert+renesas@glider.be/
> > > > 
> > > True that it will be a usecase for that, but given plan is to merge
> > > that through
> > > a different tree in next merge window, it's not available for us yet. 
> > > Hence would
> > > be a follow up patch next cycle.
> > 
> > Yes, but we can still define them here. Dunno either with #under or
> > under (namespaced) names, but still possible to use now.
> 
> OK, I will define an ST_LSM6DSX_FIELD_GET() macro.

With small letters, as this is about run-time helpers (non-constant).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 6/9] iio: imu: st_lsm6dsx: remove event_threshold field from hw struct

[David Lechner]

On 11/3/25 3:34 AM, Francesco Lavra wrote:
> On Sun, 2025-11-02 at 15:45 +0200, Andy Shevchenko wrote:
>> On Sun, Nov 2, 2025 at 1:30 PM Jonathan Cameron <jic23@kernel.org> wrote:
>>> On Thu, 30 Oct 2025 15:49:37 +0200
>>> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
>>>> On Thu, Oct 30, 2025 at 12:10:08PM +0100, Francesco Lavra wrote:
>>>>> On Thu, 2025-10-30 at 10:01 +0200, Andy Shevchenko wrote:
>>>>>> On Thu, Oct 30, 2025 at 08:27:49AM +0100, Francesco Lavra wrote:
>>
>> ...
>>
>>>>>>> +       *val = (data & reg->mask) >> __ffs(reg->mask);
>>>>>>
>>>>>> Seems like yet another candidate for field_get() macro.
>>>>>
>>>>> FIELD_GET() can only be used with compile-time constant masks.
>>>>> And apparently this is the case with u8_get_bits() too, because you
>>>>> get a
>>>>> "bad bitfield mask" compiler error if you try to use u8_get_bits().
>>>>
>>>> We are talking about different things.
>>>> Here are the pointers to what I'm talking:
>>>>
>>>> - git grep -n -w 'field_get'
>>>> -
>>>> https://lore.kernel.org/linux-gpio/cover.1761588465.git.geert+renesas@glider.be/
>>>>
>>> True that it will be a usecase for that, but given plan is to merge
>>> that through
>>> a different tree in next merge window, it's not available for us yet. 
>>> Hence would
>>> be a follow up patch next cycle.
>>
>> Yes, but we can still define them here. Dunno either with #under or
>> under (namespaced) names, but still possible to use now.
> 
> OK, I will define an ST_LSM6DSX_FIELD_GET() macro.

The macro should be named exactly `field_get()`, otherwise we will
have to rename all of the callers later after the series adding it
to linux/bitfield.h is merged. And it should have an
`#undef field_get` just like the other patches that series. Then
later, we only need to remove the #undef and #def lines and not
change the rest of the code.

Re: [PATCH 6/9] iio: imu: st_lsm6dsx: remove event_threshold field from hw struct

[Jonathan Cameron]

On Mon, 3 Nov 2025 08:53:44 -0600
David Lechner <dlechner@baylibre.com> wrote:

> On 11/3/25 3:34 AM, Francesco Lavra wrote:
> > On Sun, 2025-11-02 at 15:45 +0200, Andy Shevchenko wrote:  
> >> On Sun, Nov 2, 2025 at 1:30 PM Jonathan Cameron <jic23@kernel.org> wrote:  
> >>> On Thu, 30 Oct 2025 15:49:37 +0200
> >>> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:  
> >>>> On Thu, Oct 30, 2025 at 12:10:08PM +0100, Francesco Lavra wrote:  
> >>>>> On Thu, 2025-10-30 at 10:01 +0200, Andy Shevchenko wrote:  
> >>>>>> On Thu, Oct 30, 2025 at 08:27:49AM +0100, Francesco Lavra wrote:  
> >>
> >> ...
> >>  
> >>>>>>> +       *val = (data & reg->mask) >> __ffs(reg->mask);  
> >>>>>>
> >>>>>> Seems like yet another candidate for field_get() macro.  
> >>>>>
> >>>>> FIELD_GET() can only be used with compile-time constant masks.
> >>>>> And apparently this is the case with u8_get_bits() too, because you
> >>>>> get a
> >>>>> "bad bitfield mask" compiler error if you try to use u8_get_bits().  
> >>>>
> >>>> We are talking about different things.
> >>>> Here are the pointers to what I'm talking:
> >>>>
> >>>> - git grep -n -w 'field_get'
> >>>> -
> >>>> https://lore.kernel.org/linux-gpio/cover.1761588465.git.geert+renesas@glider.be/
> >>>>  
> >>> True that it will be a usecase for that, but given plan is to merge
> >>> that through
> >>> a different tree in next merge window, it's not available for us yet. 
> >>> Hence would
> >>> be a follow up patch next cycle.  
> >>
> >> Yes, but we can still define them here. Dunno either with #under or
> >> under (namespaced) names, but still possible to use now.  
> > 
> > OK, I will define an ST_LSM6DSX_FIELD_GET() macro.  
> 
> The macro should be named exactly `field_get()`, otherwise we will
> have to rename all of the callers later after the series adding it
> to linux/bitfield.h is merged. And it should have an
> `#undef field_get` just like the other patches that series. Then
> later, we only need to remove the #undef and #def lines and not
> change the rest of the code.

Carrying undefs for reasons we can't see in this code is for me something I'd
consider problematic.

For a case like this where we have just once instance I'd just prefer
either not using the macros, or namespacing them then replacing next cycle.
If there are loads of uses, then maybe on the undef nastiness.
These tend not to be common in drivers though and so far I've not seen
a case where the undef is worth doing (except maybe in the series adding
these new helpers where it is a very temporary thing)

Jonathan

[PATCH 7/9] iio: imu: st_lsm6dsx: make event management functions generic

[Francesco Lavra]

In preparation for adding support for more event types, use an
array indexed by event ID instead of a scalar value to store
enabled events, and refactor the functions to configure and report
events so that their implementation is not specific for wakeup
events. Move the logic to update the global event interrupt enable
flag from st_lsm6dsx_event_setup() to its calling function, so that
it can take into account also event sources different from the
source being configured. While changing the signature of the
st_lsm6dsx_event_setup() function, opportunistically add the
currently unused `axis` parameter, which will be used when adding
support for enabling and disabling events on a per axis basis.

Signed-off-by: Francesco Lavra <flavra@baylibre.com>
---
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |   2 +-
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 127 +++++++++++++------
 2 files changed, 88 insertions(+), 41 deletions(-)

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
index 98aa3cfb711b..0e0642ca1b6f 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
@@ -421,7 +421,7 @@ struct st_lsm6dsx_hw {
 	u8 sip;
 
 	u8 irq_routing;
-	u8 enable_event;
+	u8 enable_event[ST_LSM6DSX_EVENT_MAX];
 
 	u8 *buff;
 
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index ea145e15cd36..87d40e70ca26 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -1673,11 +1673,16 @@ static int
 st_lsm6dsx_check_events(struct st_lsm6dsx_sensor *sensor)
 {
 	struct st_lsm6dsx_hw *hw = sensor->hw;
+	int event;
 
 	if (sensor->id != ST_LSM6DSX_ID_ACC)
 		return 0;
 
-	return hw->enable_event;
+	for (event = 0; event < ST_LSM6DSX_EVENT_MAX; event++) {
+		if (hw->enable_event[event])
+			return true;
+	}
+	return false;
 }
 
 int st_lsm6dsx_sensor_set_enable(struct st_lsm6dsx_sensor *sensor,
@@ -1790,9 +1795,10 @@ static int st_lsm6dsx_write_raw(struct iio_dev *iio_dev,
 	return err;
 }
 
-static int st_lsm6dsx_event_setup(struct st_lsm6dsx_hw *hw, bool state)
+static int st_lsm6dsx_event_setup(struct st_lsm6dsx_hw *hw, enum st_lsm6dsx_event_id event,
+				  int axis, bool state)
 {
-	const struct st_lsm6dsx_reg *reg;
+	const struct st_lsm6dsx_event_src *src = &hw->settings->event_settings.sources[event];
 	u8 enable_mask;
 	unsigned int data;
 	int err;
@@ -1800,22 +1806,23 @@ static int st_lsm6dsx_event_setup(struct st_lsm6dsx_hw *hw, bool state)
 	if (!hw->irq_routing)
 		return -ENOTSUPP;
 
-	reg = &hw->settings->event_settings.enable_reg;
-	if (reg->addr) {
-		data = ST_LSM6DSX_SHIFT_VAL(state, reg->mask);
-		err = st_lsm6dsx_update_bits_locked(hw, reg->addr,
-						    reg->mask, data);
-		if (err < 0)
-			return err;
-	}
-
-	/* Enable wakeup interrupt */
-	enable_mask = hw->settings->event_settings.sources[ST_LSM6DSX_EVENT_WAKEUP].enable_mask;
+	/* Enable/disable event interrupt */
+	enable_mask = src->enable_mask;
 	data = ST_LSM6DSX_SHIFT_VAL(state, enable_mask);
 	return st_lsm6dsx_update_bits_locked(hw, hw->irq_routing,
 					     enable_mask, data);
 }
 
+static enum st_lsm6dsx_event_id st_lsm6dsx_get_event_id(enum iio_event_type type)
+{
+	switch (type) {
+	case IIO_EV_TYPE_THRESH:
+		return ST_LSM6DSX_EVENT_WAKEUP;
+	default:
+		return ST_LSM6DSX_EVENT_MAX;
+	}
+}
+
 static int st_lsm6dsx_read_event(struct iio_dev *iio_dev,
 				 const struct iio_chan_spec *chan,
 				 enum iio_event_type type,
@@ -1825,14 +1832,15 @@ static int st_lsm6dsx_read_event(struct iio_dev *iio_dev,
 {
 	struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
 	struct st_lsm6dsx_hw *hw = sensor->hw;
+	enum st_lsm6dsx_event_id event = st_lsm6dsx_get_event_id(type);
 	const struct st_lsm6dsx_reg *reg;
 	u8 data;
 	int err;
 
-	if (type != IIO_EV_TYPE_THRESH)
+	if (event == ST_LSM6DSX_EVENT_MAX)
 		return -EINVAL;
 
-	reg = &hw->settings->event_settings.sources[ST_LSM6DSX_EVENT_WAKEUP].value;
+	reg = &hw->settings->event_settings.sources[event].value;
 	err = st_lsm6dsx_read_locked(hw, reg->addr, &data, sizeof(data));
 	if (err < 0)
 		return err;
@@ -1851,19 +1859,20 @@ st_lsm6dsx_write_event(struct iio_dev *iio_dev,
 		       enum iio_event_info info,
 		       int val, int val2)
 {
+	enum st_lsm6dsx_event_id event = st_lsm6dsx_get_event_id(type);
 	struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
 	struct st_lsm6dsx_hw *hw = sensor->hw;
 	const struct st_lsm6dsx_reg *reg;
 	unsigned int data;
 	int err;
 
-	if (type != IIO_EV_TYPE_THRESH)
+	if (event == ST_LSM6DSX_EVENT_MAX)
 		return -EINVAL;
 
 	if (val < 0 || val > 31)
 		return -EINVAL;
 
-	reg = &hw->settings->event_settings.sources[ST_LSM6DSX_EVENT_WAKEUP].value;
+	reg = &hw->settings->event_settings.sources[event].value;
 	data = ST_LSM6DSX_SHIFT_VAL(val, reg->mask);
 	err = st_lsm6dsx_update_bits_locked(hw, reg->addr,
 					    reg->mask, data);
@@ -1879,13 +1888,14 @@ st_lsm6dsx_read_event_config(struct iio_dev *iio_dev,
 			     enum iio_event_type type,
 			     enum iio_event_direction dir)
 {
+	enum st_lsm6dsx_event_id event = st_lsm6dsx_get_event_id(type);
 	struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
 	struct st_lsm6dsx_hw *hw = sensor->hw;
 
-	if (type != IIO_EV_TYPE_THRESH)
+	if (event == ST_LSM6DSX_EVENT_MAX)
 		return -EINVAL;
 
-	return !!(hw->enable_event & BIT(chan->channel2));
+	return !!(hw->enable_event[event] & BIT(chan->channel2));
 }
 
 static int
@@ -1894,22 +1904,25 @@ st_lsm6dsx_write_event_config(struct iio_dev *iio_dev,
 			      enum iio_event_type type,
 			      enum iio_event_direction dir, bool state)
 {
+	enum st_lsm6dsx_event_id event = st_lsm6dsx_get_event_id(type);
 	struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
 	struct st_lsm6dsx_hw *hw = sensor->hw;
+	int axis = chan->channel2;
 	u8 enable_event;
 	int err;
+	bool any_events_enabled = false;
 
-	if (type != IIO_EV_TYPE_THRESH)
+	if (event == ST_LSM6DSX_EVENT_MAX)
 		return -EINVAL;
 
 	if (state) {
-		enable_event = hw->enable_event | BIT(chan->channel2);
+		enable_event = hw->enable_event[event] | BIT(axis);
 
 		/* do not enable events if they are already enabled */
-		if (hw->enable_event)
+		if (hw->enable_event[event])
 			goto out;
 	} else {
-		enable_event = hw->enable_event & ~BIT(chan->channel2);
+		enable_event = hw->enable_event[event] & ~BIT(axis);
 
 		/* only turn off sensor if no events is enabled */
 		if (enable_event)
@@ -1917,22 +1930,43 @@ st_lsm6dsx_write_event_config(struct iio_dev *iio_dev,
 	}
 
 	/* stop here if no changes have been made */
-	if (hw->enable_event == enable_event)
+	if (hw->enable_event[event] == enable_event)
 		return 0;
 
-	err = st_lsm6dsx_event_setup(hw, state);
+	err = st_lsm6dsx_event_setup(hw, event, axis, state);
 	if (err < 0)
 		return err;
 
 	mutex_lock(&hw->conf_lock);
-	if (enable_event || !(hw->fifo_mask & BIT(sensor->id)))
+	if (!enable_event) {
+		enum st_lsm6dsx_event_id other_event;
+
+		for (other_event = 0; other_event < ST_LSM6DSX_EVENT_MAX; other_event++) {
+			if (other_event != event && hw->enable_event[other_event]) {
+				any_events_enabled = true;
+				break;
+			}
+		}
+	}
+	if (enable_event || !any_events_enabled) {
+		const struct st_lsm6dsx_reg *reg = &hw->settings->event_settings.enable_reg;
+
+		if (reg->addr) {
+			err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
+						 ST_LSM6DSX_SHIFT_VAL(state, reg->mask));
+			if (err < 0)
+				goto unlock_out;
+		}
+	}
+	if (enable_event || (!any_events_enabled && !(hw->fifo_mask & BIT(sensor->id))))
 		err = __st_lsm6dsx_sensor_set_enable(sensor, state);
+unlock_out:
 	mutex_unlock(&hw->conf_lock);
 	if (err < 0)
 		return err;
 
 out:
-	hw->enable_event = enable_event;
+	hw->enable_event[event] = enable_event;
 
 	return 0;
 }
@@ -2410,18 +2444,20 @@ static struct iio_dev *st_lsm6dsx_alloc_iiodev(struct st_lsm6dsx_hw *hw,
 }
 
 static bool
-st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw)
+st_lsm6dsx_report_events(struct st_lsm6dsx_hw *hw, enum st_lsm6dsx_event_id id,
+			 enum iio_event_type type, enum iio_event_direction dir)
 {
+	u8 enable_event = hw->enable_event[id];
 	const struct st_lsm6dsx_event_settings *event_settings;
 	const struct st_lsm6dsx_event_src *event_src;
 	int err, data;
 	s64 timestamp;
 
-	if (!hw->enable_event)
+	if (!enable_event)
 		return false;
 
 	event_settings = &hw->settings->event_settings;
-	event_src = &event_settings->sources[ST_LSM6DSX_EVENT_WAKEUP];
+	event_src = &event_settings->sources[id];
 	err = st_lsm6dsx_read_locked(hw, event_src->status_reg,
 				     &data, sizeof(data));
 	if (err < 0)
@@ -2429,38 +2465,49 @@ st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw)
 
 	timestamp = iio_get_time_ns(hw->iio_devs[ST_LSM6DSX_ID_ACC]);
 	if ((data & event_src->status_z_mask) &&
-	    (hw->enable_event & BIT(IIO_MOD_Z)))
+	    (enable_event & BIT(IIO_MOD_Z)))
 		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
 			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
 						  0,
 						  IIO_MOD_Z,
-						  IIO_EV_TYPE_THRESH,
-						  IIO_EV_DIR_EITHER),
+						  type,
+						  dir),
 						  timestamp);
 
 	if ((data & event_src->status_y_mask) &&
-	    (hw->enable_event & BIT(IIO_MOD_Y)))
+	    (enable_event & BIT(IIO_MOD_Y)))
 		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
 			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
 						  0,
 						  IIO_MOD_Y,
-						  IIO_EV_TYPE_THRESH,
-						  IIO_EV_DIR_EITHER),
+						  type,
+						  dir),
 						  timestamp);
 
 	if ((data & event_src->status_x_mask) &&
-	    (hw->enable_event & BIT(IIO_MOD_X)))
+	    (enable_event & BIT(IIO_MOD_X)))
 		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
 			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
 						  0,
 						  IIO_MOD_X,
-						  IIO_EV_TYPE_THRESH,
-						  IIO_EV_DIR_EITHER),
+						  type,
+						  dir),
 						  timestamp);
 
 	return data & event_src->status_mask;
 }
 
+static bool
+st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw)
+{
+	bool events_found;
+
+	events_found = st_lsm6dsx_report_events(hw, ST_LSM6DSX_EVENT_WAKEUP, IIO_EV_TYPE_THRESH,
+						IIO_EV_DIR_EITHER);
+
+	return events_found;
+}
+
 static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
 {
 	struct st_lsm6dsx_hw *hw = private;
-- 
2.39.5

Re: [PATCH 7/9] iio: imu: st_lsm6dsx: make event management functions generic

[Andy Shevchenko]

On Thu, Oct 30, 2025 at 08:27:50AM +0100, Francesco Lavra wrote:
> In preparation for adding support for more event types, use an
> array indexed by event ID instead of a scalar value to store
> enabled events, and refactor the functions to configure and report
> events so that their implementation is not specific for wakeup
> events. Move the logic to update the global event interrupt enable
> flag from st_lsm6dsx_event_setup() to its calling function, so that
> it can take into account also event sources different from the
> source being configured. While changing the signature of the
> st_lsm6dsx_event_setup() function, opportunistically add the
> currently unused `axis` parameter, which will be used when adding
> support for enabling and disabling events on a per axis basis.

...

>  	mutex_lock(&hw->conf_lock);
> -	if (enable_event || !(hw->fifo_mask & BIT(sensor->id)))
> +	if (!enable_event) {
> +		enum st_lsm6dsx_event_id other_event;
> +
> +		for (other_event = 0; other_event < ST_LSM6DSX_EVENT_MAX; other_event++) {
> +			if (other_event != event && hw->enable_event[other_event]) {
> +				any_events_enabled = true;
> +				break;
> +			}
> +		}
> +	}
> +	if (enable_event || !any_events_enabled) {
> +		const struct st_lsm6dsx_reg *reg = &hw->settings->event_settings.enable_reg;
> +
> +		if (reg->addr) {
> +			err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
> +						 ST_LSM6DSX_SHIFT_VAL(state, reg->mask));
> +			if (err < 0)
> +				goto unlock_out;
> +		}
> +	}
> +	if (enable_event || (!any_events_enabled && !(hw->fifo_mask & BIT(sensor->id))))
>  		err = __st_lsm6dsx_sensor_set_enable(sensor, state);
> +unlock_out:
>  	mutex_unlock(&hw->conf_lock);
>  	if (err < 0)
>  		return err;

This whole block is hard to read. Perhaps you need to refactor it to have something like

	if (enable_event) {
		err = call_helper1();
		...
		err = __st_lsm6dsx_sensor_set_enable(sensor, state);
	} else {
		any_events_enabled = call_helper2();
		if (!any_events_enabled) {
			err = call_helper1();
			...
			if (!(hw->fifo_mask & BIT(sensor->id)))
				err = __st_lsm6dsx_sensor_set_enable(sensor, state);
		}
	}

With this you can see that actually helper1 can be modified (with one
additional parameter) to combination of

new_helper1()
{
	err = call_helper1();
	...
	if (!(hw->fifo_mask & BIT(sensor->id)))
		return __st_lsm6dsx_sensor_set_enable(sensor, state);
	return 0;
}

And the above goes as

	if (enable_event) {
		err = new_helper1(false);
	} else {
		any_events_enabled = call_helper2();
		if (!any_events_enabled)
			err = new_helper1(hw->fifo_mask & BIT(sensor->id));
	}

with assumed good names given this looks to me much easier to understand.

...

> +static bool
> +st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw)

Why not one line?

> +{
> +	bool events_found;

Seems useless. Is this function going to be expanded down in the series?

> +	events_found = st_lsm6dsx_report_events(hw, ST_LSM6DSX_EVENT_WAKEUP, IIO_EV_TYPE_THRESH,
> +						IIO_EV_DIR_EITHER);

Indentation.

> +	return events_found;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 7/9] iio: imu: st_lsm6dsx: make event management functions generic

[Francesco Lavra]

[-- Attachment #1: Type: text/plain, Size: 5177 bytes --]

On Thu, 2025-10-30 at 10:15 +0200, Andy Shevchenko wrote:
> On Thu, Oct 30, 2025 at 08:27:50AM +0100, Francesco Lavra wrote:
> > In preparation for adding support for more event types, use an
> > array indexed by event ID instead of a scalar value to store
> > enabled events, and refactor the functions to configure and report
> > events so that their implementation is not specific for wakeup
> > events. Move the logic to update the global event interrupt enable
> > flag from st_lsm6dsx_event_setup() to its calling function, so that
> > it can take into account also event sources different from the
> > source being configured. While changing the signature of the
> > st_lsm6dsx_event_setup() function, opportunistically add the
> > currently unused `axis` parameter, which will be used when adding
> > support for enabling and disabling events on a per axis basis.
> 
> ...
> 
> >         mutex_lock(&hw->conf_lock);
> > -       if (enable_event || !(hw->fifo_mask & BIT(sensor->id)))
> > +       if (!enable_event) {
> > +               enum st_lsm6dsx_event_id other_event;
> > +
> > +               for (other_event = 0; other_event <
> > ST_LSM6DSX_EVENT_MAX; other_event++) {
> > +                       if (other_event != event && hw-
> > >enable_event[other_event]) {
> > +                               any_events_enabled = true;
> > +                               break;
> > +                       }
> > +               }
> > +       }
> > +       if (enable_event || !any_events_enabled) {
> > +               const struct st_lsm6dsx_reg *reg = &hw->settings-
> > >event_settings.enable_reg;
> > +
> > +               if (reg->addr) {
> > +                       err = regmap_update_bits(hw->regmap, reg->addr,
> > reg->mask,
> > +                                               
> > ST_LSM6DSX_SHIFT_VAL(state, reg->mask));
> > +                       if (err < 0)
> > +                               goto unlock_out;
> > +               }
> > +       }
> > +       if (enable_event || (!any_events_enabled && !(hw->fifo_mask &
> > BIT(sensor->id))))
> >                 err = __st_lsm6dsx_sensor_set_enable(sensor, state);
> > +unlock_out:
> >         mutex_unlock(&hw->conf_lock);
> >         if (err < 0)
> >                 return err;
> 
> This whole block is hard to read. Perhaps you need to refactor it to have
> something like
> 
>         if (enable_event) {
>                 err = call_helper1();
>                 ...
>                 err = __st_lsm6dsx_sensor_set_enable(sensor, state);
>         } else {
>                 any_events_enabled = call_helper2();
>                 if (!any_events_enabled) {
>                         err = call_helper1();
>                         ...
>                         if (!(hw->fifo_mask & BIT(sensor->id)))
>                                 err =
> __st_lsm6dsx_sensor_set_enable(sensor, state);
>                 }
>         }
> 
> With this you can see that actually helper1 can be modified (with one
> additional parameter) to combination of
> 
> new_helper1()
> {
>         err = call_helper1();
>         ...
>         if (!(hw->fifo_mask & BIT(sensor->id)))
>                 return __st_lsm6dsx_sensor_set_enable(sensor, state);
>         return 0;
> }
> 
> And the above goes as
> 
>         if (enable_event) {
>                 err = new_helper1(false);
>         } else {
>                 any_events_enabled = call_helper2();
>                 if (!any_events_enabled)
>                         err = new_helper1(hw->fifo_mask & BIT(sensor-
> >id));
>         }
> 
> with assumed good names given this looks to me much easier to understand.

Will do

> 
> > +static bool
> > +st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw)
> 
> Why not one line?

This function was already there as is, even though the diff makes it appear
as a new function. Will make it one line.

> 
> > +{
> > +       bool events_found;
> 
> Seems useless. Is this function going to be expanded down in the series?

Yes, when adding support for tap events in patch 9/9.

> > +       events_found = st_lsm6dsx_report_events(hw,
> > ST_LSM6DSX_EVENT_WAKEUP, IIO_EV_TYPE_THRESH,
> > +                                               IIO_EV_DIR_EITHER);
> 
> Indentation.

Seems good to me, what should I change?

> > +       return events_found;
> > +}
> 


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 7/9] iio: imu: st_lsm6dsx: make event management functions generic

[Andy Shevchenko]

On Thu, Oct 30, 2025 at 12:17:52PM +0100, Francesco Lavra wrote:
> On Thu, 2025-10-30 at 10:15 +0200, Andy Shevchenko wrote:
> > On Thu, Oct 30, 2025 at 08:27:50AM +0100, Francesco Lavra wrote:

...

> > > +static bool
> > > +st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw)
> > 
> > Why not one line?
> 
> This function was already there as is, even though the diff makes it appear
> as a new function. Will make it one line.

Do you use --histogram diff algo when preparing patches? Perhaps that helps
without modifying the code?

...

> > > +       events_found = st_lsm6dsx_report_events(hw,
> > > ST_LSM6DSX_EVENT_WAKEUP, IIO_EV_TYPE_THRESH,
> > > +                                               IIO_EV_DIR_EITHER);
> > 
> > Indentation.
> 
> Seems good to me, what should I change?

First line is way too long for IIO which tries to keep 80 limit.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 7/9] iio: imu: st_lsm6dsx: make event management functions generic

[Jonathan Cameron]

On Thu, 30 Oct 2025 08:27:50 +0100
Francesco Lavra <flavra@baylibre.com> wrote:

> In preparation for adding support for more event types, use an

General comment.  Wrap commit descriptions a bit longer.  Standard is 75 chars.
This is about 68.

> array indexed by event ID instead of a scalar value to store
> enabled events, and refactor the functions to configure and report
> events so that their implementation is not specific for wakeup
> events. Move the logic to update the global event interrupt enable
> flag from st_lsm6dsx_event_setup() to its calling function, so that
> it can take into account also event sources different from the
> source being configured. While changing the signature of the
> st_lsm6dsx_event_setup() function, opportunistically add the
> currently unused `axis` parameter, which will be used when adding
> support for enabling and disabling events on a per axis basis.

I have nothing to add to Andy's review on the code.

[PATCH 8/9] iio: imu: st_lsm6dsx: add event configurability on a per axis basis

[Francesco Lavra]

In order to be able to configure event detection on a per axis
basis (for either setting an event threshold/sensitivity value, or
enabling/disabling event detection), add new axis-specific fields
to struct st_lsm6dsx_event_src, and modify the logic that handles
event configuration to properly handle axis-specific settings when
supported by a given event source.
A future commit will add actual event sources with per-axis
configurability.

Signed-off-by: Francesco Lavra <flavra@baylibre.com>
---
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  7 ++
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 77 ++++++++++++++++----
 2 files changed, 70 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
index 0e0642ca1b6f..62edd177c87c 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
@@ -228,7 +228,14 @@ enum st_lsm6dsx_event_id {
 
 struct st_lsm6dsx_event_src {
 	struct st_lsm6dsx_reg value;
+	struct st_lsm6dsx_reg x_value;
+	struct st_lsm6dsx_reg y_value;
+	struct st_lsm6dsx_reg z_value;
 	u8 enable_mask;
+	u8 enable_axis_reg;
+	u8 enable_x_mask;
+	u8 enable_y_mask;
+	u8 enable_z_mask;
 	u8 status_reg;
 	u8 status_mask;
 	u8 status_x_mask;
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index 87d40e70ca26..6d1b7b2a371a 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -1802,10 +1802,38 @@ static int st_lsm6dsx_event_setup(struct st_lsm6dsx_hw *hw, enum st_lsm6dsx_even
 	u8 enable_mask;
 	unsigned int data;
 	int err;
+	u8 old_enable, new_enable;
 
 	if (!hw->irq_routing)
 		return -ENOTSUPP;
 
+	if (src->enable_axis_reg) {
+		switch (axis) {
+		case IIO_MOD_X:
+			enable_mask = src->enable_x_mask;
+			break;
+		case IIO_MOD_Y:
+			enable_mask = src->enable_y_mask;
+			break;
+		case IIO_MOD_Z:
+			enable_mask = src->enable_z_mask;
+			break;
+		default:
+			enable_mask = 0;
+		}
+		if (enable_mask) {
+			data = ST_LSM6DSX_SHIFT_VAL(state, enable_mask);
+			err = st_lsm6dsx_update_bits_locked(hw, src->enable_axis_reg,
+							    enable_mask, data);
+			if (err < 0)
+				return err;
+		}
+	}
+	old_enable = hw->enable_event[event];
+	new_enable = state ? (old_enable | BIT(axis)) : (old_enable & ~BIT(axis));
+	if (!!old_enable == !!new_enable)
+		return 0;
+
 	/* Enable/disable event interrupt */
 	enable_mask = src->enable_mask;
 	data = ST_LSM6DSX_SHIFT_VAL(state, enable_mask);
@@ -1823,6 +1851,31 @@ static enum st_lsm6dsx_event_id st_lsm6dsx_get_event_id(enum iio_event_type type
 	}
 }
 
+static const struct st_lsm6dsx_reg *st_lsm6dsx_get_event_reg(struct st_lsm6dsx_hw *hw,
+							     enum st_lsm6dsx_event_id event,
+							     const struct iio_chan_spec *chan)
+{
+	const struct st_lsm6dsx_event_src *src = &hw->settings->event_settings.sources[event];
+	const struct st_lsm6dsx_reg *reg;
+
+	switch (chan->channel2) {
+	case IIO_MOD_X:
+		reg = &src->x_value;
+		break;
+	case IIO_MOD_Y:
+		reg = &src->y_value;
+		break;
+	case IIO_MOD_Z:
+		reg = &src->z_value;
+		break;
+	default:
+		return NULL;
+	}
+	if (!reg->addr)
+		reg = &src->value;
+	return reg;
+}
+
 static int st_lsm6dsx_read_event(struct iio_dev *iio_dev,
 				 const struct iio_chan_spec *chan,
 				 enum iio_event_type type,
@@ -1840,7 +1893,10 @@ static int st_lsm6dsx_read_event(struct iio_dev *iio_dev,
 	if (event == ST_LSM6DSX_EVENT_MAX)
 		return -EINVAL;
 
-	reg = &hw->settings->event_settings.sources[event].value;
+	reg = st_lsm6dsx_get_event_reg(hw, event, chan);
+	if (!reg)
+		return -EINVAL;
+
 	err = st_lsm6dsx_read_locked(hw, reg->addr, &data, sizeof(data));
 	if (err < 0)
 		return err;
@@ -1872,7 +1928,10 @@ st_lsm6dsx_write_event(struct iio_dev *iio_dev,
 	if (val < 0 || val > 31)
 		return -EINVAL;
 
-	reg = &hw->settings->event_settings.sources[event].value;
+	reg = st_lsm6dsx_get_event_reg(hw, event, chan);
+	if (!reg)
+		return -EINVAL;
+
 	data = ST_LSM6DSX_SHIFT_VAL(val, reg->mask);
 	err = st_lsm6dsx_update_bits_locked(hw, reg->addr,
 					    reg->mask, data);
@@ -1915,20 +1974,11 @@ st_lsm6dsx_write_event_config(struct iio_dev *iio_dev,
 	if (event == ST_LSM6DSX_EVENT_MAX)
 		return -EINVAL;
 
-	if (state) {
+	if (state)
 		enable_event = hw->enable_event[event] | BIT(axis);
-
-		/* do not enable events if they are already enabled */
-		if (hw->enable_event[event])
-			goto out;
-	} else {
+	else
 		enable_event = hw->enable_event[event] & ~BIT(axis);
 
-		/* only turn off sensor if no events is enabled */
-		if (enable_event)
-			goto out;
-	}
-
 	/* stop here if no changes have been made */
 	if (hw->enable_event[event] == enable_event)
 		return 0;
@@ -1965,7 +2015,6 @@ st_lsm6dsx_write_event_config(struct iio_dev *iio_dev,
 	if (err < 0)
 		return err;
 
-out:
 	hw->enable_event[event] = enable_event;
 
 	return 0;
-- 
2.39.5

Re: [PATCH 8/9] iio: imu: st_lsm6dsx: add event configurability on a per axis basis

[Andy Shevchenko]

On Thu, Oct 30, 2025 at 08:27:51AM +0100, Francesco Lavra wrote:
> In order to be able to configure event detection on a per axis
> basis (for either setting an event threshold/sensitivity value, or
> enabling/disabling event detection), add new axis-specific fields
> to struct st_lsm6dsx_event_src, and modify the logic that handles
> event configuration to properly handle axis-specific settings when
> supported by a given event source.
> A future commit will add actual event sources with per-axis
> configurability.

...

> +	old_enable = hw->enable_event[event];
> +	new_enable = state ? (old_enable | BIT(axis)) : (old_enable & ~BIT(axis));
> +	if (!!old_enable == !!new_enable)

This is an interesting check. So, old_enable and new_enable are _not_ booleans, right?
So, this means the check test if _any_ of the bit was set and kept set or none were set
and non is going to be set. Correct? I think a short comment would be good to have.

> +		return 0;

...

> +static const struct st_lsm6dsx_reg *st_lsm6dsx_get_event_reg(struct st_lsm6dsx_hw *hw,
> +							     enum st_lsm6dsx_event_id event,
> +							     const struct iio_chan_spec *chan)
> +{
> +	const struct st_lsm6dsx_event_src *src = &hw->settings->event_settings.sources[event];
> +	const struct st_lsm6dsx_reg *reg;
> +
> +	switch (chan->channel2) {
> +	case IIO_MOD_X:
> +		reg = &src->x_value;
> +		break;
> +	case IIO_MOD_Y:
> +		reg = &src->y_value;
> +		break;
> +	case IIO_MOD_Z:
> +		reg = &src->z_value;
> +		break;
> +	default:
> +		return NULL;
> +	}

> +	if (!reg->addr)
> +		reg = &src->value;
> +	return reg;

	if (reg->addr)
		return reg;

	/* Perhaps a comment here to explain the choice */
	return &src->value;

> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 8/9] iio: imu: st_lsm6dsx: add event configurability on a per axis basis

[Francesco Lavra]

[-- Attachment #1: Type: text/plain, Size: 2819 bytes --]

On Thu, 2025-10-30 at 10:24 +0200, Andy Shevchenko wrote:
> On Thu, Oct 30, 2025 at 08:27:51AM +0100, Francesco Lavra wrote:
> > In order to be able to configure event detection on a per axis
> > basis (for either setting an event threshold/sensitivity value, or
> > enabling/disabling event detection), add new axis-specific fields
> > to struct st_lsm6dsx_event_src, and modify the logic that handles
> > event configuration to properly handle axis-specific settings when
> > supported by a given event source.
> > A future commit will add actual event sources with per-axis
> > configurability.
> 
> ...
> 
> > +       old_enable = hw->enable_event[event];
> > +       new_enable = state ? (old_enable | BIT(axis)) : (old_enable &
> > ~BIT(axis));
> > +       if (!!old_enable == !!new_enable)
> 
> This is an interesting check. So, old_enable and new_enable are _not_
> booleans, right?
> So, this means the check test if _any_ of the bit was set and kept set or
> none were set
> and non is going to be set. Correct? I think a short comment would be
> good to have.

old_enable and new_enable are bit masks, but we are only interested in
whether any bit is set, to catch the cases where the bit mask goes from
zero to non-zero and vice versa. Will add a comment.

> 
> > +               return 0;
> 
> ...
> 
> > +static const struct st_lsm6dsx_reg *st_lsm6dsx_get_event_reg(struct
> > st_lsm6dsx_hw *hw,
> > +                                                            enum
> > st_lsm6dsx_event_id event,
> > +                                                            const
> > struct iio_chan_spec *chan)
> > +{
> > +       const struct st_lsm6dsx_event_src *src = &hw->settings-
> > >event_settings.sources[event];
> > +       const struct st_lsm6dsx_reg *reg;
> > +
> > +       switch (chan->channel2) {
> > +       case IIO_MOD_X:
> > +               reg = &src->x_value;
> > +               break;
> > +       case IIO_MOD_Y:
> > +               reg = &src->y_value;
> > +               break;
> > +       case IIO_MOD_Z:
> > +               reg = &src->z_value;
> > +               break;
> > +       default:
> > +               return NULL;
> > +       }
> 
> > +       if (!reg->addr)
> > +               reg = &src->value;
> > +       return reg;
> 
>         if (reg->addr)
>                 return reg;
> 
>         /* Perhaps a comment here to explain the choice */
>         return &src->value;
> > 
Will do.
> 


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 8/9] iio: imu: st_lsm6dsx: add event configurability on a per axis basis

[Andy Shevchenko]

On Thu, Oct 30, 2025 at 12:23:19PM +0100, Francesco Lavra wrote:
> On Thu, 2025-10-30 at 10:24 +0200, Andy Shevchenko wrote:
> > On Thu, Oct 30, 2025 at 08:27:51AM +0100, Francesco Lavra wrote:

...

> > > +       old_enable = hw->enable_event[event];
> > > +       new_enable = state ? (old_enable | BIT(axis)) : (old_enable &
> > > ~BIT(axis));
> > > +       if (!!old_enable == !!new_enable)
> > 
> > This is an interesting check. So, old_enable and new_enable are _not_
> > booleans, right?
> > So, this means the check test if _any_ of the bit was set and kept set or
> > none were set
> > and non is going to be set. Correct? I think a short comment would be
> > good to have.
> 
> old_enable and new_enable are bit masks, but we are only interested in
> whether any bit is set, to catch the cases where the bit mask goes from
> zero to non-zero and vice versa. Will add a comment.

If it's a true bitmask (assuming unsigned long type) then all this can be done
via bitmap API calls. Otherwise you can also compare a Hamming weights of them
(probably that gives even the same size of the object file, but !! instructions
 will be changed to hweight() calls (still a single assembly instr on modern
 architectures).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 8/9] iio: imu: st_lsm6dsx: add event configurability on a per axis basis

[Francesco Lavra]

[-- Attachment #1: Type: text/plain, Size: 1579 bytes --]

On Thu, 2025-10-30 at 15:56 +0200, Andy Shevchenko wrote:
> On Thu, Oct 30, 2025 at 12:23:19PM +0100, Francesco Lavra wrote:
> > On Thu, 2025-10-30 at 10:24 +0200, Andy Shevchenko wrote:
> > > On Thu, Oct 30, 2025 at 08:27:51AM +0100, Francesco Lavra wrote:
> 
> ...
> 
> > > > +       old_enable = hw->enable_event[event];
> > > > +       new_enable = state ? (old_enable | BIT(axis)) : (old_enable
> > > > &
> > > > ~BIT(axis));
> > > > +       if (!!old_enable == !!new_enable)
> > > 
> > > This is an interesting check. So, old_enable and new_enable are _not_
> > > booleans, right?
> > > So, this means the check test if _any_ of the bit was set and kept
> > > set or
> > > none were set
> > > and non is going to be set. Correct? I think a short comment would be
> > > good to have.
> > 
> > old_enable and new_enable are bit masks, but we are only interested in
> > whether any bit is set, to catch the cases where the bit mask goes from
> > zero to non-zero and vice versa. Will add a comment.
> 
> If it's a true bitmask (assuming unsigned long type) then all this can be
> done
> via bitmap API calls. Otherwise you can also compare a Hamming weights of
> them
> (probably that gives even the same size of the object file, but !!
> instructions
>  will be changed to hweight() calls (still a single assembly instr on
> modern
>  architectures).

These are u8 variables, so we can't use the bitmap API. And I don't
understand the reason for using hweight(), given that the !! operators
would still be needed.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 8/9] iio: imu: st_lsm6dsx: add event configurability on a per axis basis

[Andy Shevchenko]

On Mon, Nov 17, 2025 at 08:23:35PM +0100, Francesco Lavra wrote:
> On Thu, 2025-10-30 at 15:56 +0200, Andy Shevchenko wrote:
> > On Thu, Oct 30, 2025 at 12:23:19PM +0100, Francesco Lavra wrote:
> > > On Thu, 2025-10-30 at 10:24 +0200, Andy Shevchenko wrote:
> > > > On Thu, Oct 30, 2025 at 08:27:51AM +0100, Francesco Lavra wrote:

...

> > > > > +       old_enable = hw->enable_event[event];
> > > > > +       new_enable = state ? (old_enable | BIT(axis)) : (old_enable
> > > > > &
> > > > > ~BIT(axis));
> > > > > +       if (!!old_enable == !!new_enable)
> > > > 
> > > > This is an interesting check. So, old_enable and new_enable are _not_
> > > > booleans, right?
> > > > So, this means the check test if _any_ of the bit was set and kept
> > > > set or
> > > > none were set
> > > > and non is going to be set. Correct? I think a short comment would be
> > > > good to have.
> > > 
> > > old_enable and new_enable are bit masks, but we are only interested in
> > > whether any bit is set, to catch the cases where the bit mask goes from
> > > zero to non-zero and vice versa. Will add a comment.
> > 
> > If it's a true bitmask (assuming unsigned long type) then all this can be
> > done
> > via bitmap API calls. Otherwise you can also compare a Hamming weights of
> > them
> > (probably that gives even the same size of the object file, but !!
> > instructions
> >  will be changed to hweight() calls (still a single assembly instr on
> > modern
> >  architectures).
> 
> These are u8 variables, so we can't use the bitmap API.

OK. But hweight8() can still be used.

> And I don't
> understand the reason for using hweight(), given that the !! operators
> would still be needed.

No, you won't need !! with that.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 8/9] iio: imu: st_lsm6dsx: add event configurability on a per axis basis

[Francesco Lavra]

On Tue, 2025-11-18 at 11:44 +0100, Andy Shevchenko wrote:
> On Mon, Nov 17, 2025 at 08:23:35PM +0100, Francesco Lavra wrote:
> > On Thu, 2025-10-30 at 15:56 +0200, Andy Shevchenko wrote:
> > > On Thu, Oct 30, 2025 at 12:23:19PM +0100, Francesco Lavra wrote:
> > > > On Thu, 2025-10-30 at 10:24 +0200, Andy Shevchenko wrote:
> > > > > On Thu, Oct 30, 2025 at 08:27:51AM +0100, Francesco Lavra wrote:
> 
> ...
> 
> > > > > > +       old_enable = hw->enable_event[event];
> > > > > > +       new_enable = state ? (old_enable | BIT(axis)) :
> > > > > > (old_enable
> > > > > > &
> > > > > > ~BIT(axis));
> > > > > > +       if (!!old_enable == !!new_enable)
> > > > > 
> > > > > This is an interesting check. So, old_enable and new_enable are
> > > > > _not_
> > > > > booleans, right?
> > > > > So, this means the check test if _any_ of the bit was set and
> > > > > kept
> > > > > set or
> > > > > none were set
> > > > > and non is going to be set. Correct? I think a short comment
> > > > > would be
> > > > > good to have.
> > > > 
> > > > old_enable and new_enable are bit masks, but we are only interested
> > > > in
> > > > whether any bit is set, to catch the cases where the bit mask goes
> > > > from
> > > > zero to non-zero and vice versa. Will add a comment.
> > > 
> > > If it's a true bitmask (assuming unsigned long type) then all this
> > > can be
> > > done
> > > via bitmap API calls. Otherwise you can also compare a Hamming
> > > weights of
> > > them
> > > (probably that gives even the same size of the object file, but !!
> > > instructions
> > >  will be changed to hweight() calls (still a single assembly instr on
> > > modern
> > >  architectures).
> > 
> > These are u8 variables, so we can't use the bitmap API.
> 
> OK. But hweight8() can still be used.
> 
> > And I don't
> > understand the reason for using hweight(), given that the !! operators
> > would still be needed.
> 
> No, you won't need !! with that.

I still don't understand. Are you proposing to replace `if (!!old_enable ==
!!new_enable)` with `if (hweight8(old_enable) == hweight8(new_enable))`?
That won't work, because we only need to check whether the Hamming weight
goes from zero to non-zero and vice versa.

Re: [PATCH 8/9] iio: imu: st_lsm6dsx: add event configurability on a per axis basis

[Andy Shevchenko]

On Tue, Nov 18, 2025 at 12:01:57PM +0100, Francesco Lavra wrote:
> On Tue, 2025-11-18 at 11:44 +0100, Andy Shevchenko wrote:
> > On Mon, Nov 17, 2025 at 08:23:35PM +0100, Francesco Lavra wrote:
> > > On Thu, 2025-10-30 at 15:56 +0200, Andy Shevchenko wrote:
> > > > On Thu, Oct 30, 2025 at 12:23:19PM +0100, Francesco Lavra wrote:
> > > > > On Thu, 2025-10-30 at 10:24 +0200, Andy Shevchenko wrote:
> > > > > > On Thu, Oct 30, 2025 at 08:27:51AM +0100, Francesco Lavra wrote:

...

> > > > > > > +       old_enable = hw->enable_event[event];
> > > > > > > +       new_enable = state ? (old_enable | BIT(axis)) :
> > > > > > > (old_enable
> > > > > > > &
> > > > > > > ~BIT(axis));
> > > > > > > +       if (!!old_enable == !!new_enable)
> > > > > > 
> > > > > > This is an interesting check. So, old_enable and new_enable are
> > > > > > _not_
> > > > > > booleans, right?
> > > > > > So, this means the check test if _any_ of the bit was set and
> > > > > > kept
> > > > > > set or
> > > > > > none were set
> > > > > > and non is going to be set. Correct? I think a short comment
> > > > > > would be
> > > > > > good to have.
> > > > > 
> > > > > old_enable and new_enable are bit masks, but we are only interested
> > > > > in
> > > > > whether any bit is set, to catch the cases where the bit mask goes
> > > > > from
> > > > > zero to non-zero and vice versa. Will add a comment.
> > > > 
> > > > If it's a true bitmask (assuming unsigned long type) then all this
> > > > can be
> > > > done
> > > > via bitmap API calls. Otherwise you can also compare a Hamming
> > > > weights of
> > > > them
> > > > (probably that gives even the same size of the object file, but !!
> > > > instructions
> > > >  will be changed to hweight() calls (still a single assembly instr on
> > > > modern
> > > >  architectures).
> > > 
> > > These are u8 variables, so we can't use the bitmap API.
> > 
> > OK. But hweight8() can still be used.
> > 
> > > And I don't
> > > understand the reason for using hweight(), given that the !! operators
> > > would still be needed.
> > 
> > No, you won't need !! with that.
> 
> I still don't understand. Are you proposing to replace `if (!!old_enable ==
> !!new_enable)` with `if (hweight8(old_enable) == hweight8(new_enable))`?
> That won't work, because we only need to check whether the Hamming weight
> goes from zero to non-zero and vice versa.

       old_enable = hw->enable_event[event];
       new_enable = state ? (old_enable | BIT(axis)) :
                            (old_enable & ~BIT(axis));
       if (!!old_enable == !!new_enable)
               return 0;

If I am not mistaken this will do exactly the same in a simpler way.

	old_enable = hw->enable_event[event];
	if (state)
		new_enable = old_enable | BIT(axis);
	else
		new_enable = old_enable & ~BIT(axis);
	if ((new_enable ^ old_enable) != BIT(axis))
		return 0;


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 8/9] iio: imu: st_lsm6dsx: add event configurability on a per axis basis

[Francesco Lavra]

[-- Attachment #1: Type: text/plain, Size: 3931 bytes --]

On Thu, 2025-11-20 at 10:05 +0100, Andy Shevchenko wrote:
> On Tue, Nov 18, 2025 at 12:01:57PM +0100, Francesco Lavra wrote:
> > On Tue, 2025-11-18 at 11:44 +0100, Andy Shevchenko wrote:
> > > On Mon, Nov 17, 2025 at 08:23:35PM +0100, Francesco Lavra wrote:
> > > > On Thu, 2025-10-30 at 15:56 +0200, Andy Shevchenko wrote:
> > > > > On Thu, Oct 30, 2025 at 12:23:19PM +0100, Francesco Lavra wrote:
> > > > > > On Thu, 2025-10-30 at 10:24 +0200, Andy Shevchenko wrote:
> > > > > > > On Thu, Oct 30, 2025 at 08:27:51AM +0100, Francesco Lavra
> > > > > > > wrote:
> 
> ...
> 
> > > > > > > > +       old_enable = hw->enable_event[event];
> > > > > > > > +       new_enable = state ? (old_enable | BIT(axis)) :
> > > > > > > > (old_enable
> > > > > > > > &
> > > > > > > > ~BIT(axis));
> > > > > > > > +       if (!!old_enable == !!new_enable)
> > > > > > > 
> > > > > > > This is an interesting check. So, old_enable and new_enable
> > > > > > > are
> > > > > > > _not_
> > > > > > > booleans, right?
> > > > > > > So, this means the check test if _any_ of the bit was set and
> > > > > > > kept
> > > > > > > set or
> > > > > > > none were set
> > > > > > > and non is going to be set. Correct? I think a short comment
> > > > > > > would be
> > > > > > > good to have.
> > > > > > 
> > > > > > old_enable and new_enable are bit masks, but we are only
> > > > > > interested
> > > > > > in
> > > > > > whether any bit is set, to catch the cases where the bit mask
> > > > > > goes
> > > > > > from
> > > > > > zero to non-zero and vice versa. Will add a comment.
> > > > > 
> > > > > If it's a true bitmask (assuming unsigned long type) then all
> > > > > this
> > > > > can be
> > > > > done
> > > > > via bitmap API calls. Otherwise you can also compare a Hamming
> > > > > weights of
> > > > > them
> > > > > (probably that gives even the same size of the object file, but
> > > > > !!
> > > > > instructions
> > > > >  will be changed to hweight() calls (still a single assembly
> > > > > instr on
> > > > > modern
> > > > >  architectures).
> > > > 
> > > > These are u8 variables, so we can't use the bitmap API.
> > > 
> > > OK. But hweight8() can still be used.
> > > 
> > > > And I don't
> > > > understand the reason for using hweight(), given that the !!
> > > > operators
> > > > would still be needed.
> > > 
> > > No, you won't need !! with that.
> > 
> > I still don't understand. Are you proposing to replace `if
> > (!!old_enable ==
> > !!new_enable)` with `if (hweight8(old_enable) ==
> > hweight8(new_enable))`?
> > That won't work, because we only need to check whether the Hamming
> > weight
> > goes from zero to non-zero and vice versa.
> 
>        old_enable = hw->enable_event[event];
>        new_enable = state ? (old_enable | BIT(axis)) :
>                             (old_enable & ~BIT(axis));
>        if (!!old_enable == !!new_enable)
>                return 0;
> 
> If I am not mistaken this will do exactly the same in a simpler way.
> 
>         old_enable = hw->enable_event[event];
>         if (state)
>                 new_enable = old_enable | BIT(axis);
>         else
>                 new_enable = old_enable & ~BIT(axis);
>         if ((new_enable ^ old_enable) != BIT(axis))
>                 return 0;

This doesn't look right to me, if new_enable differs from old_enable by
just one bit (which it does), then the XOR result will always have this bit
(and no others) set, so (new_enable ^ old_enable) will always equal
BIT(axis).
We are not checking if the bit was already set or clear, when this code
runs we already know that the bit is changing, what we are checking is
whether all bits are zero before or after this change.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 8/9] iio: imu: st_lsm6dsx: add event configurability on a per axis basis

[Andy Shevchenko]

On Thu, Nov 20, 2025 at 12:43:09PM +0100, Francesco Lavra wrote:
> On Thu, 2025-11-20 at 10:05 +0100, Andy Shevchenko wrote:
> > On Tue, Nov 18, 2025 at 12:01:57PM +0100, Francesco Lavra wrote:
> > > On Tue, 2025-11-18 at 11:44 +0100, Andy Shevchenko wrote:
> > > > On Mon, Nov 17, 2025 at 08:23:35PM +0100, Francesco Lavra wrote:
> > > > > On Thu, 2025-10-30 at 15:56 +0200, Andy Shevchenko wrote:
> > > > > > On Thu, Oct 30, 2025 at 12:23:19PM +0100, Francesco Lavra wrote:
> > > > > > > On Thu, 2025-10-30 at 10:24 +0200, Andy Shevchenko wrote:
> > > > > > > > On Thu, Oct 30, 2025 at 08:27:51AM +0100, Francesco Lavra
> > > > > > > > wrote:

...

> > > > > > > > > +       old_enable = hw->enable_event[event];
> > > > > > > > > +       new_enable = state ? (old_enable | BIT(axis)) :
> > > > > > > > > (old_enable
> > > > > > > > > &
> > > > > > > > > ~BIT(axis));
> > > > > > > > > +       if (!!old_enable == !!new_enable)
> > > > > > > > 
> > > > > > > > This is an interesting check. So, old_enable and new_enable
> > > > > > > > are
> > > > > > > > _not_
> > > > > > > > booleans, right?
> > > > > > > > So, this means the check test if _any_ of the bit was set and
> > > > > > > > kept
> > > > > > > > set or
> > > > > > > > none were set
> > > > > > > > and non is going to be set. Correct? I think a short comment
> > > > > > > > would be
> > > > > > > > good to have.
> > > > > > > 
> > > > > > > old_enable and new_enable are bit masks, but we are only
> > > > > > > interested
> > > > > > > in
> > > > > > > whether any bit is set, to catch the cases where the bit mask
> > > > > > > goes
> > > > > > > from
> > > > > > > zero to non-zero and vice versa. Will add a comment.
> > > > > > 
> > > > > > If it's a true bitmask (assuming unsigned long type) then all
> > > > > > this
> > > > > > can be
> > > > > > done
> > > > > > via bitmap API calls. Otherwise you can also compare a Hamming
> > > > > > weights of
> > > > > > them
> > > > > > (probably that gives even the same size of the object file, but
> > > > > > !!
> > > > > > instructions
> > > > > >  will be changed to hweight() calls (still a single assembly
> > > > > > instr on
> > > > > > modern
> > > > > >  architectures).
> > > > > 
> > > > > These are u8 variables, so we can't use the bitmap API.
> > > > 
> > > > OK. But hweight8() can still be used.
> > > > 
> > > > > And I don't
> > > > > understand the reason for using hweight(), given that the !!
> > > > > operators
> > > > > would still be needed.
> > > > 
> > > > No, you won't need !! with that.
> > > 
> > > I still don't understand. Are you proposing to replace `if
> > > (!!old_enable ==
> > > !!new_enable)` with `if (hweight8(old_enable) ==
> > > hweight8(new_enable))`?
> > > That won't work, because we only need to check whether the Hamming
> > > weight
> > > goes from zero to non-zero and vice versa.
> > 
> >        old_enable = hw->enable_event[event];
> >        new_enable = state ? (old_enable | BIT(axis)) :
> >                             (old_enable & ~BIT(axis));
> >        if (!!old_enable == !!new_enable)
> >                return 0;
> > 
> > If I am not mistaken this will do exactly the same in a simpler way.
> > 
> >         old_enable = hw->enable_event[event];
> >         if (state)
> >                 new_enable = old_enable | BIT(axis);
> >         else
> >                 new_enable = old_enable & ~BIT(axis);
> >         if ((new_enable ^ old_enable) != BIT(axis))
> >                 return 0;
> 
> This doesn't look right to me, if new_enable differs from old_enable by
> just one bit (which it does), then the XOR result will always have this bit
> (and no others) set, so (new_enable ^ old_enable) will always equal
> BIT(axis).
> We are not checking if the bit was already set or clear, when this code
> runs we already know that the bit is changing, what we are checking is
> whether all bits are zero before or after this change.

The check I proposed is to have a look for the cases when old_enable was 0 and
the BIT(axis) is set and when the BIT(axis) was _the last_ bit in the mask that
got reset. If it's not the case, the code will return 0. I think my check is
correct.

Should I write a test case?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 8/9] iio: imu: st_lsm6dsx: add event configurability on a per axis basis

[Andy Shevchenko]

On Thu, Nov 20, 2025 at 03:59:18PM +0200, Andy Shevchenko wrote:
> On Thu, Nov 20, 2025 at 12:43:09PM +0100, Francesco Lavra wrote:
> > On Thu, 2025-11-20 at 10:05 +0100, Andy Shevchenko wrote:
> > > On Tue, Nov 18, 2025 at 12:01:57PM +0100, Francesco Lavra wrote:
> > > > On Tue, 2025-11-18 at 11:44 +0100, Andy Shevchenko wrote:
> > > > > On Mon, Nov 17, 2025 at 08:23:35PM +0100, Francesco Lavra wrote:
> > > > > > On Thu, 2025-10-30 at 15:56 +0200, Andy Shevchenko wrote:
> > > > > > > On Thu, Oct 30, 2025 at 12:23:19PM +0100, Francesco Lavra wrote:
> > > > > > > > On Thu, 2025-10-30 at 10:24 +0200, Andy Shevchenko wrote:
> > > > > > > > > On Thu, Oct 30, 2025 at 08:27:51AM +0100, Francesco Lavra
> > > > > > > > > wrote:

...

> > > > > > > > > > +       old_enable = hw->enable_event[event];
> > > > > > > > > > +       new_enable = state ? (old_enable | BIT(axis)) :
> > > > > > > > > > (old_enable
> > > > > > > > > > &
> > > > > > > > > > ~BIT(axis));
> > > > > > > > > > +       if (!!old_enable == !!new_enable)
> > > > > > > > > 
> > > > > > > > > This is an interesting check. So, old_enable and new_enable
> > > > > > > > > are
> > > > > > > > > _not_
> > > > > > > > > booleans, right?
> > > > > > > > > So, this means the check test if _any_ of the bit was set and
> > > > > > > > > kept
> > > > > > > > > set or
> > > > > > > > > none were set
> > > > > > > > > and non is going to be set. Correct? I think a short comment
> > > > > > > > > would be
> > > > > > > > > good to have.
> > > > > > > > 
> > > > > > > > old_enable and new_enable are bit masks, but we are only
> > > > > > > > interested
> > > > > > > > in
> > > > > > > > whether any bit is set, to catch the cases where the bit mask
> > > > > > > > goes
> > > > > > > > from
> > > > > > > > zero to non-zero and vice versa. Will add a comment.
> > > > > > > 
> > > > > > > If it's a true bitmask (assuming unsigned long type) then all
> > > > > > > this
> > > > > > > can be
> > > > > > > done
> > > > > > > via bitmap API calls. Otherwise you can also compare a Hamming
> > > > > > > weights of
> > > > > > > them
> > > > > > > (probably that gives even the same size of the object file, but
> > > > > > > !!
> > > > > > > instructions
> > > > > > >  will be changed to hweight() calls (still a single assembly
> > > > > > > instr on
> > > > > > > modern
> > > > > > >  architectures).
> > > > > > 
> > > > > > These are u8 variables, so we can't use the bitmap API.
> > > > > 
> > > > > OK. But hweight8() can still be used.
> > > > > 
> > > > > > And I don't
> > > > > > understand the reason for using hweight(), given that the !!
> > > > > > operators
> > > > > > would still be needed.
> > > > > 
> > > > > No, you won't need !! with that.
> > > > 
> > > > I still don't understand. Are you proposing to replace `if
> > > > (!!old_enable ==
> > > > !!new_enable)` with `if (hweight8(old_enable) ==
> > > > hweight8(new_enable))`?
> > > > That won't work, because we only need to check whether the Hamming
> > > > weight
> > > > goes from zero to non-zero and vice versa.
> > > 
> > >        old_enable = hw->enable_event[event];
> > >        new_enable = state ? (old_enable | BIT(axis)) :
> > >                             (old_enable & ~BIT(axis));
> > >        if (!!old_enable == !!new_enable)
> > >                return 0;
> > > 
> > > If I am not mistaken this will do exactly the same in a simpler way.
> > > 
> > >         old_enable = hw->enable_event[event];
> > >         if (state)
> > >                 new_enable = old_enable | BIT(axis);
> > >         else
> > >                 new_enable = old_enable & ~BIT(axis);
> > >         if ((new_enable ^ old_enable) != BIT(axis))
> > >                 return 0;
> > 
> > This doesn't look right to me, if new_enable differs from old_enable by
> > just one bit (which it does), then the XOR result will always have this bit
> > (and no others) set, so (new_enable ^ old_enable) will always equal
> > BIT(axis).
> > We are not checking if the bit was already set or clear, when this code
> > runs we already know that the bit is changing, what we are checking is
> > whether all bits are zero before or after this change.
> 
> The check I proposed is to have a look for the cases when old_enable was 0 and
> the BIT(axis) is set and when the BIT(axis) was _the last_ bit in the mask that
> got reset. If it's not the case, the code will return 0. I think my check is
> correct.
> 
> Should I write a test case?

FWIW, https://gist.github.com/andy-shev/afe4c0e009cb3008ac613d8384aaa6eb

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 8/9] iio: imu: st_lsm6dsx: add event configurability on a per axis basis

[Francesco Lavra]

[-- Attachment #1: Type: text/plain, Size: 6093 bytes --]

On Thu, 2025-11-20 at 20:31 +0200, Andy Shevchenko wrote:
> On Thu, Nov 20, 2025 at 03:59:18PM +0200, Andy Shevchenko wrote:
> > On Thu, Nov 20, 2025 at 12:43:09PM +0100, Francesco Lavra wrote:
> > > On Thu, 2025-11-20 at 10:05 +0100, Andy Shevchenko wrote:
> > > > On Tue, Nov 18, 2025 at 12:01:57PM +0100, Francesco Lavra wrote:
> > > > > On Tue, 2025-11-18 at 11:44 +0100, Andy Shevchenko wrote:
> > > > > > On Mon, Nov 17, 2025 at 08:23:35PM +0100, Francesco Lavra
> > > > > > wrote:
> > > > > > > On Thu, 2025-10-30 at 15:56 +0200, Andy Shevchenko wrote:
> > > > > > > > On Thu, Oct 30, 2025 at 12:23:19PM +0100, Francesco Lavra
> > > > > > > > wrote:
> > > > > > > > > On Thu, 2025-10-30 at 10:24 +0200, Andy Shevchenko wrote:
> > > > > > > > > > On Thu, Oct 30, 2025 at 08:27:51AM +0100, Francesco
> > > > > > > > > > Lavra
> > > > > > > > > > wrote:
> 
> ...
> 
> > > > > > > > > > > +       old_enable = hw->enable_event[event];
> > > > > > > > > > > +       new_enable = state ? (old_enable | BIT(axis))
> > > > > > > > > > > :
> > > > > > > > > > > (old_enable
> > > > > > > > > > > &
> > > > > > > > > > > ~BIT(axis));
> > > > > > > > > > > +       if (!!old_enable == !!new_enable)
> > > > > > > > > > 
> > > > > > > > > > This is an interesting check. So, old_enable and
> > > > > > > > > > new_enable
> > > > > > > > > > are
> > > > > > > > > > _not_
> > > > > > > > > > booleans, right?
> > > > > > > > > > So, this means the check test if _any_ of the bit was
> > > > > > > > > > set and
> > > > > > > > > > kept
> > > > > > > > > > set or
> > > > > > > > > > none were set
> > > > > > > > > > and non is going to be set. Correct? I think a short
> > > > > > > > > > comment
> > > > > > > > > > would be
> > > > > > > > > > good to have.
> > > > > > > > > 
> > > > > > > > > old_enable and new_enable are bit masks, but we are only
> > > > > > > > > interested
> > > > > > > > > in
> > > > > > > > > whether any bit is set, to catch the cases where the bit
> > > > > > > > > mask
> > > > > > > > > goes
> > > > > > > > > from
> > > > > > > > > zero to non-zero and vice versa. Will add a comment.
> > > > > > > > 
> > > > > > > > If it's a true bitmask (assuming unsigned long type) then
> > > > > > > > all
> > > > > > > > this
> > > > > > > > can be
> > > > > > > > done
> > > > > > > > via bitmap API calls. Otherwise you can also compare a
> > > > > > > > Hamming
> > > > > > > > weights of
> > > > > > > > them
> > > > > > > > (probably that gives even the same size of the object file,
> > > > > > > > but
> > > > > > > > !!
> > > > > > > > instructions
> > > > > > > >  will be changed to hweight() calls (still a single
> > > > > > > > assembly
> > > > > > > > instr on
> > > > > > > > modern
> > > > > > > >  architectures).
> > > > > > > 
> > > > > > > These are u8 variables, so we can't use the bitmap API.
> > > > > > 
> > > > > > OK. But hweight8() can still be used.
> > > > > > 
> > > > > > > And I don't
> > > > > > > understand the reason for using hweight(), given that the !!
> > > > > > > operators
> > > > > > > would still be needed.
> > > > > > 
> > > > > > No, you won't need !! with that.
> > > > > 
> > > > > I still don't understand. Are you proposing to replace `if
> > > > > (!!old_enable ==
> > > > > !!new_enable)` with `if (hweight8(old_enable) ==
> > > > > hweight8(new_enable))`?
> > > > > That won't work, because we only need to check whether the
> > > > > Hamming
> > > > > weight
> > > > > goes from zero to non-zero and vice versa.
> > > > 
> > > >        old_enable = hw->enable_event[event];
> > > >        new_enable = state ? (old_enable | BIT(axis)) :
> > > >                             (old_enable & ~BIT(axis));
> > > >        if (!!old_enable == !!new_enable)
> > > >                return 0;
> > > > 
> > > > If I am not mistaken this will do exactly the same in a simpler
> > > > way.
> > > > 
> > > >         old_enable = hw->enable_event[event];
> > > >         if (state)
> > > >                 new_enable = old_enable | BIT(axis);
> > > >         else
> > > >                 new_enable = old_enable & ~BIT(axis);
> > > >         if ((new_enable ^ old_enable) != BIT(axis))
> > > >                 return 0;
> > > 
> > > This doesn't look right to me, if new_enable differs from old_enable
> > > by
> > > just one bit (which it does), then the XOR result will always have
> > > this bit
> > > (and no others) set, so (new_enable ^ old_enable) will always equal
> > > BIT(axis).
> > > We are not checking if the bit was already set or clear, when this
> > > code
> > > runs we already know that the bit is changing, what we are checking
> > > is
> > > whether all bits are zero before or after this change.
> > 
> > The check I proposed is to have a look for the cases when old_enable
> > was 0 and
> > the BIT(axis) is set and when the BIT(axis) was _the last_ bit in the
> > mask that
> > got reset. If it's not the case, the code will return 0. I think my
> > check is
> > correct.
> > 
> > Should I write a test case?
> 
> FWIW, https://gist.github.com/andy-shev/afe4c0e009cb3008ac613d8384aaa6eb

The code in your gist produces:

Initial event: 0x92, new state: True for bit 0x20
[-] 0x00 | 0x20 --> 1: handle
[0] 0x92 | 0x20 --> 1: handle
[1] 0x93 | 0x20 --> 1: handle
[2] 0x93 | 0x20 --> 1: handle
[3] 0x97 | 0x20 --> 1: handle
[4] 0x9f | 0x20 --> 1: handle
[5] 0x9f | 0x20 --> 1: handle
[6] 0xbf | 0x20 --> 0: return
[7] 0xff | 0x20 --> 0: return
[-] 0xff | 0x20 --> 0: return

But this is not what I need. I need "handle" to be there only when the
bitmask goes from 0x00 to non-zero (in the above example, only at the first
[-] iteration); all the other cases should be a "return". Likewise, if
there is '&' instead of '|', I need "handle" to be there only when the
bitmask goes from non-zero to 0x00.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 8/9] iio: imu: st_lsm6dsx: add event configurability on a per axis basis

[Andy Shevchenko]

On Fri, Nov 21, 2025 at 10:14:06AM +0100, Francesco Lavra wrote:
> On Thu, 2025-11-20 at 20:31 +0200, Andy Shevchenko wrote:
> > On Thu, Nov 20, 2025 at 03:59:18PM +0200, Andy Shevchenko wrote:
> > > On Thu, Nov 20, 2025 at 12:43:09PM +0100, Francesco Lavra wrote:
> > > > On Thu, 2025-11-20 at 10:05 +0100, Andy Shevchenko wrote:
> > > > > On Tue, Nov 18, 2025 at 12:01:57PM +0100, Francesco Lavra wrote:
> > > > > > On Tue, 2025-11-18 at 11:44 +0100, Andy Shevchenko wrote:
> > > > > > > On Mon, Nov 17, 2025 at 08:23:35PM +0100, Francesco Lavra
> > > > > > > wrote:
> > > > > > > > On Thu, 2025-10-30 at 15:56 +0200, Andy Shevchenko wrote:
> > > > > > > > > On Thu, Oct 30, 2025 at 12:23:19PM +0100, Francesco Lavra
> > > > > > > > > wrote:
> > > > > > > > > > On Thu, 2025-10-30 at 10:24 +0200, Andy Shevchenko wrote:
> > > > > > > > > > > On Thu, Oct 30, 2025 at 08:27:51AM +0100, Francesco
> > > > > > > > > > > Lavra
> > > > > > > > > > > wrote:

...

> > > > > > > > > > > > +       old_enable = hw->enable_event[event];
> > > > > > > > > > > > +       new_enable = state ? (old_enable | BIT(axis))
> > > > > > > > > > > > :
> > > > > > > > > > > > (old_enable
> > > > > > > > > > > > &
> > > > > > > > > > > > ~BIT(axis));
> > > > > > > > > > > > +       if (!!old_enable == !!new_enable)
> > > > > > > > > > > 
> > > > > > > > > > > This is an interesting check. So, old_enable and
> > > > > > > > > > > new_enable
> > > > > > > > > > > are
> > > > > > > > > > > _not_
> > > > > > > > > > > booleans, right?
> > > > > > > > > > > So, this means the check test if _any_ of the bit was
> > > > > > > > > > > set and
> > > > > > > > > > > kept
> > > > > > > > > > > set or
> > > > > > > > > > > none were set
> > > > > > > > > > > and non is going to be set. Correct? I think a short
> > > > > > > > > > > comment
> > > > > > > > > > > would be
> > > > > > > > > > > good to have.
> > > > > > > > > > 
> > > > > > > > > > old_enable and new_enable are bit masks, but we are only
> > > > > > > > > > interested
> > > > > > > > > > in
> > > > > > > > > > whether any bit is set, to catch the cases where the bit
> > > > > > > > > > mask
> > > > > > > > > > goes
> > > > > > > > > > from
> > > > > > > > > > zero to non-zero and vice versa. Will add a comment.
> > > > > > > > > 
> > > > > > > > > If it's a true bitmask (assuming unsigned long type) then
> > > > > > > > > all
> > > > > > > > > this
> > > > > > > > > can be
> > > > > > > > > done
> > > > > > > > > via bitmap API calls. Otherwise you can also compare a
> > > > > > > > > Hamming
> > > > > > > > > weights of
> > > > > > > > > them
> > > > > > > > > (probably that gives even the same size of the object file,
> > > > > > > > > but
> > > > > > > > > !!
> > > > > > > > > instructions
> > > > > > > > >  will be changed to hweight() calls (still a single
> > > > > > > > > assembly
> > > > > > > > > instr on
> > > > > > > > > modern
> > > > > > > > >  architectures).
> > > > > > > > 
> > > > > > > > These are u8 variables, so we can't use the bitmap API.
> > > > > > > 
> > > > > > > OK. But hweight8() can still be used.
> > > > > > > 
> > > > > > > > And I don't
> > > > > > > > understand the reason for using hweight(), given that the !!
> > > > > > > > operators
> > > > > > > > would still be needed.
> > > > > > > 
> > > > > > > No, you won't need !! with that.
> > > > > > 
> > > > > > I still don't understand. Are you proposing to replace `if
> > > > > > (!!old_enable ==
> > > > > > !!new_enable)` with `if (hweight8(old_enable) ==
> > > > > > hweight8(new_enable))`?
> > > > > > That won't work, because we only need to check whether the
> > > > > > Hamming
> > > > > > weight
> > > > > > goes from zero to non-zero and vice versa.
> > > > > 
> > > > >        old_enable = hw->enable_event[event];
> > > > >        new_enable = state ? (old_enable | BIT(axis)) :
> > > > >                             (old_enable & ~BIT(axis));
> > > > >        if (!!old_enable == !!new_enable)
> > > > >                return 0;
> > > > > 
> > > > > If I am not mistaken this will do exactly the same in a simpler
> > > > > way.
> > > > > 
> > > > >         old_enable = hw->enable_event[event];
> > > > >         if (state)
> > > > >                 new_enable = old_enable | BIT(axis);
> > > > >         else
> > > > >                 new_enable = old_enable & ~BIT(axis);
> > > > >         if ((new_enable ^ old_enable) != BIT(axis))
> > > > >                 return 0;
> > > > 
> > > > This doesn't look right to me, if new_enable differs from old_enable
> > > > by
> > > > just one bit (which it does), then the XOR result will always have
> > > > this bit
> > > > (and no others) set, so (new_enable ^ old_enable) will always equal
> > > > BIT(axis).
> > > > We are not checking if the bit was already set or clear, when this
> > > > code
> > > > runs we already know that the bit is changing, what we are checking
> > > > is
> > > > whether all bits are zero before or after this change.
> > > 
> > > The check I proposed is to have a look for the cases when old_enable
> > > was 0 and
> > > the BIT(axis) is set and when the BIT(axis) was _the last_ bit in the
> > > mask that
> > > got reset. If it's not the case, the code will return 0. I think my
> > > check is
> > > correct.
> > > 
> > > Should I write a test case?
> > 
> > FWIW, https://gist.github.com/andy-shev/afe4c0e009cb3008ac613d8384aaa6eb
> 
> The code in your gist produces:
> 
> Initial event: 0x92, new state: True for bit 0x20

Initial event is 10010010b, we assume that we got in the code when required
state is to 'set' (True) with axis = 5 (means 00100000b when powered).

The [-] are special cases just to show the algo.

> [-] 0x00 | 0x20 --> 1: handle

If initial event is 0 we handle

If _after_ that the bit 5 set (which is NOT the case in _this_ iteration),
we will stop handling.

> [0] 0x92 | 0x20 --> 1: handle

So, it's again step 1. I _assumed_ that your code works and sets the bit. Check
the 0 and bit events (two other groups), they have exactly one handle
(excluding special [-] cases).


> [1] 0x93 | 0x20 --> 1: handle
> [2] 0x93 | 0x20 --> 1: handle
> [3] 0x97 | 0x20 --> 1: handle
> [4] 0x9f | 0x20 --> 1: handle
> [5] 0x9f | 0x20 --> 1: handle
> [6] 0xbf | 0x20 --> 0: return
> [7] 0xff | 0x20 --> 0: return
> [-] 0xff | 0x20 --> 0: return
> 
> But this is not what I need. I need "handle" to be there only when the
> bitmask goes from 0x00 to non-zero (in the above example, only at the first
> [-] iteration); all the other cases should be a "return". Likewise, if
> there is '&' instead of '|', I need "handle" to be there only when the
> bitmask goes from non-zero to 0x00.

Probably we are reading results differently. I put jut several iterations to
show different _inputs_, not outputs. Please, check again.

P.S. I know, bits are very hard...


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 8/9] iio: imu: st_lsm6dsx: add event configurability on a per axis basis

[Francesco Lavra]

[-- Attachment #1: Type: text/plain, Size: 8162 bytes --]

On Fri, 2025-11-21 at 11:31 +0200, Andy Shevchenko wrote:
> On Fri, Nov 21, 2025 at 10:14:06AM +0100, Francesco Lavra wrote:
> > On Thu, 2025-11-20 at 20:31 +0200, Andy Shevchenko wrote:
> > > On Thu, Nov 20, 2025 at 03:59:18PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Nov 20, 2025 at 12:43:09PM +0100, Francesco Lavra wrote:
> > > > > On Thu, 2025-11-20 at 10:05 +0100, Andy Shevchenko wrote:
> > > > > > On Tue, Nov 18, 2025 at 12:01:57PM +0100, Francesco Lavra
> > > > > > wrote:
> > > > > > > On Tue, 2025-11-18 at 11:44 +0100, Andy Shevchenko wrote:
> > > > > > > > On Mon, Nov 17, 2025 at 08:23:35PM +0100, Francesco Lavra
> > > > > > > > wrote:
> > > > > > > > > On Thu, 2025-10-30 at 15:56 +0200, Andy Shevchenko wrote:
> > > > > > > > > > On Thu, Oct 30, 2025 at 12:23:19PM +0100, Francesco
> > > > > > > > > > Lavra
> > > > > > > > > > wrote:
> > > > > > > > > > > On Thu, 2025-10-30 at 10:24 +0200, Andy Shevchenko
> > > > > > > > > > > wrote:
> > > > > > > > > > > > On Thu, Oct 30, 2025 at 08:27:51AM +0100, Francesco
> > > > > > > > > > > > Lavra
> > > > > > > > > > > > wrote:
> 
> ...
> 
> > > > > > > > > > > > > +       old_enable = hw->enable_event[event];
> > > > > > > > > > > > > +       new_enable = state ? (old_enable |
> > > > > > > > > > > > > BIT(axis))
> > > > > > > > > > > > > :
> > > > > > > > > > > > > (old_enable
> > > > > > > > > > > > > &
> > > > > > > > > > > > > ~BIT(axis));
> > > > > > > > > > > > > +       if (!!old_enable == !!new_enable)
> > > > > > > > > > > > 
> > > > > > > > > > > > This is an interesting check. So, old_enable and
> > > > > > > > > > > > new_enable
> > > > > > > > > > > > are
> > > > > > > > > > > > _not_
> > > > > > > > > > > > booleans, right?
> > > > > > > > > > > > So, this means the check test if _any_ of the bit
> > > > > > > > > > > > was
> > > > > > > > > > > > set and
> > > > > > > > > > > > kept
> > > > > > > > > > > > set or
> > > > > > > > > > > > none were set
> > > > > > > > > > > > and non is going to be set. Correct? I think a
> > > > > > > > > > > > short
> > > > > > > > > > > > comment
> > > > > > > > > > > > would be
> > > > > > > > > > > > good to have.
> > > > > > > > > > > 
> > > > > > > > > > > old_enable and new_enable are bit masks, but we are
> > > > > > > > > > > only
> > > > > > > > > > > interested
> > > > > > > > > > > in
> > > > > > > > > > > whether any bit is set, to catch the cases where the
> > > > > > > > > > > bit
> > > > > > > > > > > mask
> > > > > > > > > > > goes
> > > > > > > > > > > from
> > > > > > > > > > > zero to non-zero and vice versa. Will add a comment.
> > > > > > > > > > 
> > > > > > > > > > If it's a true bitmask (assuming unsigned long type)
> > > > > > > > > > then
> > > > > > > > > > all
> > > > > > > > > > this
> > > > > > > > > > can be
> > > > > > > > > > done
> > > > > > > > > > via bitmap API calls. Otherwise you can also compare a
> > > > > > > > > > Hamming
> > > > > > > > > > weights of
> > > > > > > > > > them
> > > > > > > > > > (probably that gives even the same size of the object
> > > > > > > > > > file,
> > > > > > > > > > but
> > > > > > > > > > !!
> > > > > > > > > > instructions
> > > > > > > > > >  will be changed to hweight() calls (still a single
> > > > > > > > > > assembly
> > > > > > > > > > instr on
> > > > > > > > > > modern
> > > > > > > > > >  architectures).
> > > > > > > > > 
> > > > > > > > > These are u8 variables, so we can't use the bitmap API.
> > > > > > > > 
> > > > > > > > OK. But hweight8() can still be used.
> > > > > > > > 
> > > > > > > > > And I don't
> > > > > > > > > understand the reason for using hweight(), given that the
> > > > > > > > > !!
> > > > > > > > > operators
> > > > > > > > > would still be needed.
> > > > > > > > 
> > > > > > > > No, you won't need !! with that.
> > > > > > > 
> > > > > > > I still don't understand. Are you proposing to replace `if
> > > > > > > (!!old_enable ==
> > > > > > > !!new_enable)` with `if (hweight8(old_enable) ==
> > > > > > > hweight8(new_enable))`?
> > > > > > > That won't work, because we only need to check whether the
> > > > > > > Hamming
> > > > > > > weight
> > > > > > > goes from zero to non-zero and vice versa.
> > > > > > 
> > > > > >        old_enable = hw->enable_event[event];
> > > > > >        new_enable = state ? (old_enable | BIT(axis)) :
> > > > > >                             (old_enable & ~BIT(axis));
> > > > > >        if (!!old_enable == !!new_enable)
> > > > > >                return 0;
> > > > > > 
> > > > > > If I am not mistaken this will do exactly the same in a simpler
> > > > > > way.
> > > > > > 
> > > > > >         old_enable = hw->enable_event[event];
> > > > > >         if (state)
> > > > > >                 new_enable = old_enable | BIT(axis);
> > > > > >         else
> > > > > >                 new_enable = old_enable & ~BIT(axis);
> > > > > >         if ((new_enable ^ old_enable) != BIT(axis))
> > > > > >                 return 0;
> > > > > 
> > > > > This doesn't look right to me, if new_enable differs from
> > > > > old_enable
> > > > > by
> > > > > just one bit (which it does), then the XOR result will always
> > > > > have
> > > > > this bit
> > > > > (and no others) set, so (new_enable ^ old_enable) will always
> > > > > equal
> > > > > BIT(axis).
> > > > > We are not checking if the bit was already set or clear, when
> > > > > this
> > > > > code
> > > > > runs we already know that the bit is changing, what we are
> > > > > checking
> > > > > is
> > > > > whether all bits are zero before or after this change.
> > > > 
> > > > The check I proposed is to have a look for the cases when
> > > > old_enable
> > > > was 0 and
> > > > the BIT(axis) is set and when the BIT(axis) was _the last_ bit in
> > > > the
> > > > mask that
> > > > got reset. If it's not the case, the code will return 0. I think my
> > > > check is
> > > > correct.
> > > > 
> > > > Should I write a test case?
> > > 
> > > FWIW,
> > > https://gist.github.com/andy-shev/afe4c0e009cb3008ac613d8384aaa6eb
> > 
> > The code in your gist produces:
> > 
> > Initial event: 0x92, new state: True for bit 0x20
> 
> Initial event is 10010010b, we assume that we got in the code when
> required
> state is to 'set' (True) with axis = 5 (means 00100000b when powered).
> 
> The [-] are special cases just to show the algo.
> 
> > [-] 0x00 | 0x20 --> 1: handle
> 
> If initial event is 0 we handle
> 
> If _after_ that the bit 5 set (which is NOT the case in _this_
> iteration),
> we will stop handling.

We have to stop handling not only if bit 5 is set, but also if any other
bit is set after that.

> > [0] 0x92 | 0x20 --> 1: handle
> 
> So, it's again step 1. I _assumed_ that your code works and sets the bit.

Even if it's again step 1, this is not supposed to be handled, because
there are bits already set in the current bitmask.

Enabling an event source for an axis may need two registers to be set:
1) an axis-specific enable register
2) an event-specific enable register (global for all axes)

If no events are enabled on any axis, when we enable the event source for
axis X, we have to do both steps above; if then we enable the same event
source for axis Y, we have to do just step 1 but not step 2; and that's
what the (!!old_enable == !!new_enable) check is supposed to do: to check
if, after setting the axis-specific enable register, we have to set also
the event-specific enable register. If I replace that check with
((new_enable ^ old_enable) != BIT(axis)), then I'm doing both steps for
every axis, which is unnecessary when enabling the event (because we don't
need to set again the event-specific register after we set it for the first
axis), and is wrong when disabling the event (because disabling the event
for a single axis would inadvertently disable it globally (i.e. for all
axes).


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 8/9] iio: imu: st_lsm6dsx: add event configurability on a per axis basis

[Jonathan Cameron]

On Fri, 21 Nov 2025 15:57:57 +0100
Francesco Lavra <flavra@baylibre.com> wrote:

> On Fri, 2025-11-21 at 11:31 +0200, Andy Shevchenko wrote:
> > On Fri, Nov 21, 2025 at 10:14:06AM +0100, Francesco Lavra wrote:  
> > > On Thu, 2025-11-20 at 20:31 +0200, Andy Shevchenko wrote:  
> > > > On Thu, Nov 20, 2025 at 03:59:18PM +0200, Andy Shevchenko wrote:  
> > > > > On Thu, Nov 20, 2025 at 12:43:09PM +0100, Francesco Lavra wrote:  
> > > > > > On Thu, 2025-11-20 at 10:05 +0100, Andy Shevchenko wrote:  
> > > > > > > On Tue, Nov 18, 2025 at 12:01:57PM +0100, Francesco Lavra
> > > > > > > wrote:  
> > > > > > > > On Tue, 2025-11-18 at 11:44 +0100, Andy Shevchenko wrote:  
> > > > > > > > > On Mon, Nov 17, 2025 at 08:23:35PM +0100, Francesco Lavra
> > > > > > > > > wrote:  
> > > > > > > > > > On Thu, 2025-10-30 at 15:56 +0200, Andy Shevchenko wrote:  
> > > > > > > > > > > On Thu, Oct 30, 2025 at 12:23:19PM +0100, Francesco
> > > > > > > > > > > Lavra
> > > > > > > > > > > wrote:  
> > > > > > > > > > > > On Thu, 2025-10-30 at 10:24 +0200, Andy Shevchenko
> > > > > > > > > > > > wrote:  
> > > > > > > > > > > > > On Thu, Oct 30, 2025 at 08:27:51AM +0100, Francesco
> > > > > > > > > > > > > Lavra
> > > > > > > > > > > > > wrote:  
> > 
> > ...
> >   
> > > > > > > > > > > > > > +       old_enable = hw->enable_event[event];
> > > > > > > > > > > > > > +       new_enable = state ? (old_enable |
> > > > > > > > > > > > > > BIT(axis))
> > > > > > > > > > > > > > :
> > > > > > > > > > > > > > (old_enable
> > > > > > > > > > > > > > &
> > > > > > > > > > > > > > ~BIT(axis));
> > > > > > > > > > > > > > +       if (!!old_enable == !!new_enable)  
> > > > > > > > > > > > > 
> > > > > > > > > > > > > This is an interesting check. So, old_enable and
> > > > > > > > > > > > > new_enable
> > > > > > > > > > > > > are
> > > > > > > > > > > > > _not_
> > > > > > > > > > > > > booleans, right?
> > > > > > > > > > > > > So, this means the check test if _any_ of the bit
> > > > > > > > > > > > > was
> > > > > > > > > > > > > set and
> > > > > > > > > > > > > kept
> > > > > > > > > > > > > set or
> > > > > > > > > > > > > none were set
> > > > > > > > > > > > > and non is going to be set. Correct? I think a
> > > > > > > > > > > > > short
> > > > > > > > > > > > > comment
> > > > > > > > > > > > > would be
> > > > > > > > > > > > > good to have.  
> > > > > > > > > > > > 
> > > > > > > > > > > > old_enable and new_enable are bit masks, but we are
> > > > > > > > > > > > only
> > > > > > > > > > > > interested
> > > > > > > > > > > > in
> > > > > > > > > > > > whether any bit is set, to catch the cases where the
> > > > > > > > > > > > bit
> > > > > > > > > > > > mask
> > > > > > > > > > > > goes
> > > > > > > > > > > > from
> > > > > > > > > > > > zero to non-zero and vice versa. Will add a comment.  
> > > > > > > > > > > 
> > > > > > > > > > > If it's a true bitmask (assuming unsigned long type)
> > > > > > > > > > > then
> > > > > > > > > > > all
> > > > > > > > > > > this
> > > > > > > > > > > can be
> > > > > > > > > > > done
> > > > > > > > > > > via bitmap API calls. Otherwise you can also compare a
> > > > > > > > > > > Hamming
> > > > > > > > > > > weights of
> > > > > > > > > > > them
> > > > > > > > > > > (probably that gives even the same size of the object
> > > > > > > > > > > file,
> > > > > > > > > > > but
> > > > > > > > > > > !!
> > > > > > > > > > > instructions
> > > > > > > > > > >  will be changed to hweight() calls (still a single
> > > > > > > > > > > assembly
> > > > > > > > > > > instr on
> > > > > > > > > > > modern
> > > > > > > > > > >  architectures).  
> > > > > > > > > > 
> > > > > > > > > > These are u8 variables, so we can't use the bitmap API.  
> > > > > > > > > 
> > > > > > > > > OK. But hweight8() can still be used.
> > > > > > > > >   
> > > > > > > > > > And I don't
> > > > > > > > > > understand the reason for using hweight(), given that the
> > > > > > > > > > !!
> > > > > > > > > > operators
> > > > > > > > > > would still be needed.  
> > > > > > > > > 
> > > > > > > > > No, you won't need !! with that.  
> > > > > > > > 
> > > > > > > > I still don't understand. Are you proposing to replace `if
> > > > > > > > (!!old_enable ==
> > > > > > > > !!new_enable)` with `if (hweight8(old_enable) ==
> > > > > > > > hweight8(new_enable))`?
> > > > > > > > That won't work, because we only need to check whether the
> > > > > > > > Hamming
> > > > > > > > weight
> > > > > > > > goes from zero to non-zero and vice versa.  
> > > > > > > 
> > > > > > >        old_enable = hw->enable_event[event];
> > > > > > >        new_enable = state ? (old_enable | BIT(axis)) :
> > > > > > >                             (old_enable & ~BIT(axis));
> > > > > > >        if (!!old_enable == !!new_enable)
> > > > > > >                return 0;
> > > > > > > 
> > > > > > > If I am not mistaken this will do exactly the same in a simpler
> > > > > > > way.
> > > > > > > 
> > > > > > >         old_enable = hw->enable_event[event];
> > > > > > >         if (state)
> > > > > > >                 new_enable = old_enable | BIT(axis);
> > > > > > >         else
> > > > > > >                 new_enable = old_enable & ~BIT(axis);
> > > > > > >         if ((new_enable ^ old_enable) != BIT(axis))
> > > > > > >                 return 0;  
> > > > > > 
> > > > > > This doesn't look right to me, if new_enable differs from
> > > > > > old_enable
> > > > > > by
> > > > > > just one bit (which it does), then the XOR result will always
> > > > > > have
> > > > > > this bit
> > > > > > (and no others) set, so (new_enable ^ old_enable) will always
> > > > > > equal
> > > > > > BIT(axis).
> > > > > > We are not checking if the bit was already set or clear, when
> > > > > > this
> > > > > > code
> > > > > > runs we already know that the bit is changing, what we are
> > > > > > checking
> > > > > > is
> > > > > > whether all bits are zero before or after this change.  
> > > > > 
> > > > > The check I proposed is to have a look for the cases when
> > > > > old_enable
> > > > > was 0 and
> > > > > the BIT(axis) is set and when the BIT(axis) was _the last_ bit in
> > > > > the
> > > > > mask that
> > > > > got reset. If it's not the case, the code will return 0. I think my
> > > > > check is
> > > > > correct.
> > > > > 
> > > > > Should I write a test case?  
> > > > 
> > > > FWIW,
> > > > https://gist.github.com/andy-shev/afe4c0e009cb3008ac613d8384aaa6eb  
> > > 
> > > The code in your gist produces:
> > > 
> > > Initial event: 0x92, new state: True for bit 0x20  
> > 
> > Initial event is 10010010b, we assume that we got in the code when
> > required
> > state is to 'set' (True) with axis = 5 (means 00100000b when powered).
> > 
> > The [-] are special cases just to show the algo.
> >   
> > > [-] 0x00 | 0x20 --> 1: handle  
> > 
> > If initial event is 0 we handle
> > 
> > If _after_ that the bit 5 set (which is NOT the case in _this_
> > iteration),
> > we will stop handling.  
> 
> We have to stop handling not only if bit 5 is set, but also if any other
> bit is set after that.
> 
> > > [0] 0x92 | 0x20 --> 1: handle  
> > 
> > So, it's again step 1. I _assumed_ that your code works and sets the bit.  
> 
> Even if it's again step 1, this is not supposed to be handled, because
> there are bits already set in the current bitmask.
> 
> Enabling an event source for an axis may need two registers to be set:
> 1) an axis-specific enable register
> 2) an event-specific enable register (global for all axes)
> 
> If no events are enabled on any axis, when we enable the event source for
> axis X, we have to do both steps above; if then we enable the same event
> source for axis Y, we have to do just step 1 but not step 2; and that's
> what the (!!old_enable == !!new_enable) check is supposed to do: to check
> if, after setting the axis-specific enable register, we have to set also
> the event-specific enable register. If I replace that check with
> ((new_enable ^ old_enable) != BIT(axis)), then I'm doing both steps for
> every axis, which is unnecessary when enabling the event (because we don't
> need to set again the event-specific register after we set it for the first
> axis), and is wrong when disabling the event (because disabling the event
> for a single axis would inadvertently disable it globally (i.e. for all
> axes).
> 
Obviously I've very late to this thread, but just thought I'd comment
that if the driver was using regcache (which might well make sense) then
I wouldn't bother with the check at all. The write to update the register
to the value already has wouldn't do anything other than a few trivial
operations to check the cache value matches.

Might be worth enabling the regcache part of regmap simply to avoid
having to care about corners like this.

Jonathan

[PATCH 9/9] iio: imu: st_lsm6dsx: add tap event detection

[Francesco Lavra]

Add the logic to advertise tap event capability and report tap
events; define a tap event source for the LSM6DSV chip family.
Tested on LSMDSV16X.

Signed-off-by: Francesco Lavra <flavra@baylibre.com>
---
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  1 +
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 35 ++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
index 62edd177c87c..75953a78fc04 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
@@ -223,6 +223,7 @@ struct st_lsm6dsx_shub_settings {
 
 enum st_lsm6dsx_event_id {
 	ST_LSM6DSX_EVENT_WAKEUP,
+	ST_LSM6DSX_EVENT_TAP,
 	ST_LSM6DSX_EVENT_MAX
 };
 
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index 6d1b7b2a371a..1bc69c6c1b9d 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -1349,6 +1349,30 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 					.status_y_mask = BIT(1),
 					.status_x_mask = BIT(2),
 				},
+				[ST_LSM6DSX_EVENT_TAP] = {
+					.x_value = {
+						.addr = 0x57,
+						.mask = GENMASK(4, 0),
+					},
+					.y_value = {
+						.addr = 0x58,
+						.mask = GENMASK(4, 0),
+					},
+					.z_value = {
+						.addr = 0x59,
+						.mask = GENMASK(4, 0),
+					},
+					.enable_mask = BIT(6),
+					.enable_axis_reg = 0x56,
+					.enable_x_mask = BIT(3),
+					.enable_y_mask = BIT(2),
+					.enable_z_mask = BIT(1),
+					.status_reg = 0x46,
+					.status_mask = BIT(5),
+					.status_x_mask = BIT(2),
+					.status_y_mask = BIT(1),
+					.status_z_mask = BIT(0),
+				},
 			},
 		},
 	},
@@ -1846,6 +1870,8 @@ static enum st_lsm6dsx_event_id st_lsm6dsx_get_event_id(enum iio_event_type type
 	switch (type) {
 	case IIO_EV_TYPE_THRESH:
 		return ST_LSM6DSX_EVENT_WAKEUP;
+	case IIO_EV_TYPE_GESTURE:
+		return ST_LSM6DSX_EVENT_TAP;
 	default:
 		return ST_LSM6DSX_EVENT_MAX;
 	}
@@ -2427,6 +2453,13 @@ static int st_lsm6dsx_chan_init(struct iio_chan_spec *channels, struct st_lsm6ds
 				event_spec->dir = IIO_EV_DIR_EITHER;
 				event_spec->mask_separate = BIT(IIO_EV_INFO_VALUE) |
 							    BIT(IIO_EV_INFO_ENABLE);
+				event_spec++;
+			}
+			if (event_sources & BIT(ST_LSM6DSX_EVENT_TAP)) {
+				event_spec->type = IIO_EV_TYPE_GESTURE;
+				event_spec->dir = IIO_EV_DIR_SINGLETAP;
+				event_spec->mask_separate = BIT(IIO_EV_INFO_VALUE) |
+							    BIT(IIO_EV_INFO_ENABLE);
 			}
 		}
 	}
@@ -2553,6 +2586,8 @@ st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw)
 
 	events_found = st_lsm6dsx_report_events(hw, ST_LSM6DSX_EVENT_WAKEUP, IIO_EV_TYPE_THRESH,
 						IIO_EV_DIR_EITHER);
+	events_found |= st_lsm6dsx_report_events(hw, ST_LSM6DSX_EVENT_TAP, IIO_EV_TYPE_GESTURE,
+						 IIO_EV_DIR_SINGLETAP);
 
 	return events_found;
 }
-- 
2.39.5