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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 79C95C00140 for ; Tue, 26 Jul 2022 14:49:23 +0000 (UTC) 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 7357815CC; Tue, 26 Jul 2022 16:48:31 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz 7357815CC DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1658846961; bh=AVdLXZczB78wzjYne0LzeumnXgq283J0Ar7FnM/OkOY=; h=Date:Subject:To:References:From:In-Reply-To:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=l2e8x7S3+Zwf0GUKQMJNfREnfkmSCDEyTWCWgvB0ZguDjYibejibZseiotbXC5q0I DvLhq1xwtb0O95xW5LjGbeT6HhQwkhn5cWE7Ja9tdcsXMWqvjdkn4MK9ulBa9JCA3a XW6Xs3XnlWu44JylU/C1RdopwSg5gP5mqTi0e9ck= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id 12517F80543; Tue, 26 Jul 2022 16:48:01 +0200 (CEST) Received: by alsa1.perex.cz (Postfix, from userid 50401) id 9B5D7F80153; Tue, 26 Jul 2022 16:48:00 +0200 (CEST) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id C2E19F80153 for ; Tue, 26 Jul 2022 16:47:53 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz C2E19F80153 Authentication-Results: alsa1.perex.cz; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b="A8Kt9q8u" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1658846875; x=1690382875; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=AVdLXZczB78wzjYne0LzeumnXgq283J0Ar7FnM/OkOY=; b=A8Kt9q8uRnWTnOZhNK28y+GPIPFPWgfEXR7A7RxeCtmu16dZvWm/9vFP aGUcerk2Z0lcnr2PofOUMCl3upukLbG8bh7ftZEPhhQwcucNi0oJR4ONW 0KCUR9rAxcIJuO32WJJIP3y0oUrlAV3sUu5OA/k0X7A99TRG2VsUeVVuK 7tWQRgcKdrwMPsC/v/Dod9NK8iS/7IWe13WSqt6CYsxMIrTQ2Ak6uB0C1 7qMm5VcqLyUz1ZjlKZUrObsXcHKoGnf5hpCLL+rs0RAK2Pg+rmvB/Oemf KdJsSwj25nzAEcAqbcLRkiv+9ipswVELfRYmqWwTqv1+CyDQCl65eEZzU A==; X-IronPort-AV: E=McAfee;i="6400,9594,10420"; a="289160580" X-IronPort-AV: E=Sophos;i="5.93,193,1654585200"; d="scan'208";a="289160580" Received: from orsmga007.jf.intel.com ([10.7.209.58]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 26 Jul 2022 07:47:47 -0700 X-IronPort-AV: E=Sophos;i="5.93,193,1654585200"; d="scan'208";a="597047566" Received: from adamreed-mobl.amr.corp.intel.com (HELO [10.212.70.145]) ([10.212.70.145]) by orsmga007-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 26 Jul 2022 07:47:46 -0700 Message-ID: <013c0854-5b8e-6968-1ab2-88f2d0b142a0@linux.intel.com> Date: Tue, 26 Jul 2022 09:34:05 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Firefox/91.0 Thunderbird/91.11.0 Subject: Re: [PATCH v1] ASoC: Intel: cirrus-common: Use UID to map correct amp to prefix Content-Language: en-US To: Stefan Binding , Mark Brown , Liam Girdwood , Brent Lu , xliu References: <20220726134634.2842185-1-sbinding@opensource.cirrus.com> From: Pierre-Louis Bossart In-Reply-To: <20220726134634.2842185-1-sbinding@opensource.cirrus.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Cc: Vitaly Rodionov , patches@opensource.cirrus.com, alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org 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 7/26/22 08:46, Stefan Binding wrote: > Since the order of the amps in the ACPI determines the device name, > and the ACPI order may change depending on hardware configuration, > use UID to dynamically compute the dai links, allowing dynamic > assignment of the name_prefix. This is interesting, I didn't even know what this _UID thingy could be used for. ACPI is the gift that keeps on giving after 30 years, eh? I think you should add an explanation on what this _UID is, nothing says it actually needs to be an integer, I see e.g. this sort of 'id' in various DSDT Name (_UID, Zero) // _UID: Unique ID Name (_UID, 0x05) // _UID: Unique ID Name (_UID, "SerialIoUart0") // _UID: Unique ID Name (_UID, "PCHRESV") // _UID: Unique ID Name (_UID, "IoTraps") // _UID: Unique ID Name (_UID, "SADDLESTRING") // _UID: Unique ID and my favorite Name (_UID, "TestDev") // _UID: Unique ID > /* > * Mapping between ACPI instance id and speaker position. > - * > - * Four speakers: > - * 0: Tweeter left, 1: Woofer left > - * 2: Tweeter right, 3: Woofer right > */ > -static struct snd_soc_codec_conf cs35l41_codec_conf[] = { > - { > - .dlc = COMP_CODEC_CONF(CS35L41_DEV0_NAME), > - .name_prefix = "TL", > - }, > - { > - .dlc = COMP_CODEC_CONF(CS35L41_DEV1_NAME), > - .name_prefix = "WL", > - }, > - { > - .dlc = COMP_CODEC_CONF(CS35L41_DEV2_NAME), > - .name_prefix = "TR", > - }, > - { > - .dlc = COMP_CODEC_CONF(CS35L41_DEV3_NAME), > - .name_prefix = "WR", > - }, > -}; > +static struct snd_soc_codec_conf cs35l41_codec_conf[CS35L41_MAX_AMPS]; > > static int cs35l41_init(struct snd_soc_pcm_runtime *rtd) > { > @@ -117,10 +82,10 @@ static int cs35l41_init(struct snd_soc_pcm_runtime *rtd) > static const struct { > unsigned int rx[2]; > } cs35l41_channel_map[] = { > - {.rx = {0, 1}}, /* TL */ > {.rx = {0, 1}}, /* WL */ > - {.rx = {1, 0}}, /* TR */ > {.rx = {1, 0}}, /* WR */ > + {.rx = {0, 1}}, /* TL */ > + {.rx = {1, 0}}, /* TR */ > }; > > static int cs35l41_hw_params(struct snd_pcm_substream *substream, > @@ -175,8 +140,32 @@ static const struct snd_soc_ops cs35l41_ops = { > .hw_params = cs35l41_hw_params, > }; > > +static const char * const cs35l41_name_prefixes[] = { "WL", "WR", "TL", "TR" }; > + > +static const char * const cs35l41_uid_strings[] = { "0", "1", "2", "3" }; I must admit not understanding why you changed the order. I vaguely recall Brent Lu added this on TL, WL, TR, WR order on purpose and that it matches the order in the SOF topology. Brent, can you please comment on this? I don't really care about the order selected, just want to make sure we don't introduce a channel swap with what the firmware does. > +static void cs35l41_compute_codec_conf(void) > +{ > + int uid; > + struct acpi_device *adev; > + struct device *physdev; > + > + for (uid = 0; uid < CS35L41_MAX_AMPS; uid++) { > + adev = acpi_dev_get_first_match_dev(CS35L41_HID, cs35l41_uid_strings[uid], -1); > + if (!adev) > + return; shouldn't you log an error or something telling the user that their DSDT configuration is incorrect? If I understand the code above, is the expectation that the UID expected in the DSDT should be: Name (_UID, "0") // _UID: Unique ID for WL Name (_UID, "1") // _UID: Unique ID for WR Name (_UID, "2") // _UID: Unique ID for TL Name (_UID, "3") // _UID: Unique ID for TR Is yes that's worthy of a comment for future generations. > + physdev = get_device(acpi_get_first_physical_node(adev)); > + cs35l41_components[uid].name = dev_name(physdev); > + cs35l41_components[uid].dai_name = CS35L41_CODEC_DAI; > + cs35l41_codec_conf[uid].dlc.name = dev_name(physdev); > + cs35l41_codec_conf[uid].name_prefix = cs35l41_name_prefixes[uid]; > + acpi_dev_put(adev); > + } > +} > + > void cs35l41_set_dai_link(struct snd_soc_dai_link *link) > { > + cs35l41_compute_codec_conf(); > link->codecs = cs35l41_components; > link->num_codecs = ARRAY_SIZE(cs35l41_components); > link->init = cs35l41_init; 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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id BDF31C00140 for ; Tue, 26 Jul 2022 14:47:55 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233668AbiGZOry (ORCPT ); Tue, 26 Jul 2022 10:47:54 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54294 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S239445AbiGZOru (ORCPT ); Tue, 26 Jul 2022 10:47:50 -0400 Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1B22919037 for ; Tue, 26 Jul 2022 07:47:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1658846868; x=1690382868; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=AVdLXZczB78wzjYne0LzeumnXgq283J0Ar7FnM/OkOY=; b=JrJx4XRUU4kj6ryLQZwGweBpZLSRlfU8DQLBtP26mCMRf+MIjaWlMvXW s4dRhjkq9pwMWUwN32hvfS6pRakmxQqZfiwkKOEqud1mpQlMF4bKfVw3E DjZnG/b+Pt4FRkH0eLRe6MjD5RGGQhombq8Ppj7dD9N01JmBJXGieLMmM bbmeStlUUN+FN9kIJabnWt6glnB5zuw8+CGHJhHD/sBCOLCVd9LyZh4hr a+mahSA4pqxcaF0r5L/8+fAcohBme9MQwJ6CTem2DV1VZQlqPbAW6OUkN nT980Jwui+bwKaYcct4WiKOH3WkETD8hDhBT1j1BmbEQP38m99O5vZswM Q==; X-IronPort-AV: E=McAfee;i="6400,9594,10420"; a="286727545" X-IronPort-AV: E=Sophos;i="5.93,193,1654585200"; d="scan'208";a="286727545" Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 26 Jul 2022 07:47:47 -0700 X-IronPort-AV: E=Sophos;i="5.93,193,1654585200"; d="scan'208";a="597047566" Received: from adamreed-mobl.amr.corp.intel.com (HELO [10.212.70.145]) ([10.212.70.145]) by orsmga007-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 26 Jul 2022 07:47:46 -0700 Message-ID: <013c0854-5b8e-6968-1ab2-88f2d0b142a0@linux.intel.com> Date: Tue, 26 Jul 2022 09:34:05 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Firefox/91.0 Thunderbird/91.11.0 Subject: Re: [PATCH v1] ASoC: Intel: cirrus-common: Use UID to map correct amp to prefix Content-Language: en-US To: Stefan Binding , Mark Brown , Liam Girdwood , Brent Lu , xliu Cc: alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org, patches@opensource.cirrus.com, Vitaly Rodionov References: <20220726134634.2842185-1-sbinding@opensource.cirrus.com> From: Pierre-Louis Bossart In-Reply-To: <20220726134634.2842185-1-sbinding@opensource.cirrus.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 7/26/22 08:46, Stefan Binding wrote: > Since the order of the amps in the ACPI determines the device name, > and the ACPI order may change depending on hardware configuration, > use UID to dynamically compute the dai links, allowing dynamic > assignment of the name_prefix. This is interesting, I didn't even know what this _UID thingy could be used for. ACPI is the gift that keeps on giving after 30 years, eh? I think you should add an explanation on what this _UID is, nothing says it actually needs to be an integer, I see e.g. this sort of 'id' in various DSDT Name (_UID, Zero) // _UID: Unique ID Name (_UID, 0x05) // _UID: Unique ID Name (_UID, "SerialIoUart0") // _UID: Unique ID Name (_UID, "PCHRESV") // _UID: Unique ID Name (_UID, "IoTraps") // _UID: Unique ID Name (_UID, "SADDLESTRING") // _UID: Unique ID and my favorite Name (_UID, "TestDev") // _UID: Unique ID > /* > * Mapping between ACPI instance id and speaker position. > - * > - * Four speakers: > - * 0: Tweeter left, 1: Woofer left > - * 2: Tweeter right, 3: Woofer right > */ > -static struct snd_soc_codec_conf cs35l41_codec_conf[] = { > - { > - .dlc = COMP_CODEC_CONF(CS35L41_DEV0_NAME), > - .name_prefix = "TL", > - }, > - { > - .dlc = COMP_CODEC_CONF(CS35L41_DEV1_NAME), > - .name_prefix = "WL", > - }, > - { > - .dlc = COMP_CODEC_CONF(CS35L41_DEV2_NAME), > - .name_prefix = "TR", > - }, > - { > - .dlc = COMP_CODEC_CONF(CS35L41_DEV3_NAME), > - .name_prefix = "WR", > - }, > -}; > +static struct snd_soc_codec_conf cs35l41_codec_conf[CS35L41_MAX_AMPS]; > > static int cs35l41_init(struct snd_soc_pcm_runtime *rtd) > { > @@ -117,10 +82,10 @@ static int cs35l41_init(struct snd_soc_pcm_runtime *rtd) > static const struct { > unsigned int rx[2]; > } cs35l41_channel_map[] = { > - {.rx = {0, 1}}, /* TL */ > {.rx = {0, 1}}, /* WL */ > - {.rx = {1, 0}}, /* TR */ > {.rx = {1, 0}}, /* WR */ > + {.rx = {0, 1}}, /* TL */ > + {.rx = {1, 0}}, /* TR */ > }; > > static int cs35l41_hw_params(struct snd_pcm_substream *substream, > @@ -175,8 +140,32 @@ static const struct snd_soc_ops cs35l41_ops = { > .hw_params = cs35l41_hw_params, > }; > > +static const char * const cs35l41_name_prefixes[] = { "WL", "WR", "TL", "TR" }; > + > +static const char * const cs35l41_uid_strings[] = { "0", "1", "2", "3" }; I must admit not understanding why you changed the order. I vaguely recall Brent Lu added this on TL, WL, TR, WR order on purpose and that it matches the order in the SOF topology. Brent, can you please comment on this? I don't really care about the order selected, just want to make sure we don't introduce a channel swap with what the firmware does. > +static void cs35l41_compute_codec_conf(void) > +{ > + int uid; > + struct acpi_device *adev; > + struct device *physdev; > + > + for (uid = 0; uid < CS35L41_MAX_AMPS; uid++) { > + adev = acpi_dev_get_first_match_dev(CS35L41_HID, cs35l41_uid_strings[uid], -1); > + if (!adev) > + return; shouldn't you log an error or something telling the user that their DSDT configuration is incorrect? If I understand the code above, is the expectation that the UID expected in the DSDT should be: Name (_UID, "0") // _UID: Unique ID for WL Name (_UID, "1") // _UID: Unique ID for WR Name (_UID, "2") // _UID: Unique ID for TL Name (_UID, "3") // _UID: Unique ID for TR Is yes that's worthy of a comment for future generations. > + physdev = get_device(acpi_get_first_physical_node(adev)); > + cs35l41_components[uid].name = dev_name(physdev); > + cs35l41_components[uid].dai_name = CS35L41_CODEC_DAI; > + cs35l41_codec_conf[uid].dlc.name = dev_name(physdev); > + cs35l41_codec_conf[uid].name_prefix = cs35l41_name_prefixes[uid]; > + acpi_dev_put(adev); > + } > +} > + > void cs35l41_set_dai_link(struct snd_soc_dai_link *link) > { > + cs35l41_compute_codec_conf(); > link->codecs = cs35l41_components; > link->num_codecs = ARRAY_SIZE(cs35l41_components); > link->init = cs35l41_init;