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

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