| Message ID | 8737oysj1d.wl%kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
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?
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
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 (?)
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().
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 --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);