Re: [PATCH][RFC] NAND subpage read feature. Take 2.

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

On Wed, 30 April 2008 13:13:24 +0100, Alexey Korolev wrote:
> 
> This patch enables NAND subpage read functionality. 

...and creates a peculiar conflict of interests in me.  I've added full
caching of the complete mtd in logfs, which gave a huge speedup to some
of my testcases.  However caching is done in page granularity, therefore
it would completely defeat this patch, which also gives a speedup.

But until I can figure out how to combine both benefits, this looks
rather promising and should clearly get in.

>  /**
> + * nand_read_subpage - [REPLACABLE] software ecc based sub-page read function
> + * @mtd:	mtd info structure
> + * @chip:	nand chip info structure
> + * @dataofs	offset of requested data within the page 
> + * @readlen	data length 
> + * @buf:	buffer to store read data
> + */
> +static int nand_read_subpage(struct mtd_info * mtd,struct nand_chip *chip, uint32_t data_offs, uint32_t readlen, uint8_t *bufpoi)
                                                 ^   ^^
A linebreak would help.  The indicated space should move five bytes to
the right.  And there are a few similar stylistic things below.  If you
feed this through checkpatch.pl, you should find them all.

buf and bufpoi should be the same name (I personally would prefer buf).
And you should document that the buffer has to be bigger than readlen.
There is just a single caller, but such a counter-intuitive detail can
easily cause bugs some years from now.

> +{
> +	int start_step, end_step, num_steps;
> +	uint32_t *eccpos = chip->ecc.layout->eccpos;
> +	uint8_t *p;
> +	int data_col_addr, i, gaps = 0;
> +	int datafrag_len, eccfrag_len, aligned_len, aligned_pos;
> +	int busw = (chip->options & NAND_BUSWIDTH_16) ? 2 : 1;
> +	
> +	/* Column address wihin the page aligned to ECC size (256bytes). */

within

> +	start_step = data_offs / chip->ecc.size;
> +	end_step = (data_offs + readlen - 1) / chip->ecc.size;

Divisions are fairly expensive computations.  Can they be replaced by
shifts?

Rest looks good to me, apart from some checkpatch-type details.
Although I am tired and could have missed something.

Jörn

-- 
Joern's library part 9:
http://www.scl.ameslab.gov/Publications/Gus/TwelveWays.html