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

[Hannes Petermaier]

"U-Boot" <u-boot-bounces at lists.denx.de> schrieb am 18.03.2015 13:56:49:

> 
> Hi Hannes,
Hi Igor,
thanks for response - we come closer to the final solution :-)

> 
> > +}
> > +
> > +void __weak lcd_init_console_rot(struct console_t *pcons)
> > +{
> > +   return;
> > +}
> > +
> > +void lcd_init_console(void *address, int vl_cols, int vl_rows, int 
vl_rot)
> > +{
> > +   memset(&cons, 0, sizeof(cons));
> > +   cons.lcd_address = address;
> > +
> > +   cons.lcdsizex = vl_cols;
> > +   cons.lcdsizey = vl_rows;
> > +   cons.lcdrot = vl_rot;
> > +
> > +   cons.fp_putc_xy = &lcd_putc_xy0;
> > +   cons.fp_console_moverow = &console_moverow0;
> > +   cons.fp_console_setrow = &console_setrow0;
> > +   console_calc_rowcol(&cons);
> 
> I think the above four lines is exactly what should be placed in the
> __weak variant of lcd_init_console_rot() function (the one just above
> this one).
I think not so.
If the lcd_console_rotation.c is compiled in, the __weak function isn't 
called anymore.
And if user wants to have 0 degree rotation, the init-function from 
lcd_console_rotation.c doesn't anything.
Therefore it is necessary to initialize these pointers here.

> > +
> > +   lcd_init_console_rot(&cons);
> > +
> > +   debug("lcd_console: have %d/%d col/rws on scr %dx%d (%d deg 
rotated)\n",
> > +         cons.cols, cons.rows, cons.lcdsizex, cons.lcdsizey, vl_rot);
> > +
> 
> no need for the empty line here.
Will be changed in v3.

> > +
> > +#include <common.h>
> > +#include <lcd.h>
> > +#include <video_font.h>      /* Get font data, width and height */
> > +
> > +#if LCD_BPP == LCD_COLOR16
> > +   #define fbptr_t ushort
> > +#elif LCD_BPP == LCD_COLOR32
> > +   #define fbptr_t u32
> > +#else
> > +   #define fbptr_t uchar
> > +#endif
> 
> That is a duplication of the code in lcd_console.c.
> If we can get rid of these size games, probably we should have in the 
lcd.h,
> or lcd_console.h, or ...
It might be possible to move this into lcd.h, in every case it is 
necesarry that common.h
is included before - from here the information about LCD_BPP is coming.
I will have a 2nd look to that to avoid this duplication.

> > +
> > +void lcd_init_console_rot(struct console_t *pcons)
> > +{
> > +   if (pcons->lcdrot == 0) {
> > +      return;
> > +   } else if (pcons->lcdrot == 1) {
> > +      pcons->fp_putc_xy = &lcd_putc_xy90;
> > +      pcons->fp_console_moverow = &console_moverow90;
> > +      pcons->fp_console_setrow = &console_setrow90;
> > +   } else if (pcons->lcdrot == 2) {
> > +      pcons->fp_putc_xy = &lcd_putc_xy180;
> > +      pcons->fp_console_moverow = &console_moverow180;
> > +      pcons->fp_console_setrow = &console_setrow180;
> > +   } else if (pcons->lcdrot == 3) {
> > +      pcons->fp_putc_xy = &lcd_putc_xy270;
> > +      pcons->fp_console_moverow = &console_moverow270;
> > +      pcons->fp_console_setrow = &console_setrow270;
> > +   } else {
> > +      puts("lcd_init_console_rot: invalid framebuffer rotation!\n");
> 
> How about
> printf("%s: invalid framebuffer rotation!\n", __func__);
> ?
Okay, i will change that.
Sometime ago, somebody told me on the mailing list that i should prefer 
the puts function if i don't have to print out some values.
If we want to use the printf we can also printout the given rotation 
pcons->lcdrot.

> -- 
> Regards,
> Igor.
best regards,
Hannes