[U-Boot] [PATCH0/6] patchset to support TPL and P1021MDS board

[Kumar Gala]

On Jan 31, 2011, at 2:31 PM, Scott Wood wrote:

> On Mon, 31 Jan 2011 21:22:04 +0100
> Wolfgang Denk <wd at denx.de> wrote:
> 
>> Dear Scott Wood,
>> 
>> In message <20110131141332.5a4a297d at udp111988uds.am.freescale.net> you wrote:
>>> 
>>>> I think these patches are incorrectly split.
>>> 
>>> I think the intent was to split the arch-neutral stuff from the 85xx
>>> stuff from the board stuff -- you'd rather they be all bunched together?
>> 
>> No, of course not all together.
>> 
>>>> 	This patch adds stuff to the Makefile, which would result in
>>>> 	errors if used, as the referenced directories don't exist yet.
>>> 
>>> Lots of patches add features, disabled by default, that require CPU or
>>> board code to provide things, that would cause errors if the feature
>>> were enabled on a board otherwise.
>> 
>> But here nothing is disabled. It's added to the top level Makefile.
>> It's dead code if unused, and causes errors if used.  WHy not add the
>> tpl target when you actually add the tpl code?
>> 
>>> I don't think it's even possible to add an empty directory with git.
>> 
>> True.  Butt that would not fix anythign, it would still not work.
>> 
>>>> [PATCH 2/6] powerpc/85xx: add TPL support
>>>> 
>>>> 	This patch creates unused files, like
>>>> 	arch/powerpc/cpu/mpc85xx/u-boot-tpl.lds
>>> 
>>> It gets used in later in the patchset, when a board with tpl is added.
>> 
>> Then this is where that file belongs to.
> 
> I'm confused.  You say "of course not all together", but the first one
> you say to include with the second, and the second you say to include
> with the third.
> 
> If you're suggesting keeping them mostly separate, but just moving some
> bits into the subsequent patch, that makes no sense to me.  They
> logically belong where they are -- e.g.
> arch/powerpc/cpu/mpc85xx/u-boot-tpl.lds is part of 85xx TPL support, it
> is not p1021mds-specific.  And every bit of the first two patches is
> technically dead until a board is added that uses it.
> 
> Has your aversion to "dead" code grown so strong it can't exist even in
> a transitory state between members of a patchset, even when necessary
> to avoid mixing users of a facility with the facility itself in the
> same patch?  I think that would do significant harm to reviewability.
> 
> -Scott

I'm in agreement with Scott on this.  I believe we've taken this a bit too far about "dead code".  It should be reasonable in a patch series to have code that will be used in a subsequent patch.

- k