| Message ID | 20241216074450.8590-1-tzimmermann@suse.de |
|---|---|
| Headers | show |
| Series | drm,fbdev: Fix module dependencies | expand |
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
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
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
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
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