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=-2.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, 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 15584C4338F for ; Sat, 21 Aug 2021 20:46:58 +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 B6FB461246 for ; Sat, 21 Aug 2021 20:46:56 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org B6FB461246 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=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 6F7E1950; Sat, 21 Aug 2021 22:46:04 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz 6F7E1950 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1629578814; bh=1t7bzOgppcoI+NJicZLViIMDFKIgpgz9mH68jGH9RKE=; h=From:To:Subject:Date:In-Reply-To:References:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=e8XIywmMaqcy96kENJIy1vzQDbgmG9mBUAZ3DLTR6rmJzfrGdABmbPgq8aF2QVGIt /wP65rAAFIhxyRygGq0Q4oHu1PgCMDqh41c5Ky28P0Nq7TQ4hKxoRENQ0VTomADwIl wAnCQIztWFq2sXbG6yZcv+O9vWDKkPvmtMSgm8AU= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id 06FC1F8016B; Sat, 21 Aug 2021 22:46:04 +0200 (CEST) Received: by alsa1.perex.cz (Postfix, from userid 50401) id C329AF8026D; Sat, 21 Aug 2021 22:46:02 +0200 (CEST) Received: from mail-ej1-x630.google.com (mail-ej1-x630.google.com [IPv6:2a00:1450:4864:20::630]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id 08363F8016B for ; Sat, 21 Aug 2021 22:45:59 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz 08363F8016B Authentication-Results: alsa1.perex.cz; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="qqY8UxSx" Received: by mail-ej1-x630.google.com with SMTP id x11so27766352ejv.0 for ; Sat, 21 Aug 2021 13:45:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=jW6WgkkcgqeMv2g70HWibDwKNVYFyjoAOtV6/XbdB3I=; b=qqY8UxSxlX5wea3lPQ0OcfWiDDRe3k47+uUluXghkrYr5Iwu/+xTeJVZepCNhD9FDx hSnV7rQVGcsYDrNkKL4NK/Iiic0cOQnAVm3a+F85vu25NDyTQS5P+Q2v95e7/V4FpRhw AxArOzTMRYHNlwdO5xtWiLJ4EbNU2CYyyvwr1G8ylP2AHFggPe4hO0UDkHZVdJwtjhY/ vdZH/BjCC8dlxI0p7w6p3CimbD9aEk/QACb27bGRGY64coKJtYtqXGSoGWtYS6UGEno2 26A1SCtxhH/QTspbix9EuFOWfpliO8a8/0ijMxxyKxPtBwog7NwJUZLfbp7xi2PeQ84d 1ziQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=jW6WgkkcgqeMv2g70HWibDwKNVYFyjoAOtV6/XbdB3I=; b=NABonvaaCm8/cJg9cBiL1vDZM3cv4H9zBSycn/rHrgb916VoPw6/66PIlmAVaZ/Y+T Lr9a3FSF897qg9ZEVaEVpH/BiFhiVubhPZ9iPpvjf8Z8XI3K42LGr/NFZKqhX7cEXWVH QwXZDEK6i3Vp0DnyvW3aqu4FNrvehDtJOjMwTcs0NL+HJhv/H6Ms9/PEHkPJqVZT3xDi k5/I3QVveEEHPoJo+ZTyCkNRZf2ZmM4BtQ/2BnJaArEeF3gDUT9NzWW08662pk25aAKe Jyo6sLGwUS/SM+NVPmWpBeE5jJf+Uc/nhPRnl9jevdODcOK3TkNYH3pQATkDyNBQxwJ8 jjYQ== X-Gm-Message-State: AOAM533wws+vznWfw1P97VlZfajV7/iDQ5P2+ue4/P5TpKggEJ/DRgeJ E65e9H8gcnH7aTWHO1+Tn+I= X-Google-Smtp-Source: ABdhPJylGo+JqRqLQ/39m2bF0KZUWoYNkH79V+Bh47oCk6+evGfniowz7vDAvz9iwcOv5AjqB18Djg== X-Received: by 2002:a17:906:5f93:: with SMTP id a19mr29018008eju.126.1629578758085; Sat, 21 Aug 2021 13:45:58 -0700 (PDT) Received: from archbook.localnet (84-72-105-84.dclient.hispeed.ch. [84.72.105.84]) by smtp.gmail.com with ESMTPSA id o6sm229307eje.6.2021.08.21.13.45.56 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 21 Aug 2021 13:45:57 -0700 (PDT) From: Nicolas Frattaroli To: Liam Girdwood , Mark Brown , Jaroslav Kysela , Takashi Iwai , Heiko Stuebner , Philipp Zabel , Pierre-Louis Bossart Subject: Re: [PATCH v2 1/4] ASoC: rockchip: add support for i2s-tdm controller Date: Sat, 21 Aug 2021 22:45:52 +0200 Message-ID: <3469189.PC3msRC2N5@archbook> In-Reply-To: <66d6bd43-ee43-eff4-7a68-333fbb996787@linux.intel.com> References: <20210820182731.29370-1-frattaroli.nicolas@gmail.com> <20210820182731.29370-2-frattaroli.nicolas@gmail.com> <66d6bd43-ee43-eff4-7a68-333fbb996787@linux.intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Cc: linux-rockchip@lists.infradead.org, alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.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 Freitag, 20. August 2021 21:02:16 CEST Pierre-Louis Bossart wrote: > > + regmap_read(i2s_tdm->regmap, I2S_CLR, &val); > > + /* Wait on the clear operation to finish */ > > + while (val) { > > delay needed here? > The rockchip_i2s.c code doesn't have a delay here either, but I can add one of 1 usec for good measure, it seems weird to retry the read as fast as it can. > > +static int rockchip_i2s_tdm_clk_set_rate(struct rk_i2s_tdm_dev *i2s_tdm, > > + struct clk *clk, unsigned long rate, > > + int ppm) > > +{ > > + unsigned long rate_target; > > + int delta, ret; > > + > > + if (ppm == i2s_tdm->clk_ppm) > > + return 0; > > + > > + delta = (ppm < 0) ? -1 : 1; > > + delta *= (int)div64_u64((u64)rate * (u64)abs(ppm) + 500000, > > + 1000000); > > formula looks odd? looks like you are implementing a round to nearest > operation, but that shouldn't require this multiplication? > I believe the multiplication is there to compensate for clock drift. ppm is a value between -1000 and 1000 that specifies the clock drift in presumably parts per million, going by the variable name. > > + pm_runtime_enable(&pdev->dev); > > + if (!pm_runtime_enabled(&pdev->dev)) { > > + ret = i2s_tdm_runtime_resume(&pdev->dev); > > that looks like dead code? you've just enabled pm_runtime, why would > this fail? > I've had a look at the upstream rockchip_i2s.c code which does the same thing, and I believe the idea here is that we need to manually prepare and enable the master clocks (mclk_rx/mclk_tx) if pm_runtime is not available. Otherwise, pm_runtime will presumably call our resume callback at some point. If runtime power management is disabled in the kernel config then pm_runtime_enabled is always going to return false. > > +err_suspend: > > + if (!pm_runtime_status_suspended(&pdev->dev)) > > + i2s_tdm_runtime_suspend(&pdev->dev); > > why is this necessary? I believe this is the same kind of situation as before, and the other driver does this too: if pm_runtime is not available, we need to stop our clocks manually on probe failure. > > +err_pm_disable: > > + pm_runtime_disable(&pdev->dev); > > + > > + return ret; > > +} > > + > > +static int rockchip_i2s_tdm_remove(struct platform_device *pdev) > > +{ > > + struct rk_i2s_tdm_dev *i2s_tdm = dev_get_drvdata(&pdev->dev); > > + > > + pm_runtime_disable(&pdev->dev); > > + if (!pm_runtime_status_suspended(&pdev->dev)) > > + i2s_tdm_runtime_suspend(&pdev->dev); > > this looks backwards, if you disable pm_runtime first what is the > expectation for the rest. I'm not well versed in the PM code but if my theory of this being related to unavailable PM is correct, then my best guess is that pm_runtime_disable does suspend the device, so if it's not suspended then we don't have pm_runtime and therefore need to call it manually. > > + > > + if (!IS_ERR(i2s_tdm->mclk_tx)) > > + clk_prepare_enable(i2s_tdm->mclk_tx); > > + if (!IS_ERR(i2s_tdm->mclk_rx)) > > + clk_prepare_enable(i2s_tdm->mclk_rx); > > + if (!IS_ERR(i2s_tdm->hclk)) > > + clk_disable_unprepare(i2s_tdm->hclk); > > + > > + return 0; > > +} > > + > > +#ifdef CONFIG_PM_SLEEP > > use __maybe_unused You mean instead of the ifdef stuff to just add this attribute to the following functions like this? static int rockchip_i2s_tdm_suspend(struct device *dev) __maybe_unused > > > +static int rockchip_i2s_tdm_suspend(struct device *dev) > > +{ > > + struct rk_i2s_tdm_dev *i2s_tdm = dev_get_drvdata(dev); > > + > > + regcache_mark_dirty(i2s_tdm->regmap); > > + > > + return 0; > > +} > > + > > +static int rockchip_i2s_tdm_resume(struct device *dev) > > +{ > > + struct rk_i2s_tdm_dev *i2s_tdm = dev_get_drvdata(dev); > > + int ret; > > + > > + ret = pm_runtime_get_sync(dev); > > + if (ret < 0) > > + return ret; > > + ret = regcache_sync(i2s_tdm->regmap); > > + pm_runtime_put(dev); > > + > > + return ret; > > +} > > +#endif Thank you for your review! Regards, Nicolas Frattaroli 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=-0.7 required=3.0 tests=BAYES_00,DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED,DKIM_VALID,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, 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 D0185C4338F for ; Sat, 21 Aug 2021 20:46:10 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (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 839E5601FF for ; Sat, 21 Aug 2021 20:46:10 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 839E5601FF Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-ID:Date:Subject:Cc:To:From:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=Ki2+80AU8+aloGj3U3kswFRIHSRDIq/nVBNFUhauWCM=; b=jZxAWsJMmFsQjG 5/RreyM3i1f6tlfD1v3iBgZjOLdGYrZuTkbAC03PLxTizdKySZvFqciyICpxxVKrgKPF7yYUnVwVP P6E2qegLlQKhgi9FOS9SFNVyz90TF7Q3htP5I5fP26dfquly+OSEhrZQKtbDUP3NoSQhpJ5yB160A TxmVpgZRuX5rCamltw9GKbb+vUodmqXezMcV0ZyUfVQ9nDVisLR1qGZLMweTzFJnlbQ/xDLOeSWQl 1YRnaOQOeQ14pjU2fmNOZbb1ANnAyOv+eMAZmMFLHp51ywxovUJo7QTHNY0AiPiRN+tf6DUIoPNPP XFALDYrSVwbYvJwpQgOQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mHXsR-00DApV-TL; Sat, 21 Aug 2021 20:46:03 +0000 Received: from mail-ej1-x62f.google.com ([2a00:1450:4864:20::62f]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mHXsO-00DAp6-IC; Sat, 21 Aug 2021 20:46:02 +0000 Received: by mail-ej1-x62f.google.com with SMTP id u14so2822778ejf.13; Sat, 21 Aug 2021 13:45:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=jW6WgkkcgqeMv2g70HWibDwKNVYFyjoAOtV6/XbdB3I=; b=qqY8UxSxlX5wea3lPQ0OcfWiDDRe3k47+uUluXghkrYr5Iwu/+xTeJVZepCNhD9FDx hSnV7rQVGcsYDrNkKL4NK/Iiic0cOQnAVm3a+F85vu25NDyTQS5P+Q2v95e7/V4FpRhw AxArOzTMRYHNlwdO5xtWiLJ4EbNU2CYyyvwr1G8ylP2AHFggPe4hO0UDkHZVdJwtjhY/ vdZH/BjCC8dlxI0p7w6p3CimbD9aEk/QACb27bGRGY64coKJtYtqXGSoGWtYS6UGEno2 26A1SCtxhH/QTspbix9EuFOWfpliO8a8/0ijMxxyKxPtBwog7NwJUZLfbp7xi2PeQ84d 1ziQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=jW6WgkkcgqeMv2g70HWibDwKNVYFyjoAOtV6/XbdB3I=; b=maerz3gbSreySclIk4uaaWqy069B/awQngfRc/f8YkwmeFo7yow8atx+IqR+5HGlJP HfvpERC9OymMAYU1gjbqUh1oDaqOSvNOY2AqVXKOLelDlbK5xS/SYSlmSPb9cImY87uk vpa+3oZfzSQfZLJ3Nw5EhlYesunYAAwRSCp24phA+40lj/Fz0HCFQ9es22g8b0QriVvM UaQiJCSY/xL2OPKwdyJjAZJxBAr0LAFIprM6XZ94JjDoNKEV+k4yamdhtkfT40xwpDIr 0CkyTldTl0caSDtLTkIw+TIj6E7p8WMbGzpHdH+WI/K228RQmqKKGMCOTDFKEPKO17hL 4pew== X-Gm-Message-State: AOAM530r/bIOIF+hDe0TRo7X1qXPYKuHK7LgTDshS71pKpbS1I6ppcT/ KxHl0g8OS+3VBgq0SBc04QPcfrS8I1Ob0A== X-Google-Smtp-Source: ABdhPJylGo+JqRqLQ/39m2bF0KZUWoYNkH79V+Bh47oCk6+evGfniowz7vDAvz9iwcOv5AjqB18Djg== X-Received: by 2002:a17:906:5f93:: with SMTP id a19mr29018008eju.126.1629578758085; Sat, 21 Aug 2021 13:45:58 -0700 (PDT) Received: from archbook.localnet (84-72-105-84.dclient.hispeed.ch. [84.72.105.84]) by smtp.gmail.com with ESMTPSA id o6sm229307eje.6.2021.08.21.13.45.56 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 21 Aug 2021 13:45:57 -0700 (PDT) From: Nicolas Frattaroli To: Liam Girdwood , Mark Brown , Jaroslav Kysela , Takashi Iwai , Heiko Stuebner , Philipp Zabel , Pierre-Louis Bossart Cc: linux-rockchip@lists.infradead.org, alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v2 1/4] ASoC: rockchip: add support for i2s-tdm controller Date: Sat, 21 Aug 2021 22:45:52 +0200 Message-ID: <3469189.PC3msRC2N5@archbook> In-Reply-To: <66d6bd43-ee43-eff4-7a68-333fbb996787@linux.intel.com> References: <20210820182731.29370-1-frattaroli.nicolas@gmail.com> <20210820182731.29370-2-frattaroli.nicolas@gmail.com> <66d6bd43-ee43-eff4-7a68-333fbb996787@linux.intel.com> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210821_134600_678023_B0A75A76 X-CRM114-Status: GOOD ( 31.33 ) X-BeenThere: linux-rockchip@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Upstream kernel work for Rockchip platforms List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "Linux-rockchip" Errors-To: linux-rockchip-bounces+linux-rockchip=archiver.kernel.org@lists.infradead.org On Freitag, 20. August 2021 21:02:16 CEST Pierre-Louis Bossart wrote: > > + regmap_read(i2s_tdm->regmap, I2S_CLR, &val); > > + /* Wait on the clear operation to finish */ > > + while (val) { > > delay needed here? > The rockchip_i2s.c code doesn't have a delay here either, but I can add one of 1 usec for good measure, it seems weird to retry the read as fast as it can. > > +static int rockchip_i2s_tdm_clk_set_rate(struct rk_i2s_tdm_dev *i2s_tdm, > > + struct clk *clk, unsigned long rate, > > + int ppm) > > +{ > > + unsigned long rate_target; > > + int delta, ret; > > + > > + if (ppm == i2s_tdm->clk_ppm) > > + return 0; > > + > > + delta = (ppm < 0) ? -1 : 1; > > + delta *= (int)div64_u64((u64)rate * (u64)abs(ppm) + 500000, > > + 1000000); > > formula looks odd? looks like you are implementing a round to nearest > operation, but that shouldn't require this multiplication? > I believe the multiplication is there to compensate for clock drift. ppm is a value between -1000 and 1000 that specifies the clock drift in presumably parts per million, going by the variable name. > > + pm_runtime_enable(&pdev->dev); > > + if (!pm_runtime_enabled(&pdev->dev)) { > > + ret = i2s_tdm_runtime_resume(&pdev->dev); > > that looks like dead code? you've just enabled pm_runtime, why would > this fail? > I've had a look at the upstream rockchip_i2s.c code which does the same thing, and I believe the idea here is that we need to manually prepare and enable the master clocks (mclk_rx/mclk_tx) if pm_runtime is not available. Otherwise, pm_runtime will presumably call our resume callback at some point. If runtime power management is disabled in the kernel config then pm_runtime_enabled is always going to return false. > > +err_suspend: > > + if (!pm_runtime_status_suspended(&pdev->dev)) > > + i2s_tdm_runtime_suspend(&pdev->dev); > > why is this necessary? I believe this is the same kind of situation as before, and the other driver does this too: if pm_runtime is not available, we need to stop our clocks manually on probe failure. > > +err_pm_disable: > > + pm_runtime_disable(&pdev->dev); > > + > > + return ret; > > +} > > + > > +static int rockchip_i2s_tdm_remove(struct platform_device *pdev) > > +{ > > + struct rk_i2s_tdm_dev *i2s_tdm = dev_get_drvdata(&pdev->dev); > > + > > + pm_runtime_disable(&pdev->dev); > > + if (!pm_runtime_status_suspended(&pdev->dev)) > > + i2s_tdm_runtime_suspend(&pdev->dev); > > this looks backwards, if you disable pm_runtime first what is the > expectation for the rest. I'm not well versed in the PM code but if my theory of this being related to unavailable PM is correct, then my best guess is that pm_runtime_disable does suspend the device, so if it's not suspended then we don't have pm_runtime and therefore need to call it manually. > > + > > + if (!IS_ERR(i2s_tdm->mclk_tx)) > > + clk_prepare_enable(i2s_tdm->mclk_tx); > > + if (!IS_ERR(i2s_tdm->mclk_rx)) > > + clk_prepare_enable(i2s_tdm->mclk_rx); > > + if (!IS_ERR(i2s_tdm->hclk)) > > + clk_disable_unprepare(i2s_tdm->hclk); > > + > > + return 0; > > +} > > + > > +#ifdef CONFIG_PM_SLEEP > > use __maybe_unused You mean instead of the ifdef stuff to just add this attribute to the following functions like this? static int rockchip_i2s_tdm_suspend(struct device *dev) __maybe_unused > > > +static int rockchip_i2s_tdm_suspend(struct device *dev) > > +{ > > + struct rk_i2s_tdm_dev *i2s_tdm = dev_get_drvdata(dev); > > + > > + regcache_mark_dirty(i2s_tdm->regmap); > > + > > + return 0; > > +} > > + > > +static int rockchip_i2s_tdm_resume(struct device *dev) > > +{ > > + struct rk_i2s_tdm_dev *i2s_tdm = dev_get_drvdata(dev); > > + int ret; > > + > > + ret = pm_runtime_get_sync(dev); > > + if (ret < 0) > > + return ret; > > + ret = regcache_sync(i2s_tdm->regmap); > > + pm_runtime_put(dev); > > + > > + return ret; > > +} > > +#endif Thank you for your review! Regards, Nicolas Frattaroli _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip 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=-0.7 required=3.0 tests=BAYES_00,DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED,DKIM_VALID,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, 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 A58F0C4338F for ; Sat, 21 Aug 2021 20:47:50 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (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 6D955611F0 for ; Sat, 21 Aug 2021 20:47:50 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 6D955611F0 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-ID:Date:Subject:Cc:To:From:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=DzPbhgrdWHi6BLIWVaMQDvRyzKlF+4AMbuIf/WsBSUE=; b=HtKsP6pLNwVLv7 +jA3AnkcXJgdPgEnoMB5ulGvL2g8hifxwhiwLg3V7e4ESjTaPB4VacGpuf7VP+7Hb+cqJPD+DnhIK CBIERRWgy5LSaZ4PHCVtjTc6mZGrtrnKTWgvOkhYHd9sOETGByRj33RigyuWzRtRqYXTOUl4CqU1W UgRIu+/Idd5eMYQ+Ur70Z+DhPMLTJ0XE6ZFashHUvcTtuu/g1ZelGxJrYp/kmbHAnSPOMVfbJKtNq QGVKkA/OAaxtFiIat4xhsNHzF3WpP6JzT7fwq0F+rHFHJlmSIUYoaMxyEgJ0Jq6Slpq3qIWSYR9z0 2watzroUv/UVlkKtP2ew==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mHXsT-00DApZ-Fw; Sat, 21 Aug 2021 20:46:05 +0000 Received: from mail-ej1-x62f.google.com ([2a00:1450:4864:20::62f]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mHXsO-00DAp6-IC; Sat, 21 Aug 2021 20:46:02 +0000 Received: by mail-ej1-x62f.google.com with SMTP id u14so2822778ejf.13; Sat, 21 Aug 2021 13:45:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=jW6WgkkcgqeMv2g70HWibDwKNVYFyjoAOtV6/XbdB3I=; b=qqY8UxSxlX5wea3lPQ0OcfWiDDRe3k47+uUluXghkrYr5Iwu/+xTeJVZepCNhD9FDx hSnV7rQVGcsYDrNkKL4NK/Iiic0cOQnAVm3a+F85vu25NDyTQS5P+Q2v95e7/V4FpRhw AxArOzTMRYHNlwdO5xtWiLJ4EbNU2CYyyvwr1G8ylP2AHFggPe4hO0UDkHZVdJwtjhY/ vdZH/BjCC8dlxI0p7w6p3CimbD9aEk/QACb27bGRGY64coKJtYtqXGSoGWtYS6UGEno2 26A1SCtxhH/QTspbix9EuFOWfpliO8a8/0ijMxxyKxPtBwog7NwJUZLfbp7xi2PeQ84d 1ziQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=jW6WgkkcgqeMv2g70HWibDwKNVYFyjoAOtV6/XbdB3I=; b=maerz3gbSreySclIk4uaaWqy069B/awQngfRc/f8YkwmeFo7yow8atx+IqR+5HGlJP HfvpERC9OymMAYU1gjbqUh1oDaqOSvNOY2AqVXKOLelDlbK5xS/SYSlmSPb9cImY87uk vpa+3oZfzSQfZLJ3Nw5EhlYesunYAAwRSCp24phA+40lj/Fz0HCFQ9es22g8b0QriVvM UaQiJCSY/xL2OPKwdyJjAZJxBAr0LAFIprM6XZ94JjDoNKEV+k4yamdhtkfT40xwpDIr 0CkyTldTl0caSDtLTkIw+TIj6E7p8WMbGzpHdH+WI/K228RQmqKKGMCOTDFKEPKO17hL 4pew== X-Gm-Message-State: AOAM530r/bIOIF+hDe0TRo7X1qXPYKuHK7LgTDshS71pKpbS1I6ppcT/ KxHl0g8OS+3VBgq0SBc04QPcfrS8I1Ob0A== X-Google-Smtp-Source: ABdhPJylGo+JqRqLQ/39m2bF0KZUWoYNkH79V+Bh47oCk6+evGfniowz7vDAvz9iwcOv5AjqB18Djg== X-Received: by 2002:a17:906:5f93:: with SMTP id a19mr29018008eju.126.1629578758085; Sat, 21 Aug 2021 13:45:58 -0700 (PDT) Received: from archbook.localnet (84-72-105-84.dclient.hispeed.ch. [84.72.105.84]) by smtp.gmail.com with ESMTPSA id o6sm229307eje.6.2021.08.21.13.45.56 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 21 Aug 2021 13:45:57 -0700 (PDT) From: Nicolas Frattaroli To: Liam Girdwood , Mark Brown , Jaroslav Kysela , Takashi Iwai , Heiko Stuebner , Philipp Zabel , Pierre-Louis Bossart Cc: linux-rockchip@lists.infradead.org, alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v2 1/4] ASoC: rockchip: add support for i2s-tdm controller Date: Sat, 21 Aug 2021 22:45:52 +0200 Message-ID: <3469189.PC3msRC2N5@archbook> In-Reply-To: <66d6bd43-ee43-eff4-7a68-333fbb996787@linux.intel.com> References: <20210820182731.29370-1-frattaroli.nicolas@gmail.com> <20210820182731.29370-2-frattaroli.nicolas@gmail.com> <66d6bd43-ee43-eff4-7a68-333fbb996787@linux.intel.com> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210821_134600_678023_B0A75A76 X-CRM114-Status: GOOD ( 31.33 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Freitag, 20. August 2021 21:02:16 CEST Pierre-Louis Bossart wrote: > > + regmap_read(i2s_tdm->regmap, I2S_CLR, &val); > > + /* Wait on the clear operation to finish */ > > + while (val) { > > delay needed here? > The rockchip_i2s.c code doesn't have a delay here either, but I can add one of 1 usec for good measure, it seems weird to retry the read as fast as it can. > > +static int rockchip_i2s_tdm_clk_set_rate(struct rk_i2s_tdm_dev *i2s_tdm, > > + struct clk *clk, unsigned long rate, > > + int ppm) > > +{ > > + unsigned long rate_target; > > + int delta, ret; > > + > > + if (ppm == i2s_tdm->clk_ppm) > > + return 0; > > + > > + delta = (ppm < 0) ? -1 : 1; > > + delta *= (int)div64_u64((u64)rate * (u64)abs(ppm) + 500000, > > + 1000000); > > formula looks odd? looks like you are implementing a round to nearest > operation, but that shouldn't require this multiplication? > I believe the multiplication is there to compensate for clock drift. ppm is a value between -1000 and 1000 that specifies the clock drift in presumably parts per million, going by the variable name. > > + pm_runtime_enable(&pdev->dev); > > + if (!pm_runtime_enabled(&pdev->dev)) { > > + ret = i2s_tdm_runtime_resume(&pdev->dev); > > that looks like dead code? you've just enabled pm_runtime, why would > this fail? > I've had a look at the upstream rockchip_i2s.c code which does the same thing, and I believe the idea here is that we need to manually prepare and enable the master clocks (mclk_rx/mclk_tx) if pm_runtime is not available. Otherwise, pm_runtime will presumably call our resume callback at some point. If runtime power management is disabled in the kernel config then pm_runtime_enabled is always going to return false. > > +err_suspend: > > + if (!pm_runtime_status_suspended(&pdev->dev)) > > + i2s_tdm_runtime_suspend(&pdev->dev); > > why is this necessary? I believe this is the same kind of situation as before, and the other driver does this too: if pm_runtime is not available, we need to stop our clocks manually on probe failure. > > +err_pm_disable: > > + pm_runtime_disable(&pdev->dev); > > + > > + return ret; > > +} > > + > > +static int rockchip_i2s_tdm_remove(struct platform_device *pdev) > > +{ > > + struct rk_i2s_tdm_dev *i2s_tdm = dev_get_drvdata(&pdev->dev); > > + > > + pm_runtime_disable(&pdev->dev); > > + if (!pm_runtime_status_suspended(&pdev->dev)) > > + i2s_tdm_runtime_suspend(&pdev->dev); > > this looks backwards, if you disable pm_runtime first what is the > expectation for the rest. I'm not well versed in the PM code but if my theory of this being related to unavailable PM is correct, then my best guess is that pm_runtime_disable does suspend the device, so if it's not suspended then we don't have pm_runtime and therefore need to call it manually. > > + > > + if (!IS_ERR(i2s_tdm->mclk_tx)) > > + clk_prepare_enable(i2s_tdm->mclk_tx); > > + if (!IS_ERR(i2s_tdm->mclk_rx)) > > + clk_prepare_enable(i2s_tdm->mclk_rx); > > + if (!IS_ERR(i2s_tdm->hclk)) > > + clk_disable_unprepare(i2s_tdm->hclk); > > + > > + return 0; > > +} > > + > > +#ifdef CONFIG_PM_SLEEP > > use __maybe_unused You mean instead of the ifdef stuff to just add this attribute to the following functions like this? static int rockchip_i2s_tdm_suspend(struct device *dev) __maybe_unused > > > +static int rockchip_i2s_tdm_suspend(struct device *dev) > > +{ > > + struct rk_i2s_tdm_dev *i2s_tdm = dev_get_drvdata(dev); > > + > > + regcache_mark_dirty(i2s_tdm->regmap); > > + > > + return 0; > > +} > > + > > +static int rockchip_i2s_tdm_resume(struct device *dev) > > +{ > > + struct rk_i2s_tdm_dev *i2s_tdm = dev_get_drvdata(dev); > > + int ret; > > + > > + ret = pm_runtime_get_sync(dev); > > + if (ret < 0) > > + return ret; > > + ret = regcache_sync(i2s_tdm->regmap); > > + pm_runtime_put(dev); > > + > > + return ret; > > +} > > +#endif Thank you for your review! Regards, Nicolas Frattaroli _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel