| Message ID | 20170726185729.28897-2-krzk@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <krzk@kernel.org> > > --- > > 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
On Thu, Jul 27, 2017 at 10:41:35AM +0530, Arvind Yadav wrote: > 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 <krzk@kernel.org> > > > > --- > > > > 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. This should be rather separate change from the fix... unless it is a fix by itself. > > 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. Right, what a obfuscated code is this... Thanks for pointing this out. Also the s3c_i2sv2_probe() misses clk_prepare(). Best regards, Krzysztof > > @@ -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
On Thu, Jul 27, 2017 at 06:51:13PM +0200, Krzysztof Kozlowski wrote: > On Thu, Jul 27, 2017 at 10:41:35AM +0530, Arvind Yadav wrote: > > On Thursday 27 July 2017 12:27 AM, Krzysztof Kozlowski wrote: > > 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. > Right, what a obfuscated code is this... Thanks for pointing this out. > Also the s3c_i2sv2_probe() misses clk_prepare(). Yeah, I think the root cause here is that the code needs a cleanup so people can tell what's going on.
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")); ret = clk_prepare_enable(s3c2412_i2s.iis_cclk); if (ret) - return ret; + goto err; s3c2412_i2s.iis_cclk = s3c2412_i2s.iis_pclk; @@ -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); return 0; }
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 <krzk@kernel.org> --- 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(-)