[PATCH] ASoC: wm8804: Allow fine-grained control of the PLL generation

[PATCH] ASoC: wm8804: Allow fine-grained control of the PLL generation

[Daniel Matuschek]

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 */
-- 
1.7.9.5

Re: [PATCH] ASoC: wm8804: Allow fine-grained control of the PLL generation

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3005 bytes --]

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?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [alsa-devel] [PATCH] ASoC: wm8804: Allow fine-grained control of the PLL generation

[Charles Keepax]

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

Re: [alsa-devel] [PATCH] ASoC: wm8804: Allow fine-grained control of the PLL generation

[Daniel Matuschek]

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.

Re: [alsa-devel] [PATCH] ASoC: wm8804: Allow fine-grained control of the PLL generation

[Trent Piepho]

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.

Re: [alsa-devel] [PATCH] ASoC: wm8804: Allow fine-grained control of the PLL generation

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 891 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]