Hello.
disclamer: this is my first patch review for flashrom.
disclamer: this is my first patch ever :-) Also, although I'm a decent programmer, my knowledge of C is pretty limited.
Based on the datasheets of both, the SST25LF080A is basically a larger version of the SST25LF040A. However, flashrom had an entry for SST25LF040A, but not for SST25LF080A. As such, I modeled it after the entry for SST25LF040A, and altered the values that differed.
- .name = "SST25LF080A.RES",
i dont see a reason why you are using .RES here. according to the datasheet REMS work as well as RES. but maybe i dont know some detail.
- .probe = probe_spi_res2,
ok, or probe_spi_rems
I was confused as well as to why the oscillation between REMS, RES, and RES2 in the SST25LF040A entry, but figured that there was probably a reason I did not understand, and copied it over in the same manner. So, you recommend I change it all to REMS?
Index: flashchips.h
-#define SST_SST25VF080_REMS 0x80 /* REMS or RES opcode */ +#define SST_SST25VF080_REMS 0x80 /* REMS or RES opcode, same as SST25LF080A */
since the SST25VF080 is not implemented and that define is not used at all as far as i (and grep) can see, there is no reason to use the rems suffix imho.
Apparently SST25VFxxx and SST25LFxxx chips often share the same REMS/RES device id. And since the one for SST25LF040A was "tacked on" to the define for SST25VF040_REMS (since they use the same byte), and there was an already existing define for SST25VF080_REMS (which uses the same byte as I was planning to include for the SST25LF080A), I just "tacked on" the SST25LF080A in the same manner. So, you recommend I change it to read?: #define SST_SST25LF080A 0x80"
- .tested = TEST_UNTESTED,
did you really not test it? why have you added it?
The only sample I have is currently inside my laptop, and flashrom access to it is being blocked by the EC/KBC (an ENE KB926). Besides, there are about 170 untested chips in flashchips.c, so I figured one more, especially based on an incremental upgrade to a probe-tested chip (SST25LF040A), wouldn't be problematic.
- .unlock = spi_disable_blockprotect,
this will not work if bit 7 (BPL) is set some of the at25* write protections are similar. one might use them.
spi_disable_blockprotect is sufficient for this chip: The important bits are 2-5 (although this chip only cares about 2 and 3). If those bits are 0, the chip isn't protected. If there is protection, spi_disable_blockprotect correctly attempts to set them to 0, and checks if it succeeded. The only way it can fail is if bit 7 is set and the Write Protect Pin (WP#) is being pulled low, in which case there is nothing that can be done (block protection cannot be disabled without reseting the chip) and spi_disable_blockprotect returns 1. No additional functionality is possible.
Best regards, Zeus Castro