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

[Jackie Dong <xy-jackie@139.com>]

On 1/14/25 23:47, Jackie Dong wrote:
> On 1/14/25 16:28, Takashi Iwai wrote:
>> 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.
> Hi Takashi,
>    You're right.
>    I commented two lines in [ALC269_FIXUP_LENOVO_XPAD_ACPI] and got the 
> result of previous mail. I try to look for which funcion call 
> alc_fixup_thinkpad_acpi() after add below patch. And I hope to impeleted 
> the function with minimum changes.
>    Many thanks,
> -----------------------------------------------------------------------
> --- a/sound/pci/hda/patch_realtek.c
> +++ b/sound/pci/hda/patch_realtek.c
> @@ -6126,6 +6126,7 @@ static void alc_fixup_no_shutup(struct hda_codec 
> *codec,
>                  struct alc_spec *spec = codec->spec;
>                  spec->no_shutup_pins = 1;
>          }
> +       printk("This is from alc_fixup_no_shutup+ 
> +444444444444444444444444444444+-.\n");//Deg
>   }
> 
>   static void alc_fixup_disable_aamix(struct hda_codec *codec,
> @@ -6930,10 +6931,22 @@ static void alc285_fixup_hp_envy_x360(struct 
> hda_codec *codec,
>   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 */
> +       alc_fixup_no_shutup(codec, fix, action); /* reduce click noise 
> */ //Deg
> +       printk("This is from alc_fixup_no_shutup+ 
> +TTTTTTTTTTTTTTTTTTTTTTTTTTTTTTT+-.\n");//Deg
>          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 
> */ //Deg
> +       printk("This is from alc_fixup_no_shutup+ 
> +IIIIIIIIIIIIIIIIIIIIIIIIIIIIII+-.\n");//Deg
> +       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 +7569,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 +8341,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 +11085,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 +11150,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"},
Hi Takashi,
    I study for the logic of ALC269_FIXUP_THINKPAD_ACPI call and test 
many times. From my view, only removed alc_fixup_no_shutup() from 
alc_fixup_ideapad_acpi() is the best way which has no impact for other 
functions call ALC269_FIXUP_THINKPAD_ACPI.
    I have submitted the v3 patch which only removed 
alc_fixup_no_shutup() from alc_fixup_ideapad_acpi() to make
    sure alc_fixup_no_shutup() only be called once for Thinkpad and
    Ideapad.
    Pls review it again and give your comments.

  Best Regards,
Jackie Dong