[flashrom] [Patch] Added SST25LF080A Flash Chip

[Zeus Castro]

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