[PATCH 09/10] S3C64XX I2S: Set parent links for clock audio-bus.

[PATCH 09/10] S3C64XX I2S: Set parent links for clock audio-bus.

[Jassi]

Explicitly route audio-bus from FOUTepll via MOUTepll.

Signed-Off-by: Jassi <jassi.brar@samsung.com>
---
 sound/soc/s3c24xx/s3c64xx-i2s.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/sound/soc/s3c24xx/s3c64xx-i2s.c b/sound/soc/s3c24xx/s3c64xx-i2s.c
index 66f4ded..71aeb33 100644
--- a/sound/soc/s3c24xx/s3c64xx-i2s.c
+++ b/sound/soc/s3c24xx/s3c64xx-i2s.c
@@ -254,6 +254,7 @@ static __devinit int s3c64xx_iis_dev_probe(struct platform_device *pdev)
 {
 	struct s3c_i2sv2_info *i2s;
 	struct snd_soc_dai *dai;
+	struct clk *cm, *cf;
 	int ret;
 
 	if (pdev->id >= ARRAY_SIZE(s3c64xx_i2s)) {
@@ -275,6 +276,21 @@ static __devinit int s3c64xx_iis_dev_probe(struct platform_device *pdev)
 		goto err;
 	}
 
+	cm = clk_get(NULL, "mout_epll");
+	if (IS_ERR(cm)) {
+		dev_err(&pdev->dev, "failed to get mout_epll\n");
+		ret = PTR_ERR(cm);
+		goto mout_err;
+	}
+	clk_set_parent(i2s->iis_cclk, cm);
+	cf = clk_get(NULL, "fout_epll");
+	if (IS_ERR(cf)) {
+		dev_err(&pdev->dev, "failed to get fout_epll\n");
+		ret = PTR_ERR(cf);
+		goto fout_err;
+	}
+	clk_set_parent(cm, cf);
+
 	ret = s3c_i2sv2_probe(pdev, dai, i2s, 0);
 	if (ret)
 		goto err_clk;
@@ -283,11 +299,17 @@ static __devinit int s3c64xx_iis_dev_probe(struct platform_device *pdev)
 	if (ret != 0)
 		goto err_i2sv2;
 
+	clk_put(cf);
+	clk_put(cm);
 	return 0;
 
 err_i2sv2:
 	/* Not implemented for I2Sv2 core yet */
 err_clk:
+	clk_put(cf);
+fout_err:
+	clk_put(cm);
+mout_err:
 	clk_put(i2s->iis_cclk);
 err:
 	return ret;
-- 
1.6.2.5

[PATCH 09/10] S3C64XX I2S: Set parent links for clock audio-bus.

[Mark Brown]

On Tue, Sep 15, 2009 at 07:02:41PM +0900, Jassi wrote:
> Explicitly route audio-bus from FOUTepll via MOUTepll.

> Signed-Off-by: Jassi <jassi.brar@samsung.com>

Why do this in the audio driver rather than doing it in the arch/arm
code?  It seems bad form for the audio driver to be looking at clock
configuration outside its domain.

> +	cm = clk_get(NULL, "mout_epll");
> +	if (IS_ERR(cm)) {
> +		dev_err(&pdev->dev, "failed to get mout_epll\n");
> +		ret = PTR_ERR(cm);
> +		goto mout_err;
> +	}
> +	clk_set_parent(i2s->iis_cclk, cm);
> +	cf = clk_get(NULL, "fout_epll");
> +	if (IS_ERR(cf)) {
> +		dev_err(&pdev->dev, "failed to get fout_epll\n");
> +		ret = PTR_ERR(cf);
> +		goto fout_err;
> +	}
> +	clk_set_parent(cm, cf);

The mout/fout connection in particular isn't audio local, there's way
more EPLL users than just the audio.

[PATCH 09/10] S3C64XX I2S: Set parent links for clock audio-bus.

[jassi brar]

On Tue, Sep 15, 2009 at 8:12 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Tue, Sep 15, 2009 at 07:02:41PM +0900, Jassi wrote:
>> Explicitly route audio-bus from FOUTepll via MOUTepll.
>
>> Signed-Off-by: Jassi <jassi.brar@samsung.com>
>
> Why do this in the audio driver rather than doing it in the arch/arm
> code? ?It seems bad form for the audio driver to be looking at clock
> configuration outside its domain.
I understand that.
Even without this patch it sources FOUTepll by default.
This is not meant to be forever.  I patched it because
I didn't want to get stuck making platform perfect to push the idea.

>> + ? ? cm = clk_get(NULL, "mout_epll");
>> + ? ? if (IS_ERR(cm)) {
>> + ? ? ? ? ? ? dev_err(&pdev->dev, "failed to get mout_epll\n");
>> + ? ? ? ? ? ? ret = PTR_ERR(cm);
>> + ? ? ? ? ? ? goto mout_err;
>> + ? ? }
>> + ? ? clk_set_parent(i2s->iis_cclk, cm);
>> + ? ? cf = clk_get(NULL, "fout_epll");
>> + ? ? if (IS_ERR(cf)) {
>> + ? ? ? ? ? ? dev_err(&pdev->dev, "failed to get fout_epll\n");
>> + ? ? ? ? ? ? ret = PTR_ERR(cf);
>> + ? ? ? ? ? ? goto fout_err;
>> + ? ? }
>> + ? ? clk_set_parent(cm, cf);
>
> The mout/fout connection in particular isn't audio local, there's way
> more EPLL users than just the audio.
Ofcourse, but i think those drivers and requirements are far off that
may need EPLL.
EPLL is a shared but unused resource. Using EPLL gives far more accurate clocks
and we shudn't keep hands off just because there is no arbiter.
If not here, i favor controlling it from machine specific code.

[PATCH 09/10] S3C64XX I2S: Set parent links for clock audio-bus.

[Mark Brown]

On Tue, Sep 15, 2009 at 08:42:32PM +0900, jassi brar wrote:
> On Tue, Sep 15, 2009 at 8:12 PM, Mark Brown

> > code? ?It seems bad form for the audio driver to be looking at clock
> > configuration outside its domain.

> I understand that.
> Even without this patch it sources FOUTepll by default.
> This is not meant to be forever.  I patched it because
> I didn't want to get stuck making platform perfect to push the idea.

I'm not saying don't touch this, I'm saying that this doesn't belong in
this driver.

> > The mout/fout connection in particular isn't audio local, there's way
> > more EPLL users than just the audio.

> Ofcourse, but i think those drivers and requirements are far off that
> may need EPLL.

Are you sure?  UART, USB, MMC and SPI all have options to clock off EPLL
- I've worked with designs that were actively using it for at least USB.

> EPLL is a shared but unused resource. Using EPLL gives far more accurate clocks
> and we shudn't keep hands off just because there is no arbiter.

That doesn't seem like it's going to work so well, especially if Samsung
are actively pushing more drivers into mainline - if two drivers start
trying to play about with this simultaneously then what'll happen is
that users who end up trying to use those drivers together will have to
deal with the fallout.

> If not here, i favor controlling it from machine specific code.

Well, for the EPLL mux/PLL connection I'm not sure why we're not just
doing it in the default clock setup - I had thought that that was being
done by default already to be honest but I've not checked.

For anything where there's serious decisions to be taken it does need to
be machine-specific but if the clock implementation were to choose
sensible defaults I wouldn't see any problem with that, though I don't
know what Ben thinks.

[PATCH 09/10] S3C64XX I2S: Set parent links for clock audio-bus.

[jassi brar]

On Tue, Sep 15, 2009 at 9:15 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Tue, Sep 15, 2009 at 08:42:32PM +0900, jassi brar wrote:
>> On Tue, Sep 15, 2009 at 8:12 PM, Mark Brown
>
>> > The mout/fout connection in particular isn't audio local, there's way
>> > more EPLL users than just the audio.
>
>> Ofcourse, but i think those drivers and requirements are far off that
>> may need EPLL.
>
> Are you sure? ?UART, USB, MMC and SPI all have options to clock off EPLL
> - I've worked with designs that were actively using it for at least USB.
In mainline?
All these devices do have option to source from EPLL but they don't
seem to use it.

>> EPLL is a shared but unused resource. Using EPLL gives far more accurate clocks
>> and we shudn't keep hands off just because there is no arbiter.
>
> That doesn't seem like it's going to work so well, especially if Samsung
> are actively pushing more drivers into mainline - if two drivers start
> trying to play about with this simultaneously then what'll happen is
> that users who end up trying to use those drivers together will have to
> deal with the fallout.
as i said in another reply, current set of patches are meant to be as
simple as possible.
once that is done I am all to implement and support new features.

>> If not here, i favor controlling it from machine specific code.
>
> Well, for the EPLL mux/PLL connection I'm not sure why we're not just
> doing it in the default clock setup - I had thought that that was being
> done by default already to be honest but I've not checked.
yes, we can simply do clk_setparent in platform/machine code. thats
what some of
samsung-git drivers do.

[PATCH 09/10] S3C64XX I2S: Set parent links for clock audio-bus.

[Mark Brown]

On Tue, Sep 15, 2009 at 09:57:05PM +0900, jassi brar wrote:
> On Tue, Sep 15, 2009 at 9:15 PM, Mark Brown

> > Are you sure? ?UART, USB, MMC and SPI all have options to clock off EPLL
> > - I've worked with designs that were actively using it for at least USB.

> In mainline?
> All these devices do have option to source from EPLL but they don't
> seem to use it.

I'd hope that given the active efforts that Samsung are making to push
things into mainline anything that isn't in mainline will be there
sooner rather than later.  Given that it seems better to avoid things
like this which will create issues after merge to mainline without
consideration for how things are supposed to work after the merge.

> > trying to play about with this simultaneously then what'll happen is
> > that users who end up trying to use those drivers together will have to
> > deal with the fallout.

> as i said in another reply, current set of patches are meant to be as
> simple as possible.
> once that is done I am all to implement and support new features.

Something with this much of a system wide effect really isn't a good
idea - the wider the effect of what you're doing the worse the effect of
any breakage that is introduced.

[PATCH 09/10] S3C64XX I2S: Set parent links for clock audio-bus.

[Ben Dooks]

On Tue, Sep 15, 2009 at 07:02:41PM +0900, Jassi wrote:
> Explicitly route audio-bus from FOUTepll via MOUTepll.
> 
> Signed-Off-by: Jassi <jassi.brar@samsung.com>
> ---
>  sound/soc/s3c24xx/s3c64xx-i2s.c |   22 ++++++++++++++++++++++
>  1 files changed, 22 insertions(+), 0 deletions(-)
> 
> diff --git a/sound/soc/s3c24xx/s3c64xx-i2s.c b/sound/soc/s3c24xx/s3c64xx-i2s.c
> index 66f4ded..71aeb33 100644
> --- a/sound/soc/s3c24xx/s3c64xx-i2s.c
> +++ b/sound/soc/s3c24xx/s3c64xx-i2s.c
> @@ -254,6 +254,7 @@ static __devinit int s3c64xx_iis_dev_probe(struct platform_device *pdev)
>  {
>  	struct s3c_i2sv2_info *i2s;
>  	struct snd_soc_dai *dai;
> +	struct clk *cm, *cf;
>  	int ret;
>  
>  	if (pdev->id >= ARRAY_SIZE(s3c64xx_i2s)) {
> @@ -275,6 +276,21 @@ static __devinit int s3c64xx_iis_dev_probe(struct platform_device *pdev)
>  		goto err;
>  	}
>  
> +	cm = clk_get(NULL, "mout_epll");
> +	if (IS_ERR(cm)) {
> +		dev_err(&pdev->dev, "failed to get mout_epll\n");
> +		ret = PTR_ERR(cm);
> +		goto mout_err;
> +	}
> +	clk_set_parent(i2s->iis_cclk, cm);
> +	cf = clk_get(NULL, "fout_epll");
> +	if (IS_ERR(cf)) {
> +		dev_err(&pdev->dev, "failed to get fout_epll\n");
> +		ret = PTR_ERR(cf);
> +		goto fout_err;
> +	}
> +	clk_set_parent(cm, cf);
> +
>  	ret = s3c_i2sv2_probe(pdev, dai, i2s, 0);
>  	if (ret)
>  		goto err_clk;
> @@ -283,11 +299,17 @@ static __devinit int s3c64xx_iis_dev_probe(struct platform_device *pdev)
>  	if (ret != 0)
>  		goto err_i2sv2;
>  
> +	clk_put(cf);
> +	clk_put(cm);
>  	return 0;
>  
>  err_i2sv2:
>  	/* Not implemented for I2Sv2 core yet */
>  err_clk:
> +	clk_put(cf);
> +fout_err:
> +	clk_put(cm);
> +mout_err:
>  	clk_put(i2s->iis_cclk);
>  err:
>  	return ret;

This is the wrong place to be changing the clocks, it is making a policy
for _all_ boards that may use this code. It either belongs in the board
setup code, possibly in the board specific ASoC glue driver (maybe with
a common library if enough do it).

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.