[alsa-devel] [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 at 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
>