Hi Hannes,
Hi Igor,
- /* setup text-console */
- debug("[LCD] setting up console...\n");
+#ifdef CONFIG_LCD_ROTATION
- lcd_init_console(lcd_base,
panel_info.vl_col,panel_info.vl_row,panel_info.vl_rot);
- console_rows = panel_info.vl_row / VIDEO_FONT_HEIGHT;
- lcd_init_console(lcd_base,
panel_info.vl_col,panel_info.vl_row,0);Please, don't start the #ifdef mess here... just always pass the panel_info.vl_rot.
This is not possible, because 'vl_rot' does'nt exist if CONFIG_LCD_ROTATION is not defined. (have a look into lcd.h).
Of course I did before sending the reply... What I'm trying to say is - let it exist and default to 0 angle
rotation.
I made this to be compatible to all who have allready initialized a panel_info without vl_rot.
This increases the mess and I think is not sensible enough. Just change the users to initialize the panel_info with vl_rot. I think also that default initialization of globals is 0, right? If so, those users that do not initialize the vl_rot explicitly, should have it initialized to 0 implicitly by compiler.
Yes, thats a good idea. I will check if the compiler really initializes the global struct panel_info with zero and change this.
[...]
} +static inline void console_setrow180(u32 row, int clr) +{
- int i;
- uchar *dst = (uchar *)(cons.lcd_address +
(cons.rows-row-1) * VIDEO_FONT_HEIGHT *cons.lcdsizex * PIXLBYTES);- fbptr_t *d = (fbptr_t *)dst;
- for (i = 0; i < (VIDEO_FONT_HEIGHT * cons.lcdsizex); i++)
*d++ = clr;+}
+static inline void console_moverow180(u32 rowdst, u32 rowsrc) +{
- int i;
- uchar *dst = (uchar *)(cons.lcd_address +
(cons.rows-rowdst-1) * VIDEO_FONT_HEIGHT *cons.lcdsizex * PIXLBYTES);- uchar *src = (uchar *)(cons.lcd_address +
(cons.rows-rowsrc-1) * VIDEO_FONT_HEIGHT *cons.lcdsizex * PIXLBYTES);- fbptr_t *pdst = (fbptr_t *)dst;
- fbptr_t *psrc = (fbptr_t *)src;
- for (i = 0; i < (VIDEO_FONT_HEIGHT * cons.lcdsizex); i++)
*pdst++ = *psrc++;+}
+#endif /* CONFIG_LCD_ROTATION */
Can't this whole thing go into a separate file? So, the console stuff will only define weak functions which can be
overridden
by the rotation functionality. This will keep the console code clean (also from ifdefs) and have the
rotation
functionality cleanly added by a CONFIG_ symbol, which will control
the
compilation for the separate file.
Might be possible, which name should we give to it ?
lcd_console_rotation.c ?
Sounds good.
But how we deal with the function-pointer initialization ?
I think the usual method would do... You call some kind of lcd_console_init_rot() with most of the code that you currently have in lcd_init_console() that is related to
rotation.
If the CONFIG_LCD_ROTATION is not set, then the lcd_init_console() stub just returns the 0 rotation config.
I just started to move rotation specific functions into own file, called lcd_console_rotation.c and ran into some trouble.
1) I need during initialization the console_calc_rowcol(...) function, which is provided by lcd.c. A possible solution might be to "un-static" it - but i am not happy with this. Another way could be to take up vl_rot into console_t structure and pass only a pointer to structure to this function and decide inside the function. But this would create a little mix between 0 degree and rotation code. Yet another idea is to have (also having pointer to console_t in call) in lcd_console_rotation also such a calc function which overrides the values calculated before.
or maybe you've another solution ?
2) I need in almost every "paint-function" the framebuffer base (cons.lcd_address) and the screen dimension. This information is stored in the static structure within lcd.c - i don't like to make this public. A possible solution could be to change all painting function to work without some global variable and pass a third argument, pointer to console_t, and take informations from there. This will consume one more register on function call, runtime is equal i think.
Whats your opinion around this ?
I would recommend extracting the whole if else ... structure into a separate function say lcd_setup_console_rot() or something and make the default one doing only the vl_rot == 0 stuff.
Whats the use of this ? Should this also be in a separate file?
Yes, that is how I think it will do better.
Just to explain my points:
- make the rotation feature an integrative part of the code by always
using
the rotation code: if CONFIG_LCD_ROTATION is *not* set then just
rotate it
to 0 degrees and compile out the rest of the code. 2) make the rotation feature a bit more separate from the console code. I believe this way will make it cleaner.
Actually (within new code) i do initialize the pointers and things around with 0 degree rotation and call afterwards the new function lcd_init_console_rot which is implemented in lcd_console_rotation.c and "__weak" in lcd_console.c which does re-initialze fps and col/row stuff.
Also, might it be useful to allow specifiying the angle through the CONFIG_LCD_ROTATION define? Have you considered using Kconfig for this?
First i thought about this to specifiy rotation angle with a value defined CONFIG_LCD_ROTATION - but this is only usefull to one of my boards (where display is rotated 180degrees). In another board i have one U-Boot for a bunch of variants (16 at this time) where all rotation angles are needed. I want to take there this information out of device-tree and write it to vl_rot.
-- Regards, Igor.
best regards, Hannes