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

[Mark Brown]

On Fri, Jan 22, 2010 at 05:27:53PM +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>

A few issues below remaining, mostly to do with the DAPM updates.

> 1. thanks for the header, Mark, but unfortunately it contains errors 
>    (duplicate register names, duplicate and wrong bitfield names)

Discussion on IRC revealed that this was a case of a small number of
typos and stuff that can be quickly fixed with indent :/

> 3. I am still not convinced, that macro names like WM8978_WL or 
>    WM8978_DLRSWAP or WM8978_MS or... better describe the meaning of the 
>    bitfield than respective comment in the source. Here's an example of a 
>    comment from this patch:

> 	/* bit 3: enable bias, bit 2: enable I/O tie off buffer */
> 	power1 |= 0xc;

The comments and bitfield names do different things here; having words
is useful to explain what you're trying to accomplish while using
symbolic bitmasks is for connecting the bitbashing with the datasheet
when the datasheet names the bitfields.

> +static const int update_reg[] = {
> +	0xb, 0xc, 0xf, 0x10, 0x2d, 0x2e, 0x34, 0x35, 0x36, 0x37
> +};

It would be really helpful to use the symbolic names for the registers
here.  It'd also be nice to move this closer to the point where it's
used or have a comment explaining what will happen with it.

> +static const SOC_ENUM_SINGLE_DECL(adc_compand, WM8978_COMPANDING_CONTROL, 1, wm8978_companding);
> +static const SOC_ENUM_SINGLE_DECL(dac_compand, WM8978_COMPANDING_CONTROL, 3, wm8978_companding);

Line wrapping please.

> +	SOC_SINGLE_TLV("Left PCM Volume",
> +		WM8978_LEFT_DAC_DIGITAL_VOLUME, 0, 255, 0, digital_tlv),
> +	SOC_SINGLE_TLV("Right PCM Volume",
> +		WM8978_RIGHT_DAC_DIGITAL_VOLUME, 0, 255, 0, digital_tlv),

This should be a single stereo (_DOUBLE) control.

> +	SOC_SINGLE_TLV("Left ADC Volume",
> +	       WM8978_LEFT_ADC_DIGITAL_VOLUME, 0, 255, 0, digital_tlv),
> +	SOC_SINGLE_TLV("Right ADC Volume",
> +	       WM8978_RIGHT_ADC_DIGITAL_VOLUME, 0, 255, 0, digital_tlv),

As should this.

> +	SOC_SINGLE_TLV("Left Headphone Playback Volume",
> +		WM8978_LOUT1_HP_CONTROL, 0, 63, 0, spk_tlv),

These also, and the speaker ones too.

> +/* Mixer #3: Boost (Input) mixer */
> +static const struct snd_kcontrol_new wm8978_left_boost_mixer[] = {
> +
> +	SOC_DAPM_SINGLE("PGA Boost (+20dB)",
> +		WM8978_LEFT_ADC_BOOST_CONTROL, 8, 1, 0),

This isn't actually a mute so the connection to the PGA should (unless
there's something extra going on) be unconditional and this should be a
non-DAPM switch.

> +/* Input PGA volume */
> +static const struct snd_kcontrol_new wm8978_left_input_pga_volume[] = {
> +	SOC_DAPM_SINGLE_TLV("Volume",
> +		WM8978_LEFT_INP_PGA_CONTROL, 0, 63, 0, inpga_tlv),
> +};
> +static const struct snd_kcontrol_new wm8978_right_input_pga_volume[] = {
> +	SOC_DAPM_SINGLE_TLV("Volume",
> +		WM8978_RIGHT_INP_PGA_CONTROL, 0, 63, 0, inpga_tlv),
> +};

It's just as well to take these off the PGAs, there's unlikely to be any
actual benefit from handling them with the PGAs on a modern CODEC.

> +/* Headphone */
> +static const struct snd_kcontrol_new wm8978_left_hp_mute[] = {
> +	SOC_DAPM_SINGLE("Mute Switch", WM8978_LOUT1_HP_CONTROL, 6, 1, 1),
> +};
> +static const struct snd_kcontrol_new wm8978_right_hp_mute[] = {
> +	SOC_DAPM_SINGLE("Mute Switch", WM8978_ROUT1_HP_CONTROL, 6, 1, 1),
> +};

Mute switches should always be just "Switch" - ALSA will map "Foo
Switch" and "Foo Volume" together to create a single control in the UI.

These (and the other output nodes) should really be stereo controls
outside of DAPM.  Managing them within DAPM as switches means that if
you mute the output then the path will be powered down.  This is
normally unhelpful since power up and down during playback can produce
audible effects for the listener such as pops and clicks or audible
volume ramps as the power comes on and off.  It's also possible that
some output will leak through when the output is powered down since
normally mutes on PGAs only work when they are powered.

> +	if (freq_in == 0 || freq_out == 0) {
> +		/* Clock CODEC directly from MCLK */
> +		reg = snd_soc_read(codec, WM8978_CLOCKING);
> +		snd_soc_write(codec, WM8978_CLOCKING, reg & ~0x100);
> +
> +		/* Turn off PLL */
> +		reg = snd_soc_read(codec, WM8978_POWER_MANAGEMENT_1);
> +		snd_soc_write(codec, WM8978_POWER_MANAGEMENT_1, reg & ~0x20);
> +		return 0;
> +	}

Note that ideally the PLL updates will handle the case where the PLL is
already on and power down the PLL while reconfiguring it, though it's
not essential.

> +	opclk_div = ((snd_soc_read(codec, WM8978_GPIO_CONTROL) & 0x30) >> 4) + 1;

> +	wm8978->f_pllout = freq_out * opclk_div;

Hrm.  I fear that there's a bit of confusion here - the PLL output is
used separately as the source for both OPCLK and the system clock for
the CODEC.  This means that the PLL output frequency that we need to
refer to later on is that before the OPCLK divide.

Looking further down the driver f_pllout is also being used as the
system master clock frequency, meaning that the driver will only work
when the PLL is in use.  Normally what would happen with this stuff is
that the system clock used by hw_params() will be set using the
set_sysclk() API call, which can tell the driver what the clock rate is
and also select between multiple clock sources.  You'd end up saying
something like:

	snd_soc_dai_set_pll(dai, 0, 0, 12000000, 256 * params_rate(params));
	snd_soc_dai_set_sysclk(dai, WM8978_PLL, 256 * params_rate(params), 0);

in the machine driver (note that this also means that the switchover to
use of the PLL as the clock would then be managed via set_sysclk() not
set_pll()).