Re: [PATCH 2/2] NAND: nandsim sector-wise allocation

[Artem Bityutskiy <dedekind@infradead.org>]

Vijay,

thank you for your patches. Please, find my comments below.

On Thu, 2006-10-05 at 22:43 +0530, Vijay Kumar wrote:
> For page wise allocation, an array of flash page pointers is allocated
> during initialization. The flash pages are themselves allocated when a
> write occurs to the page. The flash pages are deallocated when they
> are erased.
> 
> The init, read, prog and erase operations are moved to separate functions,
> and for each allocation/mapping type a set of init/read/prog/erase
> functions are defined.
> 
> Signed-off-by: Vijay Kumar <vijaykumar@bravegnu.org>

Frankly, I do not understand many things in nandsim code now, although I
wrote it :-) So no deep review for now, just minor or obvious things.

>  config  MTD_NAND_NANDSIM_CHIP_ALLOC
> -	bool "Use allocated memory"
> +	bool "Allocate entire chip"
>  	help
> -	  The flash data is stored in allocated memory.
> +	  The flash data is stored in allocated memory. The entire
> +	  memory to hold the flash data is allocated during 
> +	  initialization.
>  
> +config MTD_NAND_NANDSIM_PAGE_ALLOC
> +	bool "Allocate flash pages as required"
> +	help
> +	  The flash data is stored in allocated memory. The
> +	  allocation is done on a per page basis, when data is 
> +	  written to the page. Enables simulation of 
> +	  higher capacity NAND devices in systems that do
> +	  not have as much RAM.

I believe there is no need in these options. As soon as you've added the
new method, what's the reason to keep the old one? Why you didn't delete
it - any good reason?

> -#ifdef CONFIG_MTD_NAND_NANDSIM_CHIP_MAP
> +#if defined(CONFIG_MTD_NAND_NANDSIM_CHIP_MAP)

I wonder, why "#if defined(kaka)" is better then "#ifdef kaka" ? What
for is this change :-) ?

>  #include <asm/io.h>
>  #endif
>  
> @@ -45,8 +45,10 @@
>  #define CONFIG_NANDSIM_CHIP_MAP      1
>  #elif defined(CONFIG_MTD_NAND_NANDSIM_CHIP_ALLOC)
>  #define CONFIG_NANDSIM_CHIP_ALLOC    1
> +#elif defined(CONFIG_MTD_NAND_NANDSIM_PAGE_ALLOC)
> +#define CONFIG_NANDSIM_PAGE_ALLOC  1

Ditto. Although it is anyway reasonable to get rid of this junk #ifdefs
at all.

> +union ns_mem_t {
> +	u_char *byte;
> +	uint16_t *word;
> +};

Minor, but 'union ns_mem' is a better name. We usually use "_t" for
typedefs, and we use typedefs very rarely and only if there is a good
reason.

> +static int
> +alloc_map_device(struct nandsim *ns)

Again minor. I understand that everybody has its own style, and I
appreciate this. But when one fixes an existing code, it makes sense to
follow the same style. So I offer to use definitions like

static int alloc_map_device(struct nandsim *ns);

This is just a nice stuff to bear in mind, IMHO.

> +	for (i = 0; i < num; i++) {
> +		ns->mem.byte[NS_RAW_OFFSET(ns) + ns->regs.off + i] &= ns->buf.byte[i];
> +	}

Again a nit. We usually don't use brackets in case of one-line code -
just a convention.

> 			ns->mem.byte[NS_RAW_OFFSET(ns) + ns->regs.off + i] &= ns->buf.byte[i];
> +		if (prog_page(ns, num) == -1) {
> +			return -1;
> +		}

Ditto.

-- 
Best Regards,
Artem Bityutskiy (Битюцкий Артём)