[PATCH]new driver for siliconmotion

Matt Turner mattst88 at gmail.com
Tue Jul 10 17:24:51 PDT 2012


I applaud the effort and am thankful that Silicon Motion is
contributing to the driver.

I actually own a 712, so I feel like I should at least take a look at
this patch. Unfortunately, after a quick glance, I'm not very
impressed.

Generally, I wonder why you haven't been working on the upstream
driver to begin with? It's really hard to review a huge attached patch
that is obviously an accumulation of a few years of work.

Clearly rather difficult now, but this kind of work should be split
into many different patches.

On Mon, Jul 9, 2012 at 10:58 PM, Aaron.Chen  陈俊杰
<aaron.chen at siliconmotion.com> wrote:
> From bda2e699bfee84e1266f28da53a6debe432db96b Mon Sep 17 00:00:00 2001
>
> From: root <root at aaron-VirtualBox.(none)>

Set your git name and email address.

> Date: Tue, 10 Jul 2012 09:41:13 +0800
>
> Subject: [PATCH] New driver v4.0.6
>
>
>
> [patch]New driver works for SM712/722/502/750/718/750LE. This is the first
> submission.
>

Need a more descriptive subject and commit message.

>
> Signed-off-by:Aaron Chen<aaron.chen at siliconmotion.com.cn>
>
> ---
>
> Makefile.am                     |   11 +-
>
> QA                              |   35 +
>
> README                          |   34 +-

You're clobbering the standard X.Org README file with build and
installation instructions. Don't do this.

I highly doubt we need build and install instructions, but if so they
should go elsewhere (INSTALL, perhaps?).

> Release.txt                     |  381 +--
>
> configure.ac                    |   81 +-

There are a lot of useless changes in configure.ac, like lowering the
AC_PREREQ number and changing the bugzilla link. Also, the version
number in configure.ac is wrong.

> man/Makefile.am                 |   64 +-

You replaced the Oracle copyright notice with the old Sun copyright
notice. Clearly wrong, and it makes it clear that this wasn't well
reviewed or rebased.

> src/CALLMAP                     |   22 +

This is a useless file that was probably removed from git a long time
ago. Don't add it back.

> src/Imakefile                   |  116 +

WTF? No. Just no.

I see that the driver has RANDR support. No version of the X server
ever had an Imake build system and also RANDR support. Remove this
Imakefile.

