From mboxrd@z Thu Jan  1 00:00:00 1970
From: Hannes Petermaier <hannes@petermaier.org>
Date: Wed, 25 Mar 2015 22:24:25 +0100
Subject: [U-Boot] [PATCH v3 4/4] common/lcd_console: introduce
 display/framebuffer rotation
In-Reply-To: <5512E13A.8000902@compulab.co.il>
References: <1426664243-30998-2-git-send-email-oe5hpm@oevsv.at>
 <1426754263-29073-1-git-send-email-oe5hpm@oevsv.at>
 <1426754263-29073-5-git-send-email-oe5hpm@oevsv.at>
 <5512E13A.8000902@compulab.co.il>
Message-ID: <55132789.2020609@petermaier.org>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de


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