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

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

Commit Message

Bo Shen March 21, 2014, 2:51 a.m. UTC
Enable WM8904 to support common clock framework.
And also supports different clk configuration.

Signed-off-by: Bo Shen <voice.shen@atmel.com>
---

 Documentation/devicetree/bindings/sound/wm8904.txt | 57 ++++++++++++++++++++++
 sound/soc/codecs/wm8904.c                          | 39 +++++++++++++++
 2 files changed, 96 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/wm8904.txt

Comments

Mark Rutland March 21, 2014, 10:37 a.m. UTC | #1
On Fri, Mar 21, 2014 at 02:51:02AM +0000, Bo Shen wrote:
> Enable WM8904 to support common clock framework.
> And also supports different clk configuration.
> 
> Signed-off-by: Bo Shen <voice.shen@atmel.com>
> ---
> 
>  Documentation/devicetree/bindings/sound/wm8904.txt | 57 ++++++++++++++++++++++
>  sound/soc/codecs/wm8904.c                          | 39 +++++++++++++++
>  2 files changed, 96 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..039374d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/wm8904.txt
> @@ -0,0 +1,57 @@
> +WM8904 audio CODEC
> +
> +This device supports I2C only.
> +
> +Required properties:
> +  - compatible: "wlf,wm8904"
> +  - reg: the I2C address of the device.
> +
> +  if work with CCF, the following properties as required:

The DT shouldn't care about the software stack. You don't need to refer
to the CCF, just the clock bindings.

> +  - wlf,sysclk-from-mclk: set the sys clock is driven from mclk,

Why can the kernel not decide this?

> +    - wlf,mclk-use-xtal: if the mclk is generated by crystal.
> +      if without this property, the mclk is generated from SOC.

Huh? What exact property do you actually are about here?

> +      - clocks: which clock generated clock to mclk.
> +      - clock-names: the clock names use for retrieve.

NO. Is you wish to use clock-names, then define the set of names you
expect; clock-names are the names of the _input_ lines. The
documentation sets this out clearly. Take a look at
Documentation/devicetree/bindings/clock/clock-bindings.txt.

> +    - wlf,mclk-freq: mclk's frequency

If you expect mclk, you should be able to query this from it. You don't
need a separate property.

Unless this is a frequency to set it to? If so, why can the kernel not
choose this?

Chers,
Mark.
Mark Brown March 21, 2014, 11:55 a.m. UTC | #2
On Fri, Mar 21, 2014 at 10:37:47AM +0000, Mark Rutland wrote:
> On Fri, Mar 21, 2014 at 02:51:02AM +0000, Bo Shen wrote:

> > +  - wlf,sysclk-from-mclk: set the sys clock is driven from mclk,

> Why can the kernel not decide this?

It can.

> > +    - wlf,mclk-use-xtal: if the mclk is generated by crystal.
> > +      if without this property, the mclk is generated from SOC.

> Huh? What exact property do you actually are about here?

This should just be omitted - based on the previous posting it's saying
if this is a fixed or variable rate clock.

> > +    - wlf,mclk-freq: mclk's frequency

> If you expect mclk, you should be able to query this from it. You don't
> need a separate property.

> Unless this is a frequency to set it to? If so, why can the kernel not
> choose this?

Yes, quite - and even if it needs to be set explicitly the clock API
generic bindings should be able to support this (I *think* that is due
to go in during the next merge window but iddn't check yet).
Bo Shen March 24, 2014, 2:15 a.m. UTC | #3
Hi Mark,

On 03/21/2014 07:55 PM, Mark Brown wrote:
> On Fri, Mar 21, 2014 at 10:37:47AM +0000, Mark Rutland wrote:
>> On Fri, Mar 21, 2014 at 02:51:02AM +0000, Bo Shen wrote:
>
>>> +  - wlf,sysclk-from-mclk: set the sys clock is driven from mclk,
>
>> Why can the kernel not decide this?
>
> It can.

I really don't know how it decided by kernel (hardcode in machine driver 
?), can you point me directly? Thanks.

>>> +    - wlf,mclk-use-xtal: if the mclk is generated by crystal.
>>> +      if without this property, the mclk is generated from SOC.
>
>> Huh? What exact property do you actually are about here?
>
> This should just be omitted - based on the previous posting it's saying
> if this is a fixed or variable rate clock.
>
>>> +    - wlf,mclk-freq: mclk's frequency
>
>> If you expect mclk, you should be able to query this from it. You don't
>> need a separate property.
>
>> Unless this is a frequency to set it to? If so, why can the kernel not
>> choose this?
>
> Yes, quite - and even if it needs to be set explicitly the clock API
> generic bindings should be able to support this (I *think* that is due
> to go in during the next merge window but iddn't check yet).
>

I will check it. If some hints about this, will be appreciated.
Thanks.

Best Regards,
Bo Shen
Boris BREZILLON March 24, 2014, 9:44 a.m. UTC | #4
Le 21/03/2014 12:55, Mark Brown a écrit :
> On Fri, Mar 21, 2014 at 10:37:47AM +0000, Mark Rutland wrote:
>> On Fri, Mar 21, 2014 at 02:51:02AM +0000, Bo Shen wrote:
>>> +  - wlf,sysclk-from-mclk: set the sys clock is driven from mclk,
>> Why can the kernel not decide this?
> It can.
>
>>> +    - wlf,mclk-use-xtal: if the mclk is generated by crystal.
>>> +      if without this property, the mclk is generated from SOC.
>> Huh? What exact property do you actually are about here?
> This should just be omitted - based on the previous posting it's saying
> if this is a fixed or variable rate clock.
>
>>> +    - wlf,mclk-freq: mclk's frequency
>> If you expect mclk, you should be able to query this from it. You don't
>> need a separate property.
>> Unless this is a frequency to set it to? If so, why can the kernel not
>> choose this?
> Yes, quite - and even if it needs to be set explicitly the clock API
> generic bindings should be able to support this (I *think* that is due
> to go in during the next merge window but iddn't check yet).
I guess you're talking about this: https://lkml.org/lkml/2014/3/3/324.
AFAIK this has not been accepted yet, but this is definitely the way to
go if we need to specify a default-rate from the DT.

On the whole subject, I'd like to have your opinion on how we should
implement clk handling within the wm8904 driver.

 From my point of view we have 2 choices:

1) Expose the wm8904 clock tree using the CCF (and its DT bindings)

 From what I read in the datasheet this would give the following DT
definition:

wm8904@xx {
     compatible="wm8904";
     [...]

     clocks {
         fll {
             compatible = "wolfson,wm8904-fll"
             #clock-cells = <0>;
             clocks = <&pck0>;
             clock-output-names = "wm8904-fll";
         };

         sysclk {
             compatible = "wolfson,wm8904-sysclk"
             clocks = <&pck0 &fll>;
             clock-output-names = "wm8904-sysclk";
         };

         /* other clk derived from sysclk */
     };
};

If we use the CCF in the wm8904 driver, we'll have to move all users
of this chip to the CCF too (in other terms, we need to move the sam9n12
SoC and boards to the CCF).


2) keep the driver as it is except for the mclk retrieval.

wm8904@xx {
     compatible="wm8904";
     clocks = <&pck0>;
     clock-names = "mclk";
     clock-rates = <32768>;
};


The first solution is obviously more complicated to implement (and 
requires the
CCF), but in the other hand, it gives fine-grained control over clk 
configuration
and use a standard way to expose/manipulate clks.

Best Regards,

Boris
Mark Brown March 24, 2014, 11:06 a.m. UTC | #5
On Mon, Mar 24, 2014 at 10:15:51AM +0800, Bo Shen wrote:
> On 03/21/2014 07:55 PM, Mark Brown wrote:
> >On Fri, Mar 21, 2014 at 10:37:47AM +0000, Mark Rutland wrote:
> >>On Fri, Mar 21, 2014 at 02:51:02AM +0000, Bo Shen wrote:

> >>>+  - wlf,sysclk-from-mclk: set the sys clock is driven from mclk,

> >>Why can the kernel not decide this?

> >It can.

> I really don't know how it decided by kernel (hardcode in machine driver ?),
> can you point me directly? Thanks.

It can just look to see if there is a suitable system clock present,
this shouldn't be hard.  If there's a clock connected at a suitable
rate it should use that, otherwise if there's a clock at an unsuitable
rate it should either change the rate of the clock or use the FLL.
Mark Brown March 24, 2014, 11:07 a.m. UTC | #6
On Mon, Mar 24, 2014 at 10:44:27AM +0100, Boris BREZILLON wrote:

> 1) Expose the wm8904 clock tree using the CCF (and its DT bindings)

We can't use the common clock framework in generic drivers since the
common clock framework isn't generally available.

>     clocks {
>         fll {
>             compatible = "wolfson,wm8904-fll"

If we're adding compatible strings for non-reusable subcomponents of a
device like this we're doing something seriously broken.
Bo Shen March 25, 2014, 8:01 a.m. UTC | #7
Hi Mark,

On 03/24/2014 07:06 PM, Mark Brown wrote:
> On Mon, Mar 24, 2014 at 10:15:51AM +0800, Bo Shen wrote:
>> On 03/21/2014 07:55 PM, Mark Brown wrote:
>>> On Fri, Mar 21, 2014 at 10:37:47AM +0000, Mark Rutland wrote:
>>>> On Fri, Mar 21, 2014 at 02:51:02AM +0000, Bo Shen wrote:
>
>>>>> +  - wlf,sysclk-from-mclk: set the sys clock is driven from mclk,
>
>>>> Why can the kernel not decide this?
>
>>> It can.
>
>> I really don't know how it decided by kernel (hardcode in machine driver ?),
>> can you point me directly? Thanks.
>
> It can just look to see if there is a suitable system clock present,
> this shouldn't be hard.  If there's a clock connected at a suitable
> rate it should use that, otherwise if there's a clock at an unsuitable
> rate it should either change the rate of the clock or use the FLL.

I try to implement this, I think we can write the wm8904 dt as simple as 
following, can it be acceptable?

Case 1: using SoC provided clock
wm8904:wm8904@1a {
	compatible = "wlf,wm8904";
	reg = <0x1a>;
	clocks = <&pck0>;
	clock-name = "mclk";
}

Case 2: using external crystal (For this case, I have no idea to put to 
CCF, so need "wlf,xtal-clk-freq" property).
wm8904:wm8904@1a {
	compatible = "wlf,wm8904";
	reg = <0x1a>;
	wlf,xtal-clk-freq = <12000000>;
}

Then we use these clocks to configure FLL.

Btw, I see the "wm2000.c" retrieve the clock, however not see the usage 
cases for it.

Best Regards,
Bo Shen
Bo Shen March 25, 2014, 8:19 a.m. UTC | #8
Hi Mark,

On 03/25/2014 04:01 PM, Bo Shen wrote:
> Hi Mark,
>
> On 03/24/2014 07:06 PM, Mark Brown wrote:
>> On Mon, Mar 24, 2014 at 10:15:51AM +0800, Bo Shen wrote:
>>> On 03/21/2014 07:55 PM, Mark Brown wrote:
>>>> On Fri, Mar 21, 2014 at 10:37:47AM +0000, Mark Rutland wrote:
>>>>> On Fri, Mar 21, 2014 at 02:51:02AM +0000, Bo Shen wrote:
>>
>>>>>> +  - wlf,sysclk-from-mclk: set the sys clock is driven from mclk,
>>
>>>>> Why can the kernel not decide this?
>>
>>>> It can.
>>
>>> I really don't know how it decided by kernel (hardcode in machine
>>> driver ?),
>>> can you point me directly? Thanks.
>>
>> It can just look to see if there is a suitable system clock present,
>> this shouldn't be hard.  If there's a clock connected at a suitable
>> rate it should use that, otherwise if there's a clock at an unsuitable
>> rate it should either change the rate of the clock or use the FLL.
>
> I try to implement this, I think we can write the wm8904 dt as simple as
> following, can it be acceptable?
>
> Case 1: using SoC provided clock
> wm8904:wm8904@1a {
>      compatible = "wlf,wm8904";
>      reg = <0x1a>;
>      clocks = <&pck0>;
>      clock-name = "mclk";
> }
>
> Case 2: using external crystal (For this case, I have no idea to put to
> CCF, so need "wlf,xtal-clk-freq" property).
> wm8904:wm8904@1a {
>      compatible = "wlf,wm8904";
>      reg = <0x1a>;
>      wlf,xtal-clk-freq = <12000000>;
> }

Please forget this, with Boris's help, we can use clk-fixed-rate.c driver.

So, I will try to implement another RFC patch.

> Then we use these clocks to configure FLL.
>
> Btw, I see the "wm2000.c" retrieve the clock, however not see the usage
> cases for it.
>
> Best Regards,
> Bo Shen
>
>

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..039374d
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/wm8904.txt
@@ -0,0 +1,57 @@ 
+WM8904 audio CODEC
+
+This device supports I2C only.
+
+Required properties:
+  - compatible: "wlf,wm8904"
+  - reg: the I2C address of the device.
+
+  if work with CCF, the following properties as required:
+  - wlf,sysclk-from-mclk: set the sys clock is driven from mclk,
+    - wlf,mclk-use-xtal: if the mclk is generated by crystal.
+      if without this property, the mclk is generated from SOC.
+      - clocks: which clock generated clock to mclk.
+      - clock-names: the clock names use for retrieve.
+    - wlf,mclk-freq: mclk's frequency
+
+
+Pins on the device (for linking into audio routes):
+
+  * IN1L
+  * IN1R
+  * IN2L
+  * IN2R
+  * IN3L
+  * IN3R
+  * HPOUTL
+  * HPOUTR
+  * LINEOUTL
+  * LINEOUTR
+  * MICBIAS
+
+Examples:
+
+1. General usage:
+codec: wm8904@1a {
+	compatible = "wlf,wm8904";
+	reg = <0x1a>;
+};
+
+2. Working with CCF supports and using crystall provide mclk:
+codec: wm8904@1a {
+	compatible = "wlf,wm8904";
+	reg = <0x1a>;
+	wlf,sysclk-from-mclk;
+	wlf,mclk-use-xtal;
+	wlf,mclk-freq = <12000000>;
+};
+
+3. Working with CCF supports and using SoC provide mclk:
+codec: wm8904@1a {
+	compatible = "wlf,wm8904";
+	reg = <0x1a>;
+	wlf,sysclk-from-mclk;
+	clocks = <&pck0>;
+	clock-names = "mclk";
+	wlf,mclk-freq = <32768>;
+};
diff --git a/sound/soc/codecs/wm8904.c b/sound/soc/codecs/wm8904.c
index 49c35c3..99e3e90 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,10 @@  static const char *wm8904_supply_names[WM8904_NUM_SUPPLIES] = {
 /* codec private data */
 struct wm8904_priv {
 	struct regmap *regmap;
+	struct clk *mclk;
+	bool sysclk_from_mclk;
+	bool mclk_use_xtal;
+	u32 mclk_freq;
 
 	enum wm8904_type devtype;
 
@@ -1828,6 +1833,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_use_xtal)
+				clk_prepare_enable(wm8904->mclk);
 		break;
 
 	case SND_SOC_BIAS_PREPARE:
@@ -1894,6 +1902,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_use_xtal)
+				clk_disable_unprepare(wm8904->mclk);
 		break;
 	}
 	codec->dapm.bias_level = level;
@@ -2139,6 +2150,34 @@  static int wm8904_i2c_probe(struct i2c_client *i2c,
 		return ret;
 	}
 
+	if (IS_ENABLED(CONFIG_COMMON_CLK)) {
+		struct device_node *np = i2c->dev.of_node;
+		wm8904->sysclk_from_mclk =
+			of_property_read_bool(np, "wlf,sysclk-from-mclk");
+		if (wm8904->sysclk_from_mclk) {
+			wm8904->mclk_use_xtal =
+				of_property_read_bool(np, "wlf,mclk-use-xtal");
+
+			ret = of_property_read_u32(np, "wlf,mclk-freq",
+					&wm8904->mclk_freq);
+			if (ret) {
+				dev_err(&i2c->dev, "Failed to read mclk freq");
+				goto err_enable;
+			}
+
+			if (!wm8904->mclk_use_xtal) {
+				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;
+				}
+
+				clk_set_rate(wm8904->mclk, wm8904->mclk_freq);
+			}
+		}
+	}
+
 	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);