[U-Boot] [PATCH 2/7] common lcd: simplify #ifdefs

[Nikita Kiryanov <nikita@compulab.co.il>]

On 06/08/2012 03:52 PM, Anatolij Gustschin wrote:
 > Hi,
 >
 > On Thu, 24 May 2012 14:42:39 +0300
 > Igor Grinberg<grinberg@compulab.co.il>  wrote:
 >
 >> From: Nikita Kiryanov<nikita@compulab.co.il>
 >>
 >> Simplify #ifdefs by slightly changing the order of operations
 >>
 >> Signed-off-by: Nikita Kiryanov<nikita@compulab.co.il>
 >> Signed-off-by: Igor Grinberg<grinberg@compulab.co.il>
 >> ---
 >>   common/lcd.c |   20 ++++++++------------
 >>   1 files changed, 8 insertions(+), 12 deletions(-)
 >>
 >> diff --git a/common/lcd.c b/common/lcd.c
 >> index 506a138..3b2f25f 100644
 >> --- a/common/lcd.c
 >> +++ b/common/lcd.c
 >> @@ -525,20 +525,18 @@ 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 */
 >> +		/* Leave room for default color map
 >> +		 * 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 ifdefs in the next stanza
 >> +		 */
 >> +		cmap = bmp_logo_palette;
 >>   #if defined(CONFIG_CPU_PXA)
 >>   		cmap = (ushort *) fbi->palette;
 >>   #elif 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));
 >> -#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;
 >>   #endif
 >>
 >>   		WATCHDOG_RESET();
 >> @@ -680,14 +678,12 @@ int lcd_display_bitmap(ulong bmp_image, int x, 
int y)
 >>   		for (i = 0; i<  colors; ++i) {
 >>   			bmp_color_table_entry_t cte = bmp->color_table[i];
 >>   #if !defined(CONFIG_ATMEL_LCD)
 >> -			ushort colreg =
 >> +			*cmap =
 >>   				( ((cte.red)<<  8)&  0xf800) |
 >>   				( ((cte.green)<<  3)&  0x07e0) |
 >>   				( ((cte.blue)>>  3)&  0x001f) ;
 >>   #ifdef CONFIG_SYS_INVERT_COLORS
 >> -			*cmap = 0xffff - colreg;
 >> -#else
 >> -			*cmap = colreg;
 >> +			*cmap = 0xffff - *cmap;
 >>   #endif
 >>   #if defined(CONFIG_MPC823)
 >>   			cmap--;
 >
 > Sorry, but I do not like this change. We should not try to simplify it
 > this way. Better would be to just reduce all this color map setting code
 > to something like:
 >
 >          for (i = 0; i<  ARRAY_SIZE(bmp_logo_palette); ++i) {
 >                  bmp_color_table_entry_t cte = bmp->color_table[i];
 >                  lcd_setcolreg(i, cte.red, cte.green, cte.blue);
 >          }
 >
 > and fix lcd_setcolreg() functions of the drivers if/where needed.
 >
 > Thanks,
 >
 > Anatolij

This does sound like the way to go, but having looked at the different
versions of lcd_setcolreg I am unsure of how to consolidate the code in
lcd_display_bitmap (and bitmap_plot in patch 3) with the
implementations I see in the drivers.

For one, they shift the color values in a different way than the code
in common/lcd, and I'm not sure who I should listen to, what's in the
drivers or common/lcd...
Also, I see that lcd_setcolreg is used in lcd_clear, where it sets
console colors. This raises the question can/should the treatment of
bmp color map values be similar to that of console color values?

I'd appreciate some guidance on these issues.

Thanks,
Nikita