Julius Werner 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:
(6 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.h> rather than declare this in two places?
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@36 PS6, Line 36: static struct fb_info *list; Why not use <list.h>?
https://review.coreboot.org/c/coreboot/+/39002/6/src/lib/edid_fill_fb.c@95 PS6, Line 95: info->edid_fb.blue_mask_size = blue_mask_size; nit: at this point, isn't it simpler to just let the caller fill out the structure directly and then call something like fb_info_validate() to check invariants? Functions with a dozen arguments that are all integers aren't great, it's very easy to mix stuff up and not notice the error.
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. Can we ever have a framebuffer in the list that's not valid?
https://review.coreboot.org/c/coreboot/+/39002/6/src/lib/edid_fill_fb.c@148 PS6, Line 148: fb_fill_framebuffer_set_orientation nit: "fill" makes me think this fills out the whole thing, not just sets a single value. Why not just fb_set_orientation()?
https://review.coreboot.org/c/coreboot/+/39002/6/src/lib/edid_fill_fb.c@172 PS6, Line 172: for (struct fb_info *i = list; i != NULL; i = i->next) { If this is the only reason for having a list, you don't need a list. You just need a single pointer that you keep updating whenever someone creates a new valid FB.