[PATCH] staging: misplaced parentheses?

[PATCH] staging: misplaced parentheses?

[Roel Kluin]

The leftmost `+' has a higher precedence than the `?' so this returns
(vres * hres * (1 << color_mode) + (VMODE && CMODE) ? 0x10 : 0x20) + 0x1000

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
The original code looks odd, but I am not entirely certain whether my patch is
correct. So please review.

diff --git a/drivers/video/platinumfb.c b/drivers/video/platinumfb.c
index 03b3670..a88f4b6 100644
--- a/drivers/video/platinumfb.c
+++ b/drivers/video/platinumfb.c
@@ -224,7 +224,7 @@ static inline int platinum_vram_reqd(int video_mode, int color_mode)
 	return vmode_attrs[video_mode-1].vres *
 	       (vmode_attrs[video_mode-1].hres * (1<<color_mode) +
 		((video_mode == VMODE_832_624_75) &&
-		 (color_mode > CMODE_8)) ? 0x10 : 0x20) + 0x1000;
+		 (color_mode > CMODE_8) ? 0x10 : 0x20)) + 0x1000;
 }
 
 #define STORE_D2(a, d) { \

------------------------------------------------------------------------------
The NEW KODAK i700 Series Scanners deliver under ANY circumstances! Your
production scanning environment may not be a perfect world - but thanks to
Kodak, there's a perfect scanner to get the job done! With the NEW KODAK i700
Series Scanner you'll get full speed at 300 dpi even with all image 
processing features enabled. http://p.sf.net/sfu/kodak-com

Re: [PATCH] staging: misplaced parentheses?

[Andrew Morton]

> Subject: [PATCH] staging: misplaced parentheses?

The title is wrong.

On Thu, 14 May 2009 14:19:26 +0200
Roel Kluin <roel.kluin@gmail.com> wrote:

> The leftmost `+' has a higher precedence than the `?' so this returns
> (vres * hres * (1 << color_mode) + (VMODE && CMODE) ? 0x10 : 0x20) + 0x1000

That seems OK.

> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> The original code looks odd, but I am not entirely certain whether my patch is
> correct. So please review.
> 
> diff --git a/drivers/video/platinumfb.c b/drivers/video/platinumfb.c
> index 03b3670..a88f4b6 100644
> --- a/drivers/video/platinumfb.c
> +++ b/drivers/video/platinumfb.c
> @@ -224,7 +224,7 @@ static inline int platinum_vram_reqd(int video_mode, int color_mode)
>  	return vmode_attrs[video_mode-1].vres *
>  	       (vmode_attrs[video_mode-1].hres * (1<<color_mode) +
>  		((video_mode == VMODE_832_624_75) &&
> -		 (color_mode > CMODE_8)) ? 0x10 : 0x20) + 0x1000;
> +		 (color_mode > CMODE_8) ? 0x10 : 0x20)) + 0x1000;
>  }
>  
>  #define STORE_D2(a, d) { \

The patch changes it to

(vres * hres * (1 << color_mode) + (VMODE && (CMODE ? 0x10 : 0x20)) + 0x1000

I think?  It makes no sense to do (foo && non-zero-value).

------------------------------------------------------------------------------
The NEW KODAK i700 Series Scanners deliver under ANY circumstances! Your
production scanning environment may not be a perfect world - but thanks to
Kodak, there's a perfect scanner to get the job done! With the NEW KODAK i700
Series Scanner you'll get full speed at 300 dpi even with all image 
processing features enabled. http://p.sf.net/sfu/kodak-com

Re: [PATCH] staging: misplaced parentheses?

[Benjamin Herrenschmidt]

On Thu, 2009-05-14 at 09:38 -0700, Andrew Morton wrote:
> > Subject: [PATCH] staging: misplaced parentheses?
> 
> The title is wrong.
> 
> On Thu, 14 May 2009 14:19:26 +0200
> Roel Kluin <roel.kluin@gmail.com> wrote:
> 
> > The leftmost `+' has a higher precedence than the `?' so this returns
> > (vres * hres * (1 << color_mode) + (VMODE && CMODE) ? 0x10 : 0x20) + 0x1000
> 
> That seems OK.

Yes, the original code looks ok and the patches one not quite ...

The idea is that the framebuffer is offset by 0x1020 normally, except
when the video mode is VMODE_832_624_75 and the depth > 8 in which case
it's offet by 0x1010. It's a weird piece of HW but I think the original
code is correct. Or do I miss something ?

Cheers,
Ben.

> > Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> > ---
> > The original code looks odd, but I am not entirely certain whether my patch is
> > correct. So please review.
> > 
> > diff --git a/drivers/video/platinumfb.c b/drivers/video/platinumfb.c
> > index 03b3670..a88f4b6 100644
> > --- a/drivers/video/platinumfb.c
> > +++ b/drivers/video/platinumfb.c
> > @@ -224,7 +224,7 @@ static inline int platinum_vram_reqd(int video_mode, int color_mode)
> >  	return vmode_attrs[video_mode-1].vres *
> >  	       (vmode_attrs[video_mode-1].hres * (1<<color_mode) +
> >  		((video_mode == VMODE_832_624_75) &&
> > -		 (color_mode > CMODE_8)) ? 0x10 : 0x20) + 0x1000;
> > +		 (color_mode > CMODE_8) ? 0x10 : 0x20)) + 0x1000;
> >  }
> >  
> >  #define STORE_D2(a, d) { \
> 
> The patch changes it to
> 
> (vres * hres * (1 << color_mode) + (VMODE && (CMODE ? 0x10 : 0x20)) + 0x1000
> 
> I think?  It makes no sense to do (foo && non-zero-value).
> 
> ------------------------------------------------------------------------------
> The NEW KODAK i700 Series Scanners deliver under ANY circumstances! Your
> production scanning environment may not be a perfect world - but thanks to
> Kodak, there's a perfect scanner to get the job done! With the NEW KODAK i700
> Series Scanner you'll get full speed at 300 dpi even with all image 
> processing features enabled. http://p.sf.net/sfu/kodak-com
> _______________________________________________
> Linux-fbdev-devel mailing list
> Linux-fbdev-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel


------------------------------------------------------------------------------
Crystal Reports - New Free Runtime and 30 Day Trial
Check out the new simplified licensing option that enables 
unlimited royalty-free distribution of the report engine 
for externally facing server and web deployment. 
http://p.sf.net/sfu/businessobjects

Re: [PATCH] staging: misplaced parentheses?

[Roel Kluin]

The leftmost `+' has a higher precedence than the `?' so this returns
(vres * (hres * (1 << color_mode) + (VMODE && CMODE) ? 0x10 : 0x20) + 0x1000

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---

> Yes, the original code looks ok and the patches one not quite ...

Ok, my former patch may have been wrong, but unless I am very much
mistaken, there is something fishy here.

> The idea is that the framebuffer is offset by 0x1020 normally, except
> when the video mode is VMODE_832_624_75 and the depth > 8 in which case
> it's offet by 0x1010. It's a weird piece of HW but I think the original
> code is correct. Or do I miss something ?

Thanks for this, let's simplify to explain what I think that goes wrong:

printf("%d\n", 1 + 0 ? 2 : 4);

This prints `2': since the `+' operator has a higher precedence than the
trinary `?' operator.

In the original code we had:

return vmode_attrs[video_mode-1].vres *
  	       (vmode_attrs[video_mode-1].hres * (1<<color_mode) +
  		((video_mode == VMODE_832_624_75) &&
		 (color_mode > CMODE_8)) ? 0x10 : 0x20) + 0x1000;

note that `hres * (1<<color_mode)' is added to
((video_mode == VMODE_832_624_75) && (color_mode > CMODE_8))
before the trinary operator `?' occurs.

So maybe instead of the first patch you may want to apply this?

diff --git a/drivers/video/platinumfb.c b/drivers/video/platinumfb.c
index 03b3670..2cc986f 100644
--- a/drivers/video/platinumfb.c
+++ b/drivers/video/platinumfb.c
@@ -224,7 +224,7 @@ static inline int platinum_vram_reqd(int video_mode, int color_mode)
 	return vmode_attrs[video_mode-1].vres *
 	       (vmode_attrs[video_mode-1].hres * (1<<color_mode) +
 		((video_mode == VMODE_832_624_75) &&
-		 (color_mode > CMODE_8)) ? 0x10 : 0x20) + 0x1000;
+		 (color_mode > CMODE_8) ? 0x1010 : 0x1020));
 }
 
 #define STORE_D2(a, d) { \

------------------------------------------------------------------------------
Crystal Reports - New Free Runtime and 30 Day Trial
Check out the new simplified licensing option that enables 
unlimited royalty-free distribution of the report engine 
for externally facing server and web deployment. 
http://p.sf.net/sfu/businessobjects

[PATCH] platinumfb: misplaced parenthesis

[Roel Kluin]

Since `+' has a higher precedence than the trinary operator `?', this
added `hres * (1 << color_mode)' to the boolean, testing videomode
and depth.

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
Sorry for the dup. this title and changelog should be better.

> The idea is that the framebuffer is offset by 0x1020 normally, except
> when the video mode is VMODE_832_624_75 and the depth > 8 in which case
> it's offet by 0x1010. It's a weird piece of HW but I think the original
> code is correct. Or do I miss something ?

Thanks for this, let's simplify to explain what I think that goes wrong:

printf("%d\n", 1 + 0 ? 2 : 4);

This prints `2': since the `+' operator has a higher precedence than the
trinary `?' operator.

In the original code we had:

return vmode_attrs[video_mode-1].vres *
  	       (vmode_attrs[video_mode-1].hres * (1<<color_mode) +
  		((video_mode == VMODE_832_624_75) &&
		 (color_mode > CMODE_8)) ? 0x10 : 0x20) + 0x1000;

note that `hres * (1 << color_mode)' is added to
((video_mode == VMODE_832_624_75) && (color_mode > CMODE_8))
before the trinary operator `?' occurs.

So maybe instead of the first patch you may want to apply this?

diff --git a/drivers/video/platinumfb.c b/drivers/video/platinumfb.c
index 03b3670..2cc986f 100644
--- a/drivers/video/platinumfb.c
+++ b/drivers/video/platinumfb.c
@@ -224,7 +224,7 @@ static inline int platinum_vram_reqd(int video_mode, int color_mode)
 	return vmode_attrs[video_mode-1].vres *
 	       (vmode_attrs[video_mode-1].hres * (1<<color_mode) +
 		((video_mode == VMODE_832_624_75) &&
-		 (color_mode > CMODE_8)) ? 0x10 : 0x20) + 0x1000;
+		 (color_mode > CMODE_8) ? 0x1010 : 0x1020));
 }
 
 #define STORE_D2(a, d) { \


------------------------------------------------------------------------------
Crystal Reports - New Free Runtime and 30 Day Trial
Check out the new simplified licensing option that enables 
unlimited royalty-free distribution of the report engine 
for externally facing server and web deployment. 
http://p.sf.net/sfu/businessobjects

Re: [PATCH] platinumfb: misplaced parenthesis

[Ville Syrjälä]

On Fri, May 15, 2009 at 10:43:51PM +0200, Roel Kluin wrote:
> Since `+' has a higher precedence than the trinary operator `?', this
> added `hres * (1 << color_mode)' to the boolean, testing videomode
> and depth.
> 
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> Sorry for the dup. this title and changelog should be better.
> 
> > The idea is that the framebuffer is offset by 0x1020 normally, except
> > when the video mode is VMODE_832_624_75 and the depth > 8 in which case
> > it's offet by 0x1010. It's a weird piece of HW but I think the original
> > code is correct. Or do I miss something ?
> 
> Thanks for this, let's simplify to explain what I think that goes wrong:
> 
> printf("%d\n", 1 + 0 ? 2 : 4);
> 
> This prints `2': since the `+' operator has a higher precedence than the
> trinary `?' operator.
> 
> In the original code we had:
> 
> return vmode_attrs[video_mode-1].vres *
>   	       (vmode_attrs[video_mode-1].hres * (1<<color_mode) +
>   		((video_mode == VMODE_832_624_75) &&
> 		 (color_mode > CMODE_8)) ? 0x10 : 0x20) + 0x1000;
> 
> note that `hres * (1 << color_mode)' is added to
> ((video_mode == VMODE_832_624_75) && (color_mode > CMODE_8))
> before the trinary operator `?' occurs.
> 
> So maybe instead of the first patch you may want to apply this?
> 
> diff --git a/drivers/video/platinumfb.c b/drivers/video/platinumfb.c
> index 03b3670..2cc986f 100644
> --- a/drivers/video/platinumfb.c
> +++ b/drivers/video/platinumfb.c
> @@ -224,7 +224,7 @@ static inline int platinum_vram_reqd(int video_mode, int color_mode)
>  	return vmode_attrs[video_mode-1].vres *
>  	       (vmode_attrs[video_mode-1].hres * (1<<color_mode) +
>  		((video_mode == VMODE_832_624_75) &&
> -		 (color_mode > CMODE_8)) ? 0x10 : 0x20) + 0x1000;
> +		 (color_mode > CMODE_8) ? 0x1010 : 0x1020));

How about just pulling that magic video_mode/cmode handling out of that
single statement so that it could be parsed with less effort?

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/

------------------------------------------------------------------------------
Crystal Reports - New Free Runtime and 30 Day Trial
Check out the new simplified licensing option that enables 
unlimited royalty-free distribution of the report engine 
for externally facing server and web deployment. 
http://p.sf.net/sfu/businessobjects

Re: [PATCH] platinumfb: misplaced parenthesis

[Benjamin Herrenschmidt]

On Sat, 2009-05-16 at 00:24 +0300, Ville Syrjälä wrote:
> How about just pulling that magic video_mode/cmode handling out of
> that
> single statement so that it could be parsed with less effort?
> 
Agreed.

Just store the base value in a variable, then do an if (VMODE/CMODE)
that adds the offset or something like that.

Cheers,
Ben.



------------------------------------------------------------------------------
Crystal Reports - New Free Runtime and 30 Day Trial
Check out the new simplified licensing option that enables 
unlimited royalty-free distribution of the report engine 
for externally facing server and web deployment. 
http://p.sf.net/sfu/businessobjects
_______________________________________________
Linux-fbdev-devel mailing list
Linux-fbdev-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

Re: [PATCH] platinumfb: misplaced parenthesis

[Roel Kluin]

Since `+' has a higher precedence than the trinary operator `?', this
added `hres * (1 << color_mode)' to the boolean testing videomode
and depth.

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
 static inline int platinum_vram_reqd(int video_mode, int color_mode)
 {
+	int offset;
+
+	if ((video_mode == VMODE_832_624_75) && (color_mode > CMODE_8))
+		offset = 0x1010;
+	else
+		offset = 0x1020;
+
 	return vmode_attrs[video_mode-1].vres *
-	       (vmode_attrs[video_mode-1].hres * (1<<color_mode) +
-		((video_mode == VMODE_832_624_75) &&
-		 (color_mode > CMODE_8)) ? 0x10 : 0x20) + 0x1000;
+		(vmode_attrs[video_mode-1].hres * (1<<color_mode) + offset);
 }
 
 #define STORE_D2(a, d) { \

------------------------------------------------------------------------------
Crystal Reports - New Free Runtime and 30 Day Trial
Check out the new simplified licensing option that enables 
unlimited royalty-free distribution of the report engine 
for externally facing server and web deployment. 
http://p.sf.net/sfu/businessobjects

Re: [PATCH] platinumfb: misplaced parenthesis

[Geert Uytterhoeven]

On Sun, May 17, 2009 at 13:53, Roel Kluin <roel.kluin@gmail.com> wrote:
> Since `+' has a higher precedence than the trinary operator `?', this
> added `hres * (1 << color_mode)' to the boolean testing videomode
> and depth.
>
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
>  static inline int platinum_vram_reqd(int video_mode, int color_mode)
>  {
> +       int offset;
> +
> +       if ((video_mode == VMODE_832_624_75) && (color_mode > CMODE_8))
> +               offset = 0x1010;
> +       else
> +               offset = 0x1020;
> +
>        return vmode_attrs[video_mode-1].vres *
> -              (vmode_attrs[video_mode-1].hres * (1<<color_mode) +
> -               ((video_mode == VMODE_832_624_75) &&
> -                (color_mode > CMODE_8)) ? 0x10 : 0x20) + 0x1000;
> +               (vmode_attrs[video_mode-1].hres * (1<<color_mode) + offset);

Now òffset' is inside the parentheses, hence it's multiplied by ...vres?

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

------------------------------------------------------------------------------
Crystal Reports - New Free Runtime and 30 Day Trial
Check out the new simplified licensing option that enables 
unlimited royalty-free distribution of the report engine 
for externally facing server and web deployment. 
http://p.sf.net/sfu/businessobjects
_______________________________________________
Linux-fbdev-devel mailing list
Linux-fbdev-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

[PATCH] platinumfb: misplaced parenthesis

[Roel Kluin]

Since `+' has a higher precedence than the trinary operator `?', this
added `hres * (1 << color_mode)' to the boolean testing videomode
and depth.

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
{Thanks,Bedankt} Geert.

diff --git a/drivers/video/platinumfb.c b/drivers/video/platinumfb.c
index 03b3670..97f3bf4 100644
--- a/drivers/video/platinumfb.c
+++ b/drivers/video/platinumfb.c
@@ -221,10 +221,14 @@ static int platinumfb_setcolreg(u_int regno, u_int red, u_int green, u_int blue,
 
 static inline int platinum_vram_reqd(int video_mode, int color_mode)
 {
-	return vmode_attrs[video_mode-1].vres *
-	       (vmode_attrs[video_mode-1].hres * (1<<color_mode) +
-		((video_mode == VMODE_832_624_75) &&
-		 (color_mode > CMODE_8)) ? 0x10 : 0x20) + 0x1000;
+	int baseval = vmode_attrs[video_mode-1].hres * (1<<color_mode);
+
+	if ((video_mode == VMODE_832_624_75) && (color_mode > CMODE_8))
+		baseval += 0x10;
+	else
+		baseval += 0x20;
+
+	return vmode_attrs[video_mode-1].vres * baseval + 0x1000;
 }
 
 #define STORE_D2(a, d) { \


------------------------------------------------------------------------------
Crystal Reports - New Free Runtime and 30 Day Trial
Check out the new simplified licensing option that enables 
unlimited royalty-free distribution of the report engine 
for externally facing server and web deployment. 
http://p.sf.net/sfu/businessobjects