Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1667413 > unrolled thread

[PATCH 0/7] fujitsu-laptop: ACPI-related cleanups

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/7] fujitsu-laptop: ACPI-related cleanups Michał Kępień <kernel@kempniu.pl> - 2017-06-16 06:50 +0200
    [PATCH 6/7] platform/x86: fujitsu-laptop: do not evaluate ACPI _INI methods Michał Kępień <kernel@kempniu.pl> - 2017-06-16 06:50 +0200
    [PATCH 5/7] platform/x86: fujitsu-laptop: do not update ACPI device power status Michał Kępień <kernel@kempniu.pl> - 2017-06-16 06:50 +0200
    [PATCH 7/7] platform/x86: fujitsu-laptop: rework debugging Michał Kępień <kernel@kempniu.pl> - 2017-06-16 06:50 +0200
    [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for storing hotkey scancodes Michał Kępień <kernel@kempniu.pl> - 2017-06-16 06:50 +0200
    Re: [PATCH 0/7] fujitsu-laptop: ACPI-related cleanups Jonathan Woithe <jwoithe@just42.net> - 2017-06-18 12:40 +0200

#1667413 — [PATCH 0/7] fujitsu-laptop: ACPI-related cleanups

In preparation for splitting fujitsu-laptop into two separate modules, I
prepared two more cleanup series to minimize the amount of code being
moved around.  This first series contains all patches that touch ACPI in
some way, which is why I am CCing linux-acpi as per Rafael's previous
advice.

This patch series was based on platform-driver-x86/for-next and tested
on a Lifebook S7020.

 drivers/platform/x86/Kconfig          |  10 --
 drivers/platform/x86/fujitsu-laptop.c | 187 +++++++++-------------------------
 2 files changed, 48 insertions(+), 149 deletions(-)

-- 
2.13.1

[toc] | [next] | [standalone]

#1667414 — [PATCH 6/7] platform/x86: fujitsu-laptop: do not evaluate ACPI _INI methods

acpi_ns_initialize_devices(), which is called during system-wide ACPI
initialization, already detects and calls all _INI methods belonging to
objects present in ACPI tables.  There is no need to call these methods
again every time the module is loaded because they only initialize
status flags and hotkey-related variables; status flags are effectively
constants, hotkey-related variables may be assigned non-zero values
before acpi_fujitsu_laptop_add() is called, but that does not really
matter as we drain the scancodes queued in the firmware's ring buffer
before doing anything else.

Remove sections of code which invoke and check evaluation status of the
_INI methods belonging to the ACPI devices handled by the driver.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/fujitsu-laptop.c | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index b9f3ede4d567..0861be36305d 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -421,14 +421,6 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device)
 	pr_info("ACPI: %s [%s]\n",
 		acpi_device_name(device), acpi_device_bid(device));
 
-	if (acpi_has_method(device->handle, METHOD_NAME__INI)) {
-		vdbg_printk(FUJLAPTOP_DBG_INFO, "Invoking _INI\n");
-		if (ACPI_FAILURE
-		    (acpi_evaluate_object
-		     (device->handle, METHOD_NAME__INI, NULL, NULL)))
-			pr_err("_INI Method failed\n");
-	}
-
 	if (get_max_brightness(device) <= 0)
 		priv->max_brightness = FUJITSU_LCD_N_LEVELS;
 	get_lcd_level(device);
@@ -801,14 +793,6 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 	pr_info("ACPI: %s [%s]\n",
 		acpi_device_name(device), acpi_device_bid(device));
 
-	if (acpi_has_method(device->handle, METHOD_NAME__INI)) {
-		vdbg_printk(FUJLAPTOP_DBG_INFO, "Invoking _INI\n");
-		if (ACPI_FAILURE
-		    (acpi_evaluate_object
-		     (device->handle, METHOD_NAME__INI, NULL, NULL)))
-			pr_err("_INI Method failed\n");
-	}
-
 	i = 0;
 	while (call_fext_func(device, FUNC_BUTTONS, 0x1, 0x0, 0x0) != 0
 		&& (i++) < MAX_HOTKEY_RINGBUFFER_SIZE)
-- 
2.13.1

[toc] | [prev] | [next] | [standalone]

#1667415 — [PATCH 5/7] platform/x86: fujitsu-laptop: do not update ACPI device power status

