答复: 答复: [PATCH]new driver for siliconmotion

[Aaron.Chen 陈俊杰]

Hi Matt&Michal,

Thank you for your advice. We are updating our codes in order to submit the source code successfully.

We are doing the following changes. Next patch will be in the end of this month. (by the way, shall I make a new patch based on older source code or based on last patch?)
1. > 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.

We are updating the file. 

2. > 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.

We've replaced old Sun copyright notice with the Oracle copyright notice. Is it OK?

3. > src/CALLMAP                     |   22 +
>This is a useless file that was probably removed from git a long time ago. Don't add it back.

The file has been deleted.

4. > 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.

The file has been deleted.

5. 
+*         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.

We will use the same old format:    Copyright (C) YYYY Silicon Motion, Inc.  All Rights Reserved. 
Is this OK?


>I guess this is not really a requirement but makes the code needlessly unattractive.

>Since this is an initial submission it is the right time to change this.

>If you look at the other drivers they have rather simple structure with all files in a single directory. Large part of the code is shared. When support for new chipset family is added a few files specific to that chipset are typically required which have the chipset name in their file name but much of the rest is just updated slightly.

>Now you add support for half dozen of chips and I have no idea how they are related to each other and to the chips previously supported by the smi driver. You should know the best since you wrote the code.

>However, the file naming suggests that you started with separate driver for every chip and just added hooks in the old driver to call one or the other with very little sharing.

>This has both advantages and drawbacks but with few people working on the code the less needs to be maintained the better. Then sharing as much code as reasonably possible makes sense, and making bazillion subdirectories does not help that.

We've talked about this issue. We prefer not to change the driver's structure. There is a lots of reason why we decide to separate the ddk file for each chip, and we decide to separate the code in the beginning of the driver design. So we need to confirm one thing: Is this driver ok to be added into the X.Org source tree or we must change the codes structure otherwise we cannot be accepted.

Is there anything else we need to change to be accepted by the X.Org. We really appreciate your advice. Thank you so much!

Aaron

-----邮件原件-----
发件人: Michal Suchanek [mailto:hramrach at gmail.com] 
发送时间: 2012年7月17日 20:15
收件人: Aaron.Chen 陈俊杰
抄送: Matt Turner; xorg-devel at lists.x.org; caesar.qiu 裘赛海; Ilena.Zhou周菁华
主题: Re: 答复: [PATCH]new driver for siliconmotion

On 17 July 2012 10:31, Aaron.Chen  陈俊杰 <aaron.chen at siliconmotion.com> wrote:
> Hi Matt,
>
> We really appreciate your advice! The project is very important to us! We have worked for the project for two years. It can support all the SMI graphics chips and works OK on FC, SUSE, Ubuntu Red Hat, etc. for both 32 and 64 bit OS. The code contains two different types of driver. One is support XRandr and the other is not which is for multi-adapter.
> So it may remain some old structure files.
>
> And one more question asked by our develop team:
>>" Do we really have to prefix all these files and directories with 'ddk'?"
>
> So, We'd better change all the name of files named "ddk*". Is that right?
> Aaron

I guess this is not really a requirement but makes the code needlessly unattractive.

Since this is an initial submission it is the right time to change this.

If you look at the other drivers they have rather simple structure with all files in a single directory. Large part of the code is shared. When support for new chipset family is added a few files specific to that chipset are typically required which have the chipset name in their file name but much of the rest is just updated slightly.

Now you add support for half dozen of chips and I have no idea how they are related to each other and to the chips previously supported by the smi driver. You should know the best since you wrote the code.

However, the file naming suggests that you started with separate driver for every chip and just added hooks in the old driver to call one or the other with very little sharing.

This has both advantages and drawbacks but with few people working on the code the less needs to be maintained the better. Then sharing as much code as reasonably possible makes sense, and making bazillion subdirectories does not help that.

Thanks

Michal