| Message ID | 20241224083316.20222-1-xy-jackie@139.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] ALSA: hda: Support for Ideapad hotkey mute LEDs | expand |
On Tue, 24 Dec 2024 09:33:16 +0100, Jackie Dong wrote: > > --- a/sound/pci/hda/patch_realtek.c > +++ b/sound/pci/hda/patch_realtek.c > @@ -6934,6 +6934,16 @@ static void alc_fixup_thinkpad_acpi(struct hda_codec *codec, > hda_fixup_thinkpad_acpi(codec, fix, action); > } > > +/* for hda_fixup_ideapad_acpi() */ > +#include "ideapad_hotkey_led_helper.c" > + > +static void alc_fixup_ideapad_acpi(struct hda_codec *codec, > + const struct hda_fixup *fix, int action) > +{ > + alc_fixup_no_shutup(codec, fix, action); /* reduce click noise */ > + hda_fixup_ideapad_acpi(codec, fix, action); > +} So this unconditionally call alc_fixup_no_shutup(), and this introduces another behavior to the existing entry -- i.e. there is a chance of breakage. If we want to be very conservative, this call should be limited to Ideapad. thanks, Takashi
> On Tue, 24 Dec 2024 09:33:16 +0100, > Jackie Dong wrote: >> >> --- a/sound/pci/hda/patch_realtek.c >> +++ b/sound/pci/hda/patch_realtek.c >> @@ -6934,6 +6934,16 @@ static void alc_fixup_thinkpad_acpi(struct hda_codec *codec, >> hda_fixup_thinkpad_acpi(codec, fix, action); >> } >> >> +/* for hda_fixup_ideapad_acpi() */ >> +#include "ideapad_hotkey_led_helper.c" >> + >> +static void alc_fixup_ideapad_acpi(struct hda_codec *codec, >> + const struct hda_fixup *fix, int action) >> +{ >> + alc_fixup_no_shutup(codec, fix, action); /* reduce click noise */ >> + hda_fixup_ideapad_acpi(codec, fix, action); >> +} > > So this unconditionally call alc_fixup_no_shutup(), and this > introduces another behavior to the existing entry -- i.e. there is a > chance of breakage. > > If we want to be very conservative, this call should be limited to > Ideapad. > For alc_fixup_no_shutup(codec, fix, action), I want to keep same behavior with alc_fixup_thinkpad_apci() and alc_fixup_idea_acpi() for one sound card. So, I add alc_fixup_no_shutup() in alc_fixup_ideapad_acpi(). ----------Related source code of patch_reatek.c are FYR as below. static void alc_fixup_thinkpad_acpi(struct hda_codec *codec, const struct hda_fixup *fix, int action) { alc_fixup_no_shutup(codec, fix, action); /* reduce click noise */ hda_fixup_thinkpad_acpi(codec, fix, action); } /* for hda_fixup_ideapad_acpi() */ #include "ideapad_hotkey_led_helper.c" static void alc_fixup_ideapad_acpi(struct hda_codec *codec, const struct hda_fixup *fix, int action) { alc_fixup_no_shutup(codec, fix, action); /* reduce click noise */ hda_fixup_ideapad_acpi(codec, fix, action); } Thanks, Jackie > thanks, > > Takashi >
On Mon, 30 Dec 2024 01:33:01 +0100, Jackie EG1 Dong wrote: > > > On Tue, 24 Dec 2024 09:33:16 +0100, > > Jackie Dong wrote: > >> > >> --- a/sound/pci/hda/patch_realtek.c > >> +++ b/sound/pci/hda/patch_realtek.c > >> @@ -6934,6 +6934,16 @@ static void alc_fixup_thinkpad_acpi(struct hda_codec *codec, > >> hda_fixup_thinkpad_acpi(codec, fix, action); > >> } > >> > >> +/* for hda_fixup_ideapad_acpi() */ > >> +#include "ideapad_hotkey_led_helper.c" > >> + > >> +static void alc_fixup_ideapad_acpi(struct hda_codec *codec, > >> + const struct hda_fixup *fix, int action) > >> +{ > >> + alc_fixup_no_shutup(codec, fix, action); /* reduce click noise */ > >> + hda_fixup_ideapad_acpi(codec, fix, action); > >> +} > > > > So this unconditionally call alc_fixup_no_shutup(), and this > introduces another behavior to the existing entry -- i.e. there is a > chance of breakage. > > > > If we want to be very conservative, this call should be limited to > Ideapad. > > For alc_fixup_no_shutup(codec, fix, action), > I want to keep same behavior with alc_fixup_thinkpad_apci() and alc_fixup_idea_acpi() for one sound card. So, I add alc_fixup_no_shutup() in alc_fixup_ideapad_acpi(). > ----------Related source code of patch_reatek.c are FYR as below. > static void alc_fixup_thinkpad_acpi(struct hda_codec *codec, > const struct hda_fixup *fix, int > action) > { > alc_fixup_no_shutup(codec, fix, action); /* reduce click noise */ > hda_fixup_thinkpad_acpi(codec, fix, action); } > > /* for hda_fixup_ideapad_acpi() */ > #include "ideapad_hotkey_led_helper.c" > > static void alc_fixup_ideapad_acpi(struct hda_codec *codec, > const struct hda_fixup *fix, int action) { > alc_fixup_no_shutup(codec, fix, action); /* reduce click noise */ > hda_fixup_ideapad_acpi(codec, fix, action); > } Oh yeah, but then it can be bad in other way round; the chain call of alc_fixup_thinkpad_acpi() contains alc_fixup_no_shutup() and the alc_fixup_ideadpad_acpi() also contains alc_fixup_no_shutup(). That is, alc_fixup_no_shutup() will be called twice for Thinkpad. Instead, you can change like: --- a/sound/pci/hda/patch_realtek.c +++ b/sound/pci/hda/patch_realtek.c @@ -6925,11 +6925,16 @@ static void alc285_fixup_hp_envy_x360(struct hda_codec *codec, /* for hda_fixup_thinkpad_acpi() */ #include "thinkpad_helper.c" -static void alc_fixup_thinkpad_acpi(struct hda_codec *codec, - const struct hda_fixup *fix, int action) +/* for hda_fixup_ideapad_acpi() */ +#include "ideapad_hotkey_led_helper.c" + +/* generic fixup for both Lenovo Thinkpad and Ideapad */ +static void alc_fixup_xpad_acpi(struct hda_codec *codec, + const struct hda_fixup *fix, int action) { alc_fixup_no_shutup(codec, fix, action); /* reduce click noise */ hda_fixup_thinkpad_acpi(codec, fix, action); + hda_fixup_ideapad_acpi(codec, fix, action); } /* Fixup for Lenovo Legion 15IMHg05 speaker output on headset removal. */ @@ -8321,7 +8326,7 @@ static const struct hda_fixup alc269_fixups[] = { }, [ALC269_FIXUP_THINKPAD_ACPI] = { .type = HDA_FIXUP_FUNC, - .v.func = alc_fixup_thinkpad_acpi, + .v.func = alc_fixup_xpad_acpi, .chained = true, .chain_id = ALC269_FIXUP_SKU_IGNORE, }, Since hda_fixup_ideapad_acpi() is NOP except for Ideapad, this shouldn't break other models, while it covers the Ideadpad now. thanks, Takashi
On 2025/1/3 23:17, Takashi Iwai wrote: > On Mon, 30 Dec 2024 01:33:01 +0100, > Jackie EG1 Dong wrote: >> >>> On Tue, 24 Dec 2024 09:33:16 +0100, >> > Jackie Dong wrote: >> >> >> >> --- a/sound/pci/hda/patch_realtek.c >> >> +++ b/sound/pci/hda/patch_realtek.c >> >> @@ -6934,6 +6934,16 @@ static void alc_fixup_thinkpad_acpi(struct hda_codec *codec, >> >> hda_fixup_thinkpad_acpi(codec, fix, action); >> >> } >> >> >> >> +/* for hda_fixup_ideapad_acpi() */ >> >> +#include "ideapad_hotkey_led_helper.c" >> >> + >> >> +static void alc_fixup_ideapad_acpi(struct hda_codec *codec, >> >> + const struct hda_fixup *fix, int action) >> >> +{ >> >> + alc_fixup_no_shutup(codec, fix, action); /* reduce click noise */ >> >> + hda_fixup_ideapad_acpi(codec, fix, action); >> >> +} >> > >> > So this unconditionally call alc_fixup_no_shutup(), and this > introduces another behavior to the existing entry -- i.e. there is a > chance of breakage. >> > >> > If we want to be very conservative, this call should be limited to > Ideapad. >> > For alc_fixup_no_shutup(codec, fix, action), >> I want to keep same behavior with alc_fixup_thinkpad_apci() and alc_fixup_idea_acpi() for one sound card. So, I add alc_fixup_no_shutup() in alc_fixup_ideapad_acpi(). >> ----------Related source code of patch_reatek.c are FYR as below. >> static void alc_fixup_thinkpad_acpi(struct hda_codec *codec, >> const struct hda_fixup *fix, int >> action) >> { >> alc_fixup_no_shutup(codec, fix, action); /* reduce click noise */ >> hda_fixup_thinkpad_acpi(codec, fix, action); } >> >> /* for hda_fixup_ideapad_acpi() */ >> #include "ideapad_hotkey_led_helper.c" >> >> static void alc_fixup_ideapad_acpi(struct hda_codec *codec, >> const struct hda_fixup *fix, int action) { >> alc_fixup_no_shutup(codec, fix, action); /* reduce click noise */ >> hda_fixup_ideapad_acpi(codec, fix, action); >> } > > Oh yeah, but then it can be bad in other way round; the chain call of > alc_fixup_thinkpad_acpi() contains alc_fixup_no_shutup() and the > alc_fixup_ideadpad_acpi() also contains alc_fixup_no_shutup(). > That is, alc_fixup_no_shutup() will be called twice for Thinkpad. > Many thanks to Takashi for your detail comments and sample code, I understand it now. I'll check the logic of the code and update the patch later. Best Regards, Jackie Dong > Instead, you can change like: > > --- a/sound/pci/hda/patch_realtek.c > +++ b/sound/pci/hda/patch_realtek.c > @@ -6925,11 +6925,16 @@ static void alc285_fixup_hp_envy_x360(struct hda_codec *codec, > /* for hda_fixup_thinkpad_acpi() */ > #include "thinkpad_helper.c" > > -static void alc_fixup_thinkpad_acpi(struct hda_codec *codec, > - const struct hda_fixup *fix, int action) > +/* for hda_fixup_ideapad_acpi() */ > +#include "ideapad_hotkey_led_helper.c" > + > +/* generic fixup for both Lenovo Thinkpad and Ideapad */ > +static void alc_fixup_xpad_acpi(struct hda_codec *codec, > + const struct hda_fixup *fix, int action) > { > alc_fixup_no_shutup(codec, fix, action); /* reduce click noise */ > hda_fixup_thinkpad_acpi(codec, fix, action); > + hda_fixup_ideapad_acpi(codec, fix, action); > } > > /* Fixup for Lenovo Legion 15IMHg05 speaker output on headset removal. */ > @@ -8321,7 +8326,7 @@ static const struct hda_fixup alc269_fixups[] = { > }, > [ALC269_FIXUP_THINKPAD_ACPI] = { > .type = HDA_FIXUP_FUNC, > - .v.func = alc_fixup_thinkpad_acpi, > + .v.func = alc_fixup_xpad_acpi, > .chained = true, > .chain_id = ALC269_FIXUP_SKU_IGNORE, > }, > > Since hda_fixup_ideapad_acpi() is NOP except for Ideapad, this > shouldn't break other models, while it covers the Ideadpad now. > > > thanks, > > Takashi
diff --git a/sound/pci/hda/ideapad_hotkey_led_helper.c b/sound/pci/hda/ideapad_hotkey_led_helper.c new file mode 100644 index 000000000000..c10d97964d49 --- /dev/null +++ b/sound/pci/hda/ideapad_hotkey_led_helper.c @@ -0,0 +1,36 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Ideapad helper functions for Lenovo Ideapad LED control, + * It should be included from codec driver. + */ + +#if IS_ENABLED(CONFIG_IDEAPAD_LAPTOP) + +#include <linux/acpi.h> +#include <linux/leds.h> + +static bool is_ideapad(struct hda_codec *codec) +{ + return (codec->core.subsystem_id >> 16 == 0x17aa) && + (acpi_dev_found("LHK2019") || acpi_dev_found("VPC2004")); +} + +static void hda_fixup_ideapad_acpi(struct hda_codec *codec, + const struct hda_fixup *fix, int action) +{ + if (action == HDA_FIXUP_ACT_PRE_PROBE) { + if (!is_ideapad(codec)) + return; + snd_hda_gen_add_mute_led_cdev(codec, NULL); + snd_hda_gen_add_micmute_led_cdev(codec, NULL); + } +} + +#else /* CONFIG_IDEAPAD_LAPTOP */ + +static void hda_fixup_ideapad_acpi(struct hda_codec *codec, + const struct hda_fixup *fix, int action) +{ +} + +#endif /* CONFIG_IDEAPAD_LAPTOP */ diff --git a/sound/pci/hda/patch_conexant.c b/sound/pci/hda/patch_conexant.c index 538c37a78a56..4985e72b9094 100644 --- a/sound/pci/hda/patch_conexant.c +++ b/sound/pci/hda/patch_conexant.c @@ -291,6 +291,7 @@ enum { CXT_FIXUP_GPIO1, CXT_FIXUP_ASPIRE_DMIC, CXT_FIXUP_THINKPAD_ACPI, + CXT_FIXUP_LENOVO_XPAD_ACPI, CXT_FIXUP_OLPC_XO, CXT_FIXUP_CAP_MIX_AMP, CXT_FIXUP_TOSHIBA_P105, @@ -313,6 +314,9 @@ enum { /* for hda_fixup_thinkpad_acpi() */ #include "thinkpad_helper.c" +/* for hda_fixup_ideapad_acpi() */ +#include "ideapad_hotkey_led_helper.c" + static void cxt_fixup_stereo_dmic(struct hda_codec *codec, const struct hda_fixup *fix, int action) { @@ -928,6 +932,12 @@ static const struct hda_fixup cxt_fixups[] = { .type = HDA_FIXUP_FUNC, .v.func = hda_fixup_thinkpad_acpi, }, + [CXT_FIXUP_LENOVO_XPAD_ACPI] = { + .type = HDA_FIXUP_FUNC, + .v.func = hda_fixup_ideapad_acpi, + .chained = true, + .chain_id = CXT_FIXUP_THINKPAD_ACPI, + }, [CXT_FIXUP_OLPC_XO] = { .type = HDA_FIXUP_FUNC, .v.func = cxt_fixup_olpc_xo, @@ -1119,7 +1129,7 @@ static const struct hda_quirk cxt5066_fixups[] = { SND_PCI_QUIRK(0x17aa, 0x3977, "Lenovo IdeaPad U310", CXT_FIXUP_STEREO_DMIC), SND_PCI_QUIRK(0x17aa, 0x3978, "Lenovo G50-70", 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_VENDOR(0x17aa, "Thinkpad/Ideapad", CXT_FIXUP_LENOVO_XPAD_ACPI), SND_PCI_QUIRK(0x1c06, 0x2011, "Lemote A1004", CXT_PINCFG_LEMOTE_A1004), SND_PCI_QUIRK(0x1c06, 0x2012, "Lemote A1205", CXT_PINCFG_LEMOTE_A1205), HDA_CODEC_QUIRK(0x2782, 0x12c3, "Sirius Gen1", CXT_PINCFG_TOP_SPEAKER), @@ -1133,6 +1143,7 @@ static const struct hda_model_fixup cxt5066_fixup_models[] = { { .id = CXT_FIXUP_HEADPHONE_MIC_PIN, .name = "headphone-mic-pin" }, { .id = CXT_PINCFG_LENOVO_TP410, .name = "tp410" }, { .id = CXT_FIXUP_THINKPAD_ACPI, .name = "thinkpad" }, + { .id = CXT_FIXUP_LENOVO_XPAD_ACPI, .name = "thinkpad-ideapad" }, { .id = CXT_PINCFG_LEMOTE_A1004, .name = "lemote-a1004" }, { .id = CXT_PINCFG_LEMOTE_A1205, .name = "lemote-a1205" }, { .id = CXT_FIXUP_OLPC_XO, .name = "olpc-xo" }, diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c index 61ba5dc35b8b..5f9927401322 100644 --- a/sound/pci/hda/patch_realtek.c +++ b/sound/pci/hda/patch_realtek.c @@ -6934,6 +6934,16 @@ static void alc_fixup_thinkpad_acpi(struct hda_codec *codec, hda_fixup_thinkpad_acpi(codec, fix, action); } +/* for hda_fixup_ideapad_acpi() */ +#include "ideapad_hotkey_led_helper.c" + +static void alc_fixup_ideapad_acpi(struct hda_codec *codec, + const struct hda_fixup *fix, int action) +{ + alc_fixup_no_shutup(codec, fix, action); /* reduce click noise */ + hda_fixup_ideapad_acpi(codec, fix, action); +} + /* Fixup for Lenovo Legion 15IMHg05 speaker output on headset removal. */ static void alc287_fixup_legion_15imhg05_speakers(struct hda_codec *codec, const struct hda_fixup *fix, @@ -7556,6 +7566,7 @@ enum { ALC290_FIXUP_SUBWOOFER, ALC290_FIXUP_SUBWOOFER_HSJACK, ALC269_FIXUP_THINKPAD_ACPI, + ALC269_FIXUP_LENOVO_XPAD_ACPI, ALC269_FIXUP_DMIC_THINKPAD_ACPI, ALC269VB_FIXUP_INFINIX_ZERO_BOOK_13, ALC269VC_FIXUP_INFINIX_Y4_MAX, @@ -8327,6 +8338,12 @@ static const struct hda_fixup alc269_fixups[] = { .chained = true, .chain_id = ALC269_FIXUP_SKU_IGNORE, }, + [ALC269_FIXUP_LENOVO_XPAD_ACPI] = { + .type = HDA_FIXUP_FUNC, + .v.func = alc_fixup_ideapad_acpi, + .chained = true, + .chain_id = ALC269_FIXUP_THINKPAD_ACPI, + }, [ALC269_FIXUP_DMIC_THINKPAD_ACPI] = { .type = HDA_FIXUP_FUNC, .v.func = alc_fixup_inv_dmic, @@ -11065,7 +11082,7 @@ static const struct hda_quirk alc269_fixup_vendor_tbl[] = { SND_PCI_QUIRK_VENDOR(0x1025, "Acer Aspire", ALC271_FIXUP_DMIC), SND_PCI_QUIRK_VENDOR(0x103c, "HP", ALC269_FIXUP_HP_MUTE_LED), SND_PCI_QUIRK_VENDOR(0x104d, "Sony VAIO", ALC269_FIXUP_SONY_VAIO), - SND_PCI_QUIRK_VENDOR(0x17aa, "Thinkpad", ALC269_FIXUP_THINKPAD_ACPI), + SND_PCI_QUIRK_VENDOR(0x17aa, "Lenovo XPAD", ALC269_FIXUP_LENOVO_XPAD_ACPI), SND_PCI_QUIRK_VENDOR(0x19e5, "Huawei Matebook", ALC255_FIXUP_MIC_MUTE_LED), {} }; @@ -11130,6 +11147,7 @@ static const struct hda_model_fixup alc269_fixup_models[] = { {.id = ALC290_FIXUP_MONO_SPEAKERS_HSJACK, .name = "mono-speakers"}, {.id = ALC290_FIXUP_SUBWOOFER_HSJACK, .name = "alc290-subwoofer"}, {.id = ALC269_FIXUP_THINKPAD_ACPI, .name = "thinkpad"}, + {.id = ALC269_FIXUP_LENOVO_XPAD_ACPI, .name = "lenovo xpad led"}, {.id = ALC269_FIXUP_DMIC_THINKPAD_ACPI, .name = "dmic-thinkpad"}, {.id = ALC255_FIXUP_ACER_MIC_NO_PRESENCE, .name = "alc255-acer"}, {.id = ALC255_FIXUP_ASUS_MIC_NO_PRESENCE, .name = "alc255-asus"},
New ideapad helper file with support for handling FN key mute LEDs. Update conexant and realtec codec to add LED support. Suggested-by: Mark Pearson <mpearson-lenovo@squebb.ca> Signed-off-by: Jackie Dong <xy-jackie@139.com> --- Changes in v2: - Add a new enum CXT_FIXUP_LENOVO_XPAD_ACPI and define it as an entry in patch_conexant.c - Add a new enum ALC269_FIXUP_LENOVO_XPAD_ACPI and define it as an entry in patch_realtek.c sound/pci/hda/ideapad_hotkey_led_helper.c | 36 +++++++++++++++++++++++ sound/pci/hda/patch_conexant.c | 13 +++++++- sound/pci/hda/patch_realtek.c | 20 ++++++++++++- 3 files changed, 67 insertions(+), 2 deletions(-) create mode 100644 sound/pci/hda/ideapad_hotkey_led_helper.c