[PATCH] [thinkpad-acpi] Add T410s and T420s LED support

[PATCH] [thinkpad-acpi] Add T410s and T420s LED support

[Jens Taprogge]

A new LED is integrated into the Microphone mute button.  On the other
hand many LEDs are missing and have been masked out from useful_leds.

The mute LED is not added to the TPACPI_SAFE_LEDS.  Safe handling of
muting should first be worked out.  I have tested all the masked out
LEDs (attached to dock) but I could not observe any effect.

Signed-off-by: Jens Taprogge <jens.taprogge@taprogge.org>
---
 Documentation/laptops/thinkpad-acpi.txt |    4 +++-
 drivers/platform/x86/thinkpad_acpi.c    |    6 ++++++
 2 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/Documentation/laptops/thinkpad-acpi.txt b/Documentation/laptops/thinkpad-acpi.txt
index 9d66682..de7ddac 100644
--- a/Documentation/laptops/thinkpad-acpi.txt
+++ b/Documentation/laptops/thinkpad-acpi.txt
@@ -826,7 +826,9 @@ mapping:
 	9 - dock status 2
 	10, 11 - (unknown)
 	12 - thinkvantage
-	13, 14, 15 - (unknown)
+	13 - (unknown)
+	14 - mute mic
+	15 - (unknown)
 
 All of the above can be turned on and off and can be made to blink.
 
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index ea0c607..a0bc79f 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -169,6 +169,7 @@ enum tpacpi_hkey_event_t {
 	TP_HKEY_EV_VOL_UP		= 0x1015, /* Volume up or unmute */
 	TP_HKEY_EV_VOL_DOWN		= 0x1016, /* Volume down or unmute */
 	TP_HKEY_EV_VOL_MUTE		= 0x1017, /* Mixer output mute */
+	TP_HKEY_EV_VOL_MUTE_MIC		= 0x101b, /* Microphone mute */
 
 	/* Reasons for waking up from S3/S4 */
 	TP_HKEY_EV_WKUP_S3_UNDOCK	= 0x2304, /* undock requested, S3 */
@@ -5055,6 +5056,8 @@ static const char * const tpacpi_led_names[TPACPI_LED_NUMLEDS] = {
 	"tpacpi::unknown_led2",
 	"tpacpi::unknown_led3",
 	"tpacpi::thinkvantage",
+	"tpacpi::unknown_led4",
+	"tpacpi::mute_mic",
 };
 #define TPACPI_SAFE_LEDS	0x1081U
 
@@ -5272,6 +5275,9 @@ 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) */
 
+	TPACPI_Q_LNV('6', 'U', 0x4087), /* T410s */
+	TPACPI_Q_LNV('8', 'C', 0x4087), /* T420s */
+
 	/* (1) - may have excess leds enabled on MSB */
 
 	/* Defaults (order matters, keep last, don't reorder!) */
-- 
1.7.5.4

Re: [PATCH] [thinkpad-acpi] Add T410s and T420s LED support

[Henrique de Moraes Holschuh]

On Fri, 23 Mar 2012, Jens Taprogge wrote:
> A new LED is integrated into the Microphone mute button.  On the other
> hand many LEDs are missing and have been masked out from useful_leds.
> 
> The mute LED is not added to the TPACPI_SAFE_LEDS.  Safe handling of
> muting should first be worked out.  I have tested all the masked out
> LEDs (attached to dock) but I could not observe any effect.
> 
> Signed-off-by: Jens Taprogge <jens.taprogge@taprogge.org>

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

Matthew, could you pick this one up for the next merge window (or the
currently open one if it is possible to do so, since the patch is a rather
safe one?)

Thanks.

