re: fbdev: ssd1307fb: Unify init code and obtain hw specific bits from DT

re: fbdev: ssd1307fb: Unify init code and obtain hw specific bits from DT

[Dan Carpenter]

Hello Thomas Niederprüm,

The patch a3998fe03e87: "fbdev: ssd1307fb: Unify init code and obtain
hw specific bits from DT" from Mar 31, 2015, leads to the following
static checker warning:

	drivers/video/fbdev/ssd1307fb.c:371 ssd1307fb_init()
	warn: add some parenthesis here?

drivers/video/fbdev/ssd1307fb.c
   366          /* Set COM pins configuration */
   367          ret = ssd1307fb_write_cmd(par->client, SSD1307FB_SET_COM_PINS_CONFIG);
   368          if (ret < 0)
   369                  return ret;
   370  
   371          compins = 0x02 | (!par->com_seq & 0x1) << 4
   372                                     | (par->com_lrremap & 0x1) << 5;

Smatch is complaining because it's normally  "!par->com_seq & 0x1" is
a bug and "!(par->com_seq & 0x1)" is intended.  I don't know what was
intended here though.  If the current code is correct, you can silence
the static checker warning by writing it as "(!par->com_seq) & 0x1".

But I also have a hard time remembering if | or << is higher precedence
so that might be clearer with parenthesis as well even though the code
is clearly correct when I google for "order of operations" in C.

   373          ret = ssd1307fb_write_cmd(par->client, compins);
   374          if (ret < 0)
   375                  return ret;

regards,
dan carpenter

Re: fbdev: ssd1307fb: Unify init code and obtain hw specific bits from DT

[Sudip Mukherjee]

On Sat, May 23, 2015 at 08:32:45PM +0300, Dan Carpenter wrote:
> Hello Thomas Niederprüm,

> But I also have a hard time remembering if | or << is higher precedence

man page of operator (man operator) will show the precedence.
<< is higher precendence than |

regards
sudip

Re: fbdev: ssd1307fb: Unify init code and obtain hw specific bits from DT

[Thomas Niederprüm]

Am Sat, 23 May 2015 20:32:45 +0300
schrieb Dan Carpenter <dan.carpenter@oracle.com>:

> Hello Thomas Niederprüm,
> 
> The patch a3998fe03e87: "fbdev: ssd1307fb: Unify init code and obtain
> hw specific bits from DT" from Mar 31, 2015, leads to the following
> static checker warning:
> 
> 	drivers/video/fbdev/ssd1307fb.c:371 ssd1307fb_init()
> 	warn: add some parenthesis here?
> 
> drivers/video/fbdev/ssd1307fb.c
>    366          /* Set COM pins configuration */
>    367          ret = ssd1307fb_write_cmd(par->client,
> SSD1307FB_SET_COM_PINS_CONFIG); 368          if (ret < 0)
>    369                  return ret;
>    370  
>    371          compins = 0x02 | (!par->com_seq & 0x1) << 4
>    372                                     | (par->com_lrremap & 0x1)
> << 5;
> 
> Smatch is complaining because it's normally  "!par->com_seq & 0x1" is
> a bug and "!(par->com_seq & 0x1)" is intended.  I don't know what was
> intended here though.  If the current code is correct, you can silence
> the static checker warning by writing it as "(!par->com_seq) & 0x1".

Indeed "!(par->com_seq & 0x1)" is what I intended. Thanks for spotting
this. 
What is the best way to handle this now? Will you send a fixup
patch as for the backlight code or will this be my task?

> 
> But I also have a hard time remembering if | or << is higher
> precedence so that might be clearer with parenthesis as well even
> though the code is clearly correct when I google for "order of
> operations" in C.
> 
>    373          ret = ssd1307fb_write_cmd(par->client, compins);
>    374          if (ret < 0)
>    375                  return ret;
> 

Thanks!

Thomas

Re: fbdev: ssd1307fb: Unify init code and obtain hw specific bits from DT

[Dan Carpenter]

On Mon, May 25, 2015 at 10:30:38AM +0530, Sudip Mukherjee wrote:
> On Sat, May 23, 2015 at 08:32:45PM +0300, Dan Carpenter wrote:
> > Hello Thomas Niederprüm,
> 
> > But I also have a hard time remembering if | or << is higher precedence
> 
> man page of operator (man operator) will show the precedence.
> << is higher precendence than |

The point is that people should just use parenthesis when it's not
something that's obvious.

Also the more I think about it, writing "!par->com_seq & 0x1" is
nonsense.  If that's really what was intended then just say
"!par->com_seq".  I think it's a bug though and "!(par->com_seq & 0x1)"
is intended.

regards,
dan carpenter

Re: fbdev: ssd1307fb: Unify init code and obtain hw specific bits from DT

[Dan Carpenter]

On Mon, May 25, 2015 at 09:34:32AM +0200, Thomas Niederprüm wrote:
> Am Sat, 23 May 2015 20:32:45 +0300
> schrieb Dan Carpenter <dan.carpenter@oracle.com>:
> 
> > Hello Thomas Niederprüm,
> > 
> > The patch a3998fe03e87: "fbdev: ssd1307fb: Unify init code and obtain
> > hw specific bits from DT" from Mar 31, 2015, leads to the following
> > static checker warning:
> > 
> > 	drivers/video/fbdev/ssd1307fb.c:371 ssd1307fb_init()
> > 	warn: add some parenthesis here?
> > 
> > drivers/video/fbdev/ssd1307fb.c
> >    366          /* Set COM pins configuration */
> >    367          ret = ssd1307fb_write_cmd(par->client,
> > SSD1307FB_SET_COM_PINS_CONFIG); 368          if (ret < 0)
> >    369                  return ret;
> >    370  
> >    371          compins = 0x02 | (!par->com_seq & 0x1) << 4
> >    372                                     | (par->com_lrremap & 0x1)
> > << 5;
> > 
> > Smatch is complaining because it's normally  "!par->com_seq & 0x1" is
> > a bug and "!(par->com_seq & 0x1)" is intended.  I don't know what was
> > intended here though.  If the current code is correct, you can silence
> > the static checker warning by writing it as "(!par->com_seq) & 0x1".
> 
> Indeed "!(par->com_seq & 0x1)" is what I intended. Thanks for spotting
> this. 
> What is the best way to handle this now? Will you send a fixup
> patch as for the backlight code or will this be my task?

Could you send a fix and give me a:

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

cookie?

regards,
dan carpenter