Re: [PATCH 1/2] fbdev: Split frame buffer support in FB and FB_CORE symbols

["Arnd Bergmann" <arnd@arndb.de>]

On Fri, Jun 30, 2023, at 00:51, Javier Martinez Canillas wrote:
> Currently the CONFIG_FB option has to be enabled even if no legacy fbdev
> drivers are needed (e.g: only to have support for framebuffer consoles).
>
> The DRM subsystem has a fbdev emulation layer, but depends on CONFIG_FB
> and so it can only be enabled if that dependency is enabled as well.
>
> That means fbdev drivers have to be explicitly disabled if users want to
> enable CONFIG_FB, only to use fbcon and/or the DRM fbdev emulation layer.
>
> This patch introduces a CONFIG_FB_CORE option that could be enabled just
> to have the core support needed for CONFIG_DRM_FBDEV_EMULATION, allowing
> CONFIG_FB to be disabled (and automatically disabling all fbdev drivers).
>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---

This looks really nice!

I tried to do something like this a few years ago, but failed as
I did too much at once by attempting to cut out msot of the fb core
support that is not actually used by DRM at the same time.

Doing just the Kconfig bits as you do here is probably better
anyway, cutting out unneeded bits into separate modules or #ifdef
sections can come later.

> @@ -59,7 +71,7 @@ config FIRMWARE_EDID
> 
>  config FB_DEVICE
>  	bool "Provide legacy /dev/fb* device"
> -	depends on FB
> +	depends on FB_CORE
>  	default y
>  	help
>  	  Say Y here if you want the legacy /dev/fb* device file and

I don't see this symbol in linux-next yet, what tree are you using
as a base?

> @@ -69,13 +81,13 @@ config FB_DEVICE
> 
>  config FB_DDC
>  	tristate
> -	depends on FB
> +	depends on FB_CORE
>  	select I2C_ALGOBIT
>  	select I2C

This seems to only be used by actual fbdev drivers, so maybe
don't change the dependency here.

> @@ -162,22 +174,22 @@ endchoice
> 
>  config FB_SYS_FOPS
>  	tristate
> -	depends on FB
> +	depends on FB_CORE

Same for this one

>  config FB_HECUBA
>  	tristate
> -	depends on FB
> +	depends on FB_CORE
>  	depends on FB_DEFERRED_IO
> 
>  config FB_SVGALIB
>  	tristate
> -	depends on FB
> +	depends on FB_CORE
>  	help
>  	  Common utility functions useful to fbdev drivers of VGA-based
>  	  cards.
>  config FB_MACMODES
>  	tristate
> -	depends on FB
> +	depends on FB_CORE
> 

These three seem to actually be part of fbdev drivers rather
than the core, and it may be best to move them into
drviers/video/fbdev/ as standalone modules. That would be a
separate patch of course.
 
>  config FB_BACKLIGHT
>  	tristate
> -	depends on FB
> +	depends on FB_CORE
>  	select BACKLIGHT_CLASS_DEVICE

Separating this one from FB_CORE would help avoid circular dependencies,
this one keeps causing issues.

> @@ -1,22 +1,22 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_FB_NOTIFY)           += fb_notify.o
> -obj-$(CONFIG_FB)                  += fb.o
> -fb-y                              := fb_backlight.o \
> +obj-$(CONFIG_FB_CORE)             += fb_core.o
> +fb_core-y                         := fb_backlight.o \
>                                       fb_info.o \
>                                       fbmem.o fbmon.o fbcmap.o \
>                                       modedb.o fbcvt.o fb_cmdline.o 

I would not bother renaming the module itself here, that
might cause problems if anything relies on loading the
module by name or a named module parameter.

      Arnd