[PATCH 0/3] drm,fbdev: Fix module dependencies

[PATCH 0/3] drm,fbdev: Fix module dependencies

[Thomas Zimmermann]

Fix the dependencies among the various graphics modules.

Before addressing the FB_CORE issue, patch 1 first resolves a problem
with BACKLIGHT_CLASS_DEVICE. A number of fbdev drivers select it, which
results in a recursive-dependency error after patch has been applied.
Making these drivers (or parts of them) depend on BACKLIGHT_CLASS_DEVICE
fixes this.

Patch 2 selects FB_CORE for DRM_GEM_DMA_HELPER and DRM_TTM_HELPER.
This is necessary with the recently added DRM client library.

Patch 3 is the second half of the patch provided by Arnd at [1]. It
could not yet be merged because of the issues fixed by patch 1.

Side note: For the majority of graphics drivers, backlight functionality
depends on BACKLIGHT_CLASS_DEVICE. In a few cases drivers select the
Kconfig token automatically. These drivers should be updated to depends
on the token as well, such that backlight functionality is fully user-
controlled.

[1] https://patchwork.freedesktop.org/series/141411/

Arnd Bergmann (1):
  drm: rework FB_CORE dependency

Thomas Zimmermann (2):
  fbdev: Fix recursive dependencies wrt BACKLIGHT_CLASS_DEVICE
  drm/fbdev: Select FB_CORE dependency for fbdev on DMA and TTM

 drivers/auxdisplay/Kconfig       |  2 +-
 drivers/gpu/drm/Kconfig          |  3 +++
 drivers/macintosh/Kconfig        |  1 +
 drivers/staging/fbtft/Kconfig    |  1 +
 drivers/video/fbdev/Kconfig      | 18 +++++++++++++-----
 drivers/video/fbdev/core/Kconfig |  3 +--
 6 files changed, 20 insertions(+), 8 deletions(-)

-- 
2.47.1

[PATCH 1/3] fbdev: Fix recursive dependencies wrt BACKLIGHT_CLASS_DEVICE

[Thomas Zimmermann]

Do not select BACKLIGHT_CLASS_DEVICE from FB_BACKLIGHT. The latter
only controls backlight support within fbdev core code and data
structures.

Make fbdev drivers depend on BACKLIGHT_CLASS_DEVICE and let users
select it explicitly. Fixes warnings about recursive dependencies,
such as

error: recursive dependency detected!
	symbol BACKLIGHT_CLASS_DEVICE is selected by FB_BACKLIGHT
	symbol FB_BACKLIGHT is selected by FB_SH_MOBILE_LCDC
	symbol FB_SH_MOBILE_LCDC depends on FB_DEVICE
	symbol FB_DEVICE depends on FB_CORE
	symbol FB_CORE is selected by DRM_GEM_DMA_HELPER
	symbol DRM_GEM_DMA_HELPER is selected by DRM_PANEL_ILITEK_ILI9341
	symbol DRM_PANEL_ILITEK_ILI9341 depends on BACKLIGHT_CLASS_DEVICE

BACKLIGHT_CLASS_DEVICE is user-selectable, so making drivers adapt to
it is the correct approach in any case. For most drivers, backlight
support is also configurable separately.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/auxdisplay/Kconfig       |  2 +-
 drivers/macintosh/Kconfig        |  1 +
 drivers/staging/fbtft/Kconfig    |  1 +
 drivers/video/fbdev/Kconfig      | 18 +++++++++++++-----
 drivers/video/fbdev/core/Kconfig |  3 +--
 5 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
index 21545ffba065..8934e6ad5772 100644
--- a/drivers/auxdisplay/Kconfig
+++ b/drivers/auxdisplay/Kconfig
@@ -489,7 +489,7 @@ config IMG_ASCII_LCD
 
 config HT16K33
 	tristate "Holtek Ht16K33 LED controller with keyscan"
-	depends on FB && I2C && INPUT
+	depends on FB && I2C && INPUT && BACKLIGHT_CLASS_DEVICE
 	select FB_SYSMEM_HELPERS
 	select INPUT_MATRIXKMAP
 	select FB_BACKLIGHT
diff --git a/drivers/macintosh/Kconfig b/drivers/macintosh/Kconfig
index fb38f684444f..bf3824032d61 100644
--- a/drivers/macintosh/Kconfig
+++ b/drivers/macintosh/Kconfig
@@ -120,6 +120,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 77ab44362f16..577e91ff7bf6 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 FB_DEVICE
+	depends on BACKLIGHT_DEVICE_CLASS
 	depends on GPIOLIB || COMPILE_TEST
 	select FB_BACKLIGHT
 	select FB_SYSMEM_HELPERS_DEFERRED
diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
index de035071fedb..7f2f08748a27 100644
--- a/drivers/video/fbdev/Kconfig
+++ b/drivers/video/fbdev/Kconfig
@@ -649,6 +649,7 @@ config FB_S1D13XXX
 config FB_ATMEL
 	tristate "AT91 LCD Controller support"
 	depends on FB && OF && HAVE_CLK && HAS_IOMEM
+	depends on BACKLIGHT_CLASS_DEVICE
 	depends on HAVE_FB_ATMEL || COMPILE_TEST
 	select FB_BACKLIGHT
 	select FB_IOMEM_HELPERS
@@ -660,7 +661,6 @@ config FB_ATMEL
 config FB_NVIDIA
 	tristate "nVidia Framebuffer Support"
 	depends on FB && PCI
-	select FB_BACKLIGHT if FB_NVIDIA_BACKLIGHT
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
 	select FB_CFB_IMAGEBLIT
@@ -700,6 +700,8 @@ config FB_NVIDIA_DEBUG
 config FB_NVIDIA_BACKLIGHT
 	bool "Support for backlight control"
 	depends on FB_NVIDIA
+	depends on BACKLIGHT_CLASS_DEVICE
+	select FB_BACKLIGHT
 	default y
 	help
 	  Say Y here if you want to control the backlight of your display.
@@ -707,7 +709,6 @@ config FB_NVIDIA_BACKLIGHT
 config FB_RIVA
 	tristate "nVidia Riva support"
 	depends on FB && PCI
-	select FB_BACKLIGHT if FB_RIVA_BACKLIGHT
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
 	select FB_CFB_IMAGEBLIT
@@ -747,6 +748,8 @@ config FB_RIVA_DEBUG
 config FB_RIVA_BACKLIGHT
 	bool "Support for backlight control"
 	depends on FB_RIVA
+	depends on BACKLIGHT_CLASS_DEVICE
+	select FB_BACKLIGHT
 	default y
 	help
 	  Say Y here if you want to control the backlight of your display.
@@ -934,7 +937,6 @@ config FB_MATROX_MAVEN
 config FB_RADEON
 	tristate "ATI Radeon display support"
 	depends on FB && PCI
-	select FB_BACKLIGHT if FB_RADEON_BACKLIGHT
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
 	select FB_CFB_IMAGEBLIT
@@ -960,6 +962,8 @@ config FB_RADEON_I2C
 config FB_RADEON_BACKLIGHT
 	bool "Support for backlight control"
 	depends on FB_RADEON
+	depends on BACKLIGHT_CLASS_DEVICE
+	select FB_BACKLIGHT
 	default y
 	help
 	  Say Y here if you want to control the backlight of your display.
@@ -975,7 +979,6 @@ config FB_RADEON_DEBUG
 config FB_ATY128
 	tristate "ATI Rage128 display support"
 	depends on FB && PCI
-	select FB_BACKLIGHT if FB_ATY128_BACKLIGHT
 	select FB_IOMEM_HELPERS
 	select FB_MACMODES if PPC_PMAC
 	help
@@ -989,6 +992,8 @@ config FB_ATY128
 config FB_ATY128_BACKLIGHT
 	bool "Support for backlight control"
 	depends on FB_ATY128
+	depends on BACKLIGHT_CLASS_DEVICE
+	select FB_BACKLIGHT
 	default y
 	help
 	  Say Y here if you want to control the backlight of your display.
@@ -999,7 +1004,6 @@ config FB_ATY
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
 	select FB_CFB_IMAGEBLIT
-	select FB_BACKLIGHT if FB_ATY_BACKLIGHT
 	select FB_IOMEM_FOPS
 	select FB_MACMODES if PPC
 	select FB_ATY_CT if SPARC64 && PCI
@@ -1040,6 +1044,8 @@ config FB_ATY_GX
 config FB_ATY_BACKLIGHT
 	bool "Support for backlight control"
 	depends on FB_ATY
+	depends on BACKLIGHT_CLASS_DEVICE
+	select FB_BACKLIGHT
 	default y
 	help
 	  Say Y here if you want to control the backlight of your display.
