[RFC,v2] ASoC: wm8904: add CCF support
diff mbox

Message ID 1395741416-23981-1-git-send-email-voice.shen@atmel.com
State New, archived
Headers show

Commit Message

Bo Shen March 25, 2014, 9:56 a.m. UTC
Enable WM8904 to support common clock framework.
  - Now it supports use SoC provided clock or external oscillator
    as MCLK.

Signed-off-by: Bo Shen <voice.shen@atmel.com>
---
 Documentation/devicetree/bindings/sound/wm8904.txt | 48 ++++++++++++++++++++++
 sound/soc/codecs/wm8904.c                          | 17 ++++++++
 2 files changed, 65 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/wm8904.txt

Comments

Mark Brown March 25, 2014, 11:27 a.m. UTC | #1
On Tue, Mar 25, 2014 at 05:56:56PM +0800, Bo Shen wrote:

>  	case SND_SOC_BIAS_ON:
> +		if (IS_ENABLED(CONFIG_COMMON_CLK))
> +			if (wm8904->mclk)
> +				clk_prepare_enable(wm8904->mclk);

No, this shouldn't depend on COMMON_CLK - there is no reason other clock
API implementations shouldn't be able to use this.  Providing a clock is
something you can only do with COMMON_CLK but using one doesn't need
that, generally using a clock is done unconditionally.
Boris BREZILLON March 25, 2014, 11:59 a.m. UTC | #2
Hello Bo,

Le 25/03/2014 10:56, Bo Shen a écrit :
> Enable WM8904 to support common clock framework.
>    - Now it supports use SoC provided clock or external oscillator
>      as MCLK.
>
> Signed-off-by: Bo Shen <voice.shen@atmel.com>
> ---
>   Documentation/devicetree/bindings/sound/wm8904.txt | 48 ++++++++++++++++++++++
>   sound/soc/codecs/wm8904.c                          | 17 ++++++++
>   2 files changed, 65 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/sound/wm8904.txt
>
> diff --git a/Documentation/devicetree/bindings/sound/wm8904.txt b/Documentation/devicetree/bindings/sound/wm8904.txt
> new file mode 100644
> index 0000000..bcf4aa0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/wm8904.txt
> @@ -0,0 +1,48 @@
> +WM8904 audio CODEC
> +
> +This device supports I2C only.
> +
> +Required properties:
> +  - compatible: "wlf,wm8904"
> +  - reg: the I2C address of the device.
> +  - clocks and clock-names: reference to
> +    <Documentation/devicetree/bindings/clock/clock-bindings.txt>
> +
> +Pins on the device (for linking into audio routes):
> +
> +  * IN1L
> +  * IN1R
> +  * IN2L
> +  * IN2R
> +  * IN3L
> +  * IN3R
> +  * HPOUTL
> +  * HPOUTR
> +  * LINEOUTL
> +  * LINEOUTR
> +  * MICBIAS
> +
> +Examples:
> +
> +1. Using SoC provided clock as mclk:
> +codec: wm8904@1a {
> +	compatible = "wlf,wm8904";
> +	reg = <0x1a>;
> +	clocks = <&pck0>;
> +	clock-names = "mclk";
> +};


> +
> +2. Using external oscillator as mclk:
> +codec: wm8904@1a {
> +	compatible = "wlf,wm8904";
> +	reg = <0x1a>;
> +
> +	wm8904_osc {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		#clock-frequency = <12000000>;
> +	}
> +
> +	clocks = <&wm8904_osc>;
> +	clock-names = "mclk";
> +};

I think there is a misunderstanding here (and it certainly comes from my
explanations during our last talk :)): both cases should be handled the same
way because an external oscillator is no more or less than an external clk
(we don't care how this clk is generated).

I suggested to put the osc node inside the wm8904 one but I thought you were
trying to represent the internal 12MHz clk generated when using the fll free
running mode.

> diff --git a/sound/soc/codecs/wm8904.c b/sound/soc/codecs/wm8904.c
> index 49c35c3..4db6a26 100644
> --- a/sound/soc/codecs/wm8904.c
> +++ b/sound/soc/codecs/wm8904.c
> @@ -11,6 +11,7 @@
>    * published by the Free Software Foundation.
>    */
>   
> +#include <linux/clk.h>
>   #include <linux/module.h>
>   #include <linux/moduleparam.h>
>   #include <linux/init.h>
> @@ -49,6 +50,7 @@ static const char *wm8904_supply_names[WM8904_NUM_SUPPLIES] = {
>   /* codec private data */
>   struct wm8904_priv {
>   	struct regmap *regmap;
> +	struct clk *mclk;
>   
>   	enum wm8904_type devtype;
>   
> @@ -1828,6 +1830,9 @@ static int wm8904_set_bias_level(struct snd_soc_codec *codec,
>   
>   	switch (level) {
>   	case SND_SOC_BIAS_ON:
> +		if (IS_ENABLED(CONFIG_COMMON_CLK))
> +			if (wm8904->mclk)
> +				clk_prepare_enable(wm8904->mclk);
>   		break;
>   
>   	case SND_SOC_BIAS_PREPARE:
> @@ -1894,6 +1899,9 @@ static int wm8904_set_bias_level(struct snd_soc_codec *codec,
>   
>   		regulator_bulk_disable(ARRAY_SIZE(wm8904->supplies),
>   				       wm8904->supplies);
> +		if (IS_ENABLED(CONFIG_COMMON_CLK))
> +			if (wm8904->mclk)
> +				clk_disable_unprepare(wm8904->mclk);
>   		break;
>   	}
>   	codec->dapm.bias_level = level;
> @@ -2139,6 +2147,15 @@ static int wm8904_i2c_probe(struct i2c_client *i2c,
>   		return ret;
>   	}
>   
> +	if (IS_ENABLED(CONFIG_COMMON_CLK)) {
> +		wm8904->mclk = devm_clk_get(&i2c->dev, "mclk");
> +		if (IS_ERR(wm8904->mclk)) {
> +			dev_err(&i2c->dev, "Failed to get MCLK\n");
> +			ret = PTR_ERR(wm8904->mclk);
> +			goto err_enable;
> +		}
> +	}
> +
>   	ret = regmap_read(wm8904->regmap, WM8904_SW_RESET_AND_ID, &val);
>   	if (ret < 0) {
>   		dev_err(&i2c->dev, "Failed to read ID register: %d\n", ret);
Luis Alberto Sanchez March 25, 2014, 12:09 p.m. UTC | #3
El mar 25, 2014 7:59 AM, "Boris BREZILLON" <b.brezillon.dev@gmail.com>
escribió:

> Hello Bo,
>
> Le 25/03/2014 10:56, Bo Shen a écrit :
>
>> Enable WM8904 to support common clock framework.
>>    - Now it supports use SoC provided clock or external oscillator
>>      as MCLK.
>>
>> Signed-off-by: Bo Shen <voice.shen@atmel.com>
>> ---
>>   Documentation/devicetree/bindings/sound/wm8904.txt | 48
>> ++++++++++++++++++++++
>>   sound/soc/codecs/wm8904.c                          | 17 ++++++++
>>   2 files changed, 65 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/sound/wm8904.txt
>>
>> diff --git a/Documentation/devicetree/bindings/sound/wm8904.txt
>> b/Documentation/devicetree/bindings/sound/wm8904.txt
>> new file mode 100644
>> index 0000000..bcf4aa0
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/sound/wm8904.txt
>> @@ -0,0 +1,48 @@
>> +WM8904 audio CODEC
>> +
>> +This device supports I2C only.
>> +
>> +Required properties:
>> +  - compatible: "wlf,wm8904"
>> +  - reg: the I2C address of the device.
>> +  - clocks and clock-names: reference to
>> +    <Documentation/devicetree/bindings/clock/clock-bindings.txt>
>> +
>> +Pins on the device (for linking into audio routes):
>> +
>> +  * IN1L
>> +  * IN1R
>> +  * IN2L
>> +  * IN2R
>> +  * IN3L
>> +  * IN3R
>> +  * HPOUTL
>> +  * HPOUTR
>> +  * LINEOUTL
>> +  * LINEOUTR
>> +  * MICBIAS
>> +
>> +Examples:
>> +
>> +1. Using SoC provided clock as mclk:
>> +codec: wm8904@1a {
>> +       compatible = "wlf,wm8904";
>> +       reg = <0x1a>;
>> +       clocks = <&pck0>;
>> +       clock-names = "mclk";
>> +};
>>
>
>
>  +
>> +2. Using external oscillator as mclk:
>> +codec: wm8904@1a {
>> +       compatible = "wlf,wm8904";
>> +       reg = <0x1a>;
>> +
>> +       wm8904_osc {
>> +               compatible = "fixed-clock";
>> +               #clock-cells = <0>;
>> +               #clock-frequency = <12000000>;
>> +       }
>> +
>> +       clocks = <&wm8904_osc>;
>> +       clock-names = "mclk";
>> +};
>>
>
> I think there is a misunderstanding here (and it certainly comes from my
> explanations during our last talk :)): both cases should be handled the
> same
> way because an external oscillator is no more or less than an external clk
> (we don't care how this clk is generated).
>
> I suggested to put the osc node inside the wm8904 one but I thought you
> were
> trying to represent the internal 12MHz clk generated when using the fll
> free
> running mode.
>
>  diff --git a/sound/soc/codecs/wm8904.c b/sound/soc/codecs/wm8904.c
>> index 49c35c3..4db6a26 100644
>> --- a/sound/soc/codecs/wm8904.c
>> +++ b/sound/soc/codecs/wm8904.c
>> @@ -11,6 +11,7 @@
>>    * published by the Free Software Foundation.
>>    */
>>   +#include <linux/clk.h>
>>   #include <linux/module.h>
>>   #include <linux/moduleparam.h>
>>   #include <linux/init.h>
>> @@ -49,6 +50,7 @@ static const char *wm8904_supply_names[WM8904_NUM_SUPPLIES]
>> = {
>>   /* codec private data */
>>   struct wm8904_priv {
>>         struct regmap *regmap;
>> +       struct clk *mclk;
>>         enum wm8904_type devtype;
>>   @@ -1828,6 +1830,9 @@ static int wm8904_set_bias_level(struct
>> snd_soc_codec *codec,
>>         switch (level) {
>>         case SND_SOC_BIAS_ON:
>> +               if (IS_ENABLED(CONFIG_COMMON_CLK))
>> +                       if (wm8904->mclk)
>> +                               clk_prepare_enable(wm8904->mclk);
>>                 break;
>>         case SND_SOC_BIAS_PREPARE:
>> @@ -1894,6 +1899,9 @@ static int wm8904_set_bias_level(struct
>> snd_soc_codec *codec,
>>                 regulator_bulk_disable(ARRAY_SIZE(wm8904->supplies),
>>                                        wm8904->supplies);
>> +               if (IS_ENABLED(CONFIG_COMMON_CLK))
>> +                       if (wm8904->mclk)
>> +                               clk_disable_unprepare(wm8904->mclk);
>>                 break;
>>         }
>>         codec->dapm.bias_level = level;
>> @@ -2139,6 +2147,15 @@ static int wm8904_i2c_probe(struct i2c_client *i2c,
>>                 return ret;
>>         }
>>   +     if (IS_ENABLED(CONFIG_COMMON_CLK)) {
>> +               wm8904->mclk = devm_clk_get(&i2c->dev, "mclk");
>> +               if (IS_ERR(wm8904->mclk)) {
>> +                       dev_err(&i2c->dev, "Failed to get MCLK\n");
>> +                       ret = PTR_ERR(wm8904->mclk);
>> +                       goto err_enable;
>> +               }
>> +       }
>> +
>>         ret = regmap_read(wm8904->regmap, WM8904_SW_RESET_AND_ID, &val);
>>         if (ret < 0) {
>>                 dev_err(&i2c->dev, "Failed to read ID register: %d\n",
>> ret);
>>
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
Bo Shen March 26, 2014, 1:32 a.m. UTC | #4
Hi Mark,

On 03/25/2014 07:27 PM, Mark Brown wrote:
> On Tue, Mar 25, 2014 at 05:56:56PM +0800, Bo Shen wrote:
>
>>   	case SND_SOC_BIAS_ON:
>> +		if (IS_ENABLED(CONFIG_COMMON_CLK))
>> +			if (wm8904->mclk)
>> +				clk_prepare_enable(wm8904->mclk);
>
> No, this shouldn't depend on COMMON_CLK - there is no reason other clock
> API implementations shouldn't be able to use this.  Providing a clock is
> something you can only do with COMMON_CLK but using one doesn't need
> that, generally using a clock is done unconditionally.

Actually in probe function, it does this check, if not CCF support, the 
wm8904->mclk is NULL. So, we can remove the CCF check here.

Thanks.

Best Regards,
Bo Shen

Patch
diff mbox

diff --git a/Documentation/devicetree/bindings/sound/wm8904.txt b/Documentation/devicetree/bindings/sound/wm8904.txt
new file mode 100644
index 0000000..bcf4aa0
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/wm8904.txt
@@ -0,0 +1,48 @@ 
+WM8904 audio CODEC
+
+This device supports I2C only.
+
+Required properties:
+  - compatible: "wlf,wm8904"
+  - reg: the I2C address of the device.
+  - clocks and clock-names: reference to
+    <Documentation/devicetree/bindings/clock/clock-bindings.txt>
+
+Pins on the device (for linking into audio routes):
+
+  * IN1L
+  * IN1R
+  * IN2L
+  * IN2R
+  * IN3L
+  * IN3R
+  * HPOUTL
+  * HPOUTR
+  * LINEOUTL
+  * LINEOUTR
+  * MICBIAS
+
+Examples:
+
+1. Using SoC provided clock as mclk:
+codec: wm8904@1a {
+	compatible = "wlf,wm8904";
+	reg = <0x1a>;
+	clocks = <&pck0>;
+	clock-names = "mclk";
+};
+
+2. Using external oscillator as mclk:
+codec: wm8904@1a {
+	compatible = "wlf,wm8904";
+	reg = <0x1a>;
+
+	wm8904_osc {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		#clock-frequency = <12000000>;
+	}
+
+	clocks = <&wm8904_osc>;
+	clock-names = "mclk";
+};
diff --git a/sound/soc/codecs/wm8904.c b/sound/soc/codecs/wm8904.c
index 49c35c3..4db6a26 100644
--- a/sound/soc/codecs/wm8904.c
+++ b/sound/soc/codecs/wm8904.c
@@ -11,6 +11,7 @@ 
  * published by the Free Software Foundation.
  */
 
+#include <linux/clk.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/init.h>
@@ -49,6 +50,7 @@  static const char *wm8904_supply_names[WM8904_NUM_SUPPLIES] = {
 /* codec private data */
 struct wm8904_priv {
 	struct regmap *regmap;
+	struct clk *mclk;
 
 	enum wm8904_type devtype;
 
@@ -1828,6 +1830,9 @@  static int wm8904_set_bias_level(struct snd_soc_codec *codec,
 
 	switch (level) {
 	case SND_SOC_BIAS_ON:
+		if (IS_ENABLED(CONFIG_COMMON_CLK))
+			if (wm8904->mclk)
+				clk_prepare_enable(wm8904->mclk);
 		break;
 
 	case SND_SOC_BIAS_PREPARE:
@@ -1894,6 +1899,9 @@  static int wm8904_set_bias_level(struct snd_soc_codec *codec,
 
 		regulator_bulk_disable(ARRAY_SIZE(wm8904->supplies),
 				       wm8904->supplies);
+		if (IS_ENABLED(CONFIG_COMMON_CLK))
+			if (wm8904->mclk)
+				clk_disable_unprepare(wm8904->mclk);
 		break;
 	}
 	codec->dapm.bias_level = level;
@@ -2139,6 +2147,15 @@  static int wm8904_i2c_probe(struct i2c_client *i2c,
 		return ret;
 	}
 
+	if (IS_ENABLED(CONFIG_COMMON_CLK)) {
+		wm8904->mclk = devm_clk_get(&i2c->dev, "mclk");
+		if (IS_ERR(wm8904->mclk)) {
+			dev_err(&i2c->dev, "Failed to get MCLK\n");
+			ret = PTR_ERR(wm8904->mclk);
+			goto err_enable;
+		}
+	}
+
 	ret = regmap_read(wm8904->regmap, WM8904_SW_RESET_AND_ID, &val);
 	if (ret < 0) {
 		dev_err(&i2c->dev, "Failed to read ID register: %d\n", ret);