From mboxrd@z Thu Jan  1 00:00:00 1970
From: Chee, Tien Fong <tien.fong.chee@intel.com>
Date: Thu, 18 Jan 2018 03:42:14 +0000
Subject: [U-Boot] [U-Boot, v6,
	2/2] common: Generic firmware loader for file system
In-Reply-To: <20180116143504.GO4660@bill-the-cat>
References: <1514351078-3487-3-git-send-email-tien.fong.chee@intel.com>
 <20180115163603.GO4660@bill-the-cat> <1516089479.32057.4.camel@intel.com>
 <20180116143504.GO4660@bill-the-cat>
Message-ID: <1516246934.2704.7.camel@intel.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
To: u-boot@lists.denx.de

On Tue, 2018-01-16 at 09:35 -0500, Tom Rini wrote:
> On Tue, Jan 16, 2018 at 07:58:00AM +0000, Chee, Tien Fong wrote:
> > 
> > On Mon, 2018-01-15 at 11:36 -0500, Tom Rini wrote:
> > > 
> > > On Wed, Dec 27, 2017 at 01:04:38PM +0800, tien.fong.chee at intel.co
> > > m
> > > wrote:
> > > 
> > > > 
> > > > 
> > > > From: Tien Fong Chee <tien.fong.chee@intel.com>
> > > > 
> > > > This is file system generic loader which can be used to load
> > > > the file image from the storage into target such as memory.
> > > > The consumer driver would then use this loader to program
> > > > whatever,
> > > > ie. the FPGA device.
> > > > 
> > > > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
> > > Please add Lothar's Reviewed-by for v7.  There's a number of
> > > minor
> > > checkpatch.pl issues that checkpatch.pl can in turn fixup itself,
> > > please
> > > correct them.
> > > 
> > I have ran the checkpatch.pl on this patch, i didn't see any error.
> When I ran it, it was pointing out cases where you have:
> if (foo->bar != NULL)
> when you can just use:
> if (foo->bar)
> 
It's weird for checkpatch.pl not showing the same. I would fix the code
with above example.
> [snip]
> > 
> > > 
> > > > 
> > > > diff --git a/common/fs_loader.c b/common/fs_loader.c
> > > > new file mode 100644
> > > > index 0000000..56d29b6
> > > > --- /dev/null
> > > > +++ b/common/fs_loader.c
> > > > @@ -0,0 +1,309 @@
> > > > +/*
> > > > + * Copyright (C) 2017 Intel Corporation <www.intel.com>
> > > > + *
> > > > + * SPDX-License-Identifier:    GPL-2.0
> > > > + */
> > > > +
> > > > +#include <common.h>
> > > > +#include <errno.h>
> > > > +#include <fs.h>
> > > > +#include <fs_loader.h>
> > > > +#include <nand.h>
> > > > +#include <sata.h>
> > > > +#include <spi.h>
> > > > +#include <spi_flash.h>
> > > > +#include <spl.h>
> > > This wants <asm/spl.h> which is not globally available, so you
> > > need
> > > to
> > > come up with something here.  At least making this Kconfig-
> > > enabled
> > > will
> > > be a start and perhaps OK for now.
> > > 
> > I can enable the Kconfig, and put the caution about dependency on
> > <asm/spl.h> in document.
> Well, you need to make that part of the code depend on CONFIG_SPL as
> SPL
> support requires <asm/spl.h> to exist.   Perhaps part of the code
> needs
> to be refactored to more easily deal with SPL not always being
> present?
> 
How about i just move the whole enum { } from line 17 to line 35 in
asm/spl.h to fs.h, and then enum{} above replaced with #include <fs.h>?

asm/spl.h
----------
#if defined(CONFIG_ARCH_OMAP2PLUS) \
	|| defined(CONFIG_EXYNOS4) || defined(CONFIG_EXYNOS5) \
	|| defined(CONFIG_EXYNOS4210)
/* Platform-specific defines */
#include <asm/arch/spl.h>

#else
#include <fs.h>  <-- replacement
#endif
[...]