[U-Boot] [PATCH 2/2 v12] Introduced btrfs file-system with btrload command

[Wolfgang Denk]

Dear Adnan Ali,

In message <1367660512-10489-3-git-send-email-adnan9901 at yahoo.com> you wrote:

...
 include/crc.h              |    8 +
 include/fs.h               |    1 +
 lib/Makefile               |    1 +
 lib/crc32_c.c              |   40 ++
...

These changes should be factored out into a separate commit.
In any case, the code should be compiled in only when needed, i. e.
when btrfs support is selected.  Otherwise you just bloat the code
size and build time for all systems without need.

Hm... do we really need yet another crc32 implementation?



I still see an checkpatch error and warnings; also incorrect
multi-line comments - did these slip through, or is it intentional
not to fix these? Why?


> +U_BOOT_CMD(
> +btrload,        7,      0,      do_btr_fsload,
> +	"load binary file from a btr filesystem",
--------------^^^^^^
> +	"<interface> [<dev[:part]>]  <addr> <filename> [subvol_name]\n"
> +	"    - Load binary file 'filename' from 'dev' on 'interface'\n"
--------------------^^^^^^

Please remove the "binary" here.  I guess we can also load text files
and any other file tpes as well.

Also please decide how to call the B-Tree file system in descriptions
like this.  IIRC, Linux tends to write "btrfs" in cases like this one.

Which makes: "load file from btrfs file system", etc.


> +	"      to address 'addr' from better filesystem.\n"

"better filesystem"? "BTRFS" means B-tree file system.  Pronounciation
may sound like this, but we don't write it that way.

i. e. something like:

	"Load file 'filename' from btrfs file system on 'dev' on
	'interface' to address 'addr'"

> +	"      the load stops on end of file.\n"

Would it be easy to add support for loading only a certain (smaller)
lenght?

> +	"      subvol_name is used read that file from this subvolume.\n"

It seems this sentence is not complete.

> +	"      All numeric parameters are assumed to be hex."

Drop this.  This is standard in U-Boot.


> +++ b/lib/crc32_c.c
> @@ -0,0 +1,40 @@
> +/*
> + * Copied from Linux kernel crypto/crc32c.c

Which exact version of the Linux kernel has this been copied from?
The current version of crypto/crc32c.c does not contain any such code
at all.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Real programmers don't comment their code. It was hard to  write,  it
should be hard to understand.