[PATCH 0/3] clkdev: add devm_get_clk_from_child()

[PATCH 0/3] clkdev: add devm_get_clk_from_child()

[Kuninori Morimoto]

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

[PATCH 1/3] clkdev: add devm_get_clk_from_child()

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

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 <kuninori.morimoto.gx@renesas.com>
---
 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

[PATCH 2/3] ASoC: simple-card: use devm_get_clk_from_child()

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

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 <kuninori.morimoto.gx@renesas.com>
---
 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

[PATCH 3/3] ASoC: simple-card-utils: enable clocks/clock-names/clock-ranges

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

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 <kuninori.morimoto.gx@renesas.com>
---
 .../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

Re: [PATCH 0/3] clkdev: add devm_get_clk_from_child()

[Stephen Boyd]

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

Re: [PATCH 3/3] ASoC: simple-card-utils: enable clocks/clock-names/clock-ranges

[Stephen Boyd]

On 12/05, Kuninori Morimoto wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> 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 <kuninori.morimoto.gx@renesas.com>

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

Re: [PATCH 2/3] ASoC: simple-card: use devm_get_clk_from_child()

[Stephen Boyd]

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

Re: [PATCH 2/3] ASoC: simple-card: use devm_get_clk_from_child()

[Kuninori Morimoto]

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

Re: [PATCH 3/3] ASoC: simple-card-utils: enable clocks/clock-names/clock-ranges

[Kuninori Morimoto]

Hi Stephen

> > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > 
> > 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 <kuninori.morimoto.gx@renesas.com>
> 
> 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 (?)

Re: [PATCH 3/3] ASoC: simple-card-utils: enable clocks/clock-names/clock-ranges

[Kuninori Morimoto]

Hi Stephen

> > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > 
> > 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 <kuninori.morimoto.gx@renesas.com>
> 
> 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 (?)

Re: [PATCH 0/3] clkdev: add devm_get_clk_from_child()

[Kuninori Morimoto]

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

Re: [PATCH 3/3] ASoC: simple-card-utils: enable clocks/clock-names/clock-ranges

[Stephen Boyd]

On 12/09, Kuninori Morimoto wrote:
> 
> Hi Stephen
> 
> > > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > > 
> > > 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 <kuninori.morimoto.gx@renesas.com>
> > 
> > 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

Re: [PATCH 2/3] ASoC: simple-card: use devm_get_clk_from_child()

[Stephen Boyd]

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

Re: [PATCH 2/3] ASoC: simple-card: use devm_get_clk_from_child()

[Kuninori Morimoto]

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

Re: [PATCH 3/3] ASoC: simple-card-utils: enable clocks/clock-names/clock-ranges

[Kuninori Morimoto]

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 :)

Re: [PATCH 1/3] clkdev: add devm_get_clk_from_child()

[Russell King - ARM Linux]

On Mon, Dec 05, 2016 at 05:23:20AM +0000, Kuninori Morimoto wrote:
> 
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> 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 <kuninori.morimoto.gx@renesas.com>

This looks lots better, thanks.

Acked-by: Russell King <rmk+kernel@armlinux.org.uk>

-- 
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.

Re: [PATCH 0/3] clkdev: add devm_get_clk_from_child()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 291 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/3] clkdev: add devm_get_clk_from_child()

[Kuninori Morimoto]

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 !

Applied "ASoC: simple-card: use devm_get_clk_from_child()" to the asoc tree

[Mark Brown]

The patch

   ASoC: simple-card: use devm_get_clk_from_child()

has been applied to the asoc tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From e984fd61e860ce3c45e79d69cf214b8cc6cae7d9 Mon Sep 17 00:00:00 2001
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Date: Mon, 23 Jan 2017 07:29:42 +0000
Subject: [PATCH] ASoC: simple-card: use devm_get_clk_from_child()

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 <kuninori.morimoto.gx@renesas.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 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 64e90ca9ad32..af58d2362975 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 cf026252cd4a..4924575d2e95 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 a385ff6bfa4b..85b4f1806514 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 bb86ee042490..308ff4c11a8d 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;
 
-- 
2.11.0

Question about of_clk_put ?

[Kuninori Morimoto]

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

Re: [alsa-devel] Question about of_clk_put ?

[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
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel