[PATCH v2 0/8] fujitsu-laptop: use device-specific data instead of module-wide globals

[PATCH v2 0/8] fujitsu-laptop: use device-specific data instead of module-wide globals

[Michał Kępień]

fujitsu-laptop registers two ACPI drivers that access each other's
module-wide structures.  To improve data encapsulation and lay the
groundwork for separating the two aforementioned ACPI drivers into
separate modules, move away from module-wide global data structures by
using device-specific data instead.

To avoid breaking a working feature (backlight power synchronization
upon module load), this series leaves the module-wide struct fujitsu_bl
in place.  It will be taken care of when the backlight driver is split
into a separate module.

As we agreed that grabbing a handle to an ACPI device using its absolute
path is not a truly elegant solution, this series uses a different
approach to call_fext_func() than v1.  By passing that function a
pointer to a struct acpi_device instead of an acpi_handle, all relevant
static functions of the module will now use the same type for their
first argument and the acpi_handle fields of both module-wide structures
are removed altogether.

This patch series was tested on a Lifebook S7020 and a Lifebook E744.

As with v1, adhering to the "one logical change per patch" rule was
tricky.  If the changes introduced are illegible, I will be happy to
further explain and/or improve the series.  Using --color-words should
make reviewing much more manageable.

Changes from v1:

  - Drop patch 01/10 from v1, i.e. do not introduce fext_*() helper
    functions.

  - Drop patch 02/10 from v1 as the acpi_handle fields of both
    module-wide structures are removed altogether by other patches.

  - Replace patch 03/10 from v1 with patch 6/8, passing call_fext_func()
    a pointer to struct acpi_device instead of an acpi_handle.

  - Drop patch 04/10 from v1, thus deferring driver separation until the
    split into separate modules.  Consider patch 5/8 a partial spiritual
    successor ;)  More information is available in the commit message of
    that patch.

  - Add an additional check to patch 2/8 to avoid a NULL dereference
    which could happen due to patch 04/10 from v1 being dropped.

  - Do not store ACPI handles in private structures.  Instead, extract
    them directly from struct acpi_device pointers passed as function
    arguments.

  - Updated commit messages.

  - As the above might be a bit confusing, here is the patch number
    mapping from v1 to v2:

        v1    | v2
	------+--------
        01/10 | dropped
        02/10 | dropped
        03/10 | 6/8
        04/10 | 5/8
        05/10 | 1/8
        06/10 | 2/8
        07/10 | 3/8
        08/10 | 4/8
        09/10 | 7/8
        10/10 | 8/8

 drivers/platform/x86/fujitsu-laptop.c | 417 +++++++++++++++++-----------------
 1 file changed, 213 insertions(+), 204 deletions(-)

-- 
2.13.0

[PATCH v2 1/8] platform/x86: fujitsu-laptop: distinguish current uses of device-specific data

[Michał Kępień]

