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

[Anatolij Gustschin <agust@denx.de>]

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