[PATCH] staging: misplaced parentheses?

Roel Kluin

author

2009-05-14 12:49 UTC #0 lore raw

PATCH 1/1 staging: misplaced parentheses?

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 <[email protected]>

---
The original code looks odd, but I am not entirely certain whether my patch is correct. So please review.

drivers/video/platinumfb.c +1 -1

index 03b3670..a88f4b6 100644
@@ -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

Andrew Morton

2009-05-14 16:39 UTC #1 lore raw

Quoted 1 line

Subject: [PATCH] staging: misplaced parentheses?

The title is wrong.

Quoted 2 lines · On Thu, 14 May 2009 14:19:26 +0200 Roel Kluin <[email protected]> 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.

Quoted 10 lines

Signed-off-by Roel Kluin <[email protected]>
---
The original code looks odd, but I am not entirely certain whether my patch is correct. So please review.
drivers/video/platinumfb.c +0 -0
index 03b3670..a88f4b6 100644

drivers/video/platinumfb.c static inline int platinum_vram_reqd(int video_mode, int color_mode) · line 230

Quoted 8 lines

drivers/video/platinumfb.c +1 -1

@@ -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

Benjamin Herrenschmidt

2009-05-14 23:25 UTC #2 lore raw

Quoted 11 lines · Andrew Morton

Quoted 1 line
Subject: [PATCH] staging: misplaced parentheses?
The title is wrong.
Quoted 2 lines · On Thu, 14 May 2009 14:19:26 +0200 Roel Kluin <[email protected]> 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.

Quoted 25 lines

Quoted 10 lines
Signed-off-by Roel Kluin <[email protected]>
---
The original code looks odd, but I am not entirely certain whether my patch is correct. So please review.
drivers/video/platinumfb.c +0 -0
index 03b3670..a88f4b6 100644
drivers/video/platinumfb.c static inline int platinum_vram_reqd(int video_mode, int color_mode) · line 230
Quoted 8 lines
drivers/video/platinumfb.c +1 -1
@@ -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).

Quoted 10 lines

------------------------------------------------------------------------------
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 [email protected]
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

Roel Kluin

author

2009-05-15 10:52 UTC #3 lore raw

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 <[email protected]>

---

Quoted 1 line

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.

Quoted 4 lines

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?

drivers/video/platinumfb.c +1 -1

index 03b3670..2cc986f 100644
@@ -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

Roel Kluin

author

2009-05-15 20:44 UTC #4 lore raw

PATCH 1/1 platinumfb: misplaced parenthesis

In reply to Benjamin Herrenschmidt

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 <[email protected]>

---
Sorry for the dup. this title and changelog should be better.

Quoted 4 lines

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?

drivers/video/platinumfb.c +1 -1

index 03b3670..2cc986f 100644
@@ -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

Ville Syrjälä

2009-05-15 21:25 UTC #5 lore raw

Re: [PATCH] platinumfb: misplaced parenthesis

Quoted 38 lines · 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 <[email protected]>
---
Sorry for the dup. this title and changelog should be better.
Quoted 4 lines
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?
drivers/video/platinumfb.c +0 -0
index 03b3670..2cc986f 100644

drivers/video/platinumfb.c static inline int platinum_vram_reqd(int video_mode, int color_mode) · line 227

Quoted 5 lines

drivers/video/platinumfb.c +1 -1

@@ -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?

Signature · 10 lines

Ville Syrjälä
[email protected]
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

Benjamin Herrenschmidt

2009-05-15 22:21 UTC #6 lore raw

Re: [PATCH] platinumfb: misplaced parenthesis

Quoted 4 lines · Ville Syrjälä

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 [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

Roel Kluin

author

2009-05-17 11:53 UTC #7 lore raw

Re: [PATCH] platinumfb: misplaced parenthesis

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 <[email protected]>

---
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

Geert Uytterhoeven

2009-05-17 17:11 UTC #8 lore raw

Re: [PATCH] platinumfb: misplaced parenthesis

Quoted 15 lines · 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 <[email protected]>
---
static inline int platinum_vram_reqd(int video_mode, int color_mode) {
+ int offset;
(quoted diff) +6 -0
+
+       if ((video_mode == VMODE_832_624_75) && (color_mode > CMODE_8))
+               offset = 0x1010;
+       else
+               offset = 0x1020;
+

Quoted 5 lines

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 -- [email protected]

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 [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

Roel Kluin

author

2009-05-18 10:24 UTC #9 lore raw

PATCH 1/1 platinumfb: misplaced parenthesis

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 <[email protected]>

---
{Thanks,Bedankt} Geert.

drivers/video/platinumfb.c +8 -4

index 03b3670..97f3bf4 100644
@@ -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