[PATCH] ALSA: hda/realtek: Fix quirk for TongFang PHxTxX1

[PATCH] ALSA: hda/realtek: Fix quirk for TongFang PHxTxX1

[Werner Sembach]

This fixes the SND_PCI_QUIRK(...) of the TongFang PHxTxX1 barebone. This
fixes the issue of sound not working after s3 suspend.

When waking up from s3 suspend the Coef 0x10 is set to 0x0220 instead of
0x0020. Setting the value manually makes the sound work again. This patch
does this automatically.

While being on it, I also fixed the comment formatting of the quirk.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
Cc: <stable@vger.kernel.org>
---
 sound/pci/hda/patch_realtek.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index 9ce7457533c96..d3a16843c7afd 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -6503,22 +6503,25 @@ static void alc287_fixup_legion_15imhg05_speakers(struct hda_codec *codec,
 /* for alc285_fixup_ideapad_s740_coef() */
 #include "ideapad_s740_helper.c"
 
+static const struct coef_fw alc256_fixup_tongfang_reset_persistent_settings_coefs[] = {
+	WRITE_COEF(0x10, 0x0020), WRITE_COEF(0x24, 0x0000), WRITE_COEF(0x26, 0x0000),
+	WRITE_COEF(0x29, 0x3000), WRITE_COEF(0x37, 0xfe05), WRITE_COEF(0x45, 0x5089),
+	{}
+};
+
 static void alc256_fixup_tongfang_reset_persistent_settings(struct hda_codec *codec,
 							    const struct hda_fixup *fix,
 							    int action)
 {
 	/*
-	* A certain other OS sets these coeffs to different values. On at least one TongFang
-	* barebone these settings might survive even a cold reboot. So to restore a clean slate the
-	* values are explicitly reset to default here. Without this, the external microphone is
-	* always in a plugged-in state, while the internal microphone is always in an unplugged
-	* state, breaking the ability to use the internal microphone.
-	*/
-	alc_write_coef_idx(codec, 0x24, 0x0000);
-	alc_write_coef_idx(codec, 0x26, 0x0000);
-	alc_write_coef_idx(codec, 0x29, 0x3000);
-	alc_write_coef_idx(codec, 0x37, 0xfe05);
-	alc_write_coef_idx(codec, 0x45, 0x5089);
+	 * A certain other OS sets these coeffs to different values. On at least
+	 * one TongFang barebone these settings might survive even a cold
+	 * reboot. So to restore a clean slate the values are explicitly reset
+	 * to default here. Without this, the external microphone is always in a
+	 * plugged-in state, while the internal microphone is always in an
+	 * unplugged state, breaking the ability to use the internal microphone.
+	 */
+	alc_process_coef_fw(codec, alc256_fixup_tongfang_reset_persistent_settings_coefs);
 }
 
 static const struct coef_fw alc233_fixup_no_audio_jack_coefs[] = {
-- 
2.25.1

Re: [PATCH] ALSA: hda/realtek: Fix quirk for TongFang PHxTxX1

[Werner Sembach]

Am 02.12.21 um 16:28 schrieb Werner Sembach:
> This fixes the SND_PCI_QUIRK(...) of the TongFang PHxTxX1 barebone. This
> fixes the issue of sound not working after s3 suspend.
>
> When waking up from s3 suspend the Coef 0x10 is set to 0x0220 instead of
> 0x0020. Setting the value manually makes the sound work again. This patch
> does this automatically.
An educational question: Is there any, at least partial, documentation available on what these Coef values actually do
besides https://www.kernel.org/doc/html/v5.15/sound/hd-audio/realtek-pc-beep.html ?
>
> While being on it, I also fixed the comment formatting of the quirk.
>
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> Cc: <stable@vger.kernel.org>
> ---
>  sound/pci/hda/patch_realtek.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
> index 9ce7457533c96..d3a16843c7afd 100644
> --- a/sound/pci/hda/patch_realtek.c
> +++ b/sound/pci/hda/patch_realtek.c
> @@ -6503,22 +6503,25 @@ static void alc287_fixup_legion_15imhg05_speakers(struct hda_codec *codec,
>  /* for alc285_fixup_ideapad_s740_coef() */
>  #include "ideapad_s740_helper.c"
>  
> +static const struct coef_fw alc256_fixup_tongfang_reset_persistent_settings_coefs[] = {
> +	WRITE_COEF(0x10, 0x0020), WRITE_COEF(0x24, 0x0000), WRITE_COEF(0x26, 0x0000),
> +	WRITE_COEF(0x29, 0x3000), WRITE_COEF(0x37, 0xfe05), WRITE_COEF(0x45, 0x5089),
> +	{}
> +};
> +
>  static void alc256_fixup_tongfang_reset_persistent_settings(struct hda_codec *codec,
>  							    const struct hda_fixup *fix,
>  							    int action)
>  {
>  	/*
> -	* A certain other OS sets these coeffs to different values. On at least one TongFang
> -	* barebone these settings might survive even a cold reboot. So to restore a clean slate the
> -	* values are explicitly reset to default here. Without this, the external microphone is
> -	* always in a plugged-in state, while the internal microphone is always in an unplugged
> -	* state, breaking the ability to use the internal microphone.
> -	*/
> -	alc_write_coef_idx(codec, 0x24, 0x0000);
> -	alc_write_coef_idx(codec, 0x26, 0x0000);
> -	alc_write_coef_idx(codec, 0x29, 0x3000);
> -	alc_write_coef_idx(codec, 0x37, 0xfe05);
> -	alc_write_coef_idx(codec, 0x45, 0x5089);
> +	 * A certain other OS sets these coeffs to different values. On at least
> +	 * one TongFang barebone these settings might survive even a cold
> +	 * reboot. So to restore a clean slate the values are explicitly reset
> +	 * to default here. Without this, the external microphone is always in a
> +	 * plugged-in state, while the internal microphone is always in an
> +	 * unplugged state, breaking the ability to use the internal microphone.
> +	 */
> +	alc_process_coef_fw(codec, alc256_fixup_tongfang_reset_persistent_settings_coefs);
>  }
>  
>  static const struct coef_fw alc233_fixup_no_audio_jack_coefs[] = {

Re: [PATCH] ALSA: hda/realtek: Fix quirk for TongFang PHxTxX1

[Takashi Iwai]

On Thu, 02 Dec 2021 16:28:43 +0100,
Werner Sembach wrote:
> 
> This fixes the SND_PCI_QUIRK(...) of the TongFang PHxTxX1 barebone. This
> fixes the issue of sound not working after s3 suspend.
> 
> When waking up from s3 suspend the Coef 0x10 is set to 0x0220 instead of
> 0x0020. Setting the value manually makes the sound work again. This patch
> does this automatically.
> 
> While being on it, I also fixed the comment formatting of the quirk.
> 
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> Cc: <stable@vger.kernel.org>

Please try to put Fixes tag as this is a fix for the existing change.

The code change looks almost good, but just minor nitpicking:

> ---
>  sound/pci/hda/patch_realtek.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
> index 9ce7457533c96..d3a16843c7afd 100644
> --- a/sound/pci/hda/patch_realtek.c
> +++ b/sound/pci/hda/patch_realtek.c
> @@ -6503,22 +6503,25 @@ static void alc287_fixup_legion_15imhg05_speakers(struct hda_codec *codec,
>  /* for alc285_fixup_ideapad_s740_coef() */
>  #include "ideapad_s740_helper.c"
>  
> +static const struct coef_fw alc256_fixup_tongfang_reset_persistent_settings_coefs[] = {

The variable name is unnecessarily too long.  Please use a shorter
one.

> +	WRITE_COEF(0x10, 0x0020), WRITE_COEF(0x24, 0x0000), WRITE_COEF(0x26, 0x0000),
> +	WRITE_COEF(0x29, 0x3000), WRITE_COEF(0x37, 0xfe05), WRITE_COEF(0x45, 0x5089),

Try to keep the line in 80 column.  It's OK to take a longer if it has
to be, but in a case like this, it doesn't need to be that long.


thanks,

Takashi

> +	{}
> +};
> +
>  static void alc256_fixup_tongfang_reset_persistent_settings(struct hda_codec *codec,
>  							    const struct hda_fixup *fix,
>  							    int action)
>  {
>  	/*
> -	* A certain other OS sets these coeffs to different values. On at least one TongFang
> -	* barebone these settings might survive even a cold reboot. So to restore a clean slate the
> -	* values are explicitly reset to default here. Without this, the external microphone is
> -	* always in a plugged-in state, while the internal microphone is always in an unplugged
> -	* state, breaking the ability to use the internal microphone.
> -	*/
> -	alc_write_coef_idx(codec, 0x24, 0x0000);
> -	alc_write_coef_idx(codec, 0x26, 0x0000);
> -	alc_write_coef_idx(codec, 0x29, 0x3000);
> -	alc_write_coef_idx(codec, 0x37, 0xfe05);
> -	alc_write_coef_idx(codec, 0x45, 0x5089);
> +	 * A certain other OS sets these coeffs to different values. On at least
> +	 * one TongFang barebone these settings might survive even a cold
> +	 * reboot. So to restore a clean slate the values are explicitly reset
> +	 * to default here. Without this, the external microphone is always in a
> +	 * plugged-in state, while the internal microphone is always in an
> +	 * unplugged state, breaking the ability to use the internal microphone.
> +	 */
> +	alc_process_coef_fw(codec, alc256_fixup_tongfang_reset_persistent_settings_coefs);
>  }
>  
>  static const struct coef_fw alc233_fixup_no_audio_jack_coefs[] = {
> -- 
> 2.25.1
> 

Re: [PATCH] ALSA: hda/realtek: Fix quirk for TongFang PHxTxX1

[Takashi Iwai]

On Thu, 02 Dec 2021 16:35:23 +0100,
Werner Sembach wrote:
> 
> Am 02.12.21 um 16:28 schrieb Werner Sembach:
> > This fixes the SND_PCI_QUIRK(...) of the TongFang PHxTxX1 barebone. This
> > fixes the issue of sound not working after s3 suspend.
> >
> > When waking up from s3 suspend the Coef 0x10 is set to 0x0220 instead of
> > 0x0020. Setting the value manually makes the sound work again. This patch
> > does this automatically.
> An educational question: Is there any, at least partial, documentation available on what these Coef values actually do
> besides https://www.kernel.org/doc/html/v5.15/sound/hd-audio/realtek-pc-beep.html ?

Unfortunately, no.  It's a completely vendor-specific black magic.


Takashi

Re: [PATCH] ALSA: hda/realtek: Fix quirk for TongFang PHxTxX1

[Werner Sembach]

Am 02.12.21 um 16:35 schrieb Takashi Iwai:
> On Thu, 02 Dec 2021 16:28:43 +0100,
> Werner Sembach wrote:
>> This fixes the SND_PCI_QUIRK(...) of the TongFang PHxTxX1 barebone. This
>> fixes the issue of sound not working after s3 suspend.
>>
>> When waking up from s3 suspend the Coef 0x10 is set to 0x0220 instead of
>> 0x0020. Setting the value manually makes the sound work again. This patch
>> does this automatically.
>>
>> While being on it, I also fixed the comment formatting of the quirk.
>>
>> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
>> Cc: <stable@vger.kernel.org>
> Please try to put Fixes tag as this is a fix for the existing change.
Sorry, wasn't aware of this. I guess it's [PATCH Fixes] as the beginning of the subject line?
>
> The code change looks almost good, but just minor nitpicking:
>
>> ---
>>  sound/pci/hda/patch_realtek.c | 25 ++++++++++++++-----------
>>  1 file changed, 14 insertions(+), 11 deletions(-)
>>
>> diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
>> index 9ce7457533c96..d3a16843c7afd 100644
>> --- a/sound/pci/hda/patch_realtek.c
>> +++ b/sound/pci/hda/patch_realtek.c
>> @@ -6503,22 +6503,25 @@ static void alc287_fixup_legion_15imhg05_speakers(struct hda_codec *codec,
>>  /* for alc285_fixup_ideapad_s740_coef() */
>>  #include "ideapad_s740_helper.c"
>>  
>> +static const struct coef_fw alc256_fixup_tongfang_reset_persistent_settings_coefs[] = {
> The variable name is unnecessarily too long.  Please use a shorter
> one.
Done.
>
>> +	WRITE_COEF(0x10, 0x0020), WRITE_COEF(0x24, 0x0000), WRITE_COEF(0x26, 0x0000),
>> +	WRITE_COEF(0x29, 0x3000), WRITE_COEF(0x37, 0xfe05), WRITE_COEF(0x45, 0x5089),
> Try to keep the line in 80 column.  It's OK to take a longer if it has
> to be, but in a case like this, it doesn't need to be that long.

Wrong tab width in my editor so I didn't see it, fixed in v2.

Compiling and testing now.

Kind regards,

Werner Sembach

>
>
> thanks,
>
> Takashi
>
>> +	{}
>> +};
>> +
>>  static void alc256_fixup_tongfang_reset_persistent_settings(struct hda_codec *codec,
>>  							    const struct hda_fixup *fix,
>>  							    int action)
>>  {
>>  	/*
>> -	* A certain other OS sets these coeffs to different values. On at least one TongFang
>> -	* barebone these settings might survive even a cold reboot. So to restore a clean slate the
>> -	* values are explicitly reset to default here. Without this, the external microphone is
>> -	* always in a plugged-in state, while the internal microphone is always in an unplugged
>> -	* state, breaking the ability to use the internal microphone.
>> -	*/
>> -	alc_write_coef_idx(codec, 0x24, 0x0000);
>> -	alc_write_coef_idx(codec, 0x26, 0x0000);
>> -	alc_write_coef_idx(codec, 0x29, 0x3000);
>> -	alc_write_coef_idx(codec, 0x37, 0xfe05);
>> -	alc_write_coef_idx(codec, 0x45, 0x5089);
>> +	 * A certain other OS sets these coeffs to different values. On at least
>> +	 * one TongFang barebone these settings might survive even a cold
>> +	 * reboot. So to restore a clean slate the values are explicitly reset
>> +	 * to default here. Without this, the external microphone is always in a
>> +	 * plugged-in state, while the internal microphone is always in an
>> +	 * unplugged state, breaking the ability to use the internal microphone.
>> +	 */
>> +	alc_process_coef_fw(codec, alc256_fixup_tongfang_reset_persistent_settings_coefs);
>>  }
>>  
>>  static const struct coef_fw alc233_fixup_no_audio_jack_coefs[] = {
>> -- 
>> 2.25.1
>>

Re: [PATCH] ALSA: hda/realtek: Fix quirk for TongFang PHxTxX1

[Takashi Iwai]

On Thu, 02 Dec 2021 17:02:51 +0100,
Werner Sembach wrote:
> 
> Am 02.12.21 um 16:35 schrieb Takashi Iwai:
> > On Thu, 02 Dec 2021 16:28:43 +0100,
> > Werner Sembach wrote:
> >> This fixes the SND_PCI_QUIRK(...) of the TongFang PHxTxX1 barebone. This
> >> fixes the issue of sound not working after s3 suspend.
> >>
> >> When waking up from s3 suspend the Coef 0x10 is set to 0x0220 instead of
> >> 0x0020. Setting the value manually makes the sound work again. This patch
> >> does this automatically.
> >>
> >> While being on it, I also fixed the comment formatting of the quirk.
> >>
> >> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> >> Cc: <stable@vger.kernel.org>
> > Please try to put Fixes tag as this is a fix for the existing change.
> Sorry, wasn't aware of this. I guess it's [PATCH Fixes] as the beginning of the subject line?

No, the Fixes tag is another line you add around your Signed-off-by
line for indicating that that the patch is for fixing the given
commit.  In your case, it'll be like

Fixes: dd6dd6e3c791 ("ALSA: hda/realtek: Add quirk for TongFang PHxTxX1")


HTH,

Takashi

Re: [PATCH] ALSA: hda/realtek: Fix quirk for TongFang PHxTxX1

[Werner Sembach]

Am 02.12.21 um 17:19 schrieb Takashi Iwai:
> On Thu, 02 Dec 2021 17:02:51 +0100,
> Werner Sembach wrote:
>> Am 02.12.21 um 16:35 schrieb Takashi Iwai:
>>> On Thu, 02 Dec 2021 16:28:43 +0100,
>>> Werner Sembach wrote:
>>>> This fixes the SND_PCI_QUIRK(...) of the TongFang PHxTxX1 barebone. This
>>>> fixes the issue of sound not working after s3 suspend.
>>>>
>>>> When waking up from s3 suspend the Coef 0x10 is set to 0x0220 instead of
>>>> 0x0020. Setting the value manually makes the sound work again. This patch
>>>> does this automatically.
>>>>
>>>> While being on it, I also fixed the comment formatting of the quirk.
>>>>
>>>> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
>>>> Cc: <stable@vger.kernel.org>
>>> Please try to put Fixes tag as this is a fix for the existing change.
>> Sorry, wasn't aware of this. I guess it's [PATCH Fixes] as the beginning of the subject line?
> No, the Fixes tag is another line you add around your Signed-off-by
> line for indicating that that the patch is for fixing the given
> commit.  In your case, it'll be like
>
> Fixes: dd6dd6e3c791 ("ALSA: hda/realtek: Add quirk for TongFang PHxTxX1")
>
>
> HTH,
>
> Takashi

Thanks, that was indeed very helpful.

Sorry for still making noob mistakes.