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

Jens Scharsig wrote:

This patch adds a new video driver

• adds common bus_vcxk framebuffer driver

Signed-off-by: Jens Scharsig esw@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@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@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