Re: [PATCH 1/1] New Framebuffer for Intel based Macs

[Randy.Dunlap]

On Tue, 20 Jun 2006 15:37:22 +0200 Edgar Hucek wrote:

> This patch add a new framebuffer driver for the Intel Based macs.
> This framebuffer is needed when booting from EFI to get something
> out the box ;)


To the extent possible, please list #include files in
alpha order within each group of <linux>, <asm>, etc.


+typedef enum _MAC_TAPE {
+	M_I17,
+	M_I20,
+	M_MINI,
+	M_MACBOOK,
+	M_NEW
+} MAC_TAPE;

What is TAPE?  should this be TYPE?

Use hard tabs on this line:
+      	.activate	= FB_ACTIVATE_NOW,


+static int             inverse   = 0;
+static int             mtrr      = 0; /* disable mtrr */
+static int	       vram_remap __initdata = 0; /* Set amount of memory to be used */
+static int	       vram_total __initdata = 0; /* Set total amount of memory */
+static int             depth;
+static int             model = M_NEW;
+static int             manual_height = 0;
+static int             manual_width = 0;

Misaligned.  Using hard tabs would help.

+		if (! strcmp(this_opt, "inverse"))
+			inverse=1;

No space between ! and strcmp (multiple places).
Do put spaces around '=' sign.

+#define dac_reg	(0x3c8)
+#define dac_val	(0x3c9)

Are these used?  And the parens are superfluous.
However, if you want to use parens, then using them here would
make more sense:
+#define	DEFAULT_FB_MEM	1024*1024*16

+	if(manual_height > 0)
+		screen_info.lfb_height = manual_height;
+	if(manual_width > 0)
+		screen_info.lfb_width = manual_width;

Space after 'if'.

+	printk(KERN_INFO "imacfb: mode is %dx%dx%d, linelength=%d, pages=%d\n",
+	       imacfb_defined.xres, imacfb_defined.yres, imacfb_defined.bits_per_pixel, imacfb_fix.line_length, screen_info.pages);

Break long source lines at < 80 columns, please.

+	if (imacfb_defined.bits_per_pixel <= 8) {
+		depth = imacfb_defined.green.length;
+		imacfb_defined.red.length =
+		imacfb_defined.green.length =
+		imacfb_defined.blue.length =
+		imacfb_defined.bits_per_pixel;
+	}

Maybe that last line should be indented one more tab stop.
Anyway, they shouldn't all be at the same indentation.


---
~Randy
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/