Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30182 )
Change subject: IntelFB: Add support for Intel font driver ......................................................................
Patch Set 1:
(14 comments)
Hi Maulik, I think this driver could easily be modified to work with any coreboot framebuffer (not just FSP ones). Though, it's hard to anticipate the requirements without seeing it hooked up.
Is there already a patch in queue that makes use of this?
https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/Kconfig File src/drivers/intel/fb/Kconfig:
https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/Kconfig@3 PS1, Line 3: depends on ARCH_X86 The current implementation depends on PLATFORM_USES_FSP2_0 and RUN_FSP_GOP. Though, a generic implementation should only depend on LINEAR_FRAMEBUFFER.
https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/fbdriver.c File src/drivers/intel/fb/fbdriver.c:
https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/fbdriver.c@28 PS1, Line 28: #define FI fb_info This seems unnecessary, `fb_info` should already be short enough?
https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/fbdriver.c@29 PS1, Line 29: unsigned long Use `uintptr_t` instead of `unsigned long`. There's no guarantee that the latter has the same width as a pointer.
https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/fbdriver.c@29 PS1, Line 29: no space, please
https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/fbdriver.c@41 PS1, Line 41: 0xFF It seems more common in coreboot to write hex-digits in lower case, i.e. `0xff`. In any case, please make sure to use one style within this file.
Hmmm, also why not make it an explicit `unsigned char`?
https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/fbdriver.c@42 PS1, Line 42: / Please put spaces around the /'s
https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/fbdriver.c@51 PS1, Line 51: pci_write_config16(dev1, 0x04, 0x07); There are definitions for these registers/bits in `device/pci_def.h`.
I don't think, it's legal to activate IO and MMIO here, and also bus mastering should be taken care of by the device driver.
https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/fbdriver.c@53 PS1, Line 53: status = fsp_fill_lb_framebuffer (&fb_info);
space prohibited between function name and open parenthesis '('
If you'd use fill_lb_framebuffer() instead, this driver could work with all framebuffers in coreboot.
Please always check the return value.
https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/fbdriver.c@54 PS1, Line 54: fb_info.physical_address = pci_read_config32(dev1, 0x18); Shouldn't be necessary if you use fill_lb_framebuffer() proper.
https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/fbdriver.c@60 PS1, Line 60: x_resolution bytes_per_line
https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/fbdriver.c@61 PS1, Line 61: (void *) Cast to (void *) shouldn't be necessary.
https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/fbdriver.c@73 PS1, Line 73: fb_inverse = 0; : previous_write = 0; Why?
https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/fbdriver.c@79 PS1, Line 79: = 0 unnecessary initialization
https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/fbdriver.c@101 PS1, Line 101: for(x = font_width - 1; x >= 0; x--) {
space required before the open parenthesis '('
why the reverse loop (from font_width down to 0)?