Nice work, I didn't think it would be that simple :)

Beside coding style, there are some thinks that would probably
much easier be done now:

• Probing can be generic (i.e. decide the command bytes based on EEPROM size).
• _erase_emulation() can be much simpler if restricted to the page size.
• As we don't have to erase before writing FEATURE_NO_ERASE should work (erase emulation is still useful for -E).

View Change

8 comments:

• File flashchips.h:

  • Patch Set #1, Line 856: 0x00

    If we know it's supposed to be 0x00, why not check it?

• File flashchips.c:

  • Patch Set #1, Line 14114: .feature_bits = FEATURE_WRSR_WREN,

    FEATURE_NO_ERASE and FEATURE_ERASED_ZERO?

  • Patch Set #1, Line 14121: .eraseblocks = { { 256 * 1024, 1 } },

    Alternatively to handle pages in spi_block_erase_emulation(),
    we could declare this as { 256, 1 }. Actually, I think that
    would be much cleaner (you can also add a check there that
    `blocklen` matches the page size). So you can ignore my comment
    in `spi25.c`.

• File spi25.c:

  • Patch Set #1:

    (spi)25 != 95, please don't hesitate to start a new file. Or maybe
    we should rename this one to `spi_jedec.c`?

    General coding style is that of the Linux kernel with the exception
    that we allow up to 112 chars per line (if it helps readability).
    Use tabs for indentation (they count as 8 chars w.r.t. the line length).

  • Patch Set #1, Line 269: /* ST_M95_RDID_OUTSIZE depends on size of the flash and

    Please start multi-line comments with /* on a separate line. However this
    should fit on one line (112 char limit).

    Also, why not turn it into code? you can read the size of the
    EEPROM entry (flash->chip->total_size?), decide the number of
    bytes to send based on that?

  • Patch Set #1, Line 272: static const unsigned char cmd[ST_M95_RDID_OUTSIZE] = {ST_M95_RDID};

    spaces around ST_M95_RDID, please

  • Patch Set #1, Line 282: "L 0x%02x, M 0x%02x, H 0x%02x]\n",

    Please align output with the existing probing functions.

    `id1` and `id2` usually refer to the data read from the chip, afaik.

  • Patch Set #1, Line 635: }

    Alternatively, allocate `blocklen` bytes and let spi_write_chunked() take
    care of the pages.

To view, visit change 34496. To unsubscribe, or for help writing mail filters, visit settings.