This series simplifies handling of both brightness key and hotkey input
events on Fujitsu laptops by making use of sparse keymaps. This not
only makes the driver shorter and, hopefully, cleaner, but also enables
us to get rid of the keycodeX fields inside struct fujitsu_bl, which
facilitates further cleanups. Also, to simplify error handling, input
devices registered by fujitsu-laptop are migrated to the devres API
along the way.
This series was tested on a Lifebook S7020 and a Lifebook E744.
This series depends on the platform cleanup series I posted last week.
While that series has not yet been merged into testing, Jonathan has
reviewed it and Darren also seemed to be okay with it, so I just assumed
it will get merged soon. I wanted to post this one as soon as possible
as it requires a bit more thorough review and testing compared to the
previous series I posted for fujitsu-laptop.
drivers/platform/x86/Kconfig | 1 +
drivers/platform/x86/fujitsu-laptop.c | 355 +++++++++++++++-------------------
2 files changed, 154 insertions(+), 202 deletions(-)
--
2.12.0
Use a managed input device for hotkey events in order to simplify two
error paths and one cleanup function while also reducing the number of
local variables required. Remove double assignment to make checkpatch
happy.
Signed-off-by: Michał Kępień <[email protected]>
---
drivers/platform/x86/fujitsu-laptop.c | 55 ++++++++++++-----------------------
1 file changed, 19 insertions(+), 36 deletions(-)
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index b1a08d83330b..74da588ed871 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -759,41 +759,29 @@ static void acpi_fujitsu_bl_notify(struct acpi_device *device, u32 event)
static int acpi_fujitsu_laptop_input_setup(struct acpi_device *device)
{
struct fujitsu_laptop *fujitsu_laptop = acpi_driver_data(device);
- struct input_dev *input;
- int error;
- fujitsu_laptop->input = input = input_allocate_device();
- if (!input)
+ fujitsu_laptop->input = devm_input_allocate_device(&device->dev);
+ if (!fujitsu_laptop->input)
return -ENOMEM;
snprintf(fujitsu_laptop->phys, sizeof(fujitsu_laptop->phys),
"%s/video/input0", acpi_device_hid(device));
- input->name = acpi_device_name(device);
- input->phys = fujitsu_laptop->phys;
- input->id.bustype = BUS_HOST;
- input->id.product = 0x06;
- input->dev.parent = &device->dev;
-
- set_bit(EV_KEY, input->evbit);
- set_bit(fujitsu_bl->keycode1, input->keybit);
- set_bit(fujitsu_bl->keycode2, input->keybit);
- set_bit(fujitsu_bl->keycode3, input->keybit);
- set_bit(fujitsu_bl->keycode4, input->keybit);
- set_bit(fujitsu_bl->keycode5, input->keybit);
- set_bit(KEY_TOUCHPAD_TOGGLE, input->keybit);
- set_bit(KEY_UNKNOWN, input->keybit);
-
- error = input_register_device(input);
- if (error)
- goto err_free_input_dev;
-
- return 0;
-
-err_free_input_dev:
- input_free_device(input);
-
- return error;
+ 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;
+
+ set_bit(EV_KEY, fujitsu_laptop->input->evbit);
+ set_bit(fujitsu_bl->keycode1, fujitsu_laptop->input->keybit);
+ set_bit(fujitsu_bl->keycode2, fujitsu_laptop->input->keybit);
+ set_bit(fujitsu_bl->keycode3, fujitsu_laptop->input->keybit);
+ set_bit(fujitsu_bl->keycode4, fujitsu_laptop->input->keybit);
+ set_bit(fujitsu_bl->keycode5, fujitsu_laptop->input->keybit);
+ set_bit(KEY_TOUCHPAD_TOGGLE, fujitsu_laptop->input->keybit);
+ set_bit(KEY_UNKNOWN, fujitsu_laptop->input->keybit);
+
+ return input_register_device(fujitsu_laptop->input);
}
static int fujitsu_laptop_platform_add(void)
@@ -862,7 +850,7 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
error = acpi_bus_update_power(fujitsu_laptop->acpi_handle, &state);
if (error) {
pr_err("Error reading power state\n");
- goto err_unregister_input_dev;
+ goto err_free_fifo;
}
pr_info("ACPI: %s [%s] (%s)\n",
@@ -911,7 +899,7 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
error = fujitsu_laptop_platform_add();
if (error)
- goto err_unregister_input_dev;
+ goto err_free_fifo;
#if IS_ENABLED(CONFIG_LEDS_CLASS)
if (call_fext_func(FUNC_LEDS, 0x0, 0x0, 0x0) & LOGOLAMP_POWERON) {
@@ -974,8 +962,6 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
return result;
-err_unregister_input_dev:
- input_unregister_device(fujitsu_laptop->input);
err_free_fifo:
kfifo_free(&fujitsu_laptop->fifo);
err_stop:
@@ -985,7 +971,6 @@ 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 input_dev *input = fujitsu_laptop->input;
#if IS_ENABLED(CONFIG_LEDS_CLASS)
if (fujitsu_laptop->logolamp_registered)
@@ -1003,8 +988,6 @@ static int acpi_fujitsu_laptop_remove(struct acpi_device *device)
fujitsu_laptop_platform_remove();
- input_unregister_device(input);
-
kfifo_free(&fujitsu_laptop->fifo);
fujitsu_laptop->acpi_handle = NULL;
--
2.12.0
Some laptop models need to have different keycodes assigned to hotkey
scancodes. Change the sparse keymap upon a DMI match, before the hotkey
input device is setup.
Instead of using three different callbacks in the DMI match table,
simplify code by using the driver_data field of struct dmi_system_id to
supply the requested keymap to a common callback. Also merge keymaps
for S6410 and S6420 as they are identical.
Rename fujitsu_dmi_table to fujitsu_laptop_dmi_table to emphasize it is
no longer used by the backlight part of fujitsu-laptop. Adjust
whitespace to make checkpatch happy.
Signed-off-by: Michał Kępień <[email protected]>
---
drivers/platform/x86/fujitsu-laptop.c | 111 +++++++++++++++++-----------------
1 file changed, 55 insertions(+), 56 deletions(-)
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 8f1c9c204110..1487eb2396c6 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -534,61 +534,6 @@ static struct platform_driver fujitsu_pf_driver = {
}
};
-static void __init dmi_check_cb_common(const struct dmi_system_id *id)
-{
- pr_info("Identified laptop model '%s'\n", id->ident);
-}
-
-static int __init dmi_check_cb_s6410(const struct dmi_system_id *id)
-{
- dmi_check_cb_common(id);
- fujitsu_bl->keycode1 = KEY_SCREENLOCK; /* "Lock" */
- fujitsu_bl->keycode2 = KEY_HELP; /* "Mobility Center" */
- return 1;
-}
-
-static int __init dmi_check_cb_s6420(const struct dmi_system_id *id)
-{
- dmi_check_cb_common(id);
- fujitsu_bl->keycode1 = KEY_SCREENLOCK; /* "Lock" */
- fujitsu_bl->keycode2 = KEY_HELP; /* "Mobility Center" */
- return 1;
-}
-
-static int __init dmi_check_cb_p8010(const struct dmi_system_id *id)
-{
- dmi_check_cb_common(id);
- fujitsu_bl->keycode1 = KEY_HELP; /* "Support" */
- fujitsu_bl->keycode3 = KEY_SWITCHVIDEOMODE; /* "Presentation" */
- fujitsu_bl->keycode4 = KEY_WWW; /* "Internet" */
- return 1;
-}
-
-static const struct dmi_system_id fujitsu_dmi_table[] __initconst = {
- {
- .ident = "Fujitsu Siemens S6410",
- .matches = {
- DMI_MATCH(DMI_SYS_VENDOR, "FUJITSU SIEMENS"),
- DMI_MATCH(DMI_PRODUCT_NAME, "LIFEBOOK S6410"),
- },
- .callback = dmi_check_cb_s6410},
- {
- .ident = "Fujitsu Siemens S6420",
- .matches = {
- DMI_MATCH(DMI_SYS_VENDOR, "FUJITSU SIEMENS"),
- DMI_MATCH(DMI_PRODUCT_NAME, "LIFEBOOK S6420"),
- },
- .callback = dmi_check_cb_s6420},
- {
- .ident = "Fujitsu LifeBook P8010",
- .matches = {
- DMI_MATCH(DMI_SYS_VENDOR, "FUJITSU"),
- DMI_MATCH(DMI_PRODUCT_NAME, "LifeBook P8010"),
- },
- .callback = dmi_check_cb_p8010},
- {}
-};
-
/* ACPI device for LCD brightness control */
static const struct key_entry keymap_backlight[] = {
@@ -766,8 +711,62 @@ static const struct key_entry keymap_default[] = {
{ KE_END, 0 }
};
+static const struct key_entry keymap_s6400[] = {
+ { KE_KEY, KEY1_CODE, { KEY_SCREENLOCK } }, /* "Lock" */
+ { KE_KEY, KEY2_CODE, { KEY_HELP } }, /* "Mobility Center */
+ { KE_KEY, KEY3_CODE, { KEY_PROG3 } },
+ { KE_KEY, KEY4_CODE, { KEY_PROG4 } },
+ { KE_END, 0 }
+};
+
+static const struct key_entry keymap_p8010[] = {
+ { KE_KEY, KEY1_CODE, { KEY_HELP } }, /* "Support" */
+ { KE_KEY, KEY2_CODE, { KEY_PROG2 } },
+ { KE_KEY, KEY3_CODE, { KEY_SWITCHVIDEOMODE } }, /* "Presentation" */
+ { KE_KEY, KEY4_CODE, { KEY_WWW } }, /* "WWW" */
+ { KE_END, 0 }
+};
+
static const struct key_entry *keymap = keymap_default;
+static int fujitsu_laptop_dmi_keymap_override(const struct dmi_system_id *id)
+{
+ pr_info("Identified laptop model '%s'\n", id->ident);
+ keymap = id->driver_data;
+ return 1;
+}
+
+static const struct dmi_system_id fujitsu_laptop_dmi_table[] = {
+ {
+ .callback = fujitsu_laptop_dmi_keymap_override,
+ .ident = "Fujitsu Siemens S6410",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "FUJITSU SIEMENS"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "LIFEBOOK S6410"),
+ },
+ .driver_data = (void *)keymap_s6400
+ },
+ {
+ .callback = fujitsu_laptop_dmi_keymap_override,
+ .ident = "Fujitsu Siemens S6420",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "FUJITSU SIEMENS"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "LIFEBOOK S6420"),
+ },
+ .driver_data = (void *)keymap_s6400
+ },
+ {
+ .callback = fujitsu_laptop_dmi_keymap_override,
+ .ident = "Fujitsu LifeBook P8010",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "FUJITSU"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "LifeBook P8010"),
+ },
+ .driver_data = (void *)keymap_p8010
+ },
+ {}
+};
+
static int acpi_fujitsu_laptop_input_setup(struct acpi_device *device)
{
struct fujitsu_laptop *fujitsu_laptop = acpi_driver_data(device);
@@ -785,6 +784,7 @@ static int acpi_fujitsu_laptop_input_setup(struct acpi_device *device)
fujitsu_laptop->input->id.bustype = BUS_HOST;
fujitsu_laptop->input->id.product = 0x06;
+ dmi_check_system(fujitsu_laptop_dmi_table);
ret = sparse_keymap_setup(fujitsu_laptop->input, keymap, NULL);
if (ret)
return ret;
@@ -1135,7 +1135,6 @@ static int __init fujitsu_init(void)
fujitsu_bl->keycode3 = KEY_PROG3;
fujitsu_bl->keycode4 = KEY_PROG4;
fujitsu_bl->keycode5 = KEY_RFKILL;
- dmi_check_system(fujitsu_dmi_table);
ret = acpi_bus_register_driver(&acpi_fujitsu_bl_driver);
if (ret)
--
2.12.0
Remove the keycode[1-5] fields from struct fujitsu_bl as they are not
needed any more as a result of the sparse keymap migration.
Signed-off-by: Michał Kępień <[email protected]>
---
drivers/platform/x86/fujitsu-laptop.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 1487eb2396c6..34c913e621b6 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -139,7 +139,6 @@ struct fujitsu_bl {
struct input_dev *input;
char phys[32];
struct backlight_device *bl_device;
- int keycode1, keycode2, keycode3, keycode4, keycode5;
unsigned int max_brightness;
unsigned int brightness_changed;
@@ -1130,11 +1129,6 @@ static int __init fujitsu_init(void)
fujitsu_bl = kzalloc(sizeof(struct fujitsu_bl), GFP_KERNEL);
if (!fujitsu_bl)
return -ENOMEM;
- fujitsu_bl->keycode1 = KEY_PROG1;
- fujitsu_bl->keycode2 = KEY_PROG2;
- fujitsu_bl->keycode3 = KEY_PROG3;
- fujitsu_bl->keycode4 = KEY_PROG4;
- fujitsu_bl->keycode5 = KEY_RFKILL;
ret = acpi_bus_register_driver(&acpi_fujitsu_bl_driver);
if (ret)
--
2.12.0
Simplify hotkey event generation by using a sparse keymap. As sparse
keymap operates on scancodes instead of keycodes, adjust variable names
and debug messages accordingly.
This patch only handles the default keymap for clarity.
Signed-off-by: Michał Kępień <[email protected]>
---
drivers/platform/x86/fujitsu-laptop.c | 111 +++++++++++++---------------------
1 file changed, 41 insertions(+), 70 deletions(-)
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 74da588ed871..8f1c9c204110 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -756,9 +756,22 @@ static void acpi_fujitsu_bl_notify(struct acpi_device *device, u32 event)
/* ACPI device for hotkey handling */
+static const struct key_entry keymap_default[] = {
+ { KE_KEY, KEY1_CODE, { KEY_PROG1 } },
+ { KE_KEY, KEY2_CODE, { KEY_PROG2 } },
+ { KE_KEY, KEY3_CODE, { KEY_PROG3 } },
+ { KE_KEY, KEY4_CODE, { KEY_PROG4 } },
+ { KE_KEY, KEY5_CODE, { KEY_RFKILL } },
+ { KE_KEY, BIT(26), { KEY_TOUCHPAD_TOGGLE } },
+ { KE_END, 0 }
+};
+
+static const struct key_entry *keymap = keymap_default;
+
static int acpi_fujitsu_laptop_input_setup(struct acpi_device *device)
{
struct fujitsu_laptop *fujitsu_laptop = acpi_driver_data(device);
+ int ret;
fujitsu_laptop->input = devm_input_allocate_device(&device->dev);
if (!fujitsu_laptop->input)
@@ -772,14 +785,9 @@ static int acpi_fujitsu_laptop_input_setup(struct acpi_device *device)
fujitsu_laptop->input->id.bustype = BUS_HOST;
fujitsu_laptop->input->id.product = 0x06;
- set_bit(EV_KEY, fujitsu_laptop->input->evbit);
- set_bit(fujitsu_bl->keycode1, fujitsu_laptop->input->keybit);
- set_bit(fujitsu_bl->keycode2, fujitsu_laptop->input->keybit);
- set_bit(fujitsu_bl->keycode3, fujitsu_laptop->input->keybit);
- set_bit(fujitsu_bl->keycode4, fujitsu_laptop->input->keybit);
- set_bit(fujitsu_bl->keycode5, fujitsu_laptop->input->keybit);
- set_bit(KEY_TOUCHPAD_TOGGLE, fujitsu_laptop->input->keybit);
- set_bit(KEY_UNKNOWN, fujitsu_laptop->input->keybit);
+ ret = sparse_keymap_setup(fujitsu_laptop->input, keymap, NULL);
+ if (ret)
+ return ret;
return input_register_device(fujitsu_laptop->input);
}
@@ -995,61 +1003,54 @@ static int acpi_fujitsu_laptop_remove(struct acpi_device *device)
return 0;
}
-static void acpi_fujitsu_laptop_press(int keycode)
+static void acpi_fujitsu_laptop_press(int scancode)
{
struct input_dev *input = fujitsu_laptop->input;
int status;
status = kfifo_in_locked(&fujitsu_laptop->fifo,
- (unsigned char *)&keycode, sizeof(keycode),
+ (unsigned char *)&scancode, sizeof(scancode),
&fujitsu_laptop->fifo_lock);
- if (status != sizeof(keycode)) {
+ if (status != sizeof(scancode)) {
vdbg_printk(FUJLAPTOP_DBG_WARN,
- "Could not push keycode [0x%x]\n", keycode);
+ "Could not push scancode [0x%x]\n", scancode);
return;
}
- input_report_key(input, keycode, 1);
- input_sync(input);
+ sparse_keymap_report_event(input, scancode, 1, false);
vdbg_printk(FUJLAPTOP_DBG_TRACE,
- "Push keycode into ringbuffer [%d]\n", keycode);
+ "Push scancode into ringbuffer [0x%x]\n", scancode);
}
static void acpi_fujitsu_laptop_release(void)
{
struct input_dev *input = fujitsu_laptop->input;
- int keycode, status;
+ int scancode, status;
while (true) {
status = kfifo_out_locked(&fujitsu_laptop->fifo,
- (unsigned char *)&keycode,
- sizeof(keycode),
+ (unsigned char *)&scancode,
+ sizeof(scancode),
&fujitsu_laptop->fifo_lock);
- if (status != sizeof(keycode))
+ if (status != sizeof(scancode))
return;
- input_report_key(input, keycode, 0);
- input_sync(input);
+ sparse_keymap_report_event(input, scancode, 0, false);
vdbg_printk(FUJLAPTOP_DBG_TRACE,
- "Pop keycode from ringbuffer [%d]\n", keycode);
+ "Pop scancode from ringbuffer [0x%x]\n", scancode);
}
}
static void acpi_fujitsu_laptop_notify(struct acpi_device *device, u32 event)
{
struct input_dev *input;
- int keycode;
- unsigned int irb = 1;
- int i;
+ int scancode, i = 0;
+ unsigned int irb;
input = fujitsu_laptop->input;
if (event != ACPI_FUJITSU_NOTIFY_CODE1) {
- keycode = KEY_UNKNOWN;
vdbg_printk(FUJLAPTOP_DBG_WARN,
"Unsupported event [0x%x]\n", event);
- input_report_key(input, keycode, 1);
- input_sync(input);
- input_report_key(input, keycode, 0);
- input_sync(input);
+ sparse_keymap_report_event(input, -1, 1, true);
return;
}
@@ -1057,40 +1058,16 @@ static void acpi_fujitsu_laptop_notify(struct acpi_device *device, u32 event)
fujitsu_laptop->flags_state =
call_fext_func(FUNC_FLAGS, 0x4, 0x0, 0x0);
- i = 0;
- while ((irb =
- call_fext_func(FUNC_BUTTONS, 0x1, 0x0, 0x0)) != 0
- && (i++) < MAX_HOTKEY_RINGBUFFER_SIZE) {
- switch (irb & 0x4ff) {
- case KEY1_CODE:
- keycode = fujitsu_bl->keycode1;
- break;
- case KEY2_CODE:
- keycode = fujitsu_bl->keycode2;
- break;
- case KEY3_CODE:
- keycode = fujitsu_bl->keycode3;
- break;
- case KEY4_CODE:
- keycode = fujitsu_bl->keycode4;
- break;
- case KEY5_CODE:
- keycode = fujitsu_bl->keycode5;
- break;
- case 0:
- keycode = 0;
- break;
- default:
+ while ((irb = call_fext_func(FUNC_BUTTONS, 0x1, 0x0, 0x0)) != 0 &&
+ i++ < MAX_HOTKEY_RINGBUFFER_SIZE) {
+ scancode = irb & 0x4ff;
+ if (sparse_keymap_entry_from_scancode(input, scancode))
+ acpi_fujitsu_laptop_press(scancode);
+ else if (scancode == 0)
+ acpi_fujitsu_laptop_release();
+ else
vdbg_printk(FUJLAPTOP_DBG_WARN,
"Unknown GIRB result [%x]\n", irb);
- keycode = -1;
- break;
- }
-
- if (keycode > 0)
- acpi_fujitsu_laptop_press(keycode);
- else if (keycode == 0)
- acpi_fujitsu_laptop_release();
}
/* On some models (first seen on the Skylake-based Lifebook
@@ -1098,14 +1075,8 @@ 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))) {
- keycode = KEY_TOUCHPAD_TOGGLE;
- input_report_key(input, keycode, 1);
- input_sync(input);
- input_report_key(input, keycode, 0);
- input_sync(input);
- }
-
+ (call_fext_func(FLAG_RFKILL, 0x1, 0x0, 0x0) & BIT(26)))
+ sparse_keymap_report_event(input, BIT(26), 1, true);
}
/* Initialization */
--
2.12.0
Simplify error handling in acpi_fujitsu_bl_add() by moving code
responsible for setting up the input device to a separate function.
Signed-off-by: Michał Kępień <[email protected]>
---
drivers/platform/x86/fujitsu-laptop.c | 64 +++++++++++++++++++++--------------
1 file changed, 38 insertions(+), 26 deletions(-)
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index f3ccef3d5a1e..2b0dcf989e2a 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -590,6 +590,41 @@ static const struct dmi_system_id fujitsu_dmi_table[] __initconst = {
/* ACPI device for LCD brightness control */
+static int acpi_fujitsu_bl_input_setup(struct acpi_device *device)
+{
+ struct fujitsu_bl *fujitsu_bl = acpi_driver_data(device);
+ struct input_dev *input;
+ int error;
+
+ fujitsu_bl->input = input = input_allocate_device();
+ if (!input)
+ return -ENOMEM;
+
+ snprintf(fujitsu_bl->phys, sizeof(fujitsu_bl->phys),
+ "%s/video/input0", acpi_device_hid(device));
+
+ input->name = acpi_device_name(device);
+ input->phys = fujitsu_bl->phys;
+ input->id.bustype = BUS_HOST;
+ input->id.product = 0x06;
+ input->dev.parent = &device->dev;
+ input->evbit[0] = BIT(EV_KEY);
+ set_bit(KEY_BRIGHTNESSUP, input->keybit);
+ set_bit(KEY_BRIGHTNESSDOWN, input->keybit);
+ set_bit(KEY_UNKNOWN, input->keybit);
+
+ error = input_register_device(input);
+ if (error)
+ goto err_free_input_dev;
+
+ return 0;
+
+err_free_input_dev:
+ input_free_device(input);
+
+ return error;
+}
+
static int fujitsu_backlight_register(void)
{
struct backlight_properties props = {
@@ -612,7 +647,6 @@ static int fujitsu_backlight_register(void)
static int acpi_fujitsu_bl_add(struct acpi_device *device)
{
int state = 0;
- struct input_dev *input;
int error;
if (!device)
@@ -623,28 +657,9 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device)
sprintf(acpi_device_class(device), "%s", ACPI_FUJITSU_CLASS);
device->driver_data = fujitsu_bl;
- fujitsu_bl->input = input = input_allocate_device();
- if (!input) {
- error = -ENOMEM;
- goto err_stop;
- }
-
- snprintf(fujitsu_bl->phys, sizeof(fujitsu_bl->phys),
- "%s/video/input0", acpi_device_hid(device));
-
- input->name = acpi_device_name(device);
- input->phys = fujitsu_bl->phys;
- input->id.bustype = BUS_HOST;
- input->id.product = 0x06;
- input->dev.parent = &device->dev;
- input->evbit[0] = BIT(EV_KEY);
- set_bit(KEY_BRIGHTNESSUP, input->keybit);
- set_bit(KEY_BRIGHTNESSDOWN, input->keybit);
- set_bit(KEY_UNKNOWN, input->keybit);
-
- error = input_register_device(input);
+ error = acpi_fujitsu_bl_input_setup(device);
if (error)
- goto err_free_input_dev;
+ goto err_stop;
error = acpi_bus_update_power(fujitsu_bl->acpi_handle, &state);
if (error) {
@@ -695,10 +710,7 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device)
return 0;
err_unregister_input_dev:
- input_unregister_device(input);
- input = NULL;
-err_free_input_dev:
- input_free_device(input);
+ input_unregister_device(fujitsu_bl->input);
err_stop:
return error;
}
--
2.12.0
Simplify error handling in acpi_fujitsu_laptop_add() by moving code
responsible for setting up the input device to a separate function.
Signed-off-by: Michał Kępień <[email protected]>
---
drivers/platform/x86/fujitsu-laptop.c | 74 ++++++++++++++++++++---------------
1 file changed, 43 insertions(+), 31 deletions(-)
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 3483ac37bee5..b1a08d83330b 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -756,6 +756,46 @@ static void acpi_fujitsu_bl_notify(struct acpi_device *device, u32 event)
/* ACPI device for hotkey handling */
+static int acpi_fujitsu_laptop_input_setup(struct acpi_device *device)
+{
+ struct fujitsu_laptop *fujitsu_laptop = acpi_driver_data(device);
+ struct input_dev *input;
+ int error;
+
+ fujitsu_laptop->input = input = input_allocate_device();
+ if (!input)
+ return -ENOMEM;
+
+ snprintf(fujitsu_laptop->phys, sizeof(fujitsu_laptop->phys),
+ "%s/video/input0", acpi_device_hid(device));
+
+ input->name = acpi_device_name(device);
+ input->phys = fujitsu_laptop->phys;
+ input->id.bustype = BUS_HOST;
+ input->id.product = 0x06;
+ input->dev.parent = &device->dev;
+
+ set_bit(EV_KEY, input->evbit);
+ set_bit(fujitsu_bl->keycode1, input->keybit);
+ set_bit(fujitsu_bl->keycode2, input->keybit);
+ set_bit(fujitsu_bl->keycode3, input->keybit);
+ set_bit(fujitsu_bl->keycode4, input->keybit);
+ set_bit(fujitsu_bl->keycode5, input->keybit);
+ set_bit(KEY_TOUCHPAD_TOGGLE, input->keybit);
+ set_bit(KEY_UNKNOWN, input->keybit);
+
+ error = input_register_device(input);
+ if (error)
+ goto err_free_input_dev;
+
+ return 0;
+
+err_free_input_dev:
+ input_free_device(input);
+
+ return error;
+}
+
static int fujitsu_laptop_platform_add(void)
{
int ret;
@@ -794,7 +834,6 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
{
int result = 0;
int state = 0;
- struct input_dev *input;
int error;
int i;
@@ -816,33 +855,9 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
goto err_stop;
}
- fujitsu_laptop->input = input = input_allocate_device();
- if (!input) {
- error = -ENOMEM;
- goto err_free_fifo;
- }
-
- snprintf(fujitsu_laptop->phys, sizeof(fujitsu_laptop->phys),
- "%s/video/input0", acpi_device_hid(device));
-
- input->name = acpi_device_name(device);
- input->phys = fujitsu_laptop->phys;
- input->id.bustype = BUS_HOST;
- input->id.product = 0x06;
- input->dev.parent = &device->dev;
-
- set_bit(EV_KEY, input->evbit);
- set_bit(fujitsu_bl->keycode1, input->keybit);
- set_bit(fujitsu_bl->keycode2, input->keybit);
- set_bit(fujitsu_bl->keycode3, input->keybit);
- set_bit(fujitsu_bl->keycode4, input->keybit);
- set_bit(fujitsu_bl->keycode5, input->keybit);
- set_bit(KEY_TOUCHPAD_TOGGLE, input->keybit);
- set_bit(KEY_UNKNOWN, input->keybit);
-
- error = input_register_device(input);
+ error = acpi_fujitsu_laptop_input_setup(device);
if (error)
- goto err_free_input_dev;
+ goto err_free_fifo;
error = acpi_bus_update_power(fujitsu_laptop->acpi_handle, &state);
if (error) {
@@ -960,10 +975,7 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
return result;
err_unregister_input_dev:
- input_unregister_device(input);
- input = NULL;
-err_free_input_dev:
- input_free_device(input);
+ input_unregister_device(fujitsu_laptop->input);
err_free_fifo:
kfifo_free(&fujitsu_laptop->fifo);
err_stop:
--
2.12.0
Use a managed input device for brightness events in order to simplify
two error paths and one cleanup function while also reducing the number
of local variables required. Remove double assignment to make
checkpatch happy.
Signed-off-by: Michał Kępień <[email protected]>
---
drivers/platform/x86/fujitsu-laptop.c | 47 +++++++++++------------------------
1 file changed, 14 insertions(+), 33 deletions(-)
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 2b0dcf989e2a..68e338c6a876 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -593,36 +593,24 @@ static const struct dmi_system_id fujitsu_dmi_table[] __initconst = {
static int acpi_fujitsu_bl_input_setup(struct acpi_device *device)
{
struct fujitsu_bl *fujitsu_bl = acpi_driver_data(device);
- struct input_dev *input;
- int error;
- fujitsu_bl->input = input = input_allocate_device();
- if (!input)
+ fujitsu_bl->input = devm_input_allocate_device(&device->dev);
+ if (!fujitsu_bl->input)
return -ENOMEM;
snprintf(fujitsu_bl->phys, sizeof(fujitsu_bl->phys),
"%s/video/input0", acpi_device_hid(device));
- input->name = acpi_device_name(device);
- input->phys = fujitsu_bl->phys;
- input->id.bustype = BUS_HOST;
- input->id.product = 0x06;
- input->dev.parent = &device->dev;
- input->evbit[0] = BIT(EV_KEY);
- set_bit(KEY_BRIGHTNESSUP, input->keybit);
- set_bit(KEY_BRIGHTNESSDOWN, input->keybit);
- set_bit(KEY_UNKNOWN, input->keybit);
-
- error = input_register_device(input);
- if (error)
- goto err_free_input_dev;
-
- return 0;
-
-err_free_input_dev:
- input_free_device(input);
+ 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;
+ fujitsu_bl->input->evbit[0] = BIT(EV_KEY);
+ set_bit(KEY_BRIGHTNESSUP, fujitsu_bl->input->keybit);
+ set_bit(KEY_BRIGHTNESSDOWN, fujitsu_bl->input->keybit);
+ set_bit(KEY_UNKNOWN, fujitsu_bl->input->keybit);
- return error;
+ return input_register_device(fujitsu_bl->input);
}
static int fujitsu_backlight_register(void)
@@ -659,12 +647,12 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device)
error = acpi_fujitsu_bl_input_setup(device);
if (error)
- goto err_stop;
+ return error;
error = acpi_bus_update_power(fujitsu_bl->acpi_handle, &state);
if (error) {
pr_err("Error reading power state\n");
- goto err_unregister_input_dev;
+ return error;
}
pr_info("ACPI: %s [%s] (%s)\n",
@@ -704,24 +692,17 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device)
if (acpi_video_get_backlight_type() == acpi_backlight_vendor) {
error = fujitsu_backlight_register();
if (error)
- goto err_unregister_input_dev;
+ return error;
}
return 0;
-
-err_unregister_input_dev:
- input_unregister_device(fujitsu_bl->input);
-err_stop:
- return error;
}
static int acpi_fujitsu_bl_remove(struct acpi_device *device)
{
struct fujitsu_bl *fujitsu_bl = acpi_driver_data(device);
- struct input_dev *input = fujitsu_bl->input;
backlight_device_unregister(fujitsu_bl->bl_device);
- input_unregister_device(input);
fujitsu_bl->acpi_handle = NULL;
--
2.12.0
Simplify brightness key event generation by using a sparse keymap.
Signed-off-by: Michał Kępień <[email protected]>
---
drivers/platform/x86/Kconfig | 1 +
drivers/platform/x86/fujitsu-laptop.c | 31 +++++++++++++++----------------
2 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 4bc88eb52712..f614521a6876 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -175,6 +175,7 @@ config FUJITSU_LAPTOP
depends on BACKLIGHT_CLASS_DEVICE
depends on ACPI_VIDEO || ACPI_VIDEO = n
depends on LEDS_CLASS || LEDS_CLASS=n
+ select INPUT_SPARSEKMAP
---help---
This is a driver for laptops built by Fujitsu:
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 68e338c6a876..3483ac37bee5 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -57,6 +57,7 @@
#include <linux/backlight.h>
#include <linux/fb.h>
#include <linux/input.h>
+#include <linux/input/sparse-keymap.h>
#include <linux/kfifo.h>
#include <linux/platform_device.h>
#include <linux/slab.h>
@@ -590,9 +591,16 @@ static const struct dmi_system_id fujitsu_dmi_table[] __initconst = {
/* ACPI device for LCD brightness control */
+static const struct key_entry keymap_backlight[] = {
+ { KE_KEY, true, { KEY_BRIGHTNESSUP } },
+ { KE_KEY, false, { KEY_BRIGHTNESSDOWN } },
+ { KE_END, 0 }
+};
+
static int acpi_fujitsu_bl_input_setup(struct acpi_device *device)
{
struct fujitsu_bl *fujitsu_bl = acpi_driver_data(device);
+ int ret;
fujitsu_bl->input = devm_input_allocate_device(&device->dev);
if (!fujitsu_bl->input)
@@ -605,10 +613,10 @@ static int acpi_fujitsu_bl_input_setup(struct acpi_device *device)
fujitsu_bl->input->phys = fujitsu_bl->phys;
fujitsu_bl->input->id.bustype = BUS_HOST;
fujitsu_bl->input->id.product = 0x06;
- fujitsu_bl->input->evbit[0] = BIT(EV_KEY);
- set_bit(KEY_BRIGHTNESSUP, fujitsu_bl->input->keybit);
- set_bit(KEY_BRIGHTNESSDOWN, fujitsu_bl->input->keybit);
- set_bit(KEY_UNKNOWN, fujitsu_bl->input->keybit);
+
+ ret = sparse_keymap_setup(fujitsu_bl->input, keymap_backlight, NULL);
+ if (ret)
+ return ret;
return input_register_device(fujitsu_bl->input);
}
@@ -714,18 +722,14 @@ static int acpi_fujitsu_bl_remove(struct acpi_device *device)
static void acpi_fujitsu_bl_notify(struct acpi_device *device, u32 event)
{
struct input_dev *input;
- int oldb, newb, keycode;
+ int oldb, newb;
input = fujitsu_bl->input;
if (event != ACPI_FUJITSU_NOTIFY_CODE1) {
- keycode = KEY_UNKNOWN;
vdbg_printk(FUJLAPTOP_DBG_WARN,
"unsupported event [0x%x]\n", event);
- input_report_key(input, keycode, 1);
- input_sync(input);
- input_report_key(input, keycode, 0);
- input_sync(input);
+ sparse_keymap_report_event(input, -1, 1, true);
return;
}
@@ -747,12 +751,7 @@ static void acpi_fujitsu_bl_notify(struct acpi_device *device, u32 event)
set_lcd_level(newb);
}
- keycode = oldb < newb ? KEY_BRIGHTNESSUP : KEY_BRIGHTNESSDOWN;
-
- input_report_key(input, keycode, 1);
- input_sync(input);
- input_report_key(input, keycode, 0);
- input_sync(input);
+ sparse_keymap_report_event(input, oldb < newb, 1, true);
}
/* ACPI device for hotkey handling */
--
2.12.0
On Mon, Mar 20, 2017 at 10:32:16AM +0100, Micha?? K??pie?? wrote:
> This series simplifies handling of both brightness key and hotkey input
> events on Fujitsu laptops by making use of sparse keymaps. This not
> only makes the driver shorter and, hopefully, cleaner, but also enables
> us to get rid of the keycodeX fields inside struct fujitsu_bl, which
> facilitates further cleanups. Also, to simplify error handling, input
> devices registered by fujitsu-laptop are migrated to the devres API
> along the way.
>
> This series was tested on a Lifebook S7020 and a Lifebook E744.
>
> This series depends on the platform cleanup series I posted last week.
> While that series has not yet been merged into testing, Jonathan has
> reviewed it and Darren also seemed to be okay with it, so I just assumed
> it will get merged soon. I wanted to post this one as soon as possible
> as it requires a bit more thorough review and testing compared to the
> previous series I posted for fujitsu-laptop.
Thanks for posting this. I have started going through this and hope to
complete my review by the end of this weekend.
Regards
jonathan
On Mon, Mar 20, 2017 at 10:32:23AM +0100, Micha?? K??pie?? wrote:
> Some laptop models need to have different keycodes assigned to hotkey
> scancodes. Change the sparse keymap upon a DMI match, before the hotkey
> input device is setup.
>
> Instead of using three different callbacks in the DMI match table,
> simplify code by using the driver_data field of struct dmi_system_id to
> supply the requested keymap to a common callback. Also merge keymaps
> for S6410 and S6420 as they are identical.
>
> [cut]
>
> diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> index 8f1c9c204110..1487eb2396c6 100644
> --- a/drivers/platform/x86/fujitsu-laptop.c
> +++ b/drivers/platform/x86/fujitsu-laptop.c
> @@ -766,8 +711,62 @@ static const struct key_entry keymap_default[] = {
> { KE_END, 0 }
> };
>
> +static const struct key_entry keymap_s6400[] = {
> + { KE_KEY, KEY1_CODE, { KEY_SCREENLOCK } }, /* "Lock" */
> + { KE_KEY, KEY2_CODE, { KEY_HELP } }, /* "Mobility Center */
> + { KE_KEY, KEY3_CODE, { KEY_PROG3 } },
> + { KE_KEY, KEY4_CODE, { KEY_PROG4 } },
> + { KE_END, 0 }
> +};
Since this keymap applies to both the S6410 and S6420, referencing S6400 in
its name might be slightly confusing in future. Calling this "keymap_s64x0"
or (if it fits better with conventions used elsewhere) "keymap_s64X0" would
avoid the confusion.
Regards
jonathan
On Fri, Mar 24, 2017 at 09:19:59PM +1030, Jonathan Woithe wrote:
> On Mon, Mar 20, 2017 at 10:32:16AM +0100, Micha?? K??pie?? wrote:
> > This series simplifies handling of both brightness key and hotkey input
> > events on Fujitsu laptops by making use of sparse keymaps. This not
> > only makes the driver shorter and, hopefully, cleaner, but also enables
> > us to get rid of the keycodeX fields inside struct fujitsu_bl, which
> > facilitates further cleanups. Also, to simplify error handling, input
> > devices registered by fujitsu-laptop are migrated to the devres API
> > along the way.
> >
> > This series was tested on a Lifebook S7020 and a Lifebook E744.
> >
> > This series depends on the platform cleanup series I posted last week.
> > While that series has not yet been merged into testing, Jonathan has
> > reviewed it and Darren also seemed to be okay with it, so I just assumed
> > it will get merged soon. I wanted to post this one as soon as possible
> > as it requires a bit more thorough review and testing compared to the
> > previous series I posted for fujitsu-laptop.
>
> Thanks for posting this. I have started going through this and hope to
> complete my review by the end of this weekend.
I have completed my initial review of this patch series. Aside from the
single recommendation about patch 7/8 (posted separately) it looks good.
I await your thoughts regarding patch 7/8 so we can finalise and sign off on
this series.
Regards
jonathan
> On Fri, Mar 24, 2017 at 09:19:59PM +1030, Jonathan Woithe wrote:
> > On Mon, Mar 20, 2017 at 10:32:16AM +0100, Micha?? K??pie?? wrote:
> > > This series simplifies handling of both brightness key and hotkey input
> > > events on Fujitsu laptops by making use of sparse keymaps. This not
> > > only makes the driver shorter and, hopefully, cleaner, but also enables
> > > us to get rid of the keycodeX fields inside struct fujitsu_bl, which
> > > facilitates further cleanups. Also, to simplify error handling, input
> > > devices registered by fujitsu-laptop are migrated to the devres API
> > > along the way.
> > >
> > > This series was tested on a Lifebook S7020 and a Lifebook E744.
> > >
> > > This series depends on the platform cleanup series I posted last week.
> > > While that series has not yet been merged into testing, Jonathan has
> > > reviewed it and Darren also seemed to be okay with it, so I just assumed
> > > it will get merged soon. I wanted to post this one as soon as possible
> > > as it requires a bit more thorough review and testing compared to the
> > > previous series I posted for fujitsu-laptop.
> >
> > Thanks for posting this. I have started going through this and hope to
> > complete my review by the end of this weekend.
>
> I have completed my initial review of this patch series. Aside from the
> single recommendation about patch 7/8 (posted separately) it looks good.
> I await your thoughts regarding patch 7/8 so we can finalise and sign off on
> this series.
Thanks for the review, Jonathan. I agree with your remark regarding the
potentially confusing name of the variable holding the S64x0 keymap.
As in the past, we have at least two options: I can either post v2 of
all eight patches with three characters changed or you can provide your
Reviewed-by for v1, in which case I will kindly ask the maintainers to
run:
sed -i 's|s6400|s64x0|;' drivers/platform/x86/fujitsu-laptop.c
after applying patch 7/8. Given that this series does a bit more than
the cleanup series I posted previously, I sense it might be a good idea
to defer submitting v2 until after subsystem maintainers review v1, just
in case they find more issues. If that happens, I will post v2 to avoid
confusion. If not, then v1 can be applied with the one-liner above
taken into account (or I can post v2 anyway if that would be preferred
by Darren and Andy).
In other words, I am happy to follow whatever route you and the
subsystem maintainers suggest and I just want to avoid spamming the
mailing list.
--
Best regards,
Michał Kępień
On Tue, Mar 28, 2017 at 08:16:18AM +0200, Micha?? K??pie?? wrote:
> > On Fri, Mar 24, 2017 at 09:19:59PM +1030, Jonathan Woithe wrote:
> > > On Mon, Mar 20, 2017 at 10:32:16AM +0100, Micha?? K??pie?? wrote:
> > > > This series simplifies handling of both brightness key and hotkey input
> > > > events on Fujitsu laptops by making use of sparse keymaps. This not
> > > > only makes the driver shorter and, hopefully, cleaner, but also enables
> > > > us to get rid of the keycodeX fields inside struct fujitsu_bl, which
> > > > facilitates further cleanups. Also, to simplify error handling, input
> > > > devices registered by fujitsu-laptop are migrated to the devres API
> > > > along the way.
> > > > :
> > I have completed my initial review of this patch series. Aside from the
> > single recommendation about patch 7/8 (posted separately) it looks good.
> > I await your thoughts regarding patch 7/8 so we can finalise and sign off on
> > this series.
>
> Thanks for the review, Jonathan. I agree with your remark regarding the
> potentially confusing name of the variable holding the S64x0 keymap.
>
> As in the past, we have at least two options: I can either post v2 of
> all eight patches with three characters changed or you can provide your
> Reviewed-by for v1, in which case I will kindly ask the maintainers to
> run:
>
> sed -i 's|s6400|s64x0|;' drivers/platform/x86/fujitsu-laptop.c
>
> after applying patch 7/8. Given that this series does a bit more than
> the cleanup series I posted previously, I sense it might be a good idea
> to defer submitting v2 until after subsystem maintainers review v1, just
> in case they find more issues. If that happens, I will post v2 to avoid
> confusion. If not, then v1 can be applied with the one-liner above
> taken into account (or I can post v2 anyway if that would be preferred
> by Darren and Andy).
>
> In other words, I am happy to follow whatever route you and the
> subsystem maintainers suggest and I just want to avoid spamming the
> mailing list.
I would be interested to hear what Darren and Andy's preference would be,
and have no issue with following that. My personal thought is that it's
best to have a patch series v2 submitted with the keymap fix since I think
that reduces the potential confusion now and in the future. It makes it
clear what exactly is being signed off on in any Reviewed-By tags. This
approach also removes the need for Darren or Andy to special case the
eventual merge of the series (having to remember to do the replacement),
which in turn reduces the chances of little errors creeping in. However, if
Darren and Andy are happy with an alternative then we can go with that.
As to whether v2 is held until Darren or Andy do their own review, I guess
it makes sense in case they have other suggestions which are similarly
trivial to implement.
Regards
jonathan
> On Tue, Mar 28, 2017 at 08:16:18AM +0200, Micha?? K??pie?? wrote:
> > > On Fri, Mar 24, 2017 at 09:19:59PM +1030, Jonathan Woithe wrote:
> > > > On Mon, Mar 20, 2017 at 10:32:16AM +0100, Micha?? K??pie?? wrote:
> > > > > This series simplifies handling of both brightness key and hotkey input
> > > > > events on Fujitsu laptops by making use of sparse keymaps. This not
> > > > > only makes the driver shorter and, hopefully, cleaner, but also enables
> > > > > us to get rid of the keycodeX fields inside struct fujitsu_bl, which
> > > > > facilitates further cleanups. Also, to simplify error handling, input
> > > > > devices registered by fujitsu-laptop are migrated to the devres API
> > > > > along the way.
> > > > > :
> > > I have completed my initial review of this patch series. Aside from the
> > > single recommendation about patch 7/8 (posted separately) it looks good.
> > > I await your thoughts regarding patch 7/8 so we can finalise and sign off on
> > > this series.
> >
> > Thanks for the review, Jonathan. I agree with your remark regarding the
> > potentially confusing name of the variable holding the S64x0 keymap.
> >
> > As in the past, we have at least two options: I can either post v2 of
> > all eight patches with three characters changed or you can provide your
> > Reviewed-by for v1, in which case I will kindly ask the maintainers to
> > run:
> >
> > sed -i 's|s6400|s64x0|;' drivers/platform/x86/fujitsu-laptop.c
> >
> > after applying patch 7/8. Given that this series does a bit more than
> > the cleanup series I posted previously, I sense it might be a good idea
> > to defer submitting v2 until after subsystem maintainers review v1, just
> > in case they find more issues. If that happens, I will post v2 to avoid
> > confusion. If not, then v1 can be applied with the one-liner above
> > taken into account (or I can post v2 anyway if that would be preferred
> > by Darren and Andy).
> >
> > In other words, I am happy to follow whatever route you and the
> > subsystem maintainers suggest and I just want to avoid spamming the
> > mailing list.
>
> I would be interested to hear what Darren and Andy's preference would be,
> and have no issue with following that. My personal thought is that it's
> best to have a patch series v2 submitted with the keymap fix since I think
> that reduces the potential confusion now and in the future. It makes it
> clear what exactly is being signed off on in any Reviewed-By tags. This
> approach also removes the need for Darren or Andy to special case the
> eventual merge of the series (having to remember to do the replacement),
> which in turn reduces the chances of little errors creeping in. However, if
> Darren and Andy are happy with an alternative then we can go with that.
Agreed.
> As to whether v2 is held until Darren or Andy do their own review, I guess
> it makes sense in case they have other suggestions which are similarly
> trivial to implement.
Darren, Andy, in light of the above I will be awaiting your review of
this series. I will submit v2 afterwards, with all remarks from both
you and Jonathan taken into account.
--
Best regards,
Michał Kępień
On Wed, Mar 29, 2017 at 10:19 AM, Michał Kępień <[email protected]> wrote:
> Darren, Andy, in light of the above I will be awaiting your review of
> this series. I will submit v2 afterwards, with all remarks from both
> you and Jonathan taken into account.
Darren marked this series under his name to review, so, I let him to
speak for us.
--
With Best Regards,
Andy Shevchenko
On Wed, Mar 29, 2017 at 09:30:29AM +1030, Jonathan Woithe wrote:
> On Tue, Mar 28, 2017 at 08:16:18AM +0200, Micha?? K??pie?? wrote:
> > > On Fri, Mar 24, 2017 at 09:19:59PM +1030, Jonathan Woithe wrote:
> > > > On Mon, Mar 20, 2017 at 10:32:16AM +0100, Micha?? K??pie?? wrote:
> > > > > This series simplifies handling of both brightness key and hotkey input
> > > > > events on Fujitsu laptops by making use of sparse keymaps. This not
> > > > > only makes the driver shorter and, hopefully, cleaner, but also enables
> > > > > us to get rid of the keycodeX fields inside struct fujitsu_bl, which
> > > > > facilitates further cleanups. Also, to simplify error handling, input
> > > > > devices registered by fujitsu-laptop are migrated to the devres API
> > > > > along the way.
> > > > > :
> > > I have completed my initial review of this patch series. Aside from the
> > > single recommendation about patch 7/8 (posted separately) it looks good.
> > > I await your thoughts regarding patch 7/8 so we can finalise and sign off on
> > > this series.
> >
> > Thanks for the review, Jonathan. I agree with your remark regarding the
> > potentially confusing name of the variable holding the S64x0 keymap.
> >
> > As in the past, we have at least two options: I can either post v2 of
> > all eight patches with three characters changed or you can provide your
> > Reviewed-by for v1, in which case I will kindly ask the maintainers to
> > run:
> >
> > sed -i 's|s6400|s64x0|;' drivers/platform/x86/fujitsu-laptop.c
> >
> > after applying patch 7/8. Given that this series does a bit more than
> > the cleanup series I posted previously, I sense it might be a good idea
> > to defer submitting v2 until after subsystem maintainers review v1, just
> > in case they find more issues. If that happens, I will post v2 to avoid
> > confusion. If not, then v1 can be applied with the one-liner above
> > taken into account (or I can post v2 anyway if that would be preferred
> > by Darren and Andy).
> >
> > In other words, I am happy to follow whatever route you and the
> > subsystem maintainers suggest and I just want to avoid spamming the
> > mailing list.
>
> I would be interested to hear what Darren and Andy's preference would be,
> and have no issue with following that. My personal thought is that it's
> best to have a patch series v2 submitted with the keymap fix since I think
> that reduces the potential confusion now and in the future. It makes it
> clear what exactly is being signed off on in any Reviewed-By tags. This
No concerns applying the minor fix. It's very common to say "With XXX,
Reviewed-by: ...", so I that's fine.
> approach also removes the need for Darren or Andy to special case the
> eventual merge of the series (having to remember to do the replacement),
> which in turn reduces the chances of little errors creeping in. However, if
> Darren and Andy are happy with an alternative then we can go with that.
>
> As to whether v2 is held until Darren or Andy do their own review, I guess
> it makes sense in case they have other suggestions which are similarly
> trivial to implement.
Completing my review now...
>
> Regards
> jonathan
>
--
Darren Hart
VMware Open Source Technology Center
On Mon, Mar 20, 2017 at 10:32:17AM +0100, Michał Kępień wrote:
> Simplify error handling in acpi_fujitsu_bl_add() by moving code
> responsible for setting up the input device to a separate function.
Modularization is nice, two minor nits/questions below:
>
> Signed-off-by: Michał Kępień <[email protected]>
> ---
> drivers/platform/x86/fujitsu-laptop.c | 64 +++++++++++++++++++++--------------
> 1 file changed, 38 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> index f3ccef3d5a1e..2b0dcf989e2a 100644
> --- a/drivers/platform/x86/fujitsu-laptop.c
> +++ b/drivers/platform/x86/fujitsu-laptop.c
> @@ -590,6 +590,41 @@ static const struct dmi_system_id fujitsu_dmi_table[] __initconst = {
>
> /* ACPI device for LCD brightness control */
>
> +static int acpi_fujitsu_bl_input_setup(struct acpi_device *device)
> +{
> + struct fujitsu_bl *fujitsu_bl = acpi_driver_data(device);
> + struct input_dev *input;
> + int error;
> +
> + fujitsu_bl->input = input = input_allocate_device();
> + if (!input)
> + return -ENOMEM;
> +
> + snprintf(fujitsu_bl->phys, sizeof(fujitsu_bl->phys),
> + "%s/video/input0", acpi_device_hid(device));
> +
> + input->name = acpi_device_name(device);
> + input->phys = fujitsu_bl->phys;
> + input->id.bustype = BUS_HOST;
> + input->id.product = 0x06;
> + input->dev.parent = &device->dev;
> + input->evbit[0] = BIT(EV_KEY);
> + set_bit(KEY_BRIGHTNESSUP, input->keybit);
> + set_bit(KEY_BRIGHTNESSDOWN, input->keybit);
> + set_bit(KEY_UNKNOWN, input->keybit);
> +
> + error = input_register_device(input);
> + if (error)
> + goto err_free_input_dev;
> +
> + return 0;
> +
> +err_free_input_dev:
> + input_free_device(input);
This leaves fujitsu_bl->input pointing at freed memory. Can this be assigned
only after successful registration? If not, it would be cleaner to NULL it on
failure.
> +
> + return error;
This return path could be cleaned up a bit:
error = input_register_device(input);
if (error)
input_free_device(input);
return error;
But, this driver uses this "error/return 0" pattern pretty consistently, whereas
most of the kernel uses ret instead of error, and will return ret on success and
failure, relying on it being 0 in the successful case. Over the whole driver,
we'd save several lines with the conversion and be more consistent with the rest
of the kernel. But, local consistency is important too. Jonathan, do you have a
preference for this driver?
--
Darren Hart
VMware Open Source Technology Center
On Mon, Mar 20, 2017 at 10:32:18AM +0100, Michał Kępień wrote:
> Use a managed input device for brightness events in order to simplify
> two error paths and one cleanup function while also reducing the number
> of local variables required. Remove double assignment to make
> checkpatch happy.
>
> Signed-off-by: Michał Kępień <[email protected]>
And this one mostly addresses my commentary on 1/8.
--
Darren Hart
VMware Open Source Technology Center
On Mon, Mar 20, 2017 at 10:32:20AM +0100, Michał Kępień wrote:
> Simplify error handling in acpi_fujitsu_laptop_add() by moving code
> responsible for setting up the input device to a separate function.
>
> Signed-off-by: Michał Kępień <[email protected]>
> ---
> drivers/platform/x86/fujitsu-laptop.c | 74 ++++++++++++++++++++---------------
> 1 file changed, 43 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> index 3483ac37bee5..b1a08d83330b 100644
> --- a/drivers/platform/x86/fujitsu-laptop.c
> +++ b/drivers/platform/x86/fujitsu-laptop.c
> @@ -794,7 +834,6 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
> {
> int result = 0;
> int state = 0;
> - struct input_dev *input;
> int error;
This patch highlights the odd error handling / return path with the odd mix of
result and error. Not introduced here, but something for a future cleanup
perhaps.
--
Darren Hart
VMware Open Source Technology Center
On Wed, Mar 29, 2017 at 07:35:50PM +0300, Andy Shevchenko wrote:
> On Wed, Mar 29, 2017 at 10:19 AM, Michał Kępień <[email protected]> wrote:
>
> > Darren, Andy, in light of the above I will be awaiting your review of
> > this series. I will submit v2 afterwards, with all remarks from both
> > you and Jonathan taken into account.
>
> Darren marked this series under his name to review, so, I let him to
> speak for us.
The series looks good to me. Nice work Michał. They are logically divided and
address issues in a procedural way (so I stopped commenting until I read the
full series through as a couple of times you addressed a concern from a move in
a cleanup to follow).
I've applied the noted change to 7/8 and will run this through my tests, but
don't anticipate any problems. Jonathan, if you don't have any additional
concerns, let me know if I can add your Reviewed-by.
Thanks,
--
Darren Hart
VMware Open Source Technology Center
On Wed, Mar 29, 2017 at 08:36:50PM -0700, Darren Hart wrote:
> On Wed, Mar 29, 2017 at 07:35:50PM +0300, Andy Shevchenko wrote:
> > On Wed, Mar 29, 2017 at 10:19 AM, Micha?? K??pie?? <[email protected]> wrote:
> >
> > > Darren, Andy, in light of the above I will be awaiting your review of
> > > this series. I will submit v2 afterwards, with all remarks from both
> > > you and Jonathan taken into account.
> >
> > Darren marked this series under his name to review, so, I let him to
> > speak for us.
>
> The series looks good to me. Nice work Micha??. They are logically divided and
> address issues in a procedural way (so I stopped commenting until I read the
> full series through as a couple of times you addressed a concern from a move in
> a cleanup to follow).
>
> I've applied the noted change to 7/8 and will run this through my tests, but
> don't anticipate any problems. Jonathan, if you don't have any additional
> concerns, let me know if I can add your Reviewed-by.
With the noted change to 7/8 applied I'm happy with the resulting series.
As you noted, there is still some scope for making things more consistent,
especially with regard to error handling. However, that is really a
separate task which can be addressed in a later series. This present series
doesn't impact on this issue in any significant way so it makes sense that
be applied.
Reviewed-by: Jonathan Woithe <[email protected]>
Regards
jonathan
On Wed, Mar 29, 2017 at 12:54:15PM -0700, Darren Hart wrote:
> On Mon, Mar 20, 2017 at 10:32:17AM +0100, Micha?? K??pie?? wrote:
> > +
> > + return error;
>
> This return path could be cleaned up a bit:
>
> error = input_register_device(input);
> if (error)
> input_free_device(input);
>
> return error;
>
> But, this driver uses this "error/return 0" pattern pretty consistently, whereas
> most of the kernel uses ret instead of error, and will return ret on success and
> failure, relying on it being 0 in the successful case. Over the whole driver,
> we'd save several lines with the conversion and be more consistent with the rest
> of the kernel. But, local consistency is important too. Jonathan, do you have a
> preference for this driver?
I have no strong preferences, except to say that clarity is important. As I
eluded to a few minutes ago, I agree that there's scope to address error
handling and there is a case to be made for bringing it into line with the
rest of the kernel. I think this can be addressed in a separate patch
series though. The present series under consideration doesn't make the
situation any worse (it actually improves it in some places) and introduces
worthwhile changes. As such I don't see that we gain anything by delaying
it in order to address what is, at the end of the day, a separate concern.
Regards
jonathan
On Thu, Mar 30, 2017 at 02:26:26PM +1030, Jonathan Woithe wrote:
> On Wed, Mar 29, 2017 at 08:36:50PM -0700, Darren Hart wrote:
> > On Wed, Mar 29, 2017 at 07:35:50PM +0300, Andy Shevchenko wrote:
> > > On Wed, Mar 29, 2017 at 10:19 AM, Micha?? K??pie?? <[email protected]> wrote:
> > >
> > > > Darren, Andy, in light of the above I will be awaiting your review of
> > > > this series. I will submit v2 afterwards, with all remarks from both
> > > > you and Jonathan taken into account.
> > >
> > > Darren marked this series under his name to review, so, I let him to
> > > speak for us.
> >
> > The series looks good to me. Nice work Micha??. They are logically divided and
> > address issues in a procedural way (so I stopped commenting until I read the
> > full series through as a couple of times you addressed a concern from a move in
> > a cleanup to follow).
> >
> > I've applied the noted change to 7/8 and will run this through my tests, but
> > don't anticipate any problems. Jonathan, if you don't have any additional
> > concerns, let me know if I can add your Reviewed-by.
>
> With the noted change to 7/8 applied I'm happy with the resulting series.
> As you noted, there is still some scope for making things more consistent,
> especially with regard to error handling. However, that is really a
> separate task which can be addressed in a later series. This present series
> doesn't impact on this issue in any significant way so it makes sense that
> be applied.
>
> Reviewed-by: Jonathan Woithe <[email protected]>
Merged, thanks!
--
Darren Hart
VMware Open Source Technology Center
> On Thu, Mar 30, 2017 at 02:26:26PM +1030, Jonathan Woithe wrote:
> > On Wed, Mar 29, 2017 at 08:36:50PM -0700, Darren Hart wrote:
> > > On Wed, Mar 29, 2017 at 07:35:50PM +0300, Andy Shevchenko wrote:
> > > > On Wed, Mar 29, 2017 at 10:19 AM, Micha?? K??pie?? <[email protected]> wrote:
> > > >
> > > > > Darren, Andy, in light of the above I will be awaiting your review of
> > > > > this series. I will submit v2 afterwards, with all remarks from both
> > > > > you and Jonathan taken into account.
> > > >
> > > > Darren marked this series under his name to review, so, I let him to
> > > > speak for us.
> > >
> > > The series looks good to me. Nice work Micha??. They are logically divided and
> > > address issues in a procedural way (so I stopped commenting until I read the
> > > full series through as a couple of times you addressed a concern from a move in
> > > a cleanup to follow).
> > >
> > > I've applied the noted change to 7/8 and will run this through my tests, but
> > > don't anticipate any problems. Jonathan, if you don't have any additional
> > > concerns, let me know if I can add your Reviewed-by.
> >
> > With the noted change to 7/8 applied I'm happy with the resulting series.
> > As you noted, there is still some scope for making things more consistent,
> > especially with regard to error handling. However, that is really a
> > separate task which can be addressed in a later series. This present series
> > doesn't impact on this issue in any significant way so it makes sense that
> > be applied.
> >
> > Reviewed-by: Jonathan Woithe <[email protected]>
>
> Merged, thanks!
Thanks to everyone involved. I noted your concerns about error handling
in the driver, rest assured that I share them. I have a local git
branch with miscellaneous fixes that I plan to submit (as one or more
series) in the future. I think error handling improvements fall right
into that "miscellaneous" category of fixes. For the time being I would
rather concentrate on the issues I personally consider more pressing as
I will most likely lose access to modern Fujitsu hardware in about six
weeks and I would really like to be able to test my patches on hardware
on which the code touched by these patches actually runs (LEDs come to
mind, specifically).
--
Best regards,
Michał Kępień
Hi Michael
On Thu, Mar 30, 2017 at 08:41:15AM +0200, Micha?? K??pie?? wrote:
> I noted your concerns about error handling in the driver ... For the time
> being I would rather concentrate on the issues I personally consider more
> pressing as I will most likely lose access to modern Fujitsu hardware in
> about six weeks and I would really like to be able to test my patches on
> hardware on which the code touched by these patches actually runs (LEDs
> come to mind, specifically).
I have no problem with this. Fixing consistency in connection with error
handling is certainly something which can be done by me (or anyone else)
at a later date. I agree that the other work is more important.
Regards
jonathan
> @@ -1098,14 +1075,8 @@ 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))) {
> - keycode = KEY_TOUCHPAD_TOGGLE;
> - input_report_key(input, keycode, 1);
> - input_sync(input);
> - input_report_key(input, keycode, 0);
> - input_sync(input);
> - }
> -
> + (call_fext_func(FLAG_RFKILL, 0x1, 0x0, 0x0) & BIT(26)))
> + sparse_keymap_report_event(input, BIT(26), 1, true);
I have only just now noticed that a typo crept in here, causing a bug.
The original call to call_fext_func() passed FUNC_FLAGS as the first
argument while the added one uses FLAG_RFKILL instead. This is wrong as
call_fext_func() arguments should be left intact by this patch.
Darren, could you please amend this in testing? The call_fext_func()
call added by the above patch chunk should pass FUNC_FLAGS as the first
argument, not FLAG_RFKILL.
Thanks and sorry for the trouble.
--
Best regards,
Michał Kępień
On Fri, Mar 31, 2017 at 01:22:02PM +0200, Michał Kępień wrote:
> > @@ -1098,14 +1075,8 @@ 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))) {
> > - keycode = KEY_TOUCHPAD_TOGGLE;
> > - input_report_key(input, keycode, 1);
> > - input_sync(input);
> > - input_report_key(input, keycode, 0);
> > - input_sync(input);
> > - }
> > -
> > + (call_fext_func(FLAG_RFKILL, 0x1, 0x0, 0x0) & BIT(26)))
> > + sparse_keymap_report_event(input, BIT(26), 1, true);
>
> I have only just now noticed that a typo crept in here, causing a bug.
> The original call to call_fext_func() passed FUNC_FLAGS as the first
> argument while the added one uses FLAG_RFKILL instead. This is wrong as
> call_fext_func() arguments should be left intact by this patch.
>
> Darren, could you please amend this in testing? The call_fext_func()
> call added by the above patch chunk should pass FUNC_FLAGS as the first
> argument, not FLAG_RFKILL.
>
> Thanks and sorry for the trouble.
Gah, I didn't catch that either :(
I've updated this patch with:
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 52d6d21..f66da4b 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -1074,7 +1074,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(FLAG_RFKILL, 0x1, 0x0, 0x0) & BIT(26)))
+ (call_fext_func(FUNC_FLAGS, 0x1, 0x0, 0x0) & BIT(26)))
sparse_keymap_report_event(input, BIT(26), 1, true);
}
in pdx86/testing, I'll push to for-next today, just as soon as CI confirms no
issues.
Thank you for catching it,
--
Darren Hart
VMware Open Source Technology Center
On Sat, Apr 01, 2017 at 01:00:23PM -0700, Darren Hart wrote:
> On Fri, Mar 31, 2017 at 01:22:02PM +0200, Micha?? K??pie?? wrote:
> > > @@ -1098,14 +1075,8 @@ 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))) {
> > > - keycode = KEY_TOUCHPAD_TOGGLE;
> > > - input_report_key(input, keycode, 1);
> > > - input_sync(input);
> > > - input_report_key(input, keycode, 0);
> > > - input_sync(input);
> > > - }
> > > -
> > > + (call_fext_func(FLAG_RFKILL, 0x1, 0x0, 0x0) & BIT(26)))
> > > + sparse_keymap_report_event(input, BIT(26), 1, true);
> >
> > I have only just now noticed that a typo crept in here, causing a bug.
> > The original call to call_fext_func() passed FUNC_FLAGS as the first
> > argument while the added one uses FLAG_RFKILL instead. This is wrong as
> > call_fext_func() arguments should be left intact by this patch.
> >
> > Darren, could you please amend this in testing? The call_fext_func()
> > call added by the above patch chunk should pass FUNC_FLAGS as the first
> > argument, not FLAG_RFKILL.
> >
> > Thanks and sorry for the trouble.
>
> Gah, I didn't catch that either :(
And I missed it too. :-( Thanks for catching this Michael. The reason it
never showed up in my testing is that my Fujitsu hardware doesn't have the
feature which exercises this code branch.
> I've updated this patch with:
>
> diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> index 52d6d21..f66da4b 100644
Thanks Darren.
Regards
jonathan