| Message ID | 1395370262-23305-1-git-send-email-voice.shen@atmel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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).
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
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
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.
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.
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
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
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);
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