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

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?