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

[Takashi Iwai <tiwai@xxxxxxx>]

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