[Flashrom] [PATCH] Separate Makefile's main OBJS list into Programmer, Chip, Lib, CLI object lists

[Sean Nelson]

This patch shouldn't affect anything else in patchwork. It just splits 
$(OBJS) in Makefile into separate lists for Programmer, Chip, and CLI 
related files/objects. This should help later on figuring out where 
files may go for a libflashrom library.

Signed-off-by: Sean Nelson

[Carl-Daniel Hailfinger]

On 22.12.2009 20:32, Sean Nelson wrote:
> This patch shouldn't affect anything else in patchwork. It just splits
> $(OBJS) in Makefile into separate lists for Programmer, Chip, and CLI
> related files/objects. This should help later on figuring out where
> files may go for a libflashrom library.
>
> Signed-off-by: Sean Nelson
>
> commit 4df344c1a743a7d4ee633ce07706f3fb51f28074
> Author: Sean Nelson 
> Date:   Tue Dec 22 11:03:33 2009 -0800
> Modify Makfile to separate the main object list into Programmer, Chip, CLI lists.
> Signed-off-by: Sean Nelson <[e-mail]>

Maybe it is the fault of my mail client, but it seems your comment is
indented by 4 spaces.
> diff --git a/Makefile b/Makefile
> index 77731ed..4f4d870 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -43,11 +43,18 @@ endif
>
> LIBS += -lpci
>
> -OBJS = jedec.o stm50flw0x0x.o w39v080fa.o sharplhf00l04.o w29ee011.o \
> +CHIP_OBJS = jedec.o stm50flw0x0x.o w39v080fa.o sharplhf00l04.o w29ee011.o \
>    sst28sf040.o am29f040b.o mx29f002.o m29f400bt.o pm29f002.o w39v040c.o \
>    w49f002u.o 82802ab.o pm49fl00x.o sst49lf040.o en29f002a.o m29f002.o \
> sst49lfxxxc.o sst_fwhub.o flashchips.o layout.o spi.o \
> flashrom.o print.o physmap.o internal.o udelay.o
> sst49lfxxxc.o sst_fwhub.o spi.o
>
> +CLI_OBJS = flashrom.o print.o

Can you change print_wiki.o to be added to CLI_OBJS?
> +LIB_OBJS = flashchips.o layout.o physmap.o udelay.o

flashchips.o belongs to CHIP_OBJS.
physmap.o and udelay.o are PROGRAMMER_OBJS
> +PROGRAMMER_OBJS = internal.o
>
> +OBJS = $(CHIP_OBJS) $(CLI_OBJS) $(PROGRAMMER_OBJS) $(LIB_OBJS)
>
> all: pciutils features dep $(PROGRAM)

And can you please convert all the programmers to go to PROGRAMMER_OBJS
as well?
pcidev.o and serial.o are special. Not sure whether they are
PROGRAMMER_OBJS stuff or LIB_OBJS.

Thanks.

Regards,
Carl-Daniel

[Sean Nelson]

On 12/23/09 1:54 PM, Carl-Daniel Hailfinger wrote:
> Maybe it is the fault of my mail client, but it seems your comment is
> indented by 4 spaces.

That's because of how git formats inline-patch logs, and how Thunderbird 
formats inline patches/diffs. That will be the last time, I've started 
to do straight git diffs because I'm using/doing multiple commits for 
each patch, which if done in diff-log format starts to break patch 
applies. If I'm doing something that breaks things, tell me so I can 
figure out a proper workflow that works with what ever tools you're using.

Patch updated with your requests/suggestions. I'll withhold a signoff 
until we finalize the patch.

[Carl-Daniel Hailfinger]

On 23.12.2009 23:32, Sean Nelson wrote:
> On 12/23/09 1:54 PM, Carl-Daniel Hailfinger wrote:
> > Maybe it is the fault of my mail client, but it seems your comment is
> > indented by 4 spaces.
>
> That's because of how git formats inline-patch logs, and how
> Thunderbird formats inline patches/diffs. That will be the last time,
> I've started to do straight git diffs because I'm using/doing multiple
> commits for each patch, which if done in diff-log format starts to
> break patch applies. If I'm doing something that breaks things, tell
> me so I can figure out a proper workflow that works with what ever
> tools you're using.
>
> Patch updated with your requests/suggestions. I'll withhold a signoff
> until we finalize the patch.
>
> diff --git a/Makefile b/Makefile
> index 77731ed..8a53b76 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -43,11 +43,16 @@ endif
>
> LIBS += -lpci
>
> -OBJS = jedec.o stm50flw0x0x.o w39v080fa.o sharplhf00l04.o w29ee011.o \
> +CHIP_OBJS = jedec.o stm50flw0x0x.o w39v080fa.o sharplhf00l04.o w29ee011.o \
>    sst28sf040.o am29f040b.o mx29f002.o m29f400bt.o pm29f002.o w39v040c.o \
>    w49f002u.o 82802ab.o pm49fl00x.o sst49lf040.o en29f002a.o m29f002.o \
> sst49lfxxxc.o sst_fwhub.o flashchips.o layout.o spi.o \
> flashrom.o print.o physmap.o internal.o udelay.o
> sst49lfxxxc.o sst_fwhub.o spi.o flashchips.o
>
> +CLI_OBJS = flashrom.o print.o
>
> +LIB_OBJS = layout.o
>
> +PROGRAMMER_OBJS = physmap.o udelay.o internal.o

