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

[Wolfgang Denk]

Dear Xiangfu Liu,

In message <1289446657-12499-4-git-send-email-xiangfu at openmobilefree.net> you wrote:
> From: Xiangfu Liu <xiangfu at sharism.cc>
> 
> Signed-off-by: Xiangfu Liu <xiangfu at 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

-- 
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
There's no honorable way to kill, no gentle way to destroy.  There is
nothing good in war.  Except its ending.
	-- Abraham Lincoln, "The Savage Curtain", stardate 5906.5