
Wolfgang Denk wrote:
Dear Himba,
in message 41125EF9.40607@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 speculated that it was just too small for any framebuffer out there nowadays, so I took liberty in increasing it.
[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...
--- 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.
Acked.
--- 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!!!
And I thought my double check was enough...
+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.
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.
Please fix these issues and resubmit.
OK, I'll try. Will put out RFC here in a while.
regards, himba