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

[Scott Wood]

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