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

On 06/08/2012 03:52 PM, Anatolij Gustschin wrote:

Hi,

On Thu, 24 May 2012 14:42:39 +0300 Igor Grinberggrinberg@compulab.co.il wrote:

...

From: Nikita Kiryanovnikita@compulab.co.il

Simplify #ifdefs by slightly changing the order of operations

Signed-off-by: Nikita Kiryanovnikita@compulab.co.il Signed-off-by: Igor Grinberggrinberg@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