On Tue, 14 Jan 2025 07:54:01 +0100,
Jackie Dong wrote:
>
> On 1/6/25 20:49, Jackie Dong wrote:
> > 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
>
> Hi Takashi,
> For this function, I added three debug message in patch_realtek.c as
> below. I find alc_fixup_no_shutup() only run once, no matter it's in
> alc_fixup_thinkpad_acpi(), or it's in alc_fixup_ideadpad_acpi(). Some
> kernel log for your reference.
> So, I think the patch is ok for this concern.
> If you have any other concern for the patch, let me know.
> Thanks for your comment and guide in past.
That's really weird. Are you testing your v2 patch, right?
(That is, the ALC269_FIXUP_LENOVO_XPAD_ACPI entry calls
alc_fixup_ideadpad_acpi() and is chained with
ALC269_FIXUP_THINKPAD_ACPI. If this entry is really used, it *must*
call the alc_fixup_thinkpad_acpi() as well.
Please double-check.
Takashi
