Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32511 )
Change subject: mb/google/kukui: Initialize display ......................................................................
Patch Set 28:
(16 comments)
https://review.coreboot.org/c/coreboot/+/32511/14//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/32511/14//COMMIT_MSG@7 PS14, Line 7: google/kukui: Elaborate panel support for Kukui family boards.
Please remove the dot/period at the end.
Ack
https://review.coreboot.org/c/coreboot/+/32511/14//COMMIT_MSG@10 PS14, Line 10: inf
information?
Ack
https://review.coreboot.org/c/coreboot/+/32511/14//COMMIT_MSG@14 PS14, Line 14: it's
its
Ack
https://review.coreboot.org/c/coreboot/+/32511/14//COMMIT_MSG@14 PS14, Line 14: it's
its
Ack
https://review.coreboot.org/c/coreboot/+/32511/14//COMMIT_MSG@14 PS14, Line 14:
one space
Ack
https://review.coreboot.org/c/coreboot/+/32511/8/src/mainboard/google/kukui/... File src/mainboard/google/kukui/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/32511/8/src/mainboard/google/kukui/... PS8, Line 26: y
$(CONFIG_BOARD_GOOGLE_KUKUI)
Ack
https://review.coreboot.org/c/coreboot/+/32511/14/src/mainboard/google/kukui... File src/mainboard/google/kukui/display.h:
https://review.coreboot.org/c/coreboot/+/32511/14/src/mainboard/google/kukui... PS14, Line 66: /* : * board related functions : */
Are these comments needed?
Ack
https://review.coreboot.org/c/coreboot/+/32511/8/src/mainboard/google/kukui/... File src/mainboard/google/kukui/display.c:
https://review.coreboot.org/c/coreboot/+/32511/8/src/mainboard/google/kukui/... PS8, Line 98: if (CONFIG(BOARD_GOOGLE_KUKUI)) : return &kukui_display_intf; : else : return NULL;
This implies we'll need to include all interfaces for any builds. […]
Ack
https://review.coreboot.org/c/coreboot/+/32511/8/src/mainboard/google/kukui/... PS8, Line 116: return ERR;
return of an errno should typically be negative (ie: return -ERR)
Ack
https://review.coreboot.org/c/coreboot/+/32511/14/src/mainboard/google/kukui... File src/mainboard/google/kukui/display.c:
PS14:
Did you run this through clang-format?
Ack
https://review.coreboot.org/c/coreboot/+/32511/14/src/mainboard/google/kukui... PS14, Line 38: printk(BIOS_ERR, "%s: wrong parameters\n", __func__);
Error messages should be more elaborate, so a user can understand it.
Ack
https://review.coreboot.org/c/coreboot/+/32511/14/src/mainboard/google/kukui... PS14, Line 52: printk(BIOS_ERR, "dsi init fail\n");
Maybe? […]
Ack
https://review.coreboot.org/c/coreboot/+/32511/14/src/mainboard/google/kukui... PS14, Line 93: /* Exported Functions */
Are these comments neeeded?
Ack
https://review.coreboot.org/c/coreboot/+/32511/14/src/mainboard/google/kukui... PS14, Line 113: -1
We have error codes in coreboot. […]
Ack
https://review.coreboot.org/c/coreboot/+/32511/24/src/mainboard/google/kukui... File src/mainboard/google/kukui/mainboard.c:
https://review.coreboot.org/c/coreboot/+/32511/24/src/mainboard/google/kukui... PS24, Line 119: int
boolean or the coreboot CB_SUCCESS/ERROR enums.
Done
https://review.coreboot.org/c/coreboot/+/32511/14/src/mainboard/google/kukui... File src/mainboard/google/kukui/panel_kukui.c:
https://review.coreboot.org/c/coreboot/+/32511/14/src/mainboard/google/kukui... PS14, Line 34: .name = "768x1024@60Hz",
Isn’t it landscape?
Ack