Re: [alsa-devel] [PATCH] ASoC: wm8994: Manage wm8994's MCLK in codec driver

2015-03-12 Thread Mark Brown 
On Thu, Mar 12, 2015 at 11:49:39AM +0900, Inha Song wrote:

> - if (codec->dapm.bias_level == SND_SOC_BIAS_STANDBY)
> + if (codec->dapm.bias_level == SND_SOC_BIAS_STANDBY) {
> + for (i = 0; i < ARRAY_SIZE(wm8994->clk_mclk); i++) {
> + if (!IS_ERR(wm8994->clk_mclk[i]))
> + clk_prepare_enable(wm8994->clk_mclk[i]);
> + }

This isn't checking return values.  It's also enabling both clocks all
the time which isn't always going to be what's desired - it may result
in an expensive high frequency reference clock being enabled when only
a 32kHz clock is needed for example.  That doesn't seem desirable.

>   active_reference(codec);

Should this perhaps be being done in active_reference instead?


signature.asc
Description: Digital signature

Re: [alsa-devel] [PATCH] ASoC: wm8994: Manage wm8994's MCLK in codec driver

2015-03-11 Thread Inha Song 
Hi, Chanwoo,

Thanks for your comments.

On Thu, 12 Mar 2015 13:33:49 +0900
Chanwoo Choi  wrote:

> Hi Inha,
> 
> On 03/12/2015 11:49 AM, Inha Song wrote:
> > Previously, When we use wm8994 codec, we should control its MCLK in machine 
> > driver.
> > But, It should be managed by wm8994 codec driver, not machine driver.
> > 
> > This patch add MCLK clock DT parsing logic and control own MCLK.
> > 
> > Signed-off-by: Inha Song 
> > ---
> >  sound/soc/codecs/wm8994.c | 27 ++-
> >  sound/soc/codecs/wm8994.h |  1 +
> >  2 files changed, 27 insertions(+), 1 deletion(-)
> > 
> > diff --git a/sound/soc/codecs/wm8994.c b/sound/soc/codecs/wm8994.c
> > index 247b390..d80dcc6 100644
> > --- a/sound/soc/codecs/wm8994.c
> > +++ b/sound/soc/codecs/wm8994.c
> > @@ -16,6 +16,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -2470,6 +2471,7 @@ static int wm8994_set_bias_level(struct snd_soc_codec 
> > *codec,
> >  {
> > struct wm8994_priv *wm8994 = snd_soc_codec_get_drvdata(codec);
> > struct wm8994 *control = wm8994->wm8994;
> > +   int i;
> >  
> > wm_hubs_set_bias_level(codec, level);
> >  
> > @@ -2491,8 +2493,19 @@ static int wm8994_set_bias_level(struct 
> > snd_soc_codec *codec,
> > break;
> > }
> >  
> > -   if (codec->dapm.bias_level == SND_SOC_BIAS_STANDBY)
> > +   if (codec->dapm.bias_level == SND_SOC_BIAS_STANDBY) {
> > +   for (i = 0; i < ARRAY_SIZE(wm8994->clk_mclk); i++) {
> > +   if (!IS_ERR(wm8994->clk_mclk[i]))
> > +   clk_prepare_enable(wm8994->clk_mclk[i]);
> 
> I think that you don't need to check error by using 'if' statement.
> clk_prepare_enable() function will check the NULL state of clk instance
> before preparing and enabling the clock. 

So that, I can handle it more concisely. Thanks, I will fix this.

> 
>   for (i = 0; i < ARRAY_SIZE(wm8994->clk_mclk); i++)
>   clk_prepare_enable(wm8994->clk_mclk[i]);
> 
> But, wm8994_probe() have to initialize the NULL for wm8994->clk_mclk[i].
> I added the comment about this on below.
> 
> > +   }
> > active_reference(codec);
> > +   } else if (codec->dapm.bias_level == SND_SOC_BIAS_ON) {
> > +   for (i = 0; i < ARRAY_SIZE(wm8994->clk_mclk); i++) {
> > +   if (!IS_ERR(wm8994->clk_mclk[i]))
> > +   clk_disable_unprepare(
> > +   wm8994->clk_mclk[i]);

ditto

> > +   }
> > +   }
> > break;
> >  
> > case SND_SOC_BIAS_STANDBY:
> > @@ -4451,6 +4464,7 @@ static struct snd_soc_codec_driver 
> > soc_codec_dev_wm8994 = {
> >  static int wm8994_probe(struct platform_device *pdev)
> >  {
> > struct wm8994_priv *wm8994;
> > +   int i;
> >  
> > wm8994 = devm_kzalloc(&pdev->dev, sizeof(struct wm8994_priv),
> >   GFP_KERNEL);
> > @@ -4460,6 +4474,17 @@ static int wm8994_probe(struct platform_device *pdev)
> >  
> > wm8994->wm8994 = dev_get_drvdata(pdev->dev.parent);
> >  
> > +   for (i = 0; i < ARRAY_SIZE(wm8994->clk_mclk); i++) {
> > +   char mclk_name[] = "MCLKx";
> > +
> > +   snprintf(mclk_name, sizeof(mclk_name), "MCLK%d", i + 1);
> > +
> > +   wm8994->clk_mclk[i] = devm_clk_get(pdev->dev.parent, mclk_name);
> > +   if (IS_ERR(wm8994->clk_mclk[i]))
> > +   if (PTR_ERR(wm8994->clk_mclk) == -EPROBE_DEFER)
> > +   return -EPROBE_DEFER;
> 
> I think you better to modify exception handling as following:
> 
>   if (IS_ERR(wm8994->clk_mclk[i])) {
>   wm8994->clk_mclk[i] = NULL;
>   return PTR_ERR(wm8994->clk_mclk);

I will fix. (MCLK is optional properties in wm8994)

if (IS_ERR(wm8994->clk_mclk[i]))
wm8994->clk_mclk[i] = NULL;

Best Regards,
Inha Song.

>   }
> 
> > +   }
> > +
> > pm_runtime_enable(&pdev->dev);
> > pm_runtime_idle(&pdev->dev);
> >  
> > diff --git a/sound/soc/codecs/wm8994.h b/sound/soc/codecs/wm8994.h
> > index 6536f8d..8cf06a4 100644
> > --- a/sound/soc/codecs/wm8994.h
> > +++ b/sound/soc/codecs/wm8994.h
> > @@ -78,6 +78,7 @@ struct wm8994_priv {
> > int sysclk[2];
> > int sysclk_rate[2];
> > int mclk[2];
> > +   struct clk *clk_mclk[2];
> > int aifclk[2];
> > int aifdiv[2];
> > int channels[2];
> > 
> 
> Thanks,
> Chanwoo Choi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [alsa-devel] [PATCH] ASoC: wm8994: Manage wm8994's MCLK in codec driver

2015-03-11 Thread Chanwoo Choi 
Hi Inha,

On 03/12/2015 11:49 AM, Inha Song wrote:
> Previously, When we use wm8994 codec, we should control its MCLK in machine 
> driver.
> But, It should be managed by wm8994 codec driver, not machine driver.
> 
> This patch add MCLK clock DT parsing logic and control own MCLK.
> 
> Signed-off-by: Inha Song 
> ---
>  sound/soc/codecs/wm8994.c | 27 ++-
>  sound/soc/codecs/wm8994.h |  1 +
>  2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/soc/codecs/wm8994.c b/sound/soc/codecs/wm8994.c
> index 247b390..d80dcc6 100644
> --- a/sound/soc/codecs/wm8994.c
> +++ b/sound/soc/codecs/wm8994.c
> @@ -16,6 +16,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -2470,6 +2471,7 @@ static int wm8994_set_bias_level(struct snd_soc_codec 
> *codec,
>  {
>   struct wm8994_priv *wm8994 = snd_soc_codec_get_drvdata(codec);
>   struct wm8994 *control = wm8994->wm8994;
> + int i;
>  
>   wm_hubs_set_bias_level(codec, level);
>  
> @@ -2491,8 +2493,19 @@ static int wm8994_set_bias_level(struct snd_soc_codec 
> *codec,
>   break;
>   }
>  
> - if (codec->dapm.bias_level == SND_SOC_BIAS_STANDBY)
> + if (codec->dapm.bias_level == SND_SOC_BIAS_STANDBY) {
> + for (i = 0; i < ARRAY_SIZE(wm8994->clk_mclk); i++) {
> + if (!IS_ERR(wm8994->clk_mclk[i]))
> + clk_prepare_enable(wm8994->clk_mclk[i]);

I think that you don't need to check error by using 'if' statement.
clk_prepare_enable() function will check the NULL state of clk instance
before preparing and enabling the clock. 

for (i = 0; i < ARRAY_SIZE(wm8994->clk_mclk); i++)
clk_prepare_enable(wm8994->clk_mclk[i]);

But, wm8994_probe() have to initialize the NULL for wm8994->clk_mclk[i].
I added the comment about this on below.

> + }
>   active_reference(codec);
> + } else if (codec->dapm.bias_level == SND_SOC_BIAS_ON) {
> + for (i = 0; i < ARRAY_SIZE(wm8994->clk_mclk); i++) {
> + if (!IS_ERR(wm8994->clk_mclk[i]))
> + clk_disable_unprepare(
> + wm8994->clk_mclk[i]);
> + }
> + }
>   break;
>  
>   case SND_SOC_BIAS_STANDBY:
> @@ -4451,6 +4464,7 @@ static struct snd_soc_codec_driver soc_codec_dev_wm8994 
> = {
>  static int wm8994_probe(struct platform_device *pdev)
>  {
>   struct wm8994_priv *wm8994;
> + int i;
>  
>   wm8994 = devm_kzalloc(&pdev->dev, sizeof(struct wm8994_priv),
> GFP_KERNEL);
> @@ -4460,6 +4474,17 @@ static int wm8994_probe(struct platform_device *pdev)
>  
>   wm8994->wm8994 = dev_get_drvdata(pdev->dev.parent);
>  
> + for (i = 0; i < ARRAY_SIZE(wm8994->clk_mclk); i++) {
> + char mclk_name[] = "MCLKx";
> +
> + snprintf(mclk_name, sizeof(mclk_name), "MCLK%d", i + 1);
> +
> + wm8994->clk_mclk[i] = devm_clk_get(pdev->dev.parent, mclk_name);
> + if (IS_ERR(wm8994->clk_mclk[i]))
> + if (PTR_ERR(wm8994->clk_mclk) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;

I think you better to modify exception handling as following:

if (IS_ERR(wm8994->clk_mclk[i])) {
wm8994->clk_mclk[i] = NULL;
return PTR_ERR(wm8994->clk_mclk);
}

> + }
> +
>   pm_runtime_enable(&pdev->dev);
>   pm_runtime_idle(&pdev->dev);
>  
> diff --git a/sound/soc/codecs/wm8994.h b/sound/soc/codecs/wm8994.h
> index 6536f8d..8cf06a4 100644
> --- a/sound/soc/codecs/wm8994.h
> +++ b/sound/soc/codecs/wm8994.h
> @@ -78,6 +78,7 @@ struct wm8994_priv {
>   int sysclk[2];
>   int sysclk_rate[2];
>   int mclk[2];
> + struct clk *clk_mclk[2];
>   int aifclk[2];
>   int aifdiv[2];
>   int channels[2];
> 

Thanks,
Chanwoo Choi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[alsa-devel] [PATCH] ASoC: wm8994: Manage wm8994's MCLK in codec driver

2015-03-11 Thread Inha Song 
Previously, When we use wm8994 codec, we should control its MCLK in machine 
driver.
But, It should be managed by wm8994 codec driver, not machine driver.

This patch add MCLK clock DT parsing logic and control own MCLK.

Signed-off-by: Inha Song 
---
 sound/soc/codecs/wm8994.c | 27 ++-
 sound/soc/codecs/wm8994.h |  1 +
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/wm8994.c b/sound/soc/codecs/wm8994.c
index 247b390..d80dcc6 100644
--- a/sound/soc/codecs/wm8994.c
+++ b/sound/soc/codecs/wm8994.c
@@ -16,6 +16,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -2470,6 +2471,7 @@ static int wm8994_set_bias_level(struct snd_soc_codec 
*codec,
 {
struct wm8994_priv *wm8994 = snd_soc_codec_get_drvdata(codec);
struct wm8994 *control = wm8994->wm8994;
+   int i;
 
wm_hubs_set_bias_level(codec, level);
 
@@ -2491,8 +2493,19 @@ static int wm8994_set_bias_level(struct snd_soc_codec 
*codec,
break;
}
 
-   if (codec->dapm.bias_level == SND_SOC_BIAS_STANDBY)
+   if (codec->dapm.bias_level == SND_SOC_BIAS_STANDBY) {
+   for (i = 0; i < ARRAY_SIZE(wm8994->clk_mclk); i++) {
+   if (!IS_ERR(wm8994->clk_mclk[i]))
+   clk_prepare_enable(wm8994->clk_mclk[i]);
+   }
active_reference(codec);
+   } else if (codec->dapm.bias_level == SND_SOC_BIAS_ON) {
+   for (i = 0; i < ARRAY_SIZE(wm8994->clk_mclk); i++) {
+   if (!IS_ERR(wm8994->clk_mclk[i]))
+   clk_disable_unprepare(
+   wm8994->clk_mclk[i]);
+   }
+   }
break;
 
case SND_SOC_BIAS_STANDBY:
@@ -4451,6 +4464,7 @@ static struct snd_soc_codec_driver soc_codec_dev_wm8994 = 
{
 static int wm8994_probe(struct platform_device *pdev)
 {
struct wm8994_priv *wm8994;
+   int i;
 
wm8994 = devm_kzalloc(&pdev->dev, sizeof(struct wm8994_priv),
  GFP_KERNEL);
@@ -4460,6 +4474,17 @@ static int wm8994_probe(struct platform_device *pdev)
 
wm8994->wm8994 = dev_get_drvdata(pdev->dev.parent);
 
+   for (i = 0; i < ARRAY_SIZE(wm8994->clk_mclk); i++) {
+   char mclk_name[] = "MCLKx";
+
+   snprintf(mclk_name, sizeof(mclk_name), "MCLK%d", i + 1);
+
+   wm8994->clk_mclk[i] = devm_clk_get(pdev->dev.parent, mclk_name);
+   if (IS_ERR(wm8994->clk_mclk[i]))
+   if (PTR_ERR(wm8994->clk_mclk) == -EPROBE_DEFER)
+   return -EPROBE_DEFER;
+   }
+
pm_runtime_enable(&pdev->dev);
pm_runtime_idle(&pdev->dev);
 
diff --git a/sound/soc/codecs/wm8994.h b/sound/soc/codecs/wm8994.h
index 6536f8d..8cf06a4 100644
--- a/sound/soc/codecs/wm8994.h
+++ b/sound/soc/codecs/wm8994.h
@@ -78,6 +78,7 @@ struct wm8994_priv {
int sysclk[2];
int sysclk_rate[2];
int mclk[2];
+   struct clk *clk_mclk[2];
int aifclk[2];
int aifdiv[2];
int channels[2];
-- 
2.0.0.390.gcb682f8

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/