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
