[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 at matuschek.net> wrote:
> On 09.01.2014, at 15:27, Charles Keepax <ckeepax at opensource.wolfsonmicro.com> wrote:
>
>> On Wed, Jan 08, 2014 at 10:36:53PM +0100, Daniel Matuschek wrote:
>>> Signed-off-by: Daniel Matuschek <daniel at 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.