On Tue, 12 Apr 2011 15:55:58 -0400
Zeus Castro <[email protected]> wrote:

> Added support for SST25LF080A flash chip, based on public datasheet
> available here: http://www.sst.com/dotAsset/40316.pdf

hello <awesomely named guy> :)

disclamer: this is my first patch review for flashrom.

> 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.

> Index: flashchips.c
the whole block should be moved up (with the following SST25LF040A chip
too). it is not alphabetically sorted.

> ===================================================================
> --- flashchips.c      (revision 1285)
> +++ flashchips.c      (working copy)
> @@ -5342,6 +5342,35 @@
>  
>       {
>               .vendor         = "SST",
> +             .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.

> +             .bustype        = CHIP_BUSTYPE_SPI,
> +             .manufacture_id = SST_ID,
> +             .model_id       = SST_SST25VF080_REMS,
> +             .total_size     = 1024,
ok

> +             .page_size      = 256,
ok, because page_size on the whole is fucked up. :)

> +             .tested         = TEST_UNTESTED,
did you really not test it? why have you added it?

> +             .probe          = probe_spi_res2,
ok, or probe_spi_rems

> +             .probe_timing   = TIMING_ZERO,
> +             .block_erasers  =
> +             {
> +                     {
> +                             .eraseblocks = { {4 * 1024, 256} },
> +                             .block_erase = spi_block_erase_20,
> +                     }, {
> +                             .eraseblocks = { {32 * 1024, 32} },
> +                             .block_erase = spi_block_erase_52,
> +                     }, {
> +                             .eraseblocks = { {1024 * 1024, 1} },
> +                             .block_erase = spi_block_erase_60,
> +                     },
> +             },
correct

> +             .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.

> +             .write          = spi_chip_write_1,
> +             .read           = spi_chip_read,
> +     },
> +
> +     {
> +             .vendor         = "SST",
>               .name           = "SST25LF040A.RES",
>               .bustype        = CHIP_BUSTYPE_SPI,
>               .manufacture_id = SST_ID,
ok


-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner

_______________________________________________
flashrom mailing list
[email protected]
http://www.flashrom.org/mailman/listinfo/flashrom

Reply via email to