All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 0/2] asus-wmi: add support for ROG XFlow tablet mode
@ 2022-08-09  3:30 Luke D. Jones
  2022-08-09  3:30 ` [PATCH v3 1/2] asus-wmi: Adjust tablet/lidflip handling to use enum Luke D. Jones
  2022-08-09  3:30 ` [PATCH v3 2/2] asus-wmi: Add support for ROG X13 tablet mode Luke D. Jones
  0 siblings, 2 replies; 7+ messages in thread
From: Luke D. Jones @ 2022-08-09  3:30 UTC (permalink / raw)
  To: hdegoede
  Cc: andy.shevchenko, platform-driver-x86, linux-kernel, Luke D. Jones

This patch series refactors a part of the tablet/lid-flip code to
use an enum to set the different behaviours, and adds support for
the ASUS ROG XFlow (X13) 2-in-1 laptop.

Changelog:
- V3:
  + Use dev_err() in place of pr_err() in asus_wmi_input_init()
  + Adjust declaration of variables vs instantiation to match expected pattern
- V2:
  + Refactor the base handling paths of tablet modes to use enum
  + Add support for ROG XFlow using the refactored code

This obsoletes a previous patch I submitted:
- https://lkml.org/lkml/2022/8/3/94

Luke D. Jones (2):
  asus-wmi: Adjust tablet/lidflip handling to use enum
  asus-wmi: Add support for ROG X13 tablet mode

 drivers/platform/x86/asus-nb-wmi.c         |  28 ++++--
 drivers/platform/x86/asus-wmi.c            | 104 ++++++++++++++++-----
 drivers/platform/x86/asus-wmi.h            |  10 +-
 include/linux/platform_data/x86/asus-wmi.h |   1 +
 4 files changed, 112 insertions(+), 31 deletions(-)

-- 
2.37.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v3 1/2] asus-wmi: Adjust tablet/lidflip handling to use enum
  2022-08-09  3:30 [PATCH v3 0/2] asus-wmi: add support for ROG XFlow tablet mode Luke D. Jones
@ 2022-08-09  3:30 ` Luke D. Jones
  2022-08-09  8:37   ` Andy Shevchenko
  2022-08-09  3:30 ` [PATCH v3 2/2] asus-wmi: Add support for ROG X13 tablet mode Luke D. Jones
  1 sibling, 1 reply; 7+ messages in thread
From: Luke D. Jones @ 2022-08-09  3:30 UTC (permalink / raw)
  To: hdegoede
  Cc: andy.shevchenko, platform-driver-x86, linux-kernel, Luke D. Jones

Due to multiple types of tablet/lidflip, the existing code for
handling 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    | 67 ++++++++++++++++++++----------
 drivers/platform/x86/asus-wmi.h    |  9 +++-
 3 files changed, 58 insertions(+), 31 deletions(-)

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 c5fa21370481..029c26a218e1 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -507,8 +507,11 @@ static bool asus_wmi_dev_is_present(struct asus_wmi *asus, u32 dev_id)
 
 static int asus_wmi_input_init(struct asus_wmi *asus)
 {
+	struct device *dev;
 	int err, result;
 
+	dev = &asus->platform_device->dev;
+
 	asus->inputdev = input_allocate_device();
 	if (!asus->inputdev)
 		return -ENOMEM;
@@ -516,35 +519,38 @@ static int asus_wmi_input_init(struct asus_wmi *asus)
 	asus->inputdev->name = asus->driver->input_name;
 	asus->inputdev->phys = asus->driver->input_phys;
 	asus->inputdev->id.bustype = BUS_HOST;
-	asus->inputdev->dev.parent = &asus->platform_device->dev;
+	asus->inputdev->dev.parent = dev;
 	set_bit(EV_REP, asus->inputdev->evbit);
 
 	err = sparse_keymap_setup(asus->inputdev, asus->driver->keymap, NULL);
 	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);
 			input_report_switch(asus->inputdev, SW_TABLET_MODE, !result);
 		} else if (result != -ENODEV) {
-			pr_err("Error checking for keyboard-dock: %d\n", result);
+			dev_err(dev, "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);
 		} else if (result == -ENODEV) {
-			pr_err("This device has lid_flip quirk but got ENODEV checking it. This is a bug.");
+			dev_err(dev, "This device has lid_flip quirk but got ENODEV checking it. This is a bug.");
 		} else {
-			pr_err("Error checking for lid-flip: %d\n", result);
+			dev_err(dev, "Error checking for lid-flip: %d\n", result);
 		}
+		break;
 	}
 
 	err = input_register_device(asus->inputdev);
@@ -570,8 +576,9 @@ static void asus_wmi_input_exit(struct asus_wmi *asus)
 
 static void lid_flip_tablet_mode_get_state(struct asus_wmi *asus)
 {
-	int result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_LID_FLIP);
+	int result;
 
+	result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_LID_FLIP);
 	if (result >= 0) {
 		input_report_switch(asus->inputdev, SW_TABLET_MODE, result);
 		input_sync(asus->inputdev);
@@ -3404,20 +3411,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) {
@@ -4085,8 +4098,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;
 }
@@ -4127,8 +4146,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
-- 
2.37.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v3 2/2] asus-wmi: Add support for ROG X13 tablet mode
  2022-08-09  3:30 [PATCH v3 0/2] asus-wmi: add support for ROG XFlow tablet mode Luke D. Jones
  2022-08-09  3:30 ` [PATCH v3 1/2] asus-wmi: Adjust tablet/lidflip handling to use enum Luke D. Jones
@ 2022-08-09  3:30 ` Luke D. Jones
  2022-08-09  8:40   ` Andy Shevchenko
  1 sibling, 1 reply; 7+ messages in thread
From: Luke D. Jones @ 2022-08-09  3:30 UTC (permalink / raw)
  To: hdegoede
  Cc: andy.shevchenko, platform-driver-x86, linux-kernel, Luke D. Jones

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         | 15 +++++++++
 drivers/platform/x86/asus-wmi.c            | 37 ++++++++++++++++++++++
 drivers/platform/x86/asus-wmi.h            |  1 +
 include/linux/platform_data/x86/asus-wmi.h |  1 +
 4 files changed, 54 insertions(+)

diff --git a/drivers/platform/x86/asus-nb-wmi.c b/drivers/platform/x86/asus-nb-wmi.c
index 3a93e056c4e1..d4cc6afc1861 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,
+	},
 	{},
 };
 
@@ -575,6 +589,7 @@ static const struct key_entry asus_nb_wmi_keymap[] = {
 	{ KE_KEY, 0xC5, { KEY_KBDILLUMDOWN } },
 	{ KE_IGNORE, 0xC6, },  /* Ambient Light Sensor notification */
 	{ 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 029c26a218e1..8b8ab48a644e 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -69,6 +69,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)
 
@@ -551,6 +552,19 @@ static int asus_wmi_input_init(struct asus_wmi *asus)
 			dev_err(dev, "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) {
+			dev_err(dev, "This device has lid-flip-rog quirk but got ENODEV checking it. This is a bug.");
+		} else {
+			dev_err(dev, "Error checking for lid-flip: %d\n", result);
+		}
+		break;
 	}
 
 	err = input_register_device(asus->inputdev);
@@ -585,6 +599,17 @@ 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;
+
+	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)
 {
@@ -3431,6 +3456,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) {
@@ -4105,6 +4136,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;
@@ -4153,6 +4187,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 3faeb98f6ea9..69c5308ed4c5 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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 1/2] asus-wmi: Adjust tablet/lidflip handling to use enum
  2022-08-09  3:30 ` [PATCH v3 1/2] asus-wmi: Adjust tablet/lidflip handling to use enum Luke D. Jones
@ 2022-08-09  8:37   ` Andy Shevchenko
  0 siblings, 0 replies; 7+ messages in thread
From: Andy Shevchenko @ 2022-08-09  8:37 UTC (permalink / raw)
  To: Luke D. Jones; +Cc: Hans de Goede, Platform Driver, Linux Kernel Mailing List

On Tue, Aug 9, 2022 at 5:30 AM Luke D. Jones <luke@ljones.dev> wrote:
>
> Due to multiple types of tablet/lidflip, the existing code for
> handling these events is refactored to use an enum for each type.

...

>  static int asus_wmi_input_init(struct asus_wmi *asus)
>  {
> +       struct device *dev;
>         int err, result;
>
> +       dev = &asus->platform_device->dev;
> +

While the discussed pattern of splitting assignments is a good
practice, for some cases we don't do it when we rely on the guarantee
by the callers that some of the stuff won't be problematic. Here the
device is part of the platform device and can't be NULL, there is no
point to split definition and assignment (and you may find plenty
examples in the kernel), so

  struct device *dev = &asus->platform_device->dev;

is better to have as it's guaranteed not to be NULL and since that we
don't check it in the code anyway.


...

>                         input_report_switch(asus->inputdev, SW_TABLET_MODE,
> -                                           !result);
> +                                               !result);

Irrelevant change.

...

It also seems you switched to dev_err() here for the pr_err() that
aren't related to the change, either split that to a separate patch,
or don't do it right now. I.o.w. use dev_err() only in the lines your
change touches, otherwise it's irrelevant.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 2/2] asus-wmi: Add support for ROG X13 tablet mode
  2022-08-09  3:30 ` [PATCH v3 2/2] asus-wmi: Add support for ROG X13 tablet mode Luke D. Jones
@ 2022-08-09  8:40   ` Andy Shevchenko
  2022-08-09  8:46     ` Hans de Goede
  0 siblings, 1 reply; 7+ messages in thread
From: Andy Shevchenko @ 2022-08-09  8:40 UTC (permalink / raw)
  To: Luke D. Jones; +Cc: Hans de Goede, Platform Driver, Linux Kernel Mailing List

On Tue, Aug 9, 2022 at 5:31 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_IGNORE, 0xC6, },  /* Ambient Light Sensor notification */
>         { KE_KEY, 0xFA, { KEY_PROG2 } },           /* Lid flip action */
> +       { KE_KEY, 0xBD, { KEY_PROG2 } }, /* Lid flip action on ROG xflow laptops */

Shouldn't you keep it sorted by value?

...

>  #define NOTIFY_KBD_FBM                 0x99
>  #define NOTIFY_KBD_TTP                 0xae
>  #define NOTIFY_LID_FLIP                        0xfa
> +#define NOTIFY_LID_FLIP_ROG            0xbd

Ditto.

...

> +static void lid_flip_rog_tablet_mode_get_state(struct asus_wmi *asus)
> +{
> +       int result;
> +
> +       result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_LID_FLIP_ROG);
> +       if (result >= 0) {

You missed the second part of my comment. Please, read carefully _all_
reviewer's comments.

> +               input_report_switch(asus->inputdev, SW_TABLET_MODE, result);
> +               input_sync(asus->inputdev);
> +       }
> +}

...

Overall, it's getting better!

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 2/2] asus-wmi: Add support for ROG X13 tablet mode
  2022-08-09  8:40   ` Andy Shevchenko
@ 2022-08-09  8:46     ` Hans de Goede
  2022-08-09  9:03       ` Luke Jones
  0 siblings, 1 reply; 7+ messages in thread
From: Hans de Goede @ 2022-08-09  8:46 UTC (permalink / raw)
  To: Andy Shevchenko, Luke D. Jones; +Cc: Platform Driver, Linux Kernel Mailing List

Hi,

On 8/9/22 10:40, Andy Shevchenko wrote:
> On Tue, Aug 9, 2022 at 5:31 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_IGNORE, 0xC6, },  /* Ambient Light Sensor notification */
>>         { KE_KEY, 0xFA, { KEY_PROG2 } },           /* Lid flip action */
>> +       { KE_KEY, 0xBD, { KEY_PROG2 } }, /* Lid flip action on ROG xflow laptops */
> 
> Shouldn't you keep it sorted by value?

Actually as I mentioned in my review of v1, we don't want this
addition at all, see:

https://lore.kernel.org/platform-driver-x86/d9d79f9b-f3ab-c07e-9e18-5760ff828487@redhat.com/

Regards,

Hans


> 
> ...
> 
>>  #define NOTIFY_KBD_FBM                 0x99
>>  #define NOTIFY_KBD_TTP                 0xae
>>  #define NOTIFY_LID_FLIP                        0xfa
>> +#define NOTIFY_LID_FLIP_ROG            0xbd
> 
> Ditto.
> 
> ...
> 
>> +static void lid_flip_rog_tablet_mode_get_state(struct asus_wmi *asus)
>> +{
>> +       int result;
>> +
>> +       result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_LID_FLIP_ROG);
>> +       if (result >= 0) {
> 
> You missed the second part of my comment. Please, read carefully _all_
> reviewer's comments.
> 
>> +               input_report_switch(asus->inputdev, SW_TABLET_MODE, result);
>> +               input_sync(asus->inputdev);
>> +       }
>> +}
> 
> ...
> 
> Overall, it's getting better!
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 2/2] asus-wmi: Add support for ROG X13 tablet mode
  2022-08-09  8:46     ` Hans de Goede
@ 2022-08-09  9:03       ` Luke Jones
  0 siblings, 0 replies; 7+ messages in thread
From: Luke Jones @ 2022-08-09  9:03 UTC (permalink / raw)
  To: Hans de Goede; +Cc: Andy Shevchenko, Platform Driver, Linux Kernel Mailing List

Hello,

On Tue, Aug 9 2022 at 10:46:11 +0200, Hans de Goede 
<hdegoede@redhat.com> wrote:
> Hi,
> 
> On 8/9/22 10:40, Andy Shevchenko wrote:
>>  On Tue, Aug 9, 2022 at 5:31 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_IGNORE, 0xC6, },  /* Ambient Light Sensor 
>>> notification */
>>>          { KE_KEY, 0xFA, { KEY_PROG2 } },           /* Lid flip 
>>> action */
>>>  +       { KE_KEY, 0xBD, { KEY_PROG2 } }, /* Lid flip action on ROG 
>>> xflow laptops */
>> 
>>  Shouldn't you keep it sorted by value?
> 
> Actually as I mentioned in my review of v1, we don't want this
> addition at all, see:
> 
> https://lore.kernel.org/platform-driver-x86/d9d79f9b-f3ab-c07e-9e18-5760ff828487@redhat.com/
> 

My apologies, I will fix this.


> Regards,
> 
> Hans
> 
> 
>> 
>>  ...
>> 
>>>   #define NOTIFY_KBD_FBM                 0x99
>>>   #define NOTIFY_KBD_TTP                 0xae
>>>   #define NOTIFY_LID_FLIP                        0xfa
>>>  +#define NOTIFY_LID_FLIP_ROG            0xbd
>> 
>>  Ditto.
>> 
>>  ...
>> 
>>>  +static void lid_flip_rog_tablet_mode_get_state(struct asus_wmi 
>>> *asus)
>>>  +{
>>>  +       int result;
>>>  +
>>>  +       result = asus_wmi_get_devstate_simple(asus, 
>>> ASUS_WMI_DEVID_LID_FLIP_ROG);
>>>  +       if (result >= 0) {
>> 
>>  You missed the second part of my comment. Please, read carefully 
>> _all_
>>  reviewer's comments.
>> 
>>>  +               input_report_switch(asus->inputdev, 
>>> SW_TABLET_MODE, result);
>>>  +               input_sync(asus->inputdev);
>>>  +       }
>>>  +}
>> 
>>  ...
>> 
>>  Overall, it's getting better!
>> 
> 



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2022-08-09  9:03 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-09  3:30 [PATCH v3 0/2] asus-wmi: add support for ROG XFlow tablet mode Luke D. Jones
2022-08-09  3:30 ` [PATCH v3 1/2] asus-wmi: Adjust tablet/lidflip handling to use enum Luke D. Jones
2022-08-09  8:37   ` Andy Shevchenko
2022-08-09  3:30 ` [PATCH v3 2/2] asus-wmi: Add support for ROG X13 tablet mode Luke D. Jones
2022-08-09  8:40   ` Andy Shevchenko
2022-08-09  8:46     ` Hans de Goede
2022-08-09  9:03       ` Luke Jones

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.