[PATCH v2 0/5] alienware-wmi new platform and feature support

[PATCH v2 0/5] alienware-wmi new platform and feature support

[Mario Limonciello]

I've got some extensions for the alienware-wmi driver that have
been introduced for a few new platforms and can be controlled via the WMI
interface.

Changes from V1:
 - Make sure whitespace changes are in their own patch
 - Add commit descriptions to all patches
 - Allow one line to go over 80 characters in deep sleep control patch
   Other patches may have some areas that avoid going over 80 characters
   still, but this matches the style of the existing driver.  If you would
   like these to go over 80 characters as well, please comment which areas
   this is OK.

Mario Limonciello (5):
  Clean up whitespace for ASM100 platform
  Add support for new platform: X51-R3
  Add initial support for alienware graphics amplifier.
  Add support for deep sleep control.
  Add support for two new systems: ASM200 and ASM201.

 drivers/platform/x86/alienware-wmi.c | 282 +++++++++++++++++++++++++++++++----
 1 file changed, 250 insertions(+), 32 deletions(-)

-- 
1.9.1

[PATCH v2 1/5] Clean up whitespace for ASM100 platform

[Mario Limonciello]

This brings them more in line with the usage of whitespace
in other platforms.

Signed-off-by: Mario Limonciello <mario_limonciello@dell.com>
---
 drivers/platform/x86/alienware-wmi.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/x86/alienware-wmi.c b/drivers/platform/x86/alienware-wmi.c
index 1e1e594..a8750ed 100644
--- a/drivers/platform/x86/alienware-wmi.c
+++ b/drivers/platform/x86/alienware-wmi.c
@@ -105,14 +105,14 @@ static const struct dmi_system_id alienware_quirks[] __initconst = {
 	 .driver_data = &quirk_x51_family,
 	 },
 	{
-		.callback = dmi_matched,
-		.ident = "Alienware ASM100",
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "Alienware"),
-			DMI_MATCH(DMI_PRODUCT_NAME, "ASM100"),
-		},
-		.driver_data = &quirk_asm100,
-	},
+	 .callback = dmi_matched,
+	 .ident = "Alienware ASM100",
+	 .matches = {
+		     DMI_MATCH(DMI_SYS_VENDOR, "Alienware"),
+		     DMI_MATCH(DMI_PRODUCT_NAME, "ASM100"),
+		     },
+	 .driver_data = &quirk_asm100,
+	 },
 	{}
 };
 
-- 
1.9.1

[PATCH v2 2/5] Add support for new platform: X51-R3

[Mario Limonciello]

The X51-R3 is in the X51 family.  It includes 3 internal
lighting zones as well as is the first AW desktop that
includes support for a graphics amplifier.

Signed-off-by: Mario Limonciello <mario_limonciello@dell.com>
---
 drivers/platform/x86/alienware-wmi.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/alienware-wmi.c b/drivers/platform/x86/alienware-wmi.c
index a8750ed..8e8ea4f 100644
--- a/drivers/platform/x86/alienware-wmi.c
+++ b/drivers/platform/x86/alienware-wmi.c
@@ -69,11 +69,16 @@ static struct quirk_entry quirk_unknown = {
 	.hdmi_mux = 0,
 };
 
