[U-Boot] [PATCH v2] Add a CBFS driver and commands to u-boot

[Gabe Black]

On Tue, Dec 6, 2011 at 3:01 AM, Wolfgang Denk <wd at denx.de> wrote:

> Dear Gabe Black,
>
> In message <1323134730-18471-1-git-send-email-gabeblack at 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 at 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=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