[RFC PATCH 0/2] Enable mute LEDs and mic mute LEDs on Thinkpad

[RFC PATCH 0/2] Enable mute LEDs and mic mute LEDs on Thinkpad

[David Henningsson]

Hi!

These two patches bind the thinkpad-acpi and the snd-hda-intel drivers together
so that the mute and mic-mute LEDs correspond to the current mute/micmute status
of the internal sound card.

The patches are against Takashi's sound tree and are working on a
Thinkpad X220-tablet, but the patches are otherwise in draft quality. Which means
that there are several outstanding issues that would need feedback from upstream.

First, I'd like feedback on the connection between the drivers - Takashi suggested
that we could bind the drivers together like we've recently done when connecting
the snd-hda-intel and the i915 driver, so this follows a similar scheme, with
exported symbols from the thinkpad-acpi driver and a symbol_request on the
snd-hda-intel side.
This also opens up for future improvements so that the hardware mute/volume does
not need to be its own card, but instead properly integrate with the output it
actually controls on the kernel side.

Second, about the LEDs as a security feature. As implemented in these patches
there is no userspace interface at all, which would meet Matthew's concern about
that. A future improvement could be that an alsamixer control was added, that
allows the LED to be turned off, but not on. Which means, if PulseAudio or other
daemon knows that recording goes on from another source (e g bluetooth or network
connected mic) it could turn the mic mute LED off to indicate that to the user,
but the only way to turn the LED on is to actually mute the internal card's
hardware.

Finally, it is my hope that I will get some quick and constructive feedback so
we can have it all in order to be merged in time for the 3.13 kernel merge window.
I'll be at Linuxcon Europe next week to answer questions if you have any, and
we might also spend some time talking about it on the audio meeting on Monday.

Thanks in advance!

David Henningsson (2):
  thinkpad-acpi: Add mute and mic-mute LED functionality
  ALSA: hda - add connection to thinkpad_acpi to control mute/micmute
    LEDs

 drivers/platform/x86/thinkpad_acpi.c |   94 +++++++++++++++++++++++++++++++++-
 include/linux/thinkpad_acpi.h        |   10 ++++
 sound/pci/hda/patch_conexant.c       |   78 ++++++++++++++++++++++++++++
 3 files changed, 181 insertions(+), 1 deletion(-)
 create mode 100644 include/linux/thinkpad_acpi.h

-- 
1.7.9.5

[RFC PATCH 1/2] thinkpad-acpi: Add mute and mic-mute LED functionality

[David Henningsson]

Questions / notes:

This patch supersedes the second of Alex Hung's patches posted earlier at
http://www.spinics.net/lists/platform-driver-x86/msg04673.html

Not sure if thinkpad_acpi should be dropped into include/linux
though, any better suggestion?

Should TPACPI_VERSION be increased because we added a new LED driver?

Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---
 drivers/platform/x86/thinkpad_acpi.c |   94 +++++++++++++++++++++++++++++++++-
 include/linux/thinkpad_acpi.h        |   10 ++++
 2 files changed, 103 insertions(+), 1 deletion(-)
 create mode 100644 include/linux/thinkpad_acpi.h

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 03ca6c1..ecdfeae 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -23,7 +23,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
-#define TPACPI_VERSION "0.24"
+#define TPACPI_VERSION "0.25"
 #define TPACPI_SYSFS_VERSION 0x020700
 
 /*
@@ -88,6 +88,7 @@
 
 #include <linux/pci_ids.h>
 
+#include <linux/thinkpad_acpi.h>
 
 /* ThinkPad CMOS commands */
 #define TP_CMOS_VOLUME_DOWN	0
@@ -8350,6 +8351,93 @@ static struct ibm_struct fan_driver_data = {
 	.resume = fan_resume,
 };
 
+/*************************************************************************
+ * Mute LED subdriver
+ */
+
+#define MUTE_LED_INDEX 0
+#define MICMUTE_LED_INDEX 1
+
+static int mute_led_on_off(int whichled, int state)
+{
+	int output;
+	acpi_handle temp;
+
+	acpi_string m;
+	if (whichled == MICMUTE_LED_INDEX) {
+		state = state > 0 ? 2 : 0;
+		m = "MMTS";
+	} else {
+		state = state > 0 ? 1 : 0;
+		m = "SSMS";
+	}
+
+	if (!ACPI_SUCCESS(acpi_get_handle(hkey_handle, m, &temp))) {
+		pr_warn("Thinkpad ACPI has no %s interface.\n", m);
+		return -EIO;
+	}
+
+	if (!acpi_evalf(hkey_handle, &output, m, "dd", state))
+		return -EIO;
+
+	return 0;
+}
+
+static unsigned int mute_led_state;
+static unsigned int micmute_led_state;
+
+int tpacpi_mute_led_set(int on)
+{
+	int err;
+	int state = on ? 1 : 0;
+	if (mute_led_state < 0 || mute_led_state == state)
+		return mute_led_state;
+	err = mute_led_on_off(MUTE_LED_INDEX, state);
+	mute_led_state = err ? err : state;
+	return err;
+}
+EXPORT_SYMBOL(tpacpi_mute_led_set);
+
+int tpacpi_micmute_led_set(int on)
+{
+	int err;
+	int state = on ? 1 : 0;
+	if (micmute_led_state < 0 || micmute_led_state == state)
+		return micmute_led_state;
+	err = mute_led_on_off(MICMUTE_LED_INDEX, state);
+	micmute_led_state = err ? err : state;
+	return err;
+}
+EXPORT_SYMBOL(tpacpi_micmute_led_set);
+
+static int mute_led_init(struct ibm_init_struct *iibm)
+{
+	acpi_handle temp;
+
+	if (ACPI_SUCCESS(acpi_get_handle(hkey_handle, "MMTG", &temp)))
+		micmute_led_state = mute_led_on_off(MICMUTE_LED_INDEX, 0);
+	else
+		micmute_led_state = -ENODEV;
+
+	if (ACPI_SUCCESS(acpi_get_handle(hkey_handle, "GSMS", &temp)))
+		mute_led_state = mute_led_on_off(MUTE_LED_INDEX, 0);
+	else
+		mute_led_state = -ENODEV;
+
+	return 0;
+}
+
+static void mute_led_exit(void)
+{
+	tpacpi_mute_led_set(0);
+	tpacpi_micmute_led_set(0);
+}
+
+static struct ibm_struct mute_led_driver_data = {
+	.name = "mute_led",
+	.exit = mute_led_exit,
+};
+
 /****************************************************************************
  ****************************************************************************
  *
@@ -8768,6 +8856,10 @@ static struct ibm_init_struct ibms_init[] __initdata = {
 		.init = fan_init,
 		.data = &fan_driver_data,
 	},
+	{
+		.init = mute_led_init,
+		.data = &mute_led_driver_data,
+	},
 };
 
 static int __init set_ibm_param(const char *val, struct kernel_param *kp)
diff --git a/include/linux/thinkpad_acpi.h b/include/linux/thinkpad_acpi.h
new file mode 100644
index 0000000..9fecd15
--- /dev/null
+++ b/include/linux/thinkpad_acpi.h
@@ -0,0 +1,10 @@
+#ifndef __THINKPAD_ACPI_H__
+#define __THINKPAD_ACPI_H__
+
+/* These two functions return 0 if success, or negative error code
+   (e g -ENODEV if no led present) */
+
+int tpacpi_mute_led_set(int on);
+int tpacpi_micmute_led_set(int on);
+
+#endif
-- 
1.7.9.5

[RFC PATCH 2/2] ALSA: hda - add connection to thinkpad_acpi to control mute/micmute LEDs

[David Henningsson]

Notes/questions:

For mute LEDs we can follow master like we already do on HP machines, but for
mic mutes, we might have several capture switches. Honestly, on these laptops
which almost always have autoswitched mics, there is only one recording source
anyway. Can't we simply remove the extra capture PCMs, that seems easiest?

For some reason CONFIG_THINKPAD_ACPI did not work here (it was not defined),
but maybe it's due to the way I'm testing, or I'm missing something obvious. 
Not sure.

There are Thinkpad's with Realtek codecs as well which have mute/micmute.
Maybe we should consider a more generic solution instead of copy-pasting
between differen patch_* files?

And also, we're missing a symbol_put in this version. Should we add a new
"free" fixup action where we can add a call to symbol_put, or do you have a
better suggestion?

Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---
 sound/pci/hda/patch_conexant.c |   78 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)

diff --git a/sound/pci/hda/patch_conexant.c b/sound/pci/hda/patch_conexant.c
index ec68eae..237df7f 100644
--- a/sound/pci/hda/patch_conexant.c
+++ b/sound/pci/hda/patch_conexant.c
@@ -3232,8 +3232,80 @@ enum {
 	CXT_FIXUP_HEADPHONE_MIC_PIN,
 	CXT_FIXUP_HEADPHONE_MIC,
 	CXT_FIXUP_GPIO1,
+	CXT_FIXUP_THINKPAD_ACPI,
 };
 
