On Wed, Jun 5, 2013 at 10:10 AM, Insop Song Insop.Song@cohere.net wrote:
Hi Jagan,
Thank you for your feedback.
-----Original Message----- From: Jagan Teki [mailto:jagannadh.teki@gmail.com] Sent: Monday, June 03, 2013 10:31 PM To: Insop Song Cc: u-boot@lists.denx.de; yorksun@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,
Can u just send pseudo code about your logic, i couldn't get u exactly sorry.
-- Thanks, Jagan.