fujitsu-laptop: acpi_fujitsu_hotkey_notify() cleanup

acpi_fujitsu_hotkey_notify() is pretty deeply nested, which hurts readability. Strip off one level of indentation by returning early when the event code supplied as argument is not ACPI_FUJITSU_NOTIFY_CODE1.

Signed-off-by Micha?? K??pie?? <[email protected]>

---
Changes introduced by this patch are best viewed when whitespace changes are ignored.

 drivers/platform/x86/fujitsu-laptop.c | 152 +++++++++++++++++-----------------
 1 file changed, 75 insertions(+), 77 deletions(-)

drivers/platform/x86/fujitsu-laptop.c +75 -77

index b725a907a91f..c2022f8af51b 100644
@@ -1039,98 +1039,96 @@ static void acpi_fujitsu_hotkey_notify(struct acpi_device *device, u32 event)
 
 	input = fujitsu_hotkey->input;
 
+	if (event != ACPI_FUJITSU_NOTIFY_CODE1) {
+		keycode = KEY_UNKNOWN;
+		vdbg_printk(FUJLAPTOP_DBG_WARN,
+			    "Unsupported event [0x%x]\n", event);
+		input_report_key(input, keycode, 1);
+		input_sync(input);
+		input_report_key(input, keycode, 0);
+		input_sync(input);
+		return;
+	}
+
 	if (fujitsu_hotkey->rfkill_supported)
 		fujitsu_hotkey->rfkill_state =
 			call_fext_func(FUNC_RFKILL, 0x4, 0x0, 0x0);
 
-	switch (event) {
-	case ACPI_FUJITSU_NOTIFY_CODE1:
-		i = 0;
-		while ((irb =
-			call_fext_func(FUNC_BUTTONS, 0x1, 0x0, 0x0)) != 0
-				&& (i++) < MAX_HOTKEY_RINGBUFFER_SIZE) {
-			switch (irb & 0x4ff) {
-			case KEY1_CODE:
-				keycode = fujitsu->keycode1;
-				break;
-			case KEY2_CODE:
-				keycode = fujitsu->keycode2;
-				break;
-			case KEY3_CODE:
-				keycode = fujitsu->keycode3;
-				break;
-			case KEY4_CODE:
-				keycode = fujitsu->keycode4;
-				break;
-			case KEY5_CODE:
-				keycode = fujitsu->keycode5;
-				break;
-			case 0:
-				keycode = 0;
-				break;
-			default:
+	i = 0;
+	while ((irb =
+		call_fext_func(FUNC_BUTTONS, 0x1, 0x0, 0x0)) != 0
+			&& (i++) < MAX_HOTKEY_RINGBUFFER_SIZE) {
+		switch (irb & 0x4ff) {
+		case KEY1_CODE:
+			keycode = fujitsu->keycode1;
+			break;
+		case KEY2_CODE:
+			keycode = fujitsu->keycode2;
+			break;
+		case KEY3_CODE:
+			keycode = fujitsu->keycode3;
+			break;
+		case KEY4_CODE:
+			keycode = fujitsu->keycode4;
+			break;
+		case KEY5_CODE:
+			keycode = fujitsu->keycode5;
+			break;
+		case 0:
+			keycode = 0;
+			break;
+		default:
+			vdbg_printk(FUJLAPTOP_DBG_WARN,
+				    "Unknown GIRB result [%x]\n", irb);
+			keycode = -1;
+			break;
+		}
+		if (keycode > 0) {
+			vdbg_printk(FUJLAPTOP_DBG_TRACE,
+				"Push keycode into ringbuffer [%d]\n",
+				keycode);
+			status = kfifo_in_locked(&fujitsu_hotkey->fifo,
+					   (unsigned char *)&keycode,
+					   sizeof(keycode),
+					   &fujitsu_hotkey->fifo_lock);
+			if (status != sizeof(keycode)) {
 				vdbg_printk(FUJLAPTOP_DBG_WARN,
-					    "Unknown GIRB result [%x]\n", irb);
-				keycode = -1;
-				break;
+				    "Could not push keycode [0x%x]\n",
+				    keycode);
+			} else {
+				input_report_key(input, keycode, 1);
+				input_sync(input);
 			}
-			if (keycode > 0) {
+		} else if (keycode == 0) {
+			while ((status =
+				kfifo_out_locked(
+				 &fujitsu_hotkey->fifo,
+				 (unsigned char *) &keycode_r,
+				 sizeof(keycode_r),
+				 &fujitsu_hotkey->fifo_lock))
+				 == sizeof(keycode_r)) {
+				input_report_key(input, keycode_r, 0);
+				input_sync(input);
 				vdbg_printk(FUJLAPTOP_DBG_TRACE,
-					"Push keycode into ringbuffer [%d]\n",
-					keycode);
-				status = kfifo_in_locked(&fujitsu_hotkey->fifo,
-						   (unsigned char *)&keycode,
-						   sizeof(keycode),
-						   &fujitsu_hotkey->fifo_lock);
-				if (status != sizeof(keycode)) {
-					vdbg_printk(FUJLAPTOP_DBG_WARN,
-					    "Could not push keycode [0x%x]\n",
-					    keycode);
-				} else {
-					input_report_key(input, keycode, 1);
-					input_sync(input);
-				}
-			} else if (keycode == 0) {
-				while ((status =
-					kfifo_out_locked(
-					 &fujitsu_hotkey->fifo,
-					 (unsigned char *) &keycode_r,
-					 sizeof(keycode_r),
-					 &fujitsu_hotkey->fifo_lock))
-					 == sizeof(keycode_r)) {
-					input_report_key(input, keycode_r, 0);
-					input_sync(input);
-					vdbg_printk(FUJLAPTOP_DBG_TRACE,
-					  "Pop keycode from ringbuffer [%d]\n",
-					  keycode_r);
-				}
+				  "Pop keycode from ringbuffer [%d]\n",
+				  keycode_r);
 			}
 		}
+	}
 
-		/* On some models (first seen on the Skylake-based Lifebook
-		 * E736/E746/E756), the touchpad toggle hotkey (Fn+F4) is
-		 * handled in software; its state is queried using FUNC_RFKILL
-		 */
-		if ((fujitsu_hotkey->rfkill_supported & BIT(26)) &&
-		    (call_fext_func(FUNC_RFKILL, 0x1, 0x0, 0x0) & BIT(26))) {
-			keycode = KEY_TOUCHPAD_TOGGLE;
-			input_report_key(input, keycode, 1);
-			input_sync(input);
-			input_report_key(input, keycode, 0);
-			input_sync(input);
-		}
-
-		break;
-	default:
-		keycode = KEY_UNKNOWN;
-		vdbg_printk(FUJLAPTOP_DBG_WARN,
-			    "Unsupported event [0x%x]\n", event);
+	/* On some models (first seen on the Skylake-based Lifebook
+	 * E736/E746/E756), the touchpad toggle hotkey (Fn+F4) is
+	 * handled in software; its state is queried using FUNC_RFKILL
+	 */
+	if ((fujitsu_hotkey->rfkill_supported & BIT(26)) &&
+	    (call_fext_func(FUNC_RFKILL, 0x1, 0x0, 0x0) & BIT(26))) {
+		keycode = KEY_TOUCHPAD_TOGGLE;
 		input_report_key(input, keycode, 1);
 		input_sync(input);
 		input_report_key(input, keycode, 0);
 		input_sync(input);
-		break;
 	}
+
 }
 
 /* Initialization */

Signature · 1 line

2.11.0

acpi_fujitsu_hotkey_notify() is pretty deeply nested, which hurts readability. Move the keycode processing part to two separate functions to make the code easier to understand and save a few line breaks. Rename variable keycode_r to keycode as there is no longer any need to differentiate between the two. Tweak indentations to make checkpatch happy.

Signed-off-by Micha?? K??pie?? <[email protected]>

---
 drivers/platform/x86/fujitsu-laptop.c | 76 ++++++++++++++++++++---------------
 1 file changed, 43 insertions(+), 33 deletions(-)

drivers/platform/x86/fujitsu-laptop.c +43 -33

index c2022f8af51b..e57d3724d2ce 100644
@@ -1030,12 +1030,48 @@ static int acpi_fujitsu_hotkey_remove(struct acpi_device *device)
 	return 0;
 }
 
