Add quirk for ASUS ROG X13 Flow 2-in-1 to enable tablet mode with
lid flip (all screen rotations).
Signed-off-by: Luke D. Jones <[email protected]>
---
drivers/platform/x86/asus-nb-wmi.c | 17 +++++++++-
drivers/platform/x86/asus-wmi.c | 36 ++++++++++++++++++++++
drivers/platform/x86/asus-wmi.h | 1 +
include/linux/platform_data/x86/asus-wmi.h | 1 +
4 files changed, 54 insertions(+), 1 deletion(-)
diff --git a/drivers/platform/x86/asus-nb-wmi.c b/drivers/platform/x86/asus-nb-wmi.c
index 3a93e056c4e1..4aeaac92296f 100644
--- a/drivers/platform/x86/asus-nb-wmi.c
+++ b/drivers/platform/x86/asus-nb-wmi.c
@@ -123,6 +123,11 @@ static struct quirk_entry quirk_asus_use_lid_flip_devid = {
.tablet_switch_mode = asus_wmi_lid_flip_devid,
};
+static struct quirk_entry quirk_asus_tablet_mode = {
+ .wmi_backlight_set_devstate = true,
+ .tablet_switch_mode = asus_wmi_lid_flip_rog_devid,
+};
+
static int dmi_matched(const struct dmi_system_id *dmi)
{
pr_info("Identified laptop model '%s'\n", dmi->ident);
@@ -471,6 +476,15 @@ static const struct dmi_system_id asus_quirks[] = {
},
.driver_data = &quirk_asus_use_lid_flip_devid,
},
+ {
+ .callback = dmi_matched,
+ .ident = "ASUS ROG FLOW X13",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
+ DMI_MATCH(DMI_PRODUCT_NAME, "GV301Q"),
+ },
+ .driver_data = &quirk_asus_tablet_mode,
+ },
{},
};
@@ -574,7 +588,8 @@ static const struct key_entry asus_nb_wmi_keymap[] = {
{ KE_KEY, 0xC4, { KEY_KBDILLUMUP } },
{ KE_KEY, 0xC5, { KEY_KBDILLUMDOWN } },
{ KE_IGNORE, 0xC6, }, /* Ambient Light Sensor notification */
- { KE_KEY, 0xFA, { KEY_PROG2 } }, /* Lid flip action */
+ { KE_KEY, 0xFA, { KEY_PROG2 } }, /* Lid flip action */
+ { KE_KEY, 0xBD, { KEY_PROG2 } }, /* Lid flip action on ROG xflow laptops */
{ KE_END, 0},
};
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index a32e99205697..51610bd6b1c4 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -68,6 +68,7 @@ module_param(fnlock_default, bool, 0444);
#define NOTIFY_KBD_FBM 0x99
#define NOTIFY_KBD_TTP 0xae
#define NOTIFY_LID_FLIP 0xfa
+#define NOTIFY_LID_FLIP_ROG 0xbd
#define ASUS_WMI_FNLOCK_BIOS_DISABLED BIT(0)
@@ -529,6 +530,19 @@ static int asus_wmi_input_init(struct asus_wmi *asus)
pr_err("Error checking for lid-flip: %d\n", result);
}
break;
+ case asus_wmi_lid_flip_rog_devid:
+ result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_LID_FLIP_ROG);
+ if (result < 0)
+ asus->driver->quirks->tablet_switch_mode = asus_wmi_no_tablet_switch;
+ if (result >= 0) {
+ input_set_capability(asus->inputdev, EV_SW, SW_TABLET_MODE);
+ input_report_switch(asus->inputdev, SW_TABLET_MODE, result);
+ } else if (result == -ENODEV) {
+ pr_err("This device has lid-flip-rog quirk but got ENODEV checking it. This is a bug.");
+ } else {
+ pr_err("Error checking for lid-flip: %d\n", result);
+ }
+ break;
}
err = input_register_device(asus->inputdev);
@@ -562,6 +576,16 @@ static void lid_flip_tablet_mode_get_state(struct asus_wmi *asus)
}
}
+static void lid_flip_rog_tablet_mode_get_state(struct asus_wmi *asus)
+{
+ int result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_LID_FLIP_ROG);
+
+ if (result >= 0) {
+ input_report_switch(asus->inputdev, SW_TABLET_MODE, result);
+ input_sync(asus->inputdev);
+ }
+}
+
/* dGPU ********************************************************************/
static int dgpu_disable_check_present(struct asus_wmi *asus)
{
@@ -3106,6 +3130,12 @@ static void asus_wmi_handle_event_code(int code, struct asus_wmi *asus)
return;
}
break;
+ case asus_wmi_lid_flip_rog_devid:
+ if (code == NOTIFY_LID_FLIP_ROG) {
+ lid_flip_rog_tablet_mode_get_state(asus);
+ return;
+ }
+ break;
}
if (asus->fan_boost_mode_available && code == NOTIFY_KBD_FBM) {
@@ -3747,6 +3777,9 @@ static int asus_hotk_resume(struct device *device)
case asus_wmi_lid_flip_devid:
lid_flip_tablet_mode_get_state(asus);
break;
+ case asus_wmi_lid_flip_rog_devid:
+ lid_flip_rog_tablet_mode_get_state(asus);
+ break;
}
return 0;
@@ -3795,6 +3828,9 @@ static int asus_hotk_restore(struct device *device)
case asus_wmi_lid_flip_devid:
lid_flip_tablet_mode_get_state(asus);
break;
+ case asus_wmi_lid_flip_rog_devid:
+ lid_flip_rog_tablet_mode_get_state(asus);
+ break;
}
return 0;
diff --git a/drivers/platform/x86/asus-wmi.h b/drivers/platform/x86/asus-wmi.h
index 413920bad0c6..0187f13d2414 100644
--- a/drivers/platform/x86/asus-wmi.h
+++ b/drivers/platform/x86/asus-wmi.h
@@ -29,6 +29,7 @@ enum asus_wmi_tablet_switch_mode {
asus_wmi_no_tablet_switch,
asus_wmi_kbd_dock_devid,
asus_wmi_lid_flip_devid,
+ asus_wmi_lid_flip_rog_devid,
};
struct quirk_entry {
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index a571b47ff362..d54458431600 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -64,6 +64,7 @@
#define ASUS_WMI_DEVID_PANEL_OD 0x00050019
#define ASUS_WMI_DEVID_CAMERA 0x00060013
#define ASUS_WMI_DEVID_LID_FLIP 0x00060062
+#define ASUS_WMI_DEVID_LID_FLIP_ROG 0x00060077
/* Storage */
#define ASUS_WMI_DEVID_CARDREADER 0x00080013
--
2.37.1
On Mon, Aug 8, 2022 at 5:12 AM Luke D. Jones <[email protected]> wrote:
>
> Add quirk for ASUS ROG X13 Flow 2-in-1 to enable tablet mode with
> lid flip (all screen rotations).
...
> - { KE_KEY, 0xFA, { KEY_PROG2 } }, /* Lid flip action */
> + { KE_KEY, 0xFA, { KEY_PROG2 } }, /* Lid flip action */
Have maintainers asked you about this? Otherwise it is irrelevant change.
...
> + pr_err("This device has lid-flip-rog quirk but got ENODEV checking it. This is a bug.");
dev_err() ?
...
> + pr_err("Error checking for lid-flip: %d\n", result);
Ditto.
...
> +static void lid_flip_rog_tablet_mode_get_state(struct asus_wmi *asus)
> +{
> + int result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_LID_FLIP_ROG);
> +
> + if (result >= 0) {
First of all, it's better to decouple assignment and definition, and
move assignment closer to its user. This is usual pattern.
int result;
result = ...
if (result...)
> + input_report_switch(asus->inputdev, SW_TABLET_MODE, result);
> + input_sync(asus->inputdev);
> + }
Second, it will look better with standard pattern of checking for errors, i.e.
int result;
if (result < 0)
return;
...
> +}
--
With Best Regards,
Andy Shevchenko
Hi Andy,
>
>> - { KE_KEY, 0xFA, { KEY_PROG2 } }, /* Lid flip
>> action */
>> + { KE_KEY, 0xFA, { KEY_PROG2 } }, /* Lid flip action */
>
> Have maintainers asked you about this? Otherwise it is irrelevant
> change.
>
Fixed
> ...
>
>> + pr_err("This device has lid-flip-rog quirk
>> but got ENODEV checking it. This is a bug.");
>
> dev_err() ?
Okay, changed here and in previous patch to match it.
So that I'm clearer on dev_err(), this doesn't do something like exit
the module does it? It's just a more detailed error print?
>
>> +static void lid_flip_rog_tablet_mode_get_state(struct asus_wmi
>> *asus)
>> +{
>> + int result = asus_wmi_get_devstate_simple(asus,
>> ASUS_WMI_DEVID_LID_FLIP_ROG);
>> +
>> + if (result >= 0) {
>
> First of all, it's better to decouple assignment and definition, and
> move assignment closer to its user. This is usual pattern.
>
I don't fully understand why you would want the separation given how
short these two blocks are (I'll change in this and previous patch of
course, I just don't personally understand it).
Cheers,
Luke.
>
On Tue, Aug 9, 2022 at 5:26 AM Luke Jones <[email protected]> wrote:
...
> >> + pr_err("This device has lid-flip-rog quirk
> >> but got ENODEV checking it. This is a bug.");
> >
> > dev_err() ?
>
> Okay, changed here and in previous patch to match it.
>
> So that I'm clearer on dev_err(), this doesn't do something like exit
> the module does it? It's just a more detailed error print?
Yes, it's more specific when the user sees it. The pr_err() is global
and anonymous (you can only point to the driver, and not the instance
of the device bound to it), while dev_err() is device specific and the
user will immediately see which device instance is failing. Yet it's
not a problem for this particular driver, because I don't believe one
may have two, but it's a good coding practice in general.
(Note the last sentence: "good coding practice")
...
> >> +static void lid_flip_rog_tablet_mode_get_state(struct asus_wmi
> >> *asus)
> >> +{
> >> + int result = asus_wmi_get_devstate_simple(asus,
> >> ASUS_WMI_DEVID_LID_FLIP_ROG);
> >> +
> >> + if (result >= 0) {
> >
> > First of all, it's better to decouple assignment and definition, and
> > move assignment closer to its user. This is usual pattern.
>
> I don't fully understand why you would want the separation given how
> short these two blocks are (I'll change in this and previous patch of
> course, I just don't personally understand it).
See above, "good coding practice". Why?
Imagine your code to be in hypothetical v5.10:
int x = foo(param1, param2, ...);
if (x)
return Y;
Now, at v5.12 somebody adds a new feature which touches your code:
int x = foo(param1, param2, ...);
struct bar *baz;
if (we_have_such_feature_disabled)
return Z;
if (x)
return Y;
baz = ...
And then somebody else in v5.13 does another feature:
int x = foo(param1, param2, ...);
struct bar *baz;
if (we_have_such_feature_disabled)
return Z;
/* parameter 1 can be NULL, check it */
if (!param1)
return -EINVAL;
if (x)
return Y;
baz = ...
Do you see now an issue? If you emulate this as a sequence of Git
changes the last one is easily missing subtle detail. That's why "good
coding practice".
--
With Best Regards,
Andy Shevchenko
On Tue, Aug 9 2022 at 09:12:37 +0200, Andy Shevchenko
<[email protected]> wrote:
> On Tue, Aug 9, 2022 at 5:26 AM Luke Jones <[email protected]> wrote:
>
> ...
>
>> >> + pr_err("This device has lid-flip-rog
>> quirk
>> >> but got ENODEV checking it. This is a bug.");
>> >
>> > dev_err() ?
>>
>> Okay, changed here and in previous patch to match it.
>>
>> So that I'm clearer on dev_err(), this doesn't do something like
>> exit
>> the module does it? It's just a more detailed error print?
>
> Yes, it's more specific when the user sees it. The pr_err() is global
> and anonymous (you can only point to the driver, and not the instance
> of the device bound to it), while dev_err() is device specific and the
> user will immediately see which device instance is failing. Yet it's
> not a problem for this particular driver, because I don't believe one
> may have two, but it's a good coding practice in general.
>
> (Note the last sentence: "good coding practice")
>
> ...
>
>> >> +static void lid_flip_rog_tablet_mode_get_state(struct asus_wmi
>> >> *asus)
>> >> +{
>> >> + int result = asus_wmi_get_devstate_simple(asus,
>> >> ASUS_WMI_DEVID_LID_FLIP_ROG);
>> >> +
>> >> + if (result >= 0) {
>> >
>> > First of all, it's better to decouple assignment and definition,
>> and
>> > move assignment closer to its user. This is usual pattern.
>>
>> I don't fully understand why you would want the separation given how
>> short these two blocks are (I'll change in this and previous patch
>> of
>> course, I just don't personally understand it).
>
> See above, "good coding practice". Why?
>
> Imagine your code to be in hypothetical v5.10:
>
> int x = foo(param1, param2, ...);
>
> if (x)
> return Y;
>
>
> Now, at v5.12 somebody adds a new feature which touches your code:
>
> int x = foo(param1, param2, ...);
> struct bar *baz;
>
> if (we_have_such_feature_disabled)
> return Z;
>
> if (x)
> return Y;
>
> baz = ...
>
> And then somebody else in v5.13 does another feature:
>
> int x = foo(param1, param2, ...);
> struct bar *baz;
>
> if (we_have_such_feature_disabled)
> return Z;
>
> /* parameter 1 can be NULL, check it */
> if (!param1)
> return -EINVAL;
>
> if (x)
> return Y;
>
> baz = ...
>
> Do you see now an issue? If you emulate this as a sequence of Git
> changes the last one is easily missing subtle detail. That's why "good
> coding practice".
>
> --
> With Best Regards,
> Andy Shevchenko
That's a great example! Thanks mate, really appreciate it.