Sorry, I committed a patch to partially fix up compilation on Windows
which touches this area. There's another one pending in the same area.
physmap.o and internal.o are either lib or programmer, but they have to
be conditional on NEED_PCI (see at the end of the current makefile revision)
internal.c needs to be split into internal-programmer-only stuff and a
support lib for all internal+pci based stuff.
> all: pciutils features dep $(PROGRAM)
>
> @@ -98,13 +103,14 @@ CONFIG_PRINT_WIKI ?= no
>
> ifeq ($(CONFIG_INTERNAL), yes)
>  FEATURE_CFLAGS += -D'INTERNAL_SUPPORT=1'
> -OBJS += chipset_enable.o board_enable.o cbtable.o it87spi.o ichspi.o sb600spi.o wbsio_spi.o
> +LIB_OBJS += chipset_enable.o board_enable.o cbtable.o

These three above are definitely internal-programmer-only stuff, not
libs at all.
> +PROGRAMMER_OBJS += it87spi.o ichspi.o sb600spi.o wbsio_spi.o
>  NEED_PCI := yes
>  endif
>
> ifeq ($(CONFIG_SERPROG), yes)
>  FEATURE_CFLAGS += -D'SERPROG_SUPPORT=1'
> -OBJS += serprog.o
> +PROGRAMMER_OBJS += serprog.o
>  ifeq ($(OS_ARCH), SunOS)
>  LIBS += -lsocket
>  endif
> @@ -112,24 +118,24 @@ endif
>
> ifeq ($(CONFIG_BITBANG_SPI), yes)
>  FEATURE_CFLAGS += -D'BITBANG_SPI_SUPPORT=1'
> -OBJS += bitbang_spi.o
> +PROGRAMMER_OBJS += bitbang_spi.o
>  endif
>
> ifeq ($(CONFIG_NIC3COM), yes)
>  FEATURE_CFLAGS += -D'NIC3COM_SUPPORT=1'
> -OBJS += nic3com.o
> +PROGRAMMER_OBJS += nic3com.o
>  NEED_PCI := yes
>  endif
>
> ifeq ($(CONFIG_GFXNVIDIA), yes)
>  FEATURE_CFLAGS += -D'GFXNVIDIA_SUPPORT=1'
> -OBJS += gfxnvidia.o
> +PROGRAMMER_OBJS += gfxnvidia.o
>  NEED_PCI := yes
>  endif
>
> ifeq ($(CONFIG_SATASII), yes)
>  FEATURE_CFLAGS += -D'SATASII_SUPPORT=1'
> -OBJS += satasii.o
> +PROGRAMMER_OBJS += satasii.o
>  NEED_PCI := yes
>  endif
>
> @@ -138,47 +144,49 @@ ifeq ($(CONFIG_FT2232SPI), yes)
>  # This is a totally ugly hack.
>  FEATURE_CFLAGS += $(shell LC_ALL=C grep -q "FTDISUPPORT := yes" .features && printf "%s" "-D'FT2232_SPI_SUPPORT=1'")
>  FEATURE_LIBS += $(shell LC_ALL=C grep -q "FTDISUPPORT := yes" .features && printf "%s" "$(FTDILIBS)")
> -OBJS += ft2232_spi.o
> +PROGRAMMER_OBJS += ft2232_spi.o
>  endif
>
> ifeq ($(CONFIG_DUMMY), yes)
>  FEATURE_CFLAGS += -D'DUMMY_SUPPORT=1'
> -OBJS += dummyflasher.o
> +PROGRAMMER_OBJS += dummyflasher.o
>  endif
>
> ifeq ($(CONFIG_DRKAISER), yes)
>  FEATURE_CFLAGS += -D'DRKAISER_SUPPORT=1'
> -OBJS += drkaiser.o
> +PROGRAMMER_OBJS += drkaiser.o
>  NEED_PCI := yes
>  endif
>
> ifeq ($(CONFIG_BUSPIRATESPI), yes)
>  FEATURE_CFLAGS += -D'BUSPIRATE_SPI_SUPPORT=1'
> -OBJS += buspirate_spi.o
> +PROGRAMMER_OBJS += buspirate_spi.o
>  endif
>
> # Ugly, but there's no elif/elseif.
>  ifeq ($(CONFIG_SERPROG), yes)
> -OBJS += serial.o
> +LIB_OBJS += serial.o
>  else
>  ifeq ($(CONFIG_BUSPIRATESPI), yes)
> -OBJS += serial.o
> +LIB_OBJS += serial.o
>  endif
>  endif
>
> ifeq ($(NEED_PCI), yes)
>  FEATURE_CFLAGS += -D'NEED_PCI=1'
> -OBJS += pcidev.o
> +LIB_OBJS += pcidev.o
>  endif

We need to move stuff from internal.c and pcidev.c to pci.c, but that's
for another patch.
> ifeq ($(CONFIG_PRINT_WIKI), yes)
>  FEATURE_CFLAGS += -D'PRINT_WIKI_SUPPORT=1'
> -OBJS += print_wiki.o
> +CLI_OBJS += print_wiki.o
>  endif
>
> # We could use PULLED_IN_LIBS, but that would be ugly.
>  FEATURE_LIBS += $(shell LC_ALL=C grep -q "NEEDLIBZ := yes" .libdeps && printf "%s" "-lz")
>
> +OBJS = $(CHIP_OBJS) $(CLI_OBJS) $(PROGRAMMER_OBJS) $(LIB_OBJS)
>
> $(PROGRAM): $(OBJS)
>    $(CC) $(LDFLAGS) -o $(PROGRAM) $(OBJS) $(FEATURE_LIBS) $(LIBS)

Looks good otherwise.

Regards,
Carl-Daniel

[Sean Nelson]

This patch shouldn't affect anything else in patchwork. It just splits 
$(OBJS) in Makefile into separate lists for Programmer, Chip, and CLI 
related files/objects. This should help later on figuring out where 
files may go for a libflashrom library.

Diff'ed against r823.

[Carl-Daniel Hailfinger]

On 24.12.2009 07:14, Sean Nelson wrote:
> This patch shouldn't affect anything else in patchwork. It just splits
> $(OBJS) in Makefile into separate lists for Programmer, Chip, and CLI
> related files/objects. This should help later on figuring out where
> files may go for a libflashrom library.
>
> Diff'ed against r823.
>
> diff --git a/Makefile b/Makefile
> index 99625e7..4e623eb 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -41,11 +41,17 @@ CFLAGS += -I/usr/local/include
>  LDFLAGS += -L/usr/local/lib
>  endif
>
> -OBJS = jedec.o stm50flw0x0x.o w39v080fa.o sharplhf00l04.o w29ee011.o \
> +LIBS += -lpci

