Re: [U-Boot] [PATCH] Add suport for the graphic controller included in the JADE SOC

Dear Matthias,

Thanks for this patch! Please see some comments/issues below which we should resolve before committing the driver.

Matthias Weisser wrote:

Signed-off-by: Matthias Weisser matthias.weisser@graf-syteco.de

drivers/video/jadegdc.c | 205 +++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 205 insertions(+), 0 deletions(-) create mode 100755 drivers/video/jadegdc.c

diff --git a/drivers/video/jadegdc.c b/drivers/video/jadegdc.c new file mode 100755 index 0000000..88b71b7 --- /dev/null +++ b/drivers/video/jadegdc.c @@ -0,0 +1,205 @@ +/*

• • (C) Copyright 2007
• • DENX Software Engineering, Anatolij Gustschin, agust@denx.de

Please use your copyright here, as this driver for JADE GDC is mostly your work.

• • See file CREDITS for list of people who contributed to this
• • project.
• • This program is free software; you can redistribute it and/or
• • modify it under the terms of the GNU General Public License as
• • published by the Free Software Foundation; either version 2 of
• • the License, or (at your option) any later version.
• • This program is distributed in the hope that it will be useful,
• • but WITHOUT ANY WARRANTY; without even the implied warranty of
• • MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the

-----------------------------------------------------------^^^^^ please replace tab by space here.

• • GNU General Public License for more details.
• • You should have received a copy of the GNU General Public License
• • along with this program; if not, write to the Free Software
• • Foundation, Inc., 59 Temple Place, Suite 330, Boston,
• • MA 02111-1307 USA
• */

+/*

• • jade.c - Graphic interface for Fujitsu Jade integrated graphic

------------------------------------------------------------------^^^^^ and remove trailing white space here.

• • controller. Derived from mb862xx.c

same here, trailing white space in the line above, please remove.

• */

+#include <common.h>

+#if defined(CONFIG_VIDEO_JADEGDC)

drop this ifdef and add "COBJS-$(CONFIG_VIDEO_JADEGDC) += jadegdc.o" line to drivers/video/Makefile instead.

+#include <malloc.h> +#include <asm/io.h> +#include <video_fb.h> +#include "videomodes.h"

+#if defined(CONFIG_POST) +#include <post.h> +#endif

I think, this CONFIG_POST ifdef is not needed here, please remove it. Or am I missing something?

+/*

• • 4MB (at the end of system RAM)
• */

+#define VIDEO_MEM_SIZE 0x400000

+#define GDC_HOST_BASE 0xF1FC0000 +#define GDC_DSP0_BASE 0xF1FD0000 +#define GDC_DSP1_BASE 0xF1FD2000

+/*

• • Graphic Device
• */

+GraphicDevice jadegdc;

