Re: [U-Boot] [PATCH 3/5] jz4740 nand spl files

Dear Xiangfu Liu,

In message 1289446657-12499-4-git-send-email-xiangfu@openmobilefree.net you wrote:

From: Xiangfu Liu xiangfu@sharism.cc

Signed-off-by: Xiangfu Liu xiangfu@openmobilefree.net

nand_spl/board/xburst/nanonote/Makefile | 96 +++++++ nand_spl/board/xburst/nanonote/u-boot.lds | 63 +++++ nand_spl/nand_boot_jz4740.c | 395 +++++++++++++++++++++++++++++ 3 files changed, 554 insertions(+), 0 deletions(-) create mode 100644 nand_spl/board/xburst/nanonote/Makefile create mode 100644 nand_spl/board/xburst/nanonote/u-boot.lds create mode 100644 nand_spl/nand_boot_jz4740.c

You need to rework the splitting of your code into commits - as is, these patches make no sense to me. Please re-read http://www.denx.de/wiki/view/U-Boot/Patches#General_Patch_Submission_Rules

diff --git a/nand_spl/nand_boot_jz4740.c b/nand_spl/nand_boot_jz4740.c new file mode 100644

...

+#define __nand_enable() (REG_EMC_NFCSR |= EMC_NFCSR_NFE1 | EMC_NFCSR_NFCE1) +#define __nand_disable() (REG_EMC_NFCSR &= ~(EMC_NFCSR_NFCE1)) +#define __nand_ecc_rs_encoding() \

• (REG_EMC_NFECR = EMC_NFECR_ECCE | EMC_NFECR_ERST | EMC_NFECR_RS | EMC_NFECR_RS_ENCODING)

+#define __nand_ecc_rs_decoding() \

• (REG_EMC_NFECR = EMC_NFECR_ECCE | EMC_NFECR_ERST | EMC_NFECR_RS | EMC_NFECR_RS_DECODING)

+#define __nand_ecc_disable() (REG_EMC_NFECR &= ~EMC_NFECR_ECCE) +#define __nand_ecc_encode_sync() while (!(REG_EMC_NFINTS & EMC_NFINTS_ENCF)) +#define __nand_ecc_decode_sync() while (!(REG_EMC_NFINTS & EMC_NFINTS_DECF)) +#define __nand_cmd(n) (REG8(NAND_COMMPORT) = (n)) +#define __nand_addr(n) (REG8(NAND_ADDRPORT) = (n)) +#define __nand_data8() REG8(NAND_DATAPORT) +#define __nand_data16() REG16(NAND_DATAPORT)

You seem to have a tendency to use "__" a lot in front of indentifiers. The C standard says:

-- All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use.

-- All identifiers that begin with an underscore are always reserved for use as identifiers with file scope in both the ordinary and tag name spaces.

Please recheck your identifier names with this in mind.

As mentioned before, this code needs to be converted to use I/O accessors instead of these REG*() volatile pointer accesses.

Both comments apply globally.

...

• /* Check result of decoding */
  

• stat = REG_EMC_NFINTS;
  

• if (stat & EMC_NFINTS_ERR) {
  

• 	/* Error occurred */
  

• 	/* serial_puts("Error occurred\n"); */
  

• 	if (stat & EMC_NFINTS_UNCOR) {
  

• 		/* Uncorrectable error occurred */
  

• 		/* serial_puts("Uncorrectable error occurred\n"); */
  

• 	} else {
  

• 		unsigned int errcnt, index, mask;
  

• 		errcnt = (stat & EMC_NFINTS_ERRCNT_MASK) >> EMC_NFINTS_ERRCNT_BIT;
  

• 		switch (errcnt) {
  

• 		case 4:
  

• 			index = (REG_EMC_NFERR3 & EMC_NFERR_INDEX_MASK) >> EMC_NFERR_INDEX_BIT;
  

• 			mask = (REG_EMC_NFERR3 & EMC_NFERR_MASK_MASK) >> EMC_NFERR_MASK_BIT;
  

• 			rs_correct(tmpbuf, index, mask);
  

• 			/* FALL-THROUGH */
  

• 		case 3:
  

• 			index = (REG_EMC_NFERR2 & EMC_NFERR_INDEX_MASK) >> EMC_NFERR_INDEX_BIT;
  

• 			mask = (REG_EMC_NFERR2 & EMC_NFERR_MASK_MASK) >> EMC_NFERR_MASK_BIT;
  

• 			rs_correct(tmpbuf, index, mask);
  

• 			/* FALL-THROUGH */
  

• 		case 2:
  

• 			index = (REG_EMC_NFERR1 & EMC_NFERR_INDEX_MASK) >> EMC_NFERR_INDEX_BIT;
  

• 			mask = (REG_EMC_NFERR1 & EMC_NFERR_MASK_MASK) >> EMC_NFERR_MASK_BIT;
  

• 			rs_correct(tmpbuf, index, mask);
  

• 			/* FALL-THROUGH */
  

• 		case 1:
  

• 			index = (REG_EMC_NFERR0 & EMC_NFERR_INDEX_MASK) >> EMC_NFERR_INDEX_BIT;
  

• 			mask = (REG_EMC_NFERR0 & EMC_NFERR_MASK_MASK) >> EMC_NFERR_MASK_BIT;
  

• 			rs_correct(tmpbuf, index, mask);
  

• 			break;
  

Lines too long.

I stop reviewing here.

Please fix the code, and resubmit.

Thanks.

Best regards,

Wolfgang Denk