> diff --git a/Documentation/laptops/thinkpad-acpi.txt b/Documentation/laptops/thinkpad-acpi.txt
> index 9d66682..de7ddac 100644
> --- a/Documentation/laptops/thinkpad-acpi.txt
> +++ b/Documentation/laptops/thinkpad-acpi.txt
> @@ -826,7 +826,9 @@ mapping:
>  	9 - dock status 2
>  	10, 11 - (unknown)
>  	12 - thinkvantage
> -	13, 14, 15 - (unknown)
> +	13 - (unknown)
> +	14 - mute mic
> +	15 - (unknown)
>  
>  All of the above can be turned on and off and can be made to blink.
>  
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index ea0c607..a0bc79f 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -169,6 +169,7 @@ enum tpacpi_hkey_event_t {
>  	TP_HKEY_EV_VOL_UP		= 0x1015, /* Volume up or unmute */
>  	TP_HKEY_EV_VOL_DOWN		= 0x1016, /* Volume down or unmute */
>  	TP_HKEY_EV_VOL_MUTE		= 0x1017, /* Mixer output mute */
> +	TP_HKEY_EV_VOL_MUTE_MIC		= 0x101b, /* Microphone mute */
>  
>  	/* Reasons for waking up from S3/S4 */
>  	TP_HKEY_EV_WKUP_S3_UNDOCK	= 0x2304, /* undock requested, S3 */
> @@ -5055,6 +5056,8 @@ static const char * const tpacpi_led_names[TPACPI_LED_NUMLEDS] = {
>  	"tpacpi::unknown_led2",
>  	"tpacpi::unknown_led3",
>  	"tpacpi::thinkvantage",
> +	"tpacpi::unknown_led4",
> +	"tpacpi::mute_mic",
>  };
>  #define TPACPI_SAFE_LEDS	0x1081U
>  
> @@ -5272,6 +5275,9 @@ 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) */
>  
> +	TPACPI_Q_LNV('6', 'U', 0x4087), /* T410s */
> +	TPACPI_Q_LNV('8', 'C', 0x4087), /* T420s */
> +
>  	/* (1) - may have excess leds enabled on MSB */
>  
>  	/* Defaults (order matters, keep last, don't reorder!) */

-- 
  "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 T410s and T420s LED support

[Matthew Garrett]

I'm a little unenthusiastic about just pulling this in without working 
out how userspace is going to consume it. It's a problem we may hit on 
other devices as well, so we potentially need some sort of consistent 
naming to indicate that it's a built-in mute LED.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] [thinkpad-acpi] Add T410s and T420s LED support

[Jens Taprogge]

On Fri, Mar 23, 2012 at 07:43:55PM +0000, Matthew Garrett wrote:
> I'm a little unenthusiastic about just pulling this in without working 
> out how userspace is going to consume it. It's a problem we may hit on 
> other devices as well, so we potentially need some sort of consistent 
> naming to indicate that it's a built-in mute LED.

Would a name like "laptop_mute_mic" be more descriptive?  Please note
that with the patch the LED only exposed if
CONFIG_THINKPAD_ACPI_UNSAFE_LEDS is set.  (And so are other like for
example the battery LEDs.)

If possible, I think it would be good to generally handle the LED in
kernel space.  Otherwise, the risk of the LED getting out of sync with
the actually muting is just to big.  I am not sure how that could be
achieved though.  Basically ALSA would have to expose some kind of hook
that thinkpad_acpi (or other users) could connect to.

Best Regards
-Jens

Re: [PATCH] [thinkpad-acpi] Add T410s and T420s LED support

[Henrique de Moraes Holschuh]

On Fri, 23 Mar 2012, Matthew Garrett wrote:
> I'm a little unenthusiastic about just pulling this in without working 
> out how userspace is going to consume it. It's a problem we may hit on 
> other devices as well, so we potentially need some sort of consistent 
> naming to indicate that it's a built-in mute LED.

Hmm, I am not enthusiastic about exposing MIC leds for userspace to screw up
at all.  I'd rather expose an alsa mixer control that lets one mute/unmute
the platform MIC, and have the kernel (if the platform doesn't do it
already) enforce the LED to always be in the correct state.

In my book, MICs getting enabled "stealthly" is something to be avoided on
the design.

However, I am not sure this platform interface exists on thinkpads (I do
think there is something, though.  Tracking the HKEY event on several DSDTs
and looking at what it touches in the EC, plus some experiments should be
able to give us a better picture).

Exposing the LED for anyone who wants to play with it looked like a good
compromise for the time being.  Since it is not considered a "safe" led, it
is not something that will be made generally available (requires a kconfig
option to be set, which distros are not supposed to enable).

I don't have anything against using a standard name, if such a thing exists
though.  But it would make a LOT more sense to have alsa provide a
standard-named *trigger* that could be hooked to any LED and follows the MIC
mute state of a mixer...

-- 
  "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 T410s and T420s LED support

[Jens Taprogge]

On Fri, Mar 23, 2012 at 08:38:07PM -0300, Henrique de Moraes Holschuh wrote:
> On Fri, 23 Mar 2012, Matthew Garrett wrote:
> > I'm a little unenthusiastic about just pulling this in without working 
> > out how userspace is going to consume it. It's a problem we may hit on 
> > other devices as well, so we potentially need some sort of consistent 
> > naming to indicate that it's a built-in mute LED.
> 
> Hmm, I am not enthusiastic about exposing MIC leds for userspace to screw up
> at all.  I'd rather expose an alsa mixer control that lets one mute/unmute
> the platform MIC, and have the kernel (if the platform doesn't do it
> already) enforce the LED to always be in the correct state.
> 
> In my book, MICs getting enabled "stealthly" is something to be avoided on
> the design.
> 
> However, I am not sure this platform interface exists on thinkpads (I do
> think there is something, though.  Tracking the HKEY event on several DSDTs
> and looking at what it touches in the EC, plus some experiments should be
> able to give us a better picture).
> 
> Exposing the LED for anyone who wants to play with it looked like a good
> compromise for the time being.  Since it is not considered a "safe" led, it
> is not something that will be made generally available (requires a kconfig
> option to be set, which distros are not supposed to enable).
> 
> I don't have anything against using a standard name, if such a thing exists
> though.  But it would make a LOT more sense to have alsa provide a
> standard-named *trigger* that could be hooked to any LED and follows the MIC
> mute state of a mixer...

I am cc:ing Takashi Iwai.

I am not sure thinkpad_acpi needs to provide a mixer.  I think what is
important is that the user can rely on the LED to be only on if the mic
is muted.  The mute key can be handled by userspace as long as the led
always gives the correct information.

The modern laptops are all based on HDA audio.  In the BIOS the
different functions of the respective connections are stored.  So the
sound layer knows which connection is the internal mic.  (An after
analyzing the information provided on the Thinkpads seems to be correct
in this regard.)

So to make sure the LED presents the actual state of the muting it would
be sufficient for ALSA to expose the mixer state to thinkpad_acpi or
better call a callback on changes.  To make it applicable to a wider
range of devices one could come up with a design like this:

   #define SND_ENABLED_INTMIC      1 << 0
   #define SND_ENABLED_INT_SPEAKER 1 << 16
   #define SND_ENABLED_ALLINPUTS   0x0000ffff
   #define SND_ENABLED_ALLOUTPUTS  0xffff0000

   /* returns current state or -1 for error */
   int snd_register_mute_cb(void (*callback)(int muted, void *private), void *private);
   int snd_unregister_mute_cb(void (*callback)(int muted, void *private));

This would give any other module the ability to receive a notification
when a device is muted.  The internal mic is special since the user can
not unplug it.  So it gets a defined position in the array.  Apart from
that one could use the LED to display if any mic is open.

Best Regards
Jens

Re: [PATCH] [thinkpad-acpi] Add T410s and T420s LED support

[Henrique de Moraes Holschuh]

On Sat, 24 Mar 2012, Jens Taprogge wrote:
> I am not sure thinkpad_acpi needs to provide a mixer.  I think what is

It does, but not on all thinkpads.  AFAIK, even the newer ones that are not
OEM junk with a thin layer of thinkpad IP added still have a FET mute-gating
the speakers, controlled by the EC.

