Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1443950 > unrolled thread

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

Started byChanwoo Choi <cw00.choi@samsung.com>
First post2016-07-15 07:20 +0200
Last post2016-07-21 17:20 +0200
Articles 4 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 4/4] ASoC: samsung: Add machine driver for Exynos5433  based TM2 board Chanwoo Choi <cw00.choi@samsung.com> - 2016-07-15 07:20 +0200
    Re: [PATCH v4 4/4] ASoC: samsung: Add machine driver for Exynos5433  based TM2 board Sylwester Nawrocki <s.nawrocki@samsung.com> - 2016-07-18 12:50 +0200
      Re: [PATCH v4 4/4] ASoC: samsung: Add machine driver for Exynos5433  based TM2 board Chanwoo Choi <cw00.choi@samsung.com> - 2016-07-21 12:30 +0200
        Re: [alsa-devel] [PATCH v4 4/4] ASoC: samsung: Add machine driver for  Exynos5433 based TM2 board Sylwester Nawrocki <s.nawrocki@samsung.com> - 2016-07-21 17:20 +0200

#1443950 — Re: [PATCH v4 4/4] ASoC: samsung: Add machine driver for Exynos5433 based TM2 board

FromChanwoo Choi <cw00.choi@samsung.com>
Date2016-07-15 07:20 +0200
SubjectRe: [PATCH v4 4/4] ASoC: samsung: Add machine driver for Exynos5433 based TM2 board
Message-ID<rV3Hz-2Ms-9@gated-at.bofh.it>
Hi Sylwester,

[snip]
> +static int tm2_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct snd_soc_card *card = &tm2_card;
> +	struct tm2_machine_priv *priv;
> +	struct device_node *cpu_dai_node, *codec_dai_node;
> +	int ret, i;
> +
> +	if (!dev->of_node) {
> +		dev_err(dev, "DT node is missing\n");
> +		return -ENODEV;
> +	}
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	snd_soc_card_set_drvdata(card, priv);
> +	card->dev = dev;
> +
> +	priv->gpio_mic_bias = devm_gpiod_get(dev, "mic-bias",
> +						GPIOF_OUT_INIT_LOW);
> +	if (IS_ERR(priv->gpio_mic_bias)) {
> +		dev_err(dev, "Failed to get mic bias gpio\n");
> +		return PTR_ERR(priv->gpio_mic_bias);
> +	}
> +
> +	ret = snd_soc_of_parse_card_name(card, "model");
> +	if (ret < 0) {
> +		dev_err(dev, "Card name is not specified\n");
> +		return ret;
> +	}
> +
> +	ret = snd_soc_of_parse_audio_routing(card, "samsung,audio-routing");
> +	if (ret < 0) {
> +		dev_err(dev, "Audio routing is not specified or invalid\n");
> +		return ret;
> +	}
> +
> +	card->aux_dev[0].codec_of_node = of_parse_phandle(dev->of_node,
> +							"audio-amplifier", 0);
> +	if (!card->aux_dev[0].codec_of_node) {
> +		dev_err(dev, "audio-amplifier property invalid or missing\n");
> +		return -EINVAL;
> +	}
> +
> +	cpu_dai_node = of_parse_phandle(dev->of_node, "i2s-controller", 0);
> +	if (!cpu_dai_node) {
> +		dev_err(dev, "i2s-controllers property invalid or missing\n");
> +		ret = -EINVAL;
> +		goto err_put_amp;
> +	}
> +
> +	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;
> +	}
> +
> +	for (i = 0; i < card->num_links; i++) {
> +		card->dai_link[i].cpu_dai_name = NULL;
> +		card->dai_link[i].cpu_name = NULL;
> +		card->dai_link[i].platform_name = NULL;
> +		card->dai_link[i].codec_of_node = codec_dai_node;
> +		card->dai_link[i].cpu_of_node = cpu_dai_node;
> +		card->dai_link[i].platform_of_node = cpu_dai_node;
> +	}
> +
> +	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");

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

	priv->codec_mclk2 = devm_clk_get(dev, "mclk2");

> +
> +	ret = devm_snd_soc_register_component(dev, &tm2_component,
> +				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;
> +	}
> +
> +	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;
> +	}
> +
> +	return 0;
> +
> +err_put_mclk:
> +	clk_put(priv->codec_mclk1);
> +	if (!IS_ERR(priv->codec_mclk2))
> +		clk_put(priv->codec_mclk2);

If using the devm_clk_get(), clk_put() is not needed

> +err_put_codec_dai:
> +	of_node_put(codec_dai_node);
> +err_put_cpu_dai:
> +	of_node_put(cpu_dai_node);
> +err_put_amp:
> +	of_node_put(card->aux_dev[0].codec_of_node);
> +	return ret;
> +}
> +
> +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);

ditto.

> +
> +	of_node_put(card->dai_link[0].codec_of_node);
> +	of_node_put(card->dai_link[0].cpu_of_node);
> +	of_node_put(card->aux_dev[0].codec_of_node);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id tm2_of_match[] = {
> +	{ .compatible = "samsung,tm2-audio" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, tm2_of_match);
> +
> +static struct platform_driver tm2_driver = {
> +	.driver = {
> +		.name		= "tm2-audio",
> +		.pm		= &snd_soc_pm_ops,
> +		.of_match_table	= tm2_of_match,
> +	},
> +	.probe	= tm2_probe,
> +	.remove	= tm2_remove,
> +};
> +
> +module_platform_driver(tm2_driver);
> +
> +MODULE_AUTHOR("Inha Song <ideal.song@samsung.com>");
> +MODULE_DESCRIPTION("ALSA SoC Exynos TM2 Audio Support");
> +MODULE_LICENSE("GPL v2");
> 

Thanks,
Chanwoo Choi

[toc] | [next] | [standalone]

#1445374

FromSylwester Nawrocki <s.nawrocki@samsung.com>
Date2016-07-18 12:50 +0200
Message-ID<rWehz-53k-1@gated-at.bofh.it>
In reply to#1443950
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().

--
Thanks,
Sylwester

[toc] | [prev] | [next] | [standalone]

#1447773

FromChanwoo Choi <cw00.choi@samsung.com>
Date2016-07-21 12:30 +0200
Message-ID<rXjoS-6b9-21@gated-at.bofh.it>
In reply to#1445374
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:

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,
Chanwoo Choi

[toc] | [prev] | [next] | [standalone]

#1447959 — Re: [alsa-devel] [PATCH v4 4/4] ASoC: samsung: Add machine driver for Exynos5433 based TM2 board

FromSylwester Nawrocki <s.nawrocki@samsung.com>
Date2016-07-21 17:20 +0200
SubjectRe: [alsa-devel] [PATCH v4 4/4] ASoC: samsung: Add machine driver for Exynos5433 based TM2 board
Message-ID<rXnVw-Jo-5@gated-at.bofh.it>
In reply to#1447773
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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web