[U-Boot] [RFC] common/lcd: use lcd_setcolreg() in bitmap_plot

[U-Boot] [RFC] common/lcd: use lcd_setcolreg() in bitmap_plot

[Andreas Bießmann]

The lcd_setcolreg() is a API provided by the lcd driver and used to setup the
color lookup table. However the lcd driver setup the CLUT by using a lot
of #ifdiffery and accessing the CLUT arrays which are cleanly hidden by the API
call. Remove that and use the API.

Signed-off-by: Andreas Bie?mann <andreas.devel@googlemail.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Bo Shen <voice.shen@atmel.com>
Cc: Tom Rini <trini@ti.com>
---
This is a RFC currently cause it touches a lot of boards but is only runtime
tested on at91sam9263ek. Unfortunately a 'MAKEALL -a arm' does break cause the
atmel_hlcd driver do not provide the required API (lcd_setcolreg()) -> Bo, can
you please fix this, I do not have an atmel_hlcdc enabled device.

On the other hand this change also touches other architectures I can not test.
This patch is actually only compile tested for arm and avr32 arches.

And last I have to say that there is another position in this file where the
CLUT is modified directly by the code. This should also be changed to use the
lcd_setcolreg API.

This patch shall superseed http://patchwork.ozlabs.org/patch/193466/

 common/lcd.c |   47 +++++++++--------------------------------------
 1 file changed, 9 insertions(+), 38 deletions(-)

diff --git a/common/lcd.c b/common/lcd.c
index b6be800..4938381 100644
--- a/common/lcd.c
+++ b/common/lcd.c
@@ -523,11 +523,6 @@ static inline ushort *configuration_get_cmap(void)
 #ifdef CONFIG_LCD_LOGO
 void bitmap_plot(int x, int y)
 {
-#ifdef CONFIG_ATMEL_LCD
-	uint *cmap = (uint *)bmp_logo_palette;
-#else
-	ushort *cmap = (ushort *)bmp_logo_palette;
-#endif
 	ushort i, j;
 	uchar *bmap;
 	uchar *fb;
@@ -545,44 +540,20 @@ 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
-		 * 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 *)configuration_get_cmap();
-#else
-		cmap = configuration_get_cmap();
-#endif
 
 		WATCHDOG_RESET();
 
 		/* Set color map */
 		for (i = 0; i < ARRAY_SIZE(bmp_logo_palette); ++i) {
-			ushort colreg = bmp_logo_palette[i];
-#ifdef CONFIG_ATMEL_LCD
-			uint lut_entry;
-#ifdef CONFIG_ATMEL_LCD_BGR555
-			lut_entry = ((colreg & 0x000F) << 11) |
-					((colreg & 0x00F0) <<  2) |
-					((colreg & 0x0F00) >>  7);
-#else /* CONFIG_ATMEL_LCD_RGB565 */
-			lut_entry = ((colreg & 0x000F) << 1) |
-					((colreg & 0x00F0) << 3) |
-					((colreg & 0x0F00) << 4);
-#endif
-			*(cmap + BMP_LOGO_OFFSET) = lut_entry;
-			cmap++;
-#else /* !CONFIG_ATMEL_LCD */
-#ifdef  CONFIG_SYS_INVERT_COLORS
-			*cmap++ = 0xffff - colreg;
-#else
-			*cmap++ = colreg;
-#endif
-#endif /* CONFIG_ATMEL_LCD */
+			/* use the most significant bits here */
+			uint8_t red   = ((bmp_logo_palette[i] >> 4) & 0xF0);
+			uint8_t green = ((bmp_logo_palette[i] >> 0) & 0xF0);
+			uint8_t blue  = ((bmp_logo_palette[i] << 4) & 0xF0);
+			debug("LCD: setup colreg %u with "
+			      "R: 0x%02x G: 0x%02x B: 0x%02x (0x%04x)\n",
+			      i+BMP_LOGO_OFFSET, red, green, blue, bmp_logo_palette[i]);
+			/* leave room for the default colormap here */
+			lcd_setcolreg(i+BMP_LOGO_OFFSET, red, green, blue);
 		}
 
 		WATCHDOG_RESET();
-- 
1.7.10.4

[U-Boot] [RFC] common/lcd: use lcd_setcolreg() in bitmap_plot

[Nikita Kiryanov]

Hi Andreas,

I think this change can also be applied to lcd_display_bitmap(), and
then configuration_get_cmap() can be eliminated.

-- 
Regards,
Nikita.

[U-Boot] [RFC] common/lcd: use lcd_setcolreg() in bitmap_plot

[Andreas Bießmann]

Hi Nikita,

On 28.10.2012 08:10, Nikita Kiryanov wrote:
> Hi Andreas,
> 
> I think this change can also be applied to lcd_display_bitmap(), and
> then configuration_get_cmap() can be eliminated.

hopefully yes, but first of all I would like to hear that the change can
work on all supported devices, so please test and send results to the list.

Best regards

Andreas Bie?mann

[U-Boot] [RFC] common/lcd: use lcd_setcolreg() in bitmap_plot

[Bo Shen]

On 10/26/2012 19:23, Andreas Bie?mann wrote:
> The lcd_setcolreg() is a API provided by the lcd driver and used to setup the
> color lookup table. However the lcd driver setup the CLUT by using a lot
> of #ifdiffery and accessing the CLUT arrays which are cleanly hidden by the API
> call. Remove that and use the API.
>
> Signed-off-by: Andreas Bie?mann <andreas.devel@googlemail.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Bo Shen <voice.shen@atmel.com>
> Cc: Tom Rini <trini@ti.com>
> ---
> This is a RFC currently cause it touches a lot of boards but is only runtime
> tested on at91sam9263ek. Unfortunately a 'MAKEALL -a arm' does break cause the
> atmel_hlcd driver do not provide the required API (lcd_setcolreg()) -> Bo, can
> you please fix this, I do not have an atmel_hlcdc enabled device.
>
> On the other hand this change also touches other architectures I can not test.
> This patch is actually only compile tested for arm and avr32 arches.
>
> And last I have to say that there is another position in this file where the
> CLUT is modified directly by the code. This should also be changed to use the
> lcd_setcolreg API.
>
> This patch shall superseed http://patchwork.ozlabs.org/patch/193466/
>
>   common/lcd.c |   47 +++++++++--------------------------------------
>   1 file changed, 9 insertions(+), 38 deletions(-)

Tested on sam9x5ek board. It works well.

Tested-by: Bo Shen <voice.shen@atmel.com>

> diff --git a/common/lcd.c b/common/lcd.c
> index b6be800..4938381 100644
> --- a/common/lcd.c
> +++ b/common/lcd.c
> @@ -523,11 +523,6 @@ static inline ushort *configuration_get_cmap(void)
>   #ifdef CONFIG_LCD_LOGO
>   void bitmap_plot(int x, int y)
>   {
> -#ifdef CONFIG_ATMEL_LCD
> -	uint *cmap = (uint *)bmp_logo_palette;
> -#else
> -	ushort *cmap = (ushort *)bmp_logo_palette;
> -#endif
>   	ushort i, j;
>   	uchar *bmap;
>   	uchar *fb;
> @@ -545,44 +540,20 @@ 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
> -		 * 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 *)configuration_get_cmap();
> -#else
> -		cmap = configuration_get_cmap();
> -#endif
>
>   		WATCHDOG_RESET();
>
>   		/* Set color map */
>   		for (i = 0; i < ARRAY_SIZE(bmp_logo_palette); ++i) {
> -			ushort colreg = bmp_logo_palette[i];
> -#ifdef CONFIG_ATMEL_LCD
> -			uint lut_entry;
> -#ifdef CONFIG_ATMEL_LCD_BGR555
> -			lut_entry = ((colreg & 0x000F) << 11) |
> -					((colreg & 0x00F0) <<  2) |
> -					((colreg & 0x0F00) >>  7);
> -#else /* CONFIG_ATMEL_LCD_RGB565 */
> -			lut_entry = ((colreg & 0x000F) << 1) |
> -					((colreg & 0x00F0) << 3) |
> -					((colreg & 0x0F00) << 4);
> -#endif
> -			*(cmap + BMP_LOGO_OFFSET) = lut_entry;
> -			cmap++;
> -#else /* !CONFIG_ATMEL_LCD */
> -#ifdef  CONFIG_SYS_INVERT_COLORS
> -			*cmap++ = 0xffff - colreg;
> -#else
> -			*cmap++ = colreg;
> -#endif
> -#endif /* CONFIG_ATMEL_LCD */
> +			/* use the most significant bits here */
> +			uint8_t red   = ((bmp_logo_palette[i] >> 4) & 0xF0);
> +			uint8_t green = ((bmp_logo_palette[i] >> 0) & 0xF0);
> +			uint8_t blue  = ((bmp_logo_palette[i] << 4) & 0xF0);
> +			debug("LCD: setup colreg %u with "
> +			      "R: 0x%02x G: 0x%02x B: 0x%02x (0x%04x)\n",
> +			      i+BMP_LOGO_OFFSET, red, green, blue, bmp_logo_palette[i]);
> +			/* leave room for the default colormap here */
> +			lcd_setcolreg(i+BMP_LOGO_OFFSET, red, green, blue);
>   		}
>
>   		WATCHDOG_RESET();
>