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

[Tom Warren]

Peter,

On Mon, Jan 24, 2011 at 12:14 PM, Peter Tyser <ptyser at xes-inc.com> wrote:
> <snip>
>
>> I see what you're talking about now - I've got uart.c in 2 patch files - created
>> in 0001 and then moved in 0002. My bad - that wasn't the intent, just what
>> happened when I applied my V4 patches to a new branch to get the V5 patchset
>> built.  I'll fix it and resubmit.
>>
>> As to 0002 not adding serial port support for Tegra2, that's all it does - adds
>> TEGRA2 defines to serial.h/serial.c for the eserial* tables, and then adds
>> code to turn on Tegra2-specific UART HW.  If I remove any mention of uart.c
>> in patch 0001 (add basic Tegra2 support), then does patch 0002 make
>> sense to you?
>
> Yeah, that'd make more sense.  Patch 2 would just contain:
>  common/serial.c                    |    3 +-
>  drivers/serial/Makefile            |    1 +
>  drivers/serial/serial_tegra2.c     |  205 ++++++++++++++++++++++++++++++++++
>  drivers/serial/serial_tegra2.h     |   49 ++++++++
>  include/serial.h                   |    3 +-
>
Exactly what I was thinking. I'll try to get it right in patch V6.

>> >> > <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.
>> >
>> > Why should init_uart() be called two times?  It looks to initialize both
>> > ports, meaning it should only be called once, right?
>> Correct, again (need more coffee!)  Your if defined construct wouldn't work
>> as written, though, because CONFIG_TEGRA2_ENABLE_UARTx is always
>> defined (as 0 or 1). I'd have to rework it.
>
> You could also just get rid of uart_init() altogether and rename
> init_uart() to uart_init().  That would get rid of some idefs and
> simplify the flow.
Yeah, I saw that as I was cleaning up the indentation & reworking the
code to compile
with both UARTs defined. I'll get rid of  uart_init (renamed to
init_uart). Thanks.

>
> Best,
> Peter
Thanks,

Tom
>
>