[PATCH v2 0/9] eeepc cleanup

[PATCH v2 0/9] eeepc cleanup

[Frans Klaver]

Here's the second installment cleaning up some things in the eeepc laptop
driver.

This depends on "eeepc-laptop: simplify parse_arg()".

For those interested, I keep a copy based on Darren's testing branch on
github:

	https://github.com/fransklaver/linux.git tags/eeepc_cleanup_v2

v1..v2:
  - squash coding style fixes
  - drop patch moving to file permission macros, in favor of
  - move towards better sysfs api usage
  - drop changes to existing sysfs return values

Frans Klaver (9):
  eeepc-laptop: clean up coding style
  eeepc-laptop: change sysfs function names to API expectations
  eeepc-laptop: use DEVICE_ATTR* to instantiate device_attributes
  eeepc-laptop: pull out ACPI_STORE_FUNC and ACPI_SHOW_FUNC macros
  eeepc-laptop: tell sysfs that the disp attribute is write-only
  eeepc-laptop: pull out SENSOR_STORE_FUNC and SENSOR_SHOW_FUNC macros
  eeepc-laptop: make fan1_input really read-only
  eeepc-laptop: check proper return values in get_cpufv
  eeepc-laptop: store_cpufv: return error if set_acpi fails

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

-- 
2.1.0

[PATCH 1/9] eeepc-laptop: clean up coding style

[Frans Klaver]

Correct indentation and brace usage to comply with
Documentation/CodingStyle.

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

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index 3095d38..653999e 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -544,7 +544,7 @@ static int eeepc_led_init(struct eeepc_laptop *eeepc)
 	eeepc->tpd_led.name = "eeepc::touchpad";
 	eeepc->tpd_led.brightness_set = tpd_led_set;
 	if (get_acpi(eeepc, CM_ASL_TPD) >= 0) /* if method is available */
-	  eeepc->tpd_led.brightness_get = tpd_led_get;
+		eeepc->tpd_led.brightness_get = tpd_led_get;
 	eeepc->tpd_led.max_brightness = 1;
 
 	rv = led_classdev_register(&eeepc->platform_device->dev,
@@ -692,8 +692,9 @@ static int eeepc_register_rfkill_notifier(struct eeepc_laptop *eeepc,
 		 * changed during setup.
 		 */
 		eeepc_rfkill_hotplug(eeepc, handle);
-	} else
+	} else {
 		return -ENODEV;
+	}
 
 	return 0;
 }
@@ -1424,8 +1425,9 @@ static int eeepc_acpi_add(struct acpi_device *device)
 		result = eeepc_backlight_init(eeepc);
 		if (result)
 			goto fail_backlight;
-	} else
+	} else {
 		pr_info("Backlight controlled by ACPI video driver\n");
+	}
 
 	result = eeepc_input_init(eeepc);
 	if (result)
-- 
2.1.0

[PATCH 2/9] eeepc-laptop: change sysfs function names to API expectations

[Frans Klaver]

The eeepc-laptop driver follows the function naming convention
<action>_<attrname>(), while the sysfs macros are built around the
convention <attrname>_<action>(). Rename the sysfs functions to the
convention used by sysfs. This makes it easier to use the available API
later on.

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

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index 653999e..0094449 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -296,13 +296,13 @@ static ssize_t show_sys_acpi(struct device *dev, int cm, char *buf)
 }
 
 #define EEEPC_CREATE_DEVICE_ATTR(_name, _mode, _cm)			\
-	static ssize_t show_##_name(struct device *dev,			\
+	static ssize_t _name##_show(struct device *dev,			\
 				    struct device_attribute *attr,	\
 				    char *buf)				\
 	{								\
 		return show_sys_acpi(dev, _cm, buf);			\
 	}								\
-	static ssize_t store_##_name(struct device *dev,		\
+	static ssize_t _name##_store(struct device *dev,		\
 				     struct device_attribute *attr,	\
 				     const char *buf, size_t count)	\
 	{								\
@@ -312,8 +312,8 @@ static ssize_t show_sys_acpi(struct device *dev, int cm, char *buf)
 		.attr = {						\
 			.name = __stringify(_name),			\
 			.mode = _mode },				\
-		.show   = show_##_name,					\
-		.store  = store_##_name,				\
+		.show   = _name##_show,					\
+		.store  = _name##_store,				\
 	}
 
 EEEPC_CREATE_DEVICE_ATTR(camera, 0644, CM_ASL_CAMERA);
@@ -335,7 +335,7 @@ static int get_cpufv(struct eeepc_laptop *eeepc, struct eeepc_cpufv *c)
 	return 0;
 }
 
