From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from wnew2-smtp.messagingengine.com (wnew2-smtp.messagingengine.com [64.147.123.27]) (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 F14E92FB6 for ; Tue, 17 Aug 2021 07:12:27 +0000 (UTC) Received: from compute2.internal (compute2.nyi.internal [10.202.2.42]) by mailnew.west.internal (Postfix) with ESMTP id 1EB592B008FF; Tue, 17 Aug 2021 03:12:26 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute2.internal (MEProxy); Tue, 17 Aug 2021 03:12:27 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cerno.tech; h= date:from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=fm3; bh=4/94n/KGPF1vmUxfP+ZmhtXHWFk 9voOY4f3v9/jT4is=; b=uh/WT0klq5f6AqZrCKiOYaWIVTyNbpINzrtrQWQrLOy zn9iPa50CPXgWfj9REM4+u/o6KZ05onXMh9T3ya3y2vnVLlx4S+4OhWCD1SN+2r6 5Y08YS8DbneXf1/LymasxgzXw9aaACgiTIGVMVaye5OBs2WMxK8vgiEY7b6Js9X2 rhc38CJMkhgIZruXfdMuLxyh8pJrpAjceQxgFFsEYAxXuZilQmOrcfX+9BVEhSrq aqQFGR6tzEQ58PX7Oim1BhRppUIrzGEMO5VRenPl6lPj2LSqIlmJ64xvH2gULD5t GYnrq6VN+RSIzJ1X5hedDfkwHoHrRnJoGqs3/LBWobg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm3; bh=4/94n/ KGPF1vmUxfP+ZmhtXHWFk9voOY4f3v9/jT4is=; b=FW/xpWtMyCM+KWUn6W7Fxp uT9cvwaaAGJOdTyVTLjJRwrc78g6+U9CEIqmqh0pznc/DvKBTFROwK9Bf9AH05cw g/lM9H4gglFclCvsAMMWUB4HDsEqSCGaOX87fMpX/Fh4LWhlH/PVHACcM621Ox/L NgnTdO5E/nV1vWVGmg3sSg1VtePekVqxfLJiEGw1nhh5RBQhsocW1ax61Y1YjIf3 lN8vOJf56+xoKNIA59JNtyphd/B7eLavR+SNDkuogrN8nBnjpDb9FEVZVAjdq52u 8Jl2yk07dDQ6HEdhb7fjhKDMYwTayR7Jakw4eHaF+hrb9tEkvN5s7fqduPMD67ag == X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvtddrledvgdduudehucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhepfffhvffukfhfgggtuggjsehgtderredttdejnecuhfhrohhmpeforgigihhm vgcutfhiphgrrhguuceomhgrgihimhgvsegtvghrnhhordhtvggthheqnecuggftrfgrth htvghrnhepuedtgfejueduheevgfevvdettdduleffgfffkeeltdffkeegudekjeeuveei gedunecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepmh grgihimhgvsegtvghrnhhordhtvggthh X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Tue, 17 Aug 2021 03:12:22 -0400 (EDT) Date: Tue, 17 Aug 2021 09:12:20 +0200 From: Maxime Ripard To: =?utf-8?B?54+t5rab?= Cc: lgirdwood@gmail.com, Mark Brown , perex@perex.cz, tiwai@suse.com, wens@csie.org, jernej.skrabec@gmail.com, p.zabel@pengutronix.de, Samuel Holland , krzk@kernel.org, linux-kernel@vger.kernel.org, alsa-devel@alsa-project.org, linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev Subject: Re: [PATCH v6 1/2] ASoC: sunxi: Add Allwinner H6 Digital MIC driver Message-ID: <20210817071220.idoxpwzbpemdjqdz@gilmour> References: <20210711122055.4529-1-fengzheng923@gmail.com> <20210715074750.ewbggulc5kast6ez@gilmour> Precedence: bulk X-Mailing-List: linux-sunxi@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="yncixzh4whlvhxud" Content-Disposition: inline In-Reply-To: --yncixzh4whlvhxud Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi, On Sun, Aug 01, 2021 at 05:47:46PM +0800, =E7=8F=AD=E6=B6=9B wrote: > > > + /* DMIC num is N+1 */ > > > + regmap_update_bits(host->regmap, SUN50I_DMIC_CH_NUM, > > > + SUN50I_DMIC_CH_NUM_N_MASK, > > > + SUN50I_DMIC_CH_NUM_N(channels - 1)); > > > + host->chan_en =3D (1 << channels) - 1; > > > + regmap_write(host->regmap, SUN50I_DMIC_HPF_CTRL, host->chan_en); > > > > Do we need to store the channels bitmask in the main structure? It looks > > fairly easy to generate, so I guess we could just use a macro >=20 > I need to store channels bitmask and use it in sun50i_dmic_trigger functi= on. But you don't need it outside of hw_params. The channel setup is typically done in hw_params, not in the trigger hook. [...] > > > + /* Clocks */ > > > + host->bus_clk =3D devm_clk_get(&pdev->dev, "bus"); > > > + if (IS_ERR(host->bus_clk)) > > > + return dev_err_probe(&pdev->dev, PTR_ERR(host->bus_clk), > > > + "failed to get bus clock.\n"); > > > + > > > + host->dmic_clk =3D devm_clk_get(&pdev->dev, "mod"); > > > + if (IS_ERR(host->dmic_clk)) > > > + return dev_err_probe(&pdev->dev, PTR_ERR(host->dmic_clk= ), > > > + "failed to get dmic clock.\n"); > > > + > > > + host->dma_params_rx.addr =3D res->start + SUN50I_DMIC_DATA; > > > + host->dma_params_rx.maxburst =3D 8; > > > + > > > + platform_set_drvdata(pdev, host); > > > + > > > + host->rst =3D devm_reset_control_get_optional_exclusive(&pdev->= dev, NULL); > > > + if (IS_ERR(host->rst)) > > > + return dev_err_probe(&pdev->dev, PTR_ERR(host->rst), > > > + "Failed to get reset.\n"); > > > > Your binding states that the reset is mandatory so you don't need the > > optional variant. > > > > > + reset_control_deassert(host->rst); > > > > Can't this be moved to the runtime_pm hooks? >=20 > Is this necessary? I see that most of the driver files execute > reset_control_deassert in the probe function, and reset_control_assert > in the remove function. Your driver seems to not rely on the fact that the device remains powered between each run anyway, so yeah, that way you will completely power it down. > > > > > + ret =3D devm_snd_soc_register_component(&pdev->dev, > > > + &sun50i_dmic_component, &sun50i_dmic_da= i, 1); > > > > Your second line should be aligned on the opening parenthesis > > > > > + if (ret) > > > + return dev_err_probe(&pdev->dev, ret, > > > + "failed to register component.\n"); > > > + > > > + pm_runtime_enable(&pdev->dev); > > > + if (!pm_runtime_enabled(&pdev->dev)) { > > > + ret =3D sun50i_dmic_runtime_resume(&pdev->dev); > > > + if (ret) > > > + goto err_unregister; > > > + } > > > > We have a depends on PM on some drivers already, so I guess it would > > just make sense to add one more here instead of dealing with whether > > runtime_pm is compiled in or not. >=20 > I don't understand. I am referring to the sun4i-spdif.c file. Which > driver files should I refer to? That whole logic is there to enable the device is CONFIG_PM is not enabled (and thus the calls to pm_runtime_* won't do anything). Just add a depends on PM to your Kconfig entry, and call pm_runtime_enable there. Maxime --yncixzh4whlvhxud Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iHUEABYIAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCYRthVAAKCRDj7w1vZxhR xV5tAQDgo2PFfMG3oI+WJpwKhRvxyNOZwsTG3w/3ZeravzZx4AEA8cAL7Kk9k3li rT0wIeYDdfx6CDXMjnnGlqzr/0YvlAs= =VTo/ -----END PGP SIGNATURE----- --yncixzh4whlvhxud-- 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=-3.8 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 38ED0C4338F for ; Tue, 17 Aug 2021 07:13:32 +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 9BB4860F5E for ; Tue, 17 Aug 2021 07:13:30 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 9BB4860F5E Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=cerno.tech 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 68657823; Tue, 17 Aug 2021 09:12:38 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz 68657823 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1629184408; bh=gLYNp/i0X+lR6+19ICYCGfk4F/BH+/R2xv+QWt/dK0s=; h=Date:From:To:Subject:References:In-Reply-To:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=seZTFdqrlQMtg/Nu/A2e/rajT4a5lO+DpwdVJoc/tMv4v6H0MkpQLtTO6LF3Y4Wxj wqs5UAvUVjeSwKPh7inD7hTCjMcAqK5qSLacRUKTEmitIf5P0ld13pZCMkWHV9H9y5 LlWWvzsefX7BaZ6y0bIoKJC1Jz+feVHy/Ne960ho= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id F073BF80163; Tue, 17 Aug 2021 09:12:37 +0200 (CEST) Received: by alsa1.perex.cz (Postfix, from userid 50401) id B7F69F80272; Tue, 17 Aug 2021 09:12:35 +0200 (CEST) Received: from wnew2-smtp.messagingengine.com (wnew2-smtp.messagingengine.com [64.147.123.27]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id 46021F80163 for ; Tue, 17 Aug 2021 09:12:29 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz 46021F80163 Authentication-Results: alsa1.perex.cz; dkim=pass (2048-bit key) header.d=cerno.tech header.i=@cerno.tech header.b="uh/WT0kl"; dkim=pass (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b="FW/xpWtM" Received: from compute2.internal (compute2.nyi.internal [10.202.2.42]) by mailnew.west.internal (Postfix) with ESMTP id 1EB592B008FF; Tue, 17 Aug 2021 03:12:26 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute2.internal (MEProxy); Tue, 17 Aug 2021 03:12:27 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cerno.tech; h= date:from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=fm3; bh=4/94n/KGPF1vmUxfP+ZmhtXHWFk 9voOY4f3v9/jT4is=; b=uh/WT0klq5f6AqZrCKiOYaWIVTyNbpINzrtrQWQrLOy zn9iPa50CPXgWfj9REM4+u/o6KZ05onXMh9T3ya3y2vnVLlx4S+4OhWCD1SN+2r6 5Y08YS8DbneXf1/LymasxgzXw9aaACgiTIGVMVaye5OBs2WMxK8vgiEY7b6Js9X2 rhc38CJMkhgIZruXfdMuLxyh8pJrpAjceQxgFFsEYAxXuZilQmOrcfX+9BVEhSrq aqQFGR6tzEQ58PX7Oim1BhRppUIrzGEMO5VRenPl6lPj2LSqIlmJ64xvH2gULD5t GYnrq6VN+RSIzJ1X5hedDfkwHoHrRnJoGqs3/LBWobg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm3; bh=4/94n/ KGPF1vmUxfP+ZmhtXHWFk9voOY4f3v9/jT4is=; b=FW/xpWtMyCM+KWUn6W7Fxp uT9cvwaaAGJOdTyVTLjJRwrc78g6+U9CEIqmqh0pznc/DvKBTFROwK9Bf9AH05cw g/lM9H4gglFclCvsAMMWUB4HDsEqSCGaOX87fMpX/Fh4LWhlH/PVHACcM621Ox/L NgnTdO5E/nV1vWVGmg3sSg1VtePekVqxfLJiEGw1nhh5RBQhsocW1ax61Y1YjIf3 lN8vOJf56+xoKNIA59JNtyphd/B7eLavR+SNDkuogrN8nBnjpDb9FEVZVAjdq52u 8Jl2yk07dDQ6HEdhb7fjhKDMYwTayR7Jakw4eHaF+hrb9tEkvN5s7fqduPMD67ag == X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvtddrledvgdduudehucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhepfffhvffukfhfgggtuggjsehgtderredttdejnecuhfhrohhmpeforgigihhm vgcutfhiphgrrhguuceomhgrgihimhgvsegtvghrnhhordhtvggthheqnecuggftrfgrth htvghrnhepuedtgfejueduheevgfevvdettdduleffgfffkeeltdffkeegudekjeeuveei gedunecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepmh grgihimhgvsegtvghrnhhordhtvggthh X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Tue, 17 Aug 2021 03:12:22 -0400 (EDT) Date: Tue, 17 Aug 2021 09:12:20 +0200 From: Maxime Ripard To: =?utf-8?B?54+t5rab?= Subject: Re: [PATCH v6 1/2] ASoC: sunxi: Add Allwinner H6 Digital MIC driver Message-ID: <20210817071220.idoxpwzbpemdjqdz@gilmour> References: <20210711122055.4529-1-fengzheng923@gmail.com> <20210715074750.ewbggulc5kast6ez@gilmour> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="yncixzh4whlvhxud" Content-Disposition: inline In-Reply-To: Cc: alsa-devel@alsa-project.org, Samuel Holland , linux-kernel@vger.kernel.org, tiwai@suse.com, jernej.skrabec@gmail.com, lgirdwood@gmail.com, wens@csie.org, Mark Brown , krzk@kernel.org, p.zabel@pengutronix.de, linux-sunxi@lists.linux.dev, 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" --yncixzh4whlvhxud Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi, On Sun, Aug 01, 2021 at 05:47:46PM +0800, =E7=8F=AD=E6=B6=9B wrote: > > > + /* DMIC num is N+1 */ > > > + regmap_update_bits(host->regmap, SUN50I_DMIC_CH_NUM, > > > + SUN50I_DMIC_CH_NUM_N_MASK, > > > + SUN50I_DMIC_CH_NUM_N(channels - 1)); > > > + host->chan_en =3D (1 << channels) - 1; > > > + regmap_write(host->regmap, SUN50I_DMIC_HPF_CTRL, host->chan_en); > > > > Do we need to store the channels bitmask in the main structure? It looks > > fairly easy to generate, so I guess we could just use a macro >=20 > I need to store channels bitmask and use it in sun50i_dmic_trigger functi= on. But you don't need it outside of hw_params. The channel setup is typically done in hw_params, not in the trigger hook. [...] > > > + /* Clocks */ > > > + host->bus_clk =3D devm_clk_get(&pdev->dev, "bus"); > > > + if (IS_ERR(host->bus_clk)) > > > + return dev_err_probe(&pdev->dev, PTR_ERR(host->bus_clk), > > > + "failed to get bus clock.\n"); > > > + > > > + host->dmic_clk =3D devm_clk_get(&pdev->dev, "mod"); > > > + if (IS_ERR(host->dmic_clk)) > > > + return dev_err_probe(&pdev->dev, PTR_ERR(host->dmic_clk= ), > > > + "failed to get dmic clock.\n"); > > > + > > > + host->dma_params_rx.addr =3D res->start + SUN50I_DMIC_DATA; > > > + host->dma_params_rx.maxburst =3D 8; > > > + > > > + platform_set_drvdata(pdev, host); > > > + > > > + host->rst =3D devm_reset_control_get_optional_exclusive(&pdev->= dev, NULL); > > > + if (IS_ERR(host->rst)) > > > + return dev_err_probe(&pdev->dev, PTR_ERR(host->rst), > > > + "Failed to get reset.\n"); > > > > Your binding states that the reset is mandatory so you don't need the > > optional variant. > > > > > + reset_control_deassert(host->rst); > > > > Can't this be moved to the runtime_pm hooks? >=20 > Is this necessary? I see that most of the driver files execute > reset_control_deassert in the probe function, and reset_control_assert > in the remove function. Your driver seems to not rely on the fact that the device remains powered between each run anyway, so yeah, that way you will completely power it down. > > > > > + ret =3D devm_snd_soc_register_component(&pdev->dev, > > > + &sun50i_dmic_component, &sun50i_dmic_da= i, 1); > > > > Your second line should be aligned on the opening parenthesis > > > > > + if (ret) > > > + return dev_err_probe(&pdev->dev, ret, > > > + "failed to register component.\n"); > > > + > > > + pm_runtime_enable(&pdev->dev); > > > + if (!pm_runtime_enabled(&pdev->dev)) { > > > + ret =3D sun50i_dmic_runtime_resume(&pdev->dev); > > > + if (ret) > > > + goto err_unregister; > > > + } > > > > We have a depends on PM on some drivers already, so I guess it would > > just make sense to add one more here instead of dealing with whether > > runtime_pm is compiled in or not. >=20 > I don't understand. I am referring to the sun4i-spdif.c file. Which > driver files should I refer to? That whole logic is there to enable the device is CONFIG_PM is not enabled (and thus the calls to pm_runtime_* won't do anything). Just add a depends on PM to your Kconfig entry, and call pm_runtime_enable there. Maxime --yncixzh4whlvhxud Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iHUEABYIAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCYRthVAAKCRDj7w1vZxhR xV5tAQDgo2PFfMG3oI+WJpwKhRvxyNOZwsTG3w/3ZeravzZx4AEA8cAL7Kk9k3li rT0wIeYDdfx6CDXMjnnGlqzr/0YvlAs= =VTo/ -----END PGP SIGNATURE----- --yncixzh4whlvhxud-- 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.5 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED 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 2FCADC4338F for ; Tue, 17 Aug 2021 07:14:34 +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 E5AE1600D3 for ; Tue, 17 Aug 2021 07:14:33 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org E5AE1600D3 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=cerno.tech 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-Type: List-Subscribe:List-Help:List-Post:List-Archive:List-Unsubscribe:List-Id: In-Reply-To:MIME-Version:References:Message-ID:Subject:Cc:To:From:Date: Reply-To:Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date :Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=NqERadNkAutTxWL/4CAV8zAPvLCQEH+PaoBM3sImn0Q=; b=y1BIwwGAoFAv6iA8H/INmcfRB8 IVjKVfCfKQdf7JdL18Uoj+uP3x5mwbV0wNNuJWLEGqbh0vtwK3FYfoEqjoEWiWx/BcM8TegYbcln6 tA2Sh5WnKooJuT+47ovc7EDJjbEwnzjWjrc/8gBBkg6jpzlr0FT8JkXJvbyGOSRs5rRIdEaQwwbJR fNvZUnAdPY4QIiQ8RFuv+kFzCT1hE828+UvVCuM/kw15CbM9xiQg2wy86jDiWR1rKRF/awfinZ97o JlpgZYsSJGni/uUeIhBjocHFns9i7R0LV6zYcs40Spaee1Li2B5X7gj5Dly9z1bTdoypnUHePeQac p22I+YKw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mFtH5-001INn-Ph; Tue, 17 Aug 2021 07:12:40 +0000 Received: from wnew2-smtp.messagingengine.com ([64.147.123.27]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mFtGz-001IL5-1w for linux-arm-kernel@lists.infradead.org; Tue, 17 Aug 2021 07:12:37 +0000 Received: from compute2.internal (compute2.nyi.internal [10.202.2.42]) by mailnew.west.internal (Postfix) with ESMTP id 1EB592B008FF; Tue, 17 Aug 2021 03:12:26 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute2.internal (MEProxy); Tue, 17 Aug 2021 03:12:27 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cerno.tech; h= date:from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=fm3; bh=4/94n/KGPF1vmUxfP+ZmhtXHWFk 9voOY4f3v9/jT4is=; b=uh/WT0klq5f6AqZrCKiOYaWIVTyNbpINzrtrQWQrLOy zn9iPa50CPXgWfj9REM4+u/o6KZ05onXMh9T3ya3y2vnVLlx4S+4OhWCD1SN+2r6 5Y08YS8DbneXf1/LymasxgzXw9aaACgiTIGVMVaye5OBs2WMxK8vgiEY7b6Js9X2 rhc38CJMkhgIZruXfdMuLxyh8pJrpAjceQxgFFsEYAxXuZilQmOrcfX+9BVEhSrq aqQFGR6tzEQ58PX7Oim1BhRppUIrzGEMO5VRenPl6lPj2LSqIlmJ64xvH2gULD5t GYnrq6VN+RSIzJ1X5hedDfkwHoHrRnJoGqs3/LBWobg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm3; bh=4/94n/ KGPF1vmUxfP+ZmhtXHWFk9voOY4f3v9/jT4is=; b=FW/xpWtMyCM+KWUn6W7Fxp uT9cvwaaAGJOdTyVTLjJRwrc78g6+U9CEIqmqh0pznc/DvKBTFROwK9Bf9AH05cw g/lM9H4gglFclCvsAMMWUB4HDsEqSCGaOX87fMpX/Fh4LWhlH/PVHACcM621Ox/L NgnTdO5E/nV1vWVGmg3sSg1VtePekVqxfLJiEGw1nhh5RBQhsocW1ax61Y1YjIf3 lN8vOJf56+xoKNIA59JNtyphd/B7eLavR+SNDkuogrN8nBnjpDb9FEVZVAjdq52u 8Jl2yk07dDQ6HEdhb7fjhKDMYwTayR7Jakw4eHaF+hrb9tEkvN5s7fqduPMD67ag == X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvtddrledvgdduudehucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhepfffhvffukfhfgggtuggjsehgtderredttdejnecuhfhrohhmpeforgigihhm vgcutfhiphgrrhguuceomhgrgihimhgvsegtvghrnhhordhtvggthheqnecuggftrfgrth htvghrnhepuedtgfejueduheevgfevvdettdduleffgfffkeeltdffkeegudekjeeuveei gedunecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepmh grgihimhgvsegtvghrnhhordhtvggthh X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Tue, 17 Aug 2021 03:12:22 -0400 (EDT) Date: Tue, 17 Aug 2021 09:12:20 +0200 From: Maxime Ripard To: =?utf-8?B?54+t5rab?= Cc: lgirdwood@gmail.com, Mark Brown , perex@perex.cz, tiwai@suse.com, wens@csie.org, jernej.skrabec@gmail.com, p.zabel@pengutronix.de, Samuel Holland , krzk@kernel.org, linux-kernel@vger.kernel.org, alsa-devel@alsa-project.org, linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev Subject: Re: [PATCH v6 1/2] ASoC: sunxi: Add Allwinner H6 Digital MIC driver Message-ID: <20210817071220.idoxpwzbpemdjqdz@gilmour> References: <20210711122055.4529-1-fengzheng923@gmail.com> <20210715074750.ewbggulc5kast6ez@gilmour> MIME-Version: 1.0 In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210817_001233_459232_9D7D5A14 X-CRM114-Status: GOOD ( 28.21 ) 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: multipart/mixed; boundary="===============7432765287131699469==" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org --===============7432765287131699469== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="yncixzh4whlvhxud" Content-Disposition: inline --yncixzh4whlvhxud Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi, On Sun, Aug 01, 2021 at 05:47:46PM +0800, =E7=8F=AD=E6=B6=9B wrote: > > > + /* DMIC num is N+1 */ > > > + regmap_update_bits(host->regmap, SUN50I_DMIC_CH_NUM, > > > + SUN50I_DMIC_CH_NUM_N_MASK, > > > + SUN50I_DMIC_CH_NUM_N(channels - 1)); > > > + host->chan_en =3D (1 << channels) - 1; > > > + regmap_write(host->regmap, SUN50I_DMIC_HPF_CTRL, host->chan_en); > > > > Do we need to store the channels bitmask in the main structure? It looks > > fairly easy to generate, so I guess we could just use a macro >=20 > I need to store channels bitmask and use it in sun50i_dmic_trigger functi= on. But you don't need it outside of hw_params. The channel setup is typically done in hw_params, not in the trigger hook. [...] > > > + /* Clocks */ > > > + host->bus_clk =3D devm_clk_get(&pdev->dev, "bus"); > > > + if (IS_ERR(host->bus_clk)) > > > + return dev_err_probe(&pdev->dev, PTR_ERR(host->bus_clk), > > > + "failed to get bus clock.\n"); > > > + > > > + host->dmic_clk =3D devm_clk_get(&pdev->dev, "mod"); > > > + if (IS_ERR(host->dmic_clk)) > > > + return dev_err_probe(&pdev->dev, PTR_ERR(host->dmic_clk= ), > > > + "failed to get dmic clock.\n"); > > > + > > > + host->dma_params_rx.addr =3D res->start + SUN50I_DMIC_DATA; > > > + host->dma_params_rx.maxburst =3D 8; > > > + > > > + platform_set_drvdata(pdev, host); > > > + > > > + host->rst =3D devm_reset_control_get_optional_exclusive(&pdev->= dev, NULL); > > > + if (IS_ERR(host->rst)) > > > + return dev_err_probe(&pdev->dev, PTR_ERR(host->rst), > > > + "Failed to get reset.\n"); > > > > Your binding states that the reset is mandatory so you don't need the > > optional variant. > > > > > + reset_control_deassert(host->rst); > > > > Can't this be moved to the runtime_pm hooks? >=20 > Is this necessary? I see that most of the driver files execute > reset_control_deassert in the probe function, and reset_control_assert > in the remove function. Your driver seems to not rely on the fact that the device remains powered between each run anyway, so yeah, that way you will completely power it down. > > > > > + ret =3D devm_snd_soc_register_component(&pdev->dev, > > > + &sun50i_dmic_component, &sun50i_dmic_da= i, 1); > > > > Your second line should be aligned on the opening parenthesis > > > > > + if (ret) > > > + return dev_err_probe(&pdev->dev, ret, > > > + "failed to register component.\n"); > > > + > > > + pm_runtime_enable(&pdev->dev); > > > + if (!pm_runtime_enabled(&pdev->dev)) { > > > + ret =3D sun50i_dmic_runtime_resume(&pdev->dev); > > > + if (ret) > > > + goto err_unregister; > > > + } > > > > We have a depends on PM on some drivers already, so I guess it would > > just make sense to add one more here instead of dealing with whether > > runtime_pm is compiled in or not. >=20 > I don't understand. I am referring to the sun4i-spdif.c file. Which > driver files should I refer to? That whole logic is there to enable the device is CONFIG_PM is not enabled (and thus the calls to pm_runtime_* won't do anything). Just add a depends on PM to your Kconfig entry, and call pm_runtime_enable there. Maxime --yncixzh4whlvhxud Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iHUEABYIAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCYRthVAAKCRDj7w1vZxhR xV5tAQDgo2PFfMG3oI+WJpwKhRvxyNOZwsTG3w/3ZeravzZx4AEA8cAL7Kk9k3li rT0wIeYDdfx6CDXMjnnGlqzr/0YvlAs= =VTo/ -----END PGP SIGNATURE----- --yncixzh4whlvhxud-- --===============7432765287131699469== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel --===============7432765287131699469==--