Hi,
On Thu, 7 Dec 2017 08:10:06 +0000 Chee, Tien Fong wrote:
On Kha, 2017-12-07 at 08:49 +0100, Lothar Waßmann wrote:
Hi,
On Thu, 7 Dec 2017 05:29:24 +0000 Chee, Tien Fong wrote:
On Rab, 2017-12-06 at 12:00 +0100, Lothar Waßmann wrote:
Hi,
On Wed, 6 Dec 2017 10:06:21 +0000 Chee, Tien Fong wrote:
On Sel, 2017-12-05 at 09:53 +0100, Lothar Waßmann wrote:
Hi,
On Tue, 5 Dec 2017 15:57:57 +0800 tien.fong.chee@intel.com 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 > --- > common/Makefile | 1 + > common/fs_loader.c | 304 > ++++++++++++++++++++++++++++++++++++++++++++++++++++ > include/fs_loader.h | 30 ++++++ > 3 files changed, 335 insertions(+) > create mode 100644 common/fs_loader.c > create mode 100644 include/fs_loader.h > > diff --git a/common/Makefile b/common/Makefile > index 801ea31..419e915 100644 > --- a/common/Makefile > +++ b/common/Makefile > @@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o > obj-y += command.o > obj-y += s_record.o > obj-y += xyzModem.o > +obj-y += fs_loader.o > diff --git a/common/fs_loader.c b/common/fs_loader.c > new file mode 100644 > index 0000000..04f682b > --- /dev/null > +++ b/common/fs_loader.c > @@ -0,0 +1,304 @@ > +/* > + * 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> > +#include <linux/string.h> > +#include <usb.h> > + > +static struct device_location default_locations[] = { > + { > + .name = "mmc", > + .devpart = "0:1", > + }, > + { > + .name = "usb", > + .devpart = "0:1", > + }, > + { > + .name = "sata", > + .devpart = "0:1", > + }, > +}; > + > +/* USB build is not supported yet in SPL */ > +#ifndef CONFIG_SPL_BUILD > +#ifdef CONFIG_USB_STORAGE > +static int init_usb(void) > +{ > + int err = 0; > Useless initialization.
noted.
> > > > + err = usb_init(); > + > + if (err) > Unnecessary blank line.
Sorry, i'm not catching you because there is no blank line between "if" and "return"
I meant the blank line between 'err = ...' and 'if (err)'
Okay, i can change that.
> > > > + if (!strcmp(default_locations[i].name, > name)) > + default_locations[i].devpart = > devpart; > + } > + > + return; > +} > + > +/* > + * Prepare firmware struct; > + * return -ve if fail. > + */ > +static int _request_firmware_prepare(struct firmware > **firmware_p, > + const char *name, > void > *dbuf, > + size_t size, u32 > offset) > +{ > + struct firmware *firmware = NULL; > + int ret = 0; > + > + if (!name || name[0] == '\0') > + ret = -EINVAL; > + Is it really useful to continue here initializing the 'firmware' struct and returning an error at the end?
I try to keep it very close to Linux firmware loader. When more API ported in from Linux in future, this can be helper function. Anyway, i have no strong opinion, i can move to caller if you guys think that is better.
The Linux firmware loader has this:
if (!name || name[0] == '\0') { ret = -EINVAL; goto out; }
Note the 'goto out' which is missing in your code. If following the Linux code closely, you would have to set *firmware_p to NULL and exit in this case.
I can set the *firmware_p to NUll in failing case. But, i checked the static int _request_firmware_prepare in Linux, there is no goto out error handling method in the function. Fyi, there is no allocated memory release in U-Boot. https://elixir.free-electrons.com/linux/v4.13.15/source/drivers/bas e/fi rmware_class.c#L1191
I was referring to _request_firmware() which calls request_firmware_prepare() and does the checking of 'name' as your code does.
Ohh.....i skipped _request_firmware() because there is no error handling required in U-Boot such as memory release. For the checking at both firmware_p and name, i would rather to merge them into request_firware_prepare() because they are related to each other instead of creating one function just for checking purpose. What do you think?
I don't mind whether you combine those functions or not as long as you reproduce the same functionality which your patch currently does not do.
Lothar Waßmann