[U-Boot] [PATCH v3 1/4] arm: Tegra2: Add basic NVIDIA Tegra2 SoC support

[Peter Tyser]

On Thu, 2011-01-20 at 09:41 -0700, Tom Warren wrote:
> On Wed, Jan 19, 2011 at 5:04 PM, Peter Tyser <ptyser at 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