@@ -1528,6 +1534,7 @@ config FB_SH_MOBILE_LCDC
 	depends on FB && HAVE_CLK && HAS_IOMEM
 	depends on SUPERH || COMPILE_TEST
 	depends on FB_DEVICE
+	depends on BACKLIGHT_CLASS_DEVICE
 	select FB_BACKLIGHT
 	select FB_DEFERRED_IO
 	select FB_DMAMEM_HELPERS
@@ -1793,6 +1800,7 @@ config FB_SSD1307
 	tristate "Solomon SSD1307 framebuffer support"
 	depends on FB && I2C
 	depends on GPIOLIB || COMPILE_TEST
+	depends on BACKLIGHT_CLASS_DEVICE
 	select FB_BACKLIGHT
 	select FB_SYSMEM_HELPERS_DEFERRED
 	help
diff --git a/drivers/video/fbdev/core/Kconfig b/drivers/video/fbdev/core/Kconfig
index 0ab8848ba2f1..d554d8c543d4 100644
--- a/drivers/video/fbdev/core/Kconfig
+++ b/drivers/video/fbdev/core/Kconfig
@@ -183,9 +183,8 @@ config FB_SYSMEM_HELPERS_DEFERRED
 	select FB_SYSMEM_HELPERS
 
 config FB_BACKLIGHT
-	tristate
+	bool
 	depends on FB
-	select BACKLIGHT_CLASS_DEVICE
 
 config FB_MODE_HELPERS
 	bool "Enable Video Mode Handling Helpers"
-- 
2.47.1

[PATCH 2/3] drm/fbdev: Select FB_CORE dependency for fbdev on DMA and TTM

[Thomas Zimmermann]

Select FB_CORE if GEM's DMA and TTM implementations support fbdev
emulation. Fixes linker errors about missing symbols from the fbdev
subsystem.

Also see [1] for a related SHMEM fix.

Fixes: dadd28d4142f ("drm/client: Add client-lib module")
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Link: https://patchwork.freedesktop.org/series/141411/ # 1
---
 drivers/gpu/drm/Kconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index c181b2ee78b6..6105a55eb464 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -288,6 +288,7 @@ config DRM_TTM_HELPER
 	tristate
 	depends on DRM
 	select DRM_TTM
+	select FB_CORE if DRM_FBDEV_EMULATION
 	select FB_SYSMEM_HELPERS_DEFERRED if DRM_FBDEV_EMULATION
 	help
 	  Helpers for ttm-based gem objects
@@ -295,6 +296,7 @@ config DRM_TTM_HELPER
 config DRM_GEM_DMA_HELPER
 	tristate
 	depends on DRM
+	select FB_CORE if DRM_FBDEV_EMULATION
 	select FB_DMAMEM_HELPERS_DEFERRED if DRM_FBDEV_EMULATION
 	help
 	  Choose this if you need the GEM DMA helper functions
-- 
2.47.1

[PATCH 3/3] drm: rework FB_CORE dependency

[Thomas Zimmermann]

From: Arnd Bergmann <arnd@arndb.de>

The 'select FB_CORE' statement moved from CONFIG_DRM to DRM_CLIENT_LIB,
but there are now configurations that have code calling into fb_core
as built-in even though the client_lib itself is a loadable module:

x86_64-linux-ld: drivers/gpu/drm/drm_fb_helper.o: in function `drm_fb_helper_set_suspend':
drm_fb_helper.c:(.text+0x2c6): undefined reference to `fb_set_suspend'
x86_64-linux-ld: drivers/gpu/drm/drm_fb_helper.o: in function `drm_fb_helper_resume_worker':
drm_fb_helper.c:(.text+0x2e1): undefined reference to `fb_set_suspend'

In addition to DRM_CLIENT_LIB, the 'select' needs to be at least in
DRM_KMS_HELPER and DRM_GEM_SHMEM_HELPER, so add it here.

This patch is the KMS_HELPER part of [1].

Fixes: dadd28d4142f ("drm/client: Add client-lib module")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
Link: https://patchwork.freedesktop.org/series/141411/ # 1
---
 drivers/gpu/drm/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 6105a55eb464..d4efacbb24a3 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -99,6 +99,7 @@ config DRM_KUNIT_TEST
 config DRM_KMS_HELPER
 	tristate
 	depends on DRM
+	select FB_CORE if DRM_FBDEV_EMULATION
 	help
 	  CRTC helpers for KMS drivers.
 
