This is the second of the two patch series I started preparing back in
June 2017 [1]. It took me this long to post it purely due to permanent
spare time shortage, not because the changes are complicated.
The patch series contains miscellaneous cleanups which I think are worth
getting done before splitting fujitsu-laptop into two separate modules.
I am not 100% sure that all the changes in the last patch in this series
actually help, so please speak your mind.
This patch series was tested on a Lifebook S7020. AFAICT it does not
conflict with the recent draft patch from Jan-Marek Glogowski and may
thus be applied independently.
Finally, please forgive me if it takes me weeks or months to address
review comments. It is also perfectly fine for reviews to take weeks or
months ;)
[1] https://www.spinics.net/lists/kernel/msg2534759.html
drivers/platform/x86/fujitsu-laptop.c | 179 +++++++++++++++++-----------------
1 file changed, 90 insertions(+), 89 deletions(-)
--
2.16.1
Align all constant values defined in the module to a common indentation.
Rename ACPI_FUJITSU_NOTIFY_CODE1 to ACPI_FUJITSU_NOTIFY_CODE as there is
only one ACPI notification code used throughout the driver. Define all
bitmasks using the BIT() macro. Clean up comments.
Signed-off-by: Michał Kępień <[email protected]>
---
drivers/platform/x86/fujitsu-laptop.c | 65 ++++++++++++++++++-----------------
1 file changed, 33 insertions(+), 32 deletions(-)
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 0ee03d1fcbc4..07c713d9d273 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -63,9 +63,9 @@
#include <linux/platform_device.h>
#include <acpi/video.h>
-#define FUJITSU_DRIVER_VERSION "0.6.0"
+#define FUJITSU_DRIVER_VERSION "0.6.0"
-#define FUJITSU_LCD_N_LEVELS 8
+#define FUJITSU_LCD_N_LEVELS 8
#define ACPI_FUJITSU_CLASS "fujitsu"
#define ACPI_FUJITSU_BL_HID "FUJ02B1"
@@ -75,46 +75,47 @@
#define ACPI_FUJITSU_LAPTOP_DRIVER_NAME "Fujitsu laptop FUJ02E3 ACPI hotkeys driver"
#define ACPI_FUJITSU_LAPTOP_DEVICE_NAME "Fujitsu FUJ02E3"
-#define ACPI_FUJITSU_NOTIFY_CODE1 0x80
+#define ACPI_FUJITSU_NOTIFY_CODE 0x80
/* FUNC interface - command values */
-#define FUNC_FLAGS 0x1000
-#define FUNC_LEDS 0x1001
-#define FUNC_BUTTONS 0x1002
-#define FUNC_BACKLIGHT 0x1004
+#define FUNC_FLAGS BIT(12)
+#define FUNC_LEDS (BIT(12) | BIT(0))
+#define FUNC_BUTTONS (BIT(12) | BIT(1))
+#define FUNC_BACKLIGHT (BIT(12) | BIT(2))
/* FUNC interface - responses */
-#define UNSUPPORTED_CMD 0x80000000
+#define UNSUPPORTED_CMD BIT(31)
/* FUNC interface - status flags */
-#define FLAG_RFKILL 0x020
-#define FLAG_LID 0x100
-#define FLAG_DOCK 0x200
+#define FLAG_RFKILL BIT(5)
+#define FLAG_LID BIT(8)
+#define FLAG_DOCK BIT(9)
/* FUNC interface - LED control */
-#define FUNC_LED_OFF 0x1
-#define FUNC_LED_ON 0x30001
-#define KEYBOARD_LAMPS 0x100
-#define LOGOLAMP_POWERON 0x2000
-#define LOGOLAMP_ALWAYS 0x4000
-#define RADIO_LED_ON 0x20
-#define ECO_LED 0x10000
-#define ECO_LED_ON 0x80000
+#define FUNC_LED_OFF BIT(0)
+#define FUNC_LED_ON (BIT(0) | BIT(16) | BIT(17))
+#define LOGOLAMP_POWERON BIT(13)
+#define LOGOLAMP_ALWAYS BIT(14)
+#define KEYBOARD_LAMPS BIT(8)
+#define RADIO_LED_ON BIT(5)
+#define ECO_LED BIT(16)
+#define ECO_LED_ON BIT(19)
/* FUNC interface - backlight power control */
-#define BACKLIGHT_POWER 0x4
-#define BACKLIGHT_OFF 0x3
-#define BACKLIGHT_ON 0x0
+#define BACKLIGHT_POWER BIT(2)
+#define BACKLIGHT_OFF (BIT(0) | BIT(1))
+#define BACKLIGHT_ON 0
-/* Hotkey details */
-#define KEY1_CODE 0x410 /* codes for the keys in the GIRB register */
-#define KEY2_CODE 0x411
-#define KEY3_CODE 0x412
-#define KEY4_CODE 0x413
-#define KEY5_CODE 0x420
+/* Scancodes read from the GIRB register */
+#define KEY1_CODE 0x410
+#define KEY2_CODE 0x411
+#define KEY3_CODE 0x412
+#define KEY4_CODE 0x413
+#define KEY5_CODE 0x420
-#define MAX_HOTKEY_RINGBUFFER_SIZE 100
-#define RINGBUFFERSIZE 40
+/* Hotkey ringbuffer limits */
+#define MAX_HOTKEY_RINGBUFFER_SIZE 100
+#define RINGBUFFERSIZE 40
/* Module parameters */
static int use_alt_lcd_levels = -1;
@@ -428,7 +429,7 @@ static void acpi_fujitsu_bl_notify(struct acpi_device *device, u32 event)
struct fujitsu_bl *priv = acpi_driver_data(device);
int oldb, newb;
- if (event != ACPI_FUJITSU_NOTIFY_CODE1) {
+ if (event != ACPI_FUJITSU_NOTIFY_CODE) {
acpi_handle_info(device->handle, "unsupported event [0x%x]\n",
event);
sparse_keymap_report_event(priv->input, -1, 1, true);
@@ -902,7 +903,7 @@ static void acpi_fujitsu_laptop_notify(struct acpi_device *device, u32 event)
int scancode, i = 0;
unsigned int irb;
- if (event != ACPI_FUJITSU_NOTIFY_CODE1) {
+ if (event != ACPI_FUJITSU_NOTIFY_CODE) {
acpi_handle_info(device->handle, "Unsupported event [0x%x]\n",
event);
sparse_keymap_report_event(priv->input, -1, 1, true);
--
2.16.1
Do not include linux/slab.h as all module code now uses managed memory
allocations and thus neither k*alloc() nor kfree() is used.
Signed-off-by: Michał Kępień <[email protected]>
---
drivers/platform/x86/fujitsu-laptop.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 7888b779d6c5..17779b8b7f30 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -61,7 +61,6 @@
#include <linux/kfifo.h>
#include <linux/leds.h>
#include <linux/platform_device.h>
-#include <linux/slab.h>
#include <acpi/video.h>
#define FUJITSU_DRIVER_VERSION "0.6.0"
--
2.16.1
Use named constants instead of integers in call_fext_func() invocations
related to backlight power control in order to more clearly convey the
intent of each call.
Signed-off-by: Michał Kępień <[email protected]>
---
drivers/platform/x86/fujitsu-laptop.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 17779b8b7f30..0ee03d1fcbc4 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -101,6 +101,11 @@
#define ECO_LED 0x10000
#define ECO_LED_ON 0x80000
+/* FUNC interface - backlight power control */
+#define BACKLIGHT_POWER 0x4
+#define BACKLIGHT_OFF 0x3
+#define BACKLIGHT_ON 0x0
+
/* Hotkey details */
#define KEY1_CODE 0x410 /* codes for the keys in the GIRB register */
#define KEY2_CODE 0x411
@@ -257,9 +262,11 @@ static int bl_update_status(struct backlight_device *b)
if (fext) {
if (b->props.power == FB_BLANK_POWERDOWN)
- call_fext_func(fext, FUNC_BACKLIGHT, 0x1, 0x4, 0x3);
+ call_fext_func(fext, FUNC_BACKLIGHT, 0x1,
+ BACKLIGHT_POWER, BACKLIGHT_OFF);
else
- call_fext_func(fext, FUNC_BACKLIGHT, 0x1, 0x4, 0x0);
+ call_fext_func(fext, FUNC_BACKLIGHT, 0x1,
+ BACKLIGHT_POWER, BACKLIGHT_ON);
}
return set_lcd_level(device, b->props.brightness);
@@ -818,7 +825,8 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
/* Sync backlight power status */
if (fujitsu_bl && fujitsu_bl->bl_device &&
acpi_video_get_backlight_type() == acpi_backlight_vendor) {
- if (call_fext_func(fext, FUNC_BACKLIGHT, 0x2, 0x4, 0x0) == 3)
+ if (call_fext_func(fext, FUNC_BACKLIGHT, 0x2, BACKLIGHT_POWER,
+ 0x0) == 3)
fujitsu_bl->bl_device->props.power = FB_BLANK_POWERDOWN;
else
fujitsu_bl->bl_device->props.power = FB_BLANK_UNBLANK;
--
2.16.1
In order to improve code clarity, move declarations of variables that
are module parameters higher up and add a comment.
Signed-off-by: Michał Kępień <[email protected]>
---
drivers/platform/x86/fujitsu-laptop.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 94ff7f86fa8f..7888b779d6c5 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -112,6 +112,10 @@
#define MAX_HOTKEY_RINGBUFFER_SIZE 100
#define RINGBUFFERSIZE 40
+/* Module parameters */
+static int use_alt_lcd_levels = -1;
+static bool disable_brightness_adjust;
+
/* Device controlling the backlight and associated keys */
struct fujitsu_bl {
struct input_dev *input;
@@ -122,8 +126,6 @@ struct fujitsu_bl {
};
static struct fujitsu_bl *fujitsu_bl;
-static int use_alt_lcd_levels = -1;
-static bool disable_brightness_adjust;
/* Device used to access hotkeys and other features on the laptop */
struct fujitsu_laptop {
--
2.16.1
Replace the last few lines of acpi_fujitsu_bl_add() with a simple return
in order to improve code readability without changing the logic.
As acpi_fujitsu_laptop_add() uses a managed memory allocation for
device-specific data, it is fine to just return immediately upon kfifo
allocation failure. Do that instead of jumping to the end of the
function to improve code readability. Running out of memory while
allocating the kfifo does not seem probable enough to warrant logging an
error message, so do not do it.
Signed-off-by: Michał Kępień <[email protected]>
---
drivers/platform/x86/fujitsu-laptop.c | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 7f30a427a16c..94ff7f86fa8f 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -410,11 +410,7 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device)
if (ret)
return ret;
- ret = fujitsu_backlight_register(device);
- if (ret)
- return ret;
-
- return 0;
+ return fujitsu_backlight_register(device);
}
/* Brightness notify */
@@ -790,10 +786,8 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
spin_lock_init(&priv->fifo_lock);
ret = kfifo_alloc(&priv->fifo, RINGBUFFERSIZE * sizeof(int),
GFP_KERNEL);
- if (ret) {
- pr_err("kfifo_alloc failed\n");
- goto err_stop;
- }
+ if (ret)
+ return ret;
pr_info("ACPI: %s [%s]\n",
acpi_device_name(device), acpi_device_bid(device));
@@ -845,7 +839,7 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
err_free_fifo:
kfifo_free(&priv->fifo);
-err_stop:
+
return ret;
}
--
2.16.1
Only register input devices after the device-specific data structures
they access are fully initialized.
Signed-off-by: Michał Kępień <[email protected]>
---
drivers/platform/x86/fujitsu-laptop.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index b5f782807bfa..7f30a427a16c 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -399,10 +399,6 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device)
strcpy(acpi_device_class(device), ACPI_FUJITSU_CLASS);
device->driver_data = priv;
- ret = acpi_fujitsu_bl_input_setup(device);
- if (ret)
- return ret;
-
pr_info("ACPI: %s [%s]\n",
acpi_device_name(device), acpi_device_bid(device));
@@ -410,6 +406,10 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device)
priv->max_brightness = FUJITSU_LCD_N_LEVELS;
get_lcd_level(device);
+ ret = acpi_fujitsu_bl_input_setup(device);
+ if (ret)
+ return ret;
+
ret = fujitsu_backlight_register(device);
if (ret)
return ret;
@@ -795,10 +795,6 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
goto err_stop;
}
- ret = acpi_fujitsu_laptop_input_setup(device);
- if (ret)
- goto err_free_fifo;
-
pr_info("ACPI: %s [%s]\n",
acpi_device_name(device), acpi_device_bid(device));
@@ -833,6 +829,10 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
fujitsu_bl->bl_device->props.power = FB_BLANK_UNBLANK;
}
+ ret = acpi_fujitsu_laptop_input_setup(device);
+ if (ret)
+ goto err_free_fifo;
+
ret = acpi_fujitsu_laptop_leds_register(device);
if (ret)
goto err_free_fifo;
--
2.16.1
Different functions in the module use varying names (error, result,
status) for a local variable storing the return value of a function call
that has to be checked for errors. Use a common name (ret) for all
these local variables to improve code consistency. Merge integer
variable declarations in acpi_fujitsu_laptop_add() into one line.
Signed-off-by: Michał Kępień <[email protected]>
---
drivers/platform/x86/fujitsu-laptop.c | 85 +++++++++++++++++------------------
1 file changed, 41 insertions(+), 44 deletions(-)
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 2cfbd3fa5136..b5f782807bfa 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -385,7 +385,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 error;
+ int ret;
if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
return -ENODEV;
@@ -399,9 +399,9 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device)
strcpy(acpi_device_class(device), ACPI_FUJITSU_CLASS);
device->driver_data = priv;
- error = acpi_fujitsu_bl_input_setup(device);
- if (error)
- return error;
+ ret = acpi_fujitsu_bl_input_setup(device);
+ if (ret)
+ return ret;
pr_info("ACPI: %s [%s]\n",
acpi_device_name(device), acpi_device_bid(device));
@@ -410,9 +410,9 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device)
priv->max_brightness = FUJITSU_LCD_N_LEVELS;
get_lcd_level(device);
- error = fujitsu_backlight_register(device);
- if (error)
- return error;
+ ret = fujitsu_backlight_register(device);
+ if (ret)
+ return ret;
return 0;
}
@@ -693,7 +693,7 @@ 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;
+ int ret;
if (call_fext_func(device,
FUNC_LEDS, 0x0, 0x0, 0x0) & LOGOLAMP_POWERON) {
@@ -704,9 +704,9 @@ static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
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;
+ ret = devm_led_classdev_register(&device->dev, led);
+ if (ret)
+ return ret;
}
if ((call_fext_func(device,
@@ -719,9 +719,9 @@ static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
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;
+ ret = devm_led_classdev_register(&device->dev, led);
+ if (ret)
+ return ret;
}
/*
@@ -742,9 +742,9 @@ static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
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;
+ ret = devm_led_classdev_register(&device->dev, led);
+ if (ret)
+ return ret;
}
/* Support for eco led is not always signaled in bit corresponding
@@ -762,9 +762,9 @@ static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
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;
+ ret = devm_led_classdev_register(&device->dev, led);
+ if (ret)
+ return ret;
}
return 0;
@@ -773,8 +773,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 error;
- int i;
+ int ret, i = 0;
priv = devm_kzalloc(&device->dev, sizeof(*priv), GFP_KERNEL);
if (!priv)
@@ -789,23 +788,22 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
/* kfifo */
spin_lock_init(&priv->fifo_lock);
- error = kfifo_alloc(&priv->fifo, RINGBUFFERSIZE * sizeof(int),
- GFP_KERNEL);
- if (error) {
+ ret = kfifo_alloc(&priv->fifo, RINGBUFFERSIZE * sizeof(int),
+ GFP_KERNEL);
+ if (ret) {
pr_err("kfifo_alloc failed\n");
goto err_stop;
}
- error = acpi_fujitsu_laptop_input_setup(device);
- if (error)
+ ret = acpi_fujitsu_laptop_input_setup(device);
+ if (ret)
goto err_free_fifo;
pr_info("ACPI: %s [%s]\n",
acpi_device_name(device), acpi_device_bid(device));
- i = 0;
- while (call_fext_func(device, FUNC_BUTTONS, 0x1, 0x0, 0x0) != 0
- && (i++) < MAX_HOTKEY_RINGBUFFER_SIZE)
+ while (call_fext_func(device, FUNC_BUTTONS, 0x1, 0x0, 0x0) != 0 &&
+ i++ < MAX_HOTKEY_RINGBUFFER_SIZE)
; /* No action, result is discarded */
acpi_handle_debug(device->handle, "Discarded %i ringbuffer entries\n",
i);
@@ -835,12 +833,12 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
fujitsu_bl->bl_device->props.power = FB_BLANK_UNBLANK;
}
- error = acpi_fujitsu_laptop_leds_register(device);
- if (error)
+ ret = acpi_fujitsu_laptop_leds_register(device);
+ if (ret)
goto err_free_fifo;
- error = fujitsu_laptop_platform_add(device);
- if (error)
+ ret = fujitsu_laptop_platform_add(device);
+ if (ret)
goto err_free_fifo;
return 0;
@@ -848,7 +846,7 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
err_free_fifo:
kfifo_free(&priv->fifo);
err_stop:
- return error;
+ return ret;
}
static int acpi_fujitsu_laptop_remove(struct acpi_device *device)
@@ -865,11 +863,11 @@ static int acpi_fujitsu_laptop_remove(struct acpi_device *device)
static void acpi_fujitsu_laptop_press(struct acpi_device *device, int scancode)
{
struct fujitsu_laptop *priv = acpi_driver_data(device);
- int status;
+ int ret;
- status = kfifo_in_locked(&priv->fifo, (unsigned char *)&scancode,
- sizeof(scancode), &priv->fifo_lock);
- if (status != sizeof(scancode)) {
+ ret = kfifo_in_locked(&priv->fifo, (unsigned char *)&scancode,
+ sizeof(scancode), &priv->fifo_lock);
+ if (ret != sizeof(scancode)) {
dev_info(&priv->input->dev, "Could not push scancode [0x%x]\n",
scancode);
return;
@@ -882,13 +880,12 @@ static void acpi_fujitsu_laptop_press(struct acpi_device *device, int scancode)
static void acpi_fujitsu_laptop_release(struct acpi_device *device)
{
struct fujitsu_laptop *priv = acpi_driver_data(device);
- int scancode, status;
+ int scancode, ret;
while (true) {
- status = kfifo_out_locked(&priv->fifo,
- (unsigned char *)&scancode,
- sizeof(scancode), &priv->fifo_lock);
- if (status != sizeof(scancode))
+ ret = kfifo_out_locked(&priv->fifo, (unsigned char *)&scancode,
+ sizeof(scancode), &priv->fifo_lock);
+ if (ret != sizeof(scancode))
return;
sparse_keymap_report_event(priv->input, scancode, 0, false);
dev_dbg(&priv->input->dev,
--
2.16.1
On 02/11/18 13:07, Michał Kępień wrote:
> Align all constant values defined in the module to a common indentation.
> Rename ACPI_FUJITSU_NOTIFY_CODE1 to ACPI_FUJITSU_NOTIFY_CODE as there is
> only one ACPI notification code used throughout the driver. Define all
> bitmasks using the BIT() macro. Clean up comments.
>
> Signed-off-by: Michał Kępień <[email protected]>
> ---
> drivers/platform/x86/fujitsu-laptop.c | 65 ++++++++++++++++++-----------------
> 1 file changed, 33 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> index 0ee03d1fcbc4..07c713d9d273 100644
> --- a/drivers/platform/x86/fujitsu-laptop.c
> +++ b/drivers/platform/x86/fujitsu-laptop.c
> @@ -63,9 +63,9 @@
> #include <linux/platform_device.h>
> #include <acpi/video.h>
Hi,
It looks like this driver needs to #include <linux/bitops.h>
for use of BIT() macros.
See Documentation/process/submit-checklist.rst:
1) If you use a facility then #include the file that defines/declares
that facility. Don't depend on other header files pulling in ones
that you use.
Some $ARCH may work without it while another one may not.
> -#define FUJITSU_DRIVER_VERSION "0.6.0"
> +#define FUJITSU_DRIVER_VERSION "0.6.0"
>
> -#define FUJITSU_LCD_N_LEVELS 8
> +#define FUJITSU_LCD_N_LEVELS 8
>
> #define ACPI_FUJITSU_CLASS "fujitsu"
> #define ACPI_FUJITSU_BL_HID "FUJ02B1"
> @@ -75,46 +75,47 @@
> #define ACPI_FUJITSU_LAPTOP_DRIVER_NAME "Fujitsu laptop FUJ02E3 ACPI hotkeys driver"
> #define ACPI_FUJITSU_LAPTOP_DEVICE_NAME "Fujitsu FUJ02E3"
>
> -#define ACPI_FUJITSU_NOTIFY_CODE1 0x80
> +#define ACPI_FUJITSU_NOTIFY_CODE 0x80
>
> /* FUNC interface - command values */
> -#define FUNC_FLAGS 0x1000
> -#define FUNC_LEDS 0x1001
> -#define FUNC_BUTTONS 0x1002
> -#define FUNC_BACKLIGHT 0x1004
> +#define FUNC_FLAGS BIT(12)
> +#define FUNC_LEDS (BIT(12) | BIT(0))
> +#define FUNC_BUTTONS (BIT(12) | BIT(1))
> +#define FUNC_BACKLIGHT (BIT(12) | BIT(2))
>
> /* FUNC interface - responses */
> -#define UNSUPPORTED_CMD 0x80000000
> +#define UNSUPPORTED_CMD BIT(31)
>
> /* FUNC interface - status flags */
> -#define FLAG_RFKILL 0x020
> -#define FLAG_LID 0x100
> -#define FLAG_DOCK 0x200
> +#define FLAG_RFKILL BIT(5)
> +#define FLAG_LID BIT(8)
> +#define FLAG_DOCK BIT(9)
>
> /* FUNC interface - LED control */
> -#define FUNC_LED_OFF 0x1
> -#define FUNC_LED_ON 0x30001
> -#define KEYBOARD_LAMPS 0x100
> -#define LOGOLAMP_POWERON 0x2000
> -#define LOGOLAMP_ALWAYS 0x4000
> -#define RADIO_LED_ON 0x20
> -#define ECO_LED 0x10000
> -#define ECO_LED_ON 0x80000
> +#define FUNC_LED_OFF BIT(0)
> +#define FUNC_LED_ON (BIT(0) | BIT(16) | BIT(17))
> +#define LOGOLAMP_POWERON BIT(13)
> +#define LOGOLAMP_ALWAYS BIT(14)
> +#define KEYBOARD_LAMPS BIT(8)
> +#define RADIO_LED_ON BIT(5)
> +#define ECO_LED BIT(16)
> +#define ECO_LED_ON BIT(19)
>
> /* FUNC interface - backlight power control */
> -#define BACKLIGHT_POWER 0x4
> -#define BACKLIGHT_OFF 0x3
> -#define BACKLIGHT_ON 0x0
> +#define BACKLIGHT_POWER BIT(2)
> +#define BACKLIGHT_OFF (BIT(0) | BIT(1))
> +#define BACKLIGHT_ON 0
>
> -/* Hotkey details */
> -#define KEY1_CODE 0x410 /* codes for the keys in the GIRB register */
> -#define KEY2_CODE 0x411
> -#define KEY3_CODE 0x412
> -#define KEY4_CODE 0x413
> -#define KEY5_CODE 0x420
> +/* Scancodes read from the GIRB register */
> +#define KEY1_CODE 0x410
> +#define KEY2_CODE 0x411
> +#define KEY3_CODE 0x412
> +#define KEY4_CODE 0x413
> +#define KEY5_CODE 0x420
>
> -#define MAX_HOTKEY_RINGBUFFER_SIZE 100
> -#define RINGBUFFERSIZE 40
> +/* Hotkey ringbuffer limits */
> +#define MAX_HOTKEY_RINGBUFFER_SIZE 100
> +#define RINGBUFFERSIZE 40
>
> /* Module parameters */
> static int use_alt_lcd_levels = -1;
> @@ -428,7 +429,7 @@ static void acpi_fujitsu_bl_notify(struct acpi_device *device, u32 event)
> struct fujitsu_bl *priv = acpi_driver_data(device);
> int oldb, newb;
>
> - if (event != ACPI_FUJITSU_NOTIFY_CODE1) {
> + if (event != ACPI_FUJITSU_NOTIFY_CODE) {
> acpi_handle_info(device->handle, "unsupported event [0x%x]\n",
> event);
> sparse_keymap_report_event(priv->input, -1, 1, true);
> @@ -902,7 +903,7 @@ static void acpi_fujitsu_laptop_notify(struct acpi_device *device, u32 event)
> int scancode, i = 0;
> unsigned int irb;
>
> - if (event != ACPI_FUJITSU_NOTIFY_CODE1) {
> + if (event != ACPI_FUJITSU_NOTIFY_CODE) {
> acpi_handle_info(device->handle, "Unsupported event [0x%x]\n",
> event);
> sparse_keymap_report_event(priv->input, -1, 1, true);
>
--
~Randy
> Hi,
>
> It looks like this driver needs to #include <linux/bitops.h>
> for use of BIT() macros.
>
> See Documentation/process/submit-checklist.rst:
> 1) If you use a facility then #include the file that defines/declares
> that facility. Don't depend on other header files pulling in ones
> that you use.
>
> Some $ARCH may work without it while another one may not.
Noted, thanks!
--
Best regards,
Michał Kępień
On Sun, Feb 11, 2018 at 10:07:20PM +0100, Micha?? K??pie?? wrote:
> The patch series contains miscellaneous cleanups which I think are worth
> getting done before splitting fujitsu-laptop into two separate modules.
> I am not 100% sure that all the changes in the last patch in this series
> actually help, so please speak your mind.
With the relatively minor comments about patch 6 I've posted about
separately, I'm happy with this patch set. Collectively these changes
improve the readability of the code and make its intent clearer.
> This patch series was tested on a Lifebook S7020. AFAICT it does not
> conflict with the recent draft patch from Jan-Marek Glogowski and may
> thus be applied independently.
I agree.
I'm interested to hear your thoughts in connection with my queries about
patch 6 before merging. That aside, I have no issues with this series.
Reviewed-by: Jonathan Woithe <[email protected]>
Regards
jonathan
On Sun, Feb 11, 2018 at 10:07:26PM +0100, Micha?? K??pie?? wrote:
> Use named constants instead of integers in call_fext_func() invocations
> related to backlight power control in order to more clearly convey the
> intent of each call.
>
> Signed-off-by: Micha?? K??pie?? <[email protected]>
> ---
[cut]
> +/* FUNC interface - backlight power control */
> +#define BACKLIGHT_POWER 0x4
> +#define BACKLIGHT_OFF 0x3
> +#define BACKLIGHT_ON 0x0
A minor detail: BACKLIGHT_OFF and BACKLIGHT_ON are potential parameter
values while BACKLIGHT_POWER is essentially a parameter selector. Should
the name of BACKLIGHT_POWER reflect this difference? It could be difficult
to do without making the resulting identifier a little long. The best I can
come up with is BACKLIGHT_PARAM_POWER or (if a saving of one character is
desired) BACKLIGHT_PARM_POWER.
[cut]
> @@ -818,7 +825,8 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
> /* Sync backlight power status */
> if (fujitsu_bl && fujitsu_bl->bl_device &&
> acpi_video_get_backlight_type() == acpi_backlight_vendor) {
> - if (call_fext_func(fext, FUNC_BACKLIGHT, 0x2, 0x4, 0x0) == 3)
> + if (call_fext_func(fext, FUNC_BACKLIGHT, 0x2, BACKLIGHT_POWER,
> + 0x0) == 3)
> fujitsu_bl->bl_device->props.power = FB_BLANK_POWERDOWN;
> else
> fujitsu_bl->bl_device->props.power = FB_BLANK_UNBLANK;
I'm curious: this fext function call is, I think, returning the current
backlight power value. If that's the case, is the value of 3 used in the
test of the return function conceptually BACKLIGHT_OFF, and if so, should we
use BACKLIGHT_OFF instead of the numeric constant? It seems likely given
that it results in the backlight power property being set to
FB_BLANK_POWERDOWN.
Regards
jonathan
On Sun, Feb 11, 2018 at 10:07:20PM +0100, Michał Kępień wrote:
> This is the second of the two patch series I started preparing back in
> June 2017 [1]. It took me this long to post it purely due to permanent
> spare time shortage, not because the changes are complicated.
>
> The patch series contains miscellaneous cleanups which I think are worth
> getting done before splitting fujitsu-laptop into two separate modules.
> I am not 100% sure that all the changes in the last patch in this series
> actually help, so please speak your mind.
>
> This patch series was tested on a Lifebook S7020. AFAICT it does not
> conflict with the recent draft patch from Jan-Marek Glogowski and may
> thus be applied independently.
>
> Finally, please forgive me if it takes me weeks or months to address
> review comments. It is also perfectly fine for reviews to take weeks or
> months ;)
>
To avoid just having to review everything again in a few months ;-) I've queued
up patches 1-5. I'll await comments to 6 and a respin of 7 based on feedback.
Thanks,
--
Darren Hart
VMware Open Source Technology Center
> > +/* FUNC interface - backlight power control */
> > +#define BACKLIGHT_POWER 0x4
> > +#define BACKLIGHT_OFF 0x3
> > +#define BACKLIGHT_ON 0x0
>
> A minor detail: BACKLIGHT_OFF and BACKLIGHT_ON are potential parameter
> values while BACKLIGHT_POWER is essentially a parameter selector. Should
> the name of BACKLIGHT_POWER reflect this difference? It could be difficult
> to do without making the resulting identifier a little long. The best I can
> come up with is BACKLIGHT_PARAM_POWER or (if a saving of one character is
> desired) BACKLIGHT_PARM_POWER.
So, I tried to somehow unify constant naming throughout the module a few
times by now, but it seems that whichever naming pattern I chose, there
was always some exception to the rule. Of course the module code is not
to blame, it is caused by the firmware treating logically related
features (like controlling various LEDs) in diverse ways.
Thus, I am perfectly fine with using BACKLIGHT_PARAM_POWER for now,
because I do not have a better idea yet. If I ever come up with a
constant naming scheme that would cover all the constants in the module
(or at least all those directly related to call_fext_func()), I will
propose it here.
> > @@ -818,7 +825,8 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
> > /* Sync backlight power status */
> > if (fujitsu_bl && fujitsu_bl->bl_device &&
> > acpi_video_get_backlight_type() == acpi_backlight_vendor) {
> > - if (call_fext_func(fext, FUNC_BACKLIGHT, 0x2, 0x4, 0x0) == 3)
> > + if (call_fext_func(fext, FUNC_BACKLIGHT, 0x2, BACKLIGHT_POWER,
> > + 0x0) == 3)
> > fujitsu_bl->bl_device->props.power = FB_BLANK_POWERDOWN;
> > else
> > fujitsu_bl->bl_device->props.power = FB_BLANK_UNBLANK;
>
> I'm curious: this fext function call is, I think, returning the current
> backlight power value. If that's the case, is the value of 3 used in the
> test of the return function conceptually BACKLIGHT_OFF, and if so, should we
> use BACKLIGHT_OFF instead of the numeric constant? It seems likely given
> that it results in the backlight power property being set to
> FB_BLANK_POWERDOWN.
Ah, yes, good catch. Will fix, thanks.
--
Best regards,
Michał Kępień
> To avoid just having to review everything again in a few months ;-) I've queued
> up patches 1-5. I'll await comments to 6 and a respin of 7 based on feedback.
Thanks, I think I will post v2 of the entire series with changes
addressing review comments for patches 6/7 and 7/7. I think that is the
most coherent way of moving forward, but if you would rather see fixed
versions of patches 6/7 and 7/7 posted as 1/2 and 2/2, please just let
me know.
--
Best regards,
Michał Kępień
On Sun, Feb 18, 2018 at 08:55:09PM +0100, Michał Kępień wrote:
> > To avoid just having to review everything again in a few months ;-) I've queued
> > up patches 1-5. I'll await comments to 6 and a respin of 7 based on feedback.
>
> Thanks, I think I will post v2 of the entire series with changes
> addressing review comments for patches 6/7 and 7/7. I think that is the
> most coherent way of moving forward, but if you would rather see fixed
> versions of patches 6/7 and 7/7 posted as 1/2 and 2/2, please just let
> me know.
Your cover letter made it easy to manage, thanks for being clear, makes it
easier on our end.
--
Darren Hart
VMware Open Source Technology Center