diff mbox series

[v2,1/2] asus-wmi: Adjust tablet/lidflip handling to use enum

Message ID 20220808031132.10075-2-luke@ljones.dev (mailing list archive)
State Superseded, archived
Headers show
Series asus-wmi: add support for ROG XFlow tablet mode | expand

Commit Message

Luke D. Jones Aug. 8, 2022, 3:11 a.m. UTC 
Due to multiple types of tablet/lidflip, the existing code for
handlign these events is refactored to use an enum for each type.

Signed-off-by: Luke D. Jones <luke@ljones.dev>
---
 drivers/platform/x86/asus-nb-wmi.c | 13 +++-----
 drivers/platform/x86/asus-wmi.c    | 53 +++++++++++++++++++++---------
 drivers/platform/x86/asus-wmi.h    |  9 +++--
 3 files changed, 49 insertions(+), 26 deletions(-)

Comments

Andy Shevchenko Aug. 8, 2022, 3:48 p.m. UTC | #1 
On Mon, Aug 8, 2022 at 5:12 AM Luke D. Jones <luke@ljones.dev> wrote:
>
> Due to multiple types of tablet/lidflip, the existing code for
> handlign these events is refactored to use an enum for each type.

handling

Can you run a spell checker for your commit messages?

...

To the switch-cases, please add a "default" case to each of them.
Hans de Goede Aug. 8, 2022, 4:13 p.m. UTC | #2 
Hi,

On 8/8/22 17:48, Andy Shevchenko wrote:
> On Mon, Aug 8, 2022 at 5:12 AM Luke D. Jones <luke@ljones.dev> wrote:
>>
>> Due to multiple types of tablet/lidflip, the existing code for
>> handlign these events is refactored to use an enum for each type.
> 
> handling
> 
> Can you run a spell checker for your commit messages?
> 
> ...
> 
> To the switch-cases, please add a "default" case to each of them.

The switch-cases are on an enum type, so adding a default is
not necessary and adding one will actually loose the useful
compiler warning about unhandled enum values.

Regards,

Hans
Andy Shevchenko Aug. 8, 2022, 4:24 p.m. UTC | #3 
On Mon, Aug 8, 2022 at 6:13 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 8/8/22 17:48, Andy Shevchenko wrote:
> > On Mon, Aug 8, 2022 at 5:12 AM Luke D. Jones <luke@ljones.dev> wrote:

...

> > To the switch-cases, please add a "default" case to each of them.
>
> The switch-cases are on an enum type, so adding a default is
> not necessary and adding one will actually loose the useful
> compiler warning about unhandled enum values.

It's good if you can cover all enum values, which usually you can't.
enum according to the standard should be located in the type that is
enough to keep it and be compatible to a char. This means that the
code somewhere else may assign anything to enum (actually enum values
are type of int) and without default you can't see the difference here
and the compiler probably will be happy. That said, I doubt the
usefulness of such a warning. But it's up to you.
Hans de Goede Aug. 8, 2022, 5:18 p.m. UTC | #4 
Hi,

On 8/8/22 18:24, Andy Shevchenko wrote:
> On Mon, Aug 8, 2022 at 6:13 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 8/8/22 17:48, Andy Shevchenko wrote:
>>> On Mon, Aug 8, 2022 at 5:12 AM Luke D. Jones <luke@ljones.dev> wrote:
> 
> ...
> 
>>> To the switch-cases, please add a "default" case to each of them.
>>
>> The switch-cases are on an enum type, so adding a default is
>> not necessary and adding one will actually loose the useful
>> compiler warning about unhandled enum values.
> 
> It's good if you can cover all enum values, which usually you can't.
> enum according to the standard should be located in the type that is
> enough to keep it and be compatible to a char. This means that the
> code somewhere else may assign anything to enum (actually enum values
> are type of int) and without default you can't see the difference here
> and the compiler probably will be happy. That said, I doubt the
> usefulness of such a warning. But it's up to you.

I would prefer to not introduce a default label in this case; in
the unexpected case that the value gets set out of the enum range
then the switch-case will be a no-op and any added default would
also be a no-op, so adding a default gains us nothing.

Regards,

Hans
diff mbox series

Patch

diff --git a/drivers/platform/x86/asus-nb-wmi.c b/drivers/platform/x86/asus-nb-wmi.c
index a81dc4b191b7..3a93e056c4e1 100644
--- a/drivers/platform/x86/asus-nb-wmi.c
+++ b/drivers/platform/x86/asus-nb-wmi.c
@@ -115,12 +115,12 @@  static struct quirk_entry quirk_asus_forceals = {
 };
 
 static struct quirk_entry quirk_asus_use_kbd_dock_devid = {
-	.use_kbd_dock_devid = true,
+	.tablet_switch_mode = asus_wmi_kbd_dock_devid,
 };
 
 static struct quirk_entry quirk_asus_use_lid_flip_devid = {
 	.wmi_backlight_set_devstate = true,
-	.use_lid_flip_devid = true,
+	.tablet_switch_mode = asus_wmi_lid_flip_devid,
 };
 
 static int dmi_matched(const struct dmi_system_id *dmi)
@@ -492,16 +492,13 @@  static void asus_nb_wmi_quirks(struct asus_wmi_driver *driver)
 
 	switch (tablet_mode_sw) {
 	case 0:
-		quirks->use_kbd_dock_devid = false;
-		quirks->use_lid_flip_devid = false;
+		quirks->tablet_switch_mode = asus_wmi_no_tablet_switch;
 		break;
 	case 1:
-		quirks->use_kbd_dock_devid = true;
-		quirks->use_lid_flip_devid = false;
+		quirks->tablet_switch_mode = asus_wmi_kbd_dock_devid;
 		break;
 	case 2:
-		quirks->use_kbd_dock_devid = false;
-		quirks->use_lid_flip_devid = true;
+		quirks->tablet_switch_mode = asus_wmi_lid_flip_devid;
 		break;
 	}
 
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 0e7fbed8a50d..a32e99205697 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -504,7 +504,10 @@  static int asus_wmi_input_init(struct asus_wmi *asus)
 	if (err)
 		goto err_free_dev;
 
-	if (asus->driver->quirks->use_kbd_dock_devid) {
+	switch (asus->driver->quirks->tablet_switch_mode) {
+	case asus_wmi_no_tablet_switch:
+		break;
+	case asus_wmi_kbd_dock_devid:
 		result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_KBD_DOCK);
 		if (result >= 0) {
 			input_set_capability(asus->inputdev, EV_SW, SW_TABLET_MODE);
@@ -512,12 +515,11 @@  static int asus_wmi_input_init(struct asus_wmi *asus)
 		} else if (result != -ENODEV) {
 			pr_err("Error checking for keyboard-dock: %d\n", result);
 		}
-	}
-
-	if (asus->driver->quirks->use_lid_flip_devid) {
+		break;
+	case asus_wmi_lid_flip_devid:
 		result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_LID_FLIP);
 		if (result < 0)
-			asus->driver->quirks->use_lid_flip_devid = 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);
@@ -526,6 +528,7 @@  static int asus_wmi_input_init(struct asus_wmi *asus)
 		} else {
 			pr_err("Error checking for lid-flip: %d\n", result);
 		}
+		break;
 	}
 
 	err = input_register_device(asus->inputdev);
@@ -3083,20 +3086,26 @@  static void asus_wmi_handle_event_code(int code, struct asus_wmi *asus)
 		return;
 	}
 
-	if (asus->driver->quirks->use_kbd_dock_devid && code == NOTIFY_KBD_DOCK_CHANGE) {
-		result = asus_wmi_get_devstate_simple(asus,
-						      ASUS_WMI_DEVID_KBD_DOCK);
+	switch (asus->driver->quirks->tablet_switch_mode) {
+	case asus_wmi_no_tablet_switch:
+		break;
+	case asus_wmi_kbd_dock_devid:
+		if (code == NOTIFY_KBD_DOCK_CHANGE) {
+			result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_KBD_DOCK);
 		if (result >= 0) {
 			input_report_switch(asus->inputdev, SW_TABLET_MODE,
-					    !result);
+						!result);
 			input_sync(asus->inputdev);
 		}
 		return;
-	}
-
-	if (asus->driver->quirks->use_lid_flip_devid && code == NOTIFY_LID_FLIP) {
-		lid_flip_tablet_mode_get_state(asus);
-		return;
+		}
+		break;
+	case asus_wmi_lid_flip_devid:
+		if (code == NOTIFY_LID_FLIP) {
+			lid_flip_tablet_mode_get_state(asus);
+			return;
+		}
+		break;
 	}
 
 	if (asus->fan_boost_mode_available && code == NOTIFY_KBD_FBM) {
@@ -3731,8 +3740,14 @@  static int asus_hotk_resume(struct device *device)
 	if (asus_wmi_has_fnlock_key(asus))
 		asus_wmi_fnlock_update(asus);
 
-	if (asus->driver->quirks->use_lid_flip_devid)
+	switch (asus->driver->quirks->tablet_switch_mode) {
+	case asus_wmi_no_tablet_switch:
+	case asus_wmi_kbd_dock_devid:
+		break;
+	case asus_wmi_lid_flip_devid:
 		lid_flip_tablet_mode_get_state(asus);
+		break;
+	}
 
 	return 0;
 }
@@ -3773,8 +3788,14 @@  static int asus_hotk_restore(struct device *device)
 	if (asus_wmi_has_fnlock_key(asus))
 		asus_wmi_fnlock_update(asus);
 
-	if (asus->driver->quirks->use_lid_flip_devid)
+	switch (asus->driver->quirks->tablet_switch_mode) {
+	case asus_wmi_no_tablet_switch:
+	case asus_wmi_kbd_dock_devid:
+		break;
+	case asus_wmi_lid_flip_devid:
 		lid_flip_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 b302415bf1d9..413920bad0c6 100644
--- a/drivers/platform/x86/asus-wmi.h
+++ b/drivers/platform/x86/asus-wmi.h
@@ -25,6 +25,12 @@  struct module;
 struct key_entry;
 struct asus_wmi;
 
+enum asus_wmi_tablet_switch_mode {
+	asus_wmi_no_tablet_switch,
+	asus_wmi_kbd_dock_devid,
+	asus_wmi_lid_flip_devid,
+};
+
 struct quirk_entry {
 	bool hotplug_wireless;
 	bool scalar_panel_brightness;
@@ -33,8 +39,7 @@  struct quirk_entry {
 	bool wmi_backlight_native;
 	bool wmi_backlight_set_devstate;
 	bool wmi_force_als_set;
-	bool use_kbd_dock_devid;
-	bool use_lid_flip_devid;
+	enum asus_wmi_tablet_switch_mode tablet_switch_mode;
 	int wapf;
 	/*
 	 * For machines with AMD graphic chips, it will send out WMI event