[PATCH 1/2] platform/x86: thinkpad_acpi: Add LED_RETAIN_AT_SHUTDOWN to led_class_devs

[PATCH 1/2] platform/x86: thinkpad_acpi: Add LED_RETAIN_AT_SHUTDOWN to led_class_devs

[Hans de Goede]

Add the LED_RETAIN_AT_SHUTDOWN flag to the registered led_class_devs so
that the LEDs do not get turned-off when reloading the driver and thus so
that they also stay under default EC control when reloading the driver,
unless explicitly overridden by the user.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/thinkpad_acpi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index a4ef57ddcc90..49fdf16b2db9 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -5827,6 +5827,7 @@ static int __init tpacpi_init_led(unsigned int led)
 		tpacpi_leds[led].led_classdev.brightness_get = &led_sysfs_get;
 
 	tpacpi_leds[led].led_classdev.name = tpacpi_led_names[led];
+	tpacpi_leds[led].led_classdev.flags = LED_RETAIN_AT_SHUTDOWN;
 	tpacpi_leds[led].led = led;
 
 	return led_classdev_register(&tpacpi_pdev->dev, &tpacpi_leds[led].led_classdev);
-- 
2.33.1

[PATCH 2/2] platform/x86: thinkpad_acpi: Add lid-logo-led to the list of safe LEDs

[Hans de Goede]

There have been various bugs / forum threads about allowing control of
the LED in the ThinkPad logo on the lid of various models.

This seems to be something which users want to control and there really
is no reason to require setting CONFIG_THINKPAD_ACPI_UNSAFE_LEDS for this.

The lid-logo-led is LED number 10, so change the name of the 10th led
from unknown_led2 to lid_logo_led and add it to the TPACPI_SAFE_LEDS mask.

Link: https://www.reddit.com/r/thinkpad/comments/7n8eyu/thinkpad_led_control_under_gnulinux/
BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1943318
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/thinkpad_acpi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 49fdf16b2db9..28f0299ecab0 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -5661,11 +5661,11 @@ static const char * const tpacpi_led_names[TPACPI_LED_NUMLEDS] = {
 	"tpacpi::standby",
 	"tpacpi::dock_status1",
 	"tpacpi::dock_status2",
-	"tpacpi::unknown_led2",
+	"tpacpi::lid_logo_led",
 	"tpacpi::unknown_led3",
 	"tpacpi::thinkvantage",
 };