> important is that the user can rely on the LED to be only on if the mic
> is muted.  The mute key can be handled by userspace as long as the led
> always gives the correct information.

And as long as it is not in control of the firmware.  Otherwise, bad things
happen.

> The modern laptops are all based on HDA audio.  In the BIOS the
> different functions of the respective connections are stored.  So the
> sound layer knows which connection is the internal mic.  (An after
> analyzing the information provided on the Thinkpads seems to be correct
> in this regard.)

Have you tested and made sure Lenovo did not mute-gate the MIC through the
EC on the better thinkpad lines?

> So to make sure the LED presents the actual state of the muting it would
> be sufficient for ALSA to expose the mixer state to thinkpad_acpi or
> better call a callback on changes.  To make it applicable to a wider
> range of devices one could come up with a design like this:
> 
>    #define SND_ENABLED_INTMIC      1 << 0
>    #define SND_ENABLED_INT_SPEAKER 1 << 16
>    #define SND_ENABLED_ALLINPUTS   0x0000ffff
>    #define SND_ENABLED_ALLOUTPUTS  0xffff0000
> 
>    /* returns current state or -1 for error */
>    int snd_register_mute_cb(void (*callback)(int muted, void *private), void *private);
>    int snd_unregister_mute_cb(void (*callback)(int muted, void *private));
> 
> This would give any other module the ability to receive a notification
> when a device is muted.  The internal mic is special since the user can
> not unplug it.  So it gets a defined position in the array.  Apart from
> that one could use the LED to display if any mic is open.

If you're going to do it that way, why not provide LED triggers?

-- 
  "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 T410s and T420s LED support

[Jens Taprogge]

On Fri, Mar 23, 2012 at 11:50:38PM -0300, Henrique de Moraes Holschuh wrote:
> On Sat, 24 Mar 2012, Jens Taprogge wrote:
> > I am not sure thinkpad_acpi needs to provide a mixer.  I think what is
> 
> It does, but not on all thinkpads.  AFAIK, even the newer ones that are not
> OEM junk with a thin layer of thinkpad IP added still have a FET mute-gating
> the speakers, controlled by the EC.

Sorry, I have been imprecise.  I am not questioning the need for the
output mixer.  The models I have looked at all feature the EC controlled
hardware muting.  What I was trying to say was that there does not
necessarily need to be a mixer for the mic to properly report the muting
state.

> 
> > important is that the user can rely on the LED to be only on if the mic
> > is muted.  The mute key can be handled by userspace as long as the led
> > always gives the correct information.
> 
> And as long as it is not in control of the firmware.  Otherwise, bad things
> happen.
Agreed. 

> > The modern laptops are all based on HDA audio.  In the BIOS the
> > different functions of the respective connections are stored.  So the
> > sound layer knows which connection is the internal mic.  (An after
> > analyzing the information provided on the Thinkpads seems to be correct
> > in this regard.)
> 
> Have you tested and made sure Lenovo did not mute-gate the MIC through the
> EC on the better thinkpad lines?

I have tested pressing the mute key as well as switching the LED on the
T410s and T420s.  The mic stays open.  

> 
> > So to make sure the LED presents the actual state of the muting it would
> > be sufficient for ALSA to expose the mixer state to thinkpad_acpi or
> > better call a callback on changes.  To make it applicable to a wider
> > range of devices one could come up with a design like this:
> > 
> >    #define SND_ENABLED_INTMIC      1 << 0
> >    #define SND_ENABLED_INT_SPEAKER 1 << 16
> >    #define SND_ENABLED_ALLINPUTS   0x0000ffff
> >    #define SND_ENABLED_ALLOUTPUTS  0xffff0000
> > 
> >    /* returns current state or -1 for error */
> >    int snd_register_mute_cb(void (*callback)(int muted, void *private), void *private);
> >    int snd_unregister_mute_cb(void (*callback)(int muted, void *private));
> > 
> > This would give any other module the ability to receive a notification
> > when a device is muted.  The internal mic is special since the user can
> > not unplug it.  So it gets a defined position in the array.  Apart from
> > that one could use the LED to display if any mic is open.
> 
> If you're going to do it that way, why not provide LED triggers?

