Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Nikolai Artemiev, Sergii Dmytruk.

View Change

7 comments:

• File libflashrom.h:

  • Patch Set #9, Line 131:

    int flashrom_wp_read_chip_config(const struct flashrom_flashctx *flash, struct flashrom_wp_chip_config *cfg);
    int flashrom_wp_write_chip_config(struct flashrom_flashctx *flash, const struct flashrom_wp_chip_config *cfg);
    int flashrom_wp_compare_chip_configs(const struct flashrom_wp_chip_config *cfga, const struct flashrom_wp_chip_config *cfgb);
    

    You can omit parameter names if they are self-explanatory from parameter type, which all of these seems to be (see existing declarations in this header).

• File writeprotect.c:

  • Patch Set #9, Line 4: 2010

    Does anyone know, should we add 2021 here? Does it even matter?
    The file is completely re-written.

  • Patch Set #9, Line 27:

    #define wp_chip_config flashrom_wp_chip_config
    #define wp_range flashrom_wp_range
    #define wp_mode flashrom_wp_mode
    

    Oh, so why this prefix was added from the very beginning? ;)

  • Patch Set #9, Line 39:

    if (spi_read_register(flash, bit.reg, value))
    			return 1;
    

    Let's propagate return value,
     ret = spi_read_register
     if (ret)
     return ret

    Maybe now it's always 1 on error, but who knows, may change in future.

  • Patch Set #9, Line 128: return ret;

    This should be on its own line (and same below).

  • Patch Set #9, Line 158: if (ord == 0) ord = cfga->srp[i] - cfgb->srp[i];

    How does it work? If ord becomes non-0 in the first loop cycle, then the rest of (ord == 0) are skipped? is it the same as having `else return ord`?

  • Patch Set #9, Line 161: ord = cfga->cmp - cfgb->cmp;

    Please make it on its own line, and the same for other conditionals below (including ones inside for loops).

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