Re: [PATCH] ALSA: hda/realtek - Add control fixup for Lenovo Thinkpad X1 Carbon 7th

[Benjamin Poirier <benjamin.poirier@gmail.com>]

On 2020-09-02 10:28 +0200, Takashi Iwai wrote:
[...]
> 
> After testing the actual patch with hda-emu, I noticed that the
> Speaker volume changes the volume of both speakers, and it's also tied
> with Headphone, too.  That said, basically this is de facto Master
> volume, and we basically don't need to control the individual amp.
> 
> If that's the case, the following patch may work instead (checked only
> via hda-emu).  It applies the workaround to fix the routing, then
> rename the half-working volume controls that aren't touched by PA.  If
> user definitely needs to adjust the individual amp, they can still
> change the renamed kctl (DAC1 and DAC2), but this must be a rare
> requirement.
> 
> 
> Takashi
> 
> --- a/sound/pci/hda/patch_realtek.c
> +++ b/sound/pci/hda/patch_realtek.c
> @@ -5867,6 +5867,39 @@ static void alc275_fixup_gpio4_off(struct hda_codec *codec,
>  	}
>  }
>  
> +/* 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 don't touch them
                                                           ^ doesn't
> +		 * 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;
> +	}
> +}
> +
[...]

I've tested that the following all work:
* DAC1/DAC2 volume controls in all 4 speakers/headphones
* 3 mute controls
* mute led
* plugging/unplugging headphones while PA is running switches outputs as
  expected
* loud volume, of course

... as well as some of the corner cases that I had tripped on when
working on my patches:
* headphone sound "wobble" due to the "secret" equalizer on output 0x02:
  not present with this patch; connection 0x03 is used for headphones as
  expected from the code
* resume after s3 suspend (which resets the codec): desired connections
  are still used

Everything looks good.
Your patch is simple yet effective; I'm humbled, thank you. You're a
true HDA ninja!

Tested-by: Benjamin Poirier <benjamin.poirier@gmail.com>