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

[Anatolij Gustschin <agust@denx.de>]

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