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

Hi Hannes,

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

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

From: Hannes Petermaier hannes.petermaier@br-automation.com

Sometimes, for example if the display is mounted in portrait mode or even if it mounted landscape but rotated by 180 degrees, we need to rotate our content of the display respectively the framebuffer, so that user can read the messages who 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 from "vl_rot" out of "vidinfo_t" which is provided by the board specific code.

If CONFIG_LCD_ROTATION is not defined, the console will be initialized with 0 degrees rotation.

Signed-off-by: Hannes Petermaier hannes.petermaier@br-automation.com Signed-off-by: Hannes Petermaier oe5hpm@oevsv.at

---

Changes in v3:

• rename lcd_address to fbbase for better readability.
• remove empty line lcd_console.c
• use printf instead puts to inform about invalid-fb rotation.
• avoid code-duplication (move define of fbptr_t into lcd.h)

Changes in v2:

• cleanup README text for feature

• don't make code cleanups (lcd_console.c) within this patch

• remove (unnary) comment in lcd_console.h

• update year to 2015 within copyright in lcd_console.c

• move rotation related code into separate file lcd_console_rotation.c

• rework rotation code

• change meaning of vl_rot to match fbcon=rotate:<n> from the linux-kernel

  README | 22 +++++ common/Makefile | 1 + common/lcd.c | 15 ++- common/lcd_console.c | 161 +++++++++++++++++-------------- common/lcd_console_rotation.c | 208 +++++++++++++++++++++++++++++++++++++++++ include/lcd.h | 9 ++ include/lcd_console.h | 18 +++- 7 files changed, 349 insertions(+), 85 deletions(-) create mode 100644 common/lcd_console_rotation.c

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 +}

[...]

@@ -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...

diff --git a/common/lcd_console_rotation.c b/common/lcd_console_rotation.c new file mode 100644 index 0000000..dd9dadf --- /dev/null +++ b/common/lcd_console_rotation.c

[...]

+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.