[PATCH 0/8] More eeepc-laptop cleanup

[Frans Klaver]

Hi,

This is another round of cleanups in eeepc-laptop. It's mostly shuffling around
stuff, no big things. It's been running on my eeepc for two or three weeks now
without any apparent problems. Comments appreciated.

Thanks,
Frans

Frans Klaver (8):
eeepc-laptop: flatten control flow
eeepc-laptop: don't break user visible strings
eeepc-laptop: define rfkill notifier nodes only once
eeepc-laptop: pull out eeepc_destroy_rfkill
eeepc-laptop: clean up control flow in eeepc_acpi_notify
eeepc-laptop: replace magic numbers with defines
eeepc-laptop: document fan_pwm conversions
eeepc-laptop: don't assume get_acpi succeeds

drivers/platform/x86/eeepc-laptop.c | 244 +++++++++++++++++++-----------------
1 file changed, 127 insertions(+), 117 deletions(-)

[Frans Klaver]

eeepc_get_fan_pwm and eeepc_set_fan_pwm convert the PWM value read from
the fan to a range lmsensors understands. Unfortunately this is only
clear if you are familiar with how lmsensors handles duty cycles.

Introduce two conversion functions that document the goal of these
conversions.

Signed-off-by: Frans Klaver <***@gmail.com>
---
drivers/platform/x86/eeepc-laptop.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index f820bb3..275a239 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -974,18 +974,28 @@ static struct platform_driver platform_driver = {
#define EEEPC_EC_SFB0 0xD0
#define EEEPC_EC_FAN_CTRL (EEEPC_EC_SFB0 + 3) /* Byte containing SF25 */

+static inline int eeepc_pwm_to_lmsensors(int value)
+{
+ return value * 255 / 100;
+}
+
+static inline int eeepc_lmsensors_to_pwm(int value)
+{
+ value = clamp_val(value, 0, 255);
+ return value * 100 / 255;
+}
+
static int eeepc_get_fan_pwm(void)
{
u8 value = 0;

ec_read(EEEPC_EC_FAN_PWM, &value);
- return value * 255 / 100;
+ return eeepc_pwm_to_lmsensors(value);
}

static void eeepc_set_fan_pwm(int value)
{
- value = clamp_val(value, 0, 255);
- value = value * 100 / 255;
+ value = eeepc_lmsensors_to_pwm(value);
ec_write(EEEPC_EC_FAN_PWM, value);
}

[Frans Klaver]

The rfkill notifier node names are used in three different places. As a
matter of style, it is better to store them somewhere and have the
compiler warn us about typos in the function arguments.

Signed-off-by: Frans Klaver <***@gmail.com>
---
drivers/platform/x86/eeepc-laptop.c | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index 6e3be01..e92ea41 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -819,11 +819,15 @@ static int eeepc_new_rfkill(struct eeepc_laptop *eeepc,
return 0;
}

+static char EEEPC_RFKILL_NODE_1[] = "\\_SB.PCI0.P0P5";
+static char EEEPC_RFKILL_NODE_2[] = "\\_SB.PCI0.P0P6";
+static char EEEPC_RFKILL_NODE_3[] = "\\_SB.PCI0.P0P7";
+
static void eeepc_rfkill_exit(struct eeepc_laptop *eeepc)
{
- eeepc_unregister_rfkill_notifier(eeepc, "\\_SB.PCI0.P0P5");
- eeepc_unregister_rfkill_notifier(eeepc, "\\_SB.PCI0.P0P6");
- eeepc_unregister_rfkill_notifier(eeepc, "\\_SB.PCI0.P0P7");
+ eeepc_unregister_rfkill_notifier(eeepc, EEEPC_RFKILL_NODE_1);
+ eeepc_unregister_rfkill_notifier(eeepc, EEEPC_RFKILL_NODE_2);
+ eeepc_unregister_rfkill_notifier(eeepc, EEEPC_RFKILL_NODE_3);
if (eeepc->wlan_rfkill) {
rfkill_unregister(eeepc->wlan_rfkill);
rfkill_destroy(eeepc->wlan_rfkill);
@@ -895,9 +899,9 @@ static int eeepc_rfkill_init(struct eeepc_laptop *eeepc)
if (result == -EBUSY)
result = 0;

- eeepc_register_rfkill_notifier(eeepc, "\\_SB.PCI0.P0P5");
- eeepc_register_rfkill_notifier(eeepc, "\\_SB.PCI0.P0P6");
- eeepc_register_rfkill_notifier(eeepc, "\\_SB.PCI0.P0P7");
+ eeepc_register_rfkill_notifier(eeepc, EEEPC_RFKILL_NODE_1);
+ eeepc_register_rfkill_notifier(eeepc, EEEPC_RFKILL_NODE_2);
+ eeepc_register_rfkill_notifier(eeepc, EEEPC_RFKILL_NODE_3);

exit:
if (result && result != -ENODEV)
@@ -933,9 +937,9 @@ static int eeepc_hotk_restore(struct device *device)

/* Refresh both wlan rfkill state and pci hotplug */
if (eeepc->wlan_rfkill) {
- eeepc_rfkill_hotplug_update(eeepc, "\\_SB.PCI0.P0P5");
- eeepc_rfkill_hotplug_update(eeepc, "\\_SB.PCI0.P0P6");
- eeepc_rfkill_hotplug_update(eeepc, "\\_SB.PCI0.P0P7");
+ eeepc_rfkill_hotplug_update(eeepc, EEEPC_RFKILL_NODE_1);
+ eeepc_rfkill_hotplug_update(eeepc, EEEPC_RFKILL_NODE_2);
+ eeepc_rfkill_hotplug_update(eeepc, EEEPC_RFKILL_NODE_3);
}

if (eeepc->bluetooth_rfkill)

[Frans Klaver]

In eeepc_hotk_thaw, we assume that get_acpi() will effectively return a
bool. However, it is possible that get_acpi() returns an error instead.
We should not be writing error values to the ACPI device, even though
it's quite possible that we couldn't contact the ACPI device in the
first place.

Signed-off-by: Frans Klaver <***@gmail.com>
---
drivers/platform/x86/eeepc-laptop.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index 275a239..14f79ef 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -911,7 +911,7 @@ static int eeepc_hotk_thaw(struct device *device)
struct eeepc_laptop *eeepc = dev_get_drvdata(device);

if (eeepc->wlan_rfkill) {
- bool wlan;
+ int wlan;

/*
* Work around bios bug - acpi _PTS turns off the wireless led
@@ -919,7 +919,8 @@ static int eeepc_hotk_thaw(struct device *device)
* we should kick it ourselves in case hibernation is aborted.
*/
wlan = get_acpi(eeepc, CM_ASL_WLAN);
- set_acpi(eeepc, CM_ASL_WLAN, wlan);
+ if (wlan >= 0)
+ set_acpi(eeepc, CM_ASL_WLAN, wlan);
}

return 0;

[Frans Klaver]

eeepc_[gs]et_fan_ctrl uses some magic numbers. These numbers mean
something more than just the number. Describe them with macros instead
of comments in one of the functions.

Signed-off-by: Frans Klaver <***@gmail.com>
---
drivers/platform/x86/eeepc-laptop.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index 21ffe1f..f820bb3 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -999,15 +999,19 @@ static int eeepc_get_fan_rpm(void)
return high << 8 | low;
}

+#define EEEPC_EC_FAN_CTRL_BIT 0x02
+#define EEEPC_FAN_CTRL_MANUAL 1
+#define EEEPC_FAN_CTRL_AUTO 2
+
static int eeepc_get_fan_ctrl(void)
{
u8 value = 0;

ec_read(EEEPC_EC_FAN_CTRL, &value);
- if (value & 0x02)
- return 1; /* manual */
+ if (value & EEEPC_EC_FAN_CTRL_BIT)
+ return EEEPC_FAN_CTRL_MANUAL;
else
- return 2; /* automatic */
+ return EEEPC_FAN_CTRL_AUTO;
}

static void eeepc_set_fan_ctrl(int manual)
@@ -1015,10 +1019,10 @@ static void eeepc_set_fan_ctrl(int manual)
u8 value = 0;

ec_read(EEEPC_EC_FAN_CTRL, &value);
- if (manual == 1)
- value |= 0x02;
+ if (manual == EEEPC_FAN_CTRL_MANUAL)
+ value |= EEEPC_EC_FAN_CTRL_BIT;
else
- value &= ~0x02;
+ value &= ~EEEPC_EC_FAN_CTRL_BIT;
ec_write(EEEPC_EC_FAN_CTRL, value);
}

[Frans Klaver]

eeepc_acpi_notify increases the indentation level to a whopping four. If
we revise the conditions a bit, we can reduce that to a more soothing
two and satisfy the indentation guidelines in Documentation/CodingStyle.

Remove an unwanted space while we're in the neighbourhood.

Signed-off-by: Frans Klaver <***@gmail.com>
---
drivers/platform/x86/eeepc-laptop.c | 53 ++++++++++++++++++-------------------
1 file changed, 26 insertions(+), 27 deletions(-)

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index 73e8d39..21ffe1f 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -1213,7 +1213,7 @@ static void eeepc_input_exit(struct eeepc_laptop *eeepc)
static void eeepc_input_notify(struct eeepc_laptop *eeepc, int event)
{
if (!eeepc->inputdev)
- return ;
+ return;
if (!sparse_keymap_report_event(eeepc->inputdev, event, 1, true))
pr_info("Unknown key %x pressed\n", event);
}
@@ -1222,6 +1222,7 @@ static void eeepc_acpi_notify(struct acpi_device *device, u32 event)
{
struct eeepc_laptop *eeepc = acpi_driver_data(device);
u16 count;
+ int old_brightness, new_brightness;

if (event > ACPI_MAX_SYS_NOTIFY)
return;
@@ -1231,34 +1232,32 @@ static void eeepc_acpi_notify(struct acpi_device *device, u32 event)
count);

/* Brightness events are special */
- if (event >= NOTIFY_BRN_MIN && event <= NOTIFY_BRN_MAX) {
-
- /* Ignore them completely if the acpi video driver is used */
- if (eeepc->backlight_device != NULL) {
- int old_brightness, new_brightness;
-
- /* Update the backlight device. */
- old_brightness = eeepc_backlight_notify(eeepc);
-
- /* Convert event to keypress (obsolescent hack) */
- new_brightness = event - NOTIFY_BRN_MIN;
-
- if (new_brightness < old_brightness) {
- event = NOTIFY_BRN_MIN; /* brightness down */
- } else if (new_brightness > old_brightness) {
- event = NOTIFY_BRN_MAX; /* brightness up */
- } else {
- /*
- * no change in brightness - already at min/max,
- * event will be desired value (or else ignored)
- */
- }
- eeepc_input_notify(eeepc, event);
- }
- } else {
- /* Everything else is a bona-fide keypress event */
+ if (event < NOTIFY_BRN_MIN || event > NOTIFY_BRN_MAX) {
eeepc_input_notify(eeepc, event);
+ return;
+ }
+
+ /* Ignore them completely if the acpi video driver is used */
+ if (!eeepc->backlight_device)
+ return;
+
+ /* Update the backlight device. */
+ old_brightness = eeepc_backlight_notify(eeepc);
+
+ /* Convert event to keypress (obsolescent hack) */
+ new_brightness = event - NOTIFY_BRN_MIN;
+
+ if (new_brightness < old_brightness) {
+ event = NOTIFY_BRN_MIN; /* brightness down */
+ } else if (new_brightness > old_brightness) {
+ event = NOTIFY_BRN_MAX; /* brightness up */
+ } else {
+ /*
+ * no change in brightness - already at min/max,
+ * event will be desired value (or else ignored)
+ */
}
+ eeepc_input_notify(eeepc, event);
}

static void eeepc_dmi_check(struct eeepc_laptop *eeepc)

[Frans Klaver]

In eeepc_rfkill_exit, we implement the same code four times. Pull out a
function that cleans up an rfkill object to get rid of the duplication.

Signed-off-by: Frans Klaver <***@gmail.com>
---
drivers/platform/x86/eeepc-laptop.c | 34 ++++++++++++++--------------------
1 file changed, 14 insertions(+), 20 deletions(-)

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index e92ea41..73e8d39 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -823,35 +823,29 @@ static char EEEPC_RFKILL_NODE_1[] = "\\_SB.PCI0.P0P5";
static char EEEPC_RFKILL_NODE_2[] = "\\_SB.PCI0.P0P6";
static char EEEPC_RFKILL_NODE_3[] = "\\_SB.PCI0.P0P7";

+static inline void eeepc_destroy_rfkill(struct rfkill **rfkill)
+{
+ if (!*rfkill)
+ return;
+ rfkill_unregister(*rfkill);
+ rfkill_destroy(*rfkill);
+ *rfkill = NULL;
+}
+
static void eeepc_rfkill_exit(struct eeepc_laptop *eeepc)
{
eeepc_unregister_rfkill_notifier(eeepc, EEEPC_RFKILL_NODE_1);
eeepc_unregister_rfkill_notifier(eeepc, EEEPC_RFKILL_NODE_2);
eeepc_unregister_rfkill_notifier(eeepc, EEEPC_RFKILL_NODE_3);
- if (eeepc->wlan_rfkill) {
- rfkill_unregister(eeepc->wlan_rfkill);
- rfkill_destroy(eeepc->wlan_rfkill);
- eeepc->wlan_rfkill = NULL;
- }
+
+ eeepc_destroy_rfkill(&eeepc->wlan_rfkill);

if (eeepc->hotplug_slot)
pci_hp_deregister(eeepc->hotplug_slot);

- if (eeepc->bluetooth_rfkill) {
- rfkill_unregister(eeepc->bluetooth_rfkill);
- rfkill_destroy(eeepc->bluetooth_rfkill);
- eeepc->bluetooth_rfkill = NULL;
- }
- if (eeepc->wwan3g_rfkill) {
- rfkill_unregister(eeepc->wwan3g_rfkill);
- rfkill_destroy(eeepc->wwan3g_rfkill);
- eeepc->wwan3g_rfkill = NULL;
- }
- if (eeepc->wimax_rfkill) {
- rfkill_unregister(eeepc->wimax_rfkill);
- rfkill_destroy(eeepc->wimax_rfkill);
- eeepc->wimax_rfkill = NULL;
- }
+ eeepc_destroy_rfkill(&eeepc->bluetooth_rfkill);
+ eeepc_destroy_rfkill(&eeepc->wwan3g_rfkill);
+ eeepc_destroy_rfkill(&eeepc->wimax_rfkill);
}

static int eeepc_rfkill_init(struct eeepc_laptop *eeepc)

[Frans Klaver]

As per Documentation/CodingStyle ch. 2, it is preferred that we don't
break user visible strings, in order to allow users to grep for them.

Signed-off-by: Frans Klaver <***@gmail.com>
---
drivers/platform/x86/eeepc-laptop.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index bb098e5..6e3be01 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -417,8 +417,7 @@ static ssize_t cpufv_disabled_store(struct device *dev,
switch (value) {
case 0:
if (eeepc->cpufv_disabled)
- pr_warn("cpufv enabled (not officially supported "
- "on this model)\n");
+ pr_warn("cpufv enabled (not officially supported on this model)\n");
eeepc->cpufv_disabled = false;
return count;
case 1:
@@ -604,12 +603,10 @@ static void eeepc_rfkill_hotplug(struct eeepc_laptop *eeepc, acpi_handle handle)
absent = (l == 0xffffffff);

if (blocked != absent) {
- pr_warn("BIOS says wireless lan is %s, "
- "but the pci device is %s\n",
+ pr_warn("BIOS says wireless lan is %s, but the pci device is %s\n",
blocked ? "blocked" : "unblocked",
absent ? "absent" : "present");
- pr_warn("skipped wireless hotplug as probably "
- "inappropriate for this model\n");
+ pr_warn("skipped wireless hotplug as probably inappropriate for this model\n");
goto out_put_dev;
}

@@ -1295,8 +1292,8 @@ static void eeepc_dmi_check(struct eeepc_laptop *eeepc)
*/
if (strcmp(model, "701") == 0 || strcmp(model, "702") == 0) {
eeepc->cpufv_disabled = true;
- pr_info("model %s does not officially support setting cpu "
- "speed\n", model);
+ pr_info("model %s does not officially support setting cpu speed\n",
+ model);
pr_info("cpufv disabled to avoid instability\n");
}

@@ -1322,8 +1319,8 @@ static void cmsg_quirk(struct eeepc_laptop *eeepc, int cm, const char *name)
Check if cm_getv[cm] works and, if yes, assume cm should be set. */
if (!(eeepc->cm_supported & (1 << cm))
&& !read_acpi_int(eeepc->handle, cm_getv[cm], &dummy)) {
- pr_info("%s (%x) not reported by BIOS,"
- " enabling anyway\n", name, 1 << cm);
+ pr_info("%s (%x) not reported by BIOS, enabling anyway\n",
+ name, 1 << cm);
eeepc->cm_supported |= 1 << cm;
}
}

[Frans Klaver]

In eeepc_rfkill_hotplug there's an if statement with a big tail that
ends right before the out_unlock label. We might as well invert the
condition and jump to out_unlock in that case, pretty much like the rest
of the code does. This removes an indentation level for a large chunk of
code and also stops suggesting there might be an else clause.

Signed-off-by: Frans Klaver <***@gmail.com>
---
drivers/platform/x86/eeepc-laptop.c | 89 +++++++++++++++++++------------------
1 file changed, 45 insertions(+), 44 deletions(-)

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index db79902..bb098e5 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -580,59 +580,60 @@ static void eeepc_rfkill_hotplug(struct eeepc_laptop *eeepc, acpi_handle handle)
mutex_lock(&eeepc->hotplug_lock);
pci_lock_rescan_remove();

- if (eeepc->hotplug_slot) {
- port = acpi_get_pci_dev(handle);
- if (!port) {
- pr_warning("Unable to find port\n");
- goto out_unlock;
- }
+ if (!eeepc->hotplug_slot)
+ goto out_unlock;

- bus = port->subordinate;
+ port = acpi_get_pci_dev(handle);
+ if (!port) {
+ pr_warning("Unable to find port\n");
+ goto out_unlock;
+ }

- if (!bus) {
- pr_warn("Unable to find PCI bus 1?\n");
- goto out_put_dev;
- }
+ bus = port->subordinate;

- if (pci_bus_read_config_dword(bus, 0, PCI_VENDOR_ID, &l)) {
- pr_err("Unable to read PCI config space?\n");
- goto out_put_dev;
- }
+ if (!bus) {
+ pr_warn("Unable to find PCI bus 1?\n");
+ goto out_put_dev;
+ }
+
+ if (pci_bus_read_config_dword(bus, 0, PCI_VENDOR_ID, &l)) {
+ pr_err("Unable to read PCI config space?\n");
+ goto out_put_dev;
+ }

- absent = (l == 0xffffffff);
+ absent = (l == 0xffffffff);

- if (blocked != absent) {
- pr_warn("BIOS says wireless lan is %s, "
- "but the pci device is %s\n",
- blocked ? "blocked" : "unblocked",
- absent ? "absent" : "present");
- pr_warn("skipped wireless hotplug as probably "
- "inappropriate for this model\n");
+ if (blocked != absent) {
+ pr_warn("BIOS says wireless lan is %s, "
+ "but the pci device is %s\n",
+ blocked ? "blocked" : "unblocked",
+ absent ? "absent" : "present");
+ pr_warn("skipped wireless hotplug as probably "
+ "inappropriate for this model\n");
+ goto out_put_dev;
+ }
+
+ if (!blocked) {
+ dev = pci_get_slot(bus, 0);
+ if (dev) {
+ /* Device already present */
+ pci_dev_put(dev);
goto out_put_dev;
}
-
- if (!blocked) {
- dev = pci_get_slot(bus, 0);
- if (dev) {
- /* Device already present */
- pci_dev_put(dev);
- goto out_put_dev;
- }
- dev = pci_scan_single_device(bus, 0);
- if (dev) {
- pci_bus_assign_resources(bus);
- pci_bus_add_device(dev);
- }
- } else {
- dev = pci_get_slot(bus, 0);
- if (dev) {
- pci_stop_and_remove_bus_device(dev);
- pci_dev_put(dev);
- }
+ dev = pci_scan_single_device(bus, 0);
+ if (dev) {
+ pci_bus_assign_resources(bus);
+ pci_bus_add_device(dev);
+ }
+ } else {
+ dev = pci_get_slot(bus, 0);
+ if (dev) {
+ pci_stop_and_remove_bus_device(dev);
+ pci_dev_put(dev);
}
-out_put_dev:
- pci_dev_put(port);
}
+out_put_dev:
+ pci_dev_put(port);

out_unlock:
pci_unlock_rescan_remove();