[U-Boot] [PATCH] Add SPI Flash STMicro's N25Q512A & add flag status check during SPI Flash write

[Insop Song]

Hi Jagan,

Thank you for your feedback.

> -----Original Message-----
> From: Jagan Teki [mailto:jagannadh.teki at gmail.com]
> Sent: Monday, June 03, 2013 10:31 PM
> To: Insop Song
> Cc: u-boot at lists.denx.de; yorksun at freescale.com
> Subject: Re: [U-Boot] [PATCH] Add SPI Flash STMicro's N25Q512A & add flag
> status check during SPI Flash write
> >
> > I've made a change and add spi_flash_cmd_wait_program to align with
> existing code structure.
> > Please see the patch below.
> > Erase is okay without checking, so I didn't add the check.
> 
> No i see without the check in erase when i erase and read back i couldn't see
> 0xff Please check.

I've tested on the N25Q512A erase without the flag check went okay.
However, adding the check to the erase would be safe, and I've tested erase with flag status check.
Here is the diff of this change. I will use the format-patch when we finalize the change.

diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
index 4240e9d..9cf5aaf 100644
--- a/drivers/mtd/spi/spi_flash.c
+++ b/drivers/mtd/spi/spi_flash.c
@@ -247,6 +247,10 @@ int spi_flash_cmd_erase(struct spi_flash *flash, u32 offset, size_t len)
                if (ret)
                        goto out;
 
+         ret = spi_flash_cmd_wait_program(flash, SPI_FLASH_PROG_TIMEOUT);
+         if (ret)
+                 break;
+
                ret = spi_flash_cmd_wait_ready(flash, SPI_FLASH_PAGE_ERASE_TIMEOUT);
                if (ret)
                        goto out;


> >
> > I am not sure I'd agree with you on that you are putting the device check in
> spi_flash_cmd_poll_bit().
> > I am more inclined to make the change at the level where we call
> spi_flash_cmd_poll_bit() if we have to distinguish per devices.
> 
> spi_flash_cmd_poll_bit() is common call for poll for read_status, as write call
> is common to all making changes to
> spi_flash_cmd_poll_bit() is valid i guess. 
> Write call is a global which is used by
> so many user, i don't like to add the flag status there and make the confusion
> user.
> 

To your comment on "confusion to users", I agree on that.
With that argument, your patch is better since it hides the flag status check.

And how about this?  can we add "flag_status_check" flag somewhere, and if the device is needed then we set the flag during device discovery.
Then call the flag check function if the flag is set.
I think this way might be more generic then what you did in your patch.

What do you think? 

Thank you,


IS