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@xxxxxxxx]
Sent: Friday, March 10, 2006 9:46 AM
To: Tiwari, Rakesh
Cc: 'Ralf Baechle'; linux-mips@xxxxxxxxxxxxxx
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@xxxxxxxxxx and cc
netdev@xxxxxxxxxxxxxxx
[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.
