| Message ID | 20220808031132.10075-3-luke@ljones.dev (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | asus-wmi: add support for ROG XFlow tablet mode | expand |
On Mon, Aug 8, 2022 at 5:12 AM Luke D. Jones <luke@ljones.dev> 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; ... > +}
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 <luke@ljones.dev> 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".
On Tue, Aug 9 2022 at 09:12:37 +0200, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Tue, Aug 9, 2022 at 5:26 AM Luke Jones <luke@ljones.dev> 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.
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
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 <luke@ljones.dev> --- 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(-)