Mismerge of r823? -lpci should not be added conditionally.
> +CHIP_OBJS = jedec.o stm50flw0x0x.o w39v080fa.o sharplhf00l04.o w29ee011.o \
>    sst28sf040.o am29f040b.o mx29f002.o m29f400bt.o pm29f002.o w39v040c.o \
> w49f002u.o 82802ab.o pm49fl00x.o sst49lf040.o en29f002a.o m29f002.o \
> sst49lfxxxc.o sst_fwhub.o flashchips.o layout.o spi.o \
> flashrom.o print.o udelay.o
> w49f002u.o 82802ab.o pm49fl00x.o sst49lf040.o m29f002.o \
> sst49lfxxxc.o sst_fwhub.o flashchips.o spi.o
>
> +CLI_OBJS = flashrom.o print.o
>
> +LIB_OBJS = layout.o
>
> +PROGRAMMER_OBJS = udelay.o
>
> all: pciutils features dep $(PROGRAM)
>
> @@ -96,13 +102,13 @@ CONFIG_PRINT_WIKI ?= no
>
> ifeq ($(CONFIG_INTERNAL), yes)
>  FEATURE_CFLAGS += -D'INTERNAL_SUPPORT=1'
> -OBJS += chipset_enable.o board_enable.o cbtable.o it87spi.o ichspi.o sb600spi.o wbsio_spi.o
> +PROGRAMMER_OBJS += chipset_enable.o board_enable.o cbtable.o it87spi.o ichspi.o sb600spi.o wbsio_spi.o
>  NEED_PCI := yes
>  endif
>
> ifeq ($(CONFIG_SERPROG), yes)
>  FEATURE_CFLAGS += -D'SERPROG_SUPPORT=1'
> -OBJS += serprog.o
> +LIB_OBJS += serprog.o
>  ifeq ($(OS_ARCH), SunOS)
>  LIBS += -lsocket
>  endif
> @@ -110,24 +116,24 @@ endif
>
> ifeq ($(CONFIG_BITBANG_SPI), yes)
>  FEATURE_CFLAGS += -D'BITBANG_SPI_SUPPORT=1'
> -OBJS += bitbang_spi.o
> +PROGRAMMER_OBJS += bitbang_spi.o
>  endif
>
> ifeq ($(CONFIG_NIC3COM), yes)
>  FEATURE_CFLAGS += -D'NIC3COM_SUPPORT=1'
> -OBJS += nic3com.o
> +PROGRAMMER_OBJS += nic3com.o
>  NEED_PCI := yes
>  endif
>
> ifeq ($(CONFIG_GFXNVIDIA), yes)
>  FEATURE_CFLAGS += -D'GFXNVIDIA_SUPPORT=1'
> -OBJS += gfxnvidia.o
> +PROGRAMMER_OBJS += gfxnvidia.o
>  NEED_PCI := yes
>  endif
>
> ifeq ($(CONFIG_SATASII), yes)
>  FEATURE_CFLAGS += -D'SATASII_SUPPORT=1'
> -OBJS += satasii.o
> +PROGRAMMER_OBJS += satasii.o
>  NEED_PCI := yes
>  endif
>
> @@ -136,48 +142,54 @@ ifeq ($(CONFIG_FT2232SPI), yes)
>  # This is a totally ugly hack.
>  FEATURE_CFLAGS += $(shell LC_ALL=C grep -q "FTDISUPPORT := yes" .features && printf "%s" "-D'FT2232_SPI_SUPPORT=1'")
>  FEATURE_LIBS += $(shell LC_ALL=C grep -q "FTDISUPPORT := yes" .features && printf "%s" "$(FTDILIBS)")
> -OBJS += ft2232_spi.o
> +PROGRAMMER_OBJS += ft2232_spi.o
>  endif
>
> ifeq ($(CONFIG_DUMMY), yes)
>  FEATURE_CFLAGS += -D'DUMMY_SUPPORT=1'
> -OBJS += dummyflasher.o
> +PROGRAMMER_OBJS += dummyflasher.o
>  endif
>
> ifeq ($(CONFIG_DRKAISER), yes)
>  FEATURE_CFLAGS += -D'DRKAISER_SUPPORT=1'
> -OBJS += drkaiser.o
> +PROGRAMMER_OBJS += drkaiser.o
>  NEED_PCI := yes
>  endif
>
> ifeq ($(CONFIG_BUSPIRATESPI), yes)
>  FEATURE_CFLAGS += -D'BUSPIRATE_SPI_SUPPORT=1'
> -OBJS += buspirate_spi.o
> +PROGRAMMER_OBJS += buspirate_spi.o
>  endif
>
> # Ugly, but there's no elif/elseif.
>  ifeq ($(CONFIG_SERPROG), yes)
> -OBJS += serial.o
> +LIB_OBJS += serial.o
>  else
>  ifeq ($(CONFIG_BUSPIRATESPI), yes)
> -OBJS += serial.o
> +LIB_OBJS += serial.o
>  endif
>  endif
>
> ifeq ($(NEED_PCI), yes)
> -LIBS += -lpci
>  FEATURE_CFLAGS += -D'NEED_PCI=1'
> -OBJS += pcidev.o physmap.o internal.o
> +LIB_OBJS += pcidev.o 					#FIXME: We need to move stuff from
> 								# internal.c and pcidev.c to pci.c
>
> +PROGRAMMER_OBJS += physmap.o internal.o #FIXME: internal.c needs to be split

Agreed, the move needs to happen. Not sure if physmap.o qualifies as
LIB_OBJS (IIRC used by all PCI programmers as well).
> 								# into internal-programmer-only stuff
> 								# and a support lib for all internal+pci
> 								# based stuff.

Is it possible that you have tab width set to 4 or somesuch? The comment
indentation seems to be inconsistent when viewed with tab width 8.
> endif
>
> ifeq ($(CONFIG_PRINT_WIKI), yes)
>  FEATURE_CFLAGS += -D'PRINT_WIKI_SUPPORT=1'
> -OBJS += print_wiki.o
> +CLI_OBJS += print_wiki.o
>  endif
>
> # We could use PULLED_IN_LIBS, but that would be ugly.
>  FEATURE_LIBS += $(shell LC_ALL=C grep -q "NEEDLIBZ := yes" .libdeps && printf "%s" "-lz")
>
> +OBJS = $(CHIP_OBJS) $(CLI_OBJS) $(PROGRAMMER_OBJS) $(LIB_OBJS)
>
> $(PROGRAM): $(OBJS)
>    $(CC) $(LDFLAGS) -o $(PROGRAM) $(OBJS) $(FEATURE_LIBS) $(LIBS)

