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.c...@intel.co > > > > m > > > > wrote: > > > > > > > > > > > > > > > > > > > From: Tien Fong Chee <tien.fong.c...@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.c...@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.
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 <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
> [...]
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.h>
... SPL specific functions
#endif
--
Tom
signature.asc
Description: PGP signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