-static ssize_t show_available_cpufv(struct device *dev,
+static ssize_t available_cpufv_show(struct device *dev,
 				    struct device_attribute *attr,
 				    char *buf)
 {
@@ -352,7 +352,7 @@ static ssize_t show_available_cpufv(struct device *dev,
 	return len;
 }
 
-static ssize_t show_cpufv(struct device *dev,
+static ssize_t cpufv_show(struct device *dev,
 			  struct device_attribute *attr,
 			  char *buf)
 {
@@ -364,7 +364,7 @@ static ssize_t show_cpufv(struct device *dev,
 	return sprintf(buf, "%#x\n", (c.num << 8) | c.cur);
 }
 
-static ssize_t store_cpufv(struct device *dev,
+static ssize_t cpufv_store(struct device *dev,
 			   struct device_attribute *attr,
 			   const char *buf, size_t count)
 {
@@ -385,7 +385,7 @@ static ssize_t store_cpufv(struct device *dev,
 	return count;
 }
 
-static ssize_t show_cpufv_disabled(struct device *dev,
+static ssize_t cpufv_disabled_show(struct device *dev,
 			  struct device_attribute *attr,
 			  char *buf)
 {
@@ -394,7 +394,7 @@ static ssize_t show_cpufv_disabled(struct device *dev,
 	return sprintf(buf, "%d\n", eeepc->cpufv_disabled);
 }
 
-static ssize_t store_cpufv_disabled(struct device *dev,
+static ssize_t cpufv_disabled_store(struct device *dev,
 			   struct device_attribute *attr,
 			   const char *buf, size_t count)
 {
@@ -424,23 +424,23 @@ static struct device_attribute dev_attr_cpufv = {
 	.attr = {
 		.name = "cpufv",
 		.mode = 0644 },
-	.show   = show_cpufv,
-	.store  = store_cpufv
+	.show   = cpufv_show,
+	.store  = cpufv_store
 };
 
 static struct device_attribute dev_attr_available_cpufv = {
 	.attr = {
 		.name = "available_cpufv",
 		.mode = 0444 },
-	.show   = show_available_cpufv
+	.show   = available_cpufv_show
 };
 
 static struct device_attribute dev_attr_cpufv_disabled = {
 	.attr = {
 		.name = "cpufv_disabled",
 		.mode = 0644 },
-	.show   = show_cpufv_disabled,
-	.store  = store_cpufv_disabled
+	.show   = cpufv_disabled_show,
+	.store  = cpufv_disabled_store
 };
 
 
-- 
2.1.0

[PATCH 3/9] eeepc-laptop: use DEVICE_ATTR* to instantiate device_attributes

[Frans Klaver]

Device attributes are instantiated manually, while we have DEVICE_ATTR*
macros available to do much of the work for us. Let's use them.

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

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index 0094449..db26f78 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -308,13 +308,7 @@ static ssize_t show_sys_acpi(struct device *dev, int cm, char *buf)
 	{								\
 		return store_sys_acpi(dev, _cm, buf, count);		\
 	}								\
-	static struct device_attribute dev_attr_##_name = {		\
-		.attr = {						\
-			.name = __stringify(_name),			\
-			.mode = _mode },				\
-		.show   = _name##_show,					\
-		.store  = _name##_store,				\
-	}
+	static DEVICE_ATTR(_name, _mode, _name##_show, _name##_store)
 
 EEEPC_CREATE_DEVICE_ATTR(camera, 0644, CM_ASL_CAMERA);
 EEEPC_CREATE_DEVICE_ATTR(cardr, 0644, CM_ASL_CARDREADER);
@@ -420,29 +414,9 @@ static ssize_t cpufv_disabled_store(struct device *dev,
 }
 
 
-static struct device_attribute dev_attr_cpufv = {
-	.attr = {
-		.name = "cpufv",
-		.mode = 0644 },
-	.show   = cpufv_show,
-	.store  = cpufv_store
-};
-
-static struct device_attribute dev_attr_available_cpufv = {
-	.attr = {
-		.name = "available_cpufv",
-		.mode = 0444 },
-	.show   = available_cpufv_show
-};
-
-static struct device_attribute dev_attr_cpufv_disabled = {
-	.attr = {
-		.name = "cpufv_disabled",
-		.mode = 0644 },
-	.show   = cpufv_disabled_show,
-	.store  = cpufv_disabled_store
-};
-
+static DEVICE_ATTR_RW(cpufv);
+static DEVICE_ATTR_RO(available_cpufv);
+static DEVICE_ATTR_RW(cpufv_disabled);
 
 static struct attribute *platform_attributes[] = {
 	&dev_attr_camera.attr,
-- 
2.1.0

[PATCH 4/9] eeepc-laptop: pull out ACPI_STORE_FUNC and ACPI_SHOW_FUNC macros

[Frans Klaver]

Pull out macros EEEPC_ACPI_STORE_FUNC and EEEPC_ACPI_SHOW_FUNC. These
macros define functions that call store_sys_acpi() and show_sys_acpi()
respectively. This helps prevent duplication later on.

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

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index db26f78..c6d765f 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -295,19 +295,25 @@ static ssize_t show_sys_acpi(struct device *dev, int cm, char *buf)
 	return sprintf(buf, "%d\n", value);
 }
 
-#define EEEPC_CREATE_DEVICE_ATTR(_name, _mode, _cm)			\
+#define EEEPC_ACPI_SHOW_FUNC(_name, _cm)				\
 	static ssize_t _name##_show(struct device *dev,			\
 				    struct device_attribute *attr,	\
 				    char *buf)				\
 	{								\
 		return show_sys_acpi(dev, _cm, buf);			\
-	}								\
+	}
+
+#define EEEPC_ACPI_STORE_FUNC(_name, _cm)				\
 	static ssize_t _name##_store(struct device *dev,		\
 				     struct device_attribute *attr,	\
 				     const char *buf, size_t count)	\
 	{								\
 		return store_sys_acpi(dev, _cm, buf, count);		\
-	}								\
+	}
+
+#define EEEPC_CREATE_DEVICE_ATTR(_name, _mode, _cm)			\
+	EEEPC_ACPI_SHOW_FUNC(_name, _cm)				\
+	EEEPC_ACPI_STORE_FUNC(_name, _cm)				\
 	static DEVICE_ATTR(_name, _mode, _name##_show, _name##_store)
 
 EEEPC_CREATE_DEVICE_ATTR(camera, 0644, CM_ASL_CAMERA);
-- 
2.1.0

[PATCH 5/9] eeepc-laptop: tell sysfs that the disp attribute is write-only

[Frans Klaver]

The disp attribute is write-only, but sysfs doesn't know this. Currently
show_sys_acpi() is mimicking sysfs behavior, if the underlying acpi call
should fail. This was introduced in 6dff29b63a5bf2eaf3 "eeepc-laptop:
disp attribute should be write-only". This is not ideal; behaving like
sysfs is better left to sysfs.

Introduce EEEPC_CREATE_DEVICE_ATTR_WO() to instantiate a write-only
attribute, and declare the disp attribute with it. Sysfs makes sure
userspace can only write to disp at all times. This removes the need for
mimicking the sysfs behavior in show_sys_acpi() and store_sys_acpi(),
but we'll stick with -EIO, as changing sysfs return values should not be
taken lightly.

This change also causes EEEPC_CREATE_DEVICE_ATTR() to be used only for
R/W attributes. This enables us to drop the _mode argument from the
macro and use DEVICE_ATTR_RW() internally while we're at it. Append _RW
to the name for readability.

Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
Here we're sticking with -EIO as return values. It should be said that the
commit mentioned above did change the error value from -ENODEV to -EIO. I'm
still in two minds about whether the show_sys_acpi and store_sys_acpi should go
back to returning ENODEV. We'll probably stick with -EIO, though, as there is
no strong reason other than "it was like that before".

 drivers/platform/x86/eeepc-laptop.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index c6d765f..a85da4f 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -311,14 +311,18 @@ static ssize_t show_sys_acpi(struct device *dev, int cm, char *buf)
 		return store_sys_acpi(dev, _cm, buf, count);		\
 	}
 
-#define EEEPC_CREATE_DEVICE_ATTR(_name, _mode, _cm)			\
+#define EEEPC_CREATE_DEVICE_ATTR_RW(_name, _cm)				\
 	EEEPC_ACPI_SHOW_FUNC(_name, _cm)				\
 	EEEPC_ACPI_STORE_FUNC(_name, _cm)				\
-	static DEVICE_ATTR(_name, _mode, _name##_show, _name##_store)
+	static DEVICE_ATTR_RW(_name)
 
-EEEPC_CREATE_DEVICE_ATTR(camera, 0644, CM_ASL_CAMERA);
-EEEPC_CREATE_DEVICE_ATTR(cardr, 0644, CM_ASL_CARDREADER);
-EEEPC_CREATE_DEVICE_ATTR(disp, 0200, CM_ASL_DISPLAYSWITCH);
+#define EEEPC_CREATE_DEVICE_ATTR_WO(_name, _cm)				\
+	EEEPC_ACPI_STORE_FUNC(_name, _cm)				\
+	static DEVICE_ATTR_WO(_name)
+
+EEEPC_CREATE_DEVICE_ATTR_RW(camera, CM_ASL_CAMERA);
+EEEPC_CREATE_DEVICE_ATTR_RW(cardr, CM_ASL_CARDREADER);
+EEEPC_CREATE_DEVICE_ATTR_WO(disp, CM_ASL_DISPLAYSWITCH);
 
 struct eeepc_cpufv {
 	int num;
-- 
2.1.0

[PATCH 6/9] eeepc-laptop: pull out SENSOR_STORE_FUNC and SENSOR_SHOW_FUNC macros

[Frans Klaver]

Pull out EEEPC_SENSOR_STORE_FUNC and EEEPC_SENSOR_SHOW_FUNC. These
macros define functions that call store_sys_hwmon() and show_sys_hwmon()
respectively. This helps prevent duplication later on.

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

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index a85da4f..ba251bb 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -1038,19 +1038,25 @@ static ssize_t show_sys_hwmon(int (*get)(void), char *buf)
 	return sprintf(buf, "%d\n", get());
 }
 
-#define EEEPC_CREATE_SENSOR_ATTR(_name, _mode, _get, _set)		\
+#define EEEPC_SENSOR_SHOW_FUNC(_name, _get)				\
 	static ssize_t show_##_name(struct device *dev,			\
 				    struct device_attribute *attr,	\
 				    char *buf)				\
 	{								\
 		return show_sys_hwmon(_get, buf);			\
-	}								\
+	}
+
+#define EEEPC_SENSOR_STORE_FUNC(_name, _set)				\
 	static ssize_t store_##_name(struct device *dev,		\
 				     struct device_attribute *attr,	\
 				     const char *buf, size_t count)	\
 	{								\
 		return store_sys_hwmon(_set, buf, count);		\
-	}								\
+	}
+
+#define EEEPC_CREATE_SENSOR_ATTR(_name, _mode, _get, _set)		\
+	EEEPC_SENSOR_SHOW_FUNC(_name, _get)				\
+	EEEPC_SENSOR_STORE_FUNC(_name, _set)				\
 	static DEVICE_ATTR(_name, _mode, show_##_name, store_##_name)
 
 EEEPC_CREATE_SENSOR_ATTR(fan1_input, S_IRUGO, eeepc_get_fan_rpm, NULL);
-- 
2.1.0

[PATCH 7/9] eeepc-laptop: make fan1_input really read-only

[Frans Klaver]

In the instantiation of the fan1_input device attribute, NULL is passed
as set function to store_sys_hwmon. The function pointer is never
checked before dereferencing it. This is fine if we can guarantee that
it will never be called with an invalid pointer, but we can't. If
someone from user space decides to change the permissions on this
attribute and write to it, kernel will crash.

Introduce EEEPC_CREATE_SENSOR_ATTR_RO() to instantiate a read-only
attribute, and declare fan1_input with it. This ensures store_sys_hwmon
is never called with NULL parameters. If someone tries to write the
attribute, the system will at least keep its sanity.

This also causes EEEPC_CREATE_SENSOR_ATTR() to be only used for R/W
attributes.This enables us to drop the _mode argument from the macro
and use DEVICE_ATTR_RW() internally while we're at it. Append _RW to the
name for readability.

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 ba251bb..e93a54e 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -1039,7 +1039,7 @@ static ssize_t show_sys_hwmon(int (*get)(void), char *buf)
 }
 
 #define EEEPC_SENSOR_SHOW_FUNC(_name, _get)				\
-	static ssize_t show_##_name(struct device *dev,			\
+	static ssize_t _name##_show(struct device *dev,			\
 				    struct device_attribute *attr,	\
 				    char *buf)				\
 	{								\
@@ -1047,23 +1047,27 @@ static ssize_t show_sys_hwmon(int (*get)(void), char *buf)
 	}
 
 #define EEEPC_SENSOR_STORE_FUNC(_name, _set)				\
-	static ssize_t store_##_name(struct device *dev,		\
+	static ssize_t _name##_store(struct device *dev,		\
 				     struct device_attribute *attr,	\
 				     const char *buf, size_t count)	\
 	{								\
 		return store_sys_hwmon(_set, buf, count);		\
 	}
 
-#define EEEPC_CREATE_SENSOR_ATTR(_name, _mode, _get, _set)		\
+#define EEEPC_CREATE_SENSOR_ATTR_RW(_name, _get, _set)			\
 	EEEPC_SENSOR_SHOW_FUNC(_name, _get)				\
 	EEEPC_SENSOR_STORE_FUNC(_name, _set)				\
-	static DEVICE_ATTR(_name, _mode, show_##_name, store_##_name)
+	static DEVICE_ATTR_RW(_name)
+
+#define EEEPC_CREATE_SENSOR_ATTR_RO(_name, _get)			\
+	EEEPC_SENSOR_SHOW_FUNC(_name, _get)				\
+	static DEVICE_ATTR_RO(_name)
 
-EEEPC_CREATE_SENSOR_ATTR(fan1_input, S_IRUGO, eeepc_get_fan_rpm, NULL);
-EEEPC_CREATE_SENSOR_ATTR(pwm1, S_IRUGO | S_IWUSR,
-			 eeepc_get_fan_pwm, eeepc_set_fan_pwm);
-EEEPC_CREATE_SENSOR_ATTR(pwm1_enable, S_IRUGO | S_IWUSR,
-			 eeepc_get_fan_ctrl, eeepc_set_fan_ctrl);
+EEEPC_CREATE_SENSOR_ATTR_RO(fan1_input, eeepc_get_fan_rpm);
+EEEPC_CREATE_SENSOR_ATTR_RW(pwm1, eeepc_get_fan_pwm,
+			    eeepc_set_fan_pwm);
+EEEPC_CREATE_SENSOR_ATTR_RW(pwm1_enable, eeepc_get_fan_ctrl,
+			    eeepc_set_fan_ctrl);
 
 static struct attribute *hwmon_attrs[] = {
 	&dev_attr_pwm1.attr,
-- 
2.1.0

[PATCH 8/9] eeepc-laptop: check proper return values in get_cpufv

[Frans Klaver]

In get_cpufv the return value of get_acpi is stored in the cpufv struct.
Right before this value is checked for errors, it is and'ed with 0xff.
This means c->cur can never be less than zero. Besides that, the actual
error value is ignored.

c->num is also and'ed with 0xff, which means we can ignore values below
zero.

Check the result of get_acpi() right away. While at it, propagate the
error if we got one.

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

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index e93a54e..875a43f 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -332,9 +332,12 @@ struct eeepc_cpufv {
 static int get_cpufv(struct eeepc_laptop *eeepc, struct eeepc_cpufv *c)
 {
 	c->cur = get_acpi(eeepc, CM_ASL_CPUFV);
+	if (c->cur < 0)
+		return -ENODEV;
+
 	c->num = (c->cur >> 8) & 0xff;
 	c->cur &= 0xff;
-	if (c->cur < 0 || c->num <= 0 || c->num > 12)
+	if (c->num == 0 || c->num > 12)
 		return -ENODEV;
 	return 0;
 }
-- 
2.1.0

[PATCH 9/9] eeepc-laptop: store_cpufv: return error if set_acpi fails

[Frans Klaver]

The result of set_acpi is left unchecked, but it may return errors. If
one occurs, send the error to the caller. There's no reason to lie about
it, if set_acpi fails.

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

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index 875a43f..3f6c762 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -388,7 +388,9 @@ static ssize_t cpufv_store(struct device *dev,
 		return rv;
 	if (value < 0 || value >= c.num)
 		return -EINVAL;
-	set_acpi(eeepc, CM_ASL_CPUFV, value);
+	rv = set_acpi(eeepc, CM_ASL_CPUFV, value);
+	if (rv)
+		return rv;
 	return count;
 }
 
-- 
2.1.0

Re: [PATCH 3/9] eeepc-laptop: use DEVICE_ATTR* to instantiate device_attributes

[Greg Kroah-Hartman]

On Wed, Sep 17, 2014 at 11:47:21PM +0200, Frans Klaver wrote:
> Device attributes are instantiated manually, while we have DEVICE_ATTR*
> macros available to do much of the work for us. Let's use them.
> 
> Signed-off-by: Frans Klaver <fransklaver@gmail.com>
> ---
>  drivers/platform/x86/eeepc-laptop.c | 34 ++++------------------------------
>  1 file changed, 4 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
> index 0094449..db26f78 100644
> --- a/drivers/platform/x86/eeepc-laptop.c
> +++ b/drivers/platform/x86/eeepc-laptop.c
> @@ -308,13 +308,7 @@ static ssize_t show_sys_acpi(struct device *dev, int cm, char *buf)
>  	{								\
>  		return store_sys_acpi(dev, _cm, buf, count);		\
>  	}								\
> -	static struct device_attribute dev_attr_##_name = {		\
> -		.attr = {						\
> -			.name = __stringify(_name),			\
> -			.mode = _mode },				\
> -		.show   = _name##_show,					\
> -		.store  = _name##_store,				\
> -	}
> +	static DEVICE_ATTR(_name, _mode, _name##_show, _name##_store)

Can you change this to be DEVICE_ATTR_RO()?

If not, split it up into 2 different macros, one using the _RW and one
_RO version, _never_ have the driver specify the mode of the sysfs file
please.

thanks,

greg k-h

Re: [PATCH 1/9] eeepc-laptop: clean up coding style

[Joe Perches]

On Wed, 2014-09-17 at 23:47 +0200, Frans Klaver wrote:
> Correct indentation and brace usage to comply with
> Documentation/CodingStyle.
> 
> Signed-off-by: Frans Klaver <fransklaver@gmail.com>
> ---
>  drivers/platform/x86/eeepc-laptop.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
> index 3095d38..653999e 100644
> --- a/drivers/platform/x86/eeepc-laptop.c
> +++ b/drivers/platform/x86/eeepc-laptop.c
> @@ -544,7 +544,7 @@ static int eeepc_led_init(struct eeepc_laptop *eeepc)
>  	eeepc->tpd_led.name = "eeepc::touchpad";
>  	eeepc->tpd_led.brightness_set = tpd_led_set;
>  	if (get_acpi(eeepc, CM_ASL_TPD) >= 0) /* if method is available */
> -	  eeepc->tpd_led.brightness_get = tpd_led_get;
> +		eeepc->tpd_led.brightness_get = tpd_led_get;
>  	eeepc->tpd_led.max_brightness = 1;
>  
>  	rv = led_classdev_register(&eeepc->platform_device->dev,
> @@ -692,8 +692,9 @@ static int eeepc_register_rfkill_notifier(struct eeepc_laptop *eeepc,
>  		 * changed during setup.
>  		 */
>  		eeepc_rfkill_hotplug(eeepc, handle);
> -	} else
> +	} else {
>  		return -ENODEV;
> +	}
>  
>  	return 0;
>  }

This sort of code:

	if (foo) {
		[ do_something ]
	} else
		return -ERRVAL;

is generally better rewritten as:

	if (!foo)
		return -ERRVAL;

	[ do_something ]

This gives immediacy to the error handler and
as well reduces unnecessary indentation.

So instead of:

static int eeepc_register_rfkill_notifier(struct eeepc_laptop *eeepc,
					  char *node)
{
	acpi_status status;
	acpi_handle handle;

	status = acpi_get_handle(NULL, node, &handle);

	if (ACPI_SUCCESS(status)) {
		status = acpi_install_notify_handler(handle,
						     ACPI_SYSTEM_NOTIFY,
						     eeepc_rfkill_notify,
						     eeepc);
		if (ACPI_FAILURE(status))
			pr_warn("Failed to register notify on %s\n", node);

		/*
		 * Refresh pci hotplug in case the rfkill state was
		 * changed during setup.
		 */
		eeepc_rfkill_hotplug(eeepc, handle);
	} else
		return -ENODEV;

	return 0;
}

try something like:

static int eeepc_register_rfkill_notifier(struct eeepc_laptop *eeepc,
					  char *node)
{
	acpi_handle handle;
	acpi_status status = acpi_get_handle(NULL, node, &handle);

	if (!ACPI_SUCCESS(status))
		return -ENODEV;

	status = acpi_install_notify_handler(handle,
					     ACPI_SYSTEM_NOTIFY,
					     eeepc_rfkill_notify,
					     eeepc);
	if (ACPI_FAILURE(status))
		pr_warn("Failed to register notify on %s\n", node);

	/*
	 * Refresh pci hotplug in case the rfkill state was
	 * changed during setup.
	 */
	eeepc_rfkill_hotplug(eeepc, handle);

	return 0;
}

Re: [PATCH 5/9] eeepc-laptop: tell sysfs that the disp attribute is write-only

[Greg Kroah-Hartman]

On Wed, Sep 17, 2014 at 11:47:23PM +0200, Frans Klaver wrote:
> The disp attribute is write-only, but sysfs doesn't know this. Currently
> show_sys_acpi() is mimicking sysfs behavior, if the underlying acpi call
> should fail. This was introduced in 6dff29b63a5bf2eaf3 "eeepc-laptop:
> disp attribute should be write-only". This is not ideal; behaving like
> sysfs is better left to sysfs.
> 
> Introduce EEEPC_CREATE_DEVICE_ATTR_WO() to instantiate a write-only
> attribute, and declare the disp attribute with it. Sysfs makes sure
> userspace can only write to disp at all times. This removes the need for
> mimicking the sysfs behavior in show_sys_acpi() and store_sys_acpi(),
> but we'll stick with -EIO, as changing sysfs return values should not be
> taken lightly.
> 
> This change also causes EEEPC_CREATE_DEVICE_ATTR() to be used only for
> R/W attributes. This enables us to drop the _mode argument from the
> macro and use DEVICE_ATTR_RW() internally while we're at it. Append _RW
> to the name for readability.
> 
> Signed-off-by: Frans Klaver <fransklaver@gmail.com>
> ---
> Here we're sticking with -EIO as return values. It should be said that the
> commit mentioned above did change the error value from -ENODEV to -EIO. I'm
> still in two minds about whether the show_sys_acpi and store_sys_acpi should go
> back to returning ENODEV. We'll probably stick with -EIO, though, as there is
> no strong reason other than "it was like that before".
> 
>  drivers/platform/x86/eeepc-laptop.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
> index c6d765f..a85da4f 100644
> --- a/drivers/platform/x86/eeepc-laptop.c
> +++ b/drivers/platform/x86/eeepc-laptop.c
> @@ -311,14 +311,18 @@ static ssize_t show_sys_acpi(struct device *dev, int cm, char *buf)
>  		return store_sys_acpi(dev, _cm, buf, count);		\
>  	}
>  
> -#define EEEPC_CREATE_DEVICE_ATTR(_name, _mode, _cm)			\
> +#define EEEPC_CREATE_DEVICE_ATTR_RW(_name, _cm)				\
>  	EEEPC_ACPI_SHOW_FUNC(_name, _cm)				\
>  	EEEPC_ACPI_STORE_FUNC(_name, _cm)				\
> -	static DEVICE_ATTR(_name, _mode, _name##_show, _name##_store)
> +	static DEVICE_ATTR_RW(_name)
>  
> -EEEPC_CREATE_DEVICE_ATTR(camera, 0644, CM_ASL_CAMERA);
> -EEEPC_CREATE_DEVICE_ATTR(cardr, 0644, CM_ASL_CARDREADER);
> -EEEPC_CREATE_DEVICE_ATTR(disp, 0200, CM_ASL_DISPLAYSWITCH);
> +#define EEEPC_CREATE_DEVICE_ATTR_WO(_name, _cm)				\
> +	EEEPC_ACPI_STORE_FUNC(_name, _cm)				\
> +	static DEVICE_ATTR_WO(_name)
> +
> +EEEPC_CREATE_DEVICE_ATTR_RW(camera, CM_ASL_CAMERA);
> +EEEPC_CREATE_DEVICE_ATTR_RW(cardr, CM_ASL_CARDREADER);
> +EEEPC_CREATE_DEVICE_ATTR_WO(disp, CM_ASL_DISPLAYSWITCH);
>  
>  struct eeepc_cpufv {
>  	int num;

Ah, you did what I asked on a previous patch here, nevermind :)

greg k-h

Re: [PATCH 1/9] eeepc-laptop: clean up coding style

[Frans Klaver]

On 18 September 2014 00:06:52 CEST, Joe Perches <joe@perches.com> wrote:
>On Wed, 2014-09-17 at 23:47 +0200, Frans Klaver wrote:
>> Correct indentation and brace usage to comply with
>> Documentation/CodingStyle.
>> 
>> Signed-off-by: Frans Klaver <fransklaver@gmail.com>
>> ---
>>  drivers/platform/x86/eeepc-laptop.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/platform/x86/eeepc-laptop.c
>b/drivers/platform/x86/eeepc-laptop.c
>> index 3095d38..653999e 100644
>> --- a/drivers/platform/x86/eeepc-laptop.c
>> +++ b/drivers/platform/x86/eeepc-laptop.c
>> @@ -544,7 +544,7 @@ static int eeepc_led_init(struct eeepc_laptop
>*eeepc)
>>  	eeepc->tpd_led.name = "eeepc::touchpad";
>>  	eeepc->tpd_led.brightness_set = tpd_led_set;
>>  	if (get_acpi(eeepc, CM_ASL_TPD) >= 0) /* if method is available */
>> -	  eeepc->tpd_led.brightness_get = tpd_led_get;
>> +		eeepc->tpd_led.brightness_get = tpd_led_get;
>>  	eeepc->tpd_led.max_brightness = 1;
>>  
>>  	rv = led_classdev_register(&eeepc->platform_device->dev,
>> @@ -692,8 +692,9 @@ static int eeepc_register_rfkill_notifier(struct
>eeepc_laptop *eeepc,
>>  		 * changed during setup.
>>  		 */
>>  		eeepc_rfkill_hotplug(eeepc, handle);
>> -	} else
>> +	} else {
>>  		return -ENODEV;
>> +	}
>>  
>>  	return 0;
>>  }
>
>This sort of code:
>
>	if (foo) {
>		[ do_something ]
>	} else
>		return -ERRVAL;
>
>is generally better rewritten as:
>
>	if (!foo)
>		return -ERRVAL;
>
>	[ do_something ]
>
>This gives immediacy to the error handler and
>as well reduces unnecessary indentation.

I fully agree. 

Darren, do you still take this in one patch? 

Frans

Re: [PATCH 5/9] eeepc-laptop: tell sysfs that the disp attribute is write-only

[Frans Klaver]

On 18 September 2014 00:07:53 CEST, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
>On Wed, Sep 17, 2014 at 11:47:23PM +0200, Frans Klaver wrote:
>> The disp attribute is write-only, but sysfs doesn't know this.
>Currently
>> show_sys_acpi() is mimicking sysfs behavior, if the underlying acpi
>call
>> should fail. This was introduced in 6dff29b63a5bf2eaf3 "eeepc-laptop:
>> disp attribute should be write-only". This is not ideal; behaving
>like
>> sysfs is better left to sysfs.
>> 
>> Introduce EEEPC_CREATE_DEVICE_ATTR_WO() to instantiate a write-only
>> attribute, and declare the disp attribute with it. Sysfs makes sure
>> userspace can only write to disp at all times. This removes the need
>for
>> mimicking the sysfs behavior in show_sys_acpi() and store_sys_acpi(),
>> but we'll stick with -EIO, as changing sysfs return values should not
>be
>> taken lightly.
>> 
>> This change also causes EEEPC_CREATE_DEVICE_ATTR() to be used only
>for
>> R/W attributes. This enables us to drop the _mode argument from the
>> macro and use DEVICE_ATTR_RW() internally while we're at it. Append
>_RW
>> to the name for readability.
>> 
>> Signed-off-by: Frans Klaver <fransklaver@gmail.com>
>> ---
>> Here we're sticking with -EIO as return values. It should be said
>that the
>> commit mentioned above did change the error value from -ENODEV to
>-EIO. I'm
>> still in two minds about whether the show_sys_acpi and store_sys_acpi
>should go
>> back to returning ENODEV. We'll probably stick with -EIO, though, as
>there is
>> no strong reason other than "it was like that before".
>> 
>>  drivers/platform/x86/eeepc-laptop.c | 14 +++++++++-----
>>  1 file changed, 9 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/platform/x86/eeepc-laptop.c
>b/drivers/platform/x86/eeepc-laptop.c
>> index c6d765f..a85da4f 100644
>> --- a/drivers/platform/x86/eeepc-laptop.c
>> +++ b/drivers/platform/x86/eeepc-laptop.c
>> @@ -311,14 +311,18 @@ static ssize_t show_sys_acpi(struct device
>*dev, int cm, char *buf)
>>  		return store_sys_acpi(dev, _cm, buf, count);		\
>>  	}
>>  
>> -#define EEEPC_CREATE_DEVICE_ATTR(_name, _mode, _cm)			\
>> +#define EEEPC_CREATE_DEVICE_ATTR_RW(_name, _cm)				\
>>  	EEEPC_ACPI_SHOW_FUNC(_name, _cm)				\
>>  	EEEPC_ACPI_STORE_FUNC(_name, _cm)				\
>> -	static DEVICE_ATTR(_name, _mode, _name##_show, _name##_store)
>> +	static DEVICE_ATTR_RW(_name)
>>  
>> -EEEPC_CREATE_DEVICE_ATTR(camera, 0644, CM_ASL_CAMERA);
>> -EEEPC_CREATE_DEVICE_ATTR(cardr, 0644, CM_ASL_CARDREADER);
>> -EEEPC_CREATE_DEVICE_ATTR(disp, 0200, CM_ASL_DISPLAYSWITCH);
>> +#define EEEPC_CREATE_DEVICE_ATTR_WO(_name, _cm)				\
>> +	EEEPC_ACPI_STORE_FUNC(_name, _cm)				\
>> +	static DEVICE_ATTR_WO(_name)
>> +
>> +EEEPC_CREATE_DEVICE_ATTR_RW(camera, CM_ASL_CAMERA);
>> +EEEPC_CREATE_DEVICE_ATTR_RW(cardr, CM_ASL_CARDREADER);
>> +EEEPC_CREATE_DEVICE_ATTR_WO(disp, CM_ASL_DISPLAYSWITCH);
>>  
>>  struct eeepc_cpufv {
>>  	int num;
>
>Ah, you did what I asked on a previous patch here, nevermind :)

Yea, I thought I'd make more sense this way. 

Thanks, 
Frans 

Re: [PATCH 1/9] eeepc-laptop: clean up coding style

[Darren Hart]

On Wed, Sep 17, 2014 at 11:47:19PM +0200, Frans Klaver wrote:
> Correct indentation and brace usage to comply with
> Documentation/CodingStyle.
> 
> Signed-off-by: Frans Klaver <fransklaver@gmail.com>

Queued, thanks!

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 1/9] eeepc-laptop: clean up coding style

[Darren Hart]

On Thu, Sep 18, 2014 at 07:01:25AM +0200, Frans Klaver wrote:
> On 18 September 2014 00:06:52 CEST, Joe Perches <joe@perches.com> wrote:
> >On Wed, 2014-09-17 at 23:47 +0200, Frans Klaver wrote:
> >> Correct indentation and brace usage to comply with
> >> Documentation/CodingStyle.
> >> 
> >> Signed-off-by: Frans Klaver <fransklaver@gmail.com>
> >> ---
> >>  drivers/platform/x86/eeepc-laptop.c | 8 +++++---
> >>  1 file changed, 5 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/drivers/platform/x86/eeepc-laptop.c
> >b/drivers/platform/x86/eeepc-laptop.c
> >> index 3095d38..653999e 100644
> >> --- a/drivers/platform/x86/eeepc-laptop.c
> >> +++ b/drivers/platform/x86/eeepc-laptop.c
> >> @@ -544,7 +544,7 @@ static int eeepc_led_init(struct eeepc_laptop
> >*eeepc)
> >>  	eeepc->tpd_led.name = "eeepc::touchpad";
> >>  	eeepc->tpd_led.brightness_set = tpd_led_set;
> >>  	if (get_acpi(eeepc, CM_ASL_TPD) >= 0) /* if method is available */
> >> -	  eeepc->tpd_led.brightness_get = tpd_led_get;
> >> +		eeepc->tpd_led.brightness_get = tpd_led_get;
> >>  	eeepc->tpd_led.max_brightness = 1;
> >>  
> >>  	rv = led_classdev_register(&eeepc->platform_device->dev,
> >> @@ -692,8 +692,9 @@ static int eeepc_register_rfkill_notifier(struct
> >eeepc_laptop *eeepc,
> >>  		 * changed during setup.
> >>  		 */
> >>  		eeepc_rfkill_hotplug(eeepc, handle);
> >> -	} else
> >> +	} else {
> >>  		return -ENODEV;
> >> +	}
> >>  
> >>  	return 0;
> >>  }
> >
> >This sort of code:
> >
> >	if (foo) {
> >		[ do_something ]
> >	} else
> >		return -ERRVAL;
> >
> >is generally better rewritten as:
> >
> >	if (!foo)
> >		return -ERRVAL;
> >
> >	[ do_something ]
> >
> >This gives immediacy to the error handler and
> >as well reduces unnecessary indentation.
> 
> I fully agree. 
> 
> Darren, do you still take this in one patch? 

I'll take the cleanup patch as is. I'm happy to take a separate patch to improve
the code flow.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 1/9] eeepc-laptop: clean up coding style

[Frans Klaver]

On 19 September 2014 18:46:03 CEST, Darren Hart <dvhart@infradead.org> wrote:
>On Thu, Sep 18, 2014 at 07:01:25AM +0200, Frans Klaver wrote:
>> On 18 September 2014 00:06:52 CEST, Joe Perches <joe@perches.com>
>wrote:
>> >On Wed, 2014-09-17 at 23:47 +0200, Frans Klaver wrote:
>> >> Correct indentation and brace usage to comply with
>> >> Documentation/CodingStyle.
>> >> 
>> >> Signed-off-by: Frans Klaver <fransklaver@gmail.com>
>> >> ---
>> >>  drivers/platform/x86/eeepc-laptop.c | 8 +++++---
>> >>  1 file changed, 5 insertions(+), 3 deletions(-)
>> >> 
>> >> diff --git a/drivers/platform/x86/eeepc-laptop.c
>> >b/drivers/platform/x86/eeepc-laptop.c
>> >> index 3095d38..653999e 100644
>> >> --- a/drivers/platform/x86/eeepc-laptop.c
>> >> +++ b/drivers/platform/x86/eeepc-laptop.c
>> >> @@ -544,7 +544,7 @@ static int eeepc_led_init(struct eeepc_laptop
>> >*eeepc)
>> >>  	eeepc->tpd_led.name = "eeepc::touchpad";
>> >>  	eeepc->tpd_led.brightness_set = tpd_led_set;
>> >>  	if (get_acpi(eeepc, CM_ASL_TPD) >= 0) /* if method is available
>*/
>> >> -	  eeepc->tpd_led.brightness_get = tpd_led_get;
>> >> +		eeepc->tpd_led.brightness_get = tpd_led_get;
>> >>  	eeepc->tpd_led.max_brightness = 1;
>> >>  
>> >>  	rv = led_classdev_register(&eeepc->platform_device->dev,
>> >> @@ -692,8 +692,9 @@ static int
>eeepc_register_rfkill_notifier(struct
>> >eeepc_laptop *eeepc,
>> >>  		 * changed during setup.
>> >>  		 */
>> >>  		eeepc_rfkill_hotplug(eeepc, handle);
>> >> -	} else
>> >> +	} else {
>> >>  		return -ENODEV;
>> >> +	}
>> >>  
>> >>  	return 0;
>> >>  }
>> >
>> >This sort of code:
>> >
>> >	if (foo) {
>> >		[ do_something ]
>> >	} else
>> >		return -ERRVAL;
>> >
>> >is generally better rewritten as:
>> >
>> >	if (!foo)
>> >		return -ERRVAL;
>> >
>> >	[ do_something ]
>> >
>> >This gives immediacy to the error handler and
>> >as well reduces unnecessary indentation.
>> 
>> I fully agree. 
>> 
>> Darren, do you still take this in one patch? 
>
>I'll take the cleanup patch as is. I'm happy to take a separate patch
>to improve
>the code flow.

