Re: [bug report] Input: elants_i2c - add support for eKTF3624

["Michał Mirosław" <mirq-linux@rere.qmqm.pl>]

On Thu, Jan 28, 2021 at 05:37:26PM +0300, Dan Carpenter wrote:
> On Thu, Jan 28, 2021 at 02:07:05PM +0100, Michał Mirosław wrote:
> > On Thu, Jan 28, 2021 at 12:57:12PM +0300, Dan Carpenter wrote:
> > > Hello Michał Mirosław,
> > > 
> > > The patch 9517b95bdc46: "Input: elants_i2c - add support for
> > > eKTF3624" from Jan 24, 2021, leads to the following static checker
> > > warning:
> > > 
> > > 	drivers/input/touchscreen/elants_i2c.c:966 elants_i2c_mt_event()
> > > 	warn: should this be a bitwise negate mask?
> > > 
> > > drivers/input/touchscreen/elants_i2c.c
> > [...]
> > >    963                                  w = buf[FW_POS_WIDTH + i / 2];
> > >    964                                  w >>= 4 * (~i & 1);
> > >    965                                  w |= w << 4;
> > >    966                                  w |= !w;
> > >                                         ^^^^^^^^
> > > 
> > > This code is just very puzzling.  I think it may actually be correct?
> > > The boring and conventional way to write this would be to do it like so:
> > > 
> > > 	if (!w)
> > > 		w = 1;
> > 
> > It could also be written as:
> > 
> > 	w += !w;
> > 
> > or:
> > 	w += w == 0;`
> > 
> > while avoiding conditional.
> 
> Is there some kind of prize for avoiding if statements??

Less LOC and an occassional puzzle, of course. :-) Considering your
examples below I can see where the check came from. I wouldn't object
to a patch changing the code or adding a comment on what it does (it
maps a selected nibble value (0..15) to a byte (1..255) spreading the
values evenly).

> > But, in this case, the warning is bogus. Because w | ~w == all-ones (always),
> > it might as well suggested to write:
> > 
> > 	w = -1;
> > 
> > or:
> > 	w = ~0;
> > 
> > making the code broken.
> 
> Yeah.  The rule is just a simple heuristic of a logical negate used
> with a bitwise operation.
[...]

Maybe it could differentiate between "|= !x" and "&= !x", the second one
being more suspicious? I must say that 'x | !x' idiom looks obvious
as other sigle-operator-letter misspellings ('x | ~x' and 'x || !x')
beg the question of why the constant wasn't used directly?

Best Regards
Michał Mirosław