| Message ID | 51816271.3040400@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
Hi Randy and all, Seems like you found a problem... but the relevant sections of 'hid-steelseries.c' already have -- #if defined(CONFIG_LEDS_CLASS) || defined(CONFIG_LEDS_CLASS_MODULE) ... #endif -- Shouldn't this prevent the module having calls to register/unregister if the LED_CLASS is not enabled? Does forcing a 'depends on LED_CLASS' in Kconfig prevent the hid-steelseries module being built on systems without LEDs, or is this simply a way to ensure that the LED_CLASS module gets loaded first? Simon. > From: Randy Dunlap <rdunlap@infradead.org> > > Fix hid-steelseries build by making it depends on LEDS_CLASS. > Build errors happen when LEDS_CLASS=m and HID_STEELSERIES=y. > > drivers/built-in.o: In function `steelseries_srws1_remove': > hid-steelseries.c:(.text+0x3b97a1): undefined reference to > `led_classdev_unregister' > drivers/built-in.o: In function `steelseries_srws1_probe': > hid-steelseries.c:(.text+0x3b9c51): undefined reference to > `led_classdev_register' > hid-steelseries.c:(.text+0x3b9ce5): undefined reference to > `led_classdev_register' > hid-steelseries.c:(.text+0x3b9d4b): undefined reference to > `led_classdev_unregister' > > Signed-off-by: Randy Dunlap <rdunlap@infradead.org> > --- > drivers/hid/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > --- linux-next-20130501.orig/drivers/hid/Kconfig > +++ linux-next-20130501/drivers/hid/Kconfig > @@ -610,6 +610,7 @@ config HID_SPEEDLINK > config HID_STEELSERIES > tristate "Steelseries SRW-S1 steering wheel support" > depends on HID > + depends on LEDS_CLASS > ---help--- > Support for Steelseries SRW-S1 steering wheel > > -- > To unsubscribe from this list: send the line "unsubscribe linux-input" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/01/13 12:27, simon@mungewell.org wrote: > Hi Randy and all, > Seems like you found a problem... but the relevant sections of > 'hid-steelseries.c' already have > -- > #if defined(CONFIG_LEDS_CLASS) || defined(CONFIG_LEDS_CLASS_MODULE) which should be replaced with: #include <linux/kconfig.h> #if IS_ENABLED(LEDS_CLASS) > ... > #endif > -- > > Shouldn't this prevent the module having calls to register/unregister if > the LED_CLASS is not enabled? Please read the patch description. CONFIG_LEDS_CLASS=m but CONFIG_HID_STEELSEREIS=y. A builtin driver cannot make calls to a modular driver if the modular driver is not loaded. Is there a way to require that the modular driver is loaded for hid-steelseries? Maybe so, but I don't know. > Does forcing a 'depends on LED_CLASS' in Kconfig prevent the > hid-steelseries module being built on systems without LEDs, or is this > simply a way to ensure that the LED_CLASS module gets loaded first? It prevents the driver from being built. I think that a satisfactory solution would be this small change: depends on LEDS_CLASS || LEDS_CLASS=n That makes HID_STEELSERIES depend on LEDS_CLASS when it is enabled but still allows the driver to build when LEDS_CLASS=n. Makes sense? Oh, and please don't top-post. Thanks. > > Simon. > >> From: Randy Dunlap <rdunlap@infradead.org> >> >> Fix hid-steelseries build by making it depends on LEDS_CLASS. >> Build errors happen when LEDS_CLASS=m and HID_STEELSERIES=y. >> >> drivers/built-in.o: In function `steelseries_srws1_remove': >> hid-steelseries.c:(.text+0x3b97a1): undefined reference to >> `led_classdev_unregister' >> drivers/built-in.o: In function `steelseries_srws1_probe': >> hid-steelseries.c:(.text+0x3b9c51): undefined reference to >> `led_classdev_register' >> hid-steelseries.c:(.text+0x3b9ce5): undefined reference to >> `led_classdev_register' >> hid-steelseries.c:(.text+0x3b9d4b): undefined reference to >> `led_classdev_unregister' >> >> Signed-off-by: Randy Dunlap <rdunlap@infradead.org> >> --- >> drivers/hid/Kconfig | 1 + >> 1 file changed, 1 insertion(+) >> >> --- linux-next-20130501.orig/drivers/hid/Kconfig >> +++ linux-next-20130501/drivers/hid/Kconfig >> @@ -610,6 +610,7 @@ config HID_SPEEDLINK >> config HID_STEELSERIES >> tristate "Steelseries SRW-S1 steering wheel support" >> depends on HID >> + depends on LEDS_CLASS >> ---help--- >> Support for Steelseries SRW-S1 steering wheel >> >> --
> On 05/01/13 12:27, simon@mungewell.org wrote: >> Hi Randy and all, >> Seems like you found a problem... but the relevant sections of >> 'hid-steelseries.c' already have >> -- >> #if defined(CONFIG_LEDS_CLASS) || defined(CONFIG_LEDS_CLASS_MODULE) > > which should be replaced with: > > #include <linux/kconfig.h> > > #if IS_ENABLED(LEDS_CLASS) > > >> ... >> #endif >> -- >> >> Shouldn't this prevent the module having calls to register/unregister if >> the LED_CLASS is not enabled? > > Please read the patch description. CONFIG_LEDS_CLASS=m but > CONFIG_HID_STEELSEREIS=y. > A builtin driver cannot make calls to a modular driver if the modular > driver > is not loaded. Is there a way to require that the modular driver is > loaded > for hid-steelseries? Maybe so, but I don't know. I had a look around what other drivers do, but there is not much 'case law' to follow (or copy from). 'drivers/media/radio/radio-shark.c' does it like this: -- #if defined(CONFIG_LEDS_CLASS) || \ (defined(CONFIG_LEDS_CLASS_MODULE) && defined(CONFIG_RADIO_SHARK_MODULE)) ... #endif -- Which I think is another solution, and perhaps a little more readable/understandable (ie. you have led control if LED_CLASS is built-in, or both the driver _and_ LED_CLASS are modules). If there is not an objection to this style I'll prepare and test a patch in the next couple of days. Simon. PS. 'drivers/platform/x86/fujitsu-laptop.c' might have the same issues as hid-steelseries.... > > >> Does forcing a 'depends on LED_CLASS' in Kconfig prevent the >> hid-steelseries module being built on systems without LEDs, or is this >> simply a way to ensure that the LED_CLASS module gets loaded first? > > It prevents the driver from being built. I think that a satisfactory > solution > would be this small change: > > depends on LEDS_CLASS || LEDS_CLASS=n > > That makes HID_STEELSERIES depend on LEDS_CLASS when it is enabled but > still > allows the driver to build when LEDS_CLASS=n. Makes sense? > > > > Oh, and please don't top-post. > > Thanks. > >> >> Simon. >> >>> From: Randy Dunlap <rdunlap@infradead.org> >>> >>> Fix hid-steelseries build by making it depends on LEDS_CLASS. >>> Build errors happen when LEDS_CLASS=m and HID_STEELSERIES=y. >>> >>> drivers/built-in.o: In function `steelseries_srws1_remove': >>> hid-steelseries.c:(.text+0x3b97a1): undefined reference to >>> `led_classdev_unregister' >>> drivers/built-in.o: In function `steelseries_srws1_probe': >>> hid-steelseries.c:(.text+0x3b9c51): undefined reference to >>> `led_classdev_register' >>> hid-steelseries.c:(.text+0x3b9ce5): undefined reference to >>> `led_classdev_register' >>> hid-steelseries.c:(.text+0x3b9d4b): undefined reference to >>> `led_classdev_unregister' >>> >>> Signed-off-by: Randy Dunlap <rdunlap@infradead.org> >>> --- >>> drivers/hid/Kconfig | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> --- linux-next-20130501.orig/drivers/hid/Kconfig >>> +++ linux-next-20130501/drivers/hid/Kconfig >>> @@ -610,6 +610,7 @@ config HID_SPEEDLINK >>> config HID_STEELSERIES >>> tristate "Steelseries SRW-S1 steering wheel support" >>> depends on HID >>> + depends on LEDS_CLASS >>> ---help--- >>> Support for Steelseries SRW-S1 steering wheel >>> >>> -- > > > -- > ~Randy > -- > To unsubscribe from this list: send the line "unsubscribe linux-input" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- linux-next-20130501.orig/drivers/hid/Kconfig +++ linux-next-20130501/drivers/hid/Kconfig @@ -610,6 +610,7 @@ config HID_SPEEDLINK config HID_STEELSERIES tristate "Steelseries SRW-S1 steering wheel support" depends on HID + depends on LEDS_CLASS ---help--- Support for Steelseries SRW-S1 steering wheel