Thank you very much for your interest on this.
What do you think?
Thank you,
Can u just send pseudo code about your logic, i couldn't get u exactly sorry.
Here is what I think, it is similar with your patch in the sense that "do the different status" check for different device. However, the difference is that your patch checks based on device type at the status checking function. Mine is proposing to add the information to the devices' struct (spi_flash) instead, so each device can have a flexibility to call flag status check. In this way if new device use this flag_status check, you can just hide the setting in devices' probe function, and not worrying about updating generic function call (spi_flash_cmd_poll_bit() as in your patch)
Your idea seems to be clean and reasonable, but
I am not so happy to add "flag_status" in struct spi_flash; so I am open to your idea. Please see below and let me know what you think?
// 1. adding flag status struct spi_flash { struct spi_slave *spi;
const char *name; /* Total flash size */ u32 size; /* Write (page) size */ u32 page_size; /* Erase (sector) size */ u32 sector_size; /* if flag_status is use or not */ u8 flag_status; int (*read)(struct spi_flash *flash, u32 offset, size_t len, void *buf); int (*write)(struct spi_flash *flash, u32 offset, size_t len, const void *buf); int (*erase)(struct spi_flash *flash, u32 offset, size_t len);};
// 2. in probe function, set the flag_status per vendor
struct spi_flash *spi_flash_probe_stmicro(struct spi_slave *spi, u8 * idcode) {
flash->spi = spi; flash->name = params->name; flash->write = spi_flash_cmd_write_multi; flash->erase = spi_flash_cmd_erase; flash->read = spi_flash_cmd_read_fast; flash->page_size = 256; flash->sector_size = 256 * params->pages_per_sector; flash->size = flash->sector_size * params->nr_sectors; // we do this for Micron, and will do the same if any other device needs this flash->flag_status = 1; return flash;}
// 3. call flag status check if flash->flag_status is set
int spi_flash_cmd_wait_ready(struct spi_flash *flash, unsigned long timeout) { if (flash->flag_status) return spi_flash_cmd_poll_bit(flash, timeout, CMD_READ_FLAG_STATUS, STATUS_PEC, STATUS_PEC); else return spi_flash_cmd_poll_bit(flash, timeout, CMD_READ_STATUS, STATUS_WIP, 0); }
APAIK. flag status required only for stmicro flashes which has >= 512MB flashes. And as this is specific to a particular flash with particular size, i don't see any reason to add extra variable in spi_flash and then initialized at probe. Your idea seems to be reasonable if we have more numbers of flash vendors require this with respective sizes.
ie, the reason I have gave a condition for the particular state like + if ((flash->idcode0 == 0x20) && + (flash->size >= SPI_FLASH_512MB_STMIC))
And I have removed spi_flash_cmd_poll_bit as these is no separate caller for this except the spi_flash_cmd_wait_ready() and did everything on spi_flash_cmd_wait_ready().
Comments.
-- Thanks, Jagan.