ac100 team mailing list archive

[Julian Andres Klode <jak@xxxxxxxxxxxxx>]

On Sun, Oct 23, 2011 at 04:13:31AM +0200, Pierre-Hugues Husson wrote:
>  Delete PM-related uncommented calls from nvec_kbd
>  Make wakeup events configurable through /sys/module/nvec/parameters/wakeup_on_*

Write a better commit message, you're missing punctuation. And really split
out the changes to nvec_kbd, they do not really belong here.

> 
> Signed-off-by: Pierre-Hugues Husson <phhusson@xxxxxxxxx> ---
>  drivers/staging/nvec/nvec.c     |   42 +++++++++++++++++++++++++++++++++++++++
>  drivers/staging/nvec/nvec_kbd.c |    5 +--
>  2 files changed, 44 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c
> index 52af8ec..758a558 100644
> --- a/drivers/staging/nvec/nvec.c
> +++ b/drivers/staging/nvec/nvec.c
> @@ -795,6 +795,45 @@ static int __devexit tegra_nvec_remove(struct platform_device *pdev)
>  
>  #ifdef CONFIG_PM
>  
> +static int wakeup_on_lid = 1;
> +static int wakeup_on_power_button = 1;

I don't think that we need to have this conditional. Just always
wakeup on power button presses.


> +static int wakeup_on_any_key = 0;

Do not initialise variables to 0.

> +static int wakeup_on_home_key = 1;

Is this used under Android? If not, it should
not be 1.

> +module_param(wakeup_on_lid, int, 0644);
> +module_param(wakeup_on_power_button, int, 0644);
> +module_param(wakeup_on_any_key, int, 0644);
> +module_param(wakeup_on_home_key, int, 0644);
> +
> +static void tegra_nvec_setup_wakeup_events(struct nvec_chip *nvec) {
> +	{
> +		//Resume on LID
> +		char cmd[]={ 0x01, 0xFD, 0x01, 0x00, 0x00, 0x02, 0x00};
> +		cmd[2]=!!wakeup_on_lid;
> +		nvec_write_async(nvec, cmd, sizeof(cmd));
> +	}
> +
> +	{
> +		//Resume on Power button
> +		char cmd[]={ 0x01, 0xFD, 0x01, 0x00, 0x00, 0x01, 0x00};
> +		cmd[2]=!!wakeup_on_power_button;
> +		nvec_write_async(nvec, cmd, sizeof(cmd));
> +	}
> +
> +	{
> +		//Resume on any key press
> +		char cmd[]={ 0x05, 0x03, 0x01, 0x01};
> +		cmd[2]=!!wakeup_on_any_key;
> +		nvec_write_async(nvec, cmd, sizeof(cmd));
> +	}
> +
> +	{
> +		//Resume on home key press
> +		char cmd[]={ 0x05, 0x03, 0x01, 0x02};
> +		cmd[2]=!!wakeup_on_home_key;
> +		nvec_write_async(nvec, cmd, sizeof(cmd));
> +	}
> +}
> +

See what Marc wrote.

>  static int tegra_nvec_suspend(struct platform_device *pdev, pm_message_t state)
>  {
>  	struct nvec_chip *nvec = platform_get_drvdata(pdev);
> @@ -803,12 +842,15 @@ static int tegra_nvec_suspend(struct platform_device *pdev, pm_message_t state)
>  	dev_dbg(nvec->dev, "suspending\n");
>  	nvec_write_async(nvec, "\x0d\x10\x59\x94", 4);
>  
> +	tegra_nvec_setup_wakeup_events(nvec);
> +
>  	/* keep these sync or you'll break suspend */
>  	msg = nvec_write_sync(nvec, EC_DISABLE_EVENT_REPORTING, 3);
>  	nvec_msg_free(nvec, msg);
>  	msg = nvec_write_sync(nvec, "\x04\x02", 2);
>  	nvec_msg_free(nvec, msg);
>  
> +

That line does not belong here, there is already one empty line, and
one empty line is definitely enough for structuring code.

>  	nvec_disable_i2c_slave(nvec);
>  
>  	return 0;
> diff --git a/drivers/staging/nvec/nvec_kbd.c b/drivers/staging/nvec/nvec_kbd.c
> index a4ce5a7..c599f6e7 100644
> --- a/drivers/staging/nvec/nvec_kbd.c
> +++ b/drivers/staging/nvec/nvec_kbd.c
> @@ -140,10 +140,9 @@ static int __devinit nvec_kbd_probe(struct platform_device *pdev)
>  	/* Enable keyboard */
>  	nvec_write_async(nvec, "\x05\xf4", 2);
>  
> -	/* keyboard reset? */
> -	nvec_write_async(nvec, "\x05\x03\x01\x01", 4);
> -	nvec_write_async(nvec, "\x05\x04\x01", 3);
> +	//Send 0xff (reset keyboard) on PS2 bus
>  	nvec_write_async(nvec, "\x06\x01\xff\x03", 4);
> +
>  /*	FIXME
>  	wait until keyboard reset is finished or until we have a sync write */ --

Split that out into one or two separate commits, and use standard (old) C
comments.


-- 
Julian Andres Klode  - Debian Developer, Ubuntu Member

See http://wiki.debian.org/JulianAndresKlode and http://jak-linux.org/.

Attachment: pgp4SvYehtnC1.pgp
Description: PGP signature