linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 0/9] platform/x86: hp-wmi: Driver refactoring and cleanups
@ 2017-04-20  2:25 Darren Hart
  2017-04-20  2:25 ` [PATCH 1/9] platform/x86: hp-wmi: Cleanup local variable declarations Darren Hart
                   ` (10 more replies)
  0 siblings, 11 replies; 16+ messages in thread
From: Darren Hart @ 2017-04-20  2:25 UTC (permalink / raw)
  To: dvhart; +Cc: andy, platform-driver-x86, linux-kernel, carlo

From: "Darren Hart (VMware)" <dvhart@infradead.org>

This series factors out some redundant code, cleans up a number of style issues,
modernizes the sysfs usage, and cleans up the return paths. All told, the driver
is reduced in size by 37 lines (3.6%).

I do not have an HP laptop, so I'm hoping Carlo can help out with some testing.
In particular we need to verify that hotkeys and sysfs continue to work as
before.

This series is also available here for convenience:
git://git.infradead.org/users/dvhart/linux-platform-drivers-x86.git hp-wmi


Darren Hart (VMware) (9):
  platform/x86: hp-wmi: Cleanup local variable declarations
  platform/x86: hp-wmi: Add bios_args initializer
  platform/x86: hp-wmi: Standardize enum usage for constants
  platform/x86: hp-wmi: Refactor redundant HPWMI_READ functions
  platform/x86: hp-wmi: Cleanup wireless get_(hw|sw)state functions
  platform/x86: hp-wmi: Refactor dock and tablet state fetchers
  platform/x86: hp-wmi: Use DEVICE_ATTR_(RO|RW) helper macros
  platform/x86: hp-wmi: Do not shadow errors in sysfs show functions
  platform/x86: hp-wmi: Cleanup exit paths

 drivers/platform/x86/hp-wmi.c | 385 +++++++++++++++++++-----------------------
 1 file changed, 174 insertions(+), 211 deletions(-)

-- 
2.9.3

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/9] platform/x86: hp-wmi: Cleanup local variable declarations
  2017-04-20  2:25 [PATCH v1 0/9] platform/x86: hp-wmi: Driver refactoring and cleanups Darren Hart
@ 2017-04-20  2:25 ` Darren Hart
  2017-04-20  2:25 ` [PATCH 2/9] platform/x86: hp-wmi: Add bios_args initializer Darren Hart
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 16+ messages in thread
From: Darren Hart @ 2017-04-20  2:25 UTC (permalink / raw)
  To: dvhart; +Cc: andy, platform-driver-x86, linux-kernel, carlo

From: "Darren Hart (VMware)" <dvhart@infradead.org>

