diff mbox series

[7/8] fbdev: rework backlight dependencies

Message ID 20200417155553.675905-8-arnd@arndb.de (mailing list archive)
State Awaiting Upstream
Delegated to: Geert Uytterhoeven
Headers show
Series drm, fbdev: rework dependencies | expand

Commit Message

Arnd Bergmann April 17, 2020, 3:55 p.m. UTC 
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(-)

Comments

Sam Ravnborg April 17, 2020, 5:04 p.m. UTC | #1 
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
Arnd Bergmann April 17, 2020, 7:55 p.m. UTC | #2 
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
Jani Nikula April 20, 2020, 8:02 a.m. UTC | #3 
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 mbox series

Patch

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