Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/48196 )
Change subject: Simplifying the initialisation flow for it85spi ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/flashrom/+/48196/2/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/48196/2/it85spi.c@317 PS2, Line 317: ce_high = ((unsigned char *)base) + 0xE00; /* 0xFFFFFE00 */ : ce_low = ((unsigned char *)base) + 0xD00; /* 0xFFFFFD00 */
this is common code, move it to after the ifdef blocks.
Done
https://review.coreboot.org/c/flashrom/+/48196/2/it85spi.c@331 PS2, Line 331: {
no need for '{ .. }' for single statements under a branch.
Done
https://review.coreboot.org/c/flashrom/+/48196/2/it85spi.c@341 PS2, Line 341: msg_pdbg("%s: internal_buses_supported=0x%x\n", __func__, : internal_buses_supported); : /* Check for FWH because IT85 listens to FWH cycles. : * FIXME: The big question is whether FWH cycles are necessary : * for communication even if LPC_IO is defined. : */ : if (internal_buses_supported & BUS_FWH) : msg_pdbg("Registering IT85 SPI.\n");
this whole block constitutes checks and prints of inputs parameters to this function and therefore s […]
I moved the first debug message above, but the last makes more sense to move below, just above register_spi_master. if (internal_buses_supported & BUS_FWH) becomes redundant because if this is false than function returns early and prints error message.
https://review.coreboot.org/c/flashrom/+/48196/2/it85spi.c@354 PS2, Line 354: if (!data) {
You are checking for nullity _after_ you deference here. Remember the pattern 'check-then-do'.
Done