-- 
2.47.1

Re: [PATCH 1/3] fbdev: Fix recursive dependencies wrt BACKLIGHT_CLASS_DEVICE

[Helge Deller]

On 12/10/24 15:09, Thomas Zimmermann wrote:
> diff --git a/drivers/staging/fbtft/Kconfig b/drivers/staging/fbtft/Kconfig
> index 77ab44362f16..577e91ff7bf6 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 FB_DEVICE
> +	depends on BACKLIGHT_DEVICE_CLASS

Typo. Should be BACKLIGHT_CLASS_DEVICE...

Helge

Re: [PATCH 1/3] fbdev: Fix recursive dependencies wrt BACKLIGHT_CLASS_DEVICE

[Arnd Bergmann]

On Tue, Dec 10, 2024, at 15:09, Thomas Zimmermann wrote:
> Do not select BACKLIGHT_CLASS_DEVICE from FB_BACKLIGHT. The latter
> only controls backlight support within fbdev core code and data
> structures.
>
> Make fbdev drivers depend on BACKLIGHT_CLASS_DEVICE and let users
> select it explicitly. Fixes warnings about recursive dependencies,
> such as
>
> error: recursive dependency detected!
> 	symbol BACKLIGHT_CLASS_DEVICE is selected by FB_BACKLIGHT
> 	symbol FB_BACKLIGHT is selected by FB_SH_MOBILE_LCDC
> 	symbol FB_SH_MOBILE_LCDC depends on FB_DEVICE
> 	symbol FB_DEVICE depends on FB_CORE
> 	symbol FB_CORE is selected by DRM_GEM_DMA_HELPER
> 	symbol DRM_GEM_DMA_HELPER is selected by DRM_PANEL_ILITEK_ILI9341
> 	symbol DRM_PANEL_ILITEK_ILI9341 depends on BACKLIGHT_CLASS_DEVICE
>
> BACKLIGHT_CLASS_DEVICE is user-selectable, so making drivers adapt to
> it is the correct approach in any case. For most drivers, backlight
> support is also configurable separately.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Thanks for revisiting this!

My patch that failed to work correctly happened to work on my
randconfig tree because I still have an old variant of this
change, see

https://lore.kernel.org/linux-fbdev/20200417155553.675905-8-arnd@arndb.de/

This is almost the same as your version, except for the
optional fbdev Kconfig bits
PERS
> @@ -660,7 +661,6 @@ config FB_ATMEL
>  config FB_NVIDIA
>  	tristate "nVidia Framebuffer Support"
>  	depends on FB && PCI
> -	select FB_BACKLIGHT if FB_NVIDIA_BACKLIGHT
>  	select FB_CFB_FILLRECT
>  	select FB_CFB_COPYAREA
>  	select FB_CFB_IMAGEBLIT
> @@ -700,6 +700,8 @@ config FB_NVIDIA_DEBUG
>  config FB_NVIDIA_BACKLIGHT
>  	bool "Support for backlight control"
>  	depends on FB_NVIDIA
> +	depends on BACKLIGHT_CLASS_DEVICE
> +	select FB_BACKLIGHT
>  	default y
>  	help
>  	  Say Y here if you want to control the backlight of your display.

For instance here I used

@@ -702,6 +703,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.

while your patch causes a link failure with

CONFIG_FB_NVIDIA=y
CONFIG_FB_NVIDIA_BACKLIGHT=y
CONFIG_BACKLIGHT_CLASS_DEVICE=m

       Arnd

Re: [PATCH 1/3] fbdev: Fix recursive dependencies wrt BACKLIGHT_CLASS_DEVICE

[Helge Deller]

On 12/10/24 15:29, Helge Deller wrote:
> On 12/10/24 15:09, Thomas Zimmermann wrote:
>> diff --git a/drivers/staging/fbtft/Kconfig b/drivers/staging/fbtft/Kconfig
>> index 77ab44362f16..577e91ff7bf6 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 FB_DEVICE
>> +    depends on BACKLIGHT_DEVICE_CLASS
>
> Typo. Should be BACKLIGHT_CLASS_DEVICE...

Beside the typo:
In this case, doesn't it make sense to "select BACKLIGHT_DEVICE_CLASS" instead?
If people want the fbtft, backlight support should be enabled too.

Helge

Re: [PATCH 1/3] fbdev: Fix recursive dependencies wrt BACKLIGHT_CLASS_DEVICE

[Thomas Zimmermann]

Hi


Am 10.12.24 um 15:34 schrieb Helge Deller:
> On 12/10/24 15:29, Helge Deller wrote:
>> On 12/10/24 15:09, Thomas Zimmermann wrote:
>>> diff --git a/drivers/staging/fbtft/Kconfig 
>>> b/drivers/staging/fbtft/Kconfig
>>> index 77ab44362f16..577e91ff7bf6 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 FB_DEVICE
>>> +    depends on BACKLIGHT_DEVICE_CLASS
>>
>> Typo. Should be BACKLIGHT_CLASS_DEVICE...

Ah, thanks. I'll better check the rest of the series for similar mistakes.

>
> Beside the typo:
> In this case, doesn't it make sense to "select BACKLIGHT_DEVICE_CLASS" 
> instead?

That causes the dependency error mentioned in the commit message. This 
time it's just for fbtft instead of shmobilefb.

> If people want the fbtft, backlight support should be enabled too.

As a user-visible option, it should not be auto-selected unnecessarily. 
The DRM panel drivers already depend on the backlight instead of 
selecting it. It's the correct approach. As I mentioned in the cover 
letter, the few remaining driver that select it should probably be updated.

Best regards
Thomas

>
> Helge
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH 1/3] fbdev: Fix recursive dependencies wrt BACKLIGHT_CLASS_DEVICE

[Thomas Zimmermann]

Hi


Am 10.12.24 um 15:30 schrieb Arnd Bergmann:
> On Tue, Dec 10, 2024, at 15:09, Thomas Zimmermann wrote:
>> Do not select BACKLIGHT_CLASS_DEVICE from FB_BACKLIGHT. The latter
>> only controls backlight support within fbdev core code and data
>> structures.
>>
>> Make fbdev drivers depend on BACKLIGHT_CLASS_DEVICE and let users
>> select it explicitly. Fixes warnings about recursive dependencies,
>> such as
>>
>> error: recursive dependency detected!
>> 	symbol BACKLIGHT_CLASS_DEVICE is selected by FB_BACKLIGHT
>> 	symbol FB_BACKLIGHT is selected by FB_SH_MOBILE_LCDC
>> 	symbol FB_SH_MOBILE_LCDC depends on FB_DEVICE
>> 	symbol FB_DEVICE depends on FB_CORE
>> 	symbol FB_CORE is selected by DRM_GEM_DMA_HELPER
>> 	symbol DRM_GEM_DMA_HELPER is selected by DRM_PANEL_ILITEK_ILI9341
>> 	symbol DRM_PANEL_ILITEK_ILI9341 depends on BACKLIGHT_CLASS_DEVICE
>>
>> BACKLIGHT_CLASS_DEVICE is user-selectable, so making drivers adapt to
>> it is the correct approach in any case. For most drivers, backlight
>> support is also configurable separately.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Thanks for revisiting this!
>
> My patch that failed to work correctly happened to work on my
> randconfig tree because I still have an old variant of this
> change, see
>
> https://lore.kernel.org/linux-fbdev/20200417155553.675905-8-arnd@arndb.de/
>
> This is almost the same as your version, except for the
> optional fbdev Kconfig bits
> PERS
>> @@ -660,7 +661,6 @@ config FB_ATMEL
>>   config FB_NVIDIA
>>   	tristate "nVidia Framebuffer Support"
>>   	depends on FB && PCI
>> -	select FB_BACKLIGHT if FB_NVIDIA_BACKLIGHT
>>   	select FB_CFB_FILLRECT
>>   	select FB_CFB_COPYAREA
>>   	select FB_CFB_IMAGEBLIT
>> @@ -700,6 +700,8 @@ config FB_NVIDIA_DEBUG
>>   config FB_NVIDIA_BACKLIGHT
>>   	bool "Support for backlight control"
>>   	depends on FB_NVIDIA
>> +	depends on BACKLIGHT_CLASS_DEVICE
>> +	select FB_BACKLIGHT
>>   	default y
>>   	help
>>   	  Say Y here if you want to control the backlight of your display.
> For instance here I used
>
> @@ -702,6 +703,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.
>
> while your patch causes a link failure with
>
> CONFIG_FB_NVIDIA=y
> CONFIG_FB_NVIDIA_BACKLIGHT=y
> CONFIG_BACKLIGHT_CLASS_DEVICE=m

Ah, right. I'll update the series to use your approach.

Best regards
Thomas

>
>         Arnd

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH 1/3] fbdev: Fix recursive dependencies wrt BACKLIGHT_CLASS_DEVICE

[Helge Deller]

On 12/10/24 16:41, Thomas Zimmermann wrote:
> Hi
>
>
> Am 10.12.24 um 15:34 schrieb Helge Deller:
>> On 12/10/24 15:29, Helge Deller wrote:
>>> On 12/10/24 15:09, Thomas Zimmermann wrote:
>>>> diff --git a/drivers/staging/fbtft/Kconfig b/drivers/staging/fbtft/Kconfig
>>>> index 77ab44362f16..577e91ff7bf6 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 FB_DEVICE
>>>> +    depends on BACKLIGHT_DEVICE_CLASS
>>>
>>> Typo. Should be BACKLIGHT_CLASS_DEVICE...
>
> Ah, thanks. I'll better check the rest of the series for similar mistakes.
>
>>
>> Beside the typo:
>> In this case, doesn't it make sense to "select BACKLIGHT_DEVICE_CLASS" instead?
>
> That causes the dependency error mentioned in the commit message. This time it's just for fbtft instead of shmobilefb.
>
>> If people want the fbtft, backlight support should be enabled too.
>
> As a user-visible option, it should not be auto-selected
> unnecessarily.

Right, it should not be auto-selected.
Unless if fbtft really needs it enabled to function.
IMHO all fb/drm drivers have higher priority than some low-level
background backlight controller code.

> The DRM panel drivers already depend on the backlight
> instead of selecting it. It's the correct approach.

Sounds wrong IMHO.

> As I mentioned
> in the cover letter, the few remaining driver that select it should
> probably be updated.

That dependency sounds weird, but maybe I simply misunderstand your logic...?

As a Linux end user I usually know which graphic cards are in my machine
and which ones I want to enable.
But as a normal user I think I shouldn't be expected to know
that I first need to enable the "backlight class device"
so that I'm then able to afterwards enable the fbtft (or any other drm/fb driver).

Am I wrong?

Helge

Re: [PATCH 1/3] fbdev: Fix recursive dependencies wrt BACKLIGHT_CLASS_DEVICE

[Helge Deller]

On 12/11/24 00:37, Helge Deller wrote:
> On 12/10/24 16:41, Thomas Zimmermann wrote:
>> Hi
>>
>>
>> Am 10.12.24 um 15:34 schrieb Helge Deller:
>>> On 12/10/24 15:29, Helge Deller wrote:
>>>> On 12/10/24 15:09, Thomas Zimmermann wrote:
>>>>> diff --git a/drivers/staging/fbtft/Kconfig b/drivers/staging/fbtft/Kconfig
>>>>> index 77ab44362f16..577e91ff7bf6 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 FB_DEVICE
>>>>> +    depends on BACKLIGHT_DEVICE_CLASS
>>>>
>>>> Typo. Should be BACKLIGHT_CLASS_DEVICE...
>>
>> Ah, thanks. I'll better check the rest of the series for similar mistakes.
>>
>>>
>>> Beside the typo:
>>> In this case, doesn't it make sense to "select BACKLIGHT_DEVICE_CLASS" instead?
>>
>> That causes the dependency error mentioned in the commit message. This time it's just for fbtft instead of shmobilefb.
>>
>>> If people want the fbtft, backlight support should be enabled too.
>>
>> As a user-visible option, it should not be auto-selected
>> unnecessarily.
>
> Right, it should not be auto-selected.
> Unless if fbtft really needs it enabled to function.
> IMHO all fb/drm drivers have higher priority than some low-level
> background backlight controller code.
>
>> The DRM panel drivers already depend on the backlight
>> instead of selecting it. It's the correct approach.
>
> Sounds wrong IMHO.
>
>> As I mentioned
>> in the cover letter, the few remaining driver that select it should
>> probably be updated.
>
> That dependency sounds weird, but maybe I simply misunderstand your logic...?
>
> As a Linux end user I usually know which graphic cards are in my machine
> and which ones I want to enable.
> But as a normal user I think I shouldn't be expected to know
> that I first need to enable the "backlight class device"
> so that I'm then able to afterwards enable the fbtft (or any other drm/fb driver).
>
> Am I wrong?

Looking closer on this...
You propose:

--- 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 FB_DEVICE
+	depends on BACKLIGHT_DEVICE_CLASS
  	depends on GPIOLIB || COMPILE_TEST
  	select FB_BACKLIGHT

