From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754103AbaIOKhi (ORCPT ); Mon, 15 Sep 2014 06:37:38 -0400 Received: from mail-bl2on0140.outbound.protection.outlook.com ([65.55.169.140]:63488 "EHLO na01-bl2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753204AbaIOKhf (ORCPT ); Mon, 15 Sep 2014 06:37:35 -0400 Date: Mon, 15 Sep 2014 18:37:15 +0800 From: Shengjiu Wang To: Markus Pargmann CC: , , , , , , , , , Subject: Re: [PATCH V3] ASoC: fsl_ssi: refine ipg clock usage in this module Message-ID: <20140915103714.GB23877@audiosh1> References: <894383e00876763f22988fc5f3f9f232f939f923.1410517971.git.shengjiu.wang@freescale.com> <20140915100541.GD8844@pengutronix.de> <20140915102224.GA23877@audiosh1> <20140915103213.GE8844@pengutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <20140915103213.GE8844@pengutronix.de> User-Agent: Mutt/1.5.21 (2010-09-15) X-EOPAttributedMessage: 0 X-Forefront-Antispam-Report: CIP:192.88.168.50;CTRY:US;IPV:CAL;IPV:NLI;EFV:NLI;SFV:NSPM;SFS:(10019020)(6009001)(199003)(51704005)(45984002)(189002)(54534003)(24454002)(87936001)(102836001)(50986999)(105606002)(85852003)(97736003)(44976005)(83322001)(99396002)(92726001)(92566001)(21056001)(86362001)(6806004)(95666004)(15202345003)(26826002)(54356999)(76176999)(104016003)(81542001)(50466002)(33716001)(80022001)(46102001)(64706001)(110136001)(46406003)(23726002)(84676001)(74502001)(107046002)(106466001)(81342001)(47776003)(85306004)(93886004)(77982001)(79102001)(20776003)(74662001)(19580395003)(31966008)(19580405001)(83072002)(68736004)(76482001)(83506001)(15975445006)(4396001)(33656002)(90102001)(97756001);DIR:OUT;SFP:1102;SCL:1;SRVR:DM2PR0301MB0624;H:tx30smr01.am.freescale.net;FPR:;MLV:ovrnspm;PTR:InfoDomainNonexistent;A:1;MX:1;LANG:en; X-Microsoft-Antispam: BCL:0;PCL:0;RULEID:;UriScan:; X-Forefront-PRVS: 03355EE97E Authentication-Results: spf=fail (sender IP is 192.88.168.50) smtp.mailfrom=shengjiu.wang@freescale.com; X-OriginatorOrg: freescale.com Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Sep 15, 2014 at 12:32:13PM +0200, Markus Pargmann wrote: > On Mon, Sep 15, 2014 at 06:22:27PM +0800, Shengjiu Wang wrote: > > On Mon, Sep 15, 2014 at 12:05:41PM +0200, Markus Pargmann wrote: > > > On Fri, Sep 12, 2014 at 06:35:15PM +0800, Shengjiu Wang wrote: > > > > Check if ipg clock is in clock-names property, then we can move the > > > > ipg clock enable and disable operation to startup and shutdown, that > > > > is only enable ipg clock when ssi is working and keep clock is disabled > > > > when ssi is in idle. > > > > But when the checking is failed, remain the clock control as before. > > > > > > > > Signed-off-by: Shengjiu Wang > > > > --- > > > > V3 change log: > > > > update patch according Nicolin and markus's comments > > > > > > > > > > > > sound/soc/fsl/fsl_ssi.c | 53 ++++++++++++++++++++++++++++++++++++++++------- > > > > 1 file changed, 45 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c > > > > index 2fc3e66..6d1dfd5 100644 > > > > --- a/sound/soc/fsl/fsl_ssi.c > > > > +++ b/sound/soc/fsl/fsl_ssi.c > > > > @@ -169,6 +169,7 @@ struct fsl_ssi_private { > > > > u8 i2s_mode; > > > > bool use_dma; > > > > bool use_dual_fifo; > > > > + bool has_ipg_clk_name; > > > > unsigned int fifo_depth; > > > > struct fsl_ssi_rxtx_reg_val rxtx_reg_val; > > > > > > > > @@ -530,6 +531,11 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream, > > > > struct snd_soc_pcm_runtime *rtd = substream->private_data; > > > > struct fsl_ssi_private *ssi_private = > > > > snd_soc_dai_get_drvdata(rtd->cpu_dai); > > > > + int ret; > > > > + > > > > + ret = clk_prepare_enable(ssi_private->clk); > > > > + if (ret) > > > > + return ret; > > > > > > > > /* When using dual fifo mode, it is safer to ensure an even period > > > > * size. If appearing to an odd number while DMA always starts its > > > > @@ -544,6 +550,21 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream, > > > > } > > > > > > > > /** > > > > + * fsl_ssi_shutdown: shutdown the SSI > > > > + * > > > > + */ > > > > +static void fsl_ssi_shutdown(struct snd_pcm_substream *substream, > > > > + struct snd_soc_dai *dai) > > > > +{ > > > > + struct snd_soc_pcm_runtime *rtd = substream->private_data; > > > > + struct fsl_ssi_private *ssi_private = > > > > + snd_soc_dai_get_drvdata(rtd->cpu_dai); > > > > + > > > > + clk_disable_unprepare(ssi_private->clk); > > > > + > > > > +} > > > > + > > > > +/** > > > > * fsl_ssi_set_bclk - configure Digital Audio Interface bit clock > > > > * > > > > * Note: This function can be only called when using SSI as DAI master > > > > @@ -1043,6 +1064,7 @@ static int fsl_ssi_dai_probe(struct snd_soc_dai *dai) > > > > > > > > static const struct snd_soc_dai_ops fsl_ssi_dai_ops = { > > > > .startup = fsl_ssi_startup, > > > > + .shutdown = fsl_ssi_shutdown, > > > > .hw_params = fsl_ssi_hw_params, > > > > .hw_free = fsl_ssi_hw_free, > > > > .set_fmt = fsl_ssi_set_dai_fmt, > > > > @@ -1168,17 +1190,22 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev, > > > > u32 dmas[4]; > > > > int ret; > > > > > > > > - ssi_private->clk = devm_clk_get(&pdev->dev, NULL); > > > > + if (ssi_private->has_ipg_clk_name) > > > > + ssi_private->clk = devm_clk_get(&pdev->dev, "ipg"); > > > > + else > > > > + ssi_private->clk = devm_clk_get(&pdev->dev, NULL); > > > > if (IS_ERR(ssi_private->clk)) { > > > > ret = PTR_ERR(ssi_private->clk); > > > > dev_err(&pdev->dev, "could not get clock: %d\n", ret); > > > > return ret; > > > > } > > > > > > > > - ret = clk_prepare_enable(ssi_private->clk); > > > > - if (ret) { > > > > - dev_err(&pdev->dev, "clk_prepare_enable failed: %d\n", ret); > > > > - return ret; > > > > + if (!ssi_private->has_ipg_clk_name) { > > > > + ret = clk_prepare_enable(ssi_private->clk); > > > > + if (ret) { > > > > + dev_err(&pdev->dev, "clk_prepare_enable failed: %d\n", ret); > > > > + return ret; > > > > + } > > > > } > > > > > > > > /* For those SLAVE implementations, we ingore non-baudclk cases > > > > @@ -1236,8 +1263,9 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev, > > > > return 0; > > > > > > > > error_pcm: > > > > - clk_disable_unprepare(ssi_private->clk); > > > > > > > > + if (!ssi_private->has_ipg_clk_name) > > > > + clk_disable_unprepare(ssi_private->clk); > > > > return ret; > > > > } > > > > > > > > @@ -1246,7 +1274,8 @@ static void fsl_ssi_imx_clean(struct platform_device *pdev, > > > > { > > > > if (!ssi_private->use_dma) > > > > imx_pcm_fiq_exit(pdev); > > > > - clk_disable_unprepare(ssi_private->clk); > > > > + if (!ssi_private->has_ipg_clk_name) > > > > + clk_disable_unprepare(ssi_private->clk); > > > > } > > > > > > > > static int fsl_ssi_probe(struct platform_device *pdev) > > > > @@ -1321,8 +1350,16 @@ static int fsl_ssi_probe(struct platform_device *pdev) > > > > return -ENOMEM; > > > > } > > > > > > > > - ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem, > > > > + ret = of_property_match_string(np, "clock-names", "ipg"); > > > > + if (ret < 0) { > > > > + ssi_private->has_ipg_clk_name = false; > > > > + ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem, > > > > &fsl_ssi_regconfig); > > > > > > Sorry if I was unclear about that. My suggestion was to enable the clock > > > right here: > > > clk_prepare_enable(ssi_private->clk); > > > > > > Then you can remove ssi_private->has_ipg_clk_name and all > > > clk_prepare_enable() and clk_disable_unprepare() from above. Also you > > > can move the devm_clk_get() into this block. > > > > > > > ipg clock not only need to be enabled when accessing register, but also > > need to be enabled when ssi is working. So I add clk_enable in startup. > > > > > It seems you really want to implement this for devicetrees where the > > > "ipg" clock-name is missing, but I don't understand why? I really can't > > > see any benefit of adding all these clk_prepare_enable() calls for all > > > cornercases that may occure. For example the clocks for AC97 are still > > > missing in this version. > > > > When ipg clock-name is missing, I just remain the code logic as before. > > So AC97 case is same as before too. > > When have ipg clock-name, register it to regmap for accessing register, > > and enabling ipg clock in startup also is useful for AC97. > > > > I don't understand why you think clock for AC97 is still missing? > > Sorry got it all wrong, monday morning tiredness perhaps ;-). > > The patch looks good for me at the second look. I will test it later > today and give you feedback. > > Best regards, > > Markus Thank you very much. Your comments help me a lot. and wait your feedback. best regards wang shengjiu > > -- > Pengutronix e.K. | | > Industrial Linux Solutions | http://www.pengutronix.de/ | > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shengjiu Wang Subject: Re: [PATCH V3] ASoC: fsl_ssi: refine ipg clock usage in this module Date: Mon, 15 Sep 2014 18:37:15 +0800 Message-ID: <20140915103714.GB23877@audiosh1> References: <894383e00876763f22988fc5f3f9f232f939f923.1410517971.git.shengjiu.wang@freescale.com> <20140915100541.GD8844@pengutronix.de> <20140915102224.GA23877@audiosh1> <20140915103213.GE8844@pengutronix.de> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Return-path: Content-Disposition: inline In-Reply-To: <20140915103213.GE8844@pengutronix.de> Sender: linux-kernel-owner@vger.kernel.org To: Markus Pargmann Cc: timur@tabi.org, nicoleotsuka@gmail.com, Li.Xiubo@freescale.com, lgirdwood@gmail.com, broonie@kernel.org, perex@perex.cz, tiwai@suse.de, alsa-devel@alsa-project.org, linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org List-Id: alsa-devel@alsa-project.org On Mon, Sep 15, 2014 at 12:32:13PM +0200, Markus Pargmann wrote: > On Mon, Sep 15, 2014 at 06:22:27PM +0800, Shengjiu Wang wrote: > > On Mon, Sep 15, 2014 at 12:05:41PM +0200, Markus Pargmann wrote: > > > On Fri, Sep 12, 2014 at 06:35:15PM +0800, Shengjiu Wang wrote: > > > > Check if ipg clock is in clock-names property, then we can move the > > > > ipg clock enable and disable operation to startup and shutdown, that > > > > is only enable ipg clock when ssi is working and keep clock is disabled > > > > when ssi is in idle. > > > > But when the checking is failed, remain the clock control as before. > > > > > > > > Signed-off-by: Shengjiu Wang > > > > --- > > > > V3 change log: > > > > update patch according Nicolin and markus's comments > > > > > > > > > > > > sound/soc/fsl/fsl_ssi.c | 53 ++++++++++++++++++++++++++++++++++++++++------- > > > > 1 file changed, 45 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c > > > > index 2fc3e66..6d1dfd5 100644 > > > > --- a/sound/soc/fsl/fsl_ssi.c > > > > +++ b/sound/soc/fsl/fsl_ssi.c > > > > @@ -169,6 +169,7 @@ struct fsl_ssi_private { > > > > u8 i2s_mode; > > > > bool use_dma; > > > > bool use_dual_fifo; > > > > + bool has_ipg_clk_name; > > > > unsigned int fifo_depth; > > > > struct fsl_ssi_rxtx_reg_val rxtx_reg_val; > > > > > > > > @@ -530,6 +531,11 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream, > > > > struct snd_soc_pcm_runtime *rtd = substream->private_data; > > > > struct fsl_ssi_private *ssi_private = > > > > snd_soc_dai_get_drvdata(rtd->cpu_dai); > > > > + int ret; > > > > + > > > > + ret = clk_prepare_enable(ssi_private->clk); > > > > + if (ret) > > > > + return ret; > > > > > > > > /* When using dual fifo mode, it is safer to ensure an even period > > > > * size. If appearing to an odd number while DMA always starts its > > > > @@ -544,6 +550,21 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream, > > > > } > > > > > > > > /** > > > > + * fsl_ssi_shutdown: shutdown the SSI > > > > + * > > > > + */ > > > > +static void fsl_ssi_shutdown(struct snd_pcm_substream *substream, > > > > + struct snd_soc_dai *dai) > > > > +{ > > > > + struct snd_soc_pcm_runtime *rtd = substream->private_data; > > > > + struct fsl_ssi_private *ssi_private = > > > > + snd_soc_dai_get_drvdata(rtd->cpu_dai); > > > > + > > > > + clk_disable_unprepare(ssi_private->clk); > > > > + > > > > +} > > > > + > > > > +/** > > > > * fsl_ssi_set_bclk - configure Digital Audio Interface bit clock > > > > * > > > > * Note: This function can be only called when using SSI as DAI master > > > > @@ -1043,6 +1064,7 @@ static int fsl_ssi_dai_probe(struct snd_soc_dai *dai) > > > > > > > > static const struct snd_soc_dai_ops fsl_ssi_dai_ops = { > > > > .startup = fsl_ssi_startup, > > > > + .shutdown = fsl_ssi_shutdown, > > > > .hw_params = fsl_ssi_hw_params, > > > > .hw_free = fsl_ssi_hw_free, > > > > .set_fmt = fsl_ssi_set_dai_fmt, > > > > @@ -1168,17 +1190,22 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev, > > > > u32 dmas[4]; > > > > int ret; > > > > > > > > - ssi_private->clk = devm_clk_get(&pdev->dev, NULL); > > > > + if (ssi_private->has_ipg_clk_name) > > > > + ssi_private->clk = devm_clk_get(&pdev->dev, "ipg"); > > > > + else > > > > + ssi_private->clk = devm_clk_get(&pdev->dev, NULL); > > > > if (IS_ERR(ssi_private->clk)) { > > > > ret = PTR_ERR(ssi_private->clk); > > > > dev_err(&pdev->dev, "could not get clock: %d\n", ret); > > > > return ret; > > > > } > > > > > > > > - ret = clk_prepare_enable(ssi_private->clk); > > > > - if (ret) { > > > > - dev_err(&pdev->dev, "clk_prepare_enable failed: %d\n", ret); > > > > - return ret; > > > > + if (!ssi_private->has_ipg_clk_name) { > > > > + ret = clk_prepare_enable(ssi_private->clk); > > > > + if (ret) { > > > > + dev_err(&pdev->dev, "clk_prepare_enable failed: %d\n", ret); > > > > + return ret; > > > > + } > > > > } > > > > > > > > /* For those SLAVE implementations, we ingore non-baudclk cases > > > > @@ -1236,8 +1263,9 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev, > > > > return 0; > > > > > > > > error_pcm: > > > > - clk_disable_unprepare(ssi_private->clk); > > > > > > > > + if (!ssi_private->has_ipg_clk_name) > > > > + clk_disable_unprepare(ssi_private->clk); > > > > return ret; > > > > } > > > > > > > > @@ -1246,7 +1274,8 @@ static void fsl_ssi_imx_clean(struct platform_device *pdev, > > > > { > > > > if (!ssi_private->use_dma) > > > > imx_pcm_fiq_exit(pdev); > > > > - clk_disable_unprepare(ssi_private->clk); > > > > + if (!ssi_private->has_ipg_clk_name) > > > > + clk_disable_unprepare(ssi_private->clk); > > > > } > > > > > > > > static int fsl_ssi_probe(struct platform_device *pdev) > > > > @@ -1321,8 +1350,16 @@ static int fsl_ssi_probe(struct platform_device *pdev) > > > > return -ENOMEM; > > > > } > > > > > > > > - ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem, > > > > + ret = of_property_match_string(np, "clock-names", "ipg"); > > > > + if (ret < 0) { > > > > + ssi_private->has_ipg_clk_name = false; > > > > + ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem, > > > > &fsl_ssi_regconfig); > > > > > > Sorry if I was unclear about that. My suggestion was to enable the clock > > > right here: > > > clk_prepare_enable(ssi_private->clk); > > > > > > Then you can remove ssi_private->has_ipg_clk_name and all > > > clk_prepare_enable() and clk_disable_unprepare() from above. Also you > > > can move the devm_clk_get() into this block. > > > > > > > ipg clock not only need to be enabled when accessing register, but also > > need to be enabled when ssi is working. So I add clk_enable in startup. > > > > > It seems you really want to implement this for devicetrees where the > > > "ipg" clock-name is missing, but I don't understand why? I really can't > > > see any benefit of adding all these clk_prepare_enable() calls for all > > > cornercases that may occure. For example the clocks for AC97 are still > > > missing in this version. > > > > When ipg clock-name is missing, I just remain the code logic as before. > > So AC97 case is same as before too. > > When have ipg clock-name, register it to regmap for accessing register, > > and enabling ipg clock in startup also is useful for AC97. > > > > I don't understand why you think clock for AC97 is still missing? > > Sorry got it all wrong, monday morning tiredness perhaps ;-). > > The patch looks good for me at the second look. I will test it later > today and give you feedback. > > Best regards, > > Markus Thank you very much. Your comments help me a lot. and wait your feedback. best regards wang shengjiu > > -- > Pengutronix e.K. | | > Industrial Linux Solutions | http://www.pengutronix.de/ | > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from na01-bl2-obe.outbound.protection.outlook.com (mail-bl2on0141.outbound.protection.outlook.com [65.55.169.141]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id CD0EE1A00C7 for ; Mon, 15 Sep 2014 20:37:40 +1000 (EST) Date: Mon, 15 Sep 2014 18:37:15 +0800 From: Shengjiu Wang To: Markus Pargmann Subject: Re: [PATCH V3] ASoC: fsl_ssi: refine ipg clock usage in this module Message-ID: <20140915103714.GB23877@audiosh1> References: <894383e00876763f22988fc5f3f9f232f939f923.1410517971.git.shengjiu.wang@freescale.com> <20140915100541.GD8844@pengutronix.de> <20140915102224.GA23877@audiosh1> <20140915103213.GE8844@pengutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" In-Reply-To: <20140915103213.GE8844@pengutronix.de> Cc: alsa-devel@alsa-project.org, lgirdwood@gmail.com, tiwai@suse.de, Li.Xiubo@freescale.com, timur@tabi.org, perex@perex.cz, nicoleotsuka@gmail.com, broonie@kernel.org, linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Mon, Sep 15, 2014 at 12:32:13PM +0200, Markus Pargmann wrote: > On Mon, Sep 15, 2014 at 06:22:27PM +0800, Shengjiu Wang wrote: > > On Mon, Sep 15, 2014 at 12:05:41PM +0200, Markus Pargmann wrote: > > > On Fri, Sep 12, 2014 at 06:35:15PM +0800, Shengjiu Wang wrote: > > > > Check if ipg clock is in clock-names property, then we can move the > > > > ipg clock enable and disable operation to startup and shutdown, that > > > > is only enable ipg clock when ssi is working and keep clock is disabled > > > > when ssi is in idle. > > > > But when the checking is failed, remain the clock control as before. > > > > > > > > Signed-off-by: Shengjiu Wang > > > > --- > > > > V3 change log: > > > > update patch according Nicolin and markus's comments > > > > > > > > > > > > sound/soc/fsl/fsl_ssi.c | 53 ++++++++++++++++++++++++++++++++++++++++------- > > > > 1 file changed, 45 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c > > > > index 2fc3e66..6d1dfd5 100644 > > > > --- a/sound/soc/fsl/fsl_ssi.c > > > > +++ b/sound/soc/fsl/fsl_ssi.c > > > > @@ -169,6 +169,7 @@ struct fsl_ssi_private { > > > > u8 i2s_mode; > > > > bool use_dma; > > > > bool use_dual_fifo; > > > > + bool has_ipg_clk_name; > > > > unsigned int fifo_depth; > > > > struct fsl_ssi_rxtx_reg_val rxtx_reg_val; > > > > > > > > @@ -530,6 +531,11 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream, > > > > struct snd_soc_pcm_runtime *rtd = substream->private_data; > > > > struct fsl_ssi_private *ssi_private = > > > > snd_soc_dai_get_drvdata(rtd->cpu_dai); > > > > + int ret; > > > > + > > > > + ret = clk_prepare_enable(ssi_private->clk); > > > > + if (ret) > > > > + return ret; > > > > > > > > /* When using dual fifo mode, it is safer to ensure an even period > > > > * size. If appearing to an odd number while DMA always starts its > > > > @@ -544,6 +550,21 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream, > > > > } > > > > > > > > /** > > > > + * fsl_ssi_shutdown: shutdown the SSI > > > > + * > > > > + */ > > > > +static void fsl_ssi_shutdown(struct snd_pcm_substream *substream, > > > > + struct snd_soc_dai *dai) > > > > +{ > > > > + struct snd_soc_pcm_runtime *rtd = substream->private_data; > > > > + struct fsl_ssi_private *ssi_private = > > > > + snd_soc_dai_get_drvdata(rtd->cpu_dai); > > > > + > > > > + clk_disable_unprepare(ssi_private->clk); > > > > + > > > > +} > > > > + > > > > +/** > > > > * fsl_ssi_set_bclk - configure Digital Audio Interface bit clock > > > > * > > > > * Note: This function can be only called when using SSI as DAI master > > > > @@ -1043,6 +1064,7 @@ static int fsl_ssi_dai_probe(struct snd_soc_dai *dai) > > > > > > > > static const struct snd_soc_dai_ops fsl_ssi_dai_ops = { > > > > .startup = fsl_ssi_startup, > > > > + .shutdown = fsl_ssi_shutdown, > > > > .hw_params = fsl_ssi_hw_params, > > > > .hw_free = fsl_ssi_hw_free, > > > > .set_fmt = fsl_ssi_set_dai_fmt, > > > > @@ -1168,17 +1190,22 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev, > > > > u32 dmas[4]; > > > > int ret; > > > > > > > > - ssi_private->clk = devm_clk_get(&pdev->dev, NULL); > > > > + if (ssi_private->has_ipg_clk_name) > > > > + ssi_private->clk = devm_clk_get(&pdev->dev, "ipg"); > > > > + else > > > > + ssi_private->clk = devm_clk_get(&pdev->dev, NULL); > > > > if (IS_ERR(ssi_private->clk)) { > > > > ret = PTR_ERR(ssi_private->clk); > > > > dev_err(&pdev->dev, "could not get clock: %d\n", ret); > > > > return ret; > > > > } > > > > > > > > - ret = clk_prepare_enable(ssi_private->clk); > > > > - if (ret) { > > > > - dev_err(&pdev->dev, "clk_prepare_enable failed: %d\n", ret); > > > > - return ret; > > > > + if (!ssi_private->has_ipg_clk_name) { > > > > + ret = clk_prepare_enable(ssi_private->clk); > > > > + if (ret) { > > > > + dev_err(&pdev->dev, "clk_prepare_enable failed: %d\n", ret); > > > > + return ret; > > > > + } > > > > } > > > > > > > > /* For those SLAVE implementations, we ingore non-baudclk cases > > > > @@ -1236,8 +1263,9 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev, > > > > return 0; > > > > > > > > error_pcm: > > > > - clk_disable_unprepare(ssi_private->clk); > > > > > > > > + if (!ssi_private->has_ipg_clk_name) > > > > + clk_disable_unprepare(ssi_private->clk); > > > > return ret; > > > > } > > > > > > > > @@ -1246,7 +1274,8 @@ static void fsl_ssi_imx_clean(struct platform_device *pdev, > > > > { > > > > if (!ssi_private->use_dma) > > > > imx_pcm_fiq_exit(pdev); > > > > - clk_disable_unprepare(ssi_private->clk); > > > > + if (!ssi_private->has_ipg_clk_name) > > > > + clk_disable_unprepare(ssi_private->clk); > > > > } > > > > > > > > static int fsl_ssi_probe(struct platform_device *pdev) > > > > @@ -1321,8 +1350,16 @@ static int fsl_ssi_probe(struct platform_device *pdev) > > > > return -ENOMEM; > > > > } > > > > > > > > - ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem, > > > > + ret = of_property_match_string(np, "clock-names", "ipg"); > > > > + if (ret < 0) { > > > > + ssi_private->has_ipg_clk_name = false; > > > > + ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem, > > > > &fsl_ssi_regconfig); > > > > > > Sorry if I was unclear about that. My suggestion was to enable the clock > > > right here: > > > clk_prepare_enable(ssi_private->clk); > > > > > > Then you can remove ssi_private->has_ipg_clk_name and all > > > clk_prepare_enable() and clk_disable_unprepare() from above. Also you > > > can move the devm_clk_get() into this block. > > > > > > > ipg clock not only need to be enabled when accessing register, but also > > need to be enabled when ssi is working. So I add clk_enable in startup. > > > > > It seems you really want to implement this for devicetrees where the > > > "ipg" clock-name is missing, but I don't understand why? I really can't > > > see any benefit of adding all these clk_prepare_enable() calls for all > > > cornercases that may occure. For example the clocks for AC97 are still > > > missing in this version. > > > > When ipg clock-name is missing, I just remain the code logic as before. > > So AC97 case is same as before too. > > When have ipg clock-name, register it to regmap for accessing register, > > and enabling ipg clock in startup also is useful for AC97. > > > > I don't understand why you think clock for AC97 is still missing? > > Sorry got it all wrong, monday morning tiredness perhaps ;-). > > The patch looks good for me at the second look. I will test it later > today and give you feedback. > > Best regards, > > Markus Thank you very much. Your comments help me a lot. and wait your feedback. best regards wang shengjiu > > -- > Pengutronix e.K. | | > Industrial Linux Solutions | http://www.pengutronix.de/ | > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |