[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