[PATCH v2 1/3] asus-laptop: fix is_visible return value

[PATCH v2 1/3] asus-laptop: fix is_visible return value

[Vivien Didelot]

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

[PATCH v2 2/3] asus-laptop: use DEVICE_ATTR_* macros

[Vivien Didelot]

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

[PATCH v2 3/3] asus-laptop: cleanup is_visible

[Vivien Didelot]

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

Re: [PATCH v2 3/3] asus-laptop: cleanup is_visible

[Corentin Chary]

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

Re: [PATCH v2 3/3] asus-laptop: cleanup is_visible

[Corentin Chary]

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

Re: [PATCH v2 3/3] asus-laptop: cleanup is_visible

[Vivien Didelot]

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

Re: [PATCH v2 3/3] asus-laptop: cleanup is_visible

[Darren Hart]

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

Re: [PATCH v2 1/3] asus-laptop: fix is_visible return value

[Darren Hart]

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

Re: [PATCH v2 2/3] asus-laptop: use DEVICE_ATTR_* macros

[Darren Hart]

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

Re: [PATCH v2 2/3] asus-laptop: use DEVICE_ATTR_* macros

[Vivien Didelot]

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

Re: [PATCH v2 2/3] asus-laptop: use DEVICE_ATTR_* macros

[Darren Hart]

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