From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gabe Black Date: Tue, 6 Dec 2011 15:32:57 -0800 Subject: [U-Boot] [PATCH v2] Add a CBFS driver and commands to u-boot In-Reply-To: <20111206110152.784D91F9E48@gemini.denx.de> References: <20111205222505.22B4D18E00D9@gemini.denx.de> <1323134730-18471-1-git-send-email-gabeblack@chromium.org> <20111206110152.784D91F9E48@gemini.denx.de> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Tue, Dec 6, 2011 at 3:01 AM, Wolfgang Denk 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 > > --- > > 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=Documentation/CodingStyle 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]); > > + else > > + printf("%s.\n", file_cbfs_error()); > > See above. Please consider changing globally. > The same explanation applies globally. > ... > > + if (type_name) > > + printf(" %16s", type_name); > > + else > > + printf(" %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); > > + else > > + printf(" %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