[PATCH v2 1/3] asus-laptop: fix is

public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed

* [PATCH v2 1/3] asus-laptop: fix is_visible return value
@ 2015-01-18 23:25 Vivien Didelot
  2015-01-18 23:25 ` [PATCH v2 2/3] asus-laptop: use DEVICE_ATTR_* macros Vivien Didelot
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Vivien Didelot @ 2015-01-18 23:25 UTC (permalink / raw)
  To: platform-driver-x86
  Cc: Vivien Didelot, Darren Hart, Corentin Chary, acpi4asus-user,
	linux-kernel, kernel

With a Lucid platform, asus_sysfs_is_visible() returned a boolean for
ls_switch and ls_level attributes.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/platform/x86/asus-laptop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/asus-laptop.c b/drivers/platform/x86/asus-laptop.c
index f71700e..00f5e82 100644
--- a/drivers/platform/x86/asus-laptop.c
+++ b/drivers/platform/x86/asus-laptop.c
@@ -1616,7 +1616,7 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
 		else
 			goto normal;
 
-		return supported;
+		return supported ? attr->mode : 0;
 	}
 
 normal:
-- 
2.2.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 2/3] asus-laptop: use DEVICE_ATTR_* macros
  2015-01-18 23:25 [PATCH v2 1/3] asus-laptop: fix is_visible return value Vivien Didelot
@ 2015-01-18 23:25 ` Vivien Didelot
  2015-01-21 18:28   ` Darren Hart
  2015-01-18 23:25 ` [PATCH v2 3/3] asus-laptop: cleanup is_visible Vivien Didelot
  2015-01-21 18:25 ` [PATCH v2 1/3] asus-laptop: fix is_visible return value Darren Hart
  2 siblings, 1 reply; 10+ messages in thread
From: Vivien Didelot @ 2015-01-18 23:25 UTC (permalink / raw)
  To: platform-driver-x86
  Cc: Vivien Didelot, Darren Hart, Corentin Chary, acpi4asus-user,
	linux-kernel, kernel

Use DEVICE_ATTR_{RO,WO,RW} macros to simplify attributes declarations.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/platform/x86/asus-laptop.c | 95 ++++++++++++++++++--------------------
 1 file changed, 46 insertions(+), 49 deletions(-)

diff --git a/drivers/platform/x86/asus-laptop.c b/drivers/platform/x86/asus-laptop.c
index 00f5e82..46b2746 100644
--- a/drivers/platform/x86/asus-laptop.c
+++ b/drivers/platform/x86/asus-laptop.c
@@ -856,8 +856,8 @@ static void asus_backlight_exit(struct asus_laptop *asus)
  * than count bytes. We set eof to 1 if we handle those 2 values. We return the
  * number of bytes written in page
  */
-static ssize_t show_infos(struct device *dev,
-			  struct device_attribute *attr, char *page)
+static ssize_t infos_show(struct device *dev, struct device_attribute *attr,
+			  char *page)
 {
 	struct asus_laptop *asus = dev_get_drvdata(dev);
 	int len = 0;
@@ -926,6 +926,7 @@ static ssize_t show_infos(struct device *dev,
 
 	return len;
 }
+static DEVICE_ATTR_RO(infos);
 
 static int parse_arg(const char *buf, unsigned long count, int *val)
 {
@@ -957,15 +958,15 @@ static ssize_t sysfs_acpi_set(struct asus_laptop *asus,
 /*
  * LEDD display
  */
-static ssize_t show_ledd(struct device *dev,
-			 struct device_attribute *attr, char *buf)
+static ssize_t ledd_show(struct device *dev, struct device_attribute *attr,
+			 char *buf)
 {
 	struct asus_laptop *asus = dev_get_drvdata(dev);
 
 	return sprintf(buf, "0x%08x\n", asus->ledd_status);
 }
 
-static ssize_t store_ledd(struct device *dev, struct device_attribute *attr,
+static ssize_t ledd_store(struct device *dev, struct device_attribute *attr,
 			  const char *buf, size_t count)
 {
 	struct asus_laptop *asus = dev_get_drvdata(dev);
@@ -981,6 +982,7 @@ static ssize_t store_ledd(struct device *dev, struct device_attribute *attr,
 	}
 	return rv;
 }
+static DEVICE_ATTR_RW(ledd);
 
 /*
  * Wireless
@@ -1014,21 +1016,22 @@ static int asus_wlan_set(struct asus_laptop *asus, int status)
 	return 0;
 }
 
-static ssize_t show_wlan(struct device *dev,
-			 struct device_attribute *attr, char *buf)
+static ssize_t wlan_show(struct device *dev, struct device_attribute *attr,
+			 char *buf)
 {
 	struct asus_laptop *asus = dev_get_drvdata(dev);
 
 	return sprintf(buf, "%d\n", asus_wireless_status(asus, WL_RSTS));
 }
 
-static ssize_t store_wlan(struct device *dev, struct device_attribute *attr,
+static ssize_t wlan_store(struct device *dev, struct device_attribute *attr,
 			  const char *buf, size_t count)
 {
 	struct asus_laptop *asus = dev_get_drvdata(dev);
 
 	return sysfs_acpi_set(asus, buf, count, METHOD_WLAN);
 }
+static DEVICE_ATTR_RW(wlan);
 
 /*e
  * Bluetooth
@@ -1042,15 +1045,15 @@ static int asus_bluetooth_set(struct asus_laptop *asus, int status)
 	return 0;
 }
 
-static ssize_t show_bluetooth(struct device *dev,
-			      struct device_attribute *attr, char *buf)
+static ssize_t bluetooth_show(struct device *dev, struct device_attribute *attr,
+			      char *buf)
 {
 	struct asus_laptop *asus = dev_get_drvdata(dev);
 
 	return sprintf(buf, "%d\n", asus_wireless_status(asus, BT_RSTS));
 }
 
-static ssize_t store_bluetooth(struct device *dev,
+static ssize_t bluetooth_store(struct device *dev,
 			       struct device_attribute *attr, const char *buf,
 			       size_t count)
 {
@@ -1058,6 +1061,7 @@ static ssize_t store_bluetooth(struct device *dev,
 
 	return sysfs_acpi_set(asus, buf, count, METHOD_BLUETOOTH);
 }
+static DEVICE_ATTR_RW(bluetooth);
 
 /*
  * Wimax
@@ -1071,22 +1075,22 @@ static int asus_wimax_set(struct asus_laptop *asus, int status)
 	return 0;
 }
 
-static ssize_t show_wimax(struct device *dev,
-			      struct device_attribute *attr, char *buf)
+static ssize_t wimax_show(struct device *dev, struct device_attribute *attr,
+			  char *buf)
 {
 	struct asus_laptop *asus = dev_get_drvdata(dev);
 
 	return sprintf(buf, "%d\n", asus_wireless_status(asus, WM_RSTS));
 }
 
-static ssize_t store_wimax(struct device *dev,
-			       struct device_attribute *attr, const char *buf,
-			       size_t count)
+static ssize_t wimax_store(struct device *dev, struct device_attribute *attr,
+			   const char *buf, size_t count)
 {
 	struct asus_laptop *asus = dev_get_drvdata(dev);
 
 	return sysfs_acpi_set(asus, buf, count, METHOD_WIMAX);
 }
+static DEVICE_ATTR_RW(wimax);
 
 /*
  * Wwan
@@ -1100,22 +1104,22 @@ static int asus_wwan_set(struct asus_laptop *asus, int status)
 	return 0;
 }
 
-static ssize_t show_wwan(struct device *dev,
-			      struct device_attribute *attr, char *buf)
+static ssize_t wwan_show(struct device *dev, struct device_attribute *attr,
+			 char *buf)
 {
 	struct asus_laptop *asus = dev_get_drvdata(dev);
 
 	return sprintf(buf, "%d\n", asus_wireless_status(asus, WW_RSTS));
 }
 
-static ssize_t store_wwan(struct device *dev,
-			       struct device_attribute *attr, const char *buf,
-			       size_t count)
+static ssize_t wwan_store(struct device *dev, struct device_attribute *attr,
+			  const char *buf, size_t count)
 {
 	struct asus_laptop *asus = dev_get_drvdata(dev);
 
 	return sysfs_acpi_set(asus, buf, count, METHOD_WWAN);
 }
+static DEVICE_ATTR_RW(wwan);
 
 /*
  * Display
@@ -1135,8 +1139,8 @@ static void asus_set_display(struct asus_laptop *asus, int value)
  * displays hooked up simultaneously, so be warned. See the acpi4asus README
  * for more info.
  */
-static ssize_t store_disp(struct device *dev, struct device_attribute *attr,
-			  const char *buf, size_t count)
+static ssize_t display_store(struct device *dev, struct device_attribute *attr,
+			     const char *buf, size_t count)
 {
 	struct asus_laptop *asus = dev_get_drvdata(dev);
 	int rv, value;
@@ -1146,6 +1150,7 @@ static ssize_t store_disp(struct device *dev, struct device_attribute *attr,
 		asus_set_display(asus, value);
 	return rv;
 }
+static DEVICE_ATTR_WO(display);
 
 /*
  * Light Sens
@@ -1167,16 +1172,17 @@ static void asus_als_switch(struct asus_laptop *asus, int value)
 	asus->light_switch = value;
 }
 
-static ssize_t show_lssw(struct device *dev,
-			 struct device_attribute *attr, char *buf)
+static ssize_t ls_switch_show(struct device *dev, struct device_attribute *attr,
+			      char *buf)
 {
 	struct asus_laptop *asus = dev_get_drvdata(dev);
 
 	return sprintf(buf, "%d\n", asus->light_switch);
 }
 
-static ssize_t store_lssw(struct device *dev, struct device_attribute *attr,
-			  const char *buf, size_t count)
+static ssize_t ls_switch_store(struct device *dev,
+			       struct device_attribute *attr, const char *buf,
+			       size_t count)
 {
 	struct asus_laptop *asus = dev_get_drvdata(dev);
 	int rv, value;
@@ -1187,6 +1193,7 @@ static ssize_t store_lssw(struct device *dev, struct device_attribute *attr,
 
 	return rv;
 }
+static DEVICE_ATTR_RW(ls_switch);
 
 static void asus_als_level(struct asus_laptop *asus, int value)
 {
@@ -1195,16 +1202,16 @@ static void asus_als_level(struct asus_laptop *asus, int value)
 	asus->light_level = value;
 }
 
-static ssize_t show_lslvl(struct device *dev,
-			  struct device_attribute *attr, char *buf)
+static ssize_t ls_level_show(struct device *dev, struct device_attribute *attr,
+			     char *buf)
 {
 	struct asus_laptop *asus = dev_get_drvdata(dev);
 
 	return sprintf(buf, "%d\n", asus->light_level);
 }
 
-static ssize_t store_lslvl(struct device *dev, struct device_attribute *attr,
-			   const char *buf, size_t count)
+static ssize_t ls_level_store(struct device *dev, struct device_attribute *attr,
+			      const char *buf, size_t count)
 {
 	struct asus_laptop *asus = dev_get_drvdata(dev);
 	int rv, value;
@@ -1218,6 +1225,7 @@ static ssize_t store_lslvl(struct device *dev, struct device_attribute *attr,
 
 	return rv;
 }
+static DEVICE_ATTR_RW(ls_level);
 
 static int pega_int_read(struct asus_laptop *asus, int arg, int *result)
 {
@@ -1234,8 +1242,8 @@ static int pega_int_read(struct asus_laptop *asus, int arg, int *result)
 	return err;
 }
 
-static ssize_t show_lsvalue(struct device *dev,
-			    struct device_attribute *attr, char *buf)
+static ssize_t ls_value_show(struct device *dev, struct device_attribute *attr,
+			     char *buf)
 {
 	struct asus_laptop *asus = dev_get_drvdata(dev);
 	int err, hi, lo;
@@ -1247,6 +1255,7 @@ static ssize_t show_lsvalue(struct device *dev,
 		return sprintf(buf, "%d\n", 10 * hi + lo);
 	return err;
 }
+static DEVICE_ATTR_RO(ls_value);
 
 /*
  * GPS
@@ -1274,15 +1283,15 @@ static int asus_gps_switch(struct asus_laptop *asus, int status)
 	return 0;
 }
 
-static ssize_t show_gps(struct device *dev,
-			struct device_attribute *attr, char *buf)
+static ssize_t gps_show(struct device *dev, struct device_attribute *attr,
+			char *buf)
 {
 	struct asus_laptop *asus = dev_get_drvdata(dev);
 
 	return sprintf(buf, "%d\n", asus_gps_status(asus));
 }
 
-static ssize_t store_gps(struct device *dev, struct device_attribute *attr,
+static ssize_t gps_store(struct device *dev, struct device_attribute *attr,
 			 const char *buf, size_t count)
 {
 	struct asus_laptop *asus = dev_get_drvdata(dev);
@@ -1298,6 +1307,7 @@ static ssize_t store_gps(struct device *dev, struct device_attribute *attr,
 	rfkill_set_sw_state(asus->gps.rfkill, !value);
 	return rv;
 }
+static DEVICE_ATTR_RW(gps);
 
 /*
  * rfkill
@@ -1569,19 +1579,6 @@ static void asus_acpi_notify(struct acpi_device *device, u32 event)
 	asus_input_notify(asus, event);
 }
 
-static DEVICE_ATTR(infos, S_IRUGO, show_infos, NULL);
-static DEVICE_ATTR(wlan, S_IRUGO | S_IWUSR, show_wlan, store_wlan);
-static DEVICE_ATTR(bluetooth, S_IRUGO | S_IWUSR,
-		   show_bluetooth, store_bluetooth);
-static DEVICE_ATTR(wimax, S_IRUGO | S_IWUSR, show_wimax, store_wimax);
-static DEVICE_ATTR(wwan, S_IRUGO | S_IWUSR, show_wwan, store_wwan);
-static DEVICE_ATTR(display, S_IWUSR, NULL, store_disp);
-static DEVICE_ATTR(ledd, S_IRUGO | S_IWUSR, show_ledd, store_ledd);
-static DEVICE_ATTR(ls_value, S_IRUGO, show_lsvalue, NULL);
-static DEVICE_ATTR(ls_level, S_IRUGO | S_IWUSR, show_lslvl, store_lslvl);
-static DEVICE_ATTR(ls_switch, S_IRUGO | S_IWUSR, show_lssw, store_lssw);
-static DEVICE_ATTR(gps, S_IRUGO | S_IWUSR, show_gps, store_gps);
-
 static struct attribute *asus_attributes[] = {
 	&dev_attr_infos.attr,
 	&dev_attr_wlan.attr,
-- 
2.2.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/3] asus-laptop: use DEVICE_ATTR_* macros
  2015-01-18 23:25 ` [PATCH v2 2/3] asus-laptop: use DEVICE_ATTR_* macros Vivien Didelot
@ 2015-01-21 18:28   ` Darren Hart
  2015-01-21 20:19     ` Vivien Didelot
  0 siblings, 1 reply; 10+ messages in thread
From: Darren Hart @ 2015-01-21 18:28 UTC (permalink / raw)
  To: Vivien Didelot
  Cc: platform-driver-x86, Corentin Chary, acpi4asus-user, linux-kernel,
	kernel

On Sun, Jan 18, 2015 at 06:25:25PM -0500, Vivien Didelot wrote:
> Use DEVICE_ATTR_{RO,WO,RW} macros to simplify attributes declarations.

It does a lot more than that, including a lot of seemingly superfluous
reformatting of function declarations and renaming.

-- 
Darren Hart
Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/3] asus-laptop: use DEVICE_ATTR_* macros
  2015-01-21 18:28   ` Darren Hart
@ 2015-01-21 20:19     ` Vivien Didelot
  2015-01-22 16:26       ` Darren Hart
  0 siblings, 1 reply; 10+ messages in thread
From: Vivien Didelot @ 2015-01-21 20:19 UTC (permalink / raw)
  To: Darren Hart
  Cc: platform-driver-x86, Corentin Chary, acpi4asus-user, linux-kernel,
	kernel

Hi Darren,

> > Use DEVICE_ATTR_{RO,WO,RW} macros to simplify attributes
> > declarations.
>
> It does a lot more than that, including a lot of seemingly
> superfluous reformatting of function declarations and renaming.

What do you mean? DEVICE_ATTR_RW(foo) requires foo_show() and foo_store()
functions, not show_foo() and store_foo().

Thanks,
-v

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/3] asus-laptop: use DEVICE_ATTR_* macros
  2015-01-21 20:19     ` Vivien Didelot
