From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755629AbbHFLUw (ORCPT ); Thu, 6 Aug 2015 07:20:52 -0400 Received: from mga09.intel.com ([134.134.136.24]:9630 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754579AbbHFLUu (ORCPT ); Thu, 6 Aug 2015 07:20:50 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.15,622,1432623600"; d="scan'208";a="763200716" From: "Chen, Yu C" To: "joe@perches.com" CC: "linux-kernel@vger.kernel.org" , "Zhang, Rui" , "jslaby@suse.com" , "dvhart@infradead.org" , "platform-driver-x86@vger.kernel.org" , "akpm@linux-foundation.org" , "Wysocki, Rafael J" , "Westerberg, Mika" , "gregkh@linuxfoundation.org" , "mchehab@osg.samsung.com" , "arnd@arndb.de" Subject: Re: [PATCH] surface pro 3: Add support driver for Surface Pro 3 buttons Thread-Topic: [PATCH] surface pro 3: Add support driver for Surface Pro 3 buttons Thread-Index: AQHQ0AkQXVPjpShW00KsPiAE1Q2ztJ3+TqwA Date: Thu, 6 Aug 2015 11:20:44 +0000 Message-ID: <1438860256.2127.37.camel@localhost> References: <1438838165-3791-1-git-send-email-yu.c.chen@intel.com> <1438839022.2679.49.camel@perches.com> In-Reply-To: <1438839022.2679.49.camel@perches.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.160.131] Content-Type: text/plain; charset="utf-8" Content-ID: MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id t76BKuIF014933 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 {.n++%ݶw{.n+{G{ayʇڙ,jfhz_(階ݢj"mG?&~iOzv^m ?I