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

[Takashi Iwai <tiwai@suse.de>]

On Tue, 14 Jan 2025 08:19:17 +0100,
Takashi Iwai wrote:
> 
> 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.

On the second thought, alc_fixup_no_shutup() itself is mostly harmless
even if it's called multiple times, as it sets only the flag.
But, unifying the quirk function makes more sense as it results in
smaller changes.

In anyway, the check of the alc_fixup_no_shutup() should be done
again; if a test is negative, it doesn't mean it's OK but it's
something wrong.


thanks,

Takashi