Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 03 Jul 2023 17:13:31 +0200
Message-ID: <87o7kt3u5g.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     Shenghao Ding <13916275206@139.com>
Cc:     broonie@kernel.org, devicetree@vger.kernel.org, robh+dt@kernel.org,
        lgirdwood@gmail.com, perex@perex.cz,
        pierre-louis.bossart@linux.intel.com, kevin-lu@ti.com,
        shenghao-ding@ti.com, alsa-devel@alsa-project.org,
        linux-kernel@vger.kernel.org, x1077012@ti.com, peeyush@ti.com,
        navada@ti.com
Subject: Re: [PATCH v1 1/3] ALSA: hda/tas2781: Add tas2781 HDA driver
In-Reply-To: <20230702081857.799693-1-13916275206@139.com>
References: <20230702081857.799693-1-13916275206@139.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) Emacs/27.2 Mule/6.0
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Sun, 02 Jul 2023 10:18:55 +0200,
Shenghao Ding wrote:
> 
> Integrate tas2781 configs for Lenovo Laptops. All of the tas2781s in the
> laptop will be aggregated as one speaker. The code support realtek as the
> primary codec.

It's not only that -- you changed the struct name used in the code,
too.  Please describe it, too.

> @@ -5883,7 +5883,7 @@ static void alc_fixup_headset_mode_alc255_no_hp_mic(struct hda_codec *codec,
>  		struct alc_spec *spec = codec->spec;
>  		spec->parse_flags |= HDA_PINCFG_HEADSET_MIC;
>  		alc255_set_default_jack_type(codec);
> -	} 
> +	}
>  	else
>  		alc_fixup_headset_mode(codec, fix, action);
>  }

This change is irrelevant with your code, and should be fixed
individually.  Please drop the hunk.

> @@ -9255,6 +9317,12 @@ static const struct hda_fixup alc269_fixups[] = {
>  		.chained = true,
>  		.chain_id = ALC255_FIXUP_DELL1_MIC_NO_PRESENCE,
>  	},
> +	[ALC287_FIXUP_TAS2781_I2C] = {
> +		.type = HDA_FIXUP_FUNC,
> +		.v.func = tas2781_fixup_i2c,
> +		.chained = true,
> +		.chain_id = ALC269_FIXUP_THINKPAD_ACPI,
> +	},

So this is supposed to be Lenovo-specific, and maybe better to rename,
e.g. ALC287_FIXUP_LENOVO_TAS2781_I2C or such?


> @@ -9813,6 +9881,33 @@ static const struct snd_pci_quirk alc269_fixup_tbl[] = {
>  	SND_PCI_QUIRK(0x17aa, 0x3853, "Lenovo Yoga 7 15ITL5", ALC287_FIXUP_YOGA7_14ITL_SPEAKERS),
>  	SND_PCI_QUIRK(0x17aa, 0x3855, "Legion 7 16ITHG6", ALC287_FIXUP_LEGION_16ITHG6),
>  	SND_PCI_QUIRK(0x17aa, 0x3869, "Lenovo Yoga7 14IAL7", ALC287_FIXUP_YOGA9_14IAP7_BASS_SPK_PIN),
> +	SND_PCI_QUIRK(0x17aa, 0x387d, "Yoga S780-16 pro Quad AAC",
> +		ALC287_FIXUP_TAS2781_I2C),
> +	SND_PCI_QUIRK(0x17aa, 0x387e, "Yoga S780-16 pro Quad YC",
> +		ALC287_FIXUP_TAS2781_I2C),
> +	SND_PCI_QUIRK(0x17aa, 0x3881, "YB9 dual powe mode2 YC",
> +		ALC287_FIXUP_TAS2781_I2C),
> +	SND_PCI_QUIRK(0x17aa, 0x3884, "Y780 YG DUAL",
> +		ALC287_FIXUP_TAS2781_I2C),
> +	SND_PCI_QUIRK(0x17aa, 0x3886, "Y780 VECO DUAL",
> +		ALC287_FIXUP_TAS2781_I2C),
> +	SND_PCI_QUIRK(0x17aa, 0x38a7, "Y780P AMD YG dual",
> +		ALC287_FIXUP_TAS2781_I2C),
> +	SND_PCI_QUIRK(0x17aa, 0x38a8, "Y780P AMD VECO dual",
> +		ALC287_FIXUP_TAS2781_I2C),
> +	SND_PCI_QUIRK(0x17aa, 0x38ba, "Yoga S780-14.5 Air AMD quad YC",
> +		ALC287_FIXUP_TAS2781_I2C),
> +	SND_PCI_QUIRK(0x17aa, 0x38bb, "Yoga S780-14.5 Air AMD quad AAC",
> +		ALC287_FIXUP_TAS2781_I2C),
> +	SND_PCI_QUIRK(0x17aa, 0x38be, "Yoga S980-14.5 proX YC Dual",
> +		ALC287_FIXUP_TAS2781_I2C),
> +	SND_PCI_QUIRK(0x17aa, 0x38bf, "Yoga S980-14.5 proX LX Dual",
> +		ALC287_FIXUP_TAS2781_I2C),
> +	SND_PCI_QUIRK(0x17aa, 0x38c3, "Y980 DUAL", ALC287_FIXUP_TAS2781_I2C),
> +	SND_PCI_QUIRK(0x17aa, 0x38cb, "Y790 YG DUAL",
> +		ALC287_FIXUP_TAS2781_I2C),
> +	SND_PCI_QUIRK(0x17aa, 0x38cd, "Y790 VECO DUAL",
> +		ALC287_FIXUP_TAS2781_I2C),

Please keep one entry per line.  Let's ignore the checkpatch
complaints.

> @@ -10728,6 +10823,17 @@ static int patch_alc269(struct hda_codec *codec)
>  		codec->fixup_id = HDA_FIXUP_ID_NOT_SET;
>  	}
>  
> +	/* FIXME: Laptop 0x17aa38be will get the wrong fixup_id and
> +	 * enter into the wrong entry.
> +	 * Correct the wrong entry.
> +	 */
> +	if (codec->fixup_id == ALC287_FIXUP_YOGA7_14ITL_SPEAKERS &&
> +		codec->core.vendor_id == 0x10ec0287 &&
> +		codec->core.subsystem_id == 0x17aa38be) {
> +		codec_dbg(codec, "Clear wrong fixup for 17aa38be\n");
> +		codec->fixup_id = ALC287_FIXUP_TAS2781_I2C;
> +	}

Why this is needed at all?  IOW, which entry causes this wrong
attribute?


thanks,

Takashi