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

[José Relvas <josemonsantorelvas@xxxxxxxxx>]

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.

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.

Sorry for the confusion.
José Relvas

On Wed, Jan 31, 2024 at 08:20:29AM +0100, Takashi Iwai wrote:
> On Tue, 30 Jan 2024 20:25:36 +0100,
> José Relvas wrote:
> > 
> > Thinkpads have a fixup for alc285 which enables headset media buttons, but not for alc287.
> > This patch removes the chain to the X1 gen 7 fixup in the alc285 headset fixup, making it applicable for models without bass speakers.
> > It then chains said headset fixup with every alc287 thinkpad fixup, making headset media buttons work for these laptops.
> > 
> > Signed-off-by: José Relvas <josemonsantorelvas@xxxxxxxxx>
> 
> Thanks for the patch.  But the changes look too intrusive.  We don't
> want to change the behavior of the existing quirk entries
> unnecessarily.  Your change will break if anyone explicitly uses a
> specific model option, for example.
> So, please rather create a new entry for handling the headset jack and
> use it (maybe better with some comments).
> 
> Also the patch isn't cleanly applicable.  Your MUA broke tabs and
> spaces.  Please fix the setup, or use git-send-email.
> 
> 
> thanks,
> 
> Takashi