Hi Tien Fong,
On 12 July 2018 at 01:19, Chee, Tien Fong <tien.fong.c...@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.c...@intel.com> 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>
>> > ---
>> > 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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
