Hi Stephen
This is v5 of "clkdev: add devm_of_clk_get()", but new series.
I hope my understanding was correct with your idea.
Kuninori Morimoto (3):
1) clkdev: add devm_get_clk_from_child()
2) ASoC: simple-card: use devm_get_clk_from_child()
3) ASoC: simple-card-utils: enable clocks/clock-names/clock-ranges
.../devicetree/bindings/sound/simple-card.txt | 32 +++++++++++++++
drivers/clk/clk-devres.c | 21 ++++++++++
include/linux/clk.h | 29 ++++++++++++--
include/sound/simple_card_utils.h | 11 +++---
sound/soc/generic/simple-card-utils.c | 45 ++++++++++++++++++++--
sound/soc/generic/simple-card.c | 4 +-
sound/soc/generic/simple-scu-card.c | 4 +-
7 files changed, 129 insertions(+), 17 deletions(-)
--
1.9.1
From: Kuninori Morimoto <[email protected]>
Current simple-card-utils is getting clk by of_clk_get(), but didn't call
clk_free(). Now we can use devm_get_clk_from_child() for this purpose.
Let's use it.
Signed-off-by: Kuninori Morimoto <[email protected]>
---
include/sound/simple_card_utils.h | 11 ++++++-----
sound/soc/generic/simple-card-utils.c | 8 ++++----
sound/soc/generic/simple-card.c | 4 ++--
sound/soc/generic/simple-scu-card.c | 4 ++--
4 files changed, 14 insertions(+), 13 deletions(-)
diff --git a/include/sound/simple_card_utils.h b/include/sound/simple_card_utils.h
index 64e90ca..af58d23 100644
--- a/include/sound/simple_card_utils.h
+++ b/include/sound/simple_card_utils.h
@@ -34,11 +34,12 @@ int asoc_simple_card_set_dailink_name(struct device *dev,
int asoc_simple_card_parse_card_name(struct snd_soc_card *card,
char *prefix);
-#define asoc_simple_card_parse_clk_cpu(node, dai_link, simple_dai) \
- asoc_simple_card_parse_clk(node, dai_link->cpu_of_node, simple_dai)
-#define asoc_simple_card_parse_clk_codec(node, dai_link, simple_dai) \
- asoc_simple_card_parse_clk(node, dai_link->codec_of_node, simple_dai)
-int asoc_simple_card_parse_clk(struct device_node *node,
+#define asoc_simple_card_parse_clk_cpu(dev, node, dai_link, simple_dai) \
+ asoc_simple_card_parse_clk(dev, node, dai_link->cpu_of_node, simple_dai)
+#define asoc_simple_card_parse_clk_codec(dev, node, dai_link, simple_dai) \
+ asoc_simple_card_parse_clk(dev, node, dai_link->codec_of_node, simple_dai)
+int asoc_simple_card_parse_clk(struct device *dev,
+ struct device_node *node,
struct device_node *dai_of_node,
struct asoc_simple_dai *simple_dai);
diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c
index cf02625..4924575 100644
--- a/sound/soc/generic/simple-card-utils.c
+++ b/sound/soc/generic/simple-card-utils.c
@@ -98,7 +98,8 @@ int asoc_simple_card_parse_card_name(struct snd_soc_card *card,
}
EXPORT_SYMBOL_GPL(asoc_simple_card_parse_card_name);
-int asoc_simple_card_parse_clk(struct device_node *node,
+int asoc_simple_card_parse_clk(struct device *dev,
+ struct device_node *node,
struct device_node *dai_of_node,
struct asoc_simple_dai *simple_dai)
{
@@ -111,14 +112,13 @@ int asoc_simple_card_parse_clk(struct device_node *node,
* or "system-clock-frequency = <xxx>"
* or device's module clock.
*/
- clk = of_clk_get(node, 0);
+ clk = devm_get_clk_from_child(dev, node, NULL);
if (!IS_ERR(clk)) {
simple_dai->sysclk = clk_get_rate(clk);
- simple_dai->clk = clk;
} else if (!of_property_read_u32(node, "system-clock-frequency", &val)) {
simple_dai->sysclk = val;
} else {
- clk = of_clk_get(dai_of_node, 0);
+ clk = devm_get_clk_from_child(dev, dai_of_node, NULL);
if (!IS_ERR(clk))
simple_dai->sysclk = clk_get_rate(clk);
}
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index a385ff6..85b4f18 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -278,11 +278,11 @@ static int asoc_simple_card_dai_link_of(struct device_node *node,
if (ret < 0)
goto dai_link_of_err;
- ret = asoc_simple_card_parse_clk_cpu(cpu, dai_link, cpu_dai);
+ ret = asoc_simple_card_parse_clk_cpu(dev, cpu, dai_link, cpu_dai);
if (ret < 0)
goto dai_link_of_err;
- ret = asoc_simple_card_parse_clk_codec(codec, dai_link, codec_dai);
+ ret = asoc_simple_card_parse_clk_codec(dev, codec, dai_link, codec_dai);
if (ret < 0)
goto dai_link_of_err;
diff --git a/sound/soc/generic/simple-scu-card.c b/sound/soc/generic/simple-scu-card.c
index bb86ee0..308ff4c 100644
--- a/sound/soc/generic/simple-scu-card.c
+++ b/sound/soc/generic/simple-scu-card.c
@@ -128,7 +128,7 @@ static int asoc_simple_card_dai_link_of(struct device_node *np,
if (ret)
return ret;
- ret = asoc_simple_card_parse_clk_cpu(np, dai_link, dai_props);
+ ret = asoc_simple_card_parse_clk_cpu(dev, np, dai_link, dai_props);
if (ret < 0)
return ret;
@@ -153,7 +153,7 @@ static int asoc_simple_card_dai_link_of(struct device_node *np,
if (ret < 0)
return ret;
- ret = asoc_simple_card_parse_clk_codec(np, dai_link, dai_props);
+ ret = asoc_simple_card_parse_clk_codec(dev, np, dai_link, dai_props);
if (ret < 0)
return ret;
--
1.9.1
From: Kuninori Morimoto <[email protected]>
Current simple-card is supporting this style for clocks
sound {
...
simple-audio-card,cpu {
sound-dai = <&xxx>;
clocks = <&cpu_clock>;
};
simple-audio-card,codec {
sound-dai = <&xxx>;
clocks = <&codec_clock>;
};
};
Now, it can support this style too, because we can use
devm_get_clk_from_child() now.
sound {
...
clocks = <&cpu_clock>, <&codec_clock>;
clock-names = "cpu", "codec";
clock-ranges;
...
simple-audio-card,cpu {
sound-dai = <&xxx>;
};
simple-audio-card,codec {
sound-dai = <&xxx>;
};
};
Signed-off-by: Kuninori Morimoto <[email protected]>
---
.../devicetree/bindings/sound/simple-card.txt | 32 ++++++++++++++++++
sound/soc/generic/simple-card-utils.c | 39 +++++++++++++++++++++-
2 files changed, 70 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
index c7a9393..43a710b 100644
--- a/Documentation/devicetree/bindings/sound/simple-card.txt
+++ b/Documentation/devicetree/bindings/sound/simple-card.txt
@@ -86,6 +86,7 @@ Optional CPU/CODEC subnodes properties:
in dai startup() and disabled with
clk_disable_unprepare() in dai
shutdown().
+ see Clock Example.
Example 1 - single DAI link:
@@ -199,3 +200,34 @@ sound {
clocks = ...
};
};
+
+
+Clock Example 1 - clock settings on each subnode
+
+sound {
+ ...
+ simple-audio-card,cpu {
+ sound-dai = <&xxx>;
+ clocks = <&cpu_clock>;
+ };
+ simple-audio-card,codec {
+ sound-dai = <&xxx>;
+ clocks = <&codec_clock>;
+ };
+};
+
+Clock Example 2 - clock settings by clocks
+
+sound {
+ ...
+ clocks = <&cpu_clock>, <&codec_clock>;
+ clock-names = "cpu", "codec";
+ clock-ranges;
+ ...
+ simple-audio-card,cpu {
+ sound-dai = <&xxx>;
+ };
+ simple-audio-card,codec {
+ sound-dai = <&xxx>;
+ };
+};
diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c
index 4924575..c3031a5 100644
--- a/sound/soc/generic/simple-card-utils.c
+++ b/sound/soc/generic/simple-card-utils.c
@@ -104,15 +104,52 @@ int asoc_simple_card_parse_clk(struct device *dev,
struct asoc_simple_dai *simple_dai)
{
struct clk *clk;
+ const char *con_id = NULL;
+ const char *port_name[] = {
+ "cpu", "codec"
+ };
u32 val;
/*
+ * We can use this style if "con_id" is not NULL
+ *
+ * sound {
+ * ...
+ * clocks = <&xxx>, <&xxx>;
+ * clock-names = "cpu", "codec";
+ * clock-ranges;
+ *
+ * simple-audio-card,cpu {
+ * sound-dai = <&xxx>;
+ * };
+ * simple-audio-card,codec {
+ * sound-dai = <&xxx>;
+ * };
+ * };
+ */
+ if (of_find_property(dev->of_node, "clock-names", NULL)) {
+ int i;
+ int port_len, node_len;
+
+ for (i = 0; i < ARRAY_SIZE(port_name); i++) {
+ node_len = strlen(node->name);
+ port_len = strlen(port_name[i]);
+
+ if (0 == strncmp(node->name + node_len - port_len,
+ port_name[i], port_len)) {
+ con_id = port_name[i];
+ break;
+ }
+ }
+ }
+
+ /*
* Parse dai->sysclk come from "clocks = <&xxx>"
* (if system has common clock)
* or "system-clock-frequency = <xxx>"
* or device's module clock.
*/
- clk = devm_get_clk_from_child(dev, node, NULL);
+ clk = devm_get_clk_from_child(dev, node, con_id);
if (!IS_ERR(clk)) {
simple_dai->sysclk = clk_get_rate(clk);
} else if (!of_property_read_u32(node, "system-clock-frequency", &val)) {
--
1.9.1
From: Kuninori Morimoto <[email protected]>
Some driver is using this type of DT bindings for clock (more detail,
see ${LINUX}/Documentation/devicetree/bindings/sound/simple-card.txt).
sound_soc {
...
cpu {
clocks = <&xxx>;
...
};
codec {
clocks = <&xxx>;
...
};
};
Current driver in this case uses of_clk_get() for each node, but there
is no devm_of_clk_get() today.
OTOH, the problem of having devm_of_clk_get() is that it encourages the
use of of_clk_get() when clk_get() is more desirable.
Thus, this patch adds new devm_get_clk_from_chile() which explicitly
reads as get a clock from a child node of this device.
By this function, we can also use this type of DT bindings
sound_soc {
clocks = <&xxx>, <&xxx>;
clock-names = "cpu", "codec";
...
cpu {
...
};
codec {
...
};
};
Signed-off-by: Kuninori Morimoto <[email protected]>
---
drivers/clk/clk-devres.c | 21 +++++++++++++++++++++
include/linux/clk.h | 29 +++++++++++++++++++++++++----
2 files changed, 46 insertions(+), 4 deletions(-)
diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index 8f57154..3a218c3 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -53,3 +53,24 @@ void devm_clk_put(struct device *dev, struct clk *clk)
WARN_ON(ret);
}
EXPORT_SYMBOL(devm_clk_put);
+
+struct clk *devm_get_clk_from_child(struct device *dev,
+ struct device_node *np, const char *con_id)
+{
+ struct clk **ptr, *clk;
+
+ ptr = devres_alloc(devm_clk_release, sizeof(*ptr), GFP_KERNEL);
+ if (!ptr)
+ return ERR_PTR(-ENOMEM);
+
+ clk = of_clk_get_by_name(np, con_id);
+ if (!IS_ERR(clk)) {
+ *ptr = clk;
+ devres_add(dev, ptr);
+ } else {
+ devres_free(ptr);
+ }
+
+ return clk;
+}
+EXPORT_SYMBOL(devm_get_clk_from_child);
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 123c027..e9d36b3 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -17,8 +17,9 @@
#include <linux/notifier.h>
struct device;
-
struct clk;
+struct device_node;
+struct of_phandle_args;
/**
* DOC: clk notifier callback types
@@ -249,6 +250,23 @@ static inline void clk_unprepare(struct clk *clk)
struct clk *devm_clk_get(struct device *dev, const char *id);
/**
+ * devm_get_clk_from_child - lookup and obtain a managed reference to a
+ * clock producer from child node.
+ * @dev: device for clock "consumer"
+ * @np: pointer to clock consumer node
+ * @con_id: clock consumer ID
+ *
+ * This function parses the clocks, and uses them to look up the
+ * struct clk from the registered list of clock providers by using
+ * @np and @con_id
+ *
+ * The clock will automatically be freed when the device is unbound
+ * from the bus.
+ */
+struct clk *devm_get_clk_from_child(struct device *dev,
+ struct device_node *np, const char *con_id);
+
+/**
* clk_enable - inform the system when the clock source should be running.
* @clk: clock source
*
@@ -432,6 +450,12 @@ static inline struct clk *devm_clk_get(struct device *dev, const char *id)
return NULL;
}
+static inline struct clk *devm_get_clk_from_child(struct device *dev,
+ struct device_node *np, const char *con_id)
+{
+ return NULL;
+}
+
static inline void clk_put(struct clk *clk) {}
static inline void devm_clk_put(struct device *dev, struct clk *clk) {}
@@ -501,9 +525,6 @@ static inline void clk_disable_unprepare(struct clk *clk)
clk_unprepare(clk);
}
-struct device_node;
-struct of_phandle_args;
-
#if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
struct clk *of_clk_get(struct device_node *np, int index);
struct clk *of_clk_get_by_name(struct device_node *np, const char *name);
--
1.9.1
On 12/05, Kuninori Morimoto wrote:
>
> Hi Stephen
>
> This is v5 of "clkdev: add devm_of_clk_get()", but new series.
> I hope my understanding was correct with your idea.
Yes this looks good. Given that we're so close to the merge
window, perhaps I should just merge the first patch into clk-next
and then it will be ready for anyone who wants to use it? The
sound patches can be left up to others to handle.
>
> Kuninori Morimoto (3):
> 1) clkdev: add devm_get_clk_from_child()
> 2) ASoC: simple-card: use devm_get_clk_from_child()
> 3) ASoC: simple-card-utils: enable clocks/clock-names/clock-ranges
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On 12/05, Kuninori Morimoto wrote:
> From: Kuninori Morimoto <[email protected]>
>
> Current simple-card is supporting this style for clocks
>
> sound {
> ...
> simple-audio-card,cpu {
> sound-dai = <&xxx>;
> clocks = <&cpu_clock>;
> };
> simple-audio-card,codec {
> sound-dai = <&xxx>;
> clocks = <&codec_clock>;
> };
> };
>
> Now, it can support this style too, because we can use
> devm_get_clk_from_child() now.
>
> sound {
> ...
> clocks = <&cpu_clock>, <&codec_clock>;
> clock-names = "cpu", "codec";
> clock-ranges;
> ...
> simple-audio-card,cpu {
> sound-dai = <&xxx>;
> };
> simple-audio-card,codec {
> sound-dai = <&xxx>;
> };
> };
>
> Signed-off-by: Kuninori Morimoto <[email protected]>
I don't see any reason why we need this patch though. The binding
works as is, so supporting different styles doesn't seem like a
good idea to me. Let's just keep what we have? Even if a sub-node
like cpu or codec gets more than one element in the clocks list
property, we can make that work by passing a clock-name then
based on some sort of other knowledge.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On 12/05, Kuninori Morimoto wrote:
> diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c
> index cf02625..4924575 100644
> --- a/sound/soc/generic/simple-card-utils.c
> +++ b/sound/soc/generic/simple-card-utils.c
> @@ -98,7 +98,8 @@ int asoc_simple_card_parse_card_name(struct snd_soc_card *card,
> }
> EXPORT_SYMBOL_GPL(asoc_simple_card_parse_card_name);
>
> -int asoc_simple_card_parse_clk(struct device_node *node,
> +int asoc_simple_card_parse_clk(struct device *dev,
> + struct device_node *node,
> struct device_node *dai_of_node,
> struct asoc_simple_dai *simple_dai)
> {
> @@ -111,14 +112,13 @@ int asoc_simple_card_parse_clk(struct device_node *node,
> * or "system-clock-frequency = <xxx>"
> * or device's module clock.
> */
> - clk = of_clk_get(node, 0);
> + clk = devm_get_clk_from_child(dev, node, NULL);
> if (!IS_ERR(clk)) {
> simple_dai->sysclk = clk_get_rate(clk);
> - simple_dai->clk = clk;
> } else if (!of_property_read_u32(node, "system-clock-frequency", &val)) {
> simple_dai->sysclk = val;
> } else {
> - clk = of_clk_get(dai_of_node, 0);
> + clk = devm_get_clk_from_child(dev, dai_of_node, NULL);
I was confused for a minute about how the second of_clk_get()
call with the dai_link node could work. Is that documented
anywhere or used by anyone? It seems like it's at least another
child node of the sound node (which is dev here) so it seems ok.
> if (!IS_ERR(clk))
> simple_dai->sysclk = clk_get_rate(clk);
> }
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
Hi Stephen
> > @@ -111,14 +112,13 @@ int asoc_simple_card_parse_clk(struct device_node *node,
> > * or "system-clock-frequency = <xxx>"
> > * or device's module clock.
> > */
> > - clk = of_clk_get(node, 0);
> > + clk = devm_get_clk_from_child(dev, node, NULL);
> > if (!IS_ERR(clk)) {
> > simple_dai->sysclk = clk_get_rate(clk);
> > - simple_dai->clk = clk;
> > } else if (!of_property_read_u32(node, "system-clock-frequency", &val)) {
> > simple_dai->sysclk = val;
> > } else {
> > - clk = of_clk_get(dai_of_node, 0);
> > + clk = devm_get_clk_from_child(dev, dai_of_node, NULL);
>
>
> I was confused for a minute about how the second of_clk_get()
> call with the dai_link node could work. Is that documented
> anywhere or used by anyone? It seems like it's at least another
> child node of the sound node (which is dev here) so it seems ok.
Documentation/devicetree/bindings/sound/simple-card.txt
explains 1st of_clk_get will be used as "if needed",
2nd of_clk_get will be used as "not needed pattern".
1st pattern will use specific clock, 2nd pattern will use
"cpu" or "codec" clock.
2nd one was added by someone (I forgot), and many driver is
based on this feature.
Best regards
---
Kuninori Morimoto
Hi Stephen
> > From: Kuninori Morimoto <[email protected]>
> >
> > Current simple-card is supporting this style for clocks
> >
> > sound {
> > ...
> > simple-audio-card,cpu {
> > sound-dai = <&xxx>;
> > clocks = <&cpu_clock>;
> > };
> > simple-audio-card,codec {
> > sound-dai = <&xxx>;
> > clocks = <&codec_clock>;
> > };
> > };
> >
> > Now, it can support this style too, because we can use
> > devm_get_clk_from_child() now.
> >
> > sound {
> > ...
> > clocks = <&cpu_clock>, <&codec_clock>;
> > clock-names = "cpu", "codec";
> > clock-ranges;
> > ...
> > simple-audio-card,cpu {
> > sound-dai = <&xxx>;
> > };
> > simple-audio-card,codec {
> > sound-dai = <&xxx>;
> > };
> > };
> >
> > Signed-off-by: Kuninori Morimoto <[email protected]>
>
> I don't see any reason why we need this patch though. The binding
> works as is, so supporting different styles doesn't seem like a
> good idea to me. Let's just keep what we have? Even if a sub-node
> like cpu or codec gets more than one element in the clocks list
> property, we can make that work by passing a clock-name then
> based on some sort of other knowledge.
OK, thanks. Let's skip this patch.
But I believe this idea itself is not wrong (?)
Hi Stephen
> > From: Kuninori Morimoto <[email protected]>
> >
> > Current simple-card is supporting this style for clocks
> >
> > sound {
> > ...
> > simple-audio-card,cpu {
> > sound-dai = <&xxx>;
> > clocks = <&cpu_clock>;
> > };
> > simple-audio-card,codec {
> > sound-dai = <&xxx>;
> > clocks = <&codec_clock>;
> > };
> > };
> >
> > Now, it can support this style too, because we can use
> > devm_get_clk_from_child() now.
> >
> > sound {
> > ...
> > clocks = <&cpu_clock>, <&codec_clock>;
> > clock-names = "cpu", "codec";
> > clock-ranges;
> > ...
> > simple-audio-card,cpu {
> > sound-dai = <&xxx>;
> > };
> > simple-audio-card,codec {
> > sound-dai = <&xxx>;
> > };
> > };
> >
> > Signed-off-by: Kuninori Morimoto <[email protected]>
>
> I don't see any reason why we need this patch though. The binding
> works as is, so supporting different styles doesn't seem like a
> good idea to me. Let's just keep what we have? Even if a sub-node
> like cpu or codec gets more than one element in the clocks list
> property, we can make that work by passing a clock-name then
> based on some sort of other knowledge.
OK, thanks. Let's skip this patch.
But I believe this idea/method itself is not wrong (?)
Hi Stephen, Mark
> > This is v5 of "clkdev: add devm_of_clk_get()", but new series.
> > I hope my understanding was correct with your idea.
>
> Yes this looks good. Given that we're so close to the merge
> window, perhaps I should just merge the first patch into clk-next
> and then it will be ready for anyone who wants to use it? The
> sound patches can be left up to others to handle.
OK thanks.
Mark, I think I should re-post 2nd patch (3rd will be dropped) after
merge window ? There will be no branch dependency
On 12/09, Kuninori Morimoto wrote:
>
> Hi Stephen
>
> > > From: Kuninori Morimoto <[email protected]>
> > >
> > > Current simple-card is supporting this style for clocks
> > >
> > > sound {
> > > ...
> > > simple-audio-card,cpu {
> > > sound-dai = <&xxx>;
> > > clocks = <&cpu_clock>;
> > > };
> > > simple-audio-card,codec {
> > > sound-dai = <&xxx>;
> > > clocks = <&codec_clock>;
> > > };
> > > };
> > >
> > > Now, it can support this style too, because we can use
> > > devm_get_clk_from_child() now.
> > >
> > > sound {
> > > ...
> > > clocks = <&cpu_clock>, <&codec_clock>;
> > > clock-names = "cpu", "codec";
> > > clock-ranges;
> > > ...
> > > simple-audio-card,cpu {
> > > sound-dai = <&xxx>;
> > > };
> > > simple-audio-card,codec {
> > > sound-dai = <&xxx>;
> > > };
> > > };
> > >
> > > Signed-off-by: Kuninori Morimoto <[email protected]>
> >
> > I don't see any reason why we need this patch though. The binding
> > works as is, so supporting different styles doesn't seem like a
> > good idea to me. Let's just keep what we have? Even if a sub-node
> > like cpu or codec gets more than one element in the clocks list
> > property, we can make that work by passing a clock-name then
> > based on some sort of other knowledge.
>
> OK, thanks. Let's skip this patch.
> But I believe this idea/method itself is not wrong (?)
>
Right it's not wrong, just seems confusing to have two methods.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On 12/09, Kuninori Morimoto wrote:
>
> Hi Stephen
>
> > > @@ -111,14 +112,13 @@ int asoc_simple_card_parse_clk(struct device_node *node,
> > > * or "system-clock-frequency = <xxx>"
> > > * or device's module clock.
> > > */
> > > - clk = of_clk_get(node, 0);
> > > + clk = devm_get_clk_from_child(dev, node, NULL);
> > > if (!IS_ERR(clk)) {
> > > simple_dai->sysclk = clk_get_rate(clk);
> > > - simple_dai->clk = clk;
> > > } else if (!of_property_read_u32(node, "system-clock-frequency", &val)) {
> > > simple_dai->sysclk = val;
> > > } else {
> > > - clk = of_clk_get(dai_of_node, 0);
> > > + clk = devm_get_clk_from_child(dev, dai_of_node, NULL);
> >
> >
> > I was confused for a minute about how the second of_clk_get()
> > call with the dai_link node could work. Is that documented
> > anywhere or used by anyone? It seems like it's at least another
> > child node of the sound node (which is dev here) so it seems ok.
>
> Documentation/devicetree/bindings/sound/simple-card.txt
> explains 1st of_clk_get will be used as "if needed",
> 2nd of_clk_get will be used as "not needed pattern".
> 1st pattern will use specific clock, 2nd pattern will use
> "cpu" or "codec" clock.
> 2nd one was added by someone (I forgot), and many driver is
> based on this feature.
>
Can you point to some dts file in the kernel that falls into the
devm_get_clk_from_child(dev, dai_of_node, NULL) part?
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
Hi Stephen
> > Documentation/devicetree/bindings/sound/simple-card.txt
> > explains 1st of_clk_get will be used as "if needed",
> > 2nd of_clk_get will be used as "not needed pattern".
> > 1st pattern will use specific clock, 2nd pattern will use
> > "cpu" or "codec" clock.
> > 2nd one was added by someone (I forgot), and many driver is
> > based on this feature.
> >
>
> Can you point to some dts file in the kernel that falls into the
> devm_get_clk_from_child(dev, dai_of_node, NULL) part?
How about this ?
linux/arch/arm/boot/dts/r8a7790-lager.dts :: rsnd_ak4643
Hi Stephen
> > > I don't see any reason why we need this patch though. The binding
> > > works as is, so supporting different styles doesn't seem like a
> > > good idea to me. Let's just keep what we have? Even if a sub-node
> > > like cpu or codec gets more than one element in the clocks list
> > > property, we can make that work by passing a clock-name then
> > > based on some sort of other knowledge.
> >
> > OK, thanks. Let's skip this patch.
> > But I believe this idea/method itself is not wrong (?)
> >
> Right it's not wrong, just seems confusing to have two methods.
Thanks.
Very clear for me :)
On Mon, Dec 05, 2016 at 05:23:20AM +0000, Kuninori Morimoto wrote:
>
> From: Kuninori Morimoto <[email protected]>
>
> Some driver is using this type of DT bindings for clock (more detail,
> see ${LINUX}/Documentation/devicetree/bindings/sound/simple-card.txt).
>
> sound_soc {
> ...
> cpu {
> clocks = <&xxx>;
> ...
> };
> codec {
> clocks = <&xxx>;
> ...
> };
> };
>
> Current driver in this case uses of_clk_get() for each node, but there
> is no devm_of_clk_get() today.
> OTOH, the problem of having devm_of_clk_get() is that it encourages the
> use of of_clk_get() when clk_get() is more desirable.
>
> Thus, this patch adds new devm_get_clk_from_chile() which explicitly
> reads as get a clock from a child node of this device.
> By this function, we can also use this type of DT bindings
>
> sound_soc {
> clocks = <&xxx>, <&xxx>;
> clock-names = "cpu", "codec";
> ...
> cpu {
> ...
> };
> codec {
> ...
> };
> };
>
> Signed-off-by: Kuninori Morimoto <[email protected]>
This looks lots better, thanks.
Acked-by: Russell King <[email protected]>
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
On Fri, Dec 09, 2016 at 12:25:48AM +0000, Kuninori Morimoto wrote:
> Mark, I think I should re-post 2nd patch (3rd will be dropped) after
> merge window ? There will be no branch dependency
Should be fine without but obviously if it looks like it's gone AWOL
then reposting would be good.
Hi Mark
> > Mark, I think I should re-post 2nd patch (3rd will be dropped) after
> > merge window ? There will be no branch dependency
>
> Should be fine without but obviously if it looks like it's gone AWOL
> then reposting would be good.
OK, Thanks !
Hi Stephen
Now, I'm using devm_get_clk_from_child() (= linux/drivers/clk/clk-devres.c)
and I got Oops if I do bind/unbind driver several times.
[ 32.008847] Unable to handle kernel paging request at virtual address d503201faa1e03e0
[ 32.017124] pgd = ffff8006f9060000
[ 32.020883] [d503201faa1e03e0] *pgd=0000000000000000
[ 32.026243] Internal error: Oops: 96000004 [#1] PREEMPT SMP
[ 32.032198] CPU: 0 PID: 934 Comm: kworker/0:2 Not tainted 4.11.0-rc3+ #1259
[ 32.039573] Hardware name: Renesas Salvator-X board based on r8a7795 (DT)
[ 32.046814] Workqueue: events deferred_probe_work_func
[ 32.052405] task: ffff8006fad2d800 task.stack: ffff8006f90b8000
[ 32.058809] PC is at __of_clk_get_from_provider+0x174/0x1b0
[ 32.064878] LR is at __of_clk_get_from_provider+0x164/0x1b0
....
[ 32.746677] [<ffff0000083b581c>] __of_clk_get_from_provider+0x174/0x1b0
[ 32.754131] [<ffff0000083aded4>] __of_clk_get_by_name+0x104/0x140
[ 32.761058] [<ffff0000083adfd8>] of_clk_get_by_name+0x30/0x50
[ 32.767630] [<ffff0000083add1c>] devm_get_clk_from_child+0x54/0xb0
...
I tried to find the criminal point, but, I couldn't specify where it is.
Sometimes it is NULL pointer access crashed,
sometimes it is crashed on of_clk_src_onecell_get(),
sometimes there is no Oops.
I want to solve this issue, but I want to know about of_clk_put as 1st step.
In devm_clk_get() case, it is using clk_get() <-> clk_put() pair, this is OK.
In devm_get_clk_from_child() case, it is using of_clk_get_by_name() (= __of_clk_get())
<-> clk_put() pair.
If my understand was correct, __of_clk_get() uses __clk_create_clk(),
so, its pair should be __clk_free_clk() ?
I wonder I can find of_clk_get(), but couldn't find of_clk_put().
Is using of_clk_get() <-> clk_put() OK ?
Best regards
---
Kuninori Morimoto
Hi
Sorry for my noise.
I could solve my issue.
> Now, I'm using devm_get_clk_from_child() (= linux/drivers/clk/clk-devres.c)
> and I got Oops if I do bind/unbind driver several times.
>
> [ 32.008847] Unable to handle kernel paging request at virtual address d503201faa1e03e0
> [ 32.017124] pgd = ffff8006f9060000
> [ 32.020883] [d503201faa1e03e0] *pgd=0000000000000000
> [ 32.026243] Internal error: Oops: 96000004 [#1] PREEMPT SMP
> [ 32.032198] CPU: 0 PID: 934 Comm: kworker/0:2 Not tainted 4.11.0-rc3+ #1259
> [ 32.039573] Hardware name: Renesas Salvator-X board based on r8a7795 (DT)
> [ 32.046814] Workqueue: events deferred_probe_work_func
> [ 32.052405] task: ffff8006fad2d800 task.stack: ffff8006f90b8000
> [ 32.058809] PC is at __of_clk_get_from_provider+0x174/0x1b0
> [ 32.064878] LR is at __of_clk_get_from_provider+0x164/0x1b0
> ....
> [ 32.746677] [<ffff0000083b581c>] __of_clk_get_from_provider+0x174/0x1b0
> [ 32.754131] [<ffff0000083aded4>] __of_clk_get_by_name+0x104/0x140
> [ 32.761058] [<ffff0000083adfd8>] of_clk_get_by_name+0x30/0x50
> [ 32.767630] [<ffff0000083add1c>] devm_get_clk_from_child+0x54/0xb0
> ...
>
> I tried to find the criminal point, but, I couldn't specify where it is.
> Sometimes it is NULL pointer access crashed,
> sometimes it is crashed on of_clk_src_onecell_get(),
> sometimes there is no Oops.
> I want to solve this issue, but I want to know about of_clk_put as 1st step.
>
> In devm_clk_get() case, it is using clk_get() <-> clk_put() pair, this is OK.
> In devm_get_clk_from_child() case, it is using of_clk_get_by_name() (= __of_clk_get())
> <-> clk_put() pair.
> If my understand was correct, __of_clk_get() uses __clk_create_clk(),
> so, its pair should be __clk_free_clk() ?
> I wonder I can find of_clk_get(), but couldn't find of_clk_put().
> Is using of_clk_get() <-> clk_put() OK ?
>
> Best regards
> ---
> Kuninori Morimoto
> _______________________________________________
> Alsa-devel mailing list
> [email protected]
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel