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

[PATCH v3 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 depend
on the token as well, such that backlight functionality is fully user-
controlled.

v3:
- Fix PMAC_BACKLIGHT case (Christophe)
v2:
- s/BACKLIGHT_DEVICE_CLASS/BACKLIGHT_CLASS_DEVICE (Helge)
- Fix fbdev driver-dependency corner case (Arnd)

[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

 arch/powerpc/configs/pmac32_defconfig |  1 +
 arch/powerpc/configs/ppc6xx_defconfig |  1 +
 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 +--
 8 files changed, 22 insertions(+), 8 deletions(-)

-- 
2.47.1

[PATCH v3 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.

v3:
- Select BACKLIGHT_CLASS_DEVICE in PowerMac defconfigs (Christophe)
- Fix PMAC_BACKLIGHT module dependency corner cases (Christophe)
v2:
- s/BACKLIGHT_DEVICE_CLASS/BACKLIGHT_CLASS_DEVICE (Helge)
- Fix fbdev driver-dependency corner case (Arnd)

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

diff --git a/arch/powerpc/configs/pmac32_defconfig b/arch/powerpc/configs/pmac32_defconfig
index 57ded82c2840..e8b3f67bf3f5 100644
--- a/arch/powerpc/configs/pmac32_defconfig
+++ b/arch/powerpc/configs/pmac32_defconfig
@@ -208,6 +208,7 @@ CONFIG_FB_ATY=y
 CONFIG_FB_ATY_CT=y
 CONFIG_FB_ATY_GX=y
 CONFIG_FB_3DFX=y
+CONFIG_BACKLIGHT_CLASS_DEVICE=y
 # CONFIG_VGA_CONSOLE is not set
 CONFIG_FRAMEBUFFER_CONSOLE=y
 CONFIG_LOGO=y
diff --git a/arch/powerpc/configs/ppc6xx_defconfig b/arch/powerpc/configs/ppc6xx_defconfig
index 4d77e17541e9..ca0c90e95837 100644
--- a/arch/powerpc/configs/ppc6xx_defconfig
+++ b/arch/powerpc/configs/ppc6xx_defconfig
@@ -716,6 +716,7 @@ CONFIG_FB_TRIDENT=m
 CONFIG_FB_SM501=m
 CONFIG_FB_IBM_GXT4500=y
 CONFIG_LCD_PLATFORM=m
+CONFIG_BACKLIGHT_CLASS_DEVICE=y
 CONFIG_FRAMEBUFFER_CONSOLE=y
 CONFIG_FRAMEBUFFER_CONSOLE_ROTATION=y
 CONFIG_LOGO=y
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..d00e713c1092 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=y
 	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..dcf6a70455cc 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_CLASS_DEVICE
 	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..55c6686f091e 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=y || BACKLIGHT_CLASS_DEVICE=FB_NVIDIA
+	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=y || BACKLIGHT_CLASS_DEVICE=FB_RIVA
+	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=y || BACKLIGHT_CLASS_DEVICE=FB_RADEON
+	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=y || BACKLIGHT_CLASS_DEVICE=FB_ATY128
+	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=y || BACKLIGHT_CLASS_DEVICE=FB_ATY
+	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 v3 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 9abb41da4e3a..55ce61a46984 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -293,6 +293,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
@@ -300,6 +301,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 v3 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 55ce61a46984..2f51546b0b88 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 v3 0/3] drm,fbdev: Fix module dependencies

[Arnd Bergmann]

On Mon, Dec 16, 2024, at 08:42, Thomas Zimmermann wrote:
> 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 depend
> on the token as well, such that backlight functionality is fully user-
> controlled.
>
> v3:
> - Fix PMAC_BACKLIGHT case (Christophe)
> v2:

The patches look good to me. I've had a slightly different version
in my randconfig test tree and have replaced it with yours now
to do some more regression testing, but I expect this to be fine.

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

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

[Helge Deller]

On 12/16/24 08:42, 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.
>
> v3:
> - Select BACKLIGHT_CLASS_DEVICE in PowerMac defconfigs (Christophe)
> - Fix PMAC_BACKLIGHT module dependency corner cases (Christophe)
> v2:
> - s/BACKLIGHT_DEVICE_CLASS/BACKLIGHT_CLASS_DEVICE (Helge)
> - Fix fbdev driver-dependency corner case (Arnd)
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   arch/powerpc/configs/pmac32_defconfig |  1 +
>   arch/powerpc/configs/ppc6xx_defconfig |  1 +
>   drivers/auxdisplay/Kconfig            |  2 +-
>   drivers/macintosh/Kconfig             |  1 +
>   drivers/staging/fbtft/Kconfig         |  1 +
>   drivers/video/fbdev/Kconfig           | 18 +++++++++++++-----
>   drivers/video/fbdev/core/Kconfig      |  3 +--
>   7 files changed, 19 insertions(+), 8 deletions(-)
>
> ...
> diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
> index de035071fedb..55c6686f091e 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=y || BACKLIGHT_CLASS_DEVICE=FB_NVIDIA

Seems wrong. BACKLIGHT_CLASS_DEVICE is of type tristate.
There are more of those, please check.

Helge

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

[Thomas Zimmermann]

Hi


Am 22.12.24 um 07:31 schrieb Helge Deller:
> On 12/16/24 08:42, 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.
>>
>> v3:
>> - Select BACKLIGHT_CLASS_DEVICE in PowerMac defconfigs (Christophe)
>> - Fix PMAC_BACKLIGHT module dependency corner cases (Christophe)
>> v2:
>> - s/BACKLIGHT_DEVICE_CLASS/BACKLIGHT_CLASS_DEVICE (Helge)
>> - Fix fbdev driver-dependency corner case (Arnd)
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   arch/powerpc/configs/pmac32_defconfig |  1 +
>>   arch/powerpc/configs/ppc6xx_defconfig |  1 +
>>   drivers/auxdisplay/Kconfig            |  2 +-
>>   drivers/macintosh/Kconfig             |  1 +
>>   drivers/staging/fbtft/Kconfig         |  1 +
>>   drivers/video/fbdev/Kconfig           | 18 +++++++++++++-----
>>   drivers/video/fbdev/core/Kconfig      |  3 +--
>>   7 files changed, 19 insertions(+), 8 deletions(-)
>>
>> ...
>> diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
>> index de035071fedb..55c6686f091e 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=y || 
>> BACKLIGHT_CLASS_DEVICE=FB_NVIDIA
>
> Seems wrong. BACKLIGHT_CLASS_DEVICE is of type tristate.
> There are more of those, please check.

It's exactly correct. Linking would fail with FB_NVIDIA=y and 
BACKLIGHT=m. The above construct avoids this case. Please see Arnd's 
review comment at [1].

That's also why I mentioned that 'imply' could be used rather than 
'depends on'. It would handle this situation automatically.

Best regards
Thomas

[1] https://patchwork.freedesktop.org/patch/628099/?series=142356&rev=1


>
> 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 v3 1/3] fbdev: Fix recursive dependencies wrt BACKLIGHT_CLASS_DEVICE

[Helge Deller]

On 12/22/24 17:09, Thomas Zimmermann wrote:
> Hi
>
>
> Am 22.12.24 um 07:31 schrieb Helge Deller:
>> On 12/16/24 08:42, 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.
>>>
>>> v3:
>>> - Select BACKLIGHT_CLASS_DEVICE in PowerMac defconfigs (Christophe)
>>> - Fix PMAC_BACKLIGHT module dependency corner cases (Christophe)
>>> v2:
>>> - s/BACKLIGHT_DEVICE_CLASS/BACKLIGHT_CLASS_DEVICE (Helge)
>>> - Fix fbdev driver-dependency corner case (Arnd)
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> ---
>>>   arch/powerpc/configs/pmac32_defconfig |  1 +
>>>   arch/powerpc/configs/ppc6xx_defconfig |  1 +
>>>   drivers/auxdisplay/Kconfig            |  2 +-
>>>   drivers/macintosh/Kconfig             |  1 +
>>>   drivers/staging/fbtft/Kconfig         |  1 +
>>>   drivers/video/fbdev/Kconfig           | 18 +++++++++++++-----
>>>   drivers/video/fbdev/core/Kconfig      |  3 +--
>>>   7 files changed, 19 insertions(+), 8 deletions(-)
>>>
>>> ...
>>> diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
>>> index de035071fedb..55c6686f091e 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=y || BACKLIGHT_CLASS_DEVICE=FB_NVIDIA
>>
>> Seems wrong. BACKLIGHT_CLASS_DEVICE is of type tristate.
>> There are more of those, please check.
>
> It's exactly correct. Linking would fail with FB_NVIDIA=y and BACKLIGHT=m. The above construct avoids this case. Please see Arnd's review comment at [1].

I'm not arguing against "depends on BACKLIGHT_CLASS_DEVICE=y".
It's the "BACKLIGHT_CLASS_DEVICE=FB_NVIDIA" which is wrong.
BACKLIGHT_CLASS_DEVICE is tristate, so either "y", "n" or "m", but never "FB_NVIDIA".

> That's also why I mentioned that 'imply' could be used rather than 'depends on'. It would handle this situation automatically.
>
> Best regards
> Thomas
>
> [1] https://patchwork.freedesktop.org/patch/628099/?series=142356&rev=1

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

[Arnd Bergmann]

On Sun, Dec 22, 2024, at 21:15, Helge Deller wrote:
> On 12/22/24 17:09, Thomas Zimmermann wrote:

>>>> +    depends on BACKLIGHT_CLASS_DEVICE=y || BACKLIGHT_CLASS_DEVICE=FB_NVIDIA
>>>
>>> Seems wrong. BACKLIGHT_CLASS_DEVICE is of type tristate.
>>> There are more of those, please check.
>>
>> It's exactly correct. Linking would fail with FB_NVIDIA=y and BACKLIGHT=m. The above construct avoids this case. Please see Arnd's review comment at [1].
>
> I'm not arguing against "depends on BACKLIGHT_CLASS_DEVICE=y".
> It's the "BACKLIGHT_CLASS_DEVICE=FB_NVIDIA" which is wrong.
> BACKLIGHT_CLASS_DEVICE is tristate, so either "y", "n" or "m", but 
> never "FB_NVIDIA".

There are multiple ways to express this, but that line is a correct
way to allow all of

  BACKLIGHT_CLASS_DEVICE=y, FB_NVIDIA=m, FB_NVIDIA_BACKLIGHT=y
  BACKLIGHT_CLASS_DEVICE=y, FB_NVIDIA=y, FB_NVIDIA_BACKLIGHT=y
  BACKLIGHT_CLASS_DEVICE=m, FB_NVIDIA=m, FB_NVIDIA_BACKLIGHT=y

but disallow the broken

  BACKLIGHT_CLASS_DEVICE=m, FB_NVIDIA=y, FB_NVIDIA_BACKLIGHT=y

If you find this line too confusing, can you suggest a different
expression that has the same behavior?

       Arnd

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

[Helge Deller]

On 12/22/24 21:50, Arnd Bergmann wrote:
> On Sun, Dec 22, 2024, at 21:15, Helge Deller wrote:
>> On 12/22/24 17:09, Thomas Zimmermann wrote:
>
>>>>> +    depends on BACKLIGHT_CLASS_DEVICE=y || BACKLIGHT_CLASS_DEVICE=FB_NVIDIA
>>>>
>>>> Seems wrong. BACKLIGHT_CLASS_DEVICE is of type tristate.
>>>> There are more of those, please check.
>>>
>>> It's exactly correct. Linking would fail with FB_NVIDIA=y and BACKLIGHT=m. The above construct avoids this case. Please see Arnd's review comment at [1].
>>
>> I'm not arguing against "depends on BACKLIGHT_CLASS_DEVICE=y".
>> It's the "BACKLIGHT_CLASS_DEVICE=FB_NVIDIA" which is wrong.
>> BACKLIGHT_CLASS_DEVICE is tristate, so either "y", "n" or "m", but
>> never "FB_NVIDIA".
>
> There are multiple ways to express this, but that line is a correct
> way to allow all of
>
>    BACKLIGHT_CLASS_DEVICE=y, FB_NVIDIA=m, FB_NVIDIA_BACKLIGHT=y
>    BACKLIGHT_CLASS_DEVICE=y, FB_NVIDIA=y, FB_NVIDIA_BACKLIGHT=y
>    BACKLIGHT_CLASS_DEVICE=m, FB_NVIDIA=m, FB_NVIDIA_BACKLIGHT=y
>
> but disallow the broken
>
>    BACKLIGHT_CLASS_DEVICE=m, FB_NVIDIA=y, FB_NVIDIA_BACKLIGHT=y

Ouch.
So, in BACKLIGHT_CLASS_DEVICE=FB_NVIDIA,  "FB_NVIDIA" is simply
being replaced by the current value of the FB_NVIDIA config option
(which is then one of: y/n/m).
I didn't thought of that!

> If you find this line too confusing, can you suggest a different
> expression that has the same behavior?

It's confusing, but probably the shortest one.

Arnd, thanks for explaining!

Thomas, you may add
Acked-by: Helge Deller <deller@gmx.de>
to the series.
In case you want me to take the patch, please let me know.

Helge