Nico Huber posted comments on this change.

View Change

Patch set 3:

Rather clumsy. I don't like to do invasive edits on other people's
commits. Will write a follow-up, not sure if any line will stay.

(6 comments)

• File flash.h:

  • Patch Set #3, Line 176: } four_bytes_addr_funcs;

    This makes 4BA look like an alien...

• File flashrom.c:

  • Patch Set #3, Line 2227: if(flash->chip->feature_bits & FEATURE_4BA_SUPPORT) {

    missing space after `if`

  • Patch Set #3, Line 2240: if(flash->chip->four_bytes_addr_funcs.enter_4ba(flash)) {

    missing space

  • Patch Set #3, Line 2253: {

    Hmmm, what if another master expects the chip to be in 3BA mode?

• File spi.c:

  • Patch Set #3, Line 115: if (!(flash->chip->feature_bits & FEATURE_4BA_SUPPORT) &&

    What if 4BA is supported? Shouldn't we do the same check with `1 << 32`?

    (I can't tell. The whole `addrbase` thing looks like a layering
    violation to me that should be handled by programmers that actually
    do map the chip.)

• File spi25.c:

  • Patch Set #3, Line 1019: rc = (flash->chip->feature_bits & FEATURE_4BA_SUPPORT) == 0

    The condition should be checked in the called function, IMHO. This
    is too ugly...

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