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

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

[Michał Kępień]

This series contains a few cleanups for the call_fext_func() function.
It does not depend on the previous series I submitted for
fujitsu-laptop, which is why I am sending it in parallel with the
backlight cleanup series.

While patches 1 and 2 are tangible improvements, I will understand if
you deem patch 3 to be unnecessary churn.  I decided to submit it anyway
as I think it might bring some benefit to a casual reader of the
module's code, but it is basically just a suggestion and as it is the
last patch, it can simply be omitted when the series is applied.

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

-- 
2.12.1

[PATCH 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.1

[PATCH 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.  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..e5413d268b24 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, "FUNC interface is not present\n");
 		return -ENODEV;
 	}
 
-- 
2.12.1

[PATCH 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 e5413d268b24..26149f58dba7 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.1

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

[Darren Hart]

On Mon, Apr 03, 2017 at 11:38:56AM +0200, Michał Kępień wrote:
> This series contains a few cleanups for the call_fext_func() function.
> It does not depend on the previous series I submitted for
> fujitsu-laptop, which is why I am sending it in parallel with the
> backlight cleanup series.
> 
> While patches 1 and 2 are tangible improvements, I will understand if
> you deem patch 3 to be unnecessary churn.  I decided to submit it anyway
> as I think it might bring some benefit to a casual reader of the
> module's code, but it is basically just a suggestion and as it is the
> last patch, it can simply be omitted when the series is applied.
> 
>  drivers/platform/x86/fujitsu-laptop.c | 39 +++++++++++------------------------
>  1 file changed, 12 insertions(+), 27 deletions(-)

These all look like worthwhile changes to me. Jonathan, are you aware of any
usage of the arguments in 3/3 that would argue against changing their names to
the more specific ones?

Pushed to the temporary fujitsu branch for testing, awaiting ack from Jonathan.

-- 
Darren Hart
VMware Open Source Technology Center

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

[Jonathan Woithe]

On Mon, Apr 03, 2017 at 03:13:11PM -0700, Darren Hart wrote:
> On Mon, Apr 03, 2017 at 11:38:56AM +0200, Micha?? K??pie?? wrote:
> > This series contains a few cleanups for the call_fext_func() function.
> > It does not depend on the previous series I submitted for
> > fujitsu-laptop, which is why I am sending it in parallel with the
> > backlight cleanup series.
> > 
> > While patches 1 and 2 are tangible improvements, I will understand if
> > you deem patch 3 to be unnecessary churn.  I decided to submit it anyway
> > as I think it might bring some benefit to a casual reader of the
> > module's code, but it is basically just a suggestion and as it is the
> > last patch, it can simply be omitted when the series is applied.
> > 
> >  drivers/platform/x86/fujitsu-laptop.c | 39 +++++++++++------------------------
> >  1 file changed, 12 insertions(+), 27 deletions(-)
> 
> These all look like worthwhile changes to me. Jonathan, are you aware of
> any usage of the arguments in 3/3 that would argue against changing their
> names to the more specific ones?

Certainly not at this stage and realistically I don't expect this to change.
The suggested renaming of fields does add a degree of clarity to the code so
I have no problem with it.

> Pushed to the temporary fujitsu branch for testing, awaiting ack from
> Jonathan.

Even though these are small changes I agree that all three patches contain
worthwhile improvements.  I'm happy for all three of them to be merged.

Reviewed-by: Jonathan Woithe <jwoithe@just42.net>

FYI I am still working through the more extensive backlight cleanup series.
This is a particularly busy week for me so my review comments might only
come through towards the end of the week or during the weekend.

Regards
  jonathan

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

[Darren Hart]

On Tue, Apr 04, 2017 at 09:13:50AM +0930, Jonathan Woithe wrote:
> On Mon, Apr 03, 2017 at 03:13:11PM -0700, Darren Hart wrote:
> > On Mon, Apr 03, 2017 at 11:38:56AM +0200, Micha?? K??pie?? wrote:
> > > This series contains a few cleanups for the call_fext_func() function.
> > > It does not depend on the previous series I submitted for
> > > fujitsu-laptop, which is why I am sending it in parallel with the
> > > backlight cleanup series.
> > > 
> > > While patches 1 and 2 are tangible improvements, I will understand if
> > > you deem patch 3 to be unnecessary churn.  I decided to submit it anyway
> > > as I think it might bring some benefit to a casual reader of the
> > > module's code, but it is basically just a suggestion and as it is the
> > > last patch, it can simply be omitted when the series is applied.
> > > 
> > >  drivers/platform/x86/fujitsu-laptop.c | 39 +++++++++++------------------------
> > >  1 file changed, 12 insertions(+), 27 deletions(-)
> > 
> > These all look like worthwhile changes to me. Jonathan, are you aware of
> > any usage of the arguments in 3/3 that would argue against changing their
> > names to the more specific ones?
> 
> Certainly not at this stage and realistically I don't expect this to change.
> The suggested renaming of fields does add a degree of clarity to the code so
> I have no problem with it.
> 
> > Pushed to the temporary fujitsu branch for testing, awaiting ack from
> > Jonathan.
> 
> Even though these are small changes I agree that all three patches contain
> worthwhile improvements.  I'm happy for all three of them to be merged.
> 
> Reviewed-by: Jonathan Woithe <jwoithe@just42.net>
> 
> FYI I am still working through the more extensive backlight cleanup series.
> This is a particularly busy week for me so my review comments might only
> come through towards the end of the week or during the weekend.

No problem, thanks!

-- 
Darren Hart
VMware Open Source Technology Center