sony_laptop: Default values for keyboard backlight

sony_laptop: Default values for keyboard backlight

[Karol Babioch]

[-- Attachment #1: Type: text/plain, Size: 1701 bytes --]

Hi,

the keyboard backlight has been working for me for the last couple of
years without any problems and I was fine with the default values, so I
wasn't too happy when I realized that a recent kernel upgrade has
changed this by leaving the backlight always on.

I've tracked down the "issue" to a specific commit (see [1]) from Mattia
itself, so I guess he had every reason to make this change. I've dealt
with the "problem" with a specific modprobe.d file.

I'm wondering whether this is the right approach, though. The default
behavior before even any "sony-laptop" module is loaded at all (e.g.
within the BIOS and/or the bootloader selection menu) is to enable the
keyboard backlight with a timeout of 10 seconds - at least on my machine.

This has changed with the recent commit once the module is actually
loaded and the backlight is never turned off again. The sysfs interface
reports "-1" for both "kbd_backlight" and "kbd_backlight_timeout", which
in itself is fine according to the message describing the commit as the
values can't be known until having them set for the first time.

But to my understanding the commit shouldn't change the default behavior
for the keyboard backlight itself, as it actually doesn't touch the
registers involved. But as the behavior has actually changed, something
is definitely going on here, which might not necessarily be intentional.

So is this something that has been overlooked, or is my understanding of
the new behavior wrong?

Best regards,
Karol Babioch

[1]:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/platform/x86/sony-laptop.c?id=294d31e8227c9892a89d6b3e58d17886b79ea4e6


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: sony_laptop: Default values for keyboard backlight

[Mattia Dongili]

On Mon, Nov 18, 2013 at 11:48:07AM +0100, Karol Babioch wrote:
> Hi,
> 
> the keyboard backlight has been working for me for the last couple of
> years without any problems and I was fine with the default values, so I
> wasn't too happy when I realized that a recent kernel upgrade has
> changed this by leaving the backlight always on.
> 
> I've tracked down the "issue" to a specific commit (see [1]) from Mattia
> itself, so I guess he had every reason to make this change. I've dealt
> with the "problem" with a specific modprobe.d file.

This should work of course but if I broke the majority of older models
then maybe a different approach is a better idea.

...
> But to my understanding the commit shouldn't change the default behavior
> for the keyboard backlight itself, as it actually doesn't touch the

that's right, the new code specifically doesn't do aanything with
keyboard backlight unless you have a module parameter that sets a value.

> registers involved. But as the behavior has actually changed, something
> is definitely going on here, which might not necessarily be intentional.

The code was changed on the assumption that keyboard backlight settings
value are persisted across reboots. I guess this is not the case on your
machine. If you don't load sony-laptop module you still have the 10
seconds timeout that you see in the BIOS? Only _after_ you load the
module the backlight never goes off?

One alternative approach is the following.
From the DSDTs I have hanging around these models may see the same
behaviour as you're seeing:
VPCF127FX
VPCS11E7E
VPCS12A7R
VPCS12C5E
VPCZ12C5E
i.e. VPC F/S/Z series.
They all have handle 0x137 which is what the very original code was
developed for and, after all, 0x137 is special cased in the keyboard
backlight code.
One approach could be to make the default setting handle based with
again 0x137 being different from all others.

-- 
mattia
:wq!

Re: sony_laptop: Default values for keyboard backlight

[Karol Babioch]

[-- Attachment #1: Type: text/plain, Size: 2154 bytes --]

Hi,

Am 18.11.2013 23:30, schrieb Mattia Dongili:
> I guess this is not the case on your
> machine. If you don't load sony-laptop module you still have the 10
> seconds timeout that you see in the BIOS? Only _after_ you load the
> module the backlight never goes off?

After playing around with this for a while I think that I narrowed the
issue down (or at least one aspect of it):

So when actually rebooting and/or powering up the device for the first
time, it works just fine by default without setting any module
parameters explicitly. The keyboard backlight is enabled and the timeout
is set to 10 seconds, which is the default.

But as soon as I suspend the machine it absolutely makes a difference
whether or not the module parameter is set. When it is not set
explicitly (sysfs interface reporting "-1") the keyboard backlight will
stay on from the moment I resume the machine and press a button. When
the module parameters are set explicitly it works just fine and as
expected.

Does this make any sense from your perspective? Maybe it's not the whole
story, but only one aspect of it. Is this something you can check and/or
verify?

> From the DSDTs I have hanging around these models may see the same
> behaviour as you're seeing:

Can't deny or confirm this, but this would be a reasonable guess.
Personally I'm using a Sony Vaio VPCS12C5E. I might provide you the
appropriate DSDT, if it can provide you any more insights.

> One approach could be to make the default setting handle based with
> again 0x137 being different from all others.

Not sure what the right approach would be. When there is absolutely no
way of reading the value before it was set at least once, then I don't
see much of an alternative, but to set it explicitly for these models
with an option to overwrite the default values using the appropriate
module parameters.

You'll definitely have more experience in this field, so it's your call.
Furthermore there are a lot of other models you have to keep in mind ;).

Let me know if you come up with something you might want to test ;).

Best regards,
Karol Babioch


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: sony_laptop: Default values for keyboard backlight

[Mattia Dongili]

On Fri, Nov 22, 2013 at 12:05:10AM +0100, Karol Babioch wrote:
> Hi,
> 
> Am 18.11.2013 23:30, schrieb Mattia Dongili:
> > I guess this is not the case on your
> > machine. If you don't load sony-laptop module you still have the 10
> > seconds timeout that you see in the BIOS? Only _after_ you load the
> > module the backlight never goes off?
> 
> After playing around with this for a while I think that I narrowed the
> issue down (or at least one aspect of it):
> 
> So when actually rebooting and/or powering up the device for the first
> time, it works just fine by default without setting any module
> parameters explicitly. The keyboard backlight is enabled and the timeout
> is set to 10 seconds, which is the default.
> 
> But as soon as I suspend the machine it absolutely makes a difference
> whether or not the module parameter is set. When it is not set
> explicitly (sysfs interface reporting "-1") the keyboard backlight will
> stay on from the moment I resume the machine and press a button. When
> the module parameters are set explicitly it works just fine and as
> expected.

Ha! good catch, I missed this bit in the patch that you already
identified. Can you apply it and see if it makes a difference?

commit 0d72d73e594828381d8ea0291bffa5770fe020a2
Author: Mattia Dongili <malattia@linux.it>
Date:   Sat Nov 23 06:55:40 2013 +0900

    sony-laptop: do not poke the keyboard backlight on resume
    
    Follow-up to 294d31e8227c9892a89d6b3e58d17886b79ea4e6, avoid messing up
    the state on resume leaving it to what was before suspending as it's
    likely that we don't know what value we should write to the EC
    registers.
    
    Signed-off-by: Mattia Dongili <malattia@linux.it>

diff --git a/drivers/platform/x86/sony-laptop.c b/drivers/platform/x86/sony-laptop.c
index 8d80f2c..921b2a9 100644
--- a/drivers/platform/x86/sony-laptop.c
+++ b/drivers/platform/x86/sony-laptop.c
@@ -140,7 +140,6 @@ MODULE_PARM_DESC(kbd_backlight_timeout,
 		 "on the model (default: no change from current value)");
 
 #ifdef CONFIG_PM_SLEEP
-static void sony_nc_kbd_backlight_resume(void);
 static void sony_nc_thermal_resume(void);
 #endif
 static int sony_nc_kbd_backlight_setup(struct platform_device *pd,
@@ -1487,13 +1486,6 @@ static void sony_nc_function_resume(void)
 		case 0x0135:
 			sony_nc_rfkill_update();
 			break;
-		case 0x0137:
-		case 0x0143:
-		case 0x014b:
-		case 0x014c:
-		case 0x0163:
-			sony_nc_kbd_backlight_resume();
-			break;
 		default:
 			continue;
 		}
@@ -1902,25 +1894,6 @@ static void sony_nc_kbd_backlight_cleanup(struct platform_device *pd,
 	}
 }
 
-#ifdef CONFIG_PM_SLEEP
-static void sony_nc_kbd_backlight_resume(void)
-{
-	int ignore = 0;
-
-	if (!kbdbl_ctl)
-		return;
-
-	if (kbdbl_ctl->mode == 0)
-		sony_call_snc_handle(kbdbl_ctl->handle, kbdbl_ctl->base,
-				&ignore);
-
-	if (kbdbl_ctl->timeout != 0)
-		sony_call_snc_handle(kbdbl_ctl->handle,
-				(kbdbl_ctl->base + 0x200) |
-				(kbdbl_ctl->timeout << 0x10), &ignore);
-}
-#endif
-
 struct battery_care_control {
 	struct device_attribute attrs[2];
 	unsigned int handle;
-- 
mattia
:wq!

Re: sony_laptop: Default values for keyboard backlight

[Karol Babioch]

[-- Attachment #1: Type: text/plain, Size: 533 bytes --]

Hi,

Am 22.11.2013 23:11, schrieb Mattia Dongili:
Can you apply it and see if it makes a difference?

Yes, it seems to do the job just fine. Couldn't find any problems with
it so far. I'm using it without setting any parameters explicitly, and
it behaves just like beforehand (keyboard backlight on by default with a
10 second timeout).

So thanks for the fast turn around. I'm sorry that it took me so long to
actually test it. Hoping to see this being merged into the mainline kernel.

Best regards,
Karol Babioch


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH] sony-laptop: do not scribble keyboard backlight registers on resume

[Mattia Dongili]

Follow-up to 294d31e8227c9892a89d6b3e58d17886b79ea4e6: avoid messing up
the state on resume. Leave it to what was before suspending as it's
anyway likely that we still don't know what value we should write to the
EC registers.
This fix is also required in 3.12

Cc: stable@vger.kernel.org
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Tested-by: Karol Babioch <karol@babioch.de>
Signed-off-by: Mattia Dongili <malattia@linux.it>
---
 drivers/platform/x86/sony-laptop.c | 27 ---------------------------
 1 file changed, 27 deletions(-)

diff --git a/drivers/platform/x86/sony-laptop.c b/drivers/platform/x86/sony-laptop.c
index 8d80f2c..921b2a9 100644
--- a/drivers/platform/x86/sony-laptop.c
+++ b/drivers/platform/x86/sony-laptop.c
@@ -140,7 +140,6 @@ MODULE_PARM_DESC(kbd_backlight_timeout,
 		 "on the model (default: no change from current value)");
 
 #ifdef CONFIG_PM_SLEEP
-static void sony_nc_kbd_backlight_resume(void);
 static void sony_nc_thermal_resume(void);
 #endif
 static int sony_nc_kbd_backlight_setup(struct platform_device *pd,
@@ -1487,13 +1486,6 @@ static void sony_nc_function_resume(void)
 		case 0x0135:
 			sony_nc_rfkill_update();
 			break;
-		case 0x0137:
-		case 0x0143:
-		case 0x014b:
-		case 0x014c:
-		case 0x0163:
-			sony_nc_kbd_backlight_resume();
-			break;
 		default:
 			continue;
 		}
@@ -1902,25 +1894,6 @@ static void sony_nc_kbd_backlight_cleanup(struct platform_device *pd,
 	}
 }
 
-#ifdef CONFIG_PM_SLEEP
-static void sony_nc_kbd_backlight_resume(void)
-{
-	int ignore = 0;
-
-	if (!kbdbl_ctl)
-		return;
-
-	if (kbdbl_ctl->mode == 0)
-		sony_call_snc_handle(kbdbl_ctl->handle, kbdbl_ctl->base,
-				&ignore);
-
-	if (kbdbl_ctl->timeout != 0)
-		sony_call_snc_handle(kbdbl_ctl->handle,
-				(kbdbl_ctl->base + 0x200) |
-				(kbdbl_ctl->timeout << 0x10), &ignore);
-}
-#endif
-
 struct battery_care_control {
 	struct device_attribute attrs[2];
 	unsigned int handle;
-- 
1.8.4.4

Re: [PATCH] sony-laptop: do not scribble keyboard backlight registers on resume

[Linus Torvalds]

On Mon, Nov 25, 2013 at 2:43 PM, Mattia Dongili <malattia@linux.it> wrote:
> Follow-up to 294d31e8227c9892a89d6b3e58d17886b79ea4e6: avoid messing up
> the state on resume.

Tested fine here too, so I applied it directly to my tree since I have
the hardware..

              Linus