-#define TPACPI_SAFE_LEDS	0x1081U
+#define TPACPI_SAFE_LEDS	0x1481U
 
 static inline bool tpacpi_is_led_restricted(const unsigned int led)
 {
-- 
2.33.1

Re: [PATCH 2/2] platform/x86: thinkpad_acpi: Add lid-logo-led to the list of safe LEDs

[Thomas Weißschuh]

Hi,

On 2021-11-23 22:05+0100, Hans de Goede wrote:
> There have been various bugs / forum threads about allowing control of
> the LED in the ThinkPad logo on the lid of various models.
> 
> This seems to be something which users want to control and there really
> is no reason to require setting CONFIG_THINKPAD_ACPI_UNSAFE_LEDS for this.
> 
> The lid-logo-led is LED number 10, so change the name of the 10th led
> from unknown_led2 to lid_logo_led and add it to the TPACPI_SAFE_LEDS mask.
> 
> Link: https://www.reddit.com/r/thinkpad/comments/7n8eyu/thinkpad_led_control_under_gnulinux/
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1943318
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/platform/x86/thinkpad_acpi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 49fdf16b2db9..28f0299ecab0 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -5661,11 +5661,11 @@ static const char * const tpacpi_led_names[TPACPI_LED_NUMLEDS] = {
>  	"tpacpi::standby",
>  	"tpacpi::dock_status1",
>  	"tpacpi::dock_status2",
> -	"tpacpi::unknown_led2",
> +	"tpacpi::lid_logo_led",

The suffix "_led" looks a bit redundant. Also non of the other LEDs have it.

Also currently the reported brightness is 0 before writing to it, although the
LED is powered on by default, not sure how this could be fixed though.

>  	"tpacpi::unknown_led3",
>  	"tpacpi::thinkvantage",
>  };
> -#define TPACPI_SAFE_LEDS	0x1081U
> +#define TPACPI_SAFE_LEDS	0x1481U
>  
>  static inline bool tpacpi_is_led_restricted(const unsigned int led)
>  {

Thomas

Re: [PATCH 2/2] platform/x86: thinkpad_acpi: Add lid-logo-led to the list of safe LEDs

[Hans de Goede]

Hi,

On 11/24/21 16:53, Thomas Weißschuh wrote:
> Hi,
> 
> On 2021-11-23 22:05+0100, Hans de Goede wrote:
>> There have been various bugs / forum threads about allowing control of
>> the LED in the ThinkPad logo on the lid of various models.
>>
>> This seems to be something which users want to control and there really
>> is no reason to require setting CONFIG_THINKPAD_ACPI_UNSAFE_LEDS for this.
>>
>> The lid-logo-led is LED number 10, so change the name of the 10th led
>> from unknown_led2 to lid_logo_led and add it to the TPACPI_SAFE_LEDS mask.
>>
>> Link: https://www.reddit.com/r/thinkpad/comments/7n8eyu/thinkpad_led_control_under_gnulinux/
>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1943318
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/platform/x86/thinkpad_acpi.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
>> index 49fdf16b2db9..28f0299ecab0 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -5661,11 +5661,11 @@ static const char * const tpacpi_led_names[TPACPI_LED_NUMLEDS] = {
>>  	"tpacpi::standby",
>>  	"tpacpi::dock_status1",
>>  	"tpacpi::dock_status2",
>> -	"tpacpi::unknown_led2",
>> +	"tpacpi::lid_logo_led",
> 
> The suffix "_led" looks a bit redundant. Also non of the other LEDs have it.

Hmm, good point, but without the _led to me it sounds as if it controls
some backlight for the entire logo, where it really is just the dot of the i.

So I'm not sure what to do here :)

> Also currently the reported brightness is 0 before writing to it, although the
> LED is powered on by default, not sure how this could be fixed though.

Right, this is a known short-coming of the tpacpi LED interface, LEDs can be
set but you cannot get the current status.

And once set, the LED is now fully under usercontrol, until the next reboot
(or maybe even power-cycle).

Regards,

Hans

Re: [External] Re: [PATCH 2/2] platform/x86: thinkpad_acpi: Add lid-logo-led to the list of safe LEDs

[Mark Pearson]

On 2021-11-24 11:28, Hans de Goede wrote:
> Hi,
> 
> On 11/24/21 16:53, Thomas Weißschuh wrote:
>> Hi,
>>
>> On 2021-11-23 22:05+0100, Hans de Goede wrote:
>>> There have been various bugs / forum threads about allowing control of
>>> the LED in the ThinkPad logo on the lid of various models.
>>>
>>> This seems to be something which users want to control and there really
>>> is no reason to require setting CONFIG_THINKPAD_ACPI_UNSAFE_LEDS for this.
>>>
>>> The lid-logo-led is LED number 10, so change the name of the 10th led
>>> from unknown_led2 to lid_logo_led and add it to the TPACPI_SAFE_LEDS mask.
>>>
>>> Link: https://www.reddit.com/r/thinkpad/comments/7n8eyu/thinkpad_led_control_under_gnulinux/>>>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1943318>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>  drivers/platform/x86/thinkpad_acpi.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
>>> index 49fdf16b2db9..28f0299ecab0 100644
>>> --- a/drivers/platform/x86/thinkpad_acpi.c
>>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>>> @@ -5661,11 +5661,11 @@ static const char * const tpacpi_led_names[TPACPI_LED_NUMLEDS] = {
>>>  	"tpacpi::standby",
>>>  	"tpacpi::dock_status1",
>>>  	"tpacpi::dock_status2",
>>> -	"tpacpi::unknown_led2",
>>> +	"tpacpi::lid_logo_led",
>>
>> The suffix "_led" looks a bit redundant. Also non of the other LEDs have it.
> 
> Hmm, good point, but without the _led to me it sounds as if it controls
> some backlight for the entire logo, where it really is just the dot of the i.
> 
> So I'm not sure what to do here :)
> 
>> Also currently the reported brightness is 0 before writing to it, although the
>> LED is powered on by default, not sure how this could be fixed though.
> 
> Right, this is a known short-coming of the tpacpi LED interface, LEDs can be
> set but you cannot get the current status.
> 
> And once set, the LED is now fully under usercontrol, until the next reboot
> (or maybe even power-cycle).
> 
Apart from being vaguely fascinated that people want to play with the
LED (I assume because it's annoying and a small waste of power?) - is this
something that I should put in a request for an API to get the LED status?

I would like to get the FW teams point of view here too. We use the LED
to show if the system is suspended or not so I'm somewhat curious as to
what happens if a user overrides the setting. I doubt it's a big deal
but I'd like to double check if there are any gotcha's.

As a note - I added a comment to the Fedora bug linked above too...not
related to LEDs.

Thanks
Mark

Re: [External] Re: [PATCH 2/2] platform/x86: thinkpad_acpi: Add lid-logo-led to the list of safe LEDs

[Hans de Goede]

Hi,

On 11/24/21 20:13, Mark Pearson wrote:
> 
> 
> On 2021-11-24 11:28, Hans de Goede wrote:
>> Hi,
>>
>> On 11/24/21 16:53, Thomas Weißschuh wrote:
>>> Hi,
>>>
>>> On 2021-11-23 22:05+0100, Hans de Goede wrote:
>>>> There have been various bugs / forum threads about allowing control of
>>>> the LED in the ThinkPad logo on the lid of various models.
>>>>
>>>> This seems to be something which users want to control and there really
>>>> is no reason to require setting CONFIG_THINKPAD_ACPI_UNSAFE_LEDS for this.
>>>>
>>>> The lid-logo-led is LED number 10, so change the name of the 10th led
>>>> from unknown_led2 to lid_logo_led and add it to the TPACPI_SAFE_LEDS mask.
>>>>
>>>> Link: https://www.reddit.com/r/thinkpad/comments/7n8eyu/thinkpad_led_control_under_gnulinux/>>>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1943318>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>  drivers/platform/x86/thinkpad_acpi.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
>>>> index 49fdf16b2db9..28f0299ecab0 100644
>>>> --- a/drivers/platform/x86/thinkpad_acpi.c
>>>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>>>> @@ -5661,11 +5661,11 @@ static const char * const tpacpi_led_names[TPACPI_LED_NUMLEDS] = {
>>>>  	"tpacpi::standby",
>>>>  	"tpacpi::dock_status1",
>>>>  	"tpacpi::dock_status2",
>>>> -	"tpacpi::unknown_led2",
>>>> +	"tpacpi::lid_logo_led",
>>>
>>> The suffix "_led" looks a bit redundant. Also non of the other LEDs have it.
>>
>> Hmm, good point, but without the _led to me it sounds as if it controls
>> some backlight for the entire logo, where it really is just the dot of the i.
>>
>> So I'm not sure what to do here :)
>>
>>> Also currently the reported brightness is 0 before writing to it, although the
>>> LED is powered on by default, not sure how this could be fixed though.
>>
>> Right, this is a known short-coming of the tpacpi LED interface, LEDs can be
>> set but you cannot get the current status.
>>
>> And once set, the LED is now fully under usercontrol, until the next reboot
>> (or maybe even power-cycle).
>>
> Apart from being vaguely fascinated that people want to play with the
> LED (I assume because it's annoying and a small waste of power?)

Some people just find the LED annoying so they just want to turn it off
I believe. Others want to repurpose it for their own purposes.

> is this
> something that I should put in a request for an API to get the LED status?

I don't believe not being able to read the status is really a big deal,
with that said it would be nice to have, but definitely a low priority item.

> I would like to get the FW teams point of view here too. We use the LED
> to show if the system is suspended or not so I'm somewhat curious as to
> what happens if a user overrides the setting.

AFAIK once the user has overridden the value it stays at the user selected
value until a reboot or power-cycle.

Note that most thinkpads also make the power-button-led "glow" when
suspended, and we already allow overriding this.

The reason why thinkpad_acpi has the notion of safe LEDs is to disallow
overriding some LEDs in older docks which indicate when it is safe to
unplug the laptop.

In that case overriding the LEDs could be quite bad but here we just loose
"I'm suspended" notification which may be annoying if the user is waiting
for the LED to start glowing after closing the lid (and before say bagging
the laptop). But presumably a user deliberately overriding the LED knows
it is not going to glow now.

> I doubt it's a big deal
> but I'd like to double check if there are any gotcha's.

Sounds good, thanks.

Regards,

Hans

Re: [PATCH 2/2] platform/x86: thinkpad_acpi: Add lid-logo-led to the list of safe LEDs

[Thomas Weißschuh]

On 2021-11-24 17:28+0100, Hans de Goede wrote:
> On 11/24/21 16:53, Thomas Weißschuh wrote:
> > Hi,
> > 
> > On 2021-11-23 22:05+0100, Hans de Goede wrote:
> >> There have been various bugs / forum threads about allowing control of
> >> the LED in the ThinkPad logo on the lid of various models.
> >>
> >> This seems to be something which users want to control and there really
> >> is no reason to require setting CONFIG_THINKPAD_ACPI_UNSAFE_LEDS for this.
> >>
> >> The lid-logo-led is LED number 10, so change the name of the 10th led
> >> from unknown_led2 to lid_logo_led and add it to the TPACPI_SAFE_LEDS mask.
> >>
> >> Link: https://www.reddit.com/r/thinkpad/comments/7n8eyu/thinkpad_led_control_under_gnulinux/
> >> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1943318
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >>  drivers/platform/x86/thinkpad_acpi.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> >> index 49fdf16b2db9..28f0299ecab0 100644
> >> --- a/drivers/platform/x86/thinkpad_acpi.c
> >> +++ b/drivers/platform/x86/thinkpad_acpi.c
> >> @@ -5661,11 +5661,11 @@ static const char * const tpacpi_led_names[TPACPI_LED_NUMLEDS] = {
> >>  	"tpacpi::standby",
> >>  	"tpacpi::dock_status1",
> >>  	"tpacpi::dock_status2",
> >> -	"tpacpi::unknown_led2",
> >> +	"tpacpi::lid_logo_led",
> > 
> > The suffix "_led" looks a bit redundant. Also non of the other LEDs have it.
> 
> Hmm, good point, but without the _led to me it sounds as if it controls
> some backlight for the entire logo, where it really is just the dot of the i.

lid_logo_dot? Not sure it's worth the bikeshedding :-)

Thomas

Re: [PATCH 2/2] platform/x86: thinkpad_acpi: Add lid-logo-led to the list of safe LEDs

[Hans de Goede]

Hi,

On 11/24/21 23:59, Thomas Weißschuh wrote:
> On 2021-11-24 17:28+0100, Hans de Goede wrote:
>> On 11/24/21 16:53, Thomas Weißschuh wrote:
>>> Hi,
>>>
>>> On 2021-11-23 22:05+0100, Hans de Goede wrote:
>>>> There have been various bugs / forum threads about allowing control of
>>>> the LED in the ThinkPad logo on the lid of various models.
>>>>
>>>> This seems to be something which users want to control and there really
>>>> is no reason to require setting CONFIG_THINKPAD_ACPI_UNSAFE_LEDS for this.
>>>>
>>>> The lid-logo-led is LED number 10, so change the name of the 10th led
>>>> from unknown_led2 to lid_logo_led and add it to the TPACPI_SAFE_LEDS mask.
>>>>
>>>> Link: https://www.reddit.com/r/thinkpad/comments/7n8eyu/thinkpad_led_control_under_gnulinux/
>>>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1943318
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>  drivers/platform/x86/thinkpad_acpi.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
>>>> index 49fdf16b2db9..28f0299ecab0 100644
>>>> --- a/drivers/platform/x86/thinkpad_acpi.c
>>>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>>>> @@ -5661,11 +5661,11 @@ static const char * const tpacpi_led_names[TPACPI_LED_NUMLEDS] = {
>>>>  	"tpacpi::standby",
>>>>  	"tpacpi::dock_status1",
>>>>  	"tpacpi::dock_status2",
>>>> -	"tpacpi::unknown_led2",
>>>> +	"tpacpi::lid_logo_led",
>>>
>>> The suffix "_led" looks a bit redundant. Also non of the other LEDs have it.
>>
>> Hmm, good point, but without the _led to me it sounds as if it controls
>> some backlight for the entire logo, where it really is just the dot of the i.
> 
> lid_logo_dot? Not sure it's worth the bikeshedding :-)

lid_logo_dot, I like that, lid_logo_dot it is; and agreed on the bikeshedding :)

Regards,

Hans

Re: [PATCH 1/2] platform/x86: thinkpad_acpi: Add LED_RETAIN_AT_SHUTDOWN to led_class_devs

[Hans de Goede]

Hi All,

On 11/23/21 22:05, Hans de Goede wrote:
> Add the LED_RETAIN_AT_SHUTDOWN flag to the registered led_class_devs so
> that the LEDs do not get turned-off when reloading the driver and thus so
> that they also stay under default EC control when reloading the driver,
> unless explicitly overridden by the user.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

I've added this series to my review-hans branch now, with the LED
name changed to lid_logo_dot as discussed.

Regards,

Hans


> ---
>  drivers/platform/x86/thinkpad_acpi.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index a4ef57ddcc90..49fdf16b2db9 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -5827,6 +5827,7 @@ static int __init tpacpi_init_led(unsigned int led)
>  		tpacpi_leds[led].led_classdev.brightness_get = &led_sysfs_get;
>  
>  	tpacpi_leds[led].led_classdev.name = tpacpi_led_names[led];
> +	tpacpi_leds[led].led_classdev.flags = LED_RETAIN_AT_SHUTDOWN;
>  	tpacpi_leds[led].led = led;
>  
>  	return led_classdev_register(&tpacpi_pdev->dev, &tpacpi_leds[led].led_classdev);
>