[PATCH] ASoC: rsnd: use device dependency clock

[PATCH] ASoC: rsnd: use device dependency clock

[Kuninori Morimoto]

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

Current R-Car sound driver is using device
independent audio clock, but it is not good
design for DT (= common clock) support.
This patch adds device dependent clock support.
But, there are some platform which is using
old style clock.
Old style clock is still supported
at this point, but it will be removed soon.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/sh/rcar/adg.c |   35 ++++++++++++++++++++++++++++++-----
 1 file changed, 30 insertions(+), 5 deletions(-)

diff --git a/sound/soc/sh/rcar/adg.c b/sound/soc/sh/rcar/adg.c
index a53235c..e82ab75 100644
--- a/sound/soc/sh/rcar/adg.c
+++ b/sound/soc/sh/rcar/adg.c
@@ -259,8 +259,9 @@ int rsnd_adg_probe(struct platform_device *pdev,
 {
 	struct rsnd_adg *adg;
 	struct device *dev = rsnd_priv_to_dev(priv);
-	struct clk *clk;
+	struct clk *clk, *clk_orig;
 	int i;
+	bool use_old_style = false;
 
 	adg = devm_kzalloc(dev, sizeof(*adg), GFP_KERNEL);
 	if (!adg) {
@@ -268,10 +269,34 @@ int rsnd_adg_probe(struct platform_device *pdev,
 		return -ENOMEM;
 	}
 
-	adg->clk[CLKA] = clk_get(NULL, "audio_clk_a");
-	adg->clk[CLKB] = clk_get(NULL, "audio_clk_b");
-	adg->clk[CLKC] = clk_get(NULL, "audio_clk_c");
-	adg->clk[CLKI] = clk_get(NULL, "audio_clk_internal");
+	clk_orig	= clk_get(dev, NULL);
+	adg->clk[CLKA]	= clk_get(dev, "clk_a");
+	adg->clk[CLKB]	= clk_get(dev, "clk_b");
+	adg->clk[CLKC]	= clk_get(dev, "clk_c");
+	adg->clk[CLKI]	= clk_get(dev, "clk_i");
+
+	for_each_rsnd_clk(clk, adg, i) {
+		if (clk_orig == clk) {
+			dev_warn(dev,
+				 "Audio clock doesn't support new style,"
+				 "use old style\n");
+			use_old_style = true;
+			break;
+		}
+	}
+
+	/*
+	 * note:
+	 * these exist in order to keep compatible with old style,
+	 * but will be removed soon
+	 */
+	if (use_old_style) {
+		adg->clk[CLKA] = clk_get(NULL, "audio_clk_a");
+		adg->clk[CLKB] = clk_get(NULL, "audio_clk_b");
+		adg->clk[CLKC] = clk_get(NULL, "audio_clk_c");
+		adg->clk[CLKI] = clk_get(NULL, "audio_clk_internal");
+	}
+
 	for_each_rsnd_clk(clk, adg, i) {
 		if (IS_ERR(clk)) {
 			dev_err(dev, "Audio clock failed\n");
-- 
1.7.9.5

Re: [PATCH] ASoC: rsnd: use device dependency clock

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 902 bytes --]

On Thu, Jan 30, 2014 at 11:30:26PM -0800, Kuninori Morimoto wrote:

> +	clk_orig	= clk_get(dev, NULL);
> +	adg->clk[CLKA]	= clk_get(dev, "clk_a");
> +	adg->clk[CLKB]	= clk_get(dev, "clk_b");
> +	adg->clk[CLKC]	= clk_get(dev, "clk_c");
> +	adg->clk[CLKI]	= clk_get(dev, "clk_i");
> +
> +	for_each_rsnd_clk(clk, adg, i) {
> +		if (clk_orig == clk) {
> +			dev_warn(dev,
> +				 "Audio clock doesn't support new style,"
> +				 "use old style\n");
> +			use_old_style = true;
> +			break;
> +		}
> +	}

This looks really strange though I appreciate it's a workaround for a
custom clock implementation.  I guess it's not possible for a kernel to
be built with both common and legacy clock implementations?  If that is
the case I think it would be clearer to just use an ifdef.

It's also better to not split error messages between lines, that makes
it easier to grep for the code that generated the error.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: rsnd: use device dependency clock

[Kuninori Morimoto]

Hi Mark

> > +	clk_orig	= clk_get(dev, NULL);
> > +	adg->clk[CLKA]	= clk_get(dev, "clk_a");
> > +	adg->clk[CLKB]	= clk_get(dev, "clk_b");
> > +	adg->clk[CLKC]	= clk_get(dev, "clk_c");
> > +	adg->clk[CLKI]	= clk_get(dev, "clk_i");
> > +
> > +	for_each_rsnd_clk(clk, adg, i) {
> > +		if (clk_orig == clk) {
> > +			dev_warn(dev,
> > +				 "Audio clock doesn't support new style,"
> > +				 "use old style\n");
> > +			use_old_style = true;
> > +			break;
> > +		}
> > +	}
> 
> This looks really strange though I appreciate it's a workaround for a
> custom clock implementation.  I guess it's not possible for a kernel to
> be built with both common and legacy clock implementations?  If that is
> the case I think it would be clearer to just use an ifdef.

Sorry, I guess my comment makes confusion.
Here "new style" means "external sound clock has relationship with sound driver",
"old style" means "external sound clock is independent clock"

This patch adds new clock-relationship rule to rsnd <-> platform.
(sound clock should has relationship to sound drvier)

The external sound clocks (= clk_a/b/c/i) are "independent" clock
in current linus/master.
Then, all clk_get(dev, xxx) will be same clock (= module clock)
(It was not strange if clk_get(dev, xxx) returns NULL though, but...)

# Above code can works well both common/legacy clock implementations.
# And, it can't select both common/legacy clock in same time.

I will exchange git log comment (= about "new/old" clock),
is it OK ?

> It's also better to not split error messages between lines, that makes
> it easier to grep for the code that generated the error.

I see
I will fix it in v2 patch

Re: [PATCH] ASoC: rsnd: use device dependency clock

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1025 bytes --]

On Tue, Feb 04, 2014 at 04:44:34PM -0800, Kuninori Morimoto wrote:

> Here "new style" means "external sound clock has relationship with sound driver",
> "old style" means "external sound clock is independent clock"

> This patch adds new clock-relationship rule to rsnd <-> platform.
> (sound clock should has relationship to sound drvier)

> The external sound clocks (= clk_a/b/c/i) are "independent" clock
> in current linus/master.
> Then, all clk_get(dev, xxx) will be same clock (= module clock)
> (It was not strange if clk_get(dev, xxx) returns NULL though, but...)

> # Above code can works well both common/legacy clock implementations.
> # And, it can't select both common/legacy clock in same time.

> I will exchange git log comment (= about "new/old" clock),
> is it OK ?

OK, I'm hoping that this situation is not expected to last more than a
kernel release or so until all the platforms are converted - can we have
a comment explaining the issue giving an ETA in the code as well as in
the changelog please?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH v2] ASoC: rsnd: use device dependency clock

[Kuninori Morimoto]

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

Current R-Car sound driver is using device
independent audio clock, but it is not good
design for DT support.
This patch adds device dependent clock support.
But, there are some platform which is using
independent audio clock.
It is still supported at this point,
but it will be removed soon.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
v1 -> v2

 - fixup git log comment
 - add explain on code

 sound/soc/sh/rcar/adg.c |   40 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 35 insertions(+), 5 deletions(-)

diff --git a/sound/soc/sh/rcar/adg.c b/sound/soc/sh/rcar/adg.c
index 821791e..8d3a82e 100644
--- a/sound/soc/sh/rcar/adg.c
+++ b/sound/soc/sh/rcar/adg.c
@@ -385,8 +385,9 @@ int rsnd_adg_probe(struct platform_device *pdev,
 {
 	struct rsnd_adg *adg;
 	struct device *dev = rsnd_priv_to_dev(priv);
-	struct clk *clk;
+	struct clk *clk, *clk_orig;
 	int i;
+	bool use_old_style = false;
 
 	adg = devm_kzalloc(dev, sizeof(*adg), GFP_KERNEL);
 	if (!adg) {
@@ -394,10 +395,39 @@ int rsnd_adg_probe(struct platform_device *pdev,
 		return -ENOMEM;
 	}
 
-	adg->clk[CLKA] = clk_get(NULL, "audio_clk_a");
-	adg->clk[CLKB] = clk_get(NULL, "audio_clk_b");
-	adg->clk[CLKC] = clk_get(NULL, "audio_clk_c");
-	adg->clk[CLKI] = clk_get(NULL, "audio_clk_internal");
+	clk_orig	= clk_get(dev, NULL);
+	adg->clk[CLKA]	= clk_get(dev, "clk_a");
+	adg->clk[CLKB]	= clk_get(dev, "clk_b");
+	adg->clk[CLKC]	= clk_get(dev, "clk_c");
+	adg->clk[CLKI]	= clk_get(dev, "clk_i");
+
+	/*
+	 * It request device dependent audio clock.
+	 * But above all clks will indicate rsnd module clock
+	 * if platform doesn't it
+	 */
+	for_each_rsnd_clk(clk, adg, i) {
+		if (clk_orig == clk) {
+			dev_warn(dev,
+				 "doesn't have device dependent clock, use independent clock\n");
+			use_old_style = true;
+			break;
+		}
+	}
+
+	/*
+	 * note:
+	 * these exist in order to keep compatible with
+	 * platform which has device independent audio clock,
+	 * but will be removed soon
+	 */
+	if (use_old_style) {
+		adg->clk[CLKA] = clk_get(NULL, "audio_clk_a");
+		adg->clk[CLKB] = clk_get(NULL, "audio_clk_b");
+		adg->clk[CLKC] = clk_get(NULL, "audio_clk_c");
+		adg->clk[CLKI] = clk_get(NULL, "audio_clk_internal");
+	}
+
 	for_each_rsnd_clk(clk, adg, i) {
 		if (IS_ERR(clk)) {
 			dev_err(dev, "Audio clock failed\n");
-- 
1.7.9.5

Re: [PATCH v2] ASoC: rsnd: use device dependency clock

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 273 bytes --]

On Fri, Feb 07, 2014 at 12:53:06AM -0800, Kuninori Morimoto wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> Current R-Car sound driver is using device
> independent audio clock, but it is not good
> design for DT support.

Applied, thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]