| Message ID | 5ab3d1fe7b67ab10e4bc1bdbc0fa7731f7960965.1690300189.git.geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | video: logo: LOGO should depend on FB_CORE i.s.o. FB | expand |
Geert Uytterhoeven <geert+renesas@glider.be> writes: Hello Geert, Thanks a lot for your patch! > If CONFIG_FB_CORE=y but CONFIG_FB=n, the frame buffer bootup logos can > no longer be enabled. Fix this by making CONFIG_LOGO depend on > CONFIG_FB_CORE instead of CONFIG_FB, as there is no good reason for the > logo code to depend on the presence of real frame buffer device drivers. > Indeed. > Fixes: 55bffc8170bb5813 ("fbdev: Split frame buffer support in FB and FB_CORE symbols") > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > drivers/video/Kconfig | 2 +- > drivers/video/logo/Kconfig | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig > index e5b1cc54cafa10d5..b694d7669d3200b1 100644 > --- a/drivers/video/Kconfig > +++ b/drivers/video/Kconfig > @@ -63,7 +63,7 @@ if VT > source "drivers/video/console/Kconfig" > endif > > -if FB || SGI_NEWPORT_CONSOLE > +if FB_CORE || SGI_NEWPORT_CONSOLE > source "drivers/video/logo/Kconfig" > > endif > diff --git a/drivers/video/logo/Kconfig b/drivers/video/logo/Kconfig > index 6d6f8c08792dc897..b7d94d1dd1585a84 100644 > --- a/drivers/video/logo/Kconfig > +++ b/drivers/video/logo/Kconfig > @@ -5,7 +5,7 @@ > > menuconfig LOGO > bool "Bootup logo" > - depends on FB || SGI_NEWPORT_CONSOLE > + depends on FB_CORE || SGI_NEWPORT_CONSOLE > help > Enable and select frame buffer bootup logos. Should then move this option to drivers/video/fbdev/core/Kconfig ? Regardless, could be done as a follow-up and the fix looks good to me. Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Hi Javier, On Tue, Jul 25, 2023 at 6:07 PM Javier Martinez Canillas <javierm@redhat.com> wrote: > Geert Uytterhoeven <geert+renesas@glider.be> writes: > > If CONFIG_FB_CORE=y but CONFIG_FB=n, the frame buffer bootup logos can > > no longer be enabled. Fix this by making CONFIG_LOGO depend on > > CONFIG_FB_CORE instead of CONFIG_FB, as there is no good reason for the > > logo code to depend on the presence of real frame buffer device drivers. > > Indeed. > > > Fixes: 55bffc8170bb5813 ("fbdev: Split frame buffer support in FB and FB_CORE symbols") > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > --- > > drivers/video/Kconfig | 2 +- > > drivers/video/logo/Kconfig | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig > > index e5b1cc54cafa10d5..b694d7669d3200b1 100644 > > --- a/drivers/video/Kconfig > > +++ b/drivers/video/Kconfig > > @@ -63,7 +63,7 @@ if VT > > source "drivers/video/console/Kconfig" > > endif > > > > -if FB || SGI_NEWPORT_CONSOLE > > +if FB_CORE || SGI_NEWPORT_CONSOLE > > source "drivers/video/logo/Kconfig" > > > > endif > > diff --git a/drivers/video/logo/Kconfig b/drivers/video/logo/Kconfig > > index 6d6f8c08792dc897..b7d94d1dd1585a84 100644 > > --- a/drivers/video/logo/Kconfig > > +++ b/drivers/video/logo/Kconfig > > @@ -5,7 +5,7 @@ > > > > menuconfig LOGO > > bool "Bootup logo" > > - depends on FB || SGI_NEWPORT_CONSOLE > > + depends on FB_CORE || SGI_NEWPORT_CONSOLE > > help > > Enable and select frame buffer bootup logos. > > Should then move this option to drivers/video/fbdev/core/Kconfig ? No, all logo options are in their own file. > Regardless, could be done as a follow-up and the fix looks good to me. > > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> Thanks! Gr{oetje,eeting}s, Geert
Geert Uytterhoeven <geert@linux-m68k.org> writes: > Hi Javier, > > On Tue, Jul 25, 2023 at 6:07 PM Javier Martinez Canillas > <javierm@redhat.com> wrote: >> Geert Uytterhoeven <geert+renesas@glider.be> writes: >> > If CONFIG_FB_CORE=y but CONFIG_FB=n, the frame buffer bootup logos can >> > no longer be enabled. Fix this by making CONFIG_LOGO depend on >> > CONFIG_FB_CORE instead of CONFIG_FB, as there is no good reason for the >> > logo code to depend on the presence of real frame buffer device drivers. >> >> Indeed. >> >> > Fixes: 55bffc8170bb5813 ("fbdev: Split frame buffer support in FB and FB_CORE symbols") >> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >> > --- >> > drivers/video/Kconfig | 2 +- >> > drivers/video/logo/Kconfig | 2 +- >> > 2 files changed, 2 insertions(+), 2 deletions(-) >> > >> > diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig >> > index e5b1cc54cafa10d5..b694d7669d3200b1 100644 >> > --- a/drivers/video/Kconfig >> > +++ b/drivers/video/Kconfig >> > @@ -63,7 +63,7 @@ if VT >> > source "drivers/video/console/Kconfig" >> > endif >> > >> > -if FB || SGI_NEWPORT_CONSOLE >> > +if FB_CORE || SGI_NEWPORT_CONSOLE >> > source "drivers/video/logo/Kconfig" >> > >> > endif >> > diff --git a/drivers/video/logo/Kconfig b/drivers/video/logo/Kconfig >> > index 6d6f8c08792dc897..b7d94d1dd1585a84 100644 >> > --- a/drivers/video/logo/Kconfig >> > +++ b/drivers/video/logo/Kconfig >> > @@ -5,7 +5,7 @@ >> > >> > menuconfig LOGO >> > bool "Bootup logo" >> > - depends on FB || SGI_NEWPORT_CONSOLE >> > + depends on FB_CORE || SGI_NEWPORT_CONSOLE >> > help >> > Enable and select frame buffer bootup logos. >> >> Should then move this option to drivers/video/fbdev/core/Kconfig ? > > No, all logo options are in their own file. > Yes. I meant to move drivers/video/logo/ to drivers/fbdev/core/logo and to source its Kconfig from drivers/fbdev/core/Kconfig, since it now depends on FB_CORE. But I see now that it also depends on SGI_NEWPORT_CONSOLE, so having those in drivers/video/logo makes sense indeed.
Hi Javier, > >> > menuconfig LOGO > >> > bool "Bootup logo" > >> > - depends on FB || SGI_NEWPORT_CONSOLE > >> > + depends on FB_CORE || SGI_NEWPORT_CONSOLE > >> > help > >> > Enable and select frame buffer bootup logos. > >> > >> Should then move this option to drivers/video/fbdev/core/Kconfig ? > > > > No, all logo options are in their own file. > > > > Yes. I meant to move drivers/video/logo/ to drivers/fbdev/core/logo and to > source its Kconfig from drivers/fbdev/core/Kconfig, since it now depends > on FB_CORE. > > But I see now that it also depends on SGI_NEWPORT_CONSOLE, so having those > in drivers/video/logo makes sense indeed. The SGI_NEWPORT_CONSOLE should be replaced by some ifdef in the newport_con.c code - to do what other drivers do. But thats for another day. Sam
Hi Am 25.07.23 um 18:50 schrieb Javier Martinez Canillas: > Geert Uytterhoeven <geert@linux-m68k.org> writes: > >> Hi Javier, >> >> On Tue, Jul 25, 2023 at 6:07 PM Javier Martinez Canillas >> <javierm@redhat.com> wrote: >>> Geert Uytterhoeven <geert+renesas@glider.be> writes: >>>> If CONFIG_FB_CORE=y but CONFIG_FB=n, the frame buffer bootup logos can >>>> no longer be enabled. Fix this by making CONFIG_LOGO depend on >>>> CONFIG_FB_CORE instead of CONFIG_FB, as there is no good reason for the >>>> logo code to depend on the presence of real frame buffer device drivers. >>> >>> Indeed. >>> >>>> Fixes: 55bffc8170bb5813 ("fbdev: Split frame buffer support in FB and FB_CORE symbols") >>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >>>> --- >>>> drivers/video/Kconfig | 2 +- >>>> drivers/video/logo/Kconfig | 2 +- >>>> 2 files changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig >>>> index e5b1cc54cafa10d5..b694d7669d3200b1 100644 >>>> --- a/drivers/video/Kconfig >>>> +++ b/drivers/video/Kconfig >>>> @@ -63,7 +63,7 @@ if VT >>>> source "drivers/video/console/Kconfig" >>>> endif >>>> >>>> -if FB || SGI_NEWPORT_CONSOLE >>>> +if FB_CORE || SGI_NEWPORT_CONSOLE >>>> source "drivers/video/logo/Kconfig" >>>> >>>> endif >>>> diff --git a/drivers/video/logo/Kconfig b/drivers/video/logo/Kconfig >>>> index 6d6f8c08792dc897..b7d94d1dd1585a84 100644 >>>> --- a/drivers/video/logo/Kconfig >>>> +++ b/drivers/video/logo/Kconfig >>>> @@ -5,7 +5,7 @@ >>>> >>>> menuconfig LOGO >>>> bool "Bootup logo" >>>> - depends on FB || SGI_NEWPORT_CONSOLE >>>> + depends on FB_CORE || SGI_NEWPORT_CONSOLE >>>> help >>>> Enable and select frame buffer bootup logos. >>> >>> Should then move this option to drivers/video/fbdev/core/Kconfig ? >> >> No, all logo options are in their own file. >> > > Yes. I meant to move drivers/video/logo/ to drivers/fbdev/core/logo and to > source its Kconfig from drivers/fbdev/core/Kconfig, since it now depends > on FB_CORE. No, please rather leave it where it is. There's no code dependencies to the fbdev core; it merely depends on the Kconfig token. Best regards Thomas > > But I see now that it also depends on SGI_NEWPORT_CONSOLE, so having those > in drivers/video/logo makes sense indeed. >
Thomas Zimmermann <tzimmermann@suse.de> writes: > Hi > [...] >> >> Yes. I meant to move drivers/video/logo/ to drivers/fbdev/core/logo and to >> source its Kconfig from drivers/fbdev/core/Kconfig, since it now depends >> on FB_CORE. > > No, please rather leave it where it is. There's no code dependencies to > the fbdev core; it merely depends on the Kconfig token. > Sure, fine by me. But I disagree that there's merely a Kconfig dependency. The include/linux/linux_logo.h header declares both fb_find_logo() and fb_append_extra_logo(). The fb_find_logo() function is defined in drivers/video/logo.c while the fb_append_extra_logo() is in drivers/video/fbdev/core/fbmem.c, even though only arch/powerpc/platforms/cell/spu_base.c uses fb_append_extra_logo(). So there's a relationship already between logo and fbdev/core, that's why I wondered if would make sense to also move drivers/video/logo.c to have both functions in there. Yes, as noted drivers/video/console/newport_con.c also uses fb_find_logo() but the only other user of that in drivers/video/fbdev/core/fbmem.c.
On Tue, Jul 25, 2023 at 09:53:16PM +0200, Javier Martinez Canillas wrote: > Thomas Zimmermann <tzimmermann@suse.de> writes: > > > Hi > > > > [...] > > >> > >> Yes. I meant to move drivers/video/logo/ to drivers/fbdev/core/logo and to > >> source its Kconfig from drivers/fbdev/core/Kconfig, since it now depends > >> on FB_CORE. > > > > No, please rather leave it where it is. There's no code dependencies to > > the fbdev core; it merely depends on the Kconfig token. > > > > Sure, fine by me. But I disagree that there's merely a Kconfig dependency. > The include/linux/linux_logo.h header declares both fb_find_logo() and > fb_append_extra_logo(). > > The fb_find_logo() function is defined in drivers/video/logo.c while the > fb_append_extra_logo() is in drivers/video/fbdev/core/fbmem.c, even though > only arch/powerpc/platforms/cell/spu_base.c uses fb_append_extra_logo(). > > So there's a relationship already between logo and fbdev/core, that's why > I wondered if would make sense to also move drivers/video/logo.c to have > both functions in there. Or as I also suggested on irc - to pull out all the logo stuff from fbmem and put it in video/logo/ With a bit of refactoring to make it obvious this is logo stuff and maybe avoid some of the ifdeffery in the code of the users. Sam
Sam Ravnborg <sam@ravnborg.org> writes: > On Tue, Jul 25, 2023 at 09:53:16PM +0200, Javier Martinez Canillas wrote: >> Thomas Zimmermann <tzimmermann@suse.de> writes: >> >> > Hi >> > >> >> [...] >> >> >> >> >> Yes. I meant to move drivers/video/logo/ to drivers/fbdev/core/logo and to >> >> source its Kconfig from drivers/fbdev/core/Kconfig, since it now depends >> >> on FB_CORE. >> > >> > No, please rather leave it where it is. There's no code dependencies to >> > the fbdev core; it merely depends on the Kconfig token. >> > >> >> Sure, fine by me. But I disagree that there's merely a Kconfig dependency. >> The include/linux/linux_logo.h header declares both fb_find_logo() and >> fb_append_extra_logo(). >> >> The fb_find_logo() function is defined in drivers/video/logo.c while the >> fb_append_extra_logo() is in drivers/video/fbdev/core/fbmem.c, even though >> only arch/powerpc/platforms/cell/spu_base.c uses fb_append_extra_logo(). >> >> So there's a relationship already between logo and fbdev/core, that's why >> I wondered if would make sense to also move drivers/video/logo.c to have >> both functions in there. > Or as I also suggested on irc - to pull out all the logo stuff from > fbmem and put it in video/logo/ > With a bit of refactoring to make it obvious this is logo stuff and > maybe avoid some of the ifdeffery in the code of the users. > Agreed. That option may be better. > Sam >
Hi Javier Am 25.07.23 um 21:53 schrieb Javier Martinez Canillas: > Thomas Zimmermann <tzimmermann@suse.de> writes: > >> Hi >> > > [...] > >>> >>> Yes. I meant to move drivers/video/logo/ to drivers/fbdev/core/logo and to >>> source its Kconfig from drivers/fbdev/core/Kconfig, since it now depends >>> on FB_CORE. >> >> No, please rather leave it where it is. There's no code dependencies to >> the fbdev core; it merely depends on the Kconfig token. >> > > Sure, fine by me. But I disagree that there's merely a Kconfig dependency. > The include/linux/linux_logo.h header declares both fb_find_logo() and > fb_append_extra_logo(). > > The fb_find_logo() function is defined in drivers/video/logo.c while the > fb_append_extra_logo() is in drivers/video/fbdev/core/fbmem.c, even though > only arch/powerpc/platforms/cell/spu_base.c uses fb_append_extra_logo(). > > So there's a relationship already between logo and fbdev/core, that's why > I wondered if would make sense to also move drivers/video/logo.c to have > both functions in there. Fair enough. I was looking for references to struct fb_info in the logo code and found none. Sam's suggestion to move the remaining code from fbdev to logo/ might be the way to go. If we ever get that DRM boot-up client, it might want to use the logo as well. Hence, it needs to be unrelated to fbdev. Best regards Thomas > > Yes, as noted drivers/video/console/newport_con.c also uses fb_find_logo() > but the only other user of that in drivers/video/fbdev/core/fbmem.c. >
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig index e5b1cc54cafa10d5..b694d7669d3200b1 100644 --- a/drivers/video/Kconfig +++ b/drivers/video/Kconfig @@ -63,7 +63,7 @@ if VT source "drivers/video/console/Kconfig" endif -if FB || SGI_NEWPORT_CONSOLE +if FB_CORE || SGI_NEWPORT_CONSOLE source "drivers/video/logo/Kconfig" endif diff --git a/drivers/video/logo/Kconfig b/drivers/video/logo/Kconfig index 6d6f8c08792dc897..b7d94d1dd1585a84 100644 --- a/drivers/video/logo/Kconfig +++ b/drivers/video/logo/Kconfig @@ -5,7 +5,7 @@ menuconfig LOGO bool "Bootup logo" - depends on FB || SGI_NEWPORT_CONSOLE + depends on FB_CORE || SGI_NEWPORT_CONSOLE help Enable and select frame buffer bootup logos.
If CONFIG_FB_CORE=y but CONFIG_FB=n, the frame buffer bootup logos can no longer be enabled. Fix this by making CONFIG_LOGO depend on CONFIG_FB_CORE instead of CONFIG_FB, as there is no good reason for the logo code to depend on the presence of real frame buffer device drivers. Fixes: 55bffc8170bb5813 ("fbdev: Split frame buffer support in FB and FB_CORE symbols") Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- drivers/video/Kconfig | 2 +- drivers/video/logo/Kconfig | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)