Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751711AbdG0FMW (ORCPT ); Thu, 27 Jul 2017 01:12:22 -0400 Received: from mail-pf0-f196.google.com ([209.85.192.196]:33313 "EHLO mail-pf0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751481AbdG0FMV (ORCPT ); Thu, 27 Jul 2017 01:12:21 -0400 Subject: Re: [RFT 2/2] ASoC: samsung: Add proper error paths to s3c24xx I2S driver To: Krzysztof Kozlowski , Sangbeom Kim , Sylwester Nawrocki , Liam Girdwood , Mark Brown , Jaroslav Kysela , Takashi Iwai , alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org References: <20170726185729.28897-1-krzk@kernel.org> <20170726185729.28897-2-krzk@kernel.org> From: Arvind Yadav Message-ID: <49f6f3df-f7c9-cf7e-95ad-e3d6fdc78014@gmail.com> Date: Thu, 27 Jul 2017 10:41:35 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: <20170726185729.28897-2-krzk@kernel.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3559 Lines: 104 Hi, On Thursday 27 July 2017 12:27 AM, Krzysztof Kozlowski wrote: > s3c2412_i2s_probe() might fail so driver has to revert work done by > s3c_i2sv2_probe() (clock enabling). Missing doing this would lead to > clock enable in-balance. > > Signed-off-by: Krzysztof Kozlowski > > --- > > Please, kindly test on S3C24xx hardware. > --- > sound/soc/samsung/s3c-i2s-v2.c | 9 +++++++++ > sound/soc/samsung/s3c-i2s-v2.h | 7 +++++++ > sound/soc/samsung/s3c2412-i2s.c | 11 +++++++++-- > 3 files changed, 25 insertions(+), 2 deletions(-) > > diff --git a/sound/soc/samsung/s3c-i2s-v2.c b/sound/soc/samsung/s3c-i2s-v2.c > index 9b28046eea8e..df83f2730a53 100644 > --- a/sound/soc/samsung/s3c-i2s-v2.c > +++ b/sound/soc/samsung/s3c-i2s-v2.c > @@ -651,6 +651,15 @@ int s3c_i2sv2_probe(struct snd_soc_dai *dai, > } > EXPORT_SYMBOL_GPL(s3c_i2sv2_probe); > > +void s3c_i2sv2_cleanup(struct snd_soc_dai *dai, > + struct s3c_i2sv2_info *i2s) > +{ > + clk_disable(i2s->iis_pclk); > + clk_put(i2s->iis_pclk); > + i2s->iis_pclk = NULL; > +} > +EXPORT_SYMBOL_GPL(s3c_i2sv2_cleanup); > + > #ifdef CONFIG_PM > static int s3c2412_i2s_suspend(struct snd_soc_dai *dai) > { > diff --git a/sound/soc/samsung/s3c-i2s-v2.h b/sound/soc/samsung/s3c-i2s-v2.h > index 182d80564e37..3fca20f7a853 100644 > --- a/sound/soc/samsung/s3c-i2s-v2.h > +++ b/sound/soc/samsung/s3c-i2s-v2.h > @@ -92,6 +92,13 @@ extern int s3c_i2sv2_probe(struct snd_soc_dai *dai, > unsigned long base); > > /** > + * s3c_i2sv2_cleanup - cleanup resources allocated in s3c_i2sv2_probe > + * @dai: The ASoC DAI structure supplied to the original probe. > + * @i2s: Our local i2s structure to fill in. > + */ > +extern void s3c_i2sv2_cleanup(struct snd_soc_dai *dai, > + struct s3c_i2sv2_info *i2s); > +/** > * s3c_i2sv2_register_component - register component and dai with soc core > * @dev: DAI device > * @id: DAI ID > diff --git a/sound/soc/samsung/s3c2412-i2s.c b/sound/soc/samsung/s3c2412-i2s.c > index bcd1cbdeac93..005cd6db7d07 100644 > --- a/sound/soc/samsung/s3c2412-i2s.c > +++ b/sound/soc/samsung/s3c2412-i2s.c > @@ -65,7 +65,8 @@ static int s3c2412_i2s_probe(struct snd_soc_dai *dai) > s3c2412_i2s.iis_cclk = devm_clk_get(dai->dev, "i2sclk"); > if (IS_ERR(s3c2412_i2s.iis_cclk)) { > pr_err("failed to get i2sclk clock\n"); > - return PTR_ERR(s3c2412_i2s.iis_cclk); > + ret = PTR_ERR(s3c2412_i2s.iis_cclk); > + goto err; > } > > /* Set MPLL as the source for IIS CLK */ > @@ -73,7 +74,7 @@ static int s3c2412_i2s_probe(struct snd_soc_dai *dai) > clk_set_parent(s3c2412_i2s.iis_cclk, clk_get(NULL, "mpll")); we can use devm_clk_get for "mpll" clock. > ret = clk_prepare_enable(s3c2412_i2s.iis_cclk); > if (ret) > - return ret; > + goto err; > > s3c2412_i2s.iis_cclk = s3c2412_i2s.iis_pclk; Now s3c2412_i2s.iis_cclk and s3c2412_i2s.iis_pclk are holding "iis" clock. Now no one handling "mpll" clock. If we will call s3c2412_i2s_remove. It's means. we are disabling "iis" clock twice. Which is not correct. > > @@ -82,11 +83,17 @@ static int s3c2412_i2s_probe(struct snd_soc_dai *dai) > S3C_GPIO_PULL_NONE); > > return 0; > + > +err: > + s3c_i2sv2_cleanup(dai, &s3c2412_i2s); > + > + return ret; > } > > static int s3c2412_i2s_remove(struct snd_soc_dai *dai) > { > clk_disable_unprepare(s3c2412_i2s.iis_cclk); > + s3c_i2sv2_cleanup(dai, &s3c2412_i2s); here, We are disabling "iis" clock twice. please check my previous comment. > > return 0; > } ~arvind