Jens Scharsig wrote: > This patch adds a new video driver > > * adds common bus_vcxk framebuffer driver > > Signed-off-by: Jens Scharsig <e...@bus-elektronik.de> > ---
Sorry it took so long to review/comment. We need to resolve some issues before this patch can be applied, then it can go in for coming release. > diff --git a/doc/README.bus_vcxk b/doc/README.bus_vcxk > new file mode 100644 > index 0000000..44e1238 > --- /dev/null > +++ b/doc/README.bus_vcxk > @@ -0,0 +1,95 @@ > +/* > + * (C) Copyright 2008-2009 > + * BuS Elektronik GmbH & Co. KG <www.bus-elektronik.de> > + * Jens Scharsig <e...@bus-elektronik.de> > + * > + * Configuation settings for the EB+CPUx9K2 board. Typo in Configuration, please fix. > + * > + * 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 > + * 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 > + */ > + > +U-Boot vcxk video controller driver > +====================================== > + > +The driver can use with VC2K, VC4K and VC8K devices on following boards: > + > +board | ARCH | Vendor > +----------------------------------------------------------------------- > +EB+CPU5282-T1 | MCF5282 | BuS Elektronik GmbH & Co. KG > +EB+MCF-EVB123 | MCF5282 | BuS Elektronik GmbH & Co. KG > +EB+CPUx9K2 | AT91RM9200 | BuS Elektronik GmbH & Co. KG > +ZLSA | AT91RM9200 | Ruf Telematik AG Please fix indentation/alignment in lines above, so it will look like initially intended. > + > +by define CONFIG_VIDEO_VCXK I suggest to move this to the beginning of the sentence, e.g.: "By defining CONFIG_VIDEO_VCXK this driver can be used with VC2K, VC4K ..." > + > +Driver configuration > +-------------------- > + > +The driver needs some defines to descrip the target hardware: s/descrip/describe > + > +CONFIG_SYS_VCXK_BASE > + > +base address of VCxK hardware memory > + > +CONFIG_SYS_VCXK_DEFAULT_LINEALIGN > + > +defines the physical alignment of a pixel row > + > +CONFIG_SYS_VCXK_DOUBLEBUFFERED > + > +some boards that use vcxk prevent read from framebuffer memory. > +define this option to enable double buffering (needs 16KiB RAM) It is more readable if you indent the lines describing the CONFIG_SYS_ option by tab, please fix, also in appropriate lines below, e.g.: CONFIG_SYS_VCXK_BASE base address of VCxK hardware memory CONFIG_SYS_VCXK_DEFAULT_LINEALIGN defines the physical alignment of a pixel row and so on. > + > +CONFIG_SYS_VCXK_ACKNOWLEDGE_INIT > + > +initialize the acknowledge line from vcxk hardware > + > +#define CONFIG_SYS_VCXK_ACKNOWLEDGE please remove "#define" before CONFIG_SYS_VCXK_ACKNOWLEDGE > + > +should return true (1), if vcxk hardware acknowledges a viewing reqest please fix a typo, s/reqest/request > + > +CONFIG_SYS_VCXK_ENABLE_INIT > + > +initialize the enable line to vcxk hardware > + > +CONFIG_SYS_VCXK_DISABLE > + > +set vcxk enable line to disable level / display off > + > +CONFIG_SYS_VCXK_ENABLE > + > +set vcxk enable line enable level / display on > + > +CONFIG_SYS_VCXK_REQUEST_INIT > + > +initialize the request line to vcxk hardware > + > +CONFIG_SYS_VCXK_REQUEST > + > +should be send an request impulse to vcxk hardware > + > +CONFIG_SYS_VCXK_INVERT_INIT > + > +should initialize the invert line to vcxk hardware and set it to > +non invers mode > + > +CONFIG_SYS_VCXK_RESET_INIT > + > +initialize the reset line to vcxk hardware and release it from reset Looking on the changes to board configuration file "include/configs/EB+MCF-EV123.h" in the second patch of this series I now realize that most of these CONFIG_SYS_ macros above expand to C code, so these are not configuration options any more. I tend to reject all this. CONFIG_SYS_ options are supposed to be options and not board specific code. VCxK.c driver you removed by this patch series did it in more correct way, I think. What about moving this code to functions which use board specific macros? These functions should be placed in this new video driver then. > + > diff --git a/drivers/video/Makefile b/drivers/video/Makefile > index bc00852..bb6b5a0 100644 > --- a/drivers/video/Makefile > +++ b/drivers/video/Makefile > @@ -36,6 +36,7 @@ COBJS-$(CONFIG_VIDEO_SED13806) += sed13806.o > COBJS-$(CONFIG_SED156X) += sed156x.o > COBJS-$(CONFIG_VIDEO_SM501) += sm501.o > COBJS-$(CONFIG_VIDEO_SMI_LYNXEM) += smiLynxEM.o > +COBJS-$(CONFIG_VIDEO_VCXK) += bus_vcxk.o > COBJS-y += videomodes.o > > COBJS := $(COBJS-y) > diff --git a/drivers/video/bus_vcxk.c b/drivers/video/bus_vcxk.c > new file mode 100644 > index 0000000..db49c83 > --- /dev/null > +++ b/drivers/video/bus_vcxk.c > @@ -0,0 +1,469 @@ > +/* > + * (C) Copyright 2005-2009 > + * Jens Scharsig @ BuS Elektronik GmbH & Co. KG, <e...@bus-elektronik.de> > + * > + * 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 > + * 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 > + */ > + > +#include <common.h> > +#include <bmp_layout.h> > +#include <asm/io.h> > + > +vu_char *vcxk_bws = ((vu_char *) (CONFIG_SYS_VCXK_BASE)); > +vu_short *vcxk_bws_word = ((vu_short *)(CONFIG_SYS_VCXK_BASE)); > +vu_long *vcxk_bws_long = ((vu_long *) (CONFIG_SYS_VCXK_BASE)); > + > +#ifdef CONFIG_AT91RM9200 > + #include <asm/arch/hardware.h> > + #ifndef VCBITMASK > + #define VCBITMASK(bitno) (0x0001 << (bitno % 16)) > + #endif > +#elif defined(CONFIG_MCF52x2) > + #include <asm/m5282.h> > + #ifndef VCBITMASK > + #define VCBITMASK(bitno) (0x8000 >> (bitno % 16)) > + #endif > +#else > + #error not vcxk support for selected ARCH s/not/no > +#endif > + > +#ifndef CONFIG_SYS_VCXK_DOUBLEBUFFERED > + #define VCXK_BWS(x,data) vcxk_bws[x] = data; > + #define VCXK_BWS_WORD_SET(x,mask) vcxk_bws_word[x] |= mask; > + #define VCXK_BWS_WORD_CLEAR(x,mask) vcxk_bws_word[x] &= ~mask; > + #define VCXK_BWS_LONG(x,data) vcxk_bws_long[x] = data; > +#else > + u_char double_bws[16384]; > + u_short *double_bws_word; > + u_long *double_bws_long; > + #define VCXK_BWS(x,data) \ > + double_bws[x] = data; vcxk_bws[x] = data; > + #define VCXK_BWS_WORD_SET(x,mask) \ > + double_bws_word[x] |= mask; vcxk_bws_word[x] = > double_bws_word[x]; line too long, please fix to max. 80 characters. > + #define VCXK_BWS_WORD_CLEAR(x,mask) \ > + double_bws_word[x] &= ~mask; vcxk_bws_word[x] = > double_bws_word[x]; line too long, too. > + #define VCXK_BWS_LONG(x,data) \ > + double_bws_long[x] = data; vcxk_bws_long[x] = data; > +#endif > + > +#define VC4K16_Bright1 vcxk_bws_word[0x20004 / 2] > +#define VC4K16_Bright2 vcxk_bws_word[0x20006 / 2] > +#define VC2K_Bright vcxk_bws[0x8000] please remove one tab in the above line. ... > + > +static vu_long vcxk_driver; > +vu_char VC4K16; > + > +u_long display_width; > +u_long display_height; > +u_long display_bwidth; > + > +ulong search_vcxk_driver(void); > +void vcxk_cls(void); > +void vcxk_setbrightness(unsigned int side, short brightness); > +int vcxk_request(void); > +int vcxk_acknowledge_wait(void); > +void vcxk_clear(void); > + > +/*---------------------------------------------------------------------------- > + ****f* bus_vcxk/vcxk_init > + * FUNCTION > + * initialalize Video Controller > + * PARAMETERS > + * width visible display width in pixel > + * height visible display height in pixel > + *** > +----------------------------------------------------------------------------*/ please use this style for multi-line comments: /* * multi-line * comment */ > + > +int vcxk_init(unsigned long width, unsigned long height) > +{ > + CONFIG_SYS_VCXK_RESET_INIT; > + > +#ifdef CONFIG_SYS_VCXK_DOUBLEBUFFERED > + double_bws_word = (u_short *) double_bws; > + double_bws_long = (u_long *)double_bws; > + debug("%lx %lx %lx \n",double_bws,double_bws_word,double_bws_long); > +#endif > + > + display_width = width; > + display_height = height; > + #if (CONFIG_SYS_VCXK_DEFAULT_LINEALIGN==4) > + display_bwidth =((width+31) / 8) & ~0x3; > + #elif (CONFIG_SYS_VCXK_DEFAULT_LINEALIGN==2) > + display_bwidth =((width+15) / 8) & ~0x1; > + #else > + #error CONFIG_SYS_VCXK_DEFAULT_LINEALIGN is invalid > + #endif do not indent these 7 lines above please. Also add spaces around "+". > + debug("linesize ((%d + 15)/8 & ~0x1) = > %d\n",display_width,display_bwidth); line too long. Add spaces around "/", before display_width and display_bwidth please. > + > + #ifdef CONFIG_SYS_VCXK_AUTODETECT > + VC4K16 = 0; > + vcxk_bws_long[1] = 0x0; > + vcxk_bws_long[1] = 0x55AAAA55; > + vcxk_bws_long[5] = 0x0; > + if (vcxk_bws_long[1] == 0x55AAAA55) > + { > + VC4K16 = 1; > + } no braces here for single line if statement, please: if (vcxk_bws_long[1] == 0x55AAAA55) VC4K16 = 1; Also do not indent here, too. > + #else > + VC4K16 = 1; > + debug("No autodetect: use vc4k\n"); > + #endif > + > + CONFIG_SYS_VCXK_INVERT_INIT; > + > + CONFIG_SYS_VCXK_REQUEST_INIT; > + > + CONFIG_SYS_VCXK_ACKNOWLEDGE_INIT; > + > + CONFIG_SYS_VCXK_DISABLE; > + CONFIG_SYS_VCXK_ENABLE_INIT; CONFIG_SYS_* expand to code, please place board dependent code here or in inline functions. > + > + vcxk_driver = search_vcxk_driver(); > + if (vcxk_driver) > + { > + /* use flash resist driver */ > + } > + else > + { > + vcxk_cls(); > + vcxk_cls(); > + } Use this style here: if (vcxk_driver) { /* use flash resident driver */ } else { vcxk_cls(); vcxk_cls(); } Are two vcxk_cls() calls really needed? If not, then remove. Also, will search_vcxk_driver() ever be extended to return not 0? If not, remove these checks from the driver. > + > + vcxk_setbrightness(3,1000); > + CONFIG_SYS_VCXK_ENABLE; CONFIG_SYS_VCXK_ENABLE expands to code, just place the code here or move to a function. > + return 1; > +} > + > +/*---------------------------------------------------------------------------- > + ****f* bus_vcxk/vcxk_setpixel > + * FUNCTION > + * set the pixel[x,y] with the given color > + * PARAMETER > + * x pixel colum > + * y pixel row > + * color <0x40 off/black > + * >0x40 on > + *** > +----------------------------------------------------------------------------*/ Fix multi-line comment style, please. > +void vcxk_setpixel (int x, int y, unsigned long color) > +{ > + vu_short dataptr; > + > + if (vcxk_driver) > + { > + /* use flash resist driver */ > + } > + else > + { > + if ((x<display_width) && (y<display_height)) > + { > + > + dataptr = ((x / 16)) + (y * (display_bwidth>>1)); > + > + color = ((color >> 16) & 0xFF) | > + ((color >> 8) & 0xFF) | (color & 0xFF); > + > + if (color > 0x40) > + { > + VCXK_BWS_WORD_SET(dataptr,VCBITMASK(x)); > + } > + else > + { > + VCXK_BWS_WORD_CLEAR(dataptr,VCBITMASK(x)); > + } > + } > + } > +} Check for coding style issues here, too. E.g.: if statements, spaces around "<", ">", ">>", etc. Fix it anywhere in the code, please. Best regards, Anatolij _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot