Carl-Daniel Hailfinger wrote:
The following guidelines are for coreboot, but most of them apply to flashrom as well: http://www.coreboot.org/Development_Guidelines The really important part is about the Signed-off-by procedure.
Sounds good, I'll read through that.
We try to reuse as much code as possible and create new files only if absolutely needed, so if you find a function somewhere in the tree which already does what you want (even if it is for a totally different chip), please use it.
Excellent, I was thinking the same thing. No sense in having to maintain redundant code.
Looks good. And for a first patch, you managed to not only write the code, but also test it. Some hints about proper submission procedure are mentioned above.
Instead of a full review, I just wrote all this stuff in a special wiki page so others can benefit from this as well: http://www.coreboot.org/Flashrom/Random_notes . Please read it and comment if something is unclear.
thanks :)
As you can see, there are quite a lot of probe functions which seem to work fine (and that's mostly because of the ignored address bits). probe_jedec is the most-used function in our tree, so if the sequence looks ok, please use that one.
Ah, I hadn't realized that the high byte was ignored, that makes a big difference! If that's the case, then 29f002.c functions should work fine. I assume there is some difference between the 29f002 functions and the JEDEC ones, or else there would only be the JEDEC file. I'll look them both over this evening, and try flashing, and report back.
The reset command you've added at the beginning of erase_29f001() makes sense, but AFAICS it would make sense in the original erase_29f002() function as well, so please change that one and use it afterwards. The resent command I'm talking about is chip_writeb(0xF0, bios + 0x555);
I believe that was in the original file ['f002.c], and it's just commented out in my modified version ['f001.c]. If it is there [as in mx29f002.c], should there not be a small delay between the reset, and writing the erase commands? Either way, erase seems to function fine without the reset.
I guess I got a little too excited. I think I have a few more unlisted chips around here, I'll try and play with them in the future, and I'll make sure to check if existing functions will support them!
In the case where you have a file written for one chip - in this case mx29f002.c - and it is found to support multiple chips in the future, is it a good idea to rename it to something more descriptive? Perhaps in this case, it would be called mx29f00x.c ? Or is it better to keep the original name, do you think?
Mark