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 陈俊杰 <[email protected]> wrote: > From bda2e699bfee84e1266f28da53a6debe432db96b Mon Sep 17 00:00:00 2001 > > From: root <root@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<[email protected]> > > --- > > 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 _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
