From patchwork Thu Apr 20 20:31:39 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Darren Hart X-Patchwork-Id: 9691235 X-Patchwork-Delegate: dvhart@infradead.org Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id C72C46037F for ; Thu, 20 Apr 2017 20:31:45 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id B77B228480 for ; Thu, 20 Apr 2017 20:31:45 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id A97DC2847C; Thu, 20 Apr 2017 20:31:45 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id AF09E2847C for ; Thu, 20 Apr 2017 20:31:44 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1031891AbdDTUbn (ORCPT ); Thu, 20 Apr 2017 16:31:43 -0400 Received: from bombadil.infradead.org ([65.50.211.133]:53060 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1031651AbdDTUbm (ORCPT ); Thu, 20 Apr 2017 16:31:42 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=bombadil.20170209; h=In-Reply-To:Content-Type:MIME-Version :References:Message-ID:Subject:To:From:Date:Sender:Reply-To:Cc: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=CF4qiVEB21A1+VOM0pGAcPYdmcP9hQOZMQgfzzpcOJg=; b=ZdVbq7HhY8d4GNzeoyiqWvR7t YRPtXQMR+HIIGAXdzZSmdTrr6WfRvfBrOoYDQWtG5yiXhcbuEQK7uARNvluVNJ4vmULMvNIw3Pgvi yzkfB2k00rfAhAD/+PGWb6uq7JrLlOBGnIQwp2FMOpt6OXIWRaxE/skjvpyz8+qMh0pm6idaG7fo1 9bokJaSD6Rvq4h5rJ0pToERxIjJx2bV5U7v7wCEF/Xj3fQmLJq/TC6OW4jPlNadhzJ/E5NWigXvc2 hG/XxrC0GQWFYRyjlOtkVXJT+nOzowtyuj8AowIxbjTvoPOqw1JHl/TL/Nn/fCX4hnkvOHe73txw8 RGG9d0obw==; Received: from dvhart by bombadil.infradead.org with local (Exim 4.87 #1 (Red Hat Linux)) id 1d1Ijt-0007zy-0n; Thu, 20 Apr 2017 20:31:41 +0000 Date: Thu, 20 Apr 2017 13:31:39 -0700 From: Darren Hart To: andy@infradead.org, platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org, carlo@caione.org Subject: Re: [PATCH 3/9] platform/x86: hp-wmi: Standardize enum usage for constants Message-ID: <20170420203139.GB3209@fury> References: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.7.1 (2016-10-04) Sender: platform-driver-x86-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: platform-driver-x86@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Wed, Apr 19, 2017 at 07:25:15PM -0700, Darren Hart wrote: > From: "Darren Hart (VMware)" > > Use enums consistently throughout the hp-wmi driver for groups of > related constants. Use hex and align the assignment within groups. Move > the *QUERY constants into an enum, create a new enum defining the READ, > WRITE, and ODM constants and use them instead of 0 and 1 at the call > sites. > > Signed-off-by: Darren Hart (VMware) > --- > drivers/platform/x86/hp-wmi.c | 119 +++++++++++++++++++++++------------------- > 1 file changed, 64 insertions(+), 55 deletions(-) > > diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c > index aa9d99c..60d1e4c 100644 > --- a/drivers/platform/x86/hp-wmi.c > +++ b/drivers/platform/x86/hp-wmi.c ... > +enum hp_wmi_command { > + HPWMI_READ = 0x00, > + HPWMI_WRITE = 0x01, > + HPWMI_ODM = 0x03, > +}; > + > #define BIOS_ARGS_INIT(write, ctype, size) \ > (struct bios_args) { .signature = 0x55434553, \ > .command = (write) ? 0x2 : 0x1, \ > @@ -177,8 +186,8 @@ static struct rfkill2_device rfkill2[HPWMI_MAX_RFKILL2_DEVICES]; > /* > * hp_wmi_perform_query > * > - * query: The commandtype -> What should be queried > - * write: The command -> 0 read, 1 write, 3 ODM specific > + * query: The commandtype (enum hp_wmi_commandtype) > + * write: The command (enum hp_wmi_command) Eeek, I failed to use the enum directly and was still using the ternary operator in the BIOS_ARGS_INIT above. Luckily, with read as 0 and write as non-zero in the new ENUMs, and no usage of ODM, things happened to just work. Updated patch below. I've now: * dropped the BIOS_ARGS_INIT per Andy's recommendation. * Renumbered the hp_wmi_command to match the ternary assignment: HPWMI_READ = 0x01, HPWMI_WRITE = 0x02, * Assigned bios_args.command with one of the enums directly * replaced the "int write" argument with "enum hp_wmi_command command" As follows: Queued to testing, please let me know if you have any concerns. From d8193cff33906e24ac1830ecb2ca95833d058a3a Mon Sep 17 00:00:00 2001 Message-Id: From: "Darren Hart (VMware)" Date: Wed, 19 Apr 2017 15:42:01 -0700 Subject: [PATCH] platform/x86: hp-wmi: Standardize enum usage for constants Use enums consistently throughout the hp-wmi driver for groups of related constants. Use hex and align the assignment within groups. Move the *QUERY constants into an enum, create a new enum defining the READ, WRITE, and ODM constants and use them instead of 0 and 1 at the call sites. Set the command directly instead of using the ternary operator since both 1 and 3 as previously documented would result in the command being set to 0x2. Signed-off-by: Darren Hart (VMware) Tested-by: Carlo Caione Reviewed-by: Andy Shevchenko --- drivers/platform/x86/hp-wmi.c | 132 +++++++++++++++++++++++------------------- 1 file changed, 74 insertions(+), 58 deletions(-) diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c index e772105..2073f1e 100644 --- a/drivers/platform/x86/hp-wmi.c +++ b/drivers/platform/x86/hp-wmi.c @@ -48,41 +48,29 @@ MODULE_ALIAS("wmi:5FB7F034-2C63-45e9-BE91-3D44E2C707E4"); #define HPWMI_EVENT_GUID "95F24279-4D7B-4334-9387-ACCDC67EF61C" #define HPWMI_BIOS_GUID "5FB7F034-2C63-45e9-BE91-3D44E2C707E4" -#define HPWMI_DISPLAY_QUERY 0x1 -#define HPWMI_HDDTEMP_QUERY 0x2 -#define HPWMI_ALS_QUERY 0x3 -#define HPWMI_HARDWARE_QUERY 0x4 -#define HPWMI_WIRELESS_QUERY 0x5 -#define HPWMI_BIOS_QUERY 0x9 -#define HPWMI_FEATURE_QUERY 0xb -#define HPWMI_HOTKEY_QUERY 0xc -#define HPWMI_FEATURE2_QUERY 0xd -#define HPWMI_WIRELESS2_QUERY 0x1b -#define HPWMI_POSTCODEERROR_QUERY 0x2a - enum hp_wmi_radio { - HPWMI_WIFI = 0, - HPWMI_BLUETOOTH = 1, - HPWMI_WWAN = 2, - HPWMI_GPS = 3, + HPWMI_WIFI = 0x0, + HPWMI_BLUETOOTH = 0x1, + HPWMI_WWAN = 0x2, + HPWMI_GPS = 0x3, }; enum hp_wmi_event_ids { - HPWMI_DOCK_EVENT = 1, - HPWMI_PARK_HDD = 2, - HPWMI_SMART_ADAPTER = 3, - HPWMI_BEZEL_BUTTON = 4, - HPWMI_WIRELESS = 5, - HPWMI_CPU_BATTERY_THROTTLE = 6, - HPWMI_LOCK_SWITCH = 7, - HPWMI_LID_SWITCH = 8, - HPWMI_SCREEN_ROTATION = 9, - HPWMI_COOLSENSE_SYSTEM_MOBILE = 0x0A, - HPWMI_COOLSENSE_SYSTEM_HOT = 0x0B, - HPWMI_PROXIMITY_SENSOR = 0x0C, - HPWMI_BACKLIT_KB_BRIGHTNESS = 0x0D, - HPWMI_PEAKSHIFT_PERIOD = 0x0F, - HPWMI_BATTERY_CHARGE_PERIOD = 0x10, + HPWMI_DOCK_EVENT = 0x01, + HPWMI_PARK_HDD = 0x02, + HPWMI_SMART_ADAPTER = 0x03, + HPWMI_BEZEL_BUTTON = 0x04, + HPWMI_WIRELESS = 0x05, + HPWMI_CPU_BATTERY_THROTTLE = 0x06, + HPWMI_LOCK_SWITCH = 0x07, + HPWMI_LID_SWITCH = 0x08, + HPWMI_SCREEN_ROTATION = 0x09, + HPWMI_COOLSENSE_SYSTEM_MOBILE = 0x0A, + HPWMI_COOLSENSE_SYSTEM_HOT = 0x0B, + HPWMI_PROXIMITY_SENSOR = 0x0C, + HPWMI_BACKLIT_KB_BRIGHTNESS = 0x0D, + HPWMI_PEAKSHIFT_PERIOD = 0x0F, + HPWMI_BATTERY_CHARGE_PERIOD = 0x10, }; struct bios_args { @@ -93,6 +81,34 @@ struct bios_args { u32 data; }; +enum hp_wmi_commandtype { + HPWMI_DISPLAY_QUERY = 0x01, + HPWMI_HDDTEMP_QUERY = 0x02, + HPWMI_ALS_QUERY = 0x03, + HPWMI_HARDWARE_QUERY = 0x04, + HPWMI_WIRELESS_QUERY = 0x05, + HPWMI_BATTERY_QUERY = 0x07, + HPWMI_BIOS_QUERY = 0x09, + HPWMI_FEATURE_QUERY = 0x0b, + HPWMI_HOTKEY_QUERY = 0x0c, + HPWMI_FEATURE2_QUERY = 0x0d, + HPWMI_WIRELESS2_QUERY = 0x1b, + HPWMI_POSTCODEERROR_QUERY = 0x2a, +}; + +enum hp_wmi_command { + HPWMI_READ = 0x01, + HPWMI_WRITE = 0x02, + HPWMI_ODM = 0x03, +}; + +#define BIOS_ARGS_INIT(write, ctype, size) \ + (struct bios_args) { .signature = 0x55434553, \ + .command = (write) ? 0x2 : 0x1, \ + .commandtype = (ctype), \ + .datasize = (size), \ + .data = 0 } + struct bios_return { u32 sigpass; u32 return_code; @@ -170,8 +186,8 @@ static struct rfkill2_device rfkill2[HPWMI_MAX_RFKILL2_DEVICES]; /* * hp_wmi_perform_query * - * query: The commandtype -> What should be queried - * write: The command -> 0 read, 1 write, 3 ODM specific + * query: The commandtype (enum hp_wmi_commandtype) + * write: The command (enum hp_wmi_command) * buffer: Buffer used as input and/or output * insize: Size of input buffer * outsize: Size of output buffer @@ -182,20 +198,20 @@ static struct rfkill2_device rfkill2[HPWMI_MAX_RFKILL2_DEVICES]; * -EINVAL if the output buffer size exceeds buffersize * * Note: The buffersize must at least be the maximum of the input and output - * size. E.g. Battery info query (0x7) is defined to have 1 byte input + * size. E.g. Battery info query is defined to have 1 byte input * and 128 byte output. The caller would do: * buffer = kzalloc(128, GFP_KERNEL); - * ret = hp_wmi_perform_query(0x7, 0, buffer, 1, 128) + * ret = hp_wmi_perform_query(HPWMI_BATTERY_QUERY, HPWMI_READ, buffer, 1, 128) */ -static int hp_wmi_perform_query(int query, int write, void *buffer, - int insize, int outsize) +static int hp_wmi_perform_query(int query, enum hp_wmi_command command, + void *buffer, int insize, int outsize) { struct bios_return *bios_return; int actual_outsize; union acpi_object *obj; struct bios_args args = { .signature = 0x55434553, - .command = write ? 0x2 : 0x1, + .command = command, .commandtype = query, .datasize = insize, .data = 0, @@ -245,7 +261,7 @@ static int hp_wmi_perform_query(int query, int write, void *buffer, static int hp_wmi_display_state(void) { int state = 0; - int ret = hp_wmi_perform_query(HPWMI_DISPLAY_QUERY, 0, &state, + int ret = hp_wmi_perform_query(HPWMI_DISPLAY_QUERY, HPWMI_READ, &state, sizeof(state), sizeof(state)); if (ret) return ret < 0 ? ret : -EINVAL; @@ -255,7 +271,7 @@ static int hp_wmi_display_state(void) static int hp_wmi_hddtemp_state(void) { int state = 0; - int ret = hp_wmi_perform_query(HPWMI_HDDTEMP_QUERY, 0, &state, + int ret = hp_wmi_perform_query(HPWMI_HDDTEMP_QUERY, HPWMI_READ, &state, sizeof(state), sizeof(state)); if (ret) return ret < 0 ? ret : -EINVAL; @@ -265,7 +281,7 @@ static int hp_wmi_hddtemp_state(void) static int hp_wmi_als_state(void) { int state = 0; - int ret = hp_wmi_perform_query(HPWMI_ALS_QUERY, 0, &state, + int ret = hp_wmi_perform_query(HPWMI_ALS_QUERY, HPWMI_READ, &state, sizeof(state), sizeof(state)); if (ret) return ret < 0 ? ret : -EINVAL; @@ -275,7 +291,7 @@ static int hp_wmi_als_state(void) static int hp_wmi_dock_state(void) { int state = 0; - int ret = hp_wmi_perform_query(HPWMI_HARDWARE_QUERY, 0, &state, + int ret = hp_wmi_perform_query(HPWMI_HARDWARE_QUERY, HPWMI_READ, &state, sizeof(state), sizeof(state)); if (ret) @@ -287,7 +303,7 @@ static int hp_wmi_dock_state(void) static int hp_wmi_tablet_state(void) { int state = 0; - int ret = hp_wmi_perform_query(HPWMI_HARDWARE_QUERY, 0, &state, + int ret = hp_wmi_perform_query(HPWMI_HARDWARE_QUERY, HPWMI_READ, &state, sizeof(state), sizeof(state)); if (ret) return ret < 0 ? ret : -EINVAL; @@ -298,7 +314,7 @@ static int hp_wmi_tablet_state(void) static int __init hp_wmi_bios_2008_later(void) { int state = 0; - int ret = hp_wmi_perform_query(HPWMI_FEATURE_QUERY, 0, &state, + int ret = hp_wmi_perform_query(HPWMI_FEATURE_QUERY, HPWMI_READ, &state, sizeof(state), sizeof(state)); if (!ret) return 1; @@ -309,7 +325,7 @@ static int __init hp_wmi_bios_2008_later(void) static int __init hp_wmi_bios_2009_later(void) { int state = 0; - int ret = hp_wmi_perform_query(HPWMI_FEATURE2_QUERY, 0, &state, + int ret = hp_wmi_perform_query(HPWMI_FEATURE2_QUERY, HPWMI_READ, &state, sizeof(state), sizeof(state)); if (!ret) return 1; @@ -320,7 +336,7 @@ static int __init hp_wmi_bios_2009_later(void) static int __init hp_wmi_enable_hotkeys(void) { int value = 0x6e; - int ret = hp_wmi_perform_query(HPWMI_BIOS_QUERY, 1, &value, + int ret = hp_wmi_perform_query(HPWMI_BIOS_QUERY, HPWMI_WRITE, &value, sizeof(value), 0); if (ret) return ret < 0 ? ret : -EINVAL; @@ -333,7 +349,7 @@ static int hp_wmi_set_block(void *data, bool blocked) int query = BIT(r + 8) | ((!blocked) << r); int ret; - ret = hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, 1, + ret = hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, HPWMI_WRITE, &query, sizeof(query), 0); if (ret) return ret < 0 ? ret : -EINVAL; @@ -349,7 +365,7 @@ static bool hp_wmi_get_sw_state(enum hp_wmi_radio r) int mask = 0x200 << (r * 8); int wireless = 0; - hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, 0, + hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, HPWMI_READ, &wireless, sizeof(wireless), sizeof(wireless)); /* TBD: Pass error */ @@ -365,7 +381,7 @@ static bool hp_wmi_get_hw_state(enum hp_wmi_radio r) int mask = 0x800 << (r * 8); int wireless = 0; - hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, 0, + hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, HPWMI_READ, &wireless, sizeof(wireless), sizeof(wireless)); /* TBD: Pass error */ @@ -381,7 +397,7 @@ static int hp_wmi_rfkill2_set_block(void *data, bool blocked) int rfkill_id = (int)(long)data; char buffer[4] = { 0x01, 0x00, rfkill_id, !blocked }; - if (hp_wmi_perform_query(HPWMI_WIRELESS2_QUERY, 1, + if (hp_wmi_perform_query(HPWMI_WIRELESS2_QUERY, HPWMI_WRITE, buffer, sizeof(buffer), 0)) return -EINVAL; return 0; @@ -396,7 +412,7 @@ static int hp_wmi_rfkill2_refresh(void) struct bios_rfkill2_state state; int err, i; - err = hp_wmi_perform_query(HPWMI_WIRELESS2_QUERY, 0, &state, + err = hp_wmi_perform_query(HPWMI_WIRELESS2_QUERY, HPWMI_READ, &state, 0, sizeof(state)); if (err) return err; @@ -423,7 +439,7 @@ static int hp_wmi_rfkill2_refresh(void) static int hp_wmi_post_code_state(void) { int state = 0; - int ret = hp_wmi_perform_query(HPWMI_POSTCODEERROR_QUERY, 0, &state, + int ret = hp_wmi_perform_query(HPWMI_POSTCODEERROR_QUERY, HPWMI_READ, &state, sizeof(state), sizeof(state)); if (ret) return ret < 0 ? ret : -EINVAL; @@ -489,7 +505,7 @@ static ssize_t set_als(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { u32 tmp = simple_strtoul(buf, NULL, 10); - int ret = hp_wmi_perform_query(HPWMI_ALS_QUERY, 1, &tmp, + int ret = hp_wmi_perform_query(HPWMI_ALS_QUERY, HPWMI_WRITE, &tmp, sizeof(tmp), sizeof(tmp)); if (ret) return ret < 0 ? ret : -EINVAL; @@ -510,7 +526,7 @@ static ssize_t set_postcode(struct device *dev, struct device_attribute *attr, /* Clear the POST error code. It is kept until until cleared. */ tmp = (u32) tmp2; - ret = hp_wmi_perform_query(HPWMI_POSTCODEERROR_QUERY, 1, &tmp, + ret = hp_wmi_perform_query(HPWMI_POSTCODEERROR_QUERY, HPWMI_WRITE, &tmp, sizeof(tmp), sizeof(tmp)); if (ret) return ret < 0 ? ret : -EINVAL; @@ -583,7 +599,7 @@ static void hp_wmi_notify(u32 value, void *context) case HPWMI_SMART_ADAPTER: break; case HPWMI_BEZEL_BUTTON: - ret = hp_wmi_perform_query(HPWMI_HOTKEY_QUERY, 0, + ret = hp_wmi_perform_query(HPWMI_HOTKEY_QUERY, HPWMI_READ, &key_code, sizeof(key_code), sizeof(key_code)); @@ -718,12 +734,12 @@ static int __init hp_wmi_rfkill_setup(struct platform_device *device) { int err, wireless = 0; - err = hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, 0, &wireless, + err = hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, HPWMI_READ, &wireless, sizeof(wireless), sizeof(wireless)); if (err) return err; - err = hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, 1, &wireless, + err = hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, HPWMI_WRITE, &wireless, sizeof(wireless), 0); if (err) return err; @@ -803,7 +819,7 @@ static int __init hp_wmi_rfkill2_setup(struct platform_device *device) struct bios_rfkill2_state state; int err, i; - err = hp_wmi_perform_query(HPWMI_WIRELESS2_QUERY, 0, &state, + err = hp_wmi_perform_query(HPWMI_WIRELESS2_QUERY, HPWMI_READ, &state, 0, sizeof(state)); if (err) return err;