[PATCH v2 1/4] ASoC: imx-sgtl5000: Do not enter the error path on success

[PATCH v2 1/4] ASoC: imx-sgtl5000: Do not enter the error path on success

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@freescale.com>

Return on success instead of entering the error path.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
Changes since v1:
- Keep of_node_put in the success path.

 sound/soc/fsl/imx-sgtl5000.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/sound/soc/fsl/imx-sgtl5000.c b/sound/soc/fsl/imx-sgtl5000.c
index 9584e78..5a6aaa3 100644
--- a/sound/soc/fsl/imx-sgtl5000.c
+++ b/sound/soc/fsl/imx-sgtl5000.c
@@ -174,6 +174,11 @@ static int imx_sgtl5000_probe(struct platform_device *pdev)
 	}
 
 	platform_set_drvdata(pdev, data);
+	of_node_put(ssi_np);
+	of_node_put(codec_np);
+
+	return 0;
+
 clk_fail:
 	clk_put(data->codec_clk);
 fail:
-- 
1.7.9.5

[PATCH v2 2/4] ASoC: imx-sgtl5000: Let the code clock be a mandatory binding

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@freescale.com>

Currently passing a codec clock is optional.

Make the codec clock to be a required binding in order to simplify codec clock 
handling in imx-sgtl5000.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
Changes since v1:
- Rebased against Shawn's tree and remove mx53qsb clock fix, as it was already
applied on Shawn's tree
- Confirmed that audio is working on mx51babbage.

 arch/arm/boot/dts/imx51-babbage.dts |   13 ++++++++++++-
 sound/soc/fsl/imx-sgtl5000.c        |   18 ++++++------------
 2 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/arch/arm/boot/dts/imx51-babbage.dts b/arch/arm/boot/dts/imx51-babbage.dts
index 6dd9486..5318d26 100644
--- a/arch/arm/boot/dts/imx51-babbage.dts
+++ b/arch/arm/boot/dts/imx51-babbage.dts
@@ -61,6 +61,16 @@
 		mux-int-port = <2>;
 		mux-ext-port = <3>;
 	};
+
+	clocks {
+		clk_26M: clock {
+			compatible = "fixed-clock";
+			reg=<0>;
+			#clock-cells = <0>;
+			clock-frequency = <26000000>;
+			gpios = <&gpio4 26 1>;
+		};
+	};
 };
 
 &esdhc1 {
@@ -229,6 +239,7 @@
 				MX51_PAD_EIM_A27__GPIO2_21   0x5
 				MX51_PAD_CSPI1_SS0__GPIO4_24 0x85
 				MX51_PAD_CSPI1_SS1__GPIO4_25 0x85
+				MX51_PAD_CSPI1_RDY__GPIO4_26 0x80000000
 			>;
 		};
 	};
@@ -255,7 +266,7 @@
 	sgtl5000: codec@0a {
 		compatible = "fsl,sgtl5000";
 		reg = <0x0a>;
-		clock-frequency = <26000000>;
+		clocks = <&clk_26M>;
 		VDDA-supply = <&vdig_reg>;
 		VDDIO-supply = <&vvideo_reg>;
 	};
diff --git a/sound/soc/fsl/imx-sgtl5000.c b/sound/soc/fsl/imx-sgtl5000.c
index fa308b1..6198ca5 100644
--- a/sound/soc/fsl/imx-sgtl5000.c
+++ b/sound/soc/fsl/imx-sgtl5000.c
@@ -130,20 +130,14 @@ static int imx_sgtl5000_probe(struct platform_device *pdev)
 
 	data->codec_clk = clk_get(&codec_dev->dev, NULL);
 	if (IS_ERR(data->codec_clk)) {
-		/* assuming clock enabled by default */
-		data->codec_clk = NULL;
-		ret = of_property_read_u32(codec_np, "clock-frequency",
-					&data->clk_frequency);
-		if (ret) {
-			dev_err(&codec_dev->dev,
-				"clock-frequency missing or invalid\n");
-			goto fail;
-		}
-	} else {
-		data->clk_frequency = clk_get_rate(data->codec_clk);
-		clk_prepare_enable(data->codec_clk);
+		ret = PTR_ERR(data->codec_clk);
+		dev_err(&codec_dev->dev, "could not get codec clk: %d\n", ret);
+		goto fail;
 	}
 
+	data->clk_frequency = clk_get_rate(data->codec_clk);
+	clk_prepare_enable(data->codec_clk);
+
 	data->dai.name = "HiFi";
 	data->dai.stream_name = "HiFi";
 	data->dai.codec_dai_name = "sgtl5000";
-- 
1.7.9.5

[PATCH v2 3/4] ASoC: imx-sgtl5000: Do a sanity check on the codec clock

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@freescale.com>

Only turn on the codec clock if it is within the valid range.

Also, disable the codec clock on the clk_fail path.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
Changes since v1:
- None

 sound/soc/fsl/imx-sgtl5000.c |   18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/sound/soc/fsl/imx-sgtl5000.c b/sound/soc/fsl/imx-sgtl5000.c
index 9c286e6..18af815 100644
--- a/sound/soc/fsl/imx-sgtl5000.c
+++ b/sound/soc/fsl/imx-sgtl5000.c
@@ -56,6 +56,15 @@ static const struct snd_soc_dapm_widget imx_sgtl5000_dapm_widgets[] = {
 	SND_SOC_DAPM_SPK("Ext Spk", NULL),
 };
 
+static int sgtl5000_is_valid_sysclk(int freq)
+{
+
+	if (freq < 8000000 || freq > 27000000)
+		return -EINVAL;
+	else
+		return 0;
+}
+
 static int imx_sgtl5000_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
@@ -135,9 +144,13 @@ static int imx_sgtl5000_probe(struct platform_device *pdev)
 		goto fail;
 	}
 
-	data->clk_frequency = clk_get_rate(data->codec_clk);
-	clk_prepare_enable(data->codec_clk);
+	data->clk_frequency = clk_get_rate(data->codec_clk);
+	ret = sgtl5000_is_valid_sysclk(data->clk_frequency);
+	if (ret)
+		goto fail;
 
+	clk_prepare_enable(data->codec_clk);
+
 	data->dai.name = "HiFi";
 	data->dai.stream_name = "HiFi";
 	data->dai.codec_dai_name = "sgtl5000";
@@ -172,6 +185,7 @@ static int imx_sgtl5000_probe(struct platform_device *pdev)
 	return 0;
 
 clk_fail:
+	clk_disable_unprepare(data->codec_clk);
 	clk_put(data->codec_clk);
 fail:
 	if (ssi_np)
-- 
1.7.9.5

[PATCH v2 4/4] ASoC: imx-sgtl5000: Convert to devm_clk_get()

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@freescale.com>

Converting to devm_clk_get() can simplify the code a bit.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
Changes since v1:
- None

 sound/soc/fsl/imx-sgtl5000.c |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/sound/soc/fsl/imx-sgtl5000.c b/sound/soc/fsl/imx-sgtl5000.c
index 18af815..c869996 100644
--- a/sound/soc/fsl/imx-sgtl5000.c
+++ b/sound/soc/fsl/imx-sgtl5000.c
@@ -137,7 +137,7 @@ static int imx_sgtl5000_probe(struct platform_device *pdev)
 		goto fail;
 	}
 
-	data->codec_clk = clk_get(&codec_dev->dev, NULL);
+	data->codec_clk = devm_clk_get(&codec_dev->dev, NULL);
 	if (IS_ERR(data->codec_clk)) {
 		ret = PTR_ERR(data->codec_clk);
 		dev_err(&codec_dev->dev, "could not get codec clk: %d\n", ret);
@@ -186,7 +186,6 @@ static int imx_sgtl5000_probe(struct platform_device *pdev)
 
 clk_fail:
 	clk_disable_unprepare(data->codec_clk);
-	clk_put(data->codec_clk);
 fail:
 	if (ssi_np)
 		of_node_put(ssi_np);
@@ -200,10 +199,7 @@ static int imx_sgtl5000_remove(struct platform_device *pdev)
 {
 	struct imx_sgtl5000_data *data = platform_get_drvdata(pdev);
 
-	if (data->codec_clk) {
-		clk_disable_unprepare(data->codec_clk);
-		clk_put(data->codec_clk);
-	}
+	clk_disable_unprepare(data->codec_clk);
 	snd_soc_unregister_card(&data->card);
 
 	return 0;
-- 
1.7.9.5

Re: [PATCH v2 1/4] ASoC: imx-sgtl5000: Do not enter the error path on success

[Shawn Guo]

On Wed, Apr 24, 2013 at 11:54:43AM -0300, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> Return on success instead of entering the error path.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>

Acked-by: Shawn Guo <shawn.guo@linaro.org>

> ---
> Changes since v1:
> - Keep of_node_put in the success path.
> 
>  sound/soc/fsl/imx-sgtl5000.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/sound/soc/fsl/imx-sgtl5000.c b/sound/soc/fsl/imx-sgtl5000.c
> index 9584e78..5a6aaa3 100644
> --- a/sound/soc/fsl/imx-sgtl5000.c
> +++ b/sound/soc/fsl/imx-sgtl5000.c
> @@ -174,6 +174,11 @@ static int imx_sgtl5000_probe(struct platform_device *pdev)
>  	}
>  
>  	platform_set_drvdata(pdev, data);
> +	of_node_put(ssi_np);
> +	of_node_put(codec_np);
> +
> +	return 0;
> +
>  clk_fail:
>  	clk_put(data->codec_clk);
>  fail:
> -- 
> 1.7.9.5
> 

Re: [PATCH v2 2/4] ASoC: imx-sgtl5000: Let the code clock be a mandatory binding

[Shawn Guo]

On Wed, Apr 24, 2013 at 11:54:44AM -0300, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> Currently passing a codec clock is optional.
> 
> Make the codec clock to be a required binding in order to simplify codec clock 
> handling in imx-sgtl5000.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
> Changes since v1:
> - Rebased against Shawn's tree and remove mx53qsb clock fix, as it was already
> applied on Shawn's tree

It seems that you expect the patch to go via my tree?  In that case, I
need Mark's ACK.

> - Confirmed that audio is working on mx51babbage.
> 
>  arch/arm/boot/dts/imx51-babbage.dts |   13 ++++++++++++-
>  sound/soc/fsl/imx-sgtl5000.c        |   18 ++++++------------
>  2 files changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/imx51-babbage.dts b/arch/arm/boot/dts/imx51-babbage.dts
> index 6dd9486..5318d26 100644
> --- a/arch/arm/boot/dts/imx51-babbage.dts
> +++ b/arch/arm/boot/dts/imx51-babbage.dts
> @@ -61,6 +61,16 @@
>  		mux-int-port = <2>;
>  		mux-ext-port = <3>;
>  	};
> +
> +	clocks {
> +		clk_26M: clock {

The name "clock" is too generic.

Shawn

> +			compatible = "fixed-clock";
> +			reg=<0>;
> +			#clock-cells = <0>;
> +			clock-frequency = <26000000>;
> +			gpios = <&gpio4 26 1>;
> +		};
> +	};
>  };
>  
>  &esdhc1 {
> @@ -229,6 +239,7 @@
>  				MX51_PAD_EIM_A27__GPIO2_21   0x5
>  				MX51_PAD_CSPI1_SS0__GPIO4_24 0x85
>  				MX51_PAD_CSPI1_SS1__GPIO4_25 0x85
> +				MX51_PAD_CSPI1_RDY__GPIO4_26 0x80000000
>  			>;
>  		};
>  	};
> @@ -255,7 +266,7 @@
>  	sgtl5000: codec@0a {
>  		compatible = "fsl,sgtl5000";
>  		reg = <0x0a>;
> -		clock-frequency = <26000000>;
> +		clocks = <&clk_26M>;
>  		VDDA-supply = <&vdig_reg>;
>  		VDDIO-supply = <&vvideo_reg>;
>  	};
> diff --git a/sound/soc/fsl/imx-sgtl5000.c b/sound/soc/fsl/imx-sgtl5000.c
> index fa308b1..6198ca5 100644
> --- a/sound/soc/fsl/imx-sgtl5000.c
> +++ b/sound/soc/fsl/imx-sgtl5000.c
> @@ -130,20 +130,14 @@ static int imx_sgtl5000_probe(struct platform_device *pdev)
>  
>  	data->codec_clk = clk_get(&codec_dev->dev, NULL);
>  	if (IS_ERR(data->codec_clk)) {
> -		/* assuming clock enabled by default */
> -		data->codec_clk = NULL;
> -		ret = of_property_read_u32(codec_np, "clock-frequency",
> -					&data->clk_frequency);
> -		if (ret) {
> -			dev_err(&codec_dev->dev,
> -				"clock-frequency missing or invalid\n");
> -			goto fail;
> -		}
> -	} else {
> -		data->clk_frequency = clk_get_rate(data->codec_clk);
> -		clk_prepare_enable(data->codec_clk);
> +		ret = PTR_ERR(data->codec_clk);
> +		dev_err(&codec_dev->dev, "could not get codec clk: %d\n", ret);
> +		goto fail;
>  	}
>  
> +	data->clk_frequency = clk_get_rate(data->codec_clk);
> +	clk_prepare_enable(data->codec_clk);
> +
>  	data->dai.name = "HiFi";
>  	data->dai.stream_name = "HiFi";
>  	data->dai.codec_dai_name = "sgtl5000";
> -- 
> 1.7.9.5
> 

Re: [PATCH v2 3/4] ASoC: imx-sgtl5000: Do a sanity check on the codec clock

[Shawn Guo]

On Wed, Apr 24, 2013 at 11:54:45AM -0300, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> Only turn on the codec clock if it is within the valid range.
> 
> Also, disable the codec clock on the clk_fail path.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
> Changes since v1:
> - None
> 
>  sound/soc/fsl/imx-sgtl5000.c |   18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/fsl/imx-sgtl5000.c b/sound/soc/fsl/imx-sgtl5000.c
> index 9c286e6..18af815 100644
> --- a/sound/soc/fsl/imx-sgtl5000.c
> +++ b/sound/soc/fsl/imx-sgtl5000.c
> @@ -56,6 +56,15 @@ static const struct snd_soc_dapm_widget imx_sgtl5000_dapm_widgets[] = {
>  	SND_SOC_DAPM_SPK("Ext Spk", NULL),
>  };
>  
> +static int sgtl5000_is_valid_sysclk(int freq)
> +{
> +
> +	if (freq < 8000000 || freq > 27000000)
> +		return -EINVAL;
> +	else
> +		return 0;
> +}
> +
>  static int imx_sgtl5000_probe(struct platform_device *pdev)
>  {
>  	struct device_node *np = pdev->dev.of_node;
> @@ -135,9 +144,13 @@ static int imx_sgtl5000_probe(struct platform_device *pdev)
>  		goto fail;
>  	}
>  
> -	data->clk_frequency = clk_get_rate(data->codec_clk);
> -	clk_prepare_enable(data->codec_clk);
> +	data->clk_frequency = clk_get_rate(data->codec_clk);
> +	ret = sgtl5000_is_valid_sysclk(data->clk_frequency);
> +	if (ret)
> +		goto fail;

In this patch, you haven't converted clk_get to devm_clk_get yet.  If
you goto "fail" here, clk_put() below will be skipped.

Shawn

>  
> +	clk_prepare_enable(data->codec_clk);
> +
>  	data->dai.name = "HiFi";
>  	data->dai.stream_name = "HiFi";
>  	data->dai.codec_dai_name = "sgtl5000";
> @@ -172,6 +185,7 @@ static int imx_sgtl5000_probe(struct platform_device *pdev)
>  	return 0;
>  
>  clk_fail:
> +	clk_disable_unprepare(data->codec_clk);
>  	clk_put(data->codec_clk);
>  fail:
>  	if (ssi_np)
> -- 
> 1.7.9.5
> 

Re: [PATCH v2 4/4] ASoC: imx-sgtl5000: Convert to devm_clk_get()

[Shawn Guo]

On Wed, Apr 24, 2013 at 11:54:46AM -0300, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> Converting to devm_clk_get() can simplify the code a bit.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>

Acked-by: Shawn Guo <shawn.guo@linaro.org>

Re: [PATCH v2 1/4] ASoC: imx-sgtl5000: Do not enter the error path on success

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 198 bytes --]

On Wed, Apr 24, 2013 at 11:54:43AM -0300, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> Return on success instead of entering the error path.

Applied, thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 2/4] ASoC: imx-sgtl5000: Let the code clock be a mandatory binding

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 486 bytes --]

On Thu, Apr 25, 2013 at 01:16:55PM +0800, Shawn Guo wrote:
> On Wed, Apr 24, 2013 at 11:54:44AM -0300, Fabio Estevam wrote:

> > - Rebased against Shawn's tree and remove mx53qsb clock fix, as it was already
> > applied on Shawn's tree

> It seems that you expect the patch to go via my tree?  In that case, I
> need Mark's ACK.

Well, it's a bit late for ARM changes to go in so I guess it'll be
sorted out after the merge window anyway and can be applied to the
ASoC tree after that?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 2/4] ASoC: imx-sgtl5000: Let the code clock be a mandatory binding

[Shawn Guo]

On Thu, Apr 25, 2013 at 12:01:27PM +0100, Mark Brown wrote:
> On Thu, Apr 25, 2013 at 01:16:55PM +0800, Shawn Guo wrote:
> > On Wed, Apr 24, 2013 at 11:54:44AM -0300, Fabio Estevam wrote:
> 
> > > - Rebased against Shawn's tree and remove mx53qsb clock fix, as it was already
> > > applied on Shawn's tree
> 
> > It seems that you expect the patch to go via my tree?  In that case, I
> > need Mark's ACK.
> 
> Well, it's a bit late for ARM changes to go in so I guess it'll be
> sorted out after the merge window anyway and can be applied to the
> ASoC tree after that?

Even worse.  The dependant patches in my tree are 3.11 material.

Shawn

Re: [PATCH v2 2/4] ASoC: imx-sgtl5000: Let the code clock be a mandatory binding

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1049 bytes --]

On Thu, Apr 25, 2013 at 10:35:24PM +0800, Shawn Guo wrote:
> On Thu, Apr 25, 2013 at 12:01:27PM +0100, Mark Brown wrote:
> > On Thu, Apr 25, 2013 at 01:16:55PM +0800, Shawn Guo wrote:
> > > On Wed, Apr 24, 2013 at 11:54:44AM -0300, Fabio Estevam wrote:

> > > > - Rebased against Shawn's tree and remove mx53qsb clock fix, as it was already
> > > > applied on Shawn's tree

> > > It seems that you expect the patch to go via my tree?  In that case, I
> > > need Mark's ACK.

> > Well, it's a bit late for ARM changes to go in so I guess it'll be
> > sorted out after the merge window anyway and can be applied to the
> > ASoC tree after that?

> Even worse.  The dependant patches in my tree are 3.11 material.

Oh, that's distressing - I guess there's more than just the fix
mentioned above?  Would it be possible for you to provide a branch I can
pull into my tree for v3.11?  My workflow is easier if I don't have to
worry about having potential collisions with other trees.  If it'd be
too inconvenient for you then I can cope but it'd be nice.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 2/4] ASoC: imx-sgtl5000: Let the code clock be a mandatory binding

[Shawn Guo]

On Wed, May 01, 2013 at 05:53:01PM +0100, Mark Brown wrote:
> On Thu, Apr 25, 2013 at 10:35:24PM +0800, Shawn Guo wrote:
> > On Thu, Apr 25, 2013 at 12:01:27PM +0100, Mark Brown wrote:
> > > On Thu, Apr 25, 2013 at 01:16:55PM +0800, Shawn Guo wrote:
> > > > On Wed, Apr 24, 2013 at 11:54:44AM -0300, Fabio Estevam wrote:
> 
> > > > > - Rebased against Shawn's tree and remove mx53qsb clock fix, as it was already
> > > > > applied on Shawn's tree
> 
> > > > It seems that you expect the patch to go via my tree?  In that case, I
> > > > need Mark's ACK.
> 
> > > Well, it's a bit late for ARM changes to go in so I guess it'll be
> > > sorted out after the merge window anyway and can be applied to the
> > > ASoC tree after that?
> 
> > Even worse.  The dependant patches in my tree are 3.11 material.
> 
> Oh, that's distressing - I guess there's more than just the fix
> mentioned above?

Yes, a couple of patches.

> Would it be possible for you to provide a branch I can
> pull into my tree for v3.11?

Ok, I will try to once we have a -rc to be the base.

Shawn

> My workflow is easier if I don't have to
> worry about having potential collisions with other trees.  If it'd be
> too inconvenient for you then I can cope but it'd be nice.