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

["Tiwari, Rakesh" <Rakesh.Tiwari@idt.com>]

Hi Chris,

Appreciate your feedback.
Please see my comments below, inline prefixed by [rkt]

Look forward for additional comments/suggestions, if any.

Thanks
Rakesh

-----Original Message-----
From: Chris Wedgwood [mailto:cw@f00f.org]
Sent: Friday, March 10, 2006 9:46 AM
To: Tiwari, Rakesh
Cc: 'Ralf Baechle'; linux-mips@linux-mips.org
Subject: Re: [PATCH] IDT Interprise Processor Support for Linux 2.6.x

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.

    [rkt] Agreed, 1.6MB is a huge patch. I will try to break it down into
          multiple patches (ard 5) based on platform/eval board and will
          send it out soon.

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

    [rkt] I think these are still there.
       
  * The Ethernet drivers should probably go jeff@garzik.org and cc
    netdev@vger.kernel.org

    [rkt] The Ethernet interface/driver is integral to each processor
    and dependent upon other system header files, unlike a regular NIC.
    I can try posting the patches (probably sub-patch) to Jeff and netdev,
    in order to get feedback on the driver.

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

    [rkt] Chris you hit the bulls eye. This is the only function which
    I missed out... Will clean it up.
       
  * 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.

    [rkt] Since all the IDT's processors are primarily SoC's, the UARTS are
     part of the processor. In case on rc32434 there is only 1 UART. However
     rc32438 has 2 UARTS

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

   [rkt] Will try to clean up as much as possible...

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.