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

[Hannes Petermaier]

Nikita Kiryanov <nikita at compulab.co.il> schrieb am 15.03.2015 19:56:31:

> 
> Hi Hannes,
Hi Nikita,
many thanks for response.
> 
> I second Grinberg's suggestion of a separate file and 0 degree default 
(also as a
> fallback for invalid rotation value, see below).
Okay - i will provide a V2 from this patch where i try to implement as 
sugessted, maybe we will need some V3 :-)
Probably on wednesday.

> Some additional comments:
> 
> > +
> > +      If CONFIG_LCD_ROTATION is not defined, the console will be
> > +      initialized with 0degree rotation
> 
> This is enough. "the screen behaves like the days before" is vague and 
unnecessary
> (days before what?)
Okay - i will do so.

> 
> > +static inline void console_setrow0(u32 row, int clr)
> > +{
> >      int i;
> > +   uchar *dst = (uchar *)(cons.lcd_address +
> > +                row * VIDEO_FONT_HEIGHT *
> > +                cons.lcdsizex * PIXLBYTES);
> >
> > -   dest = (uchar *)(cons.lcd_address +
> > -          y * lcd_line_length + x * NBITS(LCD_BPP) / 8);
> > +   fbptr_t *d = (fbptr_t *)dst;
> 
> Here you can just create the fbptr variable directly. You have a bunch 
of
> function where this recasting is avoidable.
> 
> > +   for (i = 0; i < (VIDEO_FONT_HEIGHT * cons.lcdsizex); i++)
> > +      *d++ = clr;
> > +}

> 
> > +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;
> > +
> > +   if (vl_rot == 0) {
> > +      cons.fp_putc_xy = &lcd_putc_xy0;
> > +      cons.fp_console_moverow = &console_moverow0;
> > +      cons.fp_console_setrow = &console_setrow0;
> > +      console_calc_rowcol(vl_cols, vl_rows, &cons.cols, &cons.rows);
> > +#ifdef CONFIG_LCD_ROTATION
> > +   } else if (vl_rot == 90) {
> > +      cons.fp_putc_xy = &lcd_putc_xy90;
> > +      cons.fp_console_moverow = &console_moverow90;
> > +      cons.fp_console_setrow = &console_setrow90;
> > +      console_calc_rowcol(vl_rows, vl_cols, &cons.cols, &cons.rows);
> > +   } else if (vl_rot == 180) {
> > +      cons.fp_putc_xy = &lcd_putc_xy180;
> > +      cons.fp_console_moverow = &console_moverow180;
> > +      cons.fp_console_setrow = &console_setrow180;
> > +      console_calc_rowcol(vl_cols, vl_rows, &cons.cols, &cons.rows);
> > +   } else if (vl_rot == 270) {
> > +      cons.fp_putc_xy = &lcd_putc_xy270;
> > +      cons.fp_console_moverow = &console_moverow270;
> > +      cons.fp_console_setrow = &console_setrow270;
> > +      console_calc_rowcol(vl_rows, vl_cols, &cons.cols, &cons.rows);
> > +#endif
> > +   } else {
> > +      puts("lcd_init_console: invalid framebuffer rotation!\n");
> 
> This case leaves the function pointers uninitialized, which would crash 
the 
> system later on.
> I suggest you default to 0 degree rotation both for the generic case and 
for 
> the fallback behavior
> (with the warning message where necessary).
Oh my god, i haven't seen this mess ... i will remove it during moving 
lcd_rotation stuff
into separate file.

> >   void lcd_putc(const char c)
> >   {
> >      if (!lcd_is_enabled) {
> >         serial_putc(c);
> > -
> 
> This is a cleanup. It should not be in this patch.
I will make this cleanup in a following patch, after this series has been 
merged.


> > --- a/include/lcd_console.h
> > +++ b/include/lcd_console.h
> > @@ -16,11 +16,12 @@
> >    * console has.
> >    *
> >    * @address: Console base address
> > - * @rows: Number of rows in the console
> > - * @cols: Number of columns in the console
> > + * @vl_rows: Number of rows in the console
> > + * @vl_cols: Number of columns in the console
> > + * @vl_rot: Rotation of display in degree (0 - 90 - 180 - 270) 
counterlockwise
> >    */
> > -void lcd_init_console(void *address, int rows, int cols);
> > -
> > +/*void lcd_init_console(void *address, int rows, int cols); */
> 
> Please delete this comment
Okay.

> -- 
> Regards,
> Nikita Kiryanov
best regards,
Hannes