linux-kernel - Re: [PATCH v1 1/3] ALSA: hda/tas2781: Add tas2781 HDA driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87o7kt3u5g.wl-tiwai@suse.de>
Date:   Mon, 03 Jul 2023 17:13:31 +0200
From:   Takashi Iwai <tiwai@...e.de>
To:     Shenghao Ding <13916275206@....com>
Cc:     broonie@...nel.org, devicetree@...r.kernel.org, robh+dt@...nel.org,
        lgirdwood@...il.com, perex@...ex.cz,
        pierre-louis.bossart@...ux.intel.com, kevin-lu@...com,
        shenghao-ding@...com, alsa-devel@...a-project.org,
        linux-kernel@...r.kernel.org, x1077012@...com, peeyush@...com,
        navada@...com
Subject: Re: [PATCH v1 1/3] ALSA: hda/tas2781: Add tas2781 HDA driver

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