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

[Maulik V Vaghela (Code Review)]

Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30182 )

Change subject: IntelFB: Add support for Intel font driver
......................................................................


Patch Set 3:

(8 comments)

> Patch Set 1:
> 
> (2 comments)

HI Patrick,
The use of this library will be to display message on screen once display has been initialized. For example,
1. We can display firmware update message to user 
2. We can also provide any BIOS setup option or display any information

I don't think it'll be used to display debug message on screen but rather enhance user experience.

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 FONT_SCALE 2
> This seems unnecessary, `fb_info` should already be short enough?
Done


https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/fbdriver.c@29 
PS1, Line 29: d
> no space, please
Done


https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/fbdriver.c@29 
PS1, Line 29: char *)(uintp
> Use `uintptr_t` instead of `unsigned long`. There's no guarantee […]
Done


https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/fbdriver.c@54 
PS1, Line 54: 	uint32_t pixels = fb_info.x_resolution * fb_info.y_resolution;
> Shouldn't be necessary if you use fill_lb_framebuffer() proper.
Done


https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/fbdriver.c@59 
PS1, Line 59: 	else
> space prohibited between function name and open parenthesis '('
Done


https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/fbdriver.c@60 
PS1, Line 60: DDR_PTR, 0x0
> bytes_per_line
Done


https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/fbdriver.c@61 
PS1, Line 61: 
> Cast to (void *) shouldn't be necessary.
Done


https://review.coreboot.org/#/c/30182/1/src/drivers/intel/fb/fbdriver.c@101 
PS1, Line 101: 
> why the reverse loop (from font_width down to 0)?
Done



-- 
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: 3
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: Thu, 13 Dec 2018 10:25:42 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: build bot (Jenkins) <no-reply at coreboot.org>
Comment-In-Reply-To: Nico Huber <nico.h at gmx.de>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20181213/c4472107/attachment.html>