[U-Boot] [PATCH V7 0/4] Add disk support to orion5x and edminiv2

[Albert ARIBAUD]

Le 05/08/2010 15:02, Wolfgang Denk a écrit :
> Dear Rogan Dawes,
>
> In message<4C5AB1A4.3000409 at dawes.za.net>  you wrote:
>> This may be a stupid comment, but from my perspective implementing
>> Albert's orion5x changes for my DNS323, all I am doing is copying a lot
>> of what Albert is doing for the edminiv2 verbatim.
>>
>> Would it not make sense perhaps to define defaults in a SoC config file,
>> and then allow them to be overridden as required for each specific board?
>
> That would make sense, but quite often you don't know what will be
> common code and what not when writing the first version of such code.
>
> I recommend that you discuss with Albert (as part of the review
> process here) what should be handled as common driver code that you
> can easily reuse.
>
> It is pretty likely that your copied code would not be accepted for
> mainline, and you will be requested to factor out common parts when
> you submit it (that's quite often the fate for the second or third in
> such a row).
>
> Best regards,
>
> Wolfgang Denk

Rogan,

As Wolfgang points out, we need to know which code can be common and 
which cannot. IIRC, you are creating a new board for dns323, so you most 
probably copied the edminiv2 board code (board/LaCie/edminiv2/) and the 
edminiv2 config (include/configs/edminiv2).

As for configurations, I think they should be kept separate -- unless 
Wolfgang wants to do something similar to what is happening at the 
moment with ARM defconfigs in the Linux kernel, but he showed no sign of 
it. :)

As for the edminiv2 board code, it only provides three functions, 
board_flash_get_legacy(), board_init() and reset_phy().

We know that flash_get_legacy() cannot be shared (we both have non-CFI 
compliant flash chips, but they're different). Granted, we could rewrite 
it as a common/ function and keep the board-specifics in 
CONFIG_FLASH_LEGACY_xxxx defines (or a struct), but I don't see a 
benefit in code size, simplicity or clarity in this.

We could try to commonalize board_init(), which should only differ in 
machine type, but it is very small, so I don't see a great benefit in 
factorizing, and if any initialization is to be added later on to one 
board, we'd have to revert somehow.

What we could share is reset_phy() if your board has an MV88E1116 
Ethernet phy; then you could move reset_phy() to a driver in net/ that 
would be built conditionally to e.g. CONFIG_PHY_MV88E1116, and then you 
would add the corresponding #define to the edminiv2 and dns323 config files.

The rest of my devs (gbe, ide) is only a matter of setting the right 
configuration. Granted, that makes our config files more similar, but as 
I said, just because they are similar is not enough of a reason to 
factorize these IMO.

Rogan, what other code do you think you'd be duplicating?

Amicalement,
-- 
Albert.