[PATCH] dell: add new dell WMI format for the AIO machines

[PATCH] dell: add new dell WMI format for the AIO machines

[AceLan Kao]

There is a new DELL WMI spec. with new WMI event format.
I'm working on the AIO machines, but I think the new format will apply to
all the Dell's machines, not only for AIO, which will be released later
this year.

The new format of the WMI buffer is shown as below
word 0 - the number of words following in the WMI buffer(not including
        this word.
word 1 - the event type
	0x0000 - A hot key is pressed or an event occurred
	0x000F - A sequence of hot keys are pressed
word 2 and on - the event data

Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
---
 drivers/platform/x86/dell-wmi-aio.c | 50 +++++++++++++++++++++++++++++++++----
 1 file changed, 45 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/dell-wmi-aio.c b/drivers/platform/x86/dell-wmi-aio.c
index 3f94545..d77fa86 100644
--- a/drivers/platform/x86/dell-wmi-aio.c
+++ b/drivers/platform/x86/dell-wmi-aio.c
@@ -34,6 +34,14 @@ MODULE_LICENSE("GPL");
 #define EVENT_GUID1 "284A0E6B-380E-472A-921F-E52786257FB4"
 #define EVENT_GUID2 "02314822-307C-4F66-BF0E-48AEAEB26CC8"
 
+struct dell_wmi_event {
+	u16	length;
+	/* 0x000: A hot key pressed or an event occurred
+	 * 0x00F: A sequence of hot keys are pressed */
+	u16	type;
+	u16	event[];
+};
+
 static const char *dell_wmi_aio_guids[] = {
 	EVENT_GUID1,
 	EVENT_GUID2,
@@ -46,11 +54,35 @@ MODULE_ALIAS("wmi:"EVENT_GUID2);
 static const struct key_entry dell_wmi_aio_keymap[] = {
 	{ KE_KEY, 0xc0, { KEY_VOLUMEUP } },
 	{ KE_KEY, 0xc1, { KEY_VOLUMEDOWN } },
+	{ KE_KEY, 0xe030, { KEY_VOLUMEUP } },
+	{ KE_KEY, 0xe02e, { KEY_VOLUMEDOWN } },
+	{ KE_KEY, 0xe020, { KEY_MUTE } },
+	{ KE_KEY, 0xe027, { KEY_DISPLAYTOGGLE } },
+	{ KE_KEY, 0xe006, { KEY_BRIGHTNESSUP } },
+	{ KE_KEY, 0xe005, { KEY_BRIGHTNESSDOWN } },
+	{ KE_KEY, 0xe00b, { KEY_SWITCHVIDEOMODE } },
 	{ KE_END, 0 }
 };
 
 static struct input_dev *dell_wmi_aio_input_dev;
 
+/*
+ * The new WMI event data format will follow the dell_wmi_event structure
+ * So, we will check if the buffer matches the format
+ */
+static bool dell_wmi_aio_event_check(u8 *buffer)
+{
+	dell_wmi_event *event = (dell_wmi_event *)buffer;
+
+	/* longest length is 6 right now */
+	if (event->length > 10)
+		return false;
+	if (event->type != 0 && event->type != 0xf)
+		return false;
+
+	return true;
+}
+
 static void dell_wmi_aio_notify(u32 value, void *context)
 {
 	struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
@@ -65,7 +97,7 @@ static void dell_wmi_aio_notify(u32 value, void *context)
 
 	obj = (union acpi_object *)response.pointer;
 	if (obj) {
-		unsigned int scancode;
+		unsigned int scancode = 0;
 
 		switch (obj->type) {
 		case ACPI_TYPE_INTEGER:
@@ -75,13 +107,21 @@ static void dell_wmi_aio_notify(u32 value, void *context)
 				scancode, 1, true);
 			break;
 		case ACPI_TYPE_BUFFER:
-			/* Broken machines return the scancode in a buffer */
-			if (obj->buffer.pointer && obj->buffer.length > 0) {
-				scancode = obj->buffer.pointer[0];
+			if (dell_wmi_aio_event_check(obj->buffer.pointer)) {
+				dell_wmi_event *event =
+					(dell_wmi_event *)obj->buffer.pointer;
+				scancode = event->event[0];
+			} else {
+				/* Broken machines return the scancode in a
+				   buffer */
+				if (obj->buffer.pointer &&
+						obj->buffer.length > 0)
+					scancode = obj->buffer.pointer[0];
+			}
+			if (scancode)
 				sparse_keymap_report_event(
 					dell_wmi_aio_input_dev,
 					scancode, 1, true);
-			}
 			break;
 		}
 	}
-- 
1.8.1.2

Re: [PATCH] dell: add new dell WMI format for the AIO machines

[Colin Ian King]

On 06/03/13 07:51, AceLan Kao wrote:
> There is a new DELL WMI spec. with new WMI event format.
> I'm working on the AIO machines, but I think the new format will apply to
> all the Dell's machines, not only for AIO, which will be released later
> this year.
>
> The new format of the WMI buffer is shown as below
> word 0 - the number of words following in the WMI buffer(not including
>          this word.
> word 1 - the event type
> 	0x0000 - A hot key is pressed or an event occurred
> 	0x000F - A sequence of hot keys are pressed
> word 2 and on - the event data
>
> Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
> ---
>   drivers/platform/x86/dell-wmi-aio.c | 50 +++++++++++++++++++++++++++++++++----
>   1 file changed, 45 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/platform/x86/dell-wmi-aio.c b/drivers/platform/x86/dell-wmi-aio.c
> index 3f94545..d77fa86 100644
> --- a/drivers/platform/x86/dell-wmi-aio.c
> +++ b/drivers/platform/x86/dell-wmi-aio.c
> @@ -34,6 +34,14 @@ MODULE_LICENSE("GPL");
>   #define EVENT_GUID1 "284A0E6B-380E-472A-921F-E52786257FB4"
>   #define EVENT_GUID2 "02314822-307C-4F66-BF0E-48AEAEB26CC8"
>
> +struct dell_wmi_event {
> +	u16	length;
> +	/* 0x000: A hot key pressed or an event occurred
> +	 * 0x00F: A sequence of hot keys are pressed */
> +	u16	type;
> +	u16	event[];
> +};
> +
>   static const char *dell_wmi_aio_guids[] = {
>   	EVENT_GUID1,
>   	EVENT_GUID2,
> @@ -46,11 +54,35 @@ MODULE_ALIAS("wmi:"EVENT_GUID2);
>   static const struct key_entry dell_wmi_aio_keymap[] = {
>   	{ KE_KEY, 0xc0, { KEY_VOLUMEUP } },
>   	{ KE_KEY, 0xc1, { KEY_VOLUMEDOWN } },
> +	{ KE_KEY, 0xe030, { KEY_VOLUMEUP } },
> +	{ KE_KEY, 0xe02e, { KEY_VOLUMEDOWN } },
> +	{ KE_KEY, 0xe020, { KEY_MUTE } },
> +	{ KE_KEY, 0xe027, { KEY_DISPLAYTOGGLE } },
> +	{ KE_KEY, 0xe006, { KEY_BRIGHTNESSUP } },
> +	{ KE_KEY, 0xe005, { KEY_BRIGHTNESSDOWN } },
> +	{ KE_KEY, 0xe00b, { KEY_SWITCHVIDEOMODE } },
>   	{ KE_END, 0 }
>   };
>
>   static struct input_dev *dell_wmi_aio_input_dev;
>
> +/*
> + * The new WMI event data format will follow the dell_wmi_event structure
> + * So, we will check if the buffer matches the format
> + */
> +static bool dell_wmi_aio_event_check(u8 *buffer)
> +{
> +	dell_wmi_event *event = (dell_wmi_event *)buffer;
> +
> +	/* longest length is 6 right now */
> +	if (event->length > 10)
> +		return false;
> +	if (event->type != 0 && event->type != 0xf)
> +		return false;

This does assume that the buffer pointer is not null and long enough to 
access the event structure fields,  which may not be so in future 
firmware.  So, please check that the pointer is valid and the buffer is 
long enough.

> +
> +	return true;
> +}
> +
>   static void dell_wmi_aio_notify(u32 value, void *context)
>   {
>   	struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
> @@ -65,7 +97,7 @@ static void dell_wmi_aio_notify(u32 value, void *context)
>
>   	obj = (union acpi_object *)response.pointer;
>   	if (obj) {
> -		unsigned int scancode;
> +		unsigned int scancode = 0;
>
>   		switch (obj->type) {
>   		case ACPI_TYPE_INTEGER:
> @@ -75,13 +107,21 @@ static void dell_wmi_aio_notify(u32 value, void *context)
>   				scancode, 1, true);
>   			break;
>   		case ACPI_TYPE_BUFFER:
> -			/* Broken machines return the scancode in a buffer */
> -			if (obj->buffer.pointer && obj->buffer.length > 0) {
> -				scancode = obj->buffer.pointer[0];
> +			if (dell_wmi_aio_event_check(obj->buffer.pointer)) {
> +				dell_wmi_event *event =
> +					(dell_wmi_event *)obj->buffer.pointer;
> +				scancode = event->event[0];
> +			} else {
> +				/* Broken machines return the scancode in a
> +				   buffer */
> +				if (obj->buffer.pointer &&
> +						obj->buffer.length > 0)
> +					scancode = obj->buffer.pointer[0];
> +			}
> +			if (scancode)
>   				sparse_keymap_report_event(
>   					dell_wmi_aio_input_dev,
>   					scancode, 1, true);
> -			}
>   			break;
>   		}
>   	}
>

[PATCH] dell: add new dell WMI format for the AIO machines

[AceLan Kao]

There is a new DELL WMI spec. with new WMI event format.
I'm working on the AIO machines, but I think the new format will apply to
all the Dell's machines, not only for AIO, which will be released later
this year.

The new format of the WMI buffer is shown as below
word 0 - the number of words following in the WMI buffer(not including
        this word.
word 1 - the event type
	0x0000 - A hot key is pressed or an event occurred
	0x000F - A sequence of hot keys are pressed
word 2 and on - the event data

Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
---
 drivers/platform/x86/dell-wmi-aio.c | 51 +++++++++++++++++++++++++++++++++----
 1 file changed, 46 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/dell-wmi-aio.c b/drivers/platform/x86/dell-wmi-aio.c
index 3f94545..bf8fc53 100644
--- a/drivers/platform/x86/dell-wmi-aio.c
+++ b/drivers/platform/x86/dell-wmi-aio.c
@@ -34,6 +34,14 @@ MODULE_LICENSE("GPL");
 #define EVENT_GUID1 "284A0E6B-380E-472A-921F-E52786257FB4"
 #define EVENT_GUID2 "02314822-307C-4F66-BF0E-48AEAEB26CC8"
 
+struct dell_wmi_event {
+	u16	length;
+	/* 0x000: A hot key pressed or an event occurred
+	 * 0x00F: A sequence of hot keys are pressed */
+	u16	type;
+	u16	event[];
+};
+
 static const char *dell_wmi_aio_guids[] = {
 	EVENT_GUID1,
 	EVENT_GUID2,
@@ -46,11 +54,36 @@ MODULE_ALIAS("wmi:"EVENT_GUID2);
 static const struct key_entry dell_wmi_aio_keymap[] = {
 	{ KE_KEY, 0xc0, { KEY_VOLUMEUP } },
 	{ KE_KEY, 0xc1, { KEY_VOLUMEDOWN } },
+	{ KE_KEY, 0xe030, { KEY_VOLUMEUP } },
+	{ KE_KEY, 0xe02e, { KEY_VOLUMEDOWN } },
+	{ KE_KEY, 0xe020, { KEY_MUTE } },
+	{ KE_KEY, 0xe027, { KEY_DISPLAYTOGGLE } },
+	{ KE_KEY, 0xe006, { KEY_BRIGHTNESSUP } },
+	{ KE_KEY, 0xe005, { KEY_BRIGHTNESSDOWN } },
+	{ KE_KEY, 0xe00b, { KEY_SWITCHVIDEOMODE } },
 	{ KE_END, 0 }
 };
 
 static struct input_dev *dell_wmi_aio_input_dev;
 
+/*
+ * The new WMI event data format will follow the dell_wmi_event structure
+ * So, we will check if the buffer matches the format
+ */
+static bool dell_wmi_aio_event_check(u8 *buffer)
+{
+	dell_wmi_event *event = (dell_wmi_event *)buffer;
+
+	if (event == NULL)
+		return false;
+
+	if ((event->type == 0 || event->type == 0xf) &&
+			event->length >= 2)
+		return true;
+
+	return false;
+}
+
 static void dell_wmi_aio_notify(u32 value, void *context)
 {
 	struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
@@ -65,7 +98,7 @@ static void dell_wmi_aio_notify(u32 value, void *context)
 
 	obj = (union acpi_object *)response.pointer;
 	if (obj) {
-		unsigned int scancode;
+		unsigned int scancode = 0;
 
 		switch (obj->type) {
 		case ACPI_TYPE_INTEGER:
@@ -75,13 +108,21 @@ static void dell_wmi_aio_notify(u32 value, void *context)
 				scancode, 1, true);
 			break;
 		case ACPI_TYPE_BUFFER:
-			/* Broken machines return the scancode in a buffer */
-			if (obj->buffer.pointer && obj->buffer.length > 0) {
-				scancode = obj->buffer.pointer[0];
+			if (dell_wmi_aio_event_check(obj->buffer.pointer)) {
+				dell_wmi_event *event =
+					(dell_wmi_event *)obj->buffer.pointer;
+				scancode = event->event[0];
+			} else {
+				/* Broken machines return the scancode in a
+				   buffer */
+				if (obj->buffer.pointer &&
+						obj->buffer.length > 0)
+					scancode = obj->buffer.pointer[0];
+			}
+			if (scancode)
 				sparse_keymap_report_event(
 					dell_wmi_aio_input_dev,
 					scancode, 1, true);
-			}
 			break;
 		}
 	}
-- 
1.8.1.2

Re: [PATCH] dell: add new dell WMI format for the AIO machines

[Colin Ian King]

On 07/03/13 02:48, AceLan Kao wrote:
> There is a new DELL WMI spec. with new WMI event format.
> I'm working on the AIO machines, but I think the new format will apply to
> all the Dell's machines, not only for AIO, which will be released later
> this year.
>
> The new format of the WMI buffer is shown as below
> word 0 - the number of words following in the WMI buffer(not including
>          this word.
> word 1 - the event type
> 	0x0000 - A hot key is pressed or an event occurred
> 	0x000F - A sequence of hot keys are pressed
> word 2 and on - the event data
>
> Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
> ---
>   drivers/platform/x86/dell-wmi-aio.c | 51 +++++++++++++++++++++++++++++++++----
>   1 file changed, 46 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/platform/x86/dell-wmi-aio.c b/drivers/platform/x86/dell-wmi-aio.c
> index 3f94545..bf8fc53 100644
> --- a/drivers/platform/x86/dell-wmi-aio.c
> +++ b/drivers/platform/x86/dell-wmi-aio.c
> @@ -34,6 +34,14 @@ MODULE_LICENSE("GPL");
>   #define EVENT_GUID1 "284A0E6B-380E-472A-921F-E52786257FB4"
>   #define EVENT_GUID2 "02314822-307C-4F66-BF0E-48AEAEB26CC8"
>
> +struct dell_wmi_event {
> +	u16	length;
> +	/* 0x000: A hot key pressed or an event occurred
> +	 * 0x00F: A sequence of hot keys are pressed */
> +	u16	type;
> +	u16	event[];
> +};
> +
>   static const char *dell_wmi_aio_guids[] = {
>   	EVENT_GUID1,
>   	EVENT_GUID2,
> @@ -46,11 +54,36 @@ MODULE_ALIAS("wmi:"EVENT_GUID2);
>   static const struct key_entry dell_wmi_aio_keymap[] = {
>   	{ KE_KEY, 0xc0, { KEY_VOLUMEUP } },
>   	{ KE_KEY, 0xc1, { KEY_VOLUMEDOWN } },
> +	{ KE_KEY, 0xe030, { KEY_VOLUMEUP } },
> +	{ KE_KEY, 0xe02e, { KEY_VOLUMEDOWN } },
> +	{ KE_KEY, 0xe020, { KEY_MUTE } },
> +	{ KE_KEY, 0xe027, { KEY_DISPLAYTOGGLE } },
> +	{ KE_KEY, 0xe006, { KEY_BRIGHTNESSUP } },
> +	{ KE_KEY, 0xe005, { KEY_BRIGHTNESSDOWN } },
> +	{ KE_KEY, 0xe00b, { KEY_SWITCHVIDEOMODE } },
>   	{ KE_END, 0 }
>   };
>
>   static struct input_dev *dell_wmi_aio_input_dev;
>
> +/*
> + * The new WMI event data format will follow the dell_wmi_event structure
> + * So, we will check if the buffer matches the format
> + */
> +static bool dell_wmi_aio_event_check(u8 *buffer)
> +{
> +	dell_wmi_event *event = (dell_wmi_event *)buffer;
> +
> +	if (event == NULL)
> +		return false;

I did mention in my last message that we need to also check that the 
buffer length needs checking. Suppose it is just 1 byte long (which it 
may be in the original Dell AIO case or in the future) - access to 
fields in event will be outside this buffer.

So you need to ensure that obj->buffer.length is at least 6 to be able 
to reference event->length, event->type and event[0].

> +
> +	if ((event->type == 0 || event->type == 0xf) &&
> +			event->length >= 2)
> +		return true;
> +
> +	return false;
> +}
> +
>   static void dell_wmi_aio_notify(u32 value, void *context)
>   {
>   	struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
> @@ -65,7 +98,7 @@ static void dell_wmi_aio_notify(u32 value, void *context)
>
>   	obj = (union acpi_object *)response.pointer;
>   	if (obj) {
> -		unsigned int scancode;
> +		unsigned int scancode = 0;
>
>   		switch (obj->type) {
>   		case ACPI_TYPE_INTEGER:
> @@ -75,13 +108,21 @@ static void dell_wmi_aio_notify(u32 value, void *context)
>   				scancode, 1, true);
>   			break;
>   		case ACPI_TYPE_BUFFER:
> -			/* Broken machines return the scancode in a buffer */
> -			if (obj->buffer.pointer && obj->buffer.length > 0) {
> -				scancode = obj->buffer.pointer[0];
> +			if (dell_wmi_aio_event_check(obj->buffer.pointer)) {
> +				dell_wmi_event *event =
> +					(dell_wmi_event *)obj->buffer.pointer;
> +				scancode = event->event[0];
> +			} else {
> +				/* Broken machines return the scancode in a
> +				   buffer */
> +				if (obj->buffer.pointer &&
> +						obj->buffer.length > 0)
> +					scancode = obj->buffer.pointer[0];
> +			}
> +			if (scancode)
>   				sparse_keymap_report_event(
>   					dell_wmi_aio_input_dev,
>   					scancode, 1, true);
> -			}
>   			break;
>   		}
>   	}
>

[PATCH] dell: add new dell WMI format for the AIO machines

[AceLan Kao]

There is a new DELL WMI spec. with new WMI event format.
I'm working on the AIO machines, but I think the new format will apply to
all the Dell's machines, not only for AIO, which will be released later
this year.

The new format of the WMI buffer is shown as below
word 0 - the number of words following in the WMI buffer(not including
        this word.
word 1 - the event type
	0x0000 - A hot key is pressed or an event occurred
	0x000F - A sequence of hot keys are pressed
word 2 and on - the event data

Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
---
 drivers/platform/x86/dell-wmi-aio.c | 53 +++++++++++++++++++++++++++++++++----
 1 file changed, 48 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/dell-wmi-aio.c b/drivers/platform/x86/dell-wmi-aio.c
index 3f94545..bcf8cc6 100644
--- a/drivers/platform/x86/dell-wmi-aio.c
+++ b/drivers/platform/x86/dell-wmi-aio.c
@@ -34,6 +34,14 @@ MODULE_LICENSE("GPL");
 #define EVENT_GUID1 "284A0E6B-380E-472A-921F-E52786257FB4"
 #define EVENT_GUID2 "02314822-307C-4F66-BF0E-48AEAEB26CC8"
 
+struct dell_wmi_event {
+	u16	length;
+	/* 0x000: A hot key pressed or an event occurred
+	 * 0x00F: A sequence of hot keys are pressed */
+	u16	type;
+	u16	event[];
+};
+
 static const char *dell_wmi_aio_guids[] = {
 	EVENT_GUID1,
 	EVENT_GUID2,
@@ -46,15 +54,41 @@ MODULE_ALIAS("wmi:"EVENT_GUID2);
 static const struct key_entry dell_wmi_aio_keymap[] = {
 	{ KE_KEY, 0xc0, { KEY_VOLUMEUP } },
 	{ KE_KEY, 0xc1, { KEY_VOLUMEDOWN } },
+	{ KE_KEY, 0xe030, { KEY_VOLUMEUP } },
+	{ KE_KEY, 0xe02e, { KEY_VOLUMEDOWN } },
+	{ KE_KEY, 0xe020, { KEY_MUTE } },
+	{ KE_KEY, 0xe027, { KEY_DISPLAYTOGGLE } },
+	{ KE_KEY, 0xe006, { KEY_BRIGHTNESSUP } },
+	{ KE_KEY, 0xe005, { KEY_BRIGHTNESSDOWN } },
+	{ KE_KEY, 0xe00b, { KEY_SWITCHVIDEOMODE } },
 	{ KE_END, 0 }
 };
 
 static struct input_dev *dell_wmi_aio_input_dev;
 
+/*
+ * The new WMI event data format will follow the dell_wmi_event structure
+ * So, we will check if the buffer matches the format
+ */
+static bool dell_wmi_aio_event_check(u8 *buffer, int length)
+{
+	struct dell_wmi_event *event = (struct dell_wmi_event *)buffer;
+
+	if (event == NULL || length < 6)
+		return false;
+
+	if ((event->type == 0 || event->type == 0xf) &&
+			event->length >= 2)
+		return true;
+
+	return false;
+}
+
 static void dell_wmi_aio_notify(u32 value, void *context)
 {
 	struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
 	union acpi_object *obj;
+	struct dell_wmi_event *event;
 	acpi_status status;
 
 	status = wmi_get_event_data(value, &response);
@@ -65,7 +99,7 @@ static void dell_wmi_aio_notify(u32 value, void *context)
 
 	obj = (union acpi_object *)response.pointer;
 	if (obj) {
-		unsigned int scancode;
+		unsigned int scancode = 0;
 
 		switch (obj->type) {
 		case ACPI_TYPE_INTEGER:
@@ -75,13 +109,22 @@ static void dell_wmi_aio_notify(u32 value, void *context)
 				scancode, 1, true);
 			break;
 		case ACPI_TYPE_BUFFER:
-			/* Broken machines return the scancode in a buffer */
-			if (obj->buffer.pointer && obj->buffer.length > 0) {
-				scancode = obj->buffer.pointer[0];
+			if (dell_wmi_aio_event_check(obj->buffer.pointer,
+						obj->buffer.length)) {
+				event = (struct dell_wmi_event *)
+					obj->buffer.pointer;
+				scancode = event->event[0];
+			} else {
+				/* Broken machines return the scancode in a
+				   buffer */
+				if (obj->buffer.pointer &&
+						obj->buffer.length > 0)
+					scancode = obj->buffer.pointer[0];
+			}
+			if (scancode)
 				sparse_keymap_report_event(
 					dell_wmi_aio_input_dev,
 					scancode, 1, true);
-			}
 			break;
 		}
 	}
-- 
1.8.1.2

Re: [PATCH] dell: add new dell WMI format for the AIO machines

[Matthew Garrett]

On Fri, Mar 08, 2013 at 03:44:30PM +0800, AceLan Kao wrote:
>  static const struct key_entry dell_wmi_aio_keymap[] = {
>  	{ KE_KEY, 0xc0, { KEY_VOLUMEUP } },
>  	{ KE_KEY, 0xc1, { KEY_VOLUMEDOWN } },
> +	{ KE_KEY, 0xe030, { KEY_VOLUMEUP } },
> +	{ KE_KEY, 0xe02e, { KEY_VOLUMEDOWN } },
> +	{ KE_KEY, 0xe020, { KEY_MUTE } },
> +	{ KE_KEY, 0xe027, { KEY_DISPLAYTOGGLE } },
> +	{ KE_KEY, 0xe006, { KEY_BRIGHTNESSUP } },
> +	{ KE_KEY, 0xe005, { KEY_BRIGHTNESSDOWN } },
> +	{ KE_KEY, 0xe00b, { KEY_SWITCHVIDEOMODE } },

This is starting to look awfully like the keymap in dell-wmi.c. There's 
probably an argument for merging them at the source level, even if it 
ends up duplicated in both drivers.

> +			if (dell_wmi_aio_event_check(obj->buffer.pointer,
> +						obj->buffer.length)) {

Are we guaranteed that the old events will never look like this?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] dell: add new dell WMI format for the AIO machines

[AceLan Kao]

2013/3/8 Matthew Garrett <mjg59@srcf.ucam.org>:
> On Fri, Mar 08, 2013 at 03:44:30PM +0800, AceLan Kao wrote:
>>  static const struct key_entry dell_wmi_aio_keymap[] = {
>>       { KE_KEY, 0xc0, { KEY_VOLUMEUP } },
>>       { KE_KEY, 0xc1, { KEY_VOLUMEDOWN } },
>> +     { KE_KEY, 0xe030, { KEY_VOLUMEUP } },
>> +     { KE_KEY, 0xe02e, { KEY_VOLUMEDOWN } },
>> +     { KE_KEY, 0xe020, { KEY_MUTE } },
>> +     { KE_KEY, 0xe027, { KEY_DISPLAYTOGGLE } },
>> +     { KE_KEY, 0xe006, { KEY_BRIGHTNESSUP } },
>> +     { KE_KEY, 0xe005, { KEY_BRIGHTNESSDOWN } },
>> +     { KE_KEY, 0xe00b, { KEY_SWITCHVIDEOMODE } },
>
> This is starting to look awfully like the keymap in dell-wmi.c. There's
> probably an argument for merging them at the source level, even if it
> ends up duplicated in both drivers.
For All-In-One machines, there will be only some function keys on the
side of the machine,
so the list won't grow like the dell-wmi driver and probably stop
growing quickly.

>> +                     if (dell_wmi_aio_event_check(obj->buffer.pointer,
>> +                                             obj->buffer.length)) {
>
> Are we guaranteed that the old events will never look like this?
There is no easy way to distinguish between the old and the new WMI event,
so what we can do is to add more constraints on it.
With buffer length and the event type checking,
I think it's sufficient for identifying them.


-- 
Chia-Lin Kao(AceLan)
http://blog.acelan.idv.tw/
E-Mail: acelan.kaoATcanonical.com (s/AT/@/)

Re: [PATCH] dell: add new dell WMI format for the AIO machines

[Colin Ian King]

On 08/03/13 08:19, AceLan Kao wrote:
> 2013/3/8 Matthew Garrett <mjg59@srcf.ucam.org>:
>> On Fri, Mar 08, 2013 at 03:44:30PM +0800, AceLan Kao wrote:
>>>   static const struct key_entry dell_wmi_aio_keymap[] = {
>>>        { KE_KEY, 0xc0, { KEY_VOLUMEUP } },
>>>        { KE_KEY, 0xc1, { KEY_VOLUMEDOWN } },
>>> +     { KE_KEY, 0xe030, { KEY_VOLUMEUP } },
>>> +     { KE_KEY, 0xe02e, { KEY_VOLUMEDOWN } },
>>> +     { KE_KEY, 0xe020, { KEY_MUTE } },
>>> +     { KE_KEY, 0xe027, { KEY_DISPLAYTOGGLE } },
>>> +     { KE_KEY, 0xe006, { KEY_BRIGHTNESSUP } },
>>> +     { KE_KEY, 0xe005, { KEY_BRIGHTNESSDOWN } },
>>> +     { KE_KEY, 0xe00b, { KEY_SWITCHVIDEOMODE } },
>>
>> This is starting to look awfully like the keymap in dell-wmi.c. There's
>> probably an argument for merging them at the source level, even if it
>> ends up duplicated in both drivers.
> For All-In-One machines, there will be only some function keys on the
> side of the machine,
> so the list won't grow like the dell-wmi driver and probably stop
> growing quickly.
>
>>> +                     if (dell_wmi_aio_event_check(obj->buffer.pointer,
>>> +                                             obj->buffer.length)) {
>>
>> Are we guaranteed that the old events will never look like this?
> There is no easy way to distinguish between the old and the new WMI event,
> so what we can do is to add more constraints on it.
> With buffer length and the event type checking,
> I think it's sufficient for identifying them.
>
..I'm in agreement about this. The original set of machines that passed 
the event info in a buffer were a minimal batch and had the firmware 
written by a totally different BIOS vendor than the original set of AOI 
machines - and they were a limit run.  The checks you've now added are 
most probably sufficient until somebody decides to change the Dell AIO 
implementation again.  So I'm happy with this as it stands.

Colin