@ 2015-01-22 16:26       ` Darren Hart
  0 siblings, 0 replies; 10+ messages in thread
From: Darren Hart @ 2015-01-22 16:26 UTC (permalink / raw)
  To: Vivien Didelot
  Cc: platform-driver-x86, Corentin Chary, acpi4asus-user, linux-kernel,
	kernel

On Wed, Jan 21, 2015 at 03:19:17PM -0500, Vivien Didelot wrote:
> Hi Darren,
> 
> > > Use DEVICE_ATTR_{RO,WO,RW} macros to simplify attributes
> > > declarations.
> >
> > It does a lot more than that, including a lot of seemingly
> > superfluous reformatting of function declarations and renaming.
> 
> What do you mean? DEVICE_ATTR_RW(foo) requires foo_show() and foo_store()
> functions, not show_foo() and store_foo().

Ah yes, of course. Perhaps obvious in hindsight, but a bit more explanation in
the commit message would have eliminated the confusion.

Always provide enough information in your commit log to explain to someone else
that hasn't been looking at the code as recently as you have to understand the
problem, the solution, and which provides sufficient explanation for all changes
included in the patch.

Please resubmit with a more complete commit message.

-- 
Darren Hart
Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 3/3] asus-laptop: cleanup is_visible
  2015-01-18 23:25 [PATCH v2 1/3] asus-laptop: fix is_visible return value Vivien Didelot
  2015-01-18 23:25 ` [PATCH v2 2/3] asus-laptop: use DEVICE_ATTR_* macros Vivien Didelot
@ 2015-01-18 23:25 ` Vivien Didelot
  2015-01-20 16:48   ` Corentin Chary
  2015-01-21 18:25 ` [PATCH v2 1/3] asus-laptop: fix is_visible return value Darren Hart
  2 siblings, 1 reply; 10+ messages in thread
From: Vivien Didelot @ 2015-01-18 23:25 UTC (permalink / raw)
  To: platform-driver-x86
  Cc: Vivien Didelot, Darren Hart, Corentin Chary, acpi4asus-user,
	linux-kernel, kernel

Use the attribute indexes and concise the if statements.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/platform/x86/asus-laptop.c | 98 ++++++++++++++++----------------------
 1 file changed, 40 insertions(+), 58 deletions(-)

diff --git a/drivers/platform/x86/asus-laptop.c b/drivers/platform/x86/asus-laptop.c
index 46b2746..b576929 100644
--- a/drivers/platform/x86/asus-laptop.c
+++ b/drivers/platform/x86/asus-laptop.c
@@ -1580,77 +1580,59 @@ static void asus_acpi_notify(struct acpi_device *device, u32 event)
 }
 
 static struct attribute *asus_attributes[] = {
-	&dev_attr_infos.attr,
-	&dev_attr_wlan.attr,
-	&dev_attr_bluetooth.attr,
-	&dev_attr_wimax.attr,
-	&dev_attr_wwan.attr,
-	&dev_attr_display.attr,
-	&dev_attr_ledd.attr,
-	&dev_attr_ls_value.attr,
-	&dev_attr_ls_level.attr,
-	&dev_attr_ls_switch.attr,
-	&dev_attr_gps.attr,
+	&dev_attr_infos.attr,		/* 0 */
+	&dev_attr_wlan.attr,		/* 1 */
+	&dev_attr_bluetooth.attr,	/* 2 */
+	&dev_attr_wimax.attr,		/* 3 */
+	&dev_attr_wwan.attr,		/* 4 */
+	&dev_attr_display.attr,		/* 5 */
+	&dev_attr_ledd.attr,		/* 6 */
+	&dev_attr_ls_value.attr,	/* 7 */
+	&dev_attr_ls_level.attr,	/* 8 */
+	&dev_attr_ls_switch.attr,	/* 9 */
+	&dev_attr_gps.attr,		/* 10 */
 	NULL
 };
 
 static umode_t asus_sysfs_is_visible(struct kobject *kobj,
-				    struct attribute *attr,
-				    int idx)
+				     struct attribute *attr, int index)
 {
 	struct device *dev = container_of(kobj, struct device, kobj);
 	struct platform_device *pdev = to_platform_device(dev);
 	struct asus_laptop *asus = platform_get_drvdata(pdev);
 	acpi_handle handle = asus->handle;
-	bool supported;
-
-	if (asus->is_pega_lucid) {
+	bool ret = true;
+
+	if (index == 1) {
+		ret = !acpi_check_handle(handle, METHOD_WLAN, NULL);
+	} else if (index == 2) {
+		ret = !acpi_check_handle(handle, METHOD_BLUETOOTH, NULL);
+	} else if (index == 3) {
+		ret = !acpi_check_handle(handle, METHOD_WIMAX, NULL);
+	} else if (index == 4) {
+		ret = !acpi_check_handle(handle, METHOD_WWAN, NULL);
+	} else if (index == 5) {
+		ret = !acpi_check_handle(handle, METHOD_SWITCH_DISPLAY, NULL);
+	} else if (index == 6) {
+		ret = !acpi_check_handle(handle, METHOD_LEDD, NULL);
+	} else if (index == 7) {
+		ret = asus->is_pega_lucid;
+	} else if (index == 8) {
 		/* no ls_level interface on the Lucid */
-		if (attr == &dev_attr_ls_switch.attr)
-			supported = true;
-		else if (attr == &dev_attr_ls_level.attr)
-			supported = false;
-		else
-			goto normal;
-
-		return supported ? attr->mode : 0;
-	}
-
-normal:
-	if (attr == &dev_attr_wlan.attr) {
-		supported = !acpi_check_handle(handle, METHOD_WLAN, NULL);
-
-	} else if (attr == &dev_attr_bluetooth.attr) {
-		supported = !acpi_check_handle(handle, METHOD_BLUETOOTH, NULL);
-
-	} else if (attr == &dev_attr_display.attr) {
-		supported = !acpi_check_handle(handle, METHOD_SWITCH_DISPLAY, NULL);
-
-	} else if (attr == &dev_attr_wimax.attr) {
-		supported =
-			!acpi_check_handle(asus->handle, METHOD_WIMAX, NULL);
-
-	} else if (attr == &dev_attr_wwan.attr) {
-		supported = !acpi_check_handle(asus->handle, METHOD_WWAN, NULL);
-
-	} else if (attr == &dev_attr_ledd.attr) {
-		supported = !acpi_check_handle(handle, METHOD_LEDD, NULL);
-
-	} else if (attr == &dev_attr_ls_switch.attr ||
-		   attr == &dev_attr_ls_level.attr) {
-		supported = !acpi_check_handle(handle, METHOD_ALS_CONTROL, NULL) &&
+		ret = !asus->is_pega_lucid &&
+			!acpi_check_handle(handle, METHOD_ALS_CONTROL, NULL) &&
 			!acpi_check_handle(handle, METHOD_ALS_LEVEL, NULL);
-	} else if (attr == &dev_attr_ls_value.attr) {
-		supported = asus->is_pega_lucid;
-	} else if (attr == &dev_attr_gps.attr) {
-		supported = !acpi_check_handle(handle, METHOD_GPS_ON, NULL) &&
-			    !acpi_check_handle(handle, METHOD_GPS_OFF, NULL) &&
-			    !acpi_check_handle(handle, METHOD_GPS_STATUS, NULL);
-	} else {
-		supported = true;
+	} else if (index == 9) {
+		ret = asus->is_pega_lucid ||
+			(!acpi_check_handle(handle, METHOD_ALS_CONTROL, NULL) &&
+			 !acpi_check_handle(handle, METHOD_ALS_LEVEL, NULL));
+	} else if (index == 10) {
+		ret = !acpi_check_handle(handle, METHOD_GPS_ON, NULL) &&
+			!acpi_check_handle(handle, METHOD_GPS_OFF, NULL) &&
+			!acpi_check_handle(handle, METHOD_GPS_STATUS, NULL);
 	}
 
-	return supported ? attr->mode : 0;
+	return ret ? attr->mode : 0;
 }
 
 
-- 
2.2.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 3/3] asus-laptop: cleanup is_visible
  2015-01-18 23:25 ` [PATCH v2 3/3] asus-laptop: cleanup is_visible Vivien Didelot
