From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.0 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 18E89C433E2 for ; Wed, 2 Sep 2020 08:29:45 +0000 (UTC) Received: from alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 94FDA20829 for ; Wed, 2 Sep 2020 08:29:44 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=alsa-project.org header.i=@alsa-project.org header.b="VdbHHjmF" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 94FDA20829 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=suse.de Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=alsa-devel-bounces@alsa-project.org Received: from alsa1.perex.cz (alsa1.perex.cz [207.180.221.201]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa0.perex.cz (Postfix) with ESMTPS id 1018C1759; Wed, 2 Sep 2020 10:28:53 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz 1018C1759 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1599035383; bh=f7gzV8aG8TdcakKWUc+ljuDY+K22vfuHvaouRur0hSU=; h=Date:From:To:Subject:In-Reply-To:References:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=VdbHHjmFDUKPMlDpMQIg7XWx8doNN+K1kgzXTTO/+//6QPcetmRa8CyeABCrvyD/W z8H8W0uiulMBi1K7k7tDAK8hxs8JPmt/w24JI38W3b2Orku9KXrCvfw4L8KDV1cn4x lcw2htdr7B9+UrxeoVeXrYFvl7JLJBTawh02p/Tk= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id 8FB3DF80212; Wed, 2 Sep 2020 10:28:52 +0200 (CEST) Received: by alsa1.perex.cz (Postfix, from userid 50401) id 6665BF8024A; Wed, 2 Sep 2020 10:28:49 +0200 (CEST) Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id EC75FF800F3 for ; Wed, 2 Sep 2020 10:28:36 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz EC75FF800F3 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 34995B1DD; Wed, 2 Sep 2020 08:28:35 +0000 (UTC) Date: Wed, 02 Sep 2020 10:28:33 +0200 Message-ID: From: Takashi Iwai To: Benjamin Poirier Subject: Re: [PATCH] ALSA: hda/realtek - Add control fixup for Lenovo Thinkpad X1 Carbon 7th In-Reply-To: References: <20200829112746.3118-1-benjamin.poirier@gmail.com> <32b649db-ede6-d3ea-a963-d0bac331e4b4@perex.cz> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Cc: alsa-devel@alsa-project.org, Kailang Yang , Hui Wang , Kai-Heng Feng , Vincent Bernat , Even Brenden , Mark Pearson X-BeenThere: alsa-devel@alsa-project.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: "Alsa-devel" On Tue, 01 Sep 2020 17:01:55 +0200, Takashi Iwai wrote: > > On Tue, 01 Sep 2020 15:52:09 +0200, > Jaroslav Kysela wrote: > > > > > +} > > > + > > > +static int tpx1_dual_speaker_vol_put(struct snd_kcontrol *kcontrol, > > > + struct snd_ctl_elem_value *ucontrol) > > > +{ > > > + struct tpx1_dual_speaker *speaker_priv = snd_kcontrol_chip(kcontrol); > > > + int err; > > > + > > > + /* Control tweeter volume */ > > > + err = speaker_priv->underlying.put(&speaker_priv->underlying, > > > + ucontrol); > > > + if (err < 0) > > > + return err; > > > + > > > + /* Control woofer volume (shared with headphone) */ > > > + err = speaker_priv->hp_vol.put(&speaker_priv->hp_vol, ucontrol); > > > + if (err < 0) > > > + return err; > > > + > > > + snd_ctl_notify(speaker_priv->codec->card, SNDRV_CTL_EVENT_MASK_VALUE, > > > + &speaker_priv->hp_vol.id); > > > + return err; > > > +} > > > + > > > +static int tpx1_dual_speaker_vol_tlv(struct snd_kcontrol *kcontrol, > > > + int op_flag, unsigned int size, > > > + unsigned int __user *tlv) > > > +{ > > > + struct tpx1_dual_speaker *speaker_priv = snd_kcontrol_chip(kcontrol); > > > + > > > + return speaker_priv->underlying.tlv.c(&speaker_priv->underlying, > > > + op_flag, size, tlv); > > > +} > > > + > > > +static void tpx1_dual_speaker_vol_free(struct snd_kcontrol *kcontrol) > > > +{ > > > + struct tpx1_dual_speaker *speaker_priv = snd_kcontrol_chip(kcontrol); > > > + > > > + if (speaker_priv->underlying.private_free) > > > + speaker_priv->underlying.private_free( > > > + &speaker_priv->underlying); > > > + kfree(speaker_priv); > > > +} > > > + > > > +static int tpx1_dual_override_speaker_vol(struct hda_codec *codec, > > > + struct snd_kcontrol *speaker_vol, > > > + struct snd_kcontrol *hp_vol) > > > +{ > > > + struct tpx1_dual_speaker *speaker_priv; > > > + > > > + speaker_priv = kmalloc(sizeof(struct tpx1_dual_speaker), GFP_KERNEL); > > > + if (!speaker_priv) > > > + return -ENOMEM; > > > + speaker_priv->codec = codec; > > > + memcpy(&speaker_priv->underlying, speaker_vol, > > > + sizeof(struct snd_kcontrol)); > > > + memcpy(&speaker_priv->hp_vol, hp_vol, sizeof(struct snd_kcontrol)); > > > > This is a bit clumsy part. It would be probably nice to have a helper in the > > upper control code to clone the original control safely. Takashi? > > The purpose of those is to have two controls managing the same amp and > get notified with each other at other's update, right? The missing > piece is only about notification, and that could be done in the common > code somehow, too. For example, we can reduce the 16bit usage of NID > to 8 bit embedded in private_value, then we'll have 8 bit space for > storing the coupled kctl nid or some other tag for notification. > > However, the approach by this patch has minor problems, as far as I > see: > > - The notification may be issued unnecessarily for Master volume > change; > when you change Master volume, it'll notify Headphone and/or Speaker > as well although those (virtual) values aren't changed. > It's a minor issue and can be almost negligible, though. > > - The volume status depends on the operation order; > e.g. when switching the output from speaker to headphone, at first > mute and set volume zero Speaker, then unmute/raise Headphone. > But if we do unmute/raise Headphone at first, then mute/zero > Speaker, the headphone output will be also zero volume out of > sudden. > It seems that PA does in the former way, so the current approach > might work practically, but it can be a pitfall in some corner > cases. 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 + * 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; + } +} + static void alc233_alc662_fixup_lenovo_dual_codecs(struct hda_codec *codec, const struct hda_fixup *fix, int action) @@ -6135,6 +6168,7 @@ enum { ALC289_FIXUP_DUAL_SPK, ALC294_FIXUP_SPK2_TO_DAC1, ALC294_FIXUP_ASUS_DUAL_SPK, + ALC285_FIXUP_THINKPAD_X1_GEN7, ALC285_FIXUP_THINKPAD_HEADSET_JACK, ALC294_FIXUP_ASUS_HPE, ALC294_FIXUP_ASUS_COEF_1B, @@ -7280,11 +7314,17 @@ static const struct hda_fixup alc269_fixups[] = { .chained = true, .chain_id = ALC294_FIXUP_SPK2_TO_DAC1 }, + [ALC285_FIXUP_THINKPAD_X1_GEN7] = { + .type = HDA_FIXUP_FUNC, + .v.func = alc285_fixup_thinkpad_x1_gen7, + .chained = true, + .chain_id = ALC269_FIXUP_THINKPAD_ACPI + }, [ALC285_FIXUP_THINKPAD_HEADSET_JACK] = { .type = HDA_FIXUP_FUNC, .v.func = alc_fixup_headset_jack, .chained = true, - .chain_id = ALC285_FIXUP_SPEAKER2_TO_DAC1 + .chain_id = ALC285_FIXUP_THINKPAD_X1_GEN7 }, [ALC294_FIXUP_ASUS_HPE] = { .type = HDA_FIXUP_VERBS,