Message ID | 1450982818-11694-1-git-send-email-pali.rohar@gmail.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
On Thu, 24 Dec 2015, Pali Rohár wrote: > This patch adds support for controlling keyboard backlight via standard > linux led class interface (::kbd_backlight). It uses ACPI HKEY device with > MLCG and MLCS methods. > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com> > Tested-by: Fabio D'Urso <fabiodurso@hotmail.it> It looks good at a first glance. But at init you might need to take steps so that the current state is not changed. Did you test this? Also, is it working properly across suspend+resume?
On Monday 28 December 2015 00:14:17 Henrique de Moraes Holschuh wrote: > On Thu, 24 Dec 2015, Pali Rohár wrote: > > This patch adds support for controlling keyboard backlight via > > standard linux led class interface (::kbd_backlight). It uses ACPI > > HKEY device with MLCG and MLCS methods. > > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com> > > Tested-by: Fabio D'Urso <fabiodurso@hotmail.it> > > It looks good at a first glance. But at init you might need to take > steps so that the current state is not changed. Did you test this? Tested. Modprobing driver does not change keyboard backlight level. > Also, is it working properly across suspend+resume? When doing resume from suspend or hibernate BIOS turning keyboard backlight automatically off. But driver at any time report correct backlight level from sysfs node.
On Monday 28 December 2015 15:48:14 Pali Rohár wrote: > > Also, is it working properly across suspend+resume? > > When doing resume from suspend or hibernate BIOS turning keyboard > backlight automatically off. Fixed in v2.
Hi1 > This patch adds support for controlling keyboard backlight via standard > linux led class interface (::kbd_backlight). It uses ACPI HKEY device with > MLCG and MLCS methods. > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com> > Tested-by: Fabio D'Urso <fabiodurso@hotmail.it> On my thinkpad, keyboard light is controlled by /sys/class/leds/tpacpi\:\:thinklight/brightness (that's a bad name). On n900, it is .../leds/kb0..kb6. Now we'd have kbd_backlight. I guess we should standartize on one name for this light, so that userspace has the chance to handle it automatically... Also, neccessity of workqueues for LED setting is slowly being removed from the kernel, see LED mailing list for details. Thanks, Pavel
On Monday 04 January 2016 21:12:31 Pavel Machek wrote: > Hi1 > > > This patch adds support for controlling keyboard backlight via > > standard linux led class interface (::kbd_backlight). It uses ACPI > > HKEY device with MLCG and MLCS methods. > > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com> > > Tested-by: Fabio D'Urso <fabiodurso@hotmail.it> > > On my thinkpad, keyboard light is controlled by > > /sys/class/leds/tpacpi\:\:thinklight/brightness > > (that's a bad name). Hi! That is light in upper case of bezel/display, right? Thinklight is probably official marketing name for that by IBM/Lenovo. My patch adds support for keyboard backlight (light under the keyboard). > On n900, it is .../leds/kb0..kb6. Now we'd have kbd_backlight. I > guess we should standartize on one name for this light, so that > userspace has the chance to handle it automatically... Looks like userspace already uses /sys/class/leds/*::kbd_backlight for keyboard backlight (light under the keyboard). At least other drivers uses this name and my KDE desktop recognized "dell::kbd_backlight" (from dell-laptop.ko) and "tpacpi::kbd_backlight" too. So really for keyboard backlight use *::kbd_backlight it is already handled by existing userspace applications. > Also, neccessity of workqueues for LED setting is slowly being > removed from the kernel, see LED mailing list for details. My patch uses kbd_backlight LED in same as as other LEDs in thinkpad acpi driver.
On Monday 04 January 2016 21:23:52 Pali Rohár wrote: > Looks like userspace already uses /sys/class/leds/*::kbd_backlight > for keyboard backlight (light under the keyboard). At least other > drivers uses this name and my KDE desktop recognized > "dell::kbd_backlight" (from dell-laptop.ko) and > "tpacpi::kbd_backlight" too. > > So really for keyboard backlight use *::kbd_backlight it is already > handled by existing userspace applications. upower is one of tool used on linux desktops. See this source code: http://cgit.freedesktop.org/upower/tree/src/up-kbd-backlight.c It has function up_kbd_backlight_find() which do: /* find a led device that is a keyboard device */ while ((filename = g_dir_read_name (dir)) != NULL) { if (g_strstr_len (filename, -1, "kbd_backlight") != NULL) { dir_path = g_build_filename ("/sys/class/leds", filename, NULL); break; } }
On Wednesday 30 December 2015 23:28:48 Pali Rohár wrote: > On Monday 28 December 2015 15:48:14 Pali Rohár wrote: > > > Also, is it working properly across suspend+resume? > > > > When doing resume from suspend or hibernate BIOS turning keyboard > > backlight automatically off. > > Fixed in v2. > Now I see that BIOS try to be too intelligent and automatically turn of keyboard backlight when LID is closed. When LID is open again then keyboard backlight stay turned off. Sysfs show correct state (brightness is zero). Should thinkpad acpi driver do something? Or let BIOS do that job?
On Wed, Jan 6, 2016, at 06:45, Pali Rohár wrote: > On Wednesday 30 December 2015 23:28:48 Pali Rohár wrote: > > On Monday 28 December 2015 15:48:14 Pali Rohár wrote: > > > > Also, is it working properly across suspend+resume? > > > > > > When doing resume from suspend or hibernate BIOS turning keyboard > > > backlight automatically off. > > > > Fixed in v2. > > > > Now I see that BIOS try to be too intelligent and automatically turn of > keyboard backlight when LID is closed. When LID is open again then > keyboard backlight stay turned off. Sysfs show correct state (brightness > is zero). > > Should thinkpad acpi driver do something? Or let BIOS do that job? Either is fine, as long as you ensure the driver knows the BIOS turned off the backlight, which you did. So, really, I don't mind if we follow the BIOS.
On Mon, Jan 4, 2016, at 18:12, Pavel Machek wrote: > > This patch adds support for controlling keyboard backlight via standard > > linux led class interface (::kbd_backlight). It uses ACPI HKEY device with > > MLCG and MLCS methods. > > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com> > > Tested-by: Fabio D'Urso <fabiodurso@hotmail.it> > > On my thinkpad, keyboard light is controlled by > > /sys/class/leds/tpacpi\:\:thinklight/brightness > > (that's a bad name). That's because the driver was not updated to support your laptop, AND I don't recall if someone mapped the full behavior of the ACPI thinklight interface in your thinkpad :-(
On Sat, Jan 9, 2016, at 15:39, Henrique de Moraes Holschuh wrote: > On Mon, Jan 4, 2016, at 18:12, Pavel Machek wrote: > > > This patch adds support for controlling keyboard backlight via standard > > > linux led class interface (::kbd_backlight). It uses ACPI HKEY device with > > > MLCG and MLCS methods. > > > > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com> > > > Tested-by: Fabio D'Urso <fabiodurso@hotmail.it> > > > > On my thinkpad, keyboard light is controlled by > > > > /sys/class/leds/tpacpi\:\:thinklight/brightness > > > > (that's a bad name). > > That's because the driver was not updated to support your laptop, AND I > don't recall if someone mapped the full behavior of the ACPI thinklight > interface in your thinkpad :-( Argh. If by "keyboard light" you mean the LED above the *screen* that shines down on the whole keyboard, please disregard my previous reply... As for the naming, the idea of a LED up there shining in the keyboard is: 1. patented by IBM 2. named "ThinkLight" by IBM, one of the "Think Technologies" in the "ThinkPad" (add TM after everything :p) and every old-timer thinkpad user knew it by that name. 4. called "thinklight" by the driver since before the LED sysfs class even existed :p ibm-acpi, since then renamed thinkpad-acpi *predates* most generic interfaces. Heck, it predates sysfs. So, this is ABI set in stone. If there is a way to add an "alias" of kbd_backlight that won't drive userspace crazy, we might do that though. But it looks quite risky to me...
On Sat, Jan 09, 2016 at 03:46:41PM -0200, Henrique de Moraes Holschuh wrote: > On Sat, Jan 9, 2016, at 15:39, Henrique de Moraes Holschuh wrote: > > On Mon, Jan 4, 2016, at 18:12, Pavel Machek wrote: > > > > This patch adds support for controlling keyboard backlight via standard > > > > linux led class interface (::kbd_backlight). It uses ACPI HKEY device with > > > > MLCG and MLCS methods. > > > > > > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com> > > > > Tested-by: Fabio D'Urso <fabiodurso@hotmail.it> > > > > > > On my thinkpad, keyboard light is controlled by > > > > > > /sys/class/leds/tpacpi\:\:thinklight/brightness > > > > > > (that's a bad name). > > > > That's because the driver was not updated to support your laptop, AND I > > don't recall if someone mapped the full behavior of the ACPI thinklight > > interface in your thinkpad :-( > > Argh. If by "keyboard light" you mean the LED above the *screen* that > shines down on the whole keyboard, please disregard my previous reply... > > As for the naming, the idea of a LED up there shining in the keyboard > is: > 1. patented by IBM > 2. named "ThinkLight" by IBM, one of the "Think Technologies" in the > "ThinkPad" (add TM after everything :p) and every > old-timer thinkpad user knew it by that name. > 4. called "thinklight" by the driver since before the LED sysfs class > even existed :p > > ibm-acpi, since then renamed thinkpad-acpi *predates* most generic > interfaces. Heck, it predates sysfs. > > So, this is ABI set in stone. If there is a way to add an "alias" of > kbd_backlight that won't drive userspace crazy, we might do that though. > But it looks quite risky to me... Henrique, so are you taking back your Ack from 10 minutes prior? I've dropped this patch. Please let me know if I should pick it back up.
On Mon, Jan 11, 2016, at 17:04, Darren Hart wrote: > On Sat, Jan 09, 2016 at 03:46:41PM -0200, Henrique de Moraes Holschuh > wrote: > > On Sat, Jan 9, 2016, at 15:39, Henrique de Moraes Holschuh wrote: > > > On Mon, Jan 4, 2016, at 18:12, Pavel Machek wrote: > > > > > This patch adds support for controlling keyboard backlight via standard > > > > > linux led class interface (::kbd_backlight). It uses ACPI HKEY device with > > > > > MLCG and MLCS methods. > > > > > > > > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com> > > > > > Tested-by: Fabio D'Urso <fabiodurso@hotmail.it> > > > > > > > > On my thinkpad, keyboard light is controlled by > > > > > > > > /sys/class/leds/tpacpi\:\:thinklight/brightness > > > > > > > > (that's a bad name). > > > > > > That's because the driver was not updated to support your laptop, AND I > > > don't recall if someone mapped the full behavior of the ACPI thinklight > > > interface in your thinkpad :-( > > > > Argh. If by "keyboard light" you mean the LED above the *screen* that > > shines down on the whole keyboard, please disregard my previous reply... > > > > As for the naming, the idea of a LED up there shining in the keyboard > > is: > > 1. patented by IBM > > 2. named "ThinkLight" by IBM, one of the "Think Technologies" in the > > "ThinkPad" (add TM after everything :p) and every > > old-timer thinkpad user knew it by that name. > > 4. called "thinklight" by the driver since before the LED sysfs class > > even existed :p > > > > ibm-acpi, since then renamed thinkpad-acpi *predates* most generic > > interfaces. Heck, it predates sysfs. > > > > So, this is ABI set in stone. If there is a way to add an "alias" of > > kbd_backlight that won't drive userspace crazy, we might do that though. > > But it looks quite risky to me... > > Henrique, so are you taking back your Ack from 10 minutes prior? Hmm? No, the ACK stands. Pavel was talking about another feature altogether, apparently: older thinkpads did not have "keyboard backlight" (as in light from below the keys). They had a "ThinkLight", which is an overhead light that shines down on the keyboard. The two features are not the same (and are handled differently by the firmware, for whatever reason), although they do serve the same purpose. I don't think a thinkpad will ever have both features at the same time, so I have no idea why they changed the firmware interface. The patch adds support to the "keyboard backlight" feature, which was previously NOT supported. > I've dropped this patch. Please let me know if I should pick it back up. Please pick it back up.
On Monday 11 January 2016 20:28:00 Henrique de Moraes Holschuh wrote > The two features are not the same (and are handled differently by the > firmware, for whatever reason), although they do serve the same > purpose. I don't think a thinkpad will ever have both features at > the same time, so I have no idea why they changed the firmware > interface. Maybe we should decide if ::kbd_backlight LED suffix could be used also for other LED devices and not only for those which are physically under the keyboard. At least I understand "keyboard backlight" as light which is under the keyboard... And more important, I was told that ThinkPad x230 comes in variant with both ThinkLight (that LED upper the display) and keyboard backlight (under they keyboard). So I'm against merging ThinkLight and keyboard backlight to one sysfs path...
On Mon, Jan 11, 2016 at 09:03:01PM +0100, Pali Rohár wrote: > On Monday 11 January 2016 20:28:00 Henrique de Moraes Holschuh wrote > > The two features are not the same (and are handled differently by the > > firmware, for whatever reason), although they do serve the same > > purpose. I don't think a thinkpad will ever have both features at > > the same time, so I have no idea why they changed the firmware > > interface. ... > And more important, I was told that ThinkPad x230 comes in variant with > both ThinkLight (that LED upper the display) and keyboard backlight > (under they keyboard). I can confirm this, my x230 has both. FWIW, BIOS hotkey Fn+Space cycles through off, dim backlight, full backlight and full thinklight. /sys/class/leds/tpacpi::thinklight/brightness can be read (showing either 0 or 255) but writes are ignored. Dito for /proc/acpi/ibm/light. Johannes -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On lun., 2016-01-11 at 17:28 -0200, Henrique de Moraes Holschuh wrote: > I don't think a thinkpad will ever have both features at the same time, > so I have no idea why they changed the firmware interface. At least the X230 (and I guess other ThinkPads from the *30 generation) have both ThinkLight and keyboard backlight. Regards,
On Mon, Jan 11, 2016, at 18:03, Pali Rohár wrote: > On Monday 11 January 2016 20:28:00 Henrique de Moraes Holschuh wrote > > The two features are not the same (and are handled differently by the > > firmware, for whatever reason), although they do serve the same > > purpose. I don't think a thinkpad will ever have both features at > > the same time, so I have no idea why they changed the firmware > > interface. > > Maybe we should decide if ::kbd_backlight LED suffix could be used also > for other LED devices and not only for those which are physically under > the keyboard. Maybe, but that doesn't matter for thinkpad-acpi: the "thinklight" LED name is kernel-userspace ABI so frozen by years and years of use that it is not even funny. It cannot be changed. > At least I understand "keyboard backlight" as light which is under the > keyboard... So do I... > And more important, I was told that ThinkPad x230 comes in variant with > both ThinkLight (that LED upper the display) and keyboard backlight > (under they keyboard). Ok, so there is yet another damn good reason to keep both separate.
On Mon, Jan 11, 2016, at 19:12, Johannes Stezenbach wrote: > On Mon, Jan 11, 2016 at 09:03:01PM +0100, Pali Rohár wrote: > > On Monday 11 January 2016 20:28:00 Henrique de Moraes Holschuh wrote > > > The two features are not the same (and are handled differently by the > > > firmware, for whatever reason), although they do serve the same > > > purpose. I don't think a thinkpad will ever have both features at > > > the same time, so I have no idea why they changed the firmware > > > interface. > ... > > And more important, I was told that ThinkPad x230 comes in variant with > > both ThinkLight (that LED upper the display) and keyboard backlight > > (under they keyboard). > > I can confirm this, my x230 has both. FWIW, BIOS hotkey > Fn+Space cycles through off, dim backlight, full backlight > and full thinklight. /sys/class/leds/tpacpi::thinklight/brightness > can be read (showing either 0 or 255) but writes are ignored. > Dito for /proc/acpi/ibm/light. We likely need to integrate better (future work) the new backlight control with the thinklight control and the firmware interface in the x230. Is the ACPI AML for fn+space readable enough, or is it trapping directly into SMM? Because if it interacts with the traditional higher-level ACPI AML we already use to talk to the thinklight (and now to the backlight), it should be relatively easy to fix the driver to better support the x230.
On mar., 2016-01-12 at 14:07 -0200, Henrique de Moraes Holschuh wrote: > We likely need to integrate better (future work) the new backlight > control with the thinklight control and the firmware interface in the > x230. Ok so I've just checked my work X230, and I was just wrong: no ThinkLight on it, although I really had the impression there /was/ a dual generation. Regards,
On Tue, Jan 12, 2016 at 02:07:10PM -0200, Henrique de Moraes Holschuh wrote: > On Mon, Jan 11, 2016, at 19:12, Johannes Stezenbach wrote: > > > > I can confirm this, my x230 has both. FWIW, BIOS hotkey > > Fn+Space cycles through off, dim backlight, full backlight > > and full thinklight. /sys/class/leds/tpacpi::thinklight/brightness > > can be read (showing either 0 or 255) but writes are ignored. > > Dito for /proc/acpi/ibm/light. > > We likely need to integrate better (future work) the new backlight > control with the thinklight control and the firmware interface in the > x230. > > Is the ACPI AML for fn+space readable enough, or is it trapping directly > into SMM? Because if it interacts with the traditional higher-level > ACPI AML we already use to talk to the thinklight (and now to the > backlight), it should be relatively easy to fix the driver to better > support the x230. I have no clue about ACPI, do you have some hints how to get the info you want? I found this: sudo acpidump > acpidata.dat acpixtract -sSSDT acpidata.dat acpixtract -sDSDT acpidata.dat iasl -d DSDT.dat SSDT*.dat ..and then look at the .dsl files that contain the AML assembler. But what to look out for? PS: failed to mention my x230 kernel was still 4.2.x in case it matters Thanks, Johannes -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 11, 2016 at 05:28:00PM -0200, Henrique de Moraes Holschuh wrote: > > > Henrique, so are you taking back your Ack from 10 minutes prior? > > Hmm? No, the ACK stands. > > Pavel was talking about another feature altogether, apparently: older > thinkpads did not have "keyboard backlight" (as in light from below the > keys). They had a "ThinkLight", which is an overhead light that shines > down on the keyboard. > > The two features are not the same (and are handled differently by the > firmware, for whatever reason), although they do serve the same purpose. > I don't think a thinkpad will ever have both features at the same time, > so I have no idea why they changed the firmware interface. > > The patch adds support to the "keyboard backlight" feature, which was > previously NOT supported. > > > I've dropped this patch. Please let me know if I should pick it back up. > > Please pick it back up. > Gah, these two threads landed next to each other in my Inbox and I didn't pick up the split. Apologies. This is queued in testing (again).
On Tue, Jan 12, 2016, at 14:59, Darren Hart wrote: > Gah, these two threads landed next to each other in my Inbox and I didn't > pick > up the split. Apologies. > > This is queued in testing (again). Thanks!
On Tue, Jan 12, 2016, at 14:35, Johannes Stezenbach wrote: > On Tue, Jan 12, 2016 at 02:07:10PM -0200, Henrique de Moraes Holschuh > wrote: > > Is the ACPI AML for fn+space readable enough, or is it trapping directly > > into SMM? Because if it interacts with the traditional higher-level > > ACPI AML we already use to talk to the thinklight (and now to the > > backlight), it should be relatively easy to fix the driver to better > > support the x230. > > I have no clue about ACPI, do you have some hints > how to get the info you want? I found this: > > sudo acpidump > acpidata.dat > acpixtract -sSSDT acpidata.dat > acpixtract -sDSDT acpidata.dat > iasl -d DSDT.dat SSDT*.dat > > ..and then look at the .dsl files that contain the AML assembler. > > But what to look out for? Well, you should familiarize yourself with ACPI AML a bit. Then, look for the ACPI methods that thinkpad-acpi (and the patch to add backlight support) calls, and try to work out how they work in your thinkpad. Typical thinkpad firmware behavior is to sometimes have a lower level implementation, a higher level ACPI API that the O.S. is supposed to call (and which will call into the lower level implementation, or to the work itself). We want to stick to the higher level API, as it is more stable. Use of lower-level APIs or direct EC access must be whitelist-restricted. > PS: failed to mention my x230 kernel was still 4.2.x in case it matters It doesn't.
On 01/12/2016 08:07 AM, Henrique de Moraes Holschuh wrote: > On Mon, Jan 11, 2016, at 19:12, Johannes Stezenbach wrote: >> I can confirm this, my x230 has both. FWIW, BIOS hotkey >> Fn+Space cycles through off, dim backlight, full backlight >> and full thinklight. /sys/class/leds/tpacpi::thinklight/brightness >> can be read (showing either 0 or 255) but writes are ignored. >> Dito for /proc/acpi/ibm/light. > > We likely need to integrate better (future work) the new backlight > control with the thinklight control and the firmware interface in the > x230. > > Is the ACPI AML for fn+space readable enough, or is it trapping directly > into SMM? Because if it interacts with the traditional higher-level > ACPI AML we already use to talk to the thinklight (and now to the > backlight), it should be relatively easy to fix the driver to better > support the x230. I looked through the DSDT and SSDT AML on the T430 (which also has both a keyboard backlight and ThinkLight) and couldn't deduce which method was being invoked for Fn+Space or an analog to MLCG/MLCS for the ThinkLight. However, that may be due to my inexperience interpreting AML. I'll keep looking/learning. I've posted the files online if anyone else has time/interest to take a look: https://kevinlocke.name/misc/t430-acpi/ Best regards, Kevin -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 12, 2016, at 16:11, Kevin Locke wrote: > I looked through the DSDT and SSDT AML on the T430 (which also has both > a keyboard backlight and ThinkLight) and couldn't deduce which method > was being invoked for Fn+Space or an analog to MLCG/MLCS for the > ThinkLight. However, that may be due to my inexperience interpreting > AML. I'll keep looking/learning. It calls directly into the SMBIOS :-(
On 01/12/2016 10:20 AM, Henrique de Moraes Holschuh wrote: > On Tue, Jan 12, 2016, at 16:11, Kevin Locke wrote: >> I looked through the DSDT and SSDT AML on the T430 (which also has both >> a keyboard backlight and ThinkLight) and couldn't deduce which method >> was being invoked for Fn+Space or an analog to MLCG/MLCS for the >> ThinkLight. However, that may be due to my inexperience interpreting >> AML. I'll keep looking/learning. > > It calls directly into the SMBIOS :-( Bummer! Thank you for taking the time to figure that out. Kevin -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi! On Mon 2016-01-11 21:03:01, Pali Rohár wrote: > On Monday 11 January 2016 20:28:00 Henrique de Moraes Holschuh wrote > > The two features are not the same (and are handled differently by the > > firmware, for whatever reason), although they do serve the same > > purpose. I don't think a thinkpad will ever have both features at > > the same time, so I have no idea why they changed the firmware > > interface. > > Maybe we should decide if ::kbd_backlight LED suffix could be used also > for other LED devices and not only for those which are physically under > the keyboard. Another problem is that N900 has _6_ backlight LEDs. Named lp5523::kb1..6. ... Which does means desktop software will probably not pick them up :-(. I guess we could have "/sys/class/kbd_light/brightness" that would control all of them with one write. Next question is.. apparently there are some keyboards that have per-key RGB backlight... but maybe we can just call that "weird enough" and ignore... Pavel
On Tue, Jan 12, 2016 at 03:56:07PM -0200, Henrique de Moraes Holschuh wrote: > On Tue, Jan 12, 2016, at 14:35, Johannes Stezenbach wrote: > > But what to look out for? > > Well, you should familiarize yourself with ACPI AML a bit. Then, look > for the ACPI methods that thinkpad-acpi (and the patch to add backlight > support) calls, and try to work out how they work in your thinkpad. > > Typical thinkpad firmware behavior is to sometimes have a lower level > implementation, a higher level ACPI API that the O.S. is supposed to > call (and which will call into the lower level implementation, or to the > work itself). > > We want to stick to the higher level API, as it is more stable. Use of > lower-level APIs or direct EC access must be whitelist-restricted. So if I interpret this correctly it calls SMI BIOS on x230, too: Method (MLCG, 1, NotSerialized) { Local0 = \KBLS (0x00, 0x00) Return (Local0) } Method (MLCS, 1, NotSerialized) { Local0 = \KBLS (0x01, Arg0) If (!(Local0 & 0x80000000)) { If ((Arg0 & 0x00010000)) { \_SB.PCI0.LPC.EC.HKEY.MHKQ (0x6001) } ElseIf (\_SB.PCI0.LPC.EC.HKEY.MHKK (0x00020000)) { \_SB.PCI0.LPC.EC.HKEY.MHKQ (0x1012) } } Return (Local0) } Method (KBLS, 2, NotSerialized) { Return (SMI (0x14, 0x09, Arg0, Arg1, 0x00)) } FWIW, full dump is here: https://linuxtv.org/~js/ahSheaw0eezahqu1aethohLae4MeiPhe/ Johannes -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 12 January 2016 22:58:04 Pavel Machek wrote: > Hi! > > On Mon 2016-01-11 21:03:01, Pali Rohár wrote: > > On Monday 11 January 2016 20:28:00 Henrique de Moraes Holschuh wrote > > > The two features are not the same (and are handled differently by the > > > firmware, for whatever reason), although they do serve the same > > > purpose. I don't think a thinkpad will ever have both features at > > > the same time, so I have no idea why they changed the firmware > > > interface. > > > > Maybe we should decide if ::kbd_backlight LED suffix could be used also > > for other LED devices and not only for those which are physically under > > the keyboard. > > Another problem is that N900 has _6_ backlight LEDs. Named > lp5523::kb1..6. ... Which does means desktop software will probably > not pick them up :-(. > > I guess we could have "/sys/class/kbd_light/brightness" that would > control all of them with one write. Probably... But there is problem that lp5523 is not ordinary on/off light, it can be programmed to execute own "light" application. > Next question is.. apparently there are some keyboards that have > per-key RGB backlight... but maybe we can just call that "weird > enough" and ignore... First we need to defines stable kernel ABI for keyboard backlight. And I suggest to use existing convention used by upower/console-kit and other userspace apps...
On Wed 2016-01-13 09:54:55, Pali Rohár wrote: > On Tuesday 12 January 2016 22:58:04 Pavel Machek wrote: > > Hi! > > > > On Mon 2016-01-11 21:03:01, Pali Rohár wrote: > > > On Monday 11 January 2016 20:28:00 Henrique de Moraes Holschuh wrote > > > > The two features are not the same (and are handled differently by the > > > > firmware, for whatever reason), although they do serve the same > > > > purpose. I don't think a thinkpad will ever have both features at > > > > the same time, so I have no idea why they changed the firmware > > > > interface. > > > > > > Maybe we should decide if ::kbd_backlight LED suffix could be used also > > > for other LED devices and not only for those which are physically under > > > the keyboard. > > > > Another problem is that N900 has _6_ backlight LEDs. Named > > lp5523::kb1..6. ... Which does means desktop software will probably > > not pick them up :-(. > > > > I guess we could have "/sys/class/kbd_light/brightness" that would > > control all of them with one write. > > Probably... But there is problem that lp5523 is not ordinary on/off > light, it can be programmed to execute own "light" application. Yeah, but that's not a problem, right. lp5523 can still set brightness by hand. > > Next question is.. apparently there are some keyboards that have > > per-key RGB backlight... but maybe we can just call that "weird > > enough" and ignore... > > First we need to defines stable kernel ABI for keyboard backlight. And I > suggest to use existing convention used by upower/console-kit and other > userspace apps... Hmm... I'm not sure that can be done. What were the masks used by upower again? Will upower write to all 6 leds if we present them? Pavel
On Wed 2016-01-13 20:07:36, Pavel Machek wrote: > On Wed 2016-01-13 09:54:55, Pali Rohár wrote: > > On Tuesday 12 January 2016 22:58:04 Pavel Machek wrote: > > > Hi! > > > > > > Next question is.. apparently there are some keyboards that have > > > per-key RGB backlight... but maybe we can just call that "weird > > > enough" and ignore... > > > > First we need to defines stable kernel ABI for keyboard backlight. And I > > suggest to use existing convention used by upower/console-kit and other > > userspace apps... > > Hmm... I'm not sure that can be done. What were the masks used by > upower again? Will upower write to all 6 leds if we present them? Got it... It has function up_kbd_backlight_find() which do: /* find a led device that is a keyboard device */ while ((filename = g_dir_read_name (dir)) != NULL) { if (g_strstr_len (filename, -1, "kbd_backlight") != NULL) { dir_path = g_build_filename ("/sys/class/leds", filename, NULL); break; That suggests that it stops at the first matching device. Adding new "virtual" led controlling 6 physical leds would be ugly. So... new interface should be done. Pavel
On Wednesday 13 January 2016 20:10:30 Pavel Machek wrote: > On Wed 2016-01-13 20:07:36, Pavel Machek wrote: > > On Wed 2016-01-13 09:54:55, Pali Rohár wrote: > > > On Tuesday 12 January 2016 22:58:04 Pavel Machek wrote: > > > > Hi! > > > > > > > > > Next question is.. apparently there are some keyboards that have > > > > per-key RGB backlight... but maybe we can just call that "weird > > > > enough" and ignore... > > > > > > First we need to defines stable kernel ABI for keyboard backlight. And I > > > suggest to use existing convention used by upower/console-kit and other > > > userspace apps... > > > > Hmm... I'm not sure that can be done. What were the masks used by > > upower again? Will upower write to all 6 leds if we present them? > > Got it... > > It has function up_kbd_backlight_find() which do: > > /* find a led device that is a keyboard device */ > while ((filename = g_dir_read_name (dir)) != NULL) { > if (g_strstr_len (filename, -1, "kbd_backlight") != NULL) { > dir_path = g_build_filename ("/sys/class/leds", > filename, > NULL); > break; > > That suggests that it stops at the first matching device. Adding new > "virtual" led controlling 6 physical leds would be ugly. So... new > interface should be done. > > Pavel Yes, it would be ugly, but lp5523 is already ugly... it can accept numeric value, trigger and also program in bytecode either via sysfs or via request_firmware... Another virtual led control should not be problem for this :-)
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c index 0bed473..477afa1 100644 --- a/drivers/platform/x86/thinkpad_acpi.c +++ b/drivers/platform/x86/thinkpad_acpi.c @@ -303,6 +303,7 @@ static struct { u32 hotkey_mask:1; u32 hotkey_wlsw:1; u32 hotkey_tablet:1; + u32 kbdlight:1; u32 light:1; u32 light_status:1; u32 bright_acpimode:1; @@ -4986,6 +4987,206 @@ static struct ibm_struct video_driver_data = { #endif /* CONFIG_THINKPAD_ACPI_VIDEO */ /************************************************************************* + * Keyboard backlight subdriver + */ + +static int kbdlight_set_level(int level) +{ + if (!hkey_handle) + return -ENXIO; + + if (!acpi_evalf(hkey_handle, NULL, "MLCS", "dd", level)) + return -EIO; + + return 0; +} + +static int kbdlight_get_level(void) +{ + int status = 0; + + if (!hkey_handle) + return -ENXIO; + + if (!acpi_evalf(hkey_handle, &status, "MLCG", "dd", 0)) + return -EIO; + + if (status < 0) + return status; + + return status & 0x3; +} + +static bool kbdlight_is_supported(void) +{ + int status = 0; + + if (!hkey_handle) + return false; + + if (!acpi_has_method(hkey_handle, "MLCG")) { + vdbg_printk(TPACPI_DBG_INIT, "kbdlight MLCG is unavailable\n"); + return false; + } + + if (!acpi_evalf(hkey_handle, &status, "MLCG", "qdd", 0)) { + vdbg_printk(TPACPI_DBG_INIT, "kbdlight MLCG failed\n"); + return false; + } + + if (status < 0) { + vdbg_printk(TPACPI_DBG_INIT, "kbdlight MLCG err: %d\n", status); + return false; + } + + vdbg_printk(TPACPI_DBG_INIT, "kbdlight MLCG returned 0x%x\n", status); + /* + * Guessed test for keyboard backlight: + * + * Machines with backlight keyboard return: + * b010100000010000000XX - ThinkPad X1 Carbon 3rd + * b110100010010000000XX - ThinkPad x230 + * b010100000010000000XX - ThinkPad x240 + * b010100000010000000XX - ThinkPad W541 + * (XX is current backlight level) + * + * Machines without backlight keyboard return: + * b10100001000000000000 - ThinkPad x230 + * b10110001000000000000 - ThinkPad E430 + * b00000000000000000000 - ThinkPad E450 + * + * Candidate BITs for detection test (XOR): + * b01000000001000000000 + * ^ + */ + return status & BIT(9); +} + +static void kbdlight_set_worker(struct work_struct *work) +{ + struct tpacpi_led_classdev *data = + container_of(work, struct tpacpi_led_classdev, work); + + if (likely(tpacpi_lifecycle == TPACPI_LIFE_RUNNING)) + kbdlight_set_level(data->new_state); +} + +static void kbdlight_sysfs_set(struct led_classdev *led_cdev, + enum led_brightness brightness) +{ + struct tpacpi_led_classdev *data = + container_of(led_cdev, + struct tpacpi_led_classdev, + led_classdev); + data->new_state = brightness; + queue_work(tpacpi_wq, &data->work); +} + +static enum led_brightness kbdlight_sysfs_get(struct led_classdev *led_cdev) +{ + int level; + + level = kbdlight_get_level(); + if (level < 0) + return 0; + + return level; +} + +static struct tpacpi_led_classdev tpacpi_led_kbdlight = { + .led_classdev = { + .name = "tpacpi::kbd_backlight", + .max_brightness = 2, + .brightness_set = &kbdlight_sysfs_set, + .brightness_get = &kbdlight_sysfs_get, + } +}; + +static int __init kbdlight_init(struct ibm_init_struct *iibm) +{ + int rc; + + vdbg_printk(TPACPI_DBG_INIT, "initializing kbdlight subdriver\n"); + + TPACPI_ACPIHANDLE_INIT(hkey); + INIT_WORK(&tpacpi_led_kbdlight.work, kbdlight_set_worker); + + if (!kbdlight_is_supported()) { + tp_features.kbdlight = 0; + vdbg_printk(TPACPI_DBG_INIT, "kbdlight is unsupported\n"); + return 1; + } + + tp_features.kbdlight = 1; + + rc = led_classdev_register(&tpacpi_pdev->dev, + &tpacpi_led_kbdlight.led_classdev); + if (rc < 0) { + tp_features.kbdlight = 0; + return rc; + } + + return 0; +} + +static void kbdlight_exit(void) +{ + if (tp_features.kbdlight) + led_classdev_unregister(&tpacpi_led_kbdlight.led_classdev); + flush_workqueue(tpacpi_wq); +} + +static int kbdlight_read(struct seq_file *m) +{ + int level; + + if (!tp_features.kbdlight) { + seq_printf(m, "status:\t\tnot supported\n"); + } else { + level = kbdlight_get_level(); + if (level < 0) + seq_printf(m, "status:\t\terror %d\n", level); + else + seq_printf(m, "status:\t\t%d\n", level); + seq_printf(m, "commands:\t0, 1, 2\n"); + } + + return 0; +} + +static int kbdlight_write(char *buf) +{ + char *cmd; + int level = -1; + + if (!tp_features.kbdlight) + return -ENODEV; + + while ((cmd = next_cmd(&buf))) { + if (strlencmp(cmd, "0") == 0) + level = 0; + else if (strlencmp(cmd, "1") == 0) + level = 1; + else if (strlencmp(cmd, "2") == 0) + level = 2; + else + return -EINVAL; + } + + if (level == -1) + return -EINVAL; + + return kbdlight_set_level(level); +} + +static struct ibm_struct kbdlight_driver_data = { + .name = "kbdlight", + .read = kbdlight_read, + .write = kbdlight_write, + .exit = kbdlight_exit, +}; + +/************************************************************************* * Light (thinklight) subdriver */ @@ -9207,6 +9408,10 @@ static struct ibm_init_struct ibms_init[] __initdata = { }, #endif { + .init = kbdlight_init, + .data = &kbdlight_driver_data, + }, + { .init = light_init, .data = &light_driver_data, },