@ 2015-01-20 16:48   ` Corentin Chary
  2015-01-20 18:20     ` Vivien Didelot
  0 siblings, 1 reply; 10+ messages in thread
From: Corentin Chary @ 2015-01-20 16:48 UTC (permalink / raw)
  To: Vivien Didelot
  Cc: platform-driver-x86@vger.kernel.org, Darren Hart, acpi4asus-user,
	LKML, kernel

On Sun, Jan 18, 2015 at 11:25 PM, Vivien Didelot
<vivien.didelot@savoirfairelinux.com> wrote:
>
> Use the attribute indexes and concise the if statements.

Why ? I really don't see that as an improvement.

> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>  drivers/platform/x86/asus-laptop.c | 98 ++++++++++++++++----------------------
>  1 file changed, 40 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/platform/x86/asus-laptop.c b/drivers/platform/x86/asus-laptop.c
> index 46b2746..b576929 100644
> --- a/drivers/platform/x86/asus-laptop.c
> +++ b/drivers/platform/x86/asus-laptop.c
> @@ -1580,77 +1580,59 @@ static void asus_acpi_notify(struct acpi_device *device, u32 event)
>  }
>
>  static struct attribute *asus_attributes[] = {
> -       &dev_attr_infos.attr,
> -       &dev_attr_wlan.attr,
> -       &dev_attr_bluetooth.attr,
> -       &dev_attr_wimax.attr,
> -       &dev_attr_wwan.attr,
> -       &dev_attr_display.attr,
> -       &dev_attr_ledd.attr,
> -       &dev_attr_ls_value.attr,
> -       &dev_attr_ls_level.attr,
> -       &dev_attr_ls_switch.attr,
> -       &dev_attr_gps.attr,
> +       &dev_attr_infos.attr,           /* 0 */
> +       &dev_attr_wlan.attr,            /* 1 */
> +       &dev_attr_bluetooth.attr,       /* 2 */
> +       &dev_attr_wimax.attr,           /* 3 */
> +       &dev_attr_wwan.attr,            /* 4 */
> +       &dev_attr_display.attr,         /* 5 */
> +       &dev_attr_ledd.attr,            /* 6 */
> +       &dev_attr_ls_value.attr,        /* 7 */
> +       &dev_attr_ls_level.attr,        /* 8 */
> +       &dev_attr_ls_switch.attr,       /* 9 */
> +       &dev_attr_gps.attr,             /* 10 */
>         NULL
>  };
>
>  static umode_t asus_sysfs_is_visible(struct kobject *kobj,
> -                                   struct attribute *attr,
> -                                   int idx)
> +                                    struct attribute *attr, int index)
>  {
>         struct device *dev = container_of(kobj, struct device, kobj);
>         struct platform_device *pdev = to_platform_device(dev);
>         struct asus_laptop *asus = platform_get_drvdata(pdev);
>         acpi_handle handle = asus->handle;
> -       bool supported;
> -
> -       if (asus->is_pega_lucid) {
> +       bool ret = true;
> +
> +       if (index == 1) {
> +               ret = !acpi_check_handle(handle, METHOD_WLAN, NULL);
> +       } else if (index == 2) {
> +               ret = !acpi_check_handle(handle, METHOD_BLUETOOTH, NULL);
> +       } else if (index == 3) {
> +               ret = !acpi_check_handle(handle, METHOD_WIMAX, NULL);
> +       } else if (index == 4) {
> +               ret = !acpi_check_handle(handle, METHOD_WWAN, NULL);
> +       } else if (index == 5) {
> +               ret = !acpi_check_handle(handle, METHOD_SWITCH_DISPLAY, NULL);
> +       } else if (index == 6) {
> +               ret = !acpi_check_handle(handle, METHOD_LEDD, NULL);
> +       } else if (index == 7) {
> +               ret = asus->is_pega_lucid;
> +       } else if (index == 8) {
>                 /* no ls_level interface on the Lucid */
> -               if (attr == &dev_attr_ls_switch.attr)
> -                       supported = true;
> -               else if (attr == &dev_attr_ls_level.attr)
> -                       supported = false;
> -               else
> -                       goto normal;
> -
> -               return supported ? attr->mode : 0;
> -       }
> -
> -normal:
> -       if (attr == &dev_attr_wlan.attr) {
> -               supported = !acpi_check_handle(handle, METHOD_WLAN, NULL);
> -
> -       } else if (attr == &dev_attr_bluetooth.attr) {
> -               supported = !acpi_check_handle(handle, METHOD_BLUETOOTH, NULL);
> -
> -       } else if (attr == &dev_attr_display.attr) {
> -               supported = !acpi_check_handle(handle, METHOD_SWITCH_DISPLAY, NULL);
> -
> -       } else if (attr == &dev_attr_wimax.attr) {
> -               supported =
> -                       !acpi_check_handle(asus->handle, METHOD_WIMAX, NULL);
> -
> -       } else if (attr == &dev_attr_wwan.attr) {
> -               supported = !acpi_check_handle(asus->handle, METHOD_WWAN, NULL);
> -
> -       } else if (attr == &dev_attr_ledd.attr) {
> -               supported = !acpi_check_handle(handle, METHOD_LEDD, NULL);
> -
> -       } else if (attr == &dev_attr_ls_switch.attr ||
> -                  attr == &dev_attr_ls_level.attr) {
> -               supported = !acpi_check_handle(handle, METHOD_ALS_CONTROL, NULL) &&
> +               ret = !asus->is_pega_lucid &&
> +                       !acpi_check_handle(handle, METHOD_ALS_CONTROL, NULL) &&
>                         !acpi_check_handle(handle, METHOD_ALS_LEVEL, NULL);
> -       } else if (attr == &dev_attr_ls_value.attr) {
> -               supported = asus->is_pega_lucid;
> -       } else if (attr == &dev_attr_gps.attr) {
> -               supported = !acpi_check_handle(handle, METHOD_GPS_ON, NULL) &&
> -                           !acpi_check_handle(handle, METHOD_GPS_OFF, NULL) &&
> -                           !acpi_check_handle(handle, METHOD_GPS_STATUS, NULL);
> -       } else {
> -               supported = true;
> +       } else if (index == 9) {
> +               ret = asus->is_pega_lucid ||
> +                       (!acpi_check_handle(handle, METHOD_ALS_CONTROL, NULL) &&
> +                        !acpi_check_handle(handle, METHOD_ALS_LEVEL, NULL));
> +       } else if (index == 10) {
> +               ret = !acpi_check_handle(handle, METHOD_GPS_ON, NULL) &&
> +                       !acpi_check_handle(handle, METHOD_GPS_OFF, NULL) &&
> +                       !acpi_check_handle(handle, METHOD_GPS_STATUS, NULL);
>         }
>
> -       return supported ? attr->mode : 0;
> +       return ret ? attr->mode : 0;
>  }
>
>
> --
> 2.2.2
>



-- 
Corentin Chary
http://xf.iksaif.net

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 3/3] asus-laptop: cleanup is_visible
  2015-01-20 16:48   ` Corentin Chary
@ 2015-01-20 18:20     ` Vivien Didelot
  2015-01-21 18:19       ` Darren Hart
  0 siblings, 1 reply; 10+ messages in thread
From: Vivien Didelot @ 2015-01-20 18:20 UTC (permalink / raw)
  To: Corentin Chary
  Cc: platform-driver-x86, Darren Hart, acpi4asus-user, LKML, kernel

Hi Corentin,

> > Use the attribute indexes and concise the if statements.
> >
> Why ? I really don't see that as an improvement.

The improvement is code clarity and maintainability. I'm not use we want
to keep multiple returns and this goto thing. I think per-attribute 
if-statements are clearer.

Thanks,
-v

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 3/3] asus-laptop: cleanup is_visible
  2015-01-20 18:20     ` Vivien Didelot
