| Message ID | 20180522205542.11991-2-drake@endlessm.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Andy Shevchenko |
| Headers | show |
On Tue, May 22, 2018 at 11:55 PM, Daniel Drake <drake@endlessm.com> wrote: > The Asus GL502VSK has the same 0B05:1837 keyboard as we've seen in > several Republic of Gamers laptops. > > However, in this model, the keybard backlight control exposed by hid-asus > has no effect on the keyboard backlight. Instead, the keyboard > backlight is correctly driven by asus-wmi. > > With two keyboard backlight devices available (and only the acer-wmi > one working), GNOME is picking the wrong one to drive in the UI. > > Avoid this problem by not creating the backlight interface when we > detect a WMI-driven keyboard backlight. > > We have also tested Asus GL702VMK which does have the hid-asus > backlight present, and it still works fine with this patch (WMI method > call returns UNSUPPORTED_METHOD). > > Signed-off-by: Daniel Drake <drake@endlessm.com> > --- > drivers/hid/Kconfig | 1 + > drivers/hid/hid-asus.c | 24 +++++++++++++++++++++++- > 2 files changed, 24 insertions(+), 1 deletion(-) > > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig > index 0000434a1fbd..3c0d461bb830 100644 > --- a/drivers/hid/Kconfig > +++ b/drivers/hid/Kconfig > @@ -149,6 +149,7 @@ config HID_APPLEIR > config HID_ASUS > tristate "Asus" > depends on LEDS_CLASS > + 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. > ---help--- > Support for Asus notebook built-in keyboard and touchpad via i2c, and > the Asus Republic of Gamers laptop keyboard special keys. > diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c > index 88a5672f42cd..c42af8511b38 100644 > --- a/drivers/hid/hid-asus.c > +++ b/drivers/hid/hid-asus.c > @@ -26,6 +26,7 @@ > * any later version. > */ > > +#include <linux/asus-wmi.h> > #include <linux/dmi.h> > #include <linux/hid.h> > #include <linux/module.h> > @@ -349,6 +350,25 @@ static void asus_kbd_backlight_work(struct work_struct *work) > hid_err(led->hdev, "Asus failed to set keyboard backlight: %d\n", ret); > } > > +/* WMI-based keyboard backlight LED control (via asus-wmi driver) takes > + * precedence. We only activate HID-based backlight control when the > + * WMI control is not available. > + */ > +static bool asus_kbd_wmi_led_control_present(struct hid_device *hdev) > +{ > + 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); > +} > + > static int asus_kbd_register_leds(struct hid_device *hdev) > { > struct asus_drvdata *drvdata = hid_get_drvdata(hdev); > @@ -436,7 +456,9 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi) > > drvdata->input = input; > > - if (drvdata->enable_backlight && asus_kbd_register_leds(hdev)) > + if (drvdata->enable_backlight && > + !asus_kbd_wmi_led_control_present(hdev) && > + asus_kbd_register_leds(hdev)) > hid_warn(hdev, "Failed to initialize backlight.\n"); > > return 0; > -- > 2.17.0 >
On Thu, May 31, 2018 at 12:43 PM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Tue, May 22, 2018 at 11:55 PM, Daniel Drake <drake@endlessm.com> wrote: >> The Asus GL502VSK has the same 0B05:1837 keyboard as we've seen in >> several Republic of Gamers laptops. >> >> However, in this model, the keybard backlight control exposed by hid-asus >> has no effect on the keyboard backlight. Instead, the keyboard >> backlight is correctly driven by asus-wmi. >> >> With two keyboard backlight devices available (and only the acer-wmi >> one working), GNOME is picking the wrong one to drive in the UI. >> >> Avoid this problem by not creating the backlight interface when we >> detect a WMI-driven keyboard backlight. >> >> We have also tested Asus GL702VMK which does have the hid-asus >> backlight present, and it still works fine with this patch (WMI method >> call returns UNSUPPORTED_METHOD). >> >> Signed-off-by: Daniel Drake <drake@endlessm.com> >> --- >> drivers/hid/Kconfig | 1 + >> drivers/hid/hid-asus.c | 24 +++++++++++++++++++++++- >> 2 files changed, 24 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig >> index 0000434a1fbd..3c0d461bb830 100644 >> --- a/drivers/hid/Kconfig >> +++ b/drivers/hid/Kconfig >> @@ -149,6 +149,7 @@ config HID_APPLEIR >> config HID_ASUS >> tristate "Asus" >> depends on LEDS_CLASS > >> + 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. I wonder if the report descriptors are not enough to provide the information that the keyboard backlights are driven by wmi instead of plain HID (or if there is any register that tells us that the backlights are not handled by HID). If not, then I have nothing against the approach. Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> Cheers, Benjamin > >> ---help--- >> Support for Asus notebook built-in keyboard and touchpad via i2c, and >> the Asus Republic of Gamers laptop keyboard special keys. >> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c >> index 88a5672f42cd..c42af8511b38 100644 >> --- a/drivers/hid/hid-asus.c >> +++ b/drivers/hid/hid-asus.c >> @@ -26,6 +26,7 @@ >> * any later version. >> */ >> >> +#include <linux/asus-wmi.h> >> #include <linux/dmi.h> >> #include <linux/hid.h> >> #include <linux/module.h> >> @@ -349,6 +350,25 @@ static void asus_kbd_backlight_work(struct work_struct *work) >> hid_err(led->hdev, "Asus failed to set keyboard backlight: %d\n", ret); >> } >> >> +/* WMI-based keyboard backlight LED control (via asus-wmi driver) takes >> + * precedence. We only activate HID-based backlight control when the >> + * WMI control is not available. >> + */ >> +static bool asus_kbd_wmi_led_control_present(struct hid_device *hdev) >> +{ >> + 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); >> +} >> + >> static int asus_kbd_register_leds(struct hid_device *hdev) >> { >> struct asus_drvdata *drvdata = hid_get_drvdata(hdev); >> @@ -436,7 +456,9 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi) >> >> drvdata->input = input; >> >> - if (drvdata->enable_backlight && asus_kbd_register_leds(hdev)) >> + if (drvdata->enable_backlight && >> + !asus_kbd_wmi_led_control_present(hdev) && >> + asus_kbd_register_leds(hdev)) >> hid_warn(hdev, "Failed to initialize backlight.\n"); >> >> return 0; >> -- >> 2.17.0 >> > > > > -- > With Best Regards, > Andy Shevchenko
On Thu, May 31, 2018 at 4:43 AM, Andy Shevchenko <andy.shevchenko@gmail.com> 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... Daniel
On Sat, Jun 2, 2018 at 12:28 AM, Daniel Drake <drake@endlessm.com> wrote: > On Thu, May 31, 2018 at 4:43 AM, Andy Shevchenko > <andy.shevchenko@gmail.com> 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.
On Sat, Jun 2, 2018 at 8:08 AM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Sat, Jun 2, 2018 at 12:28 AM, Daniel Drake <drake@endlessm.com> wrote: >> On Thu, May 31, 2018 at 4:43 AM, Andy Shevchenko >> <andy.shevchenko@gmail.com> 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. 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? 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 } Thanks Daniel
On Tue, Jun 5, 2018 at 8:33 PM, Daniel Drake <drake@endlessm.com> wrote: > On Sat, Jun 2, 2018 at 8:08 AM, Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: >> On Sat, Jun 2, 2018 at 12:28 AM, Daniel Drake <drake@endlessm.com> wrote: >>> On Thu, May 31, 2018 at 4:43 AM, Andy Shevchenko >>> <andy.shevchenko@gmail.com> 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 > }
On Tue, Jun 5, 2018 at 3:16 PM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > 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. I have looked through many Kconfig files under drivers as examples, and I can't see any way to write the "depends on" line to enforce the specific required logic of disallowing HID_ASUS=y combined with ASUS_WMI=m, other than the approach that I already put in the patch: config HID_ASUS tristate "Asus" depends on LEDS_CLASS depends on ASUS_WMI The alternative is to use "select", but that also deviates away from your suggestion, and I don't think it's right. Plus "select ASUS_WMI" causes this circular dependency: drivers/platform/x86/Kconfig:627:error: recursive dependency detected! drivers/platform/x86/Kconfig:627: symbol ASUS_WMI is selected by HID_ASUS drivers/hid/Kconfig:149: symbol HID_ASUS depends on LEDS_CLASS drivers/leds/Kconfig:16: symbol LEDS_CLASS is selected by ASUS_WMI For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" So if you do prefer "select" we would have to make it like this: config HID_ASUS tristate "Asus" select LEDS_CLASS select ASUS_WMI Thanks for your input Daniel
Hi Andy, On Thu, Jun 14, 2018 at 12:18 PM, Daniel Drake <drake@endlessm.com> wrote: > On Tue, Jun 5, 2018 at 3:16 PM, Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: >> 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. > > I have looked through many Kconfig files under drivers as examples, > and I can't see any way to write the "depends on" line to enforce the > specific required logic of disallowing HID_ASUS=y combined with > ASUS_WMI=m, other than the approach that I already put in the patch: > > config HID_ASUS > tristate "Asus" > depends on LEDS_CLASS > depends on ASUS_WMI > > The alternative is to use "select", but that also deviates away from > your suggestion, and I don't think it's right. Plus "select ASUS_WMI" > causes this circular dependency: > > drivers/platform/x86/Kconfig:627:error: recursive dependency detected! > drivers/platform/x86/Kconfig:627: symbol ASUS_WMI is selected by HID_ASUS > drivers/hid/Kconfig:149: symbol HID_ASUS depends on LEDS_CLASS > drivers/leds/Kconfig:16: symbol LEDS_CLASS is selected by ASUS_WMI > For a resolution refer to Documentation/kbuild/kconfig-language.txt > subsection "Kconfig recursive dependency limitations" > > So if you do prefer "select" we would have to make it like this: > > config HID_ASUS > tristate "Asus" > select LEDS_CLASS > select ASUS_WMI This fix is still blocked on your feedback/review. Any clarifications? Thanks Daniel
On Tue, Jul 17, 2018 at 5:11 PM, Daniel Drake <drake@endlessm.com> wrote: > Hi Andy, > > On Thu, Jun 14, 2018 at 12:18 PM, Daniel Drake <drake@endlessm.com> wrote: >> On Tue, Jun 5, 2018 at 3:16 PM, Andy Shevchenko >> <andy.shevchenko@gmail.com> wrote: >>> 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. >> >> I have looked through many Kconfig files under drivers as examples, >> and I can't see any way to write the "depends on" line to enforce the >> specific required logic of disallowing HID_ASUS=y combined with >> ASUS_WMI=m, other than the approach that I already put in the patch: >> >> config HID_ASUS >> tristate "Asus" >> depends on LEDS_CLASS >> depends on ASUS_WMI >> >> The alternative is to use "select", but that also deviates away from >> your suggestion, and I don't think it's right. Plus "select ASUS_WMI" >> causes this circular dependency: >> >> drivers/platform/x86/Kconfig:627:error: recursive dependency detected! >> drivers/platform/x86/Kconfig:627: symbol ASUS_WMI is selected by HID_ASUS >> drivers/hid/Kconfig:149: symbol HID_ASUS depends on LEDS_CLASS >> drivers/leds/Kconfig:16: symbol LEDS_CLASS is selected by ASUS_WMI >> For a resolution refer to Documentation/kbuild/kconfig-language.txt >> subsection "Kconfig recursive dependency limitations" >> >> So if you do prefer "select" we would have to make it like this: >> >> config HID_ASUS >> tristate "Asus" >> select LEDS_CLASS >> select ASUS_WMI > > This fix is still blocked on your feedback/review. Any clarifications? I guess you are looking something like below with corresponding header that provides stubs in case it's not enabled. depends on CHARGER_BQ24190=y || (CHARGER_BQ24190=m && m)
On Mon, Jul 30, 2018 at 1:06 PM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > I guess you are looking something like below with corresponding header > that provides stubs in case it's not enabled. > > depends on CHARGER_BQ24190=y || (CHARGER_BQ24190=m && m) I can make config HID_ASUS depends on ASUS_WMI=y || (ASUS_WMI=m && m) and the corner case you raise (HID_ASUS=y and ASUS_WMI=m) can not be selected. And there is no need to add any stub headers as far as I can see, but I must admit I still don't understand why this type of dependency statement is preferred over a simple "depends on ASUS_WMI". Am I missing something? Thanks Daniel
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig index 0000434a1fbd..3c0d461bb830 100644 --- a/drivers/hid/Kconfig +++ b/drivers/hid/Kconfig @@ -149,6 +149,7 @@ config HID_APPLEIR config HID_ASUS tristate "Asus" depends on LEDS_CLASS + depends on ASUS_WMI ---help--- Support for Asus notebook built-in keyboard and touchpad via i2c, and the Asus Republic of Gamers laptop keyboard special keys. diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c index 88a5672f42cd..c42af8511b38 100644 --- a/drivers/hid/hid-asus.c +++ b/drivers/hid/hid-asus.c @@ -26,6 +26,7 @@ * any later version. */ +#include <linux/asus-wmi.h> #include <linux/dmi.h> #include <linux/hid.h> #include <linux/module.h> @@ -349,6 +350,25 @@ static void asus_kbd_backlight_work(struct work_struct *work) hid_err(led->hdev, "Asus failed to set keyboard backlight: %d\n", ret); } +/* WMI-based keyboard backlight LED control (via asus-wmi driver) takes + * precedence. We only activate HID-based backlight control when the + * WMI control is not available. + */ +static bool asus_kbd_wmi_led_control_present(struct hid_device *hdev) +{ + 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); +} + static int asus_kbd_register_leds(struct hid_device *hdev) { struct asus_drvdata *drvdata = hid_get_drvdata(hdev); @@ -436,7 +456,9 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi) drvdata->input = input; - if (drvdata->enable_backlight && asus_kbd_register_leds(hdev)) + if (drvdata->enable_backlight && + !asus_kbd_wmi_led_control_present(hdev) && + asus_kbd_register_leds(hdev)) hid_warn(hdev, "Failed to initialize backlight.\n"); return 0;
The Asus GL502VSK has the same 0B05:1837 keyboard as we've seen in several Republic of Gamers laptops. However, in this model, the keybard backlight control exposed by hid-asus has no effect on the keyboard backlight. Instead, the keyboard backlight is correctly driven by asus-wmi. With two keyboard backlight devices available (and only the acer-wmi one working), GNOME is picking the wrong one to drive in the UI. Avoid this problem by not creating the backlight interface when we detect a WMI-driven keyboard backlight. We have also tested Asus GL702VMK which does have the hid-asus backlight present, and it still works fine with this patch (WMI method call returns UNSUPPORTED_METHOD). Signed-off-by: Daniel Drake <drake@endlessm.com> --- drivers/hid/Kconfig | 1 + drivers/hid/hid-asus.c | 24 +++++++++++++++++++++++- 2 files changed, 24 insertions(+), 1 deletion(-)