[PATCH] thinkpad-acpi: disable alsa volume mixer for SL410/SL510

[PATCH] thinkpad-acpi: disable alsa volume mixer for SL410/SL510

[Keng-Yu Lin]

The mute key on SL410/SL510 only works when the alsa volume mixer
is not enabled. This patch makes the alsa volume mixer disabled
on the matched SL410/SL510 EC versions.

Signed-off-by: Keng-Yu Lin <keng-yu.lin@canonical.com>
---
 drivers/platform/x86/thinkpad_acpi.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

BugLink: https://launchpad.net/bugs/595896

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index eb99223..d4ee8f2 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -6870,6 +6870,7 @@ err_exit:
 
 #define TPACPI_VOL_Q_MUTEONLY	0x0001	/* Mute-only control available */
 #define TPACPI_VOL_Q_LEVEL	0x0002  /* Volume control available */
+#define TPACPI_VOL_Q_IGNORE	0x0003  /* No Volume control available */
 
 static const struct tpacpi_quirk volume_quirk_table[] __initconst = {
 	/* Whitelist volume level on all IBM by default */
@@ -6886,6 +6887,8 @@ static const struct tpacpi_quirk volume_quirk_table[] __initconst = {
 	TPACPI_QEC_LNV('7', 'J', TPACPI_VOL_Q_LEVEL), /* X60t */
 	TPACPI_QEC_LNV('7', '7', TPACPI_VOL_Q_LEVEL), /* Z60 */
 	TPACPI_QEC_LNV('7', 'F', TPACPI_VOL_Q_LEVEL), /* Z61 */
+	TPACPI_QEC_LNV('6', 'J', TPACPI_VOL_Q_IGNORE), /* SL410/SL510 */
+	TPACPI_QEC_LNV('7', 'X', TPACPI_VOL_Q_IGNORE), /* SL410/SL510 */
 
 	/* Whitelist mute-only on all Lenovo by default */
 	{ .vendor = PCI_VENDOR_ID_LENOVO,
@@ -6921,20 +6924,21 @@ static int __init volume_init(struct ibm_init_struct *iibm)
 	if (volume_capabilities >= TPACPI_VOL_CAP_MAX)
 		return -EINVAL;
 
+	quirks = tpacpi_check_quirks(volume_quirk_table,
+				     ARRAY_SIZE(volume_quirk_table));
+
 	/*
 	 * The ALSA mixer is our primary interface.
 	 * When disabled, don't install the subdriver at all
 	 */
-	if (!alsa_enable) {
+	if (!alsa_enable || quirks & TPACPI_VOL_Q_IGNORE) {
 		vdbg_printk(TPACPI_DBG_INIT | TPACPI_DBG_MIXER,
 			    "ALSA mixer disabled by parameter, "
 			    "not loading volume subdriver...\n");
+		alsa_enable = 0; /* reflect the quirk status in sysfs */
 		return 1;
 	}
 
-	quirks = tpacpi_check_quirks(volume_quirk_table,
-				     ARRAY_SIZE(volume_quirk_table));
-
 	switch (volume_capabilities) {
 	case TPACPI_VOL_CAP_AUTO:
 		if (quirks & TPACPI_VOL_Q_MUTEONLY)
-- 
1.7.1

Re: [ibm-acpi-devel] [PATCH] thinkpad-acpi: disable alsa volume mixer for SL410/SL510

[Henrique de Moraes Holschuh]

On Tue, 15 Mar 2011, Keng-Yu Lin wrote:
> The mute key on SL410/SL510 only works when the alsa volume mixer
> is not enabled. This patch makes the alsa volume mixer disabled
> on the matched SL410/SL510 EC versions.

I'd like more data on this.  Is this some sort of weird interaction with
userspace, or does the firmware actually changes behaviour when we
enable the MUTE HKEY event in the event mask?

>  #define TPACPI_VOL_Q_MUTEONLY	0x0001	/* Mute-only control available */
>  #define TPACPI_VOL_Q_LEVEL	0x0002  /* Volume control available */
> +#define TPACPI_VOL_Q_IGNORE	0x0003  /* No Volume control available */

Change the comment to /* Blacklist volume control */, please. And name
it TPACPI_VOL_Q_BLACKLIST.

> @@ -6921,20 +6924,21 @@ static int __init volume_init(struct ibm_init_struct *iibm)
>  	if (volume_capabilities >= TPACPI_VOL_CAP_MAX)
>  		return -EINVAL;
>  
> +	quirks = tpacpi_check_quirks(volume_quirk_table,
> +				     ARRAY_SIZE(volume_quirk_table));
> +
>  	/*
>  	 * The ALSA mixer is our primary interface.
>  	 * When disabled, don't install the subdriver at all
>  	 */
> -	if (!alsa_enable) {
> +	if (!alsa_enable || quirks & TPACPI_VOL_Q_IGNORE) {
>  		vdbg_printk(TPACPI_DBG_INIT | TPACPI_DBG_MIXER,
>  			    "ALSA mixer disabled by parameter, "
>  			    "not loading volume subdriver...\n");
> +		alsa_enable = 0; /* reflect the quirk status in sysfs */
>  		return 1;
>  	}
>  
> -	quirks = tpacpi_check_quirks(volume_quirk_table,
> -				     ARRAY_SIZE(volume_quirk_table));
> -
>  	switch (volume_capabilities) {
>  	case TPACPI_VOL_CAP_AUTO:
>  		if (quirks & TPACPI_VOL_Q_MUTEONLY)

Do not overload error messages like that.  It will now spill an
incorrect message when the quirk list blacklists the volume control.

Unfortunately, we're constrained by the unextensible alsa module command
line ABI, so something more sensible (implementing "auto") is not
available.

Instead, please do it like this:

1.  Do not move the quirks test or error message up.

2.  After the quirk check, do this:

if (quirks & TPACPI_VOL_Q_IGNORE) {
	dbg_printk(TPACPI_DBG_INIT | TPACPI_DBG_MIXER,
		   "ALSA mixer blacklisted for this model, not loading
volume subdriver...\n");
	return 1;
}

Note that it is a "dbg_printk", and NOT a "vdbg_printk".

I'd much rather have something that allows the user to override the
blacklisting, though.  But it cannot be done through alsa_enable.  Maybe
you could be persuaded to add support for a "force_volume" parameter?

If you do add support for "force_volume", please make it an unsigned int
(not bool), use 0 for no, and 1 for yes, and leave the other values
undefined.  And add it to the documentation in
Documentation/laptops/thinkpad-acpi.txt.

-- 
  "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: disable alsa volume mixer for SL410/SL510

[Keng-Yü Lin]

On Thu, Mar 17, 2011 at 2:57 AM, Henrique de Moraes Holschuh
<hmh@hmh.eng.br> wrote:
> On Tue, 15 Mar 2011, Keng-Yu Lin wrote:
>> The mute key on SL410/SL510 only works when the alsa volume mixer
>> is not enabled. This patch makes the alsa volume mixer disabled
>> on the matched SL410/SL510 EC versions.
>
> I'd like more data on this.  Is this some sort of weird interaction with
> userspace, or does the firmware actually changes behaviour when we
> enable the MUTE HKEY event in the event mask?
>

The firmware does not change its behaviour with the key enabled in the keymask.
The mute key (and the LED on it) of the two models works good without
thinkpad_acpi loaded. So in the patch I like to exclude the volume
control part for the two models.

>>  #define TPACPI_VOL_Q_MUTEONLY        0x0001  /* Mute-only control available */
>>  #define TPACPI_VOL_Q_LEVEL   0x0002  /* Volume control available */
>> +#define TPACPI_VOL_Q_IGNORE  0x0003  /* No Volume control available */
>
> Change the comment to /* Blacklist volume control */, please. And name
> it TPACPI_VOL_Q_BLACKLIST.
>
>> @@ -6921,20 +6924,21 @@ static int __init volume_init(struct ibm_init_struct *iibm)
>>       if (volume_capabilities >= TPACPI_VOL_CAP_MAX)
>>               return -EINVAL;
>>
>> +     quirks = tpacpi_check_quirks(volume_quirk_table,
>> +                                  ARRAY_SIZE(volume_quirk_table));
>> +
>>       /*
>>        * The ALSA mixer is our primary interface.
>>        * When disabled, don't install the subdriver at all
>>        */
>> -     if (!alsa_enable) {
>> +     if (!alsa_enable || quirks & TPACPI_VOL_Q_IGNORE) {
>>               vdbg_printk(TPACPI_DBG_INIT | TPACPI_DBG_MIXER,
>>                           "ALSA mixer disabled by parameter, "
>>                           "not loading volume subdriver...\n");
>> +             alsa_enable = 0; /* reflect the quirk status in sysfs */
>>               return 1;
>>       }
>>
>> -     quirks = tpacpi_check_quirks(volume_quirk_table,
>> -                                  ARRAY_SIZE(volume_quirk_table));
>> -
>>       switch (volume_capabilities) {
>>       case TPACPI_VOL_CAP_AUTO:
>>               if (quirks & TPACPI_VOL_Q_MUTEONLY)
>
> Do not overload error messages like that.  It will now spill an
> incorrect message when the quirk list blacklists the volume control.
>
> Unfortunately, we're constrained by the unextensible alsa module command
> line ABI, so something more sensible (implementing "auto") is not
> available.
>
> Instead, please do it like this:
>
> 1.  Do not move the quirks test or error message up.
>
> 2.  After the quirk check, do this:
>
> if (quirks & TPACPI_VOL_Q_IGNORE) {
>        dbg_printk(TPACPI_DBG_INIT | TPACPI_DBG_MIXER,
>                   "ALSA mixer blacklisted for this model, not loading
> volume subdriver...\n");
>        return 1;
> }
>
> Note that it is a "dbg_printk", and NOT a "vdbg_printk".
>
> I'd much rather have something that allows the user to override the
> blacklisting, though.  But it cannot be done through alsa_enable.  Maybe
> you could be persuaded to add support for a "force_volume" parameter?
>
> If you do add support for "force_volume", please make it an unsigned int
> (not bool), use 0 for no, and 1 for yes, and leave the other values
> undefined.  And add it to the documentation in
> Documentation/laptops/thinkpad-acpi.txt.
>

Thanks for the suggestion. I will be sending a new version of patches.

Re: [ibm-acpi-devel] [PATCH] thinkpad-acpi: disable alsa volume mixer for SL410/SL510

[Henrique de Moraes Holschuh]

On Thu, 06 Oct 2011, Keng-Yü Lin wrote:
> On Thu, Mar 17, 2011 at 2:57 AM, Henrique de Moraes Holschuh
> <hmh@hmh.eng.br> wrote:
> > On Tue, 15 Mar 2011, Keng-Yu Lin wrote:
> >> The mute key on SL410/SL510 only works when the alsa volume mixer
> >> is not enabled. This patch makes the alsa volume mixer disabled
> >> on the matched SL410/SL510 EC versions.
> >
> > I'd like more data on this.  Is this some sort of weird interaction with
> > userspace, or does the firmware actually changes behaviour when we
> > enable the MUTE HKEY event in the event mask?
> 
> The firmware does not change its behaviour with the key enabled in the keymask.
> The mute key (and the LED on it) of the two models works good without
> thinkpad_acpi loaded. So in the patch I like to exclude the volume
> control part for the two models.

If the firmware does not change behaviour, doesn't that mean the bug is
elsewhere?

Is the firmware reporting these keys somewhere else (e.g. through the
keyboard)?

-- 
  "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: disable alsa volume mixer for SL410/SL510

[Keng-Yü Lin]

On Fri, Oct 7, 2011 at 2:12 AM, Henrique de Moraes Holschuh
<hmh@hmh.eng.br> wrote:
> On Thu, 06 Oct 2011, Keng-Yü Lin wrote:
>> On Thu, Mar 17, 2011 at 2:57 AM, Henrique de Moraes Holschuh
>> <hmh@hmh.eng.br> wrote:
>> > On Tue, 15 Mar 2011, Keng-Yu Lin wrote:
>> >> The mute key on SL410/SL510 only works when the alsa volume mixer
>> >> is not enabled. This patch makes the alsa volume mixer disabled
>> >> on the matched SL410/SL510 EC versions.
>> >
>> > I'd like more data on this.  Is this some sort of weird interaction with
>> > userspace, or does the firmware actually changes behaviour when we
>> > enable the MUTE HKEY event in the event mask?
>>
>> The firmware does not change its behaviour with the key enabled in the keymask.
>> The mute key (and the LED on it) of the two models works good without
>> thinkpad_acpi loaded. So in the patch I like to exclude the volume
>> control part for the two models.
>
> If the firmware does not change behaviour, doesn't that mean the bug is
> elsewhere?
>
> Is the firmware reporting these keys somewhere else (e.g. through the
> keyboard)?
>

As the users reported in https://launchpad.net/bugs/595896, there is
no KEY_MUTE either from thinkpad_acpi nor the keyboard when pressing
the mute hotkey. (I do not really possess the hardware. but just
debugged/helped remotely).

It will be really appreciated if there is any suggestion/insight for
looking into the bug.  At the moment I can just find this fix for the
issue.