@ 2015-01-21 18:19       ` Darren Hart
  0 siblings, 0 replies; 10+ messages in thread
From: Darren Hart @ 2015-01-21 18:19 UTC (permalink / raw)
  To: Vivien Didelot
  Cc: Corentin Chary, platform-driver-x86, acpi4asus-user, LKML, kernel

On Tue, Jan 20, 2015 at 01:20:36PM -0500, Vivien Didelot wrote:
> Hi Corentin,
> 
> > > Use the attribute indexes and concise the if statements.
> > >
> > Why ? I really don't see that as an improvement.
> 
> The improvement is code clarity and maintainability. I'm not use we want
> to keep multiple returns and this goto thing. I think per-attribute 
> if-statements are clearer.

I have to concur with Corentin, changing to numerical indices rather than the
named atrributes makes the code harder to read in my opinion, and is more likely
to lead to mistakes than not.

-- 
Darren Hart
Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/3] asus-laptop: fix is_visible return value
  2015-01-18 23:25 [PATCH v2 1/3] asus-laptop: fix is_visible return value Vivien Didelot
  2015-01-18 23:25 ` [PATCH v2 2/3] asus-laptop: use DEVICE_ATTR_* macros Vivien Didelot
  2015-01-18 23:25 ` [PATCH v2 3/3] asus-laptop: cleanup is_visible Vivien Didelot
@ 2015-01-21 18:25 ` Darren Hart
  2 siblings, 0 replies; 10+ messages in thread
