[U-Boot] [PATCH 1/2] new video driver for bus vcxk framebuffers

[Anatolij Gustschin]

Jens Scharsig wrote:
> This patch adds a new video driver
> 
> * adds common bus_vcxk framebuffer driver 
> 
> Signed-off-by: Jens Scharsig <esw at 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 <esw at 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, <esw at 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