[U-Boot-Users] [PATCH] drivers: initial tree import for drivers reorganization

[Grant Likely <grant.likely@secretlab.ca>]

On 10/11/07, Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote:
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>

First, a general comment.  This patch should go away entirely.  Create
the new makefile one at a time in the same patch where you move the
files.  It will be easier to review/merge that way.

Specific comments below.  Otherwise, not a bad patch series.  Address
the comments and it will probably look pretty good.  Thanks for the
work.

> ---
>  Makefile                                 |   17 +++++++++++++++++
>  drivers/{nand_legacy => ata}/Makefile    |   11 ++++++-----
>  drivers/bios_emulator/Makefile           |   20 ++++++++++++--------
>  drivers/{nand_legacy => block}/Makefile  |   11 ++++++-----
>  drivers/{nand_legacy => char}/Makefile   |   11 ++++++-----
>  drivers/{nand_legacy => eeprom}/Makefile |   11 ++++++-----
>  drivers/{nand_legacy => hwmon}/Makefile  |   11 ++++++-----
>  drivers/{nand_legacy => i2c}/Makefile    |   11 ++++++-----
>  drivers/{nand_legacy => ide}/Makefile    |   11 ++++++-----
>  drivers/{nand_legacy => input}/Makefile  |   11 ++++++-----
>  drivers/{nand_legacy => leds}/Makefile   |   11 ++++++-----
>  drivers/{nand_legacy => misc}/Makefile   |   11 ++++++-----
>  drivers/{nand_legacy => mtd}/Makefile    |   11 ++++++-----
>  drivers/nand_legacy/Makefile             |    3 ++-
>  drivers/net/Makefile                     |    3 ++-
>  drivers/{nand_legacy => pci}/Makefile    |   11 ++++++-----
>  drivers/{nand_legacy => pcmcia}/Makefile |   11 ++++++-----
>  drivers/qe/Makefile                      |    6 +++++-
>  drivers/{nand_legacy => sata}/Makefile   |   11 ++++++-----
>  drivers/{nand_legacy => scsi}/Makefile   |   11 ++++++-----
>  drivers/serial/Makefile                  |    3 ++-
>  drivers/{nand_legacy => usb}/Makefile    |   11 ++++++-----
>  drivers/{nand_legacy => video}/Makefile  |   11 ++++++-----
>  23 files changed, 142 insertions(+), 97 deletions(-)
>  copy drivers/{nand_legacy => ata}/Makefile (88%)
>  copy drivers/{nand_legacy => block}/Makefile (87%)
>  copy drivers/{nand_legacy => char}/Makefile (87%)
>  copy drivers/{nand_legacy => eeprom}/Makefile (87%)
>  copy drivers/{nand_legacy => hwmon}/Makefile (87%)
>  copy drivers/{nand_legacy => i2c}/Makefile (88%)
>  copy drivers/{nand_legacy => ide}/Makefile (88%)
>  copy drivers/{nand_legacy => input}/Makefile (87%)
>  copy drivers/{nand_legacy => leds}/Makefile (87%)
>  copy drivers/{nand_legacy => misc}/Makefile (87%)
>  copy drivers/{nand_legacy => mtd}/Makefile (88%)
>  copy drivers/{nand_legacy => pci}/Makefile (88%)
>  copy drivers/{nand_legacy => pcmcia}/Makefile (87%)
>  copy drivers/{nand_legacy => sata}/Makefile (87%)
>  copy drivers/{nand_legacy => scsi}/Makefile (87%)
>  copy drivers/{nand_legacy => usb}/Makefile (88%)
>  copy drivers/{nand_legacy => video}/Makefile (87%)

Here are my comments on the layout:
>
> diff --git a/Makefile b/Makefile
> index 6410f08..57038e7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -208,18 +208,35 @@ LIBS += disk/libdisk.a
>  LIBS += rtc/librtc.a
>  LIBS += dtt/libdtt.a
>  LIBS += drivers/libdrivers.a
> +LIBS += drivers/ata/libata.a

Merge with drivers/block

>  LIBS += drivers/bios_emulator/libatibiosemu.a
> +LIBS += drivers/block/libblock.a
> +LIBS += drivers/char/libchar.a

What is this?  how is 'char' different from 'serial'?  We don't have a
char device API, so I don't understand how this fits in.

You're moving keyboard drivers into here; so maybe merge with drivers/input.

> +LIBS += drivers/eeprom/libeeprom.a

I'm not sure about this one; it feels kind of wrong (there can be i2c
eeprom, SPI eeprom, etc).  I need to think some more...

> +LIBS += drivers/hwmon/libhwmon.a

We don't really have an hwmon api, and only one driver is being moved
there which is an i2c device.  Probably just merge with drivers/i2c.

> +LIBS += drivers/i2c/libi2c.a
> +LIBS += drivers/ide/libide.a

merge with drivers/block

> +LIBS += drivers/input/libinput.a
> +LIBS += drivers/leds/libleds.a

merge with drivers/misc

> +LIBS += drivers/misc/libmisc.a
> +LIBS += drivers/mtd/libmtd.a

eeprom and NAND might be better to go in here.


>  LIBS += drivers/nand/libnand.a
>  LIBS += drivers/nand_legacy/libnand_legacy.a

These two need to be sorted out; but that doesn't affect your patch
series... just thinking out loud.

>  LIBS += drivers/net/libnet.a
> +LIBS += drivers/pci/libpci.a
> +LIBS += drivers/pcmcia/libpcmcia.a

I'm tempted to just merge these under drivers/bus since it's not a lot
of files...  not sure though.

>  ifeq ($(CPU),mpc83xx)
>  LIBS += drivers/qe/qe.a
>  endif
>  ifeq ($(CPU),mpc85xx)
>  LIBS += drivers/qe/qe.a
>  endif
> +LIBS += drivers/sata/libsata.a

merge with drivers/block

> +LIBS += drivers/scsi/libscsi.a

merge with drivers/block

>  LIBS += drivers/serial/libserial.a
>  LIBS += drivers/sk98lin/libsk98lin.a
> +LIBS += drivers/usb/libusb.a
> +LIBS += drivers/video/libvideo.a
>  LIBS += post/libpost.a post/drivers/libpostdrivers.a
>  LIBS += $(shell if [ -d post/lib_$(ARCH) ]; then echo \
>         "post/lib_$(ARCH)/libpost$(ARCH).a"; fi)
> diff --git a/drivers/nand_legacy/Makefile b/drivers/ata/Makefile
> similarity index 88%
> copy from drivers/nand_legacy/Makefile
> copy to drivers/ata/Makefile
> index 95314d8..f93d6cb 100644
> --- a/drivers/nand_legacy/Makefile
> +++ b/drivers/ata/Makefile
> @@ -1,7 +1,7 @@
>  #
> -# (C) Copyright 2006
> +# (C) Copyright 2007
>  # Wolfgang Denk, DENX Software Engineering, wd at denx.de.
> -#
> +# Jean-Christophe PLAGNIOL-VILLARD, plagnioj at jcrosoft.com.

Really?  This is just a boilerplate Makefile where you've moved lines
from one file to another.  It's a pretty small change to be asserting
copyrights.  (I didn't make claims on the rework to drivers/Makefile
and common/Makefile which was more invasive).

Don't worry, you'll still get credit for your work.

> diff --git a/drivers/bios_emulator/Makefile b/drivers/bios_emulator/Makefile
> index 90c64da..cb0a13e 100644
> --- a/drivers/bios_emulator/Makefile
> +++ b/drivers/bios_emulator/Makefile
> @@ -6,14 +6,18 @@ X86DIR  = x86emu
>
>  $(shell mkdir -p $(obj)$(X86DIR))
>
> -COBJS  = atibios.o biosemu.o besys.o bios.o \
> -       $(X86DIR)/decode.o \
> -       $(X86DIR)/ops2.o \
> -       $(X86DIR)/ops.o \
> -       $(X86DIR)/prim_ops.o \
> -       $(X86DIR)/sys.o \
> -       $(X86DIR)/debug.o
> -
> +COBJS-y        += atibios.o
> +COBJS-y        += biosemu.o
> +COBJS-y        += besys.o
> +COBJS-y        += bios.o
> +COBJS-y        += $(X86DIR)/decode.o
> +COBJS-y        += $(X86DIR)/ops2.o
> +COBJS-y        += $(X86DIR)/ops.o
> +COBJS-y        += $(X86DIR)/prim_ops.o
> +COBJS-y        += $(X86DIR)/sys.o
> +COBJS-y        += $(X86DIR)/debug.o
> +
> +COBJS  := $(COBJS-y)
>  SRCS   := $(COBJS:.o=.c)
>  OBJS   := $(addprefix $(obj),$(COBJS))

Split this change into it's own makefile.... Actually drop this change
entirely.  It is unnecessary because all the bios emulation files are
all compiled at the same time regardless.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely at secretlab.ca
(403) 399-0195