Attention is currently required from: Felix Singer, Nico Huber, Edward O'Callaghan, Angel Pons, Nikolai Artemiev. Anastasia Klimchuk 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 2:
(7 comments)
Patchset:
PS1:
ICH7 does not support hardware sequencing (hwseq).
To close the comment: I could not find devices with older chipsets, but Nico saved me and tested those!
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/58735/comment/e27aec77_250307e6 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, […]
Done in CB:60272. I actually discovered there is one line of code hidden between lots of debug messages! So I added new lines around it.
https://review.coreboot.org/c/flashrom/+/58735/comment/c065bebe_eb7d2c88 PS1, Line 1739: mmio_readl(spibar + offs), i);
I guess this also doesn't need a line break.
Done in CB:60272
https://review.coreboot.org/c/flashrom/+/58735/comment/2454aea2_a4687793 PS1, Line 1836: arg);
This is addressed in the follow-up commit, can we leave this as-is in this commit?
Oh yes, I removed this line break in CB:58736 , probably because I read a comment in that other patch first :) Is it alright?
https://review.coreboot.org/c/flashrom/+/58735/comment/4da5ee5e_73a11c84 PS1, Line 1917: );
yeah, the line break before `);` looks really odd.
Done in CB:60272. So many line breaks that I decided all of them can go into a separate commit.
https://review.coreboot.org/c/flashrom/+/58735/comment/56b79b0c_a7fe0eb3 PS1, Line 2049: else { : register_spi_master(&spi_master_ich9, NULL); : }
Not in this commit, please. […]
Do you mind if I do this idea separately (not in this chain)? I don't want to grow this chain too long. There is also another idea in another patch, and I know for sure I will be doing more work on ichspi next year.
https://review.coreboot.org/c/flashrom/+/58735/comment/b40d821e_a3774449 PS1, Line 2068: return 0;
You've replaced the `break` statements above with `return` ones. So this […]
Compiler says fine!
-
Anastasia Klimchuk (Code Review)