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 <[email protected]>
---
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
--
2.9.0
On July 28, 2017 6:18:00 AM PDT, Arnd Bergmann <[email protected]> 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 <[email protected]>
>---
> 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
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
On Fri, Jul 28, 2017 at 4:07 PM, Jason Gerecke <[email protected]> wrote:
> On July 28, 2017 6:18:00 AM PDT, Arnd Bergmann <[email protected]> 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
On Fri, Jul 28, 2017 at 7:18 AM, Arnd Bergmann <[email protected]> wrote:
> On Fri, Jul 28, 2017 at 4:07 PM, Jason Gerecke <[email protected]> wrote:
>> On July 28, 2017 6:18:00 AM PDT, Arnd Bergmann <[email protected]> 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
On Fri, Jul 28, 2017 at 4:24 PM, Jason Gerecke <[email protected]> wrote:
> On Fri, Jul 28, 2017 at 7:18 AM, Arnd Bergmann <[email protected]> wrote:
>> On Fri, Jul 28, 2017 at 4:07 PM, Jason Gerecke <[email protected]> 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
On Fri, Jul 28, 2017 at 7:29 AM, Arnd Bergmann <[email protected]> wrote:
> On Fri, Jul 28, 2017 at 4:24 PM, Jason Gerecke <[email protected]> wrote:
>> On Fri, Jul 28, 2017 at 7:18 AM, Arnd Bergmann <[email protected]> wrote:
>>> On Fri, Jul 28, 2017 at 4:07 PM, Jason Gerecke <[email protected]> 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
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,
--
Jiri Kosina
SUSE Labs