| Message ID | 20230829142109.4521-6-tzimmermann@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | fbdev: Split off code for boot-up logo | expand |
On 8/29/23 16:15, Thomas Zimmermann wrote: > Only build fb_logo.c if CONFIG_LOGO has been selected. Otherwise > provide empty implementations of the contained interfaces and avoid > using the exported variables. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> ... > diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c > index f157a5a1dffc..24b038510a71 100644 > --- a/drivers/video/fbdev/core/fbcon.c > +++ b/drivers/video/fbdev/core/fbcon.c > @@ -474,15 +474,19 @@ static int __init fb_console_setup(char *this_opt) > > if (!strncmp(options, "logo-pos:", 9)) { > options += 9; > +#ifdef CONFIG_LOGO > if (!strcmp(options, "center")) > fb_center_logo = true; > +#endif IMHO, *sometimes* it makes sense to not use #ifdef and code it instead like this: if (IS_ENABLED(CONFIG_LOGO) && !strcmp(options, "center")) ... That way the compiler will optimize that code away as well, but in addition it will compile-check that you have correct coding independend if CONFIG_LOGO is set or not. > continue; > } > > if (!strncmp(options, "logo-count:", 11)) { > options += 11; > +#ifdef CONFIG_LOGO > if (*options) > fb_logo_count = simple_strtol(options, &options, 0); > +#endif same here. Helge
Hi Am 01.09.23 um 10:22 schrieb Helge Deller: > On 8/29/23 16:15, Thomas Zimmermann wrote: >> Only build fb_logo.c if CONFIG_LOGO has been selected. Otherwise >> provide empty implementations of the contained interfaces and avoid >> using the exported variables. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > ... >> diff --git a/drivers/video/fbdev/core/fbcon.c >> b/drivers/video/fbdev/core/fbcon.c >> index f157a5a1dffc..24b038510a71 100644 >> --- a/drivers/video/fbdev/core/fbcon.c >> +++ b/drivers/video/fbdev/core/fbcon.c >> @@ -474,15 +474,19 @@ static int __init fb_console_setup(char *this_opt) >> >> if (!strncmp(options, "logo-pos:", 9)) { >> options += 9; >> +#ifdef CONFIG_LOGO >> if (!strcmp(options, "center")) >> fb_center_logo = true; >> +#endif > > IMHO, *sometimes* it makes sense to not use #ifdef and code it instead > like this: > if (IS_ENABLED(CONFIG_LOGO) && > !strcmp(options, "center")) > ... > That way the compiler will optimize that code away as well, but in > addition it will compile-check that you have correct coding independend > if CONFIG_LOGO is set or not. Good idea. I'll change it. The IS_ENABLED code is also easier to read IMHO. Best regards Thomas > >> continue; >> } >> >> if (!strncmp(options, "logo-count:", 11)) { >> options += 11; >> +#ifdef CONFIG_LOGO >> if (*options) >> fb_logo_count = simple_strtol(options, &options, 0); >> +#endif > > same here. > > Helge
Thomas Zimmermann <tzimmermann@suse.de> writes: > Only build fb_logo.c if CONFIG_LOGO has been selected. Otherwise > provide empty implementations of the contained interfaces and avoid > using the exported variables. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> Ah! You are doing in this patch exactly what I mentioned in my previous email :) I would just squash this patch with #4, but up to you. Acked-by: Javier Martinez Canillas <javierm@redhat.com>
Am 04.09.23 um 09:08 schrieb Thomas Zimmermann: > Hi > > Am 01.09.23 um 10:22 schrieb Helge Deller: >> On 8/29/23 16:15, Thomas Zimmermann wrote: >>> Only build fb_logo.c if CONFIG_LOGO has been selected. Otherwise >>> provide empty implementations of the contained interfaces and avoid >>> using the exported variables. >>> >>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> ... >>> diff --git a/drivers/video/fbdev/core/fbcon.c >>> b/drivers/video/fbdev/core/fbcon.c >>> index f157a5a1dffc..24b038510a71 100644 >>> --- a/drivers/video/fbdev/core/fbcon.c >>> +++ b/drivers/video/fbdev/core/fbcon.c >>> @@ -474,15 +474,19 @@ static int __init fb_console_setup(char *this_opt) >>> >>> if (!strncmp(options, "logo-pos:", 9)) { >>> options += 9; >>> +#ifdef CONFIG_LOGO >>> if (!strcmp(options, "center")) >>> fb_center_logo = true; >>> +#endif >> >> IMHO, *sometimes* it makes sense to not use #ifdef and code it instead >> like this: >> if (IS_ENABLED(CONFIG_LOGO) && >> !strcmp(options, "center")) >> ... >> That way the compiler will optimize that code away as well, but in >> addition it will compile-check that you have correct coding independend >> if CONFIG_LOGO is set or not. > > Good idea. I'll change it. The IS_ENABLED code is also easier to read IMHO. I'll keep the current approach, but in a simplified form. > > Best regards > Thomas > >> >>> continue; >>> } >>> >>> if (!strncmp(options, "logo-count:", 11)) { >>> options += 11; >>> +#ifdef CONFIG_LOGO >>> if (*options) >>> fb_logo_count = simple_strtol(options, &options, 0); >>> +#endif >> >> same here. >> >> Helge >
Am 06.09.23 um 12:12 schrieb Javier Martinez Canillas: > Thomas Zimmermann <tzimmermann@suse.de> writes: > >> Only build fb_logo.c if CONFIG_LOGO has been selected. Otherwise >> provide empty implementations of the contained interfaces and avoid >> using the exported variables. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > > Ah! You are doing in this patch exactly what I mentioned in my previous > email :) > > I would just squash this patch with #4, but up to you. I'll refactor these patches slightly: The unexporting of the helpers goes into a new patch and the rest of patches 4 and 5 will be squashed. > > Acked-by: Javier Martinez Canillas <javierm@redhat.com> >
diff --git a/drivers/video/fbdev/core/Makefile b/drivers/video/fbdev/core/Makefile index adce31155e92..36d3156dc759 100644 --- a/drivers/video/fbdev/core/Makefile +++ b/drivers/video/fbdev/core/Makefile @@ -2,7 +2,6 @@ obj-$(CONFIG_FB_NOTIFY) += fb_notify.o obj-$(CONFIG_FB_CORE) += fb.o fb-y := fb_info.o \ - fb_logo.o \ fbmem.o fbcmap.o \ modedb.o fbcvt.o fb_cmdline.o fb_io_fops.o ifdef CONFIG_FB @@ -24,6 +23,8 @@ fb-y += fbcon_rotate.o fbcon_cw.o fbcon_ud.o \ endif endif +fb-$(CONFIG_LOGO) += fb_logo.o + obj-$(CONFIG_FB_CFB_FILLRECT) += cfbfillrect.o obj-$(CONFIG_FB_CFB_COPYAREA) += cfbcopyarea.o obj-$(CONFIG_FB_CFB_IMAGEBLIT) += cfbimgblt.o diff --git a/drivers/video/fbdev/core/fb_internal.h b/drivers/video/fbdev/core/fb_internal.h index 79e57a5e6e7e..613832d335fe 100644 --- a/drivers/video/fbdev/core/fb_internal.h +++ b/drivers/video/fbdev/core/fb_internal.h @@ -21,10 +21,21 @@ static inline void fb_unregister_chrdev(void) #endif /* fb_logo.c */ +#if defined(CONFIG_LOGO) extern bool fb_center_logo; extern int fb_logo_count; int fb_prepare_logo(struct fb_info *fb_info, int rotate); int fb_show_logo(struct fb_info *fb_info, int rotate); +#else +static inline int fb_prepare_logo(struct fb_info *info, int rotate) +{ + return 0; +} +static inline int fb_show_logo(struct fb_info *info, int rotate) +{ + return 0; +} +#endif /* CONFIG_LOGO */ /* fbmem.c */ extern struct class *fb_class; diff --git a/drivers/video/fbdev/core/fb_logo.c b/drivers/video/fbdev/core/fb_logo.c index 76ba5a2bebae..cde0a330b2ad 100644 --- a/drivers/video/fbdev/core/fb_logo.c +++ b/drivers/video/fbdev/core/fb_logo.c @@ -7,7 +7,6 @@ bool fb_center_logo __read_mostly; int fb_logo_count __read_mostly = -1; -#ifdef CONFIG_LOGO static inline unsigned int safe_shift(unsigned int d, int n) { return n < 0 ? d >> -n : d << n; @@ -518,16 +517,3 @@ int fb_show_logo(struct fb_info *info, int rotate) return y; } EXPORT_SYMBOL(fb_show_logo); -#else -int fb_prepare_logo(struct fb_info *info, int rotate) -{ - return 0; -} -EXPORT_SYMBOL(fb_prepare_logo); - -int fb_show_logo(struct fb_info *info, int rotate) -{ - return 0; -} -EXPORT_SYMBOL(fb_show_logo); -#endif /* CONFIG_LOGO */ diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c index f157a5a1dffc..24b038510a71 100644 --- a/drivers/video/fbdev/core/fbcon.c +++ b/drivers/video/fbdev/core/fbcon.c @@ -474,15 +474,19 @@ static int __init fb_console_setup(char *this_opt) if (!strncmp(options, "logo-pos:", 9)) { options += 9; +#ifdef CONFIG_LOGO if (!strcmp(options, "center")) fb_center_logo = true; +#endif continue; } if (!strncmp(options, "logo-count:", 11)) { options += 11; +#ifdef CONFIG_LOGO if (*options) fb_logo_count = simple_strtol(options, &options, 0); +#endif continue; } }
Only build fb_logo.c if CONFIG_LOGO has been selected. Otherwise provide empty implementations of the contained interfaces and avoid using the exported variables. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/video/fbdev/core/Makefile | 3 ++- drivers/video/fbdev/core/fb_internal.h | 11 +++++++++++ drivers/video/fbdev/core/fb_logo.c | 14 -------------- drivers/video/fbdev/core/fbcon.c | 4 ++++ 4 files changed, 17 insertions(+), 15 deletions(-)