[U-Boot] [PATCH 03/11] DM: add block controller core

[Marek Vasut <marex@denx.de>]

Dear Pavel Herrmann,

> On Friday 21 of September 2012 14:51:33 Marek Vasut wrote:
> > Dear Pavel Herrmann,
> > 
> > > On Thursday 20 of September 2012 22:05:36 Marek Vasut wrote:
> > > > Dear Pavel Herrmann,
> > > > 
> > > > > This core provides unified access to different block controllers
> > > > > (SATA,
> > > > > SCSI).
> > > > 
> > > > Description of the patch missing or is sub-par. You should work on
> > > > this skill.
> > > > 
> > > > > Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
> > > > > ---
> > > > > 
> > > > >  Makefile                   |   1 +
> > > > >  drivers/blockctrl/Makefile |  42 ++++++
> > > > >  drivers/blockctrl/core.c   | 349
> > > > > 
> > > > > +++++++++++++++++++++++++++++++++++++++++++++
> > > > > include/dm/blockctrl.h
> > > > > 
> > > > >  75 ++++++++++
> > > > >  4 files changed, 467 insertions(+)
> > > > >  create mode 100644 drivers/blockctrl/Makefile
> > > > >  create mode 100644 drivers/blockctrl/core.c
> > > > >  create mode 100644 include/dm/blockctrl.h
> > > > > 
> > > > > diff --git a/Makefile b/Makefile
> > > > > index e43fd9d..4420484 100644
> > > > > --- a/Makefile
> > > > > +++ b/Makefile
> > > > > @@ -304,6 +304,7 @@ LIBS-y += test/libtest.o
> > > > > 
> > > > >  LIBS-$(CONFIG_DM) += common/dm/libdm.o
> > > > >  LIBS-$(CONFIG_DM) += drivers/demo/libdemo.o
> > > > >  LIBS-${CONFIG_DM_BLOCK} += drivers/blockdev/libblockdev.o
> > > > > 
> > > > > +LIBS-${CONFIG_DM_BLOCK} += drivers/blockctrl/libblockctrl.o
> > > > 
> > > > ${} ? What is this ?
> > 
> > Why not just reuse drivers/block and in drivers/block compile in the
> > libblock.o so you don't polute the top-level makefile ? Easy as that.
> > 
> > > > [..]
> > > > 
> > > > This handles SCSI? Sata ? what ?
> > > > 
> > > > Should this not be called scsi_core ? sata_core ? What did the
> > > > previous core do? sata?  scsi? block? I'm lost.
> > > 
> > > the previous core handled disks (and cards and stuff) and partitions
> > > (think
> > > /dev/sdxy), and was largely a replacement of /disk
> > > this core handles any interface those disks are connected to (SATA,
> > > PATA, SCSI), and should replace /drivers/block
> > 
> > Why is this not in the commit message then ? I have a proposal, before
> > you submit a patchset, prepare it, work on something else for a bit,
> > then read again the commit message only and see if you still understand
> > what it means.
> 
> I actually did. the "something else" was splitting it into smaller patches,

I mean something totally different, so you won't have the code in front of you. 
You DO understand the code because you wrote it, you need to work on the part 
where you explain others properly what your change does. Even if it mean writing 
essay-esque commit message.

> so the original text information got distributed into the other patches.
> if i put it all here you would surely complain about it not being there,
> or it being duplicated

Not really ...

> > Am I correct that this will look as such:
> > user -> [ 01/11 ] -> [ 03/11 or something else ] -> [ if 03/11, then disc
> > ]
> 
> no idea what this means, sorry

Patch numbers, how the code added in them connect into each other.
> 
> Pavel Herrmann

Best regards,
Marek Vasut