PATCH: bootsplash for pxa

CHANGELOG:
Patch by Hinko Kocevar, 05 Aug 2004:
Add support for bmp command and bootsplash feature under pxa Add support for ATAG_VIDEOLFB usage when LCD is used

Dear Himba,

Quoted 4 lines · in message <[email protected]> you wrote:

CHANGELOG:
Patch by Hinko Kocevar, 05 Aug 2004:
Add support for bmp command and bootsplash feature under pxa Add support for ATAG_VIDEOLFB usage when LCD is used

I'm sorry, but I have to reject this patch.

lib_arm/armlinux.c renamed from lib_arm/armlinux.c~xcep-lcd +10 -2

...

@@ -365,8 +365,16 @@
 	params->hdr.size = tag_size (tag_videolfb);
 
 	params->u.videolfb.lfb_base = (u32) gd->fb_base;
-	/* 7168 = 256*4*56/8 - actually 2 pages (8192 bytes) are allocated */
-	params->u.videolfb.lfb_size = 7168;
+	/* 80896 = (320*240) + PAGE_SIZE; for 8bpp and not page aligned
+	 * UPDATE: lcd_setmem is not needed to get fb working, because
+	 * we are not relocating the code properly. 80896b includes
+	 * palette also - see pxafb_init_mem() for (al)location!
+	 * layout of the mem block:
+	 *  FB
+	 *  DMA descriptors
+	 *  palette
+	 */
+	params->u.videolfb.lfb_size = 80896;
 

I cannot accept this. So far, on the TRAB board only 7 kB of memory were allocated for the VFD buffer, and now you hard-wire 79 kB instead - more than 10 times more than before!

I agree that it's bad to have a hardwired value, but it's even worse to simply overwrite it. Your modification breaks U-Boot on the TRAB board. Please fix this.

[I recommend to make the actual value configurable in the board's config file.]

common/cmd_bmp.c renamed from common/cmd_bmp.c~xcep-lcd +1 -0

@@ -28,6 +28,7 @@
 #include <common.h>
 #include <bmp_layout.h>
 #include <command.h>
+#include <linux/byteorder/little_endian.h>
 
 

You should NEVER include linux/byteorder/little_endian.h!!! Always include <asm/byteorder.h> only. Anything else is broken.

cpu/pxa/pxafb.c renamed from cpu/pxa/pxafb.c~xcep-lcd +1 -0

@@ -34,6 +34,7 @@
 #include <linux/types.h>
 #include <devices.h>
 #include <asm/arch/pxa-regs.h>
+#include <linux/byteorder/little_endian.h>
 
 

Same here.

cpu/pxa/pxafb.c renamed from cpu/pxa/pxafb.c~xcep-lcd +1 -1

@@ -604,7 +643,7 @@
 	lcd_line_length = (panel_info.vl_col * NBITS (panel_info.vl_bpix)) / 8;
 
 	lcd_init (lcd_base);		/* LCD initialization */
-
+	
 

PLEASE DO NOT ADD TRAILING WHITE SPACE!!!

cpu/pxa/pxafb.c renamed from cpu/pxa/pxafb.c~xcep-lcd +24 -0

+int lcd_display_bitmap(ulong bmp_image, int x, int y)
+{
+        ushort *cmap;
+        ushort i, j;
+        uchar *fb;
+        bmp_image_t *bmp=(bmp_image_t *)bmp_image;
+        uchar *bmap;
+        ushort padded_line;
+        unsigned long width, height;
+        unsigned colors,bpix;
+        unsigned long compression;
+        struct pxafb_info *fbi = &panel_info.pxa;
+
+
+        if (!((bmp->header.signature[0]=='B') &&
+              (bmp->header.signature[1]=='M'))) {
+                printf ("Error: no valid bmp image at %lx\n", bmp_image);
+                return 1;
+        }
+
+        width = le32_to_cpu (bmp->header.width);
+        height = le32_to_cpu (bmp->header.height);
+        colors = 1<<le16_to_cpu (bmp->header.bit_count);
+        compression = le32_to_cpu (bmp->header.compression);
...

This code looks like a 1:1 copy of the same function in
"cpu/mpc8xx/lcd.c". Instead of duplicating the code, we should factor it out into a common source file.

Please fix these issues and resubmit.

Quoted 3 lines

(quoted diff) +0 -2

-	/* 7168 = 256*4*56/8 - actually 2 pages (8192 bytes) are allocated */
-	params->u.videolfb.lfb_size = 7168;

Quoted 1 line

(quoted diff) +1 -0

+	params->u.videolfb.lfb_size = 80896;

I speculated that it was just too small for any framebuffer out there nowadays, so I took liberty in increasing it.

Quoted 2 lines

[I recommend to make the actual value configurable in the board's config file.]

Fine. That would introduce another CONFIG_xxx variable, right?

Or maybe we could provide FB driver with simple function that calculates the value upon configured LCD type and returns size on-the-fly instead of hardcoding it again somewhere...

Quoted 3 lines

This code looks like a 1:1 copy of the same function in
"cpu/mpc8xx/lcd.c". Instead of duplicating the code, we should factor it out into a common source file.

It is from "cpu/mpc8xx/lcd.c".
This could be put in common/ or drivers/ I presume - all common FB functions could be there. I haven't really compared the mpc8xx and pxa FB drivers in detail, but I think pxa FB is driven out of mpc8xx FB.

OK, I'll try. Will put out RFC here in a while.