Re: [PATCH] eeepc-wmi: new driver for WMI based hotkeys on Eee PC laptops

Re: [PATCH] eeepc-wmi: new driver for WMI based hotkeys on Eee PC laptops

[Matthew Garrett]

(Cc:ing input)

On Fri, Mar 19, 2010 at 09:39:24PM +0800, Yong Wang wrote:
> Add a WMI driver for Eee PC laptops. Currently it only supports hotkeys.

Looks good.

> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/types.h>
> +#include <linux/input.h>
> +#include <acpi/acpi_bus.h>
> +#include <acpi/acpi_drivers.h>
> +
> +MODULE_AUTHOR("Yong Wang <yong.y.wang@intel.com>");
> +MODULE_DESCRIPTION("Eee PC WMI Hotkey Driver");
> +MODULE_LICENSE("GPL");
> +
> +#define EEEPC_WMI_EVENT_GUID	"ABBC0F72-8EA1-11D1-00A0-C90629100000"
> +
> +#define NOTIFY_BRNUP_MIN	0x11
> +#define NOTIFY_BRNUP_MAX	0x1f
> +#define NOTIFY_BRNDOWN_MIN	0x20
> +#define NOTIFY_BRNDOWN_MAX	0x2e
> +
> +struct key_entry {
> +	char type;
> +	u8 code;
> +	u16 keycode;
> +};
> +
> +enum { KE_KEY, KE_END };
> +
> +static struct key_entry eeepc_wmi_keymap[] = {
> +	/* Sleep already handled via generic ACPI code */
> +	{KE_KEY, 0x5d, KEY_WLAN },
> +	{KE_KEY, 0x32, KEY_MUTE },
> +	{KE_KEY, 0x31, KEY_VOLUMEDOWN },
> +	{KE_KEY, 0x30, KEY_VOLUMEUP },
> +	{KE_KEY, NOTIFY_BRNDOWN_MIN, KEY_BRIGHTNESSDOWN },
> +	{KE_KEY, NOTIFY_BRNUP_MIN, KEY_BRIGHTNESSUP },
> +	{KE_KEY, 0xcc, KEY_SWITCHVIDEOMODE },
> +	{KE_END, 0},
> +};

This probably ought to use the new sparse keymap code. I know that there 
are drivers that are currently in the tree that don't, but it's probably 
preferable to avoid adding new ones.

> +		if (code >= NOTIFY_BRNUP_MIN && code <= NOTIFY_BRNUP_MAX)
> +			code = NOTIFY_BRNUP_MIN;
> +		else if (code >= NOTIFY_BRNDOWN_MIN && code <= NOTIFY_BRNDOWN_MAX)
> +			code = NOTIFY_BRNDOWN_MIN;

Do the brightness keys just send notifications, or do they actually 
change the brightness? If they actually change the brightness, we 
shouldn't send input events.

Other than that, looks good!
-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] eeepc-wmi: new driver for WMI based hotkeys on Eee PC laptops

[Yong Wang]

On Fri, Mar 19, 2010 at 01:59:29PM +0000, Matthew Garrett wrote:
> 
> This probably ought to use the new sparse keymap code. I know that there 
> are drivers that are currently in the tree that don't, but it's probably 
> preferable to avoid adding new ones.
> 

OK, will take a look at the new interface and revise accordingly.

> > +		if (code >= NOTIFY_BRNUP_MIN && code <= NOTIFY_BRNUP_MAX)
> > +			code = NOTIFY_BRNUP_MIN;
> > +		else if (code >= NOTIFY_BRNDOWN_MIN && code <= NOTIFY_BRNDOWN_MAX)
> > +			code = NOTIFY_BRNDOWN_MIN;
> 
> Do the brightness keys just send notifications, or do they actually 
> change the brightness? If they actually change the brightness, we 
> shouldn't send input events.
> 

Yes, hardware and bios change brightness by themselves without software intervention
on my Eee PC 1005 when pressing the hotkeys.

Thanks for the review.

-Yong

Re: [PATCH] eeepc-wmi: new driver for WMI based hotkeys on Eee PC laptops

[Matthew Garrett]

On Fri, Mar 19, 2010 at 11:10:54PM +0800, Yong Wang wrote:
> On Fri, Mar 19, 2010 at 01:59:29PM +0000, Matthew Garrett wrote:
> > 
> > This probably ought to use the new sparse keymap code. I know that there 
> > are drivers that are currently in the tree that don't, but it's probably 
> > preferable to avoid adding new ones.
> > 
> 
> OK, will take a look at the new interface and revise accordingly.

