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 8B99BC433E2 for ; Tue, 1 Sep 2020 15:03:01 +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 12467206CD for ; Tue, 1 Sep 2020 15:03:01 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=alsa-project.org header.i=@alsa-project.org header.b="M2sNfRR1" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 12467206CD 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 7374E17BB; Tue, 1 Sep 2020 17:02:09 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz 7374E17BB DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1598972579; bh=2sn+LK0S8SKkNzXegWVAmwmL7+Fsh4MLgHvT/o+ZGxs=; h=Date:From:To:Subject:In-Reply-To:References:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=M2sNfRR1gUb4dLmw+yssmxpHrgTUR0+NbMYJwcyci7wJuig0fVcmLO1+98F2rq4ds Oqx420CWltkpJ/0+eq4WWRMIBwaqN7S0n5hsNuvike1biPCp9FYefxZv+YYv77fgjN qIaceN7m4VdtGphEU2DSRgSJmfwioZ1FUOFxLoYY= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id 04BD6F801EB; Tue, 1 Sep 2020 17:02:09 +0200 (CEST) Received: by alsa1.perex.cz (Postfix, from userid 50401) id CA2E4F80217; Tue, 1 Sep 2020 17:02:07 +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 64D2EF801EB for ; Tue, 1 Sep 2020 17:01:56 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz 64D2EF801EB 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 A76E4ACC3; Tue, 1 Sep 2020 15:01:56 +0000 (UTC) Date: Tue, 01 Sep 2020 17:01:55 +0200 Message-ID: From: Takashi Iwai To: Jaroslav Kysela Subject: Re: [PATCH] ALSA: hda/realtek - Add control fixup for Lenovo Thinkpad X1 Carbon 7th In-Reply-To: <32b649db-ede6-d3ea-a963-d0bac331e4b4@perex.cz> 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 , Takashi Iwai , Hui Wang , Benjamin Poirier , 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 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. BTW, if this approach with the tied kctls sharing the same amp is acceptable, we may apply it also for the existing case; e.g. the generic parser already creates a bit weird kctl like "Headphone+LO" or "Speaker+LO". Those can be re-implemented with two tied kctls, too. thanks, Takashi