| Message ID | 1456190782-18865-1-git-send-email-sugar.zhang@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2/22/16 7:26 PM, Sugar Zhang wrote: > enable/disable master clock when codec is active or not. > > Signed-off-by: Sugar Zhang <sugar.zhang@rock-chips.com> > --- > > Documentation/devicetree/bindings/sound/rt5640.txt | 3 +++ > sound/soc/codecs/rt5640.c | 31 ++++++++++++++++++++++ > sound/soc/codecs/rt5640.h | 2 ++ > 3 files changed, 36 insertions(+) > > diff --git a/Documentation/devicetree/bindings/sound/rt5640.txt b/Documentation/devicetree/bindings/sound/rt5640.txt > index 9e62f6e..57fe646 100644 > --- a/Documentation/devicetree/bindings/sound/rt5640.txt > +++ b/Documentation/devicetree/bindings/sound/rt5640.txt > @@ -12,6 +12,9 @@ Required properties: > > Optional properties: > > +- clocks: The phandle of the master clock to the CODEC > +- clock-names: Should be "mclk" This patch assumes that the information on mclk comes from DeviceTree. The mclk may also be enabled/disabled in the machine driver with an explicit transition to use an internal clock when the codec is not used. I hope this patch doesn't preclude such usages or there will be a conflict with the patches we are about to upstream for Baytrail/cht devices. > + > - realtek,in1-differential > - realtek,in2-differential > - realtek,in3-differential > diff --git a/sound/soc/codecs/rt5640.c b/sound/soc/codecs/rt5640.c > index 11d032c..6cd84fb 100644 > --- a/sound/soc/codecs/rt5640.c > +++ b/sound/soc/codecs/rt5640.c > @@ -1949,7 +1949,33 @@ static int rt5640_set_dai_pll(struct snd_soc_dai *dai, int pll_id, int source, > static int rt5640_set_bias_level(struct snd_soc_codec *codec, > enum snd_soc_bias_level level) > { > + struct rt5640_priv *rt5640 = snd_soc_codec_get_drvdata(codec); > + int ret; > + > switch (level) { > + case SND_SOC_BIAS_ON: > + break; > + > + case SND_SOC_BIAS_PREPARE: > + /* > + * SND_SOC_BIAS_PREPARE is called while preparing for a > + * transition to ON or away from ON. If current bias_level > + * is SND_SOC_BIAS_ON, then it is preparing for a transition > + * away from ON. Disable the clock in that case, otherwise > + * enable it. > + */ > + if (IS_ERR(rt5640->mclk)) > + break; > + > + if (snd_soc_codec_get_bias_level(codec) == SND_SOC_BIAS_ON) { > + clk_disable_unprepare(rt5640->mclk); > + } else { > + ret = clk_prepare_enable(rt5640->mclk); > + if (ret) > + return ret; > + } > + break; > + > case SND_SOC_BIAS_STANDBY: > if (SND_SOC_BIAS_OFF == snd_soc_codec_get_bias_level(codec)) { > snd_soc_update_bits(codec, RT5640_PWR_ANLG1, > @@ -2088,6 +2114,11 @@ static int rt5640_probe(struct snd_soc_codec *codec) > struct snd_soc_dapm_context *dapm = snd_soc_codec_get_dapm(codec); > struct rt5640_priv *rt5640 = snd_soc_codec_get_drvdata(codec); > > + /* Check if MCLK provided */ > + rt5640->mclk = devm_clk_get(codec->dev, "mclk"); > + if (PTR_ERR(rt5640->mclk) == -EPROBE_DEFER) > + return -EPROBE_DEFER; > + > rt5640->codec = codec; > > snd_soc_codec_force_bias_level(codec, SND_SOC_BIAS_OFF); > diff --git a/sound/soc/codecs/rt5640.h b/sound/soc/codecs/rt5640.h > index 83a7150..1761c3a9 100644 > --- a/sound/soc/codecs/rt5640.h > +++ b/sound/soc/codecs/rt5640.h > @@ -12,6 +12,7 @@ > #ifndef _RT5640_H > #define _RT5640_H > > +#include <linux/clk.h> > #include <sound/rt5640.h> > > /* Info */ > @@ -2097,6 +2098,7 @@ struct rt5640_priv { > struct snd_soc_codec *codec; > struct rt5640_platform_data pdata; > struct regmap *regmap; > + struct clk *mclk; > > int sysclk; > int sysclk_src; >
On Tue, Feb 23, 2016 at 09:26:22AM +0800, Sugar Zhang wrote: > enable/disable master clock when codec is active or not. > > Signed-off-by: Sugar Zhang <sugar.zhang@rock-chips.com> > --- > > Documentation/devicetree/bindings/sound/rt5640.txt | 3 +++ Acked-by: Rob Herring <robh@kernel.org> > sound/soc/codecs/rt5640.c | 31 ++++++++++++++++++++++ > sound/soc/codecs/rt5640.h | 2 ++ > 3 files changed, 36 insertions(+) >
Am Dienstag, 23. Februar 2016, 08:50:04 schrieb Pierre-Louis Bossart: > On 2/22/16 7:26 PM, Sugar Zhang wrote: > > enable/disable master clock when codec is active or not. > > > > Signed-off-by: Sugar Zhang <sugar.zhang@rock-chips.com> > > --- > > > > Documentation/devicetree/bindings/sound/rt5640.txt | 3 +++ > > sound/soc/codecs/rt5640.c | 31 > > ++++++++++++++++++++++ sound/soc/codecs/rt5640.h > > | 2 ++ > > 3 files changed, 36 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/sound/rt5640.txt > > b/Documentation/devicetree/bindings/sound/rt5640.txt index > > 9e62f6e..57fe646 100644 > > --- a/Documentation/devicetree/bindings/sound/rt5640.txt > > +++ b/Documentation/devicetree/bindings/sound/rt5640.txt > > > > @@ -12,6 +12,9 @@ Required properties: > > Optional properties: > > +- clocks: The phandle of the master clock to the CODEC > > +- clock-names: Should be "mclk" > > This patch assumes that the information on mclk comes from DeviceTree. > The mclk may also be enabled/disabled in the machine driver with an > explicit transition to use an internal clock when the codec is not used. > I hope this patch doesn't preclude such usages or there will be a > conflict with the patches we are about to upstream for Baytrail/cht > devices. As you can see, the clock property itself is optional and the clk_get below acts accordingly of also continuing if the clock is not present. So it won't affect any users doing it otherwise. Heiko > > + > > > > - realtek,in1-differential > > - realtek,in2-differential > > - realtek,in3-differential > > > > diff --git a/sound/soc/codecs/rt5640.c b/sound/soc/codecs/rt5640.c > > index 11d032c..6cd84fb 100644 > > --- a/sound/soc/codecs/rt5640.c > > +++ b/sound/soc/codecs/rt5640.c > > @@ -1949,7 +1949,33 @@ static int rt5640_set_dai_pll(struct snd_soc_dai > > *dai, int pll_id, int source,> > > static int rt5640_set_bias_level(struct snd_soc_codec *codec, > > > > enum snd_soc_bias_level level) > > > > { > > > > + struct rt5640_priv *rt5640 = snd_soc_codec_get_drvdata(codec); > > + int ret; > > + > > > > switch (level) { > > > > + case SND_SOC_BIAS_ON: > > + break; > > + > > + case SND_SOC_BIAS_PREPARE: > > + /* > > + * SND_SOC_BIAS_PREPARE is called while preparing for a > > + * transition to ON or away from ON. If current bias_level > > + * is SND_SOC_BIAS_ON, then it is preparing for a transition > > + * away from ON. Disable the clock in that case, otherwise > > + * enable it. > > + */ > > + if (IS_ERR(rt5640->mclk)) > > + break; > > + > > + if (snd_soc_codec_get_bias_level(codec) == SND_SOC_BIAS_ON) { > > + clk_disable_unprepare(rt5640->mclk); > > + } else { > > + ret = clk_prepare_enable(rt5640->mclk); > > + if (ret) > > + return ret; > > + } > > + break; > > + > > > > case SND_SOC_BIAS_STANDBY: > > if (SND_SOC_BIAS_OFF == snd_soc_codec_get_bias_level(codec)) { > > > > snd_soc_update_bits(codec, RT5640_PWR_ANLG1, > > > > @@ -2088,6 +2114,11 @@ static int rt5640_probe(struct snd_soc_codec > > *codec)> > > struct snd_soc_dapm_context *dapm = snd_soc_codec_get_dapm(codec); > > struct rt5640_priv *rt5640 = snd_soc_codec_get_drvdata(codec); > > > > + /* Check if MCLK provided */ > > + rt5640->mclk = devm_clk_get(codec->dev, "mclk"); > > + if (PTR_ERR(rt5640->mclk) == -EPROBE_DEFER) > > + return -EPROBE_DEFER; > > + > > > > rt5640->codec = codec; > > > > snd_soc_codec_force_bias_level(codec, SND_SOC_BIAS_OFF); > > > > diff --git a/sound/soc/codecs/rt5640.h b/sound/soc/codecs/rt5640.h > > index 83a7150..1761c3a9 100644 > > --- a/sound/soc/codecs/rt5640.h > > +++ b/sound/soc/codecs/rt5640.h > > @@ -12,6 +12,7 @@ > > > > #ifndef _RT5640_H > > #define _RT5640_H > > > > +#include <linux/clk.h> > > > > #include <sound/rt5640.h> > > > > /* Info */ > > > > @@ -2097,6 +2098,7 @@ struct rt5640_priv { > > > > struct snd_soc_codec *codec; > > struct rt5640_platform_data pdata; > > struct regmap *regmap; > > > > + struct clk *mclk; > > > > int sysclk; > > int sysclk_src;
On Wed, Feb 24, 2016 at 12:08:31AM +0100, Heiko Stuebner wrote: > Am Dienstag, 23. Februar 2016, 08:50:04 schrieb Pierre-Louis Bossart: > > This patch assumes that the information on mclk comes from DeviceTree. > > The mclk may also be enabled/disabled in the machine driver with an > > explicit transition to use an internal clock when the codec is not used. > > I hope this patch doesn't preclude such usages or there will be a > > conflict with the patches we are about to upstream for Baytrail/cht > > devices. > As you can see, the clock property itself is optional and the clk_get below > acts accordingly of also continuing if the clock is not present. > So it won't affect any users doing it otherwise. That said we really do need x86 to transition to use the clock API in order to integrate with external devices, where the machine driver does manage clocks we want that to move to being done using the clock API rather than custom APIs.
On Tue, Feb 23, 2016 at 09:26:22AM +0800, Sugar Zhang wrote:
> enable/disable master clock when codec is active or not.
This patch has already been applied, please send incremental patches if
any changes are required.
On 2/23/16 9:32 PM, Mark Brown wrote: > On Wed, Feb 24, 2016 at 12:08:31AM +0100, Heiko Stuebner wrote: >> Am Dienstag, 23. Februar 2016, 08:50:04 schrieb Pierre-Louis >> Bossart: > >>> This patch assumes that the information on mclk comes from >>> DeviceTree. The mclk may also be enabled/disabled in the machine >>> driver with an explicit transition to use an internal clock when >>> the codec is not used. I hope this patch doesn't preclude such >>> usages or there will be a conflict with the patches we are about >>> to upstream for Baytrail/cht devices. > >> As you can see, the clock property itself is optional and the >> clk_get below acts accordingly of also continuing if the clock is >> not present. So it won't affect any users doing it otherwise. > > That said we really do need x86 to transition to use the clock API > in order to integrate with external devices, where the machine driver > does manage clocks we want that to move to being done using the clock > API rather than custom APIs. For Baytrail audio we have a single platform clock that can be turned on/off and set to 19.2 MHz or 25 MHz. No other controls are available, no multipliers or complicated dependencies on other clocks, parents or children and no other users for this clock but the audio subsystem. I looked at the clock framework and couldn't figure out how it would simply map the hardware so for now the use of the MCLK is only enabled with an on/off or set-rate(19.2|25) custom API. I am not an expert here so if this clock framework becomes a requirement to upstream code I would appreciate any pointers to do the right thing. I really couldn't find a simple example with 'Put your code here' comments to use this framework.
On Wed, Feb 24, 2016 at 10:10:05AM -0600, Pierre-Louis Bossart wrote: > On 2/23/16 9:32 PM, Mark Brown wrote: > >That said we really do need x86 to transition to use the clock API > >in order to integrate with external devices, where the machine driver > >does manage clocks we want that to move to being done using the clock > >API rather than custom APIs. > For Baytrail audio we have a single platform clock that can be turned > on/off and set to 19.2 MHz or 25 MHz. No other controls are available, > no multipliers or complicated dependencies on other clocks, parents or > children and no other users for this clock but the audio subsystem. > I looked at the clock framework and couldn't figure out how it would simply > map the hardware so for now the use of the MCLK is only enabled with an > on/off or set-rate(19.2|25) custom API. I am not an expert here so if this > clock framework becomes a requirement to upstream code I would appreciate > any pointers to do the right thing. I really couldn't find a simple example > with 'Put your code here' comments to use this framework. There's a lot of helpers like -fixed, -gate and so on - this sounds like you could probably use those helpers (there should be lots of examples in the kernel) or just implement a simple provider (see clk-provider.h) depending on how the control is mapped in. If it's that simple just open coding a provider ought to do the job. Then set up the client linkage with clkdev in some way that makes sense for your platform.
On 2/25/16 7:08 PM, Mark Brown wrote: > On Wed, Feb 24, 2016 at 10:10:05AM -0600, Pierre-Louis Bossart wrote: >> On 2/23/16 9:32 PM, Mark Brown wrote: > >>> That said we really do need x86 to transition to use the clock API >>> in order to integrate with external devices, where the machine driver >>> does manage clocks we want that to move to being done using the clock >>> API rather than custom APIs. > >> For Baytrail audio we have a single platform clock that can be turned >> on/off and set to 19.2 MHz or 25 MHz. No other controls are available, >> no multipliers or complicated dependencies on other clocks, parents or >> children and no other users for this clock but the audio subsystem. >> I looked at the clock framework and couldn't figure out how it would simply >> map the hardware so for now the use of the MCLK is only enabled with an >> on/off or set-rate(19.2|25) custom API. I am not an expert here so if this >> clock framework becomes a requirement to upstream code I would appreciate >> any pointers to do the right thing. I really couldn't find a simple example >> with 'Put your code here' comments to use this framework. > > There's a lot of helpers like -fixed, -gate and so on - this sounds like > you could probably use those helpers (there should be lots of examples > in the kernel) or just implement a simple provider (see clk-provider.h) > depending on how the control is mapped in. If it's that simple just > open coding a provider ought to do the job. Then set up the client > linkage with clkdev in some way that makes sense for your platform. Yes there are documentation and helpers I looked into, but to the best of my limited knowledge not for the gate + dual-rate case. Then you jump to complicated configurations combining multiple base clocks that don't really make sense here. After one frustrating day of trying to wrap 100 lines of code into the clock framework I gave up and kept the simple custom driver. There is a single client for this clock so I don't quite see the benefits/ROI of mapping a simple piece of hardware with a complex model.
On Fri, Feb 26, 2016 at 09:43:15AM -0600, Pierre-Louis Bossart wrote: > Yes there are documentation and helpers I looked into, but to the best of my > limited knowledge not for the gate + dual-rate case. Then you jump to > complicated configurations combining multiple base clocks that don't really > make sense here. After one frustrating day of trying to wrap 100 lines of > code into the clock framework I gave up and kept the simple custom driver. > There is a single client for this clock so I don't quite see the > benefits/ROI of mapping a simple piece of hardware with a complex model. If the helpers aren't fitting your needs then surely just writing a simple driver with set rate and enable callbacks isn't going to be too hard? The wm831x driver was pretty trivial to write and seems to be on a similar order of complexity to this.
diff --git a/Documentation/devicetree/bindings/sound/rt5640.txt b/Documentation/devicetree/bindings/sound/rt5640.txt index 9e62f6e..57fe646 100644 --- a/Documentation/devicetree/bindings/sound/rt5640.txt +++ b/Documentation/devicetree/bindings/sound/rt5640.txt @@ -12,6 +12,9 @@ Required properties: Optional properties: +- clocks: The phandle of the master clock to the CODEC +- clock-names: Should be "mclk" + - realtek,in1-differential - realtek,in2-differential - realtek,in3-differential diff --git a/sound/soc/codecs/rt5640.c b/sound/soc/codecs/rt5640.c index 11d032c..6cd84fb 100644 --- a/sound/soc/codecs/rt5640.c +++ b/sound/soc/codecs/rt5640.c @@ -1949,7 +1949,33 @@ static int rt5640_set_dai_pll(struct snd_soc_dai *dai, int pll_id, int source, static int rt5640_set_bias_level(struct snd_soc_codec *codec, enum snd_soc_bias_level level) { + struct rt5640_priv *rt5640 = snd_soc_codec_get_drvdata(codec); + int ret; + switch (level) { + case SND_SOC_BIAS_ON: + break; + + case SND_SOC_BIAS_PREPARE: + /* + * SND_SOC_BIAS_PREPARE is called while preparing for a + * transition to ON or away from ON. If current bias_level + * is SND_SOC_BIAS_ON, then it is preparing for a transition + * away from ON. Disable the clock in that case, otherwise + * enable it. + */ + if (IS_ERR(rt5640->mclk)) + break; + + if (snd_soc_codec_get_bias_level(codec) == SND_SOC_BIAS_ON) { + clk_disable_unprepare(rt5640->mclk); + } else { + ret = clk_prepare_enable(rt5640->mclk); + if (ret) + return ret; + } + break; + case SND_SOC_BIAS_STANDBY: if (SND_SOC_BIAS_OFF == snd_soc_codec_get_bias_level(codec)) { snd_soc_update_bits(codec, RT5640_PWR_ANLG1, @@ -2088,6 +2114,11 @@ static int rt5640_probe(struct snd_soc_codec *codec) struct snd_soc_dapm_context *dapm = snd_soc_codec_get_dapm(codec); struct rt5640_priv *rt5640 = snd_soc_codec_get_drvdata(codec); + /* Check if MCLK provided */ + rt5640->mclk = devm_clk_get(codec->dev, "mclk"); + if (PTR_ERR(rt5640->mclk) == -EPROBE_DEFER) + return -EPROBE_DEFER; + rt5640->codec = codec; snd_soc_codec_force_bias_level(codec, SND_SOC_BIAS_OFF); diff --git a/sound/soc/codecs/rt5640.h b/sound/soc/codecs/rt5640.h index 83a7150..1761c3a9 100644 --- a/sound/soc/codecs/rt5640.h +++ b/sound/soc/codecs/rt5640.h @@ -12,6 +12,7 @@ #ifndef _RT5640_H #define _RT5640_H +#include <linux/clk.h> #include <sound/rt5640.h> /* Info */ @@ -2097,6 +2098,7 @@ struct rt5640_priv { struct snd_soc_codec *codec; struct rt5640_platform_data pdata; struct regmap *regmap; + struct clk *mclk; int sysclk; int sysclk_src;
enable/disable master clock when codec is active or not. Signed-off-by: Sugar Zhang <sugar.zhang@rock-chips.com> --- Documentation/devicetree/bindings/sound/rt5640.txt | 3 +++ sound/soc/codecs/rt5640.c | 31 ++++++++++++++++++++++ sound/soc/codecs/rt5640.h | 2 ++ 3 files changed, 36 insertions(+)