* [PATCH] ALSA: hda - Enable mute/mic-mute LEDs for more Thinkpads with Conexant codec @ 2013-11-27 3:26 Hui Wang 2013-11-27 5:17 ` David Henningsson 0 siblings, 1 reply; 8+ messages in thread From: Hui Wang @ 2013-11-27 3:26 UTC (permalink / raw) To: tiwai, alsa-devel; +Cc: hui.wang, alex.hung, yk, david.henningsson 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. 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) -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] ALSA: hda - Enable mute/mic-mute LEDs for more Thinkpads with Conexant codec 2013-11-27 3:26 [PATCH] ALSA: hda - Enable mute/mic-mute LEDs for more Thinkpads with Conexant codec Hui Wang @ 2013-11-27 5:17 ` David Henningsson 2013-11-27 6:21 ` Hui Wang 2013-11-27 6:44 ` Takashi Iwai 0 siblings, 2 replies; 8+ messages in thread From: David Henningsson @ 2013-11-27 5:17 UTC (permalink / raw) To: Hui Wang, tiwai, alsa-devel; +Cc: hui.wang, alex.hung, yk 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. > > 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ALSA: hda - Enable mute/mic-mute LEDs for more Thinkpads with Conexant codec 2013-11-27 5:17 ` David Henningsson @ 2013-11-27 6:21 ` Hui Wang 2013-11-27 6:44 ` Takashi Iwai 1 sibling, 0 replies; 8+ messages in thread From: Hui Wang @ 2013-11-27 6:21 UTC (permalink / raw) To: David Henningsson; +Cc: tiwai, hui.wang, alsa-devel, yk, alex.hung On 11/27/2013 01:17 PM, 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. OK, got it, I will send a V2 patch according to your suggestion. Regards, Hui. >> 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(-) >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ALSA: hda - Enable mute/mic-mute LEDs for more Thinkpads with Conexant codec 2013-11-27 5:17 ` David Henningsson 2013-11-27 6:21 ` Hui Wang @ 2013-11-27 6:44 ` Takashi Iwai 2013-11-27 7:11 ` Hui Wang 1 sibling, 1 reply; 8+ messages in thread From: Takashi Iwai @ 2013-11-27 6:44 UTC (permalink / raw) To: David Henningsson; +Cc: hui.wang, alsa-devel, Hui Wang, alex.hung, yk 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. 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. 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ALSA: hda - Enable mute/mic-mute LEDs for more Thinkpads with Conexant codec 2013-11-27 6:44 ` Takashi Iwai @ 2013-11-27 7:11 ` Hui Wang 2013-11-27 7:23 ` Takashi Iwai 0 siblings, 1 reply; 8+ messages in thread From: Hui Wang @ 2013-11-27 7:11 UTC (permalink / raw) To: Takashi Iwai; +Cc: hui.wang, alsa-devel, yk, alex.hung, David Henningsson 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 >> > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ALSA: hda - Enable mute/mic-mute LEDs for more Thinkpads with Conexant codec 2013-11-27 7:11 ` Hui Wang @ 2013-11-27 7:23 ` Takashi Iwai 2013-11-27 7:39 ` Hui Wang 0 siblings, 1 reply; 8+ messages in thread From: Takashi Iwai @ 2013-11-27 7:23 UTC (permalink / raw) To: Hui Wang; +Cc: alsa-devel, yk, alex.hung, David Henningsson At Wed, 27 Nov 2013 15:11:41 +0800, Hui Wang wrote: > > 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. Hrm, this comment makes me worry a bit to include into 3.13. Hasn't been the patch tested fully? The merge window was already closed, and it means that we really need only fixes, the ones that won't regress obviously... Takashi > > 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 > >> > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ALSA: hda - Enable mute/mic-mute LEDs for more Thinkpads with Conexant codec 2013-11-27 7:23 ` Takashi Iwai @ 2013-11-27 7:39 ` Hui Wang 2013-11-27 7:55 ` Takashi Iwai 0 siblings, 1 reply; 8+ messages in thread From: Hui Wang @ 2013-11-27 7:39 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel, yk, alex.hung, David Henningsson On 11/27/2013 03:23 PM, Takashi Iwai wrote: > At Wed, 27 Nov 2013 15:11:41 +0800, > Hui Wang wrote: >> 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. > Hrm, this comment makes me worry a bit to include into 3.13. > Hasn't been the patch tested fully? The merge window was already > closed, and it means that we really need only fixes, the ones that > won't regress obviously... Sorry to make you misunderstand, this patch definitely works well on the machines with Conexant codec (cxt5066 family). I have tested this patch on 9 laptops whose codecs all belong to cxt5066 family, they all worked well. The problem i pointed to was that, assuming we meet a machine with conexant codec in future, but this codec doesn't belong to cxt5066 family, this patch will not work on this machine, but this patch will not introduce regress on this machine as well. Regards, Hui. > > Takashi > > >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ALSA: hda - Enable mute/mic-mute LEDs for more Thinkpads with Conexant codec 2013-11-27 7:39 ` Hui Wang @ 2013-11-27 7:55 ` Takashi Iwai 0 siblings, 0 replies; 8+ messages in thread From: Takashi Iwai @ 2013-11-27 7:55 UTC (permalink / raw) To: Hui Wang; +Cc: alsa-devel, yk, alex.hung, David Henningsson At Wed, 27 Nov 2013 15:39:19 +0800, Hui Wang wrote: > > On 11/27/2013 03:23 PM, Takashi Iwai wrote: > > At Wed, 27 Nov 2013 15:11:41 +0800, > > Hui Wang wrote: > >> 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. > > Hrm, this comment makes me worry a bit to include into 3.13. > > Hasn't been the patch tested fully? The merge window was already > > closed, and it means that we really need only fixes, the ones that > > won't regress obviously... > Sorry to make you misunderstand, this patch definitely works well on the > machines with Conexant codec (cxt5066 family). I have tested this patch > on 9 laptops whose codecs all belong to cxt5066 family, they all worked > well. > > The problem i pointed to was that, assuming we meet a machine with > conexant codec in future, but this codec doesn't belong to cxt5066 > family, this patch will not work on this machine, but this patch will > not introduce regress on this machine as well. OK, now applied. Thanks. Takashi ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-11-27 7:55 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-11-27 3:26 [PATCH] ALSA: hda - Enable mute/mic-mute LEDs for more Thinkpads with Conexant codec Hui Wang 2013-11-27 5:17 ` David Henningsson 2013-11-27 6:21 ` Hui Wang 2013-11-27 6:44 ` Takashi Iwai 2013-11-27 7:11 ` Hui Wang 2013-11-27 7:23 ` Takashi Iwai 2013-11-27 7:39 ` Hui Wang 2013-11-27 7:55 ` Takashi Iwai
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.