On 12/22/24 17:09, Thomas Zimmermann wrote:

Hi


Am 22.12.24 um 07:31 schrieb Helge Deller:

quoted

On 12/16/24 08:42, Thomas Zimmermann wrote:

quoted

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