+static void acpi_fujitsu_hotkey_press(int keycode)
+{
+	struct input_dev *input = fujitsu_hotkey->input;
+	int status;
+
+	vdbg_printk(FUJLAPTOP_DBG_TRACE,
+		    "Push keycode into ringbuffer [%d]\n", keycode);
+	status = kfifo_in_locked(&fujitsu_hotkey->fifo,
+				 (unsigned char *)&keycode, sizeof(keycode),
+				 &fujitsu_hotkey->fifo_lock);
+	if (status != sizeof(keycode)) {
+		vdbg_printk(FUJLAPTOP_DBG_WARN,
+			    "Could not push keycode [0x%x]\n", keycode);
+	} else {
+		input_report_key(input, keycode, 1);
+		input_sync(input);
+	}
+}
+
+static void acpi_fujitsu_hotkey_release(void)
+{
+	struct input_dev *input = fujitsu_hotkey->input;
+	int keycode, status;
+
+	while ((status = kfifo_out_locked(&fujitsu_hotkey->fifo,
+					  (unsigned char *)&keycode,
+					  sizeof(keycode),
+					  &fujitsu_hotkey->fifo_lock))
+					  == sizeof(keycode)) {
+		input_report_key(input, keycode, 0);
+		input_sync(input);
+		vdbg_printk(FUJLAPTOP_DBG_TRACE,
+			    "Pop keycode from ringbuffer [%d]\n", keycode);
+	}
+}
+
 static void acpi_fujitsu_hotkey_notify(struct acpi_device *device, u32 event)
 {
 	struct input_dev *input;
-	int keycode, keycode_r;
+	int keycode;
 	unsigned int irb = 1;
-	int i, status;
+	int i;
 
 	input = fujitsu_hotkey->input;
 

@@ -1083,37 +1119,11 @@ static void acpi_fujitsu_hotkey_notify(struct acpi_device *device, u32 event)
 			keycode = -1;
 			break;
 		}
