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.5On 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
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.
>
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
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.
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
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.
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
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.
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
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
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.
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
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).
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
© 2016 - 2026 Red Hat, Inc.