[PATCH 0/4] fujitsu-laptop: acpi_fujitsu_hotkey_notify() cleanup

[PATCH 0/4] fujitsu-laptop: acpi_fujitsu_hotkey_notify() cleanup

[Michał Kępień]

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.

 drivers/platform/x86/fujitsu-laptop.c | 174 ++++++++++++++++++----------------
 1 file changed, 92 insertions(+), 82 deletions(-)

-- 
2.11.0

[PATCH 1/4] platform/x86: fujitsu-laptop: decrease indentation in acpi_fujitsu_hotkey_notify()

[Michał Kępień]

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ń <kernel@kempniu.pl>
---
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(-)

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index b725a907a91f..c2022f8af51b 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -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 */
-- 
2.11.0

[PATCH 2/4] platform/x86: fujitsu-laptop: move keycode processing to separate functions

[Michał Kępień]

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ń <kernel@kempniu.pl>
---
 drivers/platform/x86/fujitsu-laptop.c | 76 ++++++++++++++++++++---------------
 1 file changed, 43 insertions(+), 33 deletions(-)

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index c2022f8af51b..e57d3724d2ce 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -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
-- 
2.11.0

[PATCH 3/4] platform/x86: fujitsu-laptop: break up complex loop condition

[Michał Kępień]

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ń <kernel@kempniu.pl>
---
 drivers/platform/x86/fujitsu-laptop.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index e57d3724d2ce..06653a8594ed 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -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,
-- 
2.11.0

[PATCH 4/4] platform/x86: fujitsu-laptop: make hotkey handling functions more similar

[Michał Kępień]

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ń <kernel@kempniu.pl>
---
 drivers/platform/x86/fujitsu-laptop.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 06653a8594ed..96f8163d5002 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -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)
-- 
2.11.0

Re: [PATCH 0/4] fujitsu-laptop: acpi_fujitsu_hotkey_notify() cleanup

[Jonathan Woithe]

On Wed, Jan 11, 2017 at 09:59:29AM +0100, Micha?? K??pie?? wrote:
> 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.

Regards
  jonathan

Re: [PATCH 0/4] fujitsu-laptop: acpi_fujitsu_hotkey_notify() cleanup

[Michał Kępień]

> On Wed, Jan 11, 2017 at 09:59:29AM +0100, Micha?? K??pie?? wrote:
> > 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.

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

-- 
Best regards,
Michał Kępień

Re: [PATCH 0/4] fujitsu-laptop: acpi_fujitsu_hotkey_notify() cleanup

[Jonathan Woithe]

On Wed, Jan 11, 2017 at 01:26:49PM +0100, Micha?? K??pie?? wrote:
> > On Wed, Jan 11, 2017 at 09:59:29AM +0100, Micha?? K??pie?? wrote:
> > > 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.
> 
> 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 <jwoithe@just42.net>

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

Regards
  jonathan

Re: [PATCH 1/4] platform/x86: fujitsu-laptop: decrease indentation in acpi_fujitsu_hotkey_notify()

[Jonathan Woithe]

On Wed, Jan 11, 2017 at 09:59:30AM +0100, Micha?? K??pie?? wrote:
> 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?? <kernel@kempniu.pl>

Acked-by: Jonathan Woithe <jwoithe@just42.net>

> ---
> 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(-)
> 
> diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> index b725a907a91f..c2022f8af51b 100644
> --- a/drivers/platform/x86/fujitsu-laptop.c
> +++ b/drivers/platform/x86/fujitsu-laptop.c
> @@ -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 */
> -- 
> 2.11.0

-- 

Re: [PATCH 2/4] platform/x86: fujitsu-laptop: move keycode processing to separate functions

[Jonathan Woithe]

On Wed, Jan 11, 2017 at 09:59:31AM +0100, Micha?? K??pie?? wrote:
> 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?? <kernel@kempniu.pl>

Acked-by: Jonathan Woithe <jwoithe@just42.net>

> ---
>  drivers/platform/x86/fujitsu-laptop.c | 76 ++++++++++++++++++++---------------
>  1 file changed, 43 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> index c2022f8af51b..e57d3724d2ce 100644
> --- a/drivers/platform/x86/fujitsu-laptop.c
> +++ b/drivers/platform/x86/fujitsu-laptop.c
> @@ -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
> -- 
> 2.11.0

-- 

Re: [PATCH 3/4] platform/x86: fujitsu-laptop: break up complex loop condition

[Jonathan Woithe]

On Wed, Jan 11, 2017 at 09:59:32AM +0100, Micha?? K??pie?? wrote:
> 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?? <kernel@kempniu.pl>

Acked-by: Jonathan Woithe <jwoithe@just42.net>

> ---
>  drivers/platform/x86/fujitsu-laptop.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> index e57d3724d2ce..06653a8594ed 100644
> --- a/drivers/platform/x86/fujitsu-laptop.c
> +++ b/drivers/platform/x86/fujitsu-laptop.c
> @@ -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,
> -- 
> 2.11.0

-- 

Re: [PATCH 4/4] platform/x86: fujitsu-laptop: make hotkey handling functions more similar

[Jonathan Woithe]

On Wed, Jan 11, 2017 at 09:59:33AM +0100, Micha?? K??pie?? wrote:
> 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?? <kernel@kempniu.pl>

Acked-by: Jonathan Woithe <jwoithe@just42.net>

> ---
>  drivers/platform/x86/fujitsu-laptop.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> index 06653a8594ed..96f8163d5002 100644
> --- a/drivers/platform/x86/fujitsu-laptop.c
> +++ b/drivers/platform/x86/fujitsu-laptop.c
> @@ -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)
> -- 
> 2.11.0

-- 

Re: [PATCH 0/4] fujitsu-laptop: acpi_fujitsu_hotkey_notify() cleanup

[Darren Hart]

On Wed, Jan 11, 2017 at 11:18:13PM +1030, Jonathan Woithe wrote:
> On Wed, Jan 11, 2017 at 01:26:49PM +0100, Micha?? K??pie?? wrote:
> > > On Wed, Jan 11, 2017 at 09:59:29AM +0100, Micha?? K??pie?? wrote:
> > > > 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.
> > 
> > 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 <jwoithe@just42.net>
> 
> Darren: do you want me to explicitly ack all 4 parts, or the above
> sufficient for your processes?

The above is sufficient as far as I'm concerned.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 0/4] fujitsu-laptop: acpi_fujitsu_hotkey_notify() cleanup

[Darren Hart]

On Wed, Jan 11, 2017 at 11:18:13PM +1030, Jonathan Woithe wrote:
> On Wed, Jan 11, 2017 at 01:26:49PM +0100, Micha?? K??pie?? wrote:
> > > On Wed, Jan 11, 2017 at 09:59:29AM +0100, Micha?? K??pie?? wrote:
> > > > 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.
> > 
> > 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 <jwoithe@just42.net>

Queued to testing, thanks!

-- 
Darren Hart
Intel Open Source Technology Center