Calling acpi_bus_update_power() for ACPI devices FUJ02B1 and FUJ02E3 is
pointless as they are not power manageable (neither _PS0 nor _PR0 is
defined for any of them), which causes their power state to be inherited
from their parent devices.  Given the ACPI paths of these two devices
(\_SB.PCI0.LPCB.FJEX, \_SB.FEXT), their parent devices are also not
power manageable.  These parent devices will thus have their power state
initialized to ACPI_STATE_D0, which in turn causes the power state for
both FUJ02B1 and FUJ02E3 to always be ACPI_STATE_D0 ("on").

Remove relevant acpi_bus_update_power() calls along with parts of debug
messages that they were supposed to have an effect on.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/fujitsu-laptop.c | 24 ++++--------------------
 1 file changed, 4 insertions(+), 20 deletions(-)

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 5c0dc2126313..b9f3ede4d567 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -400,7 +400,6 @@ static int fujitsu_backlight_register(struct acpi_device *device)
 static int acpi_fujitsu_bl_add(struct acpi_device *device)
 {
 	struct fujitsu_bl *priv;
-	int state = 0;
 	int error;
 
 	if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
@@ -419,15 +418,8 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device)
 	if (error)
 		return error;
 
-	error = acpi_bus_update_power(device->handle, &state);
-	if (error) {
-		pr_err("Error reading power state\n");
-		return error;
-	}
-
-	pr_info("ACPI: %s [%s] (%s)\n",
-	       acpi_device_name(device), acpi_device_bid(device),
-	       !device->power.state ? "on" : "off");
+	pr_info("ACPI: %s [%s]\n",
+		acpi_device_name(device), acpi_device_bid(device));
 
 	if (acpi_has_method(device->handle, METHOD_NAME__INI)) {
 		vdbg_printk(FUJLAPTOP_DBG_INFO, "Invoking _INI\n");
@@ -788,7 +780,6 @@ static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
 static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 {
 	struct fujitsu_laptop *priv;
-	int state = 0;
 	int error;
 	int i;
 
@@ -807,15 +798,8 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 	if (error)
 		return error;
 
-	error = acpi_bus_update_power(device->handle, &state);
-	if (error) {
-		pr_err("Error reading power state\n");
-		return error;
-	}
-
-	pr_info("ACPI: %s [%s] (%s)\n",
-		acpi_device_name(device), acpi_device_bid(device),
-		!device->power.state ? "on" : "off");
+	pr_info("ACPI: %s [%s]\n",
+		acpi_device_name(device), acpi_device_bid(device));
 
 	if (acpi_has_method(device->handle, METHOD_NAME__INI)) {
 		vdbg_printk(FUJLAPTOP_DBG_INFO, "Invoking _INI\n");
-- 
2.13.1

[toc] | [prev] | [next] | [standalone]

#1667416 — [PATCH 7/7] platform/x86: fujitsu-laptop: rework debugging

Using a dedicated Kconfig option for enabling debugging means the user
may be forced to recompile their kernel in order to gather debugging
information, which is inconvenient.  Replace custom debugging
infrastructure with standard logging functions, taking advantage of
dynamic debug.  Replace a pr_info() call inside an ACPI callback with an
acpi_handle_info() call.

The following mapping was used:

  - FUJLAPTOP_DBG_ERROR -> acpi_handle_err()
  - FUJLAPTOP_DBG_WARN  -> acpi_handle_info() / dev_info()
  - FUJLAPTOP_DBG_INFO  -> acpi_handle_debug()
  - FUJLAPTOP_DBG_TRACE -> acpi_handle_debug() / dev_dbg()

This means that some events which used to only be logged when the user
explicitly requested it will now be logged by default:

  - ACPI method evaluation errors,
  - unknown ACPI notification codes,
  - unknown hotkey scancodes.

The first type of events should happen rarely, if ever at all.  The rest
is interesting from driver development perspective as their presence in
the logs will mean the driver is unaware of certain events, handling of
which should be implemented.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/Kconfig          | 10 -----
 drivers/platform/x86/fujitsu-laptop.c | 78 +++++++++++++----------------------
 2 files changed, 28 insertions(+), 60 deletions(-)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 49a1d012f025..980419081ed7 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -195,16 +195,6 @@ config FUJITSU_LAPTOP
 
 	  If you have a Fujitsu laptop, say Y or M here.
 
-config FUJITSU_LAPTOP_DEBUG
-	bool "Verbose debug mode for Fujitsu Laptop Extras"
-	depends on FUJITSU_LAPTOP
-	default n
-	---help---
-	  Enables extra debug output from the fujitsu extras driver, at the
-	  expense of a slight increase in driver size.
-
-	  If you are not sure, say N here.
-
 config FUJITSU_TABLET
        tristate "Fujitsu Tablet Extras"
        depends on ACPI
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 0861be36305d..d50872d96e63 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -110,22 +110,6 @@
 
 #define MAX_HOTKEY_RINGBUFFER_SIZE 100
 
-/* Debugging */
-#define FUJLAPTOP_DBG_ERROR	  0x0001
-#define FUJLAPTOP_DBG_WARN	  0x0002
-#define FUJLAPTOP_DBG_INFO	  0x0004
-#define FUJLAPTOP_DBG_TRACE	  0x0008
-
-#ifdef CONFIG_FUJITSU_LAPTOP_DEBUG
-#define vdbg_printk(a_dbg_level, format, arg...) \
-	do { if (dbg_level & a_dbg_level) \
-		printk(KERN_DEBUG pr_fmt("%s: " format), __func__, ## arg); \
-	} while (0)
-#else
-#define vdbg_printk(a_dbg_level, format, arg...) \
-	do { } while (0)
-#endif
-
 /* Device controlling the backlight and associated keys */
 struct fujitsu_bl {
 	struct input_dev *input;
@@ -152,10 +136,6 @@ struct fujitsu_laptop {
 
 static struct acpi_device *fext;
 
-#ifdef CONFIG_FUJITSU_LAPTOP_DEBUG
-static u32 dbg_level = 0x03;
-#endif
-
 /* Fujitsu ACPI interface function */
 
 static int call_fext_func(struct acpi_device *device,
@@ -174,12 +154,13 @@ static int call_fext_func(struct acpi_device *device,
 	status = acpi_evaluate_integer(device->handle, "FUNC", &arg_list,
 				       &value);
 	if (ACPI_FAILURE(status)) {
-		vdbg_printk(FUJLAPTOP_DBG_ERROR, "Failed to evaluate FUNC\n");
+		acpi_handle_err(device->handle, "Failed to evaluate FUNC\n");
 		return -ENODEV;
 	}
 
-	vdbg_printk(FUJLAPTOP_DBG_TRACE, "FUNC 0x%x (args 0x%x, 0x%x, 0x%x) returned 0x%x\n",
-		    func, op, feature, state, (int)value);
+	acpi_handle_debug(device->handle,
+			  "FUNC 0x%x (args 0x%x, 0x%x, 0x%x) returned 0x%x\n",
+			  func, op, feature, state, (int)value);
 	return value;
 }
 
@@ -206,16 +187,16 @@ static int set_lcd_level(struct acpi_device *device, int level)
 		break;
 	}
 
-	vdbg_printk(FUJLAPTOP_DBG_TRACE, "set lcd level via %s [%d]\n",
-		    method, level);
+	acpi_handle_debug(device->handle, "set lcd level via %s [%d]\n", method,
+			  level);
 
 	if (level < 0 || level >= priv->max_brightness)
 		return -EINVAL;
 
 	status = acpi_execute_simple_method(device->handle, method, level);
 	if (ACPI_FAILURE(status)) {
-		vdbg_printk(FUJLAPTOP_DBG_ERROR, "Failed to evaluate %s\n",
-			    method);
+		acpi_handle_err(device->handle, "Failed to evaluate %s\n",
+				method);
 		return -ENODEV;
 	}
 
@@ -230,7 +211,7 @@ static int get_lcd_level(struct acpi_device *device)
 	unsigned long long state = 0;
 	acpi_status status = AE_OK;
 
-	vdbg_printk(FUJLAPTOP_DBG_TRACE, "get lcd level via GBLL\n");
+	acpi_handle_debug(device->handle, "get lcd level via GBLL\n");
 
 	status = acpi_evaluate_integer(device->handle, "GBLL", NULL, &state);
 	if (ACPI_FAILURE(status))
@@ -247,7 +228,7 @@ static int get_max_brightness(struct acpi_device *device)
 	unsigned long long state = 0;
 	acpi_status status = AE_OK;
 
-	vdbg_printk(FUJLAPTOP_DBG_TRACE, "get max lcd level via RBLL\n");
+	acpi_handle_debug(device->handle, "get max lcd level via RBLL\n");
 
 	status = acpi_evaluate_integer(device->handle, "RBLL", NULL, &state);
 	if (ACPI_FAILURE(status))
@@ -440,8 +421,8 @@ static void acpi_fujitsu_bl_notify(struct acpi_device *device, u32 event)
 	int oldb, newb;
 
 	if (event != ACPI_FUJITSU_NOTIFY_CODE1) {
-		vdbg_printk(FUJLAPTOP_DBG_WARN,
-			    "unsupported event [0x%x]\n", event);
+		acpi_handle_info(device->handle, "unsupported event [0x%x]\n",
+				 event);
 		sparse_keymap_report_event(priv->input, -1, 1, true);
 		return;
 	}
@@ -450,8 +431,8 @@ static void acpi_fujitsu_bl_notify(struct acpi_device *device, u32 event)
 	get_lcd_level(device);
 	newb = priv->brightness_level;
 
-	vdbg_printk(FUJLAPTOP_DBG_TRACE, "brightness button event [%i -> %i]\n",
-		    oldb, newb);
+	acpi_handle_debug(device->handle,
+			  "brightness button event [%i -> %i]\n", oldb, newb);
 
 	if (oldb == newb)
 		return;
@@ -797,7 +778,8 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 	while (call_fext_func(device, FUNC_BUTTONS, 0x1, 0x0, 0x0) != 0
 		&& (i++) < MAX_HOTKEY_RINGBUFFER_SIZE)
 		; /* No action, result is discarded */
-	vdbg_printk(FUJLAPTOP_DBG_INFO, "Discarded %i ringbuffer entries\n", i);
+	acpi_handle_debug(device->handle, "Discarded %i ringbuffer entries\n",
+			  i);
 
 	priv->flags_supported = call_fext_func(device, FUNC_FLAGS, 0x0, 0x0,
 					       0x0);
@@ -812,8 +794,8 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 						   0x0);
 
 	/* Suspect this is a keymap of the application panel, print it */
-	pr_info("BTNI: [0x%x]\n", call_fext_func(device,
-						 FUNC_BUTTONS, 0x0, 0x0, 0x0));
+	acpi_handle_info(device->handle, "BTNI: [0x%x]\n",
+			 call_fext_func(device, FUNC_BUTTONS, 0x0, 0x0, 0x0));
 
 	/* Sync backlight power status */
 	if (fujitsu_bl && fujitsu_bl->bl_device &&
@@ -847,14 +829,14 @@ static void acpi_fujitsu_laptop_press(struct acpi_device *device, int scancode)
 	struct fujitsu_laptop *priv = acpi_driver_data(device);
 
 	if (priv->scancode_count == ARRAY_SIZE(priv->scancode_buf)) {
-		vdbg_printk(FUJLAPTOP_DBG_WARN,
-			    "Could not push scancode [0x%x]\n", scancode);
+		dev_info(&priv->input->dev, "Could not push scancode [0x%x]\n",
+			 scancode);
 		return;
 	}
 	priv->scancode_buf[priv->scancode_count++] = scancode;
 	sparse_keymap_report_event(priv->input, scancode, 1, false);
-	vdbg_printk(FUJLAPTOP_DBG_TRACE,
-		    "Push scancode into buffer [0x%x]\n", scancode);
+	dev_dbg(&priv->input->dev, "Push scancode into buffer [0x%x]\n",
+		scancode);
 }
 
 static void acpi_fujitsu_laptop_release(struct acpi_device *device)
@@ -865,8 +847,8 @@ static void acpi_fujitsu_laptop_release(struct acpi_device *device)
 	while (priv->scancode_count > 0) {
 		scancode = priv->scancode_buf[--priv->scancode_count];
 		sparse_keymap_report_event(priv->input, scancode, 0, false);
-		vdbg_printk(FUJLAPTOP_DBG_TRACE,
-			    "Pop scancode from buffer [0x%x]\n", scancode);
+		dev_dbg(&priv->input->dev, "Pop scancode from buffer [0x%x]\n",
+			scancode);
 	}
 }
 
@@ -877,8 +859,8 @@ static void acpi_fujitsu_laptop_notify(struct acpi_device *device, u32 event)
 	unsigned int irb;
 
 	if (event != ACPI_FUJITSU_NOTIFY_CODE1) {
-		vdbg_printk(FUJLAPTOP_DBG_WARN,
-			    "Unsupported event [0x%x]\n", event);
+		acpi_handle_info(device->handle, "Unsupported event [0x%x]\n",
+				 event);
 		sparse_keymap_report_event(priv->input, -1, 1, true);
 		return;
 	}
@@ -896,8 +878,8 @@ static void acpi_fujitsu_laptop_notify(struct acpi_device *device, u32 event)
 		else if (scancode == 0)
 			acpi_fujitsu_laptop_release(device);
 		else
-			vdbg_printk(FUJLAPTOP_DBG_WARN,
-				    "Unknown GIRB result [%x]\n", irb);
+			acpi_handle_info(device->handle,
+					 "Unknown GIRB result [%x]\n", irb);
 	}
 
 	/* On some models (first seen on the Skylake-based Lifebook
@@ -999,10 +981,6 @@ module_param(use_alt_lcd_levels, int, 0644);
 MODULE_PARM_DESC(use_alt_lcd_levels, "Interface used for setting LCD brightness level (-1 = auto, 0 = force SBLL, 1 = force SBL2)");
 module_param(disable_brightness_adjust, bool, 0644);
 MODULE_PARM_DESC(disable_brightness_adjust, "Disable LCD brightness adjustment");
-#ifdef CONFIG_FUJITSU_LAPTOP_DEBUG
-module_param_named(debug, dbg_level, uint, 0644);
-MODULE_PARM_DESC(debug, "Sets debug level bit-mask");
-#endif
 
 MODULE_AUTHOR("Jonathan Woithe, Peter Gruber, Tony Vroon");
 MODULE_DESCRIPTION("Fujitsu laptop extras support");
-- 
2.13.1

[toc] | [prev] | [next] | [standalone]

#1667417 — [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for storing hotkey scancodes

All ACPI device notify callbacks are invoked using acpi_os_execute(),
which causes the supplied callback to be queued to a static workqueue
which always executes on CPU 0.  This means that there is no possibility
for any ACPI device notify callback to be concurrently executed on
multiple CPUs, which in the case of fujitsu-laptop means that using a
locked kfifo for handling hotkeys is redundant: as hotkey scancodes are
only pushed and popped from within acpi_fujitsu_laptop_notify(), no risk
of concurrent pushing and popping exists.

Instead of a kfifo, use a fixed-size integer table for storing pushed
scancodes and an associated variable holding the number of scancodes
currently stored in that table.  Change debug messages so that they no
longer contain the term "ringbuffer".

In order to simplify implementation, hotkey input device behavior is
slightly changed (from FIFO to LIFO).  The order of release events
generated by the input device should not matter from end user
perspective as upon releasing any hotkey the firmware only produces a
single event which means "all hotkeys were released".

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/fujitsu-laptop.c | 54 +++++++++--------------------------
 1 file changed, 14 insertions(+), 40 deletions(-)

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 1c6fdd952c75..54cb7ae541d4 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -58,7 +58,6 @@
 #include <linux/fb.h>
 #include <linux/input.h>
 #include <linux/input/sparse-keymap.h>
-#include <linux/kfifo.h>
 #include <linux/leds.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
@@ -110,7 +109,6 @@
 #define KEY5_CODE	0x420
 
 #define MAX_HOTKEY_RINGBUFFER_SIZE 100
-#define RINGBUFFERSIZE 40
 
 /* Debugging */
 #define FUJLAPTOP_DBG_ERROR	  0x0001
@@ -146,8 +144,8 @@ struct fujitsu_laptop {
 	struct input_dev *input;
 	char phys[32];
 	struct platform_device *pf_device;
-	struct kfifo fifo;
-	spinlock_t fifo_lock;
+	int scancode_buf[40];
+	int scancode_count;
 	int flags_supported;
 	int flags_state;
 };
@@ -813,23 +811,14 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 	sprintf(acpi_device_class(device), "%s", ACPI_FUJITSU_CLASS);
 	device->driver_data = priv;
 
-	/* kfifo */
-	spin_lock_init(&priv->fifo_lock);
-	error = kfifo_alloc(&priv->fifo, RINGBUFFERSIZE * sizeof(int),
-			    GFP_KERNEL);
-	if (error) {
-		pr_err("kfifo_alloc failed\n");
-		goto err_stop;
-	}
-
 	error = acpi_fujitsu_laptop_input_setup(device);
 	if (error)
-		goto err_free_fifo;
+		return error;
 
 	error = acpi_bus_update_power(device->handle, &state);
 	if (error) {
 		pr_err("Error reading power state\n");
-		goto err_free_fifo;
+		return error;
 	}
 
 	pr_info("ACPI: %s [%s] (%s)\n",
@@ -877,62 +866,47 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 
 	error = acpi_fujitsu_laptop_leds_register(device);
 	if (error)
-		goto err_free_fifo;
+		return error;
 
 	error = fujitsu_laptop_platform_add(device);
 	if (error)
-		goto err_free_fifo;
+		return error;
 
 	return 0;
-
-err_free_fifo:
-	kfifo_free(&priv->fifo);
-err_stop:
-	return error;
 }
 
 static int acpi_fujitsu_laptop_remove(struct acpi_device *device)
 {
-	struct fujitsu_laptop *priv = acpi_driver_data(device);
-
 	fujitsu_laptop_platform_remove(device);
 
-	kfifo_free(&priv->fifo);
-
 	return 0;
 }
 
 static void acpi_fujitsu_laptop_press(struct acpi_device *device, int scancode)
 {
 	struct fujitsu_laptop *priv = acpi_driver_data(device);
-	int status;
 
-	status = kfifo_in_locked(&priv->fifo, (unsigned char *)&scancode,
-				 sizeof(scancode), &priv->fifo_lock);
-	if (status != sizeof(scancode)) {
+	if (priv->scancode_count == ARRAY_SIZE(priv->scancode_buf)) {
 		vdbg_printk(FUJLAPTOP_DBG_WARN,
 			    "Could not push scancode [0x%x]\n", scancode);
 		return;
 	}
+	priv->scancode_buf[priv->scancode_count++] = scancode;
 	sparse_keymap_report_event(priv->input, scancode, 1, false);
 	vdbg_printk(FUJLAPTOP_DBG_TRACE,
-		    "Push scancode into ringbuffer [0x%x]\n", scancode);
+		    "Push scancode into buffer [0x%x]\n", scancode);
 }
 
 static void acpi_fujitsu_laptop_release(struct acpi_device *device)
 {
 	struct fujitsu_laptop *priv = acpi_driver_data(device);
-	int scancode, status;
-
-	while (true) {
-		status = kfifo_out_locked(&priv->fifo,
-					  (unsigned char *)&scancode,
-					  sizeof(scancode), &priv->fifo_lock);
-		if (status != sizeof(scancode))
-			return;
+	int scancode;
+
+	while (priv->scancode_count > 0) {
+		scancode = priv->scancode_buf[--priv->scancode_count];
 		sparse_keymap_report_event(priv->input, scancode, 0, false);
 		vdbg_printk(FUJLAPTOP_DBG_TRACE,
-			    "Pop scancode from ringbuffer [0x%x]\n", scancode);
+			    "Pop scancode from buffer [0x%x]\n", scancode);
 	}
 }
 
-- 
2.13.1

[toc] | [prev] | [next] | [standalone]

#1668577

Hi Michel

On Fri, Jun 16, 2017 at 06:40:51AM +0200, Micha?? K??pie?? wrote:
> In preparation for splitting fujitsu-laptop into two separate modules, I
> prepared two more cleanup series to minimize the amount of code being
> moved around.  This first series contains all patches that touch ACPI in
> some way, which is why I am CCing linux-acpi as per Rafael's previous
> advice.
> 
> This patch series was based on platform-driver-x86/for-next and tested
> on a Lifebook S7020.

As far as I can tell this series looks good.  It is, as you said, another
step towards the splitting of this driver into two separate modules.  I have
a few brief comments.

Patch 1: 
 - The argument for this change assumes that the execution methodology of
   the acpi_os_execute() remains as is, since this is what guarantees the
   absence of concurrency concerns.  If it is fair to assume that we don't
   have to worry about such things which may never happen then there's no
   problem with this patch.

Patch 2:
 - No problem.

Patch 3: 
 - No problem.

Patch 4: 
 - Presumedly the change in device path doesn't count as an API change. 
   Given that, I have no objection since the argument put forward makes
   sense.

Patch 5:
 - I am not overly familiar with the power management infrastructure within 
   the ACPI subsystem, but the presented argument seems sensible to me. 
   This patch is therefore fine if the given assumptions hold.

Patch 6:
 - This seems fair.

Patch 7: 
 - The motivations which lead to the creation of the debug Kconfig option
   are now moot.  The elimination of this and the custom logging code is
   therefore entirely justified.

To summarise, I see no major issues with this patch set so long as the minor
details noted above are of no consequence.  Applying this will assist with
the end-goal of splitting the driver into two modules since it will minimise
the changes needed at the time the split is carried out.

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

Regards
  jonathan

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web