Re: [U-Boot] [PATCH v3 4/4] common/lcd_console: introduce display/framebuffer rotation

On 2015-03-25 17:24, Nikita Kiryanov wrote:

Hi Hannes,

Hi Nikita,

This is almost an Acked-By from me, just a few final comments:

Perfect, i think with v4 we can finish the thing :-)

On 03/19/2015 10:37 AM, Hannes Petermaier wrote:

...

diff --git a/README b/README index b0124d6..c649de1 100644 --- a/README +++ b/README @@ -1947,6 +1947,28 @@ CBFS (Coreboot Filesystem) support the console jump but can help speed up operation when scrolling is slow.

•    CONFIG_LCD_ROTATION
  

•    Sometimes, for example if the display is mounted in portrait
  

•    mode or even if it mounted landscape but rotated by 180degree,
  

s/if it/if it's/

...

•    we need to rotate our content of the display respectively the
  

s/respectively the/relative to the/

...

•    framebuffer, so that user can read the messages who are printed
  

s/who are printed/which are printed/

...

•    out.
  

•    For this we introduce the feature called "CONFIG_LCD_ROTATION",
  

•    this may be defined in the board-configuration if needed.  
  

After

•    this the lcd_console will be initialized with a given rotation
  

"this may be defined in the board-configuration if needed" This is true for all config options in general, no need to mention this. Also, "For this we introduce" is good for a commit message, but doesn't look good once committed. How about just "Once CONFIG_LCD_ROTATION is defined, the lcd_console will be..."

...

•    from "vl_rot" out of "vidinfo_t" which is provided by the board
  

•    specific code.
  

•    The value for vl_rot is coded as following (matching to
  

•    fbcon=rotate:<n> linux-kernel commandline):
  

•    0 = no rotation respectively 0 degree
  

•    1 = 90 degree rotation
  

•    2 = 180 degree rotation
  

•    3 = 270 degree rotation
  

•    If CONFIG_LCD_ROTATION is not defined, the console will be
  

•    initialized with 0degree rotation.
  

•     CONFIG_LCD_BMP_RLE8
  
      Support drawing of RLE8-compressed bitmaps on the LCD.
  

[...]

...

+static void console_calc_rowcol(struct console_t *pcons) +{

• pcons->cols = pcons->lcdsizex / VIDEO_FONT_WIDTH;

+#if defined(CONFIG_LCD_LOGO) && !defined(CONFIG_LCD_INFO_BELOW_LOGO)

• pcons->rows = (pcons->lcdsizey - BMP_LOGO_HEIGHT);
• pcons->rows /= VIDEO_FONT_HEIGHT;

+#else

• pcons->rows = pcons->lcdsizey / VIDEO_FONT_HEIGHT;

+#endif +}

Okay, i will fixup the description in v4 ... maybe these troubles are coming from, lets say, sub-optimal english-language practise :-) In original i speak german.

[...]

...

@@ -235,4 +253,3 @@ U_BOOT_CMD( "print string on lcd-framebuffer", " <string>" );

Looks like part of the cleanup from the previous series slipped through...

Okay, i will remove it.

...

+static void console_calc_rowcol_rot(struct console_t *pcons) +{

• u32 cols, rows;
• if (pcons->lcdrot == 1 || pcons->lcdrot == 3) {

•    cols = pcons->lcdsizey;
  

•    rows = pcons->lcdsizex;
  

• } else {

•    cols = pcons->lcdsizex;
  

•    rows = pcons->lcdsizey;
  

• }
• pcons->cols = cols / VIDEO_FONT_WIDTH;

+#if defined(CONFIG_LCD_LOGO) && !defined(CONFIG_LCD_INFO_BELOW_LOGO)

• pcons->rows = (rows - BMP_LOGO_HEIGHT);
• pcons->rows /= VIDEO_FONT_HEIGHT;

+#else

• pcons->rows = rows / VIDEO_FONT_HEIGHT;

+#endif

This duplication with console_calc_rowcol() exists because the lcdsizey and lcdsizex data is expected by the functions to be already in pcons. If you change console_calc_rowcol() to accept both variables as additional arguments, then console_calc_rowcol() could be reused in console_calc_rowcol_rot() and we'll get rid of the code duplication.

I'm not sure about what is more uggly or better. To avoid this duplication and use one function i have to make this function non-static and make a mix of rotation-code into lcd_console.c - i wouldn't prefer this. Maybe the actual way of this (little) duplication is the beautiful one and gives most readability of the code.

what do you mean about?

best regards, Hannes