Wonderful, thanks.

> > > +		if (code >= NOTIFY_BRNUP_MIN && code <= NOTIFY_BRNUP_MAX)
> > > +			code = NOTIFY_BRNUP_MIN;
> > > +		else if (code >= NOTIFY_BRNDOWN_MIN && code <= NOTIFY_BRNDOWN_MAX)
> > > +			code = NOTIFY_BRNDOWN_MIN;
> > 
> > Do the brightness keys just send notifications, or do they actually 
> > change the brightness? If they actually change the brightness, we 
> > shouldn't send input events.
> > 
> 
> Yes, hardware and bios change brightness by themselves without software intervention
> on my Eee PC 1005 when pressing the hotkeys.

Ok. In that case, you shouldn't send input events. Once backlight 
control is implemented in the eee-wmi driver you can send notifications 
via that instead.

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

Re: [PATCH] eeepc-wmi: new driver for WMI based hotkeys on Eee PC laptops

[Yong Wang]

On Fri, Mar 19, 2010 at 03:23:23PM +0000, Matthew Garrett wrote:
> On Fri, Mar 19, 2010 at 11:10:54PM +0800, Yong Wang wrote:
> > On Fri, Mar 19, 2010 at 01:59:29PM +0000, Matthew Garrett wrote:
> > > 
> > > This probably ought to use the new sparse keymap code. I know that there 
> > > are drivers that are currently in the tree that don't, but it's probably 
> > > preferable to avoid adding new ones.
> > > 
> > 
> > OK, will take a look at the new interface and revise accordingly.
> 
> Wonderful, thanks.
> 

You are welcome.

> > > > +		if (code >= NOTIFY_BRNUP_MIN && code <= NOTIFY_BRNUP_MAX)
> > > > +			code = NOTIFY_BRNUP_MIN;
> > > > +		else if (code >= NOTIFY_BRNDOWN_MIN && code <= NOTIFY_BRNDOWN_MAX)
> > > > +			code = NOTIFY_BRNDOWN_MIN;
> > > 
> > > Do the brightness keys just send notifications, or do they actually 
> > > change the brightness? If they actually change the brightness, we 
> > > shouldn't send input events.
> > > 
> > 
> > Yes, hardware and bios change brightness by themselves without software intervention
> > on my Eee PC 1005 when pressing the hotkeys.
> 
> Ok. In that case, you shouldn't send input events. Once backlight 
> control is implemented in the eee-wmi driver you can send notifications 
> via that instead.
> 

Got it. Backlight is on my TODO list and I plan to add those features
incrementally so that people could starting playing with it asap since
I've only got a 1005 and I am not sure how it works on other make and
models. Make sense?

-Yong

Re: [PATCH] eeepc-wmi: new driver for WMI based hotkeys on Eee PC laptops

[Yong Wang]

On Fri, Mar 19, 2010 at 03:23:23PM +0000, Matthew Garrett wrote:
> 
> > > > +		if (code >= NOTIFY_BRNUP_MIN && code <= NOTIFY_BRNUP_MAX)
> > > > +			code = NOTIFY_BRNUP_MIN;
> > > > +		else if (code >= NOTIFY_BRNDOWN_MIN && code <= NOTIFY_BRNDOWN_MAX)
> > > > +			code = NOTIFY_BRNDOWN_MIN;
> > > 
> > > Do the brightness keys just send notifications, or do they actually 
> > > change the brightness? If they actually change the brightness, we 
> > > shouldn't send input events.
> > > 
> > 
> > Yes, hardware and bios change brightness by themselves without software intervention
> > on my Eee PC 1005 when pressing the hotkeys.
> 
> Ok. In that case, you shouldn't send input events. Once backlight 
> control is implemented in the eee-wmi driver you can send notifications 
> via that instead.
> 

One question just popped off the top my head. What if there is a power
applet that wants to display a slider field at the bottom of the screen
showing the current brightness real time whenever users press brightness
hotkeys? Shouldn't it listen to the standard input events translated by
X into standard XF86 keysyms? Or shall it listen to the ACPI backlight
events? If so, it is the ACPI LCD event when using acpi backlight
driver. But what if those vendor specific backlight drivers are used?

Thanks
-Yong

Re: [PATCH] eeepc-wmi: new driver for WMI based hotkeys on Eee PC laptops

[cascardo]

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

On Sat, Mar 20, 2010 at 08:55:53AM +0800, Yong Wang wrote:
> On Fri, Mar 19, 2010 at 03:23:23PM +0000, Matthew Garrett wrote:
> > 
> > > > > +		if (code >= NOTIFY_BRNUP_MIN && code <= NOTIFY_BRNUP_MAX)
> > > > > +			code = NOTIFY_BRNUP_MIN;
> > > > > +		else if (code >= NOTIFY_BRNDOWN_MIN && code <= NOTIFY_BRNDOWN_MAX)
> > > > > +			code = NOTIFY_BRNDOWN_MIN;
> > > > 
> > > > Do the brightness keys just send notifications, or do they actually 
> > > > change the brightness? If they actually change the brightness, we 
> > > > shouldn't send input events.
> > > > 
> > > 
> > > Yes, hardware and bios change brightness by themselves without software intervention
> > > on my Eee PC 1005 when pressing the hotkeys.
> > 
> > Ok. In that case, you shouldn't send input events. Once backlight 
> > control is implemented in the eee-wmi driver you can send notifications 
> > via that instead.
> > 
> 
> One question just popped off the top my head. What if there is a power
> applet that wants to display a slider field at the bottom of the screen
> showing the current brightness real time whenever users press brightness
> hotkeys? Shouldn't it listen to the standard input events translated by
> X into standard XF86 keysyms? Or shall it listen to the ACPI backlight
> events? If so, it is the ACPI LCD event when using acpi backlight
> driver. But what if those vendor specific backlight drivers are used?
> 
> Thanks
> -Yong
> --

You may select/poll for the sysfs file actual_brightness. It will return
POLLPRI. Basically, backlight devices end up calling sysfs_notify that
will allow sysfs_poll to work. Read the comments about sysfs_poll at
fs/sysfs/file.c.

You should either use backlight_force_update in your driver or let the
user update it writing to the brightness file. In your case, I'd say you
should use backlight_force_update and give BACKLIGHT_UPDATE_HOTKEY as
the reason.

Regards,
Cascardo.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] eeepc-wmi: new driver for WMI based hotkeys on Eee PC laptops

[Yong Wang]

On Sat, Mar 20, 2010 at 09:20:50AM -0300, cascardo@holoscopio.com wrote:
> On Sat, Mar 20, 2010 at 08:55:53AM +0800, Yong Wang wrote:
> > 
> > One question just popped off the top my head. What if there is a power
> > applet that wants to display a slider field at the bottom of the screen
> > showing the current brightness real time whenever users press brightness
> > hotkeys? Shouldn't it listen to the standard input events translated by
> > X into standard XF86 keysyms? Or shall it listen to the ACPI backlight
> > events? If so, it is the ACPI LCD event when using acpi backlight
> > driver. But what if those vendor specific backlight drivers are used?
> > 
> > Thanks
> > -Yong
> > --
> 
> You may select/poll for the sysfs file actual_brightness. It will return
> POLLPRI. Basically, backlight devices end up calling sysfs_notify that
> will allow sysfs_poll to work. Read the comments about sysfs_poll at
> fs/sysfs/file.c.
> 
> You should either use backlight_force_update in your driver or let the
> user update it writing to the brightness file. In your case, I'd say you
> should use backlight_force_update and give BACKLIGHT_UPDATE_HOTKEY as
> the reason.
> 

Oh, I see. Thank for clarifying, Cascardo.

-Yong

Re: [PATCH] eeepc-wmi: new driver for WMI based hotkeys on Eee PC laptops

[Corentin Chary]

>> You should either use backlight_force_update in your driver or let the
>> user update it writing to the brightness file. In your case, I'd say you
>> should use backlight_force_update and give BACKLIGHT_UPDATE_HOTKEY as
>> the reason.
>>
>
> Oh, I see. Thank for clarifying, Cascardo.
>
> -Yong
>

Hi Yong,

You can check how eeepc-laptop work for backlight stuff.
I think you should also remove all the NOTIFY_BRNDOWN_* code. It's
still in eeepc-laptop for backward compatibility, but it should not be
i newer drivers.

Anyway, most of the time the backlight will be handled by the
acpi/video.ko driver which will
send events itself.

I will update this page soon
http://dev.iksaif.net/projects/acpi4asus/wiki/Eeepc-wmi .

Do you want this driver to be part of acpi4asus project ? It would
allow to share the same git tree / bugtracker / wiki. I can give you
access to all that stuff quickly. I think it would be easier for users
if we merge into one single project.

Thanks for you work.

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

Re: [PATCH] eeepc-wmi: new driver for WMI based hotkeys on Eee PC laptops

[Yong Wang]

On Sat, Mar 20, 2010 at 06:21:42PM +0100, Corentin Chary wrote:
> 
> Hi Yong,
> 
> You can check how eeepc-laptop work for backlight stuff.
> I think you should also remove all the NOTIFY_BRNDOWN_* code. It's
> still in eeepc-laptop for backward compatibility, but it should not be
> i newer drivers.
> 
> Anyway, most of the time the backlight will be handled by the
> acpi/video.ko driver which will
> send events itself.
> 

Thanks for the info. I will take a look.

> I will update this page soon
> http://dev.iksaif.net/projects/acpi4asus/wiki/Eeepc-wmi .
> 

Yeah, I actually passed by this wiki page last week and I read the "how
to help" section. I cannot donate hardware so I wrote the driver;-)

> Do you want this driver to be part of acpi4asus project ? It would
> allow to share the same git tree / bugtracker / wiki. I can give you
> access to all that stuff quickly. I think it would be easier for users
> if we merge into one single project.
> 

I have no problem. What is the relationship between acpi4asus and
platform x86 drivers? Will Matthew merge your tree?

Thanks
-Yong

Re: [PATCH] eeepc-wmi: new driver for WMI based hotkeys on Eee PC laptops

[Corentin Chary]

On Sun, Mar 21, 2010 at 1:59 AM, Yong Wang <yong.y.wang@linux.intel.com> wrote:
> On Sat, Mar 20, 2010 at 06:21:42PM +0100, Corentin Chary wrote:
>>
>> Hi Yong,
>>
>> You can check how eeepc-laptop work for backlight stuff.
>> I think you should also remove all the NOTIFY_BRNDOWN_* code. It's
>> still in eeepc-laptop for backward compatibility, but it should not be
>> i newer drivers.
>>
>> Anyway, most of the time the backlight will be handled by the
>> acpi/video.ko driver which will
>> send events itself.
>>
>
> Thanks for the info. I will take a look.
>
>> I will update this page soon
>> http://dev.iksaif.net/projects/acpi4asus/wiki/Eeepc-wmi .
>>
>
> Yeah, I actually passed by this wiki page last week and I read the "how
> to help" section. I cannot donate hardware so I wrote the driver;-)
>
>> Do you want this driver to be part of acpi4asus project ? It would
>> allow to share the same git tree / bugtracker / wiki. I can give you
>> access to all that stuff quickly. I think it would be easier for users
>> if we merge into one single project.
>>
>
> I have no problem. What is the relationship between acpi4asus and
> platform x86 drivers? Will Matthew merge your tree?

Yep Matthew will merge my tree. Platform x86 subsystem is young, so it
only happened once, but I think
he will continue to do so. Do you want a write access to acpi4asus
tree, or do you want me to merge your tree ?

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

Re: [PATCH] eeepc-wmi: new driver for WMI based hotkeys on Eee PC laptops

[Yong Wang]

On Sun, Mar 21, 2010 at 02:14:18PM +0100, Corentin Chary wrote:
> 
> Yep Matthew will merge my tree. Platform x86 subsystem is young, so it
> only happened once, but I think
> he will continue to do so. Do you want a write access to acpi4asus
> tree, or do you want me to merge your tree ?
> 

I don't have a public tree and I don't think I need write access to your
tree. I will continue to send out patches when I have time to work on
it. You can pick them up into your tree if you think they look ok to
you. Is that OK?

Thanks
-Yong

Re: [PATCH] eeepc-wmi: new driver for WMI based hotkeys on Eee PC laptops

[Corentin Chary]

On Sun, Mar 21, 2010 at 2:35 PM, Yong Wang <yong.y.wang@linux.intel.com> wrote:
> On Sun, Mar 21, 2010 at 02:14:18PM +0100, Corentin Chary wrote:
>>
>> Yep Matthew will merge my tree. Platform x86 subsystem is young, so it
>> only happened once, but I think
>> he will continue to do so. Do you want a write access to acpi4asus
>> tree, or do you want me to merge your tree ?
>>
>
> I don't have a public tree and I don't think I need write access to your
> tree. I will continue to send out patches when I have time to work on
> it. You can pick them up into your tree if you think they look ok to
> you. Is that OK?
>
> Thanks
> -Yong
>

Yep,

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