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

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

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.

-- 
Regards,
Nikita Kiryanov