Re: [PATCH 2/2] video: fix build with CONFIG_VIDEO && !CONFIG_VIDEO_LOGO

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 3379 bytes --]

On Fri, Sep 18, 2026 at 03:49:44PM -0500, Simon Glass wrote:
> Hi Julien,
> 
> On 2026-09-01T08:52:42, Julien Stephan <jstephan@baylibre.com> 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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]