-		if (keycode > 0) {
-			vdbg_printk(FUJLAPTOP_DBG_TRACE,
-				"Push keycode into ringbuffer [%d]\n",
-				keycode);
-			status = kfifo_in_locked(&fujitsu_hotkey->fifo,
-					   (unsigned char *)&keycode,
-					   sizeof(keycode),
-					   &fujitsu_hotkey->fifo_lock);
-			if (status != sizeof(keycode)) {
-				vdbg_printk(FUJLAPTOP_DBG_WARN,
-				    "Could not push keycode [0x%x]\n",
-				    keycode);
-			} else {
-				input_report_key(input, keycode, 1);
-				input_sync(input);
-			}
-		} else if (keycode == 0) {
-			while ((status =
-				kfifo_out_locked(
-				 &fujitsu_hotkey->fifo,
-				 (unsigned char *) &keycode_r,
-				 sizeof(keycode_r),
-				 &fujitsu_hotkey->fifo_lock))
-				 == sizeof(keycode_r)) {
-				input_report_key(input, keycode_r, 0);
-				input_sync(input);
-				vdbg_printk(FUJLAPTOP_DBG_TRACE,
-				  "Pop keycode from ringbuffer [%d]\n",
-				  keycode_r);
-			}
-		}
+
+		if (keycode > 0)
+			acpi_fujitsu_hotkey_press(keycode);
+		else if (keycode == 0)
+			acpi_fujitsu_hotkey_release();
 	}
 
 	/* On some models (first seen on the Skylake-based Lifebook

Signature · 1 line

2.11.0

The loop condition in acpi_fujitsu_hotkey_release() includes an assignment, a four-argument function call and a comparison, making it hard to read. Separate the assignment from the comparison to improve readability.

Signed-off-by Micha?? K??pie?? <[email protected]>

---
 drivers/platform/x86/fujitsu-laptop.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

drivers/platform/x86/fujitsu-laptop.c +5 -3

index e57d3724d2ce..06653a8594ed 100644
@@ -1054,11 +1054,13 @@ static void acpi_fujitsu_hotkey_release(void)
 	struct input_dev *input = fujitsu_hotkey->input;
 	int keycode, status;
 
-	while ((status = kfifo_out_locked(&fujitsu_hotkey->fifo,
+	while (true) {
+		status = kfifo_out_locked(&fujitsu_hotkey->fifo,
 					  (unsigned char *)&keycode,
 					  sizeof(keycode),
-					  &fujitsu_hotkey->fifo_lock))
-					  == sizeof(keycode)) {
+					  &fujitsu_hotkey->fifo_lock);
+		if (status != sizeof(keycode))
+			return;
 		input_report_key(input, keycode, 0);
 		input_sync(input);
 		vdbg_printk(FUJLAPTOP_DBG_TRACE,

Signature · 1 line

2.11.0

Make two minor tweaks to acpi_fujitsu_hotkey_press() to make it more similar to acpi_fujitsu_hotkey_release():

* call vdbg_printk() after reporting the input event,
* return immediately when kfifo_in_locked() fails.

Signed-off-by Micha?? K??pie?? <[email protected]>

---
 drivers/platform/x86/fujitsu-laptop.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

drivers/platform/x86/fujitsu-laptop.c +5 -5

index 06653a8594ed..96f8163d5002 100644
@@ -1035,18 +1035,18 @@ static void acpi_fujitsu_hotkey_press(int keycode)
 	struct input_dev *input = fujitsu_hotkey->input;
 	int status;
 
-	vdbg_printk(FUJLAPTOP_DBG_TRACE,
-		    "Push keycode into ringbuffer [%d]\n", keycode);
 	status = kfifo_in_locked(&fujitsu_hotkey->fifo,
 				 (unsigned char *)&keycode, sizeof(keycode),
 				 &fujitsu_hotkey->fifo_lock);
 	if (status != sizeof(keycode)) {
 		vdbg_printk(FUJLAPTOP_DBG_WARN,
 			    "Could not push keycode [0x%x]\n", keycode);
-	} else {
-		input_report_key(input, keycode, 1);
-		input_sync(input);
+		return;
 	}
+	input_report_key(input, keycode, 1);
+	input_sync(input);
+	vdbg_printk(FUJLAPTOP_DBG_TRACE,
+		    "Push keycode into ringbuffer [%d]\n", keycode);
 }
 
 static void acpi_fujitsu_hotkey_release(void)

Signature · 1 line

2.11.0

I am currently preparing a patch series which makes fujitsu-laptop use a sparse keymap for hotkey handling. Before that will happen, though, acpi_fujitsu_hotkey_notify() could use a revamp because it is pretty hard to read as it is. To avoid posting everything at once, here are a few patches which IMHO make that function easier to read. Some of these changes might be a matter of taste, so feel free to NACK them or suggest a preferred alternative.

Quoted 7 lines

I am currently preparing a patch series which makes fujitsu-laptop use a sparse keymap for hotkey handling. Before that will happen, though, acpi_fujitsu_hotkey_notify() could use a revamp because it is pretty hard to read as it is. To avoid posting everything at once, here are a few patches which IMHO make that function easier to read. Some of these changes might be a matter of taste, so feel free to NACK them or suggest a preferred alternative.

This patch series provides a significant clean up to the functions it focuses on. As such I have no real objections to them. However, because my Fujitsu laptop doesn't have any of the hotkeys of later models I am unable to test these patches with real hardware. Have you been able to do so? If they have been verified I have no problem acking these. Otherwise I will have to do as much as I can (given no access to relevant hardware) to ensure the overall behaviour isn't changed.

Quoted 9 lines · Micha?? K??pie??

Quoted 7 lines

I am currently preparing a patch series which makes fujitsu-laptop use a sparse keymap for hotkey handling. Before that will happen, though, acpi_fujitsu_hotkey_notify() could use a revamp because it is pretty hard to read as it is. To avoid posting everything at once, here are a few patches which IMHO make that function easier to read. Some of these changes might be a matter of taste, so feel free to NACK them or suggest a preferred alternative.

Quoted 7 lines

This patch series provides a significant clean up to the functions it focuses on. As such I have no real objections to them. However, because my Fujitsu laptop doesn't have any of the hotkeys of later models I am unable to test these patches with real hardware. Have you been able to do so? If they have been verified I have no problem acking these. Otherwise I will have to do as much as I can (given no access to relevant hardware) to ensure the overall behaviour isn't changed.

I tested these on a Lifebook E744, which is capable of generating KEY4_CODE ("ECO on/off button") and KEY5_CODE ("Wireless/Bluetooth on/off button"). I checked that these hotkeys still work fine with this patch series applied. By temporarily reversing some logical conditions, I also did my best to ensure that unexpected behaviors (unknown ACPI event code, kfifo failures) are still handled in the same way as previously (apart from the "Push keycode into ringbuffer" debug message, which is now only printed upon a successful push due to the last patch).

Quoted 17 lines · Micha?? K??pie??

Quoted 9 lines · Micha?? K??pie??

Quoted 7 lines

I am currently preparing a patch series which makes fujitsu-laptop use a sparse keymap for hotkey handling. Before that will happen, though, acpi_fujitsu_hotkey_notify() could use a revamp because it is pretty hard to read as it is. To avoid posting everything at once, here are a few patches which IMHO make that function easier to read. Some of these changes might be a matter of taste, so feel free to NACK them or suggest a preferred alternative.

Quoted 7 lines

This patch series provides a significant clean up to the functions it focuses on. As such I have no real objections to them. However, because my Fujitsu laptop doesn't have any of the hotkeys of later models I am unable to test these patches with real hardware. Have you been able to do so? If they have been verified I have no problem acking these. Otherwise I will have to do as much as I can (given no access to relevant hardware) to ensure the overall behaviour isn't changed.

Quoted 8 lines

I tested these on a Lifebook E744, which is capable of generating KEY4_CODE ("ECO on/off button") and KEY5_CODE ("Wireless/Bluetooth on/off button"). I checked that these hotkeys still work fine with this patch series applied. By temporarily reversing some logical conditions, I also did my best to ensure that unexpected behaviors (unknown ACPI event code, kfifo failures) are still handled in the same way as previously (apart from the "Push keycode into ringbuffer" debug message, which is now only printed upon a successful push due to the last patch).

Thanks for clarifying. It may be worth adding a comment to the effect that the patches were tested on a Lifebook E744. That aside, I'm happy with these clean ups.

Acked-by Jonathan Woithe <[email protected]>

Darren: do you want me to explicitly ack all 4 parts, or the above sufficient for your processes?

Quoted 17 lines · Micha?? K??pie??

Quoted 9 lines · Micha?? K??pie??

Quoted 7 lines

I am currently preparing a patch series which makes fujitsu-laptop use a sparse keymap for hotkey handling. Before that will happen, though, acpi_fujitsu_hotkey_notify() could use a revamp because it is pretty hard to read as it is. To avoid posting everything at once, here are a few patches which IMHO make that function easier to read. Some of these changes might be a matter of taste, so feel free to NACK them or suggest a preferred alternative.

Quoted 7 lines

This patch series provides a significant clean up to the functions it focuses on. As such I have no real objections to them. However, because my Fujitsu laptop doesn't have any of the hotkeys of later models I am unable to test these patches with real hardware. Have you been able to do so? If they have been verified I have no problem acking these. Otherwise I will have to do as much as I can (given no access to relevant hardware) to ensure the overall behaviour isn't changed.

Quoted 8 lines

I tested these on a Lifebook E744, which is capable of generating KEY4_CODE ("ECO on/off button") and KEY5_CODE ("Wireless/Bluetooth on/off button"). I checked that these hotkeys still work fine with this patch series applied. By temporarily reversing some logical conditions, I also did my best to ensure that unexpected behaviors (unknown ACPI event code, kfifo failures) are still handled in the same way as previously (apart from the "Push keycode into ringbuffer" debug message, which is now only printed upon a successful push due to the last patch).

Thanks for clarifying. It may be worth adding a comment to the effect that the patches were tested on a Lifebook E744. That aside, I'm happy with these clean ups.

Acked-by Jonathan Woithe <[email protected]>