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

[Francesco Lavra <flavra@baylibre.com>]

[-- 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 --]