I just did not know about them.  Is there a way to prevent userspace
from changing the trigger -> led association?

Takashi:  where would we need to add the LED triggers? In
core/control.c?  Would such a change be acceptable to ALSA?

Re: [PATCH] [thinkpad-acpi] Add T410s and T420s LED support

[Takashi Iwai]

At Sat, 24 Mar 2012 16:44:43 +0100,
Jens Taprogge wrote:
> 
> On Fri, Mar 23, 2012 at 11:50:38PM -0300, Henrique de Moraes Holschuh wrote:
> > On Sat, 24 Mar 2012, Jens Taprogge wrote:
> > > I am not sure thinkpad_acpi needs to provide a mixer.  I think what is
> > 
> > It does, but not on all thinkpads.  AFAIK, even the newer ones that are not
> > OEM junk with a thin layer of thinkpad IP added still have a FET mute-gating
> > the speakers, controlled by the EC.
> 
> Sorry, I have been imprecise.  I am not questioning the need for the
> output mixer.  The models I have looked at all feature the EC controlled
> hardware muting.  What I was trying to say was that there does not
> necessarily need to be a mixer for the mic to properly report the muting
> state.
> 
> > 
> > > important is that the user can rely on the LED to be only on if the mic
> > > is muted.  The mute key can be handled by userspace as long as the led
> > > always gives the correct information.
> > 
> > And as long as it is not in control of the firmware.  Otherwise, bad things
> > happen.
> Agreed. 
> 
> > > The modern laptops are all based on HDA audio.  In the BIOS the
> > > different functions of the respective connections are stored.  So the
> > > sound layer knows which connection is the internal mic.  (An after
> > > analyzing the information provided on the Thinkpads seems to be correct
> > > in this regard.)
> > 
> > Have you tested and made sure Lenovo did not mute-gate the MIC through the
> > EC on the better thinkpad lines?
> 
> I have tested pressing the mute key as well as switching the LED on the
> T410s and T420s.  The mic stays open.  
> 
> > 
> > > So to make sure the LED presents the actual state of the muting it would
> > > be sufficient for ALSA to expose the mixer state to thinkpad_acpi or
> > > better call a callback on changes.  To make it applicable to a wider
> > > range of devices one could come up with a design like this:
> > > 
> > >    #define SND_ENABLED_INTMIC      1 << 0
> > >    #define SND_ENABLED_INT_SPEAKER 1 << 16
> > >    #define SND_ENABLED_ALLINPUTS   0x0000ffff
> > >    #define SND_ENABLED_ALLOUTPUTS  0xffff0000
> > > 
> > >    /* returns current state or -1 for error */
> > >    int snd_register_mute_cb(void (*callback)(int muted, void *private), void *private);
> > >    int snd_unregister_mute_cb(void (*callback)(int muted, void *private));
> > > 
> > > This would give any other module the ability to receive a notification
> > > when a device is muted.  The internal mic is special since the user can
> > > not unplug it.  So it gets a defined position in the array.  Apart from
> > > that one could use the LED to display if any mic is open.
> > 
> > If you're going to do it that way, why not provide LED triggers?
> 
> I just did not know about them.  Is there a way to prevent userspace
> from changing the trigger -> led association?
> 
> Takashi:  where would we need to add the LED triggers? In
> core/control.c?  Would such a change be acceptable to ALSA?

Well, in the style above, you can't know which device is the mute
stuff bound with.  What if you have a USB device in addition, for
example?

I'm not sure whether such a hard-binding in the kernel side is
inevitably necessary.  And, if it is, then we shouldn't expose the
control to user-space.  Or, just let user-space, such as PulseAudio,
working on it.

(Cc'ed David, who mentioned about micmute stuff in other threads.)


thanks,

Takashi