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

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

[Vijay Kumar]

The present nandsim allocates memory equal to the size of the NAND
flash during initialization in one shot. Thus it is impossible to
simulate a 256MB NAND flash on a system with 128MB RAM. 

For most testing purposes, only a part of the the NAND memory
allocated in the simulator is used. This patch set modifies the
simulator so as to allocate a NAND page when it is written to, and
deallocates the page when it is erased. With this it is possible to
simulate large NAND flash devices on computers with less RAM.

Regards,
Vijay

-- 
Free the Code, Free the User.
Website: http://www.bravegnu.org

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

[Vijay Kumar]

The existing config options related to chip mapping and chip-wise
allocation are moved to kernel configuration system.

Index: linux-2.6-mtd-quilt/drivers/mtd/nand/Kconfig
===================================================================
--- linux-2.6-mtd-quilt.orig/drivers/mtd/nand/Kconfig	2006-10-01 18:56:35.000000000 +0530
+++ linux-2.6-mtd-quilt/drivers/mtd/nand/Kconfig	2006-10-01 20:35:47.000000000 +0530
@@ -239,4 +239,28 @@
 	  The simulator may simulate various NAND flash chips for the
 	  MTD nand layer.
 
+choice 
+	depends on MTD_NAND_NANDSIM
+	prompt "Mapping/allocation method"
+	default MTD_NAND_NANDSIM_CHIP_ALLOC
+
+config  MTD_NAND_NANDSIM_CHIP_MAP
+	bool "Mapped flash image"
+	help
+	  The flash data is stored in an image mapped to memory.
+
+config  MTD_NAND_NANDSIM_CHIP_ALLOC
+	bool "Use allocated memory"
+	help
+	  The flash data is stored in allocated memory.
+
+endchoice
+
+config MTD_NAND_NANDSIM_ABS_POS
+	hex "Location of the flash image"
+	depends on MTD_NAND_NANDSIM_CHIP_MAP
+	help
+	  The value specifies the location at which the flash
+	  image exists.
+
 endmenu
Index: linux-2.6-mtd-quilt/drivers/mtd/nand/nandsim.c
===================================================================
--- linux-2.6-mtd-quilt.orig/drivers/mtd/nand/nandsim.c	2006-10-01 18:53:35.000000000 +0530
+++ linux-2.6-mtd-quilt/drivers/mtd/nand/nandsim.c	2006-10-01 20:39:39.000000000 +0530
@@ -37,10 +37,17 @@
 #include <linux/mtd/nand.h>
 #include <linux/mtd/partitions.h>
 #include <linux/delay.h>
-#ifdef CONFIG_NS_ABS_POS
+#ifdef CONFIG_MTD_NAND_NANDSIM_CHIP_MAP
 #include <asm/io.h>
 #endif
 
+#if defined(CONFIG_MTD_NAND_NANDSIM_CHIP_MAP)
+#define CONFIG_NANDSIM_CHIP_MAP      1
+#elif defined(CONFIG_MTD_NAND_NANDSIM_CHIP_ALLOC)
+#define CONFIG_NANDSIM_CHIP_ALLOC    1
+#else
+#error "One of chip map or chip alloc has to be selected."
+#endif
 
 /* Default simulator parameters values */
 #if !defined(CONFIG_NANDSIM_FIRST_ID_BYTE)  || \
@@ -53,6 +60,8 @@
 #define CONFIG_NANDSIM_FOURTH_ID_BYTE 0xFF /* No byte */
 #endif
 
+#define CONFIG_NANDSIM_ABS_POS       CONFIG_MTD_NAND_NANDSIM_ABS_POS
+
 #ifndef CONFIG_NANDSIM_ACCESS_DELAY
 #define CONFIG_NANDSIM_ACCESS_DELAY 25
 #endif
@@ -440,8 +449,8 @@
 	printk("options: %#x\n",                ns->options);
 
 	/* Map / allocate and initialize the flash image */
