linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals
@ 2017-04-24 13:33 Michał Kępień
  2017-04-24 13:33 ` [PATCH 01/10] platform/x86: fujitsu-laptop: introduce fext_*() helper functions Michał Kępień
                   ` (10 more replies)
  0 siblings, 11 replies; 39+ messages in thread
From: Michał Kępień @ 2017-04-24 13:33 UTC (permalink / raw)
  To: Jonathan Woithe, Darren Hart, Andy Shevchenko
  Cc: platform-driver-x86, linux-kernel

fujitsu-laptop registers two ACPI drivers.  Whenever an ACPI device with
a matching identifier is found by the ACPI bus, a new instance of the
relevant driver is bound to that ACPI device.  However, both ACPI
drivers registered by fujitsu-laptop access module-wide global data
structures, assuming neither ACPI driver will ever be instantiated more
than once.  While there are currently no indications of such issues
happening in the wild, it is theoretically possible for multiple
FUJ02B1/FUJ02E3 ACPI devices to be present in the firmware, which would
cause two instances of the relevant driver to simultaneously access
module-wide globals without any locking in place.  Also, modern Fujitsu
laptops ship without the FUJ02B1 ACPI device present in firmware,
causing memory to be needlessly allocated inside fujitsu_init().

To future-proof the module 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.

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

I found it challenging to adhere to the "one logical change per patch"
rule while touching code commonly used by almost all other module code.
If the changes introduced are illegible, I will be happy to further
explain and/or improve the series.  Please also note that while the diff
stats for this series may seem daunting at first, using --color-words
should hopefully make reviewing much more manageable.

 drivers/platform/x86/fujitsu-laptop.c | 460 +++++++++++++++++-----------------
 1 file changed, 236 insertions(+), 224 deletions(-)

-- 
2.12.2

^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 01/10] platform/x86: fujitsu-laptop: introduce fext_*() helper functions
  2017-04-24 13:33 [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals Michał Kępień
@ 2017-04-24 13:33 ` Michał Kępień
  2017-05-01 13:13   ` Jonathan Woithe
  2017-04-24 13:33 ` [PATCH 02/10] platform/x86: fujitsu-laptop: shorten names of acpi_handle fields Michał Kępień
                   ` (9 subsequent siblings)
  10 siblings, 1 reply; 39+ messages in thread
From: Michał Kępień @ 2017-04-24 13:33 UTC (permalink / raw)
  To: Jonathan Woithe, Darren Hart, Andy Shevchenko
  Cc: platform-driver-x86, linux-kernel

Stop invoking call_fext_func() directly to improve code clarity and save
some horizontal space.  Adjust whitespace to make checkpatch happy.

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

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 7f49d92914c9..3f232967af04 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -187,6 +187,26 @@ static int call_fext_func(int func, int op, int feature, int state)
 	return value;
 }
 
+static int fext_backlight(int op, int feature, int state)
+{
+	return call_fext_func(FUNC_BACKLIGHT, op, feature, state);
+}
+
+static int fext_buttons(int op, int feature, int state)
+{
+	return call_fext_func(FUNC_BUTTONS, op, feature, state);
+}
+
+static int fext_flags(int op, int feature, int state)
+{
+	return call_fext_func(FUNC_FLAGS, op, feature, state);
+}
+
+static int fext_leds(int op, int feature, int state)
+{
+	return call_fext_func(FUNC_LEDS, op, feature, state);
+}
+
 /* Hardware access for LCD brightness control */
 
 static int set_lcd_level(int level)
@@ -272,9 +292,9 @@ static int bl_get_brightness(struct backlight_device *b)
 static int bl_update_status(struct backlight_device *b)
 {
 	if (b->props.power == FB_BLANK_POWERDOWN)
-		call_fext_func(FUNC_BACKLIGHT, 0x1, 0x4, 0x3);
+		fext_backlight(0x1, 0x4, 0x3);
 	else
-		call_fext_func(FUNC_BACKLIGHT, 0x1, 0x4, 0x0);
+		fext_backlight(0x1, 0x4, 0x0);
 
 	return set_lcd_level(b->props.brightness);
 }
@@ -610,22 +630,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 = fext_leds(0x1, LOGOLAMP_POWERON, poweron);
 	if (ret < 0)
 		return ret;
 
-	return call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_ALWAYS, always);
+	return fext_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 = fext_leds(0x2, LOGOLAMP_ALWAYS, 0x0);
 	if (ret == FUNC_LED_ON)
 		return LED_FULL;
 
-	ret = call_fext_func(FUNC_LEDS, 0x2, LOGOLAMP_POWERON, 0x0);
+	ret = fext_leds(0x2, LOGOLAMP_POWERON, 0x0);
 	if (ret == FUNC_LED_ON)
 		return LED_HALF;
 
@@ -642,18 +662,16 @@ 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,
-				      FUNC_LED_ON);
+		return fext_leds(0x1, KEYBOARD_LAMPS, FUNC_LED_ON);
 	else
-		return call_fext_func(FUNC_LEDS, 0x1, KEYBOARD_LAMPS,
-				      FUNC_LED_OFF);
+		return fext_leds(0x1, KEYBOARD_LAMPS, FUNC_LED_OFF);
 }
 
 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 (fext_leds(0x2, KEYBOARD_LAMPS, 0x0) == FUNC_LED_ON)
 		brightness = LED_FULL;
 
 	return brightness;
@@ -669,17 +687,16 @@ 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,
-				      RADIO_LED_ON);
+		return fext_flags(0x5, RADIO_LED_ON, RADIO_LED_ON);
 	else
-		return call_fext_func(FUNC_FLAGS, 0x5, RADIO_LED_ON, 0x0);
+		return fext_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 (fext_flags(0x4, 0x0, 0x0) & RADIO_LED_ON)
 		brightness = LED_FULL;
 
 	return brightness;
