[PATCH] thinkpad_acpi: Add support for keyboard backlight

[PATCH] thinkpad_acpi: Add support for keyboard backlight

[Pali Rohár]

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>
---
 drivers/platform/x86/thinkpad_acpi.c |  205 ++++++++++++++++++++++++++++++++++
 1 file changed, 205 insertions(+)

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,
 	},
-- 
1.7.9.5

Re: [PATCH] thinkpad_acpi: Add support for keyboard backlight

[Henrique de Moraes Holschuh]

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?

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] thinkpad_acpi: Add support for keyboard backlight

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 905 bytes --]

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.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] thinkpad_acpi: Add support for keyboard backlight

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 276 bytes --]

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.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] thinkpad_acpi: Add support for keyboard backlight

[Pali Rohár]

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?

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH] thinkpad_acpi: Add support for keyboard backlight

[Henrique de Moraes Holschuh]

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.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

[PATCH v2] thinkpad_acpi: Add support for keyboard backlight

[Pali Rohár]

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>
---
Changes since v1:
* Added LED_CORE_SUSPENDRESUME to preserve led state across suspend/hibernate
---
 drivers/platform/x86/thinkpad_acpi.c |  206 ++++++++++++++++++++++++++++++++++
 1 file changed, 206 insertions(+)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 0bed473..a268a7a 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,207 @@ 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,
+		.flags		= LED_CORE_SUSPENDRESUME,
+	}
+};
+
+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 +9409,10 @@ static struct ibm_init_struct ibms_init[] __initdata = {
 	},
 #endif
 	{
+		.init = kbdlight_init,
+		.data = &kbdlight_driver_data,
+	},
+	{
 		.init = light_init,
 		.data = &light_driver_data,
 	},
-- 
1.7.9.5

Re: [PATCH v2] thinkpad_acpi: Add support for keyboard backlight

[Darren Hart]

On Wed, Dec 30, 2015 at 11:27:41PM +0100, 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.
> 

Which laptops is this intended to support?

Henrique, I'm holding off a bit more to give you time to respond given the
holiday season.

Thanks,

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v2] thinkpad_acpi: Add support for keyboard backlight

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 459 bytes --]

On Monday 04 January 2016 21:04:25 Darren Hart wrote:
> On Wed, Dec 30, 2015 at 11:27:41PM +0100, 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.
> 
> Which laptops is this intended to support?

Thinkpad ??30 series and new which have backlight keyboard.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v2] thinkpad_acpi: Add support for keyboard backlight

[Darren Hart]

On Mon, Jan 04, 2016 at 09:26:19PM +0100, Pali Rohár wrote:
> On Monday 04 January 2016 21:04:25 Darren Hart wrote:
> > On Wed, Dec 30, 2015 at 11:27:41PM +0100, 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.
> > 
> > Which laptops is this intended to support?
> 
> Thinkpad ??30 series and new which have backlight keyboard.

Thanks, we should include that in the commit message as well as the comments
surrounding the driver section.

Henrique, are your concerns surrounding suspend/resume resolved?

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v2] thinkpad_acpi: Add support for keyboard backlight

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1171 bytes --]

On Monday 04 January 2016 21:40:20 Darren Hart wrote:
> On Mon, Jan 04, 2016 at 09:26:19PM +0100, Pali Rohár wrote:
> > On Monday 04 January 2016 21:04:25 Darren Hart wrote:
> > > On Wed, Dec 30, 2015 at 11:27:41PM +0100, 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.
> > > 
> > > Which laptops is this intended to support?
> > 
> > Thinkpad ??30 series and new which have backlight keyboard.
> 
> Thanks, we should include that in the commit message as well as the
> comments surrounding the driver section.

??30 is probably not good characteristic, but I mean all those Thinkpad 
laptops like T430, x230, E430, X1 (1st) and their successors (T440, 
T450, X1 3rd, ...) All those which are from Ivy Bridge processor 
generation (and new).

But basically it cover all Thinkpad laptops which have backlight 
keyboard. Older Thinkpad laptops had only light in bezel.

So if you have better idea for commit message, feel free to change it.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v2] thinkpad_acpi: Add support for keyboard backlight

[Darren Hart]

On Mon, Jan 04, 2016 at 09:51:23PM +0100, Pali Rohár wrote:
> On Monday 04 January 2016 21:40:20 Darren Hart wrote:
> > On Mon, Jan 04, 2016 at 09:26:19PM +0100, Pali Rohár wrote:
> > > On Monday 04 January 2016 21:04:25 Darren Hart wrote:
> > > > On Wed, Dec 30, 2015 at 11:27:41PM +0100, 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.
> > > > 
> > > > Which laptops is this intended to support?
> > > 
> > > Thinkpad ??30 series and new which have backlight keyboard.
> > 
> > Thanks, we should include that in the commit message as well as the
> > comments surrounding the driver section.
> 
> ??30 is probably not good characteristic, but I mean all those Thinkpad 
> laptops like T430, x230, E430, X1 (1st) and their successors (T440, 
> T450, X1 3rd, ...) All those which are from Ivy Bridge processor 
> generation (and new).
> 
> But basically it cover all Thinkpad laptops which have backlight 
> keyboard. Older Thinkpad laptops had only light in bezel.
> 
> So if you have better idea for commit message, feel free to change it.

Ah, I see. New feature that didn't exist previously. OK, no need for changes on
that score then.



-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v2] thinkpad_acpi: Add support for keyboard backlight

[Henrique de Moraes Holschuh]

On Mon, Jan 4, 2016, at 18:40, Darren Hart wrote:
> On Mon, Jan 04, 2016 at 09:26:19PM +0100, Pali Rohár wrote:
> > On Monday 04 January 2016 21:04:25 Darren Hart wrote:
> > > On Wed, Dec 30, 2015 at 11:27:41PM +0100, 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.
> > > 
> > > Which laptops is this intended to support?
> > 
> > Thinkpad ??30 series and new which have backlight keyboard.
> 
> Thanks, we should include that in the commit message as well as the
> comments
> surrounding the driver section.
> 
> Henrique, are your concerns surrounding suspend/resume resolved?

Yes.

Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] thinkpad_acpi: Add support for keyboard backlight

[Pavel Machek]

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
									
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] thinkpad_acpi: Add support for keyboard backlight

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1559 bytes --]

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.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] thinkpad_acpi: Add support for keyboard backlight

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 959 bytes --]

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;
		}
	}

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] thinkpad_acpi: Add support for keyboard backlight

[Henrique de Moraes Holschuh]

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 :-(

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] thinkpad_acpi: Add support for keyboard backlight

[Henrique de Moraes Holschuh]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > Tested-by: Fabio D'Urso <fabiodurso-PkbjNfxxIASonA0d6jMUrA@public.gmane.org>
> > 
> > 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...

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=267308311&iu=/4140

Re: [PATCH] thinkpad_acpi: Add support for keyboard backlight

[Henrique de Moraes Holschuh]

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...

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] thinkpad_acpi: Add support for keyboard backlight

[Darren Hart]

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.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH] thinkpad_acpi: Add support for keyboard backlight

[Henrique de Moraes Holschuh]

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.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] thinkpad_acpi: Add support for keyboard backlight

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 907 bytes --]

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...

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] thinkpad_acpi: Add support for keyboard backlight

[Johannes Stezenbach]

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

Re: [PATCH] thinkpad_acpi: Add support for keyboard backlight

[Henrique de Moraes Holschuh]

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.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [ibm-acpi-devel] [PATCH] thinkpad_acpi: Add support for keyboard backlight

[Yves-Alexis Perez]

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

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,
-- 
Yves-Alexis


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] thinkpad_acpi: Add support for keyboard backlight

[Johannes Stezenbach]

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

Re: [PATCH] thinkpad_acpi: Add support for keyboard backlight

[Henrique de Moraes Holschuh]

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.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] thinkpad_acpi: Add support for keyboard backlight

[Johannes Stezenbach]

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

Re: [ibm-acpi-devel] [PATCH] thinkpad_acpi: Add support for keyboard backlight

[Kevin Locke]

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

Re: [ibm-acpi-devel] [PATCH] thinkpad_acpi: Add support for keyboard backlight

[Henrique de Moraes Holschuh]

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 :-(

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [ibm-acpi-devel] [PATCH] thinkpad_acpi: Add support for keyboard backlight

[Kevin Locke]

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

Re: [PATCH] thinkpad_acpi: Add support for keyboard backlight

[Henrique de Moraes Holschuh]

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.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] thinkpad_acpi: Add support for keyboard backlight

[Pavel Machek]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] thinkpad_acpi: Add support for keyboard backlight

[Pali Rohár]

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...

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH] thinkpad_acpi: Add support for keyboard backlight

[Pavel Machek]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] thinkpad_acpi: Add support for keyboard backlight

[Pavel Machek]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] thinkpad_acpi: Add support for keyboard backlight

[Pali Rohár]

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 :-)

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [ibm-acpi-devel] [PATCH] thinkpad_acpi: Add support for keyboard backlight

[Yves-Alexis Perez]

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

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,
-- 
Yves-Alexis


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] thinkpad_acpi: Add support for keyboard backlight

[Darren Hart]

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).

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH] thinkpad_acpi: Add support for keyboard backlight

[Henrique de Moraes Holschuh]

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!

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh