[PATCH v2 0/3] fujitsu-laptop: call_fext_func() cleanup

[PATCH v2 0/3] fujitsu-laptop: call_fext_func() cleanup

[Michał Kępień]

This series contains a few cleanups for the call_fext_func() function.
Just as a reminder, please note that v1 of this series is currently
applied in testing.

Changes from v1:

  - Update debug message logged by call_fext_func() to reflect code flow
    changes introduced by patch 2/3.

 drivers/platform/x86/fujitsu-laptop.c | 39 +++++++++++------------------------
 1 file changed, 12 insertions(+), 27 deletions(-)

-- 
2.12.2

[PATCH v2 1/3] platform/x86: fujitsu-laptop: clean up local variables in call_fext_func()

[Michał Kępień]

Set values of FUNC call parameters in a designated initializer.  Do not
initialize status and handle variables as the values these are
initialized to have no influence on execution flow.  Use an array
variable instead of the address of the first element of that array.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/fujitsu-laptop.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index f66da4b0c31a..ca1491ff659e 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -219,16 +219,16 @@ static u32 dbg_level = 0x03;
 
 static int call_fext_func(int cmd, int arg0, int arg1, int arg2)
 {
-	acpi_status status = AE_OK;
 	union acpi_object params[4] = {
-	{ .type = ACPI_TYPE_INTEGER },
-	{ .type = ACPI_TYPE_INTEGER },
-	{ .type = ACPI_TYPE_INTEGER },
-	{ .type = ACPI_TYPE_INTEGER }
+		{ .integer.type = ACPI_TYPE_INTEGER, .integer.value = cmd },
+		{ .integer.type = ACPI_TYPE_INTEGER, .integer.value = arg0 },
+		{ .integer.type = ACPI_TYPE_INTEGER, .integer.value = arg1 },
+		{ .integer.type = ACPI_TYPE_INTEGER, .integer.value = arg2 }
 	};
-	struct acpi_object_list arg_list = { 4, &params[0] };
+	struct acpi_object_list arg_list = { 4, params };
 	unsigned long long value;
-	acpi_handle handle = NULL;
+	acpi_status status;
+	acpi_handle handle;
 
 	status = acpi_get_handle(fujitsu_laptop->acpi_handle, "FUNC", &handle);
 	if (ACPI_FAILURE(status)) {
@@ -237,11 +237,6 @@ static int call_fext_func(int cmd, int arg0, int arg1, int arg2)
 		return -ENODEV;
 	}
 
-	params[0].integer.value = cmd;
-	params[1].integer.value = arg0;
-	params[2].integer.value = arg1;
-	params[3].integer.value = arg2;
-
 	status = acpi_evaluate_integer(handle, NULL, &arg_list, &value);
 	if (ACPI_FAILURE(status)) {
 		vdbg_printk(FUJLAPTOP_DBG_WARN,
-- 
2.12.2

[PATCH v2 2/3] platform/x86: fujitsu-laptop: simplify call_fext_func()

[Michał Kępień]

acpi_evaluate_integer() takes a pathname parameter which contains the
name of the entity to evaluate underneath the given handle, so calling
acpi_get_handle() beforehand is redundant.  Replace the call to
acpi_get_handle() with a call to acpi_evaluate_integer(), thus
eliminating the need for a local variable storing the handle.  Update
debug message to reflect this change.  Adjust whitespace to make
checkpatch happy.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/fujitsu-laptop.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index ca1491ff659e..8c41968d9e7f 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -228,20 +228,11 @@ static int call_fext_func(int cmd, int arg0, int arg1, int arg2)
 	struct acpi_object_list arg_list = { 4, params };
 	unsigned long long value;
 	acpi_status status;
-	acpi_handle handle;
 
-	status = acpi_get_handle(fujitsu_laptop->acpi_handle, "FUNC", &handle);
+	status = acpi_evaluate_integer(fujitsu_laptop->acpi_handle, "FUNC",
+				       &arg_list, &value);
 	if (ACPI_FAILURE(status)) {
-		vdbg_printk(FUJLAPTOP_DBG_ERROR,
-				"FUNC interface is not present\n");
-		return -ENODEV;
-	}
-
-	status = acpi_evaluate_integer(handle, NULL, &arg_list, &value);
-	if (ACPI_FAILURE(status)) {
-		vdbg_printk(FUJLAPTOP_DBG_WARN,
-			"FUNC 0x%x (args 0x%x, 0x%x, 0x%x) call failed\n",
-				cmd, arg0, arg1, arg2);
+		vdbg_printk(FUJLAPTOP_DBG_ERROR, "Failed to evaluate FUNC\n");
 		return -ENODEV;
 	}
 
-- 
2.12.2

[PATCH v2 3/3] platform/x86: fujitsu-laptop: rename call_fext_func() arguments

[Michał Kępień]

Rename call_fext_func() arguments so that each argument's name signifies
its role:

  - cmd -> func: sub-function to call (flags, buttons etc.),
  - arg0 -> op: operation to perform (get, set, get capabilities etc.),
  - arg1 -> feature: feature to act on (e.g. which LED), if relevant,
  - arg2 -> state: state to set (e.g. LED on or off), if relevant.

Adjust whitespace to make checkpatch happy.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/fujitsu-laptop.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 8c41968d9e7f..928778ccc4c1 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -217,13 +217,13 @@ static u32 dbg_level = 0x03;
 
 /* Fujitsu ACPI interface function */
 
-static int call_fext_func(int cmd, int arg0, int arg1, int arg2)
+static int call_fext_func(int func, int op, int feature, int state)
 {
 	union acpi_object params[4] = {
-		{ .integer.type = ACPI_TYPE_INTEGER, .integer.value = cmd },
-		{ .integer.type = ACPI_TYPE_INTEGER, .integer.value = arg0 },
-		{ .integer.type = ACPI_TYPE_INTEGER, .integer.value = arg1 },
-		{ .integer.type = ACPI_TYPE_INTEGER, .integer.value = arg2 }
+		{ .integer.type = ACPI_TYPE_INTEGER, .integer.value = func },
+		{ .integer.type = ACPI_TYPE_INTEGER, .integer.value = op },
+		{ .integer.type = ACPI_TYPE_INTEGER, .integer.value = feature },
+		{ .integer.type = ACPI_TYPE_INTEGER, .integer.value = state }
 	};
 	struct acpi_object_list arg_list = { 4, params };
 	unsigned long long value;
@@ -236,9 +236,8 @@ static int call_fext_func(int cmd, int arg0, int arg1, int arg2)
 		return -ENODEV;
 	}
 
-	vdbg_printk(FUJLAPTOP_DBG_TRACE,
-		"FUNC 0x%x (args 0x%x, 0x%x, 0x%x) returned 0x%x\n",
-			cmd, arg0, arg1, arg2, (int)value);
+	vdbg_printk(FUJLAPTOP_DBG_TRACE, "FUNC 0x%x (args 0x%x, 0x%x, 0x%x) returned 0x%x\n",
+		    func, op, feature, state, (int)value);
 	return value;
 }
 
-- 
2.12.2

Re: [PATCH v2 0/3] fujitsu-laptop: call_fext_func() cleanup

[Darren Hart]

On Wed, Apr 05, 2017 at 08:50:20AM +0200, Michał Kępień wrote:
> This series contains a few cleanups for the call_fext_func() function.
> Just as a reminder, please note that v1 of this series is currently
> applied in testing.
> 
> Changes from v1:
> 
>   - Update debug message logged by call_fext_func() to reflect code flow
>     changes introduced by patch 2/3.

My apologies Michał, I misunderstood - I was thinking you had a second change to
the 11 patch series, so I asked for a v2.

This series is already in for-next, and we only rebase that for bugs that would
be bad to leave in the history - like things that break bisecting.

For this change, would you please send just 1 new patch on top of for-next?

Again, I'm sorry that, my fault.

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH v2 0/3] fujitsu-laptop: call_fext_func() cleanup

[Michał Kępień]

> On Wed, Apr 05, 2017 at 08:50:20AM +0200, Michał Kępień wrote:
> > This series contains a few cleanups for the call_fext_func() function.
> > Just as a reminder, please note that v1 of this series is currently
> > applied in testing.
> > 
> > Changes from v1:
> > 
> >   - Update debug message logged by call_fext_func() to reflect code flow
> >     changes introduced by patch 2/3.
> 
> My apologies Michał, I misunderstood - I was thinking you had a second change to
> the 11 patch series, so I asked for a v2.
> 
> This series is already in for-next, and we only rebase that for bugs that would
> be bad to leave in the history - like things that break bisecting.
> 
> For this change, would you please send just 1 new patch on top of for-next?
> 
> Again, I'm sorry that, my fault.

No worries.  I will send the debug message update as a separate patch on
top of for-next tomorrow.

-- 
Best regards,
Michał Kępień