Attention is currently required from: Edward O'Callaghan, Anastasia Klimchuk, Nikolai Artemiev. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58735 )
Change subject: ichspi: Split very long init function into two ......................................................................
Patch Set 1: Code-Review+1
(6 comments)
Patchset:
PS1: Looks quite nice overall, just one nit and a little whitspace.
Tested probe and read on Panther Point (7 series PCH). Data and verbose logs look the same.
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/58735/comment/ec6e3838_aa515ac0 PS1, Line 1717: msg_pdbg("0x00: 0x%04x (SPIS)\n", : mmio_readw(spibar + 0)); : msg_pdbg("0x02: 0x%04x (SPIC)\n", : mmio_readw(spibar + 2)); : msg_pdbg("0x04: 0x%08x (SPIA)\n", : mmio_readl(spibar + 4)); : ichspi_bbar = mmio_readl(spibar + 0x50); : msg_pdbg("0x50: 0x%08x (BBAR)\n", : ichspi_bbar); : msg_pdbg("0x54: 0x%04x (PREOP)\n", : mmio_readw(spibar + 0x54)); : msg_pdbg("0x56: 0x%04x (OPTYPE)\n", : mmio_readw(spibar + 0x56)); : msg_pdbg("0x58: 0x%08x (OPMENU)\n", : mmio_readl(spibar + 0x58)); : msg_pdbg("0x5c: 0x%08x (OPMENU+4)\n", : mmio_readl(spibar + 0x5c)); Please remove unnecessary line breaks. If you put tabs after the commas, it might actually look nice ;)
https://review.coreboot.org/c/flashrom/+/58735/comment/1d39bf50_93feff6e PS1, Line 1739: mmio_readl(spibar + offs), i); I guess this also doesn't need a line break.
https://review.coreboot.org/c/flashrom/+/58735/comment/f45726dd_4cd8430c PS1, Line 1836: arg); Another unnecessary line break.
https://review.coreboot.org/c/flashrom/+/58735/comment/7d6cdd3a_1da3ce53 PS1, Line 1917: ); Drop line break?
https://review.coreboot.org/c/flashrom/+/58735/comment/90b8becc_bbe3a304 PS1, Line 2068: return 0; You've replaced the `break` statements above with `return` ones. So this can't be reached anymore. Would the compiler mind if we drop it?