Re: [PATCH v2 1/2] iio: accel: fxls8962af: add threshold event handling

[Sean Nyekjaer <sean@geanix.com>]

On Tue, Aug 24, 2021 at 03:42:58PM +0300, Andy Shevchenko wrote:
> On Tue, Aug 24, 2021 at 3:32 PM Sean Nyekjaer <sean@geanix.com> wrote:
> > On Tue, Aug 24, 2021 at 03:15:28PM +0300, Andy Shevchenko wrote:
> > > On Tue, Aug 24, 2021 at 2:38 PM Sean Nyekjaer <sean@geanix.com> wrote:
> 
> ...
> 
> > > > +       /*
> > > > +        * Add the same value to the lower-threshold register with a reversed sign
> > > > +        * in 2-complement 12 bit format.
> > > > +        */
> > > > +       data->lower_thres = (~val & GENMASK(11, 0)) + 1;
> > >
> > > This looks suspicious.
> > >
> > > 0 => 0xfff + 1 => 0x1000. Is it what is wanted?
> > > I thought that -val & mask is what you need.
> > >
> > > Can you explain more in the comment (maybe with examples) on what is
> > > coming and what is expected?
> >
> > It's a bit messy I know :)
> >
> > Some examples:
> > val = 0 => upper = 0x0, lower = 0x0
> > val = 500 => upper = 0x1F4, lower = 0xe0c
> > val = 1000 => upper = 0x3e8, lower = 0xc18
> >
> > Guess it could work if we special case val = 0...
> >
> > It doesn't even makes sense to write 0 to this register as noise would
> > trigger events.
> >
> > > > +       data->upper_thres = val & GENMASK(10, 0);
> 
> So, I just tested all three and with '-' (minus) it works, while your
> code is buggy :-)

Agree, just tested with:

---
#include <stdio.h>

int main() {
	signed int lower_thres, upper_thres, lower, upper;

	int val;

	for (val = 0; val <= 1000; val+=500) {
		lower = -val & 0xFFF;
		upper = val & 0x7FF;

		printf("val %d, upper 0x%x, lower 0x%x\n", val, upper, lower);
	}
}
---

val 0, upper 0x0, lower 0x0
val 500, upper 0x1f4, lower 0xe0c
val 1000, upper 0x3e8, lower 0xc18

Thanks :)

/Sean