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
