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

Tom Warren
2011-01-24T17:32:01Z

Peter,

On Fri, Jan 21, 2011 at 4:46 PM, Peter Tyser <ptyser@xes-inc.com> wrote:
> Hi Tom,
>
> On Fri, 2011-01-21 at 16:06 -0700, Tom Warren wrote:
>> Signed-off-by: Tom Warren <twarren@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
>
>