From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chee, Tien Fong Date: Tue, 17 Jul 2018 04:46:36 +0000 Subject: [U-Boot] [PATCH v4 6/6] common: Generic loader for file system In-Reply-To: References: <1530865683-23207-1-git-send-email-tien.fong.chee@intel.com> <1531379941.9615.16.camel@intel.com> Message-ID: <1531802795.9620.9.camel@intel.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de On Sun, 2018-07-15 at 15:21 -0600, Simon Glass wrote: > Hi Tien Fong, > > On 12 July 2018 at 01:19, Chee, Tien Fong > wrote: > > > > On Wed, 2018-07-11 at 14:13 -0600, Simon Glass wrote: > > > > > > Hi Tien, > > > > > > On 6 July 2018 at 02:28,   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 > > > > --- > > > >  drivers/misc/Kconfig     |  10 ++ > > > >  drivers/misc/Makefile    |   1 + > > > >  drivers/misc/fs_loader.c | 295 > > > > +++++++++++++++++++++++++++++++++++++++++++++++ > > > >  include/dm/uclass-id.h   |   1 + > > > >  include/fs_loader.h      |  79 +++++++++++++ > > > >  5 files changed, 386 insertions(+) > > > >  create mode 100644 drivers/misc/fs_loader.c > > > >  create mode 100644 include/fs_loader.h > > > > > [..] > > > > > > > > > > > > > > > > > > > > > +               (*firmwarep)->priv = calloc(1, sizeof(struct > > > > firmware_priv)); > > > > +               if (!(*firmwarep)->priv) { > > > > +                       free(*firmwarep); > > > > +                       return -ENOMEM; > > > > +               } > > > > +       } > > > > + > > > > +       ((struct firmware_priv *)((*firmwarep)->priv))->name = > > > > name; > > > Again this needs a local variable. > > Why? > Because these casts are really ugly and repetitive, particularly when > used for assignments :-) Okay. > > [..] > > > > > > > > > > > > > > + * release_firmware - Release the resource associated with a > > > > firmware image > > > > + * @firmware: Firmware resource to release > > > > + */ > > > > +void release_firmware(struct firmware *firmware); > > > > + > > > > +/** > > > > + * request_firmware_into_buf - Load firmware into a previously > > > > allocated buffer. > > > > + * @plat: Platform data such as storage and partition firmware > > > > loading from. > > > > + * @name: Name of firmware file. > > > > + * @buf: Address of buffer to load firmware into. > > > > + * @size: Size of buffer. > > > > + * @offset: Offset of a file for start reading into buffer. > > > > + * @firmwarep: Pointer to firmware image. > > > > + * > > > > + * The firmware is loaded directly into the buffer pointed to > > > > by > > > > @buf and > > > > + * the @firmwarep data member is pointed at @buf. > > > > + * > > > > + * Return: Size of total read, negative value when error. > > > > + */ > > > > +int request_firmware_into_buf(struct device_platdata *plat, > > > > +                             const char *name, > > > > +                             void *buf, size_t size, u32 > > > > offset, > > > > +                             struct firmware **firmwarep); > > > Without a test it's hard to see how this is used, but I'm not > > > keen on > > > the struct firmware ** here. > > > > > > Can you just use struct firmware * instead? > > > > > > Or maybe just return the fields individually since you only have > > > start > > > address and size, I think. > > I can try to remove struct firmware, let driver model handles all > > memory allocation, and then struct device_platdata *plat will > > change > > to struct udevice *dev. So, it would become like this > > int request_firmware_into_buf(struct udevice *dev, > >                               const char *name, > >                               void *buf, size_t size, u32 offset); > > I will remove release_firmware() after letting driver model to > > handle > > all memory deallocation. > > So, the API interface would looks a bit different compare to Linux > > firmware API interface. Does this change OK for you? > I believe you would need: > > > > > int request_firmware_into_buf(struct udevice *dev, > >                               const char *name, > >                               void **bufp, size_t *sizep, u32 > > offset); > since you need to return the buffer and size? Why need to return the buffer and size? There is no modification required on noth buffer and size arguments by request_firmware_into_buf(). Both buffer and size are allocated and defined by user, then request_firmware_into_buf() would load the file into buffer based on the size exactly defined by user. > > What exactly is 'dev'? Is it the device in the FS_LOADER uclass? Yes, contains FDT HW data. > > Regards, > Simon