+void *video_hw_init (void) +{

• GraphicDevice *pGD = (GraphicDevice *)&jadegdc;
• struct ctfb_res_modes var_mode[2];
• unsigned long * vid;
• unsigned long div;
• unsigned long dspBase[2];
• char *penv;
• int bpp;
• int i, j;

----^^^ please remove tab in the line above.

• memset (pGD, 0, sizeof (GraphicDevice));

same here, remove tab.

• dspBase[0] = GDC_DSP0_BASE;
• dspBase[1] = GDC_DSP1_BASE;

----^^^ same here.

• /* Preliminary init of the onboard graphic controller,

•   retrieve base address */
  

• if ((pGD->frameAdrs = GDC_HOST_BASE) == 0) {

• printf ("Controller not found!\n");
  

• return (NULL);
  

• }

please replace above 4 lines by

pGD->frameAdrs = GDC_HOST_BASE;

and for multi line comments we now use following style

/* * Multi line * comment */

please use it here too.

• pGD->gdfIndex = GDF_15BIT_555RGB;
• pGD->gdfBytesPP = 2;
• pGD->memSize = VIDEO_MEM_SIZE;
• pGD->frameAdrs = PHYS_SDRAM + PHYS_SDRAM_SIZE - VIDEO_MEM_SIZE;
• vid = (unsigned long *)pGD->frameAdrs;

remove tabs in empty lines above, please.

• for(i = 0; i < 2; i++)
• {

please use the following coding style:

for (i = 0; i < 2; i++) {

• char varName[32];
  

• u32 dcm1, dcm2, dcm3;
  

• u16 htp, hdp, hdb, hsp, vtr, vsp, vdp;
  

• u8 hsw, vsw;
  

• u32 l2m, l2em, l2oa0, l2da0, l2oa1, l2da1;
  

• u16 l2dx, l2dy, l2wx, l2wy, l2ww, l2wh;
  

-----^^^^^^^^^^^^ tabs again, please drop.

• sprintf(varName, "gs_dsp_%d_param", i);
  

please add a space between function name and open parenthesis as you seem to use this style in other places too. Use either

function (...); or function(...);

style throughout the file an do not intermix.

• if(NULL == (penv = getenv (varName)))
  

• 	if(NULL == (penv = getenv ("videomode")))
  

• 		continue;
  

please add a space between if and open parenthesis and remove tabs in the line above if statement.

• bpp = 0;
  

• bpp = video_get_params (&var_mode[i], penv);
  

------^^^ please remove.

• if(0 == bpp){
  

if (bpp == 0) {

• 	var_mode[i].xres = 640;
  

• 	var_mode[i].yres = 480;
  

• 	var_mode[i].pixclock = 39721; /* 25MHz */
  

• 	var_mode[i].left_margin = 48;
  

• 	var_mode[i].right_margin = 16;
  

• 	var_mode[i].upper_margin = 33;
  

• 	var_mode[i].lower_margin = 10;
  

• 	var_mode[i].hsync_len = 96;
  

• 	var_mode[i].vsync_len = 2;
  

• 	var_mode[i].sync = 0;
  

• 	var_mode[i].vmode = 0;
  

• }
  

------^^^^^ please remove.

• for(j = 0; j < var_mode[i].xres * var_mode[i].yres / 2; j++)
  

• {
  

• 	*vid++ = 0xFFFFFFFF;
  

• }
  

fix the coding style here, please:

for (...) one line statement;

for (...) { statement1; statement2; ... }

tabs here, too, please check for tabs in empty lines throughout the file and remove them.

• pGD->winSizeX = var_mode[i].xres;
  

• pGD->winSizeY = var_mode[i].yres;
  

• /* LCD base clock is ~ 660MHZ. We do calculations in kHz */
  

• div = 660000 / (1000000000L / var_mode[i].pixclock);
  

• if(div > 64)
  

• 	div = 64;
  

please add space between if and open parenthesis.

• dcm1 = div << 8;
  

• dcm2 = 0x00000000;
  

• dcm3 = 0x00000000;
  

• htp = var_mode[i].left_margin + var_mode[i].xres + var_mode[i].hsync_len + var_mode[i].right_margin;
  

line to long, max. 80 chars allowed.

• hdp = var_mode[i].xres;
  

• hdb = var_mode[i].xres;
  

• hsp = var_mode[i].xres + var_mode[i].right_margin;
  

• hsw = var_mode[i].hsync_len;
  

• vsw = var_mode[i].vsync_len;
  

• vtr = var_mode[i].upper_margin + var_mode[i].yres + var_mode[i].vsync_len + var_mode[i].lower_margin;
  

here too.

• vsp = var_mode[i].yres + var_mode[i].lower_margin;
  

• vdp = var_mode[i].yres; 
  

remove trailing white space in the line above.

• l2m =	(       (var_mode[i].yres-1) << ( 0)) |
  

• 		(((var_mode[i].xres * 2)/64) << (16)) |
  

• 		(                        (1) << (31));
  

• l2em =	(1<<0) | (1 <<1);
  

please add spaces around "<<", "-", "/" in the lines above. Also use this coding style elsewhere in the file.

• l2oa0 = pGD->frameAdrs;
  

• l2da0 = pGD->frameAdrs;
  

• l2oa1 = pGD->frameAdrs;
  

• l2da1 = pGD->frameAdrs;
  

• l2dx = 0; 
  

• l2dy = 0; 
  

• l2wx = 0; 
  

• l2wy = 0; 
  

trailing white space in 4 lines above, please remove.

• l2ww = var_mode[i].xres;
  

• l2wh = var_mode[i].yres - 1;
  

• writel(dcm1, dspBase[i] + 0x100);
  

• writel(dcm2, dspBase[i] + 0x104);
  

• writel(dcm3, dspBase[i] + 0x108);
  

• writew(htp, dspBase[i] + 0x006); 
  

trailing white space in the line above.

• writew(hdp, dspBase[i] + 0x008);
  

• writew(hdb, dspBase[i] + 0x00A);
  

• writew(hsp, dspBase[i] + 0x00C);
  

• writeb(hsw, dspBase[i] + 0x00E);
  

• writeb(vsw, dspBase[i] + 0x00F);
  

• writew(vtr, dspBase[i] + 0x012);
  

• writew(vsp, dspBase[i] + 0x014);
  

• writew(vdp, dspBase[i] + 0x016);
  

• writel(  l2m, dspBase[i] + 0x040);
  

• writel( l2em, dspBase[i] + 0x130);
  

• writel(l2oa0, dspBase[i] + 0x044);
  

• writel(l2da0, dspBase[i] + 0x048);
  

• writel(l2oa1, dspBase[i] + 0x04C);
  

• writel(l2da1, dspBase[i] + 0x050);
  

• writew( l2dx, dspBase[i] + 0x054);
  

• writew( l2dy, dspBase[i] + 0x056);
  

• writew( l2wx, dspBase[i] + 0x134);
  

• writew( l2wy, dspBase[i] + 0x136);
  

• writew( l2ww, dspBase[i] + 0x138);
  

• writew( l2wh, dspBase[i] + 0x13A);
  

• writel(dcm1 | (1<<18) | (1<<31), dspBase[i] + 0x100);
  

• }
• return pGD;

+}

+/*

• • Set a RGB color in the LUT
• */

+void video_set_lut (unsigned int index, unsigned char r, unsigned char g, unsigned char b)

line too long, please fix.

+{

+}

+#endif /* CONFIG_VIDEO_JADEGDC */

We also should use macros for register offsets, I think. Can we coordinate efforts for fixing this in mb862xx driver also? I know that mb862xx driver has similar style issues and I'm willing to fix them soon. For new patches the policy is to resolve issues before inclusion.

Thanks, Anatolij