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 7:
(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;
`framebuffer_info.h` is the public API and doesn't provide more information (doesn't share internals either).
Yeah, that's why I'm unclear why we wouldn't just #include it. It does provide the declaration of struct fb_info which is all that's needed here.
https://review.coreboot.org/c/coreboot/+/39002/7/src/lib/edid_fill_fb.c File src/lib/edid_fill_fb.c:
https://review.coreboot.org/c/coreboot/+/39002/7/src/lib/edid_fill_fb.c@34 PS7, Line 34: edid_fb nit: Would probably be a good occasion to rename this field since it really has nothing to do with EDID anymore. Just 'fb' would probably be fine?
https://review.coreboot.org/c/coreboot/+/39002/7/src/lib/edid_fill_fb.c@36 PS7, Line 36: static struct fb_info list; This should just be a raw struct list_node.
https://review.coreboot.org/c/coreboot/+/39002/7/src/lib/edid_fill_fb.c@56 PS7, Line 56: struct fb_info *info, Why does this work so differently from fb_set_framebuffer_info()? Why can't you call fb_new_framebuffer_info() internally from here too (so you don't have to export that function at all)? If you're just doing this to share code, you can have a static function that does this, but the exported wrappers should have a consistent interface.
https://review.coreboot.org/c/coreboot/+/39002/7/src/lib/edid_fill_fb.c@63 PS7, Line 63: uint8_t blue_mask_size) I still really don't like this super long function signature. How about passing in a struct lb_framebuffer pointer instead (which is memcpied to the new fb_info) so that the caller can pass the same values, but in a more structured (less error prone) manner?
https://review.coreboot.org/c/coreboot/+/39002/7/src/lib/edid_fill_fb.c@109 PS7, Line 109: set 'set' seems like the wrong verb here, especially in comparison to fb_set_orientation() (which changes an existing fb_info rather than create a new one). Maybe fb_add_framebuffer_info() would be better?