Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34537 )
Change subject: src/device/oprom: Fix bootsplash display code for optionroms ......................................................................
Patch Set 17:
(10 comments)
https://review.coreboot.org/c/coreboot/+/34537/15/src/device/Makefile.inc File src/device/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/34537/15/src/device/Makefile.inc@51 PS15, Line 51: ramstage-$(CONFIG_BOOTSPLASH) += ../lib/bootsplash.c
You can just modify lib/Makefile.inc instead, jpeg.c is probably there already.
Done
https://review.coreboot.org/c/coreboot/+/34537/15/src/device/oprom/realmode/... File src/device/oprom/realmode/x86.c:
https://review.coreboot.org/c/coreboot/+/34537/15/src/device/oprom/realmode/... PS15, Line 225: int mode_info_valid;
I think you can declare those static, and do: […]
Done
https://review.coreboot.org/c/coreboot/+/34537/15/src/device/oprom/yabel/vbe... File src/device/oprom/yabel/vbe.c:
https://review.coreboot.org/c/coreboot/+/34537/15/src/device/oprom/yabel/vbe... PS15, Line 722: }
Like before.
Done
https://review.coreboot.org/c/coreboot/+/34537/15/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/c/coreboot/+/34537/15/src/device/pci_device.c@77... PS15, Line 772: #if CONFIG(BOOTSPLASH)
I disagree, as lib/bootsplash should be generic without the dependency of handling vesa related thin […]
And pci_device.c should be even more unaware of vesa headers than bootsplash.c.
https://review.coreboot.org/c/coreboot/+/34537/15/src/device/pci_device.c@78... PS15, Line 788: timestamp_add_now(TS_OPROM_END);
I agree. […]
Done
https://review.coreboot.org/c/coreboot/+/34537/15/src/include/vbe.h File src/include/vbe.h:
https://review.coreboot.org/c/coreboot/+/34537/15/src/include/vbe.h@105 PS15, Line 105: #if CONFIG(FRAMEBUFFER_SET_VESA_MODE)
Okay. […]
Done
https://review.coreboot.org/c/coreboot/+/34537/15/src/include/vbe.h@107 PS15, Line 107: int vbe_mode_info_valid(void);
Will use the function approach instead.
Done
https://review.coreboot.org/c/coreboot/+/34537/17/src/lib/bootsplash.h File src/lib/bootsplash.h:
https://review.coreboot.org/c/coreboot/+/34537/17/src/lib/bootsplash.h@4 PS17, Line 4: * Copyright (C) 2019 Johanna Schander coreboot@mimoja.de We have sort of stopped filling names in license headers. You can have it but it may just get stripped out in near future.
https://review.coreboot.org/c/coreboot/+/34537/17/src/lib/bootsplash.h@15 PS17, Line 15: We generally always guard header files
#ifndef __BOOTSPLASH_H__ #define __BOOTSPLASH_H__ .... #endif
https://review.coreboot.org/c/coreboot/+/34537/17/src/lib/bootsplash.c File src/lib/bootsplash.c:
https://review.coreboot.org/c/coreboot/+/34537/17/src/lib/bootsplash.c@19 PS17, Line 19: #include "bootsplash.h" IMHO <bootsplash.h> and the file added would be src/include/bootsplash.h