On Tue, Dec 6, 2011 at 3:01 AM, Wolfgang Denk wd@denx.de wrote:
Dear Gabe Black,
In message 1323134730-18471-1-git-send-email-gabeblack@chromium.org you wrote:
Coreboot uses a very simple "file system" called CBFS to keep track of
and
allow access to multiple "files" in a ROM image. This change adds CBFS support and some commands to use it to u-boot. These commands are:
cbfsinit - Initialize CBFS support and pull all metadata into RAM. The
end
of the ROM is an optional parameter which defaults to the standard 0xffffffff and can be used to support multiple CBFSes in a system. The
last
one set up with cbfsinit is the one that will be used.
cbfsinfo - Print information from the CBFS header.
cbfsls - Print out the size, type, and name of all the files in the
current
CBFS. Recognized types are translated into symbolic names.
cbfsload - Load a file from CBFS into memory. Like the similar command
for
fat filesystems, you can optionally provide a maximum size.
Also, if u-boot needs something out of CBFS very early before the heap is configured, it won't be able to use the normal CBFS support which caches some information in memory it allocates from the heap. This change adds a new cbfs_file_find_uncached function which searchs a CBFS instance
without
touching the heap.
Support for CBFS is compiled in when the CONFIG_CMD_CBFS option is specified.
Signed-off-by: Gabe Black gabeblack@chromium.org
Changes in v2: Fix checkpatch problems, change around identifiers, and change printf to puts where possible.
There is still a checkpatch warning that should be fixed:
WARNING: do not add new typedefs #853: FILE: include/cbfs.h:61: +typedef const struct cbfs_cache_node *cbfs_file;
This typedef is to create an opaque handle for CBFS files. If you look here:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=Do...
In chapter 5 in the list of acceptable uses for typedefs, creating an opaque type is item a. This use of typedef is consistent with the Linux coding standards as described in that document and functionally important for this change and should stay.
+int do_cbfs_init(cmd_tbl_t *cmdtp, int flag, int argc, char *const
argv[])
+{
uintptr_t end_of_rom = 0xffffffff;char *ep;if (argc > 2) {puts("usage: cbfsls [end of rom]>\n");What is the meaning / intention of that tailing '>' ?
A typo.
if (file_cbfs_result != CBFS_SUCCESS) {printf("%s.\n", file_cbfs_error());Use puts(file_cbfs_error()); ?
That would leave out the \n. Whatever came next (like the prompt) would continue on the same line which would be wrong.
+int do_cbfs_fsload(cmd_tbl_t *cmdtp, int flag, int argc, char *const
argv[])
+{
...
if (!file) {if (file_cbfs_result == CBFS_FILE_NOT_FOUND)printf("%s: %s\n", file_cbfs_error(), argv[2]);elseprintf("%s.\n", file_cbfs_error());See above. Please consider changing globally.
The same explanation applies globally.
...
if (type_name)printf(" %16s", type_name);elseprintf(" %16d", type);This is probably confusing to the user. How can he know if the file type has the numerical value of "123" or if the file name is "123" ?
This isn't file name, it's the file type. No file type is named after a number. There are also labels on the columns indicating which is which.
if (filename[0])printf(" %s\n", filename);elseprintf(" %s\n", "(empty)");Use puts().
The first one can't be a puts, it would have to be three. That's a lot of clutter to avoid a slight performance penalty when interacting with a human that won't notice such a tiny delay anyway. The second one could be a puts, but that would make the two branches of the if uneven. I'll change the second one.
printf("\n%d file(s)\n\n", files);Please do not print that many empty lines.
Imagine output is going to a QVGA display or similar which shows only 12 text lines or so...
I put newlines there because the FAT commands do, and I have no problem removing them. You may want to look at the other commands and remove them there too.
puts("\n");What is this blank line good for? Drop it!
Readability? I don't care that much one way or the other though. I'll change it.
Gabe