[PATCH] ALSA: hda/conexant: Fix headset auto detect fail in cx8070 and SN6140

[PATCH] ALSA: hda/conexant: Fix headset auto detect fail in cx8070 and SN6140

[bo liu]

When OMTP headset plugin the headset jack of CX8070 and SN6160 sound cards,
the headset type detection circuit will recognize the headset type as CTIA.
At this point, plugout and plugin the headset will get the correct headset
type as OMTP.
The reason for the failure of headset type recognition is that the sound
card creation will enable the VREF voltage of the headset mic, which
interferes with the headset type automatic detection circuit. Plugout and
plugin the headset will restart the headset detection and get the correct
headset type.
The patch is disable the VREF voltage when the headset is not present, and
will enable the VREF voltage when the headset is present.

Signed-off-by: bo liu <bo.liu@senarytech.com>
---
 sound/pci/hda/patch_conexant.c | 72 +++++++++++++++++++++++++++++++++-
 1 file changed, 71 insertions(+), 1 deletion(-)

diff --git a/sound/pci/hda/patch_conexant.c b/sound/pci/hda/patch_conexant.c
index a889cccdd607..e24befa1fad9 100644
--- a/sound/pci/hda/patch_conexant.c
+++ b/sound/pci/hda/patch_conexant.c
@@ -166,6 +166,7 @@ static void cxt_init_gpio_led(struct hda_codec *codec)
 
 static int cx_auto_init(struct hda_codec *codec)
 {
+	unsigned int mic_persent;
 	struct conexant_spec *spec = codec->spec;
 	snd_hda_gen_init(codec);
 	if (!spec->dynamic_eapd)
@@ -174,6 +175,22 @@ static int cx_auto_init(struct hda_codec *codec)
 	cxt_init_gpio_led(codec);
 	snd_hda_apply_fixup(codec, HDA_FIXUP_ACT_INIT);
 
+	switch (codec->core.vendor_id) {
+	case 0x14f11f86:
+	case 0x14f11f87:
+		/* fix some headset type recognize fail issue, such as EDIFIER headset */
+		snd_hda_codec_write(codec, 0x1c, 0, 0x320, 0x010);
+		snd_hda_codec_write(codec, 0x1c, 0, 0x3b0, 0xe10);
+		snd_hda_codec_write(codec, 0x1c, 0, 0x4f0, 0x0eb);
+		/* fix reboot headset type recognize fail issue */
+		mic_persent = snd_hda_codec_read(codec, 0x19, 0, 0xf09, 0x0);
+		if (mic_persent&0x80000000)
+			snd_hda_codec_write(codec, 0x19, 0, 0x707, 0x24);
+		else
+			snd_hda_codec_write(codec, 0x19, 0, 0x707, 0x20);
+		break;
+	}
+
 	return 0;
 }
 
@@ -192,6 +209,58 @@ static void cx_auto_free(struct hda_codec *codec)
 	snd_hda_gen_free(codec);
 }
 
+static int headset_present_flag;
+static void cx_jack_unsol_event(struct hda_codec *codec, unsigned int res)
+{
+	unsigned int val, phone_present, mic_persent, phone_tag, mic_tag;
+	unsigned int count = 0;
+
+	switch (codec->core.vendor_id) {
+	case 0x14f11f86:
+	case 0x14f11f87:
+		/* check hp&mic tag to process headset pulgin&plugout */
+		phone_tag = snd_hda_codec_read(codec, 0x16, 0, 0xf08, 0x0);
+		mic_tag = snd_hda_codec_read(codec, 0x19, 0, 0xf08, 0x0);
+		if ((phone_tag&(res>>26)) || (mic_tag&(res>>26))) {
+			phone_present = snd_hda_codec_read(codec, 0x16, 0, 0xf09, 0x0);
+			if (!(phone_present&0x80000000)) {/* headphone plugout */
+				headset_present_flag = 0;
+				snd_hda_codec_write(codec, 0x19, 0, 0x707, 0x20);
+				break;
+			}
+			if (headset_present_flag == 0) {
+				headset_present_flag = 1;
+			} else if (headset_present_flag == 1) {
+				mic_persent = snd_hda_codec_read(codec, 0x19, 0, 0xf09, 0x0);
+				/* headset is present */
+				if ((phone_present&0x80000000) && (mic_persent&0x80000000)) {
+					/* wait headset detect done */
+					do {
+						msleep(20);
+						val = snd_hda_codec_read(codec, 0x1c,
+									0, 0xca0, 0x0);
+						count += 1;
+					} while ((count > 3) || (val&0x080));
+					val = snd_hda_codec_read(codec, 0x1c, 0, 0xcb0, 0x0);
+					if (val&0x800) {
+						codec_dbg(codec, "headset plugin, type is CTIA\n");
+						snd_hda_codec_write(codec, 0x19, 0, 0x707, 0x24);
+					} else if (val&0x400) {
+						codec_dbg(codec, "headset plugin, type is OMTP\n");
+						snd_hda_codec_write(codec, 0x19, 0, 0x707, 0x24);
+					} else {
+						codec_dbg(codec, "headphone plugin\n");
+					}
+					headset_present_flag = 2;
+				}
+			}
+		}
+		break;
+	}
+
+	snd_hda_jack_unsol_event(codec, res);
+}
+
 #ifdef CONFIG_PM
 static int cx_auto_suspend(struct hda_codec *codec)
 {
@@ -205,7 +274,7 @@ static const struct hda_codec_ops cx_auto_patch_ops = {
 	.build_pcms = snd_hda_gen_build_pcms,
 	.init = cx_auto_init,
 	.free = cx_auto_free,
-	.unsol_event = snd_hda_jack_unsol_event,
+	.unsol_event = cx_jack_unsol_event,
 #ifdef CONFIG_PM
 	.suspend = cx_auto_suspend,
 	.check_power_status = snd_hda_gen_check_power_status,
@@ -1042,6 +1111,7 @@ static int patch_conexant_auto(struct hda_codec *codec)
 	codec->spec = spec;
 	codec->patch_ops = cx_auto_patch_ops;
 
+	headset_present_flag = 0;
 	cx_auto_parse_eapd(codec);
 	spec->gen.own_eapd_ctl = 1;
 
-- 
2.34.1

Re: [PATCH] ALSA: hda/conexant: Fix headset auto detect fail in cx8070 and SN6140

[Takashi Iwai]

On Tue, 02 Jan 2024 07:04:57 +0100,
bo liu wrote:
> 
> When OMTP headset plugin the headset jack of CX8070 and SN6160 sound cards,
> the headset type detection circuit will recognize the headset type as CTIA.
> At this point, plugout and plugin the headset will get the correct headset
> type as OMTP.
> The reason for the failure of headset type recognition is that the sound
> card creation will enable the VREF voltage of the headset mic, which
> interferes with the headset type automatic detection circuit. Plugout and
> plugin the headset will restart the headset detection and get the correct
> headset type.
> The patch is disable the VREF voltage when the headset is not present, and
> will enable the VREF voltage when the headset is present.
> 
> Signed-off-by: bo liu <bo.liu@senarytech.com>

Thanks, this is *much* better than the previous version!

However, something still need to be fixed in the content:

> ---
>  sound/pci/hda/patch_conexant.c | 72 +++++++++++++++++++++++++++++++++-
>  1 file changed, 71 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/pci/hda/patch_conexant.c b/sound/pci/hda/patch_conexant.c
> index a889cccdd607..e24befa1fad9 100644
> --- a/sound/pci/hda/patch_conexant.c
> +++ b/sound/pci/hda/patch_conexant.c
> @@ -166,6 +166,7 @@ static void cxt_init_gpio_led(struct hda_codec *codec)
>  
>  static int cx_auto_init(struct hda_codec *codec)
>  {
> +	unsigned int mic_persent;
>  	struct conexant_spec *spec = codec->spec;
>  	snd_hda_gen_init(codec);
>  	if (!spec->dynamic_eapd)
> @@ -174,6 +175,22 @@ static int cx_auto_init(struct hda_codec *codec)
>  	cxt_init_gpio_led(codec);
>  	snd_hda_apply_fixup(codec, HDA_FIXUP_ACT_INIT);
>  
> +	switch (codec->core.vendor_id) {
> +	case 0x14f11f86:
> +	case 0x14f11f87:

If those ID checks appear multiple times, it's better to make it as a
flag in conexant_spec, and set it at the probe time.

> +		/* fix some headset type recognize fail issue, such as EDIFIER headset */
> +		snd_hda_codec_write(codec, 0x1c, 0, 0x320, 0x010);
> +		snd_hda_codec_write(codec, 0x1c, 0, 0x3b0, 0xe10);
> +		snd_hda_codec_write(codec, 0x1c, 0, 0x4f0, 0x0eb);
(snip)

Those code can be better factored out to a function.
It'll lead to less indentation, hence it makes easier to read, too.

> @@ -192,6 +209,58 @@ static void cx_auto_free(struct hda_codec *codec)
>  	snd_hda_gen_free(codec);
>  }
>  
> +static int headset_present_flag;

It's bad to use a static variable here.  In theory, there can be
multiple same codecs used on the bus.

If any, put this into conexant_spec and use locally.

> +static void cx_jack_unsol_event(struct hda_codec *codec, unsigned int res)
> +{
> +	unsigned int val, phone_present, mic_persent, phone_tag, mic_tag;
> +	unsigned int count = 0;
> +
> +	switch (codec->core.vendor_id) {
> +	case 0x14f11f86:
> +	case 0x14f11f87:

Again, use a different flag.  Also factor out the specific code into a
function.

> +		/* check hp&mic tag to process headset pulgin&plugout */
> +		phone_tag = snd_hda_codec_read(codec, 0x16, 0, 0xf08, 0x0);
> +		mic_tag = snd_hda_codec_read(codec, 0x19, 0, 0xf08, 0x0);

Are those pins *always* fixed to 0x16 and 0x19?  Or they might be
assigned to different pins...?   In the latter case, the pin nid
should be taken from the parsed configuration instead of fixed
numbers.

> +		if ((phone_tag&(res>>26)) || (mic_tag&(res>>26))) {

Some coding style issues here.  Consult scripts/checkpatch.pl.
Also avoid a magic number.  0xf08 is AC_VERB_GET_UNSOLICITED_RESPONSE,
and 26 is AC_UNSOL_RES_TAG_SHIFT, for example.

> +			phone_present = snd_hda_codec_read(codec, 0x16, 0, 0xf09, 0x0);
> +			if (!(phone_present&0x80000000)) {/* headphone plugout */

Ditto.  0x80000000 is AC_PINSENSE_PRESENCE.

> +				headset_present_flag = 0;

Better to use an enum to hold the state instead of the raw 0, 1, 2.


> +				break;
> +			}
> +			if (headset_present_flag == 0) {
> +				headset_present_flag = 1;
> +			} else if (headset_present_flag == 1) {
> +				mic_persent = snd_hda_codec_read(codec, 0x19, 0, 0xf09, 0x0);
> +				/* headset is present */
> +				if ((phone_present&0x80000000) && (mic_persent&0x80000000)) {
> +					/* wait headset detect done */
> +					do {
> +						msleep(20);
> +						val = snd_hda_codec_read(codec, 0x1c,
> +									0, 0xca0, 0x0);
> +						count += 1;

Usually we use "++" for increment.


thanks,

Takashi

[PATCH] ALSA: hda/conexant: Fix headset auto detect fail in cx8070 and SN6140

[bo liu]

When OMTP headset plugin the headset jack of CX8070 and SN6160 sound cards,
the headset type detection circuit will recognize the headset type as CTIA.
At this point, plugout and plugin the headset will get the correct headset
type as OMTP.
The reason for the failure of headset type recognition is that the sound
card creation will enable the VREF voltage of the headset mic, which
interferes with the headset type automatic detection circuit. Plugout and
plugin the headset will restart the headset detection and get the correct
headset type.
The patch is disable the VREF voltage when the headset is not present, and
will enable the VREF voltage when the headset is present.

Signed-off-by: bo liu <bo.liu@senarytech.com>
---
 sound/pci/hda/patch_conexant.c | 108 ++++++++++++++++++++++++++++++++-
 1 file changed, 106 insertions(+), 2 deletions(-)

diff --git a/sound/pci/hda/patch_conexant.c b/sound/pci/hda/patch_conexant.c
index a889cccdd607..29c62181dbc3 100644
--- a/sound/pci/hda/patch_conexant.c
+++ b/sound/pci/hda/patch_conexant.c
@@ -42,7 +42,8 @@ struct conexant_spec {
 	unsigned int gpio_led;
 	unsigned int gpio_mute_led_mask;
 	unsigned int gpio_mic_led_mask;
-
+	unsigned int headset_present_flag;
+	bool is_cx8070_sn6140;
 };
 
 
@@ -164,6 +165,22 @@ static void cxt_init_gpio_led(struct hda_codec *codec)
 	}
 }
 
+static void cx_fixup_headset_recog(struct hda_codec *codec)
+{
+	unsigned int mic_persent;
+
+	/* fix some headset type recognize fail issue, such as EDIFIER headset */
+	snd_hda_codec_write(codec, 0x1c, 0, 0x320, 0x010);
+	snd_hda_codec_write(codec, 0x1c, 0, 0x3b0, 0xe10);
+	snd_hda_codec_write(codec, 0x1c, 0, 0x4f0, 0x0eb);
+	/* fix reboot headset type recognize fail issue */
+	mic_persent = snd_hda_codec_read(codec, 0x19, 0, AC_VERB_GET_PIN_SENSE, 0x0);
+	if (mic_persent&0x80000000)
+		snd_hda_codec_write(codec, 0x19, 0, AC_VERB_SET_PIN_WIDGET_CONTROL, 0x24);
+	else
+		snd_hda_codec_write(codec, 0x19, 0, AC_VERB_SET_PIN_WIDGET_CONTROL, 0x20);
+}
+
 static int cx_auto_init(struct hda_codec *codec)
 {
 	struct conexant_spec *spec = codec->spec;
@@ -174,6 +191,9 @@ static int cx_auto_init(struct hda_codec *codec)
 	cxt_init_gpio_led(codec);
 	snd_hda_apply_fixup(codec, HDA_FIXUP_ACT_INIT);
 
+	if (spec->is_cx8070_sn6140)
+		cx_fixup_headset_recog(codec);
+
 	return 0;
 }
 
@@ -192,6 +212,81 @@ static void cx_auto_free(struct hda_codec *codec)
 	snd_hda_gen_free(codec);
 }
 
+enum {
+	CX_HEADSET_NOPRESENT = 0,
+	CX_HEADSET_PARTPRESENT,
+	CX_HEADSET_ALLPRESENT,
+};
+
+static void cx_process_headset_plugin(struct hda_codec *codec)
+{
+	unsigned int val;
+	unsigned int count = 0;
+
+	/* Wait headset detect done. */
+	do {
+		val = snd_hda_codec_read(codec, 0x1c, 0, 0xca0, 0x0);
+		if (val&0x080) {
+			codec_dbg(codec, "headset type detect done!\n");
+			break;
+		}
+		msleep(20);
+		count++;
+	} while (count < 3);
+	val = snd_hda_codec_read(codec, 0x1c, 0, 0xcb0, 0x0);
+	if (val&0x800) {
+		codec_dbg(codec, "headset plugin, type is CTIA\n");
+		snd_hda_codec_write(codec, 0x19, 0, AC_VERB_SET_PIN_WIDGET_CONTROL, 0x24);
+	} else if (val&0x400) {
+		codec_dbg(codec, "headset plugin, type is OMTP\n");
+		snd_hda_codec_write(codec, 0x19, 0, AC_VERB_SET_PIN_WIDGET_CONTROL, 0x24);
+	} else {
+		codec_dbg(codec, "headphone plugin\n");
+	}
+}
+
+static void cx_update_headset_mic_vref(struct hda_codec *codec, unsigned int res)
+{
+	unsigned int phone_present, mic_persent, phone_tag, mic_tag;
+	struct conexant_spec *spec = codec->spec;
+
+	/* In cx8070 and sn6140, headset is fixed to use node 16 and node 19.
+	 * Check hp and mic tag to process headset pulgin and plugout.
+	 */
+	phone_tag = snd_hda_codec_read(codec, 0x16, 0, AC_VERB_GET_UNSOLICITED_RESPONSE, 0x0);
+	mic_tag = snd_hda_codec_read(codec, 0x19, 0, AC_VERB_GET_UNSOLICITED_RESPONSE, 0x0);
+	if ((phone_tag&(res>>AC_UNSOL_RES_TAG_SHIFT)) || (mic_tag&(res>>AC_UNSOL_RES_TAG_SHIFT))) {
+		phone_present = snd_hda_codec_read(codec, 0x16, 0, AC_VERB_GET_PIN_SENSE, 0x0);
+		if (!(phone_present&AC_PINSENSE_PRESENCE)) {/* headphone plugout */
+			spec->headset_present_flag = CX_HEADSET_NOPRESENT;
+			snd_hda_codec_write(codec, 0x19, 0, AC_VERB_SET_PIN_WIDGET_CONTROL, 0x20);
+			return;
+		}
+		if (spec->headset_present_flag == CX_HEADSET_NOPRESENT) {
+			spec->headset_present_flag = CX_HEADSET_PARTPRESENT;
+		} else if (spec->headset_present_flag == CX_HEADSET_PARTPRESENT) {
+			mic_persent = snd_hda_codec_read(codec, 0x19, 0,
+							 AC_VERB_GET_PIN_SENSE, 0x0);
+			/* headset is present */
+			if ((phone_present&AC_PINSENSE_PRESENCE) &&
+			    (mic_persent&AC_PINSENSE_PRESENCE)) {
+				cx_process_headset_plugin(codec);
+				spec->headset_present_flag = CX_HEADSET_ALLPRESENT;
+			}
+		}
+	}
+}
+
+static void cx_jack_unsol_event(struct hda_codec *codec, unsigned int res)
+{
+	struct conexant_spec *spec = codec->spec;
+
+	if (spec->is_cx8070_sn6140)
+		cx_update_headset_mic_vref(codec, res);
+
+	snd_hda_jack_unsol_event(codec, res);
+}
+
 #ifdef CONFIG_PM
 static int cx_auto_suspend(struct hda_codec *codec)
 {
@@ -205,7 +300,7 @@ static const struct hda_codec_ops cx_auto_patch_ops = {
 	.build_pcms = snd_hda_gen_build_pcms,
 	.init = cx_auto_init,
 	.free = cx_auto_free,
-	.unsol_event = snd_hda_jack_unsol_event,
+	.unsol_event = cx_jack_unsol_event,
 #ifdef CONFIG_PM
 	.suspend = cx_auto_suspend,
 	.check_power_status = snd_hda_gen_check_power_status,
@@ -1042,6 +1137,15 @@ static int patch_conexant_auto(struct hda_codec *codec)
 	codec->spec = spec;
 	codec->patch_ops = cx_auto_patch_ops;
 
+	/* init cx8070/sn6140 flag and reset headset_present_flag */
+	switch (codec->core.vendor_id) {
+	case 0x14f11f86:
+	case 0x14f11f87:
+		spec->is_cx8070_sn6140 = true;
+		spec->headset_present_flag = CX_HEADSET_NOPRESENT;
+		break;
+	}
+
 	cx_auto_parse_eapd(codec);
 	spec->gen.own_eapd_ctl = 1;
 
-- 
2.34.1

Re: [PATCH] ALSA: hda/conexant: Fix headset auto detect fail in cx8070 and SN6140

[Takashi Iwai]

On Thu, 04 Jan 2024 12:10:44 +0100,
bo liu wrote:
> 
> When OMTP headset plugin the headset jack of CX8070 and SN6160 sound cards,
> the headset type detection circuit will recognize the headset type as CTIA.
> At this point, plugout and plugin the headset will get the correct headset
> type as OMTP.
> The reason for the failure of headset type recognition is that the sound
> card creation will enable the VREF voltage of the headset mic, which
> interferes with the headset type automatic detection circuit. Plugout and
> plugin the headset will restart the headset detection and get the correct
> headset type.
> The patch is disable the VREF voltage when the headset is not present, and
> will enable the VREF voltage when the headset is present.
> 
> Signed-off-by: bo liu <bo.liu@senarytech.com>

Please put the revision number to the subject prefix, i.e.
"Subject: [PATCH v3] ALSA: hda/conexant: ...."

> +static void cx_fixup_headset_recog(struct hda_codec *codec)
> +{
> +	unsigned int mic_persent;
> +
> +	/* fix some headset type recognize fail issue, such as EDIFIER headset */
> +	snd_hda_codec_write(codec, 0x1c, 0, 0x320, 0x010);
> +	snd_hda_codec_write(codec, 0x1c, 0, 0x3b0, 0xe10);
> +	snd_hda_codec_write(codec, 0x1c, 0, 0x4f0, 0x0eb);

Please use the defined verbs in sound/hda_verbs.h.
The arguments (0x320, 0x010) are (AC_VERB_SET_AMP_GAIN_MUTE, 0x2010)
etc.

Also, it's still not clear what if other nodes are used for headphone
and mic pins -- or when either only headphone or only mic is present.
A rare case, but we need to cover.

> +	/* fix reboot headset type recognize fail issue */
> +	mic_persent = snd_hda_codec_read(codec, 0x19, 0, AC_VERB_GET_PIN_SENSE, 0x0);
> +	if (mic_persent&0x80000000)

A coding style problem.  Similar issues seen in a few other places,
too.  Consult scripts/checkpatch.pl.

> +enum {
> +	CX_HEADSET_NOPRESENT = 0,
> +	CX_HEADSET_PARTPRESENT,
> +	CX_HEADSET_ALLPRESENT,
> +};

This should be defined earlier.  You can use the enum type for
spec->headset_present_flag, too.

> +static void cx_process_headset_plugin(struct hda_codec *codec)
> +{
> +	unsigned int val;
> +	unsigned int count = 0;
> +
> +	/* Wait headset detect done. */
> +	do {
> +		val = snd_hda_codec_read(codec, 0x1c, 0, 0xca0, 0x0);

Use the verb: AC_VERB_GET_PROC_COEF, 0xa000.
At best, define the COEF values 0xa000 and 0xb000, and the
corresponding value bits, too.

> +static void cx_update_headset_mic_vref(struct hda_codec *codec, unsigned int res)
> +{
> +	unsigned int phone_present, mic_persent, phone_tag, mic_tag;
> +	struct conexant_spec *spec = codec->spec;
> +
> +	/* In cx8070 and sn6140, headset is fixed to use node 16 and node 19.

Is it really guaranteed?  IMO, we should check the pin configs
beforehand at the parsing time.


thanks,

Takashi

Re: [PATCH] ALSA: hda/conexant: Fix headset auto detect fail in cx8070 and SN6140

[刘博]

hi Takashi Iwai,
	Thank you very much for your patient guidance. Below is the reply to
the question, please kindly correct it, thanks.

> +static void cx_fixup_headset_recog(struct hda_codec *codec) {
> +	unsigned int mic_persent;
> +
> +	/* fix some headset type recognize fail issue, such as EDIFIER
headset */
> +	snd_hda_codec_write(codec, 0x1c, 0, 0x320, 0x010);
> +	snd_hda_codec_write(codec, 0x1c, 0, 0x3b0, 0xe10);
> +	snd_hda_codec_write(codec, 0x1c, 0, 0x4f0, 0x0eb);

Please use the defined verbs in sound/hda_verbs.h.
The arguments (0x320, 0x010) are (AC_VERB_SET_AMP_GAIN_MUTE, 0x2010) etc.

Re: (0x1c, 0x320) is not amp gain register, but vendor defined register as
rx control register. Use AC_VERB_SET_AMP_GAIN_MUTE will confused. It's
similar to 0x4f0 and 0xca0.

Also, it's still not clear what if other nodes are used for headphone and
mic pins -- or when either only headphone or only mic is present.
A rare case, but we need to cover.

Re: in cx8070 and sn6140, only 0x16 and 0x19 can be used together as
headset. Other nodes can be used separately as headphones or microphones,
but not as headset, 
so their configuration will not interfere with the type detection of
headset.

> +	/* fix reboot headset type recognize fail issue */
> +	mic_persent = snd_hda_codec_read(codec, 0x19, 0,
AC_VERB_GET_PIN_SENSE, 0x0);
> +	if (mic_persent&0x80000000)

A coding style problem.  Similar issues seen in a few other places, too.
Consult scripts/checkpatch.pl.

Re: was & need space? I have checked with scripts/checkpatch.pl before
submitting the patch and there are no warnings or errors.

> +static void cx_process_headset_plugin(struct hda_codec *codec) {
> +	unsigned int val;
> +	unsigned int count = 0;
> +
> +	/* Wait headset detect done. */
> +	do {
> +		val = snd_hda_codec_read(codec, 0x1c, 0, 0xca0, 0x0);

Use the verb: AC_VERB_GET_PROC_COEF, 0xa000.
At best, define the COEF values 0xa000 and 0xb000, and the corresponding
value bits, too.

Re: (0x1c, 0xca0) is not COEF register, but vendor defined register as
jacksense register.

> +static void cx_update_headset_mic_vref(struct hda_codec *codec, 
> +unsigned int res) {
> +	unsigned int phone_present, mic_persent, phone_tag, mic_tag;
> +	struct conexant_spec *spec = codec->spec;
> +
> +	/* In cx8070 and sn6140, headset is fixed to use node 16 and node
19.

Is it really guaranteed?  IMO, we should check the pin configs beforehand at
the parsing time.

Re: in cx8070 and sn6140, only 0x16 and 0x19 can be used together as
headset. The node 16 can only be config to headphone or disable,
The node 19 can only be config to microphone or disable. Only node 16 and
node 19 both enable, the patch will process.

Best Regards
Bo Liu
　
-----邮件原件-----
发件人: Takashi Iwai <tiwai@suse.de> 
发送时间: 2024年1月6日 1:02
收件人: bo liu <bo.liu@senarytech.com>
抄送: perex@perex.cz; tiwai@suse.com; linux-sound@vger.kernel.org;
linux-kernel@vger.kernel.org
主题: Re: [PATCH] ALSA: hda/conexant: Fix headset auto detect fail in cx8070
and SN6140

On Thu, 04 Jan 2024 12:10:44 +0100,
bo liu wrote:
> 
> When OMTP headset plugin the headset jack of CX8070 and SN6160 sound 
> cards, the headset type detection circuit will recognize the headset type
as CTIA.
> At this point, plugout and plugin the headset will get the correct 
> headset type as OMTP.
> The reason for the failure of headset type recognition is that the 
> sound card creation will enable the VREF voltage of the headset mic, 
> which interferes with the headset type automatic detection circuit. 
> Plugout and plugin the headset will restart the headset detection and 
> get the correct headset type.
> The patch is disable the VREF voltage when the headset is not present, 
> and will enable the VREF voltage when the headset is present.
> 
> Signed-off-by: bo liu <bo.liu@senarytech.com>

Please put the revision number to the subject prefix, i.e.
"Subject: [PATCH v3] ALSA: hda/conexant: ...."

> +static void cx_fixup_headset_recog(struct hda_codec *codec) {
> +	unsigned int mic_persent;
> +
> +	/* fix some headset type recognize fail issue, such as EDIFIER
headset */
> +	snd_hda_codec_write(codec, 0x1c, 0, 0x320, 0x010);
> +	snd_hda_codec_write(codec, 0x1c, 0, 0x3b0, 0xe10);
> +	snd_hda_codec_write(codec, 0x1c, 0, 0x4f0, 0x0eb);

Please use the defined verbs in sound/hda_verbs.h.
The arguments (0x320, 0x010) are (AC_VERB_SET_AMP_GAIN_MUTE, 0x2010) etc.

Also, it's still not clear what if other nodes are used for headphone and
mic pins -- or when either only headphone or only mic is present.
A rare case, but we need to cover.

> +	/* fix reboot headset type recognize fail issue */
> +	mic_persent = snd_hda_codec_read(codec, 0x19, 0,
AC_VERB_GET_PIN_SENSE, 0x0);
> +	if (mic_persent&0x80000000)

A coding style problem.  Similar issues seen in a few other places, too.
Consult scripts/checkpatch.pl.

> +enum {
> +	CX_HEADSET_NOPRESENT = 0,
> +	CX_HEADSET_PARTPRESENT,
> +	CX_HEADSET_ALLPRESENT,
> +};

This should be defined earlier.  You can use the enum type for
spec->headset_present_flag, too.

> +static void cx_process_headset_plugin(struct hda_codec *codec) {
> +	unsigned int val;
> +	unsigned int count = 0;
> +
> +	/* Wait headset detect done. */
> +	do {
> +		val = snd_hda_codec_read(codec, 0x1c, 0, 0xca0, 0x0);

Use the verb: AC_VERB_GET_PROC_COEF, 0xa000.
At best, define the COEF values 0xa000 and 0xb000, and the corresponding
value bits, too.

> +static void cx_update_headset_mic_vref(struct hda_codec *codec, 
> +unsigned int res) {
> +	unsigned int phone_present, mic_persent, phone_tag, mic_tag;
> +	struct conexant_spec *spec = codec->spec;
> +
> +	/* In cx8070 and sn6140, headset is fixed to use node 16 and node
19.

Is it really guaranteed?  IMO, we should check the pin configs beforehand at
the parsing time.


thanks,

Takashi

Re: [PATCH] ALSA: hda/conexant: Fix headset auto detect fail in cx8070 and SN6140

[Takashi Iwai]

On Mon, 08 Jan 2024 04:29:51 +0100,
刘博 wrote:
> 
> hi Takashi Iwai,
> 	Thank you very much for your patient guidance. Below is the reply to
> the question, please kindly correct it, thanks.
> 
> > +static void cx_fixup_headset_recog(struct hda_codec *codec) {
> > +	unsigned int mic_persent;
> > +
> > +	/* fix some headset type recognize fail issue, such as EDIFIER
> headset */
> > +	snd_hda_codec_write(codec, 0x1c, 0, 0x320, 0x010);
> > +	snd_hda_codec_write(codec, 0x1c, 0, 0x3b0, 0xe10);
> > +	snd_hda_codec_write(codec, 0x1c, 0, 0x4f0, 0x0eb);
> 
> Please use the defined verbs in sound/hda_verbs.h.
> The arguments (0x320, 0x010) are (AC_VERB_SET_AMP_GAIN_MUTE, 0x2010) etc.
> 
> Re: (0x1c, 0x320) is not amp gain register, but vendor defined register as
> rx control register. Use AC_VERB_SET_AMP_GAIN_MUTE will confused. It's
> similar to 0x4f0 and 0xca0.

Ah interesting.  But the verb is actually seen as
AC_VERB_SET_AMP_GAIN_MUTE -- although the resultant bits seem invalid.

HD-audio combines the verb and the value into 20 bits, e.g. (0x320,
0x10) is composed as 0x32010, and (0x3b0, 0xe10) is 0x3be10.
0x3xx is translated as SET_AMP_GAIN_MUTE, but in your case, 0x32010
leaves 0 to both the input/output bits (bits 14 and 15), which makes
it as invalid.

0x3be10 is another invalid verb, which sets SET_AMP_GAIN_MUTE with
OUTPUT, but it sets both LEFT and RIGHT, and passes a high index
(14).

And, what actually (0x4f0, 0x0eb) does?  It's composed as 0x4f0eb, and
in this case, it's a valid verb (SET_PROC_COEF + 0xf0eb).  But COEF is
vendor-specific, so it can be translated in everything the chip
wants.

So, if those verbs are vendor-specific ones, please define them and/or
give proper comments to explain what they do for each.


> Also, it's still not clear what if other nodes are used for headphone and
> mic pins -- or when either only headphone or only mic is present.
> A rare case, but we need to cover.
> 
> Re: in cx8070 and sn6140, only 0x16 and 0x19 can be used together as
> headset. Other nodes can be used separately as headphones or microphones,
> but not as headset, 
> so their configuration will not interfere with the type detection of
> headset.

OK, then explain this in comments, too (that we blindly assume those
pins).


> > +	/* fix reboot headset type recognize fail issue */
> > +	mic_persent = snd_hda_codec_read(codec, 0x19, 0,
> AC_VERB_GET_PIN_SENSE, 0x0);
> > +	if (mic_persent&0x80000000)
> 
> A coding style problem.  Similar issues seen in a few other places, too.
> Consult scripts/checkpatch.pl.
> 
> Re: was & need space? I have checked with scripts/checkpatch.pl before
> submitting the patch and there are no warnings or errors.

Yes.  Please put spaces around the operators.


> > +static void cx_process_headset_plugin(struct hda_codec *codec) {
> > +	unsigned int val;
> > +	unsigned int count = 0;
> > +
> > +	/* Wait headset detect done. */
> > +	do {
> > +		val = snd_hda_codec_read(codec, 0x1c, 0, 0xca0, 0x0);
> 
> Use the verb: AC_VERB_GET_PROC_COEF, 0xa000.
> At best, define the COEF values 0xa000 and 0xb000, and the corresponding
> value bits, too.
> 
> Re: (0x1c, 0xca0) is not COEF register, but vendor defined register as
> jacksense register.
> 
> > +static void cx_update_headset_mic_vref(struct hda_codec *codec, 
> > +unsigned int res) {
> > +	unsigned int phone_present, mic_persent, phone_tag, mic_tag;
> > +	struct conexant_spec *spec = codec->spec;
> > +
> > +	/* In cx8070 and sn6140, headset is fixed to use node 16 and node
> 19.
> 
> Is it really guaranteed?  IMO, we should check the pin configs beforehand at
> the parsing time.
> 
> Re: in cx8070 and sn6140, only 0x16 and 0x19 can be used together as
> headset. The node 16 can only be config to headphone or disable,
> The node 19 can only be config to microphone or disable. Only node 16 and
> node 19 both enable, the patch will process.

Then we still might need a check for the condition?


thanks,

Takashi

Re: [PATCH] ALSA: hda/conexant: Fix headset auto detect fail in cx8070 and SN6140

[刘博]

Hi Takashi,
	Thank you for your quick reply. I hope the following response can be
explained clearly, thanks.

> > > +	/* fix some headset type recognize fail issue, such as EDIFIER
> > headset */
> > > +	snd_hda_codec_write(codec, 0x1c, 0, 0x320, 0x010);
> > > +	snd_hda_codec_write(codec, 0x1c, 0, 0x3b0, 0xe10);
> > > +	snd_hda_codec_write(codec, 0x1c, 0, 0x4f0, 0x0eb);
> > 
> > Please use the defined verbs in sound/hda_verbs.h.
> > The arguments (0x320, 0x010) are (AC_VERB_SET_AMP_GAIN_MUTE, 0x2010)
etc.
> > 
> > Re: (0x1c, 0x320) is not amp gain register, but vendor defined 
> > register as rx control register. Use AC_VERB_SET_AMP_GAIN_MUTE will 
> > confused. It's similar to 0x4f0 and 0xca0.
>
> Ah interesting.  But the verb is actually seen as
AC_VERB_SET_AMP_GAIN_MUTE -- although the resultant bits seem invalid.
>
> HD-audio combines the verb and the value into 20 bits, e.g. (0x320,
> 0x10) is composed as 0x32010, and (0x3b0, 0xe10) is 0x3be10.
> 0x3xx is translated as SET_AMP_GAIN_MUTE, but in your case, 0x32010 leaves
0 to both the input/output bits (bits 14 and 15), which makes it as invalid.
>
> 0x3be10 is another invalid verb, which sets SET_AMP_GAIN_MUTE with OUTPUT,
but it sets both LEFT and RIGHT, and passes a high index (14).
>
> And, what actually (0x4f0, 0x0eb) does?  It's composed as 0x4f0eb, and in
this case, it's a valid verb (SET_PROC_COEF + 0xf0eb).  But COEF is
vendor-specific, so it can be translated in everything the chip wants.
>
> So, if those verbs are vendor-specific ones, please define them and/or
give proper comments to explain what they do for each.

Re: In cx8070 and sn6140, (0x1c, 0x320, 0x010) is used to set micbiasd
output current comparator 
threshold from 66% to 55%. (0x1c, 0x3b0, 0xe10) is used to set OFF voltage
for DFET from -1.2V to 
-0.8V, set headset micbias registor value adjustment trim from 2.2K ohms to
2.0K ohms.
(0x1c, 0x4f0, 0x0eb) is used to set headset detect debounce time, this will
impact detection time.
As it needs to be adjusted according to the project, i think it set to bios
may better. So I will remove
the setting from this patch.

> > Also, it's still not clear what if other nodes are used for headphone 
> > and mic pins -- or when either only headphone or only mic is present.
> > A rare case, but we need to cover.
> > 
> > Re: in cx8070 and sn6140, only 0x16 and 0x19 can be used together as 
> > headset. Other nodes can be used separately as headphones or 
> > microphones, but not as headset, so their configuration will not 
> > interfere with the type detection of headset.
>
> OK, then explain this in comments, too (that we blindly assume those
pins).

Re: OK, I will add the description to patch.

> > > +static void cx_process_headset_plugin(struct hda_codec *codec) {
> > > +	unsigned int val;
> > > +	unsigned int count = 0;
> > > +
> > > +	/* Wait headset detect done. */
> > > +	do {
> > > +		val = snd_hda_codec_read(codec, 0x1c, 0, 0xca0, 0x0);
> > 
> > Use the verb: AC_VERB_GET_PROC_COEF, 0xa000.
> > At best, define the COEF values 0xa000 and 0xb000, and the 
> > corresponding value bits, too.
> > 
> > Re: (0x1c, 0xca0) is not COEF register, but vendor defined register as 
> > jacksense register.
> > 
> > > +static void cx_update_headset_mic_vref(struct hda_codec *codec, 
> > > +unsigned int res) {
> > > +	unsigned int phone_present, mic_persent, phone_tag, mic_tag;
> > > +	struct conexant_spec *spec = codec->spec;
> > > +
> > > +	/* In cx8070 and sn6140, headset is fixed to use node 16 and node
> > 19.
> > 
> > Is it really guaranteed?  IMO, we should check the pin configs 
> > beforehand at the parsing time.
> > 
> > Re: in cx8070 and sn6140, only 0x16 and 0x19 can be used together as 
> > headset. The node 16 can only be config to headphone or disable, The 
> > node 19 can only be config to microphone or disable. Only node 16 and 
> > node 19 both enable, the patch will process.
>
> Then we still might need a check for the condition?

Re: because the node 16 and node 19 can only be config to headphone and
microphone, as describe 
before, whether node 16 and node 19 enable, the patch can process, so I
think it's no need to check
their pin configs.

Best Regards
Bo Liu