[v4,4/4] ASoC: samsung: Add machine driver for Exynos5433 based TM2 board

Commit Message

Chanwoo Choi July 21, 2016, 10:28 a.m. UTC

Hi Sylwester,

On 2016년 07월 18일 19:41, Sylwester Nawrocki wrote:
> Hi Chanwoo,
> 
> On 07/15/2016 07:18 AM, Chanwoo Choi wrote:
>>> +static int tm2_probe(struct platform_device *pdev)
>>> +{
>>> +	struct device *dev = &pdev->dev;
> 
>>> +	codec_dai_node = of_parse_phandle(dev->of_node, "audio-codec", 0);
>>> +	if (!codec_dai_node) {
>>> +		dev_err(dev, "audio-codec property invalid or missing\n");
>>> +		ret = -EINVAL;
>>> +		goto err_put_cpu_dai;
>>> +	}
> 
>>> +	priv->codec_mclk1 = of_clk_get_by_name(codec_dai_node, "mclk1");
>>> +	if (IS_ERR(priv->codec_mclk1)) {
>>> +		dev_err(dev, "Failed to get mclk1 clock\n");
>>> +		ret = PTR_ERR(priv->codec_mclk1);
>>> +		goto err_put_codec_dai;
>>> +	}
>>
>> I think that you better to use the devm_clk_get() instead of of_clk_get_by_name()
>> because you don't need to handle the clk_put() when error happen and remove the 
>> this driver.
>>
>> 	priv->codec_mclk1 = devm_clk_get(dev, "mclk1");
> 
> The clocks are from the CODEC DT node, for which we don't have struct
> device pointer here, that's why I used of_clk_get_by_name().

When I test it, I can get the clock pointer by devm_clk_get() as following:

Chanwoo Choi

Comments

Sylwester Nawrocki/Kernel \(PLT\) /SRPOL/Staff Engineer/Samsung Electronics July 21, 2016, 3:12 p.m. UTC | #1

On 07/21/2016 12:28 PM, Chanwoo Choi wrote:
> When I test it, I can get the clock pointer by devm_clk_get() as following:
> 
> diff --git a/sound/soc/samsung/tm2_wm5110.c b/sound/soc/samsung/tm2_wm5110.c
> index 9728b3c5927f..5de4fc554aec 100644
> --- a/sound/soc/samsung/tm2_wm5110.c
> +++ b/sound/soc/samsung/tm2_wm5110.c
> @@ -500,7 +500,7 @@ static int tm2_probe(struct platform_device *pdev)
>                 card->dai_link[i].platform_of_node = cpu_dai_node;
>         }
> 
> -       priv->codec_mclk1 = of_clk_get_by_name(codec_dai_node, "mclk1");
> +       priv->codec_mclk1 = devm_clk_get(dev, "mclk1");

I guess you have and old dts where clock/clock-names are in the sound
node.  Instead the clock/clock-names properties should be specified
in the CODEC's node, the related binding's documentation can be found
in Documentation/devicetree/bindings/mfd/arizona.txt. Also please see
patch 3/4, there is no clock properties in the sound card's DT binding.

--
Thanks,
Sylwester

Patch

diff --git a/sound/soc/samsung/tm2_wm5110.c b/sound/soc/samsung/tm2_wm5110.c
index 9728b3c5927f..5de4fc554aec 100644
--- a/sound/soc/samsung/tm2_wm5110.c
+++ b/sound/soc/samsung/tm2_wm5110.c
@@ -500,7 +500,7 @@  static int tm2_probe(struct platform_device *pdev)
                card->dai_link[i].platform_of_node = cpu_dai_node;
        }

-       priv->codec_mclk1 = of_clk_get_by_name(codec_dai_node, "mclk1");
+       priv->codec_mclk1 = devm_clk_get(dev, "mclk1");
        if (IS_ERR(priv->codec_mclk1)) {
                dev_err(dev, "Failed to get mclk1 clock\n");
                ret = PTR_ERR(priv->codec_mclk1);
@@ -508,7 +508,7 @@  static int tm2_probe(struct platform_device *pdev)
        }

        /* mclk2 is optional */
-       priv->codec_mclk2 = of_clk_get_by_name(codec_dai_node, "mclk2");
+       priv->codec_mclk2 = devm_clk_get(dev, "mclk2");
        if (IS_ERR(priv->codec_mclk2))
                dev_info(dev, "Not using mclk2 clock\n");

@@ -516,21 +516,17 @@  static int tm2_probe(struct platform_device *pdev)
                                tm2_ext_dai, ARRAY_SIZE(tm2_ext_dai));
        if (ret < 0) {
                dev_err(dev, "Failed to register component: %d\n", ret);
-               goto err_put_mclk;
+               goto err_put_codec_dai;
        }

        ret = devm_snd_soc_register_card(dev, card);
        if (ret < 0) {
                dev_err(dev, "Failed to register card: %d\n", ret);
-               goto err_put_mclk;
+               goto err_put_codec_dai;
        }

        return 0;

-err_put_mclk:
-       clk_put(priv->codec_mclk1);
-       if (!IS_ERR(priv->codec_mclk2))
-               clk_put(priv->codec_mclk2);
 err_put_codec_dai:
        of_node_put(codec_dai_node);
 err_put_cpu_dai:
@@ -543,11 +539,6 @@  err_put_amp:
 static int tm2_remove(struct platform_device *pdev)
 {
        struct snd_soc_card *card = &tm2_card;
-       struct tm2_machine_priv *priv = snd_soc_card_get_drvdata(card);
-
-       clk_put(priv->codec_mclk1);
-       if (!IS_ERR(priv->codec_mclk2))
-               clk_put(priv->codec_mclk2);


Regards,