| Message ID | 20170728131826.180483-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On July 28, 2017 6:18:00 AM PDT, Arnd Bergmann <arnd@arndb.de> wrote: >The driver has gained a compile-time dependency that we should >express in Kconfig to avoid this link error: > Would conditional compilation be an acceptable alternative to adding a dependency? The USB_HID code is only used to check if the driver is working with a USB device. With USB_HID disabled, there's no need for the check so there's no need for the dependency. >drivers/hid/wacom_sys.o: In function `wacom_parse_and_register': >wacom_sys.c:(.text+0x2eec): undefined reference to `usb_hid_driver' > >Fixes: 09dc28acaec7 ("HID: wacom: Improve generic name generation") >Signed-off-by: Arnd Bergmann <arnd@arndb.de> >--- > drivers/hid/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > >diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig >index 3cd60f460b61..0a3117cc29e7 100644 >--- a/drivers/hid/Kconfig >+++ b/drivers/hid/Kconfig >@@ -924,7 +924,7 @@ config HID_UDRAW_PS3 > > config HID_WACOM > tristate "Wacom Intuos/Graphire tablet support (USB)" >- depends on HID >+ depends on USB_HID > select POWER_SUPPLY > select NEW_LEDS > select LEDS_CLASS
On Fri, Jul 28, 2017 at 4:07 PM, Jason Gerecke <killertofu@gmail.com> wrote: > On July 28, 2017 6:18:00 AM PDT, Arnd Bergmann <arnd@arndb.de> wrote: >>The driver has gained a compile-time dependency that we should >>express in Kconfig to avoid this link error: >> > > Would conditional compilation be an acceptable alternative to adding > a dependency? The USB_HID code is only used to check if the driver > is working with a USB device. With USB_HID disabled, there's no need > for the check so there's no need for the dependency. I think that should work, e.g. you could replace the hid_is_using_ll_driver and 'extern' defintions with a helper per ll-driver like #ifdef CONFIG_USB_HID extern bool hid_is_using_usb_driver(struct hid_device *hdev) #else static inline bool hid_is_using_usb_driver(struct hid_device *hdev) { return false; } #endif but is it worth it to avoid the dependency? Arnd -- 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 Fri, Jul 28, 2017 at 7:18 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Fri, Jul 28, 2017 at 4:07 PM, Jason Gerecke <killertofu@gmail.com> wrote: >> On July 28, 2017 6:18:00 AM PDT, Arnd Bergmann <arnd@arndb.de> wrote: >>>The driver has gained a compile-time dependency that we should >>>express in Kconfig to avoid this link error: >>> >> >> Would conditional compilation be an acceptable alternative to adding >> a dependency? The USB_HID code is only used to check if the driver >> is working with a USB device. With USB_HID disabled, there's no need >> for the check so there's no need for the dependency. > > I think that should work, e.g. you could replace the hid_is_using_ll_driver > and 'extern' defintions with a helper per ll-driver like > > #ifdef CONFIG_USB_HID > extern bool hid_is_using_usb_driver(struct hid_device *hdev) > #else > static inline bool hid_is_using_usb_driver(struct hid_device *hdev) > { > return false; > } > #endif > > but is it worth it to avoid the dependency? > > Arnd I was thinking something more along the lines of the following since the idea of per-transport helper functions was dismissed earlier: #ifdef CONFIG_USB_HID if (hid_is_using_ll_driver(wacom->hdev, &usb_hid_driver)) { [...] } #endif Jason -- 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 Fri, Jul 28, 2017 at 4:24 PM, Jason Gerecke <killertofu@gmail.com> wrote: > On Fri, Jul 28, 2017 at 7:18 AM, Arnd Bergmann <arnd@arndb.de> wrote: >> On Fri, Jul 28, 2017 at 4:07 PM, Jason Gerecke <killertofu@gmail.com> wrote: >> #ifdef CONFIG_USB_HID >> extern bool hid_is_using_usb_driver(struct hid_device *hdev) >> #else >> static inline bool hid_is_using_usb_driver(struct hid_device *hdev) >> { >> return false; >> } >> #endif >> >> but is it worth it to avoid the dependency? >> >> Arnd > > I was thinking something more along the lines of the following since > the idea of per-transport helper functions was dismissed earlier: > > #ifdef CONFIG_USB_HID > if (hid_is_using_ll_driver(wacom->hdev, &usb_hid_driver)) { I would consider that rather ugly, a driver shouldn't really use #ifdef like this, but you can hide stuff like this in a header. The method I proposed also has the advantage of avoiding exporting the usb_hid_driver object. Drivers shouldn't really need to access this, and wacom_sys.c is the only remaining user of the export. Arnd -- 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 Fri, Jul 28, 2017 at 7:29 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Fri, Jul 28, 2017 at 4:24 PM, Jason Gerecke <killertofu@gmail.com> wrote: >> On Fri, Jul 28, 2017 at 7:18 AM, Arnd Bergmann <arnd@arndb.de> wrote: >>> On Fri, Jul 28, 2017 at 4:07 PM, Jason Gerecke <killertofu@gmail.com> wrote: > >>> #ifdef CONFIG_USB_HID >>> extern bool hid_is_using_usb_driver(struct hid_device *hdev) >>> #else >>> static inline bool hid_is_using_usb_driver(struct hid_device *hdev) >>> { >>> return false; >>> } >>> #endif >>> >>> but is it worth it to avoid the dependency? >>> >>> Arnd >> >> I was thinking something more along the lines of the following since >> the idea of per-transport helper functions was dismissed earlier: >> >> #ifdef CONFIG_USB_HID >> if (hid_is_using_ll_driver(wacom->hdev, &usb_hid_driver)) { > > I would consider that rather ugly, a driver shouldn't really use > #ifdef like this, but you can hide stuff like this in a header. The method > I proposed also has the advantage of avoiding exporting the > usb_hid_driver object. Drivers shouldn't really need to access this, > and wacom_sys.c is the only remaining user of the export. > > Arnd The exports and hid_is_using_ll_driver were actually introduced in the patch immediately preceding the change to wacom_sys.c which triggered this error (making it the "first", not "last" user). That said, after reading through the patch discussion[1] again, I see that my memory is faulty: per-transport functions were *not* dismissed. Rather, a more-generic function that is fed the hid_ll_driver of interest was suggested instead. Given that these exports are liable to cause this same issue for future users, perhaps providing per-transport functions is the better option after all. I could accept either the strict dependency you originally suggested or a modified header, but don't see much reason for the former. Checking if a HID device is using a particular transport shouldn't require that that transport even be available IMO, but that's ultimately not my call... Jiri? Benjamin? [1]: https://patchwork.kernel.org/patch/9815539/ Jason -- 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 Fri, 28 Jul 2017, Jason Gerecke wrote: > I could accept either the strict dependency you originally suggested > or a modified header, but don't see much reason for the former. Well, until any better solution is proposed (in the form of patch), I'm applying Arnd's patch introducing the hard dependency. We can always revisit this later. The dependency is currently simply there, because we do rely on the actual existence of usb_hid_driver structure for the purpose of testing against it. Thanks,
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig index 3cd60f460b61..0a3117cc29e7 100644 --- a/drivers/hid/Kconfig +++ b/drivers/hid/Kconfig @@ -924,7 +924,7 @@ config HID_UDRAW_PS3 config HID_WACOM tristate "Wacom Intuos/Graphire tablet support (USB)" - depends on HID + depends on USB_HID select POWER_SUPPLY select NEW_LEDS select LEDS_CLASS
The driver has gained a compile-time dependency that we should express in Kconfig to avoid this link error: drivers/hid/wacom_sys.o: In function `wacom_parse_and_register': wacom_sys.c:(.text+0x2eec): undefined reference to `usb_hid_driver' Fixes: 09dc28acaec7 ("HID: wacom: Improve generic name generation") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/hid/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)