Hi Tom, On Fri, 18 Sept 2026 at 16:41, Tom Rini <[email protected]> wrote: > > On Fri, Sep 18, 2026 at 03:49:44PM -0500, Simon Glass wrote: > > Hi Julien, > > > > On 2026-09-01T08:52:42, Julien Stephan <[email protected]> wrote: > > > video: fix build with CONFIG_VIDEO && !CONFIG_VIDEO_LOGO > > > > > > Building with CONFIG_VIDEO enabled but CONFIG_VIDEO_LOGO disabled fails > > > at link time: > > > > > > video-uclass.o: in function `video_get_u_boot_logo': > > > video-uclass.c:593: undefined reference to `__splash_u_boot_logo_begin' > > > > > > The __splash_u_boot_logo_begin/_end symbols are provided by > > > u_boot_logo.bmp.o, which is only built when CONFIG_VIDEO_LOGO is set: > > > > > > obj-$(CONFIG_VIDEO_LOGO) += u_boot_logo.bmp.o > > > > > > video_get_u_boot_logo() and show_splash() reference those symbols > > > unconditionally, so with the logo disabled the reference is left > > > dangling. show_splash() alone would be dead-code eliminated (it is > > > static and only reached under a CONFIG_IS_ENABLED(VIDEO_LOGO) guard), > > > but video_get_u_boot_logo() is an exported function and is always > > > emitted. > > > > > > Guard the splash helpers and their symbol references with > > > CONFIG_IS_ENABLED(VIDEO_LOGO), and provide a static inline > > > video_get_u_boot_logo() stub returning NULL for the disabled case in > > > video.h. Callers already handle a NULL logo pointer (e.g. > > > bootflow_menu.c), so no caller changes are needed. > > > > > > Reproduce with any board that enables VIDEO without VIDEO_LOGO or > > > enabling SPLASH_SCREEN (it disables automatically VIDEO_LOGO). > > > > I won't comment on the word order / mixed tenses. > > > > > [...] > > > > > > drivers/video/video-uclass.c | 7 +++++-- > > > include/video.h | 9 ++++++++- > > > 2 files changed, 13 insertions(+), 3 deletions(-) > > > > > diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c > > > @@ -579,6 +579,7 @@ int video_get_ysize(struct udevice *dev) > > > return priv->ysize; > > > } > > > > > > +#if CONFIG_IS_ENABLED(VIDEO_LOGO) > > > #define SPLASH_DECL(_name) \ > > > extern u8 __splash_ ## _name ## _begin[]; \ > > > extern u8 __splash_ ## _name ## _end[] > > > > We try to avoid #ifdef in .c files and prefer if > > (CONFIG_IS_ENABLED(...)) so both branches are compile-checked. The > > SPLASH_DECL externs do need the preprocessor guard since the linker > > symbols don't exist, but the guard around show_splash() and its call > > site in video_post_probe() could be avoided by keeping show_splash() > > defined and providing an #else stub that returns 0. The existing if > > (CONFIG_IS_ENABLED(VIDEO_LOGO) && ...) at the caller can then stay. > > > > > diff --git a/include/video.h b/include/video.h > > > @@ -418,9 +418,16 @@ bool video_is_active(void); > > > /** > > > * video_get_u_boot_logo() - Get a pointer to the U-Boot logo > > > * > > > - * Returns: Pointer to logo > > > + * Returns: Pointer to logo, or NULL if CONFIG_VIDEO_LOGO is disabled > > > */ > > > > The stub is guarded with CONFIG_IS_ENABLED(VIDEO_LOGO), so the doc > > should refer to VIDEO_LOGO in general terms - SPL may have it disabled > > while proper enables it. Something like 'Returns: Pointer to logo, or > > NULL if VIDEO_LOGO is not enabled in this phase'. > > Simon, can you please make sure you're looking at both the current > version, and previous feedback, when commenting on older patches? > There's been a v2 for about 2 weeks. Thanks.
Ooops, yes will do. Regards, Simon
