[U-Boot-Users] PATCH: bootsplash for pxa

[Wolfgang Denk]

Dear Himba,

in message <41125EF9.40607 at siol.net> 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.

--- u-boot/lib_arm/armlinux.c~xcep-lcd	2004-08-04 22:40:27.000000000 +0200
+++ u-boot/lib_arm/armlinux.c	2004-08-04 23:10:20.000000000 +0200
...
@@ -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.]

--- u-boot/common/cmd_bmp.c~xcep-lcd	2004-08-04 22:40:55.000000000 +0200
+++ u-boot/common/cmd_bmp.c	2004-08-04 23:35:37.000000000 +0200
@@ -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.

 
--- u-boot/cpu/pxa/pxafb.c~xcep-lcd	2004-08-04 22:40:27.000000000 +0200
+++ u-boot/cpu/pxa/pxafb.c	2004-08-04 23:34:49.000000000 +0200
@@ -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.

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

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

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-4596-87  Fax: (+49)-8142-4596-88  Email: wd at denx.de
Is a computer language with goto's totally Wirth-less?