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

[José Relvas <josemonsantorelvas@xxxxxxxxx>]

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.

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