Declare like types on one line. Order declarations in decreasing length
where possible.

Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
---
 drivers/platform/x86/hp-wmi.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index 5ca9328..e772105 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -346,15 +346,14 @@ static const struct rfkill_ops hp_wmi_rfkill_ops = {
 
 static bool hp_wmi_get_sw_state(enum hp_wmi_radio r)
 {
+	int mask = 0x200 << (r * 8);
 	int wireless = 0;
-	int mask;
+
 	hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, 0,
 			     &wireless, sizeof(wireless),
 			     sizeof(wireless));
 	/* TBD: Pass error */
 
-	mask = 0x200 << (r * 8);
-
 	if (wireless & mask)
 		return false;
 	else
@@ -363,15 +362,14 @@ static bool hp_wmi_get_sw_state(enum hp_wmi_radio r)
 
 static bool hp_wmi_get_hw_state(enum hp_wmi_radio r)
 {
+	int mask = 0x800 << (r * 8);
 	int wireless = 0;
-	int mask;
+
 	hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, 0,
 			     &wireless, sizeof(wireless),
 			     sizeof(wireless));
 	/* TBD: Pass error */
 
-	mask = 0x800 << (r * 8);
-
 	if (wireless & mask)
 		return false;
 	else
@@ -395,8 +393,8 @@ static const struct rfkill_ops hp_wmi_rfkill2_ops = {
 
 static int hp_wmi_rfkill2_refresh(void)
 {
-	int err, i;
 	struct bios_rfkill2_state state;
+	int err, i;
 
 	err = hp_wmi_perform_query(HPWMI_WIRELESS2_QUERY, 0, &state,
 				   0, sizeof(state));
@@ -502,9 +500,9 @@ static ssize_t set_als(struct device *dev, struct device_attribute *attr,
 static ssize_t set_postcode(struct device *dev, struct device_attribute *attr,
 		       const char *buf, size_t count)
 {
+	long unsigned int tmp2;
 	int ret;
 	u32 tmp;
-	long unsigned int tmp2;
 
 	ret = kstrtoul(buf, 10, &tmp2);
 	if (ret || tmp2 != 1)
@@ -530,11 +528,11 @@ static DEVICE_ATTR(postcode, S_IRUGO | S_IWUSR, show_postcode, set_postcode);
 static void hp_wmi_notify(u32 value, void *context)
 {
 	struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
-	union acpi_object *obj;
 	u32 event_id, event_data;
+	union acpi_object *obj;
 	int key_code = 0, ret;
-	u32 *location;
 	acpi_status status;
+	u32 *location;
 
 	status = wmi_get_event_data(value, &response);
 	if (status != AE_OK) {
@@ -645,8 +643,7 @@ static void hp_wmi_notify(u32 value, void *context)
 static int __init hp_wmi_input_setup(void)
 {
 	acpi_status status;
-	int err;
-	int val;
+	int err, val;
 
 	hp_wmi_input_dev = input_allocate_device();
 	if (!hp_wmi_input_dev)
@@ -719,8 +716,7 @@ static void cleanup_sysfs(struct platform_device *device)
 
 static int __init hp_wmi_rfkill_setup(struct platform_device *device)
 {
-	int err;
-	int wireless = 0;
+	int err, wireless = 0;
 
 	err = hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, 0, &wireless,
 				   sizeof(wireless), sizeof(wireless));
@@ -804,8 +800,9 @@ static int __init hp_wmi_rfkill_setup(struct platform_device *device)
 
 static int __init hp_wmi_rfkill2_setup(struct platform_device *device)
 {
-	int err, i;
 	struct bios_rfkill2_state state;
+	int err, i;
+
 	err = hp_wmi_perform_query(HPWMI_WIRELESS2_QUERY, 0, &state,
 				   0, sizeof(state));
 	if (err)
@@ -1002,9 +999,9 @@ static struct platform_driver hp_wmi_driver = {
 
 static int __init hp_wmi_init(void)
 {
-	int err;
 	int event_capable = wmi_has_guid(HPWMI_EVENT_GUID);
 	int bios_capable = wmi_has_guid(HPWMI_BIOS_GUID);
+	int err;
 
 	if (!bios_capable && !event_capable)
 		return -ENODEV;
-- 
2.9.3

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 2/9] platform/x86: hp-wmi: Add bios_args initializer
  2017-04-20  2:25 [PATCH v1 0/9] platform/x86: hp-wmi: Driver refactoring and cleanups Darren Hart
  2017-04-20  2:25 ` [PATCH 1/9] platform/x86: hp-wmi: Cleanup local variable declarations Darren Hart
@ 2017-04-20  2:25 ` Darren Hart
  2017-04-20  7:37   ` Andy Shevchenko
  2017-04-20  2:25 ` [PATCH 3/9] platform/x86: hp-wmi: Standardize enum usage for constants Darren Hart
                   ` (8 subsequent siblings)
  10 siblings, 1 reply; 16+ messages in thread
From: Darren Hart @ 2017-04-20  2:25 UTC (permalink / raw)
  To: dvhart; +Cc: andy, platform-driver-x86, linux-kernel, carlo

From: "Darren Hart (VMware)" <dvhart@infradead.org>

Cleanup the hp_wmi_perform_query function some by providing a bios_args
initializer. No functional changes.

Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
---
 drivers/platform/x86/hp-wmi.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index e772105..aa9d99c 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -93,6 +93,13 @@ struct bios_args {
 	u32 data;
 };
 
+#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;
@@ -190,18 +197,12 @@ static struct rfkill2_device rfkill2[HPWMI_MAX_RFKILL2_DEVICES];
 static int hp_wmi_perform_query(int query, int write, 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,
-		.commandtype = query,
-		.datasize = insize,
-		.data = 0,
-	};
+	struct bios_args args = BIOS_ARGS_INIT(write, query, insize);
 	struct acpi_buffer input = { sizeof(struct bios_args), &args };
 	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
+	struct bios_return *bios_return;
+	union acpi_object *obj;
+	int actual_outsize;
 	u32 rc;
 
 	if (WARN_ON(insize > sizeof(args.data)))
-- 
2.9.3

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 3/9] platform/x86: hp-wmi: Standardize enum usage for constants
  2017-04-20  2:25 [PATCH v1 0/9] platform/x86: hp-wmi: Driver refactoring and cleanups Darren Hart
  2017-04-20  2:25 ` [PATCH 1/9] platform/x86: hp-wmi: Cleanup local variable declarations Darren Hart
  2017-04-20  2:25 ` [PATCH 2/9] platform/x86: hp-wmi: Add bios_args initializer Darren Hart
@ 2017-04-20  2:25 ` Darren Hart
  2017-04-20  7:19   ` Andy Shevchenko
  2017-04-20 20:31   ` Darren Hart
  2017-04-20  2:25 ` [PATCH 4/9] platform/x86: hp-wmi: Refactor redundant HPWMI_READ functions Darren Hart
                   ` (7 subsequent siblings)
  10 siblings, 2 replies; 16+ messages in thread
From: Darren Hart @ 2017-04-20  2:25 UTC (permalink / raw)
  To: dvhart; +Cc: andy, platform-driver-x86, linux-kernel, carlo

From: "Darren Hart (VMware)" <dvhart@infradead.org>

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) <dvhart@infradead.org>
---
 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
@@ -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,27 @@ 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	= 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)
  * buffer:	Buffer used as input and/or output
  * insize:	Size of input buffer
  * outsize:	Size of output buffer
@@ -189,10 +198,10 @@ 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)
@@ -246,7 +255,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;
@@ -256,7 +265,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;
@@ -266,7 +275,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;
@@ -276,7 +285,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)
@@ -288,7 +297,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;
@@ -299,7 +308,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;
@@ -310,7 +319,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;
@@ -321,7 +330,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;
@@ -334,7 +343,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;
@@ -350,7 +359,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 */
@@ -366,7 +375,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 */
@@ -382,7 +391,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;
@@ -397,7 +406,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;
@@ -424,7 +433,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;
@@ -490,7 +499,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;
@@ -511,7 +520,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;
@@ -584,7 +593,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));
@@ -719,12 +728,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;
@@ -804,7 +813,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;
-- 
2.9.3

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 4/9] platform/x86: hp-wmi: Refactor redundant HPWMI_READ functions
  2017-04-20  2:25 [PATCH v1 0/9] platform/x86: hp-wmi: Driver refactoring and cleanups Darren Hart
                   ` (2 preceding siblings ...)
  2017-04-20  2:25 ` [PATCH 3/9] platform/x86: hp-wmi: Standardize enum usage for constants Darren Hart
@ 2017-04-20  2:25 ` Darren Hart
  2017-04-20  2:25 ` [PATCH 5/9] platform/x86: hp-wmi: Cleanup wireless get_(hw|sw)state functions Darren Hart
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 16+ messages in thread
From: Darren Hart @ 2017-04-20  2:25 UTC (permalink / raw)
  To: dvhart; +Cc: andy, platform-driver-x86, linux-kernel, carlo

From: "Darren Hart (VMware)" <dvhart@infradead.org>

Several functions perform the same WMI read int with different query
arguments. Refactor this into a single hp_wmi_read_int function.

Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
---
 drivers/platform/x86/hp-wmi.c | 82 +++++++++++++------------------------------
 1 file changed, 24 insertions(+), 58 deletions(-)

diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index 60d1e4c..758c229 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -252,55 +252,35 @@ static int hp_wmi_perform_query(int query, int write, void *buffer,
 	return 0;
 }
 
-static int hp_wmi_display_state(void)
+static int hp_wmi_read_int(int query)
 {
-	int state = 0;
-	int ret = hp_wmi_perform_query(HPWMI_DISPLAY_QUERY, HPWMI_READ, &state,
-				       sizeof(state), sizeof(state));
-	if (ret)
-		return ret < 0 ? ret : -EINVAL;
-	return state;
-}
+	int val = 0, ret;
 
-static int hp_wmi_hddtemp_state(void)
-{
-	int state = 0;
-	int ret = hp_wmi_perform_query(HPWMI_HDDTEMP_QUERY, HPWMI_READ, &state,
-				       sizeof(state), sizeof(state));
-	if (ret)
-		return ret < 0 ? ret : -EINVAL;
-	return state;
-}
+	ret = hp_wmi_perform_query(query, HPWMI_READ, &val,
+				   sizeof(val), sizeof(val));
 
-static int hp_wmi_als_state(void)
-{
-	int state = 0;
-	int ret = hp_wmi_perform_query(HPWMI_ALS_QUERY, HPWMI_READ, &state,
-				       sizeof(state), sizeof(state));
 	if (ret)
 		return ret < 0 ? ret : -EINVAL;
-	return state;
+
+	return val;
 }
 
 static int hp_wmi_dock_state(void)
 {
-	int state = 0;
-	int ret = hp_wmi_perform_query(HPWMI_HARDWARE_QUERY, HPWMI_READ, &state,
-				       sizeof(state), sizeof(state));
+	int state = hp_wmi_read_int(HPWMI_HARDWARE_QUERY);
 
-	if (ret)
-		return ret < 0 ? ret : -EINVAL;
+	if (state < 0)
+		return state;
 
 	return state & 0x1;
 }
 
 static int hp_wmi_tablet_state(void)
 {
-	int state = 0;
-	int ret = hp_wmi_perform_query(HPWMI_HARDWARE_QUERY, HPWMI_READ, &state,
-				       sizeof(state), sizeof(state));
-	if (ret)
-		return ret < 0 ? ret : -EINVAL;
+	int state = hp_wmi_read_int(HPWMI_HARDWARE_QUERY);
+
+	if (state < 0)
+		return state;
 
 	return (state & 0x4) ? 1 : 0;
 }
@@ -430,20 +410,10 @@ static int hp_wmi_rfkill2_refresh(void)
 	return 0;
 }
 
-static int hp_wmi_post_code_state(void)
-{
-	int state = 0;
-	int ret = hp_wmi_perform_query(HPWMI_POSTCODEERROR_QUERY, HPWMI_READ, &state,
-				       sizeof(state), sizeof(state));
-	if (ret)
-		return ret < 0 ? ret : -EINVAL;
-	return state;
-}
-
 static ssize_t show_display(struct device *dev, struct device_attribute *attr,
 			    char *buf)
 {
-	int value = hp_wmi_display_state();
+	int value = hp_wmi_read_int(HPWMI_DISPLAY_QUERY);
 	if (value < 0)
 		return -EINVAL;
 	return sprintf(buf, "%d\n", value);
@@ -452,7 +422,7 @@ static ssize_t show_display(struct device *dev, struct device_attribute *attr,
 static ssize_t show_hddtemp(struct device *dev, struct device_attribute *attr,
 			    char *buf)
 {
-	int value = hp_wmi_hddtemp_state();
+	int value = hp_wmi_read_int(HPWMI_HDDTEMP_QUERY);
 	if (value < 0)
 		return -EINVAL;
 	return sprintf(buf, "%d\n", value);
@@ -461,7 +431,7 @@ static ssize_t show_hddtemp(struct device *dev, struct device_attribute *attr,
 static ssize_t show_als(struct device *dev, struct device_attribute *attr,
 			char *buf)
 {
-	int value = hp_wmi_als_state();
+	int value = hp_wmi_read_int(HPWMI_ALS_QUERY);
 	if (value < 0)
 		return -EINVAL;
 	return sprintf(buf, "%d\n", value);
@@ -489,7 +459,7 @@ static ssize_t show_postcode(struct device *dev, struct device_attribute *attr,
 			 char *buf)
 {
 	/* Get the POST error code of previous boot failure. */
-	int value = hp_wmi_post_code_state();
+	int value = hp_wmi_read_int(HPWMI_POSTCODEERROR_QUERY);
 	if (value < 0)
 		return -EINVAL;
 	return sprintf(buf, "0x%x\n", value);
@@ -540,9 +510,9 @@ static void hp_wmi_notify(u32 value, void *context)
 	struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
 	u32 event_id, event_data;
 	union acpi_object *obj;
-	int key_code = 0, ret;
 	acpi_status status;
 	u32 *location;
+	int key_code;
 
 	status = wmi_get_event_data(value, &response);
 	if (status != AE_OK) {
@@ -593,11 +563,8 @@ 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, HPWMI_READ,
-					   &key_code,
-					   sizeof(key_code),
-					   sizeof(key_code));
-		if (ret)
+		key_code = hp_wmi_read_int(HPWMI_HOTKEY_QUERY);
+		if (key_code < 0)
 			break;
 
 		if (!sparse_keymap_report_event(hp_wmi_input_dev,
@@ -726,12 +693,11 @@ static void cleanup_sysfs(struct platform_device *device)
 
 static int __init hp_wmi_rfkill_setup(struct platform_device *device)
 {
-	int err, wireless = 0;
+	int err, wireless;
 
-	err = hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, HPWMI_READ, &wireless,
-				   sizeof(wireless), sizeof(wireless));
-	if (err)
-		return err;
+	wireless = hp_wmi_read_int(HPWMI_WIRELESS_QUERY);
+	if (wireless)
+		return wireless;
 
 	err = hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, HPWMI_WRITE, &wireless,
 				   sizeof(wireless), 0);
-- 
2.9.3

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 5/9] platform/x86: hp-wmi: Cleanup wireless get_(hw|sw)state functions
  2017-04-20  2:25 [PATCH v1 0/9] platform/x86: hp-wmi: Driver refactoring and cleanups Darren Hart
                   ` (3 preceding siblings ...)
  2017-04-20  2:25 ` [PATCH 4/9] platform/x86: hp-wmi: Refactor redundant HPWMI_READ functions Darren Hart
@ 2017-04-20  2:25 ` Darren Hart
  2017-04-20  2:25 ` [PATCH 6/9] platform/x86: hp-wmi: Refactor dock and tablet state fetchers Darren Hart
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 16+ messages in thread
From: Darren Hart @ 2017-04-20  2:25 UTC (permalink / raw)
  To: dvhart; +Cc: andy, platform-driver-x86, linux-kernel, carlo

From: "Darren Hart (VMware)" <dvhart@infradead.org>

Use the new hp_wmi_read_int() function and add a WARN_ONCE() to the TBD
regarding passing the error through. These are used in a null return
function unfortunately.

Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
---
 drivers/platform/x86/hp-wmi.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index 758c229..e46b61c 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -337,33 +337,25 @@ static const struct rfkill_ops hp_wmi_rfkill_ops = {
 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, HPWMI_READ,
-			     &wireless, sizeof(wireless),
-			     sizeof(wireless));
+	int wireless = hp_wmi_read_int(HPWMI_WIRELESS_QUERY);
+
 	/* TBD: Pass error */
+	WARN_ONCE(wireless < 0, "error executing HPWMI_WIRELESS_QUERY");
 
-	if (wireless & mask)
-		return false;
-	else
-		return true;
+	return !(wireless & mask);
 }
 
 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, HPWMI_READ,
-			     &wireless, sizeof(wireless),
-			     sizeof(wireless));
+	int wireless = hp_wmi_read_int(HPWMI_WIRELESS_QUERY);
+
 	/* TBD: Pass error */
+	WARN_ONCE(wireless < 0, "error executing HPWMI_WIRELESS_QUERY");
 
-	if (wireless & mask)
-		return false;
-	else
-		return true;
+	return !(wireless & mask);
 }
 
 static int hp_wmi_rfkill2_set_block(void *data, bool blocked)
-- 
2.9.3

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 6/9] platform/x86: hp-wmi: Refactor dock and tablet state fetchers
  2017-04-20  2:25 [PATCH v1 0/9] platform/x86: hp-wmi: Driver refactoring and cleanups Darren Hart
                   ` (4 preceding siblings ...)
  2017-04-20  2:25 ` [PATCH 5/9] platform/x86: hp-wmi: Cleanup wireless get_(hw|sw)state functions Darren Hart
@ 2017-04-20  2:25 ` Darren Hart
  2017-04-20  2:25 ` [PATCH 7/9] platform/x86: hp-wmi: Use DEVICE_ATTR_(RO|RW) helper macros Darren Hart
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 16+ messages in thread
From: Darren Hart @ 2017-04-20  2:25 UTC (permalink / raw)
  To: dvhart; +Cc: andy, platform-driver-x86, linux-kernel, carlo

From: "Darren Hart (VMware)" <dvhart@infradead.org>

Both dock and tablet use the HPWMI_HARDWARE_QUERY, but require different
masks. Rather than using two functions with magic masks, define the
masks, and use a common accessor.

Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
---
 drivers/platform/x86/hp-wmi.c | 33 ++++++++++++++-------------------
 1 file changed, 14 insertions(+), 19 deletions(-)

diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index e46b61c..89d6278 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -102,6 +102,11 @@ enum hp_wmi_command {
 	HPWMI_ODM	= 0x03,
 };
 
+enum hp_wmi_hardware_mask {
+	HPWMI_DOCK_MASK		= 0x01,
+	HPWMI_TABLET_MASK	= 0x04,
+};
+
 #define BIOS_ARGS_INIT(write, ctype, size)				\
 	(struct bios_args)	{	.signature = 0x55434553,	\
 					.command = (write) ? 0x2 : 0x1,	\
@@ -265,7 +270,7 @@ static int hp_wmi_read_int(int query)
 	return val;
 }
 
-static int hp_wmi_dock_state(void)
+static int hp_wmi_hw_state(int mask)
 {
 	int state = hp_wmi_read_int(HPWMI_HARDWARE_QUERY);
 
@@ -275,16 +280,6 @@ static int hp_wmi_dock_state(void)
 	return state & 0x1;
 }
 
-static int hp_wmi_tablet_state(void)
-{
-	int state = hp_wmi_read_int(HPWMI_HARDWARE_QUERY);
-
-	if (state < 0)
-		return state;
-
-	return (state & 0x4) ? 1 : 0;
-}
-
 static int __init hp_wmi_bios_2008_later(void)
 {
 	int state = 0;
@@ -432,7 +427,7 @@ static ssize_t show_als(struct device *dev, struct device_attribute *attr,
 static ssize_t show_dock(struct device *dev, struct device_attribute *attr,
 			 char *buf)
 {
-	int value = hp_wmi_dock_state();
+	int value = hp_wmi_hw_state(HPWMI_DOCK_MASK);
 	if (value < 0)
 		return -EINVAL;
 	return sprintf(buf, "%d\n", value);
@@ -441,7 +436,7 @@ static ssize_t show_dock(struct device *dev, struct device_attribute *attr,
 static ssize_t show_tablet(struct device *dev, struct device_attribute *attr,
 			 char *buf)
 {
-	int value = hp_wmi_tablet_state();
+	int value = hp_wmi_hw_state(HPWMI_TABLET_MASK);
 	if (value < 0)
 		return -EINVAL;
 	return sprintf(buf, "%d\n", value);
@@ -544,10 +539,10 @@ static void hp_wmi_notify(u32 value, void *context)
 	case HPWMI_DOCK_EVENT:
 		if (test_bit(SW_DOCK, hp_wmi_input_dev->swbit))
 			input_report_switch(hp_wmi_input_dev, SW_DOCK,
-					    hp_wmi_dock_state());
+					    hp_wmi_hw_state(HPWMI_DOCK_MASK));
 		if (test_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit))
 			input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE,
-					    hp_wmi_tablet_state());
+					    hp_wmi_hw_state(HPWMI_TABLET_MASK));
 		input_sync(hp_wmi_input_dev);
 		break;
 	case HPWMI_PARK_HDD:
@@ -625,14 +620,14 @@ static int __init hp_wmi_input_setup(void)
 	__set_bit(EV_SW, hp_wmi_input_dev->evbit);
 
 	/* Dock */
-	val = hp_wmi_dock_state();
+	val = hp_wmi_hw_state(HPWMI_DOCK_MASK);
 	if (!(val < 0)) {
 		__set_bit(SW_DOCK, hp_wmi_input_dev->swbit);
 		input_report_switch(hp_wmi_input_dev, SW_DOCK, val);
 	}
 
 	/* Tablet mode */
-	val = hp_wmi_tablet_state();
+	val = hp_wmi_hw_state(HPWMI_TABLET_MASK);
 	if (!(val < 0)) {
 		__set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit);
 		input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, val);
@@ -926,10 +921,10 @@ static int hp_wmi_resume_handler(struct device *device)
 	if (hp_wmi_input_dev) {
 		if (test_bit(SW_DOCK, hp_wmi_input_dev->swbit))
 			input_report_switch(hp_wmi_input_dev, SW_DOCK,
-					    hp_wmi_dock_state());
+					    hp_wmi_hw_state(HPWMI_DOCK_MASK));
 		if (test_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit))
 			input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE,
-					    hp_wmi_tablet_state());
+					    hp_wmi_hw_state(HPWMI_TABLET_MASK));
 		input_sync(hp_wmi_input_dev);
 	}
 
