[alsa-devel] [PATCH v2 1/2] ASoC: wm8960: Let wm8960 codec driver manage its own MCLK
From: Zidan Wang b50113@freescale.com
When we want to use wm8960 codec, we should enable its MCLK in machine driver. It's reasonable for wm8960 codec driver to manage its own MCLK.
When current bias_level is SND_SOC_BIAS_ON, it is preparing for a transition away from ON. In this case, disable the codec mclk. When current bias_level is not SND_SOC_BIAS_ON, it preparing for a transition to ON. In this case, enable the codec mclk.
Signed-off-by: Zidan Wang b50113@freescale.com --- sound/soc/codecs/wm8960.c | 51 ++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 48 insertions(+), 3 deletions(-)
diff --git a/sound/soc/codecs/wm8960.c b/sound/soc/codecs/wm8960.c index 75cd7de..fd25973 100644 --- a/sound/soc/codecs/wm8960.c +++ b/sound/soc/codecs/wm8960.c @@ -15,6 +15,7 @@ #include <linux/init.h> #include <linux/delay.h> #include <linux/pm.h> +#include <linux/clk.h> #include <linux/i2c.h> #include <linux/slab.h> #include <sound/core.h> @@ -117,6 +118,7 @@ static bool wm8960_volatile(struct device *dev, unsigned int reg) }
struct wm8960_priv { + struct clk *mclk; struct regmap *regmap; int (*set_bias_level)(struct snd_soc_codec *, enum snd_soc_bias_level level); @@ -618,14 +620,38 @@ static int wm8960_set_bias_level_out3(struct snd_soc_codec *codec, enum snd_soc_bias_level level) { struct wm8960_priv *wm8960 = snd_soc_codec_get_drvdata(codec); + int ret;
switch (level) { case SND_SOC_BIAS_ON: break;
case SND_SOC_BIAS_PREPARE: - /* Set VMID to 2x50k */ - snd_soc_update_bits(codec, WM8960_POWER1, 0x180, 0x80); + switch (codec->dapm.bias_level) { + case SND_SOC_BIAS_STANDBY: + if (!IS_ERR(wm8960->mclk)) { + ret = clk_prepare_enable(wm8960->mclk); + if (ret) { + dev_err(codec->dev, + "Failed to enable MCLK: %d\n", + ret); + return ret; + } + } + + /* Set VMID to 2x50k */ + snd_soc_update_bits(codec, WM8960_POWER1, 0x180, 0x80); + break; + + case SND_SOC_BIAS_ON: + if (!IS_ERR(wm8960->mclk)) + clk_disable_unprepare(wm8960->mclk); + break; + + default: + break; + } + break;
case SND_SOC_BIAS_STANDBY: @@ -674,7 +700,7 @@ static int wm8960_set_bias_level_capless(struct snd_soc_codec *codec, enum snd_soc_bias_level level) { struct wm8960_priv *wm8960 = snd_soc_codec_get_drvdata(codec); - int reg; + int reg, ret;
switch (level) { case SND_SOC_BIAS_ON: @@ -715,9 +741,22 @@ static int wm8960_set_bias_level_capless(struct snd_soc_codec *codec, WM8960_VREF, WM8960_VREF);
msleep(100); + + if (!IS_ERR(wm8960->mclk)) { + ret = clk_prepare_enable(wm8960->mclk); + if (ret) { + dev_err(codec->dev, + "Failed to enable MCLK: %d\n", + ret); + return ret; + } + } break;
case SND_SOC_BIAS_ON: + if (!IS_ERR(wm8960->mclk)) + clk_disable_unprepare(wm8960->mclk); + /* Enable anti-pop mode */ snd_soc_update_bits(codec, WM8960_APOP1, WM8960_POBCTRL | WM8960_SOFT_ST | @@ -1002,6 +1041,12 @@ static int wm8960_i2c_probe(struct i2c_client *i2c, if (wm8960 == NULL) return -ENOMEM;
+ wm8960->mclk = devm_clk_get(&i2c->dev, "mclk"); + if (IS_ERR(wm8960->mclk)) { + if (PTR_ERR(wm8960->mclk) == -EPROBE_DEFER) + return -EPROBE_DEFER; + } + wm8960->regmap = devm_regmap_init_i2c(i2c, &wm8960_regmap); if (IS_ERR(wm8960->regmap)) return PTR_ERR(wm8960->regmap);
From: Zidan Wang b50113@freescale.com
wm8960 codec driver missing configure its bit clock and frame clock, so add support for it. It will calculate a appropriate frequency dividing ratio according to the system clock, bit clock and frame clock, then set the corresponding registers.
Signed-off-by: Zidan Wang b50113@freescale.com --- sound/soc/codecs/wm8960.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 97 insertions(+)
diff --git a/sound/soc/codecs/wm8960.c b/sound/soc/codecs/wm8960.c index fd25973..aba6bbf 100644 --- a/sound/soc/codecs/wm8960.c +++ b/sound/soc/codecs/wm8960.c @@ -127,6 +127,8 @@ struct wm8960_priv { struct snd_soc_dapm_widget *out3; bool deemph; int playback_fs; + int bclk; + int sysclk; struct wm8960_data pdata; };
@@ -563,6 +565,68 @@ static struct { { 8000, 5 }, };
+/* Multiply 256 for internal 256 div */ +static const int dac_divs[] = { 256, 384, 512, 768, 1024, 1408, 1536 }; + +/* Multiply 10 to eliminate decimials */ +static const int bclk_divs[] = { + 10, 15, 20, 30, 40, 55, 60, 80, 110, + 120, 160, 220, 240, 320, 320, 320 +}; + +static void wm8960_configure_clocking(struct snd_soc_codec *codec, + int stream, int lrclk) +{ + struct wm8960_priv *wm8960 = snd_soc_codec_get_drvdata(codec); + u16 iface1 = snd_soc_read(codec, WM8960_IFACE1); + u16 iface2 = snd_soc_read(codec, WM8960_IFACE2); + int i, j; + + if (!(iface1 & (1<<6))) { + dev_dbg(codec->dev, + "Codec is slave mode, no need to configure clock\n"); + return; + } + + if (!wm8960->sysclk) { + dev_dbg(codec->dev, "No SYSCLK configured\n"); + return; + } + + if (!wm8960->bclk || !lrclk) { + dev_dbg(codec->dev, "No audio clocks configured\n"); + return; + } + + for (i = 0; i < ARRAY_SIZE(dac_divs); ++i) { + if (wm8960->sysclk == lrclk * dac_divs[i]) { + for (j = 0; j < ARRAY_SIZE(bclk_divs); ++j) { + if (wm8960->sysclk == wm8960->bclk * + bclk_divs[j] / 10) { + goto config_clock; + } + } + } + } + + dev_err(codec->dev, "Unsupported sysclk %d\n", wm8960->sysclk); + return; + +config_clock: + /* configure frame clock. If ADCLRC configure as GPIO pin, DACLRC + * pin is used as a frame clock for ADCs and DACs. + */ + if (iface2 & (1<<6)) + snd_soc_update_bits(codec, WM8960_CLOCK1, 0x7 << 3, i << 3); + else if (SNDRV_PCM_STREAM_PLAYBACK == stream) + snd_soc_update_bits(codec, WM8960_CLOCK1, 0x7 << 3, i << 3); + else if (SNDRV_PCM_STREAM_CAPTURE == stream) + snd_soc_update_bits(codec, WM8960_CLOCK1, 0x7 << 6, i << 6); + + /* configure bit clock */ + snd_soc_update_bits(codec, WM8960_CLOCK2, 0xf, j); +} + static int wm8960_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params, struct snd_soc_dai *dai) @@ -572,6 +636,10 @@ static int wm8960_hw_params(struct snd_pcm_substream *substream, u16 iface = snd_soc_read(codec, WM8960_IFACE1) & 0xfff3; int i;
+ wm8960->bclk = snd_soc_params_to_bclk(params); + if (params_channels(params) == 1) + wm8960->bclk *= 2; + /* bit size */ switch (params_width(params)) { case 16: @@ -602,6 +670,10 @@ static int wm8960_hw_params(struct snd_pcm_substream *substream,
/* set iface */ snd_soc_write(codec, WM8960_IFACE1, iface); + + wm8960_configure_clocking(codec, substream->stream, + params_rate(params)); + return 0; }
@@ -950,6 +1022,30 @@ static int wm8960_set_bias_level(struct snd_soc_codec *codec, return wm8960->set_bias_level(codec, level); }
+static int wm8960_set_dai_sysclk(struct snd_soc_dai *dai, int clk_id, + unsigned int freq, int dir) +{ + struct snd_soc_codec *codec = dai->codec; + struct wm8960_priv *wm8960 = snd_soc_codec_get_drvdata(codec); + + switch (clk_id) { + case WM8960_SYSCLK_MCLK: + snd_soc_update_bits(codec, WM8960_CLOCK1, + 0x1, WM8960_SYSCLK_MCLK); + break; + case WM8960_SYSCLK_PLL: + snd_soc_update_bits(codec, WM8960_CLOCK1, + 0x1, WM8960_SYSCLK_PLL); + break; + default: + return -EINVAL; + } + + wm8960->sysclk = freq; + + return 0; +} + #define WM8960_RATES SNDRV_PCM_RATE_8000_48000
#define WM8960_FORMATS \ @@ -962,6 +1058,7 @@ static const struct snd_soc_dai_ops wm8960_dai_ops = { .set_fmt = wm8960_set_dai_fmt, .set_clkdiv = wm8960_set_dai_clkdiv, .set_pll = wm8960_set_dai_pll, + .set_sysclk = wm8960_set_dai_sysclk, };
static struct snd_soc_dai_driver wm8960_dai = {
On Wed, Jan 07, 2015 at 03:31:45PM +0800, Zidan Wang wrote:
- for (i = 0; i < ARRAY_SIZE(dac_divs); ++i) {
if (wm8960->sysclk == lrclk * dac_divs[i]) {for (j = 0; j < ARRAY_SIZE(bclk_divs); ++j) {if (wm8960->sysclk == wm8960->bclk *bclk_divs[j] / 10) {goto config_clock;}}}- }
- dev_err(codec->dev, "Unsupported sysclk %d\n", wm8960->sysclk);
- return;
It's a bit awkward using the goto like this. A more common way of writing this is to change the above block to be
if (i == ARRAY_SIZE(dac_divs)) /* return error */
rather than skipping over the error. Otherwise this looks good.
On Wed, Jan 14, 2015 at 07:27:03PM +0000, Mark Brown wrote:
On Wed, Jan 07, 2015 at 03:31:45PM +0800, Zidan Wang wrote:
- for (i = 0; i < ARRAY_SIZE(dac_divs); ++i) {
if (wm8960->sysclk == lrclk * dac_divs[i]) {for (j = 0; j < ARRAY_SIZE(bclk_divs); ++j) {if (wm8960->sysclk == wm8960->bclk *bclk_divs[j] / 10) {goto config_clock;}}}- }
- dev_err(codec->dev, "Unsupported sysclk %d\n", wm8960->sysclk);
- return;
It's a bit awkward using the goto like this. A more common way of writing this is to change the above block to be
if (i == ARRAY_SIZE(dac_divs)) /* return error */
rather than skipping over the error. Otherwise this looks good.
Hi Mark,
I found it can't generate bclk for S20_3LE data format.
For 2 channel S20_3LE data format:
bclk = fs * 20 * 2 Sysclk = BCLKDIV * bclk = BCLKDIV * fs * 40 Sysclk = DACDIV * fs * 256
BCLKDIV/DACDIV = 256/40 = 32/5
But BCLKDIV/DACDIV can't be 32/5. So I want to support tdm slot.
bclk = fs * slot_width * slots * channal.
Do you think it make sense, or any other ideas?
Best Regards, Zidan
On Thu, Jan 15, 2015 at 3:34 PM, Zidan Wang b50113@freescale.com wrote:
On Wed, Jan 14, 2015 at 07:27:03PM +0000, Mark Brown wrote:
On Wed, Jan 07, 2015 at 03:31:45PM +0800, Zidan Wang wrote:
- for (i = 0; i < ARRAY_SIZE(dac_divs); ++i) {
if (wm8960->sysclk == lrclk * dac_divs[i]) {for (j = 0; j < ARRAY_SIZE(bclk_divs); ++j) {if (wm8960->sysclk == wm8960->bclk *bclk_divs[j] / 10) {goto config_clock;}}}- }
- dev_err(codec->dev, "Unsupported sysclk %d\n", wm8960->sysclk);
- return;
It's a bit awkward using the goto like this. A more common way of writing this is to change the above block to be
if (i == ARRAY_SIZE(dac_divs)) /* return error */rather than skipping over the error. Otherwise this looks good.
Hi Mark,
I found it can't generate bclk for S20_3LE data format.
For 2 channel S20_3LE data format:
bclk = fs * 20 * 2 Sysclk = BCLKDIV * bclk = BCLKDIV * fs * 40 Sysclk = DACDIV * fs * 256
BCLKDIV/DACDIV = 256/40 = 32/5
But BCLKDIV/DACDIV can't be 32/5. So I want to support tdm slot.
bclk = fs * slot_width * slots * channal.
Do you think it make sense, or any other ideas?
Reviving this question after two years :).
After "ASoC: codec: wm8960: Relax bit clock computation" patch
https://patchwork.kernel.org/patch/9636769/
we can now support S20_3LE for round rates like 8000, 16000, 32000 and 48000.
But not for 11025, 22050, 441000. Do you think it's worth exploring "tdm slot" idea? I don't know exactly what it implies.
Another idea, is to completely remove support for S20_3LE since it is not trivial to derive bitclk from sysclk.
What do you guys think?
Daniel.
On Mon, Apr 03, 2017 at 04:16:23PM +0300, Daniel Baluta wrote:
On Thu, Jan 15, 2015 at 3:34 PM, Zidan Wang b50113@freescale.com wrote:
On Wed, Jan 14, 2015 at 07:27:03PM +0000, Mark Brown wrote:
On Wed, Jan 07, 2015 at 03:31:45PM +0800, Zidan Wang wrote:
I found it can't generate bclk for S20_3LE data format.
For 2 channel S20_3LE data format:
bclk = fs * 20 * 2 Sysclk = BCLKDIV * bclk = BCLKDIV * fs * 40 Sysclk = DACDIV * fs * 256
BCLKDIV/DACDIV = 256/40 = 32/5
But BCLKDIV/DACDIV can't be 32/5. So I want to support tdm slot.
bclk = fs * slot_width * slots * channal.
Do you think it make sense, or any other ideas?
Reviving this question after two years :).
After "ASoC: codec: wm8960: Relax bit clock computation" patch
https://patchwork.kernel.org/patch/9636769/
we can now support S20_3LE for round rates like 8000, 16000, 32000 and 48000.
But not for 11025, 22050, 441000. Do you think it's worth exploring "tdm slot" idea? I don't know exactly what it implies.
Another idea, is to completely remove support for S20_3LE since it is not trivial to derive bitclk from sysclk.
What do you guys think?
Does this problem still remain after the relaxed clock computation? The maths you quote depends on the derived BCLK being exactly the correct speed for the audio, that is no longer the case anymore.
I would have thought the patch would cover both situations, as in if we can produce a suitable LRCLK, then we just pick a BCLK we can produce that is higher than we need. I don't see why that depends on things being a 48k based rate there. Am I missing something?
Thanks, Charles
On Mon, Apr 3, 2017 at 4:34 PM, Charles Keepax ckeepax@opensource.wolfsonmicro.com wrote:
On Mon, Apr 03, 2017 at 04:16:23PM +0300, Daniel Baluta wrote:
On Thu, Jan 15, 2015 at 3:34 PM, Zidan Wang b50113@freescale.com wrote:
On Wed, Jan 14, 2015 at 07:27:03PM +0000, Mark Brown wrote:
On Wed, Jan 07, 2015 at 03:31:45PM +0800, Zidan Wang wrote:
I found it can't generate bclk for S20_3LE data format.
For 2 channel S20_3LE data format:
bclk = fs * 20 * 2 Sysclk = BCLKDIV * bclk = BCLKDIV * fs * 40 Sysclk = DACDIV * fs * 256
BCLKDIV/DACDIV = 256/40 = 32/5
But BCLKDIV/DACDIV can't be 32/5. So I want to support tdm slot.
bclk = fs * slot_width * slots * channal.
Do you think it make sense, or any other ideas?
Reviving this question after two years :).
After "ASoC: codec: wm8960: Relax bit clock computation" patch
https://patchwork.kernel.org/patch/9636769/
we can now support S20_3LE for round rates like 8000, 16000, 32000 and 48000.
But not for 11025, 22050, 441000. Do you think it's worth exploring "tdm slot" idea? I don't know exactly what it implies.
Another idea, is to completely remove support for S20_3LE since it is not trivial to derive bitclk from sysclk.
What do you guys think?
Does this problem still remain after the relaxed clock computation? The maths you quote depends on the derived BCLK being exactly the correct speed for the audio, that is no longer the case anymore.
I would have thought the patch would cover both situations, as in if we can produce a suitable LRCLK, then we just pick a BCLK we
That!
The problem for remaining rates is that we cannot derive the LRCLK
<snip> + for (j = 0; j < ARRAY_SIZE(dac_divs); ++j) { + if (sysclk != dac_divs[j] * lrclk) + continue; </snip>
can produce that is higher than we need. I don't see why that depends on things being a 48k based rate there. Am I missing something?
On Mon, Apr 03, 2017 at 04:39:40PM +0300, Daniel Baluta wrote:
On Mon, Apr 3, 2017 at 4:34 PM, Charles Keepax ckeepax@opensource.wolfsonmicro.com wrote:
On Mon, Apr 03, 2017 at 04:16:23PM +0300, Daniel Baluta wrote: Does this problem still remain after the relaxed clock computation? The maths you quote depends on the derived BCLK being exactly the correct speed for the audio, that is no longer the case anymore.
I would have thought the patch would cover both situations, as in if we can produce a suitable LRCLK, then we just pick a BCLK we
That!
The problem for remaining rates is that we cannot derive the LRCLK
<snip> + for (j = 0; j < ARRAY_SIZE(dac_divs); ++j) { + if (sysclk != dac_divs[j] * lrclk) + continue; </snip>
If you can't generate the LRCLK you either need a different source clock or to use the PLL. You don't want to be trying to pull 44.1k audio over a link that is clocked on a 48k based clock.
Is the problem here that the PLL part of the code is making the same assumption as the direct part of the code was, that the bclk should be exact?
Thanks, Charles
<Removing Zidan from thread because the address no longer exists>
On Mon, Apr 3, 2017 at 4:54 PM, Charles Keepax ckeepax@opensource.wolfsonmicro.com wrote:
On Mon, Apr 03, 2017 at 04:39:40PM +0300, Daniel Baluta wrote:
On Mon, Apr 3, 2017 at 4:34 PM, Charles Keepax ckeepax@opensource.wolfsonmicro.com wrote:
On Mon, Apr 03, 2017 at 04:16:23PM +0300, Daniel Baluta wrote: Does this problem still remain after the relaxed clock computation? The maths you quote depends on the derived BCLK being exactly the correct speed for the audio, that is no longer the case anymore.
I would have thought the patch would cover both situations, as in if we can produce a suitable LRCLK, then we just pick a BCLK we
That!
The problem for remaining rates is that we cannot derive the LRCLK
<snip> + for (j = 0; j < ARRAY_SIZE(dac_divs); ++j) { + if (sysclk != dac_divs[j] * lrclk) + continue; </snip>
If you can't generate the LRCLK you either need a different source clock or to use the PLL. You don't want to be trying to pull 44.1k audio over a link that is clocked on a 48k based clock.
Yup, this makes sense to me.
Is the problem here that the PLL part of the code is making the same assumption as the direct part of the code was, that the bclk should be exact?
Yes.
After wm8960_configure_sysclk fails to find a LRCLK, we try to use the PLL.
Anyhow, here we don't even reach to check if the PLL can be used because there is no solution for the following system:
freq_out = sysclk * sysclk_divs[i]; sysclk = lrclk * dac_divs[j]; sysclk == bclk * bclk_divs[k]
Perhaps, we can also try here to relax bitclk computation like we did for when sysclk was directly derived from mclk.
thanks, Daniel.
On Tue, Apr 04, 2017 at 10:55:00AM +0300, Daniel Baluta wrote:
<Removing Zidan from thread because the address no longer exists>
On Mon, Apr 3, 2017 at 4:54 PM, Charles Keepax ckeepax@opensource.wolfsonmicro.com wrote:
On Mon, Apr 03, 2017 at 04:39:40PM +0300, Daniel Baluta wrote:
On Mon, Apr 3, 2017 at 4:34 PM, Charles Keepax ckeepax@opensource.wolfsonmicro.com wrote:
Is the problem here that the PLL part of the code is making the same assumption as the direct part of the code was, that the bclk should be exact?
Yes.
After wm8960_configure_sysclk fails to find a LRCLK, we try to use the PLL.
Anyhow, here we don't even reach to check if the PLL can be used because there is no solution for the following system:
freq_out = sysclk * sysclk_divs[i]; sysclk = lrclk * dac_divs[j]; sysclk == bclk * bclk_divs[k]
Perhaps, we can also try here to relax bitclk computation like we did for when sysclk was directly derived from mclk.
Exactly that is what I am saying it looks like the PLL part of the process still assumes it requires bclk to be an exact frequency if we relax that, the same way we did for the direct MCLK then we should be good.
Thanks, Charles
On Wed, Jan 07, 2015 at 03:31:44PM +0800, Zidan Wang wrote:
From: Zidan Wang b50113@freescale.com
When we want to use wm8960 codec, we should enable its MCLK in machine driver. It's reasonable for wm8960 codec driver to manage its own MCLK.
Applied, thanks.
participants (5)
-
Charles Keepax -
Daniel Baluta -
Mark Brown -
Zidan Wang -
Zidan Wang