From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tom Rini Date: Thu, 18 Jan 2018 08:18:44 -0500 Subject: [U-Boot] [U-Boot, v6, 2/2] common: Generic firmware loader for file system In-Reply-To: <1516246934.2704.7.camel@intel.com> 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> <1516246934.2704.7.camel@intel.com> Message-ID: <20180118131844.GK4660@bill-the-cat> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de On Thu, Jan 18, 2018 at 03:42:14AM +0000, Chee, Tien Fong wrote: > 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 > > > > > > > > > > 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 > > > > 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. That is odd. Are you running it from within the U-Boot tree? > > [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 > > > > > + * > > > > > + * SPDX-License-Identifier:    GPL-2.0 > > > > > + */ > > > > > + > > > > > +#include > > > > > +#include > > > > > +#include > > > > > +#include > > > > > +#include > > > > > +#include > > > > > +#include > > > > > +#include > > > > > +#include > > > > This wants 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 > > > in document. > > Well, you need to make that part of the code depend on CONFIG_SPL as > > SPL > > support requires 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 ? > > asm/spl.h > ---------- > #if defined(CONFIG_ARCH_OMAP2PLUS) \ > || defined(CONFIG_EXYNOS4) || defined(CONFIG_EXYNOS5) \ > || defined(CONFIG_EXYNOS4210) > /* Platform-specific defines */ > #include > > #else > #include  <-- replacement > #endif > [...] No, because that's still arch specific code and will blow up some platforms that do not have asm/spl.h at all and is still non-portable (look at the various asm/spl.h files that do exist for how BOOT_DEVICE_xxx is handled in different places). It's OK to restructure your code to have: #ifdef CONFIG_SPL #include ... SPL specific functions #endif -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: