Re: [RFC/PATCH 3/3] NAND multiple plane feature

["Jörn Engel" <joern@logfs.org>]

On Wed, 28 May 2008 14:42:48 +0100, Alexey Korolev wrote:
>
> +static int nand_read_operations(struct mtd_info *mtd, struct nand_chip *chip,
> +				      const uint8_t *buf, loff_t offt, int len, int sndcmd, int raw)
> +{
> +	int i = 0;
> +	int ret, phys_page;
> +	int page = (int)(offt >> chip->page_shift) & chip->pagemask;
> +	int block_mask = (1 << (chip->erase_shift - chip->page_shift)) - 1;
> +	int col, bytes, start_block, page_num;
> +	uint8_t *datbuf, *oobbuf;
> +
> +	/* starting erase block at first plane */
> +	start_block = (page & ~block_mask) * chip->numplanes;
> +	page_num = page & block_mask;
> +	while (i < chip->numplanes) {
> +		datbuf = (uint8_t *)(buf + chip->page_phys_size * i);
> +		oobbuf = chip->oob_poi + chip->oob_phys_size * i;
> +		col = (int)(offt & (mtd->writesize - 1));

Does the explicit cast make any difference?

> +		/* calculate column for current plane */
> +		col = (int)(col & (chip->page_phys_size - 1));
> +		bytes = min(chip->page_phys_size - col, len);
> +
> +		phys_page = start_block + page_num + (block_mask + 1) * i;
> +		if (likely(sndcmd)) {
> +			chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, phys_page);
> +			if (chip->numplanes == 1)
> +				sndcmd = 0;
> +		}
> +		/* Now read the page into the buffer */
> +		if (unlikely(raw))
> +			ret = chip->ecc.read_page_raw(mtd, chip, datbuf, oobbuf);
> +		else
> +			ret = chip->ecc.read_page(mtd, chip, datbuf, oobbuf);
> +
> +		offt += bytes;
> +		len -= bytes;
> +		i++;

Looks like this could be a standard for loop as well.

And I am a bit surprised that you don't break out of the loop if bytes
ever becomes zero.  Is there a reason or did you just forget?

> +
> +		if (ret)
> +			break;
> +
> +		if (!(chip->options & NAND_NO_READRDY)) {
> +			if (!chip->dev_ready)
> +				udelay(chip->chip_delay);
> +			else
> +				nand_wait_ready(mtd);
> +			}

Wrong indentation.

> -			if (likely(sndcmd)) {
> -				chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page);
> -				sndcmd = 0;
> -			}
> -
> -			/* Now read the page into the buffer */
> -			if (unlikely(ops->mode == MTD_OOB_RAW))
> -				ret = chip->ecc.read_page_raw(mtd, chip, bufpoi);
> -			else
> -				ret = chip->ecc.read_page(mtd, chip, bufpoi);
> -			if (ret < 0)
> -				break;
> +			nand_read_operations(mtd, chip, bufpoi, from, bytes, sndcmd, (ops->mode == MTD_OOB_RAW));

I like this part.  Actually making our monster-functions shorter is
always a good thing.

> -					int toread = min(oobreadlen,
> -						chip->ecc.layout->oobavail);
> +					int toread = min(oobreadlen, chip->ecc.layout->oobavail);

Please don't reformat the code.  Or if you do, send that as a seperate
patch, either the first or the last in the series.  Makes the review a
lot easier if I don't have to double-check every time. :)

>  					if (toread) {
> -						oob = nand_transfer_oob(chip,
> -							oob, ops, toread);
> +						oob = nand_transfer_oob(chip, oob, ops, toread);
>  						oobreadlen -= toread;
>  					}
>  				} else
> -					buf = nand_transfer_oob(chip,
> -						buf, ops, mtd->oobsize);
> +					buf = nand_transfer_oob(chip, buf, ops, mtd->oobsize);

Same here.  And dito for the newline-removals somewhere above.

> -static int nand_write_page(struct mtd_info *mtd, struct nand_chip *chip,
> +static int nand_write_operations(struct mtd_info *mtd, struct nand_chip *chip,
>  			   const uint8_t *buf, int page, int cached, int raw)
>  {
> +	int i = 0;
>  	int status;
> +	int block_mask = (1 << (chip->erase_shift - chip->page_shift)) - 1;
> +	int start_block = (page & ~block_mask) * chip->numplanes;
> +	int page_num = page & block_mask;
> +
> +	/* calculate physycal page at first plane */
> +	while (i < chip->numplanes) {
> +		page = start_block + page_num + (block_mask + 1) * i;
> +		chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x00, page);
> +
> +		if (unlikely(raw))
> +			chip->ecc.write_page_raw(mtd, chip, buf + chip->page_phys_size * i,
> +									chip->oob_poi + chip->oob_phys_size * i);
> +		else
> +			chip->ecc.write_page(mtd, chip, buf + chip->page_phys_size * i,
> +									chip->oob_poi + chip->oob_phys_size * i);
> +		i++;

Even here I'd prefer a for loop and use "i - 1" in the condition below.

> +		if (i < chip->numplanes) {
> +			chip->cmdfunc(mtd, NAND_CMD_PAGEPROG_MP, -1, -1);
> +			if (!chip->dev_ready)
> +				udelay(chip->chip_delay);
> +			else
> +				nand_wait_ready(mtd);

This occurs for the second time.  And each time it was new code added by
you.  Looks like you should move it into some helper function, along
with a few lines of explanations why it is needed.

Jörn

-- 
There are three principal ways to lose money: wine, women, and engineers.
While the first two are more pleasant, the third is by far the more certain.
-- Baron Rothschild