Hi Boris,

On Tue, 13 Feb 2018 09:17:14 +0100, Boris Brezillon
[off-list ref] wrote:

On Tue, 13 Feb 2018 08:42:46 +0100
Miquel Raynal [off-list ref] wrote:

quoted

Hi Boris,

Just a few comments about the form.

Otherwise:
Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>

  

quoted

diff --git a/drivers/mtd/devices/lart.c b/drivers/mtd/devices/lart.c
index 555b94406e0b..3d6c8ffd351f 100644
--- a/drivers/mtd/devices/lart.c
+++ b/drivers/mtd/devices/lart.c

@@ -415,7 +415,6 @@ static int flash_erase (struct mtd_info *mtd,struct erase_info *instr)
 	 {
 		if (!erase_block (addr))
 		  {
-			 instr->state = MTD_ERASE_FAILED;
 			 return (-EIO);
 		  }    

You can also safely remove these '{' '}'  

Well, this patch is not about fixing coding style issues, otherwise I'd
have a lot more work on this driver :-)

Sure, I was not referring to the weird style but just that you switch
from two to one line in the block, thus the braces are not needed
anymore.

quoted

quoted

@@ -425,9 +424,6 @@ static int flash_erase (struct mtd_info *mtd,struct erase_info *instr)
 		if (addr == mtd->eraseregions[i].offset + (mtd->eraseregions[i].erasesize * mtd->eraseregions[i].numblocks)) i++;
 	 }
 
-   instr->state = MTD_ERASE_DONE;
-   mtd_erase_callback(instr);
-
    return (0);
 }
 

diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
index 5dc8bd042cc5..aaaeaae01e1d 100644
--- a/drivers/mtd/devices/mtd_dataflash.c
+++ b/drivers/mtd/devices/mtd_dataflash.c

@@ -220,10 +220,6 @@ static int dataflash_erase(struct mtd_info *mtd, struct erase_info *instr)
 	}
 	mutex_unlock(&priv->lock);
 
-	/* Inform MTD subsystem that erase is complete */
-	instr->state = MTD_ERASE_DONE;
-	mtd_erase_callback(instr);
-
 	return 0;
 }
 

diff --git a/drivers/mtd/devices/mtdram.c b/drivers/mtd/devices/mtdram.c
index 0bf4aeaf0cb8..efef43c6684b 100644
--- a/drivers/mtd/devices/mtdram.c
+++ b/drivers/mtd/devices/mtdram.c

@@ -60,8 +60,6 @@ static int ram_erase(struct mtd_info *mtd, struct erase_info *instr)
 	if (check_offs_len(mtd, instr->addr, instr->len))
 		return -EINVAL;
 	memset((char *)mtd->priv + instr->addr, 0xff, instr->len);
-	instr->state = MTD_ERASE_DONE;
-	mtd_erase_callback(instr);    

Space ?  

I could add a blank line, but again, I'm just following the coding style
in place in this file :-).

Ok.

quoted

quoted

 	return 0;
 }
 

diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
index 7287696a21f9..a963c88d392d 100644
--- a/drivers/mtd/devices/phram.c
+++ b/drivers/mtd/devices/phram.c

@@ -44,8 +44,6 @@ static int phram_erase(struct mtd_info *mtd, struct erase_info *instr)
 	 * I don't feel at all ashamed. This kind of thing is possible anyway
 	 * with flash, but unlikely.
 	 */    

Not sure this comment is still relevant? Maybe you could remove it
or at least change it? 
  

quoted

-	instr->state = MTD_ERASE_DONE;
-	mtd_erase_callback(instr);    

Space ?
  

quoted

 	return 0;
 }
 

diff --git a/drivers/mtd/devices/pmc551.c b/drivers/mtd/devices/pmc551.c
index cadea0620cd0..5d842cbca3de 100644
--- a/drivers/mtd/devices/pmc551.c
+++ b/drivers/mtd/devices/pmc551.c

@@ -184,12 +184,10 @@ static int pmc551_erase(struct mtd_info *mtd, struct erase_info *instr)
 	}
 
       out:
-	instr->state = MTD_ERASE_DONE;
 #ifdef CONFIG_MTD_PMC551_DEBUG
 	printk(KERN_DEBUG "pmc551_erase() done\n");
 #endif
 
-	mtd_erase_callback(instr);
 	return 0;
 }
 

diff --git a/drivers/mtd/devices/powernv_flash.c b/drivers/mtd/devices/powernv_flash.c
index 26f9feaa5d17..5f383630c16f 100644
--- a/drivers/mtd/devices/powernv_flash.c
+++ b/drivers/mtd/devices/powernv_flash.c

@@ -175,19 +175,12 @@ static int powernv_flash_erase(struct mtd_info *mtd, struct erase_info *erase)
 {
 	int rc;
 
-	erase->state = MTD_ERASING;
-
 	/* todo: register our own notifier to do a true async implementation */
 	rc =  powernv_flash_async_op(mtd, FLASH_OP_ERASE, erase->addr,
 			erase->len, NULL, NULL);    

Are you sure this is still needed? Maybe this should go away in your
first patch?  

Hm, indeed. This comment should be dropped.

quoted

quoted

-
-	if (rc) {
+	if (rc)
 		erase->fail_addr = erase->addr;
-		erase->state = MTD_ERASE_FAILED;
-	} else {
-		erase->state = MTD_ERASE_DONE;
-	}
-	mtd_erase_callback(erase);
+
 	return rc;
 }
 

diff --git a/drivers/mtd/devices/slram.c b/drivers/mtd/devices/slram.c
index 0ec85f316d24..2f05e1801047 100644
--- a/drivers/mtd/devices/slram.c
+++ b/drivers/mtd/devices/slram.c

@@ -88,8 +88,6 @@ static int slram_erase(struct mtd_info *mtd, struct erase_info *instr)
 	 * I don't feel at all ashamed. This kind of thing is possible anyway
 	 * with flash, but unlikely.
 	 */    

Same with this comment.  

Actually, I'm not sure I understand that comment, but I guess it's
talking about races between read/write and erase paths, so nothing
related to the changed I'm doing here.

I thought that races could only happen because of the asynchronous
manner how erases were handled, but if you feel this is still relevant,
I trust you :)

quoted

quoted

-	instr->state = MTD_ERASE_DONE;
-	mtd_erase_callback(instr);    

Space ?
  

quoted

 	return(0);
 }
     

-- 
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com