@@ -697,20 +714,18 @@ static int eco_led_set(struct led_classdev *cdev,
 {
 	int curr;
 
-	curr = call_fext_func(FUNC_LEDS, 0x2, ECO_LED, 0x0);
+	curr = fext_leds(0x2, ECO_LED, 0x0);
 	if (brightness >= LED_FULL)
-		return call_fext_func(FUNC_LEDS, 0x1, ECO_LED,
-				      curr | ECO_LED_ON);
+		return fext_leds(0x1, ECO_LED, curr | ECO_LED_ON);
 	else
-		return call_fext_func(FUNC_LEDS, 0x1, ECO_LED,
-				      curr & ~ECO_LED_ON);
+		return fext_leds(0x1, ECO_LED, curr & ~ECO_LED_ON);
 }
 
 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 (fext_leds(0x2, ECO_LED, 0x0) & ECO_LED_ON)
 		brightness = LED_FULL;
 
 	return brightness;
@@ -726,15 +741,15 @@ 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 (fext_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 ((fext_leds(0x0, 0x0, 0x0) & KEYBOARD_LAMPS) &&
+	    (fext_buttons(0x0, 0x0, 0x0) == 0x0)) {
 		result = devm_led_classdev_register(&device->dev, &kblamps_led);
 		if (result)
 			return result;
@@ -746,7 +761,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 (fext_buttons(0x0, 0x0, 0x0) & BIT(24)) {
 		result = devm_led_classdev_register(&device->dev, &radio_led);
 		if (result)
 			return result;
@@ -757,8 +772,8 @@ 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 ((fext_leds(0x0, 0x0, 0x0) & BIT(14)) &&
+	    (fext_leds(0x2, ECO_LED, 0x0) != UNSUPPORTED_CMD)) {
 		result = devm_led_classdev_register(&device->dev, &eco_led);
 		if (result)
 			return result;
@@ -816,13 +831,12 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 	}
 
 	i = 0;
-	while (call_fext_func(FUNC_BUTTONS, 0x1, 0x0, 0x0) != 0
-		&& (i++) < MAX_HOTKEY_RINGBUFFER_SIZE)
+	while (fext_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);
+	fujitsu_laptop->flags_supported = fext_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. */
@@ -830,16 +844,15 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 		fujitsu_laptop->flags_supported = 0;
 
 	if (fujitsu_laptop->flags_supported)
-		fujitsu_laptop->flags_state =
-			call_fext_func(FUNC_FLAGS, 0x4, 0x0, 0x0);
+		fujitsu_laptop->flags_state = fext_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", fext_buttons(0x0, 0x0, 0x0));
 
 	/* Sync backlight power status */
 	if (fujitsu_bl->bl_device &&
 	    acpi_video_get_backlight_type() == acpi_backlight_vendor) {
-		if (call_fext_func(FUNC_BACKLIGHT, 0x2, 0x4, 0x0) == 3)
+		if (fext_backlight(0x2, 0x4, 0x0) == 3)
 			fujitsu_bl->bl_device->props.power = FB_BLANK_POWERDOWN;
 		else
 			fujitsu_bl->bl_device->props.power = FB_BLANK_UNBLANK;
@@ -924,10 +937,9 @@ 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);
+		fujitsu_laptop->flags_state = fext_flags(0x4, 0x0, 0x0);
 
-	while ((irb = call_fext_func(FUNC_BUTTONS, 0x1, 0x0, 0x0)) != 0 &&
+	while ((irb = fext_buttons(0x1, 0x0, 0x0)) != 0 &&
 	       i++ < MAX_HOTKEY_RINGBUFFER_SIZE) {
 		scancode = irb & 0x4ff;
 		if (sparse_keymap_entry_from_scancode(input, scancode))
@@ -944,7 +956,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)))
+	    (fext_flags(0x1, 0x0, 0x0) & BIT(26)))
 		sparse_keymap_report_event(input, BIT(26), 1, true);
 }
 
-- 
2.12.2

^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH 02/10] platform/x86: fujitsu-laptop: shorten names of acpi_handle fields
  2017-04-24 13:33 [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals Michał Kępień
  2017-04-24 13:33 ` [PATCH 01/10] platform/x86: fujitsu-laptop: introduce fext_*() helper functions Michał Kępień
@ 2017-04-24 13:33 ` Michał Kępień
  2017-05-01 13:19   ` Jonathan Woithe
  2017-04-24 13:33 ` [PATCH 03/10] platform/x86: fujitsu-laptop: explicitly pass ACPI handle to call_fext_func() Michał Kępień
                   ` (8 subsequent siblings)
  10 siblings, 1 reply; 39+ messages in thread
From: Michał Kępień @ 2017-04-24 13:33 UTC (permalink / raw)
  To: Jonathan Woithe, Darren Hart, Andy Shevchenko
  Cc: platform-driver-x86, linux-kernel

As both struct fujitsu_bl and struct fujitsu_laptop represent data
associated with ACPI devices, drop the "acpi_" prefix from the names of
the relevant fields of these structures to save some horizontal space.

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

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 3f232967af04..3695e8075aa6 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -130,7 +130,7 @@
 
 /* Device controlling the backlight and associated keys */
 struct fujitsu_bl {
-	acpi_handle acpi_handle;
+	acpi_handle handle;
 	struct input_dev *input;
 	char phys[32];
 	struct backlight_device *bl_device;
@@ -144,7 +144,7 @@ static bool disable_brightness_adjust;
 
 /* Device used to access hotkeys and other features on the laptop */
 struct fujitsu_laptop {
-	acpi_handle acpi_handle;
+	acpi_handle handle;
 	struct acpi_device *dev;
 	struct input_dev *input;
 	char phys[32];
@@ -175,7 +175,7 @@ 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",
+	status = acpi_evaluate_integer(fujitsu_laptop->handle, "FUNC",
 				       &arg_list, &value);
 	if (ACPI_FAILURE(status)) {
 		vdbg_printk(FUJLAPTOP_DBG_ERROR, "Failed to evaluate FUNC\n");
@@ -216,7 +216,7 @@ static int set_lcd_level(int level)
 
 	switch (use_alt_lcd_levels) {
 	case -1:
-		if (acpi_has_method(fujitsu_bl->acpi_handle, "SBL2"))
+		if (acpi_has_method(fujitsu_bl->handle, "SBL2"))
 			method = "SBL2";
 		else
 			method = "SBLL";
@@ -235,8 +235,7 @@ static int set_lcd_level(int level)
 	if (level < 0 || level >= fujitsu_bl->max_brightness)
 		return -EINVAL;
 
-	status = acpi_execute_simple_method(fujitsu_bl->acpi_handle, method,
-					    level);
+	status = acpi_execute_simple_method(fujitsu_bl->handle, method, level);
 	if (ACPI_FAILURE(status)) {
 		vdbg_printk(FUJLAPTOP_DBG_ERROR, "Failed to evaluate %s\n",
 			    method);
@@ -255,7 +254,7 @@ static int get_lcd_level(void)
 
 	vdbg_printk(FUJLAPTOP_DBG_TRACE, "get lcd level via GBLL\n");
 
-	status = acpi_evaluate_integer(fujitsu_bl->acpi_handle, "GBLL", NULL,
+	status = acpi_evaluate_integer(fujitsu_bl->handle, "GBLL", NULL,
 				       &state);
 	if (ACPI_FAILURE(status))
 		return 0;
@@ -272,7 +271,7 @@ static int get_max_brightness(void)
 
 	vdbg_printk(FUJLAPTOP_DBG_TRACE, "get max lcd level via RBLL\n");
 
-	status = acpi_evaluate_integer(fujitsu_bl->acpi_handle, "RBLL", NULL,
+	status = acpi_evaluate_integer(fujitsu_bl->handle, "RBLL", NULL,
 				       &state);
 	if (ACPI_FAILURE(status))
 		return -1;
@@ -421,7 +420,7 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device)
 	if (!device)
 		return -EINVAL;
 
-	fujitsu_bl->acpi_handle = device->handle;
+	fujitsu_bl->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;
@@ -430,7 +429,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(fujitsu_bl->handle, &state);
 	if (error) {
 		pr_err("Error reading power state\n");
 		return error;
@@ -791,7 +790,7 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 	if (!device)
 		return -EINVAL;
 
-	fujitsu_laptop->acpi_handle = device->handle;
+	fujitsu_laptop->handle = device->handle;
 	sprintf(acpi_device_name(device), "%s",
 		ACPI_FUJITSU_LAPTOP_DEVICE_NAME);
 	sprintf(acpi_device_class(device), "%s", ACPI_FUJITSU_CLASS);
@@ -810,7 +809,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(fujitsu_laptop->handle, &state);
 	if (error) {
 		pr_err("Error reading power state\n");
 		goto err_free_fifo;
-- 
2.12.2

^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH 03/10] platform/x86: fujitsu-laptop: explicitly pass ACPI handle to call_fext_func()
  2017-04-24 13:33 [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals Michał Kępień
  2017-04-24 13:33 ` [PATCH 01/10] platform/x86: fujitsu-laptop: introduce fext_*() helper functions Michał Kępień
  2017-04-24 13:33 ` [PATCH 02/10] platform/x86: fujitsu-laptop: shorten names of acpi_handle fields Michał Kępień
@ 2017-04-24 13:33 ` Michał Kępień
  2017-04-24 13:33 ` [PATCH 04/10] platform/x86: fujitsu-laptop: rework backlight power synchronization Michał Kępień
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 39+ messages in thread
From: Michał Kępień @ 2017-04-24 13:33 UTC (permalink / raw)
  To: Jonathan Woithe, Darren Hart, Andy Shevchenko
  Cc: platform-driver-x86, linux-kernel

Prepare for not using module-wide data in call_fext_func() by explicitly
passing it an ACPI handle to the FUJ02E3 device while still using the
module-wide handle in each call to fext_*() helper functions.

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

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 3695e8075aa6..ea3210ee83ec 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -163,7 +163,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(acpi_handle handle,
+			  int func, int op, int feature, int state)
 {
 	union acpi_object params[4] = {
 		{ .integer.type = ACPI_TYPE_INTEGER, .integer.value = func },
@@ -175,8 +176,7 @@ 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->handle, "FUNC",
-				       &arg_list, &value);
+	status = acpi_evaluate_integer(handle, "FUNC", &arg_list, &value);
 	if (ACPI_FAILURE(status)) {
 		vdbg_printk(FUJLAPTOP_DBG_ERROR, "Failed to evaluate FUNC\n");
 		return -ENODEV;
@@ -187,24 +187,24 @@ static int call_fext_func(int func, int op, int feature, int state)
 	return value;
 }
 
-static int fext_backlight(int op, int feature, int state)
+static int fext_backlight(acpi_handle handle, int op, int feature, int state)
 {
-	return call_fext_func(FUNC_BACKLIGHT, op, feature, state);
+	return call_fext_func(handle, FUNC_BACKLIGHT, op, feature, state);
 }
 
-static int fext_buttons(int op, int feature, int state)
+static int fext_buttons(acpi_handle handle, int op, int feature, int state)
 {
-	return call_fext_func(FUNC_BUTTONS, op, feature, state);
+	return call_fext_func(handle, FUNC_BUTTONS, op, feature, state);
 }
 
-static int fext_flags(int op, int feature, int state)
+static int fext_flags(acpi_handle handle, int op, int feature, int state)
 {
-	return call_fext_func(FUNC_FLAGS, op, feature, state);
+	return call_fext_func(handle, FUNC_FLAGS, op, feature, state);
 }
 
-static int fext_leds(int op, int feature, int state)
+static int fext_leds(acpi_handle handle, int op, int feature, int state)
 {
-	return call_fext_func(FUNC_LEDS, op, feature, state);
+	return call_fext_func(handle, FUNC_LEDS, op, feature, state);
 }
 
 /* Hardware access for LCD brightness control */
@@ -291,9 +291,9 @@ static int bl_get_brightness(struct backlight_device *b)
 static int bl_update_status(struct backlight_device *b)
 {
 	if (b->props.power == FB_BLANK_POWERDOWN)
-		fext_backlight(0x1, 0x4, 0x3);
+		fext_backlight(fujitsu_laptop->handle, 0x1, 0x4, 0x3);
 	else
-		fext_backlight(0x1, 0x4, 0x0);
+		fext_backlight(fujitsu_laptop->handle, 0x1, 0x4, 0x0);
 
 	return set_lcd_level(b->props.brightness);
 }
@@ -629,22 +629,22 @@ static int logolamp_set(struct led_classdev *cdev,
 	if (brightness < LED_FULL)
 		always = FUNC_LED_OFF;
 
-	ret = fext_leds(0x1, LOGOLAMP_POWERON, poweron);
+	ret = fext_leds(fujitsu_laptop->handle, 0x1, LOGOLAMP_POWERON, poweron);
 	if (ret < 0)
 		return ret;
 
-	return fext_leds(0x1, LOGOLAMP_ALWAYS, always);
+	return fext_leds(fujitsu_laptop->handle, 0x1, LOGOLAMP_ALWAYS, always);
 }
 
 static enum led_brightness logolamp_get(struct led_classdev *cdev)
 {
 	int ret;
 
-	ret = fext_leds(0x2, LOGOLAMP_ALWAYS, 0x0);
+	ret = fext_leds(fujitsu_laptop->handle, 0x2, LOGOLAMP_ALWAYS, 0x0);
 	if (ret == FUNC_LED_ON)
 		return LED_FULL;
 
-	ret = fext_leds(0x2, LOGOLAMP_POWERON, 0x0);
+	ret = fext_leds(fujitsu_laptop->handle, 0x2, LOGOLAMP_POWERON, 0x0);
 	if (ret == FUNC_LED_ON)
 		return LED_HALF;
 
@@ -661,16 +661,19 @@ static int kblamps_set(struct led_classdev *cdev,
 		       enum led_brightness brightness)
 {
 	if (brightness >= LED_FULL)
-		return fext_leds(0x1, KEYBOARD_LAMPS, FUNC_LED_ON);
+		return fext_leds(fujitsu_laptop->handle, 0x1, KEYBOARD_LAMPS,
+				 FUNC_LED_ON);
 	else
-		return fext_leds(0x1, KEYBOARD_LAMPS, FUNC_LED_OFF);
+		return fext_leds(fujitsu_laptop->handle, 0x1, KEYBOARD_LAMPS,
+				 FUNC_LED_OFF);
 }
 
 static enum led_brightness kblamps_get(struct led_classdev *cdev)
 {
 	enum led_brightness brightness = LED_OFF;
 
-	if (fext_leds(0x2, KEYBOARD_LAMPS, 0x0) == FUNC_LED_ON)
+	if (fext_leds(fujitsu_laptop->handle,
+		      0x2, KEYBOARD_LAMPS, 0x0) == FUNC_LED_ON)
 		brightness = LED_FULL;
 
 	return brightness;
@@ -686,16 +689,18 @@ static int radio_led_set(struct led_classdev *cdev,
 			 enum led_brightness brightness)
 {
 	if (brightness >= LED_FULL)
-		return fext_flags(0x5, RADIO_LED_ON, RADIO_LED_ON);
+		return fext_flags(fujitsu_laptop->handle, 0x5, RADIO_LED_ON,
+				  RADIO_LED_ON);
 	else
-		return fext_flags(0x5, RADIO_LED_ON, 0x0);
+		return fext_flags(fujitsu_laptop->handle, 0x5, RADIO_LED_ON,
+				  0x0);
 }
 
 static enum led_brightness radio_led_get(struct led_classdev *cdev)
 {
 	enum led_brightness brightness = LED_OFF;
 
-	if (fext_flags(0x4, 0x0, 0x0) & RADIO_LED_ON)
+	if (fext_flags(fujitsu_laptop->handle, 0x4, 0x0, 0x0) & RADIO_LED_ON)
 		brightness = LED_FULL;
 
 	return brightness;
@@ -713,18 +718,20 @@ static int eco_led_set(struct led_classdev *cdev,
 {
 	int curr;
 
-	curr = fext_leds(0x2, ECO_LED, 0x0);
+	curr = fext_leds(fujitsu_laptop->handle, 0x2, ECO_LED, 0x0);
 	if (brightness >= LED_FULL)
-		return fext_leds(0x1, ECO_LED, curr | ECO_LED_ON);
+		return fext_leds(fujitsu_laptop->handle, 0x1, ECO_LED,
+				 curr | ECO_LED_ON);
 	else
-		return fext_leds(0x1, ECO_LED, curr & ~ECO_LED_ON);
+		return fext_leds(fujitsu_laptop->handle, 0x1, ECO_LED,
+				 curr & ~ECO_LED_ON);
 }
 
 static enum led_brightness eco_led_get(struct led_classdev *cdev)
 {
 	enum led_brightness brightness = LED_OFF;
 
-	if (fext_leds(0x2, ECO_LED, 0x0) & ECO_LED_ON)
+	if (fext_leds(fujitsu_laptop->handle, 0x2, ECO_LED, 0x0) & ECO_LED_ON)
 		brightness = LED_FULL;
 
 	return brightness;
@@ -740,15 +747,17 @@ static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
 {
 	int result;
 
-	if (fext_leds(0x0, 0x0, 0x0) & LOGOLAMP_POWERON) {
+	if (fext_leds(fujitsu_laptop->handle,
+		      0x0, 0x0, 0x0) & LOGOLAMP_POWERON) {
 		result = devm_led_classdev_register(&device->dev,
 						    &logolamp_led);
 		if (result)
 			return result;
 	}
 
-	if ((fext_leds(0x0, 0x0, 0x0) & KEYBOARD_LAMPS) &&
-	    (fext_buttons(0x0, 0x0, 0x0) == 0x0)) {
+	if ((fext_leds(fujitsu_laptop->handle,
+		       0x0, 0x0, 0x0) & KEYBOARD_LAMPS) &&
+	    (fext_buttons(fujitsu_laptop->handle, 0x0, 0x0, 0x0) == 0x0)) {
 		result = devm_led_classdev_register(&device->dev, &kblamps_led);
 		if (result)
 			return result;
@@ -760,7 +769,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 (fext_buttons(0x0, 0x0, 0x0) & BIT(24)) {
+	if (fext_buttons(fujitsu_laptop->handle, 0x0, 0x0, 0x0) & BIT(24)) {
 		result = devm_led_classdev_register(&device->dev, &radio_led);
 		if (result)
 			return result;
@@ -771,8 +780,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 ((fext_leds(0x0, 0x0, 0x0) & BIT(14)) &&
-	    (fext_leds(0x2, ECO_LED, 0x0) != UNSUPPORTED_CMD)) {
+	if ((fext_leds(fujitsu_laptop->handle, 0x0, 0x0, 0x0) & BIT(14)) &&
+	    (fext_leds(fujitsu_laptop->handle,
+		       0x2, ECO_LED, 0x0) != UNSUPPORTED_CMD)) {
 		result = devm_led_classdev_register(&device->dev, &eco_led);
 		if (result)
 			return result;
@@ -830,12 +840,13 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 	}
 
 	i = 0;
-	while (fext_buttons(0x1, 0x0, 0x0) != 0 &&
+	while (fext_buttons(fujitsu_laptop->handle, 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 = fext_flags(0x0, 0x0, 0x0);
+	fujitsu_laptop->flags_supported = fext_flags(fujitsu_laptop->handle,
+						     0x0, 0x0, 0x0);
 
 	/* Make sure our bitmask of supported functions is cleared if the
 	   RFKILL function block is not implemented, like on the S7020. */
@@ -843,15 +854,17 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 		fujitsu_laptop->flags_supported = 0;
 
 	if (fujitsu_laptop->flags_supported)
-		fujitsu_laptop->flags_state = fext_flags(0x4, 0x0, 0x0);
+		fujitsu_laptop->flags_state = fext_flags(fujitsu_laptop->handle,
+							 0x4, 0x0, 0x0);
 
 	/* Suspect this is a keymap of the application panel, print it */
-	pr_info("BTNI: [0x%x]\n", fext_buttons(0x0, 0x0, 0x0));
+	pr_info("BTNI: [0x%x]\n", fext_buttons(fujitsu_laptop->handle,
+					       0x0, 0x0, 0x0));
 
 	/* Sync backlight power status */
 	if (fujitsu_bl->bl_device &&
 	    acpi_video_get_backlight_type() == acpi_backlight_vendor) {
-		if (fext_backlight(0x2, 0x4, 0x0) == 3)
+		if (fext_backlight(fujitsu_laptop->handle, 0x2, 0x4, 0x0) == 3)
 			fujitsu_bl->bl_device->props.power = FB_BLANK_POWERDOWN;
 		else
 			fujitsu_bl->bl_device->props.power = FB_BLANK_UNBLANK;
@@ -936,9 +949,11 @@ static void acpi_fujitsu_laptop_notify(struct acpi_device *device, u32 event)
 	}
 
 	if (fujitsu_laptop->flags_supported)
-		fujitsu_laptop->flags_state = fext_flags(0x4, 0x0, 0x0);
+		fujitsu_laptop->flags_state = fext_flags(fujitsu_laptop->handle,
+							 0x4, 0x0, 0x0);
 
-	while ((irb = fext_buttons(0x1, 0x0, 0x0)) != 0 &&
+	while ((irb = fext_buttons(fujitsu_laptop->handle,
+				   0x1, 0x0, 0x0)) != 0 &&
 	       i++ < MAX_HOTKEY_RINGBUFFER_SIZE) {
 		scancode = irb & 0x4ff;
 		if (sparse_keymap_entry_from_scancode(input, scancode))
@@ -955,7 +970,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)) &&
-	    (fext_flags(0x1, 0x0, 0x0) & BIT(26)))
+	    (fext_flags(fujitsu_laptop->handle, 0x1, 0x0, 0x0) & BIT(26)))
 		sparse_keymap_report_event(input, BIT(26), 1, true);
 }
 
-- 
2.12.2

^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH 04/10] platform/x86: fujitsu-laptop: rework backlight power synchronization
  2017-04-24 13:33 [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals Michał Kępień
                   ` (2 preceding siblings ...)
  2017-04-24 13:33 ` [PATCH 03/10] platform/x86: fujitsu-laptop: explicitly pass ACPI handle to call_fext_func() Michał Kępień
@ 2017-04-24 13:33 ` Michał Kępień
  2017-05-01 13:32   ` Jonathan Woithe
  2017-04-24 13:33 ` [PATCH 05/10] platform/x86: fujitsu-laptop: distinguish current uses of device-specific data Michał Kępień
                   ` (6 subsequent siblings)
  10 siblings, 1 reply; 39+ messages in thread
From: Michał Kępień @ 2017-04-24 13:33 UTC (permalink / raw)
  To: Jonathan Woithe, Darren Hart, Andy Shevchenko
  Cc: platform-driver-x86, linux-kernel

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.)  So far, these two
drivers have been entangled by calls to fext_backlight() (previously
known as call_fext_func()) in the backlight part of the module which use
module-wide data managed by the other part of the module and accesses to
the backlight device from within acpi_fujitsu_laptop_add().  This
entaglement can be solved by storing an independently fetched ACPI
handle to the FUJ02E3 device inside the data structure managed by the
backlight part of the module.

Add a field to struct fujitsu_bl for storing a handle to the FUJ02E3
ACPI device.  Make fext_backlight() calls use that handle instead of the
one from struct fujitsu_laptop.  Move backlight power synchronization
from acpi_fujitsu_laptop_add() to fujitsu_backlight_register().

This makes the bl_device field of struct fujitsu_bl redundant, so remove
it.

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

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index ea3210ee83ec..5f6b34a97348 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -131,9 +131,9 @@
 /* Device controlling the backlight and associated keys */
 struct fujitsu_bl {
 	acpi_handle handle;
+	acpi_handle fext_handle;
 	struct input_dev *input;
 	char phys[32];
-	struct backlight_device *bl_device;
 	unsigned int max_brightness;
 	unsigned int brightness_level;
 };
@@ -290,10 +290,12 @@ static int bl_get_brightness(struct backlight_device *b)
 
 static int bl_update_status(struct backlight_device *b)
 {
-	if (b->props.power == FB_BLANK_POWERDOWN)
-		fext_backlight(fujitsu_laptop->handle, 0x1, 0x4, 0x3);
-	else
-		fext_backlight(fujitsu_laptop->handle, 0x1, 0x4, 0x0);
+	if (fujitsu_bl->fext_handle) {
+		if (b->props.power == FB_BLANK_POWERDOWN)
+			fext_backlight(fujitsu_bl->fext_handle, 0x1, 0x4, 0x3);
+		else
+			fext_backlight(fujitsu_bl->fext_handle, 0x1, 0x4, 0x0);
+	}
 
 	return set_lcd_level(b->props.brightness);
 }
@@ -397,6 +399,7 @@ static int fujitsu_backlight_register(struct acpi_device *device)
 		.type = BACKLIGHT_PLATFORM
 	};
 	struct backlight_device *bd;
+	acpi_status status;
 
 	bd = devm_backlight_device_register(&device->dev, "fujitsu-laptop",
 					    &device->dev, NULL,
@@ -404,7 +407,10 @@ static int fujitsu_backlight_register(struct acpi_device *device)
 	if (IS_ERR(bd))
 		return PTR_ERR(bd);
 
-	fujitsu_bl->bl_device = bd;
+	status = acpi_get_handle(NULL, "\\_SB.FEXT", &fujitsu_bl->fext_handle);
+	if (ACPI_SUCCESS(status) &&
+	    fext_backlight(fujitsu_bl->fext_handle, 0x2, 0x4, 0x0) == 3)
+		bd->props.power = FB_BLANK_POWERDOWN;
 
 	return 0;
 }
@@ -861,15 +867,6 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 	pr_info("BTNI: [0x%x]\n", fext_buttons(fujitsu_laptop->handle,
 					       0x0, 0x0, 0x0));
 
-	/* Sync backlight power status */
-	if (fujitsu_bl->bl_device &&
-	    acpi_video_get_backlight_type() == acpi_backlight_vendor) {
-		if (fext_backlight(fujitsu_laptop->handle, 0x2, 0x4, 0x0) == 3)
-			fujitsu_bl->bl_device->props.power = FB_BLANK_POWERDOWN;
-		else
-			fujitsu_bl->bl_device->props.power = FB_BLANK_UNBLANK;
-	}
-
 	error = acpi_fujitsu_laptop_leds_register(device);
 	if (error)
 		goto err_free_fifo;
-- 
2.12.2

^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH 05/10] platform/x86: fujitsu-laptop: distinguish current uses of device-specific data
  2017-04-24 13:33 [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals Michał Kępień
                   ` (3 preceding siblings ...)
  2017-04-24 13:33 ` [PATCH 04/10] platform/x86: fujitsu-laptop: rework backlight power synchronization Michał Kępień
@ 2017-04-24 13:33 ` Michał Kępień
  2017-05-01 13:40   ` Jonathan Woithe
  2017-04-24 13:33 ` [PATCH 06/10] platform/x86: fujitsu-laptop: allocate struct fujitsu_bl in acpi_fujitsu_bl_add() Michał Kępień
                   ` (5 subsequent siblings)
  10 siblings, 1 reply; 39+ messages in thread
From: Michał Kępień @ 2017-04-24 13:33 UTC (permalink / raw)
  To: Jonathan Woithe, Darren Hart, Andy Shevchenko
  Cc: platform-driver-x86, linux-kernel

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 5f6b34a97348..536b601c7067 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -369,26 +369,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)
@@ -566,27 +566,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)
@@ -885,11 +885,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.12.2

^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH 06/10] platform/x86: fujitsu-laptop: allocate struct fujitsu_bl in acpi_fujitsu_bl_add()
  2017-04-24 13:33 [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals Michał Kępień
                   ` (4 preceding siblings ...)
  2017-04-24 13:33 ` [PATCH 05/10] platform/x86: fujitsu-laptop: distinguish current uses of device-specific data Michał Kępień
@ 2017-04-24 13:33 ` Michał Kępień
  2017-04-24 13:33 ` [PATCH 07/10] platform/x86: fujitsu-laptop: use device-specific data in backlight code Michał Kępień
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 39+ messages in thread
From: Michał Kępień @ 2017-04-24 13:33 UTC (permalink / raw)
  To: Jonathan Woithe, Darren Hart, Andy Shevchenko
  Cc: platform-driver-x86, linux-kernel

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

Until all backlight-related code is modified to only use device-specific
data, the pointer to the allocated memory still has to be stored in a
module-wide variable.

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

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 536b601c7067..780e11b43d27 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -417,6 +417,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;
 
@@ -426,10 +427,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->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)
@@ -1018,13 +1024,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 */
 
@@ -1054,8 +1056,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;
 }
@@ -1070,8 +1070,6 @@ static void __exit fujitsu_cleanup(void)
 
 	acpi_bus_unregister_driver(&acpi_fujitsu_bl_driver);
 
-	kfree(fujitsu_bl);
-
 	pr_info("driver unloaded\n");
 }
 
-- 
2.12.2

^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH 07/10] platform/x86: fujitsu-laptop: use device-specific data in backlight code
  2017-04-24 13:33 [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals Michał Kępień
                   ` (5 preceding siblings ...)
  2017-04-24 13:33 ` [PATCH 06/10] platform/x86: fujitsu-laptop: allocate struct fujitsu_bl in acpi_fujitsu_bl_add() Michał Kępień
@ 2017-04-24 13:33 ` Michał Kępień
  2017-04-24 13:33 ` [PATCH 08/10] platform/x86: fujitsu-laptop: allocate struct fujitsu_laptop in acpi_fujitsu_laptop_add() Michał Kępień
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 39+ messages in thread
From: Michał Kępień @ 2017-04-24 13:33 UTC (permalink / raw)
  To: Jonathan Woithe, Darren Hart, Andy Shevchenko
  Cc: platform-driver-x86, linux-kernel

To prevent using module-wide data in backlight-related code, employ
acpi_driver_data() and bl_get_data() to fetch device-specific data to
work on in each function.  This makes the input local variable in
acpi_fujitsu_bl_notify() redundant, so remove it.  Remove the
module-wide struct fujitsu_bl as it is now redundant.

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

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 780e11b43d27..fb46652250c7 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -138,7 +138,6 @@ struct fujitsu_bl {
 	unsigned int brightness_level;
 };
 
-static struct fujitsu_bl *fujitsu_bl;
 static int use_alt_lcd_levels = -1;
 static bool disable_brightness_adjust;
 
@@ -209,14 +208,15 @@ static int fext_leds(acpi_handle handle, 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->handle, "SBL2"))
+		if (acpi_has_method(priv->handle, "SBL2"))
 			method = "SBL2";
 		else
 			method = "SBLL";
@@ -232,72 +232,77 @@ 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->handle, method, level);
+	status = acpi_execute_simple_method(priv->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->handle, "GBLL", NULL,
-				       &state);
+	status = acpi_evaluate_integer(priv->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->handle, "RBLL", NULL,
-				       &state);
+	status = acpi_evaluate_integer(priv->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)
 {
-	if (fujitsu_bl->fext_handle) {
+	struct acpi_device *device = bl_get_data(b);
+	struct fujitsu_bl *priv = acpi_driver_data(device);
+
+	if (priv->fext_handle) {
 		if (b->props.power == FB_BLANK_POWERDOWN)
-			fext_backlight(fujitsu_bl->fext_handle, 0x1, 0x4, 0x3);
+			fext_backlight(priv->fext_handle, 0x1, 0x4, 0x3);
 		else
-			fext_backlight(fujitsu_bl->fext_handle, 0x1, 0x4, 0x0);
+			fext_backlight(priv->fext_handle, 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 = {
@@ -393,23 +398,24 @@ 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;
 	acpi_status status;
 
 	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);
 
-	status = acpi_get_handle(NULL, "\\_SB.FEXT", &fujitsu_bl->fext_handle);
+	status = acpi_get_handle(NULL, "\\_SB.FEXT", &priv->fext_handle);
 	if (ACPI_SUCCESS(status) &&
-	    fext_backlight(fujitsu_bl->fext_handle, 0x2, 0x4, 0x0) == 3)
+	    fext_backlight(priv->fext_handle, 0x2, 0x4, 0x0) == 3)
 		bd->props.power = FB_BLANK_POWERDOWN;
 
 	return 0;
@@ -431,8 +437,7 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device)
 	if (!priv)
 		return -ENOMEM;
 
-	fujitsu_bl = priv;
-	fujitsu_bl->handle = device->handle;
+	priv->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;
@@ -441,7 +446,7 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device)
 	if (error)
 		return error;
 
-	error = acpi_bus_update_power(fujitsu_bl->handle, &state);
+	error = acpi_bus_update_power(priv->handle, &state);
 	if (error) {
 		pr_err("Error reading power state\n");
 		return error;
@@ -451,17 +456,17 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device)
 	       acpi_device_name(device), acpi_device_bid(device),
 	       !device->power.state ? "on" : "off");
 
-	if (acpi_has_method(device->handle, METHOD_NAME__INI)) {
+	if (acpi_has_method(priv->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)))
+		     (priv->handle, METHOD_NAME__INI, NULL, NULL)))
 			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)
@@ -474,21 +479,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);
@@ -497,9 +500,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.12.2

^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH 08/10] platform/x86: fujitsu-laptop: allocate struct fujitsu_laptop in acpi_fujitsu_laptop_add()
  2017-04-24 13:33 [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals Michał Kępień
                   ` (6 preceding siblings ...)
  2017-04-24 13:33 ` [PATCH 07/10] platform/x86: fujitsu-laptop: use device-specific data in backlight code Michał Kępień
@ 2017-04-24 13:33 ` Michał Kępień
  2017-04-24 13:33 ` [PATCH 09/10] platform/x86: fujitsu-laptop: use device-specific data in LED-related code Michał Kępień
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 39+ messages in thread
From: Michał Kępień @ 2017-04-24 13:33 UTC (permalink / raw)
  To: Jonathan Woithe, Darren Hart, Andy Shevchenko
  Cc: platform-driver-x86, linux-kernel

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

Until all remaining module code is modified to only use device-specific
data, the pointer to the allocated memory still has to be stored in a
module-wide variable.

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 fb46652250c7..f26abc41266e 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -808,6 +808,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;
@@ -815,11 +816,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->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);
@@ -1039,22 +1045,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:
@@ -1067,8 +1065,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.12.2

^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH 09/10] platform/x86: fujitsu-laptop: use device-specific data in LED-related code
  2017-04-24 13:33 [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals Michał Kępień
                   ` (7 preceding siblings ...)
  2017-04-24 13:33 ` [PATCH 08/10] platform/x86: fujitsu-laptop: allocate struct fujitsu_laptop in acpi_fujitsu_laptop_add() Michał Kępień
@ 2017-04-24 13:33 ` Michał Kępień
  2017-04-24 13:33 ` [PATCH 10/10] platform/x86: fujitsu-laptop: use device-specific data in remaining module code Michał Kępień
  2017-05-01 13:05 ` [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals Jonathan Woithe
  10 siblings, 0 replies; 39+ messages in thread
From: Michał Kępień @ 2017-04-24 13:33 UTC (permalink / raw)
  To: Jonathan Woithe, Darren Hart, Andy Shevchenko
  Cc: platform-driver-x86, linux-kernel

In order to perform their duties, all LED callbacks need a handle to the
FUJ02E3 ACPI device.  If that handle is to be fetched without accessing
module-wide data, it needs to be extracted from data that gets passed to
the LED callbacks as arguments.  However, LED core does not currently
support supplying driver-specific pointers to struct led_classdev
callbacks, so the latter will 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, we can get to the struct acpi_device
representing the former by following the parent link present inside the
struct device belonging to the struct led_classdev passed as an argument
to each LED callback.  Note that acpi_driver_data() is not used to
retrieve the ACPI handle.  Doing that would break cleanup upon module
removal, because the managed LED class device release callback,
devm_led_classdev_release(), would be called after the bus-level device
removal callback, acpi_device_remove(), which sets the driver_data
member of struct acpi_device to NULL and as devm_led_classdev_release()
calls led_classdev_unregister(), which in turn resets the LED's
brightness to LED_OFF, LED callbacks would not be able to retrieve the
ACPI handle needed to perform that operation.  To work around this, the
handle is retrieved directly from struct acpi_device as the latter is
guaranteed to be available upon brightness reset because parent device's
reference count is only decremented once brightness gets reset.

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 | 113 ++++++++++++++++------------------
 1 file changed, 53 insertions(+), 60 deletions(-)

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index f26abc41266e..77082e35f26a 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -635,6 +635,7 @@ static void fujitsu_laptop_platform_remove(void)
 static int logolamp_set(struct led_classdev *cdev,
 			enum led_brightness brightness)
 {
+	acpi_handle handle = to_acpi_device(cdev->dev->parent)->handle;
 	int poweron = FUNC_LED_ON, always = FUNC_LED_ON;
 	int ret;
 
@@ -644,136 +645,120 @@ static int logolamp_set(struct led_classdev *cdev,
 	if (brightness < LED_FULL)
 		always = FUNC_LED_OFF;
 
-	ret = fext_leds(fujitsu_laptop->handle, 0x1, LOGOLAMP_POWERON, poweron);
+	ret = fext_leds(handle, 0x1, LOGOLAMP_POWERON, poweron);
 	if (ret < 0)
 		return ret;
 
-	return fext_leds(fujitsu_laptop->handle, 0x1, LOGOLAMP_ALWAYS, always);
+	return fext_leds(handle, 0x1, LOGOLAMP_ALWAYS, always);
 }
 
 static enum led_brightness logolamp_get(struct led_classdev *cdev)
 {
+	acpi_handle handle = to_acpi_device(cdev->dev->parent)->handle;
 	int ret;
 
-	ret = fext_leds(fujitsu_laptop->handle, 0x2, LOGOLAMP_ALWAYS, 0x0);
+	ret = fext_leds(handle, 0x2, LOGOLAMP_ALWAYS, 0x0);
 	if (ret == FUNC_LED_ON)
 		return LED_FULL;
 
-	ret = fext_leds(fujitsu_laptop->handle, 0x2, LOGOLAMP_POWERON, 0x0);
+	ret = fext_leds(handle, 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)
 {
+	acpi_handle handle = to_acpi_device(cdev->dev->parent)->handle;
+
 	if (brightness >= LED_FULL)
-		return fext_leds(fujitsu_laptop->handle, 0x1, KEYBOARD_LAMPS,
-				 FUNC_LED_ON);
+		return fext_leds(handle, 0x1, KEYBOARD_LAMPS, FUNC_LED_ON);
 	else
-		return fext_leds(fujitsu_laptop->handle, 0x1, KEYBOARD_LAMPS,
-				 FUNC_LED_OFF);
+		return fext_leds(handle, 0x1, KEYBOARD_LAMPS, FUNC_LED_OFF);
 }
 
 static enum led_brightness kblamps_get(struct led_classdev *cdev)
 {
+	acpi_handle handle = to_acpi_device(cdev->dev->parent)->handle;
 	enum led_brightness brightness = LED_OFF;
 
-	if (fext_leds(fujitsu_laptop->handle,
-		      0x2, KEYBOARD_LAMPS, 0x0) == FUNC_LED_ON)
+	if (fext_leds(handle, 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)
 {
+	acpi_handle handle = to_acpi_device(cdev->dev->parent)->handle;
+
 	if (brightness >= LED_FULL)
-		return fext_flags(fujitsu_laptop->handle, 0x5, RADIO_LED_ON,
-				  RADIO_LED_ON);
+		return fext_flags(handle, 0x5, RADIO_LED_ON, RADIO_LED_ON);
 	else
-		return fext_flags(fujitsu_laptop->handle, 0x5, RADIO_LED_ON,
-				  0x0);
+		return fext_flags(handle, 0x5, RADIO_LED_ON, 0x0);
 }
 
 static enum led_brightness radio_led_get(struct led_classdev *cdev)
 {
+	acpi_handle handle = to_acpi_device(cdev->dev->parent)->handle;
 	enum led_brightness brightness = LED_OFF;
 
-	if (fext_flags(fujitsu_laptop->handle, 0x4, 0x0, 0x0) & RADIO_LED_ON)
+	if (fext_flags(handle, 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)
 {
+	acpi_handle handle = to_acpi_device(cdev->dev->parent)->handle;
 	int curr;
 
-	curr = fext_leds(fujitsu_laptop->handle, 0x2, ECO_LED, 0x0);
+	curr = fext_leds(handle, 0x2, ECO_LED, 0x0);
 	if (brightness >= LED_FULL)
-		return fext_leds(fujitsu_laptop->handle, 0x1, ECO_LED,
-				 curr | ECO_LED_ON);
+		return fext_leds(handle, 0x1, ECO_LED, curr | ECO_LED_ON);
 	else
-		return fext_leds(fujitsu_laptop->handle, 0x1, ECO_LED,
-				 curr & ~ECO_LED_ON);
+		return fext_leds(handle, 0x1, ECO_LED, curr & ~ECO_LED_ON);
 }
 
 static enum led_brightness eco_led_get(struct led_classdev *cdev)
 {
+	acpi_handle handle = to_acpi_device(cdev->dev->parent)->handle;
 	enum led_brightness brightness = LED_OFF;
 
-	if (fext_leds(fujitsu_laptop->handle, 0x2, ECO_LED, 0x0) & ECO_LED_ON)
+	if (fext_leds(handle, 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 fujitsu_laptop *priv = acpi_driver_data(device);
+	struct led_classdev *led;
 	int result;
 
-	if (fext_leds(fujitsu_laptop->handle,
-		      0x0, 0x0, 0x0) & LOGOLAMP_POWERON) {
-		result = devm_led_classdev_register(&device->dev,
-						    &logolamp_led);
+	if (fext_leds(priv->handle, 0x0, 0x0, 0x0) & LOGOLAMP_POWERON) {
+		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 ((fext_leds(fujitsu_laptop->handle,
-		       0x0, 0x0, 0x0) & KEYBOARD_LAMPS) &&
-	    (fext_buttons(fujitsu_laptop->handle, 0x0, 0x0, 0x0) == 0x0)) {
-		result = devm_led_classdev_register(&device->dev, &kblamps_led);
+	if ((fext_leds(priv->handle, 0x0, 0x0, 0x0) & KEYBOARD_LAMPS) &&
+	    (fext_buttons(priv->handle, 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;
 	}
@@ -784,8 +769,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 (fext_buttons(fujitsu_laptop->handle, 0x0, 0x0, 0x0) & BIT(24)) {
-		result = devm_led_classdev_register(&device->dev, &radio_led);
+	if (fext_buttons(priv->handle, 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;
 	}
@@ -795,10 +785,13 @@ 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 ((fext_leds(fujitsu_laptop->handle, 0x0, 0x0, 0x0) & BIT(14)) &&
-	    (fext_leds(fujitsu_laptop->handle,
-		       0x2, ECO_LED, 0x0) != UNSUPPORTED_CMD)) {
-		result = devm_led_classdev_register(&device->dev, &eco_led);
+	if ((fext_leds(priv->handle, 0x0, 0x0, 0x0) & BIT(14)) &&
+	    (fext_leds(priv->handle, 0x2, ECO_LED, 0x0) != UNSUPPORTED_CMD)) {
+		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.12.2

^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH 10/10] platform/x86: fujitsu-laptop: use device-specific data in remaining module code
  2017-04-24 13:33 [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals Michał Kępień
                   ` (8 preceding siblings ...)
  2017-04-24 13:33 ` [PATCH 09/10] platform/x86: fujitsu-laptop: use device-specific data in LED-related code Michał Kępień
@ 2017-04-24 13:33 ` Michał Kępień
  2017-05-01 13:05 ` [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals Jonathan Woithe
  10 siblings, 0 replies; 39+ messages in thread
From: Michał Kępień @ 2017-04-24 13:33 UTC (permalink / raw)
  To: Jonathan Woithe, Darren Hart, Andy Shevchenko
  Cc: platform-driver-x86, linux-kernel

To prevent 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 redundant, so remove them.  Remove the
module-wide struct fujitsu_laptop as it is now redundant.  Adjust
whitespace to make checkpatch happy.

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

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 77082e35f26a..9275f3c829a2 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -154,8 +154,6 @@ struct fujitsu_laptop {
 	int flags_state;
 };
 
-static struct fujitsu_laptop *fujitsu_laptop;
-
 #ifdef CONFIG_FUJITSU_LAPTOP_DEBUG
 static u32 dbg_level = 0x03;
 #endif
@@ -313,9 +311,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");
@@ -324,9 +324,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");
@@ -335,9 +337,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");
@@ -598,19 +602,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;
@@ -618,18 +625,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,
@@ -813,17 +822,16 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 	if (!priv)
 		return -ENOMEM;
 
-	fujitsu_laptop = priv;
-	fujitsu_laptop->handle = device->handle;
+	priv->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 = 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;
@@ -833,7 +841,7 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 	if (error)
 		goto err_free_fifo;
 
-	error = acpi_bus_update_power(fujitsu_laptop->handle, &state);
+	error = acpi_bus_update_power(priv->handle, &state);
 	if (error) {
 		pr_err("Error reading power state\n");
 		goto err_free_fifo;
@@ -843,50 +851,47 @@ 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;
+	priv->dev = device;
 
-	if (acpi_has_method(device->handle, METHOD_NAME__INI)) {
+	if (acpi_has_method(priv->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)))
+		     (priv->handle, METHOD_NAME__INI, NULL, NULL)))
 			pr_err("_INI Method failed\n");
 	}
 
 	i = 0;
-	while (fext_buttons(fujitsu_laptop->handle, 0x1, 0x0, 0x0) != 0 &&
+	while (fext_buttons(priv->handle, 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 = fext_flags(fujitsu_laptop->handle,
-						     0x0, 0x0, 0x0);
+	priv->flags_supported = fext_flags(priv->handle, 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 = fext_flags(fujitsu_laptop->handle,
-							 0x4, 0x0, 0x0);
+	if (priv->flags_supported)
+		priv->flags_state = fext_flags(priv->handle, 0x4, 0x0, 0x0);
 
 	/* Suspect this is a keymap of the application panel, print it */
-	pr_info("BTNI: [0x%x]\n", fext_buttons(fujitsu_laptop->handle,
-					       0x0, 0x0, 0x0));
+	pr_info("BTNI: [0x%x]\n", fext_buttons(priv->handle, 0x0, 0x0, 0x0));
 
 	error = acpi_fujitsu_laptop_leds_register(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;
 }
@@ -895,44 +900,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);
 	}
@@ -940,31 +943,27 @@ 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 = fext_flags(fujitsu_laptop->handle,
-							 0x4, 0x0, 0x0);
+	if (priv->flags_supported)
+		priv->flags_state = fext_flags(priv->handle, 0x4, 0x0, 0x0);
 
-	while ((irb = fext_buttons(fujitsu_laptop->handle,
-				   0x1, 0x0, 0x0)) != 0 &&
+	while ((irb = fext_buttons(priv->handle, 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);
@@ -974,9 +973,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)) &&
-	    (fext_flags(fujitsu_laptop->handle, 0x1, 0x0, 0x0) & BIT(26)))
-		sparse_keymap_report_event(input, BIT(26), 1, true);
+	if ((priv->flags_supported & BIT(26)) &&
+	    (fext_flags(priv->handle, 0x1, 0x0, 0x0) & BIT(26)))
+		sparse_keymap_report_event(priv->input, BIT(26), 1, true);
 }
 
 /* Initialization */
-- 
2.12.2

^ permalink raw reply related	[flat|nested] 39+ messages in thread

* Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals
  2017-04-24 13:33 [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals Michał Kępień
                   ` (9 preceding siblings ...)
  2017-04-24 13:33 ` [PATCH 10/10] platform/x86: fujitsu-laptop: use device-specific data in remaining module code Michał Kępień
@ 2017-05-01 13:05 ` Jonathan Woithe
  2017-05-02 13:21   ` Michał Kępień
  10 siblings, 1 reply; 39+ messages in thread
From: Jonathan Woithe @ 2017-05-01 13:05 UTC (permalink / raw)
  To: Micha?? K??pie??
  Cc: Darren Hart, Andy Shevchenko, platform-driver-x86, linux-kernel

Hi Michael

On Mon, Apr 24, 2017 at 03:33:24PM +0200, Micha?? K??pie?? wrote:
> fujitsu-laptop registers two ACPI drivers.  Whenever an ACPI device with
> a matching identifier is found by the ACPI bus, a new instance of the
> relevant driver is bound to that ACPI device.  However, both ACPI
> drivers registered by fujitsu-laptop access module-wide global data
> structures, assuming neither ACPI driver will ever be instantiated more
> than once.  While there are currently no indications of such issues
> happening in the wild, it is theoretically possible for multiple
> FUJ02B1/FUJ02E3 ACPI devices to be present in the firmware, which would
> cause two instances of the relevant driver to simultaneously access
> module-wide globals without any locking in place.  Also, modern Fujitsu
> laptops ship without the FUJ02B1 ACPI device present in firmware,
> causing memory to be needlessly allocated inside fujitsu_init().
> 
> To future-proof the module 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.

Apologies for the delay in getting this first set of feedback to you.  It's
a combination of the extent of the patch set and a very busy week.

This patch set represents another worthwhile clean up of the fujitsu-laptop
driver.  While I sincerely doubt any laptop vendor will place more than one
FUJ02B1 (or FUJ02E3) in a single machine, removing the dependency on global
variables makes the driver self contained and more consistent.  I have some
points of clarification which I will post as follow ups to the respective
patchs.

Regards
  jonathan

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 01/10] platform/x86: fujitsu-laptop: introduce fext_*() helper functions
  2017-04-24 13:33 ` [PATCH 01/10] platform/x86: fujitsu-laptop: introduce fext_*() helper functions Michał Kępień
@ 2017-05-01 13:13   ` Jonathan Woithe
  2017-05-02 13:24     ` Michał Kępień
  0 siblings, 1 reply; 39+ messages in thread
From: Jonathan Woithe @ 2017-05-01 13:13 UTC (permalink / raw)
  To: Micha?? K??pie??
  Cc: Darren Hart, Andy Shevchenko, platform-driver-x86, linux-kernel

On Mon, Apr 24, 2017 at 03:33:25PM +0200, Micha?? K??pie?? wrote:
> Stop invoking call_fext_func() directly to improve code clarity and save
> some horizontal space.  Adjust whitespace to make checkpatch happy.

A comment: this patch in and of itself does not seem to be worthwhile.  In
particular, the saving of horzontal space seems academic.  The value comes
when later patches build on it.

> Signed-off-by: Micha?? K??pie?? <kernel@kempniu.pl>
> ---
>  drivers/platform/x86/fujitsu-laptop.c | 90 ++++++++++++++++++++---------------
>  1 file changed, 51 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> index 7f49d92914c9..3f232967af04 100644
> --- a/drivers/platform/x86/fujitsu-laptop.c
> +++ b/drivers/platform/x86/fujitsu-laptop.c
> @@ -187,6 +187,26 @@ static int call_fext_func(int func, int op, int feature, int state)
>  	return value;
>  }
>  
> +static int fext_backlight(int op, int feature, int state)
> +{
> +	return call_fext_func(FUNC_BACKLIGHT, op, feature, state);
> +}
> +
> +static int fext_buttons(int op, int feature, int state)
> +{
> +	return call_fext_func(FUNC_BUTTONS, op, feature, state);
> +}
> +
> +static int fext_flags(int op, int feature, int state)
> +{
> +	return call_fext_func(FUNC_FLAGS, op, feature, state);
> +}
> +
> +static int fext_leds(int op, int feature, int state)
> +{
> +	return call_fext_func(FUNC_LEDS, op, feature, state);
> +}
> +
>  /* Hardware access for LCD brightness control */
>  
>  static int set_lcd_level(int level)
> @@ -272,9 +292,9 @@ static int bl_get_brightness(struct backlight_device *b)
>  static int bl_update_status(struct backlight_device *b)
>  {
>  	if (b->props.power == FB_BLANK_POWERDOWN)
> -		call_fext_func(FUNC_BACKLIGHT, 0x1, 0x4, 0x3);
> +		fext_backlight(0x1, 0x4, 0x3);
>  	else
> -		call_fext_func(FUNC_BACKLIGHT, 0x1, 0x4, 0x0);
> +		fext_backlight(0x1, 0x4, 0x0);
>  
>  	return set_lcd_level(b->props.brightness);
>  }
> @@ -610,22 +630,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 = fext_leds(0x1, LOGOLAMP_POWERON, poweron);
>  	if (ret < 0)
>  		return ret;
>  
> -	return call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_ALWAYS, always);
> +	return fext_leds(0x1, LOGOLAMP_ALWAYS, always);
>  }

I've only just noticed this.  For the led calls we have symbolic identifiers
defined for the "features" parameter, but in the backlight case we are still
using arbitrary numeric constants.  Although not necessary for this patch
set, we should consider adding feature identifiers for the other fext_*() calls.
Similarly for the "op" parameter where it makes sense to do so.

Regards
  jonathan

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 02/10] platform/x86: fujitsu-laptop: shorten names of acpi_handle fields
  2017-04-24 13:33 ` [PATCH 02/10] platform/x86: fujitsu-laptop: shorten names of acpi_handle fields Michał Kępień
@ 2017-05-01 13:19   ` Jonathan Woithe
  2017-05-01 16:09     ` Darren Hart
  0 siblings, 1 reply; 39+ messages in thread
From: Jonathan Woithe @ 2017-05-01 13:19 UTC (permalink / raw)
  To: Micha?? K??pie??
  Cc: Darren Hart, Andy Shevchenko, platform-driver-x86, linux-kernel

On Mon, Apr 24, 2017 at 03:33:26PM +0200, Micha?? K??pie?? wrote:
> As both struct fujitsu_bl and struct fujitsu_laptop represent data
> associated with ACPI devices, drop the "acpi_" prefix from the names of
> the relevant fields of these structures to save some horizontal space.
> 
> Signed-off-by: Micha?? K??pie?? <kernel@kempniu.pl>
> ---
>  drivers/platform/x86/fujitsu-laptop.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> index 3f232967af04..3695e8075aa6 100644
> --- a/drivers/platform/x86/fujitsu-laptop.c
> +++ b/drivers/platform/x86/fujitsu-laptop.c
> @@ -130,7 +130,7 @@
>  
>  /* Device controlling the backlight and associated keys */
>  struct fujitsu_bl {
> -	acpi_handle acpi_handle;
> +	acpi_handle handle;

I must admit I'm not entirely convinced about this change.  "handle" to me
is very generic and it's not immediately clear from the source usage what it
might be a handle of.  A later patch in the series introduces an additional
handle which includes a suitable suffix, which leaves us with generic and
specific handles within the code.  Although it consumes an additional 5
characters, my feeling is that the additional clarification is worth it.

Regards
  jonathan

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 04/10] platform/x86: fujitsu-laptop: rework backlight power synchronization
  2017-04-24 13:33 ` [PATCH 04/10] platform/x86: fujitsu-laptop: rework backlight power synchronization Michał Kępień
@ 2017-05-01 13:32   ` Jonathan Woithe
  2017-05-01 16:17     ` Darren Hart
  0 siblings, 1 reply; 39+ messages in thread
From: Jonathan Woithe @ 2017-05-01 13:32 UTC (permalink / raw)
  To: Micha?? K??pie??
  Cc: Darren Hart, Andy Shevchenko, platform-driver-x86, linux-kernel

On Mon, Apr 24, 2017 at 03:33:28PM +0200, Micha?? K??pie?? wrote:
> 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.)  So far, these two
> drivers have been entangled by calls to fext_backlight() (previously
> known as call_fext_func()) in the backlight part of the module which use
> module-wide data managed by the other part of the module and accesses to
> the backlight device from within acpi_fujitsu_laptop_add().  This
> entaglement can be solved by storing an independently fetched ACPI
> handle to the FUJ02E3 device inside the data structure managed by the
> backlight part of the module.
> 
> Add a field to struct fujitsu_bl for storing a handle to the FUJ02E3
> ACPI device.  Make fext_backlight() calls use that handle instead of the
> one from struct fujitsu_laptop.  Move backlight power synchronization
> from acpi_fujitsu_laptop_add() to fujitsu_backlight_register().
> 
> This makes the bl_device field of struct fujitsu_bl redundant, so remove
> it.
> 
> Signed-off-by: Micha?? K??pie?? <kernel@kempniu.pl>
> ---
>  drivers/platform/x86/fujitsu-laptop.c | 27 ++++++++++++---------------
>  1 file changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> index ea3210ee83ec..5f6b34a97348 100644
> --- a/drivers/platform/x86/fujitsu-laptop.c
> +++ b/drivers/platform/x86/fujitsu-laptop.c
> @@ -131,9 +131,9 @@
>  /* Device controlling the backlight and associated keys */
>  struct fujitsu_bl {
>  	acpi_handle handle;
> +	acpi_handle fext_handle;

This is the extra "handle" field I eluded to in my comment about an earlier
part of this patch series.  The end result is two "handle" fields: one whose
job is obvious (fext_handle) and one whose name in no way reflects what it
might be used for (handle).  One could of course adopt the view that any
unqualified handle is a generic acpi handle, but I like the clarification
which comes with the additional suffix.  Perhaps "acpi_handle" is too
generic and I'm certainly not opposed to the use of something more specific. 
However, IMHO leaving it as "handle" seems like an unnecessary obfuscation
without much gain.

This change reinforces the shift away from ACPI drivers linked to specific
ACPI devices, and towards a focus on the driver's functionality (backlight
and "various other stuff").  With the evolution of the hardware I think this
makes sense.  While the "other stuff" still needs a driver, the backlight
component is not always needed.  The case for separation therefore makes
sense.

As a point of discussion though, since the backlight driver needs access to
both FUJ02B1 and FUJ02E3, should we consider rolling both ACPI drivers into
one?  Aside from conceptual neatness and perhaps a small runtime memory
footprint saving in the event that no backlight control functionality need
be provided by fujitsu-laptop, is there a whole lot to be gained through the
use of two separate drivers?

Regards
  jonathan

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 05/10] platform/x86: fujitsu-laptop: distinguish current uses of device-specific data
  2017-04-24 13:33 ` [PATCH 05/10] platform/x86: fujitsu-laptop: distinguish current uses of device-specific data Michał Kępień
@ 2017-05-01 13:40   ` Jonathan Woithe
  0 siblings, 0 replies; 39+ messages in thread
From: Jonathan Woithe @ 2017-05-01 13:40 UTC (permalink / raw)
  To: Micha?? K??pie??
  Cc: Darren Hart, Andy Shevchenko, platform-driver-x86, linux-kernel

On Mon, Apr 24, 2017 at 03:33:29PM +0200, Micha?? K??pie?? wrote:
> 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 5f6b34a97348..536b601c7067 100644
> --- a/drivers/platform/x86/fujitsu-laptop.c
> +++ b/drivers/platform/x86/fujitsu-laptop.c
> @@ -369,26 +369,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);

[cut]

>  static int fujitsu_backlight_register(struct acpi_device *device)
> @@ -566,27 +566,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;

Distinguishing between local and global use like this makes sense, but I
feel we should stick with a slightly more descriptive name than "priv". 
Without any qualification, "priv" could refer to private device-specific
data from either the fujitsu_bl or fujitsu_laptop drivers.  From the source
it is far from obvious which is being accessed in a given function.  If we
implemented only a single ACPI device driver then this would be largely a
moot point, but as there are two within the one module the loss of the
description could make it harder to follow the code later on.

Could we use "bl_priv" and "laptop_priv" for example, so as to provide a
clue within the source code as to what exactly is being referenced? 
Obviously it doesn't provide any compile time type checking, but it's better
than nothing.

Regards
  jonathan

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 02/10] platform/x86: fujitsu-laptop: shorten names of acpi_handle fields
  2017-05-01 13:19   ` Jonathan Woithe
@ 2017-05-01 16:09     ` Darren Hart
  0 siblings, 0 replies; 39+ messages in thread
From: Darren Hart @ 2017-05-01 16:09 UTC (permalink / raw)
  To: Jonathan Woithe
  Cc: Micha?? K??pie??, Andy Shevchenko, platform-driver-x86, linux-kernel

On Mon, May 01, 2017 at 10:49:26PM +0930, Jonathan Woithe wrote:
> On Mon, Apr 24, 2017 at 03:33:26PM +0200, Micha?? K??pie?? wrote:
> > As both struct fujitsu_bl and struct fujitsu_laptop represent data
> > associated with ACPI devices, drop the "acpi_" prefix from the names of
> > the relevant fields of these structures to save some horizontal space.
> > 
> > Signed-off-by: Micha?? K??pie?? <kernel@kempniu.pl>
> > ---
> >  drivers/platform/x86/fujitsu-laptop.c | 23 +++++++++++------------
> >  1 file changed, 11 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> > index 3f232967af04..3695e8075aa6 100644
> > --- a/drivers/platform/x86/fujitsu-laptop.c
> > +++ b/drivers/platform/x86/fujitsu-laptop.c
> > @@ -130,7 +130,7 @@
> >  
> >  /* Device controlling the backlight and associated keys */
> >  struct fujitsu_bl {
> > -	acpi_handle acpi_handle;
> > +	acpi_handle handle;
> 
> I must admit I'm not entirely convinced about this change.  "handle" to me
> is very generic and it's not immediately clear from the source usage what it
> might be a handle of.  A later patch in the series introduces an additional
> handle which includes a suitable suffix, which leaves us with generic and
> specific handles within the code.  Although it consumes an additional 5
> characters, my feeling is that the additional clarification is worth it.

ACPI handles are commonly "handle" in other drivers - and it does make me cringe
to see the type reused as the variable name :-) That much at least, I appreciate
in this patch.

dvhart@fury:~/source/linux/linux-pdx86 [testing]
$ git grep "acpi_handle handle" | wc -l
520

dvhart@fury:~/source/linux/linux-pdx86 [testing]
$ git grep "acpi_handle acpi_handle" | wc -l
4

-- 
Darren Hart
VMware Open Source Technology Center

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 04/10] platform/x86: fujitsu-laptop: rework backlight power synchronization
  2017-05-01 13:32   ` Jonathan Woithe
@ 2017-05-01 16:17     ` Darren Hart
  0 siblings, 0 replies; 39+ messages in thread
From: Darren Hart @ 2017-05-01 16:17 UTC (permalink / raw)
  To: Jonathan Woithe
  Cc: Micha?? K??pie??, Andy Shevchenko, platform-driver-x86, linux-kernel

On Mon, May 01, 2017 at 11:02:45PM +0930, Jonathan Woithe wrote:
> On Mon, Apr 24, 2017 at 03:33:28PM +0200, Micha?? K??pie?? wrote:
> > 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.)  So far, these two
> > drivers have been entangled by calls to fext_backlight() (previously
> > known as call_fext_func()) in the backlight part of the module which use
> > module-wide data managed by the other part of the module and accesses to
> > the backlight device from within acpi_fujitsu_laptop_add().  This
> > entaglement can be solved by storing an independently fetched ACPI
> > handle to the FUJ02E3 device inside the data structure managed by the
> > backlight part of the module.
> > 
> > Add a field to struct fujitsu_bl for storing a handle to the FUJ02E3
> > ACPI device.  Make fext_backlight() calls use that handle instead of the
> > one from struct fujitsu_laptop.  Move backlight power synchronization
> > from acpi_fujitsu_laptop_add() to fujitsu_backlight_register().
> > 
> > This makes the bl_device field of struct fujitsu_bl redundant, so remove
> > it.
> > 
> > Signed-off-by: Micha?? K??pie?? <kernel@kempniu.pl>
> > ---
> >  drivers/platform/x86/fujitsu-laptop.c | 27 ++++++++++++---------------
> >  1 file changed, 12 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> > index ea3210ee83ec..5f6b34a97348 100644
> > --- a/drivers/platform/x86/fujitsu-laptop.c
> > +++ b/drivers/platform/x86/fujitsu-laptop.c
> > @@ -131,9 +131,9 @@
> >  /* Device controlling the backlight and associated keys */
> >  struct fujitsu_bl {
> >  	acpi_handle handle;
> > +	acpi_handle fext_handle;
> 
> This is the extra "handle" field I eluded to in my comment about an earlier
> part of this patch series.  The end result is two "handle" fields: one whose
> job is obvious (fext_handle) and one whose name in no way reflects what it
> might be used for (handle).  One could of course adopt the view that any
> unqualified handle is a generic acpi handle, but I like the clarification
> which comes with the additional suffix.  Perhaps "acpi_handle" is too
> generic and I'm certainly not opposed to the use of something more specific. 
> However, IMHO leaving it as "handle" seems like an unnecessary obfuscation
> without much gain.

Yeah, that's a fair criticism. Naming is hard. I can see the argument for
"handle" is for the system in general, "fext_handle" is for this specific subset
of functionality". The alternative appears to be "plt_handle", "fuj_handle",
"main_handle", or "hk_led_etc_handle ;-)" which honestly doesn't add any new
information or is just silly. So, if you want to prefix handle, go ahead and do
so, but I don't think it's a big deal.

> 
> This change reinforces the shift away from ACPI drivers linked to specific
> ACPI devices, and towards a focus on the driver's functionality (backlight
> and "various other stuff").  With the evolution of the hardware I think this
> makes sense.  While the "other stuff" still needs a driver, the backlight
> component is not always needed.  The case for separation therefore makes
> sense.
> 
> As a point of discussion though, since the backlight driver needs access to
> both FUJ02B1 and FUJ02E3, should we consider rolling both ACPI drivers into
> one?  Aside from conceptual neatness and perhaps a small runtime memory
> footprint saving in the event that no backlight control functionality need
> be provided by fujitsu-laptop, is there a whole lot to be gained through the
> use of two separate drivers?

This on the other hand is something we need to consider carefully. I'd like to
hear from Michal on this before I comment further.

-- 
Darren Hart
VMware Open Source Technology Center

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals
  2017-05-01 13:05 ` [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals Jonathan Woithe
@ 2017-05-02 13:21   ` Michał Kępień
  2017-05-04 23:40     ` Jonathan Woithe
  0 siblings, 1 reply; 39+ messages in thread
From: Michał Kępień @ 2017-05-02 13:21 UTC (permalink / raw)
  To: Jonathan Woithe
  Cc: Darren Hart, Andy Shevchenko, platform-driver-x86, linux-kernel

> Hi Michael
> 
> On Mon, Apr 24, 2017 at 03:33:24PM +0200, Micha?? K??pie?? wrote:
> > fujitsu-laptop registers two ACPI drivers.  Whenever an ACPI device with
> > a matching identifier is found by the ACPI bus, a new instance of the
> > relevant driver is bound to that ACPI device.  However, both ACPI
> > drivers registered by fujitsu-laptop access module-wide global data
> > structures, assuming neither ACPI driver will ever be instantiated more
> > than once.  While there are currently no indications of such issues
> > happening in the wild, it is theoretically possible for multiple
> > FUJ02B1/FUJ02E3 ACPI devices to be present in the firmware, which would
> > cause two instances of the relevant driver to simultaneously access
> > module-wide globals without any locking in place.  Also, modern Fujitsu
> > laptops ship without the FUJ02B1 ACPI device present in firmware,
> > causing memory to be needlessly allocated inside fujitsu_init().
> > 
> > To future-proof the module 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.
> 
> Apologies for the delay in getting this first set of feedback to you.  It's
> a combination of the extent of the patch set and a very busy week.
> 
> This patch set represents another worthwhile clean up of the fujitsu-laptop
> driver.  While I sincerely doubt any laptop vendor will place more than one
> FUJ02B1 (or FUJ02E3) in a single machine, removing the dependency on global
> variables makes the driver self contained and more consistent.  I have some
> points of clarification which I will post as follow ups to the respective
> patchs.

Jonathan,

Thanks for the review.  My hidden agenda, which, in retrospect,
I probably should have included in the cover letter, follows.

In order to avoid accessing global structures from call_fext_func(), we
need to pass it an ACPI handle to FUJ02E3.  This decreases code
readability in two ways: by increasing the function's parameter count
from an already challenging four to an even worse five and by causing
line breaks to be inserted (due to the 80-column line rule) in places
they were previously not necessary in.

To counter this growing obfuscation, patches 01/10, 02/10 and 05/10 (all
called out in your review) work in tandem to ensure that all uses of
call_fext_func() remain legible _and_ fit in one line.  All three of
these patches are needed to prevent line breaks from being inserted
(granted, that is an arbitrary objective), because call_fext_func()
needs to get the ACPI handle somehow and the latter is stored in a field
of a device-specific structure.  Thus, for all call sites, these patches
shorten:

  - (01/10) name of the called function,
  - (02/10) name of the field holding the ACPI handle,
  - (05/10) name of the variable denoting device-specific data.

In other words, these patches are the only sane approach I could come up
with to ensure that, in the end, _all_ uses of call_fext_func() neatly
fit into a single line, thus ensuring reasonable readability even when
taking the added parameter (ACPI handle) into consideration.

I have pasted some examples at the end of this message of what a few
call_fext_func() call sites look like after adding the ACPI handle
parameter and fixing the code to make checkpatch happy, both with ("new
style") and without ("old style") the above three patches applied.  As
you can see, compound conditional expressions benefit the most from the
changes I suggested.

Separating fext_backlight() from the other functions of call_fext_func()
also has the added benefit of only exposing that specific function from
fujitsu-laptop to fujitsu-backlight (where fujitsu-backlight is the
backlight part of the current module), shall the module be split into
two.

And thus we come back to the question of "to split or not to split".
The three options we have are:

  - one module, two drivers: current, suboptimal, state of affairs,

  - two modules, one driver in each: the original cleanup approach I
    have been targeting in all of my patch series for fujitsu-laptop,

  - one module, one driver handling both ACPI devices: the new approach
    you suggested in your review.

I have not considered the last option until now as I deemed it
unacceptable in light of the kernel's philosophy in this regard.
However, such an approach might not be bad in and of itself, because:

  - FUJ02B1 is not fully standalone as it needs FUJ02E3 on some models,

  - FUJ02E3 is present in all models we know of, while FUJ02B1 seems to
    be phased out in newer models,

  - userspace is unlikely to care which input device each hotkey event
    comes from,

  - the memory footprint of both drivers is negligible, considering that
    both are only loaded on machines with hundreds of MB of RAM.

So we could perhaps make fujitsu-laptop register _one_ ACPI driver,
which binds to the FUJ02E3 device and only deals with backlight when the
FUJ02B1 device is present and the vendor interface is either
automatically selected by the kernel or explicitly requested by the
user.  We would then have a single device-specific structure ("priv"
would not be ambiguous any more) holding two ACPI handles ("fjex_handle"
and "fext_handle"?) and all the other fields from both struct fujitsu_bl
and struct fujitsu_laptop.  Please note that I have not played with this
idea in code yet and perhaps handling the added complexity will make the
driver more, not less, convoluted.

Darren, does the above sound more like a viable plan or rather a pipe
dream?  Answering Jonathan's question, there is no added benefit from
splitting fujitsu-laptop into two separate modules, it is only about
following the "one module, one driver" philosophy.  Any answer to this
question puts the variable naming discussion on a specific track, so
perhaps this is the first dilemma that we should sort out.

------------------------------------------------------------------------
old style:
	if ((call_fext_func(fujitsu_laptop->acpi_handle, FUNC_LEDS,
			    0x0, 0x0, 0x0) & BIT(14)) &&
	    (call_fext_func(fujitsu_laptop->acpi_handle, FUNC_LEDS,
	    		    0x2, ECO_LED, 0x0) != UNSUPPORTED_CMD)) {
new style:
	if ((fext_leds(priv->handle, 0x0, 0x0, 0x0) & BIT(14)) &&
	    (fext_leds(priv->handle, 0x2, ECO_LED, 0x0) != UNSUPPORTED_CMD)) {
------------------------------------------------------------------------
old style:
	if ((fujitsu_laptop->flags_supported & BIT(26)) &&
	    (call_fext_func(fujitsu_laptop->acpi_handle, FUNC_FLAGS,
	    	            0x1, 0x0, 0x0) & BIT(26)))
new style:
	if ((priv->flags_supported & BIT(26)) &&
	    (fext_flags(priv->handle, 0x1, 0x0, 0x0) & BIT(26)))
------------------------------------------------------------------------
old style:
	if (fujitsu_laptop->fext_handle) {
		if (b->props.power == FB_BLANK_POWERDOWN)
			call_fext_func(fujitsu_bl->fext_handle, FUNC_BACKLIGHT,
				       0x1, 0x4, 0x3);
		else
			call_fext_func(fujitsu_bl->fext_handle, FUNC_BACKLIGHT,
				       0x1, 0x4, 0x0);
	}
new style:
	if (priv->fext_handle) {
		if (b->props.power == FB_BLANK_POWERDOWN)
			fext_backlight(priv->fext_handle, 0x1, 0x4, 0x3);
		else
			fext_backlight(priv->fext_handle, 0x1, 0x4, 0x0);
	}
------------------------------------------------------------------------
old style:
	if (brightness >= LED_FULL)
		return call_fext_func(handle, FUNC_LEDS, 0x1, KEYBOARD_LAMPS,
				      FUNC_LED_ON);
	else
		return call_fext_func(handle, FUNC_LEDS, 0x1, KEYBOARD_LAMPS,
				      FUNC_LED_OFF);
new style:
	if (brightness >= LED_FULL)
		return fext_leds(handle, 0x1, KEYBOARD_LAMPS, FUNC_LED_ON);
	else
		return fext_leds(handle, 0x1, KEYBOARD_LAMPS, FUNC_LED_OFF);
------------------------------------------------------------------------
old style:
	if (call_fext_func(handle, FUNC_LEDS, 0x2, KEYBOARD_LAMPS,
			   0x0) == FUNC_LED_ON)
		brightness = LED_FULL;
new style:
	if (fext_leds(handle, 0x2, KEYBOARD_LAMPS, 0x0) == FUNC_LED_ON)
		brightness = LED_FULL;
------------------------------------------------------------------------
old style:
	if ((call_fext_func(fujitsu_laptop->acpi_handle, FUNC_LEDS,
			    0x0, 0x0, 0x0) & KEYBOARD_LAMPS) &&
	    (call_fext_func(fujitsu_laptop->acpi_handle, FUNC_LEDS,
	    		    0x0, 0x0, 0x0) == 0x0)) {
new style:
	if ((fext_leds(priv->handle, 0x0, 0x0, 0x0) & KEYBOARD_LAMPS) &&
	    (fext_buttons(priv->handle, 0x0, 0x0, 0x0) == 0x0)) {
------------------------------------------------------------------------
old style:
	while (call_fext_func(fujitsu_laptop->acpi_handle, FUNC_BUTTONS,
			      0x1, 0x0, 0x0) != 0 &&
	       i++ < MAX_HOTKEY_RINGBUFFER_SIZE)
new style:
	while (fext_buttons(priv->handle, 0x1, 0x0, 0x0) != 0 &&
	       i++ < MAX_HOTKEY_RINGBUFFER_SIZE)
------------------------------------------------------------------------

-- 
Best regards,
Michał Kępień

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 01/10] platform/x86: fujitsu-laptop: introduce fext_*() helper functions
  2017-05-01 13:13   ` Jonathan Woithe
@ 2017-05-02 13:24     ` Michał Kępień
  0 siblings, 0 replies; 39+ messages in thread
From: Michał Kępień @ 2017-05-02 13:24 UTC (permalink / raw)
  To: Jonathan Woithe
  Cc: Darren Hart, Andy Shevchenko, platform-driver-x86, linux-kernel

> > @@ -272,9 +292,9 @@ static int bl_get_brightness(struct backlight_device *b)
> >  static int bl_update_status(struct backlight_device *b)
> >  {
> >  	if (b->props.power == FB_BLANK_POWERDOWN)
> > -		call_fext_func(FUNC_BACKLIGHT, 0x1, 0x4, 0x3);
> > +		fext_backlight(0x1, 0x4, 0x3);
> >  	else
> > -		call_fext_func(FUNC_BACKLIGHT, 0x1, 0x4, 0x0);
> > +		fext_backlight(0x1, 0x4, 0x0);
> >  
> >  	return set_lcd_level(b->props.brightness);
> >  }
> > @@ -610,22 +630,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 = fext_leds(0x1, LOGOLAMP_POWERON, poweron);
> >  	if (ret < 0)
> >  		return ret;
> >  
> > -	return call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_ALWAYS, always);
> > +	return fext_leds(0x1, LOGOLAMP_ALWAYS, always);
> >  }
> 
> I've only just noticed this.  For the led calls we have symbolic identifiers
> defined for the "features" parameter, but in the backlight case we are still
> using arbitrary numeric constants.  Although not necessary for this patch
> set, we should consider adding feature identifiers for the other fext_*() calls.
> Similarly for the "op" parameter where it makes sense to do so.

Good point, I will keep that in mind for the next patch series.

-- 
Best regards,
Michał Kępień

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals
  2017-05-02 13:21   ` Michał Kępień
@ 2017-05-04 23:40     ` Jonathan Woithe
  2017-05-05 16:15       ` Darren Hart
  0 siblings, 1 reply; 39+ messages in thread
From: Jonathan Woithe @ 2017-05-04 23:40 UTC (permalink / raw)
  To: Micha?? K??pie??
  Cc: Darren Hart, Andy Shevchenko, platform-driver-x86, linux-kernel

Hi Michael

On Tue, May 02, 2017 at 03:21:44PM +0200, Micha?? K??pie?? wrote:
> In order to avoid accessing global structures from call_fext_func(), we
> need to pass it an ACPI handle to FUJ02E3.  This decreases code
> readability in two ways: by increasing the function's parameter count
> from an already challenging four to an even worse five and by causing
> line breaks to be inserted (due to the 80-column line rule) in places
> they were previously not necessary in.
> 
> To counter this growing obfuscation, patches 01/10, 02/10 and 05/10 (all
> called out in your review) work in tandem to ensure that all uses of
> call_fext_func() remain legible _and_ fit in one line.  All three of
> these patches are needed to prevent line breaks from being inserted
> (granted, that is an arbitrary objective), because call_fext_func()
> needs to get the ACPI handle somehow and the latter is stored in a field
> of a device-specific structure. ...

Thanks for the explanation of your rationale behind patchs 1, 2 and 5.  In
short, they are (at the lowest level) cosmetic aimed at the adherence to the
80-column guideline, but for the reasons you outlined this is not
necessarily a bad thing.

> And thus we come back to the question of "to split or not to split".
> The three options we have are:
> 
>   - one module, two drivers: current, suboptimal, state of affairs,
> 
>   - two modules, one driver in each: the original cleanup approach I
>     have been targeting in all of my patch series for fujitsu-laptop,
> 
>   - one module, one driver handling both ACPI devices: the new approach
>     you suggested in your review.
> 
> I have not considered the last option until now as I deemed it
> unacceptable in light of the kernel's philosophy in this regard.
> However, such an approach might not be bad in and of itself, because:
> 
>   - FUJ02B1 is not fully standalone as it needs FUJ02E3 on some models,

This to me is a fairly strong indication that migrating to the "one module
one driver" approach is worthwhile considering.  If we do split we will end
up with two modules interacting with FUJ02E3, at least on some hardware. 
Conceptually it makes more sense to me that all interaction with FUJ02E3 is
instigated from one module/driver as it will make it easier to ensure that
minipulations of FUJ02E3 for one task don't have unintended side effects for
others.

>   - FUJ02E3 is present in all models we know of, while FUJ02B1 seems to
>     be phased out in newer models,

Agreed.  Furthermore, if FUJ02E3 is phased out it is reasonable to expect
that any platform driver required by the resulting hardware would be so
different to fujitsu-laptop that a new driver would be needed anyway.

>   - userspace is unlikely to care which input device each hotkey event
>     comes from,

Agreed.

>   - the memory footprint of both drivers is negligible, considering that
>     both are only loaded on machines with hundreds of MB of RAM.

Agreed.

> So we could perhaps make fujitsu-laptop register _one_ ACPI driver,
> which binds to the FUJ02E3 device and only deals with backlight when the
> FUJ02B1 device is present and the vendor interface is either
> automatically selected by the kernel or explicitly requested by the
> user.  We would then have a single device-specific structure ("priv"
> would not be ambiguous any more) holding two ACPI handles ("fjex_handle"
> and "fext_handle"?) and all the other fields from both struct fujitsu_bl
> and struct fujitsu_laptop.

Yes, these are the kinds of benefits I was thinking about.

> Please note that I have not played with this idea in code yet and perhaps
> handling the added complexity will make the driver more, not less,
> convoluted.

I understand.  Since FUJ02B1 is only relevant to the backlight I can't see
how the above approach would result in a signficant increase in complexity,
but like you I haven't had a close look at the implications.

> Darren, does the above sound more like a viable plan or rather a pipe
> dream?  Answering Jonathan's question, there is no added benefit from
> splitting fujitsu-laptop into two separate modules, it is only about
> following the "one module, one driver" philosophy.  Any answer to this
> question puts the variable naming discussion on a specific track, so
> perhaps this is the first dilemma that we should sort out.

I agree.  We should resolve the question of the split/no-split option first
since the answer does influence many of the other pending questions. 

Darren: I would therefore be interested in your take on the three options
(as summarised by Michael) so we can determine a way forward.

Regards
  jonathan

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals
  2017-05-04 23:40     ` Jonathan Woithe
@ 2017-05-05 16:15       ` Darren Hart
  2017-05-06 12:31         ` Michał Kępień
  0 siblings, 1 reply; 39+ messages in thread
From: Darren Hart @ 2017-05-05 16:15 UTC (permalink / raw)
  To: Jonathan Woithe, Rafael Wysocki
  Cc: Micha?? K??pie??, Andy Shevchenko, platform-driver-x86, linux-kernel

On Fri, May 05, 2017 at 09:10:58AM +0930, Jonathan Woithe wrote:
> Hi Michael
> 
> On Tue, May 02, 2017 at 03:21:44PM +0200, Micha?? K??pie?? wrote:
> > In order to avoid accessing global structures from call_fext_func(), we
> > need to pass it an ACPI handle to FUJ02E3.  This decreases code
> > readability in two ways: by increasing the function's parameter count
> > from an already challenging four to an even worse five and by causing
> > line breaks to be inserted (due to the 80-column line rule) in places
> > they were previously not necessary in.
> > 
> > To counter this growing obfuscation, patches 01/10, 02/10 and 05/10 (all
> > called out in your review) work in tandem to ensure that all uses of
> > call_fext_func() remain legible _and_ fit in one line.  All three of
> > these patches are needed to prevent line breaks from being inserted
> > (granted, that is an arbitrary objective), because call_fext_func()
> > needs to get the ACPI handle somehow and the latter is stored in a field
> > of a device-specific structure. ...
> 
> Thanks for the explanation of your rationale behind patchs 1, 2 and 5.  In
> short, they are (at the lowest level) cosmetic aimed at the adherence to the
> 80-column guideline, but for the reasons you outlined this is not
> necessarily a bad thing.
> 
> > And thus we come back to the question of "to split or not to split".
> > The three options we have are:
> > 
> >   - one module, two drivers: current, suboptimal, state of affairs,
> > 
> >   - two modules, one driver in each: the original cleanup approach I
> >     have been targeting in all of my patch series for fujitsu-laptop,
> > 
> >   - one module, one driver handling both ACPI devices: the new approach
> >     you suggested in your review.
> > 
> > I have not considered the last option until now as I deemed it
> > unacceptable in light of the kernel's philosophy in this regard.
> > However, such an approach might not be bad in and of itself, because:
> > 
> >   - FUJ02B1 is not fully standalone as it needs FUJ02E3 on some models,
> 
> This to me is a fairly strong indication that migrating to the "one module
> one driver" approach is worthwhile considering.  If we do split we will end
> up with two modules interacting with FUJ02E3, at least on some hardware. 
> Conceptually it makes more sense to me that all interaction with FUJ02E3 is
> instigated from one module/driver as it will make it easier to ensure that
> minipulations of FUJ02E3 for one task don't have unintended side effects for
> others.
> 
> >   - FUJ02E3 is present in all models we know of, while FUJ02B1 seems to
> >     be phased out in newer models,
> 
> Agreed.  Furthermore, if FUJ02E3 is phased out it is reasonable to expect
> that any platform driver required by the resulting hardware would be so
> different to fujitsu-laptop that a new driver would be needed anyway.

*cough* thinkpad_acpi *cough*

> 
> >   - userspace is unlikely to care which input device each hotkey event
> >     comes from,
> 
> Agreed.
> 
> >   - the memory footprint of both drivers is negligible, considering that
> >     both are only loaded on machines with hundreds of MB of RAM.
> 
> Agreed.
> 
> > So we could perhaps make fujitsu-laptop register _one_ ACPI driver,
> > which binds to the FUJ02E3 device and only deals with backlight when the
> > FUJ02B1 device is present and the vendor interface is either
> > automatically selected by the kernel or explicitly requested by the
> > user.  We would then have a single device-specific structure ("priv"
> > would not be ambiguous any more) holding two ACPI handles ("fjex_handle"
> > and "fext_handle"?) and all the other fields from both struct fujitsu_bl
> > and struct fujitsu_laptop.
> 
> Yes, these are the kinds of benefits I was thinking about.
> 
> > Please note that I have not played with this idea in code yet and perhaps
> > handling the added complexity will make the driver more, not less,
> > convoluted.
> 
> I understand.  Since FUJ02B1 is only relevant to the backlight I can't see
> how the above approach would result in a signficant increase in complexity,
> but like you I haven't had a close look at the implications.
> 
> > Darren, does the above sound more like a viable plan or rather a pipe
> > dream?  Answering Jonathan's question, there is no added benefit from
> > splitting fujitsu-laptop into two separate modules, it is only about
> > following the "one module, one driver" philosophy.  Any answer to this
> > question puts the variable naming discussion on a specific track, so
> > perhaps this is the first dilemma that we should sort out.
> 
> I agree.  We should resolve the question of the split/no-split option first
> since the answer does influence many of the other pending questions. 
> 
> Darren: I would therefore be interested in your take on the three options
> (as summarised by Michael) so we can determine a way forward.

+Rafael for his insight from an ACPI driver model perspective.

Unfortunately, this is a fairly subjective area of driver design. We have
competing goals:

a) Driver coupling
   Module load order dependencies and such is to be avoided whenever possible.
   Drivers should be as independent as possible from one another.

b) Single function
   Can't think of a better name for this right now. But this is Michal's point
   about one driver per device. As we add more devices, we risk growing the
   driver until it carries a lot of legacy baggage and is more and more
   difficult to maintain. thinkpad_acpi is the prime example of this.

In an ideal world, Single Function drivers are preferred, but if we end up have
to perform unnatural acts to keep the drivers separated, the advantages can be
lost. So it all hinges on how much Driver Coupling would exist in the separate
driver approach.

We'll accept either with supporting evidence for why it's the better choice. My
preference, under ideal conditions, would be for separate drivers, separate
modules, one per device.

-- 
Darren Hart
VMware Open Source Technology Center

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals
  2017-05-05 16:15       ` Darren Hart
@ 2017-05-06 12:31         ` Michał Kępień
  2017-05-06 12:45           ` Michał Kępień
  0 siblings, 1 reply; 39+ messages in thread
From: Michał Kępień @ 2017-05-06 12:31 UTC (permalink / raw)
  To: Darren Hart
  Cc: Jonathan Woithe, Rafael Wysocki, Andy Shevchenko,
	platform-driver-x86, linux-kernel

> On Fri, May 05, 2017 at 09:10:58AM +0930, Jonathan Woithe wrote:
> > Hi Michael
> > 
> > On Tue, May 02, 2017 at 03:21:44PM +0200, Micha?? K??pie?? wrote:
> > > In order to avoid accessing global structures from call_fext_func(), we
> > > need to pass it an ACPI handle to FUJ02E3.  This decreases code
> > > readability in two ways: by increasing the function's parameter count
> > > from an already challenging four to an even worse five and by causing
> > > line breaks to be inserted (due to the 80-column line rule) in places
> > > they were previously not necessary in.
> > > 
> > > To counter this growing obfuscation, patches 01/10, 02/10 and 05/10 (all
> > > called out in your review) work in tandem to ensure that all uses of
> > > call_fext_func() remain legible _and_ fit in one line.  All three of
> > > these patches are needed to prevent line breaks from being inserted
> > > (granted, that is an arbitrary objective), because call_fext_func()
> > > needs to get the ACPI handle somehow and the latter is stored in a field
> > > of a device-specific structure. ...
> > 
> > Thanks for the explanation of your rationale behind patchs 1, 2 and 5.  In
> > short, they are (at the lowest level) cosmetic aimed at the adherence to the
> > 80-column guideline, but for the reasons you outlined this is not
> > necessarily a bad thing.
> > 
> > > And thus we come back to the question of "to split or not to split".
> > > The three options we have are:
> > > 
> > >   - one module, two drivers: current, suboptimal, state of affairs,
> > > 
> > >   - two modules, one driver in each: the original cleanup approach I
> > >     have been targeting in all of my patch series for fujitsu-laptop,
> > > 
> > >   - one module, one driver handling both ACPI devices: the new approach
> > >     you suggested in your review.
> > > 
> > > I have not considered the last option until now as I deemed it
> > > unacceptable in light of the kernel's philosophy in this regard.
> > > However, such an approach might not be bad in and of itself, because:
> > > 
> > >   - FUJ02B1 is not fully standalone as it needs FUJ02E3 on some models,
> > 
> > This to me is a fairly strong indication that migrating to the "one module
> > one driver" approach is worthwhile considering.  If we do split we will end
> > up with two modules interacting with FUJ02E3, at least on some hardware. 
> > Conceptually it makes more sense to me that all interaction with FUJ02E3 is
> > instigated from one module/driver as it will make it easier to ensure that
> > minipulations of FUJ02E3 for one task don't have unintended side effects for
> > others.
> > 
> > >   - FUJ02E3 is present in all models we know of, while FUJ02B1 seems to
> > >     be phased out in newer models,
> > 
> > Agreed.  Furthermore, if FUJ02E3 is phased out it is reasonable to expect
> > that any platform driver required by the resulting hardware would be so
> > different to fujitsu-laptop that a new driver would be needed anyway.
> 
> *cough* thinkpad_acpi *cough*
> 
> > 
> > >   - userspace is unlikely to care which input device each hotkey event
> > >     comes from,
> > 
> > Agreed.
> > 
> > >   - the memory footprint of both drivers is negligible, considering that
> > >     both are only loaded on machines with hundreds of MB of RAM.
> > 
> > Agreed.
> > 
> > > So we could perhaps make fujitsu-laptop register _one_ ACPI driver,
> > > which binds to the FUJ02E3 device and only deals with backlight when the
> > > FUJ02B1 device is present and the vendor interface is either
> > > automatically selected by the kernel or explicitly requested by the
> > > user.  We would then have a single device-specific structure ("priv"
> > > would not be ambiguous any more) holding two ACPI handles ("fjex_handle"
> > > and "fext_handle"?) and all the other fields from both struct fujitsu_bl
> > > and struct fujitsu_laptop.
> > 
> > Yes, these are the kinds of benefits I was thinking about.
> > 
> > > Please note that I have not played with this idea in code yet and perhaps
> > > handling the added complexity will make the driver more, not less,
> > > convoluted.
> > 
> > I understand.  Since FUJ02B1 is only relevant to the backlight I can't see
> > how the above approach would result in a signficant increase in complexity,
> > but like you I haven't had a close look at the implications.
> > 
> > > Darren, does the above sound more like a viable plan or rather a pipe
> > > dream?  Answering Jonathan's question, there is no added benefit from
> > > splitting fujitsu-laptop into two separate modules, it is only about
> > > following the "one module, one driver" philosophy.  Any answer to this
> > > question puts the variable naming discussion on a specific track, so
> > > perhaps this is the first dilemma that we should sort out.
> > 
> > I agree.  We should resolve the question of the split/no-split option first
> > since the answer does influence many of the other pending questions. 
> > 
> > Darren: I would therefore be interested in your take on the three options
> > (as summarised by Michael) so we can determine a way forward.
> 
> +Rafael for his insight from an ACPI driver model perspective.
> 
> Unfortunately, this is a fairly subjective area of driver design. We have
> competing goals:
> 
> a) Driver coupling
>    Module load order dependencies and such is to be avoided whenever possible.
>    Drivers should be as independent as possible from one another.
> 
> b) Single function
>    Can't think of a better name for this right now. But this is Michal's point
>    about one driver per device. As we add more devices, we risk growing the
>    driver until it carries a lot of legacy baggage and is more and more
>    difficult to maintain. thinkpad_acpi is the prime example of this.
> 
> In an ideal world, Single Function drivers are preferred, but if we end up have
> to perform unnatural acts to keep the drivers separated, the advantages can be
> lost. So it all hinges on how much Driver Coupling would exist in the separate
> driver approach.

Just to make sure we are all on the same page here, choosing the "two
separate modules, each with one driver for one ACPI device" approach
would mean ending up with two modules:

  - fujitsu-laptop, binding to the FUJ02E3 ACPI device, handling
    everything _except_ backlight,

  - fujitsu-backlight, binding to the FUJ02B1 ACPI device, handling
    backlight and depending on fujitsu-laptop.

We would need to export one function from fujitsu-laptop, namely
fext_backlight().  I understand this would require creating a separate
header file which would then be included in fujitsu-backlight.

fext_backlight() causes the FUNC method of the FUJ02E3 ACPI device to be
called.  This method is marked as Serialized, which AFAIU means we do
not need a separate lock in kernel code because all calls to this method
are implicitly serialized by firmware itself.

I do not see anything "unnatural" in this approach, but I would love to
be corrected if I am wrong.

> We'll accept either with supporting evidence for why it's the better choice. My
> preference, under ideal conditions, would be for separate drivers, separate
> modules, one per device.

Putting my two cents in, that would be my choice, too.  Among other
issues, if we choose the "one module, one driver handling two ACPI
devices" approach, only FUJ02E3 will be bound to a driver from the
kernel's perspective, while FUJ02B1 will not, even though it actually
_will_ be handled by the same driver as FUJ02E3.  Sounds ugly to me, but
I would also really like to hear Rafael's opinion.

-- 
Best regards,
Michał Kępień

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals
  2017-05-06 12:31         ` Michał Kępień
@ 2017-05-06 12:45           ` Michał Kępień
  2017-05-06 14:21             ` Andy Shevchenko
  2017-05-08 16:01             ` Darren Hart
  0 siblings, 2 replies; 39+ messages in thread
From: Michał Kępień @ 2017-05-06 12:45 UTC (permalink / raw)
  To: Darren Hart
  Cc: Jonathan Woithe, Rafael Wysocki, Andy Shevchenko,
	platform-driver-x86, linux-kernel

> Just to make sure we are all on the same page here, choosing the "two
> separate modules, each with one driver for one ACPI device" approach
> would mean ending up with two modules:
> 
>   - fujitsu-laptop, binding to the FUJ02E3 ACPI device, handling
>     everything _except_ backlight,
> 
>   - fujitsu-backlight, binding to the FUJ02B1 ACPI device, handling
>     backlight and depending on fujitsu-laptop.
> 
> We would need to export one function from fujitsu-laptop, namely
> fext_backlight().  I understand this would require creating a separate
> header file which would then be included in fujitsu-backlight.
> 
> fext_backlight() causes the FUNC method of the FUJ02E3 ACPI device to be
> called.  This method is marked as Serialized, which AFAIU means we do
> not need a separate lock in kernel code because all calls to this method
> are implicitly serialized by firmware itself.
> 
> I do not see anything "unnatural" in this approach, but I would love to
> be corrected if I am wrong.

To be fair, one thing that may be "unnatural" with this approach is that
even though fujitsu-backlight would depend on fujitsu-laptop, it would
still have to get a handle to FUJ02E3 using:

    acpi_get_handle(NULL, "\\_SB.FEXT", ...)
    
because call_fext_func() - and thus fext_backlight() - needs to be
passed a handle to FUJ02E3 and the two ACPI devices (FUJ02B1 handled by
fujitsu-backlight and FUJ02E3 handled by fujitsu-laptop) are not related
from the perspective of the ACPI device hierarchy.  Unless there is a
better way of implementing this, in which case I am open to suggestions.

-- 
Best regards,
Michał Kępień

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals
  2017-05-06 12:45           ` Michał Kępień
@ 2017-05-06 14:21             ` Andy Shevchenko
  2017-05-06 14:23               ` Andy Shevchenko
  2017-05-08 16:01             ` Darren Hart
  1 sibling, 1 reply; 39+ messages in thread
From: Andy Shevchenko @ 2017-05-06 14:21 UTC (permalink / raw)
  To: Michał Kępień
  Cc: Darren Hart, Jonathan Woithe, Rafael Wysocki, Andy Shevchenko,
	Platform Driver, linux-kernel

On Sat, May 6, 2017 at 3:45 PM, Michał Kępień <kernel@kempniu.pl> wrote:

> To be fair, one thing that may be "unnatural" with this approach is that
> even though fujitsu-backlight would depend on fujitsu-laptop, it would
> still have to get a handle to FUJ02E3 using:
>
>     acpi_get_handle(NULL, "\\_SB.FEXT", ...)
>
> because call_fext_func() - and thus fext_backlight() - needs to be
> passed a handle to FUJ02E3 and the two ACPI devices (FUJ02B1 handled by
> fujitsu-backlight and FUJ02E3 handled by fujitsu-laptop) are not related
> from the perspective of the ACPI device hierarchy.  Unless there is a
> better way of implementing this, in which case I am open to suggestions.

There are two areas to check with:
1. Remote graph node
https://lwn.net/Articles/718184/
2. Component framework (only works in case when all devices are
mandatory to have).

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals
  2017-05-06 14:21             ` Andy Shevchenko
@ 2017-05-06 14:23               ` Andy Shevchenko
  0 siblings, 0 replies; 39+ messages in thread
From: Andy Shevchenko @ 2017-05-06 14:23 UTC (permalink / raw)
  To: Michał Kępień
  Cc: Darren Hart, Jonathan Woithe, Rafael Wysocki, Andy Shevchenko,
	Platform Driver, linux-kernel

On Sat, May 6, 2017 at 5:21 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Sat, May 6, 2017 at 3:45 PM, Michał Kępień <kernel@kempniu.pl> wrote:
>
>> To be fair, one thing that may be "unnatural" with this approach is that
>> even though fujitsu-backlight would depend on fujitsu-laptop, it would
>> still have to get a handle to FUJ02E3 using:
>>
>>     acpi_get_handle(NULL, "\\_SB.FEXT", ...)
>>
>> because call_fext_func() - and thus fext_backlight() - needs to be
>> passed a handle to FUJ02E3 and the two ACPI devices (FUJ02B1 handled by
>> fujitsu-backlight and FUJ02E3 handled by fujitsu-laptop) are not related
>> from the perspective of the ACPI device hierarchy.  Unless there is a
>> better way of implementing this, in which case I am open to suggestions.
>
> There are two areas to check with:
> 1. Remote graph node
> https://lwn.net/Articles/718184/
> 2. Component framework (only works in case when all devices are
> mandatory to have).

Ah, and third one is MFD framework.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals
  2017-05-06 12:45           ` Michał Kępień
  2017-05-06 14:21             ` Andy Shevchenko
@ 2017-05-08 16:01             ` Darren Hart
  2017-05-09  9:35               ` Michał Kępień
  1 sibling, 1 reply; 39+ messages in thread
From: Darren Hart @ 2017-05-08 16:01 UTC (permalink / raw)
  To: Michał Kępień
  Cc: Jonathan Woithe, Rafael Wysocki, Andy Shevchenko,
	platform-driver-x86, linux-kernel

On Sat, May 06, 2017 at 02:45:16PM +0200, Michał Kępień wrote:
> > Just to make sure we are all on the same page here, choosing the "two
> > separate modules, each with one driver for one ACPI device" approach
> > would mean ending up with two modules:
> > 
> >   - fujitsu-laptop, binding to the FUJ02E3 ACPI device, handling
> >     everything _except_ backlight,
> > 
> >   - fujitsu-backlight, binding to the FUJ02B1 ACPI device, handling
> >     backlight and depending on fujitsu-laptop.
> > 
> > We would need to export one function from fujitsu-laptop, namely
> > fext_backlight().  I understand this would require creating a separate
> > header file which would then be included in fujitsu-backlight.
> > 
> > fext_backlight() causes the FUNC method of the FUJ02E3 ACPI device to be
> > called.  This method is marked as Serialized, which AFAIU means we do
> > not need a separate lock in kernel code because all calls to this method
> > are implicitly serialized by firmware itself.
> > 
> > I do not see anything "unnatural" in this approach, but I would love to
> > be corrected if I am wrong.
> 
> To be fair, one thing that may be "unnatural" with this approach is that
> even though fujitsu-backlight would depend on fujitsu-laptop, it would
> still have to get a handle to FUJ02E3 using:
> 
>     acpi_get_handle(NULL, "\\_SB.FEXT", ...)
>     
> because call_fext_func() - and thus fext_backlight() - needs to be
> passed a handle to FUJ02E3 and the two ACPI devices (FUJ02B1 handled by
> fujitsu-backlight and FUJ02E3 handled by fujitsu-laptop) are not related
> from the perspective of the ACPI device hierarchy.  Unless there is a
> better way of implementing this, in which case I am open to suggestions.

At a high level, I would consider the handle to be private data which should be
encapsulated in fujitsu_laptop. Or... where is FEXT in the ACPI hierarchy
relative to FUJ02E3?

Assuming FEXT is below FUJ02E3, the we appear to be making an assumption that
there is only one FUJ02E3 on the system. While I think this is perfectly
reasonable, it does contradict the argumentation from some of the other patches
in this series.

If FEXT is not below fujitsu laptop... then it is a shared function which either
one of them can own and serialize (or not if fw indeed handles that).

Either way, the owning driver should abstract away the private data and present
an interface the other can use with only the "public" information.

I suggest investigating the various mechanisms Andy pointed at and revisiting
this after that.

-- 
Darren Hart
VMware Open Source Technology Center

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals
  2017-05-08 16:01             ` Darren Hart
@ 2017-05-09  9:35               ` Michał Kępień
  2017-05-09 12:13                 ` Jonathan Woithe
  2017-05-09 16:47                 ` Darren Hart
  0 siblings, 2 replies; 39+ messages in thread
From: Michał Kępień @ 2017-05-09  9:35 UTC (permalink / raw)
  To: Darren Hart
  Cc: Jonathan Woithe, Rafael Wysocki, Andy Shevchenko,
	platform-driver-x86, linux-kernel

> On Sat, May 06, 2017 at 02:45:16PM +0200, Michał Kępień wrote:
> > > Just to make sure we are all on the same page here, choosing the "two
> > > separate modules, each with one driver for one ACPI device" approach
> > > would mean ending up with two modules:
> > > 
> > >   - fujitsu-laptop, binding to the FUJ02E3 ACPI device, handling
> > >     everything _except_ backlight,
> > > 
> > >   - fujitsu-backlight, binding to the FUJ02B1 ACPI device, handling
> > >     backlight and depending on fujitsu-laptop.
> > > 
> > > We would need to export one function from fujitsu-laptop, namely
> > > fext_backlight().  I understand this would require creating a separate
> > > header file which would then be included in fujitsu-backlight.
> > > 
> > > fext_backlight() causes the FUNC method of the FUJ02E3 ACPI device to be
> > > called.  This method is marked as Serialized, which AFAIU means we do
> > > not need a separate lock in kernel code because all calls to this method
> > > are implicitly serialized by firmware itself.
> > > 
> > > I do not see anything "unnatural" in this approach, but I would love to
> > > be corrected if I am wrong.
> > 
> > To be fair, one thing that may be "unnatural" with this approach is that
> > even though fujitsu-backlight would depend on fujitsu-laptop, it would
> > still have to get a handle to FUJ02E3 using:
> > 
> >     acpi_get_handle(NULL, "\\_SB.FEXT", ...)
> >     
> > because call_fext_func() - and thus fext_backlight() - needs to be
> > passed a handle to FUJ02E3 and the two ACPI devices (FUJ02B1 handled by
> > fujitsu-backlight and FUJ02E3 handled by fujitsu-laptop) are not related
> > from the perspective of the ACPI device hierarchy.  Unless there is a
> > better way of implementing this, in which case I am open to suggestions.
> 
> At a high level, I would consider the handle to be private data which should be
> encapsulated in fujitsu_laptop. Or... where is FEXT in the ACPI hierarchy
> relative to FUJ02E3?

FEXT *is* FUJ02E3:

Device (FEXT)
{
    Name (_HID, "FUJ02E3")  // _HID: Hardware ID
    ...
    Method (FUNC, 4, Serialized)
    {
        ...
    }
    ...
}

See also below.

> Assuming FEXT is below FUJ02E3, the we appear to be making an assumption that
> there is only one FUJ02E3 on the system. While I think this is perfectly
> reasonable, it does contradict the argumentation from some of the other patches
> in this series.

Exactly.  The whole purpose of this patch series is to stop using
module-wide data.  We have a different situation here than in the case
of e.g. dell-smbios, which coordinates access to a module-wide buffer it
allocates.  

> If FEXT is not below fujitsu laptop... then it is a shared function which either
> one of them can own and serialize (or not if fw indeed handles that).
> 
> Either way, the owning driver should abstract away the private data and present
> an interface the other can use with only the "public" information.

I feel the problem at hand needs a fresh explanation.  I will be as
concise as possible.

We are considering two ACPI devices present on Fujitsu laptops:

  - FJEX:
      * path: \_SB_.PCI0.LPCB.FJEX
      * HID: FUJ02B1
      * methods invoked by kernel: GBLL, RBLL, SBLL, SBL2
      * handles: backlight level (LCD brightness)

  - FEXT:
      * path: \_SB_.FEXT
      * HID: FUJ02E3
      * methods invoked by kernel: FUNC
      * handles: hotkey, LEDs, platform attributes, backlight power
                                                    ^^^^^^^^^^^^^^^

The problem is that if we split the ACPI drivers for those two devices
into separate modules, the FJEX driver will need to access the FUNC
method of device FEXT, handled by another driver in another module.

One way of solving this cleanly is to store a handle to the most
recently found FEXT instance (there should always be at most one anyway)
in a module-wide variable inside the FEXT driver, but that defeats the
purpose of this series.

Another solution is proposed by patch 04/10 of this series: make the
FJEX driver independently grab a handle to FEXT using the absolute ACPI
path to the latter.  It feels unnatural (AFAICT only one driver outside
drivers/acpi, namely pcc-cpufreq, does that), but it is safe and allows
us to drop all module-wide data.

Finally, perhaps the approach I took in my patch series is simply too
zealous.  Maybe the simplest solution is to just keep using module-wide
data, but then we are left with a single module with two intertwined
ACPI drivers inside that need to be registered in the correct order.  It
feels a bit brittle.

> I suggest investigating the various mechanisms Andy pointed at and revisiting
> this after that.

For now, these yield no immediate silver bullets for me.  But I will dig
a bit deeper and report back.  Meanwhile, if anyone feels like sharing
their thoughts after reading the summary I wrote above, I am all ears.

-- 
Best regards,
Michał Kępień

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals
  2017-05-09  9:35               ` Michał Kępień
@ 2017-05-09 12:13                 ` Jonathan Woithe
  2017-05-09 16:47                 ` Darren Hart
  1 sibling, 0 replies; 39+ messages in thread
From: Jonathan Woithe @ 2017-05-09 12:13 UTC (permalink / raw)
  To: Micha?? K??pie??
  Cc: Darren Hart, Rafael Wysocki, Andy Shevchenko,
	platform-driver-x86, linux-kernel

On Tue, May 09, 2017 at 11:35:24AM +0200, Micha?? K??pie?? wrote:
> > If FEXT is not below fujitsu laptop... then it is a shared function which either
> > one of them can own and serialize (or not if fw indeed handles that).
> > 
> > Either way, the owning driver should abstract away the private data and present
> > an interface the other can use with only the "public" information.
> 
> I feel the problem at hand needs a fresh explanation.  I will be as
> concise as possible.
> 
> We are considering two ACPI devices present on Fujitsu laptops:
> 
>   - FJEX:
>       * path: \_SB_.PCI0.LPCB.FJEX
>       * HID: FUJ02B1
>       * methods invoked by kernel: GBLL, RBLL, SBLL, SBL2
>       * handles: backlight level (LCD brightness)
> 
>   - FEXT:
>       * path: \_SB_.FEXT
>       * HID: FUJ02E3
>       * methods invoked by kernel: FUNC
>       * handles: hotkey, LEDs, platform attributes, backlight power
>                                                     ^^^^^^^^^^^^^^^
> 
> The problem is that if we split the ACPI drivers for those two devices
> into separate modules, the FJEX driver will need to access the FUNC
> method of device FEXT, handled by another driver in another module.
> 
> One way of solving this cleanly is to store a handle to the most
> recently found FEXT instance (there should always be at most one anyway)
> in a module-wide variable inside the FEXT driver, but that defeats the
> purpose of this series.
> 
> Another solution is proposed by patch 04/10 of this series: make the
> FJEX driver independently grab a handle to FEXT using the absolute ACPI
> path to the latter.  It feels unnatural (AFAICT only one driver outside
> drivers/acpi, namely pcc-cpufreq, does that), but it is safe and allows
> us to drop all module-wide data.
> 
> Finally, perhaps the approach I took in my patch series is simply too
> zealous.  Maybe the simplest solution is to just keep using module-wide
> data, but then we are left with a single module with two intertwined
> ACPI drivers inside that need to be registered in the correct order.  It
> feels a bit brittle.

I think this revised summary is a good description of the situation.

> > I suggest investigating the various mechanisms Andy pointed at and revisiting
> > this after that.
> 
> For now, these yield no immediate silver bullets for me.  But I will dig
> a bit deeper and report back.  Meanwhile, if anyone feels like sharing
> their thoughts after reading the summary I wrote above, I am all ears.

I appreciate you taking the time to continue pondering the situation - the
best approach is certainly not immediately obvious.  From where I sit I
suspect that whichever solution we eventually adopt there are going to be
wrinkles.  The trick is to identify the approach which has the fewest rough
edges and where the resulting code is the easiest to follow in future.  At
this point in time I cannot come up with an argument which definitively
supports one particular option over the others.  It may come down to
personal taste.

Regards
  jonathan

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals
  2017-05-09  9:35               ` Michał Kępień
  2017-05-09 12:13                 ` Jonathan Woithe
@ 2017-05-09 16:47                 ` Darren Hart
  2017-05-09 21:24                   ` Rafael J. Wysocki
  2017-05-11 13:40                   ` Michał Kępień
  1 sibling, 2 replies; 39+ messages in thread
From: Darren Hart @ 2017-05-09 16:47 UTC (permalink / raw)
  To: Michał Kępień
  Cc: Jonathan Woithe, Rafael Wysocki, Andy Shevchenko,
	platform-driver-x86, linux-kernel

On Tue, May 09, 2017 at 11:35:24AM +0200, Michał Kępień wrote:
> > On Sat, May 06, 2017 at 02:45:16PM +0200, Michał Kępień wrote:
> > > > Just to make sure we are all on the same page here, choosing the "two
> > > > separate modules, each with one driver for one ACPI device" approach
> > > > would mean ending up with two modules:
> > > > 
> > > >   - fujitsu-laptop, binding to the FUJ02E3 ACPI device, handling
> > > >     everything _except_ backlight,
> > > > 
> > > >   - fujitsu-backlight, binding to the FUJ02B1 ACPI device, handling
> > > >     backlight and depending on fujitsu-laptop.
> > > > 
> > > > We would need to export one function from fujitsu-laptop, namely
> > > > fext_backlight().  I understand this would require creating a separate
> > > > header file which would then be included in fujitsu-backlight.
> > > > 
> > > > fext_backlight() causes the FUNC method of the FUJ02E3 ACPI device to be
> > > > called.  This method is marked as Serialized, which AFAIU means we do
> > > > not need a separate lock in kernel code because all calls to this method
> > > > are implicitly serialized by firmware itself.
> > > > 
> > > > I do not see anything "unnatural" in this approach, but I would love to
> > > > be corrected if I am wrong.
> > > 
> > > To be fair, one thing that may be "unnatural" with this approach is that
> > > even though fujitsu-backlight would depend on fujitsu-laptop, it would
> > > still have to get a handle to FUJ02E3 using:
> > > 
> > >     acpi_get_handle(NULL, "\\_SB.FEXT", ...)
> > >     
> > > because call_fext_func() - and thus fext_backlight() - needs to be
> > > passed a handle to FUJ02E3 and the two ACPI devices (FUJ02B1 handled by
> > > fujitsu-backlight and FUJ02E3 handled by fujitsu-laptop) are not related
> > > from the perspective of the ACPI device hierarchy.  Unless there is a
> > > better way of implementing this, in which case I am open to suggestions.
> > 
> > At a high level, I would consider the handle to be private data which should be
> > encapsulated in fujitsu_laptop. Or... where is FEXT in the ACPI hierarchy
> > relative to FUJ02E3?
> 
> FEXT *is* FUJ02E3:
> 
> Device (FEXT)
> {
>     Name (_HID, "FUJ02E3")  // _HID: Hardware ID
>     ...
>     Method (FUNC, 4, Serialized)
>     {
>         ...
>     }
>     ...
> }
> 
> See also below.
> 
> > Assuming FEXT is below FUJ02E3, the we appear to be making an assumption that
> > there is only one FUJ02E3 on the system. While I think this is perfectly
> > reasonable, it does contradict the argumentation from some of the other patches
> > in this series.
> 
> Exactly.  The whole purpose of this patch series is to stop using
> module-wide data.  We have a different situation here than in the case
> of e.g. dell-smbios, which coordinates access to a module-wide buffer it
> allocates.  
> 
> > If FEXT is not below fujitsu laptop... then it is a shared function which either
> > one of them can own and serialize (or not if fw indeed handles that).
> > 
> > Either way, the owning driver should abstract away the private data and present
> > an interface the other can use with only the "public" information.
> 
> I feel the problem at hand needs a fresh explanation.  I will be as
> concise as possible.
> 
> We are considering two ACPI devices present on Fujitsu laptops:
> 
>   - FJEX:
>       * path: \_SB_.PCI0.LPCB.FJEX
>       * HID: FUJ02B1
>       * methods invoked by kernel: GBLL, RBLL, SBLL, SBL2
>       * handles: backlight level (LCD brightness)
> 
>   - FEXT:
>       * path: \_SB_.FEXT
>       * HID: FUJ02E3
>       * methods invoked by kernel: FUNC
>       * handles: hotkey, LEDs, platform attributes, backlight power
>                                                     ^^^^^^^^^^^^^^^

This is very concise and describes the problem clearly, thank you!

> 
> The problem is that if we split the ACPI drivers for those two devices
> into separate modules, the FJEX driver will need to access the FUNC
> method of device FEXT, handled by another driver in another module.
> 
> One way of solving this cleanly is to store a handle to the most
> recently found FEXT instance (there should always be at most one anyway)
> in a module-wide variable inside the FEXT driver, but that defeats the
> purpose of this series.
> 
> Another solution is proposed by patch 04/10 of this series: make the
> FJEX driver independently grab a handle to FEXT using the absolute ACPI
> path to the latter.  It feels unnatural (AFAICT only one driver outside
> drivers/acpi, namely pcc-cpufreq, does that), but it is safe and allows
> us to drop all module-wide data.

Rafael's take on this would be useful.

> 
> Finally, perhaps the approach I took in my patch series is simply too
> zealous.  Maybe the simplest solution is to just keep using module-wide
> data, but then we are left with a single module with two intertwined
> ACPI drivers inside that need to be registered in the correct order.  It
> feels a bit brittle.

Perhaps so (overly zealous). Regarding the globals, let's be clear on the
motivation. We want to follow good sw engineering practice, use data
encapsulation, etc. However, using an explicit path to an ACPI device to avoid
having a static file-level global doesn't really improve encapsulation in any
way - it just shifts the blame :-)

Another reason to eliminate globals is to allow one driver to handle multiple
devices - all device-specific data must be bound to the device, not the driver.
In our case, there literally cannot be more than one _SB.FEXT. While there could
theoretically be more than one FUJ02E3, I think we all agree that is highly
improbable - and if it did happen, the explicit ACPI path approach would also be
broken.

The motivation to divide the drivers was to provide functional encapsulation,
accurately represent the system in the device tree, and to improve readability
and maintainability of the driver code. So long as we can keep coupling to a
minimum, I still think this makes sense.

So - static global variable for a driver with exactly one device that needs
offer services to another driver... not really all that horrible.

You could accomplish this by making call_fext_func() not static and calling it
from fujitsu-backlight. Or, you could further restrict it by exporting a
fujitsu_backlight_power() function which wraps call_fext_func() providing a
specific interface for fujitsu-backlight. This makes the ownership very explicit
and ensures the usage doesn't grow without explicit changes to fujitsu-laptop.

That is probably the most practical solution IFF we still feel it is worth
splitting the driver into two separate modules. We need to develop a more robust
and objective decision making process on module granularity (when to split, when
to keep together). Will continue to give this more thought.

-- 
Darren Hart
VMware Open Source Technology Center

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals
  2017-05-09 16:47                 ` Darren Hart
@ 2017-05-09 21:24                   ` Rafael J. Wysocki
  2017-05-11 13:52                     ` Michał Kępień
  2017-05-11 13:40                   ` Michał Kępień
  1 sibling, 1 reply; 39+ messages in thread
From: Rafael J. Wysocki @ 2017-05-09 21:24 UTC (permalink / raw)
  To: Darren Hart, Michał Kępień
  Cc: Jonathan Woithe, Andy Shevchenko, platform-driver-x86, linux-kernel

On Tuesday, May 09, 2017 09:47:34 AM Darren Hart wrote:
> On Tue, May 09, 2017 at 11:35:24AM +0200, Michał Kępień wrote:
> > > On Sat, May 06, 2017 at 02:45:16PM +0200, Michał Kępień wrote:
> > > > > Just to make sure we are all on the same page here, choosing the "two
> > > > > separate modules, each with one driver for one ACPI device" approach
> > > > > would mean ending up with two modules:
> > > > > 
> > > > >   - fujitsu-laptop, binding to the FUJ02E3 ACPI device, handling
> > > > >     everything _except_ backlight,
> > > > > 
> > > > >   - fujitsu-backlight, binding to the FUJ02B1 ACPI device, handling
> > > > >     backlight and depending on fujitsu-laptop.
> > > > > 
> > > > > We would need to export one function from fujitsu-laptop, namely
> > > > > fext_backlight().  I understand this would require creating a separate
> > > > > header file which would then be included in fujitsu-backlight.
> > > > > 
> > > > > fext_backlight() causes the FUNC method of the FUJ02E3 ACPI device to be
> > > > > called.  This method is marked as Serialized, which AFAIU means we do
> > > > > not need a separate lock in kernel code because all calls to this method
> > > > > are implicitly serialized by firmware itself.
> > > > > 
> > > > > I do not see anything "unnatural" in this approach, but I would love to
> > > > > be corrected if I am wrong.
> > > > 
> > > > To be fair, one thing that may be "unnatural" with this approach is that
> > > > even though fujitsu-backlight would depend on fujitsu-laptop, it would
> > > > still have to get a handle to FUJ02E3 using:
> > > > 
> > > >     acpi_get_handle(NULL, "\\_SB.FEXT", ...)
> > > >     
> > > > because call_fext_func() - and thus fext_backlight() - needs to be
> > > > passed a handle to FUJ02E3 and the two ACPI devices (FUJ02B1 handled by
> > > > fujitsu-backlight and FUJ02E3 handled by fujitsu-laptop) are not related
> > > > from the perspective of the ACPI device hierarchy.  Unless there is a
> > > > better way of implementing this, in which case I am open to suggestions.
> > > 
> > > At a high level, I would consider the handle to be private data which should be
> > > encapsulated in fujitsu_laptop. Or... where is FEXT in the ACPI hierarchy
> > > relative to FUJ02E3?
> > 
> > FEXT *is* FUJ02E3:
> > 
> > Device (FEXT)
> > {
> >     Name (_HID, "FUJ02E3")  // _HID: Hardware ID
> >     ...
> >     Method (FUNC, 4, Serialized)
> >     {
> >         ...
> >     }
> >     ...
> > }
> > 
> > See also below.
> > 
> > > Assuming FEXT is below FUJ02E3, the we appear to be making an assumption that
> > > there is only one FUJ02E3 on the system. While I think this is perfectly
> > > reasonable, it does contradict the argumentation from some of the other patches
> > > in this series.
> > 
> > Exactly.  The whole purpose of this patch series is to stop using
> > module-wide data.  We have a different situation here than in the case
> > of e.g. dell-smbios, which coordinates access to a module-wide buffer it
> > allocates.  
> > 
> > > If FEXT is not below fujitsu laptop... then it is a shared function which either
> > > one of them can own and serialize (or not if fw indeed handles that).
> > > 
> > > Either way, the owning driver should abstract away the private data and present
> > > an interface the other can use with only the "public" information.
> > 
> > I feel the problem at hand needs a fresh explanation.  I will be as
> > concise as possible.
> > 
> > We are considering two ACPI devices present on Fujitsu laptops:
> > 
> >   - FJEX:
> >       * path: \_SB_.PCI0.LPCB.FJEX
> >       * HID: FUJ02B1
> >       * methods invoked by kernel: GBLL, RBLL, SBLL, SBL2
> >       * handles: backlight level (LCD brightness)
> > 
> >   - FEXT:
> >       * path: \_SB_.FEXT
> >       * HID: FUJ02E3
> >       * methods invoked by kernel: FUNC
> >       * handles: hotkey, LEDs, platform attributes, backlight power
> >                                                     ^^^^^^^^^^^^^^^
> 
> This is very concise and describes the problem clearly, thank you!
> 
> > 
> > The problem is that if we split the ACPI drivers for those two devices
> > into separate modules, the FJEX driver will need to access the FUNC
> > method of device FEXT, handled by another driver in another module.
> > 
> > One way of solving this cleanly is to store a handle to the most
> > recently found FEXT instance (there should always be at most one anyway)
> > in a module-wide variable inside the FEXT driver, but that defeats the
> > purpose of this series.
> > 
> > Another solution is proposed by patch 04/10 of this series: make the
> > FJEX driver independently grab a handle to FEXT using the absolute ACPI
> > path to the latter.  It feels unnatural (AFAICT only one driver outside
> > drivers/acpi, namely pcc-cpufreq, does that), but it is safe and allows
> > us to drop all module-wide data.
> 
> Rafael's take on this would be useful.

Well, can you point me to patch [04/10] then?

Or better resend the whole series with a CC to linux-acpi (which it should go
to to start with IMO).

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals
  2017-05-09 16:47                 ` Darren Hart
  2017-05-09 21:24                   ` Rafael J. Wysocki
@ 2017-05-11 13:40                   ` Michał Kępień
  2017-05-15 23:27                     ` Darren Hart
  2017-05-15 23:56                     ` Jonathan Woithe
  1 sibling, 2 replies; 39+ messages in thread
From: Michał Kępień @ 2017-05-11 13:40 UTC (permalink / raw)
  To: Darren Hart, Jonathan Woithe
  Cc: Rafael Wysocki, Andy Shevchenko, platform-driver-x86, linux-kernel

> Perhaps so (overly zealous). Regarding the globals, let's be clear on the
> motivation. We want to follow good sw engineering practice, use data
> encapsulation, etc. However, using an explicit path to an ACPI device to avoid
> having a static file-level global doesn't really improve encapsulation in any
> way - it just shifts the blame :-)

Indeed, thanks for a clear-headed opinion.  I got a bit carried away :)

> Another reason to eliminate globals is to allow one driver to handle multiple
> devices - all device-specific data must be bound to the device, not the driver.
> In our case, there literally cannot be more than one _SB.FEXT. While there could
> theoretically be more than one FUJ02E3, I think we all agree that is highly
> improbable - and if it did happen, the explicit ACPI path approach would also be
> broken.

Good point.

> The motivation to divide the drivers was to provide functional encapsulation,
> accurately represent the system in the device tree, and to improve readability
> and maintainability of the driver code. So long as we can keep coupling to a
> minimum, I still think this makes sense.
> 
> So - static global variable for a driver with exactly one device that needs
> offer services to another driver... not really all that horrible.
> 
> You could accomplish this by making call_fext_func() not static and calling it
> from fujitsu-backlight. Or, you could further restrict it by exporting a
> fujitsu_backlight_power() function which wraps call_fext_func() providing a
> specific interface for fujitsu-backlight. This makes the ownership very explicit
> and ensures the usage doesn't grow without explicit changes to fujitsu-laptop.

I like the latter option more.  Exporting call_fext_func() as it is
would mean enabling other modules to reimplement fujitsu-laptop's
features and we do not want that.

> That is probably the most practical solution IFF we still feel it is worth
> splitting the driver into two separate modules. We need to develop a more robust
> and objective decision making process on module granularity (when to split, when
> to keep together). Will continue to give this more thought.

In light of the above, I still feel the split is worth going through
with.  The question is whether Jonathan feels the same :)

Jonathan, assuming the objective of splitting the module in two, allow
me to pick your brain a bit:

 1. Would you be okay with leaving "priv" as the variable name for
    device-specific data in both drivers?  If they are to be separated,
    "priv" would soon become unambiguous.  I do not have any strong
    feelings about this, though.

 2. Would you be okay with renaming "acpi_handle" to "handle"?  Darren
    seems to like this idea and in light of the above we would not have
    another ACPI handle inside struct fujitsu_bl any more.

 3. You mentioned earlier that you were not really fond of the fext_*()
    helper functions.  Would you like me to drop them and simply use
    call_fext_func() with five arguments everywhere?  Or should I keep
    the helper functions in v2?

Thanks,

-- 
Best regards,
Michał Kępień

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals
  2017-05-09 21:24                   ` Rafael J. Wysocki
@ 2017-05-11 13:52                     ` Michał Kępień
  2017-05-11 14:37                       ` Rafael J. Wysocki
  0 siblings, 1 reply; 39+ messages in thread
From: Michał Kępień @ 2017-05-11 13:52 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Darren Hart, Jonathan Woithe, Andy Shevchenko,
	platform-driver-x86, linux-kernel

> On Tuesday, May 09, 2017 09:47:34 AM Darren Hart wrote:
> > On Tue, May 09, 2017 at 11:35:24AM +0200, Michał Kępień wrote:
> > > > On Sat, May 06, 2017 at 02:45:16PM +0200, Michał Kępień wrote:
> > > > > > Just to make sure we are all on the same page here, choosing the "two
> > > > > > separate modules, each with one driver for one ACPI device" approach
> > > > > > would mean ending up with two modules:
> > > > > > 
> > > > > >   - fujitsu-laptop, binding to the FUJ02E3 ACPI device, handling
> > > > > >     everything _except_ backlight,
> > > > > > 
> > > > > >   - fujitsu-backlight, binding to the FUJ02B1 ACPI device, handling
> > > > > >     backlight and depending on fujitsu-laptop.
> > > > > > 
> > > > > > We would need to export one function from fujitsu-laptop, namely
> > > > > > fext_backlight().  I understand this would require creating a separate
> > > > > > header file which would then be included in fujitsu-backlight.
> > > > > > 
> > > > > > fext_backlight() causes the FUNC method of the FUJ02E3 ACPI device to be
> > > > > > called.  This method is marked as Serialized, which AFAIU means we do
> > > > > > not need a separate lock in kernel code because all calls to this method
> > > > > > are implicitly serialized by firmware itself.
> > > > > > 
> > > > > > I do not see anything "unnatural" in this approach, but I would love to
> > > > > > be corrected if I am wrong.
> > > > > 
> > > > > To be fair, one thing that may be "unnatural" with this approach is that
> > > > > even though fujitsu-backlight would depend on fujitsu-laptop, it would
> > > > > still have to get a handle to FUJ02E3 using:
> > > > > 
> > > > >     acpi_get_handle(NULL, "\\_SB.FEXT", ...)
> > > > >     
> > > > > because call_fext_func() - and thus fext_backlight() - needs to be
> > > > > passed a handle to FUJ02E3 and the two ACPI devices (FUJ02B1 handled by
> > > > > fujitsu-backlight and FUJ02E3 handled by fujitsu-laptop) are not related
> > > > > from the perspective of the ACPI device hierarchy.  Unless there is a
> > > > > better way of implementing this, in which case I am open to suggestions.
> > > > 
> > > > At a high level, I would consider the handle to be private data which should be
> > > > encapsulated in fujitsu_laptop. Or... where is FEXT in the ACPI hierarchy
> > > > relative to FUJ02E3?
> > > 
> > > FEXT *is* FUJ02E3:
> > > 
> > > Device (FEXT)
> > > {
> > >     Name (_HID, "FUJ02E3")  // _HID: Hardware ID
> > >     ...
> > >     Method (FUNC, 4, Serialized)
> > >     {
> > >         ...
> > >     }
> > >     ...
> > > }
> > > 
> > > See also below.
> > > 
> > > > Assuming FEXT is below FUJ02E3, the we appear to be making an assumption that
> > > > there is only one FUJ02E3 on the system. While I think this is perfectly
> > > > reasonable, it does contradict the argumentation from some of the other patches
> > > > in this series.
> > > 
> > > Exactly.  The whole purpose of this patch series is to stop using
> > > module-wide data.  We have a different situation here than in the case
> > > of e.g. dell-smbios, which coordinates access to a module-wide buffer it
> > > allocates.  
> > > 
> > > > If FEXT is not below fujitsu laptop... then it is a shared function which either
> > > > one of them can own and serialize (or not if fw indeed handles that).
> > > > 
> > > > Either way, the owning driver should abstract away the private data and present
> > > > an interface the other can use with only the "public" information.
> > > 
> > > I feel the problem at hand needs a fresh explanation.  I will be as
> > > concise as possible.
> > > 
> > > We are considering two ACPI devices present on Fujitsu laptops:
> > > 
> > >   - FJEX:
> > >       * path: \_SB_.PCI0.LPCB.FJEX
> > >       * HID: FUJ02B1
> > >       * methods invoked by kernel: GBLL, RBLL, SBLL, SBL2
> > >       * handles: backlight level (LCD brightness)
> > > 
> > >   - FEXT:
> > >       * path: \_SB_.FEXT
> > >       * HID: FUJ02E3
> > >       * methods invoked by kernel: FUNC
> > >       * handles: hotkey, LEDs, platform attributes, backlight power
> > >                                                     ^^^^^^^^^^^^^^^
> > 
> > This is very concise and describes the problem clearly, thank you!
> > 
> > > 
> > > The problem is that if we split the ACPI drivers for those two devices
> > > into separate modules, the FJEX driver will need to access the FUNC
> > > method of device FEXT, handled by another driver in another module.
> > > 
> > > One way of solving this cleanly is to store a handle to the most
> > > recently found FEXT instance (there should always be at most one anyway)
> > > in a module-wide variable inside the FEXT driver, but that defeats the
> > > purpose of this series.
> > > 
> > > Another solution is proposed by patch 04/10 of this series: make the
> > > FJEX driver independently grab a handle to FEXT using the absolute ACPI
> > > path to the latter.  It feels unnatural (AFAICT only one driver outside
> > > drivers/acpi, namely pcc-cpufreq, does that), but it is safe and allows
> > > us to drop all module-wide data.
> > 
> > Rafael's take on this would be useful.
> 
> Well, can you point me to patch [04/10] then?

Here is a link:

    https://www.spinics.net/lists/platform-driver-x86/msg11412.html

However, please note that in light of what Darren wrote, this specific
patch is likely to be dropped from v2.  Thus, there may be no point in
reviewing it after all, though your feedback would certainly be
appreciated for future reference.

> Or better resend the whole series with a CC to linux-acpi (which it should go
> to to start with IMO).

I did not think of that as this ten-patch series mostly revolves around
data encapsulation.  However, I think it might be worthwhile to CC
linux-acpi for the series that will split fujitsu-laptop in two, shall
it ever be posted.

-- 
Best regards,
Michał Kępień

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals
  2017-05-11 13:52                     ` Michał Kępień
@ 2017-05-11 14:37                       ` Rafael J. Wysocki
  2017-05-11 15:38                         ` Darren Hart
  0 siblings, 1 reply; 39+ messages in thread
From: Rafael J. Wysocki @ 2017-05-11 14:37 UTC (permalink / raw)
  To: Michał Kępień
  Cc: Darren Hart, Jonathan Woithe, Andy Shevchenko,
	platform-driver-x86, linux-kernel

On Thursday, May 11, 2017 03:52:11 PM Michał Kępień wrote:
> > On Tuesday, May 09, 2017 09:47:34 AM Darren Hart wrote:
> > > On Tue, May 09, 2017 at 11:35:24AM +0200, Michał Kępień wrote:
> > > > > On Sat, May 06, 2017 at 02:45:16PM +0200, Michał Kępień wrote:
> > > > > > > Just to make sure we are all on the same page here, choosing the "two
> > > > > > > separate modules, each with one driver for one ACPI device" approach
> > > > > > > would mean ending up with two modules:
> > > > > > > 
> > > > > > >   - fujitsu-laptop, binding to the FUJ02E3 ACPI device, handling
> > > > > > >     everything _except_ backlight,
> > > > > > > 
> > > > > > >   - fujitsu-backlight, binding to the FUJ02B1 ACPI device, handling
> > > > > > >     backlight and depending on fujitsu-laptop.
> > > > > > > 
> > > > > > > We would need to export one function from fujitsu-laptop, namely
> > > > > > > fext_backlight().  I understand this would require creating a separate
> > > > > > > header file which would then be included in fujitsu-backlight.
> > > > > > > 
> > > > > > > fext_backlight() causes the FUNC method of the FUJ02E3 ACPI device to be
> > > > > > > called.  This method is marked as Serialized, which AFAIU means we do
> > > > > > > not need a separate lock in kernel code because all calls to this method
> > > > > > > are implicitly serialized by firmware itself.
> > > > > > > 
> > > > > > > I do not see anything "unnatural" in this approach, but I would love to
> > > > > > > be corrected if I am wrong.
> > > > > > 
> > > > > > To be fair, one thing that may be "unnatural" with this approach is that
> > > > > > even though fujitsu-backlight would depend on fujitsu-laptop, it would
> > > > > > still have to get a handle to FUJ02E3 using:
> > > > > > 
> > > > > >     acpi_get_handle(NULL, "\\_SB.FEXT", ...)
> > > > > >     
> > > > > > because call_fext_func() - and thus fext_backlight() - needs to be
> > > > > > passed a handle to FUJ02E3 and the two ACPI devices (FUJ02B1 handled by
> > > > > > fujitsu-backlight and FUJ02E3 handled by fujitsu-laptop) are not related
> > > > > > from the perspective of the ACPI device hierarchy.  Unless there is a
> > > > > > better way of implementing this, in which case I am open to suggestions.
> > > > > 
> > > > > At a high level, I would consider the handle to be private data which should be
> > > > > encapsulated in fujitsu_laptop. Or... where is FEXT in the ACPI hierarchy
> > > > > relative to FUJ02E3?
> > > > 
> > > > FEXT *is* FUJ02E3:
> > > > 
> > > > Device (FEXT)
> > > > {
> > > >     Name (_HID, "FUJ02E3")  // _HID: Hardware ID
> > > >     ...
> > > >     Method (FUNC, 4, Serialized)
> > > >     {
> > > >         ...
> > > >     }
> > > >     ...
> > > > }
> > > > 
> > > > See also below.
> > > > 
> > > > > Assuming FEXT is below FUJ02E3, the we appear to be making an assumption that
> > > > > there is only one FUJ02E3 on the system. While I think this is perfectly
> > > > > reasonable, it does contradict the argumentation from some of the other patches
> > > > > in this series.
> > > > 
> > > > Exactly.  The whole purpose of this patch series is to stop using
> > > > module-wide data.  We have a different situation here than in the case
> > > > of e.g. dell-smbios, which coordinates access to a module-wide buffer it
> > > > allocates.  
> > > > 
> > > > > If FEXT is not below fujitsu laptop... then it is a shared function which either
> > > > > one of them can own and serialize (or not if fw indeed handles that).
> > > > > 
> > > > > Either way, the owning driver should abstract away the private data and present
> > > > > an interface the other can use with only the "public" information.
> > > > 
> > > > I feel the problem at hand needs a fresh explanation.  I will be as
> > > > concise as possible.
> > > > 
> > > > We are considering two ACPI devices present on Fujitsu laptops:
> > > > 
> > > >   - FJEX:
> > > >       * path: \_SB_.PCI0.LPCB.FJEX
> > > >       * HID: FUJ02B1
> > > >       * methods invoked by kernel: GBLL, RBLL, SBLL, SBL2
> > > >       * handles: backlight level (LCD brightness)
> > > > 
> > > >   - FEXT:
> > > >       * path: \_SB_.FEXT
> > > >       * HID: FUJ02E3
> > > >       * methods invoked by kernel: FUNC
> > > >       * handles: hotkey, LEDs, platform attributes, backlight power
> > > >                                                     ^^^^^^^^^^^^^^^
> > > 
> > > This is very concise and describes the problem clearly, thank you!
> > > 
> > > > 
> > > > The problem is that if we split the ACPI drivers for those two devices
> > > > into separate modules, the FJEX driver will need to access the FUNC
> > > > method of device FEXT, handled by another driver in another module.
> > > > 
> > > > One way of solving this cleanly is to store a handle to the most
> > > > recently found FEXT instance (there should always be at most one anyway)
> > > > in a module-wide variable inside the FEXT driver, but that defeats the
> > > > purpose of this series.
> > > > 
> > > > Another solution is proposed by patch 04/10 of this series: make the
> > > > FJEX driver independently grab a handle to FEXT using the absolute ACPI
> > > > path to the latter.  It feels unnatural (AFAICT only one driver outside
> > > > drivers/acpi, namely pcc-cpufreq, does that), but it is safe and allows
> > > > us to drop all module-wide data.
> > > 
> > > Rafael's take on this would be useful.
> > 
> > Well, can you point me to patch [04/10] then?
> 
> Here is a link:
> 
>     https://www.spinics.net/lists/platform-driver-x86/msg11412.html

Thanks!

> However, please note that in light of what Darren wrote, this specific
> patch is likely to be dropped from v2.  Thus, there may be no point in
> reviewing it after all, though your feedback would certainly be
> appreciated for future reference.

OK

> > Or better resend the whole series with a CC to linux-acpi (which it should go
> > to to start with IMO).
> 
> I did not think of that as this ten-patch series mostly revolves around
> data encapsulation.  However, I think it might be worthwhile to CC
> linux-acpi for the series that will split fujitsu-laptop in two, shall
> it ever be posted.

OK, but as a rule of thumb, it is better to CC everything touching ACPI to
linux-acpi just to let people know what you're doing if nothing else.

And if there are ACPI-related questions down the road, the context is there
aleady, so it is generally easier to answer them then.

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals
  2017-05-11 14:37                       ` Rafael J. Wysocki
@ 2017-05-11 15:38                         ` Darren Hart
  0 siblings, 0 replies; 39+ messages in thread
From: Darren Hart @ 2017-05-11 15:38 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Michał Kępień,
	Jonathan Woithe, Andy Shevchenko, platform-driver-x86,
	linux-kernel

On Thu, May 11, 2017 at 04:37:30PM +0200, Rafael Wysocki wrote:
> On Thursday, May 11, 2017 03:52:11 PM Michał Kępień wrote:
> > > On Tuesday, May 09, 2017 09:47:34 AM Darren Hart wrote:
> > > > On Tue, May 09, 2017 at 11:35:24AM +0200, Michał Kępień wrote:
> > > > > > On Sat, May 06, 2017 at 02:45:16PM +0200, Michał Kępień wrote:
...
> > > > > I feel the problem at hand needs a fresh explanation.  I will be as
> > > > > concise as possible.
> > > > > 
> > > > > We are considering two ACPI devices present on Fujitsu laptops:
> > > > > 
> > > > >   - FJEX:
> > > > >       * path: \_SB_.PCI0.LPCB.FJEX
> > > > >       * HID: FUJ02B1
> > > > >       * methods invoked by kernel: GBLL, RBLL, SBLL, SBL2
> > > > >       * handles: backlight level (LCD brightness)
> > > > > 
> > > > >   - FEXT:
> > > > >       * path: \_SB_.FEXT
> > > > >       * HID: FUJ02E3
> > > > >       * methods invoked by kernel: FUNC
> > > > >       * handles: hotkey, LEDs, platform attributes, backlight power
> > > > >                                                     ^^^^^^^^^^^^^^^
> > > > 
> > > > This is very concise and describes the problem clearly, thank you!
> > > > 
> > > > > 
> > > > > The problem is that if we split the ACPI drivers for those two devices
> > > > > into separate modules, the FJEX driver will need to access the FUNC
> > > > > method of device FEXT, handled by another driver in another module.
> > > > > 
> > > > > One way of solving this cleanly is to store a handle to the most
> > > > > recently found FEXT instance (there should always be at most one anyway)
> > > > > in a module-wide variable inside the FEXT driver, but that defeats the
> > > > > purpose of this series.
> > > > > 
> > > > > Another solution is proposed by patch 04/10 of this series: make the
> > > > > FJEX driver independently grab a handle to FEXT using the absolute ACPI
> > > > > path to the latter.  It feels unnatural (AFAICT only one driver outside
> > > > > drivers/acpi, namely pcc-cpufreq, does that), but it is safe and allows
> > > > > us to drop all module-wide data.
> > > > 
> > > > Rafael's take on this would be useful.
> > > 
> > > Well, can you point me to patch [04/10] then?
> > 
> > Here is a link:
> > 
> >     https://www.spinics.net/lists/platform-driver-x86/msg11412.html
> 
> Thanks!
> 
> > However, please note that in light of what Darren wrote, this specific
> > patch is likely to be dropped from v2.  Thus, there may be no point in
> > reviewing it after all, though your feedback would certainly be
> > appreciated for future reference.
> 
> OK

Rafael's take on balancing one driver per device, versus a single driver with
interdependent ACPI devices, even if one of them doesn't show up in the device
hierarchy, as well as one driver calling into another one, all from his
experience with ACPI device drivers would be valuable, and could sway my advice
above.

> 
> > > Or better resend the whole series with a CC to linux-acpi (which it should go
> > > to to start with IMO).
> > 
> > I did not think of that as this ten-patch series mostly revolves around
> > data encapsulation.  However, I think it might be worthwhile to CC
> > linux-acpi for the series that will split fujitsu-laptop in two, shall
> > it ever be posted.
> 
> OK, but as a rule of thumb, it is better to CC everything touching ACPI to
> linux-acpi just to let people know what you're doing if nothing else.
> 
> And if there are ACPI-related questions down the road, the context is there
> aleady, so it is generally easier to answer them then.
> 

Agreed. Unfortunately, we don't have a good way to make this clear in
MAINTAINERS without enumerating every driver. I'll try to make sure the regular
contributors know this, but new folks will continue to miss it unless we can
find a better way to make it obvious.


-- 
Darren Hart
VMware Open Source Technology Center

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals
  2017-05-11 13:40                   ` Michał Kępień
@ 2017-05-15 23:27                     ` Darren Hart
  2017-05-16  0:06                       ` Jonathan Woithe
  2017-05-15 23:56                     ` Jonathan Woithe
  1 sibling, 1 reply; 39+ messages in thread
From: Darren Hart @ 2017-05-15 23:27 UTC (permalink / raw)
  To: Michał Kępień
  Cc: Jonathan Woithe, Rafael Wysocki, Andy Shevchenko,
	platform-driver-x86, linux-kernel

On Thu, May 11, 2017 at 03:40:28PM +0200, Michał Kępień wrote:
> > Perhaps so (overly zealous). Regarding the globals, let's be clear on the
> > motivation. We want to follow good sw engineering practice, use data
> > encapsulation, etc. However, using an explicit path to an ACPI device to avoid
> > having a static file-level global doesn't really improve encapsulation in any
> > way - it just shifts the blame :-)
> 
> Indeed, thanks for a clear-headed opinion.  I got a bit carried away :)
> 
> > Another reason to eliminate globals is to allow one driver to handle multiple
> > devices - all device-specific data must be bound to the device, not the driver.
> > In our case, there literally cannot be more than one _SB.FEXT. While there could
> > theoretically be more than one FUJ02E3, I think we all agree that is highly
> > improbable - and if it did happen, the explicit ACPI path approach would also be
> > broken.
> 
> Good point.
> 
> > The motivation to divide the drivers was to provide functional encapsulation,
> > accurately represent the system in the device tree, and to improve readability
> > and maintainability of the driver code. So long as we can keep coupling to a
> > minimum, I still think this makes sense.
> > 
> > So - static global variable for a driver with exactly one device that needs
> > offer services to another driver... not really all that horrible.
> > 
> > You could accomplish this by making call_fext_func() not static and calling it
> > from fujitsu-backlight. Or, you could further restrict it by exporting a
> > fujitsu_backlight_power() function which wraps call_fext_func() providing a
> > specific interface for fujitsu-backlight. This makes the ownership very explicit
> > and ensures the usage doesn't grow without explicit changes to fujitsu-laptop.
> 
> I like the latter option more.  Exporting call_fext_func() as it is
> would mean enabling other modules to reimplement fujitsu-laptop's
> features and we do not want that.
> 
> > That is probably the most practical solution IFF we still feel it is worth
> > splitting the driver into two separate modules. We need to develop a more robust
> > and objective decision making process on module granularity (when to split, when
> > to keep together). Will continue to give this more thought.
> 
> In light of the above, I still feel the split is worth going through
> with.  The question is whether Jonathan feels the same :)
> 

In the interest of keeping this moving... As I'm not sure there is a "right
answer" to split or not, and nobody screamed out against splitting, and this is
the direction Michal seems to prefer, and he is doing the work, let's proceed
with the split of -backlight and -laptop.

> Jonathan, assuming the objective of splitting the module in two, allow
> me to pick your brain a bit:
> 
>  1. Would you be okay with leaving "priv" as the variable name for
>     device-specific data in both drivers?  If they are to be separated,
>     "priv" would soon become unambiguous.  I do not have any strong
>     feelings about this, though.
> 
>  2. Would you be okay with renaming "acpi_handle" to "handle"?  Darren
>     seems to like this idea and in light of the above we would not have
>     another ACPI handle inside struct fujitsu_bl any more.

Both of these are easily discussed in the next series which will most likely
have at least one respin anyway.

>  3. You mentioned earlier that you were not really fond of the fext_*()
>     helper functions.  Would you like me to drop them and simply use
>     call_fext_func() with five arguments everywhere?  Or should I keep
>     the helper functions in v2?

I was torn on this as well - I didn't think they added much value. Let's focus
on splitting the driver, and we can revisit this later for the -laptop driver if
there is interest.

-- 
Darren Hart
VMware Open Source Technology Center

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals
  2017-05-11 13:40                   ` Michał Kępień
  2017-05-15 23:27                     ` Darren Hart
@ 2017-05-15 23:56                     ` Jonathan Woithe
  1 sibling, 0 replies; 39+ messages in thread
From: Jonathan Woithe @ 2017-05-15 23:56 UTC (permalink / raw)
  To: Micha?? K??pie??
  Cc: Darren Hart, Rafael Wysocki, Andy Shevchenko,
	platform-driver-x86, linux-kernel

Hi Michael

Apologies for the delayed response - things have been a bit crazy lately. 
Darren's mail just now has reminded me that you were awaiting feedback.

On Thu, May 11, 2017 at 03:40:28PM +0200, Micha?? K??pie?? wrote:
> > You could accomplish this by making call_fext_func() not static and calling it
> > from fujitsu-backlight. Or, you could further restrict it by exporting a
> > fujitsu_backlight_power() function which wraps call_fext_func() providing a
> > specific interface for fujitsu-backlight. This makes the ownership very explicit
> > and ensures the usage doesn't grow without explicit changes to fujitsu-laptop.
> 
> I like the latter option more.  Exporting call_fext_func() as it is
> would mean enabling other modules to reimplement fujitsu-laptop's
> features and we do not want that.

Agreed - we don't need duplicate implementations.

> > That is probably the most practical solution IFF we still feel it is worth
> > splitting the driver into two separate modules. We need to develop a more robust
> > and objective decision making process on module granularity (when to split, when
> > to keep together). Will continue to give this more thought.
> 
> In light of the above, I still feel the split is worth going through
> with.  The question is whether Jonathan feels the same :)

I'm pretty much sitting on the fence regarding the split.  Darren has
provided a compelling argument for proceeding with the split so I guess we
can pursue that and see where it leads.  It still feels odd to me that two
different drivers will end up touching the same hardware control, but at the
end of the day the fact that this is necessary says more about the hardware
design than anything else.  There isn't really a clean solution which ticks
every box so we just need to pick one and run with it.

> Jonathan, assuming the objective of splitting the module in two, allow
> me to pick your brain a bit:
> 
>  1. Would you be okay with leaving "priv" as the variable name for
>     device-specific data in both drivers?  If they are to be separated,
>     "priv" would soon become unambiguous.  I do not have any strong
>     feelings about this, though.

With separate drivers I don't have a problem with the generic "priv" since
the ambiguity drops away due to the driver encapsulation.

>  2. Would you be okay with renaming "acpi_handle" to "handle"?  Darren
>     seems to like this idea and in light of the above we would not have
>     another ACPI handle inside struct fujitsu_bl any more.

My primary concern was the presence of two ACPI handles.  If there are two
such handles, calling one of them by a very generic term tends to make the
code confusing IMHO.  However, since the split avoids the need for either
driver to carry two ACPI handles this becomes a moot point and the plain
"handle" can be justified.  This is of course contingent on the split
happening.

>  3. You mentioned earlier that you were not really fond of the fext_*()
>     helper functions.  Would you like me to drop them and simply use
>     call_fext_func() with five arguments everywhere?  Or should I keep
>     the helper functions in v2?

The extended explanation of the rationale for these functions (provided a
few days later) makes their presence clearer.  In some ways though it still
feels like this is being done solely due to the (somewhat arbitrary)
checkpatch 80 column thing, rather than being driven by a bona fide code
structuring issue.  Having said that, the number of parameters to
call_fext_func() has grown to 5 as you said, and that is arguably an issue
in its own right.

On balance (taking your detailed explanation into consideration), since
Darren hasn't voiced any concern about these functions and they do permit a
clean up of the call sites I am happy for you to retain them in v2.

Regards
  jonathan

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals
  2017-05-15 23:27                     ` Darren Hart
@ 2017-05-16  0:06                       ` Jonathan Woithe
  2017-05-16  6:40                         ` Michał Kępień
  0 siblings, 1 reply; 39+ messages in thread
From: Jonathan Woithe @ 2017-05-16  0:06 UTC (permalink / raw)
  To: Darren Hart
  Cc: Micha?? K??pie??,
	Rafael Wysocki, Andy Shevchenko, platform-driver-x86,
	linux-kernel

On Mon, May 15, 2017 at 04:27:25PM -0700, Darren Hart wrote:
> > In light of the above, I still feel the split is worth going through
> > with.  The question is whether Jonathan feels the same :)
> 
> In the interest of keeping this moving... As I'm not sure there is a "right
> answer" to split or not, and nobody screamed out against splitting, and this is
> the direction Michal seems to prefer, and he is doing the work, let's proceed
> with the split of -backlight and -laptop.

Apologies for not getting back about this earlier.  As mentioned in my
follow up to Michael's post from a few minutes ago I agree with the above
sentiment.

> > Jonathan, assuming the objective of splitting the module in two, allow
> > me to pick your brain a bit:
> > 
> >  1. Would you be okay with leaving "priv" as the variable name for
> >     device-specific data in both drivers?  If they are to be separated,
> >     "priv" would soon become unambiguous.  I do not have any strong
> >     feelings about this, though.
> > 
> >  2. Would you be okay with renaming "acpi_handle" to "handle"?  Darren
> >     seems to like this idea and in light of the above we would not have
> >     another ACPI handle inside struct fujitsu_bl any more.
> 
> Both of these are easily discussed in the next series which will most likely
> have at least one respin anyway.

Assuming the split happens I am happy with both of these proposals.  The
concerns raised earlier were precipitated mostly because I was unaware of
the medium term goal of splitting the driver (not because it hadn't been
mentioned, but because I had forgotten about it in the time since it was
first raised earlier in the year).

> >  3. You mentioned earlier that you were not really fond of the fext_*()
> >     helper functions.  Would you like me to drop them and simply use
> >     call_fext_func() with five arguments everywhere?  Or should I keep
> >     the helper functions in v2?
> 
> I was torn on this as well - I didn't think they added much value. Let's
> focus on splitting the driver, and we can revisit this later for the
> -laptop driver if there is interest.

It seems I misinterpreted Darren's stance on this one and misrepresented him
in my previous post (sorry Darren).  Since Darren's preferred approach
is to drop them for the moment let's run with that.  As he said, once the
split has been made we can obviously revisit this to see if there value in
using them in the context of the split drivers.

Regards
  jonathan

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals
  2017-05-16  0:06                       ` Jonathan Woithe
@ 2017-05-16  6:40                         ` Michał Kępień
  0 siblings, 0 replies; 39+ messages in thread
From: Michał Kępień @ 2017-05-16  6:40 UTC (permalink / raw)
  To: Jonathan Woithe, Darren Hart
  Cc: Rafael Wysocki, Andy Shevchenko, platform-driver-x86, linux-kernel

> On Mon, May 15, 2017 at 04:27:25PM -0700, Darren Hart wrote:
> > > In light of the above, I still feel the split is worth going through
> > > with.  The question is whether Jonathan feels the same :)
> > 
> > In the interest of keeping this moving... As I'm not sure there is a "right
> > answer" to split or not, and nobody screamed out against splitting, and this is
> > the direction Michal seems to prefer, and he is doing the work, let's proceed
> > with the split of -backlight and -laptop.
> 
> Apologies for not getting back about this earlier.  As mentioned in my
> follow up to Michael's post from a few minutes ago I agree with the above
> sentiment.
> 
> > > Jonathan, assuming the objective of splitting the module in two, allow
> > > me to pick your brain a bit:
> > > 
> > >  1. Would you be okay with leaving "priv" as the variable name for
> > >     device-specific data in both drivers?  If they are to be separated,
> > >     "priv" would soon become unambiguous.  I do not have any strong
> > >     feelings about this, though.
> > > 
> > >  2. Would you be okay with renaming "acpi_handle" to "handle"?  Darren
> > >     seems to like this idea and in light of the above we would not have
> > >     another ACPI handle inside struct fujitsu_bl any more.
> > 
> > Both of these are easily discussed in the next series which will most likely
> > have at least one respin anyway.
> 
> Assuming the split happens I am happy with both of these proposals.  The
> concerns raised earlier were precipitated mostly because I was unaware of
> the medium term goal of splitting the driver (not because it hadn't been
> mentioned, but because I had forgotten about it in the time since it was
> first raised earlier in the year).
> 
> > >  3. You mentioned earlier that you were not really fond of the fext_*()
> > >     helper functions.  Would you like me to drop them and simply use
> > >     call_fext_func() with five arguments everywhere?  Or should I keep
> > >     the helper functions in v2?
> > 
> > I was torn on this as well - I didn't think they added much value. Let's
> > focus on splitting the driver, and we can revisit this later for the
> > -laptop driver if there is interest.
> 
> It seems I misinterpreted Darren's stance on this one and misrepresented him
> in my previous post (sorry Darren).  Since Darren's preferred approach
> is to drop them for the moment let's run with that.  As he said, once the
> split has been made we can obviously revisit this to see if there value in
> using them in the context of the split drivers.

Jonathan, Darren, thank you for all the feedback.  Silence on my behalf
has not been coincidental as I have also been busy lately and had to put
kernel stuff on the back burner.  Sadly, I can also now confirm that I
will no longer have access to the E744 I used to test my patches on as
of next Monday.  I will do my best to prepare v2 of this series before
that.

-- 
Best regards,
Michał Kępień

^ permalink raw reply	[flat|nested] 39+ messages in thread

end of thread, other threads:[~2017-05-16  6:40 UTC | newest]

Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-24 13:33 [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals Michał Kępień
2017-04-24 13:33 ` [PATCH 01/10] platform/x86: fujitsu-laptop: introduce fext_*() helper functions Michał Kępień
2017-05-01 13:13   ` Jonathan Woithe
2017-05-02 13:24     ` Michał Kępień
2017-04-24 13:33 ` [PATCH 02/10] platform/x86: fujitsu-laptop: shorten names of acpi_handle fields Michał Kępień
2017-05-01 13:19   ` Jonathan Woithe
2017-05-01 16:09     ` Darren Hart
2017-04-24 13:33 ` [PATCH 03/10] platform/x86: fujitsu-laptop: explicitly pass ACPI handle to call_fext_func() Michał Kępień
2017-04-24 13:33 ` [PATCH 04/10] platform/x86: fujitsu-laptop: rework backlight power synchronization Michał Kępień
2017-05-01 13:32   ` Jonathan Woithe
2017-05-01 16:17     ` Darren Hart
2017-04-24 13:33 ` [PATCH 05/10] platform/x86: fujitsu-laptop: distinguish current uses of device-specific data Michał Kępień
2017-05-01 13:40   ` Jonathan Woithe
2017-04-24 13:33 ` [PATCH 06/10] platform/x86: fujitsu-laptop: allocate struct fujitsu_bl in acpi_fujitsu_bl_add() Michał Kępień
2017-04-24 13:33 ` [PATCH 07/10] platform/x86: fujitsu-laptop: use device-specific data in backlight code Michał Kępień
2017-04-24 13:33 ` [PATCH 08/10] platform/x86: fujitsu-laptop: allocate struct fujitsu_laptop in acpi_fujitsu_laptop_add() Michał Kępień
2017-04-24 13:33 ` [PATCH 09/10] platform/x86: fujitsu-laptop: use device-specific data in LED-related code Michał Kępień
2017-04-24 13:33 ` [PATCH 10/10] platform/x86: fujitsu-laptop: use device-specific data in remaining module code Michał Kępień
2017-05-01 13:05 ` [PATCH 00/10] fujitsu-laptop: use device-specific data instead of module-wide globals Jonathan Woithe
2017-05-02 13:21   ` Michał Kępień
2017-05-04 23:40     ` Jonathan Woithe
2017-05-05 16:15       ` Darren Hart
2017-05-06 12:31         ` Michał Kępień
2017-05-06 12:45           ` Michał Kępień
2017-05-06 14:21             ` Andy Shevchenko
2017-05-06 14:23               ` Andy Shevchenko
2017-05-08 16:01             ` Darren Hart
2017-05-09  9:35               ` Michał Kępień
2017-05-09 12:13                 ` Jonathan Woithe
2017-05-09 16:47                 ` Darren Hart
2017-05-09 21:24                   ` Rafael J. Wysocki
2017-05-11 13:52                     ` Michał Kępień
2017-05-11 14:37                       ` Rafael J. Wysocki
2017-05-11 15:38                         ` Darren Hart
2017-05-11 13:40                   ` Michał Kępień
2017-05-15 23:27                     ` Darren Hart
2017-05-16  0:06                       ` Jonathan Woithe
2017-05-16  6:40                         ` Michał Kępień
2017-05-15 23:56                     ` Jonathan Woithe

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).