From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Yu C" Subject: Re: [PATCH] surface pro 3: Add support driver for Surface Pro 3 buttons 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> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <1438839022.2679.49.camel@perches.com> Content-Language: en-US Content-ID: Sender: linux-kernel-owner@vger.kernel.org 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" List-Id: platform-driver-x86.vger.kernel.org VGhhbmtzIEpvZSwNCk9uIFdlZCwgMjAxNS0wOC0wNSBhdCAyMjozMCAtMDcwMCwgSm9lIFBlcmNo ZXMgd3JvdGU6DQo+IE9uIFRodSwgMjAxNS0wOC0wNiBhdCAxMzoxNiArMDgwMCwgQ2hlbiBZdSB3 cm90ZToNCj4gPiBTaW5jZSBTdXJmYWNlIFBybyAzIGRvZXMgbm90IGZvbGxvdyB0aGUgc3BlY3Mg b2YgIldpbmRvd3MgQUNQSSBEZXNpZ24NCj4gPiBHdWlkZSBmb3IgU29DIFBsYXRmb3JtIiwgY29k ZSBpbiBkcml2ZXJzL2lucHV0L21pc2Mvc29jX2FycmF5LmMgY2FuDQo+ID4gbm90IGRldGVjdCB0 aGVzZSBidXR0b25zIG9uIGl0Lg0KPiANCj4gc3R5bGUgdHJpdmlhOg0KPiANCj4gPiBkaWZmIC0t Z2l0IGEvZHJpdmVycy9wbGF0Zm9ybS94ODYvc3VyZmFjZXBybzNfYnV0dG9uLmMgYi9kcml2ZXJz L3BsYXRmb3JtL3g4Ni9zdXJmYWNlcHJvM19idXR0b24uYw0KPiBbXQ0KPiA+ICtzdGF0aWMgdm9p ZCBzdXJmYWNlX2J1dHRvbl9ub3RpZnkoc3RydWN0IGFjcGlfZGV2aWNlICpkZXZpY2UsIHUzMiBl dmVudCkNCj4gPiArew0KPiBbXQ0KPiA+ICsJc3dpdGNoIChldmVudCkgew0KPiA+ICsJY2FzZSBT VVJGQUNFX0JVVFRPTl9OT1RJRllfUFJFU1NfUE9XRVI6DQo+ID4gKwkJcHJlc3NlZCA9IHRydWU7 DQo+ID4gKwkJLypnbyB0aHJvdWdoKi8NCj4gDQo+IC8qIGZhbGwgdGhyb3VnaCAqLyBpcyBtb3Jl IGNvbW1vbg0KPiANCk9LLg0KPiA+ICsJY2FzZSBTVVJGQUNFX0JVVFRPTl9OT1RJRllfUFJFU1Nf SE9NRToNCj4gPiArCQlwcmVzc2VkID0gdHJ1ZTsNCj4gPiArCWNhc2UgU1VSRkFDRV9CVVRUT05f Tk9USUZZX1JFTEVBU0VfSE9NRToNCj4gPiArCQlrZXlfY29kZSA9IEtFWV9MRUZUTUVUQTsNCj4g PiArCQlicmVhazsNCj4gDQo+IEl0IG1heSBiZSBiZXR0ZXIgdG8gYWRkIGEgY29tbWVudCBhYm91 dCB0aGUgc3R5bGUgb3INCj4gbWF5YmUgYWRkIGEgbWFjcm8gbGlrZQ0KPiANCj4gI2RlZmluZSBI QU5ETEVfU1VSRkFDRV9CVVRUT05fTk9USUZZKHR5cGUsIGNvZGUpCVwNCj4gCWNhc2UgU1VSRkFD RV9CVVRUT05fTk9USUZZX1BSRVNTXyMjdHlwZToJXA0KPiAJCXByZXNzZWQgPSB0cnVlOwkvKiBh bmQgZmFsbC10aHJvdWdoICovCVwNCj4gCWNhc2UgU1VSRkFDRV9CVVRUT05fTk9USUZZX1JFTEVB U0VfIyN0eXBlOglcDQo+IAkJa2V5X2NvZGUgPSBjb2RlOwkJCVwNCj4gCQlicmVhazsNCj4gDQpX UlQgbWFjcm8gSEFORExFX1NVUkZBQ0VfQlVUVE9OX05PVElGWSwgdGhlIGNoZWNrcGF0Y2gucGwN CmNvbXBsYWlucyB0aGF0IG11bHRpIGxpbmVzIG9mIGNvZGVzIHNob3VsZCBiZSB3cmFwcGVkIGlu ICdkbw0Kd2hpbGUnc3RhdGUsIGJ1dCBkb2luZyBsaWtlIHRoaXMgbWlnaHQgbGVhZCB0byBpbmNv cnJlY3Qgc2VtYW50aWMuDQpJcyBpdCBvayB0byBrZWVwIHRoZXNlIGNvZGVzIGFuZCBhZGQgY29t bWVudHMgbGlrZToNCi8qDQogKiBXaGVuIGEgYnV0dG9uKHBvd2VyIGJ1dHRvbi92b2x1bWUgYnV0 dG9uL2hvbWUgYnV0dG9uKSBpcyANCiAqIHByZXNzZWQgZG93biBvciByZWxlYXNlZCwgZGlmZmVy ZW50IEFDUEkgbm90aWZpY2F0aW9uIGNvZGVzIA0KICogd2lsbCBiZSBnZW5lcmF0ZWQuIFdlIGNh biBkaXN0aW5ndWlzaCBkaWZmZXJlbnQgZXZlbnQgY29kZSANCiAqIGFuZCB2YWx1ZSBvZiBidXR0 b25zIGJ5IHRoZXNlIG5vdGlmaWNhdGlvbiBjb2RlcywgdGhlbiBwYXNzDQogKiAoRVZfS0VZLCBl dmVudCBjb2RlKGtleV9jb2RlKSwgdmFsdWUocHJlc3NlZCkpIHRvIGlucHV0IGxheWVyLg0KICov DQoNCj4gPiArCWNhc2UgU1VSRkFDRV9CVVRUT05fTk9USUZZX1BSRVNTX1ZPTFVNRV9VUDoNCj4g PiArCQlwcmVzc2VkID0gdHJ1ZTsNCj4gPiArCWNhc2UgU1VSRkFDRV9CVVRUT05fTk9USUZZX1JF TEVBU0VfVk9MVU1FX1VQOg0KPiA+ICsJCWtleV9jb2RlID0gS0VZX1ZPTFVNRVVQOw0KPiA+ICsJ CWJyZWFrOw0KPiANCj4gPiArCWNhc2UgU1VSRkFDRV9CVVRUT05fTk9USUZZX1BSRVNTX1ZPTFVN RV9ET1dOOg0KPiA+ICsJCXByZXNzZWQgPSB0cnVlOw0KPiA+ICsJY2FzZSBTVVJGQUNFX0JVVFRP Tl9OT1RJRllfUkVMRUFTRV9WT0xVTUVfRE9XTjoNCj4gPiArCQlrZXlfY29kZSA9IEtFWV9WT0xV TUVET1dOOw0KPiA+ICsJCWJyZWFrOw0KPiA+ICsJZGVmYXVsdDoNCj4gPiArCQlkZXZfaW5mbygm ZGV2aWNlLT5kZXYsDQo+ID4gKwkJCQkgICJVbnN1cHBvcnRlZCBldmVudCBbMHgleF1cbiIsIGV2 ZW50KTsNCj4gDQo+IEl0IG1pZ2h0IGJlIHVzZWZ1bCB0byByYXRlbGltaXQgdGhpcw0KPiANCk9L LCBjaGFuZ2VkIHRvIGRldl9pbmZvX3JhdGVsaW1pdGVkDQo+IA0KDQpCZXN0IFJlZ2FyZHMsDQpZ dQ0KDQo= 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