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.weis...@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, ag...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
