[U-Boot] [PATCH] video: Add new driver for Silicon Motion SM501/SM502 Part 1/2

Anatolij Gustschin
2008-12-08T13:03:14Z

Hello Stefan,

Stefan Althoefer wrote:
> Dear All,
>
>> Dear Anatolij & Stefan,
>>
>> In message <493AD29C.80409@denx.de> you wrote:
>>>> Use CONFIG_VIDEO_SM501NEW to enable the driver.
>>> not sure if CONFIG_VIDEO_SM501NEW is a good chose here. Maybe
>>> we should use s.th. like CONFIG_VIDEO_SM50x. This applies to
>>> the file names too: sm50x.h, sm50x.c, etc. Even better would
>>> be a merge with the existing driver.
>> I agree with Anatolij here. A merge would definitely be best.
>
> Before I start to implement the good suggestions, I'd like
> to know how such a merge would look like in your opinion.

see inlined patch below how it could be done without removing
old sm501 code. Defining CONFIG_VIDEO_SM501_PCI enables using
new sm501 driver code.

> Over the long term, I see the old driver vanishing, as it is too
> complicated to use (you need to be a video and SMI register
> expert). It is not much more than a jump into the board specific
> setup routines. However, removing the old driver would break half
> a dozen of boards (inacceptable).

we do not have to remove old code. Everyone who doesn't
additionally define CONFIG_VIDEO_SM501_PCI will be able to
use it. The advantage of the old code is that it is small.
This is fundamental design principle #1 of U-Boot:
http://www.denx.de/wiki/view/U-Boot/DesignPrinciples#1_Keep_it_Small

I agree that the old sm501 code is suboptimal and could be
improved.

diff --git a/drivers/video/sm501.c b/drivers/video/sm501.c
index 283d2d9..20a5335 100644
--- a/drivers/video/sm501.c
+++ b/drivers/video/sm501.c
@@ -33,30 +33,35 @@

#include <video_fb.h>
#include <sm501.h>
-
-#define read8(ptrReg) \
- *(volatile unsigned char *)(sm501.isaBase + ptrReg)
-
-#define write8(ptrReg,value) \
- *(volatile unsigned char *)(sm501.isaBase + ptrReg) = value
-
-#define read16(ptrReg) \
- (*(volatile unsigned short *)(sm501.isaBase + ptrReg))
-
-#define write16(ptrReg,value) \
- (*(volatile unsigned short *)(sm501.isaBase + ptrReg) = value)
-
-#define read32(ptrReg) \
- (*(volatile unsigned int *)(sm501.isaBase + ptrReg))
-
-#define write32(ptrReg, value) \
- (*(volatile unsigned int *)(sm501.isaBase + ptrReg) = value)
+#ifdef CONFIG_VIDEO_SM501_PCI
+#include <pci.h>
+#include "videomodes.h"
+#include "sm50x-regs.h"
+#include "sm50x.h"
+#endif

GraphicDevice sm501;

-/*-----------------------------------------------------------------------------
- * SmiSetRegs --
- *-----------------------------------------------------------------------------
+#ifdef CONFIG_VIDEO_SM501_PCI
+/*
+ * code used in sm501_pci_init()
+ * comes here.
+ */
+
+static int sm501_pci_init(void)
+{
+ GraphicDevice *pGD = (GraphicDevice *)&sm501;
+ /*
+ * Code to find the pci devide, setup video mode and
+ * init sm501 struct comes here.
+ * return 0 on success, non-zero otherwise.
+ * This is mainly the code you currently have
+ * in video_hw_init() in sm501new.c file.
+ */
+}
+#else
+/*
+ * SmiSetRegs
*/
static void SmiSetRegs (void)
{
@@ -66,7 +71,7 @@ static void SmiSetRegs (void)
*/
const SMI_REGS *preg = board_get_regs ();
while (preg->Index) {
- write32 (preg->Index, preg->Value);
+ writel (preg->Value, sm501.isaBase + preg->Index);
/*
* Insert a delay between
*/
@@ -74,10 +79,10 @@ static void SmiSetRegs (void)
preg ++;
}
}
+#endif

-/*-----------------------------------------------------------------------------
- * video_hw_init --
- *-----------------------------------------------------------------------------
+/*
+ * video_hw_init
*/
void *video_hw_init (void)
{
@@ -85,6 +90,7 @@ void *video_hw_init (void)

memset (&sm501, 0, sizeof (GraphicDevice));

+#ifndef CONFIG_VIDEO_SM501_PCI
/*
* Initialization of the access to the graphic chipset Retreive base
* address of the chipset (see board/RPXClassic/eccx.c)
@@ -122,6 +128,10 @@ void *video_hw_init (void)

/* (see board/RPXClassic/RPXClassic.c) */
board_validate_screen (sm501.isaBase);
+#else
+ if (!sm501_pci_init())
+ return 0;
+#endif /* CONFIG_VIDEO_SM501_PCI */

/* Clear video memory */
i = sm501.memSize/4;
@@ -132,9 +142,8 @@ void *video_hw_init (void)
return (&sm501);
}

-/*-----------------------------------------------------------------------------
- * video_set_lut --
- *-----------------------------------------------------------------------------
+/*
+ * video_set_lut
*/
void video_set_lut (
unsigned int index, /* color number */
--

Better suggestions are always welcome.

Best regards,
Anatolij

DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office at denx.de