Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39002 )
Change subject: lib/edid_fill_fb: Support multiple framebuffers ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39002/6/src/include/edid.h File src/include/edid.h:
https://review.coreboot.org/c/coreboot/+/39002/6/src/include/edid.h@107 PS6, Line 107: struct fb_info;
Shouldn't you #include <framebuffer_info. […]
I don't see why exposing the internals helps here. The callers doesn't need to know anything but the function prototypes. This way all users are forced to use the API as it's intended to.
https://review.coreboot.org/c/coreboot/+/39002/6/src/include/framebuffer_inf... File src/include/framebuffer_info.h:
https://review.coreboot.org/c/coreboot/+/39002/6/src/include/framebuffer_inf... PS6, Line 37: bool fb_fill_framebuffer_info(struct fb_info *info,
Does this function really need to be exported separately? I only see it used once with the xgifb stu […]
It's only used in xgifb_modeset. All methods that could be called multiple times use the *_fill_* methods. It doesn't make much sense to call those methods multiple times, so I'll rework those first.
https://review.coreboot.org/c/coreboot/+/39002/6/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/6/src/lib/edid_fill_fb.c@96 PS6, Line 96: info->valid = true;
I'm a bit confused who needs this. […]
yes, if it doesn't pass the pre check: " if (!x_resolution || !y_resolution || !bytes_per_line || !bits_per_pixel)"
Seems to be a bad design, I'll rework that.