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

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

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