[coreboot-gerrit] Change in ...coreboot[master]: IntelFB: Add support for Intel font driver

Wed Dec 12 13:22:13 CET 2018

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)?

-- 
To view, visit https://review.coreboot.org/c/coreboot/+/30182
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I70715d9c84dae2d0efa1631649ce27ea3c39e237
Gerrit-Change-Number: 30182
Gerrit-PatchSet: 1
Gerrit-Owner: Maulik V Vaghela <maulik.v.vaghela at intel.com>
Gerrit-Reviewer: Balaji Manigandan <balaji.manigandan at intel.com>
Gerrit-Reviewer: Martin Roth <martinroth at google.com>
Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela at intel.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi at google.com>
Gerrit-Reviewer: Patrick Rudolph <siro at das-labor.org>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi at intel.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik at intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-CC: Nico Huber <nico.h at gmx.de>
Gerrit-Comment-Date: Wed, 12 Dec 2018 12:22:13 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20181212/cbd89e55/attachment.html>