On Thu, 2011-01-20 at 09:41 -0700, Tom Warren wrote:
On Wed, Jan 19, 2011 at 5:04 PM, Peter Tyser ptyser@xes-inc.com wrote:
Hi Tom, Some last minutes nits:
It looks like some of the new functions can be declared statically. It'd be nice to do so where possible.
Which functions, Peter? Please point them out specifically, thanks.
Any function that won't be called from outside the scope of the file. Eg it looks like init_uart() and setup_uart() are local functions and should be static. Those are the 2 that jumped out initially, but you should review to see if there are others.
<snip>
+/*
- Routine: uart_clock_init
- Description: init the PLL and clock for the UART in uart_num
- */
+void uart_clock_init(int uart_num) +{
Are all these uart functions board-specific? They look more CPU-specific. If that's the case they should be moved somewhere in arch/arm/*. Other boards that use the Tegra2 don't want to duplicate this code or link into Nvidia's board/nvidia directory.
It's Tegra2 SoC-specific - that's not the CPU, per se. I guess I could move it to arch/arm/cpu/armv7/tegra2, if you think it's important enough.
I think they should be moved. If they aren't, the next board vendor (eg my company) that uses the Tegra2 will copy your board.[ch] into their board/<vendor> directory and use them as a starting point, which is a large duplication of code. Moving it somewhere in arch/arm is the "right" thing to do and will make every future tegra2 board port cleaner and easier.
Best, Peter