On 03/16/15 10:32, Hannes Petermaier wrote: >> 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); >>>> #else >>>> - console_rows = panel_info.vl_row / VIDEO_FONT_HEIGHT; >>>> + lcd_init_console(lcd_base, >>>> + panel_info.vl_col, >>>> + panel_info.vl_row, >>>> + 0); >>>> #endif >>>> 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 ?
Well, you need to perform the rows and columns calculation regardless of the rotation feature, so the console_calc_rowcol() should be there anyway. It is a "bonus" that the rotation code can use the same function (and it looks generic enough) for rows and columns calculation, so IMO, a cleaner solution would be just make it non static. > > 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 ? Well, since Nikita is working on refactoring the lcd.c stuff and already examined several aproaches, I think he can provide a better opinion on this. Nikita? > >>>> 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: >> 1) 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. Sounds good. > >> >> 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. Yep. This sounds even better. Does DT have an already defined property for this? Have you considered may be using an environment variable for this? -- Regards, Igor. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot