From: Lorenzo Bianconi <lorenzo@kernel.org>
To: Francesco Lavra <flavra@baylibre.com>
Cc: "Jonathan Cameron" <jic23@kernel.org>,
"David Lechner" <dlechner@baylibre.com>,
"Nuno Sá" <nuno.sa@analog.com>,
"Andy Shevchenko" <andy@kernel.org>,
linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 6/9] iio: imu: st_lsm6dsx: make event management functions generic
Date: Fri, 21 Nov 2025 09:53:10 +0100 [thread overview]
Message-ID: <aSAodnHzW6fvm9yb@lore-desk> (raw)
In-Reply-To: <20251120082615.3263892-7-flavra@baylibre.com>
[-- Attachment #1: Type: text/plain, Size: 12113 bytes --]
> 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.
Just some nits inline. Fixing them:
Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>
>
> 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 | 149 ++++++++++++++-----
> 2 files changed, 109 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> index e727a87413e5..037b3b817e83 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> @@ -446,7 +446,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 117ecb080d8e..18a09ed6907c 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> @@ -1747,11 +1747,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,
> @@ -1864,9 +1869,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;
> u8 enable_mask;
> unsigned int data;
> @@ -1875,22 +1881,24 @@ 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 */
> - src = &hw->settings->event_settings.sources[ST_LSM6DSX_EVENT_WAKEUP];
> + /* Enable/disable event interrupt */
> + src = &hw->settings->event_settings.sources[event];
> 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,
> @@ -1898,16 +1906,17 @@ static int st_lsm6dsx_read_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;
> 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;
> @@ -1926,19 +1935,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);
> @@ -1954,13 +1964,53 @@ 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));
> +}
> +
> +/**
> + * st_lsm6dsx_check_other_events - Check for enabled sensor events.
> + * @hw: Sensor hardware instance.
> + * @curr: Current event type.
> + *
> + * Return: whether any events other than @curr are enabled.
> + */
> +static bool st_lsm6dsx_check_other_events(struct st_lsm6dsx_hw *hw,
> + enum st_lsm6dsx_event_id curr)
> +{
> + enum st_lsm6dsx_event_id other;
> +
> + for (other = 0; other < ST_LSM6DSX_EVENT_MAX; other++) {
> + if (other != curr && hw->enable_event[other])
> + return true;
> + }
new line here
> + return false;
> +}
> +
> +static int st_lsm6dsx_events_enable(struct st_lsm6dsx_sensor *sensor,
> + bool state)
> +{
> + struct st_lsm6dsx_hw *hw = sensor->hw;
> + const struct st_lsm6dsx_reg *reg;
> +
> + reg = &hw->settings->event_settings.enable_reg;
> + if (reg->addr) {
> + int err;
> +
> + err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
> + ST_LSM6DSX_SHIFT_VAL(state, reg->mask));
> + if (err)
> + return err;
> + }
new line here
> + if (state || !(hw->fifo_mask & BIT(sensor->id)))
> + return __st_lsm6dsx_sensor_set_enable(sensor, state);
new line here
> + return 0;
> }
>
> static int
> @@ -1969,22 +2019,24 @@ 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;
I think you can drop axis here and just use chan->channel2 directly.
> u8 enable_event;
> int err;
>
> - 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)
> @@ -1992,22 +2044,24 @@ 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)))
> - err = __st_lsm6dsx_sensor_set_enable(sensor, state);
> + if (enable_event)
> + err = st_lsm6dsx_events_enable(sensor, true);
> + else if (!st_lsm6dsx_check_other_events(hw, event))
> + err = st_lsm6dsx_events_enable(sensor, false);
> mutex_unlock(&hw->conf_lock);
> if (err < 0)
> return err;
>
> out:
> - hw->enable_event = enable_event;
> + hw->enable_event[event] = enable_event;
>
> return 0;
> }
> @@ -2418,18 +2472,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];
same here, can you just use hw->enable_event[id] directly?
> const struct st_lsm6dsx_event_settings *event_settings;
> const struct st_lsm6dsx_event_src *src;
> int err, data;
> s64 timestamp;
>
> - if (!hw->enable_event)
> + if (!enable_event)
> return false;
>
> event_settings = &hw->settings->event_settings;
> - src = &event_settings->sources[ST_LSM6DSX_EVENT_WAKEUP];
> + src = &event_settings->sources[id];
> err = st_lsm6dsx_read_locked(hw, src->status_reg,
> &data, sizeof(data));
> if (err < 0)
> @@ -2437,38 +2493,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 & 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 & 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 & 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 & 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
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
next prev parent reply other threads:[~2025-11-21 8:53 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-11-20 8:26 [PATCH v2 0/9] st_lsm6dsx: add tap event detection Francesco Lavra
2025-11-20 8:26 ` [PATCH v2 1/9] iio: imu: st_lsm6dsx: fix iio_chan_spec for sensors without " Francesco Lavra
2025-11-21 7:50 ` Lorenzo Bianconi
2025-11-20 8:26 ` [PATCH v2 2/9] iio: imu: st_lsm6dsx: make event_settings more generic Francesco Lavra
2025-11-21 7:54 ` Lorenzo Bianconi
2025-11-20 8:26 ` [PATCH v2 3/9] iio: imu: st_lsm6dsx: move wakeup event enable mask to event_src Francesco Lavra
2025-11-21 8:01 ` Lorenzo Bianconi
2025-11-20 8:26 ` [PATCH v2 4/9] iio: imu: st_lsm6dsx: rework code to check for enabled events Francesco Lavra
2025-11-20 8:26 ` [PATCH v2 5/9] iio: imu: st_lsm6dsx: remove event_threshold field from hw struct Francesco Lavra
2025-11-21 8:14 ` Lorenzo Bianconi
2025-11-21 8:43 ` Francesco Lavra
2025-11-21 9:34 ` Andy Shevchenko
2025-11-20 8:26 ` [PATCH v2 6/9] iio: imu: st_lsm6dsx: make event management functions generic Francesco Lavra
2025-11-21 8:53 ` Lorenzo Bianconi [this message]
2025-11-20 8:26 ` [PATCH v2 7/9] iio: imu: st_lsm6dsx: add event configurability on a per axis basis Francesco Lavra
2025-11-20 9:06 ` Andy Shevchenko
2025-11-20 8:26 ` [PATCH v2 8/9] iio: imu: st_lsm6dsx: add event spec parameter to iio_chan_spec initializer Francesco Lavra
2025-11-20 8:26 ` [PATCH v2 9/9] iio: imu: st_lsm6dsx: add tap event detection Francesco Lavra
2025-11-21 8:19 ` Lorenzo Bianconi
2025-11-20 9:09 ` [PATCH v2 0/9] " Andy Shevchenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aSAodnHzW6fvm9yb@lore-desk \
--to=lorenzo@kernel.org \
--cc=andy@kernel.org \
--cc=dlechner@baylibre.com \
--cc=flavra@baylibre.com \
--cc=jic23@kernel.org \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=nuno.sa@analog.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox