Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39613 )
Change subject: sc7180: Add display 10nm phy & pll programming support [Patch 1 of 3] ......................................................................
Patch Set 17:
(12 comments)
https://review.coreboot.org/c/coreboot/+/39613/17//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39613/17//COMMIT_MSG@7 PS17, Line 7: sc7180: Add display 10nm phy & pll programming support [Patch 1 of 3] We do not use the *PATCH 1 of 3* tags in coreboot. Just having them in the same branch, and describing the work in the commit message is enough.
https://review.coreboot.org/c/coreboot/+/39613/17//COMMIT_MSG@11 PS17, Line 11: This is too terse for a 2500 lines diffstat. Please elaborate, elaborate on the implementation/data structures, mention the used datasheets and their version, and, if not written from scratch, where the code comes from.
https://review.coreboot.org/c/coreboot/+/39613/17/src/soc/qualcomm/sc7180/di... File src/soc/qualcomm/sc7180/display/dsi_phy.c:
https://review.coreboot.org/c/coreboot/+/39613/17/src/soc/qualcomm/sc7180/di... PS17, Line 14: */ The project uses SPDX headers now.
https://review.coreboot.org/c/coreboot/+/39613/17/src/soc/qualcomm/sc7180/di... PS17, Line 30: HAL_DSI_PHY_PLL_READY_TIMEOUT Please add the unit into the macro name.
https://review.coreboot.org/c/coreboot/+/39613/17/src/soc/qualcomm/sc7180/di... PS17, Line 45: { 2, 11}, Please add a space before the closing }.
https://review.coreboot.org/c/coreboot/+/39613/17/src/soc/qualcomm/sc7180/di... PS17, Line 81: */ Please follow https://doc.coreboot.org/coding_style.html#commenting.
https://review.coreboot.org/c/coreboot/+/39613/17/src/soc/qualcomm/sc7180/di... PS17, Line 1044: int Please use CB_SUCCESS and friends.
https://review.coreboot.org/c/coreboot/+/39613/17/src/soc/qualcomm/sc7180/di... PS17, Line 1049: printk(BIOS_ERR, "%s: pinfo NULL, error\n", __func__); Error messages should be user understandable. This is written like a debug message. Please elaborate.
https://review.coreboot.org/c/coreboot/+/39613/17/src/soc/qualcomm/sc7180/di... PS17, Line 1053: set up The noun is spelled without a space. Maybe:
Set up Phy
https://review.coreboot.org/c/coreboot/+/39613/17/src/soc/qualcomm/sc7180/di... PS17, Line 1056: goto out; Just return?
https://review.coreboot.org/c/coreboot/+/39613/17/src/soc/qualcomm/sc7180/di... File src/soc/qualcomm/sc7180/display/dsi_phy_pll.c:
https://review.coreboot.org/c/coreboot/+/39613/17/src/soc/qualcomm/sc7180/di... PS17, Line 332: Please remove the blank lines at the end of the file.
https://review.coreboot.org/c/coreboot/+/39613/17/src/soc/qualcomm/sc7180/in... File src/soc/qualcomm/sc7180/include/soc/display/display_resources.h:
https://review.coreboot.org/c/coreboot/+/39613/17/src/soc/qualcomm/sc7180/in... PS17, Line 49: Please remove the blank lines.