* [PATCH 2/6] thinkpad_acpi: Factor out get/set adaptive kbd mode
@ 2015-02-18 20:53 Bastien Nocera
2015-02-20 5:22 ` Darren Hart
0 siblings, 1 reply; 3+ messages in thread
From: Bastien Nocera @ 2015-02-18 20:53 UTC (permalink / raw)
To: Henrique de Moraes Holschuh, Darren Hart, ibm-acpi-devel,
platform-driver-x86, linux-kernel, linux-input
Signed-off-by: Bastien Nocera <hadess@hadess.net>
---
drivers/platform/x86/thinkpad_acpi.c | 61 ++++++++++++++++++++++--------------
1 file changed, 38 insertions(+), 23 deletions(-)
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 80db3ce..a6dd017 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -3480,6 +3480,32 @@ const int adaptive_keyboard_modes[] = {
static bool adaptive_keyboard_mode_is_saved;
static int adaptive_keyboard_prev_mode;
+static int adaptive_keyboard_get_mode(void)
+{
+ u32 mode = 0;
+
+ if (!acpi_evalf(hkey_handle, &mode, "GTRW", "dd", 0)) {
+ pr_err("Cannot read adaptive keyboard mode\n");
+ return -EIO;
+ }
+
+ return mode;
+}
+
+static int adaptive_keyboard_set_mode(int new_mode)
+{
+ if (new_mode < 0 ||
+ new_mode > LAYFLAT_MODE)
+ return -EINVAL;
+
+ if (!acpi_evalf(hkey_handle, NULL, "STRW", "vd", new_mode)) {
+ pr_err("Cannot set adaptive keyboard mode\n");
+ return -EIO;
+ }
+
+ return 0;
+}
+
static int adaptive_keyboard_get_next_mode(int mode)
{
size_t i;
@@ -3509,39 +3535,28 @@ static bool adaptive_keyboard_hotkey_notify_hotkey(unsigned int scancode)
new_mode = adaptive_keyboard_prev_mode;
adaptive_keyboard_mode_is_saved = false;
} else {
- if (!acpi_evalf(
- hkey_handle, ¤t_mode,
- "GTRW", "dd", 0)) {
- pr_err("Cannot read adaptive keyboard mode\n");
+ current_mode = adaptive_keyboard_get_mode();
+ if (current_mode < 0)
return false;
- } else {
- new_mode = adaptive_keyboard_get_next_mode(
- current_mode);
- }
+ new_mode = adaptive_keyboard_get_next_mode(
+ current_mode);
}
- if (!acpi_evalf(hkey_handle, NULL, "STRW", "vd", new_mode)) {
- pr_err("Cannot set adaptive keyboard mode\n");
+ if (adaptive_keyboard_set_mode(new_mode) < 0)
return false;
- }
return true;
case DFR_SHOW_QUICKVIEW_ROW:
- if (!acpi_evalf(hkey_handle,
- &adaptive_keyboard_prev_mode,
- "GTRW", "dd", 0)) {
- pr_err("Cannot read adaptive keyboard mode\n");
+ current_mode = adaptive_keyboard_get_mode();
+ if (current_mode < 0)
return false;
- } else {
- adaptive_keyboard_mode_is_saved = true;
- if (!acpi_evalf(hkey_handle,
- NULL, "STRW", "vd", FUNCTION_MODE)) {
- pr_err("Cannot set adaptive keyboard mode\n");
- return false;
- }
- }
+ adaptive_keyboard_prev_mode = current_mode;
+ adaptive_keyboard_mode_is_saved = true;
+
+ if (adaptive_keyboard_set_mode (FUNCTION_MODE) < 0)
+ return false;
return true;
default:
--
2.1.0
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH 2/6] thinkpad_acpi: Factor out get/set adaptive kbd mode
2015-02-18 20:53 [PATCH 2/6] thinkpad_acpi: Factor out get/set adaptive kbd mode Bastien Nocera
@ 2015-02-20 5:22 ` Darren Hart
2015-02-20 13:54 ` Bastien Nocera
0 siblings, 1 reply; 3+ messages in thread
From: Darren Hart @ 2015-02-20 5:22 UTC (permalink / raw)
To: Bastien Nocera
Cc: Henrique de Moraes Holschuh, ibm-acpi-devel, platform-driver-x86,
linux-kernel, linux-input
On Wed, Feb 18, 2015 at 09:53:35PM +0100, Bastien Nocera wrote:
Please provide a commit message. There is always something to say beyond what is
in the subject. In this case, I suggest the motivation and justification for the
change.
While I appreciate the abstraction, it makes the code at the call site easier to
read, note that you added more code than you removed.
So, please provide a justificaiton.
Under no circumstances will I accept a patch without a commit message body.
> Signed-off-by: Bastien Nocera <hadess@hadess.net>
> ---
> drivers/platform/x86/thinkpad_acpi.c | 61 ++++++++++++++++++++++--------------
> 1 file changed, 38 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 80db3ce..a6dd017 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -3480,6 +3480,32 @@ const int adaptive_keyboard_modes[] = {
> static bool adaptive_keyboard_mode_is_saved;
> static int adaptive_keyboard_prev_mode;
>
> +static int adaptive_keyboard_get_mode(void)
> +{
> + u32 mode = 0;
acpi_evalf second argument takes an "int" and this function returns "int". Is
there a reason to use u32 for mode?
...
> @@ -3509,39 +3535,28 @@ static bool adaptive_keyboard_hotkey_notify_hotkey(unsigned int scancode)
> new_mode = adaptive_keyboard_prev_mode;
> adaptive_keyboard_mode_is_saved = false;
> } else {
> - if (!acpi_evalf(
> - hkey_handle, ¤t_mode,
> - "GTRW", "dd", 0)) {
> - pr_err("Cannot read adaptive keyboard mode\n");
> + current_mode = adaptive_keyboard_get_mode();
> + if (current_mode < 0)
> return false;
> - } else {
> - new_mode = adaptive_keyboard_get_next_mode(
> - current_mode);
> - }
> + new_mode = adaptive_keyboard_get_next_mode(
> + current_mode);
This now fits on one line I believe.
...
--
Darren Hart
Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH 2/6] thinkpad_acpi: Factor out get/set adaptive kbd mode
2015-02-20 5:22 ` Darren Hart
@ 2015-02-20 13:54 ` Bastien Nocera
0 siblings, 0 replies; 3+ messages in thread
From: Bastien Nocera @ 2015-02-20 13:54 UTC (permalink / raw)
To: Darren Hart
Cc: Henrique de Moraes Holschuh, ibm-acpi-devel, platform-driver-x86,
linux-kernel, linux-input
On Thu, 2015-02-19 at 21:22 -0800, Darren Hart wrote:
> On Wed, Feb 18, 2015 at 09:53:35PM +0100, Bastien Nocera wrote:
>
> Please provide a commit message. There is always something to say beyond what is
> in the subject. In this case, I suggest the motivation and justification for the
> change.
>
> While I appreciate the abstraction, it makes the code at the call site easier to
> read, note that you added more code than you removed.
>
> So, please provide a justificaiton.
Sure, done.
> Under no circumstances will I accept a patch without a commit message body.
>
> > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> > ---
> > drivers/platform/x86/thinkpad_acpi.c | 61 ++++++++++++++++++++++--------------
> > 1 file changed, 38 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> > index 80db3ce..a6dd017 100644
> > --- a/drivers/platform/x86/thinkpad_acpi.c
> > +++ b/drivers/platform/x86/thinkpad_acpi.c
> > @@ -3480,6 +3480,32 @@ const int adaptive_keyboard_modes[] = {
> > static bool adaptive_keyboard_mode_is_saved;
> > static int adaptive_keyboard_prev_mode;
> >
> > +static int adaptive_keyboard_get_mode(void)
> > +{
> > + u32 mode = 0;
>
> acpi_evalf second argument takes an "int" and this function returns "int". Is
> there a reason to use u32 for mode?
Used int, done.
> > @@ -3509,39 +3535,28 @@ static bool adaptive_keyboard_hotkey_notify_hotkey(unsigned int scancode)
> > new_mode = adaptive_keyboard_prev_mode;
> > adaptive_keyboard_mode_is_saved = false;
> > } else {
> > - if (!acpi_evalf(
> > - hkey_handle, ¤t_mode,
> > - "GTRW", "dd", 0)) {
> > - pr_err("Cannot read adaptive keyboard mode\n");
> > + current_mode = adaptive_keyboard_get_mode();
> > + if (current_mode < 0)
> > return false;
> > - } else {
> > - new_mode = adaptive_keyboard_get_next_mode(
> > - current_mode);
> > - }
> > + new_mode = adaptive_keyboard_get_next_mode(
> > + current_mode);
>
> This now fits on one line I believe.
Nope, 81 characters. I've kept it as-is.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-02-20 13:54 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-02-18 20:53 [PATCH 2/6] thinkpad_acpi: Factor out get/set adaptive kbd mode Bastien Nocera
2015-02-20 5:22 ` Darren Hart
2015-02-20 13:54 ` Bastien Nocera
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).