In portions of the driver which use device-specific data, rename local
variables from fujitsu_bl and fujitsu_laptop to priv in order to clearly
distinguish these parts from code that uses module-wide data.

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

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 7f49d92914c9..024ef339179a 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -348,26 +348,26 @@ static const struct key_entry keymap_backlight[] = {
 
 static int acpi_fujitsu_bl_input_setup(struct acpi_device *device)
 {
-	struct fujitsu_bl *fujitsu_bl = acpi_driver_data(device);
+	struct fujitsu_bl *priv = acpi_driver_data(device);
 	int ret;
 
-	fujitsu_bl->input = devm_input_allocate_device(&device->dev);
-	if (!fujitsu_bl->input)
+	priv->input = devm_input_allocate_device(&device->dev);
+	if (!priv->input)
 		return -ENOMEM;
 
-	snprintf(fujitsu_bl->phys, sizeof(fujitsu_bl->phys),
-		 "%s/video/input0", acpi_device_hid(device));
+	snprintf(priv->phys, sizeof(priv->phys), "%s/video/input0",
+		 acpi_device_hid(device));
 
-	fujitsu_bl->input->name = acpi_device_name(device);
-	fujitsu_bl->input->phys = fujitsu_bl->phys;
-	fujitsu_bl->input->id.bustype = BUS_HOST;
-	fujitsu_bl->input->id.product = 0x06;
+	priv->input->name = acpi_device_name(device);
+	priv->input->phys = priv->phys;
+	priv->input->id.bustype = BUS_HOST;
+	priv->input->id.product = 0x06;
 
-	ret = sparse_keymap_setup(fujitsu_bl->input, keymap_backlight, NULL);
+	ret = sparse_keymap_setup(priv->input, keymap_backlight, NULL);
 	if (ret)
 		return ret;
 
-	return input_register_device(fujitsu_bl->input);
+	return input_register_device(priv->input);
 }
 
 static int fujitsu_backlight_register(struct acpi_device *device)
@@ -541,27 +541,27 @@ static const struct dmi_system_id fujitsu_laptop_dmi_table[] = {
 
 static int acpi_fujitsu_laptop_input_setup(struct acpi_device *device)
 {
-	struct fujitsu_laptop *fujitsu_laptop = acpi_driver_data(device);
+	struct fujitsu_laptop *priv = acpi_driver_data(device);
 	int ret;
 
-	fujitsu_laptop->input = devm_input_allocate_device(&device->dev);
-	if (!fujitsu_laptop->input)
+	priv->input = devm_input_allocate_device(&device->dev);
+	if (!priv->input)
 		return -ENOMEM;
 
-	snprintf(fujitsu_laptop->phys, sizeof(fujitsu_laptop->phys),
-		 "%s/video/input0", acpi_device_hid(device));
+	snprintf(priv->phys, sizeof(priv->phys), "%s/video/input0",
+		 acpi_device_hid(device));
 
-	fujitsu_laptop->input->name = acpi_device_name(device);
-	fujitsu_laptop->input->phys = fujitsu_laptop->phys;
-	fujitsu_laptop->input->id.bustype = BUS_HOST;
-	fujitsu_laptop->input->id.product = 0x06;
+	priv->input->name = acpi_device_name(device);
+	priv->input->phys = priv->phys;
+	priv->input->id.bustype = BUS_HOST;
+	priv->input->id.product = 0x06;
 
 	dmi_check_system(fujitsu_laptop_dmi_table);
-	ret = sparse_keymap_setup(fujitsu_laptop->input, keymap, NULL);
+	ret = sparse_keymap_setup(priv->input, keymap, NULL);
 	if (ret)
 		return ret;
 
-	return input_register_device(fujitsu_laptop->input);
+	return input_register_device(priv->input);
 }
 
 static int fujitsu_laptop_platform_add(void)
@@ -863,11 +863,11 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 
 static int acpi_fujitsu_laptop_remove(struct acpi_device *device)
 {
-	struct fujitsu_laptop *fujitsu_laptop = acpi_driver_data(device);
+	struct fujitsu_laptop *priv = acpi_driver_data(device);
 
 	fujitsu_laptop_platform_remove();
 
-	kfifo_free(&fujitsu_laptop->fifo);
+	kfifo_free(&priv->fifo);
 
 	return 0;
 }
-- 
2.13.0

[PATCH v2 2/8] platform/x86: fujitsu-laptop: allocate struct fujitsu_bl in acpi_fujitsu_bl_add()

[Michał Kępień]

Only allocate memory for struct fujitsu_bl when the FUJ02B1 ACPI device
is present.  Use devm_kzalloc() for allocating memory to simplify
cleanup.

Due to the fact that the power property of the backlight device created
by the backlight driver is accessed from acpi_fujitsu_laptop_add(),
pointer to the allocated memory will remain stored in a module-wide
variable until the backlight driver is extracted into a separate module.

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

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 024ef339179a..63a9b98967fa 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -392,6 +392,7 @@ 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;
 
@@ -401,10 +402,15 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device)
 	if (!device)
 		return -EINVAL;
 
+	priv = devm_kzalloc(&device->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	fujitsu_bl = priv;
 	fujitsu_bl->acpi_handle = device->handle;
 	sprintf(acpi_device_name(device), "%s", ACPI_FUJITSU_BL_DEVICE_NAME);
 	sprintf(acpi_device_class(device), "%s", ACPI_FUJITSU_CLASS);
-	device->driver_data = fujitsu_bl;
+	device->driver_data = priv;
 
 	error = acpi_fujitsu_bl_input_setup(device);
 	if (error)
@@ -837,7 +843,7 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 	pr_info("BTNI: [0x%x]\n", call_fext_func(FUNC_BUTTONS, 0x0, 0x0, 0x0));
 
 	/* Sync backlight power status */
-	if (fujitsu_bl->bl_device &&
+	if (fujitsu_bl && fujitsu_bl->bl_device &&
 	    acpi_video_get_backlight_type() == acpi_backlight_vendor) {
 		if (call_fext_func(FUNC_BACKLIGHT, 0x2, 0x4, 0x0) == 3)
 			fujitsu_bl->bl_device->props.power = FB_BLANK_POWERDOWN;
@@ -995,13 +1001,9 @@ static int __init fujitsu_init(void)
 	if (acpi_disabled)
 		return -ENODEV;
 
-	fujitsu_bl = kzalloc(sizeof(struct fujitsu_bl), GFP_KERNEL);
-	if (!fujitsu_bl)
-		return -ENOMEM;
-
 	ret = acpi_bus_register_driver(&acpi_fujitsu_bl_driver);
 	if (ret)
-		goto err_free_fujitsu_bl;
+		return ret;
 
 	/* Register platform stuff */
 
@@ -1031,8 +1033,6 @@ static int __init fujitsu_init(void)
 	platform_driver_unregister(&fujitsu_pf_driver);
 err_unregister_acpi:
 	acpi_bus_unregister_driver(&acpi_fujitsu_bl_driver);
-err_free_fujitsu_bl:
-	kfree(fujitsu_bl);
 
 	return ret;
 }
@@ -1047,8 +1047,6 @@ static void __exit fujitsu_cleanup(void)
 
 	acpi_bus_unregister_driver(&acpi_fujitsu_bl_driver);
 
-	kfree(fujitsu_bl);
-
 	pr_info("driver unloaded\n");
 }
 
-- 
2.13.0

[PATCH v2 3/8] platform/x86: fujitsu-laptop: use device-specific data in backlight code

[Michał Kępień]

To prevent using module-wide data in backlight-related code, employ
acpi_driver_data() and bl_get_data() where possible to fetch
device-specific data to work on in each function.  This makes the input
local variable in acpi_fujitsu_bl_notify() and the acpi_handle field of
struct fujitsu_bl redundant, so remove them.

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

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 63a9b98967fa..1124070aad2d 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -130,7 +130,6 @@
 
 /* Device controlling the backlight and associated keys */
 struct fujitsu_bl {
-	acpi_handle acpi_handle;
 	struct input_dev *input;
 	char phys[32];
 	struct backlight_device *bl_device;
@@ -189,14 +188,15 @@ static int call_fext_func(int func, int op, int feature, int state)
 
 /* Hardware access for LCD brightness control */
 
-static int set_lcd_level(int level)
+static int set_lcd_level(struct acpi_device *device, int level)
 {
+	struct fujitsu_bl *priv = acpi_driver_data(device);
 	acpi_status status;
 	char *method;
 
 	switch (use_alt_lcd_levels) {
 	case -1:
-		if (acpi_has_method(fujitsu_bl->acpi_handle, "SBL2"))
+		if (acpi_has_method(device->handle, "SBL2"))
 			method = "SBL2";
 		else
 			method = "SBLL";
@@ -212,71 +212,74 @@ static int set_lcd_level(int level)
 	vdbg_printk(FUJLAPTOP_DBG_TRACE, "set lcd level via %s [%d]\n",
 		    method, level);
 
-	if (level < 0 || level >= fujitsu_bl->max_brightness)
+	if (level < 0 || level >= priv->max_brightness)
 		return -EINVAL;
 
-	status = acpi_execute_simple_method(fujitsu_bl->acpi_handle, method,
-					    level);
+	status = acpi_execute_simple_method(device->handle, method, level);
 	if (ACPI_FAILURE(status)) {
 		vdbg_printk(FUJLAPTOP_DBG_ERROR, "Failed to evaluate %s\n",
 			    method);
 		return -ENODEV;
 	}
 
-	fujitsu_bl->brightness_level = level;
+	priv->brightness_level = level;
 
 	return 0;
 }
 
-static int get_lcd_level(void)
+static int get_lcd_level(struct acpi_device *device)
 {
+	struct fujitsu_bl *priv = acpi_driver_data(device);
 	unsigned long long state = 0;
 	acpi_status status = AE_OK;
 
 	vdbg_printk(FUJLAPTOP_DBG_TRACE, "get lcd level via GBLL\n");
 
-	status = acpi_evaluate_integer(fujitsu_bl->acpi_handle, "GBLL", NULL,
-				       &state);
+	status = acpi_evaluate_integer(device->handle, "GBLL", NULL, &state);
 	if (ACPI_FAILURE(status))
 		return 0;
 
-	fujitsu_bl->brightness_level = state & 0x0fffffff;
+	priv->brightness_level = state & 0x0fffffff;
 
-	return fujitsu_bl->brightness_level;
+	return priv->brightness_level;
 }
 
-static int get_max_brightness(void)
+static int get_max_brightness(struct acpi_device *device)
 {
+	struct fujitsu_bl *priv = acpi_driver_data(device);
 	unsigned long long state = 0;
 	acpi_status status = AE_OK;
 
 	vdbg_printk(FUJLAPTOP_DBG_TRACE, "get max lcd level via RBLL\n");
 
-	status = acpi_evaluate_integer(fujitsu_bl->acpi_handle, "RBLL", NULL,
-				       &state);
+	status = acpi_evaluate_integer(device->handle, "RBLL", NULL, &state);
 	if (ACPI_FAILURE(status))
 		return -1;
 
-	fujitsu_bl->max_brightness = state;
+	priv->max_brightness = state;
 
-	return fujitsu_bl->max_brightness;
+	return priv->max_brightness;
 }
 
 /* Backlight device stuff */
 
 static int bl_get_brightness(struct backlight_device *b)
 {
-	return b->props.power == FB_BLANK_POWERDOWN ? 0 : get_lcd_level();
+	struct acpi_device *device = bl_get_data(b);
+
+	return b->props.power == FB_BLANK_POWERDOWN ? 0 : get_lcd_level(device);
 }
 
 static int bl_update_status(struct backlight_device *b)
 {
+	struct acpi_device *device = bl_get_data(b);
+
 	if (b->props.power == FB_BLANK_POWERDOWN)
 		call_fext_func(FUNC_BACKLIGHT, 0x1, 0x4, 0x3);
 	else
 		call_fext_func(FUNC_BACKLIGHT, 0x1, 0x4, 0x0);
 
-	return set_lcd_level(b->props.brightness);
+	return set_lcd_level(device, b->props.brightness);
 }
 
 static const struct backlight_ops fujitsu_bl_ops = {
@@ -372,20 +375,21 @@ static int acpi_fujitsu_bl_input_setup(struct acpi_device *device)
 
 static int fujitsu_backlight_register(struct acpi_device *device)
 {
+	struct fujitsu_bl *priv = acpi_driver_data(device);
 	const struct backlight_properties props = {
-		.brightness = fujitsu_bl->brightness_level,
-		.max_brightness = fujitsu_bl->max_brightness - 1,
+		.brightness = priv->brightness_level,
+		.max_brightness = priv->max_brightness - 1,
 		.type = BACKLIGHT_PLATFORM
 	};
 	struct backlight_device *bd;
 
 	bd = devm_backlight_device_register(&device->dev, "fujitsu-laptop",
-					    &device->dev, NULL,
+					    &device->dev, device,
 					    &fujitsu_bl_ops, &props);
 	if (IS_ERR(bd))
 		return PTR_ERR(bd);
 
-	fujitsu_bl->bl_device = bd;
+	priv->bl_device = bd;
 
 	return 0;
 }
@@ -407,7 +411,6 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device)
 		return -ENOMEM;
 
 	fujitsu_bl = priv;
-	fujitsu_bl->acpi_handle = device->handle;
 	sprintf(acpi_device_name(device), "%s", ACPI_FUJITSU_BL_DEVICE_NAME);
 	sprintf(acpi_device_class(device), "%s", ACPI_FUJITSU_CLASS);
 	device->driver_data = priv;
@@ -416,7 +419,7 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device)
 	if (error)
 		return error;
 
-	error = acpi_bus_update_power(fujitsu_bl->acpi_handle, &state);
+	error = acpi_bus_update_power(device->handle, &state);
 	if (error) {
 		pr_err("Error reading power state\n");
 		return error;
@@ -434,9 +437,9 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device)
 			pr_err("_INI Method failed\n");
 	}
 
-	if (get_max_brightness() <= 0)
-		fujitsu_bl->max_brightness = FUJITSU_LCD_N_LEVELS;
-	get_lcd_level();
+	if (get_max_brightness(device) <= 0)
+		priv->max_brightness = FUJITSU_LCD_N_LEVELS;
+	get_lcd_level(device);
 
 	error = fujitsu_backlight_register(device);
 	if (error)
@@ -449,21 +452,19 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device)
 
 static void acpi_fujitsu_bl_notify(struct acpi_device *device, u32 event)
 {
-	struct input_dev *input;
+	struct fujitsu_bl *priv = acpi_driver_data(device);
 	int oldb, newb;
 
-	input = fujitsu_bl->input;
-
 	if (event != ACPI_FUJITSU_NOTIFY_CODE1) {
 		vdbg_printk(FUJLAPTOP_DBG_WARN,
 			    "unsupported event [0x%x]\n", event);
-		sparse_keymap_report_event(input, -1, 1, true);
+		sparse_keymap_report_event(priv->input, -1, 1, true);
 		return;
 	}
 
-	oldb = fujitsu_bl->brightness_level;
-	get_lcd_level();
-	newb = fujitsu_bl->brightness_level;
+	oldb = priv->brightness_level;
+	get_lcd_level(device);
+	newb = priv->brightness_level;
 
 	vdbg_printk(FUJLAPTOP_DBG_TRACE, "brightness button event [%i -> %i]\n",
 		    oldb, newb);
@@ -472,9 +473,9 @@ static void acpi_fujitsu_bl_notify(struct acpi_device *device, u32 event)
 		return;
 
 	if (!disable_brightness_adjust)
-		set_lcd_level(newb);
+		set_lcd_level(device, newb);
 
-	sparse_keymap_report_event(input, oldb < newb, 1, true);
+	sparse_keymap_report_event(priv->input, oldb < newb, 1, true);
 }
 
 /* ACPI device for hotkey handling */
-- 
2.13.0

[PATCH v2 4/8] platform/x86: fujitsu-laptop: allocate struct fujitsu_laptop in acpi_fujitsu_laptop_add()

[Michał Kępień]

Only allocate memory for struct fujitsu_laptop when the FUJ02E3 ACPI
device is present.  Use devm_kzalloc() for allocating memory to simplify
cleanup.

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

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 1124070aad2d..3916f0ae59f3 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -776,6 +776,7 @@ 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;
@@ -783,11 +784,16 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 	if (!device)
 		return -EINVAL;
 
+	priv = devm_kzalloc(&device->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	fujitsu_laptop = priv;
 	fujitsu_laptop->acpi_handle = device->handle;
 	sprintf(acpi_device_name(device), "%s",
 		ACPI_FUJITSU_LAPTOP_DEVICE_NAME);
 	sprintf(acpi_device_class(device), "%s", ACPI_FUJITSU_CLASS);
-	device->driver_data = fujitsu_laptop;
+	device->driver_data = priv;
 
 	/* kfifo */
 	spin_lock_init(&fujitsu_laptop->fifo_lock);
@@ -1014,22 +1020,14 @@ static int __init fujitsu_init(void)
 
 	/* Register laptop driver */
 
-	fujitsu_laptop = kzalloc(sizeof(struct fujitsu_laptop), GFP_KERNEL);
-	if (!fujitsu_laptop) {
-		ret = -ENOMEM;
-		goto err_unregister_platform_driver;
-	}
-
 	ret = acpi_bus_register_driver(&acpi_fujitsu_laptop_driver);
 	if (ret)
-		goto err_free_fujitsu_laptop;
+		goto err_unregister_platform_driver;
 
 	pr_info("driver " FUJITSU_DRIVER_VERSION " successfully loaded\n");
 
 	return 0;
 
-err_free_fujitsu_laptop:
-	kfree(fujitsu_laptop);
 err_unregister_platform_driver:
 	platform_driver_unregister(&fujitsu_pf_driver);
 err_unregister_acpi:
@@ -1042,8 +1040,6 @@ static void __exit fujitsu_cleanup(void)
 {
 	acpi_bus_unregister_driver(&acpi_fujitsu_laptop_driver);
 
-	kfree(fujitsu_laptop);
-
 	platform_driver_unregister(&fujitsu_pf_driver);
 
 	acpi_bus_unregister_driver(&acpi_fujitsu_bl_driver);
-- 
2.13.0

[PATCH v2 5/8] platform/x86: fujitsu-laptop: track the last instantiated FUJ02E3 ACPI device

[Michał Kępień]

fujitsu-laptop registers two ACPI drivers: one for ACPI device FUJ02B1
enabling backlight control and another for ACPI device FUJ02E3 which
handles various other stuff (hotkeys, LEDs, etc.)  In a perfect world,
private data used by each of these drivers would be neatly encapsulated
in a structure specific to a given driver instance.  Sadly, firmware
present on some Fujitsu laptops makes that impossible by exposing
backlight power control (which is what the FUJ02B1 ACPI device should
take care of) through the FUJ02E3 ACPI device.  This means the backlight
driver needs a way to access an ACPI device it is not bound to.  When
the backlight driver is extracted into a separate module, it will not be
able to rely on a module-wide variable any more and such access will
happen through an API exposed by fujitsu-laptop.

For all known firmwares out in the wild, it seems that whenever the
FUJ02B1 ACPI device is present, it is always accompanied by a single
instance of the FUJ02E3 ACPI device.  We could independently grab an
ACPI handle to the FUJ02E3 ACPI device from the backlight driver, but
that would require using a hardcoded absolute path to that ACPI device,
which is subject to change.  It is easier to simply store a module-wide
pointer to the last (most likely only) FUJ02E3 ACPI device found, make
the aforementioned API use it and cover our bases by warning the user if
firmware exposes multiple FUJ02E3 ACPI devices.

Introducing this pointer in advance allows us to get rid of the
acpi_handle field of struct fujitsu_bl and also enables a bit more
step-by-step migration to a device-specific implementation of
call_fext_func().

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

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 3916f0ae59f3..21bd60afea75 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -155,6 +155,7 @@ struct fujitsu_laptop {
 };
 
 static struct fujitsu_laptop *fujitsu_laptop;
+static struct acpi_device *fext;
 
 #ifdef CONFIG_FUJITSU_LAPTOP_DEBUG
 static u32 dbg_level = 0x03;
@@ -788,6 +789,9 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 	if (!priv)
 		return -ENOMEM;
 
+	WARN_ONCE(fext, "More than one FUJ02E3 ACPI device was found.  Driver may not work as intended.");
+	fext = device;
+
 	fujitsu_laptop = priv;
 	fujitsu_laptop->acpi_handle = device->handle;
 	sprintf(acpi_device_name(device), "%s",
-- 
2.13.0

Re: [PATCH v2 5/8] platform/x86: fujitsu-laptop: track the last instantiated FUJ02E3 ACPI device

[Jonathan Woithe]

On Fri, May 19, 2017 at 09:44:45AM +0200, Micha?? K??pie?? wrote:
> It is easier to simply store a module-wide
> pointer to the last (most likely only) FUJ02E3 ACPI device found, make
> the aforementioned API use it and cover our bases by warning the user if
> firmware exposes multiple FUJ02E3 ACPI devices.
> :
> @@ -788,6 +789,9 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
>  	if (!priv)
>  		return -ENOMEM;
>  
> +	WARN_ONCE(fext, "More than one FUJ02E3 ACPI device was found.  Driver may not work as intended.");
> +	fext = device;
> +
>  	fujitsu_laptop = priv;
>  	fujitsu_laptop->acpi_handle = device->handle;
>  	sprintf(acpi_device_name(device), "%s",

I thought WARN_ONCE() printed the warning when it was encountered for the
first time and then suppressed it on all other occasions.  If this is true
then we'll get the warning even when there is only one FUJ02E3 in the
system.  Am I missing something?

Regards
  jonathan

Re: [PATCH v2 5/8] platform/x86: fujitsu-laptop: track the last instantiated FUJ02E3 ACPI device

[Michał Kępień]

> On Fri, May 19, 2017 at 09:44:45AM +0200, Micha?? K??pie?? wrote:
> > It is easier to simply store a module-wide
> > pointer to the last (most likely only) FUJ02E3 ACPI device found, make
> > the aforementioned API use it and cover our bases by warning the user if
> > firmware exposes multiple FUJ02E3 ACPI devices.
> > :
> > @@ -788,6 +789,9 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
> >  	if (!priv)
> >  		return -ENOMEM;
> >  
> > +	WARN_ONCE(fext, "More than one FUJ02E3 ACPI device was found.  Driver may not work as intended.");
> > +	fext = device;
> > +
> >  	fujitsu_laptop = priv;
> >  	fujitsu_laptop->acpi_handle = device->handle;
> >  	sprintf(acpi_device_name(device), "%s",
> 
> I thought WARN_ONCE() printed the warning when it was encountered for the
> first time and then suppressed it on all other occasions.

Correct.

> If this is true
> then we'll get the warning even when there is only one FUJ02E3 in the
> system.  Am I missing something?

Probably the fact that the first argument of the macro is a conditional
expression ("fext" is functionally equivalent to "fext != NULL" in this
case).  In other words, the warning will only ever be issued if
acpi_fujitsu_laptop_add() is called at least twice - note the assignment
on the line following WARN_ONCE().

-- 
Best regards,
Michał Kępień

Re: [PATCH v2 5/8] platform/x86: fujitsu-laptop: track the last instantiated FUJ02E3 ACPI device

[Jonathan Woithe]

On Tue, May 23, 2017 at 11:47:06PM +0200, Micha?? K??pie?? wrote:
> > On Fri, May 19, 2017 at 09:44:45AM +0200, Micha?? K??pie?? wrote:
> > > It is easier to simply store a module-wide
> > > pointer to the last (most likely only) FUJ02E3 ACPI device found, make
> > > the aforementioned API use it and cover our bases by warning the user if
> > > firmware exposes multiple FUJ02E3 ACPI devices.
> > > :
> > > @@ -788,6 +789,9 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
> > >  	if (!priv)
> > >  		return -ENOMEM;
> > >  
> > > +	WARN_ONCE(fext, "More than one FUJ02E3 ACPI device was found.  Driver may not work as intended.");
> > > +	fext = device;
> > > +
> > >  	fujitsu_laptop = priv;
> > >  	fujitsu_laptop->acpi_handle = device->handle;
> > >  	sprintf(acpi_device_name(device), "%s",
> > 
> > I thought WARN_ONCE() printed the warning when it was encountered for the
> > first time and then suppressed it on all other occasions.
> 
> Correct.
> 
> > If this is true
> > then we'll get the warning even when there is only one FUJ02E3 in the
> > system.  Am I missing something?
> 
> Probably the fact that the first argument of the macro is a conditional
> expression ...

Ah (having now had an opportunity to look at the source of WARN_ONCE()),
it's tested within WARN_ONCE.

> ("fext" is functionally equivalent to "fext != NULL" in this case).

Indeed, by virtue of the test done in WARN_ONCE().  Ok, that makes sense.

Regards
  jonathan

[PATCH v2 6/8] platform/x86: fujitsu-laptop: explicitly pass ACPI device to call_fext_func()

[Michał Kępień]

Prepare for not using module-wide data in call_fext_func() by explicitly
passing it a pointer to struct acpi_device while still using a
module-wide pointer in each call.

Doing this enables call_fext_func() to fetch the ACPI handle from its
argument, making the acpi_handle field of struct fujitsu_laptop useless,
so remove that field.  While we are at it, the dev field of the same
structure is assigned in acpi_fujitsu_laptop_add() but not used for
anything, so remove it as well.

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

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 21bd60afea75..b78c97ebfee4 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -143,8 +143,6 @@ static bool disable_brightness_adjust;
 
 /* Device used to access hotkeys and other features on the laptop */
 struct fujitsu_laptop {
-	acpi_handle acpi_handle;
-	struct acpi_device *dev;
 	struct input_dev *input;
 	char phys[32];
 	struct platform_device *pf_device;
@@ -163,7 +161,8 @@ static u32 dbg_level = 0x03;
 
 /* Fujitsu ACPI interface function */
 
-static int call_fext_func(int func, int op, int feature, int state)
+static int call_fext_func(struct acpi_device *device,
+			  int func, int op, int feature, int state)
 {
 	union acpi_object params[4] = {
 		{ .integer.type = ACPI_TYPE_INTEGER, .integer.value = func },
@@ -175,8 +174,8 @@ static int call_fext_func(int func, int op, int feature, int state)
 	unsigned long long value;
 	acpi_status status;
 
-	status = acpi_evaluate_integer(fujitsu_laptop->acpi_handle, "FUNC",
-				       &arg_list, &value);
+	status = acpi_evaluate_integer(device->handle, "FUNC", &arg_list,
+				       &value);
 	if (ACPI_FAILURE(status)) {
 		vdbg_printk(FUJLAPTOP_DBG_ERROR, "Failed to evaluate FUNC\n");
 		return -ENODEV;
@@ -276,9 +275,9 @@ static int bl_update_status(struct backlight_device *b)
 	struct acpi_device *device = bl_get_data(b);
 
 	if (b->props.power == FB_BLANK_POWERDOWN)
-		call_fext_func(FUNC_BACKLIGHT, 0x1, 0x4, 0x3);
+		call_fext_func(fext, FUNC_BACKLIGHT, 0x1, 0x4, 0x3);
 	else
-		call_fext_func(FUNC_BACKLIGHT, 0x1, 0x4, 0x0);
+		call_fext_func(fext, FUNC_BACKLIGHT, 0x1, 0x4, 0x0);
 
 	return set_lcd_level(device, b->props.brightness);
 }
@@ -618,22 +617,22 @@ static int logolamp_set(struct led_classdev *cdev,
 	if (brightness < LED_FULL)
 		always = FUNC_LED_OFF;
 
-	ret = call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_POWERON, poweron);
+	ret = call_fext_func(fext, FUNC_LEDS, 0x1, LOGOLAMP_POWERON, poweron);
 	if (ret < 0)
 		return ret;
 
-	return call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_ALWAYS, always);
+	return call_fext_func(fext, FUNC_LEDS, 0x1, LOGOLAMP_ALWAYS, always);
 }
 
 static enum led_brightness logolamp_get(struct led_classdev *cdev)
 {
 	int ret;
 
-	ret = call_fext_func(FUNC_LEDS, 0x2, LOGOLAMP_ALWAYS, 0x0);
+	ret = call_fext_func(fext, FUNC_LEDS, 0x2, LOGOLAMP_ALWAYS, 0x0);
 	if (ret == FUNC_LED_ON)
 		return LED_FULL;
 
-	ret = call_fext_func(FUNC_LEDS, 0x2, LOGOLAMP_POWERON, 0x0);
+	ret = call_fext_func(fext, FUNC_LEDS, 0x2, LOGOLAMP_POWERON, 0x0);
 	if (ret == FUNC_LED_ON)
 		return LED_HALF;
 
@@ -650,10 +649,10 @@ static int kblamps_set(struct led_classdev *cdev,
 		       enum led_brightness brightness)
 {
 	if (brightness >= LED_FULL)
-		return call_fext_func(FUNC_LEDS, 0x1, KEYBOARD_LAMPS,
+		return call_fext_func(fext, FUNC_LEDS, 0x1, KEYBOARD_LAMPS,
 				      FUNC_LED_ON);
 	else
-		return call_fext_func(FUNC_LEDS, 0x1, KEYBOARD_LAMPS,
+		return call_fext_func(fext, FUNC_LEDS, 0x1, KEYBOARD_LAMPS,
 				      FUNC_LED_OFF);
 }
 
@@ -661,7 +660,8 @@ static enum led_brightness kblamps_get(struct led_classdev *cdev)
 {
 	enum led_brightness brightness = LED_OFF;
 
-	if (call_fext_func(FUNC_LEDS, 0x2, KEYBOARD_LAMPS, 0x0) == FUNC_LED_ON)
+	if (call_fext_func(fext,
+			   FUNC_LEDS, 0x2, KEYBOARD_LAMPS, 0x0) == FUNC_LED_ON)
 		brightness = LED_FULL;
 
 	return brightness;
@@ -677,17 +677,18 @@ static int radio_led_set(struct led_classdev *cdev,
 			 enum led_brightness brightness)
 {
 	if (brightness >= LED_FULL)
-		return call_fext_func(FUNC_FLAGS, 0x5, RADIO_LED_ON,
+		return call_fext_func(fext, FUNC_FLAGS, 0x5, RADIO_LED_ON,
 				      RADIO_LED_ON);
 	else
-		return call_fext_func(FUNC_FLAGS, 0x5, RADIO_LED_ON, 0x0);
+		return call_fext_func(fext, FUNC_FLAGS, 0x5, RADIO_LED_ON,
+				      0x0);
 }
 
 static enum led_brightness radio_led_get(struct led_classdev *cdev)
 {
 	enum led_brightness brightness = LED_OFF;
 
-	if (call_fext_func(FUNC_FLAGS, 0x4, 0x0, 0x0) & RADIO_LED_ON)
+	if (call_fext_func(fext, FUNC_FLAGS, 0x4, 0x0, 0x0) & RADIO_LED_ON)
 		brightness = LED_FULL;
 
 	return brightness;
@@ -705,12 +706,12 @@ static int eco_led_set(struct led_classdev *cdev,
 {
 	int curr;
 
-	curr = call_fext_func(FUNC_LEDS, 0x2, ECO_LED, 0x0);
+	curr = call_fext_func(fext, FUNC_LEDS, 0x2, ECO_LED, 0x0);
 	if (brightness >= LED_FULL)
-		return call_fext_func(FUNC_LEDS, 0x1, ECO_LED,
+		return call_fext_func(fext, FUNC_LEDS, 0x1, ECO_LED,
 				      curr | ECO_LED_ON);
 	else
-		return call_fext_func(FUNC_LEDS, 0x1, ECO_LED,
+		return call_fext_func(fext, FUNC_LEDS, 0x1, ECO_LED,
 				      curr & ~ECO_LED_ON);
 }
 
@@ -718,7 +719,7 @@ static enum led_brightness eco_led_get(struct led_classdev *cdev)
 {
 	enum led_brightness brightness = LED_OFF;
 
-	if (call_fext_func(FUNC_LEDS, 0x2, ECO_LED, 0x0) & ECO_LED_ON)
+	if (call_fext_func(fext, FUNC_LEDS, 0x2, ECO_LED, 0x0) & ECO_LED_ON)
 		brightness = LED_FULL;
 
 	return brightness;
@@ -734,15 +735,17 @@ static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
 {
 	int result;
 
-	if (call_fext_func(FUNC_LEDS, 0x0, 0x0, 0x0) & LOGOLAMP_POWERON) {
+	if (call_fext_func(fext,
+			   FUNC_LEDS, 0x0, 0x0, 0x0) & LOGOLAMP_POWERON) {
 		result = devm_led_classdev_register(&device->dev,
 						    &logolamp_led);
 		if (result)
 			return result;
 	}
 
-	if ((call_fext_func(FUNC_LEDS, 0x0, 0x0, 0x0) & KEYBOARD_LAMPS) &&
-	    (call_fext_func(FUNC_BUTTONS, 0x0, 0x0, 0x0) == 0x0)) {
+	if ((call_fext_func(fext,
+			    FUNC_LEDS, 0x0, 0x0, 0x0) & KEYBOARD_LAMPS) &&
+	    (call_fext_func(fext, FUNC_BUTTONS, 0x0, 0x0, 0x0) == 0x0)) {
 		result = devm_led_classdev_register(&device->dev, &kblamps_led);
 		if (result)
 			return result;
@@ -754,7 +757,7 @@ static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
 	 * to also have an RF LED.  Therefore use bit 24 as an indicator
 	 * that an RF LED is present.
 	 */
-	if (call_fext_func(FUNC_BUTTONS, 0x0, 0x0, 0x0) & BIT(24)) {
+	if (call_fext_func(fext, FUNC_BUTTONS, 0x0, 0x0, 0x0) & BIT(24)) {
 		result = devm_led_classdev_register(&device->dev, &radio_led);
 		if (result)
 			return result;
@@ -765,8 +768,9 @@ static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
 	 * bit 14 seems to indicate presence of said led as well.
 	 * Confirm by testing the status.
 	 */
-	if ((call_fext_func(FUNC_LEDS, 0x0, 0x0, 0x0) & BIT(14)) &&
-	    (call_fext_func(FUNC_LEDS, 0x2, ECO_LED, 0x0) != UNSUPPORTED_CMD)) {
+	if ((call_fext_func(fext, FUNC_LEDS, 0x0, 0x0, 0x0) & BIT(14)) &&
+	    (call_fext_func(fext,
+			    FUNC_LEDS, 0x2, ECO_LED, 0x0) != UNSUPPORTED_CMD)) {
 		result = devm_led_classdev_register(&device->dev, &eco_led);
 		if (result)
 			return result;
@@ -793,7 +797,6 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 	fext = device;
 
 	fujitsu_laptop = priv;
-	fujitsu_laptop->acpi_handle = device->handle;
 	sprintf(acpi_device_name(device), "%s",
 		ACPI_FUJITSU_LAPTOP_DEVICE_NAME);
 	sprintf(acpi_device_class(device), "%s", ACPI_FUJITSU_CLASS);
@@ -812,7 +815,7 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 	if (error)
 		goto err_free_fifo;
 
-	error = acpi_bus_update_power(fujitsu_laptop->acpi_handle, &state);
+	error = acpi_bus_update_power(device->handle, &state);
 	if (error) {
 		pr_err("Error reading power state\n");
 		goto err_free_fifo;
@@ -822,8 +825,6 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 		acpi_device_name(device), acpi_device_bid(device),
 		!device->power.state ? "on" : "off");
 
-	fujitsu_laptop->dev = device;
-
 	if (acpi_has_method(device->handle, METHOD_NAME__INI)) {
 		vdbg_printk(FUJLAPTOP_DBG_INFO, "Invoking _INI\n");
 		if (ACPI_FAILURE
@@ -833,13 +834,13 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 	}
 
 	i = 0;
-	while (call_fext_func(FUNC_BUTTONS, 0x1, 0x0, 0x0) != 0
+	while (call_fext_func(fext, 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);
 
 	fujitsu_laptop->flags_supported =
-		call_fext_func(FUNC_FLAGS, 0x0, 0x0, 0x0);
+		call_fext_func(fext, FUNC_FLAGS, 0x0, 0x0, 0x0);
 
 	/* Make sure our bitmask of supported functions is cleared if the
 	   RFKILL function block is not implemented, like on the S7020. */
@@ -848,15 +849,16 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 
 	if (fujitsu_laptop->flags_supported)
 		fujitsu_laptop->flags_state =
-			call_fext_func(FUNC_FLAGS, 0x4, 0x0, 0x0);
+			call_fext_func(fext, FUNC_FLAGS, 0x4, 0x0, 0x0);
 
 	/* Suspect this is a keymap of the application panel, print it */
-	pr_info("BTNI: [0x%x]\n", call_fext_func(FUNC_BUTTONS, 0x0, 0x0, 0x0));
+	pr_info("BTNI: [0x%x]\n", call_fext_func(fext,
+						 FUNC_BUTTONS, 0x0, 0x0, 0x0));
 
 	/* Sync backlight power status */
 	if (fujitsu_bl && fujitsu_bl->bl_device &&
 	    acpi_video_get_backlight_type() == acpi_backlight_vendor) {
-		if (call_fext_func(FUNC_BACKLIGHT, 0x2, 0x4, 0x0) == 3)
+		if (call_fext_func(fext, FUNC_BACKLIGHT, 0x2, 0x4, 0x0) == 3)
 			fujitsu_bl->bl_device->props.power = FB_BLANK_POWERDOWN;
 		else
 			fujitsu_bl->bl_device->props.power = FB_BLANK_UNBLANK;
@@ -942,9 +944,10 @@ static void acpi_fujitsu_laptop_notify(struct acpi_device *device, u32 event)
 
 	if (fujitsu_laptop->flags_supported)
 		fujitsu_laptop->flags_state =
-			call_fext_func(FUNC_FLAGS, 0x4, 0x0, 0x0);
+			call_fext_func(fext, FUNC_FLAGS, 0x4, 0x0, 0x0);
 
-	while ((irb = call_fext_func(FUNC_BUTTONS, 0x1, 0x0, 0x0)) != 0 &&
+	while ((irb = call_fext_func(fext,
+				     FUNC_BUTTONS, 0x1, 0x0, 0x0)) != 0 &&
 	       i++ < MAX_HOTKEY_RINGBUFFER_SIZE) {
 		scancode = irb & 0x4ff;
 		if (sparse_keymap_entry_from_scancode(input, scancode))
@@ -961,7 +964,7 @@ static void acpi_fujitsu_laptop_notify(struct acpi_device *device, u32 event)
 	 * handled in software; its state is queried using FUNC_FLAGS
 	 */
 	if ((fujitsu_laptop->flags_supported & BIT(26)) &&
-	    (call_fext_func(FUNC_FLAGS, 0x1, 0x0, 0x0) & BIT(26)))
+	    (call_fext_func(fext, FUNC_FLAGS, 0x1, 0x0, 0x0) & BIT(26)))
 		sparse_keymap_report_event(input, BIT(26), 1, true);
 }
 
-- 
2.13.0

[PATCH v2 7/8] platform/x86: fujitsu-laptop: use device-specific data in LED-related code

[Michał Kępień]

In order to perform their duties, all LED callbacks need a pointer to
the struct acpi_device representing the FUJ02E3 ACPI device.  To limit
the use of the module-wide pointer, the same pointer should be extracted
from data that gets passed to LED callbacks as arguments.  However, LED
core does not currently support supplying driver-specific pointers to
struct led_classdev callbacks, so the latter have to be implemented a
bit differently than backlight device callbacks and platform device
attribute callbacks.  As the FUJ02E3 ACPI device is the parent device of
all LED class devices registered by fujitsu-laptop, struct acpi_device
representing the former can be extracted by following the parent link
present inside the struct device belonging to the struct led_classdev
passed as an argument to each LED callback.

To get rid of module-wide structures defining LED class devices,
allocate them dynamically using devm_kzalloc() and initialize them in
acpi_fujitsu_laptop_leds_register().

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

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index b78c97ebfee4..3d33be9f88f6 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -608,6 +608,7 @@ static void fujitsu_laptop_platform_remove(void)
 static int logolamp_set(struct led_classdev *cdev,
 			enum led_brightness brightness)
 {
+	struct acpi_device *device = to_acpi_device(cdev->dev->parent);
 	int poweron = FUNC_LED_ON, always = FUNC_LED_ON;
 	int ret;
 
@@ -617,136 +618,128 @@ static int logolamp_set(struct led_classdev *cdev,
 	if (brightness < LED_FULL)
 		always = FUNC_LED_OFF;
 
-	ret = call_fext_func(fext, FUNC_LEDS, 0x1, LOGOLAMP_POWERON, poweron);
+	ret = call_fext_func(device, FUNC_LEDS, 0x1, LOGOLAMP_POWERON, poweron);
 	if (ret < 0)
 		return ret;
 
-	return call_fext_func(fext, FUNC_LEDS, 0x1, LOGOLAMP_ALWAYS, always);
+	return call_fext_func(device, FUNC_LEDS, 0x1, LOGOLAMP_ALWAYS, always);
 }
 
 static enum led_brightness logolamp_get(struct led_classdev *cdev)
 {
+	struct acpi_device *device = to_acpi_device(cdev->dev->parent);
 	int ret;
 
-	ret = call_fext_func(fext, FUNC_LEDS, 0x2, LOGOLAMP_ALWAYS, 0x0);
+	ret = call_fext_func(device, FUNC_LEDS, 0x2, LOGOLAMP_ALWAYS, 0x0);
 	if (ret == FUNC_LED_ON)
 		return LED_FULL;
 
-	ret = call_fext_func(fext, FUNC_LEDS, 0x2, LOGOLAMP_POWERON, 0x0);
+	ret = call_fext_func(device, FUNC_LEDS, 0x2, LOGOLAMP_POWERON, 0x0);
 	if (ret == FUNC_LED_ON)
 		return LED_HALF;
 
 	return LED_OFF;
 }
 
-static struct led_classdev logolamp_led = {
-	.name = "fujitsu::logolamp",
-	.brightness_set_blocking = logolamp_set,
-	.brightness_get = logolamp_get
-};
-
 static int kblamps_set(struct led_classdev *cdev,
 		       enum led_brightness brightness)
 {
+	struct acpi_device *device = to_acpi_device(cdev->dev->parent);
+
 	if (brightness >= LED_FULL)
-		return call_fext_func(fext, FUNC_LEDS, 0x1, KEYBOARD_LAMPS,
+		return call_fext_func(device, FUNC_LEDS, 0x1, KEYBOARD_LAMPS,
 				      FUNC_LED_ON);
 	else
-		return call_fext_func(fext, FUNC_LEDS, 0x1, KEYBOARD_LAMPS,
+		return call_fext_func(device, FUNC_LEDS, 0x1, KEYBOARD_LAMPS,
 				      FUNC_LED_OFF);
 }
 
 static enum led_brightness kblamps_get(struct led_classdev *cdev)
 {
+	struct acpi_device *device = to_acpi_device(cdev->dev->parent);
 	enum led_brightness brightness = LED_OFF;
 
-	if (call_fext_func(fext,
+	if (call_fext_func(device,
 			   FUNC_LEDS, 0x2, KEYBOARD_LAMPS, 0x0) == FUNC_LED_ON)
 		brightness = LED_FULL;
 
 	return brightness;
 }
 
-static struct led_classdev kblamps_led = {
-	.name = "fujitsu::kblamps",
-	.brightness_set_blocking = kblamps_set,
-	.brightness_get = kblamps_get
-};
-
 static int radio_led_set(struct led_classdev *cdev,
 			 enum led_brightness brightness)
 {
+	struct acpi_device *device = to_acpi_device(cdev->dev->parent);
+
 	if (brightness >= LED_FULL)
-		return call_fext_func(fext, FUNC_FLAGS, 0x5, RADIO_LED_ON,
+		return call_fext_func(device, FUNC_FLAGS, 0x5, RADIO_LED_ON,
 				      RADIO_LED_ON);
 	else
-		return call_fext_func(fext, FUNC_FLAGS, 0x5, RADIO_LED_ON,
+		return call_fext_func(device, FUNC_FLAGS, 0x5, RADIO_LED_ON,
 				      0x0);
 }
 
 static enum led_brightness radio_led_get(struct led_classdev *cdev)
 {
+	struct acpi_device *device = to_acpi_device(cdev->dev->parent);
 	enum led_brightness brightness = LED_OFF;
 
-	if (call_fext_func(fext, FUNC_FLAGS, 0x4, 0x0, 0x0) & RADIO_LED_ON)
+	if (call_fext_func(device, FUNC_FLAGS, 0x4, 0x0, 0x0) & RADIO_LED_ON)
 		brightness = LED_FULL;
 
 	return brightness;
 }
 
-static struct led_classdev radio_led = {
-	.name = "fujitsu::radio_led",
-	.brightness_set_blocking = radio_led_set,
-	.brightness_get = radio_led_get,
-	.default_trigger = "rfkill-any"
-};
-
 static int eco_led_set(struct led_classdev *cdev,
 		       enum led_brightness brightness)
 {
+	struct acpi_device *device = to_acpi_device(cdev->dev->parent);
 	int curr;
 
-	curr = call_fext_func(fext, FUNC_LEDS, 0x2, ECO_LED, 0x0);
+	curr = call_fext_func(device, FUNC_LEDS, 0x2, ECO_LED, 0x0);
 	if (brightness >= LED_FULL)
-		return call_fext_func(fext, FUNC_LEDS, 0x1, ECO_LED,
+		return call_fext_func(device, FUNC_LEDS, 0x1, ECO_LED,
 				      curr | ECO_LED_ON);
 	else
-		return call_fext_func(fext, FUNC_LEDS, 0x1, ECO_LED,
+		return call_fext_func(device, FUNC_LEDS, 0x1, ECO_LED,
 				      curr & ~ECO_LED_ON);
 }
 
 static enum led_brightness eco_led_get(struct led_classdev *cdev)
 {
+	struct acpi_device *device = to_acpi_device(cdev->dev->parent);
 	enum led_brightness brightness = LED_OFF;
 
-	if (call_fext_func(fext, FUNC_LEDS, 0x2, ECO_LED, 0x0) & ECO_LED_ON)
+	if (call_fext_func(device, FUNC_LEDS, 0x2, ECO_LED, 0x0) & ECO_LED_ON)
 		brightness = LED_FULL;
 
 	return brightness;
 }
 
-static struct led_classdev eco_led = {
-	.name = "fujitsu::eco_led",
-	.brightness_set_blocking = eco_led_set,
-	.brightness_get = eco_led_get
-};
-
 static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
 {
+	struct led_classdev *led;
 	int result;
 
-	if (call_fext_func(fext,
+	if (call_fext_func(device,
 			   FUNC_LEDS, 0x0, 0x0, 0x0) & LOGOLAMP_POWERON) {
-		result = devm_led_classdev_register(&device->dev,
-						    &logolamp_led);
+		led = devm_kzalloc(&device->dev, sizeof(*led), GFP_KERNEL);
+		led->name = "fujitsu::logolamp";
+		led->brightness_set_blocking = logolamp_set;
+		led->brightness_get = logolamp_get;
+		result = devm_led_classdev_register(&device->dev, led);
 		if (result)
 			return result;
 	}
 
-	if ((call_fext_func(fext,
+	if ((call_fext_func(device,
 			    FUNC_LEDS, 0x0, 0x0, 0x0) & KEYBOARD_LAMPS) &&
-	    (call_fext_func(fext, FUNC_BUTTONS, 0x0, 0x0, 0x0) == 0x0)) {
-		result = devm_led_classdev_register(&device->dev, &kblamps_led);
+	    (call_fext_func(device, FUNC_BUTTONS, 0x0, 0x0, 0x0) == 0x0)) {
+		led = devm_kzalloc(&device->dev, sizeof(*led), GFP_KERNEL);
+		led->name = "fujitsu::kblamps";
+		led->brightness_set_blocking = kblamps_set;
+		led->brightness_get = kblamps_get;
+		result = devm_led_classdev_register(&device->dev, led);
 		if (result)
 			return result;
 	}
@@ -757,8 +750,13 @@ static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
 	 * to also have an RF LED.  Therefore use bit 24 as an indicator
 	 * that an RF LED is present.
 	 */
-	if (call_fext_func(fext, FUNC_BUTTONS, 0x0, 0x0, 0x0) & BIT(24)) {
-		result = devm_led_classdev_register(&device->dev, &radio_led);
+	if (call_fext_func(device, FUNC_BUTTONS, 0x0, 0x0, 0x0) & BIT(24)) {
+		led = devm_kzalloc(&device->dev, sizeof(*led), GFP_KERNEL);
+		led->name = "fujitsu::radio_led";
+		led->brightness_set_blocking = radio_led_set;
+		led->brightness_get = radio_led_get;
+		led->default_trigger = "rfkill-any";
+		result = devm_led_classdev_register(&device->dev, led);
 		if (result)
 			return result;
 	}
@@ -768,10 +766,14 @@ static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
 	 * bit 14 seems to indicate presence of said led as well.
 	 * Confirm by testing the status.
 	 */
-	if ((call_fext_func(fext, FUNC_LEDS, 0x0, 0x0, 0x0) & BIT(14)) &&
-	    (call_fext_func(fext,
+	if ((call_fext_func(device, FUNC_LEDS, 0x0, 0x0, 0x0) & BIT(14)) &&
+	    (call_fext_func(device,
 			    FUNC_LEDS, 0x2, ECO_LED, 0x0) != UNSUPPORTED_CMD)) {
-		result = devm_led_classdev_register(&device->dev, &eco_led);
+		led = devm_kzalloc(&device->dev, sizeof(*led), GFP_KERNEL);
+		led->name = "fujitsu::eco_led";
+		led->brightness_set_blocking = eco_led_set;
+		led->brightness_get = eco_led_get;
+		result = devm_led_classdev_register(&device->dev, led);
 		if (result)
 			return result;
 	}
-- 
2.13.0

[PATCH v2 8/8] platform/x86: fujitsu-laptop: use device-specific data in remaining module code

[Michał Kępień]

To avoid using module-wide data in remaining module code, employ
acpi_driver_data() and dev_get_drvdata() to fetch device-specific data
to work on in each function.  This makes the input local variables in
hotkey-related callbacks and the module-wide struct fujitsu_laptop
redundant, so remove them.  Adjust whitespace to make checkpatch happy.

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

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 3d33be9f88f6..1c6fdd952c75 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -152,7 +152,6 @@ struct fujitsu_laptop {
 	int flags_state;
 };
 
-static struct fujitsu_laptop *fujitsu_laptop;
 static struct acpi_device *fext;
 
 #ifdef CONFIG_FUJITSU_LAPTOP_DEBUG
@@ -290,9 +289,11 @@ static const struct backlight_ops fujitsu_bl_ops = {
 static ssize_t lid_show(struct device *dev, struct device_attribute *attr,
 			char *buf)
 {
-	if (!(fujitsu_laptop->flags_supported & FLAG_LID))
+	struct fujitsu_laptop *priv = dev_get_drvdata(dev);
+
+	if (!(priv->flags_supported & FLAG_LID))
 		return sprintf(buf, "unknown\n");
-	if (fujitsu_laptop->flags_state & FLAG_LID)
+	if (priv->flags_state & FLAG_LID)
 		return sprintf(buf, "open\n");
 	else
 		return sprintf(buf, "closed\n");
@@ -301,9 +302,11 @@ static ssize_t lid_show(struct device *dev, struct device_attribute *attr,
 static ssize_t dock_show(struct device *dev, struct device_attribute *attr,
 			 char *buf)
 {
-	if (!(fujitsu_laptop->flags_supported & FLAG_DOCK))
+	struct fujitsu_laptop *priv = dev_get_drvdata(dev);
+
+	if (!(priv->flags_supported & FLAG_DOCK))
 		return sprintf(buf, "unknown\n");
-	if (fujitsu_laptop->flags_state & FLAG_DOCK)
+	if (priv->flags_state & FLAG_DOCK)
 		return sprintf(buf, "docked\n");
 	else
 		return sprintf(buf, "undocked\n");
@@ -312,9 +315,11 @@ static ssize_t dock_show(struct device *dev, struct device_attribute *attr,
 static ssize_t radios_show(struct device *dev, struct device_attribute *attr,
 			   char *buf)
 {
-	if (!(fujitsu_laptop->flags_supported & FLAG_RFKILL))
+	struct fujitsu_laptop *priv = dev_get_drvdata(dev);
+
+	if (!(priv->flags_supported & FLAG_RFKILL))
 		return sprintf(buf, "unknown\n");
-	if (fujitsu_laptop->flags_state & FLAG_RFKILL)
+	if (priv->flags_state & FLAG_RFKILL)
 		return sprintf(buf, "on\n");
 	else
 		return sprintf(buf, "killed\n");
@@ -571,19 +576,22 @@ static int acpi_fujitsu_laptop_input_setup(struct acpi_device *device)
 	return input_register_device(priv->input);
 }
 
-static int fujitsu_laptop_platform_add(void)
+static int fujitsu_laptop_platform_add(struct acpi_device *device)
 {
+	struct fujitsu_laptop *priv = acpi_driver_data(device);
 	int ret;
 
-	fujitsu_laptop->pf_device = platform_device_alloc("fujitsu-laptop", -1);
-	if (!fujitsu_laptop->pf_device)
+	priv->pf_device = platform_device_alloc("fujitsu-laptop", -1);
+	if (!priv->pf_device)
 		return -ENOMEM;
 
-	ret = platform_device_add(fujitsu_laptop->pf_device);
+	platform_set_drvdata(priv->pf_device, priv);
+
+	ret = platform_device_add(priv->pf_device);
 	if (ret)
 		goto err_put_platform_device;
 
-	ret = sysfs_create_group(&fujitsu_laptop->pf_device->dev.kobj,
+	ret = sysfs_create_group(&priv->pf_device->dev.kobj,
 				 &fujitsu_pf_attribute_group);
 	if (ret)
 		goto err_del_platform_device;
@@ -591,18 +599,20 @@ static int fujitsu_laptop_platform_add(void)
 	return 0;
 
 err_del_platform_device:
-	platform_device_del(fujitsu_laptop->pf_device);
+	platform_device_del(priv->pf_device);
 err_put_platform_device:
-	platform_device_put(fujitsu_laptop->pf_device);
+	platform_device_put(priv->pf_device);
 
 	return ret;
 }
 
-static void fujitsu_laptop_platform_remove(void)
+static void fujitsu_laptop_platform_remove(struct acpi_device *device)
 {
-	sysfs_remove_group(&fujitsu_laptop->pf_device->dev.kobj,
+	struct fujitsu_laptop *priv = acpi_driver_data(device);
+
+	sysfs_remove_group(&priv->pf_device->dev.kobj,
 			   &fujitsu_pf_attribute_group);
-	platform_device_unregister(fujitsu_laptop->pf_device);
+	platform_device_unregister(priv->pf_device);
 }
 
 static int logolamp_set(struct led_classdev *cdev,
@@ -798,16 +808,15 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 	WARN_ONCE(fext, "More than one FUJ02E3 ACPI device was found.  Driver may not work as intended.");
 	fext = device;
 
-	fujitsu_laptop = priv;
 	sprintf(acpi_device_name(device), "%s",
 		ACPI_FUJITSU_LAPTOP_DEVICE_NAME);
 	sprintf(acpi_device_class(device), "%s", ACPI_FUJITSU_CLASS);
 	device->driver_data = priv;
 
 	/* kfifo */
-	spin_lock_init(&fujitsu_laptop->fifo_lock);
-	error = kfifo_alloc(&fujitsu_laptop->fifo, RINGBUFFERSIZE * sizeof(int),
-			GFP_KERNEL);
+	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;
@@ -836,25 +845,25 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 	}
 
 	i = 0;
-	while (call_fext_func(fext, FUNC_BUTTONS, 0x1, 0x0, 0x0) != 0
+	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);
 
-	fujitsu_laptop->flags_supported =
-		call_fext_func(fext, FUNC_FLAGS, 0x0, 0x0, 0x0);
+	priv->flags_supported = call_fext_func(device, FUNC_FLAGS, 0x0, 0x0,
+					       0x0);
 
 	/* Make sure our bitmask of supported functions is cleared if the
 	   RFKILL function block is not implemented, like on the S7020. */
-	if (fujitsu_laptop->flags_supported == UNSUPPORTED_CMD)
-		fujitsu_laptop->flags_supported = 0;
+	if (priv->flags_supported == UNSUPPORTED_CMD)
+		priv->flags_supported = 0;
 
-	if (fujitsu_laptop->flags_supported)
-		fujitsu_laptop->flags_state =
-			call_fext_func(fext, FUNC_FLAGS, 0x4, 0x0, 0x0);
+	if (priv->flags_supported)
+		priv->flags_state = call_fext_func(device, FUNC_FLAGS, 0x4, 0x0,
+						   0x0);
 
 	/* Suspect this is a keymap of the application panel, print it */
-	pr_info("BTNI: [0x%x]\n", call_fext_func(fext,
+	pr_info("BTNI: [0x%x]\n", call_fext_func(device,
 						 FUNC_BUTTONS, 0x0, 0x0, 0x0));
 
 	/* Sync backlight power status */
@@ -870,14 +879,14 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 	if (error)
 		goto err_free_fifo;
 
-	error = fujitsu_laptop_platform_add();
+	error = fujitsu_laptop_platform_add(device);
 	if (error)
 		goto err_free_fifo;
 
 	return 0;
 
 err_free_fifo:
-	kfifo_free(&fujitsu_laptop->fifo);
+	kfifo_free(&priv->fifo);
 err_stop:
 	return error;
 }
@@ -886,44 +895,42 @@ static int acpi_fujitsu_laptop_remove(struct acpi_device *device)
 {
 	struct fujitsu_laptop *priv = acpi_driver_data(device);
 
-	fujitsu_laptop_platform_remove();
+	fujitsu_laptop_platform_remove(device);
 
 	kfifo_free(&priv->fifo);
 
 	return 0;
 }
 
-static void acpi_fujitsu_laptop_press(int scancode)
+static void acpi_fujitsu_laptop_press(struct acpi_device *device, int scancode)
 {
-	struct input_dev *input = fujitsu_laptop->input;
+	struct fujitsu_laptop *priv = acpi_driver_data(device);
 	int status;
 
-	status = kfifo_in_locked(&fujitsu_laptop->fifo,
-				 (unsigned char *)&scancode, sizeof(scancode),
-				 &fujitsu_laptop->fifo_lock);
+	status = kfifo_in_locked(&priv->fifo, (unsigned char *)&scancode,
+				 sizeof(scancode), &priv->fifo_lock);
 	if (status != sizeof(scancode)) {
 		vdbg_printk(FUJLAPTOP_DBG_WARN,
 			    "Could not push scancode [0x%x]\n", scancode);
 		return;
 	}
-	sparse_keymap_report_event(input, scancode, 1, false);
+	sparse_keymap_report_event(priv->input, scancode, 1, false);
 	vdbg_printk(FUJLAPTOP_DBG_TRACE,
 		    "Push scancode into ringbuffer [0x%x]\n", scancode);
 }
 
-static void acpi_fujitsu_laptop_release(void)
+static void acpi_fujitsu_laptop_release(struct acpi_device *device)
 {
-	struct input_dev *input = fujitsu_laptop->input;
+	struct fujitsu_laptop *priv = acpi_driver_data(device);
 	int scancode, status;
 
 	while (true) {
-		status = kfifo_out_locked(&fujitsu_laptop->fifo,
+		status = kfifo_out_locked(&priv->fifo,
 					  (unsigned char *)&scancode,
-					  sizeof(scancode),
-					  &fujitsu_laptop->fifo_lock);
+					  sizeof(scancode), &priv->fifo_lock);
 		if (status != sizeof(scancode))
 			return;
-		sparse_keymap_report_event(input, scancode, 0, false);
+		sparse_keymap_report_event(priv->input, scancode, 0, false);
 		vdbg_printk(FUJLAPTOP_DBG_TRACE,
 			    "Pop scancode from ringbuffer [0x%x]\n", scancode);
 	}
@@ -931,31 +938,29 @@ static void acpi_fujitsu_laptop_release(void)
 
 static void acpi_fujitsu_laptop_notify(struct acpi_device *device, u32 event)
 {
-	struct input_dev *input;
+	struct fujitsu_laptop *priv = acpi_driver_data(device);
 	int scancode, i = 0;
 	unsigned int irb;
 
-	input = fujitsu_laptop->input;
-
 	if (event != ACPI_FUJITSU_NOTIFY_CODE1) {
 		vdbg_printk(FUJLAPTOP_DBG_WARN,
 			    "Unsupported event [0x%x]\n", event);
-		sparse_keymap_report_event(input, -1, 1, true);
+		sparse_keymap_report_event(priv->input, -1, 1, true);
 		return;
 	}
 
-	if (fujitsu_laptop->flags_supported)
-		fujitsu_laptop->flags_state =
-			call_fext_func(fext, FUNC_FLAGS, 0x4, 0x0, 0x0);
+	if (priv->flags_supported)
+		priv->flags_state = call_fext_func(device, FUNC_FLAGS, 0x4, 0x0,
+						   0x0);
 
-	while ((irb = call_fext_func(fext,
+	while ((irb = call_fext_func(device,
 				     FUNC_BUTTONS, 0x1, 0x0, 0x0)) != 0 &&
 	       i++ < MAX_HOTKEY_RINGBUFFER_SIZE) {
 		scancode = irb & 0x4ff;
-		if (sparse_keymap_entry_from_scancode(input, scancode))
-			acpi_fujitsu_laptop_press(scancode);
+		if (sparse_keymap_entry_from_scancode(priv->input, scancode))
+			acpi_fujitsu_laptop_press(device, scancode);
 		else if (scancode == 0)
-			acpi_fujitsu_laptop_release();
+			acpi_fujitsu_laptop_release(device);
 		else
 			vdbg_printk(FUJLAPTOP_DBG_WARN,
 				    "Unknown GIRB result [%x]\n", irb);
@@ -965,9 +970,9 @@ static void acpi_fujitsu_laptop_notify(struct acpi_device *device, u32 event)
 	 * E736/E746/E756), the touchpad toggle hotkey (Fn+F4) is
 	 * handled in software; its state is queried using FUNC_FLAGS
 	 */
-	if ((fujitsu_laptop->flags_supported & BIT(26)) &&
-	    (call_fext_func(fext, FUNC_FLAGS, 0x1, 0x0, 0x0) & BIT(26)))
-		sparse_keymap_report_event(input, BIT(26), 1, true);
+	if ((priv->flags_supported & BIT(26)) &&
+	    (call_fext_func(device, FUNC_FLAGS, 0x1, 0x0, 0x0) & BIT(26)))
+		sparse_keymap_report_event(priv->input, BIT(26), 1, true);
 }
 
 /* Initialization */
-- 
2.13.0

Re: [PATCH v2 0/8] fujitsu-laptop: use device-specific data instead of module-wide globals

[Jonathan Woithe]

Hi Michael

On Fri, May 19, 2017 at 09:44:40AM +0200, Micha?? K??pie?? wrote:
> fujitsu-laptop registers two ACPI drivers that access each other's
> module-wide structures.  To improve data encapsulation and lay the
> groundwork for separating the two aforementioned ACPI drivers into
> separate modules, move away from module-wide global data structures by
> using device-specific data instead.
> :

I have had a quick look through this series.  To my eye it addresses the
outcome of our discussion over the last couple of weeks, and looks good.  I
had one query regarding patch 5/8, but that might just be a case of me not
knowing about a subtlety of WARN_ONCE().  In any case this isn't a major
issue and would be easily resolved if needed.

Once I get feedback on patch 5/8 (and after seeing any additional comments
from Darren et al) I can send through a reviewed-by.

Regards
  jonathan

Re: [PATCH v2 0/8] fujitsu-laptop: use device-specific data instead of module-wide globals

[Jonathan Woithe]

Hi Michael

On Mon, May 22, 2017 at 08:53:23AM +0930, Jonathan Woithe wrote:
> On Fri, May 19, 2017 at 09:44:40AM +0200, Micha?? K??pie?? wrote:
> > fujitsu-laptop registers two ACPI drivers that access each other's
> > module-wide structures.  To improve data encapsulation and lay the
> > groundwork for separating the two aforementioned ACPI drivers into
> > separate modules, move away from module-wide global data structures by
> > using device-specific data instead.
> > :
> 
> I have had a quick look through this series.  To my eye it addresses the
> outcome of our discussion over the last couple of weeks, and looks good.  I
> had one query regarding patch 5/8, but that might just be a case of me not
> knowing about a subtlety of WARN_ONCE().  In any case this isn't a major
> issue and would be easily resolved if needed.
> 
> Once I get feedback on patch 5/8 (and after seeing any additional comments
> from Darren et al) I can send through a reviewed-by.

With the minor query in 5.8 sorted, I'm happy to proceed with this (subject
of course to any further comments from Darren or Andy).  While not an
endpoint in and of itslef, it is never-the-less an important step towards
the agreed goal to separate the fujitsu-laptop module into two drivers. 
This is motivated by the current use of two ACPI devices by the single
fujitsu-laptop module, which is inconsistent with the kernel's "one driver
per module" approach.

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

Regards
  jonathan

Re: [PATCH v2 0/8] fujitsu-laptop: use device-specific data instead of module-wide globals

[Darren Hart]

On Wed, May 24, 2017 at 09:30:18AM +0930, Jonathan Woithe wrote:
> Hi Michael
> 
> On Mon, May 22, 2017 at 08:53:23AM +0930, Jonathan Woithe wrote:
> > On Fri, May 19, 2017 at 09:44:40AM +0200, Micha?? K??pie?? wrote:
> > > fujitsu-laptop registers two ACPI drivers that access each other's
> > > module-wide structures.  To improve data encapsulation and lay the
> > > groundwork for separating the two aforementioned ACPI drivers into
> > > separate modules, move away from module-wide global data structures by
> > > using device-specific data instead.
> > > :
> > 
> > I have had a quick look through this series.  To my eye it addresses the
> > outcome of our discussion over the last couple of weeks, and looks good.  I
> > had one query regarding patch 5/8, but that might just be a case of me not
> > knowing about a subtlety of WARN_ONCE().  In any case this isn't a major
> > issue and would be easily resolved if needed.
> > 
> > Once I get feedback on patch 5/8 (and after seeing any additional comments
> > from Darren et al) I can send through a reviewed-by.
> 
> With the minor query in 5.8 sorted, I'm happy to proceed with this (subject
> of course to any further comments from Darren or Andy).  While not an
> endpoint in and of itslef, it is never-the-less an important step towards
> the agreed goal to separate the fujitsu-laptop module into two drivers. 
> This is motivated by the current use of two ACPI devices by the single
> fujitsu-laptop module, which is inconsistent with the kernel's "one driver
> per module" approach.
> 
> Reviewed-by: Jonathan Woithe <jwoithe@just42.net>

Apologies for the delay gents. Reviewed and pushed to testing.

-- 
Darren Hart
VMware Open Source Technology Center