[U-Boot] [PATCH v4 6/6] common: Generic loader for file system

[Chee, Tien Fong]

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 <tien.fong.chee at intel.com>
> wrote:
> > 
> > On Wed, 2018-07-11 at 14:13 -0600, Simon Glass wrote:
> > > 
> > > Hi Tien,
> > > 
> > > On 6 July 2018 at 02:28,  <tien.fong.chee at intel.com> wrote:
> > > > 
> > > > 
> > > > From: Tien Fong Chee <tien.fong.chee at 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 at intel.com>
> > > > ---
> > > >  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