Attention is currently required from: Daniel Kurtz, Martin Roth, Paul Menzel. Kevin Chang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56100 )
Change subject: grunt/treeya: add Realtek ALC5682 codec support ......................................................................
Patch Set 7:
(8 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/56100/comment/ae4ed23a_fb66705e PS4, Line 10:
A very brief description of *why* this patch is needed and what hardware models it effects would be […]
Update in patchset 7.
https://review.coreboot.org/c/coreboot/+/56100/comment/56a1a72f_fa704952 PS4, Line 13: TEST=emerge-grunt coreboot
I hope you did more than build test. […]
Update in patchset 7.
File src/mainboard/google/kahlee/variants/treeya/audio.c:
https://review.coreboot.org/c/coreboot/+/56100/comment/aca83c00_d0a2c45b 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). […]
Update in patchset 7.
https://review.coreboot.org/c/coreboot/+/56100/comment/1b33719a_e672b402 PS4, Line 21: if (mmio_dev == NULL) : break;
(!mmio_dev) is probably the preferred form, but there are over 1000 places in the code where '== NUL […]
Update in patchset 7.
https://review.coreboot.org/c/coreboot/+/56100/comment/f6bf1ec3_b2dfba96 PS4, Line 29:
nit: I'd add a brief comment here describing what this next loop is doing.
Update in patchset 7.
https://review.coreboot.org/c/coreboot/+/56100/comment/d9bb25d4_434de4ae 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.
Update in patchset 7.
File src/mainboard/google/kahlee/variants/treeya/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/56100/comment/cdb1acda_c1f3442a PS4, Line 126: 1a
Unfortunately, that's not a simple request. […]
Following Martin's recommendation, I'm just planning on leaving this set to '1a' unless someone has a specific suggestion about how to update this.
File src/soc/amd/stoneyridge/include/soc/iomap.h:
https://review.coreboot.org/c/coreboot/+/56100/comment/2e2d8fc0_365da002 PS4, Line 17: #define I2C_DEVICE_PATH 0x1a
I think this should be something like: […]
Update in patchset 7.
-
Kevin Chang (Code Review)