linux-kernel - Re: [PATCH] surface pro 3: Add support driver for Surface Pro 3 buttons

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 6 Aug 2015 11:20:44 +0000
From:	"Chen, Yu C" <yu.c.chen@...el.com>
To:	"joe@...ches.com" <joe@...ches.com>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Zhang, Rui" <rui.zhang@...el.com>,
	"jslaby@...e.com" <jslaby@...e.com>,
	"dvhart@...radead.org" <dvhart@...radead.org>,
	"platform-driver-x86@...r.kernel.org" 
	<platform-driver-x86@...r.kernel.org>,
	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
	"Wysocki, Rafael J" <rafael.j.wysocki@...el.com>,
	"Westerberg, Mika" <mika.westerberg@...el.com>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"mchehab@....samsung.com" <mchehab@....samsung.com>,
	"arnd@...db.de" <arnd@...db.de>
Subject: Re: [PATCH] surface pro 3: Add support driver for Surface Pro 3
 buttons

Thanks Joe,
On Wed, 2015-08-05 at 22:30 -0700, Joe Perches wrote:
> On Thu, 2015-08-06 at 13:16 +0800, Chen Yu wrote:
> > Since Surface Pro 3 does not follow the specs of "Windows ACPI Design
> > Guide for SoC Platform", code in drivers/input/misc/soc_array.c can
> > not detect these buttons on it.
> 
> style trivia:
> 
> > diff --git a/drivers/platform/x86/surfacepro3_button.c b/drivers/platform/x86/surfacepro3_button.c
> []
> > +static void surface_button_notify(struct acpi_device *device, u32 event)
> > +{
> []
> > +	switch (event) {
> > +	case SURFACE_BUTTON_NOTIFY_PRESS_POWER:
> > +		pressed = true;
> > +		/*go through*/
> 
> /* fall through */ is more common
> 
OK.
> > +	case SURFACE_BUTTON_NOTIFY_PRESS_HOME:
> > +		pressed = true;
> > +	case SURFACE_BUTTON_NOTIFY_RELEASE_HOME:
> > +		key_code = KEY_LEFTMETA;
> > +		break;
> 
> It may be better to add a comment about the style or
> maybe add a macro like
> 
> #define HANDLE_SURFACE_BUTTON_NOTIFY(type, code)	\
> 	case SURFACE_BUTTON_NOTIFY_PRESS_##type:	\
> 		pressed = true;	/* and fall-through */	\
> 	case SURFACE_BUTTON_NOTIFY_RELEASE_##type:	\
> 		key_code = code;			\
> 		break;
> 
WRT macro HANDLE_SURFACE_BUTTON_NOTIFY, the checkpatch.pl
complains that multi lines of codes should be wrapped in 'do
while'state, but doing like this might lead to incorrect semantic.
Is it ok to keep these codes and add comments like:
/*
 * When a button(power button/volume button/home button) is 
 * pressed down or released, different ACPI notification codes 
 * will be generated. We can distinguish different event code 
 * and value of buttons by these notification codes, then pass
 * (EV_KEY, event code(key_code), value(pressed)) to input layer.
 */

> > +	case SURFACE_BUTTON_NOTIFY_PRESS_VOLUME_UP:
> > +		pressed = true;
> > +	case SURFACE_BUTTON_NOTIFY_RELEASE_VOLUME_UP:
> > +		key_code = KEY_VOLUMEUP;
> > +		break;
> 
> > +	case SURFACE_BUTTON_NOTIFY_PRESS_VOLUME_DOWN:
> > +		pressed = true;
> > +	case SURFACE_BUTTON_NOTIFY_RELEASE_VOLUME_DOWN:
> > +		key_code = KEY_VOLUMEDOWN;
> > +		break;
> > +	default:
> > +		dev_info(&device->dev,
> > +				  "Unsupported event [0x%x]\n", event);
> 
> It might be useful to ratelimit this
> 
OK, changed to dev_info_ratelimited
> 

Best Regards,
Yu