Re: [External] Re: [PATCH v2] ALSA: hda: Support for Ideapad hotkeymute LEDs

[Takashi Iwai <tiwai@suse.de>]

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