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

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

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
> 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