[PATCH] drm/nouveau: fix LEDS_CLASS=m configuration
On 08/11/16 15:56, Arnd Bergmann wrote: > The newly introduced LED handling for nouveau fails to link when the > driver is built-in but the LED subsystem is a loadable module: > > drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_do_suspend': > tvnv17.c:(.text.nouveau_do_suspend+0x10): undefined reference to > `nouveau_led_suspend' > drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_do_resume': > tvnv17.c:(.text.nouveau_do_resume+0xf0): undefined reference to > `nouveau_led_resume' > drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_drm_unload': > tvnv17.c:(.text.nouveau_drm_unload+0x34): undefined reference to > `nouveau_led_fini' > drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_drm_load': > tvnv17.c:(.text.nouveau_drm_load+0x7d0): undefined reference to > `nouveau_led_init' > > This adds a separate Kconfig symbol for the LED support that > correctly tracks the dependencies. > > Fixes: 8d021d71b324 ("drm/nouveau/drm/nouveau: add a LED driver for the > NVIDIA logo") > Signed-off-by: Arnd Bergmann > --- > drivers/gpu/drm/nouveau/Kbuild| 2 +- > drivers/gpu/drm/nouveau/Kconfig | 8 > drivers/gpu/drm/nouveau/nouveau_led.h | 2 +- > 3 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/Kbuild b/drivers/gpu/drm/nouveau/Kbuild > index fde6e3656636..5e00e911daa6 100644 > --- a/drivers/gpu/drm/nouveau/Kbuild > +++ b/drivers/gpu/drm/nouveau/Kbuild > @@ -22,7 +22,7 @@ nouveau-$(CONFIG_DEBUG_FS) += nouveau_debugfs.o > nouveau-y += nouveau_drm.o > nouveau-y += nouveau_hwmon.o > nouveau-$(CONFIG_COMPAT) += nouveau_ioc32.o > -nouveau-$(CONFIG_LEDS_CLASS) += nouveau_led.o > +nouveau-$(CONFIG_DRM_NOUVEAU_LED) += nouveau_led.o > nouveau-y += nouveau_nvif.o > nouveau-$(CONFIG_NOUVEAU_PLATFORM_DRIVER) += nouveau_platform.o > nouveau-y += nouveau_usif.o # userspace <-> nvif > diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig > index 78631fb61adf..715cd6f4dc31 100644 > --- a/drivers/gpu/drm/nouveau/Kconfig > +++ b/drivers/gpu/drm/nouveau/Kconfig > @@ -46,6 +46,14 @@ config NOUVEAU_DEBUG > The paranoia and spam levels will add a lot of extra checks which > may potentially slow down driver operation. > > +config DRM_NOUVEAU_LED > + bool "Support for logo LED" > + depends on DRM_NOUVEAU && LEDS_CLASS > + depends on !(DRM_NOUVEAU=y && LEDS_CLASS=m) > + help > + Say Y here to enabling controlling the brightness of the > + LED behind NVIDIA logo on certain Titan cards. Titan is a little too specific and inaccurate. How about high-end cards? As for the patch itself... I was going for the auto-magic approach but apparently failed at it :s Sorry... I guess what I wanted to do was to only enable the LED support if LEDS_CLASS=y. Anyway, your approach is cleaner because it makes it easy for the user to say if he/she wants to enable. I will have a closer look at it later. Thanks anyway for reporting the issue! Martin
[Nouveau] [PATCH] drm/nouveau: fix LEDS_CLASS=m configuration
2016-11-08 17:12 GMT+01:00 Arnd Bergmann : > On Tuesday, November 8, 2016 5:07:01 PM CET Lukas Wunner wrote: >> On Tue, Nov 08, 2016 at 04:52:49PM +0100, Arnd Bergmann wrote: >> > The underlying problem is that we already have a number of other >> > symbols that either have "depends on LEDS_CLASS" or >> > "select LEDS_CLASS". To clean that up properly, we should either >> > make the symbol itself hidden and only select it from other drivers, >> > or use "depends on LEDS_CLASS" everywhere. >> > >> > Another option is to use the IS_REACHABLE() macro instead of IS_ENABLED() >> > in the header file, to stub out the calls into the new file, but >> > that can be a bit confusing. >> >> Why don't you just add empty inline stubs for nouveau_led_init / _fini / >> _suspend / _resume? >> > > That's what I was suggesting: > > diff --git a/drivers/gpu/drm/nouveau/nouveau_led.h > b/drivers/gpu/drm/nouveau/nouveau_led.h > index 9c9bb6ac938e..bc5f47cb516b 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_led.h > +++ b/drivers/gpu/drm/nouveau/nouveau_led.h > @@ -35,21 +35,21 @@ struct nouveau_led { > struct led_classdev led; > }; > > static inline struct nouveau_led * > nouveau_led(struct drm_device *dev) > { > return nouveau_drm(dev)->led; > } > > /* nouveau_led.c */ > -#if IS_ENABLED(CONFIG_LEDS_CLASS) > +#if IS_REACHABLE(CONFIG_LEDS_CLASS) > int nouveau_led_init(struct drm_device *dev); > void nouveau_led_suspend(struct drm_device *dev); > void nouveau_led_resume(struct drm_device *dev); > void nouveau_led_fini(struct drm_device *dev); > #else > static inline int nouveau_led_init(struct drm_device *dev) { return 0; }; > static inline void nouveau_led_suspend(struct drm_device *dev) { }; > static inline void nouveau_led_resume(struct drm_device *dev) { }; > static inline void nouveau_led_fini(struct drm_device *dev) { }; > #endif > > The downside is that now the nouveau_led_init() just won't be called > if CONFIG_LEDS_CLASS=m and CONFIG_DRM_NOUVEAU=y, which can be > surprising to users. yeah, it will. I guess it is fine to force LEDS to y if nouveau is set to y. The thinks I absolutely dislike is: 1. auto hiding of features I _want_ to have, because I would have to enable the dependencies first, which is like super annoying if there are somewhere else 2. preventing me from enabling something, cause the dependency is missing. We should clarify first if we actually want to enable those features optionally, because there isn't much of a reason not to enable the dependencies, except embedded systems. We have a lot more stuff where we could add things like that: hwmon, debugfs, acpi, compat and maybe there are even more things > > Arnd > ___ > Nouveau mailing list > Nouveau at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/nouveau
[Nouveau] [PATCH] drm/nouveau: fix LEDS_CLASS=m configuration
2016-11-08 16:46 GMT+01:00 Ilia Mirkin : > On Tue, Nov 8, 2016 at 8:56 AM, Arnd Bergmann wrote: >> The newly introduced LED handling for nouveau fails to link when the >> driver is built-in but the LED subsystem is a loadable module: >> >> drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_do_suspend': >> tvnv17.c:(.text.nouveau_do_suspend+0x10): undefined reference to >> `nouveau_led_suspend' >> drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_do_resume': >> tvnv17.c:(.text.nouveau_do_resume+0xf0): undefined reference to >> `nouveau_led_resume' >> drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_drm_unload': >> tvnv17.c:(.text.nouveau_drm_unload+0x34): undefined reference to >> `nouveau_led_fini' >> drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_drm_load': >> tvnv17.c:(.text.nouveau_drm_load+0x7d0): undefined reference to >> `nouveau_led_init' >> >> This adds a separate Kconfig symbol for the LED support that >> correctly tracks the dependencies. >> >> Fixes: 8d021d71b324 ("drm/nouveau/drm/nouveau: add a LED driver for the >> NVIDIA logo") >> Signed-off-by: Arnd Bergmann >> --- >> drivers/gpu/drm/nouveau/Kbuild| 2 +- >> drivers/gpu/drm/nouveau/Kconfig | 8 >> drivers/gpu/drm/nouveau/nouveau_led.h | 2 +- >> 3 files changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/nouveau/Kbuild b/drivers/gpu/drm/nouveau/Kbuild >> index fde6e3656636..5e00e911daa6 100644 >> --- a/drivers/gpu/drm/nouveau/Kbuild >> +++ b/drivers/gpu/drm/nouveau/Kbuild >> @@ -22,7 +22,7 @@ nouveau-$(CONFIG_DEBUG_FS) += nouveau_debugfs.o >> nouveau-y += nouveau_drm.o >> nouveau-y += nouveau_hwmon.o >> nouveau-$(CONFIG_COMPAT) += nouveau_ioc32.o >> -nouveau-$(CONFIG_LEDS_CLASS) += nouveau_led.o >> +nouveau-$(CONFIG_DRM_NOUVEAU_LED) += nouveau_led.o >> nouveau-y += nouveau_nvif.o >> nouveau-$(CONFIG_NOUVEAU_PLATFORM_DRIVER) += nouveau_platform.o >> nouveau-y += nouveau_usif.o # userspace <-> nvif >> diff --git a/drivers/gpu/drm/nouveau/Kconfig >> b/drivers/gpu/drm/nouveau/Kconfig >> index 78631fb61adf..715cd6f4dc31 100644 >> --- a/drivers/gpu/drm/nouveau/Kconfig >> +++ b/drivers/gpu/drm/nouveau/Kconfig >> @@ -46,6 +46,14 @@ config NOUVEAU_DEBUG >> The paranoia and spam levels will add a lot of extra checks which >> may potentially slow down driver operation. >> >> +config DRM_NOUVEAU_LED >> + bool "Support for logo LED" >> + depends on DRM_NOUVEAU && LEDS_CLASS >> + depends on !(DRM_NOUVEAU=y && LEDS_CLASS=m) >> + help >> + Say Y here to enabling controlling the brightness of the >> + LED behind NVIDIA logo on certain Titan cards. > > This is a very odd restriction... could this be written as > > depends on DRM_NOUVEAU > select LEDS_CLASS > > Or will that not flip the LEDS_CLASS from m to y in case > DRM_NOUVEAU=y? If not, is there a way to cause that to happen? > > Separately, perhaps we should just drop this LEDS_CLASS select into > DRM_NOUVEAU? We've tended to avoid adding tons of options. > well, that would mean that you always need the LEDS_CLASS and maybe on a tegra system you don't want to, so the led stuff should stay completely optional. Don't know though. Alex, maybe you want to clarify which dependencies should stay optional? If nobody on your side care, then we won't care as well and only add switches if users actually request it. > Cheers, > > -ilia > ___ > Nouveau mailing list > Nouveau at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/nouveau
[Nouveau] [PATCH] drm/nouveau: fix LEDS_CLASS=m configuration
On Tuesday, November 8, 2016 5:07:01 PM CET Lukas Wunner wrote: > On Tue, Nov 08, 2016 at 04:52:49PM +0100, Arnd Bergmann wrote: > > The underlying problem is that we already have a number of other > > symbols that either have "depends on LEDS_CLASS" or > > "select LEDS_CLASS". To clean that up properly, we should either > > make the symbol itself hidden and only select it from other drivers, > > or use "depends on LEDS_CLASS" everywhere. > > > > Another option is to use the IS_REACHABLE() macro instead of IS_ENABLED() > > in the header file, to stub out the calls into the new file, but > > that can be a bit confusing. > > Why don't you just add empty inline stubs for nouveau_led_init / _fini / > _suspend / _resume? > That's what I was suggesting: diff --git a/drivers/gpu/drm/nouveau/nouveau_led.h b/drivers/gpu/drm/nouveau/nouveau_led.h index 9c9bb6ac938e..bc5f47cb516b 100644 --- a/drivers/gpu/drm/nouveau/nouveau_led.h +++ b/drivers/gpu/drm/nouveau/nouveau_led.h @@ -35,21 +35,21 @@ struct nouveau_led { struct led_classdev led; }; static inline struct nouveau_led * nouveau_led(struct drm_device *dev) { return nouveau_drm(dev)->led; } /* nouveau_led.c */ -#if IS_ENABLED(CONFIG_LEDS_CLASS) +#if IS_REACHABLE(CONFIG_LEDS_CLASS) int nouveau_led_init(struct drm_device *dev); void nouveau_led_suspend(struct drm_device *dev); void nouveau_led_resume(struct drm_device *dev); void nouveau_led_fini(struct drm_device *dev); #else static inline int nouveau_led_init(struct drm_device *dev) { return 0; }; static inline void nouveau_led_suspend(struct drm_device *dev) { }; static inline void nouveau_led_resume(struct drm_device *dev) { }; static inline void nouveau_led_fini(struct drm_device *dev) { }; #endif The downside is that now the nouveau_led_init() just won't be called if CONFIG_LEDS_CLASS=m and CONFIG_DRM_NOUVEAU=y, which can be surprising to users. Arnd
[Nouveau] [PATCH] drm/nouveau: fix LEDS_CLASS=m configuration
On Tue, Nov 08, 2016 at 04:52:49PM +0100, Arnd Bergmann wrote: > The underlying problem is that we already have a number of other > symbols that either have "depends on LEDS_CLASS" or > "select LEDS_CLASS". To clean that up properly, we should either > make the symbol itself hidden and only select it from other drivers, > or use "depends on LEDS_CLASS" everywhere. > > Another option is to use the IS_REACHABLE() macro instead of IS_ENABLED() > in the header file, to stub out the calls into the new file, but > that can be a bit confusing. Why don't you just add empty inline stubs for nouveau_led_init / _fini / _suspend / _resume? Thanks, Lukas
[PATCH] drm/nouveau: fix LEDS_CLASS=m configuration
On Tuesday, November 8, 2016 10:46:07 AM CET Ilia Mirkin wrote: > > diff --git a/drivers/gpu/drm/nouveau/Kconfig > > b/drivers/gpu/drm/nouveau/Kconfig > > index 78631fb61adf..715cd6f4dc31 100644 > > --- a/drivers/gpu/drm/nouveau/Kconfig > > +++ b/drivers/gpu/drm/nouveau/Kconfig > > @@ -46,6 +46,14 @@ config NOUVEAU_DEBUG > > The paranoia and spam levels will add a lot of extra checks which > > may potentially slow down driver operation. > > > > +config DRM_NOUVEAU_LED > > + bool "Support for logo LED" > > + depends on DRM_NOUVEAU && LEDS_CLASS > > + depends on !(DRM_NOUVEAU=y && LEDS_CLASS=m) > > + help > > + Say Y here to enabling controlling the brightness of the > > + LED behind NVIDIA logo on certain Titan cards. > > This is a very odd restriction... could this be written as > > depends on DRM_NOUVEAU > select LEDS_CLASS > > Or will that not flip the LEDS_CLASS from m to y in case > DRM_NOUVEAU=y? If not, is there a way to cause that to happen? Using 'select' on user-selectable symbols risks introducing circular dependencies. > Separately, perhaps we should just drop this LEDS_CLASS select into > DRM_NOUVEAU? We've tended to avoid adding tons of options. My first attempt was to add "depends on LEDS_CLASS || !LEDS_CLASS" to DRM_NOUVEAU, which led to one long circle: drivers/usb/Kconfig:39:error: recursive dependency detected! For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/usb/Kconfig:39: symbol USB is selected by MOUSE_APPLETOUCH For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/input/mouse/Kconfig:187:symbol MOUSE_APPLETOUCH depends on INPUT For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/input/Kconfig:8:symbol INPUT is selected by VT For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/tty/Kconfig:12: symbol VT is selected by FB_STI For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/video/fbdev/Kconfig:669:symbol FB_STI depends on FB For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/video/fbdev/Kconfig:5: symbol FB is selected by DRM_KMS_FB_HELPER For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/gpu/drm/Kconfig:42: symbol DRM_KMS_FB_HELPER depends on DRM_KMS_HELPER For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/gpu/drm/Kconfig:36: symbol DRM_KMS_HELPER is selected by DRM_NOUVEAU For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/gpu/drm/nouveau/Kconfig:1: symbol DRM_NOUVEAU depends on LEDS_CLASS For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/leds/Kconfig:16:symbol LEDS_CLASS is selected by OMAP_DEBUG_LEDS For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" arch/arm/plat-omap/Kconfig:19: symbol OMAP_DEBUG_LEDS depends on NEW_LEDS For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/leds/Kconfig:8: symbol NEW_LEDS is selected by ATH9K_HTC For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/net/wireless/ath/ath9k/Kconfig:158: symbol ATH9K_HTC depends on USB Tue, 08 Nov 2016 11:49:44 +0100 build/0x3053A542_defconfig warnings drivers/usb/Kconfig:39:error: recursive dependency detected! For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/usb/Kconfig:39: symbol USB is selected by MOUSE_APPLETOUCH For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/input/mouse/Kconfig:187:symbol MOUSE_APPLETOUCH depends on INPUT For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/input/Kconfig:8:symbol INPUT is selected by VT For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/tty/Kconfig:12: symbol VT is selected by FB_STI For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations"
[Nouveau] [PATCH] drm/nouveau: fix LEDS_CLASS=m configuration
On 8 November 2016 at 16:21, Karol Herbst wrote: > 2016-11-08 17:12 GMT+01:00 Arnd Bergmann : >> On Tuesday, November 8, 2016 5:07:01 PM CET Lukas Wunner wrote: >>> On Tue, Nov 08, 2016 at 04:52:49PM +0100, Arnd Bergmann wrote: >>> > The underlying problem is that we already have a number of other >>> > symbols that either have "depends on LEDS_CLASS" or >>> > "select LEDS_CLASS". To clean that up properly, we should either >>> > make the symbol itself hidden and only select it from other drivers, >>> > or use "depends on LEDS_CLASS" everywhere. >>> > >>> > Another option is to use the IS_REACHABLE() macro instead of IS_ENABLED() >>> > in the header file, to stub out the calls into the new file, but >>> > that can be a bit confusing. >>> >>> Why don't you just add empty inline stubs for nouveau_led_init / _fini / >>> _suspend / _resume? >>> >> >> That's what I was suggesting: >> >> diff --git a/drivers/gpu/drm/nouveau/nouveau_led.h >> b/drivers/gpu/drm/nouveau/nouveau_led.h >> index 9c9bb6ac938e..bc5f47cb516b 100644 >> --- a/drivers/gpu/drm/nouveau/nouveau_led.h >> +++ b/drivers/gpu/drm/nouveau/nouveau_led.h >> @@ -35,21 +35,21 @@ struct nouveau_led { >> struct led_classdev led; >> }; >> >> static inline struct nouveau_led * >> nouveau_led(struct drm_device *dev) >> { >> return nouveau_drm(dev)->led; >> } >> >> /* nouveau_led.c */ >> -#if IS_ENABLED(CONFIG_LEDS_CLASS) >> +#if IS_REACHABLE(CONFIG_LEDS_CLASS) >> int nouveau_led_init(struct drm_device *dev); >> void nouveau_led_suspend(struct drm_device *dev); >> void nouveau_led_resume(struct drm_device *dev); >> void nouveau_led_fini(struct drm_device *dev); >> #else >> static inline int nouveau_led_init(struct drm_device *dev) { return 0; }; >> static inline void nouveau_led_suspend(struct drm_device *dev) { }; >> static inline void nouveau_led_resume(struct drm_device *dev) { }; >> static inline void nouveau_led_fini(struct drm_device *dev) { }; >> #endif >> >> The downside is that now the nouveau_led_init() just won't be called >> if CONFIG_LEDS_CLASS=m and CONFIG_DRM_NOUVEAU=y, which can be >> surprising to users. > > yeah, it will. I guess it is fine to force LEDS to y if nouveau is set > to y. The thinks I absolutely dislike is: > 1. auto hiding of features I _want_ to have, because I would have to > enable the dependencies first, which is like super annoying if there > are somewhere else > 2. preventing me from enabling something, cause the dependency is missing. > > We should clarify first if we actually want to enable those features > optionally, because there isn't much of a reason not to enable the > dependencies, except embedded systems. We have a lot more stuff where > we could add things like that: hwmon, debugfs, acpi, compat and maybe > there are even more things > Sounds like people may have missed the core part: This/earlier patch are required due to select "abuse" elsewhere in the kernel. The IS_REACHABLE/DRM_NOUVEAU_LED is patch to workaround things on nouveau side, with a proper one to remove/untangle the "select LEDS_CLASS". The latter will likely be slow/pain, since devs love to use select because it's convenient (and indeed it is). Thus [temporary] workaround on nouveau side will be good in the short term. Afaict, "forcing LEDS to y if nouveau is set to y" is going the opposite direction of what one should be going ;-) Regards, Emil
[PATCH] drm/nouveau: fix LEDS_CLASS=m configuration
The newly introduced LED handling for nouveau fails to link when the driver is built-in but the LED subsystem is a loadable module: drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_do_suspend': tvnv17.c:(.text.nouveau_do_suspend+0x10): undefined reference to `nouveau_led_suspend' drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_do_resume': tvnv17.c:(.text.nouveau_do_resume+0xf0): undefined reference to `nouveau_led_resume' drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_drm_unload': tvnv17.c:(.text.nouveau_drm_unload+0x34): undefined reference to `nouveau_led_fini' drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_drm_load': tvnv17.c:(.text.nouveau_drm_load+0x7d0): undefined reference to `nouveau_led_init' This adds a separate Kconfig symbol for the LED support that correctly tracks the dependencies. Fixes: 8d021d71b324 ("drm/nouveau/drm/nouveau: add a LED driver for the NVIDIA logo") Signed-off-by: Arnd Bergmann --- drivers/gpu/drm/nouveau/Kbuild| 2 +- drivers/gpu/drm/nouveau/Kconfig | 8 drivers/gpu/drm/nouveau/nouveau_led.h | 2 +- 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/nouveau/Kbuild b/drivers/gpu/drm/nouveau/Kbuild index fde6e3656636..5e00e911daa6 100644 --- a/drivers/gpu/drm/nouveau/Kbuild +++ b/drivers/gpu/drm/nouveau/Kbuild @@ -22,7 +22,7 @@ nouveau-$(CONFIG_DEBUG_FS) += nouveau_debugfs.o nouveau-y += nouveau_drm.o nouveau-y += nouveau_hwmon.o nouveau-$(CONFIG_COMPAT) += nouveau_ioc32.o -nouveau-$(CONFIG_LEDS_CLASS) += nouveau_led.o +nouveau-$(CONFIG_DRM_NOUVEAU_LED) += nouveau_led.o nouveau-y += nouveau_nvif.o nouveau-$(CONFIG_NOUVEAU_PLATFORM_DRIVER) += nouveau_platform.o nouveau-y += nouveau_usif.o # userspace <-> nvif diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig index 78631fb61adf..715cd6f4dc31 100644 --- a/drivers/gpu/drm/nouveau/Kconfig +++ b/drivers/gpu/drm/nouveau/Kconfig @@ -46,6 +46,14 @@ config NOUVEAU_DEBUG The paranoia and spam levels will add a lot of extra checks which may potentially slow down driver operation. +config DRM_NOUVEAU_LED + bool "Support for logo LED" + depends on DRM_NOUVEAU && LEDS_CLASS + depends on !(DRM_NOUVEAU=y && LEDS_CLASS=m) + help + Say Y here to enabling controlling the brightness of the + LED behind NVIDIA logo on certain Titan cards. + config NOUVEAU_DEBUG_DEFAULT int "Default debug level" depends on DRM_NOUVEAU diff --git a/drivers/gpu/drm/nouveau/nouveau_led.h b/drivers/gpu/drm/nouveau/nouveau_led.h index 187ecdb82002..9c9bb6ac938e 100644 --- a/drivers/gpu/drm/nouveau/nouveau_led.h +++ b/drivers/gpu/drm/nouveau/nouveau_led.h @@ -42,7 +42,7 @@ nouveau_led(struct drm_device *dev) } /* nouveau_led.c */ -#if IS_ENABLED(CONFIG_LEDS_CLASS) +#if IS_ENABLED(CONFIG_DRM_NOUVEAU_LED) int nouveau_led_init(struct drm_device *dev); void nouveau_led_suspend(struct drm_device *dev); void nouveau_led_resume(struct drm_device *dev); -- 2.9.0
[PATCH] drm/nouveau: fix LEDS_CLASS=m configuration
On Tue, Nov 8, 2016 at 8:56 AM, Arnd Bergmann wrote: > The newly introduced LED handling for nouveau fails to link when the > driver is built-in but the LED subsystem is a loadable module: > > drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_do_suspend': > tvnv17.c:(.text.nouveau_do_suspend+0x10): undefined reference to > `nouveau_led_suspend' > drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_do_resume': > tvnv17.c:(.text.nouveau_do_resume+0xf0): undefined reference to > `nouveau_led_resume' > drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_drm_unload': > tvnv17.c:(.text.nouveau_drm_unload+0x34): undefined reference to > `nouveau_led_fini' > drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_drm_load': > tvnv17.c:(.text.nouveau_drm_load+0x7d0): undefined reference to > `nouveau_led_init' > > This adds a separate Kconfig symbol for the LED support that > correctly tracks the dependencies. > > Fixes: 8d021d71b324 ("drm/nouveau/drm/nouveau: add a LED driver for the > NVIDIA logo") > Signed-off-by: Arnd Bergmann > --- > drivers/gpu/drm/nouveau/Kbuild| 2 +- > drivers/gpu/drm/nouveau/Kconfig | 8 > drivers/gpu/drm/nouveau/nouveau_led.h | 2 +- > 3 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/Kbuild b/drivers/gpu/drm/nouveau/Kbuild > index fde6e3656636..5e00e911daa6 100644 > --- a/drivers/gpu/drm/nouveau/Kbuild > +++ b/drivers/gpu/drm/nouveau/Kbuild > @@ -22,7 +22,7 @@ nouveau-$(CONFIG_DEBUG_FS) += nouveau_debugfs.o > nouveau-y += nouveau_drm.o > nouveau-y += nouveau_hwmon.o > nouveau-$(CONFIG_COMPAT) += nouveau_ioc32.o > -nouveau-$(CONFIG_LEDS_CLASS) += nouveau_led.o > +nouveau-$(CONFIG_DRM_NOUVEAU_LED) += nouveau_led.o > nouveau-y += nouveau_nvif.o > nouveau-$(CONFIG_NOUVEAU_PLATFORM_DRIVER) += nouveau_platform.o > nouveau-y += nouveau_usif.o # userspace <-> nvif > diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig > index 78631fb61adf..715cd6f4dc31 100644 > --- a/drivers/gpu/drm/nouveau/Kconfig > +++ b/drivers/gpu/drm/nouveau/Kconfig > @@ -46,6 +46,14 @@ config NOUVEAU_DEBUG > The paranoia and spam levels will add a lot of extra checks which > may potentially slow down driver operation. > > +config DRM_NOUVEAU_LED > + bool "Support for logo LED" > + depends on DRM_NOUVEAU && LEDS_CLASS > + depends on !(DRM_NOUVEAU=y && LEDS_CLASS=m) > + help > + Say Y here to enabling controlling the brightness of the > + LED behind NVIDIA logo on certain Titan cards. This is a very odd restriction... could this be written as depends on DRM_NOUVEAU select LEDS_CLASS Or will that not flip the LEDS_CLASS from m to y in case DRM_NOUVEAU=y? If not, is there a way to cause that to happen? Separately, perhaps we should just drop this LEDS_CLASS select into DRM_NOUVEAU? We've tended to avoid adding tons of options. Cheers, -ilia