[alsa-devel] [PATCH 1/4] ASoC: imx-sgtl5000: Do not enter the error path on success
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 --- sound/soc/fsl/imx-sgtl5000.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/sound/soc/fsl/imx-sgtl5000.c b/sound/soc/fsl/imx-sgtl5000.c index 9584e78..88fc02c 100644 --- a/sound/soc/fsl/imx-sgtl5000.c +++ b/sound/soc/fsl/imx-sgtl5000.c @@ -174,6 +174,9 @@ static int imx_sgtl5000_probe(struct platform_device *pdev) }
platform_set_drvdata(pdev, data); + + return 0; + clk_fail: clk_put(data->codec_clk); fail:
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 --- Mark/Shawn/Sascha,
I am marking this one as RFC because I don't have mx51babbage nor mx53qsb handy to test it.
Will test on these platforms when I have a chance, but just wanted to share it now and maybe get some feedback first.
arch/arm/boot/dts/imx51-babbage.dts | 13 ++++++++++++- arch/arm/boot/dts/imx53-qsb.dts | 1 + arch/arm/mach-imx/mach-imx53.c | 16 ---------------- sound/soc/fsl/imx-sgtl5000.c | 18 ++++++------------ 4 files changed, 19 insertions(+), 29 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/arch/arm/boot/dts/imx53-qsb.dts b/arch/arm/boot/dts/imx53-qsb.dts index 160d1bc..27e56e0 100644 --- a/arch/arm/boot/dts/imx53-qsb.dts +++ b/arch/arm/boot/dts/imx53-qsb.dts @@ -145,6 +145,7 @@ sgtl5000: codec@0a { compatible = "fsl,sgtl5000"; reg = <0x0a>; + clocks = <&clks 150>; VDDA-supply = <®_3p2v>; VDDIO-supply = <®_3p2v>; }; diff --git a/arch/arm/mach-imx/mach-imx53.c b/arch/arm/mach-imx/mach-imx53.c index f579c61..8d47571 100644 --- a/arch/arm/mach-imx/mach-imx53.c +++ b/arch/arm/mach-imx/mach-imx53.c @@ -23,24 +23,8 @@ #include "common.h" #include "mx53.h"
-static void __init imx53_qsb_init(void) -{ - struct clk *clk; - - clk = clk_get_sys(NULL, "ssi_ext1"); - if (IS_ERR(clk)) { - pr_err("failed to get clk ssi_ext1\n"); - return; - } - - clk_register_clkdev(clk, NULL, "0-000a"); -} - static void __init imx53_dt_init(void) { - if (of_machine_is_compatible("fsl,imx53-qsb")) - imx53_qsb_init(); - of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); }
diff --git a/sound/soc/fsl/imx-sgtl5000.c b/sound/soc/fsl/imx-sgtl5000.c index 88fc02c..9c286e6 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";
On Wed, Apr 17, 2013 at 08:55:19PM -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
Mark/Shawn/Sascha,
I am marking this one as RFC because I don't have mx51babbage nor mx53qsb handy to test it.
It works on imx53-qsb but doe not on imx5q-babbage. That clk_26M is not registered to clock framework as a clk provider.
Shawn
Will test on these platforms when I have a chance, but just wanted to share it now and maybe get some feedback first.
arch/arm/boot/dts/imx51-babbage.dts | 13 ++++++++++++- arch/arm/boot/dts/imx53-qsb.dts | 1 + arch/arm/mach-imx/mach-imx53.c | 16 ---------------- sound/soc/fsl/imx-sgtl5000.c | 18 ++++++------------ 4 files changed, 19 insertions(+), 29 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>;diff --git a/arch/arm/boot/dts/imx53-qsb.dts b/arch/arm/boot/dts/imx53-qsb.dts index 160d1bc..27e56e0 100644 --- a/arch/arm/boot/dts/imx53-qsb.dts +++ b/arch/arm/boot/dts/imx53-qsb.dts @@ -145,6 +145,7 @@ sgtl5000: codec@0a { compatible = "fsl,sgtl5000"; reg = <0x0a>;
clocks = <&clks 150>;diff --git a/arch/arm/mach-imx/mach-imx53.c b/arch/arm/mach-imx/mach-imx53.c index f579c61..8d47571 100644 --- a/arch/arm/mach-imx/mach-imx53.c +++ b/arch/arm/mach-imx/mach-imx53.c @@ -23,24 +23,8 @@ #include "common.h" #include "mx53.h"
-static void __init imx53_qsb_init(void) -{
- struct clk *clk;
- clk = clk_get_sys(NULL, "ssi_ext1");
- if (IS_ERR(clk)) {
pr_err("failed to get clk ssi_ext1\n");return;- }
- clk_register_clkdev(clk, NULL, "0-000a");
-}
static void __init imx53_dt_init(void) {
- if (of_machine_is_compatible("fsl,imx53-qsb"))
imx53_qsb_init();- of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
}
diff --git a/sound/soc/fsl/imx-sgtl5000.c b/sound/soc/fsl/imx-sgtl5000.c index 88fc02c..9c286e6 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
On Thu, Apr 18, 2013 at 12:36 AM, Shawn Guo shawn.guo@linaro.org wrote:
I am marking this one as RFC because I don't have mx51babbage nor mx53qsb handy to test it.
It works on imx53-qsb but doe not on imx5q-babbage. That clk_26M is not registered to clock framework as a clk provider.
Thanks for testing it, Shawn.
I will investigate this when I get access to a mx51-babbage.
If anyone has any hints as to how register this clk_26M properly via device tree, please let me know.
Thanks,
Fabio Estevam
On Thu, Apr 18, 2013 at 12:56:15AM -0300, Fabio Estevam wrote:
On Thu, Apr 18, 2013 at 12:36 AM, Shawn Guo shawn.guo@linaro.org wrote:
I am marking this one as RFC because I don't have mx51babbage nor mx53qsb handy to test it.
It works on imx53-qsb but doe not on imx5q-babbage. That clk_26M is not registered to clock framework as a clk provider.
Thanks for testing it, Shawn.
I will investigate this when I get access to a mx51-babbage.
If anyone has any hints as to how register this clk_26M properly via device tree, please let me know.
Though it's still under review, the patch [1] from Martin Fuzzey is just what you need.
Shawn
On Thu, Apr 18, 2013 at 3:41 AM, Shawn Guo shawn.guo@linaro.org wrote:
Though it's still under review, the patch [1] from Martin Fuzzey is just what you need.
Excellent, just applied Martin's patch and audio is functional on mx51babbage now.
I will repost the series after Martin's patch gets applied.
Thanks,
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 --- 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)
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 --- 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;
On Wed, Apr 17, 2013 at 08:55:18PM -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
sound/soc/fsl/imx-sgtl5000.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/sound/soc/fsl/imx-sgtl5000.c b/sound/soc/fsl/imx-sgtl5000.c index 9584e78..88fc02c 100644 --- a/sound/soc/fsl/imx-sgtl5000.c +++ b/sound/soc/fsl/imx-sgtl5000.c @@ -174,6 +174,9 @@ static int imx_sgtl5000_probe(struct platform_device *pdev) }
platform_set_drvdata(pdev, data);
- return 0;
This does not look correct. Doing so will skip clk_put() and of_node_put() on success.
Shawn
clk_fail: clk_put(data->codec_clk); fail: -- 1.7.9.5
On Thu, Apr 18, 2013 at 09:29:40AM +0800, Shawn Guo wrote:
On Wed, Apr 17, 2013 at 08:55:18PM -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
sound/soc/fsl/imx-sgtl5000.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/sound/soc/fsl/imx-sgtl5000.c b/sound/soc/fsl/imx-sgtl5000.c index 9584e78..88fc02c 100644 --- a/sound/soc/fsl/imx-sgtl5000.c +++ b/sound/soc/fsl/imx-sgtl5000.c @@ -174,6 +174,9 @@ static int imx_sgtl5000_probe(struct platform_device *pdev) }
platform_set_drvdata(pdev, data);
- return 0;
This does not look correct. Doing so will skip clk_put() and of_node_put() on success.
Well, clk_put() shouldn't be called on success, but of_node_put() should.
Shawn
On Wed, Apr 17, 2013 at 10:38 PM, Shawn Guo shawn.guo@linaro.org wrote:
Well, clk_put() shouldn't be called on success, but of_node_put() should.
Ok, then tomorrow I will send v2 as:
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:
participants (2)
-
Fabio Estevam -
Shawn Guo