[U-Boot] [PATCH V2 0/4] common/lcd cleanup

[U-Boot] [PATCH V2 0/4] common/lcd cleanup

[Nikita Kiryanov]

This patch series attempts to simplify #ifdef complexity in common/lcd.c.

It was compile tested on Arm and PowerPC using MAKEALL

checkpatch reports warnings on some of the patches:
0003: WARNING: use of volatile is usually wrong
	Since 'volatile' was in the original code I left it in the patch as
	well.

Changes in V2:
	- Rebased on u-boot-video
	- patches 2 and 3 of original patchset dropped because I'm not sure
	what to do about them
	- simplify lcd_logo: used bitmap_display() to further simplify code
	- simplify lcd_display_bitmap: fixed pointer increment error
	- simplify lcd_display_bitmap: change to simplify lcd_logo breaks
	MCC200 board because it does not #define CONFIG_CMD_BMP. Added a local
	implementation of bitmap_display().

Nikita Kiryanov (4):
  common lcd: simplify lcd_logo
  common lcd: simplify lcd_display
  common lcd: simplify core functions
  common lcd: simplify lcd_display_bitmap

 board/mcc200/lcd.c |   20 +++++++
 common/lcd.c       |  141 +++++++++++++++++++++++++++-------------------------
 2 files changed, 94 insertions(+), 67 deletions(-)

[U-Boot] [PATCH V2 1/4] common lcd: simplify lcd_logo

[Nikita Kiryanov]

Simplify lcd_logo by extracting bmp unzip into its own function.

Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
---
Changes in V2:
	- used bitmap_display() to further simplify code

 common/lcd.c |   12 +-----------
 1 files changed, 1 insertions(+), 11 deletions(-)

diff --git a/common/lcd.c b/common/lcd.c
index 506a138..8890635 100644
--- a/common/lcd.c
+++ b/common/lcd.c
@@ -842,17 +842,7 @@ static void *lcd_logo(void)
 		}
 #endif /* CONFIG_SPLASH_SCREEN_ALIGN */
 
-#ifdef CONFIG_VIDEO_BMP_GZIP
-		bmp_image_t *bmp = (bmp_image_t *)addr;
-		unsigned long len;
-
-		if (!((bmp->header.signature[0] == 'B') &&
-			(bmp->header.signature[1] == 'M'))) {
-			addr = (ulong)gunzip_bmp(addr, &len);
-		}
-#endif
-
-		if (lcd_display_bitmap(addr, x, y) == 0)
+		if (bmp_display(addr, x, y) == 0)
 			return (void *)lcd_base;
 	}
 #endif /* CONFIG_SPLASH_SCREEN */
-- 
1.7.1

[U-Boot] [PATCH V2 2/4] common lcd: simplify lcd_display

[Nikita Kiryanov]

Simplify lcd_display by centralizing code into a funciton

Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
---
 common/lcd.c |   27 ++++++++++++++++++---------
 1 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/common/lcd.c b/common/lcd.c
index 8890635..4a5c8d5 100644
--- a/common/lcd.c
+++ b/common/lcd.c
@@ -607,6 +607,22 @@ static inline void bitmap_plot(int x, int y) {}
 
 #ifdef CONFIG_SPLASH_SCREEN_ALIGN
 #define BMP_ALIGN_CENTER	0x7FFF
+
+static void splash_align_axis(int *axis, unsigned long panel_size,
+					unsigned long picture_size)
+{
+	unsigned long panel_picture_delta = panel_size - picture_size;
+	unsigned long axis_alignment;
+
+	if (*axis == BMP_ALIGN_CENTER)
+		axis_alignment = panel_picture_delta / 2;
+	else if (*axis < 0)
+		axis_alignment = panel_picture_delta + *axis + 1;
+	else
+		return;
+
+	*axis = max(0, axis_alignment);
+}
 #endif
 
 int lcd_display_bitmap(ulong bmp_image, int x, int y)
@@ -722,15 +738,8 @@ int lcd_display_bitmap(ulong bmp_image, int x, int y)
 	padded_line = (width&0x3) ? ((width&~0x3)+4) : (width);
 
 #ifdef CONFIG_SPLASH_SCREEN_ALIGN
-	if (x == BMP_ALIGN_CENTER)
-		x = max(0, (pwidth - width) / 2);
-	else if (x < 0)
-		x = max(0, pwidth - width + x + 1);
-
-	if (y == BMP_ALIGN_CENTER)
-		y = max(0, (panel_info.vl_row - height) / 2);
-	else if (y < 0)
-		y = max(0, panel_info.vl_row - height + y + 1);
+	splash_align_axis(&x, pwidth, width);
+	splash_align_axis(&y, panel_info.vl_row, height);
 #endif /* CONFIG_SPLASH_SCREEN_ALIGN */
 
 	if ((x + width) > pwidth)
-- 
1.7.1

[U-Boot] [PATCH V2 3/4] common lcd: simplify core functions

[Nikita Kiryanov]

Move highly platform dependant code into its own function to reduce the
number of #ifdefs in the bigger functions

Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
---
 common/lcd.c |   58 ++++++++++++++++++++++++++++------------------------------
 1 files changed, 28 insertions(+), 30 deletions(-)

diff --git a/common/lcd.c b/common/lcd.c
index 4a5c8d5..3c0f1b1 100644
--- a/common/lcd.c
+++ b/common/lcd.c
@@ -498,21 +498,35 @@ static int lcd_getbgcolor(void)
 /************************************************************************/
 /* ** Chipset depending Bitmap / Logo stuff...                          */
 /************************************************************************/
+static inline ushort *configuration_get_cmap(void)
+{
+#if defined CONFIG_CPU_PXA
+	struct pxafb_info *fbi = &panel_info.pxa;
+	return (ushort *)fbi->palette;
+#elif defined(CONFIG_MPC823)
+	volatile immap_t *immr = (immap_t *) CONFIG_SYS_IMMR;
+	volatile cpm8xx_t *cp = &(immr->im_cpm);
+	return (ushort *)&(cp->lcd_cmap[255 * sizeof(ushort)]);
+#elif defined(CONFIG_ATMEL_LCD)
+	return (ushort *)(panel_info.mmio + ATMEL_LCDC_LUT(0));
+#else
+	return (ushort *)panel_info.cmap;
+#endif
+}
+
 #ifdef CONFIG_LCD_LOGO
 void bitmap_plot(int x, int y)
 {
 #ifdef CONFIG_ATMEL_LCD
-	uint *cmap;
+	uint *cmap = (uint *)bmp_logo_palette;
 #else
-	ushort *cmap;
+	ushort *cmap = (ushort *)bmp_logo_palette;
 #endif
 	ushort i, j;
 	uchar *bmap;
 	uchar *fb;
 	ushort *fb16;
-#if defined(CONFIG_CPU_PXA)
-	struct pxafb_info *fbi = &panel_info.pxa;
-#elif defined(CONFIG_MPC823)
+#if defined(CONFIG_MPC823)
 	volatile immap_t *immr = (immap_t *) CONFIG_SYS_IMMR;
 	volatile cpm8xx_t *cp = &(immr->im_cpm);
 #endif
@@ -525,20 +539,17 @@ void bitmap_plot(int x, int y)
 	fb   = (uchar *)(lcd_base + y * lcd_line_length + x);
 
 	if (NBITS(panel_info.vl_bpix) < 12) {
-		/* Leave room for default color map */
-#if defined(CONFIG_CPU_PXA)
-		cmap = (ushort *) fbi->palette;
-#elif defined(CONFIG_MPC823)
+		/* Leave room for default color map
+		 * default case: generic system with no cmap (most likely 16bpp)
+		 * cmap was set to the source palette, so no change is done.
+		 * This avoids even more ifdefs in the next stanza
+		 */
+#if defined(CONFIG_MPC823)
 		cmap = (ushort *) &(cp->lcd_cmap[BMP_LOGO_OFFSET * sizeof(ushort)]);
 #elif defined(CONFIG_ATMEL_LCD)
-		cmap = (uint *) (panel_info.mmio + ATMEL_LCDC_LUT(0));
+		cmap = (uint *)configuration_get_cmap();
 #else
-		/*
-		 * default case: generic system with no cmap (most likely 16bpp)
-		 * We set cmap to the source palette, so no change is done.
-		 * This avoids even more ifdef in the next stanza
-		 */
-		cmap = bmp_logo_palette;
+		cmap = configuration_get_cmap();
 #endif
 
 		WATCHDOG_RESET();
@@ -639,12 +650,6 @@ int lcd_display_bitmap(ulong bmp_image, int x, int y)
 	unsigned long width, height, byte_width;
 	unsigned long pwidth = panel_info.vl_col;
 	unsigned colors, bpix, bmp_bpix;
-#if defined(CONFIG_CPU_PXA)
-	struct pxafb_info *fbi = &panel_info.pxa;
-#elif defined(CONFIG_MPC823)
-	volatile immap_t *immr = (immap_t *) CONFIG_SYS_IMMR;
-	volatile cpm8xx_t *cp = &(immr->im_cpm);
-#endif
 
 	if (!((bmp->header.signature[0] == 'B') &&
 		(bmp->header.signature[1] == 'M'))) {
@@ -682,14 +687,7 @@ int lcd_display_bitmap(ulong bmp_image, int x, int y)
 #if !defined(CONFIG_MCC200)
 	/* MCC200 LCD doesn't need CMAP, supports 1bpp b&w only */
 	if (bmp_bpix == 8) {
-#if defined(CONFIG_CPU_PXA)
-		cmap = (ushort *)fbi->palette;
-#elif defined(CONFIG_MPC823)
-		cmap = (ushort *)&(cp->lcd_cmap[255*sizeof(ushort)]);
-#elif !defined(CONFIG_ATMEL_LCD) && !defined(CONFIG_EXYNOS_FB)
-		cmap = panel_info.cmap;
-#endif
-
+		cmap = configuration_get_cmap();
 		cmap_base = cmap;
 
 		/* Set color map */
-- 
1.7.1

[U-Boot] [PATCH V2 4/4] common lcd: simplify lcd_display_bitmap

[Nikita Kiryanov]

Move highly platform dependant code into its own functions to reduce the
number of #ifdefs in lcd_display_bitmap

To avoid breaking the mcc200 board which does not #define
CONFIG_CMD_BMP, this patch also implements bmp_display() for mcc200.

Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
---
Changes in V2:
	- A change in a previous patch broke mcc200 board which does not
	#define CONFIG_CMD_BMP. Added a local implementation of bitmap_display()

 board/mcc200/lcd.c |   20 ++++++++++++++++++++
 common/lcd.c       |   44 +++++++++++++++++++++++++++-----------------
 2 files changed, 47 insertions(+), 17 deletions(-)

diff --git a/board/mcc200/lcd.c b/board/mcc200/lcd.c
index d8f754c..893f4b7 100644
--- a/board/mcc200/lcd.c
+++ b/board/mcc200/lcd.c
@@ -21,6 +21,7 @@
 #include <common.h>
 #include <lcd.h>
 #include <mpc5xxx.h>
+#include <malloc.h>
 
 #ifdef CONFIG_LCD
 
@@ -210,4 +211,23 @@ void show_progress (int size, int tot)
 }
 
 #endif
+
+int bmp_display(ulong addr, int x, int y)
+{
+	int ret;
+	bmp_image_t *bmp = (bmp_image_t *)addr;
+
+	if (!bmp) {
+		printf("There is no valid bmp file at the given address\n");
+		return 1;
+	}
+
+	ret = lcd_display_bitmap((ulong)bmp, x, y);
+
+	if ((unsigned long)bmp != addr)
+		free(bmp);
+
+	return ret;
+}
+
 #endif /* CONFIG_LCD */
diff --git a/common/lcd.c b/common/lcd.c
index 3c0f1b1..e55b457 100644
--- a/common/lcd.c
+++ b/common/lcd.c
@@ -636,6 +636,29 @@ static void splash_align_axis(int *axis, unsigned long panel_size,
 }
 #endif
 
+#if defined CONFIG_CPU_PXA || defined(CONFIG_ATMEL_LCD)
+#define FB_PUT_BYTE(fb, from) *(fb)++ = *(from)++
+#elif defined(CONFIG_MPC823) || defined(CONFIG_MCC200)
+#define FB_PUT_BYTE(fb, from) *(fb)++ = (255 - *(from)++)
+#endif
+
+#if defined(CONFIG_BMP_16BPP)
+#if defined(CONFIG_ATMEL_LCD_BGR555)
+static inline void fb_put_word(uchar **fb, uchar **from)
+{
+	*(*fb)++ = (((*from)[0] & 0x1f) << 2) | ((*from)[1] & 0x03);
+	*(*fb)++ = ((*from)[0] & 0xe0) | (((*from)[1] & 0x7c) >> 2);
+	*from += 2;
+}
+#else
+static inline void fb_put_word(uchar **fb, uchar **from)
+{
+	*(*fb)++ = *(*from)++;
+	*(*fb)++ = *(*from)++;
+}
+#endif
+#endif /* CONFIG_BMP_16BPP */
+
 int lcd_display_bitmap(ulong bmp_image, int x, int y)
 {
 #if !defined(CONFIG_MCC200)
@@ -761,11 +784,7 @@ int lcd_display_bitmap(ulong bmp_image, int x, int y)
 			WATCHDOG_RESET();
 			for (j = 0; j < width; j++) {
 				if (bpix != 16) {
-#if defined(CONFIG_CPU_PXA) || defined(CONFIG_ATMEL_LCD)
-					*(fb++) = *(bmap++);
-#elif defined(CONFIG_MPC823) || defined(CONFIG_MCC200)
-					*(fb++) = 255 - *(bmap++);
-#endif
+					FB_PUT_BYTE(fb, bmap);
 				} else {
 					*(uint16_t *)fb = cmap_base[*(bmap++)];
 					fb += sizeof(uint16_t) / sizeof(*fb);
@@ -780,18 +799,9 @@ int lcd_display_bitmap(ulong bmp_image, int x, int y)
 	case 16:
 		for (i = 0; i < height; ++i) {
 			WATCHDOG_RESET();
-			for (j = 0; j < width; j++) {
-#if defined(CONFIG_ATMEL_LCD_BGR555)
-				*(fb++) = ((bmap[0] & 0x1f) << 2) |
-					(bmap[1] & 0x03);
-				*(fb++) = (bmap[0] & 0xe0) |
-					((bmap[1] & 0x7c) >> 2);
-				bmap += 2;
-#else
-				*(fb++) = *(bmap++);
-				*(fb++) = *(bmap++);
-#endif
-			}
+			for (j = 0; j < width; j++)
+				fb_put_word(&fb, &bmap);
+
 			bmap += (padded_line - width) * 2;
 			fb   -= (width * 2 + lcd_line_length);
 		}
-- 
1.7.1

[U-Boot] [PATCH V2 3/4] common lcd: simplify core functions

[Wolfgang Denk]

Dear Nikita Kiryanov,

In message <1340607844-8718-4-git-send-email-nikita@compulab.co.il> you wrote:
> Move highly platform dependant code into its own function to reduce the
> number of #ifdefs in the bigger functions
> 
> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
> Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
> ---
>  common/lcd.c |   58 ++++++++++++++++++++++++++++------------------------------
>  1 files changed, 28 insertions(+), 30 deletions(-)
> 
> diff --git a/common/lcd.c b/common/lcd.c
> index 4a5c8d5..3c0f1b1 100644
> --- a/common/lcd.c
> +++ b/common/lcd.c
> @@ -498,21 +498,35 @@ static int lcd_getbgcolor(void)
>  /************************************************************************/
>  /* ** Chipset depending Bitmap / Logo stuff...                          */
>  /************************************************************************/
> +static inline ushort *configuration_get_cmap(void)
> +{
> +#if defined CONFIG_CPU_PXA
> +	struct pxafb_info *fbi = &panel_info.pxa;
> +	return (ushort *)fbi->palette;
> +#elif defined(CONFIG_MPC823)
> +	volatile immap_t *immr = (immap_t *) CONFIG_SYS_IMMR;
> +	volatile cpm8xx_t *cp = &(immr->im_cpm);
> +	return (ushort *)&(cp->lcd_cmap[255 * sizeof(ushort)]);
> +#elif defined(CONFIG_ATMEL_LCD)
> +	return (ushort *)(panel_info.mmio + ATMEL_LCDC_LUT(0));
> +#else
> +	return (ushort *)panel_info.cmap;
> +#endif
> +}

Please fix and use I/O accessors instead of volatile pointers.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Program maintenance is an entropy-increasing process,  and  even  its
most skilfull execution only delays the subsidence of the system into
unfixable obsolescence.       - Fred Brooks, "The Mythical Man Month"

[U-Boot] [PATCH V2 3/4] common lcd: simplify core functions

[Nikita Kiryanov]

On 06/25/2012 10:23 AM, Wolfgang Denk wrote:
> Dear Nikita Kiryanov,
>
> In message <1340607844-8718-4-git-send-email-nikita@compulab.co.il> you wrote:
>> Move highly platform dependant code into its own function to reduce the
>> number of #ifdefs in the bigger functions
>>
>> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
>> Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
>> ---
>>   common/lcd.c |   58 ++++++++++++++++++++++++++++------------------------------
>>   1 files changed, 28 insertions(+), 30 deletions(-)
>>
>> diff --git a/common/lcd.c b/common/lcd.c
>> index 4a5c8d5..3c0f1b1 100644
>> --- a/common/lcd.c
>> +++ b/common/lcd.c
>> @@ -498,21 +498,35 @@ static int lcd_getbgcolor(void)
>>   /************************************************************************/
>>   /* ** Chipset depending Bitmap / Logo stuff...                          */
>>   /************************************************************************/
>> +static inline ushort *configuration_get_cmap(void)
>> +{
>> +#if defined CONFIG_CPU_PXA
>> +	struct pxafb_info *fbi = &panel_info.pxa;
>> +	return (ushort *)fbi->palette;
>> +#elif defined(CONFIG_MPC823)
>> +	volatile immap_t *immr = (immap_t *) CONFIG_SYS_IMMR;
>> +	volatile cpm8xx_t *cp = &(immr->im_cpm);
>> +	return (ushort *)&(cp->lcd_cmap[255 * sizeof(ushort)]);
>> +#elif defined(CONFIG_ATMEL_LCD)
>> +	return (ushort *)(panel_info.mmio + ATMEL_LCDC_LUT(0));
>> +#else
>> +	return (ushort *)panel_info.cmap;
>> +#endif
>> +}
>
> Please fix and use I/O accessors instead of volatile pointers.
>
> Best regards,
>
> Wolfgang Denk
>

The goal of this patch set was to reduce #define complexity in common/lcd.
Do you mind if I address the volatile pointers with a follow up patch?

--
Nikita