Nico Huber posted comments on this change.

View Change

Patch set 2:Code-Review +1

Just nits, style and non-functional issues we can also handle in a
!fixup, I suppose. I can do that if nobody beats me to it. Or shall
I edit this change and you ack it again?

(12 comments)

• File flash.h:

  • Patch Set #2, Line 124: #define FEATURE_4BA_ONLY (1 << 11)

    nit, doesn't align well

• File spi4ba.c:

  • Patch Set #1, Line 127: + 1) - 1

    I'd eventually like to switch to using dynamically allocated memory
    so we don't duplicate all this code between 3BA and 4BA versions. I
    implemented that in a pending patch for the CrOS branch:
    https://chromium-review.googlesource.com/c/337202/1 . But that can
    happen later if desired.

    Actually we don't need dynamically allocated memory, just the offset
    for the memcpy() would have to be adjusted (and the buffer has to big
    enough ofc).

    But some abstraction like your new_spi_write_command() also crossed
    my mind during this review.

• File spi4ba.c:

  • Patch Set #2, Line 46:

    nit, spurious space

  • Patch Set #2, Line 74:

    same here

  • Patch Set #2, Line 77: msg_cerr("%s failed during command execution\n", __func__);

    Don't know the coding style very well, in coreboot a single-
    line body shouldn't have braces.

  • Patch Set #2, Line 84: uint8_t databyte)

    Inconsistent alignment, sometimes two tabs are used,
    sometimes tabs+spaces, here tabs only.

  • Patch Set #2, Line 100: (addr & 0xff),

    `>> 0` used in other places

  • Patch Set #2, Line 127: + 1) - 1

    I don't get what the +1-1 should tell the reader?

    Ok, guess now I understand it:
    +1 for the 4th address byte
    -1 for the single byte that is included in the 256

    Would be more clear if we'd define something like
    JEDEC_4_BYTE_ADDR_BYTE_PROGRAM_OUTSIZE (6) and
    JEDEC_4_BYTE_ADDR_PROGRAM_OUTSIZE (5).

    (This is all ugly shit, IMHO. But not due to this patch but the
    spi25 code that obviously was used as a template. The size of the
    static parts of the command should be implicitly given by the data,
    IMO. Currently the code suffers from redundancy that makes it har-
    der to read, i.e. I trust the compiler to do the calculations but
    not developers.)

  • Patch Set #2, Line 175: uint8_t *bytes, unsigned int len)

    alignment

  • Patch Set #2, Line 193: unsigned int blocklen)

    weirdest alignment

  • Patch Set #2, Line 237: 32

    Is this specified somewhere? IIRC, for 3BA it was inconsistent
    across chips.

  • Patch Set #2, Line 327: }

    flashrom the project of redundancy? I just reviewed the same
    function three times...

To view, visit change 20513. To unsubscribe, visit settings.