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

[Anatolij Gustschin <agust@denx.de>]

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 at 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