Received: by 2002:a05:6a10:f347:0:0:0:0 with SMTP id d7csp202826pxu; Wed, 2 Dec 2020 19:55:38 -0800 (PST) X-Google-Smtp-Source: ABdhPJyV5y+lwO+f807UEes00518HzlsWaXelyFam8xbNp6mRPbzWSzPpJOhN71M0GHgbvdLH32t X-Received: by 2002:a17:906:1294:: with SMTP id k20mr853389ejb.404.1606967738309; Wed, 02 Dec 2020 19:55:38 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1606967738; cv=none; d=google.com; s=arc-20160816; b=AYbbSQSXnKCCmaeDECmXotiTiuMPoVO5qG0hFHZra04/vxZHJeXaMTqxN5qyDHtNvO SObVfZDT1afGV2jGQVP7T/r2IgDVKCSbOObtuN4n+hwc/wGRKASUYxLs2aDRyX8Ru8BF hNiMF4yyuRLW+1BdDqNOr3yZFxBwOr1/KqpG6HwIKLBzC41JuehF+QSZjq7P22QB4knE Q+hxH0Gj7IgMxcfo0pd3eySoz32ynAjDDHdnrsCmiJ86E8a8d2Lmke4n2fZXz1BNnzrh dZi1+aBqyAOtzEi5dHtK0FKq27zQz/fmj72uHqtpDVfki9uH35Myhxm8nult3Wlj3iQO nqgg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:subject:message-id:date:from:in-reply-to :references:mime-version:dkim-signature; bh=SUzQs/+SoVYKqbh24yWO/KQhjnAXrlDusJdUSTJS5A8=; b=wK+TwhH+wfVIGziXLS00iZ0FrLE23IsDCB6qDqLDA1wvXLqz06xo7xtgpHoaRk4O9v 3i3K7GLocBMSTXy30+Rl7gyPOgAPk2eidu2HDrHsM6BLEteTrYZBy4wZHCHa/hj/RErT jPpcHMRVP23a9qMa+oAii6D4J9YMq6aru7AJscsShtFWODXtuqkgkzK9pEeCvWuEko3u OhSLKkFUX6LdGrmhvirxO/Tt9v/Ejd9f5Jq4moq2Wxd34fWAYgrmNKdb5mG1r/a69Mzz CfzRz6F30gXkbXNUnph9Bb7+z6FrvgcR7CrQl89VIFkB0vgqAugHJXpNlm2+x5bKTAgf 57Fw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=Mhea7zKS; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id v4si293730edj.57.2020.12.02.19.55.16; Wed, 02 Dec 2020 19:55:38 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=Mhea7zKS; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728521AbgLCDwY (ORCPT + 99 others); Wed, 2 Dec 2020 22:52:24 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54458 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726734AbgLCDwY (ORCPT ); Wed, 2 Dec 2020 22:52:24 -0500 Received: from mail-qk1-x744.google.com (mail-qk1-x744.google.com [IPv6:2607:f8b0:4864:20::744]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B3005C061A4D; Wed, 2 Dec 2020 19:51:43 -0800 (PST) Received: by mail-qk1-x744.google.com with SMTP id n132so1016026qke.1; Wed, 02 Dec 2020 19:51:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=SUzQs/+SoVYKqbh24yWO/KQhjnAXrlDusJdUSTJS5A8=; b=Mhea7zKSrwE8kuWuWy+h9M4qWmkuaGN4aIb747Gar6/nB54tO2IxYzyAY5JGJIBgFD 4lNXCYHb96+jK8BAabG5UbTB6v+QvEBfjJ4Bu3NKUQepLKB3saaoGPMzgBSAvvAF2dGK DOTSmr7c4mjAX+j/vsXoRVSKJCKOW3ZsKIdPhkqZ9xqynSGNxavwDYgdOKFnNcxCDIUB Fhr24MF6tv758ybaKhhDZ17K3qDhsPb/0glb7vdNKf0Ihw9JAepVuoGN9K7CJGcgoj/h z1oJa4ibzH3I4wFno7uS5j/qBUnMpq7ogbpp+jR2XJewTmeN2Y9rGeebn7KHR7/ov8Yi hSWA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=SUzQs/+SoVYKqbh24yWO/KQhjnAXrlDusJdUSTJS5A8=; b=EKUvCg4dDgqUOsLx9y6MBFTUGvZcRnUwulwsL63Um0uQ/t226yw7c0AIpQkYpvC4m9 LRJL/ISS4HUP6ycFaeNKaGK2ycyPDlVA1FmU4TKo085qE34CZfroVCkTvz6gy5xlAHM4 N+OFnbha7vO/PSCs92+GrpHV9EUK8F/SBVRN88UyWQQkLldEGcDTq9vfyBpWDXGnISTm nF1ZYJkz54wzwyh0TT3Y7uAnA9x3Ly0e8vcQN7ku8BUxfDwAd+cnDhB9Z9Ch2/gGpNry E5BDrLPGxrvjOvLP4AyuY+s4Q1IzxSJmPIStcBmLEMp+OQPnjmMnstiJr1cSLXloK1tX WsYA== X-Gm-Message-State: AOAM531DIjQgJsQzXQiU8wj24eysrtr921L++wjSyY4H6OQOZbwQQPtV T47ehTgqaehRHhOvTDQttfQQROqWx+iWSjeeR4A= X-Received: by 2002:a37:ad17:: with SMTP id f23mr1146570qkm.121.1606967502905; Wed, 02 Dec 2020 19:51:42 -0800 (PST) MIME-Version: 1.0 References: <1606455021-18882-1-git-send-email-shengjiu.wang@nxp.com> <1606455021-18882-2-git-send-email-shengjiu.wang@nxp.com> <20201202201955.GB1498@Asurada-Nvidia> In-Reply-To: <20201202201955.GB1498@Asurada-Nvidia> From: Shengjiu Wang Date: Thu, 3 Dec 2020 11:51:32 +0800 Message-ID: Subject: Re: [PATCH 2/2] ASoC: fsl: Add imx-hdmi machine driver To: Nicolin Chen Cc: Shengjiu Wang , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , alsa-devel@alsa-project.org, Timur Tabi , Xiubo Li , Liam Girdwood , linuxppc-dev@lists.ozlabs.org, Takashi Iwai , Rob Herring , Mark Brown , Fabio Estevam , linux-kernel Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Dec 3, 2020 at 4:23 AM Nicolin Chen wrote: > > On Fri, Nov 27, 2020 at 01:30:21PM +0800, Shengjiu Wang wrote: > > The driver is initially designed for sound card using HDMI > > interface on i.MX platform. There is internal HDMI IP or > > external HDMI modules connect with SAI or AUD2HTX interface. > > It supports both transmitter and receiver devices. > > > > Signed-off-by: Shengjiu Wang > > --- > > sound/soc/fsl/Kconfig | 12 ++ > > sound/soc/fsl/Makefile | 2 + > > sound/soc/fsl/imx-hdmi.c | 235 +++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 249 insertions(+) > > create mode 100644 sound/soc/fsl/imx-hdmi.c > > > diff --git a/sound/soc/fsl/imx-hdmi.c b/sound/soc/fsl/imx-hdmi.c > > new file mode 100644 > > index 000000000000..ac164514b1b2 > > --- /dev/null > > +++ b/sound/soc/fsl/imx-hdmi.c > > > +static int imx_hdmi_hw_params(struct snd_pcm_substream *substream, > > + struct snd_pcm_hw_params *params) > > +{ > > + struct snd_soc_pcm_runtime *rtd = substream->private_data; > > + struct imx_hdmi_data *data = snd_soc_card_get_drvdata(rtd->card); > > + bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK; > > + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); > > + struct snd_soc_card *card = rtd->card; > > + struct device *dev = card->dev; > > + int ret; > > + > > + /* set cpu DAI configuration */ > > + ret = snd_soc_dai_set_sysclk(cpu_dai, data->cpu_priv.sysclk_id[tx], > > + 8 * data->cpu_priv.slot_width * params_rate(params), > > Looks like fixed 2 slots being used, judging by the set_tdm_slot > call below. Then...why "8 *"? Probably need a line of comments? The master clock always 256 * rate, when slot_width=32. so use the 8 * slot_width. will add comments. > > > + tx ? SND_SOC_CLOCK_OUT : SND_SOC_CLOCK_IN); > > + if (ret && ret != -ENOTSUPP) { > > + dev_err(dev, "failed to set cpu sysclk: %d\n", ret); > > + return ret; > > + } > > + > > + ret = snd_soc_dai_set_tdm_slot(cpu_dai, 0, 0, 2, data->cpu_priv.slot_width); > > May have a local variable to cache slot_width. ok. > > > +static int imx_hdmi_probe(struct platform_device *pdev) > > > + data->dai.name = "i.MX HDMI"; > > + data->dai.stream_name = "i.MX HDMI"; > > + data->dai.cpus->dai_name = dev_name(&cpu_pdev->dev); > > + data->dai.platforms->of_node = cpu_np; > > + data->dai.ops = &imx_hdmi_ops; > > + data->dai.playback_only = true; > > + data->dai.capture_only = false; > > + data->dai.init = imx_hdmi_init; > > + > > + > > + if (of_property_read_bool(np, "hdmi-out")) { > > + data->dai.playback_only = true; > > + data->dai.capture_only = false; > > + data->dai.codecs->dai_name = "i2s-hifi"; > > + data->dai.codecs->name = "hdmi-audio-codec.1"; > > + data->dai.dai_fmt = data->dai_fmt | > > + SND_SOC_DAIFMT_NB_NF | > > + SND_SOC_DAIFMT_CBS_CFS; > > + } > > + > > + if (of_property_read_bool(np, "hdmi-in")) { > > + data->dai.playback_only = false; > > + data->dai.capture_only = true; > > + data->dai.codecs->dai_name = "i2s-hifi"; > > + data->dai.codecs->name = "hdmi-audio-codec.2"; > > + data->dai.dai_fmt = data->dai_fmt | > > + SND_SOC_DAIFMT_NB_NF | > > + SND_SOC_DAIFMT_CBM_CFM; > > + } > > + > > + if ((data->dai.playback_only && data->dai.capture_only) || > > + (!data->dai.playback_only && !data->dai.capture_only)) { > > + dev_err(&pdev->dev, "Wrongly enable HDMI DAI link\n"); > > + goto fail; > > + } > > Seems that this condition check can never be true, given that: > 1. By default: playback_only=true && capture_only=false > 2. Conditionally overwritten: playback_only=true && capture_only=false > 3. Conditionally overwritten: playback_only=false && capture_only=true > > If I understand it correctly, probably should be something like: > bool hdmi_out = of_property_read_bool(np, "hdmi-out"); > bool hdmi_in = of_property_read_bool(np, "hdmi-in"); > > if ((hdmi_out && hdmi_in) || (!hdmi_out || !hdmi_in)) > // "Invalid HDMI DAI link"; goto fail; > > if (hdmi_out) { > // ... > } else if (hdmi_in) { > // ... > } else // No need of this line if two properties are exclusive > Good catch, will update it. > > + data->card.num_links = 1; > > + data->card.dai_link = &data->dai; > > + > > + platform_set_drvdata(pdev, &data->card); > > Why pass card pointer? Seems it duplicates with dev_set_drvdata(card->dev, card); in snd_soc_register_card. will remove it. best regards wang shengjiu