On 17.11.2008 15:04, Stefan Reinauer wrote:
Carl-Daniel Hailfinger wrote:
On 17.11.2008 14:28, Stefan Reinauer wrote:
I tried factoring it out, but since we don't want to die() completely, we have to return with an error code from this function. That pretty much rules out factoring out the checks into a separate function. I could move all checks one level down in the call graph and clutter up only the end of each function. What do you think?
I guess your current version is fine then
OK, thanks.
+int spi_chip_erase_60_c7(struct flashchip *flash) +{
- int result;
- result = spi_chip_erase_60(flash);
- if (result) {
printf_debug("spi_chip_erase_60 failed, trying c7\n");result = spi_chip_erase_c7(flash);- }
- return result;
+}
I don't particularly like this. Maybe we should have spi_chip_erase() try all different erase functions in a row, and check whether the erase was actually successfull (all bytes 0xff)?
That's a bit difficult. There are some chips which have either the 0x60 or the 0xc7 opcode used for something else, so we need to keep this chip specific.
Hm.. So do we need to keep an array of supported read, write, erase, id opcodes for each spi chip to handle this? That way we could check the chip's capabilities and compare to the OPMENUs capabilities.
For read at normal speed, it's the same opcode for all SPI EEPROM chips with power-of-two sizes I saw so far. For id, there are up to four opcodes, each of them yielding a different answer.
That one function does one job, but I'm a little concerned that we end up writing functions for all possible variations of possible commands and have a hard time tracking it afterwards.
The most sane way for SPI would be to bundle valid opcodes for a given function together with the function and any auxilliary data for that function. Let me give an example:
struct available_command { u32 opcodes[4]; int somefunction(int param1, int param2, int param3, void* param4, void* param5); char auxiliary_data[64]; }
struct available_command chip_erase = { .opcodes = {0x60, 0xc7,} .somefunction = chip_erase(0,0,0, struct flashchip, chip_erase.opcodes, NULL); .auxiliary_data = {}; }
For sector erase, auxiliary_data would hold sector sizes.
Regards, Carl-Daniel