diff mbox

[09/39,v2] ASoC: simple-card-utils: add asoc_simple_card_parse_clk()

Message ID 8737oysj1d.wl%kuninori.morimoto.gx@renesas.com (mailing list archive)
State Changes Requested
Delegated to: Geert Uytterhoeven
Headers show

Commit Message

Kuninori Morimoto May 31, 2016, 9:02 a.m. UTC
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Current simple-card can get clock via DT clocks or
"system-clock-frequency" property.
This patch makes it simple style standard

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 include/sound/simple_card_utils.h     |  8 ++++++++
 sound/soc/generic/simple-card-utils.c | 30 ++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

Comments

Mark Brown June 29, 2016, 6:18 p.m. UTC | #1
On Tue, May 31, 2016 at 09:02:22AM +0000, Kuninori Morimoto wrote:

> +	struct clk *clk;
> +	u32 val;
> +
> +	/*
> +	 * Parse dai->sysclk come from "clocks = <&xxx>"
> +	 * (if system has common clock)
> +	 *  or "system-clock-frequency = <xxx>"
> +	 *  or device's module clock.
> +	 */
> +	clk = of_clk_get(port_np, 0);
> +	if (!IS_ERR(clk)) {
> +		simple_dai->sysclk = clk_get_rate(clk);
> +		simple_dai->clk = clk;
> +	} else if (!of_property_read_u32(port_np, "system-clock-frequency", &val)) {
> +		simple_dai->sysclk = val;
> +	} else {
> +		clk = of_clk_get(endpoint_np, 0);
> +		if (!IS_ERR(clk))
> +			simple_dai->sysclk = clk_get_rate(clk);
> +	}

This looks like we're leaking the clocks - devm_ might help here
perhaps?
Kuninori Morimoto June 30, 2016, 12:25 a.m. UTC | #2
Hi Mark

> > +	struct clk *clk;
> > +	u32 val;
> > +
> > +	/*
> > +	 * Parse dai->sysclk come from "clocks = <&xxx>"
> > +	 * (if system has common clock)
> > +	 *  or "system-clock-frequency = <xxx>"
> > +	 *  or device's module clock.
> > +	 */
> > +	clk = of_clk_get(port_np, 0);
> > +	if (!IS_ERR(clk)) {
> > +		simple_dai->sysclk = clk_get_rate(clk);
> > +		simple_dai->clk = clk;
> > +	} else if (!of_property_read_u32(port_np, "system-clock-frequency", &val)) {
> > +		simple_dai->sysclk = val;
> > +	} else {
> > +		clk = of_clk_get(endpoint_np, 0);
> > +		if (!IS_ERR(clk))
> > +			simple_dai->sysclk = clk_get_rate(clk);
> > +	}
> 
> This looks like we're leaking the clocks - devm_ might help here
> perhaps?

Good catch.
This came from original simple-card, but yes, we should use devm_*
I will fix it on v3
Kuninori Morimoto June 30, 2016, 12:39 a.m. UTC | #3
Hi Mark, again

> > > +	struct clk *clk;
> > > +	u32 val;
> > > +
> > > +	/*
> > > +	 * Parse dai->sysclk come from "clocks = <&xxx>"
> > > +	 * (if system has common clock)
> > > +	 *  or "system-clock-frequency = <xxx>"
> > > +	 *  or device's module clock.
> > > +	 */
> > > +	clk = of_clk_get(port_np, 0);
> > > +	if (!IS_ERR(clk)) {
> > > +		simple_dai->sysclk = clk_get_rate(clk);
> > > +		simple_dai->clk = clk;
> > > +	} else if (!of_property_read_u32(port_np, "system-clock-frequency", &val)) {
> > > +		simple_dai->sysclk = val;
> > > +	} else {
> > > +		clk = of_clk_get(endpoint_np, 0);
> > > +		if (!IS_ERR(clk))
> > > +			simple_dai->sysclk = clk_get_rate(clk);
> > > +	}
> > 
> > This looks like we're leaking the clocks - devm_ might help here
> > perhaps?
> 
> Good catch.
> This came from original simple-card, but yes, we should use devm_*
> I will fix it on v3

Oops, of_clk_get() doesn't have devm_of_clk_get() ?
(and no of_clk_put() ... )
I will keep above as-is in v3. We can fix it incrementally (?)
Mark Brown July 1, 2016, 9:55 a.m. UTC | #4
On Thu, Jun 30, 2016 at 12:39:12AM +0000, Kuninori Morimoto wrote:

> Oops, of_clk_get() doesn't have devm_of_clk_get() ?

Perhaps add it? 

> (and no of_clk_put() ... )
> I will keep above as-is in v3. We can fix it incrementally (?)

You can just use regular clk_put() with of_clk_get().
Kuninori Morimoto July 3, 2016, 11:58 p.m. UTC | #5
Hi Mark

> > Oops, of_clk_get() doesn't have devm_of_clk_get() ?
> 
> Perhaps add it? 
> 
> > (and no of_clk_put() ... )
> > I will keep above as-is in v3. We can fix it incrementally (?)
> 
> You can just use regular clk_put() with of_clk_get().

Ohh.. OK, but can I do it in additional patch ?
Because main purpose of this patch is cleanup by using util.c,
not adding new feature.
Maybe it can be
 0001 add devm_of_clk_get()
 0002 use devm_of_clk_get() on simple-card-utils.c
diff mbox

Patch

diff --git a/include/sound/simple_card_utils.h b/include/sound/simple_card_utils.h
index 89172aa..b8a8649 100644
--- a/include/sound/simple_card_utils.h
+++ b/include/sound/simple_card_utils.h
@@ -38,4 +38,12 @@  int asoc_simple_card_parse_card_prefix(struct snd_soc_card *card,
 				       struct snd_soc_codec_conf *codec_conf,
 				       char *prefix);
 
+#define asoc_simple_card_parse_clk_cpu(port_np, dai_link, simple_dai)\
+	asoc_simple_card_parse_clk(port_np, dai_link->cpu_of_node, simple_dai)
+#define asoc_simple_card_parse_clk_codec(port_np, dai_link, simple_dai)  \
+	asoc_simple_card_parse_clk(port_np, dai_link->codec_of_node, simple_dai)
+int asoc_simple_card_parse_clk(struct device_node *port_np,
+			       struct device_node *endpoint_np,
+			       struct asoc_simple_dai *simple_dai);
+
 #endif /* __SIMPLE_CARD_CORE_H */
diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c
index 439fc01..dbf4b00 100644
--- a/sound/soc/generic/simple-card-utils.c
+++ b/sound/soc/generic/simple-card-utils.c
@@ -7,6 +7,7 @@ 
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  */
+#include <linux/clk.h>
 #include <linux/of.h>
 #include <sound/simple_card_utils.h>
 
@@ -142,3 +143,32 @@  int asoc_simple_card_parse_card_prefix(struct snd_soc_card *card,
 	return 0;
 }
 EXPORT_SYMBOL_GPL(asoc_simple_card_parse_card_prefix);
+
+int asoc_simple_card_parse_clk(struct device_node *port_np,
+			       struct device_node *endpoint_np,
+			       struct asoc_simple_dai *simple_dai)
+{
+	struct clk *clk;
+	u32 val;
+
+	/*
+	 * Parse dai->sysclk come from "clocks = <&xxx>"
+	 * (if system has common clock)
+	 *  or "system-clock-frequency = <xxx>"
+	 *  or device's module clock.
+	 */
+	clk = of_clk_get(port_np, 0);
+	if (!IS_ERR(clk)) {
+		simple_dai->sysclk = clk_get_rate(clk);
+		simple_dai->clk = clk;
+	} else if (!of_property_read_u32(port_np, "system-clock-frequency", &val)) {
+		simple_dai->sysclk = val;
+	} else {
+		clk = of_clk_get(endpoint_np, 0);
+		if (!IS_ERR(clk))
+			simple_dai->sysclk = clk_get_rate(clk);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(asoc_simple_card_parse_clk);