Re: [PATCH for mtd-utils] flash_erase: check the nand type

[Brian Norris <computersforpeace@gmail.com>]

On Thu, Oct 24, 2013 at 02:06:06PM +0800, Huang Shijie wrote:
> Now, the MTD_NANDFLASH stands for SLC nand, and the MTD_MLCNANDFLASH
> stands for the MLC nand.
> 
> This patch checks the right nand type for the MLC and SLC nand.
> And if it is a MLC nand, we do not write cleanmarker for it, only print
> out a message to warn the user.
> 
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
>  flash_erase.c |    9 ++++++++-
>  1 files changed, 8 insertions(+), 1 deletions(-)
> 
> diff --git a/flash_erase.c b/flash_erase.c
> index 1421cf9..dd75334 100644
> --- a/flash_erase.c
> +++ b/flash_erase.c
> @@ -184,7 +184,8 @@ int main(int argc, char *argv[])
>  
>  	eb_start = start / mtd.eb_size;
>  
> -	isNAND = mtd.type == MTD_NANDFLASH ? 1 : 0;
> +	isNAND = (mtd.type == MTD_NANDFLASH || mtd.type == MTD_MLCNANDFLASH)
> +			? 1 : 0;

Perhaps you can #include <stdbool.h> and make 'isNAND' into a proper
bool variable? Then this becomes:

	isNAND = mtd.type == MTD_NANDFLASH || mtd.type == MTD_MLCNANDFLASH;

>  
>  	if (jffs2) {
>  		cleanmarker.magic = cpu_to_je16 (JFFS2_MAGIC_BITMASK);
> @@ -272,6 +273,12 @@ int main(int argc, char *argv[])
>  
>  		/* write cleanmarker */
>  		if (isNAND) {
> +			if (mtd.type == MTD_MLCNANDFLASH) {
> +				sys_errmsg("%s: we can't write the cleanmarker "
> +					"for the MLC nand.", mtd_device);
> +				continue;
> +			}
> +

This is in a loop. This will print the same message repeatedly if you
are erasing/marking an entire partition. Please test your changes before
sending v2.

You could probably just error out (or warn) like the following.

diff --git a/flash_erase.c b/flash_erase.c
index 1421cf9..0e385e9 100644
--- a/flash_erase.c
+++ b/flash_erase.c
@@ -169,6 +169,9 @@ int main(int argc, char *argv[])
 	if (error)
 		return errmsg("Try `--help' for more information");
 
+	if (jffs2 && mtd.type == MTD_MLCNANDFLASH)
+		return errmsg("JFFS2 cannot support MLC NAND");
+
 	/*
 	 * Locate MTD and prepare for erasure
 	 */

>  			if (mtd_write_oob(mtd_desc, &mtd, fd, (uint64_t)offset + clmpos, clmlen, &cleanmarker) != 0) {
>  				sys_errmsg("%s: MTD writeoob failure", mtd_device);
>  				continue;

Brian