Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1576619 > unrolled thread
| Started by | Michał Kępień <kernel@kempniu.pl> |
|---|---|
| First post | 2017-02-08 15:30 +0100 |
| Last post | 2017-02-10 01:50 +0100 |
| Articles | 15 — 4 participants |
Back to article view | Back to linux.kernel
[PATCH 00/10] fujitsu-laptop: renames and cleanups Michał Kępień <kernel@kempniu.pl> - 2017-02-08 15:30 +0100
[PATCH 01/10] platform/x86: fujitsu-laptop: clearly denote backlight-related symbols Michał Kępień <kernel@kempniu.pl> - 2017-02-08 15:30 +0100
[PATCH 07/10] platform/x86: fujitsu-laptop: simplify acpi_bus_register_driver() error handling Michał Kępień <kernel@kempniu.pl> - 2017-02-08 15:40 +0100
[PATCH 06/10] platform/x86: fujitsu-laptop: remove redundant forward declarations Michał Kępień <kernel@kempniu.pl> - 2017-02-08 15:50 +0100
Re: [PATCH 00/10] fujitsu-laptop: renames and cleanups Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-02-08 16:30 +0100
Re: [PATCH 00/10] fujitsu-laptop: renames and cleanups Darren Hart <dvhart@infradead.org> - 2017-02-09 02:40 +0100
Re: [PATCH 00/10] fujitsu-laptop: renames and cleanups Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-02-09 02:50 +0100
Re: [PATCH 00/10] fujitsu-laptop: renames and cleanups Michał Kępień <kernel@kempniu.pl> - 2017-02-09 08:00 +0100
[PATCH 10/10] platform/x86: fujitsu-laptop: remove redundant MODULE_ALIAS entries Michał Kępień <kernel@kempniu.pl> - 2017-02-08 16:50 +0100
[PATCH 04/10] platform/x86: fujitsu-laptop: rename FUNC_RFKILL to FUNC_FLAGS Michał Kępień <kernel@kempniu.pl> - 2017-02-08 17:40 +0100
[PATCH 02/10] platform/x86: fujitsu-laptop: replace "hotkey" with "laptop" in symbol names Michał Kępień <kernel@kempniu.pl> - 2017-02-08 20:10 +0100
Re: [PATCH 00/10] fujitsu-laptop: renames and cleanups Jonathan Woithe <jwoithe@just42.net> - 2017-02-09 00:10 +0100
Re: [PATCH 00/10] fujitsu-laptop: renames and cleanups Michał Kępień <kernel@kempniu.pl> - 2017-02-09 08:40 +0100
Re: [PATCH 00/10] fujitsu-laptop: renames and cleanups Jonathan Woithe <jwoithe@just42.net> - 2017-02-10 01:20 +0100
Re: [PATCH 00/10] fujitsu-laptop: renames and cleanups Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-02-10 01:50 +0100
| From | Michał Kępień <kernel@kempniu.pl> |
|---|---|
| Date | 2017-02-08 15:30 +0100 |
| Subject | [PATCH 00/10] fujitsu-laptop: renames and cleanups |
| Message-ID | <t8Bgd-1nA-5@gated-at.bofh.it> |
This series of patches was originally submitted by Alan Jenkins in
September 2009. For various reasons they were never acted upon before.
Sadly, their original state makes them unreviewable due to multiple
changes happening within one patch and unreliable commit messages.
In order for Alan's efforts not to go to waste, I have rebased his
patches on top of dvhart/for-next, further split them into logically
separate changes and rewrote commit messages from scratch, based on the
assumed intent of the original author.
Here is how the original patches map to the rebased and split patches:
1/4 -> 1-6/10
2/4 -> 7-8/10
3/4 -> 9/10
4/4 -> 10/10
Some of these patches raise checkpatch warnings. This is intentional,
in order to make renames clean and easy to review. All of these issues
will be fixed by upcoming patch series.
Not all ideas suggested by Alan are present in the rebased series. A
brief discussion of each rejected suggestion follows (most important
issues first).
- Restructure fujitsu_init(). Quite frankly, I am pretty sure Alan
did not test his series on Fujitsu hardware with a logo lamp and/or
keyboard lamps. Neither have I, but with his original patches
applied, what happens on such a model is:
1. fujitsu_init() calls acpi_bus_register_driver().
2. acpi_fujitsu_laptop_add() is called.
3. fujitsu_laptop is kzalloc()'ed.
4. fujitsu_laptop->pf_device->dev is accessed.
This results in a NULL dereference, because the platform device is
only allocated and registered later in fujitsu_init().
On the other hand, registering the platform device before the ACPI
driver is also incorrect due to reasons I already pointed out in
another thread (in short: we cannot _assume_ FUJ02E3 is present).
This can only be fixed properly by registering the platform device
inside acpi_fujitsu_laptop_add(), but I am still waiting for
comments from Darren and Andy in the other thread before moving
forward down this path.
- Bail out when FUJ02B1 is not present. It could have been deemed
correct back in the day, but we now know that Fujitsu started
shipping devices without that ACPI device present, though with
FUJ02E3 still in place. These two ACPI devices are independent and
thus should not rely on each other's presence.
- Move keycode[1-5] fields to struct fujitsu_laptop. Doing this
causes ordering issues inside fujitsu_init(), while a patch series I
have queued that makes fujitsu-laptop use a sparse keymap removes
these fields altogether.
- Allocate and free fujitsu_bl and fujitsu_laptop inside ACPI
callbacks. While elegant and correct, this also causes ordering
issues inside fujitsu_init() and can only be done once platform
device registration is properly fixed.
- Sync backlight power status in acpi_fujitsu_bl_add(). The long-term
objective for fujitsu-laptop should be to achieve a clean split
between the backlight-related part and the laptop-related part.
This change keeps both parts intertwined. My fujitsu_init() cleanup
series contains a similar fix, but I have since found a different
solution to this problem which I will post once Alan's rebased
series gets applied.
- Remove dmi_check_cb_common(). The sparse keymap series I have
queued gets rid of this function without introducing an additional
field inside struct fujitsu_laptop.
Moreover, some other minor changes present in original patch 1/4 were
left out. A brief discussion of each such case follows.
-#define ACPI_FUJITSU_BL_NOTIFY_CODE1 0x80
+#define ACPI_FUJITSU_NOTIFY_CODE1 0x80
Notify code 0x80 is used by both parts of the driver (the
brightness-related one and the laptop-related one) and thus it should
not be annotated using the "_BL" infix specific to brightness-related
code.
-#if defined(CONFIG_LEDS_CLASS) || defined(CONFIG_LEDS_CLASS_MODULE)
...
-#endif
This does not really bring any benefit while breaking consistency with
other parts of the driver.
- dmi_check_system(fujitsu_laptop_dmi_table);
+ dmi_check_system(fujitsu_dmi_table);
The original patches moved this dmi_check_system() call to
acpi_fujitsu_laptop_add(), which justifies renaming fujitsu_dmi_table to
fujitsu_laptop_dmi_table. However, for reasons discussed above, the
rebased patches leave the dmi_check_system() call inside fujitsu_init(),
thus making the rename dubious.
- DMI_MATCH(DMI_SYS_VENDOR, "FUJITSU SIEMENS"),
+ DMI_MATCH(DMI_SYS_VENDOR, "FUJITSU_LAPTOP SIEMENS"),
There are multiple changes like this in original patch 1/4. They are
obviously wrong and were probably introduced by an unreviewed automatic
replacement.
@@ -495,7 +495,6 @@ static ssize_t
show_max_brightness(struct device *dev,
struct device_attribute *attr, char *buf)
{
-
int ret;
There are four instances of such a removal in original patch 1/4. They
are obviously correct, though they all happen inside functions related
to platform device attributes which I hope will soon get removed
altogether. Thus I refrained from applying these to reduce churn (at
least a bit).
Finally, some of the changes suggested by Alan were already applied
along the way:
- kmalloc() + memset() occurences were changed to kzalloc() by commit
6c75dd0f965b ("drivers/platform/x86: Use kzalloc").
- Unused debug macros were removed by commit 00816e1b3839
("fujitsu-laptop: Remove unused macros").
drivers/platform/x86/fujitsu-laptop.c | 476 +++++++++++++++++-----------------
1 file changed, 232 insertions(+), 244 deletions(-)
--
2.11.1
[toc] | [next] | [standalone]
| From | Michał Kępień <kernel@kempniu.pl> |
|---|---|
| Date | 2017-02-08 15:30 +0100 |
| Subject | [PATCH 01/10] platform/x86: fujitsu-laptop: clearly denote backlight-related symbols |
| Message-ID | <t8BpT-1qN-5@gated-at.bofh.it> |
| In reply to | #1576619 |
From: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
Unify naming for all backlight-related functions, structures, variables
and constants by using a consistent "_bl"/"_BL" suffix/infix. Adjust
indentation to make checkpatch happy.
Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
[kempniu: rebase patch, rewrite commit message]
Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
drivers/platform/x86/fujitsu-laptop.c | 214 +++++++++++++++++-----------------
1 file changed, 107 insertions(+), 107 deletions(-)
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 2b218b1d13e5..e1737b9d9d95 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -78,10 +78,10 @@
#define FUJITSU_LCD_N_LEVELS 8
-#define ACPI_FUJITSU_CLASS "fujitsu"
-#define ACPI_FUJITSU_HID "FUJ02B1"
-#define ACPI_FUJITSU_DRIVER_NAME "Fujitsu laptop FUJ02B1 ACPI brightness driver"
-#define ACPI_FUJITSU_DEVICE_NAME "Fujitsu FUJ02B1"
+#define ACPI_FUJITSU_CLASS "fujitsu"
+#define ACPI_FUJITSU_BL_HID "FUJ02B1"
+#define ACPI_FUJITSU_BL_DRIVER_NAME "Fujitsu laptop FUJ02B1 ACPI brightness driver"
+#define ACPI_FUJITSU_BL_DEVICE_NAME "Fujitsu FUJ02B1"
#define ACPI_FUJITSU_HOTKEY_HID "FUJ02E3"
#define ACPI_FUJITSU_HOTKEY_DRIVER_NAME "Fujitsu laptop FUJ02E3 ACPI hotkeys driver"
#define ACPI_FUJITSU_HOTKEY_DEVICE_NAME "Fujitsu FUJ02E3"
@@ -136,7 +136,7 @@
#endif
/* Device controlling the backlight and associated keys */
-struct fujitsu_t {
+struct fujitsu_bl {
acpi_handle acpi_handle;
struct acpi_device *dev;
struct input_dev *input;
@@ -150,7 +150,7 @@ struct fujitsu_t {
unsigned int brightness_level;
};
-static struct fujitsu_t *fujitsu;
+static struct fujitsu_bl *fujitsu_bl;
static int use_alt_lcd_levels = -1;
static int disable_brightness_adjust = -1;
@@ -222,7 +222,7 @@ static struct led_classdev eco_led = {
static u32 dbg_level = 0x03;
#endif
-static void acpi_fujitsu_notify(struct acpi_device *device, u32 event);
+static void acpi_fujitsu_bl_notify(struct acpi_device *device, u32 event);
/* Fujitsu ACPI interface function */
@@ -373,10 +373,10 @@ static int set_lcd_level(int level)
vdbg_printk(FUJLAPTOP_DBG_TRACE, "set lcd level via SBLL [%d]\n",
level);
- if (level < 0 || level >= fujitsu->max_brightness)
+ if (level < 0 || level >= fujitsu_bl->max_brightness)
return -EINVAL;
- status = acpi_get_handle(fujitsu->acpi_handle, "SBLL", &handle);
+ status = acpi_get_handle(fujitsu_bl->acpi_handle, "SBLL", &handle);
if (ACPI_FAILURE(status)) {
vdbg_printk(FUJLAPTOP_DBG_ERROR, "SBLL not present\n");
return -ENODEV;
@@ -398,10 +398,10 @@ static int set_lcd_level_alt(int level)
vdbg_printk(FUJLAPTOP_DBG_TRACE, "set lcd level via SBL2 [%d]\n",
level);
- if (level < 0 || level >= fujitsu->max_brightness)
+ if (level < 0 || level >= fujitsu_bl->max_brightness)
return -EINVAL;
- status = acpi_get_handle(fujitsu->acpi_handle, "SBL2", &handle);
+ status = acpi_get_handle(fujitsu_bl->acpi_handle, "SBL2", &handle);
if (ACPI_FAILURE(status)) {
vdbg_printk(FUJLAPTOP_DBG_ERROR, "SBL2 not present\n");
return -ENODEV;
@@ -421,19 +421,19 @@ static int get_lcd_level(void)
vdbg_printk(FUJLAPTOP_DBG_TRACE, "get lcd level via GBLL\n");
- status =
- acpi_evaluate_integer(fujitsu->acpi_handle, "GBLL", NULL, &state);
+ status = acpi_evaluate_integer(fujitsu_bl->acpi_handle, "GBLL", NULL,
+ &state);
if (ACPI_FAILURE(status))
return 0;
- fujitsu->brightness_level = state & 0x0fffffff;
+ fujitsu_bl->brightness_level = state & 0x0fffffff;
if (state & 0x80000000)
- fujitsu->brightness_changed = 1;
+ fujitsu_bl->brightness_changed = 1;
else
- fujitsu->brightness_changed = 0;
+ fujitsu_bl->brightness_changed = 0;
- return fujitsu->brightness_level;
+ return fujitsu_bl->brightness_level;
}
static int get_max_brightness(void)
@@ -443,14 +443,14 @@ static int get_max_brightness(void)
vdbg_printk(FUJLAPTOP_DBG_TRACE, "get max lcd level via RBLL\n");
- status =
- acpi_evaluate_integer(fujitsu->acpi_handle, "RBLL", NULL, &state);
+ status = acpi_evaluate_integer(fujitsu_bl->acpi_handle, "RBLL", NULL,
+ &state);
if (ACPI_FAILURE(status))
return -1;
- fujitsu->max_brightness = state;
+ fujitsu_bl->max_brightness = state;
- return fujitsu->max_brightness;
+ return fujitsu_bl->max_brightness;
}
/* Backlight device stuff */
@@ -483,7 +483,7 @@ static int bl_update_status(struct backlight_device *b)
return ret;
}
-static const struct backlight_ops fujitsubl_ops = {
+static const struct backlight_ops fujitsu_bl_ops = {
.get_brightness = bl_get_brightness,
.update_status = bl_update_status,
};
@@ -511,7 +511,7 @@ show_brightness_changed(struct device *dev,
int ret;
- ret = fujitsu->brightness_changed;
+ ret = fujitsu_bl->brightness_changed;
if (ret < 0)
return ret;
@@ -539,7 +539,7 @@ static ssize_t store_lcd_level(struct device *dev,
int level, ret;
if (sscanf(buf, "%i", &level) != 1
- || (level < 0 || level >= fujitsu->max_brightness))
+ || (level < 0 || level >= fujitsu_bl->max_brightness))
return -EINVAL;
if (use_alt_lcd_levels)
@@ -644,25 +644,25 @@ static void __init dmi_check_cb_common(const struct dmi_system_id *id)
static int __init dmi_check_cb_s6410(const struct dmi_system_id *id)
{
dmi_check_cb_common(id);
- fujitsu->keycode1 = KEY_SCREENLOCK; /* "Lock" */
- fujitsu->keycode2 = KEY_HELP; /* "Mobility Center" */
+ 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->keycode1 = KEY_SCREENLOCK; /* "Lock" */
- fujitsu->keycode2 = KEY_HELP; /* "Mobility Center" */
+ 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->keycode1 = KEY_HELP; /* "Support" */
- fujitsu->keycode3 = KEY_SWITCHVIDEOMODE; /* "Presentation" */
- fujitsu->keycode4 = KEY_WWW; /* "Internet" */
+ fujitsu_bl->keycode1 = KEY_HELP; /* "Support" */
+ fujitsu_bl->keycode3 = KEY_SWITCHVIDEOMODE; /* "Presentation" */
+ fujitsu_bl->keycode4 = KEY_WWW; /* "Internet" */
return 1;
}
@@ -693,7 +693,7 @@ static const struct dmi_system_id fujitsu_dmi_table[] __initconst = {
/* ACPI device for LCD brightness control */
-static int acpi_fujitsu_add(struct acpi_device *device)
+static int acpi_fujitsu_bl_add(struct acpi_device *device)
{
int state = 0;
struct input_dev *input;
@@ -702,22 +702,22 @@ static int acpi_fujitsu_add(struct acpi_device *device)
if (!device)
return -EINVAL;
- fujitsu->acpi_handle = device->handle;
- sprintf(acpi_device_name(device), "%s", ACPI_FUJITSU_DEVICE_NAME);
+ fujitsu_bl->acpi_handle = device->handle;
+ sprintf(acpi_device_name(device), "%s", ACPI_FUJITSU_BL_DEVICE_NAME);
sprintf(acpi_device_class(device), "%s", ACPI_FUJITSU_CLASS);
- device->driver_data = fujitsu;
+ device->driver_data = fujitsu_bl;
- fujitsu->input = input = input_allocate_device();
+ fujitsu_bl->input = input = input_allocate_device();
if (!input) {
error = -ENOMEM;
goto err_stop;
}
- snprintf(fujitsu->phys, sizeof(fujitsu->phys),
+ snprintf(fujitsu_bl->phys, sizeof(fujitsu_bl->phys),
"%s/video/input0", acpi_device_hid(device));
input->name = acpi_device_name(device);
- input->phys = fujitsu->phys;
+ input->phys = fujitsu_bl->phys;
input->id.bustype = BUS_HOST;
input->id.product = 0x06;
input->dev.parent = &device->dev;
@@ -730,7 +730,7 @@ static int acpi_fujitsu_add(struct acpi_device *device)
if (error)
goto err_free_input_dev;
- error = acpi_bus_update_power(fujitsu->acpi_handle, &state);
+ error = acpi_bus_update_power(fujitsu_bl->acpi_handle, &state);
if (error) {
pr_err("Error reading power state\n");
goto err_unregister_input_dev;
@@ -740,7 +740,7 @@ static int acpi_fujitsu_add(struct acpi_device *device)
acpi_device_name(device), acpi_device_bid(device),
!device->power.state ? "on" : "off");
- fujitsu->dev = device;
+ fujitsu_bl->dev = device;
if (acpi_has_method(device->handle, METHOD_NAME__INI)) {
vdbg_printk(FUJLAPTOP_DBG_INFO, "Invoking _INI\n");
@@ -758,7 +758,7 @@ static int acpi_fujitsu_add(struct acpi_device *device)
use_alt_lcd_levels, disable_brightness_adjust);
if (get_max_brightness() <= 0)
- fujitsu->max_brightness = FUJITSU_LCD_N_LEVELS;
+ fujitsu_bl->max_brightness = FUJITSU_LCD_N_LEVELS;
get_lcd_level();
return 0;
@@ -772,38 +772,38 @@ static int acpi_fujitsu_add(struct acpi_device *device)
return error;
}
-static int acpi_fujitsu_remove(struct acpi_device *device)
+static int acpi_fujitsu_bl_remove(struct acpi_device *device)
{
- struct fujitsu_t *fujitsu = acpi_driver_data(device);
- struct input_dev *input = fujitsu->input;
+ struct fujitsu_bl *fujitsu_bl = acpi_driver_data(device);
+ struct input_dev *input = fujitsu_bl->input;
input_unregister_device(input);
- fujitsu->acpi_handle = NULL;
+ fujitsu_bl->acpi_handle = NULL;
return 0;
}
/* Brightness notify */
-static void acpi_fujitsu_notify(struct acpi_device *device, u32 event)
+static void acpi_fujitsu_bl_notify(struct acpi_device *device, u32 event)
{
struct input_dev *input;
int keycode;
int oldb, newb;
- input = fujitsu->input;
+ input = fujitsu_bl->input;
switch (event) {
case ACPI_FUJITSU_NOTIFY_CODE1:
keycode = 0;
- oldb = fujitsu->brightness_level;
+ oldb = fujitsu_bl->brightness_level;
get_lcd_level();
- newb = fujitsu->brightness_level;
+ newb = fujitsu_bl->brightness_level;
vdbg_printk(FUJLAPTOP_DBG_TRACE,
"brightness button event [%i -> %i (%i)]\n",
- oldb, newb, fujitsu->brightness_changed);
+ oldb, newb, fujitsu_bl->brightness_changed);
if (oldb < newb) {
if (disable_brightness_adjust != 1) {
@@ -882,11 +882,11 @@ static int acpi_fujitsu_hotkey_add(struct acpi_device *device)
input->dev.parent = &device->dev;
set_bit(EV_KEY, input->evbit);
- set_bit(fujitsu->keycode1, input->keybit);
- set_bit(fujitsu->keycode2, input->keybit);
- set_bit(fujitsu->keycode3, input->keybit);
- set_bit(fujitsu->keycode4, input->keybit);
- set_bit(fujitsu->keycode5, input->keybit);
+ 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);
@@ -937,7 +937,7 @@ static int acpi_fujitsu_hotkey_add(struct acpi_device *device)
#if IS_ENABLED(CONFIG_LEDS_CLASS)
if (call_fext_func(FUNC_LEDS, 0x0, 0x0, 0x0) & LOGOLAMP_POWERON) {
- result = led_classdev_register(&fujitsu->pf_device->dev,
+ result = led_classdev_register(&fujitsu_bl->pf_device->dev,
&logolamp_led);
if (result == 0) {
fujitsu_hotkey->logolamp_registered = 1;
@@ -949,7 +949,7 @@ static int acpi_fujitsu_hotkey_add(struct acpi_device *device)
if ((call_fext_func(FUNC_LEDS, 0x0, 0x0, 0x0) & KEYBOARD_LAMPS) &&
(call_fext_func(FUNC_BUTTONS, 0x0, 0x0, 0x0) == 0x0)) {
- result = led_classdev_register(&fujitsu->pf_device->dev,
+ result = led_classdev_register(&fujitsu_bl->pf_device->dev,
&kblamps_led);
if (result == 0) {
fujitsu_hotkey->kblamps_registered = 1;
@@ -966,7 +966,7 @@ static int acpi_fujitsu_hotkey_add(struct acpi_device *device)
* that an RF LED is present.
*/
if (call_fext_func(FUNC_BUTTONS, 0x0, 0x0, 0x0) & BIT(24)) {
- result = led_classdev_register(&fujitsu->pf_device->dev,
+ result = led_classdev_register(&fujitsu_bl->pf_device->dev,
&radio_led);
if (result == 0) {
fujitsu_hotkey->radio_led_registered = 1;
@@ -983,7 +983,7 @@ static int acpi_fujitsu_hotkey_add(struct acpi_device *device)
*/
if ((call_fext_func(FUNC_LEDS, 0x0, 0x0, 0x0) & BIT(14)) &&
(call_fext_func(FUNC_LEDS, 0x2, ECO_LED, 0x0) != UNSUPPORTED_CMD)) {
- result = led_classdev_register(&fujitsu->pf_device->dev,
+ result = led_classdev_register(&fujitsu_bl->pf_device->dev,
&eco_led);
if (result == 0) {
fujitsu_hotkey->eco_led_registered = 1;
@@ -1103,19 +1103,19 @@ static void acpi_fujitsu_hotkey_notify(struct acpi_device *device, u32 event)
&& (i++) < MAX_HOTKEY_RINGBUFFER_SIZE) {
switch (irb & 0x4ff) {
case KEY1_CODE:
- keycode = fujitsu->keycode1;
+ keycode = fujitsu_bl->keycode1;
break;
case KEY2_CODE:
- keycode = fujitsu->keycode2;
+ keycode = fujitsu_bl->keycode2;
break;
case KEY3_CODE:
- keycode = fujitsu->keycode3;
+ keycode = fujitsu_bl->keycode3;
break;
case KEY4_CODE:
- keycode = fujitsu->keycode4;
+ keycode = fujitsu_bl->keycode4;
break;
case KEY5_CODE:
- keycode = fujitsu->keycode5;
+ keycode = fujitsu_bl->keycode5;
break;
case 0:
keycode = 0;
@@ -1150,19 +1150,19 @@ static void acpi_fujitsu_hotkey_notify(struct acpi_device *device, u32 event)
/* Initialization */
-static const struct acpi_device_id fujitsu_device_ids[] = {
- {ACPI_FUJITSU_HID, 0},
+static const struct acpi_device_id fujitsu_bl_device_ids[] = {
+ {ACPI_FUJITSU_BL_HID, 0},
{"", 0},
};
-static struct acpi_driver acpi_fujitsu_driver = {
- .name = ACPI_FUJITSU_DRIVER_NAME,
+static struct acpi_driver acpi_fujitsu_bl_driver = {
+ .name = ACPI_FUJITSU_BL_DRIVER_NAME,
.class = ACPI_FUJITSU_CLASS,
- .ids = fujitsu_device_ids,
+ .ids = fujitsu_bl_device_ids,
.ops = {
- .add = acpi_fujitsu_add,
- .remove = acpi_fujitsu_remove,
- .notify = acpi_fujitsu_notify,
+ .add = acpi_fujitsu_bl_add,
+ .remove = acpi_fujitsu_bl_remove,
+ .notify = acpi_fujitsu_bl_notify,
},
};
@@ -1183,7 +1183,7 @@ static struct acpi_driver acpi_fujitsu_hotkey_driver = {
};
static const struct acpi_device_id fujitsu_ids[] __used = {
- {ACPI_FUJITSU_HID, 0},
+ {ACPI_FUJITSU_BL_HID, 0},
{ACPI_FUJITSU_HOTKEY_HID, 0},
{"", 0}
};
@@ -1196,17 +1196,17 @@ static int __init fujitsu_init(void)
if (acpi_disabled)
return -ENODEV;
- fujitsu = kzalloc(sizeof(struct fujitsu_t), GFP_KERNEL);
- if (!fujitsu)
+ fujitsu_bl = kzalloc(sizeof(struct fujitsu_bl), GFP_KERNEL);
+ if (!fujitsu_bl)
return -ENOMEM;
- fujitsu->keycode1 = KEY_PROG1;
- fujitsu->keycode2 = KEY_PROG2;
- fujitsu->keycode3 = KEY_PROG3;
- fujitsu->keycode4 = KEY_PROG4;
- fujitsu->keycode5 = KEY_RFKILL;
+ 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;
dmi_check_system(fujitsu_dmi_table);
- result = acpi_bus_register_driver(&acpi_fujitsu_driver);
+ result = acpi_bus_register_driver(&acpi_fujitsu_bl_driver);
if (result < 0) {
ret = -ENODEV;
goto fail_acpi;
@@ -1214,18 +1214,18 @@ static int __init fujitsu_init(void)
/* Register platform stuff */
- fujitsu->pf_device = platform_device_alloc("fujitsu-laptop", -1);
- if (!fujitsu->pf_device) {
+ fujitsu_bl->pf_device = platform_device_alloc("fujitsu-laptop", -1);
+ if (!fujitsu_bl->pf_device) {
ret = -ENOMEM;
goto fail_platform_driver;
}
- ret = platform_device_add(fujitsu->pf_device);
+ ret = platform_device_add(fujitsu_bl->pf_device);
if (ret)
goto fail_platform_device1;
ret =
- sysfs_create_group(&fujitsu->pf_device->dev.kobj,
+ sysfs_create_group(&fujitsu_bl->pf_device->dev.kobj,
&fujitsupf_attribute_group);
if (ret)
goto fail_platform_device2;
@@ -1236,19 +1236,19 @@ static int __init fujitsu_init(void)
struct backlight_properties props;
memset(&props, 0, sizeof(struct backlight_properties));
- max_brightness = fujitsu->max_brightness;
+ max_brightness = fujitsu_bl->max_brightness;
props.type = BACKLIGHT_PLATFORM;
props.max_brightness = max_brightness - 1;
- fujitsu->bl_device = backlight_device_register("fujitsu-laptop",
- NULL, NULL,
- &fujitsubl_ops,
- &props);
- if (IS_ERR(fujitsu->bl_device)) {
- ret = PTR_ERR(fujitsu->bl_device);
- fujitsu->bl_device = NULL;
+ fujitsu_bl->bl_device = backlight_device_register("fujitsu-laptop",
+ NULL, NULL,
+ &fujitsu_bl_ops,
+ &props);
+ if (IS_ERR(fujitsu_bl->bl_device)) {
+ ret = PTR_ERR(fujitsu_bl->bl_device);
+ fujitsu_bl->bl_device = NULL;
goto fail_sysfs_group;
}
- fujitsu->bl_device->props.brightness = fujitsu->brightness_level;
+ fujitsu_bl->bl_device->props.brightness = fujitsu_bl->brightness_level;
}
ret = platform_driver_register(&fujitsupf_driver);
@@ -1272,9 +1272,9 @@ static int __init fujitsu_init(void)
/* Sync backlight power status (needs FUJ02E3 device, hence deferred) */
if (acpi_video_get_backlight_type() == acpi_backlight_vendor) {
if (call_fext_func(FUNC_BACKLIGHT, 0x2, 0x4, 0x0) == 3)
- fujitsu->bl_device->props.power = FB_BLANK_POWERDOWN;
+ fujitsu_bl->bl_device->props.power = FB_BLANK_POWERDOWN;
else
- fujitsu->bl_device->props.power = FB_BLANK_UNBLANK;
+ fujitsu_bl->bl_device->props.power = FB_BLANK_UNBLANK;
}
pr_info("driver " FUJITSU_DRIVER_VERSION " successfully loaded\n");
@@ -1286,18 +1286,18 @@ static int __init fujitsu_init(void)
fail_hotkey:
platform_driver_unregister(&fujitsupf_driver);
fail_backlight:
- backlight_device_unregister(fujitsu->bl_device);
+ backlight_device_unregister(fujitsu_bl->bl_device);
fail_sysfs_group:
- sysfs_remove_group(&fujitsu->pf_device->dev.kobj,
+ sysfs_remove_group(&fujitsu_bl->pf_device->dev.kobj,
&fujitsupf_attribute_group);
fail_platform_device2:
- platform_device_del(fujitsu->pf_device);
+ platform_device_del(fujitsu_bl->pf_device);
fail_platform_device1:
- platform_device_put(fujitsu->pf_device);
+ platform_device_put(fujitsu_bl->pf_device);
fail_platform_driver:
- acpi_bus_unregister_driver(&acpi_fujitsu_driver);
+ acpi_bus_unregister_driver(&acpi_fujitsu_bl_driver);
fail_acpi:
- kfree(fujitsu);
+ kfree(fujitsu_bl);
return ret;
}
@@ -1310,16 +1310,16 @@ static void __exit fujitsu_cleanup(void)
platform_driver_unregister(&fujitsupf_driver);
- backlight_device_unregister(fujitsu->bl_device);
+ backlight_device_unregister(fujitsu_bl->bl_device);
- sysfs_remove_group(&fujitsu->pf_device->dev.kobj,
+ sysfs_remove_group(&fujitsu_bl->pf_device->dev.kobj,
&fujitsupf_attribute_group);
- platform_device_unregister(fujitsu->pf_device);
+ platform_device_unregister(fujitsu_bl->pf_device);
- acpi_bus_unregister_driver(&acpi_fujitsu_driver);
+ acpi_bus_unregister_driver(&acpi_fujitsu_bl_driver);
- kfree(fujitsu);
+ kfree(fujitsu_bl);
pr_info("driver unloaded\n");
}
--
2.11.1
[toc] | [prev] | [next] | [standalone]
| From | Michał Kępień <kernel@kempniu.pl> |
|---|---|
| Date | 2017-02-08 15:40 +0100 |
| Subject | [PATCH 07/10] platform/x86: fujitsu-laptop: simplify acpi_bus_register_driver() error handling |
| Message-ID | <t8BzA-1u7-35@gated-at.bofh.it> |
| In reply to | #1576619 |
From: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
A separate variable is not needed to handle error codes returned by
acpi_bus_register_driver(). If the latter fails, just use the value it
returned as the value returned by fujitsu_init().
Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
[kempniu: rebase patch, rewrite commit message]
Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
drivers/platform/x86/fujitsu-laptop.c | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index ba0c0c211251..a5478a011b90 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -1192,7 +1192,7 @@ MODULE_DEVICE_TABLE(acpi, fujitsu_ids);
static int __init fujitsu_init(void)
{
- int ret, result, max_brightness;
+ int ret, max_brightness;
if (acpi_disabled)
return -ENODEV;
@@ -1207,11 +1207,9 @@ static int __init fujitsu_init(void)
fujitsu_bl->keycode5 = KEY_RFKILL;
dmi_check_system(fujitsu_dmi_table);
- result = acpi_bus_register_driver(&acpi_fujitsu_bl_driver);
- if (result < 0) {
- ret = -ENODEV;
+ ret = acpi_bus_register_driver(&acpi_fujitsu_bl_driver);
+ if (ret)
goto fail_acpi;
- }
/* Register platform stuff */
@@ -1264,11 +1262,9 @@ static int __init fujitsu_init(void)
goto fail_laptop;
}
- result = acpi_bus_register_driver(&acpi_fujitsu_laptop_driver);
- if (result < 0) {
- ret = -ENODEV;
+ ret = acpi_bus_register_driver(&acpi_fujitsu_laptop_driver);
+ if (ret)
goto fail_laptop1;
- }
/* Sync backlight power status (needs FUJ02E3 device, hence deferred) */
if (acpi_video_get_backlight_type() == acpi_backlight_vendor) {
--
2.11.1
[toc] | [prev] | [next] | [standalone]
| From | Michał Kępień <kernel@kempniu.pl> |
|---|---|
| Date | 2017-02-08 15:50 +0100 |
| Subject | [PATCH 06/10] platform/x86: fujitsu-laptop: remove redundant forward declarations |
| Message-ID | <t8BJg-1xy-19@gated-at.bofh.it> |
| In reply to | #1576619 |
From: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
Both acpi_fujitsu_bl_notify() and acpi_fujitsu_laptop_notify() are
defined before they are first used, so remove their forward declarations
as they are redundant.
Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
[kempniu: rebase patch, rewrite commit message]
Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
drivers/platform/x86/fujitsu-laptop.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index deef40a03b6d..ba0c0c211251 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -178,8 +178,6 @@ struct fujitsu_laptop {
static struct fujitsu_laptop *fujitsu_laptop;
-static void acpi_fujitsu_laptop_notify(struct acpi_device *device, u32 event);
-
#if IS_ENABLED(CONFIG_LEDS_CLASS)
static enum led_brightness logolamp_get(struct led_classdev *cdev);
static int logolamp_set(struct led_classdev *cdev,
@@ -227,8 +225,6 @@ static struct led_classdev eco_led = {
static u32 dbg_level = 0x03;
#endif
-static void acpi_fujitsu_bl_notify(struct acpi_device *device, u32 event);
-
/* Fujitsu ACPI interface function */
static int call_fext_func(int cmd, int arg0, int arg1, int arg2)
--
2.11.1
[toc] | [prev] | [next] | [standalone]
| From | Andy Shevchenko <andy.shevchenko@gmail.com> |
|---|---|
| Date | 2017-02-08 16:30 +0100 |
| Message-ID | <t8ClY-20B-15@gated-at.bofh.it> |
| In reply to | #1576619 |
On Wed, Feb 8, 2017 at 3:46 PM, Michał Kępień <kernel@kempniu.pl> wrote:
> This series of patches was originally submitted by Alan Jenkins in
> September 2009. For various reasons they were never acted upon before.
> Sadly, their original state makes them unreviewable due to multiple
> changes happening within one patch and unreliable commit messages.
>
> In order for Alan's efforts not to go to waste, I have rebased his
> patches on top of dvhart/for-next, further split them into logically
> separate changes and rewrote commit messages from scratch, based on the
> assumed intent of the original author.
>
> Here is how the original patches map to the rebased and split patches:
>
> 1/4 -> 1-6/10
> 2/4 -> 7-8/10
> 3/4 -> 9/10
> 4/4 -> 10/10
>
> Some of these patches raise checkpatch warnings. This is intentional,
> in order to make renames clean and easy to review. All of these issues
> will be fixed by upcoming patch series.
>
> Not all ideas suggested by Alan are present in the rebased series. A
> brief discussion of each rejected suggestion follows (most important
> issues first).
>
> - Restructure fujitsu_init(). Quite frankly, I am pretty sure Alan
> did not test his series on Fujitsu hardware with a logo lamp and/or
> keyboard lamps. Neither have I, but with his original patches
> applied, what happens on such a model is:
>
> 1. fujitsu_init() calls acpi_bus_register_driver().
> 2. acpi_fujitsu_laptop_add() is called.
> 3. fujitsu_laptop is kzalloc()'ed.
> 4. fujitsu_laptop->pf_device->dev is accessed.
>
> This results in a NULL dereference, because the platform device is
> only allocated and registered later in fujitsu_init().
>
> On the other hand, registering the platform device before the ACPI
> driver is also incorrect due to reasons I already pointed out in
> another thread (in short: we cannot _assume_ FUJ02E3 is present).
> This can only be fixed properly by registering the platform device
> inside acpi_fujitsu_laptop_add(), but I am still waiting for
> comments from Darren and Andy in the other thread before moving
> forward down this path.
>
> - Bail out when FUJ02B1 is not present. It could have been deemed
> correct back in the day, but we now know that Fujitsu started
> shipping devices without that ACPI device present, though with
> FUJ02E3 still in place. These two ACPI devices are independent and
> thus should not rely on each other's presence.
>
> - Move keycode[1-5] fields to struct fujitsu_laptop. Doing this
> causes ordering issues inside fujitsu_init(), while a patch series I
> have queued that makes fujitsu-laptop use a sparse keymap removes
> these fields altogether.
>
> - Allocate and free fujitsu_bl and fujitsu_laptop inside ACPI
> callbacks. While elegant and correct, this also causes ordering
> issues inside fujitsu_init() and can only be done once platform
> device registration is properly fixed.
>
> - Sync backlight power status in acpi_fujitsu_bl_add(). The long-term
> objective for fujitsu-laptop should be to achieve a clean split
> between the backlight-related part and the laptop-related part.
> This change keeps both parts intertwined. My fujitsu_init() cleanup
> series contains a similar fix, but I have since found a different
> solution to this problem which I will post once Alan's rebased
> series gets applied.
>
> - Remove dmi_check_cb_common(). The sparse keymap series I have
> queued gets rid of this function without introducing an additional
> field inside struct fujitsu_laptop.
>
> Moreover, some other minor changes present in original patch 1/4 were
> left out. A brief discussion of each such case follows.
>
> -#define ACPI_FUJITSU_BL_NOTIFY_CODE1 0x80
> +#define ACPI_FUJITSU_NOTIFY_CODE1 0x80
>
> Notify code 0x80 is used by both parts of the driver (the
> brightness-related one and the laptop-related one) and thus it should
> not be annotated using the "_BL" infix specific to brightness-related
> code.
>
> -#if defined(CONFIG_LEDS_CLASS) || defined(CONFIG_LEDS_CLASS_MODULE)
> ...
> -#endif
>
> This does not really bring any benefit while breaking consistency with
> other parts of the driver.
>
> - dmi_check_system(fujitsu_laptop_dmi_table);
> + dmi_check_system(fujitsu_dmi_table);
>
> The original patches moved this dmi_check_system() call to
> acpi_fujitsu_laptop_add(), which justifies renaming fujitsu_dmi_table to
> fujitsu_laptop_dmi_table. However, for reasons discussed above, the
> rebased patches leave the dmi_check_system() call inside fujitsu_init(),
> thus making the rename dubious.
>
> - DMI_MATCH(DMI_SYS_VENDOR, "FUJITSU SIEMENS"),
> + DMI_MATCH(DMI_SYS_VENDOR, "FUJITSU_LAPTOP SIEMENS"),
>
> There are multiple changes like this in original patch 1/4. They are
> obviously wrong and were probably introduced by an unreviewed automatic
> replacement.
>
> @@ -495,7 +495,6 @@ static ssize_t
> show_max_brightness(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> -
> int ret;
>
> There are four instances of such a removal in original patch 1/4. They
> are obviously correct, though they all happen inside functions related
> to platform device attributes which I hope will soon get removed
> altogether. Thus I refrained from applying these to reduce churn (at
> least a bit).
>
> Finally, some of the changes suggested by Alan were already applied
> along the way:
>
> - kmalloc() + memset() occurences were changed to kzalloc() by commit
> 6c75dd0f965b ("drivers/platform/x86: Use kzalloc").
>
> - Unused debug macros were removed by commit 00816e1b3839
> ("fujitsu-laptop: Remove unused macros").
>
Nice clean up!
So, I would apply 1-7, for the rest I need more time to review.
Regarding ACPI case and device presents you may assume it if you just call
acpi_walk_namespace() (AFAIU) and check _STA for the device if it's in
the table.
So, at any point you may have got understanding if device is present
or not, and if it's active or not.
--
With Best Regards,
Andy Shevchenko
[toc] | [prev] | [next] | [standalone]
| From | Darren Hart <dvhart@infradead.org> |
|---|---|
| Date | 2017-02-09 02:40 +0100 |
| Message-ID | <t8LSi-7Ue-15@gated-at.bofh.it> |
| In reply to | #1576651 |
On Wed, Feb 08, 2017 at 05:24:53PM +0200, Andy Shevchenko wrote: > On Wed, Feb 8, 2017 at 3:46 PM, Michał Kępień <kernel@kempniu.pl> wrote: ... > > Nice clean up! > So, I would apply 1-7, for the rest I need more time to review. As a point of policy for this tree. If the patches touch a file with a driver maintainer listed in MAINTAINERS, we need to make sure they are Cc'd and give them an opportunity to review before we commit them to the queue. In this case, Jonathan Woithe. Jonathan has committed to review in the next couple of days, so lets please give him first say on the patches. Thanks, -- Darren Hart Intel Open Source Technology Center
[toc] | [prev] | [next] | [standalone]
| From | Andy Shevchenko <andy.shevchenko@gmail.com> |
|---|---|
| Date | 2017-02-09 02:50 +0100 |
| Message-ID | <t8M1Y-7XZ-7@gated-at.bofh.it> |
| In reply to | #1577245 |
On Thu, Feb 9, 2017 at 3:36 AM, Darren Hart <dvhart@infradead.org> wrote: > On Wed, Feb 08, 2017 at 05:24:53PM +0200, Andy Shevchenko wrote: >> On Wed, Feb 8, 2017 at 3:46 PM, Michał Kępień <kernel@kempniu.pl> wrote: >> Nice clean up! >> So, I would apply 1-7, for the rest I need more time to review. > > As a point of policy for this tree. If the patches touch a file with a driver > maintainer listed in MAINTAINERS, we need to make sure they are Cc'd and give > them an opportunity to review before we commit them to the queue. In this case, > Jonathan Woithe. Good point! > Jonathan has committed to review in the next couple of days, so lets please give > him first say on the patches. Indeed, I'm not going to push this to for-next. -- With Best Regards, Andy Shevchenko
[toc] | [prev] | [next] | [standalone]
| From | Michał Kępień <kernel@kempniu.pl> |
|---|---|
| Date | 2017-02-09 08:00 +0100 |
| Message-ID | <t8QRX-2BZ-1@gated-at.bofh.it> |
| In reply to | #1576651 |
> Regarding ACPI case and device presents you may assume it if you just call > acpi_walk_namespace() (AFAIU) and check _STA for the device if it's in > the table. > > So, at any point you may have got understanding if device is present > or not, and if it's active or not. Thank you for the tip, though I am a bit confused, sorry. I infer the above is a response to this part of my message: > > On the other hand, registering the platform device before the ACPI > > driver is also incorrect due to reasons I already pointed out in > > another thread (in short: we cannot _assume_ FUJ02E3 is present). Perhaps my wording was inaccurate, but I do not have a technical problem with detecting an ACPI device. Instead, this passage was meant to be a reference to the fourth paragraph of my cover letter for the series titled "fujitsu_init() cleanup", i.e. the lengthy one awaiting comments from you and/or Darren. For your convenience, here is the paragraph in question (roughly the second half is relevant): > That would leave us with the remaining three sysfs attributes of the > platform device, namely dock, lid and radios. These all depend on the > FUJ02E3 ACPI device. Which begs the question: shall we reassign them to > that ACPI device and drop the platform device altogether? This would > logically be the correct thing to do (panasonic-laptop and toshiba_acpi > already assign extra sysfs attributes to ACPI nodes). But I understand > that this would break an 8-year-old userspace interface as functions > previously exposed through /sys/devices/platform/fujitsu-laptop would be > moved to /sys/bus/acpi/devices/FUJ02E3:00. If that is unacceptable, the > least we can (and should) do is to move platform device registration to > acpi_fujitsu_hotkey_add(). What the driver currently does may create > confusion in the future, because the platform device is registered > unconditionally while it clearly depends on FUJ02E3 being present. I do > not know whether FUJ02E3 is present on all Fujitsu devices today without > exception, but I do know that if Fujitsu ever decides to drop that > device from its firmware, we would again (see above) expose a userspace > interface (dock, lid, radios) which simply will not be able to function > properly. Perhaps things will become a bit more clear once you dig through that thread :) -- Best regards, Michał Kępień
[toc] | [prev] | [next] | [standalone]
| From | Michał Kępień <kernel@kempniu.pl> |
|---|---|
| Date | 2017-02-08 16:50 +0100 |
| Subject | [PATCH 10/10] platform/x86: fujitsu-laptop: remove redundant MODULE_ALIAS entries |
| Message-ID | <t8CFj-27w-1@gated-at.bofh.it> |
| In reply to | #1576619 |
From: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
MODULE_DEVICE_TABLE is all that is needed for fujitsu-laptop to be
properly autoloaded based on presence of its associated ACPI devices, so
remove redundant MODULE_ALIAS entries.
Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
[kempniu: rebase patch, rewrite commit message]
Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
drivers/platform/x86/fujitsu-laptop.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 4eabb169e2c9..33133a34e0bb 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -1333,7 +1333,3 @@ MODULE_AUTHOR("Jonathan Woithe, Peter Gruber, Tony Vroon");
MODULE_DESCRIPTION("Fujitsu laptop extras support");
MODULE_VERSION(FUJITSU_DRIVER_VERSION);
MODULE_LICENSE("GPL");
-
-MODULE_ALIAS("dmi:*:svnFUJITSUSIEMENS:*:pvr:rvnFUJITSU:rnFJNB1D3:*:cvrS6410:*");
-MODULE_ALIAS("dmi:*:svnFUJITSUSIEMENS:*:pvr:rvnFUJITSU:rnFJNB1E6:*:cvrS6420:*");
-MODULE_ALIAS("dmi:*:svnFUJITSU:*:pvr:rvnFUJITSU:rnFJNB19C:*:cvrS7020:*");
--
2.11.1
[toc] | [prev] | [next] | [standalone]
| From | Michał Kępień <kernel@kempniu.pl> |
|---|---|
| Date | 2017-02-08 17:40 +0100 |
| Subject | [PATCH 04/10] platform/x86: fujitsu-laptop: rename FUNC_RFKILL to FUNC_FLAGS |
| Message-ID | <t8DrJ-2DS-35@gated-at.bofh.it> |
| In reply to | #1576619 |
From: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
FUNC subfunction 0x1000 is currently referred to as FUNC_RFKILL, which
is misleading, because it handles more than just radio devices (also
lid, dock, LEDs). Rename the FUNC_RFKILL constant to FUNC_FLAGS.
Replace "rfkill" with "flags" in the names of its associated fields
inside struct fujitsu_laptop.
Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
[kempniu: rebase patch, rewrite commit message]
Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
drivers/platform/x86/fujitsu-laptop.c | 50 +++++++++++++++++------------------
1 file changed, 25 insertions(+), 25 deletions(-)
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 6cdd1b334e8a..55d696262301 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -89,7 +89,7 @@
#define ACPI_FUJITSU_NOTIFY_CODE1 0x80
/* FUNC interface - command values */
-#define FUNC_RFKILL 0x1000
+#define FUNC_FLAGS 0x1000
#define FUNC_LEDS 0x1001
#define FUNC_BUTTONS 0x1002
#define FUNC_BACKLIGHT 0x1004
@@ -163,8 +163,8 @@ struct fujitsu_laptop {
struct platform_device *pf_device;
struct kfifo fifo;
spinlock_t fifo_lock;
- int rfkill_supported;
- int rfkill_state;
+ int flags_supported;
+ int flags_state;
int logolamp_registered;
int kblamps_registered;
int radio_led_registered;
@@ -300,9 +300,9 @@ static int radio_led_set(struct led_classdev *cdev,
enum led_brightness brightness)
{
if (brightness >= LED_FULL)
- return call_fext_func(FUNC_RFKILL, 0x5, RADIO_LED_ON, RADIO_LED_ON);
+ return call_fext_func(FUNC_FLAGS, 0x5, RADIO_LED_ON, RADIO_LED_ON);
else
- return call_fext_func(FUNC_RFKILL, 0x5, RADIO_LED_ON, 0x0);
+ return call_fext_func(FUNC_FLAGS, 0x5, RADIO_LED_ON, 0x0);
}
static int eco_led_set(struct led_classdev *cdev,
@@ -346,7 +346,7 @@ static enum led_brightness radio_led_get(struct led_classdev *cdev)
{
enum led_brightness brightness = LED_OFF;
- if (call_fext_func(FUNC_RFKILL, 0x4, 0x0, 0x0) & RADIO_LED_ON)
+ if (call_fext_func(FUNC_FLAGS, 0x4, 0x0, 0x0) & RADIO_LED_ON)
brightness = LED_FULL;
return brightness;
@@ -567,9 +567,9 @@ static ssize_t
show_lid_state(struct device *dev,
struct device_attribute *attr, char *buf)
{
- if (!(fujitsu_laptop->rfkill_supported & 0x100))
+ if (!(fujitsu_laptop->flags_supported & 0x100))
return sprintf(buf, "unknown\n");
- if (fujitsu_laptop->rfkill_state & 0x100)
+ if (fujitsu_laptop->flags_state & 0x100)
return sprintf(buf, "open\n");
else
return sprintf(buf, "closed\n");
@@ -579,9 +579,9 @@ static ssize_t
show_dock_state(struct device *dev,
struct device_attribute *attr, char *buf)
{
- if (!(fujitsu_laptop->rfkill_supported & 0x200))
+ if (!(fujitsu_laptop->flags_supported & 0x200))
return sprintf(buf, "unknown\n");
- if (fujitsu_laptop->rfkill_state & 0x200)
+ if (fujitsu_laptop->flags_state & 0x200)
return sprintf(buf, "docked\n");
else
return sprintf(buf, "undocked\n");
@@ -591,9 +591,9 @@ static ssize_t
show_radios_state(struct device *dev,
struct device_attribute *attr, char *buf)
{
- if (!(fujitsu_laptop->rfkill_supported & 0x20))
+ if (!(fujitsu_laptop->flags_supported & 0x20))
return sprintf(buf, "unknown\n");
- if (fujitsu_laptop->rfkill_state & 0x20)
+ if (fujitsu_laptop->flags_state & 0x20)
return sprintf(buf, "on\n");
else
return sprintf(buf, "killed\n");
@@ -920,17 +920,17 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
; /* No action, result is discarded */
vdbg_printk(FUJLAPTOP_DBG_INFO, "Discarded %i ringbuffer entries\n", i);
- fujitsu_laptop->rfkill_supported =
- call_fext_func(FUNC_RFKILL, 0x0, 0x0, 0x0);
+ fujitsu_laptop->flags_supported =
+ call_fext_func(FUNC_FLAGS, 0x0, 0x0, 0x0);
/* Make sure our bitmask of supported functions is cleared if the
RFKILL function block is not implemented, like on the S7020. */
- if (fujitsu_laptop->rfkill_supported == UNSUPPORTED_CMD)
- fujitsu_laptop->rfkill_supported = 0;
+ if (fujitsu_laptop->flags_supported == UNSUPPORTED_CMD)
+ fujitsu_laptop->flags_supported = 0;
- if (fujitsu_laptop->rfkill_supported)
- fujitsu_laptop->rfkill_state =
- call_fext_func(FUNC_RFKILL, 0x4, 0x0, 0x0);
+ if (fujitsu_laptop->flags_supported)
+ fujitsu_laptop->flags_state =
+ call_fext_func(FUNC_FLAGS, 0x4, 0x0, 0x0);
/* Suspect this is a keymap of the application panel, print it */
pr_info("BTNI: [0x%x]\n", call_fext_func(FUNC_BUTTONS, 0x0, 0x0, 0x0));
@@ -1093,9 +1093,9 @@ static void acpi_fujitsu_laptop_notify(struct acpi_device *device, u32 event)
return;
}
- if (fujitsu_laptop->rfkill_supported)
- fujitsu_laptop->rfkill_state =
- call_fext_func(FUNC_RFKILL, 0x4, 0x0, 0x0);
+ if (fujitsu_laptop->flags_supported)
+ fujitsu_laptop->flags_state =
+ call_fext_func(FUNC_FLAGS, 0x4, 0x0, 0x0);
i = 0;
while ((irb =
@@ -1135,10 +1135,10 @@ static void acpi_fujitsu_laptop_notify(struct acpi_device *device, u32 event)
/* On some models (first seen on the Skylake-based Lifebook
* E736/E746/E756), the touchpad toggle hotkey (Fn+F4) is
- * handled in software; its state is queried using FUNC_RFKILL
+ * handled in software; its state is queried using FUNC_FLAGS
*/
- if ((fujitsu_laptop->rfkill_supported & BIT(26)) &&
- (call_fext_func(FUNC_RFKILL, 0x1, 0x0, 0x0) & BIT(26))) {
+ 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);
--
2.11.1
[toc] | [prev] | [next] | [standalone]
| From | Michał Kępień <kernel@kempniu.pl> |
|---|---|
| Date | 2017-02-08 20:10 +0100 |
| Subject | [PATCH 02/10] platform/x86: fujitsu-laptop: replace "hotkey" with "laptop" in symbol names |
| Message-ID | <t8FMV-4eP-97@gated-at.bofh.it> |
| In reply to | #1576619 |
From: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
Functions, structures, variables and constants whose names currently
contain the "hotkey" keyword are not only responsible for handling
hotkeys, but also other laptop-related features (rfkill, lid, dock,
LEDs). Fix their naming by using a consistent "_laptop"/"_LAPTOP"
suffix/infix. Update comments so that they reflect this change.
Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
[kempniu: rebase patch, rewrite commit message]
Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
drivers/platform/x86/fujitsu-laptop.c | 158 +++++++++++++++++-----------------
1 file changed, 79 insertions(+), 79 deletions(-)
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index e1737b9d9d95..70124004b346 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -82,9 +82,9 @@
#define ACPI_FUJITSU_BL_HID "FUJ02B1"
#define ACPI_FUJITSU_BL_DRIVER_NAME "Fujitsu laptop FUJ02B1 ACPI brightness driver"
#define ACPI_FUJITSU_BL_DEVICE_NAME "Fujitsu FUJ02B1"
-#define ACPI_FUJITSU_HOTKEY_HID "FUJ02E3"
-#define ACPI_FUJITSU_HOTKEY_DRIVER_NAME "Fujitsu laptop FUJ02E3 ACPI hotkeys driver"
-#define ACPI_FUJITSU_HOTKEY_DEVICE_NAME "Fujitsu FUJ02E3"
+#define ACPI_FUJITSU_LAPTOP_HID "FUJ02E3"
+#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
@@ -154,8 +154,8 @@ static struct fujitsu_bl *fujitsu_bl;
static int use_alt_lcd_levels = -1;
static int disable_brightness_adjust = -1;
-/* Device used to access other hotkeys on the laptop */
-struct fujitsu_hotkey_t {
+/* Device used to access hotkeys and other features on the laptop */
+struct fujitsu_laptop {
acpi_handle acpi_handle;
struct acpi_device *dev;
struct input_dev *input;
@@ -171,9 +171,9 @@ struct fujitsu_hotkey_t {
int eco_led_registered;
};
-static struct fujitsu_hotkey_t *fujitsu_hotkey;
+static struct fujitsu_laptop *fujitsu_laptop;
-static void acpi_fujitsu_hotkey_notify(struct acpi_device *device, u32 event);
+static void acpi_fujitsu_laptop_notify(struct acpi_device *device, u32 event);
#if IS_ENABLED(CONFIG_LEDS_CLASS)
static enum led_brightness logolamp_get(struct led_classdev *cdev);
@@ -239,7 +239,7 @@ static int call_fext_func(int cmd, int arg0, int arg1, int arg2)
unsigned long long value;
acpi_handle handle = NULL;
- status = acpi_get_handle(fujitsu_hotkey->acpi_handle, "FUNC", &handle);
+ status = acpi_get_handle(fujitsu_laptop->acpi_handle, "FUNC", &handle);
if (ACPI_FAILURE(status)) {
vdbg_printk(FUJLAPTOP_DBG_ERROR,
"FUNC interface is not present\n");
@@ -567,9 +567,9 @@ static ssize_t
show_lid_state(struct device *dev,
struct device_attribute *attr, char *buf)
{
- if (!(fujitsu_hotkey->rfkill_supported & 0x100))
+ if (!(fujitsu_laptop->rfkill_supported & 0x100))
return sprintf(buf, "unknown\n");
- if (fujitsu_hotkey->rfkill_state & 0x100)
+ if (fujitsu_laptop->rfkill_state & 0x100)
return sprintf(buf, "open\n");
else
return sprintf(buf, "closed\n");
@@ -579,9 +579,9 @@ static ssize_t
show_dock_state(struct device *dev,
struct device_attribute *attr, char *buf)
{
- if (!(fujitsu_hotkey->rfkill_supported & 0x200))
+ if (!(fujitsu_laptop->rfkill_supported & 0x200))
return sprintf(buf, "unknown\n");
- if (fujitsu_hotkey->rfkill_state & 0x200)
+ if (fujitsu_laptop->rfkill_state & 0x200)
return sprintf(buf, "docked\n");
else
return sprintf(buf, "undocked\n");
@@ -591,9 +591,9 @@ static ssize_t
show_radios_state(struct device *dev,
struct device_attribute *attr, char *buf)
{
- if (!(fujitsu_hotkey->rfkill_supported & 0x20))
+ if (!(fujitsu_laptop->rfkill_supported & 0x20))
return sprintf(buf, "unknown\n");
- if (fujitsu_hotkey->rfkill_state & 0x20)
+ if (fujitsu_laptop->rfkill_state & 0x20)
return sprintf(buf, "on\n");
else
return sprintf(buf, "killed\n");
@@ -840,7 +840,7 @@ static void acpi_fujitsu_bl_notify(struct acpi_device *device, u32 event)
/* ACPI device for hotkey handling */
-static int acpi_fujitsu_hotkey_add(struct acpi_device *device)
+static int acpi_fujitsu_laptop_add(struct acpi_device *device)
{
int result = 0;
int state = 0;
@@ -851,32 +851,32 @@ static int acpi_fujitsu_hotkey_add(struct acpi_device *device)
if (!device)
return -EINVAL;
- fujitsu_hotkey->acpi_handle = device->handle;
+ fujitsu_laptop->acpi_handle = device->handle;
sprintf(acpi_device_name(device), "%s",
- ACPI_FUJITSU_HOTKEY_DEVICE_NAME);
+ ACPI_FUJITSU_LAPTOP_DEVICE_NAME);
sprintf(acpi_device_class(device), "%s", ACPI_FUJITSU_CLASS);
- device->driver_data = fujitsu_hotkey;
+ device->driver_data = fujitsu_laptop;
/* kfifo */
- spin_lock_init(&fujitsu_hotkey->fifo_lock);
- error = kfifo_alloc(&fujitsu_hotkey->fifo, RINGBUFFERSIZE * sizeof(int),
+ spin_lock_init(&fujitsu_laptop->fifo_lock);
+ error = kfifo_alloc(&fujitsu_laptop->fifo, RINGBUFFERSIZE * sizeof(int),
GFP_KERNEL);
if (error) {
pr_err("kfifo_alloc failed\n");
goto err_stop;
}
- fujitsu_hotkey->input = input = input_allocate_device();
+ fujitsu_laptop->input = input = input_allocate_device();
if (!input) {
error = -ENOMEM;
goto err_free_fifo;
}
- snprintf(fujitsu_hotkey->phys, sizeof(fujitsu_hotkey->phys),
+ snprintf(fujitsu_laptop->phys, sizeof(fujitsu_laptop->phys),
"%s/video/input0", acpi_device_hid(device));
input->name = acpi_device_name(device);
- input->phys = fujitsu_hotkey->phys;
+ input->phys = fujitsu_laptop->phys;
input->id.bustype = BUS_HOST;
input->id.product = 0x06;
input->dev.parent = &device->dev;
@@ -894,7 +894,7 @@ static int acpi_fujitsu_hotkey_add(struct acpi_device *device)
if (error)
goto err_free_input_dev;
- error = acpi_bus_update_power(fujitsu_hotkey->acpi_handle, &state);
+ error = acpi_bus_update_power(fujitsu_laptop->acpi_handle, &state);
if (error) {
pr_err("Error reading power state\n");
goto err_unregister_input_dev;
@@ -904,7 +904,7 @@ static int acpi_fujitsu_hotkey_add(struct acpi_device *device)
acpi_device_name(device), acpi_device_bid(device),
!device->power.state ? "on" : "off");
- fujitsu_hotkey->dev = device;
+ fujitsu_laptop->dev = device;
if (acpi_has_method(device->handle, METHOD_NAME__INI)) {
vdbg_printk(FUJLAPTOP_DBG_INFO, "Invoking _INI\n");
@@ -920,16 +920,16 @@ static int acpi_fujitsu_hotkey_add(struct acpi_device *device)
; /* No action, result is discarded */
vdbg_printk(FUJLAPTOP_DBG_INFO, "Discarded %i ringbuffer entries\n", i);
- fujitsu_hotkey->rfkill_supported =
+ fujitsu_laptop->rfkill_supported =
call_fext_func(FUNC_RFKILL, 0x0, 0x0, 0x0);
/* Make sure our bitmask of supported functions is cleared if the
RFKILL function block is not implemented, like on the S7020. */
- if (fujitsu_hotkey->rfkill_supported == UNSUPPORTED_CMD)
- fujitsu_hotkey->rfkill_supported = 0;
+ if (fujitsu_laptop->rfkill_supported == UNSUPPORTED_CMD)
+ fujitsu_laptop->rfkill_supported = 0;
- if (fujitsu_hotkey->rfkill_supported)
- fujitsu_hotkey->rfkill_state =
+ if (fujitsu_laptop->rfkill_supported)
+ fujitsu_laptop->rfkill_state =
call_fext_func(FUNC_RFKILL, 0x4, 0x0, 0x0);
/* Suspect this is a keymap of the application panel, print it */
@@ -940,7 +940,7 @@ static int acpi_fujitsu_hotkey_add(struct acpi_device *device)
result = led_classdev_register(&fujitsu_bl->pf_device->dev,
&logolamp_led);
if (result == 0) {
- fujitsu_hotkey->logolamp_registered = 1;
+ fujitsu_laptop->logolamp_registered = 1;
} else {
pr_err("Could not register LED handler for logo lamp, error %i\n",
result);
@@ -952,7 +952,7 @@ static int acpi_fujitsu_hotkey_add(struct acpi_device *device)
result = led_classdev_register(&fujitsu_bl->pf_device->dev,
&kblamps_led);
if (result == 0) {
- fujitsu_hotkey->kblamps_registered = 1;
+ fujitsu_laptop->kblamps_registered = 1;
} else {
pr_err("Could not register LED handler for keyboard lamps, error %i\n",
result);
@@ -969,7 +969,7 @@ static int acpi_fujitsu_hotkey_add(struct acpi_device *device)
result = led_classdev_register(&fujitsu_bl->pf_device->dev,
&radio_led);
if (result == 0) {
- fujitsu_hotkey->radio_led_registered = 1;
+ fujitsu_laptop->radio_led_registered = 1;
} else {
pr_err("Could not register LED handler for radio LED, error %i\n",
result);
@@ -986,7 +986,7 @@ static int acpi_fujitsu_hotkey_add(struct acpi_device *device)
result = led_classdev_register(&fujitsu_bl->pf_device->dev,
&eco_led);
if (result == 0) {
- fujitsu_hotkey->eco_led_registered = 1;
+ fujitsu_laptop->eco_led_registered = 1;
} else {
pr_err("Could not register LED handler for eco LED, error %i\n",
result);
@@ -1002,47 +1002,47 @@ static int acpi_fujitsu_hotkey_add(struct acpi_device *device)
err_free_input_dev:
input_free_device(input);
err_free_fifo:
- kfifo_free(&fujitsu_hotkey->fifo);
+ kfifo_free(&fujitsu_laptop->fifo);
err_stop:
return error;
}
-static int acpi_fujitsu_hotkey_remove(struct acpi_device *device)
+static int acpi_fujitsu_laptop_remove(struct acpi_device *device)
{
- struct fujitsu_hotkey_t *fujitsu_hotkey = acpi_driver_data(device);
- struct input_dev *input = fujitsu_hotkey->input;
+ struct fujitsu_laptop *fujitsu_laptop = acpi_driver_data(device);
+ struct input_dev *input = fujitsu_laptop->input;
#if IS_ENABLED(CONFIG_LEDS_CLASS)
- if (fujitsu_hotkey->logolamp_registered)
+ if (fujitsu_laptop->logolamp_registered)
led_classdev_unregister(&logolamp_led);
- if (fujitsu_hotkey->kblamps_registered)
+ if (fujitsu_laptop->kblamps_registered)
led_classdev_unregister(&kblamps_led);
- if (fujitsu_hotkey->radio_led_registered)
+ if (fujitsu_laptop->radio_led_registered)
led_classdev_unregister(&radio_led);
- if (fujitsu_hotkey->eco_led_registered)
+ if (fujitsu_laptop->eco_led_registered)
led_classdev_unregister(&eco_led);
#endif
input_unregister_device(input);
- kfifo_free(&fujitsu_hotkey->fifo);
+ kfifo_free(&fujitsu_laptop->fifo);
- fujitsu_hotkey->acpi_handle = NULL;
+ fujitsu_laptop->acpi_handle = NULL;
return 0;
}
-static void acpi_fujitsu_hotkey_press(int keycode)
+static void acpi_fujitsu_laptop_press(int keycode)
{
- struct input_dev *input = fujitsu_hotkey->input;
+ struct input_dev *input = fujitsu_laptop->input;
int status;
- status = kfifo_in_locked(&fujitsu_hotkey->fifo,
+ status = kfifo_in_locked(&fujitsu_laptop->fifo,
(unsigned char *)&keycode, sizeof(keycode),
- &fujitsu_hotkey->fifo_lock);
+ &fujitsu_laptop->fifo_lock);
if (status != sizeof(keycode)) {
vdbg_printk(FUJLAPTOP_DBG_WARN,
"Could not push keycode [0x%x]\n", keycode);
@@ -1054,16 +1054,16 @@ static void acpi_fujitsu_hotkey_press(int keycode)
"Push keycode into ringbuffer [%d]\n", keycode);
}
-static void acpi_fujitsu_hotkey_release(void)
+static void acpi_fujitsu_laptop_release(void)
{
- struct input_dev *input = fujitsu_hotkey->input;
+ struct input_dev *input = fujitsu_laptop->input;
int keycode, status;
while (true) {
- status = kfifo_out_locked(&fujitsu_hotkey->fifo,
+ status = kfifo_out_locked(&fujitsu_laptop->fifo,
(unsigned char *)&keycode,
sizeof(keycode),
- &fujitsu_hotkey->fifo_lock);
+ &fujitsu_laptop->fifo_lock);
if (status != sizeof(keycode))
return;
input_report_key(input, keycode, 0);
@@ -1073,14 +1073,14 @@ static void acpi_fujitsu_hotkey_release(void)
}
}
-static void acpi_fujitsu_hotkey_notify(struct acpi_device *device, u32 event)
+static void acpi_fujitsu_laptop_notify(struct acpi_device *device, u32 event)
{
struct input_dev *input;
int keycode;
unsigned int irb = 1;
int i;
- input = fujitsu_hotkey->input;
+ input = fujitsu_laptop->input;
if (event != ACPI_FUJITSU_NOTIFY_CODE1) {
keycode = KEY_UNKNOWN;
@@ -1093,8 +1093,8 @@ static void acpi_fujitsu_hotkey_notify(struct acpi_device *device, u32 event)
return;
}
- if (fujitsu_hotkey->rfkill_supported)
- fujitsu_hotkey->rfkill_state =
+ if (fujitsu_laptop->rfkill_supported)
+ fujitsu_laptop->rfkill_state =
call_fext_func(FUNC_RFKILL, 0x4, 0x0, 0x0);
i = 0;
@@ -1128,16 +1128,16 @@ static void acpi_fujitsu_hotkey_notify(struct acpi_device *device, u32 event)
}
if (keycode > 0)
- acpi_fujitsu_hotkey_press(keycode);
+ acpi_fujitsu_laptop_press(keycode);
else if (keycode == 0)
- acpi_fujitsu_hotkey_release();
+ acpi_fujitsu_laptop_release();
}
/* On some models (first seen on the Skylake-based Lifebook
* E736/E746/E756), the touchpad toggle hotkey (Fn+F4) is
* handled in software; its state is queried using FUNC_RFKILL
*/
- if ((fujitsu_hotkey->rfkill_supported & BIT(26)) &&
+ if ((fujitsu_laptop->rfkill_supported & BIT(26)) &&
(call_fext_func(FUNC_RFKILL, 0x1, 0x0, 0x0) & BIT(26))) {
keycode = KEY_TOUCHPAD_TOGGLE;
input_report_key(input, keycode, 1);
@@ -1166,25 +1166,25 @@ static struct acpi_driver acpi_fujitsu_bl_driver = {
},
};
-static const struct acpi_device_id fujitsu_hotkey_device_ids[] = {
- {ACPI_FUJITSU_HOTKEY_HID, 0},
+static const struct acpi_device_id fujitsu_laptop_device_ids[] = {
+ {ACPI_FUJITSU_LAPTOP_HID, 0},
{"", 0},
};
-static struct acpi_driver acpi_fujitsu_hotkey_driver = {
- .name = ACPI_FUJITSU_HOTKEY_DRIVER_NAME,
+static struct acpi_driver acpi_fujitsu_laptop_driver = {
+ .name = ACPI_FUJITSU_LAPTOP_DRIVER_NAME,
.class = ACPI_FUJITSU_CLASS,
- .ids = fujitsu_hotkey_device_ids,
+ .ids = fujitsu_laptop_device_ids,
.ops = {
- .add = acpi_fujitsu_hotkey_add,
- .remove = acpi_fujitsu_hotkey_remove,
- .notify = acpi_fujitsu_hotkey_notify,
+ .add = acpi_fujitsu_laptop_add,
+ .remove = acpi_fujitsu_laptop_remove,
+ .notify = acpi_fujitsu_laptop_notify,
},
};
static const struct acpi_device_id fujitsu_ids[] __used = {
{ACPI_FUJITSU_BL_HID, 0},
- {ACPI_FUJITSU_HOTKEY_HID, 0},
+ {ACPI_FUJITSU_LAPTOP_HID, 0},
{"", 0}
};
MODULE_DEVICE_TABLE(acpi, fujitsu_ids);
@@ -1255,18 +1255,18 @@ static int __init fujitsu_init(void)
if (ret)
goto fail_backlight;
- /* Register hotkey driver */
+ /* Register laptop driver */
- fujitsu_hotkey = kzalloc(sizeof(struct fujitsu_hotkey_t), GFP_KERNEL);
- if (!fujitsu_hotkey) {
+ fujitsu_laptop = kzalloc(sizeof(struct fujitsu_laptop), GFP_KERNEL);
+ if (!fujitsu_laptop) {
ret = -ENOMEM;
- goto fail_hotkey;
+ goto fail_laptop;
}
- result = acpi_bus_register_driver(&acpi_fujitsu_hotkey_driver);
+ result = acpi_bus_register_driver(&acpi_fujitsu_laptop_driver);
if (result < 0) {
ret = -ENODEV;
- goto fail_hotkey1;
+ goto fail_laptop1;
}
/* Sync backlight power status (needs FUJ02E3 device, hence deferred) */
@@ -1281,9 +1281,9 @@ static int __init fujitsu_init(void)
return 0;
-fail_hotkey1:
- kfree(fujitsu_hotkey);
-fail_hotkey:
+fail_laptop1:
+ kfree(fujitsu_laptop);
+fail_laptop:
platform_driver_unregister(&fujitsupf_driver);
fail_backlight:
backlight_device_unregister(fujitsu_bl->bl_device);
@@ -1304,9 +1304,9 @@ static int __init fujitsu_init(void)
static void __exit fujitsu_cleanup(void)
{
- acpi_bus_unregister_driver(&acpi_fujitsu_hotkey_driver);
+ acpi_bus_unregister_driver(&acpi_fujitsu_laptop_driver);
- kfree(fujitsu_hotkey);
+ kfree(fujitsu_laptop);
platform_driver_unregister(&fujitsupf_driver);
--
2.11.1
[toc] | [prev] | [next] | [standalone]
| From | Jonathan Woithe <jwoithe@just42.net> |
|---|---|
| Date | 2017-02-09 00:10 +0100 |
| Message-ID | <t8Jx8-6AK-19@gated-at.bofh.it> |
| In reply to | #1576619 |
Hi Michael
Thanks very much for the work you've put in to clean up these patches. I
very much appreciate it. I will go through them myself in the next day or
so, and most importantly test them on my hardware to confirm there are no
regressions. Some initial comments follow.
On Wed, Feb 08, 2017 at 02:46:23PM +0100, Micha?? K??pie?? wrote:
> Some of these patches raise checkpatch warnings. This is intentional,
> in order to make renames clean and easy to review.
For what it's worth, I was also planning to go through and fix the
checkpatch warnings in this driver, but held off until the issues raised by
Alan's patches were addressed (essentially to make it easier to apply those
old patches. If you are planning on doing this then I won't start it. :-)
On to the ommitted changes from Alan's orginal patch series.
> - Restructure fujitsu_init(). Quite frankly, I am pretty sure Alan
> did not test his series on Fujitsu hardware with a logo lamp and/or
> keyboard lamps.
Back in 2009 I don't think there were many models with these features. This
observation is therefore quite possible. I'm more than happy to leave
fujitsu_init()'s structure as is, especially given that related issues will
be addressed in your furture patch series.
> - Bail out when FUJ02B1 is not present. It could have been deemed
> correct back in the day, but we now know that Fujitsu started
> shipping devices without that ACPI device present, though with
> FUJ02E3 still in place. These two ACPI devices are independent and
> thus should not rely on each other's presence.
Agreed. You are right, in 2009 I think it seemed like a good idea but it's
clearly no longer valid.
> - Move keycode[1-5] fields to struct fujitsu_laptop. Doing this
> causes ordering issues inside fujitsu_init(), while a patch series I
> have queued that makes fujitsu-laptop use a sparse keymap removes
> these fields altogether.
This makes sense.
> - Allocate and free fujitsu_bl and fujitsu_laptop inside ACPI
> callbacks. While elegant and correct, this also causes ordering
> issues inside fujitsu_init() and can only be done once platform
> device registration is properly fixed.
Agreed.
> - Sync backlight power status in acpi_fujitsu_bl_add(). The long-term
> objective for fujitsu-laptop should be to achieve a clean split
> between the backlight-related part and the laptop-related part.
> This change keeps both parts intertwined. My fujitsu_init() cleanup
> series contains a similar fix, but I have since found a different
> solution to this problem which I will post once Alan's rebased
> series gets applied.
I'm happy to wait to see this alternative solution. In the meantime,
keeping the two parts independent is a good idea.
> - Remove dmi_check_cb_common(). The sparse keymap series I have
> queued gets rid of this function without introducing an additional
> field inside struct fujitsu_laptop.
Ok.
> Moreover, some other minor changes present in original patch 1/4 were
> left out. A brief discussion of each such case follows.
>
> -#define ACPI_FUJITSU_BL_NOTIFY_CODE1 0x80
> +#define ACPI_FUJITSU_NOTIFY_CODE1 0x80
>
> Notify code 0x80 is used by both parts of the driver (the
> brightness-related one and the laptop-related one) and thus it should
> not be annotated using the "_BL" infix specific to brightness-related
> code.
Agreed. Perhaps back in the day it was thought that it did only apply to
the backlight but with the benefit of hindsight this is clearly not the
case.
> -#if defined(CONFIG_LEDS_CLASS) || defined(CONFIG_LEDS_CLASS_MODULE)
> ...
> -#endif
>
> This does not really bring any benefit while breaking consistency with
> other parts of the driver.
Fair enough.
>
> - dmi_check_system(fujitsu_laptop_dmi_table);
> + dmi_check_system(fujitsu_dmi_table);
>
> The original patches moved this dmi_check_system() call to
> acpi_fujitsu_laptop_add(), which justifies renaming fujitsu_dmi_table to
> fujitsu_laptop_dmi_table. However, for reasons discussed above, the
> rebased patches leave the dmi_check_system() call inside fujitsu_init(),
> thus making the rename dubious.
Agreed. In light of the reasons given this makes no sense.
> - DMI_MATCH(DMI_SYS_VENDOR, "FUJITSU SIEMENS"),
> + DMI_MATCH(DMI_SYS_VENDOR, "FUJITSU_LAPTOP SIEMENS"),
>
> There are multiple changes like this in original patch 1/4. They are
> obviously wrong and were probably introduced by an unreviewed automatic
> replacement.
I noticed this myself; they are clearly incorrect and have the hallmarks of
a global search/replace operation. They were omitted in the initial RFC
series I sent through for this reason.
[Blank line removal]
> There are four instances of such a removal in original patch 1/4. They
> are obviously correct, though they all happen inside functions related
> to platform device attributes which I hope will soon get removed
> altogether. Thus I refrained from applying these to reduce churn (at
> least a bit).
I'm happy with that. The future checkpatch cleanup series could address
these if they are deemed important enough.
> Finally, some of the changes suggested by Alan were already applied
> along the way:
>
> - kmalloc() + memset() occurences were changed to kzalloc() by commit
> 6c75dd0f965b ("drivers/platform/x86: Use kzalloc").
>
> - Unused debug macros were removed by commit 00816e1b3839
> ("fujitsu-laptop: Remove unused macros").
The entire debug structure has been revised since Alan's original patch was
made, making most if not all of the debug changes unnecessary.
My ack will follow once I've tested this series on my hardware. I will try
to get to this in the next 24 hours.
Regards
jonathan
[toc] | [prev] | [next] | [standalone]
| From | Michał Kępień <kernel@kempniu.pl> |
|---|---|
| Date | 2017-02-09 08:40 +0100 |
| Message-ID | <t8RuG-3aL-9@gated-at.bofh.it> |
| In reply to | #1577177 |
> Hi Michael
>
> Thanks very much for the work you've put in to clean up these patches. I
> very much appreciate it. I will go through them myself in the next day or
> so, and most importantly test them on my hardware to confirm there are no
> regressions.
Thanks! I did my best to keep track of everything, but something surely
could have slipped through, given the number of changes Alan's patches
introduce.
> Some initial comments follow.
>
> On Wed, Feb 08, 2017 at 02:46:23PM +0100, Micha?? K??pie?? wrote:
> > Some of these patches raise checkpatch warnings. This is intentional,
> > in order to make renames clean and easy to review.
>
> For what it's worth, I was also planning to go through and fix the
> checkpatch warnings in this driver, but held off until the issues raised by
> Alan's patches were addressed (essentially to make it easier to apply those
> old patches. If you are planning on doing this then I won't start it. :-)
I have a few draft patch series prepared that contain "real" cleanups
and incidentally also lower the total count of checkpatch issues in the
driver from ~70 to ~20 (ballpark figures). I think it is way better to
fix as many checkpatch issues as possible while also fixing actual code
and then iron out the leftovers than to first generate a lot of churn
which fixes spaces and newlines and then fix the code anyway. So yeah,
I would hold off pure checkpatch fixes until the more pressing matters
get sorted out.
>
> On to the ommitted changes from Alan's orginal patch series.
>
> > - Restructure fujitsu_init(). Quite frankly, I am pretty sure Alan
> > did not test his series on Fujitsu hardware with a logo lamp and/or
> > keyboard lamps.
>
> Back in 2009 I don't think there were many models with these features. This
> observation is therefore quite possible. I'm more than happy to leave
> fujitsu_init()'s structure as is, especially given that related issues will
> be addressed in your furture patch series.
Yes, they will. How these fixes will exactly look depends on the
feedback I get from Darren and Andy.
>
> > - Bail out when FUJ02B1 is not present. It could have been deemed
> > correct back in the day, but we now know that Fujitsu started
> > shipping devices without that ACPI device present, though with
> > FUJ02E3 still in place. These two ACPI devices are independent and
> > thus should not rely on each other's presence.
>
> Agreed. You are right, in 2009 I think it seemed like a good idea but it's
> clearly no longer valid.
>
> > - Move keycode[1-5] fields to struct fujitsu_laptop. Doing this
> > causes ordering issues inside fujitsu_init(), while a patch series I
> > have queued that makes fujitsu-laptop use a sparse keymap removes
> > these fields altogether.
>
> This makes sense.
>
> > - Allocate and free fujitsu_bl and fujitsu_laptop inside ACPI
> > callbacks. While elegant and correct, this also causes ordering
> > issues inside fujitsu_init() and can only be done once platform
> > device registration is properly fixed.
>
> Agreed.
>
> > - Sync backlight power status in acpi_fujitsu_bl_add(). The long-term
> > objective for fujitsu-laptop should be to achieve a clean split
> > between the backlight-related part and the laptop-related part.
> > This change keeps both parts intertwined. My fujitsu_init() cleanup
> > series contains a similar fix, but I have since found a different
> > solution to this problem which I will post once Alan's rebased
> > series gets applied.
>
> I'm happy to wait to see this alternative solution. In the meantime,
> keeping the two parts independent is a good idea.
>
> > - Remove dmi_check_cb_common(). The sparse keymap series I have
> > queued gets rid of this function without introducing an additional
> > field inside struct fujitsu_laptop.
>
> Ok.
>
> > Moreover, some other minor changes present in original patch 1/4 were
> > left out. A brief discussion of each such case follows.
> >
> > -#define ACPI_FUJITSU_BL_NOTIFY_CODE1 0x80
> > +#define ACPI_FUJITSU_NOTIFY_CODE1 0x80
> >
> > Notify code 0x80 is used by both parts of the driver (the
> > brightness-related one and the laptop-related one) and thus it should
> > not be annotated using the "_BL" infix specific to brightness-related
> > code.
>
> Agreed. Perhaps back in the day it was thought that it did only apply to
> the backlight but with the benefit of hindsight this is clearly not the
> case.
>
> > -#if defined(CONFIG_LEDS_CLASS) || defined(CONFIG_LEDS_CLASS_MODULE)
> > ...
> > -#endif
> >
> > This does not really bring any benefit while breaking consistency with
> > other parts of the driver.
>
> Fair enough.
>
> >
> > - dmi_check_system(fujitsu_laptop_dmi_table);
> > + dmi_check_system(fujitsu_dmi_table);
> >
> > The original patches moved this dmi_check_system() call to
> > acpi_fujitsu_laptop_add(), which justifies renaming fujitsu_dmi_table to
> > fujitsu_laptop_dmi_table. However, for reasons discussed above, the
> > rebased patches leave the dmi_check_system() call inside fujitsu_init(),
> > thus making the rename dubious.
>
> Agreed. In light of the reasons given this makes no sense.
>
> > - DMI_MATCH(DMI_SYS_VENDOR, "FUJITSU SIEMENS"),
> > + DMI_MATCH(DMI_SYS_VENDOR, "FUJITSU_LAPTOP SIEMENS"),
> >
> > There are multiple changes like this in original patch 1/4. They are
> > obviously wrong and were probably introduced by an unreviewed automatic
> > replacement.
>
> I noticed this myself; they are clearly incorrect and have the hallmarks of
> a global search/replace operation. They were omitted in the initial RFC
> series I sent through for this reason.
>
> [Blank line removal]
> > There are four instances of such a removal in original patch 1/4. They
> > are obviously correct, though they all happen inside functions related
> > to platform device attributes which I hope will soon get removed
> > altogether. Thus I refrained from applying these to reduce churn (at
> > least a bit).
>
> I'm happy with that. The future checkpatch cleanup series could address
> these if they are deemed important enough.
Yes, I have this specific cleanup queued in a branch containing
miscellaneous fixes anyway, so it should not get lost in the noise.
>
> > Finally, some of the changes suggested by Alan were already applied
> > along the way:
> >
> > - kmalloc() + memset() occurences were changed to kzalloc() by commit
> > 6c75dd0f965b ("drivers/platform/x86: Use kzalloc").
> >
> > - Unused debug macros were removed by commit 00816e1b3839
> > ("fujitsu-laptop: Remove unused macros").
>
> The entire debug structure has been revised since Alan's original patch was
> made, making most if not all of the debug changes unnecessary.
>
> My ack will follow once I've tested this series on my hardware. I will try
> to get to this in the next 24 hours.
Great, thanks!
--
Best regards,
Michał Kępień
[toc] | [prev] | [next] | [standalone]
| From | Jonathan Woithe <jwoithe@just42.net> |
|---|---|
| Date | 2017-02-10 01:20 +0100 |
| Message-ID | <t976q-4Cv-17@gated-at.bofh.it> |
| In reply to | #1576619 |
On Wed, Feb 08, 2017 at 02:46:23PM +0100, Micha?? K??pie?? wrote: > This series of patches was originally submitted by Alan Jenkins in > September 2009. ... I have applied and tested this patch series on an S7020. Being an older model (and the one the driver was originally targetting) means that many driver functions cannot be tested because the respective hardware isn't present. However, the driver was correctly loaded and the backlight controls continue to work as they should. That is, I see no regressions on the S7020 hardware. Patches 1-6 are essentially renames to tidy up the naming convention used throughout the driver. The original names date to the first implementation which targeted more limited hardware than what we have now, hence the need to generalise some of the fields, structure names and so on with the benefit of hindsight. Patch 7: I agree with this approach. There's no need to clobber the actual error code from acpi_bus_register_driver(). Patch 8: Good. I can confirm that the backlight device is correctly registered when FUJ02B1 is present. I couldn't test the case where FUJ02B1 is not present because my hardware has the FUJ02B1 device, but can see no reason why it won't work as described. Patch 9: I think the original reasoning was that SBL2 was only ever seen on laptops which required the hotkey keycode overrides. However, testing for SBL2 across all models should not change practical behaviour. It also breaks the somewhat arbitrary dependency between SBL2 and the need for overrides which may not hold true for future hardware. Patch 10: Agreed, there is no longer a need for these MODULE_ALIAS entries. In summary, I see no issues with this patch series which provides a much needed clean up of the code and naming conventions within the fujitsu-laptop driver. I'm happy for this series (patches 1-10/10) to be applied. Signed-off-by: Jonathan Woithe <jwoithe@just42.net> Regards jonathan
[toc] | [prev] | [next] | [standalone]
| From | Andy Shevchenko <andy.shevchenko@gmail.com> |
|---|---|
| Date | 2017-02-10 01:50 +0100 |
| Message-ID | <t97zr-4Mk-7@gated-at.bofh.it> |
| In reply to | #1578068 |
On Fri, Feb 10, 2017 at 2:16 AM, Jonathan Woithe <jwoithe@just42.net> wrote: > On Wed, Feb 08, 2017 at 02:46:23PM +0100, Micha?? K??pie?? wrote: > In summary, I see no issues with this patch series which provides a much > needed clean up of the code and naming conventions within the fujitsu-laptop > driver. I'm happy for this series (patches 1-10/10) to be applied. > > Signed-off-by: Jonathan Woithe <jwoithe@just42.net> I have noticed people start using SoB for the code they are maintaining w/o sending any pull requests. It is okay, but there is, as Wolfram pointed, a downside for patchwork users. Patchwork is tracking tags (A/R/T) which by a glance allows to see what patches are acked/reviewed/tested. -- With Best Regards, Andy Shevchenko
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web