From: Darren Hart @ 2015-01-21 18:25 UTC (permalink / raw)
  To: Vivien Didelot
  Cc: platform-driver-x86, Corentin Chary, acpi4asus-user, linux-kernel,
	kernel

On Sun, Jan 18, 2015 at 06:25:24PM -0500, Vivien Didelot wrote:
> With a Lucid platform, asus_sysfs_is_visible() returned a boolean for
> ls_switch and ls_level attributes.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

Queued, thanks.

> ---
>  drivers/platform/x86/asus-laptop.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/asus-laptop.c b/drivers/platform/x86/asus-laptop.c
> index f71700e..00f5e82 100644
> --- a/drivers/platform/x86/asus-laptop.c
> +++ b/drivers/platform/x86/asus-laptop.c
> @@ -1616,7 +1616,7 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
>  		else
>  			goto normal;
>  
> -		return supported;
> +		return supported ? attr->mode : 0;
>  	}
>  
>  normal:
> -- 
> 2.2.2
> 
> 

-- 
Darren Hart
Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2015-01-22 16:26 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-01-18 23:25 [PATCH v2 1/3] asus-laptop: fix is_visible return value Vivien Didelot
2015-01-18 23:25 ` [PATCH v2 2/3] asus-laptop: use DEVICE_ATTR_* macros Vivien Didelot
2015-01-21 18:28   ` Darren Hart
2015-01-21 20:19     ` Vivien Didelot
2015-01-22 16:26       ` Darren Hart
2015-01-18 23:25 ` [PATCH v2 3/3] asus-laptop: cleanup is_visible Vivien Didelot
2015-01-20 16:48   ` Corentin Chary
2015-01-20 18:20     ` Vivien Didelot
2015-01-21 18:19       ` Darren Hart
2015-01-21 18:25 ` [PATCH v2 1/3] asus-laptop: fix is_visible return value Darren Hart

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox