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.