On Mon, 22 Oct 2007 18:17:49 +0800 "Peter Chan" <accusys.sw5@gmail.com> wrote:
Please, you really will need to become a bit more familiar with the way we
work. As far as I know, nobody in the linux world uses RAR format - that's
a windows thing. I doubt if anyone except I has actually gone to the
effort to decrypt that attachment.
Start with
Documentation/SubmittingPatches
Documentation/SubmittingDrivers
Documentation/SubmitChecklist
http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txthttp://linux.yyz.us/patch-format.html
There are a number of remaining stylistic things which we can look at more
closely when we have patches which are in a usable form.
- Linux doesn't use capitalisation in variable names. Use "tail", not "Tail"
- Linux uses underscored to separate words. Use "reply_frame", not
"replyframe" or "ReplyFrame".
- We don't like to see code which has any dependency on
LINUX_VERSION_CODE or KENREL_VERSION: the code in Linux is suppsoed to
work correctly in the version of the kernel which it s found and that's
it.
- I don't know what this:
+#if defined(CONFIG_MODVERSIONS) && !defined(MODVERSIONS)
+ #define MODVERSIONS
+#endif
is doing, but it's probably wrong.
- Use request_node, not RequestNode, etc.
- Don't parenthesise the argument to `return'.
- I see at least one U32 in there. Please use u32. (Does U32 even work?)
- This:
+static int acs_ame_get_log(
+ struct Acs_Adapter *acs_adt,
+ struct EventLog *event_log)
isn't preferred style. Use
static int acs_ame_get_log(struct Acs_Adapter *acs_adt,
struct EventLog *event_log)
or, if you particularly dislike that, blow the 80-col rule and do
static int acs_ame_get_log(struct Acs_Adapter *acs_adt, struct EventLog *event_log)
- This
+ writel((replyframe), base_addr+AME_REPLY_MSG_PORT);
is overparenthesised.
- Beware that the scatter/gather APIs just got significantly changed.
You code might need adjustment to work against the latest mainline tree.
- What does CHAR_DEV do? Probably it should be a Kconfig CONFIG_* option.
- All the code around acs_ame_schedule_command() (which is incorrectly
identified as arcmsr_schedule_command in its comment block) is indented a
tab stop. That's really weird. Please make it normal.
- acs_ame_schedule_command() has an up-to-sixty-second busywait. Bad.
Can we get a sleep+wakeup in there?
- This:
+ struct
+ {
+ unsigned int vendor_id;
+ unsigned int device_id;
+ } const acs_ame_devices[] = {
+ { 0x14D6, DEVICEID_ACS_61000_XX }
+ , { 0x14D6, DEVICEID_ACS_62000_08 }
+ , { 0x1AB6, DEVICEID_ACS_61000_XX }
+ , { 0x1AB6, DEVICEID_ACS_62000_08 }
+ };
should be
static const struct {
unsigned int vendor_id;
unsigned int device_id;
} acs_ame_devices[] = {
{ 0x14D6, DEVICEID_ACS_61000_XX },
{ 0x14D6, DEVICEID_ACS_62000_08 },
{ 0x1AB6, DEVICEID_ACS_61000_XX },
{ 0x1AB6, DEVICEID_ACS_62000_08 },
};
which has many changes from the original.
I'd have thought that the kernel already has a data type for this, but
I can't find it. Most drivers just rely upon the normal PCI device ID
tables. It is suspicious that this one doesn't.
Anyway, that's just from a quick scan. There are a huge number of similar
issues in there. Please take some time to study some well-maintained Linux
driver code and the interfaces which scsi and PCI drivers use and try to
make this driver a lot more Linux-like, thanks.
-
