[alsa-devel] [RFC PATCH 0/2] Enable mute LEDs and mic mute LEDs on Thinkpad
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
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
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.
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
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
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[] = {
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,@@ -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
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.
participants (3)
-
David Henningsson -
Henrique de Moraes Holschuh -
Takashi Iwai