[U-Boot-Users] [PATCH 6/7] NAND: add NAND driver for s3c64xx

[Wolfgang Denk]

In message <Pine.LNX.4.64.0807311246100.4832 at axis700.grange> you wrote:
> Ported from u-boot-1.1.6 driver by Samsung.
> 
> Signed-off-by: Guennadi Liakhovetski <lg at denx.de>
...

> --- /dev/null
> +++ b/drivers/mtd/nand/s3c64xx.c
> @@ -0,0 +1,315 @@
> +/*
> + * (C) Copyright 2006 DENX Software Engineering

Please add your name here.

> +/*
> + * Function to print out oob buffer for debugging
> + * Written by jsgood

Then jsgood should be mentioned in the (C) lines?

> +	case 1: /* 1 bit error (Correctable)
> +		   (nfestat0 >> 7) & 0x7ff	:error byte number
> +		   (nfestat0 >> 4) & 0x7	:error bit number */
> +		printf("S3C NAND: 1 bit error detected at byte %ld. Correcting from "
> +		       "0x%02x ", (nfestat0 >> 7) & 0x7ff, dat[(nfestat0 >> 7) & 0x7ff]);
> +		dat[(nfestat0 >> 7) & 0x7ff] ^= (1 << ((nfestat0 >> 4) & 0x7));
> +		printf("to 0x%02x...OK\n", dat[(nfestat0 >> 7) & 0x7ff]);

I guess there should be a way to write this code so  it  is  actually
readable?  This  is  U-Boot,  not  the  IOCCC  (yes,  there *is* some
difference, still).

> +int board_nand_init(struct nand_chip *nand)
> +{
> +	NFCONT_REG 		= (NFCONT_REG & ~NFCONT_WP) | NFCONT_ENABLE | 0x6;
> +
> +	nand->IO_ADDR_R		= (void __iomem *)NFDATA;
> +	nand->IO_ADDR_W		= (void __iomem *)NFDATA;
> +	nand->hwcontrol		= s3c_nand_hwcontrol;
> +	nand->dev_ready		= s3c_nand_device_ready;
> +	nand->select_chip	= s3c_nand_select_chip;
> +	nand->options		= 0;
> +#ifdef CONFIG_NAND_SPL
> +	nand->read_byte		= nand_read_byte;
> +	nand->write_byte	= nand_write_byte;
> +	nand->read_buf		= nand_read_buf;
> +#endif
> +
> +	do {
> +		struct mtd_info m;
> +		int cellinfo;
> +
> +		m.priv = nand;
> +
> +		s3c_nand_select_chip(&m, 0);
> +		s3c_nand_hwcontrol(&m, NAND_CTL_SETCLE);
> +		writeb(NAND_CMD_READID, nand->IO_ADDR_W);
> +		s3c_nand_hwcontrol(&m, NAND_CTL_CLRCLE);
> +		s3c_nand_device_ready(&m);
> +		cellinfo = readb(nand->IO_ADDR_R);
> +		cellinfo += readb(nand->IO_ADDR_R);
> +		cellinfo += readb(nand->IO_ADDR_R);
> +		s3c_nand_select_chip(&m, -1);
> +		nand->eccmode = cellinfo;
> +	} while (0);

Please get rid of the do...while here - or does it serve any purpose?

> +	/* A shot in the dark, I don't know how to use this smart ECC-mode
> +	 * selection with the current driver */

Incorrect multiline comment style.

Please check other files for that, too.

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
Once upon a time, "PC" meant "personal computer".  Now  it  seems  to
only  mean  "Prisoner  of Bill". Alas! All my PCs run Unix, and those
include Intel, Sparc, and other processors.
          -- Tom "Bring back the non-PC meaning of `PC'" Christiansen