Attention is currently required from: Daniel Kurtz, Martin Roth, Kevin Chang, Paul Menzel. Daniel Kurtz has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56100 )
Change subject: grunt/treeya: add Realtek ALC5682 codec support ......................................................................
Patch Set 4: Code-Review-1
(8 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/56100/comment/698f7f24_cad45217 PS4, Line 10: A very brief description of *why* this patch is needed and what hardware models it effects would be useful.
https://review.coreboot.org/c/coreboot/+/56100/comment/972580ac_54450765 PS4, Line 13: TEST=emerge-grunt coreboot I hope you did more than build test. Please explain what other tests were used to ensure this patch does as intended and no more or less.
File src/mainboard/google/kahlee/variants/treeya/audio.c:
https://review.coreboot.org/c/coreboot/+/56100/comment/ea58bb3e_4e8de77a PS4, Line 15: sku = google_chromeec_get_sku_id(); nit: I'd move this sku = ... down to where it is actually used (just before line 44). In fact, the variable isn't needed at all, just inline the function call:
switch (google_chromeec_get_sku_id()) {
https://review.coreboot.org/c/coreboot/+/56100/comment/7b48b608_4f792634 PS4, Line 21: if (mmio_dev == NULL) : break;
I think Paul is saying that this check should be your return.
Martin, is "== NULL" a common thing in coreboot?
In kernel land, this pattern would more often be "if (!mmio_dev)".
(and yes, to simplify the code, the printk & return from lines 25-28 should just be moved here)
https://review.coreboot.org/c/coreboot/+/56100/comment/56cbdc0a_fc46af6e PS4, Line 29: nit: I'd add a brief comment here describing what this next loop is doing.
https://review.coreboot.org/c/coreboot/+/56100/comment/fba345f3_748db6b9 PS4, Line 45: default: : /* da7219 only */ : if (da7219_dev) : da7219_dev->enabled = 1; : if (alc_dev) : alc_dev->enabled = 0; : break; : nit: It seems a bit awkward to have default first in a switch statement.
File src/mainboard/google/kahlee/variants/treeya/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/56100/comment/81e3a3c8_8bb97bf4 PS4, Line 126: 1a here you are explicitly configuring the Audio codec i2c address 0x1a (which is the same as da7219 above). Can you use this as the source of truth at runtime while matching, instead of hard-coding this same value as a constant in another file?
File src/soc/amd/stoneyridge/include/soc/iomap.h:
https://review.coreboot.org/c/coreboot/+/56100/comment/e3ff1b7b_a516224f PS4, Line 17: #define I2C_DEVICE_PATH 0x1a No, this board-specific audio component specific I2C address does not belong here in general SoC constants land.
Also I'm not sure what you mean here by "PATH".