[U-Boot] [PATCH 2/3] Add board specific code for da850 EVM

[Nick Thompson <nick.thompson@ge.com>]

On 18/12/09 10:23, Sudhakar Rajashekhar wrote:
> Nick Thompson <nick.thompson <at> ge.com> writes:
> 
>>
>> On 16/12/09 10:24, Sudhakar Rajashekhara wrote:
>>> Provides initial support for TI OMAP-L138/DA850 SoC devices on
>>> a Logic PD EVM board.
>>>
>>> Provides:
>>> Initial boot and configuration.
>>> Support for i2c.
>>> UART support (console).
>>>
>>> Signed-off-by: Sudhakar Rajashekhara <sudhakar.raj <at> ti.com>
>>> ---
>>>  board/davinci/da8xxevm/Makefile         |    1 +
>>>  board/davinci/da8xxevm/da850evm.c       |  124 
> +++++++++++++++++++++++++++++++
>>>  include/asm-arm/arch-davinci/hardware.h |    1 +
>>>  3 files changed, 126 insertions(+), 0 deletions(-)
>>>  create mode 100644 board/davinci/da8xxevm/da850evm.c
>>>
>>
>> [...]
>>
>>> diff --git a/board/davinci/da8xxevm/da850evm.c 
> b/board/davinci/da8xxevm/da850evm.c
>>> new file mode 100644
>>> index 0000000..92548e2
>>> --- /dev/null
>>> +++ b/board/davinci/da8xxevm/da850evm.c
>>> @@ -0,0 +1,124 @@
>>> +/*
>>> + * (C) Copyright 2009, Texas Instruments, Inc. http://www.ti.com/
>>> + *
>>> + * Based on da830evm.c
>>
>> At this point, this seem to be a copy of da830evm.c (well except the first 
> few lines above).
>>
> 
> I don't think you have observed that the PINMUX registers are different on 
> da850. So we are at risk of adding many ifdefs now itself if we combine da830 
> and da850. Also, there are few things which differ between da830 and da850, for 
> example, MAC address is stored in I2C eeprom on da830 and in SPI eeprom on 
> da850. You can have a look at http://www.arago-project.org/git/people/?
> p=sekhar/u-boot-omapl1.git;a=summary to understand how the da830 and da850 
> files look later when support for most peripherals are added.
> 
> To avoid code cluttering, I prefer using two separate files for da830 and da850.

Yes, the pinmux table definitions are different, but I don't see how this
requires duplicating code.

In the latest patches for da830, the pinmux setup is table driven. It would
be very simple to pull out the two tables into da830 and da850 specific
files (selected by the build system) and use common pinmux code to setup the
pins appropriately. The code part for this is simply:

	/* configure pinmux settings */
	if (davinci_configure_pin_mux_items(pinmuxes, ARRAY_SIZE(pinmuxes)))
		return 1;

That would be exactly the same line of code for both devices.

It should be possible to put i2c (da830) and spi (da850) equivalent functions
into that same file to read the MAC address. Abstractions of this form are
also likely to make the code more readable. [In fact the function is already
abstracted into misc.c, but it only provides for i2c currently]

There might be an issue with functions supported on one platform that are not
supported on the other, but these could also be abstracted out. They also
ought to be few because of the similarities of the devices.

Not doing so puts us at risk of increasing commmits and maintenance. There is
no need to clutter the code - it can be layered and abstracted to handle
differences cleanly.

Nick.