+/* FIXME: figure out why #ifdef CONFIG_THINKPAD_ACPI does not work */
+
+#if 1 /* #ifdef CONFIG_THINKPAD_ACPI */
+
+#include <linux/thinkpad_acpi.h>
+
+static int (*mute_led_set_func)(int);
+static int (*micmute_led_set_func)(int);
+
+static void update_tpacpi_mute_led(void *private_data, int enabled)
+{
+	struct hda_codec *codec = private_data;
+	struct conexant_spec *spec = codec->spec;
+
+	if (spec->dynamic_eapd)
+		cx_auto_vmaster_hook(private_data, enabled);
+
+	if (mute_led_set_func)
+		mute_led_set_func(!enabled);
+}
+
+static void update_tpacpi_micmute_led(struct hda_codec *codec,
+				      struct snd_ctl_elem_value *ucontrol)
+{
+	int val;
+	if (!ucontrol || !micmute_led_set_func)
+		return;
+	/* TODO: if this is a stereo switch, better check both channels? What about additional capture switches with indices, on other ADCs? */
+	val = ucontrol->value.integer.value[0];
+	if (strcmp("Capture Switch", ucontrol->id.name) == 0 && ucontrol->id.index == 0)
+		micmute_led_set_func(!val);
+
+}
+
+static void cxt_fixup_thinkpad_acpi(struct hda_codec *codec,
+				  const struct hda_fixup *fix, int action)
+{
+	struct conexant_spec *spec = codec->spec;
+	if (action == HDA_FIXUP_ACT_PRE_PROBE) {
+		mute_led_set_func = symbol_request(tpacpi_mute_led_set);
+		if (!mute_led_set_func)
+			snd_printk(KERN_WARNING "Failed to find thinkpad-acpi symbol tpacpi_mute_led_set\n");
+		else if (mute_led_set_func(0) < 0) {
+			symbol_put(mute_led_set_func);
+			mute_led_set_func = NULL;
+		}
+
+		micmute_led_set_func = symbol_request(tpacpi_micmute_led_set);
+		if (!micmute_led_set_func)
+			snd_printk(KERN_WARNING "Failed to find thinkpad-acpi symbol tpacpi_micmute_led_set\n");
+		else if (micmute_led_set_func(0) < 0) {
+			symbol_put(micmute_led_set_func);
+			micmute_led_set_func = NULL;
+		}
+	/* FIXME: when do we call symbol_put? In a new FREE fixup action? */
+	}
+	if (action == HDA_FIXUP_ACT_PROBE && micmute_led_set_func)
+		spec->gen.cap_sync_hook = update_tpacpi_micmute_led;
+	if (action == HDA_FIXUP_ACT_PROBE && mute_led_set_func)
+		spec->gen.vmaster_mute.hook = update_tpacpi_mute_led;
+}
+
+#else
+
+static void cxt_fixup_thinkpad_acpi(struct hda_codec *codec,
+				  const struct hda_fixup *fix, int action)
+{
+}
+
+#endif
+
 static void cxt_fixup_stereo_dmic(struct hda_codec *codec,
 				  const struct hda_fixup *fix, int action)
 {
@@ -3344,6 +3416,8 @@ static const struct hda_fixup cxt_fixups[] = {
 	[CXT_PINCFG_LENOVO_TP410] = {
 		.type = HDA_FIXUP_PINS,
 		.v.pins = cxt_pincfg_lenovo_tp410,
+		.chained = true,
+		.chain_id = CXT_FIXUP_THINKPAD_ACPI,
 	},
 	[CXT_PINCFG_LEMOTE_A1004] = {
 		.type = HDA_FIXUP_PINS,
@@ -3385,6 +3459,10 @@ static const struct hda_fixup cxt_fixups[] = {
 			{ }
 		},
 	},
+	[CXT_FIXUP_THINKPAD_ACPI] = {
+		.type = HDA_FIXUP_FUNC,
+		.v.func = cxt_fixup_thinkpad_acpi,
+	},
 };
 
 static const struct snd_pci_quirk cxt5051_fixups[] = {
-- 
1.7.9.5

Re: [RFC PATCH 1/2] thinkpad-acpi: Add mute and mic-mute LED functionality

[Henrique de Moraes Holschuh]

On Wed, 16 Oct 2013, David Henningsson wrote:
> Not sure if thinkpad_acpi should be dropped into include/linux
> though, any better suggestion?

I'm fine with it wherever...

> Should TPACPI_VERSION be increased because we added a new LED driver?

TPACPI_SYSFS_VERSION needs to be increased when you add some feature or
change some behaviour that userspace needs to know to be present/ausent AND
which cannot be detected by other means (such as the presence of a sysfs
node).  A good example is poll() support for sysfs nodes.

If you need to bump TPACPI_SYSFS_VERSION, you absolutely must add the proper
documentation for the feature to Documentation/laptops/thinkpad-acpi.txt.

TPACPI_VERSION is mostly cosmetic, and adding the mute LED driver looks like
as good a reason to bump it as any other.  You need to at least describe the
new functionality and bump the driver version and date in
Documentation/laptops/thinkpad-acpi.txt.

Other than that, it looks good from the thinkpad-acpi side.  I'll ack it if
you update 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: [RFC PATCH 1/2] thinkpad-acpi: Add mute and mic-mute LED functionality

[Takashi Iwai]

At Wed, 16 Oct 2013 14:15:35 +0200,
David Henningsson wrote:
> 
> Questions / notes:
> 
> This patch supersedes the second of Alex Hung's patches posted earlier at
> http://www.spinics.net/lists/platform-driver-x86/msg04673.html
> 
> Not sure if thinkpad_acpi should be dropped into include/linux
> though, any better suggestion?
> 
> Should TPACPI_VERSION be increased because we added a new LED driver?
> 
> Signed-off-by: David Henningsson <david.henningsson@canonical.com>
> ---
>  drivers/platform/x86/thinkpad_acpi.c |   94 +++++++++++++++++++++++++++++++++-
>  include/linux/thinkpad_acpi.h        |   10 ++++
>  2 files changed, 103 insertions(+), 1 deletion(-)
>  create mode 100644 include/linux/thinkpad_acpi.h
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 03ca6c1..ecdfeae 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -23,7 +23,7 @@
>  
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
> -#define TPACPI_VERSION "0.24"
> +#define TPACPI_VERSION "0.25"
>  #define TPACPI_SYSFS_VERSION 0x020700
>  
>  /*
> @@ -88,6 +88,7 @@
>  
>  #include <linux/pci_ids.h>
>  
> +#include <linux/thinkpad_acpi.h>
>  
>  /* ThinkPad CMOS commands */
>  #define TP_CMOS_VOLUME_DOWN	0
> @@ -8350,6 +8351,93 @@ static struct ibm_struct fan_driver_data = {
>  	.resume = fan_resume,
>  };
>  
> +/*************************************************************************
> + * Mute LED subdriver
> + */
> +
> +#define MUTE_LED_INDEX 0
> +#define MICMUTE_LED_INDEX 1
> +
> +static int mute_led_on_off(int whichled, int state)
> +{
> +	int output;
> +	acpi_handle temp;
> +
> +	acpi_string m;
> +	if (whichled == MICMUTE_LED_INDEX) {
> +		state = state > 0 ? 2 : 0;
> +		m = "MMTS";
> +	} else {
> +		state = state > 0 ? 1 : 0;
> +		m = "SSMS";
> +	}
> +
> +	if (!ACPI_SUCCESS(acpi_get_handle(hkey_handle, m, &temp))) {
> +		pr_warn("Thinkpad ACPI has no %s interface.\n", m);
> +		return -EIO;
> +	}
> +
> +	if (!acpi_evalf(hkey_handle, &output, m, "dd", state))
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +static unsigned int mute_led_state;
> +static unsigned int micmute_led_state;
> +
> +int tpacpi_mute_led_set(int on)
> +{
> +	int err;
> +	int state = on ? 1 : 0;
> +	if (mute_led_state < 0 || mute_led_state == state)
> +		return mute_led_state;
> +	err = mute_led_on_off(MUTE_LED_INDEX, state);
> +	mute_led_state = err ? err : state;
> +	return err;
> +}
> +EXPORT_SYMBOL(tpacpi_mute_led_set);
> +
> +int tpacpi_micmute_led_set(int on)
> +{
> +	int err;
> +	int state = on ? 1 : 0;
> +	if (micmute_led_state < 0 || micmute_led_state == state)
> +		return micmute_led_state;
> +	err = mute_led_on_off(MICMUTE_LED_INDEX, state);
> +	micmute_led_state = err ? err : state;
> +	return err;
> +}
> +EXPORT_SYMBOL(tpacpi_micmute_led_set);
> +
> +static int mute_led_init(struct ibm_init_struct *iibm)
> +{
> +	acpi_handle temp;
> +
> +	if (ACPI_SUCCESS(acpi_get_handle(hkey_handle, "MMTG", &temp)))
> +		micmute_led_state = mute_led_on_off(MICMUTE_LED_INDEX, 0);
> +	else
> +		micmute_led_state = -ENODEV;
> +
> +	if (ACPI_SUCCESS(acpi_get_handle(hkey_handle, "GSMS", &temp)))
> +		mute_led_state = mute_led_on_off(MUTE_LED_INDEX, 0);
> +	else
> +		mute_led_state = -ENODEV;
> +
> +	return 0;
> +}
> +
> +static void mute_led_exit(void)
> +{
> +	tpacpi_mute_led_set(0);
> +	tpacpi_micmute_led_set(0);
> +}
> +
> +static struct ibm_struct mute_led_driver_data = {
> +	.name = "mute_led",
> +	.exit = mute_led_exit,
> +};


How about managing with a table?  For example,

struct tp_led_table {
	acpi_string name;
	int on_value;
	int off_value;
	int state;
};

static struct tp_led_table led_tables[] = {
	[TP_LED_MUTE] = {
		.name = "SSMS",
		.on_value = 1,
		.off_value = 0,
	},
	[TP_LED_MICMUTE] = {
		.name = "MMTS",
		.on_value = 2,
		.off_value = 0,
	},
};

static int mute_led_on_off(struct tp_led_table *t, bool state)
{
	acpi_handle temp;

	if (!ACPI_SUCCESS(acpi_get_handle(hkey_handle, t->name, &temp))) {
		pr_warn("Thinkpad ACPI has no %s interface.\n", t->name);
		return -EIO;
	}

	if (!acpi_evalf(hkey_handle, &output, t->name, "dd",
			state ? t->on_value : t->off_value))
		return -EIO;

	t->state = state;
	return state;
}

int tpacpi_led_set(int whichled, bool on)
{
	struct tp_led_table *t;

	if (whichled < 0 || whichled >= TP_LED_MAX)
		return -EINVAL;

	t = &led_tables[whichled];
	if (t->state < 0 || t->state == on)
		return t->state;
	return mute_led_on_off(t, on);
}
EXPORT_SYMBOL_GPL(tpacpi_led_set);

static int mute_led_init(struct ibm_init_struct *iibm)
{
	acpi_handle temp;
	int i;

	for (i = 0; i < TP_LED_MAX; i++) {
		struct tp_led_table *t = &led_tables[i];
		if (ACPI_SUCCESS(acpi_get_handle(hkey_handle, t->name, &temp)))
			mute_led_on_off(t, false);
		else
			t-.state = -ENODEV;
	}
	return 0;
}

static void mute_led_exit(void)
{
	int i;

	for (i = 0; i < TP_LED_MAX; i++)
		tpacpi_led_set(i, false);
}


Also, the exported symbol should be marked with *_GPL().


thanks,

Takashi

Re: [RFC PATCH 1/2] thinkpad-acpi: Add mute and mic-mute LED functionality

[David Henningsson]

On 10/16/2013 03:09 PM, Takashi Iwai wrote:
> At Wed, 16 Oct 2013 14:15:35 +0200,
> David Henningsson wrote:
>>
>> Questions / notes:
>>
>> This patch supersedes the second of Alex Hung's patches posted earlier at
>> http://www.spinics.net/lists/platform-driver-x86/msg04673.html
>>
>> Not sure if thinkpad_acpi should be dropped into include/linux
>> though, any better suggestion?
>>
>> Should TPACPI_VERSION be increased because we added a new LED driver?
>>
>> Signed-off-by: David Henningsson <david.henningsson@canonical.com>
>> ---
>>  drivers/platform/x86/thinkpad_acpi.c |   94 +++++++++++++++++++++++++++++++++-
>>  include/linux/thinkpad_acpi.h        |   10 ++++
>>  2 files changed, 103 insertions(+), 1 deletion(-)
>>  create mode 100644 include/linux/thinkpad_acpi.h
>>
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
>> index 03ca6c1..ecdfeae 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -23,7 +23,7 @@
>>  
>>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>  
>> -#define TPACPI_VERSION "0.24"
>> +#define TPACPI_VERSION "0.25"
>>  #define TPACPI_SYSFS_VERSION 0x020700
>>  
>>  /*
>> @@ -88,6 +88,7 @@
>>  
>>  #include <linux/pci_ids.h>
>>  
>> +#include <linux/thinkpad_acpi.h>
>>  
>>  /* ThinkPad CMOS commands */
>>  #define TP_CMOS_VOLUME_DOWN	0
>> @@ -8350,6 +8351,93 @@ static struct ibm_struct fan_driver_data = {
>>  	.resume = fan_resume,
>>  };
>>  
>> +/*************************************************************************
>> + * Mute LED subdriver
>> + */
>> +
>> +#define MUTE_LED_INDEX 0
>> +#define MICMUTE_LED_INDEX 1
>> +
>> +static int mute_led_on_off(int whichled, int state)
>> +{
>> +	int output;
>> +	acpi_handle temp;
>> +
>> +	acpi_string m;
>> +	if (whichled == MICMUTE_LED_INDEX) {
>> +		state = state > 0 ? 2 : 0;
>> +		m = "MMTS";
>> +	} else {
>> +		state = state > 0 ? 1 : 0;
>> +		m = "SSMS";
>> +	}
>> +
>> +	if (!ACPI_SUCCESS(acpi_get_handle(hkey_handle, m, &temp))) {
>> +		pr_warn("Thinkpad ACPI has no %s interface.\n", m);
>> +		return -EIO;
>> +	}
>> +
>> +	if (!acpi_evalf(hkey_handle, &output, m, "dd", state))
>> +		return -EIO;
>> +
>> +	return 0;
>> +}
>> +
>> +static unsigned int mute_led_state;
>> +static unsigned int micmute_led_state;
>> +
>> +int tpacpi_mute_led_set(int on)
>> +{
>> +	int err;
>> +	int state = on ? 1 : 0;
>> +	if (mute_led_state < 0 || mute_led_state == state)
>> +		return mute_led_state;
>> +	err = mute_led_on_off(MUTE_LED_INDEX, state);
>> +	mute_led_state = err ? err : state;
>> +	return err;
>> +}
>> +EXPORT_SYMBOL(tpacpi_mute_led_set);
>> +
>> +int tpacpi_micmute_led_set(int on)
>> +{
>> +	int err;
>> +	int state = on ? 1 : 0;
>> +	if (micmute_led_state < 0 || micmute_led_state == state)
>> +		return micmute_led_state;
>> +	err = mute_led_on_off(MICMUTE_LED_INDEX, state);
>> +	micmute_led_state = err ? err : state;
>> +	return err;
>> +}
>> +EXPORT_SYMBOL(tpacpi_micmute_led_set);
>> +
>> +static int mute_led_init(struct ibm_init_struct *iibm)
>> +{
>> +	acpi_handle temp;
>> +
>> +	if (ACPI_SUCCESS(acpi_get_handle(hkey_handle, "MMTG", &temp)))
>> +		micmute_led_state = mute_led_on_off(MICMUTE_LED_INDEX, 0);
>> +	else
>> +		micmute_led_state = -ENODEV;
>> +
>> +	if (ACPI_SUCCESS(acpi_get_handle(hkey_handle, "GSMS", &temp)))
>> +		mute_led_state = mute_led_on_off(MUTE_LED_INDEX, 0);
>> +	else
>> +		mute_led_state = -ENODEV;
>> +
>> +	return 0;
>> +}
>> +
>> +static void mute_led_exit(void)
>> +{
>> +	tpacpi_mute_led_set(0);
>> +	tpacpi_micmute_led_set(0);
>> +}
>> +
>> +static struct ibm_struct mute_led_driver_data = {
>> +	.name = "mute_led",
>> +	.exit = mute_led_exit,
>> +};
> 
> 
> How about managing with a table?  

Sure, that looks a bit more elegant. I'll incorporate your suggestions
into v2 of the patch (together with the documentation additions
suggested by Henrique), when you have reviewed the ALSA part too.

For example,
> 
> struct tp_led_table {
> 	acpi_string name;
> 	int on_value;
> 	int off_value;
> 	int state;
> };
> 
> static struct tp_led_table led_tables[] = {
> 	[TP_LED_MUTE] = {
> 		.name = "SSMS",
> 		.on_value = 1,
> 		.off_value = 0,
> 	},
> 	[TP_LED_MICMUTE] = {
> 		.name = "MMTS",
> 		.on_value = 2,
> 		.off_value = 0,
> 	},
> };
> 
> static int mute_led_on_off(struct tp_led_table *t, bool state)
> {
> 	acpi_handle temp;
> 
> 	if (!ACPI_SUCCESS(acpi_get_handle(hkey_handle, t->name, &temp))) {
> 		pr_warn("Thinkpad ACPI has no %s interface.\n", t->name);
> 		return -EIO;
> 	}
> 
> 	if (!acpi_evalf(hkey_handle, &output, t->name, "dd",
> 			state ? t->on_value : t->off_value))
> 		return -EIO;
> 
> 	t->state = state;
> 	return state;
> }
> 
> int tpacpi_led_set(int whichled, bool on)
> {
> 	struct tp_led_table *t;
> 
> 	if (whichled < 0 || whichled >= TP_LED_MAX)
> 		return -EINVAL;
> 
> 	t = &led_tables[whichled];
> 	if (t->state < 0 || t->state == on)
> 		return t->state;
> 	return mute_led_on_off(t, on);
> }
> EXPORT_SYMBOL_GPL(tpacpi_led_set);
> 
> static int mute_led_init(struct ibm_init_struct *iibm)
> {
> 	acpi_handle temp;
> 	int i;
> 
> 	for (i = 0; i < TP_LED_MAX; i++) {
> 		struct tp_led_table *t = &led_tables[i];
> 		if (ACPI_SUCCESS(acpi_get_handle(hkey_handle, t->name, &temp)))
> 			mute_led_on_off(t, false);
> 		else
> 			t-.state = -ENODEV;
> 	}
> 	return 0;
> }
> 
> static void mute_led_exit(void)
> {
> 	int i;
> 
> 	for (i = 0; i < TP_LED_MAX; i++)
> 		tpacpi_led_set(i, false);
> }
> 
> 
> Also, the exported symbol should be marked with *_GPL().
> 
> 
> thanks,
> 
> Takashi
> 



-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

Re: [RFC PATCH 2/2] ALSA: hda - add connection to thinkpad_acpi to control mute/micmute LEDs

[Takashi Iwai]

At Wed, 16 Oct 2013 14:15:36 +0200,
David Henningsson wrote:
> 
> Notes/questions:
> 
> For mute LEDs we can follow master like we already do on HP machines, but for
> mic mutes, we might have several capture switches. Honestly, on these laptops
> which almost always have autoswitched mics, there is only one recording source
> anyway. Can't we simply remove the extra capture PCMs, that seems easiest?

The fixup would be applied only to specific machines, so they are
without extra capture PCMs, no?

> For some reason CONFIG_THINKPAD_ACPI did not work here (it was not defined),
> but maybe it's due to the way I'm testing, or I'm missing something obvious. 
> Not sure.

It can be a module.  Use IS_ENABLED() macro.

#if IS_ENABLED(CONFIG_THINKPAD_ACPI)

> There are Thinkpad's with Realtek codecs as well which have mute/micmute.
> Maybe we should consider a more generic solution instead of copy-pasting
> between differen patch_* files?

Maybe.  But it's not that wide spread.  Let's make this one working at
first.  The further code sharing (involving module split eventually)
can be discussed later.

> And also, we're missing a symbol_put in this version. Should we add a new
> "free" fixup action where we can add a call to symbol_put, or do you have a
> better suggestion?

Hm, yes, a new free fixup action is a feasible option.


thanks,

Takashi

> 
> Signed-off-by: David Henningsson <david.henningsson@canonical.com>
> ---
>  sound/pci/hda/patch_conexant.c |   78 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 78 insertions(+)
> 
> diff --git a/sound/pci/hda/patch_conexant.c b/sound/pci/hda/patch_conexant.c
> index ec68eae..237df7f 100644
> --- a/sound/pci/hda/patch_conexant.c
> +++ b/sound/pci/hda/patch_conexant.c
> @@ -3232,8 +3232,80 @@ enum {
>  	CXT_FIXUP_HEADPHONE_MIC_PIN,
>  	CXT_FIXUP_HEADPHONE_MIC,
>  	CXT_FIXUP_GPIO1,
> +	CXT_FIXUP_THINKPAD_ACPI,
>  };
>  
> +/* FIXME: figure out why #ifdef CONFIG_THINKPAD_ACPI does not work */
> +
> +#if 1 /* #ifdef CONFIG_THINKPAD_ACPI */
> +
> +#include <linux/thinkpad_acpi.h>
> +
> +static int (*mute_led_set_func)(int);
> +static int (*micmute_led_set_func)(int);
> +
> +static void update_tpacpi_mute_led(void *private_data, int enabled)
> +{
> +	struct hda_codec *codec = private_data;
> +	struct conexant_spec *spec = codec->spec;
> +
> +	if (spec->dynamic_eapd)
> +		cx_auto_vmaster_hook(private_data, enabled);
> +
> +	if (mute_led_set_func)
> +		mute_led_set_func(!enabled);
> +}
> +
> +static void update_tpacpi_micmute_led(struct hda_codec *codec,
> +				      struct snd_ctl_elem_value *ucontrol)
> +{
> +	int val;
> +	if (!ucontrol || !micmute_led_set_func)
> +		return;
> +	/* TODO: if this is a stereo switch, better check both channels? What about additional capture switches with indices, on other ADCs? */
> +	val = ucontrol->value.integer.value[0];
> +	if (strcmp("Capture Switch", ucontrol->id.name) == 0 && ucontrol->id.index == 0)
> +		micmute_led_set_func(!val);
> +
> +}
> +
> +static void cxt_fixup_thinkpad_acpi(struct hda_codec *codec,
> +				  const struct hda_fixup *fix, int action)
> +{
> +	struct conexant_spec *spec = codec->spec;
> +	if (action == HDA_FIXUP_ACT_PRE_PROBE) {
> +		mute_led_set_func = symbol_request(tpacpi_mute_led_set);
> +		if (!mute_led_set_func)
> +			snd_printk(KERN_WARNING "Failed to find thinkpad-acpi symbol tpacpi_mute_led_set\n");
> +		else if (mute_led_set_func(0) < 0) {
> +			symbol_put(mute_led_set_func);
> +			mute_led_set_func = NULL;
> +		}
> +
> +		micmute_led_set_func = symbol_request(tpacpi_micmute_led_set);
> +		if (!micmute_led_set_func)
> +			snd_printk(KERN_WARNING "Failed to find thinkpad-acpi symbol tpacpi_micmute_led_set\n");
> +		else if (micmute_led_set_func(0) < 0) {
> +			symbol_put(micmute_led_set_func);
> +			micmute_led_set_func = NULL;
> +		}
> +	/* FIXME: when do we call symbol_put? In a new FREE fixup action? */
> +	}
> +	if (action == HDA_FIXUP_ACT_PROBE && micmute_led_set_func)
> +		spec->gen.cap_sync_hook = update_tpacpi_micmute_led;
> +	if (action == HDA_FIXUP_ACT_PROBE && mute_led_set_func)
> +		spec->gen.vmaster_mute.hook = update_tpacpi_mute_led;
> +}
> +
> +#else
> +
> +static void cxt_fixup_thinkpad_acpi(struct hda_codec *codec,
> +				  const struct hda_fixup *fix, int action)
> +{
> +}
> +
> +#endif
> +
>  static void cxt_fixup_stereo_dmic(struct hda_codec *codec,
>  				  const struct hda_fixup *fix, int action)
>  {
> @@ -3344,6 +3416,8 @@ static const struct hda_fixup cxt_fixups[] = {
>  	[CXT_PINCFG_LENOVO_TP410] = {
>  		.type = HDA_FIXUP_PINS,
>  		.v.pins = cxt_pincfg_lenovo_tp410,
> +		.chained = true,
> +		.chain_id = CXT_FIXUP_THINKPAD_ACPI,
>  	},
>  	[CXT_PINCFG_LEMOTE_A1004] = {
>  		.type = HDA_FIXUP_PINS,
> @@ -3385,6 +3459,10 @@ static const struct hda_fixup cxt_fixups[] = {
>  			{ }
>  		},
>  	},
> +	[CXT_FIXUP_THINKPAD_ACPI] = {
> +		.type = HDA_FIXUP_FUNC,
> +		.v.func = cxt_fixup_thinkpad_acpi,
> +	},
>  };
>  
>  static const struct snd_pci_quirk cxt5051_fixups[] = {
> -- 
> 1.7.9.5
> 

Re: [RFC PATCH 2/2] ALSA: hda - add connection to thinkpad_acpi to control mute/micmute LEDs

[David Henningsson]

On 10/16/2013 04:55 PM, Takashi Iwai wrote:
> At Wed, 16 Oct 2013 14:15:36 +0200,
> David Henningsson wrote:
>>
>> Notes/questions:
>>
>> For mute LEDs we can follow master like we already do on HP machines, but for
>> mic mutes, we might have several capture switches. Honestly, on these laptops
>> which almost always have autoswitched mics, there is only one recording source
>> anyway. Can't we simply remove the extra capture PCMs, that seems easiest?
> 
> The fixup would be applied only to specific machines, so they are
> without extra capture PCMs, no?

Actually, what I was thinking of is already implemented: when auto_mic
is turned on, num_adc_nids is set to one, so there are no extra ADCs. I
think we can get away (at least for now) with just an extra check that
there are no extra ADCs (added in v2 of the patch).

>> For some reason CONFIG_THINKPAD_ACPI did not work here (it was not defined),
>> but maybe it's due to the way I'm testing, or I'm missing something obvious. 
>> Not sure.
> 
> It can be a module.  Use IS_ENABLED() macro.
> 
> #if IS_ENABLED(CONFIG_THINKPAD_ACPI)

Thanks!

>> There are Thinkpad's with Realtek codecs as well which have mute/micmute.
>> Maybe we should consider a more generic solution instead of copy-pasting
>> between differen patch_* files?
> 
> Maybe.  But it's not that wide spread. 

Perhaps other people on this list have a better overview than I, but I
get the feeling that at least the mute LED is quite common on Thinkpads.
The micmute LED perhaps not so much.

> Let's make this one working at
> first.  The further code sharing (involving module split eventually)
> can be discussed later.

Ok.

>> And also, we're missing a symbol_put in this version. Should we add a new
>> "free" fixup action where we can add a call to symbol_put, or do you have a
>> better suggestion?
> 
> Hm, yes, a new free fixup action is a feasible option.

Ok, added in v2.

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic