Some new Lenovo or ThinkPad laptops don't have EC controllable LEDs,
they control LEDs by BIOS functions, and return ACPI Warning while
operating LEDs by EC.
These patches set some quirks, optimize led_init() and led_write() to
avoid that warning.
Adam Lee (3):
thinkpad_acpi: return -NODEV while operating uninitialized LEDs
thinkpad_acpi: add the ability setting TPACPI_LED_NONE by quirk
thinkpad_acpi: add LED quirks of models which don't have EC controllable LEDs
drivers/platform/x86/thinkpad_acpi.c | 24 ++++++++++++++++++++----
1 file changed, 20 insertions(+), 4 deletions(-)
--
1.7.10.4
Not all 0-15 LEDs are available for all models, sometimes it's even not
safe. This patch return -NODEV while operating uninitialized LEDs.
Signed-off-by: Adam Lee <[email protected]>
---
drivers/platform/x86/thinkpad_acpi.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 54d31c0..bea57ae 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -5401,9 +5401,12 @@ static int led_write(char *buf)
return -ENODEV;
while ((cmd = next_cmd(&buf))) {
- if (sscanf(cmd, "%d", &led) != 1 || led < 0 || led > 15)
+ if (sscanf(cmd, "%d", &led) != 1)
return -EINVAL;
+ if (!tpacpi_leds[led].led)
+ return -ENODEV;
+
if (strstr(cmd, "off")) {
s = TPACPI_LED_OFF;
} else if (strstr(cmd, "on")) {
--
1.7.10.4
Some new Lenovo or ThinkPad laptops don't have EC controllable LEDs.
This patch adds their quirks.
Signed-off-by: Adam Lee <[email protected]>
---
drivers/platform/x86/thinkpad_acpi.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 2fb1f1c..4cc7d73 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -5268,6 +5268,12 @@ static const struct tpacpi_quirk led_useful_qtable[] __initconst = {
TPACPI_Q_IBM('7', 'F', 0x1f97), /* Z61* (1) */
TPACPI_Q_IBM('7', 'B', 0x1fb7), /* X60 (1) */
+ /* Models don't have EC controllable LEDs */
+ TPACPI_Q_LNV('G', 'D', 0), /* ThinkPad Twist */
+ TPACPI_Q_LNV('H', '0', 0), /* ThinkPad E430 */
+ TPACPI_Q_LNV('H', 'C', 0), /* Lenovo E49L */
+ TPACPI_Q_LNV('H', 'D', 0), /* Lenovo K4350 */
+
/* (1) - may have excess leds enabled on MSB */
/* Defaults (order matters, keep last, don't reorder!) */
--
1.7.10.4
Some new Lenovo or ThinkPad laptops don't have EC controllable LEDs,
their LED quirks are 0. This patch set led_supported=TPACPI_LED_NONE
when quirk equals 0.
Signed-off-by: Adam Lee <[email protected]>
---
drivers/platform/x86/thinkpad_acpi.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index bea57ae..2fb1f1c 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -5327,6 +5327,16 @@ static int __init led_init(struct ibm_init_struct *iibm)
led_supported = led_init_detect_mode();
+ if (led_supported != TPACPI_LED_NONE) {
+ useful_leds = tpacpi_check_quirks(led_useful_qtable,
+ ARRAY_SIZE(led_useful_qtable));
+
+ if (!useful_leds) {
+ led_handle = NULL;
+ led_supported = TPACPI_LED_NONE;
+ }
+ }
+
vdbg_printk(TPACPI_DBG_INIT, "LED commands are %s, mode %d\n",
str_supported(led_supported), led_supported);
@@ -5340,9 +5350,6 @@ static int __init led_init(struct ibm_init_struct *iibm)
return -ENOMEM;
}
- useful_leds = tpacpi_check_quirks(led_useful_qtable,
- ARRAY_SIZE(led_useful_qtable));
-
for (i = 0; i < TPACPI_LED_NUMLEDS; i++) {
if (!tpacpi_is_led_restricted(i) &&
test_bit(i, &useful_leds)) {
--
1.7.10.4
Adam Lee <[email protected]> writes:
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -5401,9 +5401,12 @@ static int led_write(char *buf)
> return -ENODEV;
>
> while ((cmd = next_cmd(&buf))) {
> - if (sscanf(cmd, "%d", &led) != 1 || led < 0 || led > 15)
> + if (sscanf(cmd, "%d", &led) != 1)
> return -EINVAL;
>
> + if (!tpacpi_leds[led].led)
> + return -ENODEV;
This looks risky. Why did you remove the index sanity check? What will
happen now if the input is e.g "-1" or "42"?
BTW, the magic number 15 should probably be (TPACPI_LED_NUMLEDS - 1)
instead.
Bjørn
On Fri, Jun 07, 2013 at 10:53:50AM +0200, Bj?rn Mork wrote:
> Adam Lee <[email protected]> writes:
>
> > --- a/drivers/platform/x86/thinkpad_acpi.c
> > +++ b/drivers/platform/x86/thinkpad_acpi.c
> > @@ -5401,9 +5401,12 @@ static int led_write(char *buf)
> > return -ENODEV;
> >
> > while ((cmd = next_cmd(&buf))) {
> > - if (sscanf(cmd, "%d", &led) != 1 || led < 0 || led > 15)
> > + if (sscanf(cmd, "%d", &led) != 1)
> > return -EINVAL;
> >
> > + if (!tpacpi_leds[led].led)
> > + return -ENODEV;
>
> This looks risky. Why did you remove the index sanity check? What will
> happen now if the input is e.g "-1" or "42"?
>
> BTW, the magic number 15 should probably be (TPACPI_LED_NUMLEDS - 1)
> instead.
Oh, good point! I was thinking led_init() already checked the index.
Will submit patch v2, thanks.
--
Regards,
Adam Lee
Hardware Enablement
Not all 0-15 LEDs are available for all models, sometimes it's even not
safe. This patch return -NODEV while operating uninitialized LEDs.
Signed-off-by: Adam Lee <[email protected]>
---
drivers/platform/x86/thinkpad_acpi.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 54d31c0..d2ac4e8 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -5401,9 +5401,12 @@ static int led_write(char *buf)
return -ENODEV;
while ((cmd = next_cmd(&buf))) {
- if (sscanf(cmd, "%d", &led) != 1 || led < 0 || led > 15)
+ if (sscanf(cmd, "%d", &led) != 1 || led < 0 || led > (TPACPI_LED_NUMLEDS -1))
return -EINVAL;
+ if (!tpacpi_leds[led].led)
+ return -ENODEV;
+
if (strstr(cmd, "off")) {
s = TPACPI_LED_OFF;
} else if (strstr(cmd, "on")) {
--
1.7.10.4
On Fri, 2013-06-07 at 16:20 +0800, Adam Lee wrote:
> Some new Lenovo or ThinkPad laptops don't have EC controllable LEDs.
> This patch adds their quirks.
Can you detect this case? If this is a new way of doing things then
you're going to have to add new entries to the quirk list every time
Lenovo release a new machine, which isn't really going to work.
--
Matthew Garrett | [email protected]
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Fri, Jun 07, 2013 at 01:22:39PM +0000, Matthew Garrett wrote:
> On Fri, 2013-06-07 at 16:20 +0800, Adam Lee wrote:
> > Some new Lenovo or ThinkPad laptops don't have EC controllable LEDs.
> > This patch adds their quirks.
>
> Can you detect this case? If this is a new way of doing things then
> you're going to have to add new entries to the quirk list every time
> Lenovo release a new machine, which isn't really going to work.
Thanks. The difference is their LED handle expects only one argument,
not two. Does kernel have a method detecting acpi_handle's arguments
number? (checking...)
About the new way:
Lenovo is going to assign a ACPI method to each operation. Like the
volume_toggle button and LED, hardware toggle is "SHDA", getting state
is "GSMS", setting state is "SSMS".
About the problem:
Some models only support the new way but containing internal EC LED
interfaces which makes our detecting failed at the same time. The new
interfaces accept only one argument(on, off, or blink), we couldn't
control LEDs by them because can't specify the LED number. If you give
them two arguments, it returns ACPI warning, so I wanna add these "0"
quirks.
--
Regards,
Adam Lee
Hardware Enablement
Can you get me the ACPI interface documentation? It should be simple to get it, but I haven't emailed the Lenovo BIOS engineers for some time.
Basically, create a new LED method in thinkpad-acpi, lock it down to vendor lenovo, detect the new interfaces, and only check the two-argument LED one if the newer ones aren't found.
Not supporting the old /proc interface on new thinkpads is fine.
--
"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
On Fri, Jun 7, 2013 at 5:05 PM, Adam Lee <[email protected]> wrote:
>
> Not all 0-15 LEDs are available for all models, sometimes it's even not
> safe. This patch return -NODEV while operating uninitialized LEDs.
>
> Signed-off-by: Adam Lee <[email protected]>
> ---
> drivers/platform/x86/thinkpad_acpi.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 54d31c0..d2ac4e8 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -5401,9 +5401,12 @@ static int led_write(char *buf)
> return -ENODEV;
>
> while ((cmd = next_cmd(&buf))) {
> - if (sscanf(cmd, "%d", &led) != 1 || led < 0 || led > 15)
> + if (sscanf(cmd, "%d", &led) != 1 || led < 0 || led > (TPACPI_LED_NUMLEDS -1))
> return -EINVAL;
>
> + if (!tpacpi_leds[led].led)
The .led filed might be set to '0' in led_init(), how about set the
uninitiated .led to -1? and check by "if (tpacpi_leds[led].led < 0)"
for (i = 0; i < TPACPI_LED_NUMLEDS; i++) {
if (!tpacpi_is_led_restricted(i) &&
test_bit(i, &useful_leds)) {
rc = tpacpi_init_led(i);
if (rc < 0) {
led_exit();
return rc;
- }
+ } else
+ tpacpi_leds[led].led = -1;
} else tpacpi_leds[led].led = -1;
}
> + return -ENODEV;
> +
> if (strstr(cmd, "off")) {
> s = TPACPI_LED_OFF;
> } else if (strstr(cmd, "on")) {
> --
> 1.7.10.4
Not all 0-15 LEDs are available for all models, sometimes it's even not
safe. This patch return -NODEV while operating uninitialized LEDs.
Signed-off-by: Adam Lee <[email protected]>
---
drivers/platform/x86/thinkpad_acpi.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 54d31c0..455384b 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -369,7 +369,7 @@ struct tpacpi_led_classdev {
struct led_classdev led_classdev;
struct work_struct work;
enum led_status_t new_state;
- unsigned int led;
+ int led;
};
/* brightness level capabilities */
@@ -5344,6 +5344,8 @@ static int __init led_init(struct ibm_init_struct *iibm)
ARRAY_SIZE(led_useful_qtable));
for (i = 0; i < TPACPI_LED_NUMLEDS; i++) {
+ tpacpi_leds[i].led = -1;
+
if (!tpacpi_is_led_restricted(i) &&
test_bit(i, &useful_leds)) {
rc = tpacpi_init_led(i);
@@ -5401,9 +5403,13 @@ static int led_write(char *buf)
return -ENODEV;
while ((cmd = next_cmd(&buf))) {
- if (sscanf(cmd, "%d", &led) != 1 || led < 0 || led > 15)
+ if (sscanf(cmd, "%d", &led) != 1)
return -EINVAL;
+ if (led < 0 || led > (TPACPI_LED_NUMLEDS - 1) ||
+ tpacpi_leds[led].led < 0)
+ return -ENODEV;
+
if (strstr(cmd, "off")) {
s = TPACPI_LED_OFF;
} else if (strstr(cmd, "on")) {
--
1.7.10.4
On Fri, Jun 07, 2013 at 05:16:15PM -0300, Henrique de Moraes Holschuh wrote:
> Can you get me the ACPI interface documentation? It should be simple
> to get it, but I haven't emailed the Lenovo BIOS engineers for some
> time.
>
> Basically, create a new LED method in thinkpad-acpi, lock it down to
> vendor lenovo, detect the new interfaces, and only check the
> two-argument LED one if the newer ones aren't found.
>
> Not supporting the old /proc interface on new thinkpads is fine.
Thanks, I will request the documentation from Lenovo again.
Please apply "[PATCH 1/3 v3] thinkpad_acpi: return -NODEV while operating uninitialized LEDs" and
"[PATCH 2/3] thinkpad_acpi: add the ability setting TPACPI_LED_NONE by quirk", but
hold "[PATCH 3/3] thinkpad_acpi: add LED quirks of models which don't have EC controllable LEDs",
I will drop [PATCH 3/3] to discussion again when I got the documentation.
--
Regards,
Adam Lee
Hardware Enablement
On Fri, 07 Jun 2013, Adam Lee wrote:
> Not all 0-15 LEDs are available for all models, sometimes it's even not
> safe. This patch return -NODEV while operating uninitialized LEDs.
>
> Signed-off-by: Adam Lee <[email protected]>
Acked-by: Henrique de Moraes Holschuh <[email protected]>
> ---
> drivers/platform/x86/thinkpad_acpi.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 54d31c0..bea57ae 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -5401,9 +5401,12 @@ static int led_write(char *buf)
> return -ENODEV;
>
> while ((cmd = next_cmd(&buf))) {
> - if (sscanf(cmd, "%d", &led) != 1 || led < 0 || led > 15)
> + if (sscanf(cmd, "%d", &led) != 1)
> return -EINVAL;
>
> + if (!tpacpi_leds[led].led)
> + return -ENODEV;
> +
> if (strstr(cmd, "off")) {
> s = TPACPI_LED_OFF;
> } else if (strstr(cmd, "on")) {
--
"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
On Sat, 08 Jun 2013, Adam Lee wrote:
> Not all 0-15 LEDs are available for all models, sometimes it's even not
> safe. This patch return -NODEV while operating uninitialized LEDs.
>
> Signed-off-by: Adam Lee <[email protected]>
Acked-by: Henrique de Moraes Holschuh <[email protected]>
> ---
> drivers/platform/x86/thinkpad_acpi.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 54d31c0..455384b 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -369,7 +369,7 @@ struct tpacpi_led_classdev {
> struct led_classdev led_classdev;
> struct work_struct work;
> enum led_status_t new_state;
> - unsigned int led;
> + int led;
> };
>
> /* brightness level capabilities */
> @@ -5344,6 +5344,8 @@ static int __init led_init(struct ibm_init_struct *iibm)
> ARRAY_SIZE(led_useful_qtable));
>
> for (i = 0; i < TPACPI_LED_NUMLEDS; i++) {
> + tpacpi_leds[i].led = -1;
> +
> if (!tpacpi_is_led_restricted(i) &&
> test_bit(i, &useful_leds)) {
> rc = tpacpi_init_led(i);
> @@ -5401,9 +5403,13 @@ static int led_write(char *buf)
> return -ENODEV;
>
> while ((cmd = next_cmd(&buf))) {
> - if (sscanf(cmd, "%d", &led) != 1 || led < 0 || led > 15)
> + if (sscanf(cmd, "%d", &led) != 1)
> return -EINVAL;
>
> + if (led < 0 || led > (TPACPI_LED_NUMLEDS - 1) ||
> + tpacpi_leds[led].led < 0)
> + return -ENODEV;
> +
> if (strstr(cmd, "off")) {
> s = TPACPI_LED_OFF;
> } else if (strstr(cmd, "on")) {
--
"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
On Mon, 17 Jun 2013, Henrique de Moraes Holschuh wrote:
> On Fri, 07 Jun 2013, Adam Lee wrote:
> > Not all 0-15 LEDs are available for all models, sometimes it's even not
> > safe. This patch return -NODEV while operating uninitialized LEDs.
> >
> > Signed-off-by: Adam Lee <[email protected]>
> Acked-by: Henrique de Moraes Holschuh <[email protected]>
Matthew, please pick v3 of this patch, and not this one.
--
"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
On Fri, 07 Jun 2013, Adam Lee wrote:
> Some new Lenovo or ThinkPad laptops don't have EC controllable LEDs,
> their LED quirks are 0. This patch set led_supported=TPACPI_LED_NONE
> when quirk equals 0.
>
> Signed-off-by: Adam Lee <[email protected]>
Acked-by: Henrique de Moraes Holschuh <[email protected]>
> ---
> drivers/platform/x86/thinkpad_acpi.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index bea57ae..2fb1f1c 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -5327,6 +5327,16 @@ static int __init led_init(struct ibm_init_struct *iibm)
>
> led_supported = led_init_detect_mode();
>
> + if (led_supported != TPACPI_LED_NONE) {
> + useful_leds = tpacpi_check_quirks(led_useful_qtable,
> + ARRAY_SIZE(led_useful_qtable));
> +
> + if (!useful_leds) {
> + led_handle = NULL;
> + led_supported = TPACPI_LED_NONE;
> + }
> + }
> +
> vdbg_printk(TPACPI_DBG_INIT, "LED commands are %s, mode %d\n",
> str_supported(led_supported), led_supported);
>
> @@ -5340,9 +5350,6 @@ static int __init led_init(struct ibm_init_struct *iibm)
> return -ENOMEM;
> }
>
> - useful_leds = tpacpi_check_quirks(led_useful_qtable,
> - ARRAY_SIZE(led_useful_qtable));
> -
> for (i = 0; i < TPACPI_LED_NUMLEDS; i++) {
> if (!tpacpi_is_led_restricted(i) &&
> test_bit(i, &useful_leds)) {
--
"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
On Fri, 07 Jun 2013, Adam Lee wrote:
> Some new Lenovo or ThinkPad laptops don't have EC controllable LEDs.
> This patch adds their quirks.
>
> Signed-off-by: Adam Lee <[email protected]>
NAK.
Let's see if we can do this in a better way that is a bit more future-proof.
--
"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