[alsa-devel] [PATCH 1/4] ASoC: add a WM8978 codec driver

[Guennadi Liakhovetski]

On Tue, 19 Jan 2010, Liam Girdwood wrote:

> Looks ok, some questions below.
> 
> On Tue, 2010-01-19 at 09:08 +0100, Guennadi Liakhovetski wrote:
> > The WM8978 codec from Wolfson Microelectronics is very similar to wm8974, but
> > is stereo and also has some differences in pin configuration and internal
> > signal routing. This driver is based on wm8974 and takes the differences into
> > account.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski at gmx.de>
> > ---

[snip]

> > +/* OUT1 - HeadPhones */
> > +SOC_SINGLE("Left HeadPhone Playback ZC Switch",
> > +	   WM8978_LOUT1_HP_CONTROL, 7, 1, 0),
> 
> HeadPhone is usually "Headphone"

Ok

> > +#define MIXER_ARRAY(n, r, s, i, m) SND_SOC_DAPM_MIXER(n, r, s, i, \
> > +							m, ARRAY_SIZE(m))
> 
> I'd be tempted to rename this and add to soc-dapm.h

Please, see my reply to Mark's review.

> > +static void pll_factors(struct wm8978_pll_div *pll_div, unsigned int target,
> > +			unsigned int source)
> > +{
> > +	u64 Kpart;
> > +	unsigned int K, Ndiv, Nmod;
> > +
> > +	Ndiv = target / source;
> > +	if (Ndiv < 6) {
> > +		source >>= 1;
> > +		pll_div->div2 = 1;
> > +		Ndiv = target / source;
> > +	} else {
> > +		pll_div->div2 = 0;
> > +	}
> > +
> 
> I would personally not put the extra { here

That's also what I did a couple of years ago, but this contradicts the 
kernel CodingStyle, and, I think, checkpatch would complain too.

> > +	snd_soc_write(codec, WM8978_AUDIO_INTERFACE, iface_ctl);
> > +	snd_soc_write(codec, WM8978_ADDITIONAL_CONTROL, add_ctl);
> > +
> > +	/* Mic bias */
> > +	snd_soc_write(codec, WM8978_POWER_MANAGEMENT_1,
> > +		      (snd_soc_read(codec, 1) & ~4) | 0x10);
> > +
> > +	/* Out-1 enabled, left/right input channel enabled */
> > +	snd_soc_write(codec, WM8978_POWER_MANAGEMENT_2, 0x1bf);
> > +
> > +	/* Out-2 disabled, right/left output channel enabled, dac enabled */
> > +	snd_soc_write(codec, WM8978_POWER_MANAGEMENT_3, 0x10f);
> 
> Power stuff should be in either dapm or bias functions.

Yes, still working on this.

> > +#define WM8978_LOUT2_SPK_CONTROL		0x36
> > +#define WM8978_ROUT2_SPK_CONTROL		0x37
> > +#define WM8978_OUT3_MIXER_CONTROL		0x38
> > +#define WM8978_OUT4_MIXER_CONTROL		0x39
> > +
> > +#define WM8978_CACHEREGNUM			58
> > +
> 
> It would be nice to have the relevant bits defined here for set_fmt()
> etc instead of just the magic numbers used in the above codec driver. 

As I explained privately, I agree, that using names instead of bits helps 
- but (mostly) only where those bits are reused multiple times in the 
code. If you only have to initialise a register once with some bitmask, I 
think, code like

	/* Enable input X, output Y, set default W polarity to Z */
	__raw_writel(0x123, reg);

looks better than

	__raw_writel(CHIP_INPUT_X_ENABLE | CHIP_OUTPUT_Y_ENABLE | 
			CHIP_SIGNAL_W_POLARITY_Z, reg);

so, unless there strong preferences in ALSA world, I'll try to combine 
both. Let me know if this contradicts the common ALSA style.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/