Re: [PATCH] acer-wmi: support Acer Aspire/eMachines 4739z

Re: [PATCH] acer-wmi: support Acer Aspire/eMachines 4739z

[joeyli]

於 三，2011-09-07 於 15:32 +0800，AceLan Kao 提到：
> There are 2 kind of 4739Z, the one that vendor name is ACER sents out
> the correct brightness key events, so we use the

Current brightness key events means the Fn key direct send out key code
but not from wmi notify event?

> quirk_acer_travelmate_2490. And another one with eMachines as it's
> vendor name follows Acer WMI spec. and doesn't sent out brightness key
> events, so I added a new quirk, quirk_acer_aspire_4739z, for it, and
> handle the key events in acer_wmi_notify().
> 
> BTW, we need other key event types that defined in Acer WMI spec., so
> I added them in acer_wmi_event_ids enumerate.
> 
> Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
> ---
>  drivers/platform/x86/acer-wmi.c |   69 ++++++++++++++++++++++++++++++++++++--
>  1 files changed, 65 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
> index df730c9..257c7f2 100644
> --- a/drivers/platform/x86/acer-wmi.c
> +++ b/drivers/platform/x86/acer-wmi.c
> @@ -94,6 +94,10 @@ MODULE_ALIAS("wmi:676AA15E-6A47-4D9F-A2CC-1E6D18D14026");
>  
>  enum acer_wmi_event_ids {
>  	WMID_HOTKEY_EVENT = 0x1,
> +	WMID_HOTKEY_BREAK_EVENT = 0x02,
> +	WMID_GRAPHIC_SWITCH_EVENT = 0x03,

Please change to 0x2 and 0x3 for consistency in this enum.

> +	WMID_BRIGHTNESS_CHANGE_EVENT = 0x4,
> +	WMID_SENSOR_EVENT = 0x5,
>  };
>  
>  static const struct key_entry acer_wmi_keymap[] = {
> @@ -112,9 +116,9 @@ static const struct key_entry acer_wmi_keymap[] = {
>  	{KE_IGNORE, 0x45, {KEY_STOP} },
>  	{KE_IGNORE, 0x48, {KEY_VOLUMEUP} },
>  	{KE_IGNORE, 0x49, {KEY_VOLUMEDOWN} },
> -	{KE_IGNORE, 0x61, {KEY_SWITCHVIDEOMODE} },
> -	{KE_IGNORE, 0x62, {KEY_BRIGHTNESSUP} },
> -	{KE_IGNORE, 0x63, {KEY_BRIGHTNESSDOWN} },


Why enable 0x61, 0x62 and 0x63? Does it for Aspire 4739z or eMachines
4739z?
Why choice 0x61, 0x62 and 0x63?
0x61, 0x62 and 0x63 are from your machine's wmi notify event result?

But I didn't see the result from your following code.

> +	{KE_KEY, 0x61, {KEY_SWITCHVIDEOMODE} },
> +	{KE_KEY, 0x62, {KEY_BRIGHTNESSUP} },
> +	{KE_KEY, 0x63, {KEY_BRIGHTNESSDOWN} },
>  	{KE_KEY, 0x64, {KEY_SWITCHVIDEOMODE} },	/* Display Switch */
>  	{KE_IGNORE, 0x81, {KEY_SLEEP} },
>  	{KE_KEY, 0x82, {KEY_TOUCHPAD_TOGGLE} },	/* Touch Pad On/Off */
> @@ -131,6 +135,15 @@ struct event_return_value {
>  	u32 reserved;
>  } __attribute__((packed));
>  
> +
> +struct brightness_event_return_value {
> +	u8 function;
> +	u8 brightness_level1;
> +	u8 brightness_level2;

Please don't use brightness_level"1" or level"2", it confused to me or
other code reviewer and nobody can remember the definition after a
couple of months.
Please try to give them more meaningful names.

> +	u8 reserved;
> +	u32 reserved2;
> +} __attribute__((packed));
> +
>  /*
>   * GUID3 Get Device Status device flags
>   */
> @@ -199,6 +212,7 @@ enum interface_flags {
>  #define ACER_DEFAULT_THREEG    0
>  
>  static int max_brightness = 0xF;
> +static int cur_brightness= 9;
>  

Why should you control the current brightness level but not control by
_BCL/_BCM in DSDT?

>  static int mailled = -1;
>  static int brightness = -1;
> @@ -310,6 +324,10 @@ static struct quirk_entry quirk_lenovo_ideapad_s205 = {
>  	.wireless = 3,
>  };
>  
> +static struct quirk_entry quirk_acer_aspire_4739z= {
> +	.brightness = 2,
> +};
> +
>  /* The Aspire One has a dummy ACPI-WMI interface - disable it */
>  static struct dmi_system_id __devinitdata acer_blacklist[] = {
>  	{
> @@ -368,6 +386,24 @@ static struct dmi_system_id acer_quirks[] = {
>  	},
>  	{
>  		.callback = dmi_matched,
> +		.ident = "Acer Aspire 4739Z",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "AS4739Z"),
> +		},
> +		.driver_data = &quirk_acer_travelmate_2490,
> +	},

Why "Acer Aspire 4739Z" set to quirk the same with travelmate 2490?
There only have a mailled capability flag in this quirk, why you used
it? 
Does that because Aspire 4739Z also have mail hotkey button? 

If the answer is NO, then don't need add a new quirk.

> +	{
> +		.callback = dmi_matched,
> +		.ident = "Acer eMachines 4739Z",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "eMachines"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "AS4739Z"),
> +		},
> +		.driver_data = &quirk_acer_aspire_4739z,
> +	},
> +	{
> +		.callback = dmi_matched,
>  		.ident = "Acer Aspire 5100",
>  		.matches = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
> @@ -1446,7 +1482,7 @@ static void acer_rfkill_exit(void)
>  static ssize_t show_bool_threeg(struct device *dev,
>  	struct device_attribute *attr, char *buf)
>  {
> -	u32 result; \
> +	u32 result;
>  	acpi_status status;
>  
>  	pr_info("This threeg sysfs will be removed in 2012"
> @@ -1501,6 +1537,7 @@ static void acer_wmi_notify(u32 value, void *context)
>  	struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
>  	union acpi_object *obj;
>  	struct event_return_value return_value;
> +	struct brightness_event_return_value brightness_return_value;
>  	acpi_status status;
>  	u16 device_state;
>  	const struct key_entry *key;
> @@ -1531,6 +1568,7 @@ static void acer_wmi_notify(u32 value, void *context)
>  
>  	switch (return_value.function) {
>  	case WMID_HOTKEY_EVENT:
> +	case WMID_HOTKEY_BREAK_EVENT:
>  		device_state = return_value.device_state;
>  		pr_debug("device state: 0x%x\n", device_state);
>  
> @@ -1558,6 +1596,29 @@ static void acer_wmi_notify(u32 value, void *context)
>  						   1, true);
>  		}
>  		break;
> +	case WMID_BRIGHTNESS_CHANGE_EVENT:
> +		// to avoid the brightness event be handled twice
> +		if( quirks->brightness != 2)
> +			break;
> +		brightness_return_value = *((struct brightness_event_return_value *)&return_value);
> +		/* brightness is decreasing */
> +		if( cur_brightness > brightness_return_value.brightness_level1)
> +			key = sparse_keymap_entry_from_scancode(acer_wmi_input_dev, 0x63);
> +		/* brightness is increasing */
> +		else if( cur_brightness < brightness_return_value.brightness_level1)
> +			key = sparse_keymap_entry_from_scancode(acer_wmi_input_dev, 0x62);
> +		/* brightness is already at the bottom */
> +		else if( cur_brightness == 0)
> +			key = sparse_keymap_entry_from_scancode(acer_wmi_input_dev, 0x63);
> +		/* brightness is already at the top */
> +		else if( cur_brightness == 9)
> +			key = sparse_keymap_entry_from_scancode(acer_wmi_input_dev, 0x62);
> +
> +		sparse_keymap_report_entry(acer_wmi_input_dev, key, 1, true);
> +		cur_brightness= return_value.key_num;
> +		break;

The same, 
why should you control the brightness level number in acer-wmi driver?
Does it not control by acpi video driver and _BCL/_BCM in DSDT? 

And, why choice 0x62, 0x63? The above 2 number is not from your wmi
notify result, why use them?

> +	case WMID_GRAPHIC_SWITCH_EVENT:
> +	case WMID_SENSOR_EVENT:
>  	default:
>  		pr_warn("Unknown function number - %d - %d\n",
>  			return_value.function, return_value.key_num);

Thank's a lot!
Joey Lee

Re: [PATCH] acer-wmi: support Acer Aspire/eMachines 4739z

[AceLan Kao]

Hi,

2011/9/7 joeyli <jlee@suse.com>:
> 於 三，2011-09-07 於 15:32 +0800，AceLan Kao 提到：
>> There are 2 kind of 4739Z, the one that vendor name is ACER sents out
>> the correct brightness key events, so we use the
>
> Current brightness key events means the Fn key direct send out key code
> but not from wmi notify event?
Currently, most of the machines send out the correctly brightness key
code from EC,
so we don't have to handle them. The video ACPI will capture the
events and adjust
the brightness.
Acer Aspire 4739Z sends out the correct brightness key code as other
machines do.
Acer eMachines 4739Z doesn't sends out correct key codes, it sends out
meaningless
key codes, so we have to capture the brightness event from WMI and remapping to
the correct key code and raise the correct key events to ACPI.

>
>> quirk_acer_travelmate_2490. And another one with eMachines as it's
>> vendor name follows Acer WMI spec. and doesn't sent out brightness key
>> events, so I added a new quirk, quirk_acer_aspire_4739z, for it, and
>> handle the key events in acer_wmi_notify().
>>
>> BTW, we need other key event types that defined in Acer WMI spec., so
>> I added them in acer_wmi_event_ids enumerate.
>>
>> Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
>> ---
>>  drivers/platform/x86/acer-wmi.c |   69 ++++++++++++++++++++++++++++++++++++--
>>  1 files changed, 65 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
>> index df730c9..257c7f2 100644
>> --- a/drivers/platform/x86/acer-wmi.c
>> +++ b/drivers/platform/x86/acer-wmi.c
>> @@ -94,6 +94,10 @@ MODULE_ALIAS("wmi:676AA15E-6A47-4D9F-A2CC-1E6D18D14026");
>>
>>  enum acer_wmi_event_ids {
>>       WMID_HOTKEY_EVENT = 0x1,
>> +     WMID_HOTKEY_BREAK_EVENT = 0x02,
>> +     WMID_GRAPHIC_SWITCH_EVENT = 0x03,
>
> Please change to 0x2 and 0x3 for consistency in this enum.
Okay, will do.

>
>> +     WMID_BRIGHTNESS_CHANGE_EVENT = 0x4,
>> +     WMID_SENSOR_EVENT = 0x5,
>>  };
>>
>>  static const struct key_entry acer_wmi_keymap[] = {
>> @@ -112,9 +116,9 @@ static const struct key_entry acer_wmi_keymap[] = {
>>       {KE_IGNORE, 0x45, {KEY_STOP} },
>>       {KE_IGNORE, 0x48, {KEY_VOLUMEUP} },
>>       {KE_IGNORE, 0x49, {KEY_VOLUMEDOWN} },
>> -     {KE_IGNORE, 0x61, {KEY_SWITCHVIDEOMODE} },
>> -     {KE_IGNORE, 0x62, {KEY_BRIGHTNESSUP} },
>> -     {KE_IGNORE, 0x63, {KEY_BRIGHTNESSDOWN} },
>> +     {KE_KEY, 0x61, {KEY_SWITCHVIDEOMODE} },
>> +     {KE_KEY, 0x62, {KEY_BRIGHTNESSUP} },
>> +     {KE_KEY, 0x63, {KEY_BRIGHTNESSDOWN} },
>
> Why enable 0x61, 0x62 and 0x63? Does it for Aspire 4739z or eMachines
> 4739z?
It's only for eMachines now and only be activated when quirk->brightness == 2.

> Why choice 0x61, 0x62 and 0x63?
> 0x61, 0x62 and 0x63 are from your machine's wmi notify event result?
They used to be those values, so I just enable them.
And indeed, Acer Aspire 4739Z reports those key codes, 0x62, 0x63, while
brightness key be pressed, so I think other machines might report the same key
codes, but they don't need to handle that.
acer-wmi doesn't handle brightness change event, so there will be an
error message
comes from acer_wmi_notify(), says "Unknown function number - 3 - xx", that's
why those value became ignored.

We can choose any value we like other than, 0x62, 0x63, since we don't care what
key codes are reported, I just need a mapping entry here, so that
I can use sparse_keymap_report_entry() later in acer_wmi_notify()
function to report
the correct brightness key events.

>
> But I didn't see the result from your following code.=
In acer_wmi_notify() function, I use the values, 0x62, 0x63, directly
to remap to the
correct brightness key code.

>
>>       {KE_KEY, 0x64, {KEY_SWITCHVIDEOMODE} }, /* Display Switch */
>>       {KE_IGNORE, 0x81, {KEY_SLEEP} },
>>       {KE_KEY, 0x82, {KEY_TOUCHPAD_TOGGLE} }, /* Touch Pad On/Off */
>> @@ -131,6 +135,15 @@ struct event_return_value {
>>       u32 reserved;
>>  } __attribute__((packed));
>>
>> +
>> +struct brightness_event_return_value {
>> +     u8 function;
>> +     u8 brightness_level1;
>> +     u8 brightness_level2;
>
> Please don't use brightness_level"1" or level"2", it confused to me or
> other code reviewer and nobody can remember the definition after a
> couple of months.
> Please try to give them more meaningful names.
Got it.

>
>> +     u8 reserved;
>> +     u32 reserved2;
>> +} __attribute__((packed));
>> +
>>  /*
>>   * GUID3 Get Device Status device flags
>>   */
>> @@ -199,6 +212,7 @@ enum interface_flags {
>>  #define ACER_DEFAULT_THREEG    0
>>
>>  static int max_brightness = 0xF;
>> +static int cur_brightness= 9;
>>
>
> Why should you control the current brightness level but not control by
> _BCL/_BCM in DSDT?
The value here is just recording the current brightness.
Because we don't know which key the user pressed, we just know the
brightness change event happened and what the new brightness value is,
so we should record the current brightness value and then compare to the
new value, so that we could know the brightness up or down key be pressed.

Although, in the WMI spec., the brightness value is defined from 0x0 ~ 0x64,
what I observed is 0 ~ 9, so I assume that the default brightness is 9, the max.
The value could be wrong for the first time adjusting the brightness, that's not
perfect, but acceptable I think.

>
>>  static int mailled = -1;
>>  static int brightness = -1;
>> @@ -310,6 +324,10 @@ static struct quirk_entry quirk_lenovo_ideapad_s205 = {
>>       .wireless = 3,
>>  };
>>
>> +static struct quirk_entry quirk_acer_aspire_4739z= {
>> +     .brightness = 2,
>> +};
>> +
>>  /* The Aspire One has a dummy ACPI-WMI interface - disable it */
>>  static struct dmi_system_id __devinitdata acer_blacklist[] = {
>>       {
>> @@ -368,6 +386,24 @@ static struct dmi_system_id acer_quirks[] = {
>>       },
>>       {
>>               .callback = dmi_matched,
>> +             .ident = "Acer Aspire 4739Z",
>> +             .matches = {
>> +                     DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
>> +                     DMI_MATCH(DMI_PRODUCT_NAME, "AS4739Z"),
>> +             },
>> +             .driver_data = &quirk_acer_travelmate_2490,
>> +     },
>
> Why "Acer Aspire 4739Z" set to quirk the same with travelmate 2490?
> There only have a mailled capability flag in this quirk, why you used
> it?
> Does that because Aspire 4739Z also have mail hotkey button?
>
> If the answer is NO, then don't need add a new quirk.
Sorry, I missunderstanding it, there is no mailled on it, I'll remove it.

>
>> +     {
>> +             .callback = dmi_matched,
>> +             .ident = "Acer eMachines 4739Z",
>> +             .matches = {
>> +                     DMI_MATCH(DMI_SYS_VENDOR, "eMachines"),
>> +                     DMI_MATCH(DMI_PRODUCT_NAME, "AS4739Z"),
>> +             },
>> +             .driver_data = &quirk_acer_aspire_4739z,
>> +     },
>> +     {
>> +             .callback = dmi_matched,
>>               .ident = "Acer Aspire 5100",
>>               .matches = {
>>                       DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
>> @@ -1446,7 +1482,7 @@ static void acer_rfkill_exit(void)
>>  static ssize_t show_bool_threeg(struct device *dev,
>>       struct device_attribute *attr, char *buf)
>>  {
>> -     u32 result; \
>> +     u32 result;
>>       acpi_status status;
>>
>>       pr_info("This threeg sysfs will be removed in 2012"
>> @@ -1501,6 +1537,7 @@ static void acer_wmi_notify(u32 value, void *context)
>>       struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
>>       union acpi_object *obj;
>>       struct event_return_value return_value;
>> +     struct brightness_event_return_value brightness_return_value;
>>       acpi_status status;
>>       u16 device_state;
>>       const struct key_entry *key;
>> @@ -1531,6 +1568,7 @@ static void acer_wmi_notify(u32 value, void *context)
>>
>>       switch (return_value.function) {
>>       case WMID_HOTKEY_EVENT:
>> +     case WMID_HOTKEY_BREAK_EVENT:
>>               device_state = return_value.device_state;
>>               pr_debug("device state: 0x%x\n", device_state);
>>
>> @@ -1558,6 +1596,29 @@ static void acer_wmi_notify(u32 value, void *context)
>>                                                  1, true);
>>               }
>>               break;
>> +     case WMID_BRIGHTNESS_CHANGE_EVENT:
>> +             // to avoid the brightness event be handled twice
>> +             if( quirks->brightness != 2)
>> +                     break;
>> +             brightness_return_value = *((struct brightness_event_return_value *)&return_value);
>> +             /* brightness is decreasing */
>> +             if( cur_brightness > brightness_return_value.brightness_level1)
>> +                     key = sparse_keymap_entry_from_scancode(acer_wmi_input_dev, 0x63);
>> +             /* brightness is increasing */
>> +             else if( cur_brightness < brightness_return_value.brightness_level1)
>> +                     key = sparse_keymap_entry_from_scancode(acer_wmi_input_dev, 0x62);
>> +             /* brightness is already at the bottom */
>> +             else if( cur_brightness == 0)
>> +                     key = sparse_keymap_entry_from_scancode(acer_wmi_input_dev, 0x63);
>> +             /* brightness is already at the top */
>> +             else if( cur_brightness == 9)
>> +                     key = sparse_keymap_entry_from_scancode(acer_wmi_input_dev, 0x62);
>> +
>> +             sparse_keymap_report_entry(acer_wmi_input_dev, key, 1, true);
>> +             cur_brightness= return_value.key_num;
>> +             break;
>
> The same,
> why should you control the brightness level number in acer-wmi driver?
> Does it not control by acpi video driver and _BCL/_BCM in DSDT?
>
> And, why choice 0x62, 0x63? The above 2 number is not from your wmi
> notify result, why use them?
The problem is that the BIOS/EC doesn't send out the correct brightness
key events, the BIOS engineer just choose non-used values for those keys
that are defined in Acer WMI spec. Because they think those key events
should be captured from WMI, not from BIOS/EC.
So that, we have to read the brightness change event from WMI key events.
And then use the ACPI way to adjust the brightness.

>
>> +     case WMID_GRAPHIC_SWITCH_EVENT:
>> +     case WMID_SENSOR_EVENT:
>>       default:
>>               pr_warn("Unknown function number - %d - %d\n",
>>                       return_value.function, return_value.key_num);
>
>
> Thank's a lot!
> Joey Lee
>
>

Best regards,
AceLan Kao.

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

Re: [PATCH] acer-wmi: support Acer Aspire/eMachines 4739z

[AceLan Kao]

[-- Attachment #1: Type: text/plain, Size: 11522 bytes --]

Hi,

The new patch is as attached.

Best regards,
AceLan Kao.

在 2011年9月8日上午10:30，AceLan Kao <acelan.kao@canonical.com> 寫道：
> Hi,
>
> 2011/9/7 joeyli <jlee@suse.com>:
>> 於 三，2011-09-07 於 15:32 +0800，AceLan Kao 提到：
>>> There are 2 kind of 4739Z, the one that vendor name is ACER sents out
>>> the correct brightness key events, so we use the
>>
>> Current brightness key events means the Fn key direct send out key code
>> but not from wmi notify event?
> Currently, most of the machines send out the correctly brightness key
> code from EC,
> so we don't have to handle them. The video ACPI will capture the
> events and adjust
> the brightness.
> Acer Aspire 4739Z sends out the correct brightness key code as other
> machines do.
> Acer eMachines 4739Z doesn't sends out correct key codes, it sends out
> meaningless
> key codes, so we have to capture the brightness event from WMI and remapping to
> the correct key code and raise the correct key events to ACPI.
>
>>
>>> quirk_acer_travelmate_2490. And another one with eMachines as it's
>>> vendor name follows Acer WMI spec. and doesn't sent out brightness key
>>> events, so I added a new quirk, quirk_acer_aspire_4739z, for it, and
>>> handle the key events in acer_wmi_notify().
>>>
>>> BTW, we need other key event types that defined in Acer WMI spec., so
>>> I added them in acer_wmi_event_ids enumerate.
>>>
>>> Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
>>> ---
>>>  drivers/platform/x86/acer-wmi.c |   69 ++++++++++++++++++++++++++++++++++++--
>>>  1 files changed, 65 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
>>> index df730c9..257c7f2 100644
>>> --- a/drivers/platform/x86/acer-wmi.c
>>> +++ b/drivers/platform/x86/acer-wmi.c
>>> @@ -94,6 +94,10 @@ MODULE_ALIAS("wmi:676AA15E-6A47-4D9F-A2CC-1E6D18D14026");
>>>
>>>  enum acer_wmi_event_ids {
>>>       WMID_HOTKEY_EVENT = 0x1,
>>> +     WMID_HOTKEY_BREAK_EVENT = 0x02,
>>> +     WMID_GRAPHIC_SWITCH_EVENT = 0x03,
>>
>> Please change to 0x2 and 0x3 for consistency in this enum.
> Okay, will do.
>
>>
>>> +     WMID_BRIGHTNESS_CHANGE_EVENT = 0x4,
>>> +     WMID_SENSOR_EVENT = 0x5,
>>>  };
>>>
>>>  static const struct key_entry acer_wmi_keymap[] = {
>>> @@ -112,9 +116,9 @@ static const struct key_entry acer_wmi_keymap[] = {
>>>       {KE_IGNORE, 0x45, {KEY_STOP} },
>>>       {KE_IGNORE, 0x48, {KEY_VOLUMEUP} },
>>>       {KE_IGNORE, 0x49, {KEY_VOLUMEDOWN} },
>>> -     {KE_IGNORE, 0x61, {KEY_SWITCHVIDEOMODE} },
>>> -     {KE_IGNORE, 0x62, {KEY_BRIGHTNESSUP} },
>>> -     {KE_IGNORE, 0x63, {KEY_BRIGHTNESSDOWN} },
>>> +     {KE_KEY, 0x61, {KEY_SWITCHVIDEOMODE} },
>>> +     {KE_KEY, 0x62, {KEY_BRIGHTNESSUP} },
>>> +     {KE_KEY, 0x63, {KEY_BRIGHTNESSDOWN} },
>>
>> Why enable 0x61, 0x62 and 0x63? Does it for Aspire 4739z or eMachines
>> 4739z?
> It's only for eMachines now and only be activated when quirk->brightness == 2.
>
>> Why choice 0x61, 0x62 and 0x63?
>> 0x61, 0x62 and 0x63 are from your machine's wmi notify event result?
> They used to be those values, so I just enable them.
> And indeed, Acer Aspire 4739Z reports those key codes, 0x62, 0x63, while
> brightness key be pressed, so I think other machines might report the same key
> codes, but they don't need to handle that.
> acer-wmi doesn't handle brightness change event, so there will be an
> error message
> comes from acer_wmi_notify(), says "Unknown function number - 3 - xx", that's
> why those value became ignored.
>
> We can choose any value we like other than, 0x62, 0x63, since we don't care what
> key codes are reported, I just need a mapping entry here, so that
> I can use sparse_keymap_report_entry() later in acer_wmi_notify()
> function to report
> the correct brightness key events.
>
>>
>> But I didn't see the result from your following code.=
> In acer_wmi_notify() function, I use the values, 0x62, 0x63, directly
> to remap to the
> correct brightness key code.
>
>>
>>>       {KE_KEY, 0x64, {KEY_SWITCHVIDEOMODE} }, /* Display Switch */
>>>       {KE_IGNORE, 0x81, {KEY_SLEEP} },
>>>       {KE_KEY, 0x82, {KEY_TOUCHPAD_TOGGLE} }, /* Touch Pad On/Off */
>>> @@ -131,6 +135,15 @@ struct event_return_value {
>>>       u32 reserved;
>>>  } __attribute__((packed));
>>>
>>> +
>>> +struct brightness_event_return_value {
>>> +     u8 function;
>>> +     u8 brightness_level1;
>>> +     u8 brightness_level2;
>>
>> Please don't use brightness_level"1" or level"2", it confused to me or
>> other code reviewer and nobody can remember the definition after a
>> couple of months.
>> Please try to give them more meaningful names.
> Got it.
>
>>
>>> +     u8 reserved;
>>> +     u32 reserved2;
>>> +} __attribute__((packed));
>>> +
>>>  /*
>>>   * GUID3 Get Device Status device flags
>>>   */
>>> @@ -199,6 +212,7 @@ enum interface_flags {
>>>  #define ACER_DEFAULT_THREEG    0
>>>
>>>  static int max_brightness = 0xF;
>>> +static int cur_brightness= 9;
>>>
>>
>> Why should you control the current brightness level but not control by
>> _BCL/_BCM in DSDT?
> The value here is just recording the current brightness.
> Because we don't know which key the user pressed, we just know the
> brightness change event happened and what the new brightness value is,
> so we should record the current brightness value and then compare to the
> new value, so that we could know the brightness up or down key be pressed.
>
> Although, in the WMI spec., the brightness value is defined from 0x0 ~ 0x64,
> what I observed is 0 ~ 9, so I assume that the default brightness is 9, the max.
> The value could be wrong for the first time adjusting the brightness, that's not
> perfect, but acceptable I think.
>
>>
>>>  static int mailled = -1;
>>>  static int brightness = -1;
>>> @@ -310,6 +324,10 @@ static struct quirk_entry quirk_lenovo_ideapad_s205 = {
>>>       .wireless = 3,
>>>  };
>>>
>>> +static struct quirk_entry quirk_acer_aspire_4739z= {
>>> +     .brightness = 2,
>>> +};
>>> +
>>>  /* The Aspire One has a dummy ACPI-WMI interface - disable it */
>>>  static struct dmi_system_id __devinitdata acer_blacklist[] = {
>>>       {
>>> @@ -368,6 +386,24 @@ static struct dmi_system_id acer_quirks[] = {
>>>       },
>>>       {
>>>               .callback = dmi_matched,
>>> +             .ident = "Acer Aspire 4739Z",
>>> +             .matches = {
>>> +                     DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
>>> +                     DMI_MATCH(DMI_PRODUCT_NAME, "AS4739Z"),
>>> +             },
>>> +             .driver_data = &quirk_acer_travelmate_2490,
>>> +     },
>>
>> Why "Acer Aspire 4739Z" set to quirk the same with travelmate 2490?
>> There only have a mailled capability flag in this quirk, why you used
>> it?
>> Does that because Aspire 4739Z also have mail hotkey button?
>>
>> If the answer is NO, then don't need add a new quirk.
> Sorry, I missunderstanding it, there is no mailled on it, I'll remove it.
>
>>
>>> +     {
>>> +             .callback = dmi_matched,
>>> +             .ident = "Acer eMachines 4739Z",
>>> +             .matches = {
>>> +                     DMI_MATCH(DMI_SYS_VENDOR, "eMachines"),
>>> +                     DMI_MATCH(DMI_PRODUCT_NAME, "AS4739Z"),
>>> +             },
>>> +             .driver_data = &quirk_acer_aspire_4739z,
>>> +     },
>>> +     {
>>> +             .callback = dmi_matched,
>>>               .ident = "Acer Aspire 5100",
>>>               .matches = {
>>>                       DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
>>> @@ -1446,7 +1482,7 @@ static void acer_rfkill_exit(void)
>>>  static ssize_t show_bool_threeg(struct device *dev,
>>>       struct device_attribute *attr, char *buf)
>>>  {
>>> -     u32 result; \
>>> +     u32 result;
>>>       acpi_status status;
>>>
>>>       pr_info("This threeg sysfs will be removed in 2012"
>>> @@ -1501,6 +1537,7 @@ static void acer_wmi_notify(u32 value, void *context)
>>>       struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
>>>       union acpi_object *obj;
>>>       struct event_return_value return_value;
>>> +     struct brightness_event_return_value brightness_return_value;
>>>       acpi_status status;
>>>       u16 device_state;
>>>       const struct key_entry *key;
>>> @@ -1531,6 +1568,7 @@ static void acer_wmi_notify(u32 value, void *context)
>>>
>>>       switch (return_value.function) {
>>>       case WMID_HOTKEY_EVENT:
>>> +     case WMID_HOTKEY_BREAK_EVENT:
>>>               device_state = return_value.device_state;
>>>               pr_debug("device state: 0x%x\n", device_state);
>>>
>>> @@ -1558,6 +1596,29 @@ static void acer_wmi_notify(u32 value, void *context)
>>>                                                  1, true);
>>>               }
>>>               break;
>>> +     case WMID_BRIGHTNESS_CHANGE_EVENT:
>>> +             // to avoid the brightness event be handled twice
>>> +             if( quirks->brightness != 2)
>>> +                     break;
>>> +             brightness_return_value = *((struct brightness_event_return_value *)&return_value);
>>> +             /* brightness is decreasing */
>>> +             if( cur_brightness > brightness_return_value.brightness_level1)
>>> +                     key = sparse_keymap_entry_from_scancode(acer_wmi_input_dev, 0x63);
>>> +             /* brightness is increasing */
>>> +             else if( cur_brightness < brightness_return_value.brightness_level1)
>>> +                     key = sparse_keymap_entry_from_scancode(acer_wmi_input_dev, 0x62);
>>> +             /* brightness is already at the bottom */
>>> +             else if( cur_brightness == 0)
>>> +                     key = sparse_keymap_entry_from_scancode(acer_wmi_input_dev, 0x63);
>>> +             /* brightness is already at the top */
>>> +             else if( cur_brightness == 9)
>>> +                     key = sparse_keymap_entry_from_scancode(acer_wmi_input_dev, 0x62);
>>> +
>>> +             sparse_keymap_report_entry(acer_wmi_input_dev, key, 1, true);
>>> +             cur_brightness= return_value.key_num;
>>> +             break;
>>
>> The same,
>> why should you control the brightness level number in acer-wmi driver?
>> Does it not control by acpi video driver and _BCL/_BCM in DSDT?
>>
>> And, why choice 0x62, 0x63? The above 2 number is not from your wmi
>> notify result, why use them?
> The problem is that the BIOS/EC doesn't send out the correct brightness
> key events, the BIOS engineer just choose non-used values for those keys
> that are defined in Acer WMI spec. Because they think those key events
> should be captured from WMI, not from BIOS/EC.
> So that, we have to read the brightness change event from WMI key events.
> And then use the ACPI way to adjust the brightness.
>
>>
>>> +     case WMID_GRAPHIC_SWITCH_EVENT:
>>> +     case WMID_SENSOR_EVENT:
>>>       default:
>>>               pr_warn("Unknown function number - %d - %d\n",
>>>                       return_value.function, return_value.key_num);
>>
>>
>> Thank's a lot!
>> Joey Lee
>>
>>
>
> Best regards,
> AceLan Kao.
>
> --
> Chia-Lin Kao(AceLan)
> http://blog.acelan.idv.tw/
> E-Mail: acelan.kaoATcanonical.com (s/AT/@/)
>



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

[-- Attachment #2: 0001-acer-wmi-support-Acer-Aspire-eMachines-4739z.patch --]
[-- Type: text/x-patch, Size: 5607 bytes --]

From 17b9d03c182da6c9dba4dc39da5850e8fac1d407 Mon Sep 17 00:00:00 2001
From: AceLan Kao <acelan.kao@canonical.com>
Date: Thu, 8 Sep 2011 10:43:08 +0800
Subject: [PATCH] acer-wmi: support Acer Aspire/eMachines 4739z

There are 2 kind of 4739Z, the one that vendor name is ACER sents out
the correct brightness key events, so we don't need quirk for it.
And another one with eMachines as it's vendor name follows Acer WMI spec.
and doesn't sent out brightness key events, so I added a new quirk,
quirk_acer_aspire_4739z, for it, and handle the key events in
acer_wmi_notify().

BTW, we need other key event types that defined in Acer WMI spec., so
I added them in acer_wmi_event_ids enumerate.

Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
---
 drivers/platform/x86/acer-wmi.c |   77 +++++++++++++++++++++++++++++++++++++--
 1 files changed, 73 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
index df730c9..a353153 100644
--- a/drivers/platform/x86/acer-wmi.c
+++ b/drivers/platform/x86/acer-wmi.c
@@ -94,6 +94,10 @@ MODULE_ALIAS("wmi:676AA15E-6A47-4D9F-A2CC-1E6D18D14026");
 
 enum acer_wmi_event_ids {
 	WMID_HOTKEY_EVENT = 0x1,
+	WMID_HOTKEY_BREAK_EVENT = 0x2,
+	WMID_GRAPHIC_SWITCH_EVENT = 0x3,
+	WMID_BRIGHTNESS_CHANGE_EVENT = 0x4,
+	WMID_SENSOR_EVENT = 0x5,
 };
 
 static const struct key_entry acer_wmi_keymap[] = {
@@ -112,9 +116,9 @@ static const struct key_entry acer_wmi_keymap[] = {
 	{KE_IGNORE, 0x45, {KEY_STOP} },
 	{KE_IGNORE, 0x48, {KEY_VOLUMEUP} },
 	{KE_IGNORE, 0x49, {KEY_VOLUMEDOWN} },
-	{KE_IGNORE, 0x61, {KEY_SWITCHVIDEOMODE} },
-	{KE_IGNORE, 0x62, {KEY_BRIGHTNESSUP} },
-	{KE_IGNORE, 0x63, {KEY_BRIGHTNESSDOWN} },
+	{KE_KEY, 0x61, {KEY_SWITCHVIDEOMODE} },
+	{KE_KEY, 0x62, {KEY_BRIGHTNESSUP} },
+	{KE_KEY, 0x63, {KEY_BRIGHTNESSDOWN} },
 	{KE_KEY, 0x64, {KEY_SWITCHVIDEOMODE} },	/* Display Switch */
 	{KE_IGNORE, 0x81, {KEY_SLEEP} },
 	{KE_KEY, 0x82, {KEY_TOUCHPAD_TOGGLE} },	/* Touch Pad On/Off */
@@ -131,6 +135,15 @@ struct event_return_value {
 	u32 reserved;
 } __attribute__((packed));
 
+
+struct brightness_event {
+	u8 function;
+	u8 brightness;
+	u8 brightness_2nd_display;
+	u8 reserved;
+	u32 reserved2;
+} __packed;
+
 /*
  * GUID3 Get Device Status device flags
  */
@@ -199,6 +212,7 @@ enum interface_flags {
 #define ACER_DEFAULT_THREEG    0
 
 static int max_brightness = 0xF;
+static int cur_brightness = 9;
 
 static int mailled = -1;
 static int brightness = -1;
@@ -310,6 +324,10 @@ static struct quirk_entry quirk_lenovo_ideapad_s205 = {
 	.wireless = 3,
 };
 
+static struct quirk_entry quirk_acer_aspire_4739z = {
+	.brightness = 2,
+};
+
 /* The Aspire One has a dummy ACPI-WMI interface - disable it */
 static struct dmi_system_id __devinitdata acer_blacklist[] = {
 	{
@@ -368,6 +386,23 @@ static struct dmi_system_id acer_quirks[] = {
 	},
 	{
 		.callback = dmi_matched,
+		.ident = "Acer Aspire 4739Z",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "AS4739Z"),
+		},
+	},
+	{
+		.callback = dmi_matched,
+		.ident = "Acer eMachines 4739Z",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "eMachines"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "AS4739Z"),
+		},
+		.driver_data = &quirk_acer_aspire_4739z,
+	},
+	{
+		.callback = dmi_matched,
 		.ident = "Acer Aspire 5100",
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
@@ -1446,7 +1481,7 @@ static void acer_rfkill_exit(void)
 static ssize_t show_bool_threeg(struct device *dev,
 	struct device_attribute *attr, char *buf)
 {
-	u32 result; \
+	u32 result;
 	acpi_status status;
 
 	pr_info("This threeg sysfs will be removed in 2012"
@@ -1501,6 +1536,7 @@ static void acer_wmi_notify(u32 value, void *context)
 	struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
 	union acpi_object *obj;
 	struct event_return_value return_value;
+	struct brightness_event brightness_value;
 	acpi_status status;
 	u16 device_state;
 	const struct key_entry *key;
@@ -1531,6 +1567,7 @@ static void acer_wmi_notify(u32 value, void *context)
 
 	switch (return_value.function) {
 	case WMID_HOTKEY_EVENT:
+	case WMID_HOTKEY_BREAK_EVENT:
 		device_state = return_value.device_state;
 		pr_debug("device state: 0x%x\n", device_state);
 
@@ -1558,6 +1595,38 @@ static void acer_wmi_notify(u32 value, void *context)
 						   1, true);
 		}
 		break;
+	case WMID_BRIGHTNESS_CHANGE_EVENT:
+		/* to avoid the brightness event be handled twice */
+		if (quirks->brightness != 2)
+			break;
+		/* we need ACPI video backlight support */
+		if (!acpi_video_backlight_support())
+			break;
+
+		brightness_value =
+			*((struct brightness_event *)&return_value);
+		/* brightness is decreasing */
+		if (cur_brightness > brightness_value.brightness)
+			key = sparse_keymap_entry_from_scancode(
+					acer_wmi_input_dev, 0x63);
+		/* brightness is increasing */
+		else if (cur_brightness < brightness_value.brightness)
+			key = sparse_keymap_entry_from_scancode(
+					acer_wmi_input_dev, 0x62);
+		/* brightness is already at the bottom */
+		else if (cur_brightness == 0)
+			key = sparse_keymap_entry_from_scancode(
+					acer_wmi_input_dev, 0x63);
+		/* brightness is already at the top */
+		else if (cur_brightness == 9)
+			key = sparse_keymap_entry_from_scancode(
+					acer_wmi_input_dev, 0x62);
+
+		sparse_keymap_report_entry(acer_wmi_input_dev, key, 1, true);
+		cur_brightness = return_value.key_num;
+		break;
+	case WMID_GRAPHIC_SWITCH_EVENT:
+	case WMID_SENSOR_EVENT:
 	default:
 		pr_warn("Unknown function number - %d - %d\n",
 			return_value.function, return_value.key_num);
-- 
1.7.5.4

Re: [PATCH] acer-wmi: support Acer Aspire/eMachines 4739z

[joeyli]

於 四，2011-09-08 於 10:30 +0800，AceLan Kao 提到：
> Hi,
> 
> 2011/9/7 joeyli <jlee@suse.com>:
> > 於 三，2011-09-07 於 15:32 +0800，AceLan Kao 提到：
> >> There are 2 kind of 4739Z, the one that vendor name is ACER sents out
> >> the correct brightness key events, so we use the
> >
> > Current brightness key events means the Fn key direct send out key code
> > but not from wmi notify event?
> Currently, most of the machines send out the correctly brightness key
> code from EC,
> so we don't have to handle them. The video ACPI will capture the
> events and adjust
> the brightness.
> Acer Aspire 4739Z sends out the correct brightness key code as other
> machines do.
> Acer eMachines 4739Z doesn't sends out correct key codes, it sends out
> meaningless
> key codes, so we have to capture the brightness event from WMI and remapping to

What's the number of meaningless key codes? Maybe we should remap it in
udev but not in acer-wmi driver.

> the correct key code and raise the correct key events to ACPI.
> 

Transfer wmi notify event to key event is fine, but keep and maintain
the current brightness level in acer-wmi is not a good idea, because
BIOS also maintain a BRTL flag in DSDT.
If there have other userland application touch brightness level through
_BCM, then will have risk causes the value out of sync.

> >
> >> quirk_acer_travelmate_2490. And another one with eMachines as it's
> >> vendor name follows Acer WMI spec. and doesn't sent out brightness key
> >> events, so I added a new quirk, quirk_acer_aspire_4739z, for it, and
> >> handle the key events in acer_wmi_notify().
> >>
> >> BTW, we need other key event types that defined in Acer WMI spec., so
> >> I added them in acer_wmi_event_ids enumerate.
> >>
> >> Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
> >> ---
> >>  drivers/platform/x86/acer-wmi.c |   69 ++++++++++++++++++++++++++++++++++++--
> >>  1 files changed, 65 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
> >> index df730c9..257c7f2 100644
> >> --- a/drivers/platform/x86/acer-wmi.c
> >> +++ b/drivers/platform/x86/acer-wmi.c
> >> @@ -94,6 +94,10 @@ MODULE_ALIAS("wmi:676AA15E-6A47-4D9F-A2CC-1E6D18D14026");
> >>
> >>  enum acer_wmi_event_ids {
> >>       WMID_HOTKEY_EVENT = 0x1,
> >> +     WMID_HOTKEY_BREAK_EVENT = 0x02,
> >> +     WMID_GRAPHIC_SWITCH_EVENT = 0x03,
> >
> > Please change to 0x2 and 0x3 for consistency in this enum.
> Okay, will do.
> 

I read your new patch, thank's for your change.

> >
> >> +     WMID_BRIGHTNESS_CHANGE_EVENT = 0x4,
> >> +     WMID_SENSOR_EVENT = 0x5,
> >>  };
> >>
> >>  static const struct key_entry acer_wmi_keymap[] = {
> >> @@ -112,9 +116,9 @@ static const struct key_entry acer_wmi_keymap[] = {
> >>       {KE_IGNORE, 0x45, {KEY_STOP} },
> >>       {KE_IGNORE, 0x48, {KEY_VOLUMEUP} },
> >>       {KE_IGNORE, 0x49, {KEY_VOLUMEDOWN} },
> >> -     {KE_IGNORE, 0x61, {KEY_SWITCHVIDEOMODE} },
> >> -     {KE_IGNORE, 0x62, {KEY_BRIGHTNESSUP} },
> >> -     {KE_IGNORE, 0x63, {KEY_BRIGHTNESSDOWN} },
> >> +     {KE_KEY, 0x61, {KEY_SWITCHVIDEOMODE} },
> >> +     {KE_KEY, 0x62, {KEY_BRIGHTNESSUP} },
> >> +     {KE_KEY, 0x63, {KEY_BRIGHTNESSDOWN} },
> >
> > Why enable 0x61, 0x62 and 0x63? Does it for Aspire 4739z or eMachines
> > 4739z?
> It's only for eMachines now and only be activated when quirk->brightness == 2.
> 
> > Why choice 0x61, 0x62 and 0x63?
> > 0x61, 0x62 and 0x63 are from your machine's wmi notify event result?
> They used to be those values, so I just enable them.
> And indeed, Acer Aspire 4739Z reports those key codes, 0x62, 0x63, while
> brightness key be pressed, so I think other machines might report the same key
> codes, but they don't need to handle that.
> acer-wmi doesn't handle brightness change event, so there will be an
> error message
> comes from acer_wmi_notify(), says "Unknown function number - 3 - xx", that's
> why those value became ignored.
> 
> We can choose any value we like other than, 0x62, 0x63, since we don't care what
> key codes are reported, I just need a mapping entry here, so that

But, why you enable 0x61? It not related to brightness control, which
machine use it?

On the other hand, 
I will implement a function to fill the keymap when acer-wmi probe, that
avoid some problem from we hard-code keymap.

> I can use sparse_keymap_report_entry() later in acer_wmi_notify()
> function to report
> the correct brightness key events.
> 
> >
> > But I didn't see the result from your following code.=
> In acer_wmi_notify() function, I use the values, 0x62, 0x63, directly
> to remap to the
> correct brightness key code.
> 
> >
> >>       {KE_KEY, 0x64, {KEY_SWITCHVIDEOMODE} }, /* Display Switch */
> >>       {KE_IGNORE, 0x81, {KEY_SLEEP} },
> >>       {KE_KEY, 0x82, {KEY_TOUCHPAD_TOGGLE} }, /* Touch Pad On/Off */
> >> @@ -131,6 +135,15 @@ struct event_return_value {
> >>       u32 reserved;
> >>  } __attribute__((packed));
> >>
> >> +
> >> +struct brightness_event_return_value {
> >> +     u8 function;
> >> +     u8 brightness_level1;
> >> +     u8 brightness_level2;
> >
> > Please don't use brightness_level"1" or level"2", it confused to me or
> > other code reviewer and nobody can remember the definition after a
> > couple of months.
> > Please try to give them more meaningful names.
> Got it.
> 

thank's

> >
> >> +     u8 reserved;
> >> +     u32 reserved2;
> >> +} __attribute__((packed));
> >> +
> >>  /*
> >>   * GUID3 Get Device Status device flags
> >>   */
> >> @@ -199,6 +212,7 @@ enum interface_flags {
> >>  #define ACER_DEFAULT_THREEG    0
> >>
> >>  static int max_brightness = 0xF;
> >> +static int cur_brightness= 9;
> >>
> >
> > Why should you control the current brightness level but not control by
> > _BCL/_BCM in DSDT?
> The value here is just recording the current brightness.
> Because we don't know which key the user pressed, we just know the
> brightness change event happened and what the new brightness value is,
> so we should record the current brightness value and then compare to the
> new value, so that we could know the brightness up or down key be pressed.
> 
> Although, in the WMI spec., the brightness value is defined from 0x0 ~ 0x64,
> what I observed is 0 ~ 9, so I assume that the default brightness is 9, the max.
> The value could be wrong for the first time adjusting the brightness, that's not
> perfect, but acceptable I think.
> 

Like I said, keep current brightness acer-wmi is not good idea and have
risk to conflict with acpi video driver if there have other userland
application change brightness level through _BCM.

> >
> >>  static int mailled = -1;
> >>  static int brightness = -1;
> >> @@ -310,6 +324,10 @@ static struct quirk_entry quirk_lenovo_ideapad_s205 = {
> >>       .wireless = 3,
> >>  };
> >>
> >> +static struct quirk_entry quirk_acer_aspire_4739z= {
> >> +     .brightness = 2,
> >> +};
> >> +
> >>  /* The Aspire One has a dummy ACPI-WMI interface - disable it */
> >>  static struct dmi_system_id __devinitdata acer_blacklist[] = {
> >>       {
> >> @@ -368,6 +386,24 @@ static struct dmi_system_id acer_quirks[] = {
> >>       },
> >>       {
> >>               .callback = dmi_matched,
> >> +             .ident = "Acer Aspire 4739Z",
> >> +             .matches = {
> >> +                     DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
> >> +                     DMI_MATCH(DMI_PRODUCT_NAME, "AS4739Z"),
> >> +             },
> >> +             .driver_data = &quirk_acer_travelmate_2490,
> >> +     },
> >
> > Why "Acer Aspire 4739Z" set to quirk the same with travelmate 2490?
> > There only have a mailled capability flag in this quirk, why you used
> > it?
> > Does that because Aspire 4739Z also have mail hotkey button?
> >
> > If the answer is NO, then don't need add a new quirk.
> Sorry, I missunderstanding it, there is no mailled on it, I'll remove it.
> 

I read your new patch, please direct remove whole Acer Aspire 4739Z
quirk block. Do you have any good reason to add the quirk block?

> >> +     {
> >> +             .callback = dmi_matched,
> >> +             .ident = "Acer eMachines 4739Z",
> >> +             .matches = {
> >> +                     DMI_MATCH(DMI_SYS_VENDOR, "eMachines"),
> >> +                     DMI_MATCH(DMI_PRODUCT_NAME, "AS4739Z"),
> >> +             },
> >> +             .driver_data = &quirk_acer_aspire_4739z,
> >> +     },
> >> +     {
> >> +             .callback = dmi_matched,
> >>               .ident = "Acer Aspire 5100",
> >>               .matches = {
> >>                       DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
> >> @@ -1446,7 +1482,7 @@ static void acer_rfkill_exit(void)
> >>  static ssize_t show_bool_threeg(struct device *dev,
> >>       struct device_attribute *attr, char *buf)
> >>  {
> >> -     u32 result; \
> >> +     u32 result;
> >>       acpi_status status;
> >>
> >>       pr_info("This threeg sysfs will be removed in 2012"
> >> @@ -1501,6 +1537,7 @@ static void acer_wmi_notify(u32 value, void *context)
> >>       struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
> >>       union acpi_object *obj;
> >>       struct event_return_value return_value;
> >> +     struct brightness_event_return_value brightness_return_value;
> >>       acpi_status status;
> >>       u16 device_state;
> >>       const struct key_entry *key;
> >> @@ -1531,6 +1568,7 @@ static void acer_wmi_notify(u32 value, void *context)
> >>
> >>       switch (return_value.function) {
> >>       case WMID_HOTKEY_EVENT:
> >> +     case WMID_HOTKEY_BREAK_EVENT:

Why add BREAK EVENT here? for which key?
Does it possible causes other machine run the same function key twice?

> >>               device_state = return_value.device_state;
> >>               pr_debug("device state: 0x%x\n", device_state);
> >>
> >> @@ -1558,6 +1596,29 @@ static void acer_wmi_notify(u32 value, void *context)
> >>                                                  1, true);
> >>               }
> >>               break;
> >> +     case WMID_BRIGHTNESS_CHANGE_EVENT:
> >> +             // to avoid the brightness event be handled twice
> >> +             if( quirks->brightness != 2)
> >> +                     break;
> >> +             brightness_return_value = *((struct brightness_event_return_value *)&return_value);
> >> +             /* brightness is decreasing */
> >> +             if( cur_brightness > brightness_return_value.brightness_level1)
> >> +                     key = sparse_keymap_entry_from_scancode(acer_wmi_input_dev, 0x63);
> >> +             /* brightness is increasing */
> >> +             else if( cur_brightness < brightness_return_value.brightness_level1)
> >> +                     key = sparse_keymap_entry_from_scancode(acer_wmi_input_dev, 0x62);
> >> +             /* brightness is already at the bottom */
> >> +             else if( cur_brightness == 0)
> >> +                     key = sparse_keymap_entry_from_scancode(acer_wmi_input_dev, 0x63);
> >> +             /* brightness is already at the top */
> >> +             else if( cur_brightness == 9)
> >> +                     key = sparse_keymap_entry_from_scancode(acer_wmi_input_dev, 0x62);
> >> +
> >> +             sparse_keymap_report_entry(acer_wmi_input_dev, key, 1, true);
> >> +             cur_brightness= return_value.key_num;
> >> +             break;
> >
> > The same,
> > why should you control the brightness level number in acer-wmi driver?
> > Does it not control by acpi video driver and _BCL/_BCM in DSDT?
> >
> > And, why choice 0x62, 0x63? The above 2 number is not from your wmi
> > notify result, why use them?
> The problem is that the BIOS/EC doesn't send out the correct brightness
> key events, the BIOS engineer just choose non-used values for those keys
> that are defined in Acer WMI spec. Because they think those key events
> should be captured from WMI, not from BIOS/EC.
> So that, we have to read the brightness change event from WMI key events.
> And then use the ACPI way to adjust the brightness.
> 

After reviewed your DSDT, I thought _Q8E and _Q8F are mapping to your
brightness up/down Fn key, please enable acpi debug message to confirm
it.

And, in _Q8E/Q8F:

    Method (_Q8E, 0, NotSerialized)             /* Brightness up ? */
    {  
        If (CondRefOf (HBRT))
        {  
            HBRT (0x03)
        }

        If (IGDS)
        {   
            If (And (0x04, DSEN))               /* when DSEn is 0x04 is 100 */
            {  
                BRTN (0x86)                     /* ACPI_VIDEO_NOTIFY_INC_BRIGHTNESS 0x86  */
            }

The machine emit standard ACPI_VIDEO_NOTIFY_INC_BRIGHTNESS event to
video device when IDGS is true and DSEN is 0x04.

Please kindly check with BIOS team for what's the definition with IGDS
and DSEN, and why the DSEN should set to 100 (looks likes it need
disable automatically control the brightness level in BIOS).

This is a good chance to ask BIOS team to know more useful information
to us.

I don't want maintain brightness level in acer-wmi, we can transfer the
wmi event to key event, but maintain current brightness level is not
good and dirty.

> >
> >> +     case WMID_GRAPHIC_SWITCH_EVENT:
> >> +     case WMID_SENSOR_EVENT:
> >>       default:
> >>               pr_warn("Unknown function number - %d - %d\n",
> >>                       return_value.function, return_value.key_num);
> >
> >


Thank's a lot!
Joey Lee

Re: [PATCH] acer-wmi: support Acer Aspire/eMachines 4739z

[AceLan Kao]

Hi,

2011/9/8 joeyli <jlee@suse.com>:
> 於 四，2011-09-08 於 10:30 +0800，AceLan Kao 提到：
>> Hi,
>>
>> 2011/9/7 joeyli <jlee@suse.com>:
>> > 於 三，2011-09-07 於 15:32 +0800，AceLan Kao 提到：
>> >> There are 2 kind of 4739Z, the one that vendor name is ACER sents out
>> >> the correct brightness key events, so we use the
>> >
>> > Current brightness key events means the Fn key direct send out key code
>> > but not from wmi notify event?
>> Currently, most of the machines send out the correctly brightness key
>> code from EC,
>> so we don't have to handle them. The video ACPI will capture the
>> events and adjust
>> the brightness.
>> Acer Aspire 4739Z sends out the correct brightness key code as other
>> machines do.
>> Acer eMachines 4739Z doesn't sends out correct key codes, it sends out
>> meaningless
>> key codes, so we have to capture the brightness event from WMI and remapping to
>
> What's the number of meaningless key codes? Maybe we should remap it in
> udev but not in acer-wmi driver.
brightness up is -
brightness down is -

The reason why we shouldn't remap the keys in udev is that when it
comes to follow the
Acer WMI spec., the BIOS engineer won't care about which key code will
be submitted
while the brightness key be pressed. So, the key codes might change if
there is a BIOS
upgrade.

>
>> the correct key code and raise the correct key events to ACPI.
>>
>
> Transfer wmi notify event to key event is fine, but keep and maintain
> the current brightness level in acer-wmi is not a good idea, because
> BIOS also maintain a BRTL flag in DSDT.
> If there have other userland application touch brightness level through
> _BCM, then will have risk causes the value out of sync.
Yes, that's a problem.
I don't have any idea to overcome it right now.

>
>> >
>> >> quirk_acer_travelmate_2490. And another one with eMachines as it's
>> >> vendor name follows Acer WMI spec. and doesn't sent out brightness key
>> >> events, so I added a new quirk, quirk_acer_aspire_4739z, for it, and
>> >> handle the key events in acer_wmi_notify().
>> >>
>> >> BTW, we need other key event types that defined in Acer WMI spec., so
>> >> I added them in acer_wmi_event_ids enumerate.
>> >>
>> >> Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
>> >> ---
>> >>  drivers/platform/x86/acer-wmi.c |   69 ++++++++++++++++++++++++++++++++++++--
>> >>  1 files changed, 65 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
>> >> index df730c9..257c7f2 100644
>> >> --- a/drivers/platform/x86/acer-wmi.c
>> >> +++ b/drivers/platform/x86/acer-wmi.c
>> >> @@ -94,6 +94,10 @@ MODULE_ALIAS("wmi:676AA15E-6A47-4D9F-A2CC-1E6D18D14026");
>> >>
>> >>  enum acer_wmi_event_ids {
>> >>       WMID_HOTKEY_EVENT = 0x1,
>> >> +     WMID_HOTKEY_BREAK_EVENT = 0x02,
>> >> +     WMID_GRAPHIC_SWITCH_EVENT = 0x03,
>> >
>> > Please change to 0x2 and 0x3 for consistency in this enum.
>> Okay, will do.
>>
>
> I read your new patch, thank's for your change.
>
>> >
>> >> +     WMID_BRIGHTNESS_CHANGE_EVENT = 0x4,
>> >> +     WMID_SENSOR_EVENT = 0x5,
>> >>  };
>> >>
>> >>  static const struct key_entry acer_wmi_keymap[] = {
>> >> @@ -112,9 +116,9 @@ static const struct key_entry acer_wmi_keymap[] = {
>> >>       {KE_IGNORE, 0x45, {KEY_STOP} },
>> >>       {KE_IGNORE, 0x48, {KEY_VOLUMEUP} },
>> >>       {KE_IGNORE, 0x49, {KEY_VOLUMEDOWN} },
>> >> -     {KE_IGNORE, 0x61, {KEY_SWITCHVIDEOMODE} },
>> >> -     {KE_IGNORE, 0x62, {KEY_BRIGHTNESSUP} },
>> >> -     {KE_IGNORE, 0x63, {KEY_BRIGHTNESSDOWN} },
>> >> +     {KE_KEY, 0x61, {KEY_SWITCHVIDEOMODE} },
>> >> +     {KE_KEY, 0x62, {KEY_BRIGHTNESSUP} },
>> >> +     {KE_KEY, 0x63, {KEY_BRIGHTNESSDOWN} },
>> >
>> > Why enable 0x61, 0x62 and 0x63? Does it for Aspire 4739z or eMachines
>> > 4739z?
>> It's only for eMachines now and only be activated when quirk->brightness == 2.
>>
>> > Why choice 0x61, 0x62 and 0x63?
>> > 0x61, 0x62 and 0x63 are from your machine's wmi notify event result?
>> They used to be those values, so I just enable them.
>> And indeed, Acer Aspire 4739Z reports those key codes, 0x62, 0x63, while
>> brightness key be pressed, so I think other machines might report the same key
>> codes, but they don't need to handle that.
>> acer-wmi doesn't handle brightness change event, so there will be an
>> error message
>> comes from acer_wmi_notify(), says "Unknown function number - 3 - xx", that's
>> why those value became ignored.
>>
>> We can choose any value we like other than, 0x62, 0x63, since we don't care what
>> key codes are reported, I just need a mapping entry here, so that
>
> But, why you enable 0x61? It not related to brightness control, which
> machine use it?
It's for video toggle, both Aspire and eMachines 4739Z need that key event.

>
> On the other hand,
> I will implement a function to fill the keymap when acer-wmi probe, that
> avoid some problem from we hard-code keymap.
>
>> I can use sparse_keymap_report_entry() later in acer_wmi_notify()
>> function to report
>> the correct brightness key events.
>>
>> >
>> > But I didn't see the result from your following code.=
>> In acer_wmi_notify() function, I use the values, 0x62, 0x63, directly
>> to remap to the
>> correct brightness key code.
>>
>> >
>> >>       {KE_KEY, 0x64, {KEY_SWITCHVIDEOMODE} }, /* Display Switch */
>> >>       {KE_IGNORE, 0x81, {KEY_SLEEP} },
>> >>       {KE_KEY, 0x82, {KEY_TOUCHPAD_TOGGLE} }, /* Touch Pad On/Off */
>> >> @@ -131,6 +135,15 @@ struct event_return_value {
>> >>       u32 reserved;
>> >>  } __attribute__((packed));
>> >>
>> >> +
>> >> +struct brightness_event_return_value {
>> >> +     u8 function;
>> >> +     u8 brightness_level1;
>> >> +     u8 brightness_level2;
>> >
>> > Please don't use brightness_level"1" or level"2", it confused to me or
>> > other code reviewer and nobody can remember the definition after a
>> > couple of months.
>> > Please try to give them more meaningful names.
>> Got it.
>>
>
> thank's
>
>> >
>> >> +     u8 reserved;
>> >> +     u32 reserved2;
>> >> +} __attribute__((packed));
>> >> +
>> >>  /*
>> >>   * GUID3 Get Device Status device flags
>> >>   */
>> >> @@ -199,6 +212,7 @@ enum interface_flags {
>> >>  #define ACER_DEFAULT_THREEG    0
>> >>
>> >>  static int max_brightness = 0xF;
>> >> +static int cur_brightness= 9;
>> >>
>> >
>> > Why should you control the current brightness level but not control by
>> > _BCL/_BCM in DSDT?
>> The value here is just recording the current brightness.
>> Because we don't know which key the user pressed, we just know the
>> brightness change event happened and what the new brightness value is,
>> so we should record the current brightness value and then compare to the
>> new value, so that we could know the brightness up or down key be pressed.
>>
>> Although, in the WMI spec., the brightness value is defined from 0x0 ~ 0x64,
>> what I observed is 0 ~ 9, so I assume that the default brightness is 9, the max.
>> The value could be wrong for the first time adjusting the brightness, that's not
>> perfect, but acceptable I think.
>>
>
> Like I said, keep current brightness acer-wmi is not good idea and have
> risk to conflict with acpi video driver if there have other userland
> application change brightness level through _BCM.
You are right, it's better to reconsider the implementation.

>
>> >
>> >>  static int mailled = -1;
>> >>  static int brightness = -1;
>> >> @@ -310,6 +324,10 @@ static struct quirk_entry quirk_lenovo_ideapad_s205 = {
>> >>       .wireless = 3,
>> >>  };
>> >>
>> >> +static struct quirk_entry quirk_acer_aspire_4739z= {
>> >> +     .brightness = 2,
>> >> +};
>> >> +
>> >>  /* The Aspire One has a dummy ACPI-WMI interface - disable it */
>> >>  static struct dmi_system_id __devinitdata acer_blacklist[] = {
>> >>       {
>> >> @@ -368,6 +386,24 @@ static struct dmi_system_id acer_quirks[] = {
>> >>       },
>> >>       {
>> >>               .callback = dmi_matched,
>> >> +             .ident = "Acer Aspire 4739Z",
>> >> +             .matches = {
>> >> +                     DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
>> >> +                     DMI_MATCH(DMI_PRODUCT_NAME, "AS4739Z"),
>> >> +             },
>> >> +             .driver_data = &quirk_acer_travelmate_2490,
>> >> +     },
>> >
>> > Why "Acer Aspire 4739Z" set to quirk the same with travelmate 2490?
>> > There only have a mailled capability flag in this quirk, why you used
>> > it?
>> > Does that because Aspire 4739Z also have mail hotkey button?
>> >
>> > If the answer is NO, then don't need add a new quirk.
>> Sorry, I missunderstanding it, there is no mailled on it, I'll remove it.
>>
>
> I read your new patch, please direct remove whole Acer Aspire 4739Z
> quirk block. Do you have any good reason to add the quirk block?
Acer Aspire 4739Z needs the video toggling function and the key event
is {KE_KEY, 0x61, {KEY_SWITCHVIDEOMODE} },
that's why we need it's quirk here.

>
>> >> +     {
>> >> +             .callback = dmi_matched,
>> >> +             .ident = "Acer eMachines 4739Z",
>> >> +             .matches = {
>> >> +                     DMI_MATCH(DMI_SYS_VENDOR, "eMachines"),
>> >> +                     DMI_MATCH(DMI_PRODUCT_NAME, "AS4739Z"),
>> >> +             },
>> >> +             .driver_data = &quirk_acer_aspire_4739z,
>> >> +     },
>> >> +     {
>> >> +             .callback = dmi_matched,
>> >>               .ident = "Acer Aspire 5100",
>> >>               .matches = {
>> >>                       DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
>> >> @@ -1446,7 +1482,7 @@ static void acer_rfkill_exit(void)
>> >>  static ssize_t show_bool_threeg(struct device *dev,
>> >>       struct device_attribute *attr, char *buf)
>> >>  {
>> >> -     u32 result; \
>> >> +     u32 result;
>> >>       acpi_status status;
>> >>
>> >>       pr_info("This threeg sysfs will be removed in 2012"
>> >> @@ -1501,6 +1537,7 @@ static void acer_wmi_notify(u32 value, void *context)
>> >>       struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
>> >>       union acpi_object *obj;
>> >>       struct event_return_value return_value;
>> >> +     struct brightness_event_return_value brightness_return_value;
>> >>       acpi_status status;
>> >>       u16 device_state;
>> >>       const struct key_entry *key;
>> >> @@ -1531,6 +1568,7 @@ static void acer_wmi_notify(u32 value, void *context)
>> >>
>> >>       switch (return_value.function) {
>> >>       case WMID_HOTKEY_EVENT:
>> >> +     case WMID_HOTKEY_BREAK_EVENT:
>
> Why add BREAK EVENT here? for which key?
> Does it possible causes other machine run the same function key twice?
For video toggling,
{KE_KEY, 0x61, {KEY_SWITCHVIDEOMODE} },

And yes, you are right, that might lead to the problem that switch the
output twice.
I'll add one more check for this key.

>
>> >>               device_state = return_value.device_state;
>> >>               pr_debug("device state: 0x%x\n", device_state);
>> >>
>> >> @@ -1558,6 +1596,29 @@ static void acer_wmi_notify(u32 value, void *context)
>> >>                                                  1, true);
>> >>               }
>> >>               break;
>> >> +     case WMID_BRIGHTNESS_CHANGE_EVENT:
>> >> +             // to avoid the brightness event be handled twice
>> >> +             if( quirks->brightness != 2)
>> >> +                     break;
>> >> +             brightness_return_value = *((struct brightness_event_return_value *)&return_value);
>> >> +             /* brightness is decreasing */
>> >> +             if( cur_brightness > brightness_return_value.brightness_level1)
>> >> +                     key = sparse_keymap_entry_from_scancode(acer_wmi_input_dev, 0x63);
>> >> +             /* brightness is increasing */
>> >> +             else if( cur_brightness < brightness_return_value.brightness_level1)
>> >> +                     key = sparse_keymap_entry_from_scancode(acer_wmi_input_dev, 0x62);
>> >> +             /* brightness is already at the bottom */
>> >> +             else if( cur_brightness == 0)
>> >> +                     key = sparse_keymap_entry_from_scancode(acer_wmi_input_dev, 0x63);
>> >> +             /* brightness is already at the top */
>> >> +             else if( cur_brightness == 9)
>> >> +                     key = sparse_keymap_entry_from_scancode(acer_wmi_input_dev, 0x62);
>> >> +
>> >> +             sparse_keymap_report_entry(acer_wmi_input_dev, key, 1, true);
>> >> +             cur_brightness= return_value.key_num;
>> >> +             break;
>> >
>> > The same,
>> > why should you control the brightness level number in acer-wmi driver?
>> > Does it not control by acpi video driver and _BCL/_BCM in DSDT?
>> >
>> > And, why choice 0x62, 0x63? The above 2 number is not from your wmi
>> > notify result, why use them?
>> The problem is that the BIOS/EC doesn't send out the correct brightness
>> key events, the BIOS engineer just choose non-used values for those keys
>> that are defined in Acer WMI spec. Because they think those key events
>> should be captured from WMI, not from BIOS/EC.
>> So that, we have to read the brightness change event from WMI key events.
>> And then use the ACPI way to adjust the brightness.
>>
>
> After reviewed your DSDT, I thought _Q8E and _Q8F are mapping to your
> brightness up/down Fn key, please enable acpi debug message to confirm
> it.
>
> And, in _Q8E/Q8F:
>
>    Method (_Q8E, 0, NotSerialized)             /* Brightness up ? */
>    {
>        If (CondRefOf (HBRT))
>        {
>            HBRT (0x03)
>        }
>
>        If (IGDS)
>        {
>            If (And (0x04, DSEN))               /* when DSEn is 0x04 is 100 */
>            {
>                BRTN (0x86)                     /* ACPI_VIDEO_NOTIFY_INC_BRIGHTNESS 0x86  */
>            }
>
> The machine emit standard ACPI_VIDEO_NOTIFY_INC_BRIGHTNESS event to
> video device when IDGS is true and DSEN is 0x04.
>
> Please kindly check with BIOS team for what's the definition with IGDS
> and DSEN, and why the DSEN should set to 100 (looks likes it need
> disable automatically control the brightness level in BIOS).
>
> This is a good chance to ask BIOS team to know more useful information
> to us.
I'm sorry that the BIOS team won't change anything for us.
Acer Aspire 4739Z is already an shipping product, and
Acer eMachines 4739Z only change the BIOS bootup logo and vendor name
(and maybe mixed up some key codes), so they won't change anything
from BIOS to avoid inducing problems to the product.

And the ACPI log I gave you a month ago is Acer Aspire 4739Z, so
the key codes are correct.
The eMachine I have now is broken, I'm trying to reinstall it and dump
the eMachines' ACPI log to you later.

>
> I don't want maintain brightness level in acer-wmi, we can transfer the
> wmi event to key event, but maintain current brightness level is not
> good and dirty.
Agree, I need more time to work out other solution.
Talk to you later.

>
>> >
>> >> +     case WMID_GRAPHIC_SWITCH_EVENT:
>> >> +     case WMID_SENSOR_EVENT:
>> >>       default:
>> >>               pr_warn("Unknown function number - %d - %d\n",
>> >>                       return_value.function, return_value.key_num);
>> >
>> >
>
>
> Thank's a lot!
> Joey Lee
>
> --
> To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



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

Re: [PATCH] acer-wmi: support Acer Aspire/eMachines 4739z

[joeyli]

於 四，2011-09-08 於 14:38 +0800，AceLan Kao 提到：
> Hi,
> 
> 2011/9/8 joeyli <jlee@suse.com>:
> > 於 四，2011-09-08 於 10:30 +0800，AceLan Kao 提到：
> >> Hi,
> >>
> >> 2011/9/7 joeyli <jlee@suse.com>:
> >> > 於 三，2011-09-07 於 15:32 +0800，AceLan Kao 提到：
> >> >> There are 2 kind of 4739Z, the one that vendor name is ACER sents out
> >> >> the correct brightness key events, so we use the
> >> >
> >> > Current brightness key events means the Fn key direct send out key code
> >> > but not from wmi notify event?
> >> Currently, most of the machines send out the correctly brightness key
> >> code from EC,
> >> so we don't have to handle them. The video ACPI will capture the
> >> events and adjust
> >> the brightness.
> >> Acer Aspire 4739Z sends out the correct brightness key code as other
> >> machines do.
> >> Acer eMachines 4739Z doesn't sends out correct key codes, it sends out
> >> meaningless
> >> key codes, so we have to capture the brightness event from WMI and remapping to
> >
> > What's the number of meaningless key codes? Maybe we should remap it in
> > udev but not in acer-wmi driver.
> brightness up is -
> brightness down is -
> 
> The reason why we shouldn't remap the keys in udev is that when it
> comes to follow the
> Acer WMI spec., the BIOS engineer won't care about which key code will
> be submitted
> while the brightness key be pressed. So, the key codes might change if
> there is a BIOS
> upgrade.
> 

That's bad and not make sense they emit the same key code and wmi event
code on two brightness control keys.

> >
> >> the correct key code and raise the correct key events to ACPI.
> >>
> >
> > Transfer wmi notify event to key event is fine, but keep and maintain
> > the current brightness level in acer-wmi is not a good idea, because
> > BIOS also maintain a BRTL flag in DSDT.
> > If there have other userland application touch brightness level through
> > _BCM, then will have risk causes the value out of sync.
> Yes, that's a problem.
> I don't have any idea to overcome it right now.
> 

Could you please check with BIOS team for What's the implementation on
Windows 7?
Which component on Windows take care the brightness control job?

> >> We can choose any value we like other than, 0x62, 0x63, since we don't care what
> >> key codes are reported, I just need a mapping entry here, so that
> >
> > But, why you enable 0x61? It not related to brightness control, which
> > machine use it?
> It's for video toggle, both Aspire and eMachines 4739Z need that key event.
> 

OK!

> >
> > Like I said, keep current brightness acer-wmi is not good idea and have
> > risk to conflict with acpi video driver if there have other userland
> > application change brightness level through _BCM.
> You are right, it's better to reconsider the implementation.
> 
> >
> >> >
> >> >>  static int mailled = -1;
> >> >>  static int brightness = -1;
> >> >> @@ -310,6 +324,10 @@ static struct quirk_entry quirk_lenovo_ideapad_s205 = {
> >> >>       .wireless = 3,
> >> >>  };
> >> >>
> >> >> +static struct quirk_entry quirk_acer_aspire_4739z= {
> >> >> +     .brightness = 2,
> >> >> +};
> >> >> +
> >> >>  /* The Aspire One has a dummy ACPI-WMI interface - disable it */
> >> >>  static struct dmi_system_id __devinitdata acer_blacklist[] = {
> >> >>       {
> >> >> @@ -368,6 +386,24 @@ static struct dmi_system_id acer_quirks[] = {
> >> >>       },
> >> >>       {
> >> >>               .callback = dmi_matched,
> >> >> +             .ident = "Acer Aspire 4739Z",
> >> >> +             .matches = {
> >> >> +                     DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
> >> >> +                     DMI_MATCH(DMI_PRODUCT_NAME, "AS4739Z"),
> >> >> +             },
> >> >> +             .driver_data = &quirk_acer_travelmate_2490,
> >> >> +     },
> >> >
> >> > Why "Acer Aspire 4739Z" set to quirk the same with travelmate 2490?
> >> > There only have a mailled capability flag in this quirk, why you used
> >> > it?
> >> > Does that because Aspire 4739Z also have mail hotkey button?
> >> >
> >> > If the answer is NO, then don't need add a new quirk.
> >> Sorry, I missunderstanding it, there is no mailled on it, I'll remove it.
> >>
> >
> > I read your new patch, please direct remove whole Acer Aspire 4739Z
> > quirk block. Do you have any good reason to add the quirk block?
> Acer Aspire 4739Z needs the video toggling function and the key event
> is {KE_KEY, 0x61, {KEY_SWITCHVIDEOMODE} },
> that's why we need it's quirk here.
> 

The keymap was setup when driver deteced wmi event guid, the quirk block
doesn't have anything related to wmi event keymap setup. Please remove
the quirk block then test again.

> >> >>
> >> >>       switch (return_value.function) {
> >> >>       case WMID_HOTKEY_EVENT:
> >> >> +     case WMID_HOTKEY_BREAK_EVENT:
> >
> > Why add BREAK EVENT here? for which key?
> > Does it possible causes other machine run the same function key twice?
> For video toggling,
> {KE_KEY, 0x61, {KEY_SWITCHVIDEOMODE} },
> 
> And yes, you are right, that might lead to the problem that switch the
> output twice.
> I'll add one more check for this key.
> 
> >
> >> >>               device_state = return_value.device_state;
> >> >>               pr_debug("device state: 0x%x\n", device_state);
> >> >>
> >> >> @@ -1558,6 +1596,29 @@ static void acer_wmi_notify(u32 value, void *context)
> >> >>                                                  1, true);
> >> >>               }
> >> >>               break;
> >> >> +     case WMID_BRIGHTNESS_CHANGE_EVENT:
> >> >> +             // to avoid the brightness event be handled twice
> >> >> +             if( quirks->brightness != 2)
> >> >> +                     break;
> >> >> +             brightness_return_value = *((struct brightness_event_return_value *)&return_value);
> >> >> +             /* brightness is decreasing */
> >> >> +             if( cur_brightness > brightness_return_value.brightness_level1)
> >> >> +                     key = sparse_keymap_entry_from_scancode(acer_wmi_input_dev, 0x63);
> >> >> +             /* brightness is increasing */
> >> >> +             else if( cur_brightness < brightness_return_value.brightness_level1)
> >> >> +                     key = sparse_keymap_entry_from_scancode(acer_wmi_input_dev, 0x62);
> >> >> +             /* brightness is already at the bottom */
> >> >> +             else if( cur_brightness == 0)
> >> >> +                     key = sparse_keymap_entry_from_scancode(acer_wmi_input_dev, 0x63);
> >> >> +             /* brightness is already at the top */
> >> >> +             else if( cur_brightness == 9)
> >> >> +                     key = sparse_keymap_entry_from_scancode(acer_wmi_input_dev, 0x62);
> >> >> +
> >> >> +             sparse_keymap_report_entry(acer_wmi_input_dev, key, 1, true);
> >> >> +             cur_brightness= return_value.key_num;
> >> >> +             break;
> >> >
> >> > The same,
> >> > why should you control the brightness level number in acer-wmi driver?
> >> > Does it not control by acpi video driver and _BCL/_BCM in DSDT?
> >> >
> >> > And, why choice 0x62, 0x63? The above 2 number is not from your wmi
> >> > notify result, why use them?
> >> The problem is that the BIOS/EC doesn't send out the correct brightness
> >> key events, the BIOS engineer just choose non-used values for those keys
> >> that are defined in Acer WMI spec. Because they think those key events
> >> should be captured from WMI, not from BIOS/EC.
> >> So that, we have to read the brightness change event from WMI key events.
> >> And then use the ACPI way to adjust the brightness.
> >>
> >
> > After reviewed your DSDT, I thought _Q8E and _Q8F are mapping to your
> > brightness up/down Fn key, please enable acpi debug message to confirm
> > it.
> >
> > And, in _Q8E/Q8F:
> >
> >    Method (_Q8E, 0, NotSerialized)             /* Brightness up ? */
> >    {
> >        If (CondRefOf (HBRT))
> >        {
> >            HBRT (0x03)
> >        }
> >
> >        If (IGDS)
> >        {
> >            If (And (0x04, DSEN))               /* when DSEn is 0x04 is 100 */
> >            {
> >                BRTN (0x86)                     /* ACPI_VIDEO_NOTIFY_INC_BRIGHTNESS 0x86  */
> >            }
> >
> > The machine emit standard ACPI_VIDEO_NOTIFY_INC_BRIGHTNESS event to
> > video device when IDGS is true and DSEN is 0x04.
> >
> > Please kindly check with BIOS team for what's the definition with IGDS
> > and DSEN, and why the DSEN should set to 100 (looks likes it need
> > disable automatically control the brightness level in BIOS).
> >
> > This is a good chance to ask BIOS team to know more useful information
> > to us.
> I'm sorry that the BIOS team won't change anything for us.
> Acer Aspire 4739Z is already an shipping product, and
> Acer eMachines 4739Z only change the BIOS bootup logo and vendor name
> (and maybe mixed up some key codes), so they won't change anything
> from BIOS to avoid inducing problems to the product.
> 

Please please check with BIOS team for:

What's the definition with IGDS and DSEN, and why the DSEN should set to
100 (looks likes it need disable automatically control the brightness
level in BIOS).

This is the only moment for ODM's BIOS team can response us, please help
to check with them for how the implementation on Windows platform. I
didn't say need BIOS team modify anything, but we need dig more things
for how can they implement 2 keys used 1 event on Windows platform.

> And the ACPI log I gave you a month ago is Acer Aspire 4739Z, so
> the key codes are correct.
> The eMachine I have now is broken, I'm trying to reinstall it and dump
> the eMachines' ACPI log to you later.
> 

Thank's!

> >
> > I don't want maintain brightness level in acer-wmi, we can transfer the
> > wmi event to key event, but maintain current brightness level is not
> > good and dirty.
> Agree, I need more time to work out other solution.
> Talk to you later.
> 

Please help to check with BIOS team and ODM for how did they implement
it on Windows platform, per current information from DSDT and BIOS team,
it's dirty and not make sense to maintain the current brightness value
outside ACPI space.


Thank's for your help! 
Joey Lee