Re: [PATCH] ALSA: hda - Enable mute/mic-mute LEDs for more Thinkpads with Conexant codec

[Hui Wang <hui.wang@canonical.com>]

On 11/27/2013 02:44 PM, Takashi Iwai wrote:
> At Wed, 27 Nov 2013 06:17:16 +0100,
> David Henningsson wrote:
>> On 11/27/2013 04:26 AM, Hui Wang wrote:
>>> Most Thinkpad Edge series laptops use conexant codec, so far although
>>> the codecs have different minor Vendor Id and minor Subsystem Id, but
>>> they all belong to the cxt_5066 family, this change can make the
>>> mute/mic-mute LEDs support more generic among cxt_5066 family.
>> Hi Hui and thanks for your patch!
>>
>> It's also nice that you have tested the patch on several machines and
>> found it to be working.
>>
>> As for the actual implementation - as the rest of the thinkpad-acpi code
>> is copy-pasted between patch_realtek and patch_conexant, I think this
>> would also apply here, i e, no code in hda_local.h, instead copy from
>> patch_realtek to patch_conexant as necessary.
>>
>> While copy-pasting code is not the most elegant of solutions, I got the
>> impression from Takashi that he preferred that for now.
> If you'd like it to be included in 3.13, please just copy the small
> code snippet.
Yes we hope it can be merged to 3.13, so as your request, i just copy 
the small code snippet in the V2 patch.
>
> For 3.14, we can move the thinkpad-related code into another file, but
> I have some bad feeling to put it into hda_local.h or whatever the
> common code base.
> Maybe just create hda_thinkpad.c or such and include from both
> patch_realtek.c and patch_conexant.c.  As long as the code size is
> small enough and they are all static, it should be fine.
Good suggestion, if we have more machines to test and the testing expose 
more problems,
we should think about moving the common functions to the hda_thinkpad.c.

Regards,
Hui.
>
>
> thanks,
>
> Takashi
>
>>> Cc: Alex Hung <alex.hung@canonical.com>
>>> Cc: David Henningsson <david.henningsson@canonical.com>
>>> Signed-off-by: Hui Wang <hui.wang@canonical.com>
>>> ---
>>>   sound/pci/hda/hda_local.h      | 22 ++++++++++++++++++++++
>>>   sound/pci/hda/patch_conexant.c |  3 +++
>>>   sound/pci/hda/patch_realtek.c  | 20 --------------------
>>>   3 files changed, 25 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h
>>> index d398b64..1155958 100644
>>> --- a/sound/pci/hda/hda_local.h
>>> +++ b/sound/pci/hda/hda_local.h
>>> @@ -777,4 +777,26 @@ void snd_hdmi_write_eld_info(struct hdmi_eld *eld,
>>>   #define SND_PRINT_CHANNEL_ALLOCATION_ADVISED_BUFSIZE 80
>>>   void snd_print_channel_allocation(int spk_alloc, char *buf, int buflen);
>>>   
>>> +#if IS_ENABLED(CONFIG_THINKPAD_ACPI)
>>> +#include <acpi/acpi.h>
>>> +
>>> +static acpi_status acpi_check_cb(acpi_handle handle, u32 lvl, void *context,
>>> +				 void **rv)
>>> +{
>>> +	bool *found = context;
>>> +	*found = true;
>>> +	return AE_OK;
>>> +}
>>> +
>>> +static inline bool is_thinkpad(struct hda_codec *codec)
>>> +{
>>> +	bool found = false;
>>> +	if (codec->subsystem_id >> 16 != 0x17aa)
>>> +		return false;
>>> +	if (ACPI_SUCCESS(acpi_get_devices("LEN0068", acpi_check_cb, &found, NULL)) && found)
>>> +		return true;
>>> +	found = false;
>>> +	return ACPI_SUCCESS(acpi_get_devices("IBM0068", acpi_check_cb, &found, NULL)) && found;
>>> +}
>>> +#endif
>>>   #endif /* __SOUND_HDA_LOCAL_H */
>>> diff --git a/sound/pci/hda/patch_conexant.c b/sound/pci/hda/patch_conexant.c
>>> index c205bb1..8063ae9 100644
>>> --- a/sound/pci/hda/patch_conexant.c
>>> +++ b/sound/pci/hda/patch_conexant.c
>>> @@ -3279,6 +3279,8 @@ static void cxt_fixup_thinkpad_acpi(struct hda_codec *codec,
>>>   	bool removefunc = false;
>>>   
>>>   	if (action == HDA_FIXUP_ACT_PROBE) {
>>> +		if (!is_thinkpad(codec))
>>> +			return;
>>>   		if (!led_set_func)
>>>   			led_set_func = symbol_request(tpacpi_led_set);
>>>   		if (!led_set_func) {
>>> @@ -3494,6 +3496,7 @@ static const struct snd_pci_quirk cxt5066_fixups[] = {
>>>   	SND_PCI_QUIRK(0x17aa, 0x3975, "Lenovo U300s", CXT_FIXUP_STEREO_DMIC),
>>>   	SND_PCI_QUIRK(0x17aa, 0x3977, "Lenovo IdeaPad U310", CXT_FIXUP_STEREO_DMIC),
>>>   	SND_PCI_QUIRK(0x17aa, 0x397b, "Lenovo S205", CXT_FIXUP_STEREO_DMIC),
>>> +	SND_PCI_QUIRK_VENDOR(0x17aa, "Thinkpad", CXT_FIXUP_THINKPAD_ACPI),
>>>   	SND_PCI_QUIRK(0x1c06, 0x2011, "Lemote A1004", CXT_PINCFG_LEMOTE_A1004),
>>>   	SND_PCI_QUIRK(0x1c06, 0x2012, "Lemote A1205", CXT_PINCFG_LEMOTE_A1205),
>>>   	{}
>>> diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
>>> index 5e42059..bedf35f 100644
>>> --- a/sound/pci/hda/patch_realtek.c
>>> +++ b/sound/pci/hda/patch_realtek.c
>>> @@ -3673,29 +3673,9 @@ static void alc290_fixup_mono_speakers(struct hda_codec *codec,
>>>   #if IS_ENABLED(CONFIG_THINKPAD_ACPI)
>>>   
>>>   #include <linux/thinkpad_acpi.h>
>>> -#include <acpi/acpi.h>
>>>   
>>>   static int (*led_set_func)(int, bool);
>>>   
>>> -static acpi_status acpi_check_cb(acpi_handle handle, u32 lvl, void *context,
>>> -				 void **rv)
>>> -{
>>> -	bool *found = context;
>>> -	*found = true;
>>> -	return AE_OK;
>>> -}
>>> -
>>> -static bool is_thinkpad(struct hda_codec *codec)
>>> -{
>>> -	bool found = false;
>>> -	if (codec->subsystem_id >> 16 != 0x17aa)
>>> -		return false;
>>> -	if (ACPI_SUCCESS(acpi_get_devices("LEN0068", acpi_check_cb, &found, NULL)) && found)
>>> -		return true;
>>> -	found = false;
>>> -	return ACPI_SUCCESS(acpi_get_devices("IBM0068", acpi_check_cb, &found, NULL)) && found;
>>> -}
>>> -
>>>   static void update_tpacpi_mute_led(void *private_data, int enabled)
>>>   {
>>>   	if (led_set_func)
>>>
>>
>>
>> -- 
>> David Henningsson, Canonical Ltd.
>> https://launchpad.net/~diwic
>>
>