[RFT,2/2] ASoC: samsung: Add proper error paths to s3c24xx I2S driver
diff mbox

Message ID 20170726185729.28897-2-krzk@kernel.org
State New
Headers show

Commit Message

Krzysztof Kozlowski July 26, 2017, 6:57 p.m. UTC
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(-)

Comments

Arvind Yadav July 27, 2017, 5:11 a.m. UTC | #1
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
Krzysztof Kozlowski July 27, 2017, 4:51 p.m. UTC | #2
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
Mark Brown July 27, 2017, 4:56 p.m. UTC | #3
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.

Patch
diff mbox

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;
 }