From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 5A08116C6BD for ; Thu, 18 Apr 2024 16:29:27 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=192.198.163.17 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1713457770; cv=none; b=Tb9eFpyGYW1iGxWVHYgtXJp2WXsCLycYdZtYreSDrYVFFeXZkT9CpkKv6iIUp3qgwKc+cSENsB+hDIKpsbn3BPz3b7Q7bavn8dlQozG168DMqQkes1G1aXlK4D/JDLyp2Sf5iAAsP/QKY76ExMY+0eO+DGRa/L3rlHAaxiz4aYk= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1713457770; c=relaxed/simple; bh=0kP32U7Suy6uQoOJjorW965cKQphIdVvkl6Lvh1uZ0g=; h=Message-ID:Date:MIME-Version:Subject:To:References:From: In-Reply-To:Content-Type; b=lxu3BQTPxKW6lK8cxndhnmNZ/wMdTUSa4u2F3B4+IaBqee292whgBxbLRYY6IxaCsi3oSHpJ8ZdhyDejJFwhefTfUmdgnn7qrgB9q8pNXuNdOsT3Kr6OAJS7+WKWdQ6l12mXDc/9y9YeOSnm0IPkuSYqemKcElo5s3/qCwE+FRY= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com; spf=none smtp.mailfrom=linux.intel.com; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b=Shm5qdeW; arc=none smtp.client-ip=192.198.163.17 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b="Shm5qdeW" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1713457768; x=1744993768; h=message-id:date:mime-version:subject:to:references:from: in-reply-to:content-transfer-encoding; bh=0kP32U7Suy6uQoOJjorW965cKQphIdVvkl6Lvh1uZ0g=; b=Shm5qdeWIL1Lr/h2p2RhmqSJMJxsFARqkUsnhtb4lO4w32igMqNBbs4d G9Dg9fAr+994hUTmbCpE5p1qqlRbA/RrrZa34OP6HRor73nQwo6EsGpdd GpURXOr29L1cVytavOU2JwOsS6nUy1u2Fhd9XkNmTtwMYWcO2QHTaMy8H K1B5SoGFRWSg61FlAJ8IOr0EsfjJ03mR6It/4Hd4bFwZBAYosMW8/TGla KlI/0IcXayQItnOE0neuvdeUkUs4X3cYbr/+3HxwWZ08ECnDe7S/uuwhd rico7jHKZbg20m4XtsBvIrgWjYC3n01UU6kd6ZlYxl4r9D8O4Mc5ZhpWZ Q==; X-CSE-ConnectionGUID: 2cRmGnwjST2B6cV5LrMEoQ== X-CSE-MsgGUID: DoMMOicjSHmdG+C7YSX/bg== X-IronPort-AV: E=McAfee;i="6600,9927,11047"; a="8882316" X-IronPort-AV: E=Sophos;i="6.07,212,1708416000"; d="scan'208";a="8882316" Received: from fmviesa009.fm.intel.com ([10.60.135.149]) by fmvoesa111.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Apr 2024 09:27:57 -0700 X-CSE-ConnectionGUID: 0j6z5O/LTrGXaM4mMcGzeQ== X-CSE-MsgGUID: 6pcNHd1SRuG8qoQ9W8Rdxw== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.07,212,1708416000"; d="scan'208";a="23105110" Received: from rswoodsx-mobl.amr.corp.intel.com (HELO [10.213.167.100]) ([10.213.167.100]) by fmviesa009-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Apr 2024 09:27:50 -0700 Message-ID: Date: Thu, 18 Apr 2024 11:20:23 -0500 Precedence: bulk X-Mailing-List: imx@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 01/23] ASoC: soc-pcm: cleanup soc_get_playback_capture() To: Kuninori Morimoto , =?UTF-8?Q?Amadeusz_S=C5=82awi=C5=84ski?= , Alexandre Belloni , Alper Nebi Yasak , AngeloGioacchino Del Regno , Banajit Goswami , Bard Liao , Brent Lu , Cezary Rojewski , Charles Keepax , Claudiu Beznea , Cristian Ciocaltea , Daniel Baluta , Hans de Goede , Jaroslav Kysela , Jerome Brunet , Jiawei Wang , Jonathan Corbet , Kai Vehmanen , Kevin Hilman , Liam Girdwood , Mark Brown , Maso Huang , Matthias Brugger , Neil Armstrong , Nicolas Ferre , Peter Ujfalusi , Ranjani Sridharan , Sascha Hauer , Shawn Guo , Shengjiu Wang , Srinivas Kandagatla , Sylwester Nawrocki , Takashi Iwai , Vinod Koul , Xiubo Li , alsa-devel@alsa-project.org, imx@lists.linux.dev, linux-doc@vger.kernel.org, linux-sound@vger.kernel.org References: <87h6fz8g3u.wl-kuninori.morimoto.gx@renesas.com> <87frvj8g2v.wl-kuninori.morimoto.gx@renesas.com> Content-Language: en-US From: Pierre-Louis Bossart In-Reply-To: <87frvj8g2v.wl-kuninori.morimoto.gx@renesas.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit > (X) part is for DPCM, and it checks playback/capture availability > if dai_link has dpcm_playback/capture flag (a)(b). > This availability check should be available not only for DPCM, but for > all connections. But it is not mandatory option. Let's name it as > assertion. I don't follow the 'not mandatory option'. Why not make these 'assertions' mandatory? What happens in case the the option is not present? > In case of having assertion flag, non specific side will be disabled. Not following the wording, multiple negatives and not clear on what 'side' refers to (direction or DPCM/non-DPCM). > For example if it has playback_assertion but not have capture_assertion, > capture will be force disabled. > > dpcm_playback -> playabck_assertion = 1 > > dpcm_capture -> capture_assertion = 1 > > playback_only -> playback_assertion = 1 > capture_assertion = 0 > > capture_only -> playback_assertion = 0 > capture_assertion = 1 > > By expanding this assertion to all connections, we can use same code > for all connections, this means code can be more cleanup. I see a contradiction between "expanding the assertion to all connections" and "not mandatory". > Current validation check on DPCM ignores Codec DAI, but there is no > reason to do it. We should check both CPU/Codec in all connection. "there's no reason to do so" ? > This patch expands validation check to all cases. > > [CPU/xxxx]-[yyyy/Codec] > ***** > > In many case (not all case), above [xxxx][yyyy] part are "dummy" DAI > which is always valid for both playback/capture. > But unfortunately DPCM BE Codec (**** part) had been no validation > check before, and some Codec driver doesn't have necessary settings for > it. This means all cases validation check breaks compatibility on some > vender's drivers. Thus this patch temporary uses dummy DAI at BPCM BE vendor > Codec part, and avoid compatibility error. But it should be removed in > the future. > > Signed-off-by: Kuninori Morimoto > --- > include/sound/soc.h | 9 +++ > sound/soc/soc-pcm.c | 143 +++++++++++++++++++++++++------------------- > 2 files changed, 92 insertions(+), 60 deletions(-) > > diff --git a/include/sound/soc.h b/include/sound/soc.h > index 0376f7e4c15d..e604d74f6e33 100644 > --- a/include/sound/soc.h > +++ b/include/sound/soc.h > @@ -809,6 +809,15 @@ struct snd_soc_dai_link { > unsigned int dpcm_capture:1; > unsigned int dpcm_playback:1; > > + /* > + * Capture / Playback support assertion. Having assertion flag is not mandatory. > + * In case of having assertion flag, non specific side will be disabled. again the 'not mandatory' and 'non specific side will be disabled' are confusing. > + /* > + * Assertion check > + * > + * playback_assertion = 0 No assertion check. > + * capture_assertion = 0 ASoC will use detected playback/capture as-is. > + * No playback, No capture will be error. did you mean "this combination will be handled as an error" ? It's probably best to have a different presentation, to avoid confusions. Using multiple lines without a separator isn't great. Suggested example: playback_assertion = 0 capture_assertion = 0 this combination will be handled as an error playback_assertion = 1 capture_assertion = 0 the DAIs in this dailink must support playback. ASoC will disable capture. In other words "playback_only" > + * > + * playback_assertion = 1 DAI must playback available. ASoC will disable capture. > + * capture_assertion = 0 In other words "playback_only" > + * > + * playback_assertion = 0 DAI must capture available. ASoC will disable playback. > + * capture_assertion = 1 In other words "capture_only" > + * > + * playback_assertion = 1 DAI must both playback/capture available. > + * capture_assertion = 1 nit-pick: the 'must X available' does not read well, 'must support X' is probably what you meant. > + */ > + if (dai_link->playback_assertion) { > + if (!has_playback) { > + dev_err(rtd->dev, "%s playback assertion check error\n", dai_link->stream_name); "invalid playback_assertion" ? > + return -EINVAL; > + } > + /* makes it plyaback only */ typo: playback > + if (!dai_link->capture_assertion) > + has_capture = 0; > + } > + if (dai_link->capture_assertion) { > + if (!has_capture) { > + dev_err(rtd->dev, "%s capture assertion check error\n", dai_link->stream_name); > + return -EINVAL; > + } > + /* makes it capture only */ > + if (!dai_link->playback_assertion) > + has_playback = 0; > + } we probably want a dev_ log when the has_playback/capture is overridden? > > + /* > + * Detect Mismatch > + */ > if (!has_playback && !has_capture) { > dev_err(rtd->dev, "substream %s has no playback, no capture\n", > dai_link->stream_name); > - > return -EINVAL; > } >