Attention is currently required from: Michał Żygowski, Jakub Czapiga, Karol Zmyslowski, Stefan Reinauer, Michal Zygowski.
Maciej Pijanowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/73934 )
Change subject: util/inteltool: Add support for Jasper Lake ......................................................................
Patch Set 14:
(16 comments)
Patchset:
PS14: Still, a few comments left from me
File util/inteltool/gpio_names/jasperlake.h:
https://review.coreboot.org/c/coreboot/+/73934/comment/288ec564_56ae6ebd PS12, Line 8: const char *const jasperlake_pch_group_a_names[] = {
Done. […]
Done
https://review.coreboot.org/c/coreboot/+/73934/comment/b22a043a_d16d4307 PS12, Line 28: "GPP_A19", "PCHHOT_N", "n/a",
Done
Done
https://review.coreboot.org/c/coreboot/+/73934/comment/66f74b24_d0ac97f2 PS12, Line 63:
Done
Done
https://review.coreboot.org/c/coreboot/+/73934/comment/44051670_f649253d PS12, Line 64: "GSPI0_CLK_LOOPBK", "GSPI0_CLK_LOOPBK", "n/a", "n/a",
Done
Done
https://review.coreboot.org/c/coreboot/+/73934/comment/e759e0ab_545a31cf PS12, Line 214: "GPP_F04", "CNV_RF_RESET_N",
Done. […]
Done
https://review.coreboot.org/c/coreboot/+/73934/comment/af54df08_5176bff9 PS12, Line 226: "GPP_F18", "EMMC_RESET_N",
Done, but I could not find NF1 for this pad.
Done
https://review.coreboot.org/c/coreboot/+/73934/comment/5a0b2055_ea98ddd9 PS12, Line 274: "GPP_H19", "n/a", "n/a", "n/a",
Done
Done
https://review.coreboot.org/c/coreboot/+/73934/comment/90128ece_624e07b9 PS12, Line 308: const struct gpio_group jasperlake_pch_group_spi0 = {
Done
Done
https://review.coreboot.org/c/coreboot/+/73934/comment/b402cd26_ec57e6b0 PS12, Line 458: const struct gpio_group jasperlake_pch_group_sys_pcie = {
Done
Done
https://review.coreboot.org/c/coreboot/+/73934/comment/41ce2225_8c91c0bf PS12, Line 480: const struct gpio_group jasperlake_pch_group_sys_vusb = {
Done
Done
https://review.coreboot.org/c/coreboot/+/73934/comment/63b64b3b_6c0fb386 PS12, Line 539: // Deep Sleep Well group
Done
What was done? I see no change here. I would either remove the comment, or add empt line before, at least.
https://review.coreboot.org/c/coreboot/+/73934/comment/dd6388b6_d6c9cb6d PS12, Line 552:
Just for visibility. […]
Done
https://review.coreboot.org/c/coreboot/+/73934/comment/57d5c4fe_efb8084b PS12, Line 553: "GPD_INPUT3VSEL", "GPD_INPUT3VSEL",
You broke indents in this group.
... and not fixed
https://review.coreboot.org/c/coreboot/+/73934/comment/5ae623dd_03f3cf27 PS12, Line 584:
Only for reference with original Intel documentation. […]
I'm not against adding comments. I am against the lack of consistency in empty lines between commends and code. This still needs to be fixed.
File util/inteltool/gpio_names/jasperlake.h:
https://review.coreboot.org/c/coreboot/+/73934/comment/7d4c2766_6db960b1 PS9, Line 281: "GSPI2_CLK_LOOPBK", "GSPI2_CLK_LOOPBK",
Done
Done
-
Maciej Pijanowski (Code Review)