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

[Chen, Yu C]

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