[alsa-devel] [PATCH] ASoC: wm8804: Allow fine-grained control of the PLL generation
Signed-off-by: Daniel Matuschek daniel@matuschek.net
WM8804 can run with PLL frequencies of 256xfs and 128xfs for most sample rates. At 192kHz only 128xfs is supported. The existing driver selects 128xfs automatically for some lower samples rates. By using the "pllid" argument of the "set_pll" function is is now possible to control the behaviour. This allows using 256xfs PLL frequency on all sample rates up to 96kHz. It should allow lower jitter and better signal quality. When pllid=0, the behaviour of the driver does not change.
--- sound/soc/codecs/wm8804.c | 18 +++++++++++++----- sound/soc/codecs/wm8804.h | 7 +++++++ 2 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/sound/soc/codecs/wm8804.c b/sound/soc/codecs/wm8804.c index 1704b1e..e79c408 100644 --- a/sound/soc/codecs/wm8804.c +++ b/sound/soc/codecs/wm8804.c @@ -2,6 +2,8 @@ * wm8804.c -- WM8804 S/PDIF transceiver driver * * Copyright 2010-11 Wolfson Microelectronics plc + * patched by Daniel Matuschek info@crazyaudio.com to allow + * fine-grained control of PLL * * Author: Dimitris Papastamos dp@opensource.wolfsonmicro.com * @@ -318,7 +320,7 @@ static struct {
#define FIXED_PLL_SIZE ((1ULL << 22) * 10) static int pll_factors(struct pll_div *pll_div, unsigned int target, - unsigned int source) + unsigned int source, int mclk_div) { u64 Kpart; unsigned long int K, Ndiv, Nmod, tmp; @@ -332,9 +334,15 @@ static int pll_factors(struct pll_div *pll_div, unsigned int target, tmp = target * post_table[i].div; if (tmp >= 90000000 && tmp <= 100000000) { pll_div->freqmode = post_table[i].freqmode; - pll_div->mclkdiv = post_table[i].mclkdiv; - target *= post_table[i].div; - break; + if ((mclk_div == WM8804_MCLKDIV_DONTCARE) || + ((post_table[i].mclkdiv == 1) && + (mclk_div == WM8804_MCLKDIV_1)) || + ((post_table[i].mclkdiv == 0) && + (mclk_div == WM8804_MCLKDIV_0))) { + pll_div->mclkdiv = post_table[i].mclkdiv; + target *= post_table[i].div; + break; + } } }
@@ -388,7 +396,7 @@ static int wm8804_set_pll(struct snd_soc_dai *dai, int pll_id, int ret; struct pll_div pll_div;
- ret = pll_factors(&pll_div, freq_out, freq_in); + ret = pll_factors(&pll_div, freq_out, freq_in, pll_id); if (ret) return ret;
diff --git a/sound/soc/codecs/wm8804.h b/sound/soc/codecs/wm8804.h index 8ec14f5..0365177 100644 --- a/sound/soc/codecs/wm8804.h +++ b/sound/soc/codecs/wm8804.h @@ -58,4 +58,11 @@
#define WM8804_CLKOUT_DIV 1
+#define WM8804_MCLKDIV_DONTCARE 0 +#define WM8804_MCLKDIV_0 1 +#define WM8804_MCLKDIV_1 2 +#define WM8804_PLL_MCLKDIV_DONTCARE WM8804_MCLKDIV_DONTCARE +#define WM8804_PLL_MCLKDIV_0 WM8804_MCLKDIV_0 +#define WM8804_PLL_MCLKDIV_1 WM8804_MCLKDIV_1 + #endif /* _WM8804_H */
On Wed, Jan 08, 2014 at 10:36:53PM +0100, Daniel Matuschek wrote:
Signed-off-by: Daniel Matuschek daniel@matuschek.net
WM8804 can run with PLL frequencies of 256xfs and 128xfs for most sample rates. At 192kHz only 128xfs is supported. The existing driver selects 128xfs automatically for some lower samples rates. By using the "pllid" argument of the "set_pll" function is is now possible to control the behaviour. This allows using 256xfs PLL frequency on all sample rates up to 96kHz. It should allow lower jitter and better signal quality. When pllid=0, the behaviour of the driver does not change.
Please put the signoff at the end of the commit log like SubmittingPatches says. The formatting of the log message is also *very* odd, the first line is really short for some reason and everything is indented by a space. In general your commit message shouldn't stand out from others when viewed with git log.
- wm8804.c -- WM8804 S/PDIF transceiver driver
- Copyright 2010-11 Wolfson Microelectronics plc
- patched by Daniel Matuschek info@crazyaudio.com to allow
- fine-grained control of PLL
We have git history, we don't need changelogs in the driver too. Adding a copyright statement would be OK (though most people don't bother).
static int pll_factors(struct pll_div *pll_div, unsigned int target,
unsigned int source)
unsigned int source, int mclk_div)
Here you call this mclk_div...
pll_div->mclkdiv = post_table[i].mclkdiv;target *= post_table[i].div;break;
if ((mclk_div == WM8804_MCLKDIV_DONTCARE) ||((post_table[i].mclkdiv == 1) &&(mclk_div == WM8804_MCLKDIV_1)) ||((post_table[i].mclkdiv == 0) &&(mclk_div == WM8804_MCLKDIV_0))) {pll_div->mclkdiv = post_table[i].mclkdiv;target *= post_table[i].div;break;}
This logic is really hard to read, it's five lines of if statement with multiple levels of brackets indented to the same level as the following statements. I'd suggest either a series of if statements with continues or something like
if (mclk_div == WM8804_MCLKDIV_DONTCARE || mclk_div - 1 == post_table[i].mclkdiv) {
instead.
@@ -388,7 +396,7 @@ static int wm8804_set_pll(struct snd_soc_dai *dai, int pll_id, int ret; struct pll_div pll_div;
ret = pll_factors(&pll_div, freq_out, freq_in);
ret = pll_factors(&pll_div, freq_out, freq_in, pll_id);
...but here it's pll_id.
+#define WM8804_MCLKDIV_DONTCARE 0 +#define WM8804_MCLKDIV_0 1 +#define WM8804_MCLKDIV_1 2 +#define WM8804_PLL_MCLKDIV_DONTCARE WM8804_MCLKDIV_DONTCARE +#define WM8804_PLL_MCLKDIV_0 WM8804_MCLKDIV_0 +#define WM8804_PLL_MCLKDIV_1 WM8804_MCLKDIV_1
Why are there two different sets of constants with the same values being added here, the _PLL versions don't seem to be referenced?
On Wed, Jan 08, 2014 at 10:36:53PM +0100, Daniel Matuschek wrote:
Signed-off-by: Daniel Matuschek daniel@matuschek.net
<snip>
pll_div->freqmode = post_table[i].freqmode;
pll_div->mclkdiv = post_table[i].mclkdiv;target *= post_table[i].div;break;
if ((mclk_div == WM8804_MCLKDIV_DONTCARE) ||((post_table[i].mclkdiv == 1) &&(mclk_div == WM8804_MCLKDIV_1)) ||((post_table[i].mclkdiv == 0) &&(mclk_div == WM8804_MCLKDIV_0))) {
Would probably be nicer to update the post_table to use the new defines and directly compare.
pll_div->mclkdiv = post_table[i].mclkdiv;target *= post_table[i].div;break;}@@ -388,7 +396,7 @@ static int wm8804_set_pll(struct snd_soc_dai *dai, int pll_id, int ret; struct pll_div pll_div;
ret = pll_factors(&pll_div, freq_out, freq_in);
ret = pll_factors(&pll_div, freq_out, freq_in, pll_id);
This does feel like a slight abuse of pll_id, it feels to me that using the set_clkdiv callback would be a little more natural from a user perspective.
if (ret) return ret;diff --git a/sound/soc/codecs/wm8804.h b/sound/soc/codecs/wm8804.h index 8ec14f5..0365177 100644 --- a/sound/soc/codecs/wm8804.h +++ b/sound/soc/codecs/wm8804.h @@ -58,4 +58,11 @@
#define WM8804_CLKOUT_DIV 1
+#define WM8804_MCLKDIV_DONTCARE 0 +#define WM8804_MCLKDIV_0 1 +#define WM8804_MCLKDIV_1 2 +#define WM8804_PLL_MCLKDIV_DONTCARE WM8804_MCLKDIV_DONTCARE +#define WM8804_PLL_MCLKDIV_0 WM8804_MCLKDIV_0 +#define WM8804_PLL_MCLKDIV_1 WM8804_MCLKDIV_1
Do we really need two copies of these?
Thanks, Charles
On 09.01.2014, at 15:27, Charles Keepax ckeepax@opensource.wolfsonmicro.com wrote:
On Wed, Jan 08, 2014 at 10:36:53PM +0100, Daniel Matuschek wrote:
Signed-off-by: Daniel Matuschek daniel@matuschek.net
<snip>
pll_div->freqmode = post_table[i].freqmode;
pll_div->mclkdiv = post_table[i].mclkdiv;target *= post_table[i].div;break;
if ((mclk_div == WM8804_MCLKDIV_DONTCARE) ||((post_table[i].mclkdiv == 1) &&(mclk_div == WM8804_MCLKDIV_1)) ||((post_table[i].mclkdiv == 0) &&(mclk_div == WM8804_MCLKDIV_0))) {Would probably be nicer to update the post_table to use the new defines and directly compare.
Yes, this is an option.
pll_div->mclkdiv = post_table[i].mclkdiv;target *= post_table[i].div;break;}@@ -388,7 +396,7 @@ static int wm8804_set_pll(struct snd_soc_dai *dai, int pll_id, int ret; struct pll_div pll_div;
ret = pll_factors(&pll_div, freq_out, freq_in);
ret = pll_factors(&pll_div, freq_out, freq_in, pll_id);This does feel like a slight abuse of pll_id, it feels to me that using the set_clkdiv callback would be a little more natural from a user perspective.
Good idea, I will move it to set_clkdiv.
if (ret) return ret;diff --git a/sound/soc/codecs/wm8804.h b/sound/soc/codecs/wm8804.h index 8ec14f5..0365177 100644 --- a/sound/soc/codecs/wm8804.h +++ b/sound/soc/codecs/wm8804.h @@ -58,4 +58,11 @@
#define WM8804_CLKOUT_DIV 1
+#define WM8804_MCLKDIV_DONTCARE 0 +#define WM8804_MCLKDIV_0 1 +#define WM8804_MCLKDIV_1 2 +#define WM8804_PLL_MCLKDIV_DONTCARE WM8804_MCLKDIV_DONTCARE +#define WM8804_PLL_MCLKDIV_0 WM8804_MCLKDIV_0 +#define WM8804_PLL_MCLKDIV_1 WM8804_MCLKDIV_1
Do we really need two copies of these?
Not really ;-) I will remove the duplicates.
On Thu, Jan 9, 2014 at 7:29 AM, Daniel Matuschek daniel@matuschek.net wrote:
On 09.01.2014, at 15:27, Charles Keepax ckeepax@opensource.wolfsonmicro.com wrote:
On Wed, Jan 08, 2014 at 10:36:53PM +0100, Daniel Matuschek wrote:
Signed-off-by: Daniel Matuschek daniel@matuschek.net
<snip>
pll_div->freqmode = post_table[i].freqmode;
pll_div->mclkdiv = post_table[i].mclkdiv;target *= post_table[i].div;break;
if ((mclk_div == WM8804_MCLKDIV_DONTCARE) ||((post_table[i].mclkdiv == 1) &&(mclk_div == WM8804_MCLKDIV_1)) ||((post_table[i].mclkdiv == 0) &&(mclk_div == WM8804_MCLKDIV_0))) {Would probably be nicer to update the post_table to use the new defines and directly compare.
How about giving the defines better names? Like WM8804_MCLKDIV_256x for 0. Assuming that's the value for 256x, I can't actually tell. Which is why the define name could be better.
This does feel like a slight abuse of pll_id, it feels to me that using the set_clkdiv callback would be a little more natural from a user perspective.
Good idea, I will move it to set_clkdiv.
Why does it need to be an option? If 256x is better, then why not always use it? Maybe the code to select the divisor should be better?
If we look at the post_table: static struct { unsigned int div; unsigned int freqmode; unsigned int mclkdiv; } post_table[] = { { 2, 0, 0 }, { 4, 0, 1 }, { 4, 1, 0 }, { 8, 1, 1 }, { 8, 2, 0 }, { 16, 2, 1 }, { 12, 3, 0 }, { 24, 3, 1 } };
The code loops through that in order to find the first div that when multiplied by the target rate gives 90-100 MHz. So if the first div=4 in the table doesn't work, why would the second 4 work? It looks like it's just doing unnecessary tries to re-reject the same divisor reached by different freqmode & mclkdiv combinations.
Since it stops at the first divisor that works, won't it always use mclkdiv=1? If mclkdiv=0 is better, why not just list those first/only in the table so they get used?
BTW, if ((K % 10) >= 5) K += 5; K /= 10;
Worst Division With Rounding Ever. And don't even get me started on the base 10 fixed point on a binary computer.
On Thu, Jan 09, 2014 at 09:14:07AM -0800, Trent Piepho wrote:
On Thu, Jan 9, 2014 at 7:29 AM, Daniel Matuschek daniel@matuschek.net wrote:
Daniel, please take more care with the CC lists when posting things - there's nothing to do with device tree in this patch but you've CCed the DT list and some DT people wich means more noise in their mailbox.
Why does it need to be an option? If 256x is better, then why not always use it? Maybe the code to select the divisor should be better?
It's typically a power/performance tradeoff, though for this part I can't see anyone caring about power.
Since it stops at the first divisor that works, won't it always use mclkdiv=1? If mclkdiv=0 is better, why not just list those first/only in the table so they get used?
This seems like the best idea. I suspect the table just got typed in from the datasheet.
participants (4)
-
Charles Keepax -
Daniel Matuschek -
Mark Brown -
Trent Piepho