On Wed, Jan 08, 2014 at 10:36:53PM +0100, Daniel Matuschek wrote: > Signed-off-by: Daniel Matuschek <[email protected]> > > 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 <[email protected]> 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?
signature.asc
Description: Digital signature
