Received-SPF: pass (google.com: domain of linux-kernel+bounces-14543-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Tue, 02 Jan 2024 16:11:53 +0100
Message-ID: <87wmsr7osm.wl-tiwai@suse.de>
From: Takashi Iwai <tiwai@suse.de>
To: bo liu <bo.liu@senarytech.com>
Cc: 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: <20240102060457.4917-1-bo.liu@senarytech.com>
References: <20240102060457.4917-1-bo.liu@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=US-ASCII

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