Message-ID: <0da1f46640226ab52ef0a726a5110cc56b85cdd4.camel@baylibre.com>
Date: Fri, 21 Nov 2025 15:57:57 +0100
From: Francesco Lavra <flavra@...libre.com>
To: Andy Shevchenko <andriy.shevchenko@...el.com>
Cc: Lorenzo Bianconi <lorenzo@...nel.org>, Jonathan Cameron
 <jic23@...nel.org>,  David Lechner <dlechner@...libre.com>, Nuno
 Sá <nuno.sa@...log.com>,  linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 8/9] iio: imu: st_lsm6dsx: add event configurability on
 a per axis basis

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).


Download attachment "signature.asc" of type "application/pgp-signature" (660 bytes)

Powered by blists - more mailing lists