-- 
2.9.3

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 7/9] platform/x86: hp-wmi: Use DEVICE_ATTR_(RO|RW) helper macros
  2017-04-20  2:25 [PATCH v1 0/9] platform/x86: hp-wmi: Driver refactoring and cleanups Darren Hart
                   ` (5 preceding siblings ...)
  2017-04-20  2:25 ` [PATCH 6/9] platform/x86: hp-wmi: Refactor dock and tablet state fetchers Darren Hart
@ 2017-04-20  2:25 ` Darren Hart
  2017-04-20  2:25 ` [PATCH 8/9] platform/x86: hp-wmi: Do not shadow errors in sysfs show functions Darren Hart
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 16+ messages in thread
From: Darren Hart @ 2017-04-20  2:25 UTC (permalink / raw)
  To: dvhart; +Cc: andy, platform-driver-x86, linux-kernel, carlo

From: "Darren Hart (VMware)" <dvhart@infradead.org>

Use the DEVICE_ATTR_(RO|RW) macros, ranaming the show and store
functions accordingly.

Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
---
 drivers/platform/x86/hp-wmi.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index 89d6278..ccacd1a 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -397,7 +397,7 @@ static int hp_wmi_rfkill2_refresh(void)
 	return 0;
 }
 
-static ssize_t show_display(struct device *dev, struct device_attribute *attr,
+static ssize_t display_show(struct device *dev, struct device_attribute *attr,
 			    char *buf)
 {
 	int value = hp_wmi_read_int(HPWMI_DISPLAY_QUERY);
@@ -406,7 +406,7 @@ static ssize_t show_display(struct device *dev, struct device_attribute *attr,
 	return sprintf(buf, "%d\n", value);
 }
 
-static ssize_t show_hddtemp(struct device *dev, struct device_attribute *attr,
+static ssize_t hddtemp_show(struct device *dev, struct device_attribute *attr,
 			    char *buf)
 {
 	int value = hp_wmi_read_int(HPWMI_HDDTEMP_QUERY);
@@ -415,7 +415,7 @@ static ssize_t show_hddtemp(struct device *dev, struct device_attribute *attr,
 	return sprintf(buf, "%d\n", value);
 }
 
-static ssize_t show_als(struct device *dev, struct device_attribute *attr,
+static ssize_t als_show(struct device *dev, struct device_attribute *attr,
 			char *buf)
 {
 	int value = hp_wmi_read_int(HPWMI_ALS_QUERY);
@@ -424,7 +424,7 @@ static ssize_t show_als(struct device *dev, struct device_attribute *attr,
 	return sprintf(buf, "%d\n", value);
 }
 
-static ssize_t show_dock(struct device *dev, struct device_attribute *attr,
+static ssize_t dock_show(struct device *dev, struct device_attribute *attr,
 			 char *buf)
 {
 	int value = hp_wmi_hw_state(HPWMI_DOCK_MASK);
@@ -433,8 +433,8 @@ static ssize_t show_dock(struct device *dev, struct device_attribute *attr,
 	return sprintf(buf, "%d\n", value);
 }
 
-static ssize_t show_tablet(struct device *dev, struct device_attribute *attr,
-			 char *buf)
+static ssize_t tablet_show(struct device *dev, struct device_attribute *attr,
+			   char *buf)
 {
 	int value = hp_wmi_hw_state(HPWMI_TABLET_MASK);
 	if (value < 0)
@@ -442,8 +442,8 @@ static ssize_t show_tablet(struct device *dev, struct device_attribute *attr,
 	return sprintf(buf, "%d\n", value);
 }
 
-static ssize_t show_postcode(struct device *dev, struct device_attribute *attr,
-			 char *buf)
+static ssize_t postcode_show(struct device *dev, struct device_attribute *attr,
+			     char *buf)
 {
 	/* Get the POST error code of previous boot failure. */
 	int value = hp_wmi_read_int(HPWMI_POSTCODEERROR_QUERY);
@@ -452,8 +452,8 @@ static ssize_t show_postcode(struct device *dev, struct device_attribute *attr,
 	return sprintf(buf, "0x%x\n", value);
 }
 
-static ssize_t set_als(struct device *dev, struct device_attribute *attr,
-		       const char *buf, size_t count)
+static ssize_t als_store(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, HPWMI_WRITE, &tmp,
@@ -464,8 +464,8 @@ static ssize_t set_als(struct device *dev, struct device_attribute *attr,
 	return count;
 }
 
-static ssize_t set_postcode(struct device *dev, struct device_attribute *attr,
-		       const char *buf, size_t count)
+static ssize_t postcode_store(struct device *dev, struct device_attribute *attr,
+			      const char *buf, size_t count)
 {
 	long unsigned int tmp2;
 	int ret;
@@ -485,12 +485,12 @@ static ssize_t set_postcode(struct device *dev, struct device_attribute *attr,
 	return count;
 }
 
-static DEVICE_ATTR(display, S_IRUGO, show_display, NULL);
-static DEVICE_ATTR(hddtemp, S_IRUGO, show_hddtemp, NULL);
-static DEVICE_ATTR(als, S_IRUGO | S_IWUSR, show_als, set_als);
-static DEVICE_ATTR(dock, S_IRUGO, show_dock, NULL);
-static DEVICE_ATTR(tablet, S_IRUGO, show_tablet, NULL);
-static DEVICE_ATTR(postcode, S_IRUGO | S_IWUSR, show_postcode, set_postcode);
+static DEVICE_ATTR_RO(display);
+static DEVICE_ATTR_RO(hddtemp);
+static DEVICE_ATTR_RW(als);
+static DEVICE_ATTR_RO(dock);
+static DEVICE_ATTR_RO(tablet);
+static DEVICE_ATTR_RW(postcode);
 
 static void hp_wmi_notify(u32 value, void *context)
 {
-- 
2.9.3

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 8/9] platform/x86: hp-wmi: Do not shadow errors in sysfs show functions
  2017-04-20  2:25 [PATCH v1 0/9] platform/x86: hp-wmi: Driver refactoring and cleanups Darren Hart
                   ` (6 preceding siblings ...)
  2017-04-20  2:25 ` [PATCH 7/9] platform/x86: hp-wmi: Use DEVICE_ATTR_(RO|RW) helper macros Darren Hart
@ 2017-04-20  2:25 ` Darren Hart
  2017-04-20  2:25 ` [PATCH 9/9] platform/x86: hp-wmi: Cleanup exit paths Darren Hart
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 16+ messages in thread
From: Darren Hart @ 2017-04-20  2:25 UTC (permalink / raw)
  To: dvhart; +Cc: andy, platform-driver-x86, linux-kernel, carlo

From: "Darren Hart (VMware)" <dvhart@infradead.org>

The new hp_wmi_read_int function returns a negative value in case of
error, pass this on directly rather than always replacing it with
-EINVAL.

Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
---
 drivers/platform/x86/hp-wmi.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index ccacd1a..90b8652 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -402,7 +402,7 @@ static ssize_t display_show(struct device *dev, struct device_attribute *attr,
 {
 	int value = hp_wmi_read_int(HPWMI_DISPLAY_QUERY);
 	if (value < 0)
-		return -EINVAL;
+		return value;
 	return sprintf(buf, "%d\n", value);
 }
 
@@ -411,7 +411,7 @@ static ssize_t hddtemp_show(struct device *dev, struct device_attribute *attr,
 {
 	int value = hp_wmi_read_int(HPWMI_HDDTEMP_QUERY);
 	if (value < 0)
-		return -EINVAL;
+		return value;
 	return sprintf(buf, "%d\n", value);
 }
 
@@ -420,7 +420,7 @@ static ssize_t als_show(struct device *dev, struct device_attribute *attr,
 {
 	int value = hp_wmi_read_int(HPWMI_ALS_QUERY);
 	if (value < 0)
-		return -EINVAL;
+		return value;
 	return sprintf(buf, "%d\n", value);
 }
 
@@ -429,7 +429,7 @@ static ssize_t dock_show(struct device *dev, struct device_attribute *attr,
 {
 	int value = hp_wmi_hw_state(HPWMI_DOCK_MASK);
 	if (value < 0)
-		return -EINVAL;
+		return value;
 	return sprintf(buf, "%d\n", value);
 }
 
@@ -438,7 +438,7 @@ static ssize_t tablet_show(struct device *dev, struct device_attribute *attr,
 {
 	int value = hp_wmi_hw_state(HPWMI_TABLET_MASK);
 	if (value < 0)
-		return -EINVAL;
+		return value;
 	return sprintf(buf, "%d\n", value);
 }
 
@@ -448,7 +448,7 @@ static ssize_t postcode_show(struct device *dev, struct device_attribute *attr,
 	/* Get the POST error code of previous boot failure. */
 	int value = hp_wmi_read_int(HPWMI_POSTCODEERROR_QUERY);
 	if (value < 0)
-		return -EINVAL;
+		return value;
 	return sprintf(buf, "0x%x\n", value);
 }
 
-- 
2.9.3

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 9/9] platform/x86: hp-wmi: Cleanup exit paths
  2017-04-20  2:25 [PATCH v1 0/9] platform/x86: hp-wmi: Driver refactoring and cleanups Darren Hart
                   ` (7 preceding siblings ...)
  2017-04-20  2:25 ` [PATCH 8/9] platform/x86: hp-wmi: Do not shadow errors in sysfs show functions Darren Hart
@ 2017-04-20  2:25 ` Darren Hart
  2017-04-20  7:38 ` [PATCH v1 0/9] platform/x86: hp-wmi: Driver refactoring and cleanups Andy Shevchenko
  2017-04-20  9:06 ` Carlo Caione
  10 siblings, 0 replies; 16+ messages in thread
From: Darren Hart @ 2017-04-20  2:25 UTC (permalink / raw)
  To: dvhart; +Cc: andy, platform-driver-x86, linux-kernel, carlo

From: "Darren Hart (VMware)" <dvhart@infradead.org>

Several exit paths were more complex than they needed to be. Remove
superfluous conditionals, use labels common cleanup, do not shadow
negative error codes.

Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
---
 drivers/platform/x86/hp-wmi.c | 63 ++++++++++++++++++++++---------------------
 1 file changed, 33 insertions(+), 30 deletions(-)

diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index 90b8652..effb5d5 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -217,7 +217,7 @@ static int hp_wmi_perform_query(int query, int write, void *buffer,
 	struct bios_return *bios_return;
 	union acpi_object *obj;
 	int actual_outsize;
-	u32 rc;
+	int ret = 0;
 
 	if (WARN_ON(insize > sizeof(args.data)))
 		return -EINVAL;
@@ -229,32 +229,32 @@ static int hp_wmi_perform_query(int query, int write, void *buffer,
 
 	if (!obj)
 		return -EINVAL;
-	else if (obj->type != ACPI_TYPE_BUFFER) {
-		kfree(obj);
-		return -EINVAL;
+
+	if (obj->type != ACPI_TYPE_BUFFER) {
+		ret = -EINVAL;
+		goto out_free;
 	}
 
 	bios_return = (struct bios_return *)obj->buffer.pointer;
-	rc = bios_return->return_code;
+	ret = bios_return->return_code;
 
-	if (rc) {
-		if (rc != HPWMI_RET_UNKNOWN_CMDTYPE)
-			pr_warn("query 0x%x returned error 0x%x\n", query, rc);
-		kfree(obj);
-		return rc;
+	if (ret) {
+		if (ret != HPWMI_RET_UNKNOWN_CMDTYPE)
+			pr_warn("query 0x%x returned error 0x%x\n", query, ret);
+		goto out_free;
 	}
 
-	if (!outsize) {
-		/* ignore output data */
-		kfree(obj);
-		return 0;
-	}
+	/* Ignore output data of zero size */
+	if (!outsize)
+		goto out_free;
 
 	actual_outsize = min(outsize, (int)(obj->buffer.length - sizeof(*bios_return)));
 	memcpy(buffer, obj->buffer.pointer + sizeof(*bios_return), actual_outsize);
 	memset(buffer + actual_outsize, 0, outsize - actual_outsize);
+
+out_free:
 	kfree(obj);
-	return 0;
+	return ret;
 }
 
 static int hp_wmi_read_int(int query)
@@ -307,9 +307,8 @@ static int __init hp_wmi_enable_hotkeys(void)
 	int value = 0x6e;
 	int ret = hp_wmi_perform_query(HPWMI_BIOS_QUERY, HPWMI_WRITE, &value,
 				       sizeof(value), 0);
-	if (ret)
-		return ret < 0 ? ret : -EINVAL;
-	return 0;
+
+	return ret <= 0 ? ret : -EINVAL;
 }
 
 static int hp_wmi_set_block(void *data, bool blocked)
@@ -320,9 +319,8 @@ static int hp_wmi_set_block(void *data, bool blocked)
 
 	ret = hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, HPWMI_WRITE,
 				   &query, sizeof(query), 0);
-	if (ret)
-		return ret < 0 ? ret : -EINVAL;
-	return 0;
+
+	return ret <= 0 ? ret : -EINVAL;
 }
 
 static const struct rfkill_ops hp_wmi_rfkill_ops = {
@@ -357,11 +355,12 @@ 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 };
+	int ret;
 
-	if (hp_wmi_perform_query(HPWMI_WIRELESS2_QUERY, HPWMI_WRITE,
-				   buffer, sizeof(buffer), 0))
-		return -EINVAL;
-	return 0;
+	ret = hp_wmi_perform_query(HPWMI_WIRELESS2_QUERY, HPWMI_WRITE,
+				   buffer, sizeof(buffer), 0);
+
+	return ret <= 0 ? ret : -EINVAL;
 }
 
 static const struct rfkill_ops hp_wmi_rfkill2_ops = {
@@ -472,13 +471,17 @@ static ssize_t postcode_store(struct device *dev, struct device_attribute *attr,
 	u32 tmp;
 
 	ret = kstrtoul(buf, 10, &tmp2);
-	if (ret || tmp2 != 1)
-		return -EINVAL;
+	if (!ret && tmp2 != 1)
+		ret = -EINVAL;
+	if (ret)
+		goto out;
 
 	/* Clear the POST error code. It is kept until until cleared. */
 	tmp = (u32) tmp2;
 	ret = hp_wmi_perform_query(HPWMI_POSTCODEERROR_QUERY, HPWMI_WRITE, &tmp,
 				       sizeof(tmp), sizeof(tmp));
+
+out:
 	if (ret)
 		return ret < 0 ? ret : -EINVAL;
 
@@ -683,7 +686,7 @@ static int __init hp_wmi_rfkill_setup(struct platform_device *device)
 	int err, wireless;
 
 	wireless = hp_wmi_read_int(HPWMI_WIRELESS_QUERY);
-	if (wireless)
+	if (wireless < 0)
 		return wireless;
 
 	err = hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, HPWMI_WRITE, &wireless,
@@ -769,7 +772,7 @@ static int __init hp_wmi_rfkill2_setup(struct platform_device *device)
 	err = hp_wmi_perform_query(HPWMI_WIRELESS2_QUERY, HPWMI_READ, &state,
 				   0, sizeof(state));
 	if (err)
-		return err;
+		return err < 0 ? err : -EINVAL;
 
 	if (state.count > HPWMI_MAX_RFKILL2_DEVICES) {
 		pr_warn("unable to parse 0x1b query output\n");
-- 
2.9.3

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/9] platform/x86: hp-wmi: Standardize enum usage for constants
  2017-04-20  2:25 ` [PATCH 3/9] platform/x86: hp-wmi: Standardize enum usage for constants Darren Hart
@ 2017-04-20  7:19   ` Andy Shevchenko
  2017-04-20 20:31   ` Darren Hart
  1 sibling, 0 replies; 16+ messages in thread
From: Andy Shevchenko @ 2017-04-20  7:19 UTC (permalink / raw)
  To: Darren Hart; +Cc: Andy Shevchenko, Platform Driver, linux-kernel, carlo

On Thu, Apr 20, 2017 at 5:25 AM, Darren Hart <dvhart@infradead.org> wrote:
> From: "Darren Hart (VMware)" <dvhart@infradead.org>
>
> 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.
>

Looks good to me:
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
> ---
>  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
> @@ -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,27 @@ 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      = 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)
>   * buffer:     Buffer used as input and/or output
>   * insize:     Size of input buffer
>   * outsize:    Size of output buffer
> @@ -189,10 +198,10 @@ 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)
> @@ -246,7 +255,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;
> @@ -256,7 +265,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;
> @@ -266,7 +275,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;
> @@ -276,7 +285,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)
> @@ -288,7 +297,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;
> @@ -299,7 +308,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;
> @@ -310,7 +319,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;
> @@ -321,7 +330,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;
> @@ -334,7 +343,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;
> @@ -350,7 +359,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 */
> @@ -366,7 +375,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 */
> @@ -382,7 +391,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;
> @@ -397,7 +406,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;
> @@ -424,7 +433,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;
> @@ -490,7 +499,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;
> @@ -511,7 +520,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;
> @@ -584,7 +593,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));
> @@ -719,12 +728,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;
> @@ -804,7 +813,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;
> --
> 2.9.3
>



-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/9] platform/x86: hp-wmi: Add bios_args initializer
  2017-04-20  2:25 ` [PATCH 2/9] platform/x86: hp-wmi: Add bios_args initializer Darren Hart
@ 2017-04-20  7:37   ` Andy Shevchenko
  0 siblings, 0 replies; 16+ messages in thread
From: Andy Shevchenko @ 2017-04-20  7:37 UTC (permalink / raw)
  To: Darren Hart; +Cc: Andy Shevchenko, Platform Driver, linux-kernel, Carlo Caione

On Thu, Apr 20, 2017 at 5:25 AM, Darren Hart <dvhart@infradead.org> wrote:
> From: "Darren Hart (VMware)" <dvhart@infradead.org>
>
> Cleanup the hp_wmi_perform_query function some by providing a bios_args
> initializer. No functional changes.

Is it going to be used only once?
If so, I would go with current code.

>
> Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
> ---
>  drivers/platform/x86/hp-wmi.c | 21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
> index e772105..aa9d99c 100644
> --- a/drivers/platform/x86/hp-wmi.c
> +++ b/drivers/platform/x86/hp-wmi.c
> @@ -93,6 +93,13 @@ struct bios_args {
>         u32 data;
>  };
>
> +#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;
> @@ -190,18 +197,12 @@ static struct rfkill2_device rfkill2[HPWMI_MAX_RFKILL2_DEVICES];
>  static int hp_wmi_perform_query(int query, int write, 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,
> -               .commandtype = query,
> -               .datasize = insize,
> -               .data = 0,
> -       };
> +       struct bios_args args = BIOS_ARGS_INIT(write, query, insize);
>         struct acpi_buffer input = { sizeof(struct bios_args), &args };
>         struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> +       struct bios_return *bios_return;
> +       union acpi_object *obj;
> +       int actual_outsize;
>         u32 rc;
>
>         if (WARN_ON(insize > sizeof(args.data)))
> --
> 2.9.3
>



-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v1 0/9] platform/x86: hp-wmi: Driver refactoring and cleanups
  2017-04-20  2:25 [PATCH v1 0/9] platform/x86: hp-wmi: Driver refactoring and cleanups Darren Hart
                   ` (8 preceding siblings ...)
  2017-04-20  2:25 ` [PATCH 9/9] platform/x86: hp-wmi: Cleanup exit paths Darren Hart
@ 2017-04-20  7:38 ` Andy Shevchenko
  2017-04-20 20:19   ` Darren Hart
  2017-04-20  9:06 ` Carlo Caione
  10 siblings, 1 reply; 16+ messages in thread
From: Andy Shevchenko @ 2017-04-20  7:38 UTC (permalink / raw)
  To: Darren Hart; +Cc: Andy Shevchenko, Platform Driver, linux-kernel, Carlo Caione

On Thu, Apr 20, 2017 at 5:25 AM, Darren Hart <dvhart@infradead.org> wrote:
> From: "Darren Hart (VMware)" <dvhart@infradead.org>
>
> This series factors out some redundant code, cleans up a number of style issues,
> modernizes the sysfs usage, and cleans up the return paths. All told, the driver
> is reduced in size by 37 lines (3.6%).
>
> I do not have an HP laptop, so I'm hoping Carlo can help out with some testing.
> In particular we need to verify that hotkeys and sysfs continue to work as
> before.
>

Series looks good to me except patch 2. So,

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

with above exception.

> This series is also available here for convenience:
> git://git.infradead.org/users/dvhart/linux-platform-drivers-x86.git hp-wmi
>
>
> Darren Hart (VMware) (9):
>   platform/x86: hp-wmi: Cleanup local variable declarations
>   platform/x86: hp-wmi: Add bios_args initializer
>   platform/x86: hp-wmi: Standardize enum usage for constants
>   platform/x86: hp-wmi: Refactor redundant HPWMI_READ functions
>   platform/x86: hp-wmi: Cleanup wireless get_(hw|sw)state functions
>   platform/x86: hp-wmi: Refactor dock and tablet state fetchers
>   platform/x86: hp-wmi: Use DEVICE_ATTR_(RO|RW) helper macros
>   platform/x86: hp-wmi: Do not shadow errors in sysfs show functions
>   platform/x86: hp-wmi: Cleanup exit paths
>
>  drivers/platform/x86/hp-wmi.c | 385 +++++++++++++++++++-----------------------
>  1 file changed, 174 insertions(+), 211 deletions(-)
>
> --
> 2.9.3
>



-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v1 0/9] platform/x86: hp-wmi: Driver refactoring and cleanups
  2017-04-20  2:25 [PATCH v1 0/9] platform/x86: hp-wmi: Driver refactoring and cleanups Darren Hart
                   ` (9 preceding siblings ...)
  2017-04-20  7:38 ` [PATCH v1 0/9] platform/x86: hp-wmi: Driver refactoring and cleanups Andy Shevchenko
@ 2017-04-20  9:06 ` Carlo Caione
  10 siblings, 0 replies; 16+ messages in thread
From: Carlo Caione @ 2017-04-20  9:06 UTC (permalink / raw)
  To: Darren Hart; +Cc: andy, platform-driver-x86, open list, Linux Upstreaming Team

On Thu, Apr 20, 2017 at 4:25 AM, Darren Hart <dvhart@infradead.org> wrote:
>
> From: "Darren Hart (VMware)" <dvhart@infradead.org>
>
> This series factors out some redundant code, cleans up a number of style issues,
> modernizes the sysfs usage, and cleans up the return paths. All told, the driver
> is reduced in size by 37 lines (3.6%).
>
> I do not have an HP laptop, so I'm hoping Carlo can help out with some testing.
> In particular we need to verify that hotkeys and sysfs continue to work as
> before.

On my HP 240 G5:

Tested-by: Carlo Caione <carlo@endlessm.com>

Cheers,

-- 
Carlo Caione

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v1 0/9] platform/x86: hp-wmi: Driver refactoring and cleanups
  2017-04-20  7:38 ` [PATCH v1 0/9] platform/x86: hp-wmi: Driver refactoring and cleanups Andy Shevchenko
@ 2017-04-20 20:19   ` Darren Hart
  0 siblings, 0 replies; 16+ messages in thread
From: Darren Hart @ 2017-04-20 20:19 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Andy Shevchenko, Platform Driver, linux-kernel, Carlo Caione

On Thu, Apr 20, 2017 at 10:38:56AM +0300, Andy Shevchenko wrote:
> On Thu, Apr 20, 2017 at 5:25 AM, Darren Hart <dvhart@infradead.org> wrote:
> > From: "Darren Hart (VMware)" <dvhart@infradead.org>
> >
> > This series factors out some redundant code, cleans up a number of style issues,
> > modernizes the sysfs usage, and cleans up the return paths. All told, the driver
> > is reduced in size by 37 lines (3.6%).
> >
> > I do not have an HP laptop, so I'm hoping Carlo can help out with some testing.
> > In particular we need to verify that hotkeys and sysfs continue to work as
> > before.
> >
> 
> Series looks good to me except patch 2. So,
> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
> with above exception.

I was on the fence with this one, which is why I separated it out. I'll drop it.
Thanks for the review.

-- 
Darren Hart
VMware Open Source Technology Center

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/9] platform/x86: hp-wmi: Standardize enum usage for constants
  2017-04-20  2:25 ` [PATCH 3/9] platform/x86: hp-wmi: Standardize enum usage for constants Darren Hart
  2017-04-20  7:19   ` Andy Shevchenko
@ 2017-04-20 20:31   ` Darren Hart
  1 sibling, 0 replies; 16+ messages in thread
From: Darren Hart @ 2017-04-20 20:31 UTC (permalink / raw)
  To: andy, platform-driver-x86, linux-kernel, carlo

On Wed, Apr 19, 2017 at 07:25:15PM -0700, Darren Hart wrote:
> From: "Darren Hart (VMware)" <dvhart@infradead.org>
> 
> 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) <dvhart@infradead.org>
> ---
>  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: <d8193cff33906e24ac1830ecb2ca95833d058a3a.1492719993.git.dvhart@infradead.org>
From: "Darren Hart (VMware)" <dvhart@infradead.org>
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) <dvhart@infradead.org>
Tested-by: Carlo Caione <carlo@endlessm.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 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;
-- 
2.9.3


-- 
Darren Hart
VMware Open Source Technology Center

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2017-04-20 20:31 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-20  2:25 [PATCH v1 0/9] platform/x86: hp-wmi: Driver refactoring and cleanups Darren Hart
2017-04-20  2:25 ` [PATCH 1/9] platform/x86: hp-wmi: Cleanup local variable declarations Darren Hart
2017-04-20  2:25 ` [PATCH 2/9] platform/x86: hp-wmi: Add bios_args initializer Darren Hart
2017-04-20  7:37   ` Andy Shevchenko
2017-04-20  2:25 ` [PATCH 3/9] platform/x86: hp-wmi: Standardize enum usage for constants Darren Hart
2017-04-20  7:19   ` Andy Shevchenko
2017-04-20 20:31   ` Darren Hart
2017-04-20  2:25 ` [PATCH 4/9] platform/x86: hp-wmi: Refactor redundant HPWMI_READ functions Darren Hart
2017-04-20  2:25 ` [PATCH 5/9] platform/x86: hp-wmi: Cleanup wireless get_(hw|sw)state functions Darren Hart
2017-04-20  2:25 ` [PATCH 6/9] platform/x86: hp-wmi: Refactor dock and tablet state fetchers Darren Hart
2017-04-20  2:25 ` [PATCH 7/9] platform/x86: hp-wmi: Use DEVICE_ATTR_(RO|RW) helper macros Darren Hart
2017-04-20  2:25 ` [PATCH 8/9] platform/x86: hp-wmi: Do not shadow errors in sysfs show functions Darren Hart
2017-04-20  2:25 ` [PATCH 9/9] platform/x86: hp-wmi: Cleanup exit paths Darren Hart
2017-04-20  7:38 ` [PATCH v1 0/9] platform/x86: hp-wmi: Driver refactoring and cleanups Andy Shevchenko
2017-04-20 20:19   ` Darren Hart
2017-04-20  9:06 ` Carlo Caione

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).