linux-mips
[Top] [All Lists]

Re: [PATCH] IDT Interprise Processor Support for Linux 2.6.x

To: "Tiwari, Rakesh" <Rakesh.Tiwari@idt.com>
Subject: Re: [PATCH] IDT Interprise Processor Support for Linux 2.6.x
From: Chris Wedgwood <cw@f00f.org>
Date: Fri, 10 Mar 2006 09:45:35 -0800
Cc: 'Ralf Baechle' <ralf@linux-mips.org>, linux-mips@linux-mips.org
In-reply-to: <73943A6B3BEAA1468EE1A4A090129F4316B15A73@corpbridge.corp.idt.com>
Original-recipient: rfc822;linux-mips@linux-mips.org
References: <73943A6B3BEAA1468EE1A4A090129F4316B15A73@corpbridge.corp.idt.com>
Sender: linux-mips-bounce@linux-mips.org
Additional comments:

  * Firstly, it's really great to see this!

  * A single 1.6MB patch is far from ideal, please try to break it
    into a series of smaller logically separate patches.  It's hard to
    comment on a giant patch.  Perhaps something like:
      - a patch for each CPU
      - a patch for each driver
      - a patch for each platform/eval-board
    and see what you have left. Each patch should have a suitable
    description.  Also put "Signed-off-by:" lines on your patches.

  * You shouldn't be removing .gitignore :-)

  * The Ethernet drivers should probably go jeff@garzik.org and cc
    netdev@vger.kernel.org

  * The code contains unreferenced functions?  Without even looking
    hard I can see rc32434_mii_handler is declared and not used for
    example.

  * It might be that some of the CPU-level code should be platform
    level.  For example having two UARTs is a feature of the EB434 not
    the rc32434 so EB434_UART1_IRQ is misplaced I would argue.

  * Some init code should probably be declared __init and similar

  * There is quite a bit of extraneous white-space that could be
    cleaned up and some minor indentation cleanups to match what is
    elsewhere in the kernel.

Sorry this is a little vague and 'hand-wavy', if you post smaller
logically complete patches I think you'll get better feedback where
people can comment more easily.  Ideally inline to the email if you
can, m$ lookout/$exchange as that just makes a mess, if you have to
use that then attach them as you did.

<Prev in Thread] Current Thread [Next in Thread>