[U-Boot-Users] [PATCH 1/5] OneNAND support

[Wolfgang Denk <wd@denx.de>]

In message <20070907005723.GA19727@party> you wrote:
> [PATCH] OneNAND support
> 
> This patch enables the OneNAND at U-boot

I cannot comment much on this code as I have no access to any hardware
with such a device, but here are a few formal comments:

> +U_BOOT_CMD (onenand, 6, 1, do_onenand,
> +	    "onenand - OneNAND sub-system\n",
> +	    "info   - show available OneNAND devices\n"
> +	    "onenand read[.oob] addr ofs len - read data at ofs with len to addr\n"
> +	    "onenand write addr ofs len - write data at ofs with len from addr\n"
> +	    "onenand erase block start-end - erase block from start to end\n"
> +	    "onenand erase saddr eaddr - erase block start addr to end addr\n"
> +	    "onenand block[.oob] addr block [page] [len] - "
> +	    "read data with (block [, page]) to addr\n"

This line is probably missing some additional indentation?

> +	    "onenand unlock start-end - unlock block from start to end\n"
> +	    "onenand save bootloader addr - save bootloader at addr\n");

I'm not happy with the parameter format of the "erase"  and  "unlock"
commands  -  using  an  address  specification  of "start-end" breaks
existing conventions in U-Boot. Please  use  two  separate  arguments
instead.

Also, I don;t see what  the  difference  between  "erase  block"  and
"erase"  is  -  can  we  avoid  "erase block" all together? If not, I
recommend to  change  it  into  "eraseblock"  (which  could  then  be
shortened to "eraseb") to avoid too different command formats.


After changing this, you have my ACK.

Thanks a lot.

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
In the beginning there was nothing.
And the Lord said "Let There Be Light!"
And still there was nothing, but at least now you could see it.