Re: [PATCH] Apply headset jack fixup for alc287 thinkpads

[Takashi Iwai <tiwai@xxxxxxx>]

On Wed, 31 Jan 2024 12:45:47 +0100,
José Relvas wrote:
> 
> Oops, turns out there's a non-bass variant of this quirk: ALC285_FIXUP_THINKPAD_NO_BASS_SPK_HEADSET_JACK
> I've made a V2 of this patch using it - it's much less invasive.

Yeah, I saw it, and it looks better.


thanks,

Takashi

> I'm in agreement. If the quirk's name says it targets bass systems, then that'd imply a non-bass version is available, which should prevent future confusion.
> 
> On Wed, Jan 31, 2024 at 12:39:00PM +0100, Takashi Iwai wrote:
> > On Wed, 31 Jan 2024 12:09:49 +0100,
> > José Relvas wrote:
> > > 
> > > Thanks for the feedback! So if my understanding is correct, the headset jack quirk is currently *only* applied to models with bass speakers?
> > 
> > AFAIK, it looks yes; at least the majority of machines are actually
> > with bass speakers.
> > 
> > > bass thinkpad -> headset thinkpad quirk -> bass thinkpad quirk (both quirks applied)
> > > normal thinkpad -X> headset thinkpad quirk -X> bass thinkpad quirk (neither quirks applied) 
> > >
> > > I'll go back to the drawing board, just making sure I'm not missing anything.
> > 
> > Maybe it'd be helpful to rename the current one to be more explicit
> > with bass speakers at first :)
> > 
> > 
> > Takashi
> > 
> > > José Relvas
> > > 
> > > On Wed, Jan 31, 2024 at 11:49:28AM +0100, Takashi Iwai wrote:
> > > > On Wed, 31 Jan 2024 11:28:15 +0100,
> > > > José Relvas wrote:
> > > > > 
> > > > > Thanks for the reply!
> > > > > 
> > > > > Sorry about the misformatted patch. I did send in a second one through git-send-email. Is it not correct either? 
> > > > > 
> > > > > Anyways, the reason I'm editing the existing quirks is mostly because ALC285_FIXUP_THINKPAD_HEADSET_JACK was actually wrong.
> > > > > It was actually always linking to the X1 gen 7 quirk, which has some kcontrol changes only meant for a few models with bass speakers.
> > > > 
> > > > Yes, and that's the intentional behavior.
> > > > There is the match with the pin table, not only the explicit PCI
> > > > SSID, and those are with bass speakers.
> > > > 
> > > > > It looks like this: 
> > > > > 
> > > > > /* Quirk for Thinkpad X1 7th and 8th Gen
> > > > >  * The following fixed routing needed
> > > > >  * DAC1 (NID 0x02) -> Speaker (NID 0x14); some eq applied secretly
> > > > >  * DAC2 (NID 0x03) -> Bass (NID 0x17) & Headphone (NID 0x21); sharing a DAC
> > > > >  * DAC3 (NID 0x06) -> Unused, due to the lack of volume amp
> > > > > */
> > > > > static void alc285_fixup_thinkpad_x1_gen7(struct hda_codec *codec,
> > > > > 					  const struct hda_fixup *fix, int action)
> > > > > {
> > > > > 	static const hda_nid_t conn[] = { 0x02, 0x03 }; /* exclude 0x06 */
> > > > > 	static const hda_nid_t preferred_pairs[] = {
> > > > > 		0x14, 0x02, 0x17, 0x03, 0x21, 0x03, 0
> > > > > 	};
> > > > > 	struct alc_spec *spec = codec->spec;
> > > > > 
> > > > > 	switch (action) {
> > > > > 	case HDA_FIXUP_ACT_PRE_PROBE:
> > > > > 		snd_hda_override_conn_list(codec, 0x17, ARRAY_SIZE(conn), conn);
> > > > > 		spec->gen.preferred_dacs = preferred_pairs;
> > > > > 		break;
> > > > > 	case HDA_FIXUP_ACT_BUILD:
> > > > > 		/* The generic parser creates somewhat unintuitive volume ctls
> > > > > 		 * with the fixed routing above, and the shared DAC2 may be
> > > > > 		 * confusing for PA.
> > > > > 		 * Rename those to unique names so that PA doesn't touch them
> > > > > 		 * and use only Master volume.
> > > > > 		 */
> > > > > 		rename_ctl(codec, "Front Playback Volume", "DAC1 Playback Volume");
> > > > > 		rename_ctl(codec, "Bass Speaker Playback Volume", "DAC2 Playback Volume");
> > > > > 		break;
> > > > > 	}
> > > > > }
> > > > > 
> > > > > On second review, this does break some routing with older alc285, as the speaker and headphone NIDs are still 0x02 and 0x03.
> > > > > Maybe cloning the x1 gen 7 fixup, but without the renamed ctls, is the more appropriate solution here? Let me know what your thoughts are.
> > > > 
> > > > I believe it's safer to make a specific quirk for X7 gen7.
> > > > There are too many dependencies on the existing chains.
> > > > 
> > > > 
> > > > Takashi