* re: fbdev: ssd1307fb: Unify init code and obtain hw specific bits from DT
@ 2015-05-23 17:32 Dan Carpenter
2015-05-25 5:12 ` Sudip Mukherjee
` (3 more replies)
0 siblings, 4 replies; 5+ messages in thread
From: Dan Carpenter @ 2015-05-23 17:32 UTC (permalink / raw
To: linux-fbdev
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: fbdev: ssd1307fb: Unify init code and obtain hw specific bits from DT
2015-05-23 17:32 fbdev: ssd1307fb: Unify init code and obtain hw specific bits from DT Dan Carpenter
@ 2015-05-25 5:12 ` Sudip Mukherjee
2015-05-25 7:34 ` Thomas Niederprüm
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Sudip Mukherjee @ 2015-05-25 5:12 UTC (permalink / raw
To: linux-fbdev
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: fbdev: ssd1307fb: Unify init code and obtain hw specific bits from DT
2015-05-23 17:32 fbdev: ssd1307fb: Unify init code and obtain hw specific bits from DT Dan Carpenter
2015-05-25 5:12 ` Sudip Mukherjee
@ 2015-05-25 7:34 ` Thomas Niederprüm
2015-05-25 9:48 ` Dan Carpenter
2015-05-25 10:40 ` Dan Carpenter
3 siblings, 0 replies; 5+ messages in thread
From: Thomas Niederprüm @ 2015-05-25 7:34 UTC (permalink / raw
To: linux-fbdev
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: fbdev: ssd1307fb: Unify init code and obtain hw specific bits from DT
2015-05-23 17:32 fbdev: ssd1307fb: Unify init code and obtain hw specific bits from DT Dan Carpenter
2015-05-25 5:12 ` Sudip Mukherjee
2015-05-25 7:34 ` Thomas Niederprüm
@ 2015-05-25 9:48 ` Dan Carpenter
2015-05-25 10:40 ` Dan Carpenter
3 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2015-05-25 9:48 UTC (permalink / raw
To: linux-fbdev
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: fbdev: ssd1307fb: Unify init code and obtain hw specific bits from DT
2015-05-23 17:32 fbdev: ssd1307fb: Unify init code and obtain hw specific bits from DT Dan Carpenter
` (2 preceding siblings ...)
2015-05-25 9:48 ` Dan Carpenter
@ 2015-05-25 10:40 ` Dan Carpenter
3 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2015-05-25 10:40 UTC (permalink / raw
To: linux-fbdev
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-05-25 10:40 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-05-23 17:32 fbdev: ssd1307fb: Unify init code and obtain hw specific bits from DT Dan Carpenter
2015-05-25 5:12 ` Sudip Mukherjee
2015-05-25 7:34 ` Thomas Niederprüm
2015-05-25 9:48 ` Dan Carpenter
2015-05-25 10:40 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.