[U-Boot] [PATCH V5 2/4] serial: Add Tegra2 serial port support

[Tom Warren]

Peter,

On Fri, Jan 21, 2011 at 4:46 PM, Peter Tyser <ptyser at xes-inc.com> wrote:
> Hi Tom,
>
> On Fri, 2011-01-21 at 16:06 -0700, Tom Warren wrote:
>> Signed-off-by: Tom Warren <twarren at nvidia.com>
>> ---
>> Changes for V2:
>>       - Move serial driver to separate patch
>>
>> Changes for V5:
>>       - Move arch/arm/cpu/armv7/uart.c & board.h to drivers/serial and
>>               rename to serial_tegra2.c
>>       - Remove use of uart_num & UART_A/D in serial_tegra2, simplify code
>>
>>  arch/arm/cpu/armv7/tegra2/Makefile |    2 +-
>>  arch/arm/cpu/armv7/tegra2/board.c  |    2 +-
>>  arch/arm/cpu/armv7/tegra2/board.h  |   58 ----------
>>  arch/arm/cpu/armv7/tegra2/uart.c   |  216 ------------------------------------
>>  common/serial.c                    |    3 +-
>>  drivers/serial/Makefile            |    1 +
>>  drivers/serial/serial_tegra2.c     |  205 ++++++++++++++++++++++++++++++++++
>>  drivers/serial/serial_tegra2.h     |   49 ++++++++
>>  include/serial.h                   |    3 +-
>>  9 files changed, 261 insertions(+), 278 deletions(-)
>>  delete mode 100644 arch/arm/cpu/armv7/tegra2/board.h
>>  delete mode 100644 arch/arm/cpu/armv7/tegra2/uart.c
>>  create mode 100644 drivers/serial/serial_tegra2.c
>>  create mode 100644 drivers/serial/serial_tegra2.h
>
> It looks like arch/arm/cpu/armv7/tegra2/board.h and
> arch/arm/cpu/armv7/tegra2/uart.c are added in the first patch, then
> moved in this patch.  It'd be ideal to just add them once in the proper
> location.
>
> On a side note, if you pass "git format-patch" the -M and -C options it
> will make pretty diffs that only show what lines changed during a move.
> In the case that you do move files in the future its nice to use those
> options to ease review.
>
I'll use those options in the future (thanks!) to keep things cleaner.
Hopefully we can just go with this set of patches so I can get to the
other, more interesting code (drivers, A9 CPU init, etc.).

> <snip>
>
> +void uart_init(void)
>> +{
>> +     /* Init each UART - there may be more than 1 on a board/build */
>> +#if (CONFIG_TEGRA2_ENABLE_UARTA)
>> +     init_uart();
>> +#endif
>> +#if (CONFIG_TEGRA2_ENABLE_UARTD)
>> +     init_uart();
>> +#endif
>> +}
>
> How about:
> #if defined(CONFIG_TEGRA2_ENABLE_UARTA) || defined(CONFIG_TEGRA2_ENABLE_UARTD)
>        init_uart();
> #endif
That won't work for a board like Harmony where the developer wants
both UARTs active, since uart_init is only called once.

>
> Best,
> Peter
Thanks,
Tom
>
>