-#ifdef CONFIG_NS_ABS_POS
-	ns->mem.byte = ioremap(CONFIG_NS_ABS_POS, ns->geom.totszoob);
+#ifdef CONFIG_NANDSIM_CHIP_MAP
+	ns->mem.byte = ioremap(CONFIG_NANDSIM_ABS_POS, ns->geom.totszoob);
 	if (!ns->mem.byte) {
 		NS_ERR("init_nandsim: failed to map the NAND flash image at address %p\n",
 			(void *)CONFIG_NS_ABS_POS);
@@ -474,7 +483,7 @@
 	return 0;
 
 error:
-#ifdef CONFIG_NS_ABS_POS
+#ifdef CONFIG_NANDSIM_CHIP_MAP
 	iounmap(ns->mem.byte);
 #else
 	vfree(ns->mem.byte);
@@ -491,7 +500,7 @@
 {
 	kfree(ns->buf.byte);
 
-#ifdef CONFIG_NS_ABS_POS
+#ifdef CONFIG_NANDSIM_CHIP_MAP
 	iounmap(ns->mem.byte);
 #else
 	vfree(ns->mem.byte);

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

[Vijay Kumar]

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>

Index: linux-2.6-mtd-quilt/drivers/mtd/nand/Kconfig
===================================================================
--- linux-2.6-mtd-quilt.orig/drivers/mtd/nand/Kconfig	2006-10-01 20:35:47.000000000 +0530
+++ linux-2.6-mtd-quilt/drivers/mtd/nand/Kconfig	2006-10-02 18:20:42.000000000 +0530
@@ -250,10 +250,21 @@
 	  The flash data is stored in an image mapped to memory.
 
 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.
+	  
 endchoice
 
 config MTD_NAND_NANDSIM_ABS_POS
Index: linux-2.6-mtd-quilt/drivers/mtd/nand/nandsim.c
===================================================================
--- linux-2.6-mtd-quilt.orig/drivers/mtd/nand/nandsim.c	2006-10-01 20:39:39.000000000 +0530
+++ linux-2.6-mtd-quilt/drivers/mtd/nand/nandsim.c	2006-10-02 21:40:54.000000000 +0530
@@ -37,7 +37,7 @@
 #include <linux/mtd/nand.h>
 #include <linux/mtd/partitions.h>
 #include <linux/delay.h>
-#ifdef CONFIG_MTD_NAND_NANDSIM_CHIP_MAP
+#if defined(CONFIG_MTD_NAND_NANDSIM_CHIP_MAP)
 #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
 #else
-#error "One of chip map or chip alloc has to be selected."
+#error "One of chip map, chip alloc or page alloc has to be selected."
 #endif
 
 /* Default simulator parameters values */
@@ -239,6 +241,11 @@
  */
 #define NS_MAX_PREVSTATES 1
 
+union ns_mem_t {
+	u_char *byte;
+	uint16_t *word;
+};
+
 /*
  * The structure which describes all the internal simulator data.
  */
@@ -257,16 +264,15 @@
 	uint16_t stateidx;      /* current state index */
 
 	/* The simulated NAND flash image */
-	union flash_media {
-		u_char *byte;
-		uint16_t    *word;
-	} mem;
+
+#if defined(CONFIG_NANDSIM_CHIP_MAP) || defined(CONFIG_NANDSIM_CHIP_ALLOC)
+	union ns_mem_t mem;
+#elif defined(CONFIG_NANDSIM_PAGE_ALLOC)
+	union ns_mem_t *pages;
+#endif
 
 	/* Internal buffer of page + OOB size bytes */
-	union internal_buffer {
-		u_char *byte;    /* for byte access */
-		uint16_t *word;  /* for 16-bit word access */
-	} buf;
+	union ns_mem_t buf;
 
 	/* NAND flash "geometry" */
 	struct nandsin_geometry {
@@ -354,6 +360,85 @@
 
 static u_char ns_verify_buf[NS_LARGEST_PAGE_SIZE];
 
+#if defined(CONFIG_NANDSIM_CHIP_MAP)
+
+static int
+alloc_map_device(struct nandsim *ns)
+{
+	/* Map / allocate and initialize the flash image */
+	ns->mem.byte = ioremap(CONFIG_NANDSIM_ABS_POS, ns->geom.totszoob);
+	if (!ns->mem.byte) {
+		NS_ERR("alloc_map: failed to map the NAND flash image at address %p\n",
+			(void *)CONFIG_NANDSIM_ABS_POS);
+		return -ENOMEM;
+	}
+	
+	return 0;
+}
+
+static void
+free_unmap_device(struct nandsim *ns)
+{
+	iounmap(ns->mem.byte);
+}
+
+#elif defined(CONFIG_NANDSIM_CHIP_ALLOC)
+
+static int
+alloc_map_device(struct nandsim *ns)
+{
+	ns->mem.byte = vmalloc(ns->geom.totszoob);
+	if (!ns->mem.byte) {
+		NS_ERR("alloc_map: unable to allocate %u bytes for flash image\n",
+			ns->geom.totszoob);
+		return -ENOMEM;
+	}
+	memset(ns->mem.byte, 0xFF, ns->geom.totszoob);
+
+	return 0;
+}
+
+static void
+free_unmap_device(struct nandsim *ns)
+{
+	vfree(ns->mem.byte);
+}
+
+#elif defined(CONFIG_NANDSIM_PAGE_ALLOC)
+
+static int
+alloc_map_device(struct nandsim *ns)
+{
+	int i;
+
+	ns->pages = vmalloc(ns->geom.pgnum * sizeof(union ns_mem_t));
+	if (!ns->pages) {
+		NS_ERR("alloc_map: unable to allocate page array\n");
+		return -ENOMEM;
+	}
+	for (i = 0; i < ns->geom.pgnum; i++) {
+		ns->pages[i].byte = NULL;
+	}
+
+	return 0;
+}
+
+static void
+free_unmap_device(struct nandsim *ns)
+{
+	int i;
+
+	if (ns->pages) {
+		for (i = 0; i < ns->geom.pgnum; i++) {
+			if (ns->pages[i].byte)
+				kfree(ns->pages[i].byte);
+		}
+		vfree(ns->pages);
+	}
+}
+
+#endif
+
 /*
  * Initialize the nandsim structure.
  *
@@ -448,23 +533,8 @@
 	printk("sector address bytes: %u\n",    ns->geom.secaddrbytes);
 	printk("options: %#x\n",                ns->options);
 
-	/* Map / allocate and initialize the flash image */
-#ifdef CONFIG_NANDSIM_CHIP_MAP
-	ns->mem.byte = ioremap(CONFIG_NANDSIM_ABS_POS, ns->geom.totszoob);
-	if (!ns->mem.byte) {
-		NS_ERR("init_nandsim: failed to map the NAND flash image at address %p\n",
-			(void *)CONFIG_NS_ABS_POS);
-		return -ENOMEM;
-	}
-#else
-	ns->mem.byte = vmalloc(ns->geom.totszoob);
-	if (!ns->mem.byte) {
-		NS_ERR("init_nandsim: unable to allocate %u bytes for flash image\n",
-			ns->geom.totszoob);
-		return -ENOMEM;
-	}
-	memset(ns->mem.byte, 0xFF, ns->geom.totszoob);
-#endif
+	if (alloc_map_device(ns) != 0)
+		goto error;
 
 	/* Allocate / initialize the internal buffer */
 	ns->buf.byte = kmalloc(ns->geom.pgszoob, GFP_KERNEL);
@@ -483,11 +553,7 @@
 	return 0;
 
 error:
-#ifdef CONFIG_NANDSIM_CHIP_MAP
-	iounmap(ns->mem.byte);
-#else
-	vfree(ns->mem.byte);
-#endif
+	free_unmap_device(ns);
 
 	return -ENOMEM;
 }
@@ -500,11 +566,7 @@
 {
 	kfree(ns->buf.byte);
 
-#ifdef CONFIG_NANDSIM_CHIP_MAP
-	iounmap(ns->mem.byte);
-#else
-	vfree(ns->mem.byte);
-#endif
+	free_unmap_device(ns);
 
 	return;
 }
@@ -799,6 +861,116 @@
 	return -1;
 }
 
+#if defined(CONFIG_NANDSIM_CHIP_MAP) || defined(CONFIG_NANDSIM_CHIP_ALLOC)
+
+/*
+ * Copy NUM bytes from the specified page/offset to the buffer.
+ */
+static void read_page(struct nandsim *ns, int num)
+{
+	memcpy(ns->buf.byte, ns->mem.byte + NS_RAW_OFFSET(ns) 
+	       + ns->regs.off, num);
+}
+
+/*
+ * Erase the selected sector.
+ */
+static void erase_sector(struct nandsim *ns)
+{
+	memset(ns->mem.byte + NS_RAW_OFFSET(ns), 0xFF, ns->geom.secszoob);
+}
+
+
+/*
+ * Program NUM bytes into the specified page/offset in the flash from
+ * the buffer.
+ *
+ * Returns 0 on success, -1 on failure.
+ */
+static int prog_page(struct nandsim *ns, int num)
+{
+	int i;
+
+	for (i = 0; i < num; i++) {
+		ns->mem.byte[NS_RAW_OFFSET(ns) + ns->regs.off + i] &= ns->buf.byte[i];
+	}
+
+	return 0;
+}
+
+#elif defined(CONFIG_NANDSIM_PAGE_ALLOC)
+
+/*
+ * Returns a pointer to the current page.
+ */
+static inline union ns_mem_t *NS_GET_PAGE(struct nandsim *ns) 
+{
+	return &(ns->pages[ns->regs.row]);
+}
+
+/*
+ * Retuns a pointer to the current byte, within the current page.
+ */
+static inline u_char *NS_PAGE_BYTE_OFF(struct nandsim *ns)
+{
+	return NS_GET_PAGE(ns)->byte + ns->regs.column + ns->regs.off;
+}
+
+static void read_page(struct nandsim *ns, int num)
+{
+	union ns_mem_t *mypage;
+
+	mypage = NS_GET_PAGE(ns);
+	if (mypage->byte == NULL) {
+		NS_DBG("read_page: page %d not allocated\n", ns->regs.row);
+		memset(ns->buf.byte, 0xFF, num);
+	} else {
+		NS_DBG("read_page: page %d allocated, reading from %d\n",
+			ns->regs.row, ns->regs.column + ns->regs.off);
+		memcpy(ns->buf.byte, NS_PAGE_BYTE_OFF(ns), num);
+	}
+}
+
+static void erase_sector(struct nandsim *ns)
+{
+	union ns_mem_t *mypage;
+	int i;
+
+	mypage = NS_GET_PAGE(ns);
+	for (i = 0; i < ns->geom.pgsec; i++) {
+		if (mypage->byte != NULL) {
+			NS_DBG("erase_sector: freeing page %d\n", ns->regs.row+i);
+			kfree(mypage->byte);
+			mypage->byte = NULL;
+		}
+		mypage++;
+	}
+}
+
+static int prog_page(struct nandsim *ns, int num)
+{
+	union ns_mem_t *mypage;
+	u_char *pg_off;
+
+	mypage = NS_GET_PAGE(ns);
+	if (mypage->byte == NULL) {
+		NS_DBG("prog_page: allocating page %d\n", ns->regs.row);
+		mypage->byte = kmalloc(ns->geom.pgszoob, GFP_KERNEL);
+		if (mypage->byte == NULL) {
+			NS_ERR("prog_page: error allocating memory for page %d\n", ns->regs.row);
+			return -1;
+		}
+		memset(mypage->byte, 0xFF, ns->geom.pgszoob);
+	}
+	
+	pg_off = NS_PAGE_BYTE_OFF(ns);
+	memcpy(pg_off, ns->buf.byte, num);
+
+	return 0;
+}
+
+#endif
+
 /*
  * If state has any action bit, perform this action.
  *
@@ -807,7 +979,7 @@
 static int
 do_state_action(struct nandsim *ns, uint32_t action)
 {
-	int i, num;
+	int num;
 	int busdiv = ns->busw == 8 ? 1 : 2;
 
 	action &= ACTION_MASK;
@@ -831,7 +1003,7 @@
 			break;
 		}
 		num = ns->geom.pgszoob - ns->regs.off - ns->regs.column;
-		memcpy(ns->buf.byte, ns->mem.byte + NS_RAW_OFFSET(ns) + ns->regs.off, num);
+		read_page(ns, num);
 
 		NS_DBG("do_state_action: (ACTION_CPY:) copy %d bytes to int buf, raw offset %d\n",
 			num, NS_RAW_OFFSET(ns) + ns->regs.off);
@@ -872,7 +1044,7 @@
 				ns->regs.row, NS_RAW_OFFSET(ns));
 		NS_LOG("erase sector %d\n", ns->regs.row >> (ns->geom.secshift - ns->geom.pgshift));
 
-		memset(ns->mem.byte + NS_RAW_OFFSET(ns), 0xFF, ns->geom.secszoob);
+		erase_sector(ns);
 
 		NS_MDELAY(erase_delay);
 
@@ -895,8 +1067,9 @@
 			return -1;
 		}
 
-		for (i = 0; i < num; i++)
-			ns->mem.byte[NS_RAW_OFFSET(ns) + ns->regs.off + i] &= ns->buf.byte[i];
+		if (prog_page(ns, num) == -1) {
+			return -1;
+		}
 
 		NS_DBG("do_state_action: copy %d bytes from int buf to (%#x, %#x), raw off = %d\n",
 			num, ns->regs.row, ns->regs.column, NS_RAW_OFFSET(ns) + ns->regs.off);

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

[Artem Bityutskiy]

Hello Vijay,

On Thu, 2006-10-05 at 22:27 +0530, Vijay Kumar wrote:
> The present nandsim allocates memory equal to the size of the NAND
> flash during initialization in one shot. Thus it is impossible to
> simulate a 256MB NAND flash on a system with 128MB RAM. 

Right, I've been bearing a plan to fix this since long, I was even
thinking to re-implement it and make it able to work with HDD to emulate
really huge NANDs. But, time, time ...

> For most testing purposes, only a part of the the NAND memory
> allocated in the simulator is used. This patch set modifies the
> simulator so as to allocate a NAND page when it is written to, and
> deallocates the page when it is erased. With this it is possible to
> simulate large NAND flash devices on computers with less RAM.

Well, the idea sounds nice.

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

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

[Artem Bityutskiy]

On Thu, 2006-10-05 at 22:34 +0530, Vijay Kumar wrote:
> The existing config options related to chip mapping and chip-wise
> allocation are moved to kernel configuration system.

BTW, it is a nice idea to add Signed-off-by, just a note for future
patches :-)

> 
> Index: linux-2.6-mtd-quilt/drivers/mtd/nand/Kconfig
> ===================================================================
> --- linux-2.6-mtd-quilt.orig/drivers/mtd/nand/Kconfig	2006-10-01 18:56:35.000000000 +0530
> +++ linux-2.6-mtd-quilt/drivers/mtd/nand/Kconfig	2006-10-01 20:35:47.000000000 +0530
> @@ -239,4 +239,28 @@
>  	  The simulator may simulate various NAND flash chips for the
>  	  MTD nand layer.
>  
> +choice 
> +	depends on MTD_NAND_NANDSIM
> +	prompt "Mapping/allocation method"
> +	default MTD_NAND_NANDSIM_CHIP_ALLOC
> +
> +config  MTD_NAND_NANDSIM_CHIP_MAP
> +	bool "Mapped flash image"
> +	help
> +	  The flash data is stored in an image mapped to memory.
Is this option really needed? I've never heard somebody using it. Do you
use it? If no, will you mind to get rid of it at all? And then of course
MTD_NAND_NANDSIM_CHIP_ALLOC won't be needed as well.

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

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

[Artem Bityutskiy]

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 (Битюцкий Артём)

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

[Vijay Kumar]

    >> +config  MTD_NAND_NANDSIM_CHIP_MAP
    >> +	bool "Mapped flash image"
    >> +	help
    >> +	  The flash data is stored in an image mapped to memory.

    Artem> Is this option really needed? I've never heard somebody
    Artem> using it. Do you use it? If no, will you mind to get rid of
    Artem> it at all? And then of course MTD_NAND_NANDSIM_CHIP_ALLOC
    Artem> won't be needed as well.

If you think the CHIP_MAP option is not required, I will get rid of
it. I will remove the CHIP_ALLOC option as well.

Regards,
Vijay

-- 
Free the Code, Free the User.
Website: http://www.bravegnu.org

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

[Vijay Kumar]

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

A kind of symmetry, since there is no #elifdef! :-) Anyway, the
#ifdefs are no longer required.

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

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

Point taken.

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

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

Actually, the nandsim code uses this style! I used the same style to
be consistent with the nandsim code. I won't be fixing this in my
updated patch. Probably we could have a separate patch that fixes all
function definitions.

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

Point taken.

Thanks for the review Artem. Will send an updated patch shortly.

Regards,
Vijay

-- 
Free the Code, Free the User.
Website: http://www.bravegnu.org