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

[刘博 <bo.liu@senarytech.com>]

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