Re: [flashrom] [PATCH] Add flash chip definition for ESI ES25P16

[Stefan Tauner]

On Thu, 28 Jan 2016 23:15:38 +0530
Hatim Kanchwala  wrote:
> Signed-off-by: Hatim Kanchwala
> Index: flashchips.c
>
> --- flashchips.c	(revision 1919)
> +++ flashchips.c	(working copy)
> @@ -5273,4 +5273,35 @@
>
> {
> .vendor              = "ESI",
> .name                = "ES25P16",
> .bustype             = BUS_SPI,
> .manufacture_id      = EXCEL_ID_NOPREFIX,
> .model_id            = EXCEL_ES25P16,
> .total_size          = 2 * 1024,
> .page_size           = 256,
> /* 256-byte paramter page separate from memory array */
> /* Supports read (0x53), fast read (0x5B), erase (0xD5) and program
>
> (0x52) parameter page instructions. */

Hello Hatim,

thanks for your patch. This one (I have not looked at the second one
yet) was mangled by your mail client AFAICT. It replaced some tabs with
spaces and add some line breaks as above. The easiest way (IMHO) to
send patch is to use git-svn and use git's send-email command. With
that you don't even need to open your email client and your patches are
guaranteed to be delivered correctly.
Also, you broke our 112 characters per line rule (cf.
http://www.flashrom.org/pipermail/flashrom/2012-May/009228.html) with
the comment above and thus I have rewritten it slightly.

Apart from the formatting issues there was another minor issues (which
nevertheless forced me to fix it manually) is the tiny amount of
context lines in your patch. As explicitly stated in the development
guidelines it is a good practice to add more context lines to patches
that touch flashchips.c because the giant table has quite little
entropy per line and thus patches often don't apply correctly but are
shifted and sometimes newly added chips even start in the middle of
another chip, cf.
https://www.flashrom.org/Development_Guidelines#Patch_submission
> .feature_bits        = FEATURE_WRSR_WREN,
> .tested              = TEST_UNTESTED,
> .probe               = probe_spi_rdid,
> .probe_timing        = TIMING_ZERO,
> .block_erasers       =
> {
>   {
>     .eraseblocks = { {64 * 1024, 32} },
>     .block_erase = spi_block_erase_d8,
>   }, {
>     .eraseblocks = { {1024 * 2048, 1} },

The actual content of your patch is nearly perfect. Only one small
nitpick regarding the eraseblock size: we write it usually as "x *
1024" that is to be read as "x kB". So in this case it should be
2048 * 1024, or 2 * 1024 * 1024 (which can simply be read as 2 MB).
>     .block_erase = spi_block_erase_c7,
>   }
> },
> .printlock           = spi_prettyprint_status_register_bp2_srwd,
> .unlock              = spi_disable_blockprotect_bp2_srwd,
> .write               = spi_chip_write_256,
>            .read                = spi_chip_read, /* Fast Read
>
> (0x0B) supported */
> .voltage             = {2700, 3600},
> },
> {
> .vendor		= "Fujitsu",
> .name		= "MBM29F004BC",

No need to change anything. I will commit the patch together with the
second one.