On Tue, Jun 5, 2018 at 8:33 PM, Daniel Drake <drake@xxxxxxxxxxxx> wrote:
> On Sat, Jun 2, 2018 at 8:08 AM, Andy Shevchenko
> <andy.shevchenko@xxxxxxxxx> wrote:
>> On Sat, Jun 2, 2018 at 12:28 AM, Daniel Drake <drake@xxxxxxxxxxxx> wrote:
>>> On Thu, May 31, 2018 at 4:43 AM, Andy Shevchenko
>>> <andy.shevchenko@xxxxxxxxx> wrote:
>>>>> + depends on ASUS_WMI
>>>>
>>>> I'm not sure it's a good idea.
>>>>
>>>> Rather you may use ifdef inside header which also provides a stub for
>>>> the function. But be careful with corner cases, like ASUS_WMI=m &&
>>>> HID_ASUS=y.
>>>
>>> What do you see as wrong with it? The driver already depends on e.g.
>>> LEDS_CLASS for the functionality that it sometimes uses there.
>>>
>>> In my opinion the ideal solution to the corner cases you mention is
>>> the Kconfig "depends on" which is exactly what the patch is
>>> proposing...
>>
>> depends on in this case adds less generic dependency to the more generic module.
>> Thus, we imply that we have more users when dependency is satisfied
>> than otherwise.
>> Which is simply not true.
>>
>> Thus, you add a bulk dependency to unsuspecting users w/o any need.
>
> I'm not familiar with this argument, but I'm happy to work on a
> solution along these lines.
This what comes from Linus and other maintainers who care about kernel
being high modularized.
> Upon initial attempt I can't find a clean approach that avoids the
> corner cases that you mentioned though.
>
> Specifically in the case of HID_ASUS=y and ASUS_WMI=m, we cannot try
> to branch out to the newly exposed asus-wmi functionality since we
> cannot have the kernel binary depend on a module.
>
> Are you envisioning something like this code in hid-asus?
No, no. Not uglifying it with #ifdefs.
Like I said earlier one part of the solution is to add #ifdef into
asus-wmi.h which you include here.
The other part is to carefully crafted Kconfig line for dependency
that allows you to handle that corner case. I don't remember by hart
how to cook that, but we have some examples in kernel.
I would check if I can find one. Though you may try to find yourself as well.
>
> static bool asus_kbd_wmi_led_control_present(struct hid_device *hdev)
> {
> #if defined(CONFIG_HID_ASUS) && defined(CONFIG_ASUS_WMI_MODULE)
> return 0;
> #elif defined(CONFIG_ASUS_WMI) || defined(CONFIG_ASUS_WMI_MODULE)
> u32 value;
> int ret;
>
> ret = asus_wmi_evaluate_method(ASUS_WMI_METHODID_DSTS2,
> ASUS_WMI_DEVID_KBD_BACKLIGHT, 0, &value);
> hid_dbg(hdev, "WMI backlight check: rc %d value %x", ret, value);
>
> if (ret != 0)
> return false;
>
> return !!(value & ASUS_WMI_DSTS_PRESENCE_BIT);
> #endif
> }
--
With Best Regards,
Andy Shevchenko