The stuff below reverts r823. Is that intentional?
> @@ -210,7 +222,6 @@ compiler:
>    	rm -f .test.c .test; exit 1)
>    @rm -f .test.c .test
>
> -ifeq ($(NEED_PCI), yes)
>  pciutils: compiler
>    @printf "Checking for libpci headers... "
>    @$(shell ( echo "#include <pci/pci.h>";		   \
> @@ -226,30 +237,25 @@ pciutils: compiler
>    @$(shell ( echo "#include <pci/pci.h>";		   \
>    	   echo "int main(int argc, char **argv)"; \
>    	   echo "{ return 0; }"; ) > .test1.c )
> @$(CC) $(CFLAGS) $(LDFLAGS) .test1.c -o .test1 -lpci $(LIBS) >/dev/null 2>&1 &&	\
> @$(CC) $(CFLAGS) $(LDFLAGS) .test1.c -o .test1 $(LIBS) >/dev/null 2>&1 &&	\
> echo "found." || ( echo "not found."; echo;				\
> echo "Please install libpci (package pciutils).";			\
> echo "See README for more information."; echo;				\
> rm -f .test1.c .test1; exit 1)
> @printf "Checking if libpci is sufficient... "
> @printf "" > .libdeps
> @$(CC) $(LDFLAGS) .test.o -o .test -lpci $(LIBS) >/dev/null 2>&1 &&				\
> @$(CC) $(LDFLAGS) .test.o -o .test $(LIBS) >/dev/null 2>&1 &&				\
> echo "yes." || ( echo "no.";							\
> printf "Checking if libz is present and supplies all needed symbols...";	\
> $(CC) $(LDFLAGS) .test.o -o .test -lpci -lz $(LIBS) >/dev/null 2>&1 &&		\
> $(CC) $(LDFLAGS) .test.o -o .test $(LIBS) -lz >/dev/null 2>&1 &&		\
> ( echo "yes."; echo "NEEDLIBZ := yes" > .libdeps ) || ( echo "no."; echo;	\
> echo "Please install libz.";			\
> echo "See README for more information."; echo;				\
> rm -f .test.c .test.o .test; exit 1) )
> @rm -f .test.c .test.o .test .test1.c .test1
>
> -else
> -pciutils: compiler
> @printf "" > .libdeps
>
> -endif
>
> .features: features
>
> -ifeq ($(CONFIG_FT2232SPI), yes)
>  features: compiler
>    @echo "FEATURES := yes" > .features.tmp
>    @printf "Checking for FTDI support... "
> @@ -262,10 +268,6 @@ features: compiler
>    	( echo "not found."; echo "FTDISUPPORT := no" >> .features.tmp )
>    @$(DIFF) -q .features.tmp .features >/dev/null 2>&1 && rm .features.tmp || mv .features.tmp .features
>    @rm -f .featuretest.c .featuretest
> -else
> -features: compiler
> @echo "FEATURES := yes" > .features
>
> -endif
>
> install: $(PROGRAM)
>    mkdir -p $(DESTDIR)$(PREFIX)/sbin

Looks good otherwise.

Regards,
Carl-Daniel

[Sean Nelson]

Lets try this patch again: Fixed indentation. Rediff'ed against 
svn-actual r823.

This patch shouldn't affect anything else in patchwork. It just splits 
$(OBJS) in Makefile into separate lists for Programmer, Chip, and CLI 
related files/objects. This should help later on figuring out where 
files may go for a libflashrom library.

Signed-off-by: Sean Nelson

[Carl-Daniel Hailfinger]

On 24.12.2009 17:30, Sean Nelson wrote:
> Lets try this patch again: Fixed indentation. Rediff'ed against
> svn-actual r823.
>
> This patch shouldn't affect anything else in patchwork. It just splits
> $(OBJS) in Makefile into separate lists for Programmer, Chip, and CLI
> related files/objects. This should help later on figuring out where
> files may go for a libflashrom library.
>
> Signed-off-by: Sean Nelson

Acked-by: Carl-Daniel Hailfinger

Go ahead and commit!

Regards,
Carl-Daniel

[Sean Nelson]

On 12/24/09 8:52 AM, Carl-Daniel Hailfinger wrote:
> Acked-by: Carl-Daniel Hailfinger

Thanks, r824.