[flashrom] [Patch] Added SST25LF080A Flash Chip

[Stefan Tauner]

On Tue, 12 Apr 2011 15:55:58 -0400
Zeus Castro <thezeusjuice at gmail.com> 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