> src/Makefile.am                 |   71 +-
>
> src/ddk502/502ddk_module.c      |   43 +
>
> src/ddk502/Makefile.am          |   38 +
>
> src/ddk502/ddk502_chip.c        |  348 +++
>
> src/ddk502/ddk502_chip.h        |  127 +
>
> src/ddk502/ddk502_clock.c       |  603 ++++
>
> src/ddk502/ddk502_clock.h       |  119 +
>
> src/ddk502/ddk502_ddkdebug.c    |  241 ++
>
> src/ddk502/ddk502_ddkdebug.h    |  154 +
>
> src/ddk502/ddk502_display.c     |  414 +++
>
> src/ddk502/ddk502_display.h     |   95 +
>
> src/ddk502/ddk502_hardware.c    |  458 +++
>
> src/ddk502/ddk502_hardware.h    |   93 +
>
> src/ddk502/ddk502_help.c        |   47 +
>
> src/ddk502/ddk502_help.h        |   29 +
>
> src/ddk502/ddk502_linux.c       |  407 +++
>
> src/ddk502/ddk502_mode.c        |  746 +++++
>
> src/ddk502/ddk502_mode.h        |  157 +
>
> src/ddk502/ddk502_os.c          |   26 +
>
> src/ddk502/ddk502_os.h          |  362 +++
>
> src/ddk502/ddk502_power.c       |  487 +++
>
> src/ddk502/ddk502_power.h       |  126 +
>
> src/ddk502/ddk502_regdc.h       |  769 +++++
>
> src/ddk502/ddk502_regdma.h      |   69 +
>
> src/ddk502/ddk502_reggpio.h     |  317 ++
>
> src/ddk502/ddk502_regsc.h       | 1233 ++++++++
>
> src/ddk502/ddk502_regzv.h       |  275 ++
>
> src/ddk502/ddk502_swi2c.c       |  551 ++++
>
> src/ddk502/ddk502_swi2c.h       |   39 +
>
> src/ddk502/ddk502_voyager.h     |   94 +
>
> src/ddk502/version.h            |   25 +
>
> src/ddk712/712ddk_module.c      |   44 +
>
> src/ddk712/Makefile.am          |   19 +
>
> src/ddk712/ddk712.h             |   20 +
>
> src/ddk712/ddk712_chip.c        |  163 +
>
> src/ddk712/ddk712_chip.h        |   52 +
>
> src/ddk712/ddk712_help.c        |   29 +
>
> src/ddk712/ddk712_help.h        |  100 +
>
> src/ddk712/ddk712_mode.c        |  260 ++
>
> src/ddk712/ddk712_mode.h        |   22 +
>
> src/ddk712/ddk712_reg.h         |   14 +
>
> src/ddk712/version.h            |   25 +
>
> src/ddk750/750ddk_module.c      |   43 +
>
> src/ddk750/Makefile.am          |   32 +
>
> src/ddk750/ddk750.h             |   24 +
>
> src/ddk750/ddk750_chip.c        |  657 ++++
>
> src/ddk750/ddk750_chip.h        |   84 +
>
> src/ddk750/ddk750_display.c     |  350 +++
>
> src/ddk750/ddk750_display.h     |  177 ++
>
> src/ddk750/ddk750_dvi.c         |   98 +
>
> src/ddk750/ddk750_dvi.h         |   67 +
>
> src/ddk750/ddk750_edid.c        | 1940 ++++++++++++
>
> src/ddk750/ddk750_edid.h        | 1083 +++++++
>
> src/ddk750/ddk750_help.c        |   51 +
>
> src/ddk750/ddk750_help.h        |   32 +
>
> src/ddk750/ddk750_hwi2c.c       |  285 ++
>
> src/ddk750/ddk750_hwi2c.h       |   17 +
>
> src/ddk750/ddk750_mode.c        |  219 ++
>
> src/ddk750/ddk750_mode.h        |   43 +
>
> src/ddk750/ddk750_power.c       |  240 ++
>
> src/ddk750/ddk750_power.h       |   72 +
>
> src/ddk750/ddk750_reg.h         | 2597 ++++++++++++++++
>
> src/ddk750/ddk750_sii164.c      |  423 +++
>
> src/ddk750/ddk750_sii164.h      |  170 +
>
> src/ddk750/ddk750_swi2c.c       |  592 ++++
>
> src/ddk750/ddk750_swi2c.h       |   98 +
>
> src/ddk750/version.h            |   25 +
>
> src/drv502/smi_502_crtc.c       |  729 +++++
>
> src/drv502/smi_502_driver.c     |  813 +++++
>
> src/drv502/smi_502_driver.h     |  379 +++
>
> src/drv502/smi_502_hw.c         |  139 +
>
> src/drv502/smi_502_hw.h         |   29 +
>
> src/drv502/smi_502_output.c     |  486 +++
>
> src/drv712/smi_712_crtc.c       | 1540 +++++++++
>
> src/drv712/smi_712_driver.c     |  565 ++++
>
> src/drv712/smi_712_driver.h     |   98 +
>
> src/drv712/smi_712_hw.c         |  558 ++++
>
> src/drv712/smi_712_hw.h         |  144 +
>
> src/drv712/smi_712_output.c     |  698 +++++
>
> src/drv750/smi_750_crtc.c       |  699 +++++
>
> src/drv750/smi_750_driver.c     |  636 ++++
>
> src/drv750/smi_750_driver.h     |   64 +
>
> src/drv750/smi_750_hw.c         |  237 ++
>
> src/drv750/smi_750_hw.h         |   54 +
>
> src/drv750/smi_750_output.c     |  406 +++
>
> src/drv750le/smi_750le_crtc.c   |  331 ++
>
> src/drv750le/smi_750le_driver.c |  652 ++++
>
> src/drv750le/smi_750le_driver.h |  782 +++++
>
> src/drv750le/smi_750le_hw.c     |  140 +
>
> src/drv750le/smi_750le_hw.h     |   46 +
>
> src/drv750le/smi_750le_output.c |  199 ++
>
> src/smi_accel.c                 | 1556 +++++++++-
>
> src/smi_accel.h                 |  174 ++
>
> src/smi_common.c                |   11 +
>
> src/smi_common.h                |  693 +++++
>
> src/smi_crtc.c                  |  269 +-
>
> src/smi_crtc.h                  |   33 +-
>
> src/smi_dbg.h                   |   30 +
>
> src/smi_driver.c                | 4047 ++++++++++++-------------
>
> src/smi_driver.h                |   77 +
>
> src/smi_output.c                |  197 +-
>
> src/smi_output.h                |   44 +
>
> src/smi_ver.h                   |   21 +
>
> src/smi_video.c                 | 6561
> ++++++++++++++++++++++++---------------
>
> src/smi_video.h                 |  281 ++-
>
> src/version.h                   |   28 +
>
> 114 files changed, 38463 insertions(+), 5457 deletions(-)

Do we really have to prefix all these files and directories with 'ddk'?

To actual code:

+/*
+ * This function returns frame buffer memory size in Byte units.
+ */
+//_X_EXPORT unsigned long getFrameBufSize()
+_X_EXPORT int ddk502_getFrameBufSize()
+{
+    //unsigned long sizeSymbol, memSize;
+    unsigned long sizeSymbol;
+    int memSize;
+
+    sizeSymbol = FIELD_GET(peekRegisterDWord(DRAM_CTRL), DRAM_CTRL, SIZE);
+
+    switch(sizeSymbol)
+    {
+        /* Default is set to the lowest memory setting (requested by
driver). */
+        default:
+        case DRAM_CTRL_SIZE_2:  memSize = MB(2);  break; /* 2  Mega byte */
+        case DRAM_CTRL_SIZE_4:  memSize = MB(4);  break; /* 4  Mega byte */
+        case DRAM_CTRL_SIZE_8:  memSize = MB(8);  break; /* 8  Mega byte */
+        case DRAM_CTRL_SIZE_16: memSize = MB(16); break; /* 16 Mega byte */
+        case DRAM_CTRL_SIZE_32: memSize = MB(32); break; /* 32 Mega byte */
+        case DRAM_CTRL_SIZE_64: memSize = MB(64); break; /* 64 Mega byte */
+#if 0
+        default:                memSize = MB(0);  break; /* 0  Mege byte */
+#endif
+    }
+
+    return memSize;
+}

64 Megabytes in bytes is too large for a signed int. This code cannot
work. It's clear that someone thought about this, since there's a
more-correct commented-out function signature above it.

Also, I see

+*         Copyright (c) 2007 by Silicon Motion, Inc. (SMI)
+*
+*  All rights are reserved. Reproduction or in part is prohibited
+*  without the written consent of the copyright owner.

That doesn't look good.

There's a lot more code here that I'm not going to review, given the
number of silly things I've spotted in 15 minutes.

Matt


More information about the xorg-devel mailing list