So, it will depend on BACKLIGHT_DEVICE_CLASS.
But there is "select FB_BACKLIGHT" as well, which is:
config FB_BACKLIGHT
         tristate
         depends on FB
         select BACKLIGHT_CLASS_DEVICE

So, you end up with selecting and depending on BACKLIGHT_CLASS_DEVICE ?

Helge

Re: [PATCH 1/3] fbdev: Fix recursive dependencies wrt BACKLIGHT_CLASS_DEVICE

[Helge Deller]

On 12/11/24 00:53, Helge Deller wrote:
> On 12/11/24 00:37, Helge Deller wrote:
>> On 12/10/24 16:41, Thomas Zimmermann wrote:
>>> Hi
>>>
>>>
>>> Am 10.12.24 um 15:34 schrieb Helge Deller:
>>>> On 12/10/24 15:29, Helge Deller wrote:
>>>>> On 12/10/24 15:09, Thomas Zimmermann wrote:
>>>>>> diff --git a/drivers/staging/fbtft/Kconfig b/drivers/staging/fbtft/Kconfig
>>>>>> index 77ab44362f16..577e91ff7bf6 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 FB_DEVICE
>>>>>> +    depends on BACKLIGHT_DEVICE_CLASS
>>>>>
>>>>> Typo. Should be BACKLIGHT_CLASS_DEVICE...
>>>
>>> Ah, thanks. I'll better check the rest of the series for similar mistakes.
>>>
>>>>
>>>> Beside the typo:
>>>> In this case, doesn't it make sense to "select BACKLIGHT_DEVICE_CLASS" instead?
>>>
>>> That causes the dependency error mentioned in the commit message. This time it's just for fbtft instead of shmobilefb.
>>>
>>>> If people want the fbtft, backlight support should be enabled too.
>>>
>>> As a user-visible option, it should not be auto-selected
>>> unnecessarily.
>>
>> Right, it should not be auto-selected.
>> Unless if fbtft really needs it enabled to function.
>> IMHO all fb/drm drivers have higher priority than some low-level
>> background backlight controller code.
>>
>>> The DRM panel drivers already depend on the backlight
>>> instead of selecting it. It's the correct approach.
>>
>> Sounds wrong IMHO.
>>
>>> As I mentioned
>>> in the cover letter, the few remaining driver that select it should
>>> probably be updated.
>>
>> That dependency sounds weird, but maybe I simply misunderstand your logic...?
>>
>> As a Linux end user I usually know which graphic cards are in my machine
>> and which ones I want to enable.
>> But as a normal user I think I shouldn't be expected to know
>> that I first need to enable the "backlight class device"
>> so that I'm then able to afterwards enable the fbtft (or any other drm/fb driver).
>>
>> Am I wrong?
>
> Looking closer on this...
> You propose:
>
> --- 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 FB_DEVICE
> +    depends on BACKLIGHT_DEVICE_CLASS
>       depends on GPIOLIB || COMPILE_TEST
>       select FB_BACKLIGHT
>
> So, it will depend on BACKLIGHT_DEVICE_CLASS.
> But there is "select FB_BACKLIGHT" as well, which is:
> config FB_BACKLIGHT
>          tristate
>          depends on FB
>          select BACKLIGHT_CLASS_DEVICE
>
> So, you end up with selecting and depending on BACKLIGHT_CLASS_DEVICE ?

Ok. Ignore this ^^^ . I now understand your cover letter.

Looking at your DRM tiny drivers and the i915/gma500 DRM drivers,
there is a "select BACKLIGHT_CLASS_DEVICE" in those.

So, isn't the right approach then something like:

--- a/drivers/staging/fbtft/Kconfig
        tristate "Support for small TFT LCD display modules"
        depends on FB && SPI
        depends on FB_DEVICE
   +    select BACKLIGHT_DEVICE_CLASS
        depends on GPIOLIB || COMPILE_TEST
        select FB_BACKLIGHT

config FB_BACKLIGHT
           tristate
           depends on FB
   +       depends on BACKLIGHT_CLASS_DEVICE

?

Re: [PATCH 1/3] fbdev: Fix recursive dependencies wrt BACKLIGHT_CLASS_DEVICE

[Thomas Zimmermann]

Hi


Am 11.12.24 um 00:37 schrieb Helge Deller:
> On 12/10/24 16:41, Thomas Zimmermann wrote:
>> Hi
>>
>>
>> Am 10.12.24 um 15:34 schrieb Helge Deller:
>>> On 12/10/24 15:29, Helge Deller wrote:
>>>> On 12/10/24 15:09, Thomas Zimmermann wrote:
>>>>> diff --git a/drivers/staging/fbtft/Kconfig 
>>>>> b/drivers/staging/fbtft/Kconfig
>>>>> index 77ab44362f16..577e91ff7bf6 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 FB_DEVICE
>>>>> +    depends on BACKLIGHT_DEVICE_CLASS
>>>>
>>>> Typo. Should be BACKLIGHT_CLASS_DEVICE...
>>
>> Ah, thanks. I'll better check the rest of the series for similar 
>> mistakes.
>>
>>>
>>> Beside the typo:
>>> In this case, doesn't it make sense to "select 
>>> BACKLIGHT_DEVICE_CLASS" instead?
>>
>> That causes the dependency error mentioned in the commit message. 
>> This time it's just for fbtft instead of shmobilefb.
>>
>>> If people want the fbtft, backlight support should be enabled too.
>>
>> As a user-visible option, it should not be auto-selected
>> unnecessarily.
>
> Right, it should not be auto-selected.
> Unless if fbtft really needs it enabled to function.
> IMHO all fb/drm drivers have higher priority than some low-level
> background backlight controller code.

By that logic, we'd list always list all drivers and each driver would 
auso-select the subsystems it requires. So each fbdev driver would 
select CONFIG_FB.

That's not how it works, of course. Instead, each subsystem is 
user-selected and Kconfig offers the drivers that have their 
dependencies met. The documentation for Kconfig clearly states that 
select should be used carefully. [1]

[1] 
https://elixir.bootlin.com/linux/v6.12.4/source/Documentation/kbuild/kconfig-language.rst#L137

>
>> The DRM panel drivers already depend on the backlight
>> instead of selecting it. It's the correct approach.
>
> Sounds wrong IMHO.

Generally, it's the right approach. I guess what could be done is to 
make backlight support optional in the driver code, and use the imply 
attribute [2] instead of depends. So the driver would indicate a 
preference for backlight support, but still work without. That could 
also be done for the fbdev drivers, of course.

[2] 
https://elixir.bootlin.com/linux/v6.12.4/source/Documentation/kbuild/kconfig-language.rst#L163

Best regards
Thomas

>
>> As I mentioned
>> in the cover letter, the few remaining driver that select it should
>> probably be updated.
>
> That dependency sounds weird, but maybe I simply misunderstand your 
> logic...?
>
> As a Linux end user I usually know which graphic cards are in my machine
> and which ones I want to enable.
> But as a normal user I think I shouldn't be expected to know
> that I first need to enable the "backlight class device"
> so that I'm then able to afterwards enable the fbtft (or any other 
> drm/fb driver).
>
> Am I wrong?
>
> Helge

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH 1/3] fbdev: Fix recursive dependencies wrt BACKLIGHT_CLASS_DEVICE

[kernel test robot]

Hi Thomas,

kernel test robot noticed the following build errors:

[auto build test ERROR on staging/staging-testing]
[also build test ERROR on staging/staging-next staging/staging-linus drm/drm-next drm-exynos/exynos-drm-next linus/master v6.13-rc2 next-20241211]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Thomas-Zimmermann/fbdev-Fix-recursive-dependencies-wrt-BACKLIGHT_CLASS_DEVICE/20241210-222618
base:   staging/staging-testing
patch link:    https://lore.kernel.org/r/20241210142329.660801-2-tzimmermann%40suse.de
patch subject: [PATCH 1/3] fbdev: Fix recursive dependencies wrt BACKLIGHT_CLASS_DEVICE
config: loongarch-defconfig (https://download.01.org/0day-ci/archive/20241211/202412112135.pzFeIjEo-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241211/202412112135.pzFeIjEo-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412112135.pzFeIjEo-lkp@intel.com/

All errors (new ones prefixed by >>):

   loongarch64-linux-ld: drivers/video/fbdev/aty/radeon_backlight.o: in function `radeonfb_bl_init':
>> radeon_backlight.c:(.text+0x424): undefined reference to `backlight_device_register'
   loongarch64-linux-ld: drivers/video/fbdev/aty/radeon_backlight.o: in function `radeonfb_bl_exit':
>> radeon_backlight.c:(.text+0x560): undefined reference to `backlight_device_unregister'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki