Re: [PATCH v2 1/3] fbdev: Fix recursive dependencies wrt BACKLIGHT_CLASS_DEVICE
From: Simona Vetter <hidden>
Date: 2024-12-16 14:41:36
Also in:
dri-devel, linux-fbdev, linux-staging
On Fri, Dec 13, 2024 at 08:26:19AM +0100, Thomas Zimmermann wrote:
Hi Am 13.12.24 um 00:56 schrieb Helge Deller:quoted
On 12/13/24 00:24, Jani Nikula wrote:quoted
On Thu, 12 Dec 2024, "Arnd Bergmann" [off-list ref] wrote:quoted
On Thu, Dec 12, 2024, at 19:44, Helge Deller wrote:quoted
On 12/12/24 11:04, 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 [...]I think in the fbdev drivers themselves you should do: select BACKLIGHT_CLASS_DEVICE instead of "depending" on it. This is the way as it's done in the DRM tiny and the i915/gma500 DRM drivers. So, something like:--- a/drivers/staging/fbtft/Kconfigtristate "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 - select BACKLIGHT_CLASS_DEVICE + depends on BACKLIGHT_CLASS_DEVICE Would that fix the dependency warning?The above is generally a mistake and the root cause of the dependency loops. With very few exceptions, the solution in these cases is to find the inconsistent 'select' and change it into 'depends on'.Agreed.That's fine, but my point is that it should be consistent. For example: ~:/git-kernel/linux$ grep -r "select.*BACKLIGHT_CLASS_DEVICE" drivers/gpu/ drivers/gpu/drm/tilcdc/Kconfig: select BACKLIGHT_CLASS_DEVICE drivers/gpu/drm/nouveau/Kconfig: select BACKLIGHT_CLASS_DEVICE if DRM_NOUVEAU_BACKLIGHT drivers/gpu/drm/nouveau/Kconfig: select BACKLIGHT_CLASS_DEVICE if ACPI && X86 drivers/gpu/drm/tiny/Kconfig: select BACKLIGHT_CLASS_DEVICE drivers/gpu/drm/tiny/Kconfig: select BACKLIGHT_CLASS_DEVICE drivers/gpu/drm/tiny/Kconfig: select BACKLIGHT_CLASS_DEVICE drivers/gpu/drm/tiny/Kconfig: select BACKLIGHT_CLASS_DEVICE drivers/gpu/drm/tiny/Kconfig: select BACKLIGHT_CLASS_DEVICE drivers/gpu/drm/tiny/Kconfig: select BACKLIGHT_CLASS_DEVICE drivers/gpu/drm/tiny/Kconfig: select BACKLIGHT_CLASS_DEVICE drivers/gpu/drm/fsl-dcu/Kconfig: select BACKLIGHT_CLASS_DEVICE drivers/gpu/drm/i915/Kconfig: select BACKLIGHT_CLASS_DEVICE if ACPI drivers/gpu/drm/gma500/Kconfig: select BACKLIGHT_CLASS_DEVICE if ACPI drivers/gpu/drm/amd/amdgpu/Kconfig: select BACKLIGHT_CLASS_DEVICE drivers/gpu/drm/xe/Kconfig: select BACKLIGHT_CLASS_DEVICE if ACPI drivers/gpu/drm/solomon/Kconfig: select BACKLIGHT_CLASS_DEVICE drivers/gpu/drm/radeon/Kconfig: select BACKLIGHT_CLASS_DEVICE drivers/gpu/drm/renesas/shmobile/Kconfig: select BACKLIGHT_CLASS_DEVICE drivers/gpu/drm/gud/Kconfig: select BACKLIGHT_CLASS_DEVICE drivers/gpu/drm/bridge/Kconfig: select BACKLIGHT_CLASS_DEVICE All major drm graphics drivers *select* BACKLIGHT_CLASS_DEVICE. Are you changing them to "depend on" as well?All these drivers should be changed to either 'depends on' or maybe 'imply'.
Yeah, select on non-leaf/library function Kconfig symbols tends to be a complete pain. There's some push to use select so it's easier for people to enable complex drivers, but I honestly don't think it's worth it. menuconfig can give you a list of things you need to enable first, so it's all discoverable enough (but a bit painful to get them all if it's a really complex driver with lots of dependencies). tldr; I concur fully, please no more select but instead less. -Sima
Best regards Thomasquoted
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)
-- Simona Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch