[PATCH v2 0/2] asus-wmi: add support for ROG XFlow tablet mode

[PATCH v2 0/2] asus-wmi: add support for ROG XFlow tablet mode

[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:
- 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         | 30 +++++---
 drivers/platform/x86/asus-wmi.c            | 89 ++++++++++++++++++----
 drivers/platform/x86/asus-wmi.h            | 10 ++-
 include/linux/platform_data/x86/asus-wmi.h |  1 +
 4 files changed, 103 insertions(+), 27 deletions(-)

-- 
2.37.1

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

[Luke D. Jones]

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(-)

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
-- 
2.37.1

[PATCH v2 2/2] asus-wmi: Add support for ROG X13 tablet mode

[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         | 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

Re: [PATCH v2 0/2] asus-wmi: add support for ROG XFlow tablet mode

[Andy Shevchenko]

On Mon, Aug 8, 2022 at 5:12 AM Luke D. Jones <luke@ljones.dev> wrote:
>
> 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:
> - 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

Good written cover letter, thanks!

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] asus-wmi: Add support for ROG X13 tablet mode

[Andy Shevchenko]

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;
  ...

> +}

-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

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

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

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

Re: [PATCH v2 2/2] asus-wmi: Add support for ROG X13 tablet mode

[Luke Jones]

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.
> 

Re: [PATCH v2 2/2] asus-wmi: Add support for ROG X13 tablet mode

[Andy Shevchenko]

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

Re: [PATCH v2 2/2] asus-wmi: Add support for ROG X13 tablet mode

[Luke Jones]

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.