[PATCH 0/8] More eeepc-laptop cleanup

[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(-)

-- 
2.1.0

[PATCH 1/8] eeepc-laptop: flatten control flow

[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 <fransklaver@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();
-- 
2.1.0

Re: [PATCH 1/8] eeepc-laptop: flatten control flow

[Darren Hart]

On Wed, Oct 22, 2014 at 09:12:36PM +0200, Frans Klaver wrote:
> 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 <fransklaver@gmail.com>

Queued for 3.19, thanks Frans.

-- 
Darren Hart
Intel Open Source Technology Center

[PATCH 2/8] eeepc-laptop: don't break user visible strings

[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 <fransklaver@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;
 	}
 }
-- 
2.1.0

[PATCH 3/8] eeepc-laptop: define rfkill notifier nodes only once

[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 <fransklaver@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)
-- 
2.1.0

Re: [PATCH 3/8] eeepc-laptop: define rfkill notifier nodes only once

[Darren Hart]

On Wed, Oct 22, 2014 at 09:12:38PM +0200, Frans Klaver wrote:
> 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 <fransklaver@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";

So, out of curiosity, any particular reason for static char[] instead of
#define? I see both used frequently and didn't see any advice in CodingStyle.

Regardless, Queued and thanks,

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 3/8] eeepc-laptop: define rfkill notifier nodes only once

[Frans Klaver]

On Tue, Oct 28, 2014 at 6:12 AM, Darren Hart <dvhart@infradead.org> wrote:
> On Wed, Oct 22, 2014 at 09:12:38PM +0200, Frans Klaver wrote:
>> 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 <fransklaver@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";
>
> So, out of curiosity, any particular reason for static char[] instead of
> #define? I see both used frequently and didn't see any advice in CodingStyle.

My expectation is that this is more likely to produce a smaller
binary, but I have no measurements on that to back me up.

I was a bit annoyed by the fact that the acpi functions take a char*
instead of a const char*. I would have preferred static const char[]
in any case.

Thanks,
Frans

[PATCH 4/8] eeepc-laptop: pull out eeepc_destroy_rfkill

[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 <fransklaver@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)
-- 
2.1.0

Re: [PATCH 4/8] eeepc-laptop: pull out eeepc_destroy_rfkill

[Darren Hart]

On Wed, Oct 22, 2014 at 09:12:39PM +0200, Frans Klaver wrote:
> 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 <fransklaver@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;
> +}

In this case the savings is 6 lines at the cost of some legibility as double
pointers are arguably harder to read, and definitely easier to screw up ;-)

I appreciate the goal, but I'm not convinced it is worth it.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 4/8] eeepc-laptop: pull out eeepc_destroy_rfkill

[Frans Klaver]

On Tue, Oct 28, 2014 at 6:19 AM, Darren Hart <dvhart@infradead.org> wrote:
> On Wed, Oct 22, 2014 at 09:12:39PM +0200, Frans Klaver wrote:
>> 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 <fransklaver@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;
>> +}
>
> In this case the savings is 6 lines at the cost of some legibility as double
> pointers are arguably harder to read, and definitely easier to screw up ;-)
>
> I appreciate the goal, but I'm not convinced it is worth it.

Fair enough. If anything the following would be slightly less complicated:

#define eeepc_destroy_rfkill(rfkill) \
do { \
        if (rfkill) { \
                 rfkill_unregister(rfkill); \
                 rfkill_destroy(rfkill); \
                 rkfill = NULL; \
        } \
} while (0);

I don't think any new eeepc's are being produced, so this code is
unlikely to change anyway. Let's leave it as is.

Thanks,
Frans

[PATCH 5/8] eeepc-laptop: clean up control flow in eeepc_acpi_notify

[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 <fransklaver@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)
-- 
2.1.0

Re: [PATCH 5/8] eeepc-laptop: clean up control flow in eeepc_acpi_notify

[Darren Hart]

On Wed, Oct 22, 2014 at 09:12:40PM +0200, Frans Klaver wrote:
> 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 <fransklaver@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;


Variable declarations in order of decreasing line length please.

Otherwise, looks good. Mind sending an update of this one?

-- 
Darren Hart
Intel Open Source Technology Center

[PATCH v2] eeepc-laptop: clean up control flow in eeepc_acpi_notify

[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 <fransklaver@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..03ffbce 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);
 }
@@ -1221,6 +1221,7 @@ static void eeepc_input_notify(struct eeepc_laptop *eeepc, int event)
 static void eeepc_acpi_notify(struct acpi_device *device, u32 event)
 {
 	struct eeepc_laptop *eeepc = acpi_driver_data(device);
+	int old_brightness, new_brightness;
 	u16 count;
 
 	if (event > ACPI_MAX_SYS_NOTIFY)
@@ -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)
-- 
2.1.0

Re: [PATCH v2] eeepc-laptop: clean up control flow in eeepc_acpi_notify

[Darren Hart]

On Sun, Nov 02, 2014 at 10:14:58PM +0100, Frans Klaver wrote:
> 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 <fransklaver@gmail.com>

Applied, thanks. Look for this in my 3.19 pull requests along with the other
cleanups.

-- 
Darren Hart
Intel Open Source Technology Center

[PATCH 6/8] eeepc-laptop: replace magic numbers with defines

[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 <fransklaver@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);
 }
 
-- 
2.1.0

[PATCH 7/8] eeepc-laptop: document fan_pwm conversions

[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 <fransklaver@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);
 }
 
-- 
2.1.0

Re: [PATCH 7/8] eeepc-laptop: document fan_pwm conversions

[Darren Hart]

On Wed, Oct 22, 2014 at 09:12:42PM +0200, Frans Klaver wrote:
> 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 <fransklaver@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;

Says the guy cleaning up all the magic numbers.... ;-)

But yeah, this is fine IMO. Applied.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 7/8] eeepc-laptop: document fan_pwm conversions

[Frans Klaver]

On Tue, Oct 28, 2014 at 6:31 AM, Darren Hart <dvhart@infradead.org> wrote:
> On Wed, Oct 22, 2014 at 09:12:42PM +0200, Frans Klaver wrote:
>> 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 <fransklaver@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;
>
> Says the guy cleaning up all the magic numbers.... ;-)
>
> But yeah, this is fine IMO. Applied.

Heh, I don't think this is any more or less magic than

    #define CONVERT_TO_LMSENSORS (255 / 100)

    value = value * CONVERT_TO_LMSENSORS;

or

    #define lmify(v) v * 255 / 100

    value = lmify(v);

But since you've just applied it, I would say you have pretty much the
same opinion.

Thanks,
Frans

[PATCH 8/8] eeepc-laptop: don't assume get_acpi succeeds

[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 <fransklaver@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;
-- 
2.1.0

Re: [PATCH 8/8] eeepc-laptop: don't assume get_acpi succeeds

[Darren Hart]

On Wed, Oct 22, 2014 at 09:12:43PM +0200, Frans Klaver wrote:
> 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 <fransklaver@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);

And if not? Seems like we should be passing the error code along.

>  	}
>  
>  	return 0;
> -- 
> 2.1.0
> 
> 

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 8/8] eeepc-laptop: don't assume get_acpi succeeds

[Frans Klaver]

On Tue, Oct 28, 2014 at 6:34 AM, Darren Hart <dvhart@infradead.org> wrote:
> On Wed, Oct 22, 2014 at 09:12:43PM +0200, Frans Klaver wrote:
>> 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 <fransklaver@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);
>
> And if not? Seems like we should be passing the error code along.

Wouldn't that mean that you cannot thaw the system if the wlan acpi call fails?

I'd rather have my system up and running without wlan, than I'd have
my system down because we couldn't start wlan. This is of course
assuming that returning an error from eeepc_hotk_thaw means the
thawing stops.

Thanks,
Frans

Re: [PATCH 8/8] eeepc-laptop: don't assume get_acpi succeeds

[Darren Hart]

On Tue, Oct 28, 2014 at 09:09:03AM +0100, Frans Klaver wrote:
> On Tue, Oct 28, 2014 at 6:34 AM, Darren Hart <dvhart@infradead.org> wrote:
> > On Wed, Oct 22, 2014 at 09:12:43PM +0200, Frans Klaver wrote:
> >> 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 <fransklaver@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);
> >
> > And if not? Seems like we should be passing the error code along.
> 
> Wouldn't that mean that you cannot thaw the system if the wlan acpi call fails?

Good question. I traced it back to the platform driver core, and it just
propogates the error, so I'm not sure either.

> 
> I'd rather have my system up and running without wlan, than I'd have
> my system down because we couldn't start wlan. This is of course
> assuming that returning an error from eeepc_hotk_thaw means the
> thawing stops.

I agree, and this is what it does now, and acer-wmi thaw has similar behavior.

I consider this an incremental improvement, so queued.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 0/8] More eeepc-laptop cleanup

[Darren Hart]

On Wed, Oct 22, 2014 at 09:12:35PM +0200, Frans Klaver wrote:
> 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,

Queued for 3.19 except where explicitly noted.

-- 
Darren Hart
Intel Open Source Technology Center