Received-SPF: pass (google.com: domain of linux-kernel+bounces-19200-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Mon, 08 Jan 2024 09:42:38 +0100
Message-ID: <87frz8i5c1.wl-tiwai@suse.de>
From: Takashi Iwai <tiwai@suse.de>
To: =?GB2312?B?wfWyqQ==?= <bo.liu@senarytech.com>
Cc: "'Takashi Iwai'" <tiwai@suse.de>,
	<perex@perex.cz>,
	<tiwai@suse.com>,
	<linux-sound@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] ALSA: hda/conexant: Fix headset auto detect fail in cx8070 and SN6140
In-Reply-To: <002401da41e2$f159f5f0$d40de1d0$@senarytech.com>
References: <002401da41e2$f159f5f0$d40de1d0$@senarytech.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) Emacs/27.2 Mule/6.0
Precedence: bulk
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=GB2312
Content-Transfer-Encoding: 8bit

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