-static struct quirk_entry quirk_x51_family = {
+static struct quirk_entry quirk_x51_r1_r2 = {
 	.num_zones = 3,
 	.hdmi_mux = 0.
 };
 
+static struct quirk_entry quirk_x51_r3 = {
+	.num_zones = 4,
+	.hdmi_mux = 0,
+};
+
 static struct quirk_entry quirk_asm100 = {
 	.num_zones = 2,
 	.hdmi_mux = 1,
@@ -88,12 +93,12 @@ static int __init dmi_matched(const struct dmi_system_id *dmi)
 static const struct dmi_system_id alienware_quirks[] __initconst = {
 	{
 	 .callback = dmi_matched,
-	 .ident = "Alienware X51 R1",
+	 .ident = "Alienware X51 R3",
 	 .matches = {
 		     DMI_MATCH(DMI_SYS_VENDOR, "Alienware"),
-		     DMI_MATCH(DMI_PRODUCT_NAME, "Alienware X51"),
+		     DMI_MATCH(DMI_PRODUCT_NAME, "Alienware X51 R3"),
 		     },
-	 .driver_data = &quirk_x51_family,
+	 .driver_data = &quirk_x51_r3,
 	 },
 	{
 	 .callback = dmi_matched,
@@ -102,7 +107,16 @@ static const struct dmi_system_id alienware_quirks[] __initconst = {
 		     DMI_MATCH(DMI_SYS_VENDOR, "Alienware"),
 		     DMI_MATCH(DMI_PRODUCT_NAME, "Alienware X51 R2"),
 		     },
-	 .driver_data = &quirk_x51_family,
+	 .driver_data = &quirk_x51_r1_r2,
+	 },
+	{
+	 .callback = dmi_matched,
+	 .ident = "Alienware X51 R1",
+	 .matches = {
+		     DMI_MATCH(DMI_SYS_VENDOR, "Alienware"),
+		     DMI_MATCH(DMI_PRODUCT_NAME, "Alienware X51"),
+		     },
+	 .driver_data = &quirk_x51_r1_r2,
 	 },
 	{
 	 .callback = dmi_matched,
-- 
1.9.1

[PATCH v2 3/5] Add initial support for alienware graphics amplifier.

[Mario Limonciello]

The alienware graphics amplifier is a device that provides external
access to a full PCIe slot, USB hub, and additional control zone.

This patch enables support for reading status whether the cable is
plugged in as well as for setting the colors in the new zone.

Signed-off-by: Mario Limonciello <mario_limonciello@dell.com>
---
 drivers/platform/x86/alienware-wmi.c | 110 +++++++++++++++++++++++++++++------
 1 file changed, 91 insertions(+), 19 deletions(-)

diff --git a/drivers/platform/x86/alienware-wmi.c b/drivers/platform/x86/alienware-wmi.c
index 8e8ea4f..e6f6322 100644
--- a/drivers/platform/x86/alienware-wmi.c
+++ b/drivers/platform/x86/alienware-wmi.c
@@ -33,6 +33,7 @@
 #define WMAX_METHOD_BRIGHTNESS		0x3
 #define WMAX_METHOD_ZONE_CONTROL	0x4
 #define WMAX_METHOD_HDMI_CABLE		0x5
+#define WMAX_METHOD_AMPLIFIER_CABLE	0x6
 
 MODULE_AUTHOR("Mario Limonciello <mario_limonciello@dell.com>");
 MODULE_DESCRIPTION("Alienware special feature control");
@@ -60,6 +61,7 @@ enum WMAX_CONTROL_STATES {
 struct quirk_entry {
 	u8 num_zones;
 	u8 hdmi_mux;
+	u8 amplifier;
 };
 
 static struct quirk_entry *quirks;
@@ -67,21 +69,25 @@ static struct quirk_entry *quirks;
 static struct quirk_entry quirk_unknown = {
 	.num_zones = 2,
 	.hdmi_mux = 0,
+	.amplifier = 0,
 };
 
 static struct quirk_entry quirk_x51_r1_r2 = {
 	.num_zones = 3,
-	.hdmi_mux = 0.
+	.hdmi_mux = 0,
+	.amplifier = 0,
 };
 
 static struct quirk_entry quirk_x51_r3 = {
 	.num_zones = 4,
 	.hdmi_mux = 0,
+	.amplifier = 1,
 };
 
 static struct quirk_entry quirk_asm100 = {
 	.num_zones = 2,
 	.hdmi_mux = 1,
+	.amplifier = 0,
 };
 
 static int __init dmi_matched(const struct dmi_system_id *dmi)
@@ -147,7 +153,7 @@ struct wmax_brightness_args {
 	u32 percentage;
 };
 
-struct hdmi_args {
+struct wmax_basic_args {
 	u8 arg;
 };
 
@@ -232,16 +238,16 @@ static int alienware_update_led(struct platform_zone *zone)
 	char *guid;
 	struct acpi_buffer input;
 	struct legacy_led_args legacy_args;
-	struct wmax_led_args wmax_args;
+	struct wmax_led_args wmax_basic_args;
 	if (interface == WMAX) {
-		wmax_args.led_mask = 1 << zone->location;
-		wmax_args.colors = zone->colors;
-		wmax_args.state = lighting_control_state;
+		wmax_basic_args.led_mask = 1 << zone->location;
+		wmax_basic_args.colors = zone->colors;
+		wmax_basic_args.state = lighting_control_state;
 		guid = WMAX_CONTROL_GUID;
 		method_id = WMAX_METHOD_ZONE_CONTROL;
 
-		input.length = (acpi_size) sizeof(wmax_args);
-		input.pointer = &wmax_args;
+		input.length = (acpi_size) sizeof(wmax_basic_args);
+		input.pointer = &wmax_basic_args;
 	} else {
 		legacy_args.colors = zone->colors;
 		legacy_args.brightness = global_brightness;
@@ -449,11 +455,7 @@ static void alienware_zone_exit(struct platform_device *dev)
 	kfree(zone_attrs);
 }
 
-/*
-	The HDMI mux sysfs node indicates the status of the HDMI input mux.
-	It can toggle between standard system GPU output and HDMI input.
-*/
-static acpi_status alienware_hdmi_command(struct hdmi_args *in_args,
+static acpi_status alienware_wmax_command(struct wmax_basic_args *in_args,
 					  u32 command, int *out_data)
 {
 	acpi_status status;
@@ -481,16 +483,20 @@ static acpi_status alienware_hdmi_command(struct hdmi_args *in_args,
 
 }
 
+/*
+ *	The HDMI mux sysfs node indicates the status of the HDMI input mux.
+ *	It can toggle between standard system GPU output and HDMI input.
+*/
 static ssize_t show_hdmi_cable(struct device *dev,
 			       struct device_attribute *attr, char *buf)
 {
 	acpi_status status;
 	u32 out_data;
-	struct hdmi_args in_args = {
+	struct wmax_basic_args in_args = {
 		.arg = 0,
 	};
 	status =
-	    alienware_hdmi_command(&in_args, WMAX_METHOD_HDMI_CABLE,
+	    alienware_wmax_command(&in_args, WMAX_METHOD_HDMI_CABLE,
 				   (u32 *) &out_data);
 	if (ACPI_SUCCESS(status)) {
 		if (out_data == 0)
@@ -509,11 +515,11 @@ static ssize_t show_hdmi_source(struct device *dev,
 {
 	acpi_status status;
 	u32 out_data;
-	struct hdmi_args in_args = {
+	struct wmax_basic_args in_args = {
 		.arg = 0,
 	};
 	status =
-	    alienware_hdmi_command(&in_args, WMAX_METHOD_HDMI_STATUS,
+	    alienware_wmax_command(&in_args, WMAX_METHOD_HDMI_STATUS,
 				   (u32 *) &out_data);
 
 	if (ACPI_SUCCESS(status)) {
@@ -533,7 +539,7 @@ static ssize_t toggle_hdmi_source(struct device *dev,
 				  const char *buf, size_t count)
 {
 	acpi_status status;
-	struct hdmi_args args;
+	struct wmax_basic_args args;
 	if (strcmp(buf, "gpu\n") == 0)
 		args.arg = 1;
 	else if (strcmp(buf, "input\n") == 0)
@@ -542,7 +548,7 @@ static ssize_t toggle_hdmi_source(struct device *dev,
 		args.arg = 3;
 	pr_debug("alienware-wmi: setting hdmi to %d : %s", args.arg, buf);
 
-	status = alienware_hdmi_command(&args, WMAX_METHOD_HDMI_SOURCE, NULL);
+	status = alienware_wmax_command(&args, WMAX_METHOD_HDMI_SOURCE, NULL);
 
 	if (ACPI_FAILURE(status))
 		pr_err("alienware-wmi: HDMI toggle failed: results: %u\n",
@@ -585,6 +591,65 @@ error_create_hdmi:
 	return ret;
 }
 
+/* Alienware GFX amplifier support
+ * - Currently supports reading cable status
+ * - Leaving expansion room to possibly support dock/undock events later
+*/
+static ssize_t show_amplifier_status(struct device *dev,
+				     struct device_attribute *attr, char *buf)
+{
+	acpi_status status;
+	u32 out_data;
+	struct wmax_basic_args in_args = {
+		.arg = 0,
+	};
+	status =
+	    alienware_wmax_command(&in_args, WMAX_METHOD_AMPLIFIER_CABLE,
+				   (u32 *) &out_data);
+	if (ACPI_SUCCESS(status)) {
+		if (out_data == 0)
+			return scnprintf(buf, PAGE_SIZE,
+					 "[unconnected] connected unknown\n");
+		else if (out_data == 1)
+			return scnprintf(buf, PAGE_SIZE,
+					 "unconnected [connected] unknown\n");
+	}
+	pr_err("alienware-wmi: unknown amplifier cable status: %d\n", status);
+	return scnprintf(buf, PAGE_SIZE, "unconnected connected [unknown]\n");
+}
+
+static DEVICE_ATTR(status, S_IRUGO, show_amplifier_status, NULL);
+
+static struct attribute *amplifier_attrs[] = {
+	&dev_attr_status.attr,
+	NULL,
+};
+
+static struct attribute_group amplifier_attribute_group = {
+	.name = "amplifier",
+	.attrs = amplifier_attrs,
+};
+
+static void remove_amplifier(struct platform_device *dev)
+{
+	if (quirks->amplifier > 0)
+		sysfs_remove_group(&dev->dev.kobj, &amplifier_attribute_group);
+}
+
+static int create_amplifier(struct platform_device *dev)
+{
+	int ret;
+
+	ret = sysfs_create_group(&dev->dev.kobj, &amplifier_attribute_group);
+	if (ret)
+		goto error_create_amplifier;
+	return 0;
+
+error_create_amplifier:
+	remove_amplifier(dev);
+	return ret;
+}
+
 static int __init alienware_wmi_init(void)
 {
 	int ret;
@@ -620,6 +685,12 @@ static int __init alienware_wmi_init(void)
 			goto fail_prep_hdmi;
 	}
 
+	if (quirks->amplifier > 0) {
+		ret = create_amplifier(platform_device);
+		if (ret)
+			goto fail_prep_amplifier;
+	}
+
 	ret = alienware_zone_init(platform_device);
 	if (ret)
 		goto fail_prep_zones;
@@ -628,6 +699,7 @@ static int __init alienware_wmi_init(void)
 
 fail_prep_zones:
 	alienware_zone_exit(platform_device);
+fail_prep_amplifier:
 fail_prep_hdmi:
 	platform_device_del(platform_device);
 fail_platform_device2:
-- 
1.9.1

Re: [PATCH v2 3/5] Add initial support for alienware graphics amplifier.

[Darren Hart]

On Mon, Feb 01, 2016 at 08:28:49PM -0600, Mario Limonciello wrote:
> The alienware graphics amplifier is a device that provides external
> access to a full PCIe slot, USB hub, and additional control zone.
> 
> This patch enables support for reading status whether the cable is
> plugged in as well as for setting the colors in the new zone.

Is this "new zone" related to the alienware graphics amplifier?

> 
> Signed-off-by: Mario Limonciello <mario_limonciello@dell.com>
> ---
>  drivers/platform/x86/alienware-wmi.c | 110 +++++++++++++++++++++++++++++------
>  1 file changed, 91 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/platform/x86/alienware-wmi.c b/drivers/platform/x86/alienware-wmi.c
> index 8e8ea4f..e6f6322 100644
> --- a/drivers/platform/x86/alienware-wmi.c
> +++ b/drivers/platform/x86/alienware-wmi.c
> @@ -33,6 +33,7 @@
>  #define WMAX_METHOD_BRIGHTNESS		0x3
>  #define WMAX_METHOD_ZONE_CONTROL	0x4
>  #define WMAX_METHOD_HDMI_CABLE		0x5
> +#define WMAX_METHOD_AMPLIFIER_CABLE	0x6
>  
>  MODULE_AUTHOR("Mario Limonciello <mario_limonciello@dell.com>");
>  MODULE_DESCRIPTION("Alienware special feature control");
> @@ -60,6 +61,7 @@ enum WMAX_CONTROL_STATES {
>  struct quirk_entry {
>  	u8 num_zones;
>  	u8 hdmi_mux;
> +	u8 amplifier;
>  };
>  
>  static struct quirk_entry *quirks;
> @@ -67,21 +69,25 @@ static struct quirk_entry *quirks;
>  static struct quirk_entry quirk_unknown = {
>  	.num_zones = 2,
>  	.hdmi_mux = 0,
> +	.amplifier = 0,
>  };
>  
>  static struct quirk_entry quirk_x51_r1_r2 = {
>  	.num_zones = 3,
> -	.hdmi_mux = 0.
> +	.hdmi_mux = 0,
> +	.amplifier = 0,
>  };
>  
>  static struct quirk_entry quirk_x51_r3 = {
>  	.num_zones = 4,
>  	.hdmi_mux = 0,
> +	.amplifier = 1,
>  };
>  
>  static struct quirk_entry quirk_asm100 = {
>  	.num_zones = 2,
>  	.hdmi_mux = 1,
> +	.amplifier = 0,
>  };
>  
>  static int __init dmi_matched(const struct dmi_system_id *dmi)
> @@ -147,7 +153,7 @@ struct wmax_brightness_args {
>  	u32 percentage;
>  };
>  
> -struct hdmi_args {
> +struct wmax_basic_args {
>  	u8 arg;
>  };
>  
> @@ -232,16 +238,16 @@ static int alienware_update_led(struct platform_zone *zone)
>  	char *guid;
>  	struct acpi_buffer input;
>  	struct legacy_led_args legacy_args;
> -	struct wmax_led_args wmax_args;
> +	struct wmax_led_args wmax_basic_args;
>  	if (interface == WMAX) {
> -		wmax_args.led_mask = 1 << zone->location;
> -		wmax_args.colors = zone->colors;
> -		wmax_args.state = lighting_control_state;
> +		wmax_basic_args.led_mask = 1 << zone->location;
> +		wmax_basic_args.colors = zone->colors;
> +		wmax_basic_args.state = lighting_control_state;
>  		guid = WMAX_CONTROL_GUID;
>  		method_id = WMAX_METHOD_ZONE_CONTROL;
>  
> -		input.length = (acpi_size) sizeof(wmax_args);
> -		input.pointer = &wmax_args;
> +		input.length = (acpi_size) sizeof(wmax_basic_args);
> +		input.pointer = &wmax_basic_args;
>  	} else {
>  		legacy_args.colors = zone->colors;
>  		legacy_args.brightness = global_brightness;
> @@ -449,11 +455,7 @@ static void alienware_zone_exit(struct platform_device *dev)
>  	kfree(zone_attrs);
>  }
>  
> -/*
> -	The HDMI mux sysfs node indicates the status of the HDMI input mux.
> -	It can toggle between standard system GPU output and HDMI input.
> -*/
> -static acpi_status alienware_hdmi_command(struct hdmi_args *in_args,
> +static acpi_status alienware_wmax_command(struct wmax_basic_args *in_args,
>  					  u32 command, int *out_data)
>  {
>  	acpi_status status;
> @@ -481,16 +483,20 @@ static acpi_status alienware_hdmi_command(struct hdmi_args *in_args,
>  
>  }
>  
> +/*
> + *	The HDMI mux sysfs node indicates the status of the HDMI input mux.
> + *	It can toggle between standard system GPU output and HDMI input.
> +*/

Nit, the last * should align with the preceding, same in comment blocks below.

>  static ssize_t show_hdmi_cable(struct device *dev,
>  			       struct device_attribute *attr, char *buf)
>  {
>  	acpi_status status;
>  	u32 out_data;
> -	struct hdmi_args in_args = {
> +	struct wmax_basic_args in_args = {
>  		.arg = 0,
>  	};
>  	status =
> -	    alienware_hdmi_command(&in_args, WMAX_METHOD_HDMI_CABLE,
> +	    alienware_wmax_command(&in_args, WMAX_METHOD_HDMI_CABLE,
>  				   (u32 *) &out_data);
>  	if (ACPI_SUCCESS(status)) {
>  		if (out_data == 0)
> @@ -509,11 +515,11 @@ static ssize_t show_hdmi_source(struct device *dev,
>  {
>  	acpi_status status;
>  	u32 out_data;
> -	struct hdmi_args in_args = {
> +	struct wmax_basic_args in_args = {
>  		.arg = 0,
>  	};
>  	status =
> -	    alienware_hdmi_command(&in_args, WMAX_METHOD_HDMI_STATUS,
> +	    alienware_wmax_command(&in_args, WMAX_METHOD_HDMI_STATUS,
>  				   (u32 *) &out_data);
>  
>  	if (ACPI_SUCCESS(status)) {
> @@ -533,7 +539,7 @@ static ssize_t toggle_hdmi_source(struct device *dev,
>  				  const char *buf, size_t count)
>  {
>  	acpi_status status;
> -	struct hdmi_args args;
> +	struct wmax_basic_args args;
>  	if (strcmp(buf, "gpu\n") == 0)
>  		args.arg = 1;
>  	else if (strcmp(buf, "input\n") == 0)
> @@ -542,7 +548,7 @@ static ssize_t toggle_hdmi_source(struct device *dev,
>  		args.arg = 3;
>  	pr_debug("alienware-wmi: setting hdmi to %d : %s", args.arg, buf);
>  
> -	status = alienware_hdmi_command(&args, WMAX_METHOD_HDMI_SOURCE, NULL);
> +	status = alienware_wmax_command(&args, WMAX_METHOD_HDMI_SOURCE, NULL);
>  
>  	if (ACPI_FAILURE(status))
>  		pr_err("alienware-wmi: HDMI toggle failed: results: %u\n",
> @@ -585,6 +591,65 @@ error_create_hdmi:
>  	return ret;

All of the above hdmi to wmax changes seem strange/out-of-place in the context
of this patch. Are these a functionally independent change that could be done
independently? If not, can you comment on why we're making this change? Sorry if
it's obvious.

>  }
>  
> +/* Alienware GFX amplifier support
> + * - Currently supports reading cable status
> + * - Leaving expansion room to possibly support dock/undock events later
> +*/
> +static ssize_t show_amplifier_status(struct device *dev,
> +				     struct device_attribute *attr, char *buf)
> +{
> +	acpi_status status;
> +	u32 out_data;
> +	struct wmax_basic_args in_args = {
> +		.arg = 0,
> +	};
> +	status =
> +	    alienware_wmax_command(&in_args, WMAX_METHOD_AMPLIFIER_CABLE,
> +				   (u32 *) &out_data);
> +	if (ACPI_SUCCESS(status)) {
> +		if (out_data == 0)
> +			return scnprintf(buf, PAGE_SIZE,
> +					 "[unconnected] connected unknown\n");
> +		else if (out_data == 1)
> +			return scnprintf(buf, PAGE_SIZE,
> +					 "unconnected [connected] unknown\n");
> +	}
> +	pr_err("alienware-wmi: unknown amplifier cable status: %d\n", status);
> +	return scnprintf(buf, PAGE_SIZE, "unconnected connected [unknown]\n");
> +}
> +
> +static DEVICE_ATTR(status, S_IRUGO, show_amplifier_status, NULL);
> +
> +static struct attribute *amplifier_attrs[] = {
> +	&dev_attr_status.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group amplifier_attribute_group = {
> +	.name = "amplifier",
> +	.attrs = amplifier_attrs,
> +};
> +
> +static void remove_amplifier(struct platform_device *dev)
> +{
> +	if (quirks->amplifier > 0)
> +		sysfs_remove_group(&dev->dev.kobj, &amplifier_attribute_group);
> +}
> +
> +static int create_amplifier(struct platform_device *dev)
> +{
> +	int ret;
> +
> +	ret = sysfs_create_group(&dev->dev.kobj, &amplifier_attribute_group);
> +	if (ret)
> +		goto error_create_amplifier;
> +	return 0;
> +
> +error_create_amplifier:
> +	remove_amplifier(dev);
> +	return ret;

The goto label isn't necessary here:

if (ret)
	remove_amplifier(dev);
return ret;

This covers the error and success case and is 4 lines shorter.

> +}
> +
>  static int __init alienware_wmi_init(void)
>  {
>  	int ret;
> @@ -620,6 +685,12 @@ static int __init alienware_wmi_init(void)
>  			goto fail_prep_hdmi;
>  	}
>  
> +	if (quirks->amplifier > 0) {
> +		ret = create_amplifier(platform_device);
> +		if (ret)
> +			goto fail_prep_amplifier;

Do you feel the "right thing" to do here is to fail hard? Would it be possible
to load the driver without amplifier support instead?

I don't care which you choose, I just want to have the discussion. Sometimes
we'll match a platform and fail on something like this and abort, which in turns
removes functionality the user could otherwise benefit from.

Your call.

> +	}
> +
>  	ret = alienware_zone_init(platform_device);
>  	if (ret)
>  		goto fail_prep_zones;
> @@ -628,6 +699,7 @@ static int __init alienware_wmi_init(void)
>  
>  fail_prep_zones:
>  	alienware_zone_exit(platform_device);
> +fail_prep_amplifier:
>  fail_prep_hdmi:
>  	platform_device_del(platform_device);
>  fail_platform_device2:
> -- 
> 1.9.1
> 
> 

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v2 3/5] Add initial support for alienware graphics amplifier.

[Mario Limonciello]

On 02/02/2016 11:24 AM, Darren Hart wrote:
> On Mon, Feb 01, 2016 at 08:28:49PM -0600, Mario Limonciello wrote:
>> The alienware graphics amplifier is a device that provides external
>> access to a full PCIe slot, USB hub, and additional control zone.
>>
>> This patch enables support for reading status whether the cable is
>> plugged in as well as for setting the colors in the new zone.
> Is this "new zone" related to the alienware graphics amplifier?

Yes it is.  The interface allows for changing it even if the amplifier
isn't plugged in. 
When it gets plugged in, that zone would be set to the color picked
previously.

>
>> Signed-off-by: Mario Limonciello <mario_limonciello@dell.com>
>> ---
>>  drivers/platform/x86/alienware-wmi.c | 110 +++++++++++++++++++++++++++++------
>>  1 file changed, 91 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/platform/x86/alienware-wmi.c b/drivers/platform/x86/alienware-wmi.c
>> index 8e8ea4f..e6f6322 100644
>> --- a/drivers/platform/x86/alienware-wmi.c
>> +++ b/drivers/platform/x86/alienware-wmi.c
>> @@ -33,6 +33,7 @@
>>  #define WMAX_METHOD_BRIGHTNESS		0x3
>>  #define WMAX_METHOD_ZONE_CONTROL	0x4
>>  #define WMAX_METHOD_HDMI_CABLE		0x5
>> +#define WMAX_METHOD_AMPLIFIER_CABLE	0x6
>>  
>>  MODULE_AUTHOR("Mario Limonciello <mario_limonciello@dell.com>");
>>  MODULE_DESCRIPTION("Alienware special feature control");
>> @@ -60,6 +61,7 @@ enum WMAX_CONTROL_STATES {
>>  struct quirk_entry {
>>  	u8 num_zones;
>>  	u8 hdmi_mux;
>> +	u8 amplifier;
>>  };
>>  
>>  static struct quirk_entry *quirks;
>> @@ -67,21 +69,25 @@ static struct quirk_entry *quirks;
>>  static struct quirk_entry quirk_unknown = {
>>  	.num_zones = 2,
>>  	.hdmi_mux = 0,
>> +	.amplifier = 0,
>>  };
>>  
>>  static struct quirk_entry quirk_x51_r1_r2 = {
>>  	.num_zones = 3,
>> -	.hdmi_mux = 0.
>> +	.hdmi_mux = 0,
>> +	.amplifier = 0,
>>  };
>>  
>>  static struct quirk_entry quirk_x51_r3 = {
>>  	.num_zones = 4,
>>  	.hdmi_mux = 0,
>> +	.amplifier = 1,
>>  };
>>  
>>  static struct quirk_entry quirk_asm100 = {
>>  	.num_zones = 2,
>>  	.hdmi_mux = 1,
>> +	.amplifier = 0,
>>  };
>>  
>>  static int __init dmi_matched(const struct dmi_system_id *dmi)
>> @@ -147,7 +153,7 @@ struct wmax_brightness_args {
>>  	u32 percentage;
>>  };
>>  
>> -struct hdmi_args {
>> +struct wmax_basic_args {
>>  	u8 arg;
>>  };
>>  
>> @@ -232,16 +238,16 @@ static int alienware_update_led(struct platform_zone *zone)
>>  	char *guid;
>>  	struct acpi_buffer input;
>>  	struct legacy_led_args legacy_args;
>> -	struct wmax_led_args wmax_args;
>> +	struct wmax_led_args wmax_basic_args;
>>  	if (interface == WMAX) {
>> -		wmax_args.led_mask = 1 << zone->location;
>> -		wmax_args.colors = zone->colors;
>> -		wmax_args.state = lighting_control_state;
>> +		wmax_basic_args.led_mask = 1 << zone->location;
>> +		wmax_basic_args.colors = zone->colors;
>> +		wmax_basic_args.state = lighting_control_state;
>>  		guid = WMAX_CONTROL_GUID;
>>  		method_id = WMAX_METHOD_ZONE_CONTROL;
>>  
>> -		input.length = (acpi_size) sizeof(wmax_args);
>> -		input.pointer = &wmax_args;
>> +		input.length = (acpi_size) sizeof(wmax_basic_args);
>> +		input.pointer = &wmax_basic_args;
>>  	} else {
>>  		legacy_args.colors = zone->colors;
>>  		legacy_args.brightness = global_brightness;
>> @@ -449,11 +455,7 @@ static void alienware_zone_exit(struct platform_device *dev)
>>  	kfree(zone_attrs);
>>  }
>>  
>> -/*
>> -	The HDMI mux sysfs node indicates the status of the HDMI input mux.
>> -	It can toggle between standard system GPU output and HDMI input.
>> -*/
>> -static acpi_status alienware_hdmi_command(struct hdmi_args *in_args,
>> +static acpi_status alienware_wmax_command(struct wmax_basic_args *in_args,
>>  					  u32 command, int *out_data)
>>  {
>>  	acpi_status status;
>> @@ -481,16 +483,20 @@ static acpi_status alienware_hdmi_command(struct hdmi_args *in_args,
>>  
>>  }
>>  
>> +/*
>> + *	The HDMI mux sysfs node indicates the status of the HDMI input mux.
>> + *	It can toggle between standard system GPU output and HDMI input.
>> +*/
> Nit, the last * should align with the preceding, same in comment blocks below.

OK, will fix in next submission.

>>  static ssize_t show_hdmi_cable(struct device *dev,
>>  			       struct device_attribute *attr, char *buf)
>>  {
>>  	acpi_status status;
>>  	u32 out_data;
>> -	struct hdmi_args in_args = {
>> +	struct wmax_basic_args in_args = {
>>  		.arg = 0,
>>  	};
>>  	status =
>> -	    alienware_hdmi_command(&in_args, WMAX_METHOD_HDMI_CABLE,
>> +	    alienware_wmax_command(&in_args, WMAX_METHOD_HDMI_CABLE,
>>  				   (u32 *) &out_data);
>>  	if (ACPI_SUCCESS(status)) {
>>  		if (out_data == 0)
>> @@ -509,11 +515,11 @@ static ssize_t show_hdmi_source(struct device *dev,
>>  {
>>  	acpi_status status;
>>  	u32 out_data;
>> -	struct hdmi_args in_args = {
>> +	struct wmax_basic_args in_args = {
>>  		.arg = 0,
>>  	};
>>  	status =
>> -	    alienware_hdmi_command(&in_args, WMAX_METHOD_HDMI_STATUS,
>> +	    alienware_wmax_command(&in_args, WMAX_METHOD_HDMI_STATUS,
>>  				   (u32 *) &out_data);
>>  
>>  	if (ACPI_SUCCESS(status)) {
>> @@ -533,7 +539,7 @@ static ssize_t toggle_hdmi_source(struct device *dev,
>>  				  const char *buf, size_t count)
>>  {
>>  	acpi_status status;
>> -	struct hdmi_args args;
>> +	struct wmax_basic_args args;
>>  	if (strcmp(buf, "gpu\n") == 0)
>>  		args.arg = 1;
>>  	else if (strcmp(buf, "input\n") == 0)
>> @@ -542,7 +548,7 @@ static ssize_t toggle_hdmi_source(struct device *dev,
>>  		args.arg = 3;
>>  	pr_debug("alienware-wmi: setting hdmi to %d : %s", args.arg, buf);
>>  
>> -	status = alienware_hdmi_command(&args, WMAX_METHOD_HDMI_SOURCE, NULL);
>> +	status = alienware_wmax_command(&args, WMAX_METHOD_HDMI_SOURCE, NULL);
>>  
>>  	if (ACPI_FAILURE(status))
>>  		pr_err("alienware-wmi: HDMI toggle failed: results: %u\n",
>> @@ -585,6 +591,65 @@ error_create_hdmi:
>>  	return ret;
> All of the above hdmi to wmax changes seem strange/out-of-place in the context
> of this patch. Are these a functionally independent change that could be done
> independently? If not, can you comment on why we're making this change? Sorry if
> it's obvious.

Before this platform was released we defined the interface of extended
commands to support only HDMI toggling on ASM100.  Later on after more
platforms were released (X51-R3 and others) the interface was extended
to support other commands that weren't envisioned originally.  Because
of this it makes more sense to call these "wmax" commands.

>>  }
>>  
>> +/* Alienware GFX amplifier support
>> + * - Currently supports reading cable status
>> + * - Leaving expansion room to possibly support dock/undock events later
>> +*/
>> +static ssize_t show_amplifier_status(struct device *dev,
>> +				     struct device_attribute *attr, char *buf)
>> +{
>> +	acpi_status status;
>> +	u32 out_data;
>> +	struct wmax_basic_args in_args = {
>> +		.arg = 0,
>> +	};
>> +	status =
>> +	    alienware_wmax_command(&in_args, WMAX_METHOD_AMPLIFIER_CABLE,
>> +				   (u32 *) &out_data);
>> +	if (ACPI_SUCCESS(status)) {
>> +		if (out_data == 0)
>> +			return scnprintf(buf, PAGE_SIZE,
>> +					 "[unconnected] connected unknown\n");
>> +		else if (out_data == 1)
>> +			return scnprintf(buf, PAGE_SIZE,
>> +					 "unconnected [connected] unknown\n");
>> +	}
>> +	pr_err("alienware-wmi: unknown amplifier cable status: %d\n", status);
>> +	return scnprintf(buf, PAGE_SIZE, "unconnected connected [unknown]\n");
>> +}
>> +
>> +static DEVICE_ATTR(status, S_IRUGO, show_amplifier_status, NULL);
>> +
>> +static struct attribute *amplifier_attrs[] = {
>> +	&dev_attr_status.attr,
>> +	NULL,
>> +};
>> +
>> +static struct attribute_group amplifier_attribute_group = {
>> +	.name = "amplifier",
>> +	.attrs = amplifier_attrs,
>> +};
>> +
>> +static void remove_amplifier(struct platform_device *dev)
>> +{
>> +	if (quirks->amplifier > 0)
>> +		sysfs_remove_group(&dev->dev.kobj, &amplifier_attribute_group);
>> +}
>> +
>> +static int create_amplifier(struct platform_device *dev)
>> +{
>> +	int ret;
>> +
>> +	ret = sysfs_create_group(&dev->dev.kobj, &amplifier_attribute_group);
>> +	if (ret)
>> +		goto error_create_amplifier;
>> +	return 0;
>> +
>> +error_create_amplifier:
>> +	remove_amplifier(dev);
>> +	return ret;
> The goto label isn't necessary here:
>
> if (ret)
> 	remove_amplifier(dev);
> return ret;
>
> This covers the error and success case and is 4 lines shorter.

OK, will fix in next submission.

>> +}
>> +
>>  static int __init alienware_wmi_init(void)
>>  {
>>  	int ret;
>> @@ -620,6 +685,12 @@ static int __init alienware_wmi_init(void)
>>  			goto fail_prep_hdmi;
>>  	}
>>  
>> +	if (quirks->amplifier > 0) {
>> +		ret = create_amplifier(platform_device);
>> +		if (ret)
>> +			goto fail_prep_amplifier;
> Do you feel the "right thing" to do here is to fail hard? Would it be possible
> to load the driver without amplifier support instead?
>
> I don't care which you choose, I just want to have the discussion. Sometimes
> we'll match a platform and fail on something like this and abort, which in turns
> removes functionality the user could otherwise benefit from.
>
> Your call.

Technically yes I think it would be possible to load it without
amplifier support, but I gave this some thought and really I want
everything powered by this interface to be all or nothing.  If parts of
it aren't present and userspace apps expect them (by the DMI data for
the platform) it will be another layer to debug what went wrong.  Given
how simple the initialization of create_amplifier is, if there is a
failure I'm expecting some other more interesting kernel or memory usage
problems need to be debugged anyhow :)

>
>> +	}
>> +
>>  	ret = alienware_zone_init(platform_device);
>>  	if (ret)
>>  		goto fail_prep_zones;
>> @@ -628,6 +699,7 @@ static int __init alienware_wmi_init(void)
>>  
>>  fail_prep_zones:
>>  	alienware_zone_exit(platform_device);
>> +fail_prep_amplifier:
>>  fail_prep_hdmi:
>>  	platform_device_del(platform_device);
>>  fail_platform_device2:
>> -- 
>> 1.9.1
>>
>>

[PATCH v2 4/5] Add support for deep sleep control.

[Mario Limonciello]

This allows configuration the system for wakeup with a controller.
The feature requires hardware architectural modifications and can
not be supported on on existing systems.

Signed-off-by: Mario Limonciello <mario_limonciello@dell.com>
---
 drivers/platform/x86/alienware-wmi.c | 100 +++++++++++++++++++++++++++++++++++
 1 file changed, 100 insertions(+)

diff --git a/drivers/platform/x86/alienware-wmi.c b/drivers/platform/x86/alienware-wmi.c
index e6f6322..f5ebcae 100644
--- a/drivers/platform/x86/alienware-wmi.c
+++ b/drivers/platform/x86/alienware-wmi.c
@@ -34,6 +34,8 @@
 #define WMAX_METHOD_ZONE_CONTROL	0x4
 #define WMAX_METHOD_HDMI_CABLE		0x5
 #define WMAX_METHOD_AMPLIFIER_CABLE	0x6
+#define WMAX_METHOD_DEEP_SLEEP_CONTROL	0x0B
+#define WMAX_METHOD_DEEP_SLEEP_STATUS	0x0C
 
 MODULE_AUTHOR("Mario Limonciello <mario_limonciello@dell.com>");
 MODULE_DESCRIPTION("Alienware special feature control");
@@ -62,6 +64,7 @@ struct quirk_entry {
 	u8 num_zones;
 	u8 hdmi_mux;
 	u8 amplifier;
+	u8 deepslp;
 };
 
 static struct quirk_entry *quirks;
@@ -70,24 +73,28 @@ static struct quirk_entry quirk_unknown = {
 	.num_zones = 2,
 	.hdmi_mux = 0,
 	.amplifier = 0,
+	.deepslp = 0,
 };
 
 static struct quirk_entry quirk_x51_r1_r2 = {
 	.num_zones = 3,
 	.hdmi_mux = 0,
 	.amplifier = 0,
+	.deepslp = 0,
 };
 
 static struct quirk_entry quirk_x51_r3 = {
 	.num_zones = 4,
 	.hdmi_mux = 0,
 	.amplifier = 1,
+	.deepslp = 0,
 };
 
 static struct quirk_entry quirk_asm100 = {
 	.num_zones = 2,
 	.hdmi_mux = 1,
 	.amplifier = 0,
+	.deepslp = 0,
 };
 
 static int __init dmi_matched(const struct dmi_system_id *dmi)
@@ -650,6 +657,92 @@ error_create_amplifier:
 	return ret;
 }
 
+/* Deep Sleep Control support
+ * - Modifies BIOS setting for deep sleep control allowing extra wakeup events
+*/
+static ssize_t show_deepsleep_status(struct device *dev,
+				     struct device_attribute *attr, char *buf)
+{
+	acpi_status status;
+	u32 out_data;
+	struct wmax_basic_args in_args = {
+		.arg = 0,
+	};
+	status =
+	    alienware_wmax_command(&in_args, WMAX_METHOD_DEEP_SLEEP_STATUS,
+				   (u32 *) &out_data);
+	if (ACPI_SUCCESS(status)) {
+		if (out_data == 0)
+			return scnprintf(buf, PAGE_SIZE,
+					 "[disabled] s5 s5_s4\n");
+		else if (out_data == 1)
+			return scnprintf(buf, PAGE_SIZE,
+					 "disabled [s5] s5_s4\n");
+		else if (out_data == 2)
+			return scnprintf(buf, PAGE_SIZE,
+					 "disabled s5 [s5_s4]\n");
+	}
+	pr_err("alienware-wmi: unknown deep sleep status: %d\n", status);
+	return scnprintf(buf, PAGE_SIZE, "disabled s5 s5_s4 [unknown]\n");
+}
+
+static ssize_t toggle_deepsleep(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	acpi_status status;
+	struct wmax_basic_args args;
+
+	if (strcmp(buf, "disabled\n") == 0)
+		args.arg = 0;
+	else if (strcmp(buf, "s5\n") == 0)
+		args.arg = 1;
+	else
+		args.arg = 2;
+	pr_debug("alienware-wmi: setting deep sleep to %d : %s", args.arg, buf);
+
+	status =
+	    alienware_wmax_command(&args, WMAX_METHOD_DEEP_SLEEP_CONTROL, NULL);
+
+	if (ACPI_FAILURE(status))
+		pr_err
+		    ("alienware-wmi: deep sleep control failed: results: %u\n",
+		     status);
+	return count;
+}
+
+static DEVICE_ATTR(deepsleep, S_IRUGO | S_IWUSR, show_deepsleep_status, toggle_deepsleep);
+
+static struct attribute *deepsleep_attrs[] = {
+	&dev_attr_deepsleep.attr,
+	NULL,
+};
+
+static struct attribute_group deepsleep_attribute_group = {
+	.name = "deepsleep",
+	.attrs = deepsleep_attrs,
+};
+
+static void remove_deepsleep(struct platform_device *dev)
+{
+	if (quirks->deepslp > 0)
+		sysfs_remove_group(&dev->dev.kobj, &deepsleep_attribute_group);
+}
+
+static int create_deepsleep(struct platform_device *dev)
+{
+	int ret;
+
+	ret = sysfs_create_group(&dev->dev.kobj, &deepsleep_attribute_group);
+	if (ret)
+		goto error_create_deepsleep;
+	return 0;
+
+error_create_deepsleep:
+	remove_deepsleep(dev);
+	return ret;
+}
+
 static int __init alienware_wmi_init(void)
 {
 	int ret;
@@ -691,6 +784,12 @@ static int __init alienware_wmi_init(void)
 			goto fail_prep_amplifier;
 	}
 
+	if (quirks->deepslp > 0) {
+		ret = create_deepsleep(platform_device);
+		if (ret)
+			goto fail_prep_deepsleep;
+	}
+
 	ret = alienware_zone_init(platform_device);
 	if (ret)
 		goto fail_prep_zones;
@@ -699,6 +798,7 @@ static int __init alienware_wmi_init(void)
 
 fail_prep_zones:
 	alienware_zone_exit(platform_device);
+fail_prep_deepsleep:
 fail_prep_amplifier:
 fail_prep_hdmi:
 	platform_device_del(platform_device);
-- 
1.9.1

Re: [PATCH v2 4/5] Add support for deep sleep control.

[Darren Hart]

On Mon, Feb 01, 2016 at 08:28:50PM -0600, Mario Limonciello wrote:
> This allows configuration the system for wakeup with a controller.

Hrm, I'm happy to clean up English grammar in commit messages... but I'm
struggling with the intent of the above... Is this correct:

Allow for user configuration, via sysfs, for wakeup with a controller.

If so, great - but also, what do we mean by "with a controller" ?

> The feature requires hardware architectural modifications and can
> not be supported on on existing systems.
> 
> Signed-off-by: Mario Limonciello <mario_limonciello@dell.com>
> ---
>  drivers/platform/x86/alienware-wmi.c | 100 +++++++++++++++++++++++++++++++++++
>  1 file changed, 100 insertions(+)
> 
> diff --git a/drivers/platform/x86/alienware-wmi.c b/drivers/platform/x86/alienware-wmi.c
> index e6f6322..f5ebcae 100644
> --- a/drivers/platform/x86/alienware-wmi.c
> +++ b/drivers/platform/x86/alienware-wmi.c
> @@ -34,6 +34,8 @@
>  #define WMAX_METHOD_ZONE_CONTROL	0x4
>  #define WMAX_METHOD_HDMI_CABLE		0x5
>  #define WMAX_METHOD_AMPLIFIER_CABLE	0x6
> +#define WMAX_METHOD_DEEP_SLEEP_CONTROL	0x0B
> +#define WMAX_METHOD_DEEP_SLEEP_STATUS	0x0C
>  
>  MODULE_AUTHOR("Mario Limonciello <mario_limonciello@dell.com>");
>  MODULE_DESCRIPTION("Alienware special feature control");
> @@ -62,6 +64,7 @@ struct quirk_entry {
>  	u8 num_zones;
>  	u8 hdmi_mux;
>  	u8 amplifier;
> +	u8 deepslp;
>  };
>  
>  static struct quirk_entry *quirks;
> @@ -70,24 +73,28 @@ static struct quirk_entry quirk_unknown = {
>  	.num_zones = 2,
>  	.hdmi_mux = 0,
>  	.amplifier = 0,
> +	.deepslp = 0,
>  };
>  
>  static struct quirk_entry quirk_x51_r1_r2 = {
>  	.num_zones = 3,
>  	.hdmi_mux = 0,
>  	.amplifier = 0,
> +	.deepslp = 0,
>  };
>  
>  static struct quirk_entry quirk_x51_r3 = {
>  	.num_zones = 4,
>  	.hdmi_mux = 0,
>  	.amplifier = 1,
> +	.deepslp = 0,
>  };
>  
>  static struct quirk_entry quirk_asm100 = {
>  	.num_zones = 2,
>  	.hdmi_mux = 1,
>  	.amplifier = 0,
> +	.deepslp = 0,
>  };
>  
>  static int __init dmi_matched(const struct dmi_system_id *dmi)
> @@ -650,6 +657,92 @@ error_create_amplifier:
>  	return ret;
>  }
>  
> +/* Deep Sleep Control support
> + * - Modifies BIOS setting for deep sleep control allowing extra wakeup events
> +*/
> +static ssize_t show_deepsleep_status(struct device *dev,
> +				     struct device_attribute *attr, char *buf)
> +{
> +	acpi_status status;
> +	u32 out_data;
> +	struct wmax_basic_args in_args = {
> +		.arg = 0,
> +	};
> +	status =
> +	    alienware_wmax_command(&in_args, WMAX_METHOD_DEEP_SLEEP_STATUS,
> +				   (u32 *) &out_data);
> +	if (ACPI_SUCCESS(status)) {
> +		if (out_data == 0)
> +			return scnprintf(buf, PAGE_SIZE,
> +					 "[disabled] s5 s5_s4\n");
> +		else if (out_data == 1)
> +			return scnprintf(buf, PAGE_SIZE,
> +					 "disabled [s5] s5_s4\n");
> +		else if (out_data == 2)
> +			return scnprintf(buf, PAGE_SIZE,
> +					 "disabled s5 [s5_s4]\n");
> +	}
> +	pr_err("alienware-wmi: unknown deep sleep status: %d\n", status);
> +	return scnprintf(buf, PAGE_SIZE, "disabled s5 s5_s4 [unknown]\n");
> +}
> +
> +static ssize_t toggle_deepsleep(struct device *dev,
> +				struct device_attribute *attr,
> +				const char *buf, size_t count)
> +{
> +	acpi_status status;
> +	struct wmax_basic_args args;
> +
> +	if (strcmp(buf, "disabled\n") == 0)
> +		args.arg = 0;
> +	else if (strcmp(buf, "s5\n") == 0)
> +		args.arg = 1;
> +	else
> +		args.arg = 2;
> +	pr_debug("alienware-wmi: setting deep sleep to %d : %s", args.arg, buf);
> +
> +	status =
> +	    alienware_wmax_command(&args, WMAX_METHOD_DEEP_SLEEP_CONTROL, NULL);

Prefer to wrap the arguments:

	status = alienware_wmax_command(&args, WMAX_METHOD_DEEP_SLEEP_CONTROL,
					NULL);

> +
> +	if (ACPI_FAILURE(status))
> +		pr_err
> +		    ("alienware-wmi: deep sleep control failed: results: %u\n",

Here, I don't consider the above to be "breakable" in terms of line wrapping.

		pr_err("alienware-wmi: deep sleep control failed: results: %u\n",

Please.

> +		     status);
> +	return count;
> +}
> +
> +static DEVICE_ATTR(deepsleep, S_IRUGO | S_IWUSR, show_deepsleep_status, toggle_deepsleep);
> +
> +static struct attribute *deepsleep_attrs[] = {
> +	&dev_attr_deepsleep.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group deepsleep_attribute_group = {
> +	.name = "deepsleep",
> +	.attrs = deepsleep_attrs,
> +};
> +
> +static void remove_deepsleep(struct platform_device *dev)
> +{
> +	if (quirks->deepslp > 0)
> +		sysfs_remove_group(&dev->dev.kobj, &deepsleep_attribute_group);
> +}
> +
> +static int create_deepsleep(struct platform_device *dev)
> +{
> +	int ret;
> +
> +	ret = sysfs_create_group(&dev->dev.kobj, &deepsleep_attribute_group);
> +	if (ret)
> +		goto error_create_deepsleep;
> +	return 0;
> +
> +error_create_deepsleep:
> +	remove_deepsleep(dev);
> +	return ret;

As i the previous patch, this goto label is unnecessary.

> +}
> +
>  static int __init alienware_wmi_init(void)
>  {
>  	int ret;
> @@ -691,6 +784,12 @@ static int __init alienware_wmi_init(void)
>  			goto fail_prep_amplifier;
>  	}
>  
> +	if (quirks->deepslp > 0) {
> +		ret = create_deepsleep(platform_device);
> +		if (ret)
> +			goto fail_prep_deepsleep;
> +	}
> +
>  	ret = alienware_zone_init(platform_device);
>  	if (ret)
>  		goto fail_prep_zones;
> @@ -699,6 +798,7 @@ static int __init alienware_wmi_init(void)
>  
>  fail_prep_zones:
>  	alienware_zone_exit(platform_device);
> +fail_prep_deepsleep:
>  fail_prep_amplifier:
>  fail_prep_hdmi:
>  	platform_device_del(platform_device);
> -- 
> 1.9.1
> 
> 

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v2 4/5] Add support for deep sleep control.

[Mario Limonciello]

On 02/02/2016 11:32 AM, Darren Hart wrote:
> On Mon, Feb 01, 2016 at 08:28:50PM -0600, Mario Limonciello wrote:
>> This allows configuration the system for wakeup with a controller.
> Hrm, I'm happy to clean up English grammar in commit messages... but I'm
> struggling with the intent of the above... Is this correct:
>
> Allow for user configuration, via sysfs, for wakeup with a controller.
>
> If so, great - but also, what do we mean by "with a controller" ?

I see how this is confusing to someone who isn't in the know what's
going on.
A better description is:

Allow for user configuration of BIOS settings that allow the system to
turned on via HID devices.

If that's concise enough I'll update in next submission.

>> The feature requires hardware architectural modifications and can
>> not be supported on on existing systems.
>>
>> Signed-off-by: Mario Limonciello <mario_limonciello@dell.com>
>> ---
>>  drivers/platform/x86/alienware-wmi.c | 100 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 100 insertions(+)
>>
>> diff --git a/drivers/platform/x86/alienware-wmi.c b/drivers/platform/x86/alienware-wmi.c
>> index e6f6322..f5ebcae 100644
>> --- a/drivers/platform/x86/alienware-wmi.c
>> +++ b/drivers/platform/x86/alienware-wmi.c
>> @@ -34,6 +34,8 @@
>>  #define WMAX_METHOD_ZONE_CONTROL	0x4
>>  #define WMAX_METHOD_HDMI_CABLE		0x5
>>  #define WMAX_METHOD_AMPLIFIER_CABLE	0x6
>> +#define WMAX_METHOD_DEEP_SLEEP_CONTROL	0x0B
>> +#define WMAX_METHOD_DEEP_SLEEP_STATUS	0x0C
>>  
>>  MODULE_AUTHOR("Mario Limonciello <mario_limonciello@dell.com>");
>>  MODULE_DESCRIPTION("Alienware special feature control");
>> @@ -62,6 +64,7 @@ struct quirk_entry {
>>  	u8 num_zones;
>>  	u8 hdmi_mux;
>>  	u8 amplifier;
>> +	u8 deepslp;
>>  };
>>  
>>  static struct quirk_entry *quirks;
>> @@ -70,24 +73,28 @@ static struct quirk_entry quirk_unknown = {
>>  	.num_zones = 2,
>>  	.hdmi_mux = 0,
>>  	.amplifier = 0,
>> +	.deepslp = 0,
>>  };
>>  
>>  static struct quirk_entry quirk_x51_r1_r2 = {
>>  	.num_zones = 3,
>>  	.hdmi_mux = 0,
>>  	.amplifier = 0,
>> +	.deepslp = 0,
>>  };
>>  
>>  static struct quirk_entry quirk_x51_r3 = {
>>  	.num_zones = 4,
>>  	.hdmi_mux = 0,
>>  	.amplifier = 1,
>> +	.deepslp = 0,
>>  };
>>  
>>  static struct quirk_entry quirk_asm100 = {
>>  	.num_zones = 2,
>>  	.hdmi_mux = 1,
>>  	.amplifier = 0,
>> +	.deepslp = 0,
>>  };
>>  
>>  static int __init dmi_matched(const struct dmi_system_id *dmi)
>> @@ -650,6 +657,92 @@ error_create_amplifier:
>>  	return ret;
>>  }
>>  
>> +/* Deep Sleep Control support
>> + * - Modifies BIOS setting for deep sleep control allowing extra wakeup events
>> +*/
>> +static ssize_t show_deepsleep_status(struct device *dev,
>> +				     struct device_attribute *attr, char *buf)
>> +{
>> +	acpi_status status;
>> +	u32 out_data;
>> +	struct wmax_basic_args in_args = {
>> +		.arg = 0,
>> +	};
>> +	status =
>> +	    alienware_wmax_command(&in_args, WMAX_METHOD_DEEP_SLEEP_STATUS,
>> +				   (u32 *) &out_data);
>> +	if (ACPI_SUCCESS(status)) {
>> +		if (out_data == 0)
>> +			return scnprintf(buf, PAGE_SIZE,
>> +					 "[disabled] s5 s5_s4\n");
>> +		else if (out_data == 1)
>> +			return scnprintf(buf, PAGE_SIZE,
>> +					 "disabled [s5] s5_s4\n");
>> +		else if (out_data == 2)
>> +			return scnprintf(buf, PAGE_SIZE,
>> +					 "disabled s5 [s5_s4]\n");
>> +	}
>> +	pr_err("alienware-wmi: unknown deep sleep status: %d\n", status);
>> +	return scnprintf(buf, PAGE_SIZE, "disabled s5 s5_s4 [unknown]\n");
>> +}
>> +
>> +static ssize_t toggle_deepsleep(struct device *dev,
>> +				struct device_attribute *attr,
>> +				const char *buf, size_t count)
>> +{
>> +	acpi_status status;
>> +	struct wmax_basic_args args;
>> +
>> +	if (strcmp(buf, "disabled\n") == 0)
>> +		args.arg = 0;
>> +	else if (strcmp(buf, "s5\n") == 0)
>> +		args.arg = 1;
>> +	else
>> +		args.arg = 2;
>> +	pr_debug("alienware-wmi: setting deep sleep to %d : %s", args.arg, buf);
>> +
>> +	status =
>> +	    alienware_wmax_command(&args, WMAX_METHOD_DEEP_SLEEP_CONTROL, NULL);
> Prefer to wrap the arguments:
>
> 	status = alienware_wmax_command(&args, WMAX_METHOD_DEEP_SLEEP_CONTROL,
> 					NULL);

OK.

>> +
>> +	if (ACPI_FAILURE(status))
>> +		pr_err
>> +		    ("alienware-wmi: deep sleep control failed: results: %u\n",
> Here, I don't consider the above to be "breakable" in terms of line wrapping.
>
> 		pr_err("alienware-wmi: deep sleep control failed: results: %u\n",
>
> Please.

OK.

>> +		     status);
>> +	return count;
>> +}
>> +
>> +static DEVICE_ATTR(deepsleep, S_IRUGO | S_IWUSR, show_deepsleep_status, toggle_deepsleep);
>> +
>> +static struct attribute *deepsleep_attrs[] = {
>> +	&dev_attr_deepsleep.attr,
>> +	NULL,
>> +};
>> +
>> +static struct attribute_group deepsleep_attribute_group = {
>> +	.name = "deepsleep",
>> +	.attrs = deepsleep_attrs,
>> +};
>> +
>> +static void remove_deepsleep(struct platform_device *dev)
>> +{
>> +	if (quirks->deepslp > 0)
>> +		sysfs_remove_group(&dev->dev.kobj, &deepsleep_attribute_group);
>> +}
>> +
>> +static int create_deepsleep(struct platform_device *dev)
>> +{
>> +	int ret;
>> +
>> +	ret = sysfs_create_group(&dev->dev.kobj, &deepsleep_attribute_group);
>> +	if (ret)
>> +		goto error_create_deepsleep;
>> +	return 0;
>> +
>> +error_create_deepsleep:
>> +	remove_deepsleep(dev);
>> +	return ret;
> As i the previous patch, this goto label is unnecessary.

OK.

>> +}
>> +
>>  static int __init alienware_wmi_init(void)
>>  {
>>  	int ret;
>> @@ -691,6 +784,12 @@ static int __init alienware_wmi_init(void)
>>  			goto fail_prep_amplifier;
>>  	}
>>  
>> +	if (quirks->deepslp > 0) {
>> +		ret = create_deepsleep(platform_device);
>> +		if (ret)
>> +			goto fail_prep_deepsleep;
>> +	}
>> +
>>  	ret = alienware_zone_init(platform_device);
>>  	if (ret)
>>  		goto fail_prep_zones;
>> @@ -699,6 +798,7 @@ static int __init alienware_wmi_init(void)
>>  
>>  fail_prep_zones:
>>  	alienware_zone_exit(platform_device);
>> +fail_prep_deepsleep:
>>  fail_prep_amplifier:
>>  fail_prep_hdmi:
>>  	platform_device_del(platform_device);
>> -- 
>> 1.9.1
>>
>>

Re: [PATCH v2 4/5] Add support for deep sleep control.

[Darren Hart]

On Tue, Feb 02, 2016 at 12:09:22PM -0600, Mario Limonciello wrote:
> 
> 
> On 02/02/2016 11:32 AM, Darren Hart wrote:
> > On Mon, Feb 01, 2016 at 08:28:50PM -0600, Mario Limonciello wrote:
> >> This allows configuration the system for wakeup with a controller.
> > Hrm, I'm happy to clean up English grammar in commit messages... but I'm
> > struggling with the intent of the above... Is this correct:
> >
> > Allow for user configuration, via sysfs, for wakeup with a controller.
> >
> > If so, great - but also, what do we mean by "with a controller" ?
> 
> I see how this is confusing to someone who isn't in the know what's
> going on.
> A better description is:
> 
> Allow for user configuration of BIOS settings that allow the system to
> turned on via HID devices.
> 
> If that's concise enough I'll update in next submission.

Yes, that's better. I think meant "... to be turned on ...". Otherwise, good.

Thanks,

-- 
Darren Hart
Intel Open Source Technology Center

[PATCH v2 5/5] Add support for two new systems: ASM200 and ASM201.

[Mario Limonciello]

Both of these systems support:
* 2 lighting control zones
* HDMI mux control
* deep sleep control (to enable wakup from controller)

The ASM201 also supports the external graphics amplifier.

Signed-off-by: Mario Limonciello <mario_limonciello@dell.com>
---
 drivers/platform/x86/alienware-wmi.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/platform/x86/alienware-wmi.c b/drivers/platform/x86/alienware-wmi.c
index f5ebcae..577fefa 100644
--- a/drivers/platform/x86/alienware-wmi.c
+++ b/drivers/platform/x86/alienware-wmi.c
@@ -97,6 +97,20 @@ static struct quirk_entry quirk_asm100 = {
 	.deepslp = 0,
 };
 
+static struct quirk_entry quirk_asm200 = {
+	.num_zones = 2,
+	.hdmi_mux = 1,
+	.amplifier = 0,
+	.deepslp = 1,
+};
+
+static struct quirk_entry quirk_asm201 = {
+	.num_zones = 2,
+	.hdmi_mux = 1,
+	.amplifier = 1,
+	.deepslp = 1,
+};
+
 static int __init dmi_matched(const struct dmi_system_id *dmi)
 {
 	quirks = dmi->driver_data;
@@ -140,6 +154,24 @@ static const struct dmi_system_id alienware_quirks[] __initconst = {
 		     },
 	 .driver_data = &quirk_asm100,
 	 },
+	{
+	 .callback = dmi_matched,
+	 .ident = "Alienware ASM200",
+	 .matches = {
+		     DMI_MATCH(DMI_SYS_VENDOR, "Alienware"),
+		     DMI_MATCH(DMI_PRODUCT_NAME, "ASM200"),
+		     },
+	 .driver_data = &quirk_asm200,
+	 },
+	{
+	 .callback = dmi_matched,
+	 .ident = "Alienware ASM201",
+	 .matches = {
+		     DMI_MATCH(DMI_SYS_VENDOR, "Alienware"),
+		     DMI_MATCH(DMI_PRODUCT_NAME, "ASM201"),
+		     },
+	 .driver_data = &quirk_asm201,
+	 },
 	{}
 };
 
-- 
1.9.1

Re: [PATCH v2 5/5] Add support for two new systems: ASM200 and ASM201.

[Darren Hart]

On Mon, Feb 01, 2016 at 08:28:51PM -0600, Mario Limonciello wrote:
> Both of these systems support:
> * 2 lighting control zones
> * HDMI mux control
> * deep sleep control (to enable wakup from controller)
> 
> The ASM201 also supports the external graphics amplifier.

Nit on the subject:

[file|platform]: subject

e.g.

alienware-wmi: Add support for two new systems: ASM200 and ASM201

This keeps the log consistent and aligns with Linus' merge comments.

> 
> Signed-off-by: Mario Limonciello <mario_limonciello@dell.com>
> ---
>  drivers/platform/x86/alienware-wmi.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/platform/x86/alienware-wmi.c b/drivers/platform/x86/alienware-wmi.c
> index f5ebcae..577fefa 100644
> --- a/drivers/platform/x86/alienware-wmi.c
> +++ b/drivers/platform/x86/alienware-wmi.c
> @@ -97,6 +97,20 @@ static struct quirk_entry quirk_asm100 = {
>  	.deepslp = 0,
>  };
>  
> +static struct quirk_entry quirk_asm200 = {
> +	.num_zones = 2,
> +	.hdmi_mux = 1,
> +	.amplifier = 0,
> +	.deepslp = 1,
> +};
> +
> +static struct quirk_entry quirk_asm201 = {
> +	.num_zones = 2,
> +	.hdmi_mux = 1,
> +	.amplifier = 1,
> +	.deepslp = 1,
> +};
> +
>  static int __init dmi_matched(const struct dmi_system_id *dmi)
>  {
>  	quirks = dmi->driver_data;
> @@ -140,6 +154,24 @@ static const struct dmi_system_id alienware_quirks[] __initconst = {
>  		     },
>  	 .driver_data = &quirk_asm100,
>  	 },
> +	{
> +	 .callback = dmi_matched,
> +	 .ident = "Alienware ASM200",
> +	 .matches = {
> +		     DMI_MATCH(DMI_SYS_VENDOR, "Alienware"),
> +		     DMI_MATCH(DMI_PRODUCT_NAME, "ASM200"),
> +		     },
> +	 .driver_data = &quirk_asm200,
> +	 },
> +	{
> +	 .callback = dmi_matched,
> +	 .ident = "Alienware ASM201",
> +	 .matches = {
> +		     DMI_MATCH(DMI_SYS_VENDOR, "Alienware"),
> +		     DMI_MATCH(DMI_PRODUCT_NAME, "ASM201"),
> +		     },
> +	 .driver_data = &quirk_asm201,
> +	 },
>  	{}
>  };
>  
> -- 
> 1.9.1
> 
> 

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v2 0/5] alienware-wmi new platform and feature support

[Darren Hart]

On Mon, Feb 01, 2016 at 08:28:46PM -0600, Mario Limonciello wrote:
> I've got some extensions for the alienware-wmi driver that have
> been introduced for a few new platforms and can be controlled via the WMI
> interface.
> 
> Changes from V1:
>  - Make sure whitespace changes are in their own patch
>  - Add commit descriptions to all patches
>  - Allow one line to go over 80 characters in deep sleep control patch
>    Other patches may have some areas that avoid going over 80 characters
>    still, but this matches the style of the existing driver.  If you would
>    like these to go over 80 characters as well, please comment which areas
>    this is OK.

This made it much easier to review. Thank you Mario. A few changes requested,
and a couple questions for you regarding intent/best-practice.

Thanks!

-- 
Darren Hart
Intel Open Source Technology Center