Alright. Thanks. 

Frans 

Re: [PATCH v2 0/9] eeepc cleanup

[Darren Hart]

On Wed, Sep 17, 2014 at 11:47:18PM +0200, Frans Klaver wrote:
> Here's the second installment cleaning up some things in the eeepc laptop
> driver.
> 
> This depends on "eeepc-laptop: simplify parse_arg()".
> 
> For those interested, I keep a copy based on Darren's testing branch on
> github:

Please note, "testing" is strictly for use with Fengguang's 0day robot. It
*will* rebase and be recreated willy nilly.

If you want to base on something I've queued, I suggest the "for-next" branch as
I expect that to be far more stable. Rabses should be *very* rare there.

Feel free to use "testing" if you need to, just be aware it will rebase.

> 
> 	https://github.com/fransklaver/linux.git tags/eeepc_cleanup_v2

Queued for testing.

Thanks Frans!

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v2 0/9] eeepc cleanup

[Frans Klaver]

On 19 September 2014 19:25:38 CEST, Darren Hart <dvhart@infradead.org> wrote:
>On Wed, Sep 17, 2014 at 11:47:18PM +0200, Frans Klaver wrote:
>> Here's the second installment cleaning up some things in the eeepc
>laptop
>> driver.
>> 
>> This depends on "eeepc-laptop: simplify parse_arg()".
>> 
>> For those interested, I keep a copy based on Darren's testing branch
>on
>> github:
>
>Please note, "testing" is strictly for use with Fengguang's 0day robot.
>It
>*will* rebase and be recreated willy nilly.
>
>If you want to base on something I've queued, I suggest the "for-next"
>branch as
>I expect that to be far more stable. Rabses should be *very* rare
>there.
>
>Feel free to use "testing" if you need to, just be aware it will
>rebase.
>

I'll keep that in mind for the next batch ;) 


>> 
>> 	https://github.com/fransklaver/linux.git tags/eeepc_cleanup_v2
>
>Queued for testing.
>
>Thanks Frans!

Thanks, 
Frans