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

[Simon Glass]

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 :-)

[..]

>> > + * 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?

What exactly is 'dev'? Is it the device in the FS_LOADER uclass?

Regards,
Simon