| Message ID | 20200417155553.675905-8-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm, fbdev: rework dependencies | expand |
Hi Arnd. On Fri, Apr 17, 2020 at 05:55:52PM +0200, Arnd Bergmann wrote: > Rather than having CONFIG_FB_BACKLIGHT select CONFIG_BACKLIGHT_CLASS_DEVICE, > make any driver that needs it have a dependency on the class device > being available, to prevent circular dependencies. > > This is the same way that the backlight is already treated for the DRM > subsystem. I am not happy with the direction of this patch. It is not easy to understand that one has to enable backlight to be allowed to select a display or an fbdev driver. How about somthing like this: config BACKLIGHT_CLASS_DEVICE tristate # Will enable backlight module # Has no dependencies config BACKLIGHT bool "Backlight drivers" # Will make the backlight drivers visible - a visibility option # only if BACKLIGHT config BACKLIGHT_ATMEL_LCDC ... config BACKLIGHT_EP93XX endif All drivers outside video/backlight/ can then select BACKLIGHT_CLASS_DEVICE to get the backlight core module. Or in other words BACKLIGHT_CLASS_DEVICE is used as a sort of library symbol that gices us access to backlight functionality. I tried something like this some time ago - but it did not work out for me. But maybe I just missed something obviously in the maze of Kconfig. Loking at your patch it is obviously that the approach I suggest above would require all relevant drivers to explicit add a select BACKLIGHT_CLASS_DEVICE. But that is not a bad thing IMO. Sam > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/auxdisplay/Kconfig | 1 + > drivers/macintosh/Kconfig | 1 + > drivers/staging/fbtft/Kconfig | 1 + > drivers/staging/olpc_dcon/Kconfig | 2 +- > drivers/video/fbdev/Kconfig | 14 +++++++++++--- > 5 files changed, 15 insertions(+), 4 deletions(-) > > diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig > index 48efa7a047f3..f5751b5b0e88 100644 > --- a/drivers/auxdisplay/Kconfig > +++ b/drivers/auxdisplay/Kconfig > @@ -144,6 +144,7 @@ config IMG_ASCII_LCD > config HT16K33 > tristate "Holtek Ht16K33 LED controller with keyscan" > depends on FB && OF && I2C && INPUT > + depends on BACKLIGHT_CLASS_DEVICE > select FB_SYS_FOPS > select FB_SYS_FILLRECT > select FB_SYS_COPYAREA > diff --git a/drivers/macintosh/Kconfig b/drivers/macintosh/Kconfig > index cbd46c1c5bf7..a1c6677c7043 100644 > --- a/drivers/macintosh/Kconfig > +++ b/drivers/macintosh/Kconfig > @@ -113,6 +113,7 @@ config PMAC_MEDIABAY > config PMAC_BACKLIGHT > bool "Backlight control for LCD screens" > depends on PPC_PMAC && ADB_PMU && FB = y && (BROKEN || !PPC64) > + depends on BACKLIGHT_CLASS_DEVICE > select FB_BACKLIGHT > help > Say Y here to enable Macintosh specific extensions of the generic > diff --git a/drivers/staging/fbtft/Kconfig b/drivers/staging/fbtft/Kconfig > index dad1ddcd7b0c..c4f2f01cd798 100644 > --- a/drivers/staging/fbtft/Kconfig > +++ b/drivers/staging/fbtft/Kconfig > @@ -3,6 +3,7 @@ menuconfig FB_TFT > tristate "Support for small TFT LCD display modules" > depends on FB && SPI > depends on GPIOLIB || COMPILE_TEST > + depends on BACKLIGHT_CLASS_DEVICE > select FB_SYS_FILLRECT > select FB_SYS_COPYAREA > select FB_SYS_IMAGEBLIT > diff --git a/drivers/staging/olpc_dcon/Kconfig b/drivers/staging/olpc_dcon/Kconfig > index d1a0dea09ef0..a9f36538d7ab 100644 > --- a/drivers/staging/olpc_dcon/Kconfig > +++ b/drivers/staging/olpc_dcon/Kconfig > @@ -4,7 +4,7 @@ config FB_OLPC_DCON > depends on OLPC && FB > depends on I2C > depends on GPIO_CS5535 && ACPI > - select BACKLIGHT_CLASS_DEVICE > + depends on BACKLIGHT_CLASS_DEVICE > help > In order to support very low power operation, the XO laptop uses a > secondary Display CONtroller, or DCON. This secondary controller > diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig > index bcf7834dbdbf..47e1b65276f4 100644 > --- a/drivers/video/fbdev/Kconfig > +++ b/drivers/video/fbdev/Kconfig > @@ -186,7 +186,7 @@ config FB_MACMODES > config FB_BACKLIGHT > tristate > depends on FB > - select BACKLIGHT_CLASS_DEVICE > + depends on BACKLIGHT_CLASS_DEVICE > > config FB_MODE_HELPERS > bool "Enable Video Mode Handling Helpers" > @@ -275,12 +275,12 @@ config FB_ARMCLCD > tristate "ARM PrimeCell PL110 support" > depends on ARM || ARM64 || COMPILE_TEST > depends on FB && ARM_AMBA && HAS_IOMEM > + depends on BACKLIGHT_CLASS_DEVICE || !OF > select FB_CFB_FILLRECT > select FB_CFB_COPYAREA > select FB_CFB_IMAGEBLIT > select FB_MODE_HELPERS if OF > select VIDEOMODE_HELPERS if OF > - select BACKLIGHT_CLASS_DEVICE if OF > help > This framebuffer device driver is for the ARM PrimeCell PL110 > Colour LCD controller. ARM PrimeCells provide the building > @@ -861,6 +861,7 @@ config FB_ATMEL > tristate "AT91 LCD Controller support" > depends on FB && OF && HAVE_CLK && HAS_IOMEM > depends on HAVE_FB_ATMEL || COMPILE_TEST > + depends on BACKLIGHT_CLASS_DEVICE > select FB_BACKLIGHT > select FB_CFB_FILLRECT > select FB_CFB_COPYAREA > @@ -914,6 +915,7 @@ config FB_NVIDIA_DEBUG > config FB_NVIDIA_BACKLIGHT > bool "Support for backlight control" > depends on FB_NVIDIA > + depends on BACKLIGHT_CLASS_DEVICE=y || BACKLIGHT_CLASS_DEVICE=FB_NVIDIA > default y > help > Say Y here if you want to control the backlight of your display. > @@ -961,6 +963,7 @@ config FB_RIVA_DEBUG > config FB_RIVA_BACKLIGHT > bool "Support for backlight control" > depends on FB_RIVA > + depends on BACKLIGHT_CLASS_DEVICE=y || BACKLIGHT_CLASS_DEVICE=FB_RIVA > default y > help > Say Y here if you want to control the backlight of your display. > @@ -1232,6 +1235,7 @@ config FB_RADEON_I2C > config FB_RADEON_BACKLIGHT > bool "Support for backlight control" > depends on FB_RADEON > + depends on BACKLIGHT_CLASS_DEVICE=y || BACKLIGHT_CLASS_DEVICE=FB_RADEON > default y > help > Say Y here if you want to control the backlight of your display. > @@ -1263,6 +1267,7 @@ config FB_ATY128 > config FB_ATY128_BACKLIGHT > bool "Support for backlight control" > depends on FB_ATY128 > + depends on BACKLIGHT_CLASS_DEVICE=y || BACKLIGHT_CLASS_DEVICE=FB_ATY128 > default y > help > Say Y here if you want to control the backlight of your display. > @@ -1312,6 +1317,7 @@ config FB_ATY_GX > > config FB_ATY_BACKLIGHT > bool "Support for backlight control" > + depends on BACKLIGHT_CLASS_DEVICE=y || BACKLIGHT_CLASS_DEVICE=FB_ATY > depends on FB_ATY > default y > help > @@ -1855,6 +1861,7 @@ config FB_SH_MOBILE_LCDC > tristate "SuperH Mobile LCDC framebuffer support" > depends on FB && HAVE_CLK && HAS_IOMEM > depends on SUPERH || ARCH_RENESAS || COMPILE_TEST > + depends on BACKLIGHT_CLASS_DEVICE > select FB_SYS_FILLRECT > select FB_SYS_COPYAREA > select FB_SYS_IMAGEBLIT > @@ -2183,7 +2190,7 @@ config FB_PRE_INIT_FB > config FB_MX3 > tristate "MX3 Framebuffer support" > depends on FB && MX3_IPU > - select BACKLIGHT_CLASS_DEVICE > + depends on BACKLIGHT_CLASS_DEVICE > select FB_CFB_FILLRECT > select FB_CFB_COPYAREA > select FB_CFB_IMAGEBLIT > @@ -2250,6 +2257,7 @@ config FB_SSD1307 > depends on FB && I2C > depends on OF > depends on GPIOLIB || COMPILE_TEST > + depends on BACKLIGHT_CLASS_DEVICE > select FB_SYS_FOPS > select FB_SYS_FILLRECT > select FB_SYS_COPYAREA > -- > 2.26.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Apr 17, 2020 at 7:04 PM Sam Ravnborg <sam@ravnborg.org> wrote: > > Hi Arnd. > > On Fri, Apr 17, 2020 at 05:55:52PM +0200, Arnd Bergmann wrote: > > Rather than having CONFIG_FB_BACKLIGHT select CONFIG_BACKLIGHT_CLASS_DEVICE, > > make any driver that needs it have a dependency on the class device > > being available, to prevent circular dependencies. > > > > This is the same way that the backlight is already treated for the DRM > > subsystem. > > I am not happy with the direction of this patch. > It is not easy to understand that one has to enable backlight to > be allowed to select a display or an fbdev driver. > > > How about somthing like this: > > config BACKLIGHT_CLASS_DEVICE > tristate > # Will enable backlight module > # Has no dependencies > > config BACKLIGHT > bool "Backlight drivers" > # Will make the backlight drivers visible - a visibility option > # only > > All drivers outside video/backlight/ can then select > BACKLIGHT_CLASS_DEVICE to get the backlight core > module. > Or in other words BACKLIGHT_CLASS_DEVICE is used as a sort > of library symbol that gices us access to backlight functionality. Right, this could work as long as nobody puts back any "depends on BACKLIGHT_CLASS_DEVICE, but it does cause another problem: There are a couple of drivers that can optionally use backlight support or just leave it out depending on CONFIG_BACKLIGHT_CLASS_DEVICE. Changing them to select the class device support unconditionally would make it impossible to build those drivers without it. Instead we could all need an individual Kconfig symbol, or use have "select BACKLIGHT_CLASS_DEVICE if BACKLIGHT" in each case, but I'm not sure if that's still a win over having a simple 'depends on BACKLIGHT_CLASS_DEVICE'. On a related note, the NEW_LEDS/LEDS_CLASS option probably falls into the same category, and could also be done as you suggest. At the moment, this has the problem that both symbols are user visible and also frequently selected by device drivers (but not others), which is causing another set of dependency issues. Arnd
On Fri, 17 Apr 2020, Sam Ravnborg <sam@ravnborg.org> wrote: > Hi Arnd. > > On Fri, Apr 17, 2020 at 05:55:52PM +0200, Arnd Bergmann wrote: >> Rather than having CONFIG_FB_BACKLIGHT select CONFIG_BACKLIGHT_CLASS_DEVICE, >> make any driver that needs it have a dependency on the class device >> being available, to prevent circular dependencies. >> >> This is the same way that the backlight is already treated for the DRM >> subsystem. > > I am not happy with the direction of this patch. > It is not easy to understand that one has to enable backlight to > be allowed to select a display or an fbdev driver. Arguably that is a problem in Kconfig, and that applies to *all* dependencies everywhere. It isn't something new to this patch. For example, in the context of this patch you have: config HT16K33 tristate "Holtek Ht16K33 LED controller with keyscan" depends on FB && OF && I2C && INPUT + depends on BACKLIGHT_CLASS_DEVICE The same thing could be said about FB and OF and IC2 and INPUT for HT16K33, right? Why would *backlight* be the tipping point that makes this difficult to understand? Yeah, I agree it's not straightforward, but I think depends is the right choice, not select. The effort for making this easier to understand should be directed at the various menuconfig tools to better highlight the dependencies that should be enabled to let you enable other options. > How about somthing like this: I think this is a hack in Kconfig files that should really be fixed in the Kconfig tooling instead. IMHO Kconfig should be as simple a description of the dependencies as possible, not so much a UI language. FWIW the patch is Acked-by: Jani Nikula <jani.nikula@intel.com> BR, Jani.
diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig index 48efa7a047f3..f5751b5b0e88 100644 --- a/drivers/auxdisplay/Kconfig +++ b/drivers/auxdisplay/Kconfig @@ -144,6 +144,7 @@ config IMG_ASCII_LCD config HT16K33 tristate "Holtek Ht16K33 LED controller with keyscan" depends on FB && OF && I2C && INPUT + depends on BACKLIGHT_CLASS_DEVICE select FB_SYS_FOPS select FB_SYS_FILLRECT select FB_SYS_COPYAREA diff --git a/drivers/macintosh/Kconfig b/drivers/macintosh/Kconfig index cbd46c1c5bf7..a1c6677c7043 100644 --- a/drivers/macintosh/Kconfig +++ b/drivers/macintosh/Kconfig @@ -113,6 +113,7 @@ config PMAC_MEDIABAY config PMAC_BACKLIGHT bool "Backlight control for LCD screens" depends on PPC_PMAC && ADB_PMU && FB = y && (BROKEN || !PPC64) + depends on BACKLIGHT_CLASS_DEVICE select FB_BACKLIGHT help Say Y here to enable Macintosh specific extensions of the generic diff --git a/drivers/staging/fbtft/Kconfig b/drivers/staging/fbtft/Kconfig index dad1ddcd7b0c..c4f2f01cd798 100644 --- a/drivers/staging/fbtft/Kconfig +++ b/drivers/staging/fbtft/Kconfig @@ -3,6 +3,7 @@ menuconfig FB_TFT tristate "Support for small TFT LCD display modules" depends on FB && SPI depends on GPIOLIB || COMPILE_TEST + depends on BACKLIGHT_CLASS_DEVICE select FB_SYS_FILLRECT select FB_SYS_COPYAREA select FB_SYS_IMAGEBLIT diff --git a/drivers/staging/olpc_dcon/Kconfig b/drivers/staging/olpc_dcon/Kconfig index d1a0dea09ef0..a9f36538d7ab 100644 --- a/drivers/staging/olpc_dcon/Kconfig +++ b/drivers/staging/olpc_dcon/Kconfig @@ -4,7 +4,7 @@ config FB_OLPC_DCON depends on OLPC && FB depends on I2C depends on GPIO_CS5535 && ACPI - select BACKLIGHT_CLASS_DEVICE + depends on BACKLIGHT_CLASS_DEVICE help In order to support very low power operation, the XO laptop uses a secondary Display CONtroller, or DCON. This secondary controller diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig index bcf7834dbdbf..47e1b65276f4 100644 --- a/drivers/video/fbdev/Kconfig +++ b/drivers/video/fbdev/Kconfig @@ -186,7 +186,7 @@ config FB_MACMODES config FB_BACKLIGHT tristate depends on FB - select BACKLIGHT_CLASS_DEVICE + depends on BACKLIGHT_CLASS_DEVICE config FB_MODE_HELPERS bool "Enable Video Mode Handling Helpers" @@ -275,12 +275,12 @@ config FB_ARMCLCD tristate "ARM PrimeCell PL110 support" depends on ARM || ARM64 || COMPILE_TEST depends on FB && ARM_AMBA && HAS_IOMEM + depends on BACKLIGHT_CLASS_DEVICE || !OF select FB_CFB_FILLRECT select FB_CFB_COPYAREA select FB_CFB_IMAGEBLIT select FB_MODE_HELPERS if OF select VIDEOMODE_HELPERS if OF - select BACKLIGHT_CLASS_DEVICE if OF help This framebuffer device driver is for the ARM PrimeCell PL110 Colour LCD controller. ARM PrimeCells provide the building @@ -861,6 +861,7 @@ config FB_ATMEL tristate "AT91 LCD Controller support" depends on FB && OF && HAVE_CLK && HAS_IOMEM depends on HAVE_FB_ATMEL || COMPILE_TEST + depends on BACKLIGHT_CLASS_DEVICE select FB_BACKLIGHT select FB_CFB_FILLRECT select FB_CFB_COPYAREA @@ -914,6 +915,7 @@ config FB_NVIDIA_DEBUG config FB_NVIDIA_BACKLIGHT bool "Support for backlight control" depends on FB_NVIDIA + depends on BACKLIGHT_CLASS_DEVICE=y || BACKLIGHT_CLASS_DEVICE=FB_NVIDIA default y help Say Y here if you want to control the backlight of your display. @@ -961,6 +963,7 @@ config FB_RIVA_DEBUG config FB_RIVA_BACKLIGHT bool "Support for backlight control" depends on FB_RIVA + depends on BACKLIGHT_CLASS_DEVICE=y || BACKLIGHT_CLASS_DEVICE=FB_RIVA default y help Say Y here if you want to control the backlight of your display. @@ -1232,6 +1235,7 @@ config FB_RADEON_I2C config FB_RADEON_BACKLIGHT bool "Support for backlight control" depends on FB_RADEON + depends on BACKLIGHT_CLASS_DEVICE=y || BACKLIGHT_CLASS_DEVICE=FB_RADEON default y help Say Y here if you want to control the backlight of your display. @@ -1263,6 +1267,7 @@ config FB_ATY128 config FB_ATY128_BACKLIGHT bool "Support for backlight control" depends on FB_ATY128 + depends on BACKLIGHT_CLASS_DEVICE=y || BACKLIGHT_CLASS_DEVICE=FB_ATY128 default y help Say Y here if you want to control the backlight of your display. @@ -1312,6 +1317,7 @@ config FB_ATY_GX config FB_ATY_BACKLIGHT bool "Support for backlight control" + depends on BACKLIGHT_CLASS_DEVICE=y || BACKLIGHT_CLASS_DEVICE=FB_ATY depends on FB_ATY default y help @@ -1855,6 +1861,7 @@ config FB_SH_MOBILE_LCDC tristate "SuperH Mobile LCDC framebuffer support" depends on FB && HAVE_CLK && HAS_IOMEM depends on SUPERH || ARCH_RENESAS || COMPILE_TEST + depends on BACKLIGHT_CLASS_DEVICE select FB_SYS_FILLRECT select FB_SYS_COPYAREA select FB_SYS_IMAGEBLIT @@ -2183,7 +2190,7 @@ config FB_PRE_INIT_FB config FB_MX3 tristate "MX3 Framebuffer support" depends on FB && MX3_IPU - select BACKLIGHT_CLASS_DEVICE + depends on BACKLIGHT_CLASS_DEVICE select FB_CFB_FILLRECT select FB_CFB_COPYAREA select FB_CFB_IMAGEBLIT @@ -2250,6 +2257,7 @@ config FB_SSD1307 depends on FB && I2C depends on OF depends on GPIOLIB || COMPILE_TEST + depends on BACKLIGHT_CLASS_DEVICE select FB_SYS_FOPS select FB_SYS_FILLRECT select FB_SYS_COPYAREA
Rather than having CONFIG_FB_BACKLIGHT select CONFIG_BACKLIGHT_CLASS_DEVICE, make any driver that needs it have a dependency on the class device being available, to prevent circular dependencies. This is the same way that the backlight is already treated for the DRM subsystem. Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/auxdisplay/Kconfig | 1 + drivers/macintosh/Kconfig | 1 + drivers/staging/fbtft/Kconfig | 1 + drivers/staging/olpc_dcon/Kconfig | 2 +- drivers/video/fbdev/Kconfig | 14 +++++++++++--